Re: [PATCH] ARM: OMAP: OneNAND for OMAP3

[Adrian Hunter <ext-adrian.hunter@nokia.com>]

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
>