diff mbox

[09/11] mtd: onenand: omap2: Configure driver from DT

Message ID 20171015232214.ouj4j5pebuvlobvo@lenoch (mailing list archive)
State New, archived
Headers show

Commit Message

Ladislav Michl Oct. 15, 2017, 11:22 p.m. UTC
Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 drivers/mtd/onenand/omap2.c | 145 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 110 insertions(+), 35 deletions(-)

Comments

Roger Quadros Oct. 17, 2017, 8:33 a.m. UTC | #1
Hi,


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 16/10/17 02:22, Ladislav Michl wrote:

missing commit log

> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  drivers/mtd/onenand/omap2.c | 145 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 110 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
> index 24a1388d3031..4811897a4e9f 100644
> --- a/drivers/mtd/onenand/omap2.c
> +++ b/drivers/mtd/onenand/omap2.c
> @@ -28,6 +28,8 @@
>  #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>
> @@ -38,7 +40,6 @@
>  #include <linux/gpio.h>
>  
>  #include <asm/mach/flash.h>
> -#include <linux/platform_data/mtd-onenand-omap2.h>
>  
>  #include <linux/omap-dma.h>
>  
> @@ -57,10 +58,8 @@ struct omap2_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 is_omap2;
>  };
>  
>  static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data)
> @@ -90,6 +89,52 @@ 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 set_async_mode(struct omap2_onenand *c)

omap2_onenand_set_async_mode()

> +{
> +	unsigned short reg = read_reg(c, ONENAND_REG_SYS_CFG1);
need blank line

> +	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
> +	write_reg(c, reg, ONENAND_REG_SYS_CFG1);
> +}
> +
> +static void set_cfg(struct omap2_onenand *c, bool sr, bool sw, int latency)

omap2_onenand_set_cfg()

> +{
> +	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;

This will set both HF and VHF flags if latency is greater than 7. Is this what we want?

> +	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 get_freq(struct omap2_onenand *c)

omap2_onenand_get_freq()

> +{
> +	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;

default:
	return -EINVAL;
?

> +	}
> +
> +	return 0;
> +}
> +
>  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,7 +198,7 @@ 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->is_omap2)

seems like you should have a is_omap3 flag as this quirk is for omap3 only.

