From: Yassine Oudjana <[email protected]>
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 <[email protected]>
---
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);
--
2.40.1
On 26/05/2023 19:07, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
<[email protected]> wrote:
> On 26/05/2023 19:07, Yassine Oudjana wrote:
>> From: Yassine Oudjana <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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