RE: [PATCH v2 3/5] spi: Add support for Renesas CSI

[Fabrizio Castro <fabrizio.castro.jz@renesas.com>]

Hi Geert,

Thanks your reply!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> 
> Hi Fabrizio,
> 
> On Fri, Jul 14, 2023 at 12:35 AM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI
> > >
> > > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro
> > > <fabrizio.castro.jz@renesas.com> wrote:
> > >
> > > ...
> > >
> > > > > > +#define CSI_CKS_MAX                0x3FFF
> > > > >
> > > > > If it's limited by number of bits, i would explicitly use that
> > > information
> > > > > as
> > > > > (BIT(14) - 1).
> > > >
> > > > That value represents the register setting for the maximum clock
> > > divider.
> > > > The maximum divider and corresponding register setting are
> plainly
> > > stated
> > > > in the HW User Manual, therefore I would like to use either
> (plain)
> > > value
> > > > to make it easier for the reader.
> > > >
> > > > I think perhaps the below makes this clearer:
> > > > #define CSI_CKS_MAX_DIV_RATIO   32766
> > >
> > > Hmm... To me it's a bit confusing now. Shouldn't it be 32767?
> >
> > 32766 is the correct value.
> >
> > Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to
> generate the
> > serial clock (output from master), with CSI_CLKSEL_CKS ranging from
> 0x1 (that
> > means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by
> 32766).
> >
> > >
> > > > #define CSI_CKS_MAX             (CSI_CKS_MAX_DIV_RATIO >> 1)
> > >
> > > Whatever you choose it would be better to add a comment to explain
> > > this. Because the above is more clear to me with BIT(14)-1 if the
> > > register field is 14-bit long.
> > > With this value(s) I'm lost. Definitely needs a comment.
> >
> > To cater for a wider audience (and not just for those who have read
> the
> > HW manual), I think perhaps the below would probably be the best
> compromise:
> >
> > /*
> >  * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to
> generate the
> >  * serial clock (output from master), with CSI_CLKSEL_CKS ranging
> from 0x1 (that
> >  * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by
> 32766).
> >  */
> > #define CSI_CKS_MAX             (BIT(14)-1)
> 
> Or GENMASK(13, 0)

Yeah.

> 
> As we have
> 
>     #define CSI_CLKSEL_CKS          GENMASK(14, 1)
> 
> and bit 0 must of the CLKSEL register must always be zero, the actual
> divider is incidentally FIELD_GET(GENMASK(14, 0), clksel).
> No idea if that can be useful to simplify the code, though ;-)

Thanks for pointing this out. Will have a look, but no promises ;-)

> 
> > > > static inline unsigned int x_trg(unsigned int words)
> > > > {
> > > >         return fls(words) - 1;
> > > > }
> > >
> > > OK, but I think you can use it just inplace, no need to have such
> as a
> > > standalone function.
> >
> > The above is actually equivalent to ilog2()
> >
> > >
> > > > static inline unsigned int x_trg_words(unsigned int words)
> > > > {
> > > >         return 1 << x_trg(words);
> > > > }
> > >
> > > Besides a better form of BIT(...) this looks to me like NIH
> > > roundup_pow_of_two().
> >
> > rounddown_pow_of_two().
> >
> > I have tested the driver with s/x_trg/ilog2 and
> > s/x_trg_words/roundup_pow_of_two and it looks like I am losing tiny
> bit of
> > performance (probably down to the use of ternary operators in both
> macros)
> > but I think it's okay, let's not reinvent the wheel and let's keep
> it more
> > readable, I'll switch to using the above macros.
> 
> You mean this is not lost in the noise of the big loop in
> rzv2m_csi_pio_transfer(), which is even waiting on an event?
> I find that a bit surprising...

Those calculations get done when no TX/RX is in progress, and they are
executed for every burst (as they are used to decide how many bytes
in the FIFOs to use for the current burst), therefore they add a delay
to the whole thing.

It's only a tiny drop, about 0.4% .

Cheers,
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