| Message ID | 20230808130250.188588-3-ada@thorsis.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Eugen Hristev |
| Headers | show |
| Series | mtd: nand: raw: atmel: R/B gpio on sam9x60 | expand |
Hi On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl <ada@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@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 Michael > + else > + nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; > } > > gpio_request_by_name_nodev(np, "cs-gpios", 0, > -- > 2.30.2 >
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@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@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 > > 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@amarulasolutions.com > __________________________________ > > Amarula Solutions BV > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > T. +31 (0)85 111 9172 > info@amarulasolutions.com > www.amarulasolutions.com
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@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@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 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@amarulasolutions.com >> __________________________________ >> >> Amarula Solutions BV >> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL >> T. +31 (0)85 111 9172 >> info@amarulasolutions.com >> www.amarulasolutions.com
Hi On Wed, Aug 23, 2023 at 8:28 AM Eugen Hristev <eugen.hristev@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@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@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? 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@amarulasolutions.com > >> __________________________________ > >> > >> Amarula Solutions BV > >> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > >> T. +31 (0)85 111 9172 > >> info@amarulasolutions.com > >> www.amarulasolutions.com >
On 8/23/23 09:54, Michael Nazzareno Trimarchi wrote: > Hi > > On Wed, Aug 23, 2023 at 8:28 AM Eugen Hristev > <eugen.hristev@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@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@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 ? 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@amarulasolutions.com >>>> __________________________________ >>>> >>>> Amarula Solutions BV >>>> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL >>>> T. +31 (0)85 111 9172 >>>> info@amarulasolutions.com >>>> www.amarulasolutions.com >> > >
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@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@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@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. 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@amarulasolutions.com > > > > > __________________________________ > > > > > > > > > > Amarula Solutions BV > > > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > > > > > T. +31 (0)85 111 9172 > > > > > info@amarulasolutions.com > > > > > www.amarulasolutions.com > > > > > > > >
Hi Il mer 20 set 2023, 08:13 Alexander Dahl <ada@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@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@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@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@amarulasolutions.com > > > > > > __________________________________ > > > > > > > > > > > > Amarula Solutions BV > > > > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > > > > > > T. +31 (0)85 111 9172 > > > > > > info@amarulasolutions.com > > > > > > www.amarulasolutions.com > > > > > > > > > > > > >
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); + else + nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; } gpio_request_by_name_nodev(np, "cs-gpios", 0,
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@thorsis.com> --- drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)