Re: [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar

On 3/12/21 2:01 AM, Rafał Miłecki wrote:

On 11.03.2021 23:13, Florian Fainelli wrote:

quoted

On 3/11/21 2:04 PM, Rafał Miłecki wrote:

quoted

On 10.03.2021 18:19, Florian Fainelli wrote:

quoted

On 3/10/21 3:59 AM, Rafał Miłecki wrote:

quoted

From: Rafał Miłecki <rafal@milecki.pl>

On some SoCs (e.g. BCM4908, BCM631[345]8) SF2 has an integrated
crossbar. It allows connecting its selected external ports to
internal ports. It's used by vendors to handle custom Ethernet
setups.

BCM4908 has following 3x2 crossbar. On Asus GT-AC5300 rgmii is
used for connecting external BCM53134S switch. GPHY4 is usually
used for WAN port. More fancy devices use SerDes for 2.5 Gbps
Ethernet.

┌──────────┐ SerDes ─── 0 ─┤          │ │   3x2    ├─ 0 ───
switch port 7 GPHY4 ─── 1 ─┤          │ │ crossbar ├─ 1 ───
runner (accelerator) rgmii ─── 2 ─┤          │ └──────────┘

Use setup data based on DT info to configure BCM4908's switch
port 7. Right now only GPHY and rgmii variants are supported.
Handling SerDes can be implemented later.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl> ---
drivers/net/dsa/bcm_sf2.c      | 41
++++++++++++++++++++++++++++++++++ drivers/net/dsa/bcm_sf2.h
|  1 + drivers/net/dsa/bcm_sf2_regs.h |  7 ++++++ 3 files
changed, 49 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c

b/drivers/net/dsa/bcm_sf2.c index 8f50e91d4004..b4b36408f069
100644 --- a/drivers/net/dsa/bcm_sf2.c +++
b/drivers/net/dsa/bcm_sf2.c @@ -432,6 +432,40 @@ static int
bcm_sf2_sw_rst(struct bcm_sf2_priv *priv) return 0; } +static
void bcm_sf2_crossbar_setup(struct bcm_sf2_priv *priv) +{ +
struct device *dev = priv->dev->ds->dev; +    int shift; +    u32
mask; +    u32 reg; +    int i; + +    reg = 0;

I believe you need to do a read/modify/write here otherwise you
are clobbering the other settings for the p_wan_link_status and
p_wan_link_sel bits.

Thanks, I didn't know about those bits.

quoted

quoted

+    switch (priv->type) { +    case BCM4908_DEVICE_ID: +
shift = CROSSBAR_BCM4908_INT_P7 * priv->num_crossbar_int_ports; +
if (priv->int_phy_mask & BIT(7)) +            reg |=
CROSSBAR_BCM4908_EXT_GPHY4 << shift; +        else if (0) /*
FIXME */ +            reg |= CROSSBAR_BCM4908_EXT_SERDES <<
shift; +        else

Maybe what you can do is change bcm_sf2_identify_ports() such that
when the 'phy-interface' property is retrieved from Device Tree, we
also store the 'mode' variable into the per-port structure
(bcm_sf2_port_status) and when you call bcm_sf2_crossbar_setup()
for each port that has been setup, and you update the logic to look
like this:

if (priv->int_phy_mask & BIT(7)) reg |= CROSSBAR_BCM4908_EXT_GPHY4
<< shift; else if (phy_interface_mode_is_rgmii(mode)) reg |=
CROSSBAR_BCM4908_EXT_RGMII

and we add support for SerDes when we get to that point. This would
also allow you to detect if an invalid configuration is specified
via Device Tree.

Sounds great, but I experienced a problem while trying to implement
that.

On Asus GT-AC5300 I have:

/* External BCM53134S switch */ port@7 { label = "sw"; reg = <7>;

fixed-link { speed = <1000>; full-duplex; }; };

after adding phy-mode = "rgmii"; to it, my PHYs stop working because
of SF2.

bcm_sf2_sw_mac_link_up() calls: bcm_sf2_sw_mac_link_set(ds, 7,
PHY_INTERFACE_MODE_RGMII, true); which results in setting
RGMII_MODE_EN bit in the REG_RGMII_CNTRL_P(7).

For some reason setting above bit results in stopping internal PHYs.
unimac_mdio_read() starts getting MDIO_READ_FAIL.

Do you have any idea why it happens?

RGMII_MODE_EN enables the RGMII data pad, but this usually has no
incidence on the MDIO which is separate, unless there is something I do
not understand about how the crossbar works.

REG_RGMII_CNTRL_P() macro usage is broken. It can be called with
arguments 0, 1 and 2 only.

Oh that's right, yes, none of the chips I have had access to had more
than 3 RGMII ports.

For port 7, REG_RGMII_CNTRL_P(7) returns offset exceeding array size
and causes bcm_sf2_sw_mac_config() and bcm_sf2_sw_mac_link_set() to
access random registers.

There is no a SWITCH_REG_RGMII_7_CNTRL register offset defined in fact
there is no offset for ports 0, 1 or 2, there is a
SWITCH_REG_RGMII_11_CNTRL which is at offset 0x14c from the start of
SWITCH_REG. So yes, you would definitively overrun the
bcm_sf2_4908_reg_offsets() array.
-- 
Florian