| Message ID | 20250126-msm8226-modem-v2-3-e88d76d6daff@lucaweiss.eu |
|---|---|
| State | New |
| Headers | show |
| Series | Modem support for MSM8226 | expand |
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@gmail.com> > > Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com> > > Signed-off-by: Luca Weiss <luca@lucaweiss.eu> > > --- > > 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 >
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++; @@ -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) { + pm_runtime_disable(dev); + return; + } + for (i = 0; i < pd_count; i++) dev_pm_domain_detach(pds[i], false); } @@ -2449,13 +2463,13 @@ static const struct rproc_hexagon_res msm8974_mss = { .supply = "pll", .uA = 100000, }, - {} - }, - .fallback_proxy_supply = (struct qcom_mss_reg_res[]) { { .supply = "mx", .uV = 1050000, }, + {} + }, + .fallback_proxy_supply = (struct qcom_mss_reg_res[]) { { .supply = "cx", .uA = 100000,
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@gmail.com> Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com> Signed-off-by: Luca Weiss <luca@lucaweiss.eu> --- 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(-)