* plat-orion multi purpose pins problem for mv78200 @ 2011-07-01 19:14 Joey Oravec 2011-07-01 21:56 ` Joey Oravec 2011-07-02 12:23 ` Simon Guinot 0 siblings, 2 replies; 16+ messages in thread From: Joey Oravec @ 2011-07-01 19:14 UTC (permalink / raw) To: linux-arm-kernel Nicola, Lennert - There's a problem in 3.0-rc5 when calling mv78xx0_mpp_conf() on the MV78200 processor. This processor has 49 multi purpose pins but only 31 GPIOs. MPP[31:0] map directly to GPIO[31:0] but the next 17 are a little more complex: MPP[39:32] = GPIO[7:0] MPP[46:40] = GPIO[23:17] MPP[47] = GPIO[16] MPP[48] = GPIO[8] MPP[49] = GPIO[9] See arch/arm/plat-orion/mpp.c. Inside orion_mpp_conf() the array gets processed by calling MPP_NUM() then calling orion_gpio_set_valid(). As demonstrated above, the MPP number and GPIO number are not interchangeable and on the MV78200 that's a problem when trying to setup any MPP > 31. So we need to map each MPP to a given GPIO -- I don't know how the mapping will differ across processors. We also need to keep-clear which functions are called with an MPP number and which functions are called with a GPIO number. Followup question -- Marvell has several SoC families, but Linux uess plat-orion for everything. For example MV78200 isn't an Orion it's a Discovery Innovation series. Would it make more sense to have separate plat-* code for the separate families? -joey ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-01 19:14 plat-orion multi purpose pins problem for mv78200 Joey Oravec @ 2011-07-01 21:56 ` Joey Oravec 2011-07-02 12:23 ` Simon Guinot 1 sibling, 0 replies; 16+ messages in thread From: Joey Oravec @ 2011-07-01 21:56 UTC (permalink / raw) To: linux-arm-kernel It looks like there's a second problem here. Imagine you're calling mv78xx0_mpp_conf() and the array has these entries: MPP16_GPIO MPP47_UNUSED These are separate MPP pins that can each bring out the same GPIO resource. Notice that as the code stands today, the second call is going to clobber the values valid_input and valid_output for GPIO[16]. The logic for orion_gpio_is_valid() needs to check if a given GPIO is brought-out on any MPP pin. In the short term, it might be worth disabling the check in orion_gpio_is_valid() to avoid the bug. -joey On 7/1/2011 3:14 PM, Joey Oravec wrote: > Nicola, Lennert - > > There's a problem in 3.0-rc5 when calling mv78xx0_mpp_conf() on the > MV78200 processor. This processor has 49 multi purpose pins but only > 31 GPIOs. MPP[31:0] map directly to GPIO[31:0] but the next 17 are a > little more complex: > > MPP[39:32] = GPIO[7:0] > MPP[46:40] = GPIO[23:17] > MPP[47] = GPIO[16] > MPP[48] = GPIO[8] > MPP[49] = GPIO[9] > > See arch/arm/plat-orion/mpp.c. Inside orion_mpp_conf() the array gets > processed by calling MPP_NUM() then calling orion_gpio_set_valid(). As > demonstrated above, the MPP number and GPIO number are not > interchangeable and on the MV78200 that's a problem when trying to > setup any MPP > 31. > > So we need to map each MPP to a given GPIO -- I don't know how the > mapping will differ across processors. We also need to keep-clear > which functions are called with an MPP number and which functions are > called with a GPIO number. > > Followup question -- Marvell has several SoC families, but Linux uess > plat-orion for everything. For example MV78200 isn't an Orion it's a > Discovery Innovation series. Would it make more sense to have separate > plat-* code for the separate families? > > -joey > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-01 19:14 plat-orion multi purpose pins problem for mv78200 Joey Oravec 2011-07-01 21:56 ` Joey Oravec @ 2011-07-02 12:23 ` Simon Guinot 2011-07-03 12:46 ` saeed bishara 1 sibling, 1 reply; 16+ messages in thread From: Simon Guinot @ 2011-07-02 12:23 UTC (permalink / raw) To: linux-arm-kernel Hi Joey, On Fri, Jul 01, 2011 at 03:14:48PM -0400, Joey Oravec wrote: > Nicola, Lennert - > > There's a problem in 3.0-rc5 when calling mv78xx0_mpp_conf() on the > MV78200 processor. This processor has 49 multi purpose pins but only > 31 GPIOs. MPP[31:0] map directly to GPIO[31:0] but the next 17 are a > little more complex: > > MPP[39:32] = GPIO[7:0] > MPP[46:40] = GPIO[23:17] > MPP[47] = GPIO[16] > MPP[48] = GPIO[8] > MPP[49] = GPIO[9] It doesn't seems the code was behaving differently before 3.0-rc. I mean before the MPP and GPIO consolidation work. It was ? > > See arch/arm/plat-orion/mpp.c. Inside orion_mpp_conf() the array > gets processed by calling MPP_NUM() then calling > orion_gpio_set_valid(). As demonstrated above, the MPP number and > GPIO number are not interchangeable and on the MV78200 that's a > problem when trying to setup any MPP > 31. > > So we need to map each MPP to a given GPIO -- I don't know how the > mapping will differ across processors. We also need to keep-clear > which functions are called with an MPP number and which functions > are called with a GPIO number. As I don't have access to either a MV78200 device or his specification document, let me ask you a few questions. Does hardware allows a configuration like MPP0_GE0_COL and MPP32_GPIO simultaneously ? If not (and only if not), the problem is simply mapping a MPP number on the correct GPIO number. Isn't it ? In that case, what about using the MPPxx_GPIO definitions (found in mach-mv78xx0/mpp.h) to map the right GPIO number ? For example: #define MPP32_GPIO MPP(32, 0x1, 1, 1, 1) could be replaced by: #define MPP32_GPIO MPP(0, 0x1, 1, 1, 1) > > Followup question -- Marvell has several SoC families, but Linux > uess plat-orion for everything. For example MV78200 isn't an Orion > it's a Discovery Innovation series. Would it make more sense to have > separate plat-* code for the separate families? As far as possible, we have to share code between the SoCs families. Regards, Simon -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 198 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110702/c00680b4/attachment.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-02 12:23 ` Simon Guinot @ 2011-07-03 12:46 ` saeed bishara 2011-07-04 14:33 ` [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants Simon Guinot 2011-07-05 15:37 ` plat-orion multi purpose pins problem for mv78200 Joey Oravec 0 siblings, 2 replies; 16+ messages in thread From: saeed bishara @ 2011-07-03 12:46 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jul 2, 2011 at 3:23 PM, Simon Guinot <simon@sequanux.org> wrote: > Hi Joey, > > On Fri, Jul 01, 2011 at 03:14:48PM -0400, Joey Oravec wrote: >> Nicola, Lennert - >> >> There's a problem in 3.0-rc5 when calling mv78xx0_mpp_conf() on the >> MV78200 processor. This processor has 49 multi purpose pins but only >> 31 GPIOs. MPP[31:0] map directly to GPIO[31:0] but the next 17 are a >> little more complex: >> >> MPP[39:32] = GPIO[7:0] >> MPP[46:40] = GPIO[23:17] >> MPP[47] = GPIO[16] >> MPP[48] = GPIO[8] >> MPP[49] = GPIO[9] > > It doesn't seems the code was behaving differently before 3.0-rc. I mean > before the MPP and GPIO consolidation work. It was ? that's right, this issue was before the consolidation work. > >> >> See arch/arm/plat-orion/mpp.c. Inside orion_mpp_conf() the array >> gets processed by calling MPP_NUM() then calling >> orion_gpio_set_valid(). As demonstrated above, the MPP number and >> GPIO number are not interchangeable and on the MV78200 that's a >> problem when trying to setup any MPP > 31. >> >> So we need to map each MPP to a given GPIO -- I don't know how the >> mapping will differ across processors. We also need to keep-clear >> which functions are called with an MPP number and which functions >> are called with a GPIO number. > > As I don't have access to either a MV78200 device or his specification > document, let me ask you a few questions. > > Does hardware allows a configuration like MPP0_GE0_COL and MPP32_GPIO > simultaneously ? yes, this is possible. > > If not (and only if not), the problem is simply mapping a MPP number on > the correct GPIO number. Isn't it ? > > In that case, what about using the MPPxx_GPIO definitions (found in > mach-mv78xx0/mpp.h) to map the right GPIO number ? > > For example: > > #define MPP32_GPIO ? ? ? ?MPP(32, 0x1, 1, 1, 1) > > could be replaced by: > > #define MPP32_GPIO ? ? ? ?MPP(0, 0x1, 1, 1, 1) that won't work because the num (first number) is used as the mpp index. see orion_mpp_conf() implementation. I think the solution is to extend the MPP define to include the explicit gpio number, so it will look like: #define MPP(_num, _gpio _sel, _in, _out, _78100_A0) saeed ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants 2011-07-03 12:46 ` saeed bishara @ 2011-07-04 14:33 ` Simon Guinot 2011-07-04 14:48 ` [PATCH v2] genirq: replace irq_gc_ack() with {set, clr}_bit variants Nicolas Pitre 2011-07-06 15:31 ` [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants Simon Guinot 2011-07-05 15:37 ` plat-orion multi purpose pins problem for mv78200 Joey Oravec 1 sibling, 2 replies; 16+ messages in thread From: Simon Guinot @ 2011-07-04 14:33 UTC (permalink / raw) To: linux-arm-kernel From: Simon Guinot <sguinot@lacie.com> Depending on the device, interrupts acknowledgement is done by setting or by clearing a dedicated register. Replace irq_gc_ack() with some {set,clr}_bit variants allows to handle both cases. Note that this patch affects the following SoCs: Davinci, Samsung and Orion. Except for this last, the change is minor: irq_gc_ack() is just renamed into irq_gc_ack_set_bit(). For the Orion SoCs, the edge GPIO interrupts support is currently broken. irq_gc_ack() try to acknowledge a such interrupt by setting the corresponding cause register bit. The Orion GPIO device expect the opposite. To fix this issue, the irq_gc_ack_clr_bit() variant is used. Tested on Network Space v2. Reported-by: Joey Oravec <joravec@drewtech.com> Signed-off-by: Simon Guinot <sguinot@lacie.com> --- Changes for v2: update patch description (mention the affected SoCs). arch/arm/mach-davinci/irq.c | 2 +- arch/arm/plat-orion/gpio.c | 2 +- arch/arm/plat-s5p/irq-gpioint.c | 2 +- arch/arm/plat-samsung/irq-uart.c | 2 +- include/linux/irq.h | 3 ++- kernel/irq/generic-chip.c | 18 ++++++++++++++++-- 6 files changed, 22 insertions(+), 7 deletions(-) diff --git a/arch/arm/mach-davinci/irq.c b/arch/arm/mach-davinci/irq.c index bfe68ec..d8c1af0 100644 --- a/arch/arm/mach-davinci/irq.c +++ b/arch/arm/mach-davinci/irq.c @@ -53,7 +53,7 @@ davinci_alloc_gc(void __iomem *base, unsigned int irq_start, unsigned int num) gc = irq_alloc_generic_chip("AINTC", 1, irq_start, base, handle_edge_irq); ct = gc->chip_types; - ct->chip.irq_ack = irq_gc_ack; + ct->chip.irq_ack = irq_gc_ack_set_bit; ct->chip.irq_mask = irq_gc_mask_clr_bit; ct->chip.irq_unmask = irq_gc_mask_set_bit; diff --git a/arch/arm/plat-orion/gpio.c b/arch/arm/plat-orion/gpio.c index 5b4fffa..41ab97e 100644 --- a/arch/arm/plat-orion/gpio.c +++ b/arch/arm/plat-orion/gpio.c @@ -432,7 +432,7 @@ void __init orion_gpio_init(int gpio_base, int ngpio, ct->regs.mask = ochip->mask_offset + GPIO_EDGE_MASK_OFF; ct->regs.ack = GPIO_EDGE_CAUSE_OFF; ct->type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING; - ct->chip.irq_ack = irq_gc_ack; + ct->chip.irq_ack = irq_gc_ack_clr_bit; ct->chip.irq_mask = irq_gc_mask_clr_bit; ct->chip.irq_unmask = irq_gc_mask_set_bit; ct->chip.irq_set_type = gpio_irq_set_type; diff --git a/arch/arm/plat-s5p/irq-gpioint.c b/arch/arm/plat-s5p/irq-gpioint.c index 135abda..327ab9f 100644 --- a/arch/arm/plat-s5p/irq-gpioint.c +++ b/arch/arm/plat-s5p/irq-gpioint.c @@ -152,7 +152,7 @@ static __init int s5p_gpioint_add(struct s3c_gpio_chip *chip) if (!gc) return -ENOMEM; ct = gc->chip_types; - ct->chip.irq_ack = irq_gc_ack; + ct->chip.irq_ack = irq_gc_ack_set_bit; ct->chip.irq_mask = irq_gc_mask_set_bit; ct->chip.irq_unmask = irq_gc_mask_clr_bit; ct->chip.irq_set_type = s5p_gpioint_set_type, diff --git a/arch/arm/plat-samsung/irq-uart.c b/arch/arm/plat-samsung/irq-uart.c index 32582c0..0e46588 100644 --- a/arch/arm/plat-samsung/irq-uart.c +++ b/arch/arm/plat-samsung/irq-uart.c @@ -55,7 +55,7 @@ static void __init s3c_init_uart_irq(struct s3c_uart_irq *uirq) gc = irq_alloc_generic_chip("s3c-uart", 1, uirq->base_irq, reg_base, handle_level_irq); ct = gc->chip_types; - ct->chip.irq_ack = irq_gc_ack; + ct->chip.irq_ack = irq_gc_ack_set_bit; ct->chip.irq_mask = irq_gc_mask_set_bit; ct->chip.irq_unmask = irq_gc_mask_clr_bit; ct->regs.ack = S3C64XX_UINTP; diff --git a/include/linux/irq.h b/include/linux/irq.h index 8b45384..baa397e 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -676,7 +676,8 @@ void irq_gc_mask_disable_reg(struct irq_data *d); void irq_gc_mask_set_bit(struct irq_data *d); void irq_gc_mask_clr_bit(struct irq_data *d); void irq_gc_unmask_enable_reg(struct irq_data *d); -void irq_gc_ack(struct irq_data *d); +void irq_gc_ack_set_bit(struct irq_data *d); +void irq_gc_ack_clr_bit(struct irq_data *d); void irq_gc_mask_disable_reg_and_ack(struct irq_data *d); void irq_gc_eoi(struct irq_data *d); int irq_gc_set_wake(struct irq_data *d, unsigned int on); diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c index 31a9db7..3a2cab4 100644 --- a/kernel/irq/generic-chip.c +++ b/kernel/irq/generic-chip.c @@ -101,10 +101,10 @@ void irq_gc_unmask_enable_reg(struct irq_data *d) } /** - * irq_gc_ack - Ack pending interrupt + * irq_gc_ack_set_bit - Ack pending interrupt via setting bit * @d: irq_data */ -void irq_gc_ack(struct irq_data *d) +void irq_gc_ack_set_bit(struct irq_data *d) { struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); u32 mask = 1 << (d->irq - gc->irq_base); @@ -115,6 +115,20 @@ void irq_gc_ack(struct irq_data *d) } /** + * irq_gc_ack_clr_bit - Ack pending interrupt via clearing bit + * @d: irq_data + */ +void irq_gc_ack_clr_bit(struct irq_data *d) +{ + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + u32 mask = ~(1 << (d->irq - gc->irq_base)); + + irq_gc_lock(gc); + irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); + irq_gc_unlock(gc); +} + +/** * irq_gc_mask_disable_reg_and_ack- Mask and ack pending interrupt * @d: irq_data */ -- 1.7.5.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2] genirq: replace irq_gc_ack() with {set, clr}_bit variants 2011-07-04 14:33 ` [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants Simon Guinot @ 2011-07-04 14:48 ` Nicolas Pitre 2011-07-16 3:39 ` Kukjin Kim 2011-07-06 15:31 ` [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants Simon Guinot 1 sibling, 1 reply; 16+ messages in thread From: Nicolas Pitre @ 2011-07-04 14:48 UTC (permalink / raw) To: linux-arm-kernel On Mon, 4 Jul 2011, Simon Guinot wrote: > From: Simon Guinot <sguinot@lacie.com> > > Depending on the device, interrupts acknowledgement is done by setting > or by clearing a dedicated register. Replace irq_gc_ack() with some > {set,clr}_bit variants allows to handle both cases. > > Note that this patch affects the following SoCs: Davinci, Samsung and > Orion. Except for this last, the change is minor: irq_gc_ack() is just > renamed into irq_gc_ack_set_bit(). > > For the Orion SoCs, the edge GPIO interrupts support is currently > broken. irq_gc_ack() try to acknowledge a such interrupt by setting > the corresponding cause register bit. The Orion GPIO device expect the > opposite. To fix this issue, the irq_gc_ack_clr_bit() variant is used. > > Tested on Network Space v2. > > Reported-by: Joey Oravec <joravec@drewtech.com> > Signed-off-by: Simon Guinot <sguinot@lacie.com> Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org> > --- > Changes for v2: update patch description (mention the affected SoCs). > > arch/arm/mach-davinci/irq.c | 2 +- > arch/arm/plat-orion/gpio.c | 2 +- > arch/arm/plat-s5p/irq-gpioint.c | 2 +- > arch/arm/plat-samsung/irq-uart.c | 2 +- > include/linux/irq.h | 3 ++- > kernel/irq/generic-chip.c | 18 ++++++++++++++++-- > 6 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/mach-davinci/irq.c b/arch/arm/mach-davinci/irq.c > index bfe68ec..d8c1af0 100644 > --- a/arch/arm/mach-davinci/irq.c > +++ b/arch/arm/mach-davinci/irq.c > @@ -53,7 +53,7 @@ davinci_alloc_gc(void __iomem *base, unsigned int irq_start, unsigned int num) > > gc = irq_alloc_generic_chip("AINTC", 1, irq_start, base, handle_edge_irq); > ct = gc->chip_types; > - ct->chip.irq_ack = irq_gc_ack; > + ct->chip.irq_ack = irq_gc_ack_set_bit; > ct->chip.irq_mask = irq_gc_mask_clr_bit; > ct->chip.irq_unmask = irq_gc_mask_set_bit; > > diff --git a/arch/arm/plat-orion/gpio.c b/arch/arm/plat-orion/gpio.c > index 5b4fffa..41ab97e 100644 > --- a/arch/arm/plat-orion/gpio.c > +++ b/arch/arm/plat-orion/gpio.c > @@ -432,7 +432,7 @@ void __init orion_gpio_init(int gpio_base, int ngpio, > ct->regs.mask = ochip->mask_offset + GPIO_EDGE_MASK_OFF; > ct->regs.ack = GPIO_EDGE_CAUSE_OFF; > ct->type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING; > - ct->chip.irq_ack = irq_gc_ack; > + ct->chip.irq_ack = irq_gc_ack_clr_bit; > ct->chip.irq_mask = irq_gc_mask_clr_bit; > ct->chip.irq_unmask = irq_gc_mask_set_bit; > ct->chip.irq_set_type = gpio_irq_set_type; > diff --git a/arch/arm/plat-s5p/irq-gpioint.c b/arch/arm/plat-s5p/irq-gpioint.c > index 135abda..327ab9f 100644 > --- a/arch/arm/plat-s5p/irq-gpioint.c > +++ b/arch/arm/plat-s5p/irq-gpioint.c > @@ -152,7 +152,7 @@ static __init int s5p_gpioint_add(struct s3c_gpio_chip *chip) > if (!gc) > return -ENOMEM; > ct = gc->chip_types; > - ct->chip.irq_ack = irq_gc_ack; > + ct->chip.irq_ack = irq_gc_ack_set_bit; > ct->chip.irq_mask = irq_gc_mask_set_bit; > ct->chip.irq_unmask = irq_gc_mask_clr_bit; > ct->chip.irq_set_type = s5p_gpioint_set_type, > diff --git a/arch/arm/plat-samsung/irq-uart.c b/arch/arm/plat-samsung/irq-uart.c > index 32582c0..0e46588 100644 > --- a/arch/arm/plat-samsung/irq-uart.c > +++ b/arch/arm/plat-samsung/irq-uart.c > @@ -55,7 +55,7 @@ static void __init s3c_init_uart_irq(struct s3c_uart_irq *uirq) > gc = irq_alloc_generic_chip("s3c-uart", 1, uirq->base_irq, reg_base, > handle_level_irq); > ct = gc->chip_types; > - ct->chip.irq_ack = irq_gc_ack; > + ct->chip.irq_ack = irq_gc_ack_set_bit; > ct->chip.irq_mask = irq_gc_mask_set_bit; > ct->chip.irq_unmask = irq_gc_mask_clr_bit; > ct->regs.ack = S3C64XX_UINTP; > diff --git a/include/linux/irq.h b/include/linux/irq.h > index 8b45384..baa397e 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -676,7 +676,8 @@ void irq_gc_mask_disable_reg(struct irq_data *d); > void irq_gc_mask_set_bit(struct irq_data *d); > void irq_gc_mask_clr_bit(struct irq_data *d); > void irq_gc_unmask_enable_reg(struct irq_data *d); > -void irq_gc_ack(struct irq_data *d); > +void irq_gc_ack_set_bit(struct irq_data *d); > +void irq_gc_ack_clr_bit(struct irq_data *d); > void irq_gc_mask_disable_reg_and_ack(struct irq_data *d); > void irq_gc_eoi(struct irq_data *d); > int irq_gc_set_wake(struct irq_data *d, unsigned int on); > diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c > index 31a9db7..3a2cab4 100644 > --- a/kernel/irq/generic-chip.c > +++ b/kernel/irq/generic-chip.c > @@ -101,10 +101,10 @@ void irq_gc_unmask_enable_reg(struct irq_data *d) > } > > /** > - * irq_gc_ack - Ack pending interrupt > + * irq_gc_ack_set_bit - Ack pending interrupt via setting bit > * @d: irq_data > */ > -void irq_gc_ack(struct irq_data *d) > +void irq_gc_ack_set_bit(struct irq_data *d) > { > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > u32 mask = 1 << (d->irq - gc->irq_base); > @@ -115,6 +115,20 @@ void irq_gc_ack(struct irq_data *d) > } > > /** > + * irq_gc_ack_clr_bit - Ack pending interrupt via clearing bit > + * @d: irq_data > + */ > +void irq_gc_ack_clr_bit(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + u32 mask = ~(1 << (d->irq - gc->irq_base)); > + > + irq_gc_lock(gc); > + irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); > + irq_gc_unlock(gc); > +} > + > +/** > * irq_gc_mask_disable_reg_and_ack- Mask and ack pending interrupt > * @d: irq_data > */ > -- > 1.7.5.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] genirq: replace irq_gc_ack() with {set, clr}_bit variants 2011-07-04 14:48 ` [PATCH v2] genirq: replace irq_gc_ack() with {set, clr}_bit variants Nicolas Pitre @ 2011-07-16 3:39 ` Kukjin Kim 0 siblings, 0 replies; 16+ messages in thread From: Kukjin Kim @ 2011-07-16 3:39 UTC (permalink / raw) To: linux-arm-kernel Nicolas Pitre wrote: > > On Mon, 4 Jul 2011, Simon Guinot wrote: > > > From: Simon Guinot <sguinot@lacie.com> > > > > Depending on the device, interrupts acknowledgement is done by setting > > or by clearing a dedicated register. Replace irq_gc_ack() with some > > {set,clr}_bit variants allows to handle both cases. > > > > Note that this patch affects the following SoCs: Davinci, Samsung and > > Orion. Except for this last, the change is minor: irq_gc_ack() is just > > renamed into irq_gc_ack_set_bit(). > > > > For the Orion SoCs, the edge GPIO interrupts support is currently > > broken. irq_gc_ack() try to acknowledge a such interrupt by setting > > the corresponding cause register bit. The Orion GPIO device expect the > > opposite. To fix this issue, the irq_gc_ack_clr_bit() variant is used. > > > > Tested on Network Space v2. > > > > Reported-by: Joey Oravec <joravec@drewtech.com> > > Signed-off-by: Simon Guinot <sguinot@lacie.com> > > Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org> > Sorry for late response... If required, Acked-by: Kukjin Kim <kgene.kim@samsung.com> Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd. > > > --- > > Changes for v2: update patch description (mention the affected SoCs). > > > > arch/arm/mach-davinci/irq.c | 2 +- > > arch/arm/plat-orion/gpio.c | 2 +- > > arch/arm/plat-s5p/irq-gpioint.c | 2 +- > > arch/arm/plat-samsung/irq-uart.c | 2 +- > > include/linux/irq.h | 3 ++- > > kernel/irq/generic-chip.c | 18 ++++++++++++++++-- > > 6 files changed, 22 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm/mach-davinci/irq.c b/arch/arm/mach-davinci/irq.c > > index bfe68ec..d8c1af0 100644 > > --- a/arch/arm/mach-davinci/irq.c > > +++ b/arch/arm/mach-davinci/irq.c > > @@ -53,7 +53,7 @@ davinci_alloc_gc(void __iomem *base, unsigned int > irq_start, unsigned int num) > > > > gc = irq_alloc_generic_chip("AINTC", 1, irq_start, base, handle_edge_irq); > > ct = gc->chip_types; > > - ct->chip.irq_ack = irq_gc_ack; > > + ct->chip.irq_ack = irq_gc_ack_set_bit; > > ct->chip.irq_mask = irq_gc_mask_clr_bit; > > ct->chip.irq_unmask = irq_gc_mask_set_bit; > > > > diff --git a/arch/arm/plat-orion/gpio.c b/arch/arm/plat-orion/gpio.c > > index 5b4fffa..41ab97e 100644 > > --- a/arch/arm/plat-orion/gpio.c > > +++ b/arch/arm/plat-orion/gpio.c > > @@ -432,7 +432,7 @@ void __init orion_gpio_init(int gpio_base, int ngpio, > > ct->regs.mask = ochip->mask_offset + GPIO_EDGE_MASK_OFF; > > ct->regs.ack = GPIO_EDGE_CAUSE_OFF; > > ct->type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING; > > - ct->chip.irq_ack = irq_gc_ack; > > + ct->chip.irq_ack = irq_gc_ack_clr_bit; > > ct->chip.irq_mask = irq_gc_mask_clr_bit; > > ct->chip.irq_unmask = irq_gc_mask_set_bit; > > ct->chip.irq_set_type = gpio_irq_set_type; > > diff --git a/arch/arm/plat-s5p/irq-gpioint.c b/arch/arm/plat-s5p/irq-gpioint.c > > index 135abda..327ab9f 100644 > > --- a/arch/arm/plat-s5p/irq-gpioint.c > > +++ b/arch/arm/plat-s5p/irq-gpioint.c > > @@ -152,7 +152,7 @@ static __init int s5p_gpioint_add(struct s3c_gpio_chip > *chip) > > if (!gc) > > return -ENOMEM; > > ct = gc->chip_types; > > - ct->chip.irq_ack = irq_gc_ack; > > + ct->chip.irq_ack = irq_gc_ack_set_bit; > > ct->chip.irq_mask = irq_gc_mask_set_bit; > > ct->chip.irq_unmask = irq_gc_mask_clr_bit; > > ct->chip.irq_set_type = s5p_gpioint_set_type, > > diff --git a/arch/arm/plat-samsung/irq-uart.c b/arch/arm/plat-samsung/irq-uart.c > > index 32582c0..0e46588 100644 > > --- a/arch/arm/plat-samsung/irq-uart.c > > +++ b/arch/arm/plat-samsung/irq-uart.c > > @@ -55,7 +55,7 @@ static void __init s3c_init_uart_irq(struct s3c_uart_irq *uirq) > > gc = irq_alloc_generic_chip("s3c-uart", 1, uirq->base_irq, reg_base, > > handle_level_irq); > > ct = gc->chip_types; > > - ct->chip.irq_ack = irq_gc_ack; > > + ct->chip.irq_ack = irq_gc_ack_set_bit; > > ct->chip.irq_mask = irq_gc_mask_set_bit; > > ct->chip.irq_unmask = irq_gc_mask_clr_bit; > > ct->regs.ack = S3C64XX_UINTP; > > diff --git a/include/linux/irq.h b/include/linux/irq.h > > index 8b45384..baa397e 100644 > > --- a/include/linux/irq.h > > +++ b/include/linux/irq.h > > @@ -676,7 +676,8 @@ void irq_gc_mask_disable_reg(struct irq_data *d); > > void irq_gc_mask_set_bit(struct irq_data *d); > > void irq_gc_mask_clr_bit(struct irq_data *d); > > void irq_gc_unmask_enable_reg(struct irq_data *d); > > -void irq_gc_ack(struct irq_data *d); > > +void irq_gc_ack_set_bit(struct irq_data *d); > > +void irq_gc_ack_clr_bit(struct irq_data *d); > > void irq_gc_mask_disable_reg_and_ack(struct irq_data *d); > > void irq_gc_eoi(struct irq_data *d); > > int irq_gc_set_wake(struct irq_data *d, unsigned int on); > > diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c > > index 31a9db7..3a2cab4 100644 > > --- a/kernel/irq/generic-chip.c > > +++ b/kernel/irq/generic-chip.c > > @@ -101,10 +101,10 @@ void irq_gc_unmask_enable_reg(struct irq_data *d) > > } > > > > /** > > - * irq_gc_ack - Ack pending interrupt > > + * irq_gc_ack_set_bit - Ack pending interrupt via setting bit > > * @d: irq_data > > */ > > -void irq_gc_ack(struct irq_data *d) > > +void irq_gc_ack_set_bit(struct irq_data *d) > > { > > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > u32 mask = 1 << (d->irq - gc->irq_base); > > @@ -115,6 +115,20 @@ void irq_gc_ack(struct irq_data *d) > > } > > > > /** > > + * irq_gc_ack_clr_bit - Ack pending interrupt via clearing bit > > + * @d: irq_data > > + */ > > +void irq_gc_ack_clr_bit(struct irq_data *d) > > +{ > > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > + u32 mask = ~(1 << (d->irq - gc->irq_base)); > > + > > + irq_gc_lock(gc); > > + irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); > > + irq_gc_unlock(gc); > > +} > > + > > +/** > > * irq_gc_mask_disable_reg_and_ack- Mask and ack pending interrupt > > * @d: irq_data > > */ > > -- > > 1.7.5.1 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants 2011-07-04 14:33 ` [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants Simon Guinot 2011-07-04 14:48 ` [PATCH v2] genirq: replace irq_gc_ack() with {set, clr}_bit variants Nicolas Pitre @ 2011-07-06 15:31 ` Simon Guinot 1 sibling, 0 replies; 16+ messages in thread From: Simon Guinot @ 2011-07-06 15:31 UTC (permalink / raw) To: linux-arm-kernel Hi Thomas, On Mon, Jul 04, 2011 at 04:33:19PM +0200, Simon Guinot wrote: > From: Simon Guinot <sguinot@lacie.com> > > Depending on the device, interrupts acknowledgement is done by setting > or by clearing a dedicated register. Replace irq_gc_ack() with some > {set,clr}_bit variants allows to handle both cases. > > Note that this patch affects the following SoCs: Davinci, Samsung and > Orion. Except for this last, the change is minor: irq_gc_ack() is just > renamed into irq_gc_ack_set_bit(). > > For the Orion SoCs, the edge GPIO interrupts support is currently > broken. irq_gc_ack() try to acknowledge a such interrupt by setting > the corresponding cause register bit. The Orion GPIO device expect the > opposite. To fix this issue, the irq_gc_ack_clr_bit() variant is used. > > Tested on Network Space v2. > > Reported-by: Joey Oravec <joravec@drewtech.com> > Signed-off-by: Simon Guinot <sguinot@lacie.com> > --- > Changes for v2: update patch description (mention the affected SoCs). > > arch/arm/mach-davinci/irq.c | 2 +- > arch/arm/plat-orion/gpio.c | 2 +- > arch/arm/plat-s5p/irq-gpioint.c | 2 +- > arch/arm/plat-samsung/irq-uart.c | 2 +- > include/linux/irq.h | 3 ++- > kernel/irq/generic-chip.c | 18 ++++++++++++++++-- > 6 files changed, 22 insertions(+), 7 deletions(-) Please, apply this patch. Regards, Simon -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 198 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110706/dd8a0d68/attachment.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-03 12:46 ` saeed bishara 2011-07-04 14:33 ` [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants Simon Guinot @ 2011-07-05 15:37 ` Joey Oravec 2011-07-06 16:18 ` Simon Guinot 1 sibling, 1 reply; 16+ messages in thread From: Joey Oravec @ 2011-07-05 15:37 UTC (permalink / raw) To: linux-arm-kernel On 7/3/2011 8:46 AM, saeed bishara wrote: >> It doesn't seems the code was behaving differently before 3.0-rc. I mean >> before the MPP and GPIO consolidation work. It was ? > that's right, this issue was before the consolidation work. Agreed. I only meant to say that the MPP code is broken for mv78200 in 3.0-rc5, so the maintainers should consider a short-term fix of disabling the checks in orion_gpio_is_valid(). >> Does hardware allows a configuration like MPP0_GE0_COL and MPP32_GPIO >> simultaneously ? > yes, this is possible. Agreed. MPP is the pin that can be mux'ed to 7 resources. GPIO is just one of those resources. Yes the hardware supports your example. Most resources can be routed to several different pins to allow flexibility in board design. Since MPP is just a pin-mux, it's possible for Marvell to build an SoC in the future that can route a few different GPIOs onto a given MPP pin. It seems like any solution should account for this. >> If not (and only if not), the problem is simply mapping a MPP number on >> the correct GPIO number. Isn't it ? >> >> In that case, what about using the MPPxx_GPIO definitions (found in >> mach-mv78xx0/mpp.h) to map the right GPIO number ? >> >> For example: >> >> #define MPP32_GPIO MPP(32, 0x1, 1, 1, 1) >> >> could be replaced by: >> >> #define MPP32_GPIO MPP(0, 0x1, 1, 1, 1) > that won't work because the num (first number) is used as the mpp > index. see orion_mpp_conf() implementation. > I think the solution is to extend the MPP define to include the > explicit gpio number, so it will look like: > #define MPP(_num, _gpio _sel, _in, _out, _78100_A0) That could work as long as a value is reserved to mean "not gpio" like: #define MPP0_GPIO MPP(0, 0, 0x0, 1, 1, 1) #define MPP0_GE0_COL MPP(0, -1, 0x1, 1, 0, 1) Note that orion_gpio_set_valid() and orion_gpio_is_valid() would both need rework. The functions need to handle that a GPIO can be mux'ed onto any MPP pin. I described this problem in another email on the thread. There has been some discussion of a pinmux API. Does this API offer any simpler solution? -joey ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-05 15:37 ` plat-orion multi purpose pins problem for mv78200 Joey Oravec @ 2011-07-06 16:18 ` Simon Guinot 2011-07-06 16:32 ` saeed bishara 2011-07-06 18:49 ` Joey Oravec 0 siblings, 2 replies; 16+ messages in thread From: Simon Guinot @ 2011-07-06 16:18 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jul 05, 2011 at 11:37:57AM -0400, Joey Oravec wrote: > On 7/3/2011 8:46 AM, saeed bishara wrote: > >>It doesn't seems the code was behaving differently before 3.0-rc. I mean > >>before the MPP and GPIO consolidation work. It was ? > >that's right, this issue was before the consolidation work. > Agreed. I only meant to say that the MPP code is broken for mv78200 > in 3.0-rc5, so the maintainers should consider a short-term fix of > disabling the checks in orion_gpio_is_valid(). And break code relying on the orion_gpio_is_valid() correctness ? IMHO, this kind of workaround is just not acceptable. > > >>Does hardware allows a configuration like MPP0_GE0_COL and MPP32_GPIO > >>simultaneously ? > >yes, this is possible. > Agreed. MPP is the pin that can be mux'ed to 7 resources. GPIO is > just one of those resources. Yes the hardware supports your example. > Most resources can be routed to several different pins to allow > flexibility in board design. > > Since MPP is just a pin-mux, it's possible for Marvell to build an > SoC in the future that can route a few different GPIOs onto a given > MPP pin. It seems like any solution should account for this. > > >>If not (and only if not), the problem is simply mapping a MPP number on > >>the correct GPIO number. Isn't it ? > >> > >>In that case, what about using the MPPxx_GPIO definitions (found in > >>mach-mv78xx0/mpp.h) to map the right GPIO number ? > >> > >>For example: > >> > >>#define MPP32_GPIO MPP(32, 0x1, 1, 1, 1) > >> > >>could be replaced by: > >> > >>#define MPP32_GPIO MPP(0, 0x1, 1, 1, 1) > >that won't work because the num (first number) is used as the mpp > >index. see orion_mpp_conf() implementation. > >I think the solution is to extend the MPP define to include the > >explicit gpio number, so it will look like: > >#define MPP(_num, _gpio _sel, _in, _out, _78100_A0) > That could work as long as a value is reserved to mean "not gpio" like: > > #define MPP0_GPIO MPP(0, 0, 0x0, 1, 1, 1) > #define MPP0_GE0_COL MPP(0, -1, 0x1, 1, 0, 1) Maybe we could add a function pointer to the parameter list for orion_mpp_conf(). Something like: int (*mpp_to_gpio) (unsigned int num). If given to orion_mpp_conf(), the function would be used to perform the MPP to GPIO conversion. And if NULL, a 1:1 GPIO to MPP mapping would be used (current code). A such solution would let almost unchanged the code found in mach-{kirkwood,orion5x,dove}. > > Note that orion_gpio_set_valid() and orion_gpio_is_valid() would > both need rework. The functions need to handle that a GPIO can be > mux'ed onto any MPP pin. I described this problem in another email > on the thread. I don't understand what's wrong with the GPIO array. Simon -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 198 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110706/7c8a4a63/attachment.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-06 16:18 ` Simon Guinot @ 2011-07-06 16:32 ` saeed bishara 2011-07-06 18:49 ` Joey Oravec 1 sibling, 0 replies; 16+ messages in thread From: saeed bishara @ 2011-07-06 16:32 UTC (permalink / raw) To: linux-arm-kernel >> >>It doesn't seems the code was behaving differently before 3.0-rc. I mean >> >>before the MPP and GPIO consolidation work. It was ? >> >that's right, this issue was before the consolidation work. >> Agreed. I only meant to say that the MPP code is broken for mv78200 >> in 3.0-rc5, so the maintainers should consider a short-term fix of >> disabling the checks in orion_gpio_is_valid(). > > And break code relying on the orion_gpio_is_valid() correctness ? > IMHO, this kind of workaround is just not acceptable. agree > >> >I think the solution is to extend the MPP define to include the >> >explicit gpio number, so it will look like: >> >#define MPP(_num, _gpio _sel, _in, _out, _78100_A0) >> That could work as long as a value is reserved to mean "not gpio" like: >> >> #define MPP0_GPIO ? ? ? ?MPP(0, 0, 0x0, 1, 1, 1) >> #define MPP0_GE0_COL ? ? ? ?MPP(0, -1, 0x1, 1, 0, 1) > > Maybe we could add a function pointer to the parameter list for > orion_mpp_conf(). Something like: int (*mpp_to_gpio) (unsigned int num). > > If given to orion_mpp_conf(), the function would be used to perform the > MPP to GPIO conversion. And if NULL, a 1:1 GPIO to MPP mapping would be > used (current code). this is not enough, because for each mpp element of the mpp_list, we have to know whether the mpp is selected for gpio mode or not. saeed ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-06 16:18 ` Simon Guinot 2011-07-06 16:32 ` saeed bishara @ 2011-07-06 18:49 ` Joey Oravec 2011-07-07 6:40 ` saeed bishara 1 sibling, 1 reply; 16+ messages in thread From: Joey Oravec @ 2011-07-06 18:49 UTC (permalink / raw) To: linux-arm-kernel On 7/6/2011 12:18 PM, Simon Guinot wrote: >> Note that orion_gpio_set_valid() and orion_gpio_is_valid() would >> both need rework. The functions need to handle that a GPIO can be >> mux'ed onto any MPP pin. I described this problem in another email >> on the thread. > I don't understand what's wrong with the GPIO array. I tried to describe the case in my reply: http://lists.arm.linux.org.uk/lurker/message/20110701.215657.7efe0a42.en.html Assume that we've solved the mpp_to_gpio mapping. Then imagine you pass a large array to mv78xx0_mpp_conf() that includes: MPP16_GPIO (this mpp corresponds to GPIO16) MPP47_UNUSED (this mpp corresponds to GPIO16) The code today processes the array in-order. When it processes MPP16_GPIO it will mark the GPIO16 valid. When it processes MPP47_UNUSED it would currently mark GPIO16 invalid. This is a problem because it still assumes a 1:1 relationship. -joey ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-06 18:49 ` Joey Oravec @ 2011-07-07 6:40 ` saeed bishara 2011-07-07 13:49 ` Joey Oravec 0 siblings, 1 reply; 16+ messages in thread From: saeed bishara @ 2011-07-07 6:40 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jul 6, 2011 at 9:49 PM, Joey Oravec <joravec@drewtech.com> wrote: > On 7/6/2011 12:18 PM, Simon Guinot wrote: >>> >>> Note that orion_gpio_set_valid() and orion_gpio_is_valid() would >>> both need rework. The functions need to handle that a GPIO can be >>> mux'ed onto any MPP pin. I described this problem in another email >>> on the thread. >> >> I don't understand what's wrong with the GPIO array. > > I tried to describe the case in my reply: > http://lists.arm.linux.org.uk/lurker/message/20110701.215657.7efe0a42.en.html > > Assume that we've solved the mpp_to_gpio mapping. Then imagine you pass a > large array to mv78xx0_mpp_conf() that includes: > > MPP16_GPIO (this mpp corresponds to GPIO16) > MPP47_UNUSED (this mpp corresponds to GPIO16) > > The code today processes the array in-order. When it processes MPP16_GPIO it > will mark the GPIO16 valid. When it processes MPP47_UNUSED it would > currently mark GPIO16 invalid. This is a problem because it still assumes a > 1:1 relationship. I agree, this is why we need some method to make the orion_mpp_conf() know which mpps are gpios, when that done, then the gpio of MPP47 in your case will not be set as invalid. one option to do that is to assume that mpp with _in == out_ == 0 is not a gpio. so the orion_mpp_conf() will look like this: --- a/arch/arm/plat-orion/mpp.c +++ b/arch/arm/plat-orion/mpp.c @@ -41,6 +41,7 @@ void __init orion_mpp_conf(unsigned int *mpp_list, unsigned int variant_mask, for ( ; *mpp_list; mpp_list++) { unsigned int num = MPP_NUM(*mpp_list); unsigned int sel = MPP_SEL(*mpp_list); + unsigned int gpio_num = MPP_GPIO(*mpp_list); int shift, gpio_mode; if (num > mpp_max) { @@ -64,9 +65,8 @@ void __init orion_mpp_conf(unsigned int *mpp_list, unsigned int variant_mask, gpio_mode |= GPIO_INPUT_OK; if (*mpp_list & MPP_OUTPUT_MASK) gpio_mode |= GPIO_OUTPUT_OK; - if (sel != 0) - gpio_mode = 0; - orion_gpio_set_valid(num, gpio_mode); + if (gpio_mode != 0) + orion_gpio_set_valid(gpio_num, gpio_mode); } and of course this will require that any non gpio mpp will have to have the _in and _out set to 0. saeed ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-07 6:40 ` saeed bishara @ 2011-07-07 13:49 ` Joey Oravec 2011-07-10 13:21 ` saeed bishara 0 siblings, 1 reply; 16+ messages in thread From: Joey Oravec @ 2011-07-07 13:49 UTC (permalink / raw) To: linux-arm-kernel On 7/7/2011 2:40 AM, saeed bishara wrote: >>>> Note that orion_gpio_set_valid() and orion_gpio_is_valid() would >>>> both need rework. The functions need to handle that a GPIO can be >>>> mux'ed onto any MPP pin. I described this problem in another email >>>> on the thread. >>> I don't understand what's wrong with the GPIO array. >> I tried to describe the case in my reply: >> http://lists.arm.linux.org.uk/lurker/message/20110701.215657.7efe0a42.en.html >> >> Assume that we've solved the mpp_to_gpio mapping. Then imagine you pass a >> large array to mv78xx0_mpp_conf() that includes: >> >> MPP16_GPIO (this mpp corresponds to GPIO16) >> MPP47_UNUSED (this mpp corresponds to GPIO16) >> >> The code today processes the array in-order. When it processes MPP16_GPIO it >> will mark the GPIO16 valid. When it processes MPP47_UNUSED it would >> currently mark GPIO16 invalid. This is a problem because it still assumes a >> 1:1 relationship. > I agree, this is why we need some method to make the orion_mpp_conf() > know which mpps are gpios, when that done, then the gpio of MPP47 in > your case will not be set as invalid. > one option to do that is to assume that mpp with _in == out_ == 0 is > not a gpio. so the orion_mpp_conf() will look like this: > --- a/arch/arm/plat-orion/mpp.c > +++ b/arch/arm/plat-orion/mpp.c > @@ -41,6 +41,7 @@ void __init orion_mpp_conf(unsigned int *mpp_list, > unsigned int variant_mask, > for ( ; *mpp_list; mpp_list++) { > unsigned int num = MPP_NUM(*mpp_list); > unsigned int sel = MPP_SEL(*mpp_list); > + unsigned int gpio_num = MPP_GPIO(*mpp_list); > int shift, gpio_mode; > > if (num> mpp_max) { > @@ -64,9 +65,8 @@ void __init orion_mpp_conf(unsigned int *mpp_list, > unsigned int variant_mask, > gpio_mode |= GPIO_INPUT_OK; > if (*mpp_list& MPP_OUTPUT_MASK) > gpio_mode |= GPIO_OUTPUT_OK; > - if (sel != 0) > - gpio_mode = 0; > - orion_gpio_set_valid(num, gpio_mode); > + if (gpio_mode != 0) > + orion_gpio_set_valid(gpio_num, gpio_mode); > } > > > and of course this will require that any non gpio mpp will have to > have the _in and _out set to 0. Yes this proposal is better for mv78xx0 than the current code, but it may fail if you call orion_mpp_conf() multiple times. Imagine you setup the MPP16_GPIO as described, then a subsequent call wants to configure differently and sets: MPP16_UNUSED (this mpp corresponds to GPIO16) Note orion_gpio_set_valid() contains code to mark GPIOs as valid or invalid, but proposed change will only make a call to mark a GPIO as valid. This example would reconfigure the MPP but leave the GPIO marked valid. Eventually, we would need to handle this example to implement the proposed pinmux API. Right? -joey ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-07 13:49 ` Joey Oravec @ 2011-07-10 13:21 ` saeed bishara 2011-07-12 13:36 ` Joey Oravec 0 siblings, 1 reply; 16+ messages in thread From: saeed bishara @ 2011-07-10 13:21 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jul 7, 2011 at 4:49 PM, Joey Oravec <joravec@drewtech.com> wrote: > On 7/7/2011 2:40 AM, saeed bishara wrote: >>>>> >>>>> Note that orion_gpio_set_valid() and orion_gpio_is_valid() would >>>>> both need rework. The functions need to handle that a GPIO can be >>>>> mux'ed onto any MPP pin. I described this problem in another email >>>>> on the thread. >>>> >>>> I don't understand what's wrong with the GPIO array. >>> >>> I tried to describe the case in my reply: >>> >>> http://lists.arm.linux.org.uk/lurker/message/20110701.215657.7efe0a42.en.html >>> >>> Assume that we've solved the mpp_to_gpio mapping. Then imagine you pass a >>> large array to mv78xx0_mpp_conf() that includes: >>> >>> MPP16_GPIO (this mpp corresponds to GPIO16) >>> MPP47_UNUSED (this mpp corresponds to GPIO16) >>> >>> The code today processes the array in-order. When it processes MPP16_GPIO >>> it >>> will mark the GPIO16 valid. When it processes MPP47_UNUSED it would >>> currently mark GPIO16 invalid. This is a problem because it still assumes >>> a >>> 1:1 relationship. >> >> I agree, this is why we need some method to make the orion_mpp_conf() >> know which mpps are gpios, when that done, then the gpio of MPP47 in >> your case will not be set as invalid. >> one option to do that is to assume that mpp with _in == out_ == 0 is >> not a gpio. so the orion_mpp_conf() will look like this: >> --- a/arch/arm/plat-orion/mpp.c >> +++ b/arch/arm/plat-orion/mpp.c >> @@ -41,6 +41,7 @@ void __init orion_mpp_conf(unsigned int *mpp_list, >> unsigned int variant_mask, >> ? ? ? ? for ( ; *mpp_list; mpp_list++) { >> ? ? ? ? ? ? ? ? unsigned int num = MPP_NUM(*mpp_list); >> ? ? ? ? ? ? ? ? unsigned int sel = MPP_SEL(*mpp_list); >> + ? ? ? ? ? ? ? unsigned int gpio_num = MPP_GPIO(*mpp_list); >> ? ? ? ? ? ? ? ? int shift, gpio_mode; >> >> ? ? ? ? ? ? ? ? if (num> ?mpp_max) { >> @@ -64,9 +65,8 @@ void __init orion_mpp_conf(unsigned int *mpp_list, >> unsigned int variant_mask, >> ? ? ? ? ? ? ? ? ? ? ? ? gpio_mode |= GPIO_INPUT_OK; >> ? ? ? ? ? ? ? ? if (*mpp_list& ?MPP_OUTPUT_MASK) >> ? ? ? ? ? ? ? ? ? ? ? ? gpio_mode |= GPIO_OUTPUT_OK; >> - ? ? ? ? ? ? ? if (sel != 0) >> - ? ? ? ? ? ? ? ? ? ? ? gpio_mode = 0; >> - ? ? ? ? ? ? ? orion_gpio_set_valid(num, gpio_mode); >> + ? ? ? ? ? ? ? if (gpio_mode != 0) >> + ? ? ? ? ? ? ? ? ? ? ? orion_gpio_set_valid(gpio_num, gpio_mode); >> ? ? ? ? } >> >> >> and of course this will require that any non gpio mpp will have to >> have the _in and _out set to 0. > > Yes this proposal is better for mv78xx0 than the current code, but it may > fail if you call orion_mpp_conf() multiple times. Imagine you setup the > MPP16_GPIO as described, then a subsequent call wants to configure > differently and sets: the issue that you're describing is theoretical and doesn't happen at least in the boards that supported by the kernel. I don't think it deserve to complicate our code. > > MPP16_UNUSED (this mpp corresponds to GPIO16) > > Note orion_gpio_set_valid() contains code to mark GPIOs as valid or invalid, > but proposed change will only make a call to mark a GPIO as valid. This > example would reconfigure the MPP but leave the GPIO marked valid. > > Eventually, we would need to handle this example to implement the proposed > pinmux API. Right? it makes sense. have you tried it? > > -joey > ^ permalink raw reply [flat|nested] 16+ messages in thread
* plat-orion multi purpose pins problem for mv78200 2011-07-10 13:21 ` saeed bishara @ 2011-07-12 13:36 ` Joey Oravec 0 siblings, 0 replies; 16+ messages in thread From: Joey Oravec @ 2011-07-12 13:36 UTC (permalink / raw) To: linux-arm-kernel On 7/10/2011 9:21 AM, saeed bishara wrote: >>> I agree, this is why we need some method to make the orion_mpp_conf() >>> know which mpps are gpios, when that done, then the gpio of MPP47 in >>> your case will not be set as invalid. >>> one option to do that is to assume that mpp with _in == out_ == 0 is >>> not a gpio. so the orion_mpp_conf() will look like this: >>> --- a/arch/arm/plat-orion/mpp.c >>> +++ b/arch/arm/plat-orion/mpp.c >>> @@ -41,6 +41,7 @@ void __init orion_mpp_conf(unsigned int *mpp_list, >>> unsigned int variant_mask, >>> for ( ; *mpp_list; mpp_list++) { >>> unsigned int num = MPP_NUM(*mpp_list); >>> unsigned int sel = MPP_SEL(*mpp_list); >>> + unsigned int gpio_num = MPP_GPIO(*mpp_list); >>> int shift, gpio_mode; >>> >>> if (num> mpp_max) { >>> @@ -64,9 +65,8 @@ void __init orion_mpp_conf(unsigned int *mpp_list, >>> unsigned int variant_mask, >>> gpio_mode |= GPIO_INPUT_OK; >>> if (*mpp_list& MPP_OUTPUT_MASK) >>> gpio_mode |= GPIO_OUTPUT_OK; >>> - if (sel != 0) >>> - gpio_mode = 0; >>> - orion_gpio_set_valid(num, gpio_mode); >>> + if (gpio_mode != 0) >>> + orion_gpio_set_valid(gpio_num, gpio_mode); >>> } >>> >>> >>> and of course this will require that any non gpio mpp will have to >>> have the _in and _out set to 0. >> Yes this proposal is better for mv78xx0 than the current code, but it may >> fail if you call orion_mpp_conf() multiple times. Imagine you setup the >> MPP16_GPIO as described, then a subsequent call wants to configure >> differently and sets: > the issue that you're describing is theoretical and doesn't happen at > least in the boards that supported by the kernel. > I don't think it deserve to complicate our code. I agree no boards are currently doing this, especially since the functions are marked __init and the code is not capable of it today. But it's reasonable that a driver would want to switch MPPs between a peripheral and a high-impedance state, for example during suspend/resume. >> MPP16_UNUSED (this mpp corresponds to GPIO16) >> >> Note orion_gpio_set_valid() contains code to mark GPIOs as valid or invalid, >> but proposed change will only make a call to mark a GPIO as valid. This >> example would reconfigure the MPP but leave the GPIO marked valid. >> >> Eventually, we would need to handle this example to implement the proposed >> pinmux API. Right? > it makes sense. have you tried it? The latest proposal I could find is from June: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052955.html I have not tried to implement that API yet. The proposal allows changing mux settings at any time, so it seems like the mv78xx0 should be prepared to do that at some point in the future. -joey ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-07-16 3:39 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-01 19:14 plat-orion multi purpose pins problem for mv78200 Joey Oravec
2011-07-01 21:56 ` Joey Oravec
2011-07-02 12:23 ` Simon Guinot
2011-07-03 12:46 ` saeed bishara
2011-07-04 14:33 ` [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants Simon Guinot
2011-07-04 14:48 ` [PATCH v2] genirq: replace irq_gc_ack() with {set, clr}_bit variants Nicolas Pitre
2011-07-16 3:39 ` Kukjin Kim
2011-07-06 15:31 ` [PATCH v2] genirq: replace irq_gc_ack() with {set,clr}_bit variants Simon Guinot
2011-07-05 15:37 ` plat-orion multi purpose pins problem for mv78200 Joey Oravec
2011-07-06 16:18 ` Simon Guinot
2011-07-06 16:32 ` saeed bishara
2011-07-06 18:49 ` Joey Oravec
2011-07-07 6:40 ` saeed bishara
2011-07-07 13:49 ` Joey Oravec
2011-07-10 13:21 ` saeed bishara
2011-07-12 13:36 ` Joey Oravec
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).