[PATCH] octeon: fix unstable eMMC on ER-8

[Brian Norris]

Hi Yu,

On Mon, Aug 01, 2022 at 01:50:36AM -0600, Yu Zhao via openwrt-devel wrote:
> This patch adds the DTS [8] from the stock firmware [2], and makes
> ER-8 use it. Interestingly, the only change in the device tree after
> this patch is the eMMC clock speed [9].

This usually means it'd be a better idea to share the DTS source
(probably as a DTSI) and only override a single property.

I haven't built a full octeon image yet nor grokked this target's
Makefile device definitions; what DTS are you replacing, anyway? Seems
very likely that you're repeating too much. At a minimum, I bet a large
chunk of this file can be replaced with just:

#include "cn71xx.dtsi"

>     # dtc -I fs -O dts /proc/device-tree/ -o ubnt_e200.dts

That's cute, but that doesn't quite produce a proper (in terms of
readability and usability) source file. Source files have things like
labels and reference/pandle syntax, instead of bare 'phandle' properties
with indexes.

A few notes to that end below, as well as other tips on how these kinds
of files are typically written. You might also consider reading through
a few other files (e.g., in the arch/mips/boot/dts/cavium-octeon
directory) to see what they look like.

> --- /dev/null
> +++ b/target/linux/octeon/files/arch/mips/boot/dts/cavium-octeon/ubnt_e200.dts
> @@ -0,0 +1,388 @@
> +/dts-v1/;
> +
> +/ {
> +	compatible = "ubnt,e200";
> +	interrupt-parent = <0x01>;

This (and any other interrupt-parent) should be a <&phandle> reference,
not a bare integer. e.g.:

	interrupt-parent = <&ciu>;

> +	model = "ubnt,e200";
> +	#address-cells = <0x02>;
> +	#size-cells = <0x02>;

#{address,size}-cells are almost always spelled out in decimal, not hex.

	#address-cells = <2>;
	#size-cells = <2>;

(Same for pretty much anything else that's not an address or length.)

> +	memory {
> +		reg = <0x00 0x00 0x00 0x10000000 0x00 0x20000000 0x00 0x70000000>;
> +		device_type = "memory";
> +	};
> +
> +	soc at 0 {
> +		compatible = "simple-bus";
> +		ranges;
> +		#address-cells = <0x02>;
> +		#size-cells = <0x02>;
> +
> +		dma-engine at 1180000000108 {
> +			interrupts = <0x00 0x3f>;
> +			compatible = "cavium,octeon-5750-bootbus-dma";
> +			reg = <0x11800 0x108 0x00 0x08>;
> +		};
> +
> +		interrupt-controller at 1070000000000 {

If you give this a proper label like all other cavium DTs do, then you
could refer to it by name. e.g.:

/ {
...
	interrupt-parent = <&ciu>;
...
		ciu: interrupt-controller at 1070000000000 {

> +			interrupt-controller;
> +			compatible = "cavium,octeon-3860-ciu";
> +			reg = <0x10700 0x00 0x00 0x7000>;
> +			#interrupt-cells = <0x02>;
> +			phandle = <0x01>;
> +			linux,phandle = <0x01>;

Drop the 'phandle' and 'linux,phandle' properties. Any time you see
these, that means the source should *really* have a label instead, and
references to that label should be made using &your_label_name.

> +		};

...

> +			interface at 1 {
> +				compatible = "cavium,octeon-3860-pip-interface";
> +				reg = <0x01>;
> +				#address-cells = <0x01>;
> +				#size-cells = <0x00>;
> +
> +				ethernet at 0 {
> +					compatible = "cavium,octeon-3860-pip-port";
> +					phy-handle = <0x09>;
> +					reg = <0x00>;

When the 'reg' is just an index (like a "port" number in this case),
it probably makes more sense in decimal.

> +					local-mac-address = [00 00 00 00 00 00];
> +				};

...

> +		mmc at 1180000002000 {
> +			interrupts = <0x01 0x13 0x00 0x3f>;
> +			compatible = "cavium,octeon-6130-mmc";
> +			reg = <0x11800 0x2000 0x00 0x100 0x11800 0x168 0x00 0x20>;
> +			#address-cells = <0x01>;
> +			#size-cells = <0x00>;
> +
> +			mmc-slot at 0 {
> +				cavium,bus-max-width = <0x08>;
> +				compatible = "cavium,octeon-6130-mmc-slot\0mmc-spi-slot";

Instead of including 'nil' characters in the midst of a string, try the
common syntax:

				compatible = "cavium,octeon-6130-mmc-slot", "mmc-spi-slot";

> +				voltage-ranges = <0xce4 0xce4>;

These are much better spelled out in decimal:

				voltage-ranges = <3300 3300>;

> +				reg = <0x00>;
> +				spi-max-frequency = <0x18cba80>;

Decimal.

> +			};
> +		};
> +


> +		bootbus at 1180000000000 {
> +			compatible = "cavium,octeon-3860-bootbus";
> +			reg = <0x11800 0x00 0x00 0x200>;
> +			ranges = <0x00 0x00 0x00 0x1f400000 0xc00000 0x01 0x00 0x10000 0x30000000 0x00 0x02 0x00 0x00 0x1d040000 0x10000 0x03 0x00 0x00 0x1d050000 0x10000 0x04 0x00 0x00 0x1d020000 0x10000 0x05 0x00 0x10000 0x40000000 0x00 0x06 0x00 0x10000 0x50000000 0x00 0x07 0x00 0x10000 0x90000000 0x00>;

I think this works better if you wrap it :) And probably give it some
reasonable formatting/alignment, so each range actually lines up.

...

> +	aliases {
> +		smi1 = "/soc at 0/mdio at 1180000001900";

That's not how aliases are normally written. You probably want
something like:

		smi = &smi1;

and earlier:

		smi1: mdio at 1180000001900 {
		...

Same for all the other aliases.

Brian