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

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 14/03/2022 11:30, Pavan Kondeti wrote:
> Hi Krzysztof,
> 
>>
>> 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.
>>
> These are again not register definitions. These are encodings that dT and
> driver can use. These would be constants only, no?

What do you mean it is not a register value? I don't have access to
datasheet/manual but I can clearly see code:

+	if (or->hs_disconnect.override)
+		qcom_snps_hsphy_write_mask(hsphy->base,
+			USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
+			HS_DISCONNECT_MASK,
+			or->hs_disconnect.value << HS_DISCONNECT_SHIFT);

You read the value from DT (e.g. "3" which means 6.3% for hs-disconnect)
and you write it to a register. Directly. 3 is a value for the hardware,
meaningless outside of it. It has meaning only in this one hardware
programming model. For humans it means nothing. For humans 6.3% means
something.

>>>
>>>>
>>>> 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).
>>
> 
> I am really confused on what is that you mean by not storing the registers
> here. We are only giving enum values for specific percentages supported by
> the PHY. 

The enum consists of values used in hardware registers. Values having
meaning only to this one particular hardware. Any other hardware will
not understand them.

IOW, you embed the hardware programming model in the DT. No.

> if you see -2.72 corresponds to 0 value on 0:2 bits of a register.
> I did not mention that in the device tree. we are giving constant values
> (enums) for all the possible percentage values. The user can see the
> dt-bindings file and pass the approriate value based on the compliance
> results. What is the objection?

You use some unrelated arguments. How does it matter what user can see
or cannot see?

> 
> can you please give an example if you have something in mind? 

I gave you an example - use percentages. Another example how it was done
wrong is here:
https://lore.kernel.org/linux-devicetree/c6607953-927e-4d85-21cb-72e01a121453@kernel.org/


Best regards,
Krzysztof