[U-Boot] [PATCH 2/4 v2] net: Add driver for Zynq Gem IP

[Marek Vasut]

Dear Joe Hershberger,

> On Thu, Sep 13, 2012 at 11:45 PM, Marek Vasut <marex at denx.de> wrote:
> > Dear Joe Hershberger,
> > 
> >> Hi Marek,
> >> 
> >> On Thu, Sep 13, 2012 at 4:28 AM, Marek Vasut <marex at denx.de> wrote:
> >> > Dear Michal Simek,
> >> > 
> >> >> +     /*
> >> >> +      * Following is the setup for Network Control register.
> >> >> +      * Bit 2:  Set to enable Receive operation.
> >> >> +      * Bit 3:  Set to enable Transmitt operation.
> >> >> +      * Bit 4:  Set to enable MDIO operation.
> >> >> +      */
> >> >> +     tmp = readl(&regs->nwctrl);
> >> >> +     /* MDIO, Rx and Tx enable */
> >> >> +     tmp |= ZYNQ_GEM_NWCTRL_MDEN_MASK | ZYNQ_GEM_NWCTRL_RXEN_MASK |
> >> >> +         ZYNQ_GEM_NWCTRL_TXEN_MASK;
> >> >> +     writel(tmp, &regs->nwctrl);
> >> > 
> >> > setbits_le32()
> >> 
> >> This is not equivalent.  Using setbits_le32() will not provide a dmb()
> >> on the operations the way that readl(), writel() does.  I believe this
> >> will cause problems when the dcache is enabled, right?
> > 
> > Not when dcache is enabled, the register space isn't cached. But the
> > compiler can run some wild optimizations across that. So where's the
> > problem, do we add dmb() to clrsetbits() calls ?
> 
> I'm not grokking this paragraph.

The only thing that is ever cached is DRAM. It's the only cacheable area 
configured in the MMU tables. The rest is uncached.

And the dmb() doesn't have anything to do with caches, it has to do with 
compiler reordering the register accesses, which might screw things up.

But wait, I shouldn't reply to emails when I've been awake for 20+ hours. Look 
at the macro again. Let's see an example:

clrbits_le32 expands to clrbits(le32, ...)
clrbits(le32, ...) expands to out_le32(..., in_le32(...) & ~...)

out_le32() expands to out_arch() which expands to __raw_writel()
in_le32() expands to in_arch() which expands to __raw_readl()

__raw_writel() expands to __arch_putl()
__raw_readl() expands to __arch_getl()

arch_putl() is *(volatile unsigned long *), therefore compiler reordering 
doesn't happen accross this (because of the volatile). So is arch_getl() .

Bottom line, no need for explicit dmb() in clrsetbits_le32() and friends. But 
whew, you got me scared there ;-)

> You are saying that when the dcache is enabled, that the register
> space is not cached?  Why would it not be?  It doesn't know those
> addresses from any other memory, right?
> 
> clrsetbits() does not call dmb().  Perhaps it should.  It's never been
> clear to me why writeX/readX used dmb, but out_XY/in_XY/clrsetbits_XY
> do not.  Are they not both for accessing registers?
> 
> Perhaps test all of these drivers with write-back cache enabled and
> make sure they work.
> 
> -Joe

Best regards,
Marek Vasut