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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 May 2023 22:38:44 +0100
From:   Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        Yassine Oudjana <yassine.oudjana@...il.com>,
        Robert Foss <rfoss@...nel.org>,
        Todor Tomov <todor.too@...il.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Hans Verkuil <hansverk@...co.com>
Cc:     Yassine Oudjana <y.oudjana@...tonmail.com>,
        Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
        linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

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