| Message ID | 20230228-topic-qos-v7-6-815606092fff@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | The great interconnecification fixation | expand |
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 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(-) > [..]
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.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
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, };
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(-)