Re: [PATCH] nand: denali: enable SoC64 SPL NAND boot with proper Kconfig selection

["Chee, Tien Fong" <tienfong.chee@altera.com>]

Hi Dinesh,

On 5/11/2025 1:19 am, dinesh.maniyam@altera.com wrote:
> From: Dinesh Maniyam<dinesh.maniyam@altera.com>
>
> Add SoC64-specific NAND SPL load support in denali_spl.c and update the
> related Kconfig dependencies.
>
> This patch introduces new helper functions to enable NAND boot support
> for SoCFPGA SoC64 devices using the Denali NAND controller during the
> SPL stage:
>
> - nand_get_mtd(): safely retrieves the active NAND device instance.
> - nand_spl_load_image(): supports image loading with page alignment and
>    bad block skipping using nand_read_skip_bad().
> - Includes nand_util.c for required NAND utility helpers.
> - Wrapped under IS_ENABLED(CONFIG_TARGET_SOCFPGA_SOC64) to limit
>    inclusion to SoC64 targets.
>
> In addition, the SPL_NAND_DENALI Kconfig entry has been updated to:
> - Select required NAND SPL components (BASE, DRIVERS, IDENT, INIT, ECC).
> - Exclude NAND_DENALI_SPARE_AREA_SKIP_BYTES from SoC64 builds, as SoC64
>    NAND handling no longer uses this configuration.
>
> These changes collectively enable functional NAND SPL image loading
> support for SoCFPGA SoC64 devices with Denali NAND.
>
> Signed-off-by: Dinesh Maniyam<dinesh.maniyam@altera.com>
> ---
>   drivers/mtd/nand/raw/Kconfig      |  7 ++-
>   drivers/mtd/nand/raw/denali_spl.c | 73 +++++++++++++++++++++++++++++++
>   2 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 754b99bf3eb..5e567ca4993 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -781,6 +781,11 @@ config SPL_NAND_CADENCE
>   
>   config SPL_NAND_DENALI
>   	bool "Support Denali NAND controller for SPL"
> +	select SPL_NAND_BASE
> +	select SPL_NAND_DRIVERS
> +	select SPL_NAND_IDENT
> +	select SPL_NAND_INIT
> +	select SPL_NAND_ECC


Are these strictly required for all Denali users?
If these are only needed for SoC64, it would be better to move them into:

arch/arm/Kconfig under SOCFPGA / SOC64, or the SoC64 defconfig


>   	depends on SPL_NAND_SUPPORT
>   	help
>   	  This is a small implementation of the Denali NAND controller
> @@ -788,7 +793,7 @@ config SPL_NAND_DENALI
>   
>   config NAND_DENALI_SPARE_AREA_SKIP_BYTES
>   	int "Number of bytes skipped in OOB area"
> -	depends on SPL_NAND_DENALI
> +	depends on SPL_NAND_DENALI && !TARGET_SOCFPGA_SOC64


This should not be added here in the common Kconfig.
If SoC64 does not require this option, simply disable it in:

SoC64 defconfig, or arch/arm/Kconfig under SOCFPGA

The common Denali code should not embed SoCFPGA-specific constraints.

>   	range 0 63
>   	help
>   	  This option specifies the number of bytes to skip from the beginning
> diff --git a/drivers/mtd/nand/raw/denali_spl.c b/drivers/mtd/nand/raw/denali_spl.c
> index b1e2c9d8161..78cdab51a5a 100644
> --- a/drivers/mtd/nand/raw/denali_spl.c
> +++ b/drivers/mtd/nand/raw/denali_spl.c
> @@ -5,13 +5,84 @@
>    */
>   
>   #include <config.h>
> +#include <hang.h>
>   #include <log.h>
> +#include <malloc.h>
> +#include <memalign.h>
> +#include <nand.h>
> +#include <system-constants.h>
>   #include <asm/io.h>
>   #include <asm/unaligned.h>
>   #include <linux/delay.h>
>   #include <linux/mtd/rawnand.h>
>   #include "denali.h"
>   
> +/* Only compile this code for SoCFPGA SoC64 targets */
> +#if IS_ENABLED(CONFIG_TARGET_SOCFPGA_SOC64)


Cannot add the platform specific change here in the common driver.

The Denali SPL driver is shared across platforms, so we should not add 
SoCFPGA-specific logic here.

The function:

int nand_spl_load_image(...)


should be moved to platform-specific code, e.g.:

arch/arm/mach-socfpga/misc_soc64.c


Similarly, helper functions such as:

nand_get_mtd()
nand_spl_load_image()
#include "nand_util.c"


> +
> +struct mtd_info *nand_get_mtd(void)
> +{
> +	struct mtd_info *mtd;
> +
> +	mtd = get_nand_dev_by_index(nand_curr_device);
> +	if (!mtd)
> +		hang();
> +
> +	return mtd;
> +}
> +
> +int nand_spl_load_image(u32 offset, u32 len, void *dst)
> +{
> +	size_t count = len, actual = 0, page_align_overhead = 0;
> +	u32 page_align_offset = 0;
> +	u8 *page_buffer;
> +	int err = 0;
> +	struct mtd_info *mtd;
> +
> +	if (!len || !dst)
> +		return -EINVAL;
> +
> +	mtd = nand_get_mtd();
> +
> +	if ((offset & (mtd->writesize - 1)) != 0) {
> +		page_buffer = malloc_cache_aligned(mtd->writesize);
> +		if (!page_buffer) {
> +			debug("Error: allocating buffer\n");
> +			return -ENOMEM;
> +		}
> +
> +		page_align_overhead = offset % mtd->writesize;
> +		page_align_offset = (offset / mtd->writesize) * mtd->writesize;
> +		count = mtd->writesize;
> +
> +		err = nand_read_skip_bad(mtd, page_align_offset, &count,
> +					 &actual, mtd->size, page_buffer);
> +
> +		if (err)
> +			return err;
> +
> +		count -= page_align_overhead;
> +		count = min((size_t)len, count);
> +		memcpy(dst, page_buffer + page_align_overhead, count);
> +		free(page_buffer);
> +
> +		len -= count;
> +		if (!len)
> +			return err;
> +
> +		offset += count;
> +		dst += count;
> +		count = len;
> +	}
> +
> +	return nand_read_skip_bad(mtd, offset, &count, &actual, mtd->size, dst);
> +}
> +
> +void nand_deselect(void) {}
> +#include "nand_util.c"
> +
> +#else
> +
>   #define DENALI_MAP01		(1 << 26)	/* read/write pages in PIO */
>   #define DENALI_MAP10		(2 << 26)	/* high-level control plane */
>   
> @@ -240,3 +311,5 @@ unsigned int nand_page_size(void)
>   }
>   
>   void nand_deselect(void) {}
> +#endif
> +

Thanks.

Tien Fong