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

[PATCH 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).

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         | 85 +++++++++++++------
 2 files changed, 64 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH 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

[PATCH 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 | 41 +++++++++++----------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 89f26402f8fb..1de2175269bf 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,23 +88,24 @@ 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 end, timeout;
+	int done, timedout;
 
-	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 if (timedout)
-			break;
-
-	        if (dev->err_interrupt <= 0) {
-			usleep_range(ops->poll_interval_min,
-				     ops->poll_interval_max);
+	} else {
+		timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
+		end = jiffies + timeout;
+		timedout = 0;
+
+		while (1) {
+			if (ops->is_done(dev))
+				return 0;
+			else if (timedout)
+				break;
 
-			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.
@@ -135,8 +130,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 +187,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

[PATCH 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 1de2175269bf..69d5e73be8fe 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)
@@ -237,6 +244,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;
@@ -315,6 +356,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 net-next 2/3] net: mvmdio: Avoid excessive sleeps in polled mode

[Jakub Kicinski]

On Fri,  1 Dec 2023 18:35:44 +0100 Tobias Waldekranz wrote:
> @@ -94,23 +88,24 @@ 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 end, timeout;
> +	int done, timedout;
>  
> -	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 if (timedout)
> -			break;
> -
> -	        if (dev->err_interrupt <= 0) {
> -			usleep_range(ops->poll_interval_min,
> -				     ops->poll_interval_max);
> +	} else {
> +		timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
> +		end = jiffies + timeout;
> +		timedout = 0;
> +
> +		while (1) {
> +			if (ops->is_done(dev))
> +				return 0;
> +			else if (timedout)
> +				break;
>  
> -			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.

drivers/net/ethernet/marvell/mvmdio.c:91:16: warning: variable 'end' set but not used [-Wunused-but-set-variable]
   91 |         unsigned long end, timeout;
      |                       ^
-- 
pw-bot: cr

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

[Tobias Waldekranz]

On lör, dec 02, 2023 at 12:45, Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri,  1 Dec 2023 18:35:44 +0100 Tobias Waldekranz wrote:
>> @@ -94,23 +88,24 @@ 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 end, timeout;
>> +	int done, timedout;
>>  
>> -	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 if (timedout)
>> -			break;
>> -
>> -	        if (dev->err_interrupt <= 0) {
>> -			usleep_range(ops->poll_interval_min,
>> -				     ops->poll_interval_max);
>> +	} else {
>> +		timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
>> +		end = jiffies + timeout;
>> +		timedout = 0;
>> +
>> +		while (1) {
>> +			if (ops->is_done(dev))
>> +				return 0;
>> +			else if (timedout)
>> +				break;
>>  
>> -			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.
>
> drivers/net/ethernet/marvell/mvmdio.c:91:16: warning: variable 'end' set but not used [-Wunused-but-set-variable]
>    91 |         unsigned long end, timeout;
>       |                       ^

Rookie mistake, sorry about that.

Looking at it again, I think I was too scared of touching the original
interrupt path, as I have no means of testing it (no hardware). I will
try to simplify this in v2, and hope that someone else can test it.

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

[Andrew Lunn]

> Looking at it again, I think I was too scared of touching the original
> interrupt path, as I have no means of testing it (no hardware). I will
> try to simplify this in v2, and hope that someone else can test it.

I have a couple of kirkwood machines which have the interrupt. So i
can test this.

    Andrew