[PATCH v3 0/2] clk: meson: add a sub EMMC clock controller support

[PATCH v3 0/2] clk: meson: add a sub EMMC clock controller support

[Yixun Lan]

This driver will add a MMC clock controller driver support.
The original idea about adding a clock controller is during the
discussion in the NAND driver mainline effort[1].

I've tested this in the S400 board (AXG platform) by using NAND driver.

Changes since v2 [3]:
 - squash dt-binding clock-id patch
 - update license
 - fix alignment
 - construct a clk register helper() function

Changes since v1 [2]:
 - implement phase clock
 - update compatible name
 - adjust file name
 - divider probe() into small functions, and re-use them

[1] https://lkml.kernel.org/r/20180628090034.0637a062@xps13
[2] https://lkml.kernel.org/r/20180703145716.31860-1-yixun.lan@amlogic.com
[3] https://lkml.kernel.org/r/20180710163658.6175-1-yixun.lan@amlogic.com


Yixun Lan (2):
  clk: meson: add DT documentation for emmc clock controller
  clk: meson: add sub MMC clock controller driver

 .../bindings/clock/amlogic,mmc-clkc.txt       |  31 ++
 drivers/clk/meson/Kconfig                     |   9 +
 drivers/clk/meson/Makefile                    |   1 +
 drivers/clk/meson/mmc-clkc.c                  | 367 ++++++++++++++++++
 .../clock/amlogic,meson-mmc-clkc.h            |  16 +
 5 files changed, 424 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
 create mode 100644 drivers/clk/meson/mmc-clkc.c
 create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h

-- 
2.18.0

[PATCH v3 1/2] clk: meson: add DT documentation for emmc clock controller

[Yixun Lan]

Document the MMC sub clock controller driver, the potential consumer
of this driver is MMC or NAND. Also add three clock bindings IDs which
provided by this driver.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
 .../clock/amlogic,meson-mmc-clkc.h            | 16 ++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
 create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h

diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
new file mode 100644
index 000000000000..91018221df1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
@@ -0,0 +1,31 @@
+* Amlogic MMC Sub Clock Controller Driver
+
+The Amlogic MMC clock controller generates and supplies clock to support
+MMC and NAND controller
+
+Required Properties:
+
+- compatible: should be:
+		"amlogic,meson-gx-mmc-clkc"
+		"amlogic,meson-axg-mmc-clkc"
+
+- #clock-cells: should be 1.
+- clocks: phandles to clocks corresponding to the clock-names property
+- clock-names: list of parent clock names
+	- "clkin0", "clkin1"
+
+Parent node should have the following properties :
+- compatible: "amlogic,meson-axg-mmc-clkc", "syscon".
+- reg: base address and size of the MMC control register space.
+
+Example: Clock controller node:
+
+sd_mmc_c_clkc: clock-controller@7000 {
+	compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
+	reg = <0x0 0x7000 0x0 0x4>;
+	#clock-cells = <1>;
+
+	clock-names = "clkin0", "clkin1";
+	clocks = <&clkc CLKID_SD_MMC_C_CLK0>,
+		 <&clkc CLKID_FCLK_DIV2>;
+};
diff --git a/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h b/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
new file mode 100644
index 000000000000..2ae988ebc3ae
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Meson MMC sub clock tree IDs
+ *
+ * Copyright (c) 2018 Amlogic, Inc. All rights reserved.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#ifndef __MMC_CLKC_H
+#define __MMC_CLKC_H
+
+#define CLKID_MMC_DIV				1
+#define CLKID_MMC_PHASE_TX			3
+#define CLKID_MMC_PHASE_RX			4
+
+#endif
-- 
2.18.0

[PATCH v3 2/2] clk: meson: add sub MMC clock controller driver

[Yixun Lan]

The patch will add a MMC clock controller driver which used by MMC or NAND,
It provide a mux and divider clock, and three phase clocks - core, tx, tx.

Two clocks are provided as the parent of MMC clock controller from
upper layer clock controller - eg "amlogic,axg-clkc" in AXG platform.

To specify which clock the MMC or NAND driver may consume,
the preprocessor macros in the dt-bindings/clock/emmc-clkc.h header
can be used in the device tree sources.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/Kconfig    |   9 +
 drivers/clk/meson/Makefile   |   1 +
 drivers/clk/meson/mmc-clkc.c | 367 +++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/clk/meson/mmc-clkc.c

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index efaa70f682b4..edc18e65c89b 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -15,6 +15,15 @@ config COMMON_CLK_MESON_AO
 	select COMMON_CLK_REGMAP_MESON
 	select RESET_CONTROLLER
 
+config COMMON_CLK_MMC_MESON
+	tristate "Meson MMC Sub Clock Controller Driver"
+	depends on COMMON_CLK_AMLOGIC
+	select MFD_SYSCON
+	select REGMAP
+	help
+	  Support for the MMC sub clock controller on Amlogic Meson Platform,
+	  Say Y if you want this clock enabled.
+
 config COMMON_CLK_REGMAP_MESON
 	bool
 	select REGMAP
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 72ec8c40d848..4b3817f80ba1 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
 obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
 obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
 obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
+obj-$(CONFIG_COMMON_CLK_MMC_MESON) 	+= mmc-clkc.o
 obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
new file mode 100644
index 000000000000..36c4c7cd69a6
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,367 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2017 Baylibre SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/clock/amlogic,meson-mmc-clkc.h>
+
+#include "clkc.h"
+
+/* clock ID used by internal driver */
+#define CLKID_MMC_MUX			0
+#define CLKID_MMC_PHASE_CORE		2
+
+#define SD_EMMC_CLOCK			0
+#define   CLK_DIV_MASK			GENMASK(5, 0)
+#define   CLK_SRC_MASK			GENMASK(7, 6)
+#define   CLK_CORE_PHASE_MASK		GENMASK(9, 8)
+#define   CLK_TX_PHASE_MASK		GENMASK(11, 10)
+#define   CLK_RX_PHASE_MASK		GENMASK(13, 12)
+#define   CLK_V2_TX_DELAY_MASK		GENMASK(19, 16)
+#define   CLK_V2_RX_DELAY_MASK		GENMASK(23, 20)
+#define   CLK_V2_ALWAYS_ON		BIT(24)
+
+#define   CLK_V3_TX_DELAY_MASK		GENMASK(21, 16)
+#define   CLK_V3_RX_DELAY_MASK		GENMASK(27, 22)
+#define   CLK_V3_ALWAYS_ON		BIT(28)
+
+#define   CLK_DELAY_STEP_PS		200
+#define   CLK_PHASE_STEP		30
+#define   CLK_PHASE_POINT_NUM		(360 / CLK_PHASE_STEP)
+
+#define MUX_CLK_NUM_PARENTS		2
+#define MMC_MAX_CLKS			5
+
+struct clk_phase_delay_data {
+	unsigned long			phase_mask;
+	unsigned long			delay_mask;
+	unsigned int			delay_step_ps;
+};
+
+struct mmc_clkc_data {
+	struct clk_phase_delay_data	tx;
+	struct clk_phase_delay_data	rx;
+};
+
+static inline struct clk_phase_delay_data *
+clk_get_regmap_phase_data(struct clk_regmap *clk)
+{
+	return (struct clk_phase_delay_data *)clk->data;
+}
+
+static struct clk_regmap_mux_data mmc_clkc_mux_data = {
+	.offset		= SD_EMMC_CLOCK,
+	.mask		= 0x3,
+	.shift		= 6,
+	.flags		= CLK_DIVIDER_ROUND_CLOSEST,
+};
+
+static struct clk_regmap_div_data mmc_clkc_div_data = {
+	.offset		= SD_EMMC_CLOCK,
+	.shift		= 0,
+	.width		= 6,
+	.flags		= CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
+};
+
+static struct clk_phase_delay_data mmc_clkc_core_delay_phase = {
+	.phase_mask	= CLK_CORE_PHASE_MASK,
+};
+
+static const struct mmc_clkc_data mmc_clkc_gx_data = {
+	{
+		.phase_mask	= CLK_TX_PHASE_MASK,
+		.delay_mask	= CLK_V2_TX_DELAY_MASK,
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+	{
+		.phase_mask	= CLK_RX_PHASE_MASK,
+		.delay_mask	= CLK_V2_RX_DELAY_MASK,
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+};
+
+static const struct mmc_clkc_data mmc_clkc_axg_data = {
+	{
+		.phase_mask	= CLK_TX_PHASE_MASK,
+		.delay_mask	= CLK_V3_TX_DELAY_MASK,
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+	{
+		.phase_mask	= CLK_RX_PHASE_MASK,
+		.delay_mask	= CLK_V3_RX_DELAY_MASK,
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+};
+
+static const struct of_device_id mmc_clkc_match_table[] = {
+	{
+		.compatible	= "amlogic,meson-gx-mmc-clkc",
+		.data		= &mmc_clkc_gx_data
+	},
+	{
+		.compatible	= "amlogic,meson-axg-mmc-clkc",
+		.data		= &mmc_clkc_axg_data
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
+
+static struct clk_regmap *
+mmc_clkc_register_clk(struct device *dev, struct regmap *map,
+		      struct clk_init_data *init,
+		      const char *suffix, void *data)
+{
+	struct clk_regmap *clk;
+	char *name;
+	int ret;
+
+	clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), suffix);
+	if (!name)
+		return ERR_PTR(-ENOMEM);
+
+	init->name = name;
+
+	clk->map = map;
+	clk->data = data;
+	clk->hw.init = init;
+
+	ret = devm_clk_hw_register(dev, &clk->hw);
+	if (ret)
+		clk = ERR_PTR(ret);
+
+	kfree(name);
+	return clk;
+}
+
+static struct clk_regmap *mmc_clkc_register_mux(struct device *dev,
+						struct regmap *map)
+{
+	const char *parent_names[MUX_CLK_NUM_PARENTS];
+	struct clk_init_data init;
+	struct clk_regmap *mux;
+	struct clk *clk;
+	int i;
+
+	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
+		char name[8];
+
+		snprintf(name, sizeof(name), "clkin%d", i);
+		clk = devm_clk_get(dev, name);
+		if (IS_ERR(clk)) {
+			if (clk != ERR_PTR(-EPROBE_DEFER))
+				dev_err(dev, "Missing clock %s\n", name);
+			return ERR_PTR((long)clk);
+		}
+
+		parent_names[i] = __clk_get_name(clk);
+	}
+
+	init.ops = &clk_regmap_mux_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_names = parent_names;
+	init.num_parents = MUX_CLK_NUM_PARENTS;
+
+	mux = mmc_clkc_register_clk(dev, map, &init, "mux", &mmc_clkc_mux_data);
+	if (IS_ERR(mux))
+		dev_err(dev, "Mux clock registration failed\n");
+
+	return mux;
+}
+
+static int clk_regmap_get_phase(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct clk_phase_delay_data *ph = clk_get_regmap_phase_data(clk);
+	unsigned int phase_num = 1 <<  hweight_long(ph->phase_mask);
+	unsigned long period_ps, p, d;
+	int degrees;
+	u32 val;
+
+	regmap_read(clk->map, SD_EMMC_CLOCK, &val);
+	p = (val & ph->phase_mask) >> __ffs(ph->phase_mask);
+	degrees = p * 360 / phase_num;
+
+	if (ph->delay_mask) {
+		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
+					 clk_get_rate(hw->clk));
+		d = (val & ph->delay_mask) >> __ffs(ph->delay_mask);
+		degrees += d * ph->delay_step_ps * 360 / period_ps;
+		degrees %= 360;
+	}
+
+	return degrees;
+}
+
+static void clk_regmap_apply_phase_delay(struct clk_regmap *clk,
+					 unsigned int phase,
+					 unsigned int delay)
+{
+	struct clk_phase_delay_data *ph = clk->data;
+	u32 val;
+
+	regmap_read(clk->map, SD_EMMC_CLOCK, &val);
+
+	val &= ~ph->phase_mask;
+	val |= phase << __ffs(ph->phase_mask);
+
+	if (ph->delay_mask) {
+		val &= ~ph->delay_mask;
+		val |= delay << __ffs(ph->delay_mask);
+	}
+
+	regmap_write(clk->map, SD_EMMC_CLOCK, val);
+}
+
+static int clk_regmap_set_phase(struct clk_hw *hw, int degrees)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct clk_phase_delay_data *ph = clk_get_regmap_phase_data(clk);
+	unsigned int phase_num = 1 <<  hweight_long(ph->phase_mask);
+	unsigned long period_ps, d = 0, r;
+	u64 p;
+
+	p = degrees % 360;
+
+	if (!ph->delay_mask) {
+		p = DIV_ROUND_CLOSEST_ULL(p, 360 / phase_num);
+	} else {
+		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
+					 clk_get_rate(hw->clk));
+
+		/* First compute the phase index (p), the remainder (r) is the
+		 * part we'll try to acheive using the delays (d).
+		 */
+		r = do_div(p, 360 / phase_num);
+		d = DIV_ROUND_CLOSEST(r * period_ps,
+				      360 * ph->delay_step_ps);
+		d = min(d, ph->delay_mask >> __ffs(ph->delay_mask));
+	}
+
+	clk_regmap_apply_phase_delay(clk, p, d);
+	return 0;
+}
+
+static const struct clk_ops clk_regmap_phase_ops = {
+	.get_phase = clk_regmap_get_phase,
+	.set_phase = clk_regmap_set_phase,
+};
+
+static struct clk_regmap *
+mmc_clkc_register_other_clk(struct device *dev, struct regmap *map,
+			    char *suffix, char *parent_suffix,
+			    unsigned long flags,
+			    const struct clk_ops *ops, void *data)
+{
+	struct clk_init_data init;
+	struct clk_regmap *clk;
+	char *parent;
+
+	parent = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_suffix);
+	if (!parent)
+		return ERR_PTR(-ENOMEM);
+
+	init.ops = ops;
+	init.flags = flags;
+	init.parent_names = (const char* const []){ parent, };
+	init.num_parents = 1;
+
+	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
+	if (IS_ERR(clk))
+		dev_err(dev, "Core %s clock registration failed\n", suffix);
+
+	kfree(parent);
+	return clk;
+}
+
+static int mmc_clkc_probe(struct platform_device *pdev)
+{
+	struct clk_hw_onecell_data *onecell_data;
+	struct device *dev = &pdev->dev;
+	struct mmc_clkc_data *data;
+	struct regmap *map;
+	struct clk_regmap *mux, *div, *core, *rx, *tx;
+
+	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
+	map = syscon_node_to_regmap(dev->of_node);
+	if (IS_ERR(map)) {
+		dev_err(dev, "could not find mmc clock controller\n");
+		return PTR_ERR(map);
+	}
+
+	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
+				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
+				    GFP_KERNEL);
+	if (!onecell_data)
+		return -ENOMEM;
+
+	mux = mmc_clkc_register_mux(dev, map);
+	if (IS_ERR(mux))
+		return PTR_ERR(mux);
+
+	div = mmc_clkc_register_other_clk(dev, map, "div", "mux",
+					  CLK_SET_RATE_PARENT,
+					  &clk_regmap_divider_ops,
+					  &mmc_clkc_div_data);
+	if (IS_ERR(div))
+		return PTR_ERR(div);
+
+	core = mmc_clkc_register_other_clk(dev, map, "core", "div",
+					   CLK_SET_RATE_PARENT,
+					   &clk_regmap_phase_ops,
+					   &mmc_clkc_core_delay_phase);
+	if (IS_ERR(core))
+		return PTR_ERR(core);
+
+	rx = mmc_clkc_register_other_clk(dev, map, "rx", "core", 0,
+					 &clk_regmap_phase_ops,
+					 &data->rx);
+	if (IS_ERR(rx))
+		return PTR_ERR(rx);
+
+	tx = mmc_clkc_register_other_clk(dev, map, "tx", "core", 0,
+					 &clk_regmap_phase_ops,
+					 &data->tx);
+	if (IS_ERR(tx))
+		return PTR_ERR(tx);
+
+	onecell_data->hws[CLKID_MMC_MUX]		= &mux->hw,
+	onecell_data->hws[CLKID_MMC_DIV]		= &div->hw,
+	onecell_data->hws[CLKID_MMC_PHASE_CORE]		= &core->hw,
+	onecell_data->hws[CLKID_MMC_PHASE_RX]		= &rx->hw,
+	onecell_data->hws[CLKID_MMC_PHASE_TX]		= &tx->hw,
+	onecell_data->num				= MMC_MAX_CLKS;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   onecell_data);
+}
+
+static struct platform_driver mmc_clkc_driver = {
+	.probe		= mmc_clkc_probe,
+	.driver		= {
+		.name	= "meson-mmc-clkc",
+		.of_match_table = of_match_ptr(mmc_clkc_match_table),
+	},
+};
+
+module_platform_driver(mmc_clkc_driver);
-- 
2.18.0

