[v5,0/3] MIPS: ath79: add ag71xx support
mbox series

Message ID 20190520070716.23668-1-o.rempel@pengutronix.de
Headers show
Series
  • MIPS: ath79: add ag71xx support
Related show

Message

Oleksij Rempel May 20, 2019, 7:07 a.m. UTC
2019.05.20 v5:
- ag71xx: remove MII_CMD_WRITE, the name is confusing. It is
  actually disables MII_CMD_READ.
- ag71xx: rework ag71xx_mdio_mii_read/write
- ag71xx: set proper mask for the addr in ag71xx_mdio_mii_read/write
- Kconfig: remove MDIO_BITBANG
- ag71xx: ./scripts/checkpatch.pl it.

2019.05.19 v4:
- DT: define eth and mdio clocks
- ag71xx: remove module parameters
- ag71xx: return proper error value on mdio_read/write
- ag71xx: use proper mdio clock registration
- ag71xx: add ag71xx_dma_wait_stop() for ag71xx_dma_reset()
- ag71xx: remove ag71xx_speed_str()
- ag71xx: use phydev->link/sped/duplex instead of ag-> variants
- ag71xx: use WARN() instead of BUG()
- ag71xx: drop big part of ag71xx_phy_link_adjust()
- ag71xx: drop most of ag71xx_do_ioctl()
- ag71xx: register eth clock
- ag71xx: remove AG71XX_ETH0_NO_MDIO quirk.

2019.04.22 v3:
- ag71xx: use phy_modes() instead of ag71xx_get_phy_if_mode_name()
- ag71xx: remove .ndo_poll_controller support
- ag71xx: unregister_netdev before disconnecting phy.

2019.04.18 v2:
- ag71xx: add list of openwrt authors
- ag71xx: remove redundant PHY_POLL assignment
- ag71xx: use phy_attached_info instead of netif_info
- ag71xx: remove redundant netif_carrier_off() on .stop.
- DT: use "ethernet" instead of "eth"

This patch series provide ethernet support for many Atheros/QCA
MIPS based SoCs.

I reworked ag71xx driver which was previously maintained within OpenWRT
repository. So far, following changes was made to make upstreaming
easier:
- everything what can be some how used in user space was removed. Most
  of it was debug functionality.
- most of deficetree bindings was removed. Not every thing made sense
  and most of it is SoC specific, so it is possible to detect it by
  compatible.
- mac and mdio parts are merged in to one driver. It makes easier to
  maintaine SoC specific quirks.


Oleksij Rempel (3):
  dt-bindings: net: add qca,ar71xx.txt documentation
  MIPS: ath79: ar9331: add Ethernet nodes
  net: ethernet: add ag71xx driver

 .../devicetree/bindings/net/qca,ar71xx.txt    |   45 +
 arch/mips/boot/dts/qca/ar9331.dtsi            |   26 +
 arch/mips/boot/dts/qca/ar9331_dpt_module.dts  |    8 +
 drivers/net/ethernet/atheros/Kconfig          |   10 +-
 drivers/net/ethernet/atheros/Makefile         |    1 +
 drivers/net/ethernet/atheros/ag71xx.c         | 1882 +++++++++++++++++
 6 files changed, 1971 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/qca,ar71xx.txt
 create mode 100644 drivers/net/ethernet/atheros/ag71xx.c

Comments

Andrew Lunn May 20, 2019, 3:50 p.m. UTC | #1
On Mon, May 20, 2019 at 09:07:16AM +0200, Oleksij Rempel wrote:
> Add support for Atheros/QCA AR7XXX/AR9XXX/QCA95XX built-in ethernet mac support
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

For the MDIO and PHY parts:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

I've not looked at the rest.

    Andrew
David Miller May 20, 2019, 5:51 p.m. UTC | #2
From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: Mon, 20 May 2019 09:07:16 +0200

> +struct ag71xx_buf {
> +	union {
> +		struct sk_buff *skb;
> +		void *rx_buf;
> +	};
> +	union {
> +		dma_addr_t dma_addr;
> +		unsigned int len;
> +	};
> +};

I find this double union very confusing.

When using unions you should make it strictly clear which members are used
together, at what times, and in which situations.

Therefore, please use something like anonymous structures to group the
members that are used together at the same time, something like:

struct ag71xx_buf {
	union {
		struct {
			struct sk_buff *skb;
			dma_addr_t dma_addr;
		} tx;
		struct {
			void *rx_buf;
			unsigned int len;
		} rx;
};

Or at the very least add a very big comment that explains the use of
the union members.

> +static int ag71xx_mdio_mii_read(struct mii_bus *bus, int addr, int reg)
> +{
> +	struct ag71xx *ag = bus->priv;
> +	struct net_device *ndev = ag->ndev;
> +	int err, val;

Reverse christmas tree here please.

> +static int ag71xx_mdio_mii_write(struct mii_bus *bus, int addr, int reg,
> +				 u16 val)
> +{
> +	struct ag71xx *ag = bus->priv;
> +	struct net_device *ndev = ag->ndev;
> +

Likewise.

> +static int ag71xx_mdio_probe(struct ag71xx *ag)
> +{
> +	static struct mii_bus *mii_bus;
> +	struct device *dev = &ag->pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct net_device *ndev = ag->ndev;
> +	int err;

Likewise.

> +static int ag71xx_tx_packets(struct ag71xx *ag, bool flush)
> +{
> +	struct ag71xx_ring *ring = &ag->tx_ring;
> +	struct net_device *ndev = ag->ndev;
> +	bool dma_stuck = false;
> +	int ring_mask = BIT(ring->order) - 1;
> +	int ring_size = BIT(ring->order);
> +	int sent = 0;
> +	int bytes_compl = 0;
> +	int n = 0;

Likewise.

And so on, and so forth, for the rest of this file.