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: 3106 bytes --]

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

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