[PATCH RESEND v7 0/4] clk: meson: add a sub EMMC clock controller support

[PATCH RESEND v7 0/4] 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 v6 [7]:
 - add one based support for sclk divier
 - alloc sclk in probe for multiple instance
 - fix coding styles

Changes since v5 [6]:
 - remove divider ops with .init and use sclk_div instead
 - drop CLK_DIVIDER_ROUND_CLOSEST in mux and div
 - drop the useless type cast 

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

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

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

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

[1] https://lkml.kernel.org/r/20180628090034.0637a062@xps13
[2] https://lkml.kernel.org/r/20180703145716.31860-1-yixun.lan@amlogic.com
[3] https://lkml.kernel.org/r/20180710163658.6175-1-yixun.lan@amlogic.com
[4] https://lkml.kernel.org/r/20180712211244.11428-1-yixun.lan@amlogic.com
[5] https://lkml.kernel.org/r/20180809070724.11935-4-yixun.lan@amlogic.com
[6] https://lkml.kernel.org/r/1539839245-13793-1-git-send-email-jianxin.pan@amlogic.com
[7] https://lkml.kernel.org/r/1541089855-19356-1-git-send-email-jianxin.pan@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
  clk: meson: add one based divider support for sclk divider

 .../devicetree/bindings/clock/amlogic,mmc-clkc.txt |  39 +++
 drivers/clk/meson/Kconfig                          |  10 +
 drivers/clk/meson/Makefile                         |   3 +-
 drivers/clk/meson/clk-phase-delay.c                |  64 +++++
 drivers/clk/meson/clkc-audio.h                     |   1 +
 drivers/clk/meson/clkc.h                           |  13 +
 drivers/clk/meson/mmc-clkc.c                       | 313 +++++++++++++++++++++
 drivers/clk/meson/sclk-div.c                       |  28 +-
 include/dt-bindings/clock/amlogic,mmc-clkc.h       |  17 ++
 9 files changed, 477 insertions(+), 11 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v7 1/4] 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 | 64 +++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/clkc.h            | 13 ++++++++
 3 files changed, 78 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..84e7b63
--- /dev/null
+++ b/drivers/clk/meson/clk-phase-delay.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2017 Baylibre SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ * Author: Jianxin Pan <jianxin.pan@amlogic.com>
+ */
+
+#include <linux/clk-provider.h>
+#include "clkc.h"
+
+static int meson_clk_phase_delay_get_phase(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_clk_phase_delay_data *ph;
+	unsigned long period_ps, p, d;
+	int degrees;
+
+	ph = meson_clk_get_phase_delay_data(clk);
+	p = meson_parm_read(clk->map, &ph->phase);
+	degrees = p * 360 / (1 << (ph->phase.width));
+
+	period_ps = DIV_ROUND_UP(NSEC_PER_SEC * 1000,
+				 clk_hw_get_rate(hw));
+
+	d = meson_parm_read(clk->map, &ph->delay);
+	degrees += d * ph->delay_step_ps * 360 / period_ps;
+	degrees %= 360;
+
+	return degrees;
+}
+
+static int meson_clk_phase_delay_set_phase(struct clk_hw *hw, int degrees)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_clk_phase_delay_data *ph;
+	unsigned long period_ps, d = 0, r;
+
+	ph = meson_clk_get_phase_delay_data(clk);
+	period_ps = DIV_ROUND_UP(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(degrees, 360 / 1 << (ph->phase.width));
+	d = DIV_ROUND_CLOSEST(r * period_ps,
+			      360 * ph->delay_step_ps);
+	d = min(d, PMASK(ph->delay.width));
+
+	meson_parm_write(clk->map, &ph->phase, degrees);
+	meson_parm_write(clk->map, &ph->delay, d);
+	return 0;
+}
+
+const struct clk_ops meson_clk_phase_delay_ops = {
+	.get_phase = meson_clk_phase_delay_get_phase,
+	.set_phase = meson_clk_phase_delay_set_phase,
+};
+EXPORT_SYMBOL_GPL(meson_clk_phase_delay_ops);
diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
index 6b96d55..30470c6 100644
--- a/drivers/clk/meson/clkc.h
+++ b/drivers/clk/meson/clkc.h
@@ -105,6 +105,18 @@ struct clk_regmap _name = {						\
 	},								\
 };
 
+struct meson_clk_phase_delay_data {
+	struct parm			phase;
+	struct parm			delay;
+	unsigned int			delay_step_ps;
+};
+
+static inline struct meson_clk_phase_delay_data *
+meson_clk_get_phase_delay_data(struct clk_regmap *clk)
+{
+	return clk->data;
+}
+
 /* clk_ops */
 extern const struct clk_ops meson_clk_pll_ro_ops;
 extern const struct clk_ops meson_clk_pll_ops;
@@ -112,5 +124,6 @@ struct clk_regmap _name = {						\
 extern const struct clk_ops meson_clk_mpll_ro_ops;
 extern const struct clk_ops meson_clk_mpll_ops;
 extern const struct clk_ops meson_clk_phase_ops;
+extern const struct clk_ops meson_clk_phase_delay_ops;
 
 #endif /* __CLKC_H */
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v7 1/4] clk: meson: add emmc sub clock phase delay driver

[Jerome Brunet]

