| Message ID | cover.1704788539.git.ysato@users.sourceforge.jp |
|---|---|
| Headers | show |
| Series | Device Tree support for SH7751 based board | expand |
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
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.
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
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 >
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 >
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 > >
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 :)
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
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 >
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 >
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
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