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

Yu Zhao yuzhao at google.com
Fri Sep 2 16:34:21 PDT 2022


Hi Brian,

On Thu, Sep 1, 2022 at 12:54 AM Brian Norris
<computersforpeace at gmail.com> wrote:
>
> 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?

Currently the kernel uses DTS passed in from the bootloader. Please
read the whole commit message :)

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

One of the reasons is that cn61xx is too different from cn71xx.

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

The second reason is that I consider this new DTS file "auto
generated". IOW, it's not supposed to be edited.

I see no need to study the DTS as long as it works. And I have
considered the legal ramifications of editing it or how it should be
licensed. The best answer I came up with is "don't do it".

Thanks.

> 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



More information about the openwrt-devel mailing list