| Message ID | 20201114135044.724385-4-daniel@0x0f.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/6] dt-bindings: clk: mstar msc313 mpll binding header | expand |
Quoting Daniel Palmer (2020-11-14 05:50:41) > F: include/dt-bindings/gpio/msc313-gpio.h > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index c715d4681a0b..a002f2605fa3 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -370,6 +370,7 @@ source "drivers/clk/ingenic/Kconfig" > source "drivers/clk/keystone/Kconfig" > source "drivers/clk/mediatek/Kconfig" > source "drivers/clk/meson/Kconfig" > +source "drivers/clk/mstar/Kconfig" > source "drivers/clk/mvebu/Kconfig" > source "drivers/clk/qcom/Kconfig" > source "drivers/clk/renesas/Kconfig" This looks good. > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index da8fcf147eb1..b758aae17ab8 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -124,3 +124,4 @@ endif > obj-$(CONFIG_ARCH_ZX) += zte/ > obj-$(CONFIG_ARCH_ZYNQ) += zynq/ > obj-$(CONFIG_COMMON_CLK_ZYNQMP) += zynqmp/ > +obj-$(CONFIG_ARCH_MSTARV7) += mstar/ This is in the wrong place. It looks to be sorted based on the path name, so mstar/ comes much earlier in this file. > diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig > new file mode 100644 > index 000000000000..23765edde3af > --- /dev/null > +++ b/drivers/clk/mstar/Kconfig > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +config MSTAR_MSC313_MPLL > + bool > + select REGMAP > + select REGMAP_MMIO > diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile > new file mode 100644 > index 000000000000..f8dcd25ede1d > --- /dev/null > +++ b/drivers/clk/mstar/Makefile > @@ -0,0 +1,6 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile for mstar specific clk > +# > + > +obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.o > diff --git a/drivers/clk/mstar/clk-msc313-mpll.c b/drivers/clk/mstar/clk-msc313-mpll.c > new file mode 100644 > index 000000000000..c1e2fe0fc412 > --- /dev/null > +++ b/drivers/clk/mstar/clk-msc313-mpll.c > @@ -0,0 +1,177 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MStar MSC313 MPLL driver > + * > + * Copyright (C) 2020 Daniel Palmer <daniel@thingy.jp> > + */ > + > +#include <linux/platform_device.h> > +#include <linux/of.h> > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> Please remove unused includes > +#include <linux/of_address.h> > +#include <linux/module.h> Isn't it builtin? This include should be removed. > +#include <linux/regmap.h> > + > +#define REG_CONFIG1 0x8 > +#define REG_CONFIG2 0xc > + > +static const struct regmap_config msc313_mpll_regmap_config = { > + .reg_bits = 16, > + .val_bits = 16, > + .reg_stride = 4, > +}; > + > +static const struct reg_field config1_loop_div_first = REG_FIELD(REG_CONFIG1, 8, 9); > +static const struct reg_field config1_input_div_first = REG_FIELD(REG_CONFIG1, 4, 5); > +static const struct reg_field config2_output_div_first = REG_FIELD(REG_CONFIG2, 12, 13); > +static const struct reg_field config2_loop_div_second = REG_FIELD(REG_CONFIG2, 0, 7); > + > +static const unsigned int output_dividers[] = { > + 2, 3, 4, 5, 6, 7, 10 > +}; > + > +#define NUMOUTPUTS (ARRAY_SIZE(output_dividers) + 1) > + > +struct msc313_mpll { > + struct clk_hw clk_hw; > + struct regmap_field *input_div; > + struct regmap_field *loop_div_first; > + struct regmap_field *loop_div_second; > + struct regmap_field *output_div; > + struct clk_hw_onecell_data *clk_data; > +}; > + > +#define to_mpll(_hw) container_of(_hw, struct msc313_mpll, clk_hw) > +#define to_divider_hw(_mpll, _which) _mpll->clk_data->hws[_which + 1] I'd rather not have this macro. It's confusing given that to_foo() macros are usually a container_of() invocation. Given that it's only used in the registration/unregistration loops please inline it and use a local variable. > + > +static unsigned long msc313_mpll_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct msc313_mpll *mpll = to_mpll(hw); > + unsigned int input_div, output_div, loop_first, loop_second; > + unsigned long output_rate; > + > + regmap_field_read(mpll->input_div, &input_div); > + regmap_field_read(mpll->output_div, &output_div); > + regmap_field_read(mpll->loop_div_first, &loop_first); > + regmap_field_read(mpll->loop_div_second, &loop_second); > + > + output_rate = parent_rate / (1 << input_div); > + output_rate *= (1 << loop_first) * max(loop_second, 1U); > + output_rate /= max(output_div, 1U); > + > + return output_rate; > +} > + > +static const struct clk_ops msc313_mpll_ops = { > + .recalc_rate = msc313_mpll_recalc_rate, Weird double indent here. > +}; > + > +static int msc313_mpll_probe(struct platform_device *pdev) > +{ > + void __iomem *base; > + struct msc313_mpll *mpll; > + struct clk_init_data clk_init; > + struct device *dev = &pdev->dev; > + struct regmap *regmap; > + const char *parents[1], *outputnames[NUMOUTPUTS]; > + const int numparents = ARRAY_SIZE(parents); > + int ret, i; > + > + if (of_clk_parent_fill(dev->of_node, parents, numparents) != numparents) > + return -EINVAL; > + > + if (of_property_read_string_array(pdev->dev.of_node, "clock-output-names", Hopefully this isn't required. > + outputnames, NUMOUTPUTS) != NUMOUTPUTS) > + return -EINVAL; > + > + mpll = devm_kzalloc(dev, sizeof(*mpll), GFP_KERNEL); > + if (!mpll) > + return -ENOMEM; > + > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + regmap = devm_regmap_init_mmio(dev, base, &msc313_mpll_regmap_config); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + mpll->input_div = devm_regmap_field_alloc(dev, regmap, config1_input_div_first); > + if (IS_ERR(mpll->input_div)) > + return PTR_ERR(mpll->input_div); > + mpll->output_div = devm_regmap_field_alloc(dev, regmap, config2_output_div_first); > + if (IS_ERR(mpll->output_div)) > + return PTR_ERR(mpll->output_div); > + mpll->loop_div_first = devm_regmap_field_alloc(dev, regmap, config1_loop_div_first); > + if (IS_ERR(mpll->loop_div_first)) > + return PTR_ERR(mpll->loop_div_first); > + mpll->loop_div_second = devm_regmap_field_alloc(dev, regmap, config2_loop_div_second); > + if (IS_ERR(mpll->loop_div_second)) > + return PTR_ERR(mpll->loop_div_second); > + > + mpll->clk_data = devm_kzalloc(dev, struct_size(mpll->clk_data, hws, > + ARRAY_SIZE(output_dividers)), GFP_KERNEL); > + if (!mpll->clk_data) > + return -ENOMEM; > + > + clk_init.name = outputnames[0]; > + clk_init.ops = &msc313_mpll_ops; > + clk_init.num_parents = 1; > + clk_init.parent_names = parents; > + mpll->clk_hw.init = &clk_init; > + > + ret = devm_clk_hw_register(dev, &mpll->clk_hw); > + if (ret) > + return ret; > + > + mpll->clk_data->num = NUMOUTPUTS; > + mpll->clk_data->hws[0] = &mpll->clk_hw; > + > + for (i = 0; i < ARRAY_SIZE(output_dividers); i++) { > + to_divider_hw(mpll, i) = clk_hw_register_fixed_factor(dev, > + outputnames[i + 1], outputnames[0], 0, 1, output_dividers[i]); > + if (IS_ERR(to_divider_hw(mpll, i))) { > + ret = PTR_ERR(to_divider_hw(mpll, i)); > + goto unregister_dividers; > + } > + } > + > + platform_set_drvdata(pdev, mpll); > + > + return devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, > + mpll->clk_data); > + > +unregister_dividers: > + for (i--; i >= 0; i--) > + clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i)); > + return ret; > +} > + > +static int msc313_mpll_remove(struct platform_device *pdev) > +{ > + struct msc313_mpll *mpll = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(output_dividers); i++) > + clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i)); Maybe add a devm_ for this if it doesn't exist. > + > + return 0; > +} > + > +static const struct of_device_id msc313_mpll_of_match[] = { > + { .compatible = "mstar,msc313-mpll", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, msc313_mpll_of_match); Drop this. It isn't a module. > + > +static struct platform_driver msc313_mpll_driver = { > + .driver = { > + .name = "mstar-msc313-mpll", > + .of_match_table = msc313_mpll_of_match, > + }, > + .probe = msc313_mpll_probe, > + .remove = msc313_mpll_remove, > +}; > +builtin_platform_driver(msc313_mpll_driver);
Hi Stephen, On Sun, 20 Dec 2020 at 13:36, Stephen Boyd <sboyd@kernel.org> wrote: > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > > index da8fcf147eb1..b758aae17ab8 100644 > > --- a/drivers/clk/Makefile > > +++ b/drivers/clk/Makefile > > @@ -124,3 +124,4 @@ endif > > obj-$(CONFIG_ARCH_ZX) += zte/ > > obj-$(CONFIG_ARCH_ZYNQ) += zynq/ > > obj-$(CONFIG_COMMON_CLK_ZYNQMP) += zynqmp/ > > +obj-$(CONFIG_ARCH_MSTARV7) += mstar/ > > This is in the wrong place. It looks to be sorted based on the path > name, so mstar/ comes much earlier in this file. Noted. I'll fix this. > > + > > +#include <linux/platform_device.h> > > +#include <linux/of.h> > > +#include <linux/clk-provider.h> > > +#include <linux/clkdev.h> > > Please remove unused includes > > > +#include <linux/of_address.h> > > +#include <linux/module.h> > > Isn't it builtin? This include should be removed. Yes. Sorry it was originally a module and there are some leftovers I didn't clean up when I changed it to builtin. > > +#define to_mpll(_hw) container_of(_hw, struct msc313_mpll, clk_hw) > > +#define to_divider_hw(_mpll, _which) _mpll->clk_data->hws[_which + 1] > > I'd rather not have this macro. It's confusing given that to_foo() > macros are usually a container_of() invocation. Given that it's only > used in the registration/unregistration loops please inline it and use a > local variable. > Ok I'll rework that. > > + > > +static int msc313_mpll_remove(struct platform_device *pdev) > > +{ > > + struct msc313_mpll *mpll = platform_get_drvdata(pdev); > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(output_dividers); i++) > > + clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i)); > > Maybe add a devm_ for this if it doesn't exist. I did think about adding this. Would I need to do that in a separate series or would it be ok to roll it into this one? Thanks, Daniel
Quoting Daniel Palmer (2020-12-19 22:42:40) > > > + > > > +static int msc313_mpll_remove(struct platform_device *pdev) > > > +{ > > > + struct msc313_mpll *mpll = platform_get_drvdata(pdev); > > > + int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(output_dividers); i++) > > > + clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i)); > > > > Maybe add a devm_ for this if it doesn't exist. > > I did think about adding this. Would I need to do that in a separate > series or would it be ok to roll it into this one? > Can be part of the same series.
Hi Stephen,
On Mon, 21 Dec 2020 at 03:43, Stephen Boyd <sboyd@kernel.org> wrote:
> Can be part of the same series.
Ok. I have added a small patch that adds devm_clk_hw_register_fixed_factor().
I'll send that in the v2 once the clock names issue is resolved.
Thanks,
Daniel
diff --git a/MAINTAINERS b/MAINTAINERS index d1c98a6f9f24..1783022b4aa8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2135,6 +2135,7 @@ F: Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml F: Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml F: arch/arm/boot/dts/mstar-* F: arch/arm/mach-mstar/ +F: drivers/clk/mstar/ F: drivers/gpio/gpio-msc313.c F: include/dt-bindings/clock/mstar-* F: include/dt-bindings/gpio/msc313-gpio.h diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index c715d4681a0b..a002f2605fa3 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -370,6 +370,7 @@ source "drivers/clk/ingenic/Kconfig" source "drivers/clk/keystone/Kconfig" source "drivers/clk/mediatek/Kconfig" source "drivers/clk/meson/Kconfig" +source "drivers/clk/mstar/Kconfig" source "drivers/clk/mvebu/Kconfig" source "drivers/clk/qcom/Kconfig" source "drivers/clk/renesas/Kconfig" diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index da8fcf147eb1..b758aae17ab8 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -124,3 +124,4 @@ endif obj-$(CONFIG_ARCH_ZX) += zte/ obj-$(CONFIG_ARCH_ZYNQ) += zynq/ obj-$(CONFIG_COMMON_CLK_ZYNQMP) += zynqmp/ +obj-$(CONFIG_ARCH_MSTARV7) += mstar/ diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig new file mode 100644 index 000000000000..23765edde3af --- /dev/null +++ b/drivers/clk/mstar/Kconfig @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: GPL-2.0-only +config MSTAR_MSC313_MPLL + bool + select REGMAP + select REGMAP_MMIO diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile new file mode 100644 index 000000000000..f8dcd25ede1d --- /dev/null +++ b/drivers/clk/mstar/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Makefile for mstar specific clk +# + +obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.o diff --git a/drivers/clk/mstar/clk-msc313-mpll.c b/drivers/clk/mstar/clk-msc313-mpll.c new file mode 100644 index 000000000000..c1e2fe0fc412 --- /dev/null +++ b/drivers/clk/mstar/clk-msc313-mpll.c @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * MStar MSC313 MPLL driver + * + * Copyright (C) 2020 Daniel Palmer <daniel@thingy.jp> + */ + +#include <linux/platform_device.h> +#include <linux/of.h> +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/of_address.h> +#include <linux/module.h> +#include <linux/regmap.h> + +#define REG_CONFIG1 0x8 +#define REG_CONFIG2 0xc + +static const struct regmap_config msc313_mpll_regmap_config = { + .reg_bits = 16, + .val_bits = 16, + .reg_stride = 4, +}; + +static const struct reg_field config1_loop_div_first = REG_FIELD(REG_CONFIG1, 8, 9); +static const struct reg_field config1_input_div_first = REG_FIELD(REG_CONFIG1, 4, 5); +static const struct reg_field config2_output_div_first = REG_FIELD(REG_CONFIG2, 12, 13); +static const struct reg_field config2_loop_div_second = REG_FIELD(REG_CONFIG2, 0, 7); + +static const unsigned int output_dividers[] = { + 2, 3, 4, 5, 6, 7, 10 +}; + +#define NUMOUTPUTS (ARRAY_SIZE(output_dividers) + 1) + +struct msc313_mpll { + struct clk_hw clk_hw; + struct regmap_field *input_div; + struct regmap_field *loop_div_first; + struct regmap_field *loop_div_second; + struct regmap_field *output_div; + struct clk_hw_onecell_data *clk_data; +}; + +#define to_mpll(_hw) container_of(_hw, struct msc313_mpll, clk_hw) +#define to_divider_hw(_mpll, _which) _mpll->clk_data->hws[_which + 1] + +static unsigned long msc313_mpll_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct msc313_mpll *mpll = to_mpll(hw); + unsigned int input_div, output_div, loop_first, loop_second; + unsigned long output_rate; + + regmap_field_read(mpll->input_div, &input_div); + regmap_field_read(mpll->output_div, &output_div); + regmap_field_read(mpll->loop_div_first, &loop_first); + regmap_field_read(mpll->loop_div_second, &loop_second); + + output_rate = parent_rate / (1 << input_div); + output_rate *= (1 << loop_first) * max(loop_second, 1U); + output_rate /= max(output_div, 1U); + + return output_rate; +} + +static const struct clk_ops msc313_mpll_ops = { + .recalc_rate = msc313_mpll_recalc_rate, +}; + +static int msc313_mpll_probe(struct platform_device *pdev) +{ + void __iomem *base; + struct msc313_mpll *mpll; + struct clk_init_data clk_init; + struct device *dev = &pdev->dev; + struct regmap *regmap; + const char *parents[1], *outputnames[NUMOUTPUTS]; + const int numparents = ARRAY_SIZE(parents); + int ret, i; + + if (of_clk_parent_fill(dev->of_node, parents, numparents) != numparents) + return -EINVAL; + + if (of_property_read_string_array(pdev->dev.of_node, "clock-output-names", + outputnames, NUMOUTPUTS) != NUMOUTPUTS) + return -EINVAL; + + mpll = devm_kzalloc(dev, sizeof(*mpll), GFP_KERNEL); + if (!mpll) + return -ENOMEM; + + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) + return PTR_ERR(base); + + regmap = devm_regmap_init_mmio(dev, base, &msc313_mpll_regmap_config); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + mpll->input_div = devm_regmap_field_alloc(dev, regmap, config1_input_div_first); + if (IS_ERR(mpll->input_div)) + return PTR_ERR(mpll->input_div); + mpll->output_div = devm_regmap_field_alloc(dev, regmap, config2_output_div_first); + if (IS_ERR(mpll->output_div)) + return PTR_ERR(mpll->output_div); + mpll->loop_div_first = devm_regmap_field_alloc(dev, regmap, config1_loop_div_first); + if (IS_ERR(mpll->loop_div_first)) + return PTR_ERR(mpll->loop_div_first); + mpll->loop_div_second = devm_regmap_field_alloc(dev, regmap, config2_loop_div_second); + if (IS_ERR(mpll->loop_div_second)) + return PTR_ERR(mpll->loop_div_second); + + mpll->clk_data = devm_kzalloc(dev, struct_size(mpll->clk_data, hws, + ARRAY_SIZE(output_dividers)), GFP_KERNEL); + if (!mpll->clk_data) + return -ENOMEM; + + clk_init.name = outputnames[0]; + clk_init.ops = &msc313_mpll_ops; + clk_init.num_parents = 1; + clk_init.parent_names = parents; + mpll->clk_hw.init = &clk_init; + + ret = devm_clk_hw_register(dev, &mpll->clk_hw); + if (ret) + return ret; + + mpll->clk_data->num = NUMOUTPUTS; + mpll->clk_data->hws[0] = &mpll->clk_hw; + + for (i = 0; i < ARRAY_SIZE(output_dividers); i++) { + to_divider_hw(mpll, i) = clk_hw_register_fixed_factor(dev, + outputnames[i + 1], outputnames[0], 0, 1, output_dividers[i]); + if (IS_ERR(to_divider_hw(mpll, i))) { + ret = PTR_ERR(to_divider_hw(mpll, i)); + goto unregister_dividers; + } + } + + platform_set_drvdata(pdev, mpll); + + return devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, + mpll->clk_data); + +unregister_dividers: + for (i--; i >= 0; i--) + clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i)); + return ret; +} + +static int msc313_mpll_remove(struct platform_device *pdev) +{ + struct msc313_mpll *mpll = platform_get_drvdata(pdev); + int i; + + for (i = 0; i < ARRAY_SIZE(output_dividers); i++) + clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i)); + + return 0; +} + +static const struct of_device_id msc313_mpll_of_match[] = { + { .compatible = "mstar,msc313-mpll", }, + {} +}; +MODULE_DEVICE_TABLE(of, msc313_mpll_of_match); + +static struct platform_driver msc313_mpll_driver = { + .driver = { + .name = "mstar-msc313-mpll", + .of_match_table = msc313_mpll_of_match, + }, + .probe = msc313_mpll_probe, + .remove = msc313_mpll_remove, +}; +builtin_platform_driver(msc313_mpll_driver);
This adds a basic driver for the MPLL block found in MStar/SigmaStar ARMv7 SoCs. Currently this driver is only good for calculating the rates of it's outputs and the actual configuration must be done before the kernel boots. Usually this is done even before u-boot starts. This driver targets the MPLL block found in the MSC313/MSC313E but there is no documentation this chip so the register descriptions for the another MStar chip the MST786 were used as they seem to match. Signed-off-by: Daniel Palmer <daniel@0x0f.com> --- MAINTAINERS | 1 + drivers/clk/Kconfig | 1 + drivers/clk/Makefile | 1 + drivers/clk/mstar/Kconfig | 5 + drivers/clk/mstar/Makefile | 6 + drivers/clk/mstar/clk-msc313-mpll.c | 177 ++++++++++++++++++++++++++++ 6 files changed, 191 insertions(+) create mode 100644 drivers/clk/mstar/Kconfig create mode 100644 drivers/clk/mstar/Makefile create mode 100644 drivers/clk/mstar/clk-msc313-mpll.c