diff mbox

[RFC/PATCHv2,3/3] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

Message ID 1425685594-26595-4-git-send-email-tthayer@opensource.altera.com
State New, archived
Headers show

Commit Message

tthayer@opensource.altera.com March 6, 2015, 11:46 p.m. UTC
From: Thor Thayer <tthayer@opensource.altera.com>

Altera's Arria10 SoC interconnect requires a 32 bit write for APB
peripherals. The current spi-dw driver uses 16bit accesses in
some locations. Use function pointers to support 32 bit accesses
but retain legacy 16 bit access.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
 drivers/spi/spi-dw-mmio.c |    7 ++++++-
 drivers/spi/spi-dw.c      |   29 +++++++++++++++++------------
 drivers/spi/spi-dw.h      |   12 ++++++++++++
 3 files changed, 35 insertions(+), 13 deletions(-)

Comments

Andy Shevchenko March 7, 2015, 7:52 p.m. UTC | #1
On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@opensource.altera.com> wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
> peripherals. The current spi-dw driver uses 16bit accesses in
> some locations. Use function pointers to support 32 bit accesses
> but retain legacy 16 bit access.
>

Thanks for this version. My comments below.

> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
>  drivers/spi/spi-dw-mmio.c |    7 ++++++-
>  drivers/spi/spi-dw.c      |   29 +++++++++++++++++------------
>  drivers/spi/spi-dw.h      |   12 ++++++++++++
>  3 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index eb03e12..c4fe9e9 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -76,8 +76,13 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
>
>         num_cs = 4;
>
> -       if (pdev->dev.of_node)
> +       if (pdev->dev.of_node) {
>                 of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs);
> +               if (of_property_read_bool(pdev->dev.of_node, "32bit_access")) {
> +                       dws->read_w = dw_readw32;
> +                       dws->write_w = dw_writew32;

Can we use just  readw/writew (w/o underscores) as names for the accessors?

> +               }
> +       }
>
>         dws->num_cs = num_cs;
>
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index c5fa2be..d008791 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -157,7 +157,7 @@ static inline u32 tx_max(struct dw_spi *dws)
>         u32 tx_left, tx_room, rxtx_gap;
>
>         tx_left = (dws->tx_end - dws->tx) / dws->n_bytes;
> -       tx_room = dws->fifo_len - dw_readw(dws, DW_SPI_TXFLR);
> +       tx_room = dws->fifo_len - dws->read_w(dws, DW_SPI_TXFLR);
>
>         /*
>          * Another concern is about the tx/rx mismatch, we
> @@ -178,7 +178,7 @@ static inline u32 rx_max(struct dw_spi *dws)
>  {
>         u32 rx_left = (dws->rx_end - dws->rx) / dws->n_bytes;
>
> -       return min_t(u32, rx_left, dw_readw(dws, DW_SPI_RXFLR));
> +       return min_t(u32, rx_left, dws->read_w(dws, DW_SPI_RXFLR));
>  }
>
>  static void dw_writer(struct dw_spi *dws)
> @@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)
>                         else
>                                 txw = *(u16 *)(dws->tx);
>                 }
> -               dw_writew(dws, DW_SPI_DR, txw);
> +               dws->write_w(dws, DW_SPI_DR, txw);
>                 dws->tx += dws->n_bytes;
>         }
>  }
> @@ -205,7 +205,7 @@ static void dw_reader(struct dw_spi *dws)
>         u16 rxw;
>
>         while (max--) {
> -               rxw = dw_readw(dws, DW_SPI_DR);
> +               rxw = dws->read_w(dws, DW_SPI_DR);
>                 /* Care rx only if the transfer's original "rx" is not null */
>                 if (dws->rx_end - dws->len) {
>                         if (dws->n_bytes == 1)
> @@ -254,11 +254,11 @@ static void int_error_stop(struct dw_spi *dws, const char *msg)
>
>  static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>  {
> -       u16 irq_status = dw_readw(dws, DW_SPI_ISR);
> +       u16 irq_status = dws->read_w(dws, DW_SPI_ISR);
>
>         /* Error handling */
>         if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
> -               dw_readw(dws, DW_SPI_ICR);
> +               dws->read_w(dws, DW_SPI_ICR);
>                 int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>                 return IRQ_HANDLED;
>         }
> @@ -283,7 +283,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
>  {
>         struct spi_master *master = dev_id;
>         struct dw_spi *dws = spi_master_get_devdata(master);
> -       u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
> +       u16 irq_status = dws->read_w(dws, DW_SPI_ISR) & 0x3f;
>
>         if (!irq_status)
>                 return IRQ_NONE;
> @@ -379,7 +379,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>                 cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
>         }
>
> -       dw_writew(dws, DW_SPI_CTRL0, cr0);
> +       dws->write_w(dws, DW_SPI_CTRL0, cr0);
>
>         /* Check if current transfer is a DMA transaction */
>         dws->dma_mapped = map_dma_buffers(master, spi, transfer);
> @@ -393,7 +393,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>          */
>         if (!dws->dma_mapped && !chip->poll_mode) {
>                 txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
> -               dw_writew(dws, DW_SPI_TXFLTR, txlevel);
> +               dws->write_w(dws, DW_SPI_TXFLTR, txlevel);
>
>                 /* Set the interrupt mask */
>                 imask |= SPI_INT_TXEI | SPI_INT_TXOI |
> @@ -516,11 +516,11 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
>                 u32 fifo;
>
>                 for (fifo = 1; fifo < 256; fifo++) {
> -                       dw_writew(dws, DW_SPI_TXFLTR, fifo);
> -                       if (fifo != dw_readw(dws, DW_SPI_TXFLTR))
> +                       dws->write_w(dws, DW_SPI_TXFLTR, fifo);
> +                       if (fifo != dws->read_w(dws, DW_SPI_TXFLTR))
>                                 break;
>                 }
> -               dw_writew(dws, DW_SPI_TXFLTR, 0);
> +               dws->write_w(dws, DW_SPI_TXFLTR, 0);
>
>                 dws->fifo_len = (fifo == 1) ? 0 : fifo;
>                 dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
> @@ -545,6 +545,11 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>         dws->dma_addr = (dma_addr_t)(dws->paddr + 0x60);
>         snprintf(dws->name, sizeof(dws->name), "dw_spi%d", dws->bus_num);
>
> +       if (!dws->read_w)
> +               dws->read_w = dw_readw;
> +       if (!dws->write_w)
> +               dws->write_w = dw_writew;
> +
>         ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
>                         dws->name, master);
>         if (ret < 0) {
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 855bfdd..1df09e2 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -141,6 +141,8 @@ struct dw_spi {
>  #ifdef CONFIG_DEBUG_FS
>         struct dentry *debugfs;
>  #endif
> +       u16 (*read_w)(struct dw_spi *dws, u32 offset);
> +       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);
>  };
>
>  static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
> @@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>         __raw_writew(val, dws->regs + offset);
>  }
>
> +static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
> +{
> +       return (u16)__raw_readl(dws->regs + offset);
> +}
> +
> +static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
> +{
> +       __raw_writel((u32)val, dws->regs + offset);
> +}
> +

So, does simple
dws->readw = dw_readl;
dws->writew = dw_writel;

work for you?

>  static inline void spi_enable_chip(struct dw_spi *dws, int enable)
>  {
>         dw_writel(dws, DW_SPI_SSIENR, (enable ? 1 : 0));
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
tthayer@opensource.altera.com March 9, 2015, 6:01 p.m. UTC | #2
On 03/07/2015 01:52 PM, Andy Shevchenko wrote:
> On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@opensource.altera.com> wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
>> peripherals. The current spi-dw driver uses 16bit accesses in
>> some locations. Use function pointers to support 32 bit accesses
>> but retain legacy 16 bit access.
>>
>
> Thanks for this version. My comments below.
>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>>   drivers/spi/spi-dw-mmio.c |    7 ++++++-
>>   drivers/spi/spi-dw.c      |   29 +++++++++++++++++------------
>>   drivers/spi/spi-dw.h      |   12 ++++++++++++
>>   3 files changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>> index eb03e12..c4fe9e9 100644
>> --- a/drivers/spi/spi-dw-mmio.c
>> +++ b/drivers/spi/spi-dw-mmio.c
>> @@ -76,8 +76,13 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
>>
>>          num_cs = 4;
>>
>> -       if (pdev->dev.of_node)
>> +       if (pdev->dev.of_node) {
>>                  of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs);
>> +               if (of_property_read_bool(pdev->dev.of_node, "32bit_access")) {
>> +                       dws->read_w = dw_readw32;
>> +                       dws->write_w = dw_writew32;
>
> Can we use just  readw/writew (w/o underscores) as names for the accessors?
>

I tried this initially and got a namespace conflict with the readw & 
write2 macros.
macro writew passed 3 arguments, but takes just 2
macro readw passed 2 arguments, but takes just 1

>> +               }
>> +       }
>>
>>          dws->num_cs = num_cs;
>>
>> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
>> index c5fa2be..d008791 100644
>> --- a/drivers/spi/spi-dw.c
>> +++ b/drivers/spi/spi-dw.c
>> @@ -157,7 +157,7 @@ static inline u32 tx_max(struct dw_spi *dws)
>>          u32 tx_left, tx_room, rxtx_gap;
>>
>>          tx_left = (dws->tx_end - dws->tx) / dws->n_bytes;
>> -       tx_room = dws->fifo_len - dw_readw(dws, DW_SPI_TXFLR);
>> +       tx_room = dws->fifo_len - dws->read_w(dws, DW_SPI_TXFLR);
>>
>>          /*
>>           * Another concern is about the tx/rx mismatch, we
>> @@ -178,7 +178,7 @@ static inline u32 rx_max(struct dw_spi *dws)
>>   {
>>          u32 rx_left = (dws->rx_end - dws->rx) / dws->n_bytes;
>>
>> -       return min_t(u32, rx_left, dw_readw(dws, DW_SPI_RXFLR));
>> +       return min_t(u32, rx_left, dws->read_w(dws, DW_SPI_RXFLR));
>>   }
>>
>>   static void dw_writer(struct dw_spi *dws)
>> @@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)
>>                          else
>>                                  txw = *(u16 *)(dws->tx);
>>                  }
>> -               dw_writew(dws, DW_SPI_DR, txw);
>> +               dws->write_w(dws, DW_SPI_DR, txw);
>>                  dws->tx += dws->n_bytes;
>>          }
>>   }
>> @@ -205,7 +205,7 @@ static void dw_reader(struct dw_spi *dws)
>>          u16 rxw;
>>
>>          while (max--) {
>> -               rxw = dw_readw(dws, DW_SPI_DR);
>> +               rxw = dws->read_w(dws, DW_SPI_DR);
>>                  /* Care rx only if the transfer's original "rx" is not null */
>>                  if (dws->rx_end - dws->len) {
>>                          if (dws->n_bytes == 1)
>> @@ -254,11 +254,11 @@ static void int_error_stop(struct dw_spi *dws, const char *msg)
>>
>>   static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>>   {
>> -       u16 irq_status = dw_readw(dws, DW_SPI_ISR);
>> +       u16 irq_status = dws->read_w(dws, DW_SPI_ISR);
>>
>>          /* Error handling */
>>          if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
>> -               dw_readw(dws, DW_SPI_ICR);
>> +               dws->read_w(dws, DW_SPI_ICR);
>>                  int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>>                  return IRQ_HANDLED;
>>          }
>> @@ -283,7 +283,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
>>   {
>>          struct spi_master *master = dev_id;
>>          struct dw_spi *dws = spi_master_get_devdata(master);
>> -       u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
>> +       u16 irq_status = dws->read_w(dws, DW_SPI_ISR) & 0x3f;
>>
>>          if (!irq_status)
>>                  return IRQ_NONE;
>> @@ -379,7 +379,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>>                  cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
>>          }
>>
>> -       dw_writew(dws, DW_SPI_CTRL0, cr0);
>> +       dws->write_w(dws, DW_SPI_CTRL0, cr0);
>>
>>          /* Check if current transfer is a DMA transaction */
>>          dws->dma_mapped = map_dma_buffers(master, spi, transfer);
>> @@ -393,7 +393,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>>           */
>>          if (!dws->dma_mapped && !chip->poll_mode) {
>>                  txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
>> -               dw_writew(dws, DW_SPI_TXFLTR, txlevel);
>> +               dws->write_w(dws, DW_SPI_TXFLTR, txlevel);
>>
>>                  /* Set the interrupt mask */
>>                  imask |= SPI_INT_TXEI | SPI_INT_TXOI |
>> @@ -516,11 +516,11 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
>>                  u32 fifo;
>>
>>                  for (fifo = 1; fifo < 256; fifo++) {
>> -                       dw_writew(dws, DW_SPI_TXFLTR, fifo);
>> -                       if (fifo != dw_readw(dws, DW_SPI_TXFLTR))
>> +                       dws->write_w(dws, DW_SPI_TXFLTR, fifo);
>> +                       if (fifo != dws->read_w(dws, DW_SPI_TXFLTR))
>>                                  break;
>>                  }
>> -               dw_writew(dws, DW_SPI_TXFLTR, 0);
>> +               dws->write_w(dws, DW_SPI_TXFLTR, 0);
>>
>>                  dws->fifo_len = (fifo == 1) ? 0 : fifo;
>>                  dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
>> @@ -545,6 +545,11 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>>          dws->dma_addr = (dma_addr_t)(dws->paddr + 0x60);
>>          snprintf(dws->name, sizeof(dws->name), "dw_spi%d", dws->bus_num);
>>
>> +       if (!dws->read_w)
>> +               dws->read_w = dw_readw;
>> +       if (!dws->write_w)
>> +               dws->write_w = dw_writew;
>> +
>>          ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
>>                          dws->name, master);
>>          if (ret < 0) {
>> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
>> index 855bfdd..1df09e2 100644
>> --- a/drivers/spi/spi-dw.h
>> +++ b/drivers/spi/spi-dw.h
>> @@ -141,6 +141,8 @@ struct dw_spi {
>>   #ifdef CONFIG_DEBUG_FS
>>          struct dentry *debugfs;
>>   #endif
>> +       u16 (*read_w)(struct dw_spi *dws, u32 offset);
>> +       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);
>>   };
>>
>>   static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>> @@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>>          __raw_writew(val, dws->regs + offset);
>>   }
>>
>> +static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
>> +{
>> +       return (u16)__raw_readl(dws->regs + offset);
>> +}
>> +
>> +static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
>> +{
>> +       __raw_writel((u32)val, dws->regs + offset);
>> +}
>> +
>
> So, does simple
> dws->readw = dw_readl;
> dws->writew = dw_writel;
>
> work for you?
>

Yes, but I get the macro conflict shown above and "assignment from 
incompatible pointer type" warnings. If I use the dws->read_w and 
dws->write_w names, I get the incompatible pointer type warnings but it 
works.

Thanks for reviewing.

>>   static inline void spi_enable_chip(struct dw_spi *dws, int enable)
>>   {
>>          dw_writel(dws, DW_SPI_SSIENR, (enable ? 1 : 0));
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko March 9, 2015, 6:54 p.m. UTC | #3
On Mon, Mar 9, 2015 at 8:01 PM, Thor Thayer
<tthayer@opensource.altera.com> wrote:
>
>
> On 03/07/2015 01:52 PM, Andy Shevchenko wrote:
>>
>> On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@opensource.altera.com> wrote:
>>>
>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>
>>> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
>>> peripherals. The current spi-dw driver uses 16bit accesses in
>>> some locations. Use function pointers to support 32 bit accesses
>>> but retain legacy 16 bit access.

>>> --- a/drivers/spi/spi-dw-mmio.c
>>> +++ b/drivers/spi/spi-dw-mmio.c
>>> @@ -76,8 +76,13 @@ static int dw_spi_mmio_probe(struct platform_device
>>> *pdev)
>>>
>>>          num_cs = 4;
>>>
>>> -       if (pdev->dev.of_node)
>>> +       if (pdev->dev.of_node) {
>>>                  of_property_read_u32(pdev->dev.of_node, "num-cs",
>>> &num_cs);
>>> +               if (of_property_read_bool(pdev->dev.of_node,
>>> "32bit_access")) {

"32bit-access"

>>> +                       dws->read_w = dw_readw32;
>>> +                       dws->write_w = dw_writew32;
>>
>>
>> Can we use just  readw/writew (w/o underscores) as names for the
>> accessors?
>>
>
> I tried this initially and got a namespace conflict with the readw & write2
> macros.
> macro writew passed 3 arguments, but takes just 2
> macro readw passed 2 arguments, but takes just 1

Might be I wasn't clear enough. I meant

dws->readw = …
dws->writew = …

>>> --- a/drivers/spi/spi-dw.h
>>> +++ b/drivers/spi/spi-dw.h
>>> @@ -141,6 +141,8 @@ struct dw_spi {
>>>   #ifdef CONFIG_DEBUG_FS
>>>          struct dentry *debugfs;
>>>   #endif
>>> +       u16 (*read_w)(struct dw_spi *dws, u32 offset);
>>> +       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);

readw
writew

As I can see there are no such field names in struct dw_spi.

>>>   };
>>>
>>>   static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>>> @@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32
>>> offset, u16 val)
>>>          __raw_writew(val, dws->regs + offset);
>>>   }
>>>
>>> +static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
>>> +{
>>> +       return (u16)__raw_readl(dws->regs + offset)

Maybe return dw_readl(dws, offset);

>>> +}
>>> +
>>> +static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
>>> +{
>>> +       __raw_writel((u32)val, dws->regs + offset);

dw_writel(dws, offset, val);

>>> +}
>>> +
>>
>> So, does simple
>> dws->readw = dw_readl;
>> dws->writew = dw_writel;
>>
>> work for you?
>>
>
> Yes, but I get the macro conflict shown above and "assignment from
> incompatible pointer type" warnings. If I use the dws->read_w and
> dws->write_w names, I get the incompatible pointer type warnings but it
> works.
>
> Thanks for reviewing.

