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

[Krzysztof Kozlowski]

On 14/03/2022 10:40, Pavan Kondeti wrote:
> Hi Krzysztof,
> 
> Thanks for your suggestions and guidance on this.
> 
> On Mon, Mar 14, 2022 at 09:36:02AM +0100, Krzysztof Kozlowski wrote:
>> On 14/03/2022 09:16, Pavan Kondeti wrote:
>>> Hi Krzysztof,
>>>
>>> On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote:
>>>> On 14/03/2022 04:29, Pavan Kondeti wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On Thu, Mar 03, 2022 at 04:59:22PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 03/03/2022 07:13, Sandeep Maheswaram wrote:
>>>>>>> Add device tree bindings for SNPS phy tuning parameters.
>>>>>>>
>>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@xxxxxxxxxxx>
>>>>>>> ---
>>>>>>>  .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 125 +++++++++++++++++++++
>>>>>>>  1 file changed, 125 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 0dfe691..227c097 100644
>>>>>>> --- a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
>>>>>>> @@ -50,6 +50,131 @@ properties:
>>>>>>>    vdda33-supply:
>>>>>>>      description: phandle to the regulator 3.3V supply node.
>>>>>>>  
>>>>>>> +  qcom,hs-disconnect:
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>> +    description:
>>>>>>> +      This adjusts the voltage level for the threshold used to
>>>>>>> +      detect a disconnect event at the host. Possible values are.
>>>>>>
>>>>>> ':', instead of full stop.
>>>>>>
>>>>>>> +      7 -> +21.56%
>>>>>>> +      6 -> +17.43%
>>>>>>> +      5 -> +13.32%
>>>>>>> +      4 -> +9.73%
>>>>>>> +      3 -> +6.3
>>>>>>> +      2 -> +3.17%
>>>>>>> +      1 -> 0, Design default%
>>>>>>
>>>>>> Use "default:" instead. Here and in other places.
>>>>>>
>>>>>>> +      0 -> -2.72%
>>>>>>
>>>>>> In current form this should be an enum... but actually current form is
>>>>>> wrong. You should not store register values in DT. What if next version
>>>>>> of hardware has a different meaning of these values?
>>>>>>
>>>>>> Instead, you should store here meaningful values, not register values.
>>>>>>
>>>>>
>>>>> Thanks for the feedback.
>>>>>
>>>>> The values in % really makes the tuning easy. People look at the eye diagram
>>>>> and decided whether to increase/decrease the margin. The absolute values
>>>>> may not be that useful. All we need is an "adjustment" here. The databook
>>>>> it self does not give any absolute values.
>>>>>
>>>>> I agree to the "enum" suggestion which we have been following for the
>>>>> qusb2 driver already. 
>>>>>
>>>>> The values have not changed in the last 5 years for this hardware block, so
>>>>> defining enums for the % values would be really helpful. 
>>>>
>>>> I did not say you cannot store here percentages. Quite opposite - store
>>>> here the percentages. Just do not store register value. No. Please read
>>>> my comment again - meaningful values are needed.
>>>>
>>>
>>> IIUC, you are asking us to come up with a meaningful values to encode the
>>> percentage values. However, all the % increments are not linear, so we can't
>>> come up with {min, max} scheme. Lets take an example of hostdisconnect
>>> threshold.
>>>
>>> As per the data book,
>>>
>>> +      7 -> +21.56%
>>> +      6 -> +17.43%
>>> +      5 -> +13.32%
>>> +      4 -> +9.73%
>>> +      3 -> +6.3
>>> +      2 -> +3.17%
>>> +      1 -> 0, Design default%
>>> +      0 -> -2.72%
>>>
>>> so how do we give meaningful values here? Does the below scheme make sense
>>> to you?
>>
>> By "meaningful value" I mean something which has a understandable
>> meaning to reader of this code or to hardware designer. For example
>> percentage values or some units (ms, ns, Hz, mA, mV). The value used in
>> register is not meaningful in that way to us because it has a meaning
>> only to the hardware block. Storing register values is more difficult to
>> read later, non-portable and non-scalable.
>>
>>>
>>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72	(-272)
>>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT	0
>>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17	317
>>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3	63
>>
>> This is some define in driver, does not look related to bindings.
>>
>>> In the driver, we have a mapping (which can be per SoC if required in future)
>>> that takes these values and convert to the correct values for a given
>>> register.
>>
>> You focus on driver but I am talking here only about bindings.
> 
> I was saying we define those defines in include/dt-bindings/phy/... header and
> use it in the device tree and as well in the driver.

Ah, I did not get it. That's not the solution for this case. defines in
dt-bindings are for constants which already can be in DT, e.g. IDs. Your
register values should not be stored in DT.

> 
>>
>> What could be the meaningful value? Percentage could work. You have
>> there a negative value, so I wonder what type of percentage is it? What
>> is the formula?
> 
> I just multiplied by 100 since device tree has no support for floating (as per
> my knowledge). The negative value represents it lowers the disconnect
> threshold by 2.72% of the default value. if it makes sense, we could also
> start from 0 like below.

ok

> 
> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0
> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT	1
> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT	2
> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT	3
> 
> The driver can have a table to map these bindings. This looks much better
> than those x100 formula values.

Again mention driver how he can map it. I mostly don't care about the
driver. :)

I think we are getting around the problem, so to emphasize again: do not
store register values in the bindings/DT but its meaning, so in your
case most likely percentages (or permille or ratio or some other value).


Best regards,
Krzysztof