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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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