Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1407775 > unrolled thread

Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Julian Calaby <julian.calaby@gmail.com> - 2016-05-27 04:10 +0200
    Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Michal Suchanek <hramrach@gmail.com> - 2016-05-27 07:10 +0200
      Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Julian Calaby <julian.calaby@gmail.com> - 2016-05-27 07:20 +0200
        Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Mark Brown <broonie@kernel.org> - 2016-05-30 13:30 +0200
          Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Michal Suchanek <hramrach@gmail.com> - 2016-05-31 14:00 +0200
          Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-06-01 20:30 +0200

#1407775 — Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout

Hi Michal,

On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach@gmail.com> wrote:
> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
> actual time the transfer is supposed to take and multiply by 2 for good
> measure.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>
> v2:
> - fix build error
> - use unsigned instead of int in max_t
> - use tfr->speed_hz instead of sspi->max_speed_hz
> ---
>  drivers/spi/spi-sun4i.c | 11 ++++++++++-
>  drivers/spi/spi-sun6i.c | 11 ++++++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 1ddd9e2..fe63bbd 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  {
>         struct sun4i_spi *sspi = spi_master_get_devdata(master);
>         unsigned int mclk_rate, div, timeout;
> +       unsigned int start, end, tx_time;
>         unsigned int tx_len = 0;
>         int ret = 0;
>         u32 reg;
> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>         reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>         sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>
> +       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),

You should probably use "unsigned int" instead of just "unsigned" here.

> +                       100);
> +       start = jiffies;
>         timeout = wait_for_completion_timeout(&sspi->done,
> -                                             msecs_to_jiffies(1000));
> +                                             msecs_to_jiffies(tx_time));
> +       end = jiffies;
>         if (!timeout) {
> +               dev_warn(&master->dev,
> +                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
> +                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
> +                        jiffies_to_msecs(end - start), tx_time);

Should the debug changes be in a separate patch?

>                 ret = -ETIMEDOUT;
>                 goto out;
>         }
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 42e2c4b..8be5c5c 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>         unsigned int mclk_rate, div, timeout;
>         unsigned int tx_len = 0;
>         int ret = 0;
> +       unsigned int start, end, tx_time;
>         u32 reg;
>
>         /* We don't support transfer larger than the FIFO */
> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>         reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>         sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>
> +       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),

Ditto, "unsigned int" instead of "unsigned"?

> +                       100);
> +       start = jiffies;
>         timeout = wait_for_completion_timeout(&sspi->done,
> -                                             msecs_to_jiffies(1000));
> +                                             msecs_to_jiffies(tx_time));
> +       end = jiffies;
>         if (!timeout) {
> +               dev_warn(&master->dev,
> +                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
> +                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
> +                        jiffies_to_msecs(end - start), tx_time);

Ditto, separate patch?

Also, should the changes for the drivers be in two separate patches also?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[toc] | [next] | [standalone]

#1407814

On 27 May 2016 at 04:05, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Michal,
>
> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>> actual time the transfer is supposed to take and multiply by 2 for good
>> measure.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>> ---
>>
>> v2:
>> - fix build error
>> - use unsigned instead of int in max_t
>> - use tfr->speed_hz instead of sspi->max_speed_hz
>> ---
>>  drivers/spi/spi-sun4i.c | 11 ++++++++++-
>>  drivers/spi/spi-sun6i.c | 11 ++++++++++-
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>> index 1ddd9e2..fe63bbd 100644
>> --- a/drivers/spi/spi-sun4i.c
>> +++ b/drivers/spi/spi-sun4i.c
>> @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>>  {
>>         struct sun4i_spi *sspi = spi_master_get_devdata(master);
>>         unsigned int mclk_rate, div, timeout;
>> +       unsigned int start, end, tx_time;
>>         unsigned int tx_len = 0;
>>         int ret = 0;
>>         u32 reg;
>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>>         reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>>         sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>
>> +       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> You should probably use "unsigned int" instead of just "unsigned" here.
>
>> +                       100);

Or just 100U constant and avoid max_t altogether.

>> +       start = jiffies;
>>         timeout = wait_for_completion_timeout(&sspi->done,
>> -                                             msecs_to_jiffies(1000));
>> +                                             msecs_to_jiffies(tx_time));
>> +       end = jiffies;
>>         if (!timeout) {
>> +               dev_warn(&master->dev,
>> +                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>> +                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>> +                        jiffies_to_msecs(end - start), tx_time);
>
> Should the debug changes be in a separate patch?

Is this so big of a change that it needs to be split?

>
>>                 ret = -ETIMEDOUT;
>>                 goto out;
>>         }
>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>> index 42e2c4b..8be5c5c 100644
>> --- a/drivers/spi/spi-sun6i.c
>> +++ b/drivers/spi/spi-sun6i.c
>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>         unsigned int mclk_rate, div, timeout;
>>         unsigned int tx_len = 0;
>>         int ret = 0;
>> +       unsigned int start, end, tx_time;
>>         u32 reg;
>>
>>         /* We don't support transfer larger than the FIFO */
>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>         reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>         sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>
>> +       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> Ditto, "unsigned int" instead of "unsigned"?
>
>> +                       100);
>> +       start = jiffies;
>>         timeout = wait_for_completion_timeout(&sspi->done,
>> -                                             msecs_to_jiffies(1000));
>> +                                             msecs_to_jiffies(tx_time));
>> +       end = jiffies;
>>         if (!timeout) {
>> +               dev_warn(&master->dev,
>> +                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>> +                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>> +                        jiffies_to_msecs(end - start), tx_time);
>
> Ditto, separate patch?
>
> Also, should the changes for the drivers be in two separate patches also?

That's basically the same driver with different constants so I guess not.

Thanks

Michal

[toc] | [prev] | [next] | [standalone]

#1407816

Hi Michal,

On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> On 27 May 2016 at 04:05, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Michal,
>>
>> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>>> actual time the transfer is supposed to take and multiply by 2 for good
>>> measure.
>>>
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>> ---
>>>
>>> v2:
>>> - fix build error
>>> - use unsigned instead of int in max_t
>>> - use tfr->speed_hz instead of sspi->max_speed_hz
>>> ---
>>>  drivers/spi/spi-sun4i.c | 11 ++++++++++-
>>>  drivers/spi/spi-sun6i.c | 11 ++++++++++-
>>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>> index 1ddd9e2..fe63bbd 100644
>>> --- a/drivers/spi/spi-sun4i.c
>>> +++ b/drivers/spi/spi-sun4i.c
>>> @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>>>  {
>>>         struct sun4i_spi *sspi = spi_master_get_devdata(master);
>>>         unsigned int mclk_rate, div, timeout;
>>> +       unsigned int start, end, tx_time;
>>>         unsigned int tx_len = 0;
>>>         int ret = 0;
>>>         u32 reg;
>>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>>>         reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>>>         sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>>
>>> +       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>>
>> You should probably use "unsigned int" instead of just "unsigned" here.
>>
>>> +                       100);
>
> Or just 100U constant and avoid max_t altogether.
>
>>> +       start = jiffies;
>>>         timeout = wait_for_completion_timeout(&sspi->done,
>>> -                                             msecs_to_jiffies(1000));
>>> +                                             msecs_to_jiffies(tx_time));
>>> +       end = jiffies;
>>>         if (!timeout) {
>>> +               dev_warn(&master->dev,
>>> +                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>>> +                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>>> +                        jiffies_to_msecs(end - start), tx_time);
>>
>> Should the debug changes be in a separate patch?
>
> Is this so big of a change that it needs to be split?

Some people get touchy about changes not being mentioned in the commit
log. Technically this is two changes: fixing the timeout and adding
the timeout debugging, however they're related, so maybe just mention
that you've added this debugging in the commit log.

>
>>
>>>                 ret = -ETIMEDOUT;
>>>                 goto out;
>>>         }
>>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>>> index 42e2c4b..8be5c5c 100644
>>> --- a/drivers/spi/spi-sun6i.c
>>> +++ b/drivers/spi/spi-sun6i.c
>>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>>         unsigned int mclk_rate, div, timeout;
>>>         unsigned int tx_len = 0;
>>>         int ret = 0;
>>> +       unsigned int start, end, tx_time;
>>>         u32 reg;
>>>
>>>         /* We don't support transfer larger than the FIFO */
>>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>>         reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>>         sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>>
>>> +       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>>
>> Ditto, "unsigned int" instead of "unsigned"?
>>
>>> +                       100);
>>> +       start = jiffies;
>>>         timeout = wait_for_completion_timeout(&sspi->done,
>>> -                                             msecs_to_jiffies(1000));
>>> +                                             msecs_to_jiffies(tx_time));
>>> +       end = jiffies;
>>>         if (!timeout) {
>>> +               dev_warn(&master->dev,
>>> +                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>>> +                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>>> +                        jiffies_to_msecs(end - start), tx_time);
>>
>> Ditto, separate patch?
>>
>> Also, should the changes for the drivers be in two separate patches also?
>
> That's basically the same driver with different constants so I guess not.

Fair enough, I withdraw my comment then.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[toc] | [prev] | [next] | [standalone]

#1408993

[Multipart message — attachments visible in raw view] — view raw

On Fri, May 27, 2016 at 03:10:11PM +1000, Julian Calaby wrote:
> On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek <hramrach@gmail.com> wrote:

> >> Also, should the changes for the drivers be in two separate patches also?

> > That's basically the same driver with different constants so I guess not.

> Fair enough, I withdraw my comment then.

If it's the same driver with different constants it should really
actually be the same driver - I did ask this when the drivers were
originally merged...

[toc] | [prev] | [next] | [standalone]

#1410206

Hello

On 30 May 2016 at 13:23, Mark Brown <broonie@kernel.org> wrote:
> On Fri, May 27, 2016 at 03:10:11PM +1000, Julian Calaby wrote:
>> On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek <hramrach@gmail.com> wrote:
>
>> >> Also, should the changes for the drivers be in two separate patches also?
>
>> > That's basically the same driver with different constants so I guess not.
>
>> Fair enough, I withdraw my comment then.
>
> If it's the same driver with different constants it should really
> actually be the same driver - I did ask this when the drivers were
> originally merged...

There are some slight differences and the constants really are
different for each driver.

You would need a register remap adaptation layer and a few qirks for
each revision of the driver.

It's certainly possible to merge them but I am not sure if that gives
easier to maintain code.

On the other hand, comparing the drivers there is different comment
anout clock calculation arithmetic and the code is the same :S

Thanks

Michal

--- spi-sun4i.c    2016-05-31 13:19:27.076510421 +0200
+++ spi-sun6i.c    2016-05-31 13:26:58.123580382 +0200
@@ -19,65 +19,72 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>

 #include <linux/spi/spi.h>

-#define SUNXI_FIFO_DEPTH            64
+#define SUNXI_FIFO_DEPTH            128

-#define SUNXI_RXDATA_REG            0x00
+#define SUNXI_TXDATA_REG            0x200

-#define SUNXI_TXDATA_REG            0x04
+#define SUNXI_RXDATA_REG            0x300

 #define SUNXI_TFR_CTL_REG            0x08
-#define SUNXI_TFR_CTL_ENABLE            BIT(0)
-#define SUNXI_TFR_CTL_MASTER            BIT(1)
-#define SUNXI_TFR_CTL_CPHA            BIT(2)
-#define SUNXI_TFR_CTL_CPOL            BIT(3)
-#define SUNXI_TFR_CTL_CS_ACTIVE_LOW        BIT(4)
-#define SUNXI_TFR_CTL_LMTF            BIT(6)
-#define SUNXI_TFR_CTL_TF_RST            BIT(8)
-#define SUNXI_TFR_CTL_RF_RST            BIT(9)
-#define SUNXI_TFR_CTL_XCH            BIT(10)
-#define SUNXI_TFR_CTL_CS_MASK            0x3000
-#define SUNXI_TFR_CTL_CS(cs)            (((cs) << 12) & SUNXI_TFR_CTL_CS_MASK)
-#define SUNXI_TFR_CTL_DHB            BIT(15)
-#define SUNXI_TFR_CTL_CS_MANUAL            BIT(16)
-#define SUNXI_TFR_CTL_CS_LEVEL            BIT(17)
-#define SUNXI_TFR_CTL_TP            BIT(18)
+#define SUNXI_TFR_CTL_CPHA            BIT(0)
+#define SUNXI_TFR_CTL_CPOL            BIT(1)
+#define SUNXI_TFR_CTL_SPOL            BIT(2)
+#define SUNXI_TFR_CTL_CS_MASK            0x30
+#define SUNXI_TFR_CTL_CS(cs)            (((cs) << 4) & SUNXI_TFR_CTL_CS_MASK)
+#define SUNXI_TFR_CTL_CS_MANUAL            BIT(6)
+#define SUNXI_TFR_CTL_CS_LEVEL            BIT(7)
+#define SUNXI_TFR_CTL_DHB            BIT(8)
+#define SUNXI_TFR_CTL_FBS            BIT(12)
+#define SUNXI_TFR_CTL_XCH            BIT(31)

-#define SUNXI_INT_CTL_REG            0x0c
-#define SUNXI_INT_CTL_TC            BIT(16)
+#define SUNXI_INT_CTL_REG            0x10
+#define SUNXI_INT_CTL_RF_OVF            BIT(8)
+#define SUNXI_INT_CTL_TC            BIT(12)

-#define SUNXI_INT_STA_REG            0x10
+#define SUNXI_INT_STA_REG            0x14

-#define SUNXI_DMA_CTL_REG            0x14
+#define SUNXI_FIFO_CTL_REG            0x18
+#define SUNXI_FIFO_CTL_RF_RST            BIT(15)
+#define SUNXI_FIFO_CTL_TF_RST            BIT(31)

-#define SUNXI_CLK_CTL_REG            0x1c
+#define SUNXI_CLK_CTL_REG            0x24
 #define SUNXI_CLK_CTL_CDR2_MASK            0xff
 #define SUNXI_CLK_CTL_CDR2(div)            (((div) &
SUNXI_CLK_CTL_CDR2_MASK) << 0)
 #define SUNXI_CLK_CTL_CDR1_MASK            0xf
 #define SUNXI_CLK_CTL_CDR1(div)            (((div) &
