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
