Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1178275 > unrolled thread

Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support James Liao <jamesjj.liao@mediatek.com> - 2015-07-07 11:30 +0200
    Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support Heiko Stübner <heiko@sntech.de> - 2015-07-07 11:40 +0200
      Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support James Liao <jamesjj.liao@mediatek.com> - 2015-07-07 11:50 +0200
        Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support Heiko Stübner <heiko@sntech.de> - 2015-07-07 12:50 +0200

#1178275 — Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support

Hi Heiko,

On Tue, 2015-07-07 at 10:58 +0200, Heiko Stübner wrote:
> > +#define PLL(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits,	\
> > +			_pd_reg, _pd_shift, _tuner_reg, _pcw_reg,	\
> > +			_pcw_shift)					\
> > +		PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, \
> > +			_pd_reg, _pd_shift, _tuner_reg, _pcw_reg, _pcw_shift, \
> > +			NULL)
> > +
> > +const unsigned long mmpll_div_rate[] = {
> 
> static?

OK. I'll add it.

> 
> > +	MT8173_PLL_FMAX,
> > +	1000000000,
> > +	702000000,
> > +	253500000,
> > +	126750000,
> > +	0,
> 
> it's more common to label sentinel entries (the 0 marking the end) with
> 	/* sentinel */
> instead of value 0.

OK. I'll add it.

> 
> If I'm reading the code correctly, this is a mapping divider -> frequency, 
> right? So it may be nice to make this a bit more explicit, like:
> 
> struct mtk_pll_div_table {
> 	unsigned int    freq;
> 	unsigned int    div;
> };
> 
> static const struct mtk_pll_div_table mmpll_div_rate[] = {
> 	{ .freq = MT8173_PLL_FMAX, .div = 0 },
> 	{ .freq = 1000000000, .div = 1 },
> 	{ .freq = 702000000, .div = 2 },
> 	{ .freq = 253500000, .div = 3 },
> 	{ .freq = 126750000, .div = 4 },
> 	{ /* sentinel */ },
> };

Hmm.. OK. I'll try to use a more readable way to implement it.

> -------------
> 
> > -	u32 con1, pd, val;
> > +	u32 con1, val;
> >  	int pll_en;
> > 
> > -	/* set postdiv */
> > -	pd = readl(pll->pd_addr);
> > -	pd &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > -	pd |= (ffs(postdiv) - 1) << pll->data->pd_shift;
> > -	writel(pd, pll->pd_addr);
> > -
> >  	pll_en = readl(pll->base_addr + REG_CON0) & CON0_BASE_EN;
> > 
> > -	/* set pcw */
> > -	val = readl(pll->pcw_addr);
> > +	/* set postdiv */
> > +	val = readl(pll->pd_addr);
> > +	val &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > +	val |= (ffs(postdiv) - 1) << pll->data->pd_shift;
> > +
> > +	/* postdiv and pcw need to set at the same time if on same register */
> > +	if (pll->pd_addr != pll->pcw_addr) {
> > +		writel(val, pll->pd_addr);
> > +		val = readl(pll->pcw_addr);
> > +	}
> > 
> > +	/* set pcw */
> >  	val &= ~GENMASK(pll->data->pcw_shift + pll->data->pcwbits - 1,
> >  			pll->data->pcw_shift);
> >  	val |= pcw << pll->data->pcw_shift;
> 
> This whole block probably wants to be a separate patch ;-) .
> 
> While it may not affect previous pll implementations, it changes how register 
> accesses are handled and should not hide in another patch.

OK, I'll separate it.

