Paul Walmsley wrote:
Hello Adrian,
On Fri, 1 Aug 2008, Adrian Hunter wrote:
Update OneNAND support for OMAP3.
a few quick comments.
Thanks for looking at the code.
+ reg =
omap2_onenand_readw(onenand_base+ONENAND_REG_VERSION_ID);
Just a minor nit - please use spaces around binary & ternary operators per
CodingStyle.
+ (sync_write?GPMC_CONFIG1_WRITEMULTIPLE_SUPP:0) |
+ (sync_write?GPMC_CONFIG1_WRITETYPE_SYNC:0) |
as above.
diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
index ba83900..378ee17 100644
--- a/drivers/mtd/onenand/omap2.c
+++ b/drivers/mtd/onenand/omap2.c
@@ -223,6 +227,155 @@ static inline int omap2_onenand_bufferram_offset(struct
mtd_info *mtd, int area)
return 0;
}
+#if defined(CONFIG_ARCH_OMAP3)
+
+static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
+ unsigned char *buffer, int offset,
+ size_t count)
+{
+ struct omap2_onenand *info = container_of(mtd, struct omap2_onenand,
mtd);
+ struct onenand_chip *this = mtd->priv;
+ dma_addr_t dma_src, dma_dst;
+ int bram_offset;
+ unsigned long timeout;
+ void *buf = (void *)buffer;
+ size_t xtra;
+ volatile unsigned *done;
The way this volatile is used doesn't look right...
Well, it is correct.
+ INIT_COMPLETION(info->dma_done);
+ omap_start_dma(info->dma_channel);
+
+ timeout = jiffies + msecs_to_jiffies(20);
+ done = &info->dma_done.done;
So the volatile here appears to apply to the address of 'done', but this
address does not change, correct? Only the value of 'done' itself
changes.
No, the volatile applies to the "unsigned" not the "*".
+ while (time_before(jiffies, timeout))
+ if (*done)
+ break;
Can this be replaced with wait_for_completion_timeout() or something
similar?
No. Performance testing showed that a context switch here is too expensive.
It is better to spin.
+ dma_unmap_single(&info->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
+
+ if (!*done) {
+ dev_err(&info->pdev->dev, "timeout waiting for DMA\n");
+ goto out_copy;
+ }
...
+static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
+ const unsigned char *buffer, int
offset,
+ size_t count)
+{
+ struct omap2_onenand *info = container_of(mtd, struct omap2_onenand,
mtd);
+ struct onenand_chip *this = mtd->priv;
+ dma_addr_t dma_src, dma_dst;
+ int bram_offset;
+ unsigned long timeout;
+ void *buf = (void *)buffer;
+ volatile unsigned *done;
Same comments in this function per volatile.
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
