Re: [PATCH v15 6/6] clk: meson: a1: add Amlogic A1 Peripherals clock controller driver

[Dmitry Rokosov <ddrokosov@sberdevices.ru>]

Hello Martin,

Thank you so much for the review, I really appreciate it!
Please find my comments below.

On Fri, May 19, 2023 at 11:03:54PM +0200, Martin Blumenstingl wrote:
> Hi Dmitry,
> 
> On Wed, May 17, 2023 at 3:33 PM Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:
> [...]
> > +static struct clk_regmap sys_b_sel = {
> > +       .data = &(struct clk_regmap_mux_data){
> > +               .offset = SYS_CLK_CTRL0,
> > +               .mask = 0x7,
> > +               .shift = 26,
> > +               .table = mux_table_sys,
> > +       },
> > +       .hw.init = &(struct clk_init_data){
> > +               .name = "sys_b_sel",
> > +               .ops = &clk_regmap_mux_ro_ops,
> the sys_*_sel muxes and sys_*_gate are _ro...
> 
> > +               .parent_data = sys_parents,
> > +               .num_parents = ARRAY_SIZE(sys_parents),
> > +       },
> > +};
> > +
> > +static struct clk_regmap sys_b_div = {
> > +       .data = &(struct clk_regmap_div_data){
> > +               .offset = SYS_CLK_CTRL0,
> > +               .shift = 16,
> > +               .width = 10,
> > +       },
> > +       .hw.init = &(struct clk_init_data){
> > +               .name = "sys_b_div",
> > +               .ops = &clk_regmap_divider_ops,
> ...but the sys_*_div aren't
> Is this on purpose? If it is: why can the divider be changed at
> runtime but the mux can't?
> 

Ah, that's a good catch. Since the system clock is set up by the BootROM
code, all sys_* dividers and gates should be read-only. I'll make sure
to change that in the next version.

> [...]
> > +/*
> > + * the index 2 is sys_pll_div16, it will be implemented in the CPU clock driver,
> We need to add the "sys_pll_div16" input to the dt-bindings since they
> should always describe the hardware (regardless of what the driver
> implements currently).
> I'm not sure how to manage this while we don't have the CPU clock
> driver ready yet but I'm sure Rob or Krzysztof will be able to help us
> here.
> 

I've shared my thoughts about it in the bindings thread. Please take a
look.

> > + * the index 4 is the clock measurement source, it's not supported yet
> I suspect that this comes from the clock measurer IP block and if so
> the dt-bindings should probably describe this input. But again, we'd
> need to keep it optional for now since our clock measurer driver
> doesn't even implement a clock controller.
> 

Indeed, this is a similar situation to what we have with the inputs and
clocks of the CPU and Audio clock controllers. It seems like there is
only one option here: we should mark it with a TODO tag...

> [...]
> > +static struct clk_regmap pwm_a_sel = {
> > +       .data = &(struct clk_regmap_mux_data){
> > +               .offset = PWM_CLK_AB_CTRL,
> > +               .mask = 0x1,
> > +               .shift = 9,
> > +       },
> > +       .hw.init = &(struct clk_init_data){
> > +               .name = "pwm_a_sel",
> > +               .ops = &clk_regmap_mux_ops,
> > +               .parent_data = pwm_abcd_parents,
> > +               .num_parents = ARRAY_SIZE(pwm_abcd_parents),
> > +               /* For more information, please refer to rtc clock */
> > +               .flags = CLK_SET_RATE_NO_REPARENT,
> As mentioned in [0] we'll work with Heiner to see if we can improve
> the decision making process of the PWM controller driver so that we
> can just have .flags = 0 here.
> This applies to all other occurrences of the same comment about the rtc clock.

Sure, I'll make the change in v16. In my opinion, we should remove the
CLK_SET_RATE_NO_REPARENT flag from all RTC related clock objects,
including PWM, regardless of the outcome of the Heiner discussion. Based
on our IRC talk, the decision has more pros than cons -
https://libera.irclog.whitequark.org/linux-amlogic/2023-05-18

-- 
Thank you,
Dmitry