Dear Simon Glass,
In message 1302748176-4324-1-git-send-email-sjg@chromium.org you wrote:
The SMSC95XX is a USB hub with a built-in Ethernet adapter. This adds support for this, using the USB host network framework.
TEST=usb start; bootp; tftp ...
Signed-off-by: Simon Glass sjg@chromium.org
...
+#define TX_CMD_A_FIRST_SEG_ (0x00002000) +#define TX_CMD_A_LAST_SEG_ (0x00001000)
+/* Rx status word */ +#define RX_STS_FL_ (0x3FFF0000) /* Frame Length */ +#define RX_STS_ES_ (0x00008000) /* Error Summary */
...
Please drop the parens around all these constants. Please fix globally.
+/* SCSRs */ +#define ID_REV (0x00)
+#define INT_STS (0x08)
+#define TX_CFG (0x10) +#define TX_CFG_ON_ (0x00000004)
+#define HW_CFG (0x14) +#define HW_CFG_BIR_ (0x00001000) +#define HW_CFG_RXDOFF_ (0x00000600) +#define HW_CFG_MEF_ (0x00000020) +#define HW_CFG_BCE_ (0x00000002) +#define HW_CFG_LRST_ (0x00000008)
+#define PM_CTRL (0x20) +#define PM_CTL_PHY_RST_ (0x00000010)
+#define AFC_CFG (0x2C)
+/*
- Hi watermark = 15.5Kb (~10 mtu pkts)
- low watermark = 3k (~2 mtu pkts)
- backpressure duration = ~ 350us
- Apply FC on any frame.
- */
+#define AFC_CFG_DEFAULT (0x00F830A1)
+#define E2P_CMD (0x30) +#define E2P_CMD_BUSY_ (0x80000000) +#define E2P_CMD_READ_ (0x00000000) +#define E2P_CMD_TIMEOUT_ (0x00000400) +#define E2P_CMD_LOADED_ (0x00000200) +#define E2P_CMD_ADDR_ (0x000001FF)
+#define E2P_DATA (0x34)
+#define BURST_CAP (0x38)
+#define INT_EP_CTL (0x68) +#define INT_EP_CTL_PHY_INT_ (0x00008000)
+#define BULK_IN_DLY (0x6C)
+/* MAC CSRs */ +#define MAC_CR (0x100) +#define MAC_CR_MCPAS_ (0x00080000) +#define MAC_CR_PRMS_ (0x00040000) +#define MAC_CR_HPFILT_ (0x00002000) +#define MAC_CR_TXEN_ (0x00000008) +#define MAC_CR_RXEN_ (0x00000004)
+#define ADDRH (0x104)
+#define ADDRL (0x108)
+#define MII_ADDR (0x114) +#define MII_WRITE_ (0x02) +#define MII_BUSY_ (0x01) +#define MII_READ_ (0x00) /* ~of MII Write bit */
+#define MII_DATA (0x118)
+#define FLOW (0x11C)
+#define VLAN1 (0x120)
+#define COE_CR (0x130) +#define Tx_COE_EN_ (0x00010000) +#define Rx_COE_EN_ (0x00000001)
+/* Vendor-specific PHY Definitions */ +#define PHY_INT_SRC (29)
+#define PHY_INT_MASK (30) +#define PHY_INT_MASK_ANEG_COMP_ ((u16)0x0040) +#define PHY_INT_MASK_LINK_DOWN_ ((u16)0x0010) +#define PHY_INT_MASK_DEFAULT_ (PHY_INT_MASK_ANEG_COMP_ | \
PHY_INT_MASK_LINK_DOWN_)+/* USB Vendor Requests */ +#define USB_VENDOR_REQUEST_WRITE_REGISTER 0xA0
...
- ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
- if (ret < 0) {
debug("Failed to read HW_CFG: %d\n", ret);return ret;- }
- debug("Read Value from HW_CFG : 0x%08x\n", read_buf);
- read_buf |= HW_CFG_BIR_;
- ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
- if (ret < 0) {
debug("Failed to write HW_CFG_BIR_ bit in HW_CFG ""register, ret = %d\n", ret);return ret;- }
You repeat these sequences of smsc95xx_read_reg() resp. smsc95xx_write_reg() followed by the "if (ret < 0)" part about 20 times.
Please move the respective debug() message for the failure case into the functions, and consider using a loop here instead.
- read_buf = DEFAULT_BULK_IN_DELAY;
- ret = smsc95xx_write_reg(dev, BULK_IN_DLY, read_buf);
- if (ret < 0) {
debug("ret = %d", ret);return ret;- }
Is it intentional to use the read_buf with a smsc95xx_write_reg() call?
- ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
- if (ret < 0) {
debug("Failed to write HW_CFG register, ret=%d\n", ret);return ret;- }
Ditto here?
- ret = smsc95xx_write_reg(dev, AFC_CFG, read_buf);
- if (ret < 0) {
debug("Failed to write AFC_CFG: %d\n", ret);return ret;- }
And here?
- ret = smsc95xx_read_reg(dev, INT_EP_CTL, &read_buf);
- if (ret < 0) {
debug("Failed to read INT_EP_CTL: %d", ret);return ret;- }
And here we have &read_buf while everywhere else we have read_buf only?
Best regards,
Wolfgang Denk