>  				/* Add a delay to let GPIO settle */
>  				syscfg = read_reg(c, ONENAND_REG_SYS_CFG1);
>  		}
> @@ -607,30 +652,46 @@ static int omap2_onenand_disable(struct mtd_info *mtd)
>  	return ret;
>  }
>  
> +static int omap2_onenand_get_dt(struct device *dev, struct omap2_onenand *c)
> +{
> +	struct device_node *child = dev->of_node;
> +	u32 cs, val;
> +
> +	if (of_property_read_u32(child, "reg", &cs) < 0) {
> +		dev_err(dev, "reg not found in DT\n");
> +		return -EINVAL;
> +	}
> +	c->gpmc_cs = cs;
> +
> +	if (!of_property_read_u32(child, "dma-channel", &val))
> +		c->dma_channel = val;
> +	else
> +		c->dma_channel = -1;
> +
> +	c->is_omap2 = (bool)of_device_get_match_data(dev);
> +
> +	/* TODO: gpio, regulator, unlocking */
> +
> +	return 0;
> +}
> +
>  static int omap2_onenand_probe(struct platform_device *pdev)
>  {
> -	struct omap_onenand_platform_data *pdata;
>  	struct omap2_onenand *c;
>  	struct onenand_chip *this;
> -	int r;
> +	int freq, r;
>  	struct resource *res;
>  
> -	pdata = dev_get_platdata(&pdev->dev);
> -	if (pdata == NULL) {
> -		dev_err(&pdev->dev, "platform data missing\n");
> -		return -ENODEV;
> -	}
> -
>  	c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL);
>  	if (!c)
>  		return -ENOMEM;
>  
> +	r = omap2_onenand_get_dt(&pdev->dev, c);
> +	if (r)
> +		goto err_kfree;
> +
>  	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;
> @@ -659,16 +720,23 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>  		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;
> +	set_async_mode(c);
> +	freq = get_freq(c);
> +	if (!freq) {
> +		dev_err(&pdev->dev,
> +			"Rate not detected, bad GPMC async timings?\n");
> +		r = -ENODEV;
> +		goto err_iounmap;
>  	}
>  
> +	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_iounmap;
> +	}
> +	if (r > 0)
> +		set_cfg(c, true, true, r);
> +
>  	if (c->gpio_irq) {
>  		if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) {
>  			dev_err(&pdev->dev,  "Failed to request GPIO%d for "
> @@ -706,27 +774,27 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>  
>  	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);
> +	mtd_set_of_node(&c->mtd, pdev->dev.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 {
> +		if (c->is_omap2) {
>  			this->read_bufferram = omap2_onenand_read_bufferram;
>  			this->write_bufferram = omap2_onenand_write_bufferram;
> +		} else {
> +			this->read_bufferram = omap3_onenand_read_bufferram;
> +			this->write_bufferram = omap3_onenand_write_bufferram;
>  		}
>  	}
>  
> -	if (pdata->regulator_can_sleep) {
> +	if (0 /* TODO: 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");
> @@ -737,14 +805,13 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>  		c->onenand.disable = omap2_onenand_disable;
>  	}
>  
> -	if (pdata->skip_initial_unlocking)
> +	if (0 /* TODO: pdata->skip_initial_unlocking */)
>  		this->options |= ONENAND_SKIP_INITIAL_UNLOCKING;
>  
>  	if ((r = onenand_scan(&c->mtd, 1)) < 0)
>  		goto err_release_regulator;
>  
> -	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;
>  
> @@ -794,12 +861,20 @@ static int omap2_onenand_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct of_device_id omap2_onenand_ids[] = {
> +	{ .compatible = "ti,omap2-onenand", .data = (void *)true, },
> +	{ .compatible = "ti,omap3-onenand", .data = (void *)false, },

This doesn't clearly show what we're doing.

Can we instead have a proper data structure for .data?
If that is an overkill maybe we should avoid the of_data approach and instead do

 if (of_device_is_compatible(node, "ti,omap3-nand")
	c->need_wait_delay = true;

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

We need to update the DT binding doc as well
devicetree/bindings/mtd$ vi gpmc-onenand.txt

This should be done in a separate patch.
Ladislav Michl Oct. 17, 2017, 9:24 a.m. UTC | #2
On Tue, Oct 17, 2017 at 11:33:56AM +0300, Roger Quadros wrote:
> Hi,
> 
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 16/10/17 02:22, Ladislav Michl wrote:
> 
> missing commit log

There will be enough future versions to add some :)

> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > ---
> >  drivers/mtd/onenand/omap2.c | 145 +++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 110 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
> > index 24a1388d3031..4811897a4e9f 100644
> > --- a/drivers/mtd/onenand/omap2.c
> > +++ b/drivers/mtd/onenand/omap2.c
> > @@ -28,6 +28,8 @@
> >  #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>
> > @@ -38,7 +40,6 @@
> >  #include <linux/gpio.h>
> >  
> >  #include <asm/mach/flash.h>
> > -#include <linux/platform_data/mtd-onenand-omap2.h>
> >  
> >  #include <linux/omap-dma.h>
> >  
> > @@ -57,10 +58,8 @@ struct omap2_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 is_omap2;
> >  };
> >  
> >  static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data)
> > @@ -90,6 +89,52 @@ 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 set_async_mode(struct omap2_onenand *c)
> 
> omap2_onenand_set_async_mode()
> 
> > +{
> > +	unsigned short reg = read_reg(c, ONENAND_REG_SYS_CFG1);
> need blank line
> 
> > +	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
> > +	write_reg(c, reg, ONENAND_REG_SYS_CFG1);
> > +}
> > +
> > +static void set_cfg(struct omap2_onenand *c, bool sr, bool sw, int latency)
> 
> omap2_onenand_set_cfg()
> 
> > +{
> > +	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;
> 
> This will set both HF and VHF flags if latency is greater than 7. Is this what we want?

That's why patches 1-5 are there, so you can verify...

> > +	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 get_freq(struct omap2_onenand *c)
> 
> omap2_onenand_get_freq()

All other helper functions are unprefixed, but will change that.

> > +{
> > +	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;
> 
> default:
> 	return -EINVAL;
> ?

As we were returning -ENODEV in this case I'd use it.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  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,7 +198,7 @@ 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->is_omap2)
> 
> seems like you should have a is_omap3 flag as this quirk is for omap3 only.

I was not sure about logic. Quirk is certainly not needed for omap2 and I'm
not sure if OneNAND can be connected to omap4 and above.

> >  				/* Add a delay to let GPIO settle */
> >  				syscfg = read_reg(c, ONENAND_REG_SYS_CFG1);
> >  		}
> > @@ -607,30 +652,46 @@ static int omap2_onenand_disable(struct mtd_info *mtd)
> >  	return ret;
> >  }
> >  
> > +static int omap2_onenand_get_dt(struct device *dev, struct omap2_onenand *c)
> > +{
> > +	struct device_node *child = dev->of_node;
> > +	u32 cs, val;
> > +
> > +	if (of_property_read_u32(child, "reg", &cs) < 0) {
> > +		dev_err(dev, "reg not found in DT\n");
> > +		return -EINVAL;
> > +	}
> > +	c->gpmc_cs = cs;
> > +
> > +	if (!of_property_read_u32(child, "dma-channel", &val))
> > +		c->dma_channel = val;
> > +	else
> > +		c->dma_channel = -1;
> > +
> > +	c->is_omap2 = (bool)of_device_get_match_data(dev);
> > +
> > +	/* TODO: gpio, regulator, unlocking */
> > +
> > +	return 0;
> > +}
> > +
> >  static int omap2_onenand_probe(struct platform_device *pdev)
> >  {
> > -	struct omap_onenand_platform_data *pdata;
> >  	struct omap2_onenand *c;
> >  	struct onenand_chip *this;
> > -	int r;
> > +	int freq, r;
> >  	struct resource *res;
> >  
> > -	pdata = dev_get_platdata(&pdev->dev);
> > -	if (pdata == NULL) {
> > -		dev_err(&pdev->dev, "platform data missing\n");
> > -		return -ENODEV;
> > -	}
> > -
> >  	c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL);
> >  	if (!c)
> >  		return -ENOMEM;
> >  
> > +	r = omap2_onenand_get_dt(&pdev->dev, c);
> > +	if (r)
> > +		goto err_kfree;
> > +
> >  	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;
> > @@ -659,16 +720,23 @@ static int omap2_onenand_probe(struct platform_device *pdev)
> >  		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;
> > +	set_async_mode(c);
> > +	freq = get_freq(c);
> > +	if (!freq) {
> > +		dev_err(&pdev->dev,
> > +			"Rate not detected, bad GPMC async timings?\n");
> > +		r = -ENODEV;
> > +		goto err_iounmap;
> >  	}
> >  
> > +	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_iounmap;
> > +	}
> > +	if (r > 0)
> > +		set_cfg(c, true, true, r);
> > +
> >  	if (c->gpio_irq) {
> >  		if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) {
> >  			dev_err(&pdev->dev,  "Failed to request GPIO%d for "
> > @@ -706,27 +774,27 @@ static int omap2_onenand_probe(struct platform_device *pdev)
> >  
> >  	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);
> > +	mtd_set_of_node(&c->mtd, pdev->dev.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 {
> > +		if (c->is_omap2) {
> >  			this->read_bufferram = omap2_onenand_read_bufferram;
> >  			this->write_bufferram = omap2_onenand_write_bufferram;
> > +		} else {
> > +			this->read_bufferram = omap3_onenand_read_bufferram;
> > +			this->write_bufferram = omap3_onenand_write_bufferram;
> >  		}
> >  	}
> >  
> > -	if (pdata->regulator_can_sleep) {
> > +	if (0 /* TODO: 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");
> > @@ -737,14 +805,13 @@ static int omap2_onenand_probe(struct platform_device *pdev)
> >  		c->onenand.disable = omap2_onenand_disable;
> >  	}
> >  
> > -	if (pdata->skip_initial_unlocking)
> > +	if (0 /* TODO: pdata->skip_initial_unlocking */)
> >  		this->options |= ONENAND_SKIP_INITIAL_UNLOCKING;
> >  
> >  	if ((r = onenand_scan(&c->mtd, 1)) < 0)
> >  		goto err_release_regulator;
> >  
> > -	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;
> >  
> > @@ -794,12 +861,20 @@ static int omap2_onenand_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static const struct of_device_id omap2_onenand_ids[] = {
> > +	{ .compatible = "ti,omap2-onenand", .data = (void *)true, },
> > +	{ .compatible = "ti,omap3-onenand", .data = (void *)false, },
> 
> This doesn't clearly show what we're doing.
> 
> Can we instead have a proper data structure for .data?
> If that is an overkill maybe we should avoid the of_data approach and instead do

I'll go for overkill in this case.

>  if (of_device_is_compatible(node, "ti,omap3-nand")
> 	c->need_wait_delay = true;
> 
> > +	{ },
> > +};
> > +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),
> >  	},
> >  };
> >  
> > 
> 
> We need to update the DT binding doc as well
> devicetree/bindings/mtd$ vi gpmc-onenand.txt
> 
> This should be done in a separate patch.
> 
> -- 
> cheers,
> -roger
--
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
Roger Quadros Oct. 17, 2017, 10:20 a.m. UTC | #3

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 17/10/17 12:24, Ladislav Michl wrote:
> On Tue, Oct 17, 2017 at 11:33:56AM +0300, Roger Quadros wrote:
>> Hi,
>>
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> On 16/10/17 02:22, Ladislav Michl wrote:
>>
>> missing commit log
> 
> There will be enough future versions to add some :)
> 
>>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
>>> ---
>>>  drivers/mtd/onenand/omap2.c | 145 +++++++++++++++++++++++++++++++++-----------
>>>  1 file changed, 110 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
>>> index 24a1388d3031..4811897a4e9f 100644
>>> --- a/drivers/mtd/onenand/omap2.c
>>> +++ b/drivers/mtd/onenand/omap2.c
>>> @@ -28,6 +28,8 @@
>>>  #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>
>>> @@ -38,7 +40,6 @@
>>>  #include <linux/gpio.h>
>>>  
>>>  #include <asm/mach/flash.h>
>>> -#include <linux/platform_data/mtd-onenand-omap2.h>
>>>  
>>>  #include <linux/omap-dma.h>
>>>  
>>> @@ -57,10 +58,8 @@ struct omap2_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 is_omap2;
>>>  };
>>>  
>>>  static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data)
>>> @@ -90,6 +89,52 @@ 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 set_async_mode(struct omap2_onenand *c)
>>
>> omap2_onenand_set_async_mode()
>>
>>> +{
>>> +	unsigned short reg = read_reg(c, ONENAND_REG_SYS_CFG1);
>> need blank line
>>
>>> +	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
>>> +	write_reg(c, reg, ONENAND_REG_SYS_CFG1);
>>> +}
>>> +
>>> +static void set_cfg(struct omap2_onenand *c, bool sr, bool sw, int latency)
>>
>> omap2_onenand_set_cfg()
>>
>>> +{
>>> +	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;
>>
>> This will set both HF and VHF flags if latency is greater than 7. Is this what we want?
> 
> That's why patches 1-5 are there, so you can verify...

original code is

        if (gpmc_clk_ns < 15) /* >66MHz */
                onenand_flags |= ONENAND_FLAG_HF;
        else
                onenand_flags &= ~ONENAND_FLAG_HF;
        if (gpmc_clk_ns < 12) /* >83MHz */
                onenand_flags |= ONENAND_FLAG_VHF;
        else
                onenand_flags &= ~ONENAND_FLAG_VHF;
        if (onenand_flags & ONENAND_FLAG_VHF)
                latency = 8;
        else if (onenand_flags & ONENAND_FLAG_HF)
                latency = 6;
        else if (gpmc_clk_ns >= 25) /* 40 MHz*/
                latency = 3;
        else
                latency = 4;

which looks like both HF and VHF flags are set for >=83MHz.

> 
>>> +	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 get_freq(struct omap2_onenand *c)
>>
>> omap2_onenand_get_freq()
> 
> All other helper functions are unprefixed, but will change that.
> 
>>> +{
>>> +	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;
>>
>> default:
>> 	return -EINVAL;
>> ?
> 
> As we were returning -ENODEV in this case I'd use it.
> 
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  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,7 +198,7 @@ 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->is_omap2)
>>
>> seems like you should have a is_omap3 flag as this quirk is for omap3 only.
> 
> I was not sure about logic. Quirk is certainly not needed for omap2 and I'm
> not sure if OneNAND can be connected to omap4 and above.
> 
all OMAPs can connect to OneNAND. Just that nobody used them beyond omap3.

>>>  				/* Add a delay to let GPIO settle */
>>>  				syscfg = read_reg(c, ONENAND_REG_SYS_CFG1);
>>>  		}
>>> @@ -607,30 +652,46 @@ static int omap2_onenand_disable(struct mtd_info *mtd)
>>>  	return ret;
>>>  }
>>>  
>>> +static int omap2_onenand_get_dt(struct device *dev, struct omap2_onenand *c)
>>> +{
>>> +	struct device_node *child = dev->of_node;
>>> +	u32 cs, val;
>>> +
>>> +	if (of_property_read_u32(child, "reg", &cs) < 0) {
>>> +		dev_err(dev, "reg not found in DT\n");
>>> +		return -EINVAL;
>>> +	}
>>> +	c->gpmc_cs = cs;
>>> +
>>> +	if (!of_property_read_u32(child, "dma-channel", &val))
>>> +		c->dma_channel = val;
>>> +	else
>>> +		c->dma_channel = -1;
>>> +
>>> +	c->is_omap2 = (bool)of_device_get_match_data(dev);
>>> +
>>> +	/* TODO: gpio, regulator, unlocking */
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int omap2_onenand_probe(struct platform_device *pdev)
>>>  {
>>> -	struct omap_onenand_platform_data *pdata;
>>>  	struct omap2_onenand *c;
>>>  	struct onenand_chip *this;
>>> -	int r;
>>> +	int freq, r;
>>>  	struct resource *res;
>>>  
>>> -	pdata = dev_get_platdata(&pdev->dev);
>>> -	if (pdata == NULL) {
>>> -		dev_err(&pdev->dev, "platform data missing\n");
>>> -		return -ENODEV;
>>> -	}
>>> -
>>>  	c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL);
>>>  	if (!c)
>>>  		return -ENOMEM;
>>>  
>>> +	r = omap2_onenand_get_dt(&pdev->dev, c);
>>> +	if (r)
>>> +		goto err_kfree;
>>> +
>>>  	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;
>>> @@ -659,16 +720,23 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>>>  		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;
>>> +	set_async_mode(c);
>>> +	freq = get_freq(c);
>>> +	if (!freq) {
>>> +		dev_err(&pdev->dev,
>>> +			"Rate not detected, bad GPMC async timings?\n");
>>> +		r = -ENODEV;
>>> +		goto err_iounmap;
>>>  	}
>>>  
>>> +	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_iounmap;
>>> +	}
>>> +	if (r > 0)
>>> +		set_cfg(c, true, true, r);
>>> +
>>>  	if (c->gpio_irq) {
>>>  		if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) {
>>>  			dev_err(&pdev->dev,  "Failed to request GPIO%d for "
>>> @@ -706,27 +774,27 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>>>  
>>>  	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);
>>> +	mtd_set_of_node(&c->mtd, pdev->dev.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 {
>>> +		if (c->is_omap2) {
>>>  			this->read_bufferram = omap2_onenand_read_bufferram;
>>>  			this->write_bufferram = omap2_onenand_write_bufferram;
>>> +		} else {
>>> +			this->read_bufferram = omap3_onenand_read_bufferram;
>>> +			this->write_bufferram = omap3_onenand_write_bufferram;
>>>  		}
>>>  	}
>>>  
>>> -	if (pdata->regulator_can_sleep) {
>>> +	if (0 /* TODO: 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");
>>> @@ -737,14 +805,13 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>>>  		c->onenand.disable = omap2_onenand_disable;
>>>  	}
>>>  
>>> -	if (pdata->skip_initial_unlocking)
>>> +	if (0 /* TODO: pdata->skip_initial_unlocking */)
>>>  		this->options |= ONENAND_SKIP_INITIAL_UNLOCKING;
>>>  
>>>  	if ((r = onenand_scan(&c->mtd, 1)) < 0)
>>>  		goto err_release_regulator;
>>>  
>>> -	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;
>>>  
>>> @@ -794,12 +861,20 @@ static int omap2_onenand_remove(struct platform_device *pdev)
>>>  	return 0;
>>>  }
>>>  
>>> +static const struct of_device_id omap2_onenand_ids[] = {
>>> +	{ .compatible = "ti,omap2-onenand", .data = (void *)true, },
>>> +	{ .compatible = "ti,omap3-onenand", .data = (void *)false, },
>>
>> This doesn't clearly show what we're doing.
>>
>> Can we instead have a proper data structure for .data?
>> If that is an overkill maybe we should avoid the of_data approach and instead do
> 
> I'll go for overkill in this case.
> 
>>  if (of_device_is_compatible(node, "ti,omap3-nand")
>> 	c->need_wait_delay = true;
>>
>>> +	{ },
>>> +};
>>> +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),
>>>  	},
>>>  };
>>>  
>>>
>>
>> We need to update the DT binding doc as well
>> devicetree/bindings/mtd$ vi gpmc-onenand.txt
>>
>> This should be done in a separate patch.
>>
diff mbox

