subject:"\[PATCH v2 1\/2\] usb\: typec\: Add USB Power Delivery sink port support"

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-27 Thread Bin Gao

On Wed, Jul 27, 2016 at 11:13:43AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Gao  writes:
> > This patch implements a simple USB Power Delivery sink port state machine.
> > It assumes the hardware only handles PD packet transmitting and receiving
> > over the CC line of the USB Type-C connector. The state transition is
> > completely controlled by software. This patch only implement the sink port
> > function and it doesn't support source port and port swap yet.
> >
> > This patch depends on these two patches:
> > https://lkml.org/lkml/2016/6/29/349
> > https://lkml.org/lkml/2016/6/29/350
> >
> > Signed-off-by: Bin Gao 
> > Changes in v2:
> >  - Removed work queue so messages are directly handled in phy driver's 
> > interrupt context
> >  - used pr_debug instead of pr_info for message dump
> >  - Converted PD driver to tristate and typec driver is independent of it
> 
> this should be after the tearline (---) below. We don't want this in
> changelog ;-)
> 
> > +static void handle_source_cap(struct pd_sink_port *port, u8 msg_revision,
> > +   u8 nr_objs, u8 *buf)
> > +{
> > +   int i;
> > +   u32 *obj;
> > +   u8 type;
> > +   struct pd_source_cap *cap = port->source_caps;
> > +
> > +   /*
> > +* The PD spec revision included in SOURCE_CAPABILITY message is the
> > +* highest revision that the Source supports.
> > +*/
> > +   port->pd_rev = msg_revision;
> > +
> > +   /*
> > +* First we need to save all PDOs - they may be used in the future.
> > +* USB PD spec says we must use PDOs in the most recent
> > +* SOURCE_CAPABILITY message. Here we replace old PDOs with new ones.
> > +*/
> > +   port->nr_source_caps = 0;
> > +   for (i = 0; i < nr_objs; i++) {
> > +   obj = (u32 *)(buf + i * PD_OBJ_SIZE);
> > +   type = (*obj >> SOURCE_CAP_TYPE_BIT) & SOURCE_CAP_TYPE_MASK;
> > +   switch (type) {
> > +   case PS_TYPE_FIXED:
> > +   cap->ps_type = PS_TYPE_FIXED;
> > +   cap->fixed = *(struct pd_pdo_src_fixed *)obj;
> > +   break;
> > +   case PS_TYPE_VARIABLE:
> > +   cap->ps_type = PS_TYPE_VARIABLE;
> > +   cap->variable = *(struct pd_pdo_variable *)obj;
> > +   break;
> > +   case PS_TYPE_BATTERY:
> > +   cap->ps_type = PS_TYPE_BATTERY;
> > +   cap->battery = *(struct pd_pdo_battery *)obj;
> > +   break;
> > +   default: /* shouldn't come here */
> > +   pr_err("Invalid Source Capability type: %u.\n", type);
> > +   continue;
> > +   }
> > +   port->nr_source_caps++;
> > +   cap++;
> > +   }
> > +
> > +   if (port->nr_source_caps == 0) {
> > +   pr_err("There is no valid PDOs in SOURCE_CAPABILITY message\n");
> > +   return;
> > +   }
> > +
> > +   /* If a contract is not established, we need send a REQUEST message */
> > +   if (port->state == PD_SINK_STATE_WAIT_FOR_SRC_CAP) {
> 
> this is wrong. Read the fluxchart in figure 8-42. Source can decide to
> send another Source Capability before receiving our Good CRC and we need
> to work with that. This state check is, at a minimum, wrong. I'd
> actually just go ahead and remove it.
> 
> > +   if (!send_request(port))
> > +   port->state = PD_SINK_STATE_REQUEST_SENT;
> > +   }
> > +}
> 
> -- 
> balbi

I'll fix these in next revision. Thanks for your review.

-Bin

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-27 Thread Bin Gao

On Wed, Jul 27, 2016 at 11:13:43AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Gao  writes:
> > This patch implements a simple USB Power Delivery sink port state machine.
> > It assumes the hardware only handles PD packet transmitting and receiving
> > over the CC line of the USB Type-C connector. The state transition is
> > completely controlled by software. This patch only implement the sink port
> > function and it doesn't support source port and port swap yet.
> >
> > This patch depends on these two patches:
> > https://lkml.org/lkml/2016/6/29/349
> > https://lkml.org/lkml/2016/6/29/350
> >
> > Signed-off-by: Bin Gao 
> > Changes in v2:
> >  - Removed work queue so messages are directly handled in phy driver's 
> > interrupt context
> >  - used pr_debug instead of pr_info for message dump
> >  - Converted PD driver to tristate and typec driver is independent of it
> 
> this should be after the tearline (---) below. We don't want this in
> changelog ;-)
> 
> > +static void handle_source_cap(struct pd_sink_port *port, u8 msg_revision,
> > +   u8 nr_objs, u8 *buf)
> > +{
> > +   int i;
> > +   u32 *obj;
> > +   u8 type;
> > +   struct pd_source_cap *cap = port->source_caps;
> > +
> > +   /*
> > +* The PD spec revision included in SOURCE_CAPABILITY message is the
> > +* highest revision that the Source supports.
> > +*/
> > +   port->pd_rev = msg_revision;
> > +
> > +   /*
> > +* First we need to save all PDOs - they may be used in the future.
> > +* USB PD spec says we must use PDOs in the most recent
> > +* SOURCE_CAPABILITY message. Here we replace old PDOs with new ones.
> > +*/
> > +   port->nr_source_caps = 0;
> > +   for (i = 0; i < nr_objs; i++) {
> > +   obj = (u32 *)(buf + i * PD_OBJ_SIZE);
> > +   type = (*obj >> SOURCE_CAP_TYPE_BIT) & SOURCE_CAP_TYPE_MASK;
> > +   switch (type) {
> > +   case PS_TYPE_FIXED:
> > +   cap->ps_type = PS_TYPE_FIXED;
> > +   cap->fixed = *(struct pd_pdo_src_fixed *)obj;
> > +   break;
> > +   case PS_TYPE_VARIABLE:
> > +   cap->ps_type = PS_TYPE_VARIABLE;
> > +   cap->variable = *(struct pd_pdo_variable *)obj;
> > +   break;
> > +   case PS_TYPE_BATTERY:
> > +   cap->ps_type = PS_TYPE_BATTERY;
> > +   cap->battery = *(struct pd_pdo_battery *)obj;
> > +   break;
> > +   default: /* shouldn't come here */
> > +   pr_err("Invalid Source Capability type: %u.\n", type);
> > +   continue;
> > +   }
> > +   port->nr_source_caps++;
> > +   cap++;
> > +   }
> > +
> > +   if (port->nr_source_caps == 0) {
> > +   pr_err("There is no valid PDOs in SOURCE_CAPABILITY message\n");
> > +   return;
> > +   }
> > +
> > +   /* If a contract is not established, we need send a REQUEST message */
> > +   if (port->state == PD_SINK_STATE_WAIT_FOR_SRC_CAP) {
> 
> this is wrong. Read the fluxchart in figure 8-42. Source can decide to
> send another Source Capability before receiving our Good CRC and we need
> to work with that. This state check is, at a minimum, wrong. I'd
> actually just go ahead and remove it.
> 
> > +   if (!send_request(port))
> > +   port->state = PD_SINK_STATE_REQUEST_SENT;
> > +   }
> > +}
> 
> -- 
> balbi

I'll fix these in next revision. Thanks for your review.

-Bin

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-27 Thread Bin Gao

On Wed, Jul 27, 2016 at 11:21:13AM +0200, Oliver Neukum wrote:
> On Tue, 2016-07-26 at 11:37 -0700, Bin Gao wrote:
> > +#define MAKE_HEADER(port, header, msg, objs) \
> > +do { \
> > +   header->type = msg; \
> > +   header->data_role = PD_DATA_ROLE_UFP; \
> > +   header->revision = port->pd_rev; \
> > +   header->power_role = PD_POWER_ROLE_SINK; \
> > +   header->id = roll_msg_id(port); \
> > +   header->nr_objs = objs; \
> > +   header->extended = PD_MSG_NOT_EXTENDED; \
> > +} while (0)
> > +
> > +static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
> > +static int nr_ports;
> > +
> > +BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
> > +
> > +static char *state_strings[] = {
> > +   "WAIT_FOR_SOURCE_CAPABILITY",
> > +   "REQUEST_SENT",
> > +   "ACCEPT_RECEIVED",
> > +   "POWER_SUPPLY_READY",
> > +};
> > +
> > +/* Control messages */
> > +static char *cmsg_strings[] = {
> > +   "GOODCRC",  /* 1 */
> > +   "GOTOMIN",  /* 2 */
> > +   "ACCEPT",   /* 3 */
> > +   "REJECT",   /* 4 */
> > +   "PING", /* 5 */
> > +   "PS_RDY",   /* 6 */
> > +   "GET_SRC_CAP",  /* 7 */
> > +   "GET_SINK_CAP", /* 8 */
> > +   "DR_SWAP",  /* 9 */
> > +   "PR_SWAP",  /* 10 */
> > +   "VCONN_SWAP",   /* 11 */
> > +   "WAIT", /* 12 */
> > +   "SOFT_RESET",   /* 13 */
> > +   "RESERVED", /* 14 */
> > +   "RESERVED", /* 15 */
> > +   "NOT_SUPPORTED",/* 16 */
> > +   "GET_SOURCE_CAP_EXTENDED",  /* 17 */
> > +   "GET_STATUS",   /* 18 */
> > +   "FR_SWAP",  /* 19 */
> > +   /* RESERVED 20 - 31 */
> > +};
> > +
> > +/* Data messages */
> > +static char *dmsg_strings[] = {
> > +   "SOURCE_CAP",   /* 1 */
> > +   "REQUEST",  /* 2 */
> > +   "BIST", /* 3 */
> > +   "SINK_CAP", /* 4 */
> > +   "BATTERY_STATUS",   /* 5 */
> > +   "ALERT",/* 6 */
> > +   "RESERVED", /* 7 */
> > +   "RESERVED", /* 8 */
> > +   "RESERVED", /* 9 */
> > +   "RESERVED", /* 10 */
> > +   "RESERVED", /* 11 */
> > +   "RESERVED", /* 12 */
> > +   "RESERVED", /* 13 */
> > +   "RESERVED", /* 14 */
> > +   "VENDOR_DEFINED",   /* 15 */
> > +   /* RESERVED 16 - 31 */
> > +};
> > +
> > +static char *state_to_string(enum pd_sink_state state)
> > +{
> > +   if (state < ARRAY_SIZE(state_strings))
> > +   return state_strings[state];
> > +   else
> > +   return NULL;
> > +}
> > +
> > +static char *msg_to_string(bool is_cmsg, u8 msg)
> > +{
> > +   int nr = is_cmsg ? ARRAY_SIZE(cmsg_strings) :
> > ARRAY_SIZE(dmsg_strings);
> > +
> > +   if (msg <= nr)
> > +   return is_cmsg ? cmsg_strings[msg - 1] :
> > dmsg_strings[msg - 1];
> > +   else
> > +   return "RESERVED";
> > +}
> > +
> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > +   pr_debug("sink port %d: %s message %s %s\n", port,
> > +   is_cmsg ? "Control" : "Data",
> > +   msg_to_string(is_cmsg, msg),
> > +recv ? "received" : "sent)");
> > +}
> > +
> > +static inline bool fixed_ps_equal(struct sink_ps *p1,
> > +   struct sink_ps *p2)
> > +{
> > +   return p1->ps_type == p2->ps_type &&
> > +   p1->ps_fixed.voltage_fixed ==
> > p2->ps_fixed.voltage_fixed &&
> > +   p1->ps_fixed.current_default ==
> > p2->ps_fixed.current_default;
> > +}
> > +
> > +/* The message ID increments each time we send out a new message */
> > +static u8 roll_msg_id(struct pd_sink_port *port)
> > +{
> > +   u8 msg_id = port->msg_id;
> > +
> > +   if (msg_id == PD_MSG_ID_MAX)
> > +   msg_id = PD_MSG_ID_MIN;
> > +   else
> > +   msg_id++;
> > +
> > +   port->msg_id = msg_id;
> > +   return msg_id;
> > +}
> > +
> 
> These pieces of code are completely generic. They should be shared
> among drivers. Please move them into the include files.
> 
>   Regards
>   Oliver
> 
> 

They are only internally used by this driver (USB PD state machine driver).
And they are not used by drivers (typicall USB Type-C phy drivers) which
use APIs exposed by this driver. So I think it's not appropriate to move
to include files.

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-27 Thread Bin Gao

On Wed, Jul 27, 2016 at 11:21:13AM +0200, Oliver Neukum wrote:
> On Tue, 2016-07-26 at 11:37 -0700, Bin Gao wrote:
> > +#define MAKE_HEADER(port, header, msg, objs) \
> > +do { \
> > +   header->type = msg; \
> > +   header->data_role = PD_DATA_ROLE_UFP; \
> > +   header->revision = port->pd_rev; \
> > +   header->power_role = PD_POWER_ROLE_SINK; \
> > +   header->id = roll_msg_id(port); \
> > +   header->nr_objs = objs; \
> > +   header->extended = PD_MSG_NOT_EXTENDED; \
> > +} while (0)
> > +
> > +static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
> > +static int nr_ports;
> > +
> > +BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
> > +
> > +static char *state_strings[] = {
> > +   "WAIT_FOR_SOURCE_CAPABILITY",
> > +   "REQUEST_SENT",
> > +   "ACCEPT_RECEIVED",
> > +   "POWER_SUPPLY_READY",
> > +};
> > +
> > +/* Control messages */
> > +static char *cmsg_strings[] = {
> > +   "GOODCRC",  /* 1 */
> > +   "GOTOMIN",  /* 2 */
> > +   "ACCEPT",   /* 3 */
> > +   "REJECT",   /* 4 */
> > +   "PING", /* 5 */
> > +   "PS_RDY",   /* 6 */
> > +   "GET_SRC_CAP",  /* 7 */
> > +   "GET_SINK_CAP", /* 8 */
> > +   "DR_SWAP",  /* 9 */
> > +   "PR_SWAP",  /* 10 */
> > +   "VCONN_SWAP",   /* 11 */
> > +   "WAIT", /* 12 */
> > +   "SOFT_RESET",   /* 13 */
> > +   "RESERVED", /* 14 */
> > +   "RESERVED", /* 15 */
> > +   "NOT_SUPPORTED",/* 16 */
> > +   "GET_SOURCE_CAP_EXTENDED",  /* 17 */
> > +   "GET_STATUS",   /* 18 */
> > +   "FR_SWAP",  /* 19 */
> > +   /* RESERVED 20 - 31 */
> > +};
> > +
> > +/* Data messages */
> > +static char *dmsg_strings[] = {
> > +   "SOURCE_CAP",   /* 1 */
> > +   "REQUEST",  /* 2 */
> > +   "BIST", /* 3 */
> > +   "SINK_CAP", /* 4 */
> > +   "BATTERY_STATUS",   /* 5 */
> > +   "ALERT",/* 6 */
> > +   "RESERVED", /* 7 */
> > +   "RESERVED", /* 8 */
> > +   "RESERVED", /* 9 */
> > +   "RESERVED", /* 10 */
> > +   "RESERVED", /* 11 */
> > +   "RESERVED", /* 12 */
> > +   "RESERVED", /* 13 */
> > +   "RESERVED", /* 14 */
> > +   "VENDOR_DEFINED",   /* 15 */
> > +   /* RESERVED 16 - 31 */
> > +};
> > +
> > +static char *state_to_string(enum pd_sink_state state)
> > +{
> > +   if (state < ARRAY_SIZE(state_strings))
> > +   return state_strings[state];
> > +   else
> > +   return NULL;
> > +}
> > +
> > +static char *msg_to_string(bool is_cmsg, u8 msg)
> > +{
> > +   int nr = is_cmsg ? ARRAY_SIZE(cmsg_strings) :
> > ARRAY_SIZE(dmsg_strings);
> > +
> > +   if (msg <= nr)
> > +   return is_cmsg ? cmsg_strings[msg - 1] :
> > dmsg_strings[msg - 1];
> > +   else
> > +   return "RESERVED";
> > +}
> > +
> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > +   pr_debug("sink port %d: %s message %s %s\n", port,
> > +   is_cmsg ? "Control" : "Data",
> > +   msg_to_string(is_cmsg, msg),
> > +recv ? "received" : "sent)");
> > +}
> > +
> > +static inline bool fixed_ps_equal(struct sink_ps *p1,
> > +   struct sink_ps *p2)
> > +{
> > +   return p1->ps_type == p2->ps_type &&
> > +   p1->ps_fixed.voltage_fixed ==
> > p2->ps_fixed.voltage_fixed &&
> > +   p1->ps_fixed.current_default ==
> > p2->ps_fixed.current_default;
> > +}
> > +
> > +/* The message ID increments each time we send out a new message */
> > +static u8 roll_msg_id(struct pd_sink_port *port)
> > +{
> > +   u8 msg_id = port->msg_id;
> > +
> > +   if (msg_id == PD_MSG_ID_MAX)
> > +   msg_id = PD_MSG_ID_MIN;
> > +   else
> > +   msg_id++;
> > +
> > +   port->msg_id = msg_id;
> > +   return msg_id;
> > +}
> > +
> 
> These pieces of code are completely generic. They should be shared
> among drivers. Please move them into the include files.
> 
>   Regards
>   Oliver
> 
> 

