Re: [PATCH 3/3] mtd: rawnand: marvell: add support for AC5 SoC

From: Miquel Raynal
Date: Wed Oct 19 2022 - 05:13:27 EST

Next message: Greg Kroah-Hartman: "[PATCH 6.0 536/862] RDMA/srp: Fix srp_abort()"
Previous message: Greg Kroah-Hartman: "[PATCH 6.0 534/862] RDMA/irdma: Align AE id codes to correct flush code and event"
In reply to: Vadym Kochan: "[PATCH 3/3] mtd: rawnand: marvell: add support for AC5 SoC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Vadym,

vadym.kochan@xxxxxxxxxxx wrote on Wed, 19 Oct 2022 11:20:40 +0300:

> From: Aviram Dali <aviramd@xxxxxxxxxxx>
>
> The following changes were made to add AC5 support:
>
> 1) Modify Marvell nand NFC timing set for mode 0
>
> 2) fix validation in AC5 Nand driver for ONFI timings values modes 1 and 3
>
> 3) remove unnecessary nand timing-mode in device tree of ac5.dtsi
>
> 4) add nand missing AC5X layouts , add option to use ndtr predefined values
>
> 5) Zero steps and total fields of ecc in ecc controller initialization so
> nand_scan_tail() will calculate these two fields, otherwise
> NAND initialization will fail with kernel 5.15 and above.
>
> Signed-off-by: Aviram Dali <aviramd@xxxxxxxxxxx>
> Signed-off-by: Vadym Kochan <vadym.kochan@xxxxxxxxxxx>
> ---
> drivers/mtd/nand/raw/Kconfig | 2 +-
> drivers/mtd/nand/raw/marvell_nand.c | 277 ++++++++++++++++++++++++----
> 2 files changed, 246 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 4cd40af362de..b7bb62f27e02 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -160,7 +160,7 @@ config MTD_NAND_MARVELL
> including:
> - PXA3xx processors (NFCv1)
> - 32-bit Armada platforms (XP, 37x, 38x, 39x) (NFCv2)
> - - 64-bit Aramda platforms (7k, 8k) (NFCv2)
> + - 64-bit Aramda platforms (7k, 8k, ac5) (NFCv2)
>
> config MTD_NAND_SLC_LPC32XX
> tristate "NXP LPC32xx SLC NAND controller"
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index b9d1e96e3334..940a89115782 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -226,6 +226,20 @@
> #define XTYPE_COMMAND_DISPATCH 6
> #define XTYPE_MASK 7
>
> +/* use tRP_min, tWC_min and tWP_min to distinct across timings modes */
> +#define IS_TIMINGS_EQUAL(t1, t2) \
> + ((t1->tRP_min == t2->tRP_min &&\
> + t1->tWC_min == t2->tWC_min &&\
> + t1->tWP_min == t2->tWP_min) ? true : false)
> +
> +/* ndtr0,1 set , each set has few modes level */
> +typedef enum marvell_nfc_timing_mode_set {
> + MARVELL_NFC_NDTR_SET_0, /* tested with ac5 */
> +
> + MARVELL_NFC_NDTR_NUM_OF_SET,
> + MARVELL_NFC_NDTR_SET_NON = MARVELL_NFC_NDTR_NUM_OF_SET
> +} marvell_nfc_timing_mode_set_t;
> +
> /**
> * struct marvell_hw_ecc_layout - layout of Marvell ECC
> *
> @@ -283,14 +297,21 @@ struct marvell_hw_ecc_layout {
>
> /* Layouts explained in AN-379_Marvell_SoC_NFC_ECC */
> static const struct marvell_hw_ecc_layout marvell_nfc_layouts[] = {
> - MARVELL_LAYOUT( 512, 512, 1, 1, 1, 512, 8, 8, 0, 0, 0),
> - MARVELL_LAYOUT( 2048, 512, 1, 1, 1, 2048, 40, 24, 0, 0, 0),
> - MARVELL_LAYOUT( 2048, 512, 4, 1, 1, 2048, 32, 30, 0, 0, 0),
> - MARVELL_LAYOUT( 2048, 512, 8, 2, 1, 1024, 0, 30,1024,32, 30),
> - MARVELL_LAYOUT( 4096, 512, 4, 2, 2, 2048, 32, 30, 0, 0, 0),
> - MARVELL_LAYOUT( 4096, 512, 8, 5, 4, 1024, 0, 30, 0, 64, 30),
> - MARVELL_LAYOUT( 8192, 512, 4, 4, 4, 2048, 0, 30, 0, 0, 0),
> - MARVELL_LAYOUT( 8192, 512, 8, 9, 8, 1024, 0, 30, 0, 160, 30),
> + MARVELL_LAYOUT(512, 512, 1, 1, 1, 512, 8, 8, 0, 0, 0),
> + MARVELL_LAYOUT(2048, 512, 1, 1, 1, 2048, 40, 24, 0, 0, 0),
> + MARVELL_LAYOUT(2048, 512, 4, 1, 1, 2048, 32, 30, 0, 0, 0),
> + MARVELL_LAYOUT(2048, 512, 8, 2, 1, 1024, 0, 30, 1024, 32, 30),
> + MARVELL_LAYOUT(2048, 512, 8, 2, 1, 1024, 0, 30, 1024, 64, 30),
> + MARVELL_LAYOUT(2048, 512, 12, 3, 2, 704, 0, 30, 640, 0, 30),
> + MARVELL_LAYOUT(2048, 512, 16, 5, 4, 512, 0, 30, 0, 32, 30),
> + MARVELL_LAYOUT(4096, 512, 4, 2, 2, 2048, 32, 30, 0, 0, 0),
> + MARVELL_LAYOUT(4096, 512, 8, 5, 4, 1024, 0, 30, 0, 64, 30),
> + MARVELL_LAYOUT(4096, 512, 12, 6, 5, 704, 0, 30, 576, 32, 30),
> + MARVELL_LAYOUT(4096, 512, 16, 9, 8, 512, 0, 30, 0, 32, 30),
> + MARVELL_LAYOUT(8192, 512, 4, 4, 4, 2048, 0, 30, 0, 0, 0),
> + MARVELL_LAYOUT(8192, 512, 8, 9, 8, 1024, 0, 30, 0, 160, 30),
> + MARVELL_LAYOUT(8192, 512, 12, 12, 11, 704, 0, 30, 448, 64, 30),
> + MARVELL_LAYOUT(8192, 512, 16, 17, 16, 512, 0, 30, 0, 32, 30),
> };

If you don't like the layout, it's fine, but you must do it in a
separate commit.

[...]

> +/*
> + * get nand timing-mode from device tree
> + */
> +static int get_nand_timing_mode(struct device_node *np)
> +{
> + int ret;
> + u32 val;
> +
> + ret = of_property_read_u32(np, "nand-timing-mode", &val);
> + return ret ? ret : val;
> +}
> +
> /*
> * Internal helper to conditionnally apply a delay (from the above structure,
> * most of the time).
> @@ -2257,9 +2399,21 @@ static int marvell_nand_hw_ecc_controller_init(struct mtd_info *mtd,
> }
>
> mtd_set_ooblayout(mtd, &marvell_nand_ooblayout_ops);
> - ecc->steps = l->nchunks;
> ecc->size = l->data_bytes;
>
> + /* nand_scan_tail func perform validity tests for ECC strength, and it
> + * assumes that all chunks are with same size. in our case when ecc is 12
> + * the chunk size is 704 but the last chunk is with different size so
> + * we cheat it nand_scan_tail validity tests by set info->ecc_size value to
> + * 512.

The driver already supports this, trying to cheat the core is bad. See
the last members of struct marvell_hw_ecc_layout.

> + */
> + if (ecc->strength == 12)
> + ecc->size = 512;
> +
> + /* let nand_scan_tail() calculate these two fields */
> + ecc->steps = 0;
> + ecc->total = 0;
> +
> if (ecc->strength == 1) {
> chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
> ecc->read_page_raw = marvell_nfc_hw_ecc_hmg_read_page_raw;
> @@ -2360,9 +2514,11 @@ static int marvell_nfc_setup_interface(struct nand_chip *chip, int chipnr,
> struct marvell_nand_chip *marvell_nand = to_marvell_nand(chip);
> struct marvell_nfc *nfc = to_marvell_nfc(chip->controller);
> unsigned int period_ns = 1000000000 / clk_get_rate(nfc->core_clk) * 2;
> - const struct nand_sdr_timings *sdr;
> + const struct nand_sdr_timings *sdr, *timings;
> struct marvell_nfc_timings nfc_tmg;
> int read_delay;
> + marvell_nfc_timing_mode_set_t modes_set;
> + int mode = 0;
>
> sdr = nand_get_sdr_timings(conf);
> if (IS_ERR(sdr))
> @@ -2421,32 +2577,71 @@ static int marvell_nfc_setup_interface(struct nand_chip *chip, int chipnr,
> nfc_tmg.tR = 0;
> }
>
> - if (chipnr < 0)
> - return 0;
>
> - marvell_nand->ndtr0 =
> - NDTR0_TRP(nfc_tmg.tRP) |
> - NDTR0_TRH(nfc_tmg.tRH) |
> - NDTR0_ETRP(nfc_tmg.tRP) |
> - NDTR0_TWP(nfc_tmg.tWP) |
> - NDTR0_TWH(nfc_tmg.tWH) |
> - NDTR0_TCS(nfc_tmg.tCS) |
> - NDTR0_TCH(nfc_tmg.tCH);
> + /* get the timing modes from predefined values according to its compatibility */
> + if (nfc->caps->is_marvell_timing_modes) {
> + /* get the mode set */
> + modes_set = nfc->caps->timing_mode_set;
> + if (modes_set >= MARVELL_NFC_NDTR_SET_NON) {
> + dev_warn(nfc->dev,
> + "Warning: not supported timing registers set,use set number 0 by default\n");
>
> - marvell_nand->ndtr1 =
> - NDTR1_TAR(nfc_tmg.tAR) |
> - NDTR1_TWHR(nfc_tmg.tWHR) |
> - NDTR1_TR(nfc_tmg.tR);
> + modes_set = MARVELL_NFC_NDTR_SET_0;
> + }
>
> - if (nfc->caps->is_nfcv2) {
> - marvell_nand->ndtr0 |=
> - NDTR0_RD_CNT_DEL(read_delay) |
> - NDTR0_SELCNTR |
> - NDTR0_TADL(nfc_tmg.tADL);
> + /* find the caller mode according to timings values */
> + /* if exit on error it means no more modes; not suppose to happen */
> + do {
> + timings = onfi_async_timing_mode_to_sdr_timings(mode);

The timing negotiation is handled by the core, I don't see why you
would need this.

> + if (IS_TIMINGS_EQUAL(timings, sdr))
> + break;
> + mode++;
> + } while (!IS_ERR(timings));
> +
> + /* if mode is not supported by NFC, return false or if nand-timing-mode that
> + * exists in device tree greater then caller mode also return false and wait
> + * for caller to try with next mode (mode-1). we want the nand feature to be
> + * configured with nand-timing-mode value.
> + */
> + if (mode > nfc->caps->max_mode_number ||
> + ((marvell_nand->nand_timing_mode) >= 0 &&
> + (mode > marvell_nand->nand_timing_mode)))
> + return -EOPNOTSUPP;

[...]

> @@ -2681,6 +2877,10 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
> if (of_property_read_bool(np, "marvell,nand-keep-config"))
> chip->options |= NAND_KEEP_TIMINGS;
>
> + /* read the mode from device tree */
> + dn = nand_get_flash_node(chip);
> + marvell_nand->nand_timing_mode = get_nand_timing_mode(dn);

I'm sorry but unless you give me a really convincing explanation, this
is not gonna work. The timing mode should be negotiated between the
controller and the chip, then the controller needs to configure its own
registers to fit these timings. There is no way the timing mode should
be hardcoded in the device tree.

> +
> mtd = nand_to_mtd(chip);
> mtd->dev.parent = dev;
>

Thanks,
Miquèl

Next message: Greg Kroah-Hartman: "[PATCH 6.0 536/862] RDMA/srp: Fix srp_abort()"
Previous message: Greg Kroah-Hartman: "[PATCH 6.0 534/862] RDMA/irdma: Align AE id codes to correct flush code and event"
In reply to: Vadym Kochan: "[PATCH 3/3] mtd: rawnand: marvell: add support for AC5 SoC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]