Re: [PATCH net v2 5/5] net: dsa: b53: do not touch DLL_IQQD on bcm53115

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

On 6/5/25 02:06, Paolo Abeni wrote:
> On 6/3/25 8:15 AM, Jonas Gorski wrote:
>> On Mon, Jun 2, 2025 at 11:40 PM Florian Fainelli
>> <florian.fainelli@broadcom.com> wrote:
>>> On 6/2/25 12:39, Jonas Gorski wrote:
>>>> According to OpenMDK, bit 2 of the RGMII register has a different
>>>> meaning for BCM53115 [1]:
>>>>
>>>> "DLL_IQQD         1: In the IDDQ mode, power is down0: Normal function
>>>>                     mode"
>>>>
>>>> Configuring RGMII delay works without setting this bit, so let's keep it
>>>> at the default. For other chips, we always set it, so not clearing it
>>>> is not an issue.
>>>>
>>>> One would assume BCM53118 works the same, but OpenMDK is not quite sure
>>>> what this bit actually means [2]:
>>>>
>>>> "BYPASS_IMP_2NS_DEL #1: In the IDDQ mode, power is down#0: Normal
>>>>                       function mode1: Bypass dll65_2ns_del IP0: Use
>>>>                       dll65_2ns_del IP"
>>>>
>>>> So lets keep setting it for now.
>>>>
>>>> [1] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53115/bcm53115_a0_defs.h#L19871
>>>> [2] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53118/bcm53118_a0_defs.h#L14392
>>>>
>>>> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>>>> ---
>>>> v1 -> v2:
>>>> * new patch
>>>>
>>>>    drivers/net/dsa/b53/b53_common.c | 8 +++++---
>>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>>> index be4493b769f4..862bdccb7439 100644
>>>> --- a/drivers/net/dsa/b53/b53_common.c
>>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>>> @@ -1354,8 +1354,7 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
>>>>         * tx_clk aligned timing (restoring to reset defaults)
>>>>         */
>>>>        b53_read8(dev, B53_CTRL_PAGE, off, &rgmii_ctrl);
>>>> -     rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC |
>>>> -                     RGMII_CTRL_TIMING_SEL);
>>>> +     rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>>
>>> Are not we missing a:
>>>
>>> if (dev->chip_id != BCM53115_DEVICE_ID)
>>>          rgmii_ctrl &= ~RGMII_CTRL_TIMING_SEL;
>>>
>>> here to be strictly identical before/after?
>>
>> We could add it for symmetry, but it would be purely decorational. We
>> unconditionally set this bit again later, so clearing it before has no
>> actual effect, which is why I didn't add it.
> 
> Makes sense, and the code in this patch is IMHO more readable.

Agreed, thanks for taking those patches.
-- 
Florian