linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions
@ 2020-05-26 16:22 Antoine Tenart
  2020-05-26 16:22 ` [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays Antoine Tenart
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Antoine Tenart @ 2020-05-26 16:22 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, alexandre.belloni,
	thomas.petazzoni, allan.nielsen

Hello,

This series aims at reducing the waiting time between MDIO transactions
when using the MSCC MIIM MDIO controller.

I'm not sure we need patch 4/4 and we could reasonably drop it from the
series. I'm including the patch as it could help to ensure the system
is functional with a non optimal configuration.

We needed to improve the driver's performances as when using a PHY
requiring lots of registers accesses (such as the VSC85xx family),
delays would add up and ended up to be quite large which would cause
issues such as: a slow initialization of the PHY, and issues when using
timestamping operations (this feature will be sent quite soon to the
mailing lists).

Thanks,
Antoine

Antoine Tenart (4):
  net: phy: mscc-miim: use more reasonable delays
  net: phy: mscc-miim: remove redundant timeout check
  net: phy: mscc-miim: improve waiting logic
  net: phy: mscc-miim: read poll when high resolution timers are
    disabled

 drivers/net/phy/Kconfig          |  3 ++-
 drivers/net/phy/mdio-mscc-miim.c | 33 +++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 8 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays
  2020-05-26 16:22 [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Antoine Tenart
@ 2020-05-26 16:22 ` Antoine Tenart
  2020-05-26 21:04   ` Alexandre Belloni
  2020-05-26 21:19   ` Florian Fainelli
  2020-05-26 16:22 ` [PATCH net-next 2/4] net: phy: mscc-miim: remove redundant timeout check Antoine Tenart
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 17+ messages in thread
From: Antoine Tenart @ 2020-05-26 16:22 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, alexandre.belloni,
	thomas.petazzoni, allan.nielsen

The MSCC MIIM MDIO driver uses delays to read poll a status register. I
made multiple tests on a Ocelot PCS120 platform which led me to reduce
those delays. The delay in between which the polling function is allowed
to sleep is reduced from 100us to 50us which in almost all cases is a
good value to succeed at the first retry. The overall delay is also
lowered as the prior value was really way to high, 10000us is large
enough.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/mdio-mscc-miim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
index badbc99bedd3..0b7544f593fb 100644
--- a/drivers/net/phy/mdio-mscc-miim.c
+++ b/drivers/net/phy/mdio-mscc-miim.c
@@ -44,7 +44,7 @@ static int mscc_miim_wait_ready(struct mii_bus *bus)
 	u32 val;
 
 	readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
-			   !(val & MSCC_MIIM_STATUS_STAT_BUSY), 100, 250000);
+			   !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
 	if (val & MSCC_MIIM_STATUS_STAT_BUSY)
 		return -ETIMEDOUT;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 2/4] net: phy: mscc-miim: remove redundant timeout check
  2020-05-26 16:22 [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Antoine Tenart
  2020-05-26 16:22 ` [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays Antoine Tenart
@ 2020-05-26 16:22 ` Antoine Tenart
  2020-05-26 21:04   ` Alexandre Belloni
  2020-05-26 21:19   ` Florian Fainelli
  2020-05-26 16:22 ` [PATCH net-next 3/4] net: phy: mscc-miim: improve waiting logic Antoine Tenart
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 17+ messages in thread
From: Antoine Tenart @ 2020-05-26 16:22 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, alexandre.belloni,
	thomas.petazzoni, allan.nielsen

readl_poll_timeout already returns -ETIMEDOUT if the condition isn't
satisfied, there's no need to check again the condition after calling
it. Remove the redundant timeout check.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/mdio-mscc-miim.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
index 0b7544f593fb..42119f661452 100644
--- a/drivers/net/phy/mdio-mscc-miim.c
+++ b/drivers/net/phy/mdio-mscc-miim.c
@@ -43,12 +43,8 @@ static int mscc_miim_wait_ready(struct mii_bus *bus)
 	struct mscc_miim_dev *miim = bus->priv;
 	u32 val;
 
-	readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
-			   !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
-	if (val & MSCC_MIIM_STATUS_STAT_BUSY)
-		return -ETIMEDOUT;
-
-	return 0;
+	return readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
+				  !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
 }
 
 static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 3/4] net: phy: mscc-miim: improve waiting logic
  2020-05-26 16:22 [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Antoine Tenart
  2020-05-26 16:22 ` [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays Antoine Tenart
  2020-05-26 16:22 ` [PATCH net-next 2/4] net: phy: mscc-miim: remove redundant timeout check Antoine Tenart
@ 2020-05-26 16:22 ` Antoine Tenart
  2020-05-26 21:07   ` Alexandre Belloni
  2020-05-26 21:21   ` Florian Fainelli
  2020-05-26 16:22 ` [PATCH net-next 4/4] net: phy: mscc-miim: read poll when high resolution timers are disabled Antoine Tenart
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 17+ messages in thread
From: Antoine Tenart @ 2020-05-26 16:22 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, alexandre.belloni,
	thomas.petazzoni, allan.nielsen

The MSCC MIIM MDIO driver uses a waiting logic to wait for the MDIO bus
to be ready to accept next commands. It does so by polling the BUSY
status bit which indicates the MDIO bus has completed all pending
operations. This can take time, and the controller supports writing the
next command as soon as there are no pending commands (which happens
while the MDIO bus is busy completing its current command).

This patch implements this improved logic by adding an helper to poll
the PENDING status bit, and by adjusting where we should wait for the
bus to not be busy or to not be pending.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/mdio-mscc-miim.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
index 42119f661452..aed9afa1e8f1 100644
--- a/drivers/net/phy/mdio-mscc-miim.c
+++ b/drivers/net/phy/mdio-mscc-miim.c
@@ -16,6 +16,7 @@
 #include <linux/of_mdio.h>
 
 #define MSCC_MIIM_REG_STATUS		0x0
+#define		MSCC_MIIM_STATUS_STAT_PENDING	BIT(2)
 #define		MSCC_MIIM_STATUS_STAT_BUSY	BIT(3)
 #define MSCC_MIIM_REG_CMD		0x8
 #define		MSCC_MIIM_CMD_OPR_WRITE		BIT(1)
@@ -47,13 +48,23 @@ static int mscc_miim_wait_ready(struct mii_bus *bus)
 				  !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
 }
 
+static int mscc_miim_wait_pending(struct mii_bus *bus)
+{
+	struct mscc_miim_dev *miim = bus->priv;
+	u32 val;
+
+	return readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
+				  !(val & MSCC_MIIM_STATUS_STAT_PENDING),
+				  50, 10000);
+}
+
 static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct mscc_miim_dev *miim = bus->priv;
 	u32 val;
 	int ret;
 
-	ret = mscc_miim_wait_ready(bus);
+	ret = mscc_miim_wait_pending(bus);
 	if (ret)
 		goto out;
 
@@ -82,7 +93,7 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
 	struct mscc_miim_dev *miim = bus->priv;
 	int ret;
 
-	ret = mscc_miim_wait_ready(bus);
+	ret = mscc_miim_wait_pending(bus);
 	if (ret < 0)
 		goto out;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 4/4] net: phy: mscc-miim: read poll when high resolution timers are disabled
  2020-05-26 16:22 [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Antoine Tenart
                   ` (2 preceding siblings ...)
  2020-05-26 16:22 ` [PATCH net-next 3/4] net: phy: mscc-miim: improve waiting logic Antoine Tenart
@ 2020-05-26 16:22 ` Antoine Tenart
  2020-05-26 21:22   ` Florian Fainelli
  2020-05-26 17:01 ` [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Andrew Lunn
  2020-05-26 22:34 ` David Miller
  5 siblings, 1 reply; 17+ messages in thread
From: Antoine Tenart @ 2020-05-26 16:22 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, alexandre.belloni,
	thomas.petazzoni, allan.nielsen

The driver uses a read polling mechanism to check the status of the MDIO
bus, to know if it is ready to accept next commands. This polling
mechanism uses usleep_delay() under the hood between reads which is fine
as long as high resolution timers are enabled. Otherwise the delays will
end up to be much longer than expected.

This patch fixes this by using udelay() under the hood when
CONFIG_HIGH_RES_TIMERS isn't enabled. This increases CPU usage.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/Kconfig          |  3 ++-
 drivers/net/phy/mdio-mscc-miim.c | 22 +++++++++++++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 2a32f26ead0b..047c27087b10 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -184,7 +184,8 @@ config MDIO_MSCC_MIIM
 	depends on HAS_IOMEM
 	help
 	  This driver supports the MIIM (MDIO) interface found in the network
-	  switches of the Microsemi SoCs
+	  switches of the Microsemi SoCs; it is recommended to switch on
+	  CONFIG_HIGH_RES_TIMERS
 
 config MDIO_MVUSB
 	tristate "Marvell USB to MDIO Adapter"
diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
index aed9afa1e8f1..11f583fd4611 100644
--- a/drivers/net/phy/mdio-mscc-miim.c
+++ b/drivers/net/phy/mdio-mscc-miim.c
@@ -39,13 +39,25 @@ struct mscc_miim_dev {
 	void __iomem *phy_regs;
 };
 
+/* When high resolution timers aren't built-in: we can't use usleep_range() as
+ * we would sleep way too long. Use udelay() instead.
+ */
+#define mscc_readl_poll_timeout(addr, val, cond, delay_us, timeout_us)	\
+({									\
+	if (!IS_ENABLED(CONFIG_HIGH_RES_TIMERS))			\
+		readl_poll_timeout_atomic(addr, val, cond, delay_us,	\
+					  timeout_us);			\
+	readl_poll_timeout(addr, val, cond, delay_us, timeout_us);	\
+})
+
 static int mscc_miim_wait_ready(struct mii_bus *bus)
 {
 	struct mscc_miim_dev *miim = bus->priv;
 	u32 val;
 
-	return readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
-				  !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
+	return mscc_readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
+				       !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50,
+				       10000);
 }
 
 static int mscc_miim_wait_pending(struct mii_bus *bus)
@@ -53,9 +65,9 @@ static int mscc_miim_wait_pending(struct mii_bus *bus)
 	struct mscc_miim_dev *miim = bus->priv;
 	u32 val;
 
-	return readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
-				  !(val & MSCC_MIIM_STATUS_STAT_PENDING),
-				  50, 10000);
+	return mscc_readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
+				       !(val & MSCC_MIIM_STATUS_STAT_PENDING),
+				       50, 10000);
 }
 
 static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions
  2020-05-26 16:22 [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Antoine Tenart
                   ` (3 preceding siblings ...)
  2020-05-26 16:22 ` [PATCH net-next 4/4] net: phy: mscc-miim: read poll when high resolution timers are disabled Antoine Tenart
@ 2020-05-26 17:01 ` Andrew Lunn
  2020-05-27  7:00   ` Antoine Tenart
  2020-05-26 22:34 ` David Miller
  5 siblings, 1 reply; 17+ messages in thread
From: Andrew Lunn @ 2020-05-26 17:01 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: davem, f.fainelli, hkallweit1, netdev, linux-kernel,
	alexandre.belloni, thomas.petazzoni, allan.nielsen

On Tue, May 26, 2020 at 06:22:52PM +0200, Antoine Tenart wrote:
> Hello,
> 
> This series aims at reducing the waiting time between MDIO transactions
> when using the MSCC MIIM MDIO controller.

Hi Antoine

There are a couple of other things you can look at:

Can you disable the pre-amble on the MDIO transaction. It requires
that both the bus master and all devices on the bus support it, but
when it is usable, you half the number of bits sent over the wire.

Can you control the frequency of MDC? 802.3 says 2.5MHz, but many
devices support higher speeds. Again, you need all devices on the bus
to support the speed.

When accessing raw TDR data for cable tests i also have a lot of PHY
accesses. I implemented both of these for the FEC MDIO bus, and made
it a lot faster.

   Andrew

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays
  2020-05-26 16:22 ` [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays Antoine Tenart
@ 2020-05-26 21:04   ` Alexandre Belloni
  2020-05-26 21:19   ` Florian Fainelli
  1 sibling, 0 replies; 17+ messages in thread
From: Alexandre Belloni @ 2020-05-26 21:04 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: davem, andrew, f.fainelli, hkallweit1, netdev, linux-kernel,
	thomas.petazzoni, allan.nielsen

On 26/05/2020 18:22:53+0200, Antoine Ténart wrote:
> The MSCC MIIM MDIO driver uses delays to read poll a status register. I
> made multiple tests on a Ocelot PCS120 platform which led me to reduce
> those delays. The delay in between which the polling function is allowed
> to sleep is reduced from 100us to 50us which in almost all cases is a
> good value to succeed at the first retry. The overall delay is also
> lowered as the prior value was really way to high, 10000us is large
> enough.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/phy/mdio-mscc-miim.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
> index badbc99bedd3..0b7544f593fb 100644
> --- a/drivers/net/phy/mdio-mscc-miim.c
> +++ b/drivers/net/phy/mdio-mscc-miim.c
> @@ -44,7 +44,7 @@ static int mscc_miim_wait_ready(struct mii_bus *bus)
>  	u32 val;
>  
>  	readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
> -			   !(val & MSCC_MIIM_STATUS_STAT_BUSY), 100, 250000);
> +			   !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
>  	if (val & MSCC_MIIM_STATUS_STAT_BUSY)
>  		return -ETIMEDOUT;
>  
> -- 
> 2.26.2
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 2/4] net: phy: mscc-miim: remove redundant timeout check
  2020-05-26 16:22 ` [PATCH net-next 2/4] net: phy: mscc-miim: remove redundant timeout check Antoine Tenart
@ 2020-05-26 21:04   ` Alexandre Belloni
  2020-05-26 21:19   ` Florian Fainelli
  1 sibling, 0 replies; 17+ messages in thread
From: Alexandre Belloni @ 2020-05-26 21:04 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: davem, andrew, f.fainelli, hkallweit1, netdev, linux-kernel,
	thomas.petazzoni, allan.nielsen

On 26/05/2020 18:22:54+0200, Antoine Ténart wrote:
> readl_poll_timeout already returns -ETIMEDOUT if the condition isn't
> satisfied, there's no need to check again the condition after calling
> it. Remove the redundant timeout check.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/phy/mdio-mscc-miim.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
> index 0b7544f593fb..42119f661452 100644
> --- a/drivers/net/phy/mdio-mscc-miim.c
> +++ b/drivers/net/phy/mdio-mscc-miim.c
> @@ -43,12 +43,8 @@ static int mscc_miim_wait_ready(struct mii_bus *bus)
>  	struct mscc_miim_dev *miim = bus->priv;
>  	u32 val;
>  
> -	readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
> -			   !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
> -	if (val & MSCC_MIIM_STATUS_STAT_BUSY)
> -		return -ETIMEDOUT;
> -
> -	return 0;
> +	return readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
> +				  !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
>  }
>  
>  static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
> -- 
> 2.26.2
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 3/4] net: phy: mscc-miim: improve waiting logic
  2020-05-26 16:22 ` [PATCH net-next 3/4] net: phy: mscc-miim: improve waiting logic Antoine Tenart
@ 2020-05-26 21:07   ` Alexandre Belloni
  2020-05-26 21:21   ` Florian Fainelli
  1 sibling, 0 replies; 17+ messages in thread
From: Alexandre Belloni @ 2020-05-26 21:07 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: davem, andrew, f.fainelli, hkallweit1, netdev, linux-kernel,
	thomas.petazzoni, allan.nielsen

On 26/05/2020 18:22:55+0200, Antoine Ténart wrote:
> The MSCC MIIM MDIO driver uses a waiting logic to wait for the MDIO bus
> to be ready to accept next commands. It does so by polling the BUSY
> status bit which indicates the MDIO bus has completed all pending
> operations. This can take time, and the controller supports writing the
> next command as soon as there are no pending commands (which happens
> while the MDIO bus is busy completing its current command).
> 
> This patch implements this improved logic by adding an helper to poll
> the PENDING status bit, and by adjusting where we should wait for the
> bus to not be busy or to not be pending.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/phy/mdio-mscc-miim.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
> index 42119f661452..aed9afa1e8f1 100644
> --- a/drivers/net/phy/mdio-mscc-miim.c
> +++ b/drivers/net/phy/mdio-mscc-miim.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_mdio.h>
>  
>  #define MSCC_MIIM_REG_STATUS		0x0
> +#define		MSCC_MIIM_STATUS_STAT_PENDING	BIT(2)
>  #define		MSCC_MIIM_STATUS_STAT_BUSY	BIT(3)
>  #define MSCC_MIIM_REG_CMD		0x8
>  #define		MSCC_MIIM_CMD_OPR_WRITE		BIT(1)
> @@ -47,13 +48,23 @@ static int mscc_miim_wait_ready(struct mii_bus *bus)
>  				  !(val & MSCC_MIIM_STATUS_STAT_BUSY), 50, 10000);
>  }
>  
> +static int mscc_miim_wait_pending(struct mii_bus *bus)
> +{
> +	struct mscc_miim_dev *miim = bus->priv;
> +	u32 val;
> +
> +	return readl_poll_timeout(miim->regs + MSCC_MIIM_REG_STATUS, val,
> +				  !(val & MSCC_MIIM_STATUS_STAT_PENDING),
> +				  50, 10000);
> +}
> +
>  static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
>  	u32 val;
>  	int ret;
>  
> -	ret = mscc_miim_wait_ready(bus);
> +	ret = mscc_miim_wait_pending(bus);
>  	if (ret)
>  		goto out;
>  
> @@ -82,7 +93,7 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  	struct mscc_miim_dev *miim = bus->priv;
>  	int ret;
>  
> -	ret = mscc_miim_wait_ready(bus);
> +	ret = mscc_miim_wait_pending(bus);
>  	if (ret < 0)
>  		goto out;
>  
> -- 
> 2.26.2
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays
  2020-05-26 16:22 ` [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays Antoine Tenart
  2020-05-26 21:04   ` Alexandre Belloni
@ 2020-05-26 21:19   ` Florian Fainelli
  1 sibling, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2020-05-26 21:19 UTC (permalink / raw)
  To: Antoine Tenart, davem, andrew, hkallweit1
  Cc: netdev, linux-kernel, alexandre.belloni, thomas.petazzoni, allan.nielsen



On 5/26/2020 9:22 AM, Antoine Tenart wrote:
> The MSCC MIIM MDIO driver uses delays to read poll a status register. I
> made multiple tests on a Ocelot PCS120 platform which led me to reduce
> those delays. The delay in between which the polling function is allowed
> to sleep is reduced from 100us to 50us which in almost all cases is a
> good value to succeed at the first retry. The overall delay is also
> lowered as the prior value was really way to high, 10000us is large
> enough.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 2/4] net: phy: mscc-miim: remove redundant timeout check
  2020-05-26 16:22 ` [PATCH net-next 2/4] net: phy: mscc-miim: remove redundant timeout check Antoine Tenart
  2020-05-26 21:04   ` Alexandre Belloni
@ 2020-05-26 21:19   ` Florian Fainelli
  1 sibling, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2020-05-26 21:19 UTC (permalink / raw)
  To: Antoine Tenart, davem, andrew, hkallweit1
  Cc: netdev, linux-kernel, alexandre.belloni, thomas.petazzoni, allan.nielsen



On 5/26/2020 9:22 AM, Antoine Tenart wrote:
> readl_poll_timeout already returns -ETIMEDOUT if the condition isn't
> satisfied, there's no need to check again the condition after calling
> it. Remove the redundant timeout check.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 3/4] net: phy: mscc-miim: improve waiting logic
  2020-05-26 16:22 ` [PATCH net-next 3/4] net: phy: mscc-miim: improve waiting logic Antoine Tenart
  2020-05-26 21:07   ` Alexandre Belloni
@ 2020-05-26 21:21   ` Florian Fainelli
  1 sibling, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2020-05-26 21:21 UTC (permalink / raw)
  To: Antoine Tenart, davem, andrew, hkallweit1
  Cc: netdev, linux-kernel, alexandre.belloni, thomas.petazzoni, allan.nielsen



On 5/26/2020 9:22 AM, Antoine Tenart wrote:
> The MSCC MIIM MDIO driver uses a waiting logic to wait for the MDIO bus
> to be ready to accept next commands. It does so by polling the BUSY
> status bit which indicates the MDIO bus has completed all pending
> operations. This can take time, and the controller supports writing the
> next command as soon as there are no pending commands (which happens
> while the MDIO bus is busy completing its current command).
> 
> This patch implements this improved logic by adding an helper to poll
> the PENDING status bit, and by adjusting where we should wait for the
> bus to not be busy or to not be pending.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 4/4] net: phy: mscc-miim: read poll when high resolution timers are disabled
  2020-05-26 16:22 ` [PATCH net-next 4/4] net: phy: mscc-miim: read poll when high resolution timers are disabled Antoine Tenart
@ 2020-05-26 21:22   ` Florian Fainelli
  2020-05-26 22:01     ` Andrew Lunn
  0 siblings, 1 reply; 17+ messages in thread
From: Florian Fainelli @ 2020-05-26 21:22 UTC (permalink / raw)
  To: Antoine Tenart, davem, andrew, hkallweit1
  Cc: netdev, linux-kernel, alexandre.belloni, thomas.petazzoni, allan.nielsen



On 5/26/2020 9:22 AM, Antoine Tenart wrote:
> The driver uses a read polling mechanism to check the status of the MDIO
> bus, to know if it is ready to accept next commands. This polling
> mechanism uses usleep_delay() under the hood between reads which is fine
> as long as high resolution timers are enabled. Otherwise the delays will
> end up to be much longer than expected.
> 
> This patch fixes this by using udelay() under the hood when
> CONFIG_HIGH_RES_TIMERS isn't enabled. This increases CPU usage.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  drivers/net/phy/Kconfig          |  3 ++-
>  drivers/net/phy/mdio-mscc-miim.c | 22 +++++++++++++++++-----
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 2a32f26ead0b..047c27087b10 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -184,7 +184,8 @@ config MDIO_MSCC_MIIM
>  	depends on HAS_IOMEM
>  	help
>  	  This driver supports the MIIM (MDIO) interface found in the network
> -	  switches of the Microsemi SoCs
> +	  switches of the Microsemi SoCs; it is recommended to switch on
> +	  CONFIG_HIGH_RES_TIMERS
>  
>  config MDIO_MVUSB
>  	tristate "Marvell USB to MDIO Adapter"
> diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
> index aed9afa1e8f1..11f583fd4611 100644
> --- a/drivers/net/phy/mdio-mscc-miim.c
> +++ b/drivers/net/phy/mdio-mscc-miim.c
> @@ -39,13 +39,25 @@ struct mscc_miim_dev {
>  	void __iomem *phy_regs;
>  };
>  
> +/* When high resolution timers aren't built-in: we can't use usleep_range() as
> + * we would sleep way too long. Use udelay() instead.
> + */
> +#define mscc_readl_poll_timeout(addr, val, cond, delay_us, timeout_us)	\
> +({									\
> +	if (!IS_ENABLED(CONFIG_HIGH_RES_TIMERS))			\
> +		readl_poll_timeout_atomic(addr, val, cond, delay_us,	\
> +					  timeout_us);			\
> +	readl_poll_timeout(addr, val, cond, delay_us, timeout_us);	\
> +})
> +

