Re: [PATCH v10 2/5] of: Stop DMA translation at last DMA parent

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4710 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]