[PATCH v2 0/7] i2c: npcm: Bug fixes read/write operation, checkpatch

[PATCH v2 0/7] i2c: npcm: Bug fixes read/write operation, checkpatch

[Tyrone Ting]

This patchset includes the following fixes:

- Restore the npcm_i2caddr array length to fix the smatch warning.
- Enable the target functionality in the interrupt handling routine 
  when the i2c transfer is about to finish.
- Correct the read/write operation procedure.
- Introduce a software flag to handle the bus error (BER) condition
  which is not caused by the i2c transfer.
- Modify timeout calculation.
- Assign the client address earlier logically.
- Use an i2c frequency table for the frequency parameters assignment.
- Coding style fix.

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Addressed comments from:
- kernel test robot : https://lore.kernel.org/oe-kbuild-all/
  202408080319.de2B6PgU-lkp@intel.com/
- Dan Carpenter : https://lore.kernel.org/all/202408130818
  .FgDP5uNm-lkp@intel.com/
- Andrew Jeffery : https://lore.kernel.org/lkml/
  20240807100244.16872-7-kfting@nuvoton.com/T/
  #m3ed3351bf59675bfe0de89c75aae1fb26cad5567

Changes since version 1:
- Restore the npcm_i2caddr array length to fix the smatch warning.
- Remove unused variables.
- Handle the condition where scl_table_cnt reaches to the maximum value.
- Fix the checkpatch warning.

Charles Boyer (1):
  i2c: npcm: Enable slave in eob interrupt

Tyrone Ting (6):
  i2c: npcm: restore slave addresses array length
  i2c: npcm: correct the read/write operation procedure
  i2c: npcm: use a software flag to indicate a BER condition
  i2c: npcm: Modify timeout evaluation mechanism
  i2c: npcm: Modify the client address assignment
  i2c: npcm: use i2c frequency table

 drivers/i2c/busses/i2c-npcm7xx.c | 276 +++++++++++++++++++------------
 1 file changed, 172 insertions(+), 104 deletions(-)


base-commit: 5be63fc19fcaa4c236b307420483578a56986a37
-- 
2.34.1

[PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Tyrone Ting]

The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
The original design supports 10 slave addresses although only 2
addresses are required for current implementation.

Restore the npcm_i2caddr array length to fix the smatch warning.

Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2fe68615942e..bbcb4d6668ce 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -136,11 +136,13 @@ enum i2c_addr {
  * Since the addr regs are sprinkled all over the address space,
  * use this array to get the address or each register.
  */
-#define I2C_NUM_OWN_ADDR 2
+#define I2C_NUM_OWN_ADDR 10
 #define I2C_NUM_OWN_ADDR_SUPPORTED 2
 
 static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
-	NPCM_I2CADDR1, NPCM_I2CADDR2,
+	NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
+	NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,
+	NPCM_I2CADDR9, NPCM_I2CADDR10,
 };
 #endif
 
-- 
2.34.1

Re: [PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Andi Shyti]

Hi Tyrone,

On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote:
> The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
> The original design supports 10 slave addresses although only 2

please remember that the "slave" term has been replaced by the
"target" term. I will change it when applying the patch.

> addresses are required for current implementation.
> 
> Restore the npcm_i2caddr array length to fix the smatch warning.
> 
> Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")

I don't think the Fixes tag is necessary here. This change is
primarily addressing a static analyzer warning. While some cases
come close to a buffer overflow, it couldn’t have occurred in
practice since we don't actually have the devices listed in
npcm_i2caddr[].

> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index 2fe68615942e..bbcb4d6668ce 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -136,11 +136,13 @@ enum i2c_addr {
>   * Since the addr regs are sprinkled all over the address space,
>   * use this array to get the address or each register.
>   */
> -#define I2C_NUM_OWN_ADDR 2
> +#define I2C_NUM_OWN_ADDR 10
>  #define I2C_NUM_OWN_ADDR_SUPPORTED 2
>  
>  static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
> -	NPCM_I2CADDR1, NPCM_I2CADDR2,
> +	NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
> +	NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,
> +	NPCM_I2CADDR9, NPCM_I2CADDR10,

Looks a bit hacky, but serves the purpose.

The core issue in "npcm_i2c_slave_enable()" is the lack of an
upper boundary check, which could potentially lead to a buffer
overflow. In practice, we rely on the assumption that these
addresses don’t exist in the real world.

An easier fix could have been:

@@ -629,7 +629,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
        if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10)
                dev_err(bus->dev, "try to enable more than 2 SA not supported\n");

-       if (addr_type >= I2C_ARP_ADDR)
+       if (addr_type > I2C_SLAVE_ADDR2)
                return -EFAULT;

        /* Set and enable the address */

But yours is a bit more robust, so that I'm going to take this
patch.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

>  };
>  #endif
>  
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Tyrone Ting]

Hi Andi:

Thank you for your review.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:24寫道：
>
> Hi Tyrone,
>
> On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote:
> > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
> > The original design supports 10 slave addresses although only 2
>
> please remember that the "slave" term has been replaced by the
> "target" term. I will change it when applying the patch.
>

Thank you for your reminder. I'll change the term ever since.

> > addresses are required for current implementation.
> >
> > Restore the npcm_i2caddr array length to fix the smatch warning.
> >
> > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")
>
> I don't think the Fixes tag is necessary here. This change is
> primarily addressing a static analyzer warning. While some cases
> come close to a buffer overflow, it couldn’t have occurred in
> practice since we don't actually have the devices listed in
> npcm_i2caddr[].
>

Understood. I'll remove the Fixes tag.

> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > ---
> >  drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index 2fe68615942e..bbcb4d6668ce 100644
> > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > @@ -136,11 +136,13 @@ enum i2c_addr {
> >   * Since the addr regs are sprinkled all over the address space,
> >   * use this array to get the address or each register.
> >   */
> > -#define I2C_NUM_OWN_ADDR 2
> > +#define I2C_NUM_OWN_ADDR 10
> >  #define I2C_NUM_OWN_ADDR_SUPPORTED 2
> >
> >  static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
> > -     NPCM_I2CADDR1, NPCM_I2CADDR2,
> > +     NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
> > +     NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,
> > +     NPCM_I2CADDR9, NPCM_I2CADDR10,
>
> Looks a bit hacky, but serves the purpose.
>
> The core issue in "npcm_i2c_slave_enable()" is the lack of an
> upper boundary check, which could potentially lead to a buffer
> overflow. In practice, we rely on the assumption that these
> addresses don’t exist in the real world.
>
> An easier fix could have been:
>
> @@ -629,7 +629,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
>         if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10)
>                 dev_err(bus->dev, "try to enable more than 2 SA not supported\n");
>
> -       if (addr_type >= I2C_ARP_ADDR)
> +       if (addr_type > I2C_SLAVE_ADDR2)
>                 return -EFAULT;
>
>         /* Set and enable the address */
>
> But yours is a bit more robust, so that I'm going to take this
> patch.
>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
>
> Thanks,
> Andi
>
> >  };
> >  #endif
> >
> > --
> > 2.34.1
> >

Thank you.

Regards,
Tyrone

Re: [PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Andi Shyti]

Hi Tyrone,

On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote:
> The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
> The original design supports 10 slave addresses although only 2
> addresses are required for current implementation.
> 
> Restore the npcm_i2caddr array length to fix the smatch warning.
> 
> Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>

your email used in From: is different that your e-mail used the
SoB. Is this done in purpose? If so I will keep it as it is, no
problem for me, otherwise I can fix it while applying it.

Andi

Re: [PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Tyrone Ting]

Hi Andy:

Thank you for your comments.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:36寫道：
>
> Hi Tyrone,
>
> On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote:
> > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
> > The original design supports 10 slave addresses although only 2
> > addresses are required for current implementation.
> >
> > Restore the npcm_i2caddr array length to fix the smatch warning.
> >
> > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>
> your email used in From: is different that your e-mail used the
> SoB. Is this done in purpose? If so I will keep it as it is, no
> problem for me, otherwise I can fix it while applying it.
>

I'll add the option "--from kfting@nuvoton.com", same as the patch
author's email while using the tool "git send-email"
in the next patch set.

> Andi

Have a nice day.

Thank you.

Regards,
Tyrone

Re: [PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Andi Shyti]

Hi Tyrone,

