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

[PATCH v5 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 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

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 |  31 +++
 drivers/clk/meson/Kconfig                          |  10 +
 drivers/clk/meson/Makefile                         |   3 +-
 drivers/clk/meson/clk-phase-delay.c                |  79 ++++++
 drivers/clk/meson/clk-regmap.c                     |  27 +-
 drivers/clk/meson/clk-regmap.h                     |   1 +
 drivers/clk/meson/clkc.h                           |  13 +
 drivers/clk/meson/mmc-clkc.c                       | 296 +++++++++++++++++++++
 include/dt-bindings/clock/amlogic,mmc-clkc.h       |  17 ++
 9 files changed, 475 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 v5 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 | 79 +++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/clkc.h            | 13 ++++++
 3 files changed, 93 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..b9573a7
--- /dev/null
+++ b/drivers/clk/meson/clk-phase-delay.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2017 Baylibre SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#include <linux/clk-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;
+	u32 val;
+
+	regmap_read(clk->map, ph->phase.reg_off, &val);
+	p = PARM_GET(ph->phase.width, ph->phase.shift, val);
+	degrees = p * 360 / (1 << (ph->phase.width));
+
+	period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
+				 clk_hw_get_rate(hw));
+
+	d = PARM_GET(ph->delay.width, ph->delay.shift, val);
+	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, ph->delay.reg_off, &val);
+	val = PARM_SET(ph->phase.width, ph->phase.shift, val, phase);
+	val = PARM_SET(ph->delay.width, ph->delay.shift, val, delay);
+	regmap_write(clk->map, ph->delay.reg_off, 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 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_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 6b96d55..3309d78 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 (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;
@@ -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 v5 1/3] clk: meson: add emmc sub clock phase delay driver

[Stephen Boyd]

Quoting Jianxin Pan (2018-10-17 22:07:23)
> diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
> index 6b96d55..3309d78 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 (struct meson_clk_phase_delay_data *)clk->data;

Is data a void *? If so, please drop the useless cast.

> +}
> +
>  /* clk_ops */
>  extern const struct clk_ops meson_clk_pll_ro_ops;
>  extern const struct clk_ops meson_clk_pll_ops;

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

[Jerome Brunet]

On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
> 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 | 79 +++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/clkc.h            | 13 ++++++
>  3 files changed, 93 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..b9573a7
> --- /dev/null
> +++ b/drivers/clk/meson/clk-phase-delay.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/clk-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;
> +	u32 val;
> +
> +	regmap_read(clk->map, ph->phase.reg_off, &val);
> +	p = PARM_GET(ph->phase.width, ph->phase.shift, val);

there is an helper for this: meson_parm_read()

> +	degrees = p * 360 / (1 << (ph->phase.width));
> +
> +	period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
> +				 clk_hw_get_rate(hw));
> +
> +	d = PARM_GET(ph->delay.width, ph->delay.shift, val);
> +	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)

This helper function is a bit overkill.
simply call meson_parm_write() twice in meson_clk_phase_delay_set_phase().

> +{
> +	struct meson_clk_phase_delay_data *ph = clk->data;
> +	u32 val;
> +
> +	regmap_read(clk->map, ph->delay.reg_off, &val);
> +	val = PARM_SET(ph->phase.width, ph->phase.shift, val, phase);
> +	val = PARM_SET(ph->delay.width, ph->delay.shift, val, delay);
> +	regmap_write(clk->map, ph->delay.reg_off, 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 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_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 6b96d55..3309d78 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 (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;
> @@ -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 */

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

[Jianxin Pan]

On 2018/10/24 16:58, Jerome Brunet wrote:
> On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
>> From: Yixun Lan <yixun.lan@amlogic.com>
[...]
>>
>> --- /dev/null
>> +++ b/drivers/clk/meson/clk-phase-delay.c
>> @@ -0,0 +1,79 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk-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;
>> +	u32 val;
>> +
>> +	regmap_read(clk->map, ph->phase.reg_off, &val);
>> +	p = PARM_GET(ph->phase.width, ph->phase.shift, val);
> 
> there is an helper for this: meson_parm_read()
OK. I will use meson_parm_read instead. Thanks for your review.

The result of regmap_read is shared by two PARM_GETs: phase and delay.
There will be one more register read when change to meson_parm_read, but I don't think it matters.

> 
>> +	degrees = p * 360 / (1 << (ph->phase.width));
>> +
>> +	period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
>> +				 clk_hw_get_rate(hw));
>> +
>> +	d = PARM_GET(ph->delay.width, ph->delay.shift, val);
>> +	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)
> 
> This helper function is a bit overkill.
> simply call meson_parm_write() twice in meson_clk_phase_delay_set_phase().
OK. I will use meson_parm_write() instead.
With meson_parm_write(), there will be one more register read and write
> 
>> +{
>> +	struct meson_clk_phase_delay_data *ph = clk->data;
>> +	u32 val;
>> +
>> +	regmap_read(clk->map, ph->delay.reg_off, &val);
>> +	val = PARM_SET(ph->phase.width, ph->phase.shift, val, phase);
>> +	val = PARM_SET(ph->delay.width, ph->delay.shift, val, delay);
>> +	regmap_write(clk->map, ph->delay.reg_off, 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 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_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 6b96d55..3309d78 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 (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;
>> @@ -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 */
> 
> 
> .
> 

[PATCH v5 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 | 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 0000000..9e6d343
--- /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 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>;
+};
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 v5 2/3] clk: meson: add DT documentation for emmc clock controller

[Stephen Boyd]

Quoting Jianxin Pan (2018-10-17 22:07:24)
> 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..9e6d343
> --- /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 :

The example only has one node. Can you add two nodes?

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

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

[Jianxin Pan]

On 2018/10/19 1:08, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-17 22:07:24)
>> 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..9e6d343
>> --- /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 :
> 
> The example only has one node. Can you add two nodes?
OK. This clock is used by nand and emmc. I will add a new example for emmc too.
Thank you for your review.
> 
>> +- 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 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>;
>> +};
> 
> .
> 

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

[Stephen Boyd]

Quoting Jianxin Pan (2018-10-19 08:50:08)
> On 2018/10/19 1:08, Stephen Boyd wrote:
> > Quoting Jianxin Pan (2018-10-17 22:07:24)
> >> 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..9e6d343
> >> --- /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 :
> > 
> > The example only has one node. Can you add two nodes?
> OK. This clock is used by nand and emmc. I will add a new example for emmc too.
> Thank you for your review.

Maybe I misunderstand. I thought the clk controller was two nodes, but
it isn't? This wording is trying to explain what a consumer should look
like?

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

[Jianxin Pan]

Hi Stephen,

Thanks for the fully review, we really appreciate your time.

Please see my comments below. 

On 2018/10/20 2:04, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-19 08:50:08)
>> On 2018/10/19 1:08, Stephen Boyd wrote:
>>> Quoting Jianxin Pan (2018-10-17 22:07:24)
>>>> 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..9e6d343
>>>> --- /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 :
>>>
>>> The example only has one node. Can you add two nodes?
>> OK. This clock is used by nand and emmc. I will add a new example for emmc too.
>> Thank you for your review.
> 
> Maybe I misunderstand. I thought the clk controller was two nodes, but
> it isn't? This wording is trying to explain what a consumer should look
> like?
> 
Yes.There is another clk controller.  I will add it in the next version.
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>;                 
};
sd_emmc_c_clkc is for nadn and mmc portC.
sd_emmc_b_clkc is for mmc portB.
> .
> 

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

[Jerome Brunet]

On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
> 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 | 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 0000000..9e6d343
> --- /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.

I get why Stephen is confused by your description, I am too. The example
contradict the documentation.

The  documentation above says that the parent node should be a syscon with the
mmc register space.

But your example shows this in the node itself.

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

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

[Yixun Lan]

Hi Jerome, Jianxin:

see my comments

On 10:58 Wed 24 Oct     , Jerome Brunet wrote:
> On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
> > 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 | 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 0000000..9e6d343
> > --- /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.
> 
> I get why Stephen is confused by your description, I am too. The example
> contradict the documentation.
> 
> The  documentation above says that the parent node should be a syscon with the
> mmc register space.
> 
> But your example shows this in the node itself.
> 

yes, I think the documentation need to be fixed

for the final solution, we decide to make 'mmc-clkc' an independent node
instead of being a sub-node of 'mmc', so both of them may exist in parallel..

the DT part may like this:

			sd_emmc_c_clkc: clock-controller at 7000 {
				compatible = "amlogic,axg-mmc-clkc", "syscon";
				reg = <0x0 0x7000 0x0 0x4>;
				...
			};

			sd_emmc_c: mmc at 7000 {
				compatible = "amlogic,axg-mmc";
				reg = <0x0 0x7000 0x0 0x800>;
				...
			};


> > +
> > +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>;
> > +};
> > 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
> 
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

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

[Jianxin Pan]

On 2018/10/25 15:29, Yixun Lan wrote:
> Hi Jerome, Jianxin:
> 
> see my comments
> 
> On 10:58 Wed 24 Oct     , Jerome Brunet wrote:
>> On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
>>> 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 | 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 0000000..9e6d343
>>> --- /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.
>>
>> I get why Stephen is confused by your description, I am too. The example
>> contradict the documentation.
>>
>> The  documentation above says that the parent node should be a syscon with the
>> mmc register space.
>>
>> But your example shows this in the node itself.
>>
> 
> yes, I think the documentation need to be fixed
ok, Thankyou. I will fix it in the next version.
> 
> for the final solution, we decide to make 'mmc-clkc' an independent node
> instead of being a sub-node of 'mmc', so both of them may exist in parallel..
> 
> the DT part may like this:
> 
> 			sd_emmc_c_clkc: clock-controller at 7000 {
> 				compatible = "amlogic,axg-mmc-clkc", "syscon";
> 				reg = <0x0 0x7000 0x0 0x4>;
> 				...
> 			};
> 
> 			sd_emmc_c: mmc at 7000 {
> 				compatible = "amlogic,axg-mmc";
> 				reg = <0x0 0x7000 0x0 0x800>;
> 				...
> 			};
> 
> 
>>> +
>>> +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>;
>>> +};
>>> 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
>>
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 

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

[Stephen Boyd]

Quoting Yixun Lan (2018-10-25 00:29:15)
> yes, I think the documentation need to be fixed
> 
> for the final solution, we decide to make 'mmc-clkc' an independent node
> instead of being a sub-node of 'mmc', so both of them may exist in parallel..
> 
> the DT part may like this:
> 
>                         sd_emmc_c_clkc: clock-controller at 7000 {
>                                 compatible = "amlogic,axg-mmc-clkc", "syscon";
>                                 reg = <0x0 0x7000 0x0 0x4>;
>                                 ...
>                         };
> 
>                         sd_emmc_c: mmc at 7000 {
>                                 compatible = "amlogic,axg-mmc";
>                                 reg = <0x0 0x7000 0x0 0x800>;
>                                 ...
>                         };

That's improper usage of DT. We don't want two devices at the same
register offset.

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

[Jianxin Pan]

Hi Stephen,

Thank you for the review.

On 2018/11/4 11:04, Stephen Boyd wrote:
> Quoting Yixun Lan (2018-10-25 00:29:15)
>> yes, I think the documentation need to be fixed
>>
>> for the final solution, we decide to make 'mmc-clkc' an independent node
>> instead of being a sub-node of 'mmc', so both of them may exist in parallel..
>>
>> the DT part may like this:
>>
>>                         sd_emmc_c_clkc: clock-controller at 7000 {
>>                                 compatible = "amlogic,axg-mmc-clkc", "syscon";
>>                                 reg = <0x0 0x7000 0x0 0x4>;
>>                                 ...
>>                         };
>>
>>                         sd_emmc_c: mmc at 7000 {
>>                                 compatible = "amlogic,axg-mmc";
>>                                 reg = <0x0 0x7000 0x0 0x800>;
>>                                 ...
>>                         };
> 
> That's improper usage of DT. We don't want two devices at the same
> register offset.
sd_emmc_c_clkc is shared by nand and sd_emmc_c controller, and this clock is part of the MMC controller's register space.
 
The idea of adding the clock-controller at 7000is introduced during the discussion in the NAND driver mainline effort:
https://lkml.kernel.org/r/20180628090034.0637a062 at xps13
> 
> .
> 

[PATCH v5 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 |  27 +++-
 drivers/clk/meson/clk-regmap.h |   1 +
 drivers/clk/meson/mmc-clkc.c   | 296 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 334 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..8b8ccbc 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 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..f96314d 100644
--- a/drivers/clk/meson/clk-regmap.c
+++ b/drivers/clk/meson/clk-regmap.c
@@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
 				  clk_div_mask(div->width) << div->shift, val);
 };
 
-/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
+static void clk_regmap_div_init(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(clk->map, div->offset, &val);
+	if (ret)
+		return;
 
+	val &= (clk_div_mask(div->width) << div->shift);
+	if (!val)
+		regmap_update_bits(clk->map, div->offset,
+				   clk_div_mask(div->width) << div->shift,
+				   clk_div_mask(div->width));
+}
+
+/* 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,
@@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
 };
 EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
 
+const struct clk_ops clk_regmap_divider_with_init_ops = {
+	.recalc_rate = clk_regmap_div_recalc_rate,
+	.round_rate = clk_regmap_div_round_rate,
+	.set_rate = clk_regmap_div_set_rate,
+	.init = clk_regmap_div_init,
+};
+EXPORT_SYMBOL_GPL(clk_regmap_divider_with_init_ops);
+
 const struct clk_ops clk_regmap_divider_ro_ops = {
 	.recalc_rate = clk_regmap_div_recalc_rate,
 	.round_rate = clk_regmap_div_round_rate,
diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
index ed2d434..0ab7d37 100644
--- a/drivers/clk/meson/clk-regmap.h
+++ b/drivers/clk/meson/clk-regmap.h
@@ -109,5 +109,6 @@ struct clk_regmap_mux_data {
 
 extern const struct clk_ops clk_regmap_mux_ops;
 extern const struct clk_ops clk_regmap_mux_ro_ops;
+extern const struct clk_ops clk_regmap_divider_with_init_ops;
 
 #endif /* __CLK_REGMAP_H */
diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
new file mode 100644
index 0000000..5555e3f
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2017 Baylibre SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/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_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_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 char *parent,
+				  unsigned long flags,
+				  const struct clk_ops *ops, void *data)
+{
+	struct clk_init_data init;
+	struct clk_regmap *clk;
+
+	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);
+
+	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_clk_with_parent(dev, map, "div",
+						clk_hw_get_name(&mux->hw),
+						CLK_SET_RATE_PARENT,
+						&clk_regmap_divider_with_init_ops,
+						&mmc_clkc_div_data);
+	if (IS_ERR(div))
+		return PTR_ERR(div);
+
+	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
+						 clk_hw_get_name(&div->hw),
+						 CLK_SET_RATE_PARENT,
+						 &meson_clk_phase_ops,
+						 &mmc_clkc_core_phase);
+	if (IS_ERR(core))
+		return PTR_ERR(core);
+
+	rx = mmc_clkc_register_clk_with_parent(dev, map, "rx",
+					       clk_hw_get_name(&core->hw),  0,
+					       &meson_clk_phase_delay_ops,
+					       &data->rx);
+	if (IS_ERR(rx))
+		return PTR_ERR(rx);
+
+	tx = mmc_clkc_register_clk_with_parent(dev, map, "tx",
+					       clk_hw_get_name(&core->hw),  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);
-- 
1.9.1

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

[Stephen Boyd]

Quoting Jianxin Pan (2018-10-17 22:07:25)
> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> index 305ee30..f96314d 100644
> --- a/drivers/clk/meson/clk-regmap.c
> +++ b/drivers/clk/meson/clk-regmap.c
> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>                                   clk_div_mask(div->width) << div->shift, val);
>  };
>  
> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> +static void clk_regmap_div_init(struct clk_hw *hw)
> +{
> +       struct clk_regmap *clk = to_clk_regmap(hw);
> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> +       unsigned int val;
> +       int ret;
> +
> +       ret = regmap_read(clk->map, div->offset, &val);
> +       if (ret)
> +               return;
>  
> +       val &= (clk_div_mask(div->width) << div->shift);
> +       if (!val)
> +               regmap_update_bits(clk->map, div->offset,
> +                                  clk_div_mask(div->width) << div->shift,
> +                                  clk_div_mask(div->width));
> +}
> +
> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */

We should add a patch to rename the symbol for qcom, i.e.
qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
should be meson_clk_regmap_div_ro_ops.

Or we should just give up and squash the regmap implementations together
into a new clk_regmap set of ops.

>  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..5555e3f
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/clk.h>

clk-provider.h instead of 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>
> +
> +
[...]
> +
> +static struct clk_regmap *
> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
> +                                 char *suffix, const char *parent,
> +                                 unsigned long flags,
> +                                 const struct clk_ops *ops, void *data)
> +{
> +       struct clk_init_data init;
> +       struct clk_regmap *clk;
> +
> +       init.ops = ops;
> +       init.flags = flags;
> +       init.parent_names = (const char* const []){ parent, };

Can't we just assign &parent here?

> +       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 *mux, *div, *core, *rx, *tx;
> +
> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);

Nitpick: Drop the cast.

> +       if (!data)
> +               return -ENODEV;
> +
> +       map = syscon_node_to_regmap(dev->of_node);

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

[Jianxin Pan]

On 2018/10/19 1:13, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-17 22:07:25)
>> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
>> index 305ee30..f96314d 100644
>> --- a/drivers/clk/meson/clk-regmap.c
>> +++ b/drivers/clk/meson/clk-regmap.c
>> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>                                   clk_div_mask(div->width) << div->shift, val);
>>  };
>>  
>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>> +static void clk_regmap_div_init(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap *clk = to_clk_regmap(hw);
>> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       ret = regmap_read(clk->map, div->offset, &val);
>> +       if (ret)
>> +               return;
>>  
>> +       val &= (clk_div_mask(div->width) << div->shift);
>> +       if (!val)
>> +               regmap_update_bits(clk->map, div->offset,
>> +                                  clk_div_mask(div->width) << div->shift,
>> +                                  clk_div_mask(div->width));
>> +}
>> +
>> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> 
> We should add a patch to rename the symbol for qcom, i.e.
> qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
> should be meson_clk_regmap_div_ro_ops.
"/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */"
This comment is not introduced in this patch.
I followed the naming style in this file and add clk_regmap_divider_with_init_ops.

