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

  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")
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@protonmail.com>
---
 drivers/media/platform/qcom/camss/camss.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
  

On 26/05/2023 19:07, 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")
> 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@protonmail.com>
> ---
>   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.

---
bod
  

On 26/05/2023 19:07, 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")
> 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@protonmail.com>
> ---
>   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)

Surely if you get this far you already know that

camss->genpd_num = of_count_phandle_with_args(dev->of_node,
                                               "power-domains",
                                               "#power-domain-cells");
if (camss->genpd_num < 0) {
         dev_err(dev, "Power domains are not defined for camss\n");
         return camss->genpd_num;
}

So you don't need to add this additional dependency ?

---
bod
  

On 26.05.2023 22:49, Bryan O'Donoghue wrote:
> On 26/05/2023 19:07, 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")
>> 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@protonmail.com>
>> ---
>>   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.
This code contains a whole bunch of hacky counting logic that should have
been substituted with _byname, but now we're stuck with indices to keep
compatibility with old DTs :/

If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
CAMSS-related GDSCs, we could make it their parent in the clock driver
and call it a day.

Konrad

> 
> ---
> bod
  

On 26/05/2023 21:57, Konrad Dybcio wrote:
> This code contains a whole bunch of hacky counting logic that should have
> been substituted with _byname, but now we're stuck with indices to keep
> compatibility with old DTs :/
> 
> If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
> CAMSS-related GDSCs, we could make it their parent in the clock driver
> and call it a day.

I mean, it wouldn't make much sense from a hw design POV if that weren't 
the case..

Hmm looks like its already there.

static struct gdsc vfe0_gdsc = {
         .gdscr = 0x3664,
         .cxcs = (unsigned int []){ 0x36a8 },
         .cxc_count = 1,
         .pd = {
                 .name = "vfe0",
         },
         .parent = &camss_gdsc.pd,
         .pwrsts = PWRSTS_OFF_ON,
};

static struct gdsc vfe1_gdsc = {
         .gdscr = 0x3674,
         .cxcs = (unsigned int []){ 0x36ac },
         .cxc_count = 1,
         .pd = {
                 .name = "vfe1",
         },
         .parent = &camss_gdsc.pd,
         .pwrsts = PWRSTS_OFF_ON,
};

I feel this is probably a problem in the description of dependencies for 
the CSIPHY in the dts for the 8996..

I.e. the CSIPHY requires some clocks and power-rails to be switched on ah..

