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),
> },
> };
>
>
--
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
