| Message ID | 1480280279-9552-1-git-send-email-afaerber@suse.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Signed-off-by: Andreas Färber <afaerber@suse.de> --- Documentation/devicetree/bindings/net/dsa/marvell.txt | 1 + 1 file changed, 1 insertion(+)