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

Bryan O'Donoghue
2023-05-26T21:38:54Z

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