Re: [PATCH v2 2/2] clk: en7523: add support for .set_rate for en7581

[Brian Masney]

Hi Christian,

> Add support for EN7523 driver to configure rate for Airoha EN7581 SoC.
> The SoC expose both base clock selector and clock divisor hence it's
> possible to change the rate.
> 
> This will be especially needed for new SoC AN7583 that require changes
> for the MDIO and the eMMC.
> 
> The clock were assumed correctly configured by the bootloader but this
> goes against the rule of "kernel should not depend on external
> configuration".
> 
> Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> ---
>  drivers/clk/clk-en7523.c | 136 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 136 insertions(+)
>
> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index aab4306ae037..5ce4dc692ab2 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -777,8 +777,144 @@ static unsigned long en7581_recalc_rate(struct clk_hw *hw,
>  	return rate / en7523_get_div(desc, val);
>  }
>  
> +static int en7581_get_base_val_for_rate(const struct en_clk_desc *desc,
> +					int div, unsigned long rate)
> +{
> +	int i;
> +
> +	/* Single base rate */
> +	if (!desc->base_bits) {
> +		if (rate != desc->base_value / div)
> +			goto err;
> +
> +		return 0;
> +	}
> +
> +	/* Check every base rate with provided divisor */
> +	for (i = 0; i < desc->n_base_values; i++)
> +		if (rate == desc->base_values[i] / div)
> +			return i;
> +
> +err:
> +	return -EINVAL;
> +}

Can this be updated to use the common functions in clk-divider.c like
divider_get_val() rather than rolling this same functionality in this
driver?

> +
> +static int en7581_get_vals_for_rate(const struct en_clk_desc *desc,
> +				    unsigned long rate,
> +				    u32 *base_val, u32 *div_val)
> +{
> +	int tmp_base_val = 0;
> +	int tmp_div_val = 0;
> +
> +	if (!desc->base_bits && !desc->div_bits)
> +		return -EINVAL;
> +
> +	/* Divisor not supported, just search in base rate */
> +	if (!desc->div_bits) {
> +		tmp_base_val = en7581_get_base_val_for_rate(desc, 1, rate);
> +		if (tmp_base_val < 0) {
> +			pr_err("Invalid rate for clock %s\n",
> +			       desc->name);
> +			return -EINVAL;
> +		}
> +
> +		goto exit;
> +	}
> +
> +	/* Check if div0 satisfy the request */
> +	if (desc->div_val0) {
> +		tmp_base_val = en7581_get_base_val_for_rate(desc,
> +							    desc->div_val0,
> +							    rate);
> +		if (tmp_base_val >= 0)
> +			goto exit;
> +
> +		/* Skip checking first divisor val */
> +		tmp_div_val = 1;
> +	}
> +
> +	/* Simulate rate with every divisor supported */
> +	for (; tmp_div_val < BIT(desc->div_bits); tmp_div_val++) {
> +		int div = (tmp_div_val + desc->div_offset) * desc->div_step;
> +
> +		tmp_base_val = en7581_get_base_val_for_rate(desc, div,
> +							    rate);
> +		if (tmp_base_val >= 0)
> +			goto exit;
> +	}
> +
> +	if (tmp_div_val == BIT(desc->div_bits)) {

This is always true, right?

> +		pr_err("Invalid rate for clock %s\n",
> +		       desc->name);
> +		return -EINVAL;
> +	}
> +
> +exit:
> +	*base_val = tmp_base_val;
> +	*div_val = tmp_div_val;
> +
> +	return 0;
> +}
> +
> +static int en7581_determine_rate(struct clk_hw *hw,
> +				 struct clk_rate_request *req)
> +{
> +	struct en_clk *en_clk = container_of(hw, struct en_clk, hw);
> +	u32 div_val, base_val;
> +
> +	/* Just check if the rate is possible */
> +	return en7581_get_vals_for_rate(en_clk->desc, req->rate,
> +					&base_val, &div_val);
> +}
> +
> +static int en7581_set_rate(struct clk_hw *hw, unsigned long rate,
> +			   unsigned long parent_rate)
> +{
> +	struct en_clk *en_clk = container_of(hw, struct en_clk, hw);
> +	const struct en_clk_desc *desc = en_clk->desc;
> +	struct regmap *map = en_clk->map;
> +	u32 base_val, div_val;
> +	u32 reg, val, mask;
> +	int err;
> +
> +	err = en7581_get_vals_for_rate(en_clk->desc, rate,
> +				       &base_val, &div_val);
> +	if (err)
> +		return err;
> +
> +	if (desc->div_bits) {
> +		reg = desc->div_reg ? desc->div_reg : desc->base_reg;
> +
> +		mask = (BIT(desc->div_bits) - 1) << desc->div_shift;
> +		val = div_val << desc->div_shift;
> +
> +		err = regmap_update_bits(map, reg, mask, val);
> +		if (err) {
> +			pr_err("Failed to update div reg for clock %s\n",
> +			       desc->name);
> +			return -EINVAL;

return err;

> +		}
> +	}
> +
> +	if (desc->base_bits) {
> +		mask = (BIT(desc->base_bits) - 1) << desc->base_shift;
> +		val = base_val << desc->base_shift;
> +
> +		err = regmap_update_bits(map, desc->base_reg, mask, val);
> +		if (err) {
> +			pr_err("Failed to update reg for clock %s\n",
> +			       desc->name);
> +			return -EINVAL;

return err;

Brian

-- 
Brian Masney <masneyb@xxxxxxxxxx>