Patch

diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
index 24a1388d3031..4811897a4e9f 100644
--- a/drivers/mtd/onenand/omap2.c
+++ b/drivers/mtd/onenand/omap2.c
@@ -28,6 +28,8 @@ 
 #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>
@@ -38,7 +40,6 @@ 
 #include <linux/gpio.h>
 
 #include <asm/mach/flash.h>
-#include <linux/platform_data/mtd-onenand-omap2.h>
 
 #include <linux/omap-dma.h>
 
@@ -57,10 +58,8 @@  struct omap2_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 is_omap2;
 };
 
 static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data)
@@ -90,6 +89,52 @@  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 set_async_mode(struct omap2_onenand *c)
+{
+	unsigned short 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 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 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 0;
+}
+
 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,7 +198,7 @@  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->is_omap2)
 				/* Add a delay to let GPIO settle */
 				syscfg = read_reg(c, ONENAND_REG_SYS_CFG1);
 		}
@@ -607,30 +652,46 @@  static int omap2_onenand_disable(struct mtd_info *mtd)
 	return ret;
 }
 
+static int omap2_onenand_get_dt(struct device *dev, struct omap2_onenand *c)
+{
+	struct device_node *child = dev->of_node;
+	u32 cs, val;
+
+	if (of_property_read_u32(child, "reg", &cs) < 0) {
+		dev_err(dev, "reg not found in DT\n");
+		return -EINVAL;
+	}
+	c->gpmc_cs = cs;
+
+	if (!of_property_read_u32(child, "dma-channel", &val))
+		c->dma_channel = val;
+	else
+		c->dma_channel = -1;
+
+	c->is_omap2 = (bool)of_device_get_match_data(dev);
+
+	/* TODO: gpio, regulator, unlocking */
+
+	return 0;
+}
+
 static int omap2_onenand_probe(struct platform_device *pdev)
 {
-	struct omap_onenand_platform_data *pdata;
 	struct omap2_onenand *c;
 	struct onenand_chip *this;
-	int r;
+	int freq, r;
 	struct resource *res;
 
-	pdata = dev_get_platdata(&pdev->dev);
-	if (pdata == NULL) {
-		dev_err(&pdev->dev, "platform data missing\n");
-		return -ENODEV;
-	}
-
 	c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL);
 	if (!c)
 		return -ENOMEM;
 
