diff mbox series

[3/8] of/fdt: add function to get the SoC wide DMA addressable memory size

Message ID 20190731154752.16557-4-nsaenzjulienne@suse.de
State Changes Requested, archived
Headers show
Series Raspberry Pi 4 DMA addressing support | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Nicolas Saenz Julienne July 31, 2019, 3:47 p.m. UTC
Some SoCs might have multiple interconnects each with their own DMA
addressing limitations. This function parses the 'dma-ranges' on each of
them and tries to guess the maximum SoC wide DMA addressable memory
size.

This is specially useful for arch code in order to properly setup CMA
and memory zones.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---

 drivers/of/fdt.c       | 72 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_fdt.h |  2 ++
 2 files changed, 74 insertions(+)

Comments

Rob Herring Aug. 2, 2019, 5:17 p.m. UTC | #1
On Wed, Jul 31, 2019 at 9:48 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Some SoCs might have multiple interconnects each with their own DMA
> addressing limitations. This function parses the 'dma-ranges' on each of
> them and tries to guess the maximum SoC wide DMA addressable memory
> size.
>
> This is specially useful for arch code in order to properly setup CMA
> and memory zones.

We already have a way to setup CMA in reserved-memory, so why is this
needed for that?

I have doubts this can really be generic...

>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>
>  drivers/of/fdt.c       | 72 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_fdt.h |  2 ++
>  2 files changed, 74 insertions(+)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 9cdf14b9aaab..f2444c61a136 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -953,6 +953,78 @@ int __init early_init_dt_scan_chosen_stdout(void)
>  }
>  #endif
>
> +/**
> + * early_init_dt_dma_zone_size - Look at all 'dma-ranges' and provide the
> + * maximum common dmable memory size.
> + *
> + * Some devices might have multiple interconnects each with their own DMA
> + * addressing limitations. For example the Raspberry Pi 4 has the following:
> + *
> + * soc {
> + *     dma-ranges = <0xc0000000  0x0 0x00000000  0x3c000000>;
> + *     [...]
> + * }
> + *
> + * v3dbus {
> + *     dma-ranges = <0x00000000  0x0 0x00000000  0x3c000000>;
> + *     [...]
> + * }
> + *
> + * scb {
> + *     dma-ranges = <0x0 0x00000000  0x0 0x00000000  0xfc000000>;
> + *     [...]
> + * }
> + *
> + * Here the area addressable by all devices is [0x00000000-0x3bffffff]. Hence
> + * the function will write in 'data' a size of 0x3c000000.
> + *
> + * Note that the implementation assumes all interconnects have the same physical
> + * memory view and that the mapping always start at the beginning of RAM.

Not really a valid assumption for general code.

> + */
> +int __init early_init_dt_dma_zone_size(unsigned long node, const char *uname,
> +                                      int depth, void *data)

Don't use the old fdt scanning interface with depth/data. It's not
really needed now because you can just use libfdt calls.

> +{
> +       const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> +       u64 phys_addr, dma_addr, size;
> +       u64 *dma_zone_size = data;
> +       int dma_addr_cells;
> +       const __be32 *reg;
> +       const void *prop;
> +       int len;
> +
> +       if (depth == 0)
> +               *dma_zone_size = 0;
> +
> +       /*
> +        * We avoid pci host controllers as they have their own way of using
> +        * 'dma-ranges'.
> +        */
> +       if (type && !strcmp(type, "pci"))
> +               return 0;
> +
> +       reg = of_get_flat_dt_prop(node, "dma-ranges", &len);
> +       if (!reg)
> +               return 0;
> +
> +       prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
> +       if (prop)
> +               dma_addr_cells = be32_to_cpup(prop);
> +       else
> +               dma_addr_cells = 1; /* arm64's default addr_cell size */

Relying on the defaults has been a dtc error longer than arm64 has
existed. If they are missing, just bail.

> +
> +       if (len < (dma_addr_cells + dt_root_addr_cells + dt_root_size_cells))
> +               return 0;
> +
> +       dma_addr = dt_mem_next_cell(dma_addr_cells, &reg);
> +       phys_addr = dt_mem_next_cell(dt_root_addr_cells, &reg);
> +       size = dt_mem_next_cell(dt_root_size_cells, &reg);
> +
> +       if (!*dma_zone_size || *dma_zone_size > size)
> +               *dma_zone_size = size;
> +
> +       return 0;
> +}

