Re: [PATCH 2/3] ARM: i.MX: implement GPMI NAND xload

[Andrej Picej <andrej.picej@norik.com>]

Hi Sascha,

thanks for your comments.

Responses inline.

On 21. 01. 21 10:01, Sascha Hauer wrote:
> Hi Andrej,
> 
> Some comments inline.
> 
> On Wed, Jan 20, 2021 at 01:51:06PM +0100, Andrej Picej wrote:
>> From: Sascha Hauer <s.hauer@pengutronix.de>
>>
>> Commit is based on initial Sascha Hauer's work. It implements PBL xload
>> mechanism to load image from GPMI NAND flash.
>>
>> Additional work was done, so that the NAND's size, page size and OOB's
>> size are autodetected and not hardcoded. Detection method follows the
>> same methods as used in NAND driver, meaning NAND ONFI support is probed
>> and if NAND supports ONFI, NAND memory organization is read from ONFI
>> parameter page otherwise "READ ID" is used.
>>
>> Currently only implemented for i.MX6 familly of SoCs.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>>   arch/arm/mach-imx/Makefile             |    2 +-
>>   arch/arm/mach-imx/include/mach/xload.h |    1 +
>>   arch/arm/mach-imx/xload-gpmi-nand.c    | 1163 ++++++++++++++++++++++++
>>   3 files changed, 1165 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/arm/mach-imx/xload-gpmi-nand.c
>>
>> diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
>> index e45f758e9..d94c846a1 100644
>> --- a/arch/arm/mach-imx/Makefile
>> +++ b/arch/arm/mach-imx/Makefile
>> +static int mxs_nand_get_info(struct mxs_nand_info *info, void *databuf)
>> +{
>> +	int ret, i;
>> +
>> +	ret = mxs_nand_check_onfi(info, databuf);
>> +	if (ret) {
>> +		if (ret != 1)
>> +			return ret;
>> +		pr_warn("ONFI not supported, try \"READ ID\"...\n");
> 
> You already printed a "ONFI not supported\n" message. Printing it once
> is enough. Also this message appears with every non-ONFI nand, right? In
> that case it should rather be pr_info()

OK, I agree, will fix it in v2.

> 
>> +	/*
>> +	 * If ONFI is not supported or if it fails try to get NAND's info from
>> +	 * "READ ID" command.
>> +	 */
>> +	pr_debug("Trying \"READ ID\" command...\n");
>> +	ret = mxs_nand_get_readid(info, databuf);
>> +	if (ret) {
>> +		if (ret != -EOVERFLOW)
>> +			return ret;
>> +
>> +		/*
>> +		 * If NAND's "READ ID" returns bad values, try to set them to
>> +		 * default (most common NAND memory org.) and continue.
>> +		 */
>> +		pr_warn("NANDs READ ID command returned bad values" \
>> +			" set them to default and try to continue!\n");
>> +		info->organization.pagesize = 2048;
>> +		info->organization.oobsize = 64;
>> +		info->nand_size = SZ_1G;
> 
> Is this worth it? READ ID is the most basic command, when this doesn't
> work I don't think there's a point in continuing.

We had a case with Samsung K9K8G08U0E when sometimes (reasons not known) 
the 5th byte returned 0xff instead of correct values, all other returned 
values were correct. In this case the device booted successfully because 
of this hook. Maybe a better solution would be to check only the 5th 
byte for 0xff value (5th byte is not supported from all NAND vendors), 
if this is the case set the NAND size to 1GB.
Would that make more sense?

> 
>> +	}
>> +succ:
>> +	pr_debug("NAND page_size: %d\n", info->organization.pagesize);
>> +	pr_debug("NAND block_size: %d\n",
>> +		info->organization.pages_per_eraseblock
>> +		* info->organization.pagesize);
>> +	pr_debug("NAND oob_size: %d\n", info->organization.oobsize);
>> +	pr_debug("NAND nand_size: %lu\n", info->nand_size);
>> +	pr_debug("NAND bits_per_cell: %d\n", info->organization.bits_per_cell);
>> +	pr_debug("NAND planes_per_lun: %d\n",
>> +		info->organization.planes_per_lun);
>> +	pr_debug("NAND luns_per_target: %d\n",
>> +		info->organization.luns_per_target);
>> +	pr_debug("NAND eraseblocks_per_lun: %d\n",
>> +		info->organization.eraseblocks_per_lun);
>> +	pr_debug("NAND ntargets: %d\n", info->organization.ntargets);
>> +
>> +
>> +	return 0;
>> +}
>> +
>> +/* ---------------------------- BCB handling part -------------------------- */
>> +
>> +static uint32_t calc_chksum(void *buf, size_t size)
>> +{
>> +	u32 chksum = 0;
>> +	u8 *bp = buf;
>> +	size_t i;
>> +
>> +	for (i = 0; i < size; i++)
>> +		chksum += bp[i];
>> +
>> +	return ~chksum;
>> +}
>> +
>> +static int get_fcb(struct mxs_nand_info *info, void *databuf)
>> +{
>> +	int i, pagenum, ret;
>> +	uint32_t checksum;
>> +	struct fcb_block *fcb = &info->fcb;
>> +
>> +	/* First page read fails, this shouldn't be necessary */
>> +	mxs_nand_read_page(info, info->organization.pagesize,
>> +		info->organization.oobsize, 0, databuf, 1);
>> +
>> +	for (i = 0; i < 4; i++) {
>> +		pagenum = 64 * i;
> 
> I haven't looked at the documentation, but the '64' should probably be pages
> per block, so better info->organization.pages_per_eraseblock.
Yes, I agree, will fix it in v2.

> 
>> +
>> +		ret = mxs_nand_read_page(info, info->organization.pagesize,
>> +			info->organization.oobsize, pagenum, databuf, 1);
>> +		if (ret)
>> +			continue;
>> +
>> +		memcpy(fcb, databuf + mxs_nand_aux_status_offset(),
>> +			sizeof(*fcb));
>> +
>> +		if (fcb->FingerPrint != FCB_FINGERPRINT) {
>> +			pr_err("No FCB found on page %d\n", pagenum);
>> +			continue;
>> +		}
>> +
>> +		checksum = calc_chksum((void *)fcb +
>> +			sizeof(uint32_t), sizeof(*fcb) - sizeof(uint32_t));
>> +
>> +		if (checksum != fcb->Checksum) {
>> +			pr_err("FCB on page %d has invalid checksum. " \
>> +				"Expected: 0x%08x, calculated: 0x%08x",
>> +				pagenum, fcb->Checksum, checksum);
>> +			continue;
>> +		}
>> +
>> +		pr_debug("Found FCB:\n");
>> +		pr_debug("PageDataSize:     0x%08x\n", fcb->PageDataSize);
>> +		pr_debug("TotalPageSize:    0x%08x\n", fcb->TotalPageSize);
>> +		pr_debug("SectorsPerBlock:  0x%08x\n", fcb->SectorsPerBlock);
>> +		pr_debug("FW1_startingPage: 0x%08x\n",
>> +			fcb->Firmware1_startingPage);
>> +		pr_debug("PagesInFW1:       0x%08x\n", fcb->PagesInFirmware1);
>> +		pr_debug("FW2_startingPage: 0x%08x\n",
>> +			fcb->Firmware2_startingPage);
>> +		pr_debug("PagesInFW2:       0x%08x\n", fcb->PagesInFirmware2);
>> +
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int get_dbbt(struct mxs_nand_info *info, void *databuf)
>> +{
>> +	int i, ret;
>> +	int page;
>> +	int startpage = info->fcb.DBBTSearchAreaStartAddress;
>> +	struct dbbt_block dbbt;
>> +
>> +	for (i = 0; i < 4; i++) {
>> +		page = startpage + i * 64;
> 
> Same here.

Again, will fix it in v2.

> 
> Regards,
>   Sascha
> 

Thank you for your comments and review.

Best regards,
Andrej Picej

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox