Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210315082339.9787-1-sander@svanheule.net> <20210315190806.66762-1-sander@svanheule.net>
 <20210315190806.66762-3-sander@svanheule.net> <CAHp75Vc6aaDhVN7LzvLNQjuOPguz+nbfmfpZ7TZHK=fNjCRz8w@mail.gmail.com>
 <a7d410216d35ed2b3015bfdd8e21dafd9c42d9d4.camel@svanheule.net>
 <CAHp75VdrqE0kBwzK9Jk7pZGjyfFnhatfa8UY0z-3T1w1PrbAbw@mail.gmail.com> <9d736f272aae42b154a4fdfbcadc1572ee82f516.camel@svanheule.net>
In-Reply-To: <9d736f272aae42b154a4fdfbcadc1572ee82f516.camel@svanheule.net>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 19 Mar 2021 23:24:52 +0200
Message-ID: <CAHp75Vf=-WdJ3U6o1G-Xi4W7tkCzyij0FD3MNY1q0x6bkimxow@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
To:     Sander Vanheule <sander@svanheule.net>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Rob Herring <robh+dt@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Bert Vermeulen <bert@biot.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Mar 19, 2021 at 11:20 PM Sander Vanheule <sander@svanheule.net> wrote:
> On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote:
> > On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule
> > <sander@svanheule.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@svanheule.net> wrote:

...

> > > > > +       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.

How many registers (per GPIO / port) do you have?
Can you list them and show endianess of the data for each of them and
for old and new hardware (something like a 3 column table)?

-- 
With Best Regards,
Andy Shevchenko