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.
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.
> IMO this is way better than things going unfixed for years together.
I'd say print annoying warning is enough.
> >>> + }
> >>> +
> >>> 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
