[net-next 0/3] Add Aspeed G7 FTGMAC100 support

[net-next 0/3] Add Aspeed G7 FTGMAC100 support

[Jacky Chou]

The Aspeed 7th generation SoC features features three FTGMAC100.
The main difference from the previous generation is that the 
FTGMAC100 adds support for 64-bit DMA capability. Another change
is that the RMII/RGMII pin strap configuration is changed to be set 
in the bit 20 fo register 0x50.

Jacky Chou (3):
  dt-bindings: net: ftgmac100: support for AST2700
  net: faraday: Add ARM64 in FTGMAC100 for AST2700
  net: ftgmac100: Support for AST2700

 .../bindings/net/faraday,ftgmac100.yaml       |  3 +-
 drivers/net/ethernet/faraday/Kconfig          |  5 +-
 drivers/net/ethernet/faraday/ftgmac100.c      | 62 ++++++++++++++-----
 drivers/net/ethernet/faraday/ftgmac100.h      | 10 +++
 4 files changed, 59 insertions(+), 21 deletions(-)

-- 
2.25.1

[net-next 1/3] dt-bindings: net: ftgmac100: support for AST2700

[Jacky Chou]

The AST2700 is the 7th generation SoC from Aspeed.
Add compatible support for AST2700 in yaml.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
index 9bcbacb6640d..fffe5c51daa9 100644
--- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
+++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
@@ -21,6 +21,7 @@ properties:
               - aspeed,ast2400-mac
               - aspeed,ast2500-mac
               - aspeed,ast2600-mac
+              - aspeed,ast2700-mac
           - const: faraday,ftgmac100
 
   reg:
@@ -33,7 +34,7 @@ properties:
     minItems: 1
     items:
       - description: MAC IP clock
-      - description: RMII RCLK gate for AST2500/2600
+      - description: RMII RCLK gate for AST2500/2600/2700
 
   clock-names:
     minItems: 1
-- 
2.25.1

Re: [net-next 1/3] dt-bindings: net: ftgmac100: support for AST2700

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

On Thu, Nov 07, 2024 at 07:14:58PM +0800, Jacky Chou wrote:
> The AST2700 is the 7th generation SoC from Aspeed.
> Add compatible support for AST2700 in yaml.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[net-next 2/3] net: faraday: Add ARM64 in FTGMAC100 for AST2700

[Jacky Chou]

AST2700 is a ARM 64-bit SoC and ftgmac100 adds support 64-bit
DMA capability in AST2700.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/Kconfig | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/Kconfig b/drivers/net/ethernet/faraday/Kconfig
index c699bd6bcbb9..d5a088d88c3d 100644
--- a/drivers/net/ethernet/faraday/Kconfig
+++ b/drivers/net/ethernet/faraday/Kconfig
@@ -6,7 +6,7 @@
 config NET_VENDOR_FARADAY
 	bool "Faraday devices"
 	default y
-	depends on ARM || COMPILE_TEST
+	depends on ARM || ARM64 || COMPILE_TEST
 	help
 	  If you have a network (Ethernet) card belonging to this class, say Y.
 
@@ -28,8 +28,7 @@ config FTMAC100
 
 config FTGMAC100
 	tristate "Faraday FTGMAC100 Gigabit Ethernet support"
-	depends on ARM || COMPILE_TEST
-	depends on !64BIT || BROKEN
+	depends on ARM || ARM64 || COMPILE_TEST
 	select PHYLIB
 	select MDIO_ASPEED if MACH_ASPEED_G6
 	select CRC32
-- 
2.25.1

[net-next 3/3] net: ftgmac100: Support for AST2700

[Jacky Chou]

The AST2700 is the 7th generation SoC from Aspeed, featuring three GPIO
controllers that are support 64-bit DMA capability.
Adding features is shown in the following list.
1.Support 64-bit DMA
  Add the high address (63:32) registers for description address and the
  description field for packet buffer with high address part.
  These registers and fields in legacy Aspeed SoC are reserved.
  This 64-bit DMA changing has verified on legacy Aspeed Soc, like
  AST2600.
