[U-Boot] [PATCH] ftgmac100: support of gigabit eth ftgmac100
Macpaul Lin
macpaul at gmail.com
Mon Dec 13 03:13:30 CET 2010
Dear Wolfgang,
Please take a look at the similar driver "ftmac100.c",
Some coding patterns in "ftgmac100.c" are followed as "ftmac100.c".
Since the ftgmac100 has the similar hardware logic inside as the same
as ftmac100.
The code of ftgmac100.c is modified from ftmac100.c
So I think you've permit that kind of coding style and related issues,
this code of ftgmac100 shouldn't be a problem. ;p
2010/12/13 Wolfgang Denk <wd at denx.de>:
> Dear Macpaul Lin,
>
> In message <1291969229-3884-1-git-send-email-macpaul at andestech.com> you wrote:
>> Add Faraday's ftgmac100 (gigabit ethernet)
>> MAC controller's driver.
>>
>> Sub configuration in this driver:
>>
>> CONFIG_FTGMAC100_EGIGA:
>> Support GE link update with gigabit PHY.
>
> This needs to be documented in the README.
>
Okay, next patch will trying to add correct format of this statement
into README.
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index b605eea..faeab32 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -48,6 +48,7 @@ COBJS-$(CONFIG_ETHOC) += ethoc.o
>> COBJS-$(CONFIG_FEC_MXC) += fec_mxc.o
>> COBJS-$(CONFIG_FSLDMAFEC) += fsl_mcdmafec.o mcfmii.o
>> COBJS-$(CONFIG_FTMAC100) += ftmac100.o
>> +COBJS-$(CONFIG_FTGMAC100) += ftgmac100.o
>> COBJS-$(CONFIG_GRETH) += greth.o
>> COBJS-$(CONFIG_INCA_IP_SWITCH) += inca-ip_sw.o
>> COBJS-$(CONFIG_DRIVER_KS8695ETH) += ks8695eth.o
>
> Please keep list sorted.
>
OK. g comes before m. ;p
>> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c
>> new file mode 100644
>> index 0000000..7616fcd
>> --- /dev/null
>> +++ b/drivers/net/ftgmac100.c
>> @@ -0,0 +1,598 @@
> ...
>
>> +struct ftgmac100_data {
>> + volatile struct ftgmac100_txdes txdes[PKTBUFSTX]; /* must be power of 2 */
>> + volatile struct ftgmac100_rxdes rxdes[PKTBUFSRX]; /* must be power of 2 */
>
> Please re-read Documentation/volatile-considered-harmful.txt and then
> explain why this volatile here is needed.
>
This code was copied originate from ftmac100.c
> ...
>> + for (i = 0; i < 10; i++) {
>> + phycr = readl(&ftgmac100->phycr);
>> +
>> + if ((phycr & FTGMAC100_PHYCR_MIIWR) == 0) {
>> + debug("(phycr & FTGMAC100_PHYCR_MIIWR) == 0: phy_addr: %x\n", phy_addr);
>
> Line too long. Please fix globally (consider running checkpatch.pl).
>
Ok.
>> +static int ftgmac100_mdiobus_reset(struct eth_device *dev)
>> +{
>> + return 0;
>> +}
>
> Why do we need this function?
Okay, I will take it off. It comes from Linux driver "ftgmac100.c"
>
>> +int ftgmac100_phy_read(struct eth_device *dev, int addr,
>> + int reg, u16 *value)
>> +{
>> + *value = ftgmac100_mdiobus_read(dev , addr, reg);
>> + return 0;
>> +}
>
> Error handling missing.
Okay, will fix it.
>> +int ftgmac100_phy_write(struct eth_device *dev, int addr,
>> + int reg, u16 value)
>> +{
>> + ftgmac100_mdiobus_write(dev, addr, reg, value);
>> + return 0;
>> +}
>
> Error handling missing. Please check and fix globally.
Okay, will fix it.
>
>> +static int ftgmac100_phy_reset(struct eth_device *dev)
>> +{
> ...
>> + for (i = 0; i < 100000 / 100; i++) {
>> + ftgmac100_phy_read(dev, priv->phy_addr,
>> + MII_BMSR, &status);
>> + if (status & BMSR_ANEGCOMPLETE)
>> + break;
>> + udelay(100);
>
> Are you sure that autonegotiation will always complete within 100
> milliseconds or less?
>
On the board I have was running at 200MHz ~ 500Mhz, it is usaully done.
What value will you suggest? 1000 millisecond?
>> + /* Check if the PHY is up to snuff... */
>> +
>> + for (phy_addr=0; phy_addr < CONFIG_PHY_MAX_ADDR; phy_addr++) {
>> + ftgmac100_phy_read(dev, phy_addr,
>> + MII_PHYSID1, &phy_id);
>
> See note above about error handling. You need to reqork all your
> code.
Okay.
>
>> + /* When it is unable to found PHY, the interface usually return 0xffff or 0x0000 */
>
> Ditto for line too long.
>
Okay.
>
>> + for (i = 0; i < 100000 / 100; i++) {
>> + ftgmac100_phy_read(dev, priv->phy_addr,
>> + MII_BMSR, &status);
>> + if (status & BMSR_LSTATUS)
>> + break;
>> + udelay(100);
>
> See note above - are 100 milliseconds always sufficient?
>
>> + }
>> + if (!(status & BMSR_LSTATUS)) {
>> + printf("%s: link down\n", dev->name);
>> + return 3;
>> + } else {
>
> No "else" needed after a "return".
Ah, thanks.
>
>> +#ifdef CONFIG_FTGMAC100_EGIGA
>> + ftgmac100_phy_read(dev, priv->phy_addr,
>> + MII_STAT1000, &stat_ge); /* 1000 Base-T Status Register */
>> +
>> + speed = (stat_ge & (LPA_1000FULL | LPA_1000HALF)
>> + ? 1 : 0);
>> +
>> + duplex = ((stat_ge & LPA_1000FULL)
>> + ? 1 : 0);
>> +
>> + if (speed) { /* Speed is 1000 */
>> + printf("%s: link up, %sMbps %s-duplex\n",
>> + dev->name,
>> + speed ? "1000" : "10/100",
>
> This makes no sense. You tested "speed" 4 lines above, so we know
> it's always true.
>
Oh thanks, the speed test of gigabit part will be fixed.
>> + duplex ? "full" : "half");
>> + return 0;
>> + }
>> +#endif
>> +
>> + ftgmac100_phy_read(dev, priv->phy_addr,
>> + MII_ADVERTISE, &adv);
>> + ftgmac100_phy_read(dev, priv->phy_addr,
>> + MII_LPA, &lpa);
>> + media = mii_nway_result(lpa & adv);
>> + speed = (media & (ADVERTISE_100FULL | ADVERTISE_100HALF)
>> + ? 1 : 0);
>> + duplex = (media & ADVERTISE_FULL) ? 1 : 0;
>> + printf("%s: link up, %sMbps %s-duplex\n",
>> + dev->name,
>> + speed ? "100" : "10",
>> + duplex ? "full" : "half");
>> + }
>> + return 0;
>> +}
>> +
>> +int ftgmac100_update_link_speed(struct eth_device *dev)
>> +{
>> + struct ftgmac100 *ftgmac100 = (struct ftgmac100 *)dev->iobase;
>> + struct ftgmac100_data *priv = dev->priv;
>> +
>> + unsigned short stat_fe;
>> + unsigned short stat_ge;
>> +
>> +#ifdef CONFIG_FTGMAC100_EGIGA
>> + ftgmac100_phy_read(dev, priv->phy_addr,
>> + MII_STAT1000, &stat_ge); /* 1000 Base-T Status Register */
>> +#endif
>> +
>> + ftgmac100_phy_read(dev, priv->phy_addr,
>> + MII_BMSR, &stat_fe);
>> +
>> + if (!(stat_fe & BMSR_LSTATUS)) /* link status up? */
>> + return 1;
>> +
>> +#ifdef CONFIG_FTGMAC100_EGIGA
>> + if (stat_ge & LPA_1000FULL) {
>> + /*set Emac for 1000BaseTX and Full Duplex */
>> + writel((readl(&ftgmac100->maccr) &
>> + ~(FTGMAC100_MACCR_GIGA_MODE |
>> + FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_FULLDUP)
>> + ) | FTGMAC100_MACCR_GIGA_MODE | FTGMAC100_MACCR_FULLDUP,
>> + &ftgmac100->maccr);
>> + return 0;
>> + }
>> +
>> + if (stat_ge & LPA_1000HALF) {
>> + /*set Emac for 1000BaseTX and Half Duplex */
>> + writel((readl(&ftgmac100->maccr) &
>> + ~(FTGMAC100_MACCR_GIGA_MODE |
>> + FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_FULLDUP)
>> + ) | FTGMAC100_MACCR_GIGA_MODE,
>> + &ftgmac100->maccr);
>> + return 0;
>> + }
>> +#endif
>> +
>> + if (stat_fe & BMSR_100FULL) {
>> + /*set Emac for 100BaseTX and Full Duplex */
>> + writel((readl(&ftgmac100->maccr) &
>> + ~(FTGMAC100_MACCR_GIGA_MODE |
>> + FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_FULLDUP)
>> + ) | FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_FULLDUP,
>> + &ftgmac100->maccr);
>> + return 0;
>> + }
>> +
>> + if (stat_fe & BMSR_10FULL) {
>> + /*set MII for 10BaseT and Full Duplex */
>> + writel((readl(&ftgmac100->maccr) &
>> + ~(FTGMAC100_MACCR_GIGA_MODE |
>> + FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_FULLDUP)
>> + ) | FTGMAC100_MACCR_FULLDUP,
>> + &ftgmac100->maccr);
>> + return 0;
>> + }
>> +
>> + if (stat_fe & BMSR_100HALF) {
>> + /*set MII for 100BaseTX and Half Duplex */
>> + writel((readl(&ftgmac100->maccr) &
>> + ~(FTGMAC100_MACCR_GIGA_MODE |
>> + FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_FULLDUP)
>> + ) | FTGMAC100_MACCR_FAST_MODE,
>> + &ftgmac100->maccr);
>> + return 0;
>> + }
>> +
>> + if (stat_fe & BMSR_10HALF) {
>> + /*set MII for 10BaseT and Half Duplex */
>> + writel((readl(&ftgmac100->maccr) &
>> + ~(FTGMAC100_MACCR_GIGA_MODE |
>> + FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_FULLDUP)),
>> + &ftgmac100->maccr);
>> + return 0;
>
> Instead of repeating this code fragment again and again, can you not
> make the code data driven, i. e. use a table to look up the mask?
I found at91_emac.c accepts this kind of code. So I used this pattern
to do link update detection.
I will try to think another method to do so.
>> + /* pass the packet up to the protocol layers. */
>> +
>> + NetReceive ((void *)curr_des->rxdes3, rxlen);
>> +
>> + /* release buffer to DMA */
>> +
>> + curr_des->rxdes0 &= ~FTGMAC100_RXDES0_RXPKT_RDY;
>
> Please remove the blank lines between these on-line comments and the
> code. Please do globally.
Okay, but please check ftmac100.c
I just followed the coding style I guess which should be permit on last time.
Will to cleanup on next patch.
>> +ftgmac100_send (struct eth_device *dev, volatile void *packet, int length)
>> +{
>> + struct ftgmac100 *ftgmac100 = (struct ftgmac100 *)dev->iobase;
>> + struct ftgmac100_data *priv = dev->priv;
>> + volatile struct ftgmac100_txdes *curr_des = &priv->txdes[priv->tx_index];
>
> Please explain why this volatile is needed.
>
Please check ftmac100.c
>> + tmo = get_timer (0) + 5 * CONFIG_SYS_HZ;
>> + while (curr_des->txdes0 & FTGMAC100_TXDES0_TXDMA_OWN) {
>> + if (get_timer (0) >= tmo) {
>
> This is bound to fail when the timer wraps around.
It is a tx timeout detection necessary to monitor if hardware behaves
correctly.
Hardware should clear FTGMAC100_TXDES0_TXDMA_OWN bit before driver
here to examine the bit is cleared or not. If hardware didn't clear
this bit, it might indicate hardware has problem will reset is needed.
>> +#define FTGMAC100_OFFSET_ISR 0x00
>> +#define FTGMAC100_OFFSET_IER 0x04
>> +#define FTGMAC100_OFFSET_MAC_MADR 0x08
>> +#define FTGMAC100_OFFSET_MAC_LADR 0x0c
>> +#define FTGMAC100_OFFSET_MAHT0 0x10
>> +#define FTGMAC100_OFFSET_MAHT1 0x14
>> +#define FTGMAC100_OFFSET_NPTXPD 0x18
>> +#define FTGMAC100_OFFSET_RXPD 0x1c
>> +#define FTGMAC100_OFFSET_NPTXR_BADR 0x20
>> +#define FTGMAC100_OFFSET_RXR_BADR 0x24
>> +#define FTGMAC100_OFFSET_HPTXPD 0x28
>> +#define FTGMAC100_OFFSET_HPTXR_BADR 0x2c
> ...
>
> Ouch.
>
>> +struct ftgmac100 {
>> + unsigned int isr; /* 0x00 */
>> + unsigned int ier; /* 0x04 */
>> + unsigned int mac_madr; /* 0x08 */
>> + unsigned int mac_ladr; /* 0x0c */
>> + unsigned int maht0; /* 0x10 */
>> + unsigned int maht1; /* 0x14 */
>> + unsigned int txpd; /* 0x18 */
>> + unsigned int rxpd; /* 0x1c */
>> + unsigned int txr_badr; /* 0x20 */
>> + unsigned int rxr_badr; /* 0x24 */
>> + unsigned int hptxpd; /* 0x28 */
>> + unsigned int hptxpd_badr; /* 0x2c */
>
> Um.... We don't need both offset tables and C structs, do we?
>
> Please dump the offset table.
This is all the offset table here. Start from 0x0 to 0xc8.
The offset table here and the following structure are identical.
However, I'm afraid of that some customer will need to access offset
table with assembly in lowlevel_init.S.
So I just keep a copy here.
>
>> --- a/include/netdev.h
>> +++ b/include/netdev.h
>> @@ -62,6 +62,7 @@ int eth_3com_initialize (bd_t * bis);
>> int fec_initialize (bd_t *bis);
>> int fecmxc_initialize (bd_t *bis);
>> int ftmac100_initialize(bd_t *bits);
>> +int ftgmac100_initialize(bd_t *bits);
>> int greth_initialize(bd_t *bis);
>> void gt6426x_eth_initialize(bd_t *bis);
>> int inca_switch_initialize(bd_t *bis);
>
> Please keep list sorted.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> "Have you lived in this village all your life?" "No, not yet."
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
--
Best regards,
Macpaul Lin
More information about the U-Boot
mailing list