Re: [PATCH v2 2/3] mtd: rawnand: stm32_fmc2: add STM32 FMC2 NAND flash controller driver

[Boris Brezillon]

Hi Christophe,

On Fri, 5 Oct 2018 11:41:59 +0200
<christophe.kerello@xxxxxx> 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