Messages in this thread | | | Date | Sat, 11 Mar 2023 16:01:24 +0200 | Subject | Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks | From | Dmitry Baryshkov <> |
| |
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, > }; > >
-- With best wishes Dmitry
| |