Message-ID: <20220715092017.2ftoyzm22i4amrbt@ti.com>
Date:   Fri, 15 Jul 2022 14:50:17 +0530
From:   Pratyush Yadav <p.yadav@...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>,
        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 15/07/22 12:07AM, Michal Suchánek wrote:
> 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.
> > 
> > > > 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?

If the board has a flash chip, then DT should describe it. The it does 
not have one, then DT should not describe it.

So if DT says there isn't a flash on a specific CS when there is, then 
DT should be fixed to describe the flash, and then we can probe it. You 
both seem to be saying the same thing here, and I agree.

> 
> > Maybe I don't understand your problem. What are you trying to
> > solve? I mean this just demotes an error to an info message.
> 
> 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.

Usually for non-enumerable components you can plug in or out, you should 
use DT overlays to add the correct nodes as needed. For example, CSI 
cameras just plug into a slot on the board. So you can easily remove one 
and add another. That is why we do not put the camera node in the board 
dts, but instead apply it as an overlay from the bootloader.

> 
> 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.
> 
> > > 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?
> 
> But then it must be in the device tree?
> 
> And then people will complain that if the bootloader does not have this
> feature then the kernel prints an error message?

Then add the feature to the bootloader? Adding a node in DT for a flash 
that is not present is wrong.

> 
> > Or load an overlay if it is there?
> 
> Or maybe the kernel could just detect if the storage is present?

It can't. This is a non-enumerable bus, unlike USB or PCI. And there is 
no way you can actually detect the presence or absence of a flash 
reliably. For example, TI chips come with a flash that is capable of 
8D-8D-8D mode. If the flash is handed to the kernel in 8D-8D-8D mode, 
the read ID command will return all 0xff bytes since the kernel expacts 
communication in 1S-1S-1S mode. With your patch you will say that no 
flash memory is detected. In reality the flash is present but the kernel 
just failed to properly detect it.

> 
> It's not like we don't have an identify command.

We do, but it does not let us detect the _absence_ of a flash.

> 
> Thanks
> 
> Michal

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Powered by blists - more mailing lists