All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] spi: tegra: fix hand in set_mode()
@ 2016-08-18 16:53 Stephen Warren
  2016-08-19 16:56 ` Jagan Teki
  2016-08-20 23:52 ` Simon Glass
  0 siblings, 2 replies; 8+ messages in thread
From: Stephen Warren @ 2016-08-18 16:53 UTC (permalink / raw)
  To: u-boot

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(+)

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;
 }
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] spi: tegra: fix hand in set_mode()
  2016-08-18 16:53 [U-Boot] [PATCH] spi: tegra: fix hand in set_mode() Stephen Warren
@ 2016-08-19 16:56 ` Jagan Teki
  2016-08-19 16:58   ` Stephen Warren
  2016-08-20 23:52 ` Simon Glass
  1 sibling, 1 reply; 8+ messages in thread
From: Jagan Teki @ 2016-08-19 16:56 UTC (permalink / raw)
  To: u-boot

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!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] spi: tegra: fix hand in set_mode()
  2016-08-19 16:56 ` Jagan Teki
@ 2016-08-19 16:58   ` Stephen Warren
  2016-08-29 13:08     ` Jagan Teki
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Warren @ 2016-08-19 16:58 UTC (permalink / raw)
  To: u-boot

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] spi: tegra: fix hand in set_mode()
  2016-08-18 16:53 [U-Boot] [PATCH] spi: tegra: fix hand in set_mode() Stephen Warren
  2016-08-19 16:56 ` Jagan Teki
@ 2016-08-20 23:52 ` Simon Glass
  2016-08-21  2:17   ` Stephen Warren
  1 sibling, 1 reply; 8+ messages in thread
From: Simon Glass @ 2016-08-20 23:52 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] spi: tegra: fix hand in set_mode()
  2016-08-20 23:52 ` Simon Glass
@ 2016-08-21  2:17   ` Stephen Warren
  2016-08-25 20:51     ` Tom Warren
  2016-08-25 23:42     ` Tom Warren
  0 siblings, 2 replies; 8+ messages in thread
From: Stephen Warren @ 2016-08-21  2:17 UTC (permalink / raw)
  To: u-boot

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] spi: tegra: fix hand in set_mode()
  2016-08-21  2:17   ` Stephen Warren
@ 2016-08-25 20:51     ` Tom Warren
  2016-08-25 23:42     ` Tom Warren
  1 sibling, 0 replies; 8+ messages in thread
From: Tom Warren @ 2016-08-25 20:51 UTC (permalink / raw)
  To: u-boot

Jagan,

Are you planning on taking this in via the SPI repo, or can I take it in to Tegra?

Tom

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: Saturday, August 20, 2016 7:17 PM
> To: Simon Glass <sjg@chromium.org>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Jagan Teki
> <jteki@openedev.com>; Tom Warren <TWarren@nvidia.com>; Stephen
> Warren <swarren@nvidia.com>; Mirza Krak <mirza.krak@hostmobility.com>
> Subject: Re: [PATCH] spi: tegra: fix hand in set_mode()
> 
> 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.
--
nvpublic

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] spi: tegra: fix hand in set_mode()
  2016-08-21  2:17   ` Stephen Warren
  2016-08-25 20:51     ` Tom Warren
@ 2016-08-25 23:42     ` Tom Warren
  1 sibling, 0 replies; 8+ messages in thread
From: Tom Warren @ 2016-08-25 23:42 UTC (permalink / raw)
  To: u-boot

Jagan,

I've got to get a PR into TomR, so I'm including the Tegra SPI fix from Stephen.  Hope this doesn't cause you any problems.

Tom
> -----Original Message-----
> From: Tom Warren
> Sent: Thursday, August 25, 2016 1:52 PM
> To: Jagan Teki (jteki at openedev.com) <jteki@openedev.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren
> <swarren@nvidia.com>; Mirza Krak <mirza.krak@hostmobility.com>; Stephen
> Warren <swarren@wwwdotorg.org>; 'Simon Glass' <sjg@chromium.org>
> Subject: RE: [PATCH] spi: tegra: fix hand in set_mode()
> 
> Jagan,
> 
> Are you planning on taking this in via the SPI repo, or can I take it in to Tegra?
> 
> Tom
> 
> > -----Original Message-----
> > From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> > Sent: Saturday, August 20, 2016 7:17 PM
> > To: Simon Glass <sjg@chromium.org>
> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Jagan Teki
> > <jteki@openedev.com>; Tom Warren <TWarren@nvidia.com>; Stephen
> Warren
> > <swarren@nvidia.com>; Mirza Krak <mirza.krak@hostmobility.com>
> > Subject: Re: [PATCH] spi: tegra: fix hand in set_mode()
> >
> > 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.
--
nvpublic

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] spi: tegra: fix hand in set_mode()
  2016-08-19 16:58   ` Stephen Warren
@ 2016-08-29 13:08     ` Jagan Teki
  0 siblings, 0 replies; 8+ messages in thread
From: Jagan Teki @ 2016-08-29 13:08 UTC (permalink / raw)
  To: u-boot

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.

-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-08-29 13:08 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-18 16:53 [U-Boot] [PATCH] spi: tegra: fix hand in set_mode() Stephen Warren
2016-08-19 16:56 ` Jagan Teki
2016-08-19 16:58   ` Stephen Warren
2016-08-29 13:08     ` Jagan Teki
2016-08-20 23:52 ` Simon Glass
2016-08-21  2:17   ` Stephen Warren
2016-08-25 20:51     ` Tom Warren
2016-08-25 23:42     ` Tom Warren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.