| Message ID | 20160818165333.21160-1-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Commit | 4832c7f5f79feebf8549f33c7257dec47c336470 |
| Delegated to: | Tom Warren |
| Headers | show |
On 18 August 2016 at 22:23, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > In tegra20_slink.c, the set_mode() function may be executed before the > SPI bus is claimed the first time, and hence the clocks to the SPI > controller may not be running. If so, any register read/write at this > time will hang the CPU. Fix this by ensuring the clock is running as soon > as the driver is probed. This is observed on the Tegra30 Beaver board. > > Apply the same clock initialization fix to all other Tegra SPI drivers so > that if set_mode() is ever implemented there, the same bug will not appear. > Note that tegra114_spi.c already operates in this fashion. > > The clock manipulation code is copied from claim_bus() to probe() rather > than moved. This ensures that any calls to set_speed() take effect; the > clock can't be set once during probe and left unchanged. Do we need to set the clock for claim_bus() as well? I think it's better to move this on to set_speed so-that any call to set_speed() will update the same. I don't think claim_bus would require this again. thanks!
On 08/19/2016 10:56 AM, Jagan Teki wrote: > On 18 August 2016 at 22:23, Stephen Warren <swarren@wwwdotorg.org> wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> In tegra20_slink.c, the set_mode() function may be executed before the >> SPI bus is claimed the first time, and hence the clocks to the SPI >> controller may not be running. If so, any register read/write at this >> time will hang the CPU. Fix this by ensuring the clock is running as soon >> as the driver is probed. This is observed on the Tegra30 Beaver board. >> >> Apply the same clock initialization fix to all other Tegra SPI drivers so >> that if set_mode() is ever implemented there, the same bug will not appear. >> Note that tegra114_spi.c already operates in this fashion. >> >> The clock manipulation code is copied from claim_bus() to probe() rather >> than moved. This ensures that any calls to set_speed() take effect; the >> clock can't be set once during probe and left unchanged. > > Do we need to set the clock for claim_bus() as well? I think it's > better to move this on to set_speed so-that any call to set_speed() > will update the same. I don't think claim_bus would require this > again. I explained this in the commit message: The clock rate needs to be set in claim_bus() so that the frequency requested by set_speed() is in force.
Hi Stephen, On 18 August 2016 at 10:53, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > In tegra20_slink.c, the set_mode() function may be executed before the > SPI bus is claimed the first time, and hence the clocks to the SPI > controller may not be running. If so, any register read/write at this > time will hang the CPU. Fix this by ensuring the clock is running as soon > as the driver is probed. This is observed on the Tegra30 Beaver board. > > Apply the same clock initialization fix to all other Tegra SPI drivers so > that if set_mode() is ever implemented there, the same bug will not appear. > Note that tegra114_spi.c already operates in this fashion. > > The clock manipulation code is copied from claim_bus() to probe() rather > than moved. This ensures that any calls to set_speed() take effect; the > clock can't be set once during probe and left unchanged. > > Fixes: 5cb1b7b395c0 ("spi: tegra20: Add support for mode selection") > Cc: Mirza Krak <mirza.krak@hostmobility.com> > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > drivers/spi/tegra20_sflash.c | 4 ++++ > drivers/spi/tegra20_slink.c | 4 ++++ > drivers/spi/tegra210_qspi.c | 3 +++ > 3 files changed, 11 insertions(+) Reviewed-by: Simon Glass <sjg@chromium.org> You might consider storing the mode and not acting on it until later. But this is fine. Regards, Simon
On 08/20/2016 05:52 PM, Simon Glass wrote: > Hi Stephen, > > On 18 August 2016 at 10:53, Stephen Warren <swarren@wwwdotorg.org> wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> In tegra20_slink.c, the set_mode() function may be executed before the >> SPI bus is claimed the first time, and hence the clocks to the SPI >> controller may not be running. If so, any register read/write at this >> time will hang the CPU. Fix this by ensuring the clock is running as soon >> as the driver is probed. This is observed on the Tegra30 Beaver board. >> >> Apply the same clock initialization fix to all other Tegra SPI drivers so >> that if set_mode() is ever implemented there, the same bug will not appear. >> Note that tegra114_spi.c already operates in this fashion. >> >> The clock manipulation code is copied from claim_bus() to probe() rather >> than moved. This ensures that any calls to set_speed() take effect; the >> clock can't be set once during probe and left unchanged. >> >> Fixes: 5cb1b7b395c0 ("spi: tegra20: Add support for mode selection") >> Cc: Mirza Krak <mirza.krak@hostmobility.com> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> --- >> drivers/spi/tegra20_sflash.c | 4 ++++ >> drivers/spi/tegra20_slink.c | 4 ++++ >> drivers/spi/tegra210_qspi.c | 3 +++ >> 3 files changed, 11 insertions(+) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > You might consider storing the mode and not acting on it until later. > But this is fine. I did that in previous patch (I suppose you could call it V1, but it was so different in implementation that I didn't), but Jagan asserted that claim_bus() isn't the correct place to process the mode value, and that's the place any deferred value would need to be applied.
On Fri, Aug 19, 2016 at 10:28 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/19/2016 10:56 AM, Jagan Teki wrote: >> >> On 18 August 2016 at 22:23, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> In tegra20_slink.c, the set_mode() function may be executed before the >>> SPI bus is claimed the first time, and hence the clocks to the SPI >>> controller may not be running. If so, any register read/write at this >>> time will hang the CPU. Fix this by ensuring the clock is running as soon >>> as the driver is probed. This is observed on the Tegra30 Beaver board. >>> >>> Apply the same clock initialization fix to all other Tegra SPI drivers so >>> that if set_mode() is ever implemented there, the same bug will not >>> appear. >>> Note that tegra114_spi.c already operates in this fashion. >>> >>> The clock manipulation code is copied from claim_bus() to probe() rather >>> than moved. This ensures that any calls to set_speed() take effect; the >>> clock can't be set once during probe and left unchanged. >> >> >> Do we need to set the clock for claim_bus() as well? I think it's >> better to move this on to set_speed so-that any call to set_speed() >> will update the same. I don't think claim_bus would require this >> again. > > > I explained this in the commit message: The clock rate needs to be set in > claim_bus() so that the frequency requested by set_speed() is in force. Usually I would rather skip the claim_bus for setting freq, if any call to set_speed I took the speed and evaluate the desired/correct freq and set the same in set_speed it self. This is because frequent calls to calm_bus does the same frequency init for respective transactions.
diff --git a/drivers/spi/tegra20_sflash.c b/drivers/spi/tegra20_sflash.c index 6888a96139a7..ce3a2d398cfb 100644 --- a/drivers/spi/tegra20_sflash.c +++ b/drivers/spi/tegra20_sflash.c @@ -122,6 +122,10 @@ static int tegra20_sflash_probe(struct udevice *bus) priv->freq = plat->frequency; priv->periph_id = plat->periph_id; + /* Change SPI clock to correct frequency, PLLP_OUT0 source */ + clock_start_periph_pll(priv->periph_id, CLOCK_ID_PERIPH, + priv->freq); + return 0; } diff --git a/drivers/spi/tegra20_slink.c b/drivers/spi/tegra20_slink.c index 238edec23ba5..e1da23b7b44b 100644 --- a/drivers/spi/tegra20_slink.c +++ b/drivers/spi/tegra20_slink.c @@ -128,6 +128,10 @@ static int tegra30_spi_probe(struct udevice *bus) priv->freq = plat->frequency; priv->periph_id = plat->periph_id; + /* Change SPI clock to correct frequency, PLLP_OUT0 source */ + clock_start_periph_pll(priv->periph_id, CLOCK_ID_PERIPH, + priv->freq); + return 0; } diff --git a/drivers/spi/tegra210_qspi.c b/drivers/spi/tegra210_qspi.c index 6bbbe9383954..026cff0c152e 100644 --- a/drivers/spi/tegra210_qspi.c +++ b/drivers/spi/tegra210_qspi.c @@ -131,6 +131,9 @@ static int tegra210_qspi_probe(struct udevice *bus) priv->freq = plat->frequency; priv->periph_id = plat->periph_id; + /* Change SPI clock to correct frequency, PLLP_OUT0 source */ + clock_start_periph_pll(priv->periph_id, CLOCK_ID_PERIPH, priv->freq); + return 0; }