From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut@gmail.com>
Date: Wed, 10 Aug 2011 01:15:19 +0200
Subject: [U-Boot] [PATCH] NAND: Allow per-buffer allocation
In-Reply-To: <4E41B6A9.1030701@freescale.com>
References: <1312926890-21361-1-git-send-email-marek.vasut@gmail.com>
	<4E41B6A9.1030701@freescale.com>
Message-ID: <201108100115.19953.marek.vasut@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Wednesday, August 10, 2011 12:37:29 AM Scott Wood wrote:
> On 08/09/2011 04:54 PM, Marek Vasut wrote:
> > Don't allocate NAND buffers as one block, but allocate them separately.
> > This allows systems where DMA to buffers happen to allocate these
> > buffers properly aligned.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> 
> That second sentence is hard to parse -- I think you mean something
> like, "This accommodates drivers which DMA to the buffers and have
> alignment constraints."

Yes, something like that. Sorry, it's 1.14 PM here.

> 
> Will a similar change be needed in Linux?

I'm not sure how much in sync we are with linux here. It'd be worth looking at.

> 
> >  int nand_scan_tail(struct mtd_info *mtd)
> >  {
> > 
> > -	int i;
> > +	int i, bufsize;
> > +	uint8_t *buf;
> > 
> >  	struct nand_chip *chip = mtd->priv;
> > 
> > -	if (!(chip->options & NAND_OWN_BUFFERS))
> > -		chip->buffers = kmalloc(sizeof(*chip->buffers), GFP_KERNEL);
> > -	if (!chip->buffers)
> > +	if (!(chip->options & NAND_OWN_BUFFERS)) {
> > +		chip->buffers = malloc(sizeof(struct nand_buffers));
> > +		if (!chip->buffers)
> > +			return -ENOMEM;
> 
> Why does the struct itself need to be dynamically allocated?

That was in the NOTE: ... to avoid breaking drivers. We can have that changed, 
but that'd be much more intrussive.
> 
> > +
> > +		bufsize = NAND_MAX_PAGESIZE + (3 * NAND_MAX_OOBSIZE);
> > +		buf = malloc(bufsize);
> > +
> > +		chip->buffers->buffer = (struct nand_buffers *)buf;
> > +		chip->buffers->ecccalc = buf;
> > +		chip->buffers->ecccode = buf + NAND_MAX_OOBSIZE;
> > +		chip->buffers->databuf = buf + (2 * NAND_MAX_OOBSIZE);
> > +	}
> > +
> > +	if (!chip->buffers->buffer)
> > 
> >  		return -ENOMEM;
> 
> What does "buffer" mean now?  What would a driver that supplies its own
> completely separate ecccalc/ecccode/databuf buffers put in "buffer"?

Maybe that condition should go also into the if() statement above. What do you 
think ?

> 
> -Scott