* Re: [PATCH v3] i2c: spacemit: configure ILCR for accurate SCL frequency
2025-08-14 9:06 [PATCH v3] i2c: spacemit: configure ILCR for accurate SCL frequency Troy Mitchell
@ 2025-08-14 12:57 ` Troy Mitchell
2025-09-11 5:50 ` Troy Mitchell
2025-09-28 11:03 ` Yixun Lan
2 siblings, 0 replies; 5+ messages in thread
From: Troy Mitchell @ 2025-08-14 12:57 UTC (permalink / raw)
To: Andi Shyti, Yixun Lan
Cc: linux-i2c, linux-kernel, linux-riscv, spacemit, Troy Mitchell
On Thu, Aug 14, 2025 at 05:06:01PM +0800, Troy Mitchell wrote:
> The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
> master mode operations is determined by the ILCR (I2C Load Count Register).
> Previously, the driver relied on the hardware's reset default
> values for this register.
>
> The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
> frequencies lower than intended. For example, with the default
> 31.5 MHz input clock, these default settings result in an SCL
> frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
> and approximately 338 kHz (fast mode) when targeting 400 kHz.
> These frequencies are below the 100 kHz/400 kHz nominal speeds.
>
> This patch integrates the SCL frequency management into
> the Common Clock Framework (CCF). Specifically, the ILCR register,
> which acts as a frequency divider for the SCL clock, is now registered
> as a managed clock (scl_clk) within the CCF.
>
> This patch also cleans up unnecessary whitespace
> in the included header files.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changelog in v3:
> - use MASK macro in `recalc_rate` function
> - rename clock name
oops. There is one more thing written here.
no rename.
> - Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com
>
> Changelog in v2:
> - Align line breaks.
> - Check `lv` in `clk_set_rate` function.
> - Force fast mode when SCL frequency is illegal or unavailable.
> - Change "linux/bits.h" to <linux/bits.h>
> - Kconfig: Add dependency on CCF.
> - Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
> ---
> drivers/i2c/busses/Kconfig | 2 +-
> drivers/i2c/busses/i2c-k1.c | 180 ++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 167 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index c8d115b58e449b59a38339b439190dcb0e332965..1382b6c257fa4ba4cf5098d684c1bbd5e2636fd4 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -797,7 +797,7 @@ config I2C_JZ4780
> config I2C_K1
> tristate "SpacemiT K1 I2C adapter"
> depends on ARCH_SPACEMIT || COMPILE_TEST
> - depends on OF
> + depends on OF && COMMON_CLK
> help
> This option enables support for the I2C interface on the SpacemiT K1
> platform.
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index b68a21fff0b56b59fe2032ccb7ca6953423aad32..34b22969cf6789e99de58dd93dda6f0951069f85 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -3,17 +3,20 @@
> * Copyright (C) 2024-2025 Troy Mitchell <troymitchell988@gmail.com>
> */
>
> - #include <linux/clk.h>
> - #include <linux/i2c.h>
> - #include <linux/iopoll.h>
> - #include <linux/module.h>
> - #include <linux/of_address.h>
> - #include <linux/platform_device.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
>
> /* spacemit i2c registers */
> #define SPACEMIT_ICR 0x0 /* Control register */
> #define SPACEMIT_ISR 0x4 /* Status register */
> #define SPACEMIT_IDBR 0xc /* Data buffer register */
> +#define SPACEMIT_ILCR 0x10 /* Load Count Register */
> #define SPACEMIT_IBMR 0x1c /* Bus monitor register */
>
> /* SPACEMIT_ICR register fields */
> @@ -80,6 +83,19 @@
> #define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
> #define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
>
> +#define SPACEMIT_LCR_LV_STANDARD_SHIFT 0
> +#define SPACEMIT_LCR_LV_FAST_SHIFT 9
> +#define SPACEMIT_LCR_LV_STANDARD_WIDTH 9
> +#define SPACEMIT_LCR_LV_FAST_WIDTH 9
> +#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE GENMASK(SPACEMIT_LCR_LV_STANDARD_WIDTH - 1, 0)
> +#define SPACEMIT_LCR_LV_FAST_MAX_VALUE GENMASK(SPACEMIT_LCR_LV_FAST_WIDTH - 1, 0)
> +#define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(SPACEMIT_LCR_LV_STANDARD_SHIFT +\
> + SPACEMIT_LCR_LV_STANDARD_WIDTH - 1,\
> + SPACEMIT_LCR_LV_STANDARD_SHIFT)
> +#define SPACEMIT_LCR_LV_FAST_MASK GENMASK(SPACEMIT_LCR_LV_FAST_SHIFT +\
> + SPACEMIT_LCR_LV_FAST_WIDTH - 1,\
> + SPACEMIT_LCR_LV_FAST_SHIFT)
> +
> /* i2c bus recover timeout: us */
> #define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000
>
> @@ -95,11 +111,20 @@ enum spacemit_i2c_state {
> SPACEMIT_STATE_WRITE,
> };
>
> +enum spacemit_i2c_mode {
> + SPACEMIT_MODE_STANDARD,
> + SPACEMIT_MODE_FAST
> +};
> +
> /* i2c-spacemit driver's main struct */
> struct spacemit_i2c_dev {
> struct device *dev;
> struct i2c_adapter adapt;
>
> + struct clk_hw scl_clk_hw;
> + struct clk *scl_clk;
> + enum spacemit_i2c_mode mode;
> +
> /* hardware resources */
> void __iomem *base;
> int irq;
> @@ -120,6 +145,88 @@ struct spacemit_i2c_dev {
> u32 status;
> };
>
> +static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
> +{
> + struct spacemit_i2c_dev *i2c = data;
> +
> + clk_disable_unprepare(i2c->scl_clk);
> +}
> +
> +static void spacemit_i2c_scl_clk_exclusive_put(void *data)
> +{
> + struct spacemit_i2c_dev *i2c = data;
> +
> + clk_rate_exclusive_put(i2c->scl_clk);
> +}
> +
> +static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> + u32 lv, lcr, mask, shift, max_lv;
> +
> + lv = DIV_ROUND_UP(parent_rate, rate);
> +
> + if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> + mask = SPACEMIT_LCR_LV_STANDARD_MASK;
> + shift = SPACEMIT_LCR_LV_STANDARD_SHIFT;
> + max_lv = SPACEMIT_LCR_LV_STANDARD_MAX_VALUE;
> + } else if (i2c->mode == SPACEMIT_MODE_FAST) {
> + mask = SPACEMIT_LCR_LV_FAST_MASK;
> + shift = SPACEMIT_LCR_LV_FAST_SHIFT;
> + max_lv = SPACEMIT_LCR_LV_FAST_MAX_VALUE;
> + }
> +
> + if (!lv || lv > max_lv) {
> + dev_err(i2c->dev, "set scl clock failed: lv 0x%x", lv);
> + return -EINVAL;
> + }
> +
> + lcr = readl(i2c->base + SPACEMIT_ILCR);
> + lcr &= ~mask;
> + lcr |= lv << shift;
> + writel(lcr, i2c->base + SPACEMIT_ILCR);
> +
> + return 0;
> +}
> +
> +static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + u32 lv, freq;
> +
> + lv = DIV_ROUND_UP(*parent_rate, rate);
> + freq = DIV_ROUND_UP(*parent_rate, lv);
> +
> + return freq;
> +}
> +
> +static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> + u32 lcr, lv = 0;
> +
> + lcr = readl(i2c->base + SPACEMIT_ILCR);
> +
> + if (i2c->mode == SPACEMIT_MODE_STANDARD)
> + lv = (lcr & SPACEMIT_LCR_LV_STANDARD_MASK) >>
> + SPACEMIT_LCR_LV_STANDARD_SHIFT;
> + else if (i2c->mode == SPACEMIT_MODE_FAST)
> + lv = (lcr & SPACEMIT_LCR_LV_FAST_MASK) >>
> + SPACEMIT_LCR_LV_FAST_SHIFT;
> + else
> + return 0;
> +
> + return DIV_ROUND_UP(parent_rate, lv);
> +}
> +
> +static const struct clk_ops spacemit_i2c_clk_ops = {
> + .set_rate = spacemit_i2c_clk_set_rate,
> + .round_rate = spacemit_i2c_clk_round_rate,
> + .recalc_rate = spacemit_i2c_clk_recalc_rate,
> +};
> +
> static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
> {
> u32 val;
> @@ -138,6 +245,27 @@ static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
> writel(val, i2c->base + SPACEMIT_ICR);
> }
>
> +static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
> + struct clk *parent)
> +{
> + struct clk_init_data init;
> + char name[32];
> +
> + snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
> +
> + init.name = name;
> + init.ops = &spacemit_i2c_clk_ops;
> + init.parent_data = (struct clk_parent_data[]) {
> + { .fw_name = "func" },
> + };
> + init.num_parents = 1;
> + init.flags = 0;
> +
> + i2c->scl_clk_hw.init = &init;
> +
> + return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
> +}
> +
> static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> {
> writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> @@ -224,7 +352,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> */
> val |= SPACEMIT_CR_DRFIE;
>
> - if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> + if (i2c->mode == SPACEMIT_MODE_FAST)
> val |= SPACEMIT_CR_MODE_FAST;
>
> /* disable response to general call */
> @@ -519,14 +647,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
>
> /* For now, this driver doesn't support high-speed. */
> - if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> - i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> + if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
> + i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> + i2c->mode = SPACEMIT_MODE_FAST;
> + } else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> + i2c->mode = SPACEMIT_MODE_STANDARD;
> + } else {
> + dev_warn(i2c->dev, "invalid clock-frequency, using fast mode");
> + i2c->mode = SPACEMIT_MODE_FAST;
> i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> - } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> - i2c->clock_freq, SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
> - i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
> }
>
> i2c->dev = &pdev->dev;
> @@ -548,10 +677,33 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> if (IS_ERR(clk))
> return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
>
> + i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
> + if (IS_ERR(i2c->scl_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
> + "failed to register scl clock\n");
> +
> clk = devm_clk_get_enabled(dev, "bus");
> if (IS_ERR(clk))
> return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
>
> + ret = clk_set_rate_exclusive(i2c->scl_clk, i2c->clock_freq);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to set exclusive rate for SCL clock");
> +
> + ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_exclusive_put, i2c);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register cleanup action for exclusive SCL clock rate");
> +
> + ret = clk_prepare_enable(i2c->scl_clk);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to prepare and enable clock");
> +
> + ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_disable_unprepare, i2c);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register cleanup action for clk disable and unprepare");
> +
> spacemit_i2c_reset(i2c);
>
> i2c_set_adapdata(&i2c->adapt, i2c);
>
> ---
> base-commit: 733923397fd95405a48f165c9b1fbc8c4b0a4681
> change-id: 20250709-k1-i2c-ilcr-ea347e0850a4
>
> Best regards,
> --
> Troy Mitchell <troy.mitchell@linux.spacemit.com>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] i2c: spacemit: configure ILCR for accurate SCL frequency
2025-08-14 9:06 [PATCH v3] i2c: spacemit: configure ILCR for accurate SCL frequency Troy Mitchell
2025-08-14 12:57 ` Troy Mitchell
@ 2025-09-11 5:50 ` Troy Mitchell
2025-09-28 11:03 ` Yixun Lan
2 siblings, 0 replies; 5+ messages in thread
From: Troy Mitchell @ 2025-09-11 5:50 UTC (permalink / raw)
To: Andi Shyti, Yixun Lan
Cc: linux-i2c, linux-kernel, linux-riscv, spacemit, Troy Mitchell
On Thu, Aug 14, 2025 at 05:06:01PM +0800, Troy Mitchell wrote:
> The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
> master mode operations is determined by the ILCR (I2C Load Count Register).
> Previously, the driver relied on the hardware's reset default
> values for this register.
>
> The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
> frequencies lower than intended. For example, with the default
> 31.5 MHz input clock, these default settings result in an SCL
> frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
> and approximately 338 kHz (fast mode) when targeting 400 kHz.
> These frequencies are below the 100 kHz/400 kHz nominal speeds.
>
> This patch integrates the SCL frequency management into
> the Common Clock Framework (CCF). Specifically, the ILCR register,
> which acts as a frequency divider for the SCL clock, is now registered
> as a managed clock (scl_clk) within the CCF.
>
> This patch also cleans up unnecessary whitespace
> in the included header files.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changelog in v3:
> - use MASK macro in `recalc_rate` function
> - rename clock name
> - Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com
Gentle ping. Any comments on this patch?
It was last resent about 3 weeks ago.
- Troy
>
> Changelog in v2:
> - Align line breaks.
> - Check `lv` in `clk_set_rate` function.
> - Force fast mode when SCL frequency is illegal or unavailable.
> - Change "linux/bits.h" to <linux/bits.h>
> - Kconfig: Add dependency on CCF.
> - Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
> ---
> drivers/i2c/busses/Kconfig | 2 +-
> drivers/i2c/busses/i2c-k1.c | 180 ++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 167 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index c8d115b58e449b59a38339b439190dcb0e332965..1382b6c257fa4ba4cf5098d684c1bbd5e2636fd4 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -797,7 +797,7 @@ config I2C_JZ4780
> config I2C_K1
> tristate "SpacemiT K1 I2C adapter"
> depends on ARCH_SPACEMIT || COMPILE_TEST
> - depends on OF
> + depends on OF && COMMON_CLK
> help
> This option enables support for the I2C interface on the SpacemiT K1
> platform.
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index b68a21fff0b56b59fe2032ccb7ca6953423aad32..34b22969cf6789e99de58dd93dda6f0951069f85 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -3,17 +3,20 @@
> * Copyright (C) 2024-2025 Troy Mitchell <troymitchell988@gmail.com>
> */
>
> - #include <linux/clk.h>
> - #include <linux/i2c.h>
> - #include <linux/iopoll.h>
> - #include <linux/module.h>
> - #include <linux/of_address.h>
> - #include <linux/platform_device.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
>
> /* spacemit i2c registers */
> #define SPACEMIT_ICR 0x0 /* Control register */
> #define SPACEMIT_ISR 0x4 /* Status register */
> #define SPACEMIT_IDBR 0xc /* Data buffer register */
> +#define SPACEMIT_ILCR 0x10 /* Load Count Register */
> #define SPACEMIT_IBMR 0x1c /* Bus monitor register */
>
> /* SPACEMIT_ICR register fields */
> @@ -80,6 +83,19 @@
> #define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
> #define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
>
> +#define SPACEMIT_LCR_LV_STANDARD_SHIFT 0
> +#define SPACEMIT_LCR_LV_FAST_SHIFT 9
> +#define SPACEMIT_LCR_LV_STANDARD_WIDTH 9
> +#define SPACEMIT_LCR_LV_FAST_WIDTH 9
> +#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE GENMASK(SPACEMIT_LCR_LV_STANDARD_WIDTH - 1, 0)
> +#define SPACEMIT_LCR_LV_FAST_MAX_VALUE GENMASK(SPACEMIT_LCR_LV_FAST_WIDTH - 1, 0)
> +#define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(SPACEMIT_LCR_LV_STANDARD_SHIFT +\
> + SPACEMIT_LCR_LV_STANDARD_WIDTH - 1,\
> + SPACEMIT_LCR_LV_STANDARD_SHIFT)
> +#define SPACEMIT_LCR_LV_FAST_MASK GENMASK(SPACEMIT_LCR_LV_FAST_SHIFT +\
> + SPACEMIT_LCR_LV_FAST_WIDTH - 1,\
> + SPACEMIT_LCR_LV_FAST_SHIFT)
> +
> /* i2c bus recover timeout: us */
> #define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000
>
> @@ -95,11 +111,20 @@ enum spacemit_i2c_state {
> SPACEMIT_STATE_WRITE,
> };
>
> +enum spacemit_i2c_mode {
> + SPACEMIT_MODE_STANDARD,
> + SPACEMIT_MODE_FAST
> +};
> +
> /* i2c-spacemit driver's main struct */
> struct spacemit_i2c_dev {
> struct device *dev;
> struct i2c_adapter adapt;
>
> + struct clk_hw scl_clk_hw;
> + struct clk *scl_clk;
> + enum spacemit_i2c_mode mode;
> +
> /* hardware resources */
> void __iomem *base;
> int irq;
> @@ -120,6 +145,88 @@ struct spacemit_i2c_dev {
> u32 status;
> };
>
> +static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
> +{
> + struct spacemit_i2c_dev *i2c = data;
> +
> + clk_disable_unprepare(i2c->scl_clk);
> +}
> +
> +static void spacemit_i2c_scl_clk_exclusive_put(void *data)
> +{
> + struct spacemit_i2c_dev *i2c = data;
> +
> + clk_rate_exclusive_put(i2c->scl_clk);
> +}
> +
> +static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> + u32 lv, lcr, mask, shift, max_lv;
> +
> + lv = DIV_ROUND_UP(parent_rate, rate);
> +
> + if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> + mask = SPACEMIT_LCR_LV_STANDARD_MASK;
> + shift = SPACEMIT_LCR_LV_STANDARD_SHIFT;
> + max_lv = SPACEMIT_LCR_LV_STANDARD_MAX_VALUE;
> + } else if (i2c->mode == SPACEMIT_MODE_FAST) {
> + mask = SPACEMIT_LCR_LV_FAST_MASK;
> + shift = SPACEMIT_LCR_LV_FAST_SHIFT;
> + max_lv = SPACEMIT_LCR_LV_FAST_MAX_VALUE;
> + }
> +
> + if (!lv || lv > max_lv) {
> + dev_err(i2c->dev, "set scl clock failed: lv 0x%x", lv);
> + return -EINVAL;
> + }
> +
> + lcr = readl(i2c->base + SPACEMIT_ILCR);
> + lcr &= ~mask;
> + lcr |= lv << shift;
> + writel(lcr, i2c->base + SPACEMIT_ILCR);
> +
> + return 0;
> +}
> +
> +static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + u32 lv, freq;
> +
> + lv = DIV_ROUND_UP(*parent_rate, rate);
> + freq = DIV_ROUND_UP(*parent_rate, lv);
> +
> + return freq;
> +}
> +
> +static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> + u32 lcr, lv = 0;
> +
> + lcr = readl(i2c->base + SPACEMIT_ILCR);
> +
> + if (i2c->mode == SPACEMIT_MODE_STANDARD)
> + lv = (lcr & SPACEMIT_LCR_LV_STANDARD_MASK) >>
> + SPACEMIT_LCR_LV_STANDARD_SHIFT;
> + else if (i2c->mode == SPACEMIT_MODE_FAST)
> + lv = (lcr & SPACEMIT_LCR_LV_FAST_MASK) >>
> + SPACEMIT_LCR_LV_FAST_SHIFT;
> + else
> + return 0;
> +
> + return DIV_ROUND_UP(parent_rate, lv);
> +}
> +
> +static const struct clk_ops spacemit_i2c_clk_ops = {
> + .set_rate = spacemit_i2c_clk_set_rate,
> + .round_rate = spacemit_i2c_clk_round_rate,
> + .recalc_rate = spacemit_i2c_clk_recalc_rate,
> +};
> +
> static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
> {
> u32 val;
> @@ -138,6 +245,27 @@ static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
> writel(val, i2c->base + SPACEMIT_ICR);
> }
>
> +static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
> + struct clk *parent)
> +{
> + struct clk_init_data init;
> + char name[32];
> +
> + snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
> +
> + init.name = name;
> + init.ops = &spacemit_i2c_clk_ops;
> + init.parent_data = (struct clk_parent_data[]) {
> + { .fw_name = "func" },
> + };
> + init.num_parents = 1;
> + init.flags = 0;
> +
> + i2c->scl_clk_hw.init = &init;
> +
> + return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
> +}
> +
> static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> {
> writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> @@ -224,7 +352,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> */
> val |= SPACEMIT_CR_DRFIE;
>
> - if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> + if (i2c->mode == SPACEMIT_MODE_FAST)
> val |= SPACEMIT_CR_MODE_FAST;
>
> /* disable response to general call */
> @@ -519,14 +647,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
>
> /* For now, this driver doesn't support high-speed. */
> - if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> - i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> + if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
> + i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> + i2c->mode = SPACEMIT_MODE_FAST;
> + } else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> + i2c->mode = SPACEMIT_MODE_STANDARD;
> + } else {
> + dev_warn(i2c->dev, "invalid clock-frequency, using fast mode");
> + i2c->mode = SPACEMIT_MODE_FAST;
> i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> - } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> - i2c->clock_freq, SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
> - i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
> }
>
> i2c->dev = &pdev->dev;
> @@ -548,10 +677,33 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> if (IS_ERR(clk))
> return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
>
> + i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
> + if (IS_ERR(i2c->scl_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
> + "failed to register scl clock\n");
> +
> clk = devm_clk_get_enabled(dev, "bus");
> if (IS_ERR(clk))
> return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
>
> + ret = clk_set_rate_exclusive(i2c->scl_clk, i2c->clock_freq);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to set exclusive rate for SCL clock");
> +
> + ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_exclusive_put, i2c);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register cleanup action for exclusive SCL clock rate");
> +
> + ret = clk_prepare_enable(i2c->scl_clk);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to prepare and enable clock");
> +
> + ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_disable_unprepare, i2c);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register cleanup action for clk disable and unprepare");
> +
> spacemit_i2c_reset(i2c);
>
> i2c_set_adapdata(&i2c->adapt, i2c);
>
> ---
> base-commit: 733923397fd95405a48f165c9b1fbc8c4b0a4681
> change-id: 20250709-k1-i2c-ilcr-ea347e0850a4
>
> Best regards,
> --
> Troy Mitchell <troy.mitchell@linux.spacemit.com>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] i2c: spacemit: configure ILCR for accurate SCL frequency
2025-08-14 9:06 [PATCH v3] i2c: spacemit: configure ILCR for accurate SCL frequency Troy Mitchell
2025-08-14 12:57 ` Troy Mitchell
2025-09-11 5:50 ` Troy Mitchell
@ 2025-09-28 11:03 ` Yixun Lan
2025-09-29 1:45 ` Troy Mitchell
2 siblings, 1 reply; 5+ messages in thread
From: Yixun Lan @ 2025-09-28 11:03 UTC (permalink / raw)
To: Troy Mitchell; +Cc: Andi Shyti, linux-i2c, linux-kernel, linux-riscv, spacemit
Hi Troy,
On 17:06 Thu 14 Aug , Troy Mitchell wrote:
> The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
> master mode operations is determined by the ILCR (I2C Load Count Register).
> Previously, the driver relied on the hardware's reset default
> values for this register.
>
> The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
> frequencies lower than intended. For example, with the default
> 31.5 MHz input clock, these default settings result in an SCL
> frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
> and approximately 338 kHz (fast mode) when targeting 400 kHz.
> These frequencies are below the 100 kHz/400 kHz nominal speeds.
>
> This patch integrates the SCL frequency management into
> the Common Clock Framework (CCF). Specifically, the ILCR register,
> which acts as a frequency divider for the SCL clock, is now registered
> as a managed clock (scl_clk) within the CCF.
>
> This patch also cleans up unnecessary whitespace
> in the included header files.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changelog in v3:
> - use MASK macro in `recalc_rate` function
> - rename clock name
> - Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com
>
> Changelog in v2:
> - Align line breaks.
> - Check `lv` in `clk_set_rate` function.
> - Force fast mode when SCL frequency is illegal or unavailable.
> - Change "linux/bits.h" to <linux/bits.h>
> - Kconfig: Add dependency on CCF.
> - Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
> ---
> drivers/i2c/busses/Kconfig | 2 +-
> drivers/i2c/busses/i2c-k1.c | 180 ++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 167 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index c8d115b58e449b59a38339b439190dcb0e332965..1382b6c257fa4ba4cf5098d684c1bbd5e2636fd4 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -797,7 +797,7 @@ config I2C_JZ4780
> config I2C_K1
> tristate "SpacemiT K1 I2C adapter"
> depends on ARCH_SPACEMIT || COMPILE_TEST
> - depends on OF
> + depends on OF && COMMON_CLK
> help
> This option enables support for the I2C interface on the SpacemiT K1
> platform.
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index b68a21fff0b56b59fe2032ccb7ca6953423aad32..34b22969cf6789e99de58dd93dda6f0951069f85 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -3,17 +3,20 @@
> * Copyright (C) 2024-2025 Troy Mitchell <troymitchell988@gmail.com>
> */
>
> - #include <linux/clk.h>
> - #include <linux/i2c.h>
> - #include <linux/iopoll.h>
> - #include <linux/module.h>
> - #include <linux/of_address.h>
> - #include <linux/platform_device.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
>
> /* spacemit i2c registers */
> #define SPACEMIT_ICR 0x0 /* Control register */
> #define SPACEMIT_ISR 0x4 /* Status register */
> #define SPACEMIT_IDBR 0xc /* Data buffer register */
> +#define SPACEMIT_ILCR 0x10 /* Load Count Register */
> #define SPACEMIT_IBMR 0x1c /* Bus monitor register */
>
> /* SPACEMIT_ICR register fields */
> @@ -80,6 +83,19 @@
> #define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
> #define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
>
> +#define SPACEMIT_LCR_LV_STANDARD_SHIFT 0
> +#define SPACEMIT_LCR_LV_FAST_SHIFT 9
> +#define SPACEMIT_LCR_LV_STANDARD_WIDTH 9
> +#define SPACEMIT_LCR_LV_FAST_WIDTH 9
..
> +#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE GENMASK(SPACEMIT_LCR_LV_STANDARD_WIDTH - 1, 0)
> +#define SPACEMIT_LCR_LV_FAST_MAX_VALUE GENMASK(SPACEMIT_LCR_LV_FAST_WIDTH - 1, 0)
using GENMASK() makes people even confusing since it's not a mask, I would
suggest simply put value directly here, which is 0x1ff
> +#define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(SPACEMIT_LCR_LV_STANDARD_SHIFT +\
> + SPACEMIT_LCR_LV_STANDARD_WIDTH - 1,\
> + SPACEMIT_LCR_LV_STANDARD_SHIFT)
same reason here, suggest simplify as
#define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(8, 0)
which is more readable and maintenace friendly
> +#define SPACEMIT_LCR_LV_FAST_MASK GENMASK(SPACEMIT_LCR_LV_FAST_SHIFT +\
> + SPACEMIT_LCR_LV_FAST_WIDTH - 1,\
> + SPACEMIT_LCR_LV_FAST_SHIFT)
> +
..[snip]
> +static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
> + struct clk *parent)
> +{
> + struct clk_init_data init;
> + char name[32];
> +
> + snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
> +
> + init.name = name;
> + init.ops = &spacemit_i2c_clk_ops;
> + init.parent_data = (struct clk_parent_data[]) {
> + { .fw_name = "func" },
> + };
> + init.num_parents = 1;
..
> + init.flags = 0;
I think you can drop this line, by simply init it at declaration
struct clk_init_data init = {};
..
> +
> + i2c->scl_clk_hw.init = &init;
> +
> + return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
> +}
> +
> static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> {
> writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> @@ -224,7 +352,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> */
> val |= SPACEMIT_CR_DRFIE;
>
> - if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> + if (i2c->mode == SPACEMIT_MODE_FAST)
> val |= SPACEMIT_CR_MODE_FAST;
>
> /* disable response to general call */
> @@ -519,14 +647,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
>
> /* For now, this driver doesn't support high-speed. */
> - if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> - i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> + if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
> + i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> + i2c->mode = SPACEMIT_MODE_FAST;
> + } else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> + i2c->mode = SPACEMIT_MODE_STANDARD;
> + } else {
> + dev_warn(i2c->dev, "invalid clock-frequency, using fast mode");
s/using../fallback to fast mode/
> + i2c->mode = SPACEMIT_MODE_FAST;
> i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> - } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> - i2c->clock_freq, SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
> - i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
> }
>
> i2c->dev = &pdev->dev;
> @@ -548,10 +677,33 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> if (IS_ERR(clk))
> return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
>
> + i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
> + if (IS_ERR(i2c->scl_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
> + "failed to register scl clock\n");
> +
> clk = devm_clk_get_enabled(dev, "bus");
> if (IS_ERR(clk))
> return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
you can use dev_err_ptr_probe()
>
> + ret = clk_set_rate_exclusive(i2c->scl_clk, i2c->clock_freq);
why use _exclusive_ API? is there multi consumers? can you explain?
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to set exclusive rate for SCL clock");
> +
--
Yixun Lan (dlan)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] i2c: spacemit: configure ILCR for accurate SCL frequency
2025-09-28 11:03 ` Yixun Lan
@ 2025-09-29 1:45 ` Troy Mitchell
0 siblings, 0 replies; 5+ messages in thread
From: Troy Mitchell @ 2025-09-29 1:45 UTC (permalink / raw)
To: Yixun Lan, Troy Mitchell
Cc: Andi Shyti, linux-i2c, linux-kernel, linux-riscv, spacemit
Hi Yixun, Thanks for your review!
On Sun, Sep 28, 2025 at 07:03:27PM +0800, Yixun Lan wrote:
> Hi Troy,
>
> On 17:06 Thu 14 Aug , Troy Mitchell wrote:
> > The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
> > master mode operations is determined by the ILCR (I2C Load Count Register).
> > Previously, the driver relied on the hardware's reset default
> > values for this register.
> >
> > The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
> > frequencies lower than intended. For example, with the default
> > 31.5 MHz input clock, these default settings result in an SCL
> > frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
> > and approximately 338 kHz (fast mode) when targeting 400 kHz.
> > These frequencies are below the 100 kHz/400 kHz nominal speeds.
> >
> > This patch integrates the SCL frequency management into
> > the Common Clock Framework (CCF). Specifically, the ILCR register,
> > which acts as a frequency divider for the SCL clock, is now registered
> > as a managed clock (scl_clk) within the CCF.
> >
> > This patch also cleans up unnecessary whitespace
> > in the included header files.
> >
> > Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> > ---
> > Changelog in v3:
> > - use MASK macro in `recalc_rate` function
> > - rename clock name
> > - Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com
> >
> > Changelog in v2:
> > - Align line breaks.
> > - Check `lv` in `clk_set_rate` function.
> > - Force fast mode when SCL frequency is illegal or unavailable.
> > - Change "linux/bits.h" to <linux/bits.h>
> > - Kconfig: Add dependency on CCF.
> > - Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
> > ---
> > drivers/i2c/busses/Kconfig | 2 +-
> > drivers/i2c/busses/i2c-k1.c | 180 ++++++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 167 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index c8d115b58e449b59a38339b439190dcb0e332965..1382b6c257fa4ba4cf5098d684c1bbd5e2636fd4 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -797,7 +797,7 @@ config I2C_JZ4780
> > config I2C_K1
> > tristate "SpacemiT K1 I2C adapter"
> > depends on ARCH_SPACEMIT || COMPILE_TEST
> > - depends on OF
> > + depends on OF && COMMON_CLK
> > help
> > This option enables support for the I2C interface on the SpacemiT K1
> > platform.
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index b68a21fff0b56b59fe2032ccb7ca6953423aad32..34b22969cf6789e99de58dd93dda6f0951069f85 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -3,17 +3,20 @@
> > * Copyright (C) 2024-2025 Troy Mitchell <troymitchell988@gmail.com>
> > */
> >
> > - #include <linux/clk.h>
> > - #include <linux/i2c.h>
> > - #include <linux/iopoll.h>
> > - #include <linux/module.h>
> > - #include <linux/of_address.h>
> > - #include <linux/platform_device.h>
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> >
> > /* spacemit i2c registers */
> > #define SPACEMIT_ICR 0x0 /* Control register */
> > #define SPACEMIT_ISR 0x4 /* Status register */
> > #define SPACEMIT_IDBR 0xc /* Data buffer register */
> > +#define SPACEMIT_ILCR 0x10 /* Load Count Register */
> > #define SPACEMIT_IBMR 0x1c /* Bus monitor register */
> >
> > /* SPACEMIT_ICR register fields */
> > @@ -80,6 +83,19 @@
> > #define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
> > #define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
> >
> > +#define SPACEMIT_LCR_LV_STANDARD_SHIFT 0
> > +#define SPACEMIT_LCR_LV_FAST_SHIFT 9
> > +#define SPACEMIT_LCR_LV_STANDARD_WIDTH 9
> > +#define SPACEMIT_LCR_LV_FAST_WIDTH 9
> ..
> > +#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE GENMASK(SPACEMIT_LCR_LV_STANDARD_WIDTH - 1, 0)
> > +#define SPACEMIT_LCR_LV_FAST_MAX_VALUE GENMASK(SPACEMIT_LCR_LV_FAST_WIDTH - 1, 0)
> using GENMASK() makes people even confusing since it's not a mask, I would
> suggest simply put value directly here, which is 0x1ff
>
> > +#define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(SPACEMIT_LCR_LV_STANDARD_SHIFT +\
> > + SPACEMIT_LCR_LV_STANDARD_WIDTH - 1,\
> > + SPACEMIT_LCR_LV_STANDARD_SHIFT)
> same reason here, suggest simplify as
> #define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(8, 0)
> which is more readable and maintenace friendly
Thanks, I will.
>
> > +#define SPACEMIT_LCR_LV_FAST_MASK GENMASK(SPACEMIT_LCR_LV_FAST_SHIFT +\
> > + SPACEMIT_LCR_LV_FAST_WIDTH - 1,\
> > + SPACEMIT_LCR_LV_FAST_SHIFT)
> > +
> ..[snip]
> > +static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
> > + struct clk *parent)
> > +{
> > + struct clk_init_data init;
> > + char name[32];
> > +
> > + snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
> > +
> > + init.name = name;
> > + init.ops = &spacemit_i2c_clk_ops;
> > + init.parent_data = (struct clk_parent_data[]) {
> > + { .fw_name = "func" },
> > + };
> > + init.num_parents = 1;
> ..
> > + init.flags = 0;
> I think you can drop this line, by simply init it at declaration
> struct clk_init_data init = {};
Sounds good to me.
> ..
> > +
> > + i2c->scl_clk_hw.init = &init;
> > +
> > + return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
> > +}
> > +
> > static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> > {
> > writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> > @@ -224,7 +352,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> > */
> > val |= SPACEMIT_CR_DRFIE;
> >
> > - if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> > + if (i2c->mode == SPACEMIT_MODE_FAST)
> > val |= SPACEMIT_CR_MODE_FAST;
> >
> > /* disable response to general call */
> > @@ -519,14 +647,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> > dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
> >
> > /* For now, this driver doesn't support high-speed. */
> > - if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> > - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> > - i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> > + if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
> > + i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> > + i2c->mode = SPACEMIT_MODE_FAST;
> > + } else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> > + i2c->mode = SPACEMIT_MODE_STANDARD;
> > + } else {
> > + dev_warn(i2c->dev, "invalid clock-frequency, using fast mode");
> s/using../fallback to fast mode/
Thanks.
>
> > + i2c->mode = SPACEMIT_MODE_FAST;
> > i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> > - } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> > - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> > - i2c->clock_freq, SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
> > - i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
> > }
> >
> > i2c->dev = &pdev->dev;
> > @@ -548,10 +677,33 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> > if (IS_ERR(clk))
> > return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
> >
> > + i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
> > + if (IS_ERR(i2c->scl_clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
> > + "failed to register scl clock\n");
> > +
> > clk = devm_clk_get_enabled(dev, "bus");
> > if (IS_ERR(clk))
> > return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
> you can use dev_err_ptr_probe()
Oh nice API, I will use it.
>
> >
> > + ret = clk_set_rate_exclusive(i2c->scl_clk, i2c->clock_freq);
> why use _exclusive_ API? is there multi consumers? can you explain?
No multi consumers. I will use clk_set_rate() in the next version.
Thanks for you pointing it out. Nice catch!
- Troy
>
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret, "failed to set exclusive rate for SCL clock");
> > +
>
>
> --
> Yixun Lan (dlan)
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread