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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
