[02/31] mtd: spi-nor: Tidy up error handling / debug code

The -ENODEV error value in spi_nor_read_id() is incorrect since there
clearly is a device - it just cannot be supported. Use -ENOMEDIUM instead
which has the virtue of being less common.

Fix the return value in spi_nor_scan().

Also there are a few printf() statements which should be debug() since
they bloat the code with unused strings at present. Fix those while here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/mtd/spi/sf_probe.c     | 2 +-
 drivers/mtd/spi/spi-nor-core.c | 2 +-
 drivers/mtd/spi/spi-nor-tiny.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

Hi Simon,

On 19/07/20 9:45 pm, Simon Glass wrote:
> The -ENODEV error value in spi_nor_read_id() is incorrect since there
> clearly is a device - it just cannot be supported. 

Description 's not entirely accurate... If there is no flash on the SPI
bus, then read ID would return all 0s or 0xFFs depending on the pullups
on the board which would fail to match any flash in the spi-nor-ids table.

So its not just the case of device IDs not recognized or supported by
U-Boot, it maybe that flash is absent altogether.

Regards
Vignesh

> Use -ENOMEDIUM instead
> which has the virtue of being less common.
> 
> Fix the return value in spi_nor_scan().
> 
> Also there are a few printf() statements which should be debug() since
> they bloat the code with unused strings at present. Fix those while here.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/mtd/spi/sf_probe.c     | 2 +-
>  drivers/mtd/spi/spi-nor-core.c | 2 +-
>  drivers/mtd/spi/spi-nor-tiny.c | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
> index 475f6c31db..b959e3453a 100644
> --- a/drivers/mtd/spi/sf_probe.c
> +++ b/drivers/mtd/spi/sf_probe.c
> @@ -119,7 +119,7 @@ static int spi_flash_std_erase(struct udevice *dev, u32 offset, size_t len)
>  	struct erase_info instr;
>  
>  	if (offset % mtd->erasesize || len % mtd->erasesize) {
> -		printf("SF: Erase offset/length not multiple of erase size\n");
> +		debug("SF: Erase offset/length not multiple of erase size\n");
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index fdcd830ce4..0113e70037 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -2470,7 +2470,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  		 * designer) that this is bad.
>  		 */
>  		if (nor->flags & SNOR_F_BROKEN_RESET)
> -			printf("enabling reset hack; may not recover from unexpected reboots\n");
> +			debug("enabling reset hack; may not recover from unexpected reboots\n");
>  		set_4byte(nor, nor->info, 1);
>  	}
>  
> diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
> index 9f676c649d..fa26ea33c8 100644
> --- a/drivers/mtd/spi/spi-nor-tiny.c
> +++ b/drivers/mtd/spi/spi-nor-tiny.c
> @@ -377,7 +377,7 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>  	}
>  	dev_dbg(nor->dev, "unrecognized JEDEC id bytes: %02x, %02x, %02x\n",
>  		id[0], id[1], id[2]);
> -	return ERR_PTR(-ENODEV);
> +	return ERR_PTR(-EMEDIUMTYPE);
>  }
>  
>  static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> @@ -733,7 +733,7 @@ int spi_nor_scan(struct spi_nor *nor)
>  
>  	info = spi_nor_read_id(nor);
>  	if (IS_ERR_OR_NULL(info))
> -		return -ENOENT;
> +		return PTR_ERR(info);
>  	/* Parse the Serial Flash Discoverable Parameters table. */
>  	ret = spi_nor_init_params(nor, info, &params);
>  	if (ret)
>

Hi Vignesh,

On Mon, 20 Jul 2020 at 00:26, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> Hi Simon,
>
> On 19/07/20 9:45 pm, Simon Glass wrote:
> > The -ENODEV error value in spi_nor_read_id() is incorrect since there
> > clearly is a device - it just cannot be supported.
>
> Description 's not entirely accurate... If there is no flash on the SPI
> bus, then read ID would return all 0s or 0xFFs depending on the pullups
> on the board which would fail to match any flash in the spi-nor-ids table.
>
> So its not just the case of device IDs not recognized or supported by
> U-Boot, it maybe that flash is absent altogether.

But struct udevice * exists, so there is a device. Whether it is
connected to something is a separate issue.

The problem is that -ENODEV means that the device should not be
created or should not exist.

What happens if an invalid ID is returned? Does it unbind the device?

Regards,
Simon


