From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [78.32.30.218]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 69E801E501 for ; Wed, 24 Apr 2024 14:47:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=78.32.30.218 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713970040; cv=none; b=aeynq7yULkyAMqvaTox4EFe03F4xZkYMJBGxSXK0JSoeVfIgqssUOOgVzLtjGb6uNGYgSFOK3S7oWKawYj2isR19hE+rcJ5RbFTCjVTeYY4VWLlP5/12FGllWN2eU4hBBn+m3DEOgpcEr7POE3nix5A+VJJywwY5NdCebAhSLH0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713970040; c=relaxed/simple; bh=DjPt3O4nX9DDZSFdhf3U/xqn4ugQPCdVaEimZCw140I=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ESn/nvQDcDKZdkLNFj3qHon4WNsnA9xuxCaTceSC6fsPqZu3keqRmtLt91koODIH6Z26jmsVtvJtODyscEHzsi4zTeTFun7vjFVUmjxyaojQdHqpRZnHQB8mtCCC3LCwI7gAyXGrWC3YwbQ74zfGeImnwxUAhv1UMkTE3GjG5W8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=armlinux.org.uk; spf=none smtp.mailfrom=armlinux.org.uk; dkim=pass (2048-bit key) header.d=armlinux.org.uk header.i=@armlinux.org.uk header.b=1A+v8n2v; arc=none smtp.client-ip=78.32.30.218 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=armlinux.org.uk Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=armlinux.org.uk Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=armlinux.org.uk header.i=@armlinux.org.uk header.b="1A+v8n2v" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=CtCm0L2vDbGXwu/D/ZbRdljURC/eFWJxsY7KbC7phGE=; b=1A+v8n2vvCFROC6xKOBfEmx/et czVO1WvW0IuOLnbPeNGs/0I9Mo7Cw6RPmJCjuAC2gOcdTkPpzSsjkS+feoC4jX4iHYaxlpEsOlRZm N9fu08nQSUmV0Wyw1uHoLOXEsd/3GoEAEC84t8d+aiQZLD/5OTRY/4dpW/rlY2VHv6iHx44iuxRAN E4AWrHb5V7P8yeeE4zx5cIW4iTmA0aM/wE4muRwtDVDiTyQJq0ojshgju7vAI+W0nSUsy/iuoL/24 c36xbMvPANREdvQWS72whyIJY+jpvI0n5BPzSgtHE7rmSslquUx2zI1xY5vlUvimd8LnjEbQDs/90 w4Hgo8Aw==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:45026) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1rzdtd-00067T-2M; Wed, 24 Apr 2024 15:46:55 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1rzdta-0004jx-Dg; Wed, 24 Apr 2024 15:46:50 +0100 Date: Wed, 24 Apr 2024 15:46:50 +0100 From: "Russell King (Oracle)" To: Huacai Chen Cc: Serge Semin , Yanteng Si , andrew@lunn.ch, hkallweit1@gmail.com, peppe.cavallaro@st.com, alexandre.torgue@foss.st.com, joabreu@synopsys.com, Jose.Abreu@synopsys.com, guyinggang@loongson.cn, netdev@vger.kernel.org, chris.chenfeiyang@gmail.com, siyanteng01@gmail.com Subject: Re: [PATCH net-next v11 6/6] net: stmmac: dwmac-loongson: Add Loongson GNET support Message-ID: References: <9a8e5dfef0e9706e3d42bb20f59ea9ffa264dc8c.1712917541.git.siyanteng@loongson.cn> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: Russell King (Oracle) Hi, Is there any comment in your reply? I gave up paging through trying to find it. Hint: trim the context in your replies. Thanks. On Wed, Apr 24, 2024 at 10:11:20PM +0800, Huacai Chen wrote: > Hi, Serge, > > On Thu, Apr 18, 2024 at 10:01 PM Serge Semin wrote: > > > > On Fri, Apr 12, 2024 at 07:28:51PM +0800, Yanteng Si wrote: > > > There are two types of Loongson DWGMAC. The first type shares the same > > > register definitions and has similar logic as dwmac1000. The second type > > > uses several different register definitions, we think it is necessary to > > > distinguish rx and tx, so we split these bits into two. > > > > > > Simply put, we split some single bit fields into double bits fileds: > > > > > > Name Tx Rx > > > > > > DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000; > > > DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000; > > > DMA_STATUS_NIS = 0x00040000 | 0x00020000; > > > DMA_STATUS_AIS = 0x00010000 | 0x00008000; > > > DMA_STATUS_FBI = 0x00002000 | 0x00001000; > > > > > > Therefore, when using, TX and RX must be set at the same time. > > > > > > How to use them: > > > 1. Create the Loongson GNET-specific > > > stmmac_dma_ops.dma_interrupt() > > > stmmac_dma_ops.init_chan() > > > methods in the dwmac-loongson.c driver. Adding all the > > > Loongson-specific macros > > > > > > 2. Create a Loongson GNET-specific platform setup method with the next > > > semantics: > > > + allocate stmmac_dma_ops instance and initialize it with > > > dwmac1000_dma_ops. > > > + override the stmmac_dma_ops.{dma_interrupt, init_chan} with > > > the pointers to the methods defined in 2. > > > + allocate mac_device_info instance and initialize the > > > mac_device_info.dma field with a pointer to the new > > > stmmac_dma_ops instance. > > > + initialize mac_device_info in a way it's done in > > > dwmac1000_setup(). > > > > > > 3. Initialize plat_stmmacenet_data.setup() with the pointer to the > > > method created in 2. > > > > > > GNET features: > > > > > > Speeds: 10/100/1000Mbps > > > > > DMA-descriptors type: normal and enhanced > > > > Hm, it's either one or another. They can't be both supported because > > the alternative descriptors are enabled by the DESC_ENHANCED_FORMAT > > HDL parameter defined on the IP-core synthesize stage. > > > > > L3/L4 filters availability: support > > > VLAN hash table filter: support > > > PHY-interface: GMII > > > Remote Wake-up support: support > > > Mac Management Counters (MMC): support > > > DMA chennel number: 0x10 device is 8 and 0x37 device is 1 > > > > What about adding the info like: > > Number of additional MAC addresses > > MAC Hash-based filter support and if supported the hash table size. > > > > > > > > Others: > > > > > > GNET integrates both MAC and PHY chips inside. > > > GNET device: LS2K2000, LS7A2000, the chip connection between the mac and > > > phy of these devices is not normal and requires two rounds of > > > negotiation; LS7A2000 does not support half-duplex and > > > multi-channel; > > > > > > To enable multi-channel on LS2K2000, you need to turn off > > > hardware checksum. > > > > > > > > **Note**: Currently, only the LS2K2000's IP core is 0x10, > > > > This doesn't sound correct. The LS2K2000's IP-core is v3.73a. But the > > SNPS Version ID was manually altered by the hardware designers. > > > > > while the IP > > > cores of other devices are 0x37. > > > > Real IP-core ID isn't described by the hex number. It's a digit number > > like v3.73a/v3.50a/etc. Hex number you constantly repeat is the > > MAC_Version.SNPSVER register value. So please use the IP-core version > > in the v3.xx format all over the patch text. > > > > > > > > Signed-off-by: Feiyang Chen > > > Signed-off-by: Yinggang Gu > > > Signed-off-by: Yanteng Si > > > --- > > > drivers/net/ethernet/stmicro/stmmac/common.h | 2 + > > > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 476 ++++++++++++++++-- > > > .../ethernet/stmicro/stmmac/dwmac1000_dma.c | 1 + > > > .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 + > > > include/linux/stmmac.h | 1 + > > > 5 files changed, 447 insertions(+), 39 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > > > index 9cd62b2110a1..6777dc997e9f 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > > > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > > > @@ -29,6 +29,7 @@ > > > /* Synopsys Core versions */ > > > #define DWMAC_CORE_3_40 0x34 > > > #define DWMAC_CORE_3_50 0x35 > > > +#define DWMAC_CORE_3_70 0x37 > > > #define DWMAC_CORE_4_00 0x40 > > > #define DWMAC_CORE_4_10 0x41 > > > #define DWMAC_CORE_5_00 0x50 > > > @@ -258,6 +259,7 @@ struct stmmac_safety_stats { > > > #define CSR_F_300M 300000000 > > > > > > #define MAC_CSR_H_FRQ_MASK 0x20 > > > > > +#define MAC_CTRL_PORT_SELECT_10_100 BIT(15) > > > > No, this is already defined in: > > drivers/net/ethernet/stmicro/stmmac/dwmac1000.h > > as GMAC_CONTROL_PS macros. Use it instead of adding new macros. > > > > > > > > #define HASH_TABLE_SIZE 64 > > > #define PAUSE_TIME 0xffff > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > index 69078eb1f923..4edfbb4fcb64 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > @@ -8,8 +8,70 @@ > > > #include > > > #include > > > #include "stmmac.h" > > > +#include "dwmac_dma.h" > > > +#include "dwmac1000.h" > > > + > > > +/* Normal Loongson Tx Summary */ > > > +#define DMA_INTR_ENA_NIE_TX_LOONGSON 0x00040000 > > > +/* Normal Loongson Rx Summary */ > > > +#define DMA_INTR_ENA_NIE_RX_LOONGSON 0x00020000 > > > + > > > +#define DMA_INTR_NORMAL_LOONGSON (DMA_INTR_ENA_NIE_TX_LOONGSON | \ > > > + DMA_INTR_ENA_NIE_RX_LOONGSON | \ > > > + DMA_INTR_ENA_RIE | DMA_INTR_ENA_TIE) > > > + > > > +/* Abnormal Loongson Tx Summary */ > > > +#define DMA_INTR_ENA_AIE_TX_LOONGSON 0x00010000 > > > +/* Abnormal Loongson Rx Summary */ > > > +#define DMA_INTR_ENA_AIE_RX_LOONGSON 0x00008000 > > > + > > > +#define DMA_INTR_ABNORMAL_LOONGSON (DMA_INTR_ENA_AIE_TX_LOONGSON | \ > > > + DMA_INTR_ENA_AIE_RX_LOONGSON | \ > > > + DMA_INTR_ENA_FBE | DMA_INTR_ENA_UNE) > > > + > > > +#define DMA_INTR_DEFAULT_MASK_LOONGSON (DMA_INTR_NORMAL_LOONGSON | \ > > > + DMA_INTR_ABNORMAL_LOONGSON) > > > + > > > +/* Normal Loongson Tx Interrupt Summary */ > > > +#define DMA_STATUS_NIS_TX_LOONGSON 0x00040000 > > > +/* Normal Loongson Rx Interrupt Summary */ > > > +#define DMA_STATUS_NIS_RX_LOONGSON 0x00020000 > > > + > > > +/* Abnormal Loongson Tx Interrupt Summary */ > > > +#define DMA_STATUS_AIS_TX_LOONGSON 0x00010000 > > > +/* Abnormal Loongson Rx Interrupt Summary */ > > > +#define DMA_STATUS_AIS_RX_LOONGSON 0x00008000 > > > + > > > +/* Fatal Loongson Tx Bus Error Interrupt */ > > > +#define DMA_STATUS_FBI_TX_LOONGSON 0x00002000 > > > +/* Fatal Loongson Rx Bus Error Interrupt */ > > > +#define DMA_STATUS_FBI_RX_LOONGSON 0x00001000 > > > + > > > +#define DMA_STATUS_MSK_COMMON_LOONGSON (DMA_STATUS_NIS_TX_LOONGSON | \ > > > + DMA_STATUS_NIS_RX_LOONGSON | \ > > > + DMA_STATUS_AIS_TX_LOONGSON | \ > > > + DMA_STATUS_AIS_RX_LOONGSON | \ > > > + DMA_STATUS_FBI_TX_LOONGSON | \ > > > + DMA_STATUS_FBI_RX_LOONGSON) > > > + > > > +#define DMA_STATUS_MSK_RX_LOONGSON (DMA_STATUS_ERI | DMA_STATUS_RWT | \ > > > + DMA_STATUS_RPS | DMA_STATUS_RU | \ > > > + DMA_STATUS_RI | DMA_STATUS_OVF | \ > > > + DMA_STATUS_MSK_COMMON_LOONGSON) > > > + > > > +#define DMA_STATUS_MSK_TX_LOONGSON (DMA_STATUS_ETI | DMA_STATUS_UNF | \ > > > + DMA_STATUS_TJT | DMA_STATUS_TU | \ > > > + DMA_STATUS_TPS | DMA_STATUS_TI | \ > > > + DMA_STATUS_MSK_COMMON_LOONGSON) > > > > > > #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > > > +#define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 > > > +#define LOONGSON_DWMAC_CORE_1_00 0x10 /* Loongson custom IP */ > > > +#define CHANNEL_NUM 8 > > > + > > > +struct loongson_data { > > > + struct device *dev; > > > +}; > > > > > > struct stmmac_pci_info { > > > int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat); > > > @@ -56,6 +117,8 @@ static int loongson_gmac_data(struct pci_dev *pdev, > > > plat->dma_cfg->pblx8 = true; > > > > > > plat->multicast_filter_bins = 256; > > > > > + plat->mdio_bus_data->phy_mask = 0; > > > + > > > > And this change isn't related to the GNET. Why is it here? > > > > > plat->clk_ref_rate = 125000000; > > > plat->clk_ptp_rate = 125000000; > > > > > > @@ -69,13 +132,342 @@ static struct stmmac_pci_info loongson_gmac_pci_info = { > > > .setup = loongson_gmac_data, > > > }; > > > > > > -static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > +static void loongson_gnet_dma_init_channel(struct stmmac_priv *priv, > > > + void __iomem *ioaddr, > > > + struct stmmac_dma_cfg *dma_cfg, > > > + u32 chan) > > > +{ > > > + int txpbl = dma_cfg->txpbl ?: dma_cfg->pbl; > > > + int rxpbl = dma_cfg->rxpbl ?: dma_cfg->pbl; > > > + u32 value; > > > + > > > > > + /* common channel control register config */ > > > > Useless comment. Please drop. > > > > > + value = readl(ioaddr + DMA_CHAN_BUS_MODE(chan)); > > > + > > > > > + /* Set the DMA PBL (Programmable Burst Length) mode. > > > + * > > > + * Note: before stmmac core 3.50 this mode bit was 4xPBL, and > > > + * post 3.5 mode bit acts as 8*PBL. > > > + */ > > > > Do you have an IP-core older than v3.50a? No. So please drop the > > comment. > > > > > + if (dma_cfg->pblx8) > > > + value |= DMA_BUS_MODE_MAXPBL; > > > + > > > + value |= DMA_BUS_MODE_USP; > > > + value &= ~(DMA_BUS_MODE_PBL_MASK | DMA_BUS_MODE_RPBL_MASK); > > > + value |= (txpbl << DMA_BUS_MODE_PBL_SHIFT); > > > + value |= (rxpbl << DMA_BUS_MODE_RPBL_SHIFT); > > > + > > > + /* Set the Fixed burst mode */ > > > + if (dma_cfg->fixed_burst) > > > + value |= DMA_BUS_MODE_FB; > > > + > > > + /* Mixed Burst has no effect when fb is set */ > > > + if (dma_cfg->mixed_burst) > > > + value |= DMA_BUS_MODE_MB; > > > + > > > + if (dma_cfg->atds) > > > + value |= DMA_BUS_MODE_ATDS; > > > + > > > + if (dma_cfg->aal) > > > + value |= DMA_BUS_MODE_AAL; > > > + > > > + writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan)); > > > + > > > + /* Mask interrupts by writing to CSR7 */ > > > + writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + > > > + DMA_CHAN_INTR_ENA(chan)); > > > +} > > > + > > > +static int loongson_gnet_dma_interrupt(struct stmmac_priv *priv, > > > + void __iomem *ioaddr, > > > + struct stmmac_extra_stats *x, > > > + u32 chan, u32 dir) > > > +{ > > > + struct stmmac_pcpu_stats *stats = this_cpu_ptr(priv->xstats.pcpu_stats); > > > + u32 abnor_intr_status; > > > + u32 nor_intr_status; > > > + u32 fb_intr_status; > > > + u32 intr_status; > > > + int ret = 0; > > > + > > > + /* read the status register (CSR5) */ > > > + intr_status = readl(ioaddr + DMA_CHAN_STATUS(chan)); > > > + > > > + if (dir == DMA_DIR_RX) > > > + intr_status &= DMA_STATUS_MSK_RX_LOONGSON; > > > + else if (dir == DMA_DIR_TX) > > > + intr_status &= DMA_STATUS_MSK_TX_LOONGSON; > > > + > > > + nor_intr_status = intr_status & (DMA_STATUS_NIS_TX_LOONGSON | > > > + DMA_STATUS_NIS_RX_LOONGSON); > > > + abnor_intr_status = intr_status & (DMA_STATUS_AIS_TX_LOONGSON | > > > + DMA_STATUS_AIS_RX_LOONGSON); > > > + fb_intr_status = intr_status & (DMA_STATUS_FBI_TX_LOONGSON | > > > + DMA_STATUS_FBI_RX_LOONGSON); > > > + > > > + /* ABNORMAL interrupts */ > > > + if (unlikely(abnor_intr_status)) { > > > + if (unlikely(intr_status & DMA_STATUS_UNF)) { > > > + ret = tx_hard_error_bump_tc; > > > + x->tx_undeflow_irq++; > > > + } > > > + if (unlikely(intr_status & DMA_STATUS_TJT)) > > > + x->tx_jabber_irq++; > > > + if (unlikely(intr_status & DMA_STATUS_OVF)) > > > + x->rx_overflow_irq++; > > > + if (unlikely(intr_status & DMA_STATUS_RU)) > > > + x->rx_buf_unav_irq++; > > > + if (unlikely(intr_status & DMA_STATUS_RPS)) > > > + x->rx_process_stopped_irq++; > > > + if (unlikely(intr_status & DMA_STATUS_RWT)) > > > + x->rx_watchdog_irq++; > > > + if (unlikely(intr_status & DMA_STATUS_ETI)) > > > + x->tx_early_irq++; > > > + if (unlikely(intr_status & DMA_STATUS_TPS)) { > > > + x->tx_process_stopped_irq++; > > > + ret = tx_hard_error; > > > + } > > > + if (unlikely(fb_intr_status)) { > > > + x->fatal_bus_error_irq++; > > > + ret = tx_hard_error; > > > + } > > > + } > > > + /* TX/RX NORMAL interrupts */ > > > + if (likely(nor_intr_status)) { > > > + if (likely(intr_status & DMA_STATUS_RI)) { > > > + u32 value = readl(ioaddr + DMA_INTR_ENA); > > > + /* to schedule NAPI on real RIE event. */ > > > + if (likely(value & DMA_INTR_ENA_RIE)) { > > > + u64_stats_update_begin(&stats->syncp); > > > + u64_stats_inc(&stats->rx_normal_irq_n[chan]); > > > + u64_stats_update_end(&stats->syncp); > > > + ret |= handle_rx; > > > + } > > > + } > > > + if (likely(intr_status & DMA_STATUS_TI)) { > > > + u64_stats_update_begin(&stats->syncp); > > > + u64_stats_inc(&stats->tx_normal_irq_n[chan]); > > > + u64_stats_update_end(&stats->syncp); > > > + ret |= handle_tx; > > > + } > > > + if (unlikely(intr_status & DMA_STATUS_ERI)) > > > + x->rx_early_irq++; > > > + } > > > + /* Optional hardware blocks, interrupts should be disabled */ > > > + if (unlikely(intr_status & > > > + (DMA_STATUS_GPI | DMA_STATUS_GMI | DMA_STATUS_GLI))) > > > + pr_warn("%s: unexpected status %08x\n", __func__, intr_status); > > > + > > > + /* Clear the interrupt by writing a logic 1 to the CSR5[15-0] */ > > > + writel((intr_status & 0x7ffff), ioaddr + DMA_CHAN_STATUS(chan)); > > > + > > > + return ret; > > > +} > > > + > > > +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, > > > + unsigned int mode) > > > +{ > > > + struct loongson_data *ld = (struct loongson_data *)priv; > > > + struct net_device *ndev = dev_get_drvdata(ld->dev); > > > + struct stmmac_priv *ptr = netdev_priv(ndev); > > > + > > > + /* The controller and PHY don't work well together. > > > + * We need to use the PS bit to check if the controller's status > > > + * is correct and reset PHY if necessary. > > > + * MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. > > > + */ > > > + if (speed == SPEED_1000) { > > > + if (readl(ptr->ioaddr + MAC_CTRL_REG) & > > > > > + MAC_CTRL_PORT_SELECT_10_100) > > > > GMAC_CONTROL_PS? > > > > > + /* Word around hardware bug, restart autoneg */ > > > + phy_restart_aneg(ndev->phydev); > > > + } > > > +} > > > + > > > +static int loongson_gnet_data(struct pci_dev *pdev, > > > + struct plat_stmmacenet_data *plat) > > > +{ > > > + loongson_default_data(pdev, plat); > > > + > > > + plat->phy_addr = -1; > > > + plat->phy_interface = PHY_INTERFACE_MODE_GMII; > > > + > > > + plat->dma_cfg->pbl = 32; > > > + plat->dma_cfg->pblx8 = true; > > > + > > > + plat->multicast_filter_bins = 256; > > > + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); > > > + > > > > > + plat->clk_ref_rate = 125000000; > > > + plat->clk_ptp_rate = 125000000; > > > > If this is common for both GMAC and GNET what about moving it to the > > loongson_default_data() method? > > > > If so please just do that in the patch 4 where you get to add these > > fields initialization in the first place. > > > > > + > > > + plat->fix_mac_speed = loongson_gnet_fix_speed; > > > + > > > + return 0; > > > +} > > > + > > > +static struct stmmac_pci_info loongson_gnet_pci_info = { > > > + .setup = loongson_gnet_data, > > > +}; > > > + > > > +static int loongson_dwmac_config_legacy(struct pci_dev *pdev, > > > + struct plat_stmmacenet_data *plat, > > > + struct stmmac_resources *res, > > > + struct device_node *np) > > > +{ > > > + if (np) { > > > + res->irq = of_irq_get_byname(np, "macirq"); > > > + if (res->irq < 0) { > > > + dev_err(&pdev->dev, "IRQ macirq not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); > > > + if (res->wol_irq < 0) { > > > + dev_info(&pdev->dev, > > > + "IRQ eth_wake_irq not found, using macirq\n"); > > > + res->wol_irq = res->irq; > > > + } > > > + > > > + res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); > > > + if (res->lpi_irq < 0) { > > > + dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); > > > + return -ENODEV; > > > + } > > > + } else { > > > + res->irq = pdev->irq; > > > + res->wol_irq = res->irq; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > > > + struct plat_stmmacenet_data *plat, > > > + struct stmmac_resources *res, > > > + struct device_node *np) > > > +{ > > > + int i, ret, vecs; > > > + > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > > > + if (ret < 0) { > > > + dev_info(&pdev->dev, > > > + "MSI enable failed, Fallback to legacy interrupt\n"); > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > > > + } > > > + > > > + res->irq = pci_irq_vector(pdev, 0); > > > + res->wol_irq = 0; > > > + > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > + * --------- ----- -------- -------- ... -------- -------- > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > + */ > > > + for (i = 0; i < CHANNEL_NUM; i++) { > > > + res->rx_irq[CHANNEL_NUM - 1 - i] = > > > + pci_irq_vector(pdev, 1 + i * 2); > > > + res->tx_irq[CHANNEL_NUM - 1 - i] = > > > + pci_irq_vector(pdev, 2 + i * 2); > > > + } > > > + > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > Once again. Please replace this with simpler solution: > In full PCI system the below function works fine, because alloc irq > vectors with PCI_IRQ_LEGACY do the same thing as fallback to call > loongson_dwmac_config_legacy(). But for a DT-based system it doesn't > work. > > Huacai > > > > static int loongson_dwmac_config_multi_msi(struct pci_dev *pdev, > > + struct plat_stmmacenet_data *plat, > > + struct stmmac_resources *res) > > +{ > > + int i, ret, vecs; > > + > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > + * --------- ----- -------- -------- ... -------- -------- > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > + */ > > + vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; > > + ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_LEGACY); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); > > + return ret; > > + } else if (ret >= vecs) { > > + for (i = 0; i < plat->rx_queues_to_use; i++) { > > + res->rx_irq[CHANNELS_NUM - 1 - i] = > > + pci_irq_vector(pdev, 1 + i * 2); > > + } > > + for (i = 0; i < plat->tx_queues_to_use; i++) { > > + res->tx_irq[CHANNELS_NUM - 1 - i] = > > + pci_irq_vector(pdev, 2 + i * 2); > > + } > > + > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > + } > > + > > + res->irq = pci_irq_vector(pdev, 0); > > + > > + return 0; > > +} > > > > > > > + > > > + return 0; > > > +} > > > + > > > +static struct mac_device_info *loongson_dwmac_setup(void *apriv) > > > +{ > > > + struct stmmac_priv *priv = apriv; > > > + struct stmmac_resources res; > > > + struct mac_device_info *mac; > > > + struct stmmac_dma_ops *dma; > > > + struct pci_dev *pdev; > > > + u32 loongson_gmac; > > > + > > > + memset(&res, 0, sizeof(res)); > > > + pdev = to_pci_dev(priv->device); > > > + res.addr = pcim_iomap_table(pdev)[0]; > > > + loongson_gmac = readl(res.addr + GMAC_VERSION) & 0xff; > > > + > > > + mac = devm_kzalloc(priv->device, sizeof(*mac), GFP_KERNEL); > > > + if (!mac) > > > + return NULL; > > > + > > > + dma = devm_kzalloc(priv->device, sizeof(*dma), GFP_KERNEL); > > > + if (!dma) > > > + return NULL; > > > + > > > + /* The original IP-core version is 0x37 in all Loongson GNET > > > + * (ls2k2000 and ls7a2000), but the GNET HW designers have changed the > > > + * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the Loongson > > > + * ls2k2000 MAC to emphasize the differences: multiple DMA-channels, > > > + * AV feature and GMAC_INT_STATUS CSR flags layout. Get back the > > > + * original value so the correct HW-interface would be selected. > > > + */ > > > + if (loongson_gmac == LOONGSON_DWMAC_CORE_1_00) { > > > + priv->synopsys_id = DWMAC_CORE_3_70; > > > + *dma = dwmac1000_dma_ops; > > > + dma->init_chan = loongson_gnet_dma_init_channel; > > > + dma->dma_interrupt = loongson_gnet_dma_interrupt; > > > + mac->dma = dma; > > > + } > > > + > > > + mac->mac = &dwmac1000_ops; > > > + priv->dev->priv_flags |= IFF_UNICAST_FLT; > > > + > > > + /* Pre-initialize the respective "mac" fields as it's done in > > > + * dwmac1000_setup() > > > + */ > > > + mac->pcsr = priv->ioaddr; > > > + mac->multicast_filter_bins = priv->plat->multicast_filter_bins; > > > + mac->unicast_filter_entries = priv->plat->unicast_filter_entries; > > > + mac->mcast_bits_log2 = 0; > > > + > > > + if (mac->multicast_filter_bins) > > > + mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins); > > > + > > > + /* The GMAC devices with PCI ID 0x7a03 does not support any pause mode. > > > + * The GNET devices without CORE ID 0x10 does not support half-duplex. > > > + */ > > > + if (pdev->device == PCI_DEVICE_ID_LOONGSON_GMAC) { > > > + mac->link.caps = MAC_10 | MAC_100 | MAC_1000; > > > + } else { > > > + if (loongson_gmac == LOONGSON_DWMAC_CORE_1_00) > > > + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > > + MAC_10 | MAC_100 | MAC_1000; > > > + else > > > + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > > + MAC_10FD | MAC_100FD | MAC_1000FD; > > > + } > > > + > > > + mac->link.duplex = GMAC_CONTROL_DM; > > > + mac->link.speed10 = GMAC_CONTROL_PS; > > > + mac->link.speed100 = GMAC_CONTROL_PS | GMAC_CONTROL_FES; > > > + mac->link.speed1000 = 0; > > > + mac->link.speed_mask = GMAC_CONTROL_PS | GMAC_CONTROL_FES; > > > + mac->mii.addr = GMAC_MII_ADDR; > > > + mac->mii.data = GMAC_MII_DATA; > > > + mac->mii.addr_shift = 11; > > > + mac->mii.addr_mask = 0x0000F800; > > > + mac->mii.reg_shift = 6; > > > + mac->mii.reg_mask = 0x000007C0; > > > + mac->mii.clk_csr_shift = 2; > > > + mac->mii.clk_csr_mask = GENMASK(5, 2); > > > + > > > + return mac; > > > +} > > > + > > > +static int loongson_dwmac_probe(struct pci_dev *pdev, > > > + const struct pci_device_id *id) > > > { > > > struct plat_stmmacenet_data *plat; > > > int ret, i, bus_id, phy_mode; > > > struct stmmac_pci_info *info; > > > struct stmmac_resources res; > > > + struct loongson_data *ld; > > > struct device_node *np; > > > + u32 loongson_gmac; > > > > > > plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL); > > > if (!plat) > > > @@ -88,10 +482,14 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > return -ENOMEM; > > > > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > - if (!plat->dma_cfg) { > > > - ret = -ENOMEM; > > > - goto err_put_node; > > > - } > > > + if (!plat->dma_cfg) > > > + return -ENOMEM; > > > + > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > + if (!ld) > > > + return -ENOMEM; > > > + > > > > > + np = dev_of_node(&pdev->dev); > > > > Here is the missing np from the patch 5... > > > > > > > > /* Enable pci device */ > > > ret = pci_enable_device(pdev); > > > @@ -110,14 +508,6 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > break; > > > } > > > > > > > > - phy_mode = device_get_phy_mode(&pdev->dev); > > > - if (phy_mode < 0) { > > > - dev_err(&pdev->dev, "phy_mode not found\n"); > > > - ret = phy_mode; > > > - goto err_disable_device; > > > - } > > > - > > > - plat->phy_interface = phy_mode; > > > > This change must have been added to the Patch 5 where you get to move > > all the NP-related things into the "if (np) {}" clause! > > > > > plat->mac_interface = PHY_INTERFACE_MODE_GMII; > > > > > > pci_set_master(pdev); > > > @@ -133,7 +523,6 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > dev_info(&pdev->dev, "Found MDIO subnode\n"); > > > plat->mdio_bus_data->needs_reset = true; > > > } > > > - > > > bus_id = of_alias_get_id(np, "ethernet"); > > > if (bus_id >= 0) > > > plat->bus_id = bus_id; > > > @@ -145,42 +534,49 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > goto err_disable_device; > > > } > > > plat->phy_interface = phy_mode; > > > > > + } > > > > > > - res.irq = of_irq_get_byname(np, "macirq"); > > > - if (res.irq < 0) { > > > - dev_err(&pdev->dev, "IRQ macirq not found\n"); > > > - ret = -ENODEV; > > > - goto err_disable_msi; > > > - } > > > + plat->bsp_priv = ld; > > > + plat->setup = loongson_dwmac_setup; > > > + ld->dev = &pdev->dev; > > > > > > - res.wol_irq = of_irq_get_byname(np, "eth_wake_irq"); > > > - if (res.wol_irq < 0) { > > > - dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n"); > > > - res.wol_irq = res.irq; > > > - } > > > + memset(&res, 0, sizeof(res)); > > > + res.addr = pcim_iomap_table(pdev)[0]; > > > + loongson_gmac = readl(res.addr + GMAC_VERSION) & 0xff; > > > > > > - res.lpi_irq = of_irq_get_byname(np, "eth_lpi"); > > > - if (res.lpi_irq < 0) { > > > - dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); > > > - ret = -ENODEV; > > > - goto err_disable_msi; > > > - } > > > - } else { > > > - res.irq = pdev->irq; > > > > Please, move the loongson_dwmac_config_legacy() method creation change > > into a preparation/pre-requisite patch. That will vastly simplify this > > patch and will help with reviewing it! > > > > > + switch (loongson_gmac) { > > > + case LOONGSON_DWMAC_CORE_1_00: > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > + > > > + /* Only channel 0 supports checksum, > > > + * so turn off checksum to enable multiple channels. > > > + */ > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > + > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > + break; > > > + default: /* 0x35 device and 0x37 device. */ > > > + plat->tx_queues_to_use = 1; > > > + plat->rx_queues_to_use = 1; > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > + break; > > > } > > > > > > - pci_enable_msi(pdev); > > > - memset(&res, 0, sizeof(res)); > > > - res.addr = pcim_iomap_table(pdev)[0]; > > > > > + /* GNET devices with dev revision 0x00 do not support manually > > > + * setting the speed to 1000. > > > + */ > > > + if (pdev->device == PCI_DEVICE_ID_LOONGSON_GNET && > > > + pdev->revision == 0x00) > > > + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000; > > > > Move this to the loongson_gnet_data() method. > > > > > > > > ret = stmmac_dvr_probe(&pdev->dev, plat, &res); > > > if (ret) > > > - goto err_disable_msi; > > > + goto err_disable_device; > > > > > > return ret; > > > > > > -err_disable_msi: > > > - pci_disable_msi(pdev); > > > err_disable_device: > > > pci_disable_device(pdev); > > > err_put_node: > > > @@ -248,6 +644,7 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, > > > > > > static const struct pci_device_id loongson_dwmac_id_table[] = { > > > { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, > > > + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, > > > {} > > > }; > > > MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table); > > > > > @@ -266,4 +663,5 @@ module_pci_driver(loongson_dwmac_driver); > > > > > > MODULE_DESCRIPTION("Loongson DWMAC PCI driver"); > > > MODULE_AUTHOR("Qing Zhang "); > > > +MODULE_AUTHOR("Yanteng Si "); > > > MODULE_LICENSE("GPL v2"); > > > > Please move this into a separate patch. It can be the last patch in > > the series. > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c > > > index f161ec9ac490..66c0c22908b1 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c > > > @@ -296,3 +296,4 @@ const struct stmmac_dma_ops dwmac1000_dma_ops = { > > > .get_hw_feature = dwmac1000_get_hw_feature, > > > .rx_watchdog = dwmac1000_rx_watchdog, > > > }; > > > +EXPORT_SYMBOL_GPL(dwmac1000_dma_ops); > > > > > > Please move this into a preparation/pre-requisite patch. > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > > index e1537a57815f..e94faa72f30e 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > > @@ -420,6 +420,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev, > > > return 0; > > > } > > > > > > + if (priv->plat->flags & STMMAC_FLAG_DISABLE_FORCE_1000) { > > > + if (cmd->base.speed == SPEED_1000 && > > > + cmd->base.autoneg != AUTONEG_ENABLE) > > > + return -EOPNOTSUPP; > > > + } > > > + > > > return phylink_ethtool_ksettings_set(priv->phylink, cmd); > > > } > > > > > > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > > > index 1b54b84a6785..c5d3d0ddb6f8 100644 > > > --- a/include/linux/stmmac.h > > > +++ b/include/linux/stmmac.h > > > @@ -223,6 +223,7 @@ struct dwmac4_addrs { > > > #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI BIT(10) > > > #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11) > > > #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12) > > > +#define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13) > > > > > > struct plat_stmmacenet_data { > > > int bus_id; > > > > Please move this in a separate _pre-requisite/preparation_ patch. > > Really. Why have you merged it into this one? This patch already got > > to be too complicated. Don't make it even more complex. > > > > --- > > > > I look at v8 and look at v9 series. Why have you re-shuffled the > > change so significantly??? It has made my life in reviewing your bits > > much harder because instead of checking whether you took my v8-notes > > into account I had to once again analyze the entire series. Sigh... > > > > -Serge(y) > > > > > -- > > > 2.31.4 > > > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!