I would make this a regular function which would not harm the compiler's
ability to optimize it, but would give you type checking. With that fixed:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 4/4] net: phy: mscc-miim: read poll when high resolution timers are disabled
  2020-05-26 21:22   ` Florian Fainelli
@ 2020-05-26 22:01     ` Andrew Lunn
  2020-05-26 22:05       ` Florian Fainelli
  0 siblings, 1 reply; 17+ messages in thread
From: Andrew Lunn @ 2020-05-26 22:01 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Antoine Tenart, davem, hkallweit1, netdev, linux-kernel,
	alexandre.belloni, thomas.petazzoni, allan.nielsen

> > +/* When high resolution timers aren't built-in: we can't use usleep_range() as
> > + * we would sleep way too long. Use udelay() instead.
> > + */
> > +#define mscc_readl_poll_timeout(addr, val, cond, delay_us, timeout_us)	\
> > +({									\
> > +	if (!IS_ENABLED(CONFIG_HIGH_RES_TIMERS))			\
> > +		readl_poll_timeout_atomic(addr, val, cond, delay_us,	\
> > +					  timeout_us);			\
> > +	readl_poll_timeout(addr, val, cond, delay_us, timeout_us);	\
> > +})
> > +
> 
> I would make this a regular function which would not harm the compiler's
> ability to optimize it, but would give you type checking. With that fixed:

Hi Florian

cond makes that difficult, since it is not a parameter in the usual
sense, but an expression to evaluate if the polling should terminate.

readl_poll_timeout() and readl_poll_timeout_atomic() themselves are
#define's, and there are more levels of macros under them.

	   Andrew

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 4/4] net: phy: mscc-miim: read poll when high resolution timers are disabled
  2020-05-26 22:01     ` Andrew Lunn
@ 2020-05-26 22:05       ` Florian Fainelli
  0 siblings, 0 replies; 17+ messages in thread
From: Florian Fainelli @ 2020-05-26 22:05 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Antoine Tenart, davem, hkallweit1, netdev, linux-kernel,
	alexandre.belloni, thomas.petazzoni, allan.nielsen



On 5/26/2020 3:01 PM, Andrew Lunn wrote:
>>> +/* When high resolution timers aren't built-in: we can't use usleep_range() as
>>> + * we would sleep way too long. Use udelay() instead.
>>> + */
>>> +#define mscc_readl_poll_timeout(addr, val, cond, delay_us, timeout_us)	\
>>> +({									\
>>> +	if (!IS_ENABLED(CONFIG_HIGH_RES_TIMERS))			\
>>> +		readl_poll_timeout_atomic(addr, val, cond, delay_us,	\
>>> +					  timeout_us);			\
>>> +	readl_poll_timeout(addr, val, cond, delay_us, timeout_us);	\
>>> +})
>>> +
>>
>> I would make this a regular function which would not harm the compiler's
>> ability to optimize it, but would give you type checking. With that fixed:
> 
> Hi Florian
> 
> cond makes that difficult, since it is not a parameter in the usual
> sense, but an expression to evaluate if the polling should terminate.
> 
> readl_poll_timeout() and readl_poll_timeout_atomic() themselves are
> #define's, and there are more levels of macros under them.

Oh that's right, thanks for reminding me of this.
-- 
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions
  2020-05-26 16:22 [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Antoine Tenart
                   ` (4 preceding siblings ...)
  2020-05-26 17:01 ` [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Andrew Lunn
@ 2020-05-26 22:34 ` David Miller
  5 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2020-05-26 22:34 UTC (permalink / raw)
  To: antoine.tenart
  Cc: andrew, f.fainelli, hkallweit1, netdev, linux-kernel,
	alexandre.belloni, thomas.petazzoni, allan.nielsen

