[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1f8b0258-0d09-4a65-8e1c-46d9569765bf@lunn.ch>
Date: Fri, 8 Nov 2024 00:23:09 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jacky Chou <jacky_chou@...eedtech.com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, p.zabel@...gutronix.de,
ratbert@...aday-tech.com, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next 3/3] net: ftgmac100: Support for AST2700
> /* Setup RX ring buffer base */
> - iowrite32(priv->rxdes_dma, priv->base + FTGMAC100_OFFSET_RXR_BADR);
> + iowrite32(lower_32_bits(priv->rxdes_dma), priv->base + FTGMAC100_OFFSET_RXR_BADR);
> + iowrite32(upper_32_bits(priv->rxdes_dma), priv->base + FTGMAC100_OFFSET_RXR_BADDR_HIGH);
This appears to write to registers which older generations do not
have. Is this safe? Is it defined in the datasheet what happens when
you write to reserved registers?
> /* Store DMA address into RX desc */
> - rxdes->rxdes3 = cpu_to_le32(map);
> + rxdes->rxdes2 = FIELD_PREP(FTGMAC100_RXDES2_RXBUF_BADR_HI, upper_32_bits(map));
> + rxdes->rxdes3 = lower_32_bits(map);
Maybe update the comment:
unsigned int rxdes3; /* not used by HW */
Also, should its type be changed to __le32 ?
> - map = le32_to_cpu(rxdes->rxdes3);
> + map = le32_to_cpu(rxdes->rxdes3) | ((rxdes->rxdes2 & FTGMAC100_RXDES2_RXBUF_BADR_HI) << 16);
Is this safe? You have to assume older generation of devices will
return 42 in rxdes3, since it is not used by the hardware.
> /* Mark the end of the ring */
> rxdes->rxdes0 |= cpu_to_le32(priv->rxdes0_edorr_mask);
> @@ -1249,7 +1266,6 @@ static int ftgmac100_poll(struct napi_struct *napi, int budget)
> more = ftgmac100_rx_packet(priv, &work_done);
> } while (more && work_done < budget);
>
> -
> /* The interrupt is telling us to kick the MAC back to life
> * after an RX overflow
> */
> @@ -1339,7 +1355,6 @@ static void ftgmac100_reset(struct ftgmac100 *priv)
> if (priv->mii_bus)
> mutex_lock(&priv->mii_bus->mdio_lock);
>
> -
> /* Check if the interface is still up */
> if (!netif_running(netdev))
> goto bail;
> @@ -1438,7 +1453,6 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
>
> if (netdev->phydev)
> mutex_lock(&netdev->phydev->lock);
> -
> }
There are quite a few whitespace changes like this in this
patch. Please remove them. If they are important, put them into a
patch of there own.
> + dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
This can fail, you should check the return value.
Andrew
Powered by blists - more mailing lists