On 16/12/09 22:00, Wolfgang Denk wrote:
> Dear Nick Thompson,
> 
> In message <4b2770f8.5090...@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.thomp...@ge.com>
>> Signed-off-by: Sandeep Paulraj <s-paul...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to