Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711

[Jeremy Linton <jeremy.linton@arm.com>]

On 10/8/20 2:43 PM, Ard Biesheuvel wrote:
> (+ Lorenzo)
> 
> On Thu, 8 Oct 2020 at 12:14, Catalin Marinas <catalin.marinas@arm.com> wrote:
>>
>> On Thu, Oct 08, 2020 at 12:05:25PM +0200, Nicolas Saenz Julienne wrote:
>>> On Fri, 2020-10-02 at 12:55 +0100, Catalin Marinas wrote:
>>>> On Thu, Oct 01, 2020 at 07:31:19PM +0200, Nicolas Saenz Julienne wrote:
>>>>> On Thu, 2020-10-01 at 18:23 +0100, Catalin Marinas wrote:
>>>>>> On Thu, Oct 01, 2020 at 06:15:01PM +0100, Catalin Marinas wrote:
>>>>>>> On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz Julienne wrote:
>>>>>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>>>>>> index 4602e467ca8b..cd0d115ef329 100644
>>>>>>>> --- a/drivers/of/fdt.c
>>>>>>>> +++ b/drivers/of/fdt.c
>>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>>   #include <linux/serial_core.h>
>>>>>>>>   #include <linux/sysfs.h>
>>>>>>>>   #include <linux/random.h>
>>>>>>>> +#include <linux/dma-direct.h>      /* for zone_dma_bits */
>>>>>>>>
>>>>>>>>   #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>>>>>>>   #include <asm/page.h>
>>>>>>>> @@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
>>>>>>>>      of_scan_flat_dt(early_init_dt_scan_memory, NULL);
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +void __init early_init_dt_update_zone_dma_bits(void)
>>>>>>>> +{
>>>>>>>> +   unsigned long dt_root = of_get_flat_dt_root();
>>>>>>>> +
>>>>>>>> +   if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
>>>>>>>> +           zone_dma_bits = 30;
>>>>>>>> +}
>>>>>>>
>>>>>>> I think we could keep this entirely in the arm64 setup_machine_fdt() and
>>>>>>> not pollute the core code with RPi4-specific code.
>>>>>>
>>>>>> Actually, even better, could we not move the check to
>>>>>> arm64_memblock_init() when we initialise zone_dma_bits?
>>>>>
>>>>> I did it this way as I vaguely remembered Rob saying he wanted to centralise
>>>>> all early boot fdt code in one place. But I'll be happy to move it there.
>>>>
>>>> I can see Rob replied and I'm fine if that's his preference. However,
>>>> what I don't particularly like is that in the arm64 code, if
>>>> zone_dma_bits == 24, we set it to 32 assuming that it wasn't touched by
>>>> the early_init_dt_update_zone_dma_bits(). What if at some point we'll
>>>> get a platform that actually needs 24 here (I truly hope not, but just
>>>> the principle of relying on magic values)?
>>>>
>>>> So rather than guessing, I'd prefer if the arch code can override
>>>> ZONE_DMA_BITS_DEFAULT. Then, in arm64, we'll just set it to 32 and no
>>>> need to explicitly touch the zone_dma_bits variable.
>>>
>>> Yes, sonds like the way to go. TBH I wasn't happy with that solution either,
>>> but couldn't think of a nicer alternative.
>>>
>>> Sadly I just realised that the series is incomplete, we have RPi4 users that
>>> want to boot unsing ACPI, and this series would break things for them. I'll
>>> have a word with them to see what we can do for their use-case.
>>
>> Is there a way to get some SoC information from ACPI?
>>
> 
> This is unfortunate. We used ACPI _DMA methods as they were designed
> to communicate the DMA limit of the XHCI controller to the OS.
> 
> It shouldn't be too hard to match the OEM id field in the DSDT, and
> switch to the smaller mask. But it sucks to have to add a quirk like
> that.
> It also requires delaying setting the arm64_dma_phy_limit a bit, but 
that doesn't appear to be causing a problem. I've been boot/compiling 
with a patch like:

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index cada0b816c8a..9dfe776c1c75 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -14,6 +14,7 @@

  #include <linux/acpi.h>
  #include <linux/cpumask.h>
+#include <linux/dma-direct.h>
  #include <linux/efi.h>
  #include <linux/efi-bgrt.h>
  #include <linux/init.h>
@@ -168,6 +169,11 @@ static int __init acpi_fadt_sanity_check(void)
                 ret = -EINVAL;
         }

+       if (!strncmp(table->oem_id, "RPIFDN", ACPI_OEM_ID_SIZE) &&
+           !strncmp(table->oem_table_id,  "RPI4    ", 
ACPI_OEM_TABLE_ID_SIZE) &&
+           table->oem_revision <= 0x200)  {
+               zone_dma_bits = 30;
+       }
  out:
         /*
          * acpi_get_table() creates FADT table mapping that
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index cd5caca8a929..6c8aaf1570ce 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -195,6 +195,7 @@ static void __init zone_sizes_init(unsigned long 
min, unsigned long max)
         unsigned long max_zone_pfns[MAX_NR_ZONES]  = {0};

  #ifdef CONFIG_ZONE_DMA
+       arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
         max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
  #endif
  #ifdef CONFIG_ZONE_DMA32
@@ -393,7 +394,6 @@ void __init arm64_memblock_init(void)
                  */
                 if (zone_dma_bits == ZONE_DMA_BITS_DEFAULT)
                         zone_dma_bits = 32;
-               arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
         }

         if (IS_ENABLED(CONFIG_ZONE_DMA32))