Message-ID: <20220724182847.GJ17705@kitsune.suse.cz>
Date:   Sun, 24 Jul 2022 20:28:47 +0200
From:   Michal Suchánek <msuchanek@...e.de>
To:     Andre Przywara <andre.przywara@....com>
Cc:     Samuel Holland <samuel@...lland.org>,
        Michael Walle <michael@...le.cc>, linux-sunxi@...ts.linux.dev,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Tudor Ambarus <tudor.ambarus@...rochip.com>,
        Pratyush Yadav <p.yadav@...com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org
Subject: Re: [PATCH 1/2] mtd: spi-nor: When a flash memory is missing do not
 report an error

On Sat, Jul 16, 2022 at 11:58:49AM +0100, Andre Przywara wrote:
> On Fri, 15 Jul 2022 21:28:57 -0500
> Samuel Holland <samuel@...lland.org> wrote:
> 
> Hi,
> 
> > On 7/15/22 7:20 AM, Andre Przywara wrote:
> > >>>> However, when the board is designed for a specific kind of device which
> > >>>> is not always present, and the kernel can detect the device, it is
> > >>>> perfectly fine to describe it.
> > >>>>
> > >>>> The alternative is to not use the device at all, even when present,
> > >>>> which is kind of useless.    
> > >>>
> > >>> Or let the bootloader update your device tree and disable the device
> > >>> if it's not there?    
> > > 
> > > Yes, this is what I was suggesting already: U-Boot can do the job, because
> > > a U-Boot build is device specific, and we can take certain risks that the
> > > generic and single-image kernel wants to avoid.
> > > In this case we know that there is a SPI flash footprint, and it does no
> > > harm in trying to check on CS0. So I was thinking about introducing a
> > > U-Boot Kconfig variable to probe for and potentially disable the SPI flash
> > > DT node. We would set this variable in defconfigs of boards with optional
> > > SPI flash.  
> > 
> > To support the "does no harm" assertion: the Allwinner Boot ROM will probe for
> > NOR flash (and possibly SPI NAND) on SPI0 + CS0 if no bootable MMC device is
> > found. So the board designer must already assume that JEDEC Read ID commands
> > will be sent over that bus.
> > 
> > >> But then it must be in the device tree?  
> > > 
> > > However this indeed means that the SPI flash DT node must be in and enabled
> > > in the DT, because we (try hard to) only use original Linux DT files, and
> > > DTs must have been reviewed through the kernel ML first. The U-Boot driver
> > > relies on the DT as well, so the official kernel DT copy would need to come
> > > with that node enabled. Ideally U-Boot would disable it, if needed, and
> > > the kernel error message would never appear.  
> > 
> > I don't think this works for newer SoCs where the Boot ROM supports both NOR and
> > SPI NAND. If the board is sold with the flash chip unpopulated, the user could
> > install either kind of chip. So we cannot statically assume which binding will
> > be used. We would need to add a node with the right compatible string after
> > probing for both in the bootloader.
> 
> If a *user* decides to *change* the board, it's up to the user
> to make sure the DT matches. Overlays are the typical answer, or people
> change the DT before they build U-Boot. If someone decides to hack
> their board, they have to take care of the respective DT description
> hack as well.
> 
> This case here is about the *vendor* shipping different versions of the
> board, which I think is a different case. Technically we now would need
> two DTs: one with, one without the SPI flash node, and let the user
> decide which one to include in U-Boot at build time, depending on which
> version they have.

Technically we don't. The DT is supposed to describe hardware properties
that cannot be probed. Probing presence of the SPI NOR is perfectly
possible so it suffices to record that the SPI CS is reserved for a SPI
NOR, and as much as we don't describe the size we don't need to describe
or assume the presence either.

> What I was suggesting is a U-Boot config switch, which would only be
> enabled on those boards where we have this situation (OPi Zero):
> That avoids dangerous situations (because we know it's safe *on this
> particular board*), and avoids the hassle of shipping two firmware
> versions.

Also if we really want to make u-boot probe the device (such as in cases
when there is choice of mutiple devices) DT also supports "reserved"
state which may be useful for makring the devices to probe when the DT
is loaded together with u-boot before passing it to Linux.

Thanks

Michal

Powered by blists - more mailing lists