diff mbox

[05/33] gpio: add generic single-register fixed-direction GPIO driver

Message ID 87twe0krym.fsf@belgarion.home
State New
Headers show

Commit Message

Robert Jarzmik Sept. 1, 2016, 7:19 a.m. UTC
Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Wed, Aug 31, 2016 at 09:49:38AM +0100, Russell King - ARM Linux wrote:
>> On Tue, Aug 30, 2016 at 11:32:16PM +0200, Robert Jarzmik wrote:
>> > Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>> > 
>> > > If you can wait a day or two, I'll push a branch out for everything in
>> > > all these multiple series.
>> > Sure, just ping me when you have something.
>> 
>> git://git.armlinux.org.uk/~rmk/linux-arm.git sa1100
>> 
>> should get you something suitable to test.  It's based on 4.7-rc3 plus
>> my fixes branch.
>> 
>> It would be great to have this tested on Lubbock, and get the PCMCIA
>> issues fixed.
Ok Russell, I run a first mini-test.

I used :
 - one trivial typo fix of your patches in [1]
 - one "make it better fix" in [2] to pass the pcmcia probe, without much
   thinking about it, just to have the probe finished

The result today is that my PCMCIA card inserted in Lubbock's slot is not
detected, but that's not really a surprise. As a matter of fact, I didn't look
much into it, the only data points I have are :
 - PCMCIA is probed now
   - this is based on dmesg in [3]
   - this is also based on ls /sys/bus/sa1111-rab/devices/1800
	lrwxrwxrwx    1 root     root             0 Jan  1 00:15 driver -> ../../../../bus/sa1111-rab/drivers/sa1111-pcmcia
	drwxr-xr-x    2 root     root             0 Jan  1 00:15 power
	lrwxrwxrwx    1 root     root             0 Jan  1 00:15 subsystem -> ../../../../bus/sa1111-rab
	-rw-r--r--    1 root     root          4096 Jan  1 00:15 uevent

 - PCMCIA sockets are not there
   - this is based on ls /sys/class/pcmcia_socket/ where there is nothing

 - MAX16xx gpio are not claimed, which is surprising
   - this is based on cat /sys/kernel/debug/gpio
	gpiochip0: GPIOs 0-84, gpio-pxa:
	gpiochip2: GPIOs 478-495, parent: platform/sa1111, sa1111:
	gpiochip1: GPIOs 496-511, lubbock:

I'm not familiar with drivers/pcmcia part of the kernel tree so I won't be fast
to go down to the problem, but I'll try in the next days. Maybe if I activate
debug logs in pcmcia related parts I'll find out more quickly what is happening.

> Maybe we can look at converting mainstone as well?
Yep.

> This follows the Cirrus code (also used by Lubbock.)  So, if we represent the
> MST_PCMCIA[01] registers as GPIOs, we can switch pxa2xx_mainstone.c to use the
> max1600.c code for power control.
Yes.

> There is a slight issue, which is that the interrupts can't be translated to
> an interrupt by gpio-reg, which will currently cause soc_common problems - but
> that's an easy fix, though leaves us with more code than I'd desire in
> pxa2xx_mainstone.c.  Maybe a solution there would be to have gpio-reg also
> take an array of interrupt numbers... not sure yet.
Normally these interrupts are already dealt with by
arch/arm/mach-pxa/pxa_cplds_irq.c, which adds an irqdomain for them. The missing
part is the "gpio to interrupt" translation part, right ? This is where your
array comes into play if I get your right, to be the input of the to_irq()
function of the gpiochip.

As your gpios are contiguous (0 .. 31), why an array instead of a simple offset
so that your translation is only a linear irq = gpio + offset ?

> For IrDA, it looks like it has the same transceiver as the assabet, so I've
> (already) patches to split out the gpio-based transceiver control from
> sa1100_ir - maybe we can re-use that in pxaficp_ir too.
Once PCMCIA is over, sure.

Cheers.

--
Robert

[1] Typo fix

Comments

Russell King (Oracle) Sept. 1, 2016, 9:27 a.m. UTC | #1
On Thu, Sep 01, 2016 at 09:19:13AM +0200, Robert Jarzmik wrote:
> The result today is that my PCMCIA card inserted in Lubbock's slot is not
> detected, but that's not really a surprise. As a matter of fact, I didn't
> look much into it, the only data points I have are :
>  - PCMCIA is probed now
>    - this is based on dmesg in [3]
>    - this is also based on ls /sys/bus/sa1111-rab/devices/1800
> 	lrwxrwxrwx    1 root     root             0 Jan  1 00:15 driver -> ../../../../bus/sa1111-rab/drivers/sa1111-pcmcia
> 	drwxr-xr-x    2 root     root             0 Jan  1 00:15 power
> 	lrwxrwxrwx    1 root     root             0 Jan  1 00:15 subsystem -> ../../../../bus/sa1111-rab
> 	-rw-r--r--    1 root     root          4096 Jan  1 00:15 uevent
> 
>  - PCMCIA sockets are not there
>    - this is based on ls /sys/class/pcmcia_socket/ where there is nothing
> 
>  - MAX16xx gpio are not claimed, which is surprising
>    - this is based on cat /sys/kernel/debug/gpio
> 	gpiochip0: GPIOs 0-84, gpio-pxa:
> 	gpiochip2: GPIOs 478-495, parent: platform/sa1111, sa1111:
> 	gpiochip1: GPIOs 496-511, lubbock:

It looks like:

(a) pcmcia_probe() in drivers/pcmcia/sa1111_generic.c doesn't check the
    return value from the platform specific init functions, meaning if
    they fail, the driver still binds.  (note: they return -ENODEV to
    indicate that they should skip to the next platform.)
(b) there is no clock provided for the sa1111 pcmcia device (aka "1800").
    This should be the same clock as pxa2xx-pcmcia.

> > There is a slight issue, which is that the interrupts can't be translated to
> > an interrupt by gpio-reg, which will currently cause soc_common problems - but
> > that's an easy fix, though leaves us with more code than I'd desire in
> > pxa2xx_mainstone.c.  Maybe a solution there would be to have gpio-reg also
> > take an array of interrupt numbers... not sure yet.
>
> Normally these interrupts are already dealt with by
> arch/arm/mach-pxa/pxa_cplds_irq.c, which adds an irqdomain for them. The missing
> part is the "gpio to interrupt" translation part, right ? This is where your
> array comes into play if I get your right, to be the input of the to_irq()
> function of the gpiochip.

Yes.

> As your gpios are contiguous (0 .. 31), why an array instead of a simple offset
> so that your translation is only a linear irq = gpio + offset ?

There isn't a linear translation here:

#define MST_PCMCIA_nIRQ         (1 << 10)  /* IRQ / ready signal */
#define MST_PCMCIA_nSPKR_BVD2   (1 << 9)   /* VDD sense / digital speaker */
#define MST_PCMCIA_nSTSCHG_BVD1 (1 << 8)   /* VDD sense / card status changed */#define MST_PCMCIA_nVS2         (1 << 7)   /* VSS voltage sense */
#define MST_PCMCIA_nVS1         (1 << 6)   /* VSS voltage sense */
#define MST_PCMCIA_nCD          (1 << 5)   /* Card detection signal */
#define MST_PCMCIA_RESET        (1 << 4)   /* Card reset signal */
#define MST_PCMCIA_PWR_MASK     (0x000f)   /* MAX1602 power-supply controls */

#define MAINSTONE_S0_CD_IRQ     MAINSTONE_IRQ(9)
#define MAINSTONE_S0_STSCHG_IRQ MAINSTONE_IRQ(10)
#define MAINSTONE_S0_IRQ        MAINSTONE_IRQ(11)
#define MAINSTONE_S1_CD_IRQ     MAINSTONE_IRQ(13)
#define MAINSTONE_S1_STSCHG_IRQ MAINSTONE_IRQ(14)
#define MAINSTONE_S1_IRQ        MAINSTONE_IRQ(15)

MST_PCMCIA_nCD		=> MAINSTONE_S0_CD_IRQ or MAINSTONE_S1_CD_IRQ
MST_PCMCIA_nSTSCHG_BVD1 => MAINSTONE_S0_STSCHG_IRQ or MAINSTONE_S1_STSCHG_IRQ
MST_PCMCIA_nIRQ		=> MAINSTONE_S0_IRQ or MAINSTONE_S1_IRQ

So they aren't linear, and every "gpio" doesn't have a corresponding
interrupt.

> [1] Typo fix
> diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
> index f034928b99a1..81a1de6fb46f 100644
> --- a/arch/arm/mach-pxa/lubbock.c
> +++ b/arch/arm/mach-pxa/lubbock.c
> @@ -13,7 +13,7 @@
>   */
>  #include <linux/clkdev.h>
>  #include <linux/gpio.h>
> -#include <linux/gpip/gpio-reg.h>
> +#include <linux/gpio/gpio-reg.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>

Fixed.

> [2] Kind-of fix for lubbock pcmcia probe
> >From 977c16201a752aac8a8fb2da1f4271795f0b2122 Mon Sep 17 00:00:00 2001
> From: Robert Jarzmik <robert.jarzmik@free.fr>
> Date: Thu, 1 Sep 2016 08:31:08 +0200
> Subject: [PATCH] pcmcia: lubbock: fix sockets configuration
> 
> On lubbock board, the probe of the driver crashes by dereferencing very
> early a platform_data structure which is not set, in
> pxa2xx_configure_sockets().
> 
> This patch blindly fixes it without any analysis as to know if it's the
> right fix or even if the fix doesn't break in suspend/resume.
> 
> The stack fixed is :
> [    0.244353] SA1111 Microprocessor Companion Chip: silicon revision 1, metal revision 1
> [    0.256321] sa1111 sa1111: Providing IRQ336-390
> [    0.340899] clocksource: Switched to clocksource oscr0
> [    0.472263] Unable to handle kernel NULL pointer dereference at virtual address 00000004
> [    0.480469] pgd = c0004000
> [    0.483432] [00000004] *pgd=00000000
> [    0.487105] Internal error: Oops: f5 [#1] ARM
> [    0.491497] Modules linked in:
> [    0.494650] CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc3-00080-g1aaa68426f0c-dirty #2068
> [    0.503229] Hardware name: Intel DBPXA250 Development Platform (aka Lubbock)
> [    0.510344] task: c3e42000 task.stack: c3e44000
> [    0.514984] PC is at pxa2xx_configure_sockets+0x4/0x24 (drivers/pcmcia/pxa2xx_base.c:227)
> [    0.520193] LR is at pcmcia_lubbock_init+0x1c/0x38
> [    0.525079] pc : [<c0247c30>]    lr : [<c02479b0>]    psr: a0000053
> [    0.525079] sp : c3e45e70  ip : 100019ff  fp : 00000000
> [    0.536651] r10: c0828900  r9 : c0434838  r8 : 00000000
> [    0.541953] r7 : c0820700  r6 : c0857b30  r5 : c3ec1400  r4 : c0820758
> [    0.548549] r3 : 00000000  r2 : 0000000c  r1 : c3c09c40  r0 : c3ec1400
> [    0.555154] Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
> [    0.562450] Control: 0000397f  Table: a0004000  DAC: 00000053
> [    0.568257] Process swapper (pid: 1, stack limit = 0xc3e44190)
> [    0.574154] Stack: (0xc3e45e70 to 0xc3e46000)
> [    0.578610] 5e60:                                     c4849800 00000000 c3ec1400 c024769c
> [    0.586928] 5e80: 00000000 c3ec140c c3c0ee0c c3ec1400 c3ec1434 c020c410 c3ec1400 c3ec1434
> [    0.595244] 5ea0: c0820700 c080b408 c0828900 c020c5f8 00000000 c0820700 c020c578 c020ac5c
> [    0.603560] 5ec0: c3e687cc c3e71e10 c0820700 00000000 c3c02de0 c020bae4 c03c62f7 c03c62f7
> [    0.611872] 5ee0: c3e68780 c0820700 c042e034 00000000 c043c440 c020cdec c080b408 00000005
> [    0.620188] 5f00: c042e034 c00096c0 c0034440 c01c730c 20000053 ffffffff 00000000 00000000
> [    0.628502] 5f20: 00000000 c3ffcb87 c3ffcb90 c00346ac c3e66ba0 c03f7914 00000092 00000005
> [    0.636811] 5f40: 00000005 c03f847c 00000091 c03f847c 00000000 00000005 c0434828 00000005
> [    0.645125] 5f60: c043482c 00000092 c043c440 c0828900 c0434838 c0418d2c 00000005 00000005
> [    0.653430] 5f80: 00000000 c041858c 00000000 c032e9f0 00000000 00000000 00000000 00000000
> [    0.661729] 5fa0: 00000000 c032e9f8 00000000 c000f0f0 00000000 00000000 00000000 00000000
> [    0.670020] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.678311] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [    0.686673] (pxa2xx_configure_sockets) from pcmcia_lubbock_init (/drivers/pcmcia/sa1111_lubbock.c:161)
> [    0.696026] (pcmcia_lubbock_init) from pcmcia_probe (/drivers/pcmcia/sa1111_generic.c:213)
> [    0.704358] (pcmcia_probe) from driver_probe_device (/drivers/base/dd.c:378 /drivers/base/dd.c:499)
> [    0.712848] (driver_probe_device) from __driver_attach (/./include/linux/device.h:983 /drivers/base/dd.c:733)
> [    0.721414] (__driver_attach) from bus_for_each_dev (/drivers/base/bus.c:313)
> [    0.729723] (bus_for_each_dev) from bus_add_driver (/drivers/base/bus.c:708)
> [    0.738036] (bus_add_driver) from driver_register (/drivers/base/driver.c:169)
> [    0.746185] (driver_register) from do_one_initcall (/init/main.c:778)
> [    0.754561] (do_one_initcall) from kernel_init_freeable (/init/main.c:843 /init/main.c:851 /init/main.c:869 /init/main.c:1016)
> [    0.763409] (kernel_init_freeable) from kernel_init (/init/main.c:944)
> [    0.771660] (kernel_init) from ret_from_fork (/arch/arm/kernel/entry-common.S:119)
> [ 0.779347] Code: c03c6305 c03c631e c03c632e e5903048 (e993000c)
> All code
> ========
>    0:	c03c6305 	eorsgt	r6, ip, r5, lsl #6
>    4:	c03c631e 	eorsgt	r6, ip, lr, lsl r3
>    8:	c03c632e 	eorsgt	r6, ip, lr, lsr #6
>    c:	e5903048 	ldr	r3, [r0, #72]	; 0x48
>   10:*	e993000c 	ldmib	r3, {r2, r3}		<-- trapping instruction
> 
> Code starting with the faulting instruction
> ===========================================
>    0:	e993000c 	ldmib	r3, {r2, r3}
> [    0.786319] ---[ end trace 5264be19ef367bea ]---
> [    0.791201] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [    0.791201]
> [    0.800441] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/pcmcia/pxa2xx_base.c    | 9 +++++----
>  drivers/pcmcia/pxa2xx_base.h    | 2 +-
>  drivers/pcmcia/sa1111_lubbock.c | 2 +-
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pcmcia/pxa2xx_base.c b/drivers/pcmcia/pxa2xx_base.c
> index 483f919e0d2e..91b5f5724cba 100644
> --- a/drivers/pcmcia/pxa2xx_base.c
> +++ b/drivers/pcmcia/pxa2xx_base.c
> @@ -214,9 +214,8 @@ pxa2xx_pcmcia_frequency_change(struct soc_pcmcia_socket *skt,
>  }
>  #endif
>  
> -void pxa2xx_configure_sockets(struct device *dev)
> +void pxa2xx_configure_sockets(struct device *dev, struct pcmcia_low_level *ops)
>  {
> -	struct pcmcia_low_level *ops = dev->platform_data;
>  	/*
>  	 * We have at least one socket, so set MECR:CIT
>  	 * (Card Is There)
> @@ -322,7 +321,7 @@ static int pxa2xx_drv_pcmcia_probe(struct platform_device *dev)
>  			goto err1;
>  	}
>  
> -	pxa2xx_configure_sockets(&dev->dev);
> +	pxa2xx_configure_sockets(&dev->dev, ops);
>  	dev_set_drvdata(&dev->dev, sinfo);
>  
>  	return 0;
> @@ -348,7 +347,9 @@ static int pxa2xx_drv_pcmcia_remove(struct platform_device *dev)
>  
>  static int pxa2xx_drv_pcmcia_resume(struct device *dev)
>  {
> -	pxa2xx_configure_sockets(dev);
> +	struct pcmcia_low_level *ops = (struct pcmcia_low_level *)dev->platform_data;
> +
> +	pxa2xx_configure_sockets(dev, ops);
>  	return 0;
>  }
>  
> diff --git a/drivers/pcmcia/pxa2xx_base.h b/drivers/pcmcia/pxa2xx_base.h
> index b609b45469ed..e58c7a415418 100644
> --- a/drivers/pcmcia/pxa2xx_base.h
> +++ b/drivers/pcmcia/pxa2xx_base.h
> @@ -1,4 +1,4 @@
>  int pxa2xx_drv_pcmcia_add_one(struct soc_pcmcia_socket *skt);
>  void pxa2xx_drv_pcmcia_ops(struct pcmcia_low_level *ops);
> -void pxa2xx_configure_sockets(struct device *dev);
> +void pxa2xx_configure_sockets(struct device *dev, struct pcmcia_low_level *ops);
>  
> diff --git a/drivers/pcmcia/sa1111_lubbock.c b/drivers/pcmcia/sa1111_lubbock.c
> index 9d5ffc71ae51..7c1f0f6cd578 100644
> --- a/drivers/pcmcia/sa1111_lubbock.c
> +++ b/drivers/pcmcia/sa1111_lubbock.c
> @@ -157,7 +157,7 @@ int pcmcia_lubbock_init(struct sa1111_dev *sadev)
>  
>  	if (machine_is_lubbock()) {
>  		pxa2xx_drv_pcmcia_ops(&lubbock_pcmcia_ops);
> -		pxa2xx_configure_sockets(&sadev->dev);
> +		pxa2xx_configure_sockets(&sadev->dev, &lubbock_pcmcia_ops);
>  		ret = sa1111_pcmcia_add(sadev, &lubbock_pcmcia_ops,
>  				pxa2xx_drv_pcmcia_add_one);
>  	}

I think the patch is sane - it's certainly saner than the current mess,
so I'll add it to my patch set, thanks.

> [    7.171675] atkbd serio0: keyboard reset failed on 0a00
> [    7.177087] ------------[ cut here ]------------
> [    7.181895] WARNING: CPU: 0 PID: 17 at kernel/irq/manage.c:527 enable_irq+0x58/0x70
> [    7.189606] Unbalanced enable for IRQ 357
> [    7.193662] Modules linked in:
> [    7.196854] CPU: 0 PID: 17 Comm: kworker/0:1 Not tainted 4.8.0-rc3-00080-g1aaa68426f0c-dirty #2070
> [    7.205878] Hardware name: Intel DBPXA250 Development Platform (aka Lubbock)
> [    7.213055] Workqueue: events_long serio_handle_event
> [    7.218386] [<c0015260>] (unwind_backtrace) from [<c00122f0>] (show_stack+0x10/0x14)
> [    7.226312] [<c00122f0>] (show_stack) from [<c001ea40>] (__warn+0xcc/0xf8)
> [    7.233334] [<c001ea40>] (__warn) from [<c001eaa0>] (warn_slowpath_fmt+0x34/0x44)
> [    7.240964] [<c001eaa0>] (warn_slowpath_fmt) from [<c0047d44>] (enable_irq+0x58/0x70)
> [    7.248935] [<c0047d44>] (enable_irq) from [<c0250df4>] (ps2_write+0x4c/0x78)
> [    7.256211] [<c0250df4>] (ps2_write) from [<c025107c>] (ps2_sendbyte+0x50/0x144)
> [    7.263746] [<c025107c>] (ps2_sendbyte) from [<c02513b0>] (__ps2_command+0xc0/0x3c4)
> [    7.271630] [<c02513b0>] (__ps2_command) from [<c02516d8>] (ps2_command+0x24/0x38)
> [    7.279389] [<c02516d8>] (ps2_command) from [<c0259968>] (atkbd_probe+0x60/0x140)
> [    7.287026] [<c0259968>] (atkbd_probe) from [<c025a950>] (atkbd_connect+0x130/0x238)
> [    7.294901] [<c025a950>] (atkbd_connect) from [<c0250ae4>] (serio_driver_probe+0x28/0x3c)
> [    7.303251] [<c0250ae4>] (serio_driver_probe) from [<c020c410>] (driver_probe_device+0x124/0x28c)
> [    7.312276] [<c020c410>] (driver_probe_device) from [<c020c5f8>] (__driver_attach+0x80/0xa4)
> [    7.320852] [<c020c5f8>] (__driver_attach) from [<c020ac5c>] (bus_for_each_dev+0x6c/0x90)
> [    7.329167] [<c020ac5c>] (bus_for_each_dev) from [<c0250984>] (serio_handle_event+0x154/0x1a0)
> [    7.337933] [<c0250984>] (serio_handle_event) from [<c0030638>] (process_one_work+0x1bc/0x300)
> [    7.346688] [<c0030638>] (process_one_work) from [<c0030d1c>] (worker_thread+0x2bc/0x3f8)
> [    7.355043] [<c0030d1c>] (worker_thread) from [<c0034da0>] (kthread+0xc4/0xd8)
> [    7.362418] [<c0034da0>] (kthread) from [<c000f0f0>] (ret_from_fork+0x14/0x24)
> [    7.369720] ---[ end trace 52001af966019111 ]---

Hmm, I've not seen that one, I'll look into it.
Robert Jarzmik Sept. 1, 2016, 9:58 p.m. UTC | #2
Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Thu, Sep 01, 2016 at 09:19:13AM +0200, Robert Jarzmik wrote:
> It looks like:
>
> (a) pcmcia_probe() in drivers/pcmcia/sa1111_generic.c doesn't check the
>     return value from the platform specific init functions, meaning if
>     they fail, the driver still binds.  (note: they return -ENODEV to
>     indicate that they should skip to the next platform.)
You're right, I submitted a patch for that, and I confirm it actually happens on
lubbock.

> (b) there is no clock provided for the sa1111 pcmcia device (aka "1800").
>     This should be the same clock as pxa2xx-pcmcia.
Again right in the spot.
I added temporarily a clock until I have a more complete understanding in
lubbock.c :
+	clk_add_alias(NULL, "1800", "SA1111_CLK", NULL);

With this, things look way better :
[    1.507480] pcmcia_socket pcmcia_socket1: pccard: PCMCIA card inserted into slot 1

I'm still investigating the new message errors:
[    0.479157] genirq: Setting trigger mode 3 for irq 387 failed (sa1111_type_highirq+0x0/0x6c)
[    0.488213] genirq: Setting trigger mode 3 for irq 389 failed (sa1111_type_highirq+0x0/0x6c)
[    0.507449] genirq: Setting trigger mode 3 for irq 388 failed (sa1111_type_highirq+0x0/0x6c)
[    0.516492] genirq: Setting trigger mode 3 for irq 390 failed (sa1111_type_highirq+0x0/0x6c)

Moreover, I have a bit of homework as I also see :
 - no SA1111 interrupts at all, especially nothing when I insert/remove my
   CompactFlash card
   This might be an effect of pxa_cplds_irqs.c I created, I must have a look.
 - cat /sys/class/pcmcia_socket/pcmcia_socket1/cis 
   cat: read error: Input/output error
   That will cost me a review of the memory timings registers MCIO1/MECR/xxx,
   the power lines, etc ...
 - cat /sys/class/pcmcia_socket/pcmcia_socket1/status 
slot     : 1
status   : SS_READY SS_DETECT SS_POWERON SS_3VCARD
csc_mask : SS_DETECT
cs_flags : SS_OUTPUT_ENA
Vcc      : 33
Vpp      : 33
IRQ      : 0 (386)

>> As your gpios are contiguous (0 .. 31), why an array instead of a simple offset
>> so that your translation is only a linear irq = gpio + offset ?
>
> There isn't a linear translation here:
...zip...
> MST_PCMCIA_nCD		=> MAINSTONE_S0_CD_IRQ or MAINSTONE_S1_CD_IRQ
> MST_PCMCIA_nSTSCHG_BVD1 => MAINSTONE_S0_STSCHG_IRQ or MAINSTONE_S1_STSCHG_IRQ
> MST_PCMCIA_nIRQ		=> MAINSTONE_S0_IRQ or MAINSTONE_S1_IRQ
>
> So they aren't linear, and every "gpio" doesn't have a corresponding
> interrupt.
Ah yes, too bad, it would have been so much simpler.

Cheers.
Russell King (Oracle) Sept. 1, 2016, 11:02 p.m. UTC | #3
On Thu, Sep 01, 2016 at 11:58:28PM +0200, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> > On Thu, Sep 01, 2016 at 09:19:13AM +0200, Robert Jarzmik wrote:
> > It looks like:
> >
> > (a) pcmcia_probe() in drivers/pcmcia/sa1111_generic.c doesn't check the
> >     return value from the platform specific init functions, meaning if
> >     they fail, the driver still binds.  (note: they return -ENODEV to
> >     indicate that they should skip to the next platform.)
> You're right, I submitted a patch for that, and I confirm it actually happens on
> lubbock.

That'll work fine for lubbock, but not the others (we can have several
of the others enabled on sa11x0 platforms - eg, badge4 with neponset.)

> > (b) there is no clock provided for the sa1111 pcmcia device (aka "1800").
> >     This should be the same clock as pxa2xx-pcmcia.
> Again right in the spot.
> I added temporarily a clock until I have a more complete understanding in
> lubbock.c :
> +	clk_add_alias(NULL, "1800", "SA1111_CLK", NULL);
> 
> With this, things look way better :
> [    1.507480] pcmcia_socket pcmcia_socket1: pccard: PCMCIA card inserted into slot 1

Yay!

> I'm still investigating the new message errors:
> [    0.479157] genirq: Setting trigger mode 3 for irq 387 failed (sa1111_type_highirq+0x0/0x6c)
> [    0.488213] genirq: Setting trigger mode 3 for irq 389 failed (sa1111_type_highirq+0x0/0x6c)
> [    0.507449] genirq: Setting trigger mode 3 for irq 388 failed (sa1111_type_highirq+0x0/0x6c)
> [    0.516492] genirq: Setting trigger mode 3 for irq 390 failed (sa1111_type_highirq+0x0/0x6c)

Ignore those for now - the old ARM IRQ stuff was silent on that, but genirq
is more noisy.  I should probably make the sa1111 irqchip handle the both-
edge case itself.

> Moreover, I have a bit of homework as I also see :
>  - no SA1111 interrupts at all, especially nothing when I insert/remove my
>    CompactFlash card
>    This might be an effect of pxa_cplds_irqs.c I created, I must have a look.

Do you get other SA1111 interrupts (eg, the PS/2 interrupts) delivered?

>  - cat /sys/class/pcmcia_socket/pcmcia_socket1/cis 
>    cat: read error: Input/output error
>    That will cost me a review of the memory timings registers MCIO1/MECR/xxx,
>    the power lines, etc ...

Hmm, on Neponset with a CF card inserted, I can cat that, and it reports
the CIS.  Any error messages in the kernel log?

>  - cat /sys/class/pcmcia_socket/pcmcia_socket1/status 
> slot     : 1
> status   : SS_READY SS_DETECT SS_POWERON SS_3VCARD
> csc_mask : SS_DETECT
> cs_flags : SS_OUTPUT_ENA
> Vcc      : 33
> Vpp      : 33
> IRQ      : 0 (386)

That looks hopeful, but the IRQ hasn't been properly assigned (probably
because no driver has bound to the card.)

> >> As your gpios are contiguous (0 .. 31), why an array instead of a simple offset
> >> so that your translation is only a linear irq = gpio + offset ?
> >
> > There isn't a linear translation here:
> ...zip...
> > MST_PCMCIA_nCD		=> MAINSTONE_S0_CD_IRQ or MAINSTONE_S1_CD_IRQ
> > MST_PCMCIA_nSTSCHG_BVD1 => MAINSTONE_S0_STSCHG_IRQ or MAINSTONE_S1_STSCHG_IRQ
> > MST_PCMCIA_nIRQ		=> MAINSTONE_S0_IRQ or MAINSTONE_S1_IRQ
> >
> > So they aren't linear, and every "gpio" doesn't have a corresponding
> > interrupt.
> Ah yes, too bad, it would have been so much simpler.

Indeed, but a tabular approach isn't that painful, especially if we
also insist on knowing an irqdomain as well, which relieves us of
having to know absolute interrupt numbers, which may end up being
dynamically allocated eventually.
Robert Jarzmik Sept. 2, 2016, 5:50 p.m. UTC | #4
Russell King - ARM Linux <linux@armlinux.org.uk> writes:

>> You're right, I submitted a patch for that, and I confirm it actually happens on
>> lubbock.
>
> That'll work fine for lubbock, but not the others (we can have several
> of the others enabled on sa11x0 platforms - eg, badge4 with neponset.)
Ah, I see. Let's drop the patch then, I won't be able to know what to return in
this function if 2 initialisations fail, nor if the first failure should be
fatal or not, nor if a rollback is possible after 1 success (badge4) and 1
failure (neponset).

> Ignore those for now - the old ARM IRQ stuff was silent on that, but genirq
> is more noisy.  I should probably make the sa1111 irqchip handle the both-
> edge case itself.
Ok.

>> Moreover, I have a bit of homework as I also see :
>>  - no SA1111 interrupts at all, especially nothing when I insert/remove my
>>    CompactFlash card
>>    This might be an effect of pxa_cplds_irqs.c I created, I must have a look.
>
> Do you get other SA1111 interrupts (eg, the PS/2 interrupts) delivered?
I see no other interrupts at all.
Actually I see no interrupt claimed in /proc/interrupts, where I would have
expected interrupt 305.
	cat /proc/interrupts
	           CPU0       
	 24:       1419        SC   8 Edge      gpio-0
	 25:          0        SC   9 Edge      gpio-1
	 26:          0        SC  10 Edge      gpio-mux
	 38:        118        SC  22 Edge      UART1
	 41:          0        SC  25 Edge      DMA
	 42:      40224        SC  26 Edge      ost0
	112:       1419      GPIO   0 Edge      pxa_cplds_irqs
	307:       1419  pxa_cplds   3 Edge      eth0
	387:          0  SA1111-h Edge      SA1111 PCMCIA card detect
	388:          0  SA1111-h Edge      SA1111 CF card detect
	389:          0  SA1111-h Edge      SA1111 PCMCIA BVD1
	390:          0  SA1111-h Edge      SA1111 CF BVD1
	Err:          0

Actually this leads me to think that this interrupt 305 is not "requested" nor
activated. I see in sa1111.c:506 :
  "irq_set_chained_handler_and_data(sachip->irq, sa1111_irq_handler, ..."
This puts in the handler and data, but I don't this is "enables" the interrupt,
right ?

I traced the code in the interrupt controller (pxa_cplds_irqs), the SA1111
physical line is not set (even with an AT/2 keyboard connected, and I don't
think anybody tried this AT/2 on a lubbock for a long time).

The interrupt is for sure masked, and therefore the SA1111 interrupts are not
fired. Even if they would have been enabled, the "pending interrupts register"
doesn't show any sa1111 interrupt pending, so there is something else.

As I don't know if "enable_irq()" in sa1111.c would be an option as I have no
sight on sa1111.c requirements, I would take any advice here.

> Hmm, on Neponset with a CF card inserted, I can cat that, and it reports
> the CIS.  Any error messages in the kernel log?
None.
I have an ever more suprising thing.
I tried again this morning, without changing a single line of code (excepting in
pxa_cplds_irqs.c) nor touching the CF card, and now it really rocks !!! :
hexdump -C /sys/class/pcmcia_socket/pcmcia_socket1/cis
hexdump -C /sys/class/pcmcia_socket/pcmcia_socket1/cis
00000000  01 04 df 79 01 ff 1c 04  02 db 01 ff 18 02 df 01  |...y............|
00000010  20 04 45 00 01 04 15 0b  04 01 00 43 46 43 41 52  | .E........CFCAR|
00000020  44 00 ff 21 02 04 01 22  02 01 01 22 03 02 0c 0f  |D..!..."..."....|
00000030  1a 05 01 03 00 02 0f 1b  08 c0 40 a1 01 55 08 00  |..........@..U..|
00000040  20 1b 06 00 01 21 b5 1e  4d 1b 0a c1 41 99 01 55  | ....!..M...A..U|
00000050  64 f0 ff ff 20 1b 06 01  01 21 b5 1e 4d 1b 0f c2  |d... ....!..M...|
00000060  41 99 01 55 ea 61 f0 01  07 f6 03 01 ee 20 1b 06  |A..U.a....... ..|
00000070  02 01 21 b5 1e 4d 1b 0f  c3 41 99 01 55 ea 61 70  |..!..M...A..U.ap|
00000080  01 07 76 03 01 ee 20 1b  06 03 01 21 b5 1e 4d 14  |..v... ....!..M.|
00000090  00                                                |.|

I think this proves that your patches related to lubbock pcmcia conversion to
max1602 is fully functional !

Only the interrupt part to fight a bit, and then I'll try to make a couple of
tests on the IRDA part.

Cheers.
Russell King (Oracle) Sept. 2, 2016, 6:56 p.m. UTC | #5
On Fri, Sep 02, 2016 at 07:50:35PM +0200, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> >> Moreover, I have a bit of homework as I also see :
> >>  - no SA1111 interrupts at all, especially nothing when I insert/remove my
> >>    CompactFlash card
> >>    This might be an effect of pxa_cplds_irqs.c I created, I must have a look.
> >
> > Do you get other SA1111 interrupts (eg, the PS/2 interrupts) delivered?
> I see no other interrupts at all.
> Actually I see no interrupt claimed in /proc/interrupts, where I would have
> expected interrupt 305.
> 	cat /proc/interrupts
> 	           CPU0       
> 	 24:       1419        SC   8 Edge      gpio-0
> 	 25:          0        SC   9 Edge      gpio-1
> 	 26:          0        SC  10 Edge      gpio-mux
> 	 38:        118        SC  22 Edge      UART1
> 	 41:          0        SC  25 Edge      DMA
> 	 42:      40224        SC  26 Edge      ost0
> 	112:       1419      GPIO   0 Edge      pxa_cplds_irqs
> 	307:       1419  pxa_cplds   3 Edge      eth0
> 	387:          0  SA1111-h Edge      SA1111 PCMCIA card detect
> 	388:          0  SA1111-h Edge      SA1111 CF card detect
> 	389:          0  SA1111-h Edge      SA1111 PCMCIA BVD1
> 	390:          0  SA1111-h Edge      SA1111 CF BVD1
> 	Err:          0
> 
> Actually this leads me to think that this interrupt 305 is not "requested" nor
> activated. I see in sa1111.c:506 :
>   "irq_set_chained_handler_and_data(sachip->irq, sa1111_irq_handler, ..."
> This puts in the handler and data, but I don't this is "enables" the interrupt,
> right ?

It should enable the interrupt - the end of that does:

        if (handle != handle_bad_irq && is_chained) {
                irq_settings_set_noprobe(desc);
                irq_settings_set_norequest(desc);
                irq_settings_set_nothread(desc);
                desc->action = &chained_action;
                irq_startup(desc, true);
        }

and indeed, I see that it gets enabled on Assabet.  That irq_startup()
should result in the irqchip's irq_startup, irq_enable, or irq_unmask
method being called.

So, that should result in the IRQ to which the sa1111 is connected
being unmasked.

Chained interrupts don't appear in /proc/interrupts.

> I traced the code in the interrupt controller (pxa_cplds_irqs), the SA1111
> physical line is not set (even with an AT/2 keyboard connected, and I don't
> think anybody tried this AT/2 on a lubbock for a long time).

Hmm.  Looking at pxa_cplds_irqs, that's trying to support the CPLD
interrupts using a very very old and inefficient technique.  The
whole point of the chained interrupt system is to avoid several
overheads in the system...  I'm curious why PXA moved away from that.

One of the problems is that we end up nesting irq_entry()/irq_exit()
which contains a lot of code, eg trying to run softirqs.  All that
stuff is pure overhead because we'll never get to run anything like
that in this path.

I'm also very concerned that the conversion is wrong.  The old code
has this comment in the irq_unmask function:

-       /* the irq can be acknowledged only if deasserted, so it's done here */
-       LUB_IRQ_SET_CLR &= ~(1 << lubbock_irq);

In other words, we "acknowledge" (really clear the latched status) of
the interrupt _after_ we've serviced the peripheral, not before.
The new code tries to clear down the interrupt when masking it - in
other words, before the peripheral handler has had a chance to clear
down its interrupt.

I suspect you're seeing about twice as many interrupt counts on your
ethernet interface because of this - you'll be entering the handler
once for the real interrupt, but because the clear-down was ineffective,
you end up re-entering it a second time when hopefully the peripheral
is no longer asserting the interrupt.

> The interrupt is for sure masked, and therefore the SA1111 interrupts are not
> fired. Even if they would have been enabled, the "pending interrupts register"
> doesn't show any sa1111 interrupt pending, so there is something else.

Masked where though - in the SA1111 or FPGA?

> As I don't know if "enable_irq()" in sa1111.c would be an option as I have no
> sight on sa1111.c requirements, I would take any advice here.

It shouldn't be required, as I say above, irq_set_chained_handler_and_data()
deals with unmasking the IRQ already.

> > Hmm, on Neponset with a CF card inserted, I can cat that, and it reports
> > the CIS.  Any error messages in the kernel log?
> None.
> I have an ever more suprising thing.
> I tried again this morning, without changing a single line of code (excepting in
> pxa_cplds_irqs.c) nor touching the CF card, and now it really rocks !!! :
> hexdump -C /sys/class/pcmcia_socket/pcmcia_socket1/cis
> hexdump -C /sys/class/pcmcia_socket/pcmcia_socket1/cis
> 00000000  01 04 df 79 01 ff 1c 04  02 db 01 ff 18 02 df 01  |...y............|
> 00000010  20 04 45 00 01 04 15 0b  04 01 00 43 46 43 41 52  | .E........CFCAR|
> 00000020  44 00 ff 21 02 04 01 22  02 01 01 22 03 02 0c 0f  |D..!..."..."....|
> 00000030  1a 05 01 03 00 02 0f 1b  08 c0 40 a1 01 55 08 00  |..........@..U..|
> 00000040  20 1b 06 00 01 21 b5 1e  4d 1b 0a c1 41 99 01 55  | ....!..M...A..U|
> 00000050  64 f0 ff ff 20 1b 06 01  01 21 b5 1e 4d 1b 0f c2  |d... ....!..M...|
> 00000060  41 99 01 55 ea 61 f0 01  07 f6 03 01 ee 20 1b 06  |A..U.a....... ..|
> 00000070  02 01 21 b5 1e 4d 1b 0f  c3 41 99 01 55 ea 61 70  |..!..M...A..U.ap|
> 00000080  01 07 76 03 01 ee 20 1b  06 03 01 21 b5 1e 4d 14  |..v... ....!..M.|
> 00000090  00                                                |.|
> 
> I think this proves that your patches related to lubbock pcmcia conversion to
> max1602 is fully functional !

That is good news...

> Only the interrupt part to fight a bit, and then I'll try to make a couple of
> tests on the IRDA part.

I've some patches to convert mainstone, but they're not ready yet...
Robert Jarzmik Sept. 2, 2016, 9:21 p.m. UTC | #6
Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Fri, Sep 02, 2016 at 07:50:35PM +0200, Robert Jarzmik wrote:
> It should enable the interrupt - the end of that does:
>
>         if (handle != handle_bad_irq && is_chained) {
>                 irq_settings_set_noprobe(desc);
>                 irq_settings_set_norequest(desc);
>                 irq_settings_set_nothread(desc);
>                 desc->action = &chained_action;
>                 irq_startup(desc, true);
>         }
>
> and indeed, I see that it gets enabled on Assabet.  That irq_startup()
> should result in the irqchip's irq_startup, irq_enable, or irq_unmask
> method being called.
In my case, irq_startup() is never called for irq 305, ie. LUBBOCK_SA1111_IRQ.
See deep down the mail for the cause ...

> So, that should result in the IRQ to which the sa1111 is connected
> being unmasked.
Hmm, it never happens to me ...

> Chained interrupts don't appear in /proc/interrupts.
Ah ok.

>> I traced the code in the interrupt controller (pxa_cplds_irqs), the SA1111
>> physical line is not set (even with an AT/2 keyboard connected, and I don't
>> think anybody tried this AT/2 on a lubbock for a long time).
>
> Hmm.  Looking at pxa_cplds_irqs, that's trying to support the CPLD
> interrupts using a very very old and inefficient technique.  The
> whole point of the chained interrupt system is to avoid several
> overheads in the system...  I'm curious why PXA moved away from that.
I think I traced that in the commit message of :
aa8d6b73ea33 ("ARM: pxa: pxa_cplds: add lubbock and mainstone IO")

The concern I had was that the former mechanism was not working anymore as the
chained handler was overwritten in gpio-pxa.c, and I wanted that gpio be
possibly probed at device initcall time.

> One of the problems is that we end up nesting irq_entry()/irq_exit()
> which contains a lot of code, eg trying to run softirqs.  All that
> stuff is pure overhead because we'll never get to run anything like
> that in this path.
>
> I'm also very concerned that the conversion is wrong.  The old code
> has this comment in the irq_unmask function:
>
> -       /* the irq can be acknowledged only if deasserted, so it's done here */
> -       LUB_IRQ_SET_CLR &= ~(1 << lubbock_irq);
>
> In other words, we "acknowledge" (really clear the latched status) of
> the interrupt _after_ we've serviced the peripheral, not before.
> The new code tries to clear down the interrupt when masking it - in
> other words, before the peripheral handler has had a chance to clear
> down its interrupt.

I will check it more carefully. I remember having made tests, and cross-checked
the user manual of the Cotulla Development board, chapter 3.4 "Managing
Peripheral Interrupts", where there is no restriction on when the latched status
can be cleared.

I understand the concern that clearing the status before the interrupt source is
serviced can re-set the status during the peripheral handler, if it's a level
interrupt.

Maybe it worked so far by sheer luck, or because the ethernet card was the only
really tested interrupt.

> I suspect you're seeing about twice as many interrupt counts on your
> ethernet interface because of this - you'll be entering the handler
> once for the real interrupt, but because the clear-down was ineffective,
> you end up re-entering it a second time when hopefully the peripheral
> is no longer asserting the interrupt.
Mmm I will check, but see 2 cat /proc/interrupts in a row :
307:       9384  pxa_cplds   3 Edge      eth0
307:       9417  pxa_cplds   3 Edge      eth0

The difference is not a multiple of 2. I will check anyway.

>> The interrupt is for sure masked, and therefore the SA1111 interrupts are not
>> fired. Even if they would have been enabled, the "pending interrupts register"
>> doesn't show any sa1111 interrupt pending, so there is something else.
>
> Masked where though - in the SA1111 or FPGA?
In the FPGA.

>> As I don't know if "enable_irq()" in sa1111.c would be an option as I have no
>> sight on sa1111.c requirements, I would take any advice here.
>
> It shouldn't be required, as I say above, irq_set_chained_handler_and_data()
> deals with unmasking the IRQ already.
Ah I think I know what happens. The trick is that when this
irq_set_chained_handler_and_data() is called, the irq_desc for irq 305 is not
yet allocated (ie. is NULL). And it is not allocated because
pxa_cplds_irqs.cplds_probe() was not yet called, and had not allocated the irq
descriptors.

I would bet that on neponset the input interrupt to the SA1111 is within
0..NR_IRQS, which is not the case on lubbock anymore since the pxa_cplds_irqs
conversion.

It looks that I have an ordering problem :
 - I want gpio-pxa.probe() to be called at device initcall time
 - pxa_cplds_irqs.probe() cannot complete before gpio-pxa.probe() because it
   needs GPIO0 as its interrupt source
   => it might need to honor -EPROBE_DEFER
 - sa1111.sa1111_probe() cannot complete before pxa_cplds_irqs.probe() because
   it needs IRQ305 as its source
   => it might need to honor -EPROBE_DEFER

I fail to see how the "optimized" irq chained handler can be used in a
device-tree type build, where I don't think the probe ordering is guaranteed.

Cheers.
Russell King (Oracle) Sept. 2, 2016, 11:34 p.m. UTC | #7
On Fri, Sep 02, 2016 at 11:21:12PM +0200, Robert Jarzmik wrote:
> Ah I think I know what happens. The trick is that when this
> irq_set_chained_handler_and_data() is called, the irq_desc for irq 305 is not
> yet allocated (ie. is NULL). And it is not allocated because
> pxa_cplds_irqs.cplds_probe() was not yet called, and had not allocated the irq
> descriptors.

Grr.

> I would bet that on neponset the input interrupt to the SA1111 is within
> 0..NR_IRQS, which is not the case on lubbock anymore since the pxa_cplds_irqs
> conversion.

It's not, but it's guaranteed to be allocated, because the SA1111 is
declared by neponset after ensuring that the resources are setup.

> It looks that I have an ordering problem :
>  - I want gpio-pxa.probe() to be called at device initcall time
>  - pxa_cplds_irqs.probe() cannot complete before gpio-pxa.probe() because it
>    needs GPIO0 as its interrupt source
>    => it might need to honor -EPROBE_DEFER
>  - sa1111.sa1111_probe() cannot complete before pxa_cplds_irqs.probe() because
>    it needs IRQ305 as its source
>    => it might need to honor -EPROBE_DEFER
> 
> I fail to see how the "optimized" irq chained handler can be used in a
> device-tree type build, where I don't think the probe ordering is guaranteed.

In a DT environment, platform_get_irq uses of_irq_get() to obtain its
interrupt, which uses irq_find_host() and irq_create_of_mapping() to
get the interrupt number.  The IRQ domain for this must exist.

In a DT build, the SA1111 platform device should be declared in the
DT using the normal IRQ properties, and that should cause
platform_get_irq() to return -EPROBE_DEFER if pxa_cplds_irqs hasn't
initialised.  So I think in general we're fine - where we're not is
we're not propagating the platform_get_irq() error code back (which
is an easy fix.)

The problem is then what to do with non-DT PXA builds to make sure
this works - the current order really doesn't work, and there's no
way afaics to find out whether an interrupt number passed through
as a platform resource is currently setup with handlers etc.  Even
can_request_irq() doesn't tell us that.

We could mess about with the init order to make sure the pxa cplds
stuff initialises earlier (eg, moving it to arch_initcall() rather
than being at device_initcall() time.)

I'm not willing to give up on the current interrupt handling structure
in SA1111 - it's the way it is with good reason, and with a great deal
of experience in getting it working right.  The SA1111 has some
behaviours to make it correctly work in an edge-triggered interrupt
environment that need proper handling of the upper levels of the
interrupt hierachy.

We _used_ to take this "request_irq" approach on ARM prior to my rewrite
of the ARM IRQ subsystem, and we had massive problems with lost SA1111
interrupts - particularly PCMCIA card interrupts.  That's actually the
whole reason why I rewrote the ARM interrupt code to have these chained
interrupt handlers, which is one of the basis of tglx's genirq code that
we now have.  They exist to allow not only a balanced, equal priority
IRQ handling (so any one IRQ can't starve all others if the handlers
are implemented correctly) but also to ensure that IRQs don't get lost,
while avoiding having lots of unnecessary interrupts caused by badly
handled edge IRQs.

The SA1111 is a device designed to be connected to an edge IRQ input,
and it will pulse its interrupt output whenever there's any active
interrupts and _either_ of the interrupt status/clear registers are
written.

The only way to achieve no lost interrupts and no spurious interrupts
is to have SA1111 as a chained interrupt handler and control the
parent at the appropriate points during SA1111 interrupt processing.
Russell King (Oracle) Sept. 3, 2016, 9:09 a.m. UTC | #8
On Thu, Sep 01, 2016 at 09:19:13AM +0200, Robert Jarzmik wrote:
> >From 977c16201a752aac8a8fb2da1f4271795f0b2122 Mon Sep 17 00:00:00 2001
> From: Robert Jarzmik <robert.jarzmik@free.fr>
> Date: Thu, 1 Sep 2016 08:31:08 +0200
> Subject: [PATCH] pcmcia: lubbock: fix sockets configuration
> 
> On lubbock board, the probe of the driver crashes by dereferencing very
> early a platform_data structure which is not set, in
> pxa2xx_configure_sockets().

Patch applied, with some fixups for the error code propagation changes
and edited the patch description a little.

Thanks.
Russell King (Oracle) Sept. 3, 2016, 9:15 a.m. UTC | #9
On Fri, Sep 02, 2016 at 11:21:12PM +0200, Robert Jarzmik wrote:
> It looks that I have an ordering problem :
>  - I want gpio-pxa.probe() to be called at device initcall time
>  - pxa_cplds_irqs.probe() cannot complete before gpio-pxa.probe() because it
>    needs GPIO0 as its interrupt source
>    => it might need to honor -EPROBE_DEFER
>  - sa1111.sa1111_probe() cannot complete before pxa_cplds_irqs.probe() because
>    it needs IRQ305 as its source
>    => it might need to honor -EPROBE_DEFER

I wonder if we can work around that by adding this to sa1111_probe():

	if (!irq_get_chip(irq))
		return -EPROBE_DEFER;

It's not particularly nice, but it should at least ensure that things
happen in the right order.
diff mbox

Patch

diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index f034928b99a1..81a1de6fb46f 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c
@@ -13,7 +13,7 @@ 
  */
 #include <linux/clkdev.h>
 #include <linux/gpio.h>
-#include <linux/gpip/gpio-reg.h>
+#include <linux/gpio/gpio-reg.h>
 #include <linux/gpio/machine.h>
 #include <linux/module.h>
 #include <linux/kernel.h>

[2] Kind-of fix for lubbock pcmcia probe
From 977c16201a752aac8a8fb2da1f4271795f0b2122 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <robert.jarzmik@free.fr>
Date: Thu, 1 Sep 2016 08:31:08 +0200
Subject: [PATCH] pcmcia: lubbock: fix sockets configuration

On lubbock board, the probe of the driver crashes by dereferencing very
early a platform_data structure which is not set, in
pxa2xx_configure_sockets().

This patch blindly fixes it without any analysis as to know if it's the
right fix or even if the fix doesn't break in suspend/resume.

The stack fixed is :
[    0.244353] SA1111 Microprocessor Companion Chip: silicon revision 1, metal revision 1
[    0.256321] sa1111 sa1111: Providing IRQ336-390
[    0.340899] clocksource: Switched to clocksource oscr0
[    0.472263] Unable to handle kernel NULL pointer dereference at virtual address 00000004
[    0.480469] pgd = c0004000
[    0.483432] [00000004] *pgd=00000000
[    0.487105] Internal error: Oops: f5 [#1] ARM
[    0.491497] Modules linked in:
[    0.494650] CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc3-00080-g1aaa68426f0c-dirty #2068
[    0.503229] Hardware name: Intel DBPXA250 Development Platform (aka Lubbock)
[    0.510344] task: c3e42000 task.stack: c3e44000
[    0.514984] PC is at pxa2xx_configure_sockets+0x4/0x24 (drivers/pcmcia/pxa2xx_base.c:227)
[    0.520193] LR is at pcmcia_lubbock_init+0x1c/0x38
[    0.525079] pc : [<c0247c30>]    lr : [<c02479b0>]    psr: a0000053
[    0.525079] sp : c3e45e70  ip : 100019ff  fp : 00000000
[    0.536651] r10: c0828900  r9 : c0434838  r8 : 00000000
[    0.541953] r7 : c0820700  r6 : c0857b30  r5 : c3ec1400  r4 : c0820758
[    0.548549] r3 : 00000000  r2 : 0000000c  r1 : c3c09c40  r0 : c3ec1400
[    0.555154] Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
[    0.562450] Control: 0000397f  Table: a0004000  DAC: 00000053
[    0.568257] Process swapper (pid: 1, stack limit = 0xc3e44190)
[    0.574154] Stack: (0xc3e45e70 to 0xc3e46000)
[    0.578610] 5e60:                                     c4849800 00000000 c3ec1400 c024769c
[    0.586928] 5e80: 00000000 c3ec140c c3c0ee0c c3ec1400 c3ec1434 c020c410 c3ec1400 c3ec1434
[    0.595244] 5ea0: c0820700 c080b408 c0828900 c020c5f8 00000000 c0820700 c020c578 c020ac5c
[    0.603560] 5ec0: c3e687cc c3e71e10 c0820700 00000000 c3c02de0 c020bae4 c03c62f7 c03c62f7
[    0.611872] 5ee0: c3e68780 c0820700 c042e034 00000000 c043c440 c020cdec c080b408 00000005
[    0.620188] 5f00: c042e034 c00096c0 c0034440 c01c730c 20000053 ffffffff 00000000 00000000
[    0.628502] 5f20: 00000000 c3ffcb87 c3ffcb90 c00346ac c3e66ba0 c03f7914 00000092 00000005
[    0.636811] 5f40: 00000005 c03f847c 00000091 c03f847c 00000000 00000005 c0434828 00000005
[    0.645125] 5f60: c043482c 00000092 c043c440 c0828900 c0434838 c0418d2c 00000005 00000005
[    0.653430] 5f80: 00000000 c041858c 00000000 c032e9f0 00000000 00000000 00000000 00000000
[    0.661729] 5fa0: 00000000 c032e9f8 00000000 c000f0f0 00000000 00000000 00000000 00000000
[    0.670020] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    0.678311] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[    0.686673] (pxa2xx_configure_sockets) from pcmcia_lubbock_init (/drivers/pcmcia/sa1111_lubbock.c:161)
[    0.696026] (pcmcia_lubbock_init) from pcmcia_probe (/drivers/pcmcia/sa1111_generic.c:213)
[    0.704358] (pcmcia_probe) from driver_probe_device (/drivers/base/dd.c:378 /drivers/base/dd.c:499)
[    0.712848] (driver_probe_device) from __driver_attach (/./include/linux/device.h:983 /drivers/base/dd.c:733)
[    0.721414] (__driver_attach) from bus_for_each_dev (/drivers/base/bus.c:313)
[    0.729723] (bus_for_each_dev) from bus_add_driver (/drivers/base/bus.c:708)
[    0.738036] (bus_add_driver) from driver_register (/drivers/base/driver.c:169)
[    0.746185] (driver_register) from do_one_initcall (/init/main.c:778)
[    0.754561] (do_one_initcall) from kernel_init_freeable (/init/main.c:843 /init/main.c:851 /init/main.c:869 /init/main.c:1016)
[    0.763409] (kernel_init_freeable) from kernel_init (/init/main.c:944)
[    0.771660] (kernel_init) from ret_from_fork (/arch/arm/kernel/entry-common.S:119)
[ 0.779347] Code: c03c6305 c03c631e c03c632e e5903048 (e993000c)
All code
========
   0:	c03c6305 	eorsgt	r6, ip, r5, lsl #6
   4:	c03c631e 	eorsgt	r6, ip, lr, lsl r3
   8:	c03c632e 	eorsgt	r6, ip, lr, lsr #6
   c:	e5903048 	ldr	r3, [r0, #72]	; 0x48
  10:*	e993000c 	ldmib	r3, {r2, r3}		<-- trapping instruction

Code starting with the faulting instruction
===========================================
   0:	e993000c 	ldmib	r3, {r2, r3}
[    0.786319] ---[ end trace 5264be19ef367bea ]---
[    0.791201] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[    0.791201]
[    0.800441] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/pcmcia/pxa2xx_base.c    | 9 +++++----
 drivers/pcmcia/pxa2xx_base.h    | 2 +-
 drivers/pcmcia/sa1111_lubbock.c | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pcmcia/pxa2xx_base.c b/drivers/pcmcia/pxa2xx_base.c
index 483f919e0d2e..91b5f5724cba 100644
--- a/drivers/pcmcia/pxa2xx_base.c
+++ b/drivers/pcmcia/pxa2xx_base.c
@@ -214,9 +214,8 @@  pxa2xx_pcmcia_frequency_change(struct soc_pcmcia_socket *skt,
 }
 #endif
 
-void pxa2xx_configure_sockets(struct device *dev)
+void pxa2xx_configure_sockets(struct device *dev, struct pcmcia_low_level *ops)
 {
-	struct pcmcia_low_level *ops = dev->platform_data;
 	/*
 	 * We have at least one socket, so set MECR:CIT
 	 * (Card Is There)
@@ -322,7 +321,7 @@  static int pxa2xx_drv_pcmcia_probe(struct platform_device *dev)
 			goto err1;
 	}
 
-	pxa2xx_configure_sockets(&dev->dev);
+	pxa2xx_configure_sockets(&dev->dev, ops);
 	dev_set_drvdata(&dev->dev, sinfo);
 
 	return 0;
@@ -348,7 +347,9 @@  static int pxa2xx_drv_pcmcia_remove(struct platform_device *dev)
 
 static int pxa2xx_drv_pcmcia_resume(struct device *dev)
 {
-	pxa2xx_configure_sockets(dev);
+	struct pcmcia_low_level *ops = (struct pcmcia_low_level *)dev->platform_data;
+
+	pxa2xx_configure_sockets(dev, ops);
 	return 0;
 }
 
diff --git a/drivers/pcmcia/pxa2xx_base.h b/drivers/pcmcia/pxa2xx_base.h
index b609b45469ed..e58c7a415418 100644
--- a/drivers/pcmcia/pxa2xx_base.h
+++ b/drivers/pcmcia/pxa2xx_base.h
@@ -1,4 +1,4 @@ 
 int pxa2xx_drv_pcmcia_add_one(struct soc_pcmcia_socket *skt);
 void pxa2xx_drv_pcmcia_ops(struct pcmcia_low_level *ops);
-void pxa2xx_configure_sockets(struct device *dev);
+void pxa2xx_configure_sockets(struct device *dev, struct pcmcia_low_level *ops);
 
diff --git a/drivers/pcmcia/sa1111_lubbock.c b/drivers/pcmcia/sa1111_lubbock.c
index 9d5ffc71ae51..7c1f0f6cd578 100644
--- a/drivers/pcmcia/sa1111_lubbock.c
+++ b/drivers/pcmcia/sa1111_lubbock.c
@@ -157,7 +157,7 @@  int pcmcia_lubbock_init(struct sa1111_dev *sadev)
 
 	if (machine_is_lubbock()) {
 		pxa2xx_drv_pcmcia_ops(&lubbock_pcmcia_ops);
-		pxa2xx_configure_sockets(&sadev->dev);
+		pxa2xx_configure_sockets(&sadev->dev, &lubbock_pcmcia_ops);
 		ret = sa1111_pcmcia_add(sadev, &lubbock_pcmcia_ops,
 				pxa2xx_drv_pcmcia_add_one);
 	}