SUNXI_CLK_CTL_CDR1_MASK) << 8)
 #define SUNXI_CLK_CTL_DRS            BIT(12)

-#define SUNXI_BURST_CNT_REG            0x20
+#define SUNXI_BURST_CNT_REG            0x30
 #define SUNXI_BURST_CNT(cnt)            ((cnt) & 0xffffff)

-#define SUNXI_XMIT_CNT_REG            0x24
+#define SUNXI_XMIT_CNT_REG            0x34
 #define SUNXI_XMIT_CNT(cnt)            ((cnt) & 0xffffff)

-#define SUNXI_FIFO_STA_REG            0x28
+#define SUNXI_FIFO_STA_REG            0x1c
 #define SUNXI_FIFO_STA_RF_CNT_MASK        0x7f
 #define SUNXI_FIFO_STA_RF_CNT_BITS        0
 #define SUNXI_FIFO_STA_TF_CNT_MASK        0x7f
 #define SUNXI_FIFO_STA_TF_CNT_BITS        16

-#define SUNXI_WAIT_REG                0x18
+#define SUNXI_BURST_CTL_CNT_REG            0x38
+#define SUNXI_BURST_CTL_CNT_STC(cnt)        ((cnt) & 0xffffff)
+
+#define SUNXI_GBL_CTL_REG            0x04
+#define SUNXI_GBL_CTL_BUS_ENABLE        BIT(0)
+#define SUNXI_GBL_CTL_MASTER            BIT(1)
+#define SUNXI_GBL_CTL_TP            BIT(7)
+#define SUNXI_GBL_CTL_RST            BIT(31)

 struct sunxi_spi {
     struct spi_master    *master;
     void __iomem        *base_addr;
     struct clk        *hclk;
     struct clk        *mclk;
+    struct reset_control    *rstc;

     struct completion    done;

@@ -136,37 +143,18 @@
     u32 reg;

     reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
-
     reg &= ~SUNXI_TFR_CTL_CS_MASK;
     reg |= SUNXI_TFR_CTL_CS(spi->chip_select);

-    /* We want to control the chip select manually */
-    reg |= SUNXI_TFR_CTL_CS_MANUAL;
-
     if (enable)
         reg |= SUNXI_TFR_CTL_CS_LEVEL;
     else
         reg &= ~SUNXI_TFR_CTL_CS_LEVEL;

-    /*
-     * Even though this looks irrelevant since we are supposed to
-     * be controlling the chip select manually, this bit also
-     * controls the levels of the chip select for inactive
-     * devices.
-     *
-     * If we don't set it, the chip select level will go low by
-     * default when the device is idle, which is not really
-     * expected in the common case where the chip select is active
-     * low.
-     */
-    if (spi->mode & SPI_CS_HIGH)
-        reg &= ~SUNXI_TFR_CTL_CS_ACTIVE_LOW;
-    else
-        reg |= SUNXI_TFR_CTL_CS_ACTIVE_LOW;
-
     sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg);
 }

