Date:   Sat, 13 Nov 2021 03:19:08 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Mark Brown <broonie@...nel.org>,
        Nandhini Srikandan <nandhini.srikandan@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andy Shevchenko <andy@...nel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] spi: dw: Add Synopsys Component version reading and
 parsing

On Sat, Nov 13, 2021 at 01:14:30AM +0200, Andy Shevchenko wrote:
> On Saturday, November 13, 2021, Serge Semin <fancer.lancer@...il.com> wrote:
> 
> > On Fri, Nov 12, 2021 at 11:37:36PM +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 12, 2021 at 11:27 PM Andy Shevchenko
> > > <andy.shevchenko@...il.com> wrote:
> > > > On Fri, Nov 12, 2021 at 10:52 PM Serge Semin
> > > > <Sergey.Semin@...kalelectronics.ru> wrote:
> > >
> > > > > +       /*
> > > > > +        * Retrieve the Synopsys component version if it hasn't been
> > specified
> > > > > +        * by the platform. Note the CoreKit version ID is encoded
> > as a 4bytes
> > > > > +        * ASCII string enclosed with '*' symbol.
> > > > > +        */
> > > > > +       if (!dws->ver) {
> > > > > +               u32 comp;
> > > > > +
> > > > > +               comp = dw_readl(dws, DW_SPI_VERSION);
> > > > > +               dws->ver = (DW_SPI_GET_BYTE(comp, 3) - '0') * 100;
> > > > > +               dws->ver += (DW_SPI_GET_BYTE(comp, 2) - '0') * 10;
> > > > > +               dws->ver += (DW_SPI_GET_BYTE(comp, 1) - '0');
> > > > > +
> > > > > +               dev_dbg(dev, "Synopsys DWC%sSSI v%u.%02u\n",
> > > > > +                       (dws->caps & DW_SPI_CAP_DWC_HSSI) ? " " : "
> > APB ",
> > > > > +                       dws->ver / 100, dws->ver % 100);
> > > >
> >
> > > > Oh là là, first you multiply then you divide in the same piece of code!
> > > > What's wrong with fourcc (and thus keep it in ver filed as is) ? (Also
> > > > we have %p4cc)
> >
> > Please note that's just a dev_DBG() print. So division has been used
> > in there to check whether the conversion was correct. The whole idea
> > behind the code above it was to retrieve the Component version as a
> > single number so then it could be used by the driver code in a simple
> > statement with a normal integer operation. For instance in case if we
> > need to check whether DW SSI IP-core version is greater than 1.01 we'd
> > have something like this: if (dws->ver > 101). Here 101 looks at least
> > close to the original 1.01. How would the statement look with four
> > chars? Of course we could add an another macro which would look like
> > this:
> > #define DW_SSI_VER(_maj, _mid, _min) \
> >         ((_maj) << 24 | (_mid) << 16 | (_min) << 8 | '*')
> > and use it with raw version ID, like this
> > (dws->ver > DW_SSI_VER('1', '0', '1')). But IMO it doesn't look
> > better if not worse.
> > Alternatively we could split the version ID into two parts with
> > major and minor numbers. But normally one doesn't make much sense
> > without another so each statement would need to check both of them
> > at once anyway. So I decided to stick with a simplest solution and
> > combined them into a single storage. Have you got a better idea of
> > how to implement this functionality?
> 
> 
> 

> Then check DWC3 driver which relies on IZp version a lot.

I'm still not convinced that the DWC3 solution would be better in this case.
(I had a similar approach in mind though.) Although it might be suitable
here seeing we could take the IP generation into account in a single
macro. But at the same time having macros defined for each version may
eventually turn into a clumsy set of macros space as it happened in DWC3.

I don't understand what do you see wrong in the suggested here
solution except a math in the debug string? Why would you prefer the
DWC3 approach better than the one implemented in my patch?
I don't really see much benefits in it:
if (dws->ver > 101)
or
if (DW_SPI_VER_AFTER(dws, 101))
In both cases version ID isn't represented in the original
Vendor-defined structure, like "1.01". The only part which could be
considered as better in DWC3 approach is having a macro name, which gives
a bit better notion about the operation. But does it really worth
introducing a new abstraction in the driver?

On the other hand we could intermix the approaches. For instance decode
the Component version as I suggested in this patch and implement a set of
version checking macro. Thus we won't need so many additional macro
encoding the SSI_COMP_VERSION content.

If only we could have a macro like DW_SPI_VER(dws, >=, "1.01a") with no
performance drawbacks I'd be glad to use it. AFAIU compiler can't
operate with the string literal symbols, thus the symbols extraction
like "1.01a"[0] will be performed on each statement execution which isn't
that performant comparing to a simple two integers comparison.

BTW note the DWC3 macros implicitly depend on having a local variable
with dwc name which violates the kernel coding style. 

-Sergey

> 
> 
> >
> > > >
> > > > > +       }
> > >
> >
> > > Have you seen this, btw?
> > >
> > > https://elixir.bootlin.com/linux/latest/source/drivers/
> > tty/serial/8250/8250_dwlib.c#L93
> >
> > It doesn't utilized version ID for something functional, but just
> > prints it to the console. So it isn't that good reference in this
> > case.
> >
> > -Sergey
> >
> > >
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Powered by blists - more mailing lists