Dear Macpaul Lin,
In message 1291969229-3884-1-git-send-email-macpaul@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.
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