+
 static int sunxi_spi_transfer_one(struct spi_master *master,
                   struct spi_device *spi,
                   struct spi_transfer *tfr)
@@ -189,17 +177,16 @@
     /* Clear pending interrupts */
     sunxi_spi_write(sspi, SUNXI_INT_STA_REG, ~0);

-
-    reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
-
     /* Reset FIFOs */
-    sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
-            reg | SUNXI_TFR_CTL_RF_RST | SUNXI_TFR_CTL_TF_RST);
+    sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
+            SUNXI_FIFO_CTL_RF_RST | SUNXI_FIFO_CTL_TF_RST);

     /*
      * Setup the transfer control register: Chip Select,
      * polarities, etc.
      */
+    reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
+
     if (spi->mode & SPI_CPOL)
         reg |= SUNXI_TFR_CTL_CPOL;
     else
@@ -211,10 +198,9 @@
         reg &= ~SUNXI_TFR_CTL_CPHA;

     if (spi->mode & SPI_LSB_FIRST)
-        reg |= SUNXI_TFR_CTL_LMTF;
+        reg |= SUNXI_TFR_CTL_FBS;
     else
-        reg &= ~SUNXI_TFR_CTL_LMTF;
-
+        reg &= ~SUNXI_TFR_CTL_FBS;

     /*
      * If it's a TX only transfer, we don't want to fill the RX
@@ -225,6 +211,9 @@
     else
         reg |= SUNXI_TFR_CTL_DHB;

+    /* We want to control the chip select manually */
+    reg |= SUNXI_TFR_CTL_CS_MANUAL;
+
     sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg);

     /* Ensure that we have a parent clock fast enough */
