Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver
On 07/04/2012 02:46 AM, Jim Lin wrote:
-----Original Message----- From: Scott Wood [mailto:scottwood@freescale.com] Sent: Thursday, April 26, 2012 6:17 AM To: Simon Glass Cc: U-Boot Mailing List; Tom Warren; Stephen Warren; Jim Lin; Stephen Warren Subject: Re: [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver
On 04/17/2012 01:50 PM, Simon Glass wrote:
+static void write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) +{
- int i, j, l;
- struct nand_chip *chip = mtd->priv;
- struct nand_drv *info;
- info = (struct nand_drv *)chip->priv;
- for (i = 0; i < len / 4; i++) {
l = ((int *)buf)[i];If you're assuming the buffer is 32-bit aligned, comment it. Ideally these assumptions should be stated in the interface itself...
This doesn't mean that buf needs to be 32-bit aligned. It only says each write is 32-bit.
OK, didn't realize modern ARM could deal with unaligned accesses.
Should also comment that there's an endian dependency here.
What do you mean? Could you explain more or have an example?
You load a value using host endianness, and store it using a little endian accessor. This would be fine if it represented a real 32-bit integer, but it's really a sequence of bytes that should not be swapped.
It's not a big deal if you don't see the driver ever being used with a big endian host, but a comment would be nice.
+/**
- Board-specific NAND initialization
- @param nand nand chip info structure
- @return 0, after initialized, -1 on error
- */
+int board_nand_init(struct nand_chip *nand)
Please consider using CONFIG_SYS_NAND_SELF_INIT.
So far I don't see the demand.
I'd like to see the old way go away eventually.
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
Please try to get rid of this. This is a public mailing list.
-Scott
On 07/05/2012 07:28 PM, Scott Wood wrote:
On 07/04/2012 02:46 AM, Jim Lin wrote:
-----Original Message----- From: Scott Wood [mailto:scottwood@freescale.com] Sent: Thursday, April 26, 2012 6:17 AM To: Simon Glass Cc: U-Boot Mailing List; Tom Warren; Stephen Warren; Jim Lin; Stephen Warren Subject: Re: [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver
On 04/17/2012 01:50 PM, Simon Glass wrote:
+static void write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) +{
- int i, j, l;
- struct nand_chip *chip = mtd->priv;
- struct nand_drv *info;
- info = (struct nand_drv *)chip->priv;
- for (i = 0; i < len / 4; i++) {
l = ((int *)buf)[i];If you're assuming the buffer is 32-bit aligned, comment it. Ideally these assumptions should be stated in the interface itself...
This doesn't mean that buf needs to be 32-bit aligned. It only says each write is 32-bit.
OK, didn't realize modern ARM could deal with unaligned accesses.
I'd like to see some more research here; I know that recent printk changes in the ARM kernel caused unaligned accesses which in turn caused significant problems (including on Tegra). This might only have been for 64-bit accesses, so perhaps 32-bit accesses don't have an issue, but if you could confirm this Jim, that'd be great.
participants (2)
-
Scott Wood -
Stephen Warren