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 <ra...@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 <ra...@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.
>>>
>>>
>>>>> + 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
