[v4,08/20] clk: tegra: pll: Add logic for handling SDM data

This adds logic for taking SDM_DIN (Sigma Delta Modulator) setting into
the equation to calculate the effective N value for PLL which supports
fractional divider.

The effective N = NDIV + 1/2 + SDM_DIN/2^13, where NDIV is the integer
feedback divider.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 drivers/clk/tegra/clk-pll.c |   52 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/clk/tegra/clk.h     |    9 +++++++-
 2 files changed, 59 insertions(+), 2 deletions(-)

On Mon, May 4, 2015 at 9:37 AM, Rhyland Klein <rklein@nvidia.com> wrote:
> @@ -495,6 +505,28 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
>         return 0;
>  }
>
> +static void clk_pll_set_sdm_data(struct clk_hw *hw,
> +                                struct tegra_clk_pll_freq_table *cfg)
> +{
> +       struct tegra_clk_pll *pll = to_clk_pll(hw);
> +       u32 val;
> +
> +       if (!pll->params->sdm_din_reg)
> +               return;
> +
> +       if (cfg->sdm_data) {
> +               val = pll_readl_sdm_din(pll) & (~sdm_din_mask(pll));
> +               val |= sdin_data_to_din(cfg->sdm_data) & sdm_din_mask(pll);
> +               pll_writel_sdm_din(val, pll);
> +       }
> +
> +       val = pll_readl_sdm_ctrl(pll);
> +       if (!cfg->sdm_data != !(val & pll->params->sdm_ctrl_en_mask)) {

You can use sdm_en_mask(pll) here.

I'm not super clear about what you're trying to accomplish here with
!cfg->sdm_data != !(val & mask).
Are you just checking if the masked value is different from sdm_data,
but accounting for the integer widths being different (u16 vs u32)?

> +               val ^= pll->params->sdm_ctrl_en_mask;

You can use sdm_en_mask(pll) here.

> +               pll_writel_sdm_ctrl(val, pll);
> +       }
> +}
> +

