[PATCH v11 0/3] Add QCOM SNPS PHY overriding params support

[PATCH v11 0/3] Add QCOM SNPS PHY overriding params support

[Krishna Kurapati]

From: Krishna Kurapati <kriskura@qti.qualcomm.com>

Added support for overriding tuning parameters in QCOM SNPS PHY
from device tree. This parameter tuning is required to tune the
hs signal on dp/dm lines for electrical compliance to be successful.

Changes in v11:
Made changes to logs added in phy driver.
Fixed nitpicks in code.

Changes in v10:
Fixed patch headers.

changes in v9:
Fixed nitpick in driver code.

changes in v8:
Fixed nitpick in driver code.

changes in v7:
Fixed nitpick in driver code and dtsi file.

changes in v6:
Fixed errors in dt-bindings.
Fixed nitpick in driver code.

changes in v5:
Fixed nitpicks in code.
Added minimum and maximum for each parameter added in dt-bindings.
Added proper suffixes to each parameter as per dtschema.

changes in v4:
Fixed nitpicks in code.
Initial compliance test results showed overshoot in the middle of eye
diagram. The current dt values were put in place to correct it and fix
overshoot issue.

changes in v3:
Added support for phy tuning parameters to be represented in bps and
corresponding register values to be written are obtained by traversing
through data map declared in the driver.

changes in v2:
Reading the individual fields in each overriding register from
device tree.

Krishna Kurapati (2):
  phy: qcom-snps: Add support for overriding phy tuning parameters
  arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280 IDP device

Sandeep Maheswaram (1):
  dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params
    bindings

 .../bindings/phy/qcom,usb-snps-femto-v2.yaml       |  88 +++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           |   6 +
 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c      | 262 ++++++++++++++++++++-
 3 files changed, 354 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v11 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings

[Krishna Kurapati]

From: Sandeep Maheswaram <quic_c_sanm@quicinc.com>

Add device tree bindings for SNPS phy tuning parameters.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 88 ++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
index 7a0e6a9..565ce5f 100644
--- a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
@@ -53,6 +53,94 @@ properties:
   vdda33-supply:
     description: phandle to the regulator 3.3V supply node.
 
+  qcom,hs-disconnect-bp:
+    description:
+      This adjusts the voltage level for the threshold used to
+      detect a disconnect event at the host.
+      The hardware accepts only discrete values. The value closest to the
+      provided input will be chosen as the override value for this param.
+    minimum: -272
+    maximum: 2156
+
+  qcom,squelch-detector-bp:
+    description:
+      This adjusts the voltage level for the threshold used to
+      detect valid high-speed data.
+      The hardware accepts only discrete values. The value closest to the
+      provided input will be chosen as the override value for this param.
+    minimum: -2090
+    maximum: 1590
+
+  qcom,hs-amplitude-bp:
+    description:
+      This adjusts the high-speed DC level voltage.
+      The hardware accepts only discrete values. The value closest to the
+      provided input will be chosen as the override value for this param.
+    minimum: -660
+    maximum: 2670
+
+  qcom,pre-emphasis-duration-bp:
+    description:
+      This signal controls the duration for which the
+      HS pre-emphasis current is sourced onto DP<#> or DM<#>.
+      The HS Transmitter pre-emphasis duration is defined in terms of
+      unit amounts. One unit of pre-emphasis duration is approximately
+      650 ps and is defined as 1X pre-emphasis duration.
+      The hardware accepts only discrete values. The value closest to the
+      provided input will be chosen as the override value for this param.
+    minimum: 10000
+    maximum: 20000
+
+  qcom,pre-emphasis-amplitude-bp:
+    description:
+      This signal controls the amount of current sourced to
+      DP<#> and DM<#> after a J-to-K or K-to-J transition.
+      The HS Transmitter pre-emphasis current is defined in terms of unit
+      amounts. One unit amount is approximately 2 mA and is defined as
+      1X pre-emphasis current.
+      The hardware accepts only discrete values. The value closest to the
+      provided input will be chosen as the override value for this param.
+    minimum: 10000
+    maximum: 40000
+
+  qcom,hs-rise-fall-time-bp:
+    description:
+      This adjusts the rise/fall times of the high-speed waveform.
+      The hardware accepts only discrete values. The value closest to the
+      provided input will be chosen as the override value for this param.
+    minimum: -4100
+    maximum: 5430
+
+  qcom,hs-crossover-voltage-microvolt:
+    description:
+      This adjusts the voltage at which the DP<#> and DM<#>
+      signals cross while transmitting in HS mode.
+      The hardware accepts only discrete values. The value closest to the
+      provided input will be chosen as the override value for this param.
+    minimum: -31000
+    maximum: 28000
+
+  qcom,hs-output-impedance-micro-ohms:
+    description:
+      In some applications, there can be significant series resistance
+      on the D+ and D- paths between the transceiver and cable. This adjusts
+      the driver source impedance to compensate for added series
+      resistance on the USB. The hardware accepts only discrete values. The
+      value closest to the provided input will be chosen as the override value
+      for this param.
+    minimum: -2300000
+    maximum: 6100000
+
+  qcom,ls-fs-output-impedance-bp:
+    description:
+      This adjusts the low- and full-speed single-ended source
+      impedance while driving high. The following adjustment values are based
+      on nominal process, voltage, and temperature.
+      The hardware accepts only discrete values. The value closest to the
+      provided input will be chosen as the override value for this param.
+    minimum: -1053
+    maximum: 1310
+
 required:
   - compatible
   - reg
-- 
2.7.4

