* [U-Boot] [PATCH] spi: tegra20: fix mode selection logic
@ 2016-08-12 21:06 Stephen Warren
2016-08-13 8:51 ` Mirza Krak
2016-08-13 15:56 ` Jagan Teki
0 siblings, 2 replies; 6+ messages in thread
From: Stephen Warren @ 2016-08-12 21:06 UTC (permalink / raw)
To: u-boot
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);
--
2.9.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] spi: tegra20: fix mode selection logic
2016-08-12 21:06 [U-Boot] [PATCH] spi: tegra20: fix mode selection logic Stephen Warren
@ 2016-08-13 8:51 ` Mirza Krak
2016-08-13 9:09 ` Mirza Krak
2016-08-13 15:56 ` Jagan Teki
1 sibling, 1 reply; 6+ messages in thread
From: Mirza Krak @ 2016-08-13 8:51 UTC (permalink / raw)
To: u-boot
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.
--
Med V?nliga H?lsningar / Best Regards
*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak at hostmobility.com
Anders Personsgatan 12, 416 64 G?teborg
Sweden
http://www.hostmobility.com
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] spi: tegra20: fix mode selection logic
2016-08-13 8:51 ` Mirza Krak
@ 2016-08-13 9:09 ` Mirza Krak
0 siblings, 0 replies; 6+ messages in thread
From: Mirza Krak @ 2016-08-13 9:09 UTC (permalink / raw)
To: u-boot
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 :(.
--
Med V?nliga H?lsningar / Best Regards
*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak at hostmobility.com
Anders Personsgatan 12, 416 64 G?teborg
Sweden
http://www.hostmobility.com
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] spi: tegra20: fix mode selection logic
2016-08-12 21:06 [U-Boot] [PATCH] spi: tegra20: fix mode selection logic Stephen Warren
2016-08-13 8:51 ` Mirza Krak
@ 2016-08-13 15:56 ` Jagan Teki
2016-08-15 15:35 ` Stephen Warren
1 sibling, 1 reply; 6+ messages in thread
From: Jagan Teki @ 2016-08-13 15:56 UTC (permalink / raw)
To: u-boot
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.
--
Jagan.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] spi: tegra20: fix mode selection logic
2016-08-13 15:56 ` Jagan Teki
@ 2016-08-15 15:35 ` Stephen Warren
2016-08-18 16:54 ` Stephen Warren
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2016-08-15 15:35 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] spi: tegra20: fix mode selection logic
2016-08-15 15:35 ` Stephen Warren
@ 2016-08-18 16:54 ` Stephen Warren
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Warren @ 2016-08-18 16:54 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-18 16:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-12 21:06 [U-Boot] [PATCH] spi: tegra20: fix mode selection logic Stephen Warren
2016-08-13 8:51 ` Mirza Krak
2016-08-13 9:09 ` Mirza Krak
2016-08-13 15:56 ` Jagan Teki
2016-08-15 15:35 ` Stephen Warren
2016-08-18 16:54 ` Stephen 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.