[PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Hoan Tran]

This patch adds hardware temperature and power reading support for
APM X-Gene SoC using the mailbox communication interface.

Signed-off-by: Hoan Tran 
---
 Documentation/hwmon/xgene-hwmon |  30 ++
 drivers/hwmon/Kconfig   |   7 +
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/xgene-hwmon.c | 755 
 4 files changed, 793 insertions(+)
 create mode 100644 Documentation/hwmon/xgene-hwmon
 create mode 100644 drivers/hwmon/xgene-hwmon.c

diff --git a/Documentation/hwmon/xgene-hwmon b/Documentation/hwmon/xgene-hwmon
new file mode 100644
index 000..6ec50ed
--- /dev/null
+++ b/Documentation/hwmon/xgene-hwmon
@@ -0,0 +1,30 @@
+Kernel driver xgene-hwmon
+
+
+Supported chips:
+ * APM X-Gene SoC
+
+Description
+---
+
+This driver adds hardware temperature and power reading support for
+APM X-Gene SoC using the mailbox communication interface.
+For device tree, it is the standard DT mailbox.
+For ACPI, it is the PCC mailbox.
+
+The following sensors are supported
+
+  * Temperature
+- SoC on-die temperature in milli-degree C
+- Alarm when high/over temperature occurs
+  * Power
+- CPU power in uW
+- IO power in uW
+
+sysfs-Interface
+---
+
+temp0_input - SoC on-die temperature (milli-degree C)
+temp0_critical_alarm - An 1 would indicates on-die temperature exceeded 
threshold
+power0_input - CPU power in (uW)
+power1_input - IO power in (uW)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ff94007..55218c6 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1787,6 +1787,13 @@ config SENSORS_ULTRA45
  This driver provides support for the Ultra45 workstation environmental
  sensors.
 
+config SENSORS_XGENE
+   tristate "APM X-Gene SoC hardware monitoring driver"
+   depends on XGENE_SLIMPRO_MBOX || PCC
+   help
+ If you say yes here you get support for the temperature
+ and power sensors for APM X-Gene SoC.
+
 if ACPI
 
 comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 2ef5b7c..a2460dd 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -162,6 +162,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o
 obj-$(CONFIG_SENSORS_W83L786NG)+= w83l786ng.o
 obj-$(CONFIG_SENSORS_WM831X)   += wm831x-hwmon.o
 obj-$(CONFIG_SENSORS_WM8350)   += wm8350-hwmon.o
+obj-$(CONFIG_SENSORS_XGENE)+= xgene-hwmon.o
 
 obj-$(CONFIG_PMBUS)+= pmbus/
 
diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
new file mode 100644
index 000..c263e7e
--- /dev/null
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -0,0 +1,755 @@
+/*
+ * APM X-Gene SoC Hardware Monitoring Driver
+ *
+ * Copyright (c) 2016, Applied Micro Circuits Corporation
+ * Author: Loc Ho 
+ * Hoan Tran 
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ *
+ * This driver provides the following features:
+ *  - Retrieve CPU total power (uW)
+ *  - Retrieve IO total power (uW)
+ *  - Retrieve SoC temperature (milli-degree C) and alarm
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* SLIMpro message defines */
+#define MSG_TYPE_DBG   0
+#define MSG_TYPE_ERR   7
+#define MSG_TYPE_PWRMGMT   9
+
+#define MSG_TYPE(v)(((v) & 0xF000) >> 28)
+#define MSG_TYPE_SET(v)(((v) << 28) & 0xF000)
+#define MSG_SUBTYPE(v) (((v) & 0x0F00) >> 24)
+#define MSG_SUBTYPE_SET(v) (((v) << 24) & 0x0F00)
+
+#define DBG_SUBTYPE_SENSOR_READ4
+#define SENSOR_RD_MSG  0x04FFE902
+#define SENSOR_RD_EN_ADDR(a)   ((a) & 0x000F)
+#define PMD_PWR_REG0x20
+#define PMD_PWR_MW_REG 0x26
+#define SOC_PWR_REG0x21
+#define SOC_PWR_MW_REG 0x27
+#define SOC_TEMP_REG   0x10
+
+#define TEMP_NEGATIVE_BIT  8
+#define SENSOR_INVALID_DATABIT(15)
+
+#define PWRMGMT_SUBTYPE_TPC1
+#define TPC_ALARM  2
+#define TPC_GET_ALARM  3
+#define TPC_CMD(v) (((v) & 0x00FF) >> 16)
+#define TPC_CMD_SET(v)

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Guenter Roeck]

On Thu, Jul 21, 2016 at 01:55:56PM -0700, Hoan Tran wrote:
> This patch adds hardware temperature and power reading support for
> APM X-Gene SoC using the mailbox communication interface.
> 
> Signed-off-by: Hoan Tran 
> ---

[ ... ]

> +
> + dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
> +
> + return rc;
> +

Nitpick: rc == 0 here, so return 0; is a better choice.
Otherwise looks good.

Reviewed-by: Guenter Roeck 

Note that I can not apply the patch at this time due to its dependency.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Hoan Tran]

Hi Guenter,

On Thu, Jul 21, 2016 at 3:09 PM, Guenter Roeck  wrote:
> On Thu, Jul 21, 2016 at 01:55:56PM -0700, Hoan Tran wrote:
>> This patch adds hardware temperature and power reading support for
>> APM X-Gene SoC using the mailbox communication interface.
>>
>> Signed-off-by: Hoan Tran 
>> ---
>
> [ ... ]
>
>> +
>> + dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
>> +
>> + return rc;
>> +
>
> Nitpick: rc == 0 here, so return 0; is a better choice.
> Otherwise looks good.

Yes, I'll fix it then send v4 shortly.

>
> Reviewed-by: Guenter Roeck 
>
> Note that I can not apply the patch at this time due to its dependency.

Thanks ! I'll let you know when Rafael applies that dependent patch
into linux-next.

Thanks
Hoan

>
> Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[kbuild test robot]

Hi Hoan,

[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v4.7]
[cannot apply to next-20160801]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Hoan-Tran/hwmon-xgene-Add-support-for-X-Gene-hwmon-driver/20160725-015356
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git 
hwmon-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/hwmon/xgene-hwmon.c:38:22: fatal error: acpi/pcc.h: No such file or 
>> directory
   compilation terminated.

vim +38 drivers/hwmon/xgene-hwmon.c

32  #include 
33  #include 
34  #include 
35  #include 
36  #include 
37  #include 
  > 38  #include 
39  
40  /* SLIMpro message defines */
41  #define MSG_TYPE_DBG0

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Hoan Tran]

Hi,

On Mon, Aug 1, 2016 at 6:21 AM, kbuild test robot  wrote:
> Hi Hoan,
>
> [auto build test ERROR on hwmon/hwmon-next]
> [also build test ERROR on v4.7]
> [cannot apply to next-20160801]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Hoan-Tran/hwmon-xgene-Add-support-for-X-Gene-hwmon-driver/20160725-015356
> base:   
> https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git 
> hwmon-next
> config: arm64-allmodconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
> reproduce:
> wget 
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
>  -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm64
>
> All errors (new ones prefixed by >>):
>
>>> drivers/hwmon/xgene-hwmon.c:38:22: fatal error: acpi/pcc.h: No such file or 
>>> directory
>compilation terminated.

This driver should be built with kernel 4.8 and above where the
"pcc.h" file is available.

Thanks
Hoan

>
> vim +38 drivers/hwmon/xgene-hwmon.c
>
> 32  #include 
> 33  #include 
> 34  #include 
> 35  #include 
> 36  #include 
> 37  #include 
>   > 38  #include 
> 39
> 40  /* SLIMpro message defines */
> 41  #define MSG_TYPE_DBG0
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Arnd Bergmann]

