From: Boris Brezillon <bbrezillon@kernel.org>
To: Emil Lenngren <emil.lenngren@gmail.com>
Cc: Stefan Roese <sr@denx.de>, Richard Weinberger <richard@nod.at>,
stable@vger.kernel.org, Marek Vasut <marek.vasut@gmail.com>,
linux-mtd@lists.infradead.org,
Miquel Raynal <miquel.raynal@bootlin.com>,
Brian Norris <computersforpeace@gmail.com>,
David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache
Date: Thu, 24 Jan 2019 17:33:57 +0100 [thread overview]
Message-ID: <20190124173357.2a4d7f13@bbrezillon> (raw)
In-Reply-To: <CAO1O6scZ12d-5rLnakDy0Wefg4fw=dGS3jPoJtqKztsdMrsjhw@mail.gmail.com>
On Thu, 24 Jan 2019 17:16:37 +0100
Emil Lenngren <emil.lenngren@gmail.com> wrote:
> Hi,
>
> Den tors 24 jan. 2019 kl 16:28 skrev Stefan Roese <sr@denx.de>:
> >
> > On 24.01.19 15:20, Boris Brezillon wrote:
> > > Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> > > the cache content to 0xFF (depends on vendor implementation), so we
> > > must fill the page cache entirely even if we only want to program the
> > > data portion of the page, otherwise we might corrupt the BBM or user
> > > data previously programmed in OOB area.
> > >
> > > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> > > Reported-by: Stefan Roese <sr@denx.de>
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
> >
> > Works fine (limited testing only yet), so:
> >
> > Tested-by: Stefan Roese <sr@denx.de>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> >
> > Thanks,
> > Stefan
> >
>
> Can this quirk be made vendor specific?
We can make it vendor specific, as long as it's an opt-in thing. This
way, the default behavior is the safest one, and only when we know a
chip does reset the cache content on a PROGRAM LOAD time can we add this
flag.
> It seems a waste of SPI
> transfer cycles to write 0xff to the whole OOB area when we only want
> to program the data area if the chip doesn't need this quirk.
> For which logic is this needed anyway? According to the GigaDevice
> datasheet, if a Program Load is followed by Program Execute,
> "uninitialized" bytes will be set to 0xff, which is the flow that is
> used by spinand core.
Except you're not guaranteed that the controller can fill the page in a
single spi_mem_exec_op() call (because of FIFO size limitations), so
you might have one PROGRAM LOAD followed by several PROGRAM RANDOM LOAD.
I guess most manufacturers reset the cache content at PROGRAM LOAD
time, but GigaDevice does not.
>
> /Emil
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
WARNING: multiple messages have this Message-ID (diff)
From: Boris Brezillon <bbrezillon@kernel.org>
To: Emil Lenngren <emil.lenngren@gmail.com>
Cc: Stefan Roese <sr@denx.de>, Richard Weinberger <richard@nod.at>,
Miquel Raynal <miquel.raynal@bootlin.com>,
linux-mtd@lists.infradead.org,
Marek Vasut <marek.vasut@gmail.com>,
Brian Norris <computersforpeace@gmail.com>,
David Woodhouse <dwmw2@infradead.org>,
stable@vger.kernel.org
Subject: Re: [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache
Date: Thu, 24 Jan 2019 17:33:57 +0100 [thread overview]
Message-ID: <20190124173357.2a4d7f13@bbrezillon> (raw)
In-Reply-To: <CAO1O6scZ12d-5rLnakDy0Wefg4fw=dGS3jPoJtqKztsdMrsjhw@mail.gmail.com>
On Thu, 24 Jan 2019 17:16:37 +0100
Emil Lenngren <emil.lenngren@gmail.com> wrote:
> Hi,
>
> Den tors 24 jan. 2019 kl 16:28 skrev Stefan Roese <sr@denx.de>:
> >
> > On 24.01.19 15:20, Boris Brezillon wrote:
> > > Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> > > the cache content to 0xFF (depends on vendor implementation), so we
> > > must fill the page cache entirely even if we only want to program the
> > > data portion of the page, otherwise we might corrupt the BBM or user
> > > data previously programmed in OOB area.
> > >
> > > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> > > Reported-by: Stefan Roese <sr@denx.de>
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
> >
> > Works fine (limited testing only yet), so:
> >
> > Tested-by: Stefan Roese <sr@denx.de>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> >
> > Thanks,
> > Stefan
> >
>
> Can this quirk be made vendor specific?
We can make it vendor specific, as long as it's an opt-in thing. This
way, the default behavior is the safest one, and only when we know a
chip does reset the cache content on a PROGRAM LOAD time can we add this
flag.
> It seems a waste of SPI
> transfer cycles to write 0xff to the whole OOB area when we only want
> to program the data area if the chip doesn't need this quirk.
> For which logic is this needed anyway? According to the GigaDevice
> datasheet, if a Program Load is followed by Program Execute,
> "uninitialized" bytes will be set to 0xff, which is the flow that is
> used by spinand core.
Except you're not guaranteed that the controller can fill the page in a
single spi_mem_exec_op() call (because of FIFO size limitations), so
you might have one PROGRAM LOAD followed by several PROGRAM RANDOM LOAD.
I guess most manufacturers reset the cache content at PROGRAM LOAD
time, but GigaDevice does not.
>
> /Emil
next prev parent reply other threads:[~2019-01-24 16:34 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-24 14:20 [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache Boris Brezillon
2019-01-24 14:20 ` Boris Brezillon
2019-01-24 15:25 ` Stefan Roese
2019-01-24 15:25 ` Stefan Roese
2019-01-24 16:16 ` Emil Lenngren
2019-01-24 16:16 ` Emil Lenngren
2019-01-24 16:33 ` Boris Brezillon [this message]
2019-01-24 16:33 ` Boris Brezillon
2019-01-25 11:09 ` Miquel Raynal
2019-01-25 11:09 ` Miquel Raynal
2019-01-25 16:08 ` Boris Brezillon
2019-01-25 16:08 ` Boris Brezillon
2019-01-28 9:55 ` Miquel Raynal
2019-01-28 9:55 ` Miquel Raynal
2019-01-31 11:32 ` Boris Brezillon
2019-01-31 11:32 ` Boris Brezillon
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=20190124173357.2a4d7f13@bbrezillon \
--to=bbrezillon@kernel.org \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=emil.lenngren@gmail.com \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=sr@denx.de \
--cc=stable@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.