@Jerome? What's your suggestion about this?
> 
> Or we should just give up and squash the regmap implementations together
> into a new clk_regmap set of ops.
> 
>>  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..5555e3f
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,296 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk.h>
> 
> clk-provider.h instead of clk.h?
OK.
> 
>> +#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>
>> +
>> +
> [...]
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
>> +                                 char *suffix, const char *parent,
>> +                                 unsigned long flags,
>> +                                 const struct clk_ops *ops, void *data)
>> +{
>> +       struct clk_init_data init;
>> +       struct clk_regmap *clk;
>> +
>> +       init.ops = ops;
>> +       init.flags = flags;
>> +       init.parent_names = (const char* const []){ parent, };
> 
> Can't we just assign &parent here?
OK. I will fix it in the next version.
> 
>> +       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 *mux, *div, *core, *rx, *tx;
>> +
>> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> 
> Nitpick: Drop the cast.
OK. I will drop it.
Thanks for the review.

> 
>> +       if (!data)
>> +               return -ENODEV;
>> +
>> +       map = syscon_node_to_regmap(dev->of_node);
> 
> .
> 

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

[Stephen Boyd]

Quoting Jianxin Pan (2018-10-19 09:12:53)
> On 2018/10/19 1:13, Stephen Boyd wrote:
> > Quoting Jianxin Pan (2018-10-17 22:07:25)
> >> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> >> index 305ee30..f96314d 100644
> >> --- a/drivers/clk/meson/clk-regmap.c
> >> +++ b/drivers/clk/meson/clk-regmap.c
> >> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> >>                                   clk_div_mask(div->width) << div->shift, val);
> >>  };
> >>  
> >> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> >> +static void clk_regmap_div_init(struct clk_hw *hw)
> >> +{
> >> +       struct clk_regmap *clk = to_clk_regmap(hw);
> >> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> >> +       unsigned int val;
> >> +       int ret;
> >> +
> >> +       ret = regmap_read(clk->map, div->offset, &val);
> >> +       if (ret)
> >> +               return;
> >>  
> >> +       val &= (clk_div_mask(div->width) << div->shift);
> >> +       if (!val)
> >> +               regmap_update_bits(clk->map, div->offset,
> >> +                                  clk_div_mask(div->width) << div->shift,
> >> +                                  clk_div_mask(div->width));
> >> +}
> >> +
> >> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > 
> > We should add a patch to rename the symbol for qcom, i.e.
> > qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
> > should be meson_clk_regmap_div_ro_ops.
> "/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */"
> This comment is not introduced in this patch.
> I followed the naming style in this file and add clk_regmap_divider_with_init_ops.
> 
> @Jerome? What's your suggestion about this?

Yes you don't need to fix anything in this series. Just saying that in
the future we should work on cleaning this up.

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

[Jianxin Pan]

On 2018/10/20 2:03, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-19 09:12:53)
>> On 2018/10/19 1:13, Stephen Boyd wrote:
>>> Quoting Jianxin Pan (2018-10-17 22:07:25)
>>>> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
>>>> index 305ee30..f96314d 100644
>>>> --- a/drivers/clk/meson/clk-regmap.c
>>>> +++ b/drivers/clk/meson/clk-regmap.c
>>>> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>>>                                   clk_div_mask(div->width) << div->shift, val);
>>>>  };
>>>>  
>>>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>> +{
>>>> +       struct clk_regmap *clk = to_clk_regmap(hw);
>>>> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>>> +       unsigned int val;
>>>> +       int ret;
>>>> +
>>>> +       ret = regmap_read(clk->map, div->offset, &val);
>>>> +       if (ret)
>>>> +               return;
>>>>  
>>>> +       val &= (clk_div_mask(div->width) << div->shift);
>>>> +       if (!val)
>>>> +               regmap_update_bits(clk->map, div->offset,
>>>> +                                  clk_div_mask(div->width) << div->shift,
>>>> +                                  clk_div_mask(div->width));
>>>> +}
>>>> +
>>>> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>>>
>>> We should add a patch to rename the symbol for qcom, i.e.
>>> qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
>>> should be meson_clk_regmap_div_ro_ops.
>> "/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */"
>> This comment is not introduced in this patch.
>> I followed the naming style in this file and add clk_regmap_divider_with_init_ops.
>>
>> @Jerome? What's your suggestion about this?
> 
> Yes you don't need to fix anything in this series. Just saying that in
> the future we should work on cleaning this up.
> 
OK. Thank you!
> .
> 

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

[Jerome Brunet]

On Fri, 2018-10-19 at 11:03 -0700, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-19 09:12:53)
> > On 2018/10/19 1:13, Stephen Boyd wrote:
> > > Quoting Jianxin Pan (2018-10-17 22:07:25)
> > > > diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> > > > index 305ee30..f96314d 100644
> > > > --- a/drivers/clk/meson/clk-regmap.c
> > > > +++ b/drivers/clk/meson/clk-regmap.c
> > > > @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> > > >                                   clk_div_mask(div->width) << div->shift, val);
> > > >  };
> > > >  
> > > > -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > +{
> > > > +       struct clk_regmap *clk = to_clk_regmap(hw);
> > > > +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > > +       unsigned int val;
> > > > +       int ret;
> > > > +
> > > > +       ret = regmap_read(clk->map, div->offset, &val);
> > > > +       if (ret)
> > > > +               return;
> > > >  
> > > > +       val &= (clk_div_mask(div->width) << div->shift);
> > > > +       if (!val)
> > > > +               regmap_update_bits(clk->map, div->offset,
> > > > +                                  clk_div_mask(div->width) << div->shift,
> > > > +                                  clk_div_mask(div->width));
> > > > +}
> > > > +
> > > > +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > > 
> > > We should add a patch to rename the symbol for qcom, i.e.
> > > qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
> > > should be meson_clk_regmap_div_ro_ops.
> > 
> > "/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */"
> > This comment is not introduced in this patch.
> > I followed the naming style in this file and add clk_regmap_divider_with_init_ops.
> > 
> > @Jerome? What's your suggestion about this?
> 
> Yes you don't need to fix anything in this series. Just saying that in
> the future we should work on cleaning this up.


Well, first, I wonder why such a change ends up in a patch that is supposed to
add a controller.

If such a change was really required to implement a generic div (which I doubt)
it would need to be in separate with clear explanation.

Stephen,
I agree at some point we should squash the different regmap implementations and
provide generic (enough) implementation. There is not only qcom and meson, some
other controllers are redefining regmap ops and I bet driver outside of
drivers/clk/* could use a generic implementation as well.

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

[Jianxin Pan]

On 2018/10/19 1:13, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-17 22:07:25)
[...]
>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 0000000..5555e3f
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,296 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk.h>
> 
> clk-provider.h instead of clk.h?>
Maybe we need to keep clk.h
devm_clk_get() is called in mmc_clkc_register_mux() to get parent in from DTS.
I'm sorry to miss this in previous reply.
>> +#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>
[...]> 
> .
> 

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

[Stephen Boyd]

Quoting Jianxin Pan (2018-10-23 23:29:24)
> On 2018/10/19 1:13, Stephen Boyd wrote:
> > Quoting Jianxin Pan (2018-10-17 22:07:25)
> [...]
> >> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> >> new file mode 100644
> >> index 0000000..5555e3f
> >> --- /dev/null
> >> +++ b/drivers/clk/meson/mmc-clkc.c
> >> @@ -0,0 +1,296 @@
> >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> +/*
> >> + * Amlogic Meson MMC Sub Clock Controller Driver
> >> + *
> >> + * Copyright (c) 2017 Baylibre SAS.
> >> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> >> + *
> >> + * Copyright (c) 2018 Amlogic, inc.
> >> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> >> + */
> >> +
> >> +#include <linux/clk.h>
> > 
> > clk-provider.h instead of clk.h?>
> Maybe we need to keep clk.h
> devm_clk_get() is called in mmc_clkc_register_mux() to get parent in from DTS.
> I'm sorry to miss this in previous reply.

OK. But also include clk-provider.h if provider APIs are being used
here.

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

[Jianxin Pan]

