On maandag 27 januari 2025 09:58:45 Midden-Europese standaardtijd Stephan Gerhold wrote:
> On Sun, Jan 26, 2025 at 09:57:22PM +0100, Luca Weiss wrote:
> > For example MSM8974 has mx voltage rail exposed as regulator and only cx
> > voltage rail is exposed as power domain. This power domain (cx) is
> > attached internally in power domain and cannot be attached in this driver.
> >
> > Fixes: 8750cf392394 ("remoteproc: qcom_q6v5_mss: Allow replacing regulators with power domains")
> > Co-developed-by: Matti Lehtimäki <matti.lehtimaki@xxxxxxxxx>
> > Signed-off-by: Matti Lehtimäki <matti.lehtimaki@xxxxxxxxx>
> > Signed-off-by: Luca Weiss <luca@xxxxxxxxxxxx>
> > ---
> > Changes in v2:
> > - Move MSM8974 mx-supply from "fallback_proxy_supply" to
> > "proxy_supply" to match updated DT schema
> > - Add fixes tag
> > ---
> > drivers/remoteproc/qcom_q6v5_mss.c | 20 +++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > index e78bd986dc3f256effce4470222c0a5faeea86ec..e2523b01febf393abfe50740a68b85a04011293c 100644
> > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > @@ -1828,6 +1828,13 @@ static int q6v5_pds_attach(struct device *dev, struct device **devs,
> > if (!pd_names)
> > return 0;
> >
> > + /* Handle single power domain */
> > + if (dev->pm_domain) {
> > + devs[0] = dev;
> > + pm_runtime_enable(dev);
> > + return 1;
> > + }
> > +
> > while (pd_names[num_pds])
> > num_pds++;
>
> Hmm, I think you should put the above if condition below this loop and
> verify that num_pds == 1. Otherwise this would hide the error condition
> if the platform needs multple PDs, but someone only specifies one of
> them in the DT. i.e.
>
> if (num_pds == 1 && dev->pm_domain) {
> // ...
> }
>
> >
> > @@ -1851,8 +1858,15 @@ static int q6v5_pds_attach(struct device *dev, struct device **devs,
> > static void q6v5_pds_detach(struct q6v5 *qproc, struct device **pds,
> > size_t pd_count)
> > {
> > + struct device *dev = qproc->dev;
> > int i;
> >
> > + /* Handle single power domain */
> > + if (dev->pm_domain && pd_count) {
>
> Maybe if (pd_count == 1 && dev->pm_domain) for consistency with the
> above then.
Good suggestions, thanks!
>
> > + pm_runtime_disable(dev);
>
> I'm guessing it does, but just to make sure: Have you verified that the
> power domain is indeed voted for off after this call to
> pm_runtime_disable()? Start the remoteproc and when it's booted inspect
> /sys/kernel/debug/pm_genpd/pm_genpd_summary to see if the PD/remoteproc
> dev is suspended.
Looks sane I think (modem: remoteproc@fc880000)
htc-memul:~$ sudo cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain status children performance
/device runtime status managed by
------------------------------------------------------------------------------
oxili_cx off-0 0
fdb00000.gpu suspended 0 SW
camss_vfe off-0 0
camss_jpeg off-0 0
mdss on 0
fd900000.display-subsystem active 0 SW
venus0 off-0 0
cx_vfc off-0 0
cx_ao off-0 0
cx on 0
fc880000.remoteproc suspended 0 SW
fe200000.remoteproc unsupported 0 SW
fb204000.remoteproc suspended 0 SW
usb_hs_hsic off-0 0
Regards
Luca
>
> Thanks,
> Stephan
>
