Re: [PATCH V9 4/4] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

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

Hi Dmitry,

On 2023/6/6 23:38, Dmitry Rokosov wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello Yu,
> 
> On Tue, Jun 06, 2023 at 04:38:15PM +0200, Jerome Brunet wrote:
>>
>> On Wed 17 May 2023 at 15:02, Yu Tu <yu.tu@amlogic.com> wrote:
>>
>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>> ---
>>>   drivers/clk/meson/Kconfig          |   12 +
>>>   drivers/clk/meson/Makefile         |    1 +
>>>   drivers/clk/meson/s4-peripherals.c | 3830 ++++++++++++++++++++++++++++
>>>   drivers/clk/meson/s4-peripherals.h |  217 ++
>>>   4 files changed, 4060 insertions(+)
>>>   create mode 100644 drivers/clk/meson/s4-peripherals.c
>>>   create mode 100644 drivers/clk/meson/s4-peripherals.h
>>>
>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>> index a663c90a3f3b..a6eb9fa15c74 100644
>>> --- a/drivers/clk/meson/Kconfig
>>> +++ b/drivers/clk/meson/Kconfig
>>> @@ -128,4 +128,16 @@ config COMMON_CLK_S4_PLL
>>>        aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
>>>        Say Y if you want the board to work, because plls are the parent of most
>>>        peripherals.
>>> +
>>> +config COMMON_CLK_S4
>>> +   tristate "S4 SoC Peripherals clock controllers support"
>>> +   depends on ARM64
>>> +   default y
>>> +   select COMMON_CLK_MESON_REGMAP
>>> +   select COMMON_CLK_MESON_DUALDIV
>>> +   select COMMON_CLK_MESON_VID_PLL_DIV
>>> +   help
>>> +     Support for the Peripherals clock controller on Amlogic S805X2 and S905Y4
>>> +     devices, aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
>>> +     Say Y if you want peripherals to work.
>>>   endmenu
> 
> [...]
> 
>>> +static struct clk_regmap s4_rtc_32k_by_oscin = {
>>> +   .data = &(struct clk_regmap_gate_data){
>>> +           .offset = CLKCTRL_RTC_BY_OSCIN_CTRL0,
>>> +           .bit_idx = 30,
>>> +   },
>>> +   .hw.init = &(struct clk_init_data) {
>>> +           .name = "rtc_32k_by_oscin",
>>> +           .ops = &clk_regmap_gate_ops,
>>> +           .parent_hws = (const struct clk_hw *[]) {
>>> +                   &s4_rtc_32k_by_oscin_sel.hw
>>> +           },
>>> +           .num_parents = 1,
>>> +           .flags = CLK_SET_RATE_PARENT,
>>> +   },
>>> +};
>>> +
>>> +/*
>>> + * This RTC clock can be supplied by an external 32KHz crystal oscillator.
>>> + * If it is used, it should be documented in using fw_name and documented in the
>>> + * Bindings. Not currently in use on this board.
>>> + */
>>
>> This is confusing and not really helpful
>> What you describe here is simply the purpose of fw_name ... so it does
>> not warrant a specific comment
>>
>>> +static const struct clk_parent_data rtc_clk_sel_parent_data[] = {
>>> +   { .hw = &s4_rtc_32k_by_oscin.hw },
>>> +   { .hw = &s4_rtc_32k_by_oscin_div.hw },
>>> +   { .fw_name = "ext_32k",  }
>>> +};
>>> +
>>> +/*
>>> + * All clocks that can be inherited from a more accurate RTC clock are marked
>>> + * with the CLK_SET_RATE_NO_REPARENT flag. This is because in certain
>>> + * situations, we may need to freeze their parent. The parent setup of these
>>> + * clocks should be located on the device tree side.
>>> + */
>>
>> It looks like the consensus is that CLK_SET_RATE_NO_REPARENT is not
>> required. Please have at look at the discussion between Dmitry and
>> Martin for the a1 controller
>>
> 
> I hope below links will be helpful for you:
> 
> CLK_SET_RATE_NO_REPARENT IRC discussion:
> https://libera.irclog.whitequark.org/linux-amlogic/2023-05-18
> 
> Clock driver LKML discussion about CLK_SET_RATE_NO_REPARENT:
> https://lore.kernel.org/all/20230530120640.irugyrio3qa7czjy@CAB-WSD-L081021/
> https://lore.kernel.org/all/20230524092750.ldm362chnpkwkcj4@CAB-WSD-L081021/
> 
> PWM discussion about special RTC case:
> https://lore.kernel.org/all/20230522133739.7tc35zr2npsysopd@CAB-WSD-L081021/
> 
> And I apologize for any confusion I may have caused in our previous
> discussion. I want to clarify that I have updated the implementation
> of CLK_SET_RATE_NO_REPARENT after discussing it with Martin...
> 
> [...]

Thank you so much for your comments.

> 
> --
> Thank you,
> Dmitry