| Message ID | 379ae584cea112db60f4ada79c7e5ba4f3364a64.1719862038.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3b2aef99221d395ce37efa426d7b50e7dcd621d6 |
| 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│ } > >
Hi netdev maintainers, On Tue, Jul 02, 2024 at 09:05:19AM +0000, Daniel Golle wrote: > >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. > I've noticed that this patch was marked as "Changes Requested" on patchwork despite having received a positive review. I'm afraid this is possibly due to a misunderstanding: The (unrelated and rather exotic) similar issue pointed to by Przemek Kitszel should not be fixed in the same commit. It is unrelated, and if at all, should be sent to 'net' tree rather than 'net-next'. Looking at it more closely I would not consider it an issue as we currently in the bindings we **require** the correct number of sgmiisys phandles to be present for each SoC supporting SGMII: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mediatek,net.yaml#n200 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mediatek,net.yaml#n245 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mediatek,net.yaml#n287 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mediatek,net.yaml#n325 Hence there aren't ever any gaps, also because the sgmiisys phandles are defined in the SoC-specific DTSI **even for boards not making any use of them**. I hence would like this very patch to be merged (or at least discussed) as-is, and if there is really a need to address the issue mentioned by Przemek Kitszel, then deal with it in a separate commit. Cheers Daniel
On 7/5/24 13:24, Daniel Golle wrote: > Hi netdev maintainers, > TL;DR Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> and no-one provided feedback against merging this patch so far > On Tue, Jul 02, 2024 at 09:05:19AM +0000, Daniel Golle wrote: >>> 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. >> > > I've noticed that this patch was marked as "Changes Requested" on patchwork > despite having received a positive review. > > I'm afraid this is possibly due to a misunderstanding: > > The (unrelated and rather exotic) similar issue pointed to by Przemek > Kitszel should not be fixed in the same commit. It is unrelated, and if > at all, should be sent to 'net' tree rather than 'net-next'. > > Looking at it more closely I would not consider it an issue as we > currently in the bindings we **require** the correct number of sgmiisys phandles to be > present for each SoC supporting SGMII: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mediatek,net.yaml#n200 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mediatek,net.yaml#n245 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mediatek,net.yaml#n287 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mediatek,net.yaml#n325 > > Hence there aren't ever any gaps, also because the sgmiisys phandles are > defined in the SoC-specific DTSI **even for boards not making any use of > them**. > > I hence would like this very patch to be merged (or at least discussed) > as-is, and if there is really a need to address the issue mentioned by > Przemek Kitszel, then deal with it in a separate commit. > > > Cheers > > > Daniel you are right, I have even marked this specifically as Reviewed-by: $me, so I think is just a mistake to mark it as CR (without the RB tag, it will be wise to mark such comments as no-issue or similar, and I typically do so)
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 1 Jul 2024 20:28:14 +0100 you 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> > > [...] Here is the summary with links: - [net-next] net: ethernet: mediatek: Allow gaps in MAC allocation https://git.kernel.org/netdev/net-next/c/3b2aef99221d You are awesome, thank you!
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(-)