Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230321193014.26349-1-ddrokosov@sberdevices.ru>
 <20230321193014.26349-6-ddrokosov@sberdevices.ru>
User-agent: mu4e 1.8.13; emacs 28.2
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>,
        neil.armstrong@linaro.org, mturquette@baylibre.com,
        sboyd@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, khilman@baylibre.com,
        martin.blumenstingl@googlemail.com
Cc:     jian.hu@amlogic.com, kernel@sberdevices.ru, rockosov@gmail.com,
        linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v11 5/5] clk: meson: a1: add Amlogic A1 Peripherals
 clock controller driver
Date:   Mon, 27 Mar 2023 11:58:30 +0200
In-reply-to: <20230321193014.26349-6-ddrokosov@sberdevices.ru>
Message-ID: <1jtty6o5so.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <ddrokosov@sberdevices.ru> 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@amlogic.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---
>  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.

> +		.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.

> +		.flags = CLK_SET_RATE_NO_REPARENT,
> +	},
> +};
> +

[...]

> +
> +static int meson_a1_periphs_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	void __iomem *base;
> +	struct regmap *map;
> +	int i, err;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return dev_err_probe(dev, PTR_ERR(base),
> +				     "can't ioremap resource\n");
> +
> +	map = devm_regmap_init_mmio(dev, base, &a1_periphs_regmap_cfg);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map),
> +				     "can't init regmap mmio region\n");
> +
> +	/* Populate regmap for the regmap backed clocks */
> +	for (i = 0; i < ARRAY_SIZE(a1_periphs_regmaps); i++)
> +		a1_periphs_regmaps[i]->map = map;
> +
> +	/* DT clocks registration */
> +	err = meson_a1_periphs_clks_register(dev, &a1_periphs_public_clks);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "public clks registration failed\n");
> +
> +	/* Internal clocks registration */
> +	err = meson_a1_periphs_clks_register(dev, &a1_periphs_private_clks);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "private clks registration failed\n");

I defenitely don't like this public/private concept.
Please remove it

> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					   &a1_periphs_public_clks);
> +}
> +