[PATCH 2/2] mtd: nand: raw: atmel: Add error handling when rb-gpios missing
Michael Nazzareno Trimarchi
michael at amarulasolutions.com
Wed Sep 20 09:45:18 CEST 2023
Hi
Il mer 20 set 2023, 08:13 Alexander Dahl <ada at thorsis.com> ha scritto:
> Hello Eugen, hello Michael,
>
> Am Wed, Aug 23, 2023 at 01:59:58PM +0300 schrieb Eugen Hristev:
> > On 8/23/23 09:54, Michael Nazzareno Trimarchi wrote:
> > > Hi
> > >
> > > On Wed, Aug 23, 2023 at 8:28 AM Eugen Hristev
> > > <eugen.hristev at collabora.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 8/8/23 18:03, Alexander Dahl wrote:
> > > > > Hello Michael,
> > > > >
> > > > > Am Tue, Aug 08, 2023 at 03:49:45PM +0200 schrieb Michael Nazzareno
> Trimarchi:
> > > > > > Hi
> > > > > >
> > > > > > On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl <ada at thorsis.com>
> wrote:
> > > > > > >
> > > > > > > Adapt behaviour to Linux kernel driver.
> > > > > > >
> > > > > > > The return value of gpio_request_by_name_nodev() was not
> checked before,
> > > > > > > and thus in case 'rb-gpios' was missing in DT, rb.type was set
> to
> > > > > > > ATMEL_NAND_GPIO_RB nevertheless, leading to output like this
> for
> > > > > > > example (on sam9x60-curiosity with the line removed from dts):
> > > > > > >
> > > > > > > NAND: Could not find valid ONFI parameter page; aborting
> > > > > > > device found, Manufacturer ID: 0xc2, Chip ID: 0xdc
> > > > > > > Macronix NAND 512MiB 3,3V 8-bit
> > > > > > > 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB
> size: 64
> > > > > > > atmel-nand-controller nand-controller: NAND scan failed:
> -22
> > > > > > > Failed to probe nand driver (err = -22)
> > > > > > > Failed to initialize NAND controller. (error -22)
> > > > > > > 0 MiB
> > > > > > >
> > > > > > > Note: not having that gpio assigned in dts is fine, the driver
> does not
> > > > > > > override nand_chip->dev_ready() then and a generic solution is
> used.
> > > > > > >
> > > > > > > Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver")
> > > > > > > Signed-off-by: Alexander Dahl <ada at thorsis.com>
> > > > > > > ---
> > > > > > > drivers/mtd/nand/raw/atmel/nand-controller.c | 11
> +++++++----
> > > > > > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c
> b/drivers/mtd/nand/raw/atmel/nand-controller.c
> > > > > > > index 2b29c8def6..8e745a5111 100644
> > > > > > > --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> > > > > > > +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> > > > > > > @@ -1600,10 +1600,13 @@ static struct atmel_nand
> *atmel_nand_create(struct atmel_nand_controller *nc,
> > > > > > > nand->cs[i].rb.type =
> ATMEL_NAND_NATIVE_RB;
> > > > > > > nand->cs[i].rb.id = val;
> > > > > > > } else {
> > > > > > > - gpio_request_by_name_nodev(np,
> "rb-gpios", 0,
> > > > > > > -
> &nand->cs[i].rb.gpio,
> > > > > > > -
> GPIOD_IS_IN);
> > > > > > > - nand->cs[i].rb.type =
> ATMEL_NAND_GPIO_RB;
> > > > > > > + ret = gpio_request_by_name_nodev(np,
> "rb-gpios", 0,
> > > > > > > +
> &nand->cs[i].rb.gpio,
> > > > > > > +
> GPIOD_IS_IN);
> > > > > > > + if (ret)
> > > > > > > + dev_err(nc->dev, "Failed to
> get R/B gpio (err = %d)\n", ret);
> > > > > >
> > > > > > Should not then an error here
> > > > >
> > > > > Different log level or no message at all?
> > > > >
> > > > > Note: Linux prints the same message with error level in that case.
> > > > >
> > > > > Greets
> > > > > Alex
> > > > >
> > > >
> > > > Since the rb-gpios is optional, we can continue probing without it.
> > > > Throwing an error message is optional and pure informative. So I am
> fine
> > > > with it
> > > >
> > >
> > > Yes ok, but I'm not sure linux give an error if the gpio is get as
> > > optional and condition
> > > is IS_ERR. Am I right?
> >
> >
> > if (IS_ERR(gpio) && PTR_ERR(gpio) != -ENOENT) {
> > dev_err(nc->dev,
> > "Failed to get R/B gpio (err =
> %ld)\n",
> > PTR_ERR(gpio));
> > return ERR_CAST(gpio);
> > }
> >
> > So Linux throws the message if IS_ERR . If the property is missing
> (ENOENT)
> > it moves on.
> >
> > Can we replicate the same behavior or this behavior does not suit us in
> > U-boot ?
> >
> > Basically I think it should be :
> >
> > if (ret && ret != -ENOENT)
> > dev_err(...)
> > if (!ret)
> > rb.type = ATMEL_NAND_GPIO_RB;
> >
> > Is this what you had in mind Michael ?
>
> The discussion between you guys somehow got stuck. I still have this
> patch on my TODO list, but I'm not sure how to proceed. Please advice
> in which direction to go forward, there's still an error condition
> (described in the initial commit message) here, which needs to be
> handled, otherwise the NAND flash might not be usable.
Yes the suggestion above is what is the right path to follow.
Michael
>
> Greets
> Alex
>
> >
> > Eugen
> >
> > >
> > > For the rest is fine
> > >
> > > Michael
> > >
> > > > What I wanted to ask is what happens with nand->cs[i].rb.type , is
> it 0
> > > > by default ?
> > > >
> > > > Other than that, I can apply this patch, Michael, do you have any
> more
> > > > comments on it ?
> > > >
> > > > Thanks,
> > > > Eugen
> > > > > >
> > > > > > Michael
> > > > > >
> > > > > > > + else
> > > > > > > + nand->cs[i].rb.type =
> ATMEL_NAND_GPIO_RB;
> > > > > > > }
> > > > > > >
> > > > > > > gpio_request_by_name_nodev(np, "cs-gpios", 0,
> > > > > > > --
> > > > > > > 2.30.2
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Michael Nazzareno Trimarchi
> > > > > > Co-Founder & Chief Executive Officer
> > > > > > M. +39 347 913 2170
> > > > > > michael at amarulasolutions.com
> > > > > > __________________________________
> > > > > >
> > > > > > Amarula Solutions BV
> > > > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > > > T. +31 (0)85 111 9172
> > > > > > info at amarulasolutions.com
> > > > > > www.amarulasolutions.com
> > > >
> > >
> > >
> >
>
More information about the U-Boot
mailing list