On 05/08/2016 02:58 PM, Daniel Schwierzeck wrote:
Hi!
> Am 05.05.2016 um 21:34 schrieb Marek Vasut:
>> 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>
>> ---
>> drivers/net/Kconfig | 9 +
>> drivers/net/Makefile | 1 +
>> drivers/net/ag7xxx.c | 982
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 992 insertions(+)
>> create mode 100644 drivers/net/ag7xxx.c
>
> Reviewed-by: Daniel Schwierzeck <daniel.schwierz...@gmail.com>
>
> nits below
[...]
>> +static void ag7xxx_hw_setup(struct udevice *dev)
>> +{
>> + struct ar7xxx_eth_priv *priv = dev_get_priv(dev);
>> + u32 speed;
>> +
>> + setbits_be32(priv->regs + AG7XXX_ETH_CFG1,
>> + AG7XXX_ETH_CFG1_RX_RST | AG7XXX_ETH_CFG1_TX_RST |
>> + AG7XXX_ETH_CFG1_SOFT_RST);
>
> explicitely using the BE variant is inconsistent because everywhere else
> your are using writel/readl.
This is a BE platform, so I have to use setbits_be*() . I was under the
impression that writel()/readl() are endianness agnostic now, so it's OK
to use those, no ?
> You can also use setbits_32() etc. to avoid
> forcing the endianess. Though it doesn't matter for the generated code
> unless you enable CONFIG_SWAP_IO_SPACE.
Ha, I didn't know we now have endianness-agnostic setbits_*(), nice.
>> +
>> + mdelay(10);
>> +
>> + writel(AG7XXX_ETH_CFG1_RX_EN | AG7XXX_ETH_CFG1_TX_EN,
>> + priv->regs + AG7XXX_ETH_CFG1);
>> +
>> + if (priv->interface == PHY_INTERFACE_MODE_RMII)
>> + speed = AG7XXX_ETH_CFG2_IF_10_100;
>> + else
>> + speed = AG7XXX_ETH_CFG2_IF_1000;
>> +
>> + clrsetbits_be32(priv->regs + AG7XXX_ETH_CFG2,
>> + AG7XXX_ETH_CFG2_IF_SPEED_MASK,
>> + speed | AG7XXX_ETH_CFG2_PAD_CRC_EN |
>> + AG7XXX_ETH_CFG2_LEN_CHECK);
>> +
>> + writel(0xfff0000, priv->regs + AG7XXX_ETH_FIFO_CFG_1);
>> + writel(0x1fff, priv->regs + AG7XXX_ETH_FIFO_CFG_2);
>> +
>> + writel(0x1f00, priv->regs + AG7XXX_ETH_FIFO_CFG_0);
>> + setbits_be32(priv->regs + AG7XXX_ETH_FIFO_CFG_4, 0x3ffff);
>> + writel(0x10ffff, priv->regs + AG7XXX_ETH_FIFO_CFG_1);
>> + writel(0xaaa0555, priv->regs + AG7XXX_ETH_FIFO_CFG_2);
>> + writel(0x7eccf, priv->regs + AG7XXX_ETH_FIFO_CFG_5);
>> + writel(0x1f00140, priv->regs + AG7XXX_ETH_FIFO_CFG_3);
>> +}
[...]
>> +static int ag7xxx_mdio_probe(struct udevice *dev)
>> +{
>> + struct ar7xxx_eth_priv *priv = dev_get_priv(dev);
>> + struct mii_dev *bus = mdio_alloc();
>> +
>> + if (!bus) {
>> + printf("Failed to allocate MDIO bus\n");
>
> isn't debug() better?
I think I will remove the printf() altogether, we ran out of memory, so
we're doomed anyway.
>> + return -ENOMEM;
>> + }
>> +
>> + bus->read = ag7xxx_mdio_read;
>> + bus->write = ag7xxx_mdio_write;
>> + snprintf(bus->name, sizeof(bus->name), dev->name);
>> +
>> + bus->priv = (void *)priv;
>> +
>> + return mdio_register(bus);
>> +}
[...]
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
