[PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes

John Watts posted 4 patches 2 years, 6 months ago
[PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by John Watts 2 years, 6 months ago
The Allwinner D1, T113 provide two CAN controllers that are variants
of the R40 controller.

I have tested support for these controllers on two boards:

- A Lichee Panel RV 86 Panel running a D1 chip
- A Mango Pi MQ Dual running a T113-s3 chip

Both of these fully support both CAN controllers.

Signed-off-by: John Watts <contact@jookia.org>
---
 .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
index 1bb1e5cae602..4086c0cc0f9d 100644
--- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
@@ -131,6 +131,18 @@ uart3_pb_pins: uart3-pb-pins {
 				pins = "PB6", "PB7";
 				function = "uart3";
 			};
+
+			/omit-if-no-ref/
+			can0_pins: can0-pins {
+				pins = "PB2", "PB3";
+				function = "can0";
+			};
+
+			/omit-if-no-ref/
+			can1_pins: can1-pins {
+				pins = "PB4", "PB5";
+				function = "can1";
+			};
 		};
 
 		ccu: clock-controller@2001000 {
@@ -879,5 +891,23 @@ rtc: rtc@7090000 {
 			clock-names = "bus", "hosc", "ahb";
 			#clock-cells = <1>;
 		};
+
+		can0: can@2504000 {
+			compatible = "allwinner,sun20i-d1-can";
+			reg = <0x02504000 0x400>;
+			interrupts = <SOC_PERIPHERAL_IRQ(21) IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CAN0>;
+			resets = <&ccu RST_BUS_CAN0>;
+			status = "disabled";
+		};
+
+		can1: can@2504400 {
+			compatible = "allwinner,sun20i-d1-can";
+			reg = <0x02504400 0x400>;
+			interrupts = <SOC_PERIPHERAL_IRQ(22) IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CAN1>;
+			resets = <&ccu RST_BUS_CAN1>;
+			status = "disabled";
+		};
 	};
 };
-- 
2.41.0
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by Maksim Kiselev 2 years, 6 months ago
Hi John, Jernej

On Sat, Jul 22, 2023 at 08:15:51AM +1000, John Watts wrote:
> ...
> @@ -131,6 +131,18 @@ uart3_pb_pins: uart3-pb-pins {
> 				pins = "PB6", "PB7";
> 				function = "uart3";
> 			};
> +
> +			/omit-if-no-ref/
> +			can0_pins: can0-pins {
> +				pins = "PB2", "PB3";
> +				function = "can0";
> +			};
> +
> +			/omit-if-no-ref/
> +			can1_pins: can1-pins {
> +				pins = "PB4", "PB5";
> +				function = "can1";
> +			};
> ...

Should we also keep a pinctrl nodes itself in alphabetical order?
I mean placing a CAN nodes before `clk_pg11_pin` node?
Looks like the other nodes sorted in this way...

Cheers,
Maksim
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by John Watts 2 years, 6 months ago
On Sat, Aug 05, 2023 at 07:40:52PM +0300, Maksim Kiselev wrote:
> Hi John, Jernej
> Should we also keep a pinctrl nodes itself in alphabetical order?
> I mean placing a CAN nodes before `clk_pg11_pin` node?
> Looks like the other nodes sorted in this way...

Good catch. Now that you mention it, the device tree nodes are sorted
by memory order too! These should be after i2c3.

It looks like I might need to do a patch to re-order those too.

> Cheers,
> Maksim

John.
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by Jernej Škrabec 2 years, 6 months ago
Dne sobota, 05. avgust 2023 ob 18:51:53 CEST je John Watts napisal(a):
> On Sat, Aug 05, 2023 at 07:40:52PM +0300, Maksim Kiselev wrote:
> > Hi John, Jernej
> > Should we also keep a pinctrl nodes itself in alphabetical order?
> > I mean placing a CAN nodes before `clk_pg11_pin` node?
> > Looks like the other nodes sorted in this way...
> 
> Good catch. Now that you mention it, the device tree nodes are sorted
> by memory order too! These should be after i2c3.
> 
> It looks like I might need to do a patch to re-order those too.

It would be better if DT patches are dropped from netdev tree and then post 
new versions.

Best regards,
Jernej

