Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Mon, 2 Oct 2023 09:54:34 -0400
From:   Ben Wolsieffer <ben.wolsieffer@hefring.com>
To:     Jacob Keller <jacob.e.keller@intel.com>
Cc:     linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Jose Abreu <joabreu@synopsys.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Christophe Roullier <christophe.roullier@st.com>
Subject: Re: [PATCH net] net: stmmac: dwmac-stm32: fix resume on STM32 MCU
Message-ID: <ZRrLmjxoIIx7pIcs@dell-precision-5540>
References: <20230927175749.1419774-1-ben.wolsieffer@hefring.com>
 <681cc4ca-9fd7-9436-6c7d-d7da95026ce3@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <681cc4ca-9fd7-9436-6c7d-d7da95026ce3@intel.com>
Precedence: bulk

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