Okay, I guess there are two variants:
a) replace u16 by u32 in existing functions;
b) introduce new ones like you did.

If no other opinions I think we better go b), but see above comments.

And one more thing. If dw_readw() works for maybe we leave them as is for now?
tthayer@opensource.altera.com March 9, 2015, 7:47 p.m. UTC | #4
On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
> On Mon, Mar 9, 2015 at 8:01 PM, Thor Thayer
> <tthayer@opensource.altera.com> wrote:
>>
>>
>> On 03/07/2015 01:52 PM, Andy Shevchenko wrote:
>>>
>>> On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@opensource.altera.com> wrote:
>>>>
>>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>>
>>>> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
>>>> peripherals. The current spi-dw driver uses 16bit accesses in
>>>> some locations. Use function pointers to support 32 bit accesses
>>>> but retain legacy 16 bit access.
>
>>>> --- a/drivers/spi/spi-dw-mmio.c
>>>> +++ b/drivers/spi/spi-dw-mmio.c
>>>> @@ -76,8 +76,13 @@ static int dw_spi_mmio_probe(struct platform_device
>>>> *pdev)
>>>>
>>>>           num_cs = 4;
>>>>
>>>> -       if (pdev->dev.of_node)
>>>> +       if (pdev->dev.of_node) {
>>>>                   of_property_read_u32(pdev->dev.of_node, "num-cs",
>>>> &num_cs);
>>>> +               if (of_property_read_bool(pdev->dev.of_node,
>>>> "32bit_access")) {
>
> "32bit-access"
>

Thanks. I will make the change.

>>>> +                       dws->read_w = dw_readw32;
>>>> +                       dws->write_w = dw_writew32;
>>>
>>>
>>> Can we use just  readw/writew (w/o underscores) as names for the
>>> accessors?
>>>
>>
>> I tried this initially and got a namespace conflict with the readw & write2
>> macros.
>> macro writew passed 3 arguments, but takes just 2
>> macro readw passed 2 arguments, but takes just 1
>
> Might be I wasn't clear enough. I meant
>
> dws->readw = …
> dws->writew = …
>

Yes, we agree. These are exactly what I was using when I saw the error.

>>>> --- a/drivers/spi/spi-dw.h
>>>> +++ b/drivers/spi/spi-dw.h
>>>> @@ -141,6 +141,8 @@ struct dw_spi {
>>>>    #ifdef CONFIG_DEBUG_FS
>>>>           struct dentry *debugfs;
>>>>    #endif
>>>> +       u16 (*read_w)(struct dw_spi *dws, u32 offset);
>>>> +       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);
>
> readw
> writew
>
> As I can see there are no such field names in struct dw_spi.
>

Yes. This seems safe but I still see the macro conflict which is strange 
to me. Like you, I initially used the dws->readw and dws->writew because 
I didn't see any conflicting field names.

Are readw & writew reserved variable names?

>>>>    };
>>>>
>>>>    static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>>>> @@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32
>>>> offset, u16 val)
>>>>           __raw_writew(val, dws->regs + offset);
>>>>    }
>>>>
>>>> +static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
>>>> +{
>>>> +       return (u16)__raw_readl(dws->regs + offset)
>
> Maybe return dw_readl(dws, offset);
>
>>>> +}
>>>> +
>>>> +static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
>>>> +{
>>>> +       __raw_writel((u32)val, dws->regs + offset);
>
> dw_writel(dws, offset, val);
>

Yes, I'll make these two changes - at least the write change. Based on 
feedback from your suggestion below, the dw_readw32 may be removed.

>>>> +}
>>>> +
>>>
>>> So, does simple
>>> dws->readw = dw_readl;
>>> dws->writew = dw_writel;
>>>
>>> work for you?
>>>
>>
>> Yes, but I get the macro conflict shown above and "assignment from
>> incompatible pointer type" warnings. If I use the dws->read_w and
>> dws->write_w names, I get the incompatible pointer type warnings but it
>> works.
>>
>> Thanks for reviewing.
>
> Okay, I guess there are two variants:
> a) replace u16 by u32 in existing functions;
> b) introduce new ones like you did.
>
> If no other opinions I think we better go b), but see above comments.
>
> And one more thing. If dw_readw() works for maybe we leave them as is for now?
>

