[PATCH] usb: typec: don't disable sink or source on initialization

[PATCH] usb: typec: don't disable sink or source on initialization

[Angus Ainslie (Purism)]

If the board is being powered by USB disabling the source and sink
can remove power from the board. Default to source and sink enabled.

Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
 drivers/usb/typec/tcpm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index ca7bedb46f7f..a1b819cf31da 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -2462,9 +2462,11 @@ 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;
+	/* default to source and sink enabled in case USB is our only power
+	 * source */
+	ret = port->tcpc->set_vbus(port->tcpc, true, true);
+	port->vbus_source = true;
+	port->vbus_charge = true;
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH] usb: typec: don't disable sink or source on initialization

[Sergei Shtylyov]

Hello!

On 9/6/2018 10:26 PM, Angus Ainslie (Purism) wrote:

> If the board is being powered by USB disabling the source and sink
> can remove power from the board. Default to source and sink enabled.
> 
> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
>   drivers/usb/typec/tcpm.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index ca7bedb46f7f..a1b819cf31da 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -2462,9 +2462,11 @@ 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;
> +	/* default to source and sink enabled in case USB is our only power
> +	 * source */

    The multi-line comments should look like this:

/*
  * bla
  * bla
  */

[...]

MBR, Sergei

Re: [PATCH] usb: typec: don't disable sink or source on initialization

[Heikki Krogerus]

+Guenter

On Thu, Sep 06, 2018 at 01:26:44PM -0600, Angus Ainslie (Purism) wrote:
> If the board is being powered by USB disabling the source and sink
> can remove power from the board. Default to source and sink enabled.
> 
> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
>  drivers/usb/typec/tcpm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index ca7bedb46f7f..a1b819cf31da 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -2462,9 +2462,11 @@ 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;
> +	/* default to source and sink enabled in case USB is our only power
> +	 * source */
> +	ret = port->tcpc->set_vbus(port->tcpc, true, true);
> +	port->vbus_source = true;
> +	port->vbus_charge = true;
>  	return ret;
>  }

-- 
heikki

Re: [PATCH] usb: typec: don't disable sink or source on initialization

[Guenter Roeck]

On 09/07/2018 03:34 AM, Heikki Krogerus wrote:
> +Guenter
> 
> On Thu, Sep 06, 2018 at 01:26:44PM -0600, Angus Ainslie (Purism) wrote:
>> If the board is being powered by USB disabling the source and sink
>> can remove power from the board. Default to source and sink enabled.
>>

Seems to me that might violate the specification, and cause trouble for systems
where vbus has to be off initially. It may be better to provide this kind of
information as configuration parameter instead of imposing it on everyone.

Guenter

>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>> ---
>>   drivers/usb/typec/tcpm.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>> index ca7bedb46f7f..a1b819cf31da 100644
>> --- a/drivers/usb/typec/tcpm.c
>> +++ b/drivers/usb/typec/tcpm.c
>> @@ -2462,9 +2462,11 @@ 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;
>> +	/* default to source and sink enabled in case USB is our only power
>> +	 * source */

I am personally in favor of standard multi-line comments.

>> +	ret = port->tcpc->set_vbus(port->tcpc, true, true);
>> +	port->vbus_source = true;
>> +	port->vbus_charge = true;
>>   	return ret;
>>   }
> 

Re: [PATCH] usb: typec: don't disable sink or source on initialization

[Angus Ainslie]

Hi Guenter

On 2018-09-07 06:55, Guenter Roeck wrote:
> On 09/07/2018 03:34 AM, Heikki Krogerus wrote:
>> +Guenter
>> 
>> On Thu, Sep 06, 2018 at 01:26:44PM -0600, Angus Ainslie (Purism) 
>> wrote:
>>> If the board is being powered by USB disabling the source and sink
>>> can remove power from the board. Default to source and sink enabled.
>>> 
> 
> Seems to me that might violate the specification, and cause trouble for 
> systems
> where vbus has to be off initially. It may be better to provide this 
> kind of
> information as configuration parameter instead of imposing it on 
> everyone.
> 

It felt like it would not be the correct thing to do either. I've tried 
re-arranging the code in tcpci.c to enable the sink before disabling the 
source but the only way I found to not disable the power was by setting 
both of those to true.

What about adding some device tree entries for the initial vbus state 
and default to false if they are not present ?

init-vbus-source and init-vbus-charge ?

Angus

> Guenter
> 
>>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>>> ---
>>>   drivers/usb/typec/tcpm.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>>> index ca7bedb46f7f..a1b819cf31da 100644
>>> --- a/drivers/usb/typec/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm.c
>>> @@ -2462,9 +2462,11 @@ 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;
>>> +	/* default to source and sink enabled in case USB is our only power
>>> +	 * source */
> 
> I am personally in favor of standard multi-line comments.
> 
>>> +	ret = port->tcpc->set_vbus(port->tcpc, true, true);
>>> +	port->vbus_source = true;
>>> +	port->vbus_charge = true;
>>>   	return ret;
>>>   }
>> 

