Re: [PATCH net-next] net: stmmac: dwmac-qcom-ethqos: Add support for pm ops

[Sneh Shah]

On 1/30/2024 1:12 AM, Andrew Halaney wrote:
> 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@xxxxxxxxxxx>
>> ---
>>  .../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.

I agree with all the points above. The reason we were still pushing qcom
specific changes is we wanted a PM wrapper function to add qcom specific
changes for features such as deep sleep. 

Looks like I had overlooked exit()/init()  platform callbacks you have
correctly pointed out. I can use this callbacks to do all the ethqos specific 
operations. Considering this point, the current patch is unnecessary.

Thanks for pointing this out!
> 
>>  		.of_match_table = qcom_ethqos_match,
>>  	},
>>  };
>> -- 
>> 2.17.1
>>
>