On 07/11/2014 02:27 PM, Gupta, Pekon wrote:
>> From: Quadros, Roger
>>> On 07/11/2014 10:43 AM, Gupta, Pekon wrote:
>>>> From: Quadros, Roger
> [...]
>
>>>> @@ -1176,6 +1172,7 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info
>> *mtd,
>>>> {
>>>> struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>>>> mtd);
>>>> + struct nand_chip *chip = mtd->priv;
>>>> int eccbytes = info->nand.ecc.bytes;
>>>> struct gpmc_nand_regs *gpmc_regs = &info->reg;
>>>> u8 *ecc_code;
>>>> @@ -1183,7 +1180,7 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info
>> *mtd,
>>>> u32 val;
>>>> int i, j;
>>>>
>>>> - nsectors = ((readl(info->reg.gpmc_ecc_config) >> 4) & 0x7) + 1;
>>>> + nsectors = chip->ecc.steps;
>>>
>>> Sorry NAK.. I'm sure you are breaking something here :-)
>>>
>>> NAND driver supports multiple ECC schemes like;
>>> OMAP_ECC_CODE_HAM1_HW (support for legacy reasons)
>>> OMAP_ECC_CODE_BCH4_HW_DETECTION_SW (needed for OMAP3 and AM35xx)
>>> OMAP_ECC_CODE_BCH4_HW
>>> OMAP_ECC_CODE_BCH8_HW
>>> OMAP_ECC_CODE_BCH8_HW_DETECTION_SW (needed for OMAP3 and AM35xx)
>>> OMAP_ECC_CODE_BCH16_HW
>>>
>>> IIRC ..
>>> - software based ecc-schemes OMAP_ECC_CODE_BCHx_HW_DETECTION_SW
>>> Reads/Write in per-sector granularity. (here nsector != chip->ecc.steps)
>>
>> OK. I still don't have a full understanding about the ECC schemes so to ensure we
>> don't break anything I can just leave nsectors as it is and probably just store a
>> copy of it in omap_nand_info to avoid reading it back from gpmc_ecc_config.
>>
>> I still have a few questions to clarify my understanding.
>>
>> The only difference between OMAP_ECC_CODE_BCHx_HW_DETECTION_SW and
>> OMAP_ECC_CODE_BCHx_HW is that in the former the _correction_ is done by software
>> and in the latter the _correction_ is done by hardware (i.e. ELM module).
>> In both cases the _detection_ is done by the same hardware IP via ecc.calculate(),
>> i.e. omap_calculate_ecc_bch().
>>
>> so why should nsector be different for both in the _detection_ stage?
>>
> Again IIRC, That is because of ELM driver.
> ELM hardware engine has 8 channels with each of them having 512Bytes capacity.
> Now, there can be two approaches:
> (1) SECTOR_MODE: Process only one sector of 512 bytes at a time, and iterate
> chip->ecc.steps times.
> (2) PAGE_MODE: Process complete page at a time, enabling multiple channels
> simultaneously. This improves some throughput, especially for large-page
> NAND devices and MLC NAND where bit-flips are common.
>
> As ELM uses (2)nd approach, so the GPMC also has to calculate and store
> ECC for complete page at a time. That is why trace NAND driver you will find
> - OMAP_ECC_CODE_BCHx_HW_DETECTION_SW use generic implementation
> chip->ecc.read_page= nand_read_page_hwecc() defined in nand_base.c
> whereas,
> - OMAP_ECC_CODE_BCHx_HW use custom implementation
> chip->ecc.read_page= omap_read_page_bch() defined in omap.c
>
>
>> An I right that ecc_steps is nothing but number of sub-blocks ECC calculation and correction
>> needs to be done for larger pages. This is a function of ECC hw capability (chip->ecc.size)
>> and NAND flash capability (mtd->writesize). i.e. ecc_steps = mtd->writesize / chip->ecc.size
>>
>> We hardcode chip->ecc.size to 512 for all the ECC schemes in omap_nand_probe() so
>> calculate and correct will always be called for 512 byte sized blocks. So when does
>> the usecase for nsector > 1 come in?
>>
> Ok.. I'll try to explain above details again in probably simplified version
> - OMAP_ECC_CODE_BCHx_HW_DETECTION_SW
> uses lib/bch.c (via nand_bch.c) to correct ECC. And is generic implementation
> so here each sector (data chunk of ecc_size) is corrected independently.
> So nsector = 1;
>
> - OMAP_ECC_CODE_BCHx_HW
> Uses ELM to correct ECC. But the way ELM driver is written today. It corrects
> the whole page in single go. Not individual sectors (ecc_size chunks).
Then shouldn't chip->ecc.size be equal page size and chip->ecc.steps be equal to 1 for upto 4KB pages?
For larger pages it can be a multiple of 4KB page size. i.e. 2 for 8KB, 4 for 16KB and so on.
So nsectors is not necessarily equal to ecc.steps but equal to how many 512 byte blocks are there in
one step. i.e. [min(4096, page_size) / 512]. And it must be local to omap NAND driver.
> So, its doable to make it same like OMAP_ECC_CODE_BCHx_HW_DETECTION_SW
> But then you _may_ have performance penalty for new technology NAND and MLC
> NAND on which bit-flips are very common.
> So to keep ELM driver as it is (performance tweaked), we use different
> configurations in GPMC to read complete page in single go. This is where
> nsector = chip->ecc.steps;
>
> Hope that clarifies the implementation..
>
> Good to document this detail somewhere for OMAP drivers both (u-boot and kernel).
> Any thoughts ?
Sure. we have the processors wiki. That should be a good place.
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