On 2018/10/24 16:47, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-23 23:29:24)
>> On 2018/10/19 1:13, Stephen Boyd wrote:
>>> Quoting Jianxin Pan (2018-10-17 22:07:25)
>> [...]
>>>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>>>> new file mode 100644
>>>> index 0000000..5555e3f
>>>> --- /dev/null
>>>> +++ b/drivers/clk/meson/mmc-clkc.c
>>>> @@ -0,0 +1,296 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>> +/*
>>>> + * Amlogic Meson MMC Sub Clock Controller Driver
>>>> + *
>>>> + * Copyright (c) 2017 Baylibre SAS.
>>>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>>>> + *
>>>> + * Copyright (c) 2018 Amlogic, inc.
>>>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>
>>> clk-provider.h instead of clk.h?>
>> Maybe we need to keep clk.h
>> devm_clk_get() is called in mmc_clkc_register_mux() to get parent in from DTS.
>> I'm sorry to miss this in previous reply.
> 
> OK. But also include clk-provider.h if provider APIs are being used
> here.
OK. I will add it. Thank you.
> 
> .
> 

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

[Jerome Brunet]

On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan 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>
> ---
>  drivers/clk/meson/Kconfig      |  10 ++
>  drivers/clk/meson/Makefile     |   1 +
>  drivers/clk/meson/clk-regmap.c |  27 +++-
>  drivers/clk/meson/clk-regmap.h |   1 +
>  drivers/clk/meson/mmc-clkc.c   | 296 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 334 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..8b8ccbc 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

COMMON_CLK_AMLOGIC is not something that is manually enabled
You should select it, not depends on it. Have a look at how it is done for the
other controller (like in v4.19)

> +	select MFD_SYSCON
> +	select REGMAP

this is already selected by COMMON_CLK_AMLOGIC, please drop this.

> +	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..f96314d 100644
> --- a/drivers/clk/meson/clk-regmap.c
> +++ b/drivers/clk/meson/clk-regmap.c
> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>  				  clk_div_mask(div->width) << div->shift, val);
>  };
>  
> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> +static void clk_regmap_div_init(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(clk->map, div->offset, &val);
> +	if (ret)
> +		return;
>  
> +	val &= (clk_div_mask(div->width) << div->shift);
> +	if (!val)
> +		regmap_update_bits(clk->map, div->offset,
> +				   clk_div_mask(div->width) << div->shift,
> +				   clk_div_mask(div->width));

This is wrong for several reasons:
* You should hard code the initial value in the driver.
* If shift is not 0, I doubt this will give the expected result.

> +}

Please drop this. 

> +
> +/* 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,
> @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>  };
>  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
>  
> +const struct clk_ops clk_regmap_divider_with_init_ops = {
> +	.recalc_rate = clk_regmap_div_recalc_rate,
> +	.round_rate = clk_regmap_div_round_rate,
> +	.set_rate = clk_regmap_div_set_rate,
> +	.init = clk_regmap_div_init,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_divider_with_init_ops);

... and this.

> +
>  const struct clk_ops clk_regmap_divider_ro_ops = {
>  	.recalc_rate = clk_regmap_div_recalc_rate,
>  	.round_rate = clk_regmap_div_round_rate,
> diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
> index ed2d434..0ab7d37 100644
> --- a/drivers/clk/meson/clk-regmap.h
> +++ b/drivers/clk/meson/clk-regmap.h
> @@ -109,5 +109,6 @@ struct clk_regmap_mux_data {
>  
>  extern const struct clk_ops clk_regmap_mux_ops;
>  extern const struct clk_ops clk_regmap_mux_ro_ops;
> +extern const struct clk_ops clk_regmap_divider_with_init_ops;

... and this as well.

There is no reason to to init the divider to something other than what it is
already when we register the clock controller.

If your consumer needs the clocks to be initialised, it should call
clk_set_rate()

>  
>  #endif /* __CLK_REGMAP_H */
> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> new file mode 100644
> index 0000000..5555e3f
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/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_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_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 char *parent,

I would prefer if you passed the parent clk_hw pointer and call
clk_hw_get_name() in here

> +				  unsigned long flags,
> +				  const struct clk_ops *ops, void *data)
> +{
> +	struct clk_init_data init;
> +	struct clk_regmap *clk;
> +
> +	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);
> +
> +	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;

You really don't need all these local variables ( I think I already pointed this
out in past reviews ...)

You can keep one struct clk_regmap *clk and store the clk->hw in the onecell
data right after registering the clock.


> +
> +	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_clk_with_parent(dev, map, "div",
> +						clk_hw_get_name(&mux->hw),
> +						CLK_SET_RATE_PARENT,
> +						&clk_regmap_divider_with_init_ops,
> +						&mmc_clkc_div_data);
> +	if (IS_ERR(div))
> +		return PTR_ERR(div);
> +
> +	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
> +						 clk_hw_get_name(&div->hw),
> +						 CLK_SET_RATE_PARENT,
> +						 &meson_clk_phase_ops,
> +						 &mmc_clkc_core_phase);
> +	if (IS_ERR(core))
> +		return PTR_ERR(core);
> +
> +	rx = mmc_clkc_register_clk_with_parent(dev, map, "rx",
> +					       clk_hw_get_name(&core->hw),  0,
> +					       &meson_clk_phase_delay_ops,
> +					       &data->rx);
> +	if (IS_ERR(rx))
> +		return PTR_ERR(rx);
> +
> +	tx = mmc_clkc_register_clk_with_parent(dev, map, "tx",
> +					       clk_hw_get_name(&core->hw),  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);

This can be compiled as module so please append the proper:
MODULE_DESCRIPTION()
MODULE_AUTHOR()
MODULE_LICENSE()

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

[Jianxin Pan]

Hi Jerome,

On 2018/10/24 17:01, Jerome Brunet wrote:
> On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan 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.
>>
[...]
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index efaa70f..8b8ccbc 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
> 
> COMMON_CLK_AMLOGIC is not something that is manually enabled
> You should select it, not depends on it. Have a look at how it is done for the
> other controller (like in v4.19)
OK. I will fix it.
> 
>> +	select MFD_SYSCON
>> +	select REGMAP
> 
> this is already selected by COMMON_CLK_AMLOGIC, please drop this.
OK.
> 
>> +	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..f96314d 100644
>> --- a/drivers/clk/meson/clk-regmap.c
>> +++ b/drivers/clk/meson/clk-regmap.c
>> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>  				  clk_div_mask(div->width) << div->shift, val);
>>  };
>>  
>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>> +static void clk_regmap_div_init(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap *clk = to_clk_regmap(hw);
>> +	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = regmap_read(clk->map, div->offset, &val);
>> +	if (ret)
>> +		return;
>>  
>> +	val &= (clk_div_mask(div->width) << div->shift);
>> +	if (!val)
>> +		regmap_update_bits(clk->map, div->offset,
>> +				   clk_div_mask(div->width) << div->shift,
>> +				   clk_div_mask(div->width));
> 
> This is wrong for several reasons:
> * You should hard code the initial value in the driver.
> * If shift is not 0, I doubt this will give the expected result.
The value 0x00 of divider means nand clock off, then read/write nand register is forbidden.
Should we set the initial value in nand driver, or in sub emmc clk driver?  
> 
>> +}
> 
> Please drop this. 
> 
>> +
>> +/* 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,
>> @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>  };
>>  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
>>  
>> +const struct clk_ops clk_regmap_divider_with_init_ops = {
>> +	.recalc_rate = clk_regmap_div_recalc_rate,
>> +	.round_rate = clk_regmap_div_round_rate,
>> +	.set_rate = clk_regmap_div_set_rate,
>> +	.init = clk_regmap_div_init,
>> +};
>> +EXPORT_SYMBOL_GPL(clk_regmap_divider_with_init_ops);
> 
> ... and this.
> 
>> +
>>  const struct clk_ops clk_regmap_divider_ro_ops = {
>>  	.recalc_rate = clk_regmap_div_recalc_rate,
>>  	.round_rate = clk_regmap_div_round_rate,
>> diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
>> index ed2d434..0ab7d37 100644
>> --- a/drivers/clk/meson/clk-regmap.h
>> +++ b/drivers/clk/meson/clk-regmap.h
>> @@ -109,5 +109,6 @@ struct clk_regmap_mux_data {
>>  
>>  extern const struct clk_ops clk_regmap_mux_ops;
>>  extern const struct clk_ops clk_regmap_mux_ro_ops;
>> +extern const struct clk_ops clk_regmap_divider_with_init_ops;
> 
> ... and this as well.
> 
> There is no reason to to init the divider to something other than what it is
> already when we register the clock controller.
> 
> If your consumer needs the clocks to be initialised, it should call
> clk_set_rate()
> 
>>  
>>  #endif /* __CLK_REGMAP_H */
>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 0000000..5555e3f
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,296 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/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_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_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 char *parent,
> 
> I would prefer if you passed the parent clk_hw pointer and call
> clk_hw_get_name() in here
OK. I will fix it. Thanks for your review.
> 
>> +				  unsigned long flags,
>> +				  const struct clk_ops *ops, void *data)
>> +{
>> +	struct clk_init_data init;
>> +	struct clk_regmap *clk;
>> +
>> +	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);
>> +
>> +	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;
> 
> You really don't need all these local variables ( I think I already pointed this
> out in past reviews ...)
> 
> You can keep one struct clk_regmap *clk and store the clk->hw in the onecell
> data right after registering the clock.
OK, I will remove them.
> 
> 
>> +
>> +	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_clk_with_parent(dev, map, "div",
>> +						clk_hw_get_name(&mux->hw),
>> +						CLK_SET_RATE_PARENT,
>> +						&clk_regmap_divider_with_init_ops,
>> +						&mmc_clkc_div_data);
>> +	if (IS_ERR(div))
>> +		return PTR_ERR(div);
>> +
>> +	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
>> +						 clk_hw_get_name(&div->hw),
>> +						 CLK_SET_RATE_PARENT,
>> +						 &meson_clk_phase_ops,
>> +						 &mmc_clkc_core_phase);
>> +	if (IS_ERR(core))
>> +		return PTR_ERR(core);
>> +
>> +	rx = mmc_clkc_register_clk_with_parent(dev, map, "rx",
>> +					       clk_hw_get_name(&core->hw),  0,
>> +					       &meson_clk_phase_delay_ops,
>> +					       &data->rx);
>> +	if (IS_ERR(rx))
>> +		return PTR_ERR(rx);
>> +
>> +	tx = mmc_clkc_register_clk_with_parent(dev, map, "tx",
>> +					       clk_hw_get_name(&core->hw),  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);
> 
> This can be compiled as module so please append the proper:
> MODULE_DESCRIPTION()
> MODULE_AUTHOR()
> MODULE_LICENSE()
OK. I will fix it in the next version.
> 
> .
> 

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

[Jerome Brunet]

