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

From: Yassine Oudjana
Date: Sat May 27 2023 - 02:02:23 EST

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 |
+ if (i > camss->vfe_num && i != camss_pd_index) {
camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],

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.