[PATCH] mmp: irq: Don't clear unused interrupt enable bits

[PATCH] mmp: irq: Don't clear unused interrupt enable bits

[Daniel Drake]

When enabling/masking interrupts, the existing MMP2 code clears
a mask of 0x7f in the interrupt enable register.
The lower 5 bits here are not directly used by Linux:
 0:3 is interrupt priority
 4 determines whether the interrupt gets delivered to the Security Processor

In the OLPC case, a special firmware is running on the SP, and we do not
want to mask it from receiving the interrupts it has already unmasked.

Refine the mask to only deal with the bits that are of specific interest
to Linux running on the main CPU.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 arch/arm/mach-mmp/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-mmp/irq.c b/arch/arm/mach-mmp/irq.c
index ac92433..21cc0b5 100644
--- a/arch/arm/mach-mmp/irq.c
+++ b/arch/arm/mach-mmp/irq.c
@@ -190,7 +190,7 @@ static struct mmp_intc_conf mmp_conf = {
 static struct mmp_intc_conf mmp2_conf = {
 	.conf_enable	= 0x20,
 	.conf_disable	= 0x0,
-	.conf_mask	= 0x7f,
+	.conf_mask	= 0x60,
 };
 
 /* MMP (ARMv5) */
-- 
1.8.1.4

[PATCH] mmp: irq: Don't clear unused interrupt enable bits

[Eric Miao]

On Fri, Jun 28, 2013 at 1:51 AM, Daniel Drake <dsd@laptop.org> wrote:
>
> When enabling/masking interrupts, the existing MMP2 code clears
> a mask of 0x7f in the interrupt enable register.
> The lower 5 bits here are not directly used by Linux:
>  0:3 is interrupt priority
>  4 determines whether the interrupt gets delivered to the Security Processor
>
> In the OLPC case, a special firmware is running on the SP, and we do not
> want to mask it from receiving the interrupts it has already unmasked.
>
> Refine the mask to only deal with the bits that are of specific interest
> to Linux running on the main CPU.

Is there any way to check this at run-time or at least by boot parameters?
Decide whether the interrupt gets delivered to SP seems to be a choice
that the main CPU should be doing, as well as the interrupt priority.

And this is really all about bit 4 right?

>
>
> Signed-off-by: Daniel Drake <dsd@laptop.org>
> ---
>  arch/arm/mach-mmp/irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-mmp/irq.c b/arch/arm/mach-mmp/irq.c
> index ac92433..21cc0b5 100644
> --- a/arch/arm/mach-mmp/irq.c
> +++ b/arch/arm/mach-mmp/irq.c
> @@ -190,7 +190,7 @@ static struct mmp_intc_conf mmp_conf = {
>  static struct mmp_intc_conf mmp2_conf = {
>         .conf_enable    = 0x20,
>         .conf_disable   = 0x0,
> -       .conf_mask      = 0x7f,
> +       .conf_mask      = 0x60,
>  };
>
>  /* MMP (ARMv5) */
> --
> 1.8.1.4
>

[PATCH] mmp: irq: Don't clear unused interrupt enable bits

[Daniel Drake]

On Thu, Jun 27, 2013 at 11:15 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> Is there any way to check this at run-time or at least by boot parameters?

The only thing that occurs to me would be checking for the device tree
node that corresponds to our input serio. But doing that from platform
code would be horrible, and it would also be nasty doing it from the
input driver.

> Decide whether the interrupt gets delivered to SP seems to be a choice
> that the main CPU should be doing, as well as the interrupt priority.

Incidentally the main CPU does make these choices - just before Linux
has booted.

While Linux has no interaction with the SP (it is not even aware of
it), I would say not stomping on its configuration is an OK policy to
have.

> And this is really all about bit 4 right?

Yes, bit 4 is the crucial one, it is required for the OLPC inbuilt
keyboard/touchpad to work.

However on numerous occasions during interactions with Marvell they
have stressed the importance of setting interrupt priorities on
interrupts used for wakeups. We don't think it makes a difference for
the issues that we have seen, but we do not ignore the request. As
that cannot be done in Linux, we do it in the firmware, and it seems
like while Linux does not work with interrupt priorities it should not
overwite any earlier setup.

Daniel

[PATCH] mmp: irq: Don't clear unused interrupt enable bits

[Eric Miao]

On Fri, Jun 28, 2013 at 10:24 PM, Daniel Drake <dsd@laptop.org> wrote:
> On Thu, Jun 27, 2013 at 11:15 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>> Is there any way to check this at run-time or at least by boot parameters?
>
> The only thing that occurs to me would be checking for the device tree
> node that corresponds to our input serio. But doing that from platform
> code would be horrible, and it would also be nasty doing it from the
> input driver.

I think that should be OK and clean, e.g.

  if (of_property_read_bool(np, "irq-route-to-sp")) {
     ....
  }

I'm fine with leaving these bits out as your patch does, the only concern
is some other code may have a chance to depend on this, and may
benefit from this boot configuration flexibility as well.

>
>> Decide whether the interrupt gets delivered to SP seems to be a choice
>> that the main CPU should be doing, as well as the interrupt priority.
>
> Incidentally the main CPU does make these choices - just before Linux
> has booted.
>
> While Linux has no interaction with the SP (it is not even aware of
> it), I would say not stomping on its configuration is an OK policy to
> have.
>
>> And this is really all about bit 4 right?
>
> Yes, bit 4 is the crucial one, it is required for the OLPC inbuilt
> keyboard/touchpad to work.
>
> However on numerous occasions during interactions with Marvell they
> have stressed the importance of setting interrupt priorities on
> interrupts used for wakeups. We don't think it makes a difference for
> the issues that we have seen, but we do not ignore the request. As
> that cannot be done in Linux, we do it in the firmware, and it seems
> like while Linux does not work with interrupt priorities it should not
> overwite any earlier setup.

Haojian & Mingliang, could you help check this is the case, and that no
known driver (including those for upstream) is depending on this behavior?

>
> Daniel

[PATCH] mmp: irq: Don't clear unused interrupt enable bits

[Daniel Drake]

On Fri, Jun 28, 2013 at 10:55 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> I'm fine with leaving these bits out as your patch does, the only concern
> is some other code may have a chance to depend on this, and may
> benefit from this boot configuration flexibility as well.

I would prefer to leave them out for simplicity of the driver.
However, I'm happy to weigh in if/when any future need comes up where
Linux needs to be directly involved with SP setup.

Daniel

[PATCH] mmp: irq: Don't clear unused interrupt enable bits

[Mingliang Hu]

Add Chao to comment.

-----Original Message-----
From: Eric Miao [mailto:eric.y.miao at gmail.com] 
Sent: 2013?6?29? 12:55
To: Daniel Drake
Cc: Haojian Zhuang; linux-arm-kernel; paul fox; Mingliang Hu
Subject: Re: [PATCH] mmp: irq: Don't clear unused interrupt enable bits

On Fri, Jun 28, 2013 at 10:24 PM, Daniel Drake <dsd@laptop.org> wrote:
> On Thu, Jun 27, 2013 at 11:15 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>> Is there any way to check this at run-time or at least by boot parameters?
>
> The only thing that occurs to me would be checking for the device tree 
> node that corresponds to our input serio. But doing that from platform 
> code would be horrible, and it would also be nasty doing it from the 
> input driver.

I think that should be OK and clean, e.g.

  if (of_property_read_bool(np, "irq-route-to-sp")) {
     ....
  }

I'm fine with leaving these bits out as your patch does, the only concern is some other code may have a chance to depend on this, and may benefit from this boot configuration flexibility as well.

>
>> Decide whether the interrupt gets delivered to SP seems to be a 
>> choice that the main CPU should be doing, as well as the interrupt priority.
>
> Incidentally the main CPU does make these choices - just before Linux 
> has booted.
>
> While Linux has no interaction with the SP (it is not even aware of 
> it), I would say not stomping on its configuration is an OK policy to 
> have.
>
>> And this is really all about bit 4 right?
>
> Yes, bit 4 is the crucial one, it is required for the OLPC inbuilt 
> keyboard/touchpad to work.
>
> However on numerous occasions during interactions with Marvell they 
> have stressed the importance of setting interrupt priorities on 
> interrupts used for wakeups. We don't think it makes a difference for 
> the issues that we have seen, but we do not ignore the request. As 
> that cannot be done in Linux, we do it in the firmware, and it seems 
> like while Linux does not work with interrupt priorities it should not 
> overwite any earlier setup.

Haojian & Mingliang, could you help check this is the case, and that no known driver (including those for upstream) is depending on this behavior?

>
> Daniel