Re: [PATCH 17/20] interconnect: qcom: icc-rpm: Control bus rpmcc from icc

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 30.05.2023 12:20, Konrad Dybcio wrote:
> The sole purpose of bus clocks that were previously registered with
> rpmcc was to convey the aggregated bandwidth to RPM. There's no good
> reason to keep them outside the interconnect framework, as it only
> adds to the plentiful complexity.
> 
> Add the required code to handle these clocks from within SMD RPM ICC.
> 
> RPM-owned bus clocks are no longer considered a thing, but sadly we
> have to allow for the existence of HLOS-owned bus clocks, as some
> (mostly older) SoCs (ab)use these for bus scaling (e.g. MSM8998 and
> &mmcc AHB_CLK_SRC).
> 
> This in turn is trivially solved with a single *clk, which is filled
> and used iff qp.bus_clk_desc is absent and we have a "bus" clock-names
> entry in the DT node.
> 
> This change should(tm) be fully compatible with all sorts of old
> Device Trees as far as the interconnect functionality goes (modulo
> abusing bus clock handles, but that's a mistake in and of itself).
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
[...]

> +	/* Some providers don't have a bus clock to scale */
> +	if (!qp->bus_clk_desc)
if (!qp->bus_clk_desc && !qp->bus_clk)

Konrad
> +		return 0;
> +
> +	/* Intentionally keep the rates in kHz as that's what RPM accepts */
> +	active_rate = max(agg_avg[QCOM_SMD_RPM_ACTIVE_STATE],
> +			  agg_peak[QCOM_SMD_RPM_ACTIVE_STATE]);
> +	do_div(active_rate, src_qn->buswidth);
> +
> +	sleep_rate = max(agg_avg[QCOM_SMD_RPM_SLEEP_STATE],
> +			 agg_peak[QCOM_SMD_RPM_SLEEP_STATE]);
> +	do_div(sleep_rate, src_qn->buswidth);
> +
> +	/*
> +	 * Downstream checks whether the requested rate is zero, but it makes little sense
> +	 * to vote for a value that's below the lower threshold, so let's not do so.
> +	 */
> +	if (qp->keep_alive)
> +		active_rate = max(ICC_BUS_CLK_MIN_RATE, active_rate);
> +
> +	/* Some providers have a non-RPM-owned bus clock - convert kHz->Hz for the CCF */
> +	if (qp->bus_clk)
> +		return clk_set_rate(qp->bus_clk, 1000ULL * max(active_rate, sleep_rate));
> +
> +	/* RPM only accepts <=INT_MAX rates */
> +	active_rate = min_t(u32, active_rate, INT_MAX);
> +	sleep_rate = min_t(u32, sleep_rate, INT_MAX);
> +
> +	if ((active_rate != qp->bus_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE]) ||
> +	    (sleep_rate != qp->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE])) {
> +		ret = qcom_icc_rpm_set_bus_rate(qp->bus_clk_desc,
> +						active_rate,
> +						sleep_rate);
> +		if (ret)
>  			return ret;
> -		}
> -		qp->bus_clk_rate[i] = rate;
>  	}
>  
> +	/* Cache the rate after we've successfully commited it to RPM */
> +	qp->bus_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE] = active_rate;
> +	qp->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE] = sleep_rate;
> +
>  	return 0;
>  }
>  
> -static const char * const bus_clocks[] = {
> -	"bus", "bus_a",
> -};
> -
>  int qnoc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -448,6 +448,18 @@ int qnoc_probe(struct platform_device *pdev)
>  	if (!qp->intf_clks)
>  		return -ENOMEM;
>  
> +	if (desc->bus_clk_desc) {
> +		qp->bus_clk_desc = devm_kzalloc(dev, sizeof(*qp->bus_clk_desc),
> +						GFP_KERNEL);
> +		if (!qp->bus_clk_desc)
> +			return -ENOMEM;
> +
> +		qp->bus_clk_desc = desc->bus_clk_desc;
> +	} else if (!IS_ERR(devm_clk_get(dev, "bus"))) {
> +		/* Some older SoCs may have a single non-RPM-owned bus clock. */
> +		qp->bus_clk = devm_clk_get(dev, "bus");
> +	}
> +
>  	data = devm_kzalloc(dev, struct_size(data, nodes, num_nodes),
>  			    GFP_KERNEL);
>  	if (!data)
> @@ -457,10 +469,6 @@ int qnoc_probe(struct platform_device *pdev)
>  	for (i = 0; i < cd_num; i++)
>  		qp->intf_clks[i].id = cds[i];
>  
> -	qp->num_bus_clks = desc->no_clk_scaling ? 0 : NUM_BUS_CLKS;
> -	for (i = 0; i < qp->num_bus_clks; i++)
> -		qp->bus_clks[i].id = bus_clocks[i];
> -
>  	qp->keep_alive = desc->keep_alive;
>  	qp->type = desc->type;
>  	qp->qos_offset = desc->qos_offset;
> @@ -490,13 +498,11 @@ int qnoc_probe(struct platform_device *pdev)
>  	}
>  
>  regmap_done:
> -	ret = devm_clk_bulk_get(dev, qp->num_bus_clks, qp->bus_clks);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_bulk_prepare_enable(qp->num_bus_clks, qp->bus_clks);
> -	if (ret)
> -		return ret;
> +	if (qp->bus_clk) {
> +		ret = clk_prepare_enable(qp->bus_clk);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = devm_clk_bulk_get(dev, qp->num_intf_clks, qp->intf_clks);
>  	if (ret)
> @@ -566,7 +572,8 @@ int qnoc_probe(struct platform_device *pdev)
>  	icc_provider_deregister(provider);
>  err_remove_nodes:
>  	icc_nodes_remove(provider);
> -	clk_bulk_disable_unprepare(qp->num_bus_clks, qp->bus_clks);
> +	if (qp->bus_clk)
> +		clk_disable_unprepare(qp->bus_clk);
>  
>  	return ret;
>  }
> @@ -578,7 +585,8 @@ int qnoc_remove(struct platform_device *pdev)
>  
>  	icc_provider_deregister(&qp->provider);
>  	icc_nodes_remove(&qp->provider);
> -	clk_bulk_disable_unprepare(qp->num_bus_clks, qp->bus_clks);
> +	if (qp->bus_clk)
> +		clk_disable_unprepare(qp->bus_clk);
>  
>  	return 0;
>  }
> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> index e3df066fd94e..2c8c0399378b 100644
> --- a/drivers/interconnect/qcom/icc-rpm.h
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -36,32 +36,29 @@ struct rpm_clk_resource {
>  	bool branch;
>  };
>  
> -#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 (0 or 2)
>   * @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
>   * @bus_clk_rate: bus clock rate in Hz
> - * @bus_clks: the clk_bulk_data table of bus clocks
> + * @bus_clk_desc: a pointer to a rpm_clk_resource description of bus clocks
>   * @intf_clks: a clk_bulk_data array of interface clocks
> + * @bus_clk: a pointer to a HLOS-owned bus clock
>   * @qos_offset: offset to QoS registers
>   * @keep_alive: whether to always keep a minimum vote on the bus 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;
>  	int qos_offset;
> -	u64 bus_clk_rate[NUM_BUS_CLKS];
> -	struct clk_bulk_data bus_clks[NUM_BUS_CLKS];
> +	u32 bus_clk_rate[QCOM_SMD_RPM_STATE_NUM];
>  	const struct rpm_clk_resource *bus_clk_desc;
> +	struct clk *bus_clk;
>  	struct clk_bulk_data *intf_clks;
>  	bool keep_alive;
>  	bool is_on;
> @@ -118,12 +115,10 @@ struct qcom_icc_node {
>  struct qcom_icc_desc {
>  	struct qcom_icc_node * const *nodes;
>  	size_t num_nodes;
> -	const char * const *bus_clocks;
>  	const struct rpm_clk_resource *bus_clk_desc;
>  	const char * const *intf_clocks;
>  	size_t num_intf_clocks;
>  	bool keep_alive;
> -	bool no_clk_scaling;
>  	enum qcom_icc_type type;
>  	const struct regmap_config *regmap_cfg;
>  	int qos_offset;
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index a596f4035d2e..8081b3cb1025 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -1818,7 +1818,6 @@ static const struct qcom_icc_desc msm8996_a0noc = {
>  	.num_nodes = ARRAY_SIZE(a0noc_nodes),
>  	.intf_clocks = a0noc_intf_clocks,
>  	.num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks),
> -	.no_clk_scaling = true,
>  	.regmap_cfg = &msm8996_a0noc_regmap_config
>  };
>  
> diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
> index 5743ed680e8e..211fa1fa569c 100644
> --- a/drivers/interconnect/qcom/sdm660.c
> +++ b/drivers/interconnect/qcom/sdm660.c
> @@ -1618,7 +1618,6 @@ static const struct qcom_icc_desc sdm660_gnoc = {
>  	.nodes = sdm660_gnoc_nodes,
>  	.num_nodes = ARRAY_SIZE(sdm660_gnoc_nodes),
>  	.regmap_cfg = &sdm660_gnoc_regmap_config,
> -	.no_clk_scaling = true,
>  };
>  
>  static struct qcom_icc_node * const sdm660_mnoc_nodes[] = {
>