On Fri, Sep 06, 2024 at 10:28:30AM GMT, Tyrone Ting wrote:
> Hi Andy:
> 
> Thank you for your comments.
> 
> Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:36寫道：
> >
> > Hi Tyrone,
> >
> > On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote:
> > > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
> > > The original design supports 10 slave addresses although only 2
> > > addresses are required for current implementation.
> > >
> > > Restore the npcm_i2caddr array length to fix the smatch warning.
> > >
> > > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
> > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> >
> > your email used in From: is different that your e-mail used the
> > SoB. Is this done in purpose? If so I will keep it as it is, no
> > problem for me, otherwise I can fix it while applying it.
> >
> 
> I'll add the option "--from kfting@nuvoton.com", same as the patch
> author's email while using the tool "git send-email"
> in the next patch set.

don't worry, I will apply this patch number '1' because it's
independent from the rest of the series. I will do all the
changes you agreed with me.

When you resend this series you don't need to include this patch,
just rebase on top of i2c/i2c-hostp[*].

Thanks,
Andi

[*] git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

> > Andi
> 
> Have a nice day.
> 
> Thank you.
> 
> Regards,
> Tyrone

Re: [PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 11:36:45PM +0200, Andi Shyti wrote:
> On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote:
> > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
> > The original design supports 10 slave addresses although only 2
> > addresses are required for current implementation.
> > 
> > Restore the npcm_i2caddr array length to fix the smatch warning.
> > 
> > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> 
> your email used in From: is different that your e-mail used the
> SoB. Is this done in purpose? If so I will keep it as it is, no
> problem for me, otherwise I can fix it while applying it.

IIRC Linux Next has the respective check and it will become your problem :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Andi Shyti]

Hi Tyrone,

On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote:
> The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
> The original design supports 10 slave addresses although only 2
> addresses are required for current implementation.
> 
> Restore the npcm_i2caddr array length to fix the smatch warning.
> 
> Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>

with the changes we agreed, I merged just this first patch in
i2c/i2c-host.

Thanks,
Andi

Re: [PATCH v2 1/7] i2c: npcm: restore slave addresses array length

[Tyrone Ting]

Hi Andi:

Thank you for your prompt response and help.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 下午3:05寫道：
>
> Hi Tyrone,
>
> On Fri, Aug 30, 2024 at 11:46:34AM GMT, Tyrone Ting wrote:
> > The smatch check warning is "buffer overflow 'npcm_i2caddr' 2 <= 9".
> > The original design supports 10 slave addresses although only 2
> > addresses are required for current implementation.
> >
> > Restore the npcm_i2caddr array length to fix the smatch warning.
> >
> > Fixes: 47d506d1a28f ("i2c: npcm: Remove own slave addresses 2:10")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202408130818.FgDP5uNm-lkp@intel.com/
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>
> with the changes we agreed, I merged just this first patch in
> i2c/i2c-host.
>
> Thanks,
> Andi

Have a nice day.

Regards,
Tyrone

[PATCH v2 2/7] i2c: npcm: correct the read/write operation procedure

[Tyrone Ting]

Originally the driver uses the XMIT bit in SMBnST register to decide
the upcoming i2c transaction. If XMIT bit is 1, then it will be an i2c
write operation. If it's 0, then a read operation will be executed.

After checking the datasheet, the XMIT bit is valid when the i2c module
is acting in a slave role. Use the software status to control the i2c
transaction flow instead when the i2c module is acting in a master role.

Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index bbcb4d6668ce..2b76dbfba438 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -1628,13 +1628,10 @@ static void npcm_i2c_irq_handle_sda(struct npcm_i2c *bus, u8 i2cst)
 			npcm_i2c_wr_byte(bus, bus->dest_addr | BIT(0));
 	/* SDA interrupt, after start\restart */
 	} else {
-		if (NPCM_I2CST_XMIT & i2cst) {
-			bus->operation = I2C_WRITE_OPER;
+		if (bus->operation == I2C_WRITE_OPER)
 			npcm_i2c_irq_master_handler_write(bus);
-		} else {
-			bus->operation = I2C_READ_OPER;
+		else if (bus->operation == I2C_READ_OPER)
 			npcm_i2c_irq_master_handler_read(bus);
-		}
 	}
 }
 
-- 
2.34.1

Re: [PATCH v2 2/7] i2c: npcm: correct the read/write operation procedure

[Andi Shyti]

Hi Tyronne,

On Fri, Aug 30, 2024 at 11:46:35AM GMT, Tyrone Ting wrote:
> Originally the driver uses the XMIT bit in SMBnST register to decide
> the upcoming i2c transaction. If XMIT bit is 1, then it will be an i2c
> write operation. If it's 0, then a read operation will be executed.
> 
> After checking the datasheet, the XMIT bit is valid when the i2c module
> is acting in a slave role. Use the software status to control the i2c
> transaction flow instead when the i2c module is acting in a master role.
> 
> Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")

Fixes needs to be used if you are fixing a bug (crash,
device malfunction, etc.). If you want to use it, please describe
the bug you are fixing. Otherwise, please, remove it.

> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index bbcb4d6668ce..2b76dbfba438 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -1628,13 +1628,10 @@ static void npcm_i2c_irq_handle_sda(struct npcm_i2c *bus, u8 i2cst)
>  			npcm_i2c_wr_byte(bus, bus->dest_addr | BIT(0));
>  	/* SDA interrupt, after start\restart */
>  	} else {
> -		if (NPCM_I2CST_XMIT & i2cst) {
> -			bus->operation = I2C_WRITE_OPER;
> +		if (bus->operation == I2C_WRITE_OPER)
>  			npcm_i2c_irq_master_handler_write(bus);
> -		} else {
> -			bus->operation = I2C_READ_OPER;
> +		else if (bus->operation == I2C_READ_OPER)
>  			npcm_i2c_irq_master_handler_read(bus);

mmmhhh... you are changing the logic here and you are not
describing the logic change in the commit log.

Without looking at the details, if this is a state machine you
are breaking it.

Can anyone from the ARM/NUVOTON NPCM supporters and reviewers
take a look at this patch?

Thanks,
Andi

> -		}
>  	}
>  }
>  
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/7] i2c: npcm: correct the read/write operation procedure

[Tali Perry]

Hi Andi,

On Fri, Sep 6, 2024 at 12:29 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Tyronne,
>
> On Fri, Aug 30, 2024 at 11:46:35AM GMT, Tyrone Ting wrote:
> > Originally the driver uses the XMIT bit in SMBnST register to decide
> > the upcoming i2c transaction. If XMIT bit is 1, then it will be an i2c
> > write operation. If it's 0, then a read operation will be executed.
> >
> > After checking the datasheet, the XMIT bit is valid when the i2c module
> > is acting in a slave role. Use the software status to control the i2c
> > transaction flow instead when the i2c module is acting in a master role.
> >
> > Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
>
> Fixes needs to be used if you are fixing a bug (crash,
> device malfunction, etc.). If you want to use it, please describe
> the bug you are fixing. Otherwise, please, remove it.
>
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > ---
> >  drivers/i2c/busses/i2c-npcm7xx.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index bbcb4d6668ce..2b76dbfba438 100644
> > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > @@ -1628,13 +1628,10 @@ static void npcm_i2c_irq_handle_sda(struct npcm_i2c *bus, u8 i2cst)
> >                       npcm_i2c_wr_byte(bus, bus->dest_addr | BIT(0));
> >       /* SDA interrupt, after start\restart */
> >       } else {
> > -             if (NPCM_I2CST_XMIT & i2cst) {
> > -                     bus->operation = I2C_WRITE_OPER;
> > +             if (bus->operation == I2C_WRITE_OPER)
> >                       npcm_i2c_irq_master_handler_write(bus);
> > -             } else {
> > -                     bus->operation = I2C_READ_OPER;
> > +             else if (bus->operation == I2C_READ_OPER)
> >                       npcm_i2c_irq_master_handler_read(bus);
>
> mmmhhh... you are changing the logic here and you are not
> describing the logic change in the commit log.
>
> Without looking at the details, if this is a state machine you
> are breaking it.
>
> Can anyone from the ARM/NUVOTON NPCM supporters and reviewers
> take a look at this patch?
>

Indeed, the driver can use both the register bits or the state machine
to determine the current state of the bus.
In slave mode the XMIT bit can simply be used directly to set the state.
XMIT bit can be used as indication to the current state of the state
machine during slave operation.
(meaning XMIT = 1 during writing and XMIT = 0 during reading).
In master operation XMIT is valid only if there are no bus errors.
For example: in a multi master where the same module is switching from
master to slave at runtime, and there are collisions,
the XMIT bit cannot be trusted.
However the maser already "knows" what the bus state is, so this bit
is not needed and the driver can just track
what it is currently doing.




> Thanks,
> Andi
>
> > -             }
> >       }
> >  }
> >
> > --
> > 2.34.1
> >

Re: [PATCH v2 2/7] i2c: npcm: correct the read/write operation procedure

[Tyrone Ting]

Hi Andi:

Thank you for your review.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:29寫道：
>
> Hi Tyronne,
>
> On Fri, Aug 30, 2024 at 11:46:35AM GMT, Tyrone Ting wrote:
> > Originally the driver uses the XMIT bit in SMBnST register to decide
> > the upcoming i2c transaction. If XMIT bit is 1, then it will be an i2c
> > write operation. If it's 0, then a read operation will be executed.
> >
> > After checking the datasheet, the XMIT bit is valid when the i2c module
> > is acting in a slave role. Use the software status to control the i2c
> > transaction flow instead when the i2c module is acting in a master role.
> >
> > Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
>
> Fixes needs to be used if you are fixing a bug (crash,
> device malfunction, etc.). If you want to use it, please describe
> the bug you are fixing. Otherwise, please, remove it.
>
Understood. I'll remove the Fixes tag in the next patch set.

> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > ---
> >  drivers/i2c/busses/i2c-npcm7xx.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index bbcb4d6668ce..2b76dbfba438 100644
> > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > @@ -1628,13 +1628,10 @@ static void npcm_i2c_irq_handle_sda(struct npcm_i2c *bus, u8 i2cst)
> >                       npcm_i2c_wr_byte(bus, bus->dest_addr | BIT(0));
> >       /* SDA interrupt, after start\restart */
> >       } else {
> > -             if (NPCM_I2CST_XMIT & i2cst) {
> > -                     bus->operation = I2C_WRITE_OPER;
> > +             if (bus->operation == I2C_WRITE_OPER)
> >                       npcm_i2c_irq_master_handler_write(bus);
> > -             } else {
> > -                     bus->operation = I2C_READ_OPER;
> > +             else if (bus->operation == I2C_READ_OPER)
> >                       npcm_i2c_irq_master_handler_read(bus);
>
> mmmhhh... you are changing the logic here and you are not
> describing the logic change in the commit log.
>
> Without looking at the details, if this is a state machine you
> are breaking it.
>
> Can anyone from the ARM/NUVOTON NPCM supporters and reviewers
> take a look at this patch?
>
> Thanks,
> Andi
>
> > -             }
> >       }
> >  }
> >
> > --
> > 2.34.1
> >

Thank you.

Regards,
Tyrone

[PATCH v2 3/7] i2c: npcm: use a software flag to indicate a BER condition

[Tyrone Ting]

If not clearing the BB (bus busy) condition in the BER (bus error)
interrupt, the driver causes a timeout and hence the i2c core
doesn't do the i2c transfer retry but returns the driver's return
value to the upper layer instead.

Clear the BB condition in the BER interrupt and a software flag is
used. The driver does an i2c recovery without causing the timeout
if the flag is set.

Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2b76dbfba438..2d034503d8bc 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -334,6 +334,7 @@ struct npcm_i2c {
 	u64 nack_cnt;
 	u64 timeout_cnt;
 	u64 tx_complete_cnt;
+	bool ber_state;
 };
 
 static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
@@ -1521,6 +1522,7 @@ static void npcm_i2c_irq_handle_ber(struct npcm_i2c *bus)
 	if (npcm_i2c_is_master(bus)) {
 		npcm_i2c_master_abort(bus);
 	} else {
+		bus->ber_state = true;
 		npcm_i2c_clear_master_status(bus);
 
 		/* Clear BB (BUS BUSY) bit */
@@ -1699,6 +1701,7 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
 		dev_dbg(bus->dev, "bus%d-0x%x recovery skipped, bus not stuck",
 			bus->num, bus->dest_addr);
 		npcm_i2c_reset(bus);
+		bus->ber_state = false;
 		return 0;
 	}
 
@@ -1763,6 +1766,7 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
 		if (bus->rec_succ_cnt < ULLONG_MAX)
 			bus->rec_succ_cnt++;
 	}
+	bus->ber_state = false;
 	return status;
 }
 
@@ -2158,7 +2162,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 	} while (time_is_after_jiffies(time_left) && bus_busy);
 
-	if (bus_busy) {
+	if (bus_busy || bus->ber_state) {
 		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
 		npcm_i2c_reset(bus);
 		i2c_recover_bus(adap);
-- 
2.34.1

Re: [PATCH v2 3/7] i2c: npcm: use a software flag to indicate a BER condition

[Andi Shyti]

Hi,

On Fri, Aug 30, 2024 at 11:46:36AM GMT, Tyrone Ting wrote:
> If not clearing the BB (bus busy) condition in the BER (bus error)
> interrupt, the driver causes a timeout and hence the i2c core
> doesn't do the i2c transfer retry but returns the driver's return
> value to the upper layer instead.
> 
> Clear the BB condition in the BER interrupt and a software flag is
> used. The driver does an i2c recovery without causing the timeout
> if the flag is set.
> 
> Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>

Can I have an ack from the supporters of the ARM/NUVOTON NPCM
ARCHITECTURE here?

Thanks,
Andi

[PATCH v2 4/7] i2c: npcm: Modify timeout evaluation mechanism

[Tyrone Ting]

Increase the timeout and treat it as the total timeout, including retries.
The total timeout is 2 seconds now.

The i2c core layer will have chances to retry to call the i2c driver
transfer function if the i2c driver reports that the bus is busy and
returns EAGAIN.

Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2d034503d8bc..68f3d47323ab 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2132,19 +2132,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		}
 	}
 
-	/*
-	 * Adaptive TimeOut: estimated time in usec + 100% margin:
-	 * 2: double the timeout for clock stretching case
-	 * 9: bits per transaction (including the ack/nack)
-	 */
-	timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
-	timeout = max_t(unsigned long, bus->adap.timeout, usecs_to_jiffies(timeout_usec));
 	if (nwrite >= 32 * 1024 || nread >= 32 * 1024) {
 		dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
 		return -EINVAL;
 	}
 
-	time_left = jiffies + timeout + 1;
+	time_left = jiffies + bus->adap.timeout / bus->adap.retries + 1;
 	do {
 		/*
 		 * we must clear slave address immediately when the bus is not
@@ -2183,6 +2176,14 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	if (npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
 				       write_data, read_data, read_PEC,
 				       read_block)) {
+		/*
+		 * Adaptive TimeOut: estimated time in usec + 100% margin:
+		 * 2: double the timeout for clock stretching case
+		 * 9: bits per transaction (including the ack/nack)
+		 */
+		timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
+		timeout = max_t(unsigned long, bus->adap.timeout / bus->adap.retries,
+				usecs_to_jiffies(timeout_usec));
 		time_left = wait_for_completion_timeout(&bus->cmd_complete,
 							timeout);
 
@@ -2308,7 +2309,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	adap = &bus->adap;
 	adap->owner = THIS_MODULE;
 	adap->retries = 3;
-	adap->timeout = msecs_to_jiffies(35);
+	adap->timeout = 2 * HZ;
 	adap->algo = &npcm_i2c_algo;
 	adap->quirks = &npcm_i2c_quirks;
 	adap->algo_data = bus;
-- 
2.34.1

Re: [PATCH v2 4/7] i2c: npcm: Modify timeout evaluation mechanism

[Andi Shyti]

Hi Tyrone,

On Fri, Aug 30, 2024 at 11:46:37AM GMT, Tyrone Ting wrote:
> Increase the timeout and treat it as the total timeout, including retries.
> The total timeout is 2 seconds now.

Why?

> The i2c core layer will have chances to retry to call the i2c driver
> transfer function if the i2c driver reports that the bus is busy and
> returns EAGAIN.
> 
> Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")

What is the bug you are fixing?

> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>

Still... can someone from the nuvoton supporters/reviewers check
this patch?

Thanks,
Andi

Re: [PATCH v2 4/7] i2c: npcm: Modify timeout evaluation mechanism

[Tali Perry]

Hi Andi,

On Fri, Sep 6, 2024 at 12:39 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Tyrone,
>
> On Fri, Aug 30, 2024 at 11:46:37AM GMT, Tyrone Ting wrote:
> > Increase the timeout and treat it as the total timeout, including retries.
> > The total timeout is 2 seconds now.
>
> Why?

The users want to connect a lot of masters on the same bus.
This timeout is used to determine the time it takes to take bus ownership.
The transactions are very long, so waiting 35ms is not enough.

>
> > The i2c core layer will have chances to retry to call the i2c driver
> > transfer function if the i2c driver reports that the bus is busy and
> > returns EAGAIN.
> >
> > Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
>
> What is the bug you are fixing?
>
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>
> Still... can someone from the nuvoton supporters/reviewers check
> this patch?
>
> Thanks,
> Andi

Thanks,
Tali Perry,
Nuvoton Technologies.

Re: [PATCH v2 4/7] i2c: npcm: Modify timeout evaluation mechanism

[Tyrone Ting]

Hi Andi:

Thank you for your review.

Tali Perry <tali.perry1@gmail.com> 於 2024年9月8日 週日 下午6:48寫道：
>
> Hi Andi,
>
> On Fri, Sep 6, 2024 at 12:39 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> >
> > Hi Tyrone,
> >
> > On Fri, Aug 30, 2024 at 11:46:37AM GMT, Tyrone Ting wrote:
> > > Increase the timeout and treat it as the total timeout, including retries.
> > > The total timeout is 2 seconds now.
> >
> > Why?
>
> The users want to connect a lot of masters on the same bus.
> This timeout is used to determine the time it takes to take bus ownership.
> The transactions are very long, so waiting 35ms is not enough.
>
> >
> > > The i2c core layer will have chances to retry to call the i2c driver
> > > transfer function if the i2c driver reports that the bus is busy and
> > > returns EAGAIN.
> > >
> > > Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
> >
> > What is the bug you are fixing?
> >

I'll remove the Fixes tag in the next patch set.

> > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> >
> > Still... can someone from the nuvoton supporters/reviewers check
> > this patch?
> >
> > Thanks,
> > Andi
>
> Thanks,
> Tali Perry,
> Nuvoton Technologies.

Thank you.

Regards,
Tyrone

[PATCH v2 5/7] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Store the client address earlier since it's used in the i2c_recover_bus
logic flow.

Fixes: 48acf8292280 ("i2c: Remove redundant comparison in npcm_i2c_reg_slave")
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 68f3d47323ab..67d156ed29b9 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2155,6 +2155,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 	} while (time_is_after_jiffies(time_left) && bus_busy);
 
