From: Paul Geurts <paul.geurts@prodrive-technologies.com>
Add support for the BCM53134 Ethernet switch in the existing b53 dsa driver.
BCM53134 is very similar to the BCM58XX series.
Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
drivers/net/dsa/b53/b53_common.c | 53 +++++++++++++++++++++++++++++++-
drivers/net/dsa/b53/b53_mdio.c | 5 ++-
drivers/net/dsa/b53/b53_priv.h | 9 +++++-
3 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 1f9b251a5452..aaa0813e6f59 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
if (is63xx(dev) && port >= B53_63XX_RGMII0)
b53_adjust_63xx_rgmii(ds, port, phydev->interface);
+ if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
+ if (port == dev->imp_port)
+ off = B53_RGMII_CTRL_IMP;
+ else
+ off = B53_RGMII_CTRL_P(port);
+
+ /* Configure the port RGMII clock delay by DLL disabled and
+ * 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);
+
+ /* PHY_INTERFACE_MODE_RGMII_TXID means TX internal delay, make
+ * sure that we enable the port TX clock internal delay to
+ * account for this internal delay that is inserted, otherwise
+ * the switch won't be able to receive correctly.
+ *
+ * PHY_INTERFACE_MODE_RGMII means that we are not introducing
+ * any delay neither on transmission nor reception, so the
+ * BCM53134 must also be configured accordingly to account for
+ * the lack of delay and introduce
+ *
+ * The BCM53134 switch has its RX clock and TX clock control
+ * swapped, hence the reason why we modify the TX clock path in
+ * the "RGMII" case
+ */
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+ rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
+ rgmii_ctrl |= RGMII_CTRL_DLL_TXC | RGMII_CTRL_DLL_RXC;
+ b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
+
+ dev_info(ds->dev, "Configured port %d for %s\n", port,
+ phy_modes(phydev->interface));
+ }
+
if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
if (port == dev->imp_port)
off = B53_RGMII_CTRL_IMP;
@@ -2613,6 +2649,20 @@ static const struct b53_chip_data b53_switch_chips[] = {
.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
},
+ {
+ .chip_id = BCM53134_DEVICE_ID,
+ .dev_name = "BCM53134",
+ .vlans = 4096,
+ .enabled_ports = 0x12f,
+ .imp_port = 8,
+ .cpu_port = B53_CPU_PORT,
+ .vta_regs = B53_VTA_REGS,
+ .arl_bins = 4,
+ .arl_buckets = 1024,
+ .duplex_reg = B53_DUPLEX_STAT_GE,
+ .jumbo_pm_reg = B53_JUMBO_PORT_MASK,
+ .jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+ },
};
static int b53_switch_init(struct b53_device *dev)
@@ -2671,7 +2721,7 @@ static int b53_switch_init(struct b53_device *dev)
dev->ds->num_ports = min_t(unsigned int, dev->num_ports, DSA_MAX_PORTS);
/* Include non standard CPU port built-in PHYs to be probed */
- if (is539x(dev) || is531x5(dev)) {
+ if (is539x(dev) || is531x5(dev) || is53134(dev)) {
for (i = 0; i < dev->num_ports; i++) {
if (!(dev->ds->phys_mii_mask & BIT(i)) &&
!b53_possible_cpu_port(dev->ds, i))
@@ -2790,6 +2840,7 @@ int b53_switch_detect(struct b53_device *dev)
case BCM53012_DEVICE_ID:
case BCM53018_DEVICE_ID:
case BCM53019_DEVICE_ID:
+ case BCM53134_DEVICE_ID:
dev->chip_id = id32;
break;
default:
diff --git a/drivers/net/dsa/b53/b53_mdio.c b/drivers/net/dsa/b53/b53_mdio.c
index 6ddc03b58b28..8b422b298cd5 100644
--- a/drivers/net/dsa/b53/b53_mdio.c
+++ b/drivers/net/dsa/b53/b53_mdio.c
@@ -286,6 +286,7 @@ static const struct b53_io_ops b53_mdio_ops = {
#define B53_BRCM_OUI_2 0x03625c00
#define B53_BRCM_OUI_3 0x00406000
#define B53_BRCM_OUI_4 0x01410c00
+#define B53_BRCM_OUI_5 0xae025000
static int b53_mdio_probe(struct mdio_device *mdiodev)
{
@@ -313,7 +314,8 @@ static int b53_mdio_probe(struct mdio_device *mdiodev)
if ((phy_id & 0xfffffc00) != B53_BRCM_OUI_1 &&
(phy_id & 0xfffffc00) != B53_BRCM_OUI_2 &&
(phy_id & 0xfffffc00) != B53_BRCM_OUI_3 &&
- (phy_id & 0xfffffc00) != B53_BRCM_OUI_4) {
+ (phy_id & 0xfffffc00) != B53_BRCM_OUI_4 &&
+ (phy_id & 0xfffffc00) != B53_BRCM_OUI_5) {
dev_err(&mdiodev->dev, "Unsupported device: 0x%08x\n", phy_id);
return -ENODEV;
}
@@ -375,6 +377,7 @@ static const struct of_device_id b53_of_match[] = {
{ .compatible = "brcm,bcm53115" },
{ .compatible = "brcm,bcm53125" },
{ .compatible = "brcm,bcm53128" },
+ { .compatible = "brcm,bcm53134" },
{ .compatible = "brcm,bcm5365" },
{ .compatible = "brcm,bcm5389" },
{ .compatible = "brcm,bcm5395" },
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index a689a6950189..7888be1fd23f 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -80,6 +80,7 @@ enum {
BCM583XX_DEVICE_ID = 0x58300,
BCM7445_DEVICE_ID = 0x7445,
BCM7278_DEVICE_ID = 0x7278,
+ BCM53134_DEVICE_ID = 0x5075,
};
struct b53_pcs {
@@ -215,7 +216,13 @@ static inline int is58xx(struct b53_device *dev)
return dev->chip_id == BCM58XX_DEVICE_ID ||
dev->chip_id == BCM583XX_DEVICE_ID ||
dev->chip_id == BCM7445_DEVICE_ID ||
- dev->chip_id == BCM7278_DEVICE_ID;
+ dev->chip_id == BCM7278_DEVICE_ID ||
+ dev->chip_id == BCM53134_DEVICE_ID;
+}
+
+static inline int is53134(struct b53_device *dev)
+{
+ return dev->chip_id == BCM53134_DEVICE_ID;
}
#define B53_63XX_RGMII0 4
--
2.30.2
On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
> From: Paul Geurts <paul.geurts@prodrive-technologies.com>
>
> Add support for the BCM53134 Ethernet switch in the existing b53 dsa driver.
> BCM53134 is very similar to the BCM58XX series.
>
> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
> drivers/net/dsa/b53/b53_common.c | 53 +++++++++++++++++++++++++++++++-
> drivers/net/dsa/b53/b53_mdio.c | 5 ++-
> drivers/net/dsa/b53/b53_priv.h | 9 +++++-
> 3 files changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 1f9b251a5452..aaa0813e6f59 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
> if (is63xx(dev) && port >= B53_63XX_RGMII0)
> b53_adjust_63xx_rgmii(ds, port, phydev->interface);
>
> + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
Why is not this in the same code block as the one for the is531x5()
device like this:
diff --git a/drivers/net/dsa/b53/b53_common.c
b/drivers/net/dsa/b53/b53_common.c
index 59cdfc51ce06..1c64b6ce7e78 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch *ds,
int port,
tx_pause, rx_pause);
b53_force_link(dev, port, phydev->link);
- if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
+ if ((is531x5(dev) || is53134(dev)) &&
phy_interface_is_rgmii(phydev)) {
if (port == dev->imp_port)
off = B53_RGMII_CTRL_IMP;
else
Other than that, LGTM!
--
Florian
> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: donderdag 23 maart 2023 17:43
> To: Álvaro Fernández Rojas <noltari@gmail.com>; Paul Geurts
> <paul.geurts@prodrive-technologies.com>; jonas.gorski@gmail.com;
> andrew@lunn.ch; olteanv@gmail.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for BCM53134
>
> On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
> > From: Paul Geurts <paul.geurts@prodrive-technologies.com>
> >
> > Add support for the BCM53134 Ethernet switch in the existing b53 dsa
> driver.
> > BCM53134 is very similar to the BCM58XX series.
> >
> > Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > ---
> > drivers/net/dsa/b53/b53_common.c | 53
> +++++++++++++++++++++++++++++++-
> > drivers/net/dsa/b53/b53_mdio.c | 5 ++-
> > drivers/net/dsa/b53/b53_priv.h | 9 +++++-
> > 3 files changed, 64 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/dsa/b53/b53_common.c
> > b/drivers/net/dsa/b53/b53_common.c
> > index 1f9b251a5452..aaa0813e6f59 100644
> > --- a/drivers/net/dsa/b53/b53_common.c
> > +++ b/drivers/net/dsa/b53/b53_common.c
> > @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch
> *ds, int port,
> > if (is63xx(dev) && port >= B53_63XX_RGMII0)
> > b53_adjust_63xx_rgmii(ds, port, phydev->interface);
> >
> > + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
>
> Why is not this in the same code block as the one for the is531x5() device like
> this:
>
> diff --git a/drivers/net/dsa/b53/b53_common.c
> b/drivers/net/dsa/b53/b53_common.c
> index 59cdfc51ce06..1c64b6ce7e78 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch *ds,
> int port,
> tx_pause, rx_pause);
> b53_force_link(dev, port, phydev->link);
>
> - if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
> + if ((is531x5(dev) || is53134(dev)) &&
> phy_interface_is_rgmii(phydev)) {
> if (port == dev->imp_port)
> off = B53_RGMII_CTRL_IMP;
> else
>
> Other than that, LGTM!
> --
> Florian
I think the only reason is that the BCM53134 does not support the
RGMII_CTRL_TIMING_SEL bit, which is set in the original block. I agree
Putting a if statement around
rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
would prevent a lot of code duplication. _however_, after looking at it again,
I don’t think the device does not support the bit. When looking at the datasheet,
The same bit in the this register is called BYPASS_2NS_DEL. It's very uncommon
For Broadcom to make such a change in the register interface, so maybe they
Just renamed it. Do you think this could be the same bit?
---
Paul
El jue, 23 mar 2023 a las 21:10, Paul Geurts
(<paul.geurts@prodrive-technologies.com>) escribió:
>
> > -----Original Message-----
> > From: Florian Fainelli <f.fainelli@gmail.com>
> > Sent: donderdag 23 maart 2023 17:43
> > To: Álvaro Fernández Rojas <noltari@gmail.com>; Paul Geurts
> > <paul.geurts@prodrive-technologies.com>; jonas.gorski@gmail.com;
> > andrew@lunn.ch; olteanv@gmail.com; davem@davemloft.net;
> > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for BCM53134
> >
> > On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
> > > From: Paul Geurts <paul.geurts@prodrive-technologies.com>
> > >
> > > Add support for the BCM53134 Ethernet switch in the existing b53 dsa
> > driver.
> > > BCM53134 is very similar to the BCM58XX series.
> > >
> > > Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > > ---
> > > drivers/net/dsa/b53/b53_common.c | 53
> > +++++++++++++++++++++++++++++++-
> > > drivers/net/dsa/b53/b53_mdio.c | 5 ++-
> > > drivers/net/dsa/b53/b53_priv.h | 9 +++++-
> > > 3 files changed, 64 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/dsa/b53/b53_common.c
> > > b/drivers/net/dsa/b53/b53_common.c
> > > index 1f9b251a5452..aaa0813e6f59 100644
> > > --- a/drivers/net/dsa/b53/b53_common.c
> > > +++ b/drivers/net/dsa/b53/b53_common.c
> > > @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch
> > *ds, int port,
> > > if (is63xx(dev) && port >= B53_63XX_RGMII0)
> > > b53_adjust_63xx_rgmii(ds, port, phydev->interface);
> > >
> > > + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
> >
> > Why is not this in the same code block as the one for the is531x5() device like
> > this:
> >
> > diff --git a/drivers/net/dsa/b53/b53_common.c
> > b/drivers/net/dsa/b53/b53_common.c
> > index 59cdfc51ce06..1c64b6ce7e78 100644
> > --- a/drivers/net/dsa/b53/b53_common.c
> > +++ b/drivers/net/dsa/b53/b53_common.c
> > @@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch *ds,
> > int port,
> > tx_pause, rx_pause);
> > b53_force_link(dev, port, phydev->link);
> >
> > - if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
> > + if ((is531x5(dev) || is53134(dev)) &&
> > phy_interface_is_rgmii(phydev)) {
> > if (port == dev->imp_port)
> > off = B53_RGMII_CTRL_IMP;
> > else
> >
> > Other than that, LGTM!
> > --
> > Florian
>
> I think the only reason is that the BCM53134 does not support the
> RGMII_CTRL_TIMING_SEL bit, which is set in the original block. I agree
> Putting a if statement around
> rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
> would prevent a lot of code duplication. _however_, after looking at it again,
> I don’t think the device does not support the bit. When looking at the datasheet,
> The same bit in the this register is called BYPASS_2NS_DEL. It's very uncommon
> For Broadcom to make such a change in the register interface, so maybe they
> Just renamed it. Do you think this could be the same bit?
What happens if you add that bit on your device? It doesn't work?
I will try with my device and see what happens when I add it...
>
> ---
> Paul
--
Álvaro
On 3/23/23 13:10, Paul Geurts wrote:
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: donderdag 23 maart 2023 17:43
>> To: Álvaro Fernández Rojas <noltari@gmail.com>; Paul Geurts
>> <paul.geurts@prodrive-technologies.com>; jonas.gorski@gmail.com;
>> andrew@lunn.ch; olteanv@gmail.com; davem@davemloft.net;
>> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for BCM53134
>>
>> On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
>>> From: Paul Geurts <paul.geurts@prodrive-technologies.com>
>>>
>>> Add support for the BCM53134 Ethernet switch in the existing b53 dsa
>> driver.
>>> BCM53134 is very similar to the BCM58XX series.
>>>
>>> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> ---
>>> drivers/net/dsa/b53/b53_common.c | 53
>> +++++++++++++++++++++++++++++++-
>>> drivers/net/dsa/b53/b53_mdio.c | 5 ++-
>>> drivers/net/dsa/b53/b53_priv.h | 9 +++++-
>>> 3 files changed, 64 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/b53/b53_common.c
>>> b/drivers/net/dsa/b53/b53_common.c
>>> index 1f9b251a5452..aaa0813e6f59 100644
>>> --- a/drivers/net/dsa/b53/b53_common.c
>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>> @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch
>> *ds, int port,
>>> if (is63xx(dev) && port >= B53_63XX_RGMII0)
>>> b53_adjust_63xx_rgmii(ds, port, phydev->interface);
>>>
>>> + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
>>
>> Why is not this in the same code block as the one for the is531x5() device like
>> this:
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c
>> b/drivers/net/dsa/b53/b53_common.c
>> index 59cdfc51ce06..1c64b6ce7e78 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch *ds,
>> int port,
>> tx_pause, rx_pause);
>> b53_force_link(dev, port, phydev->link);
>>
>> - if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
>> + if ((is531x5(dev) || is53134(dev)) &&
>> phy_interface_is_rgmii(phydev)) {
>> if (port == dev->imp_port)
>> off = B53_RGMII_CTRL_IMP;
>> else
>>
>> Other than that, LGTM!
>> --
>> Florian
>
> I think the only reason is that the BCM53134 does not support the
> RGMII_CTRL_TIMING_SEL bit, which is set in the original block. I agree
> Putting a if statement around
> rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
> would prevent a lot of code duplication. _however_, after looking at it again,
> I don’t think the device does not support the bit. When looking at the datasheet,
> The same bit in the this register is called BYPASS_2NS_DEL. It's very uncommon
> For Broadcom to make such a change in the register interface, so maybe they
> Just renamed it. Do you think this could be the same bit?
Yes, I think this is exactly the same bit, just named differently. What
strikes me as odd is that neither of the 53115, 53125 or 53128 which are
guarded by the is531x5() conditional have it defined.
--
Florian
El jue, 23 mar 2023 a las 22:02, Florian Fainelli
(<f.fainelli@gmail.com>) escribió:
>
> On 3/23/23 13:10, Paul Geurts wrote:
> >> -----Original Message-----
> >> From: Florian Fainelli <f.fainelli@gmail.com>
> >> Sent: donderdag 23 maart 2023 17:43
> >> To: Álvaro Fernández Rojas <noltari@gmail.com>; Paul Geurts
> >> <paul.geurts@prodrive-technologies.com>; jonas.gorski@gmail.com;
> >> andrew@lunn.ch; olteanv@gmail.com; davem@davemloft.net;
> >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> >> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> >> kernel@vger.kernel.org
> >> Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for BCM53134
> >>
> >> On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
> >>> From: Paul Geurts <paul.geurts@prodrive-technologies.com>
> >>>
> >>> Add support for the BCM53134 Ethernet switch in the existing b53 dsa
> >> driver.
> >>> BCM53134 is very similar to the BCM58XX series.
> >>>
> >>> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> >>> ---
> >>> drivers/net/dsa/b53/b53_common.c | 53
> >> +++++++++++++++++++++++++++++++-
> >>> drivers/net/dsa/b53/b53_mdio.c | 5 ++-
> >>> drivers/net/dsa/b53/b53_priv.h | 9 +++++-
> >>> 3 files changed, 64 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/dsa/b53/b53_common.c
> >>> b/drivers/net/dsa/b53/b53_common.c
> >>> index 1f9b251a5452..aaa0813e6f59 100644
> >>> --- a/drivers/net/dsa/b53/b53_common.c
> >>> +++ b/drivers/net/dsa/b53/b53_common.c
> >>> @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch
> >> *ds, int port,
> >>> if (is63xx(dev) && port >= B53_63XX_RGMII0)
> >>> b53_adjust_63xx_rgmii(ds, port, phydev->interface);
> >>>
> >>> + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
> >>
> >> Why is not this in the same code block as the one for the is531x5() device like
> >> this:
> >>
> >> diff --git a/drivers/net/dsa/b53/b53_common.c
> >> b/drivers/net/dsa/b53/b53_common.c
> >> index 59cdfc51ce06..1c64b6ce7e78 100644
> >> --- a/drivers/net/dsa/b53/b53_common.c
> >> +++ b/drivers/net/dsa/b53/b53_common.c
> >> @@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch *ds,
> >> int port,
> >> tx_pause, rx_pause);
> >> b53_force_link(dev, port, phydev->link);
> >>
> >> - if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
> >> + if ((is531x5(dev) || is53134(dev)) &&
> >> phy_interface_is_rgmii(phydev)) {
> >> if (port == dev->imp_port)
> >> off = B53_RGMII_CTRL_IMP;
> >> else
> >>
> >> Other than that, LGTM!
> >> --
> >> Florian
> >
> > I think the only reason is that the BCM53134 does not support the
> > RGMII_CTRL_TIMING_SEL bit, which is set in the original block. I agree
> > Putting a if statement around
> > rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
> > would prevent a lot of code duplication. _however_, after looking at it again,
> > I don’t think the device does not support the bit. When looking at the datasheet,
> > The same bit in the this register is called BYPASS_2NS_DEL. It's very uncommon
> > For Broadcom to make such a change in the register interface, so maybe they
> > Just renamed it. Do you think this could be the same bit?
>
> Yes, I think this is exactly the same bit, just named differently. What
> strikes me as odd is that neither of the 53115, 53125 or 53128 which are
> guarded by the is531x5() conditional have it defined.
If this is true then we can safely add 53134 to is531x5() conditional
and reuse the existing code.
> --
> Florian
>
--
Álvaro
> -----Original Message-----
> From: Álvaro Fernández Rojas <noltari@gmail.com>
> Sent: donderdag 23 maart 2023 23:13
> To: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Paul Geurts <paul.geurts@prodrive-technologies.com>;
> jonas.gorski@gmail.com; andrew@lunn.ch; olteanv@gmail.com;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for BCM53134
>
> El jue, 23 mar 2023 a las 22:02, Florian Fainelli
> (<f.fainelli@gmail.com>) escribió:
> >
> > On 3/23/23 13:10, Paul Geurts wrote:
> > >> -----Original Message-----
> > >> From: Florian Fainelli <f.fainelli@gmail.com>
> > >> Sent: donderdag 23 maart 2023 17:43
> > >> To: Álvaro Fernández Rojas <noltari@gmail.com>; Paul Geurts
> > >> <paul.geurts@prodrive-technologies.com>; jonas.gorski@gmail.com;
> > >> andrew@lunn.ch; olteanv@gmail.com; davem@davemloft.net;
> > >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > >> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > >> kernel@vger.kernel.org
> > >> Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for
> > >> BCM53134
> > >>
> > >> On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
> > >>> From: Paul Geurts <paul.geurts@prodrive-technologies.com>
> > >>>
> > >>> Add support for the BCM53134 Ethernet switch in the existing b53
> > >>> dsa
> > >> driver.
> > >>> BCM53134 is very similar to the BCM58XX series.
> > >>>
> > >>> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> > >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > >>> ---
> > >>> drivers/net/dsa/b53/b53_common.c | 53
> > >> +++++++++++++++++++++++++++++++-
> > >>> drivers/net/dsa/b53/b53_mdio.c | 5 ++-
> > >>> drivers/net/dsa/b53/b53_priv.h | 9 +++++-
> > >>> 3 files changed, 64 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/drivers/net/dsa/b53/b53_common.c
> > >>> b/drivers/net/dsa/b53/b53_common.c
> > >>> index 1f9b251a5452..aaa0813e6f59 100644
> > >>> --- a/drivers/net/dsa/b53/b53_common.c
> > >>> +++ b/drivers/net/dsa/b53/b53_common.c
> > >>> @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct
> > >>> dsa_switch
> > >> *ds, int port,
> > >>> if (is63xx(dev) && port >= B53_63XX_RGMII0)
> > >>> b53_adjust_63xx_rgmii(ds, port, phydev->interface);
> > >>>
> > >>> + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
> > >>
> > >> Why is not this in the same code block as the one for the is531x5()
> > >> device like
> > >> this:
> > >>
> > >> diff --git a/drivers/net/dsa/b53/b53_common.c
> > >> b/drivers/net/dsa/b53/b53_common.c
> > >> index 59cdfc51ce06..1c64b6ce7e78 100644
> > >> --- a/drivers/net/dsa/b53/b53_common.c
> > >> +++ b/drivers/net/dsa/b53/b53_common.c
> > >> @@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch
> > >> *ds, int port,
> > >> tx_pause, rx_pause);
> > >> b53_force_link(dev, port, phydev->link);
> > >>
> > >> - if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
> > >> + if ((is531x5(dev) || is53134(dev)) &&
> > >> phy_interface_is_rgmii(phydev)) {
> > >> if (port == dev->imp_port)
> > >> off = B53_RGMII_CTRL_IMP;
> > >> else
> > >>
> > >> Other than that, LGTM!
> > >> --
> > >> Florian
> > >
> > > I think the only reason is that the BCM53134 does not support the
> > > RGMII_CTRL_TIMING_SEL bit, which is set in the original block. I
> > > agree Putting a if statement around rgmii_ctrl |=
> > > RGMII_CTRL_TIMING_SEL; would prevent a lot of code duplication.
> > > _however_, after looking at it again, I don’t think the device does
> > > not support the bit. When looking at the datasheet, The same bit in
> > > the this register is called BYPASS_2NS_DEL. It's very uncommon For
> > > Broadcom to make such a change in the register interface, so maybe
> > > they Just renamed it. Do you think this could be the same bit?
> >
> > Yes, I think this is exactly the same bit, just named differently.
> > What strikes me as odd is that neither of the 53115, 53125 or 53128
> > which are guarded by the is531x5() conditional have it defined.
>
> If this is true then we can safely add 53134 to is531x5() conditional and reuse
> the existing code.
>
> > --
> > Florian
> >
>
> --
> Álvaro
With the bit set on my device, everything keeps working just fine. I indeed think
This is just the same bit. I have requested some clarity from our FAE at
Broadcom. We could wait for their answer (which could take a while), or just
assume setting this bit is fine an push the patches. I'll leave that up to you.
---
Paul
El vie, 24 mar 2023 a las 9:29, Paul Geurts
(<paul.geurts@prodrive-technologies.com>) escribió:
>
> > -----Original Message-----
> > From: Álvaro Fernández Rojas <noltari@gmail.com>
> > Sent: donderdag 23 maart 2023 23:13
> > To: Florian Fainelli <f.fainelli@gmail.com>
> > Cc: Paul Geurts <paul.geurts@prodrive-technologies.com>;
> > jonas.gorski@gmail.com; andrew@lunn.ch; olteanv@gmail.com;
> > davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; robh+dt@kernel.org;
> > krzysztof.kozlowski+dt@linaro.org; netdev@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for BCM53134
> >
> > El jue, 23 mar 2023 a las 22:02, Florian Fainelli
> > (<f.fainelli@gmail.com>) escribió:
> > >
> > > On 3/23/23 13:10, Paul Geurts wrote:
> > > >> -----Original Message-----
> > > >> From: Florian Fainelli <f.fainelli@gmail.com>
> > > >> Sent: donderdag 23 maart 2023 17:43
> > > >> To: Álvaro Fernández Rojas <noltari@gmail.com>; Paul Geurts
> > > >> <paul.geurts@prodrive-technologies.com>; jonas.gorski@gmail.com;
> > > >> andrew@lunn.ch; olteanv@gmail.com; davem@davemloft.net;
> > > >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > > >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > >> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > >> kernel@vger.kernel.org
> > > >> Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for
> > > >> BCM53134
> > > >>
> > > >> On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
> > > >>> From: Paul Geurts <paul.geurts@prodrive-technologies.com>
> > > >>>
> > > >>> Add support for the BCM53134 Ethernet switch in the existing b53
> > > >>> dsa
> > > >> driver.
> > > >>> BCM53134 is very similar to the BCM58XX series.
> > > >>>
> > > >>> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> > > >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > > >>> ---
> > > >>> drivers/net/dsa/b53/b53_common.c | 53
> > > >> +++++++++++++++++++++++++++++++-
> > > >>> drivers/net/dsa/b53/b53_mdio.c | 5 ++-
> > > >>> drivers/net/dsa/b53/b53_priv.h | 9 +++++-
> > > >>> 3 files changed, 64 insertions(+), 3 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/net/dsa/b53/b53_common.c
> > > >>> b/drivers/net/dsa/b53/b53_common.c
> > > >>> index 1f9b251a5452..aaa0813e6f59 100644
> > > >>> --- a/drivers/net/dsa/b53/b53_common.c
> > > >>> +++ b/drivers/net/dsa/b53/b53_common.c
> > > >>> @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct
> > > >>> dsa_switch
> > > >> *ds, int port,
> > > >>> if (is63xx(dev) && port >= B53_63XX_RGMII0)
> > > >>> b53_adjust_63xx_rgmii(ds, port, phydev->interface);
> > > >>>
> > > >>> + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
> > > >>
> > > >> Why is not this in the same code block as the one for the is531x5()
> > > >> device like
> > > >> this:
> > > >>
> > > >> diff --git a/drivers/net/dsa/b53/b53_common.c
> > > >> b/drivers/net/dsa/b53/b53_common.c
> > > >> index 59cdfc51ce06..1c64b6ce7e78 100644
> > > >> --- a/drivers/net/dsa/b53/b53_common.c
> > > >> +++ b/drivers/net/dsa/b53/b53_common.c
> > > >> @@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch
> > > >> *ds, int port,
> > > >> tx_pause, rx_pause);
> > > >> b53_force_link(dev, port, phydev->link);
> > > >>
> > > >> - if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
> > > >> + if ((is531x5(dev) || is53134(dev)) &&
> > > >> phy_interface_is_rgmii(phydev)) {
> > > >> if (port == dev->imp_port)
> > > >> off = B53_RGMII_CTRL_IMP;
> > > >> else
> > > >>
> > > >> Other than that, LGTM!
> > > >> --
> > > >> Florian
> > > >
> > > > I think the only reason is that the BCM53134 does not support the
> > > > RGMII_CTRL_TIMING_SEL bit, which is set in the original block. I
> > > > agree Putting a if statement around rgmii_ctrl |=
> > > > RGMII_CTRL_TIMING_SEL; would prevent a lot of code duplication.
> > > > _however_, after looking at it again, I don’t think the device does
> > > > not support the bit. When looking at the datasheet, The same bit in
> > > > the this register is called BYPASS_2NS_DEL. It's very uncommon For
> > > > Broadcom to make such a change in the register interface, so maybe
> > > > they Just renamed it. Do you think this could be the same bit?
> > >
> > > Yes, I think this is exactly the same bit, just named differently.
> > > What strikes me as odd is that neither of the 53115, 53125 or 53128
> > > which are guarded by the is531x5() conditional have it defined.
> >
> > If this is true then we can safely add 53134 to is531x5() conditional and reuse
> > the existing code.
> >
> > > --
> > > Florian
> > >
> >
> > --
> > Álvaro
>
> With the bit set on my device, everything keeps working just fine. I indeed think
> This is just the same bit. I have requested some clarity from our FAE at
> Broadcom. We could wait for their answer (which could take a while), or just
> assume setting this bit is fine an push the patches. I'll leave that up to you.
I've checked and mine works fine too, so I simplified the patch in v2.
>
> ---
> Paul
--
Álvaro
El jue, 23 mar 2023 a las 17:43, Florian Fainelli
(<f.fainelli@gmail.com>) escribió:
>
> On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
> > From: Paul Geurts <paul.geurts@prodrive-technologies.com>
> >
> > Add support for the BCM53134 Ethernet switch in the existing b53 dsa driver.
> > BCM53134 is very similar to the BCM58XX series.
> >
> > Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > ---
> > drivers/net/dsa/b53/b53_common.c | 53 +++++++++++++++++++++++++++++++-
> > drivers/net/dsa/b53/b53_mdio.c | 5 ++-
> > drivers/net/dsa/b53/b53_priv.h | 9 +++++-
> > 3 files changed, 64 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> > index 1f9b251a5452..aaa0813e6f59 100644
> > --- a/drivers/net/dsa/b53/b53_common.c
> > +++ b/drivers/net/dsa/b53/b53_common.c
> > @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
> > if (is63xx(dev) && port >= B53_63XX_RGMII0)
> > b53_adjust_63xx_rgmii(ds, port, phydev->interface);
> >
> > + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
>
> Why is not this in the same code block as the one for the is531x5()
> device like this:
This is what I asked on the cover letter:
> I also added a separate RGMII handling block for is53134() since according to
> Paul, BCM53134 doesn't support RGMII_CTRL_TIMING_SEL as opposed to is531x5().
Should I add it to the same block and avoid adding
RGMII_CTRL_TIMING_SEL if is53134() or is it compatible with
RGMII_CTRL_TIMING_SEL?
>
> diff --git a/drivers/net/dsa/b53/b53_common.c
> b/drivers/net/dsa/b53/b53_common.c
> index 59cdfc51ce06..1c64b6ce7e78 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch *ds,
> int port,
> tx_pause, rx_pause);
> b53_force_link(dev, port, phydev->link);
>
> - if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
> + if ((is531x5(dev) || is53134(dev)) &&
> phy_interface_is_rgmii(phydev)) {
> if (port == dev->imp_port)
> off = B53_RGMII_CTRL_IMP;
> else
>
> Other than that, LGTM!
> --
> Florian
>
--
Álvaro.
On 3/23/23 09:49, Álvaro Fernández Rojas wrote:
> El jue, 23 mar 2023 a las 17:43, Florian Fainelli
> (<f.fainelli@gmail.com>) escribió:
>>
>> On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
>>> From: Paul Geurts <paul.geurts@prodrive-technologies.com>
>>>
>>> Add support for the BCM53134 Ethernet switch in the existing b53 dsa driver.
>>> BCM53134 is very similar to the BCM58XX series.
>>>
>>> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> ---
>>> drivers/net/dsa/b53/b53_common.c | 53 +++++++++++++++++++++++++++++++-
>>> drivers/net/dsa/b53/b53_mdio.c | 5 ++-
>>> drivers/net/dsa/b53/b53_priv.h | 9 +++++-
>>> 3 files changed, 64 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>> index 1f9b251a5452..aaa0813e6f59 100644
>>> --- a/drivers/net/dsa/b53/b53_common.c
>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>> @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
>>> if (is63xx(dev) && port >= B53_63XX_RGMII0)
>>> b53_adjust_63xx_rgmii(ds, port, phydev->interface);
>>>
>>> + if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
>>
>> Why is not this in the same code block as the one for the is531x5()
>> device like this:
>
> This is what I asked on the cover letter:
>> I also added a separate RGMII handling block for is53134() since according to
>> Paul, BCM53134 doesn't support RGMII_CTRL_TIMING_SEL as opposed to is531x5().
>
> Should I add it to the same block and avoid adding
> RGMII_CTRL_TIMING_SEL if is53134() or is it compatible with
> RGMII_CTRL_TIMING_SEL?
RGMII_CTRL_TIMING_SEL is not defined for the 53125 or 53128, and for
53115, the register 0x60 is not even defined in the datasheet.
RGMII_CTRL_TIMING_SEL is defined for the 53134 however and would control
how the two other bits behave with respect to RGMII timing. My guess is
that if we removed it entirely we would not be breaking anything, and we
could just support all of the 531x5() and 53134 families within the same
block.
Jonas, do you remember why setting RGMII_CTRL_TIMING_SEL might have been
necessary?
--
Florian
© 2016 - 2026 Red Hat, Inc.