LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Antoine Tenart <antoine.tenart@free-electrons.com>
To: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Antoine Tenart <antoine.tenart@free-electrons.com>,
	sebastian.hesselbarth@gmail.com,
	ezequiel.garcia@free-electrons.com, dwmw2@infradead.org,
	computersforpeace@gmail.com, thomas.petazzoni@free-electrons.com,
	zmxu@marvell.com, linux-kernel@vger.kernel.org,
	linux-mtd@lists.infradead.org, jszhang@marvell.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 5/9] mtd: pxa3xx_nand: add support for the Marvell Berlin nand controller
Date: Wed, 11 Feb 2015 17:31:19 +0100	[thread overview]
Message-ID: <20150211163119.GG11169@kwain> (raw)
In-Reply-To: <20150209005503.3fad39c7@bbrezillon>

Boris,

On Mon, Feb 09, 2015 at 12:55:03AM +0100, Boris Brezillon wrote:
> On Tue, 27 Jan 2015 15:10:12 +0100
> Antoine Tenart <antoine.tenart@free-electrons.com> wrote:
> 
> >  
> > +		if (info->variant == PXA3XX_NAND_VARIANT_BERLIN2 &&
> > +				info->ndcb0 & NDCB0_LEN_OVRD)
> > +			nand_writel(info, NDCB0,
> > +					info->chunk_size + info->oob_size);
> > +
> 
> Why don't you reuse the following if statement (which is doing the
> exact same thing, except for the size being stored in ndbc3 when
> the command is created).

I'll update to reuse this!

> Moreover, I'm pretty sure you don't want to add oob_size on the 2nd
> chunk, but only on the last one.
> Doing that will only work for 4K pages (2 * max_chunk_size), and you
> add support for 8K pages NANDs in this patch.

Yep. As a matter of fact, oob_size was equl to 0 in this case because of
some dirty hacks. I'll remove them and will send a v2 with a proper
solution.

> >  
> > +	if (info->variant == PXA3XX_NAND_VARIANT_BERLIN2)
> > +		chip->cmdfunc = nand_cmdfunc_berlin;
> 
> Your cmdfunc looks like the nand_cmdfunc_extended one, and this one is
> only used for NAND chips that have writesize > PAGE_CHUNK_SIZE.
> 
> Is your cmdfunc supporting accesses to NAND chips with smaller pages ?
> If you're unsure, maybe you should add a check here, and refuse to
> probe such NAND chips.

I currently have no idea, so I'll add a check here.

> >  
> > -	num = ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1;
> > -	for (i = 0; i < num; i++) {
> > -		if (i < pdata->num_flash)
> > -			f = pdata->flash + i;
> > -		else
> > -			f = &builtin_flash_types[i - pdata->num_flash + 1];
> > +	if (info->variant == PXA3XX_NAND_VARIANT_BERLIN2)
> > +		flash_types = berlin_builtin_flash_types;
> > +	else
> > +		flash_types = builtin_flash_types;
> >  
> > -		/* find the chip in default list */
> > +	for (i = 0; (f = &flash_types[i]); i++)
> >  		if (f->chip_id == id)
> >  			break;
> > +
> > +	if (f == NULL) {
> > +		for (i = 0; i < pdata->num_flash; i++) {
> > +			f = pdata->flash + i;
> > +			if (f->chip_id == id)
> > +				break;
> > +		}
> >  	}
> 
> You're changing the matching order here, the initial order was:
> 
> 1/ pdata definition
> 2/ builtin types
> 
> and you're now doing:
> 
> 1/ builtin types
> 2/ pdata definition

Oops...

Thanks for the review!

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

  parent reply	other threads:[~2015-02-11 16:31 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-27 14:10 [PATCH 0/9] ARM: berlin: add nand support Antoine Tenart
2015-01-27 14:10 ` [PATCH 1/9] mtd: pxa3xx_nand: initialiaze pxa3xx_flash_ids to 0 Antoine Tenart
2015-01-27 14:10 ` [PATCH 2/9] mtd: pxa3xx_nand: add a non mandatory ECC clock Antoine Tenart
2015-01-27 15:50   ` Andrew Lunn
2015-01-28 14:14     ` Antoine Tenart
2015-02-08 19:55       ` Boris Brezillon
2015-01-28  3:35   ` Jisheng Zhang
2015-01-28 14:17     ` Antoine Tenart
2015-01-27 14:10 ` [PATCH 3/9] mtd: pxa3xx_nand: set NDCR_PG_PER_BLK if page per block is 128 Antoine Tenart
2015-02-08 20:00   ` Boris Brezillon
2015-01-27 14:10 ` [PATCH 4/9] mtd: pxa3xx_nand: add a default chunk size Antoine Tenart
2015-02-08 20:15   ` Boris Brezillon
2015-02-08 20:18     ` Boris Brezillon
2015-01-27 14:10 ` [PATCH 5/9] mtd: pxa3xx_nand: add support for the Marvell Berlin nand controller Antoine Tenart
2015-01-27 14:42   ` Ezequiel Garcia
2015-02-06  1:25     ` Brian Norris
2015-02-08 21:06   ` Boris Brezillon
2015-02-11 16:27     ` Antoine Tenart
2015-02-08 23:55   ` Boris Brezillon
2015-02-10 19:50     ` Robert Jarzmik
2015-02-11 16:33       ` Antoine Tenart
2015-02-12 16:26         ` Robert Jarzmik
2015-02-17  9:52           ` Antoine Tenart
2015-02-11 16:31     ` Antoine Tenart [this message]
2015-01-27 14:10 ` [PATCH 6/9] Documentation: bindings: add the Berlin nand controller compatible Antoine Tenart
2015-01-27 14:10 ` [PATCH 7/9] mtd: nand: let Marvell Berlin SoCs select the pxa3xx driver Antoine Tenart
2015-01-27 14:10 ` [PATCH 8/9] ARM: berlin: add BG2Q node for the nand Antoine Tenart
2015-01-27 14:10 ` [PATCH 9/9] ARM: berlin: enable flash on the BG2Q DMP Antoine Tenart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150211163119.GG11169@kwain \
    --to=antoine.tenart@free-electrons.com \
    --cc=boris.brezillon@free-electrons.com \
    --cc=computersforpeace@gmail.com \
    --cc=dwmw2@infradead.org \
    --cc=ezequiel.garcia@free-electrons.com \
    --cc=jszhang@marvell.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=sebastian.hesselbarth@gmail.com \
    --cc=thomas.petazzoni@free-electrons.com \
    --cc=zmxu@marvell.com \
    --subject='Re: [PATCH 5/9] mtd: pxa3xx_nand: add support for the Marvell Berlin nand controller' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).