| Message ID | 20201224112203.7174-1-nikita.shubin@maquefel.me |
|---|---|
| Headers | show |
| Series | gpio: ep93xx: convert to multi irqchips | expand |
On Thu, Dec 24, 2020 at 1:24 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > I was lucky enough to became an owner of some splendid piece's of > antiques called ts7250 based on the top of Cirrus Logic EP9302. > > I don't know what fate expects this hardware (it's not EOL it's just Not > recommended for new designs) but i wanted to share fixes in ep93xx gpio area. > > It seems ep93xx is deadly broken at current state usage of AB gpiochips > interrupts leads to deadlocks coused by irq_unmask/irq_mask recursions. I personally consider it quite nice to have support even for outdated hardware (reminds me about a recent LWN article about 32-bit architectures and a comment there how it could affect the environment if we drop them from being supported). So I fully support this series. -- With Best Regards, Andy Shevchenko
On Thu, Dec 24, 2020 at 1:23 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > Since gpiolib requires having separate irqchips for each gpiochip, we > need to add some we definetly need a separate one for F port, and we definitely > could combine gpiochip A and B into one - but this will break namespace > and logick. > > So despite 3 irqchips is a bit beefy we need a separate irqchip for each is a -> being a > interrupt capable port. > > - added separate irqchip for each iterrupt capable gpiochip interrupt > - dropped ep93xx_gpio_port (it wasn't working correct for port F anyway) > - moved irq registers into separate struct ep93xx_irq_chip, togather irq -> IRQ (everywhere) together > with regs current state > - reworked irq handle for ab gpiochips (through bit not tottaly sure this > is a correct thing to do) ab -> AB ? In the parentheses something like "I'm not totally sure that this is a correct thing to do, though". > - dropped has_irq and has_hierarchical_irq and added a simple index > which we rely on when adding irq's to gpiochip's IRQs to GPIO chips (It would be nice if you can spell check and proofread commit messages and comments in the code. ... > +struct ep93xx_irq_chip { > + void __iomem *int_type1; > + void __iomem *int_type2; > + void __iomem *eoi; > + void __iomem *en; > + void __iomem *debounce; > + void __iomem *status; This is a bit... overcomplicated. Can we rather use regmap API? > + u8 gpio_int_unmasked; > + u8 gpio_int_enabled; > + u8 gpio_int_type1; > + u8 gpio_int_type2; > + u8 gpio_int_debounce; > + struct irq_chip chip; > +}; ... > /* Port ordering is: A B F */ > +static const char *irq_chip_names[3] = {"gpio-irq-a", > + "gpio-irq-b", > + "gpio-irq-f"}; Can you use better pattern, ie. static const char * const foo[] = { ... }; (there are two things: splitting per lines and additional const)? ... > + ab_parent_irq = platform_get_irq(pdev, 0); Error check, please? Also, if it's an optional resource, use platform_get_irq_optional(). > + err = devm_request_irq(dev, ab_parent_irq, > + ep93xx_ab_irq_handler, > + IRQF_SHARED, eic->chip.name, gc); > + Redundant blank line. > + if (err) { > + dev_err(dev, "error requesting IRQ : %d\n", ab_parent_irq); > + return err; > + } ... > + girq->num_parents = 1; > + girq->parents = devm_kcalloc(dev, 1, > + sizeof(*girq->parents), > + GFP_KERNEL); Can be squeezed to less amount of LOCs. Also consider to use girq->num_parents as a parameter to devm_kcalloc(). > + if (!girq->parents) > + return -ENOMEM; ... > + girq->handler = handle_level_irq; Don't we want to mark them as bad by using handle_bad_irq() as default handler? ... > + /* > + * FIXME: convert this to use hierarchical IRQ support! > + * this requires fixing the root irqchip to be hierarchial. hierarchical > + */ ... > + girq->num_parents = 8; > + girq->parents = devm_kcalloc(dev, 8, > + sizeof(*girq->parents), > + GFP_KERNEL); As per above. > + Redundant blank line. > + if (!girq->parents) > + return -ENOMEM; ... > + /* Pick resources 1..8 for these IRQs */ > + for (i = 1; i <= 8; i++) > + girq->parents[i - 1] = platform_get_irq(pdev, i); I would rather like to see i + 1 as a parameter which is much easier to read and understand. > + for (i = 0; i < 8; i++) { Also in both cases replace 8 by ARRAY_SIZE() or predefined constant. > + gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i; > + irq_set_chip_data(gpio_irq, gc); > + irq_set_chip_and_handler(gpio_irq, > + girq->chip, > + handle_level_irq); > + irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST); > + } Okay, I see that this is in the original code. Consider them as suggestions for additional changes. And briefly looking into the rest of the code the recommendation is to split this and perhaps other patches to smaller logical pieces. Also, try to organize your series in groups each of them respectively represents the following 1) fixes (can be backported, usually contain Fixes tag to the culprit commit); 2) preparatory refactoring patches / cleanups; 3) new features. -- With Best Regards, Andy Shevchenko
Paging Hartley who I think uses this chip. On Thu, Dec 24, 2020 at 12:22 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > I was lucky enough to became an owner of some splendid piece's of > antiques called ts7250 based on the top of Cirrus Logic EP9302. > > I don't know what fate expects this hardware (it's not EOL it's just Not > recommended for new designs) but i wanted to share fixes in ep93xx gpio area. > > It seems ep93xx is deadly broken at current state usage of AB gpiochips > interrupts leads to deadlocks coused by irq_unmask/irq_mask recursions. > > Port F is not working at all: Yeah when I converted this gpio driver I did not have access to a hardware which made use of port F :( I used the SIM.One board but they all died on me, then I acquired another EP93xx board that I just didn't have time to test. The long term plan is to convert these boards to use device tree and multiplatform. Interested in the job? ;) Thanks for fixing up the GPIO chip, let's get the patches in shape and merge as fixes. Bartosz is handling the fixes right now, I'll try to review too! Yours, Linus Walleij
On Sat, Dec 26, 2020 at 6:34 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > I personally consider it quite nice to have support even for outdated > hardware (reminds me about a recent LWN article about 32-bit > architectures and a comment there how it could affect the environment > if we drop them from being supported). > > So I fully support this series. I had a lecture on the subject actually where EP93xx came up: https://docs.google.com/presentation/d/1pv9MTCDpb0EgzrzicA9JF3pdUL6EcUmKhTavk5_hOHs As seen it is considered a "fully developed product" and it would not surprise me if Cirrus is taping out new EP93xx chips even. This SoC is definitely on my shortlist of stuff we need to support. (I wish Cirrus would provide some manpower but ...) Yours, Linus Walleij
On Thu, Dec 24, 2020 at 12:22 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE. > > So we need to specify girq->first otherwise: > > "If device tree is used, then first_irq will be 0 and > irqs get mapped dynamically on the fly" > > And that's not the thing we want. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> We can only fix this properly once we convert the platform to device tree. (Along with making the irqchip hierarchical.) Yours, Linus Walleij
26.12.2020, 20:52, "Andy Shevchenko" <andy.shevchenko@gmail.com>: > On Thu, Dec 24, 2020 at 1:23 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: >> Since gpiolib requires having separate irqchips for each gpiochip, we >> need to add some we definetly need a separate one for F port, and we > > definitely > >> could combine gpiochip A and B into one - but this will break namespace >> and logick. >> >> So despite 3 irqchips is a bit beefy we need a separate irqchip for each > > is a -> being a > >> interrupt capable port. >> >> - added separate irqchip for each iterrupt capable gpiochip > > interrupt > >> - dropped ep93xx_gpio_port (it wasn't working correct for port F anyway) >> - moved irq registers into separate struct ep93xx_irq_chip, togather > > irq -> IRQ (everywhere) > > together > >> with regs current state >> - reworked irq handle for ab gpiochips (through bit not tottaly sure this >> is a correct thing to do) > > ab -> AB ? > > In the parentheses something like "I'm not totally sure that this is a > correct thing to do, though". > >> - dropped has_irq and has_hierarchical_irq and added a simple index >> which we rely on when adding irq's to gpiochip's > > IRQs to GPIO chips > > (It would be nice if you can spell check and proofread commit > messages and comments in the code. > > ... > >> +struct ep93xx_irq_chip { >> + void __iomem *int_type1; >> + void __iomem *int_type2; >> + void __iomem *eoi; >> + void __iomem *en; >> + void __iomem *debounce; >> + void __iomem *status; > > This is a bit... overcomplicated. > Can we rather use regmap API? > >> + u8 gpio_int_unmasked; >> + u8 gpio_int_enabled; >> + u8 gpio_int_type1; >> + u8 gpio_int_type2; >> + u8 gpio_int_debounce; >> + struct irq_chip chip; >> +}; > > ... > >> /* Port ordering is: A B F */ >> +static const char *irq_chip_names[3] = {"gpio-irq-a", >> + "gpio-irq-b", >> + "gpio-irq-f"}; > > Can you use better pattern, ie. > static const char * const foo[] = { > ... > }; > > (there are two things: splitting per lines and additional const)? > > ... > >> + ab_parent_irq = platform_get_irq(pdev, 0); > > Error check, please? > Also, if it's an optional resource, use platform_get_irq_optional(). > >> + err = devm_request_irq(dev, ab_parent_irq, >> + ep93xx_ab_irq_handler, >> + IRQF_SHARED, eic->chip.name, gc); > >> + > > Redundant blank line. > >> + if (err) { >> + dev_err(dev, "error requesting IRQ : %d\n", ab_parent_irq); >> + return err; >> + } > > ... > >> + girq->num_parents = 1; >> + girq->parents = devm_kcalloc(dev, 1, >> + sizeof(*girq->parents), >> + GFP_KERNEL); > > Can be squeezed to less amount of LOCs. Also consider to use > girq->num_parents as a parameter to devm_kcalloc(). > >> + if (!girq->parents) >> + return -ENOMEM; > > ... > >> + girq->handler = handle_level_irq; > > Don't we want to mark them as bad by using handle_bad_irq() as default handler? > > ... > >> + /* >> + * FIXME: convert this to use hierarchical IRQ support! >> + * this requires fixing the root irqchip to be hierarchial. > > hierarchical > >> + */ > > ... > >> + girq->num_parents = 8; >> + girq->parents = devm_kcalloc(dev, 8, >> + sizeof(*girq->parents), >> + GFP_KERNEL); > > As per above. > >> + > > Redundant blank line. > >> + if (!girq->parents) >> + return -ENOMEM; > > ... > >> + /* Pick resources 1..8 for these IRQs */ >> + for (i = 1; i <= 8; i++) >> + girq->parents[i - 1] = platform_get_irq(pdev, i); > > I would rather like to see i + 1 as a parameter which is much easier > to read and understand. > >> + for (i = 0; i < 8; i++) { > > Also in both cases replace 8 by ARRAY_SIZE() or predefined constant. > >> + gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i; >> + irq_set_chip_data(gpio_irq, gc); >> + irq_set_chip_and_handler(gpio_irq, >> + girq->chip, >> + handle_level_irq); >> + irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST); >> + } > > Okay, I see that this is in the original code. Consider them as > suggestions for additional changes. > > And briefly looking into the rest of the code the recommendation is to > split this and perhaps other patches to smaller logical pieces. > > Also, try to organize your series in groups each of them respectively > represents the following > 1) fixes (can be backported, usually contain Fixes tag to the culprit commit); > 2) preparatory refactoring patches / cleanups; > 3) new features. > > -- > With Best Regards, > Andy Shevchenko Thank you, Andy, i ll rework my RFC patch according to your advices and resubmit.
27.12.2020, 16:55, "Linus Walleij" <linus.walleij@linaro.org>: > Paging Hartley who I think uses this chip. > > On Thu, Dec 24, 2020 at 12:22 PM Nikita Shubin > <nikita.shubin@maquefel.me> wrote: > >> I was lucky enough to became an owner of some splendid piece's of >> antiques called ts7250 based on the top of Cirrus Logic EP9302. >> >> I don't know what fate expects this hardware (it's not EOL it's just Not >> recommended for new designs) but i wanted to share fixes in ep93xx gpio area. >> >> It seems ep93xx is deadly broken at current state usage of AB gpiochips >> interrupts leads to deadlocks coused by irq_unmask/irq_mask recursions. >> >> Port F is not working at all: > > Yeah when I converted this gpio driver I did not have access to a hardware > which made use of port F :( > > I used the SIM.One board but they all died on me, then I acquired another > EP93xx board that I just didn't have time to test. > > The long term plan is to convert these boards to use device tree and > multiplatform. Interested in the job? ;) > I had such a thought, but I was stopped by the delusion that no one is using this hardware anymore. Not promising anything right now - but such a job sounds like a real fun. > Thanks for fixing up the GPIO chip, let's get the patches in shape and > merge as fixes. Bartosz is handling the fixes right now, I'll try to review too! I am glad that this work is interesting not only for me. For this, I'm willing to put in a little effort to make the proposed changes look decent. > > Yours, > Linus Walleij
On Mon, Dec 28, 2020 at 4:14 PM <nikita.shubin@maquefel.me> wrote: > I had such a thought, but I was stopped by the delusion that no one is using this > hardware anymore. It is definately *used* a lot. It is dubious if it is used with the latest kernel. Hartley was/is using it on engraving machinery and using the latest kernel, I think his setup is pretty large. The number of deployed machines using EP93xx is likely large, but more importantly, as with MOXA Art it is likely to be internet connected and managing vital control systems. In short: nobody may be upgrading these kernels, but the definately *should* be upgrading them. For security reasons. So as a community hat I have concluded it would be socially unacceptable to delete it and not offer an upgrade path for these systems the day they realize "oh s.... that is an internet connected pressure boiler". So if possible it should be modernized. > Not promising anything right now - but such a job sounds like a real fun. I would be grateful. Yours, Linus Walleij