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]

Date:   Thu, 11 Mar 2021 23:04:49 +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 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?