Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180913021113.18150-1-badhri@google.com> <20180913021113.18150-2-badhri@google.com>
 <20180913063943.GA13306@jackp-linux.qualcomm.com>
In-Reply-To: <20180913063943.GA13306@jackp-linux.qualcomm.com>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Thu, 13 Sep 2018 06:43:09 -0700
Message-ID: <CAPTae5JLnbHnFUXv4W0GY6a=kAmGCx7XfOUKrWo0q__enbvvQA@mail.gmail.com>
Subject: Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V
To:     jackp@codeaurora.org
Cc:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        USB <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Sep 12, 2018 at 11:39 PM Jack Pham <jackp@codeaurora.org> wrote:
>
> Hello Badhri,
>
> On Wed, Sep 12, 2018 at 07:11:13PM -0700, Badhri Jagan Sridharan wrote:
> > During hard reset, TCPM turns off the charging path.
> > The spec provides an option for Sink to either drop to vSafe5V or vSafe0V.
>
> This doesn't make sense. By definition the sink isn't sourcing VBUS, so
> how can it control whether to allow the voltage to be 5V or 0V?

The way I understand it, this is for the current limits that can be
set on the Sink side.
During hard reset, sink has to fallback to VSafe5V or VSafe0V if
higher pd contract was negotiated.

>
>
> > From USB_PD_R3_0
> > 2.6.2 Sink Operation
> > ..
> > Serious errors are handled by Hard Reset Signaling issued by either Port
> > Partner. A Hard Reset:
> > resets protocol as for a Soft Reset but also returns the power supply to
> > USB Default Operation (vSafe0V or vSafe5V output) in order to protect the
> > Sink.
>
> I can see how the wording here "vSafe0V *or* vSafe5V" is misleading, as
> I think it actually is both. In later parts of the spec, the source's
> VBUS behavior is well defined in that it must first drop to vSafe0V
> and then return to vSafe5V. Please refer to section 7.1.5.


Yeah thats for the source. But for sink, Say if the source isnt PD, then,
sink initiated hard resets happen during the connection. Sink would hard reset
couple of times before deeming that the partner is non PD. When connected
to Type-A ports/non-pd partner, vbus is not going to likely drop so there isnt
a reason to setcharge to false or drop the input current limit. Do you agree ?

>
>
> > Add a config option to tcpc_dev and let the device specific driver decide
> > what needs to be done.
> >
> > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm.c | 7 ++++++-
> >  include/linux/usb/tcpm.h | 1 +
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > index a4e0c027a2a9..350d1a7c4543 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -3269,7 +3269,12 @@ static void run_state_machine(struct tcpm_port *port)
> >       case SNK_HARD_RESET_SINK_OFF:
> >               memset(&port->pps_data, 0, sizeof(port->pps_data));
> >               tcpm_set_vconn(port, false);
> > -             tcpm_set_charge(port, false);
> > +             if (port->tcpc->config->vsafe_5v_hard_reset)
>
> Therefore I think this config option doesn't make sense.
>
> > +                     tcpm_set_current_limit(port,
> > +                                            tcpm_get_current_limit(port),
> > +                                            5000);
>
> But I do think this might still be useful at least in ensuring the sink
> returns to drawing only default power during the transition before
> re-establishing a contract. Given that the sink can't control when
> exactly VBUS will go to 0V, is it ok to call set_current_limit() even if
> VBUS is momentarily 0V, so at least it is in preparation for when VBUS
> turns back on? Or would it be safer to do it during the
> SNK_HARD_RESET_SINK_ON state after we know VBUS is back to vSafe5V?

IMHO Doing it in SNK_HARD_RESET_SINK_ON makes more sense when
vsafe_5v_hard_reset
is not set.

>
>
> > +             else
> > +                     tcpm_set_charge(port, false);
>
> Regards,
> Jack
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project