| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 10 Sep 2018 08:32:48 -0600
From: Angus Ainslie <angus@...ea.ca>
To: Guenter Roeck <guenter@...ck-us.net>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
groeck7@...il.com, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] usb: typec: get the vbus source and charge values from the devicetree
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@...ea.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,
>>
>>
Powered by blists - more mailing lists