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);
/* PHY_INTERFACE_MODE_RGMII_TXID means TX internal delay, make
* sure that we enable the port TX clock internal delay to
@@ -1375,7 +1374,10 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
if (interface == PHY_INTERFACE_MODE_RGMII)
rgmii_ctrl |= RGMII_CTRL_DLL_TXC | RGMII_CTRL_DLL_RXC;
- rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
+
+ if (dev->chip_id != BCM53115_DEVICE_ID)
+ rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
+
b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
dev_info(ds->dev, "Configured port %d for %s\n", port,
--
2.43.0On 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?
>
> /* PHY_INTERFACE_MODE_RGMII_TXID means TX internal delay, make
> * sure that we enable the port TX clock internal delay to
> @@ -1375,7 +1374,10 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
> rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> if (interface == PHY_INTERFACE_MODE_RGMII)
> rgmii_ctrl |= RGMII_CTRL_DLL_TXC | RGMII_CTRL_DLL_RXC;
> - rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
> +
> + if (dev->chip_id != BCM53115_DEVICE_ID)
> + rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
> +
> b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
>
> dev_info(ds->dev, "Configured port %d for %s\n", port,
--
Florian
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.
Regards,
Jonas
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.
/P
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
© 2016 - 2025 Red Hat, Inc.