They are only internally used by this driver (USB PD state machine driver).
And they are not used by drivers (typicall USB Type-C phy drivers) which
use APIs exposed by this driver. So I think it's not appropriate to move
to include files.

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-27 Thread Oliver Neukum

On Tue, 2016-07-26 at 11:37 -0700, Bin Gao wrote:
> +#define MAKE_HEADER(port, header, msg, objs) \
> +do { \
> +   header->type = msg; \
> +   header->data_role = PD_DATA_ROLE_UFP; \
> +   header->revision = port->pd_rev; \
> +   header->power_role = PD_POWER_ROLE_SINK; \
> +   header->id = roll_msg_id(port); \
> +   header->nr_objs = objs; \
> +   header->extended = PD_MSG_NOT_EXTENDED; \
> +} while (0)
> +
> +static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
> +static int nr_ports;
> +
> +BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
> +
> +static char *state_strings[] = {
> +   "WAIT_FOR_SOURCE_CAPABILITY",
> +   "REQUEST_SENT",
> +   "ACCEPT_RECEIVED",
> +   "POWER_SUPPLY_READY",
> +};
> +
> +/* Control messages */
> +static char *cmsg_strings[] = {
> +   "GOODCRC",  /* 1 */
> +   "GOTOMIN",  /* 2 */
> +   "ACCEPT",   /* 3 */
> +   "REJECT",   /* 4 */
> +   "PING", /* 5 */
> +   "PS_RDY",   /* 6 */
> +   "GET_SRC_CAP",  /* 7 */
> +   "GET_SINK_CAP", /* 8 */
> +   "DR_SWAP",  /* 9 */
> +   "PR_SWAP",  /* 10 */
> +   "VCONN_SWAP",   /* 11 */
> +   "WAIT", /* 12 */
> +   "SOFT_RESET",   /* 13 */
> +   "RESERVED", /* 14 */
> +   "RESERVED", /* 15 */
> +   "NOT_SUPPORTED",/* 16 */
> +   "GET_SOURCE_CAP_EXTENDED",  /* 17 */
> +   "GET_STATUS",   /* 18 */
> +   "FR_SWAP",  /* 19 */
> +   /* RESERVED 20 - 31 */
> +};
> +
> +/* Data messages */
> +static char *dmsg_strings[] = {
> +   "SOURCE_CAP",   /* 1 */
> +   "REQUEST",  /* 2 */
> +   "BIST", /* 3 */
> +   "SINK_CAP", /* 4 */
> +   "BATTERY_STATUS",   /* 5 */
> +   "ALERT",/* 6 */
> +   "RESERVED", /* 7 */
> +   "RESERVED", /* 8 */
> +   "RESERVED", /* 9 */
> +   "RESERVED", /* 10 */
> +   "RESERVED", /* 11 */
> +   "RESERVED", /* 12 */
> +   "RESERVED", /* 13 */
> +   "RESERVED", /* 14 */
> +   "VENDOR_DEFINED",   /* 15 */
> +   /* RESERVED 16 - 31 */
> +};
> +
> +static char *state_to_string(enum pd_sink_state state)
> +{
> +   if (state < ARRAY_SIZE(state_strings))
> +   return state_strings[state];
> +   else
> +   return NULL;
> +}
> +
> +static char *msg_to_string(bool is_cmsg, u8 msg)
> +{
> +   int nr = is_cmsg ? ARRAY_SIZE(cmsg_strings) :
> ARRAY_SIZE(dmsg_strings);
> +
> +   if (msg <= nr)
> +   return is_cmsg ? cmsg_strings[msg - 1] :
> dmsg_strings[msg - 1];
> +   else
> +   return "RESERVED";
> +}
> +
> +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> +{
> +   pr_debug("sink port %d: %s message %s %s\n", port,
> +   is_cmsg ? "Control" : "Data",
> +   msg_to_string(is_cmsg, msg),
> +recv ? "received" : "sent)");
> +}
> +
> +static inline bool fixed_ps_equal(struct sink_ps *p1,
> +   struct sink_ps *p2)
> +{
> +   return p1->ps_type == p2->ps_type &&
> +   p1->ps_fixed.voltage_fixed ==
> p2->ps_fixed.voltage_fixed &&
> +   p1->ps_fixed.current_default ==
> p2->ps_fixed.current_default;
> +}
> +
> +/* The message ID increments each time we send out a new message */
> +static u8 roll_msg_id(struct pd_sink_port *port)
> +{
> +   u8 msg_id = port->msg_id;
> +
> +   if (msg_id == PD_MSG_ID_MAX)
> +   msg_id = PD_MSG_ID_MIN;
> +   else
> +   msg_id++;
> +
> +   port->msg_id = msg_id;
> +   return msg_id;
> +}
> +

