[U-Boot] atmel nand patch CE don't care NAND

Hi,

this patch fix the support for CE don't care nand

Michael Trimarchi

Dear Wolfgang

I have seen that there is not an atmel maintainer. Is it correct to send the patch to arm
one? Who is the right person?

Michael Trimarchi



On 02/02/2011 04:11 PM, Michael Trimarchi wrote:
> Hi,
>
> this patch fix the support for CE don't care nand
>
> Michael Trimarchi
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Dear Michael Trimarchi,

In message <4D4C4A77.4060304@gandalf.sssup.it> you wrote:
>
> I have seen that there is not an atmel maintainer. Is it correct to send the patch to arm
> one? Who is the right person?

Where have you seen that?

The official source of this information is
http://www.denx.de/wiki/U-Boot/Custodians

There you can see that Reinhard is the custodian for Atmel systems.

Best regards,

Wolfgang Denk

On 02/04/2011 08:10 PM, Wolfgang Denk wrote:
> Dear Michael Trimarchi,
>
> In message <4D4C4A77.4060304@gandalf.sssup.it> you wrote:
>> I have seen that there is not an atmel maintainer. Is it correct to send the patch to arm
>> one? Who is the right person?
> Where have you seen that?
>
> The official source of this information is
> http://www.denx.de/wiki/U-Boot/Custodians
It was a mistake throw the gitweb. I have seen the at91 tree,
sorry

Michael
> There you can see that Reinhard is the custodian for Atmel systems.
>
> Best regards,
>
> Wolfgang Denk
>

On Wed, Feb 02, 2011 at 04:11:29PM +0100, Michael Trimarchi wrote:
> Hi,
> 
> this patch fix the support for CE don't care nand
> 
> Michael Trimarchi

Please read http://www.denx.de/wiki/U-Boot/Patches and follow the
format therein -- don't use attachments.

> commit 0cb23ef858407a7a9de6e353e08394637c518c89
> Author: Michael Trimarchi <michael@evidence.eu.com>
> Date:   Wed Feb 2 14:24:21 2011 +0100
> 
>     Fix the CE for the NAND don't care
>     
>     Signed-off-by: Michael Trimarchi <michael@evidence.eu.com>

The changelog needs to be more descriptive.  What is "NAND don't care"?
What is broken?

> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index ab8bbb3..bda117a 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -249,8 +249,13 @@ static void at91_nand_hwcontrol(struct mtd_info *mtd,
>  		if (ctrl & NAND_ALE)
>  			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;
>  
> +		/*
> +		 * Nand CS don't care doesn't need the enable pin
> +		 */
> +#ifdef CONFIG_SYS_NAND_ENABLE_PIN
>  		at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,
>  				    !(ctrl & NAND_NCE));
> +#endif

New CONFIG symbols need to be documented, and this particular one should
probably be less generic.

-Scott

On Tue, 8 Feb 2011 14:29:21 -0600
Scott Wood <scottwood@freescale.com> wrote:

> On Wed, Feb 02, 2011 at 04:11:29PM +0100, Michael Trimarchi wrote:
> > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> > index ab8bbb3..bda117a 100644
> > --- a/drivers/mtd/nand/atmel_nand.c
> > +++ b/drivers/mtd/nand/atmel_nand.c
> > @@ -249,8 +249,13 @@ static void at91_nand_hwcontrol(struct mtd_info *mtd,
> >  		if (ctrl & NAND_ALE)
> >  			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;
> >  
> > +		/*
> > +		 * Nand CS don't care doesn't need the enable pin
> > +		 */
> > +#ifdef CONFIG_SYS_NAND_ENABLE_PIN
> >  		at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,
> >  				    !(ctrl & NAND_NCE));
> > +#endif
> 
> New CONFIG symbols need to be documented, and this particular one should
> probably be less generic.

Sorry, ignore that -- I see it's not new (it should still be documented,
but that's not this patch's problem).

The code change itself looks OK, just needs a better commit
message/comment. Some googling indicates that "CE don't care" refers to the
ability to deassert the chip enable line once an operation has been
initiated.  This seems to be different from not having control of CE at all
(is it just always asserted on these boards?).

-Scott

Hi,

On 02/08/2011 09:29 PM, Scott Wood wrote:
> On Wed, Feb 02, 2011 at 04:11:29PM +0100, Michael Trimarchi wrote:
>> Hi,
>>
>> this patch fix the support for CE don't care nand
>>
>> Michael Trimarchi
>
> Please read http://www.denx.de/wiki/U-Boot/Patches and follow the
> format therein -- don't use attachments.
>
The thunderbird config was wrong. I will use mutt next time
>> commit 0cb23ef858407a7a9de6e353e08394637c518c89
>> Author: Michael Trimarchi <michael@evidence.eu.com>
>> Date:   Wed Feb 2 14:24:21 2011 +0100
>>
>>     Fix the CE for the NAND don't care
>>     
>>     Signed-off-by: Michael Trimarchi <michael@evidence.eu.com>
>
> The changelog needs to be more descriptive.  What is "NAND don't care"?
> What is broken?
>
From atmel documentation:

"CE connection depends on the NAND Flash. For standard NAND Flash devices, it must be connected to any free PIO line.
For “CE don’t care” NAND Flash devices, it can be connected to either NCS3/NANDCS or to any free PIO line."

Hope that is more clear now. Linux use an hook to do it and u-boot a #define

>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index ab8bbb3..bda117a 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -249,8 +249,13 @@ static void at91_nand_hwcontrol(struct mtd_info *mtd,
>>  		if (ctrl & NAND_ALE)
>>  			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;
>>  
>> +		/*
>> +		 * Nand CS don't care doesn't need the enable pin
>> +		 */
>> +#ifdef CONFIG_SYS_NAND_ENABLE_PIN
>>  		at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,
>>  				    !(ctrl & NAND_NCE));
>> +#endif
> New CONFIG symbols need to be documented, and this particular one should
> probably be less generic.
There is not a new CONFIG symbol, I just skip that code that is not necessary
for this type of NAND
> -Scott
>
I will resend it

Michael

Hi

On 02/08/2011 09:42 PM, Scott Wood wrote:
> On Tue, 8 Feb 2011 14:29:21 -0600
> Scott Wood <scottwood@freescale.com> wrote:
> 
>> On Wed, Feb 02, 2011 at 04:11:29PM +0100, Michael Trimarchi wrote:
>>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>>> index ab8bbb3..bda117a 100644
>>> --- a/drivers/mtd/nand/atmel_nand.c
>>> +++ b/drivers/mtd/nand/atmel_nand.c
>>> @@ -249,8 +249,13 @@ static void at91_nand_hwcontrol(struct mtd_info *mtd,
>>>  		if (ctrl & NAND_ALE)
>>>  			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;
>>>  
>>> +		/*
>>> +		 * Nand CS don't care doesn't need the enable pin
>>> +		 */
>>> +#ifdef CONFIG_SYS_NAND_ENABLE_PIN
>>>  		at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,
>>>  				    !(ctrl & NAND_NCE));
>>> +#endif
>>
>> New CONFIG symbols need to be documented, and this particular one should
>> probably be less generic.
> 
> Sorry, ignore that -- I see it's not new (it should still be documented,
> but that's not this patch's problem).

too late :(

> 
> The code change itself looks OK, just needs a better commit
> message/comment. Some googling indicates that "CE don't care" refers to the
> ability to deassert the chip enable line once an operation has been
> initiated.  This seems to be different from not having control of CE at all
> (is it just always asserted on these boards?).

It is connected to the CS3.

> 
> -Scott
> 

Regards
Michael
>

Dear Scott Wood, Michael Trimarchi,
>>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>>> index ab8bbb3..bda117a 100644
>>> --- a/drivers/mtd/nand/atmel_nand.c
>>> +++ b/drivers/mtd/nand/atmel_nand.c
>>> @@ -249,8 +249,13 @@ static void at91_nand_hwcontrol(struct mtd_info *mtd,
>>>   		if (ctrl&  NAND_ALE)
>>>   			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;
>>>
>>> +		/*
>>> +		 * Nand CS don't care doesn't need the enable pin
>>> +		 */
>>> +#ifdef CONFIG_SYS_NAND_ENABLE_PIN
>>>   		at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,
>>>   				    !(ctrl&  NAND_NCE));
>>> +#endif
>>
>> New CONFIG symbols need to be documented, and this particular one should
>> probably be less generic.
>
> Sorry, ignore that -- I see it's not new (it should still be documented,
> but that's not this patch's problem).
>
> The code change itself looks OK, just needs a better commit
> message/comment. Some googling indicates that "CE don't care" refers to the
> ability to deassert the chip enable line once an operation has been
> initiated.  This seems to be different from not having control of CE at all
> (is it just always asserted on these boards?).

I agree that the code looks OK. Apparently it means that CE must be always asserted
(wired to GND) and that access to the chip is solely controlled by RE and WE.
According to (for example) the Samsung K9F2G08X0B data sheet this is possible.

I also agree that the message/comment should point this out for future understanding.

Best Regards,
Reinhard

On Tue, 8 Feb 2011 21:52:10 +0100
Michael Trimarchi <trimarchi@gandalf.sssup.it> wrote:

> Hi,
> 
> On 02/08/2011 09:29 PM, Scott Wood wrote:
> > On Wed, Feb 02, 2011 at 04:11:29PM +0100, Michael Trimarchi wrote:
> >> Hi,
> >>
> >> this patch fix the support for CE don't care nand
> >>
> >> Michael Trimarchi
> >
> > Please read http://www.denx.de/wiki/U-Boot/Patches and follow the
> > format therein -- don't use attachments.
> >
> The thunderbird config was wrong. I will use mutt next time
> >> commit 0cb23ef858407a7a9de6e353e08394637c518c89
> >> Author: Michael Trimarchi <michael@evidence.eu.com>
> >> Date:   Wed Feb 2 14:24:21 2011 +0100
> >>
> >>     Fix the CE for the NAND don't care
> >>     
> >>     Signed-off-by: Michael Trimarchi <michael@evidence.eu.com>
> >
> > The changelog needs to be more descriptive.  What is "NAND don't care"?
> > What is broken?
> >
> From atmel documentation:
> 
> "CE connection depends on the NAND Flash. For standard NAND Flash devices, it must be connected to any free PIO line.
> For “CE don’t care” NAND Flash devices, it can be connected to either NCS3/NANDCS or to any free PIO line."

So the absence of CONFIG_SYS_NAND_ENABLE_PIN means it's connected to
NCS3/NANDCS (which is not quite the same as having a "CE don't care" NAND
chip, since you still have the option of using a PIO line).

I'd just say something like this in the changelog (assuming my assumption
about how this works is correct), and wouldn't bother with an in-code
comment:

atmel_nand: don't require CONFIG_SYS_NAND_ENABLE_PIN

If NCE is hooked up to NCS3, we don't need to (and can't) explicitly set
the state of the NCE pin.  Instead, the controller asserts it
automatically as part of a command/data access.  Only "CE don't
care"-type NAND chips can be used in this manner.

-Scott