Re: [PATCH v2] scsi: ufs: add clock-scalable property for clk scaling

[Po-Wen Kao <powen.kao@mediatek.com>]

Update missing cover letter

V2 changes
- Cleaner code path as suggested. e.g. return directly
- Wrap clk_set_rate() with ufshcd_clk_set_rate() to check on "scalable".

On 5/7/22 12:57 PM, Po-Wen Kao wrote:
> In clk scaling, clk_set_rate is invoked to set dedicated PLL clk rate
> on scale up and down. On some MTK platform, scaling is only possible
> by switching parent of a mux, therefore we introduce a new
> "clock-scalable" property to gain fine control over which clock rate can
> be scaled by calling clk_set_rate. If a clock is defined as non-scalable,
> clk_set_rate won't be invoked while clki->current_rate is still updated
> if min/max freq is defined. Customized clk operation may be embedded in
> pre/post change of hba->vops->clk_scale_notify.
>
> All clocks in dts without "clock-scalable" property defined are assumed
> scalable and works as usual. There is no neeed to make change on existing
> dts.
>
> To specify scalable property, one may define dts as follow
>
> &ufshci {
>      clocks =
>          <clk1>,
>          <clk2>,
>          ...
>          <clkn>;
>
>      clock-names =
>          "clk1",
>          "clk2",
>          ...
>          "clkn";
>
>      freq-table-hz =
>          <1000 2000>,
>          <0 0>,
>          ...
>          <0 0>;
>
>      clock-scalable =
>          <0
>           0
>           ...
>           0
>          >;
> };
>
> Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
> ---
>   drivers/scsi/ufs/ufshcd-pltfrm.c | 96 +++++++++++++++++++++++---------
>   drivers/scsi/ufs/ufshcd.c        | 54 +++++++-----------
>   drivers/scsi/ufs/ufshcd.h        |  2 +
>   include/trace/events/ufs.h       | 16 +++---
>   4 files changed, 102 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index 87975d1a21c8..5f95bb519485 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -18,6 +18,56 @@
>   
>   #define UFSHCD_DEFAULT_LANES_PER_DIRECTION		2
>   
> +/**
> + * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
> + * shape check.
> + * @hba: per-adapter instance
> + * @propname: name of property
> + * @cols: column count
> + * @rows: calculated row count with given cols
> + * @array: u32 array pointer of property data with propname
> + */
> +static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
> +				    size_t cols, size_t *rows, u32 **array)
> +{
> +	struct device *dev = hba->dev;
> +	struct device_node *np = dev->of_node;
> +	int len = 0, ret = 0;
> +	int _rows = 0;
> +
> +	if (!of_get_property(np, propname, &len)) {
> +		dev_warn(dev, "%s property not specified.\n", propname);
> +		return -EINVAL;
> +	}
> +
> +	len = len / sizeof(**array);
> +	if (!cols || !len || len % cols != 0) {
> +		dev_warn(dev, "%s property define error.\n", propname);
> +		return -EINVAL;
> +	}
> +	_rows = len / cols;
> +
> +	*array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
> +	if (!*array) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = of_property_read_u32_array(np, propname, *array, len);
> +	if (ret) {
> +		dev_err(dev, "%s: error reading array %d\n",
> +			propname, ret);
> +		goto out;
> +	}
> +
> +	*rows = _rows;
> +
> +	return 0;
> +out:
> +	devm_kfree(dev, *array);
> +	return ret;
> +}
> +
>   static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>   {
>   	int ret = 0;
> @@ -27,13 +77,14 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>   	struct device_node *np = dev->of_node;
>   	char *name;
>   	u32 *clkfreq = NULL;
> +	u32 *scalable = NULL;
>   	struct ufs_clk_info *clki;
> -	int len = 0;
>   	size_t sz = 0;
>   
>   	if (!np)
>   		goto out;
>   
> +	/* clock-names */
>   	cnt = of_property_count_strings(np, "clock-names");
>   	if (!cnt || (cnt == -EINVAL)) {
>   		dev_info(dev, "%s: Unable to find clocks, assuming enabled\n",
> @@ -47,37 +98,29 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>   	if (cnt <= 0)
>   		goto out;
>   
> -	if (!of_get_property(np, "freq-table-hz", &len)) {
> -		dev_info(dev, "freq-table-hz property not specified\n");
> +	/* clock-scalable (optional) */
> +	ret = ufshcd_get_clk_u32_array(hba, "clock-scalable", 1, &sz, &scalable);
> +	if (ret) {
> +		dev_warn(dev, "Optional property %s load failed. Assume all clocks scalable.\n",
> +			 "clock-scalable");
> +	} else if (sz != cnt) {
> +		dev_err(dev, "%s len mismatch\n", "clock-scalable");
> +		ret = -EINVAL;
>   		goto out;
>   	}
>   
> -	if (len <= 0)
> +	/* freq-table-hz */
> +	ret = ufshcd_get_clk_u32_array(hba, "freq-table-hz", 2, &sz, &clkfreq);
> +	if (ret) {
> +		dev_err(dev, "Property %s load failed.", "freq-table-hz");
>   		goto out;
> -
> -	sz = len / sizeof(*clkfreq);
> -	if (sz != 2 * cnt) {
> +	} else if (sz != cnt) {
>   		dev_err(dev, "%s len mismatch\n", "freq-table-hz");
>   		ret = -EINVAL;
>   		goto out;
>   	}
>   
> -	clkfreq = devm_kcalloc(dev, sz, sizeof(*clkfreq),
> -			       GFP_KERNEL);
> -	if (!clkfreq) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
> -	ret = of_property_read_u32_array(np, "freq-table-hz",
> -			clkfreq, sz);
> -	if (ret && (ret != -EINVAL)) {
> -		dev_err(dev, "%s: error reading array %d\n",
> -				"freq-table-hz", ret);
> -		return ret;
> -	}
> -
> -	for (i = 0; i < sz; i += 2) {
> +	for (i = 0; i < cnt * 2; i += 2) {
>   		ret = of_property_read_string_index(np,
>   				"clock-names", i/2, (const char **)&name);
>   		if (ret)
> @@ -92,6 +135,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>   		clki->min_freq = clkfreq[i];
>   		clki->max_freq = clkfreq[i+1];
>   		clki->name = devm_kstrdup(dev, name, GFP_KERNEL);
> +		clki->scalable = scalable ? !!scalable[i / 2] : true;
>   		if (!clki->name) {
>   			ret = -ENOMEM;
>   			goto out;
> @@ -99,11 +143,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>   
>   		if (!strcmp(name, "ref_clk"))
>   			clki->keep_link_active = true;
> -		dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
> -				clki->min_freq, clki->max_freq, clki->name);
> +		dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
> +			clki->name, clki->scalable, clki->min_freq, clki->max_freq);
>   		list_add_tail(&clki->list, &hba->clk_list_head);
>   	}
>   out:
> +	devm_kfree(dev, scalable);
> +	devm_kfree(dev, clkfreq);
>   	return ret;
>   }
>   
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index eea5af5fa166..8dbae2f73329 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -939,6 +939,11 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>   		return false;
>   }
>   
> +static inline int ufshcd_clk_set_rate(struct ufs_clk_info *clki, unsigned long rate)
> +{
> +	return clki->scalable ? clk_set_rate(clki->clk, rate) : 0;
> +}
> +
>   /**
>    * ufshcd_set_clk_freq - set UFS controller clock frequencies
>    * @hba: per adapter instance
> @@ -950,6 +955,7 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>   static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
>   {
>   	int ret = 0;
> +	u32 target_rate = 0;
>   	struct ufs_clk_info *clki;
>   	struct list_head *head = &hba->clk_list_head;
>   
> @@ -958,41 +964,23 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
>   
>   	list_for_each_entry(clki, head, list) {
>   		if (!IS_ERR_OR_NULL(clki->clk)) {
> -			if (scale_up && clki->max_freq) {
> -				if (clki->curr_freq == clki->max_freq)
> -					continue;
> -
> -				ret = clk_set_rate(clki->clk, clki->max_freq);
> -				if (ret) {
> -					dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> -						__func__, clki->name,
> -						clki->max_freq, ret);
> -					break;
> -				}
> -				trace_ufshcd_clk_scaling(dev_name(hba->dev),
> -						"scaled up", clki->name,
> -						clki->curr_freq,
> -						clki->max_freq);
> -
> -				clki->curr_freq = clki->max_freq;
> +			target_rate = scale_up ? clki->max_freq : clki->min_freq;
>   
> -			} else if (!scale_up && clki->min_freq) {
> -				if (clki->curr_freq == clki->min_freq)
> -					continue;
> +			if (!target_rate || clki->curr_freq == target_rate)
> +				continue;
>   
> -				ret = clk_set_rate(clki->clk, clki->min_freq);
> -				if (ret) {
> -					dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> -						__func__, clki->name,
> -						clki->min_freq, ret);
> -					break;
> -				}
> -				trace_ufshcd_clk_scaling(dev_name(hba->dev),
> -						"scaled down", clki->name,
> -						clki->curr_freq,
> -						clki->min_freq);
> -				clki->curr_freq = clki->min_freq;
> +			ret = ufshcd_clk_set_rate(clki, target_rate);
> +			if (ret) {
> +				dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> +					__func__, clki->name,
> +					target_rate, ret);
> +				break;
>   			}
> +			trace_ufshcd_clk_scaling(dev_name(hba->dev),
> +						 clki->name, scale_up,
> +						 clki->curr_freq,
> +						 target_rate);
> +			clki->curr_freq = target_rate;
>   		}
>   		dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
>   				clki->name, clk_get_rate(clki->clk));
> @@ -8364,7 +8352,7 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
>   			ufshcd_parse_dev_ref_clk_freq(hba, clki->clk);
>   
>   		if (clki->max_freq) {
> -			ret = clk_set_rate(clki->clk, clki->max_freq);
> +			ret = ufshcd_clk_set_rate(clki, clki->max_freq);
>   			if (ret) {
>   				dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
>   					__func__, clki->name,
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 1637f5cc1420..eaa3d6c5b9ab 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -260,6 +260,7 @@ struct ufs_dev_cmd {
>    * @keep_link_active: indicates that the clk should not be disabled if
>   		      link is active
>    * @enabled: variable to check against multiple enable/disable
> + * @scalable: indicates if clk is scalable in clk scaling
>    */
>   struct ufs_clk_info {
>   	struct list_head list;
> @@ -270,6 +271,7 @@ struct ufs_clk_info {
>   	u32 curr_freq;
>   	bool keep_link_active;
>   	bool enabled;
> +	bool scalable;
>   };
>   
>   enum ufs_notify_change_status {
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 599739ee7b20..780c964e52c2 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -103,30 +103,30 @@ TRACE_EVENT(ufshcd_clk_gating,
>   
>   TRACE_EVENT(ufshcd_clk_scaling,
>   
> -	TP_PROTO(const char *dev_name, const char *state, const char *clk,
> -		u32 prev_state, u32 curr_state),
> +	TP_PROTO(const char *dev_name, const char *clk,
> +		 bool up, u32 prev_state, u32 curr_state),
>   
> -	TP_ARGS(dev_name, state, clk, prev_state, curr_state),
> +	TP_ARGS(dev_name, clk, up, prev_state, curr_state),
>   
>   	TP_STRUCT__entry(
>   		__string(dev_name, dev_name)
> -		__string(state, state)
>   		__string(clk, clk)
> +		__field(bool, up)
>   		__field(u32, prev_state)
>   		__field(u32, curr_state)
>   	),
>   
>   	TP_fast_assign(
>   		__assign_str(dev_name, dev_name);
> -		__assign_str(state, state);
>   		__assign_str(clk, clk);
> +		__entry->up = up;
>   		__entry->prev_state = prev_state;
>   		__entry->curr_state = curr_state;
>   	),
>   
> -	TP_printk("%s: %s %s from %u to %u Hz",
> -		__get_str(dev_name), __get_str(state), __get_str(clk),
> -		__entry->prev_state, __entry->curr_state)
> +	TP_printk("%s: scaled %s %s from %u to %u Hz",
> +		  __get_str(dev_name), __entry->up ? "up" : "down", __get_str(clk),
> +		  __entry->prev_state, __entry->curr_state)
>   );
>   
>   TRACE_EVENT(ufshcd_auto_bkops_state,