Hi,

On Wed 02 Feb 22, 14:26, Robin Murphy wrote:
> On 2022-01-30 00:46, Linus Walleij wrote:
> > On Thu, Jan 20, 2022 at 4:00 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > 
> > > There are lots of different versions of the logicvc block and it
> > > makes little sense to list them all in compatibles since all versions
> > > with the same major are found to be register-compatible.
> > 
> > The reason we try to be precise is because sometime, long after the driver
> > has been merged and maintained for a few years, a bug is discovered
> > in a specific version of the silicon.
> > 
> > What happens is that a fix is applied on all silicon whether it is needed
> > or not.
> > 
> > If you have the precise silicon compatible, you can avoid this and target
> > only a specific version.
> 
> Indeed, the better approach would be something like:
> 
>   compatible:
>     oneOf:
>       - items:
>           - enum:
>               - foo,bar-v1.0
>               - foo,bar,v1.1
>           - const: foo,bar-v1
>       - items:
>           - enum:
>               - foo,bar-v2.0
>           - const: foo,bar-v2
> 
> That way the DTs are future-proof, while drivers can still match on only the
> less-specific strings until a need arises. Plus it avoids the problem that
> if an existing OS that only understands "foo,bar-v1.0" is given a new DT
> with only "foo,bar-v1" for v1.0 hardware it won't be able to use the device,
> even though it's *functionally* capable of doing so.

Yes I understand that we need to keep compatibility with the already-defined
compatible.

> However, from skimming patch #5, it looks possible that none of these
> changes are needed at all. If LOGICVC_IP_VERSION_REG tells you the exact
> revision, and is always present (as the unconditional reading of it
> implies), then the only reason for adding new compatibles would be if, say,
> v5 has more clocks from v4 and you want the binding to enforce that;
> otherwise, newer versions are literally compatible with the
> currently-defined binding and therefore should continue to bind against the
> existing string(s) to maximise forward- and backward-compatibility. Sure,
> it's not the prettiest thing for a "generic" compatible to be based on an
> oddly-specific version number that doesn't necessarily match the actual
> software-discoverable version, but what's done is done and that's the cost
> of ABI.

Indeed it's true that hardware quirks can be applied based on the precise
version read from the register, so I don't think there is a need for overly
precise compatibles.

Since the device-tree binding is currently the same for all versions,
I understand that it makes sense to keep a single compatible (the already
defined one), so I guess I will make another iteration without introducing
new compatibles. But I will probably update the binding document to reflect
which versions are currently known to work with its current state.

> (also, nitpick for that part of patch #5 since I'm here: please include
> linux/bitfield.h rather than reinventing FIELD_GET() locally)

Ah good to know thanks, first time hearing about those.

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com