| Message ID | 20230526180712.8481-4-y.oudjana@protonmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: camss: Link CAMSS power domain on MSM8996 | expand |
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
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);