| Message ID | 20230927175749.1419774-1-ben.wolsieffer@hefring.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6f195d6b0da3b689922ba9e302af2f49592fa9fc |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: stmmac: dwmac-stm32: fix resume on STM32 MCU | expand |
On 9/27/2023 10:57 AM, Ben Wolsieffer wrote: > The STM32MP1 keeps clk_rx enabled during suspend, and therefore the > driver does not enable the clock in stm32_dwmac_init() if the device was > suspended. The problem is that this same code runs on STM32 MCUs, which > do disable clk_rx during suspend, causing the clock to never be > re-enabled on resume. > > This patch adds a variant flag to indicate that clk_rx remains enabled > during suspend, and uses this to decide whether to enable the clock in > stm32_dwmac_init() if the device was suspended. > Why not just keep clk_rx enabled unconditionally or unconditionally stop it during suspend? I guess that might be part of a larger cleanup and has more side effects? > This approach fixes this specific bug with limited opportunity for > unintended side-effects, but I have a follow up patch that will refactor > the clock configuration and hopefully make it less error prone. > I'd guess the follow-up refactor would target next? > Fixes: 6528e02cc9ff ("net: ethernet: stmmac: add adaptation for stm32mp157c.") > Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> > --- This seems pretty small and targeted so it does make sense to me as a net fix, but it definitely feels like a workaround. I look forward to reading the cleanup patch mentioned. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c > index bdb4de59a672..28c8ca5fba6c 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c > @@ -105,6 +105,7 @@ struct stm32_ops { > int (*parse_data)(struct stm32_dwmac *dwmac, > struct device *dev); > u32 syscfg_eth_mask; > + bool clk_rx_enable_in_suspend; > }; > > static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat) > @@ -122,7 +123,8 @@ static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat) > if (ret) > return ret; > > - if (!dwmac->dev->power.is_suspended) { > + if (!dwmac->ops->clk_rx_enable_in_suspend || > + !dwmac->dev->power.is_suspended) { > ret = clk_prepare_enable(dwmac->clk_rx); > if (ret) { > clk_disable_unprepare(dwmac->clk_tx); > @@ -514,7 +516,8 @@ static struct stm32_ops stm32mp1_dwmac_data = { > .suspend = stm32mp1_suspend, > .resume = stm32mp1_resume, > .parse_data = stm32mp1_parse_data, > - .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK > + .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK, > + .clk_rx_enable_in_suspend = true > }; > > static const struct of_device_id stm32_dwmac_match[] = {
Hi Jacob, On Fri, Sep 29, 2023 at 10:48:47AM -0700, Jacob Keller wrote: > > > On 9/27/2023 10:57 AM, Ben Wolsieffer wrote: > > The STM32MP1 keeps clk_rx enabled during suspend, and therefore the > > driver does not enable the clock in stm32_dwmac_init() if the device was > > suspended. The problem is that this same code runs on STM32 MCUs, which > > do disable clk_rx during suspend, causing the clock to never be > > re-enabled on resume. > > > > This patch adds a variant flag to indicate that clk_rx remains enabled > > during suspend, and uses this to decide whether to enable the clock in > > stm32_dwmac_init() if the device was suspended. > > > > Why not just keep clk_rx enabled unconditionally or unconditionally stop > it during suspend? I guess that might be part of a larger cleanup and > has more side effects? Ideally, you want to turn off as many clocks as possible in suspend to save power. I'm assuming there is some hardware reason the STM32MP1 needs the RX clock on during suspend, but it was not explained in the original patch. Without more information, I'm trying to maintain the existing behavior. > > > This approach fixes this specific bug with limited opportunity for > > unintended side-effects, but I have a follow up patch that will refactor > > the clock configuration and hopefully make it less error prone. > > > > I'd guess the follow-up refactor would target next? > > > Fixes: 6528e02cc9ff ("net: ethernet: stmmac: add adaptation for stm32mp157c.") > > Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> > > --- > > This seems pretty small and targeted so it does make sense to me as a > net fix, but it definitely feels like a workaround. > > I look forward to reading the cleanup patch mentioned. Sorry, I should have linked this when I re-posted this patch for net, but I previously submitted this patch as part of a series with the cleanup but was asked to split them up for net and net-next. Personally, I would be fine with them going into net-next together (or squashed). The original series can be found here: https://lore.kernel.org/linux-arm-kernel/20230919164535.128125-3-ben.wolsieffer@hefring.com/T/ Thanks, Ben
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 27 Sep 2023 13:57:49 -0400 you wrote: > The STM32MP1 keeps clk_rx enabled during suspend, and therefore the > driver does not enable the clock in stm32_dwmac_init() if the device was > suspended. The problem is that this same code runs on STM32 MCUs, which > do disable clk_rx during suspend, causing the clock to never be > re-enabled on resume. > > This patch adds a variant flag to indicate that clk_rx remains enabled > during suspend, and uses this to decide whether to enable the clock in > stm32_dwmac_init() if the device was suspended. > > [...] Here is the summary with links: - [net] net: stmmac: dwmac-stm32: fix resume on STM32 MCU https://git.kernel.org/netdev/net/c/6f195d6b0da3 You are awesome, thank you!
On 10/2/23 15:54, Ben Wolsieffer wrote: > Hi Jacob, > > On Fri, Sep 29, 2023 at 10:48:47AM -0700, Jacob Keller wrote: >> >> >> On 9/27/2023 10:57 AM, Ben Wolsieffer wrote: >>> The STM32MP1 keeps clk_rx enabled during suspend, and therefore the >>> driver does not enable the clock in stm32_dwmac_init() if the device was >>> suspended. The problem is that this same code runs on STM32 MCUs, which >>> do disable clk_rx during suspend, causing the clock to never be >>> re-enabled on resume. >>> >>> This patch adds a variant flag to indicate that clk_rx remains enabled >>> during suspend, and uses this to decide whether to enable the clock in >>> stm32_dwmac_init() if the device was suspended. >>> >> >> Why not just keep clk_rx enabled unconditionally or unconditionally stop >> it during suspend? I guess that might be part of a larger cleanup and >> has more side effects? > > Ideally, you want to turn off as many clocks as possible in suspend to > save power. I'm assuming there is some hardware reason the STM32MP1 > needs the RX clock on during suspend, but it was not explained in the > original patch. Without more information, I'm trying to maintain the > existing behavior. > Sorry for this late answer. We could need RX clock for WOL support. >> >>> This approach fixes this specific bug with limited opportunity for >>> unintended side-effects, but I have a follow up patch that will refactor >>> the clock configuration and hopefully make it less error prone. >>> >> >> I'd guess the follow-up refactor would target next? >> >>> Fixes: 6528e02cc9ff ("net: ethernet: stmmac: add adaptation for stm32mp157c.") >>> Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> >>> --- >> >> This seems pretty small and targeted so it does make sense to me as a >> net fix, but it definitely feels like a workaround. >> >> I look forward to reading the cleanup patch mentioned. > > Sorry, I should have linked this when I re-posted this patch for > net, but I previously submitted this patch as part of a series with > the cleanup but was asked to split them up for net and net-next. > Personally, I would be fine with them going into net-next together (or > squashed). > > The original series can be found here: > https://lore.kernel.org/linux-arm-kernel/20230919164535.128125-3-ben.wolsieffer@hefring.com/T/ > > Thanks, Ben
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c index bdb4de59a672..28c8ca5fba6c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c @@ -105,6 +105,7 @@ struct stm32_ops { int (*parse_data)(struct stm32_dwmac *dwmac, struct device *dev); u32 syscfg_eth_mask; + bool clk_rx_enable_in_suspend; }; static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat) @@ -122,7 +123,8 @@ static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat) if (ret) return ret; - if (!dwmac->dev->power.is_suspended) { + if (!dwmac->ops->clk_rx_enable_in_suspend || + !dwmac->dev->power.is_suspended) { ret = clk_prepare_enable(dwmac->clk_rx); if (ret) { clk_disable_unprepare(dwmac->clk_tx); @@ -514,7 +516,8 @@ static struct stm32_ops stm32mp1_dwmac_data = { .suspend = stm32mp1_suspend, .resume = stm32mp1_resume, .parse_data = stm32mp1_parse_data, - .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK + .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK, + .clk_rx_enable_in_suspend = true }; static const struct of_device_id stm32_dwmac_match[] = {
The STM32MP1 keeps clk_rx enabled during suspend, and therefore the driver does not enable the clock in stm32_dwmac_init() if the device was suspended. The problem is that this same code runs on STM32 MCUs, which do disable clk_rx during suspend, causing the clock to never be re-enabled on resume. This patch adds a variant flag to indicate that clk_rx remains enabled during suspend, and uses this to decide whether to enable the clock in stm32_dwmac_init() if the device was suspended. This approach fixes this specific bug with limited opportunity for unintended side-effects, but I have a follow up patch that will refactor the clock configuration and hopefully make it less error prone. Fixes: 6528e02cc9ff ("net: ethernet: stmmac: add adaptation for stm32mp157c.") Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> --- drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)