* Re: [v4,1/3] net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII [not found] <8f2139e8adf02b485a4c84d558fc23f78cf04add.1769195864.git.dan.carpenter@linaro.org> @ 2026-01-26 15:24 ` Simon Horman 2026-01-26 15:33 ` Russell King (Oracle) 0 siblings, 1 reply; 5+ messages in thread From: Simon Horman @ 2026-01-26 15:24 UTC (permalink / raw) To: dan.carpenter Cc: Simon Horman, robh, kuba, festevam, ghennadi.procopciuc, alexandre.torgue, linux-kernel, andrew+netdev, shawnguo, mbrugger, mcoquelin.stm32, linaro-s32, davem, s.hauer, edumazet, linux-arm-kernel, imx, conor+dt, krzk+dt, devicetree, Frank.li, chester62515, netdev, kernel, pabeni, jan.petrous, linux-stm32, s32 This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII This commit adds syscon/regmap support for accessing the GMAC_0_CTRL_STS register on s32 chipsets, while maintaining backwards compatibility with the existing direct MMIO approach for older device trees. > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c > index 5a485ee98fa7..2e6bb41f49e1 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c [ ... ] > @@ -40,7 +44,10 @@ struct s32_priv_data { > > static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac) > { > - writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts); > + if (gmac->ctrl_sts) > + writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts); > + else > + regmap_write(gmac->sts_regmap, gmac->sts_offset, S32_PHY_INTF_SEL_RGMII); Should the regmap_write() return value be checked here? The function returns int and the caller s32_gmac_init() checks the return value expecting errors to be propagated. For comparison, dwmac-sun55i.c in sun55i_gmac200_set_syscon() does check regmap_write() return value: ret = regmap_write(regmap, SYSCON_REG, reg); if (ret < 0) return dev_err_probe(dev, ret, "Failed to write to syscon\n"); > > dev_dbg(gmac->dev, "PHY mode set to %s\n", phy_modes(*gmac->intf_mode)); > > return 0; [ ... ] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [v4,1/3] net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII 2026-01-26 15:24 ` [v4,1/3] net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII Simon Horman @ 2026-01-26 15:33 ` Russell King (Oracle) 2026-01-26 16:01 ` Simon Horman 0 siblings, 1 reply; 5+ messages in thread From: Russell King (Oracle) @ 2026-01-26 15:33 UTC (permalink / raw) To: Simon Horman Cc: dan.carpenter, robh, kuba, festevam, ghennadi.procopciuc, alexandre.torgue, linux-kernel, andrew+netdev, shawnguo, mbrugger, mcoquelin.stm32, linaro-s32, davem, s.hauer, edumazet, linux-arm-kernel, imx, conor+dt, krzk+dt, devicetree, Frank.li, chester62515, netdev, kernel, pabeni, jan.petrous, linux-stm32, s32 On Mon, Jan 26, 2026 at 03:24:30PM +0000, Simon Horman wrote: > This is an AI-generated review of your patch. The human sending this > email has considered the AI review valid, or at least plausible. > > For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html > --- > net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII > > This commit adds syscon/regmap support for accessing the GMAC_0_CTRL_STS > register on s32 chipsets, while maintaining backwards compatibility with > the existing direct MMIO approach for older device trees. > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c > > index 5a485ee98fa7..2e6bb41f49e1 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c > > [ ... ] > > > @@ -40,7 +44,10 @@ struct s32_priv_data { > > > > static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac) > > { > > - writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts); > > + if (gmac->ctrl_sts) > > + writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts); > > + else > > + regmap_write(gmac->sts_regmap, gmac->sts_offset, S32_PHY_INTF_SEL_RGMII); > > Should the regmap_write() return value be checked here? The function > returns int and the caller s32_gmac_init() checks the return value > expecting errors to be propagated. For comparison, dwmac-sun55i.c in > sun55i_gmac200_set_syscon() does check regmap_write() return value: > > ret = regmap_write(regmap, SYSCON_REG, reg); > if (ret < 0) > return dev_err_probe(dev, ret, "Failed to write to syscon\n"); AI is wrong on this last line - s32_gmac_write_phy_intf_select() is called from s32_gmac_init(), which is called from plat_dat->init. plat_dat->init is called from two paths: 1. stmmac_pltfr_probe() -> stmmac_dvr_probe() -> plat_dat->init() 2. stmmac_resume() -> plat_dat->resume() -> stmmac_plat_resume() -> stmmac_pltfr_init() -> plat_dat->init() In the resume path, it is not appropriate to use dev_err_probe() because we're not in the probe path. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [v4,1/3] net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII 2026-01-26 15:33 ` Russell King (Oracle) @ 2026-01-26 16:01 ` Simon Horman 2026-01-27 8:55 ` Dan Carpenter 0 siblings, 1 reply; 5+ messages in thread From: Simon Horman @ 2026-01-26 16:01 UTC (permalink / raw) To: Russell King (Oracle) Cc: dan.carpenter, robh, kuba, festevam, ghennadi.procopciuc, alexandre.torgue, linux-kernel, andrew+netdev, shawnguo, mbrugger, mcoquelin.stm32, linaro-s32, davem, s.hauer, edumazet, linux-arm-kernel, imx, conor+dt, krzk+dt, devicetree, Frank.li, chester62515, netdev, kernel, pabeni, jan.petrous, linux-stm32, s32 On Mon, Jan 26, 2026 at 03:33:54PM +0000, Russell King (Oracle) wrote: > On Mon, Jan 26, 2026 at 03:24:30PM +0000, Simon Horman wrote: > > This is an AI-generated review of your patch. The human sending this > > email has considered the AI review valid, or at least plausible. > > > > For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html > > --- > > net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII > > > > This commit adds syscon/regmap support for accessing the GMAC_0_CTRL_STS > > register on s32 chipsets, while maintaining backwards compatibility with > > the existing direct MMIO approach for older device trees. > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c > > > index 5a485ee98fa7..2e6bb41f49e1 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c > > > > [ ... ] > > > > > @@ -40,7 +44,10 @@ struct s32_priv_data { > > > > > > static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac) > > > { > > > - writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts); > > > + if (gmac->ctrl_sts) > > > + writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts); > > > + else > > > + regmap_write(gmac->sts_regmap, gmac->sts_offset, S32_PHY_INTF_SEL_RGMII); > > > > Should the regmap_write() return value be checked here? The function > > returns int and the caller s32_gmac_init() checks the return value > > expecting errors to be propagated. For comparison, dwmac-sun55i.c in > > sun55i_gmac200_set_syscon() does check regmap_write() return value: > > > > ret = regmap_write(regmap, SYSCON_REG, reg); > > if (ret < 0) > > return dev_err_probe(dev, ret, "Failed to write to syscon\n"); > > AI is wrong on this last line - s32_gmac_write_phy_intf_select() is > called from s32_gmac_init(), which is called from plat_dat->init. > > plat_dat->init is called from two paths: > > 1. stmmac_pltfr_probe() -> stmmac_dvr_probe() -> plat_dat->init() > > 2. stmmac_resume() -> plat_dat->resume() -> stmmac_plat_resume() -> > stmmac_pltfr_init() -> plat_dat->init() > > In the resume path, it is not appropriate to use dev_err_probe() > because we're not in the probe path. Hi Russell, I agree that using dev_err_probe() is not appropriate here. And, FWIIW, I took that part to be an illustration that sun55i_gmac200_set_syscon() handles a similar case, rather than a suggestion of how to handle it here. But at any rate, I think the key question is should the case where regmap_write() returns an error be handled in s32_gmac_write_phy_intf_select() (by some means)? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [v4,1/3] net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII 2026-01-26 16:01 ` Simon Horman @ 2026-01-27 8:55 ` Dan Carpenter 2026-01-27 12:57 ` Simon Horman 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2026-01-27 8:55 UTC (permalink / raw) To: Simon Horman Cc: Russell King (Oracle), robh, kuba, festevam, ghennadi.procopciuc, alexandre.torgue, linux-kernel, andrew+netdev, shawnguo, mbrugger, mcoquelin.stm32, linaro-s32, davem, s.hauer, edumazet, linux-arm-kernel, imx, conor+dt, krzk+dt, devicetree, Frank.li, chester62515, netdev, kernel, pabeni, jan.petrous, linux-stm32, s32 On Mon, Jan 26, 2026 at 04:01:27PM +0000, Simon Horman wrote: > But at any rate, I think the key question is should the case > where regmap_write() returns an error be handled in > s32_gmac_write_phy_intf_select() (by some means)? Generally if register read/writes fail then there is nothing you can do a the software level, you need to buy a new computer. However, in this case we may eventually put the registers behind an SCMI interface so probably checking is a good idea. Could I leave the error message out? The callers has an error message and if you ever see the error message, and even with SCMI, the fix is probably still to buy a new computer. regards, dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [v4,1/3] net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII 2026-01-27 8:55 ` Dan Carpenter @ 2026-01-27 12:57 ` Simon Horman 0 siblings, 0 replies; 5+ messages in thread From: Simon Horman @ 2026-01-27 12:57 UTC (permalink / raw) To: Dan Carpenter Cc: Russell King (Oracle), robh, kuba, festevam, ghennadi.procopciuc, alexandre.torgue, linux-kernel, andrew+netdev, shawnguo, mbrugger, mcoquelin.stm32, linaro-s32, davem, s.hauer, edumazet, linux-arm-kernel, imx, conor+dt, krzk+dt, devicetree, Frank.li, chester62515, netdev, kernel, pabeni, jan.petrous, linux-stm32, s32 On Tue, Jan 27, 2026 at 11:55:49AM +0300, Dan Carpenter wrote: > On Mon, Jan 26, 2026 at 04:01:27PM +0000, Simon Horman wrote: > > But at any rate, I think the key question is should the case > > where regmap_write() returns an error be handled in > > s32_gmac_write_phy_intf_select() (by some means)? > > Generally if register read/writes fail then there is nothing you > can do a the software level, you need to buy a new computer. However, > in this case we may eventually put the registers behind an SCMI > interface so probably checking is a good idea. > > Could I leave the error message out? The callers has an error > message and if you ever see the error message, and even with SCMI, > the fix is probably still to buy a new computer. FWIIW, that seems reasonable to me. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-01-27 12:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <8f2139e8adf02b485a4c84d558fc23f78cf04add.1769195864.git.dan.carpenter@linaro.org>
2026-01-26 15:24 ` [v4,1/3] net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII Simon Horman
2026-01-26 15:33 ` Russell King (Oracle)
2026-01-26 16:01 ` Simon Horman
2026-01-27 8:55 ` Dan Carpenter
2026-01-27 12:57 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox