| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <020c15c9939c1c4cfac925942a582cee.sboyd@kernel.org>
Date: Wed, 28 Aug 2024 13:44:59 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Michael Turquette <mturquette@...libre.com>, Nikita Shubin via B4 Relay <devnull+nikita.shubin.maquefel.me@...nel.org>, nikita.shubin@...uefel.me
Cc: linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v11 03/38] clk: ep93xx: add DT support for Cirrus EP93xx
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@...uefel.me>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/clock.c:
> + * Copyright (C) 2006 Lennert Buytenhek <buytenh@...tstofly.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?
Powered by blists - more mailing lists