Dear Macpaul Lin,
In message <[email protected]> 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.
> 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.
> 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.
...
> + 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).
> +static int ftgmac100_mdiobus_reset(struct eth_device *dev)
> +{
> + return 0;
> +}
Why do we need this function?
> +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.
> +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.
> +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?
> + /* 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.
> + /* When it is unable to found PHY, the interface usually return
> 0xffff or 0x0000 */
Ditto for line too long.
> + 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".
> +#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.
> + 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?
> + /* 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.
> +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.
> + 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.
> +#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.
> --- 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: [email protected]
"Have you lived in this village all your life?" "No, not yet."
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
