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

From: Angus Ainslie
Date: Thu Sep 13 2018 - 07:10:12 EST

On 2018-09-13 01:27, Peter Chen wrote:
On Thu, Sep 13, 2018 at 12:35 AM Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxx>
Signed-off-by: Li Jun <jun.li@xxxxxxx>
---
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;