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

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

[Jeroen Hofstee]

cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") broke
the ethernet networking on the beaglebone enhanced.

The board relied on the bug in the at803x driver to always enable the rx
delay. So change the phy-mode to rgmii-id so it is enabled again.

Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
cc: Koen Kooi <koen@dominion.thruhere.net>
---
 arch/arm/boot/dts/am335x-sancloud-bbe.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am335x-sancloud-bbe.dts b/arch/arm/boot/dts/am335x-sancloud-bbe.dts
index 8678e6e35493..e5fdb7abb0d5 100644
--- a/arch/arm/boot/dts/am335x-sancloud-bbe.dts
+++ b/arch/arm/boot/dts/am335x-sancloud-bbe.dts
@@ -108,7 +108,7 @@
 
 &cpsw_emac0 {
 	phy-handle = <&ethphy0>;
-	phy-mode = "rgmii-txid";
+	phy-mode = "rgmii-id";
 };
 
 &i2c0 {
-- 
2.17.1

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

[Grygorii Strashko]

On 02/10/2019 12:54, Jeroen Hofstee wrote:
> cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") broke
> the ethernet networking on the beaglebone enhanced.

Above commit is incorrect (by itself) and there are few more commits on top of
it, so pls. update reference to commit(s)

bb0ce4c1517d net: phy: at803x: stop switching phy delay config needlessly
6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode


> 
> The board relied on the bug in the at803x driver to always enable the rx
> delay. So change the phy-mode to rgmii-id so it is enabled again.
> 
> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
> cc: Koen Kooi <koen@dominion.thruhere.net>
> ---
>   arch/arm/boot/dts/am335x-sancloud-bbe.dts | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/am335x-sancloud-bbe.dts b/arch/arm/boot/dts/am335x-sancloud-bbe.dts
> index 8678e6e35493..e5fdb7abb0d5 100644
> --- a/arch/arm/boot/dts/am335x-sancloud-bbe.dts
> +++ b/arch/arm/boot/dts/am335x-sancloud-bbe.dts
> @@ -108,7 +108,7 @@
>   
>   &cpsw_emac0 {
>   	phy-handle = <&ethphy0>;
> -	phy-mode = "rgmii-txid";
> +	phy-mode = "rgmii-id";
>   };
>   
>   &i2c0 {
> 

-- 
Best regards,
grygorii

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

[Jeroen Hofstee]

Hello Grygorri,

On 10/2/19 4:48 PM, Grygorii Strashko wrote:
>
>
> On 02/10/2019 12:54, Jeroen Hofstee wrote:
>> cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") 
>> broke
>> the ethernet networking on the beaglebone enhanced.
>
> Above commit is incorrect (by itself) and there are few more commits 
> on top of
> it, so pls. update reference to commit(s)
>
> bb0ce4c1517d net: phy: at803x: stop switching phy delay config needlessly
> 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode
>
>
I don't see why that is relevant. The mention patch introduces a
backwards incompatibility for the device tree. The patches you
mention don't fix that and hence are unrelated to this patch.

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.

Regards,

Jeroen

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

[Grygorii Strashko]

On 03/10/2019 11:16, Jeroen Hofstee wrote:
> Hello Grygorri,
> 
> On 10/2/19 4:48 PM, Grygorii Strashko wrote:
>>
>>
>> On 02/10/2019 12:54, Jeroen Hofstee wrote:
>>> cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode")
>>> broke
>>> the ethernet networking on the beaglebone enhanced.
>>
>> Above commit is incorrect (by itself) and there are few more commits
>> on top of
>> it, so pls. update reference to commit(s)
>>
>> bb0ce4c1517d net: phy: at803x: stop switching phy delay config needlessly
>> 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode
>>
>>
> I don't see why that is relevant. The mention patch introduces a
> backwards incompatibility for the device tree. 

Pls read https://patchwork.kernel.org/patch/10773389/
The patch you've mentioned here is buggy by itself and not related to your
fix, but final at803x behavior actually defined by above two commits.

I've posted this commit because I was confused when i've checked commit you referenced.

The patches you
> mention don't fix that and hence are unrelated to this patch.

No. but they define new at803x behavior which is:
After commits (see above) at803x driver disable RX RGMII delay
if phy-mode = "rgmii-txid"
or will disable TX RGMII delay
if phy-mode = "rgmii-rxid"

Before above commits, the at803x driver was keeping RX or TX RGMII delay setting
untouched as per bootloader or bootstraping configuration for "rgmii-txid"/"rgmii-rxid".

> 
> 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

-- 
Best regards,
grygorii

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

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [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?

Regards,

Tony

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

[Jeroen Hofstee]

Hello Tony,

On 10/8/19 4:23 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [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?


Not at the moment. I am right that the mentioned commit
is the first one to break the ethernet. Grygorii is right it
seems that that commit shouldn't affect it, yet it does.

So I would like to understand how it breaks things so I can
up with a sensible commit message (or we just drop reference
to other commits so I don't have to dig through the 5.1 history,
the patch by itself is also valid).

Regards,

Jeroen

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

[Jeroen Hofstee]

Hello Tony,

On 10/8/19 4:23 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [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.

Regards,

Jeroen

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

[Tony Lindgren]

* Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:03]:
> Hello Tony,
> 
> On 10/8/19 4:23 PM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.strashko@ti.com> [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 :)

Regards,

Tony

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

[Jeroen Hofstee]

Hello Tony,

On 10/8/19 6:14 PM, Tony Lindgren wrote:
> * Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:03]:
>> Hello Tony,
>>
>> On 10/8/19 4:23 PM, Tony Lindgren wrote:
>>> * Grygorii Strashko <grygorii.strashko@ti.com> [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...

Regards,

Jeroen

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

[Tony Lindgren]

* Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:43]:
> Hello Tony,
> 
> On 10/8/19 6:14 PM, Tony Lindgren wrote:
> > * Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:03]:
> >> Hello Tony,
> >>
> >> On 10/8/19 4:23 PM, Tony Lindgren wrote:
> >>> * Grygorii Strashko <grygorii.strashko@ti.com> [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?

Regards,

Tony

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

[Jeroen Hofstee]

Hi,

On 10/8/19 6:51 PM, Tony Lindgren wrote:
> * Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:43]:
>> Hello Tony,
>>
>> On 10/8/19 6:14 PM, Tony Lindgren wrote:
>>> * Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:03]:
>>>> Hello Tony,
>>>>
>>>> On 10/8/19 4:23 PM, Tony Lindgren wrote:
>>>>> * Grygorii Strashko <grygorii.strashko@ti.com> [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...

Regards,
Jeroen

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

[Tony Lindgren]

* Jeroen Hofstee <jhofstee@victronenergy.com> [191008 17:00]:
> Hi,
> 
> On 10/8/19 6:51 PM, Tony Lindgren wrote:
> > * Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:43]:
> >> Hello Tony,
> >>
> >> On 10/8/19 6:14 PM, Tony Lindgren wrote:
> >>> * Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:03]:
> >>>> Hello Tony,
> >>>>
> >>>> On 10/8/19 4:23 PM, Tony Lindgren wrote:
> >>>>> * Grygorii Strashko <grygorii.strashko@ti.com> [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 :)

Cheers,

Tony

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@victronenergy.com> [191008 17:00]:
>> Hi,
>>
>> On 10/8/19 6:51 PM, Tony Lindgren wrote:
>>> * Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:43]:
>>>> Hello Tony,
>>>>
>>>> On 10/8/19 6:14 PM, Tony Lindgren wrote:
>>>>> * Jeroen Hofstee <jhofstee@victronenergy.com> [191008 16:03]:
>>>>>> Hello Tony,
>>>>>>
>>>>>> On 10/8/19 4:23 PM, Tony Lindgren wrote:
>>>>>>> * Grygorii Strashko <grygorii.strashko@ti.com> [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

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

[Jeroen Hofstee]

Hello Grygorri,

On 10/9/19 11:43 AM, Grygorii Strashko wrote:
>
>
>>> 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: "
>
>

Yes, that part is clear to me, and I am not saying you are incorrect,
only that I would like to understand why above commit pops up
(since it makes no sense to me as well, my own commit). And the reason
is rather silly, I guess... I fixed it on master, thereafter checked the 
5.1 branch,
while keeping the fixed dtb... :(

Let me check that and I will send a v2. Likely tomorrow
(I am not near the device at the moment).

Regards,

Jeroen