[PATCH v11 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters

[Krishna Kurapati]

Add support for overriding electrical signal tuning parameters for
SNPS HS Phy.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Reviewed-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 262 +++++++++++++++++++++++++-
 1 file changed, 260 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
index 5d20378..24264ad 100644
--- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
+++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
@@ -52,6 +52,12 @@
 #define USB2_SUSPEND_N				BIT(2)
 #define USB2_SUSPEND_N_SEL			BIT(3)
 
+#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0		(0x6c)
+#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1		(0x70)
+#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2		(0x74)
+#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3		(0x78)
+#define PARAM_OVRD_MASK				0xFF
+
 #define USB2_PHY_USB_PHY_CFG0			(0x94)
 #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN	BIT(0)
 #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN		BIT(1)
@@ -60,12 +66,69 @@
 #define REFCLK_SEL_MASK				GENMASK(1, 0)
 #define REFCLK_SEL_DEFAULT			(0x2 << 0)
 
+#define HS_DISCONNECT_MASK			GENMASK(2, 0)
+#define SQUELCH_DETECTOR_MASK			GENMASK(7, 5)
+
+#define HS_AMPLITUDE_MASK			GENMASK(3, 0)
+#define PREEMPHASIS_DURATION_MASK		BIT(5)
+#define PREEMPHASIS_AMPLITUDE_MASK		GENMASK(7, 6)
+
+#define HS_RISE_FALL_MASK			GENMASK(1, 0)
+#define HS_CROSSOVER_VOLTAGE_MASK		GENMASK(3, 2)
+#define HS_OUTPUT_IMPEDANCE_MASK		GENMASK(5, 4)
+
+#define LS_FS_OUTPUT_IMPEDANCE_MASK		GENMASK(3, 0)
+
 static const char * const qcom_snps_hsphy_vreg_names[] = {
 	"vdda-pll", "vdda33", "vdda18",
 };
 
 #define SNPS_HS_NUM_VREGS		ARRAY_SIZE(qcom_snps_hsphy_vreg_names)
 
+struct override_param {
+	s32	value;
+	u8	reg;
+};
+
+#define OVERRIDE_PARAM(bps, val) {	\
+	.value = bps,			\
+	.reg = val,			\
+}
+
+struct override_param_map {
+	const struct override_param *param_table;
+	u8 table_size;
+	u8 reg_offset;
+	u8 param_mask;
+};
+
+#define OVERRIDE_PARAM_MAP(table, num_elements, offset, mask)		\
+{									\
+	.param_table = table,						\
+	.table_size = num_elements,					\
+	.reg_offset = offset,						\
+	.param_mask = mask,						\
+}
+
+struct phy_override_seq {
+	bool	need_update;
+	u8	offset;
+	u8	value;
+	u8	mask;
+};
+
+static const char * const phy_seq_props[] = {
+	"qcom,hs-disconnect-bp",
+	"qcom,squelch-detector-bp",
+	"qcom,hs-amplitude-bp",
+	"qcom,pre-emphasis-duration-bp",
+	"qcom,pre-emphasis-amplitude-bp",
+	"qcom,hs-rise-fall-time-bp",
+	"qcom,hs-crossover-voltage-microvolt",
+	"qcom,hs-output-impedance-micro-ohms",
+	"qcom,ls-fs-output-impedance-bp",
+};
+
 /**
  * struct qcom_snps_hsphy - snps hs phy attributes
  *
@@ -91,6 +154,7 @@ struct qcom_snps_hsphy {
 
 	bool phy_initialized;
 	enum phy_mode mode;
+	struct phy_override_seq update_seq_cfg[ARRAY_SIZE(phy_seq_props)];
 };
 
 static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
@@ -173,10 +237,147 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
 	return 0;
 }
 
+static const struct override_param hs_disconnect_sc7280[] = {
+	OVERRIDE_PARAM(-272, 0),
+	OVERRIDE_PARAM(0, 1),
+	OVERRIDE_PARAM(317, 2),
+	OVERRIDE_PARAM(630, 3),
+	OVERRIDE_PARAM(973, 4),
+	OVERRIDE_PARAM(1332, 5),
+	OVERRIDE_PARAM(1743, 6),
+	OVERRIDE_PARAM(2156, 7),
+};
+
+static const struct override_param squelch_det_threshold_sc7280[] = {
+	OVERRIDE_PARAM(-2090, 7),
+	OVERRIDE_PARAM(-1560, 6),
+	OVERRIDE_PARAM(-1030, 5),
+	OVERRIDE_PARAM(-530, 4),
+	OVERRIDE_PARAM(0, 3),
+	OVERRIDE_PARAM(530, 2),
+	OVERRIDE_PARAM(1060, 1),
+	OVERRIDE_PARAM(1590, 0),
+};
+
+static const struct override_param hs_amplitude_sc7280[] = {
+	OVERRIDE_PARAM(-660, 0),
+	OVERRIDE_PARAM(-440, 1),
+	OVERRIDE_PARAM(-220, 2),
+	OVERRIDE_PARAM(0, 3),
+	OVERRIDE_PARAM(230, 4),
+	OVERRIDE_PARAM(440, 5),
+	OVERRIDE_PARAM(650, 6),
+	OVERRIDE_PARAM(890, 7),
+	OVERRIDE_PARAM(1110, 8),
+	OVERRIDE_PARAM(1330, 9),
+	OVERRIDE_PARAM(1560, 10),
+	OVERRIDE_PARAM(1780, 11),
+	OVERRIDE_PARAM(2000, 12),
+	OVERRIDE_PARAM(2220, 13),
+	OVERRIDE_PARAM(2430, 14),
+	OVERRIDE_PARAM(2670, 15),
+};
+
+static const struct override_param preemphasis_duration_sc7280[] = {
+	OVERRIDE_PARAM(10000, 1),
+	OVERRIDE_PARAM(20000, 0),
+};
+
+static const struct override_param preemphasis_amplitude_sc7280[] = {
+	OVERRIDE_PARAM(10000, 1),
+	OVERRIDE_PARAM(20000, 2),
+	OVERRIDE_PARAM(30000, 3),
+	OVERRIDE_PARAM(40000, 0),
+};
+
+static const struct override_param hs_rise_fall_time_sc7280[] = {
+	OVERRIDE_PARAM(-4100, 3),
+	OVERRIDE_PARAM(0, 2),
+	OVERRIDE_PARAM(2810, 1),
+	OVERRIDE_PARAM(5430, 0),
+};
+
+static const struct override_param hs_crossover_voltage_sc7280[] = {
+	OVERRIDE_PARAM(-31000, 1),
+	OVERRIDE_PARAM(0, 3),
+	OVERRIDE_PARAM(28000, 2),
+};
+
+static const struct override_param hs_output_impedance_sc7280[] = {
+	OVERRIDE_PARAM(-2300000, 3),
+	OVERRIDE_PARAM(0, 2),
+	OVERRIDE_PARAM(2600000, 1),
+	OVERRIDE_PARAM(6100000, 0),
+};
+
+static const struct override_param ls_fs_output_impedance_sc7280[] = {
+	OVERRIDE_PARAM(-1053, 15),
+	OVERRIDE_PARAM(-557, 7),
+	OVERRIDE_PARAM(0, 3),
+	OVERRIDE_PARAM(612, 1),
+	OVERRIDE_PARAM(1310, 0),
+};
+
+static const struct override_param_map sc7280[] = {
+	OVERRIDE_PARAM_MAP(
+			hs_disconnect_sc7280,
+			ARRAY_SIZE(hs_disconnect_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
+			HS_DISCONNECT_MASK),
+
+	OVERRIDE_PARAM_MAP(
+			squelch_det_threshold_sc7280,
+			ARRAY_SIZE(squelch_det_threshold_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
+			SQUELCH_DETECTOR_MASK),
+
+	OVERRIDE_PARAM_MAP(
+			hs_amplitude_sc7280,
+			ARRAY_SIZE(hs_amplitude_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
+			HS_AMPLITUDE_MASK),
+
+	OVERRIDE_PARAM_MAP(
+			preemphasis_duration_sc7280,
+			ARRAY_SIZE(preemphasis_duration_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
+			PREEMPHASIS_DURATION_MASK),
+
+	OVERRIDE_PARAM_MAP(
+			preemphasis_amplitude_sc7280,
+			ARRAY_SIZE(preemphasis_amplitude_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
+			PREEMPHASIS_AMPLITUDE_MASK),
+
+	OVERRIDE_PARAM_MAP(
+			hs_rise_fall_time_sc7280,
+			ARRAY_SIZE(hs_rise_fall_time_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
+			HS_RISE_FALL_MASK),
+
+	OVERRIDE_PARAM_MAP(
+			hs_crossover_voltage_sc7280,
+			ARRAY_SIZE(hs_crossover_voltage_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
+			HS_CROSSOVER_VOLTAGE_MASK),
+
+	OVERRIDE_PARAM_MAP(
+			hs_output_impedance_sc7280,
+			ARRAY_SIZE(hs_output_impedance_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
+			HS_OUTPUT_IMPEDANCE_MASK),
+
+	OVERRIDE_PARAM_MAP(
+			ls_fs_output_impedance_sc7280,
+			ARRAY_SIZE(ls_fs_output_impedance_sc7280),
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3,
+			LS_FS_OUTPUT_IMPEDANCE_MASK),
+};
+
 static int qcom_snps_hsphy_init(struct phy *phy)
 {
 	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
-	int ret;
+	int ret, i;
 
 	dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
 
@@ -223,6 +424,14 @@ static int qcom_snps_hsphy_init(struct phy *phy)
 	qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1,
 					VBUSVLDEXT0, VBUSVLDEXT0);
 
+	for (i = 0; i < ARRAY_SIZE(hsphy->update_seq_cfg); i++) {
+		if (hsphy->update_seq_cfg[i].need_update)
+			qcom_snps_hsphy_write_mask(hsphy->base,
+					hsphy->update_seq_cfg[i].offset,
+					hsphy->update_seq_cfg[i].mask,
+					hsphy->update_seq_cfg[i].value);
+	}
+
 	qcom_snps_hsphy_write_mask(hsphy->base,
 					USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2,
 					VREGBYPASS, VREGBYPASS);
@@ -280,7 +489,10 @@ static const struct phy_ops qcom_snps_hsphy_gen_ops = {
 static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
 	{ .compatible	= "qcom,sm8150-usb-hs-phy", },
 	{ .compatible	= "qcom,usb-snps-hs-5nm-phy", },
-	{ .compatible	= "qcom,usb-snps-hs-7nm-phy", },
+	{
+		.compatible	= "qcom,usb-snps-hs-7nm-phy",
+		.data		= &sc7280,
+	},
 	{ .compatible	= "qcom,usb-snps-femto-v2-phy",	},
 	{ }
 };
@@ -291,6 +503,51 @@ static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
 			   qcom_snps_hsphy_runtime_resume, NULL)
 };
 
+static void qcom_snps_hsphy_override_param_update_val(
+			const struct override_param_map map,
+			s32 dt_val, struct phy_override_seq *seq_entry)
+{
+	int i;
+
+	/*
+	 * Param table for each param is in increasing order
+	 * of dt values. We need to iterate over the list to
+	 * select the entry that has equal or the next highest value.
+	 */
+	for (i = 0; i < map.table_size - 1; i++) {
+		if (map.param_table[i].value == dt_val)
+			break;
+	}
+
+	seq_entry->need_update = true;
+	seq_entry->offset = map.reg_offset;
+	seq_entry->mask = map.param_mask;
+	seq_entry->value =  map.param_table[i].reg << __ffs(map.param_mask);
+}
+
+static void qcom_snps_hsphy_read_override_param_seq(struct device *dev)
+{
+	struct device_node *node = dev->of_node;
+	s32 val;
+	int ret, i;
+	struct qcom_snps_hsphy *hsphy;
+	const struct override_param_map *cfg = of_device_get_match_data(dev);
+
+	hsphy = dev_get_drvdata(dev);
+
+	for (i = 0; i < ARRAY_SIZE(phy_seq_props); i++) {
+		ret = of_property_read_s32(node, phy_seq_props[i], &val);
+		if (ret)
+			continue;
+
+		qcom_snps_hsphy_override_param_update_val(cfg[i], val,
+					&hsphy->update_seq_cfg[i]);
+		dev_dbg(&hsphy->phy->dev, "Read param: %s dt_val: %d reg_val: 0x%x\n",
+			phy_seq_props[i], val, hsphy->update_seq_cfg[i].value);
+
+	}
+}
+
 static int qcom_snps_hsphy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -352,6 +609,7 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(dev, hsphy);
 	phy_set_drvdata(generic_phy, hsphy);
+	qcom_snps_hsphy_read_override_param_seq(dev);
 
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 	if (!IS_ERR(phy_provider))
-- 
2.7.4

[PATCH v11 3/3] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280 IDP device

[Krishna Kurapati]

Overriding the SNPS Phy tuning parameters for SC7280 IDP device.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 5eb6689..2ae86840 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -325,6 +325,12 @@
 	vdda-pll-supply = <&vreg_l10c_0p8>;
 	vdda33-supply = <&vreg_l2b_3p0>;
 	vdda18-supply = <&vreg_l1c_1p8>;
+	qcom,hs-rise-fall-time-bp = <0>;
+	qcom,squelch-detector-bp = <(-2090)>;
+	qcom,hs-disconnect-bp = <1743>;
+	qcom,hs-amplitude-bp = <1780>;
+	qcom,hs-crossover-voltage-microvolt = <(-31000)>;
+	qcom,hs-output-impedance-micro-ohms = <2600000>;
 };
 
 &usb_1_qmpphy {
-- 
2.7.4

Re: [PATCH v11 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters

[Bjorn Andersson]

On Thu, Jul 21, 2022 at 11:59:13AM +0530, Krishna Kurapati wrote:
> Add support for overriding electrical signal tuning parameters for
> SNPS HS Phy.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 262 +++++++++++++++++++++++++-
>  1 file changed, 260 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> index 5d20378..24264ad 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> @@ -52,6 +52,12 @@
>  #define USB2_SUSPEND_N				BIT(2)
>  #define USB2_SUSPEND_N_SEL			BIT(3)
>  
> +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0		(0x6c)
> +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1		(0x70)
> +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2		(0x74)
> +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3		(0x78)
> +#define PARAM_OVRD_MASK				0xFF
> +
>  #define USB2_PHY_USB_PHY_CFG0			(0x94)
>  #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN	BIT(0)
>  #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN		BIT(1)
> @@ -60,12 +66,69 @@
>  #define REFCLK_SEL_MASK				GENMASK(1, 0)
>  #define REFCLK_SEL_DEFAULT			(0x2 << 0)
>  
> +#define HS_DISCONNECT_MASK			GENMASK(2, 0)
> +#define SQUELCH_DETECTOR_MASK			GENMASK(7, 5)
> +
> +#define HS_AMPLITUDE_MASK			GENMASK(3, 0)
> +#define PREEMPHASIS_DURATION_MASK		BIT(5)
> +#define PREEMPHASIS_AMPLITUDE_MASK		GENMASK(7, 6)
> +
> +#define HS_RISE_FALL_MASK			GENMASK(1, 0)
> +#define HS_CROSSOVER_VOLTAGE_MASK		GENMASK(3, 2)
> +#define HS_OUTPUT_IMPEDANCE_MASK		GENMASK(5, 4)
> +
> +#define LS_FS_OUTPUT_IMPEDANCE_MASK		GENMASK(3, 0)
> +
>  static const char * const qcom_snps_hsphy_vreg_names[] = {
>  	"vdda-pll", "vdda33", "vdda18",
>  };
>  
>  #define SNPS_HS_NUM_VREGS		ARRAY_SIZE(qcom_snps_hsphy_vreg_names)
>  
> +struct override_param {
> +	s32	value;
> +	u8	reg;

It wasn't immediately obvious to me that reg meant "register value to be
written for the given value specified in DT".

I think if you changed "reg" to "reg_val" it would have been.

> +};
> +
> +#define OVERRIDE_PARAM(bps, val) {	\

TBH, I don't find this macro to add any value. In order to understand
the tables below I need to look at this macro to know that the two
integers passed to the macro are just stuffed into the array - there's
nothing more to it.

As such I would prefer if you simply wrote:

static const struct override_param hs_disconnect_sc7280[] = {
	{ -272, 0 },
	{ 0, 1 },
	{ 317, 2 },
	...
}

> +	.value = bps,			\
> +	.reg = val,			\
> +}
> +
> +struct override_param_map {
> +	const struct override_param *param_table;
> +	u8 table_size;
> +	u8 reg_offset;
> +	u8 param_mask;
> +};
> +
> +#define OVERRIDE_PARAM_MAP(table, num_elements, offset, mask)		\
> +{									\
> +	.param_table = table,						\
> +	.table_size = num_elements,					\
> +	.reg_offset = offset,						\
> +	.param_mask = mask,						\
> +}

Here you could have used the preprocessor to fill in table_size (by just
making it ARRAY_SIZE(table)). But as with OVERRIDE_PARAM I would prefer
if you simply do:


static const struct override_param_map sc7280[] = {
	{
		hs_disconnect_sc7280,
		ARRAY_SIZE(hs_disconnect_sc7280),
		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
		HS_DISCONNECT_MASK),
	},
	{
		squelch_det_threshold_sc7280,
		ARRAY_SIZE(squelch_det_threshold_sc7280),
		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
		SQUELCH_DETECTOR_MASK),
	},
	...
}

