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 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/[email protected]
Yixun Lan (3):
clk: meson: add DT documentation for emmc clock controller
clk: meson: add sub MMC clock dt-bindings IDs
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 | 419 ++++++++++++++++++
.../clock/amlogic,meson-mmc-clkc.h | 16 +
5 files changed, 476 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
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 <[email protected]>
---
drivers/clk/meson/Kconfig | 9 +
drivers/clk/meson/Makefile | 1 +
drivers/clk/meson/mmc-clkc.c | 419 +++++++++++++++++++++++++++++++++++
3 files changed, 429 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..43b7a376746d
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,419 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <[email protected]>
+ */
+
+#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_regmap_phase_data {
+ unsigned long phase_mask;
+ unsigned long delay_mask;
+ unsigned int delay_step_ps;
+};
+
+struct mmc_clkc_data {
+ struct clk_regmap_phase_data tx;
+ struct clk_regmap_phase_data rx;
+};
+
+struct mmc_clkc_info {
+ struct device *dev;
+ struct regmap *map;
+ struct mmc_clkc_data *data;
+};
+
+static inline struct clk_regmap_phase_data *
+clk_get_regmap_phase_data(struct clk_regmap *clk)
+{
+ return (struct clk_regmap_phase_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_regmap_phase_data mmc_clkc_core_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_mux(struct mmc_clkc_info *clkc)
+{
+ const char *parent_names[MUX_CLK_NUM_PARENTS];
+ struct device *dev = clkc->dev;
+ struct clk_init_data init;
+ struct clk_regmap *mux;
+ struct clk *clk;
+ char *mux_name;
+ int i, ret;
+
+ mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ return ERR_PTR(-ENOMEM);
+
+ 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);
+ }
+
+ mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
+ if (!mux_name)
+ return ERR_PTR(-ENOMEM);
+
+ mux->map = clkc->map;
+ mux->data = &mmc_clkc_mux_data;
+
+ init.name = mux_name;
+ 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->hw.init = &init;
+ ret = devm_clk_hw_register(dev, &mux->hw);
+ if (ret) {
+ dev_err(dev, "Mux clock registration failed\n");
+ mux = ERR_PTR(ret);
+ }
+
+ kfree(mux_name);
+ return mux;
+}
+
+static struct clk_regmap *mmc_clkc_register_div(struct mmc_clkc_info *clkc)
+{
+ const char *parent_names[MUX_CLK_NUM_PARENTS];
+ struct device *dev = clkc->dev;
+ struct clk_init_data init;
+ struct clk_regmap *div;
+ char *mux_name, *div_name;
+ int ret;
+
+ div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+ if (!div)
+ return ERR_PTR(-ENOMEM);
+
+ mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
+ div_name = kasprintf(GFP_KERNEL, "%s#div", dev_name(dev));
+ if (!mux_name || !div_name) {
+ div = ERR_PTR(-ENOMEM);
+ goto err;
+ }
+
+ parent_names[0] = mux_name;
+ div->map = clkc->map;
+ div->data = &mmc_clkc_div_data;
+
+ init.name = div_name;
+ init.ops = &clk_regmap_divider_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ init.parent_names = parent_names;
+ init.num_parents = 1;
+
+ div->hw.init = &init;
+ ret = devm_clk_hw_register(dev, &div->hw);
+ if (ret) {
+ dev_err(dev, "Divider clock registration failed\n");
+ div = ERR_PTR(ret);
+ }
+
+err:
+ kfree(mux_name);
+ kfree(div_name);
+ return div;
+}
+
+static int clk_regmap_get_phase(struct clk_hw *hw)
+{
+ struct clk_regmap *clk = to_clk_regmap(hw);
+ struct clk_regmap_phase_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_regmap_phase_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_regmap_phase_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_phase_clk(struct mmc_clkc_info *clkc,
+ char *name, char *parent_name, unsigned long flags,
+ struct clk_regmap_phase_data *phase_data)
+
+{
+ const char *parent_names[MUX_CLK_NUM_PARENTS];
+ struct device *dev = clkc->dev;
+ struct clk_init_data init;
+ struct clk_regmap *x;
+ char *clk_name, *core_name;
+ int ret;
+
+ /* create the mmc rx clock */
+ x = devm_kzalloc(dev, sizeof(*x), GFP_KERNEL);
+ if (!x)
+ return ERR_PTR(-ENOMEM);
+
+ clk_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), name);
+ core_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_name);
+
+ if (!clk_name || !core_name) {
+ x = ERR_PTR(-ENOMEM);
+ goto err;
+ }
+ parent_names[0] = core_name;
+
+ init.name = clk_name;
+ init.ops = &clk_regmap_phase_ops;
+ init.flags = flags;
+ init.parent_names = parent_names;
+ init.num_parents = 1;
+
+ x->map = clkc->map;
+ x->data = phase_data;
+ x->hw.init = &init;
+
+ ret = devm_clk_hw_register(dev, &x->hw);
+ if (ret) {
+ dev_err(dev, "Core %s clock registration failed\n", name);
+ x = ERR_PTR(ret);
+ }
+err:
+ kfree(clk_name);
+ kfree(core_name);
+ return x;
+}
+
+static int mmc_clkc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mmc_clkc_info *clkc;
+ struct clk_regmap *mux, *div, *core, *rx, *tx;
+ struct clk_hw_onecell_data *onecell_data;
+
+ clkc = devm_kzalloc(dev, sizeof(*clkc), GFP_KERNEL);
+ if (!clkc)
+ return -ENOMEM;
+
+ clkc->data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
+ if (!clkc->data)
+ return -EINVAL;
+
+ clkc->dev = dev;
+ clkc->map = syscon_node_to_regmap(dev->of_node);
+
+ if (IS_ERR(clkc->map)) {
+ dev_err(dev, "could not find mmc clock controller\n");
+ return PTR_ERR(clkc->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(clkc);
+ if (IS_ERR(mux))
+ return PTR_ERR(mux);
+
+ div = mmc_clkc_register_div(clkc);
+ if (IS_ERR(div))
+ return PTR_ERR(div);
+
+ core = mmc_clkc_register_phase_clk(clkc, "core", "div",
+ CLK_SET_RATE_PARENT,
+ &mmc_clkc_core_phase);
+ if (IS_ERR(core))
+ return PTR_ERR(core);
+
+ rx = mmc_clkc_register_phase_clk(clkc, "rx", "core", 0,
+ &clkc->data->rx);
+ if (IS_ERR(rx))
+ return PTR_ERR(rx);
+
+ tx = mmc_clkc_register_phase_clk(clkc, "tx", "core", 0,
+ &clkc->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
Document the MMC sub clock controller driver, the potential consumer
of this driver is MMC or NAND.
Signed-off-by: Yixun Lan <[email protected]>
---
.../bindings/clock/amlogic,mmc-clkc.txt | 31 +++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
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..ff6b4bf3ecf9
--- /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: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
+- reg: base address and size of the MMC control register space.
+
+Example: Clock controller node:
+
+sd_mmc_c_clkc: clock-controller@7000 {
+ compatible = "amlogic,mmc-clkc", "syscon", "simple-mfd";
+ reg = <0x0 0x7000 0x0 0x4>;
+ #clock-cells = <1>;
+
+ clock-names = "clkin0", "clkin1";
+ clocks = <&clkc CLKID_SD_MMC_C_CLK0>,
+ <&clkc CLKID_FCLK_DIV2>;
+};
--
2.18.0
Add two clock bindings IDs which provided by the MMC clock controller,
These two clocks will be used by MMC or NAND driver.
Signed-off-by: Yixun Lan <[email protected]>
---
.../dt-bindings/clock/amlogic,meson-mmc-clkc.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
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 <[email protected]>
+ */
+
+#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
On Tue, Jul 10, 2018 at 04:36:57PM +0000, Yixun Lan wrote:
> Add two clock bindings IDs which provided by the MMC clock controller,
> These two clocks will be used by MMC or NAND driver.
I count 3 ids.
>
> Signed-off-by: Yixun Lan <[email protected]>
> ---
> .../dt-bindings/clock/amlogic,meson-mmc-clkc.h | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
This can go with the binding patch.
>
> 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 <[email protected]>
> + */
> +
> +#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
>
On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
> Document the MMC sub clock controller driver, the potential consumer
> of this driver is MMC or NAND.
So you all have decided to properly model this now?
>
> Signed-off-by: Yixun Lan <[email protected]>
> ---
> .../bindings/clock/amlogic,mmc-clkc.txt | 31 +++++++++++++++++++
> 1 file changed, 31 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>
> 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..ff6b4bf3ecf9
> --- /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: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
You don't need "simple-mfd" and probably not syscon either. The order is
wrong too. Most specific first.
> +- reg: base address and size of the MMC control register space.
> +
> +Example: Clock controller node:
> +
> +sd_mmc_c_clkc: clock-controller@7000 {
> + compatible = "amlogic,mmc-clkc", "syscon", "simple-mfd";
Doesn't match the binding...
> + reg = <0x0 0x7000 0x0 0x4>;
> + #clock-cells = <1>;
> +
> + clock-names = "clkin0", "clkin1";
> + clocks = <&clkc CLKID_SD_MMC_C_CLK0>,
> + <&clkc CLKID_FCLK_DIV2>;
> +};
> --
> 2.18.0
>
Hi Rob
On 07/12/18 03:45, Rob Herring wrote:
> On Tue, Jul 10, 2018 at 04:36:57PM +0000, Yixun Lan wrote:
>> Add two clock bindings IDs which provided by the MMC clock controller,
>> These two clocks will be used by MMC or NAND driver.
>
> I count 3 ids.
I will update this
>
>>
>> Signed-off-by: Yixun Lan <[email protected]>
>> ---
>> .../dt-bindings/clock/amlogic,meson-mmc-clkc.h | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>> create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
>
> This can go with the binding patch.
>
sure
.
Hi Rob
see my comments
On 07/12/18 03:43, Rob Herring wrote:
> On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
>> Document the MMC sub clock controller driver, the potential consumer
>> of this driver is MMC or NAND.
>
> So you all have decided to properly model this now?
>
Yes, ;-)
>>
>> Signed-off-by: Yixun Lan <[email protected]>
>> ---
>> .../bindings/clock/amlogic,mmc-clkc.txt | 31 +++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>
>> 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..ff6b4bf3ecf9
>> --- /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: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
>
> You don't need "simple-mfd" and probably not syscon either. The order is
> wrong too. Most specific first.
>
Ok, I will drop "simple-mfd"..
but the syscon is a must, since this mmc clock model access registers
via the regmap interface
I will fix the order, thanks for pointing this out
>> +- reg: base address and size of the MMC control register space.
>> +
>> +Example: Clock controller node:
>> +
>> +sd_mmc_c_clkc: clock-controller@7000 {
>> + compatible = "amlogic,mmc-clkc", "syscon", "simple-mfd";
>
> Doesn't match the binding...
>
oops, I will update this
>> + reg = <0x0 0x7000 0x0 0x4>;
>> + #clock-cells = <1>;
>> +
>> + clock-names = "clkin0", "clkin1";
>> + clocks = <&clkc CLKID_SD_MMC_C_CLK0>,
>> + <&clkc CLKID_FCLK_DIV2>;
>> +};
>> --
>> 2.18.0
>>
>
> .
>
On Tue, 2018-07-10 at 16:36 +0000, Yixun Lan wrote:
> 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 <[email protected]>
> ---
> drivers/clk/meson/Kconfig | 9 +
> drivers/clk/meson/Makefile | 1 +
> drivers/clk/meson/mmc-clkc.c | 419 +++++++++++++++++++++++++++++++++++
> 3 files changed, 429 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..43b7a376746d
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
* Copyright (c) 2017 Baylibre SAS.
* Author: Jerome Brunet <[email protected]>
Considering that a fair share of the code below has been copied from the clock
portion of the eMMC driver, which I wrote last year.
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <[email protected]>
> + */
> +
> +#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
Some defines are aligned, some aren't. please be consistent about it.
I personally prefer when things are aligned but it is just a preference.
> +
> +struct clk_regmap_phase_data {
Considering the recent addition of clk_phase in clk/meson, it is not the best
name to choose.
clk_phase_delay_data ?
> + unsigned long phase_mask;
> + unsigned long delay_mask;
> + unsigned int delay_step_ps;
> +};
> +
> +struct mmc_clkc_data {
> + struct clk_regmap_phase_data tx;
> + struct clk_regmap_phase_data rx;
> +};
> +
> +struct mmc_clkc_info {
> + struct device *dev;
I don't think you need keep dev here, considering this is the device data.
> + struct regmap *map;
> + struct mmc_clkc_data *data;
> +};
Overall, I don't think you need this structure
> +
> +static inline struct clk_regmap_phase_data *
> +clk_get_regmap_phase_data(struct clk_regmap *clk)
Update name as well
> +{
> + return (struct clk_regmap_phase_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,
> +};
Same comment on alignement. Some structure you've aligned, and some you haven't
Consistency ...
> +
> +static struct clk_regmap_phase_data mmc_clkc_core_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_mux(struct mmc_clkc_info *clkc)
> +{
> + const char *parent_names[MUX_CLK_NUM_PARENTS];
> + struct device *dev = clkc->dev;
> + struct clk_init_data init;
> + struct clk_regmap *mux;
> + struct clk *clk;
> + char *mux_name;
> + int i, ret;
> +
> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return ERR_PTR(-ENOMEM);
> +
> + 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);
> + }
> +
> + mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
> + if (!mux_name)
> + return ERR_PTR(-ENOMEM);
> +
> + mux->map = clkc->map;
> + mux->data = &mmc_clkc_mux_data;
> +
> + init.name = mux_name;
> + 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->hw.init = &init;
> + ret = devm_clk_hw_register(dev, &mux->hw);
> + if (ret) {
> + dev_err(dev, "Mux clock registration failed\n");
> + mux = ERR_PTR(ret);
> + }
> +
> + kfree(mux_name);
> + return mux;
> +}
> +
> +static struct clk_regmap *mmc_clkc_register_div(struct mmc_clkc_info *clkc)
> +{
> + const char *parent_names[MUX_CLK_NUM_PARENTS];
> + struct device *dev = clkc->dev;
> + struct clk_init_data init;
> + struct clk_regmap *div;
> + char *mux_name, *div_name;
> + int ret;
> +
> + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
> + if (!div)
> + return ERR_PTR(-ENOMEM);
> +
> + mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
> + div_name = kasprintf(GFP_KERNEL, "%s#div", dev_name(dev));
> + if (!mux_name || !div_name) {
> + div = ERR_PTR(-ENOMEM);
> + goto err;
> + }
> +
> + parent_names[0] = mux_name;
> + div->map = clkc->map;
> + div->data = &mmc_clkc_div_data;
> +
> + init.name = div_name;
> + init.ops = &clk_regmap_divider_ops;
> + init.flags = CLK_SET_RATE_PARENT;
> + init.parent_names = parent_names;
> + init.num_parents = 1;
> +
> + div->hw.init = &init;
> + ret = devm_clk_hw_register(dev, &div->hw);
> + if (ret) {
> + dev_err(dev, "Divider clock registration failed\n");
> + div = ERR_PTR(ret);
> + }
> +
> +err:
> + kfree(mux_name);
> + kfree(div_name);
> + return div;
> +}
> +
> +static int clk_regmap_get_phase(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct clk_regmap_phase_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_regmap_phase_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_regmap_phase_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_phase_clk(struct mmc_clkc_info *clkc,
> + char *name, char *parent_name, unsigned long flags,
> + struct clk_regmap_phase_data *phase_data)
> +
> +{
> + const char *parent_names[MUX_CLK_NUM_PARENTS];
> + struct device *dev = clkc->dev;
> + struct clk_init_data init;
> + struct clk_regmap *x;
x is not a very good name, 'clk' is just 2 more letters ...
> + char *clk_name, *core_name;
> + int ret;
> +
> + /* create the mmc rx clock */
> + x = devm_kzalloc(dev, sizeof(*x), GFP_KERNEL);
> + if (!x)
> + return ERR_PTR(-ENOMEM);
> +
> + clk_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), name);
> + core_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_name);
> +
> + if (!clk_name || !core_name) {
> + x = ERR_PTR(-ENOMEM);
> + goto err;
> + }
> + parent_names[0] = core_name;
> +
> + init.name = clk_name;
> + init.ops = &clk_regmap_phase_ops;
> + init.flags = flags;
> + init.parent_names = parent_names;
> + init.num_parents = 1;
> +
> + x->map = clkc->map;
> + x->data = phase_data;
> + x->hw.init = &init;
> +
> + ret = devm_clk_hw_register(dev, &x->hw);
> + if (ret) {
> + dev_err(dev, "Core %s clock registration failed\n", name);
> + x = ERR_PTR(ret);
> + }
> +err:
> + kfree(clk_name);
> + kfree(core_name);
> + return x;
> +}
In all your mmc_clkc_register_xxx_() you should a pattern repeating itself. It
is something I pointed out in the previous version of your patchset
1. allocate a clk_regmap
2. allocate a clock name using dev name and a suffix.
3. register clk_hw
4. free clock name
5. return clk_regmap
You should be able to make an helper function out of this
Prototype will probably look like this:
static struct clk_regmap *
mmc_clkc_register_clk(struct device *dev, struct regmap *map,
struct clk_init_data *init, const char* suffix)
> +
> +static int mmc_clkc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mmc_clkc_info *clkc;
> + struct clk_regmap *mux, *div, *core, *rx, *tx;
> + struct clk_hw_onecell_data *onecell_data;
> +
> + clkc = devm_kzalloc(dev, sizeof(*clkc), GFP_KERNEL);
> + if (!clkc)
> + return -ENOMEM;
> +
> + clkc->data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> + if (!clkc->data)
> + return -EINVAL;
Prefer ENODEV for this
> +
> + clkc->dev = dev;
> + clkc->map = syscon_node_to_regmap(dev->of_node);
> +
> + if (IS_ERR(clkc->map)) {
> + dev_err(dev, "could not find mmc clock controller\n");
> + return PTR_ERR(clkc->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(clkc);
> + if (IS_ERR(mux))
> + return PTR_ERR(mux);
> +
> + div = mmc_clkc_register_div(clkc);
> + if (IS_ERR(div))
> + return PTR_ERR(div);
> +
> + core = mmc_clkc_register_phase_clk(clkc, "core", "div",
> + CLK_SET_RATE_PARENT,
> + &mmc_clkc_core_phase);
> + if (IS_ERR(core))
> + return PTR_ERR(core);
> +
> + rx = mmc_clkc_register_phase_clk(clkc, "rx", "core", 0,
> + &clkc->data->rx);
> + if (IS_ERR(rx))
> + return PTR_ERR(rx);
> +
> + tx = mmc_clkc_register_phase_clk(clkc, "tx", "core", 0,
> + &clkc->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);
Hi Jerome
thanks for the review
On 07/12/18 17:09, Jerome Brunet wrote:
> On Tue, 2018-07-10 at 16:36 +0000, Yixun Lan wrote:
>> 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 <[email protected]>
>> ---
>> drivers/clk/meson/Kconfig | 9 +
>> drivers/clk/meson/Makefile | 1 +
>> drivers/clk/meson/mmc-clkc.c | 419 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 429 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..43b7a376746d
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,419 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>
> * Copyright (c) 2017 Baylibre SAS.
> * Author: Jerome Brunet <[email protected]>
>
> Considering that a fair share of the code below has been copied from the clock
> portion of the eMMC driver, which I wrote last year.
>
Ok, fair enough, will fix in next version
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <[email protected]>
>> + */
>> +
>> +#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
>
> Some defines are aligned, some aren't. please be consistent about it.
> I personally prefer when things are aligned but it is just a preference.
>
sounds good to me, I can fix this in next version
>> +
>> +struct clk_regmap_phase_data {
>
> Considering the recent addition of clk_phase in clk/meson, it is not the best
> name to choose.
>
> clk_phase_delay_data ?
>
ok
>> + unsigned long phase_mask;
>> + unsigned long delay_mask;
>> + unsigned int delay_step_ps;
>> +};
>> +
>> +struct mmc_clkc_data {
>> + struct clk_regmap_phase_data tx;
>> + struct clk_regmap_phase_data rx;
>> +};
>
>
>
>> +
>> +struct mmc_clkc_info {
>> + struct device *dev;
>
> I don't think you need keep dev here, considering this is the device data.
>
>> + struct regmap *map;
>> + struct mmc_clkc_data *data;
>> +};
>
> Overall, I don't think you need this structure
>
ok, I could simplify this and drop this structure.
>> +
>> +static inline struct clk_regmap_phase_data *
>> +clk_get_regmap_phase_data(struct clk_regmap *clk)
>
> Update name as well
>
sure
>> +{
>> + return (struct clk_regmap_phase_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,
>> +};
>
> Same comment on alignement. Some structure you've aligned, and some you haven't
> Consistency ...
>
will fix
>> +
>> +static struct clk_regmap_phase_data mmc_clkc_core_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_mux(struct mmc_clkc_info *clkc)
>> +{
>> + const char *parent_names[MUX_CLK_NUM_PARENTS];
>> + struct device *dev = clkc->dev;
>> + struct clk_init_data init;
>> + struct clk_regmap *mux;
>> + struct clk *clk;
>> + char *mux_name;
>> + int i, ret;
>> +
>> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> + if (!mux)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + 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);
>> + }
>> +
>> + mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
>> + if (!mux_name)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mux->map = clkc->map;
>> + mux->data = &mmc_clkc_mux_data;
>> +
>> + init.name = mux_name;
>> + 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->hw.init = &init;
>> + ret = devm_clk_hw_register(dev, &mux->hw);
>> + if (ret) {
>> + dev_err(dev, "Mux clock registration failed\n");
>> + mux = ERR_PTR(ret);
>> + }
>> +
>> + kfree(mux_name);
>> + return mux;
>> +}
>> +
>> +static struct clk_regmap *mmc_clkc_register_div(struct mmc_clkc_info *clkc)
>> +{
>> + const char *parent_names[MUX_CLK_NUM_PARENTS];
>> + struct device *dev = clkc->dev;
>> + struct clk_init_data init;
>> + struct clk_regmap *div;
>> + char *mux_name, *div_name;
>> + int ret;
>> +
>> + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
>> + if (!div)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
>> + div_name = kasprintf(GFP_KERNEL, "%s#div", dev_name(dev));
>> + if (!mux_name || !div_name) {
>> + div = ERR_PTR(-ENOMEM);
>> + goto err;
>> + }
>> +
>> + parent_names[0] = mux_name;
>> + div->map = clkc->map;
>> + div->data = &mmc_clkc_div_data;
>> +
>> + init.name = div_name;
>> + init.ops = &clk_regmap_divider_ops;
>> + init.flags = CLK_SET_RATE_PARENT;
>> + init.parent_names = parent_names;
>> + init.num_parents = 1;
>> +
>> + div->hw.init = &init;
>> + ret = devm_clk_hw_register(dev, &div->hw);
>> + if (ret) {
>> + dev_err(dev, "Divider clock registration failed\n");
>> + div = ERR_PTR(ret);
>> + }
>> +
>> +err:
>> + kfree(mux_name);
>> + kfree(div_name);
>> + return div;
>> +}
>> +
>> +static int clk_regmap_get_phase(struct clk_hw *hw)
>> +{
>> + struct clk_regmap *clk = to_clk_regmap(hw);
>> + struct clk_regmap_phase_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_regmap_phase_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_regmap_phase_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_phase_clk(struct mmc_clkc_info *clkc,
>> + char *name, char *parent_name, unsigned long flags,
>> + struct clk_regmap_phase_data *phase_data)
>> +
>> +{
>> + const char *parent_names[MUX_CLK_NUM_PARENTS];
>> + struct device *dev = clkc->dev;
>> + struct clk_init_data init;
>> + struct clk_regmap *x;
>
> x is not a very good name, 'clk' is just 2 more letters ...
>
ok
>> + char *clk_name, *core_name;
>> + int ret;
>> +
>> + /* create the mmc rx clock */
>> + x = devm_kzalloc(dev, sizeof(*x), GFP_KERNEL);
>> + if (!x)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + clk_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), name);
>> + core_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_name);
>> +
>> + if (!clk_name || !core_name) {
>> + x = ERR_PTR(-ENOMEM);
>> + goto err;
>> + }
>> + parent_names[0] = core_name;
>> +
>> + init.name = clk_name;
>> + init.ops = &clk_regmap_phase_ops;
>> + init.flags = flags;
>> + init.parent_names = parent_names;
>> + init.num_parents = 1;
>> +
>> + x->map = clkc->map;
>> + x->data = phase_data;
>> + x->hw.init = &init;
>> +
>> + ret = devm_clk_hw_register(dev, &x->hw);
>> + if (ret) {
>> + dev_err(dev, "Core %s clock registration failed\n", name);
>> + x = ERR_PTR(ret);
>> + }
>> +err:
>> + kfree(clk_name);
>> + kfree(core_name);
>> + return x;
>> +}
>
> In all your mmc_clkc_register_xxx_() you should a pattern repeating itself. It
> is something I pointed out in the previous version of your patchset
>
I probably mis-understand your suggestion.. which I gave a try to
construct a helper..
I need to rethink about this.
> 1. allocate a clk_regmap
> 2. allocate a clock name using dev name and a suffix.
> 3. register clk_hw
> 4. free clock name
> 5. return clk_regmap
>
> You should be able to make an helper function out of this
> Prototype will probably look like this:
>
> static struct clk_regmap *
> mmc_clkc_register_clk(struct device *dev, struct regmap *map,
> struct clk_init_data *init, const char* suffix)
>
ok, thanks for the suggestion
>> +
>> +static int mmc_clkc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct mmc_clkc_info *clkc;
>> + struct clk_regmap *mux, *div, *core, *rx, *tx;
>> + struct clk_hw_onecell_data *onecell_data;
>> +
>> + clkc = devm_kzalloc(dev, sizeof(*clkc), GFP_KERNEL);
>> + if (!clkc)
>> + return -ENOMEM;
>> +
>> + clkc->data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
>> + if (!clkc->data)
>> + return -EINVAL;
>
> Prefer ENODEV for this
>
ok
>> +
>> + clkc->dev = dev;
>> + clkc->map = syscon_node_to_regmap(dev->of_node);
>> +
>> + if (IS_ERR(clkc->map)) {
>> + dev_err(dev, "could not find mmc clock controller\n");
>> + return PTR_ERR(clkc->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(clkc);
>> + if (IS_ERR(mux))
>> + return PTR_ERR(mux);
>> +
>> + div = mmc_clkc_register_div(clkc);
>> + if (IS_ERR(div))
>> + return PTR_ERR(div);
>> +
>> + core = mmc_clkc_register_phase_clk(clkc, "core", "div",
>> + CLK_SET_RATE_PARENT,
>> + &mmc_clkc_core_phase);
>> + if (IS_ERR(core))
>> + return PTR_ERR(core);
>> +
>> + rx = mmc_clkc_register_phase_clk(clkc, "rx", "core", 0,
>> + &clkc->data->rx);
>> + if (IS_ERR(rx))
>> + return PTR_ERR(rx);
>> +
>> + tx = mmc_clkc_register_phase_clk(clkc, "tx", "core", 0,
>> + &clkc->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);
>
> .
>
On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan <[email protected]> wrote:
>
> Hi Rob
>
> see my comments
>
> On 07/12/18 03:43, Rob Herring wrote:
> > On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
> >> Document the MMC sub clock controller driver, the potential consumer
> >> of this driver is MMC or NAND.
> >
> > So you all have decided to properly model this now?
> >
> Yes, ;-)
>
> >>
> >> Signed-off-by: Yixun Lan <[email protected]>
> >> ---
> >> .../bindings/clock/amlogic,mmc-clkc.txt | 31 +++++++++++++++++++
> >> 1 file changed, 31 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >>
> >> 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..ff6b4bf3ecf9
> >> --- /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: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
> >
> > You don't need "simple-mfd" and probably not syscon either. The order is
> > wrong too. Most specific first.
> >
> Ok, I will drop "simple-mfd"..
>
> but the syscon is a must, since this mmc clock model access registers
> via the regmap interface
A syscon compatible should not be the only way to get a regmap.
Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.
Why do you need a regmap in the first place? What else needs to access
this register directly? Don't you need a patch removing the clock code
from within the emmc driver? It's not even using regmap, so using
regmap here doesn't help.
Rob
HI Rob
see my comments
On 07/12/2018 10:17 PM, Rob Herring wrote:
> On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan <[email protected]> wrote:
>>
>> Hi Rob
>>
>> see my comments
>>
>> On 07/12/18 03:43, Rob Herring wrote:
>>> On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
>>>> Document the MMC sub clock controller driver, the potential consumer
>>>> of this driver is MMC or NAND.
>>>
>>> So you all have decided to properly model this now?
>>>
>> Yes, ;-)
>>
>>>>
>>>> Signed-off-by: Yixun Lan <[email protected]>
>>>> ---
>>>> .../bindings/clock/amlogic,mmc-clkc.txt | 31 +++++++++++++++++++
>>>> 1 file changed, 31 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>>>
>>>> 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..ff6b4bf3ecf9
>>>> --- /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: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
>>>
>>> You don't need "simple-mfd" and probably not syscon either. The order is
>>> wrong too. Most specific first.
>>>
>> Ok, I will drop "simple-mfd"..
>>
>> but the syscon is a must, since this mmc clock model access registers
>> via the regmap interface
>
> A syscon compatible should not be the only way to get a regmap.
do you have any suggestion about other function that I can use? is
devm_regmap_init_mmio() feasible
> Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.
>
I'm not sure what's the valid point of removing compatible 'syscon' in
driver/mfd/syscon.c, sounds this will break a lot DT/or need to fix?
will you propose a patch for this? then I can certainly adjust here
> Why do you need a regmap in the first place? What else needs to access
> this register directly?
Yes, the SD_EMMC_CLOCK register contain several bits which not fit well
into common clock model, and they need to be access in the NAND or eMMC
driver itself, Martin had explained this in early thread[1]
In this register
Bit[31] select NAND or eMMC function
Bit[25] enable SDIO IRQ
Bit[24] Clock always on
Bit[15:14] SRAM Power down
[1]
https://lkml.kernel.org/r/CAFBinCBeyXf6LNaZzAw6WnsxzDAv8E=Yp2eem0xCPWMEUi6pnQ@mail.gmail.com
> Don't you need a patch removing the clock code
> from within the emmc driver? It's not even using regmap, so using
> regmap here doesn't help.
>
No, and current eMMC driver still use iomap to access the register
I think we probably would like to take two steps approach.
first, from the hardware perspective, the NAND and eMMC(port C) driver
can't exist at same time, since they share the pins, clock, internal
ram, So we have to only enable one of NAND or eMMC in DT, not enable
both of them.
Second, we might like to convert eMMC driver to also use mmc-clkc model.
Yixun
On Thu, Jul 12, 2018 at 5:29 PM Yixun Lan <[email protected]> wrote:
>
> HI Rob
>
> see my comments
>
> On 07/12/2018 10:17 PM, Rob Herring wrote:
> > On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan <[email protected]> wrote:
> >>
> >> Hi Rob
> >>
> >> see my comments
> >>
> >> On 07/12/18 03:43, Rob Herring wrote:
> >>> On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
> >>>> Document the MMC sub clock controller driver, the potential consumer
> >>>> of this driver is MMC or NAND.
> >>>
> >>> So you all have decided to properly model this now?
> >>>
> >> Yes, ;-)
> >>
> >>>>
> >>>> Signed-off-by: Yixun Lan <[email protected]>
> >>>> ---
> >>>> .../bindings/clock/amlogic,mmc-clkc.txt | 31 +++++++++++++++++++
> >>>> 1 file changed, 31 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >>>>
> >>>> 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..ff6b4bf3ecf9
> >>>> --- /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: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
> >>>
> >>> You don't need "simple-mfd" and probably not syscon either. The order is
> >>> wrong too. Most specific first.
> >>>
> >> Ok, I will drop "simple-mfd"..
> >>
> >> but the syscon is a must, since this mmc clock model access registers
> >> via the regmap interface
> >
> > A syscon compatible should not be the only way to get a regmap.
> do you have any suggestion about other function that I can use? is
> devm_regmap_init_mmio() feasible
>
> > Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.
> >
> I'm not sure what's the valid point of removing compatible 'syscon' in
> driver/mfd/syscon.c, sounds this will break a lot DT/or need to fix?
> will you propose a patch for this? then I can certainly adjust here
Removing the 2 lines will simply allow any node to be a syscon. If
there's a specific driver for a node, then that makes sense to allow
that.
>
> > Why do you need a regmap in the first place? What else needs to access
> > this register directly?
> Yes, the SD_EMMC_CLOCK register contain several bits which not fit well
> into common clock model, and they need to be access in the NAND or eMMC
> driver itself, Martin had explained this in early thread[1]
>
> In this register
> Bit[31] select NAND or eMMC function
> Bit[25] enable SDIO IRQ
> Bit[24] Clock always on
> Bit[15:14] SRAM Power down
>
> [1]
> https://lkml.kernel.org/r/CAFBinCBeyXf6LNaZzAw6WnsxzDAv8E=Yp2eem0xCPWMEUi6pnQ@mail.gmail.com
>
> > Don't you need a patch removing the clock code
> > from within the emmc driver? It's not even using regmap, so using
> > regmap here doesn't help.
> >
> No, and current eMMC driver still use iomap to access the register
Which means a read-modify-write can corrupt the register value if both
users don't access thru regmap. Changes are probably infrequent enough
that you get lucky...
> I think we probably would like to take two steps approach.
> first, from the hardware perspective, the NAND and eMMC(port C) driver
> can't exist at same time, since they share the pins, clock, internal
> ram, So we have to only enable one of NAND or eMMC in DT, not enable
> both of them.
Yes, of course.
> Second, we might like to convert eMMC driver to also use mmc-clkc model.
IMO, this should be done as part of merging this series. Otherwise, we
have duplicated code for the same thing.
Rob
Hi Rob, Jerome, Kevin
see my comments
On 07/13/18 08:15, Rob Herring wrote:
> On Thu, Jul 12, 2018 at 5:29 PM Yixun Lan <[email protected]> wrote:
>>
>> HI Rob
>>
>> see my comments
>>
>> On 07/12/2018 10:17 PM, Rob Herring wrote:
>>> On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan <[email protected]> wrote:
>>>>
>>>> Hi Rob
>>>>
>>>> see my comments
>>>>
>>>> On 07/12/18 03:43, Rob Herring wrote:
>>>>> On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
>>>>>> Document the MMC sub clock controller driver, the potential consumer
>>>>>> of this driver is MMC or NAND.
>>>>>
>>>>> So you all have decided to properly model this now?
>>>>>
>>>> Yes, ;-)
>>>>
>>>>>>
>>>>>> Signed-off-by: Yixun Lan <[email protected]>
>>>>>> ---
>>>>>> .../bindings/clock/amlogic,mmc-clkc.txt | 31 +++++++++++++++++++
>>>>>> 1 file changed, 31 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>>>>>
>>>>>> 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..ff6b4bf3ecf9
>>>>>> --- /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: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
>>>>>
>>>>> You don't need "simple-mfd" and probably not syscon either. The order is
>>>>> wrong too. Most specific first.
>>>>>
>>>> Ok, I will drop "simple-mfd"..
>>>>
>>>> but the syscon is a must, since this mmc clock model access registers
>>>> via the regmap interface
>>>
>>> A syscon compatible should not be the only way to get a regmap.
>> do you have any suggestion about other function that I can use? is
>> devm_regmap_init_mmio() feasible
>>
>>> Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.
>>>
>> I'm not sure what's the valid point of removing compatible 'syscon' in
>> driver/mfd/syscon.c, sounds this will break a lot DT/or need to fix?
>> will you propose a patch for this? then I can certainly adjust here
>
> Removing the 2 lines will simply allow any node to be a syscon. If
> there's a specific driver for a node, then that makes sense to allow
> that.
>
>>
>>> Why do you need a regmap in the first place? What else needs to access
>>> this register directly?
>> Yes, the SD_EMMC_CLOCK register contain several bits which not fit well
>> into common clock model, and they need to be access in the NAND or eMMC
>> driver itself, Martin had explained this in early thread[1]
>>
>> In this register
>> Bit[31] select NAND or eMMC function
>> Bit[25] enable SDIO IRQ
>> Bit[24] Clock always on
>> Bit[15:14] SRAM Power down
>>
>> [1]
>> https://lkml.kernel.org/r/CAFBinCBeyXf6LNaZzAw6WnsxzDAv8E=Yp2eem0xCPWMEUi6pnQ@mail.gmail.com
>>
>>> Don't you need a patch removing the clock code
>>> from within the emmc driver? It's not even using regmap, so using
>>> regmap here doesn't help.
>>>
>> No, and current eMMC driver still use iomap to access the register
>
> Which means a read-modify-write can corrupt the register value if both
> users don't access thru regmap. Changes are probably infrequent enough
> that you get lucky...
>
What's you says here is true.
and we try to guarantee that only one of NAND or eMMC is enabled, so no
race condition, as a example of the use cases:
1) for enabling NAND driver, we do
a) enable both mmc-clkc, and NAND driver in DT, they can access
register by using regmap interface
b) disable eMMC DT node
2) for enabling eMMC driver, we do
a) enable eMMC node, access register by using iomap (for now)
b) disable both mmc-clkc and NAND in DT
>> I think we probably would like to take two steps approach.
>> first, from the hardware perspective, the NAND and eMMC(port C) driver
>> can't exist at same time, since they share the pins, clock, internal
>> ram, So we have to only enable one of NAND or eMMC in DT, not enable
>> both of them.
>
> Yes, of course.
>
>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>
> IMO, this should be done as part of merging this series. Otherwise, we
> have duplicated code for the same thing.
IMO, I'd leave this out of this series, since this patch series is quite
complete as itself. Although, the downside is code duplication.
Still, I need to hear Jerome, or Kevin's option, to see if or how we
should proceed the eMMC's clock conversion.
I could think of three option myself
1) don't do the conversion, downside is code duplication, upside is NO
DT change, no compatibility issue
2) add a syscon node into eMMC DT node, then only convert clock part
into this mmc-clkc model, while still leave other eMMC register access
as the usual iomap way (still no race condition)
3) convert all eMMC register access by using regmap interface.
both 2) and 3) need to update the DT.
and probably 2) is a compromise way, and 1) is also OK, 3) is probably
the worst way due to dramatically change (I think this was already
rejected in the previous discussion)
Yixun
Yixun Lan <[email protected]> writes:
[...]
>>
>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>
>> IMO, this should be done as part of merging this series. Otherwise, we
>> have duplicated code for the same thing.
>
> IMO, I'd leave this out of this series, since this patch series is quite
> complete as itself. Although, the downside is code duplication.
>
> Still, I need to hear Jerome, or Kevin's option, to see if or how we
> should proceed the eMMC's clock conversion.
>
> I could think of three option myself
> 1) don't do the conversion, downside is code duplication, upside is NO
> DT change, no compatibility issue
> 2) add a syscon node into eMMC DT node, then only convert clock part
> into this mmc-clkc model, while still leave other eMMC register access
> as the usual iomap way (still no race condition)
> 3) convert all eMMC register access by using regmap interface.
>
> both 2) and 3) need to update the DT.
>
> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
> the worst way due to dramatically change (I think this was already
> rejected in the previous discussion)
Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
step-by-step approach is fine (and preferred) by me.
Phase 1:
- add new mmc-clk provider
- add NAND driver using new mmc-clk provider
- boards using NAND should ensure emmc_c is disabled in DT
This allows us to not touch the MMC driver or existing upstream
bindings. Yes, this means there is duplicate code in the MMC driver and
the new mmc-clk provider, but that can be removed in the next phase.
Phase 2:
- convert MMC driver to use new mmc-clk provider
- update MMC users in DT and bindings
Kevin
HI Kevin
On 07/23/2018 10:12 PM, Kevin Hilman wrote:
> Yixun Lan <[email protected]> writes:
>
> [...]
>
>>>
>>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>>
>>> IMO, this should be done as part of merging this series. Otherwise, we
>>> have duplicated code for the same thing.
>>
>> IMO, I'd leave this out of this series, since this patch series is quite
>> complete as itself. Although, the downside is code duplication.
>>
>> Still, I need to hear Jerome, or Kevin's option, to see if or how we
>> should proceed the eMMC's clock conversion.
>>
>> I could think of three option myself
>> 1) don't do the conversion, downside is code duplication, upside is NO
>> DT change, no compatibility issue
>> 2) add a syscon node into eMMC DT node, then only convert clock part
>> into this mmc-clkc model, while still leave other eMMC register access
>> as the usual iomap way (still no race condition)
>> 3) convert all eMMC register access by using regmap interface.
>>
>> both 2) and 3) need to update the DT.
>>
>> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
>> the worst way due to dramatically change (I think this was already
>> rejected in the previous discussion)
>
> Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
> step-by-step approach is fine (and preferred) by me.
>
> Phase 1:
> - add new mmc-clk provider
> - add NAND driver using new mmc-clk provider
> - boards using NAND should ensure emmc_c is disabled in DT
>
> This allows us to not touch the MMC driver or existing upstream
> bindings. Yes, this means there is duplicate code in the MMC driver and
> the new mmc-clk provider, but that can be removed in the next phase.
>
Great, the approach to address this issue is reasonable.
We'd like to focus on phase 1 first, thanks
> Phase 2:
> - convert MMC driver to use new mmc-clk provider
> - update MMC users in DT and bindings
>
Ok.
Yixun