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

  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.

Limit the number of bus_clocks to 2 (which is the maximum that SMD
RPM interconnect supports anyway) and handle non-scaling clocks
separately. Update MSM8996 and SDM660 drivers to make sure they do
not regress with this change.

This unfortunately has to be done in one patch to prevent either
compile errors or broken bisect.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
 drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
 drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
 drivers/interconnect/qcom/sdm660.c  | 16 +++++-------
 4 files changed, 70 insertions(+), 34 deletions(-)
  

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 ?

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 ?

---
bod
  

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.

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

Konrad
> 
> ---
> bod
  

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 ?

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

Still not clear why these clocks are off.. your bootchain ?

---
bod
  

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
  

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
  

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
  

On 11/03/2023 00:54, Konrad Dybcio wrote:
>> - 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?
> 

I mean to say "non scaling clocks" isn't an indicator IMO of the fact 
that these are QoS node specific clocks.

Right now the interconnect model is predicated on bus and bus_a but, 
you've found that on some SoCs we have node-specific clocks too.

:g/non\ scaling/s//non-scaling\ node-specific/g

would do or "QoS node-specific" the fact the clocks don't scale is 
incidental the dependency though is that IMO at least these are 
additional node-specific clocks we need to enable.

---
bod
  

On 11.03.2023 13:11, Bryan O'Donoghue wrote:
> On 11/03/2023 00:54, Konrad Dybcio wrote:
>>> - 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?
>>
> 
> I mean to say "non scaling clocks" isn't an indicator IMO of the fact that these are QoS node specific clocks.
> 
> Right now the interconnect model is predicated on bus and bus_a but, you've found that on some SoCs we have node-specific clocks too.
> 
> :g/non\ scaling/s//non-scaling\ node-specific/g
> 
> would do or "QoS node-specific" the fact the clocks don't scale is incidental the dependency though is that IMO at least these are additional node-specific clocks we need to enable.
Okay that makes sense. Thanks.

Konrad
> 
> ---
> bod
  

On Sat, 11 Mar 2023 at 14:11, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 11/03/2023 00:54, Konrad Dybcio wrote:
> >> - 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?
> >
>
> I mean to say "non scaling clocks" isn't an indicator IMO of the fact
> that these are QoS node specific clocks.
>
> Right now the interconnect model is predicated on bus and bus_a but,
> you've found that on some SoCs we have node-specific clocks too.
>
> :g/non\ scaling/s//non-scaling\ node-specific/g
>
> would do or "QoS node-specific" the fact the clocks don't scale is
> incidental the dependency though is that IMO at least these are
> additional node-specific clocks we need to enable.

This looks somewhat close to what we have observed in the patches for
ipq9574 platform. It doesn't have a scaling interconnect (in other
words, no bus clocks), but some devices have clocks driving the NIU
(Network Interface Units) which connect the device to NoC.

>
> ---
> bod




--
With best wishes
Dmitry
  

On 08/03/2023 23: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.
> 
> Limit the number of bus_clocks to 2 (which is the maximum that SMD
> RPM interconnect supports anyway) and handle non-scaling clocks
> separately. Update MSM8996 and SDM660 drivers to make sure they do
> not regress with this change.
> 
> This unfortunately has to be done in one patch to prevent either
> compile errors or broken bisect.

Can we determine somehow if the intf clocks are required for the whole 
NoC or just for a single node? I don't think that clocks like a0noc_ufs 
are requiered to be up for the whole aggre_noc. Instead they probably 
have to be enabled only when UFS makes use of the NoC (in other words 
when is has voted for the bandwidth).

> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
>   drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
>   drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
>   drivers/interconnect/qcom/sdm660.c  | 16 +++++-------
>   4 files changed, 70 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index b52f788d8f3d..ca932ed720fb 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -369,6 +369,17 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>   
>   	qcom_icc_bus_aggregate(provider, agg_avg, agg_peak, &max_agg_avg);
>   
> +	/* If we're powering on the bus, ensure the necessary clocks are on */
> +	if (unlikely(!qp->is_on)) {
> +		if (agg_peak[0] || agg_peak[1] || max_agg_avg) {
> +			/* If this fails, bus accesses will crash the platform! */
> +			ret = clk_bulk_prepare_enable(qp->num_intf_clks, qp->intf_clks);
> +			if (ret)
> +				return ret;
> +			qp->is_on = true;
> +		}
> +	}
> +
>   	sum_bw = icc_units_to_bps(max_agg_avg);
>   
>   	ret = __qcom_icc_set(src, src_qn, sum_bw);
> @@ -409,6 +420,14 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>   		qp->bus_clk_rate[i] = rate;
>   	}
>   
> +	/* Turn off the interface clocks if the bus was shut down so as not to leak power */
> +	if (!qp->bus_clk_rate[0] && !qp->bus_clk_rate[1]) {
> +		if (!agg_peak[0] && !agg_peak[1] && !max_agg_avg) {
> +			clk_bulk_disable_unprepare(qp->num_intf_clks, qp->intf_clks);
> +			qp->is_on = false;
> +		}
> +	}
> +
>   	return 0;
>   }
>   
> @@ -441,21 +460,20 @@ int qnoc_probe(struct platform_device *pdev)
>   	qnodes = desc->nodes;
>   	num_nodes = desc->num_nodes;
>   
> -	if (desc->num_bus_clocks) {
> -		cds = desc->bus_clocks;
> -		cd_num = desc->num_bus_clocks;
> +	if (desc->num_intf_clocks) {
> +		cds = desc->intf_clocks;
> +		cd_num = desc->num_intf_clocks;
>   	} else {
> -		cds = bus_clocks;
> -		cd_num = ARRAY_SIZE(bus_clocks);
> +		/* 0 intf clocks is perfectly fine */
> +		cd_num = 0;
>   	}
>   
> -	qp = devm_kzalloc(dev, struct_size(qp, bus_clks, cd_num), GFP_KERNEL);
> +	qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
>   	if (!qp)
>   		return -ENOMEM;
>   
> -	qp->bus_clk_rate = devm_kcalloc(dev, cd_num, sizeof(*qp->bus_clk_rate),
> -					GFP_KERNEL);
> -	if (!qp->bus_clk_rate)
> +	qp->intf_clks = devm_kzalloc(dev, sizeof(qp->intf_clks), GFP_KERNEL);
> +	if (!qp->intf_clks)
>   		return -ENOMEM;
>   
>   	data = devm_kzalloc(dev, struct_size(data, nodes, num_nodes),
> @@ -463,6 +481,18 @@ int qnoc_probe(struct platform_device *pdev)
>   	if (!data)
>   		return -ENOMEM;
>   
> +	qp->num_intf_clks = cd_num;
> +	for (i = 0; i < cd_num; i++)
> +		qp->intf_clks[i].id = cds[i];
> +
> +	if (desc->num_bus_clocks) {
> +		cds = desc->bus_clocks;
> +		cd_num = desc->num_bus_clocks;
> +	} else {
> +		cds = bus_clocks;
> +		cd_num = ARRAY_SIZE(bus_clocks);
> +	}
> +
>   	for (i = 0; i < cd_num; i++)
>   		qp->bus_clks[i].id = cds[i];
>   	qp->num_bus_clks = cd_num;
> @@ -503,6 +533,10 @@ int qnoc_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	ret = devm_clk_bulk_get(dev, qp->num_intf_clks, qp->intf_clks);
> +	if (ret)
> +		return ret;
> +
>   	if (desc->has_bus_pd) {
>   		ret = dev_pm_domain_attach(dev, true);
>   		goto err_disable_clks;
> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> index d4401f35f6d2..a4ef45b4a9e0 100644
> --- a/drivers/interconnect/qcom/icc-rpm.h
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -20,24 +20,32 @@ enum qcom_icc_type {
>   	QCOM_ICC_QNOC,
>   };
>   
> +#define NUM_BUS_CLKS	2
> +
>   /**
>    * struct qcom_icc_provider - Qualcomm specific interconnect provider
>    * @provider: generic interconnect provider
>    * @num_bus_clks: the total number of bus_clks clk_bulk_data entries
> + * @num_intf_clks: the total number of intf_clks clk_bulk_data entries
>    * @type: the ICC provider type
>    * @regmap: regmap for QoS registers read/write access
>    * @qos_offset: offset to QoS registers
>    * @bus_clk_rate: bus clock rate in Hz
>    * @bus_clks: the clk_bulk_data table of bus clocks
> + * @intf_clks: a clk_bulk_data array of interface clocks
> + * @is_on: whether the bus is powered on
>    */
>   struct qcom_icc_provider {
>   	struct icc_provider provider;
>   	int num_bus_clks;
> +	int num_intf_clks;
>   	enum qcom_icc_type type;
>   	struct regmap *regmap;
>   	unsigned int qos_offset;
> -	u64 *bus_clk_rate;
> -	struct clk_bulk_data bus_clks[];
> +	u64 bus_clk_rate[NUM_BUS_CLKS];
> +	struct clk_bulk_data bus_clks[NUM_BUS_CLKS];
> +	struct clk_bulk_data *intf_clks;
> +	bool is_on;
>   };
>   
>   /**
> @@ -93,6 +101,8 @@ struct qcom_icc_desc {
>   	size_t num_nodes;
>   	const char * const *bus_clocks;
>   	size_t num_bus_clocks;
> +	const char * const *intf_clocks;
> +	size_t num_intf_clocks;
>   	bool has_bus_pd;
>   	enum qcom_icc_type type;
>   	const struct regmap_config *regmap_cfg;
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index 69fc50a6fa5c..1a5e0ad36cc4 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -21,21 +21,17 @@
>   #include "smd-rpm.h"
>   #include "msm8996.h"
>   
> -static const char * const bus_mm_clocks[] = {
> -	"bus",
> -	"bus_a",
> +static const char * const mm_intf_clocks[] = {
>   	"iface"
>   };
>   
> -static const char * const bus_a0noc_clocks[] = {
> +static const char * const a0noc_intf_clocks[] = {
>   	"aggre0_snoc_axi",
>   	"aggre0_cnoc_ahb",
>   	"aggre0_noc_mpu_cfg"
>   };
>   
> -static const char * const bus_a2noc_clocks[] = {
> -	"bus",
> -	"bus_a",
> +static const char * const a2noc_intf_clocks[] = {
>   	"aggre2_ufs_axi",
>   	"ufs_axi"
>   };
> @@ -1821,8 +1817,8 @@ static const struct qcom_icc_desc msm8996_a0noc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = a0noc_nodes,
>   	.num_nodes = ARRAY_SIZE(a0noc_nodes),
> -	.bus_clocks = bus_a0noc_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_a0noc_clocks),
> +	.intf_clocks = a0noc_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks),
>   	.has_bus_pd = true,
>   	.regmap_cfg = &msm8996_a0noc_regmap_config
>   };
> @@ -1866,8 +1862,8 @@ static const struct qcom_icc_desc msm8996_a2noc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = a2noc_nodes,
>   	.num_nodes = ARRAY_SIZE(a2noc_nodes),
> -	.bus_clocks = bus_a2noc_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
> +	.intf_clocks = a2noc_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(a2noc_intf_clocks),
>   	.regmap_cfg = &msm8996_a2noc_regmap_config
>   };
>   
> @@ -2005,8 +2001,8 @@ static const struct qcom_icc_desc msm8996_mnoc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = mnoc_nodes,
>   	.num_nodes = ARRAY_SIZE(mnoc_nodes),
> -	.bus_clocks = bus_mm_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
> +	.intf_clocks = mm_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
>   	.regmap_cfg = &msm8996_mnoc_regmap_config
>   };
>   
> diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
> index a22ba821efbf..0e8a96f4ce90 100644
> --- a/drivers/interconnect/qcom/sdm660.c
> +++ b/drivers/interconnect/qcom/sdm660.c
> @@ -127,15 +127,11 @@ enum {
>   	SDM660_SNOC,
>   };
>   
> -static const char * const bus_mm_clocks[] = {
> -	"bus",
> -	"bus_a",
> +static const char * const mm_intf_clocks[] = {
>   	"iface",
>   };
>   
> -static const char * const bus_a2noc_clocks[] = {
> -	"bus",
> -	"bus_a",
> +static const char * const a2noc_intf_clocks[] = {
>   	"ipa",
>   	"ufs_axi",
>   	"aggre2_ufs_axi",
> @@ -1516,8 +1512,8 @@ static const struct qcom_icc_desc sdm660_a2noc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = sdm660_a2noc_nodes,
>   	.num_nodes = ARRAY_SIZE(sdm660_a2noc_nodes),
> -	.bus_clocks = bus_a2noc_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
> +	.intf_clocks = a2noc_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(a2noc_intf_clocks),
>   	.regmap_cfg = &sdm660_a2noc_regmap_config,
>   };
>   
> @@ -1659,8 +1655,8 @@ static const struct qcom_icc_desc sdm660_mnoc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = sdm660_mnoc_nodes,
>   	.num_nodes = ARRAY_SIZE(sdm660_mnoc_nodes),
> -	.bus_clocks = bus_mm_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
> +	.intf_clocks = mm_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
>   	.regmap_cfg = &sdm660_mnoc_regmap_config,
>   };
>   
>
  

On 11/03/2023 14:01, Dmitry Baryshkov wrote:
>>
>> Limit the number of bus_clocks to 2 (which is the maximum that SMD
>> RPM interconnect supports anyway) and handle non-scaling clocks
>> separately. Update MSM8996 and SDM660 drivers to make sure they do
>> not regress with this change.
>>
>> This unfortunately has to be done in one patch to prevent either
>> compile errors or broken bisect.
> 
> Can we determine somehow if the intf clocks are required for the whole 
> NoC or just for a single node? I don't think that clocks like a0noc_ufs 
> are requiered to be up for the whole aggre_noc. Instead they probably 
> have to be enabled only when UFS makes use of the NoC (in other words 
> when is has voted for the bandwidth).

Its probably worthwhile experimenting to see if the *ufs*_clk can/should 
be added to the UFS device list of clocks.

I hadn't realised we were talking about enabling the intf clocks always 
but, actually that is what we are talking about isn't it ?

It seems pretty unlikely that other devices would depend on clocks named 
*ufs*

OTOH "clk-aggre2-noc-clk-no-rate" may be used across different nodes, 
seems like a pretty generic name, though still suspicious it is bundled 
with UFS, likely it is only required for UFS ?!?

Konrad can you try:

1. Moving the intf_clks/QoS clocks be contained to UFS alone ?
2. If that doesn't work just the *ufs* clocks be put there with
2a. "clk-aggre2-noc-clk-no-rate" alone added to the intf_clk list

It seems wrong to be enabling ufs related NoC clocks unless we are 
actually switching on UFS.

---
bod
  

On Sat, 11 Mar 2023 at 16:29, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 11/03/2023 14:01, Dmitry Baryshkov wrote:
> >>
> >> Limit the number of bus_clocks to 2 (which is the maximum that SMD
> >> RPM interconnect supports anyway) and handle non-scaling clocks
> >> separately. Update MSM8996 and SDM660 drivers to make sure they do
> >> not regress with this change.
> >>
> >> This unfortunately has to be done in one patch to prevent either
> >> compile errors or broken bisect.
> >
> > Can we determine somehow if the intf clocks are required for the whole
> > NoC or just for a single node? I don't think that clocks like a0noc_ufs
> > are requiered to be up for the whole aggre_noc. Instead they probably
> > have to be enabled only when UFS makes use of the NoC (in other words
> > when is has voted for the bandwidth).
>
> Its probably worthwhile experimenting to see if the *ufs*_clk can/should
> be added to the UFS device list of clocks.

While we were doing this for some of the clocks (PCIe and USB, if I'm
not mistaken), I think that generally this is not fully correct. In my
opinion it should be in the interconnect driver, who turns
corresponding clocks on and off. These clocks correspond to the SoC
topology, rather than the end-device.

>
> I hadn't realised we were talking about enabling the intf clocks always
> but, actually that is what we are talking about isn't it ?
>
> It seems pretty unlikely that other devices would depend on clocks named
> *ufs*
>
> OTOH "clk-aggre2-noc-clk-no-rate" may be used across different nodes,
> seems like a pretty generic name, though still suspicious it is bundled
> with UFS, likely it is only required for UFS ?!?
>
> Konrad can you try:
>
> 1. Moving the intf_clks/QoS clocks be contained to UFS alone ?
> 2. If that doesn't work just the *ufs* clocks be put there with
> 2a. "clk-aggre2-noc-clk-no-rate" alone added to the intf_clk list
>
> It seems wrong to be enabling ufs related NoC clocks unless we are
> actually switching on UFS.
  

On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>> be added to the UFS device list of clocks.
> While we were doing this for some of the clocks (PCIe and USB, if I'm
> not mistaken), I think that generally this is not fully correct. In my
> opinion it should be in the interconnect driver, who turns
> corresponding clocks on and off. These clocks correspond to the SoC
> topology, rather than the end-device.
> 

True enough, they are interconnect clocks.

The question is how to only turn them on when the device that depends on 
them wants them.

---
bod
  

On 11/03/2023 16:38, Bryan O'Donoghue wrote:
> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>> be added to the UFS device list of clocks.
>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>> not mistaken), I think that generally this is not fully correct. In my
>> opinion it should be in the interconnect driver, who turns
>> corresponding clocks on and off. These clocks correspond to the SoC
>> topology, rather than the end-device.
>>
> 
> True enough, they are interconnect clocks.
> 
> The question is how to only turn them on when the device that depends on 
> them wants them.

I think we can turn them on an off from qcom_icc_set(). Each node can 
list required clocks.
  

Hi Konrad,

Thank you for working on this and sorry about jumping a bit late into
the discussion.

On 8.03.23 23: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.

These "interface" clocks seem to be used only to program QoS for the
respective ip block (eg ufs). So if we don't program QoS, there should
be no crashes, right?

I believe that in downstream they defer setting QoS until the first
non-zero bandwidth request because of drivers that probe asynchronously
or there is some firmware booting involved (IPA maybe). And bad stuff
might happen if we touch the clock while the firmware is still booting.
So setting the QoS on the first non-zero bandwidth request might not be
a bad idea. Such nodes should probably be also excluded from sync_state
by implementing get_bw() to return 0 bandwidth.

BR,
Georgi

> 
> Limit the number of bus_clocks to 2 (which is the maximum that SMD
> RPM interconnect supports anyway) and handle non-scaling clocks
> separately. Update MSM8996 and SDM660 drivers to make sure they do
> not regress with this change.
> 
> This unfortunately has to be done in one patch to prevent either
> compile errors or broken bisect.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
>   drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
>   drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
>   drivers/interconnect/qcom/sdm660.c  | 16 +++++-------
>   4 files changed, 70 insertions(+), 34 deletions(-)
> 
[..]
  

Hi,

On 11.03.23 17:26, Dmitry Baryshkov wrote:
> On 11/03/2023 16:38, Bryan O'Donoghue wrote:
>> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>>> be added to the UFS device list of clocks.
>>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>>> not mistaken), I think that generally this is not fully correct. In my
>>> opinion it should be in the interconnect driver, who turns
>>> corresponding clocks on and off. These clocks correspond to the SoC
>>> topology, rather than the end-device.
>>>
>>
>> True enough, they are interconnect clocks.
>>
>> The question is how to only turn them on when the device that depends on them wants them.
> 
> I think we can turn them on an off from qcom_icc_set(). Each node can list required clocks.
> 

Yes, this is a bit weird, but looks like these are the interface clocks
required for programming the qos box of the respective peripheral and
nothing else. Maybe we can even configure QoS just once (eg. on the first
bandwidth request) and not every time we call qcom_icc_set().

BR,
Georgi
  

On 21.03.2023 14:58, Georgi Djakov wrote:
> Hi,
> 
> On 11.03.23 17:26, Dmitry Baryshkov wrote:
>> On 11/03/2023 16:38, Bryan O'Donoghue wrote:
>>> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>>>> be added to the UFS device list of clocks.
>>>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>>>> not mistaken), I think that generally this is not fully correct. In my
>>>> opinion it should be in the interconnect driver, who turns
>>>> corresponding clocks on and off. These clocks correspond to the SoC
>>>> topology, rather than the end-device.
>>>>
>>>
>>> True enough, they are interconnect clocks.
>>>
>>> The question is how to only turn them on when the device that depends on them wants them.
>>
>> I think we can turn them on an off from qcom_icc_set(). Each node can list required clocks.
>>
> 
> Yes, this is a bit weird, but looks like these are the interface clocks
> required for programming the qos box of the respective peripheral and
> nothing else. Maybe we can even configure QoS just once (eg. on the first
> bandwidth request) and not every time we call qcom_icc_set().
Would that persist a full bus reset - if we e.g. shut down MMNoC 
after the display stack is turned off in preparation for a power
collapse, would we have to reprogram it?

Another thing is, do we know "how persistent" the QoS settings are?
What could reset them? Would a bandwidth request for a node that
belongs to the same path do so?

Konrad
> 
> BR,
> Georgi
  

On 21.03.2023 14:56, Georgi Djakov wrote:
> Hi Konrad,
> 
> Thank you for working on this and sorry about jumping a bit late into
> the discussion.
> 
> On 8.03.23 23: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.
> 
> These "interface" clocks seem to be used only to program QoS for the
> respective ip block (eg ufs). So if we don't program QoS, there should
> be no crashes, right?
Correct.

> 
> I believe that in downstream they defer setting QoS until the first
> non-zero bandwidth request because of drivers that probe asynchronously
> or there is some firmware booting involved (IPA maybe). 
Hmm.. that would make sense.

> And bad stuff
> might happen if we touch the clock while the firmware is still booting.
> So setting the QoS on the first non-zero bandwidth request might not be
> a bad idea.
Sounds like a plan, I don't think setting QoS parameters on buses
that are not in use does anything (citation needed).

Such nodes should probably be also excluded from sync_state
> by implementing get_bw() to return 0 bandwidth.
I agree, setting the QoS parameters in sync_state (so, without
their dependencies fully met) sounds like a recipe for disaster,
while getting rid of QoS in sync_state at all would leave the nodes
that are not explicitly referenced in the device tree unconfigured.


Konrad

> 
> BR,
> Georgi
> 
>>
>> Limit the number of bus_clocks to 2 (which is the maximum that SMD
>> RPM interconnect supports anyway) and handle non-scaling clocks
>> separately. Update MSM8996 and SDM660 drivers to make sure they do
>> not regress with this change.
>>
>> This unfortunately has to be done in one patch to prevent either
>> compile errors or broken bisect.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
>>   drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
>>   drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
>>   drivers/interconnect/qcom/sdm660.c  | 16 +++++-------
>>   4 files changed, 70 insertions(+), 34 deletions(-)
>>
> [..]
>
  

On 21.03.23 16:11, Konrad Dybcio wrote:
> 
> 
> On 21.03.2023 14:58, Georgi Djakov wrote:
>> Hi,
>>
>> On 11.03.23 17:26, Dmitry Baryshkov wrote:
>>> On 11/03/2023 16:38, Bryan O'Donoghue wrote:
>>>> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>>>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>>>>> be added to the UFS device list of clocks.
>>>>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>>>>> not mistaken), I think that generally this is not fully correct. In my
>>>>> opinion it should be in the interconnect driver, who turns
>>>>> corresponding clocks on and off. These clocks correspond to the SoC
>>>>> topology, rather than the end-device.
>>>>>
>>>>
>>>> True enough, they are interconnect clocks.
>>>>
>>>> The question is how to only turn them on when the device that depends on them wants them.
>>>
>>> I think we can turn them on an off from qcom_icc_set(). Each node can list required clocks.
>>>
>>
>> Yes, this is a bit weird, but looks like these are the interface clocks
>> required for programming the qos box of the respective peripheral and
>> nothing else. Maybe we can even configure QoS just once (eg. on the first
>> bandwidth request) and not every time we call qcom_icc_set().
> Would that persist a full bus reset - if we e.g. shut down MMNoC
> after the display stack is turned off in preparation for a power
> collapse, would we have to reprogram it?
> 
> Another thing is, do we know "how persistent" the QoS settings are?
> What could reset them? Would a bandwidth request for a node that
> belongs to the same path do so?

That's a good question. From what i recall, i expect them to persist until
you reset the board. Probably we can verify it with an experiment by reading
them back, but let me check if i can find some info.

Thanks,
Georgi
  

On 21.03.23 16:38, Georgi Djakov wrote:
> On 21.03.23 16:11, Konrad Dybcio wrote:
>>
>>
>> On 21.03.2023 14:58, Georgi Djakov wrote:
>>> Hi,
>>>
>>> On 11.03.23 17:26, Dmitry Baryshkov wrote:
>>>> On 11/03/2023 16:38, Bryan O'Donoghue wrote:
>>>>> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>>>>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>>>>>> be added to the UFS device list of clocks.
>>>>>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>>>>>> not mistaken), I think that generally this is not fully correct. In my
>>>>>> opinion it should be in the interconnect driver, who turns
>>>>>> corresponding clocks on and off. These clocks correspond to the SoC
>>>>>> topology, rather than the end-device.
>>>>>>
>>>>>
>>>>> True enough, they are interconnect clocks.
>>>>>
>>>>> The question is how to only turn them on when the device that depends on them wants them.
>>>>
>>>> I think we can turn them on an off from qcom_icc_set(). Each node can list required clocks.
>>>>
>>>
>>> Yes, this is a bit weird, but looks like these are the interface clocks
>>> required for programming the qos box of the respective peripheral and
>>> nothing else. Maybe we can even configure QoS just once (eg. on the first
>>> bandwidth request) and not every time we call qcom_icc_set().
>> Would that persist a full bus reset - if we e.g. shut down MMNoC
>> after the display stack is turned off in preparation for a power
>> collapse, would we have to reprogram it?
>>
>> Another thing is, do we know "how persistent" the QoS settings are?
>> What could reset them? Would a bandwidth request for a node that
>> belongs to the same path do so?
> 
> That's a good question. From what i recall, i expect them to persist until
> you reset the board. Probably we can verify it with an experiment by reading
> them back, but let me check if i can find some info.
> 

This seems to be hardware specific and there is no general answer. It depends
on where the reset line for the NIU comes from. It could be from the primary
chip reset in most cases, but it could be also within the power domain of the
associated core.

Thanks,
Georgi