From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?ISO-8859-1?Q?Jos=E9_Miguel_Gon=E7alves?= <jose.goncalves@inov.pt>
Date: Thu, 13 Sep 2012 01:40:21 +0100
Subject: [U-Boot] [PATCH 5/7] S3C24XX: Add NAND Flash driver
In-Reply-To: <201209130224.38939.marex@denx.de>
References: <1347448523-19565-1-git-send-email-jose.goncalves@inov.pt>
	<505118B8.9030609@freescale.com> <50512667.7010806@inov.pt>
	<201209130224.38939.marex@denx.de>
Message-ID: <50512B75.9020401@inov.pt>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 09/13/2012 01:24 AM, Marek Vasut wrote:
> Dear Jos? Miguel Gon?alves,
>
>> Hi Scott,
>>
>> On 09/13/2012 12:20 AM, Scott Wood wrote:
>>> On 09/12/2012 06:16 PM, Jos? Miguel Gon?alves wrote:
>>>> Hi Marek,
>>>>
>>>> On 09/12/2012 10:11 PM, Marek Vasut wrote:
>>>>> Dear Jos? Miguel Gon?alves,
>>>>>
>>>>>> +
>>>>>> +/*
>>>>>> + * Hardware specific access to control-lines function
>>>>>> + */
>>>>>> +static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd,
>>>>>> unsigned int
>>>>>> ctrl) +{
>>>>>> +    s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>>>>>> +    struct nand_chip *this = mtd->priv;
>>>>>> +
>>>>>> +    if (ctrl & NAND_CTRL_CHANGE) {
>>>>>> +        if (ctrl & NAND_CLE)
>>>>>> +            this->IO_ADDR_W = (void __iomem *)&nand->nfcmmd;
>>>>>> +        else if (ctrl & NAND_ALE)
>>>>>> +            this->IO_ADDR_W = (void __iomem *)&nand->nfaddr;
>>>>>> +        else
>>>>>> +            this->IO_ADDR_W = (void __iomem *)&nand->nfdata;
>>>>> Do you need this cast ?
>>>> Without it gcc gives me a warning:
>>>>
>>>> s3c24xx_nand.c:90:20: warning: assignment discards `volatile' qualifier
>>>> from pointer target type [enabled by default]
>>> Why do you have volatile in your s3c24xx_nand struct?
>> I use that as a rule to memory mapping of hardware registers.
>> Without it GCC optimization sometimes do bad things, like completely
>> removing sequences of code.
> Not true unless your gcc is broken. Use proper accessors (readl()/writel()),
> they have proper barriers already.
>
>> For instance, if you need to pause in a loop until some bit of a
>> register is changed (as it's done in the serial driver) and the struct
>> were this register is mapped don't have the volatile attribute, the GCC
>> optimizer removes the loop.
> Yes, see above.
>

When I was debugging U-Boot on the MIN2416 I saw this over-optimization 
situation in the serial driver so I added the volatile attribute to all 
structs that map the SoC registers. But, after you pointed to me that 
the I/O macros have already incorporated the proper barriers, I looked 
again to the serial driver source and noticed that I forgot to use that 
macros on register accesses! I will change this and test it tomorrow 
before resubmitting the patch.

Best regards,
Jos? Gon?alves