From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Thu, 13 Sep 2012 02:24:38 +0200
Subject: [U-Boot] [PATCH 5/7] S3C24XX: Add NAND Flash driver
In-Reply-To: <50512667.7010806@inov.pt>
References: <1347448523-19565-1-git-send-email-jose.goncalves@inov.pt>
	<505118B8.9030609@freescale.com> <50512667.7010806@inov.pt>
Message-ID: <201209130224.38939.marex@denx.de>
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

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