Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Florian Fainelli <florian.fainelli@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4216 bytes --]

On 5/19/25 12:44, Jonas Gorski wrote:
> On Mon, May 19, 2025 at 9:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Mon, May 19, 2025 at 07:45:49PM +0200, Jonas Gorski wrote:
>>> The RGMII delay type of the PHY interface is intended for the PHY, not
>>> the MAC, so we need to configure the opposite. Else we double the delay
>>> or don't add one at all if the PHY also supports configuring delays.
>>>
>>> Additionally, we need to enable RGMII_CTRL_TIMING_SEL for the delay
>>> actually being effective.
>>>
>>> Fixes e.g. BCM54612E connected on RGMII ports that also configures RGMII
>>> delays in its driver.
>>
>> We have to be careful here not to cause regressions. It might be
>> wrong, but are there systems using this which actually work? Does this
>> change break them?
> 
> The only user (of bcm63xx and b53 dsa) I am aware of is OpenWrt, and
> we are capable of updating our dts files in case they were using
> broken configuration. Though having PHYs on the RGMII ports is a very
> rare configuration, and usually there is switch connected with a fixed
> link, so likely the issue was never detected.
> 
>>>
>>> Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>>> ---
>>>   drivers/net/dsa/b53/b53_common.c | 13 +++++++------
>>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>> index a316f8c01d0a..b00975189dab 100644
>>> --- a/drivers/net/dsa/b53/b53_common.c
>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>> @@ -1328,19 +1328,19 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
>>>
>>>        switch (interface) {
>>>        case PHY_INTERFACE_MODE_RGMII_ID:
>>> -             rgmii_ctrl |= (RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>> +             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>>                break;
>>>        case PHY_INTERFACE_MODE_RGMII_RXID:
>>> -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_TXC);
>>> -             rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
>>> +             rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
>>> +             rgmii_ctrl &= ~RGMII_CTRL_DLL_RXC;
>>>                break;
>>>        case PHY_INTERFACE_MODE_RGMII_TXID:
>>> -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC);
>>> -             rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
>>> +             rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
>>> +             rgmii_ctrl &= ~RGMII_CTRL_DLL_TXC;
>>>                break;
>>>        case PHY_INTERFACE_MODE_RGMII:
>>>        default:
>>> -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>> +             rgmii_ctrl |= RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC;
>>>                break;
>>
>> These changes look wrong. There is more background here:
>>
>> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287
> 
> This is what makes it work for me (I tested all four modes, rgmii,
> rgmii-id, rgmii-txid and rgmii-rxid). Without this change, b53 will
> configure the same delays on the MAC layer as the PHY driver (bcm54xx,
> https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/phy/broadcom.c#L73
> ), which breaks connectivity at least for me.
> 
> E.g. with a phy-mode of "rgmii-id", both b53 and the PHY driver would
> enable rx and tx delays, causing the delays to be 4 ns instead of 2
> ns. So I don't see how this could have ever worked.

It's possible this was tested with an external PHY which was not 
configured by drivers/net/phy/broadcom.c?

> 
> Also note that b53_adjust_531x5_rgmii()
> https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/dsa/b53/b53_common.c#L1360
> already behaves that way, this just makes bcm63xx now work the same
> (so these functions could now even be merged).

Yes, I was going to suggest we should be doing that.

There are precedents with other Broadcom drivers for doing things 
incorrectly, and having to put quirks to deal with that, yours truly 
having greatly contributed to doing that:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b972b54a68b2512a7528658ecd023aea108c03a5
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4208 bytes --]