RE: [LINUX PATCH v9 4/4] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Hi Miquel, > -Original Message- > From: Miquel Raynal [mailto:miquel.ray...@bootlin.com] > Sent: Friday, June 8, 2018 6:06 PM > To: Naga Sureshkumar Relli > Cc: boris.brezil...@bootlin.com; rich...@nod.at; w...@infradead.org; > computersforpe...@gmail.com; marek.va...@gmail.com; f.faine...@gmail.com; > mma...@broadcom.com; rog...@ti.com; la...@linux-mips.org; a...@thorsis.com; > honghui.zh...@mediatek.com; linux-...@lists.infradead.org; > linux-kernel@vger.kernel.org; > nagasureshkumarre...@gmail.com > Subject: Re: [LINUX PATCH v9 4/4] mtd: rawnand: pl353: Add basic driver for > arm pl353 > smc nand interface > > Hi Naga, > > > > > + ecc->read_page = pl353_nand_read_page_hwecc; > > > > + ecc->size = PL353_NAND_ECC_SIZE; > > > > + ecc->write_page = pl353_nand_write_page_hwecc; > > > > + pl353_smc_set_ecc_pg_size(mtd->writesize); > > > > + switch (mtd->writesize) { > > > > + case SZ_512: > > > > + case SZ_1K: > > > > + case SZ_2K: > > > > + pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_APB); > > > > + break; > > > > + default: > > > > + /* > > > > +* The software ECC routines won't work with the > > > > +* SMC controller > > > > +*/ > > > > + ecc->calculate = nand_calculate_ecc; > > > > + ecc->correct = nand_correct_data; > > > > + ecc->size = 256; > > > > + break; > > > > + } > > > > + if (mtd->writesize <= SZ_512) > > > > + xnand->addr_cycles = 1; > > > > + else > > > > + xnand->addr_cycles = 2; > > > > + > > > > + if (chip->options & NAND_ROW_ADDR_3) > > > > + xnand->addr_cycles += 3; > > > > + else > > > > + xnand->addr_cycles += 2; > > > > + > > > > + if (mtd->oobsize == 16) > > > > + mtd_set_ooblayout(mtd, > > > > &pl353_ecc_ooblayout16_ops); > > > > + else if (mtd->oobsize == 64) > > > > + mtd_set_ooblayout(mtd, > > > > &pl353_ecc_ooblayout64_ops); > > > > > > else? > > You mean to say, add an error condition? > > I do. Ok, I will add it in next version. > > > > > > > > + } > > > > +} > > > > + > > > > +/** > > > > + * pl353_nand_probe - Probe method for the NAND driver > > > > + * @pdev: Pointer to the platform_device structure > > > > + * > > > > + * This function initializes the driver data structures and the > > > > hardware. > > > > + * > > > > + * Return: 0 on success or error value on failure > > > > + */ > > > > +static int pl353_nand_probe(struct platform_device *pdev) { > > > > + struct pl353_nand_info *xnand; > > > > > > xnand is a strange name, more and more because its a bout NAND > > > controller data, not NAND chip. > > We added this name to represent Xilinx Nand(xnand), > > > > > I see where the x comes from. > > Maybe just nfc (for NAND flash controller) or xnfc if you prefer. What > I want is to clearly make the distinction between what is a NAND chip, > what is a NAND controller. Ok, got it. I will update like xnfc. > > > > > + struct mtd_info *mtd; > > > > + struct nand_chip *nand_chip; > > > > > > This one you can call it just "nand" or "chip". > > Ok, I will update. > > > > > > > > > + struct resource *res; > > > > + > > > > + xnand = devm_kzalloc(&pdev->dev, sizeof(*xnand), GFP_KERNEL); > > > > + if (!xnand) > > > > + return -ENOMEM; > > > > + > > > > + /* Map physical address of NAND flash */ > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + xnand->nand_base = devm_ioremap_resource(&pdev->dev, res); > > > > + if (IS_ERR(xnand->nand_base)) > > > > + return PTR_ERR(xnand->nand_base); > > > > + > > > > + nand_chip = &xnand->chip; > > > > + mtd = nand_to_mtd(nand_chip); > > > > + nand_chip->exec_op = pl353_nfc_exec_op; > > > > + nand_set_controller_data(nand_chip, xnand); > > > > + mtd->priv = nand_chip; > > > > + mtd->owner = THIS_MODULE; > > > > + mtd->name = PL353_NAND_DRIVER_NAME; > > > > > > A label property in the DT might overwrite this value. > > Could you please explain a bit more ? > > > > I meant something like this: > > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/marvell_nand.c#L251 > 5 Understood. Thanks. I will update in next version. Thanks, Naga Sureshkumar Relli. > > Thanks, > Miquèl
Re: [LINUX PATCH v9 4/4] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Hi Naga, > > > + ecc->read_page = pl353_nand_read_page_hwecc; > > > + ecc->size = PL353_NAND_ECC_SIZE; > > > + ecc->write_page = pl353_nand_write_page_hwecc; > > > + pl353_smc_set_ecc_pg_size(mtd->writesize); > > > + switch (mtd->writesize) { > > > + case SZ_512: > > > + case SZ_1K: > > > + case SZ_2K: > > > + pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_APB); > > > + break; > > > + default: > > > + /* > > > + * The software ECC routines won't work with the > > > + * SMC controller > > > + */ > > > + ecc->calculate = nand_calculate_ecc; > > > + ecc->correct = nand_correct_data; > > > + ecc->size = 256; > > > + break; > > > + } > > > + if (mtd->writesize <= SZ_512) > > > + xnand->addr_cycles = 1; > > > + else > > > + xnand->addr_cycles = 2; > > > + > > > + if (chip->options & NAND_ROW_ADDR_3) > > > + xnand->addr_cycles += 3; > > > + else > > > + xnand->addr_cycles += 2; > > > + > > > + if (mtd->oobsize == 16) > > > + mtd_set_ooblayout(mtd, &pl353_ecc_ooblayout16_ops); > > > + else if (mtd->oobsize == 64) > > > + mtd_set_ooblayout(mtd, &pl353_ecc_ooblayout64_ops); > > > > else? > You mean to say, add an error condition? I do. > > > > > + } > > > +} > > > + > > > +/** > > > + * pl353_nand_probe - Probe method for the NAND driver > > > + * @pdev:Pointer to the platform_device structure > > > + * > > > + * This function initializes the driver data structures and the hardware. > > > + * > > > + * Return: 0 on success or error value on failure > > > + */ > > > +static int pl353_nand_probe(struct platform_device *pdev) { > > > + struct pl353_nand_info *xnand; > > > > xnand is a strange name, more and more because its a bout NAND controller > > data, not NAND > > chip. > We added this name to represent Xilinx Nand(xnand), > > I see where the x comes from. Maybe just nfc (for NAND flash controller) or xnfc if you prefer. What I want is to clearly make the distinction between what is a NAND chip, what is a NAND controller. > > > + struct mtd_info *mtd; > > > + struct nand_chip *nand_chip; > > > > This one you can call it just "nand" or "chip". > Ok, I will update. > > > > > > + struct resource *res; > > > + > > > + xnand = devm_kzalloc(&pdev->dev, sizeof(*xnand), GFP_KERNEL); > > > + if (!xnand) > > > + return -ENOMEM; > > > + > > > + /* Map physical address of NAND flash */ > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + xnand->nand_base = devm_ioremap_resource(&pdev->dev, res); > > > + if (IS_ERR(xnand->nand_base)) > > > + return PTR_ERR(xnand->nand_base); > > > + > > > + nand_chip = &xnand->chip; > > > + mtd = nand_to_mtd(nand_chip); > > > + nand_chip->exec_op = pl353_nfc_exec_op; > > > + nand_set_controller_data(nand_chip, xnand); > > > + mtd->priv = nand_chip; > > > + mtd->owner = THIS_MODULE; > > > + mtd->name = PL353_NAND_DRIVER_NAME; > > > > A label property in the DT might overwrite this value. > Could you please explain a bit more ? > I meant something like this: https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/marvell_nand.c#L2515 Thanks, Miquèl
RE: [LINUX PATCH v9 4/4] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Hi Miquel, > -Original Message- > From: Miquel Raynal [mailto:miquel.ray...@bootlin.com] > Sent: Friday, June 8, 2018 1:30 AM > To: Naga Sureshkumar Relli > Cc: boris.brezil...@bootlin.com; rich...@nod.at; w...@infradead.org; > computersforpe...@gmail.com; marek.va...@gmail.com; f.faine...@gmail.com; > mma...@broadcom.com; rog...@ti.com; la...@linux-mips.org; a...@thorsis.com; > honghui.zh...@mediatek.com; linux-...@lists.infradead.org; > linux-kernel@vger.kernel.org; > nagasureshkumarre...@gmail.com > Subject: Re: [LINUX PATCH v9 4/4] mtd: rawnand: pl353: Add basic driver for > arm pl353 > smc nand interface > > Hi Naga, > > This is a partial review, enough for this version, see below. Ok, Thanks. > > On Wed, 6 Jun 2018 13:19:42 +0530, Naga Sureshkumar Relli > wrote: > > > Add driver for arm pl353 static memory controller nand interface with > > s/nand/NAND/ Ok I will update. > > > HW ECC support. This controller is used in xilinx zynq soc for > > interfacing > > s/HW/hardware/ Ok I will update. > > > the nand flash memory. > > > > Signed-off-by: Naga Sureshkumar Relli > > > > --- > > Changes in v9: > > - Addressed the below comments given by Miquel > > - instead of using pl353_nand_write32, use directly writel_relaxed > > - Fixed check patch warnings > > - Renamed write_buf/read_buf to write_data_op/read_data_op > > - use BIT macro instead of 1 << nr > > - Use NAND_ROW_ADDR_3 flag > > - Use nand_wait_ready() > > - Removed swecc functions > > - Use address cycles as per size, instead of reading it from > > Parameter page > > - Instead of writing too many patterns, use optional property Changes > > in v8: > > - Added exec_op() implementation > > - Fixed the below v7 review comments > > - removed mtd_info from pl353_nand_info struct > > - Corrected ecc layout offsets > > - Added on-die ecc support > > Changes in v7: > > - Currently not implemented the memclk rate adjustments. I will > >look into this later and once the basic driver is accepted. > > - Fixed GPL licence ident > > Changes in v6: > > - Fixed the checkpatch.pl reported warnings > > - Using the address cycles information from the onfi param page > >earlier it is hardcoded to 5 in driver Changes in v5: > > - Configure the nand timing parameters as per the onfi spec Changes > > in v4: > > - Updated the driver to sync with pl353_smc driver APIs Changes in > > v3: > > - implemented the proper error codes > > - further breakdown this patch to multiple sets > > - added the controller and driver details to Documentation section > > - updated the licenece to GPLv2 > > - reorganized the pl353_nand_ecc_init function Changes in v2: > > - use "depends on" rather than "select" option in kconfig > > - remove unused variable parts > > - remove dummy helper and use writel_relaxed directly > > --- > > > > drivers/mtd/nand/raw/Kconfig |7 + > > drivers/mtd/nand/raw/Makefile |3 + > > drivers/mtd/nand/raw/pl353_nand.c | 1236 > > + > > 3 files changed, 1246 insertions(+) > > create mode 100644 drivers/mtd/nand/raw/pl353_nand.c > > > > diff --git a/drivers/mtd/nand/raw/Kconfig > > b/drivers/mtd/nand/raw/Kconfig index 6871ff0..1c5d528 100644 > > --- a/drivers/mtd/nand/raw/Kconfig > > +++ b/drivers/mtd/nand/raw/Kconfig > > @@ -530,4 +530,11 @@ config MTD_NAND_MTK > > Enables support for NAND controller on MTK SoCs. > > This controller is found on mt27xx, mt81xx, mt65xx SoCs. > > > > +config MTD_NAND_PL353 > > + tristate "ARM Pl353 NAND flash driver" > > + depends on MTD_NAND && ARM > > + depends on PL353_SMC > > + help > > + Enables support for PrimeCell Static Memory Controller PL353. > > + > > endif # MTD_NAND > > diff --git a/drivers/mtd/nand/raw/Makefile > > b/drivers/mtd/nand/raw/Makefile index 165b7ef..6855a0d 100644 > > --- a/drivers/mtd/nand/raw/Makefile > > +++ b/drivers/mtd/nand/raw/Makefile > > @@ -56,7 +56,9 @@ obj-$(CONFIG_MTD_NAND_HISI504)+= > hisi504_nand.o > > 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_PL353) += pl353_nand.o > > > > +CFLAGS_{nand_base.o} := -DDEBUG > > Nope :) For some reason I put
Re: [LINUX PATCH v9 4/4] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Hi Naga, This is a partial review, enough for this version, see below. On Wed, 6 Jun 2018 13:19:42 +0530, Naga Sureshkumar Relli wrote: > Add driver for arm pl353 static memory controller nand interface with s/nand/NAND/ > HW ECC support. This controller is used in xilinx zynq soc for interfacing s/HW/hardware/ > the nand flash memory. > > Signed-off-by: Naga Sureshkumar Relli > --- > Changes in v9: > - Addressed the below comments given by Miquel > - instead of using pl353_nand_write32, use directly writel_relaxed > - Fixed check patch warnings > - Renamed write_buf/read_buf to write_data_op/read_data_op > - use BIT macro instead of 1 << nr > - Use NAND_ROW_ADDR_3 flag > - Use nand_wait_ready() > - Removed swecc functions > - Use address cycles as per size, instead of reading it from Parameter page > - Instead of writing too many patterns, use optional property > Changes in v8: > - Added exec_op() implementation > - Fixed the below v7 review comments > - removed mtd_info from pl353_nand_info struct > - Corrected ecc layout offsets > - Added on-die ecc support > Changes in v7: > - Currently not implemented the memclk rate adjustments. I will >look into this later and once the basic driver is accepted. > - Fixed GPL licence ident > Changes in v6: > - Fixed the checkpatch.pl reported warnings > - Using the address cycles information from the onfi param page >earlier it is hardcoded to 5 in driver > Changes in v5: > - Configure the nand timing parameters as per the onfi spec > Changes in v4: > - Updated the driver to sync with pl353_smc driver APIs > Changes in v3: > - implemented the proper error codes > - further breakdown this patch to multiple sets > - added the controller and driver details to Documentation section > - updated the licenece to GPLv2 > - reorganized the pl353_nand_ecc_init function > Changes in v2: > - use "depends on" rather than "select" option in kconfig > - remove unused variable parts > - remove dummy helper and use writel_relaxed directly > --- > > drivers/mtd/nand/raw/Kconfig |7 + > drivers/mtd/nand/raw/Makefile |3 + > drivers/mtd/nand/raw/pl353_nand.c | 1236 > + > 3 files changed, 1246 insertions(+) > create mode 100644 drivers/mtd/nand/raw/pl353_nand.c > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index 6871ff0..1c5d528 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -530,4 +530,11 @@ config MTD_NAND_MTK > Enables support for NAND controller on MTK SoCs. > This controller is found on mt27xx, mt81xx, mt65xx SoCs. > > +config MTD_NAND_PL353 > + tristate "ARM Pl353 NAND flash driver" > + depends on MTD_NAND && ARM > + depends on PL353_SMC > + help > + Enables support for PrimeCell Static Memory Controller PL353. > + > endif # MTD_NAND > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 165b7ef..6855a0d 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -56,7 +56,9 @@ obj-$(CONFIG_MTD_NAND_HISI504) += > hisi504_nand.o > 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_PL353) += pl353_nand.o > > +CFLAGS_{nand_base.o} := -DDEBUG Nope :) > nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o > nand-objs += nand_amd.o > nand-objs += nand_hynix.o > @@ -64,3 +66,4 @@ nand-objs += nand_macronix.o > nand-objs += nand_micron.o > nand-objs += nand_samsung.o > nand-objs += nand_toshiba.o > + Extra space here > diff --git a/drivers/mtd/nand/raw/pl353_nand.c > b/drivers/mtd/nand/raw/pl353_nand.c > new file mode 100644 > index 000..a880eade > --- /dev/null > +++ b/drivers/mtd/nand/raw/pl353_nand.c > @@ -0,0 +1,1236 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * ARM PL353 NAND flash controller driver > + * > + * Copyright (C) 2017 Xilinx, Inc > + * Author: Punnaiah chowdary kalluri > + * Author: Naga Sureshkumar Relli > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PL353_NAND_DRIVER_NAME "pl353-nand" > + > +/* NAND flash driver defines */ > +#define PL353_NAND_CMD_PHASE 1 /* End command valid in command phase */ > +#define PL353_NAND_DATA_PHASE2 /* End command valid in data > phase */ > +#define PL353_NAND_ECC_SIZE 512 /* Size of data for ECC operation */ > + > +/* Flash memory controller operating parameters */ > + > +#define PL353_NAND_ECC_CONFIG(BIT(4) | /* ECC read at end of > page */ \ > + (0 << 5)) /* No Jumping *
[LINUX PATCH v9 4/4] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Add driver for arm pl353 static memory controller nand interface with HW ECC support. This controller is used in xilinx zynq soc for interfacing the nand flash memory. Signed-off-by: Naga Sureshkumar Relli --- Changes in v9: - Addressed the below comments given by Miquel - instead of using pl353_nand_write32, use directly writel_relaxed - Fixed check patch warnings - Renamed write_buf/read_buf to write_data_op/read_data_op - use BIT macro instead of 1 << nr - Use NAND_ROW_ADDR_3 flag - Use nand_wait_ready() - Removed swecc functions - Use address cycles as per size, instead of reading it from Parameter page - Instead of writing too many patterns, use optional property Changes in v8: - Added exec_op() implementation - Fixed the below v7 review comments - removed mtd_info from pl353_nand_info struct - Corrected ecc layout offsets - Added on-die ecc support Changes in v7: - Currently not implemented the memclk rate adjustments. I will look into this later and once the basic driver is accepted. - Fixed GPL licence ident Changes in v6: - Fixed the checkpatch.pl reported warnings - Using the address cycles information from the onfi param page earlier it is hardcoded to 5 in driver Changes in v5: - Configure the nand timing parameters as per the onfi spec Changes in v4: - Updated the driver to sync with pl353_smc driver APIs Changes in v3: - implemented the proper error codes - further breakdown this patch to multiple sets - added the controller and driver details to Documentation section - updated the licenece to GPLv2 - reorganized the pl353_nand_ecc_init function Changes in v2: - use "depends on" rather than "select" option in kconfig - remove unused variable parts - remove dummy helper and use writel_relaxed directly --- drivers/mtd/nand/raw/Kconfig |7 + drivers/mtd/nand/raw/Makefile |3 + drivers/mtd/nand/raw/pl353_nand.c | 1236 + 3 files changed, 1246 insertions(+) create mode 100644 drivers/mtd/nand/raw/pl353_nand.c diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig index 6871ff0..1c5d528 100644 --- a/drivers/mtd/nand/raw/Kconfig +++ b/drivers/mtd/nand/raw/Kconfig @@ -530,4 +530,11 @@ config MTD_NAND_MTK Enables support for NAND controller on MTK SoCs. This controller is found on mt27xx, mt81xx, mt65xx SoCs. +config MTD_NAND_PL353 + tristate "ARM Pl353 NAND flash driver" + depends on MTD_NAND && ARM + depends on PL353_SMC + help + Enables support for PrimeCell Static Memory Controller PL353. + endif # MTD_NAND diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile index 165b7ef..6855a0d 100644 --- a/drivers/mtd/nand/raw/Makefile +++ b/drivers/mtd/nand/raw/Makefile @@ -56,7 +56,9 @@ obj-$(CONFIG_MTD_NAND_HISI504)+= hisi504_nand.o 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_PL353) += pl353_nand.o +CFLAGS_{nand_base.o} := -DDEBUG nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o nand-objs += nand_amd.o nand-objs += nand_hynix.o @@ -64,3 +66,4 @@ nand-objs += nand_macronix.o nand-objs += nand_micron.o nand-objs += nand_samsung.o nand-objs += nand_toshiba.o + diff --git a/drivers/mtd/nand/raw/pl353_nand.c b/drivers/mtd/nand/raw/pl353_nand.c new file mode 100644 index 000..a880eade --- /dev/null +++ b/drivers/mtd/nand/raw/pl353_nand.c @@ -0,0 +1,1236 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ARM PL353 NAND flash controller driver + * + * Copyright (C) 2017 Xilinx, Inc + * Author: Punnaiah chowdary kalluri + * Author: Naga Sureshkumar Relli + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define PL353_NAND_DRIVER_NAME "pl353-nand" + +/* NAND flash driver defines */ +#define PL353_NAND_CMD_PHASE 1 /* End command valid in command phase */ +#define PL353_NAND_DATA_PHASE 2 /* End command valid in data phase */ +#define PL353_NAND_ECC_SIZE512 /* Size of data for ECC operation */ + +/* Flash memory controller operating parameters */ + +#define PL353_NAND_ECC_CONFIG (BIT(4) | /* ECC read at end of page */ \ +(0 << 5)) /* No Jumping */ + +/* AXI Address definitions */ +#define START_CMD_SHIFT3 +#define END_CMD_SHIFT 11 +#define END_CMD_VALID_SHIFT20 +#define ADDR_CYCLES_SHIFT 21 +#define CLEAR_CS_SHIFT 21 +#define ECC_LAST_SHIFT 10 +#define COMMAND_PHASE (0 << 19) +#define DATA_PHASE BIT(19) + +#define PL353_NAND_ECC_LASTBIT(ECC_LAST_SHIFT) /* Set ECC_Last */ +#define PL353_NAND_CLEAR_CSBIT(CLEAR_CS_SHIFT)