On Tue, 2018-12-11 at 00:04 +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 | 64
> +++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/clkc.h            | 13 ++++++++
>  3 files changed, 78 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..84e7b63
> --- /dev/null
> +++ b/drivers/clk/meson/clk-phase-delay.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + * Author: Jianxin Pan <jianxin.pan@amlogic.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include "clkc.h"
> +
> +static int meson_clk_phase_delay_get_phase(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_clk_phase_delay_data *ph;
> +	unsigned long period_ps, p, d;
> +	int degrees;
> +
> +	ph = meson_clk_get_phase_delay_data(clk);
> +	p = meson_parm_read(clk->map, &ph->phase);
> +	degrees = p * 360 / (1 << (ph->phase.width));
> +
> +	period_ps = DIV_ROUND_UP(NSEC_PER_SEC * 1000,
> +				 clk_hw_get_rate(hw));
> +
> +	d = meson_parm_read(clk->map, &ph->delay);
> +	degrees += d * ph->delay_step_ps * 360 / period_ps;
> +	degrees %= 360;
> +
> +	return degrees;
> +}
> +
> +static int meson_clk_phase_delay_set_phase(struct clk_hw *hw, int degrees)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_clk_phase_delay_data *ph;
> +	unsigned long period_ps, d = 0, r;
> +
> +	ph = meson_clk_get_phase_delay_data(clk);
> +	period_ps = DIV_ROUND_UP(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(degrees, 360 / 1 << (ph->phase.width));
> +	d = DIV_ROUND_CLOSEST(r * period_ps,
> +			      360 * ph->delay_step_ps);
> +	d = min(d, PMASK(ph->delay.width));
> +
> +	meson_parm_write(clk->map, &ph->phase, degrees);
> +	meson_parm_write(clk->map, &ph->delay, d);
> +	return 0;
> +}
> +
> +const struct clk_ops meson_clk_phase_delay_ops = {
> +	.get_phase = meson_clk_phase_delay_get_phase,
> +	.set_phase = meson_clk_phase_delay_set_phase,
> +};
> +EXPORT_SYMBOL_GPL(meson_clk_phase_delay_ops);
> diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
> index 6b96d55..30470c6 100644
> --- a/drivers/clk/meson/clkc.h
> +++ b/drivers/clk/meson/clkc.h
> @@ -105,6 +105,18 @@ struct clk_regmap _name = {				
> 		\
>  	},								\
>  };
>  
> +struct meson_clk_phase_delay_data {
> +	struct parm			phase;
> +	struct parm			delay;
> +	unsigned int			delay_step_ps;
> +};
> +
> +static inline struct meson_clk_phase_delay_data *
> +meson_clk_get_phase_delay_data(struct clk_regmap *clk)
> +{
> +	return clk->data;
> +}

This is only usefull in the related clock driver, no need to export it

> +
>  /* 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 */



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v7 1/4] clk: meson: add emmc sub clock phase delay driver

[Jianxin Pan]

HI Jerome,

On 2018/12/12 0:28, Jerome Brunet wrote:
> On Tue, 2018-12-11 at 00:04 +0800, Jianxin Pan wrote:
>> From: Yixun Lan <yixun.lan@amlogic.com>
>>
[...]
>> +
>> +static inline struct meson_clk_phase_delay_data *
>> +meson_clk_get_phase_delay_data(struct clk_regmap *clk)
>> +{
>> +	return clk->data;
>> +}
> 
> This is only usefull in the related clock driver, no need to export it
OK, I will move it to clk-phase-delay.c.
Thanks for the review.
> 
>> +
>>  /* 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 */
> 
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v7 2/4] clk: meson: add DT documentation for emmc clock controller

[Jianxin Pan]

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

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

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
---
 .../devicetree/bindings/clock/amlogic,mmc-clkc.txt | 39 ++++++++++++++++++++++
 include/dt-bindings/clock/amlogic,mmc-clkc.h       | 17 ++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
 create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v7 3/4] 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/mmc-clkc.c | 313 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 324 insertions(+)
 create mode 100644 drivers/clk/meson/mmc-clkc.c

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index efaa70f..6bb0d44 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -15,6 +15,16 @@ config COMMON_CLK_MESON_AO
 	select COMMON_CLK_REGMAP_MESON
 	select RESET_CONTROLLER
 
+config COMMON_CLK_MMC_MESON
+	tristate "Meson MMC Sub Clock Controller Driver"
+	select MFD_SYSCON
+	select COMMON_CLK_AMLOGIC
+	select COMMON_CLK_AMLOGIC_AUDIO
+	help
+	  Support for the MMC sub clock controller on Amlogic Meson Platform,
+	  which include S905 (GXBB, GXL), A113D/X (AXG) devices.
+	  Say Y if you want this clock enabled.
+
 config COMMON_CLK_REGMAP_MESON
 	bool
 	select REGMAP
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 39ce566..31c16d5 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
 obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
 obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
 obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
+obj-$(CONFIG_COMMON_CLK_MMC_MESON) 	+= mmc-clkc.o
 obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
new file mode 100644
index 0000000..f5a79a4
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2017 Baylibre SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ * Author: Jianxin Pan <jianxin.pan@amlogic.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/clock/amlogic,mmc-clkc.h>
+
+#include "clkc.h"
+#include "clkc-audio.h"
+
+/* clock ID used by internal driver */
+#define CLKID_MMC_MUX			0
+
+#define   SD_EMMC_CLOCK		0
+#define   CLK_DELAY_STEP_PS		200
+
+#define 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,
+};
+
+static const struct meson_sclk_div_data mmc_clkc_div_data = {
+	.div = {
+		.reg_off = SD_EMMC_CLOCK,
+		.shift   = (0),
+		.width   = (6),
+	},
+	.hi = {
+		.width   = 0,
+	},
+	.flags = 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 struct clk_hw *hw,
+				  unsigned long flags,
+				  const struct clk_ops *ops, void *data)
+{
+	struct clk_init_data init;
+	struct clk_regmap *clk;
+	const char *parent_name = clk_hw_get_name(hw);
+
+	init.ops = ops;
+	init.flags = flags;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
+	if (IS_ERR(clk))
+		dev_err(dev, "Core %s clock registration failed\n", suffix);
+
+	return clk;
+}
+
+static int mmc_clkc_probe(struct platform_device *pdev)
+{
+	struct clk_hw_onecell_data *onecell_data;
+	struct device *dev = &pdev->dev;
+	struct mmc_clkc_data *data;
+	struct regmap *map;
+	struct clk_regmap *clk, *core;
+	struct meson_sclk_div_data *div_data;
+
+	/*cast to drop the const in match->data*/
+	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
+	map = syscon_node_to_regmap(dev->of_node);
+	if (IS_ERR(map)) {
+		dev_err(dev, "could not find mmc clock controller\n");
+		return PTR_ERR(map);
+	}
+
+	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
+				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
+				    GFP_KERNEL);
+	if (!onecell_data)
+		return -ENOMEM;
+
+	clk = mmc_clkc_register_mux(dev, map);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	onecell_data->hws[CLKID_MMC_MUX]		= &clk->hw,
+
+	div_data = devm_kzalloc(dev, sizeof(*div_data), GFP_KERNEL);
+	if (!div_data)
+		return -ENOMEM;
+	*div_data = mmc_clkc_div_data;
+
+	clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
+						&clk->hw,
+						CLK_SET_RATE_PARENT,
+						&meson_sclk_div_ops,
+						div_data);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	onecell_data->hws[CLKID_MMC_DIV]		= &clk->hw,
+
+	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
+						 &clk->hw,
+						 CLK_SET_RATE_PARENT,
+						 &meson_clk_phase_ops,
+						 &mmc_clkc_core_phase);
+	if (IS_ERR(core))
+		return PTR_ERR(core);
+	onecell_data->hws[CLKID_MMC_PHASE_CORE]		= &core->hw,
+
+	clk = mmc_clkc_register_clk_with_parent(dev, map, "rx",
+						&core->hw,  0,
+						&meson_clk_phase_delay_ops,
+						&data->rx);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	onecell_data->hws[CLKID_MMC_PHASE_RX]		= &clk->hw,
+
+	clk = mmc_clkc_register_clk_with_parent(dev, map, "tx",
+						&core->hw,  0,
+						&meson_clk_phase_delay_ops,
+						&data->tx);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	onecell_data->hws[CLKID_MMC_PHASE_TX]		= &clk->hw,
+
+	onecell_data->num				= MMC_MAX_CLKS;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   onecell_data);
+}
+
+static struct platform_driver mmc_clkc_driver = {
+	.probe		= mmc_clkc_probe,
+	.driver		= {
+		.name	= "meson-mmc-clkc",
+		.of_match_table = of_match_ptr(mmc_clkc_match_table),
+	},
+};
+
+module_platform_driver(mmc_clkc_driver);
+
+MODULE_DESCRIPTION("Amlogic AXG MMC clock driver");
+MODULE_AUTHOR("Jianxin Pan <jianxin.pan@amlogic.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver

[Jerome Brunet]

On Tue, 2018-12-11 at 00:04 +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/mmc-clkc.c | 313
> +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 324 insertions(+)
>  create mode 100644 drivers/clk/meson/mmc-clkc.c
> 
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index efaa70f..6bb0d44 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -15,6 +15,16 @@ config COMMON_CLK_MESON_AO
>  	select COMMON_CLK_REGMAP_MESON
>  	select RESET_CONTROLLER
>  
> +config COMMON_CLK_MMC_MESON
> +	tristate "Meson MMC Sub Clock Controller Driver"
> +	select MFD_SYSCON
> +	select COMMON_CLK_AMLOGIC
> +	select COMMON_CLK_AMLOGIC_AUDIO

No it is wrong for the mmc to select AUDIO clocks.
If as a result of your patch sclk is needed for things, make the necessary
change in the Makefile.

> +	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/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> new file mode 100644
> index 0000000..f5a79a4
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + * Author: Jianxin Pan <jianxin.pan@amlogic.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
> +
> +#include "clkc.h"
> +#include "clkc-audio.h"

Again having audio in the mmc controller is wrong.
Please make the necessary rework.

> +
> +/* clock ID used by internal driver */
> +#define CLKID_MMC_MUX			0
> +
> +#define   SD_EMMC_CLOCK		0
           ^
why the multiple space here ? this looks odd

> +#define   CLK_DELAY_STEP_PS		200

Please keep thing aligned aligned consistently.

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

Why use a tab above ?

> +};
> +
> +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
> +	.offset		= SD_EMMC_CLOCK,
> +	.mask		= 0x3,
> +	.shift		= 6,
> +};
> +
> +static const struct meson_sclk_div_data mmc_clkc_div_data = {
> +	.div = {
> +		.reg_off = SD_EMMC_CLOCK,
> +		.shift   = (0),
> +		.width   = (6),

Please remove the unncessary parenthesis

> +	},
> +	.hi = {
> +		.width   = 0,
> +	},

structure is a static const, all non-list members will be zero
drop the 

> +	.flags = 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,
> +		},

Again, an effort on alignement would appreciated, same below

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

nitpick: remove this newline

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

I don't think this cast is necessary     ^

> +		}
> +
> +		parent_names[i] = __clk_get_name(clk);
> +	}
> +
> +	init.ops = &clk_regmap_mux_ops;
> +	init.flags = CLK_SET_RATE_PARENT;
> +	init.parent_names = parent_names;
> +	init.num_parents = MUX_CLK_NUM_PARENTS;
> +
> +	mux = mmc_clkc_register_clk(dev, map, &init, "mux",
> &mmc_clkc_mux_data);
> +	if (IS_ERR(mux))
> +		dev_err(dev, "Mux clock registration failed\n");
> +
> +	return mux;
> +}
> +
> +static struct clk_regmap *
> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
> +				  char *suffix, const struct clk_hw *hw,
> +				  unsigned long flags,
> +				  const struct clk_ops *ops, void *data)
> +{
> +	struct clk_init_data init;
> +	struct clk_regmap *clk;
> +	const char *parent_name = clk_hw_get_name(hw);
> +
> +	init.ops = ops;
> +	init.flags = flags;
> +	init.parent_names = &parent_name;
> +	init.num_parents = 1;
> +
> +	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
> +	if (IS_ERR(clk))
> +		dev_err(dev, "Core %s clock registration failed\n", suffix);

                               ^
this function is not only called by the core clock, is it ?

> +
> +	return clk;
> +}
> +
> +static int mmc_clkc_probe(struct platform_device *pdev)
> +{
> +	struct clk_hw_onecell_data *onecell_data;
> +	struct device *dev = &pdev->dev;
> +	struct mmc_clkc_data *data;
> +	struct regmap *map;
> +	struct clk_regmap *clk, *core;
> +	struct meson_sclk_div_data *div_data;
> +
> +	/*cast to drop the const in match->data*/
> +	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	map = syscon_node_to_regmap(dev->of_node);
> +	if (IS_ERR(map)) {
> +		dev_err(dev, "could not find mmc clock controller\n");
> +		return PTR_ERR(map);
> +	}
> +
> +	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
> +				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
> +				    GFP_KERNEL);
> +	if (!onecell_data)
> +		return -ENOMEM;
> +
> +	clk = mmc_clkc_register_mux(dev, map);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +	onecell_data->hws[CLKID_MMC_MUX]		= &clk->hw,

It does not really need to have an ID or be in the onecell data if you are not
going to expose it. On the other controllers, we are using the onecell for the
registration as well, which is why there is unexpeosed IDs, but it is not the
case here.

... and please, stop with this unnecessary tab. a space will do. same below.

> +
> +	div_data = devm_kzalloc(dev, sizeof(*div_data), GFP_KERNEL);
> +	if (!div_data)
> +		return -ENOMEM;
> +	*div_data = mmc_clkc_div_data;

memcpy would be more appropriate.

> +
> +	clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
> +						&clk->hw,
> +						CLK_SET_RATE_PARENT,
> +						&meson_sclk_div_ops,
> +						div_data);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +	onecell_data->hws[CLKID_MMC_DIV]		= &clk->hw,
> +
> +	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
> +						 &clk->hw,
> +						 CLK_SET_RATE_PARENT,
> +						 &meson_clk_phase_ops,
> +						 &mmc_clkc_core_phase);
> +	if (IS_ERR(core))
> +		return PTR_ERR(core);
> +	onecell_data->hws[CLKID_MMC_PHASE_CORE]		= &core->hw,
> +
> +	clk = mmc_clkc_register_clk_with_parent(dev, map, "rx",
> +						&core->hw,  0,
> +						&meson_clk_phase_delay_ops,
> +						&data->rx);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +	onecell_data->hws[CLKID_MMC_PHASE_RX]		= &clk->hw,
> +
> +	clk = mmc_clkc_register_clk_with_parent(dev, map, "tx",
> +						&core->hw,  0,
> +						&meson_clk_phase_delay_ops,
> +						&data->tx);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +	onecell_data->hws[CLKID_MMC_PHASE_TX]		= &clk->hw,
> +
> +	onecell_data->num				= MMC_MAX_CLKS;
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					   onecell_data);
> +}
> +
> +static struct platform_driver mmc_clkc_driver = {
> +	.probe		= mmc_clkc_probe,
> +	.driver		= {
> +		.name	= "meson-mmc-clkc",
> +		.of_match_table = of_match_ptr(mmc_clkc_match_table),
> +	},
> +};
> +
> +module_platform_driver(mmc_clkc_driver);
> +
> +MODULE_DESCRIPTION("Amlogic AXG MMC clock driver");
> +MODULE_AUTHOR("Jianxin Pan <jianxin.pan@amlogic.com>");
> +MODULE_LICENSE("GPL v2");



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver

[Jianxin Pan]

On 2018/12/12 0:59, Jerome Brunet wrote:
> On Tue, 2018-12-11 at 00:04 +0800, Jianxin Pan wrote:
>> From: Yixun Lan <yixun.lan@amlogic.com>
>>
[...]
>>  
>> +config COMMON_CLK_MMC_MESON
>> +	tristate "Meson MMC Sub Clock Controller Driver"
>> +	select MFD_SYSCON
>> +	select COMMON_CLK_AMLOGIC
>> +	select COMMON_CLK_AMLOGIC_AUDIO
> 
> No it is wrong for the mmc to select AUDIO clocks.
> If as a result of your patch sclk is needed for things, make the necessary
> change in the Makefile.
OK, I will add COMMON_CLK_AMLOGIC_SCLKDIV for sclk-div.
> 
[...]>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
>> +
>> +#include "clkc.h"
>> +#include "clkc-audio.h"
> 
> Again having audio in the mmc controller is wrong.
> Please make the necessary rework.
Yes, I will split out sclk-div.h from clkc-audio.h in the next version.
Thanks for your time.
> 
>> +
>> +/* clock ID used by internal driver */
>> +#define CLKID_MMC_MUX			0
>> +
>> +#define   SD_EMMC_CLOCK		0
>            ^
> why the multiple space here ? this looks odd
I will check the alignement  in the whole patchset and fix them, Thank you.
> 
>> +#define   CLK_DELAY_STEP_PS		200
> 
> Please keep thing aligned aligned consistently.
> 
>> +
>> +#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;
> 
> Why use a tab above ?
OK
> 
>> +};
>> +
>> +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
>> +	.offset		= SD_EMMC_CLOCK,
>> +	.mask		= 0x3,
>> +	.shift		= 6,
>> +};
>> +
>> +static const struct meson_sclk_div_data mmc_clkc_div_data = {
>> +	.div = {
>> +		.reg_off = SD_EMMC_CLOCK,
>> +		.shift   = (0),
>> +		.width   = (6),
> 
> Please remove the unncessary parenthesis
OK, I will remove them.
> 
>> +	},
>> +	.hi = {
>> +		.width   = 0,
>> +	},
> 
> structure is a static const, all non-list members will be zero
> drop the 
OK, I will remove it in the next version. 
> 
>> +	.flags = 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,
>> +		},
> 
> Again, an effort on alignement would appreciated, same below
OK, I will fix them.
> 
>> +		.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;
>> +
> 
> nitpick: remove this newline
OK.
> 
>> +	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);
> 
> I don't think this cast is necessary     ^
Yes, return clk is ok here.
> 
>> +		}
>> +
>> +		parent_names[i] = __clk_get_name(clk);
>> +	}
>> +
>> +	init.ops = &clk_regmap_mux_ops;
>> +	init.flags = CLK_SET_RATE_PARENT;
>> +	init.parent_names = parent_names;
>> +	init.num_parents = MUX_CLK_NUM_PARENTS;
>> +
>> +	mux = mmc_clkc_register_clk(dev, map, &init, "mux",
>> &mmc_clkc_mux_data);
>> +	if (IS_ERR(mux))
>> +		dev_err(dev, "Mux clock registration failed\n");
>> +
>> +	return mux;
>> +}
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
>> +				  char *suffix, const struct clk_hw *hw,
>> +				  unsigned long flags,
>> +				  const struct clk_ops *ops, void *data)
>> +{
>> +	struct clk_init_data init;
>> +	struct clk_regmap *clk;
>> +	const char *parent_name = clk_hw_get_name(hw);
>> +
>> +	init.ops = ops;
>> +	init.flags = flags;
>> +	init.parent_names = &parent_name;
>> +	init.num_parents = 1;
>> +
>> +	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
>> +	if (IS_ERR(clk))
>> +		dev_err(dev, "Core %s clock registration failed\n", suffix);
> 
>                                ^
> this function is not only called by the core clock, is it ?
OK, I will drop "Core" in this line.
> 
>> +
>> +	return clk;
>> +}
>> +
>> +static int mmc_clkc_probe(struct platform_device *pdev)
>> +{
>> +	struct clk_hw_onecell_data *onecell_data;
>> +	struct device *dev = &pdev->dev;
>> +	struct mmc_clkc_data *data;
>> +	struct regmap *map;
>> +	struct clk_regmap *clk, *core;
>> +	struct meson_sclk_div_data *div_data;
>> +
>> +	/*cast to drop the const in match->data*/
>> +	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
>> +	if (!data)
>> +		return -ENODEV;
>> +
>> +	map = syscon_node_to_regmap(dev->of_node);
>> +	if (IS_ERR(map)) {
>> +		dev_err(dev, "could not find mmc clock controller\n");
>> +		return PTR_ERR(map);
>> +	}
>> +
>> +	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
>> +				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
>> +				    GFP_KERNEL);
>> +	if (!onecell_data)
>> +		return -ENOMEM;
>> +
>> +	clk = mmc_clkc_register_mux(dev, map);
>> +	if (IS_ERR(clk))
>> +		return PTR_ERR(clk);
>> +	onecell_data->hws[CLKID_MMC_MUX]		= &clk->hw,
> 
> It does not really need to have an ID or be in the onecell data if you are not
> going to expose it. On the other controllers, we are using the onecell for the
> registration as well, which is why there is unexpeosed IDs, but it is not the
> case here.
> 
> ... and please, stop with this unnecessary tab. a space will do. same below.
CLKID_MMC_MUX is an internal clock, and I will remove the ID and onecell date in the next version.
And I will double check and fix tabs. Thank you.
> 
>> +
>> +	div_data = devm_kzalloc(dev, sizeof(*div_data), GFP_KERNEL);
>> +	if (!div_data)
>> +		return -ENOMEM;
>> +	*div_data = mmc_clkc_div_data;
> 
> memcpy would be more appropriate.
Sure.
> 
>> +
>> +	clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
>> +						&clk->hw,
[...]
> 
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver

[Jerome Brunet]

On Thu, 2018-12-13 at 12:55 +0800, Jianxin Pan wrote:
> On 2018/12/12 0:59, Jerome Brunet wrote:
> > On Tue, 2018-12-11 at 00:04 +0800, Jianxin Pan wrote:
> > > From: Yixun Lan <yixun.lan@amlogic.com>
> > > 
> [...]
> > >  
> > > +config COMMON_CLK_MMC_MESON
> > > +	tristate "Meson MMC Sub Clock Controller Driver"
> > > +	select MFD_SYSCON
> > > +	select COMMON_CLK_AMLOGIC
> > > +	select COMMON_CLK_AMLOGIC_AUDIO
> > 
> > No it is wrong for the mmc to select AUDIO clocks.
> > If as a result of your patch sclk is needed for things, make the necessary
> > change in the Makefile.
> OK, I will add COMMON_CLK_AMLOGIC_SCLKDIV for sclk-div.

No! There is no reason to create a specific configuration for this.
please put it under COMMON_CLK_AMLOGIC

> [...]>> +#include <linux/clk.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/platform_device.h>
> > > +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
> > > +
> > > +#include "clkc.h"
> > > +#include "clkc-audio.h"
> > 
> > Again having audio in the mmc controller is wrong.
> > Please make the necessary rework.
> Yes, I will split out sclk-div.h from clkc-audio.h in the next version.

Same, clkc.h will do

> Thanks for your time.
> > > +
> > > +/* clock ID used by internal driver */
> > > +#define CLKID_MMC_MUX			0
> > > +
> > > +#define   SD_EMMC_CLOCK		0
> >            ^
> > why the multiple space here ? this looks odd
> I will check the alignement  in the whole patchset and fix them, Thank you.
> > > +#define   CLK_DELAY_STEP_PS		200
> > 
> > Please keep thing aligned aligned consistently.
> > 
> > > +
> > > +#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;
> > 
> > Why use a tab above ?
> OK
> > > +};
> > > +
> > > +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
> > > +	.offset		= SD_EMMC_CLOCK,
> > > +	.mask		= 0x3,
> > > +	.shift		= 6,
> > > +};
> > > +
> > > +static const struct meson_sclk_div_data mmc_clkc_div_data = {
> > > +	.div = {
> > > +		.reg_off = SD_EMMC_CLOCK,
> > > +		.shift   = (0),
> > > +		.width   = (6),
> > 
> > Please remove the unncessary parenthesis
> OK, I will remove them.
> > > +	},
> > > +	.hi = {
> > > +		.width   = 0,
> > > +	},
> > 
> > structure is a static const, all non-list members will be zero
> > drop the 
> OK, I will remove it in the next version. 
> > > +	.flags = 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,
> > > +		},
> > 
> > Again, an effort on alignement would appreciated, same below
> OK, I will fix them.
> > > +		.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;
> > > +
> > 
> > nitpick: remove this newline
> OK.
> > > +	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);
> > 
> > I don't think this cast is necessary     ^
> Yes, return clk is ok here.
> > > +		}
> > > +
> > > +		parent_names[i] = __clk_get_name(clk);
> > > +	}
> > > +
> > > +	init.ops = &clk_regmap_mux_ops;
> > > +	init.flags = CLK_SET_RATE_PARENT;
> > > +	init.parent_names = parent_names;
> > > +	init.num_parents = MUX_CLK_NUM_PARENTS;
> > > +
> > > +	mux = mmc_clkc_register_clk(dev, map, &init, "mux",
> > > &mmc_clkc_mux_data);
> > > +	if (IS_ERR(mux))
> > > +		dev_err(dev, "Mux clock registration failed\n");
> > > +
> > > +	return mux;
> > > +}
> > > +
> > > +static struct clk_regmap *
> > > +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap
> > > *map,
> > > +				  char *suffix, const struct clk_hw *hw,
> > > +				  unsigned long flags,
> > > +				  const struct clk_ops *ops, void *data)
> > > +{
> > > +	struct clk_init_data init;
> > > +	struct clk_regmap *clk;
> > > +	const char *parent_name = clk_hw_get_name(hw);
> > > +
> > > +	init.ops = ops;
> > > +	init.flags = flags;
> > > +	init.parent_names = &parent_name;
> > > +	init.num_parents = 1;
> > > +
> > > +	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
> > > +	if (IS_ERR(clk))
> > > +		dev_err(dev, "Core %s clock registration failed\n", suffix);
> > 
> >                                ^
> > this function is not only called by the core clock, is it ?
> OK, I will drop "Core" in this line.
> > > +
> > > +	return clk;
> > > +}
> > > +
> > > +static int mmc_clkc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct clk_hw_onecell_data *onecell_data;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct mmc_clkc_data *data;
> > > +	struct regmap *map;
> > > +	struct clk_regmap *clk, *core;
> > > +	struct meson_sclk_div_data *div_data;
> > > +
> > > +	/*cast to drop the const in match->data*/
> > > +	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> > > +	if (!data)
> > > +		return -ENODEV;
> > > +
> > > +	map = syscon_node_to_regmap(dev->of_node);
> > > +	if (IS_ERR(map)) {
> > > +		dev_err(dev, "could not find mmc clock controller\n");
> > > +		return PTR_ERR(map);
> > > +	}
> > > +
> > > +	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
> > > +				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
> > > +				    GFP_KERNEL);
> > > +	if (!onecell_data)
> > > +		return -ENOMEM;
> > > +
> > > +	clk = mmc_clkc_register_mux(dev, map);
> > > +	if (IS_ERR(clk))
> > > +		return PTR_ERR(clk);
> > > +	onecell_data->hws[CLKID_MMC_MUX]		= &clk->hw,
> > 
> > It does not really need to have an ID or be in the onecell data if you are
> > not
> > going to expose it. On the other controllers, we are using the onecell for
> > the
> > registration as well, which is why there is unexpeosed IDs, but it is not
> > the
> > case here.
> > 
> > ... and please, stop with this unnecessary tab. a space will do. same
> > below.
> CLKID_MMC_MUX is an internal clock, and I will remove the ID and onecell
> date in the next version.
> And I will double check and fix tabs. Thank you.
> > > +
> > > +	div_data = devm_kzalloc(dev, sizeof(*div_data), GFP_KERNEL);
> > > +	if (!div_data)
> > > +		return -ENOMEM;
> > > +	*div_data = mmc_clkc_div_data;
> > 
> > memcpy would be more appropriate.
> Sure.
> > > +
> > > +	clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
> > > +						&clk->hw,
> [...]
> > 
> > .
> > 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver

[Jianxin Pan]

On 2018/12/13 17:01, Jerome Brunet wrote:
> On Thu, 2018-12-13 at 12:55 +0800, Jianxin Pan wrote:
>> On 2018/12/12 0:59, Jerome Brunet wrote:
>>> On Tue, 2018-12-11 at 00:04 +0800, Jianxin Pan wrote:
>>>> From: Yixun Lan <yixun.lan@amlogic.com>
>>>>
>> [...]
>>>>  
>>>> +config COMMON_CLK_MMC_MESON
>>>> +	tristate "Meson MMC Sub Clock Controller Driver"
>>>> +	select MFD_SYSCON
>>>> +	select COMMON_CLK_AMLOGIC
>>>> +	select COMMON_CLK_AMLOGIC_AUDIO
>>>
>>> No it is wrong for the mmc to select AUDIO clocks.
>>> If as a result of your patch sclk is needed for things, make the necessary
>>> change in the Makefile.
>> OK, I will add COMMON_CLK_AMLOGIC_SCLKDIV for sclk-div.
> 
> No! There is no reason to create a specific configuration for this.
> please put it under COMMON_CLK_AMLOGIC
OK, I will use COMMON_CLK_AMLOGIC and clkc.h for sclk-div in the next version. Thank you.
> 
>> [...]>> +#include <linux/clk.h>
>>>> +#include <linux/clk-provider.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/mfd/syscon.h>
[...]



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v7 4/4] clk: meson: add one based divider support for sclk divider

[Jianxin Pan]

When CLK_DIVIDER_ONE_BASED flag is set, the sclk divider will be:
one based divider (div = val), and zero value gates the clock

Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
---
 drivers/clk/meson/clkc-audio.h |  1 +
 drivers/clk/meson/sclk-div.c   | 28 ++++++++++++++++++----------
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/meson/clkc-audio.h b/drivers/clk/meson/clkc-audio.h
index 0a7c157..9bd6ced 100644
--- a/drivers/clk/meson/clkc-audio.h
+++ b/drivers/clk/meson/clkc-audio.h
@@ -20,6 +20,7 @@ struct meson_sclk_div_data {
 	struct parm hi;
 	unsigned int cached_div;
 	struct clk_duty cached_duty;
+	u8	flags;
 };
 
 extern const struct clk_ops meson_clk_triphase_ops;
diff --git a/drivers/clk/meson/sclk-div.c b/drivers/clk/meson/sclk-div.c
index bc64019..d98707b 100644
--- a/drivers/clk/meson/sclk-div.c
+++ b/drivers/clk/meson/sclk-div.c
@@ -24,22 +24,23 @@
 	return (struct meson_sclk_div_data *)clk->data;
 }
 
-static int sclk_div_maxval(struct meson_sclk_div_data *sclk)
-{
-	return (1 << sclk->div.width) - 1;
-}
-
 static int sclk_div_maxdiv(struct meson_sclk_div_data *sclk)
 {
-	return sclk_div_maxval(sclk) + 1;
+	if (sclk->flags & CLK_DIVIDER_ONE_BASED)
+		return clk_div_mask(sclk->div.width);
+	else
+		return clk_div_mask(sclk->div.width) + 1;
 }
 
 static int sclk_div_getdiv(struct clk_hw *hw, unsigned long rate,
 			   unsigned long prate, int maxdiv)
 {
 	int div = DIV_ROUND_CLOSEST_ULL((u64)prate, rate);
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
+	int mindiv = (sclk->flags & CLK_DIVIDER_ONE_BASED) ? 1 : 2;
 
-	return clamp(div, 2, maxdiv);
+	return clamp(div, mindiv, maxdiv);
 }
 
 static int sclk_div_bestdiv(struct clk_hw *hw, unsigned long rate,
@@ -47,7 +48,7 @@ static int sclk_div_bestdiv(struct clk_hw *hw, unsigned long rate,
 			    struct meson_sclk_div_data *sclk)
 {
 	struct clk_hw *parent = clk_hw_get_parent(hw);
-	int bestdiv = 0, i;
+	int bestdiv = 0, i, mindiv;
 	unsigned long maxdiv, now, parent_now;
 	unsigned long best = 0, best_parent = 0;
 
@@ -64,8 +65,9 @@ static int sclk_div_bestdiv(struct clk_hw *hw, unsigned long rate,
 	 * unsigned long in rate * i below
 	 */
 	maxdiv = min(ULONG_MAX / rate, maxdiv);
+	mindiv = (sclk->flags & CLK_DIVIDER_ONE_BASED) ? 1 : 2;
 
-	for (i = 2; i <= maxdiv; i++) {
+	for (i = mindiv; i <= maxdiv; i++) {
 		/*
 		 * It's the most ideal case if the requested rate can be
 		 * divided from parent clock without needing to change
@@ -153,10 +155,14 @@ static int sclk_div_get_duty_cycle(struct clk_hw *hw,
 static void sclk_apply_divider(struct clk_regmap *clk,
 			       struct meson_sclk_div_data *sclk)
 {
+	unsigned int div;
+
 	if (MESON_PARM_APPLICABLE(&sclk->hi))
 		sclk_apply_ratio(clk, sclk);
 
-	meson_parm_write(clk->map, &sclk->div, sclk->cached_div - 1);
+	div = (sclk->flags & CLK_DIVIDER_ONE_BASED) ?
+		sclk->cached_div : (sclk->cached_div - 1);
+	meson_parm_write(clk->map, &sclk->div, div);
 }
 
 static int sclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -223,6 +229,8 @@ static void sclk_div_init(struct clk_hw *hw)
 	/* if the divider is initially disabled, assume max */
 	if (!val)
 		sclk->cached_div = sclk_div_maxdiv(sclk);
+	else if (sclk->flags & CLK_DIVIDER_ONE_BASED)
+		sclk->cached_div = val;
 	else
 		sclk->cached_div = val + 1;
 
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v7 4/4] clk: meson: add one based divider support for sclk divider

[Jerome Brunet]

On Tue, 2018-12-11 at 00:04 +0800, Jianxin Pan wrote:
> When CLK_DIVIDER_ONE_BASED flag is set, the sclk divider will be:
> one based divider (div = val), and zero value gates the clock
> 
> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
> ---
>  drivers/clk/meson/clkc-audio.h |  1 +
>  drivers/clk/meson/sclk-div.c   | 28 ++++++++++++++++++----------
>  2 files changed, 19 insertions(+), 10 deletions(-)

Such a patch should be done earlier in the series, at least before using sclk
in your controller, otherwise thing will be broken in between

In general, I would prefer if you had added two helper function to deal with
the translation between register value and divider value.

Only these function should care about CLK_DIVIDER_ONE_BASED, the rest should
just call them.

This, we will be able to deal the with HI (duty cycle) part as well, which you
completly skiped.

I know your device does not have this, but still the code has to make sense.

> 
> diff --git a/drivers/clk/meson/clkc-audio.h b/drivers/clk/meson/clkc-audio.h
> index 0a7c157..9bd6ced 100644
> --- a/drivers/clk/meson/clkc-audio.h
> +++ b/drivers/clk/meson/clkc-audio.h
> @@ -20,6 +20,7 @@ struct meson_sclk_div_data {
>  	struct parm hi;
>  	unsigned int cached_div;
>  	struct clk_duty cached_duty;
> +	u8	flags;
>  };
>  
>  extern const struct clk_ops meson_clk_triphase_ops;
> diff --git a/drivers/clk/meson/sclk-div.c b/drivers/clk/meson/sclk-div.c
> index bc64019..d98707b 100644
> --- a/drivers/clk/meson/sclk-div.c
> +++ b/drivers/clk/meson/sclk-div.c
> @@ -24,22 +24,23 @@
>  	return (struct meson_sclk_div_data *)clk->data;
>  }
>  
> -static int sclk_div_maxval(struct meson_sclk_div_data *sclk)
> -{
> -	return (1 << sclk->div.width) - 1;
> -}
> -
>  static int sclk_div_maxdiv(struct meson_sclk_div_data *sclk)
>  {
> -	return sclk_div_maxval(sclk) + 1;
> +	if (sclk->flags & CLK_DIVIDER_ONE_BASED)
> +		return clk_div_mask(sclk->div.width);
> +	else
> +		return clk_div_mask(sclk->div.width) + 1;

seems over complicated.
why no call clk_div_mask just once, and add 1 if necessary ?

>  }
>  
>  static int sclk_div_getdiv(struct clk_hw *hw, unsigned long rate,
>  			   unsigned long prate, int maxdiv)
>  {
>  	int div = DIV_ROUND_CLOSEST_ULL((u64)prate, rate);
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
> +	int mindiv = (sclk->flags & CLK_DIVIDER_ONE_BASED) ? 1 : 2;

This is why I want helpers, don't like this above

>  
> -	return clamp(div, 2, maxdiv);
> +	return clamp(div, mindiv, maxdiv);
>  }
>  
>  static int sclk_div_bestdiv(struct clk_hw *hw, unsigned long rate,
> @@ -47,7 +48,7 @@ static int sclk_div_bestdiv(struct clk_hw *hw, unsigned
> long rate,
>  			    struct meson_sclk_div_data *sclk)
>  {
>  	struct clk_hw *parent = clk_hw_get_parent(hw);
> -	int bestdiv = 0, i;
> +	int bestdiv = 0, i, mindiv;
>  	unsigned long maxdiv, now, parent_now;
>  	unsigned long best = 0, best_parent = 0;
>  
> @@ -64,8 +65,9 @@ static int sclk_div_bestdiv(struct clk_hw *hw, unsigned
> long rate,
>  	 * unsigned long in rate * i below
>  	 */
>  	maxdiv = min(ULONG_MAX / rate, maxdiv);
> +	mindiv = (sclk->flags & CLK_DIVIDER_ONE_BASED) ? 1 : 2;
>  
> -	for (i = 2; i <= maxdiv; i++) {
> +	for (i = mindiv; i <= maxdiv; i++) {
>  		/*
>  		 * It's the most ideal case if the requested rate can be
>  		 * divided from parent clock without needing to change
> @@ -153,10 +155,14 @@ static int sclk_div_get_duty_cycle(struct clk_hw *hw,
>  static void sclk_apply_divider(struct clk_regmap *clk,
>  			       struct meson_sclk_div_data *sclk)
>  {
> +	unsigned int div;
> +
>  	if (MESON_PARM_APPLICABLE(&sclk->hi))
>  		sclk_apply_ratio(clk, sclk);
>  
> -	meson_parm_write(clk->map, &sclk->div, sclk->cached_div - 1);
> +	div = (sclk->flags & CLK_DIVIDER_ONE_BASED) ?
> +		sclk->cached_div : (sclk->cached_div - 1);

helpers again.

> +	meson_parm_write(clk->map, &sclk->div, div);
>  }
>  
>  static int sclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
> @@ -223,6 +229,8 @@ static void sclk_div_init(struct clk_hw *hw)
>  	/* if the divider is initially disabled, assume max */
>  	if (!val)
>  		sclk->cached_div = sclk_div_maxdiv(sclk);
> +	else if (sclk->flags & CLK_DIVIDER_ONE_BASED)
> +		sclk->cached_div = val;
>  	else
>  		sclk->cached_div = val + 1;

same ...

>  



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v7 4/4] clk: meson: add one based divider support for sclk divider

[Jianxin Pan]

Hi Jerome,

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

On 2018/12/12 1:16, Jerome Brunet wrote:
> On Tue, 2018-12-11 at 00:04 +0800, Jianxin Pan wrote:
>> When CLK_DIVIDER_ONE_BASED flag is set, the sclk divider will be:
>> one based divider (div = val), and zero value gates the clock
>>
>> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
>> ---
>>  drivers/clk/meson/clkc-audio.h |  1 +
>>  drivers/clk/meson/sclk-div.c   | 28 ++++++++++++++++++----------
>>  2 files changed, 19 insertions(+), 10 deletions(-)
> 
> Such a patch should be done earlier in the series, at least before using sclk
> in your controller, otherwise thing will be broken in between
> 
I will move it to the first one of the patchset. 
> In general, I would prefer if you had added two helper function to deal with
> the translation between register value and divider value.
> 
> Only these function should care about CLK_DIVIDER_ONE_BASED, the rest should
> just call them.
> 
> This, we will be able to deal the with HI (duty cycle) part as well, which you
> completly skiped.
> 
> I know your device does not have this, but still the code has to make sense.
> 
OK, I will add two helper for value and register translation, and then appy them to both div and hi.
>>
>> diff --git a/drivers/clk/meson/clkc-audio.h b/drivers/clk/meson/clkc-audio.h
>> index 0a7c157..9bd6ced 100644
>> --- a/drivers/clk/meson/clkc-audio.h
>> +++ b/drivers/clk/meson/clkc-audio.h
>> @@ -20,6 +20,7 @@ struct meson_sclk_div_data {
>>  	struct parm hi;
>>  	unsigned int cached_div;
>>  	struct clk_duty cached_duty;
>> +	u8	flags;
>>  };
>>  
>>  extern const struct clk_ops meson_clk_triphase_ops;
>> diff --git a/drivers/clk/meson/sclk-div.c b/drivers/clk/meson/sclk-div.c
>> index bc64019..d98707b 100644
>> --- a/drivers/clk/meson/sclk-div.c
>> +++ b/drivers/clk/meson/sclk-div.c
>> @@ -24,22 +24,23 @@
>>  	return (struct meson_sclk_div_data *)clk->data;
>>  }
>>  
>> -static int sclk_div_maxval(struct meson_sclk_div_data *sclk)
>> -{
>> -	return (1 << sclk->div.width) - 1;
>> -}
>> -
>>  static int sclk_div_maxdiv(struct meson_sclk_div_data *sclk)
>>  {
>> -	return sclk_div_maxval(sclk) + 1;
>> +	if (sclk->flags & CLK_DIVIDER_ONE_BASED)
>> +		return clk_div_mask(sclk->div.width);
>> +	else
>> +		return clk_div_mask(sclk->div.width) + 1;
> 
> seems over complicated.
> why no call clk_div_mask just once, and add 1 if necessary ?
Yes, I will use helper here.
> 
>>  }
>>  
>>  static int sclk_div_getdiv(struct clk_hw *hw, unsigned long rate,
>>  			   unsigned long prate, int maxdiv)
>>  {
>>  	int div = DIV_ROUND_CLOSEST_ULL((u64)prate, rate);
>> +	struct clk_regmap *clk = to_clk_regmap(hw);
>> +	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
>> +	int mindiv = (sclk->flags & CLK_DIVIDER_ONE_BASED) ? 1 : 2;
> 
> This is why I want helpers, don't like this above
OK, I will replace it with helpers.
> 
>>  
>> -	return clamp(div, 2, maxdiv);
>> +	return clamp(div, mindiv, maxdiv);
>>  }
>>  
>>  static int sclk_div_bestdiv(struct clk_hw *hw, unsigned long rate,
>> @@ -47,7 +48,7 @@ static int sclk_div_bestdiv(struct clk_hw *hw, unsigned
>> long rate,
>>  			    struct meson_sclk_div_data *sclk)
>>  {
>>  	struct clk_hw *parent = clk_hw_get_parent(hw);
>> -	int bestdiv = 0, i;
>> +	int bestdiv = 0, i, mindiv;
>>  	unsigned long maxdiv, now, parent_now;
>>  	unsigned long best = 0, best_parent = 0;
>>  
>> @@ -64,8 +65,9 @@ static int sclk_div_bestdiv(struct clk_hw *hw, unsigned
>> long rate,
>>  	 * unsigned long in rate * i below
>>  	 */
>>  	maxdiv = min(ULONG_MAX / rate, maxdiv);
>> +	mindiv = (sclk->flags & CLK_DIVIDER_ONE_BASED) ? 1 : 2;
>>  
>> -	for (i = 2; i <= maxdiv; i++) {
>> +	for (i = mindiv; i <= maxdiv; i++) {
>>  		/*
>>  		 * It's the most ideal case if the requested rate can be
>>  		 * divided from parent clock without needing to change
>> @@ -153,10 +155,14 @@ static int sclk_div_get_duty_cycle(struct clk_hw *hw,
>>  static void sclk_apply_divider(struct clk_regmap *clk,
>>  			       struct meson_sclk_div_data *sclk)
>>  {
>> +	unsigned int div;
>> +
>>  	if (MESON_PARM_APPLICABLE(&sclk->hi))
>>  		sclk_apply_ratio(clk, sclk);
>>  
>> -	meson_parm_write(clk->map, &sclk->div, sclk->cached_div - 1);
>> +	div = (sclk->flags & CLK_DIVIDER_ONE_BASED) ?
>> +		sclk->cached_div : (sclk->cached_div - 1);
> 
> helpers again.
OK!
> 
>> +	meson_parm_write(clk->map, &sclk->div, div);
>>  }
>>  
>>  static int sclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
>> @@ -223,6 +229,8 @@ static void sclk_div_init(struct clk_hw *hw)
>>  	/* if the divider is initially disabled, assume max */
>>  	if (!val)
>>  		sclk->cached_div = sclk_div_maxdiv(sclk);
>> +	else if (sclk->flags & CLK_DIVIDER_ONE_BASED)
>> +		sclk->cached_div = val;
>>  	else
>>  		sclk->cached_div = val + 1;
> 
> same ...
OK, I will fix them. Thanks again for your time!
> 
>>  
> 
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel