Hi,
On Thu, Oct 19, 2017 at 03:56:30PM +0300, Roger Quadros wrote:
> Hi,
>
> On 17/10/17 18:03, Ladislav Michl wrote:
[snip]
> > Before describing new binding, we should agree, how it will look like.
> > Bellow is semi-v2 with GPIO, regulator and ONENAND_SKIP_INITIAL_UNLOCKING
> > done - somehow.
> > GPIO is pretty usual, unlocking could be probably defined at common
> > onenand level and regulator should be probably also described in DT.
> >
> > Ideas?
>
> Looks good to me. One question below.
>
> >
> > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
> > index 24a1388d3031..7ab56383ff7a 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,143 @@ 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;
> > - }
> > - c->onenand.base = ioremap(c->phys_base, c->mem_size);
> > - if (c->onenand.base == NULL) {
> > - r = -ENOMEM;
> > - goto err_release_mem_region;
> > - }
> >
> > - 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;
> > - }
> > - c->setup = pdata->onenand_setup;
> > - }
> > + mem_size = resource_size(res);
> > + if (devm_request_mem_region(dev, c->phys_base, mem_size,
> > + dev->driver->name) == NULL) {
> >
> > - 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;
> > - }
> > - 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;
> > + dev_err(dev, "Cannot reserve memory region at 0x%08lx, size: 0x%x\n",
> > + c->phys_base, mem_size);
> > + return -EBUSY;
> > }
> > + c->onenand.base = devm_ioremap(dev, c->phys_base, mem_size);
> > + if (c->onenand.base == NULL)
> > + return -ENOMEM;
> >
> > - 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,
> > + 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;
> > }
> > }
> >
> > - 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->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;
> > + 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;
> > }
> > + 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;
> > }
> >
> > - if (pdata->regulator_can_sleep) {
> > - c->regulator = regulator_get(&pdev->dev, "vonenand");
> > + if (of_property_read_bool(np, "skip-initial-unlocking"))
> > + this->options |= ONENAND_SKIP_INITIAL_UNLOCKING;
> > +
> > + if (of_property_read_bool(np, "regulator-can-sleep")) {
>
> why do we need "regulator-can-sleep" property?
Comes from commit 9ac4e613a88d ("mtd: OneNAND: OMAP2/3: prevent regulator
sleeping while OneNAND is in use"). Now looking at it, we do not need such
property, just a regulator to use - driver will use regulator if there is
regulator consumer reference in onenand node.
> > + c->regulator = devm_regulator_get(dev, "vonenand");
>
> 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.
So with regulator-can-sleep property drop, we could simply use:
devm_regulator_get_optional(dev, "vonenand");
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?
> > if (IS_ERR(c->regulator)) {
> > - dev_err(&pdev->dev, "Failed to get regulator\n");
> > r = PTR_ERR(c->regulator);
> > + dev_err(dev, "failed to get regulator (%d)\n", r);
> > goto err_release_dma;
> > }
> > - c->onenand.enable = omap2_onenand_enable;
> > - c->onenand.disable = omap2_onenand_disable;
> > + this->enable = omap2_onenand_enable;
> > + this->disable = omap2_onenand_disable;
> > }
> >
> > - if (pdata->skip_initial_unlocking)
> > - this->options |= ONENAND_SKIP_INITIAL_UNLOCKING;
> > + init_completion(&c->irq_done);
> > + init_completion(&c->dma_done);
> > +
> > + 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;
> > + }
> > +
> > + 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) {
> > + 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, freq);
> > +
> > + c->pdev = pdev;
> > + c->mtd.priv = &c->onenand;
> > + c->mtd.dev.parent = &pdev->dev;
> > + 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 +810,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 +822,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
--
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
