On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <[email protected]>
>
> This adds an SoC driver for the ep93xx. Currently there
> is only one thing not fitting into any other framework,
> and that is the swlock setting.
>
> It's used for clock settings and restart.
>
> Signed-off-by: Nikita Shubin <[email protected]>
> Tested-by: Alexander Sverdlin <[email protected]>
> Acked-by: Alexander Sverdlin <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> ---
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/cirrus/Kconfig | 12 ++
> drivers/soc/cirrus/Makefile | 2 +
> drivers/soc/cirrus/soc-ep93xx.c | 231 ++++++++++++++++++++++++++++++++++++++
> include/linux/soc/cirrus/ep93xx.h | 18 ++-
> 6 files changed, 264 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 4e176280113a..16327b63b773 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -8,6 +8,7 @@ source "drivers/soc/aspeed/Kconfig"
> source "drivers/soc/atmel/Kconfig"
> source "drivers/soc/bcm/Kconfig"
> source "drivers/soc/canaan/Kconfig"
> +source "drivers/soc/cirrus/Kconfig"
> source "drivers/soc/fsl/Kconfig"
> source "drivers/soc/fujitsu/Kconfig"
> source "drivers/soc/imx/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 3b0f9fb3b5c8..b76a03fe808e 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -9,6 +9,7 @@ obj-y += aspeed/
> obj-$(CONFIG_ARCH_AT91) += atmel/
> obj-y += bcm/
> obj-$(CONFIG_SOC_CANAAN) += canaan/
> +obj-$(CONFIG_EP93XX_SOC) += cirrus/
> obj-$(CONFIG_ARCH_DOVE) += dove/
> obj-$(CONFIG_MACH_DOVE) += dove/
> obj-y += fsl/
> diff --git a/drivers/soc/cirrus/Kconfig b/drivers/soc/cirrus/Kconfig
> new file mode 100644
> index 000000000000..408f3343a265
> --- /dev/null
> +++ b/drivers/soc/cirrus/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +if ARCH_EP93XX
> +
> +config EP93XX_SOC
> + bool "Cirrus EP93xx chips SoC"
> + select SOC_BUS
> + default y if !EP93XX_SOC_COMMON
> + help
> + Support SoC for Cirrus EP93xx chips.
> +
> +endif
> diff --git a/drivers/soc/cirrus/Makefile b/drivers/soc/cirrus/Makefile
> new file mode 100644
> index 000000000000..ed6752844c6f
> --- /dev/null
> +++ b/drivers/soc/cirrus/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-y += soc-ep93xx.o
> diff --git a/drivers/soc/cirrus/soc-ep93xx.c b/drivers/soc/cirrus/soc-ep93xx.c
> new file mode 100644
> index 000000000000..2fd48d900f24
> --- /dev/null
> +++ b/drivers/soc/cirrus/soc-ep93xx.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SoC driver for Cirrus EP93xx chips.
> + * Copyright (C) 2022 Nikita Shubin <[email protected]>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> + * Copyright (C) 2006 Lennert Buytenhek <[email protected]>
> + * Copyright (C) 2007 Herbert Valerio Riedel <[email protected]>
> + *
> + * Thanks go to Michael Burian and Ray Lehtiniemi for their key
> + * role in the ep93xx Linux community
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/soc/cirrus/ep93xx.h>
> +
> +#define EP93XX_EXT_CLK_RATE 14745600
> +
> +#define EP93XX_SYSCON_DEVCFG 0x80
> +
> +#define EP93XX_SWLOCK_MAGICK 0xaa
> +#define EP93XX_SYSCON_SWLOCK 0xc0
> +#define EP93XX_SYSCON_SYSCFG 0x9c
> +#define EP93XX_SYSCON_SYSCFG_REV_MASK 0xf0000000
> +#define EP93XX_SYSCON_SYSCFG_REV_SHIFT 28
> +
> +#define EP93XX_SYSCON_CLKSET1 0x20
> +#define EP93XX_SYSCON_CLKSET1_NBYP1 BIT(23)
> +#define EP93XX_SYSCON_CLKSET2 0x24
> +#define EP93XX_SYSCON_CLKSET2_NBYP2 BIT(19)
> +#define EP93XX_SYSCON_CLKSET2_PLL2_EN BIT(18)
> +
> +static DEFINE_SPINLOCK(ep93xx_swlock);
> +
> +/* EP93xx System Controller software locked register write */
> +void ep93xx_syscon_swlocked_write(struct regmap *map, unsigned int reg, unsigned int val)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> + regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> + regmap_write(map, reg, val);
> +
> + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_syscon_swlocked_write, EP93XX_SOC);
I doubt that your code compiles. Didn't you add a user of this in some
earlier patch?
Anyway, no, drop it, don't export some weird calls from core initcall to
drivers. You violate layering and driver encapsulation. There is no
dependency/probe ordering.
There is no even need for this, because this code does not use it!
> +
> +void ep93xx_devcfg_set_clear(struct regmap *map, unsigned int set_bits, unsigned int clear_bits)
> +{
> + unsigned long flags;
> + unsigned int val;
> +
> + spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> + regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> + val &= ~clear_bits;
> + val |= set_bits;
> + regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> + regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> +
> + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_devcfg_set_clear, EP93XX_SOC);
No.
> +
> +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + unsigned long flags;
> + unsigned int tmp, orig;
> +
> + spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> + regmap_read(map, EP93XX_SYSCON_DEVCFG, &orig);
> + tmp = orig & ~mask;
> + tmp |= val & mask;
> + if (tmp != orig) {
> + regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> + regmap_write(map, reg, tmp);
> + }
> +
> + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_swlocked_update_bits, EP93XX_SOC);
No.
> +
> +unsigned int __init ep93xx_chip_revision(struct regmap *map)
Why this is visible outside? This should be static.
> +{
> + unsigned int val;
> +
> + regmap_read(map, EP93XX_SYSCON_SYSCFG, &val);
> + val &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> + val >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> + return val;
> +}
> +
> +static const char __init *ep93xx_get_soc_rev(struct regmap *map)
> +{
> + int rev = ep93xx_chip_revision(map);
> +
> + switch (rev) {
> + case EP93XX_CHIP_REV_D0:
> + return "D0";
> + case EP93XX_CHIP_REV_D1:
> + return "D1";
> + case EP93XX_CHIP_REV_E0:
> + return "E0";
> + case EP93XX_CHIP_REV_E1:
> + return "E1";
> + case EP93XX_CHIP_REV_E2:
> + return "E2";
> + default:
> + return "unknown";
> + }
> +}
> +
> +/*
> + * PLL rate = 14.7456 MHz * (X1FBD + 1) * (X2FBD + 1) / (X2IPD + 1) / 2^PS
> + */
> +static unsigned long __init calc_pll_rate(u64 rate, u32 config_word)
> +{
> + rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1; /* X1FBD */
> + rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1; /* X2FBD */
> + do_div(rate, (config_word & GENMASK(4, 0)) + 1); /* X2IPD */
> + rate >>= ((config_word >> 16) & 3); /* PS */
> +
> + return rate;
> +}
> +
> +static int __init ep93xx_soc_init(void)
> +{
> + struct soc_device_attribute *attrs;
> + struct soc_device *soc_dev;
> + struct device_node *np;
> + struct regmap *map;
> + struct clk_hw *hw;
> + unsigned long clk_pll1_rate, clk_pll2_rate;
> + unsigned int clk_f_div, clk_h_div, clk_p_div, clk_usb_div;
> + const char fclk_divisors[] = { 1, 2, 4, 8, 16, 1, 1, 1 };
> + const char hclk_divisors[] = { 1, 2, 4, 5, 6, 8, 16, 32 };
> + const char pclk_divisors[] = { 1, 2, 4, 8 };
> + const char *machine = NULL;
> + u32 value;
> +
> + /* Multiplatform guard, only proceed on ep93xx */
> + if (!of_machine_is_compatible("cirrus,ep9301"))
> + return 0;
This should already be a warning sign for you...
> +
> + map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-syscon");
> + if (IS_ERR(map))
> + return PTR_ERR(map);
No, not-reusable. Use devices and device nodes.
> +
> + /* Determine the bootloader configured pll1 rate */
> + regmap_read(map, EP93XX_SYSCON_CLKSET1, &value);
> + if (!(value & EP93XX_SYSCON_CLKSET1_NBYP1))
> + clk_pll1_rate = EP93XX_EXT_CLK_RATE;
> + else
> + clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
> +
> + hw = clk_hw_register_fixed_rate(NULL, "pll1", "xtali", 0, clk_pll1_rate);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + /* Initialize the pll1 derived clocks */
> + clk_f_div = fclk_divisors[(value >> 25) & 0x7];
> + clk_h_div = hclk_divisors[(value >> 20) & 0x7];
> + clk_p_div = pclk_divisors[(value >> 18) & 0x3];
> +
> + hw = clk_hw_register_fixed_factor(NULL, "fclk", "pll1", 0, 1, clk_f_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + hw = clk_hw_register_fixed_factor(NULL, "hclk", "pll1", 0, 1, clk_h_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + hw = clk_hw_register_fixed_factor(NULL, "pclk", "hclk", 0, 1, clk_p_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + /* Determine the bootloader configured pll2 rate */
> + regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> + if (!(value & EP93XX_SYSCON_CLKSET2_NBYP2))
> + clk_pll2_rate = EP93XX_EXT_CLK_RATE;
> + else if (value & EP93XX_SYSCON_CLKSET2_PLL2_EN)
> + clk_pll2_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
> + else
> + clk_pll2_rate = 0;
> +
> + hw = clk_hw_register_fixed_rate(NULL, "pll2", "xtali", 0, clk_pll2_rate);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> + clk_usb_div = (((value >> 28) & GENMASK(3, 0)) + 1);
> + hw = clk_hw_register_fixed_factor(NULL, "usb_clk", "pll2", 0, 1, clk_usb_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> + if (!attrs)
> + return -ENOMEM;
> +
> + np = of_find_node_by_path("/");
> + of_property_read_string(np, "model", &machine);
> + if (machine)
> + attrs->machine = kstrdup(machine, GFP_KERNEL);
> + of_node_put(np);
> +
> + attrs->family = "Cirrus Logic EP93xx";
> + attrs->revision = ep93xx_get_soc_rev(map);
> +
> + soc_dev = soc_device_register(attrs);
> + if (IS_ERR(soc_dev)) {
> + kfree(attrs->soc_id);
> + kfree(attrs->serial_number);
> + kfree(attrs);
> + return PTR_ERR(soc_dev);
> + }
> +
> + pr_info("EP93xx SoC revision %s\n", attrs->revision);
> +
> + return 0;
> +}
> +core_initcall(ep93xx_soc_init);
That's not the way to add soc driver. You need a proper driver for it
Best regards,
Krzysztof
On Fri, 2023-07-21 at 16:22 +0200, Krzysztof Kozlowski wrote:
> On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> > From: Nikita Shubin <[email protected]>
> >
> > This adds an SoC driver for the ep93xx. Currently there
> > is only one thing not fitting into any other framework,
> > and that is the swlock setting.
> >
> > It's used for clock settings and restart.
> >
> > Signed-off-by: Nikita Shubin <[email protected]>
> > Tested-by: Alexander Sverdlin <[email protected]>
> > Acked-by: Alexander Sverdlin <[email protected]>
> > Reviewed-by: Linus Walleij <[email protected]>
> > ---
> > drivers/soc/Kconfig | 1 +
> > drivers/soc/Makefile | 1 +
> > drivers/soc/cirrus/Kconfig | 12 ++
> > drivers/soc/cirrus/Makefile | 2 +
> > drivers/soc/cirrus/soc-ep93xx.c | 231
> > ++++++++++++++++++++++++++++++++++++++
> > include/linux/soc/cirrus/ep93xx.h | 18 ++-
> > 6 files changed, 264 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 4e176280113a..16327b63b773 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -8,6 +8,7 @@ source "drivers/soc/aspeed/Kconfig"
> > source "drivers/soc/atmel/Kconfig"
> > source "drivers/soc/bcm/Kconfig"
> > source "drivers/soc/canaan/Kconfig"
> > +source "drivers/soc/cirrus/Kconfig"
> > source "drivers/soc/fsl/Kconfig"
> > source "drivers/soc/fujitsu/Kconfig"
> > source "drivers/soc/imx/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index 3b0f9fb3b5c8..b76a03fe808e 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -9,6 +9,7 @@ obj-y += aspeed/
> > obj-$(CONFIG_ARCH_AT91) += atmel/
> > obj-y += bcm/
> > obj-$(CONFIG_SOC_CANAAN) += canaan/
> > +obj-$(CONFIG_EP93XX_SOC) += cirrus/
> > obj-$(CONFIG_ARCH_DOVE) += dove/
> > obj-$(CONFIG_MACH_DOVE) += dove/
> > obj-y += fsl/
> > diff --git a/drivers/soc/cirrus/Kconfig
> > b/drivers/soc/cirrus/Kconfig
> > new file mode 100644
> > index 000000000000..408f3343a265
> > --- /dev/null
> > +++ b/drivers/soc/cirrus/Kconfig
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +if ARCH_EP93XX
> > +
> > +config EP93XX_SOC
> > + bool "Cirrus EP93xx chips SoC"
> > + select SOC_BUS
> > + default y if !EP93XX_SOC_COMMON
> > + help
> > + Support SoC for Cirrus EP93xx chips.
> > +
> > +endif
> > diff --git a/drivers/soc/cirrus/Makefile
> > b/drivers/soc/cirrus/Makefile
> > new file mode 100644
> > index 000000000000..ed6752844c6f
> > --- /dev/null
> > +++ b/drivers/soc/cirrus/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-y += soc-ep93xx.o
> > diff --git a/drivers/soc/cirrus/soc-ep93xx.c
> > b/drivers/soc/cirrus/soc-ep93xx.c
> > new file mode 100644
> > index 000000000000..2fd48d900f24
> > --- /dev/null
> > +++ b/drivers/soc/cirrus/soc-ep93xx.c
> > @@ -0,0 +1,231 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SoC driver for Cirrus EP93xx chips.
> > + * Copyright (C) 2022 Nikita Shubin <[email protected]>
> > + *
> > + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> > + * Copyright (C) 2006 Lennert Buytenhek <[email protected]>
> > + * Copyright (C) 2007 Herbert Valerio Riedel <[email protected]>
> > + *
> > + * Thanks go to Michael Burian and Ray Lehtiniemi for their key
> > + * role in the ep93xx Linux community
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/sys_soc.h>
> > +#include <linux/soc/cirrus/ep93xx.h>
> > +
> > +#define EP93XX_EXT_CLK_RATE 14745600
> > +
> > +#define EP93XX_SYSCON_DEVCFG 0x80
> > +
> > +#define EP93XX_SWLOCK_MAGICK 0xaa
> > +#define EP93XX_SYSCON_SWLOCK 0xc0
> > +#define EP93XX_SYSCON_SYSCFG 0x9c
> > +#define EP93XX_SYSCON_SYSCFG_REV_MASK 0xf0000000
> > +#define EP93XX_SYSCON_SYSCFG_REV_SHIFT 28
> > +
> > +#define EP93XX_SYSCON_CLKSET1 0x20
> > +#define EP93XX_SYSCON_CLKSET1_NBYP1 BIT(23)
> > +#define EP93XX_SYSCON_CLKSET2 0x24
> > +#define EP93XX_SYSCON_CLKSET2_NBYP2 BIT(19)
> > +#define EP93XX_SYSCON_CLKSET2_PLL2_EN BIT(18)
> > +
> > +static DEFINE_SPINLOCK(ep93xx_swlock);
> > +
> > +/* EP93xx System Controller software locked register write */
> > +void ep93xx_syscon_swlocked_write(struct regmap *map, unsigned int
> > reg, unsigned int val)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&ep93xx_swlock, flags);
> > +
> > + regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > EP93XX_SWLOCK_MAGICK);
> > + regmap_write(map, reg, val);
> > +
> > + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ep93xx_syscon_swlocked_write, EP93XX_SOC);
>
> I doubt that your code compiles. Didn't you add a user of this in
> some
> earlier patch?
>
> Anyway, no, drop it, don't export some weird calls from core initcall
> to
> drivers. You violate layering and driver encapsulation. There is no
> dependency/probe ordering.
>
> There is no even need for this, because this code does not use it!
It's a little emotional, so i can hardly understand the exact reason of
dissatisfaction.
Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my fault
it's not needed as both PINCTRL and CLK (the only users of this code)
can't be built as modules.
>
> > +
> > +void ep93xx_devcfg_set_clear(struct regmap *map, unsigned int
> > set_bits, unsigned int clear_bits)
> > +{
> > + unsigned long flags;
> > + unsigned int val;
> > +
> > + spin_lock_irqsave(&ep93xx_swlock, flags);
> > +
> > + regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> > + val &= ~clear_bits;
> > + val |= set_bits;
> > + regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > EP93XX_SWLOCK_MAGICK);
> > + regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> > +
> > + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ep93xx_devcfg_set_clear, EP93XX_SOC);
>
> No.
>
> > +
> > +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int
> > reg,
> > + unsigned int mask, unsigned int
> > val)
> > +{
> > + unsigned long flags;
> > + unsigned int tmp, orig;
> > +
> > + spin_lock_irqsave(&ep93xx_swlock, flags);
> > +
> > + regmap_read(map, EP93XX_SYSCON_DEVCFG, &orig);
> > + tmp = orig & ~mask;
> > + tmp |= val & mask;
> > + if (tmp != orig) {
> > + regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > EP93XX_SWLOCK_MAGICK);
> > + regmap_write(map, reg, tmp);
> > + }
> > +
> > + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ep93xx_swlocked_update_bits, EP93XX_SOC);
>
> No.
>
>
> > +
> > +unsigned int __init ep93xx_chip_revision(struct regmap *map)
>
> Why this is visible outside? This should be static.
Indeed.
>
> > +{
> > + unsigned int val;
> > +
> > + regmap_read(map, EP93XX_SYSCON_SYSCFG, &val);
> > + val &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> > + val >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> > + return val;
> > +}
>
>
> > +
> > +static const char __init *ep93xx_get_soc_rev(struct regmap *map)
> > +{
> > + int rev = ep93xx_chip_revision(map);
> > +
> > + switch (rev) {
> > + case EP93XX_CHIP_REV_D0:
> > + return "D0";
> > + case EP93XX_CHIP_REV_D1:
> > + return "D1";
> > + case EP93XX_CHIP_REV_E0:
> > + return "E0";
> > + case EP93XX_CHIP_REV_E1:
> > + return "E1";
> > + case EP93XX_CHIP_REV_E2:
> > + return "E2";
> > + default:
> > + return "unknown";
> > + }
> > +}
> > +
> > +/*
> > + * PLL rate = 14.7456 MHz * (X1FBD + 1) * (X2FBD + 1) / (X2IPD +
> > 1) / 2^PS
> > + */
> > +static unsigned long __init calc_pll_rate(u64 rate, u32
> > config_word)
> > +{
> > + rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1; /*
> > X1FBD */
> > + rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1; /*
> > X2FBD */
> > + do_div(rate, (config_word & GENMASK(4, 0)) + 1); /*
> > X2IPD */
> > + rate >>= ((config_word >> 16) & 3); /*
> > PS */
> > +
> > + return rate;
> > +}
> > +
> > +static int __init ep93xx_soc_init(void)
> > +{
> > + struct soc_device_attribute *attrs;
> > + struct soc_device *soc_dev;
> > + struct device_node *np;
> > + struct regmap *map;
> > + struct clk_hw *hw;
> > + unsigned long clk_pll1_rate, clk_pll2_rate;
> > + unsigned int clk_f_div, clk_h_div, clk_p_div, clk_usb_div;
> > + const char fclk_divisors[] = { 1, 2, 4, 8, 16, 1, 1, 1 };
> > + const char hclk_divisors[] = { 1, 2, 4, 5, 6, 8, 16, 32 };
> > + const char pclk_divisors[] = { 1, 2, 4, 8 };
> > + const char *machine = NULL;
> > + u32 value;
> > +
> > + /* Multiplatform guard, only proceed on ep93xx */
> > + if (!of_machine_is_compatible("cirrus,ep9301"))
> > + return 0;
>
> This should already be a warning sign for you...
>
> > +
> > + map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-
> > syscon");
> > + if (IS_ERR(map))
> > + return PTR_ERR(map);
>
> No, not-reusable. Use devices and device nodes.
>
> > +
> > + /* Determine the bootloader configured pll1 rate */
> > + regmap_read(map, EP93XX_SYSCON_CLKSET1, &value);
> > + if (!(value & EP93XX_SYSCON_CLKSET1_NBYP1))
> > + clk_pll1_rate = EP93XX_EXT_CLK_RATE;
> > + else
> > + clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE,
> > value);
> > +
> > + hw = clk_hw_register_fixed_rate(NULL, "pll1", "xtali", 0,
> > clk_pll1_rate);
> > + if (IS_ERR(hw))
> > + return PTR_ERR(hw);
> > +
> > + /* Initialize the pll1 derived clocks */
> > + clk_f_div = fclk_divisors[(value >> 25) & 0x7];
> > + clk_h_div = hclk_divisors[(value >> 20) & 0x7];
> > + clk_p_div = pclk_divisors[(value >> 18) & 0x3];
> > +
> > + hw = clk_hw_register_fixed_factor(NULL, "fclk", "pll1", 0,
> > 1, clk_f_div);
> > + if (IS_ERR(hw))
> > + return PTR_ERR(hw);
> > +
> > + hw = clk_hw_register_fixed_factor(NULL, "hclk", "pll1", 0,
> > 1, clk_h_div);
> > + if (IS_ERR(hw))
> > + return PTR_ERR(hw);
> > +
> > + hw = clk_hw_register_fixed_factor(NULL, "pclk", "hclk", 0,
> > 1, clk_p_div);
> > + if (IS_ERR(hw))
> > + return PTR_ERR(hw);
> > +
> > + /* Determine the bootloader configured pll2 rate */
> > + regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> > + if (!(value & EP93XX_SYSCON_CLKSET2_NBYP2))
> > + clk_pll2_rate = EP93XX_EXT_CLK_RATE;
> > + else if (value & EP93XX_SYSCON_CLKSET2_PLL2_EN)
> > + clk_pll2_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE,
> > value);
> > + else
> > + clk_pll2_rate = 0;
> > +
> > + hw = clk_hw_register_fixed_rate(NULL, "pll2", "xtali", 0,
> > clk_pll2_rate);
> > + if (IS_ERR(hw))
> > + return PTR_ERR(hw);
> > +
> > + regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> > + clk_usb_div = (((value >> 28) & GENMASK(3, 0)) + 1);
> > + hw = clk_hw_register_fixed_factor(NULL, "usb_clk", "pll2",
> > 0, 1, clk_usb_div);
> > + if (IS_ERR(hw))
> > + return PTR_ERR(hw);
> > +
> > + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> > + if (!attrs)
> > + return -ENOMEM;
> > +
> > + np = of_find_node_by_path("/");
> > + of_property_read_string(np, "model", &machine);
> > + if (machine)
> > + attrs->machine = kstrdup(machine, GFP_KERNEL);
> > + of_node_put(np);
> > +
> > + attrs->family = "Cirrus Logic EP93xx";
> > + attrs->revision = ep93xx_get_soc_rev(map);
> > +
> > + soc_dev = soc_device_register(attrs);
> > + if (IS_ERR(soc_dev)) {
> > + kfree(attrs->soc_id);
> > + kfree(attrs->serial_number);
> > + kfree(attrs);
> > + return PTR_ERR(soc_dev);
> > + }
> > +
> > + pr_info("EP93xx SoC revision %s\n", attrs->revision);
> > +
> > + return 0;
> > +}
> > +core_initcall(ep93xx_soc_init);
>
> That's not the way to add soc driver. You need a proper driver for it
What is a proper driver for these ?
Even the latest additions in drivers/soc/* go with *_initcall.
The only i can see is:
./versatile/soc-
realview.c:132:builtin_platform_driver(realview_soc_driver);
By Linus =).
>
> Best regards,
> Krzysztof
>
On 24/07/2023 17:02, Nikita Shubin wrote:
>>> +static DEFINE_SPINLOCK(ep93xx_swlock);
>>> +
>>> +/* EP93xx System Controller software locked register write */
>>> +void ep93xx_syscon_swlocked_write(struct regmap *map, unsigned int
>>> reg, unsigned int val)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&ep93xx_swlock, flags);
>>> +
>>> + regmap_write(map, EP93XX_SYSCON_SWLOCK,
>>> EP93XX_SWLOCK_MAGICK);
>>> + regmap_write(map, reg, val);
>>> +
>>> + spin_unlock_irqrestore(&ep93xx_swlock, flags);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(ep93xx_syscon_swlocked_write, EP93XX_SOC);
>>
>> I doubt that your code compiles. Didn't you add a user of this in
>> some
>> earlier patch?
>>
>> Anyway, no, drop it, don't export some weird calls from core initcall
>> to
>> drivers. You violate layering and driver encapsulation. There is no
>> dependency/probe ordering.
>>
>> There is no even need for this, because this code does not use it!
>
> It's a little emotional, so i can hardly understand the exact reason of
> dissatisfaction.
>
> Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my fault
> it's not needed as both PINCTRL and CLK (the only users of this code)
> can't be built as modules.
I am against any exported symbols and most of functions visible outside
of this driver.
...
>>> +
>>> + pr_info("EP93xx SoC revision %s\n", attrs->revision);
>>> +
>>> + return 0;
>>> +}
>>> +core_initcall(ep93xx_soc_init);
>>
>> That's not the way to add soc driver. You need a proper driver for it
>
> What is a proper driver for these ?
Usually platform_driver, e.g. exynos-chipid.
>
> Even the latest additions in drivers/soc/* go with *_initcall.
Which one? Ones which call platform_driver_register()? That's quite
different story, isn't it? I don't see other recent cases, except
regulator couplers. They might be, some people send and accept poor
code, so what do you expect from us? Crappy code got in once, so more of
it can go?
>
> The only i can see is:
> ./versatile/soc-
> realview.c:132:builtin_platform_driver(realview_soc_driver);
>
> By Linus =).
20 years ago module parameters were quite popular. Try to add one now, I
know what comment you will get. Just because something was added by
someone some time ago, is not a reason to do the same. Things change,
Linux changed.
Best regards,
Krzysztof
On Mon, Jul 24, 2023, at 18:31, Krzysztof Kozlowski wrote:
> On 24/07/2023 17:02, Nikita Shubin wrote:
>>>
>>> There is no even need for this, because this code does not use it!
>>
>> It's a little emotional, so i can hardly understand the exact reason of
>> dissatisfaction.
>>
>> Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my fault
>> it's not needed as both PINCTRL and CLK (the only users of this code)
>> can't be built as modules.
>
> I am against any exported symbols and most of functions visible outside
> of this driver.
I think it's a reasonable compromise here, this all ancient code
and we don't need to rewrite all of it as part of the DT conversion,
even if we would do it differently for a new platform.
I really just want to merge the series before Nikita runs out
of motivation to finish the work, after having done almost all
of it.
>>>> +
>>>> + pr_info("EP93xx SoC revision %s\n", attrs->revision);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +core_initcall(ep93xx_soc_init);
>>>
>>> That's not the way to add soc driver. You need a proper driver for it
>>
>> What is a proper driver for these ?
>
> Usually platform_driver, e.g. exynos-chipid.
Using a platform driver sounds like the right thing to do here,
it's cleaner and should not be hard to do if the driver just matches
the cirrus,ep9301-syscon node. I would have just merged
the v3 version, but this is something that makes sense changing
in v4.
>> Even the latest additions in drivers/soc/* go with *_initcall.
>
> Which one? Ones which call platform_driver_register()? That's quite
> different story, isn't it? I don't see other recent cases, except
> regulator couplers. They might be, some people send and accept poor
> code, so what do you expect from us? Crappy code got in once, so more of
> it can go?
That's not what is happening here, this is all part of an incremental
improvement of the existing code. If the DT bindings make sense, I'm
happy to take some shortcuts with the implementation that keep it
closer to the existing implementation and either clean them up over
time or just throw out the platform in five or ten years when the last
machines are dead.
Arnd
On 24/07/2023 20:04, Arnd Bergmann wrote:
>
>>>>> +
>>>>> + pr_info("EP93xx SoC revision %s\n", attrs->revision);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +core_initcall(ep93xx_soc_init);
>>>>
>>>> That's not the way to add soc driver. You need a proper driver for it
>>>
>>> What is a proper driver for these ?
>>
>> Usually platform_driver, e.g. exynos-chipid.
>
> Using a platform driver sounds like the right thing to do here,
> it's cleaner and should not be hard to do if the driver just matches
> the cirrus,ep9301-syscon node. I would have just merged
> the v3 version, but this is something that makes sense changing
> in v4.
>
>>> Even the latest additions in drivers/soc/* go with *_initcall.
>>
>> Which one? Ones which call platform_driver_register()? That's quite
>> different story, isn't it? I don't see other recent cases, except
>> regulator couplers. They might be, some people send and accept poor
>> code, so what do you expect from us? Crappy code got in once, so more of
>> it can go?
>
> That's not what is happening here, this is all part of an incremental
> improvement of the existing code. If the DT bindings make sense, I'm
> happy to take some shortcuts with the implementation that keep it
> closer to the existing implementation and either clean them up over
> time or just throw out the platform in five or ten years when the last
> machines are dead.
I am not saying that this is happening here, but the argument that once
we took some patch is not the reason to keep going. For sure we could
cut here some slack, unlike for big SoC vendors...
Best regards,
Krzysztof
Hello Arnd, Krzysztof !
On Mon, 2023-07-24 at 20:04 +0200, Arnd Bergmann wrote:
> On Mon, Jul 24, 2023, at 18:31, Krzysztof Kozlowski wrote:
> > On 24/07/2023 17:02, Nikita Shubin wrote:
> > > >
> > > > There is no even need for this, because this code does not use
> > > > it!
> > >
> > > It's a little emotional, so i can hardly understand the exact
> > > reason of
> > > dissatisfaction.
> > >
> > > Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my
> > > fault
> > > it's not needed as both PINCTRL and CLK (the only users of this
> > > code)
> > > can't be built as modules.
> >
> > I am against any exported symbols and most of functions visible
> > outside
> > of this driver.
>
> I think it's a reasonable compromise here, this all ancient code
> and we don't need to rewrite all of it as part of the DT conversion,
> even if we would do it differently for a new platform.
>
> I really just want to merge the series before Nikita runs out
> of motivation to finish the work, after having done almost all
> of it.
Thank you for your kind words, i think i have steam for at least for a
couple more versions.
I agree with Krzysztof, through i don't see any good options to dial
with "swlocked" registers (these means we have to write a magic value
to some register just before writing to "swlocked" register) without
exposing some functions - the only variants i can think of:
- introduce a "fake" hwlock, so the drivers will provide magic
themselves while holding the lock
- convert SYSCON, CLK, PINCTRL into "Auxiliary" (suggested by Stephen)
but this introduces more problems:
- as AFAIK CLK can use SYSCON node - but i am not sure about PINCTRL
- it still will have a common header for CLK and PINCTRL (no
functions - just structs however)
>
> > > > > +
> > > > > + pr_info("EP93xx SoC revision %s\n", attrs->revision);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +core_initcall(ep93xx_soc_init);
> > > >
> > > > That's not the way to add soc driver. You need a proper driver
> > > > for it
> > >
> > > What is a proper driver for these ?
> >
> > Usually platform_driver, e.g. exynos-chipid.
>
> Using a platform driver sounds like the right thing to do here,
> it's cleaner and should not be hard to do if the driver just matches
> the cirrus,ep9301-syscon node. I would have just merged
> the v3 version, but this is something that makes sense changing
> in v4.
Indeed.
>
> > > Even the latest additions in drivers/soc/* go with *_initcall.
> >
> > Which one? Ones which call platform_driver_register()? That's quite
> > different story, isn't it? I don't see other recent cases, except
> > regulator couplers. They might be, some people send and accept poor
> > code, so what do you expect from us? Crappy code got in once, so
> > more of
> > it can go?
>
> That's not what is happening here, this is all part of an incremental
> improvement of the existing code. If the DT bindings make sense, I'm
> happy to take some shortcuts with the implementation that keep it
> closer to the existing implementation and either clean them up over
> time or just throw out the platform in five or ten years when the
> last
> machines are dead.
>
> Arnd