| Message ID | 20130627175118.8A9FEFAACF@dev.laptop.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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
Add Chao to comment. -----Original Message----- From: Eric Miao [mailto:eric.y.miao@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
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) */
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(-)