Message-ID: <55515FFC.5050601@linux.intel.com>
Date:	Tue, 12 May 2015 10:05:48 +0800
From:	"Lu, Baolu" <baolu.lu@...ux.intel.com>
To:	Alan Stern <stern@...land.harvard.edu>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Mathias Nyman <mathias.nyman@...el.com>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume



On 05/11/2015 10:25 PM, Alan Stern wrote:
> On Sat, 9 May 2015, Lu, Baolu wrote:
>
>>>> If FSC is supported,  the cached Slot, Endpoint, Stream, or other
>>>> Context information are also saved.
>>>>
>>>> Hence, when FSC is supported, software does not have to issue Stop
>>>> Endpoint Command to push public and private endpoint state into
>>>> memory as part of system suspend process.
>>> Why do you have to push this state into memory at all?  Does the
>>> controller hardware lose the cached state information when it is in low
>>> power?
>> I don't think controller hardware will lose the cached state information
>> when it is in low power. But since cache in controller consumes power
>> and resources, by pushing state into memory, hardware can power
>> off the cache logic during suspend. Hence more power saving gains.
>>
>>>> The logic in xhci_device_suspend() will look like:
>>>>
>>>> if xhci_device_suspend() callback was called due to system suspend,
>>>> (mesg.event & PM_EVENT_SUSPEND is true) and FSC is supported by
>>>> the xHC implementation, xhci_device_suspend() could ignore stop
>>>> endpoint command, since CSS will be done in xhc_suspend() later and
>>>> where all the endpoint caches will be pushed to memory.
>>> I still don't understand this.  You said earlier that according
>>> to section 4.15.1.1 of the xHCI spec, the endpoint rings should
>>> _always_ be stopped with SP set when a device is suspended.  Now you're
>> The intention of stop endpoint with SP set is to tell hardware that
>> "a device is going to suspend, hardware don't need to contain the
>> endpoint state in internal cache anymore". Hardware _could_ use
>> this hint to push endpoint state into memory to reduce power
>> consumption.
>>
>>> saying that they don't need to be stopped during a system suspend if
>>> the controller supports FSC.  (Or maybe you're saying they need to be
>>> stopped but SP doesn't need to be set -- it's hard to tell.)
>> Even FSC is supported, controller hardware still need to push cached
>> endpoint state into memory when a USB device is suspended. The
>> difference is when FSC is enforced, CSS command will push any
>> cached endpoint state into memory unconditionally.
> You said above that the hardware _could_ push endpoint state into
> memory.  Now you're saying it _needs_ to do this!  Make up your mind.

I'm sorry that I confused you.

FSC is a different thing from what this patch series does.

I should say "software could ask hardware to push endpoint
state into memory even FSC is supported". But in some cases,
it can be optimized as I will describe it below.

>
>
>> So, when xhci_device_suspend() knows that CSS command will be
>> executed later and CSS command will push cached endpoint state
>> into memory (a.k.a. FSC is enforced), it could skip issuing stop
>> endpoint command with SP bit set to avoid duplication and reduce
>> the suspend time.
>>
>> This is the case for system suspend since CSS command is part of
>> xhci_suspend() and xhci_suspend() will be executed after all USB
>> devices have been suspended. But it's not case for run-time suspend
>> (auto-pm) since USB device suspend and host controller suspend
>> are independent for run-time case.
>>
>> That's the reason why I wanted to keep 'msg' parameter. But just as
>> Greg said, we don't need to keep a parameter when it's not used
>> and can add it later when it is required.
>>
>>> So which is it?  Do you need to stop the endpoint rings?  Is it okay
>>> not to set SP?
>> "stop endpoint" and "stop endpoint with SP set" serve different purposes
>> in Linux xhci driver as my understanding. "stop endpoint" command is
>> used to stop a active ring when upper layer want to cancel a URB.
>> "stop endpoint with SP set" is used to hint hardware that a USB is going
>> to suspend. Hence "stop endpoint with SP set" must be executed in case
>> that the transfer ring is empty.
> (How does the contents of the transfer ring affect anything?  Besides,
> there are never any active URBs when a device gets suspended, so the
> transfer ring will _always_ be empty at such times.)
>
> This is still extremely confusing.  You're not doing a good job of
> explaining the situation clearly and logically.

I'm sorry for that.

>
> Let's see if I understand it correctly:
>
> 	When the controller goes into suspend, you want the cache to
> 	be pushed into memory so that the cache can be powered down,
> 	thereby saving additional energy.

Not the controller goes into suspend, but USB devices.

In order to talking to USB devices, xHCI has an endpoint state for each
endpoint of a device. When a USB device goes into suspend, xHC driver
could ask the hardware to push the state from cache to the memory.
Thereby saving additional energy. This is the intention of this patch 
series.

>
> 	If the hardware supports FSC, this will happen automatically.
>
> 	If the hardware doesn't support FSC, the cached data won't get
> 	pushed to memory unless the driver tells the controller to do
> 	so at the time the device is suspended.  But this will slow
> 	things down, so the driver should avoid doing it when it's not
> 	needed.
>
> 	During system suspend you know in advance that the controller
> 	will be suspended.  Therefore the driver should push the cache
> 	to memory if the hardware doesn't support FSC.  During runtime
> 	suspend you don't know in advance whether the controller will
> 	be suspended, so the driver should not push the cache to
> 	memory.
>
> 	But what happens in the case where all the devices have gone
> 	into runtime suspend, so the controller also goes into runtime
> 	suspend, and the hardware doesn't support FSC?  It seems that
> 	in this case the cache would have to remain powered on.

FSC is not part of this patch series. It was introduced when I tried to
explain the reason why I kept 'msg' parameter in the callback.

If the hardware support FSC, another operation named "save context
operation" will push everything in hardware cache into memory. So when
a USB device is going to suspend and xhci_device_suspend() callback is
being called, software can do an optimization. That is, if "save context
operation" will push everything in hardware cache to memory later,
xhci_device_suspend() could skip asking hardware for cache operation
and let "save context operation" do it later.

One example of this situation is system suspend. During system suspend,
below process will be done for a USB3 fabric.

1) all USB devices suspend
2) root hub suspend
3) host controller suspend

In 1), xhci_device_suspend() call back will be called for each device 
suspend.
In 3) "save context operation" will be executed.

In this case, if FSC is supported, xhci_device_suspend() could skip asking
host hardware for cache operation.

>
> Also, it's still not clear what the driver needs to do differently in
> order to push out the cached data.  You have managed to imply both:
>
> 	Issuing Stop Endpoint with SP set is mandatory whenever
> 	a device is suspended, and
>
> 	The SP bit is what tells the controller to push the endpoint
> 	state to memory.
>
> This doesn't make sense.

Software doesn't handle the cache directly. XHC driver just tell hardware
that a device is going to suspend. XHC driver does this through stop 
endpoint
command by setting the SP (suspend) bit in the command trb.

During xHC handles stop endpoint command, it will check SP bit, if it's set,
xHC knows that the endpoint state could be pushed out to memory now,
since device is going to suspend. The operation of cache is very hardware
implementation dependent.

>
> One last question: How much extra time does it take to push the cached
> data to memory?  Bear in mind that suspending a device is not a rapid
> operation; if pushing the data takes no more than a few microseconds, I
> think it would be worthwhile to do it always, even when it's not
> necessary.

It depends on how much time software spends in xhci_device_suspend().
I don't have data in hand, but I agree with you that if this time is no more
than a few microseconds, we can let do it always.

>
> Alan Stern

Thank you!
-baolu

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists