On Tue, Nov 8, 2022 at 8:33 AM Thierry Reding <thierry.reding@xxxxxxxxx> 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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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
