lkml.org 
[lkml]   [2021]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/4] spi: dw: Add Synopsys Component version reading and parsing
On Sat, Nov 13, 2021 at 03:15:41PM +0200, Andy Shevchenko wrote:
> On Sat, Nov 13, 2021 at 2:19 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Sat, Nov 13, 2021 at 01:14:30AM +0200, Andy Shevchenko wrote:
> > > On Saturday, November 13, 2021, Serge Semin <fancer.lancer@gmail.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@gmail.com> wrote:
> > > > > > On Fri, Nov 12, 2021 at 10:52 PM Serge Semin
> > > > > > <Sergey.Semin@baikalelectronics.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?
>

> The transformation ruins the fourcc [1] representation. We know that
> this is an ASCII. We know the position and meaning of each from 4
> characters.
>
> [1]: https://en.wikipedia.org/wiki/FourCC

So that four-CC thing wasn't Synopsys invention after all, but a sort
of a relatively common approach. Your point finally starts making
sense to me.

>
> > Why would you prefer the
> > DWC3 approach better than the one implemented in my patch?
>
> You asked what is _better_ implementation than yours. It doesn't mean
> the DWC3 approach fully fits here, but
> - SPI DW has the same issue as DWC3 solves, i.e. different version
> lines for two or more IPs of the same kind (see DWC3, DWC31, DWC32)
> - it doesn't ruins 4cc
>
> In case if we need to print it, I would rather see something in %p4cc
> implementation than customized 100500 approaches spreaded over the
> entire kernel.

Yep, these are the main pros of the DWC3 approach. Just to note
in fact AFAICS Synopsys doesn't utilize the denoted components
versioning for all its IP-cores. At least DW (G|X-G|XL-G)?MACs define
either a normal one-byte component version ID (for instance version
3.3 looks as 0x33) or two-bytes with pair - IP-core and version IDs.
But that likely is an exception, while the most of DWCs are indeed
identified by the ASCII tuple.

In our case we don't have the IP-core version explicitly encoded in
the Component version register, so it is determined by the
platform-specific code. With a minor effort I'll be able to just
convert the DW_SPI_CAP_DWC_HSSI capability into two macros with virtual
IP-core ID thus we'd have a more-or-less coherent versioning API. In
this case we can retain the ASCII Version ID. Settled then. I'll send
v2 with this patch reworked as you suggest.

-Sergey

>
> > 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.
>
> > > > > > > + }
> > > >
> > > > > 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.
>
> Depends what you would like to do with this. If it's only for
> information to the developer, then it fits, if you need to compare,
> then see above.
>
> --
> With Best Regards,
> Andy Shevchenko

\
 
 \ /
  Last update: 2021-11-13 22:32    [W:0.077 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site