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@xxxxxxxxxxxxxx>
> > Sent: Tuesday, April 11, 2023 10:23 PM
> > To: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>;
> > vkoul@xxxxxxxxxx; kishon@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> > s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> > dl-linux-imx <linux-imx@xxxxxxx>; robh+dt@xxxxxxxxxx;
> > krzysztof.kozlowski+dt@xxxxxxxxxx; Jun Li <jun.li@xxxxxxx>; Bough
> > Chen
> > <haibo.chen@xxxxxxx>; linux-phy@xxxxxxxxxxxxxxxxxxx;
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx
> > 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    |