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;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 731D7609A1
Date:   Fri, 14 Sep 2018 11:42:42 -0700
From:   Jack Pham <jackp@codeaurora.org>
To:     Badhri Jagan Sridharan <badhri@google.com>
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>
Subject: Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at
 Vsafe5V
Message-ID: <20180914184242.GC13306@jackp-linux.qualcomm.com>
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>
 <CAPTae5KXAeCKerHh4PMFJ=7JhmwFwbc7-CQB-3JYamYbHtV+2A@mail.gmail.com>
 <20180913170746.GB13306@jackp-linux.qualcomm.com>
 <CAPTae5+WyxADQfDJivzmCBpFbA8RqpCGKDys6tUpHwF8+vt8iQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPTae5+WyxADQfDJivzmCBpFbA8RqpCGKDys6tUpHwF8+vt8iQ@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Badhri,

On Thu, Sep 13, 2018 at 04:38:10PM -0700, Badhri Jagan Sridharan wrote:
> On Thu, Sep 13, 2018 at 10:07 AM Jack Pham <jackp@codeaurora.org> wrote:
> > On Thu, Sep 13, 2018 at 6:43 AM Badhri Jagan Sridharan
> > > 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 ?
> >
> > Sure that makes sense. In this case I wonder if TCPM even needs to call
> > set_charge(false) considering it does not yet know if the partner is PD
> > capable or not. For sure, if the partner is PD capable and contract had
> > been previously established, we'd definitely need to set_current_limit()
> > to default levels and/or turn off charging.
> >
> > But in the case of hard reset attempts to try to determine if the source
> > will send its capabilities (thereby being PD capable), wouldn't the
> > initial default current limits still be in place? I think this is the
> > point you're trying to make, that there is no need to disrupt charging
> > if a hard reset is not going to cause VBUS to reset.
> 
> Yes that's right ! I wasnt sure how to put that in words. Thanks for
> doing that !
> You do concur right ?

Yes.

> > To me it sounds like what you're trying to do makes sense only if you
> > can make a run-time determination of a partner's PD capability, and not
> > based on a config option.
> 
> Yes this should be possible. port->pd_capable already has that info.
> 
> To sum it up:
> if the partner is pd capable, set charge to false in SNK_HARD_RESET_SINK_OFF and
> readjust current limits to default in SNK_HARD_RESET_SINK_ON and turn
> on charging.
> 
> If partner is not pd capable, do not turn off charging nor adjust
> current limit as host port is
> not going to respond to hard reset.
> 
> Does it make sense ?

Right, although the "not pd capable" path could also mean the partner is
PD capable but it is not determined yet. For example: if a sink is
connected to a PD source, established a contract, and the sink reboots
and has to initialize TCPM again. Assuming the sink never disconnected
from CC when rebooting, the source won't automatically re-send its
Source Capabilities as it will still be in its previous state(*). The
sink TCPM however would send a hard reset in order to try to
(re)establish a contract, so here is where this path overlaps with the
not-pd-capable case. In this case I think it might be proper to readjust
current limits to default also unless it was already done earlier--I see
it is done in tcpm_reset_port() and in SNK_DISCOVERY, so you might be
covered here.

The question I have is whether you still need to consider calling
set_charge(false) for this example.

(*) Just thought of one more thing, what if the previous contract was
negotiated at greater than 5V VBUS? How does the sink TCPM handle
setting charging parameters after a reboot but prior to hard reset?

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project