fsl/qbman: ARM Enablement

From: Claudiu Manoil <[email protected]>

Signed-off-by Madalin Bucur <[email protected]>

Signed-off-by Claudiu Manoil <[email protected]>

Signed-off-by Roy Pledge <[email protected]>

---
 drivers/soc/fsl/qbman/qman_ccsr.c |    6 ++++++
 1 file changed, 6 insertions(+)

drivers/soc/fsl/qbman/qman_ccsr.c +0 -0

index 43feaa9..67ae073 100644

Quoted 14 lines · Roy Pledge

From: Claudiu Manoil <[email protected]>

Signed-off-by Madalin Bucur <[email protected]>

Signed-off-by Claudiu Manoil <[email protected]>

Signed-off-by Roy Pledge <[email protected]>

---
 drivers/soc/fsl/qbman/qman_ccsr.c |    6 ++++++
 1 file changed, 6 insertions(+)

drivers/soc/fsl/qbman/qman_ccsr.c +0 -0

index 43feaa9..67ae073 100644

drivers/soc/fsl/qbman/qman_ccsr.c static int zero_priv_mem(struct device *dev, struct device_node *node, · line 453

Quoted 8 lines

drivers/soc/fsl/qbman/qman_ccsr.c +3 -0

@@ -446,8 +446,14 @@ static int zero_priv_mem(struct device *dev, struct device_node *node,
 		return -ENOMEM;
 
 	memset(tmpp, 0, sz);
+#ifdef CONFIG_PPC
 	flush_dcache_range((unsigned long)tmpp,
 			   (unsigned long)tmpp + sz);
+#elif defined(CONFIG_ARM)
+	__cpuc_flush_dcache_area(tmpp, sz);

Please do not fiddle about under the covers, there be dragons there.

It looks to me like you're trying to use __cpuc_flush_dcache_area() on an area that's been ioremap()'d, which is a waste of CPU cycles. ioremap()'d areas are mapped as "device" memory type, which means the region isn't even cached. So I don't think this is necessary.

Quoted 25 lines · Russell King - ARM Linux

Quoted 14 lines · Roy Pledge

From: Claudiu Manoil <[email protected]>

Signed-off-by Madalin Bucur <[email protected]>

Signed-off-by Claudiu Manoil <[email protected]>

Signed-off-by Roy Pledge <[email protected]>

---
 drivers/soc/fsl/qbman/qman_ccsr.c |    6 ++++++
 1 file changed, 6 insertions(+)

drivers/soc/fsl/qbman/qman_ccsr.c +0 -0

index 43feaa9..67ae073 100644

drivers/soc/fsl/qbman/qman_ccsr.c static int zero_priv_mem(struct device *dev, struct device_node *node, · line 453

Quoted 8 lines

drivers/soc/fsl/qbman/qman_ccsr.c +3 -0

@@ -446,8 +446,14 @@ static int zero_priv_mem(struct device *dev, struct device_node *node,
 		return -ENOMEM;
 
 	memset(tmpp, 0, sz);
+#ifdef CONFIG_PPC
 	flush_dcache_range((unsigned long)tmpp,
 			   (unsigned long)tmpp + sz);
+#elif defined(CONFIG_ARM)
+	__cpuc_flush_dcache_area(tmpp, sz);

Please do not fiddle about under the covers, there be dragons there.

Quoted 4 lines

It looks to me like you're trying to use __cpuc_flush_dcache_area() on an area that's been ioremap()'d, which is a waste of CPU cycles. ioremap()'d areas are mapped as "device" memory type, which means the region isn't even cached. So I don't think this is necessary.

It is mapped cachable, via memremap() (see patch 1/10). This is RAM that the device uses, non-coherently, for its own purposes (but requires software to clear it first).

Is there a non-"under the covers" way to say "flush this region" without the arch second-guessing whether it really needs to be flushed?

-Scott

Quoted 2 lines

Is there a non-"under the covers" way to say "flush this region" without the arch second-guessing whether it really needs to be flushed?

Any advice on how to resolve this? I looked into trying to do a non-cacheable mapping of the
memory so that the flush wouldn't be required but the ioremap code prevents mapping normal
memory in this way. The QMan device requiresthis memory to be zeroed at startup. Because the device
does non coherent reads and writes to the memory we must ensure that any cache in the CPU cluster(s)
is flushed in order to prevent a future castout from overwriting data. This happened before on PPC
platforms and made for some very unfun debug session trying to understand what was causing the failure.

Quoted 14 lines · CET Roy Pledge

Quoted 2 lines

Is there a non-"under the covers" way to say "flush this region" without the arch second-guessing whether it really needs to be flushed?

Any advice on how to resolve this? I looked into trying to do a non-cacheable mapping of the
memory so that the flush wouldn't be required but the ioremap code prevents mapping normal
memory in this way. The QMan device requiresthis memory to be zeroed at startup. Because the device
does non coherent reads and writes to the memory we must ensure that any cache in the CPU cluster(s)
is flushed in order to prevent a future castout from overwriting data. This happened before on PPC
platforms and made for some very unfun debug session trying to understand what was causing the failure.

If this is normal RAM, you should be able to just write zeroes, and then do a dma_map_single() for initialization.

Are there any other requirements
what to do with the memory later, is it used for communication at all, or just required to be zero?

Quoted 4 lines · Arnd Bergmann

On Monday, January 23, 2017 7:24:59 PM CET Roy Pledge wrote:

If this is normal RAM, you should be able to just write zeroes, and then do a dma_map_single() for initialization.

The DMA API on PPC currently has no way of knowing that the device accesses this memory incoherently.

If that were somehow fixed, we still couldn't use dma_map_single() as it doesn't accept virtual addresses that come from memremap() or similar dynamic mappings. We'd have to convert the physical address to an array of struct pages and pass each one to dma_map_page().

And even if we did all that, there would still be other manual cache manipulation left in this driver, to deal with its cacheable register interface.

Quoted 3 lines

Are there any other requirements
what to do with the memory later, is it used for communication at all, or just required to be zero?

It just needs to be zeroed before the device begins using it for internal storage.

Quoted 6 lines · Scott Wood

Quoted 4 lines · Arnd Bergmann

On Monday, January 23, 2017 7:24:59 PM CET Roy Pledge wrote:

Quoted 4 lines

Quoted 2 lines

If this is normal RAM, you should be able to just write zeroes, and then do a dma_map_single() for initialization.

The DMA API on PPC currently has no way of knowing that the device accesses this memory incoherently.

Ah, this is because PPC doesn't use the 'dma-coherent' property on devices but just assumes that coherency is a global property of the platform,right?

Quoted 4 lines

If that were somehow fixed, we still couldn't use dma_map_single() as it doesn't accept virtual addresses that come from memremap() or similar dynamic mappings.??We'd have to convert the physical address to an array of struct pages and pass each one to dma_map_page().

Sorry for my ignorance, but where does the memory come from to start with? Is this in the normal linearly mapped RAM, in a carveout outside of the linear mapping but the same memory, or in some on-chip buffer?

Quoted 3 lines

And even if we did all that, there would still be other manual cache manipulation left in this driver, to deal with its cacheable register interface.

I thought we had concluded that "cacheable register" is something that cannot work reliably on ARM at all when this came up before. Any updates on that?

Quoted 7 lines · Arnd Bergmann

Quoted 6 lines · Scott Wood

Quoted 4 lines · Arnd Bergmann

On Monday, January 23, 2017 7:24:59 PM CET Roy Pledge wrote:

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

If this is normal RAM, you should be able to just write zeroes, and then do a dma_map_single() for initialization.

The DMA API on PPC currently has no way of knowing that the device accesses this memory incoherently.

Ah, this is because PPC doesn't use the 'dma-coherent' property on devices but just assumes that coherency is a global property of the platform,right?

Right.

Quoted 4 lines

Quoted 4 lines

If that were somehow fixed, we still couldn't use dma_map_single() as it doesn't accept virtual addresses that come from memremap() or similar dynamic mappings. We'd have to convert the physical address to an array of struct pages and pass each one to dma_map_page().

Quoted 3 lines

Sorry for my ignorance, but where does the memory come from to start with? Is this in the normal linearly mapped RAM, in a carveout outside of the linear mapping but the same memory, or in some on-chip buffer?

It's RAM that comes from the device tree reserved memory mechanism (drivers/of/of_reserved_mem.c). On a 32-bit kernel it is not guaranteed (or likely) to be lowmem.

Quoted 6 lines

Quoted 3 lines

And even if we did all that, there would still be other manual cache manipulation left in this driver, to deal with its cacheable register interface.

I thought we had concluded that "cacheable register" is something that cannot work reliably on ARM at all when this came up before. Any updates on that?

I'm not familiar with the details there... My understanding is that the hardware people at NXP are convinced that it can work on these specific chips due to implementation details.

-Scott

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 19 lines · Scott Wood

Quoted 9 lines · Arnd Bergmann

Quoted 6 lines · Scott Wood

Quoted 3 lines · Arnd Bergmann

On Monday, January 23, 2017 7:24:59 PM CET Roy Pledge wrote:
If this is normal RAM, you should be able to just write zeroes, and then do a dma_map_single() for initialization.

The DMA API on PPC currently has no way of knowing that the device accesses this memory incoherently.

Ah, this is because PPC doesn't use the 'dma-coherent' property on devices but just assumes that coherency is a global property of the platform,right?

Right.

Quoted 7 lines

Quoted 4 lines

If that were somehow fixed, we still couldn't use dma_map_single() as it doesn't accept virtual addresses that come from memremap() or similar dynamic mappings. We'd have to convert the physical address to an array of struct pages and pass each one to dma_map_page().

Sorry for my ignorance, but where does the memory come from to start with? Is this in the normal linearly mapped RAM, in a carveout outside of the linear mapping but the same memory, or in some on-chip buffer?

Quoted 3 lines

It's RAM that comes from the device tree reserved memory mechanism (drivers/of/of_reserved_mem.c). On a 32-bit kernel it is not guaranteed (or likely) to be lowmem.

Wouldn't dma_declare_coherent_memory() be the appropriate tool for that job, then (modulo the PPC issue)? On ARM that should result in dma_alloc_coherent() giving back a non-cacheable mapping if the device is non-coherent, wherein a dmb_wmb() after writing the data from the CPU side should be enough to ensure it is published to the device.

Robin.

Quoted 18 lines

Quoted 6 lines

Quoted 3 lines

And even if we did all that, there would still be other manual cache manipulation left in this driver, to deal with its cacheable register interface.

I thought we had concluded that "cacheable register" is something that cannot work reliably on ARM at all when this came up before. Any updates on that?

I'm not familiar with the details there... My understanding is that the hardware people at NXP are convinced that it can work on these specific chips due to implementation details.

-Scott

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 20 lines · Robin Murphy

Quoted 19 lines · Scott Wood

Quoted 9 lines · Arnd Bergmann

Quoted 6 lines · Scott Wood

Quoted 3 lines · Arnd Bergmann

On Monday, January 23, 2017 7:24:59 PM CET Roy Pledge wrote:
If this is normal RAM, you should be able to just write zeroes, and then do a dma_map_single() for initialization.

The DMA API on PPC currently has no way of knowing that the device accesses this memory incoherently.

Ah, this is because PPC doesn't use the 'dma-coherent' property on devices but just assumes that coherency is a global property of the platform,right?

Right.

Quoted 7 lines

Quoted 4 lines

If that were somehow fixed, we still couldn't use dma_map_single() as it doesn't accept virtual addresses that come from memremap() or similar dynamic mappings. We'd have to convert the physical address to an array of struct pages and pass each one to dma_map_page().

Sorry for my ignorance, but where does the memory come from to start with? Is this in the normal linearly mapped RAM, in a carveout outside of the linear mapping but the same memory, or in some on-chip buffer?

Quoted 8 lines

Quoted 3 lines

It's RAM that comes from the device tree reserved memory mechanism (drivers/of/of_reserved_mem.c). On a 32-bit kernel it is not guaranteed (or likely) to be lowmem.

Wouldn't dma_declare_coherent_memory() be the appropriate tool for that job, then (modulo the PPC issue)? On ARM that should result in dma_alloc_coherent() giving back a non-cacheable mapping if the device is non-coherent, wherein a dmb_wmb() after writing the data from the CPU side should be enough to ensure it is published to the device.

I think there is some confusion here (and it may very well be mine).

My understanding is that the dma_declare_coherent_memory() API sets up a region of memory that will be managed by dma_alloc_coherent() and friends. This is useful if the driver needs to manage a region of on device memory but that isn't what this specific region is used for.

The memory that is trying to be initialized here is a big chunk (possible multiple megabytes) of RAM that only the QBMan device will access with one exception - at initialization the device expects software to zero the memory before starting to use the device. Since the CPUs will never access this region again the device does non-coherent/non-shareable accesses for performance reasons since QBMan device is the only user - no need to maintain coherency with core side caches.

We used the of_reserve memory as that seemed to be the best reliable way to guarantee that we would get a properly aligned contiguous allocation and it's been working well. The downside is that the contents of that memory is undefined so we had to map it, zero it and flush the cache in order to get the RAM into the desired state and make sure we don't get hit by a random castout in the future.

Would it make sense to add an option in the of_reserved_mem system to fill the memory? I haven't looked at the feasibility of that but it seems like a generic solution that could be useful to others. We could add the fill value to the device tree so you could initialize to any pattern.

- Roy

Quoted 22 lines

Robin.

Quoted 18 lines

Quoted 6 lines

Quoted 3 lines

And even if we did all that, there would still be other manual cache manipulation left in this driver, to deal with its cacheable register interface.

I thought we had concluded that "cacheable register" is something that cannot work reliably on ARM at all when this came up before. Any updates on that?

I'm not familiar with the details there... My understanding is that the hardware people at NXP are convinced that it can work on these specific chips due to implementation details.

-Scott

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 47 lines · Roy Pledge

Quoted 40 lines · Robin Murphy

Quoted 34 lines · Scott Wood

Quoted 10 lines · Arnd Bergmann

Quoted 7 lines · Scott Wood

Quoted 5 lines · Arnd Bergmann

On Monday, January 23, 2017 7:24:59 PM CET Roy Pledge wrote:
If this is normal RAM, you should be able to just write zeroes, and then
do a dma_map_single() for initialization.

Quoted 6 lines

Quoted 2 lines

The DMA API on PPC currently has no way of knowing that the device accesses this memory incoherently.

Ah, this is because PPC doesn't use the 'dma-coherent' property on devices
but just assumes that coherency is a global property of the platform,right?

Right.

Quoted 9 lines

Quoted 9 lines

If that were somehow fixed, we still couldn't use dma_map_single() as it
doesn't accept virtual addresses that come from memremap() or similar
dynamic mappings.??We'd have to convert the physical address to an array
of struct pages and pass each one to dma_map_page().

Quoted 5 lines

Sorry for my ignorance, but where does the memory come from to start with? Is this in the normal linearly mapped RAM, in a carveout outside
of the linear mapping but the same memory, or in some on-chip buffer?

Quoted 4 lines

It's RAM that comes from the device tree reserved memory mechanism (drivers/of/of_reserved_mem.c).??On a 32-bit kernel it is not guaranteed (or
likely) to be lowmem.

Quoted 5 lines

Wouldn't dma_declare_coherent_memory() be the appropriate tool for that job, then (modulo the PPC issue)? On ARM that should result in dma_alloc_coherent() giving back a non-cacheable mapping if the device is non-coherent, wherein a dmb_wmb() after writing the data from the CPU side should be enough to ensure it is published to the device.

Quoted 6 lines

I think there is some confusion here (and it may very well be mine).

My understanding is that the dma_declare_coherent_memory() API sets up a region of memory that will be managed by dma_alloc_coherent() and friends.??This is useful if the driver needs to manage a region of on device memory but that isn't what this specific region is used for.

It's a bit more general than that - dma_alloc_coherent() can essentially be considered "give me some memory for this device to use". We already have use-cases where such buffers are only ever accessed by the device (e.g. some display controllers, and future NVMe devices), hence DMA_ATTR_NO_KERNEL_MAPPING on ARM to save the vmalloc space.

A DMA allocation also inherently guarantees appropriate alignment, regardless of whether you're using a per-device reservation or just regular CMA,

and will also zero the underlying memory (and for a non-coherent device perform whatever cache maintenance is necessary, if the clearing isn't already done via a non-cacheable mapping).

All you need to do in the driver is allocate your buffer and hand the resulting address off to the device at probe (after optionally checking for a reservation in DT and declaring it), then free it at remove, which also ends up far more self-documenting (IMO)

than a bunch of open-coded remapping and #ifdef'ed architecture-private
cache shenanigans.

Quoted 5 lines

My understanding is that the dma_declare_coherent_memory() API sets up a region of memory that will be managed by dma_alloc_coherent() and friends. This is useful if the driver needs to manage a region of on device memory but that isn't what this specific region is used for.

It's a bit more general than that - dma_alloc_coherent() can essentially be considered "give me some memory for this device to use". We already have use-cases where such buffers are only ever accessed by the device (e.g. some display controllers, and future NVMe devices), hence DMA_ATTR_NO_KERNEL_MAPPING on ARM to save the vmalloc space.

A DMA allocation also inherently guarantees appropriate alignment, regardless of whether you're using a per-device reservation or just regular CMA, and will also zero the underlying memory (and for a non-coherent device perform whatever cache maintenance is necessary, if the clearing isn't already done via a non-cacheable mapping).

All you need to do in the driver is allocate your buffer and hand the resulting address off to the device at probe (after optionally checking for a reservation in DT and declaring it), then free it at remove, which also ends up far more self-documenting (IMO) than a bunch of open-coded remapping and #ifdef'ed architecture-private cache shenanigans.

Why don't I simplify this and use dma_sync_single_for_device() to flush the cache?

Quoted 2 lines · Roy Pledge

Why don't I simplify this and use dma_sync_single_for_device() to flush the cache?

... which can only be used on direct-mapped memory.

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Roy Pledge

Why don't I simplify this and use dma_sync_single_for_device() to flush the cache?

... which can only be used on direct-mapped memory.

Then I don't see another (or understand) any other option. There is no way I can add a large area of physical memory carved out by of_reserve_mem to a DMA allocator since both ioremap and memremap throw an error if I try to do so. I appreciate the suggestions from people to resolve this but so far there isn't a workable solution to the problem. As far as I can tell other users of dma_declare_coherent_memory() are using it for device backed memory, not DDR. It sounds like a solution on the surface but when you dig into the details it just doesn't work. Please let me know if I'm missing something, I really want to close this issue so we can get upstream support for this device on ARM.

Roy

Quoted 6 lines · Roy Pledge

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Roy Pledge

Why don't I simplify this and use dma_sync_single_for_device() to flush the cache?

... which can only be used on direct-mapped memory.

Quoted 10 lines

Then I don't see another (or understand) any other option. There is no way I can add a large area of physical memory carved out by of_reserve_mem to a DMA allocator since both ioremap and memremap throw an error if I try to do so. I appreciate the suggestions from people to resolve this but so far there isn't a workable solution to the problem. As far as I can tell other users of dma_declare_coherent_memory() are using it for device backed memory, not DDR. It sounds like a solution on the surface but when you dig into the details it just doesn't work. Please let me know if I'm missing something, I really want to close this issue so we can get upstream support for this device on ARM.

Apologies, having found time to take a proper look, it transpires I was somewhat misremembering things. You're right that dma_declare_coherent_memory() is for resources owned by the device, and a DT reservation doesn't really count as such. What I had mixed up is that a reserved-memory node has its own way of getting tied into CMA
automatically: ensure it has the "shared-dma-pool" compatible, point a memory-region property at it from your device's node, then have the driver call of_reserved_mem_device_init() - at that point, your DMA allocations should be coming out of the given region so you should be free to just grab the whole lot in one go.

I've just confirmed that experimentally by bodging a reserved-memory node into my Juno DT for the ARM HDLCD - another non-coherent device which needs ~8MB contiguous buffers - and it works as expected.

Robin.

Quoted 4 lines

Roy

Quoted 28 lines · Robin Murphy

Quoted 6 lines · Roy Pledge

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Roy Pledge

Why don't I simplify this and use dma_sync_single_for_device() to flush the cache?

... which can only be used on direct-mapped memory.

Quoted 10 lines

Then I don't see another (or understand) any other option. There is no way I can add a large area of physical memory carved out by of_reserve_mem to a DMA allocator since both ioremap and memremap throw an error if I try to do so. I appreciate the suggestions from people to resolve this but so far there isn't a workable solution to the problem. As far as I can tell other users of dma_declare_coherent_memory() are using it for device backed memory, not DDR. It sounds like a solution on the surface but when you dig into the details it just doesn't work. Please let me know if I'm missing something, I really want to close this issue so we can get upstream support for this device on ARM.

Apologies, having found time to take a proper look, it transpires I was somewhat misremembering things. You're right that dma_declare_coherent_memory() is for resources owned by the device, and a DT reservation doesn't really count as such. What I had mixed up is that a reserved-memory node has its own way of getting tied into CMA
automatically: ensure it has the "shared-dma-pool" compatible, point a memory-region property at it from your device's node, then have the driver call of_reserved_mem_device_init() - at that point, your DMA allocations should be coming out of the given region so you should be free to just grab the whole lot in one go.

Quoted 5 lines

I've just confirmed that experimentally by bodging a reserved-memory node into my Juno DT for the ARM HDLCD - another non-coherent device which needs ~8MB contiguous buffers - and it works as expected.

Robin.

This makes sense to me now, but I run into an issue when I try this on my development system.

I added 'compatible = "shared-dma-pool" into my device tree and the set the memory-region property for my device and I see this now at boottime:

[    0.000000] Reserved memory: created DMA memory pool at
0x00000009ff000000, size 8 MiB
[    0.000000] OF: reserved mem: initialized node qman-fqd, compatible

id shared-dma-pool

but when I call of_reserved_mem_device_init() during my device probe routine it fails as follows:

[    0.738814] memremap attempted on ram 0x00000009ff000000 size: 0x800000
[    0.745485] ------------[ cut here ]------------
[    0.750132] WARNING: CPU: 0 PID: 1 at kernel/memremap.c:111

memremap+0x100/0x144

<....cut out a bunch of register dumps....>

[    0.973780] [<ffff00000816a748>] memremap+0x100/0x144
[    0.978859] [<ffff00000853c1d0>] dma_init_coherent_memory+0x60/0x194
[    0.985249] [<ffff00000853c7b0>] rmem_dma_device_init+0x64/0x94
[    0.991202] [<ffff000008775c88>]
of_reserved_mem_device_init_by_idx+0xf8/0x198
[    0.998468] [<ffff00000849b1ac>] fsl_qman_probe+0xdc/0x804
[    1.003983] [<ffff000008526538>] platform_drv_probe+0x50/0xbc
[    1.009760] [<ffff00000852489c>] driver_probe_device+0x220/0x2b8
[    1.015799] [<ffff0000085249d8>] __driver_attach+0xa4/0xa8
[    1.021313] [<ffff000008522908>] bus_for_each_dev+0x58/0x98
[    1.026915] [<ffff0000085241d8>] driver_attach+0x20/0x28
[    1.032254] [<ffff000008523d98>] bus_add_driver+0x1c8/0x22c
[    1.037856] [<ffff0000085252d8>] driver_register+0x60/0xf4
[    1.043371] [<ffff00000852647c>] __platform_driver_register+0x48/0x50
[    1.049849] [<ffff000008d5800c>] fsl_qman_driver_init+0x18/0x20
[    1.055802] [<ffff000008083148>] do_one_initcall+0x38/0x118
[    1.061405] [<ffff000008d20cec>] kernel_init_freeable+0x1a0/0x240
[    1.067533] [<ffff0000088f8298>] kernel_init+0x10/0xfc
[    1.072697] [<ffff000008082ec0>] ret_from_fork+0x10/0x50
[    1.078055] Reserved memory: failed to init DMA memory pool at

0x00000009ff000000, size 8 MiB

Is there another attribute I need to set in the device tree? It seems to be trying to setup the pool but failing to do the mapping similar to the issues I faced earlier.

Thanks again

Roy

Quoted 3 lines

Quoted 3 lines

Roy

Just wanted to follow up on this - turns out the no-map; attribute is what I was missing. I now have the suggested mechanism working for ARM. However when I went to test on PowePC the region wasn't being enabled properly. It turns out the Kconfig in arch/powerpc doesn't define HAVE_GENERIC_DMA_COHERENT so this mechanism isn't available. However I added it and everything worked on my PPC board as it did on ARM.

Does anyone know why HAVE_GENERIC_DMA_COHERENT isn't defined for PowerPC? I did a quick search but didn't see previous discussion on this topic.

Quoted 7 lines · Roy Pledge

Just wanted to follow up on this - turns out the no-map; attribute is what I was missing. I now have the suggested mechanism working for ARM. However when I went to test on PowePC the region wasn't being enabled properly. It turns out the Kconfig in arch/powerpc doesn't define HAVE_GENERIC_DMA_COHERENT so this mechanism isn't available. However I added it and everything worked on my PPC board as it did on ARM.

Quoted 3 lines

Does anyone know why HAVE_GENERIC_DMA_COHERENT isn't defined for PowerPC? I did a quick search but didn't see previous discussion on this topic.

Probably because nothing on PPC needed it.

In any case, the problems with doing this on PPC run deeper than that, as I pointed out in:
https://www.spinics.net/lists/arm-kernel/msg560020.html

-Scott

Quoted 3 lines · Scott Wood

And even if we did all that, there would still be other manual cache manipulation left in this driver, to deal with its cacheable register interface.

I thought we had concluded that "cacheable register" is something that cannot work reliably on ARM at all when this came up before. Any updates on that?

Quoted 5 lines · Arnd Bergmann

Quoted 3 lines · Scott Wood

And even if we did all that, there would still be other manual cache manipulation left in this driver, to deal with its cacheable register interface.

Quoted 3 lines

I thought we had concluded that "cacheable register" is something that cannot work reliably on ARM at all when this came up before. Any updates on that?

There's a big comment in arch/arm/include/asm/io.h which explains everything. Nothing there is likely to change.

Ah right, so since there is no interface to map as "device writeback", the driver would likely end up with an MMIO register in a "normal writeback", with these properties (among others listed there):

* - writes can be repeated (in certain cases) with no side effects
* - writes can be merged before accessing the target
* - unaligned accesses can be supported
* - ordering is not guaranteed without explicit dependencies or barrier
* instructions
* - writes may be delayed before they hit the endpoint memory

I think the ordering is the most critical here, as IIRC the driver expects a whole cache line to be written into the MMIO register in a single bus cycle specifically when we issue the flush from the driver, but we can actually have partial writebacks at any time.

Quoted 3 lines · Arnd Bergmann

Ah right, so since there is no interface to map as "device writeback", the driver would likely end up with an MMIO register in a "normal writeback", with these properties (among others listed there):

It's not about there being no _interface_, there's no support in the architecture for it. The use of the "device" memory type does not allow for any form of cache hints to be specified.

Quoted 7 lines

* - writes can be repeated (in certain cases) with no side effects
* - writes can be merged before accessing the target
* - unaligned accesses can be supported
* - ordering is not guaranteed without explicit dependencies or barrier
* instructions
* - writes may be delayed before they hit the endpoint memory

Quoted 4 lines

I think the ordering is the most critical here, as IIRC the driver expects a whole cache line to be written into the MMIO register in a single bus cycle specifically when we issue the flush from the driver, but we can actually have partial writebacks at any time.

I don't think that's achievable, unless your data bus is as wide as a cache line (iow, for a 32-byte cache line, your data bus all the way to the peripheral would need to be 256 bits wide to write the cache line in a single bus cycle.)

Quoted 14 lines · CET Roy Pledge

Quoted 2 lines

Is there a non-"under the covers" way to say "flush this region" without the arch second-guessing whether it really needs to be flushed?

Any advice on how to resolve this? I looked into trying to do a non-cacheable mapping of the
memory so that the flush wouldn't be required but the ioremap code prevents mapping normal
memory in this way. The QMan device requiresthis memory to be zeroed at startup. Because the device
does non coherent reads and writes to the memory we must ensure that any cache in the CPU cluster(s)
is flushed in order to prevent a future castout from overwriting data. This happened before on PPC
platforms and made for some very unfun debug session trying to understand what was causing the failure.

If this is normal RAM, you should be able to just write zeroes, and then do a dma_map_single() for initialization. Are there any other requirements what to do with the memory later, is it used for communication at all, or just required to be zero?

Quoted 16 lines · Arnd Bergmann

Quoted 14 lines · CET Roy Pledge

Quoted 2 lines

Is there a non-"under the covers" way to say "flush this region" without the arch second-guessing whether it really needs to be flushed?

Any advice on how to resolve this? I looked into trying to do a non-cacheable mapping of the
memory so that the flush wouldn't be required but the ioremap code prevents mapping normal
memory in this way. The QMan device requiresthis memory to be zeroed at startup. Because the device
does non coherent reads and writes to the memory we must ensure that any cache in the CPU cluster(s)
is flushed in order to prevent a future castout from overwriting data. This happened before on PPC
platforms and made for some very unfun debug session trying to understand what was causing the failure.

Quoted 4 lines

If this is normal RAM, you should be able to just write zeroes, and then do a dma_map_single() for initialization. Are there any other requirements what to do with the memory later, is it used for communication at all, or just required to be zero?

Please don't encourage incorrect DMA API usage.

dma_map_single() must always be paired with an unmap at some point, otherwise it's a memory leak if DMA API debugging is enabled. So, if you wish to suggest using dma_map_single(), please also suggest where to use dma_unmap_single() too.

Quoted 6 lines · Russell King - ARM Linux

Please do not fiddle about under the covers, there be dragons there.

It looks to me like you're trying to use __cpuc_flush_dcache_area() on an area that's been ioremap()'d, which is a waste of CPU cycles. ioremap()'d areas are mapped as "device" memory type, which means the region isn't even cached. So I don't think this is necessary.

It is mapped cachable, via memremap() (see patch 1/10). This is RAM that the device uses, non-coherently, for its own purposes (but requires software to clear it first).

Is there a non-"under the covers" way to say "flush this region" without the arch second-guessing whether it really needs to be flushed?