[U-Boot,v1,3/7] nand: add nand mtd concat support

add for nand devices mtd concat support. Generic MTD concat
support is already ported to mainline, and used in the cfi_mtd
driver. This patch adds it similiar for nand devices.

Signed-off-by: Heiko Schocher <hs@denx.de>
---

 drivers/mtd/nand/nand.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

On 05/25/2016 12:07 AM, Heiko Schocher wrote:
> add for nand devices mtd concat support. Generic MTD concat
> support is already ported to mainline, and used in the cfi_mtd
> driver. This patch adds it similiar for nand devices.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> 
>  drivers/mtd/nand/nand.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c
> index 8f0a921..5413123 100644
> --- a/drivers/mtd/nand/nand.c
> +++ b/drivers/mtd/nand/nand.c
> @@ -9,6 +9,7 @@
>  #include <common.h>
>  #include <nand.h>
>  #include <errno.h>
> +#include <linux/mtd/concat.h>
>  
>  #ifndef CONFIG_SYS_NAND_BASE_LIST
>  #define CONFIG_SYS_NAND_BASE_LIST { CONFIG_SYS_NAND_BASE }
> @@ -30,6 +31,12 @@ static char dev_name[CONFIG_SYS_MAX_NAND_DEVICE][8];
>  
>  static unsigned long total_nand_size; /* in kiB */
>  
> +#ifdef CONFIG_MTD_CONCAT
> +static int nand_devices_found;
> +static struct mtd_info *mtd_nand_list[CONFIG_SYS_MAX_NAND_DEVICE];
> +static char c_mtd_name[16];
> +#endif

Why is c_mtd_name file-scope rather than local?

> +
>  /* Register an initialized NAND mtd device with the U-Boot NAND command. */
>  int nand_register(int devnum)
>  {
> @@ -49,6 +56,9 @@ int nand_register(int devnum)
>  	 * via the mtdcore infrastructure (e.g. ubi).
>  	 */
>  	add_mtd_device(mtd);
> +#ifdef CONFIG_MTD_CONCAT
> +	mtd_nand_list[nand_devices_found++] = mtd;
> +#endif

Why do we need another list of NAND devices?  Use nand_info[].

>  #endif
>  
>  	total_nand_size += mtd->size / 1024;
> @@ -102,4 +112,23 @@ void nand_init(void)
>  	 */
>  	board_nand_select_device(nand_info[nand_curr_device].priv, nand_curr_device);
>  #endif
> +
> +#ifdef CONFIG_MTD_CONCAT
> +	if (nand_devices_found > 1) {
> +		struct mtd_info *mtd;
> +
> +		/*
> +		 * We detected multiple devices. Concatenate them together.
> +		 */
> +		sprintf(c_mtd_name, "nand%d", nand_devices_found);
> +		mtd = mtd_concat_create(mtd_nand_list, nand_devices_found,
> +					c_mtd_name);
> +
> +		if (mtd == NULL)
> +			return;
> +
> +		if (add_mtd_device(mtd))
> +			return;
> +	}
> +#endif /* CONFIG_MTD_CONCAT */
>  }

Please don't add new features that depend on the old-style NAND init.

-Scott

Hello Scott,

Am 25.05.2016 um 07:30 schrieb Scott Wood:
> On 05/25/2016 12:07 AM, Heiko Schocher wrote:
>> add for nand devices mtd concat support. Generic MTD concat
>> support is already ported to mainline, and used in the cfi_mtd
>> driver. This patch adds it similiar for nand devices.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>
>>   drivers/mtd/nand/nand.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c
>> index 8f0a921..5413123 100644
>> --- a/drivers/mtd/nand/nand.c
>> +++ b/drivers/mtd/nand/nand.c
>> @@ -9,6 +9,7 @@
>>   #include <common.h>
>>   #include <nand.h>
>>   #include <errno.h>
>> +#include <linux/mtd/concat.h>
>>
>>   #ifndef CONFIG_SYS_NAND_BASE_LIST
>>   #define CONFIG_SYS_NAND_BASE_LIST { CONFIG_SYS_NAND_BASE }
>> @@ -30,6 +31,12 @@ static char dev_name[CONFIG_SYS_MAX_NAND_DEVICE][8];
>>
>>   static unsigned long total_nand_size; /* in kiB */
>>
>> +#ifdef CONFIG_MTD_CONCAT
>> +static int nand_devices_found;
>> +static struct mtd_info *mtd_nand_list[CONFIG_SYS_MAX_NAND_DEVICE];
>> +static char c_mtd_name[16];
>> +#endif
>
> Why is c_mtd_name file-scope rather than local?

fixed.

>> +
>>   /* Register an initialized NAND mtd device with the U-Boot NAND command. */
>>   int nand_register(int devnum)
>>   {
>> @@ -49,6 +56,9 @@ int nand_register(int devnum)
>>   	 * via the mtdcore infrastructure (e.g. ubi).
>>   	 */
>>   	add_mtd_device(mtd);
>> +#ifdef CONFIG_MTD_CONCAT
>> +	mtd_nand_list[nand_devices_found++] = mtd;
>> +#endif
>
> Why do we need another list of NAND devices?  Use nand_info[].

Ah, correct ... struct nand_info_t == struct mtd_info ... Hmm...
why we need a "struct nand_info_t" ?

>>   #endif
>>
>>   	total_nand_size += mtd->size / 1024;
>> @@ -102,4 +112,23 @@ void nand_init(void)
>>   	 */
>>   	board_nand_select_device(nand_info[nand_curr_device].priv, nand_curr_device);
>>   #endif
>> +
>> +#ifdef CONFIG_MTD_CONCAT
>> +	if (nand_devices_found > 1) {
>> +		struct mtd_info *mtd;
>> +
>> +		/*
>> +		 * We detected multiple devices. Concatenate them together.
>> +		 */
>> +		sprintf(c_mtd_name, "nand%d", nand_devices_found);
>> +		mtd = mtd_concat_create(mtd_nand_list, nand_devices_found,
>> +					c_mtd_name);
>> +
>> +		if (mtd == NULL)
>> +			return;
>> +
>> +		if (add_mtd_device(mtd))
>> +			return;
>> +	}
>> +#endif /* CONFIG_MTD_CONCAT */
>>   }
>
> Please don't add new features that depend on the old-style NAND init.

Hmm... my add works for the "#ifdef CONFIG_SYS_NAND_SELF_INIT" case
and the "else" case ... where would be the appropriate place?

bye,
Heiko

On 05/25/2016 01:10 AM, Heiko Schocher wrote:
> Hello Scott,
> 
> Am 25.05.2016 um 07:30 schrieb Scott Wood:
>> On 05/25/2016 12:07 AM, Heiko Schocher wrote:
>>> add for nand devices mtd concat support. Generic MTD concat
>>> support is already ported to mainline, and used in the cfi_mtd
>>> driver. This patch adds it similiar for nand devices.
>>>
>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>> ---
>>>
>>>   drivers/mtd/nand/nand.c | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c
>>> index 8f0a921..5413123 100644
>>> --- a/drivers/mtd/nand/nand.c
>>> +++ b/drivers/mtd/nand/nand.c
>>> @@ -9,6 +9,7 @@
>>>   #include <common.h>
>>>   #include <nand.h>
>>>   #include <errno.h>
>>> +#include <linux/mtd/concat.h>
>>>
>>>   #ifndef CONFIG_SYS_NAND_BASE_LIST
>>>   #define CONFIG_SYS_NAND_BASE_LIST { CONFIG_SYS_NAND_BASE }
>>> @@ -30,6 +31,12 @@ static char dev_name[CONFIG_SYS_MAX_NAND_DEVICE][8];
>>>
>>>   static unsigned long total_nand_size; /* in kiB */
>>>
>>> +#ifdef CONFIG_MTD_CONCAT
>>> +static int nand_devices_found;
>>> +static struct mtd_info *mtd_nand_list[CONFIG_SYS_MAX_NAND_DEVICE];
>>> +static char c_mtd_name[16];
>>> +#endif
>>
>> Why is c_mtd_name file-scope rather than local?
> 
> fixed.
> 
>>> +
>>>   /* Register an initialized NAND mtd device with the U-Boot NAND command. */
>>>   int nand_register(int devnum)
>>>   {
>>> @@ -49,6 +56,9 @@ int nand_register(int devnum)
>>>   	 * via the mtdcore infrastructure (e.g. ubi).
>>>   	 */
>>>   	add_mtd_device(mtd);
>>> +#ifdef CONFIG_MTD_CONCAT
>>> +	mtd_nand_list[nand_devices_found++] = mtd;
>>> +#endif
>>
>> Why do we need another list of NAND devices?  Use nand_info[].
> 
> Ah, correct ... struct nand_info_t == struct mtd_info ... Hmm...
> why we need a "struct nand_info_t" ?

It's a typedef, not a struct...  and we don't need it.  I already have a
patch in the set I'm working on for the Linux NAND sync that removes the
typedef, and another that converts nand_info[] into an array of pointers
(which should make calling mtd_concat_create easier).

> 
>>>   #endif
>>>
>>>   	total_nand_size += mtd->size / 1024;
>>> @@ -102,4 +112,23 @@ void nand_init(void)
>>>   	 */
>>>   	board_nand_select_device(nand_info[nand_curr_device].priv, nand_curr_device);
>>>   #endif
>>> +
>>> +#ifdef CONFIG_MTD_CONCAT
>>> +	if (nand_devices_found > 1) {
>>> +		struct mtd_info *mtd;
>>> +
>>> +		/*
>>> +		 * We detected multiple devices. Concatenate them together.
>>> +		 */
>>> +		sprintf(c_mtd_name, "nand%d", nand_devices_found);
>>> +		mtd = mtd_concat_create(mtd_nand_list, nand_devices_found,
>>> +					c_mtd_name);
>>> +
>>> +		if (mtd == NULL)
>>> +			return;
>>> +
>>> +		if (add_mtd_device(mtd))
>>> +			return;
>>> +	}
>>> +#endif /* CONFIG_MTD_CONCAT */
>>>   }
>>
>> Please don't add new features that depend on the old-style NAND init.
> 
> Hmm... my add works for the "#ifdef CONFIG_SYS_NAND_SELF_INIT" case
> and the "else" case ... where would be the appropriate place?

You're right...  For some reason I was thinking that none of this was
called with self-init.

-Scott

Hello Scott,

Am 25.05.2016 um 16:51 schrieb Scott Wood:
> On 05/25/2016 01:10 AM, Heiko Schocher wrote:
>> Hello Scott,
>>
>> Am 25.05.2016 um 07:30 schrieb Scott Wood:
>>> On 05/25/2016 12:07 AM, Heiko Schocher wrote:
>>>> add for nand devices mtd concat support. Generic MTD concat
>>>> support is already ported to mainline, and used in the cfi_mtd
>>>> driver. This patch adds it similiar for nand devices.
>>>>
>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>> ---
>>>>
>>>>    drivers/mtd/nand/nand.c | 29 +++++++++++++++++++++++++++++
>>>>    1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c
>>>> index 8f0a921..5413123 100644
>>>> --- a/drivers/mtd/nand/nand.c
>>>> +++ b/drivers/mtd/nand/nand.c
[...]
>>>> +
>>>>    /* Register an initialized NAND mtd device with the U-Boot NAND command. */
>>>>    int nand_register(int devnum)
>>>>    {
>>>> @@ -49,6 +56,9 @@ int nand_register(int devnum)
>>>>    	 * via the mtdcore infrastructure (e.g. ubi).
>>>>    	 */
>>>>    	add_mtd_device(mtd);
>>>> +#ifdef CONFIG_MTD_CONCAT
>>>> +	mtd_nand_list[nand_devices_found++] = mtd;
>>>> +#endif
>>>
>>> Why do we need another list of NAND devices?  Use nand_info[].
>>
>> Ah, correct ... struct nand_info_t == struct mtd_info ... Hmm...
>> why we need a "struct nand_info_t" ?
>
> It's a typedef, not a struct...  and we don't need it.  I already have a

Oh, yes, sorry.

> patch in the set I'm working on for the Linux NAND sync that removes the
> typedef, and another that converts nand_info[] into an array of pointers
> (which should make calling mtd_concat_create easier).

Thanks!

>>>> @@ -102,4 +112,23 @@ void nand_init(void)
>>>>    	 */
>>>>    	board_nand_select_device(nand_info[nand_curr_device].priv, nand_curr_device);
>>>>    #endif
>>>> +
>>>> +#ifdef CONFIG_MTD_CONCAT
>>>> +	if (nand_devices_found > 1) {
>>>> +		struct mtd_info *mtd;
>>>> +
>>>> +		/*
>>>> +		 * We detected multiple devices. Concatenate them together.
>>>> +		 */
>>>> +		sprintf(c_mtd_name, "nand%d", nand_devices_found);
>>>> +		mtd = mtd_concat_create(mtd_nand_list, nand_devices_found,
>>>> +					c_mtd_name);
>>>> +
>>>> +		if (mtd == NULL)
>>>> +			return;
>>>> +
>>>> +		if (add_mtd_device(mtd))
>>>> +			return;
>>>> +	}
>>>> +#endif /* CONFIG_MTD_CONCAT */
>>>>    }
>>>
>>> Please don't add new features that depend on the old-style NAND init.
>>
>> Hmm... my add works for the "#ifdef CONFIG_SYS_NAND_SELF_INIT" case
>> and the "else" case ... where would be the appropriate place?
>
> You're right...  For some reason I was thinking that none of this was
> called with self-init.

Ok, so I let it here.

bye,
Heiko