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

[Marek Vasut <marex@denx.de>]

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