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 <sean...@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 <jjl...@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},
>
