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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 12 Mar 2021 09:16:37 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rafał Miłecki <zajec5@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: 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:
>> On 3/11/21 2:04 PM, Rafał Miłecki wrote:
>>> On 10.03.2021 18:19, Florian Fainelli wrote:
>>>> On 3/10/21 3:59 AM, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@...ecki.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@...ecki.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.
>>>
>>>
>>>>> +    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