From: Stephen Boyd <sboyd@kernel.org>
To: Alexander Sverdlin <alexander.sverdlin@gmail.com>,
Conor Dooley <conor+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Michael Turquette <mturquette@baylibre.com>,
Nikita Shubin via B4 Relay
<devnull+nikita.shubin.maquefel.me@kernel.org>,
Rob Herring <robh@kernel.org>,
nikita.shubin@maquefel.me
Cc: linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
devicetree@vger.kernel.org,
Nikita Shubin <nikita.shubin@maquefel.me>,
Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH 2/4] clk: ep93xx: add DT support for Cirrus EP93xx
Date: Thu, 11 Apr 2024 02:32:03 -0700 [thread overview]
Message-ID: <eea032865f00a0d54b99bbe66d4b6135.sboyd@kernel.org> (raw)
In-Reply-To: <20240408-ep93xx-clk-v1-2-1d0f4c324647@maquefel.me>
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@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/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);
next prev parent reply other threads:[~2024-04-11 9:32 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-08 8:09 [PATCH 0/4] DONOTMERGE: ep93xx-clk from ep93xx device tree conversion Nikita Shubin via B4 Relay
2024-04-08 8:09 ` [PATCH 1/4] ARM: ep93xx: add regmap aux_dev Nikita Shubin via B4 Relay
2024-04-11 9:32 ` Stephen Boyd
2024-04-08 8:09 ` [PATCH 2/4] clk: ep93xx: add DT support for Cirrus EP93xx Nikita Shubin via B4 Relay
2024-04-11 9:32 ` Stephen Boyd [this message]
2024-04-14 9:35 ` Nikita Shubin
2024-04-08 8:09 ` [PATCH 3/4] dt-bindings: soc: Add " Nikita Shubin via B4 Relay
2024-04-11 9:21 ` Stephen Boyd
2024-04-08 8:09 ` [PATCH 4/4] soc: Add SoC driver for Cirrus ep93xx Nikita Shubin via B4 Relay
2024-04-11 9:26 ` Stephen Boyd
2024-04-08 17:03 ` [PATCH 0/4] DONOTMERGE: ep93xx-clk from ep93xx device tree conversion Conor Dooley
2024-04-09 11:48 ` Nikita Shubin
2024-04-09 15:09 ` Alexander Sverdlin
2024-04-09 17:32 ` Conor Dooley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eea032865f00a0d54b99bbe66d4b6135.sboyd@kernel.org \
--to=sboyd@kernel.org \
--cc=alexander.sverdlin@gmail.com \
--cc=arnd@arndb.de \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=devnull+nikita.shubin.maquefel.me@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=nikita.shubin@maquefel.me \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).