[PATCH V3 3/6] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver
Yu Tu
yu.tu at amlogic.com
Mon Oct 24 04:33:52 PDT 2022
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 at 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 at 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 at 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 at 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?
More information about the linux-amlogic
mailing list