Re: [PATCH 10/13] i2c: nomadik: support Mobileye EyeQ5 I2C controller

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello,

On Mon Feb 19, 2024 at 3:52 PM CET, Théo Lebrun wrote:
> On Mon Feb 19, 2024 at 3:35 PM CET, Linus Walleij wrote:
> > On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> >
> > > Add compatible for the integration of the same DB8500 IP block into the
> > > 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 located
> > >    in a shared register region called OLB. We access that memory region
> > >    using a syscon & regmap that gets passed as a phandle (mobileye,olb).
> > >
> > >    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éo Lebrun <theo.lebrun@bootlin.com>
> >
> > (...)
> >
> > > -               writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > > +               if (priv->has_32b_bus)
> > > +                       writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > > +               else
> > > +                       writeb(*priv->cli.buffer, priv->virtbase + I2C_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 happens
> > 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éo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com