@@ -239,7 +228,7 @@
      *
      * We have two choices there. Either we can use the clock
      * divide rate 1, which is calculated thanks to this formula:
-     * SPI_CLK = MOD_CLK / (2 ^ (cdr + 1))
+     * SPI_CLK = MOD_CLK / (2 ^ cdr)
      * Or we can use CDR2, which is calculated with the formula:
      * SPI_CLK = MOD_CLK / (2 * (cdr + 1))
      * Wether we use the former or the latter is set through the
@@ -268,6 +257,8 @@
     /* Setup the counters */
     sunxi_spi_write(sspi, SUNXI_BURST_CNT_REG, SUNXI_BURST_CNT(tfr->len));
     sunxi_spi_write(sspi, SUNXI_XMIT_CNT_REG, SUNXI_XMIT_CNT(tx_len));
+    sunxi_spi_write(sspi, SUNXI_BURST_CTL_CNT_REG,
+            SUNXI_BURST_CTL_CNT_STC(tx_len));

     /* Fill the TX FIFO */
     sunxi_spi_fill_fifo(sspi, SUNXI_FIFO_DEPTH);
@@ -327,11 +318,19 @@
         goto err;
     }

-    sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
-            SUNXI_TFR_CTL_ENABLE | SUNXI_TFR_CTL_MASTER | SUNXI_TFR_CTL_TP);
+    ret = reset_control_deassert(sspi->rstc);
+    if (ret) {
+        dev_err(dev, "Couldn't deassert the device from reset\n");
+        goto err2;
+    }
+
+    sunxi_spi_write(sspi, SUNXI_GBL_CTL_REG,
+            SUNXI_GBL_CTL_BUS_ENABLE | SUNXI_GBL_CTL_MASTER |
SUNXI_GBL_CTL_TP);

     return 0;

