Received-SPF: pass (google.com: domain of linux-kernel+bounces-71681-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Mon, 19 Feb 2024 17:27:56 +0100
Message-Id: <CZ973SP3M9PS.3GG13IHI2DNR4@bootlin.com>
To: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>, "Linus Walleij"
 <linus.walleij@linaro.org>
From: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>
Subject: Re: [PATCH 10/13] i2c: nomadik: support Mobileye EyeQ5 I2C
 controller
Cc: "Andi Shyti" <andi.shyti@kernel.org>, "Rob Herring"
 <robh+dt@kernel.org>, "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>,
 "Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
 <linux-arm-kernel@lists.infradead.org>, <linux-i2c@vger.kernel.org>,
 <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
 <linux-mips@vger.kernel.org>, "Gregory Clement"
 <gregory.clement@bootlin.com>, "Vladimir Kondratiev"
 <vladimir.kondratiev@mobileye.com>, "Thomas Petazzoni"
 <thomas.petazzoni@bootlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@mobileye.com>
References: <20240215-mbly-i2c-v1-0-19a336e91dca@bootlin.com>
 <20240215-mbly-i2c-v1-10-19a336e91dca@bootlin.com>
 <CACRpkdY4PtnWkAEa=8sHdx7zYXLVAsrqKEVJY9m7VqeG5h6ChQ@mail.gmail.com>
 <CZ952TBZGEVD.JYSAQGNL1ZAQ@bootlin.com>
In-Reply-To: <CZ952TBZGEVD.JYSAQGNL1ZAQ@bootlin.com>

Hello,

On Mon Feb 19, 2024 at 3:52 PM CET, Th=C3=A9o Lebrun wrote:
> On Mon Feb 19, 2024 at 3:35 PM CET, Linus Walleij wrote:
> > On Thu, Feb 15, 2024 at 5:52=E2=80=AFPM Th=C3=A9o Lebrun <theo.lebrun@b=
ootlin.com> wrote:
> >
> > > Add compatible for the integration of the same DB8500 IP block into t=
he
> > > Mobileye EyeQ5 platform. Two quirks are present:
> > >
> > >  - The memory bus only supports 32-bit accesses. One writeb() is done=
 to
> > >    fill the Tx FIFO which we replace with a writel().
> > >
> > >  - A register must be configured for the I2C speed mode; it is locate=
d
> > >    in a shared register region called OLB. We access that memory regi=
on
> > >    using a syscon & regmap that gets passed as a phandle (mobileye,ol=
b).
> > >
> > >    A two-bit enum per controller is written into the register; that
> > >    requires us to know the global index of the I2C
> > >    controller (mobileye,id).
> > >
> > > We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> > > headers.
> > >
> > > Signed-off-by: Th=C3=A9o Lebrun <theo.lebrun@bootlin.com>
> >
> > (...)
> >
> > > -               writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > > +               if (priv->has_32b_bus)
> > > +                       writel(*priv->cli.buffer, priv->virtbase + I2=
C_TFR);
> > > +               else
> > > +                       writeb(*priv->cli.buffer, priv->virtbase + I2=
C_TFR);
> >
> > Are the other byte accessors working flawlessly? I get the shivers.
> > If it's needed in one place I bet the others prefer 32bit access too.
>
> I see where your shivers come from; I'll investigate as I don't remember
> my conclusion from the time when I worked on this driver (a few months
> ago).
>
> > Further the MIPS is big-endian is it not? It feels that this just happe=
ns
> > to work because of byte order access? writel() is little-endian by
> > definition.
>
> Actually, no. Our platform is little-endian.
>
> The full story, summarised: the endianness of our cores in kernel and
> hypervisor mode is defined by a pin read at reset. User mode can toggle
> the endianness at runtime I believe, but that is not of our concern.
> Our endianness in kernel mode is little-endian because the pin in
> question is hardwired to the value meaning little-endian.
>
> > What happens if you replace all writeb():s with something like
> >
> > static void nmk_write_reg(struct nmk_i2c_dev *priv, u32 reg, u8 val)
> > {
> >     if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> >         writeb(val, priv->virtbase + reg + 3);
> >         // if this doesn't work then use writeb((u32)val,
> > priv->virtbase + reg) I guess
> >    else
> >         writeb(val, priv->virtbase + reg);
> > }
> >
> > and conversely for readb()?
>
> As mentionned above, big endian isn't the worry for us. I'll be checking
> the readb() calls found in i2c_irq_handler() though.

Follow up on this. It was working by luck.

 - writeb() are generating Store Byte (sb) instructions which are
   unsupported on the memory bus.

 - readb() are generating Load Doubleword (ld) instructions and not the
   expected Load Byte (lb). It explains why readb() are working.

To be safe I'll make sure to use readl() and writel() everywhere for our
compatible. There is one writeb() and three readb(). Only the writeb()
was covered by this V1.

Thanks,

--
Th=C3=A9o Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com