| Message ID | 20221103133900.1473855-3-thierry.reding@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iommu: Support mappings/reservations in reserved-memory regions | expand |
On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > DMA parent devices can define separate DMA busses via the "dma-ranges" > and "#address-cells" and "#size-cells" properties. If the DMA bus has > different cell counts than its parent, this can cause the translation > of DMA address to fails (e.g. truncation from 2 to 1 address cells). My assumption in this case was that the parent cell sizes should be increased to 2 cells. That tends to be what people want to do anyways (64-bit everywhere on 64-bit CPUs). > Avoid this by stopping to search for DMA parents when a parent without > a "dma-ranges" property is encountered. Also, since it is the DMA parent > that defines the DMA bus, use the bus' cell counts instead of its parent > cell counts. We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec even says that because I hit that case. Is this going to work for 'dma-device' with something like this?: bus@0 { dma-ranges = <...>; child-bus@... { dma-device@... { }; }; }; > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Changes in v10: > - new patch to avoid address truncation when traversing a bus hierarchy > with mismatching #address-cells properties > > Example from Tegra194 (redacted for clarity): > > reserved-memory { > #address-cells = <2>; > #size-cells = <2>; > ranges; > > framebuffer@0,0 { > compatible = "framebuffer"; > reg = <0x2 0x57320000 0x0 0x00800000>; > iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>; > }; > }; > > bus@0 { > /* truncation happens here */ > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x0 0x0 0x0 0x40000000>; > > mc: memory-controller@2c00000 { > #address-cells = <2>; > #size-cells = <2>; I think this is wrong. The parent should have more or equal number of cells. > /* > * memory controller provides access to 512 GiB > * of system RAM (root of the DMA bus) > */ > dma-ranges = <0x0 0x0 0x0 0x80 0x0>; > }; > > host1x@13e00000 { > display-hub@15200000 { > display@15200000 { > interconnect-names = "dma-mem", ...; > interconnects = <&mc ...>; > memory-region = <&fb>; > }; > }; > }; > }; > > During DMA address translation, the framebuffer address (0x257320000) > will first be translated to the DMA parent's DMA bus, which yields the > same value. After that, the current translation code will switch to the > control bus of bus@0 and then the address will be truncated to > 0x57320000 due to #address-cells = <1>. > > The idea of this patch is to interrupt DMA address translation at &mc > because it is the root of the DMA bus (i.e. its parent does not have a > dma-ranges property) so that the control bus' #address-cells doesn't > truncate the DMA address. > > drivers/of/address.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 14f137a21b0c..e2f45bdbc41a 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -475,6 +475,7 @@ static u64 __of_translate_address(struct device_node *dev, > const __be32 *in_addr, const char *rprop, > struct device_node **host) > { > + bool dma = rprop && !strcmp(rprop, "dma-ranges"); > struct device_node *parent = NULL; > struct of_bus *bus, *pbus; > __be32 addr[OF_MAX_ADDR_CELLS]; > @@ -494,7 +495,12 @@ static u64 __of_translate_address(struct device_node *dev, > bus = of_match_bus(parent); > > /* Count address cells & copy address locally */ > - bus->count_cells(dev, &na, &ns); > + if (dma) { > + na = of_bus_n_addr_cells(parent); > + ns = of_bus_n_size_cells(parent); > + } else { > + bus->count_cells(dev, &na, &ns); > + } > if (!OF_CHECK_COUNTS(na, ns)) { > pr_debug("Bad cell count for %pOF\n", dev); > goto bail; > @@ -515,7 +521,7 @@ static u64 __of_translate_address(struct device_node *dev, > parent = get_parent(dev); > > /* If root, we have finished */ > - if (parent == NULL) { > + if (parent == NULL || (dma && !of_get_property(parent, "dma-ranges", NULL))) { > pr_debug("reached root node\n"); > result = of_read_number(addr, na); > break; > @@ -536,7 +542,12 @@ static u64 __of_translate_address(struct device_node *dev, > > /* Get new parent bus and counts */ > pbus = of_match_bus(parent); > - pbus->count_cells(dev, &pna, &pns); > + if (dma) { > + pna = of_bus_n_addr_cells(parent); > + pns = of_bus_n_size_cells(parent); > + } else { > + pbus->count_cells(dev, &pna, &pns); > + } > if (!OF_CHECK_COUNTS(pna, pns)) { > pr_err("Bad cell count for %pOF\n", dev); > break; > -- > 2.38.1 > >
On Mon, Nov 07, 2022 at 01:30:35PM -0600, Rob Herring wrote: > On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > DMA parent devices can define separate DMA busses via the "dma-ranges" > > and "#address-cells" and "#size-cells" properties. If the DMA bus has > > different cell counts than its parent, this can cause the translation > > of DMA address to fails (e.g. truncation from 2 to 1 address cells). > > My assumption in this case was that the parent cell sizes should be > increased to 2 cells. That tends to be what people want to do anyways > (64-bit everywhere on 64-bit CPUs). > > > Avoid this by stopping to search for DMA parents when a parent without > > a "dma-ranges" property is encountered. Also, since it is the DMA parent > > that defines the DMA bus, use the bus' cell counts instead of its parent > > cell counts. > > We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec > even says that because I hit that case. > > Is this going to work for 'dma-device' with something like this?: > > bus@0 { > dma-ranges = <...>; > child-bus@... { > dma-device@... { > }; > }; > }; > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > Changes in v10: > > - new patch to avoid address truncation when traversing a bus hierarchy > > with mismatching #address-cells properties > > > > Example from Tegra194 (redacted for clarity): > > > > reserved-memory { > > #address-cells = <2>; > > #size-cells = <2>; > > ranges; > > > > framebuffer@0,0 { > > compatible = "framebuffer"; > > reg = <0x2 0x57320000 0x0 0x00800000>; > > iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>; > > }; > > }; > > > > bus@0 { > > /* truncation happens here */ > > #address-cells = <1>; > > #size-cells = <1>; > > ranges = <0x0 0x0 0x0 0x40000000>; > > > > mc: memory-controller@2c00000 { > > #address-cells = <2>; > > #size-cells = <2>; > > I think this is wrong. The parent should have more or equal number of > cells. I was half suspecting that. The reason why I hesitated is that I recall having the opposite discussion a while ago when we were adding bus@0 to 64-bit Tegra devices. We had at some point (probably around Tegra114 or Tegra124, 32-bit ARM chips that support LPAE) started to set #address- cells = <2> precisely because the CPU could address more than 32-bit addresses. We then did the same thing transitioning to 64-bit ARM. When we then started discussing bus@0, someone (might have been you) had argued that all these peripherals could be addressed with a single cell so there'd be no need for #address-cells = <2>, so then we went with that. Reverting back to #address-cells = <2> is now going to cause quite a bit of churn, but I guess if it's the right thing, so be it. Another possible alternative would be to move the memory-controller node from the bus@0 to the top-level. Not sure if that's any better. Thierry
On Tue, Nov 8, 2022 at 8:33 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Mon, Nov 07, 2022 at 01:30:35PM -0600, Rob Herring wrote: > > On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote: > > > From: Thierry Reding <treding@nvidia.com> > > > > > > DMA parent devices can define separate DMA busses via the "dma-ranges" > > > and "#address-cells" and "#size-cells" properties. If the DMA bus has > > > different cell counts than its parent, this can cause the translation > > > of DMA address to fails (e.g. truncation from 2 to 1 address cells). > > > > My assumption in this case was that the parent cell sizes should be > > increased to 2 cells. That tends to be what people want to do anyways > > (64-bit everywhere on 64-bit CPUs). > > > > > Avoid this by stopping to search for DMA parents when a parent without > > > a "dma-ranges" property is encountered. Also, since it is the DMA parent > > > that defines the DMA bus, use the bus' cell counts instead of its parent > > > cell counts. > > > > We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec > > even says that because I hit that case. > > > > Is this going to work for 'dma-device' with something like this?: > > > > bus@0 { > > dma-ranges = <...>; > > child-bus@... { > > dma-device@... { > > }; > > }; > > }; > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > Changes in v10: > > > - new patch to avoid address truncation when traversing a bus hierarchy > > > with mismatching #address-cells properties > > > > > > Example from Tegra194 (redacted for clarity): > > > > > > reserved-memory { > > > #address-cells = <2>; > > > #size-cells = <2>; > > > ranges; > > > > > > framebuffer@0,0 { > > > compatible = "framebuffer"; > > > reg = <0x2 0x57320000 0x0 0x00800000>; > > > iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>; > > > }; > > > }; > > > > > > bus@0 { > > > /* truncation happens here */ > > > #address-cells = <1>; > > > #size-cells = <1>; > > > ranges = <0x0 0x0 0x0 0x40000000>; > > > > > > mc: memory-controller@2c00000 { > > > #address-cells = <2>; > > > #size-cells = <2>; > > > > I think this is wrong. The parent should have more or equal number of > > cells. > > I was half suspecting that. The reason why I hesitated is that I recall > having the opposite discussion a while ago when we were adding bus@0 to > 64-bit Tegra devices. We had at some point (probably around Tegra114 or > Tegra124, 32-bit ARM chips that support LPAE) started to set #address- > cells = <2> precisely because the CPU could address more than 32-bit > addresses. We then did the same thing transitioning to 64-bit ARM. When > we then started discussing bus@0, someone (might have been you) had > argued that all these peripherals could be addressed with a single cell > so there'd be no need for #address-cells = <2>, so then we went with > that. I may have not thinking about the DMA side of things. > Reverting back to #address-cells = <2> is now going to cause quite a bit > of churn, but I guess if it's the right thing, so be it. > > Another possible alternative would be to move the memory-controller node > from the bus@0 to the top-level. Not sure if that's any better. I stumbled upon 'ibm,#dma-address-cells' and 'ibm,#dma-size-cells' while reviewing this. Those seem to be for the same purpose AFAICT. We could consider adding those (w/o 'ibm') to handle this situation. Rob
Am Dienstag, dem 08.11.2022 um 10:25 -0600 schrieb Rob Herring: > On Tue, Nov 8, 2022 at 8:33 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Mon, Nov 07, 2022 at 01:30:35PM -0600, Rob Herring wrote: > > > On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > DMA parent devices can define separate DMA busses via the "dma-ranges" > > > > and "#address-cells" and "#size-cells" properties. If the DMA bus has > > > > different cell counts than its parent, this can cause the translation > > > > of DMA address to fails (e.g. truncation from 2 to 1 address cells). > > > > > > My assumption in this case was that the parent cell sizes should be > > > increased to 2 cells. That tends to be what people want to do anyways > > > (64-bit everywhere on 64-bit CPUs). > > > > > > > Avoid this by stopping to search for DMA parents when a parent without > > > > a "dma-ranges" property is encountered. Also, since it is the DMA parent > > > > that defines the DMA bus, use the bus' cell counts instead of its parent > > > > cell counts. > > > > > > We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec > > > even says that because I hit that case. > > > > > > Is this going to work for 'dma-device' with something like this?: > > > > > > bus@0 { > > > dma-ranges = <...>; > > > child-bus@... { > > > dma-device@... { > > > }; > > > }; > > > }; > > > > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > --- > > > > Changes in v10: > > > > - new patch to avoid address truncation when traversing a bus hierarchy > > > > with mismatching #address-cells properties > > > > > > > > Example from Tegra194 (redacted for clarity): > > > > > > > > reserved-memory { > > > > #address-cells = <2>; > > > > #size-cells = <2>; > > > > ranges; > > > > > > > > framebuffer@0,0 { > > > > compatible = "framebuffer"; > > > > reg = <0x2 0x57320000 0x0 0x00800000>; > > > > iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>; > > > > }; > > > > }; > > > > > > > > bus@0 { > > > > /* truncation happens here */ > > > > #address-cells = <1>; > > > > #size-cells = <1>; > > > > ranges = <0x0 0x0 0x0 0x40000000>; > > > > > > > > mc: memory-controller@2c00000 { > > > > #address-cells = <2>; > > > > #size-cells = <2>; > > > > > > I think this is wrong. The parent should have more or equal number of > > > cells. > > > > I was half suspecting that. The reason why I hesitated is that I recall > > having the opposite discussion a while ago when we were adding bus@0 to > > 64-bit Tegra devices. We had at some point (probably around Tegra114 or > > Tegra124, 32-bit ARM chips that support LPAE) started to set #address- > > cells = <2> precisely because the CPU could address more than 32-bit > > addresses. We then did the same thing transitioning to 64-bit ARM. When > > we then started discussing bus@0, someone (might have been you) had > > argued that all these peripherals could be addressed with a single cell > > so there'd be no need for #address-cells = <2>, so then we went with > > that. > > I may have not thinking about the DMA side of things. > > > Reverting back to #address-cells = <2> is now going to cause quite a bit > > of churn, but I guess if it's the right thing, so be it. > > > > Another possible alternative would be to move the memory-controller node > > from the bus@0 to the top-level. Not sure if that's any better. > > I stumbled upon 'ibm,#dma-address-cells' and 'ibm,#dma-size-cells' > while reviewing this. Those seem to be for the same purpose AFAICT. We > could consider adding those (w/o 'ibm') to handle this situation. I would appreciate this. We have the same situation on some of the NXP i.MX8 SoCs right now: all the MMIO is addressable with 32bit, so all the busses have a single address and size cell right now, but we would need to extend the address-cells to 64bit just to properly describe the DMA addressing capabilities of the devices. Regards, Lucas
On Wed, Nov 09, 2022 at 11:07:02AM +0100, Lucas Stach wrote: > Am Dienstag, dem 08.11.2022 um 10:25 -0600 schrieb Rob Herring: > > On Tue, Nov 8, 2022 at 8:33 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Mon, Nov 07, 2022 at 01:30:35PM -0600, Rob Herring wrote: > > > > On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote: > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > > > DMA parent devices can define separate DMA busses via the "dma-ranges" > > > > > and "#address-cells" and "#size-cells" properties. If the DMA bus has > > > > > different cell counts than its parent, this can cause the translation > > > > > of DMA address to fails (e.g. truncation from 2 to 1 address cells). > > > > > > > > My assumption in this case was that the parent cell sizes should be > > > > increased to 2 cells. That tends to be what people want to do anyways > > > > (64-bit everywhere on 64-bit CPUs). > > > > > > > > > Avoid this by stopping to search for DMA parents when a parent without > > > > > a "dma-ranges" property is encountered. Also, since it is the DMA parent > > > > > that defines the DMA bus, use the bus' cell counts instead of its parent > > > > > cell counts. > > > > > > > > We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec > > > > even says that because I hit that case. > > > > > > > > Is this going to work for 'dma-device' with something like this?: > > > > > > > > bus@0 { > > > > dma-ranges = <...>; > > > > child-bus@... { > > > > dma-device@... { > > > > }; > > > > }; > > > > }; > > > > > > > > > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > > --- > > > > > Changes in v10: > > > > > - new patch to avoid address truncation when traversing a bus hierarchy > > > > > with mismatching #address-cells properties > > > > > > > > > > Example from Tegra194 (redacted for clarity): > > > > > > > > > > reserved-memory { > > > > > #address-cells = <2>; > > > > > #size-cells = <2>; > > > > > ranges; > > > > > > > > > > framebuffer@0,0 { > > > > > compatible = "framebuffer"; > > > > > reg = <0x2 0x57320000 0x0 0x00800000>; > > > > > iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>; > > > > > }; > > > > > }; > > > > > > > > > > bus@0 { > > > > > /* truncation happens here */ > > > > > #address-cells = <1>; > > > > > #size-cells = <1>; > > > > > ranges = <0x0 0x0 0x0 0x40000000>; > > > > > > > > > > mc: memory-controller@2c00000 { > > > > > #address-cells = <2>; > > > > > #size-cells = <2>; > > > > > > > > I think this is wrong. The parent should have more or equal number of > > > > cells. > > > > > > I was half suspecting that. The reason why I hesitated is that I recall > > > having the opposite discussion a while ago when we were adding bus@0 to > > > 64-bit Tegra devices. We had at some point (probably around Tegra114 or > > > Tegra124, 32-bit ARM chips that support LPAE) started to set #address- > > > cells = <2> precisely because the CPU could address more than 32-bit > > > addresses. We then did the same thing transitioning to 64-bit ARM. When > > > we then started discussing bus@0, someone (might have been you) had > > > argued that all these peripherals could be addressed with a single cell > > > so there'd be no need for #address-cells = <2>, so then we went with > > > that. > > > > I may have not thinking about the DMA side of things. > > > > > Reverting back to #address-cells = <2> is now going to cause quite a bit > > > of churn, but I guess if it's the right thing, so be it. > > > > > > Another possible alternative would be to move the memory-controller node > > > from the bus@0 to the top-level. Not sure if that's any better. > > > > I stumbled upon 'ibm,#dma-address-cells' and 'ibm,#dma-size-cells' > > while reviewing this. Those seem to be for the same purpose AFAICT. We > > could consider adding those (w/o 'ibm') to handle this situation. > > I would appreciate this. We have the same situation on some of the NXP > i.MX8 SoCs right now: all the MMIO is addressable with 32bit, so all > the busses have a single address and size cell right now, but we would > need to extend the address-cells to 64bit just to properly describe the > DMA addressing capabilities of the devices. Alright, I'll see if I can come up with some code to deal with this. Thierry
diff --git a/drivers/of/address.c b/drivers/of/address.c index 14f137a21b0c..e2f45bdbc41a 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -475,6 +475,7 @@ static u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr, const char *rprop, struct device_node **host) { + bool dma = rprop && !strcmp(rprop, "dma-ranges"); struct device_node *parent = NULL; struct of_bus *bus, *pbus; __be32 addr[OF_MAX_ADDR_CELLS]; @@ -494,7 +495,12 @@ static u64 __of_translate_address(struct device_node *dev, bus = of_match_bus(parent); /* Count address cells & copy address locally */ - bus->count_cells(dev, &na, &ns); + if (dma) { + na = of_bus_n_addr_cells(parent); + ns = of_bus_n_size_cells(parent); + } else { + bus->count_cells(dev, &na, &ns); + } if (!OF_CHECK_COUNTS(na, ns)) { pr_debug("Bad cell count for %pOF\n", dev); goto bail; @@ -515,7 +521,7 @@ static u64 __of_translate_address(struct device_node *dev, parent = get_parent(dev); /* If root, we have finished */ - if (parent == NULL) { + if (parent == NULL || (dma && !of_get_property(parent, "dma-ranges", NULL))) { pr_debug("reached root node\n"); result = of_read_number(addr, na); break; @@ -536,7 +542,12 @@ static u64 __of_translate_address(struct device_node *dev, /* Get new parent bus and counts */ pbus = of_match_bus(parent); - pbus->count_cells(dev, &pna, &pns); + if (dma) { + pna = of_bus_n_addr_cells(parent); + pns = of_bus_n_size_cells(parent); + } else { + pbus->count_cells(dev, &pna, &pns); + } if (!OF_CHECK_COUNTS(pna, pns)) { pr_err("Bad cell count for %pOF\n", dev); break;