> +
> +struct phy_override_seq {
> +	bool	need_update;
> +	u8	offset;
> +	u8	value;
> +	u8	mask;
> +};
> +
> +static const char * const phy_seq_props[] = {
> +	"qcom,hs-disconnect-bp",

The ordering of this list needs to match the order of
override_param_map[] and there's nothing indicating this in the code.

I was considering suggesting that you add a enum/define and do
	[SQUELCH_DETECTOR_BP] = "qcom,squelch-detector-bp",
	... 
and then do the same in the override_param_map array.


But I think it will be cleaner if you add a const char const pointer to
override_param_map and just specify these strings in the
override_param_map array.

Each entry will grow by a pointer, but multiple copies of the same
strings (when added in the future) should be combined by the compiler.

> +	"qcom,squelch-detector-bp",
> +	"qcom,hs-amplitude-bp",
> +	"qcom,pre-emphasis-duration-bp",
> +	"qcom,pre-emphasis-amplitude-bp",
> +	"qcom,hs-rise-fall-time-bp",
> +	"qcom,hs-crossover-voltage-microvolt",
> +	"qcom,hs-output-impedance-micro-ohms",
> +	"qcom,ls-fs-output-impedance-bp",
> +};
> +
>  /**
>   * struct qcom_snps_hsphy - snps hs phy attributes
>   *
> @@ -91,6 +154,7 @@ struct qcom_snps_hsphy {
>  
>  	bool phy_initialized;
>  	enum phy_mode mode;
> +	struct phy_override_seq update_seq_cfg[ARRAY_SIZE(phy_seq_props)];
>  };
>  
>  static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
> @@ -173,10 +237,147 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
>  	return 0;
>  }
>  
> +static const struct override_param hs_disconnect_sc7280[] = {
> +	OVERRIDE_PARAM(-272, 0),
> +	OVERRIDE_PARAM(0, 1),
> +	OVERRIDE_PARAM(317, 2),
> +	OVERRIDE_PARAM(630, 3),
> +	OVERRIDE_PARAM(973, 4),
> +	OVERRIDE_PARAM(1332, 5),
> +	OVERRIDE_PARAM(1743, 6),
> +	OVERRIDE_PARAM(2156, 7),
> +};
> +
> +static const struct override_param squelch_det_threshold_sc7280[] = {
> +	OVERRIDE_PARAM(-2090, 7),
> +	OVERRIDE_PARAM(-1560, 6),
> +	OVERRIDE_PARAM(-1030, 5),
> +	OVERRIDE_PARAM(-530, 4),
> +	OVERRIDE_PARAM(0, 3),
> +	OVERRIDE_PARAM(530, 2),
> +	OVERRIDE_PARAM(1060, 1),
> +	OVERRIDE_PARAM(1590, 0),
> +};
> +
> +static const struct override_param hs_amplitude_sc7280[] = {
> +	OVERRIDE_PARAM(-660, 0),
> +	OVERRIDE_PARAM(-440, 1),
> +	OVERRIDE_PARAM(-220, 2),
> +	OVERRIDE_PARAM(0, 3),
> +	OVERRIDE_PARAM(230, 4),
> +	OVERRIDE_PARAM(440, 5),
> +	OVERRIDE_PARAM(650, 6),
> +	OVERRIDE_PARAM(890, 7),
> +	OVERRIDE_PARAM(1110, 8),
> +	OVERRIDE_PARAM(1330, 9),
> +	OVERRIDE_PARAM(1560, 10),
> +	OVERRIDE_PARAM(1780, 11),
> +	OVERRIDE_PARAM(2000, 12),
> +	OVERRIDE_PARAM(2220, 13),
> +	OVERRIDE_PARAM(2430, 14),
> +	OVERRIDE_PARAM(2670, 15),
> +};
> +
> +static const struct override_param preemphasis_duration_sc7280[] = {
> +	OVERRIDE_PARAM(10000, 1),
> +	OVERRIDE_PARAM(20000, 0),
> +};
> +
> +static const struct override_param preemphasis_amplitude_sc7280[] = {
> +	OVERRIDE_PARAM(10000, 1),
> +	OVERRIDE_PARAM(20000, 2),
> +	OVERRIDE_PARAM(30000, 3),
> +	OVERRIDE_PARAM(40000, 0),
> +};
> +
> +static const struct override_param hs_rise_fall_time_sc7280[] = {
> +	OVERRIDE_PARAM(-4100, 3),
> +	OVERRIDE_PARAM(0, 2),
> +	OVERRIDE_PARAM(2810, 1),
> +	OVERRIDE_PARAM(5430, 0),
> +};
> +
> +static const struct override_param hs_crossover_voltage_sc7280[] = {
> +	OVERRIDE_PARAM(-31000, 1),
> +	OVERRIDE_PARAM(0, 3),
> +	OVERRIDE_PARAM(28000, 2),
> +};
> +
> +static const struct override_param hs_output_impedance_sc7280[] = {
> +	OVERRIDE_PARAM(-2300000, 3),
> +	OVERRIDE_PARAM(0, 2),
> +	OVERRIDE_PARAM(2600000, 1),
> +	OVERRIDE_PARAM(6100000, 0),
> +};
> +
> +static const struct override_param ls_fs_output_impedance_sc7280[] = {
> +	OVERRIDE_PARAM(-1053, 15),
> +	OVERRIDE_PARAM(-557, 7),
> +	OVERRIDE_PARAM(0, 3),
> +	OVERRIDE_PARAM(612, 1),
> +	OVERRIDE_PARAM(1310, 0),
> +};
> +
> +static const struct override_param_map sc7280[] = {

There's nothing ensuring that the loop below doesn't run off the end of
this array. So when the next platform is added, there's no way to
handle the fact that they might have a different set of properties.

If you add the property name to these elements, that will no longer be a
problem (and you can add a {} entry at the end of the list and check for
this when looping over the elements.

> +	OVERRIDE_PARAM_MAP(
> +			hs_disconnect_sc7280,
> +			ARRAY_SIZE(hs_disconnect_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
> +			HS_DISCONNECT_MASK),
> +
> +	OVERRIDE_PARAM_MAP(
> +			squelch_det_threshold_sc7280,
> +			ARRAY_SIZE(squelch_det_threshold_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
> +			SQUELCH_DETECTOR_MASK),
> +
> +	OVERRIDE_PARAM_MAP(
> +			hs_amplitude_sc7280,
> +			ARRAY_SIZE(hs_amplitude_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
> +			HS_AMPLITUDE_MASK),
> +
> +	OVERRIDE_PARAM_MAP(
> +			preemphasis_duration_sc7280,
> +			ARRAY_SIZE(preemphasis_duration_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
> +			PREEMPHASIS_DURATION_MASK),
> +
> +	OVERRIDE_PARAM_MAP(
> +			preemphasis_amplitude_sc7280,
> +			ARRAY_SIZE(preemphasis_amplitude_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
> +			PREEMPHASIS_AMPLITUDE_MASK),
> +
> +	OVERRIDE_PARAM_MAP(
> +			hs_rise_fall_time_sc7280,
> +			ARRAY_SIZE(hs_rise_fall_time_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
> +			HS_RISE_FALL_MASK),
> +
> +	OVERRIDE_PARAM_MAP(
> +			hs_crossover_voltage_sc7280,
> +			ARRAY_SIZE(hs_crossover_voltage_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
> +			HS_CROSSOVER_VOLTAGE_MASK),
> +
> +	OVERRIDE_PARAM_MAP(
> +			hs_output_impedance_sc7280,
> +			ARRAY_SIZE(hs_output_impedance_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
> +			HS_OUTPUT_IMPEDANCE_MASK),
> +
> +	OVERRIDE_PARAM_MAP(
> +			ls_fs_output_impedance_sc7280,
> +			ARRAY_SIZE(ls_fs_output_impedance_sc7280),
> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3,
> +			LS_FS_OUTPUT_IMPEDANCE_MASK),
> +};
> +
>  static int qcom_snps_hsphy_init(struct phy *phy)
>  {
>  	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
> -	int ret;
> +	int ret, i;
>  
>  	dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
>  
> @@ -223,6 +424,14 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>  	qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1,
>  					VBUSVLDEXT0, VBUSVLDEXT0);
>  
> +	for (i = 0; i < ARRAY_SIZE(hsphy->update_seq_cfg); i++) {
> +		if (hsphy->update_seq_cfg[i].need_update)
> +			qcom_snps_hsphy_write_mask(hsphy->base,
> +					hsphy->update_seq_cfg[i].offset,
> +					hsphy->update_seq_cfg[i].mask,
> +					hsphy->update_seq_cfg[i].value);
> +	}
> +
>  	qcom_snps_hsphy_write_mask(hsphy->base,
>  					USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2,
>  					VREGBYPASS, VREGBYPASS);
> @@ -280,7 +489,10 @@ static const struct phy_ops qcom_snps_hsphy_gen_ops = {
>  static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
>  	{ .compatible	= "qcom,sm8150-usb-hs-phy", },
>  	{ .compatible	= "qcom,usb-snps-hs-5nm-phy", },
> -	{ .compatible	= "qcom,usb-snps-hs-7nm-phy", },
> +	{
> +		.compatible	= "qcom,usb-snps-hs-7nm-phy",
> +		.data		= &sc7280,

Perhaps name "qcom_snps_hsphy_7nm_override_map" or something
along those lines instead of "sc7280"?

> +	},
>  	{ .compatible	= "qcom,usb-snps-femto-v2-phy",	},
>  	{ }
>  };
> @@ -291,6 +503,51 @@ static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
>  			   qcom_snps_hsphy_runtime_resume, NULL)
>  };
>  
> +static void qcom_snps_hsphy_override_param_update_val(
> +			const struct override_param_map map,
> +			s32 dt_val, struct phy_override_seq *seq_entry)
> +{
> +	int i;
> +
> +	/*
> +	 * Param table for each param is in increasing order
> +	 * of dt values. We need to iterate over the list to
> +	 * select the entry that has equal or the next highest value.
> +	 */
> +	for (i = 0; i < map.table_size - 1; i++) {
> +		if (map.param_table[i].value == dt_val)
> +			break;
> +	}
> +
> +	seq_entry->need_update = true;
> +	seq_entry->offset = map.reg_offset;
> +	seq_entry->mask = map.param_mask;
> +	seq_entry->value =  map.param_table[i].reg << __ffs(map.param_mask);

