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

[Dmitry Rokosov]

On Mon, Mar 27, 2023 at 11:58:30AM +0200, Jerome Brunet wrote:
> 
> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> wrote:
> 
> > Introduce Peripherals clock controller for Amlogic A1 SoC family.
> >
> > A1 SoC has four clock controllers on the board: PLL, Peripherals, CPU,
> > and Audio.
> > This patchset adds support for Amlogic A1 Peripherals clock driver and
> > allows to generate clocks for all A1 SoC peripheral IPs.
> >
> > Signed-off-by: Jian Hu <jian.hu@xxxxxxxxxxx>
> > Signed-off-by: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx>
> > ---
> >  drivers/clk/meson/Kconfig  |   10 +
> >  drivers/clk/meson/Makefile |    1 +
> >  drivers/clk/meson/a1.c     | 2344 ++++++++++++++++++++++++++++++++++++
> >  drivers/clk/meson/a1.h     |   49 +
> >  4 files changed, 2404 insertions(+)
> >  create mode 100644 drivers/clk/meson/a1.c
> >  create mode 100644 drivers/clk/meson/a1.h
> >
> 
> [...]
> 
> > +static struct clk_regmap dspa_b_sel = {
> > +	.data = &(struct clk_regmap_mux_data){
> > +		.offset = DSPA_CLK_CTRL0,
> > +		.mask = 0x7,
> > +		.shift = 26,
> > +		.table = mux_table_dsp_ab,
> > +	},
> > +	.hw.init = &(struct clk_init_data){
> > +		.name = "dspa_b_sel",
> > +		.ops = &clk_regmap_mux_ops,
> > +		.parent_data = dsp_ab_parent_data,
> > +		.num_parents = ARRAY_SIZE(dsp_ab_parent_data),
> > +		/*
> > +		 * DSPA_B clk can be inherited from more accurate RTC clock,
> > +		 * so in some situations we may want to freeze its parent.
> > +		 * Therefore force dsp parent setup on device tree side
> > +		 */
> 
> This gets copy-pasted A LOT.
> Please comment this once and for all ... then reference the comment if
> you need to.
> 

Agree. No problem. I'll collapse them in the next version.

> > +		.flags = CLK_SET_RATE_NO_REPARENT,
> > +	},
> > +};
> 
> [...]
> 
> > +static struct clk_regmap cecb_32k_sel = {
> > +	.data = &(struct clk_regmap_mux_data) {
> > +		.offset = CECB_CLK_CTRL1,
> > +		.mask = 0x1,
> > +		.shift = 31,
> > +		.flags = CLK_MUX_ROUND_CLOSEST,
> > +	},
> > +	.hw.init = &(struct clk_init_data){
> > +		.name = "cecb_32k_sel",
> > +		.ops = &clk_regmap_mux_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&cecb_32k_sel_pre.hw,
> > +			&rtc.hw,
> > +		},
> > +		.num_parents = 2,
> > +		/*
> > +		 * CECB_32K clk can be inherited from more accurate RTC clock,
> > +		 * so in some situations we may want to freeze its parent.
> > +		 * Therefore force cecb_32k parent setup on device tree side
> > +		 */
> 
> How do you plan on doing that ?
> 
> You've made the parent "private" and it is not even available to be
> referenced in DT.
> 
> This whole concept of public and private to seems broken to me.
> I would much prefer you keep the existing model, with one big table and
> possibly some IDs not exposed in the bindings.
> 
> This gives an hint about the clocks we think consummers should use while
> allowing us to change things if we got it wrong, and keep the IDs stable
> for DT. This has been working so far. I don't see the benefit of
> changing that right now.
> 

As we disccused in the IRC, there is two approaches:

1) public/private clock bindings based on splitting bindings headers
2) public/private clock objects based on limited clock hw provider
   registration

I'll redesign the whole patch series to the 1st approach with keeping the
following strict rules in the mind:
"There is no option to chage public or private CLKID list numbers.
"Never said you were allowed to change the meaning of the ID, you are just
allowed to have them declared in the bindings (or not)"

[...]

-- 
Thank you,
Dmitry