[PATCH 0/3] ARM: dts: sun8i: R40: DT fixes and updates

[PATCH 0/3] ARM: dts: sun8i: R40: DT fixes and updates

[Andre Przywara]

Hi,

some changes I made to the R40 .dtsi when playing around with a
Bananapi M2 Berry board:
- The GICC region is wrongly reported as 4K only, preventing KVM
  from using its VGIC emulation.
- The PMU node is missing, similar story as with the other SoCs: this
  time the SPI numbers are not even mentioned in the manual.
- I hooked up a SPI flash to the PortC SPI0 header pins (from which
  the board can even boot from). So patch 3 adds the SPI nodes and its
  pinmux setup, in case people need this for their peripherals.

Please have a look and apply!

Cheers,
Andre.

Andre Przywara (3):
  ARM: dts: sun8i: R40: Upgrade GICC reg size to 8K
  ARM: dts: sun8i: R40: Add PMU node
  ARM: dts: sun8i: R40: Add SPI controllers nodes and pinmuxes

 arch/arm/boot/dts/sun8i-r40.dtsi | 108 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 103 insertions(+), 5 deletions(-)

-- 
2.14.5

[PATCH 1/3] ARM: dts: sun8i: R40: Upgrade GICC reg size to 8K

[Andre Przywara]

The GIC used in the R40 SoC is an ARM GIC-400 with virtualization support,
so let's advertise the full 8K region of the GICC MMIO frame to enable
KVM's usage of the GIC (as we do already for all other SoCs).

Tested by running KVM on a Bananapi M2 Berry.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/boot/dts/sun8i-r40.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi
index 82ea0b5b0710..768dffb37117 100644
--- a/arch/arm/boot/dts/sun8i-r40.dtsi
+++ b/arch/arm/boot/dts/sun8i-r40.dtsi
@@ -831,7 +831,7 @@
 		gic: interrupt-controller@1c81000 {
 			compatible = "arm,gic-400";
 			reg = <0x01c81000 0x1000>,
-			      <0x01c82000 0x1000>,
+			      <0x01c82000 0x2000>,
 			      <0x01c84000 0x2000>,
 			      <0x01c86000 0x2000>;
 			interrupt-controller;
-- 
2.14.5

Re: [PATCH 1/3] ARM: dts: sun8i: R40: Upgrade GICC reg size to 8K

[Maxime Ripard]

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

On Thu, Jan 02, 2020 at 01:26:55AM +0000, Andre Przywara wrote:
> The GIC used in the R40 SoC is an ARM GIC-400 with virtualization support,
> so let's advertise the full 8K region of the GICC MMIO frame to enable
> KVM's usage of the GIC (as we do already for all other SoCs).
>
> Tested by running KVM on a Bananapi M2 Berry.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Applied, thanks!
Maxime

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

[PATCH 2/3] ARM: dts: sun8i: R40: Add PMU node

[Andre Przywara]

The ARM Cortex-A7 cores used in the Allwinner R40 SoC have their usual
Performance Monitoring Unit (PMU), which allows perf to use hardware
events.
The SoC integrator just needs to connect each per-core interrupt line
to the GIC. The R40 manual does not really mention those IRQ lines, but
experimentation in U-Boot shows that interrupts 152-155 are connected to
the four cores (similar to the A20).

Tested on a Bananapi M2 Berry, with perf and taskset to confirm the
association between cores and interrupts.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/boot/dts/sun8i-r40.dtsi | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi
index 768dffb37117..8dcbc4465fbb 100644
--- a/arch/arm/boot/dts/sun8i-r40.dtsi
+++ b/arch/arm/boot/dts/sun8i-r40.dtsi
@@ -78,25 +78,25 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		cpu@0 {
+		cpu0: cpu@0 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <0>;
 		};
 
-		cpu@1 {
+		cpu1: cpu@1 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <1>;
 		};
 
-		cpu@2 {
+		cpu2: cpu@2 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <2>;
 		};
 
-		cpu@3 {
+		cpu3: cpu@3 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <3>;
@@ -884,6 +884,15 @@
 		};
 	};
 
+	pmu {
+		compatible = "arm,cortex-a7-pmu";
+		interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>;
+	};
+
 	timer {
 		compatible = "arm,armv7-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
-- 
2.14.5

Re: [PATCH 2/3] ARM: dts: sun8i: R40: Add PMU node

[Maxime Ripard]

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

On Thu, Jan 02, 2020 at 01:26:56AM +0000, Andre Przywara wrote:
> The ARM Cortex-A7 cores used in the Allwinner R40 SoC have their usual
> Performance Monitoring Unit (PMU), which allows perf to use hardware
> events.
> The SoC integrator just needs to connect each per-core interrupt line
> to the GIC. The R40 manual does not really mention those IRQ lines, but
> experimentation in U-Boot shows that interrupts 152-155 are connected to
> the four cores (similar to the A20).
>
> Tested on a Bananapi M2 Berry, with perf and taskset to confirm the
> association between cores and interrupts.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Applied, thanks!
Maxime

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

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

[Andre Przywara]

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";
+			};
+
+			spi1_pa_pins: spi1-pa-pins {
+				pins = "PA0", "PA1", "PA2", "PA3", "PA4";
+				function = "spi1";
+			};
+
+			spi1_pi_pins: spi1-pi-pins {
+				pins = "PI15", "PI16", "PI17", "PI18", "PI19";
+				function = "spi1";
+			};
+
+			spi2_pb_pins: spi2-pb-pins {
+				pins = "PB13", "PB14", "PB15", "PB16", "PB17";
+				function = "spi2";
+			};
+
+			spi2_pc_pins: spi2-pc-pins {
+				pins = "PC19", "PC20", "PC21", "PC22";
+				function = "spi2";
+			};
+
+			spi3_pins: spi3-pins {
+				pins = "PA5", "PA6", "PA7", "PA8", "PA9";
+				function = "spi3";
+			};
+
 			uart0_pb_pins: uart0-pb-pins {
 				pins = "PB22", "PB23";
 				function = "uart0";
@@ -594,6 +629,60 @@
 			#size-cells = <0>;
 		};
 
+		spi0: spi@1c05000 {
+			compatible = "allwinner,sun8i-r40-spi",
+				     "allwinner,sun8i-h3-spi";
+			reg = <0x01c05000 0x1000>;
+			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>;
+			clock-names = "ahb", "mod";
+			resets = <&ccu RST_BUS_SPI0>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		spi1: spi@1c06000 {
+			compatible = "allwinner,sun8i-r40-spi",
+				     "allwinner,sun8i-h3-spi";
+			reg = <0x01c06000 0x1000>;
+			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_SPI1>;
+			clock-names = "ahb", "mod";
+			resets = <&ccu RST_BUS_SPI1>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		spi2: spi@1c07000 {
+			compatible = "allwinner,sun8i-r40-spi",
+				     "allwinner,sun8i-h3-spi";
+			reg = <0x01c07000 0x1000>;
+			interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_SPI2>, <&ccu CLK_SPI2>;
+			clock-names = "ahb", "mod";
+			resets = <&ccu RST_BUS_SPI2>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		spi3: spi@1c0f000 {
+			compatible = "allwinner,sun8i-r40-spi",
+				     "allwinner,sun8i-h3-spi";
+			reg = <0x01c0f000 0x1000>;
+			interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_SPI3>, <&ccu CLK_SPI3>;
+			clock-names = "ahb", "mod";
+			pinctrl-0 = <&spi3_pins>;
+			pinctrl-names = "default";
+			resets = <&ccu RST_BUS_SPI3>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
 		ahci: sata@1c18000 {
 			compatible = "allwinner,sun8i-r40-ahci";
 			reg = <0x01c18000 0x1000>;
-- 
2.14.5

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

[Maxime Ripard]

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

Hi,

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, 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).

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.

And please add /omit-if-no-ref/ to those groups.

Thanks!
Maxime

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

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

[Andre Przywara]

On Thu, 2 Jan 2020 10:57:11 +0100
Maxime Ripard <mripard@kernel.org> wrote:

Hi Maxime,

thanks for having a look!

> 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.

> 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 ;-)
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.

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.
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?
And should there be a dtc command line option to ignore those tags, or even to apply this tag (virtually) to every node?

Cheers,
Andre.

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

[Chen-Yu Tsai]

On Thu, Jan 2, 2020 at 6:42 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Thu, 2 Jan 2020 10:57:11 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi Maxime,
>
> thanks for having a look!
>
> > 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.
>
> > 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 ;-)
> 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.
>
> 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.

I believe it would be nice to have them for all pin descriptions, but
having them
for the peripherals that only have one muxing option probably wouldn't have any
effect, as they would be set and referenced by default in the dtsi.

> 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?

In upstream dtc, I sent a patch to make it ignore the flag if you
compile the dts
with overlay support, i.e. with the -@ flag.

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

That would probably end up trimming peripherals out, even enabled ones?

ChenYu

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

[Maxime Ripard]

[-- 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 --]

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

[André Przywara]

On 04/01/2020 10:04, Maxime Ripard wrote:

Hi,

> 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.

Agreed, I just remembered that I checked this a while ago and found that
we always control the SS_LEVEL bit, but never anything else. But indeed
irrelevant for this.

> 
>>> 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.

That's what I figured ;-)

>> 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.

OK, will post it.

>> 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,

Really? Where is that? In Linux, or in U-Boot, possibly with the caches
off? I am just curious. AFAIK there are some inefficient algorithms in
libfdt (which trade performance for a smaller memory footprint), but I
thought those would be called only very rarely.

> so if we can get rid of the unused pin
> groups, that's great :)

I see it is preferable over creating a separate copy of the .dts file.

>> 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.

Yeah, Chen-Yu mentioned that already. Thanks for the info, I missed that.

>> 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 :)

Yeah, that was a New-Year's brain-fart of mine ;-)

While I was changing the patch I figured that it gets quite lengthy.
Also looking at the a20.dtsi, I see that *all* pin groups have this tag
now. Wouldn't it be easier to introduce *one* tag that applies that to
all children of a node?

Another thing I was wondering about: Would we gain something by not
compiling nodes which have status = "disabled"? This is mentioned as a
generic property in the DT spec, although it says there that the exact
meaning is device dependent. But it sound still worthwhile, especially
since we would avoid more pin groups to be compiled in.

Cheers,
Andre.

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

[Maxime Ripard]

On Sun, Jan 05, 2020 at 04:40:58PM +0000, André Przywara wrote:
> On 04/01/2020 10:04, Maxime Ripard wrote:
> >> 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,
>
> Really? Where is that? In Linux, or in U-Boot, possibly with the caches
> off? I am just curious. AFAIK there are some inefficient algorithms in
> libfdt (which trade performance for a smaller memory footprint), but I
> thought those would be called only very rarely.

The last time I measured it was in U-Boot (and for the FIT image, not
an actual DT), but the parsing time for a FIT image with a kernel and
DTB was around 100ms.

(and adding the PSCI and simplefb nodes was in the same order of
magnitude).

Boot time was very sensitive, and I had to remove both.

> >> 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 :)
>
> Yeah, that was a New-Year's brain-fart of mine ;-)
>
> While I was changing the patch I figured that it gets quite lengthy.
> Also looking at the a20.dtsi, I see that *all* pin groups have this tag
> now. Wouldn't it be easier to introduce *one* tag that applies that to
> all children of a node?

I don't really know, I'm not sure the proliferation of tags would be a
good thing either, and I'm not the dtc maintainer, so I don't really
have a say in this :)

> Another thing I was wondering about: Would we gain something by not
> compiling nodes which have status = "disabled"? This is mentioned as a
> generic property in the DT spec, although it says there that the exact
> meaning is device dependent. But it sound still worthwhile, especially
> since we would avoid more pin groups to be compiled in.

I guess that would be good too, but the semantics are a bit different
so we'd need a different tag.

Maxime