I would prefer that you include linux/bitfield.h and use:

	u32_encode_bits(map.param_table[i].reg, map.param_mask);

instead of writing this yourself (even if it looks correct).

There's a double space after '='.

Regards,
Bjorn

> +}
> +
> +static void qcom_snps_hsphy_read_override_param_seq(struct device *dev)
> +{
> +	struct device_node *node = dev->of_node;
> +	s32 val;
> +	int ret, i;
> +	struct qcom_snps_hsphy *hsphy;
> +	const struct override_param_map *cfg = of_device_get_match_data(dev);
> +
> +	hsphy = dev_get_drvdata(dev);
> +
> +	for (i = 0; i < ARRAY_SIZE(phy_seq_props); i++) {
> +		ret = of_property_read_s32(node, phy_seq_props[i], &val);
> +		if (ret)
> +			continue;
> +
> +		qcom_snps_hsphy_override_param_update_val(cfg[i], val,
> +					&hsphy->update_seq_cfg[i]);
> +		dev_dbg(&hsphy->phy->dev, "Read param: %s dt_val: %d reg_val: 0x%x\n",
> +			phy_seq_props[i], val, hsphy->update_seq_cfg[i].value);
> +
> +	}
> +}
> +
>  static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -352,6 +609,7 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>  
>  	dev_set_drvdata(dev, hsphy);
>  	phy_set_drvdata(generic_phy, hsphy);
> +	qcom_snps_hsphy_read_override_param_seq(dev);
>  
>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>  	if (!IS_ERR(phy_provider))
> -- 
> 2.7.4
> 

Re: [PATCH v11 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters

[Krishna Kurapati PSSNV]

On 8/31/2022 2:05 AM, Bjorn Andersson wrote:
> On Thu, Jul 21, 2022 at 11:59:13AM +0530, Krishna Kurapati wrote:
>> Add support for overriding electrical signal tuning parameters for
>> SNPS HS Phy.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> Reviewed-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 262 +++++++++++++++++++++++++-
>>   1 file changed, 260 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>> index 5d20378..24264ad 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>> @@ -52,6 +52,12 @@
>>   #define USB2_SUSPEND_N				BIT(2)
>>   #define USB2_SUSPEND_N_SEL			BIT(3)
>>   
>> +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0		(0x6c)
>> +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1		(0x70)
>> +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2		(0x74)
>> +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3		(0x78)
>> +#define PARAM_OVRD_MASK				0xFF
>> +
>>   #define USB2_PHY_USB_PHY_CFG0			(0x94)
>>   #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN	BIT(0)
>>   #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN		BIT(1)
>> @@ -60,12 +66,69 @@
>>   #define REFCLK_SEL_MASK				GENMASK(1, 0)
>>   #define REFCLK_SEL_DEFAULT			(0x2 << 0)
>>   
>> +#define HS_DISCONNECT_MASK			GENMASK(2, 0)
>> +#define SQUELCH_DETECTOR_MASK			GENMASK(7, 5)
>> +
>> +#define HS_AMPLITUDE_MASK			GENMASK(3, 0)
>> +#define PREEMPHASIS_DURATION_MASK		BIT(5)
>> +#define PREEMPHASIS_AMPLITUDE_MASK		GENMASK(7, 6)
>> +
>> +#define HS_RISE_FALL_MASK			GENMASK(1, 0)
>> +#define HS_CROSSOVER_VOLTAGE_MASK		GENMASK(3, 2)
>> +#define HS_OUTPUT_IMPEDANCE_MASK		GENMASK(5, 4)
>> +
>> +#define LS_FS_OUTPUT_IMPEDANCE_MASK		GENMASK(3, 0)
>> +
>>   static const char * const qcom_snps_hsphy_vreg_names[] = {
>>   	"vdda-pll", "vdda33", "vdda18",
>>   };
>>   
>>   #define SNPS_HS_NUM_VREGS		ARRAY_SIZE(qcom_snps_hsphy_vreg_names)
>>   
>> +struct override_param {
>> +	s32	value;
>> +	u8	reg;
>
Hi Bjorn,

> It wasn't immediately obvious to me that reg meant "register value to be
> written for the given value specified in DT".
>
Yes, reg meant the register value to be written corresponding to the 
dt_val provided.

> I think if you changed "reg" to "reg_val" it would have been.
>Sure. Will make this change in the next version.

>> +};
>> +
>> +#define OVERRIDE_PARAM(bps, val) {	\
> 
> TBH, I don't find this macro to add any value. In order to understand
> the tables below I need to look at this macro to know that the two
> integers passed to the macro are just stuffed into the array - there's
> nothing more to it.
> 
> As such I would prefer if you simply wrote:
> 
Sure. I just didn't want to make it look like some random values, but 
since these are coming straight out of datasheet and the macro is just 
to assign values, I can remove this in the next version.

> static const struct override_param hs_disconnect_sc7280[] = {
> 	{ -272, 0 },
> 	{ 0, 1 },
> 	{ 317, 2 },
> 	...
> }
> 
>> +	.value = bps,			\
>> +	.reg = val,			\
>> +}
>> +
>> +struct override_param_map {
>> +	const struct override_param *param_table;
>> +	u8 table_size;
>> +	u8 reg_offset;
>> +	u8 param_mask;
>> +};
>> +
>> +#define OVERRIDE_PARAM_MAP(table, num_elements, offset, mask)		\
>> +{									\
>> +	.param_table = table,						\
>> +	.table_size = num_elements,					\
>> +	.reg_offset = offset,						\
>> +	.param_mask = mask,						\
>> +}
> 
> Here you could have used the preprocessor to fill in table_size (by just
> making it ARRAY_SIZE(table)). But as with OVERRIDE_PARAM I would prefer
> if you simply do:
> 
I used ARRAY_SIZE(table) in the code below to indicate the number of 
input data elements present for each property.
> 
> static const struct override_param_map sc7280[] = {
> 	{
> 		hs_disconnect_sc7280,
> 		ARRAY_SIZE(hs_disconnect_sc7280),
> 		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
> 		HS_DISCONNECT_MASK),
> 	},
> 	{
> 		squelch_det_threshold_sc7280,
> 		ARRAY_SIZE(squelch_det_threshold_sc7280),
> 		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
> 		SQUELCH_DETECTOR_MASK),
> 	},
> 	...
> }
> 
>> +
>> +struct phy_override_seq {
>> +	bool	need_update;
>> +	u8	offset;
>> +	u8	value;
>> +	u8	mask;
>> +};
>> +
>> +static const char * const phy_seq_props[] = {
>> +	"qcom,hs-disconnect-bp",
> 
> The ordering of this list needs to match the order of
> override_param_map[] and there's nothing indicating this in the code.
> 
> I was considering suggesting that you add a enum/define and do
> 	[SQUELCH_DETECTOR_BP] = "qcom,squelch-detector-bp",
> 	...
> and then do the same in the override_param_map array.
> >
> But I think it will be cleaner if you add a const char const pointer to
> override_param_map and just specify these strings in the
> override_param_map array.
> 
> Each entry will grow by a pointer, but multiple copies of the same
> strings (when added in the future) should be combined by the compiler.
>
IIUC, you want me to remove this array of const char*'s and embed them 
in the override_param_map and iterate through it without using this 
const phy_seq_props as a reference.

>> +	"qcom,squelch-detector-bp",
>> +	"qcom,hs-amplitude-bp",
>> +	"qcom,pre-emphasis-duration-bp",
>> +	"qcom,pre-emphasis-amplitude-bp",
>> +	"qcom,hs-rise-fall-time-bp",
>> +	"qcom,hs-crossover-voltage-microvolt",
>> +	"qcom,hs-output-impedance-micro-ohms",
>> +	"qcom,ls-fs-output-impedance-bp",
>> +};
>> +
>>   /**
>>    * struct qcom_snps_hsphy - snps hs phy attributes
>>    *
>> @@ -91,6 +154,7 @@ struct qcom_snps_hsphy {
>>   
>>   	bool phy_initialized;
>>   	enum phy_mode mode;
>> +	struct phy_override_seq update_seq_cfg[ARRAY_SIZE(phy_seq_props)];
>>   };
>>   
>>   static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
>> @@ -173,10 +237,147 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
>>   	return 0;
>>   }
>>   
>> +static const struct override_param hs_disconnect_sc7280[] = {
>> +	OVERRIDE_PARAM(-272, 0),
>> +	OVERRIDE_PARAM(0, 1),
>> +	OVERRIDE_PARAM(317, 2),
>> +	OVERRIDE_PARAM(630, 3),
>> +	OVERRIDE_PARAM(973, 4),
>> +	OVERRIDE_PARAM(1332, 5),
>> +	OVERRIDE_PARAM(1743, 6),
>> +	OVERRIDE_PARAM(2156, 7),
>> +};
>> +
>> +static const struct override_param squelch_det_threshold_sc7280[] = {
>> +	OVERRIDE_PARAM(-2090, 7),
>> +	OVERRIDE_PARAM(-1560, 6),
>> +	OVERRIDE_PARAM(-1030, 5),
>> +	OVERRIDE_PARAM(-530, 4),
>> +	OVERRIDE_PARAM(0, 3),
>> +	OVERRIDE_PARAM(530, 2),
>> +	OVERRIDE_PARAM(1060, 1),
>> +	OVERRIDE_PARAM(1590, 0),
>> +};
>> +
>> +static const struct override_param hs_amplitude_sc7280[] = {
>> +	OVERRIDE_PARAM(-660, 0),
>> +	OVERRIDE_PARAM(-440, 1),
>> +	OVERRIDE_PARAM(-220, 2),
>> +	OVERRIDE_PARAM(0, 3),
>> +	OVERRIDE_PARAM(230, 4),
>> +	OVERRIDE_PARAM(440, 5),
>> +	OVERRIDE_PARAM(650, 6),
>> +	OVERRIDE_PARAM(890, 7),
>> +	OVERRIDE_PARAM(1110, 8),
>> +	OVERRIDE_PARAM(1330, 9),
>> +	OVERRIDE_PARAM(1560, 10),
>> +	OVERRIDE_PARAM(1780, 11),
>> +	OVERRIDE_PARAM(2000, 12),
>> +	OVERRIDE_PARAM(2220, 13),
>> +	OVERRIDE_PARAM(2430, 14),
>> +	OVERRIDE_PARAM(2670, 15),
>> +};
>> +
>> +static const struct override_param preemphasis_duration_sc7280[] = {
>> +	OVERRIDE_PARAM(10000, 1),
>> +	OVERRIDE_PARAM(20000, 0),
>> +};
>> +
>> +static const struct override_param preemphasis_amplitude_sc7280[] = {
>> +	OVERRIDE_PARAM(10000, 1),
>> +	OVERRIDE_PARAM(20000, 2),
>> +	OVERRIDE_PARAM(30000, 3),
>> +	OVERRIDE_PARAM(40000, 0),
>> +};
>> +
>> +static const struct override_param hs_rise_fall_time_sc7280[] = {
>> +	OVERRIDE_PARAM(-4100, 3),
>> +	OVERRIDE_PARAM(0, 2),
>> +	OVERRIDE_PARAM(2810, 1),
>> +	OVERRIDE_PARAM(5430, 0),
>> +};
>> +
>> +static const struct override_param hs_crossover_voltage_sc7280[] = {
>> +	OVERRIDE_PARAM(-31000, 1),
>> +	OVERRIDE_PARAM(0, 3),
>> +	OVERRIDE_PARAM(28000, 2),
>> +};
>> +
>> +static const struct override_param hs_output_impedance_sc7280[] = {
>> +	OVERRIDE_PARAM(-2300000, 3),
>> +	OVERRIDE_PARAM(0, 2),
>> +	OVERRIDE_PARAM(2600000, 1),
>> +	OVERRIDE_PARAM(6100000, 0),
>> +};
>> +
>> +static const struct override_param ls_fs_output_impedance_sc7280[] = {
>> +	OVERRIDE_PARAM(-1053, 15),
>> +	OVERRIDE_PARAM(-557, 7),
>> +	OVERRIDE_PARAM(0, 3),
>> +	OVERRIDE_PARAM(612, 1),
>> +	OVERRIDE_PARAM(1310, 0),
>> +};
>> +
>> +static const struct override_param_map sc7280[] = {
> 
> There's nothing ensuring that the loop below doesn't run off the end of
> this array. So when the next platform is added, there's no way to
> handle the fact that they might have a different set of properties.
> 
> If you add the property name to these elements, that will no longer be a
> problem (and you can add a {} entry at the end of the list and check for
> this when looping over the elements.
> 
>> +	OVERRIDE_PARAM_MAP(
>> +			hs_disconnect_sc7280,
>> +			ARRAY_SIZE(hs_disconnect_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
>> +			HS_DISCONNECT_MASK),
>> +
>> +	OVERRIDE_PARAM_MAP(
>> +			squelch_det_threshold_sc7280,
>> +			ARRAY_SIZE(squelch_det_threshold_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
>> +			SQUELCH_DETECTOR_MASK),
>> +
>> +	OVERRIDE_PARAM_MAP(
>> +			hs_amplitude_sc7280,
>> +			ARRAY_SIZE(hs_amplitude_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
>> +			HS_AMPLITUDE_MASK),
>> +
>> +	OVERRIDE_PARAM_MAP(
>> +			preemphasis_duration_sc7280,
>> +			ARRAY_SIZE(preemphasis_duration_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
>> +			PREEMPHASIS_DURATION_MASK),
>> +
>> +	OVERRIDE_PARAM_MAP(
>> +			preemphasis_amplitude_sc7280,
>> +			ARRAY_SIZE(preemphasis_amplitude_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
>> +			PREEMPHASIS_AMPLITUDE_MASK),
>> +
>> +	OVERRIDE_PARAM_MAP(
>> +			hs_rise_fall_time_sc7280,
>> +			ARRAY_SIZE(hs_rise_fall_time_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
>> +			HS_RISE_FALL_MASK),
>> +
>> +	OVERRIDE_PARAM_MAP(
>> +			hs_crossover_voltage_sc7280,
>> +			ARRAY_SIZE(hs_crossover_voltage_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
>> +			HS_CROSSOVER_VOLTAGE_MASK),
>> +
>> +	OVERRIDE_PARAM_MAP(
>> +			hs_output_impedance_sc7280,
>> +			ARRAY_SIZE(hs_output_impedance_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
>> +			HS_OUTPUT_IMPEDANCE_MASK),
>> +
>> +	OVERRIDE_PARAM_MAP(
>> +			ls_fs_output_impedance_sc7280,
>> +			ARRAY_SIZE(ls_fs_output_impedance_sc7280),
>> +			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3,
>> +			LS_FS_OUTPUT_IMPEDANCE_MASK),
>> +};
>> +
>>   static int qcom_snps_hsphy_init(struct phy *phy)
>>   {
>>   	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
>> -	int ret;
>> +	int ret, i;
>>   
>>   	dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
>>   
>> @@ -223,6 +424,14 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>>   	qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1,
>>   					VBUSVLDEXT0, VBUSVLDEXT0);
>>   
>> +	for (i = 0; i < ARRAY_SIZE(hsphy->update_seq_cfg); i++) {
>> +		if (hsphy->update_seq_cfg[i].need_update)
>> +			qcom_snps_hsphy_write_mask(hsphy->base,
>> +					hsphy->update_seq_cfg[i].offset,
>> +					hsphy->update_seq_cfg[i].mask,
>> +					hsphy->update_seq_cfg[i].value);
>> +	}
>> +
>>   	qcom_snps_hsphy_write_mask(hsphy->base,
>>   					USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2,
>>   					VREGBYPASS, VREGBYPASS);
>> @@ -280,7 +489,10 @@ static const struct phy_ops qcom_snps_hsphy_gen_ops = {
>>   static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
>>   	{ .compatible	= "qcom,sm8150-usb-hs-phy", },
>>   	{ .compatible	= "qcom,usb-snps-hs-5nm-phy", },
>> -	{ .compatible	= "qcom,usb-snps-hs-7nm-phy", },
>> +	{
>> +		.compatible	= "qcom,usb-snps-hs-7nm-phy",
>> +		.data		= &sc7280,
> 
> Perhaps name "qcom_snps_hsphy_7nm_override_map" or something
> along those lines instead of "sc7280"?
My bad, I left it blant. I will rename this appropriately in the next 
version.
> 
>> +	},
>>   	{ .compatible	= "qcom,usb-snps-femto-v2-phy",	},
>>   	{ }
>>   };
>> @@ -291,6 +503,51 @@ static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
>>   			   qcom_snps_hsphy_runtime_resume, NULL)
>>   };
>>   
>> +static void qcom_snps_hsphy_override_param_update_val(
>> +			const struct override_param_map map,
>> +			s32 dt_val, struct phy_override_seq *seq_entry)
>> +{
>> +	int i;
>> +
>> +	/*
>> +	 * Param table for each param is in increasing order
>> +	 * of dt values. We need to iterate over the list to
>> +	 * select the entry that has equal or the next highest value.
>> +	 */
>> +	for (i = 0; i < map.table_size - 1; i++) {
>> +		if (map.param_table[i].value == dt_val)
>> +			break;
>> +	}
>> +
>> +	seq_entry->need_update = true;
>> +	seq_entry->offset = map.reg_offset;
>> +	seq_entry->mask = map.param_mask;
>> +	seq_entry->value =  map.param_table[i].reg << __ffs(map.param_mask);
> 
> I would prefer that you include linux/bitfield.h and use:
> 
> 	u32_encode_bits(map.param_table[i].reg, map.param_mask);
> 
> instead of writing this yourself (even if it looks correct).
> 
> There's a double space after '='.
> 
> Regards,
> Bjorn
> 
Thanks,
Krishna,
>> +}
>> +
>> +static void qcom_snps_hsphy_read_override_param_seq(struct device *dev)
>> +{
>> +	struct device_node *node = dev->of_node;
>> +	s32 val;
>> +	int ret, i;
>> +	struct qcom_snps_hsphy *hsphy;
>> +	const struct override_param_map *cfg = of_device_get_match_data(dev);
>> +
>> +	hsphy = dev_get_drvdata(dev);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(phy_seq_props); i++) {
>> +		ret = of_property_read_s32(node, phy_seq_props[i], &val);
>> +		if (ret)
>> +			continue;
>> +
>> +		qcom_snps_hsphy_override_param_update_val(cfg[i], val,
>> +					&hsphy->update_seq_cfg[i]);
>> +		dev_dbg(&hsphy->phy->dev, "Read param: %s dt_val: %d reg_val: 0x%x\n",
>> +			phy_seq_props[i], val, hsphy->update_seq_cfg[i].value);
>> +
>> +	}
>> +}
>> +
>>   static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -352,6 +609,7 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>>   
>>   	dev_set_drvdata(dev, hsphy);
>>   	phy_set_drvdata(generic_phy, hsphy);
>> +	qcom_snps_hsphy_read_override_param_seq(dev);
>>   
>>   	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>>   	if (!IS_ERR(phy_provider))
>> -- 
>> 2.7.4
>>

