Re: Passing NAND mtdparts to OMAP2+ Kernel

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Wed, 29 Mar 2017 15:10:02 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed, 29 Mar 2017 15:41:07 +0300
> Roger Quadros <rogerq@ti.com> wrote:
> 
> > Adam,
> > 
> > On 29/03/17 14:39, Adam Ford wrote:  
> > > On Tue, Mar 28, 2017 at 2:57 PM, Boris Brezillon
> > > <boris.brezillon@free-electrons.com> wrote:    
> > >> +Roger and Enrico
> > >>
> > >> On Tue, 28 Mar 2017 10:43:01 -0500
> > >> Adam Ford <aford173@gmail.com> wrote:
> > >>    
> > >>> I posted this on the linux-omap list, and I was asked to post this on
> > >>> the linux-mtd list:
> > >>>
> > >>>
> > >>> I tried to remove the MTD partitions from the Linux device tree, and I
> > >>> noticed that there was no partition information being pushed anymore
> > >>> unless I changed the mtdparts name in U-Boot.
> > >>>
> > >>> It appears as if the MTD drivers have changed a bit.  I found a few
> > >>> e-mails floating around that attempt to fix this
> > >>>
> > >>> Commit f7a8e38f07a17be907585 ("mtd: nand: assign reasonable default
> > >>> name for NAND drivers") attempts to address this, and someone over at
> > >>> https://patchwork.ozlabs.org/patch/707065/ attempted to address it as
> > >>> well in a slightly different way.    
> > >>
> > >> Can you test the patch and let me know if solves the problem. If it
> > >> does, I'll send a clean version of the patch and queue it for 4.12.
> > >>    
> > > 
> > > I tried to apply the patch directly, but it failed.  I then manually
> > > copy-pasted it into the proper place, but it fails to compile.
> > > 
> > > drivers/mtd/nand/omap2.c: In function ‘omap_nand_probe’:
> > > drivers/mtd/nand/omap2.c:1859:14: error: implicit declaration of
> > > function ‘devm_kasprinf’ [-Werror=implicit-function-declaration]
> > >   mtd->name = devm_kasprinf(&pdev->dev, "omap2-nand.%d", info->gpmc_cs);
> > >               ^~~~~~~~~~~~~
> > > drivers/mtd/nand/omap2.c:1859:12: warning: assignment makes pointer
> > > from integer without a cast [-Wint-conversion]
> > >   mtd->name = devm_kasprinf(&pdev->dev, "omap2-nand.%d", info->gpmc_cs);
> > >             ^
> > > cc1: some warnings being treated as errors
> > > 
> > > I use buildroot to build my toolchain and I am using gcc version 6.3.0
> > > with glibc 2.24.  Is there supposed to be an include somewhere?  I am
> > > not familiar with devm_kasprinf.  
> 
> It's a typo: s/devm_kasprinf/devm_kasprintf/.
> 
> > > 
> > >     
> > 
> > Does the below patch work for you?  
> 
> Should work indeed.
> 
> I had a closer look and it seems that the bug was actually introduced
> by c9711ec5250b mtd: nand: omap: Clean up device tree
> support.
> The parent device name has changed when switching to the new DT
> representation: omap2-nand.0 (where 0 is the controller instance) became
> 30000000.nand (where 30000000 is the base reg address in the physical
> address space). Which means my proposal was incorrect (0 is not the CS
> line, it's the NAND controller instance number), so we'd better
> statically set it to "omap2-nand.0".

Changing my mind (again :)). According to [1], the instance id is
related to the CS line, so devm_kasprintf() is the right solution.

Sorry for the noise.

> 
> > 
> > cheers,
> > -roger
> > 
> > ---
> >  drivers/mtd/nand/omap2.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index 2a52101..f693b8d 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> > @@ -1856,6 +1856,13 @@ static int omap_nand_probe(struct platform_device *pdev)
> >  	nand_chip->ecc.priv	= NULL;
> >  	nand_set_flash_node(nand_chip, dev->of_node);
> >    
> 
> nand_set_flash_node() is now taking the "label" DT property into account
> and assigning mtd->name to this value if present. I'd recommend doing
> 
> 	if (!mtd->name)
> 		mtd->name = "omap2-nand.0";

This comment still stands, except it should be:

	if (!mtd->name)
		mtd->name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
					   "omap2-nand.%d", info->gpmc_cs);

	if (!mtd->name) {
		dev_err(&pdev->dev, "Failed to set MTD name\n");
		return -ENOMEM;
	}

Regards,

Boris

[1]http://lxr.free-electrons.com/source/arch/arm/mach-omap2/gpmc-nand.c?v=4.6#L133