Message ID | 20220603190509.45986-2-romain.perier@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ARM: mstar: cpupll | expand |
Quoting Romain Perier (2022-06-03 12:05:09) > From: Daniel Palmer <daniel@0x0f.com> > > Add a driver for the CPU pll/ARM pll/MIPS pll that is present > in MStar SoCs. > > Currently there is no documentation for this block so it's possible > this driver isn't entirely correct. > > Only tested on the version of this IP in the MStar/SigmaStar > ARMv7 SoCs. > > Co-authored-by: Willy Tarreau <w@1wt.eu> This is not a standard tag, maybe Co-developed-by is what you want? A Signed-off-by tag should be here from Willy Tarreau then. > Signed-off-by: Daniel Palmer <daniel@0x0f.com> Your Signed-off-by needs to be here. I can't apply this otherwise. > diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig > index de37e1bce2d2..146eeb637318 100644 > --- a/drivers/clk/mstar/Kconfig > +++ b/drivers/clk/mstar/Kconfig > @@ -1,4 +1,11 @@ > # SPDX-License-Identifier: GPL-2.0-only > +config MSTAR_MSC313_CPUPLL > + bool "MStar CPUPLL driver" > + depends on ARCH_MSTARV7 || COMPILE_TEST > + default ARCH_MSTARV7 > + help > + Support for the CPU PLL present on MStar/Sigmastar SoCs. > + > config MSTAR_MSC313_MPLL > bool "MStar MPLL driver" > depends on ARCH_MSTARV7 || COMPILE_TEST > @@ -7,3 +14,4 @@ config MSTAR_MSC313_MPLL > help > Support for the MPLL PLL and dividers block present on > MStar/Sigmastar SoCs. > + > diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile > index f8dcd25ede1d..21296a04e65a 100644 > --- a/drivers/clk/mstar/Makefile > +++ b/drivers/clk/mstar/Makefile > @@ -2,5 +2,5 @@ > # > # Makefile for mstar specific clk > # > - > +obj-$(CONFIG_MSTAR_MSC313_CPUPLL) += clk-msc313-cpupll.o > obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.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..c57b509e8c20 > --- /dev/null > +++ b/drivers/clk/mstar/clk-msc313-cpupll.c > @@ -0,0 +1,221 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2019 Daniel Palmer <daniel@thingy.jp> > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> Ought to include kernel.h and device.h too. > + [...] > + > +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" }, > + {} > +}; > + > +static const struct clk_parent_data cpupll_parent = { Why not put this on the stack? It doesn't have to live forever. > + .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.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); Use devm to add the provider too.
Hi Stephen, On Fri, Sep 30, 2022 at 12:17:40PM -0700, Stephen Boyd wrote: > Quoting Romain Perier (2022-06-03 12:05:09) > > From: Daniel Palmer <daniel@0x0f.com> > > > > Add a driver for the CPU pll/ARM pll/MIPS pll that is present > > in MStar SoCs. > > > > Currently there is no documentation for this block so it's possible > > this driver isn't entirely correct. > > > > Only tested on the version of this IP in the MStar/SigmaStar > > ARMv7 SoCs. > > > > Co-authored-by: Willy Tarreau <w@1wt.eu> > > This is not a standard tag, maybe Co-developed-by is what you want? Yeah it's the same. We're seeing 122 co-authored-by tags in the kernel's history vs 3122 co-developed-by, so that's only 3% but at least not a fantasist one.. > A Signed-off-by tag should be here from Willy Tarreau then. Not all commits follow this, but indeed some do. Regardless I really don't care at all about my name being listed there. > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > > Your Signed-off-by needs to be here. I can't apply this otherwise. Quite frankly, we're talking about an old patch that was first submitted 9 months ago (!), let's not postpone it even further just for headers reason, please just drop this line mentioning my little contribution, I don't care a single second at all, really. Thanks, Willy
Hi, Quiet frankly I don't want to debate about this , as is it seems to be basically "the rule" (see https://www.kernel.org/doc/html/v6.0/process/submitting-patches.html). It is okay for you Willy, if I transform your tag to "Co-developed-by:" plus a "Signed-off-by" ? So Stephen can apply it , and everyone is happy :) Cheers, Romain Le ven. 30 sept. 2022 à 21:35, Willy Tarreau <w@1wt.eu> a écrit : > > Hi Stephen, > > On Fri, Sep 30, 2022 at 12:17:40PM -0700, Stephen Boyd wrote: > > Quoting Romain Perier (2022-06-03 12:05:09) > > > From: Daniel Palmer <daniel@0x0f.com> > > > > > > Add a driver for the CPU pll/ARM pll/MIPS pll that is present > > > in MStar SoCs. > > > > > > Currently there is no documentation for this block so it's possible > > > this driver isn't entirely correct. > > > > > > Only tested on the version of this IP in the MStar/SigmaStar > > > ARMv7 SoCs. > > > > > > Co-authored-by: Willy Tarreau <w@1wt.eu> > > > > This is not a standard tag, maybe Co-developed-by is what you want? > > Yeah it's the same. We're seeing 122 co-authored-by tags in the > kernel's history vs 3122 co-developed-by, so that's only 3% but > at least not a fantasist one.. > > > A Signed-off-by tag should be here from Willy Tarreau then. > > Not all commits follow this, but indeed some do. Regardless I > really don't care at all about my name being listed there. > > > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > > > > Your Signed-off-by needs to be here. I can't apply this otherwise. > > Quite frankly, we're talking about an old patch that was first > submitted 9 months ago (!), let's not postpone it even further just > for headers reason, please just drop this line mentioning my > little contribution, I don't care a single second at all, really. > > Thanks, > Willy
On Wed, Oct 05, 2022 at 09:33:40AM +0200, Romain Perier wrote: > Hi, > > Quiet frankly I don't want to debate about this , as is it seems to be > basically "the rule" (see > https://www.kernel.org/doc/html/v6.0/process/submitting-patches.html). > It is okay for you Willy, if I transform your tag to > "Co-developed-by:" plus a "Signed-off-by" ? So Stephen can apply it , > and everyone is happy :) Yes absolutely! I just wanted to avoid yet another round trip. Many thanks Romain! Willy
Hi, Le ven. 30 sept. 2022 à 21:17, Stephen Boyd <sboyd@kernel.org> a écrit : > > Quoting Romain Perier (2022-06-03 12:05:09) > > From: Daniel Palmer <daniel@0x0f.com> > > > > Add a driver for the CPU pll/ARM pll/MIPS pll that is present > > in MStar SoCs. > > > > Currently there is no documentation for this block so it's possible > > this driver isn't entirely correct. > > > > Only tested on the version of this IP in the MStar/SigmaStar > > ARMv7 SoCs. > > > > Co-authored-by: Willy Tarreau <w@1wt.eu> > > This is not a standard tag, maybe Co-developed-by is what you want? A > Signed-off-by tag should be here from Willy Tarreau then. > > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > > Your Signed-off-by needs to be here. I can't apply this otherwise. > > > diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig > > index de37e1bce2d2..146eeb637318 100644 > > --- a/drivers/clk/mstar/Kconfig > > +++ b/drivers/clk/mstar/Kconfig > > @@ -1,4 +1,11 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > +config MSTAR_MSC313_CPUPLL > > + bool "MStar CPUPLL driver" > > + depends on ARCH_MSTARV7 || COMPILE_TEST > > + default ARCH_MSTARV7 > > + help > > + Support for the CPU PLL present on MStar/Sigmastar SoCs. > > + > > config MSTAR_MSC313_MPLL > > bool "MStar MPLL driver" > > depends on ARCH_MSTARV7 || COMPILE_TEST > > @@ -7,3 +14,4 @@ config MSTAR_MSC313_MPLL > > help > > Support for the MPLL PLL and dividers block present on > > MStar/Sigmastar SoCs. > > + > > diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile > > index f8dcd25ede1d..21296a04e65a 100644 > > --- a/drivers/clk/mstar/Makefile > > +++ b/drivers/clk/mstar/Makefile > > @@ -2,5 +2,5 @@ > > # > > # Makefile for mstar specific clk > > # > > - > > +obj-$(CONFIG_MSTAR_MSC313_CPUPLL) += clk-msc313-cpupll.o > > obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.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..c57b509e8c20 > > --- /dev/null > > +++ b/drivers/clk/mstar/clk-msc313-cpupll.c > > @@ -0,0 +1,221 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2019 Daniel Palmer <daniel@thingy.jp> > > + */ > > + > > +#include <linux/clk-provider.h> > > +#include <linux/of_address.h> > > +#include <linux/platform_device.h> > > Ought to include kernel.h and device.h too. > > > + > [...] > > + > > +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" }, > > + {} > > +}; > > + > > +static const struct clk_parent_data cpupll_parent = { > > Why not put this on the stack? It doesn't have to live forever. > > > + .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.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); > > Use devm to add the provider too. Ok, it does not seem to be huge to change, I will send another iteration. Thanks, Cheers, Romain
diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig index de37e1bce2d2..146eeb637318 100644 --- a/drivers/clk/mstar/Kconfig +++ b/drivers/clk/mstar/Kconfig @@ -1,4 +1,11 @@ # SPDX-License-Identifier: GPL-2.0-only +config MSTAR_MSC313_CPUPLL + bool "MStar CPUPLL driver" + depends on ARCH_MSTARV7 || COMPILE_TEST + default ARCH_MSTARV7 + help + Support for the CPU PLL present on MStar/Sigmastar SoCs. + config MSTAR_MSC313_MPLL bool "MStar MPLL driver" depends on ARCH_MSTARV7 || COMPILE_TEST @@ -7,3 +14,4 @@ config MSTAR_MSC313_MPLL help Support for the MPLL PLL and dividers block present on MStar/Sigmastar SoCs. + diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile index f8dcd25ede1d..21296a04e65a 100644 --- a/drivers/clk/mstar/Makefile +++ b/drivers/clk/mstar/Makefile @@ -2,5 +2,5 @@ # # Makefile for mstar specific clk # - +obj-$(CONFIG_MSTAR_MSC313_CPUPLL) += clk-msc313-cpupll.o obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.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..c57b509e8c20 --- /dev/null +++ b/drivers/clk/mstar/clk-msc313-cpupll.c @@ -0,0 +1,221 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 Daniel Palmer <daniel@thingy.jp> + */ + +#include <linux/clk-provider.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> + +/* + * 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 LPF_LOCK_TIMEOUT 100000000 + +#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); + iowrite16(h, cpupll->base + reg + 4); +} + +static void msc313_cpupll_setfreq(struct msc313_cpupll *cpupll, u32 regvalue) +{ + ktime_t timeout; + + 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); + + timeout = ktime_add_ns(ktime_get(), LPF_LOCK_TIMEOUT); + while (!(ioread16(cpupll->base + REG_LPF_LOCK))) { + if (ktime_after(ktime_get(), timeout)) { + pr_err("timeout waiting for LPF_LOCK\n"); + return; + } + cpu_relax(); + } + + 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; + + if (prescaled == 0 || reg == 0) + return 0; + return DIV_ROUND_DOWN_ULL(prescaled, reg); +} + +static u32 msc313_cpupll_regforfrequecy(unsigned long rate, unsigned long parent_rate) +{ + unsigned long long prescaled = ((unsigned long long)parent_rate) * MULTIPLIER; + + if (prescaled == 0 || rate == 0) + return 0; + return DIV_ROUND_UP_ULL(prescaled, rate); +} + +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. + */ + 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" }, + {} +}; + +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.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);