Date:   Sat, 11 Mar 2023 00:16:18 +0000
From:   Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        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 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 ?

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

We don't see the problem all the time because of sync_state, so your 
question is should we push the clocks to the devices. Based on the dtsi 
you gave as an example, my €0.02 would say no, those are clearly 
additional clock dependencies for NoC.

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.

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

---
bod

Powered by blists - more mailing lists