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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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