* [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar
@ 2021-03-10 11:59 Rafał Miłecki
2021-03-10 17:19 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-10 11:59 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
From: Rafał Miłecki <rafal@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 <rafal@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;
+ 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
+ reg |= CROSSBAR_BCM4908_EXT_RGMII << shift;
+ break;
+ default:
+ return;
+ }
+ reg_writel(priv, reg, REG_CROSSBAR);
+
+ reg = reg_readl(priv, REG_CROSSBAR);
+ mask = BIT(priv->num_crossbar_int_ports) - 1;
+ for (i = 0; i < priv->num_crossbar_int_ports; i++) {
+ shift = i * priv->num_crossbar_int_ports;
+
+ dev_dbg(dev, "crossbar in port #%d - out port #%d\n", i,
+ (reg >> shift) & mask);
+ }
+}
+
static void bcm_sf2_intr_disable(struct bcm_sf2_priv *priv)
{
intrl2_0_mask_set(priv, 0xffffffff);
@@ -856,6 +890,8 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
return ret;
}
+ bcm_sf2_crossbar_setup(priv);
+
ret = bcm_sf2_cfp_resume(ds);
if (ret)
return ret;
@@ -1128,6 +1164,7 @@ struct bcm_sf2_of_data {
const u16 *reg_offsets;
unsigned int core_reg_align;
unsigned int num_cfp_rules;
+ unsigned int num_crossbar_int_ports;
};
static const u16 bcm_sf2_4908_reg_offsets[] = {
@@ -1152,6 +1189,7 @@ static const struct bcm_sf2_of_data bcm_sf2_4908_data = {
.core_reg_align = 0,
.reg_offsets = bcm_sf2_4908_reg_offsets,
.num_cfp_rules = 0, /* FIXME */
+ .num_crossbar_int_ports = 2,
};
/* Register offsets for the SWITCH_REG_* block */
@@ -1262,6 +1300,7 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
priv->reg_offsets = data->reg_offsets;
priv->core_reg_align = data->core_reg_align;
priv->num_cfp_rules = data->num_cfp_rules;
+ priv->num_crossbar_int_ports = data->num_crossbar_int_ports;
priv->rcdev = devm_reset_control_get_optional_exclusive(&pdev->dev,
"switch");
@@ -1335,6 +1374,8 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
goto out_clk_mdiv;
}
+ bcm_sf2_crossbar_setup(priv);
+
bcm_sf2_gphy_enable_set(priv->dev->ds, true);
ret = bcm_sf2_mdio_register(ds);
diff --git a/drivers/net/dsa/bcm_sf2.h b/drivers/net/dsa/bcm_sf2.h
index 1ed901a68536..bf0b56313bc1 100644
--- a/drivers/net/dsa/bcm_sf2.h
+++ b/drivers/net/dsa/bcm_sf2.h
@@ -73,6 +73,7 @@ struct bcm_sf2_priv {
const u16 *reg_offsets;
unsigned int core_reg_align;
unsigned int num_cfp_rules;
+ unsigned int num_crossbar_int_ports;
/* spinlock protecting access to the indirect registers */
spinlock_t indir_lock;
diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
index 1d2d55c9f8aa..e297b09411f3 100644
--- a/drivers/net/dsa/bcm_sf2_regs.h
+++ b/drivers/net/dsa/bcm_sf2_regs.h
@@ -48,6 +48,13 @@ enum bcm_sf2_reg_offs {
#define PHY_PHYAD_SHIFT 8
#define PHY_PHYAD_MASK 0x1F
+/* Relative to REG_CROSSBAR */
+#define CROSSBAR_BCM4908_INT_P7 0
+#define CROSSBAR_BCM4908_INT_RUNNER 1
+#define CROSSBAR_BCM4908_EXT_SERDES 0
+#define CROSSBAR_BCM4908_EXT_GPHY4 1
+#define CROSSBAR_BCM4908_EXT_RGMII 2
+
#define REG_RGMII_CNTRL_P(x) (REG_RGMII_0_CNTRL + (x))
/* Relative to REG_RGMII_CNTRL */
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar
2021-03-10 11:59 [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar Rafał Miłecki
@ 2021-03-10 17:19 ` Florian Fainelli
2021-03-11 22:04 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2021-03-10 17:19 UTC (permalink / raw)
To: Rafał Miłecki, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
On 3/10/21 3:59 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@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 <rafal@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.
> + 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.
Other than that, this looks good to me, thanks!
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar
2021-03-10 17:19 ` Florian Fainelli
@ 2021-03-11 22:04 ` Rafał Miłecki
2021-03-11 22:13 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-11 22:04 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
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@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 <rafal@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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar
2021-03-11 22:04 ` Rafał Miłecki
@ 2021-03-11 22:13 ` Florian Fainelli
2021-03-12 10:01 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2021-03-11 22:13 UTC (permalink / raw)
To: Rafał Miłecki, Florian Fainelli, Andrew Lunn,
Vivien Didelot, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
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@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 <rafal@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.
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar
2021-03-11 22:13 ` Florian Fainelli
@ 2021-03-12 10:01 ` Rafał Miłecki
2021-03-12 17:16 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-12 10:01 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
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@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 <rafal@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.
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar
2021-03-12 10:01 ` Rafał Miłecki
@ 2021-03-12 17:16 ` Florian Fainelli
0 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2021-03-12 17:16 UTC (permalink / raw)
To: Rafał Miłecki, Florian Fainelli, Andrew Lunn,
Vivien Didelot, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
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@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 <rafal@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-12 17:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 11:59 [PATCH] net: dsa: bcm_sf2: setup BCM4908 internal crossbar Rafał Miłecki
2021-03-10 17:19 ` Florian Fainelli
2021-03-11 22:04 ` Rafał Miłecki
2021-03-11 22:13 ` Florian Fainelli
2021-03-12 10:01 ` Rafał Miłecki
2021-03-12 17:16 ` Florian Fainelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).