Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 17/10/17 12:46, Ladislav Michl wrote:
> On Tue, Oct 17, 2017 at 12:12:15PM +0300, Roger Quadros wrote:
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> On 17/10/17 12:04, Ladislav Michl wrote:
>>> On Tue, Oct 17, 2017 at 11:46:51AM +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:20, Ladislav Michl wrote:
>>>>> Generic probe function can deal with OneNAND node, remove
>>>>> now useless gpmc_probe_onenand_child function.
>>>>>
>>>>> Signed-off-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
>>>>> ---
>>>>> drivers/memory/omap-gpmc.c | 111 ++++++++++++++++++++++++++++-----------------
>>>>> 1 file changed, 69 insertions(+), 42 deletions(-)
>>>>>
>>>>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
>>>>> index f2aef0b87bc6..fc7c3c2a0b04 100644
>>>>> --- a/drivers/memory/omap-gpmc.c
>>>>> +++ b/drivers/memory/omap-gpmc.c
>>>>> @@ -1921,52 +1921,22 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
>>>>> of_property_read_bool(np, "gpmc,time-para-granularity");
>>>>> }
>>>>>
>>>>> -#if IS_ENABLED(CONFIG_MTD_ONENAND)
>>>>> -static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>>>> - struct device_node *child)
>>>>> -{
>>>>> - u32 val;
>>>>> - struct omap_onenand_platform_data *gpmc_onenand_data;
>>>>> -
>>>>> - if (of_property_read_u32(child, "reg", &val) < 0) {
>>>>> - dev_err(&pdev->dev, "%pOF has no 'reg' property\n",
>>>>> - child);
>>>>> - return -ENODEV;
>>>>> - }
>>>>> -
>>>>> - gpmc_onenand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_onenand_data),
>>>>> - GFP_KERNEL);
>>>>> - if (!gpmc_onenand_data)
>>>>> - return -ENOMEM;
>>>>> -
>>>>> - gpmc_onenand_data->cs = val;
>>>>> - gpmc_onenand_data->of_node = child;
>>>>> - gpmc_onenand_data->dma_channel = -1;
>>>>> -
>>>>> - if (!of_property_read_u32(child, "dma-channel", &val))
>>>>> - gpmc_onenand_data->dma_channel = val;
>>>>> -
>>>>> - return gpmc_onenand_init(gpmc_onenand_data);
>>>>> -}
>>>>> -#else
>>>>> -static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>>>> - struct device_node *child)
>>>>> -{
>>>>> - return 0;
>>>>> -}
>>>>> -#endif
>>>>> +extern int
>>>>> +gpmc_omap_onenand_init(struct omap_onenand_platform_data *_onenand_data);
>>>>>
>>>>> /**
>>>>> - * gpmc_probe_generic_child - configures the gpmc for a child device
>>>>> + * gpmc_probe_child - configures the gpmc for a child device
>>>>
>>>> Let's not do this name change it is not really required.
>>>
>>> But it is nice, right? :) Will drop that change.>
>>
>> :)
>>
>>>>> * @pdev: pointer to gpmc platform device
>>>>> * @child: pointer to device-tree node for child device
>>>>> *
>>>>> * Allocates and configures a GPMC chip-select for a child device.
>>>>> * Returns 0 on success and appropriate negative error code on failure.
>>>>> */
>>>>> -static int gpmc_probe_generic_child(struct platform_device *pdev,
>>>>> +static int gpmc_probe_child(struct platform_device *pdev,
>>>>> struct device_node *child)
>>>>> {
>>>>> + struct omap_onenand_platform_data *gpmc_onenand_data;
>>>>> + struct platform_device *child_dev;
>>>>> struct gpmc_settings gpmc_s;
>>>>> struct gpmc_timings gpmc_t;
>>>>> struct resource res;
>>>>> @@ -2058,6 +2028,33 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>>>>> }
>>>>> }
>>>>>
>>>>> + if (of_node_cmp(child->name, "onenand") == 0) {
>>>>> + /*
>>>>> + * Warn about older DT blobs with no compatible property
>>>>> + * and fix them for now
>>>>> + */
>>>>> + if (!of_property_read_bool(child, "compatible")) {
>>>>> + struct property *prop;
>>>>> +
>>>>> + dev_warn(&pdev->dev,
>>>>> + "Incompatible OneNAND node, fixing.");
>>>>> +
>>>>> + prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>>>>> + if (!prop) {
>>>>> + ret = -ENOMEM;
>>>>> + goto err;
>>>>> + }
>>>>> +
>>>>> + prop->name = "compatible";
>>>>> + /* See FIXME about device-tree migration bellow */
>>>>> + prop->value = (gpmc_capability & GPMC_HAS_WR_ACCESS) ?
>>>>> + "ti,omap3-onenand" : "ti,omap2-onenand";
>>>>> + prop->length = 14;
>>>>> +
>>>>> + of_update_property(child, prop);
>>>>> + }
>>>>
>>>> I don't want to fixup broken DTs like this as they will go silently without being fixed.
>>>>
>>>> Let's instead just error out after the dev_warn().
>>>> This will force us to fix all DT nodes.
>>>
>>> Is it really right thing to do? A lot of people scream loudly when you break
>>> their boot.
>>>
>> And it is our job to fix it if it does :).
>
> Well, we can't really fix this. DT is expected to work for various kernels
> long term. At least that's what I have read long time ago - it is part of
> kernel ABI.
But gpmc,onenand doesn't even have a compatible ID. So I'm not sure if it is a qualified ABI yet.
>
> I have many OMAP3 devices with OneNAND in field.
> Those are booting in Falcon mode:
> https://www.denx.de/wiki/pub/U-Boot/MiniSummitELCE2013/2013-ELCE-U-Boot-Falcon-Boot.pdf
> FDT blob is updated only when servicing device (full reflash). In this
> case full U-Boot is run and FDT is modified - RAM size, MTD partitioning,
> SoC type, etc. is stored in DT and flashed. U-Boot SPL the just reads it
> and passes to kernel.
>
> Now this approach means I cannot do remote kernel upgrade as it will not
> boot. I'd have to write utility to fix device tree before updating and
> I do not think I'm alone using similar approach.
Oh, OK.
>
>> IMO this is way better than things going unfixed for years together.
>
> I'd say print annoying warning is enough.
Maintaining backward compatibility will only work if we leave other parts of the node as it is.
But what timings/settings are used in DT in the devices you use in the field?
Do they provide ASYNC timings or SYNC timings?
is sync-read/sync-write set?
is sync-clk-ls set?
Are the timings self sufficient or they still need patching up by driver.
Maybe you can carry the DT on the fly patch up code in your customer support tree?
>
>>>>> + }
>>>>> +
>>>>> if (of_device_is_compatible(child, "ti,omap2-nand")) {
>>>>> /* NAND specific setup */
>>>>> val = 8;
>>>>> @@ -2079,6 +2076,30 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>>>>> /* disable write protect */
>>>>> gpmc_configure(GPMC_CONFIG_WP, 0);
>>>>> gpmc_s.device_nand = true;
>>>>> + } else if (of_device_is_compatible(child, "ti,omap2-onenand") ||
>>>>> + of_device_is_compatible(child, "ti,omap3-onenand")) {
>>>>
>>>> Could just be if() instead of else if() as we don't have any dependency on nand.
>>>
>>> It could, but as node is eithrt nand or onenand, this saves few cycles.
>>>
>>
>> I don't think the few cycles matter that much. I'd choose an option which
>> has better readability. Choice is yours :)
>
> This few cycles/bytes approach lead to unbootable kernel during past 15 years
> on my omap1 devices ;-)
>
>>>>> + gpmc_onenand_data = devm_kzalloc(&pdev->dev,
>>>>> + sizeof(*gpmc_onenand_data),
>>>>> + GFP_KERNEL);
>>>>> + if (!gpmc_onenand_data) {
>>>>> + ret = -ENOMEM;
>>>>> + goto err;
>>>>> + }
>>>>> +
>>>>> + gpmc_onenand_data->cs = cs;
>>>>> + gpmc_onenand_data->of_node = child;
>>>>> + if (!of_property_read_u32(child, "dma-channel", &val))
>>>>> + gpmc_onenand_data->dma_channel = val;
>>>>> + else
>>>>> + gpmc_onenand_data->dma_channel = -1;
>>>>> +
>>>>> + ret = gpmc_omap_onenand_init(gpmc_onenand_data);
>>>>> + if (ret)
>>>>> + goto err;
>>>>
>>>> this gpmc_onenand_data should go away if we squash patch 10 here.
>>>
>>> But driver will stop working then as setup function will be NULL.
>>>
>> Which is fine. It will start working again when mtd parts get merged via mtd tree.
>>
>>>>> +
>>>>> + /* OneNAND driver handles timings on its own */
>>>>> + gpmc_cs_enable_mem(cs);
>>>>> + goto no_timings;
>>>>
>>>> Actually ASYNC timings should be handled here just as the other children.
>>>> Only SYNC timings will be triggered by the OneNAND driver.
>>>
>>> It is handled here, it just cannot be done in one patch as MTD driver
>>> needs to be modified.
>>>
>>>>> } else {
>>>>> ret = of_property_read_u32(child, "bank-width",
>>>>> &gpmc_s.device_width);
>>>>> @@ -2126,9 +2147,19 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>>>>> no_timings:
>>>>>
>>>>> /* create platform device, NULL on error or when disabled */
>>>>> - if (!of_platform_device_create(child, NULL, &pdev->dev))
>>>>> + child_dev = of_platform_device_create(child, NULL, &pdev->dev);
>>>>> + if (!child_dev)
>>>>> goto err_child_fail;
>>>>>
>>>>> + /* Use platform data until OneNAND driver is DT aware */
>>>>> + if (gpmc_onenand_data) {
>>>>> + child_dev->name = "omap2-onenand";
>>>>> + child_dev->dev.platform_data = gpmc_onenand_data;
>>>>> + ret = platform_device_add_resources(child_dev, &res, 1);
>>>>> + if (ret)
>>>>> + goto err_child_fail;
>>>>> + }
>>>>> +
>>>>
>>>> This bit will go away on squashing patch 10.
>>>
>>> I'm not sure how to do this and merge changes using two separate trees.
>>
>> - make sure build doesn't break in each tree. I'm OK with oneNAND functionality broken in
>> individual trees for this case. As there is no other way out and make your life easier.
>> - make sure everything works after trees are merged.
>>>
>>>>> /* is child a common bus? */
>>>>> if (of_match_node(of_default_bus_match_table, child))
>>>>> /* create children and other common bus children */
>>>>> @@ -2193,11 +2224,7 @@ static void gpmc_probe_dt_children(struct platform_device *pdev)
>>>>> if (!child->name)
>>>>> continue;
>>>>>
>>>>> - if (of_node_cmp(child->name, "onenand") == 0)
>>>>> - ret = gpmc_probe_onenand_child(pdev, child);
>>>>> - else
>>>>> - ret = gpmc_probe_generic_child(pdev, child);
>>>>> -
>>>>> + ret = gpmc_probe_child(pdev, child);
>>>>> if (ret) {
>>>>> dev_err(&pdev->dev, "failed to probe DT child '%s': %d\n",
>>>>> child->name, ret);
>>>>>
>>>>
--
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
