[PATCH] i2c: ls2x: Fix frequency division register access

[PATCH] i2c: ls2x: Fix frequency division register access

[Binbin Zhou]

According to the chip manual, the I2C register access type of
Loongson-2K2000/LS7A is "B", so we can only access registers in byte
form (readb/writeb).

Although Loongson-2K0500/Loongson-2K1000 do not have similar
constraints, register accesses in byte form also behave correctly.

Also, in hardware, the frequency division registers are defined as two
separate registers (high 8-bit and low 8-bit), so we just access them
directly as bytes.

Cc: stable@vger.kernel.org
Fixes: 015e61f0bffd ("i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller")
Co-developed-by: Hongliang Wang <wanghongliang@loongson.cn>
Signed-off-by: Hongliang Wang <wanghongliang@loongson.cn>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/i2c-ls2x.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c
index 8821cac3897b..a4dbbf4ca036 100644
--- a/drivers/i2c/busses/i2c-ls2x.c
+++ b/drivers/i2c/busses/i2c-ls2x.c
@@ -10,6 +10,7 @@
  * Rewritten for mainline by Binbin Zhou <zhoubinbin@loongson.cn>
  */
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/completion.h>
 #include <linux/device.h>
@@ -26,7 +27,8 @@
 #include <linux/units.h>
 
 /* I2C Registers */
-#define I2C_LS2X_PRER		0x0 /* Freq Division Register(16 bits) */
+#define I2C_LS2X_PRER_LO	0x0 /* Freq Division Low Byte Register */
+#define I2C_LS2X_PRER_HI	0x1 /* Freq Division High Byte Register */
 #define I2C_LS2X_CTR		0x2 /* Control Register */
 #define I2C_LS2X_TXR		0x3 /* Transport Data Register */
 #define I2C_LS2X_RXR		0x3 /* Receive Data Register */
@@ -93,6 +95,7 @@ static irqreturn_t ls2x_i2c_isr(int this_irq, void *dev_id)
  */
 static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
 {
+	u16 val;
 	struct i2c_timings *t = &priv->i2c_t;
 	struct device *dev = priv->adapter.dev.parent;
 	u32 acpi_speed = i2c_acpi_find_bus_speed(dev);
@@ -105,8 +108,9 @@ static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
 		t->bus_freq_hz = LS2X_I2C_FREQ_STD;
 
 	/* Calculate and set i2c frequency. */
-	writew(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1,
-	       priv->base + I2C_LS2X_PRER);
+	val = LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1;
+	writeb(FIELD_GET(GENMASK(7, 0), val), priv->base + I2C_LS2X_PRER_LO);
+	writeb(FIELD_GET(GENMASK(15, 8), val), priv->base + I2C_LS2X_PRER_HI);
 }
 
 static void ls2x_i2c_init(struct ls2x_i2c_priv *priv)

base-commit: 7e45b505e699f4c80aa8bf79b4ea2a5f5a66bb51
-- 
2.47.1

Re: [PATCH] i2c: ls2x: Fix frequency division register access

[Andy Shevchenko]

On Tue, Feb 18, 2025 at 07:11:33PM +0800, Binbin Zhou wrote:
> According to the chip manual, the I2C register access type of
> Loongson-2K2000/LS7A is "B", so we can only access registers in byte
> form (readb/writeb).
> 
> Although Loongson-2K0500/Loongson-2K1000 do not have similar
> constraints, register accesses in byte form also behave correctly.
> 
> Also, in hardware, the frequency division registers are defined as two
> separate registers (high 8-bit and low 8-bit), so we just access them
> directly as bytes.

...

>  	/* Calculate and set i2c frequency. */
> -	writew(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1,
> -	       priv->base + I2C_LS2X_PRER);
> +	val = LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1;
> +	writeb(FIELD_GET(GENMASK(7, 0), val), priv->base + I2C_LS2X_PRER_LO);
> +	writeb(FIELD_GET(GENMASK(15, 8), val), priv->base + I2C_LS2X_PRER_HI);

Now this needs a comment to prevent from appearing a patch that basically
changes that back to 16-bit write.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: ls2x: Fix frequency division register access

[Binbin Zhou]

Hi Andy:

Thanks for your reply.

On Tue, Feb 18, 2025 at 7:34 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Tue, Feb 18, 2025 at 07:11:33PM +0800, Binbin Zhou wrote:
> > According to the chip manual, the I2C register access type of
> > Loongson-2K2000/LS7A is "B", so we can only access registers in byte
> > form (readb/writeb).
> >
> > Although Loongson-2K0500/Loongson-2K1000 do not have similar
> > constraints, register accesses in byte form also behave correctly.
> >
> > Also, in hardware, the frequency division registers are defined as two
> > separate registers (high 8-bit and low 8-bit), so we just access them
> > directly as bytes.
>
> ...
>
> >       /* Calculate and set i2c frequency. */
> > -     writew(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1,
> > -            priv->base + I2C_LS2X_PRER);
> > +     val = LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1;
> > +     writeb(FIELD_GET(GENMASK(7, 0), val), priv->base + I2C_LS2X_PRER_LO);
> > +     writeb(FIELD_GET(GENMASK(15, 8), val), priv->base + I2C_LS2X_PRER_HI);
>
> Now this needs a comment to prevent from appearing a patch that basically
> changes that back to 16-bit write.

I will add a commit here.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>

-- 
Thanks.
Binbin