Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
To:     Guenter Roeck <guenter@roeck-us.net>
Subject: Re: [PATCH v2] usb: typec: get the vbus source and charge values from  the devicetree
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Mon, 10 Sep 2018 08:32:48 -0600
From:   Angus Ainslie <angus@akkea.ca>
Cc:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        groeck7@gmail.com, Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <cf237e3a-30fb-a98d-ba2b-cf5485bdb5f6@roeck-us.net>
References: <20180906192644.24587-1-angus@akkea.ca>
 <20180909180531.28092-1-angus@akkea.ca>
 <20180910073529.GL25121@kuha.fi.intel.com>
 <18a1f08b114adb65baf55f202a28d85e@www.akkea.ca>
 <cf237e3a-30fb-a98d-ba2b-cf5485bdb5f6@roeck-us.net>
Message-ID: <98e0f35924b3c1e9ce0f5fff602cdc01@www.akkea.ca>
User-Agent: Roundcube Webmail/1.1.3
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2018-09-10 07:43, Guenter Roeck wrote:
> On 09/10/2018 06:11 AM, Angus Ainslie wrote:
>> Hi Heikki
>> 
>> On 2018-09-10 01:35, Heikki Krogerus wrote:
>>> On Sun, Sep 09, 2018 at 12:05:31PM -0600, Angus Ainslie (Purism) 
>>> wrote:
>>>> If the board is being powered by USB disabling the source and sink
>>>> can remove power from the board. Allow the source and sink to be
>>>> initallized based on devicetree values.
>>>> 
>>>> Changed since V1:
>>>> 
>>>> use devicetree values instead of hardcoded initialization.
>>>> 
>>>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>>>> ---
>>>>  .../bindings/connector/usb-connector.txt           |  4 ++++
>>>>  drivers/usb/typec/tcpm.c                           | 14 
>>>> +++++++++++---
>>>>  2 files changed, 15 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/connector/usb-connector.txt 
>>>> b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>> index 8855bfcfd778..afe851a713c3 100644
>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>> @@ -22,6 +22,10 @@ Optional properties for usb-c-connector:
>>>>    or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
>>>>  - data-role: should be one of "host", "device", "dual"(DRD) if 
>>>> typec
>>>>    connector supports USB data.
>>>> +- init-vbus-source: set the initalization value for vbus-source to 
>>>> true.
>>>> +  If this property is not present the initial value will be false.
>>>> +- init-vbus-charge: set the initalization value for vbus-charge to 
>>>> true.
>>>> +  If this property is not present the initial value will be false.
>>> 
>>> If you put the description of those properties here, you are going to
>>> need to rename them. Those describe tcpm specific properties, but to
>>> that file you want to put descriptions of generic properties.
>>> 
>> 
>> They are tcpm specific but need to go into the connector sub-node to 
>> get parsed correctly. Would something like below be better ?
>> 
>> diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt 
>> b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
>> index 0dd1469e7318..ae0a3e97d9b6 100644
>> --- a/Documentation/devicetree/bindings/usb/typec-tcpci.txt
>> +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
>> @@ -15,6 +15,12 @@ Required sub-node:
>>    of connector node are specified in
>>    Documentation/devicetree/bindings/connector/usb-connector.txt
>> 
>> +Optional properties for usb-c-connector sub-node:
>> +- init-vbus-source: set the initalization value for vbus-source to 
>> true.
>> +  If this property is not present the initial value will be false.
>> +- init-vbus-charge: set the initalization value for vbus-charge to 
>> true.
>> +  If this property is not present the initial value will be false.
>> +
>>  Example:
>> 
>>  ptn5110@50 {
>> 
>> 
>>> Your problem is that you can not cope with a lose of VBUS as a sink,
>>> right? For that you just need one boolean device property IMO.
>>> Something like depend-on-vbus.
>> 
>> I thought that it would better to be able to control each 
>> independently. With my specific hardware I need both defaulted to true 
>> but for another piece of HW just being able to control one of them 
>> might be sufficient.
>> 
> 
> Turns out the problem goes deeper: Only one of the two values is 
> supposed to
> be true at any given time. The system is either a sink 
> (vbus_charge=true)
> or a source (vbus_src=true), but it can not be both.
> 

I agree but my hardware is being obstinant.

> I think we first need to figure out what actually happens in your 
> system; the
> result from setting both values to true is that the initial request to 
> set vbus
> (either as source or as sink) should fail, unless the value is actually 
> cleared
> and the other value (supposed to be untouched) is set. This does not 
> make
> sense and warrants further debugging. Can you do that and let us know 
> what
> exactly actually happens and why your hardware needs that request to 
> fail ?
> 

Neither call fails at the software level.

[    3.762504] (NULL device *): tcpci drivers/usb/typec/tcpci.c 
tcpci_register_port 557
[    3.830968] tcpci 0-0052: tcpci drivers/usb/typec/tcpci.c 
tcpci_parse_config 534
[    3.849299] tcpci 0-0052: tcpci drivers/usb/typec/tcpci.c tcpci_init 
400
[    3.872283] tcpci 0-0052: tcpci drivers/usb/typec/tcpci.c 
tcpci_set_pd_rx 265
[    3.879952] tcpci 0-0052: tcpci drivers/usb/typec/tcpci.c 
tcpci_set_vbus 297 source 1 sink 1
[    3.879957] tcpci 0-0052: tcpci set source drivers/usb/typec/tcpci.c 
tcpci_set_vbus 327
[    3.916407] tcpci 0-0052: tcpci set sink drivers/usb/typec/tcpci.c 
tcpci_set_vbus 340

When the source gets disabled is when the board loses power.

[    3.762504] (NULL device *): tcpci drivers/usb/typec/tcpci.c 
tcpci_register_port 557
[    3.770603] tcpci 0-0052: tcpci drivers/usb/typec/tcpci.c 
tcpci_parse_config 534
[    3.789508] tcpci 0-0052: tcpci drivers/usb/typec/tcpci.c tcpci_init 
400
[    3.799676] tcpci 0-0052: tcpci drivers/usb/typec/tcpci.c 
tcpci_set_pd_rx 265
[    3.809322] tcpci 0-0052: tcpci drivers/usb/typec/tcpci.c 
tcpci_set_vbus 297 source 0 sink 1
[    3.817956] tcpci 0-0052: tcpci disable source 
drivers/usb/typec/tcpci.c tcpci_set_vbus 302
< board stops booting here>

I've added these prints for debugging

@@ -275,36 +293,64 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, 
bool source, bool sink)
         struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
         int ret;

