hwmon: xgene: Add support for X-Gene hwmon driver

This patch adds the APM X-Gene hwmon device tree node documentation.

Signed-off-by Hoan Tran <[email protected]>

Acked-by Rob Herring <[email protected]>

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

Signed-off-by Hoan Tran <[email protected]>

---

Quoted 5 lines · 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 <[email protected]>

---

[ ... ]

drivers/hwmon/xgene-hwmon.c line 703

Quoted 5 lines

drivers/hwmon/xgene-hwmon.c new file +5 -0

+
+     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 <[email protected]>

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

Guenter

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 >>):

Quoted 1 line

Quoted 1 line

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 <linux/mailbox_controller.h>
    33  #include <linux/mailbox_client.h>
    34  #include <linux/module.h>
    35  #include <linux/of.h>
    36  #include <linux/platform_device.h>
    37  #include <acpi/acpi_io.h>
  > 38  #include <acpi/pcc.h>

39
40 /* SLIMpro message defines */
41 #define MSG_TYPE_DBG 0

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

drivers/hwmon/xgene-hwmon.c line 657

Quoted 6 lines · CEST Hoan Tran

drivers/hwmon/xgene-hwmon.c new file +5 -0

+               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!

Quoted 11 lines · Arnd Bergmann

drivers/hwmon/xgene-hwmon.c line 657

Quoted 6 lines · CEST Hoan Tran

drivers/hwmon/xgene-hwmon.c new file +5 -0

+               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:

Quoted 2 lines

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?

Quoted 12 lines · CEST Guenter Roeck

Quoted 11 lines · Arnd Bergmann

drivers/hwmon/xgene-hwmon.c line 657

Quoted 6 lines · CEST Hoan Tran

drivers/hwmon/xgene-hwmon.c new file +5 -0

+               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:

Quoted 6 lines

Quoted 2 lines

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

drivers/hwmon/xgene-hwmon.c line 657

Quoted 6 lines · CEST Hoan Tran

drivers/hwmon/xgene-hwmon.c new file +5 -0

+               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?

Quoted 13 lines · Arnd Bergmann

drivers/hwmon/xgene-hwmon.c line 657

Quoted 6 lines · CEST Hoan Tran

drivers/hwmon/xgene-hwmon.c new file +5 -0

+               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!

Quoted 2 lines

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 ?

Quoted 14 lines · CEST Guenter Roeck

Quoted 13 lines · Arnd Bergmann

drivers/hwmon/xgene-hwmon.c line 657

Quoted 6 lines · CEST Hoan Tran

drivers/hwmon/xgene-hwmon.c new file +5 -0

+               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!

Quoted 8 lines

Quoted 2 lines

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.

Hi,

Quoted 28 lines · Arnd Bergmann

Quoted 14 lines · CEST Guenter Roeck

Quoted 13 lines · Arnd Bergmann

Review

Quoted 6 lines · CEST Hoan Tran

(quoted diff) +5 -0

+               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!

Quoted 8 lines

Quoted 2 lines

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.

Quoted 5 lines

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

Quoted 35 lines · James Morse

Quoted 33 lines · Arnd Bergmann

Quoted 14 lines · CEST Guenter Roeck

Quoted 13 lines · Arnd Bergmann

Review

Quoted 6 lines · CEST Hoan Tran

(quoted diff) +5 -0

+               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!

Quoted 8 lines

Quoted 2 lines

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.

Quoted 3 lines

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,

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

Quoted 13 lines · Arnd Bergmann

drivers/hwmon/xgene-hwmon.c line 657

Quoted 6 lines · CEST Hoan Tran

drivers/hwmon/xgene-hwmon.c new file +5 -0

+               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!

Quoted 2 lines

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 ?

The newly added hwmon driver fails to build in an allmodconfig kernel:

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

According to comments in the code, the mailbox is a shared memory region, not a set of MMIO registers, so we should use memremap() for mapping it instead of ioremap or acpi_os_ioremap, and pointer dereferences instead of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual to work with functions that operate on __iomem pointers, and this fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior regarding the ordering of the accesses from the CPU, but note that there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether the message data was supposed to be in LE32 format in the first place, it's possible this was meant to be interpreted as a byte stream instead.

