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

[Jeroen Hofstee <jhofstee@victronenergy.com>]

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