[PATCH net-next 0/2] net: ipa: fix HOLB timer register use

[PATCH net-next 0/2] net: ipa: fix HOLB timer register use

[Alex Elder]

The function ipa_reg_init_hol_block_timer_val() generates the value
to write into the HOL_BLOCK_TIMER endpoint configuration register,
to represent a given timeout value (in microseconds).  It only
supports a timer value of 0 though, in part because that's
sufficient, but mainly because there was some confusion about
how the register is formatted in newer hardware.

I got clarification about the register format, so this series fixes 
ipa_reg_init_hol_block_timer_val() to work for any supported delay
value.

The delay is based on the IPA core clock, so determining the value
to write for a given period requires access to the current core
clock rate.  So the first patch just creates a new function to
provide that.

					-Alex

Alex Elder (2):
  net: ipa: introduce ipa_clock_rate()
  net: ipa: fix HOLB timer calculation

 drivers/net/ipa/ipa_clock.c    |  6 +++
 drivers/net/ipa/ipa_clock.h    |  8 ++++
 drivers/net/ipa/ipa_endpoint.c | 78 +++++++++++++++++++---------------
 3 files changed, 57 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH net-next 1/2] net: ipa: introduce ipa_clock_rate()

[Alex Elder]

Create a new function that returns the current rate of the IPA core
clock.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c | 6 ++++++
 drivers/net/ipa/ipa_clock.h | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index c5204fd58ac4..0fbc8b1bdf41 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -256,6 +256,12 @@ void ipa_clock_put(struct ipa *ipa)
 	mutex_unlock(&clock->mutex);
 }
 
+/* Return the current IPA core clock rate */
+u32 ipa_clock_rate(struct ipa *ipa)
+{
+	return ipa->clock ? (u32)clk_get_rate(ipa->clock->core) : 0;
+}
+
 /* Initialize IPA clocking */
 struct ipa_clock *ipa_clock_init(struct device *dev)
 {
diff --git a/drivers/net/ipa/ipa_clock.h b/drivers/net/ipa/ipa_clock.h
index bc52b35e6bb2..0f4e2877a6df 100644
--- a/drivers/net/ipa/ipa_clock.h
+++ b/drivers/net/ipa/ipa_clock.h
@@ -10,6 +10,14 @@ struct device;
 
 struct ipa;
 
+/**
+ * ipa_clock_rate() - Return the current IPA core clock rate
+ * @ipa:	IPA structure
+ *
+ * Return: The current clock rate (in Hz), or 0.
+ */
+u32 ipa_clock_rate(struct ipa *ipa);
+
 /**
  * ipa_clock_init() - Initialize IPA clocking
  * @dev:	IPA device
-- 
2.25.1

[PATCH net-next 2/2] net: ipa: fix HOLB timer calculation

[Alex Elder]

For IPA v4.2, the exact interpretation of the register that defines
the timeout for avoiding head-of-line blocking was a little unclear.
We're only assigning a 0 timeout to it right now, so that wasn't
very important.  But now that I know how it's supposed to work, I'm
fixing it.

The register represents a tick counter, where each tick is equal to
128 IPA core clock cycles.  For IPA v3.5.1, the register contains
a simple counter value.  But for IPA v4.2, the register contains two
fields, base and scale, which approximate the tick counter as:
    ticks = base << scale
The base and scale values to use for a given tick count are computed
using clever bit operations, and measures are taken to make the
resulting time period as close as possible to that requested.

There's no need for ipa_endpoint_init_hol_block_timer() to return
an error, so change its return type to void.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_endpoint.c | 78 +++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 99115a2a29ae..d4be12385bcc 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -21,6 +21,7 @@
 #include "ipa_modem.h"
 #include "ipa_table.h"
 #include "ipa_gsi.h"
+#include "ipa_clock.h"
 
 #define atomic_dec_not_zero(v)	atomic_add_unless((v), -1, 0)
 
@@ -675,63 +676,70 @@ static void ipa_endpoint_init_aggr(struct ipa_endpoint *endpoint)
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
 
-/* A return value of 0 indicates an error */
+/* The head-of-line blocking timer is defined as a tick count, where each
+ * tick represents 128 cycles of the IPA core clock.  Return the value
+ * that should be written to that register that represents the timeout
+ * period provided.
+ */
 static u32 ipa_reg_init_hol_block_timer_val(struct ipa *ipa, u32 microseconds)
 {
+	u32 width;
 	u32 scale;
-	u32 base;
+	u64 ticks;
+	u64 rate;
+	u32 high;
 	u32 val;
 
 	if (!microseconds)
-		return 0;	/* invalid delay */
+		return 0;	/* Nothing to compute if timer period is 0 */
 
-	/* Timer is represented in units of clock ticks. */
-	if (ipa->version < IPA_VERSION_4_2)
-		return microseconds;	/* XXX Needs to be computed */
+	/* Use 64 bit arithmetic to avoid overflow... */
+	rate = ipa_clock_rate(ipa);
+	ticks = DIV_ROUND_CLOSEST(microseconds * rate, 128 * USEC_PER_SEC);
+	/* ...but we still need to fit into a 32-bit register */
+	WARN_ON(ticks > U32_MAX);
 
-	/* IPA v4.2 represents the tick count as base * scale */
-	scale = 1;			/* XXX Needs to be computed */
-	if (scale > field_max(SCALE_FMASK))
-		return 0;		/* scale too big */
+	/* IPA v3.5.1 just records the tick count */
+	if (ipa->version == IPA_VERSION_3_5_1)
+		return (u32)ticks;
 
-	base = DIV_ROUND_CLOSEST(microseconds, scale);
-	if (base > field_max(BASE_VALUE_FMASK))
-		return 0;		/* microseconds too big */
+	/* For IPA v4.2, the tick count is represented by base and
+	 * scale fields within the 32-bit timer register, where:
+	 *     ticks = base << scale;
+	 * The best precision is achieved when the base value is as
+	 * large as possible.  Find the highest set bit in the tick
+	 * count, and extract the number of bits in the base field
+	 * such that that high bit is included.
+	 */
+	high = fls(ticks);		/* 1..32 */
+	width = HWEIGHT32(BASE_VALUE_FMASK);
+	scale = high > width ? high - width : 0;
+	if (scale) {
+		/* If we're scaling, round up to get a closer result */
+		ticks += 1 << (scale - 1);
+		/* High bit was set, so rounding might have affected it */
+		if (fls(ticks) != high)
+			scale++;
+	}
 
 	val = u32_encode_bits(scale, SCALE_FMASK);
-	val |= u32_encode_bits(base, BASE_VALUE_FMASK);
+	val |= u32_encode_bits(ticks >> scale, BASE_VALUE_FMASK);
 
 	return val;
 }
 
-static int ipa_endpoint_init_hol_block_timer(struct ipa_endpoint *endpoint,
-					     u32 microseconds)
+/* If microseconds is 0, timeout is immediate */
+static void ipa_endpoint_init_hol_block_timer(struct ipa_endpoint *endpoint,
+					      u32 microseconds)
 {
 	u32 endpoint_id = endpoint->endpoint_id;
 	struct ipa *ipa = endpoint->ipa;
 	u32 offset;
 	u32 val;
 
-	/* XXX We'll fix this when the register definition is clear */
-	if (microseconds) {
-		struct device *dev = &ipa->pdev->dev;
-
-		dev_err(dev, "endpoint %u non-zero HOLB period (ignoring)\n",
-			endpoint_id);
-		microseconds = 0;
-	}
-
-	if (microseconds) {
-		val = ipa_reg_init_hol_block_timer_val(ipa, microseconds);
-		if (!val)
-			return -EINVAL;
-	} else {
-		val = 0;	/* timeout is immediate */
-	}
 	offset = IPA_REG_ENDP_INIT_HOL_BLOCK_TIMER_N_OFFSET(endpoint_id);
+	val = ipa_reg_init_hol_block_timer_val(ipa, microseconds);
 	iowrite32(val, ipa->reg_virt + offset);
-
-	return 0;
 }
 
 static void
@@ -756,7 +764,7 @@ void ipa_endpoint_modem_hol_block_clear_all(struct ipa *ipa)
 		if (endpoint->toward_ipa || endpoint->ee_id != GSI_EE_MODEM)
 			continue;
 
-		(void)ipa_endpoint_init_hol_block_timer(endpoint, 0);
+		ipa_endpoint_init_hol_block_timer(endpoint, 0);
 		ipa_endpoint_init_hol_block_enable(endpoint, true);
 	}
 }
-- 
2.25.1

Re: [PATCH net-next 0/2] net: ipa: fix HOLB timer register use

[David Miller]

From: Alex Elder <elder@linaro.org>
Date: Fri,  3 Jul 2020 16:23:33 -0500

> The function ipa_reg_init_hol_block_timer_val() generates the value
> to write into the HOL_BLOCK_TIMER endpoint configuration register,
> to represent a given timeout value (in microseconds).  It only
> supports a timer value of 0 though, in part because that's
> sufficient, but mainly because there was some confusion about
> how the register is formatted in newer hardware.
> 
> I got clarification about the register format, so this series fixes 
> ipa_reg_init_hol_block_timer_val() to work for any supported delay
> value.
> 
> The delay is based on the IPA core clock, so determining the value
> to write for a given period requires access to the current core
> clock rate.  So the first patch just creates a new function to
> provide that.

Series applied, thank you.