Signed-off-by Arnd Bergmann <[email protected]>

drivers/hwmon/xgene-hwmon.c +15 -14

index bc78a5d10182..e834dfb3acca 100644
@@ -34,7 +34,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+#include <linux/io.h>
+
 #include <acpi/pcc.h>
 
 /* SLIMpro message defines */

@@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 {
 	u16 ret, val;
 
-	val = readw_relaxed(addr);
+	val = le16_to_cpu(READ_ONCE(*addr));
 	ret = val & mask;
 	val &= ~mask;
-	writew_relaxed(val, addr);
+	WRITE_ONCE(*addr, cpu_to_le16(val));
 
 	return ret;
 }

@@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 {
 	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
-	void *ptr = generic_comm_base + 1;
+	u32 *ptr = (void*)(generic_comm_base + 1);
 	int rc, i;
 	u16 val;
 

@@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 	ctx->resp_pending = true;
 
 	/* Write signature for subspace */
-	writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
-		       &generic_comm_base->signature);
+	WRITE_ONCE(generic_comm_base->signature,
+		   cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
 
 	/* Write to the shared command region */
-	writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
-		       &generic_comm_base->command);
+	WRITE_ONCE(generic_comm_base->command,
+		   cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
 
 	/* Flip CMD COMPLETE bit */
-	val = readw_relaxed(&generic_comm_base->status);
+	val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
 	val &= ~PCCS_CMD_COMPLETE;
-	writew_relaxed(val, &generic_comm_base->status);
+	WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
 
 	/* Copy the message to the PCC comm space */
 	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-		writel_relaxed(msg[i], ptr + i * 4);
+		WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
 
 	/* Ring the doorbell */
 	rc = mbox_send_message(ctx->mbox_chan, msg);

@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 		 */
 		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);
+			ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+							cppc_ss->length,
+							MEMREMAP_WT);
 		} else {
 			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
 			rc = -ENODEV;

Hi Arnd,

Quoted 23 lines · Arnd Bergmann

The newly added hwmon driver fails to build in an allmodconfig kernel:

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

According to comments in the code, the mailbox is a shared memory region, not a set of MMIO registers, so we should use memremap() for mapping it instead of ioremap or acpi_os_ioremap, and pointer dereferences instead of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual to work with functions that operate on __iomem pointers, and this fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior regarding the ordering of the accesses from the CPU, but note that there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether the message data was supposed to be in LE32 format in the first place, it's possible this was meant to be interpreted as a byte stream instead.

Quoted 2 lines

Signed-off-by Arnd Bergmann <[email protected]>

Thanks a lot for looking into this.

I'll apply this patch to address the build problem. Much better than my rude "depends on BROKEN". It would be great to get a Tested-by:
from someone with access to the hardware.

Guenter

Quoted 79 lines

drivers/hwmon/xgene-hwmon.c +15 -14

index bc78a5d10182..e834dfb3acca 100644
@@ -34,7 +34,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+#include <linux/io.h>
+
 #include <acpi/pcc.h>
 
 /* SLIMpro message defines */

@@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 {
      u16 ret, val;
 
-     val = readw_relaxed(addr);
+     val = le16_to_cpu(READ_ONCE(*addr));
      ret = val & mask;
      val &= ~mask;
-     writew_relaxed(val, addr);
+     WRITE_ONCE(*addr, cpu_to_le16(val));
 
      return ret;
 }

@@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 {
      struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
-     void *ptr = generic_comm_base + 1;
+     u32 *ptr = (void*)(generic_comm_base + 1);
      int rc, i;
      u16 val;
 

@@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
      ctx->resp_pending = true;
 
      /* Write signature for subspace */
-     writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
-                    &generic_comm_base->signature);
+     WRITE_ONCE(generic_comm_base->signature,
+                cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
 
      /* Write to the shared command region */
-     writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
-                    &generic_comm_base->command);
+     WRITE_ONCE(generic_comm_base->command,
+                cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
 
      /* Flip CMD COMPLETE bit */
-     val = readw_relaxed(&generic_comm_base->status);
+     val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
      val &= ~PCCS_CMD_COMPLETE;
-     writew_relaxed(val, &generic_comm_base->status);
+     WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
 
      /* Copy the message to the PCC comm space */
      for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-             writel_relaxed(msg[i], ptr + i * 4);
+             WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
 
      /* Ring the doorbell */
      rc = mbox_send_message(ctx->mbox_chan, msg);

@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
               */
              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);
+                     ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+                                                     cppc_ss->length,
+                                                     MEMREMAP_WT);
              } else {
                      dev_err(&pdev->dev, "Failed to get PCC comm region\n");
                      rc = -ENODEV;

The newly added hwmon driver fails to build in an allmodconfig kernel:

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

According to comments in the code, the mailbox is a shared memory region, not a set of MMIO registers, so we should use memremap() for mapping it instead of ioremap or acpi_os_ioremap, and pointer dereferences instead of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual to work with functions that operate on __iomem pointers, and this fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior regarding the ordering of the accesses from the CPU, but note that there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether the message data was supposed to be in LE32 format in the first place, it's possible this was meant to be interpreted as a byte stream instead.

Signed-off-by Arnd Bergmann <[email protected]>

drivers/hwmon/xgene-hwmon.c +0 -0

index bc78a5d10182..e834dfb3acca 100644

drivers/hwmon/xgene-hwmon.c +3 -2

+
 #include <acpi/pcc.h>
 
 /* SLIMpro message defines */

@@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 {
        u16 ret, val;
 
-       val = readw_relaxed(addr);
+       val = le16_to_cpu(READ_ONCE(*addr));
        ret = val & mask;
        val &= ~mask;
-       writew_relaxed(val, addr);
+       WRITE_ONCE(*addr, cpu_to_le16(val));
 
        return ret;
 }

drivers/hwmon/xgene-hwmon.c +7 -7

        int rc, i;
        u16 val;
 

@@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
        ctx->resp_pending = true;
 
        /* Write signature for subspace */
-       writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
-                      &generic_comm_base->signature);
+       WRITE_ONCE(generic_comm_base->signature,
+                  cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
 
        /* Write to the shared command region */
-       writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
-                      &generic_comm_base->command);
+       WRITE_ONCE(generic_comm_base->command,
+                  cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
 
        /* Flip CMD COMPLETE bit */
-       val = readw_relaxed(&generic_comm_base->status);
+       val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
        val &= ~PCCS_CMD_COMPLETE;
-       writew_relaxed(val, &generic_comm_base->status);
+       WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
 
        /* Copy the message to the PCC comm space */
        for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-               writel_relaxed(msg[i], ptr + i * 4);
+               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
 
        /* Ring the doorbell */
        rc = mbox_send_message(ctx->mbox_chan, msg);

drivers/hwmon/xgene-hwmon.c +0 -0

                } else {
                        dev_err(&pdev->dev, "Failed to get PCC comm region\n");
                        rc = -ENODEV;

drivers/hwmon/xgene-hwmon.c line 37

Quoted 10 lines · Arnd Bergmann

The newly added hwmon driver fails to build in an allmodconfig index bc78a5d10182..e834dfb3acca 100644

drivers/hwmon/xgene-hwmon.c +1 -1

@@ -34,7 +34,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+#include <linux/io.h>

Alphabetical order.

drivers/hwmon/xgene-hwmon.c static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask) · line 141

Quoted 3 lines

drivers/hwmon/xgene-hwmon.c +1 -1

        struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
-       void *ptr = generic_comm_base + 1;
+       u32 *ptr = (void*)(generic_comm_base + 1);

Space before "*".

Quoted 5 lines

drivers/hwmon/xgene-hwmon.c +0 -1

@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
                 */
                ctx->comm_base_addr = cppc_ss->base_address;
                if (ctx->comm_base_addr) {
-                       ctx->pcc_comm_addr =

drivers/hwmon/xgene-hwmon.c static int xgene_hwmon_probe(struct platform_device *pdev) · line 658

Quoted 5 lines

drivers/hwmon/xgene-hwmon.c +3 -2

-                                       acpi_os_ioremap(ctx->comm_base_addr,
-                                                       cppc_ss->length);
+                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+                                                       cppc_ss->length,
+                                                       MEMREMAP_WT);

It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our co-processor is also in the coherency domain.

The newly added hwmon driver fails to build in an allmodconfig kernel:

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

According to comments in the code, the mailbox is a shared memory region, not a set of MMIO registers, so we should use memremap() for mapping it instead of ioremap or acpi_os_ioremap, and pointer dereferences instead of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual to work with functions that operate on __iomem pointers, and this fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior regarding the ordering of the accesses from the CPU, but note that there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether the message data was supposed to be in LE32 format in the first place, it's possible this was meant to be interpreted as a byte stream instead.

Signed-off-by Arnd Bergmann <[email protected]>

---
v2: use write-back mapping instead of write-thru,
minor coding style changes

drivers/hwmon/xgene-hwmon.c +12 -11

index bc78a5d10182..e5470bd49067 100644
@@ -27,6 +27,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/kfifo.h>
 #include <linux/mailbox_controller.h>

@@ -34,7 +35,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+
 #include <acpi/pcc.h>
 
 /* SLIMpro message defines */

@@ -126,10 +127,10 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 {
        u16 ret, val;
 
-       val = readw_relaxed(addr);
+       val = le16_to_cpu(READ_ONCE(*addr));
        ret = val & mask;
        val &= ~mask;
-       writew_relaxed(val, addr);
+       WRITE_ONCE(*addr, cpu_to_le16(val));
 
        return ret;
 }

@@ -137,7 +138,7 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 {
        struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
-       void *ptr = generic_comm_base + 1;
+       u32 *ptr = (void *)(generic_comm_base + 1);
        int rc, i;
        u16 val;
 

@@ -146,21 +147,21 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
        ctx->resp_pending = true;
 
        /* Write signature for subspace */
-       writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx,
-                      &generic_comm_base->signature);
+       WRITE_ONCE(generic_comm_base->signature,
+                  cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
 
        /* Write to the shared command region */
-       writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT,
-                      &generic_comm_base->command);
+       WRITE_ONCE(generic_comm_base->command,
+                  cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
 
        /* Flip CMD COMPLETE bit */
-       val = readw_relaxed(&generic_comm_base->status);
+       val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
        val &= ~PCCS_CMD_COMPLETE;
-       writew_relaxed(val, &generic_comm_base->status);
+       WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
 
        /* Copy the message to the PCC comm space */
        for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
-               writel_relaxed(msg[i], ptr + i * 4);
+               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
 
        /* Ring the doorbell */
        rc = mbox_send_message(ctx->mbox_chan, msg);

@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
                 */
                ctx->comm_base_addr = cppc_ss->base_address;

The newly added hwmon driver fails to build in an allmodconfig kernel:

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

According to comments in the code, the mailbox is a shared memory region, not a set of MMIO registers, so we should use memremap() for mapping it instead of ioremap or acpi_os_ioremap, and pointer dereferences instead of readl/writel.

The driver already uses plain kernel pointers, so it's a bit unusual to work with functions that operate on __iomem pointers, and this fixes that part too.

I'm using READ_ONCE/WRITE_ONCE here to keep the existing behavior regarding the ordering of the accesses from the CPU, but note that there are no barriers (also unchanged from before).

I'm also keeping the endianess behavior, though I'm unsure whether the message data was supposed to be in LE32 format in the first place, it's possible this was meant to be interpreted as a byte stream instead.

Signed-off-by Arnd Bergmann <[email protected]>

Quoted 14 lines · CEST Hoan Tran

drivers/hwmon/xgene-hwmon.c line 37

Quoted 10 lines · Arnd Bergmann

The newly added hwmon driver fails to build in an allmodconfig index bc78a5d10182..e834dfb3acca 100644

drivers/hwmon/xgene-hwmon.c +1 -1

@@ -34,7 +34,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <acpi/acpi_io.h>
+#include <linux/io.h>

Alphabetical order.

Quoted 5 lines

drivers/hwmon/xgene-hwmon.c static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask) · line 141

Quoted 3 lines

drivers/hwmon/xgene-hwmon.c +1 -1

        struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
-       void *ptr = generic_comm_base + 1;
+       u32 *ptr = (void*)(generic_comm_base + 1);

Space before "*".

Ok.

Quoted 5 lines

Quoted 5 lines

drivers/hwmon/xgene-hwmon.c +0 -1

@@ -652,9 +653,9 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
                 */
                ctx->comm_base_addr = cppc_ss->base_address;
                if (ctx->comm_base_addr) {
-                       ctx->pcc_comm_addr =

Quoted 8 lines

drivers/hwmon/xgene-hwmon.c static int xgene_hwmon_probe(struct platform_device *pdev) · line 658

Quoted 5 lines

drivers/hwmon/xgene-hwmon.c +3 -2

-                                       acpi_os_ioremap(ctx->comm_base_addr,
-                                                       cppc_ss->length);
+                       ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+                                                       cppc_ss->length,
+                                                       MEMREMAP_WT);

It should be MEMREMAP_WB. As mailbox shared memory is on RAM and our co-processor is also in the coherency domain.

Right, I was wondering about this, since I could not figure out what the other side is (hardware, service processor or firmware). So MEMREMAP_WB makes sense here.

Two more questions:

* Any comment on the byte ordering of the data in this line:

        /* Copy the message to the PCC comm space */
        for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)

drivers/hwmon/xgene-hwmon.c +1 -1

-               writel_relaxed(msg[i], ptr + i * 4);
+               WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));

This assumes that the old code was correct even when running on big-endian kernels and the message data consists of 32-bit data words. If the message has some other format instead, we would need to treat this as a byte stream and not do swapping here but instead do it (if any) in the code that reads or writes the actual data here.

* Are you sure you don't need any smp_rmb()/smp_wmb() barriers between the accesses?

Arnd

Quoted 3 lines

* Are you sure you don't need any smp_rmb()/smp_wmb() barriers between the accesses?

No, we don't need a strict read/write during access PCC subspace. Just make sure all access is committed before PCC send message to the platform which done by PCC mailbox driver.

Quoted 4 lines · CEST Hoan Tran

Quoted 3 lines

* Are you sure you don't need any smp_rmb()/smp_wmb() barriers between the accesses?

Quoted 4 lines

No, we don't need a strict read/write during access PCC subspace. Just make sure all access is committed before PCC send message to the platform which done by PCC mailbox driver.

Ok, got it. The PCC mailbox driver presumably uses writel() to send the message, and that implies the necessary barrier (unlike writel_relaxed), right?

Arnd

This patch adds DT node to enable hwmon driver for APM X-Gene SoC.

Signed-off-by Hoan Tran <[email protected]>

---
 arch/arm64/boot/dts/apm/apm-shadowcat.dtsi | 5 +++++
 arch/arm64/boot/dts/apm/apm-storm.dtsi     | 5 +++++
 2 files changed, 10 insertions(+)

arch/arm64/boot/dts/apm/apm-shadowcat.dtsi +5 -0

index c569f76..a5935f5 100644
@@ -472,6 +472,11 @@
 			mboxes = <&mailbox 0>;
 		};
 
+		hwmonslimpro {
+			compatible = "apm,xgene-slimpro-hwmon";
+			mboxes = <&mailbox 7>;
+		};
+
 		serial0: serial at 10600000 {
 			device_type = "serial";
 			compatible = "ns16550";

arch/arm64/boot/dts/apm/apm-storm.dtsi +5 -0

index 5147d76..f8ea5b5 100644
@@ -716,6 +716,11 @@
 			mboxes = <&mailbox 0>;
 		};
 
+		hwmonslimpro {
+			compatible = "apm,xgene-slimpro-hwmon";
+			mboxes = <&mailbox 7>;
+		};
+
 		serial0: serial at 1c020000 {
 			status = "disabled";
 			device_type = "serial";

Signature · 1 line

1.9.1