[v2,0/9] ARM: mstar: cpupll

This series adds a basic driver for the PLL that generates
the cpu clock on MStar/SigmaStar ARMv7 SoCs.

Unfortunately there isn't much documentation for this thing
so there are few magic values and guesses.

This needs to come after the MPLL DT changes.

Changes since v2:
- Re-worked the series and ensure that 'make dt_binding_check' passes.
  The required commit is merged now, so it is okay.
- Fixed coding style issues in the driver and makes check_patch.pl happy
- Added one more commit for extending the opp_table for infinity2m.

Daniel Palmer (8):
  dt-bindings: clk: mstar msc313 cpupll binding description
  clk: mstar: msc313 cpupll clk driver
  ARM: mstar: Add cpupll to base dtsi
  ARM: mstar: Link cpupll to cpu
  ARM: mstar: Link cpupll to second core
  ARM: mstar: Add OPP table for infinity
  ARM: mstar: Add OPP table for infinity3
  ARM: mstar: Add OPP table for mercury5

Romain Perier (1):
  ARM: mstar: Extend opp_table for infinity2m

 .../bindings/clock/mstar,msc313-cpupll.yaml   |  45 ++++
 arch/arm/boot/dts/mstar-infinity.dtsi         |  34 +++
 arch/arm/boot/dts/mstar-infinity2m.dtsi       |  17 ++
 arch/arm/boot/dts/mstar-infinity3.dtsi        |  58 +++++
 arch/arm/boot/dts/mstar-mercury5.dtsi         |  36 +++
 arch/arm/boot/dts/mstar-v7.dtsi               |   9 +
 drivers/clk/mstar/Kconfig                     |   7 +
 drivers/clk/mstar/Makefile                    |   1 +
 drivers/clk/mstar/clk-msc313-cpupll.c         | 227 ++++++++++++++++++
 9 files changed, 434 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-cpupll.yaml
 create mode 100644 drivers/clk/mstar/clk-msc313-cpupll.c

Quoting Romain Perier (2022-01-02 08:57:23)
> diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig
> index de37e1bce2d2..a44ca2b180ff 100644
> --- a/drivers/clk/mstar/Kconfig
> +++ b/drivers/clk/mstar/Kconfig
> @@ -7,3 +7,10 @@ config MSTAR_MSC313_MPLL
>         help
>           Support for the MPLL PLL and dividers block present on
>           MStar/Sigmastar SoCs.
> +
> +config MSTAR_MSC313_CPUPLL

Can this file be sorted on Kconfig name?

> +       bool "MStar CPUPLL driver"
> +       depends on ARCH_MSTARV7 || COMPILE_TEST
> +       default ARCH_MSTARV7
> +       help
> +         Support for the CPU PLL present on MStar/Sigmastar SoCs.
> diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile
> index f8dcd25ede1d..9f05b73a0619 100644
> --- a/drivers/clk/mstar/Makefile
> +++ b/drivers/clk/mstar/Makefile
> @@ -4,3 +4,4 @@
>  #
>  
>  obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.o
> +obj-$(CONFIG_MSTAR_MSC313_CPUPLL) += clk-msc313-cpupll.o
> diff --git a/drivers/clk/mstar/clk-msc313-cpupll.c b/drivers/clk/mstar/clk-msc313-cpupll.c
> new file mode 100644
> index 000000000000..2229b16475eb
> --- /dev/null
> +++ b/drivers/clk/mstar/clk-msc313-cpupll.c
> @@ -0,0 +1,227 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Daniel Palmer <daniel@thingy.jp>
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>

Is this include used? Please drop unused includes.

> +#include <linux/of_address.h>
> +#include <linux/module.h>

It isn't a module though.

> +#include <linux/kernel.h>

Preferably sort includes alphabetically too.

