| Message ID | 20160812210610.24636-1-swarren@wwwdotorg.org |
|---|---|
| State | Rejected |
| Delegated to: | Tom Warren |
| Headers | show |
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.
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);