[PATCH v2] clk: nuvoton: add read only feature for clk driver

[Jim Liu]

Hi Sean

Thanks for your review.

The spi clock setting is related to booting flash, it is setup by
early bootloader and we don't want u-boot to change it


Best regards,
Jim


On Thu, Nov 9, 2023 at 2:07 AM Sean Anderson <seanga2 at gmail.com> wrote:
>
> On 11/7/23 04:01, Jim Liu wrote:
> > Add a flag to set ahb/apb/fiu clock divider as read-only
> > It just protects the clock source and can't modify it in uboot.
>
> Thanks for adding this
>
> > Signed-off-by: Jim Liu <JJLIU0 at nuvoton.com>
> > ---
> > Changes for v2:
> >     - add commit message
> > ---
> >   drivers/clk/nuvoton/clk_npcm.c    | 15 ++++++++++++---
> >   drivers/clk/nuvoton/clk_npcm.h    |  1 +
> >   drivers/clk/nuvoton/clk_npcm8xx.c | 12 ++++++------
> >   3 files changed, 19 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/clk/nuvoton/clk_npcm.c b/drivers/clk/nuvoton/clk_npcm.c
> > index 8d71f2a24b..18cb9cddbf 100644
> > --- a/drivers/clk/nuvoton/clk_npcm.c
> > +++ b/drivers/clk/nuvoton/clk_npcm.c
> > @@ -135,7 +135,7 @@ static u32 npcm_clk_get_div(struct clk *clk)
> >       return div;
> >   }
> >
> > -static u32 npcm_clk_set_div(struct clk *clk, u32 div)
> > +static int npcm_clk_set_div(struct clk *clk, u32 div)
> >   {
> >       struct npcm_clk_priv *priv = dev_get_priv(clk->dev);
> >       struct npcm_clk_div *divider;
> > @@ -145,6 +145,9 @@ static u32 npcm_clk_set_div(struct clk *clk, u32 div)
> >       if (!divider)
> >               return -EINVAL;
> >
> > +     if (divider->flags & DIV_RO)
> > +             return 0;
> > +
> >       if (divider->flags & PRE_DIV2)
> >               div = div >> 1;
> >
> > @@ -153,6 +156,12 @@ static u32 npcm_clk_set_div(struct clk *clk, u32 div)
> >       else
> >               clkdiv = ilog2(div);
> >
> > +     if (clkdiv > (divider->mask >> (ffs(divider->mask) - 1))) {
> > +             printf("clkdiv(%d) for clk(%ld) is over limit\n",
> > +                    clkdiv, clk->id);
> > +             return -EINVAL;
> > +     }
> > +
> >       val = readl(priv->base + divider->reg);
> >       val &= ~divider->mask;
> >       val |= (clkdiv << (ffs(divider->mask) - 1)) & divider->mask;
> > @@ -253,8 +262,8 @@ static ulong npcm_clk_set_rate(struct clk *clk, ulong rate)
> >       if (ret)
> >               return ret;
> >
> > -     debug("%s: rate %lu, new rate (%lu / %u)\n", __func__, rate, parent_rate, div);
> > -     return (parent_rate / div);
> > +     debug("%s: rate %lu, new rate %lu\n", __func__, rate, npcm_clk_get_rate(clk));
> > +     return npcm_clk_get_rate(clk);
> >   }
> >
> >   static int npcm_clk_set_parent(struct clk *clk, struct clk *parent)
> > diff --git a/drivers/clk/nuvoton/clk_npcm.h b/drivers/clk/nuvoton/clk_npcm.h
> > index 06b60dc8b8..b4726d8381 100644
> > --- a/drivers/clk/nuvoton/clk_npcm.h
> > +++ b/drivers/clk/nuvoton/clk_npcm.h
> > @@ -50,6 +50,7 @@
> >   #define PRE_DIV2    BIT(2)  /* Pre divisor = 2 */
> >   #define POST_DIV2   BIT(3)  /* Post divisor = 2 */
> >   #define FIXED_PARENT        BIT(4)  /* clock source is fixed */
> > +#define DIV_RO               BIT(5)  /* divider is read-only */
> >
> >   /* Parameters of PLL configuration */
> >   struct npcm_clk_pll {
> > diff --git a/drivers/clk/nuvoton/clk_npcm8xx.c b/drivers/clk/nuvoton/clk_npcm8xx.c
> > index 27e3cfcf55..d1b32e3237 100644
> > --- a/drivers/clk/nuvoton/clk_npcm8xx.c
> > +++ b/drivers/clk/nuvoton/clk_npcm8xx.c
> > @@ -45,12 +45,12 @@ static struct npcm_clk_select npcm8xx_clk_selectors[] = {
> >   };
> >
> >   static struct npcm_clk_div npcm8xx_clk_dividers[] = {
> > -     {NPCM8XX_CLK_AHB, CLKDIV1, CLK4DIV, DIV_TYPE1 | PRE_DIV2},
> > -     {NPCM8XX_CLK_APB2, CLKDIV2, APB2CKDIV, DIV_TYPE2},
> > -     {NPCM8XX_CLK_APB5, CLKDIV2, APB5CKDIV, DIV_TYPE2},
> > -     {NPCM8XX_CLK_SPI0, CLKDIV3, SPI0CKDIV, DIV_TYPE1},
> > -     {NPCM8XX_CLK_SPI1, CLKDIV3, SPI1CKDIV, DIV_TYPE1},
> > -     {NPCM8XX_CLK_SPI3, CLKDIV1, SPI3CKDIV, DIV_TYPE1},
> > +     {NPCM8XX_CLK_AHB, CLKDIV1, CLK4DIV, DIV_TYPE1 | PRE_DIV2 | DIV_RO},
> > +     {NPCM8XX_CLK_APB2, CLKDIV2, APB2CKDIV, DIV_TYPE2 | DIV_RO},
> > +     {NPCM8XX_CLK_APB5, CLKDIV2, APB5CKDIV, DIV_TYPE2 | DIV_RO},
>
> And this makes sense for bus clocks
>
> > +     {NPCM8XX_CLK_SPI0, CLKDIV3, SPI0CKDIV, DIV_TYPE1 | DIV_RO},
> > +     {NPCM8XX_CLK_SPI1, CLKDIV3, SPI1CKDIV, DIV_TYPE1 | DIV_RO},
> > +     {NPCM8XX_CLK_SPI3, CLKDIV1, SPI3CKDIV, DIV_TYPE1 | DIV_RO},
>
> But it's a little unclear to me why this is enabled for SPI. Are you e.g.
> doing XIP from SPI?
>
> --Sean
>
> >       {NPCM8XX_CLK_SPIX, CLKDIV3, SPIXCKDIV, DIV_TYPE1},
> >       {NPCM8XX_CLK_UART, CLKDIV1, UARTDIV1, DIV_TYPE1},
> >       {NPCM8XX_CLK_UART2, CLKDIV3, UARTDIV2, DIV_TYPE1},
>