On Fri, 8 Jan 2016 15:53:43 +0530
Archit Taneja <architt@xxxxxxxxxxxxxx> wrote:
>
>
> On 1/8/2016 1:31 PM, Boris Brezillon wrote:
> > On Fri, 8 Jan 2016 12:03:25 +0530
> > Archit Taneja <architt@xxxxxxxxxxxxxx> wrote:
> >
> >> Hi,
> >>
> >> On 1/6/2016 10:35 PM, Boris Brezillon wrote:
> >>> Hi Archit,
> >>>
> >>> On Tue, 5 Jan 2016 10:55:00 +0530
> >>> Archit Taneja <architt@xxxxxxxxxxxxxx> wrote:
> >>>
> >>>> The Qualcomm NAND controller is found in SoCs like IPQ806x, MSM7xx,
> >>>> MDM9x15 series.
> >>>>
> >>>> It exists as a sub block inside the IPs EBI2 (External Bus Interface 2)
> >>>> and QPIC (Qualcomm Parallel Interface Controller). These IPs provide a
> >>>> broader interface for external slow peripheral devices such as LCD and
> >>>> NAND/NOR flash memory or SRAM like interfaces.
> >>>>
> >>>> We add support for the NAND controller found within EBI2. For the SoCs
> >>>> of our interest, we only use the NAND controller within EBI2. Therefore,
> >>>> it's safe for us to assume that the NAND controller is a standalone block
> >>>> within the SoC.
> >>>>
> >>>> The controller supports 512B, 2kB, 4kB and 8kB page 8-bit and 16-bit NAND
> >>>> flash devices. It contains a HW ECC block that supports BCH ECC (4, 8 and
> >>>> 16 bit correction/step) and RS ECC(4 bit correction/step) that covers main
> >>>> and spare data. The controller contains an internal 512 byte page buffer
> >>>> to which we read/write via DMA. The EBI2 type NAND controller uses ADM DMA
> >>>> for register read/write and data transfers. The controller performs page
> >>>> reads and writes at a codeword/step level of 512 bytes. It can support up
> >>>> to 2 external chips of different configurations.
> >>>>
> >>>> The driver prepares register read and write configuration descriptors for
> >>>> each codeword, followed by data descriptors to read or write data from the
> >>>> controller's internal buffer. It uses a single ADM DMA channel that we get
> >>>> via dmaengine API. The controller requires 2 ADM CRCIs for command and
> >>>> data flow control. These are passed via DT.
> >>>>
> >>>> The ecc layout used by the controller is syndrome like, but we can't use
> >>>> the standard syndrome ecc ops because of several reasons. First, the amount
> >>>> of data bytes covered by ecc isn't same in each step. Second, writing to
> >>>> free oob space requires us writing to the entire step in which the oob
> >>>> lies. This forces us to create our own ecc ops.
> >>>>
> >>>> One more difference is how the controller accesses the bad block marker.
> >>>> The controller ignores reading the marker when ECC is enabled. ECC needs
> >>>> to be explicity disabled to read or write to the bad block marker. The
> >>>> nand_bbt helpers library hence can't access BBMs for the controller.
> >>>> For now, we skip the creation of BBT and populate chip->block_bad and
> >>>> chip->block_markbad helpers instead.
> >>>>
> >>>> Reviewed-by: Andy Gross <agross@xxxxxxxxxxxxxx>
> >>>> Reviewed-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> >>>> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> >>>> Signed-off-by: Archit Taneja <architt@xxxxxxxxxxxxxx>
> >>>> ---
> >>>> v5:
> >>>> - split chip/controller structs
> >>>> - simplify layout by considering reserved bytes as part of ECC
> >>>> - create ecc layouts automatically
> >>>> - implement block_bad and block_markbad chip ops instead of
> >>>> - read_oob_raw/write_oob_raw ecc ops to access BBMs.
> >>>> - Add NAND_SKIP_BBTSCAN flag until we get badblockbits support.
> >>>> - misc clean ups
> >>>>
> >>>> v4:
> >>>> - Shrink submit_descs
> >>>> - add desc list node at the end of dma_prep_desc
> >>>> - Endianness and warning fixes
> >>>> - Add Stephen's Signed-off since he provided a patch to fix
> >>>> endianness problems
> >>>>
> >>>> v3:
> >>>> - Refactor dma functions for maximum reuse
> >>>> - Use dma_slave_confing on stack
> >>>> - optimize and clean upempty_page_fixup using memchr_inv
> >>>> - ensure portability with dma register reads using le32_* funcs
> >>>> - use NAND_USE_BOUNCE_BUFFER instead of doing it ourselves
> >>>> - fix handling of return values of dmaengine funcs
> >>>> - constify wherever possible
> >>>> - Remove dependency on ADM DMA in Kconfig
> >>>> - Misc fixes and clean ups
> >>>>
> >>>> v2:
> >>>> - Use new BBT flag that allows us to read BBM in raw mode
> >>>> - reduce memcpy-s in the driver
> >>>> - some refactor and clean ups because of above changes
> >>>>
> >>>> drivers/mtd/nand/Kconfig | 7 +
> >>>> drivers/mtd/nand/Makefile | 1 +
> >>>> drivers/mtd/nand/qcom_nandc.c | 1981 +++++++++++++++++++++++++++++++++++++++++
> >>>> 3 files changed, 1989 insertions(+)
> >>>> create mode 100644 drivers/mtd/nand/qcom_nandc.c
> >>>>
> >>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> >>>> index 95b8d2b..2fccdfb 100644
> >>>> --- a/drivers/mtd/nand/Kconfig
> >>>> +++ b/drivers/mtd/nand/Kconfig
> >>>> @@ -546,4 +546,11 @@ config MTD_NAND_HISI504
> >>>> help
> >>>> Enables support for NAND controller on Hisilicon SoC Hip04.
> >>>>
> >>>> +config MTD_NAND_QCOM
> >>>> + tristate "Support for NAND on QCOM SoCs"
> >>>> + depends on ARCH_QCOM
> >>>> + help
> >>>> + Enables support for NAND flash chips on SoCs containing the EBI2 NAND
> >>>> + controller. This controller is found on IPQ806x SoC.
> >>>> +
> >>>> endif # MTD_NAND
> >>>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> >>>> index 2c7f014..9450cdc 100644
> >>>> --- a/drivers/mtd/nand/Makefile
> >>>> +++ b/drivers/mtd/nand/Makefile
> >>>> @@ -55,5 +55,6 @@ obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/
> >>>> obj-$(CONFIG_MTD_NAND_SUNXI) += sunxi_nand.o
> >>>> obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o
> >>>> obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
> >>>> +obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
> >>>>
> >>>> nand-objs := nand_base.o nand_bbt.o nand_timings.o
> >>>> diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c
> >>>> new file mode 100644
> >>>> index 0000000..a4dd922
> >>>> --- /dev/null
> >>>> +++ b/drivers/mtd/nand/qcom_nandc.c
> >
> > [...]
> >
> >>>
> >>>> +
> >>>> +/*
> >>>> + * when using RS ECC, the NAND controller flags an error when reading an
> >>>> + * erased page. however, there are special characters at certain offsets when
> >>>> + * we read the erased page. we check here if the page is really empty. if so,
> >>>> + * we replace the magic characters with 0xffs
> >>>> + */
> >>>> +static bool empty_page_fixup(struct qcom_nand_host *host, u8 *data_buf)
> >>>> +{
> >>>> + struct qcom_nand_controller *nandc = host->nandc;
> >>>> + struct nand_chip *chip = &host->chip;
> >>>> + struct mtd_info *mtd = nand_to_mtd(chip);
> >>>> + int cwperpage = chip->ecc.steps;
> >>>> + u8 orig1[MAX_NUM_STEPS], orig2[MAX_NUM_STEPS];
> >>>> + int i, j;
> >>>> +
> >>>> + /* if BCH is enabled, HW will take care of detecting erased pages */
> >>>> + if (host->bch_enabled || !host->use_ecc)
> >>>> + return false;
> >>>> +
> >>>> + for (i = 0; i < cwperpage; i++) {
> >>>> + u8 *empty1, *empty2;
> >>>> + u32 flash_status = le32_to_cpu(nandc->reg_read_buf[3 * i]);
> >>>> +
> >>>> + /*
> >>>> + * an erased page flags an error in NAND_FLASH_STATUS, check if
> >>>> + * the page is erased by looking for 0x54s at offsets 3 and 175
> >>>> + * from the beginning of each codeword
> >>>> + */
> >>>> + if (!(flash_status & FS_OP_ERR))
> >>>> + break;
> >>>> +
> >>>> + empty1 = &data_buf[3 + i * host->cw_data];
> >>>> + empty2 = &data_buf[175 + i * host->cw_data];
> >>>> +
> >>>> + /*
> >>>> + * if the error wasn't because of an erased page, bail out and
> >>>> + * and let someone else do the error checking
> >>>> + */
> >>>> + if ((*empty1 == 0x54 && *empty2 == 0xff) ||
> >>>> + (*empty1 == 0xff && *empty2 == 0x54)) {
> >>>> + orig1[i] = *empty1;
> >>>> + orig2[i] = *empty2;
> >>>> +
> >>>> + *empty1 = 0xff;
> >>>> + *empty2 = 0xff;
> >>>> + } else {
> >>>> + break;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + if (i < cwperpage || memchr_inv(data_buf, 0xff, mtd->writesize))
> >>>> + goto not_empty;
> >>>> +
> >>>> + /*
> >>>> + * tell the caller that the page was empty and is fixed up, so that
> >>>> + * parse_read_errors() doesn't think it's an error
> >>>> + */
> >>>> + return true;
> >>>> +
> >>>> +not_empty:
> >>>> + /* restore original values if not empty*/
> >>>> + for (j = 0; j < i; j++) {
> >>>> + data_buf[3 + j * host->cw_data] = orig1[j];
> >>>> + data_buf[175 + j * host->cw_data] = orig2[j];
> >>>> + }
> >>>> +
> >>>> + return false;
> >>>> +}
> >>>
> >>> This empty page detection seems a bit complicated to me. Could you
> >>> consider using nand_check_erased_ecc_chunk() to check is the chunk is
> >>> containing 0xff data instead of implementing your own logic?
> >>
> >> We wouldn't be able to use nand_check_erased_ecc_chunk directly because
> >> there are certain bytes in each chunk that are intentionally not 0xffs.
> >>
> >> But I could make it a two step process, where I first override those
> >> magic bytes with 0xffs, and then use nand_check_erased_ecc_chunk. That
> >> may not reduce tons of code, but would atleast consider possibility
> >> of bitflips in erased pages, which the driver currently doesn't do.
> >
> > Too bad your ECC engines decides to fix some bits even when it cannot
> > fix all of them. Are you sure there's no flag to disable this behavior?
> > Usually ECC engines just flag the data chunk as uncorrectable and leave
> > its data untouched.
>
> I don't think the engine is actually writing to the chunk at any
> point when reading an erased chunk.
>
> This whole method adopted by the engine is just a way of flagging
> to us that it read an erased chunk. Once it reads an erased chunk, it
> flags an error in one of the controller registers, and replaces
> offsets at 3 and 175 in its internal buffer.
>
> I am not sure why the HW didn't just provide another bit field which
> notifies that it it read an erased page. This is what is done in the
> future version of this controller, and we don't need to any of this
> stuff.
Ok, if that's really the case, and those pattern are just a way to
notify the software that it has detected an empty page, then I'm fine
with this implementation.
Just add an extra nand_check_erased_ecc_chunk() call (as you suggested)
if this function returns false, so that you can handle bitflips in
erased pages.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
