[PATCH linux-phy 0/4] mvebu a3720 comphy: Fix serdes transmit amplitude

[PATCH linux-phy 0/4] mvebu a3720 comphy: Fix serdes transmit amplitude

[Marek Behún]

This series adds support for setting serdes transmit amplitude for
ethernet modes (sgmii, 1000base-x, 2500base-x) in the Marvell A3720
comphy driver.

The amplitude is set according to setting in device tree.

Finally the Turris MOX device tree is changed to set the 2500base-x
mode tx amplitude to 1025 mV.

This is needed to fix a weird issue wherein when A3720 sends a packet
to Topaz, and the packet contains a long sequence of 'J's or '\xb5'
bytes (these translate to '010101010101'... in 8b/10b encoding), the
packet may be lost on Topaz due to FCS error. The probability of
loss grows with number of 'J's:

  loss
                         ______
  100%                .-^
                     /
                    /
   50%             /
                  /
                 /
    0%  ______.-^
            90    114    125     number of consecutive 'J's

Marek Behún (4):
  string.h: Add str_has_proper_prefix()
  device property: Add {fwnode/device}_get_tx_p2p_amplitude()
  phy: marvell: phy-mvebu-a3700-comphy: Support changing tx amplitude
    for ethernet
  arm64: dts: armada-3720-turris-mox: Change comphy tx amplitude for
    2500base-x mode

 .../dts/marvell/armada-3720-turris-mox.dts    |  10 ++
 drivers/base/property.c                       | 130 ++++++++++++++++++
 drivers/phy/marvell/phy-mvebu-a3700-comphy.c  | 109 ++++++++++++++-
 include/linux/property.h                      |   5 +
 include/linux/string.h                        |  18 +++
 5 files changed, 271 insertions(+), 1 deletion(-)

-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH linux-phy 1/4] string.h: Add str_has_proper_prefix()

[Marek Behún]

Add str_has_proper_prefix(), similar to str_has_prefix(), but requires
that the prefix is proper: the string itself must be longer than the
prefix.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
Andy, Kees, could you ack this if it is ok?
---
 include/linux/string.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 61ec7e4f6311..375f51b9182c 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -316,4 +316,22 @@ static __always_inline size_t str_has_prefix(const char *str, const char *prefix
 	return strncmp(str, prefix, len) == 0 ? len : 0;
 }
 
+/**
+ * str_has_proper_prefix - Test if a string has a proper prefix
+ * @str: The string to test
+ * @prefix: The string to see if @str starts with
+ *
+ * This is like str_has_prefix(), but fails if the strings are equal.
+ *
+ * Returns:
+ * * strlen(@prefix) if @str starts with @prefix and they aren't equal
+ * * 0 otherwise
+ */
+static __always_inline size_t str_has_proper_prefix(const char *str,
+						    const char *prefix)
+{
+	size_t len = strlen(prefix);
+	return strncmp(str, prefix, len) == 0 && len != strlen(str) ? len : 0;
+}
+
 #endif /* _LINUX_STRING_H_ */
-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH linux-phy 2/4] device property: Add {fwnode/device}_get_tx_p2p_amplitude()

[Marek Behún]

Add functions fwnode_get_tx_p2p_amplitude() and
device_get_tx_p2p_amplitude() that parse the 'tx-p2p-microvolt' and
'tx-p2p-microvolt-names' properties and return peak to peak transmit
amplitude in microvolts for given PHY mode.

The functions search for mode name in 'tx-p2p-microvolt-names' property,
and return amplitude at the corresponding index in the 'tx-p2p-microvolt'
property.

If given mode is not matched in 'tx-p2p-microvolt-names' array, the mode
name is generalized (for example "pcie3" -> "pcie" -> "default", or
"usb-ss" -> "usb" -> "default").

If the 'tx-p2p-microvolt-names' is not present, the 'tx-p2p-microvolt'
property is expected to contain only one value, which is considered
default, and will be returned for any mode.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
Andy et al. can I get Ack for this if this is okay?
---
 drivers/base/property.c  | 130 +++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |   5 ++
 2 files changed, 135 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index ed6f449f8e5c..34b763436c30 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -921,6 +921,136 @@ int device_get_phy_mode(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_get_phy_mode);
 
+/**
+ * fwnode_get_tx_p2p_amplitude - Get peak to peak transmit amplitude for given
+ *				 PHY mode
+ * @fwnode:	Pointer to the given node
+ * @mode:	Name of the PHY mode, or "default" / NULL
+ * @amplitude:	Pointer where to store the amplitude
+ *
+ * Gets the peak to peak transmit amplitude in microvolts for a given PHY mode
+ * by parsing the 'tx-p2p-microvolt' and 'tx-p2p-microvolt-names' properties.
+ * If amplitude is not specified for @mode exactly, tries a more generic mode,
+ * and if that isn't specified, tries "default".
+ *
+ * For example if @mode is "pcie3", we first try searching for value
+ * corresponding to "pcie3", then to "pcie", and finally to "default".
+ *
+ * Return: %0 if the amplitude was read (success),
+ *	   %-EINVAL if given arguments are not valid,
+ *	   %-ENODATA if the required properties do not have a value,
+ *	   %-EPROTO if the property is not an array of strings,
+ *	   %-ENXIO if no suitable firmware interface is present,
+ *	   %-ENOMEM if out of memory.
+ */
+int fwnode_get_tx_p2p_amplitude(struct fwnode_handle *fwnode, const char *mode,
+				u32 *amplitude)
+{
+	static const char *names_prop = "tx-p2p-microvolt-names",
+			  *vals_prop = "tx-p2p-microvolt";
+	const char **names;
+	int cnt, idx, ret;
+	u32 *vals;
+
+	cnt = fwnode_property_string_array_count(fwnode, names_prop);
+	if (!cnt || cnt == -EINVAL)
+		/*
+		 * If the names property does not exist or is empty, we expect
+		 * the values property to contain only one, default value.
+		 */
+		return fwnode_property_read_u32(fwnode, vals_prop, amplitude);
+	else if (cnt < 0)
+		return cnt;
+
+	names = kcalloc(cnt, sizeof(*names), GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	ret = fwnode_property_read_string_array(fwnode, names_prop, names, cnt);
+	if (ret < 0) {
+		kfree(names);
+		return ret;
+	}
+
+	if (!mode)
+		mode = "default";
+
+	do {
+		static const char *gen_table[] = {
+			"pcie", "usb", "ufs-hs", "dp", "mipi-dphy",
+		};
+		size_t i;
+
+		idx = match_string(names, cnt, mode);
+		if (idx >= 0)
+			break;
+
+		/* If mode was not matched, try more generic mode */
+		for (i = 0; i < ARRAY_SIZE(gen_table); ++i) {
+			if (str_has_proper_prefix(mode, gen_table[i])) {
+				mode = gen_table[i];
+				break;
+			}
+		}
+
+		/* Or "default" */
+		if (i == ARRAY_SIZE(gen_table)) {
+			if (strcmp(mode, "default"))
+				mode = "default";
+			else
+				mode = NULL;
+		}
+	} while (mode);
+
+	kfree(names);
+
+	if (idx < 0)
+		return -ENODATA;
+
+	vals = kcalloc(cnt, sizeof(*vals), GFP_KERNEL);
+	if (!vals)
+		return -ENOMEM;
+
+	ret = fwnode_property_read_u32_array(fwnode, vals_prop, vals, cnt);
+	if (ret)
+		goto out;
+
+	*amplitude = vals[idx];
+out:
+	kfree(vals);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_tx_p2p_amplitude);
+
+/**
+ * device_get_tx_p2p_amplitude - Get peak to peak transmit amplitude for given
+ *				 PHY mode
+ * @dev:	Pointer to the given device
+ * @mode:	Name of the PHY mode, or "default" / NULL
+ * @amplitude:	Pointer where to store the amplitude
+ *
+ * Gets the peak to peak transmit amplitude in microvolts for a given PHY mode
+ * by parsing the 'tx-p2p-microvolt' and 'tx-p2p-microvolt-names' properties.
+ * If amplitude is not specified for @mode exactly, tries a more generic mode,
+ * and if that isn't specified, tries "default".
+ *
+ * For example if @mode is "pcie3", we first try searching for value
+ * corresponding to "pcie3", then to "pcie", and finally to "default".
+ *
+ * Return: %0 if the amplitude was read (success),
+ *	   %-EINVAL if given arguments are not valid,
+ *	   %-ENODATA if the required properties do not have a value,
+ *	   %-EPROTO if the property is not an array of strings,
+ *	   %-ENXIO if no suitable firmware interface is present,
+ *	   %-ENOMEM if out of memory.
+ */
+int device_get_tx_p2p_amplitude(struct device *dev, const char *mode,
+				u32 *amplitude)
+{
+	return fwnode_get_tx_p2p_amplitude(dev_fwnode(dev), mode, amplitude);
+}
+EXPORT_SYMBOL_GPL(device_get_tx_p2p_amplitude);
+
 /**
  * fwnode_iomap - Maps the memory mapped IO for a given fwnode
  * @fwnode:	Pointer to the firmware node
diff --git a/include/linux/property.h b/include/linux/property.h
index a5b429d623f6..91b12a79e245 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -392,6 +392,11 @@ const void *device_get_match_data(struct device *dev);
 int device_get_phy_mode(struct device *dev);
 int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
 
+int fwnode_get_tx_p2p_amplitude(struct fwnode_handle *fwnode, const char *mode,
+				u32 *amplitude);
+int device_get_tx_p2p_amplitude(struct device *dev, const char *mode,
+				u32 *amplitude);
+
 void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index);
 
 struct fwnode_handle *fwnode_graph_get_next_endpoint(
-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH linux-phy 3/4] phy: marvell: phy-mvebu-a3700-comphy: Support changing tx amplitude for ethernet

[Marek Behún]

Add support to set SerDes transmit amplitude if specified via the
'tx-p2p-microvolt' and 'tx-p2p-microvolt-names' device-tree properties.

This support is currently only for ethernet mode.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 109 ++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
index a4d7d9bd100d..7fabd959ae0f 100644
--- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
+++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
@@ -68,6 +68,16 @@
 #define SPEED_PLL_MASK			GENMASK(7, 2)
 #define SPEED_PLL_VALUE_16		FIELD_PREP(SPEED_PLL_MASK, 0x10)
 
+#define COMPHY_GEN1_SET0		0x0d
+#define COMPHY_GEN2_SET0		0x0f
+#define COMPHY_GEN3_SET0		0x11
+#define COMPHY_GEN4_SET0		0x13
+#define COMPHY_GENx_SET0(x)		(0x0d + (((x) & 3) - 1) * 2)
+#define Gx_TX_AMP_MASK			GENMASK(5, 1)
+#define Gx_TX_AMP_VALUE(x)		FIELD_PREP(Gx_TX_AMP_MASK, x)
+#define Gx_TX_AMP_ADJ			BIT(6)
+#define Gx_TX_AMP_1025MV		(Gx_TX_AMP_VALUE(0x12) | Gx_TX_AMP_ADJ)
+
 #define COMPHY_DIG_LOOPBACK_EN		0x23
 #define SEL_DATA_WIDTH_MASK		GENMASK(11, 10)
 #define DATA_WIDTH_10BIT		FIELD_PREP(SEL_DATA_WIDTH_MASK, 0x0)
@@ -269,6 +279,7 @@ struct mvebu_a3700_comphy_priv {
 struct mvebu_a3700_comphy_lane {
 	struct mvebu_a3700_comphy_priv *priv;
 	struct device *dev;
+	struct phy *phy;
 	unsigned int id;
 	enum phy_mode mode;
 	int submode;
@@ -385,6 +396,15 @@ static inline void comphy_reg_set16(void __iomem *addr, u16 data, u16 mask)
 }
 
 /* Used for accessing lane 2 registers (SATA/USB3 PHY) */
+static u16 comphy_get_indirect(struct mvebu_a3700_comphy_priv *priv, u32 offset)
+{
+	writel(offset,
+	       priv->lane2_phy_indirect + COMPHY_LANE2_INDIR_ADDR);
+
+	/* We need to read the register with 32-bit read */
+	return readl(priv->lane2_phy_indirect + COMPHY_LANE2_INDIR_DATA);
+}
+
 static void comphy_set_indirect(struct mvebu_a3700_comphy_priv *priv,
 				u32 offset, u16 data, u16 mask)
 {
@@ -394,6 +414,21 @@ static void comphy_set_indirect(struct mvebu_a3700_comphy_priv *priv,
 		       data, mask);
 }
 
+static u16 comphy_lane_reg_get(struct mvebu_a3700_comphy_lane *lane, u16 reg)
+{
+	if (lane->id == 2) {
+		/* lane 2 PHY registers are accessed indirectly */
+		return comphy_get_indirect(lane->priv,
+					   reg + COMPHY_LANE2_REGS_BASE);
+	} else {
+		void __iomem *base = lane->id == 1 ?
+				     lane->priv->lane1_phy_regs :
+				     lane->priv->lane0_phy_regs;
+
+		return readw(base + COMPHY_LANE_REG_DIRECT(reg));
+	}
+}
+
 static void comphy_lane_reg_set(struct mvebu_a3700_comphy_lane *lane,
 				u16 reg, u16 data, u16 mask)
 {
@@ -624,10 +659,53 @@ static void comphy_gbe_phy_init(struct mvebu_a3700_comphy_lane *lane,
 	}
 }
 
+static u8 comphy_find_best_tx_amp(bool full_swing, u32 amp, u32 *true_amp)
+{
+	static const u32 half_swing_table[32] = {
+		250, 270, 290, 310, 330, 345, 365, 380,
+		400, 420, 435, 455, 470, 490, 505, 525,
+		485, 520, 555, 590, 625, 660, 695, 730,
+		765, 800, 830, 865, 900, 930, 965, 1000,
+	};
+	static const u32 full_swing_table[22] = {
+		470, 505, 540, 575, 610, 645, 680, 715,
+		750, 785, 820, 850, 885, 915, 950, 980,
+		900, 965, 1025, 1095, 1160, 1220,
+	};
+	u32 diff, min_diff;
+	const u32 *table;
+	size_t len;
+	u8 res;
+
+	if (full_swing) {
+		table = full_swing_table;
+		len = ARRAY_SIZE(full_swing_table);
+	} else {
+		table = half_swing_table;
+		len = ARRAY_SIZE(half_swing_table);
+	}
+
+	res = 0;
+	min_diff = abs(amp - table[0]);
+
+	for (size_t i = 1; i < len; ++i) {
+		diff = abs(amp - table[i]);
+		if (diff < min_diff) {
+			min_diff = diff;
+			res = i;
+		}
+	}
+
+	if (true_amp)
+		*true_amp = table[res];
+
+	return res;
+}
+
 static int
 mvebu_a3700_comphy_ethernet_power_on(struct mvebu_a3700_comphy_lane *lane)
 {
-	u32 mask, data, speed_sel;
+	u32 mask, data, speed_sel, tx_amp_uv;
 	int ret;
 
 	/* Set selector */
@@ -746,6 +824,34 @@ mvebu_a3700_comphy_ethernet_power_on(struct mvebu_a3700_comphy_lane *lane)
 		comphy_gbe_phy_init(lane,
 				    lane->submode != PHY_INTERFACE_MODE_2500BASEX);
 
+	/*
+	 * Change transmit amplitude if specified in device-tree.
+	 */
+	if (!device_get_tx_p2p_amplitude(&lane->phy->dev,
+					 phy_modes(lane->submode),
+					 &tx_amp_uv)) {
+		u32 tx_amp_mv, true_tx_amp_mv;
+		bool full_swing;
+		u8 tx_amp;
+		u16 reg;
+
+		reg = COMPHY_GENx_SET0(speed_sel + 1);
+
+		data = comphy_lane_reg_get(lane, reg);
+		full_swing = data & Gx_TX_AMP_ADJ;
+		tx_amp_mv = DIV_ROUND_CLOSEST(tx_amp_uv, 1000);
+		tx_amp = comphy_find_best_tx_amp(tx_amp_mv, full_swing,
+						 &true_tx_amp_mv);
+
+		data = Gx_TX_AMP_VALUE(tx_amp);
+		mask = Gx_TX_AMP_MASK;
+		comphy_lane_reg_set(lane, reg, data, mask);
+
+		dev_dbg(lane->dev,
+			"changed tx amplitude to %u mV (requested %u mV) on lane %d\n",
+			true_tx_amp_mv, tx_amp_mv, lane->id);
+	}
+
 	/*
 	 * 14. Check the PHY Polarity invert bit
 	 */
@@ -1382,6 +1488,7 @@ static int mvebu_a3700_comphy_probe(struct platform_device *pdev)
 
 		lane->priv = priv;
 		lane->dev = &pdev->dev;
+		lane->phy = phy;
 		lane->mode = PHY_MODE_INVALID;
 		lane->submode = PHY_INTERFACE_MODE_NA;
 		lane->id = lane_id;
-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH linux-phy 3/4] phy: marvell: phy-mvebu-a3700-comphy: Support changing tx amplitude for ethernet

[Marek Behún]

On Wed, 17 Aug 2022 21:31:18 +0200
Marek Behún <kabel@kernel.org> wrote:

> +static u8 comphy_find_best_tx_amp(bool full_swing, u32 amp, u32 *true_amp)

...

> +		tx_amp = comphy_find_best_tx_amp(tx_amp_mv, full_swing,
> +						 &true_tx_amp_mv);

OMG, I changed the full_swing parameter to be the first one at the
last minute and forgot to also do it in the function call.

Will send v2.

Sorry.

Marek

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH linux-phy 4/4] arm64: dts: armada-3720-turris-mox: Change comphy tx amplitude for 2500base-x mode

[Marek Behún]

Change comphy transmit amplitude to 1025 mV for 2500base-x mode on
comphy connected to Topaz.

This fixes issue wherein if the 8b/10b encoded packet contains a long
enough alternating sequence of bits (010101... or 101010...), which
happens if the packet contains a sequence of 'J' or '\xb5' bytes, the
packet may be lost when sent from A3720 to Topaz due to FCS error. The
probability of loss grows with the number of 'J's with default transmit
amplitude setting - with 114 'J's the probability is about 50%, with 125
'J's almost 100% of packets  are lost.

Fixes: 7109d817db2e ("arm64: dts: marvell: add DTS for Turris Mox")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index ada164d423f3..74a7ac1f8ecb 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -181,6 +181,16 @@ &eth1 {
 	phys = <&comphy0 1>;
 };
 
+&comphy0 {
+	/*
+	 * Set peak to peak transmit amplitude to 1025 mV to fix issue wherein
+	 * a packet may be lost if it contains a long enough sequence of 'J'
+	 * or '\xb5' bytes.
+	 */
+	tx-p2p-microvolt = <1025000>;
+	tx-p2p-microvolt-names = "2500base-x";
+};
+
 &sdhci0 {
 	wp-inverted;
 	bus-width = <4>;
-- 
2.35.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy