Re: [PATCH v11 03/38] clk: ep93xx: add DT support for Cirrus EP93xx

[Stephen Boyd <sboyd@kernel.org>]

Quoting Nikita Shubin via B4 Relay (2024-07-15 01:38:07)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 3e9099504fad..234b0a8b7650 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -218,6 +218,14 @@ config COMMON_CLK_EN7523
>           This driver provides the fixed clocks and gates present on Airoha
>           ARM silicon.
>  
> +config COMMON_CLK_EP93XX
> +       bool "Clock driver for Cirrus Logic ep93xx SoC"

tristate?

> +       depends on ARCH_EP93XX || COMPILE_TEST
> +       select MFD_SYSCON

Why is this selecting syscon?

> +       select REGMAP

Is this needed either?

> +       help
> +         This driver supports the SoC clocks on the Cirrus Logic ep93xx.
> +
>  config COMMON_CLK_FSL_FLEXSPI
>         tristate "Clock driver for FlexSPI on Layerscape SoCs"
>         depends on ARCH_LAYERSCAPE || COMPILE_TEST
> diff --git a/drivers/clk/clk-ep93xx.c b/drivers/clk/clk-ep93xx.c
> new file mode 100644
> index 000000000000..bb1cf59a3d47
> --- /dev/null
> +++ b/drivers/clk/clk-ep93xx.c
> @@ -0,0 +1,846 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Clock control for Cirrus EP93xx chips.
> + * Copyright (C) 2021 Nikita Shubin <nikita.shubin@maquefel.me>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/clock.c:
> + * Copyright (C) 2006 Lennert Buytenhek <buytenh@wantstofly.org>
> + */
> +#define pr_fmt(fmt) "ep93xx " KBUILD_MODNAME ": " fmt
> +
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk-provider.h>
> +#include <linux/math.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +
> +#include <linux/soc/cirrus/ep93xx.h>
> +#include <dt-bindings/clock/cirrus,ep9301-syscon.h>
> +
> +#include <asm/div64.h>
[...]
> +
> +static const char adc_divisors[] = { 16, 4 };

These are global symbols in terms of namespace because we're in the
kernel. Please prefix with ep93xx_ to help tags.

> +static const char sclk_divisors[] = { 2, 4 };
> +static const char lrclk_divisors[] = { 32, 64, 128 };
> +
> +struct ep93xx_clk {
> +       struct clk_hw hw;
> +       u16 idx;
> +       u16 reg;
> +       u32 mask;
> +       u8 bit_idx;
> +       u8 shift;
> +       u8 width;
> +       u8 num_div;
> +       const char *div;
> +};
> +
> +struct ep93xx_clk_priv {
> +       spinlock_t lock;
> +       struct ep93xx_regmap_adev *aux_dev;
> +       struct device *dev;
> +       void __iomem *base;
> +       struct regmap *map;
> +       struct clk_hw *fixed[21];

Please make a define for '21'.

> +       struct ep93xx_clk reg[];
> +};
[...]
> +
> +static const struct clk_ops ep93xx_div_ops = {
> +       .enable = ep93xx_clk_enable,
> +       .disable = ep93xx_clk_disable,
> +       .is_enabled = ep93xx_clk_is_enabled,
> +       .recalc_rate = ep93xx_div_recalc_rate,
> +       .round_rate = ep93xx_div_round_rate,
> +       .set_rate = ep93xx_div_set_rate,
> +};
> +
> +static int clk_hw_register_div(struct ep93xx_clk *clk,

Please call this something like ep93xx_register_div(). It doesn't take a
clk_hw pointer so the clk_hw prefix doesn't make sense. This same
comment applies to other clk_hw prefixed functions in this file.

> +                              const char *name,
> +                              struct clk_parent_data *parent_data,

const?

> +                              unsigned int reg,
> +                              u8 enable_bit,
> +                              u8 shift,
> +                              u8 width,
> +                              const char *clk_divisors,
> +                              u8 num_div)
> +{
> +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> +       struct clk_init_data init = { };
> +
> +       init.name = name;
> +       init.ops = &ep93xx_div_ops;
> +       init.flags = 0;
> +       init.parent_data = parent_data;
> +       init.num_parents = 1;
> +
> +       clk->reg = reg;
> +       clk->bit_idx = enable_bit;
> +       clk->mask = GENMASK(shift + width - 1, shift);
> +       clk->shift = shift;
> +       clk->div = clk_divisors;
> +       clk->num_div = num_div;
> +       clk->hw.init = &init;
> +
> +       return devm_clk_hw_register(priv->dev, &clk->hw);
> +}
> +
> +struct ep93xx_gate {
> +       unsigned int idx;
> +       unsigned int bit;
> +       const char *name;
> +};
> +
> +static const struct ep93xx_gate ep93xx_uarts[] = {
> +       { EP93XX_CLK_UART1, EP93XX_SYSCON_DEVCFG_U1EN, "uart1" },
> +       { EP93XX_CLK_UART2, EP93XX_SYSCON_DEVCFG_U2EN, "uart2" },
> +       { EP93XX_CLK_UART3, EP93XX_SYSCON_DEVCFG_U3EN, "uart3" },
> +};
> +
> +static int ep93xx_uart_clock_init(struct ep93xx_clk_priv *priv)
> +{
> +       struct clk_parent_data parent_data = { };
> +       unsigned int i, idx, ret, clk_uart_div;
> +       struct ep93xx_clk *clk;
> +       u32 val;
> +
> +       regmap_read(priv->map, EP93XX_SYSCON_PWRCNT, &val);
> +       if (val & EP93XX_SYSCON_PWRCNT_UARTBAUD)
> +               clk_uart_div = 1;
> +       else
> +               clk_uart_div = 2;
> +
> +       priv->fixed[EP93XX_CLK_UART] =
> +               clk_hw_register_fixed_factor(NULL, "uart", "xtali", 0, 1, clk_uart_div);

Pass a device instead of NULL. Don't use string names for parent ^

> +       parent_data.hw = priv->fixed[EP93XX_CLK_UART];
> +
> +       /* parenting uart gate clocks to uart clock */
> +       for (i = 0; i < ARRAY_SIZE(ep93xx_uarts); i++) {
> +               idx = ep93xx_uarts[i].idx - EP93XX_CLK_UART1;
> +               clk = &priv->reg[idx];
> +               clk->idx = idx;
> +               ret = ep93xx_clk_register_gate(clk,
> +                                       ep93xx_uarts[i].name,
> +                                       &parent_data, CLK_SET_RATE_PARENT,
> +                                       EP93XX_SYSCON_DEVCFG,
> +                                       ep93xx_uarts[i].bit);
> +               if (ret)
> +                       return dev_err_probe(priv->dev, ret,
> +                                            "failed to register uart[%d] clock\n", i);
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct ep93xx_gate ep93xx_dmas[] = {
> +       { EP93XX_CLK_M2M0, EP93XX_SYSCON_PWRCNT_DMA_M2M0, "m2m0" },
> +       { EP93XX_CLK_M2M1, EP93XX_SYSCON_PWRCNT_DMA_M2M1, "m2m1" },
> +       { EP93XX_CLK_M2P0, EP93XX_SYSCON_PWRCNT_DMA_M2P0, "m2p0" },
> +       { EP93XX_CLK_M2P1, EP93XX_SYSCON_PWRCNT_DMA_M2P1, "m2p1" },
> +       { EP93XX_CLK_M2P2, EP93XX_SYSCON_PWRCNT_DMA_M2P2, "m2p2" },
> +       { EP93XX_CLK_M2P3, EP93XX_SYSCON_PWRCNT_DMA_M2P3, "m2p3" },
> +       { EP93XX_CLK_M2P4, EP93XX_SYSCON_PWRCNT_DMA_M2P4, "m2p4" },
> +       { EP93XX_CLK_M2P5, EP93XX_SYSCON_PWRCNT_DMA_M2P5, "m2p5" },
> +       { EP93XX_CLK_M2P6, EP93XX_SYSCON_PWRCNT_DMA_M2P6, "m2p6" },
> +       { EP93XX_CLK_M2P7, EP93XX_SYSCON_PWRCNT_DMA_M2P7, "m2p7" },
> +       { EP93XX_CLK_M2P8, EP93XX_SYSCON_PWRCNT_DMA_M2P8, "m2p8" },
> +       { EP93XX_CLK_M2P9, EP93XX_SYSCON_PWRCNT_DMA_M2P9, "m2p9" },
> +};
> +
> +static int ep93xx_dma_clock_init(struct ep93xx_clk_priv *priv)
> +{
> +       struct clk_parent_data parent_data = { };
> +       unsigned int i, idx;
> +
> +       parent_data.hw = priv->fixed[EP93XX_CLK_HCLK];
> +       for (i = 0; i < ARRAY_SIZE(ep93xx_dmas); i++) {
> +               idx = ep93xx_dmas[i].idx;
> +               priv->fixed[idx] = devm_clk_hw_register_gate_parent_data(priv->dev,
> +                                       ep93xx_dmas[i].name,
> +                                       &parent_data, 0,
> +                                       priv->base + EP93XX_SYSCON_PWRCNT,
> +                                       ep93xx_dmas[i].bit,
> +                                       0,
> +                                       &priv->lock);
> +               if (IS_ERR(priv->fixed[idx]))
> +                       return PTR_ERR(priv->fixed[idx]);
> +       }
> +
> +       return 0;
> +}
> +
> +static struct clk_hw *of_clk_ep93xx_get(struct of_phandle_args *clkspec, void *data)
> +{
> +       struct ep93xx_clk_priv *priv = data;
> +       unsigned int idx = clkspec->args[0];
> +
> +       if (idx < EP93XX_CLK_UART1)
> +               return priv->fixed[idx];
> +
> +       if (idx <= EP93XX_CLK_I2S_LRCLK)
> +               return &priv->reg[idx - EP93XX_CLK_UART1].hw;
> +
> +       return ERR_PTR(-EINVAL);
> +}
> +
> +/*
> + * PLL rate = 14.7456 MHz * (X1FBD + 1) * (X2FBD + 1) / (X2IPD + 1) / 2^PS
> + */
> +static unsigned long 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) & GENMASK(1, 0);           /* PS */
> +
> +       return rate;
> +}
> +
> +#define devm_ep93xx_clk_hw_register_fixed_rate_parent_data(dev, name, parent_data, flags, fixed_rate)  \
> +       __clk_hw_register_fixed_rate((dev), NULL, (name), NULL, NULL, \
> +                                    (parent_data), (flags), (fixed_rate), 0, 0, true)

Is this to workaround a missing devm_clk_hw_register_fixed_rate_parent_data() macro?