Received-SPF: pass (google.com: domain of linux-kernel+bounces-159840-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <4b6e1399-7b1d-4506-9943-32e76aff22f4@linaro.org>
Date: Fri, 26 Apr 2024 11:10:48 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] SPI Nand patch code of SkyHigh Memory
To: KR Kim <kr.kim@skyhighmemory.com>, miquel.raynal@bootlin.com,
 richard@nod.at, vigneshr@ti.com,
 Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Cc: mika.westerberg@linux.intel.com, michael@walle.cc,
 acelan.kao@canonical.com, linux-kernel@vger.kernel.org,
 linux-mtd@lists.infradead.org, moh.sardi@skyhighmemory.com,
 zhi.feng@skyhighmemory.com, changsub.shim@skyhighmemory.com
References: <20240426072033.331212-1-kr.kim@skyhighmemory.com>
Content-Language: en-US
From: Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <20240426072033.331212-1-kr.kim@skyhighmemory.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

+Takahiro, I saw he was quoted as author

Hi, KR,

Thank you for the patch.

Plese run "./scripts/checkpatch.pl --strict" on your patch and fix the
errors and warning that are shown.

Subject is wrong, it should follow how other drivers were introduced.
You may use the following in the next version:
"mtd: spinand: add support for SkyHigh S35ML flashes"

Other comments below.

On 4/26/24 08:20, KR Kim wrote:
> The following list shows the additional features that are required to support Skyhighmemory S35ML0xG3 SPI Nand:
> 
>     [Always ECC On]
>        Always keep the ECC On during Bad Block Marking and Bad Block Checking
>        1. The on-die ECC feature is totally transparent to the host. The ECC parity bits used for this feature do not occupy the NAND spare areas.
>        2. The host is free to have its own ECC engine by using the spare areas that have standard size. 
>        3. We provide this patch to enable users who have limited ECC capabilities on the host side to use the NAND flash. This patch has been tested thoroughly on Linux. 
> 
>     [Change ECC Status information]
>        This patch changes the ECC status information as follows to maintain compatibility.
> 	00 (normal)                               
> 	01(1-2 errors corrected)          
> 	10(3-6 errors corrected)          
> 	11(uncorrectable)

Please read the guide on submitting patches before sending v2:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

It describes how one shall describe the changes. You missed to add your
Signed-of-by tag.

> ---
>  drivers/mtd/nand/spi/Makefile  |   2 +-
>  drivers/mtd/nand/spi/core.c    |  14 +++-
>  drivers/mtd/nand/spi/skyhigh.c | 145 +++++++++++++++++++++++++++++++++
>  include/linux/mtd/spinand.h    |   3 +
>  4 files changed, 162 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 drivers/mtd/nand/spi/Makefile
>  mode change 100644 => 100755 drivers/mtd/nand/spi/core.c
>  create mode 100644 drivers/mtd/nand/spi/skyhigh.c
>  mode change 100644 => 100755 include/linux/mtd/spinand.h

all these file mode changes are wrong, keep the files as they were.

> 
> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
> old mode 100644
> new mode 100755
> index 19cc77288ebb..1e61ab21893a
> --- a/drivers/mtd/nand/spi/Makefile
> +++ b/drivers/mtd/nand/spi/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
> -spinand-objs += micron.o paragon.o toshiba.o winbond.o xtx.o
> +spinand-objs += micron.o paragon.o skyhigh.o toshiba.o winbond.o xtx.o
>  obj-$(CONFIG_MTD_SPI_NAND) += spinand.o
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> old mode 100644
> new mode 100755
> index e0b6715e5dfe..d09b2bd05284
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -196,6 +196,17 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
>  static int spinand_ecc_enable(struct spinand_device *spinand,
>  			      bool enable)
>  {
> +	/* 
> +	 * SkyHigh Memory : always ECC on 

how does the configuration register look like, would you please tell us
its fields?

> +	 * The on-die ECC feature is totally transparent to the host. 

this line brings no benefit, remove it

> +	 * The ECC parity bits used for this feature do not occupy the NAND spare areas.

its already implied, remove the line

> +	 * The host is free to have its own ECC engine by using the spare areas that have standard size. 

Why would one want to have both the on-die and on-host ecc engine work
in parallel?

> +	 * We provide this patch to enable users who have limited ECC capabilities on the host side to use the NAND flash. 

remove this line, it brings no value

> +	 * This patch has been tested thoroughly on Linux. 

all code is considered tested, remove this line
> +	 */
> +	if (spinand->flags & SPINAND_ON_DIE_ECC_MANDATORY)
> +		return 0;

the always on on-die ecc shall be discussed in a dedicated patch, as it
touches the core.
> +
>  	return spinand_upd_cfg(spinand, CFG_ECC_ENABLE,
>  			       enable ? CFG_ECC_ENABLE : 0);
>  }
> @@ -561,7 +572,7 @@ static int spinand_reset_op(struct spinand_device *spinand)
>  			    NULL);
>  }
>  
> -static int spinand_lock_block(struct spinand_device *spinand, u8 lock)
> +int spinand_lock_block(struct spinand_device *spinand, u8 lock)
>  {
>  	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
>  }
> @@ -945,6 +956,7 @@ static const struct spinand_manufacturer *spinand_manufacturers[] = {
>  	&macronix_spinand_manufacturer,
>  	&micron_spinand_manufacturer,
>  	&paragon_spinand_manufacturer,
> +	&skyhigh_spinand_manufacturer,
>  	&toshiba_spinand_manufacturer,
>  	&winbond_spinand_manufacturer,
>  	&xtx_spinand_manufacturer,
> diff --git a/drivers/mtd/nand/spi/skyhigh.c b/drivers/mtd/nand/spi/skyhigh.c
> new file mode 100644
> index 000000000000..f001357b4d85
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/skyhigh.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 SkyHigh Memory Limited

Copyright followed by (c) is redundant. Update years.
> + *
> + * Author: Takahiro Kuwano <takahiro.kuwano@infineon.com>

You shall consider adding Takahiro as author, or co-author, or at least
specify that the patch is derived from Takahiro's work.
> + */
> +

cut

> +static int skyhigh_spinand_ooblayout_ecc(struct mtd_info *mtd, int section,
> +					 struct mtd_oob_region *region)
> +{
> +	if (section)
> +		return -ERANGE;
> +
> +	/* SkyHigh's ecc parity is stored in the internal hidden area */
> +	region->length = 0;

what happens when length is zero?

> +	region->offset = mtd->oobsize;
> +
> +	return 0;
> +}

cut

> +};
> +
> +static int skyhigh_spinand_init(struct spinand_device *spinand)
> +{
> +	return spinand_lock_block(spinand, SKYHIGH_CONFIG_PROTECT_EN);

I see the core unlocks all blocks. Thus I assume you can as well remove
the init method altogether, it brings no functional change.

Cheers,
ta