Message-ID: <jnwylhbparw4uwci3epbd7th4izt3rnd3uzrnm5mdunm55kdoh@yyp4dprfh3sl>
Date: Mon, 29 Jan 2024 13:42:41 -0600
From: Andrew Halaney <ahalaney@...hat.com>
To: Sneh Shah <quic_snehshah@...cinc.com>
Cc: Vinod Koul <vkoul@...nel.org>, 
	Bhupesh Sharma <bhupesh.sharma@...aro.org>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>, 
	netdev@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	linux-stm32@...md-mailman.stormreply.com, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH net-next] net: stmmac: dwmac-qcom-ethqos: Add support for
 pm ops

On Sat, Jan 27, 2024 at 06:33:27PM +0530, Sneh Shah wrote:
> Add qcom ethqos specific runtime and system sleep pm ops.
> As part of system sleep qcom ethqos needs to disable all clocks.
> This ops will be extended further with qcom specific features.

This last sentence sounds like this series is incomplete, I'd avoid such
wording if its untrue. Upstream typically won't accept things that are
building infrastructure for patches that will "eventually be posted".

You state in your commit what the code does (really it replaces the
stmmac_pltfrm_ops with its own), but only gloss over the why. I'd lead
with the "why". i.e. I'd say something like
"net: stmmac: dwmac-qcom-ethqos: Turn clocks off/on during suspend/resume"

Since there's already a handler installed for PM ops, I'd explain why
you need to change to new ones as well.

> 
> Signed-off-by: Sneh Shah <quic_snehshah@...cinc.com>
> ---
>  .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 51 ++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 31631e3f89d0..cba601ee9e01 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -720,6 +720,55 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
>  	netdev_dbg(priv->dev, "PTP rate %d\n", plat_dat->clk_ptp_rate);
>  }
>  
> +static int qcom_ethqos_runtime_suspend(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +
> +	return stmmac_bus_clks_config(priv, false);
> +}
> +

This perfectly matches the stmmac_runtime_suspend() function installed
originally. I don't see why you couldn't at a minimum reuse
that function instead of writing your own.

> +static int qcom_ethqos_runtime_resume(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +
> +	return stmmac_bus_clks_config(priv, true);
> +}

Same idea as the stmmac_runtime_suspend() comment above!

> +
> +static int qcom_ethqos_suspend(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	if (!ndev || !netif_running(ndev))
> +		return -EINVAL;
> +
> +	ret = stmmac_suspend(dev);

ret here is ignored.

> +
> +	return stmmac_bus_clks_config(priv, false);
> +}
> +
> +static int qcom_ethqos_resume(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	int ret;

unused ret.

> +
> +	if (!ndev || !netif_running(ndev))
> +		return -EINVAL;
> +
> +	stmmac_bus_clks_config(priv, true);

Probably should check this.

> +
> +	return stmmac_resume(dev);
> +}

Both the new system sleep ops installed here basically match the
stmmac_pltfrm_suspend/resume() functions that were already installed.
The only difference I'm noting is that you want to call
stmmac_bus_clks_config() in your implementation, whereas the originals call
the exit()/init() callbacks if they exist in the platform driver.

I would say "let's just make a exit()/init() callback for Qualcomm", but
looking further... (see below)

> +
> +const struct dev_pm_ops qcom_ethqos_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(qcom_ethqos_suspend, qcom_ethqos_resume)
> +	SET_RUNTIME_PM_OPS(qcom_ethqos_runtime_suspend, qcom_ethqos_runtime_resume, NULL)
> +};
> +
>  static int qcom_ethqos_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -838,7 +887,7 @@ static struct platform_driver qcom_ethqos_driver = {
>  	.probe  = qcom_ethqos_probe,
>  	.driver = {
>  		.name           = "qcom-ethqos",
> -		.pm		= &stmmac_pltfr_pm_ops,
> +		.pm		= &qcom_ethqos_pm_ops,

You effectively remove the stmmac_pltfr_noirq_suspend()/resume()
callbacks here, which do the stmmac_bus_clks_config() via
pm_runtime_force_suspend() etc during late suspend/early resume.

I do see this if statement, but I believe !device_may_wakeup() is true here,
so the clocks should get killed.

	static int __maybe_unused stmmac_pltfr_noirq_suspend(struct device *dev)
	{
		struct net_device *ndev = dev_get_drvdata(dev);
		struct stmmac_priv *priv = netdev_priv(ndev);
		int ret;

		if (!netif_running(ndev))
			return 0;

		if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
			/* Disable clock in case of PWM is off */
			clk_disable_unprepare(priv->plat->clk_ptp_ref);

			ret = pm_runtime_force_suspend(dev);
			if (ret)
				return ret;
		}

		return 0;
	}

Right now I'm of the opinion that this patch shouldn't really change
much based on that digging. Please let me know if I'm missing something
but it appears to me this should already be working.

>  		.of_match_table = qcom_ethqos_match,
>  	},
>  };
> -- 
> 2.17.1
> 

Powered by blists - more mailing lists