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

[Jon Hunter <jonathanh@nvidia.com>]

On 26/08/2026 01:05, 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 valid chip select without
> spi-cs-high. That way 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.
> 
> Fixes: f333a331adfa ("spi/tegra114: add spi driver")
> Assisted-by: Claude:Opus-5
> Signed-off-by: Abraham Zukor <abe@maticrobots.com>
> ---
> v3:
> - Drop the cs-gpios check; programming an unused native chip select does
>    not affect a device using a GPIO chip select.
> - Split the compound condition into separate checks for readability.
> - Retest on a Jetson Orin Nano with CS1 disabled. The old module left CS1
>    asserted, while the patched module parked both chip selects inactive.
> 
> v2:
> - Derive the inactive level per chip select from the device tree instead
>    of setting the whole CS_POL_INACTIVE field, which is incorrect for an
>    active-high or mixed-polarity bus.
> 
> drivers/spi/spi-tegra114.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c
> index aa44ffd09e61..463bb7e61b6e 100644
> --- a/drivers/spi/spi-tegra114.c
> +++ b/drivers/spi/spi-tegra114.c
> @@ -1395,6 +1395,32 @@ 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;
> +
> +		if (of_property_read_bool(np, "spi-cs-high"))
> +			continue;
> +
> +		if (of_property_read_u32(np, "reg", &cs))
> +			continue;
> +
> +		if (cs >= MAX_CHIP_SELECT)
> +			continue;
> +
> +		tspi->def_command1_reg |= SPI_CS_POL_INACTIVE(cs);
> +	}
> +

Thanks for updating this. This looks fine to me, the only comment
I have is whether we should check and warn for any badly configured
device-trees where the same CS is used on multiple devices even if
one is disabled? It shouldn't be, but could be good to flag this.

Jon

-- 
nvpublic