[PATCH v3] irqchip/mmp: only touch the PJ4 & FIQ bits on enable/disable

[PATCH v3] irqchip/mmp: only touch the PJ4 & FIQ bits on enable/disable

[Lubomir Rintel]

On an OLPC XO 1.75 machine, the "security processor" handles the GPIO 71
and 72 interrupts. Don't reset the "route to SP" bit (4).

I'm just assuming the bit 4 is the "route to SP" bit -- it fixes the
SP-based keyboard for me and <mach-mmp/regs-icu.h> defines
ICU_INT_ROUTE_SP_IRQ to be 1 << 4. When asked for a data sheet, Marvell
was not helpful.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>

---
Changes since v2:
- Correct subsystem maintainers on Cc (irqchip)

Changes since v1:
- Adjusted wording & ack from Pavel

 drivers/irqchip/irq-mmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
index 25f32e1d7764..1ed38f9f1d0a 100644
--- a/drivers/irqchip/irq-mmp.c
+++ b/drivers/irqchip/irq-mmp.c
@@ -190,7 +190,7 @@ static const struct mmp_intc_conf mmp_conf = {
 static const struct mmp_intc_conf mmp2_conf = {
 	.conf_enable	= 0x20,
 	.conf_disable	= 0x0,
-	.conf_mask	= 0x7f,
+	.conf_mask	= 0x60,
 };
 
 static void __exception_irq_entry mmp_handle_irq(struct pt_regs *regs)
-- 
2.19.2

Re: [PATCH v3] irqchip/mmp: only touch the PJ4 & FIQ bits on enable/disable

[Marc Zyngier]

On 19/12/2018 17:28, Lubomir Rintel wrote:
> On an OLPC XO 1.75 machine, the "security processor" handles the GPIO 71
> and 72 interrupts. Don't reset the "route to SP" bit (4).
> 
> I'm just assuming the bit 4 is the "route to SP" bit -- it fixes the
> SP-based keyboard for me and <mach-mmp/regs-icu.h> defines
> ICU_INT_ROUTE_SP_IRQ to be 1 << 4. When asked for a data sheet, Marvell
> was not helpful.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> Changes since v2:
> - Correct subsystem maintainers on Cc (irqchip)
> 
> Changes since v1:
> - Adjusted wording & ack from Pavel
> 
>  drivers/irqchip/irq-mmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
> index 25f32e1d7764..1ed38f9f1d0a 100644
> --- a/drivers/irqchip/irq-mmp.c
> +++ b/drivers/irqchip/irq-mmp.c
> @@ -190,7 +190,7 @@ static const struct mmp_intc_conf mmp_conf = {
>  static const struct mmp_intc_conf mmp2_conf = {
>  	.conf_enable	= 0x20,
>  	.conf_disable	= 0x0,
> -	.conf_mask	= 0x7f,
> +	.conf_mask	= 0x60,

You seem to have identified that ICU_INT_ROUTE_PJ4_IRQ and
ICU_INT_ROUTE_PJ4_FIQ bits are the only ones to be touched. So why don't
you use these constants? This number soup is quite unhealthy.

It'd be good to Cc some of the folks who initially wrote this code
(Haojian Zhuang, Eric Miao -- assuming they are still around) and get
some testing on a non OLPC platform, just to make sure there is no
regression due to this. I have the nagging feeling that this could be a
platform specific thing rather than a universal setting.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3] irqchip/mmp: only touch the PJ4 & FIQ bits on enable/disable

[Lubomir Rintel]

On Wed, 2018-12-19 at 18:29 +0000, Marc Zyngier wrote:
> On 19/12/2018 17:28, Lubomir Rintel wrote:
> > On an OLPC XO 1.75 machine, the "security processor" handles the GPIO 71
> > and 72 interrupts. Don't reset the "route to SP" bit (4).
> > 
> > I'm just assuming the bit 4 is the "route to SP" bit -- it fixes the
> > SP-based keyboard for me and <mach-mmp/regs-icu.h> defines
> > ICU_INT_ROUTE_SP_IRQ to be 1 << 4. When asked for a data sheet, Marvell
> > was not helpful.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > 
> > ---
> > Changes since v2:
> > - Correct subsystem maintainers on Cc (irqchip)
> > 
> > Changes since v1:
> > - Adjusted wording & ack from Pavel
> > 
> >  drivers/irqchip/irq-mmp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
> > index 25f32e1d7764..1ed38f9f1d0a 100644
> > --- a/drivers/irqchip/irq-mmp.c
> > +++ b/drivers/irqchip/irq-mmp.c
> > @@ -190,7 +190,7 @@ static const struct mmp_intc_conf mmp_conf = {
> >  static const struct mmp_intc_conf mmp2_conf = {
> >  	.conf_enable	= 0x20,
> >  	.conf_disable	= 0x0,
> > -	.conf_mask	= 0x7f,
> > +	.conf_mask	= 0x60,
> 
> You seem to have identified that ICU_INT_ROUTE_PJ4_IRQ and
> ICU_INT_ROUTE_PJ4_FIQ bits are the only ones to be touched. So why don't
> you use these constants? This number soup is quite unhealthy.

Yeah, but those #defines live in mach-mmp, so some moving would be
necessary. If you indeed prefer that then I can follow up with a patch
that does that.

> It'd be good to Cc some of the folks who initially wrote this code
> (Haojian Zhuang, Eric Miao -- assuming they are still around) and get
> some testing on a non OLPC platform, just to make sure there is no
> regression due to this. I have the nagging feeling that this could be a
> platform specific thing rather than a universal setting.

They've been Cc'd on previous spins of the patch (and tens of other
mmp-related patches that were in circulation lately), but they never
returned a response. It is safe to assume they're AWOL.

> 
> Thanks,
> 
> 	M.

Thanks
Lubo

Re: [PATCH v3] irqchip/mmp: only touch the PJ4 & FIQ bits on enable/disable

[Marc Zyngier]

On 19/12/2018 18:37, Lubomir Rintel wrote:
> On Wed, 2018-12-19 at 18:29 +0000, Marc Zyngier wrote:
>> On 19/12/2018 17:28, Lubomir Rintel wrote:
>>> On an OLPC XO 1.75 machine, the "security processor" handles the GPIO 71
>>> and 72 interrupts. Don't reset the "route to SP" bit (4).
>>>
>>> I'm just assuming the bit 4 is the "route to SP" bit -- it fixes the
>>> SP-based keyboard for me and <mach-mmp/regs-icu.h> defines
>>> ICU_INT_ROUTE_SP_IRQ to be 1 << 4. When asked for a data sheet, Marvell
>>> was not helpful.
>>>
>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>
>>> ---
>>> Changes since v2:
>>> - Correct subsystem maintainers on Cc (irqchip)
>>>
>>> Changes since v1:
>>> - Adjusted wording & ack from Pavel
>>>
>>>  drivers/irqchip/irq-mmp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
>>> index 25f32e1d7764..1ed38f9f1d0a 100644
>>> --- a/drivers/irqchip/irq-mmp.c
>>> +++ b/drivers/irqchip/irq-mmp.c
>>> @@ -190,7 +190,7 @@ static const struct mmp_intc_conf mmp_conf = {
>>>  static const struct mmp_intc_conf mmp2_conf = {
>>>  	.conf_enable	= 0x20,
>>>  	.conf_disable	= 0x0,
>>> -	.conf_mask	= 0x7f,
>>> +	.conf_mask	= 0x60,
>>
>> You seem to have identified that ICU_INT_ROUTE_PJ4_IRQ and
>> ICU_INT_ROUTE_PJ4_FIQ bits are the only ones to be touched. So why don't
>> you use these constants? This number soup is quite unhealthy.
> 
> Yeah, but those #defines live in mach-mmp, so some moving would be
> necessary. If you indeed prefer that then I can follow up with a patch
> that does that.

Given that nothing in the tree uses these #defines at all, they can be
swiftly moved in one single go.

> 
>> It'd be good to Cc some of the folks who initially wrote this code
>> (Haojian Zhuang, Eric Miao -- assuming they are still around) and get
>> some testing on a non OLPC platform, just to make sure there is no
>> regression due to this. I have the nagging feeling that this could be a
>> platform specific thing rather than a universal setting.
> 
> They've been Cc'd on previous spins of the patch (and tens of other
> mmp-related patches that were in circulation lately), but they never
> returned a response. It is safe to assume they're AWOL.

OK, so what else in the known universe uses the same SoC and runs
mainline? This really needs wider testing if we can't get information
from the MV folks.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3] irqchip/mmp: only touch the PJ4 & FIQ bits on enable/disable

[Lubomir Rintel]

Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 19/12/2018 18:37, Lubomir Rintel wrote:
>> On Wed, 2018-12-19 at 18:29 +0000, Marc Zyngier wrote:
>>> On 19/12/2018 17:28, Lubomir Rintel wrote:
>>>> On an OLPC XO 1.75 machine, the "security processor" handles the GPIO 71
>>>> and 72 interrupts. Don't reset the "route to SP" bit (4).
>>>>
>>>> I'm just assuming the bit 4 is the "route to SP" bit -- it fixes the
>>>> SP-based keyboard for me and <mach-mmp/regs-icu.h> defines
>>>> ICU_INT_ROUTE_SP_IRQ to be 1 << 4. When asked for a data sheet, Marvell
>>>> was not helpful.
>>>>
>>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>>
>>>> ---
>>>> Changes since v2:
>>>> - Correct subsystem maintainers on Cc (irqchip)
>>>>
>>>> Changes since v1:
>>>> - Adjusted wording & ack from Pavel
>>>>
>>>>  drivers/irqchip/irq-mmp.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
>>>> index 25f32e1d7764..1ed38f9f1d0a 100644
>>>> --- a/drivers/irqchip/irq-mmp.c
>>>> +++ b/drivers/irqchip/irq-mmp.c
>>>> @@ -190,7 +190,7 @@ static const struct mmp_intc_conf mmp_conf = {
>>>>  static const struct mmp_intc_conf mmp2_conf = {
>>>>  	.conf_enable	= 0x20,
>>>>  	.conf_disable	= 0x0,
>>>> -	.conf_mask	= 0x7f,
>>>> +	.conf_mask	= 0x60,
>>>
>>> You seem to have identified that ICU_INT_ROUTE_PJ4_IRQ and
>>> ICU_INT_ROUTE_PJ4_FIQ bits are the only ones to be touched. So why don't
>>> you use these constants? This number soup is quite unhealthy.
>> 
>> Yeah, but those #defines live in mach-mmp, so some moving would be
>> necessary. If you indeed prefer that then I can follow up with a patch
>> that does that.
> 
> Given that nothing in the tree uses these #defines at all, they can be
> swiftly moved in one single go.

Will do.

>>> It'd be good to Cc some of the folks who initially wrote this code
>>> (Haojian Zhuang, Eric Miao -- assuming they are still around) and get
>>> some testing on a non OLPC platform, just to make sure there is no
>>> regression due to this. I have the nagging feeling that this could be a
>>> platform specific thing rather than a universal setting.
>> 
>> They've been Cc'd on previous spins of the patch (and tens of other
>> mmp-related patches that were in circulation lately), but they never
>> returned a response. It is safe to assume they're AWOL.
> 
> OK, so what else in the known universe uses the same SoC and runs
> mainline? This really needs wider testing if we can't get information
> from the MV folks.

While I can't possibly know for sure, I think it's fairly sure there are no
users beyond OLPC. The only MMP2 board that would run this was "Brownstone"
(that can't be obtained anymore) and the whole mmp2-dt support was
unbootable until recently without anyone noticing.

I think we're safe here. If anyone with a Brownstone board running mainline
exists (unlikely), linux-next + a few RCs are going to give them a good
chance to test this,

> 
> Thanks,
> 
> 	M.

Cheers,
Lubo