Hi Roger!
On Fri, Oct 13, 2017 at 11:52:44AM +0300, Roger Quadros wrote:
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
> On 12/10/17 23:29, Ladislav Michl wrote:
> > Hi Roger,
> >
> > On Thu, Oct 12, 2017 at 06:05:31PM +0300, Roger Quadros wrote:
> > [snip]
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> >> On 12/10/17 15:27, Ladislav Michl wrote:
> >>> Not being Tony, I try to answer anyway...
> >>>
> >>> I've done patchset which removes gpmc_probe_onenand_child and uses
> >>> gpmc_probe_generic_child (renamed to gpmc_probe_child) to setup OneNAND
> >>> async timings. Sync timings is setup from mtd/onenand/omap2.c after
> >>> chip frequency is known.
> >>>
> >>> I've not sent it yet as device datasheet is available under NDA only,
> >>> I do not have it and thus I'm left with equivalent source modifications
> >>> without really understanding what should be done and why.
> >>>
> >>> That said, I sent this patch to fix OneNAND support (for IGEPv2 board
> >>> at least) and leave discussion about making things nice for later.
> >>>
> >>> I'm proposing killing gpmc_onenand_data first and teaching OneNAND
> >>> to get its configuration from DT. Later I'll cleanup sync/async
> >>> patchset and send it for review unless someone is willing to take
> >>> it over to speed things up a bit - then I'll send it as it is :-)
> >>
> >> sounds fine to me.
> >>>
> >>> Side question: in tree OneNAND DT nodes are providing timing information.
> >>> I have no clue where this timing information is coming from and driver
> >>> completely ignores it and computes timings on its own. Any hints here?
> >>
> >> My guess is nobody has access to the datasheet and we're relying on bootloader
> >> set timings or timings that were already hardcoded in the kernel.
> >>
> >> You can use CONFIG_OMAP_GPMC_DEBUG to dump the timings set by the bootloader
> >> in DT format.
> >>
> >> You can also compare the timings before and after gpmc driver loads.
> >
> > That will not help a lot, as these questions will remain unanswered using
> > above method:
> > - why SYNC_WRITE cannot be used on 24xx?
>
> No idea. This is there right from the first commit.
Okay. Lets leave this on DT and remove it from code. That code
path is not currently used anyway.
> > - why are we letting gpio line settle only on 34xx?
>
> >From commit 782b7a367d81da005d93b28cb00f9ae086773c24
>
> "On OMAP3, the driver was occasionally not seeing the GPIO
> interrupt. Adding a small delay of one register read
> eliminates the problem."
>
> I guess this was a quick workaround to the problem faced by the early users
> of OneNand on OMAP3.
>
> > - why we are not using SYNC_WRITE with 54 MHz freq?
>
> I'm not sure. Seems to be in there from the first commit.
>
> > - how should be omap_onenand_platform_data flags set by default?
> > probably few more I do not remember, but will show up once I start
> > polishing patchset :)
> >
> > Anyway, here's first untested version, just to show, that we need
> > to touch arch/arm/mach-omap2/gpmc-onenand.c anyway, just to make
> > gpmc_onenand_setup directly callable without global
> > omap_onenand_platform_data (and later move it into omap-gpmc)
>
> Best to start a fresh thread with new patches. Patch looks in the right direction.
>
> >
> > So unless someone comes with better idea, I'll drop all global
> > variables from arch/arm/mach-omap2/gpmc-onenand.c and export
> > gpmc_onenand_setup to get rid of omap_onenand_platform_data.
> > After that patch gets merged, we can make additional patch on
> > top of following one, which will drop use of omap_onenand_platform_data
> > from omap2 onenand mtd driver. There are simply too many trees to
> > get things merged to make it easy task :-/
>
> I understand it is a bit challenging. but we'll work it out. Main thing to
> keep in mind is to not break build in the different trees. So you might have to
> still keep some code around (especially platform data) but just mark it deprecated.
Okay, lets start separate thread with patchset. It seems it's doable after
all :-)
> > Also note that no DT only kernel sets flags, regulator, gpio and yet
> > noone complained, so we could leave it for later - and use more recent
> > APIs (gpiolib, etc).
>
> At least nobody seems to be setting regulator and gpio in arch/arm/gpmc-onenand.c
> so probably things work without it for everyone.
>
> >
> > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
> > index 29cc0c21138c..85fac7aed2dc 100644
> > --- a/drivers/mtd/onenand/omap2.c
> > +++ b/drivers/mtd/onenand/omap2.c
> > @@ -28,12 +28,15 @@
> > #include <linux/mtd/mtd.h>
> > #include <linux/mtd/onenand.h>
> > #include <linux/mtd/partitions.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.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/sys_soc.h>
> > #include <linux/regulator/consumer.h>
> > #include <linux/gpio.h>
> >
> > @@ -59,7 +62,7 @@ struct omap2_onenand {
> > int dma_channel;
> > int freq;
> > struct regulator *regulator;
> > - u8 flags;
> > + bool gpio_settle;
> > };
> >
> > static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data)
> > @@ -152,7 +155,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->gpio_settle)
> > /* Add a delay to let GPIO settle */
> > syscfg = read_reg(c, ONENAND_REG_SYS_CFG1);
> > }
> > @@ -606,6 +609,32 @@ 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;
> > +
> > + /* TODO: gpio, regulator, unlocking */
> > +
> > + return 0;
> > +}
> > +
> > +static const struct soc_device_attribute omap2_soc_devices[] = {
> > + { .family = "OMAP2*" },
> > + { /* sentinel */ }
> > +};
> > +
>
> Is this the preferred way of doing things? Tony?
> I was thinking that we could use separate compatible ids for omap2 vs omap3+
> e.g.
>
> for omap2, compatible = "ti,omap2-onenand";
> for omap3 onwards, compatible = "ti,omap3-onenand";
>
> and use of_device_is_compatible() to do omap2 vs omap3 specific stuff.
Well, the problem is that there are 'compatible = "ti,omap2-onenand"' DTs
around for OMAP3 devices such as Nokia 8x0 AFAIR.
Anyway, I'm also interested what preferred solution is. Once revealed I'll send
proper patch.
> > static int omap2_onenand_probe(struct platform_device *pdev)
> > {
> > struct omap_onenand_platform_data *pdata;
> > @@ -624,12 +653,12 @@ static int omap2_onenand_probe(struct platform_device *pdev)
> > 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;
> > @@ -710,21 +739,22 @@ static int omap2_onenand_probe(struct platform_device *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 (soc_device_match(omap2_soc_devices)) {
> > this->read_bufferram = omap2_onenand_read_bufferram;
> > this->write_bufferram = omap2_onenand_write_bufferram;
> > + } else {
> > + c->gpio_settle = true;
> > + 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");
> > @@ -735,14 +765,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;
>
> This should be a standard onenand binding. Maybe Boris can recommend what to do here.
>
> >
> > 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;
> >
> > @@ -792,12 +821,19 @@ static int omap2_onenand_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +static const struct of_device_id omap2_onenand_ids[] = {
> > + { .compatible = "ti,omap2-onenand", },
> > + {},
> > +};
> > +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
--
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
