Message-ID: <500c7924-c43c-4233-1688-f8b6fbbad66f@linaro.org>
Date:   Sat, 11 Mar 2023 01:54:47 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Georgi Djakov <djakov@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks



On 11.03.2023 01:16, Bryan O'Donoghue wrote:
> On 10/03/2023 18:05, Konrad Dybcio wrote:
>>> I think you could use assigned-clocks for that ..
>>>
>>> So its not part of your series but then presumably you have a follow-on patch for the 8998 dts that points your ->intf_clks at these then ?
>>>
>>> clocks = <&clock_gcc clk_aggre2_noc_clk>,
>>>           <&clock_gcc clk_gcc_ufs_axi_clk>,
>>>           <&clock_gcc clk_gcc_aggre2_ufs_axi_clk>;
>>>
>>> It seems like the right thing to do..
>> Why so? We're passing all clock references to clocks=<> and we handle
>> them separately. This is akin to treating the "core" clock differently
>> to the "iface" clock on some IP block, except on a wider scale.
> 
> Eh, that was a question, not a statement. I mean to ask if your intf_clks are intended to be populated with some/all of the above additional gcc references ?
Argh sorry, I'm not great at reading lately..

Yes that would be the plan. All of these ones from downstream plus more
that we discover as we go (if we choose to go this route)

And the discovery process looks like:

1. boot with "disabling clk %s" debug prints added
2. do something that triggers a crash (e.g. wait for an IP block
   to enter its suspend routine)
3. crash horribly
4. try to recover the last disabled clock's name or analyze WCGW
5. try resolving the clock dependency
6. goto 1 until you don't crash anymore

> 
>>> Still not clear why these clocks are off.. your bootchain ?
>> Generally the issue is that icc sync_states goes over *all the possible
>> interconnect paths on the entire SoC*. For QoS register access, you need
>> to be able to interface them. Some of the hardware blocks live on a
>> separate sort of "island". That' part of why clocks such as
>> GCC_CFG_NOC_USB3_PRIM_AXI_CLK exist. They're responsible for clocking
>> the CNoC<->USB connection, which in the bigger picture looks more or less
>> like:
>>
>>
>>      1     2-3            2-3    3-4            3-4    4-5
>> USB<->CNoC<->CNoC_to_SNoC<->SNoC<->SNoC_to_BIMC<->BIMC<->APSS
>>
>> where:
>>
>> 1 = GCC_CFG_NOC_USB3_PRIM_AXI_CLK
>> 2 = RPM CNOC CLK
>> 3 = RPM SNOC CLK
>> 4 = RPM BIMC CLK
>> 5 = (usually internally managed) HMSS / GNOC CLK
>>
>> or something along these lines, the *NoC names may be in the wrong order
>> but this is the general picture.
>>
>> On msm-4.x there is no such thing as sync_state. The votes are only
>> cast from within the IP-block drivers themselves, using data gathered from
>> qcom,msmbus-blahblah and msmbus calls from within the driver. That way,
>> downstream ensures there's never unclocked QoS register access.
>>
>> After writing this entire email, I got an idea that we could consider not
>> accessing these QoS registers from within sync_state (e.g. use sth like
>> if(is_sync_state_done))..
>>
>> That would lead to this patch being mostly
>> irrelevant (IF AND ONLY IF all the necessary clocks were handled by the
>> end device drivers AND clk/icc voting was done in correct order - which
>> as we can tell from the sad 8996 example, is not always the case).
>>
>> Not guaranteeing it (like this patch does) would make it worse from the
>> standpoint of representing the hardware needs properly, but it could
>> surely save some headaches. To an extent.
> 
> Hmm.
> 
> If I have understood you correctly above, then for some of the NoC QoS entries we basically need additional clocks, which is separate to the clocks the controller bus and bus_a clocks.
Yes.

(Technically we need them for all of the nodes, but for example we're not
going to poke at DDR registers when the DDR is shut off)

> 
> We don't see the problem all the time because of sync_state,
No, actually it's sync_state (+ improper sequencing of turning on clocks
and regulators/power domains vs setting ICC BW in device drivers) that
causes this. Downstream piggybacks off of the devices being already
set up and ready to go before setting BW (and by extension QoS).

so your question is should we push the clocks to the devices.
Yes, it's either this patch and blindly finding the required clocks

or skipping QoS on sync_state (I haven't tested whether it'd enough
yet, FWIW) with this patch and less finding required clocks (as we
could partially piggyback off of the clocks being enabled before we
cast ICC votes as part of resume/init sequence of a driver)

or being like downstream and (almost) only relying on the client devices

Based on the dtsi you gave as an example, my €0.02 would say no, those are clearly additional clock dependencies for NoC.
Yes, definitely. The question is how should be go about handling them
that would:

- scale for all the IP blocks and nodes
- not be overly complex
- preferably doesn't just rely on trial-and-error and educated guesses
- be hard to mess up on the programmer's side

> 
> Going by the name, you'd expect the UFS controller could own these two clocks
> 
> "clk-gcc-ufs-axi-clk",
> "clk-aggre2-ufs-axi-clk-no-rate"
> 
> but even then by the same logic the NoC should own "clk-aggre2-noc-clk-no-rate" I wouldn't much fancy splitting them apart.
Yeah, it seems to be a common pattern: whichever on-die block is mentioned
first in the name (or first after the clock controller's prefix), actually
"owns" the clock

> 
> So - I'd say the commit log doesn't really explain to me what we have discussed here.
> 
> Suggest rewording it a little bit "non-scaling clock" is accurate but for me on the first read doesn't really tell me that these are node-specific clock dependencies or that there is an expectation that the intf_clks should be tied to node-specific clocks.
> 
> So two asks
> 
> - Update the commit log and potentially the structure comments
I'm probably just very biased because I authored these commits, but I can't
see which part is not clear.. Could I (and this is not passive-aggressive or
anything) ask for a pointer there?

> - Can you split the devm_kzalloc() into a seperate patch ?
>   I don't see why this needs to be done but if it does need to be
>   done it could as far as I read it be done before this patch
bus_clks[] is no longer a trailing empty array (a.k.a "struct hack"),
so we shouldn't be using struct_size anymore. This sort of needs to
be done together, as e.g. 8996 (without this commit) expects >2 bus
clocks which are in reality these "interface clocks", or "node access
dependencies", so splitting that out would introduce unnecessary churn
or break bisecting.

Konrad
> 
> ---
> bod

Powered by blists - more mailing lists