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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c6c36617-dc16-ccb5-6760-ed660eb19b5b@gmail.com>
Date:   Fri, 12 Mar 2021 11:01:57 +0100
From:   Rafał Miłecki <zajec5@...il.com>
To:     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 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.

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.