Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Yassine Oudjana]

On Fri, May 26 2023 at 09:49:30 PM +01:00:00, Bryan O'Donoghue 
<bryan.odonoghue@xxxxxxxxxx> wrote:
> On 26/05/2023 19:07, Yassine Oudjana wrote:
> > From: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
> >
> > The CAMSS power domain was previously enabled implicitly when the VFE
> > power domains were enabled.
> > Commit 46cc03175498 ("media: camss: Split power domain management")
> > 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")
> > Signed-off-by: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
> > ---
> >   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);
>
> Konrad pointed this out.
>
> Are you 100% sure you want to do this. We already have a way to count 
> the # of power-domains in camss_configure_pd().
>
> Your series is now adding a dependency on power-domain-names.
>
> Is there a good reason to add that dependency ? If not, then lets 
> just take the code from camss_configure_pd() and make it so that it 
> can be used/reused here.

Is there a good reason not to? I found that using the existing 
index-based method would unnecessarily complicate things since an extra 
layer of checks would be needed to differentiate between MSM8996 and 
TITAN SoCs, since those have the TITAN GDSC at the same index where the 
CAMSS GDSC is now added for MSM8996. The same checks will also have to 
be repeated in error paths and during cleanup.

I guessed the only reason we were still using this method for the 
existing PDs was to remain compatible with old DT as Konrad mentioned, 
and since this CAMSS PD is only added now, I thought it'd be a good 
opportunity to introduce power-domain-names and simplify things a bit.

> ---
> bod