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

[Konrad Dybcio]

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