static const struct resources csiphy_res_8x96[] = {
         /* CSIPHY0 */
         {
                 .regulators = {},
                 .clock = { "top_ahb", "ispif_ahb", "ahb", 
"csiphy0_timer" },


should probably look something like

static const struct resources csiphy_res_8x96[] = {
         /* CSIPHY0 */
         {
                 .regulators = {},
                 .clock = { "top_ahb", "ispif_ahb", "ahb", 
"csiphy0_timer", "vfe0"},

But basically yeah, we haven't modeled the dependency to the CAMSS_GDSC 
via the VFEx

Hmm wait - why haven't we included the CAMSS_GDSC in the dtsi for the 8996 ?

git diff
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi 
b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 30257c07e1279..60e5d3f5336d4 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -2120,7 +2120,8 @@ camss: camss@a00000 {
                                 "vfe0",
                                 "vfe1";
                         power-domains = <&mmcc VFE0_GDSC>,
-                                       <&mmcc VFE1_GDSC>;
+                                       <&mmcc VFE1_GDSC>,
+                                       <&mmcc CAMSS_GDSC>;

Either of those approaches should mitigate this patch.

---
bod
  

On 26.05.2023 23:17, Bryan O'Donoghue wrote:
> On 26/05/2023 21:57, Konrad Dybcio wrote:
>> This code contains a whole bunch of hacky counting logic that should have
>> been substituted with _byname, but now we're stuck with indices to keep
>> compatibility with old DTs :/
>>
>> If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
>> CAMSS-related GDSCs, we could make it their parent in the clock driver
>> and call it a day.
> 
> I mean, it wouldn't make much sense from a hw design POV if that weren't the case..
> 
> Hmm looks like its already there.
> 
> static struct gdsc vfe0_gdsc = {
>         .gdscr = 0x3664,
>         .cxcs = (unsigned int []){ 0x36a8 },
>         .cxc_count = 1,
>         .pd = {
>                 .name = "vfe0",
>         },
>         .parent = &camss_gdsc.pd,
>         .pwrsts = PWRSTS_OFF_ON,
> };
> 
> static struct gdsc vfe1_gdsc = {
>         .gdscr = 0x3674,
>         .cxcs = (unsigned int []){ 0x36ac },
>         .cxc_count = 1,
>         .pd = {
>                 .name = "vfe1",
>         },
>         .parent = &camss_gdsc.pd,
>         .pwrsts = PWRSTS_OFF_ON,
> };
> 
> I feel this is probably a problem in the description of dependencies for the CSIPHY in the dts for the 8996..
> 
> I.e. the CSIPHY requires some clocks and power-rails to be switched on ah..
> 
> static const struct resources csiphy_res_8x96[] = {
>         /* CSIPHY0 */
>         {
>                 .regulators = {},
>                 .clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer" },
> 
> 
> should probably look something like
> 
> static const struct resources csiphy_res_8x96[] = {
>         /* CSIPHY0 */
>         {
>                 .regulators = {},
>                 .clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer", "vfe0"},
> 
> But basically yeah, we haven't modeled the dependency to the CAMSS_GDSC via the VFEx
I have little idea how CAMSS is tied together, but the VFEn clocks
are assigned in vfe_res_8x96.clock and ispif_res_8x96.clock_for_reset.

FWIW the ancient msm-3.18 doesn't reference the VFE clocks in CSIPHY.

Anyway, looks like the issue here is that we're not toggling the
GDSC early enough in cases where something that's not VFE needs it.

> 
> Hmm wait - why haven't we included the CAMSS_GDSC in the dtsi for the 8996 ?
Since both VFE GDSCs are children of CAMSS_GDSC and (as mentioned in the
commit message) the power sequencing used to be different, it just seems
to me like we've been piggybacking on lucky ordering since the introduction
of 8996 support.

For comparison, 8916 doesn't define it because it doesn't have it and newer
SoCs use TITAN.

SDM630 doesn't define it, but nobody touched it since like 2021 (except
Dmitry's fixups when he got his hands on the inforce baord) so it's
probably broken as well..


Konrad
> 
> ---
> bod
  

On 26/05/2023 22:28, Konrad Dybcio wrote:
> 
> 
> On 26.05.2023 23:17, Bryan O'Donoghue wrote:
>> On 26/05/2023 21:57, Konrad Dybcio wrote:
>>> This code contains a whole bunch of hacky counting logic that should have
>>> been substituted with _byname, but now we're stuck with indices to keep
>>> compatibility with old DTs :/
>>>
>>> If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
>>> CAMSS-related GDSCs, we could make it their parent in the clock driver
>>> and call it a day.
>>
>> I mean, it wouldn't make much sense from a hw design POV if that weren't the case..
>>
>> Hmm looks like its already there.
>>
>> static struct gdsc vfe0_gdsc = {
>>          .gdscr = 0x3664,
>>          .cxcs = (unsigned int []){ 0x36a8 },
>>          .cxc_count = 1,
>>          .pd = {
>>                  .name = "vfe0",
>>          },
>>          .parent = &camss_gdsc.pd,
>>          .pwrsts = PWRSTS_OFF_ON,
>> };
>>
>> static struct gdsc vfe1_gdsc = {
>>          .gdscr = 0x3674,
>>          .cxcs = (unsigned int []){ 0x36ac },
>>          .cxc_count = 1,
>>          .pd = {
>>                  .name = "vfe1",
>>          },
>>          .parent = &camss_gdsc.pd,
>>          .pwrsts = PWRSTS_OFF_ON,
>> };
>>
>> I feel this is probably a problem in the description of dependencies for the CSIPHY in the dts for the 8996..
>>
>> I.e. the CSIPHY requires some clocks and power-rails to be switched on ah..
>>
>> static const struct resources csiphy_res_8x96[] = {
>>          /* CSIPHY0 */
>>          {
>>                  .regulators = {},
>>                  .clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer" },
>>
>>
>> should probably look something like
>>
>> static const struct resources csiphy_res_8x96[] = {
>>          /* CSIPHY0 */
>>          {
>>                  .regulators = {},
>>                  .clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer", "vfe0"},
>>
>> But basically yeah, we haven't modeled the dependency to the CAMSS_GDSC via the VFEx
> I have little idea how CAMSS is tied together, but the VFEn clocks
> are assigned in vfe_res_8x96.clock and ispif_res_8x96.clock_for_reset.
> 
> FWIW the ancient msm-3.18 doesn't reference the VFE clocks in CSIPHY.
> 
> Anyway, looks like the issue here is that we're not toggling the
> GDSC early enough in cases where something that's not VFE needs it.
> 
>>
>> Hmm wait - why haven't we included the CAMSS_GDSC in the dtsi for the 8996 ?
> Since both VFE GDSCs are children of CAMSS_GDSC and (as mentioned in the
> commit message) the power sequencing used to be different, it just seems
> to me like we've been piggybacking on lucky ordering since the introduction
> of 8996 support.
> 
> For comparison, 8916 doesn't define it because it doesn't have it and newer
> SoCs use TITAN.
> 
> SDM630 doesn't define it, but nobody touched it since like 2021 (except
> Dmitry's fixups when he got his hands on the inforce baord) so it's
> probably broken as well..
> 
> 
> Konrad
>>
>> ---
>> bod

Hmm, so what I suggested is what Yassine has i.e. adds <&mmcc 
CAMSS_GDSC>; to the top-level camss node.

Without testing, this _looks_ right to me. I just think, like Conor 
flagged we don't need to add a dependency on the power-domain names.

I don't quite know whats the reference for downstream you are looking at 
but, just generally it is possible to waggle the CSIPHY, VFE, IFE - my 
guess would be that CAMX @ the time of 8996 always had the CAMSS_GDSC or 
the VFEx - and hence the VFE -> CAMSS_GDSC on by the time the CSIPHY cod 
ran.

Anyway my comments here still stand.

- I don't think we need to nor should be counting power-domain names
- I do think we should be adding CAMSS_GDSC to the 8996 top-level CAMSS node

Yassine ? Can you take a stab at that ?

---
bod
  

On Fri, May 26 2023 at 09:49:30 PM +01:00:00, Bryan O'Donoghue 
<bryan.odonoghue@linaro.org> wrote:
> On 26/05/2023 19:07, 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")
>> 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@protonmail.com>
>> ---
>>   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
  

On 27/05/2023 07:02, Yassine Oudjana wrote:
>> 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.

I think actually I agree with you but, I don't think you've gone far 
enough with this patch.

Now that I look at this code a bit more, it looks like we need to place 
the TITAN/CAMSS GDSC last in the list of power-domains or the magic 
indices won't work. So my suggestion to you to place the CAMSS_GDSC in 
the power-domain list wouldn't work, unless it was the last entry,..

Having magic indices doesn't make much sense to me. Aside from anything 
else we don't document or require that indexing behavior in our 
Documentation.

In fact, I'm wondering what is the use case of a vfe_lite on its own - 
without the TITAN_TOP GDSC switched on ? I'm looking at the block 
diagram of the clocks for the sm8250 the IFE_LITE is buried well inside 
of a series of other components..

The reverse OTOH holds. Full fat VFE can be collapsed individually, 
which is why they have their own GDSCs...

OK, we should get away from magic indices ASAP.

This is a good find, thank you for bringing it up.

Could you take a named pointer for the CAMSS/TITAN instead of an index ?

camss->genpd_camss_top * =
camss->genpd_vfe[] =

These have a very obvious meaning. We can read a top-level struct camss 
{} and immediately understand what is meant, whereas index = 0 doesn't 
mean anything and isn't obvious from the code anyway.

1. You're right we should introduce some kind of naming to
    break the bonds of magic indices.

    So lets do as you suggest and name the power-domains.

    However we should refactor the code to drop magic indices.

2. If and only if named power-domains are absent, fall back on
    legacy indexing. In this case we will assume legacy indexing
    assigns to our new named pointers.

3. New CAMSS dts will need to have named power-domains as a result.

---
bod