It's possible to have multiple levels of nodes and dma-ranges. You
need to handle that case too.

Doing that and handling differing address translations will be
complicated. IMO, I'd just do:

if (of_fdt_machine_is_compatible(blob, "brcm,bcm2711"))
    dma_zone_size = XX;

2 lines of code is much easier to maintain than 10s of incomplete code
and is clearer who needs this. Maybe if we have dozens of SoCs with
this problem we should start parsing dma-ranges.

Rob
Nicolas Saenz Julienne Aug. 5, 2019, 4:03 p.m. UTC | #2
Hi Rob,
Thanks for the review!

On Fri, 2019-08-02 at 11:17 -0600, Rob Herring wrote:
> On Wed, Jul 31, 2019 at 9:48 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Some SoCs might have multiple interconnects each with their own DMA
> > addressing limitations. This function parses the 'dma-ranges' on each of
> > them and tries to guess the maximum SoC wide DMA addressable memory
> > size.
> > 
> > This is specially useful for arch code in order to properly setup CMA
> > and memory zones.
> 
> We already have a way to setup CMA in reserved-memory, so why is this
> needed for that?

Correct me if I'm wrong but I got the feeling you got the point of the patch
later on.

> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> > 
> >  drivers/of/fdt.c       | 72 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_fdt.h |  2 ++
> >  2 files changed, 74 insertions(+)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 9cdf14b9aaab..f2444c61a136 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -953,6 +953,78 @@ int __init early_init_dt_scan_chosen_stdout(void)
> >  }
> >  #endif
> > 
> > +/**
> > + * early_init_dt_dma_zone_size - Look at all 'dma-ranges' and provide the
> > + * maximum common dmable memory size.
> > + *
> > + * Some devices might have multiple interconnects each with their own DMA
> > + * addressing limitations. For example the Raspberry Pi 4 has the
> > following:
> > + *
> > + * soc {
> > + *     dma-ranges = <0xc0000000  0x0 0x00000000  0x3c000000>;
> > + *     [...]
> > + * }
> > + *
> > + * v3dbus {
> > + *     dma-ranges = <0x00000000  0x0 0x00000000  0x3c000000>;
> > + *     [...]
> > + * }
> > + *
> > + * scb {
> > + *     dma-ranges = <0x0 0x00000000  0x0 0x00000000  0xfc000000>;
> > + *     [...]
> > + * }
> > + *
> > + * Here the area addressable by all devices is [0x00000000-0x3bffffff].
> > Hence
> > + * the function will write in 'data' a size of 0x3c000000.
> > + *
> > + * Note that the implementation assumes all interconnects have the same
> > physical
> > + * memory view and that the mapping always start at the beginning of RAM.
> 
> Not really a valid assumption for general code.

Fair enough. On my defence I settled on that assumption after grepping all dts
and being unable to find a board that behaved otherwise.

[...]

> It's possible to have multiple levels of nodes and dma-ranges. You need to
> handle that case too. Doing that and handling differing address translations
> will be complicated.

Understood.

> IMO, I'd just do:
> 
> if (of_fdt_machine_is_compatible(blob, "brcm,bcm2711"))
>     dma_zone_size = XX;
> 
> 2 lines of code is much easier to maintain than 10s of incomplete code
> and is clearer who needs this. Maybe if we have dozens of SoCs with
> this problem we should start parsing dma-ranges.

FYI that's what arm32 is doing at the moment and was my first instinct. But it
seems that arm64 has been able to survive so far without any machine specific
code and I have the feeling Catalin and Will will not be happy about this
solution. Am I wrong?
Rob Herring Aug. 5, 2019, 7:23 p.m. UTC | #3
On Mon, Aug 5, 2019 at 10:03 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Rob,
> Thanks for the review!
>
> On Fri, 2019-08-02 at 11:17 -0600, Rob Herring wrote:
> > On Wed, Jul 31, 2019 at 9:48 AM Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> wrote:
> > > Some SoCs might have multiple interconnects each with their own DMA
> > > addressing limitations. This function parses the 'dma-ranges' on each of
> > > them and tries to guess the maximum SoC wide DMA addressable memory
> > > size.
> > >
> > > This is specially useful for arch code in order to properly setup CMA
> > > and memory zones.
> >
> > We already have a way to setup CMA in reserved-memory, so why is this
> > needed for that?
>
> Correct me if I'm wrong but I got the feeling you got the point of the patch
> later on.

No, for CMA I don't. Can't we already pass a size and location for CMA
region under /reserved-memory. The only advantage here is perhaps the
CMA range could be anywhere in the DMA zone vs. a fixed location.

> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > ---
> > >
> > >  drivers/of/fdt.c       | 72 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/of_fdt.h |  2 ++
> > >  2 files changed, 74 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 9cdf14b9aaab..f2444c61a136 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -953,6 +953,78 @@ int __init early_init_dt_scan_chosen_stdout(void)
> > >  }
> > >  #endif
> > >
> > > +/**
> > > + * early_init_dt_dma_zone_size - Look at all 'dma-ranges' and provide the
> > > + * maximum common dmable memory size.
> > > + *
> > > + * Some devices might have multiple interconnects each with their own DMA
> > > + * addressing limitations. For example the Raspberry Pi 4 has the
> > > following:
> > > + *
> > > + * soc {
> > > + *     dma-ranges = <0xc0000000  0x0 0x00000000  0x3c000000>;
> > > + *     [...]
> > > + * }
> > > + *
> > > + * v3dbus {
> > > + *     dma-ranges = <0x00000000  0x0 0x00000000  0x3c000000>;
> > > + *     [...]
> > > + * }
> > > + *
> > > + * scb {
> > > + *     dma-ranges = <0x0 0x00000000  0x0 0x00000000  0xfc000000>;
> > > + *     [...]
> > > + * }
> > > + *
> > > + * Here the area addressable by all devices is [0x00000000-0x3bffffff].
> > > Hence
> > > + * the function will write in 'data' a size of 0x3c000000.
> > > + *
> > > + * Note that the implementation assumes all interconnects have the same
> > > physical
> > > + * memory view and that the mapping always start at the beginning of RAM.
> >
> > Not really a valid assumption for general code.
>
> Fair enough. On my defence I settled on that assumption after grepping all dts
> and being unable to find a board that behaved otherwise.
>
> [...]
>
> > It's possible to have multiple levels of nodes and dma-ranges. You need to
> > handle that case too. Doing that and handling differing address translations
> > will be complicated.
>
> Understood.
>
> > IMO, I'd just do:
> >
> > if (of_fdt_machine_is_compatible(blob, "brcm,bcm2711"))
> >     dma_zone_size = XX;
> >
> > 2 lines of code is much easier to maintain than 10s of incomplete code
> > and is clearer who needs this. Maybe if we have dozens of SoCs with
> > this problem we should start parsing dma-ranges.
>
> FYI that's what arm32 is doing at the moment and was my first instinct. But it
> seems that arm64 has been able to survive so far without any machine specific
> code and I have the feeling Catalin and Will will not be happy about this
> solution. Am I wrong?

No doubt. I'm fine if the 2 lines live in drivers/of/.

Note that I'm trying to reduce the number of early_init_dt_scan_*
calls from arch code into the DT code so there's more commonality
across architectures in the early DT scans. So ideally, this can all
be handled under early_init_dt_scan() call.

Rob
Nicolas Saenz Julienne Aug. 6, 2019, 6:12 p.m. UTC | #4
Hi Rob,

On Mon, 2019-08-05 at 13:23 -0600, Rob Herring wrote:
> On Mon, Aug 5, 2019 at 10:03 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Hi Rob,
> > Thanks for the review!
> > 
> > On Fri, 2019-08-02 at 11:17 -0600, Rob Herring wrote:
> > > On Wed, Jul 31, 2019 at 9:48 AM Nicolas Saenz Julienne
> > > <nsaenzjulienne@suse.de> wrote:
> > > > Some SoCs might have multiple interconnects each with their own DMA
> > > > addressing limitations. This function parses the 'dma-ranges' on each of
> > > > them and tries to guess the maximum SoC wide DMA addressable memory
> > > > size.
> > > > 
> > > > This is specially useful for arch code in order to properly setup CMA
> > > > and memory zones.
> > > 
> > > We already have a way to setup CMA in reserved-memory, so why is this
> > > needed for that?
> > 
> > Correct me if I'm wrong but I got the feeling you got the point of the patch
> > later on.
> 
> No, for CMA I don't. Can't we already pass a size and location for CMA
> region under /reserved-memory. The only advantage here is perhaps the
> CMA range could be anywhere in the DMA zone vs. a fixed location.

Now I get it, sorry I wasn't aware of that interface.

Still, I'm not convinced it matches RPi's use case as this would hard-code
CMA's size. Most people won't care, but for the ones that do, it's nicer to
change the value from the kernel command line than editing the dtb. I get that
if you need to, for example, reserve some memory for the video to work, it's
silly not to hard-code it. Yet due to the board's nature and users base I say
it's important to favor flexibility. It would also break compatibility with
earlier versions of the board and diverge from the downstream kernel behaviour.
Which is a bigger issue than it seems as most users don't always understand
which kernel they are running and unknowingly copy configuration options from
forums.

As I also need to know the DMA addressing limitations to properly configure
memory zones and dma-direct. Setting up the proper CMA constraints during the
arch's init will be trivial anyway.

> > > IMO, I'd just do:
> > > 
> > > if (of_fdt_machine_is_compatible(blob, "brcm,bcm2711"))
> > >     dma_zone_size = XX;
> > > 
> > > 2 lines of code is much easier to maintain than 10s of incomplete code
> > > and is clearer who needs this. Maybe if we have dozens of SoCs with
> > > this problem we should start parsing dma-ranges.
> > 
> > FYI that's what arm32 is doing at the moment and was my first instinct. But
> > it
> > seems that arm64 has been able to survive so far without any machine
> > specific
> > code and I have the feeling Catalin and Will will not be happy about this
> > solution. Am I wrong?
> 
> No doubt. I'm fine if the 2 lines live in drivers/of/.
> 
> Note that I'm trying to reduce the number of early_init_dt_scan_*
> calls from arch code into the DT code so there's more commonality
> across architectures in the early DT scans. So ideally, this can all
> be handled under early_init_dt_scan() call.

How does this look? (I'll split it in two patches and add a comment explaining
why dt_dma_zone_size is needed)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index f2444c61a136..1395be40b722 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -30,6 +30,8 @@
 
 #include "of_private.h"
 
+u64 dt_dma_zone_size __ro_after_init;
+
 /*
  * of_fdt_limit_memory - limit the number of regions in the /memory node
  * @limit: maximum entries
@@ -802,6 +805,11 @@ const char * __init of_flat_dt_get_machine_name(void)
        return name;
 }
 
+static const int __init of_fdt_machine_is_compatible(char *name)
+{
+       return of_compat_cmp(of_flat_dt_get_machine_name(), name, strlen(name));
+}
+
 /**
  * of_flat_dt_match_machine - Iterate match tables to find matching machine.
  *
@@ -1260,6 +1268,14 @@ void __init early_init_dt_scan_nodes(void)
        of_scan_flat_dt(early_init_dt_scan_memory, NULL);
 }
 
+void __init early_init_dt_get_dma_zone_size(void)
+{
+       dt_dma_zone_size = 0;
+
+       if (of_fdt_machine_is_compatible("brcm,bcm2711"))
+               dt_dma_zone_size = 0x3c000000;
+}
+
 bool __init early_init_dt_scan(void *params)
 {
        bool status;
@@ -1269,6 +1285,7 @@ bool __init early_init_dt_scan(void *params)
                return false;
 
        early_init_dt_scan_nodes();
+       early_init_dt_get_dma_zone_size();
        return true;
 }
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 2ad36b7bd4fa..b5a9f685de14 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -27,6 +27,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
                                   struct device_node *dad,
                                   struct device_node **mynodes);
 
+extern u64 dt_dma_zone_size __ro_after_init;
+
 /* TBD: Temporary export of fdt globals - remove when code fully merged */
 extern int __initdata dt_root_addr_cells;
 extern int __initdata dt_root_size_cells;

 
