| Message ID | 701967b0c418db333c66b48d225df60aa9d03ead.1657826188.git.msuchanek@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mtd: spi-nor: When a flash memory is missing do not report an error | expand |
Am 2022-07-16 11:38, schrieb Michal Suchánek: > On Sat, Jul 16, 2022 at 11:30:12AM +0200, Michael Walle wrote: >> Am 2022-07-16 10:20, schrieb Michal Suchánek: >> >> > > 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. >> > >> > The disagreement is about the situation when there is sometimes a flash >> > chip. >> >> No. The disagreement is what should happen if the DT says there is >> a device but there isn't. Which right now is an error and it should >> stay that way. Your hardware description says there is a flash >> but it cannot be probed, so it is an error. What about a board >> which has an actual error and the flash isn't responding? You >> trade one use case for another. > > And what if you have a SATA controller with a bad cable? > > Or a bad connection to a mmc card? Again. You don't tell the kernel via the DT that there is an MMC card nor that there is a SATA SDD. In contrast to SPI-NOR.. > Then the kernel also does not say there is an error and simply does not > see the device. > > This is normal. Not all devices that can potentially exist do exist. It > is up to the user to decide if it's an error that the device is > missing. > >> Also I've looked at the PHY subsystem and there, if a PHY is described >> in the DT but isn't there, the following error will be printed: >> dev_err(&mdio->dev, "MDIO device at address %d is missing.\n", >> addr); >> >> And that is for a bus which can even be automatically be >> probed/detected. > > If there is no use case for having a card with unpopulated PHY then it > makes sense. > > Here we do have a use case so the comparison is moot. And what use case is that? You are just demoting an error to an info. Apparently you just want to see that error go away, there is no change in functionality. -michael
On Fri, 15 Jul 2022 21:28:57 -0500 Samuel Holland <samuel@sholland.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. 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. Cheers, Andre
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@sholland.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
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 502967c76c5f..6bab540171a4 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1652,6 +1652,24 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor, return NULL; } +static const bool buffer_uniform(const u8 *buffer, size_t length) +{ + bool all0; + size_t i; + + for (all0 = true, i = 0; i < length; i++) + if (buffer[i] != 0) { + all0 = false; + break; + } + if (all0) + return true; + for (i = 0; i < length; i++) + if (buffer[i] != 0xff) + return false; + return true; +} + static const struct flash_info *spi_nor_detect(struct spi_nor *nor) { const struct flash_info *info; @@ -1666,8 +1684,11 @@ static const struct flash_info *spi_nor_detect(struct spi_nor *nor) info = spi_nor_match_id(nor, id); if (!info) { - dev_err(nor->dev, "unrecognized JEDEC id bytes: %*ph\n", - SPI_NOR_MAX_ID_LEN, id); + if (buffer_uniform(id, SPI_NOR_MAX_ID_LEN)) + dev_info(nor->dev, "No flash memory detected.\n"); + else + dev_err(nor->dev, "unrecognized JEDEC id bytes: %*ph\n", + SPI_NOR_MAX_ID_LEN, id); return ERR_PTR(-ENODEV); } return info;
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. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- drivers/mtd/spi-nor/core.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)