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

From: Tomer Maimon <tmaimon77@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: devicetree <devicetree@vger.kernel.org>,
	Tali Perry <tali.perry1@gmail.com>,
	Linux I2C <linux-i2c@vger.kernel.org>,
	Benjamin Fair <benjaminfair@google.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
	OpenBMC Maillist <openbmc@lists.ozlabs.org>,
	JJLIU0@nuvoton.com, lukas.bulwahn@gmail.com,
	Tomer Maimon <tomer.maimon@nuvoton.com>,
	KWLIU@nuvoton.com, bence98@sch.bme.hu,
	Arnd Bergmann <arnd@arndb.de>,
	sven@svenpeter.dev, Rob Herring <robh+dt@kernel.org>,
	Avi Fishman <Avi.Fishman@nuvoton.com>,
	Tyrone Ting <warp5tw@gmail.com>,
	yangyicong@hisilicon.com, semen.protsenko@linaro.org,
	jie.deng@intel.com, Avi Fishman <avifishman70@gmail.com>,
	Patrick Venture <venture@google.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Wolfram Sang <wsa@kernel.org>,
	kfting@nuvoton.com, Tali Perry <tali.perry@nuvoton.com>,
	Olof Johansson <olof@lixom.net>
Subject: Re: [PATCH v3 11/11] i2c: npcm: Support NPCM845
Date: Sun, 6 Mar 2022 15:33:20 +0200	[thread overview]
Message-ID: <CAP6Zq1iy0yNMemqDjrLu1F0rrRSDFhZ+SqdoOa9FyJDNL0ENXA@mail.gmail.com> (raw)
In-Reply-To: <YiDMX7pUqs/rLJUU@smile.fi.intel.com>

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

Hi Andy,

Thanks for your review,

On Thu, 3 Mar 2022 at 16:11, Andy Shevchenko <
andriy.shevchenko@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@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

[-- Attachment #2: Type: text/html, Size: 4046 bytes --]