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

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

[Marek Vasut]

Add DT compatible string for MV88E6352 switch.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index e07838430d16..15427380e32e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4796,6 +4796,10 @@ static const struct of_device_id mv88e6xxx_of_match[] = {
 		.compatible = "marvell,mv88e6190",
 		.data = &mv88e6xxx_table[MV88E6190],
 	},
+	{
+		.compatible = "marvell,mv88e6352",
+		.data = &mv88e6xxx_table[MV88E6352],
+	},
 	{ /* sentinel */ },
 };
 
-- 
2.18.0

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

[Andrew Lunn]

On Wed, Sep 12, 2018 at 12:15:36AM +0200, Marek Vasut wrote:
> Add DT compatible string for MV88E6352 switch.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index e07838430d16..15427380e32e 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -4796,6 +4796,10 @@ static const struct of_device_id mv88e6xxx_of_match[] = {
>  		.compatible = "marvell,mv88e6190",
>  		.data = &mv88e6xxx_table[MV88E6190],
>  	},
> +	{
> +		.compatible = "marvell,mv88e6352",
> +		.data = &mv88e6xxx_table[MV88E6352],
> +	},

NACK.

Not needed. Lots of devices use the 6352. It is compatible to the
marvell,mv88e6085.

	Andrew

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

[Marek Vasut]

On 09/12/2018 01:12 AM, Andrew Lunn wrote:
> On Wed, Sep 12, 2018 at 12:15:36AM +0200, Marek Vasut wrote:
>> Add DT compatible string for MV88E6352 switch.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index e07838430d16..15427380e32e 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -4796,6 +4796,10 @@ static const struct of_device_id mv88e6xxx_of_match[] = {
>>  		.compatible = "marvell,mv88e6190",
>>  		.data = &mv88e6xxx_table[MV88E6190],
>>  	},
>> +	{
>> +		.compatible = "marvell,mv88e6352",
>> +		.data = &mv88e6xxx_table[MV88E6352],
>> +	},
> 
> NACK.
> 
> Not needed. Lots of devices use the 6352. It is compatible to the
> marvell,mv88e6085.

So I should have both values in my DT, that is

compatible = "marvell,mv88e6352", "marvell,mv88e6085";

? That works for me ...

-- 
Best regards,
Marek Vasut

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

[Andrew Lunn]

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

     Andrew

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

[Brandon Streiff]

On 9/11/2018 6:32 PM, Andrew Lunn wrote:
> 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.

To extend what Andrew stated above, Documentation/devicetree/bindings/net/dsa/marvell.txt
(as of 22936c3e6e8c) says which of the two strings you should use for
each model.

The "marvell,mv88e6085" and "marvell,mv88e6190" compatible strings are
best thought of as the "where is the identification register" setting;
from that point the driver can figure out everything else.

-- brandon

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

[Marek Vasut]

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.

You can always patch the code to handle the difference between those
chips based on the extra DT information, but you cannot always patch the
DT itself, since it can be in ROM or whatnot.

-- 
Best regards,
Marek Vasut

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

[Andrew Lunn]

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.

   Andrew

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

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

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

[Andrew Lunn]

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

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

[Marek Vasut]

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

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

[Andrew Lunn]

> But the DT should correctly describe the hardware, if it doesn't, it's
> just broken.

It is more subtle than that. It can be broken, yet work, because it
contains information which we don't use. I really expect there will be
cut/paste errors, meaning the more specific compatible is sometimes
wrong. But since at the moment we don't use it, such a broken DT blob
will work. Until the day we need to make use of the more specific
compatible because there really is broken silicon. At that point, we
introduce a regression. All the devices with broke, yet up until now
working DT blobs, stop working. Are you really going to argue they
where always broken, so we don't care we introduced a regression?

Anyway, this is just rehasing an old discussion. Please go read the
archive. See if you have anything new to add which was not discussed
before.

      Andrew

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

[Marek Vasut]

On 09/12/2018 03:32 PM, Andrew Lunn wrote:
>> But the DT should correctly describe the hardware, if it doesn't, it's
>> just broken.
> 
> It is more subtle than that. It can be broken, yet work, because it
> contains information which we don't use. I really expect there will be
> cut/paste errors, meaning the more specific compatible is sometimes
> wrong.

If your DT is bogus, nothing can be done about that.

> But since at the moment we don't use it, such a broken DT blob
> will work. Until the day we need to make use of the more specific
> compatible because there really is broken silicon. At that point, we
> introduce a regression. All the devices with broke, yet up until now
> working DT blobs, stop working. Are you really going to argue they
> where always broken, so we don't care we introduced a regression?

If the DT is broken, it's already a bug and we cannot do anything about
that but maybe fix the DT somehow.

> Anyway, this is just rehasing an old discussion. Please go read the
> archive. See if you have anything new to add which was not discussed
> before.

Hehe, I've seen those discussions before too.

-- 
Best regards,
Marek Vasut