Hi Roger, On Thu, Oct 12, 2017 at 02:05:10PM +0300, Roger Quadros wrote: > Hi Ladislav, > > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > On 09/10/17 23:50, Ladislav Michl wrote: > > Hi! > > > > On Mon, Oct 09, 2017 at 10:24:01PM +0800, kbuild test robot wrote: > >> Hi Ladislav, > >> > >> [auto build test WARNING on linus/master] > >> [also build test WARNING on v4.14-rc4 next-20170929] > >> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > >> > >> url: https://github.com/0day-ci/linux/commits/Ladislav-Michl/memory-omap-gpmc-use-OneNAND-base-address-from-FDT/20171009-164338 > >> config: arm-allyesconfig (attached as .config) > >> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 > >> reproduce: > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > >> chmod +x ~/bin/make.cross > >> # save the attached .config to linux build tree > >> make.cross ARCH=arm > >> > >> Note: it may well be a FALSE warning. FWIW you are at least aware of it now. > >> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings > >> > >> All warnings (new ones prefixed by >>): > >> > >> drivers/memory/omap-gpmc.c: In function 'gpmc_probe': > >>>> drivers/memory/omap-gpmc.c:1952:3: warning: 'cs' may be used uninitialized in this function [-Wmaybe-uninitialized] > >> dev_err(&pdev->dev, "cannot request GPMC CS %d\n", cs); > >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> drivers/memory/omap-gpmc.c:1929:6: note: 'cs' was declared here > >> u32 cs, val; > >> ^~ > >> > >> vim +/cs +1952 drivers/memory/omap-gpmc.c > > > > Fair enough kbuild test robot, what about a bit different approach? > > It boots to userland on OneNAND with the patch bellow, but it certainly > > needs some more testing. Releasing early to get some feedback :-) > > > > From: Ladislav Michl <ladis@xxxxxxxxxxxxxx> > > Subject: memory: omap-gpmc: use OneNAND base address from FDT > > > > OneNAND base address from FDT is ignored, so driver only works > > for bootloader configured chipselect. Let gpmc_probe_(generic_)child > > handle also onenand node and let it store FDT properties in > > platform data. > > > > Signed-off-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx> > > > > diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c > > index 2944af820558..c1522f412aa0 100644 > > --- a/arch/arm/mach-omap2/gpmc-onenand.c > > +++ b/arch/arm/mach-omap2/gpmc-onenand.c > > @@ -35,17 +35,6 @@ static unsigned latency; > > > > static struct omap_onenand_platform_data *gpmc_onenand_data; > > > > -static struct resource gpmc_onenand_resource = { > > - .flags = IORESOURCE_MEM, > > -}; > > - > > -static struct platform_device gpmc_onenand_device = { > > - .name = "omap2-onenand", > > - .id = -1, > > - .num_resources = 1, > > - .resource = &gpmc_onenand_resource, > > -}; > > - > > static struct gpmc_settings onenand_async = { > > .device_width = GPMC_DEVWIDTH_16BIT, > > .mux_add_data = GPMC_MUX_AD, > > @@ -348,13 +337,12 @@ static int omap2_onenand_setup_sync(void __iomem *onenand_base, int *freq_ptr) > > > > static int gpmc_onenand_setup(void __iomem *onenand_base, int *freq_ptr) > > { > > - struct device *dev = &gpmc_onenand_device.dev; > > unsigned l = ONENAND_SYNC_READ | ONENAND_SYNC_READWRITE; > > int ret; > > > > ret = omap2_onenand_setup_async(onenand_base); > > if (ret) { > > - dev_err(dev, "unable to set to async mode\n"); > > + pr_err("OneNAND: unable to set to async mode\n"); > > return ret; > > } > > > > @@ -363,22 +351,18 @@ static int gpmc_onenand_setup(void __iomem *onenand_base, int *freq_ptr) > > > > ret = omap2_onenand_setup_sync(onenand_base, freq_ptr); > > if (ret) > > - dev_err(dev, "unable to set to sync mode\n"); > > + pr_err("OneNAND: unable to set to sync mode\n"); > > return ret; > > } > > > > -int gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data) > > +void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data) > > { > > - int err; > > - struct device *dev = &gpmc_onenand_device.dev; > > - > > gpmc_onenand_data = _onenand_data; > > gpmc_onenand_data->onenand_setup = gpmc_onenand_setup; > > - gpmc_onenand_device.dev.platform_data = gpmc_onenand_data; > > > > if (cpu_is_omap24xx() && > > (gpmc_onenand_data->flags & ONENAND_SYNC_READWRITE)) { > > - dev_warn(dev, "OneNAND using only SYNC_READ on 24xx\n"); > > + pr_warn("OneNAND: using only SYNC_READ on 24xx\n"); > > gpmc_onenand_data->flags &= ~ONENAND_SYNC_READWRITE; > > gpmc_onenand_data->flags |= ONENAND_SYNC_READ; > > } > > @@ -387,23 +371,4 @@ int gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data) > > gpmc_onenand_data->flags |= ONENAND_IN_OMAP34XX; > > else > > gpmc_onenand_data->flags &= ~ONENAND_IN_OMAP34XX; > > - > > - err = gpmc_cs_request(gpmc_onenand_data->cs, ONENAND_IO_SIZE, > > - (unsigned long *)&gpmc_onenand_resource.start); > > - if (err < 0) { > > - dev_err(dev, "Cannot request GPMC CS %d, error %d\n", > > - gpmc_onenand_data->cs, err); > > - return err; > > - } > > - > > - gpmc_onenand_resource.end = gpmc_onenand_resource.start + > > - ONENAND_IO_SIZE - 1; > > - > > - err = platform_device_register(&gpmc_onenand_device); > > - if (err) { > > - dev_err(dev, "Unable to register OneNAND device\n"); > > - gpmc_cs_free(gpmc_onenand_data->cs); > > - } > > - > > - return err; > > } > > If possible let's try to split changes to gpmc-onenand.c and omap-gpmc.c into different patches. For sure I tried, but it is quite hard to get splitted patches which are readable, compile and work at the same time... Anyway, there'll be proper v3... > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > > index 7059bbda2fac..9f416ee94324 100644 > > --- a/drivers/memory/omap-gpmc.c > > +++ b/drivers/memory/omap-gpmc.c > > @@ -1922,50 +1922,15 @@ 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 > > - > > /** > > - * gpmc_probe_generic_child - configures the gpmc for a child device > > + * gpmc_probe_child - configures the gpmc for a child device > > * @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 gpmc_settings gpmc_s; > > @@ -2080,6 +2045,51 @@ 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")) { > > + struct platform_device *onenand_pdev; > > + struct omap_onenand_platform_data *gpmc_onenand_data = > > + devm_kzalloc(&pdev->dev, sizeof(*gpmc_onenand_data), > > + GFP_KERNEL); > > + > > + if (!gpmc_onenand_data) { > > + ret = -ENOMEM; > > + goto err; > > + } > > + > > + if (!of_property_read_u32(child, "dma-channel", &val)) > > + gpmc_onenand_data->dma_channel = val; > > + else > > + gpmc_onenand_data->dma_channel = -1; > > + > > + gpmc_onenand_data->cs = cs; > > + gpmc_onenand_data->of_node = child; > > I don't like this approach of passing of_node information via platform data. I do not like it either, but that field is already here and patch sent is minimal one to get things work. > We need to teach onenand/omap2.c driver to be a legitimate device tree driver like nand/omap2.c > so that it can get its device node via dev->of_node. > > We should also be able to get cs, dma_channel and other information directly from the device tree > within the onenand driver. Indeed. There is no problem with that and this change will be merged via mtd tree. See bellow. > > + > > + onenand_pdev = platform_device_alloc("omap2-onenand", cs); > > + if (!onenand_pdev) { > > + ret = -ENOMEM; > > + goto err; > > + } > > + > > + ret = platform_device_add_resources(onenand_pdev, &res, 1); > > + if (ret) { > > + platform_device_put(onenand_pdev); > > + goto err; > > + } > > + > > + onenand_pdev->dev.platform_data = gpmc_onenand_data; > > + > > + ret = platform_device_add(onenand_pdev); > > + if (ret) { > > + platform_device_put(onenand_pdev); > > + goto err; > > + } > > + > > + /* Enable CS region */ > > + gpmc_cs_enable_mem(cs); > > + > > + gpmc_onenand_init(gpmc_onenand_data); > > call to gpmc_onenand_init() should go away and it should be deprecated. > > We can get cook the necessary flags based on the DT node's compatible property > within the onenand driver as omap2 and omap3 variants should have different compatible ids. > > We could probably just setup the ASYNC settings and timings here and get rid of > omap2_onenand_setup_async() altogether. > > The only missing piece now is calling omap2_onenand_setup_sync(). This should > probably be done directly from the onenand driver. > > Tony, > > any ideas how we'd like that to be done? 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 :-) 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? ladis > > + > > + return 0; > > > > } else { > > ret = of_property_read_u32(child, "bank-width", > > &gpmc_s.device_width); > > @@ -2194,11 +2204,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); > > diff --git a/include/linux/omap-gpmc.h b/include/linux/omap-gpmc.h > > index fd0de00c0d77..4c4d8856f443 100644 > > --- a/include/linux/omap-gpmc.h > > +++ b/include/linux/omap-gpmc.h > > @@ -77,11 +77,7 @@ struct omap_nand_platform_data; > > struct omap_onenand_platform_data; > > > > #if IS_ENABLED(CONFIG_MTD_ONENAND_OMAP2) > > -extern int gpmc_onenand_init(struct omap_onenand_platform_data *d); > > +extern void gpmc_onenand_init(struct omap_onenand_platform_data *d); > > #else > > -#define board_onenand_data NULL > > -static inline int gpmc_onenand_init(struct omap_onenand_platform_data *d) > > -{ > > - return 0; > > -} > > +static inline void gpmc_onenand_init(struct omap_onenand_platform_data *d) { } > > #endif > > > > -- > 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
