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

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

[Christian Lamparter]

| 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.

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>;
 
-		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>;
+			};
 		};
 	};
 };
-- 
2.40.1

[PATCH v1 2/3] ARM: MR26: fix dt schema violations

[Christian Lamparter]

fixes the "duplex-full" typo, adds phy-modes for the internal
switch and the PHY-chip. This also includs adding pause support
for the internal cpu port. Furthermore, both erronous unit properties
in the gpio-keys node are removed (#size-cells, #address-cells don't
belong here).

| ports:port@5:fixed-link: 'oneOf' conditional failed, one must be fixed:
|   'anyOf' conditional failed, one must be fixed:
|   {'speed': [[1000]], 'duplex-full': True} is not of type 'array'
|   'duplex-full' does not match any of the regexes
| ports:port@5: 'phy-mode' is a required property
| keys: '#address-cells', '#size-cells' do not match any of the regexes:
| [...] From schema: gpio-keys.yaml

Fixes: 935327a73553 ("ARM: dts: BCM5301X: Add DT for Meraki MR26")
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 arch/arm/boot/dts/bcm53015-meraki-mr26.dts | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
index 9ea4ffc1bb71..9acadf393dd9 100644
--- a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
+++ b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
@@ -38,8 +38,6 @@ led-1 {
 
 	keys {
 		compatible = "gpio-keys";
-		#address-cells = <1>;
-		#size-cells = <0>;
 
 		key-restart {
 			label = "Reset";
@@ -127,16 +125,19 @@ ports {
 		port@0 {
 			reg = <0>;
 			label = "poe";
+			phy-mode = "rgmii";
 		};
 
 		port@5 {
 			reg = <5>;
 			label = "cpu";
 			ethernet = <&gmac0>;
+			phy-mode = "internal";
 
 			fixed-link {
 				speed = <1000>;
-				duplex-full;
+				full-duplex;
+				pause;
 			};
 		};
 	};
-- 
2.40.1

Re: [PATCH v1 2/3] ARM: MR26: fix dt schema violations

[Rafał Miłecki]

Hi,

I've include "BCM5301X: " in the subject line.

See below too.

On 3.06.2023 16:16, Christian Lamparter wrote:
> fixes the "duplex-full" typo, adds phy-modes for the internal
> switch and the PHY-chip. This also includs adding pause support
> for the internal cpu port. Furthermore, both erronous unit properties
> in the gpio-keys node are removed (#size-cells, #address-cells don't
> belong here).
> 
> | ports:port@5:fixed-link: 'oneOf' conditional failed, one must be fixed:
> |   'anyOf' conditional failed, one must be fixed:
> |   {'speed': [[1000]], 'duplex-full': True} is not of type 'array'
> |   'duplex-full' does not match any of the regexes
> | ports:port@5: 'phy-mode' is a required property
> | keys: '#address-cells', '#size-cells' do not match any of the regexes:
> | [...] From schema: gpio-keys.yaml
> 
> Fixes: 935327a73553 ("ARM: dts: BCM5301X: Add DT for Meraki MR26")
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>   arch/arm/boot/dts/bcm53015-meraki-mr26.dts | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> index 9ea4ffc1bb71..9acadf393dd9 100644
> --- a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> +++ b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> @@ -38,8 +38,6 @@ led-1 {
>   
>   	keys {
>   		compatible = "gpio-keys";
> -		#address-cells = <1>;
> -		#size-cells = <0>;

FWIW I've already sent patch for that:
[PATCH 2/2] ARM: dts: BCM5301X: Drop invalid properties from Meraki MR32 keys


>   		key-restart {
>   			label = "Reset";
> @@ -127,16 +125,19 @@ ports {
>   		port@0 {
>   			reg = <0>;
>   			label = "poe";
> +			phy-mode = "rgmii";
>   		};

It was never clear to me how to exactly specify "phy-mode".

It'd values are documented in the:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml

In Broadcom's bcmrobo.c we can find:
#define PAGE_GPHY_MII_P0	0x10	/* Port0 Internal GPHY MII registers page */
#define PAGE_GPHY_MII_P4	0x14	/* Last/Port4 Internal GPHY MII registers page */

That suggests ports 0, 1, 2, 3 and 4 use internal MII.

Does it make "rgmii" a valid value for that?

Could we just specify a proper value for all 5 ports in the bcm-ns.dtsi?


>   		port@5 {
>   			reg = <5>;
>   			label = "cpu";
>   			ethernet = <&gmac0>;
> +			phy-mode = "internal";
>   
>   			fixed-link {
>   				speed = <1000>;
> -				duplex-full;
> +				full-duplex;
> +				pause;
>   			};
>   		};
>   	};

Same here: could we specify "phy-mode" and "fixed-link" for ports 5, 7
and 8 in the bcm-ns.dtsi? There are more devices with warnings:

   DTC_CHK arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp2.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp2.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm4708-luxul-xap-1510.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-luxul-xap-1510.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm4708-netgear-r6250.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-netgear-r6250.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47081-luxul-xap-1410.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47081-luxul-xap-1410.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm4709-netgear-r8000.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4709-netgear-r8000.dtb: ethernet-switch@18007000: ports:port@8: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-asus-rt-ac88u.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-asus-rt-ac88u.dtb: ethernet-switch@18007000: ports:port@7: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-asus-rt-ac88u.dtb: ethernet-switch@18007000: ports:port@8: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-dlink-dir-885l.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dtb: ethernet-switch@18007000: ports:port@8: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-linksys-panamera.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-linksys-panamera.dtb: ethernet-switch@18007000: ports:port@8: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-linksys-panamera.dtb: switch@0: ports:port@8: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-luxul-abr-4500.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-abr-4500.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xap-1610.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xap-1610.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xbr-4500.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xbr-4500.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xwc-2000.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xwc-2000.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xwr-3150-v1.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xwr-3150-v1.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm53015-meraki-mr26.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm53015-meraki-mr26.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm53016-meraki-mr32.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm53016-meraki-mr32.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
   DTC_CHK arch/arm/boot/dts/bcm953012er.dtb
/home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm953012er.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
         From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml

Re: [PATCH v1 2/3] ARM: MR26: fix dt schema violations

[Christian Lamparter]

Hi,

On Mon, Jun 5, 2023 at 12:58 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> I've include "BCM5301X: " in the subject line.
>
> See below too.

Thank you.

> On 3.06.2023 16:16, Christian Lamparter wrote:
> > fixes the "duplex-full" typo, adds phy-modes for the internal
> > switch and the PHY-chip. This also includs adding pause support
> > for the internal cpu port. Furthermore, both erronous unit properties
> > in the gpio-keys node are removed (#size-cells, #address-cells don't
> > belong here).
> >
> > | ports:port@5:fixed-link: 'oneOf' conditional failed, one must be fixed:
> > |   'anyOf' conditional failed, one must be fixed:
> > |   {'speed': [[1000]], 'duplex-full': True} is not of type 'array'
> > |   'duplex-full' does not match any of the regexes
> > | ports:port@5: 'phy-mode' is a required property
> > | keys: '#address-cells', '#size-cells' do not match any of the regexes:
> > | [...] From schema: gpio-keys.yaml
> >
> > Fixes: 935327a73553 ("ARM: dts: BCM5301X: Add DT for Meraki MR26")
> > Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> > ---
> >   arch/arm/boot/dts/bcm53015-meraki-mr26.dts | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> > index 9ea4ffc1bb71..9acadf393dd9 100644
> > --- a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> > +++ b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts
> > @@ -38,8 +38,6 @@ led-1 {
> >
> >       keys {
> >               compatible = "gpio-keys";
> > -             #address-cells = <1>;
> > -             #size-cells = <0>;
>
> FWIW I've already sent patch for that:
> [PATCH 2/2] ARM: dts: BCM5301X: Drop invalid properties from Meraki MR32 keys

Ok, thanks. Was there a mail CCed to me? If it was I have to look where it went.
And yes, that hunk can definitely go.

> >               key-restart {
> >                       label = "Reset";
> > @@ -127,16 +125,19 @@ ports {
> >               port@0 {
> >                       reg = <0>;
> >                       label = "poe";
> > +                     phy-mode = "rgmii";
> >               };
>
> It was never clear to me how to exactly specify "phy-mode".
>
> It'd values are documented in the:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>
> In Broadcom's bcmrobo.c we can find:
> #define PAGE_GPHY_MII_P0        0x10    /* Port0 Internal GPHY MII registers page */
> #define PAGE_GPHY_MII_P4        0x14    /* Last/Port4 Internal GPHY MII registers page */
>
> That suggests ports 0, 1, 2, 3 and 4 use internal MII.
>
> Does it make "rgmii" a valid value for that?

From what I remember, the "RGMII" comes simply from the PHY-Chip's
datasheet itself. I "think" that it was a BCM50610.
(The POE is done by a separate TPS23754 chip). The datasheet (
B50610-DS07-R ) boast the following line
in the spec overview: "Supports only RGMII MAC interface" . So by that
logic, it can only be RGMII, since it's the only
mode the PHY-chip accepts.

But let's wait, I see if I can get phytool dump by the weekend, that
should tell me the ID. This can be matched with
the kernel's broadcom phy driver to tell which PHY-chip it is.

>
> Could we just specify a proper value for all 5 ports in the bcm-ns.dtsi?
>

puh, in a dtsi for the whole platform that can stand up to that schema
validation?
Well, I just ask myself: "What can we really set them to, if like
"nothing" is connected?
Are they internally unmuxed, pulled-down or maybe even kept floating?
don't know...".

Is it possible to skip over this question, if all the ports@X are by
default all set to ' status = "disabled" '
in the dtsi? And each device's dts has to set the individual status =
"okay" property to activate them
and then we can expect that the device's dts sets a proper phy-mode
setting? (I mean this is a
established method that all nodes should follow anyway already?)

(don't know if a per-port ' status = "disabled" ' property is
evaluated by the switch drivers/dsa though,
they might just ignore that)

> >               port@5 {
> >                       reg = <5>;
> >                       label = "cpu";
> >                       ethernet = <&gmac0>;
> > +                     phy-mode = "internal";
> >
> >                       fixed-link {
> >                               speed = <1000>;
> > -                             duplex-full;
> > +                             full-duplex;
> > +                             pause;
> >                       };
> >               };
> >       };
>
> Same here: could we specify "phy-mode" and "fixed-link" for ports 5, 7
> and 8 in the bcm-ns.dtsi? There are more devices with warnings:
>
>    DTC_CHK arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp2.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp2.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm4708-luxul-xap-1510.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-luxul-xap-1510.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm4708-netgear-r6250.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-netgear-r6250.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47081-luxul-xap-1410.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47081-luxul-xap-1410.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm4709-netgear-r8000.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm4709-netgear-r8000.dtb: ethernet-switch@18007000: ports:port@8: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-asus-rt-ac88u.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-asus-rt-ac88u.dtb: ethernet-switch@18007000: ports:port@7: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-asus-rt-ac88u.dtb: ethernet-switch@18007000: ports:port@8: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-dlink-dir-885l.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dtb: ethernet-switch@18007000: ports:port@8: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-linksys-panamera.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-linksys-panamera.dtb: ethernet-switch@18007000: ports:port@8: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-linksys-panamera.dtb: switch@0: ports:port@8: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-luxul-abr-4500.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-abr-4500.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xap-1610.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xap-1610.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xbr-4500.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xbr-4500.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xwc-2000.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xwc-2000.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm47094-luxul-xwr-3150-v1.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm47094-luxul-xwr-3150-v1.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm53015-meraki-mr26.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm53015-meraki-mr26.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm53016-meraki-mr32.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm53016-meraki-mr32.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
>    DTC_CHK arch/arm/boot/dts/bcm953012er.dtb
> /home/rmilecki/linux/linux-next/arch/arm/boot/dts/bcm953012er.dtb: ethernet-switch@18007000: ports:port@5: 'phy-mode' is a required property
>          From schema: /home/rmilecki/linux/linux-next/Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml

I've seen also drivers trying to handle that issue... Now in case of
the MR26 and MR32 this ethernet switch is a
IP-Block inside the SoC right next to the Ethernet-Core. So without
any of broadcom's datasheet about this, I
would just say that the link between the internal switches MAC (on the
"CPU"-port) to the internal
ethernet MAC is as "internal as it gets". (i.e. there's no phy there
at all, it's the one MAC talking directly to the other
MAC and vice versa.)

But for real "switches" chips this is an issue too. Recently, I read
this explanation in a commit write-up for the rtl8365mb switch:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=487994ff75880569d32504d7e70da8b3328e0693

| Since commit a18e6521a7d9 ("net: phylink: handle NA interface mode in
| phylink_fwnode_phy_connect()"), phylib defaults to GMII when no phy-mode
| or phy-connection-type property is specified in a DSA port node of the
| device tree.
|    [...]
|
| It should be noted that the aforementioned regression is not because the
| blamed commit was incorrect: on the contrary, the blamed commit is
| correcting the previous behaviour whereby unspecified phy-mode would
| cause the internal interface mode to be PHY_INTERFACE_MODE_NA. The
| rtl8365mb driver only worked by accident before because it _did_
| advertise support for PHY_INTERFACE_MODE_NA, despite NA being reserved
| for internal use by phylink. With one mistake fixed, the other was exposed.

(In a way, phylib's default (whatever they currently are) could be the
answer to that decision?)

Cheers,
Christian

[PATCH v1 3/3] ARM: MR32: fix dt schema violations

[Christian Lamparter]

fixes the "duplex-full" typo, adds phy-modes for the internal
switch and the attached PHY-chip. This also includs adding
pause support for the internal cpu port.

| ports:port@5:fixed-link: 'oneOf' conditional failed,
|  {'speed': [[1000]], 'duplex-full': True} is not of type 'array'
| 'duplex-full' does not match any of the regexes: 'pinctrl-[0-9]+'
| ports:port@5: 'phy-mode' is a required property
| ports:port@5: Unevaluated properties are not allowed

Fixes: ec88a9c344d9 ("ARM: BCM5301X: Add DT for Meraki MR32")
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 arch/arm/boot/dts/bcm53016-meraki-mr32.dts | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm53016-meraki-mr32.dts b/arch/arm/boot/dts/bcm53016-meraki-mr32.dts
index bca39b30ace8..e0ad79fac7f4 100644
--- a/arch/arm/boot/dts/bcm53016-meraki-mr32.dts
+++ b/arch/arm/boot/dts/bcm53016-meraki-mr32.dts
@@ -186,16 +186,19 @@ ports {
 		port@0 {
 			reg = <0>;
 			label = "poe";
+			phy-mode = "rgmii";
 		};
 
 		port@5 {
 			reg = <5>;
 			label = "cpu";
 			ethernet = <&gmac0>;
+			phy-mode = "internal";
 
 			fixed-link {
 				speed = <1000>;
-				duplex-full;
+				full-duplex;
+				pause;
 			};
 		};
 	};
-- 
2.40.1

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

[Rafał Miłecki]

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>;
> +			};
>   		};
>   	};
>   };

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

[Christian Lamparter]

Hi,

On Mon, Jun 5, 2023 at 1:19 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> 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.

I just remembered something possibly very important.

See, the MR26 and MR32 use both one big UBI partition to store the kernel
(blob), rootfs (squashfs), "storage" (ubifs), caldata/nvmem in individual UBI
volumes. Now, this one "weird" thing with ubi is:

| Q: Why UBI does not use OOB area of NAND flashes?
| A: Because many flashes (e.g., NOR) do not have OOB and UBI was
designed to be generic. Also, modern
| MLC NAND flashes use whole OOB area for the ECC checksum, so there
is no room for application data.
| But of course, things could be optimized for SLC NAND flashes if UBI
used the space available in the OOB area.
| This is not implemented, but one could probably do this.
<http://www.linux-mtd.infradead.org/faq/ubi.html#L_why_no_oob>

so the comment about "It's clear at least that they used the
hardware's ecc..." could be very wrong,
because they could just get away with not using any of the NAND-Chips
or SOCs ECC features.

The reason why nand-ecc-step-size and nand-ecc-strength are there in
the both MR32+MR26 DTS
is because it tells UBI about the supposed layout of the flash (Note:
This does not necessarily have
to exactly match the NAND-Chips real layout, since you can force it
through ubi-parameters too.).
UBI then uses this information to store two headers (EC and VID) at
specific locations based on the
supplied information. This is also in the FAQ under these topics.
<http://www.linux-mtd.infradead.org/doc/ubi.html#L_ubi_headers>
<http://www.linux-mtd.infradead.org/faq/ubi.html#L_find_min_io_size>
<http://www.linux-mtd.infradead.org/faq/ubi.html#L_force_no_subpage>

> > 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.
>
yes, this might be the way.

Regards,
Christian