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

[Marek Vasut]

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.

> Regards,
> José Gonçalves

Best regards,
Marek Vasut