Re: [PATCH v1 1/3] ARM: MR26: MR32: remove bogus nand-ecc-algo property

["Rafał Miłecki" <zajec5@gmail.com>]

On 3.06.2023 16:16, Christian Lamparter wrote:
> | bcm53015-meraki-mr26.dtb: nand-controller@18028000:
> |   nand@0:nand-ecc-algo:0: 'hw' is not one of ['hamming', 'bch', 'rs']
> | From schema: Documentation/[...]/nand-controller.yaml
> | bcm53016-meraki-mr32.dtb: nand-controller@18028000:
> |   nand@0:nand-ecc-algo:0: 'hw' is not one of ['hamming', 'bch', 'rs']
> | From schema: Documentation/[...]/nand-controller.yaml
> 
> original ECC values for these old Merakis are sadly not
> provided by the vendor. It looks like Meraki just stuck
> with what Broadcom's SDK was doing... which left it up
> to their proprietary nand driver.
> 
> It's clear at least that they used the hardware's ecc
> engine, so update the device-tree file accordingly to
> specify the nand-controller as the ecc-engine.

I believe that initial state can be "setup" at hardware level. I believe
Broadcom's bootloader and their SDK driver just reads current ECC setup
(which goes down to the hardware level).

Years ago I proposed change for brcmnand to do the same (which was
apparently a bad idea):
[PATCH] mtd: brcmnand: set initial ECC params based on info from HW
https://lists.infradead.org/pipermail/linux-mtd/2016-February/065314.html

That said I think it still should be possible to determine what algo is
used and specify that in DT.


