Message ID | 4D497421.1010603@gandalf.sssup.it |
---|---|
State | Superseded |
Delegated to: | Reinhard Meyer |
Headers | show |
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
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> 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 this->IO_ADDR_W = (void *) IO_ADDR_W; }