[PATCH 0/2] Add phy tuning support for imx8mq-usb

[PATCH 0/2] Add phy tuning support for imx8mq-usb

[Johannes Zink]

This series adds support for USB phy tuning parameters, which are
required for meeting USB certification and EMI qualification.

Patch 1/2 adds the required properties to the fsl,imx8mq-usb-phy
devicetree binding.

Patch 2/2 adds the phy tuning parameters to the phy-fsl-imx8mp driver.
This patch is ported and cleaned up from the downstream Freescale vendor
tree.

Best regards,
Johannes

Johannes Zink (1):
  dt-bindings: phy: imx8mq-usb: add phy tuning properties

Li Jun (1):
  phy: fsl-imx8mp-usb: add support for phy tuning

 .../bindings/phy/fsl,imx8mq-usb-phy.yaml      |  40 ++++++
 drivers/phy/freescale/phy-fsl-imx8mq-usb.c    | 124 ++++++++++++++++++
 2 files changed, 164 insertions(+)

-- 
2.39.2


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

[PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Johannes Zink]

Add optional properties for tuning of usb phy.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 .../bindings/phy/fsl,imx8mq-usb-phy.yaml      | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml b/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
index e6f9f5540cc3..f452a41b4f32 100644
--- a/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
@@ -35,6 +35,46 @@ properties:
     description:
       A phandle to the regulator for USB VBUS.
 
+  fsl,phy-tx-vref-tune:
+    description:
+      HS DC Voltage level adjustment
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
+
+  fsl,phy-tx-rise-tune:
+    description:
+      HS Transmitter Rise/Fall Time Adjustment
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3]
+
+  fsl,phy-tx-preemp-amp-tune:
+    description:
+      HS Transmitter Pre-Emphasis Current Control
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3]
+
+  fsl,phy-tx-vboost-level:
+    description:
+      TX Voltage Boost Level
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 2, 3]
+
+  fsl,phy-comp-dis-tune:
+    description:
+      Disconnect Threshold Adjustment
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7]
+
+  fsl,phy-pcs-tx-deemph-3p5db:
+    description:
+      TX De-Emphasis at 3.5 dB
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  fsl,phy-pcs-tx-swing-full:
+    description:
+      TX Amplitude
+    $ref: /schemas/types.yaml#/definitions/uint32
+
 required:
   - compatible
   - reg
-- 
2.39.2


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

[PATCH 2/2] phy: fsl-imx8mp-usb: add support for phy tuning

[Johannes Zink]

From: Li Jun <jun.li@nxp.com>

Add USB PHY parameter tuning for USB certifications.

Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
[j.zink: ported to v6.3-rc1 from NXP downstream repo + cleanups]
Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 124 +++++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
index a29b4a6f7c24..ee1975aaab7e 100644
--- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
+++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
@@ -27,17 +27,137 @@
 #define PHY_CTRL2_TXENABLEN0		BIT(8)
 #define PHY_CTRL2_OTG_DISABLE		BIT(9)
 
+#define PHY_CTRL3			0xc
+#define PHY_CTRL3_COMPDISTUNE_MASK	GENMASK(2, 0)
+#define PHY_CTRL3_TXPREEMP_TUNE_MASK	GENMASK(16, 15)
+#define PHY_CTRL3_TXRISE_TUNE_MASK	GENMASK(21, 20)
+#define PHY_CTRL3_TXVREF_TUNE_MASK	GENMASK(25, 22)
+#define PHY_CTRL3_TX_VBOOST_LEVEL_MASK	GENMASK(31, 29)
+
+#define PHY_CTRL4			0x10
+#define PHY_CTRL4_PCS_TX_DEEMPH_3P5DB_MASK	GENMASK(20, 15)
+
+#define PHY_CTRL5			0x14
+#define PHY_CTRL5_DMPWD_OVERRIDE_SEL	BIT(23)
+#define PHY_CTRL5_DMPWD_OVERRIDE	BIT(22)
+#define PHY_CTRL5_DPPWD_OVERRIDE_SEL	BIT(21)
+#define PHY_CTRL5_DPPWD_OVERRIDE	BIT(20)
+#define PHY_CTRL5_PCS_TX_SWING_FULL_MASK	GENMASK(6, 0)
+
 #define PHY_CTRL6			0x18
 #define PHY_CTRL6_ALT_CLK_EN		BIT(1)
 #define PHY_CTRL6_ALT_CLK_SEL		BIT(0)
 
+#define PHY_TUNE_DEFAULT		0xffffffff
+
 struct imx8mq_usb_phy {
 	struct phy *phy;
 	struct clk *clk;
 	void __iomem *base;
 	struct regulator *vbus;
+	u32 pcs_tx_swing_full;
+	u32 pcs_tx_deemph_3p5db;
+	u32 tx_vref_tune;
+	u32 tx_rise_tune;
+	u32 tx_preemp_amp_tune;
+	u32 tx_vboost_level;
+	u32 comp_dis_tune;
 };
 
+static void imx8m_get_phy_tuning_data(struct imx8mq_usb_phy *imx_phy)
+{
+	struct device *dev = imx_phy->phy->dev.parent;
+
+	if (device_property_read_u32(dev, "fsl,phy-tx-vref-tune",
+				     &imx_phy->tx_vref_tune))
+		imx_phy->tx_vref_tune = PHY_TUNE_DEFAULT;
+
+	if (device_property_read_u32(dev, "fsl,phy-tx-rise-tune",
+				     &imx_phy->tx_rise_tune))
+		imx_phy->tx_rise_tune = PHY_TUNE_DEFAULT;
+
+	if (device_property_read_u32(dev, "fsl,phy-tx-preemp-amp-tune",
+				     &imx_phy->tx_preemp_amp_tune))
+		imx_phy->tx_preemp_amp_tune = PHY_TUNE_DEFAULT;
+
+	if (device_property_read_u32(dev, "fsl,phy-tx-vboost-level",
+				     &imx_phy->tx_vboost_level))
+		imx_phy->tx_vboost_level = PHY_TUNE_DEFAULT;
+
+	if (device_property_read_u32(dev, "fsl,phy-comp-dis-tune",
+				     &imx_phy->comp_dis_tune))
+		imx_phy->comp_dis_tune = PHY_TUNE_DEFAULT;
+
+	if (device_property_read_u32(dev, "fsl,pcs-tx-deemph-3p5db",
+				     &imx_phy->pcs_tx_deemph_3p5db))
+		imx_phy->pcs_tx_deemph_3p5db = PHY_TUNE_DEFAULT;
+
+	if (device_property_read_u32(dev, "fsl,phy-pcs-tx-swing-full",
+				     &imx_phy->pcs_tx_swing_full))
+		imx_phy->pcs_tx_swing_full = PHY_TUNE_DEFAULT;
+}
+
+static void imx8m_phy_tune(struct imx8mq_usb_phy *imx_phy)
+{
+	u32 value;
+
+	/* PHY tuning */
+	if (imx_phy->pcs_tx_deemph_3p5db != PHY_TUNE_DEFAULT) {
+		value = readl(imx_phy->base + PHY_CTRL4);
+		value &= ~PHY_CTRL4_PCS_TX_DEEMPH_3P5DB_MASK;
+		value |= FIELD_PREP(PHY_CTRL4_PCS_TX_DEEMPH_3P5DB_MASK,
+				   imx_phy->pcs_tx_deemph_3p5db);
+		writel(value, imx_phy->base + PHY_CTRL4);
+	}
+
+	if (imx_phy->pcs_tx_swing_full != PHY_TUNE_DEFAULT) {
+		value = readl(imx_phy->base + PHY_CTRL5);
+		value |= FIELD_PREP(PHY_CTRL5_PCS_TX_SWING_FULL_MASK,
+				   imx_phy->pcs_tx_swing_full);
+		writel(value, imx_phy->base + PHY_CTRL5);
+	}
+
+	if ((imx_phy->tx_vref_tune & imx_phy->tx_rise_tune &
+	     imx_phy->tx_preemp_amp_tune & imx_phy->comp_dis_tune &
+	     imx_phy->tx_vboost_level) == PHY_TUNE_DEFAULT)
+		/* If all are the default values, no need update. */
+		return;
+
+	value = readl(imx_phy->base + PHY_CTRL3);
+
+	if (imx_phy->tx_vref_tune != PHY_TUNE_DEFAULT) {
+		value &= ~PHY_CTRL3_TXVREF_TUNE_MASK;
+		value |= FIELD_PREP(PHY_CTRL3_TXVREF_TUNE_MASK,
+				   imx_phy->tx_vref_tune);
+	}
+
+	if (imx_phy->tx_rise_tune != PHY_TUNE_DEFAULT) {
+		value &= ~PHY_CTRL3_TXRISE_TUNE_MASK;
+		value |= FIELD_PREP(PHY_CTRL3_TXRISE_TUNE_MASK,
+				    imx_phy->tx_rise_tune);
+	}
+
+	if (imx_phy->tx_preemp_amp_tune != PHY_TUNE_DEFAULT) {
+		value &= ~PHY_CTRL3_TXPREEMP_TUNE_MASK;
+		value |= FIELD_PREP(PHY_CTRL3_TXPREEMP_TUNE_MASK,
+				imx_phy->tx_preemp_amp_tune);
+	}
+
+	if (imx_phy->comp_dis_tune != PHY_TUNE_DEFAULT) {
+		value &= ~PHY_CTRL3_COMPDISTUNE_MASK;
+		value |= FIELD_PREP(PHY_CTRL3_COMPDISTUNE_MASK,
+				    imx_phy->comp_dis_tune);
+	}
+
+	if (imx_phy->tx_vboost_level != PHY_TUNE_DEFAULT) {
+		value &= ~PHY_CTRL3_TX_VBOOST_LEVEL_MASK;
+		value |= FIELD_PREP(PHY_CTRL3_TX_VBOOST_LEVEL_MASK,
+				    imx_phy->tx_vboost_level);
+	}
+
+	writel(value, imx_phy->base + PHY_CTRL3);
+}
+
 static int imx8mq_usb_phy_init(struct phy *phy)
 {
 	struct imx8mq_usb_phy *imx_phy = phy_get_drvdata(phy);
@@ -99,6 +219,8 @@ static int imx8mp_usb_phy_init(struct phy *phy)
 	value &= ~(PHY_CTRL1_RESET | PHY_CTRL1_ATERESET);
 	writel(value, imx_phy->base + PHY_CTRL1);
 
+	imx8m_phy_tune(imx_phy);
+
 	return 0;
 }
 
@@ -182,6 +304,8 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
 
 	phy_set_drvdata(imx_phy->phy, imx_phy);
 
+	imx8m_get_phy_tuning_data(imx_phy);
+
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 
 	return PTR_ERR_OR_ZERO(phy_provider);
-- 
2.39.2


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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Krzysztof Kozlowski]

On 05/04/2023 13:21, Johannes Zink wrote:
> Add optional properties for tuning of usb phy.
> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  .../bindings/phy/fsl,imx8mq-usb-phy.yaml      | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml b/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
> index e6f9f5540cc3..f452a41b4f32 100644
> --- a/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
> @@ -35,6 +35,46 @@ properties:
>      description:
>        A phandle to the regulator for USB VBUS.
>  
> +  fsl,phy-tx-vref-tune:
> +    description:
> +      HS DC Voltage level adjustment

"Level" in what units?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
> +
> +  fsl,phy-tx-rise-tune:
> +    description:
> +      HS Transmitter Rise/Fall Time Adjustment
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3]
> +
> +  fsl,phy-tx-preemp-amp-tune:
> +    description:
> +      HS Transmitter Pre-Emphasis Current Control

If this is current then use standard unit suffixes.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3]
> +
> +  fsl,phy-tx-vboost-level:
> +    description:
> +      TX Voltage Boost Level
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 2, 3]
> +
> +  fsl,phy-comp-dis-tune:
> +    description:
> +      Disconnect Threshold Adjustment
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3, 4, 5, 6, 7]
> +
> +  fsl,phy-pcs-tx-deemph-3p5db:
> +    description:
> +      TX De-Emphasis at 3.5 dB
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  fsl,phy-pcs-tx-swing-full:
> +    description:
> +      TX Amplitude

I have feeling you just pasted here short titles from datasheet. They
are not that helpful.



Best regards,
Krzysztof


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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Johannes Zink]

Hi Krysztof,

thanks for your review, please find my questions below.

On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
> [snip]
> >        A phandle to the regulator for USB VBUS.
> >  
> > +  fsl,phy-tx-vref-tune:
> > +    description:
> > +      HS DC Voltage level adjustment
> 
> "Level" in what units?
> 

The datasheet just shows percent, ranging from -6 to +24%, in 2%
increments. What unit would you suggest?

> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
> > 16]
> > +
> > +  fsl,phy-tx-rise-tune:
> > +    description:
> > +      HS Transmitter Rise/Fall Time Adjustment
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2, 3]
> > +
> > +  fsl,phy-tx-preemp-amp-tune:
> > +    description:
> > +      HS Transmitter Pre-Emphasis Current Control
> 
> If this is current then use standard unit suffixes.

According to the datasheet this is in "unit amonts" of 600uA, basically
0x600uA, 1x600uA etc. Should I just suffix it with uA then?

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2, 3]
> > +
> > +  fsl,phy-tx-vboost-level:
> > +    description:
> > +      TX Voltage Boost Level
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 2, 3]
> > +
> > +  fsl,phy-comp-dis-tune:
> > +    description:
> > +      Disconnect Threshold Adjustment
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > +
> > +  fsl,phy-pcs-tx-deemph-3p5db:
> > +    description:
> > +      TX De-Emphasis at 3.5 dB
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  fsl,phy-pcs-tx-swing-full:
> > +    description:
> > +      TX Amplitude
> 
> I have feeling you just pasted here short titles from datasheet. They
> are not that helpful.

ack, I will add some more text from the register description, since I
have no other source of information on these.

Best regards
Johannes

> 
> 
> 
> Best regards,
> Krzysztof
> 
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Krzysztof Kozlowski]

On 05/04/2023 14:14, Johannes Zink wrote:
> Hi Krysztof,
> 
> thanks for your review, please find my questions below.
> 
> On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
>> [snip]
>>>        A phandle to the regulator for USB VBUS.
>>>  
>>> +  fsl,phy-tx-vref-tune:
>>> +    description:
>>> +      HS DC Voltage level adjustment
>>
>> "Level" in what units?
>>
> 
> The datasheet just shows percent, ranging from -6 to +24%, in 2%
> increments. What unit would you suggest?

percent
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

> 
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
>>> 16]
>>> +
>>> +  fsl,phy-tx-rise-tune:
>>> +    description:
>>> +      HS Transmitter Rise/Fall Time Adjustment
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [0, 1, 2, 3]
>>> +
>>> +  fsl,phy-tx-preemp-amp-tune:
>>> +    description:
>>> +      HS Transmitter Pre-Emphasis Current Control
>>
>> If this is current then use standard unit suffixes.
> 
> According to the datasheet this is in "unit amonts" of 600uA, basically
> 0x600uA, 1x600uA etc. Should I just suffix it with uA then?

Yes
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

The register values can work sometimes fine but also do not scale at
all. For any other variant all the meanings will differ. Any other IMX8
phy will need new bindings and new description/values for your
register-like-fields.

Best regards,
Krzysztof


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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Johannes Zink]

Hi Krzystof,

thank you for your explanations. As I'm still quite new to writing
bindings, I still have some questions:

On Fri, 2023-04-07 at 11:03 +0200, Krzysztof Kozlowski wrote:
> On 05/04/2023 14:14, Johannes Zink wrote:
> > Hi Krysztof,
> > 
> > thanks for your review, please find my questions below.
> > 
> > On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
> > > [snip]
> > > >        A phandle to the regulator for USB VBUS.
> > > >  
> > > > +  fsl,phy-tx-vref-tune:
> > > > +    description:
> > > > +      HS DC Voltage level adjustment
> > > 
> > > "Level" in what units?
> > > 
> > 
> > The datasheet just shows percent, ranging from -6 to +24%, in 2%
> > increments. What unit would you suggest?
> 
> percent
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

I am still a bit confused how to use this properly. How can I restrict
the values to multiples of 2 in order to avoid illegal values?

At the moment the only thing I could come up with is something like

fsl,phy-tx-vref-tune-percent:                 
  description: |                              
    Adjusts the high-speed DC level voltage   
  $ref: /schemas/types.yaml#/definitions/int32
  minimum: -6                                 
  maximum: 24                                 
  default: 0                                  

Does something like this work? I am not quite sure if I am on the right
track here, especially as this requires a signed int, of which I have
not seen many examples so far.

Also, as far as the description is concerned: This is almost the entire
information I there is in the datasheet. As I try to upstream some of
the vendor downstream patches, I do not have any additional
information.

> 
> > 
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
> > > > 15,
> > > > 16]
> > > > +
> > > > +  fsl,phy-tx-rise-tune:
> > > > +    description:
> > > > +      HS Transmitter Rise/Fall Time Adjustment
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [0, 1, 2, 3]
> > > > +
> > > > +  fsl,phy-tx-preemp-amp-tune:
> > > > +    description:
> > > > +      HS Transmitter Pre-Emphasis Current Control
> > > 
> > > If this is current then use standard unit suffixes.
> > 
> > According to the datasheet this is in "unit amonts" of 600uA,
> > basically
> > 0x600uA, 1x600uA etc. Should I just suffix it with uA then?
> 
> Yes
>  
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> The register values can work sometimes fine but also do not scale at
> all. For any other variant all the meanings will differ. Any other
> IMX8
> phy will need new bindings and new description/values for your
> register-like-fields.

I think this particular property should work, probably its something
like

fsl,phy-tx-preemp-amp-tune-microamps:                                 
  description: |                                                      
    Transmitter Pre-Emphasis Current Control                          
    Controls the amount of current sourced to DPn and DMn after a J-to-
K or K-to-J transition.                                                
  $ref: /schemas/types.yaml#/definitions/uint32                       
  minimum: 0                                                          
  maximum: 1800                                                       
  default: 0                                                          

What's the right way to communicate that the value is in multiples of
600uA and that this is only an approximate Value? Add some free-text to
the description?


For some other properties, such as fsl,phy-pcs-tx-swing-full or
fsl,phy-pcs-tx-deemph-3p5db the datasheet provides no information at
all, neither on the unit nor on a valid range. What is the proper way
for something like them (I try to get some of the freescale downstream
patches to mainline, but they did not even provide bindings for their
driver...)


For fsl,phy-comp-dis-tune-percent, the actual values to not map well to
integer amount of percent, but I have not found a permill in property-
units. Also, as the steps appear quite arbitrary large, what is the
correct way of restricting the values to valid values that the hardware
can actually support? As reference, I have only seen stuff like the
st,trim-hs-current in Documentation/devicetree/bindings/phy/phy-stm32-
usbphyc.yaml so far...

Thanks for helping me and best regards
Johannes


> 
> Best regards,
> Krzysztof
> 
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

RE: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Jun Li]

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Tuesday, April 11, 2023 10:23 PM
> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>;
> vkoul@kernel.org; kishon@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; Jun Li <jun.li@nxp.com>; Bough Chen
> <haibo.chen@nxp.com>; linux-phy@lists.infradead.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning
> properties
>
> Hi Krzystof,
>
> thank you for your explanations. As I'm still quite new to writing bindings,
> I still have some questions:
>
> On Fri, 2023-04-07 at 11:03 +0200, Krzysztof Kozlowski wrote:
> > On 05/04/2023 14:14, Johannes Zink wrote:
> > > Hi Krysztof,
> > >
> > > thanks for your review, please find my questions below.
> > >
> > > On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
> > > > [snip]
> > > > >        A phandle to the regulator for USB VBUS.
> > > > >
> > > > > +  fsl,phy-tx-vref-tune:
> > > > > +    description:
> > > > > +      HS DC Voltage level adjustment
> > > >
> > > > "Level" in what units?
> > > >
> > >
> > > The datasheet just shows percent, ranging from -6 to +24%, in 2%
> > > increments. What unit would you suggest?
> >
> > percent
> >
> https://gith/
> >
> ub.com%2Fdevicetree-org%2Fdt-schema%2Fblob%2Fmain%2Fdtschema%2Fschemas
> > %2Fproperty-units.yaml&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a78
> 4
> >
> de5941d08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63816
> >
> 8197947407580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> >
> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kY4dXkOHRs9k%
> > 2BeUQ5iixLYsNC8UotIgq6eOPGjbf01o%3D&reserved=0
>
> I am still a bit confused how to use this properly. How can I restrict the
> values to multiples of 2 in order to avoid illegal values?
>
> At the moment the only thing I could come up with is something like
>
> fsl,phy-tx-vref-tune-percent:
>   description: |
>     Adjusts the high-speed DC level voltage
>   $ref: /schemas/types.yaml#/definitions/int32
>   minimum: -6
>   maximum: 24
>   default: 0
>
> Does something like this work? I am not quite sure if I am on the right track
> here, especially as this requires a signed int, of which I have not seen
> many examples so far.
>
> Also, as far as the description is concerned: This is almost the entire
> information I there is in the datasheet. As I try to upstream some of the
> vendor downstream patches, I do not have any additional information.
>
> >
> > >
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
> > > > > 15,
> > > > > 16]
> > > > > +
> > > > > +  fsl,phy-tx-rise-tune:
> > > > > +    description:
> > > > > +      HS Transmitter Rise/Fall Time Adjustment
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    enum: [0, 1, 2, 3]
> > > > > +
> > > > > +  fsl,phy-tx-preemp-amp-tune:
> > > > > +    description:
> > > > > +      HS Transmitter Pre-Emphasis Current Control
> > > >
> > > > If this is current then use standard unit suffixes.
> > >
> > > According to the datasheet this is in "unit amonts" of 600uA,
> > > basically 0x600uA, 1x600uA etc. Should I just suffix it with uA
> > > then?
> >
> > Yes
> >
> >
> https://gith/
> >
> ub.com%2Fdevicetree-org%2Fdt-schema%2Fblob%2Fmain%2Fdtschema%2Fschemas
> > %2Fproperty-units.yaml&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a78
> 4
> >
> de5941d08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63816
> >
> 8197947407580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> >
> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kY4dXkOHRs9k%
> > 2BeUQ5iixLYsNC8UotIgq6eOPGjbf01o%3D&reserved=0
> >
> > The register values can work sometimes fine but also do not scale at
> > all. For any other variant all the meanings will differ. Any other
> > IMX8
> > phy will need new bindings and new description/values for your
> > register-like-fields.
>
> I think this particular property should work, probably its something like
>
> fsl,phy-tx-preemp-amp-tune-microamps:
>   description: |
>     Transmitter Pre-Emphasis Current Control
>     Controls the amount of current sourced to DPn and DMn after a J-to-
> K or K-to-J transition.
>   $ref: /schemas/types.yaml#/definitions/uint32
>   minimum: 0
>   maximum: 1800
>   default: 0
>
> What's the right way to communicate that the value is in multiples of 600uA
> and that this is only an approximate Value? Add some free-text to the
> description?
>
>
> For some other properties, such as fsl,phy-pcs-tx-swing-full or
> fsl,phy-pcs-tx-deemph-3p5db the datasheet provides no information at all,
> neither on the unit nor on a valid range. What is the proper way for something
> like them (I try to get some of the freescale downstream patches to mainline,
> but they did not even provide bindings for their
> driver...)

I will check with internal design team for those not well documented
properties.

Li Jun
>
>
> For fsl,phy-comp-dis-tune-percent, the actual values to not map well to
> integer amount of percent, but I have not found a permill in property- units.
> Also, as the steps appear quite arbitrary large, what is the correct way
> of restricting the values to valid values that the hardware can actually
> support? As reference, I have only seen stuff like the st,trim-hs-current
> in Documentation/devicetree/bindings/phy/phy-stm32-
> usbphyc.yaml so far...
>
> Thanks for helping me and best regards
> Johannes
>
>
> >
> > Best regards,
> > Krzysztof
> >
> >
> >
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.p/
> engutronix.de%2F&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a784de5941d
> 08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6381681979474
> 07580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBT
> iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Dq06I6tprUib5nOmp4pDFdY
> MgYULn8MLj5iHwVlQHMY%3D&reserved=0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Johannes Zink]

Hi Jun,

On Tue, 2023-04-11 at 14:59 +0000, Jun Li wrote:
> 
> 
> > -----Original Message-----
> > From: Johannes Zink <j.zink@pengutronix.de>
> > Sent: Tuesday, April 11, 2023 10:23 PM
> > To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>;
> > vkoul@kernel.org; kishon@kernel.org; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > dl-linux-imx <linux-imx@nxp.com>; robh+dt@kernel.org;
> > krzysztof.kozlowski+dt@linaro.org; Jun Li <jun.li@nxp.com>; Bough
> > Chen
> > <haibo.chen@nxp.com>; linux-phy@lists.infradead.org;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org
> > Subject: Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy
> > tuning
> > properties
> > 
> > Hi Krzystof,
> > 
> > thank you for your explanations. As I'm still quite new to writing
> > bindings,
> > I still have some questions:
> > 
> > On Fri, 2023-04-07 at 11:03 +0200, Krzysztof Kozlowski wrote:
> > > On 05/04/2023 14:14, Johannes Zink wrote:
> > > > Hi Krysztof,
> > > > 
> > > > thanks for your review, please find my questions below.
> > > > 
> > > > On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
> > > > > [snip]
> > > > > >        A phandle to the regulator for USB VBUS.
> > > > > > 
> > > > > > +  fsl,phy-tx-vref-tune:
> > > > > > +    description:
> > > > > > +      HS DC Voltage level adjustment
> > > > > 
> > > > > "Level" in what units?
> > > > > 
> > > > 
> > > > The datasheet just shows percent, ranging from -6 to +24%, in
> > > > 2%
> > > > increments. What unit would you suggest?
> > > 
> > > percent
> > > 
> > https://gith/
> > > 
> > ub.com%2Fdevicetree-org%2Fdt-
> > schema%2Fblob%2Fmain%2Fdtschema%2Fschemas
> > > %2Fproperty-
> > > units.yaml&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a78
> > 4
> > > 
> > de5941d08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> > 816
> > > 
> > 8197947407580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> > 2lu
> > > 
> > MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kY4dXkOHRs
> > 9k%
> > > 2BeUQ5iixLYsNC8UotIgq6eOPGjbf01o%3D&reserved=0
> > 
> > I am still a bit confused how to use this properly. How can I
> > restrict the
> > values to multiples of 2 in order to avoid illegal values?
> > 
> > At the moment the only thing I could come up with is something like
> > 
> > fsl,phy-tx-vref-tune-percent:
> >   description: |
> >     Adjusts the high-speed DC level voltage
> >   $ref: /schemas/types.yaml#/definitions/int32
> >   minimum: -6
> >   maximum: 24
> >   default: 0
> > 
> > Does something like this work? I am not quite sure if I am on the
> > right track
> > here, especially as this requires a signed int, of which I have not
> > seen
> > many examples so far.
> > 
> > Also, as far as the description is concerned: This is almost the
> > entire
> > information I there is in the datasheet. As I try to upstream some
> > of the
> > vendor downstream patches, I do not have any additional
> > information.
> > 
> > > 
> > > > 
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13,
> > > > > > 14,
> > > > > > 15,
> > > > > > 16]
> > > > > > +
> > > > > > +  fsl,phy-tx-rise-tune:
> > > > > > +    description:
> > > > > > +      HS Transmitter Rise/Fall Time Adjustment
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > +    enum: [0, 1, 2, 3]
> > > > > > +
> > > > > > +  fsl,phy-tx-preemp-amp-tune:
> > > > > > +    description:
> > > > > > +      HS Transmitter Pre-Emphasis Current Control
> > > > > 
> > > > > If this is current then use standard unit suffixes.
> > > > 
> > > > According to the datasheet this is in "unit amonts" of 600uA,
> > > > basically 0x600uA, 1x600uA etc. Should I just suffix it with uA
> > > > then?
> > > 
> > > Yes
> > > 
> > > 
> > https://gith/
> > > 
> > ub.com%2Fdevicetree-org%2Fdt-
> > schema%2Fblob%2Fmain%2Fdtschema%2Fschemas
> > > %2Fproperty-
> > > units.yaml&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a78
> > 4
> > > 
> > de5941d08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> > 816
> > > 
> > 8197947407580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> > 2lu
> > > 
> > MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kY4dXkOHRs
> > 9k%
> > > 2BeUQ5iixLYsNC8UotIgq6eOPGjbf01o%3D&reserved=0
> > > 
> > > The register values can work sometimes fine but also do not scale
> > > at
> > > all. For any other variant all the meanings will differ. Any
> > > other
> > > IMX8
> > > phy will need new bindings and new description/values for your
> > > register-like-fields.
> > 
> > I think this particular property should work, probably its
> > something like
> > 
> > fsl,phy-tx-preemp-amp-tune-microamps:
> >   description: |
> >     Transmitter Pre-Emphasis Current Control
> >     Controls the amount of current sourced to DPn and DMn after a
> > J-to-
> > K or K-to-J transition.
> >   $ref: /schemas/types.yaml#/definitions/uint32
> >   minimum: 0
> >   maximum: 1800
> >   default: 0
> > 
> > What's the right way to communicate that the value is in multiples
> > of 600uA
> > and that this is only an approximate Value? Add some free-text to
> > the
> > description?
> > 
> > 
> > For some other properties, such as fsl,phy-pcs-tx-swing-full or
> > fsl,phy-pcs-tx-deemph-3p5db the datasheet provides no information
> > at all,
> > neither on the unit nor on a valid range. What is the proper way
> > for something
> > like them (I try to get some of the freescale downstream patches to
> > mainline,
> > but they did not even provide bindings for their
> > driver...)
> 
> I will check with internal design team for those not well documented
> properties.
> 

That's great, thanks!

Johannes

> Li Jun
> > 
> > 
> > For fsl,phy-comp-dis-tune-percent, the actual values to not map
> > well to
> > integer amount of percent, but I have not found a permill in
> > property- units.
> > Also, as the steps appear quite arbitrary large, what is the
> > correct way
> > of restricting the values to valid values that the hardware can
> > actually
> > support? As reference, I have only seen stuff like the st,trim-hs-
> > current
> > in Documentation/devicetree/bindings/phy/phy-stm32-
> > usbphyc.yaml so far...
> > 
> > Thanks for helping me and best regards
> > Johannes
> > 
> > 
> > > 
> > > Best regards,
> > > Krzysztof
> > > 
> > > 
> > > 
> > 
> > --
> > Pengutronix e.K.                | Johannes Zink                  |
> > Steuerwalder Str. 21            |
> > https://www.p/
> > engutronix.de%2F&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a784de5
> > 941d
> > 08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638168197
> > 9474
> > 07580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> > CJBT
> > iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Dq06I6tprUib5nOmp4p
> > DFdY
> > MgYULn8MLj5iHwVlQHMY%3D&reserved=0    |
> > 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> > Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Rob Herring]

On Tue, Apr 11, 2023 at 04:22:37PM +0200, Johannes Zink wrote:
> Hi Krzystof,
> 
> thank you for your explanations. As I'm still quite new to writing
> bindings, I still have some questions:
> 
> On Fri, 2023-04-07 at 11:03 +0200, Krzysztof Kozlowski wrote:
> > On 05/04/2023 14:14, Johannes Zink wrote:
> > > Hi Krysztof,
> > > 
> > > thanks for your review, please find my questions below.
> > > 
> > > On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
> > > > [snip]
> > > > >        A phandle to the regulator for USB VBUS.
> > > > >  
> > > > > +  fsl,phy-tx-vref-tune:
> > > > > +    description:
> > > > > +      HS DC Voltage level adjustment
> > > > 
> > > > "Level" in what units?
> > > > 
> > > 
> > > The datasheet just shows percent, ranging from -6 to +24%, in 2%
> > > increments. What unit would you suggest?
> > 
> > percent
> > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> I am still a bit confused how to use this properly. How can I restrict
> the values to multiples of 2 in order to avoid illegal values?
> 
> At the moment the only thing I could come up with is something like
> 
> fsl,phy-tx-vref-tune-percent:                 
>   description: |                              
>     Adjusts the high-speed DC level voltage   
>   $ref: /schemas/types.yaml#/definitions/int32

Note that with standard unit suffixes, you don't need a type.

>   minimum: -6                                 
>   maximum: 24                                 
>   default: 0                                  
> 
> Does something like this work? I am not quite sure if I am on the right
> track here, especially as this requires a signed int, of which I have
> not seen many examples so far.

We'd have to change the type for -percent to signed. That's possible, 
but for vendor specific properties there's not much advantage to use 
standard units instead of just using the register values directly.

Rob

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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Johannes Zink]

Hi Rob,

On 4/12/23 15:39, Rob Herring wrote:
> On Tue, Apr 11, 2023 at 04:22:37PM +0200, Johannes Zink wrote:
>> Hi Krzystof,
>>
>> thank you for your explanations. As I'm still quite new to writing
>> bindings, I still have some questions:
>>
>> On Fri, 2023-04-07 at 11:03 +0200, Krzysztof Kozlowski wrote:
>>> On 05/04/2023 14:14, Johannes Zink wrote:
>>>> Hi Krysztof,
>>>>
>>>> thanks for your review, please find my questions below.
>>>>
>>>> On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
>>>>> [snip]
>>>>>>         A phandle to the regulator for USB VBUS.
>>>>>>   
>>>>>> +  fsl,phy-tx-vref-tune:
>>>>>> +    description:
>>>>>> +      HS DC Voltage level adjustment
>>>>>
>>>>> "Level" in what units?
>>>>>
>>>>
>>>> The datasheet just shows percent, ranging from -6 to +24%, in 2%
>>>> increments. What unit would you suggest?
>>>
>>> percent
>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
>>
>> I am still a bit confused how to use this properly. How can I restrict
>> the values to multiples of 2 in order to avoid illegal values?
>>
>> At the moment the only thing I could come up with is something like
>>
>> fsl,phy-tx-vref-tune-percent:
>>    description: |
>>      Adjusts the high-speed DC level voltage
>>    $ref: /schemas/types.yaml#/definitions/int32
> 
> Note that with standard unit suffixes, you don't need a type.
> 
>>    minimum: -6
>>    maximum: 24
>>    default: 0
>>
>> Does something like this work? I am not quite sure if I am on the right
>> track here, especially as this requires a signed int, of which I have
>> not seen many examples so far.
> 
> We'd have to change the type for -percent to signed. That's possible,
> but for vendor specific properties there's not much advantage to use
> standard units instead of just using the register values directly.
> 

I don't have any objections to that, this is pretty much what I sent in 
my v1 patch <20230405112118.1256151-2-j.zink@pengutronix.de>, but 
Krzysztof requested to change the vendor specific properties to use 
property-units.

Would something along the lines of the st,trim-hs-current on 
Documentation/devicetree/bindings/phy/phy-stm32-usbphyc.yaml be 
acceptable (i.e. use an enum and annotate the meaning of the values in 
the description)?

I will, nevertheless, try to make the descriptions a bit more verbose in 
my v2 (wherever the datasheet gives me proper informations), as 
Krzysztof requested.

Best regards
Johannes

> Rob
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Johannes Zink]

Hi Rob, hi Krzysztof,

On 4/12/23 16:32, Johannes Zink wrote:
> Hi Rob,
> 
> On 4/12/23 15:39, Rob Herring wrote:
>> On Tue, Apr 11, 2023 at 04:22:37PM +0200, Johannes Zink wrote:
>>> Hi Krzystof,
>>>
>>> thank you for your explanations. As I'm still quite new to writing
>>> bindings, I still have some questions:
>>>
>>> On Fri, 2023-04-07 at 11:03 +0200, Krzysztof Kozlowski wrote:
>>>> On 05/04/2023 14:14, Johannes Zink wrote:
>>>>> Hi Krysztof,
>>>>>
>>>>> thanks for your review, please find my questions below.
>>>>>
>>>>> On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
>>>>>> [snip]
>>>>>>>         A phandle to the regulator for USB VBUS.
>>>>>>> +  fsl,phy-tx-vref-tune:
>>>>>>> +    description:
>>>>>>> +      HS DC Voltage level adjustment
>>>>>>
>>>>>> "Level" in what units?
>>>>>>
>>>>>
>>>>> The datasheet just shows percent, ranging from -6 to +24%, in 2%
>>>>> increments. What unit would you suggest?
>>>>
>>>> percent
>>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
>>>
>>> I am still a bit confused how to use this properly. How can I restrict
>>> the values to multiples of 2 in order to avoid illegal values?
>>>
>>> At the moment the only thing I could come up with is something like
>>>
>>> fsl,phy-tx-vref-tune-percent:
>>>    description: |
>>>      Adjusts the high-speed DC level voltage
>>>    $ref: /schemas/types.yaml#/definitions/int32
>>
>> Note that with standard unit suffixes, you don't need a type.
>>
>>>    minimum: -6
>>>    maximum: 24
>>>    default: 0
>>>
>>> Does something like this work? I am not quite sure if I am on the right
>>> track here, especially as this requires a signed int, of which I have
>>> not seen many examples so far.
>>
>> We'd have to change the type for -percent to signed. That's possible,
>> but for vendor specific properties there's not much advantage to use
>> standard units instead of just using the register values directly.
>>
> 
> I don't have any objections to that, this is pretty much what I sent in 
> my v1 patch <20230405112118.1256151-2-j.zink@pengutronix.de>, but 
> Krzysztof requested to change the vendor specific properties to use 
> property-units.
> 
> Would something along the lines of the st,trim-hs-current on 
> Documentation/devicetree/bindings/phy/phy-stm32-usbphyc.yaml be 
> acceptable (i.e. use an enum and annotate the meaning of the values in 
> the description)?
> 
> I will, nevertheless, try to make the descriptions a bit more verbose in 
> my v2 (wherever the datasheet gives me proper informations), as 
> Krzysztof requested.

gentle ping - any opinions on this? Shall I just send a V2 along the 
lines of the phy-stm32-usbphy.c?

Best regards
Johannes

> 
> Best regards
> Johannes
> 
>> Rob
>>
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

RE: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Jun Li]

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Tuesday, April 11, 2023 11:23 PM
> To: Jun Li <jun.li@nxp.com>; Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>; vkoul@kernel.org; kishon@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; Bough Chen <haibo.chen@nxp.com>;
> linux-phy@lists.infradead.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning
> properties
>
> Hi Jun,
>
> On Tue, 2023-04-11 at 14:59 +0000, Jun Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Johannes Zink <j.zink@pengutronix.de>
> > > Sent: Tuesday, April 11, 2023 10:23 PM
> > > To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>;
> > > vkoul@kernel.org; kishon@kernel.org; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > > dl-linux-imx <linux-imx@nxp.com>; robh+dt@kernel.org;
> > > krzysztof.kozlowski+dt@linaro.org; Jun Li <jun.li@nxp.com>; Bough
> > > Chen <haibo.chen@nxp.com>; linux-phy@lists.infradead.org;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org
> > > Subject: Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy
> > > tuning properties
> > >
> > > Hi Krzystof,
> > >
> > > thank you for your explanations. As I'm still quite new to writing
> > > bindings, I still have some questions:
> > >
> > > On Fri, 2023-04-07 at 11:03 +0200, Krzysztof Kozlowski wrote:
> > > > On 05/04/2023 14:14, Johannes Zink wrote:
> > > > > Hi Krysztof,
> > > > >
> > > > > thanks for your review, please find my questions below.
> > > > >
> > > > > On Wed, 2023-04-05 at 13:51 +0200, Krzysztof Kozlowski wrote:
> > > > > > [snip]
> > > > > > >        A phandle to the regulator for USB VBUS.
> > > > > > >
> > > > > > > +  fsl,phy-tx-vref-tune:
> > > > > > > +    description:
> > > > > > > +      HS DC Voltage level adjustment
> > > > > >
> > > > > > "Level" in what units?
> > > > > >
> > > > >
> > > > > The datasheet just shows percent, ranging from -6 to +24%, in 2%
> > > > > increments. What unit would you suggest?
> > > >
> > > > percent
> > > >
> > > https://gith/
> > > >
> > > ub.com%2Fdevicetree-org%2Fdt-
> > > schema%2Fblob%2Fmain%2Fdtschema%2Fschemas
> > > > %2Fproperty-
> > > > units.yaml&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a78
> > > 4
> > > >
> > > de5941d08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> > > 816
> > > >
> > > 8197947407580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> > > 2lu
> > > >
> > > MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kY4dXkOHRs
> > > 9k%
> > > > 2BeUQ5iixLYsNC8UotIgq6eOPGjbf01o%3D&reserved=0
> > >
> > > I am still a bit confused how to use this properly. How can I
> > > restrict the values to multiples of 2 in order to avoid illegal
> > > values?
> > >
> > > At the moment the only thing I could come up with is something like
> > >
> > > fsl,phy-tx-vref-tune-percent:
> > >   description: |
> > >     Adjusts the high-speed DC level voltage
> > >   $ref: /schemas/types.yaml#/definitions/int32
> > >   minimum: -6
> > >   maximum: 24
> > >   default: 0
> > >
> > > Does something like this work? I am not quite sure if I am on the
> > > right track here, especially as this requires a signed int, of which
> > > I have not seen many examples so far.
> > >
> > > Also, as far as the description is concerned: This is almost the
> > > entire information I there is in the datasheet. As I try to upstream
> > > some of the vendor downstream patches, I do not have any additional
> > > information.
> > >
> > > >
> > > > >
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13,
> > > > > > > 14,
> > > > > > > 15,
> > > > > > > 16]
> > > > > > > +
> > > > > > > +  fsl,phy-tx-rise-tune:
> > > > > > > +    description:
> > > > > > > +      HS Transmitter Rise/Fall Time Adjustment
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > +    enum: [0, 1, 2, 3]
> > > > > > > +
> > > > > > > +  fsl,phy-tx-preemp-amp-tune:
> > > > > > > +    description:
> > > > > > > +      HS Transmitter Pre-Emphasis Current Control
> > > > > >
> > > > > > If this is current then use standard unit suffixes.
> > > > >
> > > > > According to the datasheet this is in "unit amonts" of 600uA,
> > > > > basically 0x600uA, 1x600uA etc. Should I just suffix it with uA
> > > > > then?
> > > >
> > > > Yes
> > > >
> > > >
> > > https://gith/
> > > >
> > > ub.com%2Fdevicetree-org%2Fdt-
> > > schema%2Fblob%2Fmain%2Fdtschema%2Fschemas
> > > > %2Fproperty-
> > > > units.yaml&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a78
> > > 4
> > > >
> > > de5941d08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> > > 816
> > > >
> > > 8197947407580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> > > 2lu
> > > >
> > > MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kY4dXkOHRs
> > > 9k%
> > > > 2BeUQ5iixLYsNC8UotIgq6eOPGjbf01o%3D&reserved=0
> > > >
> > > > The register values can work sometimes fine but also do not scale
> > > > at all. For any other variant all the meanings will differ. Any
> > > > other
> > > > IMX8
> > > > phy will need new bindings and new description/values for your
> > > > register-like-fields.
> > >
> > > I think this particular property should work, probably its something
> > > like
> > >
> > > fsl,phy-tx-preemp-amp-tune-microamps:
> > >   description: |
> > >     Transmitter Pre-Emphasis Current Control
> > >     Controls the amount of current sourced to DPn and DMn after a
> > > J-to-
> > > K or K-to-J transition.
> > >   $ref: /schemas/types.yaml#/definitions/uint32
> > >   minimum: 0
> > >   maximum: 1800
> > >   default: 0
> > >
> > > What's the right way to communicate that the value is in multiples
> > > of 600uA and that this is only an approximate Value? Add some
> > > free-text to the description?
> > >
> > >
> > > For some other properties, such as fsl,phy-pcs-tx-swing-full or
> > > fsl,phy-pcs-tx-deemph-3p5db the datasheet provides no information at
> > > all, neither on the unit nor on a valid range. What is the proper
> > > way for something like them (I try to get some of the freescale
> > > downstream patches to mainline, but they did not even provide
> > > bindings for their
> > > driver...)
> >
> > I will check with internal design team for those not well documented
> > properties.
> >
>
> That's great, thanks!

