| Message ID | 20230424123522.18302-1-nikita.shubin@maquefel.me |
|---|---|
| Headers | show |
| Series | ep93xx device tree conversion | expand |
On Mon, Apr 24, 2023, at 14:34, Nikita Shubin wrote: > This series aims to convert ep93xx from platform to full device tree support. > > Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302. > > Thank you Linus and Arnd for your support, review and comments, sorry > if i missed something - > these series are quite big for me. > > Big thanks to Alexander Sverdlin for his testing, support, review, > fixes and patches. Thanks a lot for your continued work. I can't merge any of this at the moment since the upstream merge window just opened, but I'm happy to take this all through the soc tree for 6.5, provided we get the sufficient Acks from the subsystem maintainers. Merging it through each individual tree would take a lot longer, so I hope we can avoid that. Arnd
On Mon, 24 Apr 2023 15:34:27 +0300, Nikita Shubin wrote: > This adds device tree bindings for the Cirrus Logic EP93xx > clock block used in these SoCs. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > .../devicetree/bindings/arm/ep93xx.yaml | 102 ++++++++++++++++++ > .../dt-bindings/clock/cirrus,ep93xx-clock.h | 53 +++++++++ > 2 files changed, 155 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/ep93xx.yaml > create mode 100644 include/dt-bindings/clock/cirrus,ep93xx-clock.h > 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/arm/ep93xx.example.dtb: /: compatible: 'oneOf' conditional failed, one must be fixed: ['technologic,ts7250', 'cirrus,ep9301'] is too short 'liebherr,bk3' was expected From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/ep93xx.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230424123522.18302-12-nikita.shubin@maquefel.me 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 Mon, Apr 24, 2023 at 03:34:32PM +0300, Nikita Shubin wrote: > +maintainers: > + - Mark Brown <broonie@kernel.org> This needs to be someone who actually knows about and works on the device. > + use_dma: > + type: boolean > + items: > + - description: Flag indicating that the SPI should use dma There don't seem to be any DMA properties here, and why would this not just be done by making them optional rather than having a separate specific property?
On Mon, Apr 24, 2023 at 03:34:38PM +0300, Nikita Shubin wrote: > Add YAML bindings for ts7250 NAND. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > .../bindings/mtd/technologic,nand.yaml | 56 +++++++++++++++++++ > 1 file changed, 56 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml > > diff --git a/Documentation/devicetree/bindings/mtd/technologic,nand.yaml b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > new file mode 100644 > index 000000000000..3234d93a1c21 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > @@ -0,0 +1,56 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mtd/technologic,nand.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Technologic Systems NAND controller > + > +maintainers: > + - Lukasz Majewski <lukma@denx.de> > + > +properties: > + compatible: > + items: > + - const: technologic,ts7200-nand > + - const: gen_nand Not a useful compatible. > + > + reg: > + maxItems: 1 > + > + '#address-cells': true > + '#size-cells': true > + > +required: > + - compatible > + - reg > + > +unevaluatedProperties: true No, 'true' is not allowed here. > + > +examples: > + - | > + nand-parts@0 { > + compatible = "technologic,ts7200-nand", "gen_nand"; > + reg = <0x60000000 0x8000000>; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "TS-BOOTROM"; > + reg = <0x00000000 0x00020000>; > + read-only; > + }; > + > + partition@20000 { > + label = "Linux"; > + reg = <0x00020000 0x07d00000>; > + }; > + > + partition@7d20000 { > + label = "RedBoot"; > + reg = <0x07d20000 0x002e0000>; > + read-only; > + }; > + }; > + > +... > -- > 2.39.2 >
On 25/04/2023 00:29, Jakub Kicinski wrote: > On Mon, 24 Apr 2023 13:31:25 +0200 Arnd Bergmann wrote: >> Thanks a lot for your continued work. I can't merge any of this at >> the moment since the upstream merge window just opened, but I'm >> happy to take this all through the soc tree for 6.5, provided we >> get the sufficient Acks from the subsystem maintainers. Merging >> it through each individual tree would take a lot longer, so I >> hope we can avoid that. > > Is there a dependency between the patches? I didn't get entire patchset and cover letter does not mention dependencies, but usually there shouldn't be such. Maybe for the next versions this should be split per subsystem? Best regards, Krzysztof
On 24/04/2023 14:34, Nikita Shubin wrote: > This adds device tree bindings for the Cirrus Logic EP93xx > timer block used in these SoCs. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> Subject: drop second/last, redundant "DT bindings for". The "dt-bindings" prefix is already stating that these are bindings. In all patches. > --- > > Notes: > Arnd Bergmann: > - replaced ep93xx wildcard with ep9301 > > .../bindings/timer/cirrus,ep93xx-timer.yaml | 41 +++++++++++++++++++ > 1 file changed, 41 insertions(+) > create mode 100644 Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml > > diff --git a/Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml b/Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml > new file mode 100644 > index 000000000000..ce8b8a5cb90a > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml > @@ -0,0 +1,41 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/timer/cirrus,ep93xx-timer.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus Logic EP93xx timers bindings Drop "bindings". In all patches. > + > +maintainers: > + - Hartley Sweeten <hsweeten@visionengravers.com> > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + > +properties: > + compatible: > + const: cirrus,ep9301-timer > + With two fixes above: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- This is an automated instruction, just in case, because many review tags are being ignored. If you do not know the process, here is a short explanation: Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions, under or above your Signed-off-by tag. Tools like b4 can help here. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply. https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submitting-patches.rst#L540 Best regards, Krzysztof
On 24/04/2023 14:34, Nikita Shubin wrote: > This adds device tree bindings for the Cirrus Logic EP93xx > timer block used in these SoCs. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > Arnd Bergmann: > - replaced ep93xx wildcard with ep9301 > > .../bindings/timer/cirrus,ep93xx-timer.yaml | 41 +++++++++++++++++++ > 1 file changed, 41 insertions(+) > create mode 100644 Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml > > diff --git a/Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml b/Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml > new file mode 100644 > index 000000000000..ce8b8a5cb90a > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml > @@ -0,0 +1,41 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/timer/cirrus,ep93xx-timer.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus Logic EP93xx timers bindings > + > +maintainers: > + - Hartley Sweeten <hsweeten@visionengravers.com> > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + > +properties: > + compatible: > + const: cirrus,ep9301-timer 1. Why only one compatible? 2. If this is kept, then filename matching compatible. Best regards, Krzysztof
On Mon, Apr 24, 2023 at 6:32 PM Rob Herring <robh@kernel.org> wrote: > On Mon, Apr 24, 2023 at 03:34:48PM +0300, Nikita Shubin wrote: > > Add YAML bindings for ep93xx SoC. > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> (...) > > + chip-label: > > + maxItems: 1 > > + description: human readable name. > > Why do you need this? It's not standard and I don't see other GPIO > controllers needing it. Caught my eye too, Nikita can you live without this and just use dev_name() or something to name the chip in Linux? If it is to conform to EP93xx documentation naming I guess it should be cirrus,ep93xx-gpio-chip-name = "..."; ? Yours, Linus Walleij
On Wed, Apr 26, 2023 at 11:02 PM Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 26, 2023 at 10:56:53PM +0200, Linus Walleij wrote: > > > This is a big patch set and the improvement to the ARM kernel it > > brings is great, so I am a bit worried about over-review stalling the > > merged. If there start to be nitpicky comments I would prefer that > > we merge it and let minor comments and "nice-to-haves" be > > addressed in-tree during the development cycle. > > I'm really not enthusiastic about the SPI bindings being merged as-is. Agree, the bindings are more important than the code IMO, they tend to get written in stone. Yours, Linus Walleij
On 28/04/2023 16:34, Nikita Shubin wrote: > On Tue, 2023-04-25 at 11:29 +0200, Krzysztof Kozlowski wrote: >> On 24/04/2023 14:34, Nikita Shubin wrote: >>> This adds device tree bindings for the Cirrus Logic EP93xx >>> timer block used in these SoCs. >>> >>> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> >>> --- >>> >>> Notes: >>> Arnd Bergmann: >>> - replaced ep93xx wildcard with ep9301 >>> >>> .../bindings/timer/cirrus,ep93xx-timer.yaml | 41 >>> +++++++++++++++++++ >>> 1 file changed, 41 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/timer/cirrus,ep93xx- >>> timer.yaml b/Documentation/devicetree/bindings/timer/cirrus,ep93xx- >>> timer.yaml >>> new file mode 100644 >>> index 000000000000..ce8b8a5cb90a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/timer/cirrus,ep93xx- >>> timer.yaml >>> @@ -0,0 +1,41 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/timer/cirrus,ep93xx-timer.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Cirrus Logic EP93xx timers bindings >>> + >>> +maintainers: >>> + - Hartley Sweeten <hsweeten@visionengravers.com> >>> + - Alexander Sverdlin <alexander.sverdlin@gmail.com> >>> + >>> +properties: >>> + compatible: >>> + const: cirrus,ep9301-timer >> >> 1. Why only one compatible? >> 2. If this is kept, then filename matching compatible. > > I should rename the file to cirrus,ep9301-timer.yaml No, at least no yet. See point 1. Best regards, Krzysztof
On Tue, 2023-04-25 at 11:29 +0200, Krzysztof Kozlowski wrote: > On 24/04/2023 14:34, Nikita Shubin wrote: > > This adds device tree bindings for the Cirrus Logic EP93xx > > timer block used in these SoCs. > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > > --- > > > > Notes: > > Arnd Bergmann: > > - replaced ep93xx wildcard with ep9301 > > > > .../bindings/timer/cirrus,ep93xx-timer.yaml | 41 > > +++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml > > > > diff --git a/Documentation/devicetree/bindings/timer/cirrus,ep93xx- > > timer.yaml b/Documentation/devicetree/bindings/timer/cirrus,ep93xx- > > timer.yaml > > new file mode 100644 > > index 000000000000..ce8b8a5cb90a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/timer/cirrus,ep93xx- > > timer.yaml > > @@ -0,0 +1,41 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/timer/cirrus,ep93xx-timer.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Cirrus Logic EP93xx timers bindings > > + > > +maintainers: > > + - Hartley Sweeten <hsweeten@visionengravers.com> > > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > > + > > +properties: > > + compatible: > > + const: cirrus,ep9301-timer > > 1. Why only one compatible? > 2. If this is kept, then filename matching compatible. I should rename the file to cirrus,ep9301-timer.yaml > > Best regards, > Krzysztof >
Hello Linus! On Wed, 2023-04-26 at 22:48 +0200, Linus Walleij wrote: > On Mon, Apr 24, 2023 at 6:32 PM Rob Herring <robh@kernel.org> wrote: > > On Mon, Apr 24, 2023 at 03:34:48PM +0300, Nikita Shubin wrote: > > > > Add YAML bindings for ep93xx SoC. > > > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > (...) > > > + chip-label: > > > + maxItems: 1 > > > + description: human readable name. > > > > Why do you need this? It's not standard and I don't see other GPIO > > controllers needing it. > > Caught my eye too, Nikita can you live without this and just use > dev_name() > or something to name the chip in Linux? > > If it is to conform to EP93xx documentation naming I guess it should > be > cirrus,ep93xx-gpio-chip-name = "..."; ? Nah, i should drop it, it was a reverence to people which are sad about gpio index reordering. Through i like the idea of "cirrus,ep93xx-gpio-chip-name". > > Yours, > Linus Walleij
On 4/24/2023 5:34 AM, Nikita Shubin wrote: > This series aims to convert ep93xx from platform to full device tree support. > > Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302. > > Thank you Linus and Arnd for your support, review and comments, sorry if i missed something - > these series are quite big for me. > > Big thanks to Alexander Sverdlin for his testing, support, review, fixes and patches. If anyone is interested I still have a TS-7300 board [1] that is fully functional and could be sent out to a new home. https://www.embeddedts.com/products/TS-7300
Hello Florian! On Mon, 2023-05-15 at 20:47 -0700, Florian Fainelli wrote: > > > On 4/24/2023 5:34 AM, Nikita Shubin wrote: > > This series aims to convert ep93xx from platform to full device > > tree support. > > > > Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302. > > > > Thank you Linus and Arnd for your support, review and comments, > > sorry if i missed something - > > these series are quite big for me. > > > > Big thanks to Alexander Sverdlin for his testing, support, review, > > fixes and patches. > > If anyone is interested I still have a TS-7300 board [1] that is > fully > functional and could be sent out to a new home. Thank you kindly, i'll keep this in mind ! > > https://www.embeddedts.com/products/TS-7300
On 01/06/2023 07:33, Nikita Shubin wrote: > This adds device tree bindings for the Cirrus Logic EP93xx. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> You already sent v1. This patchset is attached to the previous thread making it more complicated for me to process. This buries it deep in the mailbox and might interfere with applying entire sets. Is this the next version, so v3? You already had at least two versions before, so this cannot be v1. > --- > > Notes: > v0 -> v1: > > - fixed compatible - now it specifies three boards > - ts7250 > - bk3 > - edb9302 > - fixed identation in example > - dropped labels > > .../devicetree/bindings/arm/ep93xx.yaml | 107 ++++++++++++++++++ > .../dt-bindings/clock/cirrus,ep93xx-clock.h | 53 +++++++++ > 2 files changed, 160 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/ep93xx.yaml > create mode 100644 include/dt-bindings/clock/cirrus,ep93xx-clock.h > > diff --git a/Documentation/devicetree/bindings/arm/ep93xx.yaml b/Documentation/devicetree/bindings/arm/ep93xx.yaml > new file mode 100644 > index 000000000000..bcf9754d0763 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/ep93xx.yaml > @@ -0,0 +1,107 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/arm/ep93xx.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus Logic EP93xx device tree bindings No improvements. > + > +description: |+ no improvements. Do not need '|+' unless you need to preserve formatting. > + The EP93xx SoC is a ARMv4T-based with 200 MHz ARM9 CPU. > + > +maintainers: > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + - Nikita Shubin <nikita.shubin@maquefel.me> > + > +properties: > + $nodename: > + const: '/' > + compatible: > + oneOf: > + - description: The TS-7250 is a compact, full-featured Single Board Computer (SBC) > + based upon the Cirrus EP9302 ARM9 CPU > + items: > + - const: technologic,ts7250 > + - const: cirrus,ep9301 > + > + - description: The Liebherr BK3 is a derivate from ts7250 board > + items: > + - const: liebherr,bk3 > + - const: cirrus,ep9301 > + > + - description: EDB302 is an evaluation board by Cirrus Logic, > + based on a Cirrus Logic EP9302 CPU > + items: > + - const: cirrus,edb9302 > + - const: cirrus,ep9301 > + > + soc: > + type: object > + patternProperties: > + "^.*syscon@80930000$": > + type: object > + properties: > + compatible: > + items: > + - const: cirrus,ep9301-syscon > + - const: syscon > + - const: simple-mfd > + ep9301-reboot: > + type: object > + properties: > + compatible: > + const: cirrus,ep9301-reboot > + required: > + - compatible > + - reg > + - ep9301-reboot > + > + "^.*timer@80810000$": > + type: object > + properties: > + compatible: > + const: cirrus,ep9301-timer > + > + required: > + - syscon@80930000 > + - timer@80810000 I don't understand what are you putting here. Why addresses are in bindings (they should not be), why some nodes are documented in top-level compatible. Drop all this. Open existing files and look how it is done there. > + > +required: > + - compatible > + - soc> + > +additionalProperties: true > + > +examples: > + - | > + / { > + compatible = "technologic,ts7250", "cirrus,ep9301"; > + model = "TS-7250 SBC"; > + #address-cells = <1>; > + #size-cells = <1>; > + soc { > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + compatible = "simple-bus"; > + > + syscon@80930000 { > + compatible = "cirrus,ep9301-syscon", > + "syscon", "simple-mfd"; > + reg = <0x80930000 0x1000>; > + > + ep9301-reboot { > + compatible = "cirrus,ep9301-reboot"; > + }; > + }; > + > + timer@80810000 { > + compatible = "cirrus,ep9301-timer"; > + reg = <0x80810000 0x100>; > + interrupt-parent = <&vic1>; > + interrupts = <19>; > + }; > + }; > + }; Drop all this. There is no existing binding like that. > + > +... > diff --git a/include/dt-bindings/clock/cirrus,ep93xx-clock.h b/include/dt-bindings/clock/cirrus,ep93xx-clock.h Not related to top level compatible. > new file mode 100644 > index 000000000000..6a8cf33d811b > --- /dev/null > +++ b/include/dt-bindings/clock/cirrus,ep93xx-clock.h > @@ -0,0 +1,53 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ Dual license. > +#ifndef DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H > +#define DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H > + > +#define EP93XX_CLK_XTALI 0 > + Best regards, Krzysztof
On 01/06/2023 07:33, Nikita Shubin wrote: > This adds device tree bindings for the Cirrus Logic EP93xx > clock block used in these SoCs. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > > - it's now a clock controller > > .../bindings/clock/cirrus,ep9301.yaml | 64 +++++++++++++++++++ > 1 file changed, 64 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/cirrus,ep9301.yaml > > diff --git a/Documentation/devicetree/bindings/clock/cirrus,ep9301.yaml b/Documentation/devicetree/bindings/clock/cirrus,ep9301.yaml > new file mode 100644 > index 000000000000..4f9e0d483698 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/cirrus,ep9301.yaml > @@ -0,0 +1,64 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/cirrus,ep9301.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus Logic ep93xx SoC's clock controller > + > +maintainers: > + - Nikita Shubin <nikita.shubin@maquefel.me> > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + > +description: | > + Cirrus Logic EP93XX SoC clocks driver bindings. The clock First sentence is not suitable for bindings. Describe the hardware or skip it. > + controller node must be defined as a child node of the ep93xx > + system controller node. > + parent schema should define it... Best regards, Krzysztof
On 01/06/2023 07:33, Nikita Shubin wrote: > Add YAML bindings for ep93xx SoC pinctrl. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > > Notes: > v0 -> v1: > > Krzysztof Kozlowski: > - removed wildcards > - use fallback compatible and list all possible compatibles > - fix ident > - dropped bindings in title > > .../pinctrl/cirrus,ep9301-pinctrl.yaml | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml > new file mode 100644 > index 000000000000..ff7b30a11bab > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml > @@ -0,0 +1,66 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/cirrus,ep9301-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus ep93xx pins mux controller > + > +maintainers: > + - Nikita Shubin <nikita.shubin@maquefel.me> > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + > +properties: > + compatible: > + oneOf: > + - const: cirrus,ep9301-pinctrl > + - items: > + - enum: > + - cirrus,ep9302-pinctrl > + - cirrus,ep9307-pinctrl > + - cirrus,ep9312-pinctrl > + - cirrus,ep9315-pinctrl > + - const: cirrus,ep9301-pinctrl > + > +patternProperties: > + '^pins-': > + type: object > + description: pin node > + $ref: pinmux-node.yaml# > + > + properties: > + function: > + enum: [ spi, ac97, i2s, pwm, keypad, pata, lcd, gpio ] Blank line. > + groups: > + minItems: 1 > + maxItems: 2 How one pin can belong to two groups? What does it mean? > + items: > + enum: [ ssp, ac97, i2s_on_ssp, i2s_on_ac97, pwm1, gpio1agrp, > + gpio2agrp, gpio3agrp, gpio4agrp, gpio6agrp, gpio7agrp, > + rasteronsdram0grp, rasteronsdram3grp, keypadgrp, idegrp] > + > + required: > + - function > + - groups > + > +required: > + - compatible > + > +additionalProperties: false > + > +examples: > + - | > + syscon@80930000 { > + compatible = "cirrus,ep9301-syscon", > + "syscon", "simple-mfd"; Weird wrapping. > + reg = <0x80930000 0x1000>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + pinctrl { > + compatible = "cirrus,ep9312-pinctrl", "cirrus,ep9301-pinctrl"; > + spi_default_pins: pins-spi { > + function = "spi"; > + groups = "ssp"; > + }; > + }; > + }; Best regards, Krzysztof
On 01/06/2023 07:33, Nikita Shubin wrote: > This adds device tree bindings for the Cirrus Logic EP93xx > timer block used in these SoCs. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > Fix versioning. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hello Krzysztof! On Thu, 1 Jun 2023 08:37:02 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 01/06/2023 07:33, Nikita Shubin wrote: > > This adds device tree bindings for the Cirrus Logic EP93xx. > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > > You already sent v1. This patchset is attached to the previous thread > making it more complicated for me to process. This buries it deep in > the mailbox and might interfere with applying entire sets. Sorry for that, i've already realized my mistake looking at my own mailbox. > > Is this the next version, so v3? You already had at least two versions > before, so this cannot be v1. It's second on the mail lists, the first one was closed RFC. The first was without any version, i.e. v0, this one is v1 (should be v2). I promise to be more careful next series. All other comments acknowledged. > > > --- > > > > Notes: > > v0 -> v1: > > > > - fixed compatible - now it specifies three boards > > - ts7250 > > - bk3 > > - edb9302 > > - fixed identation in example > > - dropped labels > > > > .../devicetree/bindings/arm/ep93xx.yaml | 107 > > ++++++++++++++++++ .../dt-bindings/clock/cirrus,ep93xx-clock.h | > > 53 +++++++++ 2 files changed, 160 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/arm/ep93xx.yaml create mode > > 100644 include/dt-bindings/clock/cirrus,ep93xx-clock.h > > > > diff --git a/Documentation/devicetree/bindings/arm/ep93xx.yaml > > b/Documentation/devicetree/bindings/arm/ep93xx.yaml new file mode > > 100644 index 000000000000..bcf9754d0763 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/arm/ep93xx.yaml > > @@ -0,0 +1,107 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/arm/ep93xx.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Cirrus Logic EP93xx device tree bindings > > No improvements. > > > + > > +description: |+ > > no improvements. Do not need '|+' unless you need to preserve > formatting. > > > > + The EP93xx SoC is a ARMv4T-based with 200 MHz ARM9 CPU. > > + > > +maintainers: > > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > > + - Nikita Shubin <nikita.shubin@maquefel.me> > > + > > +properties: > > + $nodename: > > + const: '/' > > + compatible: > > + oneOf: > > + - description: The TS-7250 is a compact, full-featured > > Single Board Computer (SBC) > > + based upon the Cirrus EP9302 ARM9 CPU > > + items: > > + - const: technologic,ts7250 > > + - const: cirrus,ep9301 > > + > > + - description: The Liebherr BK3 is a derivate from ts7250 > > board > > + items: > > + - const: liebherr,bk3 > > + - const: cirrus,ep9301 > > + > > + - description: EDB302 is an evaluation board by Cirrus Logic, > > + based on a Cirrus Logic EP9302 CPU > > + items: > > + - const: cirrus,edb9302 > > + - const: cirrus,ep9301 > > + > > + soc: > > + type: object > > + patternProperties: > > + "^.*syscon@80930000$": > > + type: object > > + properties: > > + compatible: > > + items: > > + - const: cirrus,ep9301-syscon > > + - const: syscon > > + - const: simple-mfd > > + ep9301-reboot: > > + type: object > > + properties: > > + compatible: > > + const: cirrus,ep9301-reboot > > + required: > > + - compatible > > + - reg > > + - ep9301-reboot > > + > > + "^.*timer@80810000$": > > + type: object > > + properties: > > + compatible: > > + const: cirrus,ep9301-timer > > + > > + required: > > + - syscon@80930000 > > + - timer@80810000 > > I don't understand what are you putting here. Why addresses are in > bindings (they should not be), why some nodes are documented in > top-level compatible. Drop all this. > > Open existing files and look how it is done there. > > > + > > +required: > > + - compatible > > + - soc> + > > +additionalProperties: true > > + > > +examples: > > + - | > > + / { > > + compatible = "technologic,ts7250", "cirrus,ep9301"; > > + model = "TS-7250 SBC"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + soc { > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + compatible = "simple-bus"; > > + > > + syscon@80930000 { > > + compatible = "cirrus,ep9301-syscon", > > + "syscon", "simple-mfd"; > > + reg = <0x80930000 0x1000>; > > + > > + ep9301-reboot { > > + compatible = "cirrus,ep9301-reboot"; > > + }; > > + }; > > + > > + timer@80810000 { > > + compatible = "cirrus,ep9301-timer"; > > + reg = <0x80810000 0x100>; > > + interrupt-parent = <&vic1>; > > + interrupts = <19>; > > + }; > > + }; > > + }; > > Drop all this. There is no existing binding like that. > > > + > > +... > > diff --git a/include/dt-bindings/clock/cirrus,ep93xx-clock.h > > b/include/dt-bindings/clock/cirrus,ep93xx-clock.h > > Not related to top level compatible. > > > new file mode 100644 > > index 000000000000..6a8cf33d811b > > --- /dev/null > > +++ b/include/dt-bindings/clock/cirrus,ep93xx-clock.h > > @@ -0,0 +1,53 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > Dual license. > > > +#ifndef DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H > > +#define DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H > > + > > +#define EP93XX_CLK_XTALI 0 > > + > > > Best regards, > Krzysztof >
Hi Nikita, nikita.shubin@maquefel.me wrote on Thu, 1 Jun 2023 08:45:28 +0300: > Add YAML bindings for ts7250 NAND Controller. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > > make it a nand contoller > > .../bindings/mtd/technologic,nand.yaml | 47 +++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml > > diff --git a/Documentation/devicetree/bindings/mtd/technologic,nand.yaml b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > new file mode 100644 > index 000000000000..26d1d9c3331d > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > @@ -0,0 +1,47 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mtd/technologic,nand.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Technologic Systems NAND controller > + > +maintainers: > + - Nikita Shubin <nikita.shubin@maquefel.me> > + > +allOf: > + - $ref: nand-controller.yaml > + > +properties: > + compatible: > + oneOf: > + - const: technologic,ts7200-nand > + - items: > + - enum: > + - technologic,ts7300-nand > + - technologic,ts7260-nand > + - technologic,ts7250-nand > + - const: technologic,ts7200-nand > + > + reg: > + maxItems: 1 > + > + '#address-cells': true > + '#size-cells': true > + > +required: > + - compatible > + - reg > + > +unevaluatedProperties: true Should be false I guess. > + > +examples: > + - | > + nand-controller@60000000 { > + compatible = "technologic,ts7200-nand"; > + reg = <0x60000000 0x8000000>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > +... Thanks, Miquèl
On 01/06/2023 07:45, Nikita Shubin wrote: > Add YAML bindings for ts7250 NAND Controller. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > > make it a nand contoller > > .../bindings/mtd/technologic,nand.yaml | 47 +++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml > > diff --git a/Documentation/devicetree/bindings/mtd/technologic,nand.yaml b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > new file mode 100644 > index 000000000000..26d1d9c3331d > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > @@ -0,0 +1,47 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mtd/technologic,nand.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Technologic Systems NAND controller > + > +maintainers: > + - Nikita Shubin <nikita.shubin@maquefel.me> > + > +allOf: > + - $ref: nand-controller.yaml > + > +properties: > + compatible: > + oneOf: > + - const: technologic,ts7200-nand > + - items: > + - enum: > + - technologic,ts7300-nand > + - technologic,ts7260-nand > + - technologic,ts7250-nand > + - const: technologic,ts7200-nand > + > + reg: > + maxItems: 1 > + > + '#address-cells': true > + '#size-cells': true Except what Miquel wrote - drop these two. > + > +required: > + - compatible > + - reg > + > +unevaluatedProperties: true > + > +examples: > + - | > + nand-controller@60000000 { > + compatible = "technologic,ts7200-nand"; > + reg = <0x60000000 0x8000000>; > + #address-cells = <1>; > + #size-cells = <0>; Incomplete example. address/size cells do not make sense here alone. Finish the example. > + }; > + > +... Best regards, Krzysztof
On 01/06/2023 07:34, Nikita Shubin wrote: > Add YAML bindings for ep93xx SoC SPI. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > Krzysztof Kozlowski: > - replaced maintainers > - removed wildcards > - use fallback compatible and list all possible compatibles > - drop quotes in ref > - dropped "clock-names" > - dropped label > - fix ident > > .../devicetree/bindings/spi/spi-ep9301.yaml | 69 +++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/spi-ep9301.yaml > > diff --git a/Documentation/devicetree/bindings/spi/spi-ep9301.yaml b/Documentation/devicetree/bindings/spi/spi-ep9301.yaml > new file mode 100644 > index 000000000000..c363b25a3074 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/spi-ep9301.yaml > @@ -0,0 +1,69 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/spi/spi-ep9301.yaml# Filename based on compatible, so missing prefix, wrong order of name components. This applies everywhere, not to some files only. Applied to all your bindings. > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: EP93xx SoC SPI controller > + > +maintainers: > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + - Nikita Shubin <nikita.shubin@maquefel.me> > + > +allOf: > + - $ref: spi-controller.yaml# > + > +properties: > + "#address-cells": true > + "#size-cells": true Drop these two. > + > + compatible: Anyway, compatible is always first. > + oneOf: > + - const: cirrus,ep9301-spi > + - items: > + - enum: > + - cirrus,ep9302-spi > + - cirrus,ep9307-spi > + - cirrus,ep9312-spi > + - cirrus,ep9315-spi > + - const: cirrus,ep9301-spi > + > + reg: > + items: > + - description: SPI registers region > + > + interrupts: > + maxItems: 1 > + > + clocks: > + items: > + - description: SPI Controller reference clock source > + > + cs-gpios: true Drop, not needed. > + > + cirrus,ep9301-use-dma: > + description: Flag indicating that the SPI should use dma > + type: boolean In such case where are dmas? Unless you meant some internal dma controller? In such case extend the description because now it just duplicates property name. > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/cirrus,ep93xx-clock.h> > + spi@808a0000 { > + compatible = "cirrus,ep9301-spi"; > + reg = <0x808a0000 0x18>; > + interrupt-parent = <&vic1>; > + interrupts = <21>; > + clocks = <&syscon EP93XX_CLK_SPI>; > + cs-gpios = <&gpio5 2 0>; Use proper gpio defines for flags. > + cirrus,ep9301-use-dma; > + }; > + > +... Best regards, Krzysztof
On 01/06/2023 07:45, Nikita Shubin wrote: > Add YAML bindings for ST M48T86 / Dallas DS12887 RTC. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > > - s/dallas/st/ > - description for regs > - s/additionalProperties/unevaluatedProperties/ > - add ref rtc.yaml > - changed compatible to st,m48t86 > - dropped label in example > - replaced Alessandro Alessandro to Alexandre Belloni > > .../bindings/rtc/st,m48t86-rtc.yaml | 38 +++++++++++++++++++ > 1 file changed, 38 insertions(+) > create mode 100644 Documentation/devicetree/bindings/rtc/st,m48t86-rtc.yaml > > diff --git a/Documentation/devicetree/bindings/rtc/st,m48t86-rtc.yaml b/Documentation/devicetree/bindings/rtc/st,m48t86-rtc.yaml > new file mode 100644 > index 000000000000..eb8e6451d7c8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/st,m48t86-rtc.yaml Filename based on compatible, so drop "rtc". > @@ -0,0 +1,38 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/rtc/st,m48t86-rtc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ST M48T86 / Dallas DS12887 RTC wirh SRAM typo: with > + > +maintainers: > + - Alexandre Belloni <alexandre.belloni@bootlin.com> > + > +properties: > + compatible: > + enum: > + - st,m48t86 > + > + reg: > + items: > + - description: index register > + - description: data register > + > +allOf: > + - $ref: rtc.yaml > + > +unevaluatedProperties: false > + > +required: > + - compatible > + - reg required goes after properties: Keep the same order in all your patches. > + > +examples: > + - | > + rtc@10800000 { > + compatible = "st,m48t86"; > + reg = <0x10800000 0x1>, <0x11700000 0x1>; One byte long? Not a word? > + }; > + > +... Best regards, Krzysztof
On Thu, 01 Jun 2023 08:45:32 +0300, Nikita Shubin wrote: > Add YAML bindings for ep93xx SoC keypad. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > > - remove almost all but debounce-delay-ms and prescale > - s/ep9301-keypad/ep9307-keypad/ it's actually only for > ep9307, ep9312, ep9315 > > Krzysztof Kozlowski: > - renamed file > - changed maintainers > - dropped quotes > - dropped clock-names > - use fallback compatible and list all possible compatibles > - fix ident > > .../bindings/input/cirrus,ep9307-keypad.yaml | 86 +++++++++++++++++++ > 1 file changed, 86 insertions(+) > create mode 100644 Documentation/devicetree/bindings/input/cirrus,ep9307-keypad.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: Documentation/devicetree/bindings/input/cirrus,ep9307-keypad.example.dtb: /example-0/keypad@800f0000: failed to match any schema with compatible: ['cirrus,ep9301-keypad'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230601054549.10843-9-nikita.shubin@maquefel.me 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 01/06/2023 07:45, Nikita Shubin wrote: > This adds a divice for Cirrus ep93xx SoC amd ts7250 board that has been device > my testing target for ep93xx device support. > > Also inluded device tree for Liebherr BK3.1 board through it's not a included > complete support. Thank you for your patch. There is something to discuss/improve. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > > - add empty chosen node > - s/dallas,rtc-m48t86/st,m48t86/ > - changed phy_id to phy-handle > - dropped gpio chip-label's > - s/eth@80010000/ethernet@80010000 > - s/use_dma/ep9301,use-dma > - added i2s to bk3 > > arch/arm/boot/dts/Makefile | 1 + > arch/arm/boot/dts/ep93xx-bk3.dts | 119 +++++++ > arch/arm/boot/dts/ep93xx-ts7250.dts | 132 ++++++++ > arch/arm/boot/dts/ep93xx.dtsi | 466 ++++++++++++++++++++++++++++ Split adding DTSI from adding boards. > 4 files changed, 718 insertions(+) > create mode 100644 arch/arm/boot/dts/ep93xx-bk3.dts > create mode 100644 arch/arm/boot/dts/ep93xx-ts7250.dts > create mode 100644 arch/arm/boot/dts/ep93xx.dtsi > > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile > index 59829fc90315..a68f868fffe7 100644 > --- a/arch/arm/boot/dts/Makefile > +++ b/arch/arm/boot/dts/Makefile > @@ -1670,3 +1670,4 @@ dtb-$(CONFIG_ARCH_ASPEED) += \ > aspeed-bmc-vegman-n110.dtb \ > aspeed-bmc-vegman-rx20.dtb \ > aspeed-bmc-vegman-sx20.dtb > +dtb-$(CONFIG_ARCH_EP93XX) += ep93xx-ts7250.dtb > diff --git a/arch/arm/boot/dts/ep93xx-bk3.dts b/arch/arm/boot/dts/ep93xx-bk3.dts > new file mode 100644 > index 000000000000..215587c498e6 > --- /dev/null > +++ b/arch/arm/boot/dts/ep93xx-bk3.dts > @@ -0,0 +1,119 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Device Tree file for Liebherr controller BK3.1 based on Cirrus EP9302 SoC > + */ > +/dts-v1/; > +#include "ep93xx.dtsi" > + > +/ { > + model = "Liebherr controller BK3.1"; > + compatible = "liebherr,bk3", "cirrus,ep9301"; > + > + chosen { > + }; > + > + memory { > + device_type = "memory"; > + }; > + > + soc { > + nand-controller@60000000 { Override/extend by label/phandle. > + compatible = "technologic,ts7200-nand"; > + reg = <0x60000000 0x8000000>; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "System"; > + reg = <0x00000000 0x01e00000>; > + read-only; > + }; > + > + partition@1e00000 { > + label = "Data"; > + reg = <0x01e00000 0x05f20000>; > + }; > + > + partition@7d20000 { > + label = "RedBoot"; > + reg = <0x07d20000 0x002e0000>; > + read-only; > + }; > + }; > + }; > + > + syscon: syscon@80930000 { Override/extend by label/phandle. > + pinctrl: pinctrl { > + compatible = "cirrus,ep9301-pinctrl"; Why this is board specific? > + }; > + }; > + > + gpio1: gpio@80840004 { Override/extend by label/phandle. > + /* PWM */ > + gpio-ranges = <&pinctrl 6 163 1>; > + }; > + }; > +}; > + > +&gpio1 { > + /* PWM */ > + gpio-ranges = <&pinctrl 6 163 1>; > +}; > + > +&gpio4 { > + gpio-ranges = <&pinctrl 0 97 2>; > + status = "okay"; > +}; > + > +&gpio6 { > + gpio-ranges = <&pinctrl 0 87 2>; > + status = "okay"; > +}; > + > +&gpio7 { > + gpio-ranges = <&pinctrl 2 199 4>; > + status = "okay"; > +}; > + > +&i2c { > + status = "okay"; > +}; > + > +&spi0: spi@808a0000 { > + cs-gpios = <&gpio5 3 0>; Use proper defines for flags. > + status = "okay"; What's here? Empty enabled bus? > +}; > + > +ð0 { > + phy-handle = <&phy0>; > +}; > + > +&mdio0 { > + phy0: ethernet-phy@1 { > + reg = <1>; > + device_type = "ethernet-phy"; > + }; > +}; > + > +&uart0 { > + status = "okay"; > +}; > + > +&uart1 { > + status = "okay"; > +}; > + > +&usb { > + status = "okay"; > +}; > + > +&i2s { Up to you, but I seriously recommend keeping all labels ordered by name. Avoids conflicts. > + pinctrl-names = "default"; > + pinctrl-0 = <&i2s_on_ac97_pins>; > + /delete-property/ status; ??? I don't understand. Why would you do this? ... > diff --git a/arch/arm/boot/dts/ep93xx.dtsi b/arch/arm/boot/dts/ep93xx.dtsi > new file mode 100644 > index 000000000000..6da556ceaf04 > --- /dev/null > +++ b/arch/arm/boot/dts/ep93xx.dtsi > @@ -0,0 +1,466 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Device Tree file for Cirrus Logic systems EP93XX SoC > + */ > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/leds/common.h> > +#include <dt-bindings/input/input.h> > +#include <dt-bindings/clock/cirrus,ep93xx-clock.h> > +/ { > + soc { > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + compatible = "simple-bus"; > + > + syscon: syscon@80930000 { > + compatible = "cirrus,ep9301-syscon", > + "syscon", "simple-mfd"; Broken wrapping. Align these with previous ". > + reg = <0x80930000 0x1000>; > + > + ep9301-reboot { Just "reboot" (and fix bindings)... but why would you need it in the first place? I think something is seriously missing in your bindings. > + compatible = "cirrus,ep9301-reboot"; > + }; > + > + eclk: clock-controller { > + #clock-cells = <1>; > + compatible = "cirrus,ep9301-clk"; > + status = "okay"; > + clocks = <&xtali>; > + }; > + > + pinctrl: pinctrl { Missing compatible. > + spi_default_pins: pins-spi { > + function = "spi"; > + groups = "ssp"; > + }; > + > + ac97_default_pins: pins-ac97 { > + function = "ac97"; > + groups = "ac97"; > + }; > + > + i2s_on_ssp_pins: pins-i2sonssp { > + function = "i2s"; > + groups = "i2s_on_ssp"; > + }; > + > + i2s_on_ac97_pins: pins-i2sonac97 { > + function = "i2s"; > + groups = "i2s_on_ac97"; > + }; > + > + gpio1_default_pins: pins-gpio1 { > + function = "gpio"; > + groups = "gpio1agrp"; > + }; > + > + pwm1_default_pins: pins-pwm1 { > + function = "pwm"; > + groups = "pwm1"; > + }; > + > + gpio2_default_pins: pins-gpio2 { > + function = "gpio"; > + groups = "gpio2agrp"; > + }; > + > + gpio3_default_pins: pins-gpio3 { > + function = "gpio"; > + groups = "gpio3agrp"; > + }; > + > + keypad_default_pins: pins-keypad { > + function = "keypad"; > + groups = "keypadgrp"; > + }; > + > + gpio4_default_pins: pins-gpio4 { > + function = "gpio"; > + groups = "gpio4agrp"; > + }; > + > + gpio6_default_pins: pins-gpio6 { > + function = "gpio"; > + groups = "gpio6agrp"; > + }; > + > + gpio7_default_pins: pins-gpio7 { > + function = "gpio"; > + groups = "gpio7agrp"; > + }; > + > + ide_default_pins: pins-ide { > + function = "pata"; > + groups = "idegrp"; > + }; > + > + lcd_on_dram0_pins: pins-rasteronsdram0 { > + function = "lcd"; > + groups = "rasteronsdram0grp"; > + }; > + > + lcd_on_dram3_pins: pins-rasteronsdram3 { > + function = "lcd"; > + groups = "rasteronsdram3grp"; I would expect somewhere two groups since you explicitly allow it. > + }; > + }; > + }; > + > + vic0: interrupt-controller@800b0000 { > + compatible = "arm,pl192-vic"; > + interrupt-controller; > + reg = <0x800b0000 0x1000>; compatible first, reg is second. ranges if present - third. Fix it everywhere. > + #interrupt-cells = <1>; > + valid-mask = <0x7ffffffc>; > + valid-wakeup-mask = <0x0>; > + }; > + > + vic1: interrupt-controller@800c0000 { > + compatible = "arm,pl192-vic"; > + interrupt-controller; > + reg = <0x800c0000 0x1000>; > + #interrupt-cells = <1>; > + valid-mask = <0x1fffffff>; > + valid-wakeup-mask = <0x0>; > + }; > + > + timer: timer@80810000 { > + compatible = "cirrus,ep9301-timer"; > + reg = <0x80810000 0x100>; > + interrupt-parent = <&vic1>; > + interrupts = <19>; > + }; > + > + dma0: dma-controller@80000000 { > + compatible = "cirrus,ep9301-dma-m2p"; > + reg = <0x80000000 0x0040>, > + <0x80000040 0x0040>, > + <0x80000080 0x0040>, > + <0x800000c0 0x0040>, > + <0x80000240 0x0040>, > + <0x80000200 0x0040>, > + <0x800002c0 0x0040>, > + <0x80000280 0x0040>, > + <0x80000340 0x0040>, > + <0x80000300 0x0040>; > + clocks = <&eclk EP93XX_CLK_M2P0>, > + <&eclk EP93XX_CLK_M2P1>, > + <&eclk EP93XX_CLK_M2P2>, > + <&eclk EP93XX_CLK_M2P3>, > + <&eclk EP93XX_CLK_M2P4>, > + <&eclk EP93XX_CLK_M2P5>, > + <&eclk EP93XX_CLK_M2P6>, > + <&eclk EP93XX_CLK_M2P7>, > + <&eclk EP93XX_CLK_M2P8>, > + <&eclk EP93XX_CLK_M2P9>; > + clock-names = "m2p0", "m2p1", > + "m2p2", "m2p3", > + "m2p4", "m2p5", > + "m2p6", "m2p7", > + "m2p8", "m2p9"; > + interrupt-parent = <&vic0>; > + interrupts = <7>, <8>, <9>, <10>, <11>, > + <12>, <13>, <14>, <15>, <16>; > + #dma-cells = <1>; > + }; > + > + dma1: dma-controller@80000100 { > + compatible = "cirrus,ep9301-dma-m2m"; > + reg = <0x80000100 0x0040>, > + <0x80000140 0x0040>; > + clocks = <&eclk EP93XX_CLK_M2M0>, > + <&eclk EP93XX_CLK_M2M1>; > + clock-names = "m2m0", "m2m1"; > + interrupt-parent = <&vic0>; > + interrupts = <17>, <18>; > + #dma-cells = <1>; > + }; > + > + i2s: i2s@80820000 { > + compatible = "cirrus,ep9301-i2s"; > + #sound-dai-cells = <0>; > + reg = <0x80820000 0x100>; > + interrupt-parent = <&vic1>; > + interrupts = <28>; > + clocks = <&eclk EP93XX_CLK_I2S_MCLK > + &eclk EP93XX_CLK_I2S_SCLK > + &eclk EP93XX_CLK_I2S_LRCLK>; > + clock-names = "mclk", "sclk", "lrclk"; > + status = "disabled"; > + }; > + > + gpio0: gpio@80840000 { > + compatible = "cirrus,ep9301-gpio"; > + reg = <0x80840000 0x04>, > + <0x80840010 0x04>, > + <0x80840090 0x1c>; Messed wrapping. > + reg-names = "data", "dir", "intr"; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-controller; > + interrupt-parent = <&vic1>; > + interrupts = <27>; > + }; > + > + gpio1: gpio@80840004 { > + compatible = "cirrus,ep9301-gpio"; > + reg = <0x80840004 0x04>, > + <0x80840014 0x04>, > + <0x808400ac 0x1c>; Ditto, in other places as well. > + reg-names = "data", "dir", "intr"; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-controller; > + interrupt-parent = <&vic1>; > + interrupts = <27>; > + }; > + > + gpio2: gpio@80840008 { > + compatible = "cirrus,ep9301-gpio"; > + reg = <0x80840008 0x04>, > + <0x80840018 0x04>; > + reg-names = "data", "dir"; > + gpio-controller; > + #gpio-cells = <2>; > + status = "disabled"; Status is usually last. > + pinctrl-names = "default"; > + pinctrl-0 = <&gpio2_default_pins>; > + }; > + > + gpio3: gpio@8084000c { > + compatible = "cirrus,ep9301-gpio"; > + reg = <0x8084000c 0x04>, > + <0x8084001c 0x04>; > + reg-names = "data", "dir"; > + gpio-controller; > + #gpio-cells = <2>; > + status = "disabled"; > + pinctrl-names = "default"; > + pinctrl-0 = <&gpio3_default_pins>; > + }; > + > + gpio4: gpio@80840020 { > + compatible = "cirrus,ep9301-gpio"; > + reg = <0x80840020 0x04>, > + <0x80840024 0x04>; > + reg-names = "data", "dir"; > + gpio-controller; > + #gpio-cells = <2>; > + status = "disabled"; > + pinctrl-names = "default"; > + pinctrl-0 = <&gpio4_default_pins>; > + }; > + > + gpio5: gpio@80840030 { > + compatible = "cirrus,ep9301-gpio"; > + reg = <0x80840030 0x04>, > + <0x80840034 0x04>, > + <0x8084004c 0x1c>; > + reg-names = "data", "dir", "intr"; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-controller; > + interrupts-extended = <&vic0 19>, <&vic0 20>, > + <&vic0 21>, <&vic0 22>, > + <&vic1 15>, <&vic1 16>, > + <&vic1 17>, <&vic1 18>; > + }; > + > + gpio6: gpio@80840038 { > + compatible = "cirrus,ep9301-gpio"; > + reg = <0x80840038 0x04>, > + <0x8084003c 0x04>; > + reg-names = "data", "dir"; > + gpio-controller; > + #gpio-cells = <2>; > + status = "disabled"; > + pinctrl-names = "default"; > + pinctrl-0 = <&gpio6_default_pins>; > + }; > + > + gpio7: gpio@80840040 { > + compatible = "cirrus,ep9301-gpio"; > + reg = <0x80840040 0x04>, > + <0x80840044 0x04>; > + reg-names = "data", "dir"; > + gpio-controller; > + #gpio-cells = <2>; > + status = "disabled"; > + pinctrl-names = "default"; > + pinctrl-0 = <&gpio7_default_pins>; > + }; > + > + ide: ide@800a0000 { > + compatible = "cirrus,ep9312-pata"; > + reg = <0x800a0000 0x38>; > + interrupt-parent = <&vic1>; > + interrupts = <8>; > + status = "disabled"; > + pinctrl-names = "default"; > + pinctrl-0 = <&ide_default_pins>; > + }; > + > + uart0: uart@808c0000 { This should scream with dtbs_check. serial. It does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + compatible = "arm,primecell"; > + reg = <0x808c0000 0x1000>; > + arm,primecell-periphid = <0x00041010>; > + clocks = <&eclk EP93XX_CLK_UART1>, <&eclk EP93XX_CLK_UART>; > + clock-names = "apb:uart1", "apb_pclk"; > + interrupt-parent = <&vic1>; > + interrupts = <20>; > + status = "disabled"; > + }; > + > + uart1: uart@808d0000 { > + compatible = "arm,primecell"; > + reg = <0x808d0000 0x1000>; > + arm,primecell-periphid = <0x00041010>; > + clocks = <&eclk EP93XX_CLK_UART2>, <&eclk EP93XX_CLK_UART>; > + clock-names = "apb:uart2", "apb_pclk"; > + interrupt-parent = <&vic1>; > + interrupts = <22>; > + status = "disabled"; > + }; > + > + uart2: uart@808b0000 { > + compatible = "arm,primecell"; > + reg = <0x808b0000 0x1000>; > + arm,primecell-periphid = <0x00041010>; > + clocks = <&eclk EP93XX_CLK_UART3>, <&eclk EP93XX_CLK_UART>; > + clock-names = "apb:uart3", "apb_pclk"; > + interrupt-parent = <&vic1>; > + interrupts = <23>; > + status = "disabled"; > + }; > + > + usb0: usb@80020000 { > + compatible = "generic-ohci"; > + reg = <0x80020000 0x10000>; > + interrupt-parent = <&vic1>; > + interrupts = <24>; > + clocks = <&eclk EP93XX_CLK_USB>; > + status = "disabled"; > + }; > + > + eth0: ethernet@80010000 { > + compatible = "cirrus,ep9301-eth"; > + reg = <0x80010000 0x10000>; > + interrupt-parent = <&vic1>; > + interrupts = <7>; > + mdio0: mdio { > + #address-cells = <1>; > + #size-cells = <0>; Your SoC comes with mdio? If so, why is this empty? > + }; > + }; > + > + rtc0: rtc@80920000 { > + compatible = "cirrus,ep9301-rtc"; > + reg = <0x80920000 0x100>; > + }; > + > + spi0: spi@808a0000 { > + compatible = "cirrus,ep9301-spi"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0x808a0000 0x18>; > + interrupt-parent = <&vic1>; > + interrupts = <21>; > + clocks = <&eclk EP93XX_CLK_SPI>; > + cs-gpios = <&gpio5 2 0>; defines... but why is it here in the first place? Rarely CS gpios are part of the SoC. I have several of such questions further as well, so this looks like you are mixing SoC and boards in one file. > + cirrus,ep9301-use-dma; > + pinctrl-names = "default"; > + pinctrl-0 = <&spi_default_pins>; > + status = "disabled"; > + }; > + > + adc: adc@80900000 { > + compatible = "cirrus,ep9301-adc"; > + reg = <0x80900000 0x28>; > + clocks = <&eclk EP93XX_CLK_ADC>; > + interrupt-parent = <&vic0>; > + interrupts = <30>; > + status = "disabled"; > + }; > + > + watchdog0: watchdog@80940000 { > + compatible = "cirrus,ep9301-wdt"; > + reg = <0x80940000 0x08>; > + }; > + > + pwm0: pwm@80910000 { > + compatible = "cirrus,ep9301-pwm"; > + reg = <0x80910000 0x10>; > + clocks = <&eclk EP93XX_CLK_PWM>; > + status = "disabled"; > + }; > + > + pwm1: pwm@80910020 { > + compatible = "cirrus,ep9301-pwm"; > + reg = <0x80910020 0x10>; > + clocks = <&eclk EP93XX_CLK_PWM>; > + status = "disabled"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pwm1_default_pins>; > + }; > + > + keypad: keypad@800f0000 { > + compatible = "cirrus,ep9307-keypad"; > + reg = <0x800f0000 0x0c>; > + interrupt-parent = <&vic0>; > + interrupts = <29>; > + clocks = <&eclk EP93XX_CLK_KEYPAD>; > + pinctrl-names = "default"; > + pinctrl-0 = <&keypad_default_pins>; > + linux,keymap = > + <KEY_UP>, > + <KEY_DOWN>, > + <KEY_VOLUMEDOWN>, > + <KEY_HOME>, > + <KEY_RIGHT>, > + <KEY_LEFT>, > + <KEY_ENTER>, > + <KEY_VOLUMEUP>, > + <KEY_F6>, > + <KEY_F8>, > + <KEY_F9>, > + <KEY_F10>, > + <KEY_F1>, > + <KEY_F2>, > + <KEY_F3>, > + <KEY_POWER>; > + }; > + }; > + > + xtali: oscillator { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <14745600>; > + clock-output-names = "xtali"; > + }; > + > + i2c0: i2c0 { i2c or i2c-0 > + compatible = "i2c-gpio"; > + sda-gpios = <&gpio6 1 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; > + scl-gpios = <&gpio6 0 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; Wait, what? Why this is disabled? If this is part of the SoC, although hardly looks like, then it should be complete. What is missing? How one could design SoC with incomplete GPIO I2C controller? > + }; > + > + leds { > + compatible = "gpio-leds"; I really doubt this is property of the SoC. Please double check as it really looks wrong. > + led0 { led-0 > + label = "grled"; > + gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>; > + linux,default-trigger = "heartbeat"; > + function = LED_FUNCTION_HEARTBEAT; > + }; > + > + led1 { led-1 > + label = "rdled"; > + gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>; > + function = LED_FUNCTION_FAULT; > + }; > + }; > +}; Best regards, Krzysztof
On 01/06/2023 07:45, Nikita Shubin wrote: > From: Alexander Sverdlin <alexander.sverdlin@gmail.com> > > Add device tree for Cirrus EDB9302. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > > - added USB > - dropped 'Missing USB' in commit message > - add mdio + eth phy > > arch/arm/boot/dts/ep93xx-edb9302.dts | 160 +++++++++++++++++++++++++++ > 1 file changed, 160 insertions(+) > create mode 100644 arch/arm/boot/dts/ep93xx-edb9302.dts > > diff --git a/arch/arm/boot/dts/ep93xx-edb9302.dts b/arch/arm/boot/dts/ep93xx-edb9302.dts > new file mode 100644 > index 000000000000..3ec89f7587db > --- /dev/null > +++ b/arch/arm/boot/dts/ep93xx-edb9302.dts > @@ -0,0 +1,160 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +/* > + * Device Tree file for Cirrus Logic EDB9302 board based on EP9302 SoC > + */ > +/dts-v1/; > +#include "ep93xx.dtsi" > + > +/ { > + #address-cells = <1>; > + #size-cells = <1>; > + compatible = "cirrus,edb9302", "cirrus,ep9301"; > + model = "cirrus,edb9302"; > + > + chosen { > + }; > + > + memory { > + device_type = "memory"; > + }; > + > + soc { > + flash@60000000 { Same comments - override by label/phandle. > + compatible = "cfi-flash"; > + reg = <0x60000000 0x1000000>; Are you sure that your board adds things to the SoC? The code suggests that, but I would like to see such circuit. > + bank-width = <2>; > + }; > + }; > + > + sound { > + compatible = "simple-audio-card"; > + simple-audio-card,name = "EDB93XX"; > + simple-audio-card,format = "i2s"; > + simple-audio-card,mclk-fs = <256>; > + simple-audio-card,convert-channels = <2>; > + simple-audio-card,convert-sample-format = "s32_le"; > + > + simple-audio-card,cpu { > + sound-dai = <&i2s>; > + system-clock-direction-out; > + frame-master; > + bitclock-master; > + dai-sample-format = "s32_le"; > + dai-channels = <2>; > + }; > + > + simple-audio-card,codec { > + sound-dai = <&codec>; > + }; > + }; > +}; > + > +&pinctrl { > + compatible = "cirrus,ep9301-pinctrl"; > +}; > + > +&gpio0 { > + gpio-ranges = <&pinctrl 0 153 1>, > + <&pinctrl 1 152 1>, > + <&pinctrl 2 151 1>, > + <&pinctrl 3 148 1>, > + <&pinctrl 4 147 1>, > + <&pinctrl 5 146 1>, > + <&pinctrl 6 145 1>, > + <&pinctrl 7 144 1>; > +}; > + > +&gpio1 { > + gpio-ranges = <&pinctrl 0 143 1>, > + <&pinctrl 1 142 1>, > + <&pinctrl 2 141 1>, > + <&pinctrl 3 140 1>, > + <&pinctrl 4 165 1>, > + <&pinctrl 5 164 1>, > + <&pinctrl 6 163 1>, > + <&pinctrl 7 160 1>; > +}; > + > +&gpio2 { > + gpio-ranges = <&pinctrl 0 115 1>; > + /delete-property/ status; ??? > + /delete-property/ pinctrl-0; > + /delete-property/ pinctrl-names; ??? I have no clue what you are trying to achieve here but this is a proof your DTSI is bogus. You should never remove statuses. You should never need to remove pinctrl as these are board dependent, not SoC. If you remove them, it means they are not part of SoC in the first place! Best regards, Krzysztof
On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote: > + cirrus,ep9301-use-dma: > + description: Flag indicating that the SPI should use dma > + type: boolean My previous feedback on this property still applies. Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed. Having to repeat the same comments can get repetitive and make people question the value of time spent reviewing. If you disagree with the review comments that's fine but you need to reply and discuss your concerns so that the reviewer can understand your decisions.
Hello Mark! On Thu, 1 Jun 2023 12:17:27 +0100 Mark Brown <broonie@kernel.org> wrote: > On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote: > > > + cirrus,ep9301-use-dma: > > + description: Flag indicating that the SPI should use dma > > + type: boolean > > My previous feedback on this property still applies. > > Please don't ignore review comments, people are generally making them > for a reason and are likely to have the same concerns if issues remain > unaddressed. Having to repeat the same comments can get repetitive > and make people question the value of time spent reviewing. If you > disagree with the review comments that's fine but you need to reply > and discuss your concerns so that the reviewer can understand your > decisions. Sorry - that was totally unintentional, i was tinkering with spi and got distracted on other part of this series (it's quite big for me, first time tinkering with a series more than 5-6 patches). > > + cirrus,ep9301-use-dma: The reason is that ep93xx DMA state is not quite device-tree ready at this moment, and clients use it with the help of: https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/platform_data/dma-ep93xx.h I was hoping to slip by without changing much in ep93xx DMA driver, so i can deal with it later, especially seeing it's having some quirks like: https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/dma/ep93xx_dma.c#L471 And edb93xx and bk3 don't set use_dma with SPI for some reason. I can move "use-dma" to module parameters, if this is acceptable.
On Thu, Jun 01, 2023 at 03:41:54PM +0300, Nikita Shubin wrote: > Mark Brown <broonie@kernel.org> wrote: > > On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote: > > > + cirrus,ep9301-use-dma: > > > + description: Flag indicating that the SPI should use dma > > > + type: boolean > > My previous feedback on this property still applies. > > > + cirrus,ep9301-use-dma: > The reason is that ep93xx DMA state is not quite device-tree ready at > this moment, and clients use it with the help of: > https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/platform_data/dma-ep93xx.h > I was hoping to slip by without changing much in ep93xx DMA driver, so You're definign new ABI here, that's not a good thing to do for a temporary workaround. > I can move "use-dma" to module parameters, if this is acceptable. That's less bad. I guess you could also define the bindings for the DMA controller so that the properties are there then instead of properly using the DMA API in the clients just check to see if the DMA properties are present and then proceed accordingly?
On Thu, 1 Jun 2023 13:55:03 +0100 Mark Brown <broonie@kernel.org> wrote: > On Thu, Jun 01, 2023 at 03:41:54PM +0300, Nikita Shubin wrote: > > Mark Brown <broonie@kernel.org> wrote: > > > On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote: > > > > > + cirrus,ep9301-use-dma: > > > > + description: Flag indicating that the SPI should use dma > > > > + type: boolean > > > > My previous feedback on this property still applies. > > > > > + cirrus,ep9301-use-dma: > > > The reason is that ep93xx DMA state is not quite device-tree ready > > at this moment, and clients use it with the help of: > > > https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/platform_data/dma-ep93xx.h > > > > > I was hoping to slip by without changing much in ep93xx DMA driver, > > so > > You're definign new ABI here, that's not a good thing to do for a > temporary workaround. > > > I can move "use-dma" to module parameters, if this is acceptable. > > That's less bad. I guess you could also define the bindings for the > DMA controller so that the properties are there then instead of > properly using the DMA API in the clients just check to see if the > DMA properties are present and then proceed accordingly? This sounds like a way to go. Thank you, Mark!
On 6/1/23 14:45, Nikita Shubin wrote: > Add YAML bindings for ep93xx SoC PATA. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > > Notes: > v0 -> v1: > > - renamed file to ep9312-pata Looks OK to me but given that this is both for the cirrus,ep9315-pata and cirrus,ep9312-pata, wouldn't it be better to name the file cirrus,ep931x-pata.yaml ? > - changed email to dlemoal@kernel.org > - dropped label > - fixed ident > > .../bindings/ata/cirrus,ep9312-pata.yaml | 44 +++++++++++++++++++ > 1 file changed, 44 insertions(+) > create mode 100644 Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml > > diff --git a/Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml b/Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml > new file mode 100644 > index 000000000000..3489be55a6fe > --- /dev/null > +++ b/Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml > @@ -0,0 +1,44 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/ata/cirrus,ep9312-pata.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus Logic EP9312 PATA controller > + > +maintainers: > + - Damien Le Moal <dlemoal@kernel.org> > + > +properties: > + compatible: > + oneOf: > + - const: cirrus,ep9312-pata I am not a DT specialist, but isn't this line superfluous since it is listed in the items ? > + - items: > + - const: cirrus,ep9315-pata > + - const: cirrus,ep9312-pata > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - interrupts > + > +additionalProperties: false > + > +examples: > + - | > + ide@800a0000 { > + compatible = "cirrus,ep9312-pata"; > + reg = <0x800a0000 0x38>; > + interrupt-parent = <&vic1>; > + interrupts = <8>; > + pinctrl-names = "default"; > + pinctrl-0 = <&ide_default_pins>; > + }; > + > +...
Hello Damien! On Fri, 2023-06-02 at 08:57 +0900, Damien Le Moal wrote: > On 6/1/23 14:45, Nikita Shubin wrote: > > Add YAML bindings for ep93xx SoC PATA. > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > > --- > > > > Notes: > > v0 -> v1: > > > > - renamed file to ep9312-pata > > Looks OK to me but given that this is both for the cirrus,ep9315-pata > and > cirrus,ep9312-pata, wouldn't it be better to name the file > cirrus,ep931x-pata.yaml ? I was advised against using wildcards by Arnd and Krzysztof. See https://lore.kernel.org/all/c981e048-8925-deba-6916-9199844976b9@linaro.org/ As i understood we should have at least one fallback, in out case it's "cirrus,ep9312-pata" and one for each SoC variant that supports it. All other comments acknowledged and agreed. I will also change ``` >> + if (!drv_data) >> + return -ENXIO; ``` To ENOMEM, as a part of dt conversion patch in v2. > > > > - changed email to dlemoal@kernel.org > > - dropped label > > - fixed ident > > > > .../bindings/ata/cirrus,ep9312-pata.yaml | 44 > > +++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml > > > > diff --git a/Documentation/devicetree/bindings/ata/cirrus,ep9312- > > pata.yaml b/Documentation/devicetree/bindings/ata/cirrus,ep9312- > > pata.yaml > > new file mode 100644 > > index 000000000000..3489be55a6fe > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml > > @@ -0,0 +1,44 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/ata/cirrus,ep9312-pata.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Cirrus Logic EP9312 PATA controller > > + > > +maintainers: > > + - Damien Le Moal <dlemoal@kernel.org> > > + > > +properties: > > + compatible: > > + oneOf: > > + - const: cirrus,ep9312-pata > > I am not a DT specialist, but isn't this line superfluous since it is > listed in > the items ? > > > + - items: > > + - const: cirrus,ep9315-pata > > + - const: cirrus,ep9312-pata > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + ide@800a0000 { > > + compatible = "cirrus,ep9312-pata"; > > + reg = <0x800a0000 0x38>; > > + interrupt-parent = <&vic1>; > > + interrupts = <8>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&ide_default_pins>; > > + }; > > + > > +... >
On Fri, Jun 02, 2023 at 08:57:37AM +0900, Damien Le Moal wrote: > On 6/1/23 14:45, Nikita Shubin wrote: > > Add YAML bindings for ep93xx SoC PATA. > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > > --- > > > > Notes: > > v0 -> v1: > > > > - renamed file to ep9312-pata > > Looks OK to me but given that this is both for the cirrus,ep9315-pata and > cirrus,ep9312-pata, wouldn't it be better to name the file > cirrus,ep931x-pata.yaml ? cirrus,ep9312-pata makes sense given that is the common fallback. Wildcards are okay in filenames (only) when there's not a common fallback. > > - changed email to dlemoal@kernel.org > > - dropped label > > - fixed ident > > > > .../bindings/ata/cirrus,ep9312-pata.yaml | 44 +++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml > > > > diff --git a/Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml b/Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml > > new file mode 100644 > > index 000000000000..3489be55a6fe > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/ata/cirrus,ep9312-pata.yaml > > @@ -0,0 +1,44 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/ata/cirrus,ep9312-pata.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Cirrus Logic EP9312 PATA controller > > + > > +maintainers: > > + - Damien Le Moal <dlemoal@kernel.org> > > + > > +properties: > > + compatible: > > + oneOf: > > + - const: cirrus,ep9312-pata > > I am not a DT specialist, but isn't this line superfluous since it is listed in > the items ? No, this entry is for ep9312. The next entry is for ep9315 which is compatible with ep9312 version. The cirrus,ep9315-pata is there in case a distinction (e.g. quirk/errata) needs to be made by the driver without having to change the DT. Rob > > > + - items: > > + - const: cirrus,ep9315-pata > > + - const: cirrus,ep9312-pata
Hello Krzysztof! On Thu, 2023-06-01 at 10:30 +0200, Krzysztof Kozlowski wrote: > On 01/06/2023 07:45, Nikita Shubin wrote: > > This adds a divice for Cirrus ep93xx SoC amd ts7250 board that has > > been > > device > > > my testing target for ep93xx device support. > > > > Also inluded device tree for Liebherr BK3.1 board through it's not > > a > > included > > > complete support. > > Thank you for your patch. There is something to discuss/improve. > > > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > > --- > > > > Notes: > > v0 -> v1: > > > > - add empty chosen node > > - s/dallas,rtc-m48t86/st,m48t86/ > > - changed phy_id to phy-handle > > - dropped gpio chip-label's > > - s/eth@80010000/ethernet@80010000 > > - s/use_dma/ep9301,use-dma > > - added i2s to bk3 > > > > arch/arm/boot/dts/Makefile | 1 + > > arch/arm/boot/dts/ep93xx-bk3.dts | 119 +++++++ > > arch/arm/boot/dts/ep93xx-ts7250.dts | 132 ++++++++ > > arch/arm/boot/dts/ep93xx.dtsi | 466 > > ++++++++++++++++++++++++++++ > > Split adding DTSI from adding boards. > > > 4 files changed, 718 insertions(+) > > create mode 100644 arch/arm/boot/dts/ep93xx-bk3.dts > > create mode 100644 arch/arm/boot/dts/ep93xx-ts7250.dts > > create mode 100644 arch/arm/boot/dts/ep93xx.dtsi > > > > diff --git a/arch/arm/boot/dts/Makefile > > b/arch/arm/boot/dts/Makefile > > index 59829fc90315..a68f868fffe7 100644 > > --- a/arch/arm/boot/dts/Makefile > > +++ b/arch/arm/boot/dts/Makefile > > @@ -1670,3 +1670,4 @@ dtb-$(CONFIG_ARCH_ASPEED) += \ > > aspeed-bmc-vegman-n110.dtb \ > > aspeed-bmc-vegman-rx20.dtb \ > > aspeed-bmc-vegman-sx20.dtb > > +dtb-$(CONFIG_ARCH_EP93XX) += ep93xx-ts7250.dtb > > diff --git a/arch/arm/boot/dts/ep93xx-bk3.dts > > b/arch/arm/boot/dts/ep93xx-bk3.dts > > new file mode 100644 > > index 000000000000..215587c498e6 > > --- /dev/null > > +++ b/arch/arm/boot/dts/ep93xx-bk3.dts > > @@ -0,0 +1,119 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Device Tree file for Liebherr controller BK3.1 based on Cirrus > > EP9302 SoC > > + */ > > +/dts-v1/; > > +#include "ep93xx.dtsi" > > + > > +/ { > > + model = "Liebherr controller BK3.1"; > > + compatible = "liebherr,bk3", "cirrus,ep9301"; > > + > > + chosen { > > + }; > > + > > + memory { > > + device_type = "memory"; > > + }; > > + > > + soc { > > + nand-controller@60000000 { > > Override/extend by label/phandle. > > > + compatible = "technologic,ts7200-nand"; > > + reg = <0x60000000 0x8000000>; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + > > + partitions { > > + compatible = "fixed-partitions"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + > > + partition@0 { > > + label = "System"; > > + reg = <0x00000000 > > 0x01e00000>; > > + read-only; > > + }; > > + > > + partition@1e00000 { > > + label = "Data"; > > + reg = <0x01e00000 > > 0x05f20000>; > > + }; > > + > > + partition@7d20000 { > > + label = "RedBoot"; > > + reg = <0x07d20000 > > 0x002e0000>; > > + read-only; > > + }; > > + }; > > + }; > > + > > + syscon: syscon@80930000 { > > Override/extend by label/phandle. > > > + pinctrl: pinctrl { > > + compatible = "cirrus,ep9301- > > pinctrl"; > > Why this is board specific? You are right - it's SoC specific, but currently we only have boards with ep9302 with according device trees: - ts7250 - bk3 - edb9302 We were hoping someone with other SoC's will join - like Hartley Sweeten with Vision Engraving Systems EP9307 SoM, but no one showed up. I've changed added compatible "cirrus,ep9301-pinctrl" in ep93xx.dtsi as default and removed from all other board .dts. > > > + }; > > + }; > > + > > + gpio1: gpio@80840004 { > > Override/extend by label/phandle. > > > + /* PWM */ > > + gpio-ranges = <&pinctrl 6 163 1>; > > + }; > > + }; > > +}; > > + > > +&gpio1 { > > + /* PWM */ > > + gpio-ranges = <&pinctrl 6 163 1>; > > +}; > > + > > +&gpio4 { > > + gpio-ranges = <&pinctrl 0 97 2>; > > + status = "okay"; > > +}; > > + > > +&gpio6 { > > + gpio-ranges = <&pinctrl 0 87 2>; > > + status = "okay"; > > +}; > > + > > +&gpio7 { > > + gpio-ranges = <&pinctrl 2 199 4>; > > + status = "okay"; > > +}; > > + > > +&i2c { > > + status = "okay"; > > +}; > > + > > +&spi0: spi@808a0000 { > > + cs-gpios = <&gpio5 3 0>; > > Use proper defines for flags. > > > + status = "okay"; > > What's here? Empty enabled bus? > > > > +}; > > + > > +ð0 { > > + phy-handle = <&phy0>; > > +}; > > + > > +&mdio0 { > > + phy0: ethernet-phy@1 { > > + reg = <1>; > > + device_type = "ethernet-phy"; > > + }; > > +}; > > + > > +&uart0 { > > + status = "okay"; > > +}; > > + > > +&uart1 { > > + status = "okay"; > > +}; > > + > > +&usb { > > + status = "okay"; > > +}; > > + > > +&i2s { > > Up to you, but I seriously recommend keeping all labels ordered by > name. > Avoids conflicts. > > > + pinctrl-names = "default"; > > + pinctrl-0 = <&i2s_on_ac97_pins>; > > + /delete-property/ status; > > ??? I don't understand. Why would you do this? > > ... > > > diff --git a/arch/arm/boot/dts/ep93xx.dtsi > > b/arch/arm/boot/dts/ep93xx.dtsi > > new file mode 100644 > > index 000000000000..6da556ceaf04 > > --- /dev/null > > +++ b/arch/arm/boot/dts/ep93xx.dtsi > > @@ -0,0 +1,466 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Device Tree file for Cirrus Logic systems EP93XX SoC > > + */ > > +#include <dt-bindings/gpio/gpio.h> > > +#include <dt-bindings/leds/common.h> > > +#include <dt-bindings/input/input.h> > > +#include <dt-bindings/clock/cirrus,ep93xx-clock.h> > > +/ { > > + soc { > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + compatible = "simple-bus"; > > + > > + syscon: syscon@80930000 { > > + compatible = "cirrus,ep9301-syscon", > > + "syscon", "simple- > > mfd"; > > Broken wrapping. Align these with previous ". > > > + reg = <0x80930000 0x1000>; > > + > > + ep9301-reboot { > > Just "reboot" (and fix bindings)... but why would you need it in the > first place? I think something is seriously missing in your bindings. > > > + compatible = "cirrus,ep9301- > > reboot"; > > + }; > > + > > + eclk: clock-controller { > > + #clock-cells = <1>; > > + compatible = "cirrus,ep9301-clk"; > > + status = "okay"; > > + clocks = <&xtali>; > > + }; > > + > > + pinctrl: pinctrl { > > Missing compatible. > > > + spi_default_pins: pins-spi { > > + function = "spi"; > > + groups = "ssp"; > > + }; > > + > > + ac97_default_pins: pins-ac97 { > > + function = "ac97"; > > + groups = "ac97"; > > + }; > > + > > + i2s_on_ssp_pins: pins-i2sonssp { > > + function = "i2s"; > > + groups = "i2s_on_ssp"; > > + }; > > + > > + i2s_on_ac97_pins: pins-i2sonac97 { > > + function = "i2s"; > > + groups = "i2s_on_ac97"; > > + }; > > + > > + gpio1_default_pins: pins-gpio1 { > > + function = "gpio"; > > + groups = "gpio1agrp"; > > + }; > > + > > + pwm1_default_pins: pins-pwm1 { > > + function = "pwm"; > > + groups = "pwm1"; > > + }; > > + > > + gpio2_default_pins: pins-gpio2 { > > + function = "gpio"; > > + groups = "gpio2agrp"; > > + }; > > + > > + gpio3_default_pins: pins-gpio3 { > > + function = "gpio"; > > + groups = "gpio3agrp"; > > + }; > > + > > + keypad_default_pins: pins-keypad { > > + function = "keypad"; > > + groups = "keypadgrp"; > > + }; > > + > > + gpio4_default_pins: pins-gpio4 { > > + function = "gpio"; > > + groups = "gpio4agrp"; > > + }; > > + > > + gpio6_default_pins: pins-gpio6 { > > + function = "gpio"; > > + groups = "gpio6agrp"; > > + }; > > + > > + gpio7_default_pins: pins-gpio7 { > > + function = "gpio"; > > + groups = "gpio7agrp"; > > + }; > > + > > + ide_default_pins: pins-ide { > > + function = "pata"; > > + groups = "idegrp"; > > + }; > > + > > + lcd_on_dram0_pins: pins- > > rasteronsdram0 { > > + function = "lcd"; > > + groups = > > "rasteronsdram0grp"; > > + }; > > + > > + lcd_on_dram3_pins: pins- > > rasteronsdram3 { > > + function = "lcd"; > > + groups = > > "rasteronsdram3grp"; > > I would expect somewhere two groups since you explicitly allow it. > > > + }; > > + }; > > + }; > > + > > + vic0: interrupt-controller@800b0000 { > > + compatible = "arm,pl192-vic"; > > + interrupt-controller; > > + reg = <0x800b0000 0x1000>; > > compatible first, reg is second. ranges if present - third. Fix it > everywhere. > > > + #interrupt-cells = <1>; > > + valid-mask = <0x7ffffffc>; > > + valid-wakeup-mask = <0x0>; > > + }; > > + > > + vic1: interrupt-controller@800c0000 { > > + compatible = "arm,pl192-vic"; > > + interrupt-controller; > > + reg = <0x800c0000 0x1000>; > > + #interrupt-cells = <1>; > > + valid-mask = <0x1fffffff>; > > + valid-wakeup-mask = <0x0>; > > + }; > > + > > + timer: timer@80810000 { > > + compatible = "cirrus,ep9301-timer"; > > + reg = <0x80810000 0x100>; > > + interrupt-parent = <&vic1>; > > + interrupts = <19>; > > + }; > > + > > + dma0: dma-controller@80000000 { > > + compatible = "cirrus,ep9301-dma-m2p"; > > + reg = <0x80000000 0x0040>, > > + <0x80000040 0x0040>, > > + <0x80000080 0x0040>, > > + <0x800000c0 0x0040>, > > + <0x80000240 0x0040>, > > + <0x80000200 0x0040>, > > + <0x800002c0 0x0040>, > > + <0x80000280 0x0040>, > > + <0x80000340 0x0040>, > > + <0x80000300 0x0040>; > > + clocks = <&eclk EP93XX_CLK_M2P0>, > > + <&eclk EP93XX_CLK_M2P1>, > > + <&eclk EP93XX_CLK_M2P2>, > > + <&eclk EP93XX_CLK_M2P3>, > > + <&eclk EP93XX_CLK_M2P4>, > > + <&eclk EP93XX_CLK_M2P5>, > > + <&eclk EP93XX_CLK_M2P6>, > > + <&eclk EP93XX_CLK_M2P7>, > > + <&eclk EP93XX_CLK_M2P8>, > > + <&eclk EP93XX_CLK_M2P9>; > > + clock-names = "m2p0", "m2p1", > > + "m2p2", "m2p3", > > + "m2p4", "m2p5", > > + "m2p6", "m2p7", > > + "m2p8", "m2p9"; > > + interrupt-parent = <&vic0>; > > + interrupts = <7>, <8>, <9>, <10>, <11>, > > + <12>, <13>, <14>, <15>, <16>; > > + #dma-cells = <1>; > > + }; > > + > > + dma1: dma-controller@80000100 { > > + compatible = "cirrus,ep9301-dma-m2m"; > > + reg = <0x80000100 0x0040>, > > + <0x80000140 0x0040>; > > + clocks = <&eclk EP93XX_CLK_M2M0>, > > + <&eclk EP93XX_CLK_M2M1>; > > + clock-names = "m2m0", "m2m1"; > > + interrupt-parent = <&vic0>; > > + interrupts = <17>, <18>; > > + #dma-cells = <1>; > > + }; > > + > > + i2s: i2s@80820000 { > > + compatible = "cirrus,ep9301-i2s"; > > + #sound-dai-cells = <0>; > > + reg = <0x80820000 0x100>; > > + interrupt-parent = <&vic1>; > > + interrupts = <28>; > > + clocks = <&eclk EP93XX_CLK_I2S_MCLK > > + &eclk EP93XX_CLK_I2S_SCLK > > + &eclk EP93XX_CLK_I2S_LRCLK>; > > + clock-names = "mclk", "sclk", "lrclk"; > > + status = "disabled"; > > + }; > > + > > + gpio0: gpio@80840000 { > > + compatible = "cirrus,ep9301-gpio"; > > + reg = <0x80840000 0x04>, > > + <0x80840010 0x04>, > > + <0x80840090 0x1c>; > > Messed wrapping. > > > + reg-names = "data", "dir", "intr"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-controller; > > + interrupt-parent = <&vic1>; > > + interrupts = <27>; > > + }; > > + > > + gpio1: gpio@80840004 { > > + compatible = "cirrus,ep9301-gpio"; > > + reg = <0x80840004 0x04>, > > + <0x80840014 0x04>, > > + <0x808400ac 0x1c>; > > Ditto, in other places as well. > > > + reg-names = "data", "dir", "intr"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-controller; > > + interrupt-parent = <&vic1>; > > + interrupts = <27>; > > + }; > > + > > + gpio2: gpio@80840008 { > > + compatible = "cirrus,ep9301-gpio"; > > + reg = <0x80840008 0x04>, > > + <0x80840018 0x04>; > > + reg-names = "data", "dir"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + status = "disabled"; > > Status is usually last. > > > + pinctrl-names = "default"; > > + pinctrl-0 = <&gpio2_default_pins>; > > + }; > > + > > + gpio3: gpio@8084000c { > > + compatible = "cirrus,ep9301-gpio"; > > + reg = <0x8084000c 0x04>, > > + <0x8084001c 0x04>; > > + reg-names = "data", "dir"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + status = "disabled"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&gpio3_default_pins>; > > + }; > > + > > + gpio4: gpio@80840020 { > > + compatible = "cirrus,ep9301-gpio"; > > + reg = <0x80840020 0x04>, > > + <0x80840024 0x04>; > > + reg-names = "data", "dir"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + status = "disabled"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&gpio4_default_pins>; > > + }; > > + > > + gpio5: gpio@80840030 { > > + compatible = "cirrus,ep9301-gpio"; > > + reg = <0x80840030 0x04>, > > + <0x80840034 0x04>, > > + <0x8084004c 0x1c>; > > + reg-names = "data", "dir", "intr"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-controller; > > + interrupts-extended = <&vic0 19>, <&vic0 > > 20>, > > + <&vic0 21>, <&vic0 22>, > > + <&vic1 15>, <&vic1 16>, > > + <&vic1 17>, <&vic1 18>; > > + }; > > + > > + gpio6: gpio@80840038 { > > + compatible = "cirrus,ep9301-gpio"; > > + reg = <0x80840038 0x04>, > > + <0x8084003c 0x04>; > > + reg-names = "data", "dir"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + status = "disabled"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&gpio6_default_pins>; > > + }; > > + > > + gpio7: gpio@80840040 { > > + compatible = "cirrus,ep9301-gpio"; > > + reg = <0x80840040 0x04>, > > + <0x80840044 0x04>; > > + reg-names = "data", "dir"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + status = "disabled"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&gpio7_default_pins>; > > + }; > > + > > + ide: ide@800a0000 { > > + compatible = "cirrus,ep9312-pata"; > > + reg = <0x800a0000 0x38>; > > + interrupt-parent = <&vic1>; > > + interrupts = <8>; > > + status = "disabled"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&ide_default_pins>; > > + }; > > + > > + uart0: uart@808c0000 { > > This should scream with dtbs_check. serial. > > It does not look like you tested the DTS against bindings. Please run > `make dtbs_check` (see > Documentation/devicetree/bindings/writing-schema.rst for > instructions). make dtbs_check is totally clean now: ``` make -j34 -C build-linux ARCH=arm CROSS_COMPILE=armv4t-softfloat-linux- gnueabi- dtbs_check make[1]: Entering directory '/home/maquefel/workshop/ts7250-boot- build/build-linux' UPD include/config/kernel.release DTC_CHK arch/arm/boot/dts/cirrus/ep93xx-edb9302.dtb arch/arm/boot/dts/cirrus/ep93xx-edb9302.dtb:0:0: /soc/ac97@80880000: failed to match any schema with compatible: ['cirrus,ep9301-ac97'] arch/arm/boot/dts/cirrus/ep93xx-edb9302.dtb:0:0: /soc/spi@808a0000/codec@0: failed to match any schema with compatible: ['cirrus,cs4271'] /home/maquefel/workshop/ts7250-boot-build/build- linux/arch/arm/boot/dts/cirrus/ep93xx-edb9302.dtb: sound: 'simple- audio-card,convert-sample-format' does not match any of the regexes: '^simple-audio-card,codec(@[0-9a-f]+)?$', '^simple-audio-card,cpu(@[0- 9a-f]+)?$', '^simple-audio-card,dai-link(@[0-9a-f]+)?$', '^simple- audio-card,plat(@[0-9a-f]+)?$', 'pinctrl-[0-9]+' From schema: /home/maquefel/workshop/ts7250-boot- build/linux/Documentation/devicetree/bindings/sound/simple-card.yaml make[1]: Leaving directory '/home/maquefel/workshop/ts7250-boot- build/build-linux' ``` > > > + compatible = "arm,primecell"; > > + reg = <0x808c0000 0x1000>; > > + arm,primecell-periphid = <0x00041010>; > > + clocks = <&eclk EP93XX_CLK_UART1>, <&eclk > > EP93XX_CLK_UART>; > > + clock-names = "apb:uart1", "apb_pclk"; > > + interrupt-parent = <&vic1>; > > + interrupts = <20>; > > + status = "disabled"; > > + }; > > + > > + uart1: uart@808d0000 { > > + compatible = "arm,primecell"; > > + reg = <0x808d0000 0x1000>; > > + arm,primecell-periphid = <0x00041010>; > > + clocks = <&eclk EP93XX_CLK_UART2>, <&eclk > > EP93XX_CLK_UART>; > > + clock-names = "apb:uart2", "apb_pclk"; > > + interrupt-parent = <&vic1>; > > + interrupts = <22>; > > + status = "disabled"; > > + }; > > + > > + uart2: uart@808b0000 { > > + compatible = "arm,primecell"; > > + reg = <0x808b0000 0x1000>; > > + arm,primecell-periphid = <0x00041010>; > > + clocks = <&eclk EP93XX_CLK_UART3>, <&eclk > > EP93XX_CLK_UART>; > > + clock-names = "apb:uart3", "apb_pclk"; > > + interrupt-parent = <&vic1>; > > + interrupts = <23>; > > + status = "disabled"; > > + }; > > + > > + usb0: usb@80020000 { > > + compatible = "generic-ohci"; > > + reg = <0x80020000 0x10000>; > > + interrupt-parent = <&vic1>; > > + interrupts = <24>; > > + clocks = <&eclk EP93XX_CLK_USB>; > > + status = "disabled"; > > + }; > > + > > + eth0: ethernet@80010000 { > > + compatible = "cirrus,ep9301-eth"; > > + reg = <0x80010000 0x10000>; > > + interrupt-parent = <&vic1>; > > + interrupts = <7>; > > + mdio0: mdio { > > + #address-cells = <1>; > > + #size-cells = <0>; > > Your SoC comes with mdio? If so, why is this empty? It has a builtin MDIO and only reason for this is to provide phy-id via device tree in a correct way: ``` ð0 { phy-handle = <&phy0>; }; &mdio0 { phy0: ethernet-phy@1 { reg = <1>; device_type = "ethernet-phy"; }; }; ``` There is no other purpose for this node. Thank you Krzysztof! All other issues fixed. > > > + }; > > + }; > > + > > + rtc0: rtc@80920000 { > > + compatible = "cirrus,ep9301-rtc"; > > + reg = <0x80920000 0x100>; > > + }; > > + > > + spi0: spi@808a0000 { > > + compatible = "cirrus,ep9301-spi"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x808a0000 0x18>; > > + interrupt-parent = <&vic1>; > > + interrupts = <21>; > > + clocks = <&eclk EP93XX_CLK_SPI>; > > + cs-gpios = <&gpio5 2 0>; > > defines... but why is it here in the first place? Rarely CS gpios are > part of the SoC. I have several of such questions further as well, so > this looks like you are mixing SoC and boards in one file. > > > + cirrus,ep9301-use-dma; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&spi_default_pins>; > > + status = "disabled"; > > + }; > > + > > + adc: adc@80900000 { > > + compatible = "cirrus,ep9301-adc"; > > + reg = <0x80900000 0x28>; > > + clocks = <&eclk EP93XX_CLK_ADC>; > > + interrupt-parent = <&vic0>; > > + interrupts = <30>; > > + status = "disabled"; > > + }; > > + > > + watchdog0: watchdog@80940000 { > > + compatible = "cirrus,ep9301-wdt"; > > + reg = <0x80940000 0x08>; > > + }; > > + > > + pwm0: pwm@80910000 { > > + compatible = "cirrus,ep9301-pwm"; > > + reg = <0x80910000 0x10>; > > + clocks = <&eclk EP93XX_CLK_PWM>; > > + status = "disabled"; > > + }; > > + > > + pwm1: pwm@80910020 { > > + compatible = "cirrus,ep9301-pwm"; > > + reg = <0x80910020 0x10>; > > + clocks = <&eclk EP93XX_CLK_PWM>; > > + status = "disabled"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pwm1_default_pins>; > > + }; > > + > > + keypad: keypad@800f0000 { > > + compatible = "cirrus,ep9307-keypad"; > > + reg = <0x800f0000 0x0c>; > > + interrupt-parent = <&vic0>; > > + interrupts = <29>; > > + clocks = <&eclk EP93XX_CLK_KEYPAD>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&keypad_default_pins>; > > + linux,keymap = > > + <KEY_UP>, > > + <KEY_DOWN>, > > + <KEY_VOLUMEDOWN>, > > + <KEY_HOME>, > > + <KEY_RIGHT>, > > + <KEY_LEFT>, > > + <KEY_ENTER>, > > + <KEY_VOLUMEUP>, > > + <KEY_F6>, > > + <KEY_F8>, > > + <KEY_F9>, > > + <KEY_F10>, > > + <KEY_F1>, > > + <KEY_F2>, > > + <KEY_F3>, > > + <KEY_POWER>; > > + }; > > + }; > > + > > + xtali: oscillator { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <14745600>; > > + clock-output-names = "xtali"; > > + }; > > + > > + i2c0: i2c0 { > > i2c or i2c-0 > > > + compatible = "i2c-gpio"; > > + sda-gpios = <&gpio6 1 > > (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; > > + scl-gpios = <&gpio6 0 > > (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + status = "disabled"; > > Wait, what? Why this is disabled? If this is part of the SoC, > although > hardly looks like, then it should be complete. What is missing? How > one > could design SoC with incomplete GPIO I2C controller? > > > + }; > > + > > + leds { > > + compatible = "gpio-leds"; > > I really doubt this is property of the SoC. Please double check as it > really looks wrong. > > > > + led0 { > > led-0 > > > + label = "grled"; > > + gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>; > > + linux,default-trigger = "heartbeat"; > > + function = LED_FUNCTION_HEARTBEAT; > > + }; > > + > > + led1 { > led-1 > > > + label = "rdled"; > > + gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>; > > + function = LED_FUNCTION_FAULT; > > + }; > > + }; > > +}; > > Best regards, > Krzysztof >
This series aims to convert ep93xx from platform to full device tree support. Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302. Thank you Linus and Arnd for your support, review and comments, sorry if i missed something - these series are quite big for me. Big thanks to Alexander Sverdlin for his testing, support, review, fixes and patches. Alexander Sverdlin (4): ARM: dts: ep93xx: Add ADC node ARM: dts: ep93xx: Add I2S and AC97 nodes ARM: dts: ep93xx: Add EDB9302 DT ASoC: cirrus: edb93xx: Delete driver Nikita Shubin (39): gpio: ep93xx: split device in multiple soc: Add SoC driver for Cirrus ep93xx dt-bindings: pinctrl: Add DT bindings ep93xx pinctrl pinctrl: add a Cirrus ep93xx SoC pin controller dt-bindings: timers: add DT bindings for Cirrus EP93xx clocksource: ep93xx: Add driver for Cirrus Logic EP93xx dt-bindings: rtc: add DT bindings for Cirrus EP93xx rtc: ep93xx: add DT support for Cirrus EP93xx dt-bindings: watchdog: add DT bindings for Cirrus EP93x watchdog: ep93xx: add DT support for Cirrus EP93xx dt-bindings: clock: add DT bindings for Cirrus EP93xx clk: ep93xx: add DT support for Cirrus EP93xx power: reset: Add a driver for the ep93xx reset dt-bindings: pwm: Add DT bindings ep93xx PWM pwm: ep93xx: add DT support for Cirrus EP93xx dt-bindings: spi: Add DT bindings ep93xx spi spi: ep93xx: add DT support for Cirrus EP93xx dt-bindings: net: Add DT bindings ep93xx eth net: cirrus: add DT support for Cirrus EP93xx dt-bindings: dma: Add DT bindings ep93xx dma dma: cirrus: add DT support for Cirrus EP93xx dt-bindings: mtd: add DT bindings for ts7250 nand mtd: ts72xx_nand: add platform helper dt-bindings: ata: Add DT bindings ep93xx pata pata: cirrus: add DT support for Cirrus EP93xx dt-bindings: input: Add DT bindings ep93xx keypad input: keypad: ep93xx: add DT support for Cirrus EP93xx dt-bindings: rtc: Add DT binding m48t86 rtc rtc: m48t86: add DT support for m48t86 dt-bindings: wdt: Add DT binding ts72xx wdt wdt: ts72xx: add DT support for ts72xx dt-bindings: gpio: Add DT bindings ep93xx gpio gpio: ep93xx: add DT support for gpio-ep93xx ARM: dts: add device tree for ep93xx Soc ARM: ep93xx: DT for the Cirrus ep93xx SoC platforms pwm: ep93xx: drop legacy pinctrl input: keypad: ep93xx: drop legacy pinctrl ARM: ep93xx: soc: drop defines ARM: ep93xx: delete all boardfiles .../devicetree/bindings/arm/ep93xx.yaml | 99 + .../bindings/ata/cirrus,ep93xx-pata.yaml | 40 + .../bindings/dma/cirrus,ep93xx-dma-m2m.yaml | 66 + .../bindings/dma/cirrus,ep93xx-dma-m2p.yaml | 102 + .../devicetree/bindings/gpio/gpio-ep93xx.yaml | 161 ++ .../bindings/input/cirrus,ep93xx-keypad.yaml | 123 ++ .../bindings/mtd/technologic,nand.yaml | 56 + .../bindings/net/cirrus,ep93xx_eth.yaml | 51 + .../pinctrl/cirrus,ep93xx-pinctrl.yaml | 66 + .../bindings/pwm/cirrus,ep93xx-pwm.yaml | 45 + .../bindings/rtc/cirrus,ep93xx-rtc.yaml | 32 + .../bindings/rtc/dallas,rtc-m48t86.yaml | 33 + .../devicetree/bindings/spi/spi-ep93xx.yaml | 68 + .../bindings/timer/cirrus,ep93xx-timer.yaml | 41 + .../bindings/watchdog/cirrus,ep93xx-wdt.yaml | 38 + .../watchdog/technologic,ts72xx-wdt.yaml | 39 + arch/arm/Makefile | 1 - arch/arm/boot/dts/Makefile | 1 + arch/arm/boot/dts/ep93xx-bk3.dts | 96 + arch/arm/boot/dts/ep93xx-edb9302.dts | 150 ++ arch/arm/boot/dts/ep93xx-ts7250.dts | 113 ++ arch/arm/boot/dts/ep93xx.dtsi | 466 +++++ arch/arm/mach-ep93xx/Kconfig | 20 +- arch/arm/mach-ep93xx/Makefile | 11 - arch/arm/mach-ep93xx/core.c | 1017 ---------- arch/arm/mach-ep93xx/dma.c | 114 -- arch/arm/mach-ep93xx/edb93xx.c | 344 ---- arch/arm/mach-ep93xx/ep93xx-regs.h | 38 - arch/arm/mach-ep93xx/gpio-ep93xx.h | 111 -- arch/arm/mach-ep93xx/hardware.h | 25 - arch/arm/mach-ep93xx/irqs.h | 76 - arch/arm/mach-ep93xx/platform.h | 42 - arch/arm/mach-ep93xx/soc.h | 212 -- arch/arm/mach-ep93xx/ts72xx.c | 422 ---- arch/arm/mach-ep93xx/ts72xx.h | 94 - arch/arm/mach-ep93xx/vision_ep9307.c | 311 --- drivers/ata/pata_ep93xx.c | 9 + drivers/clk/Kconfig | 8 + drivers/clk/Makefile | 1 + .../clock.c => drivers/clk/clk-ep93xx.c | 491 +++-- drivers/clocksource/Kconfig | 11 + drivers/clocksource/Makefile | 1 + .../clocksource}/timer-ep93xx.c | 143 +- drivers/dma/ep93xx_dma.c | 119 +- drivers/gpio/gpio-ep93xx.c | 329 ++-- drivers/input/keyboard/ep93xx_keypad.c | 25 +- drivers/mtd/nand/raw/Kconfig | 8 + drivers/mtd/nand/raw/Makefile | 1 + drivers/mtd/nand/raw/ts72xx_nand.c | 94 + drivers/net/ethernet/cirrus/ep93xx_eth.c | 49 +- drivers/pinctrl/Kconfig | 7 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-ep93xx.c | 1698 +++++++++++++++++ drivers/power/reset/Kconfig | 10 + drivers/power/reset/Makefile | 1 + drivers/power/reset/ep93xx-restart.c | 65 + drivers/pwm/pwm-ep93xx.c | 24 +- drivers/rtc/rtc-ep93xx.c | 8 + drivers/rtc/rtc-m48t86.c | 10 + drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/cirrus/Kconfig | 11 + drivers/soc/cirrus/Makefile | 2 + drivers/soc/cirrus/soc-ep93xx.c | 134 ++ drivers/spi/spi-ep93xx.c | 31 +- drivers/watchdog/ep93xx_wdt.c | 8 + drivers/watchdog/ts72xx_wdt.c | 8 + .../dt-bindings/clock/cirrus,ep93xx-clock.h | 53 + include/linux/platform_data/dma-ep93xx.h | 3 + include/linux/soc/cirrus/ep93xx.h | 28 +- sound/soc/cirrus/Kconfig | 9 - sound/soc/cirrus/Makefile | 4 - sound/soc/cirrus/edb93xx.c | 119 -- 73 files changed, 4796 insertions(+), 3453 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/ep93xx.yaml create mode 100644 Documentation/devicetree/bindings/ata/cirrus,ep93xx-pata.yaml create mode 100644 Documentation/devicetree/bindings/dma/cirrus,ep93xx-dma-m2m.yaml create mode 100644 Documentation/devicetree/bindings/dma/cirrus,ep93xx-dma-m2p.yaml create mode 100644 Documentation/devicetree/bindings/gpio/gpio-ep93xx.yaml create mode 100644 Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml create mode 100644 Documentation/devicetree/bindings/net/cirrus,ep93xx_eth.yaml create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml create mode 100644 Documentation/devicetree/bindings/pwm/cirrus,ep93xx-pwm.yaml create mode 100644 Documentation/devicetree/bindings/rtc/cirrus,ep93xx-rtc.yaml create mode 100644 Documentation/devicetree/bindings/rtc/dallas,rtc-m48t86.yaml create mode 100644 Documentation/devicetree/bindings/spi/spi-ep93xx.yaml create mode 100644 Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml create mode 100644 Documentation/devicetree/bindings/watchdog/cirrus,ep93xx-wdt.yaml create mode 100644 Documentation/devicetree/bindings/watchdog/technologic,ts72xx-wdt.yaml create mode 100644 arch/arm/boot/dts/ep93xx-bk3.dts create mode 100644 arch/arm/boot/dts/ep93xx-edb9302.dts create mode 100644 arch/arm/boot/dts/ep93xx-ts7250.dts create mode 100644 arch/arm/boot/dts/ep93xx.dtsi delete mode 100644 arch/arm/mach-ep93xx/Makefile delete mode 100644 arch/arm/mach-ep93xx/core.c delete mode 100644 arch/arm/mach-ep93xx/dma.c delete mode 100644 arch/arm/mach-ep93xx/edb93xx.c delete mode 100644 arch/arm/mach-ep93xx/ep93xx-regs.h delete mode 100644 arch/arm/mach-ep93xx/gpio-ep93xx.h delete mode 100644 arch/arm/mach-ep93xx/hardware.h delete mode 100644 arch/arm/mach-ep93xx/irqs.h delete mode 100644 arch/arm/mach-ep93xx/platform.h delete mode 100644 arch/arm/mach-ep93xx/soc.h delete mode 100644 arch/arm/mach-ep93xx/ts72xx.c delete mode 100644 arch/arm/mach-ep93xx/ts72xx.h delete mode 100644 arch/arm/mach-ep93xx/vision_ep9307.c rename arch/arm/mach-ep93xx/clock.c => drivers/clk/clk-ep93xx.c (60%) rename {arch/arm/mach-ep93xx => drivers/clocksource}/timer-ep93xx.c (51%) create mode 100644 drivers/mtd/nand/raw/ts72xx_nand.c create mode 100644 drivers/pinctrl/pinctrl-ep93xx.c create mode 100644 drivers/power/reset/ep93xx-restart.c create mode 100644 drivers/soc/cirrus/Kconfig create mode 100644 drivers/soc/cirrus/Makefile create mode 100644 drivers/soc/cirrus/soc-ep93xx.c create mode 100644 include/dt-bindings/clock/cirrus,ep93xx-clock.h delete mode 100644 sound/soc/cirrus/edb93xx.c