>
> Regards
> Vignesh
>
> > Use -ENOMEDIUM instead
> > which has the virtue of being less common.
> >
> > Fix the return value in spi_nor_scan().
> >
> > Also there are a few printf() statements which should be debug() since
> > they bloat the code with unused strings at present. Fix those while here.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  drivers/mtd/spi/sf_probe.c     | 2 +-
> >  drivers/mtd/spi/spi-nor-core.c | 2 +-
> >  drivers/mtd/spi/spi-nor-tiny.c | 4 ++--
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
> > index 475f6c31db..b959e3453a 100644
> > --- a/drivers/mtd/spi/sf_probe.c
> > +++ b/drivers/mtd/spi/sf_probe.c
> > @@ -119,7 +119,7 @@ static int spi_flash_std_erase(struct udevice *dev, u32 offset, size_t len)
> >       struct erase_info instr;
> >
> >       if (offset % mtd->erasesize || len % mtd->erasesize) {
> > -             printf("SF: Erase offset/length not multiple of erase size\n");
> > +             debug("SF: Erase offset/length not multiple of erase size\n");
> >               return -EINVAL;
> >       }
> >
> > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> > index fdcd830ce4..0113e70037 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -2470,7 +2470,7 @@ static int spi_nor_init(struct spi_nor *nor)
> >                * designer) that this is bad.
> >                */
> >               if (nor->flags & SNOR_F_BROKEN_RESET)
> > -                     printf("enabling reset hack; may not recover from unexpected reboots\n");
> > +                     debug("enabling reset hack; may not recover from unexpected reboots\n");
> >               set_4byte(nor, nor->info, 1);
> >       }
> >
> > diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
> > index 9f676c649d..fa26ea33c8 100644
> > --- a/drivers/mtd/spi/spi-nor-tiny.c
> > +++ b/drivers/mtd/spi/spi-nor-tiny.c
> > @@ -377,7 +377,7 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> >       }
> >       dev_dbg(nor->dev, "unrecognized JEDEC id bytes: %02x, %02x, %02x\n",
> >               id[0], id[1], id[2]);
> > -     return ERR_PTR(-ENODEV);
> > +     return ERR_PTR(-EMEDIUMTYPE);
> >  }
> >
> >  static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> > @@ -733,7 +733,7 @@ int spi_nor_scan(struct spi_nor *nor)
> >
> >       info = spi_nor_read_id(nor);
> >       if (IS_ERR_OR_NULL(info))
> > -             return -ENOENT;
> > +             return PTR_ERR(info);
> >       /* Parse the Serial Flash Discoverable Parameters table. */
> >       ret = spi_nor_init_params(nor, info, &params);
> >       if (ret)
> >

Hi,

On 26/07/20 8:24 pm, Simon Glass wrote:
> Hi Vignesh,
> 
> On Mon, 20 Jul 2020 at 00:26, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 19/07/20 9:45 pm, Simon Glass wrote:
>>> The -ENODEV error value in spi_nor_read_id() is incorrect since there
>>> clearly is a device - it just cannot be supported.
>>
>> Description 's not entirely accurate... If there is no flash on the SPI
>> bus, then read ID would return all 0s or 0xFFs depending on the pullups
>> on the board which would fail to match any flash in the spi-nor-ids table.
>>
>> So its not just the case of device IDs not recognized or supported by
>> U-Boot, it maybe that flash is absent altogether.
> 
> But struct udevice * exists, so there is a device. Whether it is
> connected to something is a separate issue.
> 
> The problem is that -ENODEV means that the device should not be
> created or should not exist.
> 
> What happens if an invalid ID is returned? Does it unbind the device?

There are two cases:

1. spi flash node is present in the DT but there is no device on the
board. In this case device is not unbound from DT node.

2. There is no flash device described as child of SPI bus but user 
tries sf probe (Or some U-boot code calls spi_flash_probe_bus_cs()). In 
this case a device is created on the fly and bound to sf driver and 
probe is attempted (as part of which attempt is make to read Device ID). 
If there is no flash found, then the device is unbound and removed (see 
drivers/mtd/spi/sf-uclass.c::spi_flash_probe_bus_cs and 
drivers/spi/spi-uclass.c::spi_get_bus_and_cs).

So at least in case 2 its U-boot which is creating device and try to 
look for it which would fail if not present. IMO, -ENODEV makes sense 
here since the intention is to dynamically find the device w/o an actual 
DT node present.


[...]

Regards
Vignesh

On Sun, Jul 19, 2020 at 10:15:32AM -0600, Simon Glass wrote:

> The -ENODEV error value in spi_nor_read_id() is incorrect since there
> clearly is a device - it just cannot be supported. Use -ENOMEDIUM instead
> which has the virtue of being less common.
> 
> Fix the return value in spi_nor_scan().
> 
> Also there are a few printf() statements which should be debug() since
> they bloat the code with unused strings at present. Fix those while here.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!