Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:

From: Nikita Shubin nikita.shubin@maquefel.me

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@maquefel.me Tested-by: Alexander Sverdlin alexander.sverdlin@gmail.com Acked-by: Alexander Sverdlin alexander.sverdlin@gmail.com Reviewed-by: Linus Walleij linus.walleij@linaro.org

---

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@maquefel.me
• • Based on a rewrite of arch/arm/mach-ep93xx/core.c
• • Copyright (C) 2006 Lennert Buytenhek buytenh@wantstofly.org
• • Copyright (C) 2007 Herbert Valerio Riedel hvr@gnu.org
• • 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