Changes since v1:
- fix
static const char * array should probably be static const char * const
warning in patch 2
- fix wrong parameter order to function comphy_find_best_tx_amp()
in patch 3
Original cover letter:
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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 * const 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
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 <[email protected]>
---
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 @@ ð1 {
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
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 <[email protected]>
---
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(full_swing, tx_amp_mv,
+ &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
On Wed, Aug 17, 2022 at 11:09 PM Marek Behún <[email protected]> wrote:
>
> 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.
It's very specific to a domain. NAK for putting it to the generic
code, otherwise explain how it can be useful outside of the PHY world.
...
> + 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;
You may count the values and read them all, and then check the names
and compare count to the read values. In such a case you don't need
too many (overlapped) checks. I think the current implementation is
far from being optimal. Take your time and try to get rid of 20% of
lines in this function. I believe it's doable.
...
> + * 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".
Gets --> Get
tries --> try
Otherwise add a subject to the sentences.
--
With Best Regards,
Andy Shevchenko
On Thu, 18 Aug 2022 22:22:31 +0300
Andy Shevchenko <[email protected]> wrote:
> On Wed, Aug 17, 2022 at 11:09 PM Marek Behún <[email protected]> wrote:
> >
> > 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.
>
> It's very specific to a domain. NAK for putting it to the generic
> code, otherwise explain how it can be useful outside of the PHY world.
The property may be also useful for drivers that don't depend on
generic PHY subsystem. At least the mv88e6xxx DSA driver already reads
the property [1] although it simply uses of_property_read_u32(),
because it does not expect more complicated definition yet.
There are three subsystem which may want to use this function: generic
PHY, ethernet PHY and DSA. Since ethernet PHY subsystem nor DSA
subsystem do not depend on generic PHY, I thought putting it in base
would be sensible.
If you think it should be in generic PHY subsystem anyway, and that
other drivers needing it should depend on GENERIC_PHY, I can move it.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/mv88e6xxx/chip.c?h=v6.0-rc1#n3504
>
> ...
>
> > + 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;
>
> You may count the values and read them all,
What do you mean? Count the values and read them all via one
call to fwnode_property_string_array_count() ?
> and then check the names
> and compare count to the read values. In such a case you don't need
> too many (overlapped) checks. I think the current implementation is
> far from being optimal. Take your time and try to get rid of 20% of
> lines in this function. I believe it's doable.
>
> ...
>
> > + * 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".
>
> Gets --> Get
> tries --> try
>
> Otherwise add a subject to the sentences.
>
On Thu, 18 Aug 2022 22:10:58 +0300
Andy Shevchenko <[email protected]> wrote:
> On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <[email protected]> wrote:
> >
> > 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 <[email protected]>
> > ---
> > Andy, Kees, could you ack this if it is ok?
>
> Seems to me there are too many strlen():s. One is hidden in strncmp().
I thought this was ok cause gcc has optimizations for this in
https://github.com/gcc-mirror/gcc/blob/master/gcc/tree-ssa-strlen.cc
But now I see that kernel does not declare these functions as inline
that call __builtin_strlen()... so probably the optimizations are not
used.
> Besides not the good naming (what 'proper' means),
The naming comes from similar naming in math: proper subset is as
subset that is not equal to the superset. See
https://en.wikipedia.org/wiki/Substring :
"A proper prefix of a string is not equal to the string itself"
> the entire function is not needed. You may simply call
>
> str_has_prefix() && p[len] != '\0';
>
> Correct?
Do you mean that I should implement this function to simply return
str_has_prefix() && p[len] != '\0'
or that this function should not exist at all and I should do that in
the code where I would have used the function?
Thanks.
Marek
On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> Andy, Kees, could you ack this if it is ok?
Seems to me there are too many strlen():s. One is hidden in strncmp().
Besides not the good naming (what 'proper' means), the entire function
is not needed. You may simply call
str_has_prefix() && p[len] != '\0';
Correct?
--
With Best Regards,
Andy Shevchenko
On Thu, Aug 18, 2022 at 10:48 PM Marek Behún <[email protected]> wrote:
> On Thu, 18 Aug 2022 22:10:58 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <[email protected]> wrote:
...
> > Besides not the good naming (what 'proper' means),
>
> The naming comes from similar naming in math: proper subset is as
> subset that is not equal to the superset. See
> https://en.wikipedia.org/wiki/Substring :
> "A proper prefix of a string is not equal to the string itself"
It's nice to learn something, but I still think that name has too
broad meaning(s) that may easily confuse the developers.
>
> > the entire function is not needed. You may simply call
> >
> > str_has_prefix() && p[len] != '\0';
> >
> > Correct?
>
> Do you mean that I should implement this function to simply return
> str_has_prefix() && p[len] != '\0'
> or that this function should not exist at all and I should do that in
> the code where I would have used the function?
The latter since this seems do not have users, except a single newcomer,
--
With Best Regards,
Andy Shevchenko
On Thu, 18 Aug 2022 22:56:14 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, Aug 18, 2022 at 10:48 PM Marek Behún <[email protected]> wrote:
> > On Thu, 18 Aug 2022 22:10:58 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <[email protected]> wrote:
>
> ...
>
> > > Besides not the good naming (what 'proper' means),
> >
> > The naming comes from similar naming in math: proper subset is as
> > subset that is not equal to the superset. See
> > https://en.wikipedia.org/wiki/Substring :
> > "A proper prefix of a string is not equal to the string itself"
>
> It's nice to learn something, but I still think that name has too
> broad meaning(s) that may easily confuse the developers.
>
> >
> > > the entire function is not needed. You may simply call
> > >
> > > str_has_prefix() && p[len] != '\0';
> > >
> > > Correct?
> >
> > Do you mean that I should implement this function to simply return
> > str_has_prefix() && p[len] != '\0'
> > or that this function should not exist at all and I should do that in
> > the code where I would have used the function?
>
> The latter since this seems do not have users, except a single newcomer,
>
Very well. Thanks for the review.
Marek
On Thu, Aug 18, 2022 at 10:41 PM Marek Behún <[email protected]> wrote:
>
> On Thu, 18 Aug 2022 22:22:31 +0300
> Andy Shevchenko <[email protected]> wrote:
>
> > On Wed, Aug 17, 2022 at 11:09 PM Marek Behún <[email protected]> wrote:
> > >
> > > 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.
> >
> > It's very specific to a domain. NAK for putting it to the generic
> > code, otherwise explain how it can be useful outside of the PHY world.
>
> The property may be also useful for drivers that don't depend on
> generic PHY subsystem. At least the mv88e6xxx DSA driver already reads
> the property [1] although it simply uses of_property_read_u32(),
> because it does not expect more complicated definition yet.
>
> There are three subsystem which may want to use this function: generic
> PHY, ethernet PHY and DSA. Since ethernet PHY subsystem nor DSA
> subsystem do not depend on generic PHY, I thought putting it in base
> would be sensible.
>
> If you think it should be in generic PHY subsystem anyway, and that
> other drivers needing it should depend on GENERIC_PHY, I can move it.
Yes, I have no objection to put it there, just that the above
justification doesn't allow it to be in the generic code (yes, we may
still have some awkward APIs in the property.c and ideally they should
be moved to the respective subsystems).
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/mv88e6xxx/chip.c?h=v6.0-rc1#n3504
...
> > > + 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;
> >
> > You may count the values and read them all,
>
> What do you mean? Count the values and read them all via one
> call to fwnode_property_string_array_count() ?
No, you obviously may not read them via string_array APIs, esp. one
that is related to counting.
Count the vals first, read them all (it seems you need it in all
branches of your flow). Then count names and compare them to the
number of values, and so on... Also try to assign "default" only once.
> > and then check the names
> > and compare count to the read values. In such a case you don't need
> > too many (overlapped) checks. I think the current implementation is
> > far from being optimal. Take your time and try to get rid of 20% of
> > lines in this function. I believe it's doable.
--
With Best Regards,
Andy Shevchenko
On Thu, 18 Aug 2022 22:56:14 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, Aug 18, 2022 at 10:48 PM Marek Behún <[email protected]> wrote:
> > On Thu, 18 Aug 2022 22:10:58 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <[email protected]> wrote:
>
> ...
>
> > > Besides not the good naming (what 'proper' means),
> >
> > The naming comes from similar naming in math: proper subset is as
> > subset that is not equal to the superset. See
> > https://en.wikipedia.org/wiki/Substring :
> > "A proper prefix of a string is not equal to the string itself"
>
> It's nice to learn something, but I still think that name has too
> broad meaning(s) that may easily confuse the developers.
>
> >
> > > the entire function is not needed. You may simply call
> > >
> > > str_has_prefix() && p[len] != '\0';
> > >
> > > Correct?
> >
> > Do you mean that I should implement this function to simply return
> > str_has_prefix() && p[len] != '\0'
> > or that this function should not exist at all and I should do that in
> > the code where I would have used the function?
>
> The latter since this seems do not have users, except a single newcomer,
>
Just out of curiosity I grepped for all usages of str_has_prefix() and
there are some where it str_has_proper_prefix() could detect failure
earlier. For example in kernel/trace/trace_events_user.c in function
user_field_size().
Do you think I should propose converting those? There aren't that many
uses, so probably not.
Marek
On Thu, 18 Aug 2022 23:10:09 +0300
Andy Shevchenko <[email protected]> wrote:
> Yes, I have no objection to put it there, just that the above
> justification doesn't allow it to be in the generic code (yes, we may
> still have some awkward APIs in the property.c and ideally they should
> be moved to the respective subsystems).
OK
> > > You may count the values and read them all,
> >
> > What do you mean? Count the values and read them all via one
> > call to fwnode_property_string_array_count() ?
>
> No, you obviously may not read them via string_array APIs, esp. one
> that is related to counting.
>
> Count the vals first, read them all (it seems you need it in all
> branches of your flow). Then count names and compare them to the
> number of values, and so on... Also try to assign "default" only once.
1. there is one branch where I don't need to read the values: when the
"-names" property does not exist, the DT binding documentation says
that the value property should only contain one value, the default
one. So in that case I early return
return fwnode_property_read_u32(fwnode, vals_prop, amplitude);
2. I thought that I shouldn't check whether the size of the
"tx-p2p-microvolt-names" array is equal to the size of
"tx-p2p-microvolt". Rob Herring says (if I understand correctly) that
kernel shouldn't validate device-tree, that we have dt-schema for
that...
Marek