Re: [PATCH V3 3/6] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver

[Yu Tu <yu.tu@amlogic.com>]

Hi Jerome,
	Thank you for your reply.

On 2022/10/22 20:22, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Thu 29 Sep 2022 at 15:07, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Hi Jerome,
>> 	Thank you for your reply.
>>
>> On 2022/9/28 23:27, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>> On Wed 21 Sep 2022 at 16:40, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> Hi Jerome，
>>>>
>>>> On 2022/8/30 15:37, Yu Tu wrote:
>>>>> On 2022/8/30 14:44, Jerome Brunet wrote:
>>>>>> [ EXTERNAL EMAIL ]
>>>>>>
>>>>>>
>>>>>> On Tue 30 Aug 2022 at 14:13, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>
>>>>>>> On 2022/8/29 17:48, Jerome Brunet wrote:
>>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>>> On Mon 15 Aug 2022 at 21:20, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> +static struct clk_regmap s4_hdmi_pll_dco = {
>>>>>>>>>>>> +    .data = &(struct meson_clk_pll_data){
>>>>>>>>>>>> +        .en = {
>>>>>>>>>>>> +            .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>>> +            .shift   = 28,
>>>>>>>>>>>> +            .width   = 1,
>>>>>>>>>>>> +        },
>>>>>>>>>>>> +        .m = {
>>>>>>>>>>>> +            .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>>> +            .shift   = 0,
>>>>>>>>>>>> +            .width   = 8,
>>>>>>>>>>>> +        },
>>>>>>>>>>>> +        .n = {
>>>>>>>>>>>> +            .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>>> +            .shift   = 10,
>>>>>>>>>>>> +            .width   = 5,
>>>>>>>>>>>> +        },
>>>>>>>>>>>> +        .frac = {
>>>>>>>>>>>> +            .reg_off = ANACTRL_HDMIPLL_CTRL1,
>>>>>>>>>>>> +            .shift   = 0,
>>>>>>>>>>>> +            .width   = 17,
>>>>>>>>>>>> +        },
>>>>>>>>>>>> +        .l = {
>>>>>>>>>>>> +            .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>>> +            .shift   = 31,
>>>>>>>>>>>> +            .width   = 1,
>>>>>>>>>>>> +        },
>>>>>>>>>>>> +        .rst = {
>>>>>>>>>>>> +            .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>>> +            .shift   = 29,
>>>>>>>>>>>> +            .width   = 1,
>>>>>>>>>>>> +        },
>>>>>>>>>>>> +    },
>>>>>>>>>>>> +    .hw.init = &(struct clk_init_data){
>>>>>>>>>>>> +        .name = "hdmi_pll_dco",
>>>>>>>>>>>> +        .ops = &meson_clk_pll_ro_ops,
>>>>>>>>>>>> +        .parent_data = (const struct clk_parent_data []) {
>>>>>>>>>>>> +            { .fw_name = "xtal", }
>>>>>>>>>>>> +        },
>>>>>>>>>>>> +        .num_parents = 1,
>>>>>>>>>>>> +        /*
>>>>>>>>>>>> +         * Display directly handle hdmi pll registers ATM, we need
>>>>>>>>>>>> +         * NOCACHE to keep our view of the clock as accurate as
>>>>>>>>>>>> +         * possible
>>>>>>>>>>>> +         */
>>>>>>>>>>>
>>>>>>>>>>> Is it really ?
>>>>>>>>>>>
>>>>>>>>>>> Given that HDMI support for the s4 is there yet, the
>>>>>>>>>>> addresses have changes and the region is no longer a syscon, it is
>>>>>>>>>>> time
>>>>>>>>>>> for the HDMI driver to get fixed.
>>>>>>>>> The HDMI PLL is configured in the Uboot phase and does not change the
>>>>>>>>> frequency in the kernel phase. So we use the NOCACHE flag and
>>>>>>>>> "ro_ops".
>>>>>>>> That's no reason to put NOCACHE or ro-ops
>>>>>>>> If you want the frequencies to be statically assinged, the correct way
>>>>>>>> would be through assigned-rate in DT I guess.
>>>>>>>
>>>>>>> Okay. You're right. However, when registering with OPS, HDMI PLL will be
>>>>>>> reset. It takes time for PLL to stabilize the output frequency, which
>>>>>>> will
>>>>>>> lead to the startup screen flashing.
>>>>>>>
>>>>>>> I would like to know how to solve this problem if not using ro_ops.
>>>>>>>
>>>>>>>>
>>>>>>
>>>>>> You can add new ops or tweak the current init function.
>>>>> HDMI PLL is not different from other PLLS, so I think adding OPS is
>>>>> weird.
>>>>>
>>>>>>
>>>>>> Safest would be to do the following :
>>>>>>     * Check if the PLLs is already on.
>>>>>>     * Check if the 'pll->init_regs' matches what is already set
>>>>>>       - if so, you can skip the reset
>>>>>>       - if not, you need to reset as usual
>>>>> static int meson_clk_pll_init(struct clk_hw *hw)
>>>>> {
>>>>>            struct clk_regmap *clk = to_clk_regmap(hw);
>>>>>            struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>>>>            if (pll->init_count) {
>>>>>                    meson_parm_write(clk->map, &pll->rst, 1);
>>>>>                    regmap_multi_reg_write(clk->map, pll->init_regs,
>>>>>                                    |      pll->init_count);
>>>>>                    meson_parm_write(clk->map, &pll->rst, 0);
>>>>>            }
>>>>>            return 0;
>>>>> }
>>>>> Because the init function looks like this. Therefore, HDMI PLL init_count
>>>>> is not given.
>>> I don't get the remark. You've got pll->init_count right there.
>>>
>>>>> Can I change it like this?
>>> What change ? The function above looks a lot like  meson_clk_pll_init()
>>> in the actual source
>>>
>>>>
>>>> I don't know if this change meets your requirements? Please give us your
>>>> valuable advice.
>>> What change ?
>>
>> static struct clk_regmap s4_hdmi_pll_dco = {          .data = &(struct
>> meson_clk_pll_data){                  .en = {
>> .reg_off = ANACTRL_HDMIPLL_CTRL0,                          .shift   = 28,
>> .width   = 1,                  },                  .m = {
>> .reg_off = ANACTRL_HDMIPLL_CTRL0,                          .shift   = 0,
>> .width   = 8,                  },                  .n = {
>> .reg_off = ANACTRL_HDMIPLL_CTRL0,                          .shift   = 10,
>> .width   = 5,                  },                  .frac = {
>> .reg_off = ANACTRL_HDMIPLL_CTRL1,                          .shift   = 0,
>> .width   = 17,                  },                  .l = {
>> .reg_off = ANACTRL_HDMIPLL_CTRL0,                          .shift   = 31,
>> .width   = 1,                  },                  .rst = {
>> .reg_off = ANACTRL_HDMIPLL_CTRL0,                          .shift   = 29,
>> .width   = 1,                  },                  .range =
>> &s4_gp0_pll_mult_range,          },          .hw.init = &(struct
>> clk_init_data){                  .name = "hdmi_pll_dco",
>> .ops = &meson_clk_pll_ops,                  .parent_data = (const struct
>> clk_parent_data []) {                          { .fw_name = "xtal", }
>> },                  .num_parents = 1,          }, };
> 
> I'm sorry but I can't read this
> 
>>
>> This is my code right now. Because init_count and init_regs are not
>> defined, HDMI PLL is not reset. In this way, the kernel will not blink when
>> the Uboot starts. Then in the kernel stage, if we want to change the HDMI
>> PLL frequency value, we can directly change M, N and OD. In fact, we will
>> not change the HDMI PLL frequency value later.
>>
>> I wonder if you accept this change?

Sorry, the code is not formatted properly.

> 
> I think it would be better to separate this topic from your new SoC support
> 
> I think you can make another version of the clock controller for the new
> SoC, keeping the pll_ops as it is now.
> 
> With another series, you may submit something to avoid reseting the PLL if
> is already correctly initialized. This can be useful to all the SoC, not
> just the last one. It will be easier to discuss on properly formatted
> patches

Do you mean that hdmi_pll, I will submit the next version of code 
according to pll_ro_ops first, and then we will discuss how to reject 
the patch of reset pll in another topic?