Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx
From: Nikita Shubin
Date: Mon Jul 24 2023 - 11:02:30 EST
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 <nikita.shubin@xxxxxxxxxxx>
> >
> > 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 <nikita.shubin@xxxxxxxxxxx>
> > Tested-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
> > Acked-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
> > Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > ---
> > 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 <nikita.shubin@xxxxxxxxxxx>
> > + *
> > + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> > + * Copyright (C) 2006 Lennert Buytenhek <buytenh@xxxxxxxxxxxxxx>
> > + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@xxxxxxx>
> > + *
> > + * 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
>