[PATCH v5 08/14] clk: sifive: fu540-prci: Add clock enable and disable ops

[Pragnesh Patel]

Hi Bin,

>-----Original Message-----
>From: Bin Meng <bmeng.cn at gmail.com>
>Sent: 13 March 2020 13:27
>To: Pragnesh Patel <pragnesh.patel at sifive.com>
>Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Atish Patra
><atish.patra at wdc.com>; Palmer Dabbelt <palmerdabbelt at google.com>; Paul
>Walmsley <paul.walmsley at sifive.com>; Jagan Teki
><jagan at amarulasolutions.com>; Troy Benjegerdes
><troy.benjegerdes at sifive.com>; Anup Patel <anup.patel at wdc.com>; Sagar
>Kadam <sagar.kadam at sifive.com>; Rick Chen <rick at andestech.com>; Lukasz
>Majewski <lukma at denx.de>; Anatolij Gustschin <agust at denx.de>; Simon
>Glass <sjg at chromium.org>
>Subject: Re: [PATCH v5 08/14] clk: sifive: fu540-prci: Add clock enable and
>disable ops
>
>On Wed, Mar 11, 2020 at 3:04 PM Pragnesh Patel
><pragnesh.patel at sifive.com> wrote:
>>
>> Added clock enable and disable functions in prci ops
>>
>> Signed-off-by: Pragnesh Patel <pragnesh.patel at sifive.com>
>> ---
>>  drivers/clk/sifive/fu540-prci.c | 75
>> +++++++++++++++++++++++++++++++--
>>  1 file changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/sifive/fu540-prci.c
>> b/drivers/clk/sifive/fu540-prci.c index 8847178001..c02c0466a8 100644
>> --- a/drivers/clk/sifive/fu540-prci.c
>> +++ b/drivers/clk/sifive/fu540-prci.c
>> @@ -68,6 +68,11 @@
>>  #define PRCI_COREPLLCFG0_LOCK_SHIFT    31
>>  #define PRCI_COREPLLCFG0_LOCK_MASK     (0x1 <<
>PRCI_COREPLLCFG0_LOCK_SHIFT)
>>
>> +/* COREPLLCFG1 */
>> +#define PRCI_COREPLLCFG1_OFFSET                0x8
>> +#define PRCI_COREPLLCFG1_CKE_SHIFT     31
>> +#define PRCI_COREPLLCFG1_CKE_MASK      (0x1 <<
>PRCI_COREPLLCFG1_CKE_SHIFT)
>> +
>>  /* DDRPLLCFG0 */
>>  #define PRCI_DDRPLLCFG0_OFFSET         0xc
>>  #define PRCI_DDRPLLCFG0_DIVR_SHIFT     0
>> @@ -87,7 +92,7 @@
>>
>>  /* DDRPLLCFG1 */
>>  #define PRCI_DDRPLLCFG1_OFFSET         0x10
>> -#define PRCI_DDRPLLCFG1_CKE_SHIFT      24
>> +#define PRCI_DDRPLLCFG1_CKE_SHIFT      31
>>  #define PRCI_DDRPLLCFG1_CKE_MASK       (0x1 <<
>PRCI_DDRPLLCFG1_CKE_SHIFT)
>>
>>  /* GEMGXLPLLCFG0 */
>> @@ -114,7 +119,7 @@
>>
>>  /* GEMGXLPLLCFG1 */
>>  #define PRCI_GEMGXLPLLCFG1_OFFSET      0x20
>> -#define PRCI_GEMGXLPLLCFG1_CKE_SHIFT   24
>> +#define PRCI_GEMGXLPLLCFG1_CKE_SHIFT   31
>>  #define PRCI_GEMGXLPLLCFG1_CKE_MASK    (0x1 <<
>PRCI_GEMGXLPLLCFG1_CKE_SHIFT)
>>
>>  /* CORECLKSEL */
>> @@ -142,7 +147,7 @@
>>                         (0x1 <<
>> PRCI_DEVICESRESETREG_GEMGXL_RST_N_SHIFT)
>>
>>  /* CLKMUXSTATUSREG */
>> -#define PRCI_CLKMUXSTATUSREG_OFFSET            0x2c
>> +#define PRCI_CLKMUXSTATUSREG_OFFSET    0x2c
>>  #define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT 1  #define
>> PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK \
>>                         (0x1 <<
>> PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT)
>> @@ -170,6 +175,7 @@ struct __prci_data {
>>   * @enable_bypass: fn ptr to code to bypass the WRPLL (if applicable; else
>NULL)
>>   * @disable_bypass: fn ptr to code to not bypass the WRPLL (or NULL)
>>   * @cfg0_offs: WRPLL CFG0 register offset (in bytes) from the PRCI
>> base address
>> + * @cfg1_offs: WRPLL CFG1 register offset (in bytes) from the PRCI
>> + base address
>>   *
>>   * @enable_bypass and @disable_bypass are used for WRPLL instances
>>   * that contain a separate external glitchless clock mux downstream
>> @@ -180,6 +186,7 @@ struct __prci_wrpll_data {
>>         void (*enable_bypass)(struct __prci_data *pd);
>>         void (*disable_bypass)(struct __prci_data *pd);
>>         u8 cfg0_offs;
>> +       u8 cfg1_offs;
>>  };
>>
>>  struct __prci_clock;
>> @@ -194,6 +201,7 @@ struct __prci_clock_ops {
>>                                     unsigned long *parent_rate);
>>         unsigned long (*recalc_rate)(struct __prci_clock *pc,
>>                                      unsigned long parent_rate);
>> +       int (*enable_clk)(struct __prci_clock *pc, bool enable);
>>  };
>>
>>  /**
>> @@ -356,6 +364,13 @@ static void __prci_wrpll_write_cfg(struct
>__prci_data *pd,
>>         memcpy(&pwd->c, c, sizeof(*c));  }
>>
>> +static void __prci_wrpll_write_cfg1(struct __prci_data *pd,
>
>nits: we should also rename the existing function
>__prci_wrpll_write_cfg() to __prci_wrpll_write_cfg0()
>

I think you are right. I will check and update in v6.

>> +                                   struct __prci_wrpll_data *pwd,
>> +                                   u32 enable) {
>> +       __prci_writel(enable, pwd->cfg1_offs, pd); }
>> +
>
>[snip]
>
>Regards,
>Bin