Re: [PATCH v1] irqchip/irq-mvebu-icu: Fix irq_set_type for sei and nsr

Stefan Eichenberger
2025-01-17T18:03:58Z

On Thu, Jan 16, 2025 at 10:05:25PM +0100, Thomas Gleixner wrote:
> Hi!
>
> On Thu, Jan 16 2025 at 18:12, Stefan Eichenberger wrote:
> > On Wed, Jan 15, 2025 at 09:15:24AM +0100, Thomas Gleixner wrote:
> >> And looking at the potential platform MSI providers for MVEBU, then it
> >> turns out that GICP and SEI both have the irq_set_type() callback
> >> populated, though ODMI has not. So either this has never worked or there
> >> is something else fishy.
> >>
> >> Can you please enable CONFIG_GENERIC_IRQ_DEBUGFS, build/boot a 6.10
> >> kernel and provide the output of
> >>
> >> cat /sys/kernel/debug/irq/irq/$N
> >>
> >> where $N is the interrupt number of the thermal sensor.
> >>
> >> Then provide the same information for a current kernel with your patch
> >> applied.
> >
> > You are right I somehow didn't look back far enough. I tested once with
> > kernel 6.12.5 and my patch applied:
>
> > root@localhost:~# uname -a
> > Linux localhost.localdomain 6.12.5+ #157 SMP PREEMPT Thu Jan 16 17:32:21 CET 2025 aarch64 GNU/Linux
> > root@localhost:~# cat /proc/interrupts |grep thermal
> > 35: 0 0 0 0 AP SEI 18 Level f06f8000.system-controller:thermal-sensor@80
> > 90: 0 0 0 0 SEI-ICU-SEI-f21e0000.interrupt-controller:inter 116 Edge f2400000.system-controller:thermal-sensor@70
> > 91: 0 0 0 0 SEI-ICU-SEI-f61e0000.interrupt-controller:inter 116 Edge f6400000.system-controller:thermal-sensor@70
> > root@localhost:~# cat /sys/kernel/debug/irq/irqs/90
> > handler: handle_edge_irq
> > device: f21e0000.interrupt-controller:interrupt-controller@50
> > status: 0x00000000
> > istate: 0x00004000
> > ddepth: 0
> > wdepth: 0
> > dstate: 0x02400204
> > IRQ_TYPE_LEVEL_HIGH
> > IRQD_ACTIVATED
> > IRQD_IRQ_STARTED
> > IRQD_DEFAULT_TRIGGER_SET
> > node: -1
> > affinity: 0-3
> > effectiv:
> > domain: :cp0:config-space@f2000000:interrupt-controller@1e0000:interrupt-controller@50-16
> > hwirq: 0x74
> > chip: SEI-ICU-SEI-f21e0000.interrupt-controller:inter
> > flags: 0x80
> > IRQCHIP_SUPPORTS_LEVEL_MSI
> > parent:
> > domain: :ap807:config-space@f0000000:interrupt-controller@3f0200-2
> > hwirq: 0x0
> > chip: CP SEI
>
> Ok. That explains it. CP SEI has:
>
> static int mvebu_sei_cp_set_type(struct irq_data *data, unsigned int type)
> {
> if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_EDGE_RISING)
> return -EINVAL;
> return 0;
> }
>
> But the DT configuration requests:
>
> > IRQ_TYPE_LEVEL_HIGH
>
> IRQ_TYPE_LEVEL_HIGH = 0x00000004,
>
> > genirq: Setting trigger mode 4 for irq 85 failed (irq_chip_set_type_parent+0x0/0x34)
>
> which is not supported by CP SEI.
>
> With your patch the type request is just stored in the data, but no
> actual type setting happens. That's default behaviour for chips which do
> not have a set_type() callback.
>
> > root@localhost:~# cat /sys/kernel/debug/irq/irqs/90
> > handler: handle_edge_irq
> > device: (null)
> >
> > It seems with kernel 6.10 the controller device was not set correctly,
> > probably it was ignoring irq_set_type because of this.
>
> No. That's not related.
>
> > Do you by chance have an idea how to properly fix this or should I do
> > some more research?
>
> It's unclear to me how the 6.10 kernel survives that. Do you have a
> different device tree for those kernels?

I used the same device tree for the 6.10 kernel as I did for kernel
6.12. I didn't even recompile it. Unfortunately, I couldn't figure out
what exactly is different so far. However, on the 6.10 kernel I got the
following trace:
root@localhost:/sys/kernel/tracing# cat trace
# tracer: function_graph
#
# CPU DURATION FUNCTION CALLS
# | | | | | | |
1) | devm_request_threaded_irq() {
1) | request_threaded_irq() {
0) | __irq_set_trigger() {
0) | irq_chip_set_type_parent() {
0) | irq_chip_set_type_parent() {
0) 2.560 us | mvebu_sei_cp_set_type(); /* = 0x0 */
0) 8.640 us | } /* irq_chip_set_type_parent = 0x0 */
0) + 14.400 us | } /* irq_chip_set_type_parent = 0x0 */
0) + 24.320 us | } /* __irq_set_trigger = 0x0 */
0) * 13601.60 us | } /* request_threaded_irq = 0x0 */
0) * 13617.44 us | } /* devm_request_threaded_irq = 0x0 */

It seems that irq_chip_set_type_parent is not failing. By adding some
debug messages to kernel/irq/manage.c I found that irqd_get_trigger_type
returns IRQF_TRIGGER_RISING even though it should return
IRQF_TRIGGER_HIGH according to the device tree. Maybe this was fixed
between 6.10 and 6.12 but I need to analyze that again in more detail.
[ 91.069407] genirq: __setup_irq - kernel/irq/manage.c:1530, flags: 0x0
[ 91.076042] genirq: __setup_irq - kernel/irq/manage.c:1533, flags: 0x1

For a possible solution, should I just change the device tree so that
IRQF_TRIGGER_RISING is used instead of IRQF_TRIGGER_HIGH?

Regards,
Stefan