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

[Sean Anderson]

Hi Jim,

On 11/12/23 21:42, Jim Liu wrote:

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

Makes sense. Please add this to your commit message if you do a v3.

Reviewed-by: Sean Anderson <sean...@gmail.com>

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},