Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1256264 > unrolled thread
| Started by | Brian Norris <computersforpeace@gmail.com> |
|---|---|
| First post | 2015-10-26 21:50 +0100 |
| Last post | 2015-10-27 09:30 +0100 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib Brian Norris <computersforpeace@gmail.com> - 2015-10-26 21:50 +0100
Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib Roger Quadros <rogerq@ti.com> - 2015-10-27 09:10 +0100
Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-10-27 09:20 +0100
Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib Roger Quadros <rogerq@ti.com> - 2015-10-27 09:50 +0100
Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-10-27 09:30 +0100
| From | Brian Norris <computersforpeace@gmail.com> |
|---|---|
| Date | 2015-10-26 21:50 +0100 |
| Subject | Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib |
| Message-ID | <qnWSl-4rz-5@gated-at.bofh.it> |
+ others
A few comments below.
On Fri, Sep 18, 2015 at 05:53:40PM +0300, Roger Quadros wrote:
> The GPMC WAIT pin status are now available over gpiolib.
> Update the omap_dev_ready() function to use gpio instead of
> directly accessing GPMC register space.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> drivers/mtd/nand/omap2.c | 29 +++++++++++++++++-----------
> include/linux/platform_data/mtd-nand-omap2.h | 2 +-
> 2 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 228f498..d0f2620 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -12,6 +12,7 @@
> #include <linux/dmaengine.h>
> #include <linux/dma-mapping.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/module.h>
> #include <linux/interrupt.h>
> #include <linux/jiffies.h>
> @@ -184,6 +185,8 @@ struct omap_nand_info {
> /* fields specific for BCHx_HW ECC scheme */
> struct device *elm_dev;
> struct device_node *of_node;
> + /* NAND ready gpio */
> + struct gpio_desc *ready_gpiod;
> };
>
> /**
> @@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
> }
>
> /**
> - * omap_dev_ready - calls the platform specific dev_ready function
> + * omap_dev_ready - checks the NAND Ready GPIO line
> * @mtd: MTD device structure
> + *
> + * Returns true if ready and false if busy.
> */
> static int omap_dev_ready(struct mtd_info *mtd)
> {
> - unsigned int val = 0;
> struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> mtd);
>
> - val = readl(info->reg.gpmc_status);
> -
> - if ((val & 0x100) == 0x100) {
> - return 1;
> - } else {
> - return 0;
> - }
> + return gpiod_get_value(info->ready_gpiod);
> }
>
> /**
> @@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> info->reg = pdata->reg;
> info->of_node = pdata->of_node;
> info->ecc_opt = pdata->ecc_opt;
> - info->dev_ready = pdata->dev_ready;
> + if (pdata->dev_ready)
> + dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
> +
> info->xfer_type = pdata->xfer_type;
> info->devsize = pdata->devsize;
> info->elm_of_node = pdata->elm_of_node;
> @@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device *pdev)
> nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
> nand_chip->cmd_ctrl = omap_hwcontrol;
>
> + info->ready_gpiod = devm_gpiod_get_optional(&pdev->dev, "ready",
> + GPIOD_IN);
Others have been looking at using GPIOs for the ready/busy pin too. At a
minimum, we need an updated DT binding doc for this, since I see you're
adding this via device tree in a later patch (I don't see any DT binding
patch for this; but I could just be overlooking it). It'd also be great
if this support was moved to nand_dt_init() so other platforms can
benefit, but I won't require that.
Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio'
(the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't
really care, but the name should be going into a doc, so we can choose
the same one everywhere.
EDIT: looks like the discussion was partly here [1] and it seems we're
landing on "rb-gpios" in the latest version [2]. Can we stick with that?
Regards,
Brian
[0] "Previous" may be subject to debate, as both series have been going
for several revisions.
[1] http://patchwork.ozlabs.org/patch/515327/
[2] http://patchwork.ozlabs.org/patch/526819/
> + if (IS_ERR(info->ready_gpiod)) {
> + dev_err(dev, "failed to get ready gpio\n");
> + return PTR_ERR(info->ready_gpiod);
> + }
> +
> /*
> * If RDY/BSY line is connected to OMAP then use the omap ready
> * function and the generic nand_wait function which reads the status
> @@ -1822,7 +1829,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> * chip delay which is slightly more than tR (AC Timing) of the NAND
> * device and read status register until you get a failure or success
> */
> - if (info->dev_ready) {
> + if (info->ready_gpiod) {
> nand_chip->dev_ready = omap_dev_ready;
> nand_chip->chip_delay = 0;
> } else {
> diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
> index ff27e5a..19e509d 100644
> --- a/include/linux/platform_data/mtd-nand-omap2.h
> +++ b/include/linux/platform_data/mtd-nand-omap2.h
> @@ -70,7 +70,6 @@ struct omap_nand_platform_data {
> int cs;
> struct mtd_partition *parts;
> int nr_parts;
> - bool dev_ready;
> bool flash_bbt;
> enum nand_io xfer_type;
> int devsize;
> @@ -81,5 +80,6 @@ struct omap_nand_platform_data {
> /* deprecated */
> struct gpmc_nand_regs reg;
> struct device_node *of_node;
> + bool dev_ready;
> };
> #endif
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2015-10-27 09:10 +0100 |
| Message-ID | <qo7up-2NF-7@gated-at.bofh.it> |
| In reply to | #1256264 |
On 26/10/15 22:49, Brian Norris wrote:
> + others
>
> A few comments below.
>
> On Fri, Sep 18, 2015 at 05:53:40PM +0300, Roger Quadros wrote:
>> The GPMC WAIT pin status are now available over gpiolib.
>> Update the omap_dev_ready() function to use gpio instead of
>> directly accessing GPMC register space.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> drivers/mtd/nand/omap2.c | 29 +++++++++++++++++-----------
>> include/linux/platform_data/mtd-nand-omap2.h | 2 +-
>> 2 files changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index 228f498..d0f2620 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -12,6 +12,7 @@
>> #include <linux/dmaengine.h>
>> #include <linux/dma-mapping.h>
>> #include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> #include <linux/module.h>
>> #include <linux/interrupt.h>
>> #include <linux/jiffies.h>
>> @@ -184,6 +185,8 @@ struct omap_nand_info {
>> /* fields specific for BCHx_HW ECC scheme */
>> struct device *elm_dev;
>> struct device_node *of_node;
>> + /* NAND ready gpio */
>> + struct gpio_desc *ready_gpiod;
>> };
>>
>> /**
>> @@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
>> }
>>
>> /**
>> - * omap_dev_ready - calls the platform specific dev_ready function
>> + * omap_dev_ready - checks the NAND Ready GPIO line
>> * @mtd: MTD device structure
>> + *
>> + * Returns true if ready and false if busy.
>> */
>> static int omap_dev_ready(struct mtd_info *mtd)
>> {
>> - unsigned int val = 0;
>> struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>> mtd);
>>
>> - val = readl(info->reg.gpmc_status);
>> -
>> - if ((val & 0x100) == 0x100) {
>> - return 1;
>> - } else {
>> - return 0;
>> - }
>> + return gpiod_get_value(info->ready_gpiod);
>> }
>>
>> /**
>> @@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
>> info->reg = pdata->reg;
>> info->of_node = pdata->of_node;
>> info->ecc_opt = pdata->ecc_opt;
>> - info->dev_ready = pdata->dev_ready;
>> + if (pdata->dev_ready)
>> + dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
>> +
>> info->xfer_type = pdata->xfer_type;
>> info->devsize = pdata->devsize;
>> info->elm_of_node = pdata->elm_of_node;
>> @@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device *pdev)
>> nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
>> nand_chip->cmd_ctrl = omap_hwcontrol;
>>
>> + info->ready_gpiod = devm_gpiod_get_optional(&pdev->dev, "ready",
>> + GPIOD_IN);
>
> Others have been looking at using GPIOs for the ready/busy pin too. At a
> minimum, we need an updated DT binding doc for this, since I see you're
> adding this via device tree in a later patch (I don't see any DT binding
> patch for this; but I could just be overlooking it). It'd also be great
> if this support was moved to nand_dt_init() so other platforms can
> benefit, but I won't require that.
>
> Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio'
> (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't
> really care, but the name should be going into a doc, so we can choose
> the same one everywhere.
>
> EDIT: looks like the discussion was partly here [1] and it seems we're
> landing on "rb-gpios" in the latest version [2]. Can we stick with that?
Why should it be "rb-gpios" and not "rb-gpio"?
I don't think there are multiple gpios for r/b# function.
cheers,
-roger
>
> Regards,
> Brian
>
> [0] "Previous" may be subject to debate, as both series have been going
> for several revisions.
> [1] http://patchwork.ozlabs.org/patch/515327/
> [2] http://patchwork.ozlabs.org/patch/526819/
>
>> + if (IS_ERR(info->ready_gpiod)) {
>> + dev_err(dev, "failed to get ready gpio\n");
>> + return PTR_ERR(info->ready_gpiod);
>> + }
>> +
>> /*
>> * If RDY/BSY line is connected to OMAP then use the omap ready
>> * function and the generic nand_wait function which reads the status
>> @@ -1822,7 +1829,7 @@ static int omap_nand_probe(struct platform_device *pdev)
>> * chip delay which is slightly more than tR (AC Timing) of the NAND
>> * device and read status register until you get a failure or success
>> */
>> - if (info->dev_ready) {
>> + if (info->ready_gpiod) {
>> nand_chip->dev_ready = omap_dev_ready;
>> nand_chip->chip_delay = 0;
>> } else {
>> diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
>> index ff27e5a..19e509d 100644
>> --- a/include/linux/platform_data/mtd-nand-omap2.h
>> +++ b/include/linux/platform_data/mtd-nand-omap2.h
>> @@ -70,7 +70,6 @@ struct omap_nand_platform_data {
>> int cs;
>> struct mtd_partition *parts;
>> int nr_parts;
>> - bool dev_ready;
>> bool flash_bbt;
>> enum nand_io xfer_type;
>> int devsize;
>> @@ -81,5 +80,6 @@ struct omap_nand_platform_data {
>> /* deprecated */
>> struct gpmc_nand_regs reg;
>> struct device_node *of_node;
>> + bool dev_ready;
>> };
>> #endif
>> --
>> 2.1.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2015-10-27 09:20 +0100 |
| Message-ID | <qo7E7-2QQ-9@gated-at.bofh.it> |
| In reply to | #1256544 |
Hi Roger, On Tue, 27 Oct 2015 10:03:02 +0200 Roger Quadros <rogerq@ti.com> wrote: > On 26/10/15 22:49, Brian Norris wrote: > > > > Others have been looking at using GPIOs for the ready/busy pin too. At a > > minimum, we need an updated DT binding doc for this, since I see you're > > adding this via device tree in a later patch (I don't see any DT binding > > patch for this; but I could just be overlooking it). It'd also be great > > if this support was moved to nand_dt_init() so other platforms can > > benefit, but I won't require that. > > > > Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio' > > (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't > > really care, but the name should be going into a doc, so we can choose > > the same one everywhere. > > > > EDIT: looks like the discussion was partly here [1] and it seems we're > > landing on "rb-gpios" in the latest version [2]. Can we stick with that? > > Why should it be "rb-gpios" and not "rb-gpio"? > I don't think there are multiple gpios for r/b# function. Because it's supposed to be a generic binding, and some NAND chips embed several dies, thus exposing several CS and RB pins, hence the rb-gpios name. Also, as described here [1], the convention is to name your property <name>-gpios even if you only need one gpio. Best Regards, Boris [1]http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/gpio/gpio.txt#L16 -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2015-10-27 09:50 +0100 |
| Message-ID | <qo878-30x-15@gated-at.bofh.it> |
| In reply to | #1256550 |
Boris, On 27/10/15 10:12, Boris Brezillon wrote: > Hi Roger, > > On Tue, 27 Oct 2015 10:03:02 +0200 > Roger Quadros <rogerq@ti.com> wrote: > >> On 26/10/15 22:49, Brian Norris wrote: >>> >>> Others have been looking at using GPIOs for the ready/busy pin too. At a >>> minimum, we need an updated DT binding doc for this, since I see you're >>> adding this via device tree in a later patch (I don't see any DT binding >>> patch for this; but I could just be overlooking it). It'd also be great >>> if this support was moved to nand_dt_init() so other platforms can >>> benefit, but I won't require that. >>> >>> Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio' >>> (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't >>> really care, but the name should be going into a doc, so we can choose >>> the same one everywhere. >>> >>> EDIT: looks like the discussion was partly here [1] and it seems we're >>> landing on "rb-gpios" in the latest version [2]. Can we stick with that? >> >> Why should it be "rb-gpios" and not "rb-gpio"? >> I don't think there are multiple gpios for r/b# function. > > Because it's supposed to be a generic binding, and some NAND chips > embed several dies, thus exposing several CS and RB pins, hence the > rb-gpios name. > Also, as described here [1], the convention is to name your property > <name>-gpios even if you only need one gpio. Makes sense now. Thanks for the explanation. I'll update this patch to use rb-gpios and update the binding doc as well. -- cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2015-10-27 09:30 +0100 |
| Message-ID | <qo7NL-2U1-13@gated-at.bofh.it> |
| In reply to | #1256264 |
Hi Brian,
On Mon, 26 Oct 2015 13:49:00 -0700
Brian Norris <computersforpeace@gmail.com> wrote:
> + others
>
> A few comments below.
>
> On Fri, Sep 18, 2015 at 05:53:40PM +0300, Roger Quadros wrote:
> > The GPMC WAIT pin status are now available over gpiolib.
> > Update the omap_dev_ready() function to use gpio instead of
> > directly accessing GPMC register space.
> >
> > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > ---
> > drivers/mtd/nand/omap2.c | 29 +++++++++++++++++-----------
> > include/linux/platform_data/mtd-nand-omap2.h | 2 +-
> > 2 files changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index 228f498..d0f2620 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> > @@ -12,6 +12,7 @@
> > #include <linux/dmaengine.h>
> > #include <linux/dma-mapping.h>
> > #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > #include <linux/module.h>
> > #include <linux/interrupt.h>
> > #include <linux/jiffies.h>
> > @@ -184,6 +185,8 @@ struct omap_nand_info {
> > /* fields specific for BCHx_HW ECC scheme */
> > struct device *elm_dev;
> > struct device_node *of_node;
> > + /* NAND ready gpio */
> > + struct gpio_desc *ready_gpiod;
> > };
> >
> > /**
> > @@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
> > }
> >
> > /**
> > - * omap_dev_ready - calls the platform specific dev_ready function
> > + * omap_dev_ready - checks the NAND Ready GPIO line
> > * @mtd: MTD device structure
> > + *
> > + * Returns true if ready and false if busy.
> > */
> > static int omap_dev_ready(struct mtd_info *mtd)
> > {
> > - unsigned int val = 0;
> > struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> > mtd);
> >
> > - val = readl(info->reg.gpmc_status);
> > -
> > - if ((val & 0x100) == 0x100) {
> > - return 1;
> > - } else {
> > - return 0;
> > - }
> > + return gpiod_get_value(info->ready_gpiod);
> > }
> >
> > /**
> > @@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> > info->reg = pdata->reg;
> > info->of_node = pdata->of_node;
> > info->ecc_opt = pdata->ecc_opt;
> > - info->dev_ready = pdata->dev_ready;
> > + if (pdata->dev_ready)
> > + dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
> > +
> > info->xfer_type = pdata->xfer_type;
> > info->devsize = pdata->devsize;
> > info->elm_of_node = pdata->elm_of_node;
> > @@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device *pdev)
> > nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
> > nand_chip->cmd_ctrl = omap_hwcontrol;
> >
> > + info->ready_gpiod = devm_gpiod_get_optional(&pdev->dev, "ready",
> > + GPIOD_IN);
>
> Others have been looking at using GPIOs for the ready/busy pin too. At a
> minimum, we need an updated DT binding doc for this, since I see you're
> adding this via device tree in a later patch (I don't see any DT binding
> patch for this; but I could just be overlooking it). It'd also be great
> if this support was moved to nand_dt_init() so other platforms can
> benefit, but I won't require that.
Actually I started to work on a generic solution parsing the DT and
creating CS, WP and RB gpios when they are provided, but I think it's a
bit more complicated than just moving the rb-gpios parsing into
nand_dt_init().
First you'll need something to store your gpio_desc pointers, which
means you'll have to allocate a table of gpio_desc pointers (or a table
of struct embedding a gpio_desc pointer).
The other blocking point is that when nand_scan_ident() is called, the
caller is supposed to have filled the ->dev_ready() or ->waitfunc()
fields, and to choose how to implement it he may need to know
which kind of RB handler should be used (this is the case in the sunxi
driver, where the user can either use a GPIO or native R/B pin directly
connected to the controller).
All this makes me think that maybe nand_dt_init() should be called
separately or in a different helper (nand_init() ?) taking care of the
basic nand_chip initializations/allocations without interacting with
the NAND itself.
Another solution would be to add an ->init() function to nand_chip
and call it after the generic initialization has been done (but before
NAND chip detection). This way the NAND controller driver could adapt
some fields and parse controller specific properties.
What do you think?
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web