| Message ID | 20230715010407.1751715-8-fabrizio.castro.jz@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: rzv2m-csi: Code refactoring | expand |
Hi Fabrizio, On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > The RX/TX FIFOs implemented by the CSI IP are accessed by > repeatedly reading/writing the same memory address, and > therefore they are the ideal candidate for {read,write}s{b,w}. > The RZ/V2M CSI driver currently implements loops to fill up > the TX FIFO and empty the RX FIFO, differentiating between > 8-bit and 16-bit word size. > Switch to using {read,write}s{b,w} to get rid of the bespoke > loops. > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Thanks for your patch! > --- a/drivers/spi/spi-rzv2m-csi.c > +++ b/drivers/spi/spi-rzv2m-csi.c > @@ -157,22 +157,15 @@ static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi, > > static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi) > { > - int i; > - > if (readl(csi->base + CSI_OFIFOL)) > return -EIO; > > - if (csi->bytes_per_word == 2) { > - u16 *buf = (u16 *)csi->txbuf; > - > - for (i = 0; i < csi->words_to_transfer; i++) > - writel(buf[i], csi->base + CSI_OFIFO); > - } else { > - u8 *buf = (u8 *)csi->txbuf; > - > - for (i = 0; i < csi->words_to_transfer; i++) > - writel(buf[i], csi->base + CSI_OFIFO); > - } > + if (csi->bytes_per_word == 2) > + writesw(csi->base + CSI_OFIFO, csi->txbuf, > + csi->words_to_transfer); > + else > + writesb(csi->base + CSI_OFIFO, csi->txbuf, > + csi->words_to_transfer); According to the hardware documentation[1], the access size for both the CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel() resp. readl(). So please check with the hardware people first. [1] RZ/V2M User's Manual Hardware, Rev. 1.30. Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for your reply! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > {read,write}s{b,w} > > Hi Fabrizio, > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > The RX/TX FIFOs implemented by the CSI IP are accessed by > > repeatedly reading/writing the same memory address, and > > therefore they are the ideal candidate for {read,write}s{b,w}. > > The RZ/V2M CSI driver currently implements loops to fill up > > the TX FIFO and empty the RX FIFO, differentiating between > > 8-bit and 16-bit word size. > > Switch to using {read,write}s{b,w} to get rid of the bespoke > > loops. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > Thanks for your patch! > > > --- a/drivers/spi/spi-rzv2m-csi.c > > +++ b/drivers/spi/spi-rzv2m-csi.c > > > @@ -157,22 +157,15 @@ static int > rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi, > > > > static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi) > > { > > - int i; > > - > > if (readl(csi->base + CSI_OFIFOL)) > > return -EIO; > > > > - if (csi->bytes_per_word == 2) { > > - u16 *buf = (u16 *)csi->txbuf; > > - > > - for (i = 0; i < csi->words_to_transfer; i++) > > - writel(buf[i], csi->base + CSI_OFIFO); > > - } else { > > - u8 *buf = (u8 *)csi->txbuf; > > - > > - for (i = 0; i < csi->words_to_transfer; i++) > > - writel(buf[i], csi->base + CSI_OFIFO); > > - } > > + if (csi->bytes_per_word == 2) > > + writesw(csi->base + CSI_OFIFO, csi->txbuf, > > + csi->words_to_transfer); > > + else > > + writesb(csi->base + CSI_OFIFO, csi->txbuf, > > + csi->words_to_transfer); > > According to the hardware documentation[1], the access size for both > the > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel() > resp. readl(). So please check with the hardware people first. > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30. You are right, access is 32 bits (and although this patch works fine, we should avoid accessing those regs any other way). Now I remember why I decided to go for the bespoke loops in the first place, writesl and readsl provide the right register access, but the wrong pointer arithmetic for this use case. For this patch I ended up selecting writesw/writesb/readsw/readsb to get the right pointer arithmetic, but the register access is not as per manual. I can either fallback to using the bespoke loops (I can still remove the unnecessary u8* and u16* casting ;-) ), or I can add new APIs for this sort of access to io.h (e.g. writesbl, writeswl, readsbl, readswl, in order to get the pointer arithmetic right for the type of array handled, while keeping memory access as expected). What are your thoughts on that? Thanks, Fab > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > geert@linux-m68k.org > > In personal conversations with technical people, I call myself a > hacker. But > when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > > {read,write}s{b,w} > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: ... > > According to the hardware documentation[1], the access size for both > > the > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel() > > resp. readl(). So please check with the hardware people first. > > > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30. > > You are right, access is 32 bits (and although this patch works fine, > we should avoid accessing those regs any other way). Now I remember > why I decided to go for the bespoke loops in the first place, writesl > and readsl provide the right register access, but the wrong pointer > arithmetic for this use case. > For this patch I ended up selecting writesw/writesb/readsw/readsb to > get the right pointer arithmetic, but the register access is not as > per manual. > > I can either fallback to using the bespoke loops (I can still > remove the unnecessary u8* and u16* casting ;-) ), or I can add > new APIs for this sort of access to io.h (e.g. writesbl, writeswl, > readsbl, readswl, in order to get the pointer arithmetic right for > the type of array handled, while keeping memory access as expected). > > What are your thoughts on that? I think that you need to use readsl() / writesl() on the custom buffer with something like *_sparse() / *_condence() APIs added (perhaps locally to this driver) as they may be reused by others in the future. Having all flavours of read*()/write*() does not scale in my opinion.
Hi Andy, Thanks for your reply. > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > {read,write}s{b,w} > > On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > > > {read,write}s{b,w} > > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com> wrote: > > ... > > > > According to the hardware documentation[1], the access size for > both > > > the > > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use > writel() > > > resp. readl(). So please check with the hardware people first. > > > > > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30. > > > > You are right, access is 32 bits (and although this patch works > fine, > > we should avoid accessing those regs any other way). Now I remember > > why I decided to go for the bespoke loops in the first place, > writesl > > and readsl provide the right register access, but the wrong pointer > > arithmetic for this use case. > > For this patch I ended up selecting writesw/writesb/readsw/readsb to > > get the right pointer arithmetic, but the register access is not as > > per manual. > > > > I can either fallback to using the bespoke loops (I can still > > remove the unnecessary u8* and u16* casting ;-) ), or I can add > > new APIs for this sort of access to io.h (e.g. writesbl, writeswl, > > readsbl, readswl, in order to get the pointer arithmetic right for > > the type of array handled, while keeping memory access as expected). > > > > What are your thoughts on that? > > I think that you need to use readsl() / writesl() on the custom buffer > with something like > > *_sparse() / *_condence() APIs added (perhaps locally to this driver) > as they may be reused by others in the future. > Having all flavours of read*()/write*() does not scale in my opinion. Maybe a "generic" macro then (one for reading and one for writing)? So that we can pass the data type (to get the pointer arithmetic right) and the function name to use for the read/write operations (to get the register operations right)? Maybe that would scale and cater for most needs (including the one from this patch)? Cheers, Fab > > -- > With Best Regards, > Andy Shevchenko
> From: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > Subject: RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using > {read,write}s{b,w} > > Hi Andy, > > Thanks for your reply. > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > > {read,write}s{b,w} > > > > On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > > > > {read,write}s{b,w} > > > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro > > > > <fabrizio.castro.jz@renesas.com> wrote: > > > > ... > > > > > > According to the hardware documentation[1], the access size for > > both > > > > the > > > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use > > writel() > > > > resp. readl(). So please check with the hardware people first. > > > > > > > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30. > > > > > > You are right, access is 32 bits (and although this patch works > > fine, > > > we should avoid accessing those regs any other way). Now I > remember > > > why I decided to go for the bespoke loops in the first place, > > writesl > > > and readsl provide the right register access, but the wrong > pointer > > > arithmetic for this use case. > > > For this patch I ended up selecting writesw/writesb/readsw/readsb > to > > > get the right pointer arithmetic, but the register access is not > as > > > per manual. > > > > > > I can either fallback to using the bespoke loops (I can still > > > remove the unnecessary u8* and u16* casting ;-) ), or I can add > > > new APIs for this sort of access to io.h (e.g. writesbl, writeswl, > > > readsbl, readswl, in order to get the pointer arithmetic right for > > > the type of array handled, while keeping memory access as > expected). > > > > > > What are your thoughts on that? > > > > I think that you need to use readsl() / writesl() on the custom > buffer > > with something like > > > > *_sparse() / *_condence() APIs added (perhaps locally to this > driver) > > as they may be reused by others in the future. > > Having all flavours of read*()/write*() does not scale in my > opinion. > > Maybe a "generic" macro then (one for reading and one for writing)? > So that we can pass the data type (to get the pointer arithmetic > right) and the function name to use for the read/write operations > (to get the register operations right)? > Maybe that would scale and cater for most needs (including the one > from this patch)? Something like the below perhaps: #ifndef readsx #define readsx(atype, readc, addr, buffer, count) \ ({ \ if (count) { \ unsigned int cnt = count; \ atype *buf = buffer; \ \ do { \ atype x = readc(addr); \ *buf++ = x; \ } while (--cnt); \ } \ }) #endif #ifndef writesx #define writesx(atype, writec, addr, buffer, count) \ ({ \ if (count) { \ unsigned int cnt = count; \ const atype *buf = buffer; \ \ do { \ writec(*buf++, addr); \ } while (--cnt); \ } \ }) #endif Cheers, Fab > > Cheers, > Fab > > > > > -- > > With Best Regards, > > Andy Shevchenko
diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c index faf5898bc3e0..d0d6b183ffaf 100644 --- a/drivers/spi/spi-rzv2m-csi.c +++ b/drivers/spi/spi-rzv2m-csi.c @@ -86,8 +86,8 @@ struct rzv2m_csi_priv { struct clk *pclk; struct device *dev; struct spi_controller *controller; - const u8 *txbuf; - u8 *rxbuf; + const void *txbuf; + void *rxbuf; int buffer_len; int bytes_sent; int bytes_received; @@ -157,22 +157,15 @@ static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi, static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi) { - int i; - if (readl(csi->base + CSI_OFIFOL)) return -EIO; - if (csi->bytes_per_word == 2) { - u16 *buf = (u16 *)csi->txbuf; - - for (i = 0; i < csi->words_to_transfer; i++) - writel(buf[i], csi->base + CSI_OFIFO); - } else { - u8 *buf = (u8 *)csi->txbuf; - - for (i = 0; i < csi->words_to_transfer; i++) - writel(buf[i], csi->base + CSI_OFIFO); - } + if (csi->bytes_per_word == 2) + writesw(csi->base + CSI_OFIFO, csi->txbuf, + csi->words_to_transfer); + else + writesb(csi->base + CSI_OFIFO, csi->txbuf, + csi->words_to_transfer); csi->txbuf += csi->bytes_to_transfer; csi->bytes_sent += csi->bytes_to_transfer; @@ -182,22 +175,15 @@ static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi) static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi) { - int i; - if (readl(csi->base + CSI_IFIFOL) != csi->bytes_to_transfer) return -EIO; - if (csi->bytes_per_word == 2) { - u16 *buf = (u16 *)csi->rxbuf; - - for (i = 0; i < csi->words_to_transfer; i++) - buf[i] = (u16)readl(csi->base + CSI_IFIFO); - } else { - u8 *buf = (u8 *)csi->rxbuf; - - for (i = 0; i < csi->words_to_transfer; i++) - buf[i] = (u8)readl(csi->base + CSI_IFIFO); - } + if (csi->bytes_per_word == 2) + readsw(csi->base + CSI_IFIFO, csi->rxbuf, + csi->words_to_transfer); + else + readsb(csi->base + CSI_IFIFO, csi->rxbuf, + csi->words_to_transfer); csi->rxbuf += csi->bytes_to_transfer; csi->bytes_received += csi->bytes_to_transfer;
The RX/TX FIFOs implemented by the CSI IP are accessed by repeatedly reading/writing the same memory address, and therefore they are the ideal candidate for {read,write}s{b,w}. The RZ/V2M CSI driver currently implements loops to fill up the TX FIFO and empty the RX FIFO, differentiating between 8-bit and 16-bit word size. Switch to using {read,write}s{b,w} to get rid of the bespoke loops. Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> --- drivers/spi/spi-rzv2m-csi.c | 42 +++++++++++++------------------------ 1 file changed, 14 insertions(+), 28 deletions(-)