[0/6] ARM: mstar: Basic MPLL support

This series adds support for the MPLL block that is present in
MStar/SigmaStar ARMv7 SoCs.

This block is intended to be set and forgotten about before
Linux is running so all it actually does it read the registers
and calculate what the output frequencies should be.

We only care about this block because there are upstream dividers,
gates, muxes etc that need something between the input crystal
and themselves to calculate their own rates.

I think this needs to be applied after the GPIO series[0] that
is currently inflight.

0 - https://lore.kernel.org/linux-gpio/20201109121731.1537580-1-daniel@0x0f.com/

Daniel Palmer (6):
  dt-bindings: clk: mstar msc313 mpll binding header
  dt-bindings: clk: mstar msc313 mpll binding description
  clk: mstar: MStar/SigmaStar MPLL driver
  ARM: mstar: Select MSTAR_MSC313_MPLL
  ARM: mstar: Add the external clocks to the base dsti
  ARM: mstar: Add mpll to base dtsi

 .../bindings/clock/mstar,msc313-mpll.yaml     |  58 ++++++
 MAINTAINERS                                   |   3 +
 arch/arm/boot/dts/mstar-v7.dtsi               |  27 +++
 arch/arm/mach-mstar/Kconfig                   |   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 ++++++++++++++++++
 include/dt-bindings/clock/mstar-msc313-mpll.h |  19 ++
 10 files changed, 298 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
 create mode 100644 drivers/clk/mstar/Kconfig
 create mode 100644 drivers/clk/mstar/Makefile
 create mode 100644 drivers/clk/mstar/clk-msc313-mpll.c
 create mode 100644 include/dt-bindings/clock/mstar-msc313-mpll.h

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