Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Konrad Dybcio]

On 10.03.2023 17:47, Bryan O'Donoghue wrote:
> On 10/03/2023 14:26, Konrad Dybcio wrote:
>>
>>
>> On 10.03.2023 15:21, Bryan O'Donoghue wrote:
>>> On 08/03/2023 21:40, Konrad Dybcio wrote:
>>>> Some (but not all) providers (or their specific nodes) require
>>>> specific clocks to be turned on before they can be accessed. Failure
>>>> to ensure that results in a seemingly random system crash (which
>>>> would usually happen at boot with the interconnect driver built-in),
>>>> resulting in the platform not booting up properly.
>>>
>>> Can you give an example of which clocks on which SoC's ?
>> See for example 67fb53745e0b
>>
>> This was a clock documented downstream under the node-qos clocks here:
>>
>> https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.5.7.r1/arch/arm/boot/dts/qcom/msm8996-bus.dtsi#L102-L109
>>
>> but there are occasions where such clocks are undocumented and downstream
>> skips them because it relies on them being on by miracle, such as the case
>> of MASTER_IPA and the IPA rpmcc clock on msm8998. Downstream has no
>> sync_state, so they would only set the QoS registers when the relevant
>> hardware was online, so the clocks were on already.
> 
> What switched the clocks on ? Presumably LK.
> 
> Is this a symptom of using a bootloader other than LK ? If you use the same bootloader, then why hasn't the bootloader/LK already set it up on your platform ?
XBL* in this case (qcom fully switched to UEFI with 8998, I'm not
using anything other to what came on the device)

Setting up these dependencies happens all throughout the boot
chain: after the SoC starts up, BIMC/PNoC/CNoC/SNoC(/AnNoC) are
set up by some early firmware so that DDR, USB, UFS/eMMC, crypto
engines etc. can be reachable. It's *that* level of deep..

Then, they are not shut down at all, leaving USB etc. accessible
by default.

> 
>>>
>>> Is the intention of this patch to subsequently go through *.dts *.dtsi and start to remove assigned-clocks ?
>>>
>>> Are we saying that currently there ought to be assigned-clocks for some of these NoC declarations ?
>> Not really, assigned-clocks are used for static ratesetting, see
>> for example dwc3 nodes where we need it to be fast enough for
>> HS/SS operation at all times (though that should have prooobably
>> been handled in the driver but it's a separate topic), I don't
>> think any of them were used to combat what this commit tries to.
> 
> 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.

> 
> 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.

Konrad
> 
> ---
> bod