Re: [PATCH 1/2] mtd: nand: Add Cadence NAND controller driver
Hi Boris
The 01/29/2019 19:19, Boris Brezillon wrote:
+ NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYC)),
+ NAND_OP_PARSER_PATTERN(
+ cadence_nand_cmd,
+ NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, MAX_DATA_SIZE)),
+ NAND_OP_PARSER_PATTERN(
+ cadence_nand_cmd,
+ NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, MAX_DATA_SIZE)),
+ NAND_OP_PARSER_PATTERN(
+ cadence_nand_waitrdy,
+ NAND_OP_PARSER_PAT_WAITRDY_ELEM(false))
+ );
Are you sure you can't pack several instructions into an atomic
controller operation? I'd be surprised if that was not the case...
All write and reads operations are handled by function pointers. Apart from
that I
could handle erase command as a atomic operation. I will add such function to
the parser.
+static int cadence_nand_set_ecc_strength(struct cdns_nand_info *cdns_nand,
+u8 strength)
+{
+ u32 reg;
+ u8 i, corr_str_idx = 0;
+
+ if (cadence_nand_wait_for_idle(cdns_nand)) {
+ dev_err(cdns_nand->dev, "Error. Controller is busy");
+ return -ETIMEDOUT;
+ }
+
+ for (i = 0; i < BCH_MAX_NUM_CORR_CAPS; i++) {
+ if (cdns_nand->ecc_strengths[i] == strength) {
+ corr_str_idx = i;
+ break;
+ }
+ }
The index should be retrieved at init time and stored somewhere to avoid
searching it every time this function is called.
Function is called only once at initilization stage. Do we need to make
a optimalization in succh case?
+
+ reg = readl(cdns_nand->reg + ECC_CONFIG_0);
+ reg &= ~ECC_CONFIG_0_CORR_STR;
+ reg |= FIELD_PREP(ECC_CONFIG_0_CORR_STR, corr_str_idx);
+ writel(reg, cdns_nand->reg + ECC_CONFIG_0);
+
+ return 0;
+}
+
...
+
+static int cadence_nand_set_erase_detection(struct cdns_nand_info *cdns_nand,
+ bool enable,
+ u8 bitflips_threshold)
+{
+ u32 reg;
+
+ if (cadence_nand_wait_for_idle(cdns_nand)) {
+ dev_err(cdns_nand->dev, "Error. Controller is busy");
+ return -ETIMEDOUT;
+ }
+
+ reg = readl(cdns_nand->reg + ECC_CONFIG_0);
+
+ if (enable)
+ reg |= ECC_CONFIG_0_ERASE_DET_EN;
+ else
+ reg &= ~ECC_CONFIG_0_ERASE_DET_EN;
+
+ writel(reg, cdns_nand->reg + ECC_CONFIG_0);
+
+ writel(bitflips_threshold, cdns_nand->reg + ECC_CONFIG_1);
I'm curious, is the threshold defining the max number of bitflips in a
page or in an ECC-chunk (ecc_step_size)?
Threshold defines number of max bitflips in a sector/ecc_step_size.
+static void
+cadence_nand_irq_cleanup(int irqnum, struct cdns_nand_info *cdns_nand)
+{
+ /* disable interrupts */
+ writel(INTR_ENABLE_INTR_EN, cdns_nand->reg + INTR_ENABLE);
+ free_irq(irqnum, cdns_nand);
You don't need that if you use devm_request_irq(), do you?
I agree I do not need it I forgot to remove it.
+
+static int cadence_nand_calc_ecc_bytes_256(int step_size, int strength)
+{
+ return cadence_nand_calc_ecc_bytes(256, strength);
+}
+
+static int cadence_nand_calc_ecc_bytes_512(int step_size, int strength)
+{
+ return cadence_nand_calc_ecc_bytes(512, strength);
+}
+
+static int cadence_nand_calc_ecc_bytes_1024(int step_size, int strength)
+{
+ return cadence_nand_calc_ecc_bytes(1024, strength);
+}
+
+static int cadence_nand_calc_ecc_bytes_2048(int step_size, int strength)
+{
+ return cadence_nand_calc_ecc_bytes(2048, strength);
+}
+
+static int cadence_nand_calc_ecc_bytes_4096(int step_size, int strength)
+{
+ return cadence_nand_calc_ecc_bytes(4096, strength);
+}
And you absolutely don't need those wrappers, just use
cadence_nand_calc_ecc_bytes() directly.
Unfortunately I need these wrappers. It is because size of ecc
does not depend on selected step_size which is on parameter list but it
depends on maximum supported ecc step size which is not avaible in this
function. Lets say that controller supports the following steps sizes
512, 1024, 2048. No matter what step size is set the calculation will be
made always for step size 2048.
I could use such function directly if it contains nand_chip
parameter. Then I could get this parameter from cdns_nand.
I'm stopping here, but I think you got the idea: there's a lot of
duplicated code in this driver, try to factor this out or simplify the
logic.
Thans for review. I will try to simplify the rest of code by myself.
Regards,
Piotr Sroka
Re: [PATCH 1/2] mtd: nand: Add Cadence NAND controller driver
On Tue, 29 Jan 2019 16:07:43 +
Piotr Sroka wrote:
> This patch adds driver for Cadence HPNFC NAND controller.
>
> Signed-off-by: Piotr Sroka
> ---
> drivers/mtd/nand/raw/Kconfig|8 +
> drivers/mtd/nand/raw/Makefile |1 +
> drivers/mtd/nand/raw/cadence_nand.c | 2655
> +++
> drivers/mtd/nand/raw/cadence_nand.h | 631 +
> 4 files changed, 3295 insertions(+)
I'm already afraid by the diff stat. NAND controller drivers are
usually around 2000 lines, sometimes even less. I'm sure you can
simplify this driver a bit.
> create mode 100644 drivers/mtd/nand/raw/cadence_nand.c
I prefer - over _, and I think we should start naming NAND controller
drivers -nand-controller.c instead of -nand.c
> create mode 100644 drivers/mtd/nand/raw/cadence_nand.h
No need to add a header file if it's only used by cadence_nand.c, just
move the definitions directly in the .c file.
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 1a55d3e3d4c5..742dcc947203 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -541,4 +541,12 @@ config MTD_NAND_TEGRA
> is supported. Extra OOB bytes when using HW ECC are currently
> not supported.
>
> +config MTD_NAND_CADENCE
> + tristate "Support Cadence NAND (HPNFC) controller"
> + depends on OF
> + help
> + Enable the driver for NAND flash on platforms using a Cadence NAND
> + controller.
> +
> +
> endif # MTD_NAND
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 57159b349054..9c1301164996 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_CADENCE) += cadence_nand.o
>
> nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
> nand-objs += nand_onfi.o
> diff --git a/drivers/mtd/nand/raw/cadence_nand.c
> b/drivers/mtd/nand/raw/cadence_nand.c
> new file mode 100644
> index ..c941e702d325
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/cadence_nand.c
> @@ -0,0 +1,2655 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Cadence NAND flash controller driver
> + *
> + * Copyright (C) 2019 Cadence
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
I haven't checked, but please make sure you actually need to include
all these headers.
> +
> +#include "cadence_nand.h"
> +
> +MODULE_LICENSE("GPL v2");
Move the MODULE_LICENSE() at the end of the file next to
MODULE_AUTHOR()/MODULE_DESCRIPTION().
> +#define CADENCE_NAND_NAME"cadence_nand"
cadence-nand-controller, and no need to use a macro for that, just put
the name directly where needed.
> +
> +#define MAX_OOB_SIZE_PER_SECTOR 32
> +#define MAX_ADDRESS_CYC 6
> +#define MAX_DATA_SIZE0xFFFC
> +
> +static int cadence_nand_wait_for_thread(struct cdns_nand_info *cdns_nand,
> + int8_t thread);
> +static int cadence_nand_wait_for_idle(struct cdns_nand_info *cdns_nand);
> +static int cadence_nand_cmd(struct nand_chip *chip,
> + const struct nand_subop *subop);
> +static int cadence_nand_waitrdy(struct nand_chip *chip,
> + const struct nand_subop *subop);
Please avoid forward declaration unless it's really needed (which I'm
pretty sure is not the case here).
> +
> +static const struct nand_op_parser cadence_nand_op_parser = NAND_OP_PARSER(
> + NAND_OP_PARSER_PATTERN(
> + cadence_nand_cmd,
> + NAND_OP_PARSER_PAT_CMD_ELEM(false)),
> + NAND_OP_PARSER_PATTERN(
> + cadence_nand_cmd,
Since you have separate parser pattern what the point of using the same
function which then has a switch-case on the instruction type.
> + NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYC)),
> + NAND_OP_PARSER_PATTERN(
> + cadence_nand_cmd,
> + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, MAX_DATA_SIZE)),
> + NAND_OP_PARSER_PATTERN(
> + cadence_nand_cmd,
> + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, MAX_DATA_SIZE)),
> + NAND_OP_PARSER_PATTERN(
> + cadence_nand_waitrdy,
> + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false))
> + );
Are you sure you can't pack several instructions into an atomic
controller operation? I'd be surprised if that was not the case...
> +
> +static inline struct cdns_nand_info *mtd_cdns_nand_info(struct mtd_info *mtd)
Drop the inline specifier, the compiler is smart enough to figure
