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.
>
> 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
