Re: [PATCH RFC net-next 5/9] net: pcs: xpcs: initialize runtime PM as suspended

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 2156 bytes --]

> xpcs_plat_init_clk() acquires the optional CSR clock but does not
> prepare or enable it. Marking the device runtime-active at that point
> makes the PM core assume that the hardware is already powered.
> 
> On Nord, once the XPCS CSR clock was described, the first
> pm_runtime_resume_and_get() accessed XPCS registers without invoking
> runtime_resume() to enable the clock. The matching runtime put then
> invoked runtime_suspend() and attempted to disable a clock which was
> never enabled, producing an already-disabled clock warning.
> 
> Initialize the runtime-PM state as suspended instead. The first
> register access now runs the resume callback and enables the CSR clock,
> while the subsequent suspend callback performs the matching disable.
> Check and propagate the state-transition error as part of probe.
> 
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

This patch seems a fix to me even for other SoCs. The pcs device is marked
as 'PM active' in xpcs_plat_init_clk() but the CSR clock is not actually
enabled. Moreover, the CSR clock will not be enabled even in the pm ops
since xpcs_plat_pm_runtime_resume() will not be executed (the device is
already as PM active). Am I missing something?

Regards,
Lorenzo

> ---
>  drivers/net/pcs/pcs-xpcs-plat.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs-plat.c b/drivers/net/pcs/pcs-xpcs-plat.c
> index e013a5253a22651220e62352d03faabc3a3c3658..328c50940d46eb8e7bea06ecd1a20214e3cde61c 100644
> --- a/drivers/net/pcs/pcs-xpcs-plat.c
> +++ b/drivers/net/pcs/pcs-xpcs-plat.c
> @@ -322,7 +322,11 @@ static int xpcs_plat_init_clk(struct dw_xpcs_plat *pxpcs)
>  		return dev_err_probe(dev, PTR_ERR(pxpcs->cclk),
>  				     "Failed to get CSR clock\n");
>  
> -	pm_runtime_set_active(dev);
> +	ret = pm_runtime_set_suspended(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to set runtime-PM suspended state\n");
> +
>  	ret = devm_pm_runtime_enable(dev);
>  	if (ret) {
>  		dev_err(dev, "Failed to enable runtime-PM\n");
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]