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

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

[Krishna Kurapati]

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 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       |  96 ++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           |   6 +
 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c      | 267 ++++++++++++++++++++-
 3 files changed, 367 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v8 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       | 96 ++++++++++++++++++++++
 1 file changed, 96 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 1ce251d..daeeb04 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,102 @@ 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. Possible values are.
+      The values defined are in multiples of basis points (1bp = 0.01%).
+      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 values defined are in multiples of basis points (1bp = 0.01%).
+      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 values defined are in multiples of basis points (1bp = 0.01%).
+      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 values defined are in multiples of basis points (1bp = 0.01%).
+      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 values defined are in multiples of basis points (1bp = 0.01%).
+      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 values defined are in multiples of basis points (1bp = 0.01%).
+      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 values defined are in milli volts. 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 values defined are in milli ohms.
+      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 values defined are in multiples of basis points (1bp = 0.01%).
+      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 v8 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 | 267 +++++++++++++++++++++++++-
 1 file changed, 265 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..14bbb06 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,76 @@
 #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 {
+	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 *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 +161,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 +244,147 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
 	return 0;
 }
 
+static 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 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 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 struct override_param preemphasis_duration_sc7280[] = {
+	OVERRIDE_PARAM(10000, 1),
+	OVERRIDE_PARAM(20000, 0),
+};
+
+static struct override_param preemphasis_amplitude_sc7280[] = {
+	OVERRIDE_PARAM(10000, 1),
+	OVERRIDE_PARAM(20000, 2),
+	OVERRIDE_PARAM(30000, 3),
+	OVERRIDE_PARAM(40000, 0),
+};
+
+static 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 struct override_param hs_crossover_voltage_sc7280[] = {
+	OVERRIDE_PARAM(-31000, 1),
+	OVERRIDE_PARAM(0, 3),
+	OVERRIDE_PARAM(28000, 2),
+};
+
+static struct override_param hs_output_impedance_sc7280[] = {
+	OVERRIDE_PARAM(-2300000, 3),
+	OVERRIDE_PARAM(0, 2),
+	OVERRIDE_PARAM(2600000, 1),
+	OVERRIDE_PARAM(6100000, 0),
+};
+
+static 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),
+};
+
+struct override_param_map sc7280_idp[] = {
+	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 +431,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 +496,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_idp,
+	},
 	{ .compatible	= "qcom,usb-snps-femto-v2-phy",	},
 	{ }
 };
@@ -291,6 +510,49 @@ 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) {
+			dev_dbg(&hsphy->phy->dev, "Read param: %s val: %d\n",
+						phy_seq_props[i], val);
+			qcom_snps_hsphy_override_param_update_val(cfg[i], val,
+						&hsphy->update_seq_cfg[i]);
+		}
+	}
+}
+
 static int qcom_snps_hsphy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -352,6 +614,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 v8 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>
---
 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 ecbf2b8..589258a 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -317,6 +317,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 v8 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings

[Rob Herring]

On Wed, 01 Jun 2022 12:26:02 +0530, Krishna Kurapati wrote:
> 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       | 96 ++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: ignoring, error in schema: properties: qcom,hs-rise-fall-time-bp
Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.example.dtb:0:0: /example-0/phy@88e2000: failed to match any schema with compatible: ['qcom,sm8150-usb-hs-phy']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

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

[Rob Herring]

On Wed, Jun 01, 2022 at 07:28:53AM -0500, Rob Herring wrote:
> On Wed, 01 Jun 2022 12:26:02 +0530, Krishna Kurapati wrote:
> > 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       | 96 ++++++++++++++++++++++
> >  1 file changed, 96 insertions(+)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
> 	'type' is a required property
> 		hint: A vendor boolean property can use "type: boolean"
> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
> 		hint: A vendor boolean property can use "type: boolean"
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
> 		'enum' is a required property
> 		'const' is a required property
> 		hint: A vendor string property with exact values has an implicit type
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
> 		'$ref' is a required property
> 		'allOf' is a required property
> 		hint: A vendor property needs a $ref to types.yaml
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
> 	'type' is a required property
> 		hint: A vendor boolean property can use "type: boolean"
> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
> 		hint: A vendor boolean property can use "type: boolean"
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
> 		'enum' is a required property
> 		'const' is a required property
> 		hint: A vendor string property with exact values has an implicit type
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
> 		'$ref' is a required property
> 		'allOf' is a required property
> 		hint: A vendor property needs a $ref to types.yaml
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
> 	'type' is a required property
> 		hint: A vendor boolean property can use "type: boolean"
> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
> 		hint: A vendor boolean property can use "type: boolean"
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
> 		'enum' is a required property
> 		'const' is a required property
> 		hint: A vendor string property with exact values has an implicit type
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
> 		'$ref' is a required property
> 		'allOf' is a required property
> 		hint: A vendor property needs a $ref to types.yaml
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
> 	'type' is a required property
> 		hint: A vendor boolean property can use "type: boolean"
> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
> 		hint: A vendor boolean property can use "type: boolean"
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
> 		'enum' is a required property
> 		'const' is a required property
> 		hint: A vendor string property with exact values has an implicit type
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
> 		'$ref' is a required property
> 		'allOf' is a required property
> 		hint: A vendor property needs a $ref to types.yaml
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
> 	'type' is a required property
> 		hint: A vendor boolean property can use "type: boolean"
> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
> 		hint: A vendor boolean property can use "type: boolean"
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
> 		'enum' is a required property
> 		'const' is a required property
> 		hint: A vendor string property with exact values has an implicit type
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
> 		'$ref' is a required property
> 		'allOf' is a required property
> 		hint: A vendor property needs a $ref to types.yaml
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
> 	'type' is a required property
> 		hint: A vendor boolean property can use "type: boolean"
> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
> 		hint: A vendor boolean property can use "type: boolean"
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
> 		'enum' is a required property
> 		'const' is a required property
> 		hint: A vendor string property with exact values has an implicit type
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
> 		'$ref' is a required property
> 		'allOf' is a required property
> 		hint: A vendor property needs a $ref to types.yaml
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
> 	'type' is a required property
> 		hint: A vendor boolean property can use "type: boolean"
> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
> 		hint: A vendor boolean property can use "type: boolean"
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
> 		'enum' is a required property
> 		'const' is a required property
> 		hint: A vendor string property with exact values has an implicit type
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
> 		'$ref' is a required property
> 		'allOf' is a required property
> 		hint: A vendor property needs a $ref to types.yaml
> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: ignoring, error in schema: properties: qcom,hs-rise-fall-time-bp
> Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.example.dtb:0:0: /example-0/phy@88e2000: failed to match any schema with compatible: ['qcom,sm8150-usb-hs-phy']

Ignore these. A tool issue which is now fixed.

Rob

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

[Krishna Kurapati PSSNV]

Thanks Rob !


On 6/1/2022 11:03 PM, Rob Herring wrote:
> On Wed, Jun 01, 2022 at 07:28:53AM -0500, Rob Herring wrote:
>> On Wed, 01 Jun 2022 12:26:02 +0530, Krishna Kurapati wrote:
>>> 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       | 96 ++++++++++++++++++++++
>>>   1 file changed, 96 insertions(+)
>>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
>> 	'type' is a required property
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'enum' is a required property
>> 		'const' is a required property
>> 		hint: A vendor string property with exact values has an implicit type
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'$ref' is a required property
>> 		'allOf' is a required property
>> 		hint: A vendor property needs a $ref to types.yaml
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
>> 	'type' is a required property
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'enum' is a required property
>> 		'const' is a required property
>> 		hint: A vendor string property with exact values has an implicit type
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'$ref' is a required property
>> 		'allOf' is a required property
>> 		hint: A vendor property needs a $ref to types.yaml
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
>> 	'type' is a required property
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'enum' is a required property
>> 		'const' is a required property
>> 		hint: A vendor string property with exact values has an implicit type
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'$ref' is a required property
>> 		'allOf' is a required property
>> 		hint: A vendor property needs a $ref to types.yaml
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
>> 	'type' is a required property
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'enum' is a required property
>> 		'const' is a required property
>> 		hint: A vendor string property with exact values has an implicit type
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'$ref' is a required property
>> 		'allOf' is a required property
>> 		hint: A vendor property needs a $ref to types.yaml
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
>> 	'type' is a required property
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'enum' is a required property
>> 		'const' is a required property
>> 		hint: A vendor string property with exact values has an implicit type
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'$ref' is a required property
>> 		'allOf' is a required property
>> 		hint: A vendor property needs a $ref to types.yaml
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
>> 	'type' is a required property
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'enum' is a required property
>> 		'const' is a required property
>> 		hint: A vendor string property with exact values has an implicit type
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'$ref' is a required property
>> 		'allOf' is a required property
>> 		hint: A vendor property needs a $ref to types.yaml
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
>> 	'type' is a required property
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	Additional properties are not allowed ('maximum', 'minimum' were unexpected)
>> 		hint: A vendor boolean property can use "type: boolean"
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'enum' is a required property
>> 		'const' is a required property
>> 		hint: A vendor string property with exact values has an implicit type
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional failed, one must be fixed:
>> 		'$ref' is a required property
>> 		'allOf' is a required property
>> 		hint: A vendor property needs a $ref to types.yaml
>> 		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>> 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: ignoring, error in schema: properties: qcom,hs-rise-fall-time-bp
>> Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.example.dtb:0:0: /example-0/phy@88e2000: failed to match any schema with compatible: ['qcom,sm8150-usb-hs-phy']
> Ignore these. A tool issue which is now fixed.
>
> Rob

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

[Krishna Kurapati PSSNV]

Hi Bjorn, Krzysztof, Vinod, Kishon,


   Can you please help provide your inputs/review on this patch series.


Regards,

Krishna,


On 6/1/2022 11:06 PM, Krishna Kurapati PSSNV wrote:
> Thanks Rob !
>
>
> On 6/1/2022 11:03 PM, Rob Herring wrote:
>> On Wed, Jun 01, 2022 at 07:28:53AM -0500, Rob Herring wrote:
>>> On Wed, 01 Jun 2022 12:26:02 +0530, Krishna Kurapati wrote:
>>>> 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       | 96 
>>>> ++++++++++++++++++++++
>>>>   1 file changed, 96 insertions(+)
>>>>
>>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>>
>>> yamllint warnings/errors:
>>>
>>> dtschema/dtc warnings/errors:
>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, 
>>> one must be fixed:
>>>     'type' is a required property
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     Additional properties are not allowed ('maximum', 'minimum' were 
>>> unexpected)
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, 
>>> one must be fixed:
>>>         'enum' is a required property
>>>         'const' is a required property
>>>         hint: A vendor string property with exact values has an 
>>> implicit type
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-rise-fall-time-bp: 'oneOf' conditional failed, 
>>> one must be fixed:
>>>         '$ref' is a required property
>>>         'allOf' is a required property
>>>         hint: A vendor property needs a $ref to types.yaml
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     hint: Vendor specific properties must have a type and 
>>> description unless they have a defined, common suffix.
>>>     from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>     'type' is a required property
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     Additional properties are not allowed ('maximum', 'minimum' were 
>>> unexpected)
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>         'enum' is a required property
>>>         'const' is a required property
>>>         hint: A vendor string property with exact values has an 
>>> implicit type
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,ls-fs-output-impedance-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>         '$ref' is a required property
>>>         'allOf' is a required property
>>>         hint: A vendor property needs a $ref to types.yaml
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     hint: Vendor specific properties must have a type and 
>>> description unless they have a defined, common suffix.
>>>     from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>     'type' is a required property
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     Additional properties are not allowed ('maximum', 'minimum' were 
>>> unexpected)
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>         'enum' is a required property
>>>         'const' is a required property
>>>         hint: A vendor string property with exact values has an 
>>> implicit type
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,pre-emphasis-amplitude-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>         '$ref' is a required property
>>>         'allOf' is a required property
>>>         hint: A vendor property needs a $ref to types.yaml
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     hint: Vendor specific properties must have a type and 
>>> description unless they have a defined, common suffix.
>>>     from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>     'type' is a required property
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     Additional properties are not allowed ('maximum', 'minimum' were 
>>> unexpected)
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>         'enum' is a required property
>>>         'const' is a required property
>>>         hint: A vendor string property with exact values has an 
>>> implicit type
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,squelch-detector-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>         '$ref' is a required property
>>>         'allOf' is a required property
>>>         hint: A vendor property needs a $ref to types.yaml
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     hint: Vendor specific properties must have a type and 
>>> description unless they have a defined, common suffix.
>>>     from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>     'type' is a required property
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     Additional properties are not allowed ('maximum', 'minimum' were 
>>> unexpected)
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>         'enum' is a required property
>>>         'const' is a required property
>>>         hint: A vendor string property with exact values has an 
>>> implicit type
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-disconnect-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>         '$ref' is a required property
>>>         'allOf' is a required property
>>>         hint: A vendor property needs a $ref to types.yaml
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     hint: Vendor specific properties must have a type and 
>>> description unless they have a defined, common suffix.
>>>     from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>     'type' is a required property
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     Additional properties are not allowed ('maximum', 'minimum' were 
>>> unexpected)
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>         'enum' is a required property
>>>         'const' is a required property
>>>         hint: A vendor string property with exact values has an 
>>> implicit type
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,hs-amplitude-bp: 'oneOf' conditional failed, one 
>>> must be fixed:
>>>         '$ref' is a required property
>>>         'allOf' is a required property
>>>         hint: A vendor property needs a $ref to types.yaml
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     hint: Vendor specific properties must have a type and 
>>> description unless they have a defined, common suffix.
>>>     from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>     'type' is a required property
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     Additional properties are not allowed ('maximum', 'minimum' were 
>>> unexpected)
>>>         hint: A vendor boolean property can use "type: boolean"
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>         'enum' is a required property
>>>         'const' is a required property
>>>         hint: A vendor string property with exact values has an 
>>> implicit type
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> properties:qcom,pre-emphasis-duration-bp: 'oneOf' conditional 
>>> failed, one must be fixed:
>>>         '$ref' is a required property
>>>         'allOf' is a required property
>>>         hint: A vendor property needs a $ref to types.yaml
>>>         from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>>     hint: Vendor specific properties must have a type and 
>>> description unless they have a defined, common suffix.
>>>     from schema $id: 
>>> http://devicetree.org/meta-schemas/vendor-props.yaml#
>>> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml: 
>>> ignoring, error in schema: properties: qcom,hs-rise-fall-time-bp
>>> Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.example.dtb:0:0: 
>>> /example-0/phy@88e2000: failed to match any schema with compatible: 
>>> ['qcom,sm8150-usb-hs-phy']
>> Ignore these. A tool issue which is now fixed.
>>
>> Rob

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

[Krzysztof Kozlowski]

On 01/06/2022 08:56, Krishna Kurapati wrote:
> 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       | 96 ++++++++++++++++++++++
>  1 file changed, 96 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 1ce251d..daeeb04 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,102 @@ 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. Possible values are.
> +      The values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in milli volts.

It's not accurate anymore - it's microvolt. I propose to skip this one
sentence, because unit is obvious from the type.

> +    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 values defined are in milli ohms.

The same. Other places might need similar change.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 01/06/2022 08:56, Krishna Kurapati wrote:
> Overriding the SNPS Phy tuning parameters for SC7280 IDP device.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 08/06/2022 11:30, Krishna Kurapati PSSNV wrote:
> Hi Bjorn, Krzysztof, Vinod, Kishon,
> 
> 
>    Can you please help provide your inputs/review on this patch series.
> 

You got my review tag on the patch and Rob confirmed that
automated-tool-warning can be ignored. I can take a look at phy code and
do a code-style review, but I don't know (yet) much about Qualcomm PHYs.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 01/06/2022 08:56, 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 | 267 +++++++++++++++++++++++++-
>  1 file changed, 265 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..14bbb06 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,76 @@
>  #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)
> +

I wonder why do you have here blank lines after every define. Are these
bits from different registers?

> +#define HS_AMPLITUDE_MASK			GENMASK(3, 0)
> +
> +#define PREEMPHASIS_DURATION_MASK		BIT(5)
> +
> +#define PREEMPHASIS_AMPLITUDE_MASK		GENMASK(7, 6)

These two look like from same register...

> +
> +#define HS_RISE_FALL_MASK			GENMASK(1, 0)
> +
> +#define HS_CROSSOVER_VOLTAGE_MASK		GENMASK(3, 2)
> +
> +#define HS_OUTPUT_IMPEDANCE_MASK		GENMASK(5, 4)

The same

> +
> +#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 {
> +	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 *phy_seq_props[] = {

static const char * const

> +	"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 +161,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 +244,147 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
>  	return 0;
>  }
>  
> +static struct override_param hs_disconnect_sc7280[] = {

This should be const. I see you are using it in non-const way and this
makes me wonder why... You just need to store the value from the DT and
program it (after converting the units). Why modifying static driver
data? What if you have two phy drivers? Which data is being used?

IOW, all these tables should be const and you could store the final
value in 'struct qcom_snps_hsphy'. Then just program to registers this
final value.

Best regards,
Krzysztof

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

[Krishna Kurapati PSSNV]

On 6/8/2022 3:06 PM, Krzysztof Kozlowski wrote:
> On 01/06/2022 08:56, Krishna Kurapati wrote:
>> 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       | 96 ++++++++++++++++++++++
>>   1 file changed, 96 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 1ce251d..daeeb04 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,102 @@ 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. Possible values are.
>> +      The values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in milli volts.
> It's not accurate anymore - it's microvolt. I propose to skip this one
> sentence, because unit is obvious from the type.
>
>> +    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 values defined are in milli ohms.
> The same. Other places might need similar change.
>
> Best regards,
> Krzysztof

Hi Krzysztof,

     Sure, will modify the description for all the params.

Regards,

Krishna,

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

[Krishna Kurapati PSSNV]

On 6/8/2022 3:06 PM, Krzysztof Kozlowski wrote:
> On 01/06/2022 08:56, Krishna Kurapati wrote:
>> 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       | 96 ++++++++++++++++++++++
>>   1 file changed, 96 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 1ce251d..daeeb04 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,102 @@ 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. Possible values are.
>> +      The values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in milli volts.
> It's not accurate anymore - it's microvolt. I propose to skip this one
> sentence, because unit is obvious from the type.

Hi Krzysztof,

Sure, will omit this line in the next series.

>> +    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 values defined are in milli ohms.
> The same. Other places might need similar change.
>
> Best regards,
> Krzysztof

Sure, Will make sure to remove the basis points sentence from other 
params as well as bp has been added to dtschema and is self-explanatory.


Thanks,

Krishna,

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

[Vinod Koul]

On 01-06-22, 12:26, Krishna Kurapati wrote:
> 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       | 96 ++++++++++++++++++++++
>  1 file changed, 96 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 1ce251d..daeeb04 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,102 @@ 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. Possible values are.
> +      The values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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 values defined are in milli volts. 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 values defined are in milli ohms.
> +      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 values defined are in multiples of basis points (1bp = 0.01%).
> +      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

do we need all these values in DT, till now we have these in driver..
what is the reasoning to add these in DT instead?

-- 
~Vinod

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

[Vinod Koul]

On 01-06-22, 12:26, 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 | 267 +++++++++++++++++++++++++-
>  1 file changed, 265 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..14bbb06 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,76 @@
>  #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)

I guess this is a register, pls remove blank lines between them, serves
no purpose

> +
> +#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)

here two..

> +
> +

One line suffices here (checkpatch with --strict would have warned you)


>  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)\
> +{				\

this should be in preceding line

> +	.value = bps,		\
> +	.reg = val,		\
> +}
> +
> +struct override_param_map {
> +	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 *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 +161,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 +244,147 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
>  	return 0;
>  }
>  
> +static 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),
> +};

use constants for registers please

> +
> +static 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 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 struct override_param preemphasis_duration_sc7280[] = {
> +	OVERRIDE_PARAM(10000, 1),
> +	OVERRIDE_PARAM(20000, 0),
> +};
> +
> +static struct override_param preemphasis_amplitude_sc7280[] = {
> +	OVERRIDE_PARAM(10000, 1),
> +	OVERRIDE_PARAM(20000, 2),
> +	OVERRIDE_PARAM(30000, 3),
> +	OVERRIDE_PARAM(40000, 0),
> +};
> +
> +static 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 struct override_param hs_crossover_voltage_sc7280[] = {
> +	OVERRIDE_PARAM(-31000, 1),
> +	OVERRIDE_PARAM(0, 3),
> +	OVERRIDE_PARAM(28000, 2),
> +};
> +
> +static struct override_param hs_output_impedance_sc7280[] = {
> +	OVERRIDE_PARAM(-2300000, 3),
> +	OVERRIDE_PARAM(0, 2),
> +	OVERRIDE_PARAM(2600000, 1),
> +	OVERRIDE_PARAM(6100000, 0),
> +};
> +
> +static 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),
> +};
> +
> +struct override_param_map sc7280_idp[] = {
> +	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 +431,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 +496,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_idp,
> +	},
>  	{ .compatible	= "qcom,usb-snps-femto-v2-phy",	},
>  	{ }
>  };
> @@ -291,6 +510,49 @@ 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);

okay am nor sure I get the idea, if we have values in DT why do we need
these table in driver or vice-versa?


> +}
> +
> +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) {
> +			dev_dbg(&hsphy->phy->dev, "Read param: %s val: %d\n",
> +						phy_seq_props[i], val);
> +			qcom_snps_hsphy_override_param_update_val(cfg[i], val,
> +						&hsphy->update_seq_cfg[i]);
> +		}
> +	}
> +}
> +
>  static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -352,6 +614,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
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
~Vinod

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

[Krishna Kurapati PSSNV]

On 6/8/2022 9:23 PM, Vinod Koul wrote:
> On 01-06-22, 12:26, Krishna Kurapati wrote:
>> 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       | 96 ++++++++++++++++++++++
>>   1 file changed, 96 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 1ce251d..daeeb04 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,102 @@ 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. Possible values are.
>> +      The values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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 values defined are in milli volts. 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 values defined are in milli ohms.
>> +      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 values defined are in multiples of basis points (1bp = 0.01%).
>> +      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
> do we need all these values in DT, till now we have these in driver..
> what is the reasoning to add these in DT instead?

Hi Vinod,

   The patch series started out as you mentioned. We used to have phy 
tune register values passed from dT to driver and written to respective 
registers in the driver code.

But it was later suggested not to pass register values from dT. Instead 
define the meaningful properties that make up the phy tuning parameters 
and pass their values from dT. The driver code is supposed to convert 
these values and map them to required register values. This is why we 
had to come up with these parameters and declare them in bindings and dT.

More info regarding the discussion at : 
https://lore.kernel.org/linux-usb/b45b3b7e-e1c0-79b6-81c0-53c70427dd10@canonical.com/


Regards,

Krishna,

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

[Vinod Koul]

On 08-06-22, 23:04, Krishna Kurapati PSSNV wrote:
> 
> On 6/8/2022 9:34 PM, Vinod Koul wrote:
> > On 01-06-22, 12:26, 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 | 267 +++++++++++++++++++++++++-
> > >   1 file changed, 265 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..14bbb06 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,76 @@
> > >   #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)
> > I guess this is a register, pls remove blank lines between them, serves
> > no purpose
> > 
> > > +
> > > +#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)
> > here two..
> > 
> > > +
> > > +
> > One line suffices here (checkpatch with --strict would have warned you)
> > 
> > 
> > >   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)\
> > > +{				\
> > this should be in preceding line
> > 
> > > +	.value = bps,		\
> > > +	.reg = val,		\
> > > +}
> > > +
> > > +struct override_param_map {
> > > +	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 *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 +161,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 +244,147 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
> > >   	return 0;
> > >   }
> > > +static 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),
> > > +};
> > use constants for registers please
> > 
> > > +
> > > +static 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 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 struct override_param preemphasis_duration_sc7280[] = {
> > > +	OVERRIDE_PARAM(10000, 1),
> > > +	OVERRIDE_PARAM(20000, 0),
> > > +};
> > > +
> > > +static struct override_param preemphasis_amplitude_sc7280[] = {
> > > +	OVERRIDE_PARAM(10000, 1),
> > > +	OVERRIDE_PARAM(20000, 2),
> > > +	OVERRIDE_PARAM(30000, 3),
> > > +	OVERRIDE_PARAM(40000, 0),
> > > +};
> > > +
> > > +static 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 struct override_param hs_crossover_voltage_sc7280[] = {
> > > +	OVERRIDE_PARAM(-31000, 1),
> > > +	OVERRIDE_PARAM(0, 3),
> > > +	OVERRIDE_PARAM(28000, 2),
> > > +};
> > > +
> > > +static struct override_param hs_output_impedance_sc7280[] = {
> > > +	OVERRIDE_PARAM(-2300000, 3),
> > > +	OVERRIDE_PARAM(0, 2),
> > > +	OVERRIDE_PARAM(2600000, 1),
> > > +	OVERRIDE_PARAM(6100000, 0),
> > > +};
> > > +
> > > +static 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),
> > > +};
> > > +
> > > +struct override_param_map sc7280_idp[] = {
> > > +	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 +431,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 +496,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_idp,
> > > +	},
> > >   	{ .compatible	= "qcom,usb-snps-femto-v2-phy",	},
> > >   	{ }
> > >   };
> > > @@ -291,6 +510,49 @@ 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);
> > okay am nor sure I get the idea, if we have values in DT why do we need
> > these table in driver or vice-versa?
> > 
> Hi Vinod,
> 
> The idea is as follows. There are 4 registers (X0-X3) that control the phy
> tuning parameters mentioned in bindings and dt. The bitmasks mentioned above
> indicate the bitmask in these registers to be updated for tuning a
> particular parameter.

Okay why not define registers and the masks then and use the properties
in DT to calculate and program these registers...

> 
> sc7280_idp[]  -> This table contains info regarding each parameter namely
> -> which register to be modified if we need to tune that parameter
> -> What is the bitmask in the register to be modified in that particular
> register

Bit mask for registers should be defined using BIT/GENMASK macros...

-- 
~Vinod