Code generation involving __raw_readl and __raw_writel

Hello everyone,

Consider the following code (preprocessor output):

static int tangox_target(struct cpufreq_policy *policy, unsigned int index) {
while (__raw_readl((volatile void *)(0xf0000000 +(0x10024))) >> 31); __raw_writel(0, (volatile void *)(0xf0000000 +(0x10024))); return 0;
}

gcc generates the following code:
(version and command-line in sig below)

00000014 <tangox_target>:

  14:   e3a03000        mov     r3, #0
  18:   e34f3001        movt    r3, #61441      ; 0xf001
  1c:   e3a02000        mov     r2, #0
  20:   e34f2001        movt    r2, #61441      ; 0xf001
  24:   e5931024        ldr     r1, [r3, #36]   ; 0x24
  28:   e3510000        cmp     r1, #0
  2c:   bafffffa        blt     1c <tangox_target+0x8>
  30:   e3a00000        mov     r0, #0
  34:   e5820024        str     r0, [r2, #36]   ; 0x24
  38:   e12fff1e        bx      lr

Do you know why gcc duplicates the address in r2 and r3? And keeps putting the address in r2 over and over in the loop?

Hello everyone,

Consider the following code (preprocessor output):

static int tangox_target(struct cpufreq_policy *policy, unsigned int index) {
while (__raw_readl((volatile void *)(0xf0000000 +(0x10024))) >> 31); __raw_writel(0, (volatile void *)(0xf0000000 +(0x10024))); return 0;
}

gcc generates the following code:
(version and command-line in sig below)

00000014 <tangox_target>:

  14:   e3a03000        mov     r3, #0
  18:   e34f3001        movt    r3, #61441      ; 0xf001
  1c:   e3a02000        mov     r2, #0
  20:   e34f2001        movt    r2, #61441      ; 0xf001
  24:   e5931024        ldr     r1, [r3, #36]   ; 0x24
  28:   e3510000        cmp     r1, #0
  2c:   bafffffa        blt     1c <tangox_target+0x8>
  30:   e3a00000        mov     r0, #0
  34:   e5820024        str     r0, [r2, #36]   ; 0x24
  38:   e12fff1e        bx      lr

Do you know why gcc duplicates the address in r2 and r3? And keeps putting the address in r2 over and over in the loop?

Hello everyone,

Consider the following code (preprocessor output):

static int tangox_target(struct cpufreq_policy *policy, unsigned int index) {
while (__raw_readl((volatile void *)(0xf0000000 +(0x10024))) >> 31); __raw_writel(0, (volatile void *)(0xf0000000 +(0x10024))); return 0;
}

gcc generates the following code:
(version and command-line in sig below)

00000014 <tangox_target>:

   14:   e3a03000        mov     r3, #0
   18:   e34f3001        movt    r3, #61441      ; 0xf001
   1c:   e3a02000        mov     r2, #0
   20:   e34f2001        movt    r2, #61441      ; 0xf001
   24:   e5931024        ldr     r1, [r3, #36]   ; 0x24
   28:   e3510000        cmp     r1, #0
   2c:   bafffffa        blt     1c <tangox_target+0x8>
   30:   e3a00000        mov     r0, #0
   34:   e5820024        str     r0, [r2, #36]   ; 0x24
   38:   e12fff1e        bx      lr

Do you know why gcc duplicates the address in r2 and r3? And keeps putting the address in r2 over and over in the loop?

I was expecting something more along these lines:

00000014 <tangox_target>:

   14:   e3a03000        mov     r3, #0
   18:   e34f3001        movt    r3, #61441      ; 0xf001
   1c:   e5931024        ldr     r1, [r3, #36]   ; 0x24
   20:   e3510000        cmp     r1, #0
   24:   bafffffa        blt     1c <tangox_target+0x8>
   28:   e3a00000        mov     r0, #0
   2c:   e5820024        str     r0, [r3, #36]   ; 0x24
   30:   e12fff1e        bx      lr

Quoted 4 lines · On Thursday 27 November 2014 11:40:34 Mason wrote:

Hello everyone,

Consider the following code (preprocessor output):

Quoted 6 lines

static int tangox_target(struct cpufreq_policy *policy, unsigned int index)

{
   while (__raw_readl((volatile void *)(0xf0000000 +(0x10024))) >> 31);
   __raw_writel(0, (volatile void *)(0xf0000000 +(0x10024)));
   return 0;
}

First of all:

- don't use __raw_readl in driver code, use readl or readl_relaxed.

- When you do a busy-loop, add a cpu_relax().

- use proper types: 'void __iomem *', not 'volatile void *'.

- use of_iomap or devm_ioremap_resource to get to the pointer for a device, don't just hardcode virtual addresses.

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · On Thursday 27 November 2014 11:40:34 Mason wrote:

Hello everyone,

Consider the following code (preprocessor output):

Quoted 8 lines

Quoted 6 lines

static int tangox_target(struct cpufreq_policy *policy, unsigned int index)

{
   while (__raw_readl((volatile void *)(0xf0000000 +(0x10024))) >> 31);
   __raw_writel(0, (volatile void *)(0xf0000000 +(0x10024)));
   return 0;
}

First of all:

Disclaimer: I've only recently started writing driver code, so I am very grateful for every tip/suggestion I receive. Please note that what I provided was *preprocessor output* (to help in diagnosing the "C to ASM" translation). The actual code was:

   while (gbus_read_reg32(SYS_cpuclk_div_ctrl) >> 31);
   gbus_write_reg32(SYS_cpuclk_div_ctrl, 0);

these being macro wrappers:

#define gbus_read_reg32(r) __raw_readl((volatile void __iomem *)IO_ADDRESS(r))
#define gbus_write_reg32(r, v) __raw_writel(v, (volatile void __iomem *)IO_ADDRESS(r))

Quoted 1 line

- don't use __raw_readl in driver code, use readl or readl_relaxed.

Can you expand a bit on the differences between these variants? __raw_readl, readl, readl_relaxed

Is this an appropriate reference?
https://www.kernel.org/doc/htmldocs/deviceiobook/index.html
( https://lwn.net/Articles/66938/ )

NB: if it matters, I am accessing memory-mapped registers, across a non-standard (I mean not PCI, ISA, etc) memory bus.

arch/arm/include/asm/io.h defines
#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) __raw_readl(c)); __r; })
#define readl(c) ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
#define ioread32(p) ({ unsigned int __v = le32_to_cpu((__force __le32)__raw_readl(p)); __iormb(); __v; })
static inline u32 __raw_readl(const volatile void __iomem *addr)

{
	u32 val;
	asm volatile("ldr %1, %0"
		     : "+Qo" (*(volatile u32 __force *)addr),
		       "=r" (val));
	return val;
}

As far as I can tell, readl and ioread32 are equivalent, right?

Quoted 1 line

- When you do a busy-loop, add a cpu_relax().

LDD3 mentions cpu_relax in the context of long delays.
My busy wait loop has a maximum latency of 60 ?s, and the cpufreq core is given this information, so that, typically, execution falls through the loop at the first iteration.

Should I still use cpu_relax? Doesn't that risk adding latency to the calling thread?

On my platform, cpu_relax is defined to a barrier:
__asm__ __volatile__("": : :"memory")

Quoted 1 line

- use proper types: 'void __iomem *', not 'volatile void *'.

The actual code does use the __iomem attribute.

Quoted 2 lines

- use of_iomap or devm_ioremap_resource to get to the pointer for a device, don't just hardcode virtual addresses.

About that. If nothing had been done, 0xf0010024 would be an invalid virtual address, and reading from that address would generate a TLB miss, right? So something must have configured the TLB to accept and translate this address correctly.

I'm looking for an iomap or ioremap call, right?

I'm asking because I have an idea in mind: on the bus, the first 16 MB contains only memory-mapped registers, so I've been thinking I can map this region at init, and keep it for the lifetime of the system. It would use only one entry in the TLB, since the CPU supports 16 MB super-sections (or whatever they are called).

I could even lock that entry in the TLB so that these accesses are guaranteed to never TLB miss, right?

I'm also wondering: why did we choose to map physical address PA at virtual address VA = PA + 0xf000000? What happens if I map PA at VA = PA? Is it a problem if I "shadow" the 0 page? (I imagine it is.)

Quoted 2 lines · On Thursday 27 November 2014 14:01:41 Mason wrote:

#define gbus_read_reg32(r) __raw_readl((volatile void __iomem *)IO_ADDRESS(r))
#define gbus_write_reg32(r, v) __raw_writel(v, (volatile void __iomem *)IO_ADDRESS(r))

Right, that's how things used to be done a while ago.

Quoted 8 lines

Quoted 2 lines

use of_iomap or devm_ioremap_resource to get to the pointer for a device, don't just hardcode virtual addresses.

About that. If nothing had been done, 0xf0010024 would be an invalid virtual address, and reading from that address would generate a TLB miss, right? So something must have configured the TLB to accept and translate this address correctly.

Quoted 1 line

I'm looking for an iomap or ioremap call, right?

The IO_ADDRESS() macro on this platform is probably defined to match a address range that is set up from a map_io callback in the platform.

On new platforms, you can't do that because the mach/*.h header files are inaccessible to drivers, so you have to use ioremap.

Quoted 6 lines

I'm asking because I have an idea in mind: on the bus, the first 16 MB contains only memory-mapped registers, so I've been thinking I can map this region at init, and keep it for the lifetime of the system. It would use only one entry in the TLB, since the CPU supports 16 MB super-sections (or whatever they are called).

Quoted 2 lines

I could even lock that entry in the TLB so that these accesses are guaranteed to never TLB miss, right?

The map_io callback will set up a mapping like that, and when a driver calls ioremap on the same physical address, you will get the correct pointer using that TLB, you just don't communicate the address through a pointer any more.

Arnd,

First of all, thanks (a lot) for your highly informative replies!

Quoted 5 lines · Arnd Bergmann

Quoted 2 lines · On Thursday 27 November 2014 14:01:41 Mason wrote:

#define gbus_read_reg32(r) __raw_readl((volatile void __iomem *)IO_ADDRESS(r))
#define gbus_write_reg32(r, v) __raw_writel(v, (volatile void __iomem *)IO_ADDRESS(r))

Quoted 1 line

Right, that's how things used to be done a while ago.

So, IIUC, old code used to call __raw_readl directly, but modern code is supposed to call either readl or readl_relaxed?

(BTW, the original code is 4-5 years old, while my target is 3.14.x)

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

use of_iomap or devm_ioremap_resource to get to the pointer for a device, don't just hardcode virtual addresses.

About that. If nothing had been done, 0xf0010024 would be an invalid virtual address, and reading from that address would generate a TLB miss, right? So something must have configured the TLB to accept and translate this address correctly.

Quoted 5 lines

Quoted 1 line

I'm looking for an iomap or ioremap call, right?

The IO_ADDRESS() macro on this platform is probably defined to match a address range that is set up from a map_io callback in the platform.

#define __IO_START 0xf0000000
#define __IO_SIZE SZ_8M
#define __IO_END (__IO_START + __IO_SIZE)
#define IO_ADDRESS(x) (__IO_START +(x))

static struct map_desc hw_io_desc[] __initdata = {
	{
		.virtual	= SCU_VIRT_BASE_ADDR,
		.pfn		=__phys_to_pfn(SCU_BASE_ADDR),
		.length		= SZ_2M,
		.type 		= MT_DEVICE,
	},
	{
		.virtual	= IO_ADDRESS(0),
		.pfn		=__phys_to_pfn(0),
		.length		= SZ_8M,
		.type 		= MT_DEVICE,
	},
};

   ...
   iotable_init(hw_io_desc, ARRAY_SIZE(tangox_87xx_io_desc));

I'll take a much closer look at iotable_init, but I suppose it is this function that sets up the TLB? As far as I can see, it is not optimal to map 8 MB, because that will take up 8 entries in the TLB, whereas 16 MB would take only one (in theory).

Quoted 2 lines

On new platforms, you can't do that because the mach/*.h header files are inaccessible to drivers, so you have to use ioremap.

What do you mean by new platforms?

Indeed, the IO_* macros given above are defined in arch/arm/mach-tangox/include/mach/io.h

But my 3.14 driver does see the header.

Quoted 9 lines

Quoted 8 lines

I'm asking because I have an idea in mind: on the bus, the first 16 MB contains only memory-mapped registers, so I've been thinking I can map this region at init, and keep it for the lifetime of the system. It would use only one entry in the TLB, since the CPU supports 16 MB super-sections (or whatever they are called).

I could even lock that entry in the TLB so that these accesses are guaranteed to never TLB miss, right?

Quoted 4 lines

The map_io callback will set up a mapping like that, and when a driver calls ioremap on the same physical address, you will get the correct pointer using that TLB, you just don't communicate the address through a pointer any more.

IIUC, you're saying the current method using iotable_init is not appropriate, and I should use the map_io callback?

Quoted 11 lines · On Thursday 27 November 2014 15:51:55 Mason wrote:

Arnd,

First of all, thanks (a lot) for your highly informative replies!

Quoted 5 lines · Arnd Bergmann

Quoted 2 lines · On Thursday 27 November 2014 14:01:41 Mason wrote:

#define gbus_read_reg32(r) __raw_readl((volatile void __iomem *)IO_ADDRESS(r))
#define gbus_write_reg32(r, v) __raw_writel(v, (volatile void __iomem *)IO_ADDRESS(r))

Quoted 6 lines

Quoted 1 line

Right, that's how things used to be done a while ago.

So, IIUC, old code used to call __raw_readl directly, but modern code is supposed to call either readl or readl_relaxed?

(BTW, the original code is 4-5 years old, while my target is 3.14.x)

I meant the IO_ADDRESS stuff. Modern code uses ioremap() instead since the IO_ADDRESS was platform specific, and drivers can no longer use platform headers on CONFIG_ARCH_MULTIPLATFORM, which is used for all new code now.

The __raw_readl() was probably considered bad back then already, but a lot of people did it as we were lacking readl_relaxed().

Would you consider submitting the code upstream?

Quoted 1 line · Arnd Bergmann

Would you consider submitting the code upstream?

This appears to be targeting a Sigma Designs chip, and nobody has to my knowledge ever seen anything go upstream from them.

Quoted 6 lines · M?ns Rullg?rd

Quoted 1 line · Arnd Bergmann

Would you consider submitting the code upstream?

This appears to be targeting a Sigma Designs chip, and nobody has to my knowledge ever seen anything go upstream from them.

Well, I do hope to push as much as I can into the vanilla kernel. (That is what "to submit the code upstream" means, right?)

Quoted 9 lines · Mason

Quoted 6 lines · M?ns Rullg?rd

Quoted 1 line · Arnd Bergmann

Would you consider submitting the code upstream?

This appears to be targeting a Sigma Designs chip, and nobody has to my knowledge ever seen anything go upstream from them.

Quoted 2 lines

Well, I do hope to push as much as I can into the vanilla kernel. (That is what "to submit the code upstream" means, right?)

That's great. You mentioned working against 3.14. Do you have any plans to move onto the current version? That would be a prerequisite for upstream acceptance.

Quoted 3 lines · On Thursday 27 November 2014 16:18:20 M?ns Rullg?rd wrote:

That's great. You mentioned working against 3.14. Do you have any plans to move onto the current version? That would be a prerequisite for upstream acceptance.

FWIW, it's also a good strategy to do the initial bringup on the latest kernel and then backport to the older one for product work, otherwise you end up doing the forward port twice, and it's almost always harder than a backport.

Quoted 10 lines · Arnd Bergmann

Quoted 3 lines · On Thursday 27 November 2014 16:18:20 M?ns Rullg?rd wrote:

That's great. You mentioned working against 3.14. Do you have any plans to move onto the current version? That would be a prerequisite for upstream acceptance.

FWIW, it's also a good strategy to do the initial bringup on the latest kernel and then backport to the older one for product work, otherwise you end up doing the forward port twice, and it's almost always harder than a backport.

Hmmm, I was told to target 3.14, because that's what we plan to support.

Maybe I can push one driver at a time?
(I have no idea if this is realistic or not.)

Quoted 6 lines · Arnd Bergmann

Quoted 3 lines · On Thursday 27 November 2014 16:18:20 M?ns Rullg?rd wrote:

That's great. You mentioned working against 3.14. Do you have any plans to move onto the current version? That would be a prerequisite for upstream acceptance.

Quoted 4 lines

FWIW, it's also a good strategy to do the initial bringup on the latest kernel and then backport to the older one for product work, otherwise you end up doing the forward port twice, and it's almost always harder than a backport.

Hmmm, I was told to target 3.14, because that's what we plan to support.

Maybe I can push one driver at a time?

Quoted 1 line · On Thursday 27 November 2014 15:51:55 Mason wrote:

(BTW, the original code is 4-5 years old, while my target is 3.14.x)

I meant the IO_ADDRESS stuff. Modern code uses ioremap() instead since the IO_ADDRESS was platform specific, and drivers can no longer use platform headers on CONFIG_ARCH_MULTIPLATFORM, which is used for all new code now.

Would you consider submitting the code upstream?

Modern platforms no longer have an include directory and use the standard definitions. How many other header files do you have in there?

Actually, we also try to get rid of the need for most other files in mach-* as well, though commonly you need a little bit of code for bringing up secondary CPU cores. Everything else should be a device driver.

Quoted 2 lines

IIUC, you're saying the current method using iotable_init is not appropriate, and I should use the map_io callback?

map_io and iotable_init is the same thing, the former calls the latter. What I'm saying is that relying on hardcoded virtual addresses is not appropriate, you can use the iotable_init call to set up a hugetlb mapping for performance optimization, but it should really work without that.

Quoted 8 lines · Arnd Bergmann

Quoted 1 line · On Thursday 27 November 2014 15:51:55 Mason wrote:

(BTW, the original code is 4-5 years old, while my target is 3.14.x)

I meant the IO_ADDRESS stuff. Modern code uses ioremap() instead since the IO_ADDRESS was platform specific, and drivers can no longer use platform headers on CONFIG_ARCH_MULTIPLATFORM, which is used for all new code now.

$ grep CONFIG_ARCH_MULTIPLATFORM .config
# CONFIG_ARCH_MULTIPLATFORM is not set

One more thing on the TODO list (which is starting to look like a phone directory). Do you have a reference explaining this option? (I did read the short description in arch/arm/Kconfig)

Quoted 1 line

Would you consider submitting the code upstream?

This is definitely one of my objectives. I'm still learning the ropes (I rewrote the cpufreq driver to use mostly generic functions, heard of the clk "meta-framework" but haven't even started looking at that, I'm also writing a temperature sensor driver for lm-sensors.)

Quoted 2 lines

Modern platforms no longer have an include directory and use the standard definitions. How many other header files do you have in there?

$ find arch/arm/mach-tangox -name *.h | wc -l
44

Quoted 4 lines

Actually, we also try to get rid of the need for most other files in mach-* as well, though commonly you need a little bit of code for bringing up secondary CPU cores. Everything else should be a device driver.

I see that other platforms put their cpufreq driver in drivers/cpufreq Ours is in arch/arm/mach-tangox (same for cpuidle.c)
(Hmmm, now that I look more closely, I see that we also have a generic clock.c in there... More on the TODO list.)