[U-Boot] [PATCH 4/4] net: Add LL TEMAC driver to u-boot

[Michal Simek <monstr@monstr.eu>]

Dear Wolfgang,

Wolfgang Denk wrote:
> Dear Michal Simek,
> 
> sorry for the long delay.  We're still lacking a (new) network
> custodian...
> 
> In message <1280753377-2894-4-git-send-email-monstr@monstr.eu> you wrote:
>> Add Xilinx LL Temac driver to u-boot.
>>
>> Signed-off-by: Michal Simek <monstr@monstr.eu>
>> +++ b/drivers/net/xilinx_ll_temac.c
>> @@ -0,0 +1,561 @@
>> +/*
>> + *
> 
> Drop blank line.

done

> 
>> + * Copyright (C) 2008 - 2009 Michal Simek <monstr@monstr.eu>
>> + * June 2008 Microblaze optimalization, FIFO mode support
> 
> 2009? Not 2010 ?

done

> 
> 
>> +/* XPS_LL_TEMAC direct registers definition */
>> +#define TEMAC_RAF0		(dev->iobase + 0x00)
>> +#define TEMAC_TPF0		(dev->iobase + 0x04)
>> +#define TEMAC_IFGP0		(dev->iobase + 0x08)
>> +#define TEMAC_IS0		(dev->iobase + 0x0c)
>> +#define TEMAC_IP0		(dev->iobase + 0x10)
>> +#define TEMAC_IE0		(dev->iobase + 0x14)
>> +
>> +#define TEMAC_MSW0		(dev->iobase + 0x20)
>> +#define TEMAC_LSW0		(dev->iobase + 0x24)
>> +#define TEMAC_CTL0		(dev->iobase + 0x28)
>> +#define TEMAC_RDY0		(dev->iobase + 0x2c)
> 
> Please use a C struct to describe the register layout.

not fixed.

> 
>> +/* XPS_LL_TEMAC indirect registers offset definition */
>> +
>> +#define RCW0	0x200

not used

>> +#define RCW1	0x240
>> +#define TC	0x280
>> +#define FCC	0x2c0

not used

>> +#define EMMC	0x300
>> +#define PHYC	0x320

not used

>> +#define MC	0x340
>> +#define UAW0	0x380
>> +#define UAW1	0x384
>> +#define MAW0	0x388

not used

>> +#define MAW1	0x38c

not used

>> +#define AFM	0x390
>> +#define TIS	0x3a0

not used

>> +#define TIE	0x3a4

not used

>> +#define MIIMWD	0x3b0
>> +#define MIIMAI	0x3b4
> 
> Ditto.

They are offset for indirect accesses that's why should be just values.

> 
> 
>> +	out_be32((u32 *)TEMAC_LSW0, phy_data);
>> +	out_be32((u32 *)TEMAC_CTL0, CNTLREG_WRITE_ENABLE_MASK | MIIMWD);
>> +	out_be32((u32 *)TEMAC_LSW0, (phy_addr << 5) | (reg_addr));
>> +	out_be32((u32 *)TEMAC_CTL0, \
>> +			CNTLREG_WRITE_ENABLE_MASK | MIIMAI | (emac << 10));
>> +	while (!(in_be32((u32 *)TEMAC_RDY0) & XTE_RSE_MIIM_WR_MASK))
> 
> The need to have all these casts should ring some alarm bell to you.
> Please use a proper C struct instead.

sure.

> 
>> +	out_be32((u32 *)TEMAC_LSW0, reg_data);
>> +	out_be32((u32 *)TEMAC_CTL0, \
>> +			CNTLREG_WRITE_ENABLE_MASK | (emac << 10) | reg_offset);
> 
> Drop the backslash.

done

> 
>> +		debug ("%d: 0x%x ", j, result);
>> +	}
>> +	debug ("\n");
> 
> No spaces between function name and '('. Please fix globally.
> Consider running your patch through checkpatch...

done

> 
>> +	while (retries-- &&
>> +		((xps_ll_temac_hostif_get(dev, 0, phy_addr, 1) & 0x24) == 0x24))
>> +		;
> 
> Bad indentation.

done.

> 
> 
>> +	if (i == 0x7c0a3) {
> ...
>> +	if (i == 0x1410cc2) {
> 
> Please use self-explaining symbolic names for these magic numbers,
> and/or add sufficient comments what these mean.

There is option which I prefer which is phy lib.


> 
>> +static void debugll(int count)
>> +{
>> +	printf ("%d fifo isr 0x%08x, fifo_ier 0x%08x, fifo_rdfr 0x%08x, "
>> +		"fifo_rdfo 0x%08x fifo_rlr 0x%08x\n", count, ll_fifo->isr, \
>> +		ll_fifo->ier, ll_fifo->rdfr, ll_fifo->rdfo, ll_fifo->rlf);
> 
> Drop the backslash.  Please check & fix globally.

done.

> 

I have fixed some simple issues which you reported but not everything.
(microblaze custodian tree)

I am still not convince that I even would like to push this driver 
mainline and keep responsibility for it.

The next thing is that there is missing phy lib support which will be 
good to use.

Microblaze systems are slightly moving to new AXI bus where will be 
different eth IP core which will require new u-boot driver. Not sure
if will be based on this ll_temac driver.

Stephan: If you are willing to fix issues which Wolfgang reported, I am 
happy to test them.

Best regards,
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