Hi Roger,
On Fri, Oct 13, 2017 at 03:11:37PM +0300, Roger Quadros wrote:
> Ladislav,
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
> On 13/10/17 15:05, Ladislav Michl wrote:
> > 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.
>
> Nokia n8xx devices are all OMAP2 based and can continu use ti,omap2-onenand.
> Nokia n9xx devices are all OMAP3 based so need to use ti,omap3-onenand. It shouldn't
> be a big problem to add a new binding.
Quick grep arch/arm/boot/dts finds omap3-n950-n9.dtsi, but there is no
compatible property as well as there is not any in omap2420-n8x0-common.dtsi.
We could probably warn and bail out as we do in NAND case, but it would break
backward compatibility. Pavel?
Another user is IGEP board, but as it is broken in mainline anyway and
I'm probably the only mainline user, it is not a big deal ;-)
> > 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
> >
>
> --
> 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
