Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/3] mtd: rawnand: stm32_fmc2: add STM32 FMC2 NAND flash
 controller driver
To:     Miquel Raynal <miquel.raynal@bootlin.com>
CC:     <boris.brezillon@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>
References: <1537199260-7280-1-git-send-email-christophe.kerello@st.com>
 <1537199260-7280-3-git-send-email-christophe.kerello@st.com>
 <20180922154819.015dcca7@xps13>
From:   Christophe Kerello <christophe.kerello@st.com>
Message-ID: <6df8455e-2fb2-e65c-a492-fba42a9453f3@st.com>
Date:   Mon, 24 Sep 2018 18:36:36 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180922154819.015dcca7@xps13>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Miquèl,

On 09/22/2018 03:48 PM, Miquel Raynal wrote:
> Hi Christophe,
> 
> I suppose you received the kbuildrobot issues already, please have a
> look at them.

Yes, kbuidroot issues will be solved in the v2 patchset.

> 
> The driver looks well, some comments below.
> 
> <christophe.kerello@st.com> wrote on Mon, 17 Sep 2018 17:47:39 +0200:
> 
>> From: Christophe Kerello <christophe.kerello@st.com>
>>
>> The driver adds the support for the STMicroelectronics FMC2 NAND
>> Controller found on STM32MP SOCs.
>>
>> This patch is based on FMC2 command sequencer.
>> The purpose of the command sequencer is to facilitate the programming
>> and the reading of NAND flash pages with the ECC and to free the CPU
>> of sequencing tasks.
>> It requires one DMA channel for write and two DMA channels for read
>> operations.
>>
>> Only NAND_ECC_HW mode is actually supported.
>> The driver supports a maximum 8k page size.
>> The following ECC strength and step size are currently supported:
>>   - 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 patch has been tested on Micron MT29F8G08ABACAH4 and
>> MT29F8G16ABACAH4
>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
>> ---
>>   drivers/mtd/nand/raw/Kconfig           |    9 +
>>   drivers/mtd/nand/raw/Makefile          |    1 +
>>   drivers/mtd/nand/raw/stm32_fmc2_nand.c | 1729 ++++++++++++++++++++++++++++++++
>>   3 files changed, 1739 insertions(+)
>>   create mode 100644 drivers/mtd/nand/raw/stm32_fmc2_nand.c
>>
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index c7efc31..863662c 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -541,4 +541,13 @@ config MTD_NAND_TEGRA
>>   	  is supported. Extra OOB bytes when using HW ECC are currently
>>   	  not supported.
>>   
>> +config MTD_NAND_STM32_FMC2
>> +	tristate "Support for NAND controller on STM32MP SoCs"
>> +	depends on MACH_STM32MP157 || COMPILE_TEST
>> +	help
>> +	  Enables support for NAND Flash chips on SoCs containing the FMC2
>> +	  NAND controller. This controller is found on STM32MP SoCs.
>> +	  The driver supports a maximum 8k page size. HW ECC is enabled and
>> +	  supports a maximum 8-bit correction error per sector of 512 bytes.
> 
> HW ECC should not be enabled by default. 8-bit/512B of correctability
> is good but not that high and people might want to use software ECC in
> conjunction with raw accesses.

Yes, I agree. The driver only supports NAND_ECC_HW mode. NAND_ECC_SOFT 
mode was not requested by customers and was not implemented. The driver 
could be improved later to support mode like NAND_ECC_SOFT or 
NAND_ECC_ON_DIE. Should I remove "HW ECC is enabled" from Kconfig 
description?

> 
>> +
>>   endif # MTD_NAND
>> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>> index a6ef067..8c437e3 100644
>> --- a/drivers/mtd/nand/raw/Makefile
>> +++ b/drivers/mtd/nand/raw/Makefile
>> @@ -56,6 +56,7 @@ obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand/
>>   obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
>>   obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_ecc.o mtk_nand.o
>>   obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
>> +obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
>>   
>>   nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
>>   nand-objs += nand_amd.o
>> diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
>> new file mode 100644
>> index 0000000..dd5762a
>> --- /dev/null
>> +++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
>> @@ -0,0 +1,1729 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
>> + * Author: Christophe Kerello <christophe.kerello@st.com> for STMicroelectronics
> 
> I'm not sure the "All rights reserved" has a meaning here.
> 
> But you definitely not have to add "for STMicroelectronics" because it
> is already in the copyright.

Ok.

> 
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/errno.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/rawnand.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +
>> +/* Bad block marker length */
>> +#define FMC2_BBM_LEN			2
>> +
>> +/* ECC step size */
>> +#define FMC2_ECC_STEP_SIZE		512
>> +
>> +/* BCHDSRx registers length */
>> +#define FMC2_BCHDSRS_LEN		20
>> +
>> +/* HECCR length */
>> +#define FMC2_HECCR_LEN			4
>> +
>> +/* Max requests done for a 8k nand page size */
>> +#define FMC2_MAX_SG_COUNT		16
>> +
>> +/* Max chip enable */
>> +#define FMC2_MAX_CE			2
>> +
>> +/* Timings */
>> +#define FMC2_THIZ			1
>> +#define FMC2_TIO			8000
>> +#define FMC2_TSYNC			3000
>> +#define FMC2_PCR_TIMING_MASK		0xf
>> +#define FMC2_PMEM_PATT_TIMING_MASK	0xff
>> +
>> +/* FMC2 Controller Registers */
>> +#define FMC2_BCR1			0x0
>> +#define FMC2_PCR			0x80
>> +#define FMC2_SR				0x84
>> +#define FMC2_PMEM			0x88
>> +#define FMC2_PATT			0x8c
>> +#define FMC2_HECCR			0x94
>> +#define FMC2_CSQCR			0x200
>> +#define FMC2_CSQCFGR1			0x204
>> +#define FMC2_CSQCFGR2			0x208
>> +#define FMC2_CSQCFGR3			0x20c
>> +#define FMC2_CSQAR1			0x210
>> +#define FMC2_CSQAR2			0x214
>> +#define FMC2_CSQIER			0x220
>> +#define FMC2_CSQISR			0x224
>> +#define FMC2_CSQICR			0x228
>> +#define FMC2_CSQEMSR			0x230
>> +#define FMC2_BCHIER			0x250
>> +#define FMC2_BCHISR			0x254
>> +#define FMC2_BCHICR			0x258
>> +#define FMC2_BCHPBR1			0x260
>> +#define FMC2_BCHPBR2			0x264
>> +#define FMC2_BCHPBR3			0x268
>> +#define FMC2_BCHPBR4			0x26c
>> +#define FMC2_BCHDSR0			0x27c
>> +#define FMC2_BCHDSR1			0x280
>> +#define FMC2_BCHDSR2			0x284
>> +#define FMC2_BCHDSR3			0x288
>> +#define FMC2_BCHDSR4			0x28c
>> +
>> +/* Register: FMC2_BCR1 */
>> +#define FMC2_BCR1_FMC2EN		BIT(31)
>> +
>> +/* Register: FMC2_PCR */
>> +#define FMC2_PCR_PWAITEN		BIT(1)
>> +#define FMC2_PCR_PBKEN			BIT(2)
>> +#define FMC2_PCR_PWID_MASK		GENMASK(5, 4)
>> +#define FMC2_PCR_PWID(x)		(((x) & 0x3) << 4)
>> +#define FMC2_PCR_PWID_BUSWIDTH_8	0
>> +#define FMC2_PCR_PWID_BUSWIDTH_16	1
>> +#define FMC2_PCR_ECCEN			BIT(6)
>> +#define FMC2_PCR_ECCALG			BIT(8)
>> +#define FMC2_PCR_TCLR_MASK		GENMASK(12, 9)
>> +#define FMC2_PCR_TCLR(x)		(((x) & 0xf) << 9)
>> +#define FMC2_PCR_TCLR_DEFAULT		0xf
>> +#define FMC2_PCR_TAR_MASK		GENMASK(16, 13)
>> +#define FMC2_PCR_TAR(x)			(((x) & 0xf) << 13)
>> +#define FMC2_PCR_TAR_DEFAULT		0xf
>> +#define FMC2_PCR_ECCSS_MASK		GENMASK(19, 17)
>> +#define FMC2_PCR_ECCSS(x)		(((x) & 0x7) << 17)
>> +#define FMC2_PCR_ECCSS_512		1
>> +#define FMC2_PCR_ECCSS_2048		3
>> +#define FMC2_PCR_BCHECC			BIT(24)
>> +#define FMC2_PCR_WEN			BIT(25)
>> +
>> +/* Register: FMC2_SR */
>> +#define FMC2_SR_NWRF			BIT(6)
>> +
>> +/* Register: FMC2_PMEM */
>> +#define FMC2_PMEM_MEMSET(x)		(((x) & 0xff) << 0)
>> +#define FMC2_PMEM_MEMWAIT(x)		(((x) & 0xff) << 8)
>> +#define FMC2_PMEM_MEMHOLD(x)		(((x) & 0xff) << 16)
>> +#define FMC2_PMEM_MEMHIZ(x)		(((x) & 0xff) << 24)
>> +#define FMC2_PMEM_DEFAULT		0x0a0a0a0a
>> +
>> +/* Register: FMC2_PATT */
>> +#define FMC2_PATT_ATTSET(x)		(((x) & 0xff) << 0)
>> +#define FMC2_PATT_ATTWAIT(x)		(((x) & 0xff) << 8)
>> +#define FMC2_PATT_ATTHOLD(x)		(((x) & 0xff) << 16)
>> +#define FMC2_PATT_ATTHIZ(x)		(((x) & 0xff) << 24)
>> +#define FMC2_PATT_DEFAULT		0x0a0a0a0a
>> +
>> +/* Register: FMC2_CSQCR */
>> +#define FMC2_CSQCR_CSQSTART		BIT(0)
>> +
>> +/* Register: FMC2_CSQCFGR1 */
>> +#define FMC2_CSQCFGR1_CMD2EN		BIT(1)
>> +#define FMC2_CSQCFGR1_DMADEN		BIT(2)
>> +#define FMC2_CSQCFGR1_ACYNBR(x)		(((x) & 0x7) << 4)
>> +#define FMC2_CSQCFGR1_CMD1(x)		(((x) & 0xff) << 8)
>> +#define FMC2_CSQCFGR1_CMD2(x)		(((x) & 0xff) << 16)
>> +#define FMC2_CSQCFGR1_CMD1T		BIT(24)
>> +#define FMC2_CSQCFGR1_CMD2T		BIT(25)
>> +
>> +/* Register: FMC2_CSQCFGR2 */
>> +#define FMC2_CSQCFGR2_SQSDTEN		BIT(0)
>> +#define FMC2_CSQCFGR2_RCMD2EN		BIT(1)
>> +#define FMC2_CSQCFGR2_DMASEN		BIT(2)
>> +#define FMC2_CSQCFGR2_RCMD1(x)		(((x) & 0xff) << 8)
>> +#define FMC2_CSQCFGR2_RCMD2(x)		(((x) & 0xff) << 16)
>> +#define FMC2_CSQCFGR2_RCMD1T		BIT(24)
>> +#define FMC2_CSQCFGR2_RCMD2T		BIT(25)
>> +
>> +/* Register: FMC2_CSQCFGR3 */
>> +#define FMC2_CSQCFGR3_SNBR(x)		(((x) & 0x1f) << 8)
>> +#define FMC2_CSQCFGR3_AC1T		BIT(16)
>> +#define FMC2_CSQCFGR3_AC2T		BIT(17)
>> +#define FMC2_CSQCFGR3_AC3T		BIT(18)
>> +#define FMC2_CSQCFGR3_AC4T		BIT(19)
>> +#define FMC2_CSQCFGR3_AC5T		BIT(20)
>> +#define FMC2_CSQCFGR3_SDT		BIT(21)
>> +#define FMC2_CSQCFGR3_RAC1T		BIT(22)
>> +#define FMC2_CSQCFGR3_RAC2T		BIT(23)
>> +
>> +/* Register: FMC2_CSQCAR1 */
>> +#define FMC2_CSQCAR1_ADDC1(x)		(((x) & 0xff) << 0)
>> +#define FMC2_CSQCAR1_ADDC2(x)		(((x) & 0xff) << 8)
>> +#define FMC2_CSQCAR1_ADDC3(x)		(((x) & 0xff) << 16)
>> +#define FMC2_CSQCAR1_ADDC4(x)		(((x) & 0xff) << 24)
>> +
>> +/* Register: FMC2_CSQCAR2 */
>> +#define FMC2_CSQCAR2_ADDC5(x)		(((x) & 0xff) << 0)
>> +#define FMC2_CSQCAR2_NANDCEN(x)		(((x) & 0x3) << 10)
>> +#define FMC2_CSQCAR2_SAO(x)		(((x) & 0xffff) << 16)
>> +
>> +/* Register: FMC2_CSQIER */
>> +#define FMC2_CSQIER_TCIE		BIT(0)
>> +
>> +/* Register: FMC2_CSQICR */
>> +#define FMC2_CSQICR_CLEAR_IRQ		GENMASK(4, 0)
>> +
>> +/* Register: FMC2_CSQEMSR */
>> +#define FMC2_CSQEMSR_SEM		GENMASK(15, 0)
>> +
>> +/* Register: FMC2_BCHIER */
>> +#define FMC2_BCHIER_DERIE		BIT(1)
>> +#define FMC2_BCHIER_EPBRIE		BIT(4)
>> +
>> +/* Register: FMC2_BCHICR */
>> +#define FMC2_BCHICR_CLEAR_IRQ		GENMASK(4, 0)
>> +
>> +/* Register: FMC2_BCHDSR0 */
>> +#define FMC2_BCHDSR0_DUE		BIT(0)
>> +#define FMC2_BCHDSR0_DEF		BIT(1)
>> +#define FMC2_BCHDSR0_DEN_MASK		GENMASK(7, 4)
>> +#define FMC2_BCHDSR0_DEN_SHIFT		4
>> +
>> +/* Register: FMC2_BCHDSR1 */
>> +#define FMC2_BCHDSR1_EBP1_MASK		GENMASK(12, 0)
>> +#define FMC2_BCHDSR1_EBP2_MASK		GENMASK(28, 16)
>> +#define FMC2_BCHDSR1_EBP2_SHIFT		16
>> +
>> +/* Register: FMC2_BCHDSR2 */
>> +#define FMC2_BCHDSR2_EBP3_MASK		GENMASK(12, 0)
>> +#define FMC2_BCHDSR2_EBP4_MASK		GENMASK(28, 16)
>> +#define FMC2_BCHDSR2_EBP4_SHIFT		16
>> +
>> +/* Register: FMC2_BCHDSR3 */
>> +#define FMC2_BCHDSR3_EBP5_MASK		GENMASK(12, 0)
>> +#define FMC2_BCHDSR3_EBP6_MASK		GENMASK(28, 16)
>> +#define FMC2_BCHDSR3_EBP6_SHIFT		16
>> +
>> +/* Register: FMC2_BCHDSR4 */
>> +#define FMC2_BCHDSR4_EBP7_MASK		GENMASK(12, 0)
>> +#define FMC2_BCHDSR4_EBP8_MASK		GENMASK(28, 16)
>> +#define FMC2_BCHDSR4_EBP8_SHIFT		16
>> +
>> +enum stm32_fmc2_ecc {
>> +	FMC2_ECC_HAM = 1,
>> +	FMC2_ECC_BCH4 = 4,
>> +	FMC2_ECC_BCH8 = 8
>> +};
>> +
>> +struct stm32_fmc2_timings {
>> +	u8 tclr;
>> +	u8 tar;
>> +	u8 thiz;
>> +	u8 twait;
>> +	u8 thold_mem;
>> +	u8 tset_mem;
>> +	u8 thold_att;
>> +	u8 tset_att;
>> +};
>> +
>> +struct stm32_fmc2 {
>> +	struct nand_chip chip;
>> +	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;
>> +
>> +	struct stm32_fmc2_timings timings;
>> +	u8 cs_assigned;
>> +	int cs_sel;
>> +	int ncs;
>> +	int cs_used[FMC2_MAX_CE];
>> +};
>> +
>> +/* Enable irq sources in case of the sequencer is used */
>> +static inline void stm32_fmc2_enable_seq_irq(struct stm32_fmc2 *fmc2)
>> +{
>> +	u32 csqier = readl_relaxed(fmc2->io_base + FMC2_CSQIER);
>> +
>> +	csqier |= FMC2_CSQIER_TCIE;
>> +
>> +	writel_relaxed(csqier, fmc2->io_base + FMC2_CSQIER);
>> +}
>> +
>> +/* Disable irq sources in case of the sequencer is used */
>> +static inline void stm32_fmc2_disable_seq_irq(struct stm32_fmc2 *fmc2)
>> +{
>> +	u32 csqier = readl_relaxed(fmc2->io_base + FMC2_CSQIER);
>> +
>> +	csqier &= ~FMC2_CSQIER_TCIE;
>> +
>> +	writel_relaxed(csqier, fmc2->io_base + FMC2_CSQIER);
>> +}
>> +
>> +/* Clear irq sources in case of the sequencer is used */
>> +static inline void stm32_fmc2_clear_seq_irq(struct stm32_fmc2 *fmc2)
>> +{
>> +	writel_relaxed(FMC2_CSQICR_CLEAR_IRQ, fmc2->io_base + FMC2_CSQICR);
>> +}
>> +
>> +/* Select function */
>> +static void stm32_fmc2_select_chip(struct nand_chip *chip, int chipnr)
>> +{
>> +	struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
>> +	struct dma_slave_config dma_cfg;
>> +
>> +	if (chipnr < 0 || chipnr >= fmc2->ncs)
>> +		return;
>> +
>> +	if (fmc2->cs_used[chipnr] == fmc2->cs_sel)
>> +		return;
>> +
>> +	fmc2->cs_sel = fmc2->cs_used[chipnr];
>> +
>> +	if (fmc2->dma_tx_ch && fmc2->dma_rx_ch) {
>> +		memset(&dma_cfg, 0, sizeof(dma_cfg));
>> +		dma_cfg.src_addr = fmc2->data_phys_addr[fmc2->cs_sel];
>> +		dma_cfg.dst_addr = fmc2->data_phys_addr[fmc2->cs_sel];
>> +		dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +		dma_cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +		dma_cfg.src_maxburst = 32;
>> +		dma_cfg.dst_maxburst = 32;
>> +
>> +		if (dmaengine_slave_config(fmc2->dma_tx_ch, &dma_cfg))
>> +			dev_warn(fmc2->dev, "tx DMA engine slave config failed\n");
>> +
>> +		if (dmaengine_slave_config(fmc2->dma_rx_ch, &dma_cfg))
>> +			dev_warn(fmc2->dev, "rx DMA engine slave config failed\n");
>> +	}
> 
> What if there are two NAND chips using different timing modes? You
> should probably reconfigure the timings registers, unless there are
> already a set of timing registers per CS?

Yes, it's true. In case of 2 NAND chips, timings and pcr registers 
should have been reconfigured. But, the driver only supports one NAND 
chip connected to 1 or 2 CS. There was no requirement on our side to 
support 2 different NAND chips. I do not have a board to test such 
configuration, so i do not want to deliver this support without being 
able to test it on my side. The driver will be improved later to support 
2 different NAND chips, in case this configuration is requested by 
customers.

>> +}
>> +
>> +/*
>> + * ECC HAMMING calculation
>> + * ECC is 3 bytes for 512 bytes of data (supports error correction up to
>> + * max of 1-bit)
>> + */
>> +static inline void stm32_fmc2_ham_set_ecc(const u32 ecc_sta, u8 *ecc)
>> +{
>> +	ecc[0] = ecc_sta;
>> +	ecc[1] = ecc_sta >> 8;
>> +	ecc[2] = ecc_sta >> 16;
>> +}
>> +
>> +static int stm32_fmc2_ham_correct(struct nand_chip *chip, uint8_t *dat,
>> +				  uint8_t *read_ecc, uint8_t *calc_ecc)
>> +{
>> +	u8 bit_position = 0, b0, b1, b2;
>> +	u32 byte_addr = 0, b;
>> +	u32 i, shifting = 1;
>> +
>> +	/* Indicate which bit and byte is faulty (if any) */
>> +	b0 = read_ecc[0] ^ calc_ecc[0];
>> +	b1 = read_ecc[1] ^ calc_ecc[1];
>> +	b2 = read_ecc[2] ^ calc_ecc[2];
>> +	b = b0 | (b1 << 8) | (b2 << 16);
>> +
>> +	/* No errors */
>> +	if (likely(!b))
>> +		return 0;
>> +
>> +	/* Calculate bit position */
>> +	for (i = 0; i < 3; i++) {
>> +		switch (b % 4) {
>> +		case 2:
>> +			bit_position += shifting;
>> +		case 1:
>> +			break;
>> +		default:
>> +			return -EBADMSG;
>> +		}
>> +		shifting <<= 1;
>> +		b >>= 2;
>> +	}
>> +
>> +	/* Calculate byte position */
>> +	shifting = 1;
>> +	for (i = 0; i < 9; i++) {
>> +		switch (b % 4) {
>> +		case 2:
>> +			byte_addr += shifting;
>> +		case 1:
>> +			break;
>> +		default:
>> +			return -EBADMSG;
>> +		}
>> +		shifting <<= 1;
>> +		b >>= 2;
>> +	}
>> +
>> +	/* Flip the bit */
>> +	dat[byte_addr] ^= (1 << bit_position);
>> +
>> +	return 1;
>> +}
>> +
>> +/* BCH algorithm correction */
>> +static int stm32_fmc2_bch_decode(int eccsize, u8 *dat, u32 *ecc_sta)
>> +{
>> +	u32 bchdsr0 = ecc_sta[0];
>> +	u32 bchdsr1 = ecc_sta[1];
>> +	u32 bchdsr2 = ecc_sta[2];
>> +	u32 bchdsr3 = ecc_sta[3];
>> +	u32 bchdsr4 = ecc_sta[4];
>> +	u16 pos[8];
>> +	int i, den;
>> +	unsigned int nb_errs = 0;
>> +
>> +	/* No errors found */
>> +	if (likely(!(bchdsr0 & FMC2_BCHDSR0_DEF)))
>> +		return 0;
>> +
>> +	/* Too many errors detected */
>> +	if (unlikely(bchdsr0 & FMC2_BCHDSR0_DUE))
>> +		return -EBADMSG;
>> +
>> +	pos[0] = bchdsr1 & FMC2_BCHDSR1_EBP1_MASK;
>> +	pos[1] = (bchdsr1 & FMC2_BCHDSR1_EBP2_MASK) >> FMC2_BCHDSR1_EBP2_SHIFT;
>> +	pos[2] = bchdsr2 & FMC2_BCHDSR2_EBP3_MASK;
>> +	pos[3] = (bchdsr2 & FMC2_BCHDSR2_EBP4_MASK) >> FMC2_BCHDSR2_EBP4_SHIFT;
>> +	pos[4] = bchdsr3 & FMC2_BCHDSR3_EBP5_MASK;
>> +	pos[5] = (bchdsr3 & FMC2_BCHDSR3_EBP6_MASK) >> FMC2_BCHDSR3_EBP6_SHIFT;
>> +	pos[6] = bchdsr4 & FMC2_BCHDSR4_EBP7_MASK;
>> +	pos[7] = (bchdsr4 & FMC2_BCHDSR4_EBP8_MASK) >> FMC2_BCHDSR4_EBP8_SHIFT;
>> +
>> +	den = (bchdsr0 & FMC2_BCHDSR0_DEN_MASK) >> FMC2_BCHDSR0_DEN_SHIFT;
>> +	for (i = 0; i < den; i++) {
>> +		if (pos[i] < eccsize * 8) {
>> +			change_bit(pos[i], (unsigned long *)dat);
>> +			nb_errs++;
>> +		}
>> +	}
>> +
>> +	return nb_errs;
>> +}
>> +
>> +/* Sequencer read/write configuration */
>> +static void stm32_fmc2_rw_page_init(struct stm32_fmc2 *fmc2, int page,
>> +				    int raw, bool write_data)
>> +{
>> +	struct nand_chip *chip = &fmc2->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) {
>> +		csqcfgr1 |= FMC2_CSQCFGR1_ACYNBR(5);
>> +		csqar2 |= FMC2_CSQCAR2_ADDC5(page >> 16);
>> +	} else {
>> +		csqcfgr1 |= FMC2_CSQCFGR1_ACYNBR(4);
>> +	}
>> +
>> +	writel_relaxed(csqcfgr1, fmc2->io_base + FMC2_CSQCFGR1);
>> +	writel_relaxed(csqcfgr2, fmc2->io_base + FMC2_CSQCFGR2);
>> +	writel_relaxed(csqcfgr3, fmc2->io_base + FMC2_CSQCFGR3);
>> +	writel_relaxed(csqar1, fmc2->io_base + FMC2_CSQAR1);
>> +	writel_relaxed(csqar2, fmc2->io_base + FMC2_CSQAR2);
>> +}
>> +
>> +static void stm32_fmc2_dma_callback(void *arg)
>> +{
>> +	complete((struct completion *)arg);
>> +}
>> +
>> +/* Read/write data from/to a page */
>> +static int stm32_fmc2_xfer(struct stm32_fmc2 *fmc2, const u8 *buf,
>> +			   int raw, bool write_data)
>> +{
>> +	struct nand_chip *chip = &fmc2->chip;
>> +	struct dma_async_tx_descriptor *desc_data, *desc_ecc;
>> +	struct scatterlist *sg;
>> +	struct dma_chan *dma_ch = fmc2->dma_rx_ch;
>> +	enum dma_data_direction dma_data_dir = DMA_FROM_DEVICE;
>> +	enum dma_transfer_direction dma_transfer_dir = DMA_DEV_TO_MEM;
>> +	u32 csqcr = readl_relaxed(fmc2->io_base + FMC2_CSQCR);
>> +	int eccsteps = chip->ecc.steps;
>> +	int eccsize = chip->ecc.size;
>> +	const u8 *p = buf;
>> +	int s, ret;
>> +
>> +	/* Configure DMA data */
>> +	if (write_data) {
>> +		dma_data_dir = DMA_TO_DEVICE;
>> +		dma_transfer_dir = DMA_MEM_TO_DEV;
>> +		dma_ch = fmc2->dma_tx_ch;
>> +	}
>> +
>> +	for_each_sg(fmc2->dma_data_sg.sgl, sg, eccsteps, s) {
>> +		sg_set_buf(sg, p, eccsize);
>> +		p += eccsize;
>> +	}
>> +
>> +	ret = dma_map_sg(fmc2->dev, fmc2->dma_data_sg.sgl,
>> +			 eccsteps, dma_data_dir);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	desc_data = dmaengine_prep_slave_sg(dma_ch, fmc2->dma_data_sg.sgl,
>> +					    eccsteps, dma_transfer_dir,
>> +					    DMA_PREP_INTERRUPT);
>> +	if (!desc_data) {
>> +		ret = -ENOMEM;
>> +		goto err_unmap_data;
>> +	}
>> +
>> +	reinit_completion(&fmc2->dma_data_complete);
>> +	reinit_completion(&fmc2->complete);
>> +	desc_data->callback = stm32_fmc2_dma_callback;
>> +	desc_data->callback_param = &fmc2->dma_data_complete;
>> +	ret = dma_submit_error(dmaengine_submit(desc_data));
>> +	if (ret)
>> +		goto err_unmap_data;
>> +
>> +	dma_async_issue_pending(dma_ch);
>> +
>> +	if (!write_data && !raw) {
>> +		/* Configure DMA ecc status */
>> +		p = fmc2->ecc_buf;
>> +		for_each_sg(fmc2->dma_ecc_sg.sgl, sg, eccsteps, s) {
>> +			sg_set_buf(sg, p, fmc2->dma_ecc_len);
>> +			p += fmc2->dma_ecc_len;
>> +		}
>> +
>> +		ret = dma_map_sg(fmc2->dev, fmc2->dma_ecc_sg.sgl,
>> +				 eccsteps, dma_data_dir);
>> +		if (ret < 0)
>> +			goto err_unmap_data;
>> +
>> +		desc_ecc = dmaengine_prep_slave_sg(fmc2->dma_ecc_ch,
>> +						   fmc2->dma_ecc_sg.sgl,
>> +						   eccsteps, dma_transfer_dir,
>> +						   DMA_PREP_INTERRUPT);
>> +		if (!desc_ecc) {
>> +			ret = -ENOMEM;
>> +			goto err_unmap_ecc;
>> +		}
>> +
>> +		reinit_completion(&fmc2->dma_ecc_complete);
>> +		desc_ecc->callback = stm32_fmc2_dma_callback;
>> +		desc_ecc->callback_param = &fmc2->dma_ecc_complete;
>> +		ret = dma_submit_error(dmaengine_submit(desc_ecc));
>> +		if (ret)
>> +			goto err_unmap_ecc;
>> +
>> +		dma_async_issue_pending(fmc2->dma_ecc_ch);
>> +	}
>> +
>> +	stm32_fmc2_clear_seq_irq(fmc2);
>> +	stm32_fmc2_enable_seq_irq(fmc2);
>> +
>> +	/* Start the transfer */
>> +	csqcr |= FMC2_CSQCR_CSQSTART;
>> +	writel_relaxed(csqcr, fmc2->io_base + FMC2_CSQCR);
>> +
>> +	/* Wait end of sequencer transfer */
>> +	if (!wait_for_completion_timeout(&fmc2->complete,
>> +					 msecs_to_jiffies(1000))) {
>> +		dev_err(fmc2->dev, "seq timeout\n");
>> +		stm32_fmc2_disable_seq_irq(fmc2);
>> +		dmaengine_terminate_all(dma_ch);
>> +		if (!write_data && !raw)
>> +			dmaengine_terminate_all(fmc2->dma_ecc_ch);
>> +		ret = -ETIMEDOUT;
>> +		goto err_unmap_ecc;
>> +	}
>> +
>> +	/* Wait DMA data transfer completion */
>> +	if (!wait_for_completion_timeout(&fmc2->dma_data_complete,
>> +					 msecs_to_jiffies(100))) {
>> +		dev_err(fmc2->dev, "data DMA timeout\n");
>> +		dmaengine_terminate_all(dma_ch);
>> +		ret = -ETIMEDOUT;
>> +	}
>> +
>> +	/* Wait DMA ecc transfer completion */
>> +	if (!write_data && !raw) {
>> +		if (!wait_for_completion_timeout(&fmc2->dma_ecc_complete,
>> +						 msecs_to_jiffies(100))) {
>> +			dev_err(fmc2->dev, "ecc DMA timeout\n");
>> +			dmaengine_terminate_all(fmc2->dma_ecc_ch);
>> +			ret = -ETIMEDOUT;
>> +		}
>> +	}
>> +
>> +err_unmap_ecc:
>> +	if (!write_data && !raw)
>> +		dma_unmap_sg(fmc2->dev, fmc2->dma_ecc_sg.sgl,
>> +			     eccsteps, dma_data_dir);
>> +
>> +err_unmap_data:
>> +	dma_unmap_sg(fmc2->dev, fmc2->dma_data_sg.sgl, eccsteps, dma_data_dir);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_fmc2_sequencer_write(struct nand_chip *chip,
>> +				      const u8 *buf, int oob_required,
>> +				      int page, int raw)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
>> +	int ret;
>> +
>> +	/* Configure the sequencer */
>> +	stm32_fmc2_rw_page_init(fmc2, page, raw, true);
>> +
>> +	/* Write the page */
>> +	ret = stm32_fmc2_xfer(fmc2, buf, raw, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Write oob */
>> +	if (oob_required) {
>> +		ret = nand_change_write_column_op(chip, mtd->writesize,
>> +						  chip->oob_poi, mtd->oobsize,
>> +						  false);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return nand_prog_page_end_op(chip);
>> +}
>> +
>> +static int stm32_fmc2_sequencer_write_page(struct nand_chip *chip,
>> +					   const uint8_t *buf,
>> +					   int oob_required,
>> +					   int page)
>> +{
>> +	return stm32_fmc2_sequencer_write(chip, buf, oob_required, page, false);
>> +}
>> +
>> +static int stm32_fmc2_sequencer_write_page_raw(struct nand_chip *chip,
>> +					       const uint8_t *buf,
>> +					       int oob_required,
>> +					       int page)
>> +{
>> +	return stm32_fmc2_sequencer_write(chip, buf, oob_required, page, true);
>> +}
>> +
>> +/*
>> + * Get a status indicating which sectors have errors
>> + * Only available when the sequencer is used (BCH only)
>> + */
>> +static inline u16 stm32_fmc2_get_mapping_status(struct stm32_fmc2 *fmc2)
>> +{
>> +	u32 csqemsr = readl_relaxed(fmc2->io_base + FMC2_CSQEMSR);
>> +
>> +	return csqemsr & FMC2_CSQEMSR_SEM;
>> +}
>> +
>> +static int stm32_fmc2_sequencer_read_page(struct nand_chip *chip,
>> +					  uint8_t *buf,
>> +					  int oob_required,
>> +					  int page)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
>> +	int i, s, ret, eccsize = chip->ecc.size;
>> +	int eccbytes = chip->ecc.bytes;
>> +	int eccsteps = chip->ecc.steps;
>> +	int eccstrength = chip->ecc.strength;
>> +	u8 *p = buf;
>> +	u8 *ecc_calc = chip->ecc.calc_buf;
>> +	u8 *ecc_code = chip->ecc.code_buf;
>> +	u32 *ecc_sta = (u32 *)fmc2->ecc_buf;
>> +	u16 sta_map;
>> +	unsigned int max_bitflips = 0;
>> +
>> +	/* Configure the sequencer */
>> +	stm32_fmc2_rw_page_init(fmc2, page, 0, false);
>> +
>> +	/* Read the page */
>> +	ret = stm32_fmc2_xfer(fmc2, buf, 0, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sta_map = stm32_fmc2_get_mapping_status(fmc2);
>> +
>> +	/* Check if errors happen */
>> +	if (likely(!sta_map)) {
>> +		if (oob_required)
>> +			return nand_change_read_column_op(chip, mtd->writesize,
>> +							  chip->oob_poi,
>> +							  mtd->oobsize, false);
>> +
>> +		return 0;
>> +	}
>> +
>> +	/* Read oob */
>> +	ret = nand_change_read_column_op(chip, mtd->writesize,
>> +					 chip->oob_poi, mtd->oobsize, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
>> +					 chip->ecc.total);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Correct data */
>> +	for (i = 0, s = 0; s < eccsteps; s++, i += eccbytes, p += eccsize) {
>> +		int stat = 0;
>> +
>> +		if (eccstrength == FMC2_ECC_HAM) {
>> +			/* Ecc_sta = FMC2_HECCR */
>> +			if (sta_map & BIT(s)) {
>> +				stm32_fmc2_ham_set_ecc(*ecc_sta, &ecc_calc[i]);
>> +				stat = chip->ecc.correct(chip, p, &ecc_code[i],
>> +							 &ecc_calc[i]);
>> +			}
>> +			ecc_sta++;
>> +		} else {
>> +			/*
>> +			 * Ecc_sta[0] = FMC2_BCHDSR0
>> +			 * Ecc_sta[1] = FMC2_BCHDSR1
>> +			 * Ecc_sta[2] = FMC2_BCHDSR2
>> +			 * Ecc_sta[3] = FMC2_BCHDSR3
>> +			 * Ecc_sta[4] = FMC2_BCHDSR4
>> +			 */
>> +			if (sta_map & BIT(s))
>> +				stat = stm32_fmc2_bch_decode(chip->ecc.size,
>> +							     p, ecc_sta);
>> +			ecc_sta += 5;
>> +		}
>> +
>> +		if (stat == -EBADMSG)
>> +			/* Check for empty pages with bitflips */
>> +			stat = nand_check_erased_ecc_chunk(p, eccsize,
>> +							   &ecc_code[i],
>> +							   eccbytes,
>> +							   NULL, 0,
>> +							   eccstrength);
>> +
>> +		if (stat < 0) {
>> +			mtd->ecc_stats.failed++;
>> +		} else {
>> +			mtd->ecc_stats.corrected += stat;
>> +			max_bitflips = max_t(unsigned int, max_bitflips, stat);
>> +		}
>> +	}
>> +
>> +	return max_bitflips;
>> +}
>> +
>> +static int stm32_fmc2_sequencer_read_page_raw(struct nand_chip *chip,
>> +					      uint8_t *buf,
>> +					      int oob_required,
>> +					      int page)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
>> +	int ret;
>> +
>> +	/* Configure the sequencer */
>> +	stm32_fmc2_rw_page_init(fmc2, page, 1, false);
>> +
>> +	/* Read the page */
>> +	ret = stm32_fmc2_xfer(fmc2, buf, 1, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Read oob */
>> +	if (oob_required)
>> +		return nand_change_read_column_op(chip, mtd->writesize,
>> +						  chip->oob_poi, mtd->oobsize,
>> +						  false);
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t stm32_fmc2_irq(int irq, void *dev_id)
>> +{
>> +	struct stm32_fmc2 *fmc2 = (struct stm32_fmc2 *)dev_id;
>> +
>> +	stm32_fmc2_disable_seq_irq(fmc2);
>> +
>> +	complete(&fmc2->complete);
>> +
>> +	return IRQ_HANDLED;
> 
> You don't have any bit to check/ack/read?

No, nothing to check.

> 
>> +}
>> +
>> +void stm32_fmc2_read_data(struct nand_chip *chip, void *buf,
>> +			  unsigned int len, bool force_8bit)
>> +{
>> +	struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
>> +	void __iomem *io_addr_r = fmc2->data_base[fmc2->cs_sel];
>> +	u8 *p = buf;
>> +	unsigned int i;
>> +
>> +	if (force_8bit)
>> +		goto read_8bit;
>> +
>> +	if (IS_ALIGNED((u32)buf, sizeof(u32)) && IS_ALIGNED(len, sizeof(u32))) {
> 
> If you selected BOUNCE_BUFFER in the options, buf is supposedly
> aligned, or am I missing something?

2 FMC2 internal modes can be used:
  - Sequencer mode (Patch 2/3): dmas are used and NAND_USE_BOUNCE_BUFFER 
option is selected.
  - Manual mode (Patch 3/3): no dma channel is used and 
NAND_USE_BOUNCE_BUFFER is not selected.
Should i select NAND_USE_BOUNCE_BUFFER for sequencer and manual mode, 
and remove IS_ALIGNED test on buf?

> 
>> +		u32 *p = buf;
>> +
>> +		len >>= 2;
> 
> Please don't do such optimization by hand. Compilers are doing it for
> it, so let's keep the code readable. Just do a regular "/=
> sizeof(unsigned long)".

Ok, i will replace this line with: len /= sizeof(u32)

> 
>> +		for (i = 0; i < len; i++)
>> +			p[i] = readl_relaxed(io_addr_r);
>> +		return;
>> +	}
>> +
>> +	if (chip->options & NAND_BUSWIDTH_16) {
>> +		u16 *p = buf;
>> +
>> +		len >>= 1;
> 
> Ditto

Ok, i will replace this line with: len /= sizeof(u16)
Same modifications will be done in stm32_fmc2_write_data function.

> 
>> +		for (i = 0; i < len; i++)
>> +			p[i] = readw_relaxed(io_addr_r);
>> +		return;
>> +	}
>> +
>> +read_8bit:
>> +	for (i = 0; i < len; i++)
>> +		p[i] = readb_relaxed(io_addr_r);
>> +}
>> +
>> +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((u32)buf, sizeof(u32)) && IS_ALIGNED(len, sizeof(u32))) {
>> +		const u32 *p = buf;
>> +
>> +		len >>= 2;
>> +		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 >>= 1;
>> +		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);
>> +}
>> +
>> +static int stm32_fmc2_exec_op(struct nand_chip *chip,
>> +			      const struct nand_operation *op,
>> +			      bool check_only)
> 
> You should probably return 0 if check_only == true, otherwise you could
> do twice each operation.

Ok.

> 
>> +{
>> +	struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
>> +	const struct nand_op_instr *instr = NULL;
>> +	unsigned int op_id, i;
>> +	int ret = 0;
>> +
>> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
>> +		instr = &op->instrs[op_id];
>> +
>> +		switch (instr->type) {
>> +		case NAND_OP_CMD_INSTR:
>> +			writeb_relaxed(instr->ctx.cmd.opcode,
>> +				       fmc2->cmd_base[fmc2->cs_sel]);
>> +			break;
>> +
>> +		case NAND_OP_ADDR_INSTR:
>> +			for (i = 0; i < instr->ctx.addr.naddrs; i++)
>> +				writeb_relaxed(instr->ctx.addr.addrs[i],
>> +					       fmc2->addr_base[fmc2->cs_sel]);
>> +			break;
>> +
>> +		case NAND_OP_DATA_IN_INSTR:
>> +			stm32_fmc2_read_data(chip, instr->ctx.data.buf.in,
>> +					     instr->ctx.data.len,
>> +					     instr->ctx.data.force_8bit);
>> +			break;
>> +
>> +		case NAND_OP_DATA_OUT_INSTR:
>> +			stm32_fmc2_write_data(chip, instr->ctx.data.buf.out,
>> +					      instr->ctx.data.len,
>> +					      instr->ctx.data.force_8bit);
>> +			break;
>> +
>> +		case NAND_OP_WAITRDY_INSTR:
>> +			ret = nand_soft_waitrdy(chip,
>> +						instr->ctx.waitrdy.timeout_ms);
>> +			break;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/* Timings configuration */
>> +static void stm32_fmc2_timings_init(struct stm32_fmc2 *fmc2)
>> +{
>> +	struct stm32_fmc2_timings *timings = &fmc2->timings;
>> +	u32 pcr = readl_relaxed(fmc2->io_base + FMC2_PCR);
>> +	u32 pmem, patt;
>> +
>> +	/* Set tclr/tar timings */
>> +	pcr &= ~FMC2_PCR_TCLR_MASK;
>> +	pcr |= FMC2_PCR_TCLR(timings->tclr);
>> +	pcr &= ~FMC2_PCR_TAR_MASK;
>> +	pcr |= FMC2_PCR_TAR(timings->tar);
>> +
>> +	/* Set tset/twait/thold/thiz timings in common bank */
>> +	pmem = FMC2_PMEM_MEMSET(timings->tset_mem);
>> +	pmem |= FMC2_PMEM_MEMWAIT(timings->twait);
>> +	pmem |= FMC2_PMEM_MEMHOLD(timings->thold_mem);
>> +	pmem |= FMC2_PMEM_MEMHIZ(timings->thiz);
>> +
>> +	/* Set tset/twait/thold/thiz timings in attribut bank */
>> +	patt = FMC2_PATT_ATTSET(timings->tset_att);
>> +	patt |= FMC2_PATT_ATTWAIT(timings->twait);
>> +	patt |= FMC2_PATT_ATTHOLD(timings->thold_att);
>> +	patt |= FMC2_PATT_ATTHIZ(timings->thiz);
>> +
>> +	writel_relaxed(pcr, fmc2->io_base + FMC2_PCR);
>> +	writel_relaxed(pmem, fmc2->io_base + FMC2_PMEM);
>> +	writel_relaxed(patt, fmc2->io_base + FMC2_PATT);
>> +}
>> +
>> +/* Controller initialization */
>> +static void stm32_fmc2_init(struct stm32_fmc2 *fmc2)
>> +{
>> +	u32 pcr = readl_relaxed(fmc2->io_base + FMC2_PCR);
>> +	u32 bcr1 = readl_relaxed(fmc2->io_base + FMC2_BCR1);
>> +
>> +	/* Enable wait feature and nand flash memory bank */
>> +	pcr |= FMC2_PCR_PWAITEN;
>> +	pcr |= FMC2_PCR_PBKEN;
>> +
>> +	/* Set buswidth to 8 bits mode for identification */
>> +	pcr &= ~FMC2_PCR_PWID_MASK;
>> +
>> +	/* Ecc logic is disabled */
>> +	pcr &= ~FMC2_PCR_ECCEN;
>> +
>> +	/* Default mode */
>> +	pcr &= ~FMC2_PCR_ECCALG;
>> +	pcr &= ~FMC2_PCR_BCHECC;
>> +	pcr &= ~FMC2_PCR_WEN;
>> +
>> +	/* Set default ecc sector size */
>> +	pcr &= ~FMC2_PCR_ECCSS_MASK;
>> +	pcr |= FMC2_PCR_ECCSS(FMC2_PCR_ECCSS_2048);
>> +
>> +	/* Set default tclr/tar timings */
>> +	pcr &= ~FMC2_PCR_TCLR_MASK;
>> +	pcr |= FMC2_PCR_TCLR(FMC2_PCR_TCLR_DEFAULT);
>> +	pcr &= ~FMC2_PCR_TAR_MASK;
>> +	pcr |= FMC2_PCR_TAR(FMC2_PCR_TAR_DEFAULT);
>> +
>> +	/* Enable FMC2 controller */
>> +	bcr1 |= FMC2_BCR1_FMC2EN;
>> +
>> +	writel_relaxed(bcr1, fmc2->io_base + FMC2_BCR1);
>> +	writel_relaxed(pcr, fmc2->io_base + FMC2_PCR);
>> +	writel_relaxed(FMC2_PMEM_DEFAULT, fmc2->io_base + FMC2_PMEM);
>> +	writel_relaxed(FMC2_PATT_DEFAULT, fmc2->io_base + FMC2_PATT);
>> +}
>> +
>> +/* Controller configuration */
>> +static void stm32_fmc2_setup(struct stm32_fmc2 *fmc2)
>> +{
>> +	struct nand_chip *chip = &fmc2->chip;
>> +	u32 pcr = readl_relaxed(fmc2->io_base + FMC2_PCR);
>> +
>> +	/* Configure in HAMMING by default */
> 
> In other comments you use "Hamming", not "HAMMING".
> 
> And this comment is weird, you don't configure for Hamming below. Maybe
> you meant that the controller is by default configured to work with
> Hamming error detection?

Yes, it's true. Hamming error detection is set by default. Below 
implementation configures the controller in BCH4 or BCH8. I will modify 
the comment.

> 
>> +	if (chip->ecc.strength == FMC2_ECC_BCH8) {
>> +		pcr |= FMC2_PCR_ECCALG;
>> +		pcr |= FMC2_PCR_BCHECC;
>> +	} else if (chip->ecc.strength == FMC2_ECC_BCH4) {
>> +		pcr |= FMC2_PCR_ECCALG;
>> +	}
>> +
>> +	/* Set buswidth */
>> +	if (chip->options & NAND_BUSWIDTH_16)
>> +		pcr |= FMC2_PCR_PWID(FMC2_PCR_PWID_BUSWIDTH_16);
>> +
>> +	/* Set ecc sector size */
>> +	pcr &= ~FMC2_PCR_ECCSS_MASK;
>> +	pcr |= FMC2_PCR_ECCSS(FMC2_PCR_ECCSS_512);
>> +
>> +	writel_relaxed(pcr, fmc2->io_base + FMC2_PCR);
>> +}
>> +
>> +/* Controller timings */
>> +static void stm32_fmc2_calc_timings(struct stm32_fmc2 *fmc2,
>> +				    const struct nand_sdr_timings *sdrt,
>> +				    struct stm32_fmc2_timings *tims)
>> +{
>> +	unsigned long hclk = clk_get_rate(fmc2->clk);
>> +	unsigned long hclkp = NSEC_PER_SEC / (hclk / 1000);
>> +	int tar, tclr, thiz, twait, tset_mem, tset_att, thold_mem, thold_att;
>> +
>> +	tar = hclkp;
>> +	if (tar < sdrt->tAR_min)
>> +		tar = sdrt->tAR_min;
>> +	tims->tar = DIV_ROUND_UP(tar, hclkp) - 1;
>> +	if (tims->tar > FMC2_PCR_TIMING_MASK)
>> +		tims->tar = FMC2_PCR_TIMING_MASK;
>> +
>> +	tclr = hclkp;
>> +	if (tclr < sdrt->tCLR_min)
>> +		tclr = sdrt->tCLR_min;
>> +	tims->tclr = DIV_ROUND_UP(tclr, hclkp) - 1;
>> +	if (tims->tclr > FMC2_PCR_TIMING_MASK)
>> +		tims->tclr = FMC2_PCR_TIMING_MASK;
>> +
>> +	tims->thiz = FMC2_THIZ;
>> +	thiz = (tims->thiz + 1) * hclkp;
>> +
>> +	/*
>> +	 * tWAIT > tRP
>> +	 * tWAIT > tWP
>> +	 * tWAIT > tREA + tIO
>> +	 */
>> +	twait = hclkp;
>> +	if (twait < sdrt->tRP_min)
>> +		twait = sdrt->tRP_min;
>> +	if (twait < sdrt->tWP_min)
>> +		twait = sdrt->tWP_min;
>> +	if (twait < sdrt->tREA_max + FMC2_TIO)
>> +		twait = sdrt->tREA_max + FMC2_TIO;
>> +	tims->twait = DIV_ROUND_UP(twait, hclkp);
>> +	if (tims->twait == 0)
>> +		tims->twait = 1;
>> +	else if (tims->twait > FMC2_PMEM_PATT_TIMING_MASK)
>> +		tims->twait = FMC2_PMEM_PATT_TIMING_MASK;
>> +
>> +	/*
>> +	 * tSETUP_MEM > tCS - tWAIT
>> +	 * tSETUP_MEM > tALS - tWAIT
>> +	 * tSETUP_MEM > tDS - (tWAIT - tHIZ)
>> +	 */
>> +	tset_mem = hclkp;
>> +	if (sdrt->tCS_min > twait && (tset_mem < sdrt->tCS_min - twait))
>> +		tset_mem = sdrt->tCS_min - twait;
>> +	if (sdrt->tALS_min > twait && (tset_mem < sdrt->tALS_min - twait))
>> +		tset_mem = sdrt->tALS_min - twait;
>> +	if (twait > thiz && (sdrt->tDS_min > twait - thiz) &&
>> +	    (tset_mem < sdrt->tDS_min - (twait - thiz)))
>> +		tset_mem = sdrt->tDS_min - (twait - thiz);
>> +	tims->tset_mem = DIV_ROUND_UP(tset_mem, hclkp);
>> +	if (tims->tset_mem == 0)
>> +		tims->tset_mem = 1;
>> +	else if (tims->tset_mem > FMC2_PMEM_PATT_TIMING_MASK)
>> +		tims->tset_mem = FMC2_PMEM_PATT_TIMING_MASK;
>> +
>> +	/*
>> +	 * tHOLD_MEM > tCH
>> +	 * tHOLD_MEM > tREH - tSETUP_MEM
>> +	 * tHOLD_MEM > max(tRC, tWC) - (tSETUP_MEM + tWAIT)
>> +	 */
>> +	thold_mem = hclkp;
>> +	if (thold_mem < sdrt->tCH_min)
>> +		thold_mem = sdrt->tCH_min;
>> +	if (sdrt->tREH_min > tset_mem &&
>> +	    (thold_mem < sdrt->tREH_min - tset_mem))
>> +		thold_mem = sdrt->tREH_min - tset_mem;
>> +	if ((sdrt->tRC_min > tset_mem + twait) &&
>> +	    (thold_mem < sdrt->tRC_min - (tset_mem + twait)))
>> +		thold_mem = sdrt->tRC_min - (tset_mem + twait);
>> +	if ((sdrt->tWC_min > tset_mem + twait) &&
>> +	    (thold_mem < sdrt->tWC_min - (tset_mem + twait)))
>> +		thold_mem = sdrt->tWC_min - (tset_mem + twait);
>> +	tims->thold_mem = DIV_ROUND_UP(thold_mem, hclkp);
>> +	if (tims->thold_mem == 0)
>> +		tims->thold_mem = 1;
>> +	else if (tims->thold_mem > FMC2_PMEM_PATT_TIMING_MASK)
>> +		tims->thold_mem = FMC2_PMEM_PATT_TIMING_MASK;
>> +
>> +	/*
>> +	 * tSETUP_ATT > tCS - tWAIT
>> +	 * tSETUP_ATT > tCLS - tWAIT
>> +	 * tSETUP_ATT > tALS - tWAIT
>> +	 * tSETUP_ATT > tRHW - tHOLD_MEM
>> +	 * tSETUP_ATT > tDS - (tWAIT - tHIZ)
>> +	 */
>> +	tset_att = hclkp;
>> +	if (sdrt->tCS_min > twait && (tset_att < sdrt->tCS_min - twait))
>> +		tset_att = sdrt->tCS_min - twait;
>> +	if (sdrt->tCLS_min > twait && (tset_att < sdrt->tCLS_min - twait))
>> +		tset_att = sdrt->tCLS_min - twait;
>> +	if (sdrt->tALS_min > twait && (tset_att < sdrt->tALS_min - twait))
>> +		tset_att = sdrt->tALS_min - twait;
>> +	if (sdrt->tRHW_min > thold_mem &&
>> +	    (tset_att < sdrt->tRHW_min - thold_mem))
>> +		tset_att = sdrt->tRHW_min - thold_mem;
>> +	if (twait > thiz && (sdrt->tDS_min > twait - thiz) &&
>> +	    (tset_att < sdrt->tDS_min - (twait - thiz)))
>> +		tset_att = sdrt->tDS_min - (twait - thiz);
>> +	tims->tset_att = DIV_ROUND_UP(tset_att, hclkp);
>> +	if (tims->tset_att == 0)
>> +		tims->tset_att = 1;
>> +	else if (tims->tset_att > FMC2_PMEM_PATT_TIMING_MASK)
>> +		tims->tset_att = FMC2_PMEM_PATT_TIMING_MASK;
>> +
>> +	/*
>> +	 * tHOLD_ATT > tALH
>> +	 * tHOLD_ATT > tCH
>> +	 * tHOLD_ATT > tCLH
>> +	 * tHOLD_ATT > tCOH
>> +	 * tHOLD_ATT > tDH
>> +	 * tHOLD_ATT > tWB + tIO + tSYNC - tSETUP_MEM
>> +	 * tHOLD_ATT > tADL - tSETUP_MEM
>> +	 * tHOLD_ATT > tWH - tSETUP_MEM
>> +	 * tHOLD_ATT > tWHR - tSETUP_MEM
>> +	 * tHOLD_ATT > tRC - (tSETUP_ATT + tWAIT)
>> +	 * tHOLD_ATT > tWC - (tSETUP_ATT + tWAIT)
>> +	 */
>> +	thold_att = hclkp;
>> +	if (thold_att < sdrt->tALH_min)
>> +		thold_att = sdrt->tALH_min;
>> +	if (thold_att < sdrt->tCH_min)
>> +		thold_att = sdrt->tCH_min;
>> +	if (thold_att < sdrt->tCLH_min)
>> +		thold_att = sdrt->tCLH_min;
>> +	if (thold_att < sdrt->tCOH_min)
>> +		thold_att = sdrt->tCOH_min;
>> +	if (thold_att < sdrt->tDH_min)
>> +		thold_att = sdrt->tDH_min;
>> +	if ((sdrt->tWB_max + FMC2_TIO + FMC2_TSYNC > tset_mem) &&
>> +	    (thold_att < sdrt->tWB_max + FMC2_TIO + FMC2_TSYNC - tset_mem))
>> +		thold_att = sdrt->tWB_max + FMC2_TIO + FMC2_TSYNC - tset_mem;
>> +	if (sdrt->tADL_min > tset_mem &&
>> +	    (thold_att < sdrt->tADL_min - tset_mem))
>> +		thold_att = sdrt->tADL_min - tset_mem;
>> +	if (sdrt->tWH_min > tset_mem &&
>> +	    (thold_att < sdrt->tWH_min - tset_mem))
>> +		thold_att = sdrt->tWH_min - tset_mem;
>> +	if (sdrt->tWHR_min > tset_mem &&
>> +	    (thold_att < sdrt->tWHR_min - tset_mem))
>> +		thold_att = sdrt->tWHR_min - tset_mem;
>> +	if ((sdrt->tRC_min > tset_att + twait) &&
>> +	    (thold_att < sdrt->tRC_min - (tset_att + twait)))
>> +		thold_att = sdrt->tRC_min - (tset_att + twait);
>> +	if ((sdrt->tWC_min > tset_att + twait) &&
>> +	    (thold_att < sdrt->tWC_min - (tset_att + twait)))
>> +		thold_att = sdrt->tWC_min - (tset_att + twait);
>> +	tims->thold_att = DIV_ROUND_UP(thold_att, hclkp);
>> +	if (tims->thold_att == 0)
>> +		tims->thold_att = 1;
>> +	else if (tims->thold_att > FMC2_PMEM_PATT_TIMING_MASK)
>> +		tims->thold_att = FMC2_PMEM_PATT_TIMING_MASK;
>> +}
>> +
>> +static int stm32_fmc2_setup_interface(struct nand_chip *chip, int chipnr,
>> +				      const struct nand_data_interface *conf)
>> +{
>> +	struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
>> +	struct stm32_fmc2_timings tims;
>> +	const struct nand_sdr_timings *sdrt;
>> +
>> +	sdrt = nand_get_sdr_timings(conf);
>> +	if (IS_ERR(sdrt))
>> +		return PTR_ERR(sdrt);
>> +
>> +	stm32_fmc2_calc_timings(fmc2, sdrt, &tims);
> 
> Why do you need 'tims' if the only use is to memcpy it in
> fmc2->timings? I suppose you can get rid of it.

Yes, it is not needed to use a local variable.

> 
>> +
>> +	if (chipnr == NAND_DATA_IFACE_CHECK_ONLY)
>> +		return 0;
> 
> If the controller may support all the timing modes, then you can
> certainly return on CHECK_ONLY before *_calc_timings().

Yes, i will move this check before stm32_fmc2_calc_timings().

> 
>> +
>> +	/* Save and apply timings */
>> +	memcpy(&fmc2->timings, &tims, sizeof(tims));
>> +	stm32_fmc2_timings_init(fmc2);
>> +
>> +	return 0;
>> +}
>> +
>> +/* DMA configuration */
>> +static int stm32_fmc2_dma_setup(struct stm32_fmc2 *fmc2, u8 nbsect)
>> +{
>> +	struct nand_chip *chip = &fmc2->chip;
>> +	struct dma_slave_config dma_cfg;
>> +	int ret;
>> +
>> +	memset(&dma_cfg, 0, sizeof(dma_cfg));
>> +	dma_cfg.src_addr = fmc2->data_phys_addr[fmc2->cs_sel];
>> +	dma_cfg.dst_addr = fmc2->data_phys_addr[fmc2->cs_sel];
>> +	dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +	dma_cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +	dma_cfg.src_maxburst = 32;
>> +	dma_cfg.dst_maxburst = 32;
>> +
>> +	fmc2->dma_tx_ch = dma_request_slave_channel(fmc2->dev, "tx");
>> +	if (fmc2->dma_tx_ch) {
>> +		ret = dmaengine_slave_config(fmc2->dma_tx_ch, &dma_cfg);
>> +		if (ret) {
>> +			dev_err(fmc2->dev, "data tx DMA engine slave config failed\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	fmc2->dma_rx_ch = dma_request_slave_channel(fmc2->dev, "rx");
>> +	if (fmc2->dma_rx_ch) {
>> +		ret = dmaengine_slave_config(fmc2->dma_rx_ch, &dma_cfg);
>> +		if (ret) {
>> +			dev_err(fmc2->dev, "data rx DMA engine slave config failed\n");
>> +			return ret;
>> +		}
>> +
>> +		fmc2->dma_ecc_ch = dma_request_slave_channel(fmc2->dev, "ecc");
>> +		if (fmc2->dma_ecc_ch) {
>> +			/*
>> +			 * HAMMING: we read HECCR register
>> +			 * BCH4/BCH8: we read BCHDSRSx registers
>> +			 */
>> +			memset(&dma_cfg, 0, sizeof(dma_cfg));
>> +			dma_cfg.src_addr = fmc2->io_phys_addr;
>> +			dma_cfg.src_addr += chip->ecc.strength == FMC2_ECC_HAM ?
>> +					    FMC2_HECCR : FMC2_BCHDSR0;
>> +			dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +
>> +			ret = dmaengine_slave_config(fmc2->dma_ecc_ch,
>> +						     &dma_cfg);
>> +			if (ret) {
>> +				dev_err(fmc2->dev, "ecc DMA engine slave config failed\n");
>> +				return ret;
>> +			}
>> +
>> +			ret = sg_alloc_table(&fmc2->dma_ecc_sg,
>> +					     nbsect, GFP_KERNEL);
>> +			if (ret)
>> +				return ret;
>> +
>> +			/* Calculate ecc length needed for one sector */
>> +			fmc2->dma_ecc_len = chip->ecc.strength == FMC2_ECC_HAM ?
>> +					    FMC2_HECCR_LEN : FMC2_BCHDSRS_LEN;
>> +
>> +			/* Allocate a buffer to store ecc status registers */
>> +			fmc2->ecc_buf = devm_kzalloc(fmc2->dev,
>> +						     fmc2->dma_ecc_len * nbsect,
>> +						     GFP_KERNEL);
>> +			if (!fmc2->ecc_buf)
>> +				return -ENOMEM;
>> +		} else {
>> +			dev_err(fmc2->dev, "ecc DMA not defined in the device tree\n");
>> +			return -ENOENT;
>> +		}
>> +	}
>> +
>> +	if (fmc2->dma_tx_ch && fmc2->dma_rx_ch) {
>> +		ret = sg_alloc_table(&fmc2->dma_data_sg, nbsect, GFP_KERNEL);
>> +		if (ret)
>> +			return ret;
>> +
>> +		init_completion(&fmc2->dma_data_complete);
>> +		init_completion(&fmc2->dma_ecc_complete);
>> +	} else {
>> +		dev_err(fmc2->dev, "rx/tx DMA not defined in the device tree\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/* NAND callbacks setup */
>> +static void stm32_fmc2_nand_callbacks_setup(struct stm32_fmc2 *fmc2)
>> +{
>> +	struct nand_chip *chip = &fmc2->chip;
>> +
>> +	/* Specific callbacks to read/write a page */
>> +	chip->ecc.correct = stm32_fmc2_ham_correct;
>> +	chip->ecc.write_page = stm32_fmc2_sequencer_write_page;
>> +	chip->ecc.read_page = stm32_fmc2_sequencer_read_page;
>> +	chip->ecc.write_page_raw = stm32_fmc2_sequencer_write_page_raw;
>> +	chip->ecc.read_page_raw = stm32_fmc2_sequencer_read_page_raw;
>> +	chip->options |= NAND_USE_BOUNCE_BUFFER;
>> +
>> +	/* Specific configurations depending on the algo used */
>> +	if (chip->ecc.strength == FMC2_ECC_HAM)
>> +		chip->ecc.bytes = chip->options & NAND_BUSWIDTH_16 ? 4 : 3;
>> +	else if (chip->ecc.strength == FMC2_ECC_BCH8)
>> +		chip->ecc.bytes = chip->options & NAND_BUSWIDTH_16 ? 14 : 13;
>> +	else
>> +		chip->ecc.bytes = chip->options & NAND_BUSWIDTH_16 ? 8 : 7;
>> +}
>> +
>> +/* FMC2 layout */
>> +static int stm32_fmc2_nand_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +					 struct mtd_oob_region *oobregion)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oobregion->length = ecc->total;
>> +	oobregion->offset = FMC2_BBM_LEN;
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_fmc2_nand_ooblayout_free(struct mtd_info *mtd, int section,
>> +					  struct mtd_oob_region *oobregion)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oobregion->length = mtd->oobsize - ecc->total - FMC2_BBM_LEN;
>> +	oobregion->offset = ecc->total + FMC2_BBM_LEN;
>> +
>> +	return 0;
>> +}
>> +
>> +const struct mtd_ooblayout_ops stm32_fmc2_nand_ooblayout_ops = {
>> +	.ecc = stm32_fmc2_nand_ooblayout_ecc,
>> +	.free = stm32_fmc2_nand_ooblayout_free,
>> +};
>> +
>> +/* FMC2 caps */
>> +static int stm32_fmc2_calc_ecc_bytes(int step_size, int strength)
>> +{
>> +	/* Hamming */
>> +	if (strength == FMC2_ECC_HAM)
>> +		return 4;
>> +
>> +	/* BCH8 */
>> +	if (strength == FMC2_ECC_BCH8)
>> +		return 14;
>> +
>> +	/* BCH4 */
>> +	return 8;
>> +}
>> +
>> +NAND_ECC_CAPS_SINGLE(stm32_fmc2_ecc_caps, stm32_fmc2_calc_ecc_bytes,
>> +		     FMC2_ECC_STEP_SIZE,
>> +		     FMC2_ECC_HAM, FMC2_ECC_BCH4, FMC2_ECC_BCH8);
>> +
>> +/* FMC2 controller ops */
>> +static int stm32_fmc2_attach_chip(struct nand_chip *chip)
>> +{
>> +	struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	u8 nbsect;
>> +	int ret;
>> +
>> +	/*
>> +	 * Only NAND_ECC_HW mode is actually supported
>> +	 * HAMMING => ecc.strength = 1
>> +	 * BCH4 => ecc.strength = 4
>> +	 * BCH8 => ecc.strength = 8
>> +	 * ecc sector size = 512
>> +	 */
>> +	if (chip->ecc.mode != NAND_ECC_HW) {
>> +		dev_err(fmc2->dev, "nand_ecc_mode is not well defined in the DT\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = nand_ecc_choose_conf(chip, &stm32_fmc2_ecc_caps,
>> +				   mtd->oobsize - FMC2_BBM_LEN);
>> +	if (ret) {
>> +		dev_err(fmc2->dev, "no valid ECC settings set\n");
>> +		return ret;
>> +	}
>> +
>> +	nbsect = mtd->writesize / chip->ecc.size;
>> +	if (nbsect > FMC2_MAX_SG_COUNT) {
>> +		dev_err(fmc2->dev, "nand page size is not supported\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (chip->bbt_options & NAND_BBT_USE_FLASH)
>> +		chip->bbt_options |= NAND_BBT_NO_OOB;
>> +
>> +	/* FMC2 setup routine */
>> +	stm32_fmc2_setup(fmc2);
>> +
>> +	/* DMA setup */
>> +	ret = stm32_fmc2_dma_setup(fmc2, nbsect);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* NAND callbacks setup */
>> +	stm32_fmc2_nand_callbacks_setup(fmc2);
>> +
>> +	/* Define ECC layout */
>> +	mtd_set_ooblayout(mtd, &stm32_fmc2_nand_ooblayout_ops);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct nand_controller_ops stm32_fmc2_nand_controller_ops = {
>> +	.attach_chip = stm32_fmc2_attach_chip,
>> +};
>> +
>> +/* FMC2 probe */
>> +static int stm32_fmc2_parse_child(struct device *dev,
>> +				  struct stm32_fmc2 *fmc2,
>> +				  struct device_node *dn)
>> +{
>> +	u32 cs;
>> +	int ret, i;
>> +
>> +	if (!of_get_property(dn, "reg", &fmc2->ncs))
>> +		return -EINVAL;
>> +
>> +	fmc2->ncs /= sizeof(u32);
>> +	if (!fmc2->ncs) {
>> +		dev_err(dev, "invalid reg property size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < fmc2->ncs; i++) {
>> +		ret = of_property_read_u32_index(dn, "reg", i, &cs);
>> +		if (ret) {
>> +			dev_err(dev, "could not retrieve reg property: %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +
>> +		if (cs > FMC2_MAX_CE) {
>> +			dev_err(dev, "invalid reg value: %d\n", cs);
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (fmc2->cs_assigned & BIT(cs)) {
>> +			dev_err(dev, "cs already assigned: %d\n", cs);
>> +			return -EINVAL;
>> +		}
>> +
>> +		fmc2->cs_assigned |= BIT(cs);
>> +		fmc2->cs_used[i] = cs;
>> +	}
>> +
>> +	/* Default cs used */
> 
> s/cs/CS/

OK

> 
>> +	fmc2->cs_sel = fmc2->cs_used[0];
>> +
>> +	/* Timings */
>> +	ret = of_property_read_u8_array(dn, "st,fmc2_timings",
>> +					(u8 *)&fmc2->timings,
>> +					sizeof(fmc2->timings));
>> +	return ret ? 0 : 1;
>> +}
>> +
>> +static int stm32_fmc2_parse_dt(struct device *dev,
>> +			       struct stm32_fmc2 *fmc2)
>> +{
>> +	struct device_node *dn = dev->of_node;
>> +	struct device_node *child;
>> +	int nchips = of_get_child_count(dn);
>> +	int ret = 0;
>> +
>> +	if (!nchips) {
>> +		dev_err(dev, "NAND chip not defined\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (nchips > 1) {
> 
> If you have two CS, can't you have two NAND chips connected?
> 

No HW board has been designed with 2 NAND chips connected. I am not able 
to test this configuration. The driver will be improved when i will be 
able to test such configuration.

>> +		dev_err(dev, "too many NAND chips defined\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for_each_child_of_node(dn, child) {
>> +		ret = stm32_fmc2_parse_child(dev, fmc2, child);
>> +		if (ret < 0) {
>> +			of_node_put(child);
>> +			return ret;
>> +		}
>> +
>> +		nand_set_flash_node(&fmc2->chip, child);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_fmc2_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *rstc;
>> +	struct stm32_fmc2 *fmc2;
>> +	struct resource *res;
>> +	struct mtd_info *mtd;
>> +	struct nand_chip *chip;
>> +	int i, j, ret, irq, timings_def;
>> +
>> +	fmc2 = devm_kzalloc(dev, sizeof(*fmc2), GFP_KERNEL);
>> +	if (!fmc2)
>> +		return -ENOMEM;
>> +
>> +	timings_def = stm32_fmc2_parse_dt(dev, fmc2);
>> +	if (timings_def < 0)
>> +		return timings_def;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	fmc2->io_base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(fmc2->io_base))
>> +		return PTR_ERR(fmc2->io_base);
>> +
>> +	fmc2->io_phys_addr = res->start;
>> +
>> +	for (i = 0, j = 1; i < FMC2_MAX_CE; i++, j += 3) {
> 
> Maybe naming i 'chip_id' and j 'mem_region' or something else, more
> meaningful than just i/j would be more clear?

Ok.

> 
>> +		if (!(fmc2->cs_assigned & BIT(i)))
>> +			continue;
>> +
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, j);
>> +		fmc2->data_base[i] = devm_ioremap_resource(dev, res);
>> +		if (IS_ERR(fmc2->data_base[i]))
>> +			return PTR_ERR(fmc2->data_base[i]);
>> +
>> +		fmc2->data_phys_addr[i] = res->start;
>> +
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, j + 1);
>> +		fmc2->cmd_base[i] = devm_ioremap_resource(dev, res);
>> +		if (IS_ERR(fmc2->cmd_base[i]))
>> +			return PTR_ERR(fmc2->cmd_base[i]);
>> +
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, j + 2);
>> +		fmc2->addr_base[i] = devm_ioremap_resource(dev, res);
>> +		if (IS_ERR(fmc2->addr_base[i]))
>> +			return PTR_ERR(fmc2->addr_base[i]);
>> +	}
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	ret = devm_request_irq(dev, irq, stm32_fmc2_irq, 0,
>> +			       dev_name(dev), fmc2);
>> +	if (ret) {
>> +		dev_err(dev, "failed to request irq\n");
>> +		return ret;
>> +	}
>> +
>> +	init_completion(&fmc2->complete);
>> +
>> +	fmc2->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(fmc2->clk))
>> +		return PTR_ERR(fmc2->clk);
>> +
>> +	ret = clk_prepare_enable(fmc2->clk);
>> +	if (ret) {
>> +		dev_err(dev, "can not enable the clock\n");
>> +		return ret;
>> +	}
>> +
>> +	rstc = devm_reset_control_get(dev, NULL);
>> +	if (!IS_ERR(rstc)) {
>> +		reset_control_assert(rstc);
>> +		reset_control_deassert(rstc);
>> +	}
>> +
>> +	fmc2->dev = dev;
>> +	mtd = nand_to_mtd(&fmc2->chip);
>> +	chip = &fmc2->chip;
>> +	nand_set_controller_data(chip, fmc2);
>> +	mtd->dev.parent = dev;
>> +
>> +	chip->exec_op = stm32_fmc2_exec_op;
>> +	chip->select_chip = stm32_fmc2_select_chip;
>> +	chip->options |= NAND_BUSWIDTH_AUTO | NAND_NO_SUBPAGE_WRITE;
>> +
>> +	/* FMC2 init routine */
>> +	stm32_fmc2_init(fmc2);
>> +	if (timings_def)
>> +		stm32_fmc2_timings_init(fmc2);
> 
> You already implement ->setup_data_interface, why do you need this
> stm32_fmc2_timings_init()?

"st,fmc2-timings" is an optional property that allow the end user to 
overwrite the timings calculated by setup_data_interface callback. In 
case this property is defined in the device tree, timings are directly 
set and stm32_fmc2_setup_interface() will be never called.

> 
>> +	else
>> +		chip->setup_data_interface = stm32_fmc2_setup_interface;
>> +
>> +	/* Default settings */
>> +	chip->ecc.mode = NAND_ECC_HW;
>> +	chip->ecc.size = FMC2_ECC_STEP_SIZE;
>> +	chip->ecc.strength = FMC2_ECC_BCH8;
>> +
>> +	/* Scan to find existence of the device */
>> +	chip->dummy_controller.ops = &stm32_fmc2_nand_controller_ops;
>> +	ret = nand_scan(chip, fmc2->ncs);
>> +	if (ret)
>> +		goto err_scan;
>> +
>> +	ret = mtd_device_register(mtd, NULL, 0);
>> +	if (ret)
>> +		goto err_device_register;
>> +
>> +	platform_set_drvdata(pdev, fmc2);
>> +
>> +	return 0;
>> +
>> +err_device_register:
>> +	nand_cleanup(chip);
>> +
>> +err_scan:
>> +	if (fmc2->dma_ecc_ch)
>> +		dma_release_channel(fmc2->dma_ecc_ch);
>> +	if (fmc2->dma_tx_ch)
>> +		dma_release_channel(fmc2->dma_tx_ch);
>> +	if (fmc2->dma_rx_ch)
>> +		dma_release_channel(fmc2->dma_rx_ch);
>> +
>> +	sg_free_table(&fmc2->dma_data_sg);
>> +	sg_free_table(&fmc2->dma_ecc_sg);
>> +
>> +	clk_disable_unprepare(fmc2->clk);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_fmc2_remove(struct platform_device *pdev)
>> +{
>> +	struct stm32_fmc2 *fmc2 = platform_get_drvdata(pdev);
>> +
>> +	nand_release(&fmc2->chip);
>> +
>> +	if (fmc2->dma_ecc_ch)
>> +		dma_release_channel(fmc2->dma_ecc_ch);
>> +	if (fmc2->dma_tx_ch)
>> +		dma_release_channel(fmc2->dma_tx_ch);
>> +	if (fmc2->dma_rx_ch)
>> +		dma_release_channel(fmc2->dma_rx_ch);
>> +
>> +	sg_free_table(&fmc2->dma_data_sg);
>> +	sg_free_table(&fmc2->dma_ecc_sg);
>> +
>> +	clk_disable_unprepare(fmc2->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int stm32_fmc2_suspend(struct device *dev)
> 
> Please remove the pre-processor conditionals and add
> __maybe_unused in the suspend/resume functions definitions.

Ok.

> 
>> +{
>> +	struct stm32_fmc2 *fmc2 = dev_get_drvdata(dev);
>> +
>> +	clk_disable_unprepare(fmc2->clk);
>> +
>> +	pinctrl_pm_select_sleep_state(dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_fmc2_resume(struct device *dev)
>> +{
>> +	struct stm32_fmc2 *fmc2 = dev_get_drvdata(dev);
>> +	int i, ret;
>> +
>> +	pinctrl_pm_select_default_state(dev);
>> +
>> +	ret = clk_prepare_enable(fmc2->clk);
>> +	if (ret) {
>> +		dev_err(dev, "can not enable the clock\n");
>> +		return ret;
>> +	}
>> +
>> +	stm32_fmc2_init(fmc2);
>> +	stm32_fmc2_timings_init(fmc2);
>> +	stm32_fmc2_setup(fmc2);
>> +
>> +	for (i = 0; i < fmc2->ncs; i++)
>> +		nand_reset(&fmc2->chip, i);
> 
> This means you have one different NAND chip wired on each CS.
> 
> We could have two CS wired to the same NAND chip. Calling nand_reset
> twice would be harmless but a lost of time.

Ok, so i will call nand_reset() only once as this driver only supports
one or two CS wired connected to the same NAND chip.

> 
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(stm32_fmc2_pm_ops, stm32_fmc2_suspend,
>> +			 stm32_fmc2_resume);
>> +
>> +static const struct of_device_id stm32_fmc2_match[] = {
>> +	{.compatible = "st,stm32mp15-fmc2"},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_fmc2_match);
>> +
>> +static struct platform_driver stm32_fmc2_driver = {
>> +	.probe	= stm32_fmc2_probe,
>> +	.remove	= stm32_fmc2_remove,
>> +	.driver	= {
>> +		.name = "stm32-fmc2",
>> +		.of_match_table = stm32_fmc2_match,
>> +		.pm = &stm32_fmc2_pm_ops,
>> +	},
>> +};
>> +module_platform_driver(stm32_fmc2_driver);
>> +
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>> +MODULE_AUTHOR("Christophe Kerello <christophe.kerello@st.com>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 FMC2 nand driver");
>> +MODULE_LICENSE("GPL v2");
> 
> 
> Thanks,
> Miquèl
> 

Thanks,
Christophe Kerello.