Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c88396f4-4cfe-d375-1dcd-b34a6496cb06@canonical.com>
Date:   Mon, 14 Mar 2022 09:36:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy
 override params bindings
Content-Language: en-US
To:     Pavan Kondeti <quic_pkondeti@quicinc.com>
Cc:     Sandeep Maheswaram <quic_c_sanm@quicinc.com>,
        Rob Herring <robh+dt@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Vinod Koul <vkoul@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Wesley Cheng <wcheng@codeaurora.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Doug Anderson <dianders@chromium.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-phy@lists.infradead.org,
        linux-usb@vger.kernel.org, quic_ppratap@quicinc.com,
        quic_kriskura@quicinc.com
References: <1646288011-32242-1-git-send-email-quic_c_sanm@quicinc.com>
 <1646288011-32242-2-git-send-email-quic_c_sanm@quicinc.com>
 <b793195b-1d3d-63b2-19d2-72ae2aec8c0f@canonical.com>
 <20220314032952.GA27561@hu-pkondeti-hyd.qualcomm.com>
 <f1621a67-a0ff-f111-c4da-9401924e7f4a@canonical.com>
 <20220314081613.GA28402@hu-pkondeti-hyd.qualcomm.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
In-Reply-To: <20220314081613.GA28402@hu-pkondeti-hyd.qualcomm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@quicinc.com>
>>>>> ---
>>>>>  .../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.

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?

Your defines above look absolute, so maybe encode there absolute uV value?

Best regards,
Krzysztof