-       /* Disable both source and sink first before enabling anything 
*/
+       dev_err(tcpci->dev, "tcpci %s %s %d source %d sink %d\n" , 
__FILE__,
+               __func__, __LINE__, source, sink );

+       /* Disable both source and sink first before enabling anything 
*/
         if (!source) {
+               dev_err(tcpci->dev, "tcpci disable source %s %s %d\n" , 
__FILE__,
+                       __func__, __LINE__ );
                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
                                    TCPC_CMD_DISABLE_SRC_VBUS);
-               if (ret < 0)
+               if (ret < 0) {
+                       dev_err(tcpci->dev, "tcpci disable source failed 
%s %s %d\n" , __FILE__,
+                               __func__, __LINE__ );
                         return ret;
+               }
         }

         if (!sink) {
+               dev_err(tcpci->dev, "tcpci disable sink %s %s %d\n" , 
__FILE__,
+                       __func__, __LINE__ );
+
                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
                                    TCPC_CMD_DISABLE_SINK_VBUS);
-               if (ret < 0)
+               if (ret < 0) {
+                       dev_err(tcpci->dev, "tcpci disable sink failed 
%s %s %d\n" , __FILE__,
+                               __func__, __LINE__ );
                         return ret;
+               }
         }

         if (source) {
+               dev_err(tcpci->dev, "tcpci set source %s %s %d\n" , 
__FILE__,
+                       __func__, __LINE__ );
+
                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
                                    TCPC_CMD_SRC_VBUS_DEFAULT);
-               if (ret < 0)
+               if (ret < 0) {
+                       dev_err(tcpci->dev, "tcpci enable source failed 
%s %s %d\n" , __FILE__,
+                               __func__, __LINE__ );
                         return ret;
+               }
         }

         if (sink) {
+               dev_err(tcpci->dev, "tcpci set sink %s %s %d\n" , 
__FILE__,
+                       __func__, __LINE__ );
+
                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
                                    TCPC_CMD_SINK_VBUS);
-               if (ret < 0)
+               if (ret < 0) {
+                       dev_err(tcpci->dev, "tcpci enable sink failed %s 
%s %d\n" , __FILE__,
+                               __func__, __LINE__ );
                         return ret;
+               }
         }

+       dev_err(tcpci->dev, "tcpci %s %s %d source %d sink %d\n" , 
__FILE__,
+               __func__, __LINE__, source, sink );
+
         return 0;
  }


> Thanks,
> Guenter
> 
>> Thanks
>> Angus
>> 
>>> 
>>>>  Required properties for usb-c-connector with power delivery 
>>>> support:
>>>>  - source-pdos: An array of u32 with each entry providing supported 
>>>> power
>>>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>>>> index ca7bedb46f7f..7f5d4f209e07 100644
>>>> --- a/drivers/usb/typec/tcpm.c
>>>> +++ b/drivers/usb/typec/tcpm.c
>>>> @@ -2462,9 +2462,7 @@ static int tcpm_init_vbus(struct tcpm_port 
>>>> *port)
>>>>  {
>>>>      int ret;
>>>> 
>>>> -    ret = port->tcpc->set_vbus(port->tcpc, false, false);
>>>> -    port->vbus_source = false;
>>>> -    port->vbus_charge = false;
>>>> +    ret = port->tcpc->set_vbus(port->tcpc, port->vbus_source, 
>>>> port->vbus_charge);
>>>>      return ret;
>>>>  }
>>>> 
>>>> @@ -4266,6 +4264,16 @@ static int tcpm_fw_get_caps(struct tcpm_port 
>>>> *port,
>>>>          return -EINVAL;
>>>>      port->port_type = port->typec_caps.type;
>>>> 
>>>> +    if (fwnode_property_present(fwnode, "init-vbus-source"))
>>>> +        port->vbus_source = true;
>>>> +    else
>>>> +        port->vbus_source = false;
>>>> +
>>>> +    if (fwnode_property_present(fwnode, "init-vbus-charge"))
>>>> +            port->vbus_charge = true;
>>>> +    else
>>>> +            port->vbus_charge = false;
>>>> +
>>>>      if (port->port_type == TYPEC_PORT_SNK)
>>>>          goto sink;
>>>> 
>>>> -- 2.17.1
>>> 
>>> Thanks,
>> 
>>