Regards,
Nicolas
Rob Herring Aug. 8, 2019, 3:02 p.m. UTC | #5
On Tue, Aug 6, 2019 at 12:12 PM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Rob,
>
> On Mon, 2019-08-05 at 13:23 -0600, Rob Herring wrote:
> > On Mon, Aug 5, 2019 at 10:03 AM Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> wrote:
> > > Hi Rob,
> > > Thanks for the review!
> > >
> > > On Fri, 2019-08-02 at 11:17 -0600, Rob Herring wrote:
> > > > On Wed, Jul 31, 2019 at 9:48 AM Nicolas Saenz Julienne
> > > > <nsaenzjulienne@suse.de> wrote:
> > > > > Some SoCs might have multiple interconnects each with their own DMA
> > > > > addressing limitations. This function parses the 'dma-ranges' on each of
> > > > > them and tries to guess the maximum SoC wide DMA addressable memory
> > > > > size.
> > > > >
> > > > > This is specially useful for arch code in order to properly setup CMA
> > > > > and memory zones.
> > > >
> > > > We already have a way to setup CMA in reserved-memory, so why is this
> > > > needed for that?
> > >
> > > Correct me if I'm wrong but I got the feeling you got the point of the patch
> > > later on.
> >
> > No, for CMA I don't. Can't we already pass a size and location for CMA
> > region under /reserved-memory. The only advantage here is perhaps the
> > CMA range could be anywhere in the DMA zone vs. a fixed location.
>
> Now I get it, sorry I wasn't aware of that interface.
>
> Still, I'm not convinced it matches RPi's use case as this would hard-code
> CMA's size. Most people won't care, but for the ones that do, it's nicer to
> change the value from the kernel command line than editing the dtb.

Sure, I fully agree and am not a fan of the CMA DT overlays I've seen.

> I get that
> if you need to, for example, reserve some memory for the video to work, it's
> silly not to hard-code it. Yet due to the board's nature and users base I say
> it's important to favor flexibility. It would also break compatibility with
> earlier versions of the board and diverge from the downstream kernel behaviour.
> Which is a bigger issue than it seems as most users don't always understand
> which kernel they are running and unknowingly copy configuration options from
> forums.
>
> As I also need to know the DMA addressing limitations to properly configure
> memory zones and dma-direct. Setting up the proper CMA constraints during the
> arch's init will be trivial anyway.

It was really just commentary on commit text as for CMA alone we have
a solution already. I agree on the need for zones.

>
> > > > IMO, I'd just do:
> > > >
> > > > if (of_fdt_machine_is_compatible(blob, "brcm,bcm2711"))
> > > >     dma_zone_size = XX;
> > > >
> > > > 2 lines of code is much easier to maintain than 10s of incomplete code
> > > > and is clearer who needs this. Maybe if we have dozens of SoCs with
> > > > this problem we should start parsing dma-ranges.
> > >
> > > FYI that's what arm32 is doing at the moment and was my first instinct. But
> > > it
> > > seems that arm64 has been able to survive so far without any machine
> > > specific
> > > code and I have the feeling Catalin and Will will not be happy about this
> > > solution. Am I wrong?
> >
> > No doubt. I'm fine if the 2 lines live in drivers/of/.
> >
> > Note that I'm trying to reduce the number of early_init_dt_scan_*
> > calls from arch code into the DT code so there's more commonality
> > across architectures in the early DT scans. So ideally, this can all
> > be handled under early_init_dt_scan() call.
>
> How does this look? (I'll split it in two patches and add a comment explaining
> why dt_dma_zone_size is needed)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index f2444c61a136..1395be40b722 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -30,6 +30,8 @@
>
>  #include "of_private.h"
>
> +u64 dt_dma_zone_size __ro_after_init;

Avoiding a call from arch code by just having a variable isn't really
better. I'd rather see a common, non DT specific variable that can be
adjusted. Something similar to initrd_start/end. Then the arch code
doesn't have to care what hardware description code adjusted the
value.

Rob
Nicolas Saenz Julienne Aug. 8, 2019, 5:30 p.m. UTC | #6
On Thu, 2019-08-08 at 09:02 -0600, Rob Herring wrote:
> On Tue, Aug 6, 2019 at 12:12 PM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Hi Rob,
> > 
> > On Mon, 2019-08-05 at 13:23 -0600, Rob Herring wrote:
> > > On Mon, Aug 5, 2019 at 10:03 AM Nicolas Saenz Julienne
> > > <nsaenzjulienne@suse.de> wrote:
> > > > Hi Rob,
> > > > Thanks for the review!
> > > > 
> > > > On Fri, 2019-08-02 at 11:17 -0600, Rob Herring wrote:
> > > > > On Wed, Jul 31, 2019 at 9:48 AM Nicolas Saenz Julienne
> > > > > <nsaenzjulienne@suse.de> wrote:
> > > > > > Some SoCs might have multiple interconnects each with their own DMA
> > > > > > addressing limitations. This function parses the 'dma-ranges' on
> > > > > > each of
> > > > > > them and tries to guess the maximum SoC wide DMA addressable memory
> > > > > > size.
> > > > > > 
> > > > > > This is specially useful for arch code in order to properly setup
> > > > > > CMA
> > > > > > and memory zones.
> > > > > 
> > > > > We already have a way to setup CMA in reserved-memory, so why is this
> > > > > needed for that?
> > > > 
> > > > Correct me if I'm wrong but I got the feeling you got the point of the
> > > > patch
> > > > later on.
> > > 
> > > No, for CMA I don't. Can't we already pass a size and location for CMA
> > > region under /reserved-memory. The only advantage here is perhaps the
> > > CMA range could be anywhere in the DMA zone vs. a fixed location.
> > 
> > Now I get it, sorry I wasn't aware of that interface.
> > 
> > Still, I'm not convinced it matches RPi's use case as this would hard-code
> > CMA's size. Most people won't care, but for the ones that do, it's nicer to
> > change the value from the kernel command line than editing the dtb.
> 
> Sure, I fully agree and am not a fan of the CMA DT overlays I've seen.
> 
> > I get that
> > if you need to, for example, reserve some memory for the video to work, it's
> > silly not to hard-code it. Yet due to the board's nature and users base I
> > say
> > it's important to favor flexibility. It would also break compatibility with
> > earlier versions of the board and diverge from the downstream kernel
> > behaviour.
> > Which is a bigger issue than it seems as most users don't always understand
> > which kernel they are running and unknowingly copy configuration options
> > from
> > forums.
> > 
> > As I also need to know the DMA addressing limitations to properly configure
> > memory zones and dma-direct. Setting up the proper CMA constraints during
> > the
> > arch's init will be trivial anyway.
> 
> It was really just commentary on commit text as for CMA alone we have
> a solution already. I agree on the need for zones.

Ok, understood :)

> > > > > IMO, I'd just do:
> > > > > 
> > > > > if (of_fdt_machine_is_compatible(blob, "brcm,bcm2711"))
> > > > >     dma_zone_size = XX;
> > > > > 
> > > > > 2 lines of code is much easier to maintain than 10s of incomplete code
> > > > > and is clearer who needs this. Maybe if we have dozens of SoCs with
> > > > > this problem we should start parsing dma-ranges.
> > > > 
> > > > FYI that's what arm32 is doing at the moment and was my first instinct.
> > > > But
> > > > it
> > > > seems that arm64 has been able to survive so far without any machine
> > > > specific
> > > > code and I have the feeling Catalin and Will will not be happy about
> > > > this
> > > > solution. Am I wrong?
> > > 
> > > No doubt. I'm fine if the 2 lines live in drivers/of/.
> > > 
> > > Note that I'm trying to reduce the number of early_init_dt_scan_*
> > > calls from arch code into the DT code so there's more commonality
> > > across architectures in the early DT scans. So ideally, this can all
> > > be handled under early_init_dt_scan() call.
> > 
> > How does this look? (I'll split it in two patches and add a comment
> > explaining
> > why dt_dma_zone_size is needed)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index f2444c61a136..1395be40b722 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -30,6 +30,8 @@
> > 
> >  #include "of_private.h"
> > 
> > +u64 dt_dma_zone_size __ro_after_init;
> 
> Avoiding a call from arch code by just having a variable isn't really
> better. I'd rather see a common, non DT specific variable that can be
> adjusted. Something similar to initrd_start/end. Then the arch code
> doesn't have to care what hardware description code adjusted the
> value.

Way better, I'll update it.
diff mbox series

Patch

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 9cdf14b9aaab..f2444c61a136 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -953,6 +953,78 @@  int __init early_init_dt_scan_chosen_stdout(void)
 }
 #endif
 
+/**
+ * early_init_dt_dma_zone_size - Look at all 'dma-ranges' and provide the
+ * maximum common dmable memory size.
+ *
+ * Some devices might have multiple interconnects each with their own DMA
+ * addressing limitations. For example the Raspberry Pi 4 has the following:
+ *
+ * soc {
+ *	dma-ranges = <0xc0000000  0x0 0x00000000  0x3c000000>;
+ *	[...]
+ * }
+ *
+ * v3dbus {
+ *	dma-ranges = <0x00000000  0x0 0x00000000  0x3c000000>;
+ *	[...]
+ * }
+ *
+ * scb {
+ *	dma-ranges = <0x0 0x00000000  0x0 0x00000000  0xfc000000>;
+ *	[...]
+ * }
+ *
+ * Here the area addressable by all devices is [0x00000000-0x3bffffff]. Hence
+ * the function will write in 'data' a size of 0x3c000000.
+ *
+ * Note that the implementation assumes all interconnects have the same physical
+ * memory view and that the mapping always start at the beginning of RAM.
+ */
+int __init early_init_dt_dma_zone_size(unsigned long node, const char *uname,
+				       int depth, void *data)
+{
+	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
+	u64 phys_addr, dma_addr, size;
+	u64 *dma_zone_size = data;
+	int dma_addr_cells;
+	const __be32 *reg;
+	const void *prop;
+	int len;
+
+	if (depth == 0)
+		*dma_zone_size = 0;
+
+	/*
+	 * We avoid pci host controllers as they have their own way of using
+	 * 'dma-ranges'.
+	 */
+	if (type && !strcmp(type, "pci"))
+		return 0;
+
+	reg = of_get_flat_dt_prop(node, "dma-ranges", &len);
+	if (!reg)
+		return 0;
+
+	prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
+	if (prop)
+		dma_addr_cells = be32_to_cpup(prop);
+	else
+		dma_addr_cells = 1; /* arm64's default addr_cell size */
+
+	if (len < (dma_addr_cells + dt_root_addr_cells + dt_root_size_cells))
+		return 0;
+
+	dma_addr = dt_mem_next_cell(dma_addr_cells, &reg);
+	phys_addr = dt_mem_next_cell(dt_root_addr_cells, &reg);
+	size = dt_mem_next_cell(dt_root_size_cells, &reg);
+
+	if (!*dma_zone_size || *dma_zone_size > size)
+		*dma_zone_size = size;
+
+	return 0;
+}
+
 /**
  * early_init_dt_scan_root - fetch the top level address and size cells
  */
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index acf820e88952..2ad36b7bd4fa 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -72,6 +72,8 @@  extern int early_init_dt_reserve_memory_arch(phys_addr_t base, phys_addr_t size,
 					     bool no_map);
 extern u64 dt_mem_next_cell(int s, const __be32 **cellp);
 
+extern int early_init_dt_dma_zone_size(unsigned long node, const char *uname,
+				       int depth, void *data);
 /* Early flat tree scan hooks */
 extern int early_init_dt_scan_root(unsigned long node, const char *uname,
 				   int depth, void *data);