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
>>
>
