[RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 04/20/2018 04:13 PM, Guenter Roeck wrote:
> On 04/20/2018 03:51 AM, Jun Li wrote:
>>
>>> -----Original Message-----
>>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>>> Sent: 2018年4月20日 17:21
>>> To: Jun Li <jun.li@nxp.com>; linux@roeck-us.net;
>>> heikki.krogerus@linux.intel.com
>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx
>>> <linux-imx@nxp.com>
>>> Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo
>>>
>>> Hi,
>>>
>>> On 20-04-18 11:18, Jun Li wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>>>>> Sent: 2018年4月18日 19:40
>>>>> To: Jun Li <jun.li@nxp.com>; linux@roeck-us.net;
>>>>> heikki.krogerus@linux.intel.com
>>>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
>>>>> dl-linux-imx <linux-imx@nxp.com>
>>>>> Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw
>>>>> for rdo
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 17-04-18 08:01, Li Jun wrote:
>>>>>> This patch is a further update for rdo based on [1], which removed
>>>>>> max_snk_ma/mv/mw but kept operating_snk_mw.
>>>>>> operating_snk_mw is only used to judge capability mismatch, per PD
>>>>>> spec, we can achieve this via compare the selected source PDO and
>>>>>> matching sink PDO, also after patch [1], we don't limit the PDO
>>>>>> matching between the same type, so the rdo operating and max
>>>>>> current/power calculation should be updated accordingly.
>>>>>
>>>>> I do not believe that this is correct, lets take a device with a
>>>>> fusb302 tcpc with the following PDO-s:
>>>>>
>>>>>     PDO_FIXED(5000, 400, PDO_FIXED_FLAGS)
>>>>>     PDO_VAR(5000, 12000, 3000);
>>>>>
>>>>> And an operating_snk_mw of 2500mW then according to your new code a
>>>>> charger which supplies 12V 2A will now get the mismatch bit set even
>>>>> though it is delivering 24W.
>>>>>
>>>>
>>>> Per PD spec, my understanding is to judge capability mismatch, we
>>>> should use *max* current/power, not this operating power.
>>>
>>> Ok, I'm not familiar with the PD spec, perhaps someone who knows it well can
>>> comment here ?
>>
>> Hi Guenter,
>> Could you please comment here?
>>
> 
> I am not, and never was, the kind of person who would blindly follow a specification.
> I use specifications as guideline. For PD, when I implemented the code, I made sure
> that the implementation worked with as many power adapters and dongles as I could get
> my hand on.
> 
> At this point in time, I would only change the code myself if it is known to be broken.
> Otherwise I would leave it alone. I would _never_ make a change because my interpretation
> of the specification is different, unless I can prove that the current code is wrong and
> causes problems.
> 
> Unfortunately, I don't currently have a setup to test the current version of the code,
> nor the time to do all the testing. I am more and more concerned, though, that changes
> are made out of principle, not because something has been shown to be broken. But that is
> just a personal concern, and without re-testing everything I don't really have a strong
> case for or against those changes.

AFAIK Jun Li's previous series was actually necessary because some boards only
contain some input voltages rather then a range, so that series was triggered
by an actual problem encountered with real hardware, right Jun Li ?

But yes this patch feels like a change purely based on a (re)interpretation
of the SPEC, so lets drop this change and keep the current behavior.

Regards,

Hans




> 
> Guenter
> 
>> Thanks
>> Jun
>>
>>>
>>>> In your case(VAR PDO), the max current is 3000mA, can I say the max
>>>> power is 3000mA * 5V = 15W? so the max current required at 12V is 15W
>>>> / 12V = 1.25A, so yes, the 12V@2A is enough, do you think this kind of
>>>> calculation is reasonable?
>>>
>>> That does not sound unreasonable, the real question is when exactly should we
>>> set the capability mismatch bit, once we have that more clear how to
>>> implement this should become clear too.
>>
>> Fully agree, below is the capability mismatch bit related description from PD spec:
>>
>> "If the Capability Mismatch bit is set to one
>> The Maximum Operating Current/Power field may contain a value larger than
>> the maximum current/power offered in the Source_Capabilities Message’s PDO
>> as referenced by the Object position field. This enables the Sink to indicate that
>> it requires more current/power than is being offered. If the Sink requires a
>> different voltage this will be indicated by its Sink_Capabilities Message."
>>
>> "6.4.2.8 Maximum Operating Current
>> The Maximum Operating Current field in the Request Message shall be set to
>> the highest current the Sink will ever require. The difference between
>> the Operating Current and Maximum Operating Current fields
>> (when the GiveBack Flag is cleared) is used by the Device Policy Manager
>> in the Source to calculate the size of the Power Reserve to be maintained
>> (see Section 8.2.5.1). The Operating Current value shall be less than or
>> equal to the Maximum Operating Current value.
>> When the Capabilities Mismatch bit is set to zero the requested Maximum Operating
>> Current shall be less than or equal to the current in the offered Source Capabilities
>> since the Source will need to reserve this power for future use. ...
>> When the Capabilities Mismatch bit is set to one the requested Maximum
>> Operating Current may be greater than the current in the offered Source
>> Capabilities since the Source will need this information to ascertain
>> the Sink’s actual needs".
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>>
>>>>> I really don't see any way to fix this and I believe we should just
>>>>> keep the operating_snk_mw field.
>>>>
>>>> I had the same worry of breaking existing users, but found there is
>>>> something not matching PD spec, so want to get comments, if we really
>>>> need another input (beside sink PDO) in practice, we can keep it.
>>>>
>>>> Thanks
>>>> Jun
>>>>>
>>>>> Regards
>>>>>
>>>>> Hans
>>>>>
>>>>>
>>>>>
>>>>>> [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%
>>>>>> 2F
>>>>>>
>>>>>
>>> patchwork.kernel.org%2Fpatch%2F10342299%2F&data=02%7C01%7Cjun.li%4
>>>>> 0nxp
>>>>>> .com%7C796c943804cc4a333a7c08d5a5212e41%7C686ea1d3bc2b4c6fa9
>>> 2cd
>>>>> 99c5c30
>>>>>>
>>>>>
>>> 1635%7C0%7C0%7C636596484284807487&sdata=BH74zzVppAi7Fa8jtRt2cwc
>>>>> d%2F%2F
>>>>>> o6idNC5hk5zyoWIA4%3D&reserved=0
>>>>>>
>>>>>> Signed-off-by: Li Jun <jun.li@nxp.com>
>>>>>> ---
>>>>>>     drivers/usb/typec/tcpm.c | 52
>>>>> ++++++++++++++++++++++++++++++------------------
>>>>>>     1 file changed, 33 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>>>>>> index
>>>>>> 27192083..0be04b3 100644
>>>>>> --- a/drivers/usb/typec/tcpm.c
>>>>>> +++ b/drivers/usb/typec/tcpm.c
>>>>>> @@ -1854,28 +1854,42 @@ static int tcpm_pd_build_request(struct
>>>>> tcpm_port *port, u32 *rdo)
>>>>>>         else
>>>>>>             mv = pdo_min_voltage(pdo);
>>>>>>
>>>>>> -    /* Select maximum available current within the sink pdo's limit */
>>>>>> -    if (type == PDO_TYPE_BATT) {
>>>>>> -        mw = min_power(pdo, matching_snk_pdo);
>>>>>> -        ma = 1000 * mw / mv;
>>>>>> -    } else {
>>>>>> -        ma = min_current(pdo, matching_snk_pdo);
>>>>>> -        mw = ma * mv / 1000;
>>>>>> -    }
>>>>>> -
>>>>>>         flags = RDO_USB_COMM | RDO_NO_SUSPEND;
>>>>>>
>>>>>> -    /* Set mismatch bit if offered power is less than operating power */
>>>>>> -    max_ma = ma;
>>>>>> -    max_mw = mw;
>>>>>> -    if (mw < port->operating_snk_mw) {
>>>>>> -        flags |= RDO_CAP_MISMATCH;
>>>>>> -        if (type == PDO_TYPE_BATT &&
>>>>>> -            (pdo_max_power(matching_snk_pdo) >
>>> pdo_max_power(pdo)))
>>>>>> -            max_mw = pdo_max_power(matching_snk_pdo);
>>>>>> -        else if (pdo_max_current(matching_snk_pdo) >
>>>>>> -             pdo_max_current(pdo))
>>>>>> +    switch (type) {
>>>>>> +    case PDO_TYPE_FIXED:
>>>>>> +    case PDO_TYPE_VAR:
>>>>>> +        if (pdo_type(matching_snk_pdo) == PDO_TYPE_BATT)
>>>>>> +            max_ma = pdo_max_power(matching_snk_pdo) * 1000 /
>>> mv;
>>>>>> +        else
>>>>>>                 max_ma = pdo_max_current(matching_snk_pdo);
>>>>>> +
>>>>>> +        if (max_ma > pdo_max_current(pdo)) {
>>>>>> +            flags |= RDO_CAP_MISMATCH;
>>>>>> +            ma = pdo_max_current(pdo);
>>>>>> +        } else {
>>>>>> +            ma = max_ma;
>>>>>> +        }
>>>>>> +        break;
>>>>>> +    case PDO_TYPE_BATT:
>>>>>> +        if (pdo_type(matching_snk_pdo) == PDO_TYPE_BATT)
>>>>>> +            max_mw = pdo_max_power(matching_snk_pdo);
>>>>>> +        else
>>>>>> +            max_mw = pdo_max_current(matching_snk_pdo) *
>>>>>> +                 pdo_min_voltage(matching_snk_pdo) /
>>>>>> +                 1000;
>>>>>> +
>>>>>> +        if (max_mw > pdo_max_power(pdo)) {
>>>>>> +            flags |= RDO_CAP_MISMATCH;
>>>>>> +            mw = pdo_max_power(pdo);
>>>>>> +        } else {
>>>>>> +            mw = max_mw;
>>>>>> +        }
>>>>>> +
>>>>>> +        ma = mw * 1000 / mv;
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        break;
>>>>>>         }
>>>>>>
>>>>>>         tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s
>>>>> polarity=%d",
>>>>>>
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html