i2c: sprd: Modification of UNIOC Platform IIC Driver

[PATCH 4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency

Posted by Huangzheng Lai 2 years, 4 months ago

When IIC-slaves supporting different frequencies use the same IIC
controller, the IIC controller usually only operates at lower frequencies.
In order to improve the performance of IIC-slaves transmission supporting
faster frequencies, we dynamically configure the IIC operating frequency
based on the value of the input parameter msg ->flag.

Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 549b60dd3273..02c11a9ff2da 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -75,7 +75,14 @@
 #define SPRD_I2C_PM_TIMEOUT	1000
 /* timeout (ms) for transfer message */
 #define I2C_XFER_TIMEOUT	1000
-
+/* dynamic modify clk_freq flag  */
+#define	I2C_3M4_FLAG		0x0100
+#define	I2C_1M_FLAG		0x0080
+#define	I2C_400K_FLAG		0x0040
+
+#define	I2C_FREQ_400K		400000
+#define	I2C_FREQ_1M		1000000
+#define	I2C_FREQ_3_4M		3400000
 /* SPRD i2c data structure */
 struct sprd_i2c {
 	struct i2c_adapter adap;
@@ -94,6 +101,8 @@ struct sprd_i2c {
 	int err;
 };
 
+static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq);
+
 static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
 {
 	writel(count, i2c_dev->base + I2C_COUNT);
@@ -269,6 +278,12 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
 		sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
 	}
 
+	if (msg->flags & I2C_400K_FLAG)
+		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_400K);
+	else if (msg->flags & I2C_1M_FLAG)
+		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_1M);
+	else if (msg->flags & I2C_3M4_FLAG)
+		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_3_4M);
 	/*
 	 * We should enable rx fifo full interrupt to get data when receiving
 	 * full data.
-- 
2.17.1

Re: [PATCH 4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency

Posted by Chunyan Zhang 2 years, 3 months ago

On Thu, 17 Aug 2023 at 17:46, Huangzheng Lai <Huangzheng.Lai@unisoc.com> wrote:
>
> When IIC-slaves supporting different frequencies use the same IIC

%s/I2C/IIC

> controller, the IIC controller usually only operates at lower frequencies.
> In order to improve the performance of IIC-slaves transmission supporting
> faster frequencies, we dynamically configure the IIC operating frequency
> based on the value of the input parameter msg ->flag.
>
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 549b60dd3273..02c11a9ff2da 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -75,7 +75,14 @@
>  #define SPRD_I2C_PM_TIMEOUT    1000
>  /* timeout (ms) for transfer message */
>  #define I2C_XFER_TIMEOUT       1000
> -
> +/* dynamic modify clk_freq flag  */
> +#define        I2C_3M4_FLAG            0x0100

#define <space> I2C_3M4_FLAG <tab> 0x0100

> +#define        I2C_1M_FLAG             0x0080
> +#define        I2C_400K_FLAG           0x0040
> +
> +#define        I2C_FREQ_400K           400000
> +#define        I2C_FREQ_1M             1000000
> +#define        I2C_FREQ_3_4M           3400000

ditto

>  /* SPRD i2c data structure */
>  struct sprd_i2c {
>         struct i2c_adapter adap;
> @@ -94,6 +101,8 @@ struct sprd_i2c {
>         int err;
>  };
>
> +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq);

I would suggest moving this whole function above its caller.

> +
>  static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
>  {
>         writel(count, i2c_dev->base + I2C_COUNT);
> @@ -269,6 +278,12 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
>                 sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
>         }
>
> +       if (msg->flags & I2C_400K_FLAG)

Where does this flag set? I'm not sure we can use msg->flags like
this. I don't know i2c very well.

Thanks,
Chunyan

> +               sprd_i2c_set_clk(i2c_dev, I2C_FREQ_400K);
> +       else if (msg->flags & I2C_1M_FLAG)
> +               sprd_i2c_set_clk(i2c_dev, I2C_FREQ_1M);
> +       else if (msg->flags & I2C_3M4_FLAG)
> +               sprd_i2c_set_clk(i2c_dev, I2C_FREQ_3_4M);
>         /*
>          * We should enable rx fifo full interrupt to get data when receiving
>          * full data.
> --
> 2.17.1
>

Re: [PATCH 4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency

Posted by Andi Shyti 2 years, 3 months ago

Hi Chunyan,

[...]

> > When IIC-slaves supporting different frequencies use the same IIC
> 
> %s/I2C/IIC

[...]

> >  #define SPRD_I2C_PM_TIMEOUT    1000
> >  /* timeout (ms) for transfer message */
> >  #define I2C_XFER_TIMEOUT       1000
> > -
> > +/* dynamic modify clk_freq flag  */
> > +#define        I2C_3M4_FLAG            0x0100
> 
> #define <space> I2C_3M4_FLAG <tab> 0x0100
> 
> > +#define        I2C_1M_FLAG             0x0080
> > +#define        I2C_400K_FLAG           0x0040
> > +
> > +#define        I2C_FREQ_400K           400000
> > +#define        I2C_FREQ_1M             1000000
> > +#define        I2C_FREQ_3_4M           3400000
> 
> ditto

why should he use IIC instead of I2C. The file's defines start
with I2C, for consistency he should use the same prefix.

Andi

Re: [PATCH 4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency

Posted by Chunyan Zhang 2 years, 3 months ago

Hi Andi,

On Sun, 3 Sept 2023 at 05:08, Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Chunyan,
>
> [...]
>
> > > When IIC-slaves supporting different frequencies use the same IIC
> >
> > %s/I2C/IIC

I just noticed that this was the reverse, I meant "%s/IIC/I2C" :)
That's saying we should use I2C in the whole driver.

> [...]
>
> > >  #define SPRD_I2C_PM_TIMEOUT    1000
> > >  /* timeout (ms) for transfer message */
> > >  #define I2C_XFER_TIMEOUT       1000
> > > -
> > > +/* dynamic modify clk_freq flag  */
> > > +#define        I2C_3M4_FLAG            0x0100
> >
> > #define <space> I2C_3M4_FLAG <tab> 0x0100
> >
> > > +#define        I2C_1M_FLAG             0x0080
> > > +#define        I2C_400K_FLAG           0x0040
> > > +
> > > +#define        I2C_FREQ_400K           400000
> > > +#define        I2C_FREQ_1M             1000000
> > > +#define        I2C_FREQ_3_4M           3400000
> >
> > ditto

I meant "#define <space> I2C_FREQ_3_4M <tab> 3400000"

>
> why should he use IIC instead of I2C. The file's defines start
> with I2C, for consistency he should use the same prefix.
>

Yes, I agree.

Thanks,
Chunyan

Re: [PATCH 4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency

Posted by Andi Shyti 2 years, 3 months ago

Hi Chunyan,

> > > >  #define SPRD_I2C_PM_TIMEOUT    1000
> > > >  /* timeout (ms) for transfer message */
> > > >  #define I2C_XFER_TIMEOUT       1000
> > > > -
> > > > +/* dynamic modify clk_freq flag  */
> > > > +#define        I2C_3M4_FLAG            0x0100
> > >
> > > #define <space> I2C_3M4_FLAG <tab> 0x0100
> > >
> > > > +#define        I2C_1M_FLAG             0x0080
> > > > +#define        I2C_400K_FLAG           0x0040
> > > > +
> > > > +#define        I2C_FREQ_400K           400000
> > > > +#define        I2C_FREQ_1M             1000000
> > > > +#define        I2C_FREQ_3_4M           3400000
> > >
> > > ditto
> 
> I meant "#define <space> I2C_FREQ_3_4M <tab> 3400000"

right! Agree with your comment!

> >
> > why should he use IIC instead of I2C. The file's defines start
> > with I2C, for consistency he should use the same prefix.
> >
> 
> Yes, I agree.

Thanks,
Andi

[PATCH 1/8] i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies
[PATCH 2/8] i2c: sprd: Add I2C driver to use 'reset framework' function
[PATCH 3/8] i2c: sprd: Use global variables to record IIC ack/nack status instead of local variables
[PATCH 4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency
[PATCH 5/8] i2c: sprd: Configure the enable bit of the IIC controller before each transmission initiation
[PATCH 6/8] i2c: sprd: Add additional IIC control bit configuration to adapt to the new IP version of the UNISOC platform
[PATCH 7/8] i2c: sprd: Set I2C_RX_ACK when clear irq
[PATCH 8/8] i2c: sprd: Increase the waiting time for IIC transmission to avoid system crash issues