RE: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Kukjin Kim
Doug Anderson wrote:
> 
> Kukjin
> 
> 
> 
Oops, sorry.

> On Thu, Jun 13, 2013 at 4:13 PM, Kukjin Kim  wrote:
> > Doug Anderson wrote:
> >>
> >> Tomasz,
> >>
> >> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa 
> wrote:
> >> >> > BTW, probably we need a similar fixing in the mach-exynos/common.c
> >> >> > file
> >> >> > before pinct기 for distro...
> >> >>
> >> >> Is anyone using the functions in mach-exynos/common.c file anymore?
> I
> >> >> thought that non-dt exynos support was going away and then we could
> >> >> just delete a whole lot of code from that file.
> >> >
> >> > I think Kukjin meant stable kernels that support Exynos boards using
> >> board
> >> > files and without pinctrl. Would make sense to have them fixed as
> well,
> >> I
> >> > guess.
> >>
> > Yes, correct. Thanks, Tomasz.
> >
> >> Ah, makes sense.  Kukjin: do you know of someone who needs this
> >> (someone who is picking up linux-stable updates for exynos)?  I don't
> >> think it's important for ChromeOS for this particular patch.  If
> >> there's someone who needs this to officially land on linux-stable I'd
> >> be happy to review their backport of this patch.
> >>
> > As you know, developing something like Android, Tizen use the stable
> kernel (long-term? I'm not sure) and there was a problem about this issue.
> So I mean, would be fixed for the stable kernel.
> 
> Sure, but do they actually pull in from linux-stable periodically?
> I'd imagine that they have a private tree and that it would be their
> job to backport any fixes onto their kernel.

Right, the projects usually pull the linux-stable kernel when it starts. But as 
far as I know, they pick up some fixes from linux-stable during developing. Or 
for next project, would be better. I'm not sure what version will be used next 
time but it's obvious it will not be latest mainline :-)

Thanks,
- Kukjin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Doug Anderson
Kukjin



On Thu, Jun 13, 2013 at 4:13 PM, Kukjin Kim  wrote:
> Doug Anderson wrote:
>>
>> Tomasz,
>>
>> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa  wrote:
>> >> > BTW, probably we need a similar fixing in the mach-exynos/common.c
>> >> > file
>> >> > before pinct기 for distro...
>> >>
>> >> Is anyone using the functions in mach-exynos/common.c file anymore?  I
>> >> thought that non-dt exynos support was going away and then we could
>> >> just delete a whole lot of code from that file.
>> >
>> > I think Kukjin meant stable kernels that support Exynos boards using
>> board
>> > files and without pinctrl. Would make sense to have them fixed as well,
>> I
>> > guess.
>>
> Yes, correct. Thanks, Tomasz.
>
>> Ah, makes sense.  Kukjin: do you know of someone who needs this
>> (someone who is picking up linux-stable updates for exynos)?  I don't
>> think it's important for ChromeOS for this particular patch.  If
>> there's someone who needs this to officially land on linux-stable I'd
>> be happy to review their backport of this patch.
>>
> As you know, developing something like Android, Tizen use the stable kernel 
> (long-term? I'm not sure) and there was a problem about this issue. So I 
> mean, would be fixed for the stable kernel.

Sure, but do they actually pull in from linux-stable periodically?
I'd imagine that they have a private tree and that it would be their
job to backport any fixes onto their kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Kukjin Kim
Doug Anderson wrote:
> 
> Tomasz,
> 
> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa  wrote:
> >> > BTW, probably we need a similar fixing in the mach-exynos/common.c
> >> > file
> >> > before pinct기 for distro...
> >>
> >> Is anyone using the functions in mach-exynos/common.c file anymore?  I
> >> thought that non-dt exynos support was going away and then we could
> >> just delete a whole lot of code from that file.
> >
> > I think Kukjin meant stable kernels that support Exynos boards using
> board
> > files and without pinctrl. Would make sense to have them fixed as well,
> I
> > guess.
>
Yes, correct. Thanks, Tomasz.

> Ah, makes sense.  Kukjin: do you know of someone who needs this
> (someone who is picking up linux-stable updates for exynos)?  I don't
> think it's important for ChromeOS for this particular patch.  If
> there's someone who needs this to officially land on linux-stable I'd
> be happy to review their backport of this patch.
> 
As you know, developing something like Android, Tizen use the stable kernel 
(long-term? I'm not sure) and there was a problem about this issue. So I mean, 
would be fixed for the stable kernel.

Thanks,
- Kukjin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Doug Anderson
Tomasz,

On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa  wrote:
>> > BTW, probably we need a similar fixing in the mach-exynos/common.c
>> > file
>> > before pinct기 for distro...
>>
>> Is anyone using the functions in mach-exynos/common.c file anymore?  I
>> thought that non-dt exynos support was going away and then we could
>> just delete a whole lot of code from that file.
>
> I think Kukjin meant stable kernels that support Exynos boards using board
> files and without pinctrl. Would make sense to have them fixed as well, I
> guess.

Ah, makes sense.  Kukjin: do you know of someone who needs this
(someone who is picking up linux-stable updates for exynos)?  I don't
think it's important for ChromeOS for this particular patch.  If
there's someone who needs this to officially land on linux-stable I'd
be happy to review their backport of this patch.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Tomasz Figa
On Thursday 13 of June 2013 09:38:33 Doug Anderson wrote:
> Kukjin,
> 
> On Thu, Jun 13, 2013 at 5:04 AM, Kukjin Kim  
wrote:
> > Doug Anderson wrote:
> >> A level-triggered interrupt should be acked after the interrupt line
> >> becomes inactive and before it is unmasked, or else another interrupt
> >> will be immediately triggered.  Acking before or after calling the
> >> handler is not enough.
> >> 
> >> Signed-off-by: Luigi Semenzato 
> >> Signed-off-by: Doug Anderson 
> > 
> > BTW, probably we need a similar fixing in the mach-exynos/common.c
> > file
> > before pinct기 for distro...
> 
> Is anyone using the functions in mach-exynos/common.c file anymore?  I
> thought that non-dt exynos support was going away and then we could
> just delete a whole lot of code from that file.

I think Kukjin meant stable kernels that support Exynos boards using board 
files and without pinctrl. Would make sense to have them fixed as well, I 
guess.

Best regards,
Tomasz

> 
> -Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Tomasz Figa
On Thursday 13 of June 2013 09:34:43 Doug Anderson wrote:
> Tomasz,
> 
> On Thu, Jun 13, 2013 at 3:54 AM, Tomasz Figa  
wrote:
> > Hi Doug,
> > 
> > On Wednesday 12 of June 2013 10:33:19 Doug Anderson wrote:
> >> A level-triggered interrupt should be acked after the interrupt line
> >> becomes inactive and before it is unmasked, or else another interrupt
> >> will be immediately triggered.  Acking before or after calling the
> >> handler is not enough.
> > 
> > Nice catch.
> > 
> > I guess that pinctrl-s3c64xx will need similar fix as well, won't it?
> 
> It needs this whole series of 3, probably.  The mask and unmask need
> the lock and as well as the acking for level interrupts.
> 
> I don't have any way to test that code but it's a pretty simple change
> to make.  Do you want to do it or do you have an idea of someone who
> should?

I'll take care of s3c64xx, probably as a part of my patches finally adding 
DT support for it, as without them the pinctrl-s3c64xx driver is just 
sitting there unused.

> > I think you can eliminate most of the code by doing this following 
way:
> > if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
> > 
> > exynos_gpio_irq_ack(irqd);
> 
> Duh, right.  OK, v2 coming shortly.

Good!

> Thank you for pointing out the
> right way to do this!  :)

You're welcome.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Doug Anderson
Kukjin,

On Thu, Jun 13, 2013 at 5:04 AM, Kukjin Kim  wrote:
> Doug Anderson wrote:
>>
>> A level-triggered interrupt should be acked after the interrupt line
>> becomes inactive and before it is unmasked, or else another interrupt
>> will be immediately triggered.  Acking before or after calling the
>> handler is not enough.
>>
>> Signed-off-by: Luigi Semenzato 
>> Signed-off-by: Doug Anderson 
>
> BTW, probably we need a similar fixing in the mach-exynos/common.c file
> before pinct기 for distro...

Is anyone using the functions in mach-exynos/common.c file anymore?  I
thought that non-dt exynos support was going away and then we could
just delete a whole lot of code from that file.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Doug Anderson
Tomasz,

On Thu, Jun 13, 2013 at 3:54 AM, Tomasz Figa  wrote:
> Hi Doug,
>
> On Wednesday 12 of June 2013 10:33:19 Doug Anderson wrote:
>> A level-triggered interrupt should be acked after the interrupt line
>> becomes inactive and before it is unmasked, or else another interrupt
>> will be immediately triggered.  Acking before or after calling the
>> handler is not enough.
>
> Nice catch.
>
> I guess that pinctrl-s3c64xx will need similar fix as well, won't it?

It needs this whole series of 3, probably.  The mask and unmask need
the lock and as well as the acking for level interrupts.

I don't have any way to test that code but it's a pretty simple change
to make.  Do you want to do it or do you have an idea of someone who
should?


> I think you can eliminate most of the code by doing this following way:
>
> if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
> exynos_gpio_irq_ack(irqd);

Duh, right.  OK, v2 coming shortly.  Thank you for pointing out the
right way to do this!  :)

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Linus Walleij
On Wed, Jun 12, 2013 at 7:33 PM, Doug Anderson  wrote:

> A level-triggered interrupt should be acked after the interrupt line
> becomes inactive and before it is unmasked, or else another interrupt
> will be immediately triggered.  Acking before or after calling the
> handler is not enough.
>
> Signed-off-by: Luigi Semenzato 
> Signed-off-by: Doug Anderson 

I'm holding this off until Tomasz' comments are addressed.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Kukjin Kim
Doug Anderson wrote:
> 
> A level-triggered interrupt should be acked after the interrupt line
> becomes inactive and before it is unmasked, or else another interrupt
> will be immediately triggered.  Acking before or after calling the
> handler is not enough.
> 
> Signed-off-by: Luigi Semenzato 
> Signed-off-by: Doug Anderson 

BTW, probably we need a similar fixing in the mach-exynos/common.c file
before pinct기 for distro...

Anyway,
Acked-by: Kukjin Kim 

Thanks,
- Kukjin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

2013-06-13 Thread Tomasz Figa
Hi Doug,

On Wednesday 12 of June 2013 10:33:19 Doug Anderson wrote:
> A level-triggered interrupt should be acked after the interrupt line
> becomes inactive and before it is unmasked, or else another interrupt
> will be immediately triggered.  Acking before or after calling the
> handler is not enough.

Nice catch.

I guess that pinctrl-s3c64xx will need similar fix as well, won't it?

Also one comment inline.

> Signed-off-by: Luigi Semenzato 
> Signed-off-by: Doug Anderson 
> ---
>  drivers/pinctrl/pinctrl-exynos.c | 42
>  1 file changed, 42
> insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-exynos.c
> b/drivers/pinctrl/pinctrl-exynos.c index c0729a3..67b7a27 100644
> --- a/drivers/pinctrl/pinctrl-exynos.c
> +++ b/drivers/pinctrl/pinctrl-exynos.c
> @@ -81,11 +81,32 @@ static void exynos_gpio_irq_unmask(struct irq_data
> *irqd) struct samsung_pin_bank *bank =
> irq_data_get_irq_chip_data(irqd); struct samsung_pinctrl_drv_data *d =
> bank->drvdata;
>   unsigned long reg_mask = d->ctrl->geint_mask + bank->eint_offset;
> + unsigned long reg_con = d->ctrl->geint_con + bank->eint_offset;
> + unsigned int pin = irqd->hwirq;
> + unsigned int shift = EXYNOS_EINT_CON_LEN * pin;
> + unsigned int con, trig_type;
>   unsigned long mask;
>   unsigned long flags;
> 
>   spin_lock_irqsave(&bank->slock, flags);
> 
> + /*
> +  * Ack level interrupts right before unmask
> +  *
> +  * If we don't do this we'll get a double-interrupt.  Level 
triggered
> +  * interrupts must not fire an interrupt if the level is not
> +  * _currently_ active, even if it was active while the interrupt 
was
> +  * masked.
> +  */
> + con = readl(d->virt_base + reg_con);
> + trig_type = (con >> shift) & EXYNOS_EINT_CON_MASK;
> + switch (trig_type) {
> + case EXYNOS_EINT_LEVEL_HIGH:
> + case EXYNOS_EINT_LEVEL_LOW:
> + exynos_gpio_irq_ack(irqd);
> + break;
> + }

I think you can eliminate most of the code by doing this following way:

if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
exynos_gpio_irq_ack(irqd);

Best regards,
Tomasz

> +
>   mask = readl(d->virt_base + reg_mask);
>   mask &= ~(1 << irqd->hwirq);
>   writel(mask, d->virt_base + reg_mask);
> @@ -299,11 +320,32 @@ static void exynos_wkup_irq_unmask(struct irq_data
> *irqd) struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
> struct samsung_pinctrl_drv_data *d = b->drvdata;
>   unsigned long reg_mask = d->ctrl->weint_mask + b->eint_offset;
> + unsigned long reg_con = d->ctrl->weint_con + b->eint_offset;
> + unsigned int pin = irqd->hwirq;
> + unsigned long shift = EXYNOS_EINT_CON_LEN * pin;
> + unsigned long con, trig_type;
>   unsigned long mask;
>   unsigned long flags;
> 
>   spin_lock_irqsave(&b->slock, flags);
> 
> + /*
> +  * Ack level interrupts right before unmask
> +  *
> +  * If we don't do this we'll get a double-interrupt.  Level 
triggered
> +  * interrupts must not fire an interrupt if the level is not
> +  * _currently_ active, even if it was active while the interrupt 
was
> +  * masked.
> +  */
> + con = readl(d->virt_base + reg_con);
> + trig_type = (con >> shift) & EXYNOS_EINT_CON_MASK;
> + switch (trig_type) {
> + case EXYNOS_EINT_LEVEL_HIGH:
> + case EXYNOS_EINT_LEVEL_LOW:
> + exynos_wkup_irq_ack(irqd);
> + break;
> + }
> +
>   mask = readl(d->virt_base + reg_mask);
>   mask &= ~(1 << irqd->hwirq);
>   writel(mask, d->virt_base + reg_mask);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/