[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9d736f272aae42b154a4fdfbcadc1572ee82f516.camel@svanheule.net>
Date: Fri, 19 Mar 2021 22:20:22 +0100
From: Sander Vanheule <sander@...nheule.net>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Rob Herring <robh+dt@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Marc Zyngier <maz@...nel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Bert Vermeulen <bert@...t.com>
Subject: Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote:
> On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule
> <sander@...nheule.net> wrote:
> > On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <
> > > sander@...nheule.net> wrote:
>
> ...
>
> > > > +#include <linux/swab.h>
> > >
> > > Not sure why you need this? See below.
>
> > > > + return swab32(readl(ctrl->base +
> > > > REALTEK_GPIO_REG_ISR));
> > >
> > > Why swab?! How is this supposed to work on BE CPUs?
> > > Ditto for all swabXX() usage.
> >
> > My use of swab32/swahw32 has little to do with the CPU being BE or
> > LE,
> > but more with the register packing in the GPIO peripheral.
> >
> > The supported SoCs have port layout A-B-C-D in the registers, where
> > firmware built with Realtek's SDK always denotes A0 as the first
> > GPIO
> > line. So bit 24 in a register has the value for A0 (with the
> > exception
> > of the IMR register).
> >
> > I wrote these wrapper functions to be able to use the BIT() macro
> > with
> > the GPIO line number, similar to how gpio-mmio uses ioread32be()
> > when
> > the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used.
> >
> > For the IMR register, port A again comes first, but is now 16 bits
> > wide
> > instead of 8, with A0 at bits 16:17. That's why swahw32 is used for
> > this register.
> >
> > On the currently unsupported RTL9300-series, the port layout is
> > reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping
> > functions won't be required. When support for this alternate port
> > layout is added, some code will need to be added to differentiate
> > between the two cases.
>
> Yes, you have different endianess on the hardware level, why not to
> use the proper accessors (with or without utilization of the above
> mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)?
The point I was trying to make, is that it isn't an endianess issue. I
shouldn't have used a register with single byte values to try to
illustrate that.
Consider instead the interrupt masking registers. To write the IMR bits
for port A (GPIO 0-7), a 16-bit value must be written. This value (e.g.
u16 port_a_imr) is always BE, independent of the packing order of the
ports in the registers:
// On RTL8380: port A is in the upper word
writew(port_a_imr, base + OFFSET_IMR_AB);
// On RTL9300: port A is in the lower word
writew(port_a_imr, base + OFFSET_IMR_AB + 2);
I want the low GPIO lines to be in the lower half-word, so I can
manipulate GPIO lines 0-15 with simple mask and shift operations.
It just so happens, that all registers needed by bgpio_init contain
single-byte values. With BGPIO_BIG_ENDIAN_BYTE_ORDER the port order is
reversed as required, but it's a bit of a misnomer here.
Best,
Sander
Powered by blists - more mailing lists