Messages in this thread Patch in this message | | | Date | Wed, 3 May 2023 10:15:38 +0100 | Subject | Re: [PATCH 3/3] media: camss: Link CAMSS power domain | From | Bryan O'Donoghue <> |
| |
On 03/05/2023 08:25, Yassine Oudjana wrote: > From: Yassine Oudjana <y.oudjana@protonmail.com> > > The CAMSS power domain was previously enabled implicitly when the VFE power domains > were enabled. Commit 46cc03175498 (media: camss: Split power domain management, 2022-07-04) > delayed enabling VFE power domains which in turn delayed enabling the CAMSS power domain. > This made CSIPHY fail to enable camss_top_ahb_clk which requires the CAMSS power domain to > be on: > > [ 199.097810] ------------[ cut here ]------------ > [ 199.097893] camss_top_ahb_clk status stuck at 'off' > [ 199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160 > ... > [ 199.100064] clk_branch_wait+0x140/0x160 > [ 199.100112] clk_branch2_enable+0x30/0x40 > [ 199.100159] clk_core_enable+0x6c/0xb0 > [ 199.100211] clk_enable+0x2c/0x50 > [ 199.100257] camss_enable_clocks+0x94/0xe0 [qcom_camss] > [ 199.100342] csiphy_set_power+0x154/0x2a0 [qcom_camss] > ... > [ 199.101594] ---[ end trace 0000000000000000 ]--- > > Link the CAMSS power domain in camss_configure_pd to make sure it gets enabled before > CSIPHY tries to enable clocks. > > Fixes: 02afa816dbbf (media: camss: Add basic runtime PM support, 2018-07-25) > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
Same comment as in the cover letter, your Fixes: tag is broken
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 02afa816dbbf ("media: camss: Add basic runtime PM support")' #28: Fixes: 02afa816dbbf (media: camss: Add basic runtime PM support, 2018-07-25)
> --- > drivers/media/platform/qcom/camss/camss.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index 1ef26aea3eae..9aea8220d923 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -1453,6 +1453,7 @@ static const struct media_device_ops camss_media_ops = { > static int camss_configure_pd(struct camss *camss) > { > struct device *dev = camss->dev; > + int camss_pd_index; > int i; > int ret; > > @@ -1496,7 +1497,13 @@ static int camss_configure_pd(struct camss *camss) > } > } > > - if (i > camss->vfe_num) { > + /* Link CAMSS power domain if available */ > + camss_pd_index = device_property_match_string(camss->dev, "power-domain-names", "camss"); > + if (camss_pd_index >= 0) > + device_link_add(camss->dev, camss->genpd[camss_pd_index], DL_FLAG_STATELESS | > + DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); > + > + if (i > camss->vfe_num && i != camss_pd_index) { > camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1], > DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | > DL_FLAG_RPM_ACTIVE);
The rest of it seems reasonable to me. I'll give it a R/B T/B on your next iteration - including Fixes: fix as I'm OOO ATM.
--- bod
| |