Yes, I just need the 32 bit write. I was trying to remain consistent but 
I agree that only changing only writes would minimize the changes.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko March 9, 2015, 8:02 p.m. UTC | #5
On Mon, Mar 9, 2015 at 9:47 PM, Thor Thayer
<tthayer@opensource.altera.com> wrote:
> On 03/09/2015 01:54 PM, Andy Shevchenko wrote:

> Yes, I just need the 32 bit write. I was trying to remain consistent but I
> agree that only changing only writes would minimize the changes.

Which is still makes me anxious.

I have briefly read a chapter for SPI in pdf you sent link to. I
didn't find anything except SPI supports 32 bit data width.

It would be nice if you ping your HW engineers and clarify what
exactly is happening there.
tthayer@opensource.altera.com March 10, 2015, 8:34 p.m. UTC | #6
On 03/09/2015 03:02 PM, Andy Shevchenko wrote:
> On Mon, Mar 9, 2015 at 9:47 PM, Thor Thayer
> <tthayer@opensource.altera.com> wrote:
>> On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
>
>> Yes, I just need the 32 bit write. I was trying to remain consistent but I
>> agree that only changing only writes would minimize the changes.
>
> Which is still makes me anxious.
>
> I have briefly read a chapter for SPI in pdf you sent link to. I
> didn't find anything except SPI supports 32 bit data width.
>
> It would be nice if you ping your HW engineers and clarify what
> exactly is happening there.
>

I confirmed with our HW engineer that writes are required to be 32 bits. 
We are in the process of updating our documentation to explicitly say this.

Since there are no byte enables on our 32 bit APB interface, writing 
only 1 or 2 bytes could corrupt the other bytes in the 32 bit word. The 
32 bit write enforces requiring an explicit read/modify/write for less 
than 32 bit writes.

Since reads are non-destructive (nothing is being written back into the 
register), it is safe for all reads to get promoted to 32 bit reads.

Is it preferable to have both 32 bit reads and writes for consistency?

Or is it preferable to make the minimal number of changes which would be 
to only add a 32 bit write function?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown March 10, 2015, 8:40 p.m. UTC | #7
On Tue, Mar 10, 2015 at 03:34:50PM -0500, Thor Thayer wrote:

> Is it preferable to have both 32 bit reads and writes for consistency?

> Or is it preferable to make the minimal number of changes which would be to
> only add a 32 bit write function?

It's probably preferable to be symmetric, if only to help anyone else
who has a similar system that generates errors instead of silently
handling, but unless someone has any great reason it's not a blocker for
me.
Andy Shevchenko March 10, 2015, 8:44 p.m. UTC | #8
On Tue, Mar 10, 2015 at 10:34 PM, Thor Thayer
<tthayer@opensource.altera.com> wrote:
> On 03/09/2015 03:02 PM, Andy Shevchenko wrote:
>>
>> On Mon, Mar 9, 2015 at 9:47 PM, Thor Thayer
>> <tthayer@opensource.altera.com> wrote:
>>>
>>> On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
>>
>>
>>> Yes, I just need the 32 bit write. I was trying to remain consistent but
>>> I
>>> agree that only changing only writes would minimize the changes.
>>
>>
>> Which is still makes me anxious.
>>
>> I have briefly read a chapter for SPI in pdf you sent link to. I
>> didn't find anything except SPI supports 32 bit data width.
>>
>> It would be nice if you ping your HW engineers and clarify what
>> exactly is happening there.
>>
>
> I confirmed with our HW engineer that writes are required to be 32 bits. We
> are in the process of updating our documentation to explicitly say this.
>
> Since there are no byte enables on our 32 bit APB interface, writing only 1
> or 2 bytes could corrupt the other bytes in the 32 bit word. The 32 bit
> write enforces requiring an explicit read/modify/write for less than 32 bit
> writes.
>
> Since reads are non-destructive (nothing is being written back into the
> register), it is safe for all reads to get promoted to 32 bit reads.

Thanks for detailed explanation!

> Is it preferable to have both 32 bit reads and writes for consistency?
>
> Or is it preferable to make the minimal number of changes which would be to
> only add a 32 bit write function?

Both I think just to be more clear.

