Re: [PATCH] net: dsa: mv88e6xxx: Add MV88E6352 DT compatible

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Sep 12, 2018 at 10:38:41AM +0200, Marek Vasut wrote:
> On 09/12/2018 02:46 AM, Andrew Lunn wrote:
> > On Wed, Sep 12, 2018 at 02:04:54AM +0200, Marek Vasut wrote:
> >> On 09/12/2018 01:32 AM, Andrew Lunn wrote:
> >>>> compatible = "marvell,mv88e6352", "marvell,mv88e6085";
> >>>
> >>> Just "marvell,mv88e6085";
> >>>
> >>> Please take a look at all the other DT files using the Marvell
> >>> chips. You will only ever find "marvell,mv88e6085" or
> >>> "marvell,mv88e6190", because everything is compatible to one of these
> >>> two.
> >>
> >> Well, until you find a difference between those chips which you cannot
> >> discern based solely on the ID register content. Then it's better to
> >> have a compatible to match on which matches the actual chip.
> > 
> > Hi Marek
> > 
> > We have been around this loop before. The problem with putting in a
> > more specific compatible is that nothing is validating it. So it is
> > going to be wrong, simple because people cut/paste, and don't
> > necessary change it. So when we do need to look at it, we cannot look
> > at it, because it is wrong.
> > 
> > I would only add a more specific compatible if and when we need it, it
> > is actually used, and we can verify it is correct.
> 
> You may need it in the future and it may not be possible to adjust the
> DT then. So having a specific compatible and fallback compatible is I
> think the way to go. You can always match on the fallback and if the
> time comes, you can match on the specific one because it's there. In
> addition to that, such a DT would fully correctly describe the hardware,
> unlike one with only a fallback compatible.
> 
> Regarding validation, I don't see other systems using this approach
> (specific + fallback compatible) having a problem with validation.
> What is the trick there ?

The trick is, they actually use the specific version, not the
generic. So it is validated, because if the specific version is wrong,
the device generally does not work.

We have no way to do that in this case. We load the driver based on
the generic version. It then goes and looks at the ID registers, and
from that decides what the device really is.

So far, Marvell have not messed this up. The ID registers have been
correct. I trust the ID registers much more than anything in device
tree. It is baked into silicon.

But you are talking of a theoretical case where the ID register is
wrong. For this to get passed us and out their in the wild, with DT
blown in ROM, that probably means there are a batch with the correct
ID and a batch with incorrect IDs. Those with incorrect IDs are
failing. You are arguing that we should then enable the use of the
specific compatible. And i expect that breaks more devices than it
fixes, because people have cut/pasted the wrong value.

       Andrew