2.Set RMII pin strap in AST2700 compitable
  Use bit 20 of MAC 0x50 to represent the pin strap of AST2700 RMII and
  RGMII. Set to 1 is RMII pin, otherwise is RGMII.
  This bis is also reserved in legacy Aspeed SoC.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 62 +++++++++++++++++-------
 drivers/net/ethernet/faraday/ftgmac100.h | 10 ++++
 2 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 17ec35e75a65..0f4f161d0a90 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -9,6 +9,7 @@
 #define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
 
 #include <linux/clk.h>
+#include <linux/reset.h>
 #include <linux/dma-mapping.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
@@ -19,6 +20,7 @@
 #include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/crc32.h>
@@ -98,6 +100,7 @@ struct ftgmac100 {
 	struct work_struct reset_task;
 	struct mii_bus *mii_bus;
 	struct clk *clk;
+	struct reset_control *rst;
 
 	/* AST2500/AST2600 RMII ref clock gate */
 	struct clk *rclk;
@@ -265,10 +268,12 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
 	iowrite32(reg, priv->base + FTGMAC100_OFFSET_ISR);
 
 	/* 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);
 
 	/* Setup TX ring buffer base */
-	iowrite32(priv->txdes_dma, priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
+	iowrite32(lower_32_bits(priv->txdes_dma), priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
+	iowrite32(upper_32_bits(priv->txdes_dma), priv->base + FTGMAC100_OFFSET_TXR_BADDR_HIGH);
 
 	/* Configure RX buffer size */
 	iowrite32(FTGMAC100_RBSR_SIZE(RX_BUF_SIZE),
@@ -349,6 +354,10 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
 	if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
 		maccr |= FTGMAC100_MACCR_RM_VLAN;
 
+	if (of_device_is_compatible(priv->dev->of_node, "aspeed,ast2700-mac") &&
+	    priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII)
+		maccr |= FTGMAC100_MACCR_RMII_ENABLE;
+
 	/* Hit the HW */
 	iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
 }
@@ -425,7 +434,8 @@ static int ftgmac100_alloc_rx_buf(struct ftgmac100 *priv, unsigned int entry,
 	priv->rx_skbs[entry] = skb;
 
 	/* 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);
 
 	/* Ensure the above is ordered vs clearing the OWN bit */
 	dma_wmb();
@@ -551,7 +561,7 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int *processed)
 				       csum_vlan & 0xffff);
 
 	/* Tear down DMA mapping, do necessary cache management */
-	map = le32_to_cpu(rxdes->rxdes3);
+	map = le32_to_cpu(rxdes->rxdes3) | ((rxdes->rxdes2 & FTGMAC100_RXDES2_RXBUF_BADR_HI) << 16);
 
 #if defined(CONFIG_ARM) && !defined(CONFIG_ARM_DMA_USE_IOMMU)
 	/* When we don't have an iommu, we can save cycles by not
@@ -563,7 +573,6 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int *processed)
 	dma_unmap_single(priv->dev, map, RX_BUF_SIZE, DMA_FROM_DEVICE);
 #endif
 
-
 	/* Resplenish rx ring */
 	ftgmac100_alloc_rx_buf(priv, pointer, rxdes, GFP_ATOMIC);
 	priv->rx_pointer = ftgmac100_next_rx_pointer(priv, pointer);
@@ -628,7 +637,9 @@ static void ftgmac100_free_tx_packet(struct ftgmac100 *priv,
 				     struct ftgmac100_txdes *txdes,
 				     u32 ctl_stat)
 {
-	dma_addr_t map = le32_to_cpu(txdes->txdes3);
+	dma_addr_t map = le32_to_cpu(txdes->txdes3) |
+			 ((txdes->txdes2 & FTGMAC100_TXDES2_TXBUF_BADR_HI) << 16);
+
 	size_t len;
 
 	if (ctl_stat & FTGMAC100_TXDES0_FTS) {
@@ -784,7 +795,8 @@ static netdev_tx_t ftgmac100_hard_start_xmit(struct sk_buff *skb,
 	f_ctl_stat |= FTGMAC100_TXDES0_FTS;
 	if (nfrags == 0)
 		f_ctl_stat |= FTGMAC100_TXDES0_LTS;
-	txdes->txdes3 = cpu_to_le32(map);
+	txdes->txdes2 = FIELD_PREP(FTGMAC100_TXDES2_TXBUF_BADR_HI, upper_32_bits((ulong)map));
+	txdes->txdes3 = lower_32_bits(map);
 	txdes->txdes1 = cpu_to_le32(csum_vlan);
 
 	/* Next descriptor */
@@ -812,7 +824,9 @@ static netdev_tx_t ftgmac100_hard_start_xmit(struct sk_buff *skb,
 			ctl_stat |= FTGMAC100_TXDES0_LTS;
 		txdes->txdes0 = cpu_to_le32(ctl_stat);
 		txdes->txdes1 = 0;
-		txdes->txdes3 = cpu_to_le32(map);
+		txdes->txdes2 =
+			FIELD_PREP(FTGMAC100_TXDES2_TXBUF_BADR_HI, upper_32_bits((ulong)map));
+		txdes->txdes3 = lower_32_bits(map);
 
 		/* Next one */
 		pointer = ftgmac100_next_tx_pointer(priv, pointer);
@@ -887,7 +901,8 @@ static void ftgmac100_free_buffers(struct ftgmac100 *priv)
 	for (i = 0; i < priv->rx_q_entries; i++) {
 		struct ftgmac100_rxdes *rxdes = &priv->rxdes[i];
 		struct sk_buff *skb = priv->rx_skbs[i];
-		dma_addr_t map = le32_to_cpu(rxdes->rxdes3);
+		dma_addr_t map = le32_to_cpu(rxdes->rxdes3) |
+				 ((rxdes->rxdes2 & FTGMAC100_RXDES2_RXBUF_BADR_HI) << 16);
 
 		if (!skb)
 			continue;
@@ -986,7 +1001,9 @@ static void ftgmac100_init_rings(struct ftgmac100 *priv)
 	for (i = 0; i < priv->rx_q_entries; i++) {
 		rxdes = &priv->rxdes[i];
 		rxdes->rxdes0 = 0;
-		rxdes->rxdes3 = cpu_to_le32(priv->rx_scratch_dma);
+		rxdes->rxdes2 = FIELD_PREP(FTGMAC100_RXDES2_RXBUF_BADR_HI,
+					   upper_32_bits(priv->rx_scratch_dma));
+		rxdes->rxdes3 = lower_32_bits(priv->rx_scratch_dma);
 	}
 	/* 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);
-
 }
 
 static int ftgmac100_mii_probe(struct net_device *netdev)
@@ -1882,7 +1896,8 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	np = pdev->dev.of_node;
 	if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
 		   of_device_is_compatible(np, "aspeed,ast2500-mac") ||
-		   of_device_is_compatible(np, "aspeed,ast2600-mac"))) {
+		   of_device_is_compatible(np, "aspeed,ast2600-mac") ||
+		   of_device_is_compatible(np, "aspeed,ast2700-mac"))) {
 		priv->rxdes0_edorr_mask = BIT(30);
 		priv->txdes0_edotr_mask = BIT(30);
 		priv->is_aspeed = true;
@@ -1965,16 +1980,27 @@ static int ftgmac100_probe(struct platform_device *pdev)
 			dev_err(priv->dev, "MII probe failed!\n");
 			goto err_ncsi_dev;
 		}
-
 	}
 
 	if (priv->is_aspeed) {
+		struct reset_control *rst;
+
 		err = ftgmac100_setup_clk(priv);
 		if (err)
 			goto err_phy_connect;
 
-		/* Disable ast2600 problematic HW arbitration */
-		if (of_device_is_compatible(np, "aspeed,ast2600-mac"))
+		rst = devm_reset_control_get_optional(priv->dev, NULL);
+		if (IS_ERR(rst))
+			goto err_register_netdev;
+
+		priv->rst = rst;
+		err = reset_control_assert(priv->rst);
+		mdelay(10);
+		err = reset_control_deassert(priv->rst);
+
+		/* Disable some aspeed platform problematic HW arbitration */
+		if (of_device_is_compatible(np, "aspeed,ast2600-mac") ||
+		    of_device_is_compatible(np, "aspeed,ast2700-mac"))
 			iowrite32(FTGMAC100_TM_DEFAULT,
 				  priv->base + FTGMAC100_OFFSET_TM);
 	}
@@ -2010,6 +2036,8 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		goto err_register_netdev;
 	}
 
+	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+
 	netdev_info(netdev, "irq %d, mapped at %p\n", netdev->irq, priv->base);
 
 	return 0;
diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h
index 4968f6f0bdbc..ac39b864a80c 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.h
+++ b/drivers/net/ethernet/faraday/ftgmac100.h
@@ -57,6 +57,13 @@
 #define FTGMAC100_OFFSET_RX_RUNT	0xc0
 #define FTGMAC100_OFFSET_RX_CRCER_FTL	0xc4
 #define FTGMAC100_OFFSET_RX_COL_LOST	0xc8
+/* reserved 0xcc - 0x174 */
+#define FTGMAC100_OFFSET_TXR_BADDR_LOW		0x178	/* ast2700 */
+#define FTGMAC100_OFFSET_TXR_BADDR_HIGH		0x17c	/* ast2700 */
+#define FTGMAC100_OFFSET_HPTXR_BADDR_LOW	0x180	/* ast2700 */
+#define FTGMAC100_OFFSET_HPTXR_BADDR_HIGH	0x184	/* ast2700 */
+#define FTGMAC100_OFFSET_RXR_BADDR_LOW		0x188	/* ast2700 */
+#define FTGMAC100_OFFSET_RXR_BADDR_HIGH		0x18C	/* ast2700 */
 
 /*
  * Interrupt status register & interrupt enable register
@@ -166,6 +173,7 @@
 #define FTGMAC100_MACCR_RX_MULTIPKT	(1 << 16)
 #define FTGMAC100_MACCR_RX_BROADPKT	(1 << 17)
 #define FTGMAC100_MACCR_DISCARD_CRCERR	(1 << 18)
+#define FTGMAC100_MACCR_RMII_ENABLE	(1 << 20) /* defined in ast2700 */
 #define FTGMAC100_MACCR_FAST_MODE	(1 << 19)
 #define FTGMAC100_MACCR_SW_RST		(1 << 31)
 
@@ -225,6 +233,7 @@ struct ftgmac100_txdes {
 #define FTGMAC100_TXDES1_TX2FIC		(1 << 30)
 #define FTGMAC100_TXDES1_TXIC		(1 << 31)
 
+#define FTGMAC100_TXDES2_TXBUF_BADR_HI	GENMASK(18, 16)
 /*
  * Receive descriptor, aligned to 16 bytes
  */
@@ -271,4 +280,5 @@ struct ftgmac100_rxdes {
 #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR	(1 << 26)
 #define FTGMAC100_RXDES1_IP_CHKSUM_ERR	(1 << 27)
 
+#define FTGMAC100_RXDES2_RXBUF_BADR_HI	GENMASK(18, 16)
 #endif /* __FTGMAC100_H */
-- 
2.25.1

Re: [net-next 3/3] net: ftgmac100: Support for AST2700

[Maxime Chevallier]

Hi,

On Thu, 7 Nov 2024 19:15:00 +0800
Jacky Chou <jacky_chou@aspeedtech.com> wrote:

> The AST2700 is the 7th generation SoC from Aspeed, featuring three GPIO
> controllers that are support 64-bit DMA capability.
> Adding features is shown in the following list.
> 1.Support 64-bit DMA
>   Add the high address (63:32) registers for description address and the
>   description field for packet buffer with high address part.
>   These registers and fields in legacy Aspeed SoC are reserved.
>   This 64-bit DMA changing has verified on legacy Aspeed Soc, like
>   AST2600.

Maybe each of these features should be in a dedicated patch ?

> 2.Set RMII pin strap in AST2700 compitable
				  compatible

>   Use bit 20 of MAC 0x50 to represent the pin strap of AST2700 RMII and
>   RGMII. Set to 1 is RMII pin, otherwise is RGMII.
>   This bis is also reserved in legacy Aspeed SoC.
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>

[...]

> @@ -349,6 +354,10 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
>  	if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
>  		maccr |= FTGMAC100_MACCR_RM_VLAN;
>  
> +	if (of_device_is_compatible(priv->dev->of_node, "aspeed,ast2700-mac") &&
> +	    priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII)
> +		maccr |= FTGMAC100_MACCR_RMII_ENABLE;

The driver code takes the assumption that netdev->phydev might be NULL,
I think you should therefore add an extra check here as well before
getting the interface mode.

Thanks,

Maxime

回覆: [net-next 3/3] net: ftgmac100: Support for AST2700

[Jacky Chou]

Hi Maxime

Thank you for your reply.

> > The AST2700 is the 7th generation SoC from Aspeed, featuring three
> > GPIO controllers that are support 64-bit DMA capability.
> > Adding features is shown in the following list.
> > 1.Support 64-bit DMA
> >   Add the high address (63:32) registers for description address and the
> >   description field for packet buffer with high address part.
> >   These registers and fields in legacy Aspeed SoC are reserved.
> >   This 64-bit DMA changing has verified on legacy Aspeed Soc, like
> >   AST2600.
> 
> Maybe each of these features should be in a dedicated patch ?

Agree. I will separate them into separate patches on next version.

> 
> > 2.Set RMII pin strap in AST2700 compitable
> 				  compatible
> 
> >   Use bit 20 of MAC 0x50 to represent the pin strap of AST2700 RMII and
> >   RGMII. Set to 1 is RMII pin, otherwise is RGMII.
> >   This bis is also reserved in legacy Aspeed SoC.
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> 
> [...]
> 
> > @@ -349,6 +354,10 @@ static void ftgmac100_start_hw(struct ftgmac100
> *priv)
> >  	if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
> >  		maccr |= FTGMAC100_MACCR_RM_VLAN;
> >
> > +	if (of_device_is_compatible(priv->dev->of_node, "aspeed,ast2700-mac")
> &&
> > +	    priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII)
> > +		maccr |= FTGMAC100_MACCR_RMII_ENABLE;
> 
> The driver code takes the assumption that netdev->phydev might be NULL, I
> think you should therefore add an extra check here as well before getting the
> interface mode.

Agree. I will add a check for netdev->phydev here. Perhaps it is null.

Thanks,
Jacky

Re: [net-next 3/3] net: ftgmac100: Support for AST2700

[Andrew Lunn]

>  	/* 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

回覆: [net-next 3/3] net: ftgmac100: Support for AST2700

[Jacky Chou]

Hi Andrew,

Thank you for your reply.

> >  	/* 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?

These registers that add in AST2700 do not exist in older generations.
I have verified that these addresses can be accessed on older generations, 
like AST2600, etc. They are no impact.

> 
> >  	/* 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 */

The rxdes3 fills in the packet buffer address. HW will use it to do DMA to put 
packet content from receiving side.

> 
> Also, should its type be changed to __le32 ?

Agree. I will add this on next version.

> 
> > -	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.

Why does it need to return 42 in rxdes3?
The packet buffer address of the RX descriptor is used in both software and 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.

Agree, I will adjust this part on next version.

> 
> > +	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> 
> This can fail, you should check the return value.

Agree. I will add to check the return value on next version.

Thanks,
Jacky

Re: 回覆: [net-next 3/3] net: ftgmac100: Support for AST2700

[Andrew Lunn]

> > > -	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.
> 
> Why does it need to return 42 in rxdes3?
> The packet buffer address of the RX descriptor is used in both software and hardware.

42 is just a random value. The point is, what do older generation of
devices return here? Some random value? Something well defined?

You basically need to convince us that you are not breaking older
systems by accessing registers which they do not have. Describe in the
commit message how you know this is safe, what testing you have done
etc.

	Andrew

回覆: 回覆: [net-next 3/3] net: ftgmac100: Support for AST2700

[Jacky Chou]

Hi Andrew,


> > > > -	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.
> >
> > Why does it need to return 42 in rxdes3?
> > The packet buffer address of the RX descriptor is used in both software and
> hardware.
> 
> 42 is just a random value. The point is, what do older generation of devices
> return here? Some random value? Something well defined?
> 
> You basically need to convince us that you are not breaking older systems by
> accessing registers which they do not have. Describe in the commit message
> how you know this is safe, what testing you have done etc.

Thanks for your kind reminder.
I will commit more detail information in next version.

Thanks,
Jacky

Re: [net-next 3/3] net: ftgmac100: Support for AST2700

[Simon Horman]

On Thu, Nov 07, 2024 at 07:15:00PM +0800, Jacky Chou wrote:
> The AST2700 is the 7th generation SoC from Aspeed, featuring three GPIO
> controllers that are support 64-bit DMA capability.
> Adding features is shown in the following list.
> 1.Support 64-bit DMA
>   Add the high address (63:32) registers for description address and the
>   description field for packet buffer with high address part.
>   These registers and fields in legacy Aspeed SoC are reserved.
>   This 64-bit DMA changing has verified on legacy Aspeed Soc, like
>   AST2600.
> 2.Set RMII pin strap in AST2700 compitable
>   Use bit 20 of MAC 0x50 to represent the pin strap of AST2700 RMII and
>   RGMII. Set to 1 is RMII pin, otherwise is RGMII.
>   This bis is also reserved in legacy Aspeed SoC.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>

...

> @@ -1965,16 +1980,27 @@ static int ftgmac100_probe(struct platform_device *pdev)
>  			dev_err(priv->dev, "MII probe failed!\n");
>  			goto err_ncsi_dev;
>  		}
> -
>  	}
>  
>  	if (priv->is_aspeed) {
> +		struct reset_control *rst;
> +
>  		err = ftgmac100_setup_clk(priv);
>  		if (err)
>  			goto err_phy_connect;
>  
> -		/* Disable ast2600 problematic HW arbitration */
> -		if (of_device_is_compatible(np, "aspeed,ast2600-mac"))
> +		rst = devm_reset_control_get_optional(priv->dev, NULL);
> +		if (IS_ERR(rst))

Hi Jacky,

Should err be set to ERR_PTR(rst) here so that value is returned by
the function?

> +			goto err_register_netdev;
> +
> +		priv->rst = rst;
> +		err = reset_control_assert(priv->rst);
> +		mdelay(10);
> +		err = reset_control_deassert(priv->rst);
> +
> +		/* Disable some aspeed platform problematic HW arbitration */
> +		if (of_device_is_compatible(np, "aspeed,ast2600-mac") ||
> +		    of_device_is_compatible(np, "aspeed,ast2700-mac"))
>  			iowrite32(FTGMAC100_TM_DEFAULT,
>  				  priv->base + FTGMAC100_OFFSET_TM);
>  	}

回覆: [net-next 3/3] net: ftgmac100: Support for AST2700

[Jacky Chou]

Hi Simon,

Thank you for your reply.

> > @@ -1965,16 +1980,27 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
> >  			dev_err(priv->dev, "MII probe failed!\n");
> >  			goto err_ncsi_dev;
> >  		}
> > -
> >  	}
> >
> >  	if (priv->is_aspeed) {
> > +		struct reset_control *rst;
> > +
> >  		err = ftgmac100_setup_clk(priv);
> >  		if (err)
> >  			goto err_phy_connect;
> >
> > -		/* Disable ast2600 problematic HW arbitration */
> > -		if (of_device_is_compatible(np, "aspeed,ast2600-mac"))
> > +		rst = devm_reset_control_get_optional(priv->dev, NULL);
> > +		if (IS_ERR(rst))
> 
> Hi Jacky,
> 
> Should err be set to ERR_PTR(rst) here so that value is returned by the
> function?

Yes. I will add checking the return value in the next version.

> 
> > +			goto err_register_netdev;
> > +
> > +		priv->rst = rst;
> > +		err = reset_control_assert(priv->rst);
> > +		mdelay(10);
> > +		err = reset_control_deassert(priv->rst);
> > +
> > +		/* Disable some aspeed platform problematic HW arbitration */
> > +		if (of_device_is_compatible(np, "aspeed,ast2600-mac") ||
> > +		    of_device_is_compatible(np, "aspeed,ast2700-mac"))
> >  			iowrite32(FTGMAC100_TM_DEFAULT,
> >  				  priv->base + FTGMAC100_OFFSET_TM);
> >  	}

Thanks,
Jacky