| Message ID | 1480280279-9552-2-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
> > 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
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
> 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
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
> 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
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
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
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
On Tue, Nov 29, 2016 at 12:54:24PM -0500, Vivien Didelot wrote: > 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? 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
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], },
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(+)