[PATCH 0/2] spi: fix two bugs

[PATCH 0/2] spi: fix two bugs

[Addy Ke]

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

[PATCH 1/2] spi/rockchip: fix bug that case spi can't go as fast as slave request

[Addy Ke]

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 <addy.ke@rock-chips.com>
---
 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

Re: [PATCH 1/2] spi/rockchip: fix bug that case spi can't go as fast as slave request

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] spi/rockchip: fix bug that case spi can't go as fast as slave request

[addy ke]

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.

Re: [PATCH 1/2] spi/rockchip: fix bug that case spi can't go as fast as slave request

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] spi/rockchip: fix bug that case spi can't go as fast as slave request

[addy ke]

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

Re: [PATCH 1/2] spi/rockchip: fix bug that case spi can't go as fast as slave request

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/2] spi/rockchip: fix bug that cause spi transfer timed out in DMA duplex mode

[Addy Ke]

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 <addy.ke@rock-chips.com>
---
 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

Re: [PATCH 2/2] spi/rockchip: fix bug that cause spi transfer timed out in DMA duplex mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] spi/rockchip: fix bug that cause spi transfer timed out in DMA duplex mode

[addy ke]

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.
> 

Re: [PATCH 2/2] spi/rockchip: fix bug that cause spi transfer timed out in DMA duplex mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]