+	bus->dest_addr = slave_addr << 1;
 	if (bus_busy || bus->ber_state) {
 		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
 		npcm_i2c_reset(bus);
@@ -2163,7 +2164,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	}
 
 	npcm_i2c_init_params(bus);
-	bus->dest_addr = slave_addr;
 	bus->msgs = msgs;
 	bus->msgs_num = num;
 	bus->cmd_err = 0;
-- 
2.34.1

Re: [PATCH v2 5/7] i2c: npcm: Modify the client address assignment

[Andy Shevchenko]

On Fri, Aug 30, 2024 at 11:46:38AM +0800, Tyrone Ting wrote:
> Store the client address earlier since it's used in the i2c_recover_bus
> logic flow.

Here no explanation why it's now left-shifted by one bit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/7] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andy:

Thank you for your comments.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年8月31日 週六 上午3:16寫道：
>
> On Fri, Aug 30, 2024 at 11:46:38AM +0800, Tyrone Ting wrote:
> > Store the client address earlier since it's used in the i2c_recover_bus
> > logic flow.
>
> Here no explanation why it's now left-shifted by one bit.

The address is stored from bit 1 to bit 7 in the register for sending
the i2c address later.
I'll write some comments about the left-shifted by one bit behavior
above this modification in next patch set.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thank you.

Regards,
Tyrone

Re: [PATCH v2 5/7] i2c: npcm: Modify the client address assignment

[Andy Shevchenko]

On Mon, Sep 02, 2024 at 09:40:09AM +0800, Tyrone Ting wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年8月31日 週六 上午3:16寫道：
> > On Fri, Aug 30, 2024 at 11:46:38AM +0800, Tyrone Ting wrote:
> > > Store the client address earlier since it's used in the i2c_recover_bus
> > > logic flow.
> >
> > Here no explanation why it's now left-shifted by one bit.
> 
> The address is stored from bit 1 to bit 7 in the register for sending
> the i2c address later.

Yes, but previously it was stored w/o that shift.

> I'll write some comments about the left-shifted by one bit behavior
> above this modification in next patch set.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/7] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andy:

Thank you for your feedback.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年9月2日 週一 下午7:54寫道：
>
> On Mon, Sep 02, 2024 at 09:40:09AM +0800, Tyrone Ting wrote:
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年8月31日 週六 上午3:16寫道：
> > > On Fri, Aug 30, 2024 at 11:46:38AM +0800, Tyrone Ting wrote:
> > > > Store the client address earlier since it's used in the i2c_recover_bus
> > > > logic flow.
> > >
> > > Here no explanation why it's now left-shifted by one bit.
> >
> > The address is stored from bit 1 to bit 7 in the register for sending
> > the i2c address later.
>
> Yes, but previously it was stored w/o that shift.
>

Previously, the address was stored w/o that shift and with that shift
in the following call to npcm_i2c_master_start_xmit,
just like what https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2043
shows.

Since there are cases that the i2c_recover_bus gets called at the
early stage of the function npcm_i2c_master_xfer,
the address is stored with the shift and used in the i2c_recover_bus call.

> > I'll write some comments about the left-shifted by one bit behavior
> > above this modification in next patch set.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thank you.

Regards,
Tyrone

[PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Tyrone Ting]

Modify i2c frequency from table parameters
for NPCM i2c modules.

Supported frequencies are:

1. 100KHz
2. 400KHz
3. 1MHz

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 230 +++++++++++++++++++------------
 1 file changed, 144 insertions(+), 86 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 67d156ed29b9..cac4ea0b69b8 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -263,6 +263,121 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define I2C_FREQ_MIN_HZ			10000
 #define I2C_FREQ_MAX_HZ			I2C_MAX_FAST_MODE_PLUS_FREQ
 
