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

[Angus Ainslie <angus@akkea.ca>]

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;