diff mbox series

[RFC,v2,25/30] include/dt-bindings: Add sh_intc IRQ - EVT conversion helper

Message ID e4dc419e3cc4f44d323aa3686333dafe83b68bce.1694596125.git.ysato@users.sourceforge.jp (mailing list archive)
State New, archived
Headers show
Series Device Tree support for SH7751 based board | expand

Commit Message

Yoshinori Sato Sept. 13, 2023, 9:23 a.m. UTC 
Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 arch/sh/boot/dts/include/dt-bindings               | 1 +
 include/dt-bindings/interrupt-controller/sh_intc.h | 7 +++++++
 2 files changed, 8 insertions(+)
 create mode 120000 arch/sh/boot/dts/include/dt-bindings
 create mode 100644 include/dt-bindings/interrupt-controller/sh_intc.h

Comments

Krzysztof Kozlowski Sept. 13, 2023, 10:47 a.m. UTC | #1 
On 13/09/2023 11:23, Yoshinori Sato wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  arch/sh/boot/dts/include/dt-bindings               | 1 +
>  include/dt-bindings/interrupt-controller/sh_intc.h | 7 +++++++
>  2 files changed, 8 insertions(+)
>  create mode 120000 arch/sh/boot/dts/include/dt-bindings
>  create mode 100644 include/dt-bindings/interrupt-controller/sh_intc.h
> 
> diff --git a/arch/sh/boot/dts/include/dt-bindings b/arch/sh/boot/dts/include/dt-bindings
> new file mode 120000
> index 000000000000..08c00e4972fa
> --- /dev/null
> +++ b/arch/sh/boot/dts/include/dt-bindings
> @@ -0,0 +1 @@
> +../../../../../include/dt-bindings
> \ No newline at end of file

Nothing improved here.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> diff --git a/include/dt-bindings/interrupt-controller/sh_intc.h b/include/dt-bindings/interrupt-controller/sh_intc.h
> new file mode 100644
> index 000000000000..cab546fba396
> --- /dev/null
> +++ b/include/dt-bindings/interrupt-controller/sh_intc.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> + *
> + * SH3/4 INTC EVT - IRQ conversion
> + */
> +
> +#define evt2irq(evt)		((evt) >> 5)
> +#define irq2evt(irq)		((irq) << 5)

No, that's not a binding. Drop entire file.

Best regards,
Krzysztof
Geert Uytterhoeven Sept. 19, 2023, 1:02 p.m. UTC | #2 
Hi Krzysztof,

On Wed, Sep 13, 2023 at 12:50 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 13/09/2023 11:23, Yoshinori Sato wrote:
> > --- /dev/null
> > +++ b/include/dt-bindings/interrupt-controller/sh_intc.h
> > @@ -0,0 +1,7 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > + *
> > + * SH3/4 INTC EVT - IRQ conversion
> > + */
> > +
> > +#define evt2irq(evt)         ((evt) >> 5)
> > +#define irq2evt(irq)         ((irq) << 5)
>
> No, that's not a binding. Drop entire file.

The issue is that the hardware documentation does not list interrupt
numbers, but event codes.  The latter is a sparse address space.
As the "interrupts" property needs interrupt numbers, we have two
options:
  1. Use hardcoded event codes and evt2irq() in DTS files.
     This is the approach Sato-san took,
  2. Use hardcoded interrupt numbers in DTS files.
     This would avoids the need for the evt2irq() macro in the DT bindings,
     but would make life slightly harder for the DTS writer and
     for the casual reader, as the conversion needs to be done in
     one's head.

Note that the documentation for later SoCs that contain both a SuperH
and an ARM CPU core, usually lists both the event code and the interrupt
number, although the latter may be offset by 32 due to the SPI
interrupt base.

I agree we do not need irq2evt() in DTS, though.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Sept. 19, 2023, 1:05 p.m. UTC | #3 
Hi Sato-san,

On Wed, Sep 13, 2023 at 11:27 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

Thanks for your patch!

>  arch/sh/boot/dts/include/dt-bindings               | 1 +
>  include/dt-bindings/interrupt-controller/sh_intc.h | 7 +++++++
>  2 files changed, 8 insertions(+)
>  create mode 120000 arch/sh/boot/dts/include/dt-bindings
>  create mode 100644 include/dt-bindings/interrupt-controller/sh_intc.h
>
> diff --git a/arch/sh/boot/dts/include/dt-bindings b/arch/sh/boot/dts/include/dt-bindings
> new file mode 120000
> index 000000000000..08c00e4972fa
> --- /dev/null
> +++ b/arch/sh/boot/dts/include/dt-bindings
> @@ -0,0 +1 @@
> +../../../../../include/dt-bindings

Do you need this symlink? All other architectures do without.

> --- /dev/null
> +++ b/include/dt-bindings/interrupt-controller/sh_intc.h

This is used by the DTS files, so this patch should be moved up in
this series.

Gr{oetje,eeting}s,

                        Geert
Krzysztof Kozlowski Sept. 20, 2023, 11:51 a.m. UTC | #4 
On 19/09/2023 15:02, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Wed, Sep 13, 2023 at 12:50 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 13/09/2023 11:23, Yoshinori Sato wrote:
>>> --- /dev/null
>>> +++ b/include/dt-bindings/interrupt-controller/sh_intc.h
>>> @@ -0,0 +1,7 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> + *
>>> + * SH3/4 INTC EVT - IRQ conversion
>>> + */
>>> +
>>> +#define evt2irq(evt)         ((evt) >> 5)
>>> +#define irq2evt(irq)         ((irq) << 5)
>>
>> No, that's not a binding. Drop entire file.
> 
> The issue is that the hardware documentation does not list interrupt
> numbers, but event codes.  The latter is a sparse address space.
> As the "interrupts" property needs interrupt numbers, we have two
> options:
>   1. Use hardcoded event codes and evt2irq() in DTS files.
>      This is the approach Sato-san took,
>   2. Use hardcoded interrupt numbers in DTS files.
>      This would avoids the need for the evt2irq() macro in the DT bindings,
>      but would make life slightly harder for the DTS writer and
>      for the casual reader, as the conversion needs to be done in
>      one's head.
> 
> Note that the documentation for later SoCs that contain both a SuperH
> and an ARM CPU core, usually lists both the event code and the interrupt
> number, although the latter may be offset by 32 due to the SPI
> interrupt base.
> 
> I agree we do not need irq2evt() in DTS, though.

Is the macro used by the drivers? I have a feeling that not, so it would
not be suitable for the bindings, but rather as a header included in the
DTS.

Best regards,
Krzysztof
Geert Uytterhoeven Sept. 20, 2023, 12:30 p.m. UTC | #5 
Hi Krzysztof,

On Wed, Sep 20, 2023 at 1:51 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 19/09/2023 15:02, Geert Uytterhoeven wrote:
> > On Wed, Sep 13, 2023 at 12:50 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On 13/09/2023 11:23, Yoshinori Sato wrote:
> >>> --- /dev/null
> >>> +++ b/include/dt-bindings/interrupt-controller/sh_intc.h
> >>> @@ -0,0 +1,7 @@
> >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> + *
> >>> + * SH3/4 INTC EVT - IRQ conversion
> >>> + */
> >>> +
> >>> +#define evt2irq(evt)         ((evt) >> 5)
> >>> +#define irq2evt(irq)         ((irq) << 5)
> >>
> >> No, that's not a binding. Drop entire file.
> >
> > The issue is that the hardware documentation does not list interrupt
> > numbers, but event codes.  The latter is a sparse address space.
> > As the "interrupts" property needs interrupt numbers, we have two
> > options:
> >   1. Use hardcoded event codes and evt2irq() in DTS files.
> >      This is the approach Sato-san took,
> >   2. Use hardcoded interrupt numbers in DTS files.
> >      This would avoids the need for the evt2irq() macro in the DT bindings,
> >      but would make life slightly harder for the DTS writer and
> >      for the casual reader, as the conversion needs to be done in
> >      one's head.
> >
> > Note that the documentation for later SoCs that contain both a SuperH
> > and an ARM CPU core, usually lists both the event code and the interrupt
> > number, although the latter may be offset by 32 due to the SPI
> > interrupt base.
> >
> > I agree we do not need irq2evt() in DTS, though.
>
> Is the macro used by the drivers? I have a feeling that not, so it would
> not be suitable for the bindings, but rather as a header included in the
> DTS.

You mean irq2evt()? No, the new DT-aware drivers avoid the conversion
from IRQ numbers to events codes by storing evt2irq() values in the
interrupt table (see drivers/irqchip/irq-renesas-sh7751.c:iprmaps[]).

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/arch/sh/boot/dts/include/dt-bindings b/arch/sh/boot/dts/include/dt-bindings
new file mode 120000
index 000000000000..08c00e4972fa
--- /dev/null
+++ b/arch/sh/boot/dts/include/dt-bindings
@@ -0,0 +1 @@ 
+../../../../../include/dt-bindings
\ No newline at end of file
diff --git a/include/dt-bindings/interrupt-controller/sh_intc.h b/include/dt-bindings/interrupt-controller/sh_intc.h
new file mode 100644
index 000000000000..cab546fba396
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/sh_intc.h
@@ -0,0 +1,7 @@ 
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+ *
+ * SH3/4 INTC EVT - IRQ conversion
+ */
+
+#define evt2irq(evt)		((evt) >> 5)
+#define irq2evt(irq)		((irq) << 5)