[U-Boot] [PATCH] TI: DaVinci: Updating EMAC driver for DM365, DM646x and DA8XX

[Nick Thompson <nick.thompson@ge.com>]

On 16/12/09 22:00, Wolfgang Denk wrote:
> Dear Nick Thompson,
> 
> In message <4B2770F8.5090607@ge.com> you wrote:
>> The EMAC IP on DM365, DM646x and DA830 is slightly different
>> from that on DM644x. This change updates the DaVinci EMAC driver
>> so that EMAC becomes operational on SOCs with EMAC v2.
>>
>> Signed-off-by: Nick Thompson <nick.thompson@ge.com>
>> Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
>> ---
>> Applies to: u-boot-ti
>>
>> This is a combined patch with Sandeep's DM365 and DM646x changes
>> and additional changes for DA830. It replaces previous submissions
>> for EMAC support on these devices.
>>
>>  drivers/net/davinci_emac.c               |  131 ++++++++++++++++++++++++-----
>>  include/asm-arm/arch-davinci/emac_defs.h |   60 +++++++++++++-
>>  2 files changed, 164 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
>> index fa8cee4..dbf94d2 100644
>> --- a/drivers/net/davinci_emac.c
>> +++ b/drivers/net/davinci_emac.c
>> @@ -42,6 +42,7 @@
>>  #include <miiphy.h>
>>  #include <malloc.h>
>>  #include <asm/arch/emac_defs.h>
>> +#include <asm/io.h>
>>  
>>  unsigned int	emac_dbg = 0;
>>  #define debug_emac(fmt,args...)	if (emac_dbg) printf(fmt,##args)
>> @@ -107,6 +108,35 @@ static void davinci_eth_mdio_enable(void)
>>  	while (adap_mdio->CONTROL & MDIO_CONTROL_IDLE) {;}
> 
> Please fix this as well while we are here. Please make this:
> 
> 	while (adap_mdio->CONTROL & MDIO_CONTROL_IDLE)
> 		;

Yes, will do, here and elsewhere in the file. I will also change
all these cases to use readl().

> 	
> 
>> +	/* Wait for command to complete */
>> +	while (readl(&adap_mdio->USERACCESS0) & MDIO_USERACCESS0_GO);
> 
> Please make this:
> 
> 	while (readl(&adap_mdio->USERACCESS0) & MDIO_USERACCESS0_GO)
> 		;

Done.

> 
> 
>> +static void emac_gigabit_enable(void)
>> +{
>> +#ifdef DAVINCI_EMAC_GIG_ENABLE
>> +	int temp
>> +
>> +	if (mdio_read(EMAC_MDIO_PHY_NUM, 0) & (1 << 6)) {
>> +		/*
>> +		 * Check if link detected is giga-bit
>> +		 * If Gigabit mode detected, enable gigbit in MAC and PHY
>> +		 */
>> +		writel(EMAC_MACCONTROL_GIGFORCE |
>> +		       EMAC_MACCONTROL_GIGABIT_ENABLE,
>> +		       &adap_emac->MACCONTROL);
>> +
>> +		/*
>> +		 * The SYS_CLK which feeds the SOC for giga-bit operation
>> +		 * does not seem to be enabled after reset as expected.
>> +		 * Force enabling SYS_CLK by writing to the PHY
>> +		 */
>> +		temp = mdio_read(EMAC_MDIO_PHY_NUM, 22);
>> +		temp |= (1 << 4);
>> +		mdio_write(EMAC_MDIO_PHY_NUM, 22, temp);
>> +	}
>> +#endif
>> +}
> 
> Can we - instead of providing an empty function when
> DAVINCI_EMAC_GIG_ENABLE is not set - either omit this function
> completely, or use a weak implementation instead?

I don't want to use weak as it implies the function maybe replaced. This is
not the intention here. To avoid ifdefs all over the place I have added:

#ifdef DAVINCI_EMAC_GIG_ENABLE
#define mdio_gigabit_detect(phy)	(mdio_read(phy, 0) & (1 << 6))
#else
#define mdio_gigabit_detect(phy)	0
#endif

and changed the if in the above function to:

	if (mdio_gigabit_detect(EMAC_MDIO_PHY_NUM)) {
		int temp;

		...

The function is always present, but optimised away if there is a 0 method for
detecting gigabit in the phy. Is that acceptable?

> 
>>  	if (!phy.get_link_speed(active_phy_addr))
>>  		return(0);
>> +	else
>> +		emac_gigabit_enable();
> 
> No "else" is needed here. Remove it, and un-indent the
> emac_gigabit_enable() call.

Ahh yes - removed in all three cases.

I will submit a new patch tomorrow.

Thanks,
Nick.