Re: [PATCH] nand: sunxi: fix write to USER_DATA reg

[Brian Norris <computersforpeace@gmail.com>]

Hi Boris,

On Mon, Aug 24, 2015 at 11:56:30AM +0200, Boris Brezillon wrote:
> On Thu, 30 Jul 2015 12:01:06 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > The USER_DATA register cannot be accessed using byte accessors on A13
> > SoCs, thus triggering a bug when using memcpy_toio on this register.
> > Declare a temporary u32 variable to store the USER_DATA value and access
> > the register with writel.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> Could you consider taking this patch for 4.3 (if it's too late for
> 4.3-rc1, could you queue it for -rc2)?

Sorry, didn't notice this until after my pull request. I can take some
version of this for 4.3-rcX for sure. Remains to be seen which one...

> BTW, I'd like to add
> 
> Cc: <stable@vger.kernel.org> # 3.19+
> 
> should I resend a new version or could you add it while you're applying
> the patch on your branch?

I can add it if I take this one, or you can add it yourself if we make
it to v2.

> Thanks,
> 
> Boris
> 
> > ---
> >  drivers/mtd/nand/sunxi_nand.c | 24 +++++++++++++-----------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> > index 6f93b29..5e374ab 100644
> > --- a/drivers/mtd/nand/sunxi_nand.c
> > +++ b/drivers/mtd/nand/sunxi_nand.c
> > @@ -624,6 +624,8 @@ static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
> >  	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
> >  
> >  	for (i = 0; i < ecc->steps; i++) {
> > +		u32 user_data;
> > +
> >  		if (i)
> >  			chip->cmdfunc(mtd, NAND_CMD_RNDIN, i * ecc->size, -1);
> >  
> > @@ -632,16 +634,16 @@ static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
> >  		offset = layout->eccpos[i * ecc->bytes] - 4 + mtd->writesize;
> >  
> >  		/* Fill OOB data in */
> > -		if (oob_required) {
> > -			tmp = 0xffffffff;
> > -			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
> > -				    4);
> > +		if (!oob_required) {

Hmm, seems like you're flipping the condition:

  oob_required --> !oob_required

That looks like the right change, but it's not mentioned in the commit
message.

> > +			user_data = 0xffffffff;

While we're at it: this oob_required dance looks a little unnecessary.
If (as it seems here) you need to fill in the OOB data with *something*
(even if it's just 0xffffffff), then it's safe to just ignore
the oob_required parameter and just use chip->oob_poi, which will have
been filled with 0xff. (The semantic meaning is that even if programming
the OOB from ->oob_poi is "not required", it is still allowed.)

That would help simplify this code too, I think, as you can eliminate
the !oob_required case.

> >  		} else {
> > -			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE,
> > -				    chip->oob_poi + offset - mtd->writesize,
> > -				    4);
> > +			memcpy(&user_data,
> > +			       chip->oob_poi + layout->oobfree[i].offset, 4);
> > +			user_data = le32_to_cpu(user_data);

sparse doesn't like this (here and below):

drivers/mtd/nand/sunxi_nand.c:656:37: warning: cast to restricted __le32 [sparse]
drivers/mtd/nand/sunxi_nand.c:793:31: warning: cast to restricted __le32 [sparse]

Seems you're trying to shoehorn yourself into using writel(), when you
don't actually want the endian swapping. I think this is a valid case
for using __raw_writel(), actually (Arnd even noted this last time we
discussed the "right" way to use some of these accessors). Maybe put a
comment for it too.

> >  		}
> >  
> > +		writel(user_data, nfc->regs + NFC_REG_USER_DATA_BASE);
> > +
> >  		chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
> >  
> >  		ret = sunxi_nfc_wait_cmd_fifo_empty(nfc);
> > @@ -772,13 +774,13 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
> >  		/* Fill OOB data in */
> >  		if (oob_required) {
> >  			tmp = 0xffffffff;
> > -			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
> > -				    4);
> >  		} else {
> > -			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, oob,
> > -				    4);
> > +			memcpy(&tmp, oob, sizeof(tmp));
> > +			tmp = le32_to_cpu(tmp);
> >  		}
> >  
> > +		writel(tmp, nfc->regs + NFC_REG_USER_DATA_BASE);
> > +
> >  		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
> >  		      (1 << 30);
> >  		writel(tmp, nfc->regs + NFC_REG_CMD);

Brian