Re: [PATCH 8/9] dt-bindings: mtd: Split ECC engine with rawnand controller

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 28/11/2022 03:06, Xiangsheng Hou wrote:
> Split MediaTek ECC engine with rawnand controller and convert to
> YAML schema.
> 
> Signed-off-by: Xiangsheng Hou <xiangsheng.hou@xxxxxxxxxxxx>
> ---
>  .../bindings/mtd/mtk,nand-ecc-engine.yaml     |  60 ++++++
>  .../devicetree/bindings/mtd/mtk-nand.txt      | 176 ------------------
>  .../devicetree/bindings/mtd/mtk-nand.yaml     |  92 +++++++++
>  3 files changed, 152 insertions(+), 176 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/mtk,nand-ecc-engine.yaml
>  delete mode 100644 Documentation/devicetree/bindings/mtd/mtk-nand.txt
>  create mode 100644 Documentation/devicetree/bindings/mtd/mtk-nand.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtk,nand-ecc-engine.yaml b/Documentation/devicetree/bindings/mtd/mtk,nand-ecc-engine.yaml
> new file mode 100644
> index 000000000000..80321157e928
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/mtk,nand-ecc-engine.yaml

Wrong vendor prefix. Isn't it mediatek?

> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/mtk,nand-ecc-engine.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek(MTK) SoCs NAND ECC engine
> +
> +maintainers:
> +  - Xiangsheng Hou <xiangsheng.hou@xxxxxxxxxxxx>
> +
> +description: |
> +  MTK NAND ECC engine can cowork with MTK raw NAND and SPI NAND controller.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt2701-ecc
> +      - mediatek,mt2712-ecc
> +      - mediatek,mt7622-ecc
> +
> +  reg:
> +    items:
> +      - description: Base physical address and size of ECC.
> +
> +  interrupts:
> +    items:
> +      - description: ECC interrupt
> +
> +  clocks:
> +    minItems: 1

Drop minItems.

> +    items:
> +      - description: clock used for the controller

Drop items/description - it is obvious, isn't it? Rather maxItems: 1,
Unless controller is not obvious in this context - about which
controller you talk about?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +unevaluatedProperties: false

