Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 15 Jun 2022 22:58:02 +0530
From:   Manivannan Sadhasivam <mani@kernel.org>
To:     Ansuel Smith <ansuelsmth@gmail.com>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-mtd@lists.infradead.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 2/3] mtd: nand: raw: qcom_nandc: add support for
 unprotected spare data pages
Message-ID: <20220615172802.GB3606@thinkpad>
References: <20220615000612.3119-1-ansuelsmth@gmail.com>
 <20220615000612.3119-3-ansuelsmth@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220615000612.3119-3-ansuelsmth@gmail.com>
Precedence: bulk

On Wed, Jun 15, 2022 at 02:06:11AM +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@gmail.com>

Just a few nitpicks below. With those fixed,

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/mtd/nand/raw/qcom_nandc.c | 203 +++++++++++++++++++++++++++++-
>  1 file changed, 198 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index f2990d721733..0dbfe32888ff 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c

[...]

> +static bool qcom_nandc_is_boot_partition(struct qcom_nand_host *host, int page)
> +{
> +	struct qcom_nand_boot_partition *boot_partition;
> +	u32 start, end;
> +	int i;
> +
> +	/*
> +	 * Since the frequent access will be to the non-boot partitions like rootfs,
> +	 * optimize the page check by:
> +

Missing "*"

> +	 * 1. Checking if the page lies after the last boot partition.
> +	 * 2. Checking from the boot partition end.
> +	 */
> +
> +	/* First check the last boot partition */
> +	boot_partition = &host->boot_partitions[host->nr_boot_partitions - 1];
> +	start = boot_partition->page_offset;
> +	end = start + boot_partition->page_size;
> +
> +	/* Page is after the last boot partition end. This is NOT a boot partition */
> +	if (page > end)
> +		return false;
> +
> +	/* Actually check if it's a boot partition */
> +	if (page < end && page >= start)
> +		return true;
> +
> +	/* Check the other boot partition starting from the second-last partition */

s/boot partition/boot partitions

> +	for (i = host->nr_boot_partitions - 2; i >= 0; i--) {
> +		boot_partition = &host->boot_partitions[i];
> +		start = boot_partition->page_offset;
> +		end = start + boot_partition->page_size;
> +
> +		if (page < end && page >= start)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void
> +qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page)

As like other functions, please align the function on the same line

> +{
> +	bool codeword_fixup = qcom_nandc_is_boot_partition(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 = (host->cw_data - 1) << NUM_STEPS;
> +}

[...]

> +static int qcom_nand_host_parse_boot_partitions(struct qcom_nand_controller *nandc,
> +						struct qcom_nand_host *host,
> +						struct device_node *dn)
> +{
> +	struct nand_chip *chip = &host->chip;
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct qcom_nand_boot_partition *boot_partition;
> +	struct device *dev = nandc->dev;
> +	int partitions_count, i, j, ret;
> +
> +	if (!of_find_property(dn, "qcom,boot-partitions", NULL))
> +		return 0;
> +
> +	partitions_count = of_property_count_u32_elems(dn, "qcom,boot-partitions");
> +	if (partitions_count <= 0) {
> +		dev_err(dev, "Error parsing boot partition\n");
> +		if (partitions_count == 0)
> +			return -EINVAL;
> +		else
> +			return partitions_count;

		return partitions_count ? partitions_count : -EINVAL;

Thanks,
Mani

-- 
மணிவண்ணன் சதாசிவம்