Hi Bin,
>-----Original Message-----
>From: Bin Meng <bmeng...@gmail.com>
>Sent: 13 March 2020 13:27
>To: Pragnesh Patel <pragnesh.pa...@sifive.com>
>Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra
><atish.pa...@wdc.com>; Palmer Dabbelt <palmerdabb...@google.com>; Paul
>Walmsley <paul.walms...@sifive.com>; Jagan Teki
><ja...@amarulasolutions.com>; Troy Benjegerdes
><troy.benjeger...@sifive.com>; Anup Patel <anup.pa...@wdc.com>; Sagar
>Kadam <sagar.ka...@sifive.com>; Rick Chen <r...@andestech.com>; Lukasz
>Majewski <lu...@denx.de>; Anatolij Gustschin <ag...@denx.de>; Simon
>Glass <s...@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.pa...@sifive.com> wrote:
>>
>> Added clock enable and disable functions in prci ops
>>
>> Signed-off-by: Pragnesh Patel <pragnesh.pa...@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
