Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet

[Grygorii Strashko]

On 08/10/2019 20:02, Tony Lindgren wrote:
> * Jeroen Hofstee <jhofstee@xxxxxxxxxxxxxxxxx> [191008 17:00]:
> > Hi,
> >
> > On 10/8/19 6:51 PM, Tony Lindgren wrote:
> > > * Jeroen Hofstee <jhofstee@xxxxxxxxxxxxxxxxx> [191008 16:43]:
> > > > Hello Tony,
> > > >
> > > > On 10/8/19 6:14 PM, Tony Lindgren wrote:
> > > > > * Jeroen Hofstee <jhofstee@xxxxxxxxxxxxxxxxx> [191008 16:03]:
> > > > > > Hello Tony,
> > > > > >
> > > > > > On 10/8/19 4:23 PM, Tony Lindgren wrote:
> > > > > > > * Grygorii Strashko <grygorii.strashko@xxxxxx> [191003 02:32]:
> > > > > > > > On 03/10/2019 11:16, Jeroen Hofstee wrote:
> > > > > > > > > Furthermore 4.19 is fine, so there is no need to include it in stable
> > > > > > > > > and have a note to make sure also other patches are required etc.
> > > > > > > > Hence all above patches went in 5.1 it would be correct to mention only
> > > > > > > > 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode
> > > > > > > Jeroen, can you please send an updated patch with the fixes
> > > > > > > tag changed?
> > > > > > For completeness, there is no "Fixes tag" as you mentioned.
> > > > > > The commit only refers to another commit which introduces
> > > > > > a problem.
> > > > > Well please add the fixes tag, that way this will get
> > > > > properly applied to earlier stable kernels too :)
> > > > But 4.19 is fine, this is an issue in 5.1 as in EOL...
> > > > I really don't understand why I should waste time
> > > > to figure out what happened exactly during the 5.1
> > > > release cycle...
> > > Hmm so what's the issue with just adding the fixes tag Grygorii
> > > suggested:
> > >
> > > 6d4cd041f0af ("net: phy: at803x: disable delay only for RGMII mode")
> > >
> > > No need to dig further?
> >
> > Grygorii doesn't suggest to add a fixes tag, just to change the referenced
> > commit to another. Obviously I would like to understand why another commit
> > should be referenced. And then I should read and parse the response, so there
> > is no special reason, just time...
>
> OK sure. Well once you guys have the commit figured out, let me
> know what to apply. And we know Grygorii is mostly right based
> on his history of comments so best to not ignore that :)

Sry, but I do not think my request is somehow special.
Yes, your patch is correct by itself, but commit description is not:
1) commit cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode")  which you've mentioned is A BUG
and should not be merged first of all (which you can find out by reading corresponding thread).

just try checkout that commit and apply your patch on top - networking should not work.

But it was merged and not reverted - instead two more patches were applied to fix regression.

2) Those commits are defined final behavior (which i again explained above) and that new behavior hardly can
be called "the bug in the at803x driver" as, unfortunately, there were no common conclusion how default values for
RX/TX delay should be handled vs phy-mode = "rgmii-txid"/"rgmii-rxid".
Originally many PHY driver kept them default (as per boot strapping/bootloader configuration), but now
some driver (including at803x) started disabling RX delay if "rgmii-txid" or TX delay if "rgmii-rxid".

Hence, pls update commit message and add proper fixes tag. smth like:
"Now after commit 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode the driver will forcibly disable
RGMII RX delay if phy-mode = "rgmii-txid" is specified in DT which will break networking on ..

Hence change .. to ensure ...
Fixes: "


--
Best regards,
grygorii