Message-ID: <20220609111655.GF2758@thinkpad>
Date:   Thu, 9 Jun 2022 16:46:55 +0530
From:   Manivannan Sadhasivam <mani@...nel.org>
To:     Ansuel Smith <ansuelsmth@...il.com>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-mtd@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/3] mtd: nand: raw: qcom_nandc: add support for
 unprotected spare data pages

On Thu, Jun 09, 2022 at 12:24:22PM +0200, Ansuel Smith wrote:
> On Thu, Jun 09, 2022 at 01:22:54PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jun 08, 2022 at 02:10:28AM +0200, Ansuel Smith wrote:
> > > IPQ8064 nand have special pages where a different layout scheme is used.
> > > These special page are used by boot partition and on reading them
> > > lots of warning are reported about wrong ECC data and if written to
> > > results in broken data and not bootable device.
> > > 
> > > The layout scheme used by these special page consist in using 512 bytes
> > > as the codeword size (even for the last codeword) while writing to CFG0
> > > register. This forces the NAND controller to unprotect the 4 bytes of
> > > spare data.
> > > 
> > > Since the kernel is unaware of this different layout for these special
> > > page, it does try to protect the spare data too during read/write and
> > > warn about CRC errors.
> > > 
> > > Add support for this by permitting the user to declare these special
> > > pages in dts by declaring offset and size of the partition. The driver
> > > internally will convert these value to nand pages.
> > > 
> > > On user read/write the page is checked and if it's a boot page the
> > > correct layout is used.
> > > 
> > > Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> > > ---
> > >  drivers/mtd/nand/raw/qcom_nandc.c | 174 +++++++++++++++++++++++++++++-
> > >  1 file changed, 169 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > > index 1a77542c6d67..06ee9a836a3b 100644
> > > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > > @@ -80,8 +80,10 @@
> > >  #define	DISABLE_STATUS_AFTER_WRITE	4
> > >  #define	CW_PER_PAGE			6
> > >  #define	UD_SIZE_BYTES			9
> > > +#define	UD_SIZE_BYTES_MASK		GENMASK(18, 9)
> > >  #define	ECC_PARITY_SIZE_BYTES_RS	19
> > >  #define	SPARE_SIZE_BYTES		23
> > > +#define	SPARE_SIZE_BYTES_MASK		GENMASK(26, 23)
> > >  #define	NUM_ADDR_CYCLES			27
> > >  #define	STATUS_BFR_READ			30
> > >  #define	SET_RD_MODE_AFTER_STATUS	31
> > > @@ -102,6 +104,7 @@
> > >  #define	ECC_MODE			4
> > >  #define	ECC_PARITY_SIZE_BYTES_BCH	8
> > >  #define	ECC_NUM_DATA_BYTES		16
> > > +#define	ECC_NUM_DATA_BYTES_MASK		GENMASK(25, 16)
> > >  #define	ECC_FORCE_CLK_OPEN		30
> > >  
> > >  /* NAND_DEV_CMD1 bits */
> > > @@ -418,6 +421,19 @@ struct qcom_nand_controller {
> > >  	const struct qcom_nandc_props *props;
> > >  };
> > >  
> > > +/*
> > > + * NAND special boot partitions
> > > + *
> > > + * @page_offset:		offset of the partition where spare data is not protected
> > > + *				by ECC (value in pages)
> > 
> > s/page_offset/offset
> >
> 
> Mhhh i changed this to page_offset since the struct is called
> boot_partition and it can be confusing since internally the values is in
> pages. Someone can think that the real partition offset/size is used
> instead of the value converted in pages.

Hmm, okay.

> 
> > > + * @page_offset:		size of the partition where spare data is not protected
> > > + *				by ECC (value in pages)
> > 

[...]

> > > +static bool
> > > +qcom_nandc_is_boot_page(struct qcom_nand_host *host, int page)
> > 
> > Move function name to previous line. If it exceeds 100 lines then wrap
> > arguments.
> > 
> > s/qcom_nandc_is_boot_page/qcom_nandc_is_boot_partition
> > 
> 
> Again considering the check is done on the page and not on the
> partition, wonder what is correct.
> 

But the end goal is to detect if the page belongs to boot partitionis, right?

Also, mixing up boot_pages and boot_partitions is confusing. Let's stick to the
fact that this patch checks for pages belonging to the boot partition.

> > > +{
> > > +	struct qcom_nand_boot_partition *boot_partition;
> > > +	u32 start, end;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < host->nr_boot_partitions; i++) {
> > > +		boot_partition = &host->boot_partitions[i];
> > > +		start = boot_partition->page_offset;
> > > +		end = start + boot_partition->page_size;
> > > +
> > > +		/* Boot pages are normally at the start of
> > 
> > Block comments should start with:
> > 
> > 	/*
> > 	 * ...
> > 
> > Also, are you sure that only few pages in the partitions have different layout
> > and not all pages? If not, then this comment needs to be reworded.
> > 
> 
> I probably worderd this bad...
> Each page of the boot partitions require the codeword fix.
> But other may have the normal codeword
> For example this is from a Netgear router:
> mtd0: 00c80000 00020000 "qcadata" (require codeword fix)
> mtd1: 00500000 00020000 "APPSBL"  (require codeword fix)
> mtd2: 00080000 00020000 "APPSBLENV" (doesn't require codeword fix)
> mtd3: 00140000 00020000 "art"       (doesn't)
> mtd4: 00140000 00020000 "artbak"    (doesn't)
> mtd5: 00400000 00020000 "kernel"    (doesn't)
> mtd6: 06080000 00020000 "ubi"       (doesn't)
> mtd7: 00700000 00020000 "reserve"   (doesn't)
> 
> This is part of the case tho as there are some partition table with
> something like
> 
> mtd0: 00c80000 00020000 "qcadata" (require codeword fix)
> mtd1: 00500000 00020000 "APPSBL"  (require codeword fix)
> mtd2: 00080000 00020000 "APPSBLENV" (doesn't require codeword fix)
> mtd3: 00140000 00020000 "APPSBL2"  (require codeword fix)
> [...]
> 
> In all case 99% of the time in a normal system what you commonly write
> are the rootfs partition that are AFTER the boot partition so checking
> if the page is after such partition should speed up the check.

This should be included in the comment.

> Now that I think about it, a good idea is also start checking the
> boot_partition table from the end to improve the checking even more.
> 

Yes and if the page lies after last boot partition then we can bail out in the
first iteration itself. But the binding should make it clear that the offset
should be specified in ascending order.

> So tell me if the comments is wronlgy worded.
> 

Based on the above algorithm:

/*
 * Since the frequent access will be to the non-boot partitions like rootfs,
 * optimize the page check by:

 * 1. Checking if the page lies after the last boot partition.
 * 2. Checking from the boot partition end.
 */

