Re: [PATCH] arm64: dts: Add initial device tree support for Tegra132

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

On Fri, Jan 23, 2015 at 11:44:24AM +0000, Mark Rutland wrote:
> On Fri, Jan 23, 2015 at 11:31:24AM +0000, Paul Walmsley wrote:
> > On Wed, 21 Jan 2015, Mark Rutland wrote:
> > > On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
[...]
> > > > +
> > > > +               clocks = <&tegra_car TEGRA124_CLK_PCIE>,
> > > > +                        <&tegra_car TEGRA124_CLK_AFI>,
> > > > +                        <&tegra_car TEGRA124_CLK_PLL_E>,
> > > > +                        <&tegra_car TEGRA124_CLK_CML0>;
> > > > +               clock-names = "pex", "afi", "pll_e", "cml";
> > > > +               resets = <&tegra_car 70>,
> > > > +                        <&tegra_car 72>,
> > > > +                        <&tegra_car 74>;
> > > > +               reset-names = "pex", "afi", "pcie_x";
> > > > +               status = "disabled";
> > > > +
> > > > +               phys = <&xusb_padctl TEGRA_XUSB_PADCTL_PCIE>;
> > > > +               phy-names = "pcie";
> > > > +
> > > > +               pci@1,0 {
> > > > +                       device_type = "pci";
> > > > +                       assigned-addresses = <0x82000800 0 0x01000000 0 0x1000>;
> > > > +                       reg = <0x000800 0 0 0 0>;
> > > > +                       status = "disabled";
> > > > +
> > > > +                       #address-cells = <3>;
> > > > +                       #size-cells = <2>;
> > > > +                       ranges;
> > >
> > > I'm not sure why these three properties are here. Surely this is a
> > > separate address space (so isn't ranges invalid?), and do we define any
> > > sub-nodes anywhere?
> > 
> > Hmm not sure.  This was originally copied from
> > arch/arm/boot/dts/tegra124.dtsi.  Will plan to drop them for now, and then
> > if the properties (or ones like them) turn out to be needed in the future,
> > someone else can deal with it :-)
> 
> That sounds sane to me.

This follows the bindings defined almost two years ago. There was a lot
of discussion back then and this is what we agreed upon. #address-cells
and #size-cells are needed as per the PCI device tree bindings and the
ranges property needed because the PCIe root ports translate addresses
between the host bridge and the PCI endpoint devices.

> > > > +       host1x@0,50000000 {
> > > > +               compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";
> > >
> > > Regarding simple-bus, are the sub-nodes usable if this didn't probe as
> > > "nvidia,tegra124-host1x" or "nvidia,tegra132-host1x"?
> > > Do the subnodes require anything from this node?
> > >
> > > It looks like we expect/require the host1x node to be probed and
> > > initialised before we probe the children. Which would imply the
> > > simple-bus annotation is wrong.
> > 
> > Haven't tried booting with just simple-bus here.  I believe these devices
> > are accessible without the involvement of host1x.  In other words, host1x
> > is not a bus; I believe it might be most accurately described as a type of
> > DMA controller.  So in theory it should be possible for the CPU complex to
> > read and write to these devices directly without the involvement of
> > host1x, although I believe NVIDIA discourages this.
> > 
> > Under the theory that DT data should be hardware-specific, not
> > software-specific, in an ideal world I think we would list these devices
> > outside the embrace of the host1x node.  However the existing Tegra124 DT
> > file listed them together, and I am unsure whether that is required for
> > the host1x code to function correctly.
> >
> > Arto may be able to comment further.
> 
> Hmm, It would be good to hear from someone familar with that then. I'll
> wait for Arto.

We actually rely on the parent-child relationship in drivers, so we
can't just go and reorganize things at will. This is documented in the
existing binding for host1x, which again, was agreed upon a long time
ago.

As for the simple-bus compatible, I think that was used way back to make
sure of_platform_populate() gets called. I suppose we could drop it and
call of_platform_populate() from the driver if its not there. The reason
why we never considered cases where it would probe as simple-bus is that
it was deemed unreasonable for a driver to bind to simple-bus.

> > > > +       cpus {
> > > > +               #address-cells = <2>;
> > > > +               #size-cells = <0>;
> > > > +
> > > > +               cpu@0 {
> > > > +                       device_type = "cpu";
> > > > +                       compatible = "nvidia,denver", "arm,armv8";
> > > > +                       reg = <0x0 0x0>;
> > > > +                       enable-method = "spin-table";
> > > > +                       cpu-release-addr = <0x0 0x80000008>;
> > > > +               };
> > > > +
> > > > +               cpu@1 {
> > > > +                       device_type = "cpu";
> > > > +                       compatible = "nvidia,denver", "arm,armv8";
> > > > +                       reg = <0x0 0x1>;
> > > > +                       enable-method = "spin-table";
> > > > +                       cpu-release-addr = <0x0 0x80000008>;
> > > > +               };
> > > > +       };
> > >
> > > It would be nice if this were near the top, as in other dts files.
> > 
> > OK will move.

Actually this follows the rules that we've been following with every DTS
so far. Nodes with a unit address go first, sorted by unit address. They
are followed by nodes without a unit address, sorted alphabetically.

I'd prefer keeping it this way for consistency across Tegra DTS files.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]