> @@ -552,6 +586,14 @@ static void _get_pll_mnp(struct tegra_clk_pll *pll,
>                 cfg->m = (val >> div_nmp->divm_shift) & divm_mask(pll);
>                 cfg->n = (val >> div_nmp->divn_shift) & divn_mask(pll);
>                 cfg->p = (val >> div_nmp->divp_shift) & divp_mask(pll);
> +
> +               if (pll->params->sdm_din_reg) {
> +                       if (sdm_en_mask(pll) & pll_readl_sdm_ctrl(pll)) {
> +                               val = pll_readl_sdm_din(pll);
> +                               cfg->sdm_data = val & sdm_din_mask(pll);
> +                               cfg->sdm_data = sdin_din_to_data(cfg->sdm_data);

Maybe this is tighter?

val = pll_readl_sdm_din(pll);
val &= sdm_din_mask(pll);
cfg->sdm_data = sdin_din_to_data(val);

> @@ -697,6 +740,9 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
>                 pdiv = 1;
>         }
>
> +       if (pll->params->set_gain)
> +               pll->params->set_gain(&cfg);
> +

It's not really clear from this commit alone how set_gain relates to
the sdm stuff.

> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> index 8e7361886cf9..eb8103ec335e 100644
> --- a/drivers/clk/tegra/clk.h
> +++ b/drivers/clk/tegra/clk.h
>  /**
> @@ -213,6 +215,10 @@ struct tegra_clk_pll_params {
>         u32             lock_enable_bit_idx;
>         u32             iddq_reg;
>         u32             iddq_bit_idx;
> +       u32             sdm_din_reg;
> +       u32             sdm_din_mask;
> +       u32             sdm_ctrl_reg;
> +       u32             sdm_ctrl_en_mask;

Please add kerneldoc for these members as well.

>         u32             aux_reg;
>         u32             dyn_ramp_reg;
>         u32             ext_misc_reg[MAX_PLL_MISC_REG_COUNT];
> @@ -227,6 +233,7 @@ struct tegra_clk_pll_params {
>         struct div_nmp  *div_nmp;
>         struct tegra_clk_pll_freq_table *freq_table;
>         unsigned long   fixed_rate;
> +       void    (*set_gain)(struct tegra_clk_pll_freq_table *cfg);

This one too.

On 5/4/2015 7:01 PM, Benson Leung wrote:
> On Mon, May 4, 2015 at 9:37 AM, Rhyland Klein <rklein@nvidia.com> wrote:
>> @@ -495,6 +505,28 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
>>         return 0;
>>  }
>>
>> +static void clk_pll_set_sdm_data(struct clk_hw *hw,
>> +                                struct tegra_clk_pll_freq_table *cfg)
>> +{
>> +       struct tegra_clk_pll *pll = to_clk_pll(hw);
>> +       u32 val;
>> +
>> +       if (!pll->params->sdm_din_reg)
>> +               return;
>> +
>> +       if (cfg->sdm_data) {
>> +               val = pll_readl_sdm_din(pll) & (~sdm_din_mask(pll));
>> +               val |= sdin_data_to_din(cfg->sdm_data) & sdm_din_mask(pll);
>> +               pll_writel_sdm_din(val, pll);
>> +       }
>> +
>> +       val = pll_readl_sdm_ctrl(pll);
>> +       if (!cfg->sdm_data != !(val & pll->params->sdm_ctrl_en_mask)) {
> 
> You can use sdm_en_mask(pll) here.
> 
> I'm not super clear about what you're trying to accomplish here with
> !cfg->sdm_data != !(val & mask).
> Are you just checking if the masked value is different from sdm_data,
> but accounting for the integer widths being different (u16 vs u32)?

So I got clarification from the downstream author to be sure, and this
is the answer to what this is checking:

(<Configuration has non zero  SDM_DATA>  AND <sdm control is disabled>)
OR
(<Configuration has zero  SDM_DATA>  AND <sdm control is enabled>)

So the check is correct, just a complicated way of expressing it.

-rhyland

On Tue, May 05, 2015 at 03:16:08PM -0400, Rhyland Klein wrote:
> On 5/4/2015 7:01 PM, Benson Leung wrote:
> > On Mon, May 4, 2015 at 9:37 AM, Rhyland Klein <rklein@nvidia.com> wrote:
> >> @@ -495,6 +505,28 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
> >>         return 0;
> >>  }
> >>
> >> +static void clk_pll_set_sdm_data(struct clk_hw *hw,
> >> +                                struct tegra_clk_pll_freq_table *cfg)
> >> +{
> >> +       struct tegra_clk_pll *pll = to_clk_pll(hw);
> >> +       u32 val;
> >> +
> >> +       if (!pll->params->sdm_din_reg)
> >> +               return;
> >> +
> >> +       if (cfg->sdm_data) {
> >> +               val = pll_readl_sdm_din(pll) & (~sdm_din_mask(pll));
> >> +               val |= sdin_data_to_din(cfg->sdm_data) & sdm_din_mask(pll);
> >> +               pll_writel_sdm_din(val, pll);
> >> +       }
> >> +
> >> +       val = pll_readl_sdm_ctrl(pll);
> >> +       if (!cfg->sdm_data != !(val & pll->params->sdm_ctrl_en_mask)) {
> > 
> > You can use sdm_en_mask(pll) here.
> > 
> > I'm not super clear about what you're trying to accomplish here with
> > !cfg->sdm_data != !(val & mask).
> > Are you just checking if the masked value is different from sdm_data,
> > but accounting for the integer widths being different (u16 vs u32)?
> 
> So I got clarification from the downstream author to be sure, and this
> is the answer to what this is checking:
> 
> (<Configuration has non zero  SDM_DATA>  AND <sdm control is disabled>)
> OR
> (<Configuration has zero  SDM_DATA>  AND <sdm control is enabled>)
> 
> So the check is correct, just a complicated way of expressing it.

Can it be rewritten to be less complicated? I hate it when I have to
look at code for several seconds and still not understand what it's
doing. Why not something that is closer to the pseudo code you gave:

	bool enabled = (val & pll->params->sdm_ctrl_en_mask) != 0;

	if ((enabled && cfg->sdm_data == 0) || (!enabled && cfg->sdm_data != 0))

? Also I think this could use some could comments explaining what's
going on. Perhaps this could be simplified even further:

	if (cfg->sdm_data == 0 && enabled)
		val &= ~pll->params->sdm_ctrl_en_mask;

	if (cfg->sdm_data != 0 && !enabled)
		val |= pll->params->sdm_ctrl_en_mask;

	pll_writel_sdm_ctrl(val, pll);

Now that I have a /much/ easier time reading and understanding. That may
not even require comments because it's pretty plain what's going on. But
there may be some advantage in adding comments about SDM in general. The
comment could be something like what you have in the commit message, so
that people don't have to go find the commit that added the code to find
out what this is doing.

Thierry

On 5/6/2015 9:57 AM, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, May 05, 2015 at 03:16:08PM -0400, Rhyland Klein wrote:
>> On 5/4/2015 7:01 PM, Benson Leung wrote:
>>> On Mon, May 4, 2015 at 9:37 AM, Rhyland Klein <rklein@nvidia.com> wrote:
>>>> @@ -495,6 +505,28 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
>>>>         return 0;
>>>>  }
>>>>
>>>> +static void clk_pll_set_sdm_data(struct clk_hw *hw,
>>>> +                                struct tegra_clk_pll_freq_table *cfg)
>>>> +{
>>>> +       struct tegra_clk_pll *pll = to_clk_pll(hw);
>>>> +       u32 val;
>>>> +
>>>> +       if (!pll->params->sdm_din_reg)
>>>> +               return;
>>>> +
>>>> +       if (cfg->sdm_data) {
>>>> +               val = pll_readl_sdm_din(pll) & (~sdm_din_mask(pll));
>>>> +               val |= sdin_data_to_din(cfg->sdm_data) & sdm_din_mask(pll);
>>>> +               pll_writel_sdm_din(val, pll);
>>>> +       }
>>>> +
>>>> +       val = pll_readl_sdm_ctrl(pll);
>>>> +       if (!cfg->sdm_data != !(val & pll->params->sdm_ctrl_en_mask)) {
>>>
>>> You can use sdm_en_mask(pll) here.
>>>
>>> I'm not super clear about what you're trying to accomplish here with
>>> !cfg->sdm_data != !(val & mask).
>>> Are you just checking if the masked value is different from sdm_data,
>>> but accounting for the integer widths being different (u16 vs u32)?
>>
>> So I got clarification from the downstream author to be sure, and this
>> is the answer to what this is checking:
>>
>> (<Configuration has non zero  SDM_DATA>  AND <sdm control is disabled>)
>> OR
>> (<Configuration has zero  SDM_DATA>  AND <sdm control is enabled>)
>>
>> So the check is correct, just a complicated way of expressing it.
> 
> Can it be rewritten to be less complicated? I hate it when I have to
> look at code for several seconds and still not understand what it's
> doing. Why not something that is closer to the pseudo code you gave:
> 
> 	bool enabled = (val & pll->params->sdm_ctrl_en_mask) != 0;
> 
> 	if ((enabled && cfg->sdm_data == 0) || (!enabled && cfg->sdm_data != 0))
> 
> ? Also I think this could use some could comments explaining what's
> going on. Perhaps this could be simplified even further:
> 
> 	if (cfg->sdm_data == 0 && enabled)
> 		val &= ~pll->params->sdm_ctrl_en_mask;
> 
> 	if (cfg->sdm_data != 0 && !enabled)
> 		val |= pll->params->sdm_ctrl_en_mask;
> 
> 	pll_writel_sdm_ctrl(val, pll);
> 
> Now that I have a /much/ easier time reading and understanding. That may
> not even require comments because it's pretty plain what's going on. But
> there may be some advantage in adding comments about SDM in general. The
> comment could be something like what you have in the commit message, so
> that people don't have to go find the commit that added the code to find
> out what this is doing.
> 
> Thierry

I agree that the original statement was confusing. I'll replace it with
a simpler version, and comments never hurt :)

-rhyland

> 
> * Unknown Key
> * 0x7F3EB3A1
>