+struct SMB_TIMING_T {
+	u32 core_clk;
+	u8 hldt;
+	u8 dbcnt;
+	u16 sclfrq;
+	u8 scllt;
+	u8 sclht;
+	bool fast_mode;
+};
+
+static struct SMB_TIMING_T SMB_TIMING_100KHZ[] = {
+	{.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x4, .sclfrq = 0xFB, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 62500000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x9D, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 50000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x7E, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 48000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x79, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 40000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x65, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 30000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x4C, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 29000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x49, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 26000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x42, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 25000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x3F, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 24000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x3D, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 20000000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x33, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 16180000,  .hldt = 0x2A, .dbcnt = 0x1, .sclfrq = 0x29, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 15000000,  .hldt = 0x23, .dbcnt = 0x1, .sclfrq = 0x26, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 13000000,  .hldt = 0x1D, .dbcnt = 0x1, .sclfrq = 0x21, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 12000000,  .hldt = 0x1B, .dbcnt = 0x1, .sclfrq = 0x1F, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 10000000,  .hldt = 0x18, .dbcnt = 0x1, .sclfrq = 0x1A, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 9000000,   .hldt = 0x16, .dbcnt = 0x1, .sclfrq = 0x17, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 8090000,   .hldt = 0x14, .dbcnt = 0x1, .sclfrq = 0x15, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 7500000,   .hldt = 0x7,  .dbcnt = 0x1, .sclfrq = 0x13, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 6500000,   .hldt = 0xE,  .dbcnt = 0x1, .sclfrq = 0x11, .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false },
+	{.core_clk = 4000000,   .hldt = 0x9,  .dbcnt = 0x1, .sclfrq = 0xB,  .scllt = 0x0,
+	.sclht  = 0x0, .fast_mode = false }
+};
+
+static struct SMB_TIMING_T SMB_TIMING_400KHZ[] = {
+	{.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x3, .sclfrq = 0x0, .scllt = 0x47,
+	.sclht  = 0x35, .fast_mode = true },
+	{.core_clk = 62500000,  .hldt = 0x2A, .dbcnt = 0x2, .sclfrq = 0x0, .scllt = 0x2C,
+	.sclht  = 0x22, .fast_mode = true },
+	{.core_clk = 50000000,  .hldt = 0x21, .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x24,
+	.sclht  = 0x1B, .fast_mode = true },
+	{.core_clk = 48000000,  .hldt = 0x1E, .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x24,
+	.sclht  = 0x19, .fast_mode = true },
+	{.core_clk = 40000000,  .hldt = 0x1B, .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x1E,
+	.sclht  = 0x14, .fast_mode = true },
+	{.core_clk = 33000000,  .hldt = 0x15, .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x19,
+	.sclht  = 0x11, .fast_mode = true },
+	{.core_clk = 30000000,  .hldt = 0x15, .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x19,
+	.sclht  = 0xD,  .fast_mode = true },
+	{.core_clk = 29000000,  .hldt = 0x11, .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x15,
+	.sclht  = 0x10, .fast_mode = true },
+	{.core_clk = 26000000,  .hldt = 0x10, .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x13,
+	.sclht  = 0xE,  .fast_mode = true },
+	{.core_clk = 25000000,  .hldt = 0xF,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x13,
+	.sclht  = 0xD,  .fast_mode = true },
+	{.core_clk = 24000000,  .hldt = 0xD,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x12,
+	.sclht  = 0xD,  .fast_mode = true },
+	{.core_clk = 20000000,  .hldt = 0xB,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0xF,
+	.sclht  = 0xA,  .fast_mode = true },
+	{.core_clk = 16180000,  .hldt = 0xA,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0xC,
+	.sclht  = 0x9,  .fast_mode = true },
+	{.core_clk = 15000000,  .hldt = 0x9,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0xB,
+	.sclht  = 0x8,  .fast_mode = true },
+	{.core_clk = 13000000,  .hldt = 0x7,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0xA,
+	.sclht  = 0x7,  .fast_mode = true },
+	{.core_clk = 12000000,  .hldt = 0x7,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0xA,
+	.sclht  = 0x6,  .fast_mode = true },
+	{.core_clk = 10000000,  .hldt = 0x6,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x8,
+	.sclht  = 0x5,  .fast_mode = true },
+};
+
+static struct SMB_TIMING_T SMB_TIMING_1000KHZ[] = {
+	{.core_clk = 100000000, .hldt = 0x15, .dbcnt = 0x4, .sclfrq = 0x0, .scllt = 0x1C,
+	.sclht  = 0x15, .fast_mode = true },
+	{.core_clk = 62500000,  .hldt = 0xF,  .dbcnt = 0x3, .sclfrq = 0x0, .scllt = 0x11,
+	.sclht  = 0xE,  .fast_mode = true },
+	{.core_clk = 50000000,  .hldt = 0xA,  .dbcnt = 0x2, .sclfrq = 0x0, .scllt = 0xE,
+	.sclht  = 0xB,  .fast_mode = true },
+	{.core_clk = 48000000,  .hldt = 0x9,  .dbcnt = 0x2, .sclfrq = 0x0, .scllt = 0xD,
+	.sclht  = 0xB,  .fast_mode = true },
+	{.core_clk = 41000000,  .hldt = 0x9,  .dbcnt = 0x2, .sclfrq = 0x0, .scllt = 0xC,
+	.sclht  = 0x9,  .fast_mode = true },
+	{.core_clk = 40000000,  .hldt = 0x8,  .dbcnt = 0x2, .sclfrq = 0x0, .scllt = 0xB,
+	.sclht  = 0x9,  .fast_mode = true },
+	{.core_clk = 33000000,  .hldt = 0x7,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0xA,
+	.sclht  = 0x7,  .fast_mode = true },
+	{.core_clk = 25000000,  .hldt = 0x4,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x7,
+	.sclht  = 0x6,  .fast_mode = true },
+	{.core_clk = 24000000,  .hldt = 0x7,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x8,
+	.sclht  = 0x5,  .fast_mode = true },
+	{.core_clk = 20000000,  .hldt = 0x4,  .dbcnt = 0x1, .sclfrq = 0x0, .scllt = 0x6,
+	.sclht  = 0x4,  .fast_mode = true },
+};
+
 struct npcm_i2c_data {
 	u8 fifo_size;
 	u32 segctl_init_val;
@@ -1805,102 +1920,45 @@ static void npcm_i2c_recovery_init(struct i2c_adapter *_adap)
  */
 static int npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq_hz)
 {
-	u32  k1 = 0;
-	u32  k2 = 0;
-	u8   dbnct = 0;
-	u32  sclfrq = 0;
-	u8   hldt = 7;
+	struct  SMB_TIMING_T *smb_timing;
+	u8   scl_table_cnt = 0, table_size = 0;
 	u8   fast_mode = 0;
-	u32  src_clk_khz;
-	u32  bus_freq_khz;
 
-	src_clk_khz = bus->apb_clk / 1000;
-	bus_freq_khz = bus_freq_hz / 1000;
 	bus->bus_freq = bus_freq_hz;
 
-	/* 100KHz and below: */
-	if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
-		sclfrq = src_clk_khz / (bus_freq_khz * 4);
-
-		if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
-			return -EDOM;
-
-		if (src_clk_khz >= 40000)
-			hldt = 17;
-		else if (src_clk_khz >= 12500)
-			hldt = 15;
-		else
-			hldt = 7;
-	}
-
-	/* 400KHz: */
-	else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
-		sclfrq = 0;
+	switch (bus_freq_hz) {
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		smb_timing = SMB_TIMING_100KHZ;
+		table_size = ARRAY_SIZE(SMB_TIMING_100KHZ);
+		break;
+	case I2C_MAX_FAST_MODE_FREQ:
+		smb_timing = SMB_TIMING_400KHZ;
+		table_size = ARRAY_SIZE(SMB_TIMING_400KHZ);
 		fast_mode = I2CCTL3_400K_MODE;
-
-		if (src_clk_khz < 7500)
-			/* 400KHZ cannot be supported for core clock < 7.5MHz */
-			return -EDOM;
-
-		else if (src_clk_khz >= 50000) {
-			k1 = 80;
-			k2 = 48;
-			hldt = 12;
-			dbnct = 7;
-		}
-
-		/* Master or Slave with frequency > 25MHz */
-		else if (src_clk_khz > 25000) {
-			hldt = clk_coef(src_clk_khz, 300) + 7;
-			k1 = clk_coef(src_clk_khz, 1600);
-			k2 = clk_coef(src_clk_khz, 900);
-		}
-	}
-
-	/* 1MHz: */
-	else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
-		sclfrq = 0;
+		break;
+	case I2C_MAX_FAST_MODE_PLUS_FREQ:
+		smb_timing = SMB_TIMING_1000KHZ;
+		table_size = ARRAY_SIZE(SMB_TIMING_1000KHZ);
 		fast_mode = I2CCTL3_400K_MODE;
-
-		/* 1MHZ cannot be supported for core clock < 24 MHz */
-		if (src_clk_khz < 24000)
-			return -EDOM;
-
-		k1 = clk_coef(src_clk_khz, 620);
-		k2 = clk_coef(src_clk_khz, 380);
-
-		/* Core clk > 40 MHz */
-		if (src_clk_khz > 40000) {
-			/*
-			 * Set HLDT:
-			 * SDA hold time:  (HLDT-7) * T(CLK) >= 120
-			 * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
-			 */
-			hldt = clk_coef(src_clk_khz, 120) + 7;
-		} else {
-			hldt = 7;
-			dbnct = 2;
-		}
+		break;
+	default:
+		return -EINVAL;
 	}
 
-	/* Frequency larger than 1 MHz is not supported */
-	else
-		return -EINVAL;
+	for (scl_table_cnt = 0 ; scl_table_cnt < table_size ; scl_table_cnt++)
+		if (bus->apb_clk >= smb_timing[scl_table_cnt].core_clk)
+			break;
 
-	if (bus_freq_hz >= I2C_MAX_FAST_MODE_FREQ) {
-		k1 = round_up(k1, 2);
-		k2 = round_up(k2 + 1, 2);
-		if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
-		    k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
-			return -EDOM;
-	}
+	if (scl_table_cnt == table_size)
+		return -EINVAL;
 
 	/* write sclfrq value. bits [6:0] are in I2CCTL2 reg */
-	iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, sclfrq & 0x7F),
+	iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, smb_timing[scl_table_cnt].sclfrq & 0x7F),
 		 bus->reg + NPCM_I2CCTL2);
 
 	/* bits [8:7] are in I2CCTL3 reg */
-	iowrite8(fast_mode | FIELD_PREP(I2CCTL3_SCLFRQ8_7, (sclfrq >> 7) & 0x3),
+	iowrite8(fast_mode | FIELD_PREP(I2CCTL3_SCLFRQ8_7, (smb_timing[scl_table_cnt].sclfrq >> 7)
+		 & 0x3),
 		 bus->reg + NPCM_I2CCTL3);
 
 	/* Select Bank 0 to access NPCM_I2CCTL4/NPCM_I2CCTL5 */
@@ -1912,13 +1970,13 @@ static int npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq_hz)
 		 * k1 = 2 * SCLLT7-0 -> Low Time  = k1 / 2
 		 * k2 = 2 * SCLLT7-0 -> High Time = k2 / 2
 		 */
