Hi Marek,
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 <ma...@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 <ma...@denx.de>
> Cc: Daniel Schwierzeck <daniel.schwierz...@gmail.com>
> Cc: Joe Hershberger <joe.hershber...@ni.com>
> Cc: Wills Wang <wills.w...@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;
> + else
> + return -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.
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.
> + 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.
> + }
> +
> + /* 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;
> +}
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
