Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1657289 > unrolled thread

Re: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support for the NMI in A64 R_INTC

Started byChen-Yu Tsai <wens@csie.org>
First post2017-06-05 08:00 +0200
Last post2017-06-05 10:10 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support  for the NMI in A64 R_INTC Chen-Yu Tsai <wens@csie.org> - 2017-06-05 08:00 +0200
    Re: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support  for the NMI in A64 R_INTC Marc Zyngier <marc.zyngier@arm.com> - 2017-06-05 10:00 +0200
      Re: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support for the NMI in A64 R_INTC Icenowy Zheng <icenowy@aosc.io> - 2017-06-05 10:00 +0200
        Re: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support  for the NMI in A64 R_INTC Marc Zyngier <marc.zyngier@arm.com> - 2017-06-05 10:10 +0200

#1657289 — Re: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support for the NMI in A64 R_INTC

FromChen-Yu Tsai <wens@csie.org>
Date2017-06-05 08:00 +0200
SubjectRe: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support for the NMI in A64 R_INTC
Message-ID<tOTdv-7jA-7@gated-at.bofh.it>
Hi Marc,

On Mon, May 22, 2017 at 10:25 PM, Chen-Yu Tsai <wens@csie.org> wrote:
> On Mon, May 22, 2017 at 5:41 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
>>
>>
>> 于 2017年5月22日 GMT+08:00 下午5:39:22, Marc Zyngier <marc.zyngier@arm.com> 写到:
>>>On 18/05/17 08:16, Icenowy Zheng wrote:
>>>> Add support for the newly imported compatible for the A64 R_INTC in
>>>> irq-sunxi-nmi driver.
>>>>
>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>> ---
>>>> Changes in v5:
>>>> - Fix A64 R_INTC compatible.
>>>>
>>>>  drivers/irqchip/irq-sunxi-nmi.c | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-sunxi-nmi.c
>>>b/drivers/irqchip/irq-sunxi-nmi.c
>>>> index 668730c5cb66..5559c1d593bf 100644
>>>> --- a/drivers/irqchip/irq-sunxi-nmi.c
>>>> +++ b/drivers/irqchip/irq-sunxi-nmi.c
>>>> @@ -56,6 +56,12 @@ static struct sunxi_sc_nmi_reg_offs sun9i_reg_offs
>>>= {
>>>>      .enable = 0x04,
>>>>  };
>>>>
>>>> +static struct sunxi_sc_nmi_reg_offs sun50i_reg_offs = {
>>>> +    .ctrl   = 0x0c,
>>>> +    .pend   = 0x10,
>>>> +    .enable = 0x40,
>>>> +};
>>>> +
>>>
>>>Magic values? Even if no #define is provided, a pointer to the
>>>corresponding documentation would be appreciated (assuming
>>>documentation
>>>exists).
>>
>> No documents is available for A64 R_INTC.
>
> No code either. In Allwinner's BSP, the interrupts for the PMICs go
> through the (closed source) OpenRISC firmware, so there's no driver
> for it in the kernel.
>
> The registers line up with the old interrupt controller from the A10,
> but it seems only the NMI interrupt is wired up.

Is this OK? Or do you want Icenowy to respin a version with defines?

>>>
>>>>  static inline void sunxi_sc_nmi_write(struct irq_chip_generic *gc,
>>>u32 off,
>>>>                                    u32 val)
>>>>  {
>>>> @@ -220,3 +226,10 @@ static int __init sun9i_nmi_irq_init(struct
>>>device_node *node,
>>>>      return sunxi_sc_nmi_irq_init(node, &sun9i_reg_offs);
>>>>  }
>>>>  IRQCHIP_DECLARE(sun9i_nmi, "allwinner,sun9i-a80-nmi",
>>>sun9i_nmi_irq_init);
>>>> +
>>>> +static int __init sun50i_nmi_irq_init(struct device_node *node,
>>>> +                                 struct device_node *parent)
>>>> +{
>>>> +    return sunxi_sc_nmi_irq_init(node, &sun50i_reg_offs);
>>>> +}
>>>> +IRQCHIP_DECLARE(sun50i_nmi, "allwinner,sun50i-a64-r-intc",
>>>sun50i_nmi_irq_init);
>>>>
>>>
>>>Apart from the above:
>>>
>>>Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>>>
>>>Let me know how you want this to be merged.

This, and the previous dt bindings patch, can be merged through whatever
tree irqchip drivers are merged through. Is that Jason's irqchip tree?

We're mostly waiting on this before merging the remaining device tree
changes.


Thanks
ChenYu

>>
>> I think it can just go via IRQ subsystem.
>>
>>>
>>>Thanks,
>>>
>>>       M.
>>
>> --
>> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.

[toc] | [next] | [standalone]

#1657333

FromMarc Zyngier <marc.zyngier@arm.com>
Date2017-06-05 10:00 +0200
Message-ID<tOV5F-8tP-17@gated-at.bofh.it>
In reply to#1657289
On 05/06/17 06:57, Chen-Yu Tsai wrote:
> Hi Marc,
> 
> On Mon, May 22, 2017 at 10:25 PM, Chen-Yu Tsai <wens@csie.org> wrote:
>> On Mon, May 22, 2017 at 5:41 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
>>>
>>>
>>> 于 2017年5月22日 GMT+08:00 下午5:39:22, Marc Zyngier <marc.zyngier@arm.com> 写到:
>>>> On 18/05/17 08:16, Icenowy Zheng wrote:
>>>>> Add support for the newly imported compatible for the A64 R_INTC in
>>>>> irq-sunxi-nmi driver.
>>>>>
>>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>>> ---
>>>>> Changes in v5:
>>>>> - Fix A64 R_INTC compatible.
>>>>>
>>>>>  drivers/irqchip/irq-sunxi-nmi.c | 13 +++++++++++++
>>>>>  1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-sunxi-nmi.c
>>>> b/drivers/irqchip/irq-sunxi-nmi.c
>>>>> index 668730c5cb66..5559c1d593bf 100644
>>>>> --- a/drivers/irqchip/irq-sunxi-nmi.c
>>>>> +++ b/drivers/irqchip/irq-sunxi-nmi.c
>>>>> @@ -56,6 +56,12 @@ static struct sunxi_sc_nmi_reg_offs sun9i_reg_offs
>>>> = {
>>>>>      .enable = 0x04,
>>>>>  };
>>>>>
>>>>> +static struct sunxi_sc_nmi_reg_offs sun50i_reg_offs = {
>>>>> +    .ctrl   = 0x0c,
>>>>> +    .pend   = 0x10,
>>>>> +    .enable = 0x40,
>>>>> +};
>>>>> +
>>>>
>>>> Magic values? Even if no #define is provided, a pointer to the
>>>> corresponding documentation would be appreciated (assuming
>>>> documentation
>>>> exists).
>>>
>>> No documents is available for A64 R_INTC.
>>
>> No code either. In Allwinner's BSP, the interrupts for the PMICs go
>> through the (closed source) OpenRISC firmware, so there's no driver
>> for it in the kernel.
>>
>> The registers line up with the old interrupt controller from the A10,
>> but it seems only the NMI interrupt is wired up.
> 
> Is this OK? Or do you want Icenowy to respin a version with defines?

Ideally, I'd like to see some #defines, but given that the rest of the
file is already littered with hard-coded constants, you might as well do
the whole thing in a subsequent patch that I would merge with these two
patches.

>>>>
>>>>>  static inline void sunxi_sc_nmi_write(struct irq_chip_generic *gc,
>>>> u32 off,
>>>>>                                    u32 val)
>>>>>  {
>>>>> @@ -220,3 +226,10 @@ static int __init sun9i_nmi_irq_init(struct
>>>> device_node *node,
>>>>>      return sunxi_sc_nmi_irq_init(node, &sun9i_reg_offs);
>>>>>  }
>>>>>  IRQCHIP_DECLARE(sun9i_nmi, "allwinner,sun9i-a80-nmi",
>>>> sun9i_nmi_irq_init);
>>>>> +
>>>>> +static int __init sun50i_nmi_irq_init(struct device_node *node,
>>>>> +                                 struct device_node *parent)
>>>>> +{
>>>>> +    return sunxi_sc_nmi_irq_init(node, &sun50i_reg_offs);
>>>>> +}
>>>>> +IRQCHIP_DECLARE(sun50i_nmi, "allwinner,sun50i-a64-r-intc",
>>>> sun50i_nmi_irq_init);
>>>>>
>>>>
>>>> Apart from the above:
>>>>
>>>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>
>>>> Let me know how you want this to be merged.
> 
> This, and the previous dt bindings patch, can be merged through whatever
> tree irqchip drivers are merged through. Is that Jason's irqchip tree?

I'll probably start pushing a branch with all the irqchip patches I've
collected at some point this week, for Thomas to take into 4.13.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1657334 — Re: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support for the NMI in A64 R_INTC

FromIcenowy Zheng <icenowy@aosc.io>
Date2017-06-05 10:00 +0200
SubjectRe: [linux-sunxi] Re: [PATCH v6 2/9] irqchip/sunxi-nmi: add support for the NMI in A64 R_INTC
Message-ID<tOV5F-8tP-19@gated-at.bofh.it>
In reply to#1657333

于 2017年6月5日 GMT+08:00 下午3:53:50, Marc Zyngier <marc.zyngier@arm.com> 写到:
>On 05/06/17 06:57, Chen-Yu Tsai wrote:
>> Hi Marc,
>> 
>> On Mon, May 22, 2017 at 10:25 PM, Chen-Yu Tsai <wens@csie.org> wrote:
>>> On Mon, May 22, 2017 at 5:41 PM, Icenowy Zheng <icenowy@aosc.io>
>wrote:
>>>>
>>>>
>>>> 于 2017年5月22日 GMT+08:00 下午5:39:22, Marc Zyngier
><marc.zyngier@arm.com> 写到:
>>>>> On 18/05/17 08:16, Icenowy Zheng wrote:
>>>>>> Add support for the newly imported compatible for the A64 R_INTC
>in
>>>>>> irq-sunxi-nmi driver.
>>>>>>
>>>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>>>> ---
>>>>>> Changes in v5:
>>>>>> - Fix A64 R_INTC compatible.
>>>>>>
>>>>>>  drivers/irqchip/irq-sunxi-nmi.c | 13 +++++++++++++
>>>>>>  1 file changed, 13 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/irqchip/irq-sunxi-nmi.c
>>>>> b/drivers/irqchip/irq-sunxi-nmi.c
>>>>>> index 668730c5cb66..5559c1d593bf 100644
>>>>>> --- a/drivers/irqchip/irq-sunxi-nmi.c
>>>>>> +++ b/drivers/irqchip/irq-sunxi-nmi.c
>>>>>> @@ -56,6 +56,12 @@ static struct sunxi_sc_nmi_reg_offs
>sun9i_reg_offs
>>>>> = {
>>>>>>      .enable = 0x04,
>>>>>>  };
>>>>>>
>>>>>> +static struct sunxi_sc_nmi_reg_offs sun50i_reg_offs = {
>>>>>> +    .ctrl   = 0x0c,
>>>>>> +    .pend   = 0x10,
>>>>>> +    .enable = 0x40,
>>>>>> +};
>>>>>> +
>>>>>
>>>>> Magic values? Even if no #define is provided, a pointer to the
>>>>> corresponding documentation would be appreciated (assuming
>>>>> documentation
>>>>> exists).
>>>>
>>>> No documents is available for A64 R_INTC.
>>>
>>> No code either. In Allwinner's BSP, the interrupts for the PMICs go
>>> through the (closed source) OpenRISC firmware, so there's no driver
>>> for it in the kernel.
>>>
>>> The registers line up with the old interrupt controller from the
>A10,
>>> but it seems only the NMI interrupt is wired up.
>> 
>> Is this OK? Or do you want Icenowy to respin a version with defines?
>
>Ideally, I'd like to see some #defines, but given that the rest of the
>file is already littered with hard-coded constants, you might as well
>do
>the whole thing in a subsequent patch that I would merge with these two
>patches.

Personally I think the values are self-explained (the variable
name) and used only once in this structure.

Making defines doesn't make the code more clear.

>
>>>>>
>>>>>>  static inline void sunxi_sc_nmi_write(struct irq_chip_generic
>*gc,
>>>>> u32 off,
>>>>>>                                    u32 val)
>>>>>>  {
>>>>>> @@ -220,3 +226,10 @@ static int __init sun9i_nmi_irq_init(struct
>>>>> device_node *node,
>>>>>>      return sunxi_sc_nmi_irq_init(node, &sun9i_reg_offs);
>>>>>>  }
>>>>>>  IRQCHIP_DECLARE(sun9i_nmi, "allwinner,sun9i-a80-nmi",
>>>>> sun9i_nmi_irq_init);
>>>>>> +
>>>>>> +static int __init sun50i_nmi_irq_init(struct device_node *node,
>>>>>> +                                 struct device_node *parent)
>>>>>> +{
>>>>>> +    return sunxi_sc_nmi_irq_init(node, &sun50i_reg_offs);
>>>>>> +}
>>>>>> +IRQCHIP_DECLARE(sun50i_nmi, "allwinner,sun50i-a64-r-intc",
>>>>> sun50i_nmi_irq_init);
>>>>>>
>>>>>
>>>>> Apart from the above:
>>>>>
>>>>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>>
>>>>> Let me know how you want this to be merged.
>> 
>> This, and the previous dt bindings patch, can be merged through
>whatever
>> tree irqchip drivers are merged through. Is that Jason's irqchip
>tree?
>
>I'll probably start pushing a branch with all the irqchip patches I've
>collected at some point this week, for Thomas to take into 4.13.
>
>Thanks,
>
>	M.
>-- 
>Jazz is not dead. It just smells funny...
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [prev] | [next] | [standalone]

#1657336

FromMarc Zyngier <marc.zyngier@arm.com>
Date2017-06-05 10:10 +0200
Message-ID<tOVfk-kl-5@gated-at.bofh.it>
In reply to#1657334
On 05/06/17 08:56, Icenowy Zheng wrote:
> 
> 
> 于 2017年6月5日 GMT+08:00 下午3:53:50, Marc Zyngier <marc.zyngier@arm.com> 写到:
>> On 05/06/17 06:57, Chen-Yu Tsai wrote:
>>> Hi Marc,
>>>
>>> On Mon, May 22, 2017 at 10:25 PM, Chen-Yu Tsai <wens@csie.org> wrote:
>>>> On Mon, May 22, 2017 at 5:41 PM, Icenowy Zheng <icenowy@aosc.io>
>> wrote:
>>>>>
>>>>>
>>>>> 于 2017年5月22日 GMT+08:00 下午5:39:22, Marc Zyngier
>> <marc.zyngier@arm.com> 写到:
>>>>>> On 18/05/17 08:16, Icenowy Zheng wrote:
>>>>>>> Add support for the newly imported compatible for the A64 R_INTC
>> in
>>>>>>> irq-sunxi-nmi driver.
>>>>>>>
>>>>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>>>>> ---
>>>>>>> Changes in v5:
>>>>>>> - Fix A64 R_INTC compatible.
>>>>>>>
>>>>>>>  drivers/irqchip/irq-sunxi-nmi.c | 13 +++++++++++++
>>>>>>>  1 file changed, 13 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/irqchip/irq-sunxi-nmi.c
>>>>>> b/drivers/irqchip/irq-sunxi-nmi.c
>>>>>>> index 668730c5cb66..5559c1d593bf 100644
>>>>>>> --- a/drivers/irqchip/irq-sunxi-nmi.c
>>>>>>> +++ b/drivers/irqchip/irq-sunxi-nmi.c
>>>>>>> @@ -56,6 +56,12 @@ static struct sunxi_sc_nmi_reg_offs
>> sun9i_reg_offs
>>>>>> = {
>>>>>>>      .enable = 0x04,
>>>>>>>  };
>>>>>>>
>>>>>>> +static struct sunxi_sc_nmi_reg_offs sun50i_reg_offs = {
>>>>>>> +    .ctrl   = 0x0c,
>>>>>>> +    .pend   = 0x10,
>>>>>>> +    .enable = 0x40,
>>>>>>> +};
>>>>>>> +
>>>>>>
>>>>>> Magic values? Even if no #define is provided, a pointer to the
>>>>>> corresponding documentation would be appreciated (assuming
>>>>>> documentation
>>>>>> exists).
>>>>>
>>>>> No documents is available for A64 R_INTC.
>>>>
>>>> No code either. In Allwinner's BSP, the interrupts for the PMICs go
>>>> through the (closed source) OpenRISC firmware, so there's no driver
>>>> for it in the kernel.
>>>>
>>>> The registers line up with the old interrupt controller from the
>> A10,
>>>> but it seems only the NMI interrupt is wired up.
>>>
>>> Is this OK? Or do you want Icenowy to respin a version with defines?
>>
>> Ideally, I'd like to see some #defines, but given that the rest of the
>> file is already littered with hard-coded constants, you might as well
>> do
>> the whole thing in a subsequent patch that I would merge with these two
>> patches.
> 
> Personally I think the values are self-explained (the variable
> name) and used only once in this structure.
> 
> Making defines doesn't make the code more clear.

That's where you're severely misguided. There is plenty of places in the
kernel where constants are used exactly once, and yet they have a
#define. And the reason for this is consistency. We don't deal with raw
values, we deal with named constants.

For you, this code is probably "write-only". Once it works, you'll never
go back to it. On the other hand, I get to maintain it and apply tree
wide changes as necessary. So if the code looks better *to me*, then
that's the way.

Thanks,

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web