-		iowrite8(k1 / 2, bus->reg + NPCM_I2CSCLLT);
-		iowrite8(k2 / 2, bus->reg + NPCM_I2CSCLHT);
+		iowrite8(smb_timing[scl_table_cnt].scllt, bus->reg + NPCM_I2CSCLLT);
+		iowrite8(smb_timing[scl_table_cnt].sclht, bus->reg + NPCM_I2CSCLHT);
 
-		iowrite8(dbnct, bus->reg + NPCM_I2CCTL5);
+		iowrite8(smb_timing[scl_table_cnt].dbcnt, bus->reg + NPCM_I2CCTL5);
 	}
 
-	iowrite8(hldt, bus->reg + NPCM_I2CCTL4);
+	iowrite8(smb_timing[scl_table_cnt].hldt, bus->reg + NPCM_I2CCTL4);
 
 	/* Return to Bank 1, and stay there by default: */
 	npcm_i2c_select_bank(bus, I2C_BANK_1);
-- 
2.34.1

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Andy Shevchenko]

On Fri, Aug 30, 2024 at 11:46:39AM +0800, Tyrone Ting wrote:
> Modify i2c frequency from table parameters
> for NPCM i2c modules.
> 
> Supported frequencies are:
> 
> 1. 100KHz
> 2. 400KHz
> 3. 1MHz

There is no explanations "why". What's wrong with the calculations done in the
current code?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Tali Perry]

On Fri, Aug 30, 2024 at 10:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 30, 2024 at 11:46:39AM +0800, Tyrone Ting wrote:
> > Modify i2c frequency from table parameters
> > for NPCM i2c modules.
> >
> > Supported frequencies are:
> >
> > 1. 100KHz
> > 2. 400KHz
> > 3. 1MHz
>
> There is no explanations "why". What's wrong with the calculations done in the
> current code?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Hi Andy,

The original equations were tested on a variety of chips and base clocks.
Since we added devices that use higher frequencies of the module we
saw that there is a mismatch between the equation and the actual
results on the bus itself, measured on scope.
So instead of using the equations we did an optimization per module
frequency, verified on a device.
Most of the work was focused on the rise time of the SCL and SDA,
which depends on external load of the bus and PU.
We needed to make sure that in all valid range of load the rise time
is compliant of the SMB spec timing requirements.

This patch include the final values after extensive testing both at
Nuvoton as well as at customer sites.

BR,
Tali Perry
Nuvoton.

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Andy Shevchenko]

On Sun, Sep 01, 2024 at 06:53:38PM +0300, Tali Perry wrote:
> On Fri, Aug 30, 2024 at 10:19 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Fri, Aug 30, 2024 at 11:46:39AM +0800, Tyrone Ting wrote:
> > > Modify i2c frequency from table parameters
> > > for NPCM i2c modules.
> > >
> > > Supported frequencies are:
> > >
> > > 1. 100KHz
> > > 2. 400KHz
> > > 3. 1MHz
> >
> > There is no explanations "why". What's wrong with the calculations done in the
> > current code?
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> Hi Andy,
> 
> The original equations were tested on a variety of chips and base clocks.
> Since we added devices that use higher frequencies of the module we
> saw that there is a mismatch between the equation and the actual
> results on the bus itself, measured on scope.
> So instead of using the equations we did an optimization per module
> frequency, verified on a device.
> Most of the work was focused on the rise time of the SCL and SDA,
> which depends on external load of the bus and PU.
> We needed to make sure that in all valid range of load the rise time
> is compliant of the SMB spec timing requirements.
> 
> This patch include the final values after extensive testing both at
> Nuvoton as well as at customer sites.

But:
1) why is it better than do calculations?
2) can it be problematic on theoretically different PCB design in the future?

P.S. If there is a good explanations to these and more, elaborate this in the
commit message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Tali Perry]

Hi Andy,

On Mon, Sep 2, 2024 at 3:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Sun, Sep 01, 2024 at 06:53:38PM +0300, Tali Perry wrote:
> > On Fri, Aug 30, 2024 at 10:19 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Fri, Aug 30, 2024 at 11:46:39AM +0800, Tyrone Ting wrote:
> > > > Modify i2c frequency from table parameters
> > > > for NPCM i2c modules.
> > > >
> > > > Supported frequencies are:
> > > >
> > > > 1. 100KHz
> > > > 2. 400KHz
> > > > 3. 1MHz
> > >
> > > There is no explanations "why". What's wrong with the calculations done in the
> > > current code?
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> > Hi Andy,
> >
> > The original equations were tested on a variety of chips and base clocks.
> > Since we added devices that use higher frequencies of the module we
> > saw that there is a mismatch between the equation and the actual
> > results on the bus itself, measured on scope.
> > So instead of using the equations we did an optimization per module
> > frequency, verified on a device.
> > Most of the work was focused on the rise time of the SCL and SDA,
> > which depends on external load of the bus and PU.
> > We needed to make sure that in all valid range of load the rise time
> > is compliant of the SMB spec timing requirements.
> >
> > This patch include the final values after extensive testing both at
> > Nuvoton as well as at customer sites.
>
> But:
> 1) why is it better than do calculations?
> 2) can it be problematic on theoretically different PCB design in the future?
>
> P.S. If there is a good explanations to these and more, elaborate this in the
> commit message.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thanks for your comments,

1) The equations were not accurate to begin with.
  They are an approximation of the ideal value.
  The ideal value is calculated per frequency of the core module.
2) As you wrote , different PCB designs, or specifically to this case
: the number and type of targets on the bus,
   impact the required values for the timing registers.
   Users can recalculate the numbers for each bus ( out of 24) and get
an even better optimization,
   but our users chose not to.
  Instead - we manually picked values per frequency that match the
entire valid range of targets (from 1 to max number).
  Then we check against the AMR described in SMB spec and make sure
that none of the values is exceeding.
  this process was led by the chip architect and included a lot of testing.

Do we need to add this entire description to the commit message? It
sounds a bit too detailed to me.

Thanks,
Tali Perry, Nuvoton

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Andy Shevchenko]

On Sun, Sep 08, 2024 at 11:54:50AM +0300, Tali Perry wrote:
> On Mon, Sep 2, 2024 at 3:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sun, Sep 01, 2024 at 06:53:38PM +0300, Tali Perry wrote:
> > > On Fri, Aug 30, 2024 at 10:19 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > The original equations were tested on a variety of chips and base clocks.
> > > Since we added devices that use higher frequencies of the module we
> > > saw that there is a mismatch between the equation and the actual
> > > results on the bus itself, measured on scope.
> > > So instead of using the equations we did an optimization per module
> > > frequency, verified on a device.
> > > Most of the work was focused on the rise time of the SCL and SDA,
> > > which depends on external load of the bus and PU.
> > > We needed to make sure that in all valid range of load the rise time
> > > is compliant of the SMB spec timing requirements.
> > >
> > > This patch include the final values after extensive testing both at
> > > Nuvoton as well as at customer sites.
> >
> > But:
> > 1) why is it better than do calculations?
> > 2) can it be problematic on theoretically different PCB design in the future?
> >
> > P.S. If there is a good explanations to these and more, elaborate this in the
> > commit message.
> 
> Thanks for your comments,
> 
> 1) The equations were not accurate to begin with.
>   They are an approximation of the ideal value.
>   The ideal value is calculated per frequency of the core module.

This is crucial detail.

> 2) As you wrote , different PCB designs, or specifically to this case
> : the number and type of targets on the bus,
>    impact the required values for the timing registers.
>    Users can recalculate the numbers for each bus ( out of 24) and get
> an even better optimization,
>    but our users chose not to.
>   Instead - we manually picked values per frequency that match the
> entire valid range of targets (from 1 to max number).
>   Then we check against the AMR described in SMB spec and make sure
> that none of the values is exceeding.
>   this process was led by the chip architect and included a lot of testing.
> 
> Do we need to add this entire description to the commit message? It
> sounds a bit too detailed to me.

Yes, please.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Andi Shyti]

Hi,

On Fri, Aug 30, 2024 at 11:46:39AM GMT, Tyrone Ting wrote:
> Modify i2c frequency from table parameters
> for NPCM i2c modules.
> 
> Supported frequencies are:
> 
> 1. 100KHz
> 2. 400KHz
> 3. 1MHz

I agree with Andy, please add a good explanation for this change.

> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 230 +++++++++++++++++++------------
>  1 file changed, 144 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index 67d156ed29b9..cac4ea0b69b8 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -263,6 +263,121 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
>  #define I2C_FREQ_MIN_HZ			10000
>  #define I2C_FREQ_MAX_HZ			I2C_MAX_FAST_MODE_PLUS_FREQ
>  
> +struct SMB_TIMING_T {

Please run checkpatch.pl before sending the patches.

Thanks,
Andi

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Tyrone Ting]

Hi Andi:

Thank you for your review.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:43寫道：
>
> Hi,
>
> On Fri, Aug 30, 2024 at 11:46:39AM GMT, Tyrone Ting wrote:
> > Modify i2c frequency from table parameters
> > for NPCM i2c modules.
> >
> > Supported frequencies are:
> >
> > 1. 100KHz
> > 2. 400KHz
> > 3. 1MHz
>
> I agree with Andy, please add a good explanation for this change.
>
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > ---
> >  drivers/i2c/busses/i2c-npcm7xx.c | 230 +++++++++++++++++++------------
> >  1 file changed, 144 insertions(+), 86 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index 67d156ed29b9..cac4ea0b69b8 100644
> > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > @@ -263,6 +263,121 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
> >  #define I2C_FREQ_MIN_HZ                      10000
> >  #define I2C_FREQ_MAX_HZ                      I2C_MAX_FAST_MODE_PLUS_FREQ
> >
> > +struct SMB_TIMING_T {
>
> Please run checkpatch.pl before sending the patches.
I did run the checkpatch.pl against this patch.
Here is the log from the checkpatch.pl:
-------------------------------------------------------------
./patch_i2c_v2/v2-0006-i2c-npcm-use-i2c-frequency-table.patch
-------------------------------------------------------------
total: 0 errors, 0 warnings, 265 lines checked

./patch_i2c_v2/v2-0006-i2c-npcm-use-i2c-frequency-table.patch has no
obvious style problems and is ready for submission.

It seems that the values of I2C_FREQ_MIN_HZ and I2C_FREQ_MAX_HZ are
not aligned in the email but they're
aligned in my editor (Vim with a default configuration).
>
> Thanks,
> Andi

Thank you.

Regards,
Tyrone

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Andi Shyti]

On Mon, Sep 09, 2024 at 09:56:25AM GMT, Tyrone Ting wrote:
> Hi Andi:
> 
> Thank you for your review.
> 
> Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:43寫道：
> >
> > Hi,
> >
> > On Fri, Aug 30, 2024 at 11:46:39AM GMT, Tyrone Ting wrote:
> > > Modify i2c frequency from table parameters
> > > for NPCM i2c modules.
> > >
> > > Supported frequencies are:
> > >
> > > 1. 100KHz
> > > 2. 400KHz
> > > 3. 1MHz
> >
> > I agree with Andy, please add a good explanation for this change.
> >
> > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > > ---
> > >  drivers/i2c/busses/i2c-npcm7xx.c | 230 +++++++++++++++++++------------
> > >  1 file changed, 144 insertions(+), 86 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > > index 67d156ed29b9..cac4ea0b69b8 100644
> > > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > > @@ -263,6 +263,121 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
> > >  #define I2C_FREQ_MIN_HZ                      10000
> > >  #define I2C_FREQ_MAX_HZ                      I2C_MAX_FAST_MODE_PLUS_FREQ
> > >
> > > +struct SMB_TIMING_T {
> >
> > Please run checkpatch.pl before sending the patches.
> I did run the checkpatch.pl against this patch.
> Here is the log from the checkpatch.pl:
> -------------------------------------------------------------
> ./patch_i2c_v2/v2-0006-i2c-npcm-use-i2c-frequency-table.patch
> -------------------------------------------------------------
> total: 0 errors, 0 warnings, 265 lines checked
> 
> ./patch_i2c_v2/v2-0006-i2c-npcm-use-i2c-frequency-table.patch has no
> obvious style problems and is ready for submission.

mmhhh... I thought checkpatch hated capital letter declarations.

Please, then, use only lower character names for declarations.

Thanks,
Andi

Re: [PATCH v2 6/7] i2c: npcm: use i2c frequency table

[Tyrone Ting]

Hi Andy:

Thank you for your comments.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月9日 週一 下午8:57寫道：
>
> On Mon, Sep 09, 2024 at 09:56:25AM GMT, Tyrone Ting wrote:
> > Hi Andi:
> >
> > Thank you for your review.
> >
> > Andi Shyti <andi.shyti@kernel.org> 於 2024年9月6日 週五 上午5:43寫道：
> > >
> > > Hi,
> > >
> > > On Fri, Aug 30, 2024 at 11:46:39AM GMT, Tyrone Ting wrote:
> > > > Modify i2c frequency from table parameters
> > > > for NPCM i2c modules.
> > > >
> > > > Supported frequencies are:
> > > >
> > > > 1. 100KHz
> > > > 2. 400KHz
> > > > 3. 1MHz
> > >
> > > I agree with Andy, please add a good explanation for this change.
> > >
> > > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > > > ---
> > > >  drivers/i2c/busses/i2c-npcm7xx.c | 230 +++++++++++++++++++------------
> > > >  1 file changed, 144 insertions(+), 86 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > > > index 67d156ed29b9..cac4ea0b69b8 100644
> > > > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > > > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > > > @@ -263,6 +263,121 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
> > > >  #define I2C_FREQ_MIN_HZ                      10000
> > > >  #define I2C_FREQ_MAX_HZ                      I2C_MAX_FAST_MODE_PLUS_FREQ
> > > >
> > > > +struct SMB_TIMING_T {
> > >
> > > Please run checkpatch.pl before sending the patches.
> > I did run the checkpatch.pl against this patch.
> > Here is the log from the checkpatch.pl:
> > -------------------------------------------------------------
> > ./patch_i2c_v2/v2-0006-i2c-npcm-use-i2c-frequency-table.patch
> > -------------------------------------------------------------
> > total: 0 errors, 0 warnings, 265 lines checked
> >
> > ./patch_i2c_v2/v2-0006-i2c-npcm-use-i2c-frequency-table.patch has no
> > obvious style problems and is ready for submission.
>
> mmhhh... I thought checkpatch hated capital letter declarations.
>
> Please, then, use only lower character names for declarations.
>
I'll use lower character names for declarations in next patch set.

> Thanks,
> Andi

Thank you.

Regards,
Tyrone

[PATCH v2 7/7] i2c: npcm: Enable slave in eob interrupt

[Tyrone Ting]

From: Charles Boyer <Charles.Boyer@fii-usa.com>

Nuvoton slave enable was in user space API call master_xfer, so it is
subject to delays from the OS scheduler. If the BMC is not enabled for
slave mode in time for master to send response, then it will NAK the
address match. Then the PLDM request timeout occurs.

If the slave enable is moved to the EOB interrupt service routine, then
the BMC can be ready in slave mode by the time it needs to receive a
response.

Signed-off-by: Charles Boyer <Charles.Boyer@fii-usa.com>
Signed-off-by: Vivekanand Veeracholan <vveerach@google.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index cac4ea0b69b8..5bdc1b5895ac 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -1781,6 +1781,12 @@ static int npcm_i2c_int_master_handler(struct npcm_i2c *bus)
 	    (FIELD_GET(NPCM_I2CCST3_EO_BUSY,
 		       ioread8(bus->reg + NPCM_I2CCST3)))) {
 		npcm_i2c_irq_handle_eob(bus);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		/* reenable slave if it was enabled */
+		if (bus->slave)
+			iowrite8((bus->slave->addr & 0x7F) | NPCM_I2CADDR_SAEN,
+				 bus->reg + NPCM_I2CADDR1);
+#endif
 		return 0;
 	}
 
-- 
2.34.1

Re: [PATCH v2 7/7] i2c: npcm: Enable slave in eob interrupt

[Tali Perry]

Reviewed-by: Tali Perry <tali.perry1@gmail.com>

On Fri, Aug 30, 2024 at 6:49 AM Tyrone Ting <warp5tw@gmail.com> wrote:
>
> From: Charles Boyer <Charles.Boyer@fii-usa.com>
>
> Nuvoton slave enable was in user space API call master_xfer, so it is
> subject to delays from the OS scheduler. If the BMC is not enabled for
> slave mode in time for master to send response, then it will NAK the
> address match. Then the PLDM request timeout occurs.
>
> If the slave enable is moved to the EOB interrupt service routine, then
> the BMC can be ready in slave mode by the time it needs to receive a
> response.
>
> Signed-off-by: Charles Boyer <Charles.Boyer@fii-usa.com>
> Signed-off-by: Vivekanand Veeracholan <vveerach@google.com>
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index cac4ea0b69b8..5bdc1b5895ac 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -1781,6 +1781,12 @@ static int npcm_i2c_int_master_handler(struct npcm_i2c *bus)
>             (FIELD_GET(NPCM_I2CCST3_EO_BUSY,
>                        ioread8(bus->reg + NPCM_I2CCST3)))) {
>                 npcm_i2c_irq_handle_eob(bus);
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +               /* reenable slave if it was enabled */
> +               if (bus->slave)
> +                       iowrite8((bus->slave->addr & 0x7F) | NPCM_I2CADDR_SAEN,
> +                                bus->reg + NPCM_I2CADDR1);
> +#endif
>                 return 0;
>         }
>
> --
> 2.34.1
>

Re: [PATCH v2 0/7] i2c: npcm: Bug fixes read/write operation, checkpatch

[Tali Perry]

Reviewed-by: Tali Perry <tali.perry1@gmail.com>

On Fri, Aug 30, 2024 at 6:48 AM Tyrone Ting <warp5tw@gmail.com> wrote:
>
> This patchset includes the following fixes:
>
> - Restore the npcm_i2caddr array length to fix the smatch warning.
> - Enable the target functionality in the interrupt handling routine
>   when the i2c transfer is about to finish.
> - Correct the read/write operation procedure.
> - Introduce a software flag to handle the bus error (BER) condition
>   which is not caused by the i2c transfer.
> - Modify timeout calculation.
> - Assign the client address earlier logically.
> - Use an i2c frequency table for the frequency parameters assignment.
> - Coding style fix.
>
> The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.
>
> Addressed comments from:
> - kernel test robot : https://lore.kernel.org/oe-kbuild-all/
>   202408080319.de2B6PgU-lkp@intel.com/
> - Dan Carpenter : https://lore.kernel.org/all/202408130818
>   .FgDP5uNm-lkp@intel.com/
> - Andrew Jeffery : https://lore.kernel.org/lkml/
>   20240807100244.16872-7-kfting@nuvoton.com/T/
>   #m3ed3351bf59675bfe0de89c75aae1fb26cad5567
>
> Changes since version 1:
> - Restore the npcm_i2caddr array length to fix the smatch warning.
> - Remove unused variables.
> - Handle the condition where scl_table_cnt reaches to the maximum value.
> - Fix the checkpatch warning.
>
> Charles Boyer (1):
>   i2c: npcm: Enable slave in eob interrupt
>
> Tyrone Ting (6):
>   i2c: npcm: restore slave addresses array length
>   i2c: npcm: correct the read/write operation procedure
>   i2c: npcm: use a software flag to indicate a BER condition
>   i2c: npcm: Modify timeout evaluation mechanism
>   i2c: npcm: Modify the client address assignment
>   i2c: npcm: use i2c frequency table
>
>  drivers/i2c/busses/i2c-npcm7xx.c | 276 +++++++++++++++++++------------
>  1 file changed, 172 insertions(+), 104 deletions(-)
>
>
> base-commit: 5be63fc19fcaa4c236b307420483578a56986a37
> --
> 2.34.1
>

Re: [PATCH v2 0/7] i2c: npcm: Bug fixes read/write operation, checkpatch

[Andi Shyti]

Hi,

On Fri, Aug 30, 2024 at 11:46:33AM GMT, Tyrone Ting wrote:
> This patchset includes the following fixes:
> 
> - Restore the npcm_i2caddr array length to fix the smatch warning.
> - Enable the target functionality in the interrupt handling routine 
>   when the i2c transfer is about to finish.
> - Correct the read/write operation procedure.
> - Introduce a software flag to handle the bus error (BER) condition
>   which is not caused by the i2c transfer.
> - Modify timeout calculation.
> - Assign the client address earlier logically.
> - Use an i2c frequency table for the frequency parameters assignment.
> - Coding style fix.
> 
> The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

first of all, Thanks Tali for your reviews.

Second, Tyronne, can we please incorporate Tali's comments into
commit messages and improve the code comments so that we don't
leave room for more questions?

Consider that not everyone knows the device and we need good
reasons for accepting the changes.

Thanks,
Andi


> Addressed comments from:
> - kernel test robot : https://lore.kernel.org/oe-kbuild-all/
>   202408080319.de2B6PgU-lkp@intel.com/
> - Dan Carpenter : https://lore.kernel.org/all/202408130818
>   .FgDP5uNm-lkp@intel.com/
> - Andrew Jeffery : https://lore.kernel.org/lkml/
>   20240807100244.16872-7-kfting@nuvoton.com/T/
>   #m3ed3351bf59675bfe0de89c75aae1fb26cad5567
> 
> Changes since version 1:
> - Restore the npcm_i2caddr array length to fix the smatch warning.
> - Remove unused variables.
> - Handle the condition where scl_table_cnt reaches to the maximum value.
> - Fix the checkpatch warning.
> 
> Charles Boyer (1):
>   i2c: npcm: Enable slave in eob interrupt
> 
> Tyrone Ting (6):
>   i2c: npcm: restore slave addresses array length
>   i2c: npcm: correct the read/write operation procedure
>   i2c: npcm: use a software flag to indicate a BER condition
>   i2c: npcm: Modify timeout evaluation mechanism
>   i2c: npcm: Modify the client address assignment
>   i2c: npcm: use i2c frequency table
> 
>  drivers/i2c/busses/i2c-npcm7xx.c | 276 +++++++++++++++++++------------
>  1 file changed, 172 insertions(+), 104 deletions(-)
> 
> 
> base-commit: 5be63fc19fcaa4c236b307420483578a56986a37
> -- 
> 2.34.1
> 

Re: [PATCH v2 0/7] i2c: npcm: Bug fixes read/write operation, checkpatch

[Tyrone Ting]

Hi Andi:

Thank you for your feedback.

Andi Shyti <andi.shyti@kernel.org> 於 2024年9月9日 週一 下午9:00寫道：
>
> Hi,
>
> On Fri, Aug 30, 2024 at 11:46:33AM GMT, Tyrone Ting wrote:
> > This patchset includes the following fixes:
> >
> > - Restore the npcm_i2caddr array length to fix the smatch warning.
> > - Enable the target functionality in the interrupt handling routine
> >   when the i2c transfer is about to finish.
> > - Correct the read/write operation procedure.
> > - Introduce a software flag to handle the bus error (BER) condition
> >   which is not caused by the i2c transfer.
> > - Modify timeout calculation.
> > - Assign the client address earlier logically.
> > - Use an i2c frequency table for the frequency parameters assignment.
> > - Coding style fix.
> >
> > The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.
>
> first of all, Thanks Tali for your reviews.
>
> Second, Tyronne, can we please incorporate Tali's comments into
> commit messages and improve the code comments so that we don't
> leave room for more questions?
>
Understood. They'll be incorporated in next patch set.

> Consider that not everyone knows the device and we need good
> reasons for accepting the changes.
>
> Thanks,
> Andi
>
>
> > Addressed comments from:
> > - kernel test robot : https://lore.kernel.org/oe-kbuild-all/
> >   202408080319.de2B6PgU-lkp@intel.com/
> > - Dan Carpenter : https://lore.kernel.org/all/202408130818
> >   .FgDP5uNm-lkp@intel.com/
> > - Andrew Jeffery : https://lore.kernel.org/lkml/
> >   20240807100244.16872-7-kfting@nuvoton.com/T/
> >   #m3ed3351bf59675bfe0de89c75aae1fb26cad5567
> >
> > Changes since version 1:
> > - Restore the npcm_i2caddr array length to fix the smatch warning.
> > - Remove unused variables.
> > - Handle the condition where scl_table_cnt reaches to the maximum value.
> > - Fix the checkpatch warning.
> >
> > Charles Boyer (1):
> >   i2c: npcm: Enable slave in eob interrupt
> >
> > Tyrone Ting (6):
> >   i2c: npcm: restore slave addresses array length
> >   i2c: npcm: correct the read/write operation procedure
> >   i2c: npcm: use a software flag to indicate a BER condition
> >   i2c: npcm: Modify timeout evaluation mechanism
> >   i2c: npcm: Modify the client address assignment
> >   i2c: npcm: use i2c frequency table
> >
> >  drivers/i2c/busses/i2c-npcm7xx.c | 276 +++++++++++++++++++------------
> >  1 file changed, 172 insertions(+), 104 deletions(-)
> >
> >
> > base-commit: 5be63fc19fcaa4c236b307420483578a56986a37
> > --
> > 2.34.1
> >

Thank you.

Regards,
Tyrone