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

> 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

I just noticed that all those uart->something in your serial driver are actually 
register accesses. So that's flat wrong, use writel()/readl() etc accessors. Of 
course doing it like you do without memory barriers will make it go south.

> 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.

WFM, thanks.

> Best regards,
> Jos? Gon?alves

Ccing Gabriel, can you look at those patches ?

Best regards,
Marek Vasut