From: Weiyi Lu <weiyi.lu@mediatek.com>
To: Nicolas Boichat <drinkcat@chromium.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
Rob Herring <robh@kernel.org>, Stephen Boyd <sboyd@kernel.org>,
James Liao <jamesjj.liao@mediatek.com>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
lkml <linux-kernel@vger.kernel.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>, <linux-clk@vger.kernel.org>,
srv_heupstream <srv_heupstream@mediatek.com>,
Wendell Lin <wendell.lin@mediatek.com>,
Ikjoon Jang <ikjn@chromium.org>
Subject: Re: [PATCH 3/4] clk: mediatek: Add configurable enable control to mtk_pll_data
Date: Mon, 27 Jul 2020 17:04:02 +0800 [thread overview]
Message-ID: <1595840642.12203.4.camel@mtksdaap41> (raw)
In-Reply-To: <CANMq1KDzmeMcVQU=i89sa-B4EQbz6OxZP3tDasV-Q__qB_7_9g@mail.gmail.com>
On Thu, 2020-07-23 at 15:51 +0800, Nicolas Boichat wrote:
> On Thu, Jul 23, 2020 at 10:57 AM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > On Wed, 2020-07-22 at 16:51 +0800, Nicolas Boichat wrote:
> > > On Wed, Jul 22, 2020 at 2:50 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> > > >
> > > > In all MediaTek PLL design, bit 0 of CON0 register is always
> > > > the enable bit.
> > > > However, there's a special case of usbpll on MT8192.
> > > > The enable bit of usbpll is moved to bit 2 of other register.
> > > > Add configurable en_reg and base_en_bit for enable control or
> > > > using the default if without setting in pll data.
> > > >
> > > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > > > ---
> > > > drivers/clk/mediatek/clk-mtk.h | 2 ++
> > > > drivers/clk/mediatek/clk-pll.c | 26 ++++++++++++++++++++++----
> > > > 2 files changed, 24 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> > > > index c3d6756..8bb0b3d 100644
> > > > --- a/drivers/clk/mediatek/clk-mtk.h
> > > > +++ b/drivers/clk/mediatek/clk-mtk.h
> > > > @@ -233,6 +233,8 @@ struct mtk_pll_data {
> > > > uint32_t pcw_chg_reg;
> > > > const struct mtk_pll_div_table *div_table;
> > > > const char *parent_name;
> > > > + uint32_t en_reg;
> > > > + uint8_t base_en_bit;
> > > > };
> > > >
> > > > void 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 f440f2cd..b8ccd42 100644
> > > > --- a/drivers/clk/mediatek/clk-pll.c
> > > > +++ b/drivers/clk/mediatek/clk-pll.c
> > > > @@ -44,6 +44,7 @@ struct mtk_clk_pll {
> > > > void __iomem *tuner_en_addr;
> > > > void __iomem *pcw_addr;
> > > > void __iomem *pcw_chg_addr;
> > > > + void __iomem *en_addr;
> > > > const struct mtk_pll_data *data;
> > > > };
> > > >
> > > > @@ -56,7 +57,10 @@ static int mtk_pll_is_prepared(struct clk_hw *hw)
> > > > {
> > > > struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > > >
> > > > - return (readl(pll->base_addr + REG_CON0) & CON0_BASE_EN) != 0;
> > > > + if (pll->en_addr)
> > > > + return (readl(pll->en_addr) & BIT(pll->data->base_en_bit)) != 0;
> > > > + else
> > > > + return (readl(pll->base_addr + REG_CON0) & CON0_BASE_EN) != 0;
> > > > }
> > > >
> > > > static unsigned long __mtk_pll_recalc_rate(struct mtk_clk_pll *pll, u32 fin,
> > > > @@ -251,6 +255,12 @@ static int mtk_pll_prepare(struct clk_hw *hw)
> > > > r |= pll->data->en_mask;
> > > > writel(r, pll->base_addr + REG_CON0);
> > > >
> > >
> > > This is not a new change, but I'm wondering if the asymmetry is
> > > intentional here, that is, prepare sets bit pll->data->en_mask of
> > > REG_CON0; unprepare clears CON0_BASE_EN of REG_CON0.
> > >
> > > With this patch, if pll->en_addr is set, you set both
> > > pll->data->en_mask _and_ pll->data->base_en_bit, and clear only
> > > pll->data->base_en_bit.
> > >
> >
> > Hi Nicolas,
> >
> > AFAIK, the asymmetry was intentional.
> > en_mask is actually a combination of divider enable mask and the pll
> > enable bit(CON0_BASE_EN).
> > Even without my patch, it still sets divider enable mask and en_bit, and
> > only clears en_bit.
> > You could see the pll_data in clk-mt8192.c of patch [4/4]
> > Take mainpll as an example,
> > the enable mask of mainpll is 0xff000001, where 0xff000000 is the
> > divider enable mask and 0x1 is the en_bit
> >
> > For usbpll in special case, usbpll doesn't have divider enable mask on
> > MT8192 so I give nothing(0x00000000) in the en_mask field.
> > However, the main reason why I don't skip setting the en_mask of MT8192
> > usbpll is that I'd just like to reserve the divider enable mask for any
> > special plls with divider enable mask in near future.
>
> Argh, I see, it's a bit of a can of worms, with many special cases...
>
> So I played a bit with 3 examples.
>
> Current situation looks like this:
>
> 8183 CLK_APMIXED_ARMPLL_LL
> en_mask = 0x00000001
> en_reg = 0
> base_en_bit = 0
>
> prepare: REG_CON0 |= en_mask
> unprepare: REG_CON0 &= ~CON0_BASE_EN (BIT(1))
>
> 8192 CLK_APMIXED_UNIVPLL
> en_mask = 0xff000001
> en_reg = 0x039c
> base_en_bit = 0
>
> prepare:
> REG_CON0 |= en_mask
> en_reg |= base_en_bit
> unprepare:
> en_reg &= ~base_en_bit
>
> 8192 CLK_APMIXED_USBPLL
> en_mask = 0x00000000
> en_reg = 0x03cc
> base_en_bit = 2
>
> prepare:
> REG_CON0 |= en_mask (0)
> en_reg |= base_en_bit
> unprepare:
> en_reg &= ~base_en_bit
>
> And I think the logic could still be simplified by _not_ putting
> CON0_BASE_EN in en_mask, and updating the CON0 in 2 steps: first all
> the bits that are not CON0_BASE_EN, then CON0_BASE_EN. Of course I
> assume that's it's fine to do so, but I have no idea.
>
> register_pll() {
> if (!en_addr) {
> en_reg = REG_CON0
> base_en_bit = CON0_BASE_EN
> }
> }
>
> prepare() {
> REG_CON0 |= en_mask
> en_reg |= base_en_bit
> }
>
> unprepare() {
> en_reg &= ~base_en_bit
> }
>
> Then the new clock data:
>
> 8183 CLK_APMIXED_ARMPLL_LL
> en_mask = 0x00000000 (CON0_BASE_EN is implicit, but other bits could be set)
> en_reg = 0
> base_en_bit = 0
>
> prepare: {
> REG_CON0 |= en_mask (0x00000000, here, we can skip, but other bits
> could be set)
> en_reg |= base_en_bit (REG_CON0 |= CON0_BASE_EN)
> }
> unprepare: en_reg &= ~base_en_bit (REG_CON0 &= ~CON0_BASE_EN)
>
> 8192 CLK_APMIXED_UNIVPLL
> en_mask = 0xff000001 (Note the bit 1 is _not_ dropped here, as it
> needs to be set too)
> en_reg = 0x039c
> base_en_bit = 0
> (same as above)
>
> 8192 CLK_APMIXED_USBPLL
> en_mask = 0x00000000
> en_reg = 0x03cc
> base_en_bit = 2
> (same as above)
>
> Now, maybe this is also a bit overcomplicated. Maybe a simpler
> solution is just to add a comment in prepare that "r |=
> pll->data->en_mask;" is meant to include CON0_BASE_EN in most cases,
> and then the code could be ok as-is (just to make sure that the next
> person who looks at this code does not think there is a bug...).
>
Hi Nicolas,
I thought these still too complicated and I guess the asymmetrical
problem could be fixed.
And that will make this part simpler just like what you mentioned in
previous comment.
I'll confirm ASAP and send a new version if it is possible to be fixed.
> >
> > > > + if (pll->en_addr) {
> > > > + r = readl(pll->en_addr);
> > > > + r |= BIT(pll->data->base_en_bit);
> > > > + writel(r, pll->en_addr);
> > > > + }
> > > > +
> > > > __mtk_pll_tuner_enable(pll);
> > > >
> > > > udelay(20);
> > > > @@ -277,9 +287,15 @@ static void mtk_pll_unprepare(struct clk_hw *hw)
> > > >
> > > > __mtk_pll_tuner_disable(pll);
> > > >
> > > > - r = readl(pll->base_addr + REG_CON0);
> > > > - r &= ~CON0_BASE_EN;
> > > > - writel(r, pll->base_addr + REG_CON0);
> > > > + if (pll->en_addr) {
> > > > + r = readl(pll->en_addr);
> > > > + r &= ~BIT(pll->data->base_en_bit);
> > > > + writel(r, pll->en_addr);
> > > > + } else {
> > > > + r = readl(pll->base_addr + REG_CON0);
> > > > + r &= ~CON0_BASE_EN;
> > > > + writel(r, pll->base_addr + REG_CON0);
> > > > + }
> > > >
> > > > r = readl(pll->pwr_addr) | CON0_ISO_EN;
> > > > writel(r, pll->pwr_addr);
> > > > @@ -321,6 +337,8 @@ static struct clk *mtk_clk_register_pll(const struct mtk_pll_data *data,
> > > > pll->tuner_addr = base + data->tuner_reg;
> > > > if (data->tuner_en_reg)
> > > > pll->tuner_en_addr = base + data->tuner_en_reg;
> > > > + if (data->en_reg)
> > > > + pll->en_addr = base + data->en_reg;
> > >
> > > If the answer to my question above holds (asymmetry is not
> > > intentional), this patch/the code could be simplified a lot if you
> > > also added a pll->en_bit member, and, here, did this:
> > >
> > > if (pll->en_reg) {
> > > pll->en_addr = base + data->en_reg;
> > > pll->end_bit = data->en_bit;
> > > } else {
> > > pll->en_addr = pll->base_addr + REG_CON0;
> > > pll->en_bit = CON0_BASE_EN;
> > > }
> > >
> > > > pll->hw.init = &init;
> > > > pll->data = data;
> > > >
> > > > --
> > > > 1.8.1.1.dirty
> >
WARNING: multiple messages have this Message-ID (diff)
From: Weiyi Lu <weiyi.lu@mediatek.com>
To: Nicolas Boichat <drinkcat@chromium.org>
Cc: Rob Herring <robh@kernel.org>,
srv_heupstream <srv_heupstream@mediatek.com>,
James Liao <jamesjj.liao@mediatek.com>,
Stephen Boyd <sboyd@kernel.org>,
lkml <linux-kernel@vger.kernel.org>,
Ikjoon Jang <ikjn@chromium.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Wendell Lin <wendell.lin@mediatek.com>,
linux-clk@vger.kernel.org,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 3/4] clk: mediatek: Add configurable enable control to mtk_pll_data
Date: Mon, 27 Jul 2020 17:04:02 +0800 [thread overview]
Message-ID: <1595840642.12203.4.camel@mtksdaap41> (raw)
In-Reply-To: <CANMq1KDzmeMcVQU=i89sa-B4EQbz6OxZP3tDasV-Q__qB_7_9g@mail.gmail.com>
On Thu, 2020-07-23 at 15:51 +0800, Nicolas Boichat wrote:
> On Thu, Jul 23, 2020 at 10:57 AM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > On Wed, 2020-07-22 at 16:51 +0800, Nicolas Boichat wrote:
> > > On Wed, Jul 22, 2020 at 2:50 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> > > >
> > > > In all MediaTek PLL design, bit 0 of CON0 register is always
> > > > the enable bit.
> > > > However, there's a special case of usbpll on MT8192.
> > > > The enable bit of usbpll is moved to bit 2 of other register.
> > > > Add configurable en_reg and base_en_bit for enable control or
> > > > using the default if without setting in pll data.
> > > >
> > > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > > > ---
> > > > drivers/clk/mediatek/clk-mtk.h | 2 ++
> > > > drivers/clk/mediatek/clk-pll.c | 26 ++++++++++++++++++++++----
> > > > 2 files changed, 24 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> > > > index c3d6756..8bb0b3d 100644
> > > > --- a/drivers/clk/mediatek/clk-mtk.h
> > > > +++ b/drivers/clk/mediatek/clk-mtk.h
> > > > @@ -233,6 +233,8 @@ struct mtk_pll_data {
> > > > uint32_t pcw_chg_reg;
> > > > const struct mtk_pll_div_table *div_table;
> > > > const char *parent_name;
> > > > + uint32_t en_reg;
> > > > + uint8_t base_en_bit;
> > > > };
> > > >
> > > > void 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 f440f2cd..b8ccd42 100644
> > > > --- a/drivers/clk/mediatek/clk-pll.c
> > > > +++ b/drivers/clk/mediatek/clk-pll.c
> > > > @@ -44,6 +44,7 @@ struct mtk_clk_pll {
> > > > void __iomem *tuner_en_addr;
> > > > void __iomem *pcw_addr;
> > > > void __iomem *pcw_chg_addr;
> > > > + void __iomem *en_addr;
> > > > const struct mtk_pll_data *data;
> > > > };
> > > >
> > > > @@ -56,7 +57,10 @@ static int mtk_pll_is_prepared(struct clk_hw *hw)
> > > > {
> > > > struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > > >
> > > > - return (readl(pll->base_addr + REG_CON0) & CON0_BASE_EN) != 0;
> > > > + if (pll->en_addr)
> > > > + return (readl(pll->en_addr) & BIT(pll->data->base_en_bit)) != 0;
> > > > + else
> > > > + return (readl(pll->base_addr + REG_CON0) & CON0_BASE_EN) != 0;
> > > > }
> > > >
> > > > static unsigned long __mtk_pll_recalc_rate(struct mtk_clk_pll *pll, u32 fin,
> > > > @@ -251,6 +255,12 @@ static int mtk_pll_prepare(struct clk_hw *hw)
> > > > r |= pll->data->en_mask;
> > > > writel(r, pll->base_addr + REG_CON0);
> > > >
> > >
> > > This is not a new change, but I'm wondering if the asymmetry is
> > > intentional here, that is, prepare sets bit pll->data->en_mask of
> > > REG_CON0; unprepare clears CON0_BASE_EN of REG_CON0.
> > >
> > > With this patch, if pll->en_addr is set, you set both
> > > pll->data->en_mask _and_ pll->data->base_en_bit, and clear only
> > > pll->data->base_en_bit.
> > >
> >
> > Hi Nicolas,
> >
> > AFAIK, the asymmetry was intentional.
> > en_mask is actually a combination of divider enable mask and the pll
> > enable bit(CON0_BASE_EN).
> > Even without my patch, it still sets divider enable mask and en_bit, and
> > only clears en_bit.
> > You could see the pll_data in clk-mt8192.c of patch [4/4]
> > Take mainpll as an example,
> > the enable mask of mainpll is 0xff000001, where 0xff000000 is the
> > divider enable mask and 0x1 is the en_bit
> >
> > For usbpll in special case, usbpll doesn't have divider enable mask on
> > MT8192 so I give nothing(0x00000000) in the en_mask field.
> > However, the main reason why I don't skip setting the en_mask of MT8192
> > usbpll is that I'd just like to reserve the divider enable mask for any
> > special plls with divider enable mask in near future.
>
> Argh, I see, it's a bit of a can of worms, with many special cases...
>
> So I played a bit with 3 examples.
>
> Current situation looks like this:
>
> 8183 CLK_APMIXED_ARMPLL_LL
> en_mask = 0x00000001
> en_reg = 0
> base_en_bit = 0
>
> prepare: REG_CON0 |= en_mask
> unprepare: REG_CON0 &= ~CON0_BASE_EN (BIT(1))
>
> 8192 CLK_APMIXED_UNIVPLL
> en_mask = 0xff000001
> en_reg = 0x039c
> base_en_bit = 0
>
> prepare:
> REG_CON0 |= en_mask
> en_reg |= base_en_bit
> unprepare:
> en_reg &= ~base_en_bit
>
> 8192 CLK_APMIXED_USBPLL
> en_mask = 0x00000000
> en_reg = 0x03cc
> base_en_bit = 2
>
> prepare:
> REG_CON0 |= en_mask (0)
> en_reg |= base_en_bit
> unprepare:
> en_reg &= ~base_en_bit
>
> And I think the logic could still be simplified by _not_ putting
> CON0_BASE_EN in en_mask, and updating the CON0 in 2 steps: first all
> the bits that are not CON0_BASE_EN, then CON0_BASE_EN. Of course I
> assume that's it's fine to do so, but I have no idea.
>
> register_pll() {
> if (!en_addr) {
> en_reg = REG_CON0
> base_en_bit = CON0_BASE_EN
> }
> }
>
> prepare() {
> REG_CON0 |= en_mask
> en_reg |= base_en_bit
> }
>
> unprepare() {
> en_reg &= ~base_en_bit
> }
>
> Then the new clock data:
>
> 8183 CLK_APMIXED_ARMPLL_LL
> en_mask = 0x00000000 (CON0_BASE_EN is implicit, but other bits could be set)
> en_reg = 0
> base_en_bit = 0
>
> prepare: {
> REG_CON0 |= en_mask (0x00000000, here, we can skip, but other bits
> could be set)
> en_reg |= base_en_bit (REG_CON0 |= CON0_BASE_EN)
> }
> unprepare: en_reg &= ~base_en_bit (REG_CON0 &= ~CON0_BASE_EN)
>
> 8192 CLK_APMIXED_UNIVPLL
> en_mask = 0xff000001 (Note the bit 1 is _not_ dropped here, as it
> needs to be set too)
> en_reg = 0x039c
> base_en_bit = 0
> (same as above)
>
> 8192 CLK_APMIXED_USBPLL
> en_mask = 0x00000000
> en_reg = 0x03cc
> base_en_bit = 2
> (same as above)
>
> Now, maybe this is also a bit overcomplicated. Maybe a simpler
> solution is just to add a comment in prepare that "r |=
> pll->data->en_mask;" is meant to include CON0_BASE_EN in most cases,
> and then the code could be ok as-is (just to make sure that the next
> person who looks at this code does not think there is a bug...).
>
Hi Nicolas,
I thought these still too complicated and I guess the asymmetrical
problem could be fixed.
And that will make this part simpler just like what you mentioned in
previous comment.
I'll confirm ASAP and send a new version if it is possible to be fixed.
> >
> > > > + if (pll->en_addr) {
> > > > + r = readl(pll->en_addr);
> > > > + r |= BIT(pll->data->base_en_bit);
> > > > + writel(r, pll->en_addr);
> > > > + }
> > > > +
> > > > __mtk_pll_tuner_enable(pll);
> > > >
> > > > udelay(20);
> > > > @@ -277,9 +287,15 @@ static void mtk_pll_unprepare(struct clk_hw *hw)
> > > >
> > > > __mtk_pll_tuner_disable(pll);
> > > >
> > > > - r = readl(pll->base_addr + REG_CON0);
> > > > - r &= ~CON0_BASE_EN;
> > > > - writel(r, pll->base_addr + REG_CON0);
> > > > + if (pll->en_addr) {
> > > > + r = readl(pll->en_addr);
> > > > + r &= ~BIT(pll->data->base_en_bit);
> > > > + writel(r, pll->en_addr);
> > > > + } else {
> > > > + r = readl(pll->base_addr + REG_CON0);
> > > > + r &= ~CON0_BASE_EN;
> > > > + writel(r, pll->base_addr + REG_CON0);
> > > > + }
> > > >
> > > > r = readl(pll->pwr_addr) | CON0_ISO_EN;
> > > > writel(r, pll->pwr_addr);
> > > > @@ -321,6 +337,8 @@ static struct clk *mtk_clk_register_pll(const struct mtk_pll_data *data,
> > > > pll->tuner_addr = base + data->tuner_reg;
> > > > if (data->tuner_en_reg)
> > > > pll->tuner_en_addr = base + data->tuner_en_reg;
> > > > + if (data->en_reg)
> > > > + pll->en_addr = base + data->en_reg;
> > >
> > > If the answer to my question above holds (asymmetry is not
> > > intentional), this patch/the code could be simplified a lot if you
> > > also added a pll->en_bit member, and, here, did this:
> > >
> > > if (pll->en_reg) {
> > > pll->en_addr = base + data->en_reg;
> > > pll->end_bit = data->en_bit;
> > > } else {
> > > pll->en_addr = pll->base_addr + REG_CON0;
> > > pll->en_bit = CON0_BASE_EN;
> > > }
> > >
> > > > pll->hw.init = &init;
> > > > pll->data = data;
> > > >
> > > > --
> > > > 1.8.1.1.dirty
> >
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Weiyi Lu <weiyi.lu@mediatek.com>
To: Nicolas Boichat <drinkcat@chromium.org>
Cc: Rob Herring <robh@kernel.org>,
srv_heupstream <srv_heupstream@mediatek.com>,
James Liao <jamesjj.liao@mediatek.com>,
Stephen Boyd <sboyd@kernel.org>,
lkml <linux-kernel@vger.kernel.org>,
Ikjoon Jang <ikjn@chromium.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Wendell Lin <wendell.lin@mediatek.com>,
linux-clk@vger.kernel.org,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 3/4] clk: mediatek: Add configurable enable control to mtk_pll_data
Date: Mon, 27 Jul 2020 17:04:02 +0800 [thread overview]
Message-ID: <1595840642.12203.4.camel@mtksdaap41> (raw)
In-Reply-To: <CANMq1KDzmeMcVQU=i89sa-B4EQbz6OxZP3tDasV-Q__qB_7_9g@mail.gmail.com>
On Thu, 2020-07-23 at 15:51 +0800, Nicolas Boichat wrote:
> On Thu, Jul 23, 2020 at 10:57 AM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > On Wed, 2020-07-22 at 16:51 +0800, Nicolas Boichat wrote:
> > > On Wed, Jul 22, 2020 at 2:50 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> > > >
> > > > In all MediaTek PLL design, bit 0 of CON0 register is always
> > > > the enable bit.
> > > > However, there's a special case of usbpll on MT8192.
> > > > The enable bit of usbpll is moved to bit 2 of other register.
> > > > Add configurable en_reg and base_en_bit for enable control or
> > > > using the default if without setting in pll data.
> > > >
> > > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > > > ---
> > > > drivers/clk/mediatek/clk-mtk.h | 2 ++
> > > > drivers/clk/mediatek/clk-pll.c | 26 ++++++++++++++++++++++----
> > > > 2 files changed, 24 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> > > > index c3d6756..8bb0b3d 100644
> > > > --- a/drivers/clk/mediatek/clk-mtk.h
> > > > +++ b/drivers/clk/mediatek/clk-mtk.h
> > > > @@ -233,6 +233,8 @@ struct mtk_pll_data {
> > > > uint32_t pcw_chg_reg;
> > > > const struct mtk_pll_div_table *div_table;
> > > > const char *parent_name;
> > > > + uint32_t en_reg;
> > > > + uint8_t base_en_bit;
> > > > };
> > > >
> > > > void 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 f440f2cd..b8ccd42 100644
> > > > --- a/drivers/clk/mediatek/clk-pll.c
> > > > +++ b/drivers/clk/mediatek/clk-pll.c
> > > > @@ -44,6 +44,7 @@ struct mtk_clk_pll {
> > > > void __iomem *tuner_en_addr;
> > > > void __iomem *pcw_addr;
> > > > void __iomem *pcw_chg_addr;
> > > > + void __iomem *en_addr;
> > > > const struct mtk_pll_data *data;
> > > > };
> > > >
> > > > @@ -56,7 +57,10 @@ static int mtk_pll_is_prepared(struct clk_hw *hw)
> > > > {
> > > > struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > > >
> > > > - return (readl(pll->base_addr + REG_CON0) & CON0_BASE_EN) != 0;
> > > > + if (pll->en_addr)
> > > > + return (readl(pll->en_addr) & BIT(pll->data->base_en_bit)) != 0;
> > > > + else
> > > > + return (readl(pll->base_addr + REG_CON0) & CON0_BASE_EN) != 0;
> > > > }
> > > >
> > > > static unsigned long __mtk_pll_recalc_rate(struct mtk_clk_pll *pll, u32 fin,
> > > > @@ -251,6 +255,12 @@ static int mtk_pll_prepare(struct clk_hw *hw)
> > > > r |= pll->data->en_mask;
> > > > writel(r, pll->base_addr + REG_CON0);
> > > >
> > >
> > > This is not a new change, but I'm wondering if the asymmetry is
> > > intentional here, that is, prepare sets bit pll->data->en_mask of
> > > REG_CON0; unprepare clears CON0_BASE_EN of REG_CON0.
> > >
> > > With this patch, if pll->en_addr is set, you set both
> > > pll->data->en_mask _and_ pll->data->base_en_bit, and clear only
> > > pll->data->base_en_bit.
> > >
> >
> > Hi Nicolas,
> >
> > AFAIK, the asymmetry was intentional.
> > en_mask is actually a combination of divider enable mask and the pll
> > enable bit(CON0_BASE_EN).
> > Even without my patch, it still sets divider enable mask and en_bit, and
> > only clears en_bit.
> > You could see the pll_data in clk-mt8192.c of patch [4/4]
> > Take mainpll as an example,
> > the enable mask of mainpll is 0xff000001, where 0xff000000 is the
> > divider enable mask and 0x1 is the en_bit
> >
> > For usbpll in special case, usbpll doesn't have divider enable mask on
> > MT8192 so I give nothing(0x00000000) in the en_mask field.
> > However, the main reason why I don't skip setting the en_mask of MT8192
> > usbpll is that I'd just like to reserve the divider enable mask for any
> > special plls with divider enable mask in near future.
>
> Argh, I see, it's a bit of a can of worms, with many special cases...
>
> So I played a bit with 3 examples.
>
> Current situation looks like this:
>
> 8183 CLK_APMIXED_ARMPLL_LL
> en_mask = 0x00000001
> en_reg = 0
> base_en_bit = 0
>
> prepare: REG_CON0 |= en_mask
> unprepare: REG_CON0 &= ~CON0_BASE_EN (BIT(1))
>
> 8192 CLK_APMIXED_UNIVPLL
> en_mask = 0xff000001
> en_reg = 0x039c
> base_en_bit = 0
>
> prepare:
> REG_CON0 |= en_mask
> en_reg |= base_en_bit
> unprepare:
> en_reg &= ~base_en_bit
>
> 8192 CLK_APMIXED_USBPLL
> en_mask = 0x00000000
> en_reg = 0x03cc
> base_en_bit = 2
>
> prepare:
> REG_CON0 |= en_mask (0)
> en_reg |= base_en_bit
> unprepare:
> en_reg &= ~base_en_bit
>
> And I think the logic could still be simplified by _not_ putting
> CON0_BASE_EN in en_mask, and updating the CON0 in 2 steps: first all
> the bits that are not CON0_BASE_EN, then CON0_BASE_EN. Of course I
> assume that's it's fine to do so, but I have no idea.
>
> register_pll() {
> if (!en_addr) {
> en_reg = REG_CON0
> base_en_bit = CON0_BASE_EN
> }
> }
>
> prepare() {
> REG_CON0 |= en_mask
> en_reg |= base_en_bit
> }
>
> unprepare() {
> en_reg &= ~base_en_bit
> }
>
> Then the new clock data:
>
> 8183 CLK_APMIXED_ARMPLL_LL
> en_mask = 0x00000000 (CON0_BASE_EN is implicit, but other bits could be set)
> en_reg = 0
> base_en_bit = 0
>
> prepare: {
> REG_CON0 |= en_mask (0x00000000, here, we can skip, but other bits
> could be set)
> en_reg |= base_en_bit (REG_CON0 |= CON0_BASE_EN)
> }
> unprepare: en_reg &= ~base_en_bit (REG_CON0 &= ~CON0_BASE_EN)
>
> 8192 CLK_APMIXED_UNIVPLL
> en_mask = 0xff000001 (Note the bit 1 is _not_ dropped here, as it
> needs to be set too)
> en_reg = 0x039c
> base_en_bit = 0
> (same as above)
>
> 8192 CLK_APMIXED_USBPLL
> en_mask = 0x00000000
> en_reg = 0x03cc
> base_en_bit = 2
> (same as above)
>
> Now, maybe this is also a bit overcomplicated. Maybe a simpler
> solution is just to add a comment in prepare that "r |=
> pll->data->en_mask;" is meant to include CON0_BASE_EN in most cases,
> and then the code could be ok as-is (just to make sure that the next
> person who looks at this code does not think there is a bug...).
>
Hi Nicolas,
I thought these still too complicated and I guess the asymmetrical
problem could be fixed.
And that will make this part simpler just like what you mentioned in
previous comment.
I'll confirm ASAP and send a new version if it is possible to be fixed.
> >
> > > > + if (pll->en_addr) {
> > > > + r = readl(pll->en_addr);
> > > > + r |= BIT(pll->data->base_en_bit);
> > > > + writel(r, pll->en_addr);
> > > > + }
> > > > +
> > > > __mtk_pll_tuner_enable(pll);
> > > >
> > > > udelay(20);
> > > > @@ -277,9 +287,15 @@ static void mtk_pll_unprepare(struct clk_hw *hw)
> > > >
> > > > __mtk_pll_tuner_disable(pll);
> > > >
> > > > - r = readl(pll->base_addr + REG_CON0);
> > > > - r &= ~CON0_BASE_EN;
> > > > - writel(r, pll->base_addr + REG_CON0);
> > > > + if (pll->en_addr) {
> > > > + r = readl(pll->en_addr);
> > > > + r &= ~BIT(pll->data->base_en_bit);
> > > > + writel(r, pll->en_addr);
> > > > + } else {
> > > > + r = readl(pll->base_addr + REG_CON0);
> > > > + r &= ~CON0_BASE_EN;
> > > > + writel(r, pll->base_addr + REG_CON0);
> > > > + }
> > > >
> > > > r = readl(pll->pwr_addr) | CON0_ISO_EN;
> > > > writel(r, pll->pwr_addr);
> > > > @@ -321,6 +337,8 @@ static struct clk *mtk_clk_register_pll(const struct mtk_pll_data *data,
> > > > pll->tuner_addr = base + data->tuner_reg;
> > > > if (data->tuner_en_reg)
> > > > pll->tuner_en_addr = base + data->tuner_en_reg;
> > > > + if (data->en_reg)
> > > > + pll->en_addr = base + data->en_reg;
> > >
> > > If the answer to my question above holds (asymmetry is not
> > > intentional), this patch/the code could be simplified a lot if you
> > > also added a pll->en_bit member, and, here, did this:
> > >
> > > if (pll->en_reg) {
> > > pll->en_addr = base + data->en_reg;
> > > pll->end_bit = data->en_bit;
> > > } else {
> > > pll->en_addr = pll->base_addr + REG_CON0;
> > > pll->en_bit = CON0_BASE_EN;
> > > }
> > >
> > > > pll->hw.init = &init;
> > > > pll->data = data;
> > > >
> > > > --
> > > > 1.8.1.1.dirty
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-07-27 9:04 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-22 6:49 [PATCH 0/4] Mediatek MT8192 clock and scpsys support Weiyi Lu
2020-07-22 6:49 ` Weiyi Lu
2020-07-22 6:49 ` Weiyi Lu
2020-07-22 6:49 ` [PATCH 1/4] dt-bindings: ARM: Mediatek: Document bindings for MT8192 Weiyi Lu
2020-07-22 6:49 ` Weiyi Lu
2020-07-22 6:49 ` Weiyi Lu
2020-07-22 6:49 ` [PATCH 2/4] clk: mediatek: Add dt-bindings for MT8192 clocks Weiyi Lu
2020-07-22 6:49 ` Weiyi Lu
2020-07-22 6:49 ` Weiyi Lu
2020-07-22 6:50 ` [PATCH 3/4] clk: mediatek: Add configurable enable control to mtk_pll_data Weiyi Lu
2020-07-22 6:50 ` Weiyi Lu
2020-07-22 6:50 ` Weiyi Lu
2020-07-22 8:51 ` Nicolas Boichat
2020-07-22 8:51 ` Nicolas Boichat
2020-07-22 8:51 ` Nicolas Boichat
2020-07-23 2:57 ` Weiyi Lu
2020-07-23 2:57 ` Weiyi Lu
2020-07-23 2:57 ` Weiyi Lu
2020-07-23 7:51 ` Nicolas Boichat
2020-07-23 7:51 ` Nicolas Boichat
2020-07-23 7:51 ` Nicolas Boichat
2020-07-27 9:04 ` Weiyi Lu [this message]
2020-07-27 9:04 ` Weiyi Lu
2020-07-27 9:04 ` Weiyi Lu
2020-07-22 6:50 ` [PATCH 4/4] clk: mediatek: Add MT8192 clock support Weiyi Lu
2020-07-22 6:50 ` Weiyi Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1595840642.12203.4.camel@mtksdaap41 \
--to=weiyi.lu@mediatek.com \
--cc=drinkcat@chromium.org \
--cc=ikjn@chromium.org \
--cc=jamesjj.liao@mediatek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=robh@kernel.org \
--cc=sboyd@kernel.org \
--cc=srv_heupstream@mediatek.com \
--cc=wendell.lin@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.