Re: [PATCH v5 2/3] clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller

[Jerome Brunet]

On Sat 28 Mar 2020 at 01:32, Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx> wrote:

> The SDHC (MMC) controller embeds a clock controller inside one of the
> SDHC registers. The outputs of thisclock controller are dedicated to the
> SDHC controller.
>
> Implement a dedicated clock controller driver so we can keep all the
> clock specific logic outside of the MMC controller driver. There is no
> dedicated clock controller OF node because the hardware is a big SDHC IP
> block with an embedded clock controller (so the .dts doesn't need a
> separate clock controller node). Instead this driver re-uses the regmap
> as registered by the (platform_device) parent.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> ---
>  drivers/clk/meson/Kconfig         |   9 ++
>  drivers/clk/meson/Makefile        |   1 +
>  drivers/clk/meson/meson-mx-sdhc.c | 212 ++++++++++++++++++++++++++++++
>  3 files changed, 222 insertions(+)
>  create mode 100644 drivers/clk/meson/meson-mx-sdhc.c
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index dabeb435d067..8769335d2d46 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -53,6 +53,15 @@ config COMMON_CLK_MESON8B
>  	  S805 (Meson8b) and S812 (Meson8m2) devices. Say Y if you
>  	  want peripherals and CPU frequency scaling to work.
>  
> +config COMMON_CLK_MESON_MX_SDHC
> +	tristate "Meson MX SDHC MMC Clock Controller Driver"
> +	depends on ARCH_MESON
> +	select COMMON_CLK_MESON_REGMAP
> +	help
> +	  Support for the SDHC clock controller on Amlogic Meson8/8b/8m2 SoCs
> +	  devices. Say Y or M if you want to use the SDHC MMC controller.
> +	  Otherwise say N.
> +
>  config COMMON_CLK_GXBB
>  	bool
>  	depends on ARCH_MESON
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 6eca2a406ee3..b71c7ae78dbd 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
>  obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
> +obj-$(CONFIG_COMMON_CLK_MESON_MX_SDHC) += meson-mx-sdhc.o
> diff --git a/drivers/clk/meson/meson-mx-sdhc.c b/drivers/clk/meson/meson-mx-sdhc.c
> new file mode 100644
> index 000000000000..b98a35d99f65
> --- /dev/null
> +++ b/drivers/clk/meson/meson-mx-sdhc.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic Meson SDHC clock controller
> + *
> + * Copyright (C) 2019 Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> + */
> +
> +#include <dt-bindings/clock/meson-mx-sdhc-clkc.h>
> +
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "clk-regmap.h"
> +#include "clk-pll.h"

If you need the pll clocks, it should probably appear in the Kconfig
deps as well

> +
> +#define MESON_SDHC_CLKC			0x10
> +
> +static const struct clk_regmap meson_mx_sdhc_src_sel = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.mask = 0x3,
> +		.shift = 16,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_src_sel",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .fw_name = "clkin0", .index = -1, },
> +			{ .fw_name = "clkin1", .index = -1, },
> +			{ .fw_name = "clkin2", .index = -1, },
> +			{ .fw_name = "clkin3", .index = -1, },

When fw_name is specified, setting the index is not necessary

> +		},
> +		.num_parents = 4,
> +	},
> +};
> +
> +static const struct clk_div_table meson_mx_sdhc_div_table[] = {
> +	{ .div = 6, .val = 5, },
> +	{ .div = 8, .val = 7, },
> +	{ .div = 9, .val = 8, },
> +	{ .div = 10, .val = 9, },
> +	{ .div = 12, .val = 11, },
> +	{ .div = 16, .val = 15, },
> +	{ .div = 18, .val = 17, },
> +	{ .div = 34, .val = 33, },
> +	{ .div = 142, .val = 141, },
> +	{ .div = 850, .val = 849, },
> +	{ .div = 2126, .val = 2125, },
> +	{ .div = 4096, .val = 4095, },
> +	{ /* sentinel */ }
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_div = {
> +	.data = &(struct clk_regmap_div_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.shift = 0,
> +		.width = 12,
> +		.table = meson_mx_sdhc_div_table,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_div",
> +		.ops = &clk_regmap_divider_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_src_sel", .index = -1, },

Any reason for using the lagacy names and not the clk_hw pointers ?
Same comment for the rest of the clocks

> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_mod_clk_en = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.bit_idx = 15,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_mod_clk_on",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_div", .index = -1, },
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_GATE,

Why can't the clock change rate unless gated ? Maybe you prefer to
change the rate in the mmc while clock is gated, but this is the
handling of the clock by the mmc driver, not a constraint of the actual
clock HW, isn't it ?

Also, this is a gate so I suppose the rate propagates through it ?
Can you explain why CLK_SET_RATE_PARENT is not set  ?

> +	},
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_tx_clk_en = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.bit_idx = 14,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_tx_clk_on",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_div", .index = -1, },
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_GATE,
> +	},
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_rx_clk_en = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.bit_idx = 13,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_rx_clk_on",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_div", .index = -1, },
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,

Ok so apparently you only want to set the rate through the RX clock.
You are free to call set_rate() only on this clock in the mmc driver.
However, I don't think this should reflect as clock constraints.

> +	},
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_sd_clk_en = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.bit_idx = 12,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_sd_clk_on",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_div", .index = -1, },
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,

... now I lost with these flags. I'm sure there is an idea related to
the mmc driver. Clockwise, I don't get it.

> +	},
> +};
> +
> +static const struct clk_regmap *meson_mx_sdhc_clk_regmaps[] = {
> +	[SDHC_CLKID_SRC_SEL]	= &meson_mx_sdhc_src_sel,
> +	[SDHC_CLKID_DIV]	= &meson_mx_sdhc_div,
> +	[SDHC_CLKID_MOD_CLK]	= &meson_mx_sdhc_mod_clk_en,
> +	[SDHC_CLKID_SD_CLK]	= &meson_mx_sdhc_sd_clk_en,
> +	[SDHC_CLKID_TX_CLK]	= &meson_mx_sdhc_tx_clk_en,
> +	[SDHC_CLKID_RX_CLK]	= &meson_mx_sdhc_rx_clk_en,
> +};
> +
> +#define MESON_MX_SDHC_NUM_CLKS ARRAY_SIZE(meson_mx_sdhc_clk_regmaps)
> +
> +static int meson_mx_sdhc_clkc_probe(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct clk_hw_onecell_data *onecell_data;
> +	struct clk_regmap *clk_regmap;
> +	struct regmap *regmap;
> +	int i, ret;
> +
> +	regmap = dev_get_regmap(parent, NULL);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	clk_regmap = devm_kcalloc(parent, sizeof(*clk_regmap),
> +				  MESON_MX_SDHC_NUM_CLKS, GFP_KERNEL);
> +	if (!clk_regmap)
> +		return -ENOMEM;
> +
> +	onecell_data = devm_kzalloc(parent,
> +				    struct_size(onecell_data, hws,
> +						MESON_MX_SDHC_NUM_CLKS),
> +				    GFP_KERNEL);
> +	if (!onecell_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < MESON_MX_SDHC_NUM_CLKS; i++) {
> +		memcpy(&clk_regmap[i], meson_mx_sdhc_clk_regmaps[i],
> +		       sizeof(*clk_regmap));
> +
> +		clk_regmap[i].map = regmap;
> +		onecell_data->hws[i] = &clk_regmap[i].hw;
> +
> +		ret = devm_clk_hw_register(parent, onecell_data->hws[i]);
> +		if (ret) {
> +			dev_err(parent,
> +				"Registration of SDHC clock %d failed\n", i);
> +			return ret;
> +		}
> +	}
> +
> +	onecell_data->num = MESON_MX_SDHC_NUM_CLKS;
> +
> +	return devm_of_clk_add_hw_provider(parent, of_clk_hw_onecell_get,
> +					   onecell_data);
> +}
> +
> +static const struct platform_device_id meson_mx_sdhc_clkc_ids[] = {
> +	{ "meson8-sdhc-clkc" },
> +	{ "meson8b-sdhc-clkc" },
> +	{ "meson8m2-sdhc-clkc" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, meson_mx_sdhc_clkc_ids);
> +
> +static struct platform_driver meson_mx_sdhc_clkc_driver = {
> +	.id_table	= meson_mx_sdhc_clkc_ids,
> +	.probe		= meson_mx_sdhc_clkc_probe,
> +	.driver		= {
> +		.name	= "meson-mx-sdhc-clkc",
> +	},
> +};
> +module_platform_driver(meson_mx_sdhc_clkc_driver);
> +
> +MODULE_DESCRIPTION("Amlogic Meson8/8b/8m2 SDHC clock controller driver");
> +MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");