Re: [PATCH v2] spi: tegra114: initialize native chip selects inactive

[Mikko Perttunen]

On Friday, July 31, 2026 12:01 PM Abraham Zukor wrote:
> tegra_spi_probe() initializes SPI_COMMAND1 with only SPI_M_S, leaving
> CS_POL_INACTIVE clear for every chip select. This drives every native
> active-low chip select low until tegra_spi_setup() runs for that device.
> 
> SPI children are registered and probed one at a time. A synchronous
> probe of an earlier child can therefore transfer while a later child's
> chip select is still asserted. On a Tegra234 system with active-low
> devices on CS0 and CS1, this caused both devices to be selected when the
> CS1 device probed first. Its initialization then intermittently failed.
> A logic analyzer showed CS0 remained low throughout the CS1 transfer and
> went high only after the CS1 probe failed.
> 
> Walk the controller's device tree children before SPI_COMMAND1 is
> written and set CS_POL_INACTIVE for each native chip select a child
> claims without spi-cs-high. That is the level tegra_spi_setup() programs
> for the same device later, so the line is already deasserted when the
> first child transfers and does not change level when setup runs.
> 
> Disabled children are walked too. The core only registers available
> children, so tegra_spi_setup() never runs for a disabled one and probe
> is the only chance to park its chip select at the level its hardware
> needs.
> 
> Every other bit keeps the zero the driver has always written. A chip
> select with no child in the device tree, one whose child asks for
> spi-cs-high, and one driven by a GPIO are all left as before.

> An
> active-low device on a chip select the device tree does not describe is
> therefore still asserted while a declared device probes, which is a
> pre-existing problem this does not address.

I don't think this is a problem, as the device tree is supposed to 
describe the visible hardware.

> 
> Fixes: f333a331adfa ("spi/tegra114: add spi driver")
> Assisted-by: Claude:Opus-5
> Signed-off-by: Abraham Zukor <abe@xxxxxxxxxxxxxxx>
> ---
> v2: derive the inactive level per chip select from the device tree rather
> than setting the whole CS_POL_INACTIVE field. A single constant is wrong
> for an active-high or a mixed-polarity bus, as Jon pointed out on v1.
> 
> Tested on a Jetson Orin Nano carrier board with active-low devices on CS0
> and CS1, and with the CS1 node disabled to confirm its chip select is
> still parked inactive.
> 
> v1: https://lore.kernel.org/r/20260725042944.1204409-1-abe@xxxxxxxxxxxxxxx/
> 
>  drivers/spi/spi-tegra114.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c
> index aa44ffd09e61..d82b88c4fa9b 100644
> --- a/drivers/spi/spi-tegra114.c
> +++ b/drivers/spi/spi-tegra114.c
> @@ -1395,6 +1395,31 @@ static int tegra_spi_probe(struct platform_device *pdev)
>  	udelay(2);
>  	reset_control_deassert(tspi->rst);
>  	tspi->def_command1_reg  = SPI_M_S;
> +
> +	/*
> +	 * SPI_COMMAND1 is written before any child is registered, so a chip
> +	 * select with CS_POL_INACTIVE clear stays asserted until
> +	 * tegra_spi_setup() runs for that device. Program the level the
> +	 * device tree asks for up front instead.
> +	 *
> +	 * Disabled children are included deliberately. The core never
> +	 * registers them, so this is the only chance to park their chip
> +	 * select at the level their hardware needs.
> +	 */
> +	for_each_child_of_node_scoped(pdev->dev.of_node, np) {
> +		u32 cs;
> +
> +		/* Only an active-low native chip select needs its bit set. */
> +		if (of_property_read_bool(np, "spi-cs-high") ||
> +		    of_property_read_u32(np, "reg", &cs) ||
> +		    cs >= MAX_CHIP_SELECT ||
> +		    !of_parse_phandle_with_args(pdev->dev.of_node, "cs-gpios",
> +						"#gpio-cells", cs, NULL))
> +			continue;
> +
> +		tspi->def_command1_reg |= SPI_CS_POL_INACTIVE(cs);
> +	}

Why do we need to consider cs-gpios? If a device has a cs-gpio, whatever 
we program here wouldn't reach the device. The if condition is also very 
difficult to read -- please use multiple if statements and normal error 
handling patterns.

Thanks
Mikko

> +
>  	tegra_spi_writel(tspi, tspi->def_command1_reg, SPI_COMMAND1);
>  	tspi->spi_cs_timing1 = tegra_spi_readl(tspi, SPI_CS_TIMING1);
>  	tspi->spi_cs_timing2 = tegra_spi_readl(tspi, SPI_CS_TIMING2);
> -- 
> 2.54.0
> 
>