Re: [PATCH v3 1/2] clk: meson: add DT documentation for emmc clock controller

[Rob Herring]

On Thu, Jul 12, 2018 at 09:12:43PM +0000, Yixun Lan wrote:
> Document the MMC sub clock controller driver, the potential consumer
> of this driver is MMC or NAND. Also add three clock bindings IDs which
> provided by this driver.
> 
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
>  .../clock/amlogic,meson-mmc-clkc.h            | 16 ++++++++++
>  2 files changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>  create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/2] clk: meson: add sub MMC clock controller driver

[Stephen Boyd]

Quoting Yixun Lan (2018-07-12 14:12:44)
> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> new file mode 100644
> index 000000000000..36c4c7cd69a6
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,367 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/clk.h>

Is this include used?

> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/amlogic,meson-mmc-clkc.h>
> +
> +#include "clkc.h"
> +
> +
> +static struct clk_regmap *
> +mmc_clkc_register_other_clk(struct device *dev, struct regmap *map,
> +                           char *suffix, char *parent_suffix,
> +                           unsigned long flags,
> +                           const struct clk_ops *ops, void *data)
> +{
> +       struct clk_init_data init;
> +       struct clk_regmap *clk;
> +       char *parent;
> +
> +       parent = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_suffix);
> +       if (!parent)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.ops = ops;
> +       init.flags = flags;
> +       init.parent_names = (const char* const []){ parent, };

