On 26.05.2023 22:49, Bryan O'Donoghue 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.
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
