[PATCH v3 1/4] dt-bindings: rockchip-sfc: Bindings for Rockchip serial flash controller
Johan Jonker
jbx6244 at gmail.com
Wed Jun 2 01:13:35 PDT 2021
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 at 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 at 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 at 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 at sntech.de>
> + - Chris Morgan <macromorgan at 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 at 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 at 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)
> + };
> + };
> +
> +...
>
More information about the Linux-rockchip
mailing list