Re: [PATCH v3 1/4] dt-bindings: rockchip-sfc: Bindings for Rockchip serial flash controller

[Johan Jonker <jbx6244@gmail.com>]

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)


> +        };
> +    };
> +
> +...
>