From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrian Hunter Subject: Re: [PATCH] ARM: OMAP: OneNAND for OMAP3 Date: Mon, 04 Aug 2008 09:56:50 +0300 Message-ID: <4896A832.3080402@nokia.com> References: <4892C533.50904@nokia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.nokia.com ([192.100.122.230]:45885 "EHLO mgw-mx03.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757758AbYHDG4B (ORCPT ); Mon, 4 Aug 2008 02:56:01 -0400 In-Reply-To: Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Paul Walmsley Cc: linux-omap@vger.kernel.org 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 >