Instead of overloading the name field, use the relevant callback to
output the device name.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
index 5be9866c2b3c..f29d9ccf858b 100644
--- a/drivers/pinctrl/pinctrl-starfive.c
+++ b/drivers/pinctrl/pinctrl-starfive.c
@@ -15,6 +15,7 @@
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/reset.h>
+#include <linux/seq_file.h>
#include <linux/spinlock.h>
#include <linux/pinctrl/pinctrl.h>
@@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
return 0;
}
+static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
+{
+ struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
+
+ seq_printf(p, sfp->gc.label);
+}
+
static struct irq_chip starfive_irq_chip = {
.irq_ack = starfive_irq_ack,
.irq_mask = starfive_irq_mask,
.irq_mask_ack = starfive_irq_mask_ack,
.irq_unmask = starfive_irq_unmask,
.irq_set_type = starfive_irq_set_type,
+ .irq_print_chip = starfive_irq_print_chip,
.flags = IRQCHIP_SET_TYPE_MASKED,
};
@@ -1307,8 +1316,6 @@ static int starfive_probe(struct platform_device *pdev)
sfp->gc.base = -1;
sfp->gc.ngpio = NR_GPIOS;
- starfive_irq_chip.name = sfp->gc.label;
-
sfp->gc.irq.chip = &starfive_irq_chip;
sfp->gc.irq.parent_handler = starfive_gpio_irq_handler;
sfp->gc.irq.num_parents = 1;
--
2.30.2
On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <[email protected]> wrote:
>
> Instead of overloading the name field, use the relevant callback to
> output the device name.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> index 5be9866c2b3c..f29d9ccf858b 100644
> --- a/drivers/pinctrl/pinctrl-starfive.c
> +++ b/drivers/pinctrl/pinctrl-starfive.c
> @@ -15,6 +15,7 @@
> #include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/reset.h>
> +#include <linux/seq_file.h>
> #include <linux/spinlock.h>
>
> #include <linux/pinctrl/pinctrl.h>
> @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> return 0;
> }
>
> +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> +{
> + struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> +
> + seq_printf(p, sfp->gc.label);
> +}
> +
> static struct irq_chip starfive_irq_chip = {
> .irq_ack = starfive_irq_ack,
> .irq_mask = starfive_irq_mask,
> .irq_mask_ack = starfive_irq_mask_ack,
> .irq_unmask = starfive_irq_unmask,
> .irq_set_type = starfive_irq_set_type,
> + .irq_print_chip = starfive_irq_print_chip,
> .flags = IRQCHIP_SET_TYPE_MASKED,
> };
The parent interrupt doesn't show up in /proc/interrupts anyway, so if
setting the name is considered abuse we can just drop the addition
above and just delete the two lines below.
The gpio framework seems to fill in default handlers in the struct
above, so unfortunately it can't yet be made const. Is this something
you intend to fix in the future?
> @@ -1307,8 +1316,6 @@ static int starfive_probe(struct platform_device *pdev)
> sfp->gc.base = -1;
> sfp->gc.ngpio = NR_GPIOS;
>
> - starfive_irq_chip.name = sfp->gc.label;
> -
> sfp->gc.irq.chip = &starfive_irq_chip;
> sfp->gc.irq.parent_handler = starfive_gpio_irq_handler;
> sfp->gc.irq.num_parents = 1;
> --
> 2.30.2
>
On Wed, 09 Feb 2022 23:30:55 +0000,
Emil Renner Berthing <[email protected]> wrote:
>
> On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <[email protected]> wrote:
> >
> > Instead of overloading the name field, use the relevant callback to
> > output the device name.
> >
> > Signed-off-by: Marc Zyngier <[email protected]>
> > ---
> > drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> > index 5be9866c2b3c..f29d9ccf858b 100644
> > --- a/drivers/pinctrl/pinctrl-starfive.c
> > +++ b/drivers/pinctrl/pinctrl-starfive.c
> > @@ -15,6 +15,7 @@
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> > #include <linux/reset.h>
> > +#include <linux/seq_file.h>
> > #include <linux/spinlock.h>
> >
> > #include <linux/pinctrl/pinctrl.h>
> > @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> > return 0;
> > }
> >
> > +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > +{
> > + struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > +
> > + seq_printf(p, sfp->gc.label);
> > +}
> > +
> > static struct irq_chip starfive_irq_chip = {
> > .irq_ack = starfive_irq_ack,
> > .irq_mask = starfive_irq_mask,
> > .irq_mask_ack = starfive_irq_mask_ack,
> > .irq_unmask = starfive_irq_unmask,
> > .irq_set_type = starfive_irq_set_type,
> > + .irq_print_chip = starfive_irq_print_chip,
> > .flags = IRQCHIP_SET_TYPE_MASKED,
> > };
>
> The parent interrupt doesn't show up in /proc/interrupts anyway, so if
> setting the name is considered abuse we can just drop the addition
> above and just delete the two lines below.
Are you sure this never appears? Is there a another irqchip stacked on
top of this one? Could you please dump /sys/kernel/debug/irq/irqs/XX,
where XX is an interrupt number using one of these GPIO pins? Please
run it without this patch, as I just noticed that debugfs blindly
uses the name.
> The gpio framework seems to fill in default handlers in the struct
> above, so unfortunately it can't yet be made const. Is this something
> you intend to fix in the future?
This is next on my list of things to address. The whole 'let's copy a
whole irqchip structure and hijack random pointers' should not have
happened, and it certainly is going to be an interesting ride.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
[resending, as I managed to royally screw up my initial email]
On Thu, 10 Feb 2022 14:14:19 +0000,
Emil Renner Berthing <[email protected]> wrote:
>
> On Thu, 10 Feb 2022 at 14:50, Marc Zyngier <[email protected]> wrote:
> > On Thu, 10 Feb 2022 13:44:12 +0000,
> > Emil Renner Berthing <[email protected]> wrote:
> > >
> > > Gotcha. The SoC has been out in very few numbers for less than a year
> > > and the driver only entered mainline in 5.17-rc1, so I doubt anyone
> > > has had time to write scripts that check for this, but I'll let it be
> > > up to you.
> >
> > Ah, I should have checked that. In which case, would you be OK if I
> > simply pushed the removal of this label as a fix for 5.17, and just
> > have it to say "Star5 GPIO", for example, without any indication of
> > the device (which appears in debugfs anyway as part of the irqdomain)?
>
> I'm fine with it although I'd prefer "StarFive GPIO". I haven't seen
> star5 used anywhere.
Fair enough.
> But shouldn't changes like this normally go through Linus Walleij's
> tree?
Either way, I don't mind. For the record, see below what I'm
suggesting we take in before 5.17-final.
Linus?
Thanks,
M.
From a84b83c32048de2ba72e5d05645eabc95ffabe49 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Thu, 10 Feb 2022 14:13:36 +0000
Subject: [PATCH] pinctrl: starfive: Use a static name for the GPIO irq_chip
Drop the device name used for the GPIO irq_chip and replace it
with something static. The information is still available from
debugfs and carried as part of the irqdomain.
Suggested-by: Emil Renner Berthing <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/pinctrl/pinctrl-starfive.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
index 0b912152a405..266da41a6162 100644
--- a/drivers/pinctrl/pinctrl-starfive.c
+++ b/drivers/pinctrl/pinctrl-starfive.c
@@ -1164,6 +1164,7 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
}
static struct irq_chip starfive_irq_chip = {
+ .name = "StarFive GPIO",
.irq_ack = starfive_irq_ack,
.irq_mask = starfive_irq_mask,
.irq_mask_ack = starfive_irq_mask_ack,
@@ -1308,7 +1309,6 @@ static int starfive_probe(struct platform_device *pdev)
sfp->gc.ngpio = NR_GPIOS;
starfive_irq_chip.parent_device = dev;
- starfive_irq_chip.name = sfp->gc.label;
sfp->gc.irq.chip = &starfive_irq_chip;
sfp->gc.irq.parent_handler = starfive_gpio_irq_handler;
--
2.34.1
--
Without deviation from the norm, progress is not possible.
--
Without deviation from the norm, progress is not possible.
On Thu, 10 Feb 2022 at 14:50, Marc Zyngier <[email protected]> wrote:
> On Thu, 10 Feb 2022 13:44:12 +0000,
> Emil Renner Berthing <[email protected]> wrote:
> >
> > On Thu, 10 Feb 2022 at 14:32, Marc Zyngier <[email protected]> wrote:
> > > On Thu, 10 Feb 2022 12:59:59 +0000,
> > > Emil Renner Berthing <[email protected]> wrote:
> > > >
> > > > On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <[email protected]> wrote:
> > > > > On Wed, 09 Feb 2022 23:30:55 +0000,
> > > > > Emil Renner Berthing <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <[email protected]> wrote:
> > > > > > >
> > > > > > > Instead of overloading the name field, use the relevant callback to
> > > > > > > output the device name.
> > > > > > >
> > > > > > > Signed-off-by: Marc Zyngier <[email protected]>
> > > > > > > ---
> > > > > > > drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> > > > > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> > > > > > > index 5be9866c2b3c..f29d9ccf858b 100644
> > > > > > > --- a/drivers/pinctrl/pinctrl-starfive.c
> > > > > > > +++ b/drivers/pinctrl/pinctrl-starfive.c
> > > > > > > @@ -15,6 +15,7 @@
> > > > > > > #include <linux/of.h>
> > > > > > > #include <linux/platform_device.h>
> > > > > > > #include <linux/reset.h>
> > > > > > > +#include <linux/seq_file.h>
> > > > > > > #include <linux/spinlock.h>
> > > > > > >
> > > > > > > #include <linux/pinctrl/pinctrl.h>
> > > > > > > @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> > > > > > > return 0;
> > > > > > > }
> > > > > > >
> > > > > > > +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > > > > > > +{
> > > > > > > + struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > > > > > > +
> > > > > > > + seq_printf(p, sfp->gc.label);
> > > > > > > +}
> > > > > > > +
> > > > > > > static struct irq_chip starfive_irq_chip = {
> > > > > > > .irq_ack = starfive_irq_ack,
> > > > > > > .irq_mask = starfive_irq_mask,
> > > > > > > .irq_mask_ack = starfive_irq_mask_ack,
> > > > > > > .irq_unmask = starfive_irq_unmask,
> > > > > > > .irq_set_type = starfive_irq_set_type,
> > > > > > > + .irq_print_chip = starfive_irq_print_chip,
> > > > > > > .flags = IRQCHIP_SET_TYPE_MASKED,
> > > > > > > };
> > > > > >
> > > > > > The parent interrupt doesn't show up in /proc/interrupts anyway, so if
> > > > > > setting the name is considered abuse we can just drop the addition
> > > > > > above and just delete the two lines below.
> > > > >
> > > > > Are you sure this never appears? Is there a another irqchip stacked on
> > > > > top of this one? Could you please dump /sys/kernel/debug/irq/irqs/XX,
> > > > > where XX is an interrupt number using one of these GPIO pins? Please
> > > > > run it without this patch, as I just noticed that debugfs blindly
> > > > > uses the name.
> > > >
> > > > Yes, the old gpio driver this derives from used to set
> > > > sfp->gc.irq.parent_handler = NULL
> > > > and then register its own irq handler, then the parent would show up
> > > > in /proc/interrupts. But after switching to letting the gpio framework
> > > > register the handler it stopped showing up.
> > >
> > > But this patch does not deal with the parent interrupt. It deals with
> > > the irqchip that is used for the 'children interrupt'. Output
> > > interrupts for a chained handler are never shown, as they don't really
> > > make much sense on their own (you'd only see the sum of the input
> > > interrupts).
> >
> > I see. Sorry for the confusion.
> >
> > > >
> > > > root@visionfive~# cat /proc/interrupts
> > > > CPU0 CPU1
> > > > 5: 5035 4907 RISC-V INTC 5 Edge riscv-timer
> > > > 6: 960 0 SiFive PLIC 4 Edge dw-mci
> > > > 7: 4384 0 SiFive PLIC 5 Edge dw-mci
> > > > 8: 562 0 SiFive PLIC 6 Edge eth0
> > > > 10: 1 0 SiFive PLIC 7 Edge eth0
> > > > 11: 0 0 SiFive PLIC 2 Edge dw_axi_dmac_platform
> > > > 15: 2690 0 SiFive PLIC 44 Edge xhci-hcd:usb1
> > > > 17: 0 0 SiFive PLIC 43 Edge 104c0000.usb
> > > > 18: 0 0 SiFive PLIC 1 Edge dw_axi_dmac_platform
> > > > 20: 234 0 SiFive PLIC 96 Edge 118b0000.i2c
> > > > 21: 0 0 SiFive PLIC 97 Edge 118c0000.i2c
> > > > 22: 7 0 SiFive PLIC 98 Edge 118d0000.trng
> > > > 28: 0 0 SiFive PLIC 101 Edge sf_lcdc
> > > > 29: 0 0 SiFive PLIC 103 Edge sf_vpp1
> > > > 30: 0 0 SiFive PLIC 70 Edge 12410000.spi
> > > > 31: 205 0 SiFive PLIC 73 Edge ttyS0
> > > > 32: 0 0 SiFive PLIC 74 Edge 12450000.i2c
> > > > 33: 0 0 SiFive PLIC 80 Edge 12480000.watchdog
> > > > 34: 28 0 SiFive PLIC 122 Edge 124a0000.tmon
> > > > 37: 0 0 11910000.pinctrl 35 Edge gpiomon
> > > ^^^^^^^^^^^^^^^^
> > > This is what this patch deals with. Going with your suggestion of
> > > dropping this output (or to hardcode it to something else) would be a
> > > userspace visible change, and we can't do that.
> >
> > Gotcha. The SoC has been out in very few numbers for less than a year
> > and the driver only entered mainline in 5.17-rc1, so I doubt anyone
> > has had time to write scripts that check for this, but I'll let it be
> > up to you.
>
> Ah, I should have checked that. In which case, would you be OK if I
> simply pushed the removal of this label as a fix for 5.17, and just
> have it to say "Star5 GPIO", for example, without any indication of
> the device (which appears in debugfs anyway as part of the irqdomain)?
I'm fine with it although I'd prefer "StarFive GPIO". I haven't seen
star5 used anywhere.
But shouldn't changes like this normally go through Linus Walleij's tree?
/Emil
On Thu, 10 Feb 2022 at 14:32, Marc Zyngier <[email protected]> wrote:
> On Thu, 10 Feb 2022 12:59:59 +0000,
> Emil Renner Berthing <[email protected]> wrote:
> >
> > On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <[email protected]> wrote:
> > > On Wed, 09 Feb 2022 23:30:55 +0000,
> > > Emil Renner Berthing <[email protected]> wrote:
> > > >
> > > > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <[email protected]> wrote:
> > > > >
> > > > > Instead of overloading the name field, use the relevant callback to
> > > > > output the device name.
> > > > >
> > > > > Signed-off-by: Marc Zyngier <[email protected]>
> > > > > ---
> > > > > drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> > > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> > > > > index 5be9866c2b3c..f29d9ccf858b 100644
> > > > > --- a/drivers/pinctrl/pinctrl-starfive.c
> > > > > +++ b/drivers/pinctrl/pinctrl-starfive.c
> > > > > @@ -15,6 +15,7 @@
> > > > > #include <linux/of.h>
> > > > > #include <linux/platform_device.h>
> > > > > #include <linux/reset.h>
> > > > > +#include <linux/seq_file.h>
> > > > > #include <linux/spinlock.h>
> > > > >
> > > > > #include <linux/pinctrl/pinctrl.h>
> > > > > @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > > > > +{
> > > > > + struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > > > > +
> > > > > + seq_printf(p, sfp->gc.label);
> > > > > +}
> > > > > +
> > > > > static struct irq_chip starfive_irq_chip = {
> > > > > .irq_ack = starfive_irq_ack,
> > > > > .irq_mask = starfive_irq_mask,
> > > > > .irq_mask_ack = starfive_irq_mask_ack,
> > > > > .irq_unmask = starfive_irq_unmask,
> > > > > .irq_set_type = starfive_irq_set_type,
> > > > > + .irq_print_chip = starfive_irq_print_chip,
> > > > > .flags = IRQCHIP_SET_TYPE_MASKED,
> > > > > };
> > > >
> > > > The parent interrupt doesn't show up in /proc/interrupts anyway, so if
> > > > setting the name is considered abuse we can just drop the addition
> > > > above and just delete the two lines below.
> > >
> > > Are you sure this never appears? Is there a another irqchip stacked on
> > > top of this one? Could you please dump /sys/kernel/debug/irq/irqs/XX,
> > > where XX is an interrupt number using one of these GPIO pins? Please
> > > run it without this patch, as I just noticed that debugfs blindly
> > > uses the name.
> >
> > Yes, the old gpio driver this derives from used to set
> > sfp->gc.irq.parent_handler = NULL
> > and then register its own irq handler, then the parent would show up
> > in /proc/interrupts. But after switching to letting the gpio framework
> > register the handler it stopped showing up.
>
> But this patch does not deal with the parent interrupt. It deals with
> the irqchip that is used for the 'children interrupt'. Output
> interrupts for a chained handler are never shown, as they don't really
> make much sense on their own (you'd only see the sum of the input
> interrupts).
I see. Sorry for the confusion.
> >
> > root@visionfive~# cat /proc/interrupts
> > CPU0 CPU1
> > 5: 5035 4907 RISC-V INTC 5 Edge riscv-timer
> > 6: 960 0 SiFive PLIC 4 Edge dw-mci
> > 7: 4384 0 SiFive PLIC 5 Edge dw-mci
> > 8: 562 0 SiFive PLIC 6 Edge eth0
> > 10: 1 0 SiFive PLIC 7 Edge eth0
> > 11: 0 0 SiFive PLIC 2 Edge dw_axi_dmac_platform
> > 15: 2690 0 SiFive PLIC 44 Edge xhci-hcd:usb1
> > 17: 0 0 SiFive PLIC 43 Edge 104c0000.usb
> > 18: 0 0 SiFive PLIC 1 Edge dw_axi_dmac_platform
> > 20: 234 0 SiFive PLIC 96 Edge 118b0000.i2c
> > 21: 0 0 SiFive PLIC 97 Edge 118c0000.i2c
> > 22: 7 0 SiFive PLIC 98 Edge 118d0000.trng
> > 28: 0 0 SiFive PLIC 101 Edge sf_lcdc
> > 29: 0 0 SiFive PLIC 103 Edge sf_vpp1
> > 30: 0 0 SiFive PLIC 70 Edge 12410000.spi
> > 31: 205 0 SiFive PLIC 73 Edge ttyS0
> > 32: 0 0 SiFive PLIC 74 Edge 12450000.i2c
> > 33: 0 0 SiFive PLIC 80 Edge 12480000.watchdog
> > 34: 28 0 SiFive PLIC 122 Edge 124a0000.tmon
> > 37: 0 0 11910000.pinctrl 35 Edge gpiomon
> ^^^^^^^^^^^^^^^^
> This is what this patch deals with. Going with your suggestion of
> dropping this output (or to hardcode it to something else) would be a
> userspace visible change, and we can't do that.
Gotcha. The SoC has been out in very few numbers for less than a year
and the driver only entered mainline in 5.17-rc1, so I doubt anyone
has had time to write scripts that check for this, but I'll let it be
up to you.
Reviewed-by: Emil Renner Berthing <[email protected]>
Tested-by: Emil Renner Berthing <[email protected]>
Thanks.
/Emil
On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <[email protected]> wrote:
> On Wed, 09 Feb 2022 23:30:55 +0000,
> Emil Renner Berthing <[email protected]> wrote:
> >
> > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <[email protected]> wrote:
> > >
> > > Instead of overloading the name field, use the relevant callback to
> > > output the device name.
> > >
> > > Signed-off-by: Marc Zyngier <[email protected]>
> > > ---
> > > drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> > > index 5be9866c2b3c..f29d9ccf858b 100644
> > > --- a/drivers/pinctrl/pinctrl-starfive.c
> > > +++ b/drivers/pinctrl/pinctrl-starfive.c
> > > @@ -15,6 +15,7 @@
> > > #include <linux/of.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/reset.h>
> > > +#include <linux/seq_file.h>
> > > #include <linux/spinlock.h>
> > >
> > > #include <linux/pinctrl/pinctrl.h>
> > > @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> > > return 0;
> > > }
> > >
> > > +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > > +{
> > > + struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > > +
> > > + seq_printf(p, sfp->gc.label);
> > > +}
> > > +
> > > static struct irq_chip starfive_irq_chip = {
> > > .irq_ack = starfive_irq_ack,
> > > .irq_mask = starfive_irq_mask,
> > > .irq_mask_ack = starfive_irq_mask_ack,
> > > .irq_unmask = starfive_irq_unmask,
> > > .irq_set_type = starfive_irq_set_type,
> > > + .irq_print_chip = starfive_irq_print_chip,
> > > .flags = IRQCHIP_SET_TYPE_MASKED,
> > > };
> >
> > The parent interrupt doesn't show up in /proc/interrupts anyway, so if
> > setting the name is considered abuse we can just drop the addition
> > above and just delete the two lines below.
>
> Are you sure this never appears? Is there a another irqchip stacked on
> top of this one? Could you please dump /sys/kernel/debug/irq/irqs/XX,
> where XX is an interrupt number using one of these GPIO pins? Please
> run it without this patch, as I just noticed that debugfs blindly
> uses the name.
Yes, the old gpio driver this derives from used to set
sfp->gc.irq.parent_handler = NULL
and then register its own irq handler, then the parent would show up
in /proc/interrupts. But after switching to letting the gpio framework
register the handler it stopped showing up.
root@visionfive~# cat /proc/interrupts
CPU0 CPU1
5: 5035 4907 RISC-V INTC 5 Edge riscv-timer
6: 960 0 SiFive PLIC 4 Edge dw-mci
7: 4384 0 SiFive PLIC 5 Edge dw-mci
8: 562 0 SiFive PLIC 6 Edge eth0
10: 1 0 SiFive PLIC 7 Edge eth0
11: 0 0 SiFive PLIC 2 Edge dw_axi_dmac_platform
15: 2690 0 SiFive PLIC 44 Edge xhci-hcd:usb1
17: 0 0 SiFive PLIC 43 Edge 104c0000.usb
18: 0 0 SiFive PLIC 1 Edge dw_axi_dmac_platform
20: 234 0 SiFive PLIC 96 Edge 118b0000.i2c
21: 0 0 SiFive PLIC 97 Edge 118c0000.i2c
22: 7 0 SiFive PLIC 98 Edge 118d0000.trng
28: 0 0 SiFive PLIC 101 Edge sf_lcdc
29: 0 0 SiFive PLIC 103 Edge sf_vpp1
30: 0 0 SiFive PLIC 70 Edge 12410000.spi
31: 205 0 SiFive PLIC 73 Edge ttyS0
32: 0 0 SiFive PLIC 74 Edge 12450000.i2c
33: 0 0 SiFive PLIC 80 Edge 12480000.watchdog
34: 28 0 SiFive PLIC 122 Edge 124a0000.tmon
37: 0 0 11910000.pinctrl 35 Edge gpiomon
IPI0: 52 133 Rescheduling interrupts
IPI1: 2591 6924 Function call interrupts
IPI2: 0 0 CPU stop interrupts
IPI3: 0 0 IRQ work interrupts
IPI4: 0 0 Timer broadcast interrupts
root@visionfive~# cat /sys/kernel/debug/irq/irqs/26
handler: starfive_gpio_irq_handler
device: (null)
status: 0x00010c00
_IRQ_NOPROBE
_IRQ_NOREQUEST
_IRQ_NOTHREAD
istate: 0x00000000
ddepth: 0
wdepth: 0
dstate: 0x02401200
IRQD_ACTIVATED
IRQD_IRQ_STARTED
IRQD_AFFINITY_SET
IRQD_DEFAULT_TRIGGER_SET
node: 0
affinity: 0-1
domain: :soc:interrupt-controller@c000000
hwirq: 0x20
chip: SiFive PLIC
flags: 0x0
root@visionfive~# cat /sys/kernel/debug/irq/irqs/37
handler: handle_edge_irq
device: (null)
status: 0x00000403
_IRQ_NOPROBE
istate: 0x00000020
IRQS_ONESHOT
ddepth: 0
wdepth: 0
dstate: 0x00400203
IRQ_TYPE_EDGE_RISING
IRQ_TYPE_EDGE_FALLING
IRQD_ACTIVATED
IRQD_IRQ_STARTED
node: 0
affinity: 0-1
domain: :soc:pinctrl@11910000
hwirq: 0x23
chip: 11910000.pinctrl
flags: 0x1
IRQCHIP_SET_TYPE_MASKED
> > The gpio framework seems to fill in default handlers in the struct
> > above, so unfortunately it can't yet be made const. Is this something
> > you intend to fix in the future?
>
> This is next on my list of things to address. The whole 'let's copy a
> whole irqchip structure and hijack random pointers' should not have
> happened, and it certainly is going to be an interesting ride.
Sounds great, thanks.
/Emil
On Thu, 10 Feb 2022 12:59:59 +0000,
Emil Renner Berthing <[email protected]> wrote:
>
> On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <[email protected]> wrote:
> > On Wed, 09 Feb 2022 23:30:55 +0000,
> > Emil Renner Berthing <[email protected]> wrote:
> > >
> > > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <[email protected]> wrote:
> > > >
> > > > Instead of overloading the name field, use the relevant callback to
> > > > output the device name.
> > > >
> > > > Signed-off-by: Marc Zyngier <[email protected]>
> > > > ---
> > > > drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> > > > index 5be9866c2b3c..f29d9ccf858b 100644
> > > > --- a/drivers/pinctrl/pinctrl-starfive.c
> > > > +++ b/drivers/pinctrl/pinctrl-starfive.c
> > > > @@ -15,6 +15,7 @@
> > > > #include <linux/of.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/reset.h>
> > > > +#include <linux/seq_file.h>
> > > > #include <linux/spinlock.h>
> > > >
> > > > #include <linux/pinctrl/pinctrl.h>
> > > > @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> > > > return 0;
> > > > }
> > > >
> > > > +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > > > +{
> > > > + struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > > > +
> > > > + seq_printf(p, sfp->gc.label);
> > > > +}
> > > > +
> > > > static struct irq_chip starfive_irq_chip = {
> > > > .irq_ack = starfive_irq_ack,
> > > > .irq_mask = starfive_irq_mask,
> > > > .irq_mask_ack = starfive_irq_mask_ack,
> > > > .irq_unmask = starfive_irq_unmask,
> > > > .irq_set_type = starfive_irq_set_type,
> > > > + .irq_print_chip = starfive_irq_print_chip,
> > > > .flags = IRQCHIP_SET_TYPE_MASKED,
> > > > };
> > >
> > > The parent interrupt doesn't show up in /proc/interrupts anyway, so if
> > > setting the name is considered abuse we can just drop the addition
> > > above and just delete the two lines below.
> >
> > Are you sure this never appears? Is there a another irqchip stacked on
> > top of this one? Could you please dump /sys/kernel/debug/irq/irqs/XX,
> > where XX is an interrupt number using one of these GPIO pins? Please
> > run it without this patch, as I just noticed that debugfs blindly
> > uses the name.
>
> Yes, the old gpio driver this derives from used to set
> sfp->gc.irq.parent_handler = NULL
> and then register its own irq handler, then the parent would show up
> in /proc/interrupts. But after switching to letting the gpio framework
> register the handler it stopped showing up.
But this patch does not deal with the parent interrupt. It deals with
the irqchip that is used for the 'children interrupt'. Output
interrupts for a chained handler are never shown, as they don't really
make much sense on their own (you'd only see the sum of the input
interrupts).
>
> root@visionfive~# cat /proc/interrupts
> CPU0 CPU1
> 5: 5035 4907 RISC-V INTC 5 Edge riscv-timer
> 6: 960 0 SiFive PLIC 4 Edge dw-mci
> 7: 4384 0 SiFive PLIC 5 Edge dw-mci
> 8: 562 0 SiFive PLIC 6 Edge eth0
> 10: 1 0 SiFive PLIC 7 Edge eth0
> 11: 0 0 SiFive PLIC 2 Edge dw_axi_dmac_platform
> 15: 2690 0 SiFive PLIC 44 Edge xhci-hcd:usb1
> 17: 0 0 SiFive PLIC 43 Edge 104c0000.usb
> 18: 0 0 SiFive PLIC 1 Edge dw_axi_dmac_platform
> 20: 234 0 SiFive PLIC 96 Edge 118b0000.i2c
> 21: 0 0 SiFive PLIC 97 Edge 118c0000.i2c
> 22: 7 0 SiFive PLIC 98 Edge 118d0000.trng
> 28: 0 0 SiFive PLIC 101 Edge sf_lcdc
> 29: 0 0 SiFive PLIC 103 Edge sf_vpp1
> 30: 0 0 SiFive PLIC 70 Edge 12410000.spi
> 31: 205 0 SiFive PLIC 73 Edge ttyS0
> 32: 0 0 SiFive PLIC 74 Edge 12450000.i2c
> 33: 0 0 SiFive PLIC 80 Edge 12480000.watchdog
> 34: 28 0 SiFive PLIC 122 Edge 124a0000.tmon
> 37: 0 0 11910000.pinctrl 35 Edge gpiomon
^^^^^^^^^^^^^^^^
This is what this patch deals with. Going with your suggestion of
dropping this output (or to hardcode it to something else) would be a
userspace visible change, and we can't do that.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Thu, 10 Feb 2022 at 15:34, Marc Zyngier <[email protected]> wrote:
>
> [resending, as I managed to royally screw up my initial email]
>
> On Thu, 10 Feb 2022 14:14:19 +0000,
> Emil Renner Berthing <[email protected]> wrote:
> >
> > On Thu, 10 Feb 2022 at 14:50, Marc Zyngier <[email protected]> wrote:
> > > On Thu, 10 Feb 2022 13:44:12 +0000,
> > > Emil Renner Berthing <[email protected]> wrote:
> > > >
> > > > Gotcha. The SoC has been out in very few numbers for less than a year
> > > > and the driver only entered mainline in 5.17-rc1, so I doubt anyone
> > > > has had time to write scripts that check for this, but I'll let it be
> > > > up to you.
> > >
> > > Ah, I should have checked that. In which case, would you be OK if I
> > > simply pushed the removal of this label as a fix for 5.17, and just
> > > have it to say "Star5 GPIO", for example, without any indication of
> > > the device (which appears in debugfs anyway as part of the irqdomain)?
> >
> > I'm fine with it although I'd prefer "StarFive GPIO". I haven't seen
> > star5 used anywhere.
>
> Fair enough.
>
> > But shouldn't changes like this normally go through Linus Walleij's
> > tree?
>
> Either way, I don't mind. For the record, see below what I'm
> suggesting we take in before 5.17-final.
Looks good to me. I don't mind which tree it goes through, just wanted
to make sure everyone's happy.
> Linus?
>
> Thanks,
>
> M.
>
> From a84b83c32048de2ba72e5d05645eabc95ffabe49 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <[email protected]>
> Date: Thu, 10 Feb 2022 14:13:36 +0000
> Subject: [PATCH] pinctrl: starfive: Use a static name for the GPIO irq_chip
>
> Drop the device name used for the GPIO irq_chip and replace it
> with something static. The information is still available from
> debugfs and carried as part of the irqdomain.
>
> Suggested-by: Emil Renner Berthing <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/pinctrl/pinctrl-starfive.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> index 0b912152a405..266da41a6162 100644
> --- a/drivers/pinctrl/pinctrl-starfive.c
> +++ b/drivers/pinctrl/pinctrl-starfive.c
> @@ -1164,6 +1164,7 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> }
>
> static struct irq_chip starfive_irq_chip = {
> + .name = "StarFive GPIO",
> .irq_ack = starfive_irq_ack,
> .irq_mask = starfive_irq_mask,
> .irq_mask_ack = starfive_irq_mask_ack,
> @@ -1308,7 +1309,6 @@ static int starfive_probe(struct platform_device *pdev)
> sfp->gc.ngpio = NR_GPIOS;
>
> starfive_irq_chip.parent_device = dev;
> - starfive_irq_chip.name = sfp->gc.label;
>
> sfp->gc.irq.chip = &starfive_irq_chip;
> sfp->gc.irq.parent_handler = starfive_gpio_irq_handler;
> --
> 2.34.1
>
>
> --
> Without deviation from the norm, progress is not possible.
>
> --
> Without deviation from the norm, progress is not possible.
On Thu, 10 Feb 2022 13:44:12 +0000,
Emil Renner Berthing <[email protected]> wrote:
>
> On Thu, 10 Feb 2022 at 14:32, Marc Zyngier <[email protected]> wrote:
> > On Thu, 10 Feb 2022 12:59:59 +0000,
> > Emil Renner Berthing <[email protected]> wrote:
> > >
> > > On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <[email protected]> wrote:
> > > > On Wed, 09 Feb 2022 23:30:55 +0000,
> > > > Emil Renner Berthing <[email protected]> wrote:
> > > > >
> > > > > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <[email protected]> wrote:
> > > > > >
> > > > > > Instead of overloading the name field, use the relevant callback to
> > > > > > output the device name.
> > > > > >
> > > > > > Signed-off-by: Marc Zyngier <[email protected]>
> > > > > > ---
> > > > > > drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> > > > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> > > > > > index 5be9866c2b3c..f29d9ccf858b 100644
> > > > > > --- a/drivers/pinctrl/pinctrl-starfive.c
> > > > > > +++ b/drivers/pinctrl/pinctrl-starfive.c
> > > > > > @@ -15,6 +15,7 @@
> > > > > > #include <linux/of.h>
> > > > > > #include <linux/platform_device.h>
> > > > > > #include <linux/reset.h>
> > > > > > +#include <linux/seq_file.h>
> > > > > > #include <linux/spinlock.h>
> > > > > >
> > > > > > #include <linux/pinctrl/pinctrl.h>
> > > > > > @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > > > > > +{
> > > > > > + struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > > > > > +
> > > > > > + seq_printf(p, sfp->gc.label);
> > > > > > +}
> > > > > > +
> > > > > > static struct irq_chip starfive_irq_chip = {
> > > > > > .irq_ack = starfive_irq_ack,
> > > > > > .irq_mask = starfive_irq_mask,
> > > > > > .irq_mask_ack = starfive_irq_mask_ack,
> > > > > > .irq_unmask = starfive_irq_unmask,
> > > > > > .irq_set_type = starfive_irq_set_type,
> > > > > > + .irq_print_chip = starfive_irq_print_chip,
> > > > > > .flags = IRQCHIP_SET_TYPE_MASKED,
> > > > > > };
> > > > >
> > > > > The parent interrupt doesn't show up in /proc/interrupts anyway, so if
> > > > > setting the name is considered abuse we can just drop the addition
> > > > > above and just delete the two lines below.
> > > >
> > > > Are you sure this never appears? Is there a another irqchip stacked on
> > > > top of this one? Could you please dump /sys/kernel/debug/irq/irqs/XX,
> > > > where XX is an interrupt number using one of these GPIO pins? Please
> > > > run it without this patch, as I just noticed that debugfs blindly
> > > > uses the name.
> > >
> > > Yes, the old gpio driver this derives from used to set
> > > sfp->gc.irq.parent_handler = NULL
> > > and then register its own irq handler, then the parent would show up
> > > in /proc/interrupts. But after switching to letting the gpio framework
> > > register the handler it stopped showing up.
> >
> > But this patch does not deal with the parent interrupt. It deals with
> > the irqchip that is used for the 'children interrupt'. Output
> > interrupts for a chained handler are never shown, as they don't really
> > make much sense on their own (you'd only see the sum of the input
> > interrupts).
>
> I see. Sorry for the confusion.
>
> > >
> > > root@visionfive~# cat /proc/interrupts
> > > CPU0 CPU1
> > > 5: 5035 4907 RISC-V INTC 5 Edge riscv-timer
> > > 6: 960 0 SiFive PLIC 4 Edge dw-mci
> > > 7: 4384 0 SiFive PLIC 5 Edge dw-mci
> > > 8: 562 0 SiFive PLIC 6 Edge eth0
> > > 10: 1 0 SiFive PLIC 7 Edge eth0
> > > 11: 0 0 SiFive PLIC 2 Edge dw_axi_dmac_platform
> > > 15: 2690 0 SiFive PLIC 44 Edge xhci-hcd:usb1
> > > 17: 0 0 SiFive PLIC 43 Edge 104c0000.usb
> > > 18: 0 0 SiFive PLIC 1 Edge dw_axi_dmac_platform
> > > 20: 234 0 SiFive PLIC 96 Edge 118b0000.i2c
> > > 21: 0 0 SiFive PLIC 97 Edge 118c0000.i2c
> > > 22: 7 0 SiFive PLIC 98 Edge 118d0000.trng
> > > 28: 0 0 SiFive PLIC 101 Edge sf_lcdc
> > > 29: 0 0 SiFive PLIC 103 Edge sf_vpp1
> > > 30: 0 0 SiFive PLIC 70 Edge 12410000.spi
> > > 31: 205 0 SiFive PLIC 73 Edge ttyS0
> > > 32: 0 0 SiFive PLIC 74 Edge 12450000.i2c
> > > 33: 0 0 SiFive PLIC 80 Edge 12480000.watchdog
> > > 34: 28 0 SiFive PLIC 122 Edge 124a0000.tmon
> > > 37: 0 0 11910000.pinctrl 35 Edge gpiomon
> > ^^^^^^^^^^^^^^^^
> > This is what this patch deals with. Going with your suggestion of
> > dropping this output (or to hardcode it to something else) would be a
> > userspace visible change, and we can't do that.
>
> Gotcha. The SoC has been out in very few numbers for less than a year
> and the driver only entered mainline in 5.17-rc1, so I doubt anyone
> has had time to write scripts that check for this, but I'll let it be
> up to you.
Ah, I should have checked that. In which case, would you be OK if I
simply pushed the removal of this label as a fix for 5.17, and just
have it to say "Star5 GPIO", for example, without any indication of
the device (which appears in debugfs anyway as part of the irqdomain)?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Thu, Feb 10, 2022 at 10:06 AM Marc Zyngier <[email protected]> wrote:
> On Wed, 09 Feb 2022 23:30:55 +0000,
> Emil Renner Berthing <[email protected]> wrote:
> > The gpio framework seems to fill in default handlers in the struct
> > above, so unfortunately it can't yet be made const. Is this something
> > you intend to fix in the future?
>
> This is next on my list of things to address. The whole 'let's copy a
> whole irqchip structure and hijack random pointers' should not have
> happened, and it certainly is going to be an interesting ride.
Sorry about that... Probably my bad idea. The only upside is that the
things that are ugly are centralized to one spot.
Yours,
Linus Walleij
On Thu, Feb 10, 2022 at 3:34 PM Marc Zyngier <[email protected]> wrote:
> Either way, I don't mind. For the record, see below what I'm
> suggesting we take in before 5.17-final.
>
> Linus?
I'm game! :)
Can you send a separate patch because b4 doesn't let me pick a patch
in the conversation
of a patch...
Yours,
Linus Walleij
On Fri, 11 Feb 2022 00:15:25 +0000,
Linus Walleij <[email protected]> wrote:
>
> On Thu, Feb 10, 2022 at 10:06 AM Marc Zyngier <[email protected]> wrote:
> > On Wed, 09 Feb 2022 23:30:55 +0000,
> > Emil Renner Berthing <[email protected]> wrote:
>
> > > The gpio framework seems to fill in default handlers in the struct
> > > above, so unfortunately it can't yet be made const. Is this something
> > > you intend to fix in the future?
> >
> > This is next on my list of things to address. The whole 'let's copy a
> > whole irqchip structure and hijack random pointers' should not have
> > happened, and it certainly is going to be an interesting ride.
>
> Sorry about that... Probably my bad idea. The only upside is that the
> things that are ugly are centralized to one spot.
No worries. I should have kept an eye on that too and spotted it
earlier. It is only when helping with the M1 GPIO driver that this was
brought to my attention.
My current approach is to move each and every driver to using the
existing helpers, and advertise to the core GPIO code that they don't
need to be fiddled with a new irqchip flag. It is a rather simple
change, but there are a lot of drivers (I have so far converted the
Apple, Qualcomm and Tegra186 drivers, as this is the HW I have
around), allowing their irq_chip structures to be made const and only
used by reference.
Once all drivers are converted (one day), we can drop the flag and the
pointer swapping code.
The alternative approach was to use a hierarchical irqchip provided by
the GPIO code, but this proved difficult as it would need to know
which methods to implement depending on the flow used. There are also
only a handful of drivers using the hierarchical mode anyway, and we'd
be stuck for all the other drivers.
Anyway, I'll post something shortly with the stuff I have changed, and
we can happily repaint that bike shed.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.