Re: [RFC 1/2] dt-bindings: connector: Add property to set pd timer values

[Amit Sunil Dhamne <amitsd@google.com>]

Hi Krzysztof,

On 9/27/24 12:48 AM, Krzysztof Kozlowski wrote:
> On 17/09/2024 03:59, Amit Sunil Dhamne wrote:
>> Hi Krzysztof,
>>
>> Thanks for the review!
>>
>> On 9/16/24 9:05 AM, Krzysztof Kozlowski wrote:
>>> On 11/09/2024 02:07, Amit Sunil Dhamne wrote:
>>>> This commit adds a new property "pd-timers" to enable setting of
>>>> platform/board specific pd timer values for timers that have a range of
>>>> acceptable values.
>>>>
>>>> Cc: Badhri Jagan Sridharan <badhri@google.com>
>>>> Cc: linux-usb@vger.kernel.org
>>>> Cc: devicetree@vger.kernel.org
>>>> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
>>> Please work on mainline, not ancient tree. You cannot get my CC address
>>> like that from mainline.
>> I was working off gregkh's tree on usb-next branch as that's suggested
>> for USB development.
>>
>>
>>> It's not possible. So either you don't develop
>>> on mainline or you don't use get_maintainers.pl/b4/patman.
>>>
>> The above branch and even the tree on Linus' master branch has you
>> listed as a maintainer
>> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS#n17181).
>> I guess that's why the get_maintainers script probably returned your
>> email id when I ran it. Please let me know if I missed something :).
> You really just skimmed over my email... I know how maintainers work.
>
> So I REPEAT: You cannot get this email address you Cced. Point me to the
> line in your tree having such email. The one here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS#n17181
>
> does not have it.
Sorry I misunderstood. Will fix it henceforth in subsequent work.
>>
>>>> ---
>>>>    .../bindings/connector/usb-connector.yaml     | 23 +++++++++++++++++++
>>>>    include/dt-bindings/usb/pd.h                  |  8 +++++++
>>>>    2 files changed, 31 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
>>>> index fb216ce68bb3..9be4ed12f13c 100644
>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
>>>> @@ -253,6 +253,16 @@ properties:
>>>>    
>>>>        additionalProperties: false
>>>>    
>>>> +  pd-timers:
>>>> +    description: An array of u32 integers, where an even index (i) is the timer (referenced in
>>>> +      dt-bindings/usb/pd.h) and the odd index (i+1) is the timer value in ms (refer
>>> timer of what? OS behavior?
>> In the context of USB Type C Power Delivery (PD), timers are run on the
>> typec protocol driver
>> (usb/typec/tcpm/tcpm.c).
>> These are used to keep track of min/max or range of time required to
>> enter a PD state with the
>> goal of a successful USB typec capabilities negotiation.  Eg., the timer
>> PD_TIMER_SINK_WAIT_CAP (referred to as SinkWaitCapTimer in spec)would be
>> responsible to keep track of whether a power source sent us (as sink) PD
>> source capabilities pkts within 600ms (say), if yes, then we would
>> transition to the next state or do a state machine reset. USB PD 3.1
>> spec refers to these elements as timers and therefore referred to as
>> such here.
>>
>>
>>>> +      "Table 6-68 Time Values" of "USB Power Delivery Specification Revision 3.0, Version 1.2 " for
>>>> +      the appropriate value). For certain timers the PD spec defines a range rather than a fixed
>>>> +      value. The timers may need to be tuned based on the platform. This dt property allows the user
>>> Do not describe what DT is. We all know what DT properties allow.
>>> Instead describe how this relates to hardware or boards.
>>>
>>> All this is wrongly wrapped. See Coding style (and I am not telling you
>>> the value on purpose, so you will read the coding style) .
>>
>> Ack. Thanks for pointing it out, I will fix both the above in the next
>> revision.
>>
>>
>>>> +      to assign specific values based on the platform. If these values are not explicitly defined,
>>>> +      TCPM will use a valid default value for such timers.
>>> And what is the default?
>> Defaults are given in (include/linux/usb/pd.h). But I guess I should
>> have probably mentioned
>> that here.
>>
>>
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> I guess you want matrix here.
>> Yes, I should have. Though, I will be re-implementing this such that
>> each timer is represented
>> as a separate property based on Rob and Dmitry's suggestion in
>> https://lore.kernel.org/lkml/20240916163328.GA394032-robh@kernel.org/ .
>>
>>>> +
>>>>    dependencies:
>>>>      sink-vdos-v1: [ sink-vdos ]
>>>>      sink-vdos: [ sink-vdos-v1 ]
>>>> @@ -478,3 +488,16 @@ examples:
>>>>                };
>>>>            };
>>>>        };
>>>> +
>>>> +  # USB-C connector with PD timers
>>>> +  - |
>>>> +    #include <dt-bindings/usb/pd.h>
>>>> +    usb {
>>>> +        connector {
>>>> +            compatible = "usb-c-connector";
>>>> +            label = "USB-C";
>>>> +            pd-timers =
>>>> +                <PD_TIMER_SINK_WAIT_CAP 600>,
>>>> +                <PD_TIMER_CC_DEBOUNCE 170>;
>>> Incorporate it into existing example.
>>>
>> Ack.
>>
>>
>>>> +        };
>>>> +    };
>>>> diff --git a/include/dt-bindings/usb/pd.h b/include/dt-bindings/usb/pd.h
>>>> index e6526b138174..6c58c30f3f39 100644
>>>> --- a/include/dt-bindings/usb/pd.h
>>>> +++ b/include/dt-bindings/usb/pd.h
>>>> @@ -465,4 +465,12 @@
>>>>    	 | ((vbm) & 0x3) << 15 | (curr) << 14 | ((vbi) & 0x3f) << 7	\
>>>>    	 | ((gi) & 0x3f) << 1 | (ct))
>>>>    
>>>> +/* PD Timer definitions */
>>>> +/* tTypeCSinkWaitCap (Table 6-68 Time Values, USB PD3.1 Spec) */
>>> Please expand this a bit, so we won't have to reach to external sources.
>> Ack.
>>
>> I will incorporate all of your review comments.
>>
>> Since you are no longer maintaining the
>> "OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" component, please let
> Who said that? You CC wrong emails because either you work on ancient
> tree or you do not use tools like get_maintainers.pl or b4. You cannot
> get this email from proper process. It is not physically possible
> because that email is nowhere mentioned.
>
>> me know
>> if you'd still like to be CC'ed in the subsequent revisions.
>
> Damn, just use standard tools. You are not supposed to invent maintainers.
>
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> </form letter>
>
> Best regards,
> Krzysztof

Thanks for the tips! I will make sure to follow these henceforth.

Regards,

Amit