Can't this just be &parent?

> +       init.num_parents = 1;
> +
> +       clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
> +       if (IS_ERR(clk))
> +               dev_err(dev, "Core %s clock registration failed\n", suffix);
> +
> +       kfree(parent);
> +       return clk;
> +}
> +
> +static int mmc_clkc_probe(struct platform_device *pdev)
> +{
> +       struct clk_hw_onecell_data *onecell_data;
> +       struct device *dev = &pdev->dev;
> +       struct mmc_clkc_data *data;
> +       struct regmap *map;
> +       struct clk_regmap *mux, *div, *core, *rx, *tx;
> +
> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);

This cast is unnecessary. Pleas remove.

Re: [PATCH v3 1/2] clk: meson: add DT documentation for emmc clock controller

[Yixun Lan]

HI Rob

On 07/25/2018 07:29 AM, Rob Herring wrote:
> On Thu, Jul 12, 2018 at 09:12:43PM +0000, Yixun Lan wrote:
>> Document the MMC sub clock controller driver, the potential consumer
>> of this driver is MMC or NAND. Also add three clock bindings IDs which
>> provided by this driver.
>>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
>>  .../clock/amlogic,meson-mmc-clkc.h            | 16 ++++++++++
>>  2 files changed, 47 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>  create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> 

Thanks for the review!

There is a discussion about dropping "meson-" prefix in the compatible
string[1]

So, I will send another version with the new compatible name adjusted
from "amlogic,meson-axg-mmc-clkc" to "amlogic,axg-mmc-clkc"..
probably also rename the dt-bindings header file to amlogc,mmc-clkc.h

[1]  https://lkml.kernel.org/r/7hk1prmg4w.fsf@baylibre.com


Yixun

Re: [PATCH v3 2/2] clk: meson: add sub MMC clock controller driver

[Yixun Lan]

HI Stephen:

On 07/26/2018 11:20 PM, Stephen Boyd wrote:
> Quoting Yixun Lan (2018-07-12 14:12:44)
>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 000000000000..36c4c7cd69a6
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,367 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk.h>
> 
> Is this include used?
> 
this is needed by clk_get_rate()
see drivers/clk/meson/mmc-clkc.c:204

>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <dt-bindings/clock/amlogic,meson-mmc-clkc.h>
>> +
>> +#include "clkc.h"
>> +
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_other_clk(struct device *dev, struct regmap *map,
>> +                           char *suffix, char *parent_suffix,
>> +                           unsigned long flags,
>> +                           const struct clk_ops *ops, void *data)
>> +{
>> +       struct clk_init_data init;
>> +       struct clk_regmap *clk;
>> +       char *parent;
>> +
>> +       parent = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_suffix);
>> +       if (!parent)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       init.ops = ops;
>> +       init.flags = flags;
>> +       init.parent_names = (const char* const []){ parent, };
> 
> Can't this just be &parent?
sure, I can fix this

> 
>> +       init.num_parents = 1;
>> +
>> +       clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
>> +       if (IS_ERR(clk))
>> +               dev_err(dev, "Core %s clock registration failed\n", suffix);
>> +
>> +       kfree(parent);
>> +       return clk;
>> +}
>> +
>> +static int mmc_clkc_probe(struct platform_device *pdev)
>> +{
>> +       struct clk_hw_onecell_data *onecell_data;
>> +       struct device *dev = &pdev->dev;
>> +       struct mmc_clkc_data *data;
>> +       struct regmap *map;
>> +       struct clk_regmap *mux, *div, *core, *rx, *tx;
>> +
>> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> 
> This cast is unnecessary. Pleas remove.
> 
Ok, I will try to fix in next version


this was trying to silence the ’const‘ cast warning [1]
I could make a 'const struct mmc_clkc_data *data' declare, but need to
fix further cast warning issue..


[1] drivers/clk/meson/mmc-clkc.c: In function ‘mmc_clkc_probe’:
drivers/clk/meson/mmc-clkc.c:302:7: warning: assignment discards ‘const’
qualifier from pointer target type [-Wdiscarded-qualifiers]
  data = of_device_get_match_data(dev);
       ^

Re: [PATCH v3 2/2] clk: meson: add sub MMC clock controller driver

[Stephen Boyd]

Quoting Yixun Lan (2018-07-27 07:52:23)
> HI Stephen:
> 
> On 07/26/2018 11:20 PM, Stephen Boyd wrote:
> > Quoting Yixun Lan (2018-07-12 14:12:44)
> >> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> >> new file mode 100644
> >> index 000000000000..36c4c7cd69a6
> >> --- /dev/null
> >> +++ b/drivers/clk/meson/mmc-clkc.c
> >> @@ -0,0 +1,367 @@
> >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> +/*
> >> + * Amlogic Meson MMC Sub Clock Controller Driver
> >> + *
> >> + * Copyright (c) 2017 Baylibre SAS.
> >> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> >> + *
> >> + * Copyright (c) 2018 Amlogic, inc.
> >> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> >> + */
> >> +
> >> +#include <linux/clk.h>
> > 
> > Is this include used?
> > 
> this is needed by clk_get_rate()
> see drivers/clk/meson/mmc-clkc.c:204

Hmm ok. That's unfortunate.

> 
> > 
> >> +       init.num_parents = 1;
> >> +
> >> +       clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
> >> +       if (IS_ERR(clk))
> >> +               dev_err(dev, "Core %s clock registration failed\n", suffix);
> >> +
> >> +       kfree(parent);
> >> +       return clk;
> >> +}
> >> +
> >> +static int mmc_clkc_probe(struct platform_device *pdev)
> >> +{
> >> +       struct clk_hw_onecell_data *onecell_data;
> >> +       struct device *dev = &pdev->dev;
> >> +       struct mmc_clkc_data *data;
> >> +       struct regmap *map;
> >> +       struct clk_regmap *mux, *div, *core, *rx, *tx;
> >> +
> >> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> > 
> > This cast is unnecessary. Pleas remove.
> > 
> Ok, I will try to fix in next version
> 
> 
> this was trying to silence the ’const‘ cast warning [1]
> I could make a 'const struct mmc_clkc_data *data' declare, but need to
> fix further cast warning issue..
> 
> 
> [1] drivers/clk/meson/mmc-clkc.c: In function ‘mmc_clkc_probe’:
> drivers/clk/meson/mmc-clkc.c:302:7: warning: assignment discards ‘const’
> qualifier from pointer target type [-Wdiscarded-qualifiers]
>   data = of_device_get_match_data(dev);
>        ^

Yes. Casting away const is a bad idea.

Re: [PATCH v3 2/2] clk: meson: add sub MMC clock controller driver

[Stephen Boyd]

Quoting Stephen Boyd (2018-07-27 09:41:40)
> Quoting Yixun Lan (2018-07-27 07:52:23)
> > HI Stephen:
> > 
> > On 07/26/2018 11:20 PM, Stephen Boyd wrote:
> > > Quoting Yixun Lan (2018-07-12 14:12:44)
> > >> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> > >> new file mode 100644
> > >> index 000000000000..36c4c7cd69a6
> > >> --- /dev/null
> > >> +++ b/drivers/clk/meson/mmc-clkc.c
> > >> @@ -0,0 +1,367 @@
> > >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > >> +/*
> > >> + * Amlogic Meson MMC Sub Clock Controller Driver
> > >> + *
> > >> + * Copyright (c) 2017 Baylibre SAS.
> > >> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> > >> + *
> > >> + * Copyright (c) 2018 Amlogic, inc.
> > >> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> > >> + */
> > >> +
> > >> +#include <linux/clk.h>
> > > 
> > > Is this include used?
> > > 
> > this is needed by clk_get_rate()
> > see drivers/clk/meson/mmc-clkc.c:204
> 
> Hmm ok. That's unfortunate.

You should be able to read the hardware to figure out the clk frequency?
This may be a sign that the phase clk_ops are bad and should be passing
in the frequency of the parent clk to the op so that phase can be
calculated. Jerome?

Re: [PATCH v3 2/2] clk: meson: add sub MMC clock controller driver

[Jerome Brunet]

On Fri, 2018-07-27 at 09:45 -0700, Stephen Boyd wrote:
> Quoting Stephen Boyd (2018-07-27 09:41:40)
> > Quoting Yixun Lan (2018-07-27 07:52:23)
> > > HI Stephen:
> > > 
> > > On 07/26/2018 11:20 PM, Stephen Boyd wrote:
> > > > Quoting Yixun Lan (2018-07-12 14:12:44)
> > > > > diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> > > > > new file mode 100644
> > > > > index 000000000000..36c4c7cd69a6
> > > > > --- /dev/null
> > > > > +++ b/drivers/clk/meson/mmc-clkc.c
> > > > > @@ -0,0 +1,367 @@
> > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > +/*
> > > > > + * Amlogic Meson MMC Sub Clock Controller Driver
> > > > > + *
> > > > > + * Copyright (c) 2017 Baylibre SAS.
> > > > > + * Author: Jerome Brunet <jbrunet@baylibre.com>
> > > > > + *
> > > > > + * Copyright (c) 2018 Amlogic, inc.
> > > > > + * Author: Yixun Lan <yixun.lan@amlogic.com>
> > > > > + */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > 
> > > > Is this include used?
> > > > 
> > > 
> > > this is needed by clk_get_rate()
> > > see drivers/clk/meson/mmc-clkc.c:204
> > 
> > Hmm ok. That's unfortunate.
> 
> You should be able to read the hardware to figure out the clk frequency?
> This may be a sign that the phase clk_ops are bad and should be passing
> in the frequency of the parent clk to the op so that phase can be
> calculated. Jerome?
> 

It could be a away to do it but:
a) if we modify the API, we would need to update every clock driver using it.
   There is not that many users of the phase API but still, it is annoying
