| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 21 Dec 2023 05:14:43 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com,
alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com,
chenhuacai@...ngson.cn, linux@...linux.org.uk, guyinggang@...ngson.cn,
netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v7 5/9] net: stmmac: Add Loongson-specific
register definitions
On Tue, Dec 19, 2023 at 10:26:45PM +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.
Thanks for the updated patch. It looks much clearer now. Thanks to
that I came up with a better and less invasive solution. See my last
comment for details.
>
> Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
> Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
> ---
> drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 2 ++
> .../ethernet/stmicro/stmmac/dwmac1000_dma.c | 10 ++++--
> .../net/ethernet/stmicro/stmmac/dwmac_dma.h | 35 +++++++++++++++++++
> .../net/ethernet/stmicro/stmmac/dwmac_lib.c | 35 +++++++++++++++----
> drivers/net/ethernet/stmicro/stmmac/hwif.c | 3 +-
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
> include/linux/stmmac.h | 1 +
> 8 files changed, 78 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index 721c1f8e892f..48ab21243b26 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -34,6 +34,7 @@
> #define DWMAC_CORE_5_00 0x50
> #define DWMAC_CORE_5_10 0x51
> #define DWMAC_CORE_5_20 0x52
> +#define DWLGMAC_CORE_1_00 0x10
This doesn't look correct because these IDs have been defined for the
Synopsys IP-core enumerations. Loongson GNET is definitely based on
some DW GMAC v3.x IP-core, but instead of updating the USERVER field
vendor just overwrote the GMAC_VERSION.SNPSVER field. From that
perspective the correct solution would be to override the
priv->synopsys_id field with a correct IP-core version (0x37?). See my
last comment for details of how to do that.
> #define DWXGMAC_CORE_2_10 0x21
> #define DWXGMAC_CORE_2_20 0x22
> #define DWXLGMAC_CORE_2_00 0x20
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index 0d79104d7fd3..fb7506bbc21b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -101,6 +101,8 @@ static int loongson_dwmac_probe(struct pci_dev *pdev,
> plat->mdio_bus_data->needs_reset = true;
> }
>
> + plat->flags |= STMMAC_FLAG_HAS_LGMAC;
> +
If what I suggest in the last comment is implemented this will be
unnecessary.
> /* Enable pci device */
> ret = pci_enable_device(pdev);
> if (ret) {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> index 0fb48e683970..a01fe6b7540a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
None of the generic parts (dwmac1000_dma.c, dwmac_dma.h, dwmac_lib.c)
will need to be modified if you implement what I suggest in the last
comment.
> @@ -118,7 +118,10 @@ static void dwmac1000_dma_init(struct stmmac_priv *priv, void __iomem *ioaddr,
See my comment to patch 4/9. This method can be fully dropped in favor
of having the dwmac1000_dma_init_channel() function implemented.
> u32 dma_intr_mask;
>
> /* Mask interrupts by writing to CSR7 */
> - dma_intr_mask = DMA_INTR_DEFAULT_MASK;
> + if (priv->plat->flags & STMMAC_FLAG_HAS_LGMAC)
> + dma_intr_mask = DMA_INTR_DEFAULT_MASK_LOONGSON;
> + else
> + dma_intr_mask = DMA_INTR_DEFAULT_MASK;
>
> dma_config(ioaddr + DMA_BUS_MODE, ioaddr + DMA_INTR_ENA,
> dma_cfg, dma_intr_mask, atds);
> @@ -130,7 +133,10 @@ static void dwmac1000_dma_init_channel(struct stmmac_priv *priv, void __iomem *i
> u32 dma_intr_mask;
>
> /* Mask interrupts by writing to CSR7 */
> - dma_intr_mask = DMA_INTR_DEFAULT_MASK;
> + if (priv->plat->flags & STMMAC_FLAG_HAS_LGMAC)
> + dma_intr_mask = DMA_INTR_DEFAULT_MASK_LOONGSON;
> + else
> + dma_intr_mask = DMA_INTR_DEFAULT_MASK;
>
> if (dma_cfg->multi_msi_en)
> dma_config(ioaddr + DMA_CHAN_BUS_MODE(chan),
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> index 395d5e4c3922..7d33798c0e72 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> @@ -70,16 +70,23 @@
> #define DMA_CONTROL_SR 0x00000002 /* Start/Stop Receive */
>
> /* DMA Normal interrupt */
> +#define DMA_INTR_ENA_NIE_TX_LOONGSON 0x00040000 /* Normal Loongson Tx Summary */
> +#define DMA_INTR_ENA_NIE_RX_LOONGSON 0x00020000 /* Normal Loongson Rx Summary */
> #define DMA_INTR_ENA_NIE 0x00010000 /* Normal Summary */
> #define DMA_INTR_ENA_TIE 0x00000001 /* Transmit Interrupt */
> #define DMA_INTR_ENA_TUE 0x00000004 /* Transmit Buffer Unavailable */
> #define DMA_INTR_ENA_RIE 0x00000040 /* Receive Interrupt */
> #define DMA_INTR_ENA_ERE 0x00004000 /* Early Receive */
>
> +#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)
> #define DMA_INTR_NORMAL (DMA_INTR_ENA_NIE | DMA_INTR_ENA_RIE | \
> DMA_INTR_ENA_TIE)
>
> /* DMA Abnormal interrupt */
> +#define DMA_INTR_ENA_AIE_TX_LOONGSON 0x00010000 /* Abnormal Loongson Tx Summary */
> +#define DMA_INTR_ENA_AIE_RX_LOONGSON 0x00008000 /* Abnormal Loongson Rx Summary */
> #define DMA_INTR_ENA_AIE 0x00008000 /* Abnormal Summary */
> #define DMA_INTR_ENA_FBE 0x00002000 /* Fatal Bus Error */
> #define DMA_INTR_ENA_ETE 0x00000400 /* Early Transmit */
> @@ -91,10 +98,14 @@
> #define DMA_INTR_ENA_TJE 0x00000008 /* Transmit Jabber */
> #define DMA_INTR_ENA_TSE 0x00000002 /* Transmit Stopped */
>
> +#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_ABNORMAL (DMA_INTR_ENA_AIE | DMA_INTR_ENA_FBE | \
> DMA_INTR_ENA_UNE)
>
> /* DMA default interrupt mask */
> +#define DMA_INTR_DEFAULT_MASK_LOONGSON (DMA_INTR_NORMAL_LOONGSON | DMA_INTR_ABNORMAL_LOONGSON)
> #define DMA_INTR_DEFAULT_MASK (DMA_INTR_NORMAL | DMA_INTR_ABNORMAL)
> #define DMA_INTR_DEFAULT_RX (DMA_INTR_ENA_RIE)
> #define DMA_INTR_DEFAULT_TX (DMA_INTR_ENA_TIE)
> @@ -111,9 +122,15 @@
> #define DMA_STATUS_TS_SHIFT 20
> #define DMA_STATUS_RS_MASK 0x000e0000 /* Receive Process State */
> #define DMA_STATUS_RS_SHIFT 17
> +#define DMA_STATUS_NIS_TX_LOONGSON 0x00040000 /* Normal Loongson Tx Interrupt Summary */
> +#define DMA_STATUS_NIS_RX_LOONGSON 0x00020000 /* Normal Loongson Rx Interrupt Summary */
> #define DMA_STATUS_NIS 0x00010000 /* Normal Interrupt Summary */
> +#define DMA_STATUS_AIS_TX_LOONGSON 0x00010000 /* Abnormal Loongson Tx Interrupt Summary */
> +#define DMA_STATUS_AIS_RX_LOONGSON 0x00008000 /* Abnormal Loongson Rx Interrupt Summary */
> #define DMA_STATUS_AIS 0x00008000 /* Abnormal Interrupt Summary */
> #define DMA_STATUS_ERI 0x00004000 /* Early Receive Interrupt */
> +#define DMA_STATUS_FBI_TX_LOONGSON 0x00002000 /* Fatal Loongson Tx Bus Error Interrupt */
> +#define DMA_STATUS_FBI_RX_LOONGSON 0x00001000 /* Fatal Loongson Rx Bus Error Interrupt */
> #define DMA_STATUS_FBI 0x00002000 /* Fatal Bus Error Interrupt */
> #define DMA_STATUS_ETI 0x00000400 /* Early Transmit Interrupt */
> #define DMA_STATUS_RWT 0x00000200 /* Receive Watchdog Timeout */
> @@ -128,10 +145,21 @@
> #define DMA_STATUS_TI 0x00000001 /* Transmit Interrupt */
> #define DMA_CONTROL_FTF 0x00100000 /* Flush transmit FIFO */
>
> +#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_COMMON (DMA_STATUS_NIS | \
> DMA_STATUS_AIS | \
> DMA_STATUS_FBI)
>
> +#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_RX (DMA_STATUS_ERI | \
> DMA_STATUS_RWT | \
> DMA_STATUS_RPS | \
> @@ -140,6 +168,13 @@
> DMA_STATUS_OVF | \
> DMA_STATUS_MSK_COMMON)
>
> +#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 DMA_STATUS_MSK_TX (DMA_STATUS_ETI | \
> DMA_STATUS_UNF | \
> DMA_STATUS_TJT | \
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c b/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
> index 968801c694e9..a6e2ab4d0f4a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
> @@ -167,6 +167,9 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
> struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[chan];
> int ret = 0;
> /* read the status register (CSR5) */
> + u32 nor_intr_status;
> + u32 abnor_intr_status;
> + u32 fb_intr_status;
> u32 intr_status = readl(ioaddr + DMA_CHAN_STATUS(chan));
>
> #ifdef DWMAC_DMA_DEBUG
> @@ -176,13 +179,31 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
> show_rx_process_state(intr_status);
> #endif
>
> - if (dir == DMA_DIR_RX)
> - intr_status &= DMA_STATUS_MSK_RX;
> - else if (dir == DMA_DIR_TX)
> - intr_status &= DMA_STATUS_MSK_TX;
> + if (priv->plat->flags & STMMAC_FLAG_HAS_LGMAC) {
> + 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);
> + } else {
> + if (dir == DMA_DIR_RX)
> + intr_status &= DMA_STATUS_MSK_RX;
> + else if (dir == DMA_DIR_TX)
> + intr_status &= DMA_STATUS_MSK_TX;
> +
> + nor_intr_status = intr_status & DMA_STATUS_NIS;
> + abnor_intr_status = intr_status & DMA_STATUS_AIS;
> + fb_intr_status = intr_status & DMA_STATUS_FBI;
> + }
>
> /* ABNORMAL interrupts */
> - if (unlikely(intr_status & DMA_STATUS_AIS)) {
> + if (unlikely(abnor_intr_status)) {
> if (unlikely(intr_status & DMA_STATUS_UNF)) {
> ret = tx_hard_error_bump_tc;
> x->tx_undeflow_irq++;
> @@ -205,13 +226,13 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
> x->tx_process_stopped_irq++;
> ret = tx_hard_error;
> }
> - if (unlikely(intr_status & DMA_STATUS_FBI)) {
> + if (unlikely(intr_status & fb_intr_status)) {
> x->fatal_bus_error_irq++;
> ret = tx_hard_error;
> }
> }
> /* TX/RX NORMAL interrupts */
> - if (likely(intr_status & DMA_STATUS_NIS)) {
> + 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. */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> index 1bd34b2a47e8..3724cf698de6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> @@ -59,7 +59,8 @@ static int stmmac_dwmac1_quirks(struct stmmac_priv *priv)
> dev_info(priv->device, "Enhanced/Alternate descriptors\n");
>
> /* GMAC older than 3.50 has no extended descriptors */
> - if (priv->synopsys_id >= DWMAC_CORE_3_50) {
> + if (priv->synopsys_id >= DWMAC_CORE_3_50 ||
> + priv->synopsys_id == DWLGMAC_CORE_1_00) {
This could be left as is if the priv->synopsys_id field is overwritten
with a correct value (see my last comment).
> dev_info(priv->device, "Enabled extended descriptors\n");
> priv->extend_desc = 1;
> } else {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d868eb8dafc5..9764d2ab7e46 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7218,6 +7218,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
> * riwt_off field from the platform.
> */
> if (((priv->synopsys_id >= DWMAC_CORE_3_50) ||
> + (priv->synopsys_id == DWLGMAC_CORE_1_00) ||
the same comment here.
> (priv->plat->has_xgmac)) && (!priv->plat->riwt_off)) {
> priv->use_riwt = 1;
> dev_info(priv->device,
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index dee5ad6e48c5..f07f79d50b06 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -221,6 +221,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_HAS_LGMAC BIT(13)
Seeing the patch in the current state would overcomplicate the generic
code and the only functions you need to update are
dwmac_dma_interrupt()
dwmac1000_dma_init_channel()
you can have these methods re-defined with all the Loongson GNET
specifics in the low-level platform driver (dwmac-loongson.c). After
that you can just override the mac_device_info.dma pointer with a
fixed stmmac_dma_ops descriptor. Here is what should be done for that:
1. Keep the Patch 4/9 with my comments fixed. First it will be partly
useful for your GNET device. Second in general it's a correct
implementation of the normal DW GMAC v3.x multi-channels feature and
will be useful for the DW GMACs with that feature enabled.
2. Create the Loongson GNET-specific
stmmac_dma_ops.dma_interrupt()
stmmac_dma_ops.init_chan()
methods in the dwmac-loongson.c driver. Don't forget to move all the
Loongson-specific macros from dwmac_dma.h to dwmac-loongson.c.
3. 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.
+ call dwmac1000_setup() or initialize mac_device_info in a way
it's done in dwmac1000_setup() (the later might be better so you
wouldn't need to export the dwmac1000_setup() function).
+ override stmmac_priv.synopsys_id with a correct value.
4. Initialize plat_stmmacenet_data.setup() with the pointer to the
method created in 3.
If I didn't miss something stmmac_hwif_init() will call the
platform-specific setup method right after fetching the SNPSVER field
value. Your setup method will allocate mac_device_info structure then,
will pre-initialize the GNET-specific DMA ops field and fix the
Synopsys ID with a proper value (as described in 3 above). The rest of
the ops descriptors will be initialized in the loop afterwards based
on the specified device ID.
-Serge(y)
>
> struct plat_stmmacenet_data {
> int bus_id;
> --
> 2.31.4
>
Powered by blists - more mailing lists