| Message ID | 1425685594-26595-4-git-send-email-tthayer@opensource.altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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?
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
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.
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
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.
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.
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
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 --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));