Re: [PATCH v11 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters

[Vinod Koul]

On 31-08-22, 18:41, Krishna Kurapati PSSNV wrote:
 
> > The ordering of this list needs to match the order of
> > override_param_map[] and there's nothing indicating this in the code.
> > 
> > I was considering suggesting that you add a enum/define and do
> > 	[SQUELCH_DETECTOR_BP] = "qcom,squelch-detector-bp",
> > 	...
> > and then do the same in the override_param_map array.
> > >
> > But I think it will be cleaner if you add a const char const pointer to
> > override_param_map and just specify these strings in the
> > override_param_map array.
> > 
> > Each entry will grow by a pointer, but multiple copies of the same
> > strings (when added in the future) should be combined by the compiler.
> > 
> IIUC, you want me to remove this array of const char*'s and embed them in
> the override_param_map and iterate through it without using this const
> phy_seq_props as a reference.

I think that would make it simpler.. 

> > > +static const struct override_param_map sc7280[] = {
> > 
> > There's nothing ensuring that the loop below doesn't run off the end of
> > this array. So when the next platform is added, there's no way to
> > handle the fact that they might have a different set of properties.
> > 
> > If you add the property name to these elements, that will no longer be a
> > problem (and you can add a {} entry at the end of the list and check for
> > this when looping over the elements.

Would be great if this is addressed as well

-- 
~Vinod

Re: [PATCH v11 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters

[Krishna Kurapati PSSNV]

On 9/4/2022 8:47 PM, Vinod Koul wrote:
> On 31-08-22, 18:41, Krishna Kurapati PSSNV wrote:
>   
>>> The ordering of this list needs to match the order of
>>> override_param_map[] and there's nothing indicating this in the code.
>>>
>>> I was considering suggesting that you add a enum/define and do
>>> 	[SQUELCH_DETECTOR_BP] = "qcom,squelch-detector-bp",
>>> 	...
>>> and then do the same in the override_param_map array.
>>>>
>>> But I think it will be cleaner if you add a const char const pointer to
>>> override_param_map and just specify these strings in the
>>> override_param_map array.
>>>
>>> Each entry will grow by a pointer, but multiple copies of the same
>>> strings (when added in the future) should be combined by the compiler.
>>>
>> IIUC, you want me to remove this array of const char*'s and embed them in
>> the override_param_map and iterate through it without using this const
>> phy_seq_props as a reference.
> 
> I think that would make it simpler..
> 
>>>> +static const struct override_param_map sc7280[] = {
>>>
>>> There's nothing ensuring that the loop below doesn't run off the end of
>>> this array. So when the next platform is added, there's no way to
>>> handle the fact that they might have a different set of properties.
>>>
>>> If you add the property name to these elements, that will no longer be a
>>> problem (and you can add a {} entry at the end of the list and check for
>>> this when looping over the elements.
> 
> Would be great if this is addressed as well
Hi Vinod. Thanks for the review. I have posted v12 addressing these 
comments.

https://lore.kernel.org/linux-usb/1662201048-26049-1-git-send-email-quic_kriskura@quicinc.com/

Regards,
Krishna,
>