> this patch also removes the partition index numbers
> from the MR32's partition node-names and does some
> whitespace removal in order to fit the comment about
> the partition oddities into the 100 characters per
> limit.
> 
> Fixes: 935327a73553 ("ARM: dts: BCM5301X: Add DT for Meraki MR26")
> Fixes: ec88a9c344d9 ("ARM: BCM5301X: Add DT for Meraki MR32")
> Reported-by: Rafał Miłecki <zajec5@gmail.com> (via mail)
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> 
> mr32
> ---
>   arch/arm/boot/dts/bcm53015-meraki-mr26.dts | 68 +++++++++--------
>   arch/arm/boot/dts/bcm53016-meraki-mr32.dts | 88 ++++++++++++----------
>   2 files changed, 86 insertions(+), 70 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> index a2eee9a1e5a7..9ea4ffc1bb71 100644
> --- a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> +++ b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> @@ -9,7 +9,6 @@
>   /dts-v1/;
>   
>   #include "bcm4708.dtsi"
> -#include "bcm5301x-nand-cs0-bch8.dtsi"
>   #include <dt-bindings/leds/common.h>
>   
>   / {
> @@ -73,41 +72,50 @@ &gmac3 {
>   	status = "disabled";
>   };
>   
> -&nandcs {
> -	nand-ecc-algo = "hw";
> +&nand_controller {
> +	nand@0 {
> +		compatible = "brcm,nandcs";
> +		reg = <0>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
>   
> -	partitions {
> -		compatible = "fixed-partitions";
> -		#address-cells = <0x1>;
> -		#size-cells = <0x1>;
> +		nand-ecc-engine = <&nand_controller>;
> +		nand-ecc-strength = <8>;
> +		nand-ecc-step-size = <512>;

If we really can't determine ECC algo maybe we could still have sth like
bcm5301x-nand-cs0-FOO.dtsi
to match other ECC setup?

That way you probably also shouldn't need &nand_controller here.


>   
> -		partition@0 {
> -			label = "u-boot";
> -			reg = <0x0 0x200000>;
> -			read-only;
> -		};
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <0x1>;
> +			#size-cells = <0x1>;
>   
> -		partition@200000 {
> -			label = "u-boot-env";
> -			reg = <0x200000 0x200000>;
> -			/* empty */
> -		};
> +			partition@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x200000>;
> +				read-only;
> +			};
>   
> -		partition@400000 {
> -			label = "u-boot-backup";
> -			reg = <0x400000 0x200000>;
> -			/* empty */
> -		};
> +			partition@200000 {
> +				label = "u-boot-env";
> +				reg = <0x200000 0x200000>;
> +				/* empty */
> +			};
>   
> -		partition@600000 {
> -			label = "u-boot-env-backup";
> -			reg = <0x600000 0x200000>;
> -			/* empty */
> -		};
> +			partition@400000 {
> +				label = "u-boot-backup";
> +				reg = <0x400000 0x200000>;
> +				/* empty */
> +			};
>   
> -		partition@800000 {
> -			label = "ubi";
> -			reg = <0x800000 0x7780000>;
> +			partition@600000 {
> +				label = "u-boot-env-backup";
> +				reg = <0x600000 0x200000>;
> +				/* empty */
> +			};
> +
> +			partition@800000 {
> +				label = "ubi";
> +				reg = <0x800000 0x7780000>;
> +			};
>   		};
>   	};
>   };
> diff --git a/arch/arm/boot/dts/bcm53016-meraki-mr32.dts b/arch/arm/boot/dts/bcm53016-meraki-mr32.dts
> index b6a066f949ad..bca39b30ace8 100644
> --- a/arch/arm/boot/dts/bcm53016-meraki-mr32.dts
> +++ b/arch/arm/boot/dts/bcm53016-meraki-mr32.dts
> @@ -9,7 +9,6 @@
>   /dts-v1/;
>   
>   #include "bcm4708.dtsi"
> -#include "bcm5301x-nand-cs0-bch8.dtsi"
>   #include <dt-bindings/leds/common.h>
>   
>   / {
> @@ -124,49 +123,58 @@ &pwm {
>   	pinctrl-0 = <&pinmux_pwm>;
>   };
>   
> -&nandcs {
> -	nand-ecc-algo = "hw";
> -
> -	partitions {
> -		/*
> -		 * The partition autodetection does not work for this device.
> -		 * It will only detect the "nvram" partition with an incorrect size.
> -		 *	[    1.721667] 1 bcm47xxpart partitions found on MTD device brcmnand.0
> -		 *	[    1.727962] Creating 1 MTD partitions on "brcmnand.0":
> -		 *	[    1.733117] 0x000000400000-0x000008000000 : "nvram"
> -		 */
> -
> -		compatible = "fixed-partitions";
> -		#address-cells = <0x1>;
> -		#size-cells = <0x1>;
> -
> -		partition0@0 {
> -			label = "u-boot";
> -			reg = <0x0 0x100000>;
> -			read-only;
> -		};
> +&nand_controller {
> +	nand@0 {
> +		compatible = "brcm,nandcs";
> +		reg = <0>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
>   
> -		partition1@100000 {
> -			label = "bootkernel1";
> -			reg = <0x100000 0x300000>;
> -			read-only;
> -		};
> +		nand-ecc-engine = <&nand_controller>;
> +		nand-ecc-strength = <8>;
> +		nand-ecc-step-size = <512>;
> +
> +		partitions {
> +			/*
> +			 * The partition autodetection does not work for this device.
> +			 * It will only detect the "nvram" partition with an incorrect size.
> +			 *	[ 1.721667] 1 bcm47xxpart partitions found on MTD device brcmnand.0
> +			 *	[ 1.727962] Creating 1 MTD partitions on "brcmnand.0":
> +			 *	[ 1.733117] 0x000000400000-0x000008000000 : "nvram"
> +			 */
> +
> +			compatible = "fixed-partitions";
> +			#address-cells = <0x1>;
> +			#size-cells = <0x1>;
> +
> +			partition@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x100000>;
> +				read-only;
> +			};
>   
> -		partition2@400000 {
> -			label = "nvram";
> -			reg = <0x400000 0x100000>;
> -			read-only;
> -		};
> +			partition@100000 {
> +				label = "bootkernel1";
> +				reg = <0x100000 0x300000>;
> +				read-only;
> +			};
>   
> -		partition3@500000 {
> -			label = "bootkernel2";
> -			reg = <0x500000 0x300000>;
> -			read-only;
> -		};
> +			partition@400000 {
> +				label = "nvram";
> +				reg = <0x400000 0x100000>;
> +				read-only;
> +			};
>   
> -		partition4@800000 {
> -			label = "ubi";
> -			reg = <0x800000 0x7780000>;
> +			partition@500000 {
> +				label = "bootkernel2";
> +				reg = <0x500000 0x300000>;
> +				read-only;
> +			};
> +
> +			partition@800000 {
> +				label = "ubi";
> +				reg = <0x800000 0x7780000>;
> +			};
>   		};
>   	};
>   };


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel