Re: [PATCH] arm64: dts: rockchip: use cs-gpios for spi1 on ringneck

[Heiko Stuebner]

Hi Jakob,

Am Freitag, 20. Juni 2025, 13:35:46 Mitteleuropäische Sommerzeit schrieb Jakob Unterwurzacher:
> From: Jakob Unterwurzacher <jakob.unterwurzacher@xxxxxxxxx>
> 
> Hardware CS has a very slow rise time of about 6us,
> causing transmission errors when CS does not reach
> high between transaction.
> 
> It looks like it's not driven actively when transitioning
> from low to high but switched to input, so only the CPU
> pull-up pulls it high, slowly. Transitions from high to low
> are fast. On the oscilloscope, CS looks like an irregular sawtooth
> pattern like this:
>                          _____
>               ^         /     |
>       ^      /|        /      |
>      /|     / |       /       |
>     / |    /  |      /        |
> ___/  |___/   |_____/         |___
> 
> With cs-gpios we have a CS rise time of about 20ns, as it should be,
> and CS looks rectangular.
> 
> This fixes the data errors when running a flashcp loop against a
> m25p40 spi flash.
> 
> With the Rockchip 6.1 kernel we see the same slow rise time, but
> for some reason CS is always high for long enough to reach a solid
> high.
> 
> The RK3399 and RK3588 SoCs use the same SPI driver, so we also
> checked our "Puma" (RK3399) and "Tiger" (RK3588) boards.
> They do not have this problem. Hardware CS rise time is good.
> 
> Fixes: c484cf93f61b ("arm64: dts: rockchip: add PX30-µQ7 (Ringneck) SoM with Haikou baseboard")
> Cc: stable@xxxxxxxxxxxxxxx
> Reviewed-by: Quentin Schulz <quentin.schulz@xxxxxxxxx>
> Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@xxxxxxxxx>
> ---
>  .../boot/dts/rockchip/px30-ringneck.dtsi      | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
> index ab232e5c7ad6..dcc62dd9b894 100644
> --- a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
> @@ -379,6 +379,18 @@ pmic_int: pmic-int {
>  				<0 RK_PA7 RK_FUNC_GPIO &pcfg_pull_up>;
>  		};
>  	};
> +
> +	spi1 {
> +		spi1_csn0_gpio: spi1-csn0-gpio {
> +			rockchip,pins =
> +				<3 RK_PB1 RK_FUNC_GPIO &pcfg_pull_up_4ma>;
> +		};
> +
> +		spi1_csn1_gpio: spi1-csn1-gpio {

naming the node -gpio trigger the bot, I guess a better name would
be something with -pin at the end instead.

> +&spi1 {
> +	/*
> +	 * Hardware CS has a very slow rise time of about 6us,
> +	 * causing transmission errors.
> +	 * With cs-gpios we have a rise time of about 20ns.
> +	 */
> +	cs-gpios = <&gpio3 RK_PB1 GPIO_ACTIVE_LOW>, <&gpio3 RK_PB2 GPIO_ACTIVE_LOW>;

please also provide a
	pinctrl-names = "default"
here.
It feels more futur proof when overriding the pinctrl-0 entry
to also state we only expect that one.

> +	pinctrl-0 = <&spi1_clk &spi1_csn0_gpio &spi1_csn1_gpio &spi1_miso &spi1_mosi>;
> +};
> +
>  &tsadc {
>  	status = "okay";
>  };
>