RE: [PATCH] spi/spi-xilinx: Use a cached copy of CR register

[Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxxxxx>]

> -----Original Message-----
> From: Ricardo Ribalda Delgado [mailto:ricardo.ribalda@xxxxxxxxx]
> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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.

��.n��������+%������w��{.n�����{����)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