From: Ansuel Smith <ansuelsmth@gmail.com>
To: Manivannan Sadhasivam <mani@kernel.org>
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
Date: Wed, 15 Jun 2022 18:35:26 +0200 [thread overview]
Message-ID: <62aa16f6.1c69fb81.816fc.be7a@mx.google.com> (raw)
In-Reply-To: <20220615172802.GB3606@thinkpad>
On Wed, Jun 15, 2022 at 10:58:02PM +0530, Manivannan Sadhasivam wrote:
> 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
>
Sure! Thanks a lot for the various review. Will send v8 that should be
the final version.
> > ---
> > 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
>
> --
> மணிவண்ணன் சதாசிவம்
--
Ansuel
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2022-06-15 17:29 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-15 0:06 [PATCH v7 0/3] Add support for unprotected spare data page Ansuel Smith
2022-06-15 0:06 ` [PATCH v7 1/3] mtd: nand: raw: qcom_nandc: reorder qcom_nand_host struct Ansuel Smith
2022-06-15 17:11 ` Manivannan Sadhasivam
2022-06-16 0:18 ` Ansuel Smith
2022-06-16 14:37 ` Miquel Raynal
2022-06-16 14:39 ` Christian Marangi
2022-06-16 21:27 ` Manivannan Sadhasivam
2022-06-16 20:27 ` Manivannan Sadhasivam
2022-06-16 20:29 ` Manivannan Sadhasivam
2022-06-15 0:06 ` [PATCH v7 2/3] mtd: nand: raw: qcom_nandc: add support for unprotected spare data pages Ansuel Smith
2022-06-15 17:28 ` Manivannan Sadhasivam
2022-06-15 16:35 ` Ansuel Smith [this message]
2022-06-15 0:06 ` [PATCH v7 3/3] dt-bindings: mtd: qcom_nandc: document qcom,boot-partitions binding Ansuel Smith
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=62aa16f6.1c69fb81.816fc.be7a@mx.google.com \
--to=ansuelsmth@gmail.com \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=mani@kernel.org \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).