[v2] riscv: sophgo: dts: Add spi controller for SG2042

Commit Message

damon Feb. 28, 2025, 12:40 p.m. UTC

Add spi controllers for SG2042.

SG2042 uses the upstreamed Synopsys DW SPI IP.

Signed-off-by: Zixian Zeng <[e-mail]>
---
For this spi controller patch, only bindings are included.
This is tested on milkv-pioneer board. Using driver/spi/spidev.c
for creating /dev/spidevX.Y and tools/spi/spidev_test for testing
functionality.
---
Changes in v2:
- rebase v1 to sophgo/master(github.com/sophgo/linux.git).
- order properties in device node.
- remove unevaluated properties `clock-frequency`.
- set default status to disable.
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 .../riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts |  8 +++++++
 arch/riscv/boot/dts/sophgo/sg2042.dtsi             | 28 ++++++++++++++++++++++
 2 files changed, 36 insertions(+)


---
base-commit: aa5ee7180ec41bb77c3e327e95d119f2294babea
change-id: 20250228-sfg-spi-e3f2aeca09ab

Best regards,

Comments

Conor Dooley Feb. 28, 2025, 6:22 p.m. UTC | #1

On Fri, Feb 28, 2025 at 08:40:23PM +0800, Zixian Zeng wrote:
> Add spi controllers for SG2042.
> 
> SG2042 uses the upstreamed Synopsys DW SPI IP.
> 
> Signed-off-by: Zixian Zeng <[e-mail]>
> ---
> For this spi controller patch, only bindings are included.

^^^ you've not actually included any bindings in this patch, copy-paste
mistake?

> This is tested on milkv-pioneer board. Using driver/spi/spidev.c
> for creating /dev/spidevX.Y and tools/spi/spidev_test for testing
> functionality.
> ---
> Changes in v2:
> - rebase v1 to sophgo/master(github.com/sophgo/linux.git).
> - order properties in device node.
> - remove unevaluated properties `clock-frequency`.
> - set default status to disable.
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  .../riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts |  8 +++++++
>  arch/riscv/boot/dts/sophgo/sg2042.dtsi             | 28 ++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> index be596d01ff8d33bcdbe431d9731a55ee190ad5b3..c43a807af2f827b5267afe5e4fdf6e9e857dfa20 100644
> --- a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> +++ b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> @@ -72,6 +72,14 @@ &uart0 {
>  	status = "okay";
>  };
>  
> +&spi0 {
> +	status = "okay";
> +};
> +
> +&spi1 {
> +	status = "okay";
> +};
> +
>  / {
>  	thermal-zones {
>  		soc-thermal {
> diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> index e62ac51ac55abd922b5ef796ba8c2196383850c4..500645147b1f8ed0a08ad3cafb38ea79cf57d737 100644
> --- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> @@ -545,5 +545,33 @@ sd: mmc@704002b000 {
>  				      "timer";
>  			status = "disabled";
>  		};
> +
> +		spi0: spi@7040004000 {
> +			compatible = "snps,dw-apb-ssi";

I thought were were dropping the use of "snps,dw-abp-ssi" in isolation,
and starting to require soc-specific compatibles now.

Rob, Krzysztof?

Chen Wang March 1, 2025, 1:58 a.m. UTC | #2

On 2025/3/1 2:22, Conor Dooley wrote:
[......]
>> +
>> +		spi0: spi@7040004000 {
>> +			compatible = "snps,dw-apb-ssi";
> I thought were were dropping the use of "snps,dw-abp-ssi" in isolation,
> and starting to require soc-specific compatibles now.
>
> Rob, Krzysztof?
I'm also very interested to know why we can't just use 
"snps,dw-abp-ssi", maybe I missed some discussion ...
I googled examples of soc-specific defined in the code, and it doesn't 
seem to be much, only arch/mips/boot/dts/mscc/ocelot.dtsi and 
arch/riscv/boot/dts/thead/th1520.dtsi.
Specially, I looked at the commits for th1520 and saw this 
https://lore.kernel.org/linux-riscv/20240703-garbage-explicit-bd95f8deb716@wendy/. 
It tells if the fallback works identically, then the specific compatible 
is not needed.

This makes me more confused :)

Regards,

Chen

damon March 1, 2025, 1:41 p.m. UTC | #3

On 25/02/28 06:22PM, Conor Dooley wrote:
> On Fri, Feb 28, 2025 at 08:40:23PM +0800, Zixian Zeng wrote:
> > Add spi controllers for SG2042.
> >
> > SG2042 uses the upstreamed Synopsys DW SPI IP.
> >
> > Signed-off-by: Zixian Zeng <[e-mail]>
> > ---
> > For this spi controller patch, only bindings are included.
>
> ^^^ you've not actually included any bindings in this patch, copy-paste
> mistake?
>
My bad, I was confused about what’s the “binding” meaning. I will
remove this description in next revision.
> > This is tested on milkv-pioneer board. Using driver/spi/spidev.c
> > for creating /dev/spidevX.Y and tools/spi/spidev_test for testing
> > functionality.
> > ---
> > Changes in v2:
> > - rebase v1 to sophgo/master(github.com/sophgo/linux.git).
> > - order properties in device node.
> > - remove unevaluated properties `clock-frequency`.
> > - set default status to disable.
> > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > ---
> >  .../riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts |  8 +++++++
> >  arch/riscv/boot/dts/sophgo/sg2042.dtsi             | 28 ++++++++++++++++++++++
> >  2 files changed, 36 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> > index be596d01ff8d33bcdbe431d9731a55ee190ad5b3..c43a807af2f827b5267afe5e4fdf6e9e857dfa20 100644
> > --- a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> > +++ b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> > @@ -72,6 +72,14 @@ &uart0 {
> >     status = "okay";
> >  };
> >
> > +&spi0 {
> > +   status = "okay";
> > +};
> > +
> > +&spi1 {
> > +   status = "okay";
> > +};
> > +
> >  / {
> >     thermal-zones {
> >             soc-thermal {
> > diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> > index e62ac51ac55abd922b5ef796ba8c2196383850c4..500645147b1f8ed0a08ad3cafb38ea79cf57d737 100644
> > --- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> > +++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> > @@ -545,5 +545,33 @@ sd: mmc@704002b000 {
> >                                   "timer";
> >                     status = "disabled";
> >             };
> > +
> > +           spi0: spi@7040004000 {
> > +                   compatible = "snps,dw-apb-ssi";
>
> I thought were were dropping the use of "snps,dw-abp-ssi" in isolation,
> and starting to require soc-specific compatibles now.
>
That’s better, I will do it in next revision.
BTW, <https://lore.kernel.org/linux-riscv/20240703-garbage-explicit-bd95f8deb716@wendy>
has similar situation,
those patches only create binding but not implement the compatible
property of "thead,th1520-spi” in driver.
When kernel matching, it will find out there’s no compatible of
"thead,th1520-spi” so fallback to "snps,dw-apb-ssi”

May I take you a few minutes for reason behind it? My understanding is
that using soc-specific compatible is better
for future maintaining, If the device actually comes with some unknown
quirks not present in the generic IP.
And all we need to do for fixing is implementing the soc-specific
compatible property.

> Rob, Krzysztof?
>

Best regards,
Zixian Zeng

Inochi Amaoto March 2, 2025, 2:18 a.m. UTC | #4

On Fri, Feb 28, 2025 at 08:40:23PM +0800, Zixian Zeng wrote:
> Add spi controllers for SG2042.
> 
> SG2042 uses the upstreamed Synopsys DW SPI IP.
> 
> Signed-off-by: Zixian Zeng <[e-mail]>
> ---
> For this spi controller patch, only bindings are included.
> This is tested on milkv-pioneer board. Using driver/spi/spidev.c
> for creating /dev/spidevX.Y and tools/spi/spidev_test for testing
> functionality.

I wonder whether this patch is tested (or at least can be
tested), as I am not sure there are pins for spi or any
onboard device on existing SG2042 board.

> ---
> Changes in v2:
> - rebase v1 to sophgo/master(github.com/sophgo/linux.git).
> - order properties in device node.
> - remove unevaluated properties `clock-frequency`.
> - set default status to disable.
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  .../riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts |  8 +++++++
>  arch/riscv/boot/dts/sophgo/sg2042.dtsi             | 28 ++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> index be596d01ff8d33bcdbe431d9731a55ee190ad5b3..c43a807af2f827b5267afe5e4fdf6e9e857dfa20 100644
> --- a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> +++ b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
> @@ -72,6 +72,14 @@ &uart0 {
>  	status = "okay";
>  };
>  
> +&spi0 {
> +	status = "okay";
> +};
> +
> +&spi1 {
> +	status = "okay";
> +};
> +
>  / {
>  	thermal-zones {
>  		soc-thermal {
> diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> index e62ac51ac55abd922b5ef796ba8c2196383850c4..500645147b1f8ed0a08ad3cafb38ea79cf57d737 100644
> --- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> @@ -545,5 +545,33 @@ sd: mmc@704002b000 {
>  				      "timer";
>  			status = "disabled";
>  		};
> +
> +		spi0: spi@7040004000 {
> +			compatible = "snps,dw-apb-ssi";
> +			reg = <0x70 0x40004000 0x00 0x1000>;

> +			clocks = <&clkgen GATE_CLK_APB_SPI>,
> +					<&clkgen GATE_CLK_SYSDMA_AXI>;

Is the sysdma axi clock for the this device. IIRC is APB interface.

Also, Are these clock aligned? If not, use space to align the two
clocks. You also need to add the clock-names here.

> +			interrupt-parent = <&intc>;
> +			interrupts = <110 IRQ_TYPE_LEVEL_HIGH>;

> +			#address-cells = <0x01>;
> +			#size-cells = <0x00>;
> +			num-cs = <0x02>;

Just use decimal number, no hex there.

> +			resets = <&rstgen RST_SPI0>;
> +			status = "disabled";
> +		};
> +
> +		spi1: spi@7040005000 {
> +			compatible = "snps,dw-apb-ssi";
> +			reg = <0x70 0x40005000 0x00 0x1000>;

> +			clocks = <&clkgen GATE_CLK_APB_SPI>,
> +					<&clkgen GATE_CLK_SYSDMA_AXI>;

The same as above.

> +			interrupt-parent = <&intc>;
> +			interrupts = <111 IRQ_TYPE_LEVEL_HIGH>;

> +			#address-cells = <0x01>;
> +			#size-cells = <0x00>;
> +			num-cs = <0x02>;

The same as above.

> +			resets = <&rstgen RST_SPI1>;
> +			status = "disabled";
> +		};
>  	};
>  };
> 
> ---
> base-commit: aa5ee7180ec41bb77c3e327e95d119f2294babea
> change-id: 20250228-sfg-spi-e3f2aeca09ab
> 
> Best regards,
> -- 
> Zixian Zeng <[e-mail]>
>

Conor Dooley March 3, 2025, 9:08 a.m. UTC | #5

On Sat, Mar 01, 2025 at 09:58:24AM +0800, Chen Wang wrote:
> 
> On 2025/3/1 2:22, Conor Dooley wrote:
> [......]
> > > +
> > > +		spi0: spi@7040004000 {
> > > +			compatible = "snps,dw-apb-ssi";
> > I thought were were dropping the use of "snps,dw-abp-ssi" in isolation,
> > and starting to require soc-specific compatibles now.
> > 
> > Rob, Krzysztof?
> I'm also very interested to know why we can't just use "snps,dw-abp-ssi",
> maybe I missed some discussion ...

Ordinarily you're not allowed to use generic compatibles at all, the
dw stuff has kinda been an ?unofficial? exception for a while and not
just for spi. I think that "exception" has been withdrawn though, and
dw stuff is being brought in line with everything else, but cannot
remember where that started.

> I googled examples of soc-specific defined in the code, and it doesn't seem
> to be much, only arch/mips/boot/dts/mscc/ocelot.dtsi and
> arch/riscv/boot/dts/thead/th1520.dtsi.

> Specially, I looked at the commits for th1520 and saw this https://lore.kernel.org/linux-riscv/20240703-garbage-explicit-bd95f8deb716@wendy/.
> It tells if the fallback works identically, then the specific compatible is
> not needed.

This link is talking about the driver, not the dts/binding. Adding the
device-specific compatible to the driver is only needed if the specific
one has some extra features above and beyond the compatible used as the
fallback.

Cheers,
Conor.

Conor Dooley March 3, 2025, 9:12 a.m. UTC | #6

On Sat, Mar 01, 2025 at 09:41:14PM +0800, damon wrote:
> On 25/02/28 06:22PM, Conor Dooley wrote:
> > On Fri, Feb 28, 2025 at 08:40:23PM +0800, Zixian Zeng wrote:

> > > diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> > > index e62ac51ac55abd922b5ef796ba8c2196383850c4..500645147b1f8ed0a08ad3cafb38ea79cf57d737 100644
> > > --- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> > > +++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> > > @@ -545,5 +545,33 @@ sd: mmc@704002b000 {
> > >                                   "timer";
> > >                     status = "disabled";
> > >             };
> > > +
> > > +           spi0: spi@7040004000 {
> > > +                   compatible = "snps,dw-apb-ssi";
> >
> > I thought were were dropping the use of "snps,dw-abp-ssi" in isolation,
> > and starting to require soc-specific compatibles now.
> >
> That’s better, I will do it in next revision.
> BTW, <https://lore.kernel.org/linux-riscv/20240703-garbage-explicit-bd95f8deb716@wendy>
> has similar situation,
> those patches only create binding but not implement the compatible
> property of "thead,th1520-spi” in driver.
> When kernel matching, it will find out there’s no compatible of
> "thead,th1520-spi” so fallback to "snps,dw-apb-ssi”

> May I take you a few minutes for reason behind it? My understanding is
> that using soc-specific compatible is better
> for future maintaining, If the device actually comes with some unknown
> quirks not present in the generic IP.

Yup, that's pretty much it. Those quirks could be some sort of bug etc,
or they could be an extra feature. Sometimes the specific compatible is
only used to enforce constraints on what properties are allowed, but
that's more of a thing for the more configurable IPs like the dwmac.

> And all we need to do for fixing is implementing the soc-specific
> compatible property.

Correct. You just need to add a specific compatible to the binding and
to the dts, the driver will work just fine with the "fallback" dw one.

Patch

diff --git a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
index be596d01ff8d33bcdbe431d9731a55ee190ad5b3..c43a807af2f827b5267afe5e4fdf6e9e857dfa20 100644
--- a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
+++ b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
@@ -72,6 +72,14 @@  &uart0 {
 	status = "okay";
 };
 
+&spi0 {
+	status = "okay";
+};
+
+&spi1 {
+	status = "okay";
+};
+
 / {
 	thermal-zones {
 		soc-thermal {
diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
index e62ac51ac55abd922b5ef796ba8c2196383850c4..500645147b1f8ed0a08ad3cafb38ea79cf57d737 100644
--- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
+++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
@@ -545,5 +545,33 @@  sd: mmc@704002b000 {
 				      "timer";
 			status = "disabled";
 		};
+
+		spi0: spi@7040004000 {
+			compatible = "snps,dw-apb-ssi";
+			reg = <0x70 0x40004000 0x00 0x1000>;
+			clocks = <&clkgen GATE_CLK_APB_SPI>,
+					<&clkgen GATE_CLK_SYSDMA_AXI>;
+			interrupt-parent = <&intc>;
+			interrupts = <110 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <0x01>;
+			#size-cells = <0x00>;
+			num-cs = <0x02>;
+			resets = <&rstgen RST_SPI0>;
+			status = "disabled";
+		};
+
+		spi1: spi@7040005000 {
+			compatible = "snps,dw-apb-ssi";
+			reg = <0x70 0x40005000 0x00 0x1000>;
+			clocks = <&clkgen GATE_CLK_APB_SPI>,
+					<&clkgen GATE_CLK_SYSDMA_AXI>;
+			interrupt-parent = <&intc>;
+			interrupts = <111 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <0x01>;
+			#size-cells = <0x00>;
+			num-cs = <0x02>;
+			resets = <&rstgen RST_SPI1>;
+			status = "disabled";
+		};
 	};
 };