[PATCH v6 0/3] clk: meson: add a sub EMMC clock controller support

[PATCH v6 0/3] clk: meson: add a sub EMMC clock controller support

[Jianxin Pan]

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 v5 [6]:
 - remove divider ops with .init and use sclk_div instead
 - drop CLK_DIVIDER_ROUND_CLOSEST in mux and div
 - drop the useless type cast 

Changes since v4 [5]:
 - use struct parm in phase delay driver
 - remove 0 delay releted part in phase delay driver
 - don't rebuild the parent name once again
 - add divider ops with .init

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 at xps13
[2] https://lkml.kernel.org/r/20180703145716.31860-1-yixun.lan at amlogic.com
[3] https://lkml.kernel.org/r/20180710163658.6175-1-yixun.lan at amlogic.com
[4] https://lkml.kernel.org/r/20180712211244.11428-1-yixun.lan at amlogic.com
[5] https://lkml.kernel.org/r/20180809070724.11935-4-yixun.lan at amlogic.com
[6] https://lkml.kernel.org/r/1539839245-13793-1-git-send-email-jianxin.pan at amlogic.com
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

 .../devicetree/bindings/clock/amlogic,mmc-clkc.txt |  39 +++
 drivers/clk/meson/Kconfig                          |  10 +
 drivers/clk/meson/Makefile                         |   3 +-
 drivers/clk/meson/clk-phase-delay.c                |  75 +++++
 drivers/clk/meson/clk-regmap.c                     |   1 -
 drivers/clk/meson/clk-regmap.h                     |   1 +
 drivers/clk/meson/clkc.h                           |  13 +
 drivers/clk/meson/mmc-clkc.c                       | 310 +++++++++++++++++++++
 include/dt-bindings/clock/amlogic,mmc-clkc.h       |  17 ++
 9 files changed, 467 insertions(+), 2 deletions(-)
 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

-- 
1.9.1

[PATCH v6 1/3] clk: meson: add emmc sub clock phase delay driver

[Jianxin Pan]

From: Yixun Lan <yixun.lan@amlogic.com>

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

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
---
 drivers/clk/meson/Makefile          |  2 +-
 drivers/clk/meson/clk-phase-delay.c | 66 +++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/clkc.h            | 13 ++++++++
 3 files changed, 80 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 72ec8c4..39ce566 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 0000000..83e74ed
--- /dev/null
+++ b/drivers/clk/meson/clk-phase-delay.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2017 Baylibre SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ * Author: Jianxin Pan <jianxin.pan@amlogic.com>
+ */
+
+#include <linux/clk-provider.h>
+#include "clkc.h"
+
+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 long period_ps, p, d;
+	int degrees;
+
+	p = meson_parm_read(clk->map, &ph->phase);
+	degrees = p * 360 / (1 << (ph->phase.width));
+
+	period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
+				 clk_hw_get_rate(hw));
+
+	d = meson_parm_read(clk->map, &ph->delay);
+	degrees += d * ph->delay_step_ps * 360 / period_ps;
+	degrees %= 360;
+
+	return degrees;
+}
+
+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 long period_ps, d = 0, r;
+	u64 p;
+
+	p = degrees % 360;
+	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 / (1 << (ph->phase.width)));
+	d = DIV_ROUND_CLOSEST(r * period_ps,
+			      360 * ph->delay_step_ps);
+	d = min(d, PMASK(ph->delay.width));
+
+	meson_parm_write(clk->map, &ph->phase, p);
+	meson_parm_write(clk->map, &ph->delay, 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 6b96d55..30470c6 100644
--- a/drivers/clk/meson/clkc.h
+++ b/drivers/clk/meson/clkc.h
@@ -105,6 +105,18 @@ struct clk_regmap _name = {						\
 	},								\
 };
 
+struct meson_clk_phase_delay_data {
+	struct parm			phase;
+	struct parm			delay;
+	unsigned int			delay_step_ps;
+};
+
+static inline struct meson_clk_phase_delay_data *
+meson_clk_get_phase_delay_data(struct clk_regmap *clk)
+{
+	return clk->data;
+}
+
 /* clk_ops */
 extern const struct clk_ops meson_clk_pll_ro_ops;
 extern const struct clk_ops meson_clk_pll_ops;
@@ -112,5 +124,6 @@ struct clk_regmap _name = {						\
 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 */
-- 
1.9.1

[PATCH v6 1/3] clk: meson: add emmc sub clock phase delay driver

[Stephen Boyd]

Quoting Jianxin Pan (2018-11-01 09:30:53)
> diff --git a/drivers/clk/meson/clk-phase-delay.c b/drivers/clk/meson/clk-phase-delay.c
> new file mode 100644
> index 0000000..83e74ed
> --- /dev/null
> +++ b/drivers/clk/meson/clk-phase-delay.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + * Author: Jianxin Pan <jianxin.pan@amlogic.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include "clkc.h"
> +
> +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);

Nitpick: Do this after declaring variables because it splits a line.

> +       unsigned long period_ps, p, d;
> +       int degrees;
> +
> +       p = meson_parm_read(clk->map, &ph->phase);
> +       degrees = p * 360 / (1 << (ph->phase.width));

Nitpick: Remove useless parenthesis.

> +
> +       period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,

Is the cast necessary?

> +                                clk_hw_get_rate(hw));
> +
> +       d = meson_parm_read(clk->map, &ph->delay);
> +       degrees += d * ph->delay_step_ps * 360 / period_ps;
> +       degrees %= 360;
> +
> +       return degrees;
> +}
> +
> +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 long period_ps, d = 0, r;
> +       u64 p;
> +
> +       p = degrees % 360;

We don't allow phase to be larger than 360 so this isn't needed.

> +       period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,

Drop the cast?

> +                                clk_hw_get_rate(hw));
> +
> +       /* First compute the phase index (p), the remainder (r) is the

Nitpick: Please leave /* on it's own line.

> +        * part we'll try to acheive using the delays (d).
> +        */
> +       r = do_div(p, 360 / (1 << (ph->phase.width)));

Drop useless parenthesis please.

> +       d = DIV_ROUND_CLOSEST(r * period_ps,
> +                             360 * ph->delay_step_ps);
> +       d = min(d, PMASK(ph->delay.width));
> +
> +       meson_parm_write(clk->map, &ph->phase, p);
> +       meson_parm_write(clk->map, &ph->delay, d);
> +       return 0;
> +}

[PATCH v6 1/3] clk: meson: add emmc sub clock phase delay driver

[Jianxin Pan]

Hi Stephen,

Thanks for your review. 
Please see me comments below.

On 2018/11/4 11:02, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-11-01 09:30:53)
>> diff --git a/drivers/clk/meson/clk-phase-delay.c b/drivers/clk/meson/clk-phase-delay.c
>> new file mode 100644
>> index 0000000..83e74ed
>> --- /dev/null
>> +++ b/drivers/clk/meson/clk-phase-delay.c
>> @@ -0,0 +1,66 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + * Author: Jianxin Pan <jianxin.pan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include "clkc.h"
>> +
>> +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);
> 
> Nitpick: Do this after declaring variables because it splits a line.
OK. I will split the assignment into another line. Thank you.
> 
>> +       unsigned long period_ps, p, d;
>> +       int degrees;
>> +
>> +       p = meson_parm_read(clk->map, &ph->phase);
>> +       degrees = p * 360 / (1 << (ph->phase.width));
> 
> Nitpick: Remove useless parenthesis.
OK. I will remove them.
> 
>> +
>> +       period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
> 
> Is the cast necessary?
Yes, the cast can be droped. NSEC_PER_SEC is already defined wit type long.
> 
>> +                                clk_hw_get_rate(hw));
>> +
>> +       d = meson_parm_read(clk->map, &ph->delay);
>> +       degrees += d * ph->delay_step_ps * 360 / period_ps;
>> +       degrees %= 360;
>> +
>> +       return degrees;
>> +}
>> +
>> +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 long period_ps, d = 0, r;
>> +       u64 p;
>> +
>> +       p = degrees % 360;
> 
> We don't allow phase to be larger than 360 so this isn't needed.
OK, Thank you.
> 
>> +       period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
> 
> Drop the cast?
OK.
> 
>> +                                clk_hw_get_rate(hw));
>> +
>> +       /* First compute the phase index (p), the remainder (r) is the
> 
> Nitpick: Please leave /* on it's own line.
OK.
> 
>> +        * part we'll try to acheive using the delays (d).
>> +        */
>> +       r = do_div(p, 360 / (1 << (ph->phase.width)));
> 
> Drop useless parenthesis please.
OK, I will fix it. Thank you.
> 
>> +       d = DIV_ROUND_CLOSEST(r * period_ps,
>> +                             360 * ph->delay_step_ps);
>> +       d = min(d, PMASK(ph->delay.width));
>> +
>> +       meson_parm_write(clk->map, &ph->phase, p);
>> +       meson_parm_write(clk->map, &ph->delay, d);
>> +       return 0;
>> +}
> 
> .
> 

