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: <a6700669-5260-2d70-65a2-66c8cbfc6881@gmail.com>
Date:   Thu, 11 Mar 2021 14:13:54 -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/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.
-- 
Florian