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> <CAPTae5JLnbHnFUXv4W0GY6a=kAmGCx7XfOUKrWo0q__enbvvQA@mail.gmail.com>
In-Reply-To: <CAPTae5JLnbHnFUXv4W0GY6a=kAmGCx7XfOUKrWo0q__enbvvQA@mail.gmail.com>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Thu, 13 Sep 2018 06:45:41 -0700
Message-ID: <CAPTae5KXAeCKerHh4PMFJ=7JhmwFwbc7-CQB-3JYamYbHtV+2A@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>,
        Guenter Roeck <linux@roeck-us.net>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+ Guenter
On Thu, Sep 13, 2018 at 6:43 AM Badhri Jagan Sridharan
<badhri@google.com> wrote:
>
> 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