Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
On Mon, 2006-10-23 at 10:09, Zang Roy-r61911 wrote: > On Thu, 2006-09-21 at 12:46, Jeff Garzik wrote: > > > you should have a chip structure, that contains two structs (one for > > each interface/port) > > > Jeff, > > I updated the code according to all your feedback and post it here > > http://www.spinics.net/lists/netdev/msg17120.html > > Any comment? > > Roy Jeff How about it? What's your comment? Roy - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
On Thu, 2006-09-21 at 12:46, Jeff Garzik wrote: > you should have a chip structure, that contains two structs (one for > each interface/port) > Jeff, I updated the code according to all your feedback and post it here http://www.spinics.net/lists/netdev/msg17120.html Any comment? Roy - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
On Thu, 2006-09-21 at 12:46, Jeff Garzik wrote:
> > +
> > +/* Synchronization is needed between the thread and up/down events.
> > + * Note that the PHY is accessed through the same registers for
> both
> > + * interfaces, so this can't be made interface-specific.
> > + */
> > +
> > +static DEFINE_SPINLOCK(phy_lock);
>
> you should have a chip structure, that contains two structs (one for
> each interface/port)
>
>
Do you mean you want to see this spinlock within dynamically allocated
data structure, and not as a global variable?
I am not sure :-).
All your other comments have been addressed. The following is the
updated tsi108 Ethernet port driver.
Any comment?
Signed-off-by: Alexandre Bounine<[EMAIL PROTECTED]>
Signed-off-by: Roy Zang <[EMAIL PROTECTED]>
diff --git a/drivers/net/tsi108_eth.c b/drivers/net/tsi108_eth.c
new file mode 100644
index 000..6c3146d
--- /dev/null
+++ b/drivers/net/tsi108_eth.c
@@ -0,0 +1,1709 @@
+/***
+
+ Copyright(c) 2006 Tundra Semiconductor Corporation.
+
+ This program is free software; you can redistribute it and/or modify it
+ under the terms of the GNU General Public License as published by the Free
+ Software Foundation; either version 2 of the License, or (at your option)
+ any later version.
+
+ This program is distributed in the hope that it will be useful, but WITHOUT
+ ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ more details.
+
+ You should have received a copy of the GNU General Public License along with
+ this program; if not, write to the Free Software Foundation, Inc., 59
+ Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+
+***/
+
+/* This driver is based on the driver code originally developed
+ * for the Intel IOC80314 (ForestLake) Gigabit Ethernet by
+ * [EMAIL PROTECTED] * Copyright (C) 2003 TimeSys Corporation
+ *
+ * Currently changes from original version are:
+ * - porting to Tsi108-based platform and kernel 2.6 ([EMAIL PROTECTED])
+ * - modifications to handle two ports independently and support for
+ * additional PHY devices ([EMAIL PROTECTED])
+ * - Get hardware information from platform device. ([EMAIL PROTECTED])
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "tsi108_eth.h"
+
+#define MII_READ_DELAY 1 /* max link wait time in msec */
+
+#define TSI108_RXRING_LEN 256
+
+/* NOTE: The driver currently does not support receiving packets
+ * larger than the buffer size, so don't decrease this (unless you
+ * want to add such support).
+ */
+#define TSI108_RXBUF_SIZE 1536
+
+#define TSI108_TXRING_LEN 256
+
+#define TSI108_TX_INT_FREQ64
+
+/* Check the phy status every half a second. */
+#define CHECK_PHY_INTERVAL (HZ/2)
+
+static int tsi108_init_one(struct platform_device *pdev);
+static int tsi108_ether_remove(struct platform_device *pdev);
+
+struct tsi108_prv_data {
+ void __iomem *regs;/* Base of normal regs */
+ void __iomem *phyregs; /* Base of register bank used for PHY access */
+
+ unsigned int phy; /* Index of PHY for this interface */
+ unsigned int irq_num;
+ unsigned int id;
+
+ struct timer_list timer;/* Timer that triggers the check phy function */
+ unsigned int rxtail;/* Next entry in rxring to read */
+ unsigned int rxhead;/* Next entry in rxring to give a new buffer */
+ unsigned int rxfree;/* Number of free, allocated RX buffers */
+
+ unsigned int rxpending; /* Non-zero if there are still descriptors
+* to be processed from a previous descriptor
+* interrupt condition that has been cleared */
+
+ unsigned int txtail;/* Next TX descriptor to check status on */
+ unsigned int txhead;/* Next TX descriptor to use */
+
+ /* Number of free TX descriptors. This could be calculated from
+* rxhead and rxtail if one descriptor were left unused to disambiguate
+* full and empty conditions, but it's simpler to just keep track
+* explicitly. */
+
+ unsigned int txfree;
+
+ unsigned int phy_ok;/* The PHY is currently powered on. */
+
+ /* PHY status (duplex is 1 for half, 2 for full,
+* so that the default 0 indicates that neither has
+* yet been configured). */
+
+ unsigned int link_up;
+ unsigned int speed;
+ unsigned int duplex;
+
+ tx_desc *txring;
+ rx_desc *rxring;
+ struct sk_buff *txskbs[TSI108_TXRING_LE
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
Zang Roy-r61911 wrote: Could you interpret the chip structure in more detail? Need I create two net_device struct for each port? No. One net_device per port. And one container structure for the entire device. Jeff - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
On Thu, 2006-09-21 at 12:46, Jeff Garzik wrote:
> Zang Roy-r61911 wrote:
> > +struct tsi108_prv_data {
> > + void __iomem *regs;/* Base of normal regs */
> > + void __iomem *phyregs; /* Base of register bank used for PHY
> access */
> > +
> > + int phy;/* Index of PHY for this interface */
> > + int irq_num;
> > + int id;
> > +
> > + struct timer_list timer;/* Timer that triggers the check phy
> function */
> > + int rxtail; /* Next entry in rxring to read */
> > + int rxhead; /* Next entry in rxring to give a new
> buffer */
> > + int rxfree; /* Number of free, allocated RX
> buffers */
> > +
> > + int rxpending; /* Non-zero if there are still
> descriptors
> > + * to be processed from a previous
> descriptor
> > + * interrupt condition that has been
> cleared */
> > +
> > + int txtail; /* Next TX descriptor to check status
> on */
> > + int txhead; /* Next TX descriptor to use */
>
> most of these should be unsigned, to prevent bugs.
>
>
> > + /* Number of free TX descriptors. This could be calculated
> from
> > + * rxhead and rxtail if one descriptor were left unused to
> disambiguate
> > + * full and empty conditions, but it's simpler to just keep
> track
> > + * explicitly. */
> > +
> > + int txfree;
> > +
> > + int phy_ok; /* The PHY is currently powered on. */
> > +
> > + /* PHY status (duplex is 1 for half, 2 for full,
> > + * so that the default 0 indicates that neither has
> > + * yet been configured). */
> > +
> > + int link_up;
> > + int speed;
> > + int duplex;
> > +
> > + tx_desc *txring;
> > + rx_desc *rxring;
> > + struct sk_buff *txskbs[TSI108_TXRING_LEN];
> > + struct sk_buff *rxskbs[TSI108_RXRING_LEN];
> > +
> > + dma_addr_t txdma, rxdma;
> > +
> > + /* txlock nests in misclock and phy_lock */
> > +
> > + spinlock_t txlock, misclock;
> > +
> > + /* stats is used to hold the upper bits of each hardware
> counter,
> > + * and tmpstats is used to hold the full values for returning
> > + * to the caller of get_stats(). They must be separate in
> case
> > + * an overflow interrupt occurs before the stats are consumed.
> > + */
> > +
> > + struct net_device_stats stats;
> > + struct net_device_stats tmpstats;
> > +
> > + /* These stats are kept separate in hardware, thus require
> individual
> > + * fields for handling carry. They are combined in get_stats.
> > + */
> > +
> > + unsigned long rx_fcs; /* Add to rx_frame_errors */
> > + unsigned long rx_short_fcs; /* Add to rx_frame_errors */
> > + unsigned long rx_long_fcs; /* Add to rx_frame_errors */
> > + unsigned long rx_underruns; /* Add to rx_length_errors */
> > + unsigned long rx_overruns; /* Add to rx_length_errors */
> > +
> > + unsigned long tx_coll_abort;/* Add to
> tx_aborted_errors/collisions */
> > + unsigned long tx_pause_drop;/* Add to tx_aborted_errors */
> > +
> > + unsigned long mc_hash[16];
> > +};
> > +
> > +/* Structure for a device driver */
> > +
> > +static struct platform_driver tsi_eth_driver = {
> > + .probe = tsi108_init_one,
> > + .remove = tsi108_ether_remove,
> > + .driver = {
> > + .name = "tsi-ethernet",
> > + },
> > +};
> > +
> > +static void tsi108_timed_checker(unsigned long dev_ptr);
> > +
> > +static void dump_eth_one(struct net_device *dev)
> > +{
> > + struct tsi108_prv_data *data = netdev_priv(dev);
> > +
> > + printk("Dumping %s...\n", dev->name);
> > + printk("intstat %x intmask %x phy_ok %d"
> > +" link %d speed %d duplex %d\n",
> > +TSI108_ETH_READ_REG(TSI108_EC_INTSTAT),
> > +TSI108_ETH_READ_REG(TSI108_EC_INTMASK), data->phy_ok,
> > +data->link_up, data->speed, data->duplex);
> > +
> > + printk("TX: head %d, tail %d, free %d, stat %x, estat %x, err
> %x\n",
> > +data->txhead, data->txtail, data->txfree,
> > +TSI108_ETH_READ_REG(TSI108_EC_TXSTAT),
> > +TSI108_ETH_READ_REG(TSI108_EC_TXESTAT),
> > +TSI108_ETH_READ_REG(TSI108_EC_TXERR));
> > +
> > + printk("RX: head %d, tail %d, free %d, stat %x,"
> > +" estat %x, err %x, pending %d\n\n",
> > +data->rxhead, data->rxtail, data->rxfree,
> > +TSI108_ETH_READ_REG(TSI108_EC_RXSTAT),
> > +TSI108_ETH_READ_REG(TSI108_EC_RXESTAT),
> > +TSI108_ETH_READ_REG(TSI108_EC_RXERR), data->rxpending);
> > +}
> > +
> > +/* Synchronization is needed between the thread and up/down events.
> > + * Note that the PHY is accessed through the same registers for
> both
> > + * interfaces, so this can't be made interface-specific.
> > + */
> > +
> > +static DEFINE_SPINLOCK(phy_lock)
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
Zang Roy-r61911 wrote: On Thu, 2006-09-21 at 12:26, Jeff Garzik wrote: Zang Roy-r61911 wrote: +#define TSI108_ETH_WRITE_REG(offset, val) \ + writel(le32_to_cpu(val),data->regs + (offset)) + +#define TSI108_ETH_READ_REG(offset) \ + le32_to_cpu(readl(data->regs + (offset))) + +#define TSI108_ETH_WRITE_PHYREG(offset, val) \ + writel(le32_to_cpu(val), data->phyregs + (offset)) + +#define TSI108_ETH_READ_PHYREG(offset) \ + le32_to_cpu(readl(data->phyregs + (offset))) NAK: 1) writel() and readl() are defined to be little endian. If your platform is different, then your platform should have its own foobus_writel() and foobus_readl(). Tsi108 bridge is designed for powerpc platform. Originally, I use out_be32() and in_be32(). While there is no obvious reason to object using this bridge in a little endian system. Maybe some extra hardware logic needed for the bus interface. le32_to_cpu() can be aware the endian difference. To restate, readl() should read a little endian value, and return a CPU-endian value. writel() should receive a CPU-endian value, and write a little endian value. If your platform's readl/writel doesn't do that, it's broken. That's why normal PCI drivers can use readl() and writel() on either big-endian or little-endian machines, without needing to use le32_to_cpu(). Jeff - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
On Thu, 2006-09-21 at 12:26, Jeff Garzik wrote: > Zang Roy-r61911 wrote: > > +#define TSI108_ETH_WRITE_REG(offset, val) \ > > + writel(le32_to_cpu(val),data->regs + (offset)) > > + > > +#define TSI108_ETH_READ_REG(offset) \ > > + le32_to_cpu(readl(data->regs + (offset))) > > + > > +#define TSI108_ETH_WRITE_PHYREG(offset, val) \ > > + writel(le32_to_cpu(val), data->phyregs + (offset)) > > + > > +#define TSI108_ETH_READ_PHYREG(offset) \ > > + le32_to_cpu(readl(data->phyregs + (offset))) > > > NAK: > > 1) writel() and readl() are defined to be little endian. > > If your platform is different, then your platform should have its own > foobus_writel() and foobus_readl(). Tsi108 bridge is designed for powerpc platform. Originally, I use out_be32() and in_be32(). While there is no obvious reason to object using this bridge in a little endian system. Maybe some extra hardware logic needed for the bus interface. le32_to_cpu() can be aware the endian difference. Any comment? > > 2) TSI108_ETH_WRITE_REG() is just way too long. TSI_READ(), > TSI_WRITE(), TSI_READ_PHY() and TSI_WRITE_PHY() would be far more > readable. > > More in next email. > I will modify the name. Roy - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
Zang Roy-r61911 wrote:
+struct tsi108_prv_data {
+ void __iomem *regs;/* Base of normal regs */
+ void __iomem *phyregs; /* Base of register bank used for PHY access */
+
+ int phy;/* Index of PHY for this interface */
+ int irq_num;
+ int id;
+
+ struct timer_list timer;/* Timer that triggers the check phy function */
+ int rxtail; /* Next entry in rxring to read */
+ int rxhead; /* Next entry in rxring to give a new buffer */
+ int rxfree; /* Number of free, allocated RX buffers */
+
+ int rxpending; /* Non-zero if there are still descriptors
+* to be processed from a previous descriptor
+* interrupt condition that has been cleared */
+
+ int txtail; /* Next TX descriptor to check status on */
+ int txhead; /* Next TX descriptor to use */
most of these should be unsigned, to prevent bugs.
+ /* Number of free TX descriptors. This could be calculated from
+* rxhead and rxtail if one descriptor were left unused to disambiguate
+* full and empty conditions, but it's simpler to just keep track
+* explicitly. */
+
+ int txfree;
+
+ int phy_ok; /* The PHY is currently powered on. */
+
+ /* PHY status (duplex is 1 for half, 2 for full,
+* so that the default 0 indicates that neither has
+* yet been configured). */
+
+ int link_up;
+ int speed;
+ int duplex;
+
+ tx_desc *txring;
+ rx_desc *rxring;
+ struct sk_buff *txskbs[TSI108_TXRING_LEN];
+ struct sk_buff *rxskbs[TSI108_RXRING_LEN];
+
+ dma_addr_t txdma, rxdma;
+
+ /* txlock nests in misclock and phy_lock */
+
+ spinlock_t txlock, misclock;
+
+ /* stats is used to hold the upper bits of each hardware counter,
+* and tmpstats is used to hold the full values for returning
+* to the caller of get_stats(). They must be separate in case
+* an overflow interrupt occurs before the stats are consumed.
+*/
+
+ struct net_device_stats stats;
+ struct net_device_stats tmpstats;
+
+ /* These stats are kept separate in hardware, thus require individual
+* fields for handling carry. They are combined in get_stats.
+*/
+
+ unsigned long rx_fcs; /* Add to rx_frame_errors */
+ unsigned long rx_short_fcs; /* Add to rx_frame_errors */
+ unsigned long rx_long_fcs; /* Add to rx_frame_errors */
+ unsigned long rx_underruns; /* Add to rx_length_errors */
+ unsigned long rx_overruns; /* Add to rx_length_errors */
+
+ unsigned long tx_coll_abort;/* Add to tx_aborted_errors/collisions
*/
+ unsigned long tx_pause_drop;/* Add to tx_aborted_errors */
+
+ unsigned long mc_hash[16];
+};
+
+/* Structure for a device driver */
+
+static struct platform_driver tsi_eth_driver = {
+ .probe = tsi108_init_one,
+ .remove = tsi108_ether_remove,
+ .driver = {
+ .name = "tsi-ethernet",
+ },
+};
+
+static void tsi108_timed_checker(unsigned long dev_ptr);
+
+static void dump_eth_one(struct net_device *dev)
+{
+ struct tsi108_prv_data *data = netdev_priv(dev);
+
+ printk("Dumping %s...\n", dev->name);
+ printk("intstat %x intmask %x phy_ok %d"
+ " link %d speed %d duplex %d\n",
+ TSI108_ETH_READ_REG(TSI108_EC_INTSTAT),
+ TSI108_ETH_READ_REG(TSI108_EC_INTMASK), data->phy_ok,
+ data->link_up, data->speed, data->duplex);
+
+ printk("TX: head %d, tail %d, free %d, stat %x, estat %x, err %x\n",
+ data->txhead, data->txtail, data->txfree,
+ TSI108_ETH_READ_REG(TSI108_EC_TXSTAT),
+ TSI108_ETH_READ_REG(TSI108_EC_TXESTAT),
+ TSI108_ETH_READ_REG(TSI108_EC_TXERR));
+
+ printk("RX: head %d, tail %d, free %d, stat %x,"
+ " estat %x, err %x, pending %d\n\n",
+ data->rxhead, data->rxtail, data->rxfree,
+ TSI108_ETH_READ_REG(TSI108_EC_RXSTAT),
+ TSI108_ETH_READ_REG(TSI108_EC_RXESTAT),
+ TSI108_ETH_READ_REG(TSI108_EC_RXERR), data->rxpending);
+}
+
+/* Synchronization is needed between the thread and up/down events.
+ * Note that the PHY is accessed through the same registers for both
+ * interfaces, so this can't be made interface-specific.
+ */
+
+static DEFINE_SPINLOCK(phy_lock);
you should have a chip structure, that contains two structs (one for
each interface/port)
+static u16 tsi108_read_mii(struct tsi108_prv_data *data, int reg, int *status)
+{
+ int i;
+ u16 ret;
+
+ TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_ADDR,
+ (data->phy << TSI108_MAC_MII_ADDR_PHY) |
+ (reg << TSI108_MAC_MII_
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
Zang Roy-r61911 wrote: +#define TSI108_ETH_WRITE_REG(offset, val) \ + writel(le32_to_cpu(val),data->regs + (offset)) + +#define TSI108_ETH_READ_REG(offset) \ + le32_to_cpu(readl(data->regs + (offset))) + +#define TSI108_ETH_WRITE_PHYREG(offset, val) \ + writel(le32_to_cpu(val), data->phyregs + (offset)) + +#define TSI108_ETH_READ_PHYREG(offset) \ + le32_to_cpu(readl(data->phyregs + (offset))) NAK: 1) writel() and readl() are defined to be little endian. If your platform is different, then your platform should have its own foobus_writel() and foobus_readl(). 2) TSI108_ETH_WRITE_REG() is just way too long. TSI_READ(), TSI_WRITE(), TSI_READ_PHY() and TSI_WRITE_PHY() would be far more readable. More in next email. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
On Tue, 2006-09-19 at 15:39 +0800, Zang Roy-r61911 wrote: > > > > > + spin_unlock_irq(&phy_lock); > > > + msleep(10); > > > + spin_lock_irq(&phy_lock); > > > + } > > > > hmm some places take phy_lock with disabling interrupts, while others > > don't. I sort of fear "the others" may be buggy are you sure those > > are ok? > Could you interpret your comments in detail? > Roy Hi, sorry for being unclear/too short in the review. The phy_lock lock is sometimes taken as spin_lock() and sometimes as spin_lock_irq(). It looks likes it can be used in interrupt context, in which case the spin_lock_irq() version is correct and the places where spin_lock() is used would be a deadlock bug (just think what happens if the interrupt happens while spin_lock(&phy_lock) is helt, and the spinlock then again tries to take the lock!) If there is no way this lock is used in interrupt context, then the spin_lock_irq() version is doing something which is not needed and also a bit expensive; so could be optimized. But my impression is that the _irq() is needed. Also, please consider switching from spin_lock_irq() to spin_lock_irqsave() version instead; spin_unlock_irq() has some side effects (interrupts get enabled unconditionally) so it is generally safer to use spin_lock_irqsave()/spin_unlock_irqrestore() API. If you have more questions please do not hesitate to ask! Greetings, Arjan van de Ven -- if you want to mail me at work (you don't), use arjan (at) linux.intel.com - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
> > I have some review comments about your driver; please > consider them for > fixing > Thanks. > > > + spin_unlock_irq(&phy_lock); > > + msleep(10); > > + spin_lock_irq(&phy_lock); > > + } > > hmm some places take phy_lock with disabling interrupts, while others > don't. I sort of fear "the others" may be buggy are you sure those > are ok? Could you interpret your comments in detail? Roy - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
On Tue, 2006-09-12 at 22:43, Jeff Garzik wrote:
> Roland Dreier wrote:
> > > +struct tsi108_prv_data {
> > > + volatile u32 regs; /* Base of normal regs */
> > > + volatile u32 phyregs; /* Base of register bank used for PHY
> access */
> >
> > Why volatile? This looks really wrong here.
>
> Indeed.
I will remove it.
>
>
> > > + data->regs = (u32)ioremap(einfo->regs, 0x400); /*FIX ME */
> > > + data->phyregs = (u32)ioremap(einfo->phyregs, 0x400);/*FIX
> ME */
> >
> > What needs to be fixed here? And why are you casting the result of
> > ioremap to u32? Shouldn't you keep the normal return value?
>
> Oh, that's very, very wrong.
I will find method to avoid this :-).
Roy
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
Roland Dreier wrote:
> +struct tsi108_prv_data {
> + volatile u32 regs; /* Base of normal regs */
> + volatile u32 phyregs; /* Base of register bank used for PHY access */
Why volatile? This looks really wrong here.
Indeed.
> + data->regs = (u32)ioremap(einfo->regs, 0x400);/*FIX ME */
> + data->phyregs = (u32)ioremap(einfo->phyregs, 0x400); /*FIX ME */
What needs to be fixed here? And why are you casting the result of
ioremap to u32? Shouldn't you keep the normal return value?
Oh, that's very, very wrong.
Jeff
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
> +struct tsi108_prv_data {
> +volatile u32 regs; /* Base of normal regs */
> +volatile u32 phyregs; /* Base of register bank used for PHY access */
Why volatile? This looks really wrong here.
> +data->regs = (u32)ioremap(einfo->regs, 0x400); /*FIX ME */
> +data->phyregs = (u32)ioremap(einfo->phyregs, 0x400);/*FIX ME */
What needs to be fixed here? And why are you casting the result of
ioremap to u32? Shouldn't you keep the normal return value?
- R.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support
On Tue, 2006-09-12 at 16:55 +0800, Zang Roy-r61911 wrote:
> The driver for tsi108/9 on chip Ethernet port
Hi,
I have some review comments about your driver; please consider them for
fixing
> +
> +#undef DEBUG
> +#ifdef DEBUG
> +#define DBG(fmt...) do { printk(fmt); } while(0)
> +#else
> +#define DBG(fmt...) do { } while(0)
> +#endif
please don't do this, there is pr_debug and dev_dbg() for a reason
already. No reason to add a copy.
> +
> +typedef struct net_device net_device;
> +typedef struct sk_buff sk_buff;
Please don't do this...
> + TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_ADDR,
> + (data->phy << TSI108_MAC_MII_ADDR_PHY) |
> + (reg << TSI108_MAC_MII_ADDR_REG));
> + mb();
> + TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_CMD, 0);
> + mb();
> + TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_CMD, TSI108_MAC_MII_CMD_READ);
> + mb();
what is this mb() for? do you want them to be an io memory barrier?
> + while (TSI108_ETH_READ_REG(TSI108_MAC_MII_IND) &
> +TSI108_MAC_MII_IND_BUSY) ;
do you want this to be an infinate loop? At minimum the ";" needs to be
a cpu_relax() to avoid the cpu from burning up but it'd be a lot nicer
if this loop wasn't infinite
> +
> +static irqreturn_t tsi108_irq(int irq, void *dev_id, struct pt_regs *regs)
> +{
> + if (irq == NO_IRQ)
> + return IRQ_NONE;/* Not our interrupt */
> +
> + return tsi108_irq_one(dev_id);
> +}
hi this IRQ_NONE just looks odd, was this really needed?
> + mb();
> + while (tsi108_read_mii(data, PHY_CTRL, NULL) & PHY_CTRL_AUTONEG_START)
> + ;
another infinite loop that wants cpu_relax() for sure
> + spin_unlock_irq(&phy_lock);
> + msleep(10);
> + spin_lock_irq(&phy_lock);
> + }
hmm some places take phy_lock with disabling interrupts, while others
don't. I sort of fear "the others" may be buggy are you sure those
are ok?
Greetings,
Arjan van de Ven
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 3/3] Add tsi108 On Chip Ethernet device driver support
The driver for tsi108/9 on chip Ethernet port
Signed-off-by: Alexandre Bounine<[EMAIL PROTECTED]>
Signed-off-by: Roy Zang <[EMAIL PROTECTED]>
---
drivers/net/tsi108_eth.c | 1752 ++
1 files changed, 1752 insertions(+), 0 deletions(-)
diff --git a/drivers/net/tsi108_eth.c b/drivers/net/tsi108_eth.c
new file mode 100644
index 000..eece2bb
--- /dev/null
+++ b/drivers/net/tsi108_eth.c
@@ -0,0 +1,1752 @@
+/***
+
+ Copyright(c) 2005 Tundra Semiconductor Corporation.
+
+ This program is free software; you can redistribute it and/or modify it
+ under the terms of the GNU General Public License as published by the Free
+ Software Foundation; either version 2 of the License, or (at your option)
+ any later version.
+
+ This program is distributed in the hope that it will be useful, but WITHOUT
+ ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ more details.
+
+ You should have received a copy of the GNU General Public License along with
+ this program; if not, write to the Free Software Foundation, Inc., 59
+ Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+
+***/
+
+/* This driver is based on the driver code originally developed
+ * for the Intel IOC80314 (ForestLake) Gigabit Ethernet by
+ * [EMAIL PROTECTED] * Copyright (C) 2003 TimeSys Corporation
+ *
+ * Currently changes from original version are:
+ * - portig to Tsi108-based platform and kernel 2.6 ([EMAIL PROTECTED])
+ * - modifications to handle two ports independently and support for
+ * additional PHY devices ([EMAIL PROTECTED])
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "tsi108_eth.h"
+
+#undef DEBUG
+#ifdef DEBUG
+#define DBG(fmt...) do { printk(fmt); } while(0)
+#else
+#define DBG(fmt...) do { } while(0)
+#endif
+
+typedef struct net_device net_device;
+typedef struct sk_buff sk_buff;
+
+#define MII_READ_DELAY 1 /* max link wait time in msec */
+
+#define TSI108_RXRING_LEN 256
+
+/* NOTE: The driver currently does not support receiving packets
+ * larger than the buffer size, so don't decrease this (unless you
+ * want to add such support).
+ */
+#define TSI108_RXBUF_SIZE 1536
+
+#define TSI108_TXRING_LEN 256
+
+#define TSI108_TX_INT_FREQ64
+
+/* Check the phy status every half a second. */
+#define CHECK_PHY_INTERVAL (HZ/2)
+
+static int tsi108_init_one(struct platform_device *pdev);
+static int tsi108_ether_remove(struct platform_device *pdev);
+
+struct tsi108_prv_data {
+ volatile u32 regs; /* Base of normal regs */
+ volatile u32 phyregs; /* Base of register bank used for PHY access */
+ int phy;/* Index of PHY for this interface */
+ int irq_num;
+ int id;
+
+ struct timer_list timer;/* Timer that triggers the check phy function */
+ int rxtail; /* Next entry in rxring to read */
+ int rxhead; /* Next entry in rxring to give a new buffer */
+ int rxfree; /* Number of free, allocated RX buffers */
+
+ int rxpending; /* Non-zero if there are still descriptors
+* to be processed from a previous descriptor
+* interrupt condition that has been cleared */
+
+ int txtail; /* Next TX descriptor to check status on */
+ int txhead; /* Next TX descriptor to use */
+
+ /* Number of free TX descriptors. This could be calculated from
+* rxhead and rxtail if one descriptor were left unused to disambiguate
+* full and empty conditions, but it's simpler to just keep track
+* explicitly. */
+
+ int txfree;
+
+ int phy_ok; /* The PHY is currently powered on. */
+
+ /* PHY status (duplex is 1 for half, 2 for full,
+* so that the default 0 indicates that neither has
+* yet been configured). */
+
+ int link_up;
+ int speed;
+ int duplex;
+
+ tx_desc *txring;
+ rx_desc *rxring;
+ sk_buff *txskbs[TSI108_TXRING_LEN];
+ sk_buff *rxskbs[TSI108_RXRING_LEN];
+
+ dma_addr_t txdma, rxdma;
+
+ /* txlock nests in misclock and phy_lock */
+
+ spinlock_t txlock, misclock;
+
+ /* stats is used to hold the upper bits of each hardware counter,
+* and tmpstats is used to hold the full values for returning
+* to the caller of get_stats(). They must be separate in case
+* an overflow interrupt occurs before the sta
