diff mbox series

[4/4] net: dsa: b53: add BCM63268 RGMII configuration

Message ID 20230320155024.164523-5-noltari@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: b53: configure BCM63268 RGMII ports | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Álvaro Fernández Rojas March 20, 2023, 3:50 p.m. UTC
BCM63268 requires special RGMII configuration to work.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 6 +++++-
 drivers/net/dsa/b53/b53_regs.h   | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

Comments

Florian Fainelli March 20, 2023, 8 p.m. UTC | #1
On 3/20/23 08:50, Álvaro Fernández Rojas wrote:
> BCM63268 requires special RGMII configuration to work.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>   drivers/net/dsa/b53/b53_common.c | 6 +++++-
>   drivers/net/dsa/b53/b53_regs.h   | 1 +
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 6e212f6f1cb9..d0a22c8a55c9 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1240,8 +1240,12 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
>   		break;
>   	}
>   
> -	if (port != dev->imp_port)
> +	if (port != dev->imp_port) {
> +		if (is63268(dev))
> +			rgmii_ctrl |= RGMII_CTRL_MII_OVERRIDE;

AFAICT the override bit is defined and valid for both 63268 and 6318, 
essentially whenever more than one RGMII control register for port 4, 
but also for other ports, it seems like the bit becomes valid. The 
comment I have says that the override bit ensures that what is populated 
in bits 5:4 which is the actual RGMII interface mode is applied. That 
mode can be one of:

0b00: RGMII mode
0b01: MII mode
0b10: RVMII mode
0b11: GMII mode

even though this is not documented as such, I suspect that the override 
bit does not only set the mode, but also ensures that the delays are 
also applied.

Once you update patch 3, this LGTM and you may add:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

For your v2. Thanks!
Simon Horman March 20, 2023, 8:07 p.m. UTC | #2
On Mon, Mar 20, 2023 at 04:50:24PM +0100, Álvaro Fernández Rojas wrote:
> BCM63268 requires special RGMII configuration to work.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Álvaro Fernández Rojas March 21, 2023, 9:10 a.m. UTC | #3
(Excuse me for my previous email in HTML, but I forgot Android Gmail
app uses HTML)

2023-03-20 21:00 GMT+01:00, Florian Fainelli <f.fainelli@gmail.com>:
> On 3/20/23 08:50, Álvaro Fernández Rojas wrote:
>> BCM63268 requires special RGMII configuration to work.
>>
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>>   drivers/net/dsa/b53/b53_common.c | 6 +++++-
>>   drivers/net/dsa/b53/b53_regs.h   | 1 +
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c
>> b/drivers/net/dsa/b53/b53_common.c
>> index 6e212f6f1cb9..d0a22c8a55c9 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1240,8 +1240,12 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch
>> *ds, int port,
>>   		break;
>>   	}
>>
>> -	if (port != dev->imp_port)
>> +	if (port != dev->imp_port) {
>> +		if (is63268(dev))
>> +			rgmii_ctrl |= RGMII_CTRL_MII_OVERRIDE;
>
> AFAICT the override bit is defined and valid for both 63268 and 6318,

Should we add a specific ID for the 6318?
I don't know if it's different enough from the 63268 to need a special
treatment such as is6318()...

> essentially whenever more than one RGMII control register for port 4,
> but also for other ports, it seems like the bit becomes valid. The
> comment I have says that the override bit ensures that what is populated
> in bits 5:4 which is the actual RGMII interface mode is applied. That
> mode can be one of:
>
> 0b00: RGMII mode
> 0b01: MII mode
> 0b10: RVMII mode
> 0b11: GMII mode

This is interesting since we never set those bits in the bcm63xx
enetsw controller...
Should we add configuration for those bits in future patches?
Which SoCs hace those bits? Only 6318 and 63268 or every 63xx has them?

>
> even though this is not documented as such, I suspect that the override
> bit does not only set the mode, but also ensures that the delays are
> also applied.
>
> Once you update patch 3, this LGTM and you may add:
>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>
> For your v2. Thanks!
> --
> Florian
>
>

--
Álvaro
diff mbox series

Patch

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 6e212f6f1cb9..d0a22c8a55c9 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1240,8 +1240,12 @@  static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
 		break;
 	}
 
-	if (port != dev->imp_port)
+	if (port != dev->imp_port) {
+		if (is63268(dev))
+			rgmii_ctrl |= RGMII_CTRL_MII_OVERRIDE;
+
 		rgmii_ctrl |= RGMII_CTRL_ENABLE_GMII;
+	}
 
 	b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
 
diff --git a/drivers/net/dsa/b53/b53_regs.h b/drivers/net/dsa/b53/b53_regs.h
index b2c539a42154..bfbcb66bef66 100644
--- a/drivers/net/dsa/b53/b53_regs.h
+++ b/drivers/net/dsa/b53/b53_regs.h
@@ -138,6 +138,7 @@ 
 
 #define B53_RGMII_CTRL_IMP		0x60
 #define   RGMII_CTRL_ENABLE_GMII	BIT(7)
+#define   RGMII_CTRL_MII_OVERRIDE	BIT(6)
 #define   RGMII_CTRL_TIMING_SEL		BIT(2)
 #define   RGMII_CTRL_DLL_RXC		BIT(1)
 #define   RGMII_CTRL_DLL_TXC		BIT(0)