Re: [PATCH] arm64: dts: juno: Fix DMA address translations by adding SOC bus node

[Sudeep Holla <sudeep.holla@arm.com>]

On Thu, Nov 28, 2019 at 04:15:43PM +0000, Robin Murphy wrote:
> On 28/11/2019 2:15 pm, Sudeep Holla wrote:
> > On Thu, Nov 28, 2019 at 11:50:54AM +0000, Robin Murphy wrote:
> > > Hi Sudeep,
> > > 
> > > On 2019-11-26 4:53 pm, Sudeep Holla wrote:
> > > > Commit 951d48855d86 ("of: Make of_dma_get_range() work on bus nodes")
> > > > reworked the logic such that of_dma_get_range() works correctly
> > > > starting from a bus node containing "dma-ranges".
> > > > 
> > > > Since on Juno we don't have a SoC level bus node and "dma-ranges" is
> > > > present only in the root node, we get the following error:
> > > > 
> > > > OF: translation of DMA address(0) to CPU address failed node(/sram@2e000000)
> > > > OF: translation of DMA address(0) to CPU address failed node(/uart@7ff80000)
> > > > ...
> > > > OF: translation of DMA address(0) to CPU address failed node(/mhu@2b1f0000)
> > > > OF: translation of DMA address(0) to CPU address failed node(/iommu@2b600000)
> > > > OF: translation of DMA address(0) to CPU address failed node(/iommu@2b600000)
> > > > OF: translation of DMA address(0) to CPU address failed node(/iommu@2b600000)
> > > > 
> > > > Let's fix it by adding a SoC bus node and moving all the devices along
> > > > with the "dma-ranges" inside it.
> > > > 
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > ---
> > > >    arch/arm64/boot/dts/arm/juno-base.dtsi        | 162 +++++++++---------
> > > >    arch/arm64/boot/dts/arm/juno-clocks.dtsi      |   2 +
> > > >    arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi     |   2 +
> > > >    arch/arm64/boot/dts/arm/juno-motherboard.dtsi |   2 +
> > > >    4 files changed, 88 insertions(+), 80 deletions(-)
> > > > 
> > > > Hi Rob, Robin,
> > > > 
> > > > Let me know if this is correct fix for the issue I am seeing with linux-next
> > > > on Juno. This patch is generated with -b for ease of review. With lots of
> > > > indentation, actual delta is:
> > > > 
> > > > 4 files changed, 1274 insertions(+), 1266 deletions(-)
> > > 
> > > Other than a few nits - the GIC should probably be under the soc node as
> > > it's an MMIO device, while the clocks shouldn't; the dtsi's could probably
> > > avoid churn with a "&soc {...}" phandle - I think this is a reasonable thing
> > > to do, as it's generally the preferred structure.
> > > 
> > 
> > I agree and am still in confusion as what to put inside soc or not.
> 
> FWIW my understanding is that the "soc" node is used to provide a 'bus' to
> represent on-chip MMIO - in principle we could nerd out and describe the
> ACE-lite/AXI/APB slave interconnects in full, but there's really no benefit
> to going into that much detail - so everything with a "reg" representing a
> physical address goes inside it, while CPUs, clocks, firmware, regulators
> etc. sit in the root node 'outside the PA space', regardless of whether
> they're physically on-chip or not.
>

I saw few other DTs and they all keep interrupt-controller and timer
outside this "soc" node. Timer is sysreg based, so makes sense. GIC has
both sysreg and MMIO components so I was not sure if it's done like that
due to some dependency on how interrupt-parent property is used.
e.g. arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi

I expect even the motherboard IO shouldn't go inside it but again to avoid
churn I kept it inside.

I agree on firmware, clocks, regulators,..etc. I wanted to keep the churn
minimum and it was not completely aligned to the principles you mentioned
above. Anyways it's better to fix it in right manner for next cycle.

--
Regards,
Sudeep