[U-Boot] [PATCH] spi: tegra20: fix mode selection logic
From: Stephen Warren swarren@nvidia.com
When the set_mode() function runs, the SPI bus is not active, and hence the clocks to the SPI controller are not running. Any register read/write at this time will hang the CPU. Remove the code from set_mode() that does this, and move it to the correct place in claim_bus().
This essentially reverts and re-implements the patch mentioned in the fixes tag below. I'm not sure how the original could ever have worked on any Tegra platform; it certainly breaks the only Tegra board I have that uses SPI.
Fixes: 5cb1b7b395c0 ("spi: tegra20: Add support for mode selection") Cc: Mirza Krak mirza.krak@hostmobility.com Signed-off-by: Stephen Warren swarren@nvidia.com --- As far as I can tell, the fixed patch was never CC'd to any Tegra maintainer:-( --- drivers/spi/tegra20_slink.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/spi/tegra20_slink.c b/drivers/spi/tegra20_slink.c index 238edec23ba5..0e167ccac053 100644 --- a/drivers/spi/tegra20_slink.c +++ b/drivers/spi/tegra20_slink.c @@ -151,6 +151,14 @@ static int tegra30_spi_claim_bus(struct udevice *dev) /* Set master mode and sw controlled CS */ reg = readl(®s->command); reg |= SLINK_CMD_M_S | SLINK_CMD_CS_SOFT; + /* Set CPOL and CPHA */ + reg &= ~(SLINK_CMD_IDLE_SCLK_MASK | SLINK_CMD_CK_SDA); + if (priv->mode & SPI_CPHA) + reg |= SLINK_CMD_CK_SDA; + if (priv->mode & SPI_CPOL) + reg |= SLINK_CMD_IDLE_SCLK_DRIVE_HIGH; + else + reg |= SLINK_CMD_IDLE_SCLK_DRIVE_LOW; writel(reg, ®s->command); debug("%s: COMMAND = %08x\n", __func__, readl(®s->command));
@@ -321,22 +329,6 @@ static int tegra30_spi_set_speed(struct udevice *bus, uint speed) static int tegra30_spi_set_mode(struct udevice *bus, uint mode) { struct tegra30_spi_priv *priv = dev_get_priv(bus); - struct spi_regs *regs = priv->regs; - u32 reg; - - reg = readl(®s->command); - - /* Set CPOL and CPHA */ - reg &= ~(SLINK_CMD_IDLE_SCLK_MASK | SLINK_CMD_CK_SDA); - if (mode & SPI_CPHA) - reg |= SLINK_CMD_CK_SDA; - - if (mode & SPI_CPOL) - reg |= SLINK_CMD_IDLE_SCLK_DRIVE_HIGH; - else - reg |= SLINK_CMD_IDLE_SCLK_DRIVE_LOW; - - writel(reg, ®s->command);
priv->mode = mode; debug("%s: regs=%p, mode=%d\n", __func__, priv->regs, priv->mode);
2016-08-12 23:06 GMT+02:00 Stephen Warren swarren@wwwdotorg.org:
From: Stephen Warren swarren@nvidia.com
When the set_mode() function runs, the SPI bus is not active, and hence the clocks to the SPI controller are not running. Any register read/write at this time will hang the CPU. Remove the code from set_mode() that does this, and move it to the correct place in claim_bus().
This essentially reverts and re-implements the patch mentioned in the fixes tag below. I'm not sure how the original could ever have worked on any Tegra platform; it certainly breaks the only Tegra board I have that uses SPI.
Hi Stephen.
This has most definitely worked for me on both Tegra2 (colibri_t20) and Tegra3(colibri_t30). Though I am using a 2015.04 u-boot which was the release when I wrote this patch, and I haven't actually tried any later releases. Something happened along the way that "broke" it?
Fixes: 5cb1b7b395c0 ("spi: tegra20: Add support for mode selection") Cc: Mirza Krak mirza.krak@hostmobility.com Signed-off-by: Stephen Warren swarren@nvidia.com
As far as I can tell, the fixed patch was never CC'd to any Tegra maintainer:-(
This patch was one of my first contributions to an open-source project and I might have missed a few steps, like CC the Tegra maintainers.
I know better now, sorry for any inconvenience caused by this.
2016-08-13 10:51 GMT+02:00 Mirza Krak mirza.krak@hostmobility.com:
2016-08-12 23:06 GMT+02:00 Stephen Warren swarren@wwwdotorg.org:
From: Stephen Warren swarren@nvidia.com
When the set_mode() function runs, the SPI bus is not active, and hence the clocks to the SPI controller are not running. Any register read/write at this time will hang the CPU. Remove the code from set_mode() that does this, and move it to the correct place in claim_bus().
This essentially reverts and re-implements the patch mentioned in the fixes tag below. I'm not sure how the original could ever have worked on any Tegra platform; it certainly breaks the only Tegra board I have that uses SPI.
Hi Stephen.
This has most definitely worked for me on both Tegra2 (colibri_t20) and Tegra3(colibri_t30). Though I am using a 2015.04 u-boot which was the release when I wrote this patch, and I haven't actually tried any later releases. Something happened along the way that "broke" it?
Looked in my u-boot source tree, and I see that I actually do the mode selection in claim_bus and not in set_mode, where I only cache the value (like your patch does).
I probably sent out the wrong patch file back then resulting in broken driver :(.
On 13 August 2016 at 02:36, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
When the set_mode() function runs, the SPI bus is not active, and hence the clocks to the SPI controller are not running. Any register read/write at this time will hang the CPU. Remove the code from set_mode() that does this, and move it to the correct place in claim_bus().
The idea of claim_bus is just to enable the bus for any transaction to start, since set_mode is running before claim (ex: spi_get_bus_and_cs while 'sf probe') it's .probe which actual driver binding responsibility to initialize the SPI bus so-that .set_mode and .set_speed will set the mode and freq for that initialized bus based on the inputs from user, drivers like zynq, exynos will follow the same.
On 08/13/2016 09:56 AM, Jagan Teki wrote:
On 13 August 2016 at 02:36, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
When the set_mode() function runs, the SPI bus is not active, and hence the clocks to the SPI controller are not running. Any register read/write at this time will hang the CPU. Remove the code from set_mode() that does this, and move it to the correct place in claim_bus().
The idea of claim_bus is just to enable the bus for any transaction to start, since set_mode is running before claim (ex: spi_get_bus_and_cs while 'sf probe') it's .probe which actual driver binding responsibility to initialize the SPI bus so-that .set_mode and .set_speed will set the mode and freq for that initialized bus based on the inputs from user, drivers like zynq, exynos will follow the same.
I'd rather not re-structure the driver, and to be honest I see no point in mandating that drivers activate their clocks/resets in probe rather than solely during the actual SPI transaction.
Anyway, if the patch I sent isn't acceptable, please can you simply revert the patch it fixes so that SPI on Tegra works again? Thanks.
On 08/15/2016 09:35 AM, Stephen Warren wrote:
On 08/13/2016 09:56 AM, Jagan Teki wrote:
On 13 August 2016 at 02:36, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
When the set_mode() function runs, the SPI bus is not active, and hence the clocks to the SPI controller are not running. Any register read/write at this time will hang the CPU. Remove the code from set_mode() that does this, and move it to the correct place in claim_bus().
The idea of claim_bus is just to enable the bus for any transaction to start, since set_mode is running before claim (ex: spi_get_bus_and_cs while 'sf probe') it's .probe which actual driver binding responsibility to initialize the SPI bus so-that .set_mode and .set_speed will set the mode and freq for that initialized bus based on the inputs from user, drivers like zynq, exynos will follow the same.
I'd rather not re-structure the driver, and to be honest I see no point in mandating that drivers activate their clocks/resets in probe rather than solely during the actual SPI transaction.
Anyway, if the patch I sent isn't acceptable, please can you simply revert the patch it fixes so that SPI on Tegra works again? Thanks.
It turns out that getting the clocks going in probe() is pretty easy. I've sent a patch to do that, so this patch can be dropped.
participants (3)
-
Jagan Teki -
Mirza Krak -
Stephen Warren