These pieces of code are completely generic. They should be shared
among drivers. Please move them into the include files.

Regards
Oliver

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-27 Thread Oliver Neukum

On Tue, 2016-07-26 at 11:37 -0700, Bin Gao wrote:
> +#define MAKE_HEADER(port, header, msg, objs) \
> +do { \
> +   header->type = msg; \
> +   header->data_role = PD_DATA_ROLE_UFP; \
> +   header->revision = port->pd_rev; \
> +   header->power_role = PD_POWER_ROLE_SINK; \
> +   header->id = roll_msg_id(port); \
> +   header->nr_objs = objs; \
> +   header->extended = PD_MSG_NOT_EXTENDED; \
> +} while (0)
> +
> +static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
> +static int nr_ports;
> +
> +BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
> +
> +static char *state_strings[] = {
> +   "WAIT_FOR_SOURCE_CAPABILITY",
> +   "REQUEST_SENT",
> +   "ACCEPT_RECEIVED",
> +   "POWER_SUPPLY_READY",
> +};
> +
> +/* Control messages */
> +static char *cmsg_strings[] = {
> +   "GOODCRC",  /* 1 */
> +   "GOTOMIN",  /* 2 */
> +   "ACCEPT",   /* 3 */
> +   "REJECT",   /* 4 */
> +   "PING", /* 5 */
> +   "PS_RDY",   /* 6 */
> +   "GET_SRC_CAP",  /* 7 */
> +   "GET_SINK_CAP", /* 8 */
> +   "DR_SWAP",  /* 9 */
> +   "PR_SWAP",  /* 10 */
> +   "VCONN_SWAP",   /* 11 */
> +   "WAIT", /* 12 */
> +   "SOFT_RESET",   /* 13 */
> +   "RESERVED", /* 14 */
> +   "RESERVED", /* 15 */
> +   "NOT_SUPPORTED",/* 16 */
> +   "GET_SOURCE_CAP_EXTENDED",  /* 17 */
> +   "GET_STATUS",   /* 18 */
> +   "FR_SWAP",  /* 19 */
> +   /* RESERVED 20 - 31 */
> +};
> +
> +/* Data messages */
> +static char *dmsg_strings[] = {
> +   "SOURCE_CAP",   /* 1 */
> +   "REQUEST",  /* 2 */
> +   "BIST", /* 3 */
> +   "SINK_CAP", /* 4 */
> +   "BATTERY_STATUS",   /* 5 */
> +   "ALERT",/* 6 */
> +   "RESERVED", /* 7 */
> +   "RESERVED", /* 8 */
> +   "RESERVED", /* 9 */
> +   "RESERVED", /* 10 */
> +   "RESERVED", /* 11 */
> +   "RESERVED", /* 12 */
> +   "RESERVED", /* 13 */
> +   "RESERVED", /* 14 */
> +   "VENDOR_DEFINED",   /* 15 */
> +   /* RESERVED 16 - 31 */
> +};
> +
> +static char *state_to_string(enum pd_sink_state state)
> +{
> +   if (state < ARRAY_SIZE(state_strings))
> +   return state_strings[state];
> +   else
> +   return NULL;
> +}
> +
> +static char *msg_to_string(bool is_cmsg, u8 msg)
> +{
> +   int nr = is_cmsg ? ARRAY_SIZE(cmsg_strings) :
> ARRAY_SIZE(dmsg_strings);
> +
> +   if (msg <= nr)
> +   return is_cmsg ? cmsg_strings[msg - 1] :
> dmsg_strings[msg - 1];
> +   else
> +   return "RESERVED";
> +}
> +
> +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> +{
> +   pr_debug("sink port %d: %s message %s %s\n", port,
> +   is_cmsg ? "Control" : "Data",
> +   msg_to_string(is_cmsg, msg),
> +recv ? "received" : "sent)");
> +}
> +
> +static inline bool fixed_ps_equal(struct sink_ps *p1,
> +   struct sink_ps *p2)
> +{
> +   return p1->ps_type == p2->ps_type &&
> +   p1->ps_fixed.voltage_fixed ==
> p2->ps_fixed.voltage_fixed &&
> +   p1->ps_fixed.current_default ==
> p2->ps_fixed.current_default;
> +}
> +
> +/* The message ID increments each time we send out a new message */
> +static u8 roll_msg_id(struct pd_sink_port *port)
> +{
> +   u8 msg_id = port->msg_id;
> +
> +   if (msg_id == PD_MSG_ID_MAX)
> +   msg_id = PD_MSG_ID_MIN;
> +   else
> +   msg_id++;
> +
> +   port->msg_id = msg_id;
> +   return msg_id;
> +}
> +

These pieces of code are completely generic. They should be shared
among drivers. Please move them into the include files.

Regards
Oliver

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-27 Thread Felipe Balbi


Hi,

Bin Gao  writes:
> This patch implements a simple USB Power Delivery sink port state machine.
> It assumes the hardware only handles PD packet transmitting and receiving
> over the CC line of the USB Type-C connector. The state transition is
> completely controlled by software. This patch only implement the sink port
> function and it doesn't support source port and port swap yet.
>
> This patch depends on these two patches:
> https://lkml.org/lkml/2016/6/29/349
> https://lkml.org/lkml/2016/6/29/350
>
> Signed-off-by: Bin Gao 
> Changes in v2:
>  - Removed work queue so messages are directly handled in phy driver's 
> interrupt context
>  - used pr_debug instead of pr_info for message dump
>  - Converted PD driver to tristate and typec driver is independent of it

this should be after the tearline (---) below. We don't want this in
changelog ;-)

> +static void handle_source_cap(struct pd_sink_port *port, u8 msg_revision,
> + u8 nr_objs, u8 *buf)
> +{
> + int i;
> + u32 *obj;
> + u8 type;
> + struct pd_source_cap *cap = port->source_caps;
> +
> + /*
> +  * The PD spec revision included in SOURCE_CAPABILITY message is the
> +  * highest revision that the Source supports.
> +  */
> + port->pd_rev = msg_revision;
> +
> + /*
> +  * First we need to save all PDOs - they may be used in the future.
> +  * USB PD spec says we must use PDOs in the most recent
> +  * SOURCE_CAPABILITY message. Here we replace old PDOs with new ones.
> +  */
> + port->nr_source_caps = 0;
> + for (i = 0; i < nr_objs; i++) {
> + obj = (u32 *)(buf + i * PD_OBJ_SIZE);
> + type = (*obj >> SOURCE_CAP_TYPE_BIT) & SOURCE_CAP_TYPE_MASK;
> + switch (type) {
> + case PS_TYPE_FIXED:
> + cap->ps_type = PS_TYPE_FIXED;
> + cap->fixed = *(struct pd_pdo_src_fixed *)obj;
> + break;
> + case PS_TYPE_VARIABLE:
> + cap->ps_type = PS_TYPE_VARIABLE;
> + cap->variable = *(struct pd_pdo_variable *)obj;
> + break;
> + case PS_TYPE_BATTERY:
> + cap->ps_type = PS_TYPE_BATTERY;
> + cap->battery = *(struct pd_pdo_battery *)obj;
> + break;
> + default: /* shouldn't come here */
> + pr_err("Invalid Source Capability type: %u.\n", type);
> + continue;
> + }
> + port->nr_source_caps++;
> + cap++;
> + }
> +
> + if (port->nr_source_caps == 0) {
> + pr_err("There is no valid PDOs in SOURCE_CAPABILITY message\n");
> + return;
> + }
> +
> + /* If a contract is not established, we need send a REQUEST message */
> + if (port->state == PD_SINK_STATE_WAIT_FOR_SRC_CAP) {

this is wrong. Read the fluxchart in figure 8-42. Source can decide to
send another Source Capability before receiving our Good CRC and we need
to work with that. This state check is, at a minimum, wrong. I'd
actually just go ahead and remove it.

> + if (!send_request(port))
> + port->state = PD_SINK_STATE_REQUEST_SENT;
> + }
> +}

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-27 Thread Felipe Balbi


Hi,

Bin Gao  writes:
> This patch implements a simple USB Power Delivery sink port state machine.
> It assumes the hardware only handles PD packet transmitting and receiving
> over the CC line of the USB Type-C connector. The state transition is
> completely controlled by software. This patch only implement the sink port
> function and it doesn't support source port and port swap yet.
>
> This patch depends on these two patches:
> https://lkml.org/lkml/2016/6/29/349
> https://lkml.org/lkml/2016/6/29/350
>
> Signed-off-by: Bin Gao 
> Changes in v2:
>  - Removed work queue so messages are directly handled in phy driver's 
> interrupt context
>  - used pr_debug instead of pr_info for message dump
>  - Converted PD driver to tristate and typec driver is independent of it

this should be after the tearline (---) below. We don't want this in
changelog ;-)

