2018-08-09 07:09:28

by Yixun Lan

[permalink] [raw]
Subject: [PATCH v4 0/3] clk: meson: add a sub EMMC clock controller support

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

This driver is tested in the S400 board (AXG platform) with NAND driver.

Changes since v3 [4]:
- separate clk-phase-delay driver
- replace clk_get_rate() with clk_hw_get_rate()
- collect Rob's R-Y
- drop 'meson-' prefix from compatible string

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/[email protected]
[3] https://lkml.kernel.org/r/[email protected]
[4] https://lkml.kernel.org/r/[email protected]

Yixun Lan (3):
clk: meson: add emmc sub clock phase delay driver
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 | 10 +
drivers/clk/meson/Makefile | 3 +-
drivers/clk/meson/clk-phase-delay.c | 96 ++++++
drivers/clk/meson/clkc.h | 13 +
drivers/clk/meson/mmc-clkc.c | 275 ++++++++++++++++++
include/dt-bindings/clock/amlogic,mmc-clkc.h | 17 ++
7 files changed, 444 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
create mode 100644 drivers/clk/meson/clk-phase-delay.c
create mode 100644 drivers/clk/meson/mmc-clkc.c
create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h

--
2.17.1



2018-08-09 07:09:54

by Yixun Lan

[permalink] [raw]
Subject: [PATCH v4 2/3] clk: meson: add DT documentation for emmc clock controller

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

Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Yixun Lan <[email protected]>
---
.../bindings/clock/amlogic,mmc-clkc.txt | 31 +++++++++++++++++++
include/dt-bindings/clock/amlogic,mmc-clkc.h | 17 ++++++++++
2 files changed, 48 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
create mode 100644 include/dt-bindings/clock/amlogic,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..9e6d34389be8
--- /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,gx-mmc-clkc"
+ "amlogic,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,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,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,mmc-clkc.h b/include/dt-bindings/clock/amlogic,mmc-clkc.h
new file mode 100644
index 000000000000..162b94949119
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,mmc-clkc.h
@@ -0,0 +1,17 @@
+/* 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_CORE 2
+#define CLKID_MMC_PHASE_TX 3
+#define CLKID_MMC_PHASE_RX 4
+
+#endif
--
2.17.1


2018-08-09 07:09:57

by Yixun Lan

[permalink] [raw]
Subject: [PATCH v4 3/3] clk: meson: add sub MMC clock controller driver

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/amlogic,mmc-clkc.h header
can be used in the device tree sources.

Signed-off-by: Yixun Lan <[email protected]>
---
drivers/clk/meson/Kconfig | 10 ++
drivers/clk/meson/Makefile | 1 +
drivers/clk/meson/mmc-clkc.c | 275 +++++++++++++++++++++++++++++++++++
3 files changed, 286 insertions(+)
create mode 100644 drivers/clk/meson/mmc-clkc.c

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index efaa70f682b4..8b8ccbcfed1d 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -15,6 +15,16 @@ 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,
+ which include S905 (GXBB, GXL), A113D/X (AXG) devices.
+ 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 39ce5661b654..31c16d524a4b 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..6aa055f7e62c
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,275 @@
+// 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]>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <[email protected]>
+ */
+
+#include <linux/clk.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,mmc-clkc.h>
+
+#include "clkc.h"
+
+/* clock ID used by internal driver */
+#define CLKID_MMC_MUX 0
+
+#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 mmc_clkc_data {
+ struct meson_clk_phase_delay_data tx;
+ struct meson_clk_phase_delay_data rx;
+};
+
+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 meson_clk_phase_delay_data mmc_clkc_core_phase_delay = {
+ .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,gx-mmc-clkc",
+ .data = &mmc_clkc_gx_data
+ },
+ {
+ .compatible = "amlogic,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 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,
+ &meson_clk_phase_delay_ops,
+ &mmc_clkc_core_phase_delay);
+ if (IS_ERR(core))
+ return PTR_ERR(core);
+
+ rx = mmc_clkc_register_other_clk(dev, map, "rx", "core", 0,
+ &meson_clk_phase_delay_ops,
+ &data->rx);
+ if (IS_ERR(rx))
+ return PTR_ERR(rx);
+
+ tx = mmc_clkc_register_other_clk(dev, map, "tx", "core", 0,
+ &meson_clk_phase_delay_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.17.1


2018-08-09 07:10:14

by Yixun Lan

[permalink] [raw]
Subject: [PATCH v4 1/3] clk: meson: add emmc sub clock phase delay driver

Export the emmc sub clock phase delay ops which will be used
by the emmc sub clock driver itself.

Signed-off-by: Yixun Lan <[email protected]>
---
drivers/clk/meson/Makefile | 2 +-
drivers/clk/meson/clk-phase-delay.c | 96 +++++++++++++++++++++++++++++
drivers/clk/meson/clkc.h | 13 ++++
3 files changed, 110 insertions(+), 1 deletion(-)
create mode 100644 drivers/clk/meson/clk-phase-delay.c

diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 72ec8c40d848..39ce5661b654 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -2,7 +2,7 @@
# Makefile for Meson specific clk
#

-obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-phase.o
+obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-phase.o clk-phase-delay.o
obj-$(CONFIG_COMMON_CLK_AMLOGIC_AUDIO) += clk-triphase.o sclk-div.o
obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
diff --git a/drivers/clk/meson/clk-phase-delay.c b/drivers/clk/meson/clk-phase-delay.c
new file mode 100644
index 000000000000..6f226814cfec
--- /dev/null
+++ b/drivers/clk/meson/clk-phase-delay.c
@@ -0,0 +1,96 @@
+// 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]>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <[email protected]>
+ */
+
+#include <linux/clk-provider.h>
+#include "clkc.h"
+
+#define SD_EMMC_CLOCK 0
+
+static int meson_clk_phase_delay_get_phase(struct clk_hw *hw)
+{
+ struct clk_regmap *clk = to_clk_regmap(hw);
+ struct meson_clk_phase_delay_data *ph =
+ meson_clk_get_phase_delay_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_hw_get_rate(hw));
+ d = (val & ph->delay_mask) >> __ffs(ph->delay_mask);
+ degrees += d * ph->delay_step_ps * 360 / period_ps;
+ degrees %= 360;
+ }
+
+ return degrees;
+}
+
+static void meson_clk_apply_phase_delay(struct clk_regmap *clk,
+ unsigned int phase,
+ unsigned int delay)
+{
+ struct meson_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 meson_clk_phase_delay_set_phase(struct clk_hw *hw, int degrees)
+{
+ struct clk_regmap *clk = to_clk_regmap(hw);
+ struct meson_clk_phase_delay_data *ph =
+ meson_clk_get_phase_delay_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_hw_get_rate(hw));
+
+ /* 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));
+ }
+
+ meson_clk_apply_phase_delay(clk, p, d);
+ return 0;
+}
+
+const struct clk_ops meson_clk_phase_delay_ops = {
+ .get_phase = meson_clk_phase_delay_get_phase,
+ .set_phase = meson_clk_phase_delay_set_phase,
+};
+EXPORT_SYMBOL_GPL(meson_clk_phase_delay_ops);
diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
index 24cec16b6038..499834dd34f2 100644
--- a/drivers/clk/meson/clkc.h
+++ b/drivers/clk/meson/clkc.h
@@ -113,6 +113,18 @@ struct clk_regmap _name = { \
}, \
};

+struct meson_clk_phase_delay_data {
+ unsigned long phase_mask;
+ unsigned long delay_mask;
+ unsigned int delay_step_ps;
+};
+
+static inline struct meson_clk_phase_delay_data *
+meson_clk_get_phase_delay_data(struct clk_regmap *clk)
+{
+ return (struct meson_clk_phase_delay_data *)clk->data;
+}
+
/* clk_ops */
extern const struct clk_ops meson_clk_pll_ro_ops;
extern const struct clk_ops meson_clk_pll_ops;
@@ -120,5 +132,6 @@ extern const struct clk_ops meson_clk_cpu_ops;
extern const struct clk_ops meson_clk_mpll_ro_ops;
extern const struct clk_ops meson_clk_mpll_ops;
extern const struct clk_ops meson_clk_phase_ops;
+extern const struct clk_ops meson_clk_phase_delay_ops;

#endif /* __CLKC_H */
--
2.17.1


2018-08-10 12:29:40

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH v4 1/3] clk: meson: add emmc sub clock phase delay driver

On Thu, 2018-08-09 at 15:07 +0800, Yixun Lan wrote:
> Export the emmc sub clock phase delay ops which will be used
> by the emmc sub clock driver itself.
>
> Signed-off-by: Yixun Lan <[email protected]>
> ---
> drivers/clk/meson/Makefile | 2 +-
> drivers/clk/meson/clk-phase-delay.c | 96 +++++++++++++++++++++++++++++
> drivers/clk/meson/clkc.h | 13 ++++
> 3 files changed, 110 insertions(+), 1 deletion(-)
> create mode 100644 drivers/clk/meson/clk-phase-delay.c
>
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 72ec8c40d848..39ce5661b654 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -2,7 +2,7 @@
> # Makefile for Meson specific clk
> #
>
> -obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-phase.o
> +obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-phase.o clk-phase-delay.o
> obj-$(CONFIG_COMMON_CLK_AMLOGIC_AUDIO) += clk-triphase.o sclk-div.o
> obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
> obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
> diff --git a/drivers/clk/meson/clk-phase-delay.c b/drivers/clk/meson/clk-phase-delay.c
> new file mode 100644
> index 000000000000..6f226814cfec
> --- /dev/null
> +++ b/drivers/clk/meson/clk-phase-delay.c
> @@ -0,0 +1,96 @@
> +// 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]>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <[email protected]>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include "clkc.h"
> +
> +#define SD_EMMC_CLOCK 0

Drop this and use struct parm please.

> +
> +static int meson_clk_phase_delay_get_phase(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_clk_phase_delay_data *ph =
> + meson_clk_get_phase_delay_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_hw_get_rate(hw));
> + d = (val & ph->delay_mask) >> __ffs(ph->delay_mask);
> + degrees += d * ph->delay_step_ps * 360 / period_ps;
> + degrees %= 360;
> + }
> +
> + return degrees;
> +}
> +
> +static void meson_clk_apply_phase_delay(struct clk_regmap *clk,
> + unsigned int phase,
> + unsigned int delay)
> +{
> + struct meson_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) {

This imply that delay is delay is optional. In such case, it would be a regular
"phase" and we already have a driver for this. Please remove all the related
code and make this parameter required for this clock type.

In the future, maybe we'll merge the 2 drivers.

> + val &= ~ph->delay_mask;
> + val |= delay << __ffs(ph->delay_mask);
> + }
> +
> + regmap_write(clk->map, SD_EMMC_CLOCK, val);
> +}
> +
> +static int meson_clk_phase_delay_set_phase(struct clk_hw *hw, int degrees)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_clk_phase_delay_data *ph =
> + meson_clk_get_phase_delay_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_hw_get_rate(hw));
> +
> + /* 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));
> + }
> +
> + meson_clk_apply_phase_delay(clk, p, d);
> + return 0;
> +}
> +
> +const struct clk_ops meson_clk_phase_delay_ops = {
> + .get_phase = meson_clk_phase_delay_get_phase,
> + .set_phase = meson_clk_phase_delay_set_phase,
> +};
> +EXPORT_SYMBOL_GPL(meson_clk_phase_delay_ops);
> diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
> index 24cec16b6038..499834dd34f2 100644
> --- a/drivers/clk/meson/clkc.h
> +++ b/drivers/clk/meson/clkc.h
> @@ -113,6 +113,18 @@ struct clk_regmap _name = { \
> }, \
> };
>
> +struct meson_clk_phase_delay_data {
> + unsigned long phase_mask;
> + unsigned long delay_mask;

Like every other clock drivers in the amlogic directory, please use struct parm
to decribe the fields.

> + unsigned int delay_step_ps;
> +};
> +
> +static inline struct meson_clk_phase_delay_data *
> +meson_clk_get_phase_delay_data(struct clk_regmap *clk)
> +{
> + return (struct meson_clk_phase_delay_data *)clk->data;
> +}
> +
> /* clk_ops */
> extern const struct clk_ops meson_clk_pll_ro_ops;
> extern const struct clk_ops meson_clk_pll_ops;
> @@ -120,5 +132,6 @@ extern const struct clk_ops meson_clk_cpu_ops;
> extern const struct clk_ops meson_clk_mpll_ro_ops;
> extern const struct clk_ops meson_clk_mpll_ops;
> extern const struct clk_ops meson_clk_phase_ops;
> +extern const struct clk_ops meson_clk_phase_delay_ops;
>
> #endif /* __CLKC_H */



2018-08-10 12:50:18

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH v4 3/3] clk: meson: add sub MMC clock controller driver

On Thu, 2018-08-09 at 15:07 +0800, 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/amlogic,mmc-clkc.h header
> can be used in the device tree sources.
>
> Signed-off-by: Yixun Lan <[email protected]>
> ---
> drivers/clk/meson/Kconfig | 10 ++
> drivers/clk/meson/Makefile | 1 +
> drivers/clk/meson/mmc-clkc.c | 275 +++++++++++++++++++++++++++++++++++
> 3 files changed, 286 insertions(+)
> create mode 100644 drivers/clk/meson/mmc-clkc.c
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index efaa70f682b4..8b8ccbcfed1d 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -15,6 +15,16 @@ 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,
> + which include S905 (GXBB, GXL), A113D/X (AXG) devices.
> + 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 39ce5661b654..31c16d524a4b 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..6aa055f7e62c
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,275 @@
> +// 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]>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <[email protected]>
> + */
> +
> +#include <linux/clk.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,mmc-clkc.h>
> +
> +#include "clkc.h"
> +
> +/* clock ID used by internal driver */
> +#define CLKID_MMC_MUX 0
> +
> +#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 mmc_clkc_data {
> + struct meson_clk_phase_delay_data tx;
> + struct meson_clk_phase_delay_data rx;
> +};
> +
> +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 meson_clk_phase_delay_data mmc_clkc_core_phase_delay = {
> + .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,gx-mmc-clkc",
> + .data = &mmc_clkc_gx_data
> + },
> + {
> + .compatible = "amlogic,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 struct clk_regmap *
> +mmc_clkc_register_other_clk(struct device *dev, struct regmap *map,

This is a poor name. It does not help understand what the function does.
mmc_clkc_register_clk_with_parent or whatever helps understand what's going on

> + char *suffix, char *parent_suffix,

You should not have to rebuild the parent name once again.
Pass the clk_hw* of the parent and get the name from it.

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

Do you really need all this ? or could store them in your onecell_data->hws
table as you go ?

> +
> + 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,
> + &meson_clk_phase_delay_ops,
> + &mmc_clkc_core_phase_delay);
> + if (IS_ERR(core))
> + return PTR_ERR(core);
> +
> + rx = mmc_clkc_register_other_clk(dev, map, "rx", "core", 0,
> + &meson_clk_phase_delay_ops,
> + &data->rx);
> + if (IS_ERR(rx))
> + return PTR_ERR(rx);
> +
> + tx = mmc_clkc_register_other_clk(dev, map, "tx", "core", 0,
> + &meson_clk_phase_delay_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);



2018-10-11 08:39:37

by Jianxin Pan

[permalink] [raw]
Subject: Re: [PATCH v4 1/3] clk: meson: add emmc sub clock phase delay driver


Hi Jerome,

Sorry to miss this mail. I thought Yixun has finished this patchset.
I will continue his work.

On 2018/8/10 20:27, Jerome Brunet wrote:
> On Thu, 2018-08-09 at 15:07 +0800, Yixun Lan wrote:
>> Export the emmc sub clock phase delay ops which will be used
>> by the emmc sub clock driver itself.
>>
>> Signed-off-by: Yixun Lan <[email protected]>
>> ---
>> drivers/clk/meson/Makefile | 2 +-
>> drivers/clk/meson/clk-phase-delay.c | 96 +++++++++++++++++++++++++++++
>> drivers/clk/meson/clkc.h | 13 ++++
>> 3 files changed, 110 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/clk/meson/clk-phase-delay.c
>>
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index 72ec8c40d848..39ce5661b654 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -2,7 +2,7 @@
>> # Makefile for Meson specific clk
>> #
>>
>> -obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-phase.o
>> +obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-phase.o clk-phase-delay.o
>> obj-$(CONFIG_COMMON_CLK_AMLOGIC_AUDIO) += clk-triphase.o sclk-div.o
>> obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
>> obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>> diff --git a/drivers/clk/meson/clk-phase-delay.c b/drivers/clk/meson/clk-phase-delay.c
>> new file mode 100644
>> index 000000000000..6f226814cfec
>> --- /dev/null
>> +++ b/drivers/clk/meson/clk-phase-delay.c
>> @@ -0,0 +1,96 @@
>> +// 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]>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <[email protected]>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include "clkc.h"
>> +
>> +#define SD_EMMC_CLOCK 0
>
> Drop this and use struct parm please.
>
OK. I will use struct parm instead. Thanks for your time.
>> +
>> +static int meson_clk_phase_delay_get_phase(struct clk_hw *hw)
>> +{
>> + struct clk_regmap *clk = to_clk_regmap(hw);
>> + struct meson_clk_phase_delay_data *ph =
>> + meson_clk_get_phase_delay_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_hw_get_rate(hw));
>> + d = (val & ph->delay_mask) >> __ffs(ph->delay_mask);
>> + degrees += d * ph->delay_step_ps * 360 / period_ps;
>> + degrees %= 360;
>> + }
>> +
>> + return degrees;
>> +}
>> +
>> +static void meson_clk_apply_phase_delay(struct clk_regmap *clk,
>> + unsigned int phase,
>> + unsigned int delay)
>> +{
>> + struct meson_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) {
>
> This imply that delay is delay is optional. In such case, it would be a regular
> "phase" and we already have a driver for this. Please remove all the related
> code and make this parameter required for this clock type.
>
OK. I will use APIs from clk-phase.c for ‘core' phase.
> In the future, maybe we'll merge the 2 drivers.
>
>> + val &= ~ph->delay_mask;
>> + val |= delay << __ffs(ph->delay_mask);
>> + }
>> +
>> + regmap_write(clk->map, SD_EMMC_CLOCK, val);
>> +}
>> +
>> +static int meson_clk_phase_delay_set_phase(struct clk_hw *hw, int degrees)
>> +{
>> + struct clk_regmap *clk = to_clk_regmap(hw);
>> + struct meson_clk_phase_delay_data *ph =
>> + meson_clk_get_phase_delay_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_hw_get_rate(hw));
>> +
>> + /* 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));
>> + }
>> +
>> + meson_clk_apply_phase_delay(clk, p, d);
>> + return 0;
>> +}
>> +
>> +const struct clk_ops meson_clk_phase_delay_ops = {
>> + .get_phase = meson_clk_phase_delay_get_phase,
>> + .set_phase = meson_clk_phase_delay_set_phase,
>> +};
>> +EXPORT_SYMBOL_GPL(meson_clk_phase_delay_ops);
>> diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
>> index 24cec16b6038..499834dd34f2 100644
>> --- a/drivers/clk/meson/clkc.h
>> +++ b/drivers/clk/meson/clkc.h
>> @@ -113,6 +113,18 @@ struct clk_regmap _name = { \
>> }, \
>> };
>>
>> +struct meson_clk_phase_delay_data {
>> + unsigned long phase_mask;
>> + unsigned long delay_mask;
>
> Like every other clock drivers in the amlogic directory, please use struct parm
> to decribe the fields.
>
OK. I will use struct parm to describe phase and delay register fields.
>> + unsigned int delay_step_ps;
>> +};
>> +
>> +static inline struct meson_clk_phase_delay_data *
>> +meson_clk_get_phase_delay_data(struct clk_regmap *clk)
>> +{
>> + return (struct meson_clk_phase_delay_data *)clk->data;
>> +}
>> +
>> /* clk_ops */
>> extern const struct clk_ops meson_clk_pll_ro_ops;
>> extern const struct clk_ops meson_clk_pll_ops;
>> @@ -120,5 +132,6 @@ extern const struct clk_ops meson_clk_cpu_ops;
>> extern const struct clk_ops meson_clk_mpll_ro_ops;
>> extern const struct clk_ops meson_clk_mpll_ops;
>> extern const struct clk_ops meson_clk_phase_ops;
>> +extern const struct clk_ops meson_clk_phase_delay_ops;
>>
>> #endif /* __CLKC_H */
>
>
> .
>


2018-10-11 10:27:52

by Jianxin Pan

[permalink] [raw]
Subject: Re: [PATCH v4 3/3] clk: meson: add sub MMC clock controller driver

On 2018/8/10 20:47, Jerome Brunet wrote:
> On Thu, 2018-08-09 at 15:07 +0800, 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/amlogic,mmc-clkc.h header
>> can be used in the device tree sources.
>>
>> Signed-off-by: Yixun Lan <[email protected]>
>> ---
>> drivers/clk/meson/Kconfig | 10 ++
>> drivers/clk/meson/Makefile | 1 +
>> drivers/clk/meson/mmc-clkc.c | 275 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 286 insertions(+)
>> create mode 100644 drivers/clk/meson/mmc-clkc.c
>>
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index efaa70f682b4..8b8ccbcfed1d 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -15,6 +15,16 @@ 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,
>> + which include S905 (GXBB, GXL), A113D/X (AXG) devices.
>> + 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 39ce5661b654..31c16d524a4b 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..6aa055f7e62c
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,275 @@
>> +// 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]>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <[email protected]>
>> + */
>> +
>> +#include <linux/clk.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,mmc-clkc.h>
>> +
>> +#include "clkc.h"
>> +
>> +/* clock ID used by internal driver */
>> +#define CLKID_MMC_MUX 0
>> +
>> +#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 mmc_clkc_data {
>> + struct meson_clk_phase_delay_data tx;
>> + struct meson_clk_phase_delay_data rx;
>> +};
>> +
>> +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 meson_clk_phase_delay_data mmc_clkc_core_phase_delay = {
>> + .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,gx-mmc-clkc",
>> + .data = &mmc_clkc_gx_data
>> + },
>> + {
>> + .compatible = "amlogic,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 struct clk_regmap *
>> +mmc_clkc_register_other_clk(struct device *dev, struct regmap *map,
>
> This is a poor name. It does not help understand what the function does.
> mmc_clkc_register_clk_with_parent or whatever helps understand what's going on
>
OK, I will repleace it with mmc_clkc_register_clk_with_parent(). Thanks for your review.
>> + char *suffix, char *parent_suffix,
>
> You should not have to rebuild the parent name once again.
> Pass the clk_hw* of the parent and get the name from it.
>
yes, mmc_clkc_register_clk_with_parent() can use parent's hw.init->name directly.
>> + 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;
>
> Do you really need all this ? or could store them in your onecell_data->hws
> table as you go ?
>
3 of them can be removed. Then one local variable will be shared several times.
>> +
>> + 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,
>> + &meson_clk_phase_delay_ops,
>> + &mmc_clkc_core_phase_delay);
>> + if (IS_ERR(core))
>> + return PTR_ERR(core);
>> +
>> + rx = mmc_clkc_register_other_clk(dev, map, "rx", "core", 0,
>> + &meson_clk_phase_delay_ops,
>> + &data->rx);
>> + if (IS_ERR(rx))
>> + return PTR_ERR(rx);
>> +
>> + tx = mmc_clkc_register_other_clk(dev, map, "tx", "core", 0,
>> + &meson_clk_phase_delay_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);
>
>
> .
>