[PATCH] soundwire: master: use pm_runtime_set_active() on add

[PATCH] soundwire: master: use pm_runtime_set_active() on add

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The 'master' device acts as a glue layer used during bus
initialization only, and it needs to be 'transparent' for pm_runtime
management. Its behavior should be that it becomes active when one of
its children becomes active, and suspends when all of its children are
suspended.

In our tests on Intel platforms, we routinely see these sort of
warnings on the initial boot:

[ 21.447345] rt715 sdw:3:25d:715:0: runtime PM trying to activate
child device sdw:3:25d:715:0 but parent (sdw-master-3) is not active

This is root-caused to a missing setup to make the device 'active' on
probe. Since we don't want the device to remain active forever after
the probe, the autosuspend configuration is also enabled at the end of
the probe - the device will actually autosuspend only in the case
where there are no devices physically attached. In practice, the
master device will suspend when all its children are no longer active.

Fixes: bd84256e86ecf ('soundwire: master: enable pm runtime')
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/master.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
index 3488bb824e84..9b05c9e25ebe 100644
--- a/drivers/soundwire/master.c
+++ b/drivers/soundwire/master.c
@@ -8,6 +8,15 @@
 #include <linux/soundwire/sdw_type.h>
 #include "bus.h"
 
+/*
+ * The 3s value for autosuspend will only be used if there are no
+ * devices physically attached on a bus segment. In practice enabling
+ * the bus operation will result in children devices become active and
+ * the master device will only suspend when all its children are no
+ * longer active.
+ */
+#define SDW_MASTER_SUSPEND_DELAY_MS 3000
+
 /*
  * The sysfs for properties reflects the MIPI description as given
  * in the MIPI DisCo spec
@@ -154,7 +163,12 @@ int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
 	bus->dev = &md->dev;
 	bus->md = md;
 
+	pm_runtime_set_autosuspend_delay(&bus->md->dev, SDW_MASTER_SUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(&bus->md->dev);
+	pm_runtime_mark_last_busy(&bus->md->dev);
+	pm_runtime_set_active(&bus->md->dev);
 	pm_runtime_enable(&bus->md->dev);
+	pm_runtime_idle(&bus->md->dev);
 device_register_err:
 	return ret;
 }
-- 
2.17.1

Re: [PATCH] soundwire: master: use pm_runtime_set_active() on add

[Vinod Koul]

On 24-11-20, 21:07, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> The 'master' device acts as a glue layer used during bus
> initialization only, and it needs to be 'transparent' for pm_runtime
> management. Its behavior should be that it becomes active when one of
> its children becomes active, and suspends when all of its children are
> suspended.
> 
> In our tests on Intel platforms, we routinely see these sort of
> warnings on the initial boot:
> 
> [ 21.447345] rt715 sdw:3:25d:715:0: runtime PM trying to activate
> child device sdw:3:25d:715:0 but parent (sdw-master-3) is not active
> 
> This is root-caused to a missing setup to make the device 'active' on
> probe. Since we don't want the device to remain active forever after
> the probe, the autosuspend configuration is also enabled at the end of
> the probe - the device will actually autosuspend only in the case
> where there are no devices physically attached. In practice, the
> master device will suspend when all its children are no longer active.
> 
> Fixes: bd84256e86ecf ('soundwire: master: enable pm runtime')
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/master.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> index 3488bb824e84..9b05c9e25ebe 100644
> --- a/drivers/soundwire/master.c
> +++ b/drivers/soundwire/master.c
> @@ -8,6 +8,15 @@
>  #include <linux/soundwire/sdw_type.h>
>  #include "bus.h"
>  
> +/*
> + * The 3s value for autosuspend will only be used if there are no
> + * devices physically attached on a bus segment. In practice enabling
> + * the bus operation will result in children devices become active and
> + * the master device will only suspend when all its children are no
> + * longer active.
> + */
> +#define SDW_MASTER_SUSPEND_DELAY_MS 3000
> +
>  /*
>   * The sysfs for properties reflects the MIPI description as given
>   * in the MIPI DisCo spec
> @@ -154,7 +163,12 @@ int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
>  	bus->dev = &md->dev;
>  	bus->md = md;
>  
> +	pm_runtime_set_autosuspend_delay(&bus->md->dev, SDW_MASTER_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(&bus->md->dev);
> +	pm_runtime_mark_last_busy(&bus->md->dev);
> +	pm_runtime_set_active(&bus->md->dev);
>  	pm_runtime_enable(&bus->md->dev);
> +	pm_runtime_idle(&bus->md->dev);

I understand that this needs to be done somewhere but is the core the
right place. In intel case it maybe a dummy device but other controllers
are real devices and may not support pm.

I think better idea would be to do this in respective driver.. that way
it would be an opt-in for device supporting pm.

Thanks
-- 
~Vinod

RE: [PATCH] soundwire: master: use pm_runtime_set_active() on add

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Wednesday, November 25, 2020 1:05 PM
> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> gregkh@linuxfoundation.org; jank@cadence.com;
> srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>; Lin,
> Mengdong <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH] soundwire: master: use pm_runtime_set_active() on
> add
> 
> On 24-11-20, 21:07, Bard Liao wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >
> > The 'master' device acts as a glue layer used during bus
> > initialization only, and it needs to be 'transparent' for pm_runtime
> > management. Its behavior should be that it becomes active when one of
> > its children becomes active, and suspends when all of its children are
> > suspended.
> >
> > In our tests on Intel platforms, we routinely see these sort of
> > warnings on the initial boot:
> >
> > [ 21.447345] rt715 sdw:3:25d:715:0: runtime PM trying to activate
> > child device sdw:3:25d:715:0 but parent (sdw-master-3) is not active
> >
> > This is root-caused to a missing setup to make the device 'active' on
> > probe. Since we don't want the device to remain active forever after
> > the probe, the autosuspend configuration is also enabled at the end of
> > the probe - the device will actually autosuspend only in the case
> > where there are no devices physically attached. In practice, the
> > master device will suspend when all its children are no longer active.
> >
> > Fixes: bd84256e86ecf ('soundwire: master: enable pm runtime')
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com>
> > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > ---
> >  drivers/soundwire/master.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> > index 3488bb824e84..9b05c9e25ebe 100644
> > --- a/drivers/soundwire/master.c
> > +++ b/drivers/soundwire/master.c
> > @@ -8,6 +8,15 @@
> >  #include <linux/soundwire/sdw_type.h>  #include "bus.h"
> >
> > +/*
> > + * The 3s value for autosuspend will only be used if there are no
> > + * devices physically attached on a bus segment. In practice enabling
> > + * the bus operation will result in children devices become active
> > +and
> > + * the master device will only suspend when all its children are no
> > + * longer active.
> > + */
> > +#define SDW_MASTER_SUSPEND_DELAY_MS 3000
> > +
> >  /*
> >   * The sysfs for properties reflects the MIPI description as given
> >   * in the MIPI DisCo spec
> > @@ -154,7 +163,12 @@ int sdw_master_device_add(struct sdw_bus *bus,
> struct device *parent,
> >  	bus->dev = &md->dev;
> >  	bus->md = md;
> >
> > +	pm_runtime_set_autosuspend_delay(&bus->md->dev,
> SDW_MASTER_SUSPEND_DELAY_MS);
> > +	pm_runtime_use_autosuspend(&bus->md->dev);
> > +	pm_runtime_mark_last_busy(&bus->md->dev);
> > +	pm_runtime_set_active(&bus->md->dev);
> >  	pm_runtime_enable(&bus->md->dev);
> > +	pm_runtime_idle(&bus->md->dev);
> 
> I understand that this needs to be done somewhere but is the core the right
> place. In intel case it maybe a dummy device but other controllers are real
> devices and may not support pm.
> 
> I think better idea would be to do this in respective driver.. that way it
> would be an opt-in for device supporting pm.

Should it be put in the same place as pm_runtime_enable?
IMHO, pm_runtime_enable is in the core already and it seems to be harmless
for devices which don't support pm. And pm can still be optional on md's
parent device.


> 
> Thanks
> --
> ~Vinod

Re: [PATCH] soundwire: master: use pm_runtime_set_active() on add

[Vinod Koul]

On 26-11-20, 03:11, Liao, Bard wrote:
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Wednesday, November 25, 2020 1:05 PM
> > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> > gregkh@linuxfoundation.org; jank@cadence.com;
> > srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> > ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> > louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>; Lin,
> > Mengdong <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > Subject: Re: [PATCH] soundwire: master: use pm_runtime_set_active() on
> > add
> > 
> > On 24-11-20, 21:07, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > >
> > > The 'master' device acts as a glue layer used during bus
> > > initialization only, and it needs to be 'transparent' for pm_runtime
> > > management. Its behavior should be that it becomes active when one of
> > > its children becomes active, and suspends when all of its children are
> > > suspended.
> > >
> > > In our tests on Intel platforms, we routinely see these sort of
> > > warnings on the initial boot:
> > >
> > > [ 21.447345] rt715 sdw:3:25d:715:0: runtime PM trying to activate
> > > child device sdw:3:25d:715:0 but parent (sdw-master-3) is not active
> > >
> > > This is root-caused to a missing setup to make the device 'active' on
> > > probe. Since we don't want the device to remain active forever after
> > > the probe, the autosuspend configuration is also enabled at the end of
> > > the probe - the device will actually autosuspend only in the case
> > > where there are no devices physically attached. In practice, the
> > > master device will suspend when all its children are no longer active.
> > >
> > > Fixes: bd84256e86ecf ('soundwire: master: enable pm runtime')
> > > Signed-off-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > ---
> > >  drivers/soundwire/master.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> > > index 3488bb824e84..9b05c9e25ebe 100644
> > > --- a/drivers/soundwire/master.c
> > > +++ b/drivers/soundwire/master.c
> > > @@ -8,6 +8,15 @@
> > >  #include <linux/soundwire/sdw_type.h>  #include "bus.h"
> > >
> > > +/*
> > > + * The 3s value for autosuspend will only be used if there are no
> > > + * devices physically attached on a bus segment. In practice enabling
> > > + * the bus operation will result in children devices become active
> > > +and
> > > + * the master device will only suspend when all its children are no
> > > + * longer active.
> > > + */
> > > +#define SDW_MASTER_SUSPEND_DELAY_MS 3000
> > > +
> > >  /*
> > >   * The sysfs for properties reflects the MIPI description as given
> > >   * in the MIPI DisCo spec
> > > @@ -154,7 +163,12 @@ int sdw_master_device_add(struct sdw_bus *bus,
> > struct device *parent,
> > >  	bus->dev = &md->dev;
> > >  	bus->md = md;
> > >
> > > +	pm_runtime_set_autosuspend_delay(&bus->md->dev,
> > SDW_MASTER_SUSPEND_DELAY_MS);
> > > +	pm_runtime_use_autosuspend(&bus->md->dev);
> > > +	pm_runtime_mark_last_busy(&bus->md->dev);
> > > +	pm_runtime_set_active(&bus->md->dev);
> > >  	pm_runtime_enable(&bus->md->dev);
> > > +	pm_runtime_idle(&bus->md->dev);
> > 
> > I understand that this needs to be done somewhere but is the core the right
> > place. In intel case it maybe a dummy device but other controllers are real
> > devices and may not support pm.
> > 
> > I think better idea would be to do this in respective driver.. that way it
> > would be an opt-in for device supporting pm.
> 
> Should it be put in the same place as pm_runtime_enable?
> IMHO, pm_runtime_enable is in the core already and it seems to be harmless
> for devices which don't support pm. And pm can still be optional on md's
> parent device.

For intel case yes, but world is not only intel, there are md which do
not have a parent like sof. they are real sdw controller devices

-- 
~Vinod

Re: [PATCH] soundwire: master: use pm_runtime_set_active() on add

[Vinod Koul]

On 26-11-20, 09:52, Vinod Koul wrote:

> > > > @@ -154,7 +163,12 @@ int sdw_master_device_add(struct sdw_bus *bus,
> > > struct device *parent,
> > > >  	bus->dev = &md->dev;
> > > >  	bus->md = md;
> > > >
> > > > +	pm_runtime_set_autosuspend_delay(&bus->md->dev,
> > > SDW_MASTER_SUSPEND_DELAY_MS);
> > > > +	pm_runtime_use_autosuspend(&bus->md->dev);
> > > > +	pm_runtime_mark_last_busy(&bus->md->dev);
> > > > +	pm_runtime_set_active(&bus->md->dev);
> > > >  	pm_runtime_enable(&bus->md->dev);
> > > > +	pm_runtime_idle(&bus->md->dev);
> > > 
> > > I understand that this needs to be done somewhere but is the core the right
> > > place. In intel case it maybe a dummy device but other controllers are real
> > > devices and may not support pm.
> > > 
> > > I think better idea would be to do this in respective driver.. that way it
> > > would be an opt-in for device supporting pm.
> > 
> > Should it be put in the same place as pm_runtime_enable?
> > IMHO, pm_runtime_enable is in the core already and it seems to be harmless
> > for devices which don't support pm. And pm can still be optional on md's
> > parent device.
> 
> For intel case yes, but world is not only intel, there are md which do
> not have a parent like sof. they are real sdw controller devices

Sorry I confused md with real master device ;-) I guess this patch
should be okay then.. As the real parent will control.

Thanks
-- 
~Vinod

Re: [PATCH] soundwire: master: use pm_runtime_set_active() on add

[Vinod Koul]

On 24-11-20, 21:07, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> The 'master' device acts as a glue layer used during bus
> initialization only, and it needs to be 'transparent' for pm_runtime
> management. Its behavior should be that it becomes active when one of
> its children becomes active, and suspends when all of its children are
> suspended.
> 
> In our tests on Intel platforms, we routinely see these sort of
> warnings on the initial boot:
> 
> [ 21.447345] rt715 sdw:3:25d:715:0: runtime PM trying to activate
> child device sdw:3:25d:715:0 but parent (sdw-master-3) is not active
> 
> This is root-caused to a missing setup to make the device 'active' on
> probe. Since we don't want the device to remain active forever after
> the probe, the autosuspend configuration is also enabled at the end of
> the probe - the device will actually autosuspend only in the case
> where there are no devices physically attached. In practice, the
> master device will suspend when all its children are no longer active.

Applied, thanks

-- 
~Vinod