mbox series

[DO,NOT,MERGE,v6,00/37] Device Tree support for SH7751 based board

Message ID cover.1704788539.git.ysato@users.sourceforge.jp
Headers show
Series Device Tree support for SH7751 based board | expand

Message

Yoshinori Sato Jan. 9, 2024, 8:22 a.m. UTC
This is an updated version of something I wrote about 7 years ago.
Minimum support for R2D-plus and LANDISK.
I think R2D-1 will work if you add AX88796 to dts.
And board-specific functions and SCI's SPI functions are not supported.

You can get it working with qemu found here.
https://gitlab.com/yoshinori.sato/qemu/-/tree/landisk

v6 changes.
- pci-sh7751: merge register define.
- pci-sh7751: use 'dma-ranges' property.
- pci-sh7751: rename general PCI properties.
- sm501 and sm501fb: Re-design Device Tree properties.
- sh/kernel/setup: cleanup command line setup.
- irq-sh7751.c: some cleanup.

v5 changes.
- pci-sh7751: revert header changes. and some fix in previuous driver.
- sh/kernel/iomap.c: Use SH io functions.
- sm501 and sm501fb: re-write DT support.

v4 changes.
- cpg-sh7750: use clk-divider and clk-gate.
- pci-sh7751: unified header files to old PCI driver.
- irq-renesas-sh7751: IPR registers direct mapping.
- irq-renesas-sh7751irl: useful register bit mapping.
- sm501 and sm501fb: re-write dt parser.
- j2_minus: fix build error.
- dt-binding schema: fix some errors.
- *.dts: cleanup.

v3 changes.
- Rewrite clk drivers.
- Added sh_tmu to OF support.
- Cleanup PCI stuff.
- Update sm501 and sm501fb OF support.
- Update devicetree and documents.

v2 changes.
- Rebasing v6,6-rc1
- re-write irqchip driver.
- Add binding documents.
- Cleanup review comment.