[PATCH v6 2/3] clk: meson: add DT documentation for emmc clock controller

[Jianxin Pan]

From: Yixun Lan <yixun.lan@amlogic.com>

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 <robh@kernel.org>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
---
 .../devicetree/bindings/clock/amlogic,mmc-clkc.txt | 39 ++++++++++++++++++++++
 include/dt-bindings/clock/amlogic,mmc-clkc.h       | 17 ++++++++++
 2 files changed, 56 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 0000000..0f518e6
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
@@ -0,0 +1,39 @@
+* 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"
+
+- reg: address of emmc sub clock register
+
+Example: Clock controller node:
+
+sd_mmc_c_clkc: clock-controller at 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>;
+};
+
+sd_emmc_b_clkc: clock-controller at 5000 {
+	compatible = "amlogic,axg-mmc-clkc", "syscon";
+	reg = <0x0 0x5000 0x0 0x4>;
+
+	#clock-cells = <1>;
+	clock-names = "clkin0", "clkin1";
+	clocks = <&clkc CLKID_SD_EMMC_B_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 0000000..162b949
--- /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 <yixun.lan@amlogic.com>
+ */
+
+#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
-- 
1.9.1

[PATCH v6 3/3] clk: meson: add sub MMC clock controller driver

[Jianxin Pan]

From: Yixun Lan <yixun.lan@amlogic.com>

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 <yixun.lan@amlogic.com>
Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
---
 drivers/clk/meson/Kconfig      |  10 ++
 drivers/clk/meson/Makefile     |   1 +
 drivers/clk/meson/clk-regmap.c |   1 -
 drivers/clk/meson/mmc-clkc.c   | 310 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 321 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/meson/mmc-clkc.c

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index efaa70f..6bb0d44 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"
+	select MFD_SYSCON
+	select COMMON_CLK_AMLOGIC
+	select COMMON_CLK_AMLOGIC_AUDIO
+	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 39ce566..31c16d5 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/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
index 305ee30..89cee4a 100644
--- a/drivers/clk/meson/clk-regmap.c
+++ b/drivers/clk/meson/clk-regmap.c
@@ -114,7 +114,6 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
 };
 
 /* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
-
 const struct clk_ops clk_regmap_divider_ops = {
 	.recalc_rate = clk_regmap_div_recalc_rate,
 	.round_rate = clk_regmap_div_round_rate,
diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
new file mode 100644
index 0000000..a3e4c91
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,310 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2017 Baylibre SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ * Author: Jianxin Pan <jianxin.pan@amlogic.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/clock/amlogic,mmc-clkc.h>
+
+#include "clkc.h"
+#include "clkc-audio.h"
+
+/* clock ID used by internal driver */
+#define CLKID_MMC_MUX			0
+
+#define   SD_EMMC_CLOCK		0
+#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,
+};
+
+struct meson_sclk_div_data  mmc_clkc_div_data = {
+	.div = {
+		.reg_off = SD_EMMC_CLOCK,
+		.shift   = (0),
+		.width   = (6),
+	},
+	.hi = {
+		.reg_off = 0,
+		.shift   = 0,
+		.width   = 0,
+	},
+};
+
+static struct meson_clk_phase_data mmc_clkc_core_phase = {
+	.ph = {
+		.reg_off	= SD_EMMC_CLOCK,
+		.shift	= 8,
+		.width	= 2,
+	}
+};
+
+static const struct mmc_clkc_data mmc_clkc_gx_data = {
+	.tx = {
+		.phase = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 10,
+			.width	= 2,
+		},
+		.delay = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 16,
+			.width	= 4,
+		},
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+	.rx = {
+		.phase = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 12,
+			.width	= 2,
+		},
+		.delay = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 20,
+			.width	= 4,
+		},
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+};
+
+static const struct mmc_clkc_data mmc_clkc_axg_data = {
+	.tx = {
+		.phase = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 10,
+			.width	= 2,
+		},
+		.delay = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 16,
+			.width	= 6,
+		},
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+	.rx = {
+		.phase = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 12,
+			.width	= 2,
+		},
+		.delay = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 22,
+			.width	= 6,
+		},
+		.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_clk_with_parent(struct device *dev, struct regmap *map,
+				  char *suffix, const struct clk_hw *hw,
+				  unsigned long flags,
+				  const struct clk_ops *ops, void *data)
+{
+	struct clk_init_data init;
+	struct clk_regmap *clk;
+	const char *parent_name = clk_hw_get_name(hw);
+
+	init.ops = ops;
+	init.flags = flags;
+	init.parent_names = &parent_name;
+	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);
+
+	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 *clk, *core;
+
+	/*cast to drop the const in match->data*/
+	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;
+
+	clk = mmc_clkc_register_mux(dev, map);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	onecell_data->hws[CLKID_MMC_MUX]		= &clk->hw,
+
+	clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
+						&clk->hw,
+						CLK_SET_RATE_PARENT,
+						&meson_sclk_div_ops,
+						&mmc_clkc_div_data);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	onecell_data->hws[CLKID_MMC_DIV]		= &clk->hw,
+
+	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
+						 &clk->hw,
+						 CLK_SET_RATE_PARENT,
+						 &meson_clk_phase_ops,
+						 &mmc_clkc_core_phase);
+	if (IS_ERR(core))
+		return PTR_ERR(core);
+	onecell_data->hws[CLKID_MMC_PHASE_CORE]		= &core->hw,
+
+	clk = mmc_clkc_register_clk_with_parent(dev, map, "rx",
+						&core->hw,  0,
+						&meson_clk_phase_delay_ops,
+						&data->rx);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	onecell_data->hws[CLKID_MMC_PHASE_RX]		= &clk->hw,
+
+	clk = mmc_clkc_register_clk_with_parent(dev, map, "tx",
+						&core->hw,  0,
+						&meson_clk_phase_delay_ops,
+						&data->tx);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	onecell_data->hws[CLKID_MMC_PHASE_TX]		= &clk->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);
+
+MODULE_DESCRIPTION("Amlogic AXG MMC clock driver");
+MODULE_AUTHOR("Jianxin Pan <jianxin.pan@amlogic.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v6 3/3] clk: meson: add sub MMC clock controller driver

[Martin Blumenstingl]

Hi Jianxin,

On Thu, Nov 1, 2018 at 5:31 PM Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>
> From: Yixun Lan <yixun.lan@amlogic.com>
>
> 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 <yixun.lan@amlogic.com>
> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
this looks good to me in general, some comments below

> ---
>  drivers/clk/meson/Kconfig      |  10 ++
>  drivers/clk/meson/Makefile     |   1 +
>  drivers/clk/meson/clk-regmap.c |   1 -
>  drivers/clk/meson/mmc-clkc.c   | 310 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 321 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/meson/mmc-clkc.c
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index efaa70f..6bb0d44 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"
> +       select MFD_SYSCON
> +       select COMMON_CLK_AMLOGIC
> +       select COMMON_CLK_AMLOGIC_AUDIO
> +       help
> +         Support for the MMC sub clock controller on Amlogic Meson Platform,
> +         which include S905 (GXBB, GXL), A113D/X (AXG) devices.
can you confirm that (in the future) we will be able to use this with
G12A and G12B as well?

> +         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 39ce566..31c16d5 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/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> index 305ee30..89cee4a 100644
> --- a/drivers/clk/meson/clk-regmap.c
> +++ b/drivers/clk/meson/clk-regmap.c
> @@ -114,7 +114,6 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>  };
>
>  /* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> -
unnecessary whitespace change, personally I would drop this file from the patch

>  const struct clk_ops clk_regmap_divider_ops = {
>         .recalc_rate = clk_regmap_div_recalc_rate,
>         .round_rate = clk_regmap_div_round_rate,
> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> new file mode 100644
> index 0000000..a3e4c91
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,310 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + * Author: Jianxin Pan <jianxin.pan@amlogic.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
> +
> +#include "clkc.h"
> +#include "clkc-audio.h"
> +
> +/* clock ID used by internal driver */
> +#define CLKID_MMC_MUX                  0
> +
> +#define   SD_EMMC_CLOCK                0
> +#define   CLK_DELAY_STEP_PS            200
> +#define   CLK_PHASE_STEP               30
> +#define   CLK_PHASE_POINT_NUM          (360 / CLK_PHASE_STEP)
CLK_PHASE_STEP and CLK_PHASE_POINT_NUM are unused
> +
> +#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,
> +};
> +
> +struct meson_sclk_div_data  mmc_clkc_div_data = {
can this be const?
also there are two whitespaces after meson_sclk_div_data

> +       .div = {
> +               .reg_off = SD_EMMC_CLOCK,
> +               .shift   = (0),
> +               .width   = (6),
> +       },
> +       .hi = {
> +               .reg_off = 0,
> +               .shift   = 0,
> +               .width   = 0,
> +       },
> +};
> +
> +static struct meson_clk_phase_data mmc_clkc_core_phase = {
> +       .ph = {
> +               .reg_off        = SD_EMMC_CLOCK,
> +               .shift  = 8,
> +               .width  = 2,
> +       }
> +};
> +
> +static const struct mmc_clkc_data mmc_clkc_gx_data = {
> +       .tx = {
> +               .phase = {
> +                       .reg_off        = SD_EMMC_CLOCK,
> +                       .shift  = 10,
> +                       .width  = 2,
> +               },
> +               .delay = {
> +                       .reg_off        = SD_EMMC_CLOCK,
> +                       .shift  = 16,
> +                       .width  = 4,
> +               },
> +               .delay_step_ps  = CLK_DELAY_STEP_PS,
> +       },
> +       .rx = {
> +               .phase = {
> +                       .reg_off        = SD_EMMC_CLOCK,
> +                       .shift  = 12,
> +                       .width  = 2,
> +               },
> +               .delay = {
> +                       .reg_off        = SD_EMMC_CLOCK,
> +                       .shift  = 20,
> +                       .width  = 4,
> +               },
> +               .delay_step_ps  = CLK_DELAY_STEP_PS,
> +       },
> +};
> +
> +static const struct mmc_clkc_data mmc_clkc_axg_data = {
> +       .tx = {
> +               .phase = {
> +                       .reg_off        = SD_EMMC_CLOCK,
> +                       .shift  = 10,
> +                       .width  = 2,
> +               },
> +               .delay = {
> +                       .reg_off        = SD_EMMC_CLOCK,
> +                       .shift  = 16,
> +                       .width  = 6,
> +               },
> +               .delay_step_ps  = CLK_DELAY_STEP_PS,
> +       },
> +       .rx = {
> +               .phase = {
> +                       .reg_off        = SD_EMMC_CLOCK,
> +                       .shift  = 12,
> +                       .width  = 2,
> +               },
> +               .delay = {
> +                       .reg_off        = SD_EMMC_CLOCK,
> +                       .shift  = 22,
> +                       .width  = 6,
> +               },
> +               .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);
use devm_kasprintf here? this will allow you to get rid of the kfree below
> +       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_clk_with_parent(struct device *dev, struct regmap *map,
> +                                 char *suffix, const struct clk_hw *hw,
> +                                 unsigned long flags,
> +                                 const struct clk_ops *ops, void *data)
> +{
> +       struct clk_init_data init;
> +       struct clk_regmap *clk;
> +       const char *parent_name = clk_hw_get_name(hw);
> +
> +       init.ops = ops;
> +       init.flags = flags;
> +       init.parent_names = &parent_name;
> +       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);
> +
> +       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 *clk, *core;
> +
> +       /*cast to drop the const in match->data*/
> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
can you declare the data variable as "const struct mmc_clkc_data
*data;" instead?

> +       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;
> +
> +       clk = mmc_clkc_register_mux(dev, map);
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +       onecell_data->hws[CLKID_MMC_MUX]                = &clk->hw,
> +
> +       clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
> +                                               &clk->hw,
> +                                               CLK_SET_RATE_PARENT,
> +                                               &meson_sclk_div_ops,
> +                                               &mmc_clkc_div_data);
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +       onecell_data->hws[CLKID_MMC_DIV]                = &clk->hw,
> +
> +       core = mmc_clkc_register_clk_with_parent(dev, map, "core",
> +                                                &clk->hw,
> +                                                CLK_SET_RATE_PARENT,
> +                                                &meson_clk_phase_ops,
> +                                                &mmc_clkc_core_phase);
> +       if (IS_ERR(core))
> +               return PTR_ERR(core);
> +       onecell_data->hws[CLKID_MMC_PHASE_CORE]         = &core->hw,
> +
> +       clk = mmc_clkc_register_clk_with_parent(dev, map, "rx",
> +                                               &core->hw,  0,
> +                                               &meson_clk_phase_delay_ops,
> +                                               &data->rx);
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +       onecell_data->hws[CLKID_MMC_PHASE_RX]           = &clk->hw,
> +
> +       clk = mmc_clkc_register_clk_with_parent(dev, map, "tx",
> +                                               &core->hw,  0,
> +                                               &meson_clk_phase_delay_ops,
> +                                               &data->tx);
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +       onecell_data->hws[CLKID_MMC_PHASE_TX]           = &clk->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);
> +
> +MODULE_DESCRIPTION("Amlogic AXG MMC clock driver");
> +MODULE_AUTHOR("Jianxin Pan <jianxin.pan@amlogic.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>

Regards
Martin

[PATCH v6 3/3] clk: meson: add sub MMC clock controller driver

[Jianxin Pan]

Hi Martin,

Thanks for the review and suggestion. Please see my comments below.

On 2018/11/2 2:16, Martin Blumenstingl wrote:
> Hi Jianxin,
> 
> On Thu, Nov 1, 2018 at 5:31 PM Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>>
>> From: Yixun Lan <yixun.lan@amlogic.com>
>>
>> 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 <yixun.lan@amlogic.com>
>> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
> this looks good to me in general, some comments below
> 
>> ---
>>  drivers/clk/meson/Kconfig      |  10 ++
>>  drivers/clk/meson/Makefile     |   1 +
>>  drivers/clk/meson/clk-regmap.c |   1 -
>>  drivers/clk/meson/mmc-clkc.c   | 310 +++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 321 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/clk/meson/mmc-clkc.c
>>
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index efaa70f..6bb0d44 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"
>> +       select MFD_SYSCON
>> +       select COMMON_CLK_AMLOGIC
>> +       select COMMON_CLK_AMLOGIC_AUDIO
>> +       help
>> +         Support for the MMC sub clock controller on Amlogic Meson Platform,
>> +         which include S905 (GXBB, GXL), A113D/X (AXG) devices.
> can you confirm that (in the future) we will be able to use this with
> G12A and G12B as well?
Yes, this part can be reused in G12A and G12B two. 
> 
>> +         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 39ce566..31c16d5 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/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
>> index 305ee30..89cee4a 100644
>> --- a/drivers/clk/meson/clk-regmap.c
>> +++ b/drivers/clk/meson/clk-regmap.c
>> @@ -114,7 +114,6 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>  };
>>
>>  /* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>> -
> unnecessary whitespace change, personally I would drop this file from the patch
OK, I will remove the unnecessary whitespace change here. 
Thanks for your time. 
> 
>>  const struct clk_ops clk_regmap_divider_ops = {
>>         .recalc_rate = clk_regmap_div_recalc_rate,
>>         .round_rate = clk_regmap_div_round_rate,
>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 0000000..a3e4c91
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,310 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + * Author: Jianxin Pan <jianxin.pan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
>> +
>> +#include "clkc.h"
>> +#include "clkc-audio.h"
>> +
>> +/* clock ID used by internal driver */
>> +#define CLKID_MMC_MUX                  0
>> +
>> +#define   SD_EMMC_CLOCK                0
>> +#define   CLK_DELAY_STEP_PS            200
>> +#define   CLK_PHASE_STEP               30
>> +#define   CLK_PHASE_POINT_NUM          (360 / CLK_PHASE_STEP)
> CLK_PHASE_STEP and CLK_PHASE_POINT_NUM are unused
YES, I will remove them.
>> +
>> +#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,
>> +};
>> +
>> +struct meson_sclk_div_data  mmc_clkc_div_data = {
> can this be const?
The member cached_div need to  be changed danymicly in clk ops.
In addition, there are more than one instances of the controller, so I will alloc them danymicly.
> also there are two whitespaces after meson_sclk_div_data
OK, I will remove it.
>> +       .div = {
>> +               .reg_off = SD_EMMC_CLOCK,
>> +               .shift   = (0),
>> +               .width   = (6),
>> +       },
>> +       .hi = {
>> +               .reg_off = 0,
>> +               .shift   = 0,
>> +               .width   = 0,
>> +       },
>> +};
>> +
>> +static struct meson_clk_phase_data mmc_clkc_core_phase = {
>> +       .ph = {
>> +               .reg_off        = SD_EMMC_CLOCK,
>> +               .shift  = 8,
>> +               .width  = 2,
>> +       }
>> +};
>> +
>> +static const struct mmc_clkc_data mmc_clkc_gx_data = {
>> +       .tx = {
>> +               .phase = {
>> +                       .reg_off        = SD_EMMC_CLOCK,
>> +                       .shift  = 10,
>> +                       .width  = 2,
>> +               },
>> +               .delay = {
>> +                       .reg_off        = SD_EMMC_CLOCK,
>> +                       .shift  = 16,
>> +                       .width  = 4,
>> +               },
>> +               .delay_step_ps  = CLK_DELAY_STEP_PS,
>> +       },
>> +       .rx = {
>> +               .phase = {
>> +                       .reg_off        = SD_EMMC_CLOCK,
>> +                       .shift  = 12,
>> +                       .width  = 2,
>> +               },
>> +               .delay = {
>> +                       .reg_off        = SD_EMMC_CLOCK,
>> +                       .shift  = 20,
>> +                       .width  = 4,
>> +               },
>> +               .delay_step_ps  = CLK_DELAY_STEP_PS,
>> +       },
>> +};
>> +
>> +static const struct mmc_clkc_data mmc_clkc_axg_data = {
>> +       .tx = {
>> +               .phase = {
>> +                       .reg_off        = SD_EMMC_CLOCK,
>> +                       .shift  = 10,
>> +                       .width  = 2,
>> +               },
>> +               .delay = {
>> +                       .reg_off        = SD_EMMC_CLOCK,
>> +                       .shift  = 16,
>> +                       .width  = 6,
>> +               },
>> +               .delay_step_ps  = CLK_DELAY_STEP_PS,
>> +       },
>> +       .rx = {
>> +               .phase = {
>> +                       .reg_off        = SD_EMMC_CLOCK,
>> +                       .shift  = 12,
>> +                       .width  = 2,
>> +               },
>> +               .delay = {
>> +                       .reg_off        = SD_EMMC_CLOCK,
>> +                       .shift  = 22,
>> +                       .width  = 6,
>> +               },
>> +               .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);
> use devm_kasprintf here? this will allow you to get rid of the kfree below
mmc_clkc_register_clk need to kfree(name) when it returns.
But devm_kasprintf will keep the memory until the device is removed.
Thanks for your time.
>> +       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_clk_with_parent(struct device *dev, struct regmap *map,
>> +                                 char *suffix, const struct clk_hw *hw,
>> +                                 unsigned long flags,
>> +                                 const struct clk_ops *ops, void *data)
>> +{
>> +       struct clk_init_data init;
>> +       struct clk_regmap *clk;
>> +       const char *parent_name = clk_hw_get_name(hw);
>> +
>> +       init.ops = ops;
>> +       init.flags = flags;
>> +       init.parent_names = &parent_name;
>> +       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);
>> +
>> +       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 *clk, *core;
>> +
>> +       /*cast to drop the const in match->data*/
>> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> can you declare the data variable as "const struct mmc_clkc_data
> *data;" instead?
&data->rx and &data->tx are saved as 'data' member in struct clk_regmap. And it's not a const variable.
struct clk_regmap {
	struct clk_hw	hw;
	struct regmap	*map;
	void		*data;
};
mmc_clkc_register_clk_with_parent(..., void *data) is shared with other clocks,and the *data for mmc_clkc_div_data is not const.
> 
>> +       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;
>> +
>> +       clk = mmc_clkc_register_mux(dev, map);
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +       onecell_data->hws[CLKID_MMC_MUX]                = &clk->hw,
>> +
>> +       clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
>> +                                               &clk->hw,
>> +                                               CLK_SET_RATE_PARENT,
>> +                                               &meson_sclk_div_ops,
>> +                                               &mmc_clkc_div_data);
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +       onecell_data->hws[CLKID_MMC_DIV]                = &clk->hw,
>> +
>> +       core = mmc_clkc_register_clk_with_parent(dev, map, "core",
>> +                                                &clk->hw,
>> +                                                CLK_SET_RATE_PARENT,
>> +                                                &meson_clk_phase_ops,
>> +                                                &mmc_clkc_core_phase);
>> +       if (IS_ERR(core))
>> +               return PTR_ERR(core);
>> +       onecell_data->hws[CLKID_MMC_PHASE_CORE]         = &core->hw,
>> +
>> +       clk = mmc_clkc_register_clk_with_parent(dev, map, "rx",
>> +                                               &core->hw,  0,
>> +                                               &meson_clk_phase_delay_ops,
>> +                                               &data->rx);
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +       onecell_data->hws[CLKID_MMC_PHASE_RX]           = &clk->hw,
>> +
>> +       clk = mmc_clkc_register_clk_with_parent(dev, map, "tx",
>> +                                               &core->hw,  0,
>> +                                               &meson_clk_phase_delay_ops,
>> +                                               &data->tx);
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +       onecell_data->hws[CLKID_MMC_PHASE_TX]           = &clk->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);
>> +
>> +MODULE_DESCRIPTION("Amlogic AXG MMC clock driver");
>> +MODULE_AUTHOR("Jianxin Pan <jianxin.pan@amlogic.com>");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.9.1
>>
> 
> Regards
> Martin
> 
> .
> 

[PATCH v6 3/3] clk: meson: add sub MMC clock controller driver

[jbrunet at baylibre.com]

On Fri, 2018-11-02 at 00:30 +0800, Jianxin Pan wrote:
> +struct meson_sclk_div_data  mmc_clkc_div_data = {
> +       .div = {
> +               .reg_off = SD_EMMC_CLOCK,
> +               .shift   = (0),
> +               .width   = (6),
> +       },
> +       .hi = {
> +               .reg_off = 0,
> +               .shift   = 0,
> +               .width   = 0,
> +       },
> +};

Jianxin,

When replying to v5: https://patchwork.kernel.org/patch/10646723/#22288117
I think I have clearly explained that:
a. sclk needs some change you want to use it for the eMMC (not done )
b. you can't declare sclk statically like that since there is cached data in
it and this would forbib multiple instance of the controller which is not
acceptable for this pariticular controller

This is just not adressed in your series.

Also some comments from Martin about useless definition were already given on
past version.

Seeing this, I did not review the rest of series.
Please make sure you have addressed all the comments of past reviews before
reposting. It is OK to ask questions if things are unclear.

Jerome