However, I'm now considering what if we just replace
dw_{write/read}w() by l-variants without any additional DT property
and accessor functions?
Would it work for both your cases (old chip, new chip)?
On my side I may test this on Intel MID.
tthayer@opensource.altera.com March 10, 2015, 10:22 p.m. UTC | #9
On 03/10/2015 03:44 PM, Andy Shevchenko wrote:
> On Tue, Mar 10, 2015 at 10:34 PM, Thor Thayer
> <tthayer@opensource.altera.com> wrote:
>> On 03/09/2015 03:02 PM, Andy Shevchenko wrote:
>>>
>>> On Mon, Mar 9, 2015 at 9:47 PM, Thor Thayer
>>> <tthayer@opensource.altera.com> wrote:
>>>>
>>>> On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
>>>
>>>
>>>> Yes, I just need the 32 bit write. I was trying to remain consistent but
>>>> I
>>>> agree that only changing only writes would minimize the changes.
>>>
>>>
>>> Which is still makes me anxious.
>>>
>>> I have briefly read a chapter for SPI in pdf you sent link to. I
>>> didn't find anything except SPI supports 32 bit data width.
>>>
>>> It would be nice if you ping your HW engineers and clarify what
>>> exactly is happening there.
>>>
>>
>> I confirmed with our HW engineer that writes are required to be 32 bits. We
>> are in the process of updating our documentation to explicitly say this.
>>
>> Since there are no byte enables on our 32 bit APB interface, writing only 1
>> or 2 bytes could corrupt the other bytes in the 32 bit word. The 32 bit
>> write enforces requiring an explicit read/modify/write for less than 32 bit
>> writes.
>>
>> Since reads are non-destructive (nothing is being written back into the
>> register), it is safe for all reads to get promoted to 32 bit reads.
>
> Thanks for detailed explanation!
>
>> Is it preferable to have both 32 bit reads and writes for consistency?
>>
>> Or is it preferable to make the minimal number of changes which would be to
>> only add a 32 bit write function?
>
> Both I think just to be more clear.
>
> However, I'm now considering what if we just replace
> dw_{write/read}w() by l-variants without any additional DT property
> and accessor functions?
> Would it work for both your cases (old chip, new chip)?
> On my side I may test this on Intel MID.
>

Yes, this would be the simplest solution. The l-variant certainly works 
on our legacy SoCs. I'll be curious to hear your testing results.

The data sheet mentions that registers are addressed at 32-bit 
boundaries to remain consistent with the AHB bus (Section 6.1 of 
dw_apb_ssi_db.pdf).  Additionally unused bits are reserved for writes 
and 0 for reads so this seems like a good solution.

My concern is the presence of legacy devices that I have no way of 
testing. Is a Request For Test in the body of the patch sufficient?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko March 11, 2015, 10:27 a.m. UTC | #10
On Tue, 2015-03-10 at 17:22 -0500, Thor Thayer wrote:


> > However, I'm now considering what if we just replace
> > dw_{write/read}w() by l-variants without any additional DT property
> > and accessor functions?
> > Would it work for both your cases (old chip, new chip)?
> > On my side I may test this on Intel MID.
> >
> 
> Yes, this would be the simplest solution. The l-variant certainly works 
> on our legacy SoCs. I'll be curious to hear your testing results.

Whenever you send a new version of patch.

> The data sheet mentions that registers are addressed at 32-bit 
> boundaries to remain consistent with the AHB bus (Section 6.1 of 
> dw_apb_ssi_db.pdf).  Additionally unused bits are reserved for writes 
> and 0 for reads so this seems like a good solution.
> 
> My concern is the presence of legacy devices that I have no way of 
> testing. Is a Request For Test in the body of the patch sufficient?

Better to write this wider in cover letter or (in case of one patch) in
additional description usually located after '---' line.

Since I fixed couple of bugs in core I'm not sure we have a lot of
users. Nevertheless, can you check who was recent and / or active
contributor to spi-dw-mmio.c and put him / her to Cc list.
diff mbox

Patch

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index eb03e12..c4fe9e9 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -76,8 +76,13 @@  static int dw_spi_mmio_probe(struct platform_device *pdev)
 
 	num_cs = 4;
 
-	if (pdev->dev.of_node)
+	if (pdev->dev.of_node) {
 		of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs);
+		if (of_property_read_bool(pdev->dev.of_node, "32bit_access")) {
+			dws->read_w = dw_readw32;
+			dws->write_w = dw_writew32;
+		}
+	}
 
 	dws->num_cs = num_cs;
 
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index c5fa2be..d008791 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -157,7 +157,7 @@  static inline u32 tx_max(struct dw_spi *dws)
 	u32 tx_left, tx_room, rxtx_gap;
 
 	tx_left = (dws->tx_end - dws->tx) / dws->n_bytes;
-	tx_room = dws->fifo_len - dw_readw(dws, DW_SPI_TXFLR);
+	tx_room = dws->fifo_len - dws->read_w(dws, DW_SPI_TXFLR);
 
 	/*
 	 * Another concern is about the tx/rx mismatch, we
@@ -178,7 +178,7 @@  static inline u32 rx_max(struct dw_spi *dws)
 {
 	u32 rx_left = (dws->rx_end - dws->rx) / dws->n_bytes;
 
-	return min_t(u32, rx_left, dw_readw(dws, DW_SPI_RXFLR));
+	return min_t(u32, rx_left, dws->read_w(dws, DW_SPI_RXFLR));
 }
 
 static void dw_writer(struct dw_spi *dws)
@@ -194,7 +194,7 @@  static void dw_writer(struct dw_spi *dws)
 			else
 				txw = *(u16 *)(dws->tx);
 		}
-		dw_writew(dws, DW_SPI_DR, txw);
+		dws->write_w(dws, DW_SPI_DR, txw);
 		dws->tx += dws->n_bytes;
 	}
 }
@@ -205,7 +205,7 @@  static void dw_reader(struct dw_spi *dws)
 	u16 rxw;
 
 	while (max--) {
-		rxw = dw_readw(dws, DW_SPI_DR);
+		rxw = dws->read_w(dws, DW_SPI_DR);
 		/* Care rx only if the transfer's original "rx" is not null */
 		if (dws->rx_end - dws->len) {
 			if (dws->n_bytes == 1)
@@ -254,11 +254,11 @@  static void int_error_stop(struct dw_spi *dws, const char *msg)
 
 static irqreturn_t interrupt_transfer(struct dw_spi *dws)
 {
-	u16 irq_status = dw_readw(dws, DW_SPI_ISR);
+	u16 irq_status = dws->read_w(dws, DW_SPI_ISR);
 
 	/* Error handling */
 	if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
-		dw_readw(dws, DW_SPI_ICR);
+		dws->read_w(dws, DW_SPI_ICR);
 		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
 		return IRQ_HANDLED;
 	}
@@ -283,7 +283,7 @@  static irqreturn_t dw_spi_irq(int irq, void *dev_id)
 {
 	struct spi_master *master = dev_id;
 	struct dw_spi *dws = spi_master_get_devdata(master);
-	u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
+	u16 irq_status = dws->read_w(dws, DW_SPI_ISR) & 0x3f;
 
 	if (!irq_status)
 		return IRQ_NONE;
@@ -379,7 +379,7 @@  static int dw_spi_transfer_one(struct spi_master *master,
 		cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
 	}
 
-	dw_writew(dws, DW_SPI_CTRL0, cr0);
+	dws->write_w(dws, DW_SPI_CTRL0, cr0);
 
 	/* Check if current transfer is a DMA transaction */
 	dws->dma_mapped = map_dma_buffers(master, spi, transfer);
@@ -393,7 +393,7 @@  static int dw_spi_transfer_one(struct spi_master *master,
 	 */
 	if (!dws->dma_mapped && !chip->poll_mode) {
 		txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
-		dw_writew(dws, DW_SPI_TXFLTR, txlevel);
+		dws->write_w(dws, DW_SPI_TXFLTR, txlevel);
 
 		/* Set the interrupt mask */
 		imask |= SPI_INT_TXEI | SPI_INT_TXOI |
@@ -516,11 +516,11 @@  static void spi_hw_init(struct device *dev, struct dw_spi *dws)
 		u32 fifo;
 
 		for (fifo = 1; fifo < 256; fifo++) {
-			dw_writew(dws, DW_SPI_TXFLTR, fifo);
-			if (fifo != dw_readw(dws, DW_SPI_TXFLTR))
+			dws->write_w(dws, DW_SPI_TXFLTR, fifo);
+			if (fifo != dws->read_w(dws, DW_SPI_TXFLTR))
 				break;
 		}
-		dw_writew(dws, DW_SPI_TXFLTR, 0);
+		dws->write_w(dws, DW_SPI_TXFLTR, 0);
 
 		dws->fifo_len = (fifo == 1) ? 0 : fifo;
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
@@ -545,6 +545,11 @@  int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 	dws->dma_addr = (dma_addr_t)(dws->paddr + 0x60);
 	snprintf(dws->name, sizeof(dws->name), "dw_spi%d", dws->bus_num);
 
+	if (!dws->read_w)
+		dws->read_w = dw_readw;
+	if (!dws->write_w)
+		dws->write_w = dw_writew;
+
 	ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
 			dws->name, master);
 	if (ret < 0) {
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 855bfdd..1df09e2 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -141,6 +141,8 @@  struct dw_spi {
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
 #endif
+	u16 (*read_w)(struct dw_spi *dws, u32 offset);
+	void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);
 };
 
 static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
@@ -163,6 +165,16 @@  static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
 	__raw_writew(val, dws->regs + offset);
 }
 
+static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
+{
+	return (u16)__raw_readl(dws->regs + offset);
+}
+
+static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
+{
+	__raw_writel((u32)val, dws->regs + offset);
+}
+
 static inline void spi_enable_chip(struct dw_spi *dws, int enable)
 {
 	dw_writel(dws, DW_SPI_SSIENR, (enable ? 1 : 0));