| Message ID | 20190820145821.27214-1-nsaenzjulienne@suse.de |
|---|---|
| Headers | show |
| Series | Raspberry Pi 4 DMA addressing support | expand |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Aug 20, 2019 at 04:58:18PM +0200, Nicolas Saenz Julienne wrote: > - if (IS_ENABLED(CONFIG_ZONE_DMA)) > + if (IS_ENABLED(CONFIG_ZONE_DMA)) { > arm64_dma_phys_limit = max_zone_dma_phys(); > + zone_dma_bits = ilog2((arm64_dma_phys_limit - 1) & GENMASK_ULL(31, 0)) + 1; This adds a way too long line. I also find the use of GENMASK_ULL horribly obsfucating, but I know that opinion is't shared by everyone.
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Aug 20, 2019 at 04:58:09PM +0200, Nicolas Saenz Julienne wrote: > Some architectures have platform specific DMA addressing limitations. > This will allow for hardware description code to provide the constraints > in a generic manner, so as for arch code to properly setup it's memory > zones and DMA mask. I know this just spreads the arm code, but I still kinda hate it. MAX_DMA_ADDRESS is such an oddly defined concepts. We have the mm code that uses it to start allocating after the dma zones, but I think that would better be done using a function returning 1 << max(zone_dma_bits, 32) or so. Then we have about a handful of drivers using it that all seem rather bogus, and one of which I think are usable on arm64.
On Mon, 2019-08-26 at 09:06 +0200, Christoph Hellwig wrote: > On Tue, Aug 20, 2019 at 04:58:18PM +0200, Nicolas Saenz Julienne wrote: > > - if (IS_ENABLED(CONFIG_ZONE_DMA)) > > + if (IS_ENABLED(CONFIG_ZONE_DMA)) { > > arm64_dma_phys_limit = max_zone_dma_phys(); > > + zone_dma_bits = ilog2((arm64_dma_phys_limit - 1) & > > GENMASK_ULL(31, 0)) + 1; > Hi Christoph, thanks for the rewiews. > This adds a way too long line. I know, I couldn't find a way to split the operation without making it even harder to read. I'll find a solution. > I also find the use of GENMASK_ULL > horribly obsfucating, but I know that opinion is't shared by everyone. Don't have any preference so I'll happily change it. Any suggestions? Using the explicit 0xffffffffULL seems hard to read, how about SZ_4GB - 1?
Nicolas Saenz Julienne <nsaenzjulienne@suse.de> writes: > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index 0d52f57fca04..73668a21ae78 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -319,13 +319,4 @@ struct vm_area_struct; > #endif /* __ASSEMBLY__ */ > #include <asm/slice.h> > > -/* > - * Allow 30-bit DMA for very limited Broadcom wifi chips on many powerbooks. This comment got lost. > diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c > index 9191a66b3bc5..2a69f87585df 100644 > --- a/arch/powerpc/mm/mem.c > +++ b/arch/powerpc/mm/mem.c > @@ -237,9 +238,14 @@ void __init paging_init(void) > printk(KERN_DEBUG "Memory hole size: %ldMB\n", > (long int)((top_of_ram - total_ram) >> 20)); > > + if (IS_ENABLED(CONFIG_PPC32)) Can you please propagate it here? > + zone_dma_bits = 30; > + else > + zone_dma_bits = 31; > + cheers
On Mon, 2019-08-26 at 09:09 +0200, Christoph Hellwig wrote: > On Tue, Aug 20, 2019 at 04:58:09PM +0200, Nicolas Saenz Julienne wrote: > > Some architectures have platform specific DMA addressing limitations. > > This will allow for hardware description code to provide the constraints > > in a generic manner, so as for arch code to properly setup it's memory > > zones and DMA mask. > > I know this just spreads the arm code, but I still kinda hate it. Rob's main concern was finding a way to pass the constraint from HW definition to arch without widening fdt's architecture specific function surface. I'd say it's fair to argue that having a generic mechanism makes sense as it'll now traverse multiple archs and subsystems. I get adding globals like this is not very appealing, yet I went with it as it was the easier to integrate with arm's code. Any alternative suggestions? > MAX_DMA_ADDRESS is such an oddly defined concepts. We have the mm > code that uses it to start allocating after the dma zones, but > I think that would better be done using a function returning > 1 << max(zone_dma_bits, 32) or so. Then we have about a handful > of drivers using it that all seem rather bogus, and one of which > I think are usable on arm64. Is it safe to assume DMA limitations will always be a power of 2? I ask as RPi4 kinda isn't: ZONE_DMA is 0x3c000000 bytes big, I'm approximating the zone mask to 30 as [0x3c000000 0x3fffffff] isn't defined as memory so it's unlikely that we´ll encounter buffers there. But I don't know how it could affect mm initialization code. This also rules out 'zone_dma_bits' as a mechanism to pass ZONE_DMA's size from HW definition code to arch's.
On Mon, 26 Aug 2019 13:08:50 +0200 Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > On Mon, 2019-08-26 at 09:06 +0200, Christoph Hellwig wrote: > > On Tue, Aug 20, 2019 at 04:58:18PM +0200, Nicolas Saenz Julienne wrote: > > > - if (IS_ENABLED(CONFIG_ZONE_DMA)) > > > + if (IS_ENABLED(CONFIG_ZONE_DMA)) { > > > arm64_dma_phys_limit = max_zone_dma_phys(); > > > + zone_dma_bits = ilog2((arm64_dma_phys_limit - 1) & > > > GENMASK_ULL(31, 0)) + 1; > > > Hi Christoph, > thanks for the rewiews. > > > This adds a way too long line. > > I know, I couldn't find a way to split the operation without making it even > harder to read. I'll find a solution. If all else fails, move the code to an inline function and call it e.g. phys_limit_to_dma_bits(). Just my two cents, Petr T
On Mon, 2019-08-26 at 15:46 +0200, Nicolas Saenz Julienne wrote: > On Mon, 2019-08-26 at 09:09 +0200, Christoph Hellwig wrote: > > On Tue, Aug 20, 2019 at 04:58:09PM +0200, Nicolas Saenz Julienne wrote: > > > Some architectures have platform specific DMA addressing limitations. > > > This will allow for hardware description code to provide the constraints > > > in a generic manner, so as for arch code to properly setup it's memory > > > zones and DMA mask. > > > > I know this just spreads the arm code, but I still kinda hate it. > > Rob's main concern was finding a way to pass the constraint from HW definition > to arch without widening fdt's architecture specific function surface. I'd say > it's fair to argue that having a generic mechanism makes sense as it'll now > traverse multiple archs and subsystems. > > I get adding globals like this is not very appealing, yet I went with it as it > was the easier to integrate with arm's code. Any alternative suggestions? > > > MAX_DMA_ADDRESS is such an oddly defined concepts. We have the mm > > code that uses it to start allocating after the dma zones, but > > I think that would better be done using a function returning > > 1 << max(zone_dma_bits, 32) or so. Then we have about a handful > > of drivers using it that all seem rather bogus, and one of which > > I think are usable on arm64. > > Is it safe to assume DMA limitations will always be a power of 2? I ask as > RPi4 > kinda isn't: ZONE_DMA is 0x3c000000 bytes big, I'm approximating the zone mask > to 30 as [0x3c000000 0x3fffffff] isn't defined as memory so it's unlikely that > we´ll encounter buffers there. But I don't know how it could affect mm > initialization code. > > This also rules out 'zone_dma_bits' as a mechanism to pass ZONE_DMA's size > from > HW definition code to arch's. Hi Christoph, I gave it a thought and think this whole MAX_DMA_ADDRESS topic falls out of the scope of the series. I agree it's something that we should get rid of, but fixing it isn't going to affect the overall enhancement intended here. I'd rather focus on how are we going to pass the DMA zone data into the arch code and fix MAX_DMA_ADDRESS on another series. Regards, Nicolas
On Mon, Aug 26, 2019 at 03:46:52PM +0200, Nicolas Saenz Julienne wrote: > On Mon, 2019-08-26 at 09:09 +0200, Christoph Hellwig wrote: > > On Tue, Aug 20, 2019 at 04:58:09PM +0200, Nicolas Saenz Julienne wrote: > > > Some architectures have platform specific DMA addressing limitations. > > > This will allow for hardware description code to provide the constraints > > > in a generic manner, so as for arch code to properly setup it's memory > > > zones and DMA mask. > > > > I know this just spreads the arm code, but I still kinda hate it. > > Rob's main concern was finding a way to pass the constraint from HW definition > to arch without widening fdt's architecture specific function surface. I'd say > it's fair to argue that having a generic mechanism makes sense as it'll now > traverse multiple archs and subsystems. > > I get adding globals like this is not very appealing, yet I went with it as it > was the easier to integrate with arm's code. Any alternative suggestions? In some discussion with Robin, since it's just RPi4 that we are aware of having such requirement on arm64, he suggested that we have a permanent ZONE_DMA on arm64 with a default size of 1GB. It should cover all arm64 SoCs we know of without breaking the single Image binary. The arch/arm can use its current mach-* support. I may like this more than the proposed early_init_dt_get_dma_zone_size() here which checks for specific SoCs (my preferred way was to build the mask from all buses described in DT but I hadn't realised the complications).
On Fri, 2019-08-30 at 15:45 +0100, Catalin Marinas wrote: > On Mon, Aug 26, 2019 at 03:46:52PM +0200, Nicolas Saenz Julienne wrote: > > On Mon, 2019-08-26 at 09:09 +0200, Christoph Hellwig wrote: > > > On Tue, Aug 20, 2019 at 04:58:09PM +0200, Nicolas Saenz Julienne wrote: > > > > Some architectures have platform specific DMA addressing limitations. > > > > This will allow for hardware description code to provide the constraints > > > > in a generic manner, so as for arch code to properly setup it's memory > > > > zones and DMA mask. > > > > > > I know this just spreads the arm code, but I still kinda hate it. > > > > Rob's main concern was finding a way to pass the constraint from HW > > definition > > to arch without widening fdt's architecture specific function surface. I'd > > say > > it's fair to argue that having a generic mechanism makes sense as it'll now > > traverse multiple archs and subsystems. > > > > I get adding globals like this is not very appealing, yet I went with it as > > it > > was the easier to integrate with arm's code. Any alternative suggestions? > > In some discussion with Robin, since it's just RPi4 that we are aware of > having such requirement on arm64, he suggested that we have a permanent > ZONE_DMA on arm64 with a default size of 1GB. It should cover all arm64 > SoCs we know of without breaking the single Image binary. The arch/arm > can use its current mach-* support. > > I may like this more than the proposed early_init_dt_get_dma_zone_size() > here which checks for specific SoCs (my preferred way was to build the > mask from all buses described in DT but I hadn't realised the > complications). Hi Catalin, thanks for giving it a thought. I'll be happy to implement it that way. I agree it's a good compromise. @Christoph, do you still want the patch where I create 'zone_dma_bits'? With a hardcoded ZONE_DMA it's not absolutely necessary. Though I remember you said it was a first step towards being able to initialize dma-direct's min_mask in meminit. Regards, Nicolas
On Fri, Aug 30, 2019 at 07:24:25PM +0200, Nicolas Saenz Julienne wrote: > I'll be happy to implement it that way. I agree it's a good compromise. > > @Christoph, do you still want the patch where I create 'zone_dma_bits'? With a > hardcoded ZONE_DMA it's not absolutely necessary. Though I remember you said it > was a first step towards being able to initialize dma-direct's min_mask in > meminit. I do like the variable better than the current #define. I wonder if really want a mask or a max_zone_dma_address like variable. So for this series feel free to drop the patch. I'll see if I'll pick it up later or if we can find some way to automatically propagate that information from the zone initialization.
Hi all, this series attempts to address some issues we found while bringing up the new Raspberry Pi 4 in arm64 and it's intended to serve as a follow up of these discussions: v1: https://lkml.org/lkml/2019/7/31/922 RFC: https://lkml.org/lkml/2019/7/17/476 The new Raspberry Pi 4 has up to 4GB of memory but most peripherals can only address the first GB: their DMA address range is 0xc0000000-0xfc000000 which is aliased to the first GB of physical memory 0x00000000-0x3c000000. Note that only some peripherals have these limitations: the PCIe, V3D, GENET, and 40-bit DMA channels have a wider view of the address space by virtue of being hooked up trough a second interconnect. Part of this is solved in arm32 by setting up the machine specific '.dma_zone_size = SZ_1G', which takes care of reserving the coherent memory area at the right spot. That said no buffer bouncing (needed for dma streaming) is available at the moment, but that's a story for another series. Unfortunately there is no such thing as 'dma_zone_size' in arm64. Only ZONE_DMA32 is created which is interpreted by dma-direct and the arm64 arch code as if all peripherals where be able to address the first 4GB of memory. In the light of this, the series implements the following changes: - Create generic 'dma_zone_size' in order for hardware description code to set it up when needed. - Add a function in early_init_dt_scan() to setup 'dma_zone_size' for the RPi4. - Create both DMA zones in arm64, ZONE_DMA will contain the area addressable by all peripherals and ZONE_DMA32 the rest of the 32 bit addressable memory. ZONE_DMA32 might be left empty. - Reserve the CMA area in a place suitable for all peripherals. - Inform dma-direct of the new runtime calculated min_mask. This series has been tested on multiple devices both by checking the zones setup matches the expectations and by double-checking physical addresses on pages allocated on the three relevant areas GFP_DMA, GFP_DMA32, GFP_KERNEL: - On an RPi4 with variations on the ram memory size. But also forcing the situation where all three memory zones are nonempty by setting a 3G ZONE_DMA32 ceiling on a 4G setup. Both with and without NUMA support. - On a Synquacer box[1] with 32G of memory. - On an ACPI based Huawei TaiShan server[2] with 256G of memory. - On a QEMU virtual machine running arm64's OpenSUSE Tumbleweed. That's all. Regards, Nicolas [1] https://www.96boards.org/product/developerbox/ [2] https://e.huawei.com/en/products/cloud-computing-dc/servers/taishan-server/taishan-2280-v2 --- Changes in v2: - More in depth testing. - Create new global 'dma_zone_size'. - New approach to getting the dma_zone_size, instead of parsing the dts we hardcode it conditionally to the machine compatible name. - Fix ZONE_DMA and ZONE_DMA32 split, now ZONE_DMA32 remains empty if ZONE_DMA fits the whole 32 bit addressable space. - Take into account devices with DMA offset. - Rename new dma-direct variable to zone_dma_bits. - Try new approach by merging both ZONE_DMA and ZONE_DMA32 comments in mmzone.h, add new up to date examples. Nicolas Saenz Julienne (11): asm-generic: add dma_zone_size arm: use generic dma_zone_size of/fdt: add of_fdt_machine_is_compatible function of/fdt: add early_init_dt_get_dma_zone_size() arm64: mm: use arm64_dma_phys_limit instead of calling max_zone_dma_phys() arm64: rename variables used to calculate ZONE_DMA32's size arm64: re-introduce max_zone_dma_phys() arm64: use both ZONE_DMA and ZONE_DMA32 dma-direct: turn ARCH_ZONE_DMA_BITS into a variable arm64: edit zone_dma_bits to fine tune dma-direct min mask mm: refresh ZONE_DMA and ZONE_DMA32 comments in 'enum zone_type' arch/arm/include/asm/dma.h | 8 ++-- arch/arm/mm/init.c | 12 ++---- arch/arm64/Kconfig | 4 ++ arch/arm64/mm/init.c | 73 +++++++++++++++++++++++++-------- arch/powerpc/include/asm/page.h | 9 ---- arch/powerpc/mm/mem.c | 16 +++++--- arch/s390/include/asm/page.h | 2 - arch/s390/mm/init.c | 1 + drivers/of/fdt.c | 15 +++++++ include/asm-generic/dma.h | 8 +++- include/linux/dma-direct.h | 2 + include/linux/mmzone.h | 46 ++++++++++++--------- kernel/dma/direct.c | 13 +++--- mm/page_alloc.c | 3 ++ 14 files changed, 140 insertions(+), 72 deletions(-)