Re: [PATCH v4 4/7] typec: tcpm: Add core support for sink side PPS

[Heikki Krogerus]

On Tue, Feb 06, 2018 at 02:33:08PM +, Adam Thomson wrote:
> On 30 January 2018 12:47, Heikki Krogerus wrote:
> 
> > > +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
> > > +{
> > > + unsigned int target_mw;
> > > + int ret = 0;
> > > +
> > > + mutex_lock(>swap_lock);
> > > + mutex_lock(>lock);
> > > +
> > > + if (!port->pps_data.active) {
> > > + ret = -EOPNOTSUPP;
> > > + goto port_unlock;
> > > + }
> > > +
> > > + if (port->state != SNK_READY) {
> > > + ret = -EAGAIN;
> > > + goto port_unlock;
> > > + }
> > > +
> > > + if (op_curr > port->pps_data.max_curr) {
> > > + ret = -EINVAL;
> > > + goto port_unlock;
> > > + }
> > > +
> > > + target_mw = (op_curr * port->pps_data.out_volt) / 1000;
> > > + if (target_mw < port->operating_snk_mw) {
> > > + ret = -EINVAL;
> > > + goto port_unlock;
> > > + }
> > > +
> > > + reinit_completion(>pps_complete);
> > > + port->pps_data.op_curr = op_curr;
> > > + port->pps_status = 0;
> > > + port->pps_pending = true;
> > > + tcpm_set_state(port, SNK_NEGOTIATE_PPS_CAPABILITIES, 0);
> >
> > Why not just take the swap_lock here..
> 
> I believe this would result in deadlock. All of the existing uses of swap_lock
> acquire it first before the port->lock is then acquired (and vice-versa for
> unlock). We don't want the power role to change during this procedure, so we
> hold the swap_lock for the whole process. Have a look at tcpm_dr_set() and
> tcpm_pr_set() as examples of existing usage.

OK. Then I'm fine with this patch as well. FWIW:

Acked-by: Heikki Krogerus 

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 4/7] typec: tcpm: Add core support for sink side PPS

[Adam Thomson]

On 30 January 2018 12:47, Heikki Krogerus wrote:

> > +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
> > +{
> > +   unsigned int target_mw;
> > +   int ret = 0;
> > +
> > +   mutex_lock(>swap_lock);
> > +   mutex_lock(>lock);
> > +
> > +   if (!port->pps_data.active) {
> > +   ret = -EOPNOTSUPP;
> > +   goto port_unlock;
> > +   }
> > +
> > +   if (port->state != SNK_READY) {
> > +   ret = -EAGAIN;
> > +   goto port_unlock;
> > +   }
> > +
> > +   if (op_curr > port->pps_data.max_curr) {
> > +   ret = -EINVAL;
> > +   goto port_unlock;
> > +   }
> > +
> > +   target_mw = (op_curr * port->pps_data.out_volt) / 1000;
> > +   if (target_mw < port->operating_snk_mw) {
> > +   ret = -EINVAL;
> > +   goto port_unlock;
> > +   }
> > +
> > +   reinit_completion(>pps_complete);
> > +   port->pps_data.op_curr = op_curr;
> > +   port->pps_status = 0;
> > +   port->pps_pending = true;
> > +   tcpm_set_state(port, SNK_NEGOTIATE_PPS_CAPABILITIES, 0);
>
> Why not just take the swap_lock here..

I believe this would result in deadlock. All of the existing uses of swap_lock
acquire it first before the port->lock is then acquired (and vice-versa for
unlock). We don't want the power role to change during this procedure, so we
hold the swap_lock for the whole process. Have a look at tcpm_dr_set() and
tcpm_pr_set() as examples of existing usage.

> > +   return ret;
> > +}
> > +
> > +static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
> > +{
> > +   unsigned int target_mw;
> > +   int ret = 0;
> > +
> > +   mutex_lock(>swap_lock);
> > +   mutex_lock(>lock);
> > +
> > +   if (!port->pps_data.active) {
> > +   ret = -EOPNOTSUPP;
> > +   goto port_unlock;
>
> Or, on top of what I said above, you could actually consider releasing
> the port lock here and just returning. Then you would not need those
> port_unlock and swap_unlock labels at all..
>
> mutex_unlock(>lock);
> return -EOPNOTSUPP;

Based on the comment above, I don't think this makes sense as you'd still need
to release the swap_lock as well, so there would be quite a lot of duplicated
code. Would prefer to stick with the present implementation.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/7] typec: tcpm: Add core support for sink side PPS

[Heikki Krogerus]

On Tue, Jan 02, 2018 at 03:50:52PM +, Adam Thomson wrote:
> This commit adds code to handle requesting of PPS APDOs. Switching
> between standard PDOs and APDOs, and re-requesting an APDO to
> modify operating voltage/current will be triggered by an
> external call into TCPM.
> 
> Signed-off-by: Adam Thomson 
> ---
>  drivers/usb/typec/tcpm.c | 533 
> ++-
>  include/linux/usb/pd.h   |   4 +-
>  include/linux/usb/tcpm.h |   2 +-
>  3 files changed, 525 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index f4d563e..b66d26c 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -47,6 +47,7 @@
>   S(SNK_DISCOVERY_DEBOUNCE_DONE), \
>   S(SNK_WAIT_CAPABILITIES),   \
>   S(SNK_NEGOTIATE_CAPABILITIES),  \
> + S(SNK_NEGOTIATE_PPS_CAPABILITIES),  \
>   S(SNK_TRANSITION_SINK), \
>   S(SNK_TRANSITION_SINK_VBUS),\
>   S(SNK_READY),   \
> @@ -166,6 +167,16 @@ struct pd_mode_data {
>   struct typec_altmode_desc altmode_desc[SVID_DISCOVERY_MAX];
>  };
>  
> +struct pd_pps_data {
> + u32 min_volt;
> + u32 max_volt;
> + u32 max_curr;
> + u32 out_volt;
> + u32 op_curr;
> + bool supported;
> + bool active;
> +};
> +
>  struct tcpm_port {
>   struct device *dev;
>  
> @@ -233,6 +244,7 @@ struct tcpm_port {
>   struct completion swap_complete;
>   int swap_status;
>  
> + unsigned int negotiated_rev;
>   unsigned int message_id;
>   unsigned int caps_count;
>   unsigned int hard_reset_count;
> @@ -255,6 +267,7 @@ struct tcpm_port {
>   unsigned int nr_fixed; /* number of fixed sink PDOs */
>   unsigned int nr_var; /* number of variable sink PDOs */
>   unsigned int nr_batt; /* number of battery sink PDOs */
> + unsigned int nr_apdo; /* number of APDO type PDOs */
>   u32 snk_vdo[VDO_MAX_OBJECTS];
>   unsigned int nr_snk_vdo;
>  
> @@ -262,6 +275,7 @@ struct tcpm_port {
>   unsigned int max_snk_ma;
>   unsigned int max_snk_mw;
>   unsigned int operating_snk_mw;
> + bool update_sink_caps;
>  
>   /* Requested current / voltage */
>   u32 current_limit;
> @@ -278,8 +292,13 @@ struct tcpm_port {
>   /* VDO to retry if UFP responder replied busy */
>   u32 vdo_retry;
>  
> - /* Alternate mode data */
> + /* PPS */
> + struct pd_pps_data pps_data;
> + struct completion pps_complete;
> + bool pps_pending;
> + int pps_status;
>  
> + /* Alternate mode data */
>   struct pd_mode_data mode_data;
>   struct typec_altmode *partner_altmode[SVID_DISCOVERY_MAX];
>   struct typec_altmode *port_altmode[SVID_DISCOVERY_MAX];
> @@ -497,6 +516,16 @@ static void tcpm_log_source_caps(struct tcpm_port *port)
> pdo_max_voltage(pdo),
> pdo_max_power(pdo));
>   break;
> + case PDO_TYPE_APDO:
> + if (pdo_apdo_type(pdo) == APDO_TYPE_PPS)
> + scnprintf(msg, sizeof(msg),
> +   "%u-%u mV, %u mA",
> +   pdo_pps_apdo_min_voltage(pdo),
> +   pdo_pps_apdo_max_voltage(pdo),
> +   pdo_pps_apdo_max_current(pdo));
> + else
> + strcpy(msg, "undefined APDO");
> + break;
>   default:
>   strcpy(msg, "undefined");
>   break;
> @@ -791,11 +820,13 @@ static int tcpm_pd_send_source_caps(struct tcpm_port 
> *port)
>   msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
> port->pwr_role,
> port->data_role,
> +   port->negotiated_rev,
> port->message_id, 0);
>   } else {
>   msg.header = PD_HEADER_LE(PD_DATA_SOURCE_CAP,
> port->pwr_role,
> port->data_role,
> +   port->negotiated_rev,
> port->message_id,
> port->nr_src_pdo);
>   }
> @@ -816,11 +847,13 @@ static int tcpm_pd_send_sink_caps(struct tcpm_port 
> *port)
>   msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
> port->pwr_role,
> port->data_role,
> +   port->negotiated_rev,
> port->message_id, 0);
>   } else {
>   msg.header = PD_HEADER_LE(PD_DATA_SINK_CAP,