Yoshinori Sato (37):
  sh: passing FDT address to kernel startup.
  sh: Kconfig unified OF supported targets.
  sh: Enable OF support for build and configuration.
  dt-bindings: interrupt-controller: Add header for Renesas SH3/4 INTC.
  sh: GENERIC_IRQ_CHIP support for CONFIG_OF=y
  sh: kernel/setup Update DT support.
  sh: Fix COMMON_CLK support in CONFIG_OF=y.
  clocksource: sh_tmu: CLOCKSOURCE support.
  dt-bindings: timer: renesas,tmu: add renesas,tmu-sh7750
  sh: Common PCI Framework driver support.
  pci: pci-sh7751: Add SH7751 PCI driver
  dt-bindings: pci: pci-sh7751: Add SH7751 PCI
  dt-bindings: clock: sh7750-cpg: Add renesas,sh7750-cpg header.
  clk: Compatible with narrow registers
  clk: renesas: Add SH7750/7751 CPG Driver
  irqchip: Add SH7751 INTC driver
  dt-bindings: interrupt-controller: renesas,sh7751-intc: Add
    json-schema
  irqchip: SH7751 external interrupt encoder with enable gate.
  dt-bindings: interrupt-controller: renesas,sh7751-irl-ext: Add
    json-schema
  serial: sh-sci: fix SH4 OF support.
  dt-bindings: serial: renesas,scif: Add scif-sh7751.
  dt-bindings: display: smi,sm501: SMI SM501 binding json-schema
  mfd: sm501: Convert platform_data to OF property
  dt-binding: sh: cpus: Add SH CPUs json-schema
  dt-bindings: vendor-prefixes: Add iodata
  dt-bindings: vendor-prefixes:  Add smi
  dt-bindings: ata: ata-generic: Add new targets
  dt-bindings: soc: renesas: sh: Add SH7751 based target
  sh: SH7751R SoC Internal peripheral definition dtsi.
  sh: add RTS7751R2D Plus DTS
  sh: Add IO DATA LANDISK dts
  sh: Add IO DATA USL-5P dts
  sh: j2_mimas_v2.dts update
  sh: Add dtbs target support.
  sh: RTS7751R2D Plus OF defconfig
  sh: LANDISK OF defconfig
  sh: j2_defconfig: update

 .../devicetree/bindings/ata/ata-generic.yaml  |   2 +
 .../bindings/clock/renesas,sh7750-cpg.yaml    | 103 ++++
 .../bindings/display/smi,sm501.yaml           | 417 +++++++++++++++
 .../renesas,sh7751-intc.yaml                  | 105 ++++
 .../renesas,sh7751-irl-ext.yaml               |  72 +++
 .../bindings/pci/renesas,sh7751-pci.yaml      | 150 ++++++
 .../bindings/serial/renesas,scif.yaml         |   1 +
 .../devicetree/bindings/sh/cpus.yaml          |  74 +++
 .../devicetree/bindings/soc/renesas/sh.yaml   |  32 ++
 .../bindings/timer/renesas,tmu.yaml           |  67 ++-
 .../devicetree/bindings/vendor-prefixes.yaml  |   4 +
 arch/sh/Kconfig                               |  12 +-
 arch/sh/boards/Kconfig                        |  24 +-
 arch/sh/boards/of-generic.c                   |  28 +-
 arch/sh/boot/compressed/head_32.S             |   5 +-
 arch/sh/boot/dts/Makefile                     |   5 +
 arch/sh/boot/dts/j2_mimas_v2.dts              |   2 +-
 arch/sh/boot/dts/landisk.dts                  |  75 +++
 arch/sh/boot/dts/rts7751r2dplus.dts           | 180 +++++++
 arch/sh/boot/dts/sh7751r.dtsi                 | 152 ++++++
 arch/sh/boot/dts/usl-5p.dts                   |  83 +++
 arch/sh/configs/j2_defconfig                  |  11 +-
 arch/sh/configs/landisk-of_defconfig          | 104 ++++
 arch/sh/configs/rts7751r2dplus-of_defconfig   |  80 +++
 arch/sh/drivers/Makefile                      |   2 +
 arch/sh/include/asm/io.h                      |   8 +
 arch/sh/include/asm/irq.h                     |  10 +-
 arch/sh/include/asm/pci.h                     |   4 +
 arch/sh/kernel/cpu/Makefile                   |   6 +-
 arch/sh/kernel/cpu/irq/imask.c                |  17 +
 arch/sh/kernel/cpu/sh4/Makefile               |   3 +
 arch/sh/kernel/iomap.c                        |  18 +
 arch/sh/kernel/setup.c                        |  33 +-
 arch/sh/kernel/time.c                         |  12 +
 drivers/clk/clk-divider.c                     |  56 +-
 drivers/clk/clk-gate.c                        |  56 +-
 drivers/clk/renesas/Kconfig                   |  16 +-
 drivers/clk/renesas/Makefile                  |   1 +
 drivers/clk/renesas/clk-sh7750.c              | 498 ++++++++++++++++++
 drivers/clocksource/sh_tmu.c                  | 161 ++++--
 drivers/irqchip/Kconfig                       |  15 +
 drivers/irqchip/Makefile                      |   3 +
 drivers/irqchip/irq-renesas-sh7751.c          | 313 +++++++++++
 drivers/irqchip/irq-renesas-sh7751irl.c       | 228 ++++++++
 drivers/mfd/sm501.c                           | 436 +++++++++++++++
 drivers/pci/controller/Kconfig                |   9 +
 drivers/pci/controller/Makefile               |   1 +
 drivers/pci/controller/pci-sh7751.c           | 392 ++++++++++++++
 drivers/tty/serial/Kconfig                    |   2 +-
 drivers/tty/serial/sh-sci.c                   |   6 +-
 drivers/video/fbdev/sm501fb.c                 | 106 ++++
 include/dt-bindings/clock/sh7750-cpg.h        |  26 +
 .../renesas,sh7751-intc.h                     |  19 +
 include/linux/clk-provider.h                  |  22 +-
 include/linux/sh_intc.h                       |   7 +-
 55 files changed, 4096 insertions(+), 178 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml
 create mode 100644 Documentation/devicetree/bindings/display/smi,sm501.yaml
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-intc.yaml
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-irl-ext.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/renesas,sh7751-pci.yaml
 create mode 100644 Documentation/devicetree/bindings/sh/cpus.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/renesas/sh.yaml
 create mode 100644 arch/sh/boot/dts/landisk.dts
 create mode 100644 arch/sh/boot/dts/rts7751r2dplus.dts
 create mode 100644 arch/sh/boot/dts/sh7751r.dtsi
 create mode 100644 arch/sh/boot/dts/usl-5p.dts
 create mode 100644 arch/sh/configs/landisk-of_defconfig
 create mode 100644 arch/sh/configs/rts7751r2dplus-of_defconfig
 create mode 100644 drivers/clk/renesas/clk-sh7750.c
 create mode 100644 drivers/irqchip/irq-renesas-sh7751.c
 create mode 100644 drivers/irqchip/irq-renesas-sh7751irl.c
 create mode 100644 drivers/pci/controller/pci-sh7751.c
 create mode 100644 include/dt-bindings/clock/sh7750-cpg.h
 create mode 100644 include/dt-bindings/interrupt-controller/renesas,sh7751-intc.h

Comments

Linus Walleij Jan. 9, 2024, 12:30 p.m. UTC | #1
Hi Yoshinori,

thanks for your patch!

On Tue, Jan 9, 2024 at 9:24 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:

> +  renesas,icr-irlm:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: If true four independent interrupt requests mode (ICR.IRLM is 1).
> +
> +  renesas,ipr-map:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: |
> +      IRQ to IPR mapping definition.
> +      1st - INTEVT code
> +      2nd - Register
> +      3rd - bit index

(...)

> +            renesas,ipr-map = <0x240 IPRD IPR_B12>, /* IRL0 */
> +                              <0x2a0 IPRD IPR_B8>,  /* IRL1 */
> +                              <0x300 IPRD IPR_B4>,  /* IRL2 */
> +                              <0x360 IPRD IPR_B0>,  /* IRL3 */
(...)

Is it really necessary to have all this in the device tree?

You know from the compatible that this is "renesas,sh7751-intc"
and I bet this table will be the same for any sh7751 right?

Then just put it in a table in the driver instead and skip this from
the device tree and bindings. If more interrupt controllers need
to be supported by the driver, you can simply look up the table from
the compatible string.

Yours,
Linus Walleij
Rob Herring Jan. 9, 2024, 4:29 p.m. UTC | #2
On Tue, 09 Jan 2024 17:23:16 +0900, Yoshinori Sato wrote:
> Renesas SH7751 external interrupt encoder json-schema.
> 
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  .../renesas,sh7751-irl-ext.yaml               | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-irl-ext.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-irl-ext.example.dtb: sh7751irl_encoder@a4000000: '#size-cells' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/interrupt-controller/renesas,sh7751-irl-ext.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/a801115c277e65341da079c318a1b970f8d9e671.1704788539.git.ysato@users.sourceforge.jp

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Rob Herring Jan. 9, 2024, 5:18 p.m. UTC | #3
On Tue, Jan 09, 2024 at 05:23:16PM +0900, Yoshinori Sato wrote:
> Renesas SH7751 external interrupt encoder json-schema.
> 
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  .../renesas,sh7751-irl-ext.yaml               | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-irl-ext.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-irl-ext.yaml b/Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-irl-ext.yaml
> new file mode 100644
> index 000000000000..541b582b94ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-irl-ext.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/renesas,sh7751-irl-ext.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas SH7751 external interrupt encoder with enable regs.
> +
> +maintainers:
> +  - Yoshinori Sato <ysato@users.sourceforge.jp>
> +
> +description:
> +  This is the generally used external interrupt encoder on SH7751 based boards.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: renesas,sh7751-irl-ext
> +
> +  reg: true

Must define how many entries and what they are if more than one.

> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 1
> +
> +  '#address-cells':
> +    const: 0
> +
> +  renesas,set-to-disable:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: Invert enable registers. Setting the bit to 0 enables interrupts.

Why is this a property? Does it change per board or instance? If not, it 
should be implied by compatible.

> +
> +  renesas,enable-bit:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    description: |
> +      IRQ enable register bit mapping
> +      1st word interrupt level
> +      2nd word bit index of enable register

Same question here.

If it remains, then you need:

items:
  items:
    - description: interrupt level (does that mean high/low?)
    - description: bit index of enable register

Plus any constraints on the values if possible.


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - renesas,enable-bit
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    r2dintc: sh7751irl_encoder@a4000000 {

interrupt-controller@a4000000 {

> +        compatible = "renesas,sh7751-irl-ext";
> +        reg = <0xa4000000 0x02>;
> +        interrupt-controller;
> +        #address-cells = <0>;
> +        #size-cells = <0>;
> +        #interrupt-cells = <1>;
> +        renesas,enable-bit = <0 11>,            /* PCI INTD */
> +                             <1 9>,             /* CF IDE */
> +                             <2 8>,             /* CF CD */
> +                             <3 12>,            /* PCI INTC */
> +                             <4 10>,            /* SM501 */
> +                             <5 6>,             /* KEY */
> +                             <6 5>,             /* RTC ALARM */
> +                             <7 4>,             /* RTC T */
> +                             <8 7>,             /* SDCARD */
> +                             <9 14>,            /* PCI INTA */
> +                             <10 13>,           /* PCI INTB */
> +                             <11 0>,            /* EXT */
> +                             <12 15>;           /* TP */

Looks like 'interrupt level' is just the index of the values? Why not 
make this an array?

Rob
Conor Dooley Jan. 9, 2024, 6:03 p.m. UTC | #4
On Tue, Jan 09, 2024 at 05:23:22PM +0900, Yoshinori Sato wrote:
> Add IO DATA DEVICE INC.
> https://www.iodata.com/
> 
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

I think you are missing an r-b tag here from Geert:
https://lore.kernel.org/all/CAMuHMdUvNT1tDTOq4ppqn69cocAeveaXrsoL2VQ2efBQ+hv2aA@mail.gmail.com/

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 309b94c328c8..94ed63d9f7de 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -671,6 +671,8 @@ patternProperties:
>      description: Inventec
>    "^inversepath,.*":
>      description: Inverse Path
> +  "^iodata,.*":
> +    description: IO DATA DEVICE Inc.
>    "^iom,.*":
>      description: Iomega Corporation
>    "^irondevice,.*":
> -- 
> 2.39.2
>
Conor Dooley Jan. 9, 2024, 6:07 p.m. UTC | #5
On Tue, Jan 09, 2024 at 05:23:24PM +0900, Yoshinori Sato wrote:
> Added new ata-generic target.
> - iodata,usl-5p-ata
> - renesas,rts7751r2d-ata
> 
> Each boards have simple IDE Interface. Use ATA generic driver.
> 
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/ata/ata-generic.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/ata/ata-generic.yaml b/Documentation/devicetree/bindings/ata/ata-generic.yaml
> index 0697927f3d7e..1025b3b351d0 100644
> --- a/Documentation/devicetree/bindings/ata/ata-generic.yaml
> +++ b/Documentation/devicetree/bindings/ata/ata-generic.yaml
> @@ -18,6 +18,8 @@ properties:
>        - enum:
>            - arm,vexpress-cf
>            - fsl,mpc8349emitx-pata
> +          - iodata,usl-5p-ata
> +          - renesas,rts7751r2d-ata
>        - const: ata-generic
>  
>    reg:
> -- 
> 2.39.2
>
Conor Dooley Jan. 9, 2024, 6:09 p.m. UTC | #6
On Tue, Jan 09, 2024 at 06:07:19PM +0000, Conor Dooley wrote:
> On Tue, Jan 09, 2024 at 05:23:24PM +0900, Yoshinori Sato wrote:
> > Added new ata-generic target.
> > - iodata,usl-5p-ata
> > - renesas,rts7751r2d-ata
> > 
> > Each boards have simple IDE Interface. Use ATA generic driver.
> > 
> > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

That said, a bullet point list in the commit message of what
compatibles you added isn't really achieving anything, you can drop that
from the commit message if/when you resend the series.


> 
> Cheers,
> Conor.
> 
> > ---
> >  Documentation/devicetree/bindings/ata/ata-generic.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/ata/ata-generic.yaml b/Documentation/devicetree/bindings/ata/ata-generic.yaml
> > index 0697927f3d7e..1025b3b351d0 100644
> > --- a/Documentation/devicetree/bindings/ata/ata-generic.yaml
> > +++ b/Documentation/devicetree/bindings/ata/ata-generic.yaml
> > @@ -18,6 +18,8 @@ properties:
> >        - enum:
> >            - arm,vexpress-cf
> >            - fsl,mpc8349emitx-pata
> > +          - iodata,usl-5p-ata
> > +          - renesas,rts7751r2d-ata
> >        - const: ata-generic
> >  
> >    reg:
> > -- 
> > 2.39.2
> >
Damien Le Moal Jan. 10, 2024, 2:06 a.m. UTC | #7
On 1/9/24 17:23, Yoshinori Sato wrote:
> Added new ata-generic target.
> - iodata,usl-5p-ata
> - renesas,rts7751r2d-ata
> 
> Each boards have simple IDE Interface. Use ATA generic driver.

This looks OK to me, so feel free to add:

Acked-by: Damien Le Moal <dlemoal@kernel.org>

Note: The "DO NOT MERGE" patch prefix almost got me to immediately delete this
37 patches in my inbox... If you wish to get this work merged after review,
please use the regular "PATCH" prefix. No worries, the series will not be merged
until is is reviewed :)
Krzysztof Kozlowski Jan. 10, 2024, 7:19 a.m. UTC | #8
On 10/01/2024 03:06, Damien Le Moal wrote:
> On 1/9/24 17:23, Yoshinori Sato wrote:
>> Added new ata-generic target.
>> - iodata,usl-5p-ata
>> - renesas,rts7751r2d-ata
>>
>> Each boards have simple IDE Interface. Use ATA generic driver.
> 
> This looks OK to me, so feel free to add:
> 
> Acked-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Note: The "DO NOT MERGE" patch prefix almost got me to immediately delete this
> 37 patches in my inbox... If you wish to get this work merged after review,
> please use the regular "PATCH" prefix. No worries, the series will not be merged
> until is is reviewed :)

