Re: [PATCH v1 4/5] net: hpe: Add GXP UMAC Driver

[Simon Horman]

On Fri, Jul 21, 2023 at 04:20:43PM -0500, nick.hawkins@xxxxxxx wrote:

...

Hi Nick,

> diff --git a/drivers/net/ethernet/hpe/gxp-umac.c b/drivers/net/ethernet/hpe/gxp-umac.c

...

> +static void umac_set_mac_address(struct net_device *ndev, void *p_addr)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	char *addr = (char *)p_addr;
> +	unsigned int value;
> +
> +	/* update address to register */
> +	value = addr[0] << 8 | addr[1];
> +	writel(value, umac->base + UMAC_MAC_ADDR_HI);
> +	value = addr[2] << 8 | addr[3];
> +	writel(value, umac->base + UMAC_MAC_ADDR_MID);
> +	value = addr[4] << 8 | addr[5];
> +	writel(value, umac->base + UMAC_MAC_ADDR_LO);
> +}
> +
> +static int umac_eth_mac_addr(struct net_device *ndev, void *p)
> +{
> +	int ret;
> +	struct sockaddr *addr = p;

Please use reverse xmas tree - longest like to shortest - for
new Networking code. Likewise in some other places in this patch/series.

This tool can be helpful in this regard.
https://github.com/ecree-solarflare/xmastree

...

> +static int umac_init_hw(struct net_device *ndev)
> +{
> +	struct umac_priv *umac = netdev_priv(ndev);
> +	unsigned int value;
> +
> +	/* initialize tx and rx rings to first entry */
> +	writel(0, umac->base + UMAC_RING_PTR);
> +
> +	/* clear the missed bit */
> +	writel(0, umac->base + UMAC_CLEAR_STATUS);
> +
> +	/* disable checksum generation */
> +	writel(0, umac->base + UMAC_CKSUM_CONFIG);
> +
> +	/* write the ring size register */
> +	value = ((UMAC_RING_SIZE_256 << UMAC_TX_RING_SIZE_SHIFT) &
> +			UMAC_TX_RING_SIZE_MASK) |
> +		((UMAC_RING_SIZE_256 << UMAC_RX_RING_SIZE_SHIFT) &
> +			UMAC_RX_RING_SIZE_MASK);
> +	writel(value, umac->base + UMAC_RING_SIZE);
> +
> +	/* write rx ring base address */
> +	writel(cpu_to_le32(umac->rx_descs_dma_addr),
> +	       umac->base + UMAC_RX_RING_ADDR);

It is my understanding that writel will convert the value
from host byte order to little endien. If so then pre-converting value
seems incorrect. Perhaps this should be:

	writel(umac->rx_descs_dma_addr, umac->base + UMAC_RX_RING_ADDR);

Flagged by Sparse.

> +
> +	/* write tx ring base address */
> +	writel(cpu_to_le32(umac->tx_descs_dma_addr),
> +	       umac->base + UMAC_TX_RING_ADDR);

Ditto.

> +
> +	/* write burst size */
> +	writel(0x22, umac->base + UMAC_DMA_CONFIG);
> +
> +	umac_channel_disable(umac);
> +
> +	/* disable clocks and gigabit mode (leave channels disabled) */
> +	value = readl(umac->base + UMAC_CONFIG_STATUS);
> +	value &= 0xfffff9ff;
> +	writel(value, umac->base + UMAC_CONFIG_STATUS);
> +	udelay(2);
> +
> +	if (umac->use_ncsi) {
> +		/* set correct tx clock */
> +		value &= UMAC_CFG_TX_CLK_EN;
> +		value &= ~UMAC_CFG_GTX_CLK_EN;
> +		value &= ~UMAC_CFG_GIGABIT_MODE; /* RMII mode */
> +		value |= UMAC_CFG_FULL_DUPLEX; /* full duplex */
> +	} else {
> +		if (ndev->phydev->duplex)
> +			value |= UMAC_CFG_FULL_DUPLEX;
> +		else
> +			value &= ~UMAC_CFG_FULL_DUPLEX;
> +
> +		if (ndev->phydev->speed == SPEED_1000) {
> +			value &= ~UMAC_CFG_TX_CLK_EN;
> +			value |= UMAC_CFG_GTX_CLK_EN;
> +			value |= UMAC_CFG_GIGABIT_MODE;
> +		} else {
> +			value |= UMAC_CFG_TX_CLK_EN;
> +			value &= ~UMAC_CFG_GTX_CLK_EN;
> +			value &= ~UMAC_CFG_GIGABIT_MODE;
> +		}
> +	}
> +	writel(value, umac->base + UMAC_CONFIG_STATUS);
> +	udelay(2);
> +
> +	umac_channel_enable(umac);
> +
> +	return 0;
> +}

...

> diff --git a/drivers/net/ethernet/hpe/gxp-umac.h b/drivers/net/ethernet/hpe/gxp-umac.h

...

> +struct umac_rx_desc_entry {
> +	u32  dmaaddress;   // Start address for DMA operationg

1. operationg -> operating
2. It appears that this field is used to hold __le32 values.
   Perhaps it's type should be __le32.
   As is, Sparse complains about this.

> +	u16  status;       // Packet tx status and ownership flag
> +	u16  count;        // Number of bytes received
> +	u16  checksum;     // On-the-fly packet checksum
> +	u16  control;      // Checksum-in-time flag
> +	u32  reserved;
> +} __aligned(16);
> +
> +struct umac_rx_descs {
> +	struct umac_rx_desc_entry entrylist[UMAC_MAX_RX_DESC_ENTRIES];
> +	u8 framelist[UMAC_MAX_RX_DESC_ENTRIES][UMAC_MAX_RX_FRAME_SIZE];
> +} __packed;
> +
> +struct umac_tx_desc_entry {
> +	u32  dmaaddress;   // Start address for DMA operationg

Ditto.

> +	u16  status;       // Packet rx status, type, and ownership flag
> +	u16  count;        // Number of bytes received
> +	u32  cksumoffset;  // Specifies where to place packet checksum
> +	u32  reserved;
> +} __aligned(16);
> +
> +struct umac_tx_descs {
> +	struct umac_tx_desc_entry entrylist[UMAC_MAX_TX_DESC_ENTRIES];
> +	u8 framelist[UMAC_MAX_TX_DESC_ENTRIES][UMAC_MAX_TX_FRAME_SIZE];
> +} __packed;
> +
> +#endif
> -- 
> 2.17.1
> 
>