[PATCH 1/2] Documentation: net: dsa: marvell: Add 88E6176

[PATCH 1/2] Documentation: net: dsa: marvell: Add 88E6176

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>
---
 Documentation/devicetree/bindings/net/dsa/marvell.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
index b3dd6b40e0de..000bc3b16edd 100644
--- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
+++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
@@ -15,6 +15,7 @@ Additional required and optional properties can be found in dsa.txt.
 
 Required properties:
 - compatible	       : Should be one of "marvell,mv88e6085" or
+			 "marvell,mv88e6176" or
 			 "marvell,mv88e6190"
 - reg                  : Address on the MII bus for the switch.
 
-- 
2.6.6

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andreas Färber]

This model is found on the Turris Omnia.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 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 77f13ada2612..95b9efb33ec7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4280,6 +4280,10 @@ static const struct of_device_id mv88e6xxx_of_match[] = {
 		.data = &mv88e6xxx_table[MV88E6085],
 	},
 	{
+		.compatible = "marvell,mv88e6176",
+		.data = &mv88e6xxx_table[MV88E6176],
+	},
+	{
 		.compatible = "marvell,mv88e6190",
 		.data = &mv88e6xxx_table[MV88E6190],
 	},
-- 
2.6.6

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andrew Lunn]

On Sun, Nov 27, 2016 at 09:57:59PM +0100, Andreas Färber wrote:
> This model is found on the Turris Omnia.

This driver already supports nearly 30 different Marvell switch
models. Please document why the marvell,mv88e6176 is special and why
it needs its own compatible string when the others don't.

      Andrew

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andreas Färber]

Am 27.11.2016 um 22:27 schrieb Andrew Lunn:
> On Sun, Nov 27, 2016 at 09:57:59PM +0100, Andreas Färber wrote:
>> This model is found on the Turris Omnia.
> 
> This driver already supports nearly 30 different Marvell switch
> models. Please document why the marvell,mv88e6176 is special and why
> it needs its own compatible string when the others don't.

I don't understand.

The commit message above already points out for which device this is
(and you also know from the LAKML thread).

You as driver author should know that the .data pointer is vital to your
driver - you even recently accepted another model that conflicted with
my patch. So are you arguing for a ", which uses a Device Tree for
booting" half-sentence here?

The others not having an entry simply means no one needed them yet.

And any Turris Omnia side changes need to go through the mvebu tree.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andrew Lunn]

> > This driver already supports nearly 30 different Marvell switch
> > models. Please document why the marvell,mv88e6176 is special and why
> > it needs its own compatible string when the others don't.
> 
> I don't understand.

Think about what i said. Why does the 6176 need its own compatible
string, when the two 6352s and the 6165 on the zii-devel-b don't have
one? And the DIR 665 has a 6171, which does not have a compatible
string of its own. The clearfog actually has a 6176, and it seems to
work fine without a compatible string.

> You as driver author should know that the .data pointer is vital to your
> driver

Exactly, so if i ask why is it needed, maybe you should stop and think
for a while.

> you even recently accepted another model that conflicted with
> my patch.

And think about that also, and you will find the 6390 family, who's
first device is 6190, is not compatible with the 6085, and so needs a
different compatible string.

	  Andrew

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andreas Färber]

Andrew,

Am 27.11.2016 um 23:08 schrieb Andrew Lunn:
>>> This driver already supports nearly 30 different Marvell switch
>>> models. Please document why the marvell,mv88e6176 is special and why
>>> it needs its own compatible string when the others don't.
>>
>> I don't understand.
> 
> Think about what i said. Why does the 6176 need its own compatible
> string, when the two 6352s and the 6165 on the zii-devel-b don't have
> one? And the DIR 665 has a 6171, which does not have a compatible
> string of its own. The clearfog actually has a 6176, and it seems to
> work fine without a compatible string.
> 
>> You as driver author should know that the .data pointer is vital to your
>> driver
> 
> Exactly, so if i ask why is it needed, maybe you should stop and think
> for a while.
> 
>> you even recently accepted another model that conflicted with
>> my patch.
> 
> And think about that also, and you will find the 6390 family, who's
> first device is 6190, is not compatible with the 6085, and so needs a
> different compatible string.

Try to see it from my perspective: I see that some vf610 device I don't
have (found via `git grep marvell,mv88e6` or so) uses
"marvell,mv88e6085". I then assume it has that device on board. How
would I know it doesn't? Same for the other boards you mention.

Unfortunately some of your replies are slightly cryptic. Had you simply
replied 'please just use "marvell,mv88e6085" instead', it would've been
much more clear what you want. (Same for extending the subject instead
of just pointing to some FAQ.)

So are you okay with patch 1/2 documenting the compatible? Then we could
drop 2/2 and use "marvell,mv88e6176", "marvell,mv88e6085" instead of
just the latter. Or would you rather drop both and keep the actual chip
a comment?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andrew Lunn]

> Try to see it from my perspective: I see that some vf610 device I don't
> have (found via `git grep marvell,mv88e6` or so) uses
> "marvell,mv88e6085". I then assume it has that device on board. How
> would I know it doesn't? Same for the other boards you mention.
> 
> Unfortunately some of your replies are slightly cryptic. Had you simply
> replied 'please just use "marvell,mv88e6085" instead', it would've been
> much more clear what you want. (Same for extending the subject instead
> of just pointing to some FAQ.)

By reading the FAQ you have learnt more than me saying put the correct
tree in the subject line. By asking you to explain why you need a
compatible string, i'm trying to make you think, look at the code and
understand it. In the future, you might think and understand the code
before posting a patch, and then we all save time.

> So are you okay with patch 1/2 documenting the compatible? Then we could
> drop 2/2 and use "marvell,mv88e6176", "marvell,mv88e6085" instead of
> just the latter. Or would you rather drop both and keep the actual chip
> a comment?

A comment only please.

Thanks
	Andrew

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]

Hello Andrew,

On Mon, Nov 28, 2016 at 12:10:09AM +0100, Andrew Lunn wrote:
> > Try to see it from my perspective: I see that some vf610 device I don't
> > have (found via `git grep marvell,mv88e6` or so) uses
> > "marvell,mv88e6085". I then assume it has that device on board. How
> > would I know it doesn't? Same for the other boards you mention.
> > 
> > Unfortunately some of your replies are slightly cryptic. Had you simply
> > replied 'please just use "marvell,mv88e6085" instead', it would've been
> > much more clear what you want. (Same for extending the subject instead
> > of just pointing to some FAQ.)
> 
> By reading the FAQ you have learnt more than me saying put the correct
> tree in the subject line. By asking you to explain why you need a
> compatible string, i'm trying to make you think, look at the code and
> understand it. In the future, you might think and understand the code
> before posting a patch, and then we all save time.

I agree to Andreas though, that it makes an school teacher impression.
Something like:

	Please fix the subject. Check the FAQ for the details, which btw
	is worth a read completely.

is IMHO better in this regard and once you found the problem there you
don't need to ask back if it's that what was meant.

> > So are you okay with patch 1/2 documenting the compatible? Then we could
> > drop 2/2 and use "marvell,mv88e6176", "marvell,mv88e6085" instead of
> > just the latter. Or would you rather drop both and keep the actual chip
> > a comment?
> 
> A comment only please.

I still wonder (and didn't get an answer back when I asked about this)
why a comment is preferred here. For other devices I know it's usual and
requested by the maintainers to use:

	compatible = "exact name", "earlyer device to match driver";

. This is more robust, documents the situation more formally and makes
it better greppable. The price to pay is only a few bytes in the dtb
which IMO is ok.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andrew Lunn]

> I still wonder (and didn't get an answer back when I asked about this)
> why a comment is preferred here. For other devices I know it's usual and
> requested by the maintainers to use:
> 
> 	compatible = "exact name", "earlyer device to match driver";
> 
> . This is more robust, documents the situation more formally and makes
> it better greppable. The price to pay is only a few bytes in the dtb
> which IMO is ok.

We did discuss this a while back. The information is useless and
should to be ignored if present.

The switch has a register which contains its model and revision. Each
port has a set of registers, and register 3 contains the
model/version. For all devices compatible with the 6085, the port
registers start at address 0x10. For the 6190, the port registers
start at 0x0. So given one of these two compatible strings, we can
find the model of the device, from something which is burned into the
silicon.

Now, say we did add per device compatible strings. We look up the
model burned into the silicon, find it is different to what the device
tree is and do what? Fail the probe? Or just keep going using the
value in the silicon? It seems silly to fail the probe if the driver
does support the model, but that means the device tree is never
verified and hence probably wrong. Why have wrong information in the
device tree, especially wrong information which we never use. It is
better to not have that information in the device tree.

Linus has said he does not like ARM devices because of all the busses
which are not enumerable. Here we have a device which with a little
bit of help we can enumerate. So we should. 

    Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2968 bytes --]

On 11/28/2016 02:17 PM, Andrew Lunn wrote:
>> I still wonder (and didn't get an answer back when I asked about this)
>> why a comment is preferred here. For other devices I know it's usual and
>> requested by the maintainers to use:
>>
>> 	compatible = "exact name", "earlyer device to match driver";
>>
>> . This is more robust, documents the situation more formally and makes
>> it better greppable. The price to pay is only a few bytes in the dtb
>> which IMO is ok.
> 
> We did discuss this a while back. The information is useless and
> should to be ignored if present.

Who is "we"?

> The switch has a register which contains its model and revision. Each
> port has a set of registers, and register 3 contains the
> model/version. For all devices compatible with the 6085, the port
> registers start at address 0x10. For the 6190, the port registers
> start at 0x0. So given one of these two compatible strings, we can
> find the model of the device, from something which is burned into the
> silicon.
> 
> Now, say we did add per device compatible strings. We look up the
> model burned into the silicon, find it is different to what the device
> tree is and do what? Fail the probe? Or just keep going using the

I'd say fail to probe is the right thing to do. Of course that doesn't
work for already supported models because it will break compatibility.

I'd value the advantages (i.e. easily find machines with a given
hardware) higher than making broken dtbs work, so being a bit silly is
fine for me.

> value in the silicon? It seems silly to fail the probe if the driver
> does support the model, but that means the device tree is never
> verified and hence probably wrong. Why have wrong information in the
> device tree, especially wrong information which we never use. It is
> better to not have that information in the device tree.

At least we'd have a canonical way to specify the type of switch. If
it's not verified it's as good and bad as a dts comment. But the latter
isn't available in the dtb, which I consider a small disadvantage.

Also it seems wrong to write "marvell,mv88e6085" (only) if I know the
hardware is really a "marvell,mv88e6176".

> Linus has said he does not like ARM devices because of all the busses
> which are not enumerable. Here we have a device which with a little
> bit of help we can enumerate. So we should. 

If you write

	compatible = "marvell,mv88e6176", "marvell,mv88e6085";

you can still enumerate in the same way as before.

There are several more instances where the device tree specifies
something that could be probed instead. Some examples:

	compatible = "ethernet-phy-id0141.0DD1", "ethernet-phy-ieee802.3-c22";
	compatible = "spansion,s25fl164k", "jedec,spi-nor";
	compatible = "fsl,imx25-flexcan", "fsl,p1010-flexcan";
	compatible = "arm,pl011", "arm,primecell";

So you think they are all doing it wrong?

Best regards
Uwe



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andrew Lunn]

On Mon, Nov 28, 2016 at 04:44:47PM +0100, Uwe Kleine-König wrote:
> On 11/28/2016 02:17 PM, Andrew Lunn wrote:
> >> I still wonder (and didn't get an answer back when I asked about this)
> >> why a comment is preferred here. For other devices I know it's usual and
> >> requested by the maintainers to use:
> >>
> >> 	compatible = "exact name", "earlyer device to match driver";
> >>
> >> . This is more robust, documents the situation more formally and makes
> >> it better greppable. The price to pay is only a few bytes in the dtb
> >> which IMO is ok.
> > 
> > We did discuss this a while back. The information is useless and
> > should to be ignored if present.
> 
> Who is "we"?

Anybody on netdev a while back, but mostly Vivien and myself.

> I'd say fail to probe is the right thing to do. Of course that doesn't
> work for already supported models because it will break compatibility.

And that is the first rule of device tree, never break backwards
compatibility.

> Also it seems wrong to write "marvell,mv88e6085" (only) if I know the
> hardware is really a "marvell,mv88e6176".

Why? Remember, the property name is called "compatible", not "is".

> > Linus has said he does not like ARM devices because of all the busses
> > which are not enumerable. Here we have a device which with a little
> > bit of help we can enumerate. So we should. 
> 
> If you write
> 
> 	compatible = "marvell,mv88e6176", "marvell,mv88e6085";
> 
> you can still enumerate in the same way as before.

Sure it would. But if the driver decides to ignore it, it is likely to
be wrong. Developers are used to comments being wrong. We are
suspicious of comments, we don't trust them 100%. But if somebody sees
a property in a device tree, they put a higher 'trustability' value on
it. But it actually has less trustable than a comment.
 
> There are several more instances where the device tree specifies
> something that could be probed instead. Some examples:
> 
> 	compatible = "ethernet-phy-id0141.0DD1", "ethernet-phy-ieee802.3-c22";

There are examples of phys which don't implemented register 2 and
3. In that case, you do need to specify the ID, if you want the
correct driver to load.

> 	compatible = "spansion,s25fl164k", "jedec,spi-nor";

And there was a push recently to add "jedec,spi-nor" everywhere and
deprecate a specific compatible because it is also not needed.

> 	compatible = "fsl,imx25-flexcan", "fsl,p1010-flexcan";
> 	compatible = "arm,pl011", "arm,primecell";

I don't know these hardwares, so cannot comment.

  Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Vivien Didelot]

Hi,

Uwe Kleine-König <uwe@kleine-koenig.org> writes:

> Also it seems wrong to write "marvell,mv88e6085" (only) if I know the
> hardware is really a "marvell,mv88e6176".

I agree. It might be complex for a user to dig into the driver in order
to figure out how the switch ID is read and which compatible to choose.

I've sent a patch to change this https://lkml.org/lkml/2016/6/8/1198 but
Andrew had a stronger opinion on compatible strings, which makes sense.

>> Linus has said he does not like ARM devices because of all the busses
>> which are not enumerable. Here we have a device which with a little
>> bit of help we can enumerate. So we should. 
>
> If you write
>
> 	compatible = "marvell,mv88e6176", "marvell,mv88e6085";
>
> you can still enumerate in the same way as before.

So we don't break the existing DTS files, I like this.

The driver already prints info about the detected switch. Instead of
failing at probe, which seems against the notion of compatible and
breaks the existing behavior, it could report the eventual mismatch?

We have examples for both usage, still I don't know what the best
practices are. My _preference_ would go with enumerating them all.

Thanks,

        Vivien

Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support

[Andrew Lunn]

On Tue, Nov 29, 2016 at 12:54:24PM -0500, Vivien Didelot wrote:
> Hi,
> 
> Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org> writes:
> 
> > Also it seems wrong to write "marvell,mv88e6085" (only) if I know the
> > hardware is really a "marvell,mv88e6176".
> 
> I agree. It might be complex for a user to dig into the driver in order
> to figure out how the switch ID is read and which compatible to choose.
> 
> I've sent a patch to change this https://lkml.org/lkml/2016/6/8/1198 but
> Andrew had a stronger opinion on compatible strings, which makes sense.
> 
> >> Linus has said he does not like ARM devices because of all the busses
> >> which are not enumerable. Here we have a device which with a little
> >> bit of help we can enumerate. So we should. 
> >
> > If you write
> >
> > 	compatible = "marvell,mv88e6176", "marvell,mv88e6085";
> >
> > you can still enumerate in the same way as before.
> 
> So we don't break the existing DTS files, I like this.
> 
> The driver already prints info about the detected switch. Instead of
> failing at probe, which seems against the notion of compatible and
> breaks the existing behavior, it could report the eventual mismatch?

I'm still against it.

Say we let the driver probe and warn when the compatible string is
wrong. Is this likely to get fixed? Probably not, it is just a
warning, people ignore them.

A few years later, we have accumulated a number of broken device
trees. And suddenly we really do have a Marvell device with a broken
ID in its port register, or more likely, the revision number was not
incremented but there was incompatible register changes. We suddenly
do have to look at the compatible string. But we know many are actually
wrong. How do we know which to trust? We basically have to say, if the
compatible is "marvell,mv88e6042" we trust it, all the others we don't
trust and ignore.

Isn't that madness?

What we have today is verified correct. If this hypothetical
"marvell,mv88e6042" does happen, then no problems, we add a compatible
string for it, and it works.

We should probably add a comment to the mv88e6xxx_of_match array,
explaining how it currently works.

      Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html