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,
Dmitry
