Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210517193205.691147-1-clabbe@baylibre.com> <20210517193205.691147-5-clabbe@baylibre.com>
In-Reply-To: <20210517193205.691147-5-clabbe@baylibre.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Tue, 18 May 2021 01:46:20 +0200
Message-ID: <CACRpkdY3c4uvo1zbEgNW0meF-4P8be_nmoOEQAHP5V+GXgoG=A@mail.gmail.com>
Subject: Re: [PATCH 4/5] ARM: gemini: add device tree for edimax NS2502
To:     Corentin Labbe <clabbe@baylibre.com>
Cc:     Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>,
        Rob Herring <robh+dt@kernel.org>, SoC Team <soc@kernel.org>,
        Hans Ulli Kroll <ulli.kroll@googlemail.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Corentin,

this is looking nice! Just one thing:

On Mon, May 17, 2021 at 9:32 PM Corentin Labbe <clabbe@baylibre.com> wrote:

> The edimax NS2502 is a NAS box running a SL3516 SoC.
>
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>

> +&flash {
> +       status = "okay";
> +       /* 8MB of flash */
> +       reg = <0x30000000 0x00800000>;

No pin control setting? I guess it may work but this doesn't hurt
(I think):

    pinctrl-names = "enabled", "disabled";
    pinctrl-0 = <&pflash_default_pins>;
    pinctrl-1 = <&pflash_disabled_pins>;

> +       partitions {
> +               compatible = "fixed-partitions";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               partition@0 {
> +                       label = "RedBoot";
> +                       reg = <0x00000000 0x00020000>;
> +                       read-only;
> +               };
> +               partition@20000 {
> +                       label = "kernel";
> +                       reg = <0x00020000 0x00700000>;
> +               };
> +               partition@720000 {
> +                       label = "VCTL";
> +                       reg = <0x00720000 0x00020000>;
> +                       read-only;
> +               };
> +               partition@740000 {
> +                       label = "CurConf";
> +                       reg = <0x00740000 0x000a0000>;
> +                       read-only;
> +               };
> +               partition@7e0000 {
> +                       label = "FIS";
> +                       reg = <0x007e0000 0x00010000>;
> +                       read-only;
> +               };
> +       };
> +};

Is the FIS broken since you hardcode the partitions?

Doesn't this work:

partitions {
    compatible = "redboot-fis";
    /* Eraseblock at 0x7e0000 */
    fis-index-block = <0xfc>;
};

(Needs CONFIG_MTD_REDBOOT_PARTS)

Yours,
Linus Walleij