[PATCH] net: dwc_eth_qos: Revert some changes of commit 3a97da12ee7b
Revert some changes of commit 3a97da12ee7b ("net: dwc_eth_qos: add dwc
eqos for imx support") that were probably added by mistake.
One of these changes can lead to received data corruption (enabling
FUP and FEP bits). Another causes invalid register rxq_ctrl0 settings
for some platforms. And another makes some writes at unknown memory
location.
Fixes: 3a97da12ee7b ("net: dwc_eth_qos: add dwc eqos for imx support")
Signed-off-by: Daniil Stas
Cc: Ye Li
Cc: Fugang Duan
Cc: Peng Fan
Cc: Ramon Fried
Cc: Joe Hershberger
Cc: Patrice Chotard
Cc: Patrick Delaunay
---
drivers/net/dwc_eth_qos.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 2f088c758f..b012bed517 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -172,8 +172,6 @@ struct eqos_mtl_regs {
#define EQOS_MTL_RXQ0_OPERATION_MODE_RFA_MASK 0x3f
#define EQOS_MTL_RXQ0_OPERATION_MODE_EHFC BIT(7)
#define EQOS_MTL_RXQ0_OPERATION_MODE_RSF BIT(5)
-#define EQOS_MTL_RXQ0_OPERATION_MODE_FEP BIT(4)
-#define EQOS_MTL_RXQ0_OPERATION_MODE_FUP BIT(3)
#define EQOS_MTL_RXQ0_DEBUG_PRXQ_SHIFT 16
#define EQOS_MTL_RXQ0_DEBUG_PRXQ_MASK 0x7fff
@@ -1222,7 +1220,6 @@ static int eqos_start(struct udevice *dev)
}
/* Configure MTL */
- writel(0x60, >mtl_regs->txq0_quantum_weight - 0x100);
/* Enable Store and Forward mode for TX */
/* Program Tx operating mode */
@@ -1236,9 +1233,7 @@ static int eqos_start(struct udevice *dev)
/* Enable Store and Forward mode for RX, since no jumbo frame */
setbits_le32(>mtl_regs->rxq0_operation_mode,
-EQOS_MTL_RXQ0_OPERATION_MODE_RSF |
-EQOS_MTL_RXQ0_OPERATION_MODE_FEP |
-EQOS_MTL_RXQ0_OPERATION_MODE_FUP);
+EQOS_MTL_RXQ0_OPERATION_MODE_RSF);
/* Transmit/Receive queue fifo size; use all RAM for 1 queue */
val = readl(>mac_regs->hw_feature1);
@@ -1314,12 +1309,6 @@ static int eqos_start(struct udevice *dev)
eqos->config->config_mac <<
EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT);
- clrsetbits_le32(>mac_regs->rxq_ctrl0,
- EQOS_MAC_RXQ_CTRL0_RXQ0EN_MASK <<
- EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT,
- 0x2 <<
- EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT);
-
/* Multicast and Broadcast Queue Enable */
setbits_le32(>mac_regs->unused_0a4,
0x0010);
--
2.31.1
Re: [PATCH] spi: stm32_qspi: Fix short data write operation
On Mon, 24 May 2021 09:40:05 +0200
Patrice CHOTARD wrote:
> Hi Daniil
>
> On 5/24/21 12:24 AM, Daniil Stas wrote:
> > TCF flag only means that all data was sent to FIFO. To check if the
> > data was sent out of FIFO we should also wait for the BUSY flag to
> > be cleared. Otherwise there is a race condition which can lead to
> > inability to write short (one byte long) data.
> >
> > Signed-off-by: Daniil Stas
> > Cc: Patrick Delaunay
> > Cc: Patrice Chotard
> > ---
> > drivers/spi/stm32_qspi.c | 29 +++--
> > 1 file changed, 15 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/spi/stm32_qspi.c b/drivers/spi/stm32_qspi.c
> > index 4acc9047b9..8f4aabc3d1 100644
> > --- a/drivers/spi/stm32_qspi.c
> > +++ b/drivers/spi/stm32_qspi.c
> > @@ -148,23 +148,24 @@ static int _stm32_qspi_wait_cmd(struct
> > stm32_qspi_priv *priv, const struct spi_mem_op *op)
> > {
> > u32 sr;
> > - int ret;
> > -
> > - if (!op->data.nbytes)
> > - return _stm32_qspi_wait_for_not_busy(priv);
> > + int ret = 0;
> >
> > - ret = readl_poll_timeout(>regs->sr, sr,
> > -sr & STM32_QSPI_SR_TCF,
> > -STM32_QSPI_CMD_TIMEOUT_US);
> > - if (ret) {
> > - log_err("cmd timeout (stat:%#x)\n", sr);
> > - } else if (readl(>regs->sr) & STM32_QSPI_SR_TEF) {
> > - log_err("transfer error (stat:%#x)\n", sr);
> > - ret = -EIO;
> > + if (op->data.nbytes) {
> > + ret = readl_poll_timeout(>regs->sr, sr,
> > +sr & STM32_QSPI_SR_TCF,
> > +
> > STM32_QSPI_CMD_TIMEOUT_US);
> > + if (ret) {
> > + log_err("cmd timeout (stat:%#x)\n", sr);
> > + } else if (readl(>regs->sr) &
> > STM32_QSPI_SR_TEF) {
> > + log_err("transfer error (stat:%#x)\n", sr);
> > + ret = -EIO;
> > + }
> > + /* clear flags */
> > + writel(STM32_QSPI_FCR_CTCF | STM32_QSPI_FCR_CTEF,
> > >regs->fcr); }
> >
> > - /* clear flags */
> > - writel(STM32_QSPI_FCR_CTCF | STM32_QSPI_FCR_CTEF,
> > >regs->fcr);
> > + if (!ret)
> > + ret = _stm32_qspi_wait_for_not_busy(priv);
> >
> > return ret;
> > }
> >
>
> Have you got a simple test to reproduce the described race condition ?
>
> Thanks
> Patrice
Hi, Patrice
I found this issue on an stm32mp153 based board.
To reproduce it you need to set qspi peripheral clock to a low
value (for example 24 MHz).
Then you can test it in the u-boot console:
STM32MP> clk dump
Clocks:
...
- CK_PER : 24 MHz
...
- QSPI(10) => parent CK_PER(30)
...
STM32MP> sf probe
SF: Detected w25q32jv with page size 256 Bytes, erase size 64 KiB, total 4 MiB
STM32MP> sf erase 0x0030 +1
SF: 65536 bytes @ 0x30 Erased: OK
STM32MP> sf read 0xc410 0x30 10
device 0 offset 0x30, size 0x10
SF: 16 bytes @ 0x30 Read: OK
STM32MP> md.b 0xc410
c410: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
...
STM32MP> mw.b 0xc420 55
STM32MP> sf write 0xc420 0x0030 1
device 0 offset 0x30, size 0x1
SF: 1 bytes @ 0x30 Written: OK
STM32MP> sf read 0xc410 0x0030 10
device 0 offset 0x30, size 0x10
SF: 16 bytes @ 0x30 Read: OK
STM32MP> md.b 0xc410
c410: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
...
With my patch applied the last command result would be:
STM32MP> md.b 0xc410
c410: 55 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ffU...
Thanks,
Daniil
[PATCH] spi: stm32_qspi: Fix short data write operation
TCF flag only means that all data was sent to FIFO. To check if the
data was sent out of FIFO we should also wait for the BUSY flag to be
cleared. Otherwise there is a race condition which can lead to
inability to write short (one byte long) data.
Signed-off-by: Daniil Stas
Cc: Patrick Delaunay
Cc: Patrice Chotard
---
drivers/spi/stm32_qspi.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/spi/stm32_qspi.c b/drivers/spi/stm32_qspi.c
index 4acc9047b9..8f4aabc3d1 100644
--- a/drivers/spi/stm32_qspi.c
+++ b/drivers/spi/stm32_qspi.c
@@ -148,23 +148,24 @@ static int _stm32_qspi_wait_cmd(struct stm32_qspi_priv
*priv,
const struct spi_mem_op *op)
{
u32 sr;
- int ret;
-
- if (!op->data.nbytes)
- return _stm32_qspi_wait_for_not_busy(priv);
+ int ret = 0;
- ret = readl_poll_timeout(>regs->sr, sr,
-sr & STM32_QSPI_SR_TCF,
-STM32_QSPI_CMD_TIMEOUT_US);
- if (ret) {
- log_err("cmd timeout (stat:%#x)\n", sr);
- } else if (readl(>regs->sr) & STM32_QSPI_SR_TEF) {
- log_err("transfer error (stat:%#x)\n", sr);
- ret = -EIO;
+ if (op->data.nbytes) {
+ ret = readl_poll_timeout(>regs->sr, sr,
+sr & STM32_QSPI_SR_TCF,
+STM32_QSPI_CMD_TIMEOUT_US);
+ if (ret) {
+ log_err("cmd timeout (stat:%#x)\n", sr);
+ } else if (readl(>regs->sr) & STM32_QSPI_SR_TEF) {
+ log_err("transfer error (stat:%#x)\n", sr);
+ ret = -EIO;
+ }
+ /* clear flags */
+ writel(STM32_QSPI_FCR_CTCF | STM32_QSPI_FCR_CTEF,
>regs->fcr);
}
- /* clear flags */
- writel(STM32_QSPI_FCR_CTCF | STM32_QSPI_FCR_CTEF, >regs->fcr);
+ if (!ret)
+ ret = _stm32_qspi_wait_for_not_busy(priv);
return ret;
}
--
2.31.0
[PATCH] net: dwc_eth_qos: Fix needless phy auto-negotiation restarts
Disabling clk_ck clock leads to link up status loss in phy, which
leads to auto-negotiation restart before each network command
execution.
This issue is especially big for PXE boot protocol because of
auto-negotiation restarts before each configuration filename trial.
To avoid this issue don't disable clk_ck clock after it was enabled.
Signed-off-by: Daniil Stas
Cc: Ramon Fried
Cc: Joe Hershberger
Cc: Patrick Delaunay
Cc: Patrice Chotard
---
drivers/net/dwc_eth_qos.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index e8242ca4e1..2f088c758f 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -321,6 +321,7 @@ struct eqos_priv {
void *rx_pkt;
bool started;
bool reg_access_ok;
+ bool clk_ck_enabled;
};
/*
@@ -591,12 +592,13 @@ static int eqos_start_clks_stm32(struct udevice *dev)
goto err_disable_clk_rx;
}
- if (clk_valid(>clk_ck)) {
+ if (clk_valid(>clk_ck) && !eqos->clk_ck_enabled) {
ret = clk_enable(>clk_ck);
if (ret < 0) {
pr_err("clk_enable(clk_ck) failed: %d", ret);
goto err_disable_clk_tx;
}
+ eqos->clk_ck_enabled = true;
}
#endif
@@ -648,8 +650,6 @@ static void eqos_stop_clks_stm32(struct udevice *dev)
clk_disable(>clk_tx);
clk_disable(>clk_rx);
clk_disable(>clk_master_bus);
- if (clk_valid(>clk_ck))
- clk_disable(>clk_ck);
#endif
debug("%s: OK\n", __func__);
--
2.31.0
Re: [PATCH 5/8] net: dwc_eth_qos: add dwc eqos for imx support
Hi, i think there are some issues with this patch. > @@ -1131,6 +1205,7 @@ static int eqos_start(struct udevice *dev) > } > > /* Configure MTL */ > + writel(0x60, >mtl_regs->txq0_quantum_weight - 0x100); > > /* Enable Store and Forward mode for TX */ > /* Program Tx operating mode */ What is this address: >mtl_regs->txq0_quantum_weight - 0x100? Isn't it outside of MTL registers range? > @@ -1144,7 +1219,9 @@ static int eqos_start(struct udevice *dev) > > /* Enable Store and Forward mode for RX, since no jumbo frame */ > setbits_le32(>mtl_regs->rxq0_operation_mode, > - EQOS_MTL_RXQ0_OPERATION_MODE_RSF); > + EQOS_MTL_RXQ0_OPERATION_MODE_RSF | > + EQOS_MTL_RXQ0_OPERATION_MODE_FEP | > + EQOS_MTL_RXQ0_OPERATION_MODE_FUP); > > /* Transmit/Receive queue fifo size; use all RAM for 1 queue */ > val = readl(>mac_regs->hw_feature1); Why do you set FEP and FUP bits? It can lead to data corruption as they allow accepting erroneous packets. I think these options should only be used in some debugging mode but not in production. > @@ -1220,6 +1297,19 @@ static int eqos_start(struct udevice *dev) > eqos->config->config_mac << > EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT); > > + clrsetbits_le32(>mac_regs->rxq_ctrl0, > + EQOS_MAC_RXQ_CTRL0_RXQ0EN_MASK << > + EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT, > + 0x2 << > + EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT); > + This line just overrides the value set in the previous line. Is it a mistake? > + /* enable promise mode */ > + setbits_le32(>mac_regs->unused_004[1], > + 0x1); > + Isn't this mode also useful only for debugging?
