| Message ID | 1544457877-51301-4-git-send-email-jianxin.pan@amlogic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: meson: add a sub EMMC clock controller support | expand |
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");
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, [...] > > > . >
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, > [...] > > > > . > >
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> [...]
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");