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

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

[Krishna Kurapati]

Added support for overriding tuning parameters in QCOM SNPS PHY
from device tree.

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

-- 
2.7.4

[v4 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>
---
 .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 87 ++++++++++++++++++++++
 1 file changed, 87 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..70efffe 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,93 @@ properties:
   vdda33-supply:
     description: phandle to the regulator 3.3V supply node.
 
+  qcom,hs-disconnect-bps:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
+  qcom,squelch-detector-bps:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
+  qcom,hs-amplitude-bps:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
+  qcom,pre-emphasis-duration-bps:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
+  qcom,pre-emphasis-amplitude-bps:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
+  qcom,hs-rise-fall-time-bps:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
+  qcom,hs-crossover-voltage-mv:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
+  qcom,hs-output-impedance-mohm:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
+  qcom,ls-fs-output-impedance-bps:
+    $ref: /schemas/types.yaml#/definitions/int32
+    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.
+
 required:
   - compatible
   - reg
-- 
2.7.4

[v4 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>
Reported-by: kernel test robot <lkp@intel.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..7d154e7 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-bps",
+	"qcom,squelch-detector-bps",
+	"qcom,hs-amplitude-bps",
+	"qcom,preemphasis-duration-bps",
+	"qcom,preemphasis-amplitude-bps",
+	"qcom,hs-rise-fall-time-bps",
+	"qcom,hs-crossover-voltage-mv",
+	"qcom,hs-output-impedance-mohm",
+	"qcom,ls-fs-output-impedance-bps",
+};
+
 /**
  * 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(100, 1),
+	OVERRIDE_PARAM(200, 0),
+};
+
+static struct override_param preemphasis_amplitude_sc7280[] = {
+	OVERRIDE_PARAM(100, 1),
+	OVERRIDE_PARAM(200, 2),
+	OVERRIDE_PARAM(300, 3),
+	OVERRIDE_PARAM(400, 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(-31, 1),
+	OVERRIDE_PARAM(28, 2),
+	OVERRIDE_PARAM(0, 3),
+};
+
+static struct override_param hs_output_impedance_sc7280[] = {
+	OVERRIDE_PARAM(-2300, 3),
+	OVERRIDE_PARAM(0, 2),
+	OVERRIDE_PARAM(2600, 1),
+	OVERRIDE_PARAM(6100, 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;
+	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

[v4 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: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
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 5eb6689..bb18f45 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -325,6 +325,12 @@
 	vdda-pll-supply = <&vreg_l10c_0p8>;
 	vdda33-supply = <&vreg_l2b_3p0>;
 	vdda18-supply = <&vreg_l1c_1p8>;
+	qcom,hs-rise-fall-time-bps = <0>;
+	qcom,squelch-detector-bps = <(-2090)>;
+	qcom,hs-disconnect-bps = <1743>;
+	qcom,hs-amplitude-bps = <1780>;
+	qcom,hs-crossover-voltage-mv = <(-31)>;
+	qcom,hs-output-impedance-mohm = <2600>;
 };
 
 &usb_1_qmpphy {
-- 
2.7.4

Re: [v4 0/3] Add QCOM SNPS PHY overriding params support

[Krzysztof Kozlowski]

On 11/05/2022 17:26, Krishna Kurapati wrote:
> Added support for overriding tuning parameters in QCOM SNPS PHY
> from device tree.

Please use proper PATCH title. The easiest is to use git format-patch to
create proper patches.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 11/05/2022 17: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>
> ---
>  .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 87 ++++++++++++++++++++++
>  1 file changed, 87 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..70efffe 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,93 @@ properties:
>    vdda33-supply:
>      description: phandle to the regulator 3.3V supply node.
>  
> +  qcom,hs-disconnect-bps:
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    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%).

This means there is some minimum and maximum (100%)?

> +      The hardware accepts only discrete values. The value closest to the
> +      provided input will be chosen as the override value for this param.
> +
> +  qcom,squelch-detector-bps:
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    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.
> +
> +  qcom,hs-amplitude-bps:
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    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.
> +
> +  qcom,pre-emphasis-duration-bps:
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    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.
> +
> +  qcom,pre-emphasis-amplitude-bps:
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    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.
> +
> +  qcom,hs-rise-fall-time-bps:
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    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.
> +
> +  qcom,hs-crossover-voltage-mv:
> +    $ref: /schemas/types.yaml#/definitions/int32
> +    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.
> +
> +  qcom,hs-output-impedance-mohm:
> +    $ref: /schemas/types.yaml#/definitions/int32

Here and in other places, please use standard units. See
dtschema/schemas/property-units.yaml in dtschema repo.


Best regards,
Krzysztof

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

[kernel test robot]

Hi Krishna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on krzk/for-next linus/master v5.18-rc6 next-20220511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Krishna-Kurapati/Add-QCOM-SNPS-PHY-overriding-params-support/20220511-232858
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220512/202205120425.gScyEI7N-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/48c46f24873c92d3e16904af9e654962d0b923f1
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Krishna-Kurapati/Add-QCOM-SNPS-PHY-overriding-params-support/20220511-232858
        git checkout 48c46f24873c92d3e16904af9e654962d0b923f1
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/phy/qualcomm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c: In function 'qcom_snps_hsphy_read_override_param_seq':
>> drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c:541:42: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     541 |         struct override_param_map *cfg = of_device_get_match_data(dev);
         |                                          ^~~~~~~~~~~~~~~~~~~~~~~~


vim +/const +541 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c

   534	
   535	static void qcom_snps_hsphy_read_override_param_seq(struct device *dev)
   536	{
   537		struct device_node *node = dev->of_node;
   538		s32 val;
   539		int ret, i;
   540		struct qcom_snps_hsphy *hsphy;
 > 541		struct override_param_map *cfg = of_device_get_match_data(dev);
   542	
   543		hsphy = dev_get_drvdata(dev);
   544	
   545		for (i = 0; i < ARRAY_SIZE(phy_seq_props); i++) {
   546			ret = of_property_read_s32(node, phy_seq_props[i], &val);
   547			if (!ret) {
   548				dev_dbg(&hsphy->phy->dev, "Read param: %s val: %d\n",
   549							phy_seq_props[i], val);
   550				qcom_snps_hsphy_override_param_update_val(cfg[i], val,
   551							&hsphy->update_seq_cfg[i]);
   552			}
   553		}
   554	}
   555	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Krishna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on krzk/for-next linus/master v5.18-rc6 next-20220511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Krishna-Kurapati/Add-QCOM-SNPS-PHY-overriding-params-support/20220511-232858
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: riscv-randconfig-r033-20220509 (https://download.01.org/0day-ci/archive/20220512/202205120931.JrA2orb3-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 18dd123c56754edf62c7042dcf23185c3727610f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/48c46f24873c92d3e16904af9e654962d0b923f1
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Krishna-Kurapati/Add-QCOM-SNPS-PHY-overriding-params-support/20220511-232858
        git checkout 48c46f24873c92d3e16904af9e654962d0b923f1
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c:541:29: error: initializing 'struct override_param_map *' with an expression of type 'const void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
           struct override_param_map *cfg = of_device_get_match_data(dev);
                                      ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +541 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c

   534	
   535	static void qcom_snps_hsphy_read_override_param_seq(struct device *dev)
   536	{
   537		struct device_node *node = dev->of_node;
   538		s32 val;
   539		int ret, i;
   540		struct qcom_snps_hsphy *hsphy;
 > 541		struct override_param_map *cfg = of_device_get_match_data(dev);
   542	
   543		hsphy = dev_get_drvdata(dev);
   544	
   545		for (i = 0; i < ARRAY_SIZE(phy_seq_props); i++) {
   546			ret = of_property_read_s32(node, phy_seq_props[i], &val);
   547			if (!ret) {
   548				dev_dbg(&hsphy->phy->dev, "Read param: %s val: %d\n",
   549							phy_seq_props[i], val);
   550				qcom_snps_hsphy_override_param_update_val(cfg[i], val,
   551							&hsphy->update_seq_cfg[i]);
   552			}
   553		}
   554	}
   555	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Krishna Kurapati PSSNV]

On 5/11/2022 11:49 PM, Krzysztof Kozlowski wrote:
> On 11/05/2022 17: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>
>> ---
>>   .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 87 ++++++++++++++++++++++
>>   1 file changed, 87 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..70efffe 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,93 @@ properties:
>>     vdda33-supply:
>>       description: phandle to the regulator 3.3V supply node.
>>   
>> +  qcom,hs-disconnect-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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%).
> This means there is some minimum and maximum (100%)?

Hi Krzystof,

Yes there are max and min for each parameter (not necessarily 0%/100%)

As an example if we take squelch detector threshold, the register value 
vs actual percentage changer as per data book is as follows :

% change in voltage    |     corresponding reg value

  -20.90%                        |    7
  -15.60%                        |    6
-10.30%                         |    5
-5.30%                           |    4
0%                                  |    3
5.30%                            |    2
10.60%                          |    1
15.90%                          |    0

Here the min and max are 15.9% to -20.9%

The min and max differ for each parameter and might not be necessarily 
0% and 100%
>> +      The hardware accepts only discrete values. The value closest to the
>> +      provided input will be chosen as the override value for this param.
>> +
>> +  qcom,squelch-detector-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,hs-amplitude-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,pre-emphasis-duration-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,pre-emphasis-amplitude-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,hs-rise-fall-time-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,hs-crossover-voltage-mv:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,hs-output-impedance-mohm:
>> +    $ref: /schemas/types.yaml#/definitions/int32
> Here and in other places, please use standard units. See
> dtschema/schemas/property-units.yaml in dtschema repo.
>
>
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 12/05/2022 07:57, Krishna Kurapati PSSNV wrote:
> 
> On 5/11/2022 11:49 PM, Krzysztof Kozlowski wrote:
>> On 11/05/2022 17: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>
>>> ---
>>>   .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 87 ++++++++++++++++++++++
>>>   1 file changed, 87 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..70efffe 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,93 @@ properties:
>>>     vdda33-supply:
>>>       description: phandle to the regulator 3.3V supply node.
>>>   
>>> +  qcom,hs-disconnect-bps:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    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%).
>> This means there is some minimum and maximum (100%)?
> 
> Hi Krzystof,
> 
> Yes there are max and min for each parameter (not necessarily 0%/100%)
> 
> As an example if we take squelch detector threshold, the register value 
> vs actual percentage changer as per data book is as follows :
> 
> % change in voltage    |     corresponding reg value
> 
>   -20.90%                        |    7
>   -15.60%                        |    6
> -10.30%                         |    5
> -5.30%                           |    4
> 0%                                  |    3
> 5.30%                            |    2
> 10.60%                          |    1
> 15.90%                          |    0
> 
> Here the min and max are 15.9% to -20.9%
> 
> The min and max differ for each parameter and might not be necessarily 
> 0% and 100%

Then it seems possible to define minimum and maximum values - please add
them ("minimum: xxxx").


Best regards,
Krzysztof

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

[Krishna Kurapati PSSNV]

On 5/11/2022 11:49 PM, Krzysztof Kozlowski wrote:
> On 11/05/2022 17: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>
>> ---
>>   .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 87 ++++++++++++++++++++++
>>   1 file changed, 87 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..70efffe 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,93 @@ properties:
>>     vdda33-supply:
>>       description: phandle to the regulator 3.3V supply node.
>>   
>> +  qcom,hs-disconnect-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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%).
> This means there is some minimum and maximum (100%)?
>
>> +      The hardware accepts only discrete values. The value closest to the
>> +      provided input will be chosen as the override value for this param.
>> +
>> +  qcom,squelch-detector-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,hs-amplitude-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,pre-emphasis-duration-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,pre-emphasis-amplitude-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,hs-rise-fall-time-bps:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,hs-crossover-voltage-mv:
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +    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.
>> +
>> +  qcom,hs-output-impedance-mohm:
>> +    $ref: /schemas/types.yaml#/definitions/int32
> Here and in other places, please use standard units. See
> dtschema/schemas/property-units.yaml in dtschema repo.
>
>
> Best regards,
> Krzysztof

Hi Krzystof, thanks for the input.

I see there are microvolt and microohm units present in 
schemas/property-units.yaml

Would it be possible to add bps (basis point) to the list of standard 
units if it makes sense to use it ?

Regards,

Krishna,

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

[Krzysztof Kozlowski]

On 13/05/2022 09:33, Krishna Kurapati PSSNV wrote:
> 
> On 5/11/2022 11:49 PM, Krzysztof Kozlowski wrote:
>> On 11/05/2022 17: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>
>>> ---
>>>   .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 87 ++++++++++++++++++++++
>>>   1 file changed, 87 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..70efffe 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,93 @@ properties:
>>>     vdda33-supply:
>>>       description: phandle to the regulator 3.3V supply node.
>>>   
>>> +  qcom,hs-disconnect-bps:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    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%).
>> This means there is some minimum and maximum (100%)?
>>
>>> +      The hardware accepts only discrete values. The value closest to the
>>> +      provided input will be chosen as the override value for this param.
>>> +
>>> +  qcom,squelch-detector-bps:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    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.
>>> +
>>> +  qcom,hs-amplitude-bps:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    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.
>>> +
>>> +  qcom,pre-emphasis-duration-bps:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    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.
>>> +
>>> +  qcom,pre-emphasis-amplitude-bps:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    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.
>>> +
>>> +  qcom,hs-rise-fall-time-bps:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    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.
>>> +
>>> +  qcom,hs-crossover-voltage-mv:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>> +    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.
>>> +
>>> +  qcom,hs-output-impedance-mohm:
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>> Here and in other places, please use standard units. See
>> dtschema/schemas/property-units.yaml in dtschema repo.
>>
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzystof, thanks for the input.
> 
> I see there are microvolt and microohm units present in 
> schemas/property-units.yaml
> 
> Would it be possible to add bps (basis point) to the list of standard 
> units if it makes sense to use it ?

There is already 'percent' so 'bp' could be as well, makes sense to me.
I can send a patch for it and we'll see what Rob says.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 13/05/2022 12:32, Krzysztof Kozlowski wrote:
>>
>> Would it be possible to add bps (basis point) to the list of standard 
>> units if it makes sense to use it ?
> 
> There is already 'percent' so 'bp' could be as well, makes sense to me.
> I can send a patch for it and we'll see what Rob says.


Merged:
https://github.com/devicetree-org/dt-schema/pull/73


Best regards,
Krzysztof

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

[Krishna Kurapati PSSNV]

On 5/13/2022 5:50 PM, Krzysztof Kozlowski wrote:
> On 13/05/2022 12:32, Krzysztof Kozlowski wrote:
>>> Would it be possible to add bps (basis point) to the list of standard
>>> units if it makes sense to use it ?
>> There is already 'percent' so 'bp' could be as well, makes sense to me.
>> I can send a patch for it and we'll see what Rob says.
>
> Merged:
> https://github.com/devicetree-org/dt-schema/pull/73
>
>
> Best regards,
> Krzysztof

Thanks Krzysztof, Will refresh the patches with new changes.

Regards,

Krishna,

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

[Krishna Kurapati PSSNV]

On 5/12/2022 4:00 PM, Krzysztof Kozlowski wrote:
> On 12/05/2022 07:57, Krishna Kurapati PSSNV wrote:
>> On 5/11/2022 11:49 PM, Krzysztof Kozlowski wrote:
>>> On 11/05/2022 17: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>
>>>> ---
>>>>    .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 87 ++++++++++++++++++++++
>>>>    1 file changed, 87 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..70efffe 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,93 @@ properties:
>>>>      vdda33-supply:
>>>>        description: phandle to the regulator 3.3V supply node.
>>>>    
>>>> +  qcom,hs-disconnect-bps:
>>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>>> +    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%).
>>> This means there is some minimum and maximum (100%)?
>> Hi Krzystof,
>>
>> Yes there are max and min for each parameter (not necessarily 0%/100%)
>>
>> As an example if we take squelch detector threshold, the register value
>> vs actual percentage changer as per data book is as follows :
>>
>> % change in voltage    |     corresponding reg value
>>
>>    -20.90%                        |    7
>>    -15.60%                        |    6
>> -10.30%                         |    5
>> -5.30%                           |    4
>> 0%                                  |    3
>> 5.30%                            |    2
>> 10.60%                          |    1
>> 15.90%                          |    0
>>
>> Here the min and max are 15.9% to -20.9%
>>
>> The min and max differ for each parameter and might not be necessarily
>> 0% and 100%
> Then it seems possible to define minimum and maximum values - please add
> them ("minimum: xxxx").
>
>
> Best regards,
> Krzysztof

Hi Krzysztof,

  Sorry for the late reply, missed this mail.

Currently, these values have a fixed maximum and minimum. But if these 
limits change in the

future (say on a per target basis) , would it be appropriate to add them 
here in bindings file ?

Also in the driver file for sc7280 target, we have added parameter 
mapping : (map b/w register value

and bps passed from device tree). For squelch detector, it is as follows:

+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),
+};

And the code is written such that if we give a bps value in dt greater than max value in
table, we would automatically choose max value. And if we provide bps value lesser than
minimum value, we would choose the min value.

So, would it be appropriate to add the min and max in dt-bindings when there is a
slight chance of them changing in the future ?

Would like to know your thoughts on this,

Thanks,
Krishna,

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

[Krzysztof Kozlowski]

On 14/05/2022 08:24, Krishna Kurapati PSSNV wrote:
> 
> On 5/12/2022 4:00 PM, Krzysztof Kozlowski wrote:
>> On 12/05/2022 07:57, Krishna Kurapati PSSNV wrote:
>>> On 5/11/2022 11:49 PM, Krzysztof Kozlowski wrote:
>>>> On 11/05/2022 17: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>
>>>>> ---
>>>>>    .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 87 ++++++++++++++++++++++
>>>>>    1 file changed, 87 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..70efffe 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,93 @@ properties:
>>>>>      vdda33-supply:
>>>>>        description: phandle to the regulator 3.3V supply node.
>>>>>    
>>>>> +  qcom,hs-disconnect-bps:
>>>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>>>> +    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%).
>>>> This means there is some minimum and maximum (100%)?
>>> Hi Krzystof,
>>>
>>> Yes there are max and min for each parameter (not necessarily 0%/100%)
>>>
>>> As an example if we take squelch detector threshold, the register value
>>> vs actual percentage changer as per data book is as follows :
>>>
>>> % change in voltage    |     corresponding reg value
>>>
>>>    -20.90%                        |    7
>>>    -15.60%                        |    6
>>> -10.30%                         |    5
>>> -5.30%                           |    4
>>> 0%                                  |    3
>>> 5.30%                            |    2
>>> 10.60%                          |    1
>>> 15.90%                          |    0
>>>
>>> Here the min and max are 15.9% to -20.9%
>>>
>>> The min and max differ for each parameter and might not be necessarily
>>> 0% and 100%
>> Then it seems possible to define minimum and maximum values - please add
>> them ("minimum: xxxx").
>>
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzysztof,
> 
>   Sorry for the late reply, missed this mail.
> 
> Currently, these values have a fixed maximum and minimum. But if these 
> limits change in the
> 
> future (say on a per target basis) , would it be appropriate to add them 
> here in bindings file ?

Per "target" you mean compatible? Then yes, the same as customizing
number of interrupts, clocks etc.

> 
> Also in the driver file for sc7280 target, we have added parameter 
> mapping : (map b/w register value
> 
> and bps passed from device tree). For squelch detector, it is as follows:
> 
> +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),
> +};
> 
> And the code is written such that if we give a bps value in dt greater than max value in
> table, we would automatically choose max value. And if we provide bps value lesser than
> minimum value, we would choose the min value.
> 
> So, would it be appropriate to add the min and max in dt-bindings when there is a
> slight chance of them changing in the future ?

The kernel behavior should not matter here, because the bindings are
about the hardware. Therefore if hardware comes with maximum/minimum
values, it would be better to document them here.


Best regards,
Krzysztof