* [PATCH] spi/spi-xilinx: Use a cached copy of CR register
@ 2015-11-23 5:41 Shubhrajyoti Datta
[not found] ` <1448257280-15717-1-git-send-email-shubhraj-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 3+ messages in thread
From: Shubhrajyoti Datta @ 2015-11-23 5:41 UTC (permalink / raw)
To: linux-spi-u79uwXL29TY76Z2rM5mHXA
Cc: soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA,
michal.simek-gjFFaj9aHVfQT0dZR+AlfA,
anirudh-gjFFaj9aHVfQT0dZR+AlfA, Shubhrajyoti Datta
The Control register is written by the driver.
Use a cached copy of the register so that the reads
thereafter can use the variable instead of the register read.
Signed-off-by: Shubhrajyoti Datta <shubhraj-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
drivers/spi/spi-xilinx.c | 34 +++++++++++++++++++---------------
1 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 3009121..73f4453 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -91,6 +91,7 @@ struct xilinx_spi {
u8 bytes_per_word;
int buffer_size; /* buffer size in words */
u32 cs_inactive; /* Level of the CS pins when inactive*/
+ u32 cr; /* Control Register*/
unsigned int (*read_fn)(void __iomem *);
void (*write_fn)(u32, void __iomem *);
};
@@ -180,15 +181,15 @@ static void xspi_init_hw(struct xilinx_spi *xspi)
xspi->write_fn(0xffff, regs_base + XSPI_SSR_OFFSET);
/* Disable the transmitter, enable Manual Slave Select Assertion,
* put SPI controller into master mode, and enable it */
- xspi->write_fn(XSPI_CR_MANUAL_SSELECT | XSPI_CR_MASTER_MODE |
- XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET | XSPI_CR_RXFIFO_RESET,
- regs_base + XSPI_CR_OFFSET);
+ xspi->cr = XSPI_CR_MANUAL_SSELECT | XSPI_CR_MASTER_MODE |
+ XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET | XSPI_CR_RXFIFO_RESET;
+ xspi->write_fn(xspi->cr, regs_base + XSPI_CR_OFFSET);
+ xspi->cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
}
static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
{
struct xilinx_spi *xspi = spi_master_get_devdata(spi->master);
- u16 cr;
u32 cs;
if (is_on == BITBANG_CS_INACTIVE) {
@@ -198,16 +199,16 @@ static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
}
/* Set the SPI clock phase and polarity */
- cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET) & ~XSPI_CR_MODE_MASK;
+ xspi->cr &= ~XSPI_CR_MODE_MASK;
if (spi->mode & SPI_CPHA)
- cr |= XSPI_CR_CPHA;
+ xspi->cr |= XSPI_CR_CPHA;
if (spi->mode & SPI_CPOL)
- cr |= XSPI_CR_CPOL;
+ xspi->cr |= XSPI_CR_CPOL;
if (spi->mode & SPI_LSB_FIRST)
- cr |= XSPI_CR_LSB_FIRST;
+ xspi->cr |= XSPI_CR_LSB_FIRST;
if (spi->mode & SPI_LOOP)
- cr |= XSPI_CR_LOOP;
- xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
+ xspi->cr |= XSPI_CR_LOOP;
+ xspi->write_fn(xspi->cr, xspi->regs + XSPI_CR_OFFSET);
/* We do not check spi->max_speed_hz here as the SPI clock
* frequency is not software programmable (the IP block design
@@ -254,7 +255,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
u32 isr;
use_irq = true;
/* Inhibit irq to avoid spurious irqs on tx_empty*/
- cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
+ cr = xspi->cr;
+ xspi->cr = cr | XSPI_CR_TRANS_INHIBIT;
xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
xspi->regs + XSPI_CR_OFFSET);
/* ACK old irqs (if any) */
@@ -283,7 +285,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
*/
if (use_irq) {
- xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
+ xspi->cr = cr;
+ xspi->write_fn(xspi->cr, xspi->regs + XSPI_CR_OFFSET);
wait_for_completion(&xspi->done);
/* A transmit has just completed. Process received data
* and check for more data to transmit. Always inhibit
@@ -291,8 +294,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
* register/FIFO, or make sure it is stopped if we're
* done.
*/
- xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
- xspi->regs + XSPI_CR_OFFSET);
+ xspi->cr = xspi->cr | XSPI_CR_TRANS_INHIBIT;
+ xspi->write_fn(xspi->cr, xspi->regs + XSPI_CR_OFFSET);
sr = XSPI_SR_TX_EMPTY_MASK;
} else
sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
@@ -318,7 +321,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
if (use_irq) {
xspi->write_fn(0, xspi->regs + XIPIF_V123B_DGIER_OFFSET);
- xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
+ xspi->cr = cr;
+ xspi->write_fn(xspi->cr, xspi->regs + XSPI_CR_OFFSET);
}
return t->len;
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] spi/spi-xilinx: Use a cached copy of CR register
[not found] ` <1448257280-15717-1-git-send-email-shubhraj-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
@ 2015-11-24 12:32 ` Ricardo Ribalda Delgado
[not found] ` <CAPybu_1VE_PS6DD+-eMDUkUapAxQYaT1eyeT2mVag1-HVhX23g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 3+ messages in thread
From: Ricardo Ribalda Delgado @ 2015-11-24 12:32 UTC (permalink / raw)
To: Shubhrajyoti Datta
Cc: linux-spi, Sören Brinkmann, Michal Simek,
anirudh-gjFFaj9aHVfQT0dZR+AlfA, Shubhrajyoti Datta
Hello
On Mon, Nov 23, 2015 at 6:41 AM, Shubhrajyoti Datta
<shubhrajyoti.datta-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
> The Control register is written by the driver.
> Use a cached copy of the register so that the reads
> thereafter can use the variable instead of the register read.
>
Have you made any measurement of the performance impact?
How much time do we save by removing one ioread per transacion + one
ioread per chip select (only in irq mode)?
If it is meassurable, dont you think that it would be better to
encapsulate the access to the sr with 2 functions?
Other inline comments:
> Signed-off-by: Shubhrajyoti Datta <shubhraj-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/spi/spi-xilinx.c | 34 +++++++++++++++++++---------------
> 1 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
> index 3009121..73f4453 100644
> --- a/drivers/spi/spi-xilinx.c
> +++ b/drivers/spi/spi-xilinx.c
> @@ -91,6 +91,7 @@ struct xilinx_spi {
> u8 bytes_per_word;
> int buffer_size; /* buffer size in words */
> u32 cs_inactive; /* Level of the CS pins when inactive*/
> + u32 cr; /* Control Register*/
> unsigned int (*read_fn)(void __iomem *);
> void (*write_fn)(u32, void __iomem *);
> };
> @@ -180,15 +181,15 @@ static void xspi_init_hw(struct xilinx_spi *xspi)
> xspi->write_fn(0xffff, regs_base + XSPI_SSR_OFFSET);
> /* Disable the transmitter, enable Manual Slave Select Assertion,
> * put SPI controller into master mode, and enable it */
> - xspi->write_fn(XSPI_CR_MANUAL_SSELECT | XSPI_CR_MASTER_MODE |
> - XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET | XSPI_CR_RXFIFO_RESET,
> - regs_base + XSPI_CR_OFFSET);
> + xspi->cr = XSPI_CR_MANUAL_SSELECT | XSPI_CR_MASTER_MODE |
> + XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET | XSPI_CR_RXFIFO_RESET;
> + xspi->write_fn(xspi->cr, regs_base + XSPI_CR_OFFSET);
> + xspi->cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
Is this last line needed?
> }
>
> static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
> {
> struct xilinx_spi *xspi = spi_master_get_devdata(spi->master);
> - u16 cr;
> u32 cs;
>
> if (is_on == BITBANG_CS_INACTIVE) {
> @@ -198,16 +199,16 @@ static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
> }
>
> /* Set the SPI clock phase and polarity */
> - cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET) & ~XSPI_CR_MODE_MASK;
> + xspi->cr &= ~XSPI_CR_MODE_MASK;
> if (spi->mode & SPI_CPHA)
> - cr |= XSPI_CR_CPHA;
> + xspi->cr |= XSPI_CR_CPHA;
> if (spi->mode & SPI_CPOL)
> - cr |= XSPI_CR_CPOL;
> + xspi->cr |= XSPI_CR_CPOL;
> if (spi->mode & SPI_LSB_FIRST)
> - cr |= XSPI_CR_LSB_FIRST;
> + xspi->cr |= XSPI_CR_LSB_FIRST;
> if (spi->mode & SPI_LOOP)
> - cr |= XSPI_CR_LOOP;
> - xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
> + xspi->cr |= XSPI_CR_LOOP;
> + xspi->write_fn(xspi->cr, xspi->regs + XSPI_CR_OFFSET);
>
> /* We do not check spi->max_speed_hz here as the SPI clock
> * frequency is not software programmable (the IP block design
> @@ -254,7 +255,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
> u32 isr;
> use_irq = true;
> /* Inhibit irq to avoid spurious irqs on tx_empty*/
> - cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
> + cr = xspi->cr;
> + xspi->cr = cr | XSPI_CR_TRANS_INHIBIT;
> xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
> xspi->regs + XSPI_CR_OFFSET);
> /* ACK old irqs (if any) */
> @@ -283,7 +285,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
> */
>
> if (use_irq) {
> - xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
> + xspi->cr = cr;
> + xspi->write_fn(xspi->cr, xspi->regs + XSPI_CR_OFFSET);
> wait_for_completion(&xspi->done);
> /* A transmit has just completed. Process received data
> * and check for more data to transmit. Always inhibit
> @@ -291,8 +294,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
> * register/FIFO, or make sure it is stopped if we're
> * done.
> */
> - xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
> - xspi->regs + XSPI_CR_OFFSET);
> + xspi->cr = xspi->cr | XSPI_CR_TRANS_INHIBIT;
> + xspi->write_fn(xspi->cr, xspi->regs + XSPI_CR_OFFSET);
> sr = XSPI_SR_TX_EMPTY_MASK;
> } else
> sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
> @@ -318,7 +321,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
>
> if (use_irq) {
> xspi->write_fn(0, xspi->regs + XIPIF_V123B_DGIER_OFFSET);
> - xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
> + xspi->cr = cr;
> + xspi->write_fn(xspi->cr, xspi->regs + XSPI_CR_OFFSET);
> }
>
> return t->len;
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] spi/spi-xilinx: Use a cached copy of CR register
[not found] ` <CAPybu_1VE_PS6DD+-eMDUkUapAxQYaT1eyeT2mVag1-HVhX23g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2015-11-24 12:51 ` Shubhrajyoti Datta
0 siblings, 0 replies; 3+ messages in thread
From: Shubhrajyoti Datta @ 2015-11-24 12:51 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: linux-spi, Soren Brinkmann, Michal Simek, Anirudha Sarangi,
shubhrajyoti.datta-Re5JQEeQqe8AvxtiuMwx3w
> -----Original Message-----
> From: Ricardo Ribalda Delgado [mailto:ricardo.ribalda@gmail.com]
> Sent: Tuesday, November 24, 2015 6:03 PM
> To: Shubhrajyoti Datta
> Cc: linux-spi; Soren Brinkmann; Michal Simek; Anirudha Sarangi; Shubhrajyoti
> Datta
> Subject: Re: [PATCH] spi/spi-xilinx: Use a cached copy of CR register
>
> Hello
>
> On Mon, Nov 23, 2015 at 6:41 AM, Shubhrajyoti Datta
> <shubhrajyoti.datta@xilinx.com> wrote:
> > The Control register is written by the driver.
> > Use a cached copy of the register so that the reads thereafter can use
> > the variable instead of the register read.
> >
>
> Have you made any measurement of the performance impact?
>
> How much time do we save by removing one ioread per transacion + one
> ioread per chip select (only in irq mode)?
For my case it is not much.
However it was suggested on the list by Jean-Francois Dagenais.
As he had a system behind a pci bridge.
>
> If it is meassurable, dont you think that it would be better to encapsulate the
> access to the sr with 2 functions?
Ok can try that.
>
> Other inline comments:
>
> > Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>
> > ---
> > drivers/spi/spi-xilinx.c | 34 +++++++++++++++++++---------------
> > 1 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c index
> > 3009121..73f4453 100644
> > --- a/drivers/spi/spi-xilinx.c
> > +++ b/drivers/spi/spi-xilinx.c
> > @@ -91,6 +91,7 @@ struct xilinx_spi {
> > u8 bytes_per_word;
> > int buffer_size; /* buffer size in words */
> > u32 cs_inactive; /* Level of the CS pins when inactive*/
> > + u32 cr; /* Control Register*/
> > unsigned int (*read_fn)(void __iomem *);
> > void (*write_fn)(u32, void __iomem *); }; @@ -180,15 +181,15
> > @@ static void xspi_init_hw(struct xilinx_spi *xspi)
> > xspi->write_fn(0xffff, regs_base + XSPI_SSR_OFFSET);
> > /* Disable the transmitter, enable Manual Slave Select Assertion,
> > * put SPI controller into master mode, and enable it */
> > - xspi->write_fn(XSPI_CR_MANUAL_SSELECT |
> XSPI_CR_MASTER_MODE |
> > - XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET |
> XSPI_CR_RXFIFO_RESET,
> > - regs_base + XSPI_CR_OFFSET);
> > + xspi->cr = XSPI_CR_MANUAL_SSELECT | XSPI_CR_MASTER_MODE |
> > + XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET |
> XSPI_CR_RXFIFO_RESET;
> > + xspi->write_fn(xspi->cr, regs_base + XSPI_CR_OFFSET);
> > + xspi->cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
>
> Is this last line needed?
Yes because XSPI_CR_RXFIFO_RESET bits when written to 1, this bit forces a reset of the Receive FIFO to the empty
condition. One AXI clock cycle after reset, this bit is again set to 0.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-11-24 12:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-23 5:41 [PATCH] spi/spi-xilinx: Use a cached copy of CR register Shubhrajyoti Datta
[not found] ` <1448257280-15717-1-git-send-email-shubhraj-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
2015-11-24 12:32 ` Ricardo Ribalda Delgado
[not found] ` <CAPybu_1VE_PS6DD+-eMDUkUapAxQYaT1eyeT2mVag1-HVhX23g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-11-24 12:51 ` Shubhrajyoti Datta
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.