Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
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 = 10, .div = 0 },
> > > > { .freq = 70200, .div = 1 },
> > > > { .freq = 25350, .div = 2 },
> > > > { .freq = 12675, .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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
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 = 10, .div = 0 },
> > > { .freq = 70200, .div = 1 },
> > > { .freq = 25350, .div = 2 },
> > > { .freq = 12675, .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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
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,
> > > + 10,
> > > + 70200,
> > > + 25350,
> > > + 12675,
> > > + 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 intfreq;
> > unsigned intdiv;
> >
> > };
> >
> > static const struct mtk_pll_div_table mmpll_div_rate[] = {
> >
> > { .freq = MT8173_PLL_FMAX, .div = 0 },
> > { .freq = 10, .div = 1 },
> > { .freq = 70200, .div = 2 },
> > { .freq = 25350, .div = 3 },
> > { .freq = 12675, .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 = 10, .div = 0 },
> > { .freq = 70200, .div = 1 },
> > { .freq = 25350, .div = 2 },
> > { .freq = 12675, .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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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,
> > + 10,
> > + 70200,
> > + 25350,
> > + 12675,
> > + 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 intfreq;
> unsigned intdiv;
> };
>
> static const struct mtk_pll_div_table mmpll_div_rate[] = {
> { .freq = MT8173_PLL_FMAX, .div = 0 },
> { .freq = 10, .div = 1 },
> { .freq = 70200, .div = 2 },
> { .freq = 25350, .div = 3 },
> { .freq = 12675, .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 = 10, .div = 0 },
> { .freq = 70200, .div = 1 },
> { .freq = 25350, .div = 2 },
> { .freq = 12675, .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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
Hi James,
Am Dienstag, 2. Juni 2015, 13:26:00 schrieb James Liao:
> MT8173 MMPLL frequency settings are different from common PLLs.
> It needs different post divider settings for some ranges of frequency.
> This patch add support for MT8173 MMPLL frequency setting, includes:
>
> 1. Add div-rate table for PLLs.
> 2. Increase the max ost divider setting from 3 (/8) to 4 (/16).
> 3. Write postdiv and pcw settings at the same time.
>
> Signed-off-by: James Liao
> ---
> drivers/clk/mediatek/clk-mt8173.c | 24 +---
> drivers/clk/mediatek/clk-mtk.h| 1 +
> drivers/clk/mediatek/clk-pll.c| 37
> + 3 files changed, 47 insertions(+), 15
> deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8173.c
> b/drivers/clk/mediatek/clk-mt8173.c index 357b080..821de7d 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -779,8 +779,9 @@ CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg",
> mtk_pericfg_init);
>
> #define CON0_MT8173_RST_BAR BIT(24)
>
> -#define PLL(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits,
> _pd_reg, _pd_shift, \ - _tuner_reg, _pcw_reg,
> _pcw_shift) { \
> +#define PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits,
> \
> + _pd_reg, _pd_shift, _tuner_reg, _pcw_reg, \
> + _pcw_shift, _div_rate) {\
> .id = _id, \
> .name = _name, \
> .reg = _reg,\
> @@ -795,14 +796,31 @@ CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg",
> mtk_pericfg_init); .tuner_reg = _tuner_reg, \
> .pcw_reg = _pcw_reg,\
> .pcw_shift = _pcw_shift,\
> + .div_rate = _div_rate, \
> }
>
> +#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?
> + MT8173_PLL_FMAX,
> + 10,
> + 70200,
> + 25350,
> + 12675,
> + 0,
it's more common to label sentinel entries (the 0 marking the end) with
/* sentinel */
instead of value 0.
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 intfreq;
unsigned intdiv;
};
static const struct mtk_pll_div_table mmpll_div_rate[] = {
{ .freq = MT8173_PLL_FMAX, .div = 0 },
{ .freq = 10, .div = 1 },
{ .freq = 70200, .div = 2 },
{ .freq = 25350, .div = 3 },
{ .freq = 12675, .div = 4 },
{ /* sentinel */ },
};
> +};
> +
> static const struct mtk_pll_data plls[] = {
> PLL(CLK_APMIXED_ARMCA15PLL, "armca15pll", 0x200, 0x20c, 0x0001, 0,
21,
> 0x204, 24, 0x0, 0x204, 0), PLL(CLK_APMIXED_ARMCA7PLL, "armca7pll", 0x210,
> 0x21c, 0x0001, 0, 21, 0x214, 24, 0x0, 0x214, 0),
> PLL(CLK_APMIXED_MAINPLL, "mainpll", 0x220, 0x22c, 0xf101, HAVE_RST_BAR,
> 21, 0x220, 4, 0x0, 0x224, 0), PLL(CLK_APMIXED_UNIVPLL, "univpll", 0x230,
> 0x23c, 0xfe01, HAVE_RST_BAR, 7, 0x230, 4, 0x0, 0x234, 14),
> - PLL(CLK_APMIXED_MMPLL, "mmpll", 0x240, 0x24c, 0x0001, 0, 21, 0x244,
> 24, 0x0, 0x244, 0), + PLL_B(CLK_APMIXED_MMPLL, "mmpll", 0x240, 0x24c,
> 0x0001, 0, 21, 0x244, 24, 0x0, 0x244, 0, mmpll_div_rate),
> PLL(CLK_APMIXED_MSDCPLL, "msdcpll", 0x250, 0x25c, 0x0001, 0, 21, 0x250,
> 4, 0x0, 0x254, 0), PLL(CLK_APMIXED_VENCPLL, "vencpll", 0x260, 0x26c,
> 0x0001, 0, 21, 0x260, 4, 0x0, 0x264, 0), PLL(CLK_APMIXED_TVDPLL,
> "tvdpll", 0x270, 0x27c, 0x0001, 0, 21, 0x270, 4, 0x0, 0x274, 0), diff
> --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> index 61035b9..645af7c 100644
> --- a/drivers/clk/mediatek/clk-mtk.h
> +++ b/drivers/clk/mediatek/clk-mtk.h
> @@ -150,6 +150,7 @@ struct mtk_pll_data {
> int pcwbits;
> uint32_t pcw_reg;
> int pcw_shift;
> + const unsigned long *div_rate;
> };
>
> void __init mtk_clk_register_plls(struct device_node *node,
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index 44409e9..4680a09 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -90,20 +90,23 @@ static unsigned long __mtk_pll_recalc_rate(struct
> mtk_clk_pll *pll,
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
Hi,
Does anyone have comments for this patch?
Best regards,
James
On Tue, 2015-06-02 at 13:26 +0800, James Liao wrote:
> MT8173 MMPLL frequency settings are different from common PLLs.
> It needs different post divider settings for some ranges of frequency.
> This patch add support for MT8173 MMPLL frequency setting, includes:
>
> 1. Add div-rate table for PLLs.
> 2. Increase the max ost divider setting from 3 (/8) to 4 (/16).
> 3. Write postdiv and pcw settings at the same time.
>
> Signed-off-by: James Liao
> ---
> drivers/clk/mediatek/clk-mt8173.c | 24 +---
> drivers/clk/mediatek/clk-mtk.h| 1 +
> drivers/clk/mediatek/clk-pll.c| 37 +
> 3 files changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8173.c
> b/drivers/clk/mediatek/clk-mt8173.c
> index 357b080..821de7d 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -779,8 +779,9 @@ CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg",
> mtk_pericfg_init);
>
> #define CON0_MT8173_RST_BAR BIT(24)
>
> -#define PLL(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, _pd_reg,
> _pd_shift, \
> - _tuner_reg, _pcw_reg, _pcw_shift) { \
> +#define PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits,
> \
> + _pd_reg, _pd_shift, _tuner_reg, _pcw_reg, \
> + _pcw_shift, _div_rate) {\
> .id = _id, \
> .name = _name, \
> .reg = _reg,\
> @@ -795,14 +796,31 @@ CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg",
> mtk_pericfg_init);
> .tuner_reg = _tuner_reg,\
> .pcw_reg = _pcw_reg,\
> .pcw_shift = _pcw_shift,\
> + .div_rate = _div_rate, \
> }
>
> +#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[] = {
> + MT8173_PLL_FMAX,
> + 10,
> + 70200,
> + 25350,
> + 12675,
> + 0,
> +};
> +
> static const struct mtk_pll_data plls[] = {
> PLL(CLK_APMIXED_ARMCA15PLL, "armca15pll", 0x200, 0x20c, 0x0001, 0,
> 21, 0x204, 24, 0x0, 0x204, 0),
> PLL(CLK_APMIXED_ARMCA7PLL, "armca7pll", 0x210, 0x21c, 0x0001, 0,
> 21, 0x214, 24, 0x0, 0x214, 0),
> PLL(CLK_APMIXED_MAINPLL, "mainpll", 0x220, 0x22c, 0xf101,
> HAVE_RST_BAR, 21, 0x220, 4, 0x0, 0x224, 0),
> PLL(CLK_APMIXED_UNIVPLL, "univpll", 0x230, 0x23c, 0xfe01,
> HAVE_RST_BAR, 7, 0x230, 4, 0x0, 0x234, 14),
> - PLL(CLK_APMIXED_MMPLL, "mmpll", 0x240, 0x24c, 0x0001, 0, 21, 0x244,
> 24, 0x0, 0x244, 0),
> + PLL_B(CLK_APMIXED_MMPLL, "mmpll", 0x240, 0x24c, 0x0001, 0, 21,
> 0x244, 24, 0x0, 0x244, 0, mmpll_div_rate),
> PLL(CLK_APMIXED_MSDCPLL, "msdcpll", 0x250, 0x25c, 0x0001, 0, 21,
> 0x250, 4, 0x0, 0x254, 0),
> PLL(CLK_APMIXED_VENCPLL, "vencpll", 0x260, 0x26c, 0x0001, 0, 21,
> 0x260, 4, 0x0, 0x264, 0),
> PLL(CLK_APMIXED_TVDPLL, "tvdpll", 0x270, 0x27c, 0x0001, 0, 21,
> 0x270, 4, 0x0, 0x274, 0),
> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> index 61035b9..645af7c 100644
> --- a/drivers/clk/mediatek/clk-mtk.h
> +++ b/drivers/clk/mediatek/clk-mtk.h
> @@ -150,6 +150,7 @@ struct mtk_pll_data {
> int pcwbits;
> uint32_t pcw_reg;
> int pcw_shift;
> + const unsigned long *div_rate;
> };
>
> void __init mtk_clk_register_plls(struct device_node *node,
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index 44409e9..4680a09 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -90,20 +90,23 @@ static unsigned long __mtk_pll_recalc_rate(struct
> mtk_clk_pll *pll, u32 fin,
> static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
> int postdiv)
> {
> - 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 =
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
Hi,
Does anyone have comments for this patch?
Best regards,
James
On Tue, 2015-06-02 at 13:26 +0800, James Liao wrote:
MT8173 MMPLL frequency settings are different from common PLLs.
It needs different post divider settings for some ranges of frequency.
This patch add support for MT8173 MMPLL frequency setting, includes:
1. Add div-rate table for PLLs.
2. Increase the max ost divider setting from 3 (/8) to 4 (/16).
3. Write postdiv and pcw settings at the same time.
Signed-off-by: James Liao jamesjj.l...@mediatek.com
---
drivers/clk/mediatek/clk-mt8173.c | 24 +---
drivers/clk/mediatek/clk-mtk.h| 1 +
drivers/clk/mediatek/clk-pll.c| 37 +
3 files changed, 47 insertions(+), 15 deletions(-)
diff --git a/drivers/clk/mediatek/clk-mt8173.c
b/drivers/clk/mediatek/clk-mt8173.c
index 357b080..821de7d 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -779,8 +779,9 @@ CLK_OF_DECLARE(mtk_pericfg, mediatek,mt8173-pericfg,
mtk_pericfg_init);
#define CON0_MT8173_RST_BAR BIT(24)
-#define PLL(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, _pd_reg,
_pd_shift, \
- _tuner_reg, _pcw_reg, _pcw_shift) { \
+#define PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits,
\
+ _pd_reg, _pd_shift, _tuner_reg, _pcw_reg, \
+ _pcw_shift, _div_rate) {\
.id = _id, \
.name = _name, \
.reg = _reg,\
@@ -795,14 +796,31 @@ CLK_OF_DECLARE(mtk_pericfg, mediatek,mt8173-pericfg,
mtk_pericfg_init);
.tuner_reg = _tuner_reg,\
.pcw_reg = _pcw_reg,\
.pcw_shift = _pcw_shift,\
+ .div_rate = _div_rate, \
}
+#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[] = {
+ MT8173_PLL_FMAX,
+ 10,
+ 70200,
+ 25350,
+ 12675,
+ 0,
+};
+
static const struct mtk_pll_data plls[] = {
PLL(CLK_APMIXED_ARMCA15PLL, armca15pll, 0x200, 0x20c, 0x0001, 0,
21, 0x204, 24, 0x0, 0x204, 0),
PLL(CLK_APMIXED_ARMCA7PLL, armca7pll, 0x210, 0x21c, 0x0001, 0,
21, 0x214, 24, 0x0, 0x214, 0),
PLL(CLK_APMIXED_MAINPLL, mainpll, 0x220, 0x22c, 0xf101,
HAVE_RST_BAR, 21, 0x220, 4, 0x0, 0x224, 0),
PLL(CLK_APMIXED_UNIVPLL, univpll, 0x230, 0x23c, 0xfe01,
HAVE_RST_BAR, 7, 0x230, 4, 0x0, 0x234, 14),
- PLL(CLK_APMIXED_MMPLL, mmpll, 0x240, 0x24c, 0x0001, 0, 21, 0x244,
24, 0x0, 0x244, 0),
+ PLL_B(CLK_APMIXED_MMPLL, mmpll, 0x240, 0x24c, 0x0001, 0, 21,
0x244, 24, 0x0, 0x244, 0, mmpll_div_rate),
PLL(CLK_APMIXED_MSDCPLL, msdcpll, 0x250, 0x25c, 0x0001, 0, 21,
0x250, 4, 0x0, 0x254, 0),
PLL(CLK_APMIXED_VENCPLL, vencpll, 0x260, 0x26c, 0x0001, 0, 21,
0x260, 4, 0x0, 0x264, 0),
PLL(CLK_APMIXED_TVDPLL, tvdpll, 0x270, 0x27c, 0x0001, 0, 21,
0x270, 4, 0x0, 0x274, 0),
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index 61035b9..645af7c 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -150,6 +150,7 @@ struct mtk_pll_data {
int pcwbits;
uint32_t pcw_reg;
int pcw_shift;
+ const unsigned long *div_rate;
};
void __init mtk_clk_register_plls(struct device_node *node,
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 44409e9..4680a09 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -90,20 +90,23 @@ static unsigned long __mtk_pll_recalc_rate(struct
mtk_clk_pll *pll, u32 fin,
static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
int postdiv)
{
- 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 |=
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
Hi James,
Am Dienstag, 2. Juni 2015, 13:26:00 schrieb James Liao:
MT8173 MMPLL frequency settings are different from common PLLs.
It needs different post divider settings for some ranges of frequency.
This patch add support for MT8173 MMPLL frequency setting, includes:
1. Add div-rate table for PLLs.
2. Increase the max ost divider setting from 3 (/8) to 4 (/16).
3. Write postdiv and pcw settings at the same time.
Signed-off-by: James Liao jamesjj.l...@mediatek.com
---
drivers/clk/mediatek/clk-mt8173.c | 24 +---
drivers/clk/mediatek/clk-mtk.h| 1 +
drivers/clk/mediatek/clk-pll.c| 37
+ 3 files changed, 47 insertions(+), 15
deletions(-)
diff --git a/drivers/clk/mediatek/clk-mt8173.c
b/drivers/clk/mediatek/clk-mt8173.c index 357b080..821de7d 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -779,8 +779,9 @@ CLK_OF_DECLARE(mtk_pericfg, mediatek,mt8173-pericfg,
mtk_pericfg_init);
#define CON0_MT8173_RST_BAR BIT(24)
-#define PLL(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits,
_pd_reg, _pd_shift, \ - _tuner_reg, _pcw_reg,
_pcw_shift) { \
+#define PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits,
\
+ _pd_reg, _pd_shift, _tuner_reg, _pcw_reg, \
+ _pcw_shift, _div_rate) {\
.id = _id, \
.name = _name, \
.reg = _reg,\
@@ -795,14 +796,31 @@ CLK_OF_DECLARE(mtk_pericfg, mediatek,mt8173-pericfg,
mtk_pericfg_init); .tuner_reg = _tuner_reg, \
.pcw_reg = _pcw_reg,\
.pcw_shift = _pcw_shift,\
+ .div_rate = _div_rate, \
}
+#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?
+ MT8173_PLL_FMAX,
+ 10,
+ 70200,
+ 25350,
+ 12675,
+ 0,
it's more common to label sentinel entries (the 0 marking the end) with
/* sentinel */
instead of value 0.
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 intfreq;
unsigned intdiv;
};
static const struct mtk_pll_div_table mmpll_div_rate[] = {
{ .freq = MT8173_PLL_FMAX, .div = 0 },
{ .freq = 10, .div = 1 },
{ .freq = 70200, .div = 2 },
{ .freq = 25350, .div = 3 },
{ .freq = 12675, .div = 4 },
{ /* sentinel */ },
};
+};
+
static const struct mtk_pll_data plls[] = {
PLL(CLK_APMIXED_ARMCA15PLL, armca15pll, 0x200, 0x20c, 0x0001, 0,
21,
0x204, 24, 0x0, 0x204, 0), PLL(CLK_APMIXED_ARMCA7PLL, armca7pll, 0x210,
0x21c, 0x0001, 0, 21, 0x214, 24, 0x0, 0x214, 0),
PLL(CLK_APMIXED_MAINPLL, mainpll, 0x220, 0x22c, 0xf101, HAVE_RST_BAR,
21, 0x220, 4, 0x0, 0x224, 0), PLL(CLK_APMIXED_UNIVPLL, univpll, 0x230,
0x23c, 0xfe01, HAVE_RST_BAR, 7, 0x230, 4, 0x0, 0x234, 14),
- PLL(CLK_APMIXED_MMPLL, mmpll, 0x240, 0x24c, 0x0001, 0, 21, 0x244,
24, 0x0, 0x244, 0), + PLL_B(CLK_APMIXED_MMPLL, mmpll, 0x240, 0x24c,
0x0001, 0, 21, 0x244, 24, 0x0, 0x244, 0, mmpll_div_rate),
PLL(CLK_APMIXED_MSDCPLL, msdcpll, 0x250, 0x25c, 0x0001, 0, 21, 0x250,
4, 0x0, 0x254, 0), PLL(CLK_APMIXED_VENCPLL, vencpll, 0x260, 0x26c,
0x0001, 0, 21, 0x260, 4, 0x0, 0x264, 0), PLL(CLK_APMIXED_TVDPLL,
tvdpll, 0x270, 0x27c, 0x0001, 0, 21, 0x270, 4, 0x0, 0x274, 0), diff
--git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index 61035b9..645af7c 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -150,6 +150,7 @@ struct mtk_pll_data {
int pcwbits;
uint32_t pcw_reg;
int pcw_shift;
+ const unsigned long *div_rate;
};
void __init mtk_clk_register_plls(struct device_node *node,
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 44409e9..4680a09 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -90,20 +90,23 @@ static unsigned long __mtk_pll_recalc_rate(struct
mtk_clk_pll *pll, u32 fin, static void mtk_pll_set_rate_regs(struct
mtk_clk_pll *pll, u32 pcw, int
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
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 = 10, .div = 0 },
{ .freq = 70200, .div = 1 },
{ .freq = 25350, .div = 2 },
{ .freq = 12675, .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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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,
+ 10,
+ 70200,
+ 25350,
+ 12675,
+ 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 intfreq;
unsigned intdiv;
};
static const struct mtk_pll_div_table mmpll_div_rate[] = {
{ .freq = MT8173_PLL_FMAX, .div = 0 },
{ .freq = 10, .div = 1 },
{ .freq = 70200, .div = 2 },
{ .freq = 25350, .div = 3 },
{ .freq = 12675, .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 = 10, .div = 0 },
{ .freq = 70200, .div = 1 },
{ .freq = 25350, .div = 2 },
{ .freq = 12675, .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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
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 = 10, .div = 0 },
{ .freq = 70200, .div = 1 },
{ .freq = 25350, .div = 2 },
{ .freq = 12675, .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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] clk: mediatek: Add MT8173 MMPLL change rate support
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,
+ 10,
+ 70200,
+ 25350,
+ 12675,
+ 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 intfreq;
unsigned intdiv;
};
static const struct mtk_pll_div_table mmpll_div_rate[] = {
{ .freq = MT8173_PLL_FMAX, .div = 0 },
{ .freq = 10, .div = 1 },
{ .freq = 70200, .div = 2 },
{ .freq = 25350, .div = 3 },
{ .freq = 12675, .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 = 10, .div = 0 },
{ .freq = 70200, .div = 1 },
{ .freq = 25350, .div = 2 },
{ .freq = 12675, .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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
