[PATCH 1/2] typec: tcpm: Do not disconnect link for self powered devices

[PATCH 1/2] typec: tcpm: Do not disconnect link for self powered devices

[Badhri Jagan Sridharan]

During HARD_RESET the data link is disconnected.
For self powered device, the spec is advising against doing that.

From USB_PD_R3_0
7.1.5 Response to Hard Resets
Device operation during and after a Hard Reset is defined as follows:
Self-powered devices Should Not disconnect from USB during a Hard Reset
(see Section 9.1.2).
Bus powered devices will disconnect from USB during a Hard Reset due to the
loss of their power source.

Tackle this by letting TCPM know whether the device is self or bus powered.

This overcomes unnecessary port disconnections from hard reset.
Also, speeds up the enumeration time when connected to Type-A ports.

Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
---
 drivers/usb/typec/tcpm.c | 6 +++++-
 include/linux/usb/tcpm.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 4f1f4215f3d6..a4e0c027a2a9 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port)
 		memset(&port->pps_data, 0, sizeof(port->pps_data));
 		tcpm_set_vconn(port, false);
 		tcpm_set_charge(port, false);
-		tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
+
+		if (port->tcpc->config->self_powered)
+			tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
+		else
+			tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
 		/*
 		 * VBUS may or may not toggle, depending on the adapter.
 		 * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 7e7fbfb84e8e..50c74a77db55 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -89,6 +89,7 @@ struct tcpc_config {
 	enum typec_port_data data;
 	enum typec_role default_role;
 	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
+	bool self_powered;	/* port belongs to a self powered device */
 
 	const struct typec_altmode_desc *alt_modes;
 };
-- 
2.19.0.397.gdd90340f6a-goog

[1/2] typec: tcpm: Do not disconnect link for self powered devices

[Badhri Jagan Sridharan]

During HARD_RESET the data link is disconnected.
For self powered device, the spec is advising against doing that.

From USB_PD_R3_0
7.1.5 Response to Hard Resets
Device operation during and after a Hard Reset is defined as follows:
Self-powered devices Should Not disconnect from USB during a Hard Reset
(see Section 9.1.2).
Bus powered devices will disconnect from USB during a Hard Reset due to the
loss of their power source.

Tackle this by letting TCPM know whether the device is self or bus powered.

This overcomes unnecessary port disconnections from hard reset.
Also, speeds up the enumeration time when connected to Type-A ports.

Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
---
 drivers/usb/typec/tcpm.c | 6 +++++-
 include/linux/usb/tcpm.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 4f1f4215f3d6..a4e0c027a2a9 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port)
 		memset(&port->pps_data, 0, sizeof(port->pps_data));
 		tcpm_set_vconn(port, false);
 		tcpm_set_charge(port, false);
-		tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
+
+		if (port->tcpc->config->self_powered)
+			tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
+		else
+			tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
 		/*
 		 * VBUS may or may not toggle, depending on the adapter.
 		 * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 7e7fbfb84e8e..50c74a77db55 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -89,6 +89,7 @@ struct tcpc_config {
 	enum typec_port_data data;
 	enum typec_role default_role;
 	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
+	bool self_powered;	/* port belongs to a self powered device */
 
 	const struct typec_altmode_desc *alt_modes;
 };

[PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

During hard reset, TCPM turns off the charging path.
The spec provides an option for Sink to either drop to vSafe5V or vSafe0V.

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.

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)
+			tcpm_set_current_limit(port,
+					       tcpm_get_current_limit(port),
+					       5000);
+		else
+			tcpm_set_charge(port, false);
 
 		if (port->tcpc->config->self_powered)
 			tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 50c74a77db55..c6e3ccecba70 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -90,6 +90,7 @@ struct tcpc_config {
 	enum typec_role default_role;
 	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
 	bool self_powered;	/* port belongs to a self powered device */
+	bool vsafe_5v_hard_reset;	/* Maintain VSAFE5V during hard reset */
 
 	const struct typec_altmode_desc *alt_modes;
 };
-- 
2.19.0.397.gdd90340f6a-goog

[2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

During hard reset, TCPM turns off the charging path.
The spec provides an option for Sink to either drop to vSafe5V or vSafe0V.

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.

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)
+			tcpm_set_current_limit(port,
+					       tcpm_get_current_limit(port),
+					       5000);
+		else
+			tcpm_set_charge(port, false);
 
 		if (port->tcpc->config->self_powered)
 			tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 50c74a77db55..c6e3ccecba70 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -90,6 +90,7 @@ struct tcpc_config {
 	enum typec_role default_role;
 	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
 	bool self_powered;	/* port belongs to a self powered device */
+	bool vsafe_5v_hard_reset;	/* Maintain VSAFE5V during hard reset */
 
 	const struct typec_altmode_desc *alt_modes;
 };

Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Jack Pham]

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?

> 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.

> 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?

> +		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

[2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Jack Pham]

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?

> 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.

> 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?

> +		else
> +			tcpm_set_charge(port, false);

Regards,
Jack

Re: [PATCH 1/2] typec: tcpm: Do not disconnect link for self powered devices

[Heikki Krogerus]

+Guenter

On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote:
> During HARD_RESET the data link is disconnected.
> For self powered device, the spec is advising against doing that.
> 
> >From USB_PD_R3_0
> 7.1.5 Response to Hard Resets
> Device operation during and after a Hard Reset is defined as follows:
> Self-powered devices Should Not disconnect from USB during a Hard Reset
> (see Section 9.1.2).
> Bus powered devices will disconnect from USB during a Hard Reset due to the
> loss of their power source.
> 
> Tackle this by letting TCPM know whether the device is self or bus powered.
> 
> This overcomes unnecessary port disconnections from hard reset.
> Also, speeds up the enumeration time when connected to Type-A ports.
> 
> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> ---
>  drivers/usb/typec/tcpm.c | 6 +++++-
>  include/linux/usb/tcpm.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 4f1f4215f3d6..a4e0c027a2a9 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port)
>  		memset(&port->pps_data, 0, sizeof(port->pps_data));
>  		tcpm_set_vconn(port, false);
>  		tcpm_set_charge(port, false);
> -		tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> +
> +		if (port->tcpc->config->self_powered)

Add a member for that flag to the struct tcpm_port, and check that
here instead. I'll explain why below.

> +			tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
> +		else
> +			tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> 
>  		/*
>  		 * VBUS may or may not toggle, depending on the adapter.
>  		 * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 7e7fbfb84e8e..50c74a77db55 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -89,6 +89,7 @@ struct tcpc_config {
>  	enum typec_port_data data;
>  	enum typec_role default_role;
>  	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
> +	bool self_powered;	/* port belongs to a self powered device */

I'm not sure we should add any more members to that structure, but
maybe it's not a problem for now. We can't quite yet get rid of that
structure.

You do need to introduce a new device property already. Then read the
value for that new member you added to struct tcpm_port in
tcpm_fw_get_caps() and in tcpm_copy_caps().


Thanks,

-- 
heikki

[1/2] typec: tcpm: Do not disconnect link for self powered devices

[Heikki Krogerus]

+Guenter

On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote:
> During HARD_RESET the data link is disconnected.
> For self powered device, the spec is advising against doing that.
> 
> >From USB_PD_R3_0
> 7.1.5 Response to Hard Resets
> Device operation during and after a Hard Reset is defined as follows:
> Self-powered devices Should Not disconnect from USB during a Hard Reset
> (see Section 9.1.2).
> Bus powered devices will disconnect from USB during a Hard Reset due to the
> loss of their power source.
> 
> Tackle this by letting TCPM know whether the device is self or bus powered.
> 
> This overcomes unnecessary port disconnections from hard reset.
> Also, speeds up the enumeration time when connected to Type-A ports.
> 
> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> ---
>  drivers/usb/typec/tcpm.c | 6 +++++-
>  include/linux/usb/tcpm.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 4f1f4215f3d6..a4e0c027a2a9 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port)
>  		memset(&port->pps_data, 0, sizeof(port->pps_data));
>  		tcpm_set_vconn(port, false);
>  		tcpm_set_charge(port, false);
> -		tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> +
> +		if (port->tcpc->config->self_powered)

Add a member for that flag to the struct tcpm_port, and check that
here instead. I'll explain why below.

> +			tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
> +		else
> +			tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> 
>  		/*
>  		 * VBUS may or may not toggle, depending on the adapter.
>  		 * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 7e7fbfb84e8e..50c74a77db55 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -89,6 +89,7 @@ struct tcpc_config {
>  	enum typec_port_data data;
>  	enum typec_role default_role;
>  	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
> +	bool self_powered;	/* port belongs to a self powered device */

I'm not sure we should add any more members to that structure, but
maybe it's not a problem for now. We can't quite yet get rid of that
structure.

You do need to introduce a new device property already. Then read the
value for that new member you added to struct tcpm_port in
tcpm_fw_get_caps() and in tcpm_copy_caps().


Thanks,

Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

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

[2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

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

Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

+ 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

[2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

+ 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

Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Jack Pham]

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.

Ok, you are talking about sink current draw limits but vSafe{0,5}V are
voltage definitions so these are orthogonal. Again, the sink can't
directly dictate the voltage that's being sourced so I don't see how it
has a choice here. If a PD contract was negotiated for greater than 5V
and a hard reset happens then yes the voltage would fall to 0V and then
rise back to 5V and during this time sink needs to draw appropriate
current.

> > > 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 ?

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.

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.

Thanks,
Jack

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

[2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Jack Pham]

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.

Ok, you are talking about sink current draw limits but vSafe{0,5}V are
voltage definitions so these are orthogonal. Again, the sink can't
directly dictate the voltage that's being sourced so I don't see how it
has a choice here. If a PD contract was negotiated for greater than 5V
and a hard reset happens then yes the voltage would fall to 0V and then
rise back to 5V and during this time sink needs to draw appropriate
current.

> > > 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 ?

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.

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.

Thanks,
Jack

Re: [PATCH 1/2] typec: tcpm: Do not disconnect link for self powered devices

[Guenter Roeck]

On Thu, Sep 13, 2018 at 04:24:08PM +0300, Heikki Krogerus wrote:
> +Guenter
> 
> On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote:
> > During HARD_RESET the data link is disconnected.
> > For self powered device, the spec is advising against doing that.
> > 
> > >From USB_PD_R3_0
> > 7.1.5 Response to Hard Resets
> > Device operation during and after a Hard Reset is defined as follows:
> > Self-powered devices Should Not disconnect from USB during a Hard Reset
> > (see Section 9.1.2).
> > Bus powered devices will disconnect from USB during a Hard Reset due to the
> > loss of their power source.
> > 
> > Tackle this by letting TCPM know whether the device is self or bus powered.
> > 
> > This overcomes unnecessary port disconnections from hard reset.
> > Also, speeds up the enumeration time when connected to Type-A ports.
> > 
> > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm.c | 6 +++++-
> >  include/linux/usb/tcpm.h | 1 +
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > index 4f1f4215f3d6..a4e0c027a2a9 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port)
> >  		memset(&port->pps_data, 0, sizeof(port->pps_data));
> >  		tcpm_set_vconn(port, false);
> >  		tcpm_set_charge(port, false);
> > -		tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> > +
> > +		if (port->tcpc->config->self_powered)
> 
> Add a member for that flag to the struct tcpm_port, and check that
> here instead. I'll explain why below.
> 
> > +			tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
> > +		else
> > +			tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> > 

Maybe I am missing something, but
		tcpm_set_roles(port, port->self_powered,
			       TYPEC_SINK, TYPEC_DEVICE);
