[U-Boot] [PATCH 1/3] atmel_nand: if don't have gf table in rom code we will build it runtime

[Josh Wu]

Hi, Andreas, Bo

On 10/27/2014 9:47 AM, Bo Shen wrote:
> Hi Andreas,
>
> On 10/25/2014 06:22 AM, Andreas Bießmann wrote:
>> Hi Bo,
>>
>> before diving into that code I have a question. What will be the boot
>> mode for devices without integrated PMECC table? Can the first stage
>> loader be read with PMECC mode?
>> The question arises while thinking about this patch. We have generally
>> two options here:
>>   a) pre-generated PMECC table included in u-boot
>>   b) runtime generated PMECC table in RAM (this solution)
>>
>> While a) will bloat the u-boot binary b) will require some RAM to hold
>> the table. If I understood correctly we will malloc at most 128 KiB for
>> the PMECC table ... Ok option a) is not an option ;)
>> Here my question arises, how can the ROM loader allocate 128 KiB? Do
>> these devices have so many SRAM? Or am I wrong?

Does your ROM loader means the ROM code?
As the ROM code has a pre-generated PMECC table included. ROM code and 
the pre-generated PMECC table are both in the embedded readonly ROM. So 
that's not in the SRAM.

>
>   On SAMA5D4 SoC, the PMECC table is only seen by the ROM loader. So, 
> for atbootstrap, we also need to generate the PMECC table and put it 
> in DDR2 SDRAM.
>   We can use the PMECC generate by atbootstrap, however we must to 
> maintain the address where to put the PMECC table, it will easily 
> cause issues, so we try to regenerate the PMECC table also in u-boot.
>
>> On 16.09.14 09:57, Bo Shen wrote:
>>> From: Josh Wu <josh.wu at atmel.com>
>>>
>>> Add a macro NO_GALOIS_TABLE_IN_ROM. If it is defined we will build a
>>> runtime pmecc galois table.
>>>
>>> Signed-off-by: Josh Wu <josh.wu at atmel.com>
>>> Signed-off-by: Bo Shen <voice.shen at atmel.com>
>>> ---
>>>
>>>   drivers/mtd/nand/atmel_nand.c | 127 
>>> +++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 126 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/nand/atmel_nand.c 
>>> b/drivers/mtd/nand/atmel_nand.c
>>> index e73834d..7cf7b39 100644
>>> --- a/drivers/mtd/nand/atmel_nand.c
>>> +++ b/drivers/mtd/nand/atmel_nand.c
>>> @@ -761,6 +761,108 @@ static int pmecc_choose_ecc(struct 
>>> atmel_nand_host *host,
>>>   }
>>>   #endif
>>>
>>> +#if defined(NO_GALOIS_TABLE_IN_ROM)
>>> +static uint16_t *pmecc_galois_table;
>>> +static int pmecc_build_galois_table(unsigned int mm,
>>> +        int16_t *index_of, int16_t *alpha_to)
>>> +{
>>> +    unsigned int i, mask, nn;
>>> +    unsigned int p[15];
>>> +
>>> +    nn = (1 << mm) - 1;
>>> +    /* set default value */
>>> +    for (i = 1; i < mm; i++)
>>> +        p[i] = 0;
>>
>> memset(p[1], 0, mm-1); // or even the whole field?
>
> OK. I will change it.
>
>>> +
>>> +    /* 1 + X^mm */
>>> +    p[0]  = 1;
>>> +    p[mm] = 1;
>>> +
>>> +    /* others  */
>>> +    switch (mm) {
>>> +    case 3:
>>> +    case 4:
>>> +    case 6:
>>> +    case 15:
>>> +        p[1] = 1;
>>> +        break;
>>> +    case 5:
>>> +    case 11:
>>> +        p[2] = 1;
>>> +        break;
>>> +    case 7:
>>> +    case 10:
>>> +        p[3] = 1;
>>> +        break;
>>> +    case 8:
>>> +        p[2] = 1;
>>> +        p[3] = 1;
>>> +        p[4] = 1;
>>> +        break;
>>> +    case 9:
>>> +        p[4] = 1;
>>> +        break;
>>> +    case 12:
>>> +        p[1] = 1;
>>> +        p[4] = 1;
>>> +        p[6] = 1;
>>> +        break;
>>> +    case 13:
>>> +        p[1] = 1;
>>> +        p[3] = 1;
>>> +        p[4] = 1;
>>> +        break;
>>> +    case 14:
>>> +        p[1] = 1;
>>> +        p[6] = 1;
>>> +        p[10] = 1;
>>> +        break;
>>> +    default:
>>> +        /* Error */
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* Build alpha ^ mm it will help to generate the field 
>>> (primitiv) */
>>> +    alpha_to[mm] = 0;
>>> +    for (i = 0; i < mm; i++)
>>> +        if (p[i])
>>> +            alpha_to[mm] |= 1 << i;
>>
>> In fact p[] is a bit-field which is then copied to alpha_to[mm], why not
>> use alpha_to[mm] in the switch-case and eliminate p[]?
>
> I will consider that.
>
>>> +
>>> +    /*
>>> +     * Then build elements from 0 to mm - 1. As degree is less than mm
>>> +     * so it is just a logical shift.
>>> +     */
>>> +    mask = 1;
>>> +    for (i = 0; i < mm; i++) {
>>> +        alpha_to[i] = mask;
>>> +        index_of[alpha_to[i]] = i;
>>> +        mask <<= 1;
>>> +    }
>>> +
>>> +    index_of[alpha_to[mm]] = mm;
>>> +
>>> +    /* use a mask to select the MSB bit of the LFSR */
>>> +    mask >>= 1;
>>> +
>>> +    /* then finish the building */
>>> +    for (i = mm + 1; i <= nn; i++) {
>>> +        /* check if the msb bit of the lfsr is set */
>>> +        if (alpha_to[i - 1] & mask)
>>> +            alpha_to[i] = alpha_to[mm] ^
>>> +                ((alpha_to[i - 1] ^ mask) << 1);
>>> +        else
>>> +            alpha_to[i] = alpha_to[i - 1] << 1;
>>> +
>>> +        index_of[alpha_to[i]] = i % nn;
>>> +    }
>>> +
>>> +    /* index of 0 is undefined in a multiplicative field */
>>> +    index_of[0] = -1;
>>> +
>>> +    return 0;
>>
>> The whole function is so longish and complicated ... I don't really like
>> it. Maybe eliminating p[] makes it better?
>
> I will try to check the table would be usable in BCH library code.

I already submitted a patch to the kernel: 
http://patchwork.ozlabs.org/patch/398827/
In that patch, this function is rewritten according to BCH code.
So I think maybe after that patch is accepted by kernel, then, we can 
resend new version patch to u-boot.

Best Regards,
Josh Wu