| Message ID | 1434547162-6275-2-git-send-email-simon.guinot@sequanux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Simon, On 17/06/2015 15:19, Simon Guinot wrote: > The mvneta driver supports the Ethernet IP found in the Armada 370, XP, > 380 and 385 SoCs. Since at least one more hardware feature is available > for the Armada XP SoCs then a way to identify them is needed. > > This patch introduces a new compatible string "marvell,armada-xp-neta". Let's be future proof by going further. I would like to have an compatible string for each SoC even if we currently we don't use them. You even don't have to use add it in the mvneta.c file if you use: "marvell,armada-380-neta", "marvell,armada-xp-neta" Thanks, Gregory > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > Cc: <stable@vger.kernel.org> # v3.8+ > --- > Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt | 2 +- > drivers/net/ethernet/marvell/mvneta.c | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > index 750d577e8083..f5a8ca29aff0 100644 > --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > @@ -1,7 +1,7 @@ > * Marvell Armada 370 / Armada XP Ethernet Controller (NETA) > > Required properties: > -- compatible: should be "marvell,armada-370-neta". > +- compatible: "marvell,armada-370-neta" or "marvell,armada-xp-neta". > - reg: address and length of the register set for the device. > - interrupts: interrupt for the device > - phy: See ethernet.txt file in the same directory. > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > index ce5f7f9cff06..aceb977b104d 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > @@ -3179,6 +3179,7 @@ static int mvneta_remove(struct platform_device *pdev) > > static const struct of_device_id mvneta_match[] = { > { .compatible = "marvell,armada-370-neta" }, > + { .compatible = "marvell,armada-xp-neta" }, > { } > }; > MODULE_DEVICE_TABLE(of, mvneta_match); >
On 17/06/2015 17:12, Gregory CLEMENT wrote: > Hi Simon, > > On 17/06/2015 15:19, Simon Guinot wrote: >> The mvneta driver supports the Ethernet IP found in the Armada 370, XP, >> 380 and 385 SoCs. Since at least one more hardware feature is available >> for the Armada XP SoCs then a way to identify them is needed. >> >> This patch introduces a new compatible string "marvell,armada-xp-neta". > > Let's be future proof by going further. I would like to have an compatible string > for each SoC even if we currently we don't use them. > > You even don't have to use add it in the mvneta.c file if you use: > "marvell,armada-380-neta", "marvell,armada-xp-neta" Actually I meant: "marvell,armada-380-neta", "marvell,armada-370-neta" > > > Thanks, > > Gregory > > >> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> >> Cc: <stable@vger.kernel.org> # v3.8+ >> --- >> Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt | 2 +- >> drivers/net/ethernet/marvell/mvneta.c | 1 + >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt >> index 750d577e8083..f5a8ca29aff0 100644 >> --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt >> +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt >> @@ -1,7 +1,7 @@ >> * Marvell Armada 370 / Armada XP Ethernet Controller (NETA) >> >> Required properties: >> -- compatible: should be "marvell,armada-370-neta". >> +- compatible: "marvell,armada-370-neta" or "marvell,armada-xp-neta". >> - reg: address and length of the register set for the device. >> - interrupts: interrupt for the device >> - phy: See ethernet.txt file in the same directory. >> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c >> index ce5f7f9cff06..aceb977b104d 100644 >> --- a/drivers/net/ethernet/marvell/mvneta.c >> +++ b/drivers/net/ethernet/marvell/mvneta.c >> @@ -3179,6 +3179,7 @@ static int mvneta_remove(struct platform_device *pdev) >> >> static const struct of_device_id mvneta_match[] = { >> { .compatible = "marvell,armada-370-neta" }, >> + { .compatible = "marvell,armada-xp-neta" }, >> { } >> }; >> MODULE_DEVICE_TABLE(of, mvneta_match); >> > >
Hi Gregory, On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote: > On 17/06/2015 17:12, Gregory CLEMENT wrote: > > On 17/06/2015 15:19, Simon Guinot wrote: > >> The mvneta driver supports the Ethernet IP found in the Armada 370, XP, > >> 380 and 385 SoCs. Since at least one more hardware feature is available > >> for the Armada XP SoCs then a way to identify them is needed. > >> > >> This patch introduces a new compatible string "marvell,armada-xp-neta". > > > > Let's be future proof by going further. I would like to have an compatible string > > for each SoC even if we currently we don't use them. I disagree with this. We can't predict what incosistencies we'll discover in the future. We should only assign new compatible strings based on known IP variations when we discover them. This seems fraught with demons since we can't predict the scope of affected IP blocks (some steppings of one SoC, three SoCs plus two steppings of a fourth, etc) imho, the 'future-proofing' lies in being specific as to the naming of the compatible strings against known hardware variations at the time. thx, Jason.
Jason, On Wed, 17 Jun 2015 17:01:12 +0000, Jason Cooper wrote: > I disagree with this. We can't predict what incosistencies we'll discover in > the future. We should only assign new compatible strings based on known IP > variations when we discover them. This seems fraught with demons since we > can't predict the scope of affected IP blocks (some steppings of one SoC, three > SoCs plus two steppings of a fourth, etc) > > imho, the 'future-proofing' lies in being specific as to the naming of the > compatible strings against known hardware variations at the time. Except that this clearly doesn't work, and the case raised by Simon is a perfect illustration of why planning ahead is beneficial. We already had the issue several times on mvebu platforms, so it should really become the rule to have one compatible string specific to the actual SoC in the list of compatible strings. Not doing so requires breaking DT backward compatibility more often, so wanting DT backward compatibility and not wanting to plan ahead is a bit antagonist. But I personally don't care much about DT backward compatibility, and I've explained numerous times why, so in the end I don't really care much. Best regards, Thomas
Hey Thomas, On Wed, Jun 17, 2015 at 10:43:12PM +0200, Thomas Petazzoni wrote: > On Wed, 17 Jun 2015 17:01:12 +0000, Jason Cooper wrote: > > > I disagree with this. We can't predict what incosistencies we'll discover in > > the future. We should only assign new compatible strings based on known IP > > variations when we discover them. This seems fraught with demons since we > > can't predict the scope of affected IP blocks (some steppings of one SoC, three > > SoCs plus two steppings of a fourth, etc) > > > > imho, the 'future-proofing' lies in being specific as to the naming of the > > compatible strings against known hardware variations at the time. > > Except that this clearly doesn't work, and the case raised by Simon is > a perfect illustration of why planning ahead is beneficial. Odd, I'd use that as an example of the process working. ;-) we have everyone using 'armada-370-neta' for a given block. We discovered that the original IP block (on the 370s) had a limitation (no hw checksum for greater than 1600 bytes). A newer version of the IP block (XP) doesn't have the limitation. So we change the driver to honor the limit for the 370 compatible string. We create a new compatible string for xp where the block doesn't have the limitation. How did the process fail? > We already had the issue several times on mvebu platforms, so it > should really become the rule to have one compatible string specific > to the actual SoC in the list of compatible strings. Sorry, I'm just not a fan of guessing. But I'll fall back to the DT maintainers on this one. if they are ok with it, then I'll drop my objection. > Not doing so requires breaking DT backward compatibility more often, so > wanting DT backward compatibility and not wanting to plan ahead is a > bit antagonist. I'm not seeing where backwards compatibility was broken? A device with an old dtb booting a newer kernel gets a bugfix. In the case of an XP board with an old dtb (armada-370-neta), the hardware still works, but not optimally. Upgrading the dtb will enable hw checksumming for jumbo packets. thx, Jason.
Dear Jason Cooper, On Wed, 17 Jun 2015 21:39:26 +0000, Jason Cooper wrote: > Odd, I'd use that as an example of the process working. ;-) we have > everyone using 'armada-370-neta' for a given block. We discovered that > the original IP block (on the 370s) had a limitation (no hw checksum > for greater than 1600 bytes). A newer version of the IP block (XP) > doesn't have the limitation. > > So we change the driver to honor the limit for the 370 compatible > string. We create a new compatible string for xp where the block > doesn't have the limitation. > > How did the process fail? Because now all Armada XP users of jumbo frames are looking the HW checksum on their jumbo frames, which you can consider to be a regression: it was working, it is no longer working. Of course, since it falls back to SW checksumming, it still "works", but some users can complain of the performance penalty and consider it to be a regression. If on Armada XP, we had used for the beginning: compatible = "marvell,armada-xp-neta", "marvell,armada-370-neta" with only marvell,armada-370-neta supported originally, we could have added this fix without breaking HW checksumming on jumbo frames for Armada XP users. So I'm sorry, but the process indeed failed, because Armada XP users keeping their old Device Tree blob will see a regression. > I'm not seeing where backwards compatibility was broken? A device with > an old dtb booting a newer kernel gets a bugfix. In the case of an XP > board with an old dtb (armada-370-neta), the hardware still works, but > not optimally. Upgrading the dtb will enable hw checksumming for jumbo > packets. "not optimally" is still a breakage. Again, I personally don't care about DT backward compatibility as I think it's a stupid requirement. But I like to point out to the DT backward compatibility fanatics when it was actually broken :-) Best regards, Thomas
On Wed, Jun 17, 2015 at 05:01:12PM +0000, Jason Cooper wrote: > Hi Gregory, > > On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote: > > On 17/06/2015 17:12, Gregory CLEMENT wrote: > > > On 17/06/2015 15:19, Simon Guinot wrote: > > >> The mvneta driver supports the Ethernet IP found in the Armada 370, XP, > > >> 380 and 385 SoCs. Since at least one more hardware feature is available > > >> for the Armada XP SoCs then a way to identify them is needed. > > >> > > >> This patch introduces a new compatible string "marvell,armada-xp-neta". > > > > > > Let's be future proof by going further. I would like to have an compatible string > > > for each SoC even if we currently we don't use them. > > I disagree with this. We can't predict what incosistencies we'll discover in > the future. We should only assign new compatible strings based on known IP > variations when we discover them. This seems fraught with demons since we > can't predict the scope of affected IP blocks (some steppings of one SoC, three > SoCs plus two steppings of a fourth, etc) > > imho, the 'future-proofing' lies in being specific as to the naming of the > compatible strings against known hardware variations at the time. So, should I add more compatible strings or not ? Simon
On Fri, Jun 19, 2015 at 02:32:53PM +0200, Simon Guinot wrote: > On Wed, Jun 17, 2015 at 05:01:12PM +0000, Jason Cooper wrote: > > Hi Gregory, > > > > On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote: > > > On 17/06/2015 17:12, Gregory CLEMENT wrote: > > > > On 17/06/2015 15:19, Simon Guinot wrote: > > > >> The mvneta driver supports the Ethernet IP found in the Armada 370, XP, > > > >> 380 and 385 SoCs. Since at least one more hardware feature is available > > > >> for the Armada XP SoCs then a way to identify them is needed. > > > >> > > > >> This patch introduces a new compatible string "marvell,armada-xp-neta". > > > > > > > > Let's be future proof by going further. I would like to have an compatible string > > > > for each SoC even if we currently we don't use them. > > > > I disagree with this. We can't predict what incosistencies we'll discover in > > the future. We should only assign new compatible strings based on known IP > > variations when we discover them. This seems fraught with demons since we > > can't predict the scope of affected IP blocks (some steppings of one SoC, three > > SoCs plus two steppings of a fourth, etc) > > > > imho, the 'future-proofing' lies in being specific as to the naming of the > > compatible strings against known hardware variations at the time. > > So, should I add more compatible strings or not ? Hi Gregory and Jason, How do you want me to handle this ? Did you reach an agreement ? Thanks, Simon
On Thu, Jun 25, 2015 at 11:13:23AM +0200, Simon Guinot wrote: > On Fri, Jun 19, 2015 at 02:32:53PM +0200, Simon Guinot wrote: > > On Wed, Jun 17, 2015 at 05:01:12PM +0000, Jason Cooper wrote: > > > On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote: > > > > On 17/06/2015 17:12, Gregory CLEMENT wrote: > > > > > On 17/06/2015 15:19, Simon Guinot wrote: > > > > >> The mvneta driver supports the Ethernet IP found in the Armada 370, XP, > > > > >> 380 and 385 SoCs. Since at least one more hardware feature is available > > > > >> for the Armada XP SoCs then a way to identify them is needed. > > > > >> > > > > >> This patch introduces a new compatible string "marvell,armada-xp-neta". > > > > > > > > > > Let's be future proof by going further. I would like to have an compatible string > > > > > for each SoC even if we currently we don't use them. > > > > > > I disagree with this. We can't predict what incosistencies we'll discover in > > > the future. We should only assign new compatible strings based on known IP > > > variations when we discover them. This seems fraught with demons since we > > > can't predict the scope of affected IP blocks (some steppings of one SoC, three > > > SoCs plus two steppings of a fourth, etc) > > > > > > imho, the 'future-proofing' lies in being specific as to the naming of the > > > compatible strings against known hardware variations at the time. > > > > So, should I add more compatible strings or not ? > > How do you want me to handle this ? Did you reach an agreement ? Sorry, this slipped off my radar. Probably EBKAC. :) I'm still of the opinion that future-proofing equates to guessing. It has the advantage of, if we guess correctly, things are easier down the road when we discover differences between similar IP blocks. However, if we guess incorrectly, then we have a mess on our hands. iow, this proposal fails poorly. I've no problem breaking DT compatibility when it's determined that we made a mistake (or mistakes) in the past. See the irqchip rework that Marc did a few cycles ago. The difference here is that we know better. We *know* that dtbs are upgraded with the kernel. We *know* that no one is shipping products with dtbs in ROMs. So what are we really trying to protect against? thx, Jason.
Hi Jason, Simon, On 25/06/2015 15:20, Jason Cooper wrote: > On Thu, Jun 25, 2015 at 11:13:23AM +0200, Simon Guinot wrote: >> On Fri, Jun 19, 2015 at 02:32:53PM +0200, Simon Guinot wrote: >>> On Wed, Jun 17, 2015 at 05:01:12PM +0000, Jason Cooper wrote: >>>> On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote: >>>>> On 17/06/2015 17:12, Gregory CLEMENT wrote: >>>>>> On 17/06/2015 15:19, Simon Guinot wrote: >>>>>>> The mvneta driver supports the Ethernet IP found in the Armada 370, XP, >>>>>>> 380 and 385 SoCs. Since at least one more hardware feature is available >>>>>>> for the Armada XP SoCs then a way to identify them is needed. >>>>>>> >>>>>>> This patch introduces a new compatible string "marvell,armada-xp-neta". >>>>>> >>>>>> Let's be future proof by going further. I would like to have an compatible string >>>>>> for each SoC even if we currently we don't use them. >>>> >>>> I disagree with this. We can't predict what incosistencies we'll discover in >>>> the future. We should only assign new compatible strings based on known IP >>>> variations when we discover them. This seems fraught with demons since we >>>> can't predict the scope of affected IP blocks (some steppings of one SoC, three >>>> SoCs plus two steppings of a fourth, etc) >>>> >>>> imho, the 'future-proofing' lies in being specific as to the naming of the >>>> compatible strings against known hardware variations at the time. >>> >>> So, should I add more compatible strings or not ? >> >> How do you want me to handle this ? Did you reach an agreement ? > > Sorry, this slipped off my radar. Probably EBKAC. :) > > I'm still of the opinion that future-proofing equates to guessing. > It has the advantage of, if we guess correctly, things are easier down > the road when we discover differences between similar IP blocks. > However, if we guess incorrectly, then we have a mess on our hands. > iow, this proposal fails poorly. > > I've no problem breaking DT compatibility when it's determined that we > made a mistake (or mistakes) in the past. See the irqchip rework that > Marc did a few cycles ago. > > The difference here is that we know better. We *know* that dtbs are > upgraded with the kernel. We *know* that no one is shipping products > with dtbs in ROMs. So what are we really trying to protect against? Ok then, if we don't have to ensure backward compatibility, then the patch is fine as is. Thanks, Gregory
Hi Simon, On 17/06/2015 15:19, Simon Guinot wrote: > The mvneta driver supports the Ethernet IP found in the Armada 370, XP, > 380 and 385 SoCs. Since at least one more hardware feature is available > for the Armada XP SoCs then a way to identify them is needed. > > This patch introduces a new compatible string "marvell,armada-xp-neta". > > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > Cc: <stable@vger.kernel.org> # v3.8+ Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Thanks, Gregory > --- > Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt | 2 +- > drivers/net/ethernet/marvell/mvneta.c | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > index 750d577e8083..f5a8ca29aff0 100644 > --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > @@ -1,7 +1,7 @@ > * Marvell Armada 370 / Armada XP Ethernet Controller (NETA) > > Required properties: > -- compatible: should be "marvell,armada-370-neta". > +- compatible: "marvell,armada-370-neta" or "marvell,armada-xp-neta". > - reg: address and length of the register set for the device. > - interrupts: interrupt for the device > - phy: See ethernet.txt file in the same directory. > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > index ce5f7f9cff06..aceb977b104d 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > @@ -3179,6 +3179,7 @@ static int mvneta_remove(struct platform_device *pdev) > > static const struct of_device_id mvneta_match[] = { > { .compatible = "marvell,armada-370-neta" }, > + { .compatible = "marvell,armada-xp-neta" }, > { } > }; > MODULE_DEVICE_TABLE(of, mvneta_match); >
diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt index 750d577e8083..f5a8ca29aff0 100644 --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt @@ -1,7 +1,7 @@ * Marvell Armada 370 / Armada XP Ethernet Controller (NETA) Required properties: -- compatible: should be "marvell,armada-370-neta". +- compatible: "marvell,armada-370-neta" or "marvell,armada-xp-neta". - reg: address and length of the register set for the device. - interrupts: interrupt for the device - phy: See ethernet.txt file in the same directory. diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index ce5f7f9cff06..aceb977b104d 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -3179,6 +3179,7 @@ static int mvneta_remove(struct platform_device *pdev) static const struct of_device_id mvneta_match[] = { { .compatible = "marvell,armada-370-neta" }, + { .compatible = "marvell,armada-xp-neta" }, { } }; MODULE_DEVICE_TABLE(of, mvneta_match);
The mvneta driver supports the Ethernet IP found in the Armada 370, XP, 380 and 385 SoCs. Since at least one more hardware feature is available for the Armada XP SoCs then a way to identify them is needed. This patch introduces a new compatible string "marvell,armada-xp-neta". Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> Cc: <stable@vger.kernel.org> # v3.8+ --- Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt | 2 +- drivers/net/ethernet/marvell/mvneta.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)