On 06/08/2017 05:04 PM, Joe Hershberger wrote:
Hi Marek,
Hi!
I was looking at something else and noticed what looks like an issue with this code you submitted.
On Tue, May 24, 2016 at 4:29 PM, Marek Vasut marex@denx.de wrote:
Add ethernet driver for the AR933x and AR934x Atheros MIPS machines. The driver could be easily extended to other WiSoCs.
Signed-off-by: Marek Vasut marex@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Wills Wang wills.wang@live.com
V2: - Drop the printf() in case malloc fails, it's pointless to try and print something if we cannot allocate memory, since printf also allocates memory. V3: - Replace magic 0x11 with MII_MIPSCR register
[...] SNIP
+static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val)
Returns a u16
+{
u32 data;/* Dummy read followed by PHY read/write command. */ag7xxx_switch_reg_read(bus, 0x98, &data);data = val | (reg << 16) | (addr << 21) | BIT(30) | BIT(31);ag7xxx_switch_reg_write(bus, 0x98, data);/* Wait for operation to finish */do {ag7xxx_switch_reg_read(bus, 0x98, &data);} while (data & BIT(31));return data & 0xffff;+}
+static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg) +{
return ag7xxx_mdio_rw(bus, addr, reg, BIT(27));Directly returns said u16 as an int.
+}
+static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
u16 val)+{
ag7xxx_mdio_rw(bus, addr, reg, val);return 0;+}
[...] SNIP
+static int ag933x_phy_setup_common(struct udevice *dev) +{
struct ar7xxx_eth_priv *priv = dev_get_priv(dev);int i, ret, phymax;if (priv->model == AG7XXX_MODEL_AG933X)phymax = 4;else if (priv->model == AG7XXX_MODEL_AG934X)phymax = 5;elsereturn -EINVAL;if (priv->interface == PHY_INTERFACE_MODE_RMII) {ret = ag933x_phy_setup_reset_set(dev, phymax);if (ret)return ret;ret = ag933x_phy_setup_reset_fin(dev, phymax);if (ret)return ret;/* Read out link status */ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);Read the link status, which can never be negative.
Can you send a patch for this ?
Another issue: Is MII_MIPSCR really the register name? It's not better than "17" - constants should mean something, just just be a random name with the right value.
Can you check the AR9331 or AR9344 datasheet ? It should be there, although they tend to be cryptic.
if (ret < 0)return ret;return 0;Return 0 unconditionally. Presumably you want to actually check the link status to be something specific if you bother to read it out.
Actually, I think this is only for the switch ports, so we don't care about the link status.
}/* Switch ports */for (i = 0; i < phymax; i++) {ret = ag933x_phy_setup_reset_set(dev, i);if (ret)return ret;}for (i = 0; i < phymax; i++) {ret = ag933x_phy_setup_reset_fin(dev, i);if (ret)return ret;}for (i = 0; i < phymax; i++) {/* Read out link status */ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);Same thing here.
if (ret < 0)return ret;}return 0;+}