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

[Bryan O'Donoghue]

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