Patch 1: fix bug that case spi can't go as fast as slave request
Patch 2: fix bug that cause spi transfer timed out in DMA duplex mode
Tested on rk3288-pinky-version2 board.
Addy Ke (2):
spi/rockchip: fix bug that case spi can't go as fast as slave request
spi/rockchip: fix bug that cause spi transfer timed out in DMA duplex
mode
drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 11 deletions(-)
--
1.8.3.2
Because the minimum divisor in rk3x's spi controller is 2,
if spi_clk is less than 2 * sclk_out, we can't get the right divisor.
So we must set spi_clk again to match slave request.
Signed-off-by: Addy Ke <[email protected]>
---
drivers/spi/spi-rockchip.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index f96ea8a..3044c6c 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -145,6 +145,9 @@
#define RXBUSY (1 << 0)
#define TXBUSY (1 << 1)
+/* sclk_out: spi master internal logic in rk3x can support 50Mhz */
+#define MAX_SCLK_OUT 50000000
+
enum rockchip_ssi_type {
SSI_MOTO_SPI = 0,
SSI_TI_SSP,
@@ -496,6 +499,15 @@ static void rockchip_spi_config(struct rockchip_spi *rs)
dmacr |= RF_DMA_EN;
}
+ if (WARN_ON(rs->speed > MAX_SCLK_OUT))
+ rs->speed = MAX_SCLK_OUT;
+
+ /* the minimum divsor is 2 */
+ if (rs->max_freq < 2 * rs->speed) {
+ clk_set_rate(rs->spiclk, 2 * rs->speed);
+ rs->max_freq = clk_get_rate(rs->spiclk);
+ }
+
/* div doesn't support odd number */
div = max_t(u32, rs->max_freq / rs->speed, 1);
div = (div + 1) & 0xfffe;
--
1.8.3.2
In rx mode, dma must be prepared before spi is enabled.
But in tx and tr mode, spi must be enabled first.
Signed-off-by: Addy Ke <[email protected]>
---
drivers/spi/spi-rockchip.c | 34 +++++++++++++++++++++++-----------
1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 3044c6c..153269b 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -328,6 +328,8 @@ static int rockchip_spi_unprepare_message(struct spi_master *master,
spin_unlock_irqrestore(&rs->lock, flags);
+ spi_enable_chip(rs, 0);
+
return 0;
}
@@ -384,6 +386,8 @@ static int rockchip_spi_pio_transfer(struct rockchip_spi *rs)
if (rs->tx)
wait_for_idle(rs);
+ spi_enable_chip(rs, 0);
+
return 0;
}
@@ -395,8 +399,10 @@ static void rockchip_spi_dma_rxcb(void *data)
spin_lock_irqsave(&rs->lock, flags);
rs->state &= ~RXBUSY;
- if (!(rs->state & TXBUSY))
+ if (!(rs->state & TXBUSY)) {
+ spi_enable_chip(rs, 0);
spi_finalize_current_transfer(rs->master);
+ }
spin_unlock_irqrestore(&rs->lock, flags);
}
@@ -512,8 +518,6 @@ static void rockchip_spi_config(struct rockchip_spi *rs)
div = max_t(u32, rs->max_freq / rs->speed, 1);
div = (div + 1) & 0xfffe;
- spi_enable_chip(rs, 0);
-
writel_relaxed(cr0, rs->regs + ROCKCHIP_SPI_CTRLR0);
writel_relaxed(rs->len - 1, rs->regs + ROCKCHIP_SPI_CTRLR1);
@@ -527,8 +531,6 @@ static void rockchip_spi_config(struct rockchip_spi *rs)
spi_set_clk(rs, div);
dev_dbg(rs->dev, "cr0 0x%x, div %d\n", cr0, div);
-
- spi_enable_chip(rs, 1);
}
static int rockchip_spi_transfer_one(
@@ -536,7 +538,7 @@ static int rockchip_spi_transfer_one(
struct spi_device *spi,
struct spi_transfer *xfer)
{
- int ret = 0;
+ int ret = 1;
struct rockchip_spi *rs = spi_master_get_devdata(master);
WARN_ON(readl_relaxed(rs->regs + ROCKCHIP_SPI_SSIENR) &&
@@ -568,17 +570,27 @@ static int rockchip_spi_transfer_one(
rs->tmode = CR0_XFM_RO;
/* we need prepare dma before spi was enabled */
- if (master->can_dma && master->can_dma(master, spi, xfer)) {
+ if (master->can_dma && master->can_dma(master, spi, xfer))
rs->use_dma = 1;
- rockchip_spi_prepare_dma(rs);
- } else {
+ else
rs->use_dma = 0;
- }
rockchip_spi_config(rs);
- if (!rs->use_dma)
+ if (rs->use_dma) {
+ if (rs->tmode == CR0_XFM_RO) {
+ /* rx: dma must be prepared first */
+ rockchip_spi_prepare_dma(rs);
+ spi_enable_chip(rs, 1);
+ } else {
+ /* tx or tr: spi must be enabled first */
+ spi_enable_chip(rs, 1);
+ rockchip_spi_prepare_dma(rs);
+ }
+ } else {
+ spi_enable_chip(rs, 1);
ret = rockchip_spi_pio_transfer(rs);
+ }
return ret;
}
--
1.8.3.2
On Wed, Oct 15, 2014 at 07:25:49PM +0800, Addy Ke wrote:
> + if (WARN_ON(rs->speed > MAX_SCLK_OUT))
> + rs->speed = MAX_SCLK_OUT;
> +
> + /* the minimum divsor is 2 */
> + if (rs->max_freq < 2 * rs->speed) {
> + clk_set_rate(rs->spiclk, 2 * rs->speed);
> + rs->max_freq = clk_get_rate(rs->spiclk);
> + }
I'll apply this but you should be checking the return code from
clk_set_rate() here, please send a followup patch doing that. It might
also be worth consdering just setting the rate unconditionally here, it
seems like it should make things simpler.
On Wed, Oct 15, 2014 at 07:26:18PM +0800, Addy Ke wrote:
> In rx mode, dma must be prepared before spi is enabled.
> But in tx and tr mode, spi must be enabled first.
Applied, thanks.
hi, Mark
On 2014/10/15 21:04, Mark Brown wrote:
> On Wed, Oct 15, 2014 at 07:25:49PM +0800, Addy Ke wrote:
>
>> + if (WARN_ON(rs->speed > MAX_SCLK_OUT))
>> + rs->speed = MAX_SCLK_OUT;
>> +
>> + /* the minimum divsor is 2 */
>> + if (rs->max_freq < 2 * rs->speed) {
>> + clk_set_rate(rs->spiclk, 2 * rs->speed);
>> + rs->max_freq = clk_get_rate(rs->spiclk);
>> + }
>
> I'll apply this but you should be checking the return code from
> clk_set_rate() here, please send a followup patch doing that. It might
If clk_set_rate return error, do I only put dev_warn here or return error value to spi core?
> also be worth consdering just setting the rate unconditionally here, it
> seems like it should make things simpler.
>
I think we need.
If we set the rate unconditionally here, clk_set_rate() will be executed in each spi transfer.
Hi, Mark
The following changes is nessarry:(if tx complete, spi must be disabled too)
diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 153269b..87bc16f 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -418,8 +418,10 @@ static void rockchip_spi_dma_txcb(void *data)
spin_lock_irqsave(&rs->lock, flags);
rs->state &= ~TXBUSY;
- if (!(rs->state & RXBUSY))
+ if (!(rs->state & RXBUSY)) {
+ spi_enable_chip(rs, 0);
spi_finalize_current_transfer(rs->master);
+ }
spin_unlock_irqrestore(&rs->lock, flags);
}
Sorry for my mistake, I have not put these changes to this patch.
Do I need send patch v2 or a new patch to fix this issure?
On 2014/10/15 21:05, Mark Brown wrote:
> On Wed, Oct 15, 2014 at 07:26:18PM +0800, Addy Ke wrote:
>> In rx mode, dma must be prepared before spi is enabled.
>> But in tx and tr mode, spi must be enabled first.
>
> Applied, thanks.
>
On 2014/10/16 17:34, Mark Brown wrote:
> On Thu, Oct 16, 2014 at 05:16:02PM +0800, addy ke wrote:
>> On 2014/10/15 21:04, Mark Brown wrote:
>>> On Wed, Oct 15, 2014 at 07:25:49PM +0800, Addy Ke wrote:
>
>>>> + if (WARN_ON(rs->speed > MAX_SCLK_OUT))
>>>> + rs->speed = MAX_SCLK_OUT;
>
>>>> + /* the minimum divsor is 2 */
>>>> + if (rs->max_freq < 2 * rs->speed) {
>>>> + clk_set_rate(rs->spiclk, 2 * rs->speed);
>>>> + rs->max_freq = clk_get_rate(rs->spiclk);
>>>> + }
>
>>> I'll apply this but you should be checking the return code from
>>> clk_set_rate() here, please send a followup patch doing that. It might
>
>> If clk_set_rate return error, do I only put dev_warn here or return error value to spi core?
>
> It'd be better to return an error if we need to set the rate and can't
> do it.
>
>>> also be worth consdering just setting the rate unconditionally here, it
>>> seems like it should make things simpler.
>
>> I think we need.
>> If we set the rate unconditionally here, clk_set_rate() will be executed in each spi transfer.
>
> Is that really such a high cost?
>
Not high cost, but I think if the default spi_clk is enough, we do not need to set spi_clk again.
Maybe we can only set spi_clk as (2 * MAX_SCLK_OUT) in probe().
On Thu, Oct 16, 2014 at 05:16:02PM +0800, addy ke wrote:
> On 2014/10/15 21:04, Mark Brown wrote:
> > On Wed, Oct 15, 2014 at 07:25:49PM +0800, Addy Ke wrote:
> >> + if (WARN_ON(rs->speed > MAX_SCLK_OUT))
> >> + rs->speed = MAX_SCLK_OUT;
> >> + /* the minimum divsor is 2 */
> >> + if (rs->max_freq < 2 * rs->speed) {
> >> + clk_set_rate(rs->spiclk, 2 * rs->speed);
> >> + rs->max_freq = clk_get_rate(rs->spiclk);
> >> + }
> > I'll apply this but you should be checking the return code from
> > clk_set_rate() here, please send a followup patch doing that. It might
> If clk_set_rate return error, do I only put dev_warn here or return error value to spi core?
It'd be better to return an error if we need to set the rate and can't
do it.
> > also be worth consdering just setting the rate unconditionally here, it
> > seems like it should make things simpler.
> I think we need.
> If we set the rate unconditionally here, clk_set_rate() will be executed in each spi transfer.
Is that really such a high cost?
On Thu, Oct 16, 2014 at 05:26:02PM +0800, addy ke wrote:
> The following changes is nessarry:(if tx complete, spi must be disabled too)
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 153269b..87bc16f 100644
Please send this using the normal submission process.
On Thu, Oct 16, 2014 at 05:58:51PM +0800, addy ke wrote:
Please fix your mailer to word wrap within paragraphs.
> On 2014/10/16 17:34, Mark Brown wrote:
> >> If we set the rate unconditionally here, clk_set_rate() will be executed in each spi transfer.
> > Is that really such a high cost?
> Not high cost, but I think if the default spi_clk is enough, we do not need to set spi_clk again.
> Maybe we can only set spi_clk as (2 * MAX_SCLK_OUT) in probe().
It's not too bad to do it where it is, it just seems better to be
simpler.