> > @@ -135,16 +138,26 @@ static void mtk_pll_calc_values(struct mtk_clk_pll
> > *pll, u32 *pcw, u32 *postdiv, u32 freq, u32 fin)
> >  {
> >  	unsigned long fmin = 1000 * MHZ;
> > +	const unsigned long *div_rate = pll->data->div_rate;
> >  	u64 _pcw;
> >  	u32 val;
> > 
> >  	if (freq > pll->data->fmax)
> >  		freq = pll->data->fmax;
> > 
> > -	for (val = 0; val < 4; val++) {
> > +	if (div_rate) {
> > +		for (val = 1; div_rate[val] != 0; val++) {
> > +			if (freq > div_rate[val])
> > +				break;
> > +		}
> > +		val--;
> 
> if you're changing the table struct, this of course also would need to be 
> adapted.
> 
> 
> Hmm, what I don't understand is, what does MT8173_PLL_FMAX in the table, if 
> you ignore it here all the time?
> 
> So the table should probably look more like [when using the concept from 
> above]
> 
> static const struct mtk_pll_div_table mmpll_div_rate[] = {
> 	{ .freq = 1000000000, .div = 0 },
> 	{ .freq = 702000000, .div = 1 },
> 	{ .freq = 253500000, .div = 2 },
> 	{ .freq = 126750000, .div = 3 },
> 	{ /* sentinel */ },
> };

The freq-div table describes the maximum frequency of each divider
setting. Although the first element doesn't used in current
implementation, I think it's better to keep freq-div table's
completeness.


Best regards,

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1178279

Hi James,

Am Dienstag, 7. Juli 2015, 17:28:38 schrieb James Liao:
> On Tue, 2015-07-07 at 10:58 +0200, Heiko Stübner wrote:
> > > +#define PLL(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits,	\
> > > +			_pd_reg, _pd_shift, _tuner_reg, _pcw_reg,	\
> > > +			_pcw_shift)					\
> > > +		PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, \
> > > +			_pd_reg, _pd_shift, _tuner_reg, _pcw_reg, _pcw_shift, \
> > > +			NULL)
> > > +
> > > +const unsigned long mmpll_div_rate[] = {
> > 
> > static?
> 
> OK. I'll add it.
> 
> > > +	MT8173_PLL_FMAX,
> > > +	1000000000,
> > > +	702000000,
> > > +	253500000,
> > > +	126750000,
> > > +	0,
> > 
> > it's more common to label sentinel entries (the 0 marking the end) with
> > 
> > 	/* sentinel */
> > 
> > instead of value 0.
> 
> OK. I'll add it.
> 
> > If I'm reading the code correctly, this is a mapping divider -> frequency,
> > right? So it may be nice to make this a bit more explicit, like:
> > 
> > struct mtk_pll_div_table {
> > 
> > 	unsigned int    freq;
> > 	unsigned int    div;
> > 
> > };
> > 
> > static const struct mtk_pll_div_table mmpll_div_rate[] = {
> > 
> > 	{ .freq = MT8173_PLL_FMAX, .div = 0 },
> > 	{ .freq = 1000000000, .div = 1 },
> > 	{ .freq = 702000000, .div = 2 },
> > 	{ .freq = 253500000, .div = 3 },
> > 	{ .freq = 126750000, .div = 4 },
> > 	{ /* sentinel */ },
> > 
> > };
> 
> Hmm.. OK. I'll try to use a more readable way to implement it.
> 
> > -------------
> > 
> > > -	u32 con1, pd, val;
> > > +	u32 con1, val;
> > > 
> > >  	int pll_en;
> > > 
> > > -	/* set postdiv */
> > > -	pd = readl(pll->pd_addr);
> > > -	pd &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > > -	pd |= (ffs(postdiv) - 1) << pll->data->pd_shift;
> > > -	writel(pd, pll->pd_addr);
> > > -
> > > 
> > >  	pll_en = readl(pll->base_addr + REG_CON0) & CON0_BASE_EN;
> > > 
> > > -	/* set pcw */
> > > -	val = readl(pll->pcw_addr);
> > > +	/* set postdiv */
> > > +	val = readl(pll->pd_addr);
> > > +	val &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > > +	val |= (ffs(postdiv) - 1) << pll->data->pd_shift;
> > > +
> > > +	/* postdiv and pcw need to set at the same time if on same register 
*/
> > > +	if (pll->pd_addr != pll->pcw_addr) {
> > > +		writel(val, pll->pd_addr);
> > > +		val = readl(pll->pcw_addr);
> > > +	}
> > > 
> > > +	/* set pcw */
> > > 
> > >  	val &= ~GENMASK(pll->data->pcw_shift + pll->data->pcwbits - 1,
> > >  	
> > >  			pll->data->pcw_shift);
> > >  	
> > >  	val |= pcw << pll->data->pcw_shift;
> > 
> > This whole block probably wants to be a separate patch ;-) .
> > 
> > While it may not affect previous pll implementations, it changes how
> > register accesses are handled and should not hide in another patch.
> 
> OK, I'll separate it.
> 
> > > @@ -135,16 +138,26 @@ static void mtk_pll_calc_values(struct mtk_clk_pll
> > > *pll, u32 *pcw, u32 *postdiv, u32 freq, u32 fin)
> > > 
> > >  {
> > >  
> > >  	unsigned long fmin = 1000 * MHZ;
> > > 
> > > +	const unsigned long *div_rate = pll->data->div_rate;
> > > 
> > >  	u64 _pcw;
> > >  	u32 val;
> > >  	
> > >  	if (freq > pll->data->fmax)
> > >  	
> > >  		freq = pll->data->fmax;
> > > 
> > > -	for (val = 0; val < 4; val++) {
> > > +	if (div_rate) {
> > > +		for (val = 1; div_rate[val] != 0; val++) {
> > > +			if (freq > div_rate[val])
> > > +				break;
> > > +		}
> > > +		val--;
> > 
> > if you're changing the table struct, this of course also would need to be
> > adapted.
> > 
> > 
> > Hmm, what I don't understand is, what does MT8173_PLL_FMAX in the table,
> > if
> > you ignore it here all the time?
> > 
> > So the table should probably look more like [when using the concept from
> > above]
> > 
> > static const struct mtk_pll_div_table mmpll_div_rate[] = {
> > 
> > 	{ .freq = 1000000000, .div = 0 },
> > 	{ .freq = 702000000, .div = 1 },
> > 	{ .freq = 253500000, .div = 2 },
> > 	{ .freq = 126750000, .div = 3 },
> > 	{ /* sentinel */ },
> > 
> > };
> 
> The freq-div table describes the maximum frequency of each divider
> setting. Although the first element doesn't used in current
> implementation, I think it's better to keep freq-div table's
> completeness.

the issue I see is, that its value is currently 0 and the code substracts 1. 
So if anything would (accidentially) select MT8173_PLL_FMAX, the u32 val would 
wrap around, as you're subtracting 1 from 0 .


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1178291

Hi Heiko,

On Tue, 2015-07-07 at 11:34 +0200, Heiko Stübner wrote:
> > > > @@ -135,16 +138,26 @@ static void mtk_pll_calc_values(struct mtk_clk_pll
> > > > *pll, u32 *pcw, u32 *postdiv, u32 freq, u32 fin)
> > > > 
> > > >  {
> > > >  
> > > >  	unsigned long fmin = 1000 * MHZ;
> > > > 
> > > > +	const unsigned long *div_rate = pll->data->div_rate;
> > > > 
> > > >  	u64 _pcw;
> > > >  	u32 val;
> > > >  	
> > > >  	if (freq > pll->data->fmax)
> > > >  	
> > > >  		freq = pll->data->fmax;
> > > > 
> > > > -	for (val = 0; val < 4; val++) {
> > > > +	if (div_rate) {
> > > > +		for (val = 1; div_rate[val] != 0; val++) {
> > > > +			if (freq > div_rate[val])
> > > > +				break;
> > > > +		}
> > > > +		val--;
> > > 
> > > if you're changing the table struct, this of course also would need to be
> > > adapted.
> > > 
> > > 
> > > Hmm, what I don't understand is, what does MT8173_PLL_FMAX in the table,
> > > if
> > > you ignore it here all the time?
> > > 
> > > So the table should probably look more like [when using the concept from
> > > above]
> > > 
> > > static const struct mtk_pll_div_table mmpll_div_rate[] = {
> > > 
> > > 	{ .freq = 1000000000, .div = 0 },
> > > 	{ .freq = 702000000, .div = 1 },
> > > 	{ .freq = 253500000, .div = 2 },
> > > 	{ .freq = 126750000, .div = 3 },
> > > 	{ /* sentinel */ },
> > > 
> > > };
> > 
> > The freq-div table describes the maximum frequency of each divider
> > setting. Although the first element doesn't used in current
> > implementation, I think it's better to keep freq-div table's
> > completeness.
> 
> the issue I see is, that its value is currently 0 and the code substracts 1. 
> So if anything would (accidentially) select MT8173_PLL_FMAX, the u32 val would 
> wrap around, as you're subtracting 1 from 0 .

Subtracting 1 from val is safe now because it starts from 1:

  for (val = 1; div_rate[val] != 0; val++) {
    ...
  }
  val--;

I can change this implementation to a more readable one such as:

  for (val = 0; div_rate[val + 1] != 0; val++) {
    if (freq <= div_rate[val] && freq > div_rate[val + 1]) {
      ...

Do you think it is OK?


Best regards,

James

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1178328

Am Dienstag, 7. Juli 2015, 17:48:45 schrieb James Liao:
> Hi Heiko,
> 
> On Tue, 2015-07-07 at 11:34 +0200, Heiko Stübner wrote:
> > > > > @@ -135,16 +138,26 @@ static void mtk_pll_calc_values(struct
> > > > > mtk_clk_pll
> > > > > *pll, u32 *pcw, u32 *postdiv, u32 freq, u32 fin)
> > > > > 
> > > > >  {
> > > > >  
> > > > >  	unsigned long fmin = 1000 * MHZ;
> > > > > 
> > > > > +	const unsigned long *div_rate = pll->data->div_rate;
> > > > > 
> > > > >  	u64 _pcw;
> > > > >  	u32 val;
> > > > >  	
> > > > >  	if (freq > pll->data->fmax)
> > > > >  	
> > > > >  		freq = pll->data->fmax;
> > > > > 
> > > > > -	for (val = 0; val < 4; val++) {
> > > > > +	if (div_rate) {
> > > > > +		for (val = 1; div_rate[val] != 0; val++) {
> > > > > +			if (freq > div_rate[val])
> > > > > +				break;
> > > > > +		}
> > > > > +		val--;
> > > > 
> > > > if you're changing the table struct, this of course also would need to
> > > > be
> > > > adapted.
> > > > 
> > > > 
> > > > Hmm, what I don't understand is, what does MT8173_PLL_FMAX in the
> > > > table,
> > > > if
> > > > you ignore it here all the time?
> > > > 
> > > > So the table should probably look more like [when using the concept
> > > > from
> > > > above]
> > > > 
> > > > static const struct mtk_pll_div_table mmpll_div_rate[] = {
> > > > 
> > > > 	{ .freq = 1000000000, .div = 0 },
> > > > 	{ .freq = 702000000, .div = 1 },
> > > > 	{ .freq = 253500000, .div = 2 },
> > > > 	{ .freq = 126750000, .div = 3 },
> > > > 	{ /* sentinel */ },
> > > > 
> > > > };
> > > 
> > > The freq-div table describes the maximum frequency of each divider
> > > setting. Although the first element doesn't used in current
> > > implementation, I think it's better to keep freq-div table's
> > > completeness.
> > 
> > the issue I see is, that its value is currently 0 and the code substracts
> > 1. So if anything would (accidentially) select MT8173_PLL_FMAX, the u32
> > val would wrap around, as you're subtracting 1 from 0 .
> 
> Subtracting 1 from val is safe now because it starts from 1:
> 
>   for (val = 1; div_rate[val] != 0; val++) {
>     ...
>   }
>   val--;
> 
> I can change this implementation to a more readable one such as:
> 
>   for (val = 0; div_rate[val + 1] != 0; val++) {
>     if (freq <= div_rate[val] && freq > div_rate[val + 1]) {
>       ...
> 
> Do you think it is OK?

My issue is, that you have the MT8173_PLL_FMAX entry in the table, which is 
effectively unused, as it is ignored by the for loop. They why have it all, if 
nothing cares about it.

So if in the future somebody notices, "oh this is ignoring the first entry" 
without look further what the code does, this explodes ;-)


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web