| Message ID | 20210128122123.25341-2-nikita.shubin@maquefel.me |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: ep93xx: fixes series patch | expand |
On Thu, Jan 28, 2021 at 2:21 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > The port F is index 2 not 5. > > ------------[ cut here ]------------ > kernel BUG at drivers/gpio/gpio-ep93xx.c:64! Perhaps you missed my message, please cut this to have only related information and not be so noisy! > Internal error: Oops - BUG: 0 [#1] ARM > Modules linked in: > CPU: 0 PID: 403 Comm: gpio-event-mon Not tainted 5.9.10-00011-ge93e9618628b-dirty #19 > Hardware name: Technologic Systems TS-72xx SBC > PC is at ep93xx_gpio_update_int_params+0x1c/0x80 > LR is at ep93xx_gpio_update_int_params+0x14/0x80 > pc : [<c03abc44>] lr : [<c03abc3c>] psr: 20000093 > sp : c158de00 ip : 00000000 fp : 00000001 > r10: c44154d4 r9 : 00000000 r8 : c4415020 > r7 : c04ef884 r6 : c051c842 r5 : c4415020 r4 : 00000005 > r3 : 00000000 r2 : 00000000 r1 : c04eb768 r0 : 00000008 > Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none > Control: 0000717f Table: 01684000 DAC: 00000051 > Process gpio-event-mon (pid: 403, stack limit = 0x(ptrval)) > Stack: (0xc158de00 to 0xc158e000) > de00: 00000005 00000002 c051c842 c0238dc0 c0238c98 c0238c98 c04ef874 00000000 > de20: 00000003 c04fcfcc 60000013 c04ef910 c04ef8d4 c00456f0 c04ef874 c15f1e00 > de40: 00000000 00000000 00000001 c0045d40 c15f1e00 c4400160 c0044ca8 c04ef8a8 > de60: 60000013 00000000 c15f1e00 c04ef874 c04ef884 00000001 c0235d70 c158b800 > de80: be825f0f c0045ec8 00000003 c158b800 c440aa00 be825bc8 00000003 00000001 > dea0: 00000000 c0236f00 c44ed3a0 c158b800 c45c2015 00000000 00000001 00000003 > dec0: 6f697067 6576652d 6d2d746e 00006e6f 00000000 00000000 00000000 00000000 > dee0: be825df4 c00abb0c c440c500 c00aabd4 c440c500 c528b840 c45c2010 c04e1228 > df00: 00000ff0 c4478d28 c030b404 be825bc8 c1550e20 00000003 c1550e20 c00c3388 > df20: c4478d28 c00c3d48 be825f0f c00abd00 c45c2000 c45c2000 c1550e20 c00bfea8 > df40: 00000003 c00b0714 00000000 c4450000 00000004 00000100 00000001 c04e1228 > df60: c158dfb0 ffffff9c 000231f8 00000003 00000142 c00b085c 00000000 c04e1228 > df80: 00000000 be825f0f 00000003 00000003 00000036 c00083c4 c158c000 00000000 > dfa0: be825f0f c00081e0 be825f0f 00000003 00000003 c030b404 be825bc8 00000000 > dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f > dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec 60000010 00000003 00000000 00000000 > [<c03abc44>] (ep93xx_gpio_update_int_params) from [<c0238dc0>] (ep93xx_gpio_irq_type+0x128/0x1c0) > [<c0238dc0>] (ep93xx_gpio_irq_type) from [<c00456f0>] (__irq_set_trigger+0x6c/0x128) > [<c00456f0>] (__irq_set_trigger) from [<c0045d40>] (__setup_irq+0x594/0x678) > [<c0045d40>] (__setup_irq) from [<c0045ec8>] (request_threaded_irq+0xa4/0x128) > [<c0045ec8>] (request_threaded_irq) from [<c0236f00>] (gpio_ioctl+0x300/0x4d8) > [<c0236f00>] (gpio_ioctl) from [<c00c3388>] (vfs_ioctl+0x24/0x3c) > [<c00c3388>] (vfs_ioctl) from [<c00c3d48>] (sys_ioctl+0xbc/0x768) > [<c00c3d48>] (sys_ioctl) from [<c00081e0>] (ret_fast_syscall+0x0/0x50) > Exception stack(0xc158dfa8 to 0xc158dff0) > dfa0: be825f0f 00000003 00000003 c030b404 be825bc8 00000000 > dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f > dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec > Code: e59f0060 ebfff3e1 e3540002 9a000000 (e7f001f2) > ---[ end trace 3f6544e133e9f5ae ]--- > > Fixes: fd935fc421e74 ("gpio: ep93xx: Do not pingpong irq numbers") > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> ... > +/* > + * F Port index in GPIOCHIP'S array is 5 > + * but we use index 2 for stored values and offsets > + */ > +#define EP93XX_GPIO_F_PORT_INDEX 5 Hmm... Why not to use an array with holes instead. ... > + if (port == EP93XX_GPIO_F_PORT_INDEX) > + port = 2; Sorry, but I'm not in favour of this as it adds confusion. See above for the potential way to solve.
Hello Nikita, On Thu, 2021-01-28 at 18:11 +0200, Andy Shevchenko wrote: > > +/* > > + * F Port index in GPIOCHIP'S array is 5 > > + * but we use index 2 for stored values and offsets > > + */ > > +#define EP93XX_GPIO_F_PORT_INDEX 5 > > Hmm... Why not to use an array with holes instead. > > ... > > > + if (port == EP93XX_GPIO_F_PORT_INDEX) > > + port = 2; > > Sorry, but I'm not in favour of this as it adds confusion. > See above for the potential way to solve. well, I was thinking the same yesterday. It just adds another level on confusion into the code, which even the author got wrong :) Array with holes would be more obvious, but one can also embedd the necessary values into struct ep93xx_gpio_bank.
Hi Nikita, On Thu, 2021-02-04 at 15:55 +0300, nikita.shubin@maquefel.me wrote: > I considered your offer of using array with holes. > > It looks pretty ugly to me, couse it leads to bloated arrays: > > static unsigned char gpio_int_unmasked[EP93XX_GPIO_CHIP_NUM]; > static unsigned char gpio_int_enabled[EP93XX_GPIO_CHIP_NUM]; > static unsigned char gpio_int_type1[EP93XX_GPIO_CHIP_NUM]; > static unsigned char gpio_int_type2[EP93XX_GPIO_CHIP_NUM]; > static unsigned char gpio_int_debounce[EP93XX_GPIO_CHIP_NUM]; > > /* Port ordering is: A B F */ > static const u8 int_type1_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x90, 0xac, 0x0, 0x0, 0x0, 0x4c }; > static const u8 int_type2_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x94, 0xb0, 0x0, 0x0, 0x0, 0x50 }; > static const u8 eoi_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x98, 0xb4, 0x0, 0x0, 0x0, 0x54 }; > static const u8 int_en_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x9c, 0xb8, 0x0, 0x0, 0x0, 0x58 }; > static const u8 int_debounce_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0xa8, 0xc4, 0x0, 0x0, 0x0, 0x64 }; > > Is this really the thing we want ? Even in this form it's less error-prone than to have two index-spaces, and hidden conversion from one numbering scheme to other. Alternatives that I see are: 1. https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html 2. Embedd the necessary values into struct ep93xx_gpio_bank. This option can probably simplify the handling of the names for irq chips as well. > 28.01.2021, 19:19, "Alexander Sverdlin" <alexander.sverdlin@gmail.com>: > > Hello Nikita, > > > > On Thu, 2021-01-28 at 18:11 +0200, Andy Shevchenko wrote: > > > > +/* > > > > + * F Port index in GPIOCHIP'S array is 5 > > > > + * but we use index 2 for stored values and offsets > > > > + */ > > > > +#define EP93XX_GPIO_F_PORT_INDEX 5 > > > > > > Hmm... Why not to use an array with holes instead. > > > > > > ... > > > > > > > + if (port == EP93XX_GPIO_F_PORT_INDEX) > > > > + port = 2; > > > > > > Sorry, but I'm not in favour of this as it adds confusion. > > > See above for the potential way to solve. > > > > well, I was thinking the same yesterday. It just adds another > > level on confusion into the code, which even the author got > > wrong :) > > > > Array with holes would be more obvious, but one can also embedd > > the necessary values into struct ep93xx_gpio_bank. > > > > -- > > Alexander Sverdlin. > > > > -- Alexander Sverdlin.
Hi Nikita, On Thu, 2021-02-04 at 17:00 +0300, nikita.shubin@maquefel.me wrote: > > I considered your offer of using array with holes. > > > > It looks pretty ugly to me, couse it leads to bloated arrays: > > > > static unsigned char gpio_int_unmasked[EP93XX_GPIO_CHIP_NUM]; > > static unsigned char gpio_int_enabled[EP93XX_GPIO_CHIP_NUM]; > > static unsigned char gpio_int_type1[EP93XX_GPIO_CHIP_NUM]; > > static unsigned char gpio_int_type2[EP93XX_GPIO_CHIP_NUM]; > > static unsigned char gpio_int_debounce[EP93XX_GPIO_CHIP_NUM]; > > > > /* Port ordering is: A B F */ > > static const u8 int_type1_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x90, 0xac, 0x0, 0x0, 0x0, 0x4c }; > > static const u8 int_type2_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x94, 0xb0, 0x0, 0x0, 0x0, 0x50 }; > > static const u8 eoi_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x98, 0xb4, 0x0, 0x0, 0x0, 0x54 }; > > static const u8 int_en_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x9c, 0xb8, 0x0, 0x0, 0x0, 0x58 }; > > static const u8 int_debounce_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0xa8, 0xc4, 0x0, 0x0, 0x0, 0x64 }; > > > > Is this really the thing we want ? > > Even in this form it's less error-prone than to have two > index-spaces, and hidden conversion from one numbering scheme > to other. > > Alternatives that I see are: > 1. > https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html > > 2. > Embedd the necessary values into struct ep93xx_gpio_bank. > This option can probably simplify the handling of the names > for irq chips as well. > Thank you very much for your comments, and how about a 3rd option ? : > > It also makes easier to add 'struct irqchip' in following patch. > struct ep93xx_gpio_irq_chip { > u8 irq_offset; > u8 int_unmasked; > u8 int_enabled; > u8 int_type1; > u8 int_type2; > u8 int_debounce; > }; > > struct ep93xx_gpio_chip { > struct gpio_chip gc; > struct ep93xx_gpio_irq_chip *eic; > }; > > struct ep93xx_gpio { > void __iomem *base; > struct ep93xx_gpio_chip gc[8]; > }; > > static const u8 int_register_offset[8] = { 0x90, 0xac, [5] = 0x4c }; > #define EP93XX_INT_TYPE1_OFFSET 0x00 > #define EP93XX_INT_TYPE2_OFFSET 0x04 > #define EP93XX_INT_EOI_OFFSET 0x08 > #define EP93XX_INT_EN_OFFSET 0x0c > #define EP93XX_INT_STATUS_OFFSET 0x10 > #define EP93XX_INT_RAW_STATUS_OFFSET 0x14 > #define EP93XX_INT_DEBOUNCE_OFFSET 0x18 Makes sense to me. -- Alexander Sverdlin.
diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c index 226da8df6f10..0d5a9a90cf48 100644 --- a/drivers/gpio/gpio-ep93xx.c +++ b/drivers/gpio/gpio-ep93xx.c @@ -39,6 +39,12 @@ struct ep93xx_gpio { struct gpio_chip gc[8]; }; +/* + * F Port index in GPIOCHIP'S array is 5 + * but we use index 2 for stored values and offsets + */ +#define EP93XX_GPIO_F_PORT_INDEX 5 + /************************************************************************* * Interrupt handling for EP93xx on-chip GPIOs *************************************************************************/ @@ -85,6 +91,9 @@ static int ep93xx_gpio_port(struct gpio_chip *gc) return 0; } + if (port == EP93XX_GPIO_F_PORT_INDEX) + port = 2; + return port; }
The port F is index 2 not 5. ------------[ cut here ]------------ kernel BUG at drivers/gpio/gpio-ep93xx.c:64! Internal error: Oops - BUG: 0 [#1] ARM Modules linked in: CPU: 0 PID: 403 Comm: gpio-event-mon Not tainted 5.9.10-00011-ge93e9618628b-dirty #19 Hardware name: Technologic Systems TS-72xx SBC PC is at ep93xx_gpio_update_int_params+0x1c/0x80 LR is at ep93xx_gpio_update_int_params+0x14/0x80 pc : [<c03abc44>] lr : [<c03abc3c>] psr: 20000093 sp : c158de00 ip : 00000000 fp : 00000001 r10: c44154d4 r9 : 00000000 r8 : c4415020 r7 : c04ef884 r6 : c051c842 r5 : c4415020 r4 : 00000005 r3 : 00000000 r2 : 00000000 r1 : c04eb768 r0 : 00000008 Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none Control: 0000717f Table: 01684000 DAC: 00000051 Process gpio-event-mon (pid: 403, stack limit = 0x(ptrval)) Stack: (0xc158de00 to 0xc158e000) de00: 00000005 00000002 c051c842 c0238dc0 c0238c98 c0238c98 c04ef874 00000000 de20: 00000003 c04fcfcc 60000013 c04ef910 c04ef8d4 c00456f0 c04ef874 c15f1e00 de40: 00000000 00000000 00000001 c0045d40 c15f1e00 c4400160 c0044ca8 c04ef8a8 de60: 60000013 00000000 c15f1e00 c04ef874 c04ef884 00000001 c0235d70 c158b800 de80: be825f0f c0045ec8 00000003 c158b800 c440aa00 be825bc8 00000003 00000001 dea0: 00000000 c0236f00 c44ed3a0 c158b800 c45c2015 00000000 00000001 00000003 dec0: 6f697067 6576652d 6d2d746e 00006e6f 00000000 00000000 00000000 00000000 dee0: be825df4 c00abb0c c440c500 c00aabd4 c440c500 c528b840 c45c2010 c04e1228 df00: 00000ff0 c4478d28 c030b404 be825bc8 c1550e20 00000003 c1550e20 c00c3388 df20: c4478d28 c00c3d48 be825f0f c00abd00 c45c2000 c45c2000 c1550e20 c00bfea8 df40: 00000003 c00b0714 00000000 c4450000 00000004 00000100 00000001 c04e1228 df60: c158dfb0 ffffff9c 000231f8 00000003 00000142 c00b085c 00000000 c04e1228 df80: 00000000 be825f0f 00000003 00000003 00000036 c00083c4 c158c000 00000000 dfa0: be825f0f c00081e0 be825f0f 00000003 00000003 c030b404 be825bc8 00000000 dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec 60000010 00000003 00000000 00000000 [<c03abc44>] (ep93xx_gpio_update_int_params) from [<c0238dc0>] (ep93xx_gpio_irq_type+0x128/0x1c0) [<c0238dc0>] (ep93xx_gpio_irq_type) from [<c00456f0>] (__irq_set_trigger+0x6c/0x128) [<c00456f0>] (__irq_set_trigger) from [<c0045d40>] (__setup_irq+0x594/0x678) [<c0045d40>] (__setup_irq) from [<c0045ec8>] (request_threaded_irq+0xa4/0x128) [<c0045ec8>] (request_threaded_irq) from [<c0236f00>] (gpio_ioctl+0x300/0x4d8) [<c0236f00>] (gpio_ioctl) from [<c00c3388>] (vfs_ioctl+0x24/0x3c) [<c00c3388>] (vfs_ioctl) from [<c00c3d48>] (sys_ioctl+0xbc/0x768) [<c00c3d48>] (sys_ioctl) from [<c00081e0>] (ret_fast_syscall+0x0/0x50) Exception stack(0xc158dfa8 to 0xc158dff0) dfa0: be825f0f 00000003 00000003 c030b404 be825bc8 00000000 dfc0: be825f0f 00000003 00000003 00000036 00000001 00000000 00022070 be825f0f dfe0: b6f2e4e0 be825bac 00010acc b6f2e4ec Code: e59f0060 ebfff3e1 e3540002 9a000000 (e7f001f2) ---[ end trace 3f6544e133e9f5ae ]--- Fixes: fd935fc421e74 ("gpio: ep93xx: Do not pingpong irq numbers") Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> --- v2->v3: Reworded commit message. --- drivers/gpio/gpio-ep93xx.c | 9 +++++++++ 1 file changed, 9 insertions(+)