[U-Boot] [PATCH] net: Add ag7xxx driver for Atheros MIPS

[Marek Vasut]

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 at 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 at denx.de>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>> Cc: Joe Hershberger <joe.hershberger at ni.com>
>> Cc: Wills Wang <wills.wang at 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.

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;
>> +}


-- 
Best regards,
Marek Vasut