From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [PATCH v3 05/11] clk: tegra: prepare dfll driver for PWM regulator Date: Thu, 8 Mar 2018 22:50:06 +0000 Message-ID: <011f626d-8bd5-3cae-bfbf-f49a1b2833fb@nvidia.com> References: <1517934852-23255-1-git-send-email-pdeschrijver@nvidia.com> <1517934852-23255-6-git-send-email-pdeschrijver@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1517934852-23255-6-git-send-email-pdeschrijver@nvidia.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Peter De Schrijver , linux-tegra@vger.kernel.org, linux-clk@vger.kernel.org, mturquette@baylibre.com, sboyd@codeaurora.org, robh+dt@kernel.org, mark.rutland@arm.com, devicetree@vger.kernel.org, lgirdwood@gmail.com, broonie@kernel.org, linux-kernel@vger.kernel.org List-Id: devicetree@vger.kernel.org On 06/02/18 16:34, Peter De Schrijver wrote: > This patch prepares the dfll driver to work with PWM regulators. > To do this we introduce a new array lut_uv which gives the voltage for > a given index generated by the dfll logic. This index will then be > translated to a PMIC voltage ID in case of I2C using the i2c_lut. In case > of a PWM regulator, it will be used to determine the PWM duty cycle. > We also introduce lut_bottom which holds the lowest voltage we will ever > need. In case of I2C this can be set to zero because the i2c_lut will be > initialized such that entry 0 will be the lowest voltage we will ever > need. In case of PWM, the lowest voltage is determined by the regulator > hardware so we need this software limit. Note that this is different > from lut_min which gives the lowest voltage we allow taking temperature > into account. In a future patchset we will update lut_vmin dynamically. > Similarly lut_max will be the highest voltage allowed taking temperature > into accouint. Also this will be updated dynamically once temperature > dependence will be introduced. > > Signed-off-by: Peter De Schrijver > --- > drivers/clk/tegra/clk-dfll.c | 62 ++++++++++++++++++++++++++------------------ > 1 file changed, 37 insertions(+), 25 deletions(-) > > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c > index 0a7deee..fa97763 100644 > --- a/drivers/clk/tegra/clk-dfll.c > +++ b/drivers/clk/tegra/clk-dfll.c > @@ -301,9 +301,10 @@ struct tegra_dfll { > u32 i2c_slave_addr; > > /* i2c_lut array entries are regulator framework selectors */ > - unsigned i2c_lut[MAX_DFLL_VOLTAGES]; > - int i2c_lut_size; > - u8 lut_min, lut_max, lut_safe; > + unsigned int i2c_lut[MAX_DFLL_VOLTAGES]; > + unsigned int lut_uv[MAX_DFLL_VOLTAGES]; > + int lut_size; > + u8 lut_bottom, lut_min, lut_max, lut_safe; > }; > > #define clk_hw_to_dfll(_hw) container_of(_hw, struct tegra_dfll, dfll_clk_hw) > @@ -531,10 +532,10 @@ static void dfll_load_i2c_lut(struct tegra_dfll *td) > u32 val; > > for (i = 0; i < MAX_DFLL_VOLTAGES; i++) { > - if (i < td->lut_min) > - lut_index = td->lut_min; > - else if (i > td->lut_max) > - lut_index = td->lut_max; > + if (i < td->lut_bottom) > + lut_index = td->lut_bottom; > + else if (i > td->lut_size - 1) > + lut_index = td->lut_size - 1; > else > lut_index = i; > > @@ -594,9 +595,9 @@ static void dfll_init_out_if(struct tegra_dfll *td) > { > u32 val; > > - td->lut_min = 0; > - td->lut_max = td->i2c_lut_size - 1; > - td->lut_safe = td->lut_min + 1; > + td->lut_min = td->lut_bottom; > + td->lut_max = td->lut_size - 1; > + td->lut_safe = td->lut_min + (td->lut_min < td->lut_max ? 1 : 0); > > dfll_i2c_writel(td, 0, DFLL_OUTPUT_CFG); > val = (td->lut_safe << DFLL_OUTPUT_CFG_SAFE_SHIFT) | > @@ -619,11 +620,11 @@ static void dfll_init_out_if(struct tegra_dfll *td) > */ > > /** > - * find_lut_index_for_rate - determine I2C LUT index for given DFLL rate > + * find_lut_index_for_rate - determine LUT index for given DFLL rate > * @td: DFLL instance > * @rate: clock rate > * > - * Determines the index of a I2C LUT entry for a voltage that approximately > + * Determines the index of a LUT entry for a voltage that approximately > * produces the given DFLL clock rate. This is used when forcing a value > * to the integrator during rate changes. Returns -ENOENT if a suitable > * LUT index is not found. > @@ -637,11 +638,11 @@ static int find_lut_index_for_rate(struct tegra_dfll *td, unsigned long rate) > if (IS_ERR(opp)) > return PTR_ERR(opp); > > - uv = dev_pm_opp_get_voltage(opp); > + uv = dev_pm_opp_get_voltage(opp) / td->soc->alignment.step_uv; > dev_pm_opp_put(opp); > > - for (i = 0; i < td->i2c_lut_size; i++) { > - if (regulator_list_voltage(td->vdd_reg, td->i2c_lut[i]) == uv) > + for (i = td->lut_bottom; i < td->lut_size; i++) { > + if ((td->lut_uv[i] / td->soc->alignment.step_uv) >= uv) > return i; > } > > @@ -1377,15 +1378,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_mult, align_mult; > > + align_mult = 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_mult = regulator_list_voltage(td->vdd_reg, i) / > + td->soc->alignment.step_uv; > + if (reg_mult < 0) > break; > > - if (uV == reg_uV) > + if (align_mult == reg_mult) > return i; > } > > @@ -1399,15 +1402,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_mult, align_mult; > > + align_mult = 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_mult = regulator_list_voltage(td->vdd_reg, i) / > + td->soc->alignment.step_uv; > + if (reg_mult < 0) > break; > > - if (uV <= reg_uV) > + if (align_mult <= reg_mult) > return i; > } > > @@ -1450,8 +1455,10 @@ static int dfll_build_i2c_lut(struct tegra_dfll *td) > if (lut < 0) > goto out; > td->i2c_lut[0] = lut; > + td->lut_bottom = 0; > > for (j = 1, rate = 0; ; rate++) { > + > opp = dev_pm_opp_find_freq_ceil(td->soc->dev, &rate); > if (IS_ERR(opp)) > break; > @@ -1484,13 +1491,18 @@ static int dfll_build_i2c_lut(struct tegra_dfll *td) > if (v >= v_max) > break; > } > - td->i2c_lut_size = j; > + td->lut_size = j; > > if (!td->dvco_rate_min) > dev_err(td->dev, "no opp above DFLL minimum voltage %d mV\n", > td->soc->cvb->min_millivolts); > - else > + else { > ret = 0; > + for (j = 0; j < td->lut_size; j++) > + td->lut_uv[j] = > + regulator_list_voltage(td->vdd_reg, > + td->i2c_lut[j]); > + } > > out: > return ret; > I am a bit confused by this patch as I don't fully understand from the description what is being changed. For example, there are a few places where you are dividing by td->soc->alignment.step_uv, which seems to be changing the calculations/behaviour for I2C unless I am missing something? I would also consider renaming lut_bottom and lut_min as something like lut_abs_min and lut_cur_min, to indicate that one is the absolute min and the other is the current min depending on operating conditions. Cheers Jon -- nvpublic