| Message ID | 20171111212442.ucbatglvqeoiqb72@lenoch |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OMAP2+ OneNAND driver update | expand |
Hi, On 11/11/17 23:24, Ladislav Michl wrote: > DMA and R/B pin are independent on each other. Use DMA by default. Is there a R/B pin on Onenand? It looks more like it has a RDY pin and an INT pin (which is used for command completion interrupt). I think the subject and message are a bit misleading. It should be something like mtd: onenand: omap2: decouple DMA enabling from INT pin availability INT pin (gpio_irq) is not really needed for DMA but only for notification when a command that needs wait has completed. We can still have DMA even without gpio_irq available. > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org> > --- > Changes: > -v4: new patch > > drivers/mtd/onenand/omap2.c | 41 +++++++++++++++++------------------------ > 1 file changed, 17 insertions(+), 24 deletions(-) > > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c > index e4857a41760d..62e4ede918c4 100644 > --- a/drivers/mtd/onenand/omap2.c > +++ b/drivers/mtd/onenand/omap2.c > @@ -152,17 +152,13 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) > } > > reinit_completion(&c->irq_done); > - if (c->gpio_irq) { > - result = gpio_get_value(c->gpio_irq); > - if (result == -1) { > - ctrl = read_reg(c, ONENAND_REG_CTRL_STATUS); > - intr = read_reg(c, ONENAND_REG_INTERRUPT); > - wait_err("gpio error", state, ctrl, intr); > - return -EIO; > - } > - } else > - result = 0; > - if (result == 0) { > + result = gpio_get_value(c->gpio_irq); > + if (result < 0) { > + ctrl = read_reg(c, ONENAND_REG_CTRL_STATUS); > + intr = read_reg(c, ONENAND_REG_INTERRUPT); > + wait_err("gpio error", state, ctrl, intr); > + return -EIO; > + } else if (result == 0) { > int retry_cnt = 0; > retry: > if (!wait_for_completion_io_timeout(&c->irq_done, > @@ -513,13 +509,15 @@ static int omap2_onenand_probe(struct platform_device *pdev) We need to get rid if this code from probe. if (pdata->dma_channel < 0) { /* if -1, don't use DMA */ c->gpio_irq = 0; } > dev_err(&pdev->dev, "Failed to request GPIO%d for " > "OneNAND\n", c->gpio_irq); > goto err_iounmap; > - } > - gpio_direction_input(c->gpio_irq); > + } > + gpio_direction_input(c->gpio_irq); > + > + if ((r = request_irq(gpio_to_irq(c->gpio_irq), > + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, > + pdev->dev.driver->name, c)) < 0) > + goto err_release_gpio; > > - if ((r = request_irq(gpio_to_irq(c->gpio_irq), > - omap2_onenand_interrupt, IRQF_TRIGGER_RISING, > - pdev->dev.driver->name, c)) < 0) > - goto err_release_gpio; > + this->wait = omap2_onenand_wait; > } > > if (pdata->dma_channel >= 0) { > @@ -529,15 +527,11 @@ static int omap2_onenand_probe(struct platform_device *pdev) > dma_cap_set(DMA_MEMCPY, mask); > > c->dma_chan = dma_request_channel(mask, NULL, NULL); > - if (!c->dma_chan) > - dev_info(&pdev->dev, > - "failed to allocate DMA for OneNAND, " > - "using PIO instead\n"); Why get rid of the print message? Instead we could choose to error out completely if a DMA channel was provided and we couldn't get it. > } > > dev_info(&pdev->dev, "initializing on CS%d, phys base 0x%08lx, virtual " > - "base %p, freq %d MHz\n", c->gpmc_cs, c->phys_base, > - c->onenand.base, c->freq); > + "base %p, freq %d MHz, %s mode\n", c->gpmc_cs, c->phys_base, > + c->onenand.base, c->freq, c->dma_chan ? "DMA" : "PIO"); > > c->pdev = pdev; > c->mtd.priv = &c->onenand; > @@ -547,7 +541,6 @@ static int omap2_onenand_probe(struct platform_device *pdev) > > this = &c->onenand; > if (c->dma_chan) { > - this->wait = omap2_onenand_wait; > this->read_bufferram = omap2_onenand_read_bufferram; > this->write_bufferram = omap2_onenand_write_bufferram; > } >
On Wed, Nov 15, 2017 at 12:08:05PM +0200, Roger Quadros wrote: > Hi, > > On 11/11/17 23:24, Ladislav Michl wrote: > > DMA and R/B pin are independent on each other. Use DMA by default. > > Is there a R/B pin on Onenand? It looks more like it has a RDY pin and > an INT pin (which is used for command completion interrupt). (See also my previous answer) Yes, there's RDY and INT pin. I tried to use the same scheme as NAND is using, but perhaps it doesn't fit too well here. Therefore I'm proposing to keep INT pin name and use int-gpios in DT bindings. Suggestions? > I think the subject and message are a bit misleading. > > It should be something like > > mtd: onenand: omap2: decouple DMA enabling from INT pin availability > > INT pin (gpio_irq) is not really needed for DMA but only for notification > when a command that needs wait has completed. We can still have DMA > even without gpio_irq available. Thanks, that's definitely better :-) > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org> > > --- > > Changes: > > -v4: new patch > > > > drivers/mtd/onenand/omap2.c | 41 +++++++++++++++++------------------------ > > 1 file changed, 17 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c > > index e4857a41760d..62e4ede918c4 100644 > > --- a/drivers/mtd/onenand/omap2.c > > +++ b/drivers/mtd/onenand/omap2.c > > @@ -152,17 +152,13 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) > > } > > > > reinit_completion(&c->irq_done); > > - if (c->gpio_irq) { > > - result = gpio_get_value(c->gpio_irq); > > - if (result == -1) { > > - ctrl = read_reg(c, ONENAND_REG_CTRL_STATUS); > > - intr = read_reg(c, ONENAND_REG_INTERRUPT); > > - wait_err("gpio error", state, ctrl, intr); > > - return -EIO; > > - } > > - } else > > - result = 0; > > - if (result == 0) { > > + result = gpio_get_value(c->gpio_irq); > > + if (result < 0) { > > + ctrl = read_reg(c, ONENAND_REG_CTRL_STATUS); > > + intr = read_reg(c, ONENAND_REG_INTERRUPT); > > + wait_err("gpio error", state, ctrl, intr); > > + return -EIO; > > + } else if (result == 0) { > > int retry_cnt = 0; > > retry: > > if (!wait_for_completion_io_timeout(&c->irq_done, > > @@ -513,13 +509,15 @@ static int omap2_onenand_probe(struct platform_device *pdev) > > We need to get rid if this code from probe. > > if (pdata->dma_channel < 0) { > /* if -1, don't use DMA */ > c->gpio_irq = 0; > } Yes. I overlooked it when shuffling with patches. In ended in later patch, but it really belongs here. Will fix. > > dev_err(&pdev->dev, "Failed to request GPIO%d for " > > "OneNAND\n", c->gpio_irq); > > goto err_iounmap; > > - } > > - gpio_direction_input(c->gpio_irq); > > + } > > + gpio_direction_input(c->gpio_irq); > > + > > + if ((r = request_irq(gpio_to_irq(c->gpio_irq), > > + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, > > + pdev->dev.driver->name, c)) < 0) > > + goto err_release_gpio; > > > > - if ((r = request_irq(gpio_to_irq(c->gpio_irq), > > - omap2_onenand_interrupt, IRQF_TRIGGER_RISING, > > - pdev->dev.driver->name, c)) < 0) > > - goto err_release_gpio; > > + this->wait = omap2_onenand_wait; > > } > > > > if (pdata->dma_channel >= 0) { > > @@ -529,15 +527,11 @@ static int omap2_onenand_probe(struct platform_device *pdev) > > dma_cap_set(DMA_MEMCPY, mask); > > > > c->dma_chan = dma_request_channel(mask, NULL, NULL); > > - if (!c->dma_chan) > > - dev_info(&pdev->dev, > > - "failed to allocate DMA for OneNAND, " > > - "using PIO instead\n"); > > Why get rid of the print message? Instead we could choose to error out completely > if a DMA channel was provided and we couldn't get it. The point is that without pdata->dma_channel condition above, DMA is always enabled and it seems too strict to me to fail, when driver can continue to work. > > } > > > > dev_info(&pdev->dev, "initializing on CS%d, phys base 0x%08lx, virtual " > > - "base %p, freq %d MHz\n", c->gpmc_cs, c->phys_base, > > - c->onenand.base, c->freq); > > + "base %p, freq %d MHz, %s mode\n", c->gpmc_cs, c->phys_base, > > + c->onenand.base, c->freq, c->dma_chan ? "DMA" : "PIO"); See here ^. We just merely print message driver is running in PIO mode. > > c->pdev = pdev; > > c->mtd.priv = &c->onenand; > > @@ -547,7 +541,6 @@ static int omap2_onenand_probe(struct platform_device *pdev) > > > > this = &c->onenand; > > if (c->dma_chan) { > > - this->wait = omap2_onenand_wait; > > this->read_bufferram = omap2_onenand_read_bufferram; > > this->write_bufferram = omap2_onenand_write_bufferram; > > } > > > > -- > cheers, > -roger > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 15/11/17 12:32, Ladislav Michl wrote: > On Wed, Nov 15, 2017 at 12:08:05PM +0200, Roger Quadros wrote: >> Hi, >> >> On 11/11/17 23:24, Ladislav Michl wrote: >>> DMA and R/B pin are independent on each other. Use DMA by default. >> >> Is there a R/B pin on Onenand? It looks more like it has a RDY pin and >> an INT pin (which is used for command completion interrupt). > > (See also my previous answer) Yes, there's RDY and INT pin. I tried to use > the same scheme as NAND is using, but perhaps it doesn't fit too well here. > Therefore I'm proposing to keep INT pin name and use int-gpios in DT > bindings. Suggestions? > >> I think the subject and message are a bit misleading. >> >> It should be something like >> >> mtd: onenand: omap2: decouple DMA enabling from INT pin availability >> >> INT pin (gpio_irq) is not really needed for DMA but only for notification >> when a command that needs wait has completed. We can still have DMA >> even without gpio_irq available. > > Thanks, that's definitely better :-) > >>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org> >>> --- >>> Changes: >>> -v4: new patch >>> >>> drivers/mtd/onenand/omap2.c | 41 +++++++++++++++++------------------------ >>> 1 file changed, 17 insertions(+), 24 deletions(-) >>> >>> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c >>> index e4857a41760d..62e4ede918c4 100644 >>> --- a/drivers/mtd/onenand/omap2.c >>> +++ b/drivers/mtd/onenand/omap2.c >>> @@ -152,17 +152,13 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) >>> } >>> >>> reinit_completion(&c->irq_done); >>> - if (c->gpio_irq) { >>> - result = gpio_get_value(c->gpio_irq); >>> - if (result == -1) { >>> - ctrl = read_reg(c, ONENAND_REG_CTRL_STATUS); >>> - intr = read_reg(c, ONENAND_REG_INTERRUPT); >>> - wait_err("gpio error", state, ctrl, intr); >>> - return -EIO; >>> - } >>> - } else >>> - result = 0; >>> - if (result == 0) { >>> + result = gpio_get_value(c->gpio_irq); >>> + if (result < 0) { >>> + ctrl = read_reg(c, ONENAND_REG_CTRL_STATUS); >>> + intr = read_reg(c, ONENAND_REG_INTERRUPT); >>> + wait_err("gpio error", state, ctrl, intr); >>> + return -EIO; >>> + } else if (result == 0) { >>> int retry_cnt = 0; >>> retry: >>> if (!wait_for_completion_io_timeout(&c->irq_done, >>> @@ -513,13 +509,15 @@ static int omap2_onenand_probe(struct platform_device *pdev) >> >> We need to get rid if this code from probe. >> >> if (pdata->dma_channel < 0) { >> /* if -1, don't use DMA */ >> c->gpio_irq = 0; >> } > > Yes. I overlooked it when shuffling with patches. In ended in later patch, but it > really belongs here. Will fix. > >>> dev_err(&pdev->dev, "Failed to request GPIO%d for " >>> "OneNAND\n", c->gpio_irq); >>> goto err_iounmap; >>> - } >>> - gpio_direction_input(c->gpio_irq); >>> + } >>> + gpio_direction_input(c->gpio_irq); >>> + >>> + if ((r = request_irq(gpio_to_irq(c->gpio_irq), >>> + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, >>> + pdev->dev.driver->name, c)) < 0) >>> + goto err_release_gpio; >>> >>> - if ((r = request_irq(gpio_to_irq(c->gpio_irq), >>> - omap2_onenand_interrupt, IRQF_TRIGGER_RISING, >>> - pdev->dev.driver->name, c)) < 0) >>> - goto err_release_gpio; >>> + this->wait = omap2_onenand_wait; >>> } >>> >>> if (pdata->dma_channel >= 0) { >>> @@ -529,15 +527,11 @@ static int omap2_onenand_probe(struct platform_device *pdev) >>> dma_cap_set(DMA_MEMCPY, mask); >>> >>> c->dma_chan = dma_request_channel(mask, NULL, NULL); >>> - if (!c->dma_chan) >>> - dev_info(&pdev->dev, >>> - "failed to allocate DMA for OneNAND, " >>> - "using PIO instead\n"); >> >> Why get rid of the print message? Instead we could choose to error out completely >> if a DMA channel was provided and we couldn't get it. > > The point is that without pdata->dma_channel condition above, DMA is always > enabled and it seems too strict to me to fail, when driver can continue to work. OK. fair enough. > >>> } >>> >>> dev_info(&pdev->dev, "initializing on CS%d, phys base 0x%08lx, virtual " >>> - "base %p, freq %d MHz\n", c->gpmc_cs, c->phys_base, >>> - c->onenand.base, c->freq); >>> + "base %p, freq %d MHz, %s mode\n", c->gpmc_cs, c->phys_base, >>> + c->onenand.base, c->freq, c->dma_chan ? "DMA" : "PIO"); > > See here ^. We just merely print message driver is running in PIO mode. > >>> c->pdev = pdev; >>> c->mtd.priv = &c->onenand; >>> @@ -547,7 +541,6 @@ static int omap2_onenand_probe(struct platform_device *pdev) >>> >>> this = &c->onenand; >>> if (c->dma_chan) { >>> - this->wait = omap2_onenand_wait; >>> this->read_bufferram = omap2_onenand_read_bufferram; >>> this->write_bufferram = omap2_onenand_write_bufferram; >>> } >>> >> >> -- >> cheers, >> -roger >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 15/11/17 12:32, Ladislav Michl wrote: > On Wed, Nov 15, 2017 at 12:08:05PM +0200, Roger Quadros wrote: >> Hi, >> >> On 11/11/17 23:24, Ladislav Michl wrote: >>> DMA and R/B pin are independent on each other. Use DMA by default. >> >> Is there a R/B pin on Onenand? It looks more like it has a RDY pin and >> an INT pin (which is used for command completion interrupt). > > (See also my previous answer) Yes, there's RDY and INT pin. I tried to use > the same scheme as NAND is using, but perhaps it doesn't fit too well here. > Therefore I'm proposing to keep INT pin name and use int-gpios in DT > bindings. Suggestions? Yes, I think that is better too.
Hi, On Wed, Nov 15, 2017 at 12:43:19PM +0200, Roger Quadros wrote: > On 15/11/17 12:32, Ladislav Michl wrote: > > On Wed, Nov 15, 2017 at 12:08:05PM +0200, Roger Quadros wrote: > >> Hi, > >> > >> On 11/11/17 23:24, Ladislav Michl wrote: > >>> @@ -529,15 +527,11 @@ static int omap2_onenand_probe(struct platform_device *pdev) > >>> dma_cap_set(DMA_MEMCPY, mask); > >>> > >>> c->dma_chan = dma_request_channel(mask, NULL, NULL); > >>> - if (!c->dma_chan) > >>> - dev_info(&pdev->dev, > >>> - "failed to allocate DMA for OneNAND, " > >>> - "using PIO instead\n"); > >> > >> Why get rid of the print message? Instead we could choose to error out completely > >> if a DMA channel was provided and we couldn't get it. > > > > The point is that without pdata->dma_channel condition above, DMA is always > > enabled and it seems too strict to me to fail, when driver can continue to work. > > OK. fair enough. Just FYI, tested 4.15-rc1 and got this: [ 0.168701] omap_hwmod: dma: no dt node [ 0.292968] omap_device: omap_dma_system: build failed (-22) [ 0.292968] omap2_system_dma_init_dev: Can't build omap_device for omap_dma_system:dma. [ 0.411590] onenand_check_lock_status: block = 2048, wp status = 0x2 [ 0.777435] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 0.787841] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 4.959228] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 5.077667] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 5.187011] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 5.288726] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 5.426971] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 5.987457] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 6.098937] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 6.243530] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 6.311950] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 6.355957] omap-sham 480c3000.sham: initialization failed. [ 6.397247] omap-aes 480c5000.aes: Unable to request in DMA channel [ 6.403869] omap-aes 480c5000.aes: initialization failed. [ 6.614807] omap-sham 480c3000.sham: initialization failed. [ 6.621551] omap-aes 480c5000.aes: Unable to request in DMA channel [ 6.628234] omap-aes 480c5000.aes: initialization failed. [ 6.793853] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 7.176910] omap-sham 480c3000.sham: initialization failed. [ 7.183563] omap-aes 480c5000.aes: Unable to request in DMA channel [ 7.190246] omap-aes 480c5000.aes: initialization failed. [ 7.417022] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 7.629211] omap-sham 480c3000.sham: initialization failed. [ 7.635986] omap-aes 480c5000.aes: Unable to request in DMA channel [ 7.642578] omap-aes 480c5000.aes: initialization failed. [ 7.854522] omap_hsmmc 4809c000.mmc: RX DMA channel request failed [ 7.938964] omap-sham 480c3000.sham: initialization failed. [ 7.945617] omap-aes 480c5000.aes: Unable to request in DMA channel [ 7.952331] omap-aes 480c5000.aes: initialization failed. [ 8.142791] omap_hsmmc 4809c000.mmc: RX DMA channel request failed Above error seems to be caused by missing DT changes merged during -rc1 (Which DT related brokeness is allowed and which not is still mystery to me). Anyway, driver can deal with that pretty well: omap2-onenand 30000000.onenand: initializing on CS0 (0x30000000), va e0080000, PIO mode ...and oprerates normally (unlike mmc, sound, etc.) Best regards, ladis
diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c index e4857a41760d..62e4ede918c4 100644 --- a/drivers/mtd/onenand/omap2.c +++ b/drivers/mtd/onenand/omap2.c @@ -152,17 +152,13 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) } reinit_completion(&c->irq_done); - if (c->gpio_irq) { - result = gpio_get_value(c->gpio_irq); - if (result == -1) { - ctrl = read_reg(c, ONENAND_REG_CTRL_STATUS); - intr = read_reg(c, ONENAND_REG_INTERRUPT); - wait_err("gpio error", state, ctrl, intr); - return -EIO; - } - } else - result = 0; - if (result == 0) { + result = gpio_get_value(c->gpio_irq); + if (result < 0) { + ctrl = read_reg(c, ONENAND_REG_CTRL_STATUS); + intr = read_reg(c, ONENAND_REG_INTERRUPT); + wait_err("gpio error", state, ctrl, intr); + return -EIO; + } else if (result == 0) { int retry_cnt = 0; retry: if (!wait_for_completion_io_timeout(&c->irq_done, @@ -513,13 +509,15 @@ static int omap2_onenand_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Failed to request GPIO%d for " "OneNAND\n", c->gpio_irq); goto err_iounmap; - } - gpio_direction_input(c->gpio_irq); + } + gpio_direction_input(c->gpio_irq); + + if ((r = request_irq(gpio_to_irq(c->gpio_irq), + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, + pdev->dev.driver->name, c)) < 0) + goto err_release_gpio; - if ((r = request_irq(gpio_to_irq(c->gpio_irq), - omap2_onenand_interrupt, IRQF_TRIGGER_RISING, - pdev->dev.driver->name, c)) < 0) - goto err_release_gpio; + this->wait = omap2_onenand_wait; } if (pdata->dma_channel >= 0) { @@ -529,15 +527,11 @@ static int omap2_onenand_probe(struct platform_device *pdev) dma_cap_set(DMA_MEMCPY, mask); c->dma_chan = dma_request_channel(mask, NULL, NULL); - if (!c->dma_chan) - dev_info(&pdev->dev, - "failed to allocate DMA for OneNAND, " - "using PIO instead\n"); } dev_info(&pdev->dev, "initializing on CS%d, phys base 0x%08lx, virtual " - "base %p, freq %d MHz\n", c->gpmc_cs, c->phys_base, - c->onenand.base, c->freq); + "base %p, freq %d MHz, %s mode\n", c->gpmc_cs, c->phys_base, + c->onenand.base, c->freq, c->dma_chan ? "DMA" : "PIO"); c->pdev = pdev; c->mtd.priv = &c->onenand; @@ -547,7 +541,6 @@ static int omap2_onenand_probe(struct platform_device *pdev) this = &c->onenand; if (c->dma_chan) { - this->wait = omap2_onenand_wait; this->read_bufferram = omap2_onenand_read_bufferram; this->write_bufferram = omap2_onenand_write_bufferram; }
DMA and R/B pin are independent on each other. Use DMA by default. Signed-off-by: Ladislav Michl <ladis@linux-mips.org> --- Changes: -v4: new patch drivers/mtd/onenand/omap2.c | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-)