Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
From: Andy Shevchenko <hidden>
Date: 2021-03-19 21:25:45
Also in:
linux-gpio, lkml
On Fri, Mar 19, 2021 at 11:20 PM Sander Vanheule [off-list ref] wrote:
On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote:quoted
On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule [off-list ref] wrote:quoted
On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:quoted
On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule < sander@svanheule.net> wrote:
...
quoted
quoted
quoted
quoted
+ 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