[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 <seanga2 at gmail.com>

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