should accomplish the same (assuming the flag is moved to tcpm_port).

Does the code at SRC_HARD_RESET_VBUS_OFF: also have to be changed,
or only SNK_HARD_RESET_SINK_OFF: ? In other words, does this only
apply if the port is a sink, or also if is is source ?

Guenter

> >  		/*
> >  		 * VBUS may or may not toggle, depending on the adapter.
> >  		 * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > index 7e7fbfb84e8e..50c74a77db55 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -89,6 +89,7 @@ struct tcpc_config {
> >  	enum typec_port_data data;
> >  	enum typec_role default_role;
> >  	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
> > +	bool self_powered;	/* port belongs to a self powered device */
> 
> I'm not sure we should add any more members to that structure, but
> maybe it's not a problem for now. We can't quite yet get rid of that
> structure.
> 
> You do need to introduce a new device property already. Then read the
> value for that new member you added to struct tcpm_port in
> tcpm_fw_get_caps() and in tcpm_copy_caps().
> 
> 
> Thanks,
> 
> -- 
> heikki

[1/2] typec: tcpm: Do not disconnect link for self powered devices

[Guenter Roeck]

On Thu, Sep 13, 2018 at 04:24:08PM +0300, Heikki Krogerus wrote:
> +Guenter
> 
> On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote:
> > During HARD_RESET the data link is disconnected.
> > For self powered device, the spec is advising against doing that.
> > 
> > >From USB_PD_R3_0
> > 7.1.5 Response to Hard Resets
> > Device operation during and after a Hard Reset is defined as follows:
> > Self-powered devices Should Not disconnect from USB during a Hard Reset
> > (see Section 9.1.2).
> > Bus powered devices will disconnect from USB during a Hard Reset due to the
> > loss of their power source.
> > 
> > Tackle this by letting TCPM know whether the device is self or bus powered.
> > 
> > This overcomes unnecessary port disconnections from hard reset.
> > Also, speeds up the enumeration time when connected to Type-A ports.
> > 
> > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm.c | 6 +++++-
> >  include/linux/usb/tcpm.h | 1 +
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > index 4f1f4215f3d6..a4e0c027a2a9 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port)
> >  		memset(&port->pps_data, 0, sizeof(port->pps_data));
> >  		tcpm_set_vconn(port, false);
> >  		tcpm_set_charge(port, false);
> > -		tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> > +
> > +		if (port->tcpc->config->self_powered)
> 
> Add a member for that flag to the struct tcpm_port, and check that
> here instead. I'll explain why below.
> 
> > +			tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
> > +		else
> > +			tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> > 

Maybe I am missing something, but
		tcpm_set_roles(port, port->self_powered,
			       TYPEC_SINK, TYPEC_DEVICE);
should accomplish the same (assuming the flag is moved to tcpm_port).

Does the code at SRC_HARD_RESET_VBUS_OFF: also have to be changed,
or only SNK_HARD_RESET_SINK_OFF: ? In other words, does this only
apply if the port is a sink, or also if is is source ?

Guenter

> >  		/*
> >  		 * VBUS may or may not toggle, depending on the adapter.
> >  		 * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > index 7e7fbfb84e8e..50c74a77db55 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -89,6 +89,7 @@ struct tcpc_config {
> >  	enum typec_port_data data;
> >  	enum typec_role default_role;
> >  	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
> > +	bool self_powered;	/* port belongs to a self powered device */
> 
> I'm not sure we should add any more members to that structure, but
> maybe it's not a problem for now. We can't quite yet get rid of that
> structure.
> 
> You do need to introduce a new device property already. Then read the
> value for that new member you added to struct tcpm_port in
> tcpm_fw_get_caps() and in tcpm_copy_caps().
> 
> 
> Thanks,
> 
> -- 
> heikki

Re: [PATCH 1/2] typec: tcpm: Do not disconnect link for self powered devices

[Badhri Jagan Sridharan]

On Thu, Sep 13, 2018 at 10:51 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Sep 13, 2018 at 04:24:08PM +0300, Heikki Krogerus wrote:
> > +Guenter
> >
> > On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote:
> > > During HARD_RESET the data link is disconnected.
> > > For self powered device, the spec is advising against doing that.
> > >
> > > >From USB_PD_R3_0
> > > 7.1.5 Response to Hard Resets
> > > Device operation during and after a Hard Reset is defined as follows:
> > > Self-powered devices Should Not disconnect from USB during a Hard Reset
> > > (see Section 9.1.2).
> > > Bus powered devices will disconnect from USB during a Hard Reset due to the
> > > loss of their power source.
> > >
> > > Tackle this by letting TCPM know whether the device is self or bus powered.
> > >
> > > This overcomes unnecessary port disconnections from hard reset.
> > > Also, speeds up the enumeration time when connected to Type-A ports.
> > >
> > > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> > > ---
> > >  drivers/usb/typec/tcpm.c | 6 +++++-
> > >  include/linux/usb/tcpm.h | 1 +
> > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > > index 4f1f4215f3d6..a4e0c027a2a9 100644
> > > --- a/drivers/usb/typec/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm.c
> > > @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port)
> > >             memset(&port->pps_data, 0, sizeof(port->pps_data));
> > >             tcpm_set_vconn(port, false);
> > >             tcpm_set_charge(port, false);
> > > -           tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> > > +
> > > +           if (port->tcpc->config->self_powered)
> >
> > Add a member for that flag to the struct tcpm_port, and check that
> > here instead. I'll explain why below.
> >
> > > +                   tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
> > > +           else
> > > +                   tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> > >
>
> Maybe I am missing something, but
>                 tcpm_set_roles(port, port->self_powered,
>                                TYPEC_SINK, TYPEC_DEVICE);
> should accomplish the same (assuming the flag is moved to tcpm_port).

Yeah Makes sense to me..
>
> Does the code at SRC_HARD_RESET_VBUS_OFF: also have to be changed,
> or only SNK_HARD_RESET_SINK_OFF: ? In other words, does this only
> apply if the port is a sink, or also if is is source ?

Good question ! I dont see any direct guidance on the spec.
All that I can see is the following:

From 2.6.1 Source Operation:
Restores the Port’s data role to DFP.

From 7.1.5 Response to Hard Resets:
"The Source Shall meet both tSafe5V and tSafe0V relative to the start
of the voltage transition as shown in Figure 7-8."

I am leaning towards to changing the link state from false to true for
SRC_HARD_RESET_VBUS_OFF
as well. This is to keep it symmetrical. What do you think ?

>
> Guenter
>
> > >             /*
> > >              * VBUS may or may not toggle, depending on the adapter.
> > >              * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
> > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > > index 7e7fbfb84e8e..50c74a77db55 100644
> > > --- a/include/linux/usb/tcpm.h
> > > +++ b/include/linux/usb/tcpm.h
> > > @@ -89,6 +89,7 @@ struct tcpc_config {
> > >     enum typec_port_data data;
> > >     enum typec_role default_role;
> > >     bool try_role_hw;       /* try.{src,snk} implemented in hardware */
> > > +   bool self_powered;      /* port belongs to a self powered device */
> >
> > I'm not sure we should add any more members to that structure, but
> > maybe it's not a problem for now. We can't quite yet get rid of that
> > structure.
> >
> > You do need to introduce a new device property already. Then read the

This would be part of usb-connector binding. right ?

> > value for that new member you added to struct tcpm_port in
> > tcpm_fw_get_caps() and in tcpm_copy_caps().
> >
> >
> > Thanks,
> >
> > --
> > heikki

[1/2] typec: tcpm: Do not disconnect link for self powered devices

[Badhri Jagan Sridharan]

On Thu, Sep 13, 2018 at 10:51 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Sep 13, 2018 at 04:24:08PM +0300, Heikki Krogerus wrote:
> > +Guenter
> >
> > On Wed, Sep 12, 2018 at 07:11:12PM -0700, Badhri Jagan Sridharan wrote:
> > > During HARD_RESET the data link is disconnected.
> > > For self powered device, the spec is advising against doing that.
> > >
> > > >From USB_PD_R3_0
> > > 7.1.5 Response to Hard Resets
> > > Device operation during and after a Hard Reset is defined as follows:
> > > Self-powered devices Should Not disconnect from USB during a Hard Reset
> > > (see Section 9.1.2).
> > > Bus powered devices will disconnect from USB during a Hard Reset due to the
> > > loss of their power source.
> > >
> > > Tackle this by letting TCPM know whether the device is self or bus powered.
> > >
> > > This overcomes unnecessary port disconnections from hard reset.
> > > Also, speeds up the enumeration time when connected to Type-A ports.
> > >
> > > Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> > > ---
> > >  drivers/usb/typec/tcpm.c | 6 +++++-
> > >  include/linux/usb/tcpm.h | 1 +
> > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > > index 4f1f4215f3d6..a4e0c027a2a9 100644
> > > --- a/drivers/usb/typec/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm.c
> > > @@ -3270,7 +3270,11 @@ static void run_state_machine(struct tcpm_port *port)
> > >             memset(&port->pps_data, 0, sizeof(port->pps_data));
> > >             tcpm_set_vconn(port, false);
> > >             tcpm_set_charge(port, false);
> > > -           tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> > > +
> > > +           if (port->tcpc->config->self_powered)
> >
> > Add a member for that flag to the struct tcpm_port, and check that
> > here instead. I'll explain why below.
> >
> > > +                   tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
> > > +           else
> > > +                   tcpm_set_roles(port, false, TYPEC_SINK, TYPEC_DEVICE);
> > >
>
> Maybe I am missing something, but
>                 tcpm_set_roles(port, port->self_powered,
>                                TYPEC_SINK, TYPEC_DEVICE);
> should accomplish the same (assuming the flag is moved to tcpm_port).

Yeah Makes sense to me..
>
> Does the code at SRC_HARD_RESET_VBUS_OFF: also have to be changed,
> or only SNK_HARD_RESET_SINK_OFF: ? In other words, does this only
> apply if the port is a sink, or also if is is source ?

Good question ! I dont see any direct guidance on the spec.
All that I can see is the following:

From 2.6.1 Source Operation:
Restores the Port’s data role to DFP.

From 7.1.5 Response to Hard Resets:
"The Source Shall meet both tSafe5V and tSafe0V relative to the start
of the voltage transition as shown in Figure 7-8."

I am leaning towards to changing the link state from false to true for
SRC_HARD_RESET_VBUS_OFF
as well. This is to keep it symmetrical. What do you think ?

>
> Guenter
>
> > >             /*
> > >              * VBUS may or may not toggle, depending on the adapter.
> > >              * If it doesn't toggle, transition to SNK_HARD_RESET_SINK_ON
> > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > > index 7e7fbfb84e8e..50c74a77db55 100644
> > > --- a/include/linux/usb/tcpm.h
> > > +++ b/include/linux/usb/tcpm.h
> > > @@ -89,6 +89,7 @@ struct tcpc_config {
> > >     enum typec_port_data data;
> > >     enum typec_role default_role;
> > >     bool try_role_hw;       /* try.{src,snk} implemented in hardware */
> > > +   bool self_powered;      /* port belongs to a self powered device */
> >
> > I'm not sure we should add any more members to that structure, but
> > maybe it's not a problem for now. We can't quite yet get rid of that
> > structure.
> >
> > You do need to introduce a new device property already. Then read the

This would be part of usb-connector binding. right ?

> > value for that new member you added to struct tcpm_port in
> > tcpm_fw_get_caps() and in tcpm_copy_caps().
> >
> >
> > Thanks,
> >
> > --
> > heikki

Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

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
> <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.
>
> Ok, you are talking about sink current draw limits but vSafe{0,5}V are
> voltage definitions so these are orthogonal. Again, the sink can't
> directly dictate the voltage that's being sourced so I don't see how it
> has a choice here. If a PD contract was negotiated for greater than 5V
> and a hard reset happens then yes the voltage would fall to 0V and then
> rise back to 5V and during this time sink needs to draw appropriate
> current.

>
> > > > 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 ?
>
> 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 ?

>
> 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 ?

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

[2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

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
> <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.
>
> Ok, you are talking about sink current draw limits but vSafe{0,5}V are
> voltage definitions so these are orthogonal. Again, the sink can't
> directly dictate the voltage that's being sourced so I don't see how it
> has a choice here. If a PD contract was negotiated for greater than 5V
> and a hard reset happens then yes the voltage would fall to 0V and then
> rise back to 5V and during this time sink needs to draw appropriate
> current.

>
> > > > 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 ?
>
> 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 ?

>
> 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 ?

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

Re: [PATCH 1/2] typec: tcpm: Do not disconnect link for self powered devices

[Heikki Krogerus]

Hi,

On Thu, Sep 13, 2018 at 04:16:08PM -0700, Badhri Jagan Sridharan wrote:
> > > You do need to introduce a new device property already. Then read the
> 
> This would be part of usb-connector binding. right ?

Yes, that is probable the correct place for that. It's not tcpm/tcpci
specific.

But I don't think it's Type-C specific either. Perhaps it should
go under "Optional Properties" instead of "Optional properties for
usb-c-connector"?


Thanks,

-- 
heikki

[1/2] typec: tcpm: Do not disconnect link for self powered devices

[Heikki Krogerus]

Hi,

On Thu, Sep 13, 2018 at 04:16:08PM -0700, Badhri Jagan Sridharan wrote:
> > > You do need to introduce a new device property already. Then read the
> 
> This would be part of usb-connector binding. right ?

Yes, that is probable the correct place for that. It's not tcpm/tcpci
specific.

But I don't think it's Type-C specific either. Perhaps it should
go under "Optional Properties" instead of "Optional properties for
usb-c-connector"?


Thanks,

Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Jack Pham]

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

[2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Jack Pham]

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

Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

On Fri, Sep 14, 2018 at 11:42 AM Jack Pham <jackp@codeaurora.org> wrote:
>
> 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.

Yes I verified this.

>
> 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?


Rebooting the device is an interesting case ! TCPM is unique in its own
way as it neither a platform_driver nor a device_driver. Simply its the state
machine of the type-c port's device. I see that I could register to the
shutdown callback in the lower level driver which registers the port
by calling tcpm_register_port and handle the shutdown operation by calling
tcpm_unregister_port and subsequently opening the ports but dont know
how to achiever this at the TCPM level.

However, this reboot case is not directly related to the HARD_RESET
state as this would have to be handled for all the sub states. Isn't that the
case ?

Updating the patchset to do the following:
"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."

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

[2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

[Badhri Jagan Sridharan]

On Fri, Sep 14, 2018 at 11:42 AM Jack Pham <jackp@codeaurora.org> wrote:
>
> 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.

Yes I verified this.

>
> 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?


Rebooting the device is an interesting case ! TCPM is unique in its own
way as it neither a platform_driver nor a device_driver. Simply its the state
machine of the type-c port's device. I see that I could register to the
shutdown callback in the lower level driver which registers the port
by calling tcpm_register_port and handle the shutdown operation by calling
tcpm_unregister_port and subsequently opening the ports but dont know
how to achiever this at the TCPM level.

However, this reboot case is not directly related to the HARD_RESET
state as this would have to be handled for all the sub states. Isn't that the
case ?

Updating the patchset to do the following:
"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."

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