Re: [PATCH v3 3/3] riscv: dts: spacemit: define a SPI controller node

[Yixun Lan <dlan@gentoo.org>]

Hi Alex,

On 11:17 Mon 22 Sep     , Alex Elder wrote:
> Define a node for the fourth SoC SPI controller (number 3) on
> the SpacemiT K1 SoC.
> 
> Enable it on the Banana Pi BPI-F3 board, which exposes this feature
> via its GPIO block:
>   GPIO PIN 19:  MOSI
>   GPIO PIN 21:  MISO
>   GPIO PIN 23:  SCLK
>   GPIO PIN 24:  SS (inverted)
> 
> Define pincontrol configurations for the pins as used on that board.
> 
> (This was tested using a GigaDevice GD25Q64E SPI NOR chip.)
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v3: - Moved the SPI controller into the dma-bus memory region
> 
>  .../boot/dts/spacemit/k1-bananapi-f3.dts      |  7 +++++++
>  arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi  | 20 +++++++++++++++++++
>  arch/riscv/boot/dts/spacemit/k1.dtsi          | 16 +++++++++++++++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> index 2aaaff77831e1..d9d865fbe320e 100644
> --- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> +++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> @@ -14,6 +14,7 @@ aliases {
>  		ethernet0 = &eth0;
>  		ethernet1 = &eth1;
>  		serial0 = &uart0;
> +		spi3 = &spi3;
>  	};
>  
>  	chosen {
> @@ -92,6 +93,12 @@ &pdma {
>  	status = "okay";
>  };
>  
> +&spi3 {
> +	pinctrl-0 = <&ssp3_0_cfg>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
>  &uart0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&uart0_2_cfg>;
> diff --git a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
> index aff19c86d5ff3..205c201a3005c 100644
> --- a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
> @@ -76,4 +76,24 @@ pwm14-1-pins {
>  			drive-strength = <32>;
>  		};
>  	};
> +
> +	ssp3_0_cfg: ssp3-0-cfg {
..
> +		ssp3-0-no-pull-pins {
I'd prefer not to enforce "pull" info inside the name, you can't embed
all property info, besides, what's if you want to change/override later?

how about just name it as ssp3-0-defaul-pins or simply ssp3-0-pins?

> +			pinmux = <K1_PADCONF(75, 2)>,	/* SCLK */
> +				 <K1_PADCONF(77, 2)>,	/* MOSI  */
> +				 <K1_PADCONF(78, 2)>;	/* MISO */
> +
> +			bias-disable;
> +			drive-strength = <19>;
> +			power-source = <3300>;
> +		};
> +
> +		ssp3-0-frm-pins {
> +			pinmux = <K1_PADCONF(76, 2)>;	/* FRM (frame) */
> +
> +			bias-pull-up = <0>;
> +			drive-strength = <19>;
> +			power-source = <3300>;
> +		};
> +	};
>  };
> diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
> index 6cdcd80a7c83b..eb8a14dd72ea4 100644
> --- a/arch/riscv/boot/dts/spacemit/k1.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
> @@ -797,6 +797,22 @@ uart9: serial@d4017800 {
>  				status = "disabled";
>  			};
>  
> +			spi3: spi@d401c000 {
> +				compatible = "spacemit,k1-spi";
> +				reg = <0x0 0xd401c000 0x0 0x30>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&syscon_apbc CLK_SSP3>,
> +					 <&syscon_apbc CLK_SSP3_BUS>;
> +				clock-names = "core", "bus";
> +				resets = <&syscon_apbc RESET_SSP3>;
> +				interrupts = <55>;
..
> +				dmas = <&pdma 20>,
> +				       <&pdma 19>;
can we also squash the dmas into one line? but, do split if there are too many..

yes, it's simply a style change that I'd like to keep them consistent at DT level,
besides you might also want to adjust dt-binding examples to align with them here..

thanks

> +				dma-names = "rx", "tx";
> +				status = "disabled";
> +			};
> +
>  			/* sec_uart1: 0xf0612000, not available from Linux */
>  		};
>  
> -- 
> 2.48.1
> 
> 

-- 
Yixun Lan (dlan)

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv