| Message ID | 1538732520-2800-1-git-send-email-christophe.kerello@st.com |
|---|---|
| Headers | show |
| Series | mtd: rawnand: add STM32 FMC2 NAND flash controller driver | expand |
On Fri, 5 Oct 2018 11:41:59 +0200 <christophe.kerello@st.com> wrote: > +struct stm32_fmc2 { You should inherit from nand_controller even if the nand_chip already embeds a dummy nand controller object. struct nand_controller base; > + struct stm32_fmc2_nand nand; > + struct device *dev; > + void __iomem *io_base; > + void __iomem *data_base[FMC2_MAX_CE]; > + void __iomem *cmd_base[FMC2_MAX_CE]; > + void __iomem *addr_base[FMC2_MAX_CE]; > + phys_addr_t io_phys_addr; > + phys_addr_t data_phys_addr[FMC2_MAX_CE]; > + struct clk *clk; > + > + struct dma_chan *dma_tx_ch; > + struct dma_chan *dma_rx_ch; > + struct dma_chan *dma_ecc_ch; > + struct sg_table dma_data_sg; > + struct sg_table dma_ecc_sg; > + u8 *ecc_buf; > + int dma_ecc_len; > + > + struct completion complete; > + struct completion dma_data_complete; > + struct completion dma_ecc_complete; > + > + u8 cs_assigned; > + int cs_sel; > +};
On 10/05/2018 11:58 AM, Boris Brezillon wrote: > On Fri, 5 Oct 2018 11:41:59 +0200 > <christophe.kerello@st.com> wrote: > >> +struct stm32_fmc2 { > > You should inherit from nand_controller even if the nand_chip already > embeds a dummy nand controller object. > > struct nand_controller base; > Hi Boris, Ok, i will add and handle the nand controller structure. Regards, Christophe Kerello. >> + struct stm32_fmc2_nand nand; >> + struct device *dev; >> + void __iomem *io_base; >> + void __iomem *data_base[FMC2_MAX_CE]; >> + void __iomem *cmd_base[FMC2_MAX_CE]; >> + void __iomem *addr_base[FMC2_MAX_CE]; >> + phys_addr_t io_phys_addr; >> + phys_addr_t data_phys_addr[FMC2_MAX_CE]; >> + struct clk *clk; >> + >> + struct dma_chan *dma_tx_ch; >> + struct dma_chan *dma_rx_ch; >> + struct dma_chan *dma_ecc_ch; >> + struct sg_table dma_data_sg; >> + struct sg_table dma_ecc_sg; >> + u8 *ecc_buf; >> + int dma_ecc_len; >> + >> + struct completion complete; >> + struct completion dma_data_complete; >> + struct completion dma_ecc_complete; >> + >> + u8 cs_assigned; >> + int cs_sel; >> +};
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
Hi Boris, On 11/5/18 5:39 PM, Boris Brezillon wrote: > 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? > Yes, it will part of v3. >> + 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? > I have checked the framework after Miquèl comment sent on v1 => "If you selected BOUNCE_BUFFER in the options, buf is supposedly aligned, or am I missing something?". After checking the framework, my understanding was: - In case of 8-bit access is requested, the framework provides no guarantee on buf. To avoid any issue, I write byte per byte. - In case of 8-bit access is not requested, it means that the framework will try to write data in the page or in the oob. When writing to oob, chip->oob_poi will be used and this buffer is aligned. When writing to the page, as the driver enables NAND_USE_BOUNCE_BUFFER option, buf is guarantee aligned. But, I agree that it would be safe to reconfigure the bus width in 8-bit before writing byte per byte in case of a 16-bit NAND is used. write_8bit: if (chip->options & NAND_BUSWIDTH_16) { /* Reconfigure bus width to 8-bit */ pcr = readl_relaxed(fmc2->io_base + FMC2_PCR); pcr &= ~FMC2_PCR_PWID_MASK; writel_relaxed(pcr, fmc2->io_base + FMC2_PCR); } for (i = 0; i < len; i++) writeb_relaxed(p[i], io_addr_w); if (chip->options & NAND_BUSWIDTH_16) { /* Reconfigure bus width to 16-bit */ pcr = readl_relaxed(fmc2->io_base + FMC2_PCR); pcr |= FMC2_PCR_PWID(FMC2_PCR_PWID_BUSWIDTH_16); writel_relaxed(pcr, fmc2->io_base + FMC2_PCR); } Regards, Christophe Kerello. >> +} > > Regards, > > Boris >
On Wed, 7 Nov 2018 12:08:58 +0100 Christophe Kerello <christophe.kerello@st.com> wrote: > >> + > >> +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? > > > > I have checked the framework after Miquèl comment sent on v1 => "If you > selected BOUNCE_BUFFER in the options, buf is supposedly > aligned, or am I missing something?". > > After checking the framework, my understanding was: > - In case of 8-bit access is requested, the framework provides no > guarantee on buf. To avoid any issue, I write byte per byte. > - In case of 8-bit access is not requested, it means that the > framework will try to write data in the page or in the oob. When writing > to oob, chip->oob_poi will be used and this buffer is aligned. When > writing to the page, as the driver enables NAND_USE_BOUNCE_BUFFER > option, buf is guarantee aligned. It's probably what happens right now, but there's no guarantee that all non-8-bit accesses will be provided a 32-bit aligned buffer. The only guarantee we provide is on buffer passed to the chip->ecc.read/write_xxx() hooks, and ->exec_op() can be used outside of the "page access" path. > > But, I agree that it would be safe to reconfigure the bus width in 8-bit > before writing byte per byte in case of a 16-bit NAND is used. Yes, and I also think you should not base your is-aligned check on the force_8bit value. Use IS_ALIGNED() instead.
Hi Christophe, Boris Brezillon <boris.brezillon@bootlin.com> wrote on Wed, 7 Nov 2018 13:23:42 +0100: > On Wed, 7 Nov 2018 12:08:58 +0100 > Christophe Kerello <christophe.kerello@st.com> wrote: > > > >> + > > >> +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? > > > > > > > I have checked the framework after Miquèl comment sent on v1 => "If you > > selected BOUNCE_BUFFER in the options, buf is supposedly > > aligned, or am I missing something?". > > > > After checking the framework, my understanding was: > > - In case of 8-bit access is requested, the framework provides no > > guarantee on buf. To avoid any issue, I write byte per byte. > > - In case of 8-bit access is not requested, it means that the > > framework will try to write data in the page or in the oob. When writing > > to oob, chip->oob_poi will be used and this buffer is aligned. When > > writing to the page, as the driver enables NAND_USE_BOUNCE_BUFFER > > option, buf is guarantee aligned. > > It's probably what happens right now, but there's no guarantee that all > non-8-bit accesses will be provided a 32-bit aligned buffer. The only > guarantee we provide is on buffer passed to the > chip->ecc.read/write_xxx() hooks, and ->exec_op() can be used outside > of the "page access" path. > > > > > But, I agree that it would be safe to reconfigure the bus width in 8-bit > > before writing byte per byte in case of a 16-bit NAND is used. > > Yes, and I also think you should not base your is-aligned check on the > force_8bit value. Use IS_ALIGNED() instead. Maybe the "configure the bus in 8/16-bit" blocks could deserve a helper. There is probably other locations within this driver with the same logic? Thanks, Miquèl
From: Christophe Kerello <christophe.kerello@st.com> This patchset adds the support for the STMicroelectronics FMC2 NAND flash controller found on STM32MP SOCs. This patchset supports: - the command sequencer feature, a hardware accelerator for read/write within a page - the manual mode feature, useful for debug purpose - a maximum 8k page size - following ECC strength and step size - nand-ecc-strength = <8>, nand-ecc-step-size = <512> (BCH8) - nand-ecc-strength = <4>, nand-ecc-step-size = <512> (BCH4) - nand-ecc-strength = <1>, nand-ecc-step-size = <512> (Extended ecc based on HAMMING) This patchset has been tested on Micron MT29F8G08ABACAH4 (8-bit SLC NAND) and MT29F8G16ABACAH4 (16-bit SLC NAND) Changes v2: - separate the controller structure and the NAND chip structure - remove timings description from the device tree - fix typo issues - fix kbuildrobot issues Christophe Kerello (3): dt-bindings: mtd: stm32_fmc2: add STM32 FMC2 NAND controller documentation mtd: rawnand: stm32_fmc2: add STM32 FMC2 NAND flash controller driver mtd: rawnand: stm32_fmc2: add manual mode .../devicetree/bindings/mtd/stm32-fmc2-nand.txt | 59 + drivers/mtd/nand/raw/Kconfig | 9 + drivers/mtd/nand/raw/Makefile | 1 + drivers/mtd/nand/raw/stm32_fmc2_nand.c | 1997 ++++++++++++++++++++ 4 files changed, 2066 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/stm32-fmc2-nand.txt create mode 100644 drivers/mtd/nand/raw/stm32_fmc2_nand.c