From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Tue, 26 May 2020 18:22:52 +0200

> This series aims at reducing the waiting time between MDIO transactions
> when using the MSCC MIIM MDIO controller.
> 
> I'm not sure we need patch 4/4 and we could reasonably drop it from the
> series. I'm including the patch as it could help to ensure the system
> is functional with a non optimal configuration.
> 
> We needed to improve the driver's performances as when using a PHY
> requiring lots of registers accesses (such as the VSC85xx family),
> delays would add up and ended up to be quite large which would cause
> issues such as: a slow initialization of the PHY, and issues when using
> timestamping operations (this feature will be sent quite soon to the
> mailing lists).

Series applied, thank you.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions
  2020-05-26 17:01 ` [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Andrew Lunn
@ 2020-05-27  7:00   ` Antoine Tenart
  0 siblings, 0 replies; 17+ messages in thread
From: Antoine Tenart @ 2020-05-27  7:00 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: davem, f.fainelli, hkallweit1, netdev, linux-kernel,
	alexandre.belloni, thomas.petazzoni, allan.nielsen

Hi Andrew,

Quoting Andrew Lunn (2020-05-26 19:01:00)
> On Tue, May 26, 2020 at 06:22:52PM +0200, Antoine Tenart wrote:
> > 
> > This series aims at reducing the waiting time between MDIO transactions
> > when using the MSCC MIIM MDIO controller.
> 
> There are a couple of other things you can look at:
> 
> Can you disable the pre-amble on the MDIO transaction. It requires
> that both the bus master and all devices on the bus support it, but
> when it is usable, you half the number of bits sent over the wire.
> 
> Can you control the frequency of MDC? 802.3 says 2.5MHz, but many
> devices support higher speeds. Again, you need all devices on the bus
> to support the speed.
> 
> When accessing raw TDR data for cable tests i also have a lot of PHY
> accesses. I implemented both of these for the FEC MDIO bus, and made
> it a lot faster.

Thanks for the tips!

Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-05-27  7:01 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-26 16:22 [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Antoine Tenart
2020-05-26 16:22 ` [PATCH net-next 1/4] net: phy: mscc-miim: use more reasonable delays Antoine Tenart
2020-05-26 21:04   ` Alexandre Belloni
2020-05-26 21:19   ` Florian Fainelli
2020-05-26 16:22 ` [PATCH net-next 2/4] net: phy: mscc-miim: remove redundant timeout check Antoine Tenart
2020-05-26 21:04   ` Alexandre Belloni
2020-05-26 21:19   ` Florian Fainelli
2020-05-26 16:22 ` [PATCH net-next 3/4] net: phy: mscc-miim: improve waiting logic Antoine Tenart
2020-05-26 21:07   ` Alexandre Belloni
2020-05-26 21:21   ` Florian Fainelli
2020-05-26 16:22 ` [PATCH net-next 4/4] net: phy: mscc-miim: read poll when high resolution timers are disabled Antoine Tenart
2020-05-26 21:22   ` Florian Fainelli
2020-05-26 22:01     ` Andrew Lunn
2020-05-26 22:05       ` Florian Fainelli
2020-05-26 17:01 ` [PATCH net-next 0/4] net: phy: mscc-miim: reduce waiting time between MDIO transactions Andrew Lunn
2020-05-27  7:00   ` Antoine Tenart
2020-05-26 22:34 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).