Message-ID: <20220716114904.4d058637@slackpad.lan>
Date:   Sat, 16 Jul 2022 11:49:04 +0100
From:   Andre Przywara <andre.przywara@....com>
To:     Michal Suchánek <msuchanek@...e.de>
Cc:     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>,
        Samuel Holland <samuel@...lland.org>,
        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, 16 Jul 2022 09:54:21 +0200
Michal Suchánek <msuchanek@...e.de> wrote:

Hi,

> On Fri, Jul 15, 2022 at 01:20:06PM +0100, Andre Przywara wrote:
> > On Fri, 15 Jul 2022 00:07:44 +0200
> > Michal Such�nek <msuchanek@...e.de> wrote:
> > 
> > Hi,
> >   
> > > On Thu, Jul 14, 2022 at 11:51:56PM +0200, Michael Walle wrote:  
> > > > Am 2022-07-14 22:55, schrieb Michal Such�nek:    
> > > > > On Thu, Jul 14, 2022 at 09:41:48PM +0200, Michael Walle wrote:    
> > > > > > Hi,
> > > > > > 
> > > > > > Am 2022-07-14 21:19, schrieb Michal Suchanek:    
> > > > > > > It is normal that devices are designed with multiple types of storage,
> > > > > > > and only some types of storage are present.
> > > > > > >
> > > > > > > The kernel can handle this situation gracefully for many types of
> > > > > > > storage devices such as mmc or ata but it reports and error when spi
> > > > > > > flash is not present.
> > > > > > >
> > > > > > > Only print a notice that the storage device is missing when no response
> > > > > > > to the identify command is received.
> > > > > > >
> > > > > > > Consider reply buffers with all bits set to the same value no response.    
> > > > > > 
> > > > > > I'm not sure you can compare SPI with ATA and MMC. I'm just speaking
> > > > > > of
> > > > > > DT now, but there, for ATA and MMC you just describe the controller
> > > > > > and
> > > > > > it will auto-detect the connected storage. Whereas with SPI you
> > > > > > describe    
> > > > > 
> > > > > Why does mmc assume storage and SDIO must be descibed? Why the special
> > > > > casing?    
> > > > 
> > > > I can't follow you here. My SDIO wireless card just works in an SD
> > > > slot and doesn't have to be described.  
> > 
> > I think the difference is that MMC (so also SDIO) is a discoverable bus,
> > whereas SPI is not.
> > It's conceptually dangerous to blindly probe for SPI chips, and the kernel
> > tries to stay out of guessing games, in general, and leaves that up to
> > firmware.  
> 
> There is no guessing game involved. The SPI NOR is fully described in
> the device tree. The only missing bit of information is if it is mounted
> on this particular board. That can be brobed safely and reliably.

For this particular board: maybe. In general: no. I don't think the
kernel is the place to make those decisions. As Michael said: if the DT
explicitly says there is a SPI flash, and there isn't, it's an error.

> > > > > > both the controller and the flash. So I'd argue that your hardware
> > > > > > description is wrong if it describes a flash which is not present.    
> > > > > 
> > > > > At any rate the situation is the same - the storage may be present
> > > > > sometimes. I don't think assuming some kind of device by defualt is a
> > > > > sound practice.    
> > > > 
> > > > Where is the assumption when the DT tells you there is a flash
> > > > on a specific chip select but actually there it isn't. Shouldn't
> > > > the DT then be fixed?    
> > > 
> > > The DT says there isn't a flash on a specific chip select when there is.
> > > Shouldn't that be fixed?
> > >   
> > > > Maybe I don't understand your problem. What are you trying to
> > > > solve? I mean this just demotes an error to an info message.  
> > 
> > The particular problem at hand is that on those cheap development boards
> > SPI flash is somewhat optional. The PCB often has the footprint for it, but
> > sometimes it is not populated, because the vendor wants to save pennies.
> > 
> > In this case (OrangePi Zero) there was no SPI chip soldered on the first
> > batches, but later boards are shipped with a flash chip. The footprint is
> > on every version, and I for instance soldered a chip on an early board.
> >   
> > > Many boards provide multiple storage options - you get a PCB designed to
> > > carry different kinds of storage, some may be socketed, some can be
> > > soldered on in some production batches and not others.
> > > 
> > > The kernel can handle this for many kinds of storage but not SPI flash.
> > > 
> > > I don't see any reason why SPI flash should be a second class storage.  
> > 
> > See above, SPI is not discoverable, you need to know about the slave
> > devices.  
> 
> Which we do, from the device tree. Except the device is disabled in the
> device tree so the kernel does not probe it.

That doesn't count, status = "disable" has the same effect as the node
removed, the kernel doesn't use it. It's disabled because it's broken,
or the board catches fires when it's accessed, or the SPI flash is
secure only, and the kernel receives an SError when accessing it and
panics. We don't care exactly why, the kernel just skips it.

In this case having the node in DT and marking it as disabled was a
concession to users, to allow simple enablement, like this:
=> fdt addr $fdtcontroladdr
=> fdt set /soc/spi status "okay"
(on the U-Boot prompt)

And it's actually a hint that U-Boot can do this automatically, only we
need it the other way around then ("okay" from the beginning, switching
to "disabled" if needed).

> > > > > 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.  
> 
> For some cases this may be warranted.
> 
> However, in this case no additional device-specific knowledge beyond
> what is alrready specified in the device tree is needed.
> 
> A generic kernel can probe the device just fine.

The fact that sometimes there is a SPI flash and sometimes not, is a
pity device specific problem. The kernel does not have and does not
want to have any knowledge of this particular problem: we have the DT
to tell us exactly what devices are there. Firmware is encouraged to
tweak the DT, if needed.

> > 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.
> >   
> > > 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.  
> 
> Yes, that's a good point - even if it's decided that the kernel will not
> handle this, the device tree still needs to contain the node enabled for
> the bootloader to handle the device, anyway.

Yes, and I am happy to support that case when we send a patch to change
the DT in the kernel repo.
But I don't think we need an actual kernel patch to address this
problem.

Cheers,
Andre

P.S. I just see that status = "disabled" is in the wrong node, it
should be in the SPI slave node, as the controller and the SPI bus
itself are fine.

Powered by blists - more mailing lists