From: Boris Brezillon <boris.brezillon@bootlin.com>
To: <christophe.kerello@st.com>
Cc: <miquel.raynal@bootlin.com>, <richard@nod.at>,
<dwmw2@infradead.org>, <computersforpeace@gmail.com>,
<marek.vasut@gmail.com>, <robh+dt@kernel.org>,
<mark.rutland@arm.com>, <linux-mtd@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-stm32@st-md-mailman.stormreply.com>
Subject: Re: [PATCH v2 2/3] mtd: rawnand: stm32_fmc2: add STM32 FMC2 NAND flash controller driver
Date: Mon, 5 Nov 2018 17:39:05 +0100 [thread overview]
Message-ID: <20181105173905.385dd06e@bbrezillon> (raw)
In-Reply-To: <1538732520-2800-3-git-send-email-christophe.kerello@st.com>
Hi Christophe,
On Fri, 5 Oct 2018 11:41:59 +0200
<christophe.kerello@st.com> wrote:
A few more comments.
> +/* Sequencer read/write configuration */
> +static void stm32_fmc2_rw_page_init(struct nand_chip *chip, int page,
> + int raw, bool write_data)
> +{
> + struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + u32 csqcfgr1, csqcfgr2, csqcfgr3;
> + u32 csqar1, csqar2;
> + u32 ecc_offset = mtd->writesize + FMC2_BBM_LEN;
> + u32 pcr = readl_relaxed(fmc2->io_base + FMC2_PCR);
> +
> + if (write_data)
> + pcr |= FMC2_PCR_WEN;
> + else
> + pcr &= ~FMC2_PCR_WEN;
> + writel_relaxed(pcr, fmc2->io_base + FMC2_PCR);
> +
> + /*
> + * - Set Program Page/Page Read command
> + * - Enable DMA request data
> + * - Set timings
> + */
> + csqcfgr1 = FMC2_CSQCFGR1_DMADEN | FMC2_CSQCFGR1_CMD1T;
> + if (write_data)
> + csqcfgr1 |= FMC2_CSQCFGR1_CMD1(NAND_CMD_SEQIN);
> + else
> + csqcfgr1 |= FMC2_CSQCFGR1_CMD1(NAND_CMD_READ0) |
> + FMC2_CSQCFGR1_CMD2EN |
> + FMC2_CSQCFGR1_CMD2(NAND_CMD_READSTART) |
> + FMC2_CSQCFGR1_CMD2T;
> +
> + /*
> + * - Set Random Data Input/Random Data Read command
> + * - Enable the sequencer to access the Spare data area
> + * - Enable DMA request status decoding for read
> + * - Set timings
> + */
> + if (write_data)
> + csqcfgr2 = FMC2_CSQCFGR2_RCMD1(NAND_CMD_RNDIN);
> + else
> + csqcfgr2 = FMC2_CSQCFGR2_RCMD1(NAND_CMD_RNDOUT) |
> + FMC2_CSQCFGR2_RCMD2EN |
> + FMC2_CSQCFGR2_RCMD2(NAND_CMD_RNDOUTSTART) |
> + FMC2_CSQCFGR2_RCMD1T |
> + FMC2_CSQCFGR2_RCMD2T;
> + if (!raw) {
> + csqcfgr2 |= write_data ? 0 : FMC2_CSQCFGR2_DMASEN;
> + csqcfgr2 |= FMC2_CSQCFGR2_SQSDTEN;
> + }
> +
> + /*
> + * - Set the number of sectors to be written
> + * - Set timings
> + */
> + csqcfgr3 = FMC2_CSQCFGR3_SNBR(chip->ecc.steps - 1);
> + if (write_data) {
> + csqcfgr3 |= FMC2_CSQCFGR3_RAC2T;
> + if (chip->chipsize > SZ_128M)
> + csqcfgr3 |= FMC2_CSQCFGR3_AC5T;
> + else
> + csqcfgr3 |= FMC2_CSQCFGR3_AC4T;
> + }
> +
> + /*
> + * Set the fourth first address cycles
> + * Byte 1 and byte 2 => column, we start at 0x0
> + * Byte 3 and byte 4 => page
> + */
> + csqar1 = FMC2_CSQCAR1_ADDC3(page);
> + csqar1 |= FMC2_CSQCAR1_ADDC4(page >> 8);
> +
> + /*
> + * - Set chip enable number
> + * - Set ecc byte offset in the spare area
> + * - Calculate the number of address cycles to be issued
> + * - Set byte 5 of address cycle if needed
> + */
> + csqar2 = FMC2_CSQCAR2_NANDCEN(fmc2->cs_sel);
> + if (chip->options & NAND_BUSWIDTH_16)
> + csqar2 |= FMC2_CSQCAR2_SAO(ecc_offset >> 1);
> + else
> + csqar2 |= FMC2_CSQCAR2_SAO(ecc_offset);
> + if (chip->chipsize > SZ_128M) {
Can you use
if (chip->options & NAND_ROW_ADDR_3)
instead?
> + csqcfgr1 |= FMC2_CSQCFGR1_ACYNBR(5);
> + csqar2 |= FMC2_CSQCAR2_ADDC5(page >> 16);
> + } else {
> + csqcfgr1 |= FMC2_CSQCFGR1_ACYNBR(4);
> + }
[...]
> +
> +void stm32_fmc2_write_data(struct nand_chip *chip, const void *buf,
> + unsigned int len, bool force_8bit)
> +{
> + struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
> + void __iomem *io_addr_w = fmc2->data_base[fmc2->cs_sel];
> + const u8 *p = buf;
> + unsigned int i;
> +
> + if (force_8bit)
> + goto write_8bit;
> +
> + if (IS_ALIGNED(len, sizeof(u32))) {
> + const u32 *p = buf;
I'm pretty sure the framework provides no alignment guarantee on buf.
You'd better assume buf might not be aligned on 32 or 16 bits.
> +
> + len /= sizeof(u32);
> + for (i = 0; i < len; i++)
> + writel_relaxed(p[i], io_addr_w);
> + return;
> + }
> +
> + if (chip->options & NAND_BUSWIDTH_16) {
> + const u16 *p = buf;
> +
> + len /= sizeof(u16);
> + for (i = 0; i < len; i++)
> + writew_relaxed(p[i], io_addr_w);
> + return;
> + }
> +
> +write_8bit:
> + for (i = 0; i < len; i++)
> + writeb_relaxed(p[i], io_addr_w);
Is 8bit access really enforced by the byte accessor? In this case, how
can you be sure 32-bit accesses are doing the right thing? Isn't there
a bit somewhere in the config reg to configure the bus width?
> +}
Regards,
Boris
next prev parent reply other threads:[~2018-11-05 16:39 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-05 9:41 [PATCH v2 0/3] mtd: rawnand: add STM32 FMC2 NAND flash controller driver christophe.kerello
2018-10-05 9:41 ` [PATCH v2 1/3] dt-bindings: mtd: stm32_fmc2: add STM32 FMC2 NAND controller documentation christophe.kerello
2018-10-05 9:53 ` Boris Brezillon
2018-10-12 20:32 ` Rob Herring
2018-10-15 7:39 ` Christophe Kerello
2018-10-05 9:41 ` [PATCH v2 2/3] mtd: rawnand: stm32_fmc2: add STM32 FMC2 NAND flash controller driver christophe.kerello
2018-10-05 9:58 ` Boris Brezillon
2018-10-15 8:09 ` Christophe Kerello
2018-11-05 16:39 ` Boris Brezillon [this message]
2018-11-07 11:08 ` Christophe Kerello
2018-11-07 12:23 ` Boris Brezillon
2018-11-07 12:26 ` Miquel Raynal
2018-10-05 9:42 ` [PATCH v2 3/3] mtd: rawnand: stm32_fmc2: add manual mode christophe.kerello
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=20181105173905.385dd06e@bbrezillon \
--to=boris.brezillon@bootlin.com \
--cc=christophe.kerello@st.com \
--cc=computersforpeace@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=dwmw2@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=marek.vasut@gmail.com \
--cc=mark.rutland@arm.com \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=robh+dt@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 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).