+err2:
+    clk_disable_unprepare(sspi->mclk);
 err:
     clk_disable_unprepare(sspi->hclk);
 out:
@@ -343,6 +342,7 @@
     struct spi_master *master = dev_get_drvdata(dev);
     struct sunxi_spi *sspi = spi_master_get_devdata(master);

+    reset_control_assert(sspi->rstc);
     clk_disable_unprepare(sspi->mclk);
     clk_disable_unprepare(sspi->hclk);

@@ -411,6 +411,13 @@

     init_completion(&sspi->done);

+    sspi->rstc = devm_reset_control_get(&pdev->dev, NULL);
+    if (IS_ERR(sspi->rstc)) {
+        dev_err(&pdev->dev, "Couldn't get reset controller\n");
+        ret = PTR_ERR(sspi->rstc);
+        goto err_free_master;
+    }
+
     /*
      * This wake-up/shutdown pattern is to be able to have the
      * device woken up, even if runtime_pm is disabled
@@ -449,7 +456,7 @@
 }

 static const struct of_device_id sunxi_spi_match[] = {
-    { .compatible = "allwinner,sun4i-a10-spi", },
+    { .compatible = "allwinner,sun6i-a31-spi", },
     {}
 };
 MODULE_DEVICE_TABLE(of, sunxi_spi_match);
@@ -472,5 +479,5 @@

 MODULE_AUTHOR("Pan Nan <pannan@allwinnertech.com>");
 MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
-MODULE_DESCRIPTION("Allwinner A1X/A20 SPI controller driver");
+MODULE_DESCRIPTION("Allwinner A31 SPI controller driver");
 MODULE_LICENSE("GPL");

[toc] | [prev] | [next] | [standalone]

#1411458

[Multipart message — attachments visible in raw view] — view raw

Hi Mark,

On Mon, May 30, 2016 at 12:23:50PM +0100, Mark Brown wrote:
> On Fri, May 27, 2016 at 03:10:11PM +1000, Julian Calaby wrote:
> > On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> 
> > >> Also, should the changes for the drivers be in two separate patches also?
> 
> > > That's basically the same driver with different constants so I guess not.
> 
> > Fair enough, I withdraw my comment then.
> 
> If it's the same driver with different constants it should really
> actually be the same driver - I did ask this when the drivers were
> originally merged...

I think we already had this discussion a few times :)

The thing is that the SPI framework now deals pretty well with the SPI
controllers, and you basically only have the probe function and how to
start a transfer. Which is nice.

However, the sun4i and sun6i SPI controllers have very significant
register layout differences: the registers offset are different, some
registers in the sun4i have been split in several in the sun6i. So
while I concur that they look alike, merging the two in a single
driver would complicate a lot the code, while we would not be able to
share most of the code, so I am really not sure it's worth it.

Where the issue really lies is that they've been both written at the
same time, and share the same flaws, especially in their probe
method. But that's not really related to the controller itself, but
more because the code has been close to copy/pasted.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web