On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> +   ctx->comm_base_addr = cppc_ss->base_address;
> +   if (ctx->comm_base_addr) {
> +   ctx->pcc_comm_addr =
> +   acpi_os_ioremap(ctx->comm_base_addr,
> +   cppc_ss->length);
> 

This causes the arm64 allmodconfig build to fail now, according to
kernelci:

  1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

Should this perhaps call ioremap() or memremap() instead?

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Guenter Roeck]

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > +   ctx->comm_base_addr = cppc_ss->base_address;
> > +   if (ctx->comm_base_addr) {
> > +   ctx->pcc_comm_addr =
> > +   acpi_os_ioremap(ctx->comm_base_addr,
> > +   cppc_ss->length);
> > 
> 
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
> 
>   1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
How do you even get there ? arm64:allmodconfig fails for me in -next with

drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no member 
named ‘chips’

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Guenter Roeck]

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > +   ctx->comm_base_addr = cppc_ss->base_address;
> > +   if (ctx->comm_base_addr) {
> > +   ctx->pcc_comm_addr =
> > +   acpi_os_ioremap(ctx->comm_base_addr,
> > +   cppc_ss->length);
> > 
> 
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
> 
>   1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> Should this perhaps call ioremap() or memremap() instead?
> 
Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
mark reserved memblock regions explicitly in iomem") starts using a function
in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
is declared as __init_memblock, which makes me really uncomfortable.
If acpi_os_ioremap() must not be used by modules, and possibly only during
early (?) initialization, maybe its declaration should state those limitations ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +   ctx->comm_base_addr = cppc_ss->base_address;
> > > +   if (ctx->comm_base_addr) {
> > > +   ctx->pcc_comm_addr =
> > > +   
> > > acpi_os_ioremap(ctx->comm_base_addr,
> > > +   cppc_ss->length);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >   1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] 
> > undefined!
> > 
> > Should this perhaps call ioremap() or memremap() instead?
> > 
> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> mark reserved memblock regions explicitly in iomem") starts using a function
> in acpi_os_ioremap() which is not exported. On top of that, 
> memblock_is_memory()
> is declared as __init_memblock, which makes me really uncomfortable.
> If acpi_os_ioremap() must not be used by modules, and possibly only during
> early (?) initialization, maybe its declaration should state those 
> limitations ?

Ah, I didn't notice that. I guess both patches were correct individually and
got added to linux-next around the same time but caused allmodconfig to blow up
when used together.

Adding everyone who was involved in the memblock patch to Cc here, maybe one
of them has an idea what the correct fix is. There are only two other drivers
using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
that drivers are not actually supposed to use this symbol. Making
acpi_os_ioremap() an exported function in arm64 would also work.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:27:54 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +   ctx->comm_base_addr = cppc_ss->base_address;
> > > +   if (ctx->comm_base_addr) {
> > > +   ctx->pcc_comm_addr =
> > > +   
> > > acpi_os_ioremap(ctx->comm_base_addr,
> > > +   cppc_ss->length);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >   1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] 
> > undefined!
> > 
> How do you even get there ? arm64:allmodconfig fails for me in -next with
> 
> drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
> drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no 
> member named ‘chips’

That was fixed in yesterday's linux-next.

Arnd

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[James Morse]

Hi,

On 08/09/16 09:14, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
 +   ctx->comm_base_addr = cppc_ss->base_address;
 +   if (ctx->comm_base_addr) {
 +   ctx->pcc_comm_addr =
 +   
 acpi_os_ioremap(ctx->comm_base_addr,
 +   cppc_ss->length);

>>>
>>> This causes the arm64 allmodconfig build to fail now, according to
>>> kernelci:
>>>
>>>   1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] 
>>> undefined!
>>>
>>> Should this perhaps call ioremap() or memremap() instead?
>>>
>> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 
>> ("arm64:
>> mark reserved memblock regions explicitly in iomem") starts using a function
>> in acpi_os_ioremap() which is not exported. On top of that, 
>> memblock_is_memory()
>> is declared as __init_memblock, which makes me really uncomfortable.
>> If acpi_os_ioremap() must not be used by modules, and possibly only during
>> early (?) initialization, maybe its declaration should state those 
>> limitations ?
> 
> Ah, I didn't notice that. I guess both patches were correct individually and
> got added to linux-next around the same time but caused allmodconfig to blow 
> up
> when used together.
> 
> Adding everyone who was involved in the memblock patch to Cc here, maybe one
> of them has an idea what the correct fix is. There are only two other drivers
> using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
> that drivers are not actually supposed to use this symbol. Making
> acpi_os_ioremap() an exported function in arm64 would also work.

You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
already EXPORT_SYMBOL_GPL().

(I'm still waiting for allmodconfig on linux-next to finish building)


Thanks,

James


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Guenter Roeck]

On 09/08/2016 01:15 AM, Arnd Bergmann wrote:

On Wednesday, September 7, 2016 3:27:54 PM CEST Guenter Roeck wrote:

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:

On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:

+   ctx->comm_base_addr = cppc_ss->base_address;
+   if (ctx->comm_base_addr) {
+   ctx->pcc_comm_addr =
+   acpi_os_ioremap(ctx->comm_base_addr,
+   cppc_ss->length);



This causes the arm64 allmodconfig build to fail now, according to
kernelci:

  1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!


How do you even get there ? arm64:allmodconfig fails for me in -next with

drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no member 
named ‘chips’


That was fixed in yesterday's linux-next.



Maybe, but now I get

Building arm:allmodconfig ... failed
--
Error log:
crypto/crypto_engine.c: In function 'crypto_transfer_hash_request':
crypto/crypto_engine.c:234:3: error: implicit declaration of function 
'queue_kthread_work'

Building arm64:allmodconfig ... failed
--
Error log:
arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c: In function 
‘kvm_timer_hyp_init’:
arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:457:1: warning: label 
‘out_free’ defined but not used

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Arnd Bergmann]

On Thursday, September 8, 2016 7:55:44 AM CEST Guenter Roeck wrote:
> Maybe, but now I get
> 
> Building arm:allmodconfig ... failed
> --
> Error log:
> crypto/crypto_engine.c: In function 'crypto_transfer_hash_request':
> crypto/crypto_engine.c:234:3: error: implicit declaration of function 
> 'queue_kthread_work'
> 
> Building arm64:allmodconfig ... failed
> --
> Error log:
> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c: In function 
> ‘kvm_timer_hyp_init’:
> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:457:1: warning: label 
> ‘out_free’ defined but not used

Right, I sent fixes for both. I think we had a clean build yesterday,
probably tomorrow those will both be fixed.

The time between -rc5 and -rc7 seems to be the worst for build testing ;-)

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[AKASHI Takahiro]

On Thu, Sep 08, 2016 at 11:47:59AM +0100, James Morse wrote:
> Hi,
> 
> On 08/09/16 09:14, Arnd Bergmann wrote:
> > On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> >> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> >>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>  +   ctx->comm_base_addr = cppc_ss->base_address;
>  +   if (ctx->comm_base_addr) {
>  +   ctx->pcc_comm_addr =
>  +   
>  acpi_os_ioremap(ctx->comm_base_addr,
>  +   cppc_ss->length);
> 
> >>>
> >>> This causes the arm64 allmodconfig build to fail now, according to
> >>> kernelci:
> >>>
> >>>   1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] 
> >>> undefined!
> >>>
> >>> Should this perhaps call ioremap() or memremap() instead?
> >>>
> >> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 
> >> ("arm64:
> >> mark reserved memblock regions explicitly in iomem") starts using a 
> >> function
> >> in acpi_os_ioremap() which is not exported. On top of that, 
> >> memblock_is_memory()
> >> is declared as __init_memblock, which makes me really uncomfortable.
> >> If acpi_os_ioremap() must not be used by modules, and possibly only during
> >> early (?) initialization, maybe its declaration should state those 
> >> limitations ?
> > 
> > Ah, I didn't notice that. I guess both patches were correct individually and
> > got added to linux-next around the same time but caused allmodconfig to 
> > blow up
> > when used together.
> > 
> > Adding everyone who was involved in the memblock patch to Cc here, maybe one
> > of them has an idea what the correct fix is. There are only two other 
> > drivers
> > using acpi_os_ioremap() and one of them is x86-specific, so it's still 
> > likely
> > that drivers are not actually supposed to use this symbol. Making
> > acpi_os_ioremap() an exported function in arm64 would also work.
> 
> You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
> If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
> already EXPORT_SYMBOL_GPL().

acpi_os_ioremap() is re-defined in arm64/include/asm/acpi.h.

The problem is that, as memblock_is_memory() is declared as __init,
we cannot build any drivers which call acpi_os_ioremap() as modules.

As far as this specific issue is concerned, if we make a change like:

===8<===
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -207,7 +207,7 @@ static void __init request_standard_resources(void)
res = alloc_bootmem_low(sizeof(*res));
if (memblock_is_nomap(region)) {
res->name  = "reserved";
-   res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+   res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
} else {
res->name  = "System RAM";
res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
===>8===

and revert the following hunk from the commit:

===8<===
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -12,7 +12,7 @@
 #ifndef _ASM_ACPI_H
 #define _ASM_ACPI_H
 
-#include 
+#include 
 #include 
 
 #include 
@@ -32,7 +32,11 @@
 static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
acpi_size size)
 {
-   if (!page_is_ram(phys >> PAGE_SHIFT))
+   /*
+* EFI's reserve_regions() call adds memory with the WB attribute
+* to memblock via early_init_dt_add_memory_arch().
+*/
+   if (!memblock_is_memory(phys))
return ioremap(phys, size);
 
return ioremap_cache(phys, size);
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
===>8===

The build error will be gone. (and still kdump should work.)

But I don't know how we should distinguish IORESOURCE_MEM and
IORESOURCE_SYSTEM_RAM.

Thanks,
-Takahiro AKASHI

> (I'm still waiting for allmodconfig on linux-next to finish building)
> 
> 
> Thanks,
> 
> James
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[James Morse]

Hi,

On 09/09/16 04:18, AKASHI Takahiro wrote:
> On Thu, Sep 08, 2016 at 11:47:59AM +0100, James Morse wrote:
>> On 08/09/16 09:14, Arnd Bergmann wrote:
>>> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
 On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>> +   ctx->comm_base_addr = cppc_ss->base_address;
>> +   if (ctx->comm_base_addr) {
>> +   ctx->pcc_comm_addr =
>> +   
>> acpi_os_ioremap(ctx->comm_base_addr,
>> +   cppc_ss->length);
>>
>
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
>
>   1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] 
> undefined!
>
> Should this perhaps call ioremap() or memremap() instead?
>
 Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 
 ("arm64:
 mark reserved memblock regions explicitly in iomem") starts using a 
 function
 in acpi_os_ioremap() which is not exported. On top of that, 
 memblock_is_memory()
 is declared as __init_memblock, which makes me really uncomfortable.
 If acpi_os_ioremap() must not be used by modules, and possibly only during
 early (?) initialization, maybe its declaration should state those 
 limitations ?
>>>
>>> Ah, I didn't notice that. I guess both patches were correct individually and
>>> got added to linux-next around the same time but caused allmodconfig to 
>>> blow up
>>> when used together.
>>>
>>> Adding everyone who was involved in the memblock patch to Cc here, maybe one
>>> of them has an idea what the correct fix is. There are only two other 
>>> drivers
>>> using acpi_os_ioremap() and one of them is x86-specific, so it's still 
>>> likely
>>> that drivers are not actually supposed to use this symbol. Making
>>> acpi_os_ioremap() an exported function in arm64 would also work.
>>
>> You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
>> If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
>> already EXPORT_SYMBOL_GPL().
> 
> acpi_os_ioremap() is re-defined in arm64/include/asm/acpi.h.
> 
> The problem is that, as memblock_is_memory() is declared as __init,

__init_memblock ...

... as is memblock_is_map_memory(), which we call from pfn_valid() which is
EXPORT_SYMBOL()'d
and used from modules, (e.g. mac80211.ko). So something fishy is going on...

>From include/linux/memblock.h:
> #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> #define __init_memblock __meminit
> #define __initdata_memblock __meminitdata
> #else
> #define __init_memblock
> #define __initdata_memblock
> #endif

arm64 doesn't define ARCH_DISCARD_MEMBLOCK, so we always keep these symbols.
If we didn't, pfn_valid() would break too.


Thanks,

James


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +   ctx->comm_base_addr = cppc_ss->base_address;
> > > +   if (ctx->comm_base_addr) {
> > > +   ctx->pcc_comm_addr =
> > > +   
> > > acpi_os_ioremap(ctx->comm_base_addr,
> > > +   cppc_ss->length);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >   1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] 
> > undefined!
> > 
> > Should this perhaps call ioremap() or memremap() instead?
> > 
> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> mark reserved memblock regions explicitly in iomem") starts using a function
> in acpi_os_ioremap() which is not exported. On top of that, 
> memblock_is_memory()
> is declared as __init_memblock, which makes me really uncomfortable.
> If acpi_os_ioremap() must not be used by modules, and possibly only during
> early (?) initialization, maybe its declaration should state those 
> limitations ?

I think there is more wrong with it, the driver also accesses a shared
memory area with kernel pointers using readl_relaxed/writel_relaxed,
which are only valid on MMIO registers.

I've prepared a patch, please have a look at the follow-up email.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html