b) This particular driver need the parent rate, other might need something else
I guess. (parent phase ??, duty cycle ??) 

I think the real problem here it that you are using the consumer API. You should
be using the provider API like clk_hw_get_rate. Look at the clk-divider.c which
use clk_hw_round_rate() on the parent clock. 

Clock drivers should deal with 'struct clk_hw', not 'struct clk'. I think it was
mentioned in the past that the 'clk' within 'struct clk_hw' might be removed
someday.

Yixun, please don't put your clock driver within the controller driver. Please
implement your 'phase-delay' clock in its own file and export the ops, like
every other clock in the amlogic directory. Also, please review your list of
'#define', some of them are unnecessary copy/paste from the MMC driver.

Regards

Jerome

Re: [PATCH v3 1/2] clk: meson: add DT documentation for emmc clock controller

[Rob Herring]

On Fri, Jul 27, 2018 at 8:14 AM Yixun Lan <yixun.lan@amlogic.com> wrote:
>
>
> HI Rob
>
> On 07/25/2018 07:29 AM, Rob Herring wrote:
> > On Thu, Jul 12, 2018 at 09:12:43PM +0000, Yixun Lan wrote:
> >> Document the MMC sub clock controller driver, the potential consumer
> >> of this driver is MMC or NAND. Also add three clock bindings IDs which
> >> provided by this driver.
> >>
> >> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> >> ---
> >>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
> >>  .../clock/amlogic,meson-mmc-clkc.h            | 16 ++++++++++
> >>  2 files changed, 47 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >>  create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> >
>
> Thanks for the review!
>
> There is a discussion about dropping "meson-" prefix in the compatible
> string[1]
>
> So, I will send another version with the new compatible name adjusted
> from "amlogic,meson-axg-mmc-clkc" to "amlogic,axg-mmc-clkc"..
> probably also rename the dt-bindings header file to amlogc,mmc-clkc.h

Okay, you can keep my R-by.

Re: [PATCH v3 2/2] clk: meson: add sub MMC clock controller driver

[Yixun Lan]

Hi Jerome

On 07/30/18 16:57, Jerome Brunet wrote:
> On Fri, 2018-07-27 at 09:45 -0700, Stephen Boyd wrote:
>> Quoting Stephen Boyd (2018-07-27 09:41:40)
>>> Quoting Yixun Lan (2018-07-27 07:52:23)
>>>> HI Stephen:
>>>>
>>>> On 07/26/2018 11:20 PM, Stephen Boyd wrote:
>>>>> Quoting Yixun Lan (2018-07-12 14:12:44)
>>>>>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..36c4c7cd69a6
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/clk/meson/mmc-clkc.c
>>>>>> @@ -0,0 +1,367 @@
>>>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>>>> +/*
>>>>>> + * Amlogic Meson MMC Sub Clock Controller Driver
>>>>>> + *
>>>>>> + * Copyright (c) 2017 Baylibre SAS.
>>>>>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>>>>>> + *
>>>>>> + * Copyright (c) 2018 Amlogic, inc.
>>>>>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/clk.h>
>>>>>
>>>>> Is this include used?
>>>>>
>>>>
>>>> this is needed by clk_get_rate()
>>>> see drivers/clk/meson/mmc-clkc.c:204
>>>
>>> Hmm ok. That's unfortunate.
>>
>> You should be able to read the hardware to figure out the clk frequency?
>> This may be a sign that the phase clk_ops are bad and should be passing
>> in the frequency of the parent clk to the op so that phase can be
>> calculated. Jerome?
>>
> 
> It could be a away to do it but:
> a) if we modify the API, we would need to update every clock driver using it.
>    There is not that many users of the phase API but still, it is annoying
> b) This particular driver need the parent rate, other might need something else
> I guess. (parent phase ??, duty cycle ??) 
> 
> I think the real problem here it that you are using the consumer API. You should
> be using the provider API like clk_hw_get_rate. Look at the clk-divider.c which
> use clk_hw_round_rate() on the parent clock. 
I will replace it with clk_hw_get_rate()

> 
> Clock drivers should deal with 'struct clk_hw', not 'struct clk'. I think it was
> mentioned in the past that the 'clk' within 'struct clk_hw' might be removed
> someday.
> 
> Yixun, please don't put your clock driver within the controller driver. Please
> implement your 'phase-delay' clock in its own file and export the ops, like
> every other clock in the amlogic directory. Also, please review your list of
> '#define', some of them are unnecessary copy/paste from the MMC driver.
> 
will implement a clk-phase-delay.c

I can move the extra CC list

Yixun