| Message ID | 20171019144501.xw4r7wfxvfiy5xvp@lenoch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 19/10/17 17:45, Ladislav Michl wrote: > On Thu, Oct 19, 2017 at 05:02:51PM +0300, Roger Quadros wrote: >> On 19/10/17 16:42, Ladislav Michl wrote: >>> Hi, >>> >>> On Thu, Oct 19, 2017 at 03:56:30PM +0300, Roger Quadros wrote: > [snip] >>>> how about just "vdd" instead of "vonenand" ? >>> >>> Well, other subsystems are using this naming scheme, ie. vmmc, vwlan, vcpu0, >>> etc. But I do not have any strong preference here. >>> >> >> I see it this way. We already know that the supply is for oneneand since >> the property is in the onenand node. The property must only state the supply name >> e.g. vdd3v3, vdd5v, or just vdd. > > Okay, I'm convinced. > >>> So with regulator-can-sleep property drop, we could simply use: >>> devm_regulator_get_optional(dev, "vonenand"); >> >> right. >>> >>> We still need to privide something to control initial unlocking, see commit >>> c93ff6bf16523d ("mtd: omap: add new variable to platform data to control >>> onenand unlocking"). How ever it not used anywhere, so maybye just drop >>> to be readded later once needed? >>> >> >> And this commit b3dcfd35244e ("mtd: onenand: add new option to control initial >> onenand unlocking") >> >> But none of them explain why exactly it is needed. >> If none of the platforms are using it I'm OK with getting rid of it. > > No, there is no single user. > > So with those changs combined we have so far: > (How to pass parameters to omap2_onenand_set_cfg and how to get timings > right still needs to be investigated) > I think what you are doing currently is fine. See below. > --- > drivers/mtd/onenand/omap2.c | 324 ++++++++++++++++++++++++++------------------ > 1 file changed, 190 insertions(+), 134 deletions(-) > > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c > index 24a1388d3031..83847033510a 100644 > --- a/drivers/mtd/onenand/omap2.c > +++ b/drivers/mtd/onenand/omap2.c > @@ -28,17 +28,18 @@ > #include <linux/mtd/mtd.h> > #include <linux/mtd/onenand.h> > #include <linux/mtd/partitions.h> > +#include <linux/of_device.h> > +#include <linux/omap-gpmc.h> > #include <linux/platform_device.h> > #include <linux/interrupt.h> > #include <linux/delay.h> > #include <linux/dma-mapping.h> > #include <linux/io.h> > #include <linux/slab.h> > +#include <linux/gpio/consumer.h> > #include <linux/regulator/consumer.h> > -#include <linux/gpio.h> > > #include <asm/mach/flash.h> > -#include <linux/platform_data/mtd-onenand-omap2.h> > > #include <linux/omap-dma.h> > > @@ -50,17 +51,22 @@ struct omap2_onenand { > struct platform_device *pdev; > int gpmc_cs; > unsigned long phys_base; > - unsigned int mem_size; > - int gpio_irq; > + struct gpio_desc *gpiod; > struct mtd_info mtd; > struct onenand_chip onenand; > struct completion irq_done; > struct completion dma_done; > int dma_channel; > - int freq; > - int (*setup)(void __iomem *base, int *freq_ptr); > struct regulator *regulator; > - u8 flags; > + bool gpio_quirk; > +}; > + > +struct omap2_onenand_devdata { > + bool gpio_quirk; > + int (*read_bufferram)(struct mtd_info *mtd, int area, > + unsigned char *buffer, int offset, size_t count); > + int (*write_bufferram)(struct mtd_info *mtd, int area, > + const unsigned char *buffer, int offset, size_t count); > }; > > static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) > @@ -90,6 +96,55 @@ static inline void write_reg(struct omap2_onenand *c, unsigned short value, > writew(value, c->onenand.base + reg); > } > > +/* Ensure sync read and sync write are disabled */ > +static void omap2_onenand_set_async_mode(struct omap2_onenand *c) > +{ > + unsigned short reg; > + > + reg = read_reg(c, ONENAND_REG_SYS_CFG1); > + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE; > + write_reg(c, reg, ONENAND_REG_SYS_CFG1); > +} > + > +static void omap2_onenand_set_cfg(struct omap2_onenand *c, > + bool sr, bool sw, int latency) > +{ > + unsigned short reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT; > + > + reg |= (latency << ONENAND_SYS_CFG1_BRL_SHIFT) | > + ONENAND_SYS_CFG1_BL_16; > + if (latency > 5) > + reg |= ONENAND_SYS_CFG1_HF; > + if (latency > 7) > + reg |= ONENAND_SYS_CFG1_VHF; > + if (sr) > + reg |= ONENAND_SYS_CFG1_SYNC_READ; > + if (sw) > + reg |= ONENAND_SYS_CFG1_SYNC_WRITE; > + > + write_reg(c, reg, ONENAND_REG_SYS_CFG1); > +} > + > +static int omap2_onenand_get_freq(struct omap2_onenand *c) > +{ > + unsigned short ver = read_reg(c, ONENAND_REG_VERSION_ID); > + > + switch ((ver >> 4) & 0xf) { > + case 0: > + return 40; > + case 1: > + return 54; > + case 2: > + return 66; > + case 3: > + return 83; > + case 4: > + return 104; > + } > + > + return -ENODEV; Not sure if this is the right error code. The device is there. We just don't support the reported frequency. > +} > + > static void wait_err(char *msg, int state, unsigned int ctrl, unsigned int intr) > { > printk(KERN_ERR "onenand_wait: %s! state %d ctrl 0x%04x intr 0x%04x\n", > @@ -153,19 +208,19 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) > if (!(syscfg & ONENAND_SYS_CFG1_IOBE)) { > syscfg |= ONENAND_SYS_CFG1_IOBE; > write_reg(c, syscfg, ONENAND_REG_SYS_CFG1); > - if (c->flags & ONENAND_IN_OMAP34XX) > + if (c->gpio_quirk) > /* Add a delay to let GPIO settle */ > syscfg = read_reg(c, ONENAND_REG_SYS_CFG1); > } > > reinit_completion(&c->irq_done); > - if (c->gpio_irq) { > - result = gpio_get_value(c->gpio_irq); > - if (result == -1) { > + if (c->gpiod) { > + result = gpiod_get_value(c->gpiod); > + 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; > + return result; > } > } else > result = 0; > @@ -609,142 +664,140 @@ static int omap2_onenand_disable(struct mtd_info *mtd) > > static int omap2_onenand_probe(struct platform_device *pdev) > { > - struct omap_onenand_platform_data *pdata; > + u32 val; > + int freq, r; > + unsigned int mem_size; > + struct resource *res; > struct omap2_onenand *c; > struct onenand_chip *this; > - int r; > - struct resource *res; > + const struct omap2_onenand_devdata *devdata; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (res == NULL) { > + dev_err(dev, "error getting memory resource\n"); > + return -EINVAL; > + } > > - pdata = dev_get_platdata(&pdev->dev); > - if (pdata == NULL) { > - dev_err(&pdev->dev, "platform data missing\n"); > - return -ENODEV; > + r = of_property_read_u32(np, "reg", &val); > + if (r) { > + dev_err(dev, "reg not found in DT\n"); > + return r; > } > > - c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL); > + c = devm_kzalloc(dev, sizeof(struct omap2_onenand), GFP_KERNEL); > if (!c) > return -ENOMEM; > > init_completion(&c->irq_done); > init_completion(&c->dma_done); > - c->flags = pdata->flags; > - c->gpmc_cs = pdata->cs; > - c->gpio_irq = pdata->gpio_irq; > - c->dma_channel = pdata->dma_channel; > - if (c->dma_channel < 0) { > - /* if -1, don't use DMA */ > - c->gpio_irq = 0; > - } > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (res == NULL) { > - r = -EINVAL; > - dev_err(&pdev->dev, "error getting memory resource\n"); > - goto err_kfree; > - } > + devdata = of_device_get_match_data(dev); > + this = &c->onenand; > > + c->gpmc_cs = val; > + c->dma_channel = -1; > + c->gpio_quirk = devdata->gpio_quirk; > c->phys_base = res->start; > - c->mem_size = resource_size(res); > - > - if (request_mem_region(c->phys_base, c->mem_size, > - pdev->dev.driver->name) == NULL) { > - dev_err(&pdev->dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", > - c->phys_base, c->mem_size); > - r = -EBUSY; > - goto err_kfree; > + > + mem_size = resource_size(res); > + if (devm_request_mem_region(dev, c->phys_base, mem_size, > + dev->driver->name) == NULL) { > + > + dev_err(dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", > + c->phys_base, mem_size); > + return -EBUSY; > } > - c->onenand.base = ioremap(c->phys_base, c->mem_size); > - if (c->onenand.base == NULL) { > - r = -ENOMEM; > - goto err_release_mem_region; > + c->onenand.base = devm_ioremap(dev, c->phys_base, mem_size); > + if (c->onenand.base == NULL) > + return -ENOMEM; > + > + if (!of_property_read_u32(np, "dma-channel", &val)) { > + c->dma_channel = val; > + r = omap_request_dma(0, dev->driver->name, > + omap2_onenand_dma_cb, (void *) c, > + &c->dma_channel); > + if (r) { > + dev_info(dev, > + "failed to allocate DMA for OneNAND, " > + "using PIO instead\n"); Don't split the print message. It is hard to find while grepping during debug. > + c->dma_channel = -1; > + } > } > > - if (pdata->onenand_setup != NULL) { > - r = pdata->onenand_setup(c->onenand.base, &c->freq); > - if (r < 0) { > - dev_err(&pdev->dev, "Onenand platform setup failed: " > - "%d\n", r); > - goto err_iounmap; > + if (c->dma_channel >= 0) { > + this->wait = omap2_onenand_wait; > + this->read_bufferram = devdata->read_bufferram; > + this->write_bufferram = devdata->write_bufferram; > + > + c->gpiod = devm_gpiod_get(dev, "OneNAND irq", GPIOD_IN); > + if (IS_ERR(c->gpiod)) { > + r = PTR_ERR(c->gpiod); > + /* Just try again if this happens */ > + if (r != -EPROBE_DEFER) > + dev_err(dev, "error getting gpio (%d)\n", r); Use a uniform format "error getting gpio: %d", r: > + goto err_release_dma; > } > - c->setup = pdata->onenand_setup; > + r = devm_request_irq(dev, gpiod_to_irq(c->gpiod), > + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, > + "OneNAND irq", c); > + if (r) > + goto err_release_dma; > + } > + > + c->regulator = devm_regulator_get(dev, "vdd"); > + if (IS_ERR(c->regulator)) { > + r = PTR_ERR(c->regulator); > + dev_err(dev, "failed to get regulator (%d)\n", r); here too. > + goto err_release_dma; > + } > + if (c->regulator) { > + this->enable = omap2_onenand_enable; > + this->disable = omap2_onenand_disable; > } > > - if (c->gpio_irq) { > - if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) { > - dev_err(&pdev->dev, "Failed to request GPIO%d for " > - "OneNAND\n", c->gpio_irq); don't split print message. > - goto err_iounmap; > + omap2_onenand_set_async_mode(c); > + freq = omap2_onenand_get_freq(c); > + if (freq < 0) { > + dev_err(&pdev->dev, > + "Rate not detected, bad GPMC async timings?\n"); Message is misleading. Should be, "Unsupported device. MFR_ID:DEV_ID:VER_ID = %x:%x:%x" We should dump the device ID register in case someone wants to report the error and we need to add support for this new device. Not that it will happen but is just good practice. > + r = freq; > + goto err_release_dma; > } > - 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; Is it possible to operate oneNAND completely in ASYNC mode? If so then the below call should be conditional. Only if we need sync_read/write. We still need to figure out how to get the read/write sync flags here right? I suggest to add new DT properties for this. e.g. ti,onenand-sync-read; ti,onenand-sync-write; If any one or both are set we use SYNC timings. > + r = gpmc_omap_onenand_set_sync_timings(&pdev->dev, c->gpmc_cs, freq); > + if (r < 0) { > + dev_err(&pdev->dev, "Cannot set sync timings: %d\n", r); right. let's keep this format for all messages. > + goto err_release_dma; > } > + if (r > 0) > + omap2_onenand_set_cfg(c, true, true, r); Use the 2 new DT properties to figure out the sr, sw parameters. > > if (c->dma_channel >= 0) { > - r = omap_request_dma(0, pdev->dev.driver->name, > - omap2_onenand_dma_cb, (void *) c, > - &c->dma_channel); > - if (r == 0) { > - omap_set_dma_write_mode(c->dma_channel, > - OMAP_DMA_WRITE_NON_POSTED); > - omap_set_dma_src_data_pack(c->dma_channel, 1); > - omap_set_dma_src_burst_mode(c->dma_channel, > - OMAP_DMA_DATA_BURST_8); > - omap_set_dma_dest_data_pack(c->dma_channel, 1); > - omap_set_dma_dest_burst_mode(c->dma_channel, > - OMAP_DMA_DATA_BURST_8); > - } else { > - dev_info(&pdev->dev, > - "failed to allocate DMA for OneNAND, " > - "using PIO instead\n"); avoid splitting print message. > - c->dma_channel = -1; > - } > + omap_set_dma_write_mode(c->dma_channel, > + OMAP_DMA_WRITE_NON_POSTED); > + omap_set_dma_src_data_pack(c->dma_channel, 1); > + omap_set_dma_src_burst_mode(c->dma_channel, > + OMAP_DMA_DATA_BURST_8); > + omap_set_dma_dest_data_pack(c->dma_channel, 1); > + omap_set_dma_dest_burst_mode(c->dma_channel, > + OMAP_DMA_DATA_BURST_8); > } > > 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); > + c->onenand.base, freq); > > c->pdev = pdev; > c->mtd.priv = &c->onenand; > - > c->mtd.dev.parent = &pdev->dev; > - mtd_set_of_node(&c->mtd, pdata->of_node); > - > - this = &c->onenand; > - if (c->dma_channel >= 0) { > - this->wait = omap2_onenand_wait; > - if (c->flags & ONENAND_IN_OMAP34XX) { > - this->read_bufferram = omap3_onenand_read_bufferram; > - this->write_bufferram = omap3_onenand_write_bufferram; > - } else { > - this->read_bufferram = omap2_onenand_read_bufferram; > - this->write_bufferram = omap2_onenand_write_bufferram; > - } > - } > - > - if (pdata->regulator_can_sleep) { > - c->regulator = regulator_get(&pdev->dev, "vonenand"); > - if (IS_ERR(c->regulator)) { > - dev_err(&pdev->dev, "Failed to get regulator\n"); Need to print error code? > - r = PTR_ERR(c->regulator); > - goto err_release_dma; > - } > - c->onenand.enable = omap2_onenand_enable; > - c->onenand.disable = omap2_onenand_disable; > - } > - > - if (pdata->skip_initial_unlocking) > - this->options |= ONENAND_SKIP_INITIAL_UNLOCKING; > + mtd_set_of_node(&c->mtd, pdev->dev.of_node); > > if ((r = onenand_scan(&c->mtd, 1)) < 0) > - goto err_release_regulator; > + goto err_release_dma; > > - r = mtd_device_register(&c->mtd, pdata ? pdata->parts : NULL, > - pdata ? pdata->nr_parts : 0); > + r = mtd_device_register(&c->mtd, NULL, 0); > if (r) > goto err_release_onenand; > > @@ -754,22 +807,9 @@ static int omap2_onenand_probe(struct platform_device *pdev) > > err_release_onenand: > onenand_release(&c->mtd); > -err_release_regulator: > - regulator_put(c->regulator); > err_release_dma: > if (c->dma_channel != -1) > omap_free_dma(c->dma_channel); > - if (c->gpio_irq) > - free_irq(gpio_to_irq(c->gpio_irq), c); > -err_release_gpio: > - if (c->gpio_irq) > - gpio_free(c->gpio_irq); > -err_iounmap: > - iounmap(c->onenand.base); > -err_release_mem_region: > - release_mem_region(c->phys_base, c->mem_size); > -err_kfree: > - kfree(c); > > return r; > } > @@ -779,27 +819,43 @@ static int omap2_onenand_remove(struct platform_device *pdev) > struct omap2_onenand *c = dev_get_drvdata(&pdev->dev); > > onenand_release(&c->mtd); > - regulator_put(c->regulator); > if (c->dma_channel != -1) > omap_free_dma(c->dma_channel); > omap2_onenand_shutdown(pdev); > - if (c->gpio_irq) { > - free_irq(gpio_to_irq(c->gpio_irq), c); > - gpio_free(c->gpio_irq); > - } > - iounmap(c->onenand.base); > - release_mem_region(c->phys_base, c->mem_size); > - kfree(c); > > return 0; > } > > +static const struct omap2_onenand_devdata omap2_devdata = { > + .gpio_quirk = false, > + .read_bufferram = omap2_onenand_read_bufferram, > + .write_bufferram = omap2_onenand_write_bufferram, > +}; > + > +static const struct omap2_onenand_devdata omap3_devdata = { > + .gpio_quirk = true, > + .read_bufferram = omap3_onenand_read_bufferram, > + .write_bufferram = omap3_onenand_write_bufferram, > +}; > + > +static const struct of_device_id omap2_onenand_ids[] = { > + { > + .compatible = "ti,omap2-onenand", > + .data = &omap2_devdata, > + }, { > + .compatible = "ti,omap3-onenand", > + .data = &omap3_devdata, > + }, {}, > +}; > +MODULE_DEVICE_TABLE(of, omap2_onenand_ids); > + > static struct platform_driver omap2_onenand_driver = { > .probe = omap2_onenand_probe, > .remove = omap2_onenand_remove, > .shutdown = omap2_onenand_shutdown, > .driver = { > .name = DRIVER_NAME, > + .of_match_table = of_match_ptr(omap2_onenand_ids), > }, > }; > >
Hi, On Fri, Oct 20, 2017 at 10:58:29AM +0300, Roger Quadros wrote: > Hi, > > On 19/10/17 17:45, Ladislav Michl wrote: > > On Thu, Oct 19, 2017 at 05:02:51PM +0300, Roger Quadros wrote: > >> On 19/10/17 16:42, Ladislav Michl wrote: > >>> Hi, > >>> > >>> On Thu, Oct 19, 2017 at 03:56:30PM +0300, Roger Quadros wrote: > > [snip] > >>>> how about just "vdd" instead of "vonenand" ? > >>> > >>> Well, other subsystems are using this naming scheme, ie. vmmc, vwlan, vcpu0, > >>> etc. But I do not have any strong preference here. > >>> > >> > >> I see it this way. We already know that the supply is for oneneand since > >> the property is in the onenand node. The property must only state the supply name > >> e.g. vdd3v3, vdd5v, or just vdd. > > > > Okay, I'm convinced. > > > >>> So with regulator-can-sleep property drop, we could simply use: > >>> devm_regulator_get_optional(dev, "vonenand"); > >> > >> right. > >>> > >>> We still need to privide something to control initial unlocking, see commit > >>> c93ff6bf16523d ("mtd: omap: add new variable to platform data to control > >>> onenand unlocking"). How ever it not used anywhere, so maybye just drop > >>> to be readded later once needed? > >>> > >> > >> And this commit b3dcfd35244e ("mtd: onenand: add new option to control initial > >> onenand unlocking") > >> > >> But none of them explain why exactly it is needed. > >> If none of the platforms are using it I'm OK with getting rid of it. > > > > No, there is no single user. > > > > So with those changs combined we have so far: > > (How to pass parameters to omap2_onenand_set_cfg and how to get timings > > right still needs to be investigated) > > > > I think what you are doing currently is fine. See below. > > > --- > > drivers/mtd/onenand/omap2.c | 324 ++++++++++++++++++++++++++------------------ > > 1 file changed, 190 insertions(+), 134 deletions(-) > > > > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c > > index 24a1388d3031..83847033510a 100644 > > --- a/drivers/mtd/onenand/omap2.c > > +++ b/drivers/mtd/onenand/omap2.c > > @@ -28,17 +28,18 @@ > > #include <linux/mtd/mtd.h> > > #include <linux/mtd/onenand.h> > > #include <linux/mtd/partitions.h> > > +#include <linux/of_device.h> > > +#include <linux/omap-gpmc.h> > > #include <linux/platform_device.h> > > #include <linux/interrupt.h> > > #include <linux/delay.h> > > #include <linux/dma-mapping.h> > > #include <linux/io.h> > > #include <linux/slab.h> > > +#include <linux/gpio/consumer.h> > > #include <linux/regulator/consumer.h> > > -#include <linux/gpio.h> > > > > #include <asm/mach/flash.h> > > -#include <linux/platform_data/mtd-onenand-omap2.h> > > > > #include <linux/omap-dma.h> > > > > @@ -50,17 +51,22 @@ struct omap2_onenand { > > struct platform_device *pdev; > > int gpmc_cs; > > unsigned long phys_base; > > - unsigned int mem_size; > > - int gpio_irq; > > + struct gpio_desc *gpiod; > > struct mtd_info mtd; > > struct onenand_chip onenand; > > struct completion irq_done; > > struct completion dma_done; > > int dma_channel; > > - int freq; > > - int (*setup)(void __iomem *base, int *freq_ptr); > > struct regulator *regulator; > > - u8 flags; > > + bool gpio_quirk; > > +}; > > + > > +struct omap2_onenand_devdata { > > + bool gpio_quirk; > > + int (*read_bufferram)(struct mtd_info *mtd, int area, > > + unsigned char *buffer, int offset, size_t count); > > + int (*write_bufferram)(struct mtd_info *mtd, int area, > > + const unsigned char *buffer, int offset, size_t count); > > }; > > > > static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) > > @@ -90,6 +96,55 @@ static inline void write_reg(struct omap2_onenand *c, unsigned short value, > > writew(value, c->onenand.base + reg); > > } > > > > +/* Ensure sync read and sync write are disabled */ > > +static void omap2_onenand_set_async_mode(struct omap2_onenand *c) > > +{ > > + unsigned short reg; > > + > > + reg = read_reg(c, ONENAND_REG_SYS_CFG1); > > + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE; > > + write_reg(c, reg, ONENAND_REG_SYS_CFG1); > > +} > > + > > +static void omap2_onenand_set_cfg(struct omap2_onenand *c, > > + bool sr, bool sw, int latency) > > +{ > > + unsigned short reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT; > > + > > + reg |= (latency << ONENAND_SYS_CFG1_BRL_SHIFT) | > > + ONENAND_SYS_CFG1_BL_16; > > + if (latency > 5) > > + reg |= ONENAND_SYS_CFG1_HF; > > + if (latency > 7) > > + reg |= ONENAND_SYS_CFG1_VHF; > > + if (sr) > > + reg |= ONENAND_SYS_CFG1_SYNC_READ; > > + if (sw) > > + reg |= ONENAND_SYS_CFG1_SYNC_WRITE; > > + > > + write_reg(c, reg, ONENAND_REG_SYS_CFG1); > > +} > > + > > +static int omap2_onenand_get_freq(struct omap2_onenand *c) > > +{ > > + unsigned short ver = read_reg(c, ONENAND_REG_VERSION_ID); > > + > > + switch ((ver >> 4) & 0xf) { > > + case 0: > > + return 40; > > + case 1: > > + return 54; > > + case 2: > > + return 66; > > + case 3: > > + return 83; > > + case 4: > > + return 104; > > + } > > + > > + return -ENODEV; > > Not sure if this is the right error code. > The device is there. We just don't support the reported frequency. We do not know yet device is there, MTG scans for device later. I just kept the logic we had before. Premiliary datasheet lists VERSION reg yet undefined https://www.digchip.com/datasheets/download_datasheet.php?id=1089403&part-number=KFG1G16Q2A so not sure what's inside and later datasheets do not describe it either. > > +} > > + > > static void wait_err(char *msg, int state, unsigned int ctrl, unsigned int intr) > > { > > printk(KERN_ERR "onenand_wait: %s! state %d ctrl 0x%04x intr 0x%04x\n", > > @@ -153,19 +208,19 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) > > if (!(syscfg & ONENAND_SYS_CFG1_IOBE)) { > > syscfg |= ONENAND_SYS_CFG1_IOBE; > > write_reg(c, syscfg, ONENAND_REG_SYS_CFG1); > > - if (c->flags & ONENAND_IN_OMAP34XX) > > + if (c->gpio_quirk) > > /* Add a delay to let GPIO settle */ > > syscfg = read_reg(c, ONENAND_REG_SYS_CFG1); > > } > > > > reinit_completion(&c->irq_done); > > - if (c->gpio_irq) { > > - result = gpio_get_value(c->gpio_irq); > > - if (result == -1) { > > + if (c->gpiod) { > > + result = gpiod_get_value(c->gpiod); > > + 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; > > + return result; > > } > > } else > > result = 0; > > @@ -609,142 +664,140 @@ static int omap2_onenand_disable(struct mtd_info *mtd) > > > > static int omap2_onenand_probe(struct platform_device *pdev) > > { > > - struct omap_onenand_platform_data *pdata; > > + u32 val; > > + int freq, r; > > + unsigned int mem_size; > > + struct resource *res; > > struct omap2_onenand *c; > > struct onenand_chip *this; > > - int r; > > - struct resource *res; > > + const struct omap2_onenand_devdata *devdata; > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (res == NULL) { > > + dev_err(dev, "error getting memory resource\n"); > > + return -EINVAL; > > + } > > > > - pdata = dev_get_platdata(&pdev->dev); > > - if (pdata == NULL) { > > - dev_err(&pdev->dev, "platform data missing\n"); > > - return -ENODEV; > > + r = of_property_read_u32(np, "reg", &val); > > + if (r) { > > + dev_err(dev, "reg not found in DT\n"); > > + return r; > > } > > > > - c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL); > > + c = devm_kzalloc(dev, sizeof(struct omap2_onenand), GFP_KERNEL); > > if (!c) > > return -ENOMEM; > > > > init_completion(&c->irq_done); > > init_completion(&c->dma_done); > > - c->flags = pdata->flags; > > - c->gpmc_cs = pdata->cs; > > - c->gpio_irq = pdata->gpio_irq; > > - c->dma_channel = pdata->dma_channel; > > - if (c->dma_channel < 0) { > > - /* if -1, don't use DMA */ > > - c->gpio_irq = 0; > > - } > > > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > - if (res == NULL) { > > - r = -EINVAL; > > - dev_err(&pdev->dev, "error getting memory resource\n"); > > - goto err_kfree; > > - } > > + devdata = of_device_get_match_data(dev); > > + this = &c->onenand; > > > > + c->gpmc_cs = val; > > + c->dma_channel = -1; > > + c->gpio_quirk = devdata->gpio_quirk; > > c->phys_base = res->start; > > - c->mem_size = resource_size(res); > > - > > - if (request_mem_region(c->phys_base, c->mem_size, > > - pdev->dev.driver->name) == NULL) { > > - dev_err(&pdev->dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", > > - c->phys_base, c->mem_size); > > - r = -EBUSY; > > - goto err_kfree; > > + > > + mem_size = resource_size(res); > > + if (devm_request_mem_region(dev, c->phys_base, mem_size, > > + dev->driver->name) == NULL) { > > + > > + dev_err(dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", > > + c->phys_base, mem_size); > > + return -EBUSY; > > } > > - c->onenand.base = ioremap(c->phys_base, c->mem_size); > > - if (c->onenand.base == NULL) { > > - r = -ENOMEM; > > - goto err_release_mem_region; > > + c->onenand.base = devm_ioremap(dev, c->phys_base, mem_size); > > + if (c->onenand.base == NULL) > > + return -ENOMEM; > > + > > + if (!of_property_read_u32(np, "dma-channel", &val)) { > > + c->dma_channel = val; > > + r = omap_request_dma(0, dev->driver->name, > > + omap2_onenand_dma_cb, (void *) c, > > + &c->dma_channel); > > + if (r) { > > + dev_info(dev, > > + "failed to allocate DMA for OneNAND, " > > + "using PIO instead\n"); > Don't split the print message. It is hard to find while grepping during debug. Will fix all that issues in the next version. > > + c->dma_channel = -1; > > + } > > } > > > > - if (pdata->onenand_setup != NULL) { > > - r = pdata->onenand_setup(c->onenand.base, &c->freq); > > - if (r < 0) { > > - dev_err(&pdev->dev, "Onenand platform setup failed: " > > - "%d\n", r); > > - goto err_iounmap; > > + if (c->dma_channel >= 0) { > > + this->wait = omap2_onenand_wait; > > + this->read_bufferram = devdata->read_bufferram; > > + this->write_bufferram = devdata->write_bufferram; > > + > > + c->gpiod = devm_gpiod_get(dev, "OneNAND irq", GPIOD_IN); > > + if (IS_ERR(c->gpiod)) { > > + r = PTR_ERR(c->gpiod); > > + /* Just try again if this happens */ > > + if (r != -EPROBE_DEFER) > > + dev_err(dev, "error getting gpio (%d)\n", r); > > Use a uniform format > "error getting gpio: %d", r: > > > > + goto err_release_dma; > > } > > - c->setup = pdata->onenand_setup; > > + r = devm_request_irq(dev, gpiod_to_irq(c->gpiod), > > + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, > > + "OneNAND irq", c); > > + if (r) > > + goto err_release_dma; > > + } > > + > > + c->regulator = devm_regulator_get(dev, "vdd"); > > + if (IS_ERR(c->regulator)) { > > + r = PTR_ERR(c->regulator); > > + dev_err(dev, "failed to get regulator (%d)\n", r); > > here too. > > > + goto err_release_dma; > > + } > > + if (c->regulator) { > > + this->enable = omap2_onenand_enable; > > + this->disable = omap2_onenand_disable; > > } > > > > - if (c->gpio_irq) { > > - if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) { > > - dev_err(&pdev->dev, "Failed to request GPIO%d for " > > - "OneNAND\n", c->gpio_irq); > > don't split print message. > > > - goto err_iounmap; > > + omap2_onenand_set_async_mode(c); > > + freq = omap2_onenand_get_freq(c); > > + if (freq < 0) { > > + dev_err(&pdev->dev, > > + "Rate not detected, bad GPMC async timings?\n"); > > Message is misleading. Should be, > "Unsupported device. MFR_ID:DEV_ID:VER_ID = %x:%x:%x" > > We should dump the device ID register in case someone wants to report the error > and we need to add support for this new device. Not that it will happen > but is just good practice. Yes, message has historical reasons (same as this splitted lines), but see bellow. > > + r = freq; > > + goto err_release_dma; > > } > > - 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; > > Is it possible to operate oneNAND completely in ASYNC mode? > If so then the below call should be conditional. Only if we need sync_read/write. According to datasheet and origilal code, yes. I haven't tried yet (away from the lab). > We still need to figure out how to get the read/write sync flags here right? > I suggest to add new DT properties for this. > e.g. > ti,onenand-sync-read; > ti,onenand-sync-write; > > > If any one or both are set we use SYNC timings. What is we change logic a bit here? I still do think those gpmc generic properties are sufficient. Code contains following when setting asyns timings: /* * Note that we need to keep sync_write set for the call to * omap2_onenand_set_async_mode() to work to detect the onenand * supported clock rate for the sync timings. */ s->sync_read = false; s->sync_write = true; When DT mode contains async timings, lets use them and do not bother with sync ones. If it contains sync timings, then it should be those which certainly works [*]. OneNAND node can contain something like 'ti,onenand-optimize-timings' and driver will attempt to get frequency and set better timings. This will be done after devce probe, so we know device indentification and we can print it if obtaining frequency fails. Also, this way we do not need special handlings for OneNAND nodes. [*] As Version ID reg is not described in datasheet, but driver relies on its content, we should leave on DT node author to make sure sync mode works. > > + r = gpmc_omap_onenand_set_sync_timings(&pdev->dev, c->gpmc_cs, freq); > > + if (r < 0) { > > + dev_err(&pdev->dev, "Cannot set sync timings: %d\n", r); > > right. let's keep this format for all messages. > > > + goto err_release_dma; > > } > > + if (r > 0) > > + omap2_onenand_set_cfg(c, true, true, r); > > Use the 2 new DT properties to figure out the sr, sw parameters. With the above proposal we would need both ti,onenand-sync-read; ti,onenand-sync-write; and gpmc,sync-read; gpmc,sync-write; Is it really worth it? > > > > if (c->dma_channel >= 0) { > > - r = omap_request_dma(0, pdev->dev.driver->name, > > - omap2_onenand_dma_cb, (void *) c, > > - &c->dma_channel); > > - if (r == 0) { > > - omap_set_dma_write_mode(c->dma_channel, > > - OMAP_DMA_WRITE_NON_POSTED); > > - omap_set_dma_src_data_pack(c->dma_channel, 1); > > - omap_set_dma_src_burst_mode(c->dma_channel, > > - OMAP_DMA_DATA_BURST_8); > > - omap_set_dma_dest_data_pack(c->dma_channel, 1); > > - omap_set_dma_dest_burst_mode(c->dma_channel, > > - OMAP_DMA_DATA_BURST_8); > > - } else { > > - dev_info(&pdev->dev, > > - "failed to allocate DMA for OneNAND, " > > - "using PIO instead\n"); > > avoid splitting print message. > > > - c->dma_channel = -1; > > - } > > + omap_set_dma_write_mode(c->dma_channel, > > + OMAP_DMA_WRITE_NON_POSTED); > > + omap_set_dma_src_data_pack(c->dma_channel, 1); > > + omap_set_dma_src_burst_mode(c->dma_channel, > > + OMAP_DMA_DATA_BURST_8); > > + omap_set_dma_dest_data_pack(c->dma_channel, 1); > > + omap_set_dma_dest_burst_mode(c->dma_channel, > > + OMAP_DMA_DATA_BURST_8); > > } > > > > 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); > > + c->onenand.base, freq); > > > > c->pdev = pdev; > > c->mtd.priv = &c->onenand; > > - > > c->mtd.dev.parent = &pdev->dev; > > - mtd_set_of_node(&c->mtd, pdata->of_node); > > - > > - this = &c->onenand; > > - if (c->dma_channel >= 0) { > > - this->wait = omap2_onenand_wait; > > - if (c->flags & ONENAND_IN_OMAP34XX) { > > - this->read_bufferram = omap3_onenand_read_bufferram; > > - this->write_bufferram = omap3_onenand_write_bufferram; > > - } else { > > - this->read_bufferram = omap2_onenand_read_bufferram; > > - this->write_bufferram = omap2_onenand_write_bufferram; > > - } > > - } > > - > > - if (pdata->regulator_can_sleep) { > > - c->regulator = regulator_get(&pdev->dev, "vonenand"); > > - if (IS_ERR(c->regulator)) { > > - dev_err(&pdev->dev, "Failed to get regulator\n"); > > Need to print error code? > > > - r = PTR_ERR(c->regulator); > > - goto err_release_dma; > > - } > > - c->onenand.enable = omap2_onenand_enable; > > - c->onenand.disable = omap2_onenand_disable; > > - } > > - > > - if (pdata->skip_initial_unlocking) > > - this->options |= ONENAND_SKIP_INITIAL_UNLOCKING; > > + mtd_set_of_node(&c->mtd, pdev->dev.of_node); > > > > if ((r = onenand_scan(&c->mtd, 1)) < 0) > > - goto err_release_regulator; > > + goto err_release_dma; > > > > - r = mtd_device_register(&c->mtd, pdata ? pdata->parts : NULL, > > - pdata ? pdata->nr_parts : 0); > > + r = mtd_device_register(&c->mtd, NULL, 0); > > if (r) > > goto err_release_onenand; > > > > @@ -754,22 +807,9 @@ static int omap2_onenand_probe(struct platform_device *pdev) > > > > err_release_onenand: > > onenand_release(&c->mtd); > > -err_release_regulator: > > - regulator_put(c->regulator); > > err_release_dma: > > if (c->dma_channel != -1) > > omap_free_dma(c->dma_channel); > > - if (c->gpio_irq) > > - free_irq(gpio_to_irq(c->gpio_irq), c); > > -err_release_gpio: > > - if (c->gpio_irq) > > - gpio_free(c->gpio_irq); > > -err_iounmap: > > - iounmap(c->onenand.base); > > -err_release_mem_region: > > - release_mem_region(c->phys_base, c->mem_size); > > -err_kfree: > > - kfree(c); > > > > return r; > > } > > @@ -779,27 +819,43 @@ static int omap2_onenand_remove(struct platform_device *pdev) > > struct omap2_onenand *c = dev_get_drvdata(&pdev->dev); > > > > onenand_release(&c->mtd); > > - regulator_put(c->regulator); > > if (c->dma_channel != -1) > > omap_free_dma(c->dma_channel); > > omap2_onenand_shutdown(pdev); > > - if (c->gpio_irq) { > > - free_irq(gpio_to_irq(c->gpio_irq), c); > > - gpio_free(c->gpio_irq); > > - } > > - iounmap(c->onenand.base); > > - release_mem_region(c->phys_base, c->mem_size); > > - kfree(c); > > > > return 0; > > } > > > > +static const struct omap2_onenand_devdata omap2_devdata = { > > + .gpio_quirk = false, > > + .read_bufferram = omap2_onenand_read_bufferram, > > + .write_bufferram = omap2_onenand_write_bufferram, > > +}; > > + > > +static const struct omap2_onenand_devdata omap3_devdata = { > > + .gpio_quirk = true, > > + .read_bufferram = omap3_onenand_read_bufferram, > > + .write_bufferram = omap3_onenand_write_bufferram, > > +}; > > + > > +static const struct of_device_id omap2_onenand_ids[] = { > > + { > > + .compatible = "ti,omap2-onenand", > > + .data = &omap2_devdata, > > + }, { > > + .compatible = "ti,omap3-onenand", > > + .data = &omap3_devdata, > > + }, {}, > > +}; > > +MODULE_DEVICE_TABLE(of, omap2_onenand_ids); > > + > > static struct platform_driver omap2_onenand_driver = { > > .probe = omap2_onenand_probe, > > .remove = omap2_onenand_remove, > > .shutdown = omap2_onenand_shutdown, > > .driver = { > > .name = DRIVER_NAME, > > + .of_match_table = of_match_ptr(omap2_onenand_ids), > > }, > > }; > > > > > > -- > cheers, > -roger > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki Nice to see it moved to the bottom ;-) regards, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 20/10/17 13:19, Ladislav Michl wrote: > Hi, > > On Fri, Oct 20, 2017 at 10:58:29AM +0300, Roger Quadros wrote: >> Hi, >> >> On 19/10/17 17:45, Ladislav Michl wrote: >>> On Thu, Oct 19, 2017 at 05:02:51PM +0300, Roger Quadros wrote: >>>> On 19/10/17 16:42, Ladislav Michl wrote: >>>>> Hi, >>>>> >>>>> On Thu, Oct 19, 2017 at 03:56:30PM +0300, Roger Quadros wrote: >>> [snip] >>>>>> how about just "vdd" instead of "vonenand" ? >>>>> >>>>> Well, other subsystems are using this naming scheme, ie. vmmc, vwlan, vcpu0, >>>>> etc. But I do not have any strong preference here. >>>>> >>>> >>>> I see it this way. We already know that the supply is for oneneand since >>>> the property is in the onenand node. The property must only state the supply name >>>> e.g. vdd3v3, vdd5v, or just vdd. >>> >>> Okay, I'm convinced. >>> >>>>> So with regulator-can-sleep property drop, we could simply use: >>>>> devm_regulator_get_optional(dev, "vonenand"); >>>> >>>> right. >>>>> >>>>> We still need to privide something to control initial unlocking, see commit >>>>> c93ff6bf16523d ("mtd: omap: add new variable to platform data to control >>>>> onenand unlocking"). How ever it not used anywhere, so maybye just drop >>>>> to be readded later once needed? >>>>> >>>> >>>> And this commit b3dcfd35244e ("mtd: onenand: add new option to control initial >>>> onenand unlocking") >>>> >>>> But none of them explain why exactly it is needed. >>>> If none of the platforms are using it I'm OK with getting rid of it. >>> >>> No, there is no single user. >>> >>> So with those changs combined we have so far: >>> (How to pass parameters to omap2_onenand_set_cfg and how to get timings >>> right still needs to be investigated) >>> >> >> I think what you are doing currently is fine. See below. >> >>> --- >>> drivers/mtd/onenand/omap2.c | 324 ++++++++++++++++++++++++++------------------ >>> 1 file changed, 190 insertions(+), 134 deletions(-) >>> >>> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c >>> index 24a1388d3031..83847033510a 100644 >>> --- a/drivers/mtd/onenand/omap2.c >>> +++ b/drivers/mtd/onenand/omap2.c >>> @@ -28,17 +28,18 @@ >>> #include <linux/mtd/mtd.h> >>> #include <linux/mtd/onenand.h> >>> #include <linux/mtd/partitions.h> >>> +#include <linux/of_device.h> >>> +#include <linux/omap-gpmc.h> >>> #include <linux/platform_device.h> >>> #include <linux/interrupt.h> >>> #include <linux/delay.h> >>> #include <linux/dma-mapping.h> >>> #include <linux/io.h> >>> #include <linux/slab.h> >>> +#include <linux/gpio/consumer.h> >>> #include <linux/regulator/consumer.h> >>> -#include <linux/gpio.h> >>> >>> #include <asm/mach/flash.h> >>> -#include <linux/platform_data/mtd-onenand-omap2.h> >>> >>> #include <linux/omap-dma.h> >>> >>> @@ -50,17 +51,22 @@ struct omap2_onenand { >>> struct platform_device *pdev; >>> int gpmc_cs; >>> unsigned long phys_base; >>> - unsigned int mem_size; >>> - int gpio_irq; >>> + struct gpio_desc *gpiod; >>> struct mtd_info mtd; >>> struct onenand_chip onenand; >>> struct completion irq_done; >>> struct completion dma_done; >>> int dma_channel; >>> - int freq; >>> - int (*setup)(void __iomem *base, int *freq_ptr); >>> struct regulator *regulator; >>> - u8 flags; >>> + bool gpio_quirk; >>> +}; >>> + >>> +struct omap2_onenand_devdata { >>> + bool gpio_quirk; >>> + int (*read_bufferram)(struct mtd_info *mtd, int area, >>> + unsigned char *buffer, int offset, size_t count); >>> + int (*write_bufferram)(struct mtd_info *mtd, int area, >>> + const unsigned char *buffer, int offset, size_t count); >>> }; >>> >>> static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) >>> @@ -90,6 +96,55 @@ static inline void write_reg(struct omap2_onenand *c, unsigned short value, >>> writew(value, c->onenand.base + reg); >>> } >>> >>> +/* Ensure sync read and sync write are disabled */ >>> +static void omap2_onenand_set_async_mode(struct omap2_onenand *c) >>> +{ >>> + unsigned short reg; >>> + >>> + reg = read_reg(c, ONENAND_REG_SYS_CFG1); >>> + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE; >>> + write_reg(c, reg, ONENAND_REG_SYS_CFG1); >>> +} >>> + >>> +static void omap2_onenand_set_cfg(struct omap2_onenand *c, >>> + bool sr, bool sw, int latency) >>> +{ >>> + unsigned short reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT; >>> + >>> + reg |= (latency << ONENAND_SYS_CFG1_BRL_SHIFT) | >>> + ONENAND_SYS_CFG1_BL_16; >>> + if (latency > 5) >>> + reg |= ONENAND_SYS_CFG1_HF; >>> + if (latency > 7) >>> + reg |= ONENAND_SYS_CFG1_VHF; >>> + if (sr) >>> + reg |= ONENAND_SYS_CFG1_SYNC_READ; >>> + if (sw) >>> + reg |= ONENAND_SYS_CFG1_SYNC_WRITE; >>> + >>> + write_reg(c, reg, ONENAND_REG_SYS_CFG1); >>> +} >>> + >>> +static int omap2_onenand_get_freq(struct omap2_onenand *c) >>> +{ >>> + unsigned short ver = read_reg(c, ONENAND_REG_VERSION_ID); >>> + >>> + switch ((ver >> 4) & 0xf) { >>> + case 0: >>> + return 40; >>> + case 1: >>> + return 54; >>> + case 2: >>> + return 66; >>> + case 3: >>> + return 83; >>> + case 4: >>> + return 104; >>> + } >>> + >>> + return -ENODEV; >> >> Not sure if this is the right error code. >> The device is there. We just don't support the reported frequency. > > We do not know yet device is there, MTG scans for device later. I just kept > the logic we had before. OK, let's keep the logic unchanged. we can improve it separately. Just some ideas for later. There are 2 things we need to check for. 1) ASYNC timings could be incorrect so read returns invalid value. To check if that happened we could read the manufacturer register and if it is invalid, complain about incorrect gpmc ASYNC settings/timings. Fortunately there are only 2 valid manufacturers, so it should be easy. https://github.com/torvalds/linux/blob/master/include/linux/mtd/onenand.h#L211 2) If manufacturer register was read fine then we assume that frequency register will be valid. If we don't support the frequency then we say exactly that. > > Premiliary datasheet lists VERSION reg yet undefined > https://www.digchip.com/datasheets/download_datasheet.php?id=1089403&part-number=KFG1G16Q2A > so not sure what's inside and later datasheets do not describe it either. > >>> +} >>> + >>> static void wait_err(char *msg, int state, unsigned int ctrl, unsigned int intr) >>> { >>> printk(KERN_ERR "onenand_wait: %s! state %d ctrl 0x%04x intr 0x%04x\n", >>> @@ -153,19 +208,19 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) >>> if (!(syscfg & ONENAND_SYS_CFG1_IOBE)) { >>> syscfg |= ONENAND_SYS_CFG1_IOBE; >>> write_reg(c, syscfg, ONENAND_REG_SYS_CFG1); >>> - if (c->flags & ONENAND_IN_OMAP34XX) >>> + if (c->gpio_quirk) >>> /* Add a delay to let GPIO settle */ >>> syscfg = read_reg(c, ONENAND_REG_SYS_CFG1); >>> } >>> >>> reinit_completion(&c->irq_done); >>> - if (c->gpio_irq) { >>> - result = gpio_get_value(c->gpio_irq); >>> - if (result == -1) { >>> + if (c->gpiod) { >>> + result = gpiod_get_value(c->gpiod); >>> + 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; >>> + return result; >>> } >>> } else >>> result = 0; >>> @@ -609,142 +664,140 @@ static int omap2_onenand_disable(struct mtd_info *mtd) >>> >>> static int omap2_onenand_probe(struct platform_device *pdev) >>> { >>> - struct omap_onenand_platform_data *pdata; >>> + u32 val; >>> + int freq, r; >>> + unsigned int mem_size; >>> + struct resource *res; >>> struct omap2_onenand *c; >>> struct onenand_chip *this; >>> - int r; >>> - struct resource *res; >>> + const struct omap2_onenand_devdata *devdata; >>> + struct device *dev = &pdev->dev; >>> + struct device_node *np = dev->of_node; >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + if (res == NULL) { >>> + dev_err(dev, "error getting memory resource\n"); >>> + return -EINVAL; >>> + } >>> >>> - pdata = dev_get_platdata(&pdev->dev); >>> - if (pdata == NULL) { >>> - dev_err(&pdev->dev, "platform data missing\n"); >>> - return -ENODEV; >>> + r = of_property_read_u32(np, "reg", &val); >>> + if (r) { >>> + dev_err(dev, "reg not found in DT\n"); >>> + return r; >>> } >>> >>> - c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL); >>> + c = devm_kzalloc(dev, sizeof(struct omap2_onenand), GFP_KERNEL); >>> if (!c) >>> return -ENOMEM; >>> >>> init_completion(&c->irq_done); >>> init_completion(&c->dma_done); >>> - c->flags = pdata->flags; >>> - c->gpmc_cs = pdata->cs; >>> - c->gpio_irq = pdata->gpio_irq; >>> - c->dma_channel = pdata->dma_channel; >>> - if (c->dma_channel < 0) { >>> - /* if -1, don't use DMA */ >>> - c->gpio_irq = 0; >>> - } >>> >>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> - if (res == NULL) { >>> - r = -EINVAL; >>> - dev_err(&pdev->dev, "error getting memory resource\n"); >>> - goto err_kfree; >>> - } >>> + devdata = of_device_get_match_data(dev); >>> + this = &c->onenand; >>> >>> + c->gpmc_cs = val; >>> + c->dma_channel = -1; >>> + c->gpio_quirk = devdata->gpio_quirk; >>> c->phys_base = res->start; >>> - c->mem_size = resource_size(res); >>> - >>> - if (request_mem_region(c->phys_base, c->mem_size, >>> - pdev->dev.driver->name) == NULL) { >>> - dev_err(&pdev->dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", >>> - c->phys_base, c->mem_size); >>> - r = -EBUSY; >>> - goto err_kfree; >>> + >>> + mem_size = resource_size(res); >>> + if (devm_request_mem_region(dev, c->phys_base, mem_size, >>> + dev->driver->name) == NULL) { >>> + >>> + dev_err(dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", >>> + c->phys_base, mem_size); >>> + return -EBUSY; >>> } >>> - c->onenand.base = ioremap(c->phys_base, c->mem_size); >>> - if (c->onenand.base == NULL) { >>> - r = -ENOMEM; >>> - goto err_release_mem_region; >>> + c->onenand.base = devm_ioremap(dev, c->phys_base, mem_size); >>> + if (c->onenand.base == NULL) >>> + return -ENOMEM; >>> + >>> + if (!of_property_read_u32(np, "dma-channel", &val)) { >>> + c->dma_channel = val; >>> + r = omap_request_dma(0, dev->driver->name, >>> + omap2_onenand_dma_cb, (void *) c, >>> + &c->dma_channel); >>> + if (r) { >>> + dev_info(dev, >>> + "failed to allocate DMA for OneNAND, " >>> + "using PIO instead\n"); >> Don't split the print message. It is hard to find while grepping during debug. > > Will fix all that issues in the next version. > >>> + c->dma_channel = -1; >>> + } >>> } >>> >>> - if (pdata->onenand_setup != NULL) { >>> - r = pdata->onenand_setup(c->onenand.base, &c->freq); >>> - if (r < 0) { >>> - dev_err(&pdev->dev, "Onenand platform setup failed: " >>> - "%d\n", r); >>> - goto err_iounmap; >>> + if (c->dma_channel >= 0) { >>> + this->wait = omap2_onenand_wait; >>> + this->read_bufferram = devdata->read_bufferram; >>> + this->write_bufferram = devdata->write_bufferram; >>> + >>> + c->gpiod = devm_gpiod_get(dev, "OneNAND irq", GPIOD_IN); >>> + if (IS_ERR(c->gpiod)) { >>> + r = PTR_ERR(c->gpiod); >>> + /* Just try again if this happens */ >>> + if (r != -EPROBE_DEFER) >>> + dev_err(dev, "error getting gpio (%d)\n", r); >> >> Use a uniform format >> "error getting gpio: %d", r: >> >> >>> + goto err_release_dma; >>> } >>> - c->setup = pdata->onenand_setup; >>> + r = devm_request_irq(dev, gpiod_to_irq(c->gpiod), >>> + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, >>> + "OneNAND irq", c); >>> + if (r) >>> + goto err_release_dma; >>> + } >>> + >>> + c->regulator = devm_regulator_get(dev, "vdd"); >>> + if (IS_ERR(c->regulator)) { >>> + r = PTR_ERR(c->regulator); >>> + dev_err(dev, "failed to get regulator (%d)\n", r); >> >> here too. >> >>> + goto err_release_dma; >>> + } >>> + if (c->regulator) { >>> + this->enable = omap2_onenand_enable; >>> + this->disable = omap2_onenand_disable; >>> } >>> >>> - if (c->gpio_irq) { >>> - if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) { >>> - dev_err(&pdev->dev, "Failed to request GPIO%d for " >>> - "OneNAND\n", c->gpio_irq); >> >> don't split print message. >> >>> - goto err_iounmap; >>> + omap2_onenand_set_async_mode(c); >>> + freq = omap2_onenand_get_freq(c); >>> + if (freq < 0) { >>> + dev_err(&pdev->dev, >>> + "Rate not detected, bad GPMC async timings?\n"); >> >> Message is misleading. Should be, >> "Unsupported device. MFR_ID:DEV_ID:VER_ID = %x:%x:%x" >> >> We should dump the device ID register in case someone wants to report the error >> and we need to add support for this new device. Not that it will happen >> but is just good practice. > > Yes, message has historical reasons (same as this splitted lines), but see > bellow. > >>> + r = freq; >>> + goto err_release_dma; >>> } >>> - 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; >> >> Is it possible to operate oneNAND completely in ASYNC mode? >> If so then the below call should be conditional. Only if we need sync_read/write. > > According to datasheet and origilal code, yes. I haven't tried yet (away from > the lab). > >> We still need to figure out how to get the read/write sync flags here right? >> I suggest to add new DT properties for this. >> e.g. >> ti,onenand-sync-read; >> ti,onenand-sync-write; >> >> >> If any one or both are set we use SYNC timings. > > What is we change logic a bit here? I still do think those gpmc generic > properties are sufficient. > > Code contains following when setting asyns timings: > /* > * Note that we need to keep sync_write set for the call to > * omap2_onenand_set_async_mode() to work to detect the onenand > * supported clock rate for the sync timings. > */ > s->sync_read = false; > s->sync_write = true; This comes from commit e7b11dc7b77b ("ARM: OMAP2+: Fix onenand rate detection to avoid filesystem corruption") Looks like it was a quick fix. But IMO it is not correct. The real problem is that the ASYNC timings in omap2_onenand_calc_async_timings() are not suitable for n900 for ASYNC mode (too fast for ASYNC mode?). Another problem is we're not really using the DT provided GPMC timings. I don't see a call to gpmc_read_timings_dt() anywhere in the oneNAND path. ASYNC mode *should* have sync_read and sync_write set to false. > > When DT mode contains async timings, lets use them and do not bother with > sync ones. This I agree. If it contains sync timings, then it should be those which > certainly works [*]. OneNAND node can contain something like Optimized SYNC timings will not work for ASYNC mode so I don't see how we can get away without providing ASYNC timings. Which now makes me to wonder if we will really need a way to completely specify both ASYNC and SYNC mode timings for optimal results. see below. > 'ti,onenand-optimize-timings' and driver will attempt to get frequency > and set better timings. This will be done after devce probe, so we know > device indentification and we can print it if obtaining frequency fails. > Also, this way we do not need special handlings for OneNAND nodes. > > [*] As Version ID reg is not described in datasheet, but driver relies on > its content, we should leave on DT node author to make sure sync mode works. > >>> + r = gpmc_omap_onenand_set_sync_timings(&pdev->dev, c->gpmc_cs, freq); >>> + if (r < 0) { >>> + dev_err(&pdev->dev, "Cannot set sync timings: %d\n", r); >> >> right. let's keep this format for all messages. >> >>> + goto err_release_dma; >>> } >>> + if (r > 0) >>> + omap2_onenand_set_cfg(c, true, true, r); >> >> Use the 2 new DT properties to figure out the sr, sw parameters. > > With the above proposal we would need both > ti,onenand-sync-read; > ti,onenand-sync-write; > and > gpmc,sync-read; > gpmc,sync-write; > > Is it really worth it? If you see it as 2 separate complete timing specs, it probably is worth it and solves all our problems. something like this onenand { /* Mandatory ASYNC timings for default mode */ gpmc,rd-cycle-ns = <..>; /* sync-read and sync-write not set */ ... sync_timings@0 { /* Optional SYNC mode optimized timings */ gpmc,sync-clk-ps = <15000>; gpmc,sync-read; gpmc,sync-write; ... } }; We can let the omap-gpmc.c read and program the sync_timings in gpmc_omap_onenand_set_sync_timings(). Also it can verify that if gpmc,sync-clk-ps doesn't fit into the provided freq parameter, we fail. We can do away with magic timing calculations and just relay on DT to provide us the right timings. Migrating current oneNAND users should be easy. Ask them to dump GPMC gpmc_cs_show_timings() after omap2_onenand_calc_async_timings() and after omap2_onenand_calc_async_timings() and we shove those values into the DT. So gpmc_omap_onenand_set_sync_timings(freq, &latency) should reduce to - check if DT provided sync-clk-ps is suitable for provided OneNAND freq else return error code. - gpmc_read_settings_dt(sync_timings_node) - gpmc_read_timings_dt(sync_timings_node) - gpmc_cs_program_settings() - gpmc_cs_set_timings() - &latency = dev_t->cyc_iaa - 1; - return 0 The latency parameter dev_t->cyc_iaa need not be calculated. It can be provided by DT as well. Will need to add a new property though to gpmc DT bindings. > >>> >>> if (c->dma_channel >= 0) { >>> - r = omap_request_dma(0, pdev->dev.driver->name, >>> - omap2_onenand_dma_cb, (void *) c, >>> - &c->dma_channel); >>> - if (r == 0) { >>> - omap_set_dma_write_mode(c->dma_channel, >>> - OMAP_DMA_WRITE_NON_POSTED); >>> - omap_set_dma_src_data_pack(c->dma_channel, 1); >>> - omap_set_dma_src_burst_mode(c->dma_channel, >>> - OMAP_DMA_DATA_BURST_8); >>> - omap_set_dma_dest_data_pack(c->dma_channel, 1); >>> - omap_set_dma_dest_burst_mode(c->dma_channel, >>> - OMAP_DMA_DATA_BURST_8); >>> - } else { >>> - dev_info(&pdev->dev, >>> - "failed to allocate DMA for OneNAND, " >>> - "using PIO instead\n"); >> >> avoid splitting print message. >> >>> - c->dma_channel = -1; >>> - } >>> + omap_set_dma_write_mode(c->dma_channel, >>> + OMAP_DMA_WRITE_NON_POSTED); >>> + omap_set_dma_src_data_pack(c->dma_channel, 1); >>> + omap_set_dma_src_burst_mode(c->dma_channel, >>> + OMAP_DMA_DATA_BURST_8); >>> + omap_set_dma_dest_data_pack(c->dma_channel, 1); >>> + omap_set_dma_dest_burst_mode(c->dma_channel, >>> + OMAP_DMA_DATA_BURST_8); >>> } >>> >>> 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); >>> + c->onenand.base, freq); >>> >>> c->pdev = pdev; >>> c->mtd.priv = &c->onenand; >>> - >>> c->mtd.dev.parent = &pdev->dev; >>> - mtd_set_of_node(&c->mtd, pdata->of_node); >>> - >>> - this = &c->onenand; >>> - if (c->dma_channel >= 0) { >>> - this->wait = omap2_onenand_wait; >>> - if (c->flags & ONENAND_IN_OMAP34XX) { >>> - this->read_bufferram = omap3_onenand_read_bufferram; >>> - this->write_bufferram = omap3_onenand_write_bufferram; >>> - } else { >>> - this->read_bufferram = omap2_onenand_read_bufferram; >>> - this->write_bufferram = omap2_onenand_write_bufferram; >>> - } >>> - } >>> - >>> - if (pdata->regulator_can_sleep) { >>> - c->regulator = regulator_get(&pdev->dev, "vonenand"); >>> - if (IS_ERR(c->regulator)) { >>> - dev_err(&pdev->dev, "Failed to get regulator\n"); >> >> Need to print error code? >> >>> - r = PTR_ERR(c->regulator); >>> - goto err_release_dma; >>> - } >>> - c->onenand.enable = omap2_onenand_enable; >>> - c->onenand.disable = omap2_onenand_disable; >>> - } >>> - >>> - if (pdata->skip_initial_unlocking) >>> - this->options |= ONENAND_SKIP_INITIAL_UNLOCKING; >>> + mtd_set_of_node(&c->mtd, pdev->dev.of_node); >>> >>> if ((r = onenand_scan(&c->mtd, 1)) < 0) >>> - goto err_release_regulator; >>> + goto err_release_dma; >>> >>> - r = mtd_device_register(&c->mtd, pdata ? pdata->parts : NULL, >>> - pdata ? pdata->nr_parts : 0); >>> + r = mtd_device_register(&c->mtd, NULL, 0); >>> if (r) >>> goto err_release_onenand; >>> >>> @@ -754,22 +807,9 @@ static int omap2_onenand_probe(struct platform_device *pdev) >>> >>> err_release_onenand: >>> onenand_release(&c->mtd); >>> -err_release_regulator: >>> - regulator_put(c->regulator); >>> err_release_dma: >>> if (c->dma_channel != -1) >>> omap_free_dma(c->dma_channel); >>> - if (c->gpio_irq) >>> - free_irq(gpio_to_irq(c->gpio_irq), c); >>> -err_release_gpio: >>> - if (c->gpio_irq) >>> - gpio_free(c->gpio_irq); >>> -err_iounmap: >>> - iounmap(c->onenand.base); >>> -err_release_mem_region: >>> - release_mem_region(c->phys_base, c->mem_size); >>> -err_kfree: >>> - kfree(c); >>> >>> return r; >>> } >>> @@ -779,27 +819,43 @@ static int omap2_onenand_remove(struct platform_device *pdev) >>> struct omap2_onenand *c = dev_get_drvdata(&pdev->dev); >>> >>> onenand_release(&c->mtd); >>> - regulator_put(c->regulator); >>> if (c->dma_channel != -1) >>> omap_free_dma(c->dma_channel); >>> omap2_onenand_shutdown(pdev); >>> - if (c->gpio_irq) { >>> - free_irq(gpio_to_irq(c->gpio_irq), c); >>> - gpio_free(c->gpio_irq); >>> - } >>> - iounmap(c->onenand.base); >>> - release_mem_region(c->phys_base, c->mem_size); >>> - kfree(c); >>> >>> return 0; >>> } >>> >>> +static const struct omap2_onenand_devdata omap2_devdata = { >>> + .gpio_quirk = false, >>> + .read_bufferram = omap2_onenand_read_bufferram, >>> + .write_bufferram = omap2_onenand_write_bufferram, >>> +}; >>> + >>> +static const struct omap2_onenand_devdata omap3_devdata = { >>> + .gpio_quirk = true, >>> + .read_bufferram = omap3_onenand_read_bufferram, >>> + .write_bufferram = omap3_onenand_write_bufferram, >>> +}; >>> + >>> +static const struct of_device_id omap2_onenand_ids[] = { >>> + { >>> + .compatible = "ti,omap2-onenand", >>> + .data = &omap2_devdata, >>> + }, { >>> + .compatible = "ti,omap3-onenand", >>> + .data = &omap3_devdata, >>> + }, {}, >>> +}; >>> +MODULE_DEVICE_TABLE(of, omap2_onenand_ids); >>> + >>> static struct platform_driver omap2_onenand_driver = { >>> .probe = omap2_onenand_probe, >>> .remove = omap2_onenand_remove, >>> .shutdown = omap2_onenand_shutdown, >>> .driver = { >>> .name = DRIVER_NAME, >>> + .of_match_table = of_match_ptr(omap2_onenand_ids), >>> }, >>> }; >>> >>> >> >> -- >> cheers, >> -roger >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > Nice to see it moved to the bottom ;-) > Yeah, finally this is resolved and I can sent patches :) > regards, > ladis >
Hi, On Fri, Oct 20, 2017 at 02:52:09PM +0300, Roger Quadros wrote: > Hi, > > On 20/10/17 13:19, Ladislav Michl wrote: > > Hi, > > > > On Fri, Oct 20, 2017 at 10:58:29AM +0300, Roger Quadros wrote: > >> Hi, > >> > >> On 19/10/17 17:45, Ladislav Michl wrote: > >>> On Thu, Oct 19, 2017 at 05:02:51PM +0300, Roger Quadros wrote: > >>>> On 19/10/17 16:42, Ladislav Michl wrote: > >>>>> Hi, > >>>>> > >>>>> On Thu, Oct 19, 2017 at 03:56:30PM +0300, Roger Quadros wrote: > >>> [snip] > >>>>>> how about just "vdd" instead of "vonenand" ? > >>>>> > >>>>> Well, other subsystems are using this naming scheme, ie. vmmc, vwlan, vcpu0, > >>>>> etc. But I do not have any strong preference here. > >>>>> > >>>> > >>>> I see it this way. We already know that the supply is for oneneand since > >>>> the property is in the onenand node. The property must only state the supply name > >>>> e.g. vdd3v3, vdd5v, or just vdd. > >>> > >>> Okay, I'm convinced. > >>> > >>>>> So with regulator-can-sleep property drop, we could simply use: > >>>>> devm_regulator_get_optional(dev, "vonenand"); > >>>> > >>>> right. > >>>>> > >>>>> We still need to privide something to control initial unlocking, see commit > >>>>> c93ff6bf16523d ("mtd: omap: add new variable to platform data to control > >>>>> onenand unlocking"). How ever it not used anywhere, so maybye just drop > >>>>> to be readded later once needed? > >>>>> > >>>> > >>>> And this commit b3dcfd35244e ("mtd: onenand: add new option to control initial > >>>> onenand unlocking") > >>>> > >>>> But none of them explain why exactly it is needed. > >>>> If none of the platforms are using it I'm OK with getting rid of it. > >>> > >>> No, there is no single user. > >>> > >>> So with those changs combined we have so far: > >>> (How to pass parameters to omap2_onenand_set_cfg and how to get timings > >>> right still needs to be investigated) > >>> > >> > >> I think what you are doing currently is fine. See below. > >> > >>> --- > >>> drivers/mtd/onenand/omap2.c | 324 ++++++++++++++++++++++++++------------------ > >>> 1 file changed, 190 insertions(+), 134 deletions(-) > >>> > >>> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c > >>> index 24a1388d3031..83847033510a 100644 > >>> --- a/drivers/mtd/onenand/omap2.c > >>> +++ b/drivers/mtd/onenand/omap2.c > >>> @@ -28,17 +28,18 @@ > >>> #include <linux/mtd/mtd.h> > >>> #include <linux/mtd/onenand.h> > >>> #include <linux/mtd/partitions.h> > >>> +#include <linux/of_device.h> > >>> +#include <linux/omap-gpmc.h> > >>> #include <linux/platform_device.h> > >>> #include <linux/interrupt.h> > >>> #include <linux/delay.h> > >>> #include <linux/dma-mapping.h> > >>> #include <linux/io.h> > >>> #include <linux/slab.h> > >>> +#include <linux/gpio/consumer.h> > >>> #include <linux/regulator/consumer.h> > >>> -#include <linux/gpio.h> > >>> > >>> #include <asm/mach/flash.h> > >>> -#include <linux/platform_data/mtd-onenand-omap2.h> > >>> > >>> #include <linux/omap-dma.h> > >>> > >>> @@ -50,17 +51,22 @@ struct omap2_onenand { > >>> struct platform_device *pdev; > >>> int gpmc_cs; > >>> unsigned long phys_base; > >>> - unsigned int mem_size; > >>> - int gpio_irq; > >>> + struct gpio_desc *gpiod; > >>> struct mtd_info mtd; > >>> struct onenand_chip onenand; > >>> struct completion irq_done; > >>> struct completion dma_done; > >>> int dma_channel; > >>> - int freq; > >>> - int (*setup)(void __iomem *base, int *freq_ptr); > >>> struct regulator *regulator; > >>> - u8 flags; > >>> + bool gpio_quirk; > >>> +}; > >>> + > >>> +struct omap2_onenand_devdata { > >>> + bool gpio_quirk; > >>> + int (*read_bufferram)(struct mtd_info *mtd, int area, > >>> + unsigned char *buffer, int offset, size_t count); > >>> + int (*write_bufferram)(struct mtd_info *mtd, int area, > >>> + const unsigned char *buffer, int offset, size_t count); > >>> }; > >>> > >>> static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) > >>> @@ -90,6 +96,55 @@ static inline void write_reg(struct omap2_onenand *c, unsigned short value, > >>> writew(value, c->onenand.base + reg); > >>> } > >>> > >>> +/* Ensure sync read and sync write are disabled */ > >>> +static void omap2_onenand_set_async_mode(struct omap2_onenand *c) > >>> +{ > >>> + unsigned short reg; > >>> + > >>> + reg = read_reg(c, ONENAND_REG_SYS_CFG1); > >>> + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE; > >>> + write_reg(c, reg, ONENAND_REG_SYS_CFG1); > >>> +} > >>> + > >>> +static void omap2_onenand_set_cfg(struct omap2_onenand *c, > >>> + bool sr, bool sw, int latency) > >>> +{ > >>> + unsigned short reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT; > >>> + > >>> + reg |= (latency << ONENAND_SYS_CFG1_BRL_SHIFT) | > >>> + ONENAND_SYS_CFG1_BL_16; > >>> + if (latency > 5) > >>> + reg |= ONENAND_SYS_CFG1_HF; > >>> + if (latency > 7) > >>> + reg |= ONENAND_SYS_CFG1_VHF; > >>> + if (sr) > >>> + reg |= ONENAND_SYS_CFG1_SYNC_READ; > >>> + if (sw) > >>> + reg |= ONENAND_SYS_CFG1_SYNC_WRITE; > >>> + > >>> + write_reg(c, reg, ONENAND_REG_SYS_CFG1); > >>> +} > >>> + > >>> +static int omap2_onenand_get_freq(struct omap2_onenand *c) > >>> +{ > >>> + unsigned short ver = read_reg(c, ONENAND_REG_VERSION_ID); > >>> + > >>> + switch ((ver >> 4) & 0xf) { > >>> + case 0: > >>> + return 40; > >>> + case 1: > >>> + return 54; > >>> + case 2: > >>> + return 66; > >>> + case 3: > >>> + return 83; > >>> + case 4: > >>> + return 104; > >>> + } > >>> + > >>> + return -ENODEV; > >> > >> Not sure if this is the right error code. > >> The device is there. We just don't support the reported frequency. > > > > We do not know yet device is there, MTG scans for device later. I just kept > > the logic we had before. > > OK, let's keep the logic unchanged. we can improve it separately. > > Just some ideas for later. > > There are 2 things we need to check for. > 1) ASYNC timings could be incorrect so read returns invalid value. To check if that > happened we could read the manufacturer register and if it is invalid, complain about > incorrect gpmc ASYNC settings/timings. > > Fortunately there are only 2 valid manufacturers, so it should be easy. > https://github.com/torvalds/linux/blob/master/include/linux/mtd/onenand.h#L211 > > 2) If manufacturer register was read fine then we assume that frequency register will be > valid. If we don't support the frequency then we say exactly that. Well, I was thinking to move it away from omap2 mtd driver and move it to onenand_base, see reasoning bellow. > > Premiliary datasheet lists VERSION reg yet undefined > > https://www.digchip.com/datasheets/download_datasheet.php?id=1089403&part-number=KFG1G16Q2A > > so not sure what's inside and later datasheets do not describe it either. > > > >>> +} > >>> + > >>> static void wait_err(char *msg, int state, unsigned int ctrl, unsigned int intr) > >>> { > >>> printk(KERN_ERR "onenand_wait: %s! state %d ctrl 0x%04x intr 0x%04x\n", > >>> @@ -153,19 +208,19 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) > >>> if (!(syscfg & ONENAND_SYS_CFG1_IOBE)) { > >>> syscfg |= ONENAND_SYS_CFG1_IOBE; > >>> write_reg(c, syscfg, ONENAND_REG_SYS_CFG1); > >>> - if (c->flags & ONENAND_IN_OMAP34XX) > >>> + if (c->gpio_quirk) > >>> /* Add a delay to let GPIO settle */ > >>> syscfg = read_reg(c, ONENAND_REG_SYS_CFG1); > >>> } > >>> > >>> reinit_completion(&c->irq_done); > >>> - if (c->gpio_irq) { > >>> - result = gpio_get_value(c->gpio_irq); > >>> - if (result == -1) { > >>> + if (c->gpiod) { > >>> + result = gpiod_get_value(c->gpiod); > >>> + 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; > >>> + return result; > >>> } > >>> } else > >>> result = 0; > >>> @@ -609,142 +664,140 @@ static int omap2_onenand_disable(struct mtd_info *mtd) > >>> > >>> static int omap2_onenand_probe(struct platform_device *pdev) > >>> { > >>> - struct omap_onenand_platform_data *pdata; > >>> + u32 val; > >>> + int freq, r; > >>> + unsigned int mem_size; > >>> + struct resource *res; > >>> struct omap2_onenand *c; > >>> struct onenand_chip *this; > >>> - int r; > >>> - struct resource *res; > >>> + const struct omap2_onenand_devdata *devdata; > >>> + struct device *dev = &pdev->dev; > >>> + struct device_node *np = dev->of_node; > >>> + > >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> + if (res == NULL) { > >>> + dev_err(dev, "error getting memory resource\n"); > >>> + return -EINVAL; > >>> + } > >>> > >>> - pdata = dev_get_platdata(&pdev->dev); > >>> - if (pdata == NULL) { > >>> - dev_err(&pdev->dev, "platform data missing\n"); > >>> - return -ENODEV; > >>> + r = of_property_read_u32(np, "reg", &val); > >>> + if (r) { > >>> + dev_err(dev, "reg not found in DT\n"); > >>> + return r; > >>> } > >>> > >>> - c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL); > >>> + c = devm_kzalloc(dev, sizeof(struct omap2_onenand), GFP_KERNEL); > >>> if (!c) > >>> return -ENOMEM; > >>> > >>> init_completion(&c->irq_done); > >>> init_completion(&c->dma_done); > >>> - c->flags = pdata->flags; > >>> - c->gpmc_cs = pdata->cs; > >>> - c->gpio_irq = pdata->gpio_irq; > >>> - c->dma_channel = pdata->dma_channel; > >>> - if (c->dma_channel < 0) { > >>> - /* if -1, don't use DMA */ > >>> - c->gpio_irq = 0; > >>> - } > >>> > >>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> - if (res == NULL) { > >>> - r = -EINVAL; > >>> - dev_err(&pdev->dev, "error getting memory resource\n"); > >>> - goto err_kfree; > >>> - } > >>> + devdata = of_device_get_match_data(dev); > >>> + this = &c->onenand; > >>> > >>> + c->gpmc_cs = val; > >>> + c->dma_channel = -1; > >>> + c->gpio_quirk = devdata->gpio_quirk; > >>> c->phys_base = res->start; > >>> - c->mem_size = resource_size(res); > >>> - > >>> - if (request_mem_region(c->phys_base, c->mem_size, > >>> - pdev->dev.driver->name) == NULL) { > >>> - dev_err(&pdev->dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", > >>> - c->phys_base, c->mem_size); > >>> - r = -EBUSY; > >>> - goto err_kfree; > >>> + > >>> + mem_size = resource_size(res); > >>> + if (devm_request_mem_region(dev, c->phys_base, mem_size, > >>> + dev->driver->name) == NULL) { > >>> + > >>> + dev_err(dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", > >>> + c->phys_base, mem_size); > >>> + return -EBUSY; > >>> } > >>> - c->onenand.base = ioremap(c->phys_base, c->mem_size); > >>> - if (c->onenand.base == NULL) { > >>> - r = -ENOMEM; > >>> - goto err_release_mem_region; > >>> + c->onenand.base = devm_ioremap(dev, c->phys_base, mem_size); > >>> + if (c->onenand.base == NULL) > >>> + return -ENOMEM; > >>> + > >>> + if (!of_property_read_u32(np, "dma-channel", &val)) { > >>> + c->dma_channel = val; > >>> + r = omap_request_dma(0, dev->driver->name, > >>> + omap2_onenand_dma_cb, (void *) c, > >>> + &c->dma_channel); > >>> + if (r) { > >>> + dev_info(dev, > >>> + "failed to allocate DMA for OneNAND, " > >>> + "using PIO instead\n"); > >> Don't split the print message. It is hard to find while grepping during debug. > > > > Will fix all that issues in the next version. > > > >>> + c->dma_channel = -1; > >>> + } > >>> } > >>> > >>> - if (pdata->onenand_setup != NULL) { > >>> - r = pdata->onenand_setup(c->onenand.base, &c->freq); > >>> - if (r < 0) { > >>> - dev_err(&pdev->dev, "Onenand platform setup failed: " > >>> - "%d\n", r); > >>> - goto err_iounmap; > >>> + if (c->dma_channel >= 0) { > >>> + this->wait = omap2_onenand_wait; > >>> + this->read_bufferram = devdata->read_bufferram; > >>> + this->write_bufferram = devdata->write_bufferram; > >>> + > >>> + c->gpiod = devm_gpiod_get(dev, "OneNAND irq", GPIOD_IN); > >>> + if (IS_ERR(c->gpiod)) { > >>> + r = PTR_ERR(c->gpiod); > >>> + /* Just try again if this happens */ > >>> + if (r != -EPROBE_DEFER) > >>> + dev_err(dev, "error getting gpio (%d)\n", r); > >> > >> Use a uniform format > >> "error getting gpio: %d", r: > >> > >> > >>> + goto err_release_dma; > >>> } > >>> - c->setup = pdata->onenand_setup; > >>> + r = devm_request_irq(dev, gpiod_to_irq(c->gpiod), > >>> + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, > >>> + "OneNAND irq", c); > >>> + if (r) > >>> + goto err_release_dma; > >>> + } > >>> + > >>> + c->regulator = devm_regulator_get(dev, "vdd"); As a side note, datasheet mentions two independent Vcc, one for memory itself and one for i/o. > >>> + if (IS_ERR(c->regulator)) { > >>> + r = PTR_ERR(c->regulator); > >>> + dev_err(dev, "failed to get regulator (%d)\n", r); > >> > >> here too. > >> > >>> + goto err_release_dma; > >>> + } > >>> + if (c->regulator) { > >>> + this->enable = omap2_onenand_enable; > >>> + this->disable = omap2_onenand_disable; > >>> } > >>> > >>> - if (c->gpio_irq) { > >>> - if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) { > >>> - dev_err(&pdev->dev, "Failed to request GPIO%d for " > >>> - "OneNAND\n", c->gpio_irq); > >> > >> don't split print message. > >> > >>> - goto err_iounmap; > >>> + omap2_onenand_set_async_mode(c); > >>> + freq = omap2_onenand_get_freq(c); > >>> + if (freq < 0) { > >>> + dev_err(&pdev->dev, > >>> + "Rate not detected, bad GPMC async timings?\n"); > >> > >> Message is misleading. Should be, > >> "Unsupported device. MFR_ID:DEV_ID:VER_ID = %x:%x:%x" > >> > >> We should dump the device ID register in case someone wants to report the error > >> and we need to add support for this new device. Not that it will happen > >> but is just good practice. > > > > Yes, message has historical reasons (same as this splitted lines), but see > > bellow. > > > >>> + r = freq; > >>> + goto err_release_dma; > >>> } > >>> - 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; > >> > >> Is it possible to operate oneNAND completely in ASYNC mode? > >> If so then the below call should be conditional. Only if we need sync_read/write. > > > > According to datasheet and origilal code, yes. I haven't tried yet (away from > > the lab). > > > >> We still need to figure out how to get the read/write sync flags here right? > >> I suggest to add new DT properties for this. > >> e.g. > >> ti,onenand-sync-read; > >> ti,onenand-sync-write; > >> > >> > >> If any one or both are set we use SYNC timings. > > > > What is we change logic a bit here? I still do think those gpmc generic > > properties are sufficient. > > > > Code contains following when setting asyns timings: > > /* > > * Note that we need to keep sync_write set for the call to > > * omap2_onenand_set_async_mode() to work to detect the onenand > > * supported clock rate for the sync timings. > > */ > > s->sync_read = false; > > s->sync_write = true; > > This comes from commit e7b11dc7b77b ("ARM: OMAP2+: Fix onenand rate detection > to avoid filesystem corruption") > > Looks like it was a quick fix. But IMO it is not correct. > The real problem is that the ASYNC timings in omap2_onenand_calc_async_timings() > are not suitable for n900 for ASYNC mode (too fast for ASYNC mode?). Seems reasobale but certainly needs to be verified. Tony? > Another problem is we're not really using the DT provided GPMC timings. > I don't see a call to gpmc_read_timings_dt() anywhere in the oneNAND path. Correct, that's also why I sent simple patch which solved my problem and look what you did with that :) > ASYNC mode *should* have sync_read and sync_write set to false. I tried and it still works. But would be nice to find someone with n900 to verify. > > > > When DT mode contains async timings, lets use them and do not bother with > > sync ones. > > This I agree. > > If it contains sync timings, then it should be those which > > certainly works [*]. OneNAND node can contain something like > > Optimized SYNC timings will not work for ASYNC mode so I don't see how we can get > away without providing ASYNC timings. > > Which now makes me to wonder if we will really need a way to completely specify > both ASYNC and SYNC mode timings for optimal results. see below. > > > 'ti,onenand-optimize-timings' and driver will attempt to get frequency > > and set better timings. This will be done after devce probe, so we know > > device indentification and we can print it if obtaining frequency fails. > > Also, this way we do not need special handlings for OneNAND nodes. > > > > [*] As Version ID reg is not described in datasheet, but driver relies on > > its content, we should leave on DT node author to make sure sync mode works. > > > >>> + r = gpmc_omap_onenand_set_sync_timings(&pdev->dev, c->gpmc_cs, freq); > >>> + if (r < 0) { > >>> + dev_err(&pdev->dev, "Cannot set sync timings: %d\n", r); > >> > >> right. let's keep this format for all messages. > >> > >>> + goto err_release_dma; > >>> } > >>> + if (r > 0) > >>> + omap2_onenand_set_cfg(c, true, true, r); > >> > >> Use the 2 new DT properties to figure out the sr, sw parameters. > > > > With the above proposal we would need both > > ti,onenand-sync-read; > > ti,onenand-sync-write; > > and > > gpmc,sync-read; > > gpmc,sync-write; > > > > Is it really worth it? > > If you see it as 2 separate complete timing specs, it probably is worth it and solves > all our problems. > > something like this > > onenand { > > /* Mandatory ASYNC timings for default mode */ > gpmc,rd-cycle-ns = <..>; > /* sync-read and sync-write not set */ > ... > > sync_timings@0 { > /* Optional SYNC mode optimized timings */ > gpmc,sync-clk-ps = <15000>; > gpmc,sync-read; > gpmc,sync-write; > ... > } > }; > > We can let the omap-gpmc.c read and program the sync_timings in gpmc_omap_onenand_set_sync_timings(). > > Also it can verify that if gpmc,sync-clk-ps doesn't fit into the provided freq parameter, we fail. > > We can do away with magic timing calculations and just relay on DT to provide us the right timings. > Migrating current oneNAND users should be easy. Ask them to dump GPMC gpmc_cs_show_timings() > after omap2_onenand_calc_async_timings() and after omap2_onenand_calc_async_timings() and we > shove those values into the DT. > > So gpmc_omap_onenand_set_sync_timings(freq, &latency) should reduce to > > - check if DT provided sync-clk-ps is suitable for provided OneNAND freq else return error code. > - gpmc_read_settings_dt(sync_timings_node) > - gpmc_read_timings_dt(sync_timings_node) > - gpmc_cs_program_settings() > - gpmc_cs_set_timings() > - &latency = dev_t->cyc_iaa - 1; > - return 0 > > The latency parameter dev_t->cyc_iaa need not be calculated. It can be provided by DT as well. > Will need to add a new property though to gpmc DT bindings. If you look at original code path, async timings were programed and Version ID register read (they should really change its name). Before doing that, sync mode flags were disable in sys_cfg1 reg. And after reading frequency new sync timings was calculated and programmed into gpmc. Chip probe function was called _after_ all that, meaning that gpmc is in sync mode and onenand driver is still able to read manufacturer and product id. Also note that probe function temporarily clears sync mode flags in sys_cfg1 reg. So we should be able to read Version ID reg in probe function as well and provide replacable function to setup timings etc. in onenand_base as this really is not omap specific. Also our omap2_onenand_set_cfg function is not omap2 specific. And finally onenand_sync_read_bufferram seems to enable sync read flag only during bufferram read, while omap2 driver keeps it enabled all the time. Seems we do not need async timings at all as long as sync one are not too tight. Adding Kyungmin Park to cc list... > >>> > >>> if (c->dma_channel >= 0) { > >>> - r = omap_request_dma(0, pdev->dev.driver->name, > >>> - omap2_onenand_dma_cb, (void *) c, > >>> - &c->dma_channel); > >>> - if (r == 0) { > >>> - omap_set_dma_write_mode(c->dma_channel, > >>> - OMAP_DMA_WRITE_NON_POSTED); > >>> - omap_set_dma_src_data_pack(c->dma_channel, 1); > >>> - omap_set_dma_src_burst_mode(c->dma_channel, > >>> - OMAP_DMA_DATA_BURST_8); > >>> - omap_set_dma_dest_data_pack(c->dma_channel, 1); > >>> - omap_set_dma_dest_burst_mode(c->dma_channel, > >>> - OMAP_DMA_DATA_BURST_8); > >>> - } else { > >>> - dev_info(&pdev->dev, > >>> - "failed to allocate DMA for OneNAND, " > >>> - "using PIO instead\n"); > >> > >> avoid splitting print message. > >> > >>> - c->dma_channel = -1; > >>> - } > >>> + omap_set_dma_write_mode(c->dma_channel, > >>> + OMAP_DMA_WRITE_NON_POSTED); > >>> + omap_set_dma_src_data_pack(c->dma_channel, 1); > >>> + omap_set_dma_src_burst_mode(c->dma_channel, > >>> + OMAP_DMA_DATA_BURST_8); > >>> + omap_set_dma_dest_data_pack(c->dma_channel, 1); > >>> + omap_set_dma_dest_burst_mode(c->dma_channel, > >>> + OMAP_DMA_DATA_BURST_8); > >>> } > >>> > >>> 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); > >>> + c->onenand.base, freq); > >>> > >>> c->pdev = pdev; > >>> c->mtd.priv = &c->onenand; > >>> - > >>> c->mtd.dev.parent = &pdev->dev; > >>> - mtd_set_of_node(&c->mtd, pdata->of_node); > >>> - > >>> - this = &c->onenand; > >>> - if (c->dma_channel >= 0) { > >>> - this->wait = omap2_onenand_wait; > >>> - if (c->flags & ONENAND_IN_OMAP34XX) { > >>> - this->read_bufferram = omap3_onenand_read_bufferram; > >>> - this->write_bufferram = omap3_onenand_write_bufferram; > >>> - } else { > >>> - this->read_bufferram = omap2_onenand_read_bufferram; > >>> - this->write_bufferram = omap2_onenand_write_bufferram; > >>> - } > >>> - } > >>> - > >>> - if (pdata->regulator_can_sleep) { > >>> - c->regulator = regulator_get(&pdev->dev, "vonenand"); > >>> - if (IS_ERR(c->regulator)) { > >>> - dev_err(&pdev->dev, "Failed to get regulator\n"); > >> > >> Need to print error code? > >> > >>> - r = PTR_ERR(c->regulator); > >>> - goto err_release_dma; > >>> - } > >>> - c->onenand.enable = omap2_onenand_enable; > >>> - c->onenand.disable = omap2_onenand_disable; > >>> - } > >>> - > >>> - if (pdata->skip_initial_unlocking) > >>> - this->options |= ONENAND_SKIP_INITIAL_UNLOCKING; > >>> + mtd_set_of_node(&c->mtd, pdev->dev.of_node); > >>> > >>> if ((r = onenand_scan(&c->mtd, 1)) < 0) > >>> - goto err_release_regulator; > >>> + goto err_release_dma; > >>> > >>> - r = mtd_device_register(&c->mtd, pdata ? pdata->parts : NULL, > >>> - pdata ? pdata->nr_parts : 0); > >>> + r = mtd_device_register(&c->mtd, NULL, 0); > >>> if (r) > >>> goto err_release_onenand; > >>> > >>> @@ -754,22 +807,9 @@ static int omap2_onenand_probe(struct platform_device *pdev) > >>> > >>> err_release_onenand: > >>> onenand_release(&c->mtd); > >>> -err_release_regulator: > >>> - regulator_put(c->regulator); > >>> err_release_dma: > >>> if (c->dma_channel != -1) > >>> omap_free_dma(c->dma_channel); > >>> - if (c->gpio_irq) > >>> - free_irq(gpio_to_irq(c->gpio_irq), c); > >>> -err_release_gpio: > >>> - if (c->gpio_irq) > >>> - gpio_free(c->gpio_irq); > >>> -err_iounmap: > >>> - iounmap(c->onenand.base); > >>> -err_release_mem_region: > >>> - release_mem_region(c->phys_base, c->mem_size); > >>> -err_kfree: > >>> - kfree(c); > >>> > >>> return r; > >>> } > >>> @@ -779,27 +819,43 @@ static int omap2_onenand_remove(struct platform_device *pdev) > >>> struct omap2_onenand *c = dev_get_drvdata(&pdev->dev); > >>> > >>> onenand_release(&c->mtd); > >>> - regulator_put(c->regulator); > >>> if (c->dma_channel != -1) > >>> omap_free_dma(c->dma_channel); > >>> omap2_onenand_shutdown(pdev); > >>> - if (c->gpio_irq) { > >>> - free_irq(gpio_to_irq(c->gpio_irq), c); > >>> - gpio_free(c->gpio_irq); > >>> - } > >>> - iounmap(c->onenand.base); > >>> - release_mem_region(c->phys_base, c->mem_size); > >>> - kfree(c); > >>> > >>> return 0; > >>> } > >>> > >>> +static const struct omap2_onenand_devdata omap2_devdata = { > >>> + .gpio_quirk = false, > >>> + .read_bufferram = omap2_onenand_read_bufferram, > >>> + .write_bufferram = omap2_onenand_write_bufferram, > >>> +}; > >>> + > >>> +static const struct omap2_onenand_devdata omap3_devdata = { > >>> + .gpio_quirk = true, > >>> + .read_bufferram = omap3_onenand_read_bufferram, > >>> + .write_bufferram = omap3_onenand_write_bufferram, > >>> +}; > >>> + > >>> +static const struct of_device_id omap2_onenand_ids[] = { > >>> + { > >>> + .compatible = "ti,omap2-onenand", > >>> + .data = &omap2_devdata, > >>> + }, { > >>> + .compatible = "ti,omap3-onenand", > >>> + .data = &omap3_devdata, > >>> + }, {}, > >>> +}; > >>> +MODULE_DEVICE_TABLE(of, omap2_onenand_ids); > >>> + > >>> static struct platform_driver omap2_onenand_driver = { > >>> .probe = omap2_onenand_probe, > >>> .remove = omap2_onenand_remove, > >>> .shutdown = omap2_onenand_shutdown, > >>> .driver = { > >>> .name = DRIVER_NAME, > >>> + .of_match_table = of_match_ptr(omap2_onenand_ids), > >>> }, > >>> }; > >>> > >>> > >> > >> -- > >> cheers, > >> -roger > >> > >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > Nice to see it moved to the bottom ;-) > > > > Yeah, finally this is resolved and I can sent patches :) > > > regards, > > ladis > > > > -- > cheers, > -roger > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On 20/10/17 21:19, Ladislav Michl wrote: > Hi, > > On Fri, Oct 20, 2017 at 02:52:09PM +0300, Roger Quadros wrote: >> Hi, >> >> On 20/10/17 13:19, Ladislav Michl wrote: >>> Hi, >>> >>> On Fri, Oct 20, 2017 at 10:58:29AM +0300, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 19/10/17 17:45, Ladislav Michl wrote: >>>>> On Thu, Oct 19, 2017 at 05:02:51PM +0300, Roger Quadros wrote: >>>>>> On 19/10/17 16:42, Ladislav Michl wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Thu, Oct 19, 2017 at 03:56:30PM +0300, Roger Quadros wrote: >>>>> [snip] >>>>>>>> how about just "vdd" instead of "vonenand" ? >>>>>>> >>>>>>> Well, other subsystems are using this naming scheme, ie. vmmc, vwlan, vcpu0, >>>>>>> etc. But I do not have any strong preference here. >>>>>>> >>>>>> >>>>>> I see it this way. We already know that the supply is for oneneand since >>>>>> the property is in the onenand node. The property must only state the supply name >>>>>> e.g. vdd3v3, vdd5v, or just vdd. >>>>> >>>>> Okay, I'm convinced. >>>>> >>>>>>> So with regulator-can-sleep property drop, we could simply use: >>>>>>> devm_regulator_get_optional(dev, "vonenand"); >>>>>> >>>>>> right. >>>>>>> >>>>>>> We still need to privide something to control initial unlocking, see commit >>>>>>> c93ff6bf16523d ("mtd: omap: add new variable to platform data to control >>>>>>> onenand unlocking"). How ever it not used anywhere, so maybye just drop >>>>>>> to be readded later once needed? >>>>>>> >>>>>> >>>>>> And this commit b3dcfd35244e ("mtd: onenand: add new option to control initial >>>>>> onenand unlocking") >>>>>> >>>>>> But none of them explain why exactly it is needed. >>>>>> If none of the platforms are using it I'm OK with getting rid of it. >>>>> >>>>> No, there is no single user. >>>>> >>>>> So with those changs combined we have so far: >>>>> (How to pass parameters to omap2_onenand_set_cfg and how to get timings >>>>> right still needs to be investigated) >>>>> >>>> >>>> I think what you are doing currently is fine. See below. >>>> >>>>> --- >>>>> drivers/mtd/onenand/omap2.c | 324 ++++++++++++++++++++++++++------------------ >>>>> 1 file changed, 190 insertions(+), 134 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c >>>>> index 24a1388d3031..83847033510a 100644 >>>>> --- a/drivers/mtd/onenand/omap2.c >>>>> +++ b/drivers/mtd/onenand/omap2.c >>>>> @@ -28,17 +28,18 @@ >>>>> #include <linux/mtd/mtd.h> >>>>> #include <linux/mtd/onenand.h> >>>>> #include <linux/mtd/partitions.h> >>>>> +#include <linux/of_device.h> >>>>> +#include <linux/omap-gpmc.h> >>>>> #include <linux/platform_device.h> >>>>> #include <linux/interrupt.h> >>>>> #include <linux/delay.h> >>>>> #include <linux/dma-mapping.h> >>>>> #include <linux/io.h> >>>>> #include <linux/slab.h> >>>>> +#include <linux/gpio/consumer.h> >>>>> #include <linux/regulator/consumer.h> >>>>> -#include <linux/gpio.h> >>>>> >>>>> #include <asm/mach/flash.h> >>>>> -#include <linux/platform_data/mtd-onenand-omap2.h> >>>>> >>>>> #include <linux/omap-dma.h> >>>>> >>>>> @@ -50,17 +51,22 @@ struct omap2_onenand { >>>>> struct platform_device *pdev; >>>>> int gpmc_cs; >>>>> unsigned long phys_base; >>>>> - unsigned int mem_size; >>>>> - int gpio_irq; >>>>> + struct gpio_desc *gpiod; >>>>> struct mtd_info mtd; >>>>> struct onenand_chip onenand; >>>>> struct completion irq_done; >>>>> struct completion dma_done; >>>>> int dma_channel; >>>>> - int freq; >>>>> - int (*setup)(void __iomem *base, int *freq_ptr); >>>>> struct regulator *regulator; >>>>> - u8 flags; >>>>> + bool gpio_quirk; >>>>> +}; >>>>> + >>>>> +struct omap2_onenand_devdata { >>>>> + bool gpio_quirk; >>>>> + int (*read_bufferram)(struct mtd_info *mtd, int area, >>>>> + unsigned char *buffer, int offset, size_t count); >>>>> + int (*write_bufferram)(struct mtd_info *mtd, int area, >>>>> + const unsigned char *buffer, int offset, size_t count); >>>>> }; >>>>> >>>>> static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) >>>>> @@ -90,6 +96,55 @@ static inline void write_reg(struct omap2_onenand *c, unsigned short value, >>>>> writew(value, c->onenand.base + reg); >>>>> } >>>>> >>>>> +/* Ensure sync read and sync write are disabled */ >>>>> +static void omap2_onenand_set_async_mode(struct omap2_onenand *c) >>>>> +{ >>>>> + unsigned short reg; >>>>> + >>>>> + reg = read_reg(c, ONENAND_REG_SYS_CFG1); >>>>> + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE; >>>>> + write_reg(c, reg, ONENAND_REG_SYS_CFG1); >>>>> +} >>>>> + >>>>> +static void omap2_onenand_set_cfg(struct omap2_onenand *c, >>>>> + bool sr, bool sw, int latency) >>>>> +{ >>>>> + unsigned short reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT; >>>>> + >>>>> + reg |= (latency << ONENAND_SYS_CFG1_BRL_SHIFT) | >>>>> + ONENAND_SYS_CFG1_BL_16; >>>>> + if (latency > 5) >>>>> + reg |= ONENAND_SYS_CFG1_HF; >>>>> + if (latency > 7) >>>>> + reg |= ONENAND_SYS_CFG1_VHF; >>>>> + if (sr) >>>>> + reg |= ONENAND_SYS_CFG1_SYNC_READ; >>>>> + if (sw) >>>>> + reg |= ONENAND_SYS_CFG1_SYNC_WRITE; >>>>> + >>>>> + write_reg(c, reg, ONENAND_REG_SYS_CFG1); >>>>> +} >>>>> + >>>>> +static int omap2_onenand_get_freq(struct omap2_onenand *c) >>>>> +{ >>>>> + unsigned short ver = read_reg(c, ONENAND_REG_VERSION_ID); >>>>> + >>>>> + switch ((ver >> 4) & 0xf) { >>>>> + case 0: >>>>> + return 40; >>>>> + case 1: >>>>> + return 54; >>>>> + case 2: >>>>> + return 66; >>>>> + case 3: >>>>> + return 83; >>>>> + case 4: >>>>> + return 104; >>>>> + } >>>>> + >>>>> + return -ENODEV; >>>> >>>> Not sure if this is the right error code. >>>> The device is there. We just don't support the reported frequency. >>> >>> We do not know yet device is there, MTG scans for device later. I just kept >>> the logic we had before. >> >> OK, let's keep the logic unchanged. we can improve it separately. >> >> Just some ideas for later. >> >> There are 2 things we need to check for. >> 1) ASYNC timings could be incorrect so read returns invalid value. To check if that >> happened we could read the manufacturer register and if it is invalid, complain about >> incorrect gpmc ASYNC settings/timings. >> >> Fortunately there are only 2 valid manufacturers, so it should be easy. >> https://github.com/torvalds/linux/blob/master/include/linux/mtd/onenand.h#L211 >> >> 2) If manufacturer register was read fine then we assume that frequency register will be >> valid. If we don't support the frequency then we say exactly that. > > Well, I was thinking to move it away from omap2 mtd driver and move it to onenand_base, > see reasoning bellow. > >>> Premiliary datasheet lists VERSION reg yet undefined >>> https://www.digchip.com/datasheets/download_datasheet.php?id=1089403&part-number=KFG1G16Q2A >>> so not sure what's inside and later datasheets do not describe it either. >>> >>>>> +} >>>>> + >>>>> static void wait_err(char *msg, int state, unsigned int ctrl, unsigned int intr) >>>>> { >>>>> printk(KERN_ERR "onenand_wait: %s! state %d ctrl 0x%04x intr 0x%04x\n", >>>>> @@ -153,19 +208,19 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) >>>>> if (!(syscfg & ONENAND_SYS_CFG1_IOBE)) { >>>>> syscfg |= ONENAND_SYS_CFG1_IOBE; >>>>> write_reg(c, syscfg, ONENAND_REG_SYS_CFG1); >>>>> - if (c->flags & ONENAND_IN_OMAP34XX) >>>>> + if (c->gpio_quirk) >>>>> /* Add a delay to let GPIO settle */ >>>>> syscfg = read_reg(c, ONENAND_REG_SYS_CFG1); >>>>> } >>>>> >>>>> reinit_completion(&c->irq_done); >>>>> - if (c->gpio_irq) { >>>>> - result = gpio_get_value(c->gpio_irq); >>>>> - if (result == -1) { >>>>> + if (c->gpiod) { >>>>> + result = gpiod_get_value(c->gpiod); >>>>> + 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; >>>>> + return result; >>>>> } >>>>> } else >>>>> result = 0; >>>>> @@ -609,142 +664,140 @@ static int omap2_onenand_disable(struct mtd_info *mtd) >>>>> >>>>> static int omap2_onenand_probe(struct platform_device *pdev) >>>>> { >>>>> - struct omap_onenand_platform_data *pdata; >>>>> + u32 val; >>>>> + int freq, r; >>>>> + unsigned int mem_size; >>>>> + struct resource *res; >>>>> struct omap2_onenand *c; >>>>> struct onenand_chip *this; >>>>> - int r; >>>>> - struct resource *res; >>>>> + const struct omap2_onenand_devdata *devdata; >>>>> + struct device *dev = &pdev->dev; >>>>> + struct device_node *np = dev->of_node; >>>>> + >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + if (res == NULL) { >>>>> + dev_err(dev, "error getting memory resource\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> >>>>> - pdata = dev_get_platdata(&pdev->dev); >>>>> - if (pdata == NULL) { >>>>> - dev_err(&pdev->dev, "platform data missing\n"); >>>>> - return -ENODEV; >>>>> + r = of_property_read_u32(np, "reg", &val); >>>>> + if (r) { >>>>> + dev_err(dev, "reg not found in DT\n"); >>>>> + return r; >>>>> } >>>>> >>>>> - c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL); >>>>> + c = devm_kzalloc(dev, sizeof(struct omap2_onenand), GFP_KERNEL); >>>>> if (!c) >>>>> return -ENOMEM; >>>>> >>>>> init_completion(&c->irq_done); >>>>> init_completion(&c->dma_done); >>>>> - c->flags = pdata->flags; >>>>> - c->gpmc_cs = pdata->cs; >>>>> - c->gpio_irq = pdata->gpio_irq; >>>>> - c->dma_channel = pdata->dma_channel; >>>>> - if (c->dma_channel < 0) { >>>>> - /* if -1, don't use DMA */ >>>>> - c->gpio_irq = 0; >>>>> - } >>>>> >>>>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> - if (res == NULL) { >>>>> - r = -EINVAL; >>>>> - dev_err(&pdev->dev, "error getting memory resource\n"); >>>>> - goto err_kfree; >>>>> - } >>>>> + devdata = of_device_get_match_data(dev); >>>>> + this = &c->onenand; >>>>> >>>>> + c->gpmc_cs = val; >>>>> + c->dma_channel = -1; >>>>> + c->gpio_quirk = devdata->gpio_quirk; >>>>> c->phys_base = res->start; >>>>> - c->mem_size = resource_size(res); >>>>> - >>>>> - if (request_mem_region(c->phys_base, c->mem_size, >>>>> - pdev->dev.driver->name) == NULL) { >>>>> - dev_err(&pdev->dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", >>>>> - c->phys_base, c->mem_size); >>>>> - r = -EBUSY; >>>>> - goto err_kfree; >>>>> + >>>>> + mem_size = resource_size(res); >>>>> + if (devm_request_mem_region(dev, c->phys_base, mem_size, >>>>> + dev->driver->name) == NULL) { >>>>> + >>>>> + dev_err(dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", >>>>> + c->phys_base, mem_size); >>>>> + return -EBUSY; >>>>> } >>>>> - c->onenand.base = ioremap(c->phys_base, c->mem_size); >>>>> - if (c->onenand.base == NULL) { >>>>> - r = -ENOMEM; >>>>> - goto err_release_mem_region; >>>>> + c->onenand.base = devm_ioremap(dev, c->phys_base, mem_size); >>>>> + if (c->onenand.base == NULL) >>>>> + return -ENOMEM; >>>>> + >>>>> + if (!of_property_read_u32(np, "dma-channel", &val)) { >>>>> + c->dma_channel = val; >>>>> + r = omap_request_dma(0, dev->driver->name, >>>>> + omap2_onenand_dma_cb, (void *) c, >>>>> + &c->dma_channel); >>>>> + if (r) { >>>>> + dev_info(dev, >>>>> + "failed to allocate DMA for OneNAND, " >>>>> + "using PIO instead\n"); >>>> Don't split the print message. It is hard to find while grepping during debug. >>> >>> Will fix all that issues in the next version. >>> >>>>> + c->dma_channel = -1; >>>>> + } >>>>> } >>>>> >>>>> - if (pdata->onenand_setup != NULL) { >>>>> - r = pdata->onenand_setup(c->onenand.base, &c->freq); >>>>> - if (r < 0) { >>>>> - dev_err(&pdev->dev, "Onenand platform setup failed: " >>>>> - "%d\n", r); >>>>> - goto err_iounmap; >>>>> + if (c->dma_channel >= 0) { >>>>> + this->wait = omap2_onenand_wait; >>>>> + this->read_bufferram = devdata->read_bufferram; >>>>> + this->write_bufferram = devdata->write_bufferram; >>>>> + >>>>> + c->gpiod = devm_gpiod_get(dev, "OneNAND irq", GPIOD_IN); >>>>> + if (IS_ERR(c->gpiod)) { >>>>> + r = PTR_ERR(c->gpiod); >>>>> + /* Just try again if this happens */ >>>>> + if (r != -EPROBE_DEFER) >>>>> + dev_err(dev, "error getting gpio (%d)\n", r); >>>> >>>> Use a uniform format >>>> "error getting gpio: %d", r: >>>> >>>> >>>>> + goto err_release_dma; >>>>> } >>>>> - c->setup = pdata->onenand_setup; >>>>> + r = devm_request_irq(dev, gpiod_to_irq(c->gpiod), >>>>> + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, >>>>> + "OneNAND irq", c); >>>>> + if (r) >>>>> + goto err_release_dma; >>>>> + } >>>>> + >>>>> + c->regulator = devm_regulator_get(dev, "vdd"); > > As a side note, datasheet mentions two independent Vcc, one for memory > itself and one for i/o. Actually getting these regulators is not OMAP specific either. Could be done in onenand_base, before reading the ID registers. Both regulators can be optional. So I'm ok if we get rid of regulator code if no OMAP users are affected. > >>>>> + if (IS_ERR(c->regulator)) { >>>>> + r = PTR_ERR(c->regulator); >>>>> + dev_err(dev, "failed to get regulator (%d)\n", r); >>>> >>>> here too. >>>> >>>>> + goto err_release_dma; >>>>> + } >>>>> + if (c->regulator) { >>>>> + this->enable = omap2_onenand_enable; >>>>> + this->disable = omap2_onenand_disable; >>>>> } >>>>> >>>>> - if (c->gpio_irq) { >>>>> - if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) { >>>>> - dev_err(&pdev->dev, "Failed to request GPIO%d for " >>>>> - "OneNAND\n", c->gpio_irq); >>>> >>>> don't split print message. >>>> >>>>> - goto err_iounmap; >>>>> + omap2_onenand_set_async_mode(c); >>>>> + freq = omap2_onenand_get_freq(c); >>>>> + if (freq < 0) { >>>>> + dev_err(&pdev->dev, >>>>> + "Rate not detected, bad GPMC async timings?\n"); >>>> >>>> Message is misleading. Should be, >>>> "Unsupported device. MFR_ID:DEV_ID:VER_ID = %x:%x:%x" >>>> >>>> We should dump the device ID register in case someone wants to report the error >>>> and we need to add support for this new device. Not that it will happen >>>> but is just good practice. >>> >>> Yes, message has historical reasons (same as this splitted lines), but see >>> bellow. >>> >>>>> + r = freq; >>>>> + goto err_release_dma; >>>>> } >>>>> - 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; >>>> >>>> Is it possible to operate oneNAND completely in ASYNC mode? >>>> If so then the below call should be conditional. Only if we need sync_read/write. >>> >>> According to datasheet and origilal code, yes. I haven't tried yet (away from >>> the lab). >>> >>>> We still need to figure out how to get the read/write sync flags here right? >>>> I suggest to add new DT properties for this. >>>> e.g. >>>> ti,onenand-sync-read; >>>> ti,onenand-sync-write; >>>> >>>> >>>> If any one or both are set we use SYNC timings. >>> >>> What is we change logic a bit here? I still do think those gpmc generic >>> properties are sufficient. >>> >>> Code contains following when setting asyns timings: >>> /* >>> * Note that we need to keep sync_write set for the call to >>> * omap2_onenand_set_async_mode() to work to detect the onenand >>> * supported clock rate for the sync timings. >>> */ >>> s->sync_read = false; >>> s->sync_write = true; >> >> This comes from commit e7b11dc7b77b ("ARM: OMAP2+: Fix onenand rate detection >> to avoid filesystem corruption") >> >> Looks like it was a quick fix. But IMO it is not correct. >> The real problem is that the ASYNC timings in omap2_onenand_calc_async_timings() >> are not suitable for n900 for ASYNC mode (too fast for ASYNC mode?). > > Seems reasobale but certainly needs to be verified. Tony? > >> Another problem is we're not really using the DT provided GPMC timings. >> I don't see a call to gpmc_read_timings_dt() anywhere in the oneNAND path. > > Correct, that's also why I sent simple patch which solved my problem and look > what you did with that :) > >> ASYNC mode *should* have sync_read and sync_write set to false. > > I tried and it still works. But would be nice to find someone with n900 to verify. > Tony can help us with that. >>> >>> When DT mode contains async timings, lets use them and do not bother with >>> sync ones. >> >> This I agree. >> >> If it contains sync timings, then it should be those which >>> certainly works [*]. OneNAND node can contain something like >> >> Optimized SYNC timings will not work for ASYNC mode so I don't see how we can get >> away without providing ASYNC timings. >> >> Which now makes me to wonder if we will really need a way to completely specify >> both ASYNC and SYNC mode timings for optimal results. see below. >> >>> 'ti,onenand-optimize-timings' and driver will attempt to get frequency >>> and set better timings. This will be done after devce probe, so we know >>> device indentification and we can print it if obtaining frequency fails. >>> Also, this way we do not need special handlings for OneNAND nodes. >>> >>> [*] As Version ID reg is not described in datasheet, but driver relies on >>> its content, we should leave on DT node author to make sure sync mode works. >>> >>>>> + r = gpmc_omap_onenand_set_sync_timings(&pdev->dev, c->gpmc_cs, freq); >>>>> + if (r < 0) { >>>>> + dev_err(&pdev->dev, "Cannot set sync timings: %d\n", r); >>>> >>>> right. let's keep this format for all messages. >>>> >>>>> + goto err_release_dma; >>>>> } >>>>> + if (r > 0) >>>>> + omap2_onenand_set_cfg(c, true, true, r); >>>> >>>> Use the 2 new DT properties to figure out the sr, sw parameters. >>> >>> With the above proposal we would need both >>> ti,onenand-sync-read; >>> ti,onenand-sync-write; >>> and >>> gpmc,sync-read; >>> gpmc,sync-write; >>> >>> Is it really worth it? >> >> If you see it as 2 separate complete timing specs, it probably is worth it and solves >> all our problems. >> >> something like this >> >> onenand { >> >> /* Mandatory ASYNC timings for default mode */ >> gpmc,rd-cycle-ns = <..>; >> /* sync-read and sync-write not set */ >> ... >> >> sync_timings@0 { >> /* Optional SYNC mode optimized timings */ >> gpmc,sync-clk-ps = <15000>; >> gpmc,sync-read; >> gpmc,sync-write; >> ... >> } >> }; >> >> We can let the omap-gpmc.c read and program the sync_timings in gpmc_omap_onenand_set_sync_timings(). >> >> Also it can verify that if gpmc,sync-clk-ps doesn't fit into the provided freq parameter, we fail. >> >> We can do away with magic timing calculations and just relay on DT to provide us the right timings. >> Migrating current oneNAND users should be easy. Ask them to dump GPMC gpmc_cs_show_timings() >> after omap2_onenand_calc_async_timings() and after omap2_onenand_calc_async_timings() and we >> shove those values into the DT. >> >> So gpmc_omap_onenand_set_sync_timings(freq, &latency) should reduce to >> >> - check if DT provided sync-clk-ps is suitable for provided OneNAND freq else return error code. >> - gpmc_read_settings_dt(sync_timings_node) >> - gpmc_read_timings_dt(sync_timings_node) >> - gpmc_cs_program_settings() >> - gpmc_cs_set_timings() >> - &latency = dev_t->cyc_iaa - 1; >> - return 0 >> >> The latency parameter dev_t->cyc_iaa need not be calculated. It can be provided by DT as well. >> Will need to add a new property though to gpmc DT bindings. > > If you look at original code path, async timings were programed and Version ID > register read (they should really change its name). Before doing that, sync > mode flags were disable in sys_cfg1 reg. And after reading frequency new sync > timings was calculated and programmed into gpmc. Chip probe function was called > _after_ all that, meaning that gpmc is in sync mode and onenand driver is still > able to read manufacturer and product id. Also note that probe function > temporarily clears sync mode flags in sys_cfg1 reg. > > So we should be able to read Version ID reg in probe function as well and > provide replacable function to setup timings etc. in onenand_base as this > really is not omap specific. Also our omap2_onenand_set_cfg function is not > omap2 specific. > > And finally onenand_sync_read_bufferram seems to enable sync read flag only > during bufferram read, while omap2 driver keeps it enabled all the time. > > Seems we do not need async timings at all as long as sync one are not too tight. Yeah, if we assume that DT will provide the right *max* frequency for the oneNAND, we can skip the ASYNC programming altogether. On the other hand. if DT provides only ASYNC timings, we continue to use only that. This simplifies things a lot. Only one Timing set in DT. > > Adding Kyungmin Park to cc list... > <snip>
diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c index 24a1388d3031..83847033510a 100644 --- a/drivers/mtd/onenand/omap2.c +++ b/drivers/mtd/onenand/omap2.c @@ -28,17 +28,18 @@ #include <linux/mtd/mtd.h> #include <linux/mtd/onenand.h> #include <linux/mtd/partitions.h> +#include <linux/of_device.h> +#include <linux/omap-gpmc.h> #include <linux/platform_device.h> #include <linux/interrupt.h> #include <linux/delay.h> #include <linux/dma-mapping.h> #include <linux/io.h> #include <linux/slab.h> +#include <linux/gpio/consumer.h> #include <linux/regulator/consumer.h> -#include <linux/gpio.h> #include <asm/mach/flash.h> -#include <linux/platform_data/mtd-onenand-omap2.h> #include <linux/omap-dma.h> @@ -50,17 +51,22 @@ struct omap2_onenand { struct platform_device *pdev; int gpmc_cs; unsigned long phys_base; - unsigned int mem_size; - int gpio_irq; + struct gpio_desc *gpiod; struct mtd_info mtd; struct onenand_chip onenand; struct completion irq_done; struct completion dma_done; int dma_channel; - int freq; - int (*setup)(void __iomem *base, int *freq_ptr); struct regulator *regulator; - u8 flags; + bool gpio_quirk; +}; + +struct omap2_onenand_devdata { + bool gpio_quirk; + int (*read_bufferram)(struct mtd_info *mtd, int area, + unsigned char *buffer, int offset, size_t count); + int (*write_bufferram)(struct mtd_info *mtd, int area, + const unsigned char *buffer, int offset, size_t count); }; static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) @@ -90,6 +96,55 @@ static inline void write_reg(struct omap2_onenand *c, unsigned short value, writew(value, c->onenand.base + reg); } +/* Ensure sync read and sync write are disabled */ +static void omap2_onenand_set_async_mode(struct omap2_onenand *c) +{ + unsigned short reg; + + reg = read_reg(c, ONENAND_REG_SYS_CFG1); + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE; + write_reg(c, reg, ONENAND_REG_SYS_CFG1); +} + +static void omap2_onenand_set_cfg(struct omap2_onenand *c, + bool sr, bool sw, int latency) +{ + unsigned short reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT; + + reg |= (latency << ONENAND_SYS_CFG1_BRL_SHIFT) | + ONENAND_SYS_CFG1_BL_16; + if (latency > 5) + reg |= ONENAND_SYS_CFG1_HF; + if (latency > 7) + reg |= ONENAND_SYS_CFG1_VHF; + if (sr) + reg |= ONENAND_SYS_CFG1_SYNC_READ; + if (sw) + reg |= ONENAND_SYS_CFG1_SYNC_WRITE; + + write_reg(c, reg, ONENAND_REG_SYS_CFG1); +} + +static int omap2_onenand_get_freq(struct omap2_onenand *c) +{ + unsigned short ver = read_reg(c, ONENAND_REG_VERSION_ID); + + switch ((ver >> 4) & 0xf) { + case 0: + return 40; + case 1: + return 54; + case 2: + return 66; + case 3: + return 83; + case 4: + return 104; + } + + return -ENODEV; +} + static void wait_err(char *msg, int state, unsigned int ctrl, unsigned int intr) { printk(KERN_ERR "onenand_wait: %s! state %d ctrl 0x%04x intr 0x%04x\n", @@ -153,19 +208,19 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state) if (!(syscfg & ONENAND_SYS_CFG1_IOBE)) { syscfg |= ONENAND_SYS_CFG1_IOBE; write_reg(c, syscfg, ONENAND_REG_SYS_CFG1); - if (c->flags & ONENAND_IN_OMAP34XX) + if (c->gpio_quirk) /* Add a delay to let GPIO settle */ syscfg = read_reg(c, ONENAND_REG_SYS_CFG1); } reinit_completion(&c->irq_done); - if (c->gpio_irq) { - result = gpio_get_value(c->gpio_irq); - if (result == -1) { + if (c->gpiod) { + result = gpiod_get_value(c->gpiod); + 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; + return result; } } else result = 0; @@ -609,142 +664,140 @@ static int omap2_onenand_disable(struct mtd_info *mtd) static int omap2_onenand_probe(struct platform_device *pdev) { - struct omap_onenand_platform_data *pdata; + u32 val; + int freq, r; + unsigned int mem_size; + struct resource *res; struct omap2_onenand *c; struct onenand_chip *this; - int r; - struct resource *res; + const struct omap2_onenand_devdata *devdata; + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (res == NULL) { + dev_err(dev, "error getting memory resource\n"); + return -EINVAL; + } - pdata = dev_get_platdata(&pdev->dev); - if (pdata == NULL) { - dev_err(&pdev->dev, "platform data missing\n"); - return -ENODEV; + r = of_property_read_u32(np, "reg", &val); + if (r) { + dev_err(dev, "reg not found in DT\n"); + return r; } - c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL); + c = devm_kzalloc(dev, sizeof(struct omap2_onenand), GFP_KERNEL); if (!c) return -ENOMEM; init_completion(&c->irq_done); init_completion(&c->dma_done); - c->flags = pdata->flags; - c->gpmc_cs = pdata->cs; - c->gpio_irq = pdata->gpio_irq; - c->dma_channel = pdata->dma_channel; - if (c->dma_channel < 0) { - /* if -1, don't use DMA */ - c->gpio_irq = 0; - } - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (res == NULL) { - r = -EINVAL; - dev_err(&pdev->dev, "error getting memory resource\n"); - goto err_kfree; - } + devdata = of_device_get_match_data(dev); + this = &c->onenand; + c->gpmc_cs = val; + c->dma_channel = -1; + c->gpio_quirk = devdata->gpio_quirk; c->phys_base = res->start; - c->mem_size = resource_size(res); - - if (request_mem_region(c->phys_base, c->mem_size, - pdev->dev.driver->name) == NULL) { - dev_err(&pdev->dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", - c->phys_base, c->mem_size); - r = -EBUSY; - goto err_kfree; + + mem_size = resource_size(res); + if (devm_request_mem_region(dev, c->phys_base, mem_size, + dev->driver->name) == NULL) { + + dev_err(dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n", + c->phys_base, mem_size); + return -EBUSY; } - c->onenand.base = ioremap(c->phys_base, c->mem_size); - if (c->onenand.base == NULL) { - r = -ENOMEM; - goto err_release_mem_region; + c->onenand.base = devm_ioremap(dev, c->phys_base, mem_size); + if (c->onenand.base == NULL) + return -ENOMEM; + + if (!of_property_read_u32(np, "dma-channel", &val)) { + c->dma_channel = val; + r = omap_request_dma(0, dev->driver->name, + omap2_onenand_dma_cb, (void *) c, + &c->dma_channel); + if (r) { + dev_info(dev, + "failed to allocate DMA for OneNAND, " + "using PIO instead\n"); + c->dma_channel = -1; + } } - if (pdata->onenand_setup != NULL) { - r = pdata->onenand_setup(c->onenand.base, &c->freq); - if (r < 0) { - dev_err(&pdev->dev, "Onenand platform setup failed: " - "%d\n", r); - goto err_iounmap; + if (c->dma_channel >= 0) { + this->wait = omap2_onenand_wait; + this->read_bufferram = devdata->read_bufferram; + this->write_bufferram = devdata->write_bufferram; + + c->gpiod = devm_gpiod_get(dev, "OneNAND irq", GPIOD_IN); + if (IS_ERR(c->gpiod)) { + r = PTR_ERR(c->gpiod); + /* Just try again if this happens */ + if (r != -EPROBE_DEFER) + dev_err(dev, "error getting gpio (%d)\n", r); + goto err_release_dma; } - c->setup = pdata->onenand_setup; + r = devm_request_irq(dev, gpiod_to_irq(c->gpiod), + omap2_onenand_interrupt, IRQF_TRIGGER_RISING, + "OneNAND irq", c); + if (r) + goto err_release_dma; + } + + c->regulator = devm_regulator_get(dev, "vdd"); + if (IS_ERR(c->regulator)) { + r = PTR_ERR(c->regulator); + dev_err(dev, "failed to get regulator (%d)\n", r); + goto err_release_dma; + } + if (c->regulator) { + this->enable = omap2_onenand_enable; + this->disable = omap2_onenand_disable; } - if (c->gpio_irq) { - if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) { - dev_err(&pdev->dev, "Failed to request GPIO%d for " - "OneNAND\n", c->gpio_irq); - goto err_iounmap; + omap2_onenand_set_async_mode(c); + freq = omap2_onenand_get_freq(c); + if (freq < 0) { + dev_err(&pdev->dev, + "Rate not detected, bad GPMC async timings?\n"); + r = freq; + goto err_release_dma; } - 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; + r = gpmc_omap_onenand_set_sync_timings(&pdev->dev, c->gpmc_cs, freq); + if (r < 0) { + dev_err(&pdev->dev, "Cannot set sync timings: %d\n", r); + goto err_release_dma; } + if (r > 0) + omap2_onenand_set_cfg(c, true, true, r); if (c->dma_channel >= 0) { - r = omap_request_dma(0, pdev->dev.driver->name, - omap2_onenand_dma_cb, (void *) c, - &c->dma_channel); - if (r == 0) { - omap_set_dma_write_mode(c->dma_channel, - OMAP_DMA_WRITE_NON_POSTED); - omap_set_dma_src_data_pack(c->dma_channel, 1); - omap_set_dma_src_burst_mode(c->dma_channel, - OMAP_DMA_DATA_BURST_8); - omap_set_dma_dest_data_pack(c->dma_channel, 1); - omap_set_dma_dest_burst_mode(c->dma_channel, - OMAP_DMA_DATA_BURST_8); - } else { - dev_info(&pdev->dev, - "failed to allocate DMA for OneNAND, " - "using PIO instead\n"); - c->dma_channel = -1; - } + omap_set_dma_write_mode(c->dma_channel, + OMAP_DMA_WRITE_NON_POSTED); + omap_set_dma_src_data_pack(c->dma_channel, 1); + omap_set_dma_src_burst_mode(c->dma_channel, + OMAP_DMA_DATA_BURST_8); + omap_set_dma_dest_data_pack(c->dma_channel, 1); + omap_set_dma_dest_burst_mode(c->dma_channel, + OMAP_DMA_DATA_BURST_8); } 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); + c->onenand.base, freq); c->pdev = pdev; c->mtd.priv = &c->onenand; - c->mtd.dev.parent = &pdev->dev; - mtd_set_of_node(&c->mtd, pdata->of_node); - - this = &c->onenand; - if (c->dma_channel >= 0) { - this->wait = omap2_onenand_wait; - if (c->flags & ONENAND_IN_OMAP34XX) { - this->read_bufferram = omap3_onenand_read_bufferram; - this->write_bufferram = omap3_onenand_write_bufferram; - } else { - this->read_bufferram = omap2_onenand_read_bufferram; - this->write_bufferram = omap2_onenand_write_bufferram; - } - } - - if (pdata->regulator_can_sleep) { - c->regulator = regulator_get(&pdev->dev, "vonenand"); - if (IS_ERR(c->regulator)) { - dev_err(&pdev->dev, "Failed to get regulator\n"); - r = PTR_ERR(c->regulator); - goto err_release_dma; - } - c->onenand.enable = omap2_onenand_enable; - c->onenand.disable = omap2_onenand_disable; - } - - if (pdata->skip_initial_unlocking) - this->options |= ONENAND_SKIP_INITIAL_UNLOCKING; + mtd_set_of_node(&c->mtd, pdev->dev.of_node); if ((r = onenand_scan(&c->mtd, 1)) < 0) - goto err_release_regulator; + goto err_release_dma; - r = mtd_device_register(&c->mtd, pdata ? pdata->parts : NULL, - pdata ? pdata->nr_parts : 0); + r = mtd_device_register(&c->mtd, NULL, 0); if (r) goto err_release_onenand; @@ -754,22 +807,9 @@ static int omap2_onenand_probe(struct platform_device *pdev) err_release_onenand: onenand_release(&c->mtd); -err_release_regulator: - regulator_put(c->regulator); err_release_dma: if (c->dma_channel != -1) omap_free_dma(c->dma_channel); - if (c->gpio_irq) - free_irq(gpio_to_irq(c->gpio_irq), c); -err_release_gpio: - if (c->gpio_irq) - gpio_free(c->gpio_irq); -err_iounmap: - iounmap(c->onenand.base); -err_release_mem_region: - release_mem_region(c->phys_base, c->mem_size); -err_kfree: - kfree(c); return r; } @@ -779,27 +819,43 @@ static int omap2_onenand_remove(struct platform_device *pdev) struct omap2_onenand *c = dev_get_drvdata(&pdev->dev); onenand_release(&c->mtd); - regulator_put(c->regulator); if (c->dma_channel != -1) omap_free_dma(c->dma_channel); omap2_onenand_shutdown(pdev); - if (c->gpio_irq) { - free_irq(gpio_to_irq(c->gpio_irq), c); - gpio_free(c->gpio_irq); - } - iounmap(c->onenand.base); - release_mem_region(c->phys_base, c->mem_size); - kfree(c); return 0; } +static const struct omap2_onenand_devdata omap2_devdata = { + .gpio_quirk = false, + .read_bufferram = omap2_onenand_read_bufferram, + .write_bufferram = omap2_onenand_write_bufferram, +}; + +static const struct omap2_onenand_devdata omap3_devdata = { + .gpio_quirk = true, + .read_bufferram = omap3_onenand_read_bufferram, + .write_bufferram = omap3_onenand_write_bufferram, +}; + +static const struct of_device_id omap2_onenand_ids[] = { + { + .compatible = "ti,omap2-onenand", + .data = &omap2_devdata, + }, { + .compatible = "ti,omap3-onenand", + .data = &omap3_devdata, + }, {}, +}; +MODULE_DEVICE_TABLE(of, omap2_onenand_ids); + static struct platform_driver omap2_onenand_driver = { .probe = omap2_onenand_probe, .remove = omap2_onenand_remove, .shutdown = omap2_onenand_shutdown, .driver = { .name = DRIVER_NAME, + .of_match_table = of_match_ptr(omap2_onenand_ids), }, };