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