* [PATCH v3 1/2] net: macb: Allow SGMII only if we are a GEM in mac_validate
@ 2021-10-12 19:46 Sean Anderson
2021-10-12 19:46 ` [PATCH v3 2/2] net: macb: Clean up macb_validate Sean Anderson
0 siblings, 1 reply; 4+ messages in thread
From: Sean Anderson @ 2021-10-12 19:46 UTC (permalink / raw)
To: netdev, David S . Miller, Jakub Kicinski
Cc: Claudiu Beznea, Nicolas Ferre, Antoine Tenart, Russell King,
Sean Anderson
This aligns mac_validate with mac_config. In mac_config, SGMII is only
enabled if macb_is_gem. Validate should care if the mac is a gem as
well.
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
Changes in v3:
- Order bugfix patch first
Changes in v2:
- New
drivers/net/ethernet/cadence/macb_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 683f14665c2c..cb0f86544955 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -528,6 +528,7 @@ static void macb_validate(struct phylink_config *config,
if (!macb_is_gem(bp) &&
(state->interface == PHY_INTERFACE_MODE_GMII ||
+ state->interface == PHY_INTERFACE_MODE_SGMII ||
phy_interface_mode_is_rgmii(state->interface))) {
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
return;
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 2/2] net: macb: Clean up macb_validate
2021-10-12 19:46 [PATCH v3 1/2] net: macb: Allow SGMII only if we are a GEM in mac_validate Sean Anderson
@ 2021-10-12 19:46 ` Sean Anderson
2021-10-13 8:29 ` Antoine Tenart
0 siblings, 1 reply; 4+ messages in thread
From: Sean Anderson @ 2021-10-12 19:46 UTC (permalink / raw)
To: netdev, David S . Miller, Jakub Kicinski
Cc: Claudiu Beznea, Nicolas Ferre, Antoine Tenart, Russell King,
Sean Anderson
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. This reduces the number of if statements, and the number of
times the same condition is checked. No functional change intended.
Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This patch was originally submitted as [1].
There is another approach which could be used here. We could do
something like
macb_set_mode(bp, mask, iface)
{
switch (iface) {
case 10GBASER:
if (...)
phylink_set_10g_modes(mask);
else
return -EINVAL;
break;
case GMII:
/* etc etc */
}
return 0;
}
macb_validate(...)
{
if (state->interface == PHY_INTERFACE_MODE_NA) {
macb_set_mode(bp, mask, PHY_INTERFACE_MODE_10GBASER);
macb_set_mode(bp, mask, PHY_INTERFACE_MODE_GMII);
macb_set_mode(bp, mask, PHY_INTERFACE_MODE_MII);
} else if (macb_set_mode(bp, mask, state->interface)) {
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
return;
}
/* etc etc */
}
which has the advantage of much cleaner logic in the switch statement.
[1] https://lore.kernel.org/netdev/20211004191527.1610759-9-sean.anderson@seco.com/
Changes in v3:
- Remove labels/gotos in favor of explicit zeroing. Hopefully this
better illustrates the "linear" flow of the logic.
- Add comment with overview of the flow of the logic.
Changes in v2:
- Fix polarity of `one` being inverted
- Only set gigabit modes for NA if macb_is_gem()
drivers/net/ethernet/cadence/macb_main.c | 105 ++++++++++++-----------
1 file changed, 55 insertions(+), 50 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index cb0f86544955..9b2173c37edb 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -510,33 +510,65 @@ static void macb_validate(struct phylink_config *config,
unsigned long *supported,
struct phylink_link_state *state)
{
+ bool one = state->interface != PHY_INTERFACE_MODE_NA;
struct net_device *ndev = to_net_dev(config->dev);
__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
struct macb *bp = netdev_priv(ndev);
- /* We only support MII, RMII, GMII, RGMII & SGMII. */
- if (state->interface != PHY_INTERFACE_MODE_NA &&
- state->interface != PHY_INTERFACE_MODE_MII &&
- state->interface != PHY_INTERFACE_MODE_RMII &&
- state->interface != PHY_INTERFACE_MODE_GMII &&
- state->interface != PHY_INTERFACE_MODE_SGMII &&
- state->interface != PHY_INTERFACE_MODE_10GBASER &&
- !phy_interface_mode_is_rgmii(state->interface)) {
- bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
- return;
- }
-
- if (!macb_is_gem(bp) &&
- (state->interface == PHY_INTERFACE_MODE_GMII ||
- state->interface == PHY_INTERFACE_MODE_SGMII ||
- phy_interface_mode_is_rgmii(state->interface))) {
- bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
- return;
- }
-
- if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
- !(bp->caps & MACB_CAPS_HIGH_SPEED &&
- bp->caps & MACB_CAPS_PCS)) {
+ /* There are three major types of interfaces we support:
+ * - (R)MII supporting 10/100 Mbit/s
+ * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
+ * - 10GBASER supporting 10 Gbit/s only
+ * Because GMII and MII both support 10/100, GMII falls through to MII.
+ *
+ * If we can't support an interface mode, we just clear the supported
+ * mask and return. The major complication is that if we get
+ * PHY_INTERFACE_MODE_NA, we must return all modes we support. Because
+ * of this, NA starts at the top of the switch and falls all the way to
+ * the bottom, and doesn't return early if we don't support a
+ * particular mode.
+ */
+ switch (state->interface) {
+ case PHY_INTERFACE_MODE_NA:
+ case PHY_INTERFACE_MODE_10GBASER:
+ if (bp->caps & MACB_CAPS_HIGH_SPEED &&
+ bp->caps & MACB_CAPS_PCS &&
+ bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+ phylink_set_10g_modes(mask);
+ phylink_set(mask, 10000baseKR_Full);
+ } else if (one) {
+ bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ return;
+ }
+ if (one)
+ break;
+ fallthrough;
+ case PHY_INTERFACE_MODE_GMII:
+ case PHY_INTERFACE_MODE_RGMII:
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ case PHY_INTERFACE_MODE_RGMII_TXID:
+ case PHY_INTERFACE_MODE_SGMII:
+ if (macb_is_gem(bp)) {
+ if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+ phylink_set(mask, 1000baseT_Full);
+ phylink_set(mask, 1000baseX_Full);
+ if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
+ phylink_set(mask, 1000baseT_Half);
+ }
+ } else if (one) {
+ bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ return;
+ }
+ fallthrough;
+ case PHY_INTERFACE_MODE_MII:
+ case PHY_INTERFACE_MODE_RMII:
+ phylink_set(mask, 10baseT_Half);
+ phylink_set(mask, 10baseT_Full);
+ phylink_set(mask, 100baseT_Half);
+ phylink_set(mask, 100baseT_Full);
+ break;
+ default:
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
return;
}
@@ -544,33 +576,6 @@ static void macb_validate(struct phylink_config *config,
phylink_set_port_modes(mask);
phylink_set(mask, Autoneg);
phylink_set(mask, Asym_Pause);
-
- if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
- (state->interface == PHY_INTERFACE_MODE_NA ||
- state->interface == PHY_INTERFACE_MODE_10GBASER)) {
- phylink_set_10g_modes(mask);
- phylink_set(mask, 10000baseKR_Full);
- if (state->interface != PHY_INTERFACE_MODE_NA)
- goto out;
- }
-
- phylink_set(mask, 10baseT_Half);
- phylink_set(mask, 10baseT_Full);
- phylink_set(mask, 100baseT_Half);
- phylink_set(mask, 100baseT_Full);
-
- if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
- (state->interface == PHY_INTERFACE_MODE_NA ||
- state->interface == PHY_INTERFACE_MODE_GMII ||
- state->interface == PHY_INTERFACE_MODE_SGMII ||
- phy_interface_mode_is_rgmii(state->interface))) {
- phylink_set(mask, 1000baseT_Full);
- phylink_set(mask, 1000baseX_Full);
-
- if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
- phylink_set(mask, 1000baseT_Half);
- }
-out:
bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
bitmap_and(state->advertising, state->advertising, mask,
__ETHTOOL_LINK_MODE_MASK_NBITS);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/2] net: macb: Clean up macb_validate
2021-10-12 19:46 ` [PATCH v3 2/2] net: macb: Clean up macb_validate Sean Anderson
@ 2021-10-13 8:29 ` Antoine Tenart
2021-10-14 16:01 ` Sean Anderson
0 siblings, 1 reply; 4+ messages in thread
From: Antoine Tenart @ 2021-10-13 8:29 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Sean Anderson, netdev
Cc: Claudiu Beznea, Nicolas Ferre, Russell King, Sean Anderson
Quoting Sean Anderson (2021-10-12 21:46:44)
> + /* There are three major types of interfaces we support:
> + * - (R)MII supporting 10/100 Mbit/s
> + * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
> + * - 10GBASER supporting 10 Gbit/s only
> + * Because GMII and MII both support 10/100, GMII falls through to MII.
> + *
> + * If we can't support an interface mode, we just clear the supported
> + * mask and return. The major complication is that if we get
> + * PHY_INTERFACE_MODE_NA, we must return all modes we support. Because
> + * of this, NA starts at the top of the switch and falls all the way to
> + * the bottom, and doesn't return early if we don't support a
> + * particular mode.
> + */
> + switch (state->interface) {
> + case PHY_INTERFACE_MODE_NA:
> + case PHY_INTERFACE_MODE_10GBASER:
> + if (bp->caps & MACB_CAPS_HIGH_SPEED &&
> + bp->caps & MACB_CAPS_PCS &&
> + bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
> + phylink_set_10g_modes(mask);
> + phylink_set(mask, 10000baseKR_Full);
> + } else if (one) {
> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> + return;
> + }
> + if (one)
> + break;
This can go in the first if block.
> + fallthrough;
> + case PHY_INTERFACE_MODE_GMII:
> + case PHY_INTERFACE_MODE_RGMII:
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + case PHY_INTERFACE_MODE_SGMII:
> + if (macb_is_gem(bp)) {
> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
Is not having MACB_CAPS_GIGABIT_MODE_AVAILABLE acceptable here, or
should the two above checks be merged?
> + phylink_set(mask, 1000baseT_Full);
> + phylink_set(mask, 1000baseX_Full);
> + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
> + phylink_set(mask, 1000baseT_Half);
> + }
> + } else if (one) {
> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> + return;
> + }
> + fallthrough;
> + case PHY_INTERFACE_MODE_MII:
> + case PHY_INTERFACE_MODE_RMII:
> + phylink_set(mask, 10baseT_Half);
> + phylink_set(mask, 10baseT_Full);
> + phylink_set(mask, 100baseT_Half);
> + phylink_set(mask, 100baseT_Full);
> + break;
> + default:
> bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> return;
> }
(For readability, it's not for me to decide in the end).
Thanks,
Antoine
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/2] net: macb: Clean up macb_validate
2021-10-13 8:29 ` Antoine Tenart
@ 2021-10-14 16:01 ` Sean Anderson
0 siblings, 0 replies; 4+ messages in thread
From: Sean Anderson @ 2021-10-14 16:01 UTC (permalink / raw)
To: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev
Cc: Claudiu Beznea, Nicolas Ferre, Russell King
On 10/13/21 4:29 AM, Antoine Tenart wrote:
> Quoting Sean Anderson (2021-10-12 21:46:44)
>> + /* There are three major types of interfaces we support:
>> + * - (R)MII supporting 10/100 Mbit/s
>> + * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
>> + * - 10GBASER supporting 10 Gbit/s only
>> + * Because GMII and MII both support 10/100, GMII falls through to MII.
>> + *
>> + * If we can't support an interface mode, we just clear the supported
>> + * mask and return. The major complication is that if we get
>> + * PHY_INTERFACE_MODE_NA, we must return all modes we support. Because
>> + * of this, NA starts at the top of the switch and falls all the way to
>> + * the bottom, and doesn't return early if we don't support a
>> + * particular mode.
>> + */
>> + switch (state->interface) {
>> + case PHY_INTERFACE_MODE_NA:
>> + case PHY_INTERFACE_MODE_10GBASER:
>> + if (bp->caps & MACB_CAPS_HIGH_SPEED &&
>> + bp->caps & MACB_CAPS_PCS &&
>> + bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
>> + phylink_set_10g_modes(mask);
>> + phylink_set(mask, 10000baseKR_Full);
>> + } else if (one) {
>> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>> + return;
>> + }
>> + if (one)
>> + break;
>
> This can go in the first if block.
OK.
>> + fallthrough;
>> + case PHY_INTERFACE_MODE_GMII:
>> + case PHY_INTERFACE_MODE_RGMII:
>> + case PHY_INTERFACE_MODE_RGMII_ID:
>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>> + case PHY_INTERFACE_MODE_RGMII_TXID:
>> + case PHY_INTERFACE_MODE_SGMII:
>> + if (macb_is_gem(bp)) {
>> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
>
> Is not having MACB_CAPS_GIGABIT_MODE_AVAILABLE acceptable here, or
> should the two above checks be merged?
As I understand it, macb_is_gem does not imply GIGABIT_MODE. I'm not
sure if GIGABIT_MODE implies macb_is_gem. The logic here is mostly to
match that in prepare()/config(). From what I can gather, all accesses
to GEM registers are protected by macb_is_gem.
--Sean
>> + phylink_set(mask, 1000baseT_Full);
>> + phylink_set(mask, 1000baseX_Full);
>> + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
>> + phylink_set(mask, 1000baseT_Half);
>> + }
>> + } else if (one) {
>> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>> + return;
>> + }
>> + fallthrough;
>> + case PHY_INTERFACE_MODE_MII:
>> + case PHY_INTERFACE_MODE_RMII:
>> + phylink_set(mask, 10baseT_Half);
>> + phylink_set(mask, 10baseT_Full);
>> + phylink_set(mask, 100baseT_Half);
>> + phylink_set(mask, 100baseT_Full);
>> + break;
>> + default:
>> bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>> return;
>> }
>
> (For readability, it's not for me to decide in the end).
>
> Thanks,
> Antoine
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-14 16:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12 19:46 [PATCH v3 1/2] net: macb: Allow SGMII only if we are a GEM in mac_validate Sean Anderson
2021-10-12 19:46 ` [PATCH v3 2/2] net: macb: Clean up macb_validate Sean Anderson
2021-10-13 8:29 ` Antoine Tenart
2021-10-14 16:01 ` Sean Anderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.