+	r = omap2_onenand_get_dt(&pdev->dev, c);
+	if (r)
+		goto err_kfree;
+
 	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;
@@ -659,16 +720,23 @@  static int omap2_onenand_probe(struct platform_device *pdev)
 		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;
+	set_async_mode(c);
+	freq = get_freq(c);
+	if (!freq) {
+		dev_err(&pdev->dev,
+			"Rate not detected, bad GPMC async timings?\n");
+		r = -ENODEV;
+		goto err_iounmap;
 	}
 
+	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_iounmap;
+	}
+	if (r > 0)
+		set_cfg(c, true, true, r);
+
 	if (c->gpio_irq) {
 		if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) {
 			dev_err(&pdev->dev,  "Failed to request GPIO%d for "
@@ -706,27 +774,27 @@  static int omap2_onenand_probe(struct platform_device *pdev)
 
 	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);
+	mtd_set_of_node(&c->mtd, pdev->dev.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 {
+		if (c->is_omap2) {
 			this->read_bufferram = omap2_onenand_read_bufferram;
 			this->write_bufferram = omap2_onenand_write_bufferram;
+		} else {
+			this->read_bufferram = omap3_onenand_read_bufferram;
+			this->write_bufferram = omap3_onenand_write_bufferram;
 		}
 	}
 
-	if (pdata->regulator_can_sleep) {
+	if (0 /* TODO: 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");
@@ -737,14 +805,13 @@  static int omap2_onenand_probe(struct platform_device *pdev)
 		c->onenand.disable = omap2_onenand_disable;
 	}
 
-	if (pdata->skip_initial_unlocking)
+	if (0 /* TODO: pdata->skip_initial_unlocking */)
 		this->options |= ONENAND_SKIP_INITIAL_UNLOCKING;
 
 	if ((r = onenand_scan(&c->mtd, 1)) < 0)
 		goto err_release_regulator;
 
-	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;
 
@@ -794,12 +861,20 @@  static int omap2_onenand_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id omap2_onenand_ids[] = {
+	{ .compatible = "ti,omap2-onenand", .data = (void *)true, },
+	{ .compatible = "ti,omap3-onenand", .data = (void *)false, },
+	{ },
+};
+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),
 	},
 };