Message-ID: <b394b456540943b1022a7b093bf369924fca0566.camel@pengutronix.de>
Date:   Tue, 11 Apr 2023 16:22:37 +0200
From:   Johannes Zink <j.zink@...gutronix.de>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        vkoul@...nel.org, kishon@...nel.org, shawnguo@...nel.org,
        s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
        linux-imx@....com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, jun.li@....com,
        haibo.chen@....com, linux-phy@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.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://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    |

Powered by blists - more mailing lists