> > > +		 * the nand in various partition.
> > > +		 * Check the page from the boot page end first
> > > +		 * to save one extra check and optimize this
> > > +		 * in case real no-boot partition are used.
> > > +		 */
> > > +		if (page < end && page >= start)
> > > +			return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static void
> > > +qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page)
> > > +{
> > > +	bool codeword_fixup = qcom_nandc_is_boot_page(host, page);
> > > +
> > > +	/* Skip conf write if we are already in the correct mode */
> > > +	if (codeword_fixup == host->codeword_fixup)
> > > +		return;
> > > +
> > > +	host->codeword_fixup = codeword_fixup;
> > > +
> > > +	host->cw_data = codeword_fixup ? 512 : 516;
> > > +	host->spare_bytes = host->cw_size - host->ecc_bytes_hw -
> > > +			    host->bbm_size - host->cw_data;
> > > +
> > > +	host->cfg0 &= ~(SPARE_SIZE_BYTES_MASK | UD_SIZE_BYTES_MASK);
> > > +	host->cfg0 |= host->spare_bytes << SPARE_SIZE_BYTES |
> > > +		      host->cw_data << UD_SIZE_BYTES;
> > > +
> > > +	host->ecc_bch_cfg &= ~ECC_NUM_DATA_BYTES_MASK;
> > > +	host->ecc_bch_cfg |= host->cw_data << ECC_NUM_DATA_BYTES;
> > > +	host->ecc_buf_cfg = (codeword_fixup ? 0x1ff : 0x203) << NUM_STEPS;
> > 
> > s/1ff/(512 - 1)
> > s/203/(516 - 1)
> > 
> 
> ok. I wonder if this can just be reduced to (host->cw_data - 1) and
> skipping the check entirely.
> 

Yeah, that sounds even good.

Thanks,
Mani

Powered by blists - more mailing lists