Instead: additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mt2701-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    soc {
> +      #address-cells = <2>;

Mismatched iindentation. 4 spaces for DTS.

> +      #size-cells = <2>;
> +
> +      bch: ecc@1100e000 {
> +          compatible = "mediatek,mt2701-ecc";
> +          reg = <0 0x1100e000 0 0x1000>;
> +          interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_LOW>;
> +          clocks = <&pericfg CLK_PERI_NFI_ECC>;
> +      };
> +    };
> +
> diff --git a/Documentation/devicetree/bindings/mtd/mtk-nand.txt b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
> deleted file mode 100644
> index 4d3ec5e4ff8a..000000000000
> --- a/Documentation/devicetree/bindings/mtd/mtk-nand.txt
> +++ /dev/null
> @@ -1,176 +0,0 @@
> -MTK SoCs NAND FLASH controller (NFC) DT binding
> -
> -This file documents the device tree bindings for MTK SoCs NAND controllers.
> -The functional split of the controller requires two drivers to operate:
> -the nand controller interface driver and the ECC engine driver.
> -
> -The hardware description for both devices must be captured as device
> -tree nodes.
> -
> -1) NFC NAND Controller Interface (NFI):
> -=======================================
> -
> -The first part of NFC is NAND Controller Interface (NFI) HW.
> -Required NFI properties:
> -- compatible:			Should be one of
> -				"mediatek,mt2701-nfc",
> -				"mediatek,mt2712-nfc",
> -				"mediatek,mt7622-nfc".
> -- reg:				Base physical address and size of NFI.
> -- interrupts:			Interrupts of NFI.
> -- clocks:			NFI required clocks.
> -- clock-names:			NFI clocks internal name.
> -- ecc-engine:			Required ECC Engine node.
> -- #address-cells:		NAND chip index, should be 1.
> -- #size-cells:			Should be 0.
> -
> -Example:
> -
> -	nandc: nfi@1100d000 {
> -		compatible = "mediatek,mt2701-nfc";
> -		reg = <0 0x1100d000 0 0x1000>;
> -		interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_LOW>;
> -		clocks = <&pericfg CLK_PERI_NFI>,
> -			 <&pericfg CLK_PERI_NFI_PAD>;
> -		clock-names = "nfi_clk", "pad_clk";
> -		ecc-engine = <&bch>;
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -        };
> -
> -Platform related properties, should be set in {platform_name}.dts:
> -- children nodes:	NAND chips.
> -
> -Children nodes properties:
> -- reg:			Chip Select Signal, default 0.
> -			Set as reg = <0>, <1> when need 2 CS.
> -Optional:
> -- nand-on-flash-bbt:	Store BBT on NAND Flash.
> -- nand-ecc-mode:	the NAND ecc mode (check driver for supported modes)
> -- nand-ecc-step-size:	Number of data bytes covered by a single ECC step.
> -			valid values:
> -			512 and 1024 on mt2701 and mt2712.
> -			512 only on mt7622.
> -			1024 is recommended for large page NANDs.
> -- nand-ecc-strength:	Number of bits to correct per ECC step.
> -			The valid values that each controller supports:
> -			mt2701: 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28,
> -				32, 36, 40, 44, 48, 52, 56, 60.
> -			mt2712: 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28,
> -				32, 36, 40, 44, 48, 52, 56, 60, 68, 72, 80.
> -			mt7622: 4, 6, 8, 10, 12, 14, 16.
> -			The strength should be calculated as follows:
> -			E = (S - F) * 8 / B
> -			S = O / (P / Q)
> -				E :	nand-ecc-strength.
> -				S :	spare size per sector.
> -				F :	FDM size, should be in the range [1,8].
> -					It is used to store free oob data.
> -				O :	oob size.
> -				P :	page size.
> -				Q :	nand-ecc-step-size.
> -				B :	number of parity bits needed to correct
> -					1 bitflip.
> -					According to MTK NAND controller design,
> -					this number depends on max ecc step size
> -					that MTK NAND controller supports.
> -					If max ecc step size supported is 1024,
> -					then it should be always 14. And if max
> -					ecc step size is 512, then it should be
> -					always 13.
> -			If the result does not match any one of the listed
> -			choices above, please select the smaller valid value from
> -			the list.
> -			(otherwise the driver will do the adjustment at runtime)
> -- pinctrl-names:	Default NAND pin GPIO setting name.
> -- pinctrl-0:		GPIO setting node.
> -
> -Example:
> -	&pio {
> -		nand_pins_default: nanddefault {
> -			pins_dat {
> -				pinmux = <MT2701_PIN_111_MSDC0_DAT7__FUNC_NLD7>,
> -					 <MT2701_PIN_112_MSDC0_DAT6__FUNC_NLD6>,
> -					 <MT2701_PIN_114_MSDC0_DAT4__FUNC_NLD4>,
> -					 <MT2701_PIN_118_MSDC0_DAT3__FUNC_NLD3>,
> -					 <MT2701_PIN_121_MSDC0_DAT0__FUNC_NLD0>,
> -					 <MT2701_PIN_120_MSDC0_DAT1__FUNC_NLD1>,
> -					 <MT2701_PIN_113_MSDC0_DAT5__FUNC_NLD5>,
> -					 <MT2701_PIN_115_MSDC0_RSTB__FUNC_NLD8>,
> -					 <MT2701_PIN_119_MSDC0_DAT2__FUNC_NLD2>;
> -				input-enable;
> -				drive-strength = <MTK_DRIVE_8mA>;
> -				bias-pull-up;
> -			};
> -
> -			pins_we {
> -				pinmux = <MT2701_PIN_117_MSDC0_CLK__FUNC_NWEB>;
> -				drive-strength = <MTK_DRIVE_8mA>;
> -				bias-pull-up = <MTK_PUPD_SET_R1R0_10>;
> -			};
> -
> -			pins_ale {
> -				pinmux = <MT2701_PIN_116_MSDC0_CMD__FUNC_NALE>;
> -				drive-strength = <MTK_DRIVE_8mA>;
> -				bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
> -			};
> -		};
> -	};
> -
> -	&nandc {
> -		status = "okay";
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&nand_pins_default>;
> -		nand@0 {
> -			reg = <0>;
> -			nand-on-flash-bbt;
> -			nand-ecc-mode = "hw";
> -			nand-ecc-strength = <24>;
> -			nand-ecc-step-size = <1024>;
> -		};
> -	};
> -
> -NAND chip optional subnodes:
> -- Partitions, see Documentation/devicetree/bindings/mtd/partition.txt
> -
> -Example:
> -	nand@0 {
> -		partitions {
> -			compatible = "fixed-partitions";
> -			#address-cells = <1>;
> -			#size-cells = <1>;
> -
> -			preloader@0 {
> -				label = "pl";
> -				read-only;
> -				reg = <0x00000000 0x00400000>;
> -			};
> -			android@00400000 {
> -				label = "android";
> -				reg = <0x00400000 0x12c00000>;
> -			};
> -		};
> -	};
> -
> -2) ECC Engine:
> -==============
> -
> -Required BCH properties:
> -- compatible:	Should be one of
> -		"mediatek,mt2701-ecc",
> -		"mediatek,mt2712-ecc",
> -		"mediatek,mt7622-ecc".
> -- reg:		Base physical address and size of ECC.
> -- interrupts:	Interrupts of ECC.
> -- clocks:	ECC required clocks.
> -- clock-names:	ECC clocks internal name.
> -
> -Example:
> -
> -	bch: ecc@1100e000 {
> -		compatible = "mediatek,mt2701-ecc";
> -		reg = <0 0x1100e000 0 0x1000>;
> -		interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_LOW>;
> -		clocks = <&pericfg CLK_PERI_NFI_ECC>;
> -		clock-names = "nfiecc_clk";
> -	};
> diff --git a/Documentation/devicetree/bindings/mtd/mtk-nand.yaml b/Documentation/devicetree/bindings/mtd/mtk-nand.yaml
> new file mode 100644
> index 000000000000..47a1334bcddd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/mtk-nand.yaml

Filename more or less matching compatibles, so for example: mediatek,mtk-nfc

> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/mtk-nand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek(MTK) SoCs raw NAND FLASH controller (NFC) device tree bindings

Drop "device tree bindings"

> +
> +maintainers:
> +  - Xiangsheng Hou <xiangsheng.hou@xxxxxxxxxxxx>
> +
> +allOf:
> +  - $ref: "nand-controller.yaml#"

Drop quotes.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt2701-nfc
> +      - mediatek,mt2712-nfc
> +      - mediatek,mt7622-nfc
> +
> +  reg:
> +    items:
> +      - description: Base physical address and size of NFI.
> +
> +  interrupts:
> +    items:
> +      - description: NFI interrupt
> +
> +  clocks:
> +    minItems: 2

Drop

> +    items:
> +      - description: clock used for the controller
> +      - description: clock used for the pad
> +
> +  clock-names:
> +    minItems: 2

Drop

> +    items:
> +      - const: nfi_clk
> +      - const: pad_clk
> +
> +  nand-ecc-engine:
> +    description: Required ECC Engine node
> +    $ref: /schemas/types.yaml#/definitions/phandle

This does not look right. This is a property of NAND chip and original
binding was saying this as well... or was it called ecc-engine in
original bindings? But your commit msg did not explain any differences
from pure conversion.

> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0

Drop, they come from nand-controller.yaml.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - nand-ecc-engine
> +  - "#address-cells"
> +  - "#size-cells"

Drop cells here as well.

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mt2701-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      nandc: nfi@1100d000 {
> +        compatible = "mediatek,mt2701-nfc";

Messed indentation.

> +        reg = <0 0x1100d000 0 0x1000>;
> +        interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_LOW>;
> +        clocks = <&pericfg CLK_PERI_NFI>,
> +          <&pericfg CLK_PERI_NFI_PAD>;

Non aligned line.

> +        clock-names = "nfi_clk", "pad_clk";
> +        nand-ecc-engine = <&bch>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;

Incomplete example.

> +      };
> +
> +      bch: ecc@1100e000 {
> +          compatible = "mediatek,mt2701-ecc";
> +          reg = <0 0x1100e000 0 0x1000>;
> +          interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_LOW>;
> +          clocks = <&pericfg CLK_PERI_NFI_ECC>;
> +      };
> +    };

Best regards,
Krzysztof