[U-Boot] [PATCH 5/7] S3C24XX: Add NAND Flash driver

[José Miguel Gonçalves]

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