diff mbox series

[v2] usb: typec: get the vbus source and charge values from the devicetree

Message ID 20180909180531.28092-1-angus@akkea.ca (mailing list archive)
State New, archived
Headers show
Series [v2] usb: typec: get the vbus source and charge values from the devicetree | expand

Commit Message

Angus Ainslie Sept. 9, 2018, 6:05 p.m. UTC
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(-)

Comments

Guenter Roeck Sept. 9, 2018, 7:52 p.m. UTC | #1
On 09/09/2018 11:05 AM, 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.
>   
This change will require DT maintainer approval.

Thanks,
Guenter

>   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;
>
Angus Ainslie Sept. 9, 2018, 8:09 p.m. UTC | #2
+Mark
+Rob

On 2018-09-09 12:05, 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.
> 
>  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;
Heikki Krogerus Sept. 10, 2018, 7:35 a.m. UTC | #3
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.

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.

>  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,
Angus Ainslie Sept. 10, 2018, 1:11 p.m. UTC | #4
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.

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,
Guenter Roeck Sept. 10, 2018, 1:43 p.m. UTC | #5
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 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 ?

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,
>
>
Heikki Krogerus Sept. 10, 2018, 1:49 p.m. UTC | #6
Hi Angus,

On Mon, Sep 10, 2018 at 07:11:43AM -0600, Angus Ainslie wrote:
> 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.

In that file those are fine.

I would prefer init-vbus-sink over init-vbus-charge, but it's up to
you.


Thanks,
Angus Ainslie Sept. 10, 2018, 2:32 p.m. UTC | #7
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,
>> 
>>
diff mbox series

Patch

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.
 
 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;