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

[Michal Simek]

Stephan Linz wrote:
> Am Sonntag, den 27.11.2011, 20:09 +0100 schrieb Wolfgang Denk: 
>> Dear Stephan Linz,
>>
>> In message <1322419397-26326-2-git-send-email-linz at 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 at 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?

It was necessary before this patch.

phylib: reset mii bus only if reset handler is registered
sha1: e3a77218a256edbe201112a39beeed8adcabae3f

Currently you can simple remove.

Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian