| Message ID | 40981d0bb722eb509628bcf82c31f632e4cf747a.1701823757.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/4] dt-bindings: clock: mediatek: add MT7988 clock IDs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Il 06/12/23 01:57, Daniel Golle ha scritto: > From: Sam Shih <sam.shih@mediatek.com> > > Introduce pcw_chg_shfit control to optionally use that instead of the > hardcoded PCW_CHG_MASK macro. > This will needed for clocks on the MT7988 SoC. > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > v3: use git --from ... > v2: no changes > > drivers/clk/mediatek/clk-pll.c | 5 ++++- > drivers/clk/mediatek/clk-pll.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > index 513ab6b1b3229..9f08bc5d2a8a2 100644 > --- a/drivers/clk/mediatek/clk-pll.c > +++ b/drivers/clk/mediatek/clk-pll.c > @@ -114,7 +114,10 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, > pll->data->pcw_shift); > val |= pcw << pll->data->pcw_shift; > writel(val, pll->pcw_addr); > - chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; > + if (pll->data->pcw_chg_shift) > + chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); > + else > + chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; > writel(chg, pll->pcw_chg_addr); > if (pll->tuner_addr) > writel(val + 1, pll->tuner_addr); > diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h > index f17278ff15d78..d28d317e84377 100644 > --- a/drivers/clk/mediatek/clk-pll.h > +++ b/drivers/clk/mediatek/clk-pll.h > @@ -44,6 +44,7 @@ struct mtk_pll_data { > u32 pcw_reg; > int pcw_shift; > u32 pcw_chg_reg; > + int pcw_chg_shift; > const struct mtk_pll_div_table *div_table; > const char *parent_name; > u32 en_reg; Hmm... no, this is not the best at all and can be improved. Okay, so, the situation here is that one or some PLL(s) on MT7988 have a different PCW_CHG_MASK as far as I understand. Situation here is: - Each PLL must be registered to clk-pll - Each driver declaring a PLL does exactly so - There's a function to register the PLL You definitely don't want to add a conditional in pll_set_rate(): even though this is technically not a performance path on the current SoCs (and will probably never be), it's simply useless to have this (very small) overhead there. The solution is to: - Change that pcw_chg_shift to an unsigned short int type (or u8, your call): you don't need 32 bits for this number, as the expected range of this member is [0-31], and this can be expressed in just 4 bits (u8 is the smallest though) - Add that to function mtk_clk_register_pll_ops() - Change mtk_pll_set_rate_regs() to always do chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); Cheers, Angelo
Hi Angelo, thank you for taking the time to review and for the helpful comments. On Wed, Dec 06, 2023 at 11:38:36AM +0100, AngeloGioacchino Del Regno wrote: > Il 06/12/23 01:57, Daniel Golle ha scritto: > > From: Sam Shih <sam.shih@mediatek.com> > > > > Introduce pcw_chg_shfit control to optionally use that instead of the > > hardcoded PCW_CHG_MASK macro. > > This will needed for clocks on the MT7988 SoC. > > > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > v3: use git --from ... > > v2: no changes > > > > drivers/clk/mediatek/clk-pll.c | 5 ++++- > > drivers/clk/mediatek/clk-pll.h | 1 + > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > > index 513ab6b1b3229..9f08bc5d2a8a2 100644 > > --- a/drivers/clk/mediatek/clk-pll.c > > +++ b/drivers/clk/mediatek/clk-pll.c > > @@ -114,7 +114,10 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, > > pll->data->pcw_shift); > > val |= pcw << pll->data->pcw_shift; > > writel(val, pll->pcw_addr); > > - chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; > > + if (pll->data->pcw_chg_shift) > > + chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); > > + else > > + chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; > > writel(chg, pll->pcw_chg_addr); > > if (pll->tuner_addr) > > writel(val + 1, pll->tuner_addr); > > diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h > > index f17278ff15d78..d28d317e84377 100644 > > --- a/drivers/clk/mediatek/clk-pll.h > > +++ b/drivers/clk/mediatek/clk-pll.h > > @@ -44,6 +44,7 @@ struct mtk_pll_data { > > u32 pcw_reg; > > int pcw_shift; > > u32 pcw_chg_reg; > > + int pcw_chg_shift; > > const struct mtk_pll_div_table *div_table; > > const char *parent_name; > > u32 en_reg; > > Hmm... no, this is not the best at all and can be improved. > > Okay, so, the situation here is that one or some PLL(s) on MT7988 have a different > PCW_CHG_MASK as far as I understand. Correct. *All* clocks of MT7988 have a different PCW_CHG_MASK, BIT(2) instead of BIT(31). > > Situation here is: > - Each PLL must be registered to clk-pll > - Each driver declaring a PLL does exactly so > - There's a function to register the PLL > > You definitely don't want to add a conditional in pll_set_rate(): even though > this is technically not a performance path on the current SoCs (and will probably > never be), it's simply useless to have this (very small) overhead there. > > The solution is to: > - Change that pcw_chg_shift to an unsigned short int type (or u8, your call): > you don't need 32 bits for this number, as the expected range of this member > is [0-31], and this can be expressed in just 4 bits (u8 is the smallest though) Ack will use u8 instead, despite the struct not being packed, so I wonder if it actually makes a difference. > - Add that to function mtk_clk_register_pll_ops() > - Change mtk_pll_set_rate_regs() to always do > chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); As mtk_pll_data is a read-only member of the mtk_pll struct, we can't set pcw_chg_shift to 31 in mtk_clk_register_pll_ops() in case it is set to 0. The only (much more intrusive change) would be to explicitely declare .pcw_chg_shift = 31 in all current drivers setting .pcs_chg_reg != 0. Should I do that instead?
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c index 513ab6b1b3229..9f08bc5d2a8a2 100644 --- a/drivers/clk/mediatek/clk-pll.c +++ b/drivers/clk/mediatek/clk-pll.c @@ -114,7 +114,10 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, pll->data->pcw_shift); val |= pcw << pll->data->pcw_shift; writel(val, pll->pcw_addr); - chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; + if (pll->data->pcw_chg_shift) + chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); + else + chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; writel(chg, pll->pcw_chg_addr); if (pll->tuner_addr) writel(val + 1, pll->tuner_addr); diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h index f17278ff15d78..d28d317e84377 100644 --- a/drivers/clk/mediatek/clk-pll.h +++ b/drivers/clk/mediatek/clk-pll.h @@ -44,6 +44,7 @@ struct mtk_pll_data { u32 pcw_reg; int pcw_shift; u32 pcw_chg_reg; + int pcw_chg_shift; const struct mtk_pll_div_table *div_table; const char *parent_name; u32 en_reg;