Message-ID: <20160715234957.GC159605@worksta>
Date:	Fri, 15 Jul 2016 16:49:57 -0700
From:	Bin Gao <bin.gao@...ux.intel.com>
To:	Felipe Balbi <felipe.balbi@...ux.intel.com>
Cc:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	Bin Gao <bin.gao@...el.com>,
	Chandra Sekhar Anagani <chandra.sekhar.anagani@...el.com>
Subject: Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support

On Fri, Jul 15, 2016 at 10:25:36AM +0300, Felipe Balbi wrote:
> Bin Gao <bin.gao@...ux.intel.com> 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 <bin.gao@...el.com>
> > ---
> >  drivers/usb/typec/Kconfig      |  13 +
> >  drivers/usb/typec/Makefile     |   1 +
> >  drivers/usb/typec/pd_sink.c    | 967 +++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb/pd_message.h | 371 ++++++++++++++++
> >  include/linux/usb/pd_sink.h    | 286 ++++++++++++
> >  5 files changed, 1638 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..a04a900 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -4,12 +4,25 @@ menu "USB PD and Type-C drivers"
> >  config TYPEC
> >  	tristate
> >  
> > +config USB_PD_SINK
> > +	bool "USB Power Delivery Sink Port State Machine Driver"
> 
> tristate?
> 
> > +	select TYPEC
> 
> this should depend on TYPEC, not select it.
> 
> > +	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
> >  	depends on INTEL_SOC_PMIC
> >  	depends on INTEL_PMC_IPC
> >  	select TYPEC
> > +	select USB_PD_SINK
> 
> TYPEC without PD is valid, let user select PD support.
Yes will fix the Kconfig in next revision.

> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > +				is_cmsg ? "Control" : "Data",
> > +				msg_to_string(is_cmsg, msg),
> > +		 recv ? "received" : "sent(wait GOODCRC)");
> 
> looks like a debugging message to me. We don't want to spam dmesg with
> every single message transmission.
This should be a pr_debug().

> 
> > +static void start_timer(struct pd_sink_port *port, int timeout,
> > +		enum hrtimer_restart (*f)(struct hrtimer *))
> > +{
> > +	if (hrtimer_active(&port->tx_timer)) {
> > +		pr_err("Error: previous timer is still active\n");
> > +		return;
> > +	}
> > +
> > +	port->tx_timer.function = f;
> > +	/* timeout comes with ms but ktime_set takes seconds and nanoseconds */
> > +	hrtimer_start(&port->tx_timer, ktime_set(timeout / 1000,
> 
> I don't think you need HR timers here. A normal mod_timer() should do.
When hrtimer is in place, the old timer becomes "legacy". And the old timer
APIs are implemented on top of hrtimer. It's no harm to use hrtimers anywhere
in the kernel and it would be encouraged in my opinion:-)

> 
> > +static enum hrtimer_restart goodcrc_timeout(struct hrtimer *timer)
> > +{
> > +	pr_err("GOODCRC message is not received in %d ms: timeout\n",
> > +						PD_TIMEOUT_GOODCRC);
> > +	return HRTIMER_NORESTART;
> > +}
> > +
> > +/*
> > + * For any message we send, we must get a GOODCRC message from the Source.
> > + * The USB PD spec says the time should be measured between the last bit
> > + * of the sending message's EOP has been transmitted and the last bit of
> > + * the receiving GOODCRC message's EOP has been received. The allowed time
> > + * is minimal 0.9 ms and maximal 1.1 ms. However, this measurement is
> > + * performed in physical layer. When it reaches to the OS and this driver,
> > + * the actual time is difficult to predict because of the scheduling,
> > + * context switch, interrupt preemption and nesting, etc. So we only define
> > + * a safe timeout value (PD_TIMEOUT_GOODCRC) which is large enough to take
> > + * account of all software related latency.
> 
> that's true. But here's the thing. From 1ms to 500ms there are two
> orders of magnitude. If we take that long to realize we received
> GoodCRC, it's already too late. Remember that if we don't e.g. request
> power within 30ms after reception of GoodCRC, Source will issue a
> HardReset.
This part is for sure to be revisited even in the beginning of the design.
The hardware(Type-C PHY) we tested on forces auto GOODCRC transmission
and reception and doesn't report to software so we actually have no way
to test it. But any way this timeout value will be rivisited. 

> 
> > + */
> > +static int send_message(struct pd_sink_port *port, void *buf, int len,
> > +			u8 msg, bool ctrl_msg, enum sop_type sop_type)
> > +{
> > +	if (ctrl_msg && msg == PD_CMSG_GOODCRC) {
> > +		port->tx(port->port, port->tx_priv, buf, len, sop_type);
> > +		print_message(port->port, true, msg, false);
> > +		return 0;
> > +	}
> > +
> > +	port->tx(port->port, port->tx_priv, buf, len, sop_type);
> > +	print_message(port->port, ctrl_msg, msg, false);
> > +
> > +	if (!port->hw_goodcrc_rx) {
> > +		port->waiting_goodcrc = true;
> > +		start_timer(port, PD_TIMEOUT_GOODCRC, goodcrc_timeout);
> 
> Every port waits for goodcrc, it's just that some will receive it and
> process it in SW, while others will just notify of its reception.
> 
> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> > +{
> > +	unsigned long flags;
> > +	struct pd_sink_port *port;
> > +
> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> > +		pr_err("Invalid port number\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	port = sink_ports[msg->port];
> > +
> > +	spin_lock_irqsave(&port->rx_lock, flags);
> > +	list_add_tail(&msg->list, &port->rx_list);
> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
> > +
> > +	queue_work(port->rx_wq, &port->rx_work);
> 
> can we really queue several messages at a time? It seems unfeasible to
> me. It's not like we can queue several power request in a role. Why do
> you need this workqueue? Why don't you process message here, in place?
Some Type-C chargers send two messages in a short duration(less than 1 ms),
e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
message to PD stack by Type-C phy driver typically happens in a interrupt
context. So in this case a nested interrupt may happen. Our whole PD
stack while processing one message is not re-entrant so the nested
interrupt would cause a problem.
But I'll re-check and see if it will really happen.
Yes I agree the workqueue will introduce delay so we could miss the
tSenderResponse timeout deadline.
Most likey I'll remove the workqueue and directly process message one
bye one in the next revision.

> 
> -- 
> balbi
Thanks for your review.

Powered by blists - more mailing lists