[PATCH v2 net-next 0/3] net: mvmdio: Performance related improvements

[PATCH v2 net-next 0/3] net: mvmdio: Performance related improvements

[Tobias Waldekranz]

Observations of the XMDIO bus on a CN9130-based system during a
firmware download showed a very low bus utilization, which stemmed
from the 150us (10x the average access time) sleep which would take
place when the first poll did not succeed.

With this series in place, bus throughput increases by about 10x,
multiplied by whatever gain you are able to extract from running the
MDC at a higher frequency (hardware dependent).

I would really appreciate it if someone with access to hardware using
the IRQ driven path could test that out, since I have not been able to
figure out how to set this up on CN9130.

v1 -> v2:
- Remove dead code
- Simplify IRQ path, now that the polled path is separate.

Tobias Waldekranz (3):
  arm64: dts: marvell: cp11x: Provide clock names for MDIO controllers
  net: mvmdio: Avoid excessive sleeps in polled mode
  net: mvmdio: Support setting the MDC frequency on XSMI controllers

 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi |  4 +
 drivers/net/ethernet/marvell/mvmdio.c         | 97 ++++++++++++-------
 2 files changed, 64 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH v2 net-next 1/3] arm64: dts: marvell: cp11x: Provide clock names for MDIO controllers

[Tobias Waldekranz]

This will let the driver figure out the rate of the core clk, such
that custom MDC frequencies can be supported.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
index 4ec1aae0a3a9..f268017498a9 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
@@ -180,6 +180,8 @@ CP11X_LABEL(mdio): mdio@12a200 {
 			reg = <0x12a200 0x10>;
 			clocks = <&CP11X_LABEL(clk) 1 9>, <&CP11X_LABEL(clk) 1 5>,
 				 <&CP11X_LABEL(clk) 1 6>, <&CP11X_LABEL(clk) 1 18>;
+			clock-names = "gop_clk", "mg_clk",
+				      "mg_core_clk", "axi_clk";
 			status = "disabled";
 		};
 
@@ -190,6 +192,8 @@ CP11X_LABEL(xmdio): mdio@12a600 {
 			reg = <0x12a600 0x10>;
 			clocks = <&CP11X_LABEL(clk) 1 5>,
 				 <&CP11X_LABEL(clk) 1 6>, <&CP11X_LABEL(clk) 1 18>;
+			clock-names = "mg_clk",
+				      "mg_core_clk", "axi_clk";
 			status = "disabled";
 		};
 
-- 
2.34.1

Re: [PATCH v2 net-next 1/3] arm64: dts: marvell: cp11x: Provide clock names for MDIO controllers

[Andrew Lunn]

On Mon, Dec 04, 2023 at 11:08:09AM +0100, Tobias Waldekranz wrote:
> This will let the driver figure out the rate of the core clk, such
> that custom MDC frequencies can be supported.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 1/3] arm64: dts: marvell: cp11x: Provide clock names for MDIO controllers

[Jakub Kicinski]

On Mon,  4 Dec 2023 11:08:09 +0100 Tobias Waldekranz wrote:
> This will let the driver figure out the rate of the core clk, such
> that custom MDC frequencies can be supported.

Leaving this one to Gregory.

[PATCH v2 net-next 2/3] net: mvmdio: Avoid excessive sleeps in polled mode

[Tobias Waldekranz]

Before this change, when operating in polled mode, i.e. no IRQ is
available, every individual C45 access would be hit with a 150us sleep
after the bus access.

For example, on a board with a CN9130 SoC connected to an MV88X3310
PHY, a single C45 read would take around 165us:

    root@infix:~$ mdio f212a600.mdio-mii mmd 4:1 bench 0xc003
    Performed 1000 reads in 165ms

By replacing the long sleep with a tighter poll loop, we observe a 10x
increase in bus throughput:

    root@infix:~$ mdio f212a600.mdio-mii mmd 4:1 bench 0xc003
    Performed 1000 reads in 15ms

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 53 ++++++++-------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 89f26402f8fb..5f66f779e56f 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -58,11 +59,6 @@
  * - Armada 370       (Globalscale Mirabox):   41us to 43us (Polled)
  */
 #define MVMDIO_SMI_TIMEOUT		1000 /* 1000us = 1ms */
-#define MVMDIO_SMI_POLL_INTERVAL_MIN	45
-#define MVMDIO_SMI_POLL_INTERVAL_MAX	55
-
-#define MVMDIO_XSMI_POLL_INTERVAL_MIN	150
-#define MVMDIO_XSMI_POLL_INTERVAL_MAX	160
 
 struct orion_mdio_dev {
 	void __iomem *regs;
@@ -84,8 +80,6 @@ enum orion_mdio_bus_type {
 
 struct orion_mdio_ops {
 	int (*is_done)(struct orion_mdio_dev *);
-	unsigned int poll_interval_min;
-	unsigned int poll_interval_max;
 };
 
 /* Wait for the SMI unit to be ready for another operation
@@ -94,34 +88,23 @@ static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops,
 				 struct mii_bus *bus)
 {
 	struct orion_mdio_dev *dev = bus->priv;
-	unsigned long timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
-	unsigned long end = jiffies + timeout;
-	int timedout = 0;
+	unsigned long timeout;
+	int done;
 
-	while (1) {
-	        if (ops->is_done(dev))
+	if (dev->err_interrupt <= 0) {
+		if (!read_poll_timeout_atomic(ops->is_done, done, done, 2,
+					      MVMDIO_SMI_TIMEOUT, false, dev))
+			return 0;
+	} else {
+		/* wait_event_timeout does not guarantee a delay of at
+		 * least one whole jiffie, so timeout must be no less
+		 * than two.
+		 */
+		timeout = max(usecs_to_jiffies(MVMDIO_SMI_TIMEOUT), 2);
+
+		if (wait_event_timeout(dev->smi_busy_wait,
+				       ops->is_done(dev), timeout))
 			return 0;
-	        else if (timedout)
-			break;
-
-	        if (dev->err_interrupt <= 0) {
-			usleep_range(ops->poll_interval_min,
-				     ops->poll_interval_max);
-
-			if (time_is_before_jiffies(end))
-				++timedout;
-	        } else {
-			/* wait_event_timeout does not guarantee a delay of at
-			 * least one whole jiffie, so timeout must be no less
-			 * than two.
-			 */
-			if (timeout < 2)
-				timeout = 2;
-			wait_event_timeout(dev->smi_busy_wait,
-				           ops->is_done(dev), timeout);
-
-			++timedout;
-	        }
 	}
 
 	dev_err(bus->parent, "Timeout: SMI busy for too long\n");
@@ -135,8 +118,6 @@ static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev)
 
 static const struct orion_mdio_ops orion_mdio_smi_ops = {
 	.is_done = orion_mdio_smi_is_done,
-	.poll_interval_min = MVMDIO_SMI_POLL_INTERVAL_MIN,
-	.poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX,
 };
 
 static int orion_mdio_smi_read(struct mii_bus *bus, int mii_id,
@@ -194,8 +175,6 @@ static int orion_mdio_xsmi_is_done(struct orion_mdio_dev *dev)
 
 static const struct orion_mdio_ops orion_mdio_xsmi_ops = {
 	.is_done = orion_mdio_xsmi_is_done,
-	.poll_interval_min = MVMDIO_XSMI_POLL_INTERVAL_MIN,
-	.poll_interval_max = MVMDIO_XSMI_POLL_INTERVAL_MAX,
 };
 
 static int orion_mdio_xsmi_read_c45(struct mii_bus *bus, int mii_id,
-- 
2.34.1

Re: [PATCH v2 net-next 2/3] net: mvmdio: Avoid excessive sleeps in polled mode

[Andrew Lunn]

On Mon, Dec 04, 2023 at 11:08:10AM +0100, Tobias Waldekranz wrote:
> Before this change, when operating in polled mode, i.e. no IRQ is
> available, every individual C45 access would be hit with a 150us sleep
> after the bus access.
> 
> For example, on a board with a CN9130 SoC connected to an MV88X3310
> PHY, a single C45 read would take around 165us:
> 
>     root@infix:~$ mdio f212a600.mdio-mii mmd 4:1 bench 0xc003
>     Performed 1000 reads in 165ms
> 
> By replacing the long sleep with a tighter poll loop, we observe a 10x
> increase in bus throughput:
> 
>     root@infix:~$ mdio f212a600.mdio-mii mmd 4:1 bench 0xc003
>     Performed 1000 reads in 15ms
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v2 net-next 3/3] net: mvmdio: Support setting the MDC frequency on XSMI controllers

[Tobias Waldekranz]

Support the standard "clock-frequency" attribute to set the generated
MDC frequency. If not specified, the driver will leave the divisor
untouched.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/ethernet/marvell/mvmdio.c | 44 +++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 5f66f779e56f..9190eff6c0bb 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -53,6 +53,13 @@
 #define  MVMDIO_XSMI_BUSY		BIT(30)
 #define MVMDIO_XSMI_ADDR_REG		0x8
 
+#define MVMDIO_XSMI_CFG_REG		0xc
+#define  MVMDIO_XSMI_CLKDIV_MASK	0x3
+#define  MVMDIO_XSMI_CLKDIV_256		0x0
+#define  MVMDIO_XSMI_CLKDIV_64		0x1
+#define  MVMDIO_XSMI_CLKDIV_32		0x2
+#define  MVMDIO_XSMI_CLKDIV_8		0x3
+
 /*
  * SMI Timeout measurements:
  * - Kirkwood 88F6281 (Globalscale Dreamplug): 45us to 95us (Interrupt)
@@ -225,6 +232,40 @@ static int orion_mdio_xsmi_write_c45(struct mii_bus *bus, int mii_id,
 	return 0;
 }
 
+static void orion_mdio_xsmi_set_mdc_freq(struct mii_bus *bus)
+{
+	struct orion_mdio_dev *dev = bus->priv;
+	struct clk *mg_core;
+	u32 div, freq, cfg;
+
+	if (device_property_read_u32(bus->parent, "clock-frequency", &freq))
+		return;
+
+	mg_core = of_clk_get_by_name(bus->parent->of_node, "mg_core_clk");
+	if (IS_ERR(mg_core)) {
+		dev_err(bus->parent,
+			"MG core clock unknown, not changing MDC frequency");
+		return;
+	}
+
+	div = clk_get_rate(mg_core) / (freq + 1) + 1;
+	clk_put(mg_core);
+
+	if (div <= 8)
+		div = MVMDIO_XSMI_CLKDIV_8;
+	else if (div <= 32)
+		div = MVMDIO_XSMI_CLKDIV_32;
+	else if (div <= 64)
+		div = MVMDIO_XSMI_CLKDIV_64;
+	else
+		div = MVMDIO_XSMI_CLKDIV_256;
+
+	cfg = readl(dev->regs + MVMDIO_XSMI_CFG_REG);
+	cfg &= ~MVMDIO_XSMI_CLKDIV_MASK;
+	cfg |= div;
+	writel(cfg, dev->regs + MVMDIO_XSMI_CFG_REG);
+}
+
 static irqreturn_t orion_mdio_err_irq(int irq, void *dev_id)
 {
 	struct orion_mdio_dev *dev = dev_id;
@@ -303,6 +344,9 @@ static int orion_mdio_probe(struct platform_device *pdev)
 			dev_warn(&pdev->dev,
 				 "unsupported number of clocks, limiting to the first "
 				 __stringify(ARRAY_SIZE(dev->clk)) "\n");
+
+		if (type == BUS_TYPE_XSMI)
+			orion_mdio_xsmi_set_mdc_freq(bus);
 	} else {
 		dev->clk[0] = clk_get(&pdev->dev, NULL);
 		if (PTR_ERR(dev->clk[0]) == -EPROBE_DEFER) {
-- 
2.34.1

Re: [PATCH v2 net-next 3/3] net: mvmdio: Support setting the MDC frequency on XSMI controllers

[Andrew Lunn]

On Mon, Dec 04, 2023 at 11:08:11AM +0100, Tobias Waldekranz wrote:
> Support the standard "clock-frequency" attribute to set the generated
> MDC frequency. If not specified, the driver will leave the divisor
> untouched.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 0/3] net: mvmdio: Performance related improvements

[Andrew Lunn]

On Mon, Dec 04, 2023 at 11:08:08AM +0100, Tobias Waldekranz wrote:
> Observations of the XMDIO bus on a CN9130-based system during a
> firmware download showed a very low bus utilization, which stemmed
> from the 150us (10x the average access time) sleep which would take
> place when the first poll did not succeed.
> 
> With this series in place, bus throughput increases by about 10x,
> multiplied by whatever gain you are able to extract from running the
> MDC at a higher frequency (hardware dependent).
> 
> I would really appreciate it if someone with access to hardware using
> the IRQ driven path could test that out, since I have not been able to
> figure out how to set this up on CN9130.

Hi Tobias

I tested on Kirkwood:

               mdio: mdio-bus@72004 {
                        compatible = "marvell,orion-mdio";
                        #address-cells = <1>;
                        #size-cells = <0>;
                        reg = <0x72004 0x84>;
                        interrupts = <46>;

The link is reported as up, ethtool shows the expected link mode
capabilities, mii-tool dumps look O.K.

Tested-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 0/3] net: mvmdio: Performance related improvements

[Tobias Waldekranz]

On tis, dec 05, 2023 at 04:47, Andrew Lunn <andrew@lunn.ch> wrote:
> On Mon, Dec 04, 2023 at 11:08:08AM +0100, Tobias Waldekranz wrote:
>> Observations of the XMDIO bus on a CN9130-based system during a
>> firmware download showed a very low bus utilization, which stemmed
>> from the 150us (10x the average access time) sleep which would take
>> place when the first poll did not succeed.
>> 
>> With this series in place, bus throughput increases by about 10x,
>> multiplied by whatever gain you are able to extract from running the
>> MDC at a higher frequency (hardware dependent).
>> 
>> I would really appreciate it if someone with access to hardware using
>> the IRQ driven path could test that out, since I have not been able to
>> figure out how to set this up on CN9130.
>
> Hi Tobias
>
> I tested on Kirkwood:
>
>                mdio: mdio-bus@72004 {
>                         compatible = "marvell,orion-mdio";
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         reg = <0x72004 0x84>;
>                         interrupts = <46>;
>
> The link is reported as up, ethtool shows the expected link mode
> capabilities, mii-tool dumps look O.K.
>
> Tested-by: Andrew Lunn <andrew@lunn.ch>

Very much appreciated Andrew, thank you!

Re: [PATCH v2 net-next 0/3] net: mvmdio: Performance related improvements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  4 Dec 2023 11:08:08 +0100 you wrote:
> Observations of the XMDIO bus on a CN9130-based system during a
> firmware download showed a very low bus utilization, which stemmed
> from the 150us (10x the average access time) sleep which would take
> place when the first poll did not succeed.
> 
> With this series in place, bus throughput increases by about 10x,
> multiplied by whatever gain you are able to extract from running the
> MDC at a higher frequency (hardware dependent).
> 
> [...]

Here is the summary with links:
  - [v2,net-next,1/3] arm64: dts: marvell: cp11x: Provide clock names for MDIO controllers
    (no matching commit)
  - [v2,net-next,2/3] net: mvmdio: Avoid excessive sleeps in polled mode
    https://git.kernel.org/netdev/net-next/c/7dd12fe34686
  - [v2,net-next,3/3] net: mvmdio: Support setting the MDC frequency on XSMI controllers
    https://git.kernel.org/netdev/net-next/c/eb6a6605ff5a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html