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

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

[Abraham Zukor]

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.

Fixes: f333a331adfa ("spi/tegra114: add spi driver")
Assisted-by: Claude:Opus-5
Signed-off-by: Abraham Zukor <abe@maticrobots.com>
---
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@maticrobots.com/

 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);
+	}
+
 	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

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@maticrobots.com>
> ---
> 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@maticrobots.com/
> 
>  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
> 
>