> 
> > Cheers,
> > Maksim
> 
> John.
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by John Watts 2 years, 6 months ago
On Sat, Aug 05, 2023 at 07:49:51PM +0200, Jernej Škrabec wrote:
> Dne sobota, 05. avgust 2023 ob 18:51:53 CEST je John Watts napisal(a):
> > On Sat, Aug 05, 2023 at 07:40:52PM +0300, Maksim Kiselev wrote:
> > > Hi John, Jernej
> > > Should we also keep a pinctrl nodes itself in alphabetical order?
> > > I mean placing a CAN nodes before `clk_pg11_pin` node?
> > > Looks like the other nodes sorted in this way...
> > 
> > Good catch. Now that you mention it, the device tree nodes are sorted
> > by memory order too! These should be after i2c3.
> > 
> > It looks like I might need to do a patch to re-order those too.
> 
> It would be better if DT patches are dropped from netdev tree and then post 
> new versions.
> 
> Best regards,
> Jernej

Agreed. Is there a way to request that? Or will the maintainer just read this?

John.
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by Jernej Škrabec 2 years, 6 months ago
Dne nedelja, 06. avgust 2023 ob 08:33:45 CEST je John Watts napisal(a):
> On Sat, Aug 05, 2023 at 07:49:51PM +0200, Jernej Škrabec wrote:
> > Dne sobota, 05. avgust 2023 ob 18:51:53 CEST je John Watts napisal(a):
> > > On Sat, Aug 05, 2023 at 07:40:52PM +0300, Maksim Kiselev wrote:
> > > > Hi John, Jernej
> > > > Should we also keep a pinctrl nodes itself in alphabetical order?
> > > > I mean placing a CAN nodes before `clk_pg11_pin` node?
> > > > Looks like the other nodes sorted in this way...
> > > 
> > > Good catch. Now that you mention it, the device tree nodes are sorted
> > > by memory order too! These should be after i2c3.
> > > 
> > > It looks like I might need to do a patch to re-order those too.
> > 
> > It would be better if DT patches are dropped from netdev tree and then
> > post
> > new versions.
> > 
> > Best regards,
> > Jernej
> 
> Agreed. Is there a way to request that? Or will the maintainer just read
> this?

Hopefully it will.

Best regards,
Jernej
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by Marc Kleine-Budde 2 years, 6 months ago
On 06.08.2023 13:42:28, Jernej Škrabec wrote:
> Dne nedelja, 06. avgust 2023 ob 08:33:45 CEST je John Watts napisal(a):
> > On Sat, Aug 05, 2023 at 07:49:51PM +0200, Jernej Škrabec wrote:
> > > Dne sobota, 05. avgust 2023 ob 18:51:53 CEST je John Watts napisal(a):
> > > > On Sat, Aug 05, 2023 at 07:40:52PM +0300, Maksim Kiselev wrote:
> > > > > Hi John, Jernej
> > > > > Should we also keep a pinctrl nodes itself in alphabetical order?
> > > > > I mean placing a CAN nodes before `clk_pg11_pin` node?
> > > > > Looks like the other nodes sorted in this way...
> > > > 
> > > > Good catch. Now that you mention it, the device tree nodes are sorted
> > > > by memory order too! These should be after i2c3.
> > > > 
> > > > It looks like I might need to do a patch to re-order those too.
> > > 
> > > It would be better if DT patches are dropped from netdev tree and then
> > > post
> > > new versions.
> > > 
> > > Best regards,
> > > Jernej
> > 
> > Agreed. Is there a way to request that? Or will the maintainer just read
> > this?
> 
> Hopefully it will.

I'm just catching up on last week's post (I had a long off-line
weekend).

I'll revert the DT changes and send a PR to net-next.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by Marc Kleine-Budde 2 years, 6 months ago
On 07.08.2023 09:16:41, Marc Kleine-Budde wrote:
> On 06.08.2023 13:42:28, Jernej Škrabec wrote:
> > Dne nedelja, 06. avgust 2023 ob 08:33:45 CEST je John Watts napisal(a):
> > > On Sat, Aug 05, 2023 at 07:49:51PM +0200, Jernej Škrabec wrote:
> > > > Dne sobota, 05. avgust 2023 ob 18:51:53 CEST je John Watts napisal(a):
> > > > > On Sat, Aug 05, 2023 at 07:40:52PM +0300, Maksim Kiselev wrote:
> > > > > > Hi John, Jernej
> > > > > > Should we also keep a pinctrl nodes itself in alphabetical order?
> > > > > > I mean placing a CAN nodes before `clk_pg11_pin` node?
> > > > > > Looks like the other nodes sorted in this way...
> > > > > 
> > > > > Good catch. Now that you mention it, the device tree nodes are sorted
> > > > > by memory order too! These should be after i2c3.
> > > > > 
> > > > > It looks like I might need to do a patch to re-order those too.
> > > > 
> > > > It would be better if DT patches are dropped from netdev tree and then
> > > > post
> > > > new versions.
> > > > 
> > > > Best regards,
> > > > Jernej
> > > 
> > > Agreed. Is there a way to request that? Or will the maintainer just read
> > > this?
> > 
> > Hopefully it will.
> 
> I'm just catching up on last week's post (I had a long off-line
> weekend).
> 
> I'll revert the DT changes and send a PR to net-next.

Here's the revert:

| https://lore.kernel.org/all/20230807-riscv-allwinner-d1-revert-can-controller-nodes-v1-1-eb3f70b435d9@pengutronix.de/

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by John Watts 2 years, 6 months ago
On Sat, Jul 22, 2023 at 08:15:51AM +1000, John Watts wrote:
> ...
> +			/omit-if-no-ref/
> +			can0_pins: can0-pins {
> +				pins = "PB2", "PB3";
> +				function = "can0";
> +			};
> ...
> +		can0: can@2504000 {
> +			compatible = "allwinner,sun20i-d1-can";
> +			reg = <0x02504000 0x400>;
> +			interrupts = <SOC_PERIPHERAL_IRQ(21) IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ccu CLK_BUS_CAN0>;
> +			resets = <&ccu RST_BUS_CAN0>;
> +			status = "disabled";
> +		};

Just a quick late night question to people with more knowledge than me:

These chips only have one pinctrl configuration for can0 and can1. Should the
can nodes have this pre-set instead of the board dts doing this?

I see this happening in sun4i-a10.dtsi for instance, but it also seems like it
could become a problem when it comes to re-using the dtsi for newer chip variants.

John.
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by Jernej Škrabec 2 years, 6 months ago
Dne nedelja, 23. julij 2023 ob 11:18:33 CEST je John Watts napisal(a):
> On Sat, Jul 22, 2023 at 08:15:51AM +1000, John Watts wrote:
> > ...
> > +			/omit-if-no-ref/
> > +			can0_pins: can0-pins {
> > +				pins = "PB2", "PB3";
> > +				function = "can0";
> > +			};
> > ...
> > +		can0: can@2504000 {
> > +			compatible = "allwinner,sun20i-d1-can";
> > +			reg = <0x02504000 0x400>;
> > +			interrupts = <SOC_PERIPHERAL_IRQ(21) 
IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&ccu CLK_BUS_CAN0>;
> > +			resets = <&ccu RST_BUS_CAN0>;
> > +			status = "disabled";
> > +		};
> 
> Just a quick late night question to people with more knowledge than me:
> 
> These chips only have one pinctrl configuration for can0 and can1. Should
> the can nodes have this pre-set instead of the board dts doing this?

Yes, that's usually how it's done.

> 
> I see this happening in sun4i-a10.dtsi for instance, but it also seems like
> it could become a problem when it comes to re-using the dtsi for newer chip
> variants.

Properties can be either rewritten or deleted further down, so don't worry 
about that.

Best regards,
Jernej

> 
> John.
Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes
Posted by John Watts 2 years, 6 months ago
On Mon, Jul 31, 2023 at 12:03:59AM +0200, Jernej Škrabec wrote:
> Yes, that's usually how it's done.
> 
> > 
> > I see this happening in sun4i-a10.dtsi for instance, but it also seems like
> > it could become a problem when it comes to re-using the dtsi for newer chip
> > variants.
> 
> Properties can be either rewritten or deleted further down, so don't worry 
> about that.
> 
> Best regards,
> Jernej
> 
> > 
> > John.

Thanks for the feedback, I've sent a patch to address this.

John.