diff mbox series

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

Message ID 20240830034640.7049-7-kfting@nuvoton.com
State New
Headers show
Series i2c: npcm: Bug fixes read/write operation, checkpatch | expand

Commit Message

Tyrone Ting Aug. 30, 2024, 3:46 a.m. UTC
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(-)

Comments

Andy Shevchenko Aug. 30, 2024, 7:19 p.m. UTC | #1
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?
Tali Perry Sept. 1, 2024, 3:53 p.m. UTC | #2
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.
Andy Shevchenko Sept. 2, 2024, 11:53 a.m. UTC | #3
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.
Andi Shyti Sept. 5, 2024, 9:43 p.m. UTC | #4
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
Tali Perry Sept. 8, 2024, 8:54 a.m. UTC | #5
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
Tyrone Ting Sept. 9, 2024, 1:56 a.m. UTC | #6
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
Andy Shevchenko Sept. 9, 2024, 10:27 a.m. UTC | #7
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.
Andi Shyti Sept. 9, 2024, 12:57 p.m. UTC | #8
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
Tyrone Ting Sept. 10, 2024, 1:11 a.m. UTC | #9
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
diff mbox series

Patch

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);