Messages in this thread | | | Subject | Re: [PATCH v3 03/42] clk: ep93xx: add DT support for Cirrus EP93xx | From | Nikita Shubin <> | Date | Sun, 23 Jul 2023 21:37:24 +0300 |
| |
Hello Stephen.
On Thu, 2023-07-20 at 16:27 -0700, Stephen Boyd wrote: > Quoting Nikita Shubin via B4 Relay (2023-07-20 04:29:03) > > diff --git a/drivers/clk/clk-ep93xx.c b/drivers/clk/clk-ep93xx.c > > new file mode 100644 > > index 000000000000..7b9b3a0894ab > > --- /dev/null > > +++ b/drivers/clk/clk-ep93xx.c > > @@ -0,0 +1,764 @@ > > +// 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/clk-provider.h> > > +#include <linux/clk.h> > > +#include <linux/clkdev.h> > > Drop the unused includes. > > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/spinlock.h> > > +#include <linux/string.h> > > +#include <linux/sys_soc.h> > > + > > +#include <linux/soc/cirrus/ep93xx.h> > > +#include <dt-bindings/clock/cirrus,ep93xx-clock.h> > > + > > +#include <asm/div64.h> > > + > > +#define EP93XX_EXT_RTC_RATE 32768 > > + > > +#define EP93XX_SYSCON_POWER_STATE 0x00 > > +#define EP93XX_SYSCON_PWRCNT 0x04 > > +#define EP93XX_SYSCON_PWRCNT_UARTBAUD BIT(29) > > +#define EP93XX_SYSCON_PWRCNT_USH_EN 28 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2M1 27 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2M0 26 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P8 25 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P9 24 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P6 23 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P7 22 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P4 21 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P5 20 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P2 19 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P3 18 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P0 17 > > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P1 16 > > +#define EP93XX_SYSCON_DEVCFG 0x80 > > +#define EP93XX_SYSCON_DEVCFG_U3EN 24 > > +#define EP93XX_SYSCON_DEVCFG_U2EN 20 > > +#define EP93XX_SYSCON_DEVCFG_U1EN 18 > > +#define EP93XX_SYSCON_VIDCLKDIV 0x84 > > +#define EP93XX_SYSCON_CLKDIV_ENABLE 15 > > +#define EP93XX_SYSCON_CLKDIV_ESEL BIT(14) > > +#define EP93XX_SYSCON_CLKDIV_PSEL BIT(13) > > +#define EP93XX_SYSCON_CLKDIV_MASK GENMASK(14, 13) > > +#define EP93XX_SYSCON_CLKDIV_PDIV_SHIFT 8 > > +#define EP93XX_SYSCON_I2SCLKDIV 0x8c > > +#define EP93XX_SYSCON_I2SCLKDIV_SENA 31 > > +#define EP93XX_SYSCON_I2SCLKDIV_ORIDE BIT(29) > > +#define EP93XX_SYSCON_I2SCLKDIV_SPOL BIT(19) > > +#define EP93XX_I2SCLKDIV_SDIV (1 << 16) > > +#define EP93XX_I2SCLKDIV_LRDIV32 (0 << 17) > > +#define EP93XX_I2SCLKDIV_LRDIV64 (1 << 17) > > +#define EP93XX_I2SCLKDIV_LRDIV128 (2 << 17) > > +#define EP93XX_I2SCLKDIV_LRDIV_MASK (3 << 17) > > +#define EP93XX_SYSCON_KEYTCHCLKDIV 0x90 > > +#define EP93XX_SYSCON_KEYTCHCLKDIV_TSEN 31 > > +#define EP93XX_SYSCON_KEYTCHCLKDIV_ADIV 16 > > +#define EP93XX_SYSCON_KEYTCHCLKDIV_KEN 15 > > +#define EP93XX_SYSCON_KEYTCHCLKDIV_KDIV 0 > > +#define EP93XX_SYSCON_CHIPID 0x94 > > +#define EP93XX_SYSCON_CHIPID_ID 0x9213 > > + > > +/* Keeps track of all clocks */ > > +static const char adc_divisors[] = { 16, 4 }; > > +static const char sclk_divisors[] = { 2, 4 }; > > +static const char lrclk_divisors[] = { 32, 64, 128 }; > > + > > +#define EP_PARENT(NAME) { .name = NAME, .fw_name = NAME } > > + > > +static const struct clk_parent_data ep93xx_clk_parents[] = { > > + EP_PARENT("xtali"), > > + EP_PARENT("pll1"), > > + EP_PARENT("pll2"), > > pll2 and pll1 aren't in the DT binding, so they shouldn't be set for > .fw_name here. > > > +}; > > + > > +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 device *dev; > > + void __iomem *base; > > + struct regmap *map; > > + struct clk_hw *fixed[16]; > > + struct ep93xx_clk reg[]; > > +}; > > + > > +static struct ep93xx_clk *ep93xx_clk_from(struct clk_hw *hw) > > +{ > > + return container_of(hw, struct ep93xx_clk, hw); > > +} > > + > > +static struct ep93xx_clk_priv *ep93xx_priv_from(struct ep93xx_clk > > *clk) > > +{ > > + return container_of(clk, struct ep93xx_clk_priv, reg[clk- > > >idx]); > > +} > > + > > +static int ep93xx_clk_is_enabled(struct clk_hw *hw) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + u32 val; > > + > > + regmap_read(priv->map, clk->reg, &val); > > + > > + return !!(val & BIT(clk->bit_idx)); > > +} > > + > > +static int ep93xx_clk_enable(struct clk_hw *hw) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + unsigned long flags; > > + u32 val; > > + > > + spin_lock_irqsave(&priv->lock, flags); > > + > > + regmap_read(priv->map, clk->reg, &val); > > + val |= BIT(clk->bit_idx); > > + > > + ep93xx_syscon_swlocked_write(priv->map, clk->reg, val); > > + > > + spin_unlock_irqrestore(&priv->lock, flags); > > + > > + return 0; > > +} > > + > > +static void ep93xx_clk_disable(struct clk_hw *hw) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + unsigned long flags; > > + u32 val; > > + > > + spin_lock_irqsave(&priv->lock, flags); > > + > > + regmap_read(priv->map, clk->reg, &val); > > + val &= ~BIT(clk->bit_idx); > > + > > + ep93xx_syscon_swlocked_write(priv->map, clk->reg, val); > > + > > + spin_unlock_irqrestore(&priv->lock, flags); > > +} > > + > > +static const struct clk_ops clk_ep93xx_gate_ops = { > > + .enable = ep93xx_clk_enable, > > + .disable = ep93xx_clk_disable, > > + .is_enabled = ep93xx_clk_is_enabled, > > +}; > > + > > +static int ep93xx_clk_register_gate(struct ep93xx_clk *clk, > > + const char *name, > > + const char *parent_name, > > + unsigned long flags, > > + unsigned int reg, > > + u8 bit_idx) > > +{ > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + struct clk_parent_data parent_data = { }; > > + struct clk_init_data init = { }; > > + > > + init.name = name; > > + init.ops = &clk_ep93xx_gate_ops; > > + init.flags = flags; > > + > > + parent_data.fw_name = parent_name; > > + parent_data.name = parent_name; > > + init.parent_data = &parent_data; > > + init.num_parents = 1; > > + > > + clk->reg = reg; > > + clk->bit_idx = bit_idx; > > + clk->hw.init = &init; > > + > > + return devm_clk_hw_register(priv->dev, &clk->hw); > > +} > > + > > +static u8 ep93xx_mux_get_parent(struct clk_hw *hw) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + u32 val; > > + > > + regmap_read(priv->map, clk->reg, &val); > > + > > + val &= EP93XX_SYSCON_CLKDIV_MASK; > > + > > + switch (val) { > > + case EP93XX_SYSCON_CLKDIV_ESEL: > > + return 1; // PLL1 > > + case EP93XX_SYSCON_CLKDIV_MASK: > > + return 2; // PLL2 > > + default: > > + break; > > + }; > > + > > + return 0; // XTALI > > Please use /* comments like this */ > > > +} > > + > > +static int ep93xx_mux_set_parent_lock(struct clk_hw *hw, u8 index) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + unsigned long flags; > > + u32 val; > > + > > + if (index >= ARRAY_SIZE(ep93xx_clk_parents)) > > + return -EINVAL; > > + > > + spin_lock_irqsave(&priv->lock, flags); > > + > > + regmap_read(priv->map, clk->reg, &val); > > + val &= ~(EP93XX_SYSCON_CLKDIV_MASK); > > + if (index) { > > + val |= EP93XX_SYSCON_CLKDIV_ESEL; > > + val |= (index - 1) ? EP93XX_SYSCON_CLKDIV_PSEL : 0; > > + } > > + > > + ep93xx_syscon_swlocked_write(priv->map, clk->reg, val); > > + > > + spin_unlock_irqrestore(&priv->lock, flags); > > + > > + return 0; > > +} > > + > > +static bool is_best(unsigned long rate, unsigned long now, > > + unsigned long best) > > +{ > > + return abs(rate - now) < abs(rate - best); > > Another case where we need abs_diff() so that it doesn't get confused > when trying to do signed comparison. > > > +} > > + > > +static int ep93xx_mux_determine_rate(struct clk_hw *hw, > > + struct clk_rate_request *req) > > +{ > > + unsigned long best_rate = 0, actual_rate, mclk_rate; > > + unsigned long rate = req->rate; > > + struct clk_hw *parent_best = NULL; > > + unsigned long parent_rate_best; > > + unsigned long parent_rate; > > + int div, pdiv; > > + unsigned int i; > > + > > + /* > > + * Try the two pll's and the external clock > > + * Because the valid predividers are 2, 2.5 and 3, we > > multiply > > + * all the clocks by 2 to avoid floating point math. > > + * > > + * This is based on the algorithm in the ep93xx raster > > guide: > > + * http://be-a-maverick.com/en/pubs/appNote/AN269REV1.pdf > > + * > > + */ > > + for (i = 0; i < clk_hw_get_num_parents(hw); i++) { > > + struct clk_hw *parent = > > clk_hw_get_parent_by_index(hw, i); > > + > > + parent_rate = clk_hw_get_rate(parent); > > + mclk_rate = parent_rate * 2; > > + > > + /* Try each predivider value */ > > + for (pdiv = 4; pdiv <= 6; pdiv++) { > > + div = DIV_ROUND_CLOSEST(mclk_rate, rate * > > pdiv); > > + if (div < 1 || div > 127) > > + continue; > > + > > + actual_rate = DIV_ROUND_CLOSEST(mclk_rate, > > pdiv * div); > > + if (is_best(rate, actual_rate, best_rate)) > > { > > + best_rate = actual_rate; > > + parent_rate_best = parent_rate; > > + parent_best = parent; > > + } > > + } > > + } > > + > > + if (!parent_best) > > + return -EINVAL; > > + > > + req->best_parent_rate = parent_rate_best; > > + req->best_parent_hw = parent_best; > > + req->rate = best_rate; > > + > > + return 0; > > +} > > + > > +static unsigned long ep93xx_ddiv_recalc_rate(struct clk_hw *hw, > > + unsigned long > > parent_rate) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + unsigned int pdiv, div; > > + u32 val; > > + > > + regmap_read(priv->map, clk->reg, &val); > > + pdiv = ((val >> EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) & 0x03); > > + div = val & GENMASK(6, 0); > > + if (!div) > > + return 0; > > + > > + return DIV_ROUND_CLOSEST(parent_rate * 2, (pdiv + 3) * > > div); > > +} > > + > > +static int ep93xx_ddiv_set_rate(struct clk_hw *hw, unsigned long > > rate, > > + unsigned long parent_rate) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + int pdiv, div, npdiv, ndiv; > > + unsigned long actual_rate, mclk_rate, rate_err = ULONG_MAX; > > + u32 val; > > + > > + regmap_read(priv->map, clk->reg, &val); > > + mclk_rate = parent_rate * 2; > > + > > + for (pdiv = 4; pdiv <= 6; pdiv++) { > > + div = DIV_ROUND_CLOSEST(mclk_rate, rate * pdiv); > > + if (div < 1 || div > 127) > > + continue; > > + > > + actual_rate = DIV_ROUND_CLOSEST(mclk_rate, pdiv * > > div); > > + if (abs(actual_rate - rate) < rate_err) { > > + npdiv = pdiv - 3; > > + ndiv = div; > > + rate_err = abs(actual_rate - rate); > > + } > > + } > > + > > + if (rate_err == ULONG_MAX) > > + return -EINVAL; > > + > > + /* Clear old dividers */ > > + val &= ~(GENMASK(9, 0) & ~BIT(7)); > > + > > + /* Set the new pdiv and div bits for the new clock rate */ > > + val |= (npdiv << EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) | ndiv; > > + > > + ep93xx_syscon_swlocked_write(priv->map, clk->reg, val); > > + > > + return 0; > > +} > > + > > +static const struct clk_ops clk_ddiv_ops = { > > + .enable = ep93xx_clk_enable, > > + .disable = ep93xx_clk_disable, > > + .is_enabled = ep93xx_clk_is_enabled, > > + .get_parent = ep93xx_mux_get_parent, > > + .set_parent = ep93xx_mux_set_parent_lock, > > + .determine_rate = ep93xx_mux_determine_rate, > > + .recalc_rate = ep93xx_ddiv_recalc_rate, > > + .set_rate = ep93xx_ddiv_set_rate, > > +}; > > + > > +static int clk_hw_register_ddiv(struct ep93xx_clk *clk, > > + const char *name, > > + unsigned int reg, > > + u8 bit_idx) > > +{ > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + struct clk_init_data init = { }; > > + > > + init.name = name; > > + init.ops = &clk_ddiv_ops; > > + init.flags = 0; > > + init.parent_data = ep93xx_clk_parents; > > + init.num_parents = ARRAY_SIZE(ep93xx_clk_parents); > > + > > + clk->reg = reg; > > + clk->bit_idx = bit_idx; > > + clk->hw.init = &init; > > + > > + return devm_clk_hw_register(priv->dev, &clk->hw); > > +} > > + > > +static unsigned long ep93xx_div_recalc_rate(struct clk_hw *hw, > > + unsigned long > > parent_rate) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + u32 val; > > + u8 index; > > + > > + regmap_read(priv->map, clk->reg, &val); > > + index = (val & clk->mask) >> clk->shift; > > + if (index > clk->num_div) > > + return 0; > > + > > + return DIV_ROUND_CLOSEST(parent_rate, clk->div[index]); > > +} > > + > > +static long ep93xx_div_round_rate(struct clk_hw *hw, unsigned long > > rate, > > + unsigned long *parent_rate) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + unsigned long best = 0, now; > > + unsigned int i; > > + > > + for (i = 0; i < clk->num_div; i++) { > > + if ((rate * clk->div[i]) == *parent_rate) > > + return rate; > > + > > + now = DIV_ROUND_CLOSEST(*parent_rate, clk->div[i]); > > + if (!best || is_best(rate, now, best)) > > + best = now; > > + } > > + > > + return best; > > +} > > + > > +static int ep93xx_div_set_rate(struct clk_hw *hw, unsigned long > > rate, > > + unsigned long parent_rate) > > +{ > > + struct ep93xx_clk *clk = ep93xx_clk_from(hw); > > + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk); > > + unsigned int i; > > + u32 val; > > + > > + regmap_read(priv->map, clk->reg, &val); > > + val &= ~clk->mask; > > + for (i = 0; i < clk->num_div; i++) > > + if (rate == DIV_ROUND_CLOSEST(parent_rate, clk- > > >div[i])) { > > + val |= i << clk->shift; > > + break; > > + } > > + > > + if (i == clk->num_div) > > + return -EINVAL; > > + > > + ep93xx_syscon_swlocked_write(priv->map, clk->reg, val); > > + > > + return 0; > > +} > > + > > +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, > > + const char *name, > > + const char *parent_name, > > Please try to pass a direct pointer to the parent clk_hw instead or > if > the clk exists outside the controller pass a DT index. > > > + 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_parent_data parent_data = { }; > > + struct clk_init_data init = { }; > > + > > + init.name = name; > > + init.ops = &ep93xx_div_ops; > > + init.flags = 0; > > + parent_data.fw_name = parent_name; > > + parent_data.name = parent_name; > > + 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; > [...] > > + > > +static int ep93xx_clk_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + const struct soc_device_attribute *match; > > + struct ep93xx_clk_priv *priv; > > + struct ep93xx_clk *clk; > > + struct device_node *parent; > > + unsigned long clk_spi_div; > > + int ret; > > + u32 value; > > + > > + priv = devm_kzalloc(dev, struct_size(priv, reg, > > EP93XX_CLK_UART), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + spin_lock_init(&priv->lock); > > + priv->dev = dev; > > + parent = of_get_parent(np); > > + if (!parent) > > + return dev_err_probe(dev, -EINVAL, "no syscon > > parent for clk node\n"); > > + > > + priv->map = syscon_node_to_regmap(parent); > > + if (IS_ERR(priv->map)) { > > + of_node_put(parent); > > + return dev_err_probe(dev, -EINVAL, "no syscon > > regmap\n"); > > + } > > + > > + priv->base = devm_of_iomap(dev, parent, 0, NULL); > > + of_node_put(parent); > > + if (IS_ERR(priv->base)) > > + return PTR_ERR(priv->base); > > + > > + ret = ep93xx_uart_clock_init(priv); > > + if (ret) > > + return ret; > > + > > + ret = ep93xx_dma_clock_init(priv); > > + if (ret) > > + return ret; > > + > > + /* > > + * EP93xx SSP clock rate was doubled in version E2. For > > more information > > + * see: > > + * http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf > > + */ > > + clk_spi_div = 2; > > + match = soc_device_match(ep93xx_soc_table); > > Why isn't this part of the compatible string for the device? We're > able > to introduce new bindings, correct?
It's not a SoC version related, so we don't know such things in advance.
It's currently determined by querying "ep93xx_chip_revision" which determines chip revision:
https://elixir.bootlin.com/linux/v6.5-rc2/source/arch/arm/mach-ep93xx/clock.c#L631
I decided it's a good idea to drop another header exposed function.
"soc_device_match" only uses revision:
``` static const struct soc_device_attribute ep93xx_soc_table[] = { { .revision = "E2", .data = (void *)1 }, { /* sentinel */ } }; ```
But as there are no revisions greater than E2 - it is single match only.
> > > + if (match) > > + clk_spi_div = (unsigned long)match->data;
| |