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