Re: [PATCH v2 2/6] clk: tegra: DT align parameter for CVB calculation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 31, 2018 at 10:43:04AM +0000, Jon Hunter wrote:
> 
> On 24/01/18 12:45, Peter De Schrijver wrote:
> > When using a PWM controlled regulator, the voltage step and offset are
> > determined by the regulator IC in use. This is specified in DT rather
> > than fixed in the CVB table. Hence pass this information to the CVB table
> > calculation function. For backwards compatibility the table values are used
> > if the corresponding parameter is 0.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrijver@xxxxxxxxxx>
> > ---
> >  drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 23 +++++++++++++++++++++--
> >  drivers/clk/tegra/cvb.c                    | 18 ++++++++++++++----
> >  drivers/clk/tegra/cvb.h                    |  5 +++--
> >  3 files changed, 38 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> > index 440eb8d..6205ce1 100644
> > --- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> > +++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> > @@ -111,6 +111,7 @@ static int tegra124_dfll_fcpu_probe(struct platform_device *pdev)
> >  	struct tegra_dfll_soc_data *soc;
> >  	const struct of_device_id *of_id;
> >  	const struct dfll_fcpu_data *fcpu_data;
> > +	struct rail_alignment align;
> >  
> >  	of_id = of_match_device(tegra124_dfll_fcpu_of_match, &pdev->dev);
> >  	fcpu_data = of_id->data;
> > @@ -135,12 +136,30 @@ static int tegra124_dfll_fcpu_probe(struct platform_device *pdev)
> >  		return -ENODEV;
> >  	}
> >  
> > +	err = of_property_read_u32(pdev->dev.of_node, "nvidia,align-offset-uv",
> > +					&align.offset_uv);
> > +	if (err < 0) {
> > +		dev_err(&pdev->dev,
> > +			"offset uv not found, default to table value\n");
> > +		align.offset_uv = 0;
> > +	}
> > +
> > +	err = of_property_read_u32(pdev->dev.of_node, "nvidia,align-step-uv",
> > +					&align.step_uv);
> > +	if (err < 0) {
> > +		dev_err(&pdev->dev,
> > +			"step uv not found, default to table value\n");
> > +		align.step_uv = 0;
> > +	}
> > +
> 
> I am a bit confused by this ...
> 
> 1. Isn't this going to break Tegra124 DFLL support?

We fall back to the original behaviour in case the properties are missing, so
it should work.

> 2. These DT properties are not defined anywhere (so the binding doc
>    needs updating).
> 3. I don't see any patches in this series that populate these properties
>    in DT.
> 
> So I am not sure how this works?!?
> 

The DT updates are indeed missing.

> >  	soc->max_freq = fcpu_data->cpu_max_freq_table[speedo_id];
> >  
> >  	soc->cvb = tegra_cvb_add_opp_table(soc->dev, fcpu_data->cpu_cvb_tables,
> >  					   fcpu_data->cpu_cvb_tables_size,
> > -					   process_id, speedo_id, speedo_value,
> > -					   soc->max_freq);
> > +					   &align, process_id, speedo_id,
> > +					   speedo_value, soc->max_freq);
> > +	soc->alignment = align;
> > +
> 
> This is not defined yet and so breaks compile.
> 
> drivers/clk/tegra/clk-tegra124-dfll-fcpu.c: In function
> ‘tegra124_dfll_fcpu_probe’:
> drivers/clk/tegra/clk-tegra124-dfll-fcpu.c:161:5: error: ‘struct
> tegra_dfll_soc_data’ has no member named ‘alignment’
>   soc->alignment = align;
>      ^
> 
> What about Tegra124? This is set to NULL? Can we not set this before

It's a struct copy, not a pointer. And both values in the struct will indeed
be set to 0 then.

> calling tegra_cvb_add_opp_table() and then just pass soc->alignment for
> both Tegra124 and Tegra210? Then you can avoid the if-statements in
> build_opp_table.
> 

The table to be used is determined by tegra_cvb_add_opp_table() based on speedo
and process ID. So this logic would have to be duplicated in
tegra124_dfll_fcpu_probe() then. In retrospect it was probably a mistake to
put these regulator parameters in the CVB table, but I think we're stuck
with this unless we want to force people to also update their DT.

> >  	if (IS_ERR(soc->cvb)) {
> >  		dev_err(&pdev->dev, "couldn't add OPP table: %ld\n",
> >  			PTR_ERR(soc->cvb));
> > diff --git a/drivers/clk/tegra/cvb.c b/drivers/clk/tegra/cvb.c
> > index da9e8e7..561012a 100644
> > --- a/drivers/clk/tegra/cvb.c
> > +++ b/drivers/clk/tegra/cvb.c
> > @@ -62,11 +62,19 @@ static int round_voltage(int mv, const struct rail_alignment *align, int up)
> >  }
> >  
> >  static int build_opp_table(struct device *dev, const struct cvb_table *table,
> > +			   struct rail_alignment *align,
> >  			   int speedo_value, unsigned long max_freq)
> >  {
> > -	const struct rail_alignment *align = &table->alignment;
> >  	int i, ret, dfll_mv, min_mv, max_mv;
> >  
> > +	if (!align->step_uv)
> > +		align->step_uv = table->alignment.step_uv;
> > +	if (!align->step_uv)
> > +		return -EINVAL;
> > +
> > +	if (!align->offset_uv)
> > +		align->offset_uv = table->alignment.offset_uv;
> > +
> >  	min_mv = round_voltage(table->min_millivolts, align, UP);
> >  	max_mv = round_voltage(table->max_millivolts, align, DOWN);
> >  
> > @@ -109,8 +117,9 @@ static int build_opp_table(struct device *dev, const struct cvb_table *table,
> >   */
> >  const struct cvb_table *
> >  tegra_cvb_add_opp_table(struct device *dev, const struct cvb_table *tables,
> > -			size_t count, int process_id, int speedo_id,
> > -			int speedo_value, unsigned long max_freq)
> > +			size_t count, struct rail_alignment *align,
> > +			int process_id, int speedo_id, int speedo_value,
> > +			unsigned long max_freq)
> >  {
> >  	size_t i;
> >  	int ret;
> > @@ -124,7 +133,8 @@ static int build_opp_table(struct device *dev, const struct cvb_table *table,
> >  		if (table->process_id != -1 && table->process_id != process_id)
> >  			continue;
> >  
> > -		ret = build_opp_table(dev, table, speedo_value, max_freq);
> > +		ret = build_opp_table(dev, table, align, speedo_value,
> > +				      max_freq);
> >  		return ret ? ERR_PTR(ret) : table;
> >  	}
> >  
> > diff --git a/drivers/clk/tegra/cvb.h b/drivers/clk/tegra/cvb.h
> > index c1f0779..cfa110f 100644
> > --- a/drivers/clk/tegra/cvb.h
> > +++ b/drivers/clk/tegra/cvb.h
> > @@ -59,8 +59,9 @@ struct cvb_table {
> >  
> >  const struct cvb_table *
> >  tegra_cvb_add_opp_table(struct device *dev, const struct cvb_table *cvb_tables,
> > -			size_t count, int process_id, int speedo_id,
> > -			int speedo_value, unsigned long max_freq);
> > +			size_t count, struct rail_alignment *align,
> > +			int process_id, int speedo_id, int speedo_value,
> > +			unsigned long max_freq);
> >  void tegra_cvb_remove_opp_table(struct device *dev,
> >  				const struct cvb_table *table,
> >  				unsigned long max_freq);
> > 
> 
> Cheers
> Jon
> 
> -- 
> nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux