| 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
| ||
|
Message-ID: <aa08abf87b945dc5c8268fab3cb55cbb@www.akkea.ca>
Date: Thu, 13 Sep 2018 05:10:09 -0600
From: Angus Ainslie <angus@...ea.ca>
To: Peter Chen <hzpeterchen@...il.com>
Cc: linux@...ck-us.net,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-usb@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
peter.chen@....com, jun.li@....com,
Guenter Roeck <linux@...ck-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@...ck-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@....com>
> Signed-off-by: Li Jun <jun.li@....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;
Powered by blists - more mailing lists