Re: [PATCH 2/4] clk: ep93xx: add DT support for Cirrus EP93xx

[Stephen Boyd]

Quoting Nikita Shubin via B4 Relay (2024-04-08 01:09:54)
> diff --git a/drivers/clk/clk-ep93xx.c b/drivers/clk/clk-ep93xx.c
> new file mode 100644
> index 000000000000..601acb4402be
> --- /dev/null
> +++ b/drivers/clk/clk-ep93xx.c
> @@ -0,0 +1,840 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Clock control for Cirrus EP93xx chips.
> + * Copyright (C) 2021 Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/clock.c:
> + * Copyright (C) 2006 Lennert Buytenhek <buytenh@xxxxxxxxxxxxxx>
> + */
> +#define pr_fmt(fmt) "ep93xx " KBUILD_MODNAME ": " fmt
> +
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>

Is this include used?

> +#include <linux/math.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>

Is this include used?

> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +#include <linux/sys_soc.h>
> +
> +#include <linux/soc/cirrus/ep93xx.h>
> +#include <dt-bindings/clock/cirrus,ep9301-syscon.h>
> +
> +#include <asm/div64.h>


> +
[...]
> +
> +static const struct soc_device_attribute ep93xx_soc_table[] = {
> +       { .revision = "E2", .data = (void *)1 },

Can we populate a different named clk auxiliary device instead?
Preferably this table and logic lives in the soc driver that creates the
device for this driver.

> +       { /* sentinel */ }
> +};
> +
> +static int ep93xx_clk_probe(struct auxiliary_device *adev,
> +                              const struct auxiliary_device_id *id)
> +{
> +       struct ep93xx_regmap_adev *rdev = to_ep93xx_regmap_adev(adev);
> +       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 };
> +       struct clk_parent_data xtali = { .index = 0 };
> +       struct clk_parent_data ddiv_pdata[3] = { };
> +       unsigned long clk_pll1_rate, clk_pll2_rate, clk_spi_div;
> +       const struct soc_device_attribute *match;
> +       struct clk_parent_data pdata = {};
> +       struct device *dev = &adev->dev;
> +       struct ep93xx_clk_priv *priv;
> +       struct ep93xx_clk *clk;
> +       struct clk_hw *hw, *pll1;
> +       unsigned int idx;
> +       int ret;
> +       u32 value;

Maybe make the pll registration code a sub-function. This is a lot of
local variables.

> +
> +       priv = devm_kzalloc(dev, struct_size(priv, reg, 10), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       spin_lock_init(&priv->lock);
> +       priv->dev = dev;
> +       priv->aux_dev = rdev;
> +       priv->map = rdev->map;
> +       priv->base = rdev->base;
> +
> +       /* Determine the bootloader configured pll1 rate */
> +       regmap_read(priv->map, EP93XX_SYSCON_CLKSET1, &value);
> +
> +       if (value & EP93XX_SYSCON_CLKSET1_NBYP1)
> +               clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
> +       else
> +               clk_pll1_rate = EP93XX_EXT_CLK_RATE;
> +
> +       pll1 = devm_clk_hw_register_fixed_rate(dev, "pll1", "xtali", 0, clk_pll1_rate);
> +       if (IS_ERR(pll1))
> +               return PTR_ERR(pll1);
> +
> +       priv->fixed[EP93XX_CLK_PLL1] = pll1;
> +
> +       /* Initialize the pll1 derived clocks */
> +       clk_f_div = fclk_divisors[(value >> 25) & GENMASK(2, 0)];
> +       clk_h_div = hclk_divisors[(value >> 20) & GENMASK(2, 0)];
> +       clk_p_div = pclk_divisors[(value >> 18) & GENMASK(1, 0)];
> +
> +       hw = devm_clk_hw_register_fixed_factor_parent_hw(dev, "fclk", pll1, 0, 1, clk_f_div);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       priv->fixed[EP93XX_CLK_FCLK] = hw;
> +
> +       hw = devm_clk_hw_register_fixed_factor_parent_hw(dev, "hclk", pll1, 0, 1, clk_h_div);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       priv->fixed[EP93XX_CLK_HCLK] = hw;
> +
> +       hw = devm_clk_hw_register_fixed_factor_parent_hw(dev, "pclk", hw, 0, 1, clk_p_div);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       priv->fixed[EP93XX_CLK_PCLK] = hw;
> +
> +       /* Determine the bootloader configured pll2 rate */
> +       regmap_read(priv->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 = devm_clk_hw_register_fixed_rate(dev, "pll2", "xtali", 0, clk_pll2_rate);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       priv->fixed[EP93XX_CLK_PLL2] = hw;
> +
> +       regmap_read(priv->map, EP93XX_SYSCON_CLKSET2, &value);
> +       clk_usb_div = (value >> 28 & GENMASK(3, 0)) + 1;
> +       hw = devm_clk_hw_register_fixed_factor(dev, "usb_clk", "pll2", 0, 1, clk_usb_div);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       priv->fixed[EP93XX_CLK_USB] = hw;
> +
> +       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 section 6 "2x SSP (Synchronous Serial Port) Clock – Revision E2 only":
> +        *     http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf
> +        */
> +       clk_spi_div = 2;
> +       match = soc_device_match(ep93xx_soc_table);
> +       if (match)
> +               clk_spi_div = (unsigned long)match->data;
> +
> +       hw = devm_clk_hw_register_fixed_factor(dev, "ep93xx-spi.0", "xtali",
> +                                              0, 1, clk_spi_div);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       priv->fixed[EP93XX_CLK_SPI] = hw;
> +
> +       /* PWM clock */
> +       hw = devm_clk_hw_register_fixed_factor(dev, "pwm_clk", "xtali", 0, 1, 1);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       priv->fixed[EP93XX_CLK_PWM] = hw;
> +
> +       /* USB clock */
> +       hw = devm_clk_hw_register_gate(priv->dev, "ohci-platform", "usb_clk",
> +                                      0, priv->base + EP93XX_SYSCON_PWRCNT,
> +                                      EP93XX_SYSCON_PWRCNT_USH_EN, 0,
> +                                      &priv->lock);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       priv->fixed[EP93XX_CLK_USB] = hw;
> +
> +       ddiv_pdata[0].index = 0; /* XTALI external clock */
> +       ddiv_pdata[1].hw = priv->fixed[EP93XX_CLK_PLL1];
> +       ddiv_pdata[2].hw = priv->fixed[EP93XX_CLK_PLL2];
> +
> +       /* touchscreen/ADC clock */
> +       idx = EP93XX_CLK_ADC - EP93XX_CLK_UART1;
> +       clk = &priv->reg[idx];
> +       clk->idx = idx;
> +       ret = clk_hw_register_div(clk, "ep93xx-adc", &xtali,

Use devm?

> +                               EP93XX_SYSCON_KEYTCHCLKDIV,
> +                               EP93XX_SYSCON_KEYTCHCLKDIV_TSEN,
> +                               EP93XX_SYSCON_KEYTCHCLKDIV_ADIV,
> +                               1,
> +                               adc_divisors,
> +                               ARRAY_SIZE(adc_divisors));
> +
> +
> +       /* keypad clock */
> +       idx = EP93XX_CLK_KEYPAD - EP93XX_CLK_UART1;
> +       clk = &priv->reg[idx];
> +       clk->idx = idx;
> +       ret = clk_hw_register_div(clk, "ep93xx-keypad", &xtali,
> +                               EP93XX_SYSCON_KEYTCHCLKDIV,
> +                               EP93XX_SYSCON_KEYTCHCLKDIV_KEN,
> +                               EP93XX_SYSCON_KEYTCHCLKDIV_KDIV,
> +                               1,
> +                               adc_divisors,
> +                               ARRAY_SIZE(adc_divisors));
> +
> +       /*
> +        * On reset PDIV and VDIV is set to zero, while PDIV zero
> +        * means clock disable, VDIV shouldn't be zero.
> +        * So we set both video and i2s dividers to minimum.
> +        * ENA - Enable CLK divider.
> +        * PDIV - 00 - Disable clock
> +        * VDIV - at least 2
> +        */
> +
> +       /* Check and enable video clk registers */
> +       regmap_read(priv->map, EP93XX_SYSCON_VIDCLKDIV, &value);
> +       value |= BIT(EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) | 2;
> +       ep93xx_clk_write(priv, EP93XX_SYSCON_VIDCLKDIV, value);
> +
> +       /* Check and enable i2s clk registers */
> +       regmap_read(priv->map, EP93XX_SYSCON_I2SCLKDIV, &value);
> +       value |= BIT(EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) | 2;
> +
> +       /*
> +        * Override the SAI_MSTR_CLK_CFG from the I2S block and use the
> +        * I2SClkDiv Register settings. LRCLK transitions on the falling SCLK
> +        * edge.
> +        */
> +       value |= EP93XX_SYSCON_I2SCLKDIV_ORIDE | EP93XX_SYSCON_I2SCLKDIV_SPOL;
> +       ep93xx_clk_write(priv, EP93XX_SYSCON_I2SCLKDIV, value);
> +
> +       /* video clk */
> +       idx = EP93XX_CLK_VIDEO - EP93XX_CLK_UART1;
> +       clk = &priv->reg[idx];
> +       clk->idx = idx;
> +       ret = clk_hw_register_ddiv(clk, "ep93xx-fb",
> +                               ddiv_pdata, ARRAY_SIZE(ddiv_pdata),
> +                               EP93XX_SYSCON_VIDCLKDIV,
> +                               EP93XX_SYSCON_CLKDIV_ENABLE);
> +
> +       /* i2s clk */
> +       idx = EP93XX_CLK_I2S_MCLK - EP93XX_CLK_UART1;
> +       clk = &priv->reg[idx];
> +       clk->idx = idx;
> +       ret = clk_hw_register_ddiv(clk, "mclk",
> +                               ddiv_pdata, ARRAY_SIZE(ddiv_pdata),
> +                               EP93XX_SYSCON_I2SCLKDIV,
> +                               EP93XX_SYSCON_CLKDIV_ENABLE);
> +
> +       /* i2s sclk */
> +       idx = EP93XX_CLK_I2S_SCLK - EP93XX_CLK_UART1;
> +       clk = &priv->reg[idx];
> +       clk->idx = idx;
> +       pdata.hw = &priv->reg[EP93XX_CLK_I2S_MCLK - EP93XX_CLK_UART1].hw;
> +       ret = clk_hw_register_div(clk, "sclk", &pdata,
> +                               EP93XX_SYSCON_I2SCLKDIV,
> +                               EP93XX_SYSCON_I2SCLKDIV_SENA,
> +                               16, /* EP93XX_I2SCLKDIV_SDIV_SHIFT */
> +                               1,  /* EP93XX_I2SCLKDIV_SDIV_WIDTH */
> +                               sclk_divisors,
> +                               ARRAY_SIZE(sclk_divisors));
> +
> +       /* i2s lrclk */
> +       idx = EP93XX_CLK_I2S_LRCLK - EP93XX_CLK_UART1;
> +       clk = &priv->reg[idx];
> +       clk->idx = idx;
> +       pdata.hw = &priv->reg[EP93XX_CLK_I2S_SCLK - EP93XX_CLK_UART1].hw;
> +       ret = clk_hw_register_div(clk, "lrclk", &pdata,
> +                               EP93XX_SYSCON_I2SCLKDIV,
> +                               EP93XX_SYSCON_I2SCLKDIV_SENA,
> +                               17, /* EP93XX_I2SCLKDIV_LRDIV32_SHIFT */
> +                               2,  /* EP93XX_I2SCLKDIV_LRDIV32_WIDTH */
> +                               lrclk_divisors,
> +                               ARRAY_SIZE(lrclk_divisors));
> +
> +       /* IrDa clk uses same pattern but no init code presents in original clock driver */
> +       return devm_of_clk_add_hw_provider(priv->dev, of_clk_ep93xx_get, priv);
> +}
> +
> +static const struct auxiliary_device_id ep93xx_clk_ids[] = {
> +       {
> +               .name = "soc_ep93xx.clk-ep93xx",
> +       },

This can be one line instead of three.

> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, ep93xx_clk_ids);