[PATCH 2/3] ARM: i.MX: implement GPMI NAND xload
Andrej Picej
andrej.picej at norik.com
Tue Jan 26 07:18:19 EST 2021
On 26. 01. 21 13:08, Sascha Hauer wrote:
> On Tue, Jan 26, 2021 at 12:40:17PM +0100, Andrej Picej wrote:
>>
>> On 25. 01. 21 10:43, Andrej Picej wrote:
>>> On 25. 01. 21 10:15, Sascha Hauer wrote:
>>>> On Thu, Jan 21, 2021 at 11:28:39AM +0100, Andrej Picej wrote:
>>>>> Hi Sascha,
>>>>>
>>>>> thanks for your comments.
>>>>>
>>>>> Responses inline.
>>>>>
>>>>> On 21. 01. 21 10:01, Sascha Hauer wrote:
>>>>>> Hi Andrej,
>>>>>>
>>>>>> Some comments inline.
>>>>>>
>>>>>> On Wed, Jan 20, 2021 at 01:51:06PM +0100, Andrej Picej wrote:
>>>>>>> From: Sascha Hauer <s.hauer at pengutronix.de>
>>>>>>>
>>>>>>> Commit is based on initial Sascha Hauer's work. It
>>>>>>> implements PBL xload
>>>>>>> mechanism to load image from GPMI NAND flash.
>>>>>>>
>>>>>>> Additional work was done, so that the NAND's size, page size and OOB's
>>>>>>> size are autodetected and not hardcoded. Detection method follows the
>>>>>>> same methods as used in NAND driver, meaning NAND ONFI
>>>>>>> support is probed
>>>>>>> and if NAND supports ONFI, NAND memory organization is read from ONFI
>>>>>>> parameter page otherwise "READ ID" is used.
>>>>>>>
>>>>>>> Currently only implemented for i.MX6 familly of SoCs.
>>>>>>>
>>>>>>> Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
>>>>>>> Signed-off-by: Primoz Fiser <primoz.fiser at norik.com>
>>>>>>> Signed-off-by: Andrej Picej <andrej.picej at norik.com>
>>>>>>> ---
>>>>>>> arch/arm/mach-imx/Makefile | 2 +-
>>>>>>> arch/arm/mach-imx/include/mach/xload.h | 1 +
>>>>>>> arch/arm/mach-imx/xload-gpmi-nand.c | 1163
>>>>>>> ++++++++++++++++++++++++
>>>>>>> 3 files changed, 1165 insertions(+), 1 deletion(-)
>>>>>>> create mode 100644 arch/arm/mach-imx/xload-gpmi-nand.c
>>>>>>>
>>>>>>> diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
>>>>>>> index e45f758e9..d94c846a1 100644
>>>>>>> --- a/arch/arm/mach-imx/Makefile
>>>>>>> +++ b/arch/arm/mach-imx/Makefile
>>>>>>> +static int mxs_nand_get_info(struct mxs_nand_info
>>>>>>> *info, void *databuf)
>>>>>>> +{
>>>>>>> + int ret, i;
>>>>>>> +
>>>>>>> + ret = mxs_nand_check_onfi(info, databuf);
>>>>>>> + if (ret) {
>>>>>>> + if (ret != 1)
>>>>>>> + return ret;
>>>>>>> + pr_warn("ONFI not supported, try \"READ ID\"...\n");
>>>>>>
>>>>>> You already printed a "ONFI not supported\n" message. Printing it once
>>>>>> is enough. Also this message appears with every non-ONFI
>>>>>> nand, right? In
>>>>>> that case it should rather be pr_info()
>>>>>
>>>>> OK, I agree, will fix it in v2.
>>>>>
>>>>>>
>>>>>>> + /*
>>>>>>> + * If ONFI is not supported or if it fails try to
>>>>>>> get NAND's info from
>>>>>>> + * "READ ID" command.
>>>>>>> + */
>>>>>>> + pr_debug("Trying \"READ ID\" command...\n");
>>>>>>> + ret = mxs_nand_get_readid(info, databuf);
>>>>>>> + if (ret) {
>>>>>>> + if (ret != -EOVERFLOW)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * If NAND's "READ ID" returns bad values, try to set them to
>>>>>>> + * default (most common NAND memory org.) and continue.
>>>>>>> + */
>>>>>>> + pr_warn("NANDs READ ID command returned bad values" \
>>>>>>> + " set them to default and try to continue!\n");
>>>>>>> + info->organization.pagesize = 2048;
>>>>>>> + info->organization.oobsize = 64;
>>>>>>> + info->nand_size = SZ_1G;
>>>>>>
>>>>>> Is this worth it? READ ID is the most basic command, when this doesn't
>>>>>> work I don't think there's a point in continuing.
>>>>>
>>>>> We had a case with Samsung K9K8G08U0E when sometimes (reasons
>>>>> not known) the
>>>>> 5th byte returned 0xff instead of correct values, all other
>>>>> returned values
>>>>> were correct. In this case the device booted successfully
>>>>> because of this
>>>>> hook. Maybe a better solution would be to check only the 5th
>>>>> byte for 0xff
>>>>> value (5th byte is not supported from all NAND vendors), if this
>>>>> is the case
>>>>> set the NAND size to 1GB.
>>>>> Would that make more sense?
>>>>
>>>> The best would be to track the issue down and to fix it ;)
>>>> It's not very nice to assume it in case of read id failures it's exactly
>>>> that NAND type you have troubles with.
>>>> Does it help to reset the NAND chip before reading the ID?
>>>>
>>>> Sascha
>>>>
>>>
>>> I totally agree with fixing the issue and will look into it a bit more.
>>> I will reset the NAND before reading the ID in the same way as it is
>>> done before reading parameter page. I will leave the testing on over
>>> night (reading the ID failed in about 1 out of 500 boots) and report the
>>> results tomorrow.
>>>
>>
>> I guess resetting fixed this problem, I got 50.000 boots without any
>> problems. Will fix this in v2 :).
>>
>> Should I now completely abandon setting the NANDs memory to default values
>> and just return error on failure?
>
> Yes please.
>
>> Something like this:
>>
>>> /*
>>> * If ONFI is not supported or if it fails try to get NAND's info from
>>> * "READ ID" command.
>>> */
>>> ret = mxs_nand_reset(info, databuf);
>>> if (ret)
>>> return ret;
>>> pr_debug("Trying \"READ ID\" command...\n");
>>> ret = mxs_nand_get_readid(info, databuf);
>>> if (ret) {
>>> pr_err("xloader supports only ONFI and generic \"READ ID\" " \
>>> "supported NANDs\n");
>>> return -1;
>>> }
>>
>> I'm asking this because NANDs ID command is manufacturer specific, some
>> manufacturers follow generic values and other unfortunately don't. In
>> barebox NAND driver decoding is done based on manufacturer. I guess we could
>> do the same here but I don't want to expand the PBL part too much just for
>> reading FCB, where all NAND's memory parameters are decoded from.
>
> We can do that once we have to later. Decoding the ID into page/block
> sizes should also be done generically and not part of the i.MX6 NAND
> loader. Maybe we could also utilize parts of the NAND layer instead of
> adding new code.
>
Ok, will prepare v2 and submit it when done.
Thank you for your kind comments and guidance so far.
BR,
Andrej
More information about the barebox
mailing list