From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EC209ECE560 for ; Mon, 24 Sep 2018 16:38:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1B9832098A for ; Mon, 24 Sep 2018 16:38:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1B9832098A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=st.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732028AbeIXWlC (ORCPT ); Mon, 24 Sep 2018 18:41:02 -0400 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:60520 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1729080AbeIXWlA (ORCPT ); Mon, 24 Sep 2018 18:41:00 -0400 Received: from pps.filterd (m0046660.ppops.net [127.0.0.1]) by mx08-.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id w8OGZfDr026788; Mon, 24 Sep 2018 18:36:41 +0200 Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx08-00178001.pphosted.com with ESMTP id 2mnav5c62m-1 (version=TLSv1 cipher=ECDHE-RSA-AES256-SHA bits=256 verify=NOT); Mon, 24 Sep 2018 18:36:40 +0200 Received: from zeta.dmz-eu.st.com (zeta.dmz-eu.st.com [164.129.230.9]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 64A903F; Mon, 24 Sep 2018 16:36:39 +0000 (GMT) Received: from Webmail-eu.st.com (sfhdag6node2.st.com [10.75.127.17]) by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id A783EABF7; Mon, 24 Sep 2018 16:36:38 +0000 (GMT) Received: from [10.201.23.29] (10.75.127.46) by SFHDAG6NODE2.st.com (10.75.127.17) with Microsoft SMTP Server (TLS) id 15.0.1347.2; Mon, 24 Sep 2018 18:36:37 +0200 Subject: Re: [PATCH 2/3] mtd: rawnand: stm32_fmc2: add STM32 FMC2 NAND flash controller driver To: Miquel Raynal CC: , , , , , , , , , , 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 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 X-Originating-IP: [10.75.127.46] X-ClientProxiedBy: SFHDAG5NODE1.st.com (10.75.127.13) To SFHDAG6NODE2.st.com (10.75.127.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-09-24_10:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > wrote on Mon, 17 Sep 2018 17:47:39 +0200: > >> From: Christophe Kerello >> >> 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 >> --- >> 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 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* 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 "); >> +MODULE_DESCRIPTION("STMicroelectronics STM32 FMC2 nand driver"); >> +MODULE_LICENSE("GPL v2"); > > > Thanks, > Miquèl > Thanks, Christophe Kerello.