On Thu, 2018-10-25 at 19:48 +0800, Jianxin Pan wrote:
> Hi Jerome,
> 
> On 2018/10/24 17:01, Jerome Brunet wrote:
> > On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan 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.
> > > 
> 
> [...]
> > > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> > > index efaa70f..8b8ccbc 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
> > 
> > COMMON_CLK_AMLOGIC is not something that is manually enabled
> > You should select it, not depends on it. Have a look at how it is done for the
> > other controller (like in v4.19)
> 
> OK. I will fix it.
> > 
> > > +	select MFD_SYSCON
> > > +	select REGMAP
> > 
> > this is already selected by COMMON_CLK_AMLOGIC, please drop this.
> 
> OK.
> > 
> > > +	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..f96314d 100644
> > > --- a/drivers/clk/meson/clk-regmap.c
> > > +++ b/drivers/clk/meson/clk-regmap.c
> > > @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> > >  				  clk_div_mask(div->width) << div->shift, val);
> > >  };
> > >  
> > > -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > +{
> > > +	struct clk_regmap *clk = to_clk_regmap(hw);
> > > +	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > +	unsigned int val;
> > > +	int ret;
> > > +
> > > +	ret = regmap_read(clk->map, div->offset, &val);
> > > +	if (ret)
> > > +		return;
> > >  
> > > +	val &= (clk_div_mask(div->width) << div->shift);
> > > +	if (!val)
> > > +		regmap_update_bits(clk->map, div->offset,
> > > +				   clk_div_mask(div->width) << div->shift,
> > > +				   clk_div_mask(div->width));
> > 
> > This is wrong for several reasons:
> > * You should hard code the initial value in the driver.
> > * If shift is not 0, I doubt this will give the expected result.
> 
> The value 0x00 of divider means nand clock off then read/write nand register is forbidden.

That is not entirely true, you can access the clock register or you'd be in a
chicken and egg situation.

> Should we set the initial value in nand driver, or in sub emmc clk driver?

In the nand driver, which is the consumer of the clock. see my previous comments
about it.

If your device (nand in your case) needs a (sane) clock before doing anything
else, just call clk_set_rate() and enable it with clk_prepare_enable().

Look at our MMC driver, this is the first thing done after registering the
clocks.

The controller does no care at all about the clock rate or state. It is up to
the consumer to express their needs.

On a more general note:
I agree that having a 0 value for CLK_DIVIDER_ONE_BASED divider makes no sense.
If you think this point needs to be addressed, please:

* make separated generic patch
* against driver/clk/clk-divider.c 
* addressing specifically CLK_DIVIDER_ONE_BASED dividers (not all, as your
change does)
* with some comments explaining the intent.

>  
> > 
> > > +}
> > 
> > Please drop this. 
> > 
> > > +
> > > +/* 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,
> > > @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> > >  };
> > >  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
> > >  
> > > +const struct clk_ops clk_regmap_divider_with_init_ops = {
> > > +	.recalc_rate = clk_regmap_div_recalc_rate,
> > > +	.round_rate = clk_regmap_div_round_rate,
> > > +	.set_rate = clk_regmap_div_set_rate,
> > > +	.init = clk_regmap_div_init,
> > > +};
> > > +EXPORT_SYMBOL_GPL(clk_regmap_divider_with_init_ops);
> > 
> > ... and this.
> > 
> > > +
> > >  const struct clk_ops clk_regmap_divider_ro_ops = {
> > >  	.recalc_rate = clk_regmap_div_recalc_rate,
> > >  	.round_rate = clk_regmap_div_round_rate,
> > > diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
> > > index ed2d434..0ab7d37 100644
> > > --- a/drivers/clk/meson/clk-regmap.h
> > > +++ b/drivers/clk/meson/clk-regmap.h
> > > @@ -109,5 +109,6 @@ struct clk_regmap_mux_data {
> > >  
> > >  extern const struct clk_ops clk_regmap_mux_ops;
> > >  extern const struct clk_ops clk_regmap_mux_ro_ops;
> > > +extern const struct clk_ops clk_regmap_divider_with_init_ops;
> > 
> > ... and this as well.
> > 
> > There is no reason to to init the divider to something other than what it is
> > already when we register the clock controller.
> > 
> > If your consumer needs the clocks to be initialised, it should call
> > clk_set_rate()
> > 
> > >  
> > >  #endif /* __CLK_REGMAP_H */
> > > diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> > > new file mode 100644
> > > index 0000000..5555e3f
> > > --- /dev/null
> > > +++ b/drivers/clk/meson/mmc-clkc.c
> > > @@ -0,0 +1,296 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +/*
> > > + * Amlogic Meson MMC Sub Clock Controller Driver
> > > + *
> > > + * Copyright (c) 2017 Baylibre SAS.
> > > + * Author: Jerome Brunet <jbrunet@baylibre.com>
> > > + *
> > > + * Copyright (c) 2018 Amlogic, inc.
> > > + * Author: Yixun Lan <yixun.lan@amlogic.com>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/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_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,

Missed that earlier

This flag makes no sense for a mux.
Anyway this particular mux should never round up has doing so would be unsafe.
The clock provided to the nand or the eMMC should be the requested or lower.

> > > +};
> > > +
> > > +static struct clk_regmap_div_data mmc_clkc_div_data = {
> > > +	.offset		= SD_EMMC_CLOCK,
> > > +	.shift		= 0,
> > > +	.width		= 6,
> > > +	.flags		= CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,

Same here, drop CLK_DIVIDER_ROUND_CLOSEST

> > > +};
> > > +
> > > +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 char *parent,
> > 
> > I would prefer if you passed the parent clk_hw pointer and call
> > clk_hw_get_name() in here
> 
> OK. I will fix it. Thanks for your review.
> > 
> > > +				  unsigned long flags,
> > > +				  const struct clk_ops *ops, void *data)
> > > +{
> > > +	struct clk_init_data init;
> > > +	struct clk_regmap *clk;
> > > +
> > > +	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);
> > > +
> > > +	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;
> > 
> > You really don't need all these local variables ( I think I already pointed this
> > out in past reviews ...)
> > 
> > You can keep one struct clk_regmap *clk and store the clk->hw in the onecell
> > data right after registering the clock.
> 
> OK, I will remove them.
> > 
> > 
> > > +
> > > +	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_clk_with_parent(dev, map, "div",
> > > +						clk_hw_get_name(&mux->hw),
> > > +						CLK_SET_RATE_PARENT,
> > > +						&clk_regmap_divider_with_init_ops,
> > > +						&mmc_clkc_div_data);
> > > +	if (IS_ERR(div))
> > > +		return PTR_ERR(div);
> > > +
> > > +	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
> > > +						 clk_hw_get_name(&div->hw),
> > > +						 CLK_SET_RATE_PARENT,
> > > +						 &meson_clk_phase_ops,
> > > +						 &mmc_clkc_core_phase);
> > > +	if (IS_ERR(core))
> > > +		return PTR_ERR(core);
> > > +
> > > +	rx = mmc_clkc_register_clk_with_parent(dev, map, "rx",
> > > +					       clk_hw_get_name(&core->hw),  0,
> > > +					       &meson_clk_phase_delay_ops,
> > > +					       &data->rx);
> > > +	if (IS_ERR(rx))
> > > +		return PTR_ERR(rx);
> > > +
> > > +	tx = mmc_clkc_register_clk_with_parent(dev, map, "tx",
> > > +					       clk_hw_get_name(&core->hw),  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);
> > 
> > This can be compiled as module so please append the proper:
> > MODULE_DESCRIPTION()
> > MODULE_AUTHOR()
> > MODULE_LICENSE()
> 
> OK. I will fix it in the next version.
> > 
> > .
> > 
> 
> 

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

[Martin Blumenstingl]

Hi Jerome,

On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
[snip]
> > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > +{
> > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > > + unsigned int val;
> > > > + int ret;
> > > > +
> > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > + if (ret)
> > > > +         return;
> > > >
> > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > + if (!val)
> > > > +         regmap_update_bits(clk->map, div->offset,
> > > > +                            clk_div_mask(div->width) << div->shift,
> > > > +                            clk_div_mask(div->width));
> > >
> > > This is wrong for several reasons:
> > > * You should hard code the initial value in the driver.
> > > * If shift is not 0, I doubt this will give the expected result.
> >
> > The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
>
> That is not entirely true, you can access the clock register or you'd be in a
> chicken and egg situation.
>
> > Should we set the initial value in nand driver, or in sub emmc clk driver?
>
> In the nand driver, which is the consumer of the clock. see my previous comments
> about it.
an old version of this series had the code still in the NAND driver
(by writing to the registers directly instead of using the clk API).
this looks pretty much like a "sclk-div" to me (as I commented in v3
of this series: [0]):
- value 0 means disabled
- positive divider values
- (probably no duty control, but that's optional as far as I
understand sclk-div)
- uses max divider value when enabling the clock

if switching to sclk-div works then we can get rid of some duplicate code


Regards
Martin


[0] https://patchwork.kernel.org/patch/10607157/#22238243

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

[Jerome Brunet]

On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
> Hi Jerome,
> 
> On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [snip]
> > > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > > +{
> > > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > > + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > > > + unsigned int val;
> > > > > + int ret;
> > > > > +
> > > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > > + if (ret)
> > > > > +         return;
> > > > > 
> > > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > > + if (!val)
> > > > > +         regmap_update_bits(clk->map, div->offset,
> > > > > +                            clk_div_mask(div->width) << div->shift,
> > > > > +                            clk_div_mask(div->width));
> > > > 
> > > > This is wrong for several reasons:
> > > > * You should hard code the initial value in the driver.
> > > > * If shift is not 0, I doubt this will give the expected result.
> > > 
> > > The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
> > 
> > That is not entirely true, you can access the clock register or you'd be in a
> > chicken and egg situation.
> > 
> > > Should we set the initial value in nand driver, or in sub emmc clk driver?
> > 
> > In the nand driver, which is the consumer of the clock. see my previous comments
> > about it.
> 
> an old version of this series had the code still in the NAND driver
> (by writing to the registers directly instead of using the clk API).
> this looks pretty much like a "sclk-div" to me (as I commented in v3
> of this series: [0]):
> - value 0 means disabled
> - positive divider values
> - (probably no duty control, but that's optional as far as I
> understand sclk-div)
> - uses max divider value when enabling the clock
> 
> if switching to sclk-div works then we can get rid of some duplicate code

It is possible:
There is a couple of things to note though:

* sclk does not 'uses max divider value when enabling the clock': Since this
divider can gate, it needs to save the divider value when disabling, since the
divider value is no longer stored in the register,
On init, this cached value is  saved as it is. If the divider is initially
disabled, we have to set the cached value to something that makes sense, in case
the clock is enabled without a prior call to clk_set_rate().

So in sclk, the clock setting is not changed nor hard coded in init, and this is
a very important difference.
 
* Even if sclk zero value means gated, it is still a zero based divider, while
eMMC/Nand divider is one based. It this controller was to sclk, then something
needs to be done for this.

* Since sclk caches a value in its data, and there can multiple instance of eMMC
/NAND clock controller, some care must be taken when registering the data.

Both the generic divider and sclk could work here ... it's up to you Jianxin.

> 
> 
> Regards
> Martin
> 
> 
> [0] https://patchwork.kernel.org/patch/10607157/#22238243

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

[Martin Blumenstingl]

Hi Jerome,

many thanks for the whole explanation!

On Sun, Oct 28, 2018 at 8:16 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
> > Hi Jerome,
> >
> > On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > [snip]
> > > > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > > > +{
> > > > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > > > + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > > > > + unsigned int val;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > > > + if (ret)
> > > > > > +         return;
> > > > > >
> > > > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > > > + if (!val)
> > > > > > +         regmap_update_bits(clk->map, div->offset,
> > > > > > +                            clk_div_mask(div->width) << div->shift,
> > > > > > +                            clk_div_mask(div->width));
> > > > >
> > > > > This is wrong for several reasons:
> > > > > * You should hard code the initial value in the driver.
> > > > > * If shift is not 0, I doubt this will give the expected result.
> > > >
> > > > The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
> > >
> > > That is not entirely true, you can access the clock register or you'd be in a
> > > chicken and egg situation.
> > >
> > > > Should we set the initial value in nand driver, or in sub emmc clk driver?
> > >
> > > In the nand driver, which is the consumer of the clock. see my previous comments
> > > about it.
> >
> > an old version of this series had the code still in the NAND driver
> > (by writing to the registers directly instead of using the clk API).
> > this looks pretty much like a "sclk-div" to me (as I commented in v3
> > of this series: [0]):
> > - value 0 means disabled
> > - positive divider values
> > - (probably no duty control, but that's optional as far as I
> > understand sclk-div)
> > - uses max divider value when enabling the clock
> >
> > if switching to sclk-div works then we can get rid of some duplicate code
>
> It is possible:
> There is a couple of things to note though:
>
> * sclk does not 'uses max divider value when enabling the clock': Since this
> divider can gate, it needs to save the divider value when disabling, since the
> divider value is no longer stored in the register,
> On init, this cached value is  saved as it is. If the divider is initially
> disabled, we have to set the cached value to something that makes sense, in case
> the clock is enabled without a prior call to clk_set_rate().
>
> So in sclk, the clock setting is not changed nor hard coded in init, and this is
> a very important difference.
>
> * Even if sclk zero value means gated, it is still a zero based divider, while
> eMMC/Nand divider is one based. It this controller was to sclk, then something
> needs to be done for this.
>
> * Since sclk caches a value in its data, and there can multiple instance of eMMC
> /NAND clock controller, some care must be taken when registering the data.
>
> Both the generic divider and sclk could work here ... it's up to you Jianxin.
to give even more options:
the generic divider will (probably) get a CLK_DIVIDER_ZERO_GATE flag
in the next development cycle, see [0] (this may require a small
change to clk-regmap on top)


Regards
Martin


[0] https://patchwork.kernel.org/patch/10650797/

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

[Jianxin Pan]

Hi Jerome,

On 2018/10/29 3:16, Jerome Brunet wrote:
> On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
>> Hi Jerome,
>>
>> On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>> [snip]
>>>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>>>> +{
>>>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>>>> + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>>>>> + unsigned int val;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = regmap_read(clk->map, div->offset, &val);
>>>>>> + if (ret)
>>>>>> +         return;
>>>>>>
>>>>>> + val &= (clk_div_mask(div->width) << div->shift);
>>>>>> + if (!val)
>>>>>> +         regmap_update_bits(clk->map, div->offset,
>>>>>> +                            clk_div_mask(div->width) << div->shift,
>>>>>> +                            clk_div_mask(div->width));
>>>>>
>>>>> This is wrong for several reasons:
>>>>> * You should hard code the initial value in the driver.
>>>>> * If shift is not 0, I doubt this will give the expected result.
>>>>
>>>> The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
>>>
>>> That is not entirely true, you can access the clock register or you'd be in a
>>> chicken and egg situation.
>>>
>>>> Should we set the initial value in nand driver, or in sub emmc clk driver?
>>>
>>> In the nand driver, which is the consumer of the clock. see my previous comments
>>> about it.
>>
>> an old version of this series had the code still in the NAND driver
>> (by writing to the registers directly instead of using the clk API).
>> this looks pretty much like a "sclk-div" to me (as I commented in v3
>> of this series: [0]):
>> - value 0 means disabled
>> - positive divider values
>> - (probably no duty control, but that's optional as far as I
>> understand sclk-div)
>> - uses max divider value when enabling the clock
>>
>> if switching to sclk-div works then we can get rid of some duplicate code
> 
> It is possible:
> There is a couple of things to note though:
> 
> * sclk does not 'uses max divider value when enabling the clock': Since this
> divider can gate, it needs to save the divider value when disabling, since the
> divider value is no longer stored in the register,
> On init, this cached value is  saved as it is. If the divider is initially
> disabled, we have to set the cached value to something that makes sense, in case
> the clock is enabled without a prior call to clk_set_rate().
> 
> So in sclk, the clock setting is not changed nor hard coded in init, and this is
> a very important difference.
>  
> * Even if sclk zero value means gated, it is still a zero based divider, while
> eMMC/Nand divider is one based. It this controller was to sclk, then something
> needs to be done for this.
> 
> * Since sclk caches a value in its data, and there can multiple instance of eMMC
> /NAND clock controller, some care must be taken when registering the data.
> 
> Both the generic divider and sclk could work here ... it's up to you Jianxin.
Thank you for the detailed explanation.
I will use sclk here.

With generic divider, there is a WARNING in divider_recalc_rate() durning clk_register():
[    0.918238] ffe05000.clock-controller#div: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set
[    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127 divider_recalc_rate+0x88/0x90
Then I still need to hard code the initual value, or add CLK_DIVIDER_ALLOW_ZERO flags.
> 
>>
>>
>> Regards
>> Martin
>>
>>
>> [0] https://patchwork.kernel.org/patch/10607157/#22238243
> 
> 
> .
> 

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

[Jianxin Pan]

Hi Jerome,

Thanks for the review, we really appreciate your time.

I'm very sorry maybe I don't catch all your meaning very well. 

Please see my comments below.

On 2018/10/29 3:16, Jerome Brunet wrote:
> On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
>> Hi Jerome,
>>
>> On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>> [snip]
>>>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>>>> +{
>>>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>>>> + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>>>>> + unsigned int val;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = regmap_read(clk->map, div->offset, &val);
>>>>>> + if (ret)
>>>>>> +         return;
>>>>>>
>>>>>> + val &= (clk_div_mask(div->width) << div->shift);
>>>>>> + if (!val)
>>>>>> +         regmap_update_bits(clk->map, div->offset,
>>>>>> +                            clk_div_mask(div->width) << div->shift,
>>>>>> +                            clk_div_mask(div->width));
>>>>>
>>>>> This is wrong for several reasons:
>>>>> * You should hard code the initial value in the driver.
>>>>> * If shift is not 0, I doubt this will give the expected result.
>>>>
>>>> The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
>>>
>>> That is not entirely true, you can access the clock register or you'd be in a
>>> chicken and egg situation.
>>>
>>>> Should we set the initial value in nand driver, or in sub emmc clk driver?
>>>
>>> In the nand driver, which is the consumer of the clock. see my previous comments
>>> about it.
>>
>> an old version of this series had the code still in the NAND driver
>> (by writing to the registers directly instead of using the clk API).
>> this looks pretty much like a "sclk-div" to me (as I commented in v3
>> of this series: [0]):
>> - value 0 means disabled
>> - positive divider values
>> - (probably no duty control, but that's optional as far as I
>> understand sclk-div)
>> - uses max divider value when enabling the clock
>>
>> if switching to sclk-div works then we can get rid of some duplicate code
> 
> It is possible:
> There is a couple of things to note though:
> 
> * sclk does not 'uses max divider value when enabling the clock': Since this
> divider can gate, it needs to save the divider value when disabling, since the
> divider value is no longer stored in the register,
> On init, this cached value is  saved as it is. If the divider is initially
> disabled, we have to set the cached value to something that makes sense, in case
> the clock is enabled without a prior call to clk_set_rate().
>> So in sclk, the clock setting is not changed nor hard coded in init, and this is
> a very important difference.
>
I think It's ok for the latest sub mmc clock and nand driver now:
1. in mmc_clkc_register_clk_with_parent("div", ...) from mmc_clkc_probe():
   cached_div is set to div_max durning clk register?but is not set to div hw register.

2. In meson nand driver v6: 	https://lore.kernel.org/lkml/1541090542-19618-3-git-send-email-jianxin.pan at amlogic.com
1) In meson_nfc_clk_init() from probe:   get clock handle, then prepare_enable and set default rate.
   nfc->device_clk = devm_clk_get(nfc->dev, "device");
   ret = clk_prepare_enable(nfc->device_clk);			//Here div hw register changed from 0 -> cached_div.
   default_clk_rate = clk_round_rate(nfc->device_clk, 24000000);
   ret = clk_set_rate(nfc->device_clk, default_clk_rate);	//Then register and cached_div are both updated to the default 24M.
2) In meson_nfc_select_chip(), set the actual frequency
  ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate);	//Here register and cached_div are changed again.
3) if clk_disable() is called, set div hw register to zero, and cached_div  keep unchagned.
   if clk_disable() is called again,  cached_div is restored to div hw register.

When enabling the clock, divider register does not need to be div_max.  
Any value is OK except ZERO, the cached_div from init or set_rate is ok
>  
> * Even if sclk zero value means gated, it is still a zero based divider, while
> eMMC/Nand divider is one based. It this controller was to sclk, then something
> needs to be done for this.
Could I add another patch to this patchset for sclk to support CLK_DIVIDER_ONE_BASED ?
> 
> * Since sclk caches a value in its data, and there can multiple instance of eMMC
> /NAND clock controller, some care must be taken when registering the data.
OK, I will fix it and alloc mmc_clkc_div_data danymicly durning probe.
Thank you. 
> 
> Both the generic divider and sclk could work here ... it's up to you Jianxin.
> 
== Why use meson_sclk_div_ops instead of clk_regmap_divider_ops?
The default divider hw register vaule is 0 when system power on.
Then there is a WARNING in divider_recalc_rate() durning clk_hw_register():
[    0.918238] ffe05000.clock-controller#div: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set
[    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127 divider_recalc_rate+0x88/0x90
Then I still need to hard code the initual value to nand driver without CLK_DIVIDER_ALLOW_ZERO flags.
>>
>>
>> Regards
>> Martin
>>
>>
>> [0] https://patchwork.kernel.org/patch/10607157/#22238243
> 
> 
> .
> 

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

[jbrunet at baylibre.com]

On Sun, 2018-11-04 at 02:01 +0800, Jianxin Pan wrote:
> Hi Jerome,
> 
> Thanks for the review, we really appreciate your time.
> 
> I'm very sorry maybe I don't catch all your meaning very well. 
> 
> Please see my comments below.
> 
> On 2018/10/29 3:16, Jerome Brunet wrote:
> > On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
> > > Hi Jerome,
> > > 
> > > On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@baylibre.com>
> > > wrote:
> > > [snip]
> > > > > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > > > > +{
> > > > > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > > > > + struct clk_regmap_div_data *div =
> > > > > > > clk_get_regmap_div_data(clk);
> > > > > > > + unsigned int val;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > > > > + if (ret)
> > > > > > > +         return;
> > > > > > > 
> > > > > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > > > > + if (!val)
> > > > > > > +         regmap_update_bits(clk->map, div->offset,
> > > > > > > +                            clk_div_mask(div->width) << div-
> > > > > > > >shift,
> > > > > > > +                            clk_div_mask(div->width));
> > > > > > 
> > > > > > This is wrong for several reasons:
> > > > > > * You should hard code the initial value in the driver.
> > > > > > * If shift is not 0, I doubt this will give the expected result.
> > > > > 
> > > > > The value 0x00 of divider means nand clock off then read/write nand
> > > > > register is forbidden.
> > > > 
> > > > That is not entirely true, you can access the clock register or you'd
> > > > be in a
> > > > chicken and egg situation.
> > > > 
> > > > > Should we set the initial value in nand driver, or in sub emmc clk
> > > > > driver?
> > > > 
> > > > In the nand driver, which is the consumer of the clock. see my
> > > > previous comments
> > > > about it.
> > > 
> > > an old version of this series had the code still in the NAND driver
> > > (by writing to the registers directly instead of using the clk API).
> > > this looks pretty much like a "sclk-div" to me (as I commented in v3
> > > of this series: [0]):
> > > - value 0 means disabled
> > > - positive divider values
> > > - (probably no duty control, but that's optional as far as I
> > > understand sclk-div)
> > > - uses max divider value when enabling the clock
> > > 
> > > if switching to sclk-div works then we can get rid of some duplicate
> > > code
> > 
> > It is possible:
> > There is a couple of things to note though:
> > 
> > * sclk does not 'uses max divider value when enabling the clock': Since
> > this
> > divider can gate, it needs to save the divider value when disabling, since
> > the
> > divider value is no longer stored in the register,
> > On init, this cached value is  saved as it is. If the divider is initially
> > disabled, we have to set the cached value to something that makes sense,
> > in case
> > the clock is enabled without a prior call to clk_set_rate().
> > > So in sclk, the clock setting is not changed nor hard coded in init, and
> > > this is
> > a very important difference.
> > 
> I think It's ok for the latest sub mmc clock and nand driver now:
> 1. in mmc_clkc_register_clk_with_parent("div", ...) from mmc_clkc_probe():
>    cached_div is set to div_max durning clk register?but is not set to div
> hw register.
> 
> 2. In meson nand driver v6: 	
> https://lore.kernel.org/lkml/1541090542-19618-3-git-send-email-jianxin.pan at amlogic.com
> 1) In meson_nfc_clk_init() from probe:   get clock handle, then
> prepare_enable and set default rate.
>    nfc->device_clk = devm_clk_get(nfc->dev, "device");
>    ret = clk_prepare_enable(nfc->device_clk);			//Here div hw
> register changed from 0 -> cached_div.
>    default_clk_rate = clk_round_rate(nfc->device_clk, 24000000);
>    ret = clk_set_rate(nfc->device_clk, default_clk_rate);	//Then
> register and cached_div are both updated to the default 24M.
> 2) In meson_nfc_select_chip(), set the actual frequency
>   ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate);	//Here
> register and cached_div are changed again.
> 3) if clk_disable() is called, set div hw register to zero, and
> cached_div  keep unchagned.
>    if clk_disable() is called again,  cached_div is restored to div hw
> register.

You don't need to do all this in your NAND driver: enable - round - set_rate -
disable is a waste of time. 

Directly calling set_rate(24000000), with the clock still off, will have the
same result. Then if your HW needs this clock to be ON to access registers
(like you told us) you should probably turn it on.

> 
> When enabling the clock, divider register does not need to be div_max.  
> Any value is OK except ZERO, the cached_div from init or set_rate is ok
> >  
> > * Even if sclk zero value means gated, it is still a zero based divider,
> > while
> > eMMC/Nand divider is one based. It this controller was to sclk, then
> > something
> > needs to be done for this.
> Could I add another patch to this patchset for sclk to support
> CLK_DIVIDER_ONE_BASED ?

Yes, you should otherwise the calculation are just wrong for your clock.

> > * Since sclk caches a value in its data, and there can multiple instance
> > of eMMC
> > /NAND clock controller, some care must be taken when registering the data.
> OK, I will fix it and alloc mmc_clkc_div_data danymicly durning probe.
> Thank you. 
> > Both the generic divider and sclk could work here ... it's up to you
> > Jianxin.
> > 
> == Why use meson_sclk_div_ops instead of clk_regmap_divider_ops?
> The default divider hw register vaule is 0 when system power on.
> Then there is a WARNING in divider_recalc_rate() durning clk_hw_register():
> [    0.918238] ffe05000.clock-controller#div: Zero divisor and
> CLK_DIVIDER_ALLOW_ZERO not set
> [    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127
> divider_recalc_rate+0x88/0x90
> Then I still need to hard code the initual value to nand driver without
> CLK_DIVIDER_ALLOW_ZERO flags.
> > > 
> > > Regards
> > > Martin
> > > 
> > > 
> > > [0] https://patchwork.kernel.org/patch/10607157/#22238243
> > 
> > .
> > 
> 
> 

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

[Jianxin Pan]

On 2018/11/5 17:46, jbrunet at baylibre.com wrote:
> On Sun, 2018-11-04 at 02:01 +0800, Jianxin Pan wrote:
>> Hi Jerome,
>>
>> Thanks for the review, we really appreciate your time.
>>
>> I'm very sorry maybe I don't catch all your meaning very well. 
>>
>> Please see my comments below.
>>
>> On 2018/10/29 3:16, Jerome Brunet wrote:
>>> On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
>>>> Hi Jerome,
>>>>
>>>> On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@baylibre.com>
>>>> wrote:
>>>> [snip]
>>>>>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>>>>>> +{
>>>>>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>>>>>> + struct clk_regmap_div_data *div =
>>>>>>>> clk_get_regmap_div_data(clk);
>>>>>>>> + unsigned int val;
>>>>>>>> + int ret;
>>>>>>>> +
>>>>>>>> + ret = regmap_read(clk->map, div->offset, &val);
>>>>>>>> + if (ret)
>>>>>>>> +         return;
>>>>>>>>
>>>>>>>> + val &= (clk_div_mask(div->width) << div->shift);
>>>>>>>> + if (!val)
>>>>>>>> +         regmap_update_bits(clk->map, div->offset,
>>>>>>>> +                            clk_div_mask(div->width) << div-
>>>>>>>>> shift,
>>>>>>>> +                            clk_div_mask(div->width));
>>>>>>>
>>>>>>> This is wrong for several reasons:
>>>>>>> * You should hard code the initial value in the driver.
>>>>>>> * If shift is not 0, I doubt this will give the expected result.
>>>>>>
>>>>>> The value 0x00 of divider means nand clock off then read/write nand
>>>>>> register is forbidden.
>>>>>
>>>>> That is not entirely true, you can access the clock register or you'd
>>>>> be in a
>>>>> chicken and egg situation.
>>>>>
>>>>>> Should we set the initial value in nand driver, or in sub emmc clk
>>>>>> driver?
>>>>>
>>>>> In the nand driver, which is the consumer of the clock. see my
>>>>> previous comments
>>>>> about it.
>>>>
>>>> an old version of this series had the code still in the NAND driver
>>>> (by writing to the registers directly instead of using the clk API).
>>>> this looks pretty much like a "sclk-div" to me (as I commented in v3
>>>> of this series: [0]):
>>>> - value 0 means disabled
>>>> - positive divider values
>>>> - (probably no duty control, but that's optional as far as I
>>>> understand sclk-div)
>>>> - uses max divider value when enabling the clock
>>>>
>>>> if switching to sclk-div works then we can get rid of some duplicate
>>>> code
>>>
>>> It is possible:
>>> There is a couple of things to note though:
>>>
>>> * sclk does not 'uses max divider value when enabling the clock': Since
>>> this
>>> divider can gate, it needs to save the divider value when disabling, since
>>> the
>>> divider value is no longer stored in the register,
>>> On init, this cached value is  saved as it is. If the divider is initially
>>> disabled, we have to set the cached value to something that makes sense,
>>> in case
>>> the clock is enabled without a prior call to clk_set_rate().
>>>> So in sclk, the clock setting is not changed nor hard coded in init, and
>>>> this is
>>> a very important difference.
>>>
>> I think It's ok for the latest sub mmc clock and nand driver now:
>> 1. in mmc_clkc_register_clk_with_parent("div", ...) from mmc_clkc_probe():
>>    cached_div is set to div_max durning clk register?but is not set to div
>> hw register.
>>
>> 2. In meson nand driver v6: 	
>> https://lore.kernel.org/lkml/1541090542-19618-3-git-send-email-jianxin.pan at amlogic.com
>> 1) In meson_nfc_clk_init() from probe:   get clock handle, then
>> prepare_enable and set default rate.
>>    nfc->device_clk = devm_clk_get(nfc->dev, "device");
>>    ret = clk_prepare_enable(nfc->device_clk);			//Here div hw
>> register changed from 0 -> cached_div.
>>    default_clk_rate = clk_round_rate(nfc->device_clk, 24000000);
>>    ret = clk_set_rate(nfc->device_clk, default_clk_rate);	//Then
>> register and cached_div are both updated to the default 24M.
>> 2) In meson_nfc_select_chip(), set the actual frequency
>>   ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate);	//Here
>> register and cached_div are changed again.
>> 3) if clk_disable() is called, set div hw register to zero, and
>> cached_div  keep unchagned.
>>    if clk_disable() is called again,  cached_div is restored to div hw
>> register.
> 
> You don't need to do all this in your NAND driver: enable - round - set_rate -
> disable is a waste of time. 
> 
> Directly calling set_rate(24000000), with the clock still off, will have the
> same result. Then if your HW needs this clock to be ON to access registers
> (like you told us) you should probably turn it on.
I'm sorry I didn't describe it very clearly in last mail. 
The steps in nand v6 probe are: enable -> round(24M) -> set_rate(24M)? then this clock is always on.
And it's disabled only in the nand remove() callback. 
I will remove round(24M) in next version .
Thank you.
> 
>>
>> When enabling the clock, divider register does not need to be div_max.  
>> Any value is OK except ZERO, the cached_div from init or set_rate is ok
>>>  
>>> * Even if sclk zero value means gated, it is still a zero based divider,
>>> while
>>> eMMC/Nand divider is one based. It this controller was to sclk, then
>>> something
>>> needs to be done for this.
>> Could I add another patch to this patchset for sclk to support
>> CLK_DIVIDER_ONE_BASED ?
> 
> Yes, you should otherwise the calculation are just wrong for your clock.
OK. Thank you.
> 
>>> * Since sclk caches a value in its data, and there can multiple instance
>>> of eMMC
>>> /NAND clock controller, some care must be taken when registering the data.
>> OK, I will fix it and alloc mmc_clkc_div_data danymicly durning probe.
>> Thank you. 
>>> Both the generic divider and sclk could work here ... it's up to you
>>> Jianxin.
>>>
>> == Why use meson_sclk_div_ops instead of clk_regmap_divider_ops?
>> The default divider hw register vaule is 0 when system power on.
>> Then there is a WARNING in divider_recalc_rate() durning clk_hw_register():
>> [    0.918238] ffe05000.clock-controller#div: Zero divisor and
>> CLK_DIVIDER_ALLOW_ZERO not set
>> [    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127
>> divider_recalc_rate+0x88/0x90
>> Then I still need to hard code the initual value to nand driver without
>> CLK_DIVIDER_ALLOW_ZERO flags.
>>>>
>>>> Regards
>>>> Martin
>>>>
>>>>
>>>> [0] https://patchwork.kernel.org/patch/10607157/#22238243
>>>
>>> .
>>>
>>
>>
> 
> 
> .
> 

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

[Jianxin Pan]

Hi Jerome,

On 2018/10/25 20:54, Jerome Brunet wrote:
> On Thu, 2018-10-25 at 19:48 +0800, Jianxin Pan wrote:
>> Hi Jerome,
>>
>> On 2018/10/24 17:01, Jerome Brunet wrote:
>>> On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan 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.
>>>>
[...]
>>>>  
>>>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>> +{
>>>> +	struct clk_regmap *clk = to_clk_regmap(hw);
>>>> +	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>>> +	unsigned int val;
>>>> +	int ret;
>>>> +
>>>> +	ret = regmap_read(clk->map, div->offset, &val);
>>>> +	if (ret)
>>>> +		return;
>>>>  
>>>> +	val &= (clk_div_mask(div->width) << div->shift);
>>>> +	if (!val)
>>>> +		regmap_update_bits(clk->map, div->offset,
>>>> +				   clk_div_mask(div->width) << div->shift,
>>>> +				   clk_div_mask(div->width));
>>>
>>> This is wrong for several reasons:
>>> * You should hard code the initial value in the driver.
>>> * If shift is not 0, I doubt this will give the expected result.
>>
>> The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
> 
> That is not entirely true, you can access the clock register or you'd be in a
> chicken and egg situation.
> 
>> Should we set the initial value in nand driver, or in sub emmc clk driver?
> 
> In the nand driver, which is the consumer of the clock. see my previous comments
> about it.
> 
> If your device (nand in your case) needs a (sane) clock before doing anything
> else, just call clk_set_rate() and enable it with clk_prepare_enable().
> 
> Look at our MMC driver, this is the first thing done after registering the
> clocks.
> 
> The controller does no care at all about the clock rate or state. It is up to
> the consumer to express their needs.
> 
> On a more general note:
> I agree that having a 0 value for CLK_DIVIDER_ONE_BASED divider makes no sense.
> If you think this point needs to be addressed, please:
> 
> * make separated generic patch
> * against driver/clk/clk-divider.c 
> * addressing specifically CLK_DIVIDER_ONE_BASED dividers (not all, as your
> change does)
> * with some comments explaining the intent.
In our emmc driver, the CLK_DIV_MASK is hard coded before clock registering in meson_mmc_clk_init():
 clk_reg |= CLK_DIV_MASK;
 writel(clk_reg, host->regs + SD_EMMC_CLOCK);
It's hard coded In previous version of nand driver.  I will drop the new ops.

In addition , Martin suggested in previous discussions that "sclk-div" could be used [0][1]. 
This divider is just like a "sclk-div" with sclk->hi->width ==0. 


> 
>>  
>>>
>>>> +}
>>>
>>> Please drop this. 
OK. Thank you.
>>>
>>>> +
>>>> +/* 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,
>>>> @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>>>  };
>>>>  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
>>>>  
[...]
>>>> +
>>>> +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
>>>> +	.offset		= SD_EMMC_CLOCK,
>>>> +	.mask		= 0x3,
>>>> +	.shift		= 6,
>>>> +	.flags		= CLK_DIVIDER_ROUND_CLOSEST,
> 
> Missed that earlier
> 
> This flag makes no sense for a mux.
> Anyway this particular mux should never round up has doing so would be unsafe.
> The clock provided to the nand or the eMMC should be the requested or lower.
> 
OK, I will drop it. Thanks for your time.
>>>> +};
>>>> +
>>>> +static struct clk_regmap_div_data mmc_clkc_div_data = {
>>>> +	.offset		= SD_EMMC_CLOCK,
>>>> +	.shift		= 0,
>>>> +	.width		= 6,
>>>> +	.flags		= CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> 
> Same here, drop CLK_DIVIDER_ROUND_CLOSEST
OK, I will drop it. Thank you!
> 
>>>> +};
>>>> +
>>>> +static struct meson_clk_phase_data mmc_clkc_core_phase = {
>>>> +	.ph = {
>>>> +		.reg_off	= SD_EMMC_CLOCK,
>>>> +		.shift	= 8,
[...]
> 
> .
> 

[0] https://patchwork.kernel.org/patch/10607157/#22238243
[1]https://lore.kernel.org/lkml/CAFBinCCuqUmVNdwUm7WbkHy1eWvOA5oQ5FcOuytbYNbgGcXnRQ at mail.gmail.com/T/#u