| Message ID | 20210601201021.4406-2-macroalpha82@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/4] dt-bindings: rockchip-sfc: Bindings for Rockchip serial flash controller | expand |
Hi Chris, Some comments. Have a look if it's useful. === About the title: dt-bindings: spi: add Rockchip Serial Flash Controller Include a driver group name. Start with a verb. No need to mention "bindings" twice. === In order to get this patch reviewed by rob+dt you must include: devicetree@vger.kernel.org Check your review status here: https://patchwork.ozlabs.org/project/devicetree-bindings/list/ Get the other lists and maintainers with: ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback --nogit <patch1> <patch2> === Check document with: make ARCH=arm dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/rockchip-sfc.yaml make ARCH=arm dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/rockchip-sfc.yaml make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/rockchip-sfc.yaml Remove any errors before submitting. === In this serie you are introducing 4 new compatible strings, then also add 4 entries to dtsi files for the completeness. Could you give us a complete package? rockchip,px30-sfc rockchip,rk3036-sfc rockchip,rk3308-sfc rockchip,rv1108-sfc ARM: dts: rockchip: add sfc node for rv1108 Copy node from manufacturer tree in rv1108.dtsi ARM: dts: rockchip: add sfc node for rk3036 https://github.com/rockchip-linux/kernel/commit/57bfc68e70cb58bd4786a6baff6315f252a19088 arm64: dts: rockchip: add sfc node for PX30 arm64: dts: rockchip: add sfc node for rk3308 https://github.com/rockchip-linux/kernel/commit/036e7e07a41c264a076c70ebf20cfeca031799a0 Fix there commit message and clock names. === Johan On 6/1/21 10:10 PM, Chris Morgan wrote: > From: Chris Morgan <macromorgan@hotmail.com> > > Add bindings for the Rockchip serial flash controller. New device > specific parameter of rockchip,sfc-no-dma included in documentation. > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > --- > .../devicetree/bindings/spi/rockchip,sfc.yaml | 87 +++++++++++++++++++ > 1 file changed, 87 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/rockchip,sfc.yaml > > diff --git a/Documentation/devicetree/bindings/spi/rockchip,sfc.yaml b/Documentation/devicetree/bindings/spi/rockchip,sfc.yaml > new file mode 100644 > index 000000000000..d5f8edd621ae > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/rockchip,sfc.yaml > @@ -0,0 +1,87 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/spi/rockchip,sfc.yaml# rockchip-sfc.yaml The comma is only used between <manufacturer>,<SoC name>-<function> I think. Ask a maintainer for exact name. > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Rockchip Serial Flash Controller (SFC) > + > +maintainers: > + - Heiko Stuebner <heiko@sntech.de> > + - Chris Morgan <macromorgan@hotmail.com> sort no hotmail because it screws the message-ID > + > +allOf: > + - $ref: spi-controller.yaml# > + > +properties: > + compatible: > + oneOf: This format is wrong. Use fall back strings. > + - const: rockchip,px30-sfc > + - const: rockchip,rk1806-sfc > + - const: rockchip,rk1808-sfc > + - const: rockchip,rk312x-sfc remove I would advice against introducing compatible strings for boards that we don't have support for in mainline. Limit to things that we know what they look like. > + - const: rockchip,rk3308-sfc > + - const: rockchip,rv1108-sfc > + - items: > + - const: rockchip,rk3036-sfc oneOf: - const: rockchip,rk3036-sfc - items: - enum: - rockchip,px30-sfc - rockchip,rk3308-sfc - rockchip,rv1108-sfc - const: rockchip,rk3036-sfc > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + items: > + - description: Bus Clock > + - description: Module Clock swap > + > + clock-names: > + items: > + - const: ahb > + - const: sfc swap You must keep them in the same order as in your example and in the dtsi files (both mainline as manufacturer tree). Maybe use this: - const: clk - const: hclk #define HCLK_SFC 257 #define SCLK_SFC 58 COMPOSITE(SCLK_SFC, "clk_sfc", mux_gpll_cpll_p, 0, PX30_CLKSEL_CON(22), 7, 1, MFLAGS, 0, 7, DFLAGS, PX30_CLKGATE_CON(6), 7, GFLAGS), GATE(HCLK_SFC, "hclk_sfc", "hclk_mmc_nand", 0, PX30_CLKGATE_CON(6), 11, GFLAGS), === From clk-rk3036.c: COMPOSITE(SCLK_SFC, "sclk_sfc", mux_pll_src_apll_dpll_gpll_usb480m_p, 0, RK2928_CLKSEL_CON(16), 0, 2, MFLAGS, 2, 5, DFLAGS, RK2928_CLKGATE_CON(10), 5, GFLAGS), GATE(0, "hclk_sfc", "hclk_peri", CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 14, GFLAGS), The HCLK_SFC is missing and ignored in rk3036-cru.h and clk-rk3036.c Maybe this needs a fix? #define HCLK_SFC 454 See: clk: rockchip: rk3036: export the sfc clocks https://github.com/rockchip-linux/kernel/commit/8373cf713ddd32037dc86462cac2d2d3cc859a99 Maybe add more patches to your serie? ;) === > + > + power-domains: > + maxItems: 1 > + > + rockchip,sfc-no-dma: > + vendor,bool-property: remove See example vendor,bool-property at: Documentation/devicetree/bindings/example-schema.yaml > + descrption: Boolean value for disabling DMA description: > + type: boolean > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/px30-cru.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/power/px30-power.h> > + > + sfc: spi@ff3a0000 { > + compatible = "rockchip,px30-sfc","rockchip,rk3036-sfc"; > + reg = <0x0 0xff3a0000 0x0 0x4000>; > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cru SCLK_SFC>, <&cru HCLK_SFC>; > + clock-names = "sfc", "ahb"; > + pinctrl-0 = <&sfc_clk &sfc_cs &sfc_bus2>; > + pinctrl-names = "default"; > + power-domains = <&power PX30_PD_MMC_NAND>; Add include for PX30_PD_MMC_NAND. Check your example with scripts before you submit! > + #address-cells = <1>; > + #size-cells = <0>; > + > + flash@0 { This controller handles only serial flash for now. So limit that with patternProperties. Also users can't add more then 4 subnodes. That's where these documents are for, so add! > + compatible = "jedec,spi-nor"; > + reg = <0>; spi-max-frequency = <108000000>; > + spi-rx-bus-width = <2>; > + spi-tx-bus-width = <2>; The sort order is: compatible reg interrupts the rest in alphabetical order (no status in YAML examples) > + }; > + }; > + > +... >
diff --git a/Documentation/devicetree/bindings/spi/rockchip,sfc.yaml b/Documentation/devicetree/bindings/spi/rockchip,sfc.yaml new file mode 100644 index 000000000000..d5f8edd621ae --- /dev/null +++ b/Documentation/devicetree/bindings/spi/rockchip,sfc.yaml @@ -0,0 +1,87 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/spi/rockchip,sfc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Rockchip Serial Flash Controller (SFC) + +maintainers: + - Heiko Stuebner <heiko@sntech.de> + - Chris Morgan <macromorgan@hotmail.com> + +allOf: + - $ref: spi-controller.yaml# + +properties: + compatible: + oneOf: + - const: rockchip,px30-sfc + - const: rockchip,rk1806-sfc + - const: rockchip,rk1808-sfc + - const: rockchip,rk312x-sfc + - const: rockchip,rk3308-sfc + - const: rockchip,rv1108-sfc + - items: + - const: rockchip,rk3036-sfc + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + items: + - description: Bus Clock + - description: Module Clock + + clock-names: + items: + - const: ahb + - const: sfc + + power-domains: + maxItems: 1 + + rockchip,sfc-no-dma: + vendor,bool-property: + descrption: Boolean value for disabling DMA + type: boolean + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/px30-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + sfc: spi@ff3a0000 { + compatible = "rockchip,px30-sfc","rockchip,rk3036-sfc"; + reg = <0x0 0xff3a0000 0x0 0x4000>; + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cru SCLK_SFC>, <&cru HCLK_SFC>; + clock-names = "sfc", "ahb"; + pinctrl-0 = <&sfc_clk &sfc_cs &sfc_bus2>; + pinctrl-names = "default"; + power-domains = <&power PX30_PD_MMC_NAND>; + #address-cells = <1>; + #size-cells = <0>; + + flash@0 { + compatible = "jedec,spi-nor"; + spi-max-frequency = <108000000>; + reg = <0>; + spi-rx-bus-width = <2>; + spi-tx-bus-width = <2>; + }; + }; + +...