Re: [PATCH V2 08/21] clk: tegra: dfll: round down voltages based on alignment

[Joseph Lo <josephl@xxxxxxxxxx>]

On 12/13/18 7:46 PM, Jon Hunter wrote:
> On 13/12/2018 09:34, Joseph Lo wrote:
> > When generating the OPP table, the voltages are round down with the
> > alignment from the regulator. The alignment should be applied for
> > voltages look up as well.
> >
> > Based on the work of Penny Chiu <pchiu@xxxxxxxxxx>.
> >
> > Signed-off-by: Joseph Lo <josephl@xxxxxxxxxx>
> > ---
> > *V2:
> >   - s/align_volt/align_step/
> >   - s/reg_volt/reg_volt_id/
> > ---
> >   drivers/clk/tegra/clk-dfll.c | 26 +++++++++++++++-----------
> >   1 file changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> > index 72e02898006c..b3668073d9b4 100644
> > --- a/drivers/clk/tegra/clk-dfll.c
> > +++ b/drivers/clk/tegra/clk-dfll.c
> > @@ -804,17 +804,17 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >   static int find_lut_index_for_rate(struct tegra_dfll *td, unsigned long rate)
> >   {
> >   	struct dev_pm_opp *opp;
> > -	int i, uv;
> > +	int i, align_step;
> >   
> >   	opp = dev_pm_opp_find_freq_ceil(td->soc->dev, &rate);
> >   	if (IS_ERR(opp))
> >   		return PTR_ERR(opp);
> >   
> > -	uv = dev_pm_opp_get_voltage(opp);
> > +	align_step = dev_pm_opp_get_voltage(opp) / td->soc->alignment.step_uv;
> >   	dev_pm_opp_put(opp);
> >   
> >   	for (i = td->lut_bottom; i < td->lut_size; i++) {
> > -		if (td->lut_uv[i] >= uv)
> > +		if ((td->lut_uv[i] / td->soc->alignment.step_uv) >= align_step)
> >   			return i;
> >   	}
> >   
> > @@ -1532,15 +1532,17 @@ static int dfll_init(struct tegra_dfll *td)
> >    */
> >   static int find_vdd_map_entry_exact(struct tegra_dfll *td, int uV)
> >   {
> > -	int i, n_voltages, reg_uV;
> > +	int i, n_voltages, reg_volt_id, align_step;
> >   
> > +	align_step = uV / td->soc->alignment.step_uv;
> >   	n_voltages = regulator_count_voltages(td->vdd_reg);
> >   	for (i = 0; i < n_voltages; i++) {
> > -		reg_uV = regulator_list_voltage(td->vdd_reg, i);
> > -		if (reg_uV < 0)
> > +		reg_volt_id = regulator_list_voltage(td->vdd_reg, i) /
> > +			      td->soc->alignment.step_uv;
> > +		if (reg_volt_id < 0)
>
> I don't think that this will work. If the step is say 10000 and we
> return an error code greater than -10000, we will end up with 0.

Ah,I think you mean when the error code smaller than step_uv. Yes, it 
will be 0 in that case. I think I will just remove the 'if' clause, then 
it will not match anything and leave the for loop. And return error if 
it couldn't find anything. Same as below.

Thanks.

> >   			break;
> >   
> > -		if (uV == reg_uV)
> > +		if (align_step == reg_volt_id)
> >   			return i;
> >   	}
> >   
> > @@ -1554,15 +1556,17 @@ static int find_vdd_map_entry_exact(struct tegra_dfll *td, int uV)
> >    * */
> >   static int find_vdd_map_entry_min(struct tegra_dfll *td, int uV)
> >   {
> > -	int i, n_voltages, reg_uV;
> > +	int i, n_voltages, reg_volt_id, align_step;
> >   
> > +	align_step = uV / td->soc->alignment.step_uv;
> >   	n_voltages = regulator_count_voltages(td->vdd_reg);
> >   	for (i = 0; i < n_voltages; i++) {
> > -		reg_uV = regulator_list_voltage(td->vdd_reg, i);
> > -		if (reg_uV < 0)
> > +		reg_volt_id = regulator_list_voltage(td->vdd_reg, i) /
> > +			      td->soc->alignment.step_uv;
> > +		if (reg_volt_id < 0)
>
> Same here.
>
> >   			break;
> >   
> > -		if (uV <= reg_uV)
> > +		if (align_step <= reg_volt_id)
> >   			return i;
> >   	}
> >