On 19/10/17 16:42, Ladislav Michl wrote:
> 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.
>
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.
> 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.
>>> 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
