Re: [PATCH v5 5/5] i2c: qcom-cci: Enforce the required CCI clock rate

[Andi Shyti]

Hi Loic,

...

> @@ -318,7 +319,7 @@ static const struct hw_params *cci_get_hw_params(struct cci *cci, int mode)
>  	unsigned long rate = clk_get_rate(cci->cci_clk);
>  	int ri = cci_clk_rate_idx(rate);
>  
> -	if (ri >= 0 && mode <= cci->data->max_mode && cci_hw_params[ri][mode].thigh)
> +	if (ri >= 0 && mode <= cci->data->max_mode)

why are you removing the .thigh check here?

>  		return &cci_hw_params[ri][mode];
>  
>  	return NULL;

...

> +static int cci_set_core_rate(struct cci *cci)
> +{
> +	struct device *dev = cci->dev;
> +	unsigned long rate;
> +	int ret;
> +
> +	ret = cci_get_required_rate(cci, &rate);
> +	if (ret) {
> +		dev_err(dev, "no CCI clock rate satisfies all masters\n");
> +		return ret;
> +	}
> +
> +	ret = dev_pm_opp_set_rate(dev, rate);
> +	if (ret) {
> +		dev_warn(dev, "CCI clock could not be set to %lu Hz\n", rate);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Sanity: The hw_params timings are only valid at the exact
> +	 * expected rate, verify what landed on the hardware.
> +	 */
> +	if (clk_get_rate(cci->cci_clk) != rate)
> +		dev_warn(dev, "CCI clock is not at expected %lu Hz\n", rate);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused cci_suspend_runtime(struct device *dev)
>  {
>  	struct cci *cci = dev_get_drvdata(dev);
> @@ -678,6 +739,19 @@ static int cci_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(cci->cci_clk),
>  				     "failed to get CCI clock\n");
>  
> +	ret = devm_pm_opp_set_clkname(dev, "cci");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set CCI OPP clk\n");
> +
> +	/* OPP table is optional */
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to add OPP table\n");
> +
> +	ret = cci_set_core_rate(cci);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set CCI clock rate\n");

The error logging is a bit messed up here: you either print here
or in cci_set_core_rate(). Besides why is a warning there and an
error here? I would just print error in cci_set_core_rate() and
return here.

Andi

> +