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

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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