[U-Boot] [PATCH v6] net: ll_temac: Add LL TEMAC driver to u-boot

[Stephan Linz <linz@li-pro.net>]

Am Sonntag, den 27.11.2011, 20:09 +0100 schrieb Wolfgang Denk: 
> Dear Stephan Linz,
> 
> In message <1322419397-26326-2-git-send-email-linz@li-pro.net> you wrote:
> > Xilinx LocalLink Tri-Mode Ether MAC driver can be
> > used by Xilinx Microblaze or Xilinx ppc405/440 in
> > SDMA and FIFO mode. DCR or XPS bus can be used.
> > 
> > The driver uses and requires MII and PHYLIB.
> > 
> > Signed-off-by: Stephan Linz <linz@li-pro.net>
> ...
> 
> > +static inline void xps_ll_temac_check_status(struct temac_reg *regs, u32 mask)
> > +{
> > +	unsigned timeout = 2000;
> > +	while (timeout && (!(in_be32(&regs->rdy) & mask)))
> > +		timeout--;
> 
> This has been asked before:  what exactly is the limit for the timeout
> here?  2000 loops though that loop is not exactly a known value.
> Please add some udelay() here (which is also good for triggering the
> watchdog, should you have one running).

Hi Wolfgang,

I'll try to make the timeout handling more precise. Your argument with
the watchdog is an interesting issue I forget completely. Thanks for the
hint.

> 
> 
> > +	if (!timeout)
> > +		printf("%s: Timeout\n", __func__);
> 
> Why is this function void when you are know that you might have to
> handle error situations (like a timeout)?
> 
> > +static void xps_ll_temac_hostif_set(struct eth_device *dev, u8 phy_addr,
> > +					u8 reg_addr, u16 phy_data)
> > +{
> > +	struct temac_reg *regs = (struct temac_reg *)dev->iobase;
> > +
> > +	out_be32(&regs->lsw, (phy_data & LSW_REGDAT_MASK));
> > +	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMWD);
> > +	out_be32(&regs->lsw,
> > +		((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
> > +		(reg_addr & LSW_REGAD_MASK));
> > +	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMAI);
> > +	xps_ll_temac_check_status(regs, RSE_MIIM_WR);
> > +}
> 
> So what happens here if we have a timeout in
> xps_ll_temac_check_status()?  SHould such an error not be handled?

Yes we should. I'll fix it.


> > + * Undirect hostif read to ll_temac.
> "Undirect"??  Or "Indirect" ?
> "read to"??  Or "read from" ?
> 
> Please fix globally.

yep.

> 
> > +#ifdef DEBUG
> > +static inline void read_phy_reg(struct eth_device *dev, u8 phy_addr)
> > +{
> > +	int j, result;
> > +	debug("phy%d ", phy_addr);
> 
> Please always have one blank line between declarations and code.
> Please fix globally.

yep.

> > +	if (ll_temac->ctrlreset)
> > +		if (ll_temac->ctrlreset(dev))
> > +			return -1;
> 
> Braces needed for multiline statements.  Please fix globally.

yep.

> 
> > +/* halt device */
> > +static void ll_temac_halt(struct eth_device *dev)
> > +{
> > +#ifdef ETH_HALTING
> > +	struct ll_temac *ll_temac = dev->priv;
> > +
> > +	/* Disable Receiver */
> > +	xps_ll_temac_indirect_set(dev, TEMAC_RCW0, 0);
> > +
> > +	/* Disable Transmitter */
> > +	xps_ll_temac_indirect_set(dev, TEMAC_TC, 0);
> > +
> > +	if (ll_temac->ctrlhalt)
> > +		ll_temac->ctrlhalt(dev);
> > +#endif
> > +}
> 
> ETH_HALTING is permanently undef'ed, so all this is dead code.  Please
> remove.

@Michal: Is there any platform which can not halt the TEMAC? I have no
problem with this code but I left ETH_HALTING undefined from original
driver code. I would like to remove all the ETH_HALTING statements and
hold this code.

> 
> > +static int ll_temac_bus_reset(struct mii_dev *bus)
> > +{
> > +	debug("Just bus reset\n");
> > +	return 0;
> > +}
> 
> Can we remove this function?  It does not appear to perform any useful
> operation?

@Michal: Have you any objections?

@all: What is the meaning of mii_dev bus reset? What is expacted from
the perspective of the MII driver code?

> 
> > + * FIXME: This part should going up to arch/powerpc -- but where?
> 
> s/going/go/ ?
> s/rrr/rr/ ?

yep.

> 
> > +int ll_temac_halt_sdma(struct eth_device *dev)
> > +{
> > +	unsigned timeout = 2000;
> 
> See comments above.
> 
> > +	if (!timeout) {
> > +		printf("%s: Timeout\n", __func__);
> > +		return 1;
> > +	}
> 
> Interesting - here you return an error code, above you don't :-(

Grr, ache on my head. I need one more look to the timeouts (see above).


> Same issues again.  Please fix timout and error handling globally.
> 
> > +
> > +#if defined(CONFIG_XILINX_440) || defined(CONFIG_XILINX_405)
> > +/* Check for TX and RX channel errrors. */
> > +static inline int ll_temac_dmac_error(struct eth_device *dev)
> > +{
> > +	int err;
> > +--snip--
> 
> Umm.... this code is more or less exactly the same as for
> ll_temac_recv_sdma().  Can we please avoid this duplication of code?
> 
> Ditto for the other duplicated functions.

You are completely right, but for more than one week I have not found
any practicable way to avoid duplicate code here. The only really good
approach would be to use private function pointers in struct ll_temac
for all the in_*/out_* calls we need in sdma code and refactor the DCR
code to use its special in_*/out_* functions with the same prototype.
This special 'DCR' function can map into indirect DCR access. But
unfortunately in_be32() and out_be32() for Microblaze are not real
functions. They are CPP defines :-(

I will review arch/microblaze/include/asm/io.h together with Michal.
Until then it would be nice if we could keep this code as it is. I will
fix the dublicate code if we have in/out functions -- no defines.


-- 
Viele Gr??e,
Stephan Linz
______________________________________________________________________________
MB-Ref: http://www.li-pro.de/xilinx_mb:mbref:start
OpenDCC: http://www.li-pro.net/opendcc.phtml
PC/M: http://www.li-pro.net/pcm.phtml
Sourceforge: http://sourceforge.net/users/slz
Gitorious: https://gitorious.org/~slz