> +static void handle_source_cap(struct pd_sink_port *port, u8 msg_revision,
> + u8 nr_objs, u8 *buf)
> +{
> + int i;
> + u32 *obj;
> + u8 type;
> + struct pd_source_cap *cap = port->source_caps;
> +
> + /*
> +  * The PD spec revision included in SOURCE_CAPABILITY message is the
> +  * highest revision that the Source supports.
> +  */
> + port->pd_rev = msg_revision;
> +
> + /*
> +  * First we need to save all PDOs - they may be used in the future.
> +  * USB PD spec says we must use PDOs in the most recent
> +  * SOURCE_CAPABILITY message. Here we replace old PDOs with new ones.
> +  */
> + port->nr_source_caps = 0;
> + for (i = 0; i < nr_objs; i++) {
> + obj = (u32 *)(buf + i * PD_OBJ_SIZE);
> + type = (*obj >> SOURCE_CAP_TYPE_BIT) & SOURCE_CAP_TYPE_MASK;
> + switch (type) {
> + case PS_TYPE_FIXED:
> + cap->ps_type = PS_TYPE_FIXED;
> + cap->fixed = *(struct pd_pdo_src_fixed *)obj;
> + break;
> + case PS_TYPE_VARIABLE:
> + cap->ps_type = PS_TYPE_VARIABLE;
> + cap->variable = *(struct pd_pdo_variable *)obj;
> + break;
> + case PS_TYPE_BATTERY:
> + cap->ps_type = PS_TYPE_BATTERY;
> + cap->battery = *(struct pd_pdo_battery *)obj;
> + break;
> + default: /* shouldn't come here */
> + pr_err("Invalid Source Capability type: %u.\n", type);
> + continue;
> + }
> + port->nr_source_caps++;
> + cap++;
> + }
> +
> + if (port->nr_source_caps == 0) {
> + pr_err("There is no valid PDOs in SOURCE_CAPABILITY message\n");
> + return;
> + }
> +
> + /* If a contract is not established, we need send a REQUEST message */
> + if (port->state == PD_SINK_STATE_WAIT_FOR_SRC_CAP) {

this is wrong. Read the fluxchart in figure 8-42. Source can decide to
send another Source Capability before receiving our Good CRC and we need
to work with that. This state check is, at a minimum, wrong. I'd
actually just go ahead and remove it.

> + if (!send_request(port))
> + port->state = PD_SINK_STATE_REQUEST_SENT;
> + }
> +}

-- 
balbi


signature.asc
Description: PGP signature

[PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-26 Thread Bin Gao

This patch implements a simple USB Power Delivery sink port state machine.
It assumes the hardware only handles PD packet transmitting and receiving
over the CC line of the USB Type-C connector. The state transition is
completely controlled by software. This patch only implement the sink port
function and it doesn't support source port and port swap yet.

This patch depends on these two patches:
https://lkml.org/lkml/2016/6/29/349
https://lkml.org/lkml/2016/6/29/350

Signed-off-by: Bin Gao 
Changes in v2:
 - Removed work queue so messages are directly handled in phy driver's 
interrupt context
 - used pr_debug instead of pr_info for message dump
 - Converted PD driver to tristate and typec driver is independent of it
---
 drivers/usb/typec/Kconfig  |  11 +
 drivers/usb/typec/Makefile |   1 +
 drivers/usb/typec/pd_sink.c| 908 +
 include/linux/usb/pd_message.h | 369 +
 include/linux/usb/pd_sink.h| 299 ++
 5 files changed, 1588 insertions(+)
 create mode 100644 drivers/usb/typec/pd_sink.c
 create mode 100644 include/linux/usb/pd_message.h
 create mode 100644 include/linux/usb/pd_sink.h

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 7a345a4..662aa54 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,6 +4,17 @@ menu "USB PD and Type-C drivers"
 config TYPEC
tristate
 
+config USB_PD_SINK
+   tristate "USB Power Delivery Sink Port State Machine Driver"
+   help
+ Enable this to support USB PD(Power Delivery) Sink port.
+ This driver implements a simple USB PD sink state machine.
+ The underlying TypeC phy driver is responsible for cable
+ plug/unplug event, port orientation detection, transmitting
+ and receiving PD messages. This driver only process messages
+ received by the TypeC phy driver and maintain the sink port's
+ state machine.
+
 config TYPEC_WCOVE
tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
depends on ACPI
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index b9cb862..74aad6c 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_TYPEC)+= typec.o
+obj-$(CONFIG_USB_PD_SINK)  += pd_sink.o
 obj-$(CONFIG_TYPEC_WCOVE)  += typec_wcove.o
diff --git a/drivers/usb/typec/pd_sink.c b/drivers/usb/typec/pd_sink.c
new file mode 100644
index 000..1e46d3c
--- /dev/null
+++ b/drivers/usb/typec/pd_sink.c
@@ -0,0 +1,908 @@
+/*
+ * pd_sink.c - USB PD (Power Delivery) sink port state machine driver
+ *
+ * This driver implements a simple USB PD sink port state machine.
+ * It assumes the upper layer, i.e. the user of this driver, handles
+ * the PD message receiving and transmitting. The upper layer receives
+ * PD messages from the Source, queues them to us, and when processing
+ * the received message we'll call upper layer's transmitting function
+ * to send PD messages to the source.
+ * The sink port state machine is maintained in this driver but we also
+ * broadcast some important PD messages to upper layer as events.
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Bin Gao 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+
+#define MAKE_HEADER(port, header, msg, objs) \
+do { \
+   header->type = msg; \
+   header->data_role = PD_DATA_ROLE_UFP; \
+   header->revision = port->pd_rev; \
+   header->power_role = PD_POWER_ROLE_SINK; \
+   header->id = roll_msg_id(port); \
+   header->nr_objs = objs; \
+   header->extended = PD_MSG_NOT_EXTENDED; \
+} while (0)
+
+static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
+static int nr_ports;
+
+BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
+
+static char *state_strings[] = {
+   "WAIT_FOR_SOURCE_CAPABILITY",
+   "REQUEST_SENT",
+   "ACCEPT_RECEIVED",
+   "POWER_SUPPLY_READY",
+};
+
+/* Control messages */
+static char *cmsg_strings[] = {
+   "GOODCRC",  /* 1 */
+   "GOTOMIN",  /* 2 */
+   "ACCEPT",   /* 3 */
+   "REJECT",   /* 4 */
+   "PING", /* 5 */
+   "PS_RDY",   /* 6 */
+   "GET_SRC_CAP",  /* 7 */
+   "GET_SINK_CAP", /* 8 */
+   "DR_SWAP",  /* 9 */
+   "PR_SWAP",  /* 10 */
+   "VCONN_SWAP",   /* 11 */
+   "WAIT", /* 12 */
+   "SOFT_RESET",   /* 13 */
+   "RESERVED", /* 14 */

[PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

2016-07-26 Thread Bin Gao

This patch implements a simple USB Power Delivery sink port state machine.
It assumes the hardware only handles PD packet transmitting and receiving
over the CC line of the USB Type-C connector. The state transition is
completely controlled by software. This patch only implement the sink port
function and it doesn't support source port and port swap yet.

This patch depends on these two patches:
https://lkml.org/lkml/2016/6/29/349
https://lkml.org/lkml/2016/6/29/350

Signed-off-by: Bin Gao 
Changes in v2:
 - Removed work queue so messages are directly handled in phy driver's 
interrupt context
 - used pr_debug instead of pr_info for message dump
 - Converted PD driver to tristate and typec driver is independent of it
---
 drivers/usb/typec/Kconfig  |  11 +
 drivers/usb/typec/Makefile |   1 +
 drivers/usb/typec/pd_sink.c| 908 +
 include/linux/usb/pd_message.h | 369 +
 include/linux/usb/pd_sink.h| 299 ++
 5 files changed, 1588 insertions(+)
 create mode 100644 drivers/usb/typec/pd_sink.c
 create mode 100644 include/linux/usb/pd_message.h
 create mode 100644 include/linux/usb/pd_sink.h

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 7a345a4..662aa54 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,6 +4,17 @@ menu "USB PD and Type-C drivers"
 config TYPEC
tristate
 
+config USB_PD_SINK
+   tristate "USB Power Delivery Sink Port State Machine Driver"
+   help
+ Enable this to support USB PD(Power Delivery) Sink port.
+ This driver implements a simple USB PD sink state machine.
+ The underlying TypeC phy driver is responsible for cable
+ plug/unplug event, port orientation detection, transmitting
+ and receiving PD messages. This driver only process messages
+ received by the TypeC phy driver and maintain the sink port's
+ state machine.
+
 config TYPEC_WCOVE
tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
depends on ACPI
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index b9cb862..74aad6c 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_TYPEC)+= typec.o
+obj-$(CONFIG_USB_PD_SINK)  += pd_sink.o
 obj-$(CONFIG_TYPEC_WCOVE)  += typec_wcove.o
diff --git a/drivers/usb/typec/pd_sink.c b/drivers/usb/typec/pd_sink.c
new file mode 100644
index 000..1e46d3c
--- /dev/null
+++ b/drivers/usb/typec/pd_sink.c
@@ -0,0 +1,908 @@
+/*
+ * pd_sink.c - USB PD (Power Delivery) sink port state machine driver
+ *
+ * This driver implements a simple USB PD sink port state machine.
+ * It assumes the upper layer, i.e. the user of this driver, handles
+ * the PD message receiving and transmitting. The upper layer receives
+ * PD messages from the Source, queues them to us, and when processing
+ * the received message we'll call upper layer's transmitting function
+ * to send PD messages to the source.
+ * The sink port state machine is maintained in this driver but we also
+ * broadcast some important PD messages to upper layer as events.
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Bin Gao 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+
+#define MAKE_HEADER(port, header, msg, objs) \
+do { \
+   header->type = msg; \
+   header->data_role = PD_DATA_ROLE_UFP; \
+   header->revision = port->pd_rev; \
+   header->power_role = PD_POWER_ROLE_SINK; \
+   header->id = roll_msg_id(port); \
+   header->nr_objs = objs; \
+   header->extended = PD_MSG_NOT_EXTENDED; \
+} while (0)
+
+static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
+static int nr_ports;
+
+BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
+
+static char *state_strings[] = {
+   "WAIT_FOR_SOURCE_CAPABILITY",
+   "REQUEST_SENT",
+   "ACCEPT_RECEIVED",
+   "POWER_SUPPLY_READY",
+};
+
+/* Control messages */
+static char *cmsg_strings[] = {
+   "GOODCRC",  /* 1 */
+   "GOTOMIN",  /* 2 */
+   "ACCEPT",   /* 3 */
+   "REJECT",   /* 4 */
+   "PING", /* 5 */
+   "PS_RDY",   /* 6 */
+   "GET_SRC_CAP",  /* 7 */
+   "GET_SINK_CAP", /* 8 */
+   "DR_SWAP",  /* 9 */
+   "PR_SWAP",  /* 10 */
+   "VCONN_SWAP",   /* 11 */
+   "WAIT", /* 12 */
+   "SOFT_RESET",   /* 13 */
+   "RESERVED", /* 14 */
+   "RESERVED",

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

[PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

[PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

10 matches

Site Navigation

Mail list logo

Footer information