Re: [PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver
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 >> [...] +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 +#include +#include +#include +#include +#include +#include [...]
Re: [PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver
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
> > >
> [...]
> > >
> > > +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
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#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_MUX0
> > > +
> > > +#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 = _clkc_gx_data
> > > + },
Re: [PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver
On 2018/12/12 0:59, Jerome Brunet wrote:
> On Tue, 2018-12-11 at 00:04 +0800, Jianxin Pan wrote:
>> From: Yixun Lan
>>
[...]
>>
>> +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
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#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_CLKS5
>> +
>> +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 = _clkc_gx_data
>> +},
>> +{
>> +.compatible = "amlogic,axg-mmc-clkc",
>> +.data = _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,
>> +
Re: [PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver
On Tue, 2018-12-11 at 00:04 +0800, Jianxin Pan wrote:
> From: Yixun Lan
>
> 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
> Signed-off-by: Jianxin Pan
> ---
> 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 000..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
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan
> + * Author: Jianxin Pan
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#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_MUX0
> +
> +#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
[PATCH RESEND v7 3/4] clk: meson: add sub MMC clock controller driver
From: Yixun Lan
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
Signed-off-by: Jianxin Pan
---
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 000..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
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan
+ * Author: Jianxin Pan
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "clkc.h"
+#include "clkc-audio.h"
+
+/* clock ID used by internal driver */
+#define CLKID_MMC_MUX 0
+
+#define SD_EMMC_CLOCK0
+#define CLK_DELAY_STEP_PS200
+
+#define MUX_CLK_NUM_PARENTS2
+#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 =