The point of DO NOT MERGE was that feedback was not being implemented
and same set of patches with same issues were kept sending. :/

Best regards,
Krzysztof
Damien Le Moal Jan. 10, 2024, 7:25 a.m. UTC | #9
On 1/10/24 16:19, Krzysztof Kozlowski wrote:
> On 10/01/2024 03:06, Damien Le Moal wrote:
>> On 1/9/24 17:23, Yoshinori Sato wrote:
>>> Added new ata-generic target.
>>> - iodata,usl-5p-ata
>>> - renesas,rts7751r2d-ata
>>>
>>> Each boards have simple IDE Interface. Use ATA generic driver.
>>
>> This looks OK to me, so feel free to add:
>>
>> Acked-by: Damien Le Moal <dlemoal@kernel.org>
>>
>> Note: The "DO NOT MERGE" patch prefix almost got me to immediately delete this
>> 37 patches in my inbox... If you wish to get this work merged after review,
>> please use the regular "PATCH" prefix. No worries, the series will not be merged
>> until is is reviewed :)
> 
> The point of DO NOT MERGE was that feedback was not being implemented
> and same set of patches with same issues were kept sending. :/

OK. I found that prefix unusual, but not a big deal.
Thanks.

> 
> Best regards,
> Krzysztof
>
Yoshinori Sato Jan. 17, 2024, 9:46 a.m. UTC | #10
On Tue, 09 Jan 2024 21:30:34 +0900,
Linus Walleij wrote:
> 
> Hi Yoshinori,
> 
> thanks for your patch!
> 
> On Tue, Jan 9, 2024 at 9:24 AM Yoshinori Sato
> <ysato@users.sourceforge.jp> wrote:
> 
> > +  renesas,icr-irlm:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: If true four independent interrupt requests mode (ICR.IRLM is 1).
> > +
> > +  renesas,ipr-map:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description: |
> > +      IRQ to IPR mapping definition.
> > +      1st - INTEVT code
> > +      2nd - Register
> > +      3rd - bit index
> 
> (...)
> 
> > +            renesas,ipr-map = <0x240 IPRD IPR_B12>, /* IRL0 */
> > +                              <0x2a0 IPRD IPR_B8>,  /* IRL1 */
> > +                              <0x300 IPRD IPR_B4>,  /* IRL2 */
> > +                              <0x360 IPRD IPR_B0>,  /* IRL3 */
> (...)
> 
> Is it really necessary to have all this in the device tree?
> 
> You know from the compatible that this is "renesas,sh7751-intc"
> and I bet this table will be the same for any sh7751 right?
> 
> Then just put it in a table in the driver instead and skip this from
> the device tree and bindings. If more interrupt controllers need
> to be supported by the driver, you can simply look up the table from
> the compatible string.

The SH interrupt controller has the same structure, only this part is different for each SoC.
Currently, we are targeting only the 7751, but in the future we plan to handle all SoCs.
Is it better to differentiate SoC only by compatible?

> Yours,
> Linus Walleij
>
Geert Uytterhoeven Jan. 17, 2024, 10:06 a.m. UTC | #11
Hi Sato-san,

On Wed, Jan 17, 2024 at 10:46 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> On Tue, 09 Jan 2024 21:30:34 +0900,
> Linus Walleij wrote:
> > On Tue, Jan 9, 2024 at 9:24 AM Yoshinori Sato
> > <ysato@users.sourceforge.jp> wrote:
> >
> > > +  renesas,icr-irlm:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description: If true four independent interrupt requests mode (ICR.IRLM is 1).
> > > +
> > > +  renesas,ipr-map:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +    description: |
> > > +      IRQ to IPR mapping definition.
> > > +      1st - INTEVT code
> > > +      2nd - Register
> > > +      3rd - bit index
> >
> > (...)
> >
> > > +            renesas,ipr-map = <0x240 IPRD IPR_B12>, /* IRL0 */
> > > +                              <0x2a0 IPRD IPR_B8>,  /* IRL1 */
> > > +                              <0x300 IPRD IPR_B4>,  /* IRL2 */
> > > +                              <0x360 IPRD IPR_B0>,  /* IRL3 */
> > (...)
> >
> > Is it really necessary to have all this in the device tree?
> >
> > You know from the compatible that this is "renesas,sh7751-intc"
> > and I bet this table will be the same for any sh7751 right?
> >
> > Then just put it in a table in the driver instead and skip this from
> > the device tree and bindings. If more interrupt controllers need
> > to be supported by the driver, you can simply look up the table from
> > the compatible string.
>
> The SH interrupt controller has the same structure, only this part is different for each SoC.
> Currently, we are targeting only the 7751, but in the future we plan to handle all SoCs.
> Is it better to differentiate SoC only by compatible?

Yes, it is better to differentiate SoC only by compatible value.

When you describe all differences explicitly using properties, you
might discover later that you missed something important, causing
backwards compatibility issues with old DTBs.
DT is a stable ABI, while you can always update a driver when needed.

Gr{oetje,eeting}s,

                        Geert