Date:   Wed, 12 Sep 2018 15:10:11 +0200
From:   Marek Vasut <marex@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] net: dsa: mv88e6xxx: Add MV88E6352 DT compatible

On 09/12/2018 02:47 PM, Andrew Lunn wrote:
> 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.

Many of them use the fallback version to avoid polluting the tables in
the kernel with redundant compat strings. The specific string is only
added into the driver if there is a HW difference which needs to be
dealt with.

> 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.

And this works fine, until marvell screws it up in one of those chips.

> 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 the DT should correctly describe the hardware, if it doesn't, it's
just broken.

> 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.

I am arguing you should have both specific compatible which describes
which exact chip is in that device AND a fallback compatible which the
driver matches on.

-- 
Best regards,
Marek Vasut

Powered by blists - more mailing lists