| Message ID | 20231023081158.10654-1-Huangzheng.Lai@unisoc.com |
|---|---|
| Headers | show |
| Series | i2c: sprd: Modification of UNISOC Platform I2C Driver | expand |
On 10/23/2023 4:11 PM, Huangzheng Lai wrote: > When I2C-slaves supporting different frequencies use the same I2C > controller, the I2C controller usually only operates at lower frequencies. > In order to improve the performance of I2C-slaves transmission supporting > faster frequencies, we dynamically configure the I2C operating frequency > based on the value of the input parameter msg ->flag. I am not sure if this is suitable to expand the msg->flag. Andi, how do you think? Thanks. > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com> > --- > drivers/i2c/busses/i2c-sprd.c | 101 +++++++++++++++++++--------------- > 1 file changed, 57 insertions(+), 44 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c > index dec627ef408c..f1f7fad42ecd 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,49 @@ struct sprd_i2c { > int err; > }; > > +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq) > +{ > + u32 apb_clk = i2c_dev->src_clk; > + /* > + * From I2C databook, the prescale calculation formula: > + * prescale = freq_i2c / (4 * freq_scl) - 1; > + */ > + u32 i2c_dvd = apb_clk / (4 * freq) - 1; > + /* > + * From I2C databook, the high period of SCL clock is recommended as > + * 40% (2/5), and the low period of SCL clock is recommended as 60% > + * (3/5), then the formula should be: > + * high = (prescale * 2 * 2) / 5 > + * low = (prescale * 2 * 3) / 5 > + */ > + u32 high = ((i2c_dvd << 1) * 2) / 5; > + u32 low = ((i2c_dvd << 1) * 3) / 5; > + u32 div0 = I2C_ADDR_DVD0_CALC(high, low); > + u32 div1 = I2C_ADDR_DVD1_CALC(high, low); > + > + writel(div0, i2c_dev->base + ADDR_DVD0); > + writel(div1, i2c_dev->base + ADDR_DVD1); > + > + /* Start hold timing = hold time(us) * source clock */ > + switch (freq) { > + case I2C_MAX_STANDARD_MODE_FREQ: > + writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD); > + break; > + case I2C_MAX_FAST_MODE_FREQ: > + writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > + break; > + case I2C_MAX_FAST_MODE_PLUS_FREQ: > + writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > + break; > + case I2C_MAX_HIGH_SPEED_MODE_FREQ: > + writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > + break; > + default: > + dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq); > + break; > + } > +} > + > static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count) > { > writel(count, i2c_dev->base + I2C_COUNT); > @@ -269,6 +319,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. > @@ -331,49 +387,6 @@ static const struct i2c_algorithm sprd_i2c_algo = { > .functionality = sprd_i2c_func, > }; > > -static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq) > -{ > - u32 apb_clk = i2c_dev->src_clk; > - /* > - * From I2C databook, the prescale calculation formula: > - * prescale = freq_i2c / (4 * freq_scl) - 1; > - */ > - u32 i2c_dvd = apb_clk / (4 * freq) - 1; > - /* > - * From I2C databook, the high period of SCL clock is recommended as > - * 40% (2/5), and the low period of SCL clock is recommended as 60% > - * (3/5), then the formula should be: > - * high = (prescale * 2 * 2) / 5 > - * low = (prescale * 2 * 3) / 5 > - */ > - u32 high = ((i2c_dvd << 1) * 2) / 5; > - u32 low = ((i2c_dvd << 1) * 3) / 5; > - u32 div0 = I2C_ADDR_DVD0_CALC(high, low); > - u32 div1 = I2C_ADDR_DVD1_CALC(high, low); > - > - writel(div0, i2c_dev->base + ADDR_DVD0); > - writel(div1, i2c_dev->base + ADDR_DVD1); > - > - /* Start hold timing = hold time(us) * source clock */ > - switch (freq) { > - case I2C_MAX_STANDARD_MODE_FREQ: > - writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD); > - break; > - case I2C_MAX_FAST_MODE_FREQ: > - writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > - break; > - case I2C_MAX_FAST_MODE_PLUS_FREQ: > - writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > - break; > - case I2C_MAX_HIGH_SPEED_MODE_FREQ: > - writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > - break; > - default: > - dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq); > - break; > - } > -} > - > static void sprd_i2c_enable(struct sprd_i2c *i2c_dev) > { > u32 tmp = I2C_DVD_OPT;
On 10/23/2023 4:11 PM, Huangzheng Lai wrote: > The new I2C IP version on the UNISOC platform has added I2C_NACK_EN and > I2C_TRANS_EN control bits. To ensure that the I2C controller can initiate > transmission smoothly, these two bits need to be configured. What is the side impact for old hardwares that does not support these 2 bits? > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com> > --- > drivers/i2c/busses/i2c-sprd.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c > index dbdac89ad482..431c0db84d22 100644 > --- a/drivers/i2c/busses/i2c-sprd.c > +++ b/drivers/i2c/busses/i2c-sprd.c > @@ -33,6 +33,8 @@ > #define ADDR_RST 0x2c > > /* I2C_CTL */ > +#define I2C_NACK_EN BIT(22) > +#define I2C_TRANS_EN BIT(21) > #define STP_EN BIT(20) > #define FIFO_AF_LVL_MASK GENMASK(19, 16) > #define FIFO_AF_LVL 16 > @@ -366,7 +368,7 @@ static void sprd_i2c_enable(struct sprd_i2c *i2c_dev) > sprd_i2c_clear_irq(i2c_dev); > > tmp = readl(i2c_dev->base + I2C_CTL); > - writel(tmp | I2C_EN | I2C_INT_EN, i2c_dev->base + I2C_CTL); > + writel(tmp | I2C_EN | I2C_INT_EN | I2C_NACK_EN | I2C_TRANS_EN, i2c_dev->base + I2C_CTL); > } > > static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
Hi Huangzheng, On Mon, Oct 23, 2023 at 04:11:52PM +0800, Huangzheng Lai wrote: > Add support for 1Mhz and 3.4Mhz frequency configuration. > > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com> > Acked-by: Andi Shyti <andi.shyti@kernel.org> you can make this: Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > --- > drivers/i2c/busses/i2c-sprd.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c > index ffc54fbf814d..b44916c6741d 100644 > --- a/drivers/i2c/busses/i2c-sprd.c > +++ b/drivers/i2c/busses/i2c-sprd.c > @@ -343,10 +343,23 @@ static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq) > writel(div1, i2c_dev->base + ADDR_DVD1); > > /* Start hold timing = hold time(us) * source clock */ > - if (freq == I2C_MAX_FAST_MODE_FREQ) > - writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > - else if (freq == I2C_MAX_STANDARD_MODE_FREQ) > + switch (freq) { > + case I2C_MAX_STANDARD_MODE_FREQ: > writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD); > + break; > + case I2C_MAX_FAST_MODE_FREQ: > + writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > + break; > + case I2C_MAX_FAST_MODE_PLUS_FREQ: > + writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > + break; > + case I2C_MAX_HIGH_SPEED_MODE_FREQ: > + writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > + break; so these were defined but not used. > + default: > + dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq); > + break; > + } > } > > static void sprd_i2c_enable(struct sprd_i2c *i2c_dev) > @@ -519,9 +532,11 @@ static int sprd_i2c_probe(struct platform_device *pdev) > if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop)) > i2c_dev->bus_freq = prop; > > - /* We only support 100k and 400k now, otherwise will return error. */ > + /* We only support 100k\400k\1m\3.4m now, otherwise will return error. */ > if (i2c_dev->bus_freq != I2C_MAX_STANDARD_MODE_FREQ && > - i2c_dev->bus_freq != I2C_MAX_FAST_MODE_FREQ) > + i2c_dev->bus_freq != I2C_MAX_FAST_MODE_FREQ && > + i2c_dev->bus_freq != I2C_MAX_FAST_MODE_PLUS_FREQ && > + i2c_dev->bus_freq != I2C_MAX_HIGH_SPEED_MODE_FREQ) > return -EINVAL; This is a bit of a weird management of the bus_freq because it goes like this: i2c_dev->bus_freq = I2C_MAX_STANDARD_MODE_FREQ; ... if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop)) i2c_dev->bus_freq = prop; if (...) return -EINVAL; A follow-up cleanup can be removing the first assignement and instead of returning -EINVAL, we can keep going: if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop)) i2c_dev->bus_freq = prop; if (...) { dev_warn("..."); i2c_dev->bus_freq = I2C_MAX_STANDARD_MODE_FREQ; } But this is topic for a different patch. Andi > > ret = sprd_i2c_clk_init(i2c_dev); > -- > 2.17.1 >
Hi Huangzheng, On Mon, Oct 23, 2023 at 04:11:55PM +0800, Huangzheng Lai wrote: > When I2C-slaves supporting different frequencies use the same I2C > controller, the I2C controller usually only operates at lower frequencies. > In order to improve the performance of I2C-slaves transmission supporting > faster frequencies, we dynamically configure the I2C 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 | 101 +++++++++++++++++++--------------- > 1 file changed, 57 insertions(+), 44 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c > index dec627ef408c..f1f7fad42ecd 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 Why are you redefining these values? You could use the defines you already have or, if you really want a different name you could do: #define I2C_FREQ_3_4M I2C_MAX_HIGH_SPEED_MODE_FREQ Rest looks good. Andi
On Mon, Oct 23, 2023 at 04:11:56PM +0800, Huangzheng Lai wrote: > When a timeout exception occurs in the I2C driver, the I2C controller > will be reset, and after resetting, control bits such as I2C_EN and > I2C_INT_EN will be reset to 0, and the I2C master cannot initiate > Transmission unless sprd_i2c_enable() is executed. To address this issue, > this patch places sprd_i2c_enable() before each transmission initiation > to ensure that the necessary control bits of the I2C controller are > configured. where is the timeout occurring? Is it a sporadic failure? > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com> Would be nice if any from Orson, Boulin or Chunyan could take a look here. Andi
Hi Huangzheng, could you please use the [PATCH RESEND...] prefix when sending the patch as it is? Thanks, Andi On Mon, Oct 23, 2023 at 04:11:51PM +0800, Huangzheng Lai wrote: > Recently, some bugs have been discovered during use, patch3 and > patch5-6 are bug fixes. Also, this patchset add new features: > patch1 allows I2C to use more frequencies for communication, > patch2 allows I2C to use 'reset framework' for reset, and patch4 allows > I2C controller to dynamically switch frequencies during use. > > change in V2 > -Using 'I2C' instead of 'IIC' in the patch set. > -Using imperative form in patch subject. > -Use 'switch case' instead of 'else if' in PATCH 1/7. > -Modify if (i2c_dev->rst != NULL) to if (i2c_dev->rst) in PATCH 2/7. > -Modify some dev_err() to dev_warn() or dev_dbg(). > -Clear i2c_dev->ack_flag in sprd_i2c_clear_ack() in PATCH 3/7. > -Modify the indentation format of the code in PATCH 4/7. > -Move sprd_i2c_enable() above its caller in PATCH 5/7. > -Remove 'Set I2C_RX_ACK when clear irq' commit. > -Add Fixes tags. > > Huangzheng Lai (7): > i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies > i2c: sprd: Add I2C driver to use 'reset framework' function > i2c: sprd: Use global variables to record I2C ack/nack status instead > of local variables > i2c: sprd: Add I2C controller driver to support dynamic switching of > 400K/1M/3.4M frequency > i2c: sprd: Configure the enable bit of the I2C controller before each > transmission initiation > i2c: sprd: Increase the waiting time for I2C transmission to avoid > system crash issues > i2c: sprd: Add I2C_NACK_EN and I2C_TRANS_EN control bits > > drivers/i2c/busses/i2c-sprd.c | 166 ++++++++++++++++++++++------------ > 1 file changed, 106 insertions(+), 60 deletions(-) > > -- > 2.17.1 >
Recently, some bugs have been discovered during use, patch3 and patch5-6 are bug fixes. Also, this patchset add new features: patch1 allows I2C to use more frequencies for communication, patch2 allows I2C to use 'reset framework' for reset, and patch4 allows I2C controller to dynamically switch frequencies during use. change in V2 -Using 'I2C' instead of 'IIC' in the patch set. -Using imperative form in patch subject. -Use 'switch case' instead of 'else if' in PATCH 1/7. -Modify if (i2c_dev->rst != NULL) to if (i2c_dev->rst) in PATCH 2/7. -Modify some dev_err() to dev_warn() or dev_dbg(). -Clear i2c_dev->ack_flag in sprd_i2c_clear_ack() in PATCH 3/7. -Modify the indentation format of the code in PATCH 4/7. -Move sprd_i2c_enable() above its caller in PATCH 5/7. -Remove 'Set I2C_RX_ACK when clear irq' commit. -Add Fixes tags. Huangzheng Lai (7): i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies i2c: sprd: Add I2C driver to use 'reset framework' function i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables i2c: sprd: Add I2C controller driver to support dynamic switching of 400K/1M/3.4M frequency i2c: sprd: Configure the enable bit of the I2C controller before each transmission initiation i2c: sprd: Increase the waiting time for I2C transmission to avoid system crash issues i2c: sprd: Add I2C_NACK_EN and I2C_TRANS_EN control bits drivers/i2c/busses/i2c-sprd.c | 166 ++++++++++++++++++++++------------ 1 file changed, 106 insertions(+), 60 deletions(-)