Message-ID: <3546810e-281d-cc8d-2231-f409e69d180a@quantatw.com>
Date:   Fri, 11 Feb 2022 08:35:11 +0000
From:   Potin Lai (賴柏廷) <Potin.Lai@...ntatw.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Patrick Williams <patrick@...cx.xyz>
Subject: Re: [PATCH v2 2/2] usb: typec: fusb302: add support of
 supported_pd_rev

Guenter Roeck 於 2022/2/10 上午 01:50 寫道:
> On 2/8/22 19:34, Potin Lai wrote:
>>
>> Guenter Roeck 於 2022/2/8 下午 11:22 寫道:
>>> On 2/8/22 03:22, Potin Lai wrote:
>>>> Add support for passing supported PD rev to TCPM.
>>>> If "supported-pd-rev" property exist, then return supported_pd_rev as
>>>> defined value in DTS, otherwise return PD_MAX_REV
>>>>
>>>> Example of DTS:
>>>>
>>>> fusb302: typec-portc@22 {
>>>>      compatible = "fcs,fusb302";
>>>>      reg = <0x22>;
>>>>      ...
>>>>      supported-pd-rev=<1>; // PD_REV20
>>>>      ...
>>>> };
>>>>
>>>
>>> The new property needs to be documented. However, I am not entirely sure
>>> I understand why it is needed. Wouldn't the supported PD revision
>>> be a chip (fusb302) specific constant ? If so, why does it need a
>>> devicetree property ? I think that needs some additional explanation.
>>>
>>> Thanks,
>>> Guenter
>>>
>>
>> My initially intend was adding flexibility for developer to decided which PD revision
>> they want to use for negotiation.
>>
>
> I may be missing something, but I don't think that "flexibility for developer
> to decide which PD revision they want to use for negotiation" is entirely appropriate.
> This should be a chip property, not something a developer should decide. As written,
> the code does accept PC version 3 for FUSB302 by default, which seems odd and unusual.
> If the chip supports PD version 3, why artificially limit it to earlier PD revisions ?
>
> I think this requires a detailed description of the use case. Is fusb302 indeed not able
> to support a specific version of the power delivery specification ? If yes,
> what is the limitation, and why would it need to be configurable with a devicetree
> property ?
>
> Thanks,
> Guenter
>

[  414.375215] AMS DISCOVER_IDENTITY start
[  414.375228] cc:=4
[  414.380834] PD RX, header: 0x291 [1]
[  414.380846] AMS DISCOVER_IDENTITY finished
[  414.380850] cc:=5
[  414.388203] PD RX, header: 0x291 [1]
[  414.388211] PD RX, header: 0x291 [1]
[  414.388220] PD TX, header: 0x7b0
[  414.499594] Received hard reset
[  414.499615] state change SRC_READY -> HARD_RESET_START [rev3 HARD_RESET]


In my case, if I keep it as PD_MAX_REV (3.0), I always receive hard reset after "PD RX, header: 0x291" (Get_Source_Cap_Extended), then entire state machine will start over again and again.

If I force PD rev at 2.0, then I won't receive Get_Source_Cap_Extended message, and state machine become stable.

Not quite sure the reason of receiving hard reset, my guessing, fus302 not recognize PD 3.0 message, so it doesn't reply GoodCRC automatically, and then it causing timeout or events to trigger hard reset from the other side.

Thanks,
Potin
>> I saw your reply in another patch,  I agree with you, it will be simple and consistent if
>> move "supported-pd-rev" to tcpm fwnode as other capabilities.
>>
>> Just want to double confirm, is "usb-connector.yaml" right place to put documentation
>> if adding "supported-pd-rev" in fwnode?
>>
>> Thanks,
>> Potin
>>
>>>> Signed-off-by: Potin Lai <potin.lai@...ntatw.com>
>>>> ---
>>>>   drivers/usb/typec/tcpm/fusb302.c | 20 ++++++++++++++++++++
>>>>   1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
>>>> index 72f9001b0792..8cff92d58b96 100644
>>>> --- a/drivers/usb/typec/tcpm/fusb302.c
>>>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>>>> @@ -109,6 +109,9 @@ struct fusb302_chip {
>>>>       enum typec_cc_status cc2;
>>>>       u32 snk_pdo[PDO_MAX_OBJECTS];
>>>>   +    /* supported pd rev */
>>>> +    u32 supported_pd_rev;
>>>> +
>>>>   #ifdef CONFIG_DEBUG_FS
>>>>       struct dentry *dentry;
>>>>       /* lock for log buffer access */
>>>> @@ -1056,6 +1059,13 @@ static int tcpm_pd_transmit(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>>>>       return ret;
>>>>   }
>>>>   +static u32 tcpm_supported_pd_rev(struct tcpc_dev *dev)
>>>> +{
>>>> +    struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
>>>> +                         tcpc_dev);
>>>> +    return chip->supported_pd_rev;
>>>> +}
>>>> +
>>>>   static enum typec_cc_status fusb302_bc_lvl_to_cc(u8 bc_lvl)
>>>>   {
>>>>       if (bc_lvl == FUSB_REG_STATUS0_BC_LVL_1230_MAX)
>>>> @@ -1129,6 +1139,7 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
>>>>       fusb302_tcpc_dev->set_roles = tcpm_set_roles;
>>>>       fusb302_tcpc_dev->start_toggling = tcpm_start_toggling;
>>>>       fusb302_tcpc_dev->pd_transmit = tcpm_pd_transmit;
>>>> +    fusb302_tcpc_dev->supported_pd_rev = tcpm_supported_pd_rev;
>>>>   }
>>>>     static const char * const cc_polarity_name[] = {
>>>> @@ -1683,6 +1694,7 @@ static int fusb302_probe(struct i2c_client *client,
>>>>       struct fusb302_chip *chip;
>>>>       struct i2c_adapter *adapter = client->adapter;
>>>>       struct device *dev = &client->dev;
>>>> +    struct device_node *np = dev->of_node;
>>>>       const char *name;
>>>>       int ret = 0;
>>>>   @@ -1756,6 +1768,14 @@ static int fusb302_probe(struct i2c_client *client,
>>>>           dev_err(dev, "cannot request IRQ for GPIO Int_N, ret=%d", ret);
>>>>           goto tcpm_unregister_port;
>>>>       }
>>>> +
>>>> +    if (of_property_read_u32(np, "supported-pd-rev",
>>>> +                &chip->supported_pd_rev) < 0) {
>>>> +        chip->supported_pd_rev = PD_MAX_REV;
>>>> +    } else if (chip->supported_pd_rev > PD_MAX_REV) {
>>>> +        chip->supported_pd_rev = PD_MAX_REV;
>>>> +    }
>>>
>>> The else part is also checked in the tcpm code and thus seems
>>> to be redundant.
>>>
>>>> +
>>>>       enable_irq_wake(chip->gpio_int_n_irq);
>>>>       i2c_set_clientdata(client, chip);
>>>
>

Powered by blists - more mailing lists