Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1478647 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-07 23:50 +0200
    Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Guenter Roeck <linux@roeck-us.net> - 2016-09-08 00:30 +0200
      Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-08 10:20 +0200
        Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Guenter Roeck <linux@roeck-us.net> - 2016-09-08 17:00 +0200
          Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-08 17:10 +0200
    Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Guenter Roeck <linux@roeck-us.net> - 2016-09-08 00:40 +0200
      Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-08 10:20 +0200
        Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver James Morse <james.morse@arm.com> - 2016-09-08 12:50 +0200
          Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver AKASHI Takahiro <takahiro.akashi@linaro.org> - 2016-09-09 05:20 +0200
            Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver James Morse <james.morse@arm.com> - 2016-09-09 11:40 +0200
      Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-09 17:50 +0200
      [PATCH] hwmon: xgene: access mailbox as RAM Arnd Bergmann <arnd@arndb.de> - 2016-09-09 17:50 +0200
        Re: [PATCH] hwmon: xgene: access mailbox as RAM Guenter Roeck <linux@roeck-us.net> - 2016-09-09 19:00 +0200
          Re: [PATCH] hwmon: xgene: access mailbox as RAM Hoan Tran <hotran@apm.com> - 2016-09-09 19:10 +0200
        Re: [PATCH] hwmon: xgene: access mailbox as RAM Hoan Tran <hotran@apm.com> - 2016-09-09 21:30 +0200
          Re: [PATCH] hwmon: xgene: access mailbox as RAM Arnd Bergmann <arnd@arndb.de> - 2016-09-09 22:10 +0200
            [PATCH v2] hwmon: xgene: access mailbox as RAM Arnd Bergmann <arnd@arndb.de> - 2016-09-09 22:20 +0200
              Re: [PATCH v2] hwmon: xgene: access mailbox as RAM Hoan Tran <hotran@apm.com> - 2016-09-09 22:50 +0200
              Re: [PATCH v2] hwmon: xgene: access mailbox as RAM Guenter Roeck <linux@roeck-us.net> - 2016-09-10 00:00 +0200
            Re: [PATCH] hwmon: xgene: access mailbox as RAM Hoan Tran <hotran@apm.com> - 2016-09-09 22:50 +0200
              Re: [PATCH] hwmon: xgene: access mailbox as RAM Arnd Bergmann <arnd@arndb.de> - 2016-09-09 23:00 +0200
                Re: [PATCH] hwmon: xgene: access mailbox as RAM Hoan Tran <hotran@apm.com> - 2016-09-09 23:00 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

#1478666

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

[toc] | [prev] | [next] | [standalone]

#1478907

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

[toc] | [prev] | [next] | [standalone]

#1479302

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

[toc] | [prev] | [next] | [standalone]

#1479313

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

[toc] | [prev] | [next] | [standalone]

#1478668

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

[toc] | [prev] | [next] | [standalone]

#1478904

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

[toc] | [prev] | [next] | [standalone]

#1479076

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

[toc] | [prev] | [next] | [standalone]

#1479628

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 <linux/mm.h>
+#include <linux/memblock.h>
 #include <linux/psci.h>
 
 #include <asm/cputype.h>
@@ -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
> 
> 

[toc] | [prev] | [next] | [standalone]

#1479799

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

[toc] | [prev] | [next] | [standalone]

#1480164

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

[toc] | [prev] | [next] | [standalone]

#1480172 — [PATCH] hwmon: xgene: access mailbox as RAM

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 <arnd@arndb.de>

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index bc78a5d10182..e834dfb3acca 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -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;

[toc] | [prev] | [next] | [standalone]

#1480208 — Re: [PATCH] hwmon: xgene: access mailbox as RAM

Hi Arnd,

On Fri, Sep 09, 2016 at 05:38:58PM +0200, Arnd Bergmann wrote:
> 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 <arnd@arndb.de>
> 

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

> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e834dfb3acca 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -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;
> 

[toc] | [prev] | [next] | [standalone]

#1480211 — Re: [PATCH] hwmon: xgene: access mailbox as RAM

On Fri, Sep 9, 2016 at 9:58 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> Hi Arnd,
>
> On Fri, Sep 09, 2016 at 05:38:58PM +0200, Arnd Bergmann wrote:
>> 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 <arnd@arndb.de>
>>
>
> 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.
>

Hi Arnd and Guenter,

Thanks for the patch. I'm testing it out.

Hoan

> Guenter
>
>> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
>> index bc78a5d10182..e834dfb3acca 100644
>> --- a/drivers/hwmon/xgene-hwmon.c
>> +++ b/drivers/hwmon/xgene-hwmon.c
>> @@ -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;
>>

[toc] | [prev] | [next] | [standalone]

#1480274 — Re: [PATCH] hwmon: xgene: access mailbox as RAM

Hi Arnd,

On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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 <arnd@arndb.de>
>
> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e834dfb3acca 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -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.

> +
>  #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);

Space before "*".

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

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

Thanks
Hoan

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

[toc] | [prev] | [next] | [standalone]

#1480301 — Re: [PATCH] hwmon: xgene: access mailbox as RAM

On Friday, September 9, 2016 12:24:32 PM CEST Hoan Tran wrote:
> On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > The newly added hwmon driver fails to build in an allmodconfig
> > index bc78a5d10182..e834dfb3acca 100644
> > --- a/drivers/hwmon/xgene-hwmon.c
> > +++ b/drivers/hwmon/xgene-hwmon.c
> > @@ -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.
> 
> >         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.

> > @@ -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);
> 
> 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++)
-               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

[toc] | [prev] | [next] | [standalone]

#1480320 — [PATCH v2] hwmon: xgene: access mailbox as RAM

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 <arnd@arndb.de>
---
v2: use write-back mapping instead of write-thru,
    minor coding style changes

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index bc78a5d10182..e5470bd49067 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -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;
 		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_WB);
 		} else {
 			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
 			rc = -ENODEV;

[toc] | [prev] | [next] | [standalone]

#1480345 — Re: [PATCH v2] hwmon: xgene: access mailbox as RAM

Hi Arnd,

On Fri, Sep 9, 2016 at 1:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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 <arnd@arndb.de>
> ---
> v2: use write-back mapping instead of write-thru,
>     minor coding style changes
>
> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index bc78a5d10182..e5470bd49067 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -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;
>                 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_WB);
>                 } else {
>                         dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>                         rc = -ENODEV;
>

Acked-by: Hoan Tran <hotran@apm.com>
Tested-by: Hoan Tran <hotran@apm.com>

Thanks
Hoan

[toc] | [prev] | [next] | [standalone]

#1480378 — Re: [PATCH v2] hwmon: xgene: access mailbox as RAM

On Fri, Sep 09, 2016 at 10:10:45PM +0200, Arnd Bergmann wrote:
> 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 <arnd@arndb.de>

Applied after 's/endianess/endianness/' to make checkpatch happy.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1480342 — Re: [PATCH] hwmon: xgene: access mailbox as RAM

On Fri, Sep 9, 2016 at 12:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, September 9, 2016 12:24:32 PM CEST Hoan Tran wrote:
>> On Fri, Sep 9, 2016 at 8:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > The newly added hwmon driver fails to build in an allmodconfig
>> > index bc78a5d10182..e834dfb3acca 100644
>> > --- a/drivers/hwmon/xgene-hwmon.c
>> > +++ b/drivers/hwmon/xgene-hwmon.c
>> > @@ -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.
>>
>> >         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.
>
>> > @@ -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);
>>
>> 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++)
> -               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.

This is 32-bit data words.

>
> * 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.

Thanks
Hoan

>
>         Arnd

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web