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 <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.
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
