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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
