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

2016-09-02 Thread Russell King - ARM Linux 
On Fri, Sep 02, 2016 at 07:50:35PM +0200, Robert Jarzmik wrote:
> Russell King - ARM Linux  writes:
> >> Moreover, I have a bit of homework as I also see :
> >>  - no SA 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 SA 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:   1419SC   8 Edge  gpio-0
>25:  0SC   9 Edge  gpio-1
>26:  0SC  10 Edge  gpio-mux
>38:118SC  22 Edge  UART1
>41:  0SC  25 Edge  DMA
>42:  40224SC  26 Edge  ost0
>   112:   1419  GPIO   0 Edge  pxa_cplds_irqs
>   307:   1419  pxa_cplds   3 Edge  eth0
>   387:  0  SA-h Edge  SA PCMCIA card detect
>   388:  0  SA-h Edge  SA CF card detect
>   389:  0  SA-h Edge  SA PCMCIA BVD1
>   390:  0  SA-h Edge  SA CF BVD1
>   Err:  0
> 
> Actually this leads me to think that this interrupt 305 is not "requested" nor
> activated. I see in sa.c:506 :
>   "irq_set_chained_handler_and_data(sachip->irq, sa_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 = _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 sa is connected
being unmasked.

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

> I traced the code in the interrupt controller (pxa_cplds_irqs), the SA
> 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 SA interrupts are not
> fired. Even if they would have been enabled, the "pending interrupts register"
> doesn't show any sa interrupt pending, so there is something else.

Masked where though - in the SA or FPGA?

> As I don't know if "enable_irq()" in sa.c would be an option as I have no
> sight on sa.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
>   01 04 df 79 01 ff 1c 04  02 db 01 ff 18 02 df 01  |...y|
> 0010  20 04 45 00 01 04 15 0b

Re: [PATCH] pcmcia: sa1111: fix propagation of lowlevel board init return code

2016-09-02 Thread Robert Jarzmik 
Russell King  writes:

> When testing Lubbock, it was noticed that the sa pcmcia driver bound
> but was not functional due to no sockets being registered.  This is
> because the return code from the lowlevel board initialisation was not
> being propagated out of the probe function.  Fix this.
>
> Signed-off-by: Russell King 
> ---
> Robert,
>
> I'd prefer this solution.  Please check that it resolves your issue,
> thanks.

Excellent, tested it without the clock fix on lubbock, and the error message is
there.

Tested-by: Robert Jarzmik 

Cheers.

--
Robert

___
Linux PCMCIA reimplementation list
http://lists.infradead.org/mailman/listinfo/linux-pcmcia

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

2016-09-02 Thread Robert Jarzmik 
Russell King - ARM Linux  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 sa irqchip handle the both-
> edge case itself.
Ok.

>> Moreover, I have a bit of homework as I also see :
>>  - no SA 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 SA 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:   1419SC   8 Edge  gpio-0
 25:  0SC   9 Edge  gpio-1
 26:  0SC  10 Edge  gpio-mux
 38:118SC  22 Edge  UART1
 41:  0SC  25 Edge  DMA
 42:  40224SC  26 Edge  ost0
112:   1419  GPIO   0 Edge  pxa_cplds_irqs
307:   1419  pxa_cplds   3 Edge  eth0
387:  0  SA-h Edge  SA PCMCIA card detect
388:  0  SA-h Edge  SA CF card detect
389:  0  SA-h Edge  SA PCMCIA BVD1
390:  0  SA-h Edge  SA CF BVD1
Err:  0

Actually this leads me to think that this interrupt 305 is not "requested" nor
activated. I see in sa.c:506 :
  "irq_set_chained_handler_and_data(sachip->irq, sa_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 SA
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 SA interrupts are not
fired. Even if they would have been enabled, the "pending interrupts register"
doesn't show any sa interrupt pending, so there is something else.

As I don't know if "enable_irq()" in sa.c would be an option as I have no
sight on sa.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
  01 04 df 79 01 ff 1c 04  02 db 01 ff 18 02 df 01  |...y|
0010  20 04 45 00 01 04 15 0b  04 01 00 43 46 43 41 52  | .ECFCAR|
0020  44 00 ff 21 02 04 01 22  02 01 01 22 03 02 0c 0f  |D..!..."..."|
0030  1a 05 01 03 00 02 0f 1b  08 c0 40 a1 01 55 08 00  |..@..U..|
0040  20 1b 06 00 01 21 b5 1e  4d 1b 0a c1 41 99 01 55  | !..M...A..U|
0050  64 f0 ff ff 20 1b 06 01  01 21 b5 1e 4d 1b 0f c2  |d... !..M...|
0060  41 99 01 55 ea 61 f0 01  07 f6 03 01 ee 20 1b 06  |A..U.a... ..|
0070  02 01 21 b5 1e 4d 1b 0f  c3 41 99 01 55 ea 61 70  |..!..M...A..U.ap|
0080  01 07 76 03 01 ee 20 1b  06 03 01 21 b5 1e 4d 14  |..v... !..M.|
0090  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.

-- 
Robert

___
Linux PCMCIA reimplementation list
http://lists.infradead.org/mailman/listinfo/linux-pcmcia

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

2016-09-02 Thread Russell King - ARM Linux 
Linus,

There's a change I'd like to merge into patch 5 - overall it looks
like the below, and allows us to use gpio-reg with the PXA mainstone
MST_PCMCIA[01] registers.  Some of these GPIO signals have hardware
interrupts associated with them, but not all.

Do you approve?

Thanks.

8<
From: Russell King 
Subject: [PATCH] gpio: gpio-reg: add irq mapping for gpio-reg users

Add support for mapping gpio-reg gpios to interrupts.  This may be a
non-linear mapping - some gpios in the register may not even have
corresponding interrupts associated with them, so we need to pass an
array.

Signed-off-by: Russell King 
---
 arch/arm/mach-pxa/lubbock.c |  2 +-
 arch/arm/mach-sa1100/assabet.c  |  2 +-
 arch/arm/mach-sa1100/neponset.c |  2 +-
 drivers/gpio/gpio-reg.c | 25 +++--
 include/linux/gpio/gpio-reg.h   |  3 ++-
 5 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index 1f7bfabfa4eb..cd401546cea8 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c
@@ -477,7 +477,7 @@ static void __init lubbock_init(void)
 
lubbock_misc_wr_gc = gpio_reg_init(NULL, (void *)_MISC_WR,
   -1, 16, "lubbock", 0, LUB_MISC_WR,
-  NULL);
+  NULL, NULL, NULL);
if (IS_ERR(lubbock_misc_wr_gc)) {
pr_err("Lubbock: unable to register lubbock GPIOs: %ld\n",
   PTR_ERR(lubbock_misc_wr_gc));
diff --git a/arch/arm/mach-sa1100/assabet.c b/arch/arm/mach-sa1100/assabet.c
index 51950454afac..e086c609551c 100644
--- a/arch/arm/mach-sa1100/assabet.c
+++ b/arch/arm/mach-sa1100/assabet.c
@@ -120,7 +120,7 @@ static int __init assabet_init_gpio(void __iomem *reg, u32 
def_val)
writel_relaxed(def_val, reg);
 
gc = gpio_reg_init(NULL, reg, -1, 32, "assabet", 0xff00, def_val,
-  assabet_names);
+  assabet_names, NULL, NULL);
 
if (IS_ERR(gc))
return PTR_ERR(gc);
diff --git a/arch/arm/mach-sa1100/neponset.c b/arch/arm/mach-sa1100/neponset.c
index 5abc04fb196d..b16de1f0d5d0 100644
--- a/arch/arm/mach-sa1100/neponset.c
+++ b/arch/arm/mach-sa1100/neponset.c
@@ -240,7 +240,7 @@ static int neponset_init_gpio(struct gpio_chip **gcp,
struct gpio_chip *gc;
 
gc = gpio_reg_init(dev, reg, -1, num, label, in ? 0x : 0,
-  readl_relaxed(reg), names);
+  readl_relaxed(reg), names, NULL, NULL);
if (IS_ERR(gc))
return PTR_ERR(gc);
 
diff --git a/drivers/gpio/gpio-reg.c b/drivers/gpio/gpio-reg.c
index 561e28b4d3aa..39175d373871 100644
--- a/drivers/gpio/gpio-reg.c
+++ b/drivers/gpio/gpio-reg.c
@@ -19,6 +19,8 @@ struct gpio_reg {
u32 direction;
u32 out;
void __iomem *reg;
+   struct irq_domain *irqdomain;
+   const int *irqs;
 };
 
 #define to_gpio_reg(x) container_of(x, struct gpio_reg, gc)
@@ -96,6 +98,17 @@ static void gpio_reg_set_multiple(struct gpio_chip *gc, 
unsigned long *mask,
spin_unlock_irqrestore(>lock, flags);
 }
 
+static int gpio_reg_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+   struct gpio_reg *r = to_gpio_reg(gc);
+   int irq = r->irqs[offset];
+
+   if (irq >= 0 && r->irqdomain)
+   irq = irq_find_mapping(r->irqdomain, irq);
+
+   return irq;
+}
+
 /**
  * gpio_reg_init - add a fixed in/out register as gpio
  * @dev: optional struct device associated with this register
@@ -104,7 +117,12 @@ static void gpio_reg_set_multiple(struct gpio_chip *gc, 
unsigned long *mask,
  * @label: GPIO chip label
  * @direction: bitmask of fixed direction, one per GPIO signal, 1 = in
  * @def_out: initial GPIO output value
- * @names: array of %num strings describing each GPIO signal
+ * @names: array of %num strings describing each GPIO signal or %NULL
+ * @irqdom: irq domain or %NULL
+ * @irqs: array of %num ints describing the interrupt mapping for each
+ *GPIO signal, or %NULL.  If @irqdom is %NULL, then this
+ *describes the Linux interrupt number, otherwise it describes
+ *the hardware interrupt number in the specified irq domain.
  *
  * Add a single-register GPIO device containing up to 32 GPIO signals,
  * where each GPIO has a fixed input or output configuration.  Only
@@ -114,7 +132,7 @@ static void gpio_reg_set_multiple(struct gpio_chip *gc, 
unsigned long *mask,
  */
 struct gpio_chip *gpio_reg_init(struct device *dev, void __iomem *reg,
int base, int num, const char *label, u32 direction, u32 def_out,
-   const char *const *names)
+   const char *const *names, struct irq_domain *irqdom, const int *irqs)
 {
struct gpio_reg *r;
int ret;
@@ -136,12 +154,15 @@ struct gpio_chip *gpio_reg_init(struct device

[PATCH] pcmcia: sa1111: fix propagation of lowlevel board init return code

2016-09-02 Thread Russell King 
When testing Lubbock, it was noticed that the sa pcmcia driver bound
but was not functional due to no sockets being registered.  This is
because the return code from the lowlevel board initialisation was not
being propagated out of the probe function.  Fix this.

Signed-off-by: Russell King 
---
Robert,

I'd prefer this solution.  Please check that it resolves your issue,
thanks.

 drivers/pcmcia/sa_badge4.c | 22 --
 drivers/pcmcia/sa_generic.c| 22 +-
 drivers/pcmcia/sa_jornada720.c | 16 +---
 drivers/pcmcia/sa_lubbock.c| 14 --
 drivers/pcmcia/sa_neponset.c   | 12 +++-
 5 files changed, 37 insertions(+), 49 deletions(-)

diff --git a/drivers/pcmcia/sa_badge4.c b/drivers/pcmcia/sa_badge4.c
index 12f0dd091477..2f490930430d 100644
--- a/drivers/pcmcia/sa_badge4.c
+++ b/drivers/pcmcia/sa_badge4.c
@@ -134,20 +134,14 @@ static struct pcmcia_low_level badge4_pcmcia_ops = {
 
 int pcmcia_badge4_init(struct sa_dev *dev)
 {
-   int ret = -ENODEV;
-
-   if (machine_is_badge4()) {
-   printk(KERN_INFO
-  "%s: badge4_pcmvcc=%d, badge4_pcmvpp=%d, 
badge4_cfvcc=%d\n",
-  __func__,
-  badge4_pcmvcc, badge4_pcmvpp, badge4_cfvcc);
-
-   sa11xx_drv_pcmcia_ops(_pcmcia_ops);
-   ret = sa_pcmcia_add(dev, _pcmcia_ops,
-   sa11xx_drv_pcmcia_add_one);
-   }
-
-   return ret;
+   printk(KERN_INFO
+  "%s: badge4_pcmvcc=%d, badge4_pcmvpp=%d, badge4_cfvcc=%d\n",
+  __func__,
+  badge4_pcmvcc, badge4_pcmvpp, badge4_cfvcc);
+
+   sa11xx_drv_pcmcia_ops(_pcmcia_ops);
+   return sa_pcmcia_add(dev, _pcmcia_ops,
+sa11xx_drv_pcmcia_add_one);
 }
 
 static int __init pcmv_setup(char *s)
diff --git a/drivers/pcmcia/sa_generic.c b/drivers/pcmcia/sa_generic.c
index a1531feb8460..3d95dffcff7a 100644
--- a/drivers/pcmcia/sa_generic.c
+++ b/drivers/pcmcia/sa_generic.c
@@ -18,6 +18,7 @@
 
 #include 
 #include 
+#include 
 #include 
 
 #include "sa_generic.h"
@@ -203,19 +204,30 @@ static int pcmcia_probe(struct sa_dev *dev)
sa_writel(PCSSR_S0_SLEEP | PCSSR_S1_SLEEP, base + PCSSR);
sa_writel(PCCR_S0_FLT | PCCR_S1_FLT, base + PCCR);
 
+   ret = -ENODEV;
 #ifdef CONFIG_SA1100_BADGE4
-   pcmcia_badge4_init(dev);
+   if (machine_is_badge4())
+   ret = pcmcia_badge4_init(dev);
 #endif
 #ifdef CONFIG_SA1100_JORNADA720
-   pcmcia_jornada720_init(dev);
+   if (machine_is_jornada720())
+   ret = pcmcia_jornada720_init(dev);
 #endif
 #ifdef CONFIG_ARCH_LUBBOCK
-   pcmcia_lubbock_init(dev);
+   if (machine_is_lubbock())
+   ret = pcmcia_lubbock_init(dev);
 #endif
 #ifdef CONFIG_ASSABET_NEPONSET
-   pcmcia_neponset_init(dev);
+   if (machine_is_assabet())
+   ret = pcmcia_neponset_init(dev);
 #endif
-   return 0;
+
+   if (ret) {
+   release_mem_region(dev->res.start, 512);
+   sa_disable_device(dev);
+   }
+
+   return ret;
 }
 
 static int pcmcia_remove(struct sa_dev *dev)
diff --git a/drivers/pcmcia/sa_jornada720.c 
b/drivers/pcmcia/sa_jornada720.c
index bfcb9fd43352..9ce146e018c0 100644
--- a/drivers/pcmcia/sa_jornada720.c
+++ b/drivers/pcmcia/sa_jornada720.c
@@ -127,16 +127,10 @@ static struct pcmcia_low_level jornada720_pcmcia_ops = {
 
 int pcmcia_jornada720_init(struct sa_dev *sadev)
 {
-   int ret = -ENODEV;
+   /* Fixme: why messing around with SA11x0's GPIO1? */
+   GRER |= 0x0002;
 
-   if (machine_is_jornada720()) {
-   /* Fixme: why messing around with SA11x0's GPIO1? */
-   GRER |= 0x0002;
-
-   sa11xx_drv_pcmcia_ops(_pcmcia_ops);
-   ret = sa_pcmcia_add(sadev, _pcmcia_ops,
-   sa11xx_drv_pcmcia_add_one);
-   }
-
-   return ret;
+   sa11xx_drv_pcmcia_ops(_pcmcia_ops);
+   return sa_pcmcia_add(sadev, _pcmcia_ops,
+sa11xx_drv_pcmcia_add_one);
 }
diff --git a/drivers/pcmcia/sa_lubbock.c b/drivers/pcmcia/sa_lubbock.c
index 9d5ffc71ae51..ccde1a8bba12 100644
--- a/drivers/pcmcia/sa_lubbock.c
+++ b/drivers/pcmcia/sa_lubbock.c
@@ -153,16 +153,10 @@ static struct pcmcia_low_level lubbock_pcmcia_ops = {
 
 int pcmcia_lubbock_init(struct sa_dev *sadev)
 {
-   int ret = -ENODEV;
-
-   if (machine_is_lubbock()) {
-   pxa2xx_drv_pcmcia_ops(_pcmcia_ops);
-   pxa2xx_configure_sockets(>dev);
-   ret = sa_pcmcia_add(sadev, _pcmcia_ops,
-   pxa2xx_drv_pcmcia_add_one);
-   }
-
-   return ret;
+   pxa2xx_drv_pcmcia_ops(_pcmcia_ops);
+