| Message ID | CD4EBEF8-DDFD-40C3-A03E-7EC964B32357@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 12 Nov 2017 16:13:44 +0100 <kernel@martin.sperl.org> wrote: > Hi Marc! > > > On 12.11.2017, at 15:13, Marc Zyngier <marc.zyngier@arm.com> wrote: > > > > On Sun, 12 Nov 2017 13:32:44 +0100 > > <kernel@martin.sperl.org> wrote: > > > > Martin, > > > >> Hi! > >> > >> During the development of a new spi driver on a raspberry pi CM1 > >> I have seen an issue with the following code triggering strange behavior: > >> > >> ret = request_threaded_irq(spi->irq, NULL, > >> mcp2517fd_can_ist, > >> IRQF_ONESHOT | IRQF_TRIGGER_LOW, > >> DEVICE_NAME, priv); > >> > >> This works fine the first time the module is loaded (spi->irq is not 0), > >> but as soon as the module gets removed and reinstalled spi->irq is 0 > >> and I get the message in dmesg: > >> [ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio@7e200000! > >> > >> This does not happen when using the IRQF_TRIGGER_FALLING flag. > >> > >> in spi_drv_probe spi core does sets spi->dev to 0 in case > >> of_irq_get returns < 0; > >> > >> The specific code that triggers this message and return 0 is > >> irq_create_fwspec_mapping. > >> > >> After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html > >> and also checking for spi->irq == 0, I get: > >> > >> [ 87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio@7e200000! > > > > Well, you have the answer here: The interrupt has been configured with > > a falling edge trigger, while you're requesting a level low. Obviously, > > something is changing it. > > It was configured as level on the first install/request and the driver is > not changed between rmmod and insmod, so it again requests level on the > second request. > > > > > It would be interesting to see both the driver code and the DT file > > where the interrupt is described… > > The relevant patch to the device tree I am using: > --- a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts > +++ b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts > @@ -107,3 +107,38 @@ > pinctrl-0 = <&uart0_gpio14>; > status = "okay"; > }; > + > +&gpio { > + can0_pins: can0_pins { > + brcm,pins = <16>; > + brcm,function = <0>; /* input */ > + }; > +}; > + > +/ { > + can0_osc: can0_osc { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <4000000>; > + }; > +}; > + > +&spi { > + status = "okay"; > + > + can0: mcp2517fd@0 { > + reg = <0>; > + compatible = "microchip,mcp2517fd"; > + pinctrl-names = "default"; > + pinctrl-0 = <&can0_pins>; > + spi-max-frequency = <12500000>; > + interrupt-parent = <&gpio>; > + interrupts = <16 0x2>; This indicates a falling edge. No wonder the kernel is confused (I don't know why this isn't enforced the first time though, probably an issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should allow you to make some progress. Thanks, M.
> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier@arm.com> wrote: >> + >> + can0: mcp2517fd@0 { >> + reg = <0>; >> + compatible = "microchip,mcp2517fd"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&can0_pins>; >> + spi-max-frequency = <12500000>; >> + interrupt-parent = <&gpio>; >> + interrupts = <16 0x2>; > > This indicates a falling edge. No wonder the kernel is confused (I > don't know why this isn't enforced the first time though, probably an > issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should > allow you to make some progress. Thanks for the clarification - with that change it works! For a better understanding: Isn’t the interrupt type to use more of a driver decision than a HW implementation detail that needs to get defined in the device tree? In my case I probably could write some more code that would allow edge interrupts to work (without race-conditions on spi transfers - probably by using spi_async to reenable interrupts on the HW device), but it would not be as straight-forward and a bit more complex. Summary: Essentially the driver has to match the interrupt type - otherwise it will fail (on second initialization). Thanks, Martin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > +&spi { > > + status = "okay"; > > + > > + can0: mcp2517fd@0 { > > + reg = <0>; > > + compatible = "microchip,mcp2517fd"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&can0_pins>; > > + spi-max-frequency = <12500000>; > > + interrupt-parent = <&gpio>; > > + interrupts = <16 0x2>; > > This indicates a falling edge. No wonder the kernel is confused (I > don't know why this isn't enforced the first time though, probably an > issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should > allow you to make some progress. And using IRQ_TYPE_LEVEL_LOW, from interrupt-controller/irq.h would make it readable. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Nov 12 2017 at 5:49:39 pm GMT, <kernel@martin.sperl.org> wrote: >> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> + >>> + can0: mcp2517fd@0 { >>> + reg = <0>; >>> + compatible = "microchip,mcp2517fd"; >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&can0_pins>; >>> + spi-max-frequency = <12500000>; >>> + interrupt-parent = <&gpio>; >>> + interrupts = <16 0x2>; >> >> This indicates a falling edge. No wonder the kernel is confused (I >> don't know why this isn't enforced the first time though, probably an >> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should >> allow you to make some progress. > > Thanks for the clarification - with that change it works! > > For a better understanding: > Isn’t the interrupt type to use more of a driver decision than a > HW implementation detail that needs to get defined in the device tree? Absolutely *not*. The signalling of an interrupt is completely HW dependent, and the driver *must* use abide by the HW rule. That's because edge and level interrupts signal entirely different things: - An edge interrupt signals a an event. Something has happened, and many of these events can be signalled independently without the CPU doing anything. - A level interrupt indicates a change of state, and this state persist until the CPU has services the interrupt. That's the difference between receiving a SMS each time you pay something your bank card, and receiving a phone call from your bank because your account is overdrawn. > In my case I probably could write some more code that would allow edge > interrupts to work (without race-conditions on spi transfers - probably > by using spi_async to reenable interrupts on the HW device), but it > would not be as straight-forward and a bit more complex. And more or less wrong, given that the spec sheet calls out "active low" interrupts. > Summary: Essentially the driver has to match the interrupt type - > otherwise it will fail (on second initialization). And even that is not quite right. The driver should fail straight away. on the first request. I don't have the HW to track it down, but I'd appreciate it if you could have a look and enlighten me. Thanks, M.
--- a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts +++ b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts @@ -107,3 +107,38 @@ pinctrl-0 = <&uart0_gpio14>; status = "okay"; }; + +&gpio { + can0_pins: can0_pins { + brcm,pins = <16>; + brcm,function = <0>; /* input */ + }; +}; + +/ { + can0_osc: can0_osc { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <4000000>; + }; +}; + +&spi { + status = "okay"; + + can0: mcp2517fd@0 { + reg = <0>; + compatible = "microchip,mcp2517fd"; + pinctrl-names = "default"; + pinctrl-0 = <&can0_pins>; + spi-max-frequency = <12500000>; + interrupt-parent = <&gpio>; + interrupts = <16 0x2>; + clocks = <&can0_osc>; + microchip,clock_div = <1>; + microchip,clock_out_div = <4>; + microchip,gpio0_mode = <1>; + microchip,gpio1_mode = <1>; + status = "okay"; + }; +};