| Message ID | 379ae584cea112db60f4ada79c7e5ba4f3364a64.1719862038.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ethernet: mediatek: Allow gaps in MAC allocation | expand |
On 7/1/24 21:28, Daniel Golle wrote: > Some devices with MediaTek SoCs don't use the first but only the second > MAC in the chip. Especially with MT7981 which got a built-in 1GE PHY > connected to the second MAC this is quite common. > Make sure to reset and enable PSE also in those cases by skipping gaps > using 'continue' instead of aborting the loop using 'break'. > > Fixes: dee4dd10c79a ("net: ethernet: mtk_eth_soc: ppe: add support for multiple PPEs") > Suggested-by: Elad Yifee <eladwf@gmail.com> > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > Note that this should go to 'net-next' because the commit being fixed > isn't yet part of the 'net' tree. makes sense, and the fix is correct, so: Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > index 13d78d9b3197..2529b5b607c8 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > @@ -3396,7 +3396,7 @@ static int mtk_open(struct net_device *dev) > > for (i = 0; i < MTK_MAX_DEVS; i++) { > if (!eth->netdev[i]) > - break; > + continue; > > target_mac = netdev_priv(eth->netdev[i]); > if (!soc->offload_version) { what about: 4733│ static int mtk_sgmii_init(struct mtk_eth *eth) 4734│ { 4735│ struct device_node *np; 4736│ struct regmap *regmap; 4737│ u32 flags; 4738│ int i; 4739│ 4740│ for (i = 0; i < MTK_MAX_DEVS; i++) { 4741│ np = of_parse_phandle(eth->dev->of_node, "mediatek,sgmiisys", i); 4742│ if (!np) 4743│ break; should we also continue here? 4744│ 4745│ regmap = syscon_node_to_regmap(np); 4746│ flags = 0; 4747│ if (of_property_read_bool(np, "mediatek,pnswap")) 4748│ flags |= MTK_SGMII_FLAG_PN_SWAP; 4749│ 4750│ of_node_put(np); 4751│ 4752│ if (IS_ERR(regmap)) 4753│ return PTR_ERR(regmap); 4754│ 4755│ eth->sgmii_pcs[i] = mtk_pcs_lynxi_create(eth->dev, regmap, 4756│ eth->soc->ana_rgc3, 4757│ flags); 4758│ } 4759│ 4760│ return 0; 4761│ }
On 2 July 2024 06:58:22 UTC, Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote: >On 7/1/24 21:28, Daniel Golle wrote: >> Some devices with MediaTek SoCs don't use the first but only the second >> MAC in the chip. Especially with MT7981 which got a built-in 1GE PHY >> connected to the second MAC this is quite common. >> Make sure to reset and enable PSE also in those cases by skipping gaps >> using 'continue' instead of aborting the loop using 'break'. >> >> Fixes: dee4dd10c79a ("net: ethernet: mtk_eth_soc: ppe: add support for multiple PPEs") >> Suggested-by: Elad Yifee <eladwf@gmail.com> >> Signed-off-by: Daniel Golle <daniel@makrotopia.org> >> --- >> Note that this should go to 'net-next' because the commit being fixed >> isn't yet part of the 'net' tree. > >makes sense, and the fix is correct, so: >Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Thank you the fast review! >what about: >4733│ static int mtk_sgmii_init(struct mtk_eth *eth) >4734│ { >4735│ struct device_node *np; >4736│ struct regmap *regmap; >4737│ u32 flags; >4738│ int i; >4739│ >4740│ for (i = 0; i < MTK_MAX_DEVS; i++) { >4741│ np = of_parse_phandle(eth->dev->of_node, "mediatek,sgmiisys", i); >4742│ if (!np) >4743│ break; > >should we also continue here? Good point. As sgmiisys is defined in dtsi it's not so relevant in practise though, as the SoC components are of course always present even if we don't use them. Probably it is still better to not be overly strict on the presence of things we may not even use, not even emit an error message and silently break something else, so yes, worth fixing imho. > >4744│ >4745│ regmap = syscon_node_to_regmap(np); >4746│ flags = 0; >4747│ if (of_property_read_bool(np, "mediatek,pnswap")) >4748│ flags |= MTK_SGMII_FLAG_PN_SWAP; >4749│ >4750│ of_node_put(np); >4751│ >4752│ if (IS_ERR(regmap)) >4753│ return PTR_ERR(regmap); >4754│ >4755│ eth->sgmii_pcs[i] = mtk_pcs_lynxi_create(eth->dev, regmap, >4756│ eth->soc->ana_rgc3, >4757│ flags); >4758│ } >4759│ >4760│ return 0; >4761│ } > >
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index 13d78d9b3197..2529b5b607c8 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -3396,7 +3396,7 @@ static int mtk_open(struct net_device *dev) for (i = 0; i < MTK_MAX_DEVS; i++) { if (!eth->netdev[i]) - break; + continue; target_mac = netdev_priv(eth->netdev[i]); if (!soc->offload_version) {
Some devices with MediaTek SoCs don't use the first but only the second MAC in the chip. Especially with MT7981 which got a built-in 1GE PHY connected to the second MAC this is quite common. Make sure to reset and enable PSE also in those cases by skipping gaps using 'continue' instead of aborting the loop using 'break'. Fixes: dee4dd10c79a ("net: ethernet: mtk_eth_soc: ppe: add support for multiple PPEs") Suggested-by: Elad Yifee <eladwf@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- Note that this should go to 'net-next' because the commit being fixed isn't yet part of the 'net' tree. drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)