Re: [PATCH 3/3] ARM: dts: sun8i: R40: Add SPI controllers nodes and pinmuxes

[Maxime Ripard <mripard@kernel.org>]

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

On Thu, Jan 02, 2020 at 10:41:58AM +0000, Andre Przywara wrote:
> On Thu, 2 Jan 2020 10:57:11 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> > On Thu, Jan 02, 2020 at 01:26:57AM +0000, Andre Przywara wrote:
> > > The Allwinner R40 SoC contains four SPI controllers, using the newer
> > > sun6i design (but at the legacy addresses).
> > > The controller seems to be fully compatible to the A64 one, so no driver
> > > changes are necessary.
> > > The first three controller can be used on two sets of pins, but SPI3 is
> > > only routed to one set on Port A.
> > >
> > > Tested by connecting a SPI flash to a Bananapi M2 Berry on the SPI0
> > > PortC header pins.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  arch/arm/boot/dts/sun8i-r40.dtsi | 89 ++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 89 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi
> > > index 8dcbc4465fbb..af437391dcf4 100644
> > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi
> > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi
> > > @@ -418,6 +418,41 @@
> > >  				bias-pull-up;
> > >  			};
> > >
> > > +			spi0_pc_pins: spi0-pc-pins {
> > > +				pins = "PC0", "PC1", "PC2", "PC23";
> > > +				function = "spi0";
> > > +			};
> > > +
> > > +			spi0_pi_pins: spi0-pi-pins {
> > > +				pins = "PI10", "PI11", "PI12", "PI13", "PI14";
> > > +				function = "spi0";
> > > +			};
> >
> > This split doesn't really work though :/
> >
> > The PC pins group has MOSI, MISO, CLK and CS0, while the PI pins group
> > has CS0, CLK, MOSI, MISO and CS1.
> >
> > Meaning that if a board uses a GPIO CS pin, we can't really express
> > that
>
> Does that actually work? I dimly remember checking our sunxi driver
> a while ago and I wasn't sure that would be functional there.

It's something generic that should be handled by the core iirc. We
might be missing the few things to enable it though, but that's
irrelevant to the DT itself.

> > and any board using the PI pins for its SPI bus will try to
> > claim CS0 and CS1, no matter how many devices are connected on the bus
> > (and if there's one, there might be something else connected to PI14).
>
> True.
>
> > And you can't have a board using CS1 with the PC signals either.
> >
> > You should split away the CS pins into separate groups, like we're
> > doing with the A20 for example.
>
> Ah, yeah, makes sense, thanks for the pointer.
>
> > And please add /omit-if-no-ref/ to those groups.
>
> I was a bit reluctant to do this:
>
> First there does not seem to be any good documentation about it,
> neither in the official DT spec nor in dtc, even though I think I
> understand what it does ;-)

If a node doesn't have a phandle pointing to it, it will be ignored at
compilation time.

> Second it seems to break in U-Boot atm. Looks like applying your dtc
> patch fixes that, though. Do you know if U-Boot allows
> cherry-picking dtc patches? If yes, I could post your patch.

I know I did it for libfdt at some point, so I guess dtc would work
too, but I'm not really sure. Rockchip is also using it iirc, so
there's interest in supporting it in U-Boot anyway.

> But more importantly: what are the guidelines for using this tag? I
> understand the desire to provide every possible pin description on
> one hand, but wanting to avoid having *all of them* in *each* .dtb
> on the other.

Pin groups will take a lot of space in the dtb, and the DT parsing
will take some measurable time, so if we can get rid of the unused pin
groups, that's great :)

> But how does this play along with overlays? Shouldn't at least those
> interface pins that are exposed on headers stay in each .dtb? Can we
> actually make this decision in the SoC .dtsi file?

If the DT is compiled with overlays, that property is ignored iirc and
the node will be output in the compiled DT, because while if you don't
have overlays support you can tell if something points to that node,
you can't with overlays since those references might be stored
elsewhere.

> And should there be a dtc command line option to ignore those tags,
> or even to apply this tag (virtually) to every node?

Most of the nodes are (reference) leaves in a DT though. Pretty much
all the device nodes have no references pointing to them, just like
most of the buses, the CPU nodes, etc. And I'm pretty sure you want to
keep them :)

Maxime

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