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

[Marc Zyngier <marc.zyngier@arm.com>]

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...