[PATCH v3 11/11] i2c: npcm: Support NPCM845

[Tomer Maimon]

Hi Andy,

Thanks for your review,

On Thu, 3 Mar 2022 at 16:11, Andy Shevchenko <
andriy.shevchenko at linux.intel.com> wrote:

> On Thu, Mar 03, 2022 at 02:35:58PM +0200, Tali Perry wrote:
> > > On Thu, Mar 3, 2022 at 12:45 PM Andy Shevchenko <
> andriy.shevchenko at linux.intel.com> wrote:
> > > > On Thu, Mar 03, 2022 at 04:31:41PM +0800, Tyrone Ting wrote:
>
> ...
>
> > > > > -     left_in_fifo = FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
> > > > > -                              ioread8(bus->reg +
> NPCM_I2CTXF_STS));
> > > > > +     left_in_fifo = (bus->data->txf_sts_tx_bytes &
> > > > > +                     ioread8(bus->reg + NPCM_I2CTXF_STS));
> > > >
> > > > Besides too many parentheses, this is an interesting change. So, in
> different
> > > > versions of IP the field is on different bits? Perhaps it means that
> you need
> > > > something like internal ops structure for all these, where you will
> have been
> > > > using the statically defined masks?
> > > >
> >
> > Those are two very similar modules. The first generation had a 16 bytes
> HW FIFO
> > and the second generation has 32 bytes.
> > In V1 of this patchset the masks were defined under
> > CONFIG but we were asked to change the approach:
> >
> > the entire discussion can be found here:
> >
> > https://www.spinics.net/lists/linux-i2c/msg55566.html
> >
> > Did we understand the request change right?
>
> Not really. If you have not simply "one (MSB) bit more" for FIFO size, then
> I proposed to create a specific operations structure and use callbacks (see
> drivers/dma/dw/ case for iDMA 32-bit vs. DesignWare).
>
> But hold on and read set of questions below.
>
> Previously it was a fixed field with the NPCM_I2CTXF_STS_TX_BYTES mask
> applied,
> right? From above I have got that FIFO is growing twice. Is it correct?
>

What do you mean by growing twice? TX and RX?


> Does the LSB stay at the same offset? What is the meaning of the MSB in 32
> byte
> case? If it's reserved then why not to always use 32 byte approach?
>

Yes, the LSB stays in the same place, and bit 5 is reserved in the NPCM7XX
SoC.
Unfortunately, the I2C test failed when we tried to use the 32 bytes
approach at NPCM7XX Soc, this is why we added NPCM_I2CTXF_STS_TX_BYTES and
NPCM_I2C_STSRXF_RX_BYTES to the data structure.

The device tree data structure pass data for each specific device, so I
don't understand why not use device tree data for supporting the I2C
specific device? this is not the case here?


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

Tomer Maimon
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20220306/3b07b91b/attachment.htm>