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 at 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 at 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 at 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