| Message ID | 1448257280-15717-1-git-send-email-shubhraj@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)? 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@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? > } > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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.
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;
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@xilinx.com> --- drivers/spi/spi-xilinx.c | 34 +++++++++++++++++++--------------- 1 files changed, 19 insertions(+), 15 deletions(-)