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

[Yu Tu]

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@xxxxxxxxxxx> wrote:
> >
> > > Add the peripherals clock controller driver in the s4 SoC family.
> > >
> > > Signed-off-by: Yu Tu <yu.tu@xxxxxxxxxxx>
> > > ---
> > >   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