Here is the feedback from our design team:

Per the USB specification, the TX de-emphasis value is 3.5dB nominal.
Only pcs_tx_deemph_3p5db is used for USB3.  pcs_tx_deemph_6db[5:0] is not used.
pcs_tx_deemph_3p5db[5:0] range is between 0dB to -36dB (unit step in decimal 1)
pcs_tx_swing_full[6:0] range is from 0 to full scale 127(decimal), with unit step in decimal 1.
TX amplitude is determined by ( voltage corresponding to tx_vboost_lvl) * (pcs_tx_swing_full +1 )/128
The actual TX swing value should be decided by characterization with the package and PCB.

Li Jun
>
> Johannes
>
> > Li Jun
> > >
> > >
> > > For fsl,phy-comp-dis-tune-percent, the actual values to not map well
> > > to integer amount of percent, but I have not found a permill in
> > > property- units.
> > > Also, as the steps appear quite arbitrary large, what is the correct
> > > way of restricting the values to valid values that the hardware can
> > > actually support? As reference, I have only seen stuff like the
> > > st,trim-hs- current in
> > > Documentation/devicetree/bindings/phy/phy-stm32-
> > > usbphyc.yaml so far...
> > >
> > > Thanks for helping me and best regards Johannes
> > >
> > >
> > > >
> > > > Best regards,
> > > > Krzysztof
> > > >
> > > >
> > > >
> > >
> > > --
> > > Pengutronix e.K.                | Johannes Zink                  |
> > > Steuerwalder Str. 21            |
> > > https://ww/
> > > w.p%2F&data=05%7C01%7Cjun.li%40nxp.com%7C4cfd371e93084ee8c43908db3aa
> > > 0a30c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63816823382425151
> > > 8%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi
> > > I6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=EsH%2B63B32iGAJEzk9Tc
> > > BYdcMFqnVLzr1D8t9D%2Bl0J5o%3D&reserved=0
> > > engutronix.de%2F&data=05%7C01%7Cjun.li%40nxp.com%7Ca2e1e5bb6a784de5
> > > 941d
> > > 08db3a9847d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638168197
> > > 9474
> > > 07580%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> > > CJBT
> > > iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Dq06I6tprUib5nOmp4p
> > > DFdY
> > > MgYULn8MLj5iHwVlQHMY%3D&reserved=0    |
> > > 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> > > Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> >
> >
> >
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.p/
> engutronix.de%2F&data=05%7C01%7Cjun.li%40nxp.com%7C4cfd371e93084ee8c439
> 08db3aa0a30c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6381682338242
> 51518%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBT
> iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=80YR6G0A347G%2FMI9ksARn
> VSqXVQXORt8pNBQjRoIBVY%3D&reserved=0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Johannes Zink]

Hi Rob, hi Krzysztof,

[snip]
 > gentle ping - any opinions on this? Shall I just send a V2 along the 
lines of the phy-stm32-usbphy.c?

This would be something like (example for the trim-hs-current, taken 
from pyh-stm32-usbphyc):

       st,trim-hs-current:
         description: |
           Controls HS driver current trimming for choke compensation
           - <0> = 18.87 mA target current / nominal + 0%
           - <1> = 19.165 mA target current / nominal + 1.56%
           - <2> = 19.46 mA target current / nominal + 3.12%
           - <3> = 19.755 mA target current / nominal + 4.68%
           - <4> = 20.05 mA target current / nominal + 6.24%
           - <5> = 20.345 mA target current / nominal + 7.8%
           - <6> = 20.64 mA target current / nominal + 9.36%
           - <7> = 20.935 mA target current / nominal + 10.92%
           - <8> = 21.23 mA target current / nominal + 12.48%
           - <9> = 21.525 mA target current / nominal + 14.04%
           - <10> = 21.82 mA target current / nominal + 15.6%
           - <11> = 22.115 mA target current / nominal + 17.16%
           - <12> = 22.458 mA target current / nominal + 19.01%
           - <13> = 22.755 mA target current / nominal + 20.58%
           - <14> = 23.052 mA target current / nominal + 22.16%
           - <15> = 23.348 mA target current / nominal + 23.73%
         $ref: /schemas/types.yaml#/definitions/uint32
         minimum: 0
         maximum: 15
         default: 0

If you think something along these lines is acceptable, I would like to 
prepare and send a V2.

Best regards
Johannes

[snip]

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

Re: [PATCH 1/2] dt-bindings: phy: imx8mq-usb: add phy tuning properties

[Krzysztof Kozlowski]

On 04/05/2023 09:32, Johannes Zink wrote:
> Hi Rob, hi Krzysztof,
> 
> [snip]
>  > gentle ping - any opinions on this? Shall I just send a V2 along the 
> lines of the phy-stm32-usbphy.c?
> 
> This would be something like (example for the trim-hs-current, taken 
> from pyh-stm32-usbphyc):
> 
>        st,trim-hs-current:
>          description: |
>            Controls HS driver current trimming for choke compensation
>            - <0> = 18.87 mA target current / nominal + 0%
>            - <1> = 19.165 mA target current / nominal + 1.56%
>            - <2> = 19.46 mA target current / nominal + 3.12%
>            - <3> = 19.755 mA target current / nominal + 4.68%
>            - <4> = 20.05 mA target current / nominal + 6.24%
>            - <5> = 20.345 mA target current / nominal + 7.8%
>            - <6> = 20.64 mA target current / nominal + 9.36%
>            - <7> = 20.935 mA target current / nominal + 10.92%
>            - <8> = 21.23 mA target current / nominal + 12.48%
>            - <9> = 21.525 mA target current / nominal + 14.04%
>            - <10> = 21.82 mA target current / nominal + 15.6%
>            - <11> = 22.115 mA target current / nominal + 17.16%
>            - <12> = 22.458 mA target current / nominal + 19.01%
>            - <13> = 22.755 mA target current / nominal + 20.58%
>            - <14> = 23.052 mA target current / nominal + 22.16%
>            - <15> = 23.348 mA target current / nominal + 23.73%
>          $ref: /schemas/types.yaml#/definitions/uint32
>          minimum: 0
>          maximum: 15
>          default: 0
> 
> If you think something along these lines is acceptable, I would like to 
> prepare and send a V2.
> 

Go with Rob's approach, so something as you wrote above.

Best regards,
Krzysztof


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