> +
> +/*
> + * This IP is not documented outside of the messy vendor driver.
> + * Below is what we think the registers look like based on looking at
> + * the vendor code and poking at the hardware:
> + *
> + * 0x140 -- LPF low. Seems to store one half of the clock transition
> + * 0x144 /
> + * 0x148 -- LPF high. Seems to store one half of the clock transition
> + * 0x14c /
> + * 0x150 -- vendor code says "toggle lpf enable"
> + * 0x154 -- mu?
> + * 0x15c -- lpf_update_count?
> + * 0x160 -- vendor code says "switch to LPF". Clock source config? Register bank?
> + * 0x164 -- vendor code says "from low to high" which seems to mean transition from LPF low to
> + * LPF high.
> + * 0x174 -- Seems to be the PLL lock status bit
> + * 0x180 -- Seems to be the current frequency, this might need to be populated by software?
> + * 0x184 /  The vendor driver uses these to set the initial value of LPF low
> + *
> + * Frequency seems to be calculated like this:
> + * (parent clock (432mhz) / register_magic_value) * 16 * 524288
> + * Only the lower 24 bits of the resulting value will be used. In addition, the
> + * PLL doesn't seem to be able to lock on frequencies lower than 220 MHz, as
> + * divisor 0xfb586f (220 MHz) works but 0xfb7fff locks up.
> + *
> + * Vendor values:
> + * frequency - register value
> + *
> + * 400000000  - 0x0067AE14
> + * 600000000  - 0x00451EB8,
> + * 800000000  - 0x0033D70A,
> + * 1000000000 - 0x002978d4,
> + */
> +
> +#define REG_LPF_LOW_L          0x140
> +#define REG_LPF_LOW_H          0x144
> +#define REG_LPF_HIGH_BOTTOM    0x148
> +#define REG_LPF_HIGH_TOP       0x14c
> +#define REG_LPF_TOGGLE         0x150
> +#define REG_LPF_MYSTERYTWO     0x154
> +#define REG_LPF_UPDATE_COUNT   0x15c
> +#define REG_LPF_MYSTERYONE     0x160
> +#define REG_LPF_TRANSITIONCTRL 0x164
> +#define REG_LPF_LOCK           0x174
> +#define REG_CURRENT            0x180
> +
> +#define MULTIPLIER_1           16
> +#define MULTIPLIER_2           524288
> +#define MULTIPLIER             (MULTIPLIER_1 * MULTIPLIER_2)
> +
> +struct msc313_cpupll {
> +       void __iomem *base;
> +       struct clk_hw clk_hw;
> +};
> +
> +#define to_cpupll(_hw) container_of(_hw, struct msc313_cpupll, clk_hw)
> +
> +static u32 msc313_cpupll_reg_read32(struct msc313_cpupll *cpupll, unsigned int reg)
> +{
> +       u32 value;
> +
> +       value = ioread16(cpupll->base + reg + 4) << 16;
> +       value |= ioread16(cpupll->base + reg);
> +
> +       return value;
> +}
> +
> +static void msc313_cpupll_reg_write32(struct msc313_cpupll *cpupll, unsigned int reg, u32 value)
> +{
> +       u16 l = value & 0xffff, h = (value >> 16) & 0xffff;
> +
> +       iowrite16(l, cpupll->base + reg);

We don't usually see 16-bit accesses but if that's what the hardware
wants then OK.

> +       iowrite16(h, cpupll->base + reg + 4);
> +}
> +
> +static void msc313_cpupll_setfreq(struct msc313_cpupll *cpupll, u32 regvalue)
> +{
> +       msc313_cpupll_reg_write32(cpupll, REG_LPF_HIGH_BOTTOM, regvalue);
> +
> +       iowrite16(0x1, cpupll->base + REG_LPF_MYSTERYONE);
> +       iowrite16(0x6, cpupll->base + REG_LPF_MYSTERYTWO);
> +       iowrite16(0x8, cpupll->base + REG_LPF_UPDATE_COUNT);
> +       iowrite16(BIT(12), cpupll->base + REG_LPF_TRANSITIONCTRL);
> +
> +       iowrite16(0, cpupll->base + REG_LPF_TOGGLE);
> +       iowrite16(1, cpupll->base + REG_LPF_TOGGLE);
> +
> +       while (!(ioread16(cpupll->base + REG_LPF_LOCK)))
> +               cpu_relax();

Any timeout? Can this use the io read timeout APIs?

> +
> +       iowrite16(0, cpupll->base + REG_LPF_TOGGLE);
> +
> +       msc313_cpupll_reg_write32(cpupll, REG_LPF_LOW_L, regvalue);
> +}
> +
> +static unsigned long msc313_cpupll_frequencyforreg(u32 reg, unsigned long parent_rate)
> +{
> +       unsigned long long prescaled = ((unsigned long long)parent_rate) * MULTIPLIER;
> +       unsigned long long scaled;
> +
> +       if (prescaled == 0 || reg == 0)
> +               return 0;
> +       scaled = DIV_ROUND_DOWN_ULL(prescaled, reg);
> +
> +       return scaled;

Just

	return DIV_ROUND_DOWN_ULL(...);

> +}
> +
> +static u32 msc313_cpupll_regforfrequecy(unsigned long rate, unsigned long parent_rate)
> +{
> +       unsigned long long prescaled = ((unsigned long long)parent_rate) * MULTIPLIER;
> +       unsigned long long scaled;
> +       u32 reg;
> +
> +       if (prescaled == 0 || rate == 0)
> +               return 0;
> +
> +       scaled = DIV_ROUND_UP_ULL(prescaled, rate);
> +       reg = scaled;
> +
> +       return reg;

Just

	return DIV_ROUND_UP_ULL(...);

> +}
> +
> +static unsigned long msc313_cpupll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> +{
> +       struct msc313_cpupll *cpupll = to_cpupll(hw);
> +
> +       return msc313_cpupll_frequencyforreg(msc313_cpupll_reg_read32(cpupll, REG_LPF_LOW_L),
> +                                            parent_rate);
> +}
> +
> +static long msc313_cpupll_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                    unsigned long *parent_rate)
> +{
> +       u32 reg = msc313_cpupll_regforfrequecy(rate, *parent_rate);
> +       long rounded = msc313_cpupll_frequencyforreg(reg, *parent_rate);
> +
> +       /*
> +        * This is my poor attempt at making sure the resulting
> +        * rate doesn't overshoot the requested rate.

If you want better bounds you can use determine_rate and then look at
the min/max constraints to make sure you don't overshoot. But otherwise
round_rate implementation is up to the provider to figure out what
should happen, i.e. overshooting could be OK if the provider intends for
it.

> +        */
> +       for (; rounded >= rate && reg > 0; reg--)
> +               rounded = msc313_cpupll_frequencyforreg(reg, *parent_rate);
> +
> +       return rounded;
> +}
> +
> +static int msc313_cpupll_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
> +{
> +       struct msc313_cpupll *cpupll = to_cpupll(hw);
> +       u32 reg = msc313_cpupll_regforfrequecy(rate, parent_rate);
> +
> +       msc313_cpupll_setfreq(cpupll, reg);
> +
> +       return 0;
> +}
> +
> +static const struct clk_ops msc313_cpupll_ops = {
> +       .recalc_rate    = msc313_cpupll_recalc_rate,
> +       .round_rate     = msc313_cpupll_round_rate,
> +       .set_rate       = msc313_cpupll_set_rate,
> +};
> +
> +static const struct of_device_id msc313_cpupll_of_match[] = {
> +       {
> +               .compatible = "mstar,msc313-cpupll",
> +       },
> +       {}

This can and should be less lines

	{ .compatible = "mstar,msc313-cpupll", },
	{}

> +};
> +
> +static const struct clk_parent_data cpupll_parent = {
> +       .index  = 0,
> +};
> +
> +static int msc313_cpupll_probe(struct platform_device *pdev)
> +{
> +       struct clk_init_data clk_init = {};
> +       struct device *dev = &pdev->dev;
> +       struct msc313_cpupll *cpupll;
> +       int ret;
> +
> +       cpupll = devm_kzalloc(&pdev->dev, sizeof(*cpupll), GFP_KERNEL);
> +       if (!cpupll)
> +               return -ENOMEM;
> +
> +       cpupll->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(cpupll->base))
> +               return PTR_ERR(cpupll->base);
> +
> +       /* LPF might not contain the current frequency so fix that up */
> +       msc313_cpupll_reg_write32(cpupll, REG_LPF_LOW_L,
> +                                 msc313_cpupll_reg_read32(cpupll, REG_CURRENT));
> +
> +       clk_init.name = dev_name(dev);
> +       clk_init.ops = &msc313_cpupll_ops;
> +       clk_init.flags = CLK_IS_CRITICAL;