Re: [PATCH] usb: typec: don't disable sink or source on initialization

[Guenter Roeck]

On 09/09/2018 07:08 AM, Angus Ainslie wrote:
> Hi Guenter
> 
> On 2018-09-07 06:55, Guenter Roeck wrote:
>> On 09/07/2018 03:34 AM, Heikki Krogerus wrote:
>>> +Guenter
>>>
>>> On Thu, Sep 06, 2018 at 01:26:44PM -0600, Angus Ainslie (Purism) wrote:
>>>> If the board is being powered by USB disabling the source and sink
>>>> can remove power from the board. Default to source and sink enabled.
>>>>
>>
>> Seems to me that might violate the specification, and cause trouble for systems
>> where vbus has to be off initially. It may be better to provide this kind of
>> information as configuration parameter instead of imposing it on everyone.
>>
> 
> It felt like it would not be the correct thing to do either. I've tried re-arranging the code in tcpci.c to enable the sink before disabling the source but the only way I found to not disable the power was by setting both of those to true.
> 
> What about adding some device tree entries for the initial vbus state and default to false if they are not present ?
> 
> init-vbus-source and init-vbus-charge ?
> 

Yes, I think we should do something along that line. Another question is
if we could or should optionally pull the current state from the low level
driver, but that may be secondary.

Thanks,
Guenter

> Angus
> 
>> Guenter
>>
>>>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>>>> ---
>>>>   drivers/usb/typec/tcpm.c | 8 +++++---
>>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>>>> index ca7bedb46f7f..a1b819cf31da 100644
>>>> --- a/drivers/usb/typec/tcpm.c
>>>> +++ b/drivers/usb/typec/tcpm.c
>>>> @@ -2462,9 +2462,11 @@ 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;
>>>> +    /* default to source and sink enabled in case USB is our only power
>>>> +     * source */
>>
>> I am personally in favor of standard multi-line comments.
>>
>>>> +    ret = port->tcpc->set_vbus(port->tcpc, true, true);
>>>> +    port->vbus_source = true;
>>>> +    port->vbus_charge = true;
>>>>       return ret;
>>>>   }
>>>
> 
> 

Re: [PATCH] usb: typec: don't disable sink or source on initialization

[Angus Ainslie]

On 2018-09-09 08:20, Guenter Roeck wrote:
> On 09/09/2018 07:08 AM, Angus Ainslie wrote:
>> Hi Guenter
>> 
>> On 2018-09-07 06:55, Guenter Roeck wrote:
>>> On 09/07/2018 03:34 AM, Heikki Krogerus wrote:
>>>> +Guenter
>>>> 
>>>> On Thu, Sep 06, 2018 at 01:26:44PM -0600, Angus Ainslie (Purism) 
>>>> wrote:
>>>>> If the board is being powered by USB disabling the source and sink
>>>>> can remove power from the board. Default to source and sink 
>>>>> enabled.
>>>>> 
>>> 
>>> Seems to me that might violate the specification, and cause trouble 
>>> for systems
>>> where vbus has to be off initially. It may be better to provide this 
>>> kind of
>>> information as configuration parameter instead of imposing it on 
>>> everyone.
>>> 
>> 
>> It felt like it would not be the correct thing to do either. I've 
>> tried re-arranging the code in tcpci.c to enable the sink before 
>> disabling the source but the only way I found to not disable the power 
>> was by setting both of those to true.
>> 
>> What about adding some device tree entries for the initial vbus state 
>> and default to false if they are not present ?
>> 
>> init-vbus-source and init-vbus-charge ?
>> 
> 
> Yes, I think we should do something along that line. Another question 
> is
> if we could or should optionally pull the current state from the low 
> level
> driver, but that may be secondary.
> 

I thought of trying to do that but from reading the "PTN5110N PD PHY 
application programming guide" there didn't seem to be a direct way to 
read those parameters.

> Thanks,
> Guenter
> 
>> Angus
>> 
>>> Guenter
>>> 
>>>>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>>>>> ---
>>>>>   drivers/usb/typec/tcpm.c | 8 +++++---
>>>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>>>>> index ca7bedb46f7f..a1b819cf31da 100644
>>>>> --- a/drivers/usb/typec/tcpm.c
>>>>> +++ b/drivers/usb/typec/tcpm.c
>>>>> @@ -2462,9 +2462,11 @@ 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;
>>>>> +    /* default to source and sink enabled in case USB is our only 
>>>>> power
>>>>> +     * source */
>>> 
>>> I am personally in favor of standard multi-line comments.
>>> 
>>>>> +    ret = port->tcpc->set_vbus(port->tcpc, true, true);
>>>>> +    port->vbus_source = true;
>>>>> +    port->vbus_charge = true;
>>>>>       return ret;
>>>>>   }
>>>> 
>> 
>> 

Re: [PATCH] usb: typec: don't disable sink or source on initialization

[Guenter Roeck]

On 09/09/2018 07:36 AM, Angus Ainslie wrote:
> On 2018-09-09 08:20, Guenter Roeck wrote:
>> On 09/09/2018 07:08 AM, Angus Ainslie wrote:
>>> Hi Guenter
>>>
>>> On 2018-09-07 06:55, Guenter Roeck wrote:
>>>> On 09/07/2018 03:34 AM, Heikki Krogerus wrote:
>>>>> +Guenter
>>>>>
>>>>> On Thu, Sep 06, 2018 at 01:26:44PM -0600, Angus Ainslie (Purism) wrote:
>>>>>> If the board is being powered by USB disabling the source and sink
>>>>>> can remove power from the board. Default to source and sink enabled.
>>>>>>
>>>>
>>>> Seems to me that might violate the specification, and cause trouble for systems
>>>> where vbus has to be off initially. It may be better to provide this kind of
>>>> information as configuration parameter instead of imposing it on everyone.
>>>>
>>>
>>> It felt like it would not be the correct thing to do either. I've tried re-arranging the code in tcpci.c to enable the sink before disabling the source but the only way I found to not disable the power was by setting both of those to true.
>>>
>>> What about adding some device tree entries for the initial vbus state and default to false if they are not present ?
>>>
>>> init-vbus-source and init-vbus-charge ?
>>>
>>
>> Yes, I think we should do something along that line. Another question is
>> if we could or should optionally pull the current state from the low level
>> driver, but that may be secondary.
>>
> 
> I thought of trying to do that but from reading the "PTN5110N PD PHY application programming guide" there didn't seem to be a direct way to read those parameters.
> 

Let's put that on the backburner (maybe it works for some other chips)
and go with the devicetree property for now.

Guenter


>> Thanks,
>> Guenter
>>
>>> Angus
>>>
>>>> Guenter
>>>>
>>>>>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>>>>>> ---
>>>>>>   drivers/usb/typec/tcpm.c | 8 +++++---
>>>>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>>>>>> index ca7bedb46f7f..a1b819cf31da 100644
>>>>>> --- a/drivers/usb/typec/tcpm.c
>>>>>> +++ b/drivers/usb/typec/tcpm.c
>>>>>> @@ -2462,9 +2462,11 @@ 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;
>>>>>> +    /* default to source and sink enabled in case USB is our only power
>>>>>> +     * source */
>>>>
>>>> I am personally in favor of standard multi-line comments.
>>>>
>>>>>> +    ret = port->tcpc->set_vbus(port->tcpc, true, true);
>>>>>> +    port->vbus_source = true;
>>>>>> +    port->vbus_charge = true;
>>>>>>       return ret;
>>>>>>   }
>>>>>
>>>
>>>
> 
> 

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

[Angus Ainslie (Purism)]

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

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

[Guenter Roeck]

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

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

[Angus Ainslie]

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

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

[Heikki Krogerus]

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,

-- 
heikki

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

[Angus Ainslie]

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,

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

[Guenter Roeck]

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,
>
>

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

[Heikki Krogerus]

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,

-- 
heikki

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

[Angus Ainslie]

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,
>> 
>> 

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

[Angus Ainslie (Purism)]

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 V2:

Change the devicetree documentation.
Change the devicetree property names.

Changed since V1:

use devicetree values instead of hardcoded initialization.

Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
 .../devicetree/bindings/usb/typec-tcpci.txt        |  6 ++++++
 drivers/usb/typec/tcpm.c                           | 14 +++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
index 0dd1469e7318..b07418ae6482 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-sink: set the initalization value for vbus-sink to true.
+  If this property is not present the initial value will be false.
+
 Example:
 
 ptn5110@50 {
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index ca7bedb46f7f..10c14ece3858 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-sink"))
+	        port->vbus_charge = true;
+	else
+	        port->vbus_charge = false;
+
 	if (port->port_type == TYPEC_PORT_SNK)
 		goto sink;
 
-- 
2.17.1

Re: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Guenter Roeck]

I cant put my finger on it but this seems wrong. As i said both src and sink should never be true at the same time. I also din’t understand why turning off src should power off your board. Ultimately my concern is that we may be just painting over the real problem, and that would be really bad to do with dt properties.

Note that sending to groeck7@ makes it hard for me to see your emails and to reply. Using linux@roeck-us.net would be much better.

Guenter

Sent from my iPhone

> On Sep 11, 2018, at 7:59 AM, Angus Ainslie (Purism) <angus@akkea.ca> 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 V2:
> 
> Change the devicetree documentation.
> Change the devicetree property names.
> 
> Changed since V1:
> 
> use devicetree values instead of hardcoded initialization.
> 
> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
> .../devicetree/bindings/usb/typec-tcpci.txt        |  6 ++++++
> drivers/usb/typec/tcpm.c                           | 14 +++++++++++---
> 2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> index 0dd1469e7318..b07418ae6482 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-sink: set the initalization value for vbus-sink to true.
> +  If this property is not present the initial value will be false.
> +
> Example:
> 
> ptn5110@50 {
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index ca7bedb46f7f..10c14ece3858 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-sink"))
> +            port->vbus_charge = true;
> +    else
> +            port->vbus_charge = false;
> +
>    if (port->port_type == TYPEC_PORT_SNK)
>        goto sink;
> 
> -- 
> 2.17.1
> 

Re: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Angus Ainslie]

On 2018-09-11 09:33, Guenter Roeck wrote:
> I cant put my finger on it but this seems wrong. As i said both src
> and sink should never be true at the same time. I also din’t
> understand why turning off src should power off your board. Ultimately
> my concern is that we may be just painting over the real problem, and
> that would be really bad to do with dt properties.
> 

I agree that this doesn't seem like the correct way of solving the 
problem. On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has 
been connected correctly so I'm assuming that it is some quirk of the 
PTN5110.

I didn't design the HW or the chip. This is a workaround for "quirky" 
hardware and there may be others that don't behave exactly as expected.

> Note that sending to groeck7@ makes it hard for me to see your emails
> and to reply. Using linux@roeck-us.net would be much better.
> 

Ok, I think that email was grabbed from the driver source.

> Guenter
> 
> Sent from my iPhone
> 
>> On Sep 11, 2018, at 7:59 AM, Angus Ainslie (Purism) <angus@akkea.ca> 
>> 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 V2:
>> 
>> Change the devicetree documentation.
>> Change the devicetree property names.
>> 
>> Changed since V1:
>> 
>> use devicetree values instead of hardcoded initialization.
>> 
>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>> ---
>> .../devicetree/bindings/usb/typec-tcpci.txt        |  6 ++++++
>> drivers/usb/typec/tcpm.c                           | 14 +++++++++++---
>> 2 files changed, 17 insertions(+), 3 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt 
>> b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
>> index 0dd1469e7318..b07418ae6482 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-sink: set the initalization value for vbus-sink to true.
>> +  If this property is not present the initial value will be false.
>> +
>> Example:
>> 
>> ptn5110@50 {
>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>> index ca7bedb46f7f..10c14ece3858 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-sink"))
>> +            port->vbus_charge = true;
>> +    else
>> +            port->vbus_charge = false;
>> +
>>    if (port->port_type == TYPEC_PORT_SNK)
>>        goto sink;
>> 
>> --
>> 2.17.1
>> 

Re: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Guenter Roeck]

On Wed, Sep 12, 2018 at 10:08:58AM -0600, Angus Ainslie wrote:
> On 2018-09-11 09:33, Guenter Roeck wrote:
> >I cant put my finger on it but this seems wrong. As i said both src
> >and sink should never be true at the same time. I also din’t
> >understand why turning off src should power off your board. Ultimately
> >my concern is that we may be just painting over the real problem, and
> >that would be really bad to do with dt properties.
> >
> 
> I agree that this doesn't seem like the correct way of solving the problem.
> On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been connected
> correctly so I'm assuming that it is some quirk of the PTN5110.
> 
> I didn't design the HW or the chip. This is a workaround for "quirky"
> hardware and there may be others that don't behave exactly as expected.
> 

I wouldn't be that sure about that. It may as well be that the tcpc driver
and/or the tcpm driver are doing something wrong when initializing.

I didn't really understand the logs you sent out earlier. It looked like
the system would loose power if the TCPC_CMD_DISABLE_SRC_VBUS command is
sent.  That doesn't really make sense to me since it indicates that the
chip sources power to the remote, and turning that off should not result
in a local loss of power.

Note that the chip is supposed to be able to report if it is sourcing vbus
and if VBUS is present, in the POWER_STATUS register. Another question is
the content of the ROLE_CONTROL register when the system boots, and the
DEVICE_CAPABILITIES settings.

Overall I suspect that we don't handle startup for your system correctly
in the tcpc driver. The ideal solution would be to find a solution which
does not require any devicetree properties, but to do that we'll need
to get a better understanding about your system's requirements.

Guenter

Re: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Peter Chen]

On Thu, Sep 13, 2018 at 12:35 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Sep 12, 2018 at 10:08:58AM -0600, Angus Ainslie wrote:
> > On 2018-09-11 09:33, Guenter Roeck wrote:
> > >I cant put my finger on it but this seems wrong. As i said both src
> > >and sink should never be true at the same time. I also din’t
> > >understand why turning off src should power off your board. Ultimately
> > >my concern is that we may be just painting over the real problem, and
> > >that would be really bad to do with dt properties.
> > >
> >
> > I agree that this doesn't seem like the correct way of solving the problem.
> > On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been connected
> > correctly so I'm assuming that it is some quirk of the PTN5110.
> >
> > I didn't design the HW or the chip. This is a workaround for "quirky"
> > hardware and there may be others that don't behave exactly as expected.
> >
>
> I wouldn't be that sure about that. It may as well be that the tcpc driver
> and/or the tcpm driver are doing something wrong when initializing.
>
> I didn't really understand the logs you sent out earlier. It looked like
> the system would loose power if the TCPC_CMD_DISABLE_SRC_VBUS command is
> sent.  That doesn't really make sense to me since it indicates that the
> chip sources power to the remote, and turning that off should not result
> in a local loss of power.
>
> Note that the chip is supposed to be able to report if it is sourcing vbus
> and if VBUS is present, in the POWER_STATUS register. Another question is
> the content of the ROLE_CONTROL register when the system boots, and the
> DEVICE_CAPABILITIES settings.
>
> Overall I suspect that we don't handle startup for your system correctly
> in the tcpc driver. The ideal solution would be to find a solution which
> does not require any devicetree properties, but to do that we'll need
> to get a better understanding about your system's requirements.
>
> Guenter

Hi Angus,

Would you please check if below patch can fix your issue?

staging: typec: don't do vbus source disable for dead battery

In PTN5110 design, DisableSourceVBUS command also disables the sink
enable signal because the EN_SNK can be used to source higher voltage,
and, there is only one TCPC command to disable sourcing voltage without
telling whether to disable 5V or the high voltage, and to keep the
design simple they designed the PTN5110 to disable both. with this
fact, we use the flag drive_vbus to check if the source vbus enable was
issued, if yes we then do vbus source disable, in dead battery case,
we never did vbus source enable, so will not issue vbus source disable
command.

Acked-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/typec/tcpci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 2d4fbb8aac5e..7352207224b5 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -381,9 +381,8 @@ 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 */
-
- if (!source) {
+ /* Only disable source if it was enabled */
+ if (!source && tcpci->drive_vbus) {
  ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
     TCPC_CMD_DISABLE_SRC_VBUS);
  if (ret < 0)

Re: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Angus Ainslie]

On 2018-09-13 01:27, Peter Chen wrote:
> On Thu, Sep 13, 2018 at 12:35 AM Guenter Roeck <linux@roeck-us.net> 
> wrote:
>> 
>> On Wed, Sep 12, 2018 at 10:08:58AM -0600, Angus Ainslie wrote:
>> > On 2018-09-11 09:33, Guenter Roeck wrote:
>> > >I cant put my finger on it but this seems wrong. As i said both src
>> > >and sink should never be true at the same time. I also din’t
>> > >understand why turning off src should power off your board. Ultimately
>> > >my concern is that we may be just painting over the real problem, and
>> > >that would be really bad to do with dt properties.
>> > >
>> >
>> > I agree that this doesn't seem like the correct way of solving the problem.
>> > On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been connected
>> > correctly so I'm assuming that it is some quirk of the PTN5110.
>> >
>> > I didn't design the HW or the chip. This is a workaround for "quirky"
>> > hardware and there may be others that don't behave exactly as expected.
>> >
>> 
>> I wouldn't be that sure about that. It may as well be that the tcpc 
>> driver
>> and/or the tcpm driver are doing something wrong when initializing.
>> 
>> I didn't really understand the logs you sent out earlier. It looked 
>> like
>> the system would loose power if the TCPC_CMD_DISABLE_SRC_VBUS command 
>> is
>> sent.  That doesn't really make sense to me since it indicates that 
>> the
>> chip sources power to the remote, and turning that off should not 
>> result
>> in a local loss of power.
>> 
>> Note that the chip is supposed to be able to report if it is sourcing 
>> vbus
>> and if VBUS is present, in the POWER_STATUS register. Another question 
>> is
>> the content of the ROLE_CONTROL register when the system boots, and 
>> the
>> DEVICE_CAPABILITIES settings.
>> 
>> Overall I suspect that we don't handle startup for your system 
>> correctly
>> in the tcpc driver. The ideal solution would be to find a solution 
>> which
>> does not require any devicetree properties, but to do that we'll need
>> to get a better understanding about your system's requirements.
>> 
>> Guenter
> 
> Hi Angus,
> 
> Would you please check if below patch can fix your issue?
> 
> staging: typec: don't do vbus source disable for dead battery
> 
> In PTN5110 design, DisableSourceVBUS command also disables the sink
> enable signal because the EN_SNK can be used to source higher voltage,
> and, there is only one TCPC command to disable sourcing voltage without
> telling whether to disable 5V or the high voltage, and to keep the
> design simple they designed the PTN5110 to disable both. with this
> fact, we use the flag drive_vbus to check if the source vbus enable was
> issued, if yes we then do vbus source disable, in dead battery case,
> we never did vbus source enable, so will not issue vbus source disable
> command.
> 

Thanks Peter, this sounds like the missing piece of information and I 
think some form of the code below will fix that.

There is still the issue that my board will need some way of controlling 
the initial state of vbus-sink.

@Guenter: would my initial patch be acceptable to set the default state 
of vbus-source and vbus-sink. Would you like some code to sanity check 
that both were not enabled at the same time ?

> Acked-by: Peter Chen <peter.chen@nxp.com>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  drivers/staging/typec/tcpci.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c 
> b/drivers/staging/typec/tcpci.c
> index 2d4fbb8aac5e..7352207224b5 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -381,9 +381,8 @@ 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 */
> -
> - if (!source) {
> + /* Only disable source if it was enabled */
> + if (!source && tcpci->drive_vbus) {
>   ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
>      TCPC_CMD_DISABLE_SRC_VBUS);
>   if (ret < 0)

The version of struct tcpci doesn't have a drive_vbus. Where should 
drive_vbus get set and cleared ?

Is this a more complete version of what you intended ?

diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c
index ac6b418b15f1..d6168163df7b 100644
--- a/drivers/usb/typec/tcpci.c
+++ b/drivers/usb/typec/tcpci.c
@@ -28,6 +28,7 @@ struct tcpci {
         struct regmap *regmap;

         bool controls_vbus;
+       bool drive_vbus;

         struct tcpc_dev tcpc;
         struct tcpci_data *data;
@@ -277,7 +278,9 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, 
bool source, bool sink)

         /* Disable both source and sink first before enabling anything 
*/

-       if (!source) {
+       if (!source && tcpci->drive_vbus) {
+               tcpci->drive_vbus = false;
+
                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
                                    TCPC_CMD_DISABLE_SRC_VBUS);
                 if (ret < 0)
@@ -292,6 +295,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, 
bool source, bool sink)
         }

         if (source) {
+               tcpci->drive_vbus = true;
+
                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
                                    TCPC_CMD_SRC_VBUS_DEFAULT);
                 if (ret < 0)
@@ -503,6 +508,7 @@ struct tcpci *tcpci_register_port(struct device 
*dev, struct tcpci_data *data)
         tcpci->dev = dev;
         tcpci->data = data;
         tcpci->regmap = data->regmap;
+       tcpci->drive_vbus = false;

         tcpci->tcpc.init = tcpci_init;
         tcpci->tcpc.get_vbus = tcpci_get_vbus;

Re: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Guenter Roeck]

On Thu, Sep 13, 2018 at 05:10:09AM -0600, Angus Ainslie wrote:
> >
> >staging: typec: don't do vbus source disable for dead battery
> >
> >In PTN5110 design, DisableSourceVBUS command also disables the sink
> >enable signal because the EN_SNK can be used to source higher voltage,
> >and, there is only one TCPC command to disable sourcing voltage without
> >telling whether to disable 5V or the high voltage, and to keep the
> >design simple they designed the PTN5110 to disable both. with this
> >fact, we use the flag drive_vbus to check if the source vbus enable was
> >issued, if yes we then do vbus source disable, in dead battery case,
> >we never did vbus source enable, so will not issue vbus source disable
> >command.
> >
> 
> Thanks Peter, this sounds like the missing piece of information and I think
> some form of the code below will fix that.
> 
> There is still the issue that my board will need some way of controlling the
> initial state of vbus-sink.
> 
> @Guenter: would my initial patch be acceptable to set the default state of
> vbus-source and vbus-sink. Would you like some code to sanity check that
> both were not enabled at the same time ?
> 
Seems to me this is indeed a chip specific problem. I don't think a fix belongs
into the tcpm code. As mentioned before, the current status (ie drive_vbus)
should be readable from the chip. I am not sure though if we should add a
PTN5110 specific quirk to the driver to handle the situation. I am concerned
that fixing this as suggested below for PTN5110 may cause trouble with other
chips. Maybe not, but I'd rather be cautious.

Thanks,
Guenter

> >Acked-by: Peter Chen <peter.chen@nxp.com>
> >Signed-off-by: Li Jun <jun.li@nxp.com>
> >---
> > drivers/staging/typec/tcpci.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> >index 2d4fbb8aac5e..7352207224b5 100644
> >--- a/drivers/staging/typec/tcpci.c
> >+++ b/drivers/staging/typec/tcpci.c
> >@@ -381,9 +381,8 @@ 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 */
> >-
> >- if (!source) {
> >+ /* Only disable source if it was enabled */
> >+ if (!source && tcpci->drive_vbus) {
> >  ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >     TCPC_CMD_DISABLE_SRC_VBUS);
> >  if (ret < 0)
> 
> The version of struct tcpci doesn't have a drive_vbus. Where should
> drive_vbus get set and cleared ?
> 
> Is this a more complete version of what you intended ?
> 
> diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c
> index ac6b418b15f1..d6168163df7b 100644
> --- a/drivers/usb/typec/tcpci.c
> +++ b/drivers/usb/typec/tcpci.c
> @@ -28,6 +28,7 @@ struct tcpci {
>         struct regmap *regmap;
> 
>         bool controls_vbus;
> +       bool drive_vbus;
> 
>         struct tcpc_dev tcpc;
>         struct tcpci_data *data;
> @@ -277,7 +278,9 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool
> source, bool sink)
> 
>         /* Disable both source and sink first before enabling anything */
> 
> -       if (!source) {
> +       if (!source && tcpci->drive_vbus) {
> +               tcpci->drive_vbus = false;
> +
>                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
>                                    TCPC_CMD_DISABLE_SRC_VBUS);
>                 if (ret < 0)
> @@ -292,6 +295,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool
> source, bool sink)
>         }
> 
>         if (source) {
> +               tcpci->drive_vbus = true;
> +
>                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
>                                    TCPC_CMD_SRC_VBUS_DEFAULT);
>                 if (ret < 0)
> @@ -503,6 +508,7 @@ struct tcpci *tcpci_register_port(struct device *dev,
> struct tcpci_data *data)
>         tcpci->dev = dev;
>         tcpci->data = data;
>         tcpci->regmap = data->regmap;
> +       tcpci->drive_vbus = false;
> 
>         tcpci->tcpc.init = tcpci_init;
>         tcpci->tcpc.get_vbus = tcpci_get_vbus;
> 
> 
> 

RE: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Jun Li]

Hi
> -----Original Message-----
> From: Angus Ainslie <angus@akkea.ca>
> Sent: 2018年9月13日 19:10
> To: Peter Chen <hzpeterchen@gmail.com>
> Cc: linux@roeck-us.net; Heikki Krogerus <heikki.krogerus@linux.intel.com>; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; lkml
> <linux-kernel@vger.kernel.org>; Peter Chen <peter.chen@nxp.com>; Jun Li
> <jun.li@nxp.com>; Guenter Roeck <linux@roeck-us.net>
> Subject: Re: [PATCH v3] usb: typec: get the vbus source and charge values from the
> devicetree
> 
> On 2018-09-13 01:27, Peter Chen wrote:
> > On Thu, Sep 13, 2018 at 12:35 AM Guenter Roeck <linux@roeck-us.net>
> > wrote:
> >>
> >> On Wed, Sep 12, 2018 at 10:08:58AM -0600, Angus Ainslie wrote:
> >> > On 2018-09-11 09:33, Guenter Roeck wrote:
> >> > >I cant put my finger on it but this seems wrong. As i said both
> >> > >src and sink should never be true at the same time. I also din’t
> >> > >understand why turning off src should power off your board.
> >> > >Ultimately my concern is that we may be just painting over the
> >> > >real problem, and that would be really bad to do with dt properties.
> >> > >
> >> >
> >> > I agree that this doesn't seem like the correct way of solving the problem.
> >> > On this HW (Emcraft iMX8M BSB) I think the PTN5110 chip has been
> >> > connected correctly so I'm assuming that it is some quirk of the PTN5110.
> >> >
> >> > I didn't design the HW or the chip. This is a workaround for "quirky"
> >> > hardware and there may be others that don't behave exactly as expected.
> >> >
> >>
> >> I wouldn't be that sure about that. It may as well be that the tcpc
> >> driver and/or the tcpm driver are doing something wrong when
> >> initializing.
> >>
> >> I didn't really understand the logs you sent out earlier. It looked
> >> like the system would loose power if the TCPC_CMD_DISABLE_SRC_VBUS
> >> command is sent.  That doesn't really make sense to me since it
> >> indicates that the chip sources power to the remote, and turning that
> >> off should not result in a local loss of power.
> >>
> >> Note that the chip is supposed to be able to report if it is sourcing
> >> vbus and if VBUS is present, in the POWER_STATUS register. Another
> >> question is the content of the ROLE_CONTROL register when the system
> >> boots, and the DEVICE_CAPABILITIES settings.
> >>
> >> Overall I suspect that we don't handle startup for your system
> >> correctly in the tcpc driver. The ideal solution would be to find a
> >> solution which does not require any devicetree properties, but to do
> >> that we'll need to get a better understanding about your system's
> >> requirements.
> >>
> >> Guenter
> >
> > Hi Angus,
> >
> > Would you please check if below patch can fix your issue?
> >
> > staging: typec: don't do vbus source disable for dead battery
> >
> > In PTN5110 design, DisableSourceVBUS command also disables the sink
> > enable signal because the EN_SNK can be used to source higher voltage,
> > and, there is only one TCPC command to disable sourcing voltage
> > without telling whether to disable 5V or the high voltage, and to keep
> > the design simple they designed the PTN5110 to disable both. with this
> > fact, we use the flag drive_vbus to check if the source vbus enable
> > was issued, if yes we then do vbus source disable, in dead battery
> > case, we never did vbus source enable, so will not issue vbus source
> > disable command.
> >
> 
> Thanks Peter, this sounds like the missing piece of information and I think some form of
> the code below will fix that.
> 
> There is still the issue that my board will need some way of controlling the initial state of
> vbus-sink.
> 
> @Guenter: would my initial patch be acceptable to set the default state of vbus-source and
> vbus-sink. Would you like some code to sanity check that both were not enabled at the
> same time ?

I agree Guenter using a dt property is not the proper way here(maybe valid
for your HW with only typec power supply), I think a right way is to change
tcpm to handle this kind case(like the existing vbus_never_low flag) so we
will have a common handling

if (vbus_on && CC_state_is_RP)
	dead_battery = true;
	bypass tcpm_init_vbus or only enable sink.

RE: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Jun Li]

Hi
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: 2018年9月14日 1:35
> To: Angus Ainslie <angus@akkea.ca>
> Cc: Peter Chen <hzpeterchen@gmail.com>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; lkml
> <linux-kernel@vger.kernel.org>; Peter Chen <peter.chen@nxp.com>; Jun Li
> <jun.li@nxp.com>
> Subject: Re: [PATCH v3] usb: typec: get the vbus source and charge values from the
> devicetree
> 
> On Thu, Sep 13, 2018 at 05:10:09AM -0600, Angus Ainslie wrote:
> > >
> > >staging: typec: don't do vbus source disable for dead battery
> > >
> > >In PTN5110 design, DisableSourceVBUS command also disables the sink
> > >enable signal because the EN_SNK can be used to source higher
> > >voltage, and, there is only one TCPC command to disable sourcing
> > >voltage without telling whether to disable 5V or the high voltage,
> > >and to keep the design simple they designed the PTN5110 to disable
> > >both. with this fact, we use the flag drive_vbus to check if the
> > >source vbus enable was issued, if yes we then do vbus source disable,
> > >in dead battery case, we never did vbus source enable, so will not
> > >issue vbus source disable command.
> > >
> >
> > Thanks Peter, this sounds like the missing piece of information and I
> > think some form of the code below will fix that.
> >
> > There is still the issue that my board will need some way of
> > controlling the initial state of vbus-sink.
> >
> > @Guenter: would my initial patch be acceptable to set the default
> > state of vbus-source and vbus-sink. Would you like some code to sanity
> > check that both were not enabled at the same time ?
> >
> Seems to me this is indeed a chip specific problem. I don't think a fix belongs into the tcpm
> code. As mentioned before, the current status (ie drive_vbus) should be readable from the
> chip. I am not sure though if we should add a
> PTN5110 specific quirk to the driver to handle the situation. I am concerned that fixing this
> as suggested below for PTN5110 may cause trouble with other chips. Maybe not, but I'd
> rather be cautious.

Yes, this is a chip specific problem, the reason DisableSourceVBUS command also disables
the sink enable signal because the EN_SNK of PTN5110 can be used to source higher voltage
And, there is only one TCPC command to disable sourcing voltage without telling whether to
disable 5V or the high voltage, and to keep the design simple, TPN5110 is designed to disable
both.

The patch below is to not issue a vbus disable command if it was not enabled,
from my point view it should be OK and has the benefit to save one command.

Li Jun
> 
> Thanks,
> Guenter
> 
> > >Acked-by: Peter Chen <peter.chen@nxp.com>
> > >Signed-off-by: Li Jun <jun.li@nxp.com>
> > >---
> > > drivers/staging/typec/tcpci.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > >diff --git a/drivers/staging/typec/tcpci.c
> > >b/drivers/staging/typec/tcpci.c index 2d4fbb8aac5e..7352207224b5
> > >100644
> > >--- a/drivers/staging/typec/tcpci.c
> > >+++ b/drivers/staging/typec/tcpci.c
> > >@@ -381,9 +381,8 @@ 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 */
> > >-
> > >- if (!source) {
> > >+ /* Only disable source if it was enabled */ if (!source &&
> > >+ tcpci->drive_vbus) {
> > >  ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> > >     TCPC_CMD_DISABLE_SRC_VBUS);
> > >  if (ret < 0)
> >
> > The version of struct tcpci doesn't have a drive_vbus. Where should
> > drive_vbus get set and cleared ?
> >
> > Is this a more complete version of what you intended ?
> >
> > diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c
> > index ac6b418b15f1..d6168163df7b 100644
> > --- a/drivers/usb/typec/tcpci.c
> > +++ b/drivers/usb/typec/tcpci.c
> > @@ -28,6 +28,7 @@ struct tcpci {
> >         struct regmap *regmap;
> >
> >         bool controls_vbus;
> > +       bool drive_vbus;
> >
> >         struct tcpc_dev tcpc;
> >         struct tcpci_data *data;
> > @@ -277,7 +278,9 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc,
> > bool source, bool sink)
> >
> >         /* Disable both source and sink first before enabling anything
> > */
> >
> > -       if (!source) {
> > +       if (!source && tcpci->drive_vbus) {
> > +               tcpci->drive_vbus = false;
> > +
> >                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >                                    TCPC_CMD_DISABLE_SRC_VBUS);
> >                 if (ret < 0)
> > @@ -292,6 +295,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc,
> > bool source, bool sink)
> >         }
> >
> >         if (source) {
> > +               tcpci->drive_vbus = true;
> > +
> >                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >                                    TCPC_CMD_SRC_VBUS_DEFAULT);
> >                 if (ret < 0)
> > @@ -503,6 +508,7 @@ struct tcpci *tcpci_register_port(struct device
> > *dev, struct tcpci_data *data)
> >         tcpci->dev = dev;
> >         tcpci->data = data;
> >         tcpci->regmap = data->regmap;
> > +       tcpci->drive_vbus = false;
> >
> >         tcpci->tcpc.init = tcpci_init;
> >         tcpci->tcpc.get_vbus = tcpci_get_vbus;
> >
> >
> >