Why is it critical? Can we have a comment? The clk ops don't have enable
or disable so it seems like the flag won't do anything.

> +       clk_init.parent_data = &cpupll_parent;
> +       clk_init.num_parents = 1;
> +       cpupll->clk_hw.init = &clk_init;
> +
> +       ret = devm_clk_hw_register(dev, &cpupll->clk_hw);
> +       if (ret)
> +               return ret;
> +
> +       return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_simple_get, &cpupll->clk_hw);
> +}
> +
> +static struct platform_driver msc313_cpupll_driver = {
> +       .driver = {
> +               .name = "mstar-msc313-cpupll",
> +               .of_match_table = msc313_cpupll_of_match,
> +       },
> +       .probe = msc313_cpupll_probe,
> +};
> +builtin_platform_driver(msc313_cpupll_driver);

Hi Stephen,

Thank you for looking at this for us.

On Sat, 8 Jan 2022 at 10:25, Stephen Boyd <sboyd@kernel.org> wrote:

> > +static void msc313_cpupll_reg_write32(struct msc313_cpupll *cpupll, unsigned int reg, u32 value)
> > +{
> > +       u16 l = value & 0xffff, h = (value >> 16) & 0xffff;
> > +
> > +       iowrite16(l, cpupll->base + reg);
>
> We don't usually see 16-bit accesses but if that's what the hardware
> wants then OK.

This hardware is weird and most of the registers are like this where
they are 32bit spaced but only 16 bits are used in each.
32bit registers are split across 2 16 bit registers spaced 32bits
apart. Writing the two parts has to be in the right order to get the
right result.

> > +       iowrite16(h, cpupll->base + reg + 4);
> > +}
> > +
> > +static void msc313_cpupll_setfreq(struct msc313_cpupll *cpupll, u32 regvalue)
> > +{
> > +       msc313_cpupll_reg_write32(cpupll, REG_LPF_HIGH_BOTTOM, regvalue);
> > +
> > +       iowrite16(0x1, cpupll->base + REG_LPF_MYSTERYONE);
> > +       iowrite16(0x6, cpupll->base + REG_LPF_MYSTERYTWO);
> > +       iowrite16(0x8, cpupll->base + REG_LPF_UPDATE_COUNT);
> > +       iowrite16(BIT(12), cpupll->base + REG_LPF_TRANSITIONCTRL);
> > +
> > +       iowrite16(0, cpupll->base + REG_LPF_TOGGLE);
> > +       iowrite16(1, cpupll->base + REG_LPF_TOGGLE);
> > +
> > +       while (!(ioread16(cpupll->base + REG_LPF_LOCK)))
> > +               cpu_relax();
>
> Any timeout? Can this use the io read timeout APIs?

Good point. I never saw a situation where the lock didn't happen but I
think Willy did when he was poking at it.
I guess if it doesn't lock we should timeout, warn that something
isn't working and return an error.

> > +static long msc313_cpupll_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                                    unsigned long *parent_rate)
> > +{
> > +       u32 reg = msc313_cpupll_regforfrequecy(rate, *parent_rate);
> > +       long rounded = msc313_cpupll_frequencyforreg(reg, *parent_rate);
> > +
> > +       /*
> > +        * This is my poor attempt at making sure the resulting
> > +        * rate doesn't overshoot the requested rate.
>
> If you want better bounds you can use determine_rate and then look at
> the min/max constraints to make sure you don't overshoot. But otherwise
> round_rate implementation is up to the provider to figure out what
> should happen, i.e. overshooting could be OK if the provider intends for
> it.

This clock is basically only used by cpufreq-dt. I'm not sure what it
would do with determine_rate. I'll take a look.
The main thing I wanted to do here was make sure the resulting clock
wasn't higher than what we have in the opp table and end up with the
CPU locking up.

> > +       clk_init.name = dev_name(dev);
> > +       clk_init.ops = &msc313_cpupll_ops;
> > +       clk_init.flags = CLK_IS_CRITICAL;
>
> Why is it critical? Can we have a comment? The clk ops don't have enable
> or disable so it seems like the flag won't do anything.

This clock is critical in the sense that once the DDR memory is setup
by the bootloader you must not turn it off even if you switch the CPU
to the other clock source. If you disable it the system locks up.
I think it can be dropped as does nothing without enable or disable
like you wrote.

Cheers,

Daniel