Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
On Mi, 2018-11-07 at 18:10 -0800, Thinh Nguyen wrote:
>
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
> * @setup_packet_pending: true when there's a Setup Packet in FIFO.
> Workaround
> * @three_stage_setup: set if we perform a three phase setup
> * @usb3_lpm_capable: set if hadrware supports Link Power Management
> + * @usb2_lpm_disable: set to disable usb2 lpm
> * @disable_scramble_quirk: set if we enable the disable scramble quirk
> * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
> * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
> @@ -1146,6 +1147,7 @@ struct dwc3 {
> unsignedsetup_packet_pending:1;
> unsignedthree_stage_setup:1;
> unsignedusb3_lpm_capable:1;
> + unsignedusb2_lpm_disable:1;
Hi,
that may be a bit late, but why would this be a property of dwc3?
Now, you may want to do this for a specific controller,
but there is no reason to limit the flag to dwc3. We want this
flag in the generic HCD attributes, so that other HCDs can share
it. Maybe even expose it to sysfs.
Regards
Oliver
Re: Adding NovAtel USB vendor & device ID to Kernel
On Do, 2018-11-08 at 01:07 +, SNELL James wrote: > Hello, > We produce extremely high-end GNSS (GPS, etc) receivers that are often used > for a very wide range of applications. Our receivers can be connected to via > USB, which will provide 3 USB-to-serial ports that can be used to issue > commands and get receiver data. > > We typically get Linux users to create a udev file so their systems attach > the USB serial ports to /dev. Hi, thank you for the extensive bug report. AFAICT you are unlucky enough to run into three separate but related issues 1. your udev rule does not work Your idea is basically correct, but for udev issues you are on the wrong mailing list. 2. Your company's name is wrong I looked at the most recent usb.ids http://www.linux-usb.org/usb.ids and it looks correct to me: 09d7 NovAtel Inc. 0100 NovAtel FlexPack GPS receiver Please double check this and the date of your usb.ids file 3. the kernel message > I just noticed that when my receiver enumerates, dmesg outputs: > [ 414.374523] usb 1-1.1.3: new full-speed USB device number 8 using dwc_otg > [ 414.508473] usb 1-1.1.3: New USB device found, idVendor=09d7, > idProduct=0100 > [ 414.508488] usb 1-1.1.3: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [ 414.508497] usb 1-1.1.3: Product: NovAtel GPS Receiver > [ 414.508505] usb 1-1.1.3: Manufacturer: NovAtel Inc. > [ 414.508514] usb 1-1.1.3: SerialNumber: DMGW18050122R > [ 414.511608] usbserial_generic 1-1.1.3:1.0: The "generic" usb-serial driver > is only for testing and one-off prototypes. > [ 414.511624] usbserial_generic 1-1.1.3:1.0: Tell > mailto:linux-usb@vger.kernel.org to add your device to a proper driver. > [ 414.511636] usbserial_generic 1-1.1.3:1.0: generic converter detected > [ 414.512004] usb 1-1.1.3: generic converter now attached to ttyUSB0 > [ 414.512352] usb 1-1.1.3: generic converter now attached to ttyUSB1 > [ 414.512805] usb 1-1.1.3: generic converter now attached to ttyUSB2 > Indeed the kernel ought to be patched for this. You need to tell us which driver your devices is best served by, so that your device ID can be added to the correct driver. Regards Oliver
Re: USB Bluetooth dongle stop response with timeout error
On Mi, 2018-11-07 at 13:20 +0800, Morikazu Fumita wrote: Hi, > Hello Oliver, > > I got rid of the network bridge but the timeout error still happens so I > can rule out the bridge now. Good. > I also got USB packet dump and found that the error is happening > regardless of HCI commands. > > One example is below. I just inserted the USB dongle when I got the > following errors. > [ 145.046503] Bluetooth: hci0: command 0x1002 tx timeout > [ 147.086503] Bluetooth: hci0: command 0x0c52 tx timeout > [ 149.121499] Bluetooth: hci0: command 0x0c45 tx timeout > [ 151.166503] Bluetooth: hci0: command 0x0c58 tx timeout > > Please find the USB packet dump attached. I have trouble reading this. How did you make it? What does it view with? > In frame no. 161, the host sent HCI command 0x1002 (Read Local Supported > Commands). > Then the USB dongle tried to respond to that from frame no. 163 but it > did not sent all fragment packets. It seems to stop response in the middle. > Finally, above "0x1002 tx timeout" happened. Did you try to repeat this from cold boot? > As for frame no. 165, the host sent 0x0c52 (Write Extended Inquiry > Response) but there was no response to it then above "0x0c52 tx timeout" > happened. > > This is just one example. The USB dongle can be successfully inserted > and working for a while but suddenly stops response to HCI commands like > this example. > It seems there is no specific HCI commands to cause the problem. > > I do not find out what is the trigger of it yet. > Do you have any thoughts from this point? Have you tried ruling out LPM and other runtime PM? This looks like a bluetooth issue, not specifically USB so we are kind of the wrong mailing list. There is one for Bluetooth. Regards Oliver
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
Hi,
Oliver Neukum writes:
> On Mi, 2018-11-07 at 18:10 -0800, Thinh Nguyen wrote:
>>
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
>> * @setup_packet_pending: true when there's a Setup Packet in FIFO.
>> Workaround
>> * @three_stage_setup: set if we perform a three phase setup
>> * @usb3_lpm_capable: set if hadrware supports Link Power Management
>> + * @usb2_lpm_disable: set to disable usb2 lpm
>> * @disable_scramble_quirk: set if we enable the disable scramble quirk
>> * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>> * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
>> @@ -1146,6 +1147,7 @@ struct dwc3 {
>> unsignedsetup_packet_pending:1;
>> unsignedthree_stage_setup:1;
>> unsignedusb3_lpm_capable:1;
>> + unsignedusb2_lpm_disable:1;
>
> Hi,
>
> that may be a bit late, but why would this be a property of dwc3?
> Now, you may want to do this for a specific controller,
> but there is no reason to limit the flag to dwc3. We want this
> flag in the generic HCD attributes, so that other HCDs can share
> it. Maybe even expose it to sysfs.
this is used for the peripheral side of dwc3 too.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
Hi,
Felipe Balbi writes:
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
>>> * @setup_packet_pending: true when there's a Setup Packet in FIFO.
>>> Workaround
>>> * @three_stage_setup: set if we perform a three phase setup
>>> * @usb3_lpm_capable: set if hadrware supports Link Power Management
>>> + * @usb2_lpm_disable: set to disable usb2 lpm
>>> * @disable_scramble_quirk: set if we enable the disable scramble quirk
>>> * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>>> * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
>>> @@ -1146,6 +1147,7 @@ struct dwc3 {
>>> unsignedsetup_packet_pending:1;
>>> unsignedthree_stage_setup:1;
>>> unsignedusb3_lpm_capable:1;
>>> + unsignedusb2_lpm_disable:1;
>>
>> Hi,
>>
>> that may be a bit late, but why would this be a property of dwc3?
>> Now, you may want to do this for a specific controller,
>> but there is no reason to limit the flag to dwc3. We want this
>> flag in the generic HCD attributes, so that other HCDs can share
>> it. Maybe even expose it to sysfs.
>
> this is used for the peripheral side of dwc3 too.
oh, and this is exposed through dwc3's private registers, not the
generic XHCI registers.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
On Do, 2018-11-08 at 12:47 +0200, Felipe Balbi wrote:
Hi,
> Oliver Neukum writes:
> > On Mi, 2018-11-07 at 18:10 -0800, Thinh Nguyen wrote:
> > >
> > > --- a/drivers/usb/dwc3/core.h
> > > +++ b/drivers/usb/dwc3/core.h
> > > @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
> > > * @setup_packet_pending: true when there's a Setup Packet in FIFO.
> > > Workaround
> > > * @three_stage_setup: set if we perform a three phase setup
> > > * @usb3_lpm_capable: set if hadrware supports Link Power Management
> > > + * @usb2_lpm_disable: set to disable usb2 lpm
> > > * @disable_scramble_quirk: set if we enable the disable scramble quirk
> > > * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
> > > * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
> > > @@ -1146,6 +1147,7 @@ struct dwc3 {
> > > unsignedsetup_packet_pending:1;
> > > unsignedthree_stage_setup:1;
> > > unsignedusb3_lpm_capable:1;
> > > + unsignedusb2_lpm_disable:1;
> >
> > Hi,
> >
> > that may be a bit late, but why would this be a property of dwc3?
> > Now, you may want to do this for a specific controller,
> > but there is no reason to limit the flag to dwc3. We want this
> > flag in the generic HCD attributes, so that other HCDs can share
> > it. Maybe even expose it to sysfs.
>
> this is used for the peripheral side of dwc3 too.
same argument. Whether a gadget supports LPM is a question
in no way specific to dwc3. And whether this exposes internal
registers does not really matter. It is a capability of the HC
for a generic issue.
Regards
Oliver
Re: [PATCH] HID: Add quirk for Microsoft PIXART OEM mouse
[ Benjamin CCed ]
On Wed, 7 Nov 2018, Sebastian Parschauer wrote:
> The PixArt OEM mice are known for disconnecting every minute in
> runlevel 1 or 3 if they are not always polled. So add quirk
> ALWAYS_POLL for this one as well.
>
> References:
> https://www.spinics.net/lists/linux-usb/msg88965.html
> http://linet.gr.jp/~kojima/PlamoWeb/ML/htdocs/201808/msg00019.html
>
> Signed-off-by: Sebastian Parschauer
> CC: sta...@vger.kernel.org
> ---
> drivers/hid/hid-ids.h| 1 +
> drivers/hid/hid-quirks.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index f63489c882bb..4f8104cbc7f0 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -805,6 +805,7 @@
> #define USB_DEVICE_ID_MS_TYPE_COVER_20x07a9
> #define USB_DEVICE_ID_MS_POWER_COVER 0x07da
> #define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd
> +#define USB_DEVICE_ID_MS_PIXART_MOUSE0x00cb
>
> #define USB_VENDOR_ID_MOJO 0x8282
> #define USB_DEVICE_ID_RETRO_ADAPTER 0x3201
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 52c3b01917e7..c0c2537beff1 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -108,6 +108,7 @@ static const struct hid_device_id hid_quirks[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> USB_DEVICE_ID_LOGITECH_MOUSE_C06A), HID_QUIRK_ALWAYS_POLL },
> { HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_GAMEPADBLOCK),
> HID_QUIRK_MULTI_INPUT },
> { HID_USB_DEVICE(USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS),
> HID_QUIRK_NOGET },
> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_PIXART_MOUSE), HID_QUIRK_ALWAYS_POLL },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_POWER_COVER), HID_QUIRK_NO_INIT_REPORTS },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_SURFACE_PRO_2), HID_QUIRK_NO_INIT_REPORTS },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_TOUCH_COVER_2), HID_QUIRK_NO_INIT_REPORTS },
Applied, thanks.
--
Jiri Kosina
SUSE Labs
Re: Query on usb/core/devio.c
On Wed, 24 Oct 2018 10:10:32 -0400 Alan Stern wrote: > On Wed, 24 Oct 2018, Mayuresh Kulkarni wrote: > > > On Mon, 22 Oct 2018 10:24:46 -0400 > > Alan Stern wrote: > > > > > On Mon, 22 Oct 2018, Oliver Neukum wrote: > > > > > > > On Do, 2018-10-18 at 13:42 -0400, Alan Stern wrote: > > > > > On Thu, 18 Oct 2018, Mayuresh Kulkarni wrote: > > > > > > > > > > > > The only way to make the ioctl work properly is to have it do a > > > > > > > runtime-PM put at the start and then a runtime-PM get before it > > > > > > > > If and only if you want to do this with one ioctl() > > > > If you separate the runtime-PM put and the get, you can do it without > > > > the waiting part. > > > > > > Sure, but you still need a runtime-PM put at the start and a runtime-PM > > > get at the end. In fact, if you separate this into two calls then you > > > run the risk that the user may never perform the second call, and you > > > would end up with an unbalanced put. > > > > > > > I am tending to agree towards having a single ioctl call here. It is better > > to give minimal control to user-space w.r.t. runtime PM. The current > > proposal of user-space giving an hint to USB-FS/core that - it is not using > > the device, sounds better. > > We reviewed the proposal internally and looks like we have following cases: 1. Host wake from L2: This needs to be done when end-user wants to interact with device && device is in suspend/link in L2. 2. Remote wake from L2 by device: This happens when USB device detects some activity while link was in L2. As per my understanding, the current ioctl proposal seems to address (2) above. But (1) seems to have issue which is, the end-user has to wait for remote-wake from device to interact with it (since device is already open and blocked in new ioctl). With that said, it looks like we would need 3 ioctls as below: 1. suspend: to be called by user-space when it knows it is done using the device. Should be non-blocking. It should just drop the PM ref-count on device. 2. resume: to be called by user-space when it wants to actively interact with the device. Should be non-blocking. It should just get the PM ref-count on device. 3. wait-for-remote-wake: to be called by user-space when it wants to wait for remote wake of device && end user not actively use it. Should be blocking. Unblocked by resume. It should have different return value to indicate resume-by-host or resume-by-remote-wake (apart from signal info). Does this sound reasonable (considering one of my previous comments :-/)? > > > > > > /* > > > > > > * There are 3 possibilities here: > > > > > > * 1. Device did suspend and resume (success) > > > > > > * 2. Signal was received (failed suspend) > > > > > > * 3. Time-out happened (failed suspend) > > > > > > > > > > 4. Device did suspend but a signal was received before the device > > > > > resumed. > > > > > > > > > > > * In any of above cases, we need to resume device. > > > > > > */ > > > > > > usb_autoresume_device(dev); > > > > > > > > Yes and that is the problem. Why do you want to wait for the result > > > > of runtime-PM put ? If we need a channel for notifying user space > > > > about resume of a device, why wait for the result of suspend instead > > > > of using the same channel? > > > > > > This is not meant to be a general-purpose channel for notifying > > > userspace when a device resumes. Such a channel should be defined in > > > the runtime-PM layer, not in the USB layer. > > > > > > This is instead meant to be a special-purpose mechanism for adding a > > > runtime-suspend/resume interface to usbfs. > > > > > > > Just to be clear here - the worst case wait-time from user-space > > perspective will be + , right? > > That's right. > > > If yes then, timeout argument means "wait at-least timeout sec". > > Yes. Unless for a few unlikely cases, including: > > A signal is received before the timeout expires; > > The device suspends and then resumes before the timeout > expires. > > > On Wed, 24 Oct 2018, Mayuresh Kulkarni wrote: > > > We spend time internally to go over the "new" ioctl proposal. Overall > > it looks promising. > > > > However, we still have an issue as below - > > Consider a use-case where, user-space calls "new" ioctl, but suspend > > never happen (for various reasons) && async event happens on USB > > device side by the end-user. > > > > In such a case, since user-space is waiting in "new" ioctl, it is not > > in position to queue a request to read-out the async event info. It > > will be able to queue a request when the "new" ioctl returns which > > will be "time-out" later (in this case). Due to auto-suspend > > time-out's default's value of 2 sec, the user-space has to choose the > > time-out to "new" ioctl > 2 sec. > > Not so. That "2 second" value can be adjusted by the user; it can be > reduced to as little as 1 ms. > > Alan Stern
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
Hi,
Oliver Neukum writes:
>> > > --- a/drivers/usb/dwc3/core.h
>> > > +++ b/drivers/usb/dwc3/core.h
>> > > @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
>> > > * @setup_packet_pending: true when there's a Setup Packet in FIFO.
>> > > Workaround
>> > > * @three_stage_setup: set if we perform a three phase setup
>> > > * @usb3_lpm_capable: set if hadrware supports Link Power Management
>> > > + * @usb2_lpm_disable: set to disable usb2 lpm
>> > > * @disable_scramble_quirk: set if we enable the disable scramble quirk
>> > > * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>> > > * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
>> > > @@ -1146,6 +1147,7 @@ struct dwc3 {
>> > > unsignedsetup_packet_pending:1;
>> > > unsignedthree_stage_setup:1;
>> > > unsignedusb3_lpm_capable:1;
>> > > + unsignedusb2_lpm_disable:1;
>> >
>> > Hi,
>> >
>> > that may be a bit late, but why would this be a property of dwc3?
>> > Now, you may want to do this for a specific controller,
>> > but there is no reason to limit the flag to dwc3. We want this
>> > flag in the generic HCD attributes, so that other HCDs can share
>> > it. Maybe even expose it to sysfs.
>>
>> this is used for the peripheral side of dwc3 too.
>
> same argument. Whether a gadget supports LPM is a question
> in no way specific to dwc3. And whether this exposes internal
> registers does not really matter. It is a capability of the HC
> for a generic issue.
for the gadget api we don't have enough UDCs supporting LPM to design a
proper generic api. Until then, it's best to keep this private to the
driver otherwise we may make the wrong decisions in the generic layer.
--
balbi
signature.asc
Description: PGP signature
Re: Query on usb/core/devio.c
On Thu, 8 Nov 2018, Mayuresh Kulkarni wrote: > On Wed, 24 Oct 2018 10:10:32 -0400 > Alan Stern wrote: > > > On Wed, 24 Oct 2018, Mayuresh Kulkarni wrote: > > > > > On Mon, 22 Oct 2018 10:24:46 -0400 > > > Alan Stern wrote: > > > > > > > On Mon, 22 Oct 2018, Oliver Neukum wrote: > > > > > > > > > On Do, 2018-10-18 at 13:42 -0400, Alan Stern wrote: > > > > > > On Thu, 18 Oct 2018, Mayuresh Kulkarni wrote: > > > > > > > > > > > > > > The only way to make the ioctl work properly is to have it do a > > > > > > > > runtime-PM put at the start and then a runtime-PM get before it > > > > > > > > > > If and only if you want to do this with one ioctl() > > > > > If you separate the runtime-PM put and the get, you can do it without > > > > > the waiting part. > > > > > > > > Sure, but you still need a runtime-PM put at the start and a runtime-PM > > > > get at the end. In fact, if you separate this into two calls then you > > > > run the risk that the user may never perform the second call, and you > > > > would end up with an unbalanced put. > > > > > > > > > > I am tending to agree towards having a single ioctl call here. It is > > > better > > > to give minimal control to user-space w.r.t. runtime PM. The current > > > proposal of user-space giving an hint to USB-FS/core that - it is not > > > using > > > the device, sounds better. > > > > > We reviewed the proposal internally and looks like we have following cases: > 1. Host wake from L2: This needs to be done when end-user wants to interact > with > device && device is in suspend/link in L2. > 2. Remote wake from L2 by device: This happens when USB device detects some > activity while link was in L2. > > As per my understanding, the current ioctl proposal seems to address (2) > above. > But (1) seems to have issue which is, the end-user has to wait for remote-wake > from device to interact with it (since device is already open and blocked in > new ioctl). This conclusion does not follow. In fact, we have two choices. An access while the device is suspended could be forced to fail -- this could cause unexpected errors on unrelated threads if they happen to share the same file descriptor for the device. Or an access while the device is suspended could first cause it to wake up, terminating the ioctl call -- in which case there is no problem, right? Alternatively, the user can simply send a signal to the thread that is blocked in the ioctl; that will terminate the ioctl call and allow the interaction to proceed. > With that said, it looks like we would need 3 ioctls as below: > 1. suspend: to be called by user-space when it knows it is done using the > device. Should be non-blocking. It should just drop the PM ref-count on > device. > 2. resume: to be called by user-space when it wants to actively interact with > the device. Should be non-blocking. It should just get the PM ref-count on > device. > 3. wait-for-remote-wake: to be called by user-space when it wants to wait for > remote wake of device && end user not actively use it. Should be blocking. > Unblocked by resume. It should have different return value to indicate > resume-by-host or resume-by-remote-wake (apart from signal info). The driver has no way to tell whether the resume was caused by the host or by the device. (In fact, it's possible for a resume to be caused by _both_ the host and the device, if they request it at the same time.) In the end, it doesn't matter anyway. > Does this sound reasonable (considering one of my previous comments :-/)? Doesn't (3) suffer from the same problem as (1) in the first list above? Alan Stern
Re: [PATCH 1/2] usb: gadget: Add start_frame to usb_request
Hi,
On 11/7/2018 11:03 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen writes:
>> Similar to URB's start_frame, add a field start_frame to the usb_request
>> to report the scheduled (micro)frame number of an isochronous transfer.
>> This option is useful for debugging purposes.
>>
>> Signed-off-by: Thinh Nguyen
>> ---
>> include/linux/usb/gadget.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e5cd84a0f84a..ed9dbbce55ee 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -50,6 +50,7 @@ struct usb_ep;
>> * @short_not_ok: When reading data, makes short packets be
>> * treated as errors (queue stops advancing till cleanup).
>> * @dma_mapped: Indicates if request has been mapped to DMA (internal)
>> + * @start_frame: the reported (micro)frame of the scheduled isoc transfer
>> * @complete: Function called when request completes, so this request and
>> * its buffer may be re-used. The function will always be called with
>> * interrupts disabled, and it must not sleep.
>> @@ -107,6 +108,8 @@ struct usb_request {
>> unsignedshort_not_ok:1;
>> unsigneddma_mapped:1;
>>
>> +int start_frame;/* ISO ONLY */
> this name is a bit misleading. I can see functions trying to use it to
> request that a particular request start on a specific frame. Would
> "started_frame" be a better name, perhaps?
>
Sure. That's more accurate.
Thinh
Re: [PATCH v2 2/2] usb: dwc3: gadget: Report isoc scheduled frame number
Hi, On 11/7/2018 11:08 PM, Felipe Balbi wrote: > Hi, > > Thinh Nguyen writes: >> Report the scheduled frame number of an isochronous request. >> >> Signed-off-by: Thinh Nguyen >> --- >> Change in v2: >> - Capture frame number at request cleanup >> >> drivers/usb/dwc3/gadget.c | 4 >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 679c12e14522..5e5e799699de 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2345,6 +2345,10 @@ static int >> dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, >> >> req->request.actual = req->request.length - req->remaining; >> >> +/* Report scheduled frame number for isoc transfers */ >> +if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) >> +req->request.start_frame = dep->frame_number; > yeah, this is really wrong. Since you're setting start_frame when > cleaning up the request, this means that dep->frame_number was already > updated due to XferComplete/XferInProgress. This means thart start_frame > will report frame number of completion event, not start. So your > debugging will be misleading. > You're right. I'll make the fix. Thinh
Re: [PATCH v5 1/3] usb: dwc3: Track DWC_usb31 VERSIONTYPE
Hi Felipe,
On 11/7/2018 11:10 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen writes:
>> Add a new field to dwc3 structure to track VERSIONTYPE. The VERSIONTYPE
>> is represented in ASCII in the 32-bit VERSIONTYPE register. In
>> DWC_usb31, sub releases for each version are tracked with VERSIONTYPE
>> such as "ea01" and "ea02".
>>
>> Signed-off-by: Thinh Nguyen
>> ---
>> drivers/usb/dwc3/core.c | 1 +
>> drivers/usb/dwc3/core.h | 5 +
>> 2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index becfbb87f791..437816ff8860 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -702,6 +702,7 @@ static bool dwc3_core_is_valid(struct dwc3 *dwc)
>> /* Detected DWC_usb31 IP */
>> dwc->revision = dwc3_readl(dwc->regs, DWC3_VER_NUMBER);
>> dwc->revision |= DWC3_REVISION_IS_DWC31;
>> +dwc->version_type = dwc3_readl(dwc->regs, DWC3_VER_TYPE);
>> } else {
>> return false;
>> }
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 5bfb62533e0f..4573e1bfd56e 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -918,6 +918,7 @@ struct dwc3_scratchpad_array {
>> * @u1u2: only used on revisions <1.83a for workaround
>> * @maximum_speed: maximum speed requested (mainly for testing purposes)
>> * @revision: revision register contents
>> + * @version_type: VERSIONTYPE register contents, a sub release of a revision
>> * @dr_mode: requested mode of operation
>> * @current_dr_role: current role of operation when in dual-role mode
>> * @desired_dr_role: desired role of operation when in dual-role mode
>> @@ -1104,6 +1105,10 @@ struct dwc3 {
>> #define DWC3_USB31_REVISION_110A(0x3131302a | DWC3_REVISION_IS_DWC31)
>> #define DWC3_USB31_REVISION_120A(0x3132302a | DWC3_REVISION_IS_DWC31)
>>
>> +u32 version_type;
>> +
>> +#define DWC31_VERSIONTYPE_EA01 0x65613031
> on patch 2 you talk about ea06, why don't you have a define for all
> those version types?
>
>
Sure. I can do that.
Thinh
Re: [PATCH 1/3] usb: dwc3: Add reference clock properties
Hi Felipe, On 11/7/2018 11:17 PM, Felipe Balbi wrote: > Hi, > > Thinh Nguyen writes: >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >> b/Documentation/devicetree/bindings/usb/dwc3.txt >> index 636630fb92d7..712b344c3a31 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -95,6 +95,24 @@ Optional properties: >> this and tx-thr-num-pkt-prd to a valid, >> non-zero value >> 1-16 (DWC_usb31 programming guide section >> 1.2.3) to >> enable periodic ESS TX threshold. >> + - snps,refclk-period-ns: set to program the reference clock period. >> The valid >> +input periods are as follow: >> ++-+-+ >> +| Period (ns) | Freq (MHz) | >> ++-+-+ >> +| 25 | 39.7/40 | >> +| 41 | 24.4| >> +| 50 | 20 | >> +| 52 | 19.2| >> +| 58 | 17.2| >> +| 62 | 16.1| >> ++-+-+ >> + - snps,enable-refclk-lpm: set to enable low power scheduling of >> isochronous >> +transfers by running SOF/ITP counters using the >> +reference clock. Only valid for DWC_usb31 >> peripheral >> +controller v1.80a and higher. Both >> +"snps,dis_u2_susphy_quirk" and >> +"snps,dis_enblslpm_quirk" must not be set. > sounds like you should rely on clk API here. Then on driver call > clk_get_rate() to computer whatever you need to compute. > There's nothing to compute here. We can simply enable this feature with "snps, enable-refclk-lpm" and the controller will use the default refclk settings. >>> Right, right. What I'm saying, though, is that we have a clock API for >>> describing a clock. So why wouldn't we rely on that API for this? I >>> think both of these new properties can be replaced with standard clock >>> API properties: >>> >>> clocks = <&clk1>, ..., <&lpm_clk> >>> clock-names = "clock1", , "lpm"; >>> >>> Then dwc3 core could, simply, check if we have a clock named "lpm" and >>> if there is, use NSECS_PER_SEC / clk_get_rate() to get its period and >>> write it to the register that needs the information. >> There's no new clock here. We are using the ref_clk for SOF and ITP >> counter for this feature. Also, clocks are optional on non-DT platforms. >> To use the clock API, then we need to update the driver to allow some >> optional clock such as "ref" clock for non-DT platforms. Do you want to >> do it like this? > I can't think of a problem that would arise from that. Can you? Mark, > Rob, what do you think? > No problem. That can be done. This will remove the "snps,refclk-period-ns" property. But we should have "snps,enable-refclk-lpm" to enable this feature. Thinh
[PATCH v6 1/3] usb: dwc3: Track DWC_usb31 VERSIONTYPE
Add a new field to dwc3 structure to track VERSIONTYPE. The VERSIONTYPE
is represented in ASCII in the 32-bit VERSIONTYPE register. In
DWC_usb31, sub releases for each version are tracked with VERSIONTYPE
such as "ea01" and "ea02".
Signed-off-by: Thinh Nguyen
---
drivers/usb/dwc3/core.c | 1 +
drivers/usb/dwc3/core.h | 10 ++
2 files changed, 11 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index becfbb87f791..437816ff8860 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -702,6 +702,7 @@ static bool dwc3_core_is_valid(struct dwc3 *dwc)
/* Detected DWC_usb31 IP */
dwc->revision = dwc3_readl(dwc->regs, DWC3_VER_NUMBER);
dwc->revision |= DWC3_REVISION_IS_DWC31;
+ dwc->version_type = dwc3_readl(dwc->regs, DWC3_VER_TYPE);
} else {
return false;
}
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5bfb62533e0f..a2de686c5460 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -918,6 +918,7 @@ struct dwc3_scratchpad_array {
* @u1u2: only used on revisions <1.83a for workaround
* @maximum_speed: maximum speed requested (mainly for testing purposes)
* @revision: revision register contents
+ * @version_type: VERSIONTYPE register contents, a sub release of a revision
* @dr_mode: requested mode of operation
* @current_dr_role: current role of operation when in dual-role mode
* @desired_dr_role: desired role of operation when in dual-role mode
@@ -1104,6 +1105,15 @@ struct dwc3 {
#define DWC3_USB31_REVISION_110A (0x3131302a | DWC3_REVISION_IS_DWC31)
#define DWC3_USB31_REVISION_120A (0x3132302a | DWC3_REVISION_IS_DWC31)
+ u32 version_type;
+
+#define DWC31_VERSIONTYPE_EA01 0x65613031
+#define DWC31_VERSIONTYPE_EA02 0x65613032
+#define DWC31_VERSIONTYPE_EA03 0x65613033
+#define DWC31_VERSIONTYPE_EA04 0x65613034
+#define DWC31_VERSIONTYPE_EA05 0x65613035
+#define DWC31_VERSIONTYPE_EA06 0x65613036
+
enum dwc3_ep0_next ep0_next_event;
enum dwc3_ep0_state ep0state;
enum dwc3_link_statelink_state;
--
2.11.0
[PATCH v6 2/3] usb: dwc3: Add disabling of start_transfer failure quirk
DWC_usb31 peripheral v1.70a-ea06 and prior needs a SW workaround for isoc START TRANSFER command failure. However, some affected versions may have RTL patches to fix this without a SW workaround. Add this quirk to disable the SW workaround when it is not needed. Synopsys STAR 9001202023: Wrong microframe number for isochronous IN endpoints. Signed-off-by: Thinh Nguyen --- Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt index 636630fb92d7..fc64db0a7a0a 100644 --- a/Documentation/devicetree/bindings/usb/dwc3.txt +++ b/Documentation/devicetree/bindings/usb/dwc3.txt @@ -37,6 +37,9 @@ Optional properties: - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy" or "usb3-phy". - resets: a single pair of phandle and reset specifier + - snps,dis-start-transfer-quirk: when set, disable isoc START TRANSFER command + failure SW work-around for DWC_usb31 version 1.70a-ea06 + and prior. - snps,usb3_lpm_capable: determines if platform is USB3 LPM capable - snps,disable_scramble_quirk: true when SW should disable data scrambling. Only really useful for FPGA builds. -- 2.11.0
[PATCH v6 3/3] usb: dwc3: Add workaround for isoc start transfer failure
In DWC_usb31 version 1.70a-ea06 and prior, for highspeed and fullspeed
isochronous IN, BIT[15:14] of the 16-bit microframe number reported by
the XferNotReady event are invalid. The driver uses this number to
schedule the isochronous transfer and passes it to the START TRANSFER
command. Because this number is invalid, the command may fail. If
BIT[15:14] matches the internal 16-bit microframe, the START TRANSFER
command will pass and the transfer will start at the scheduled time, if
it is off by 1, the command will still pass, but the transfer will start
2 seconds in the future. For all other conditions, the START TRANSFER
command will fail with bus-expiry.
In order to workaround this issue, we can test for the correct
combination of BIT[15:14] by sending START TRANSFER commands with
different values of BIT[15:14]: 'b00, 'b01, 'b10, and 'b11. Each
combination is 2^14 uframe apart (or 2 seconds). 4 seconds into the
future will result in a bus-expiry status. As the result, within the 4
possible combinations for BIT[15:14], there will be 2 successful and 2
failure START COMMAND status. One of the 2 successful command status
will result in a 2-second delay start. The smaller BIT[15:14] value is
the correct combination.
Since there are only 4 outcomes and the results are ordered, we can
simply test 2 START TRANSFER commands with BIT[15:14] combinations 'b00
and 'b01 to deduce the smaller successful combination.
Let test0 = test status for combination 'b00 and test1 = test status for
'b01 of BIT[15:14]. The correct combination is as follow:
if test0 fails and test1 passes, BIT[15:14] is 'b01
if test0 fails and test1 fails, BIT[15:14] is 'b10
if test0 passes and test1 fails, BIT[15:14] is 'b11
if test0 passes and test1 passes, BIT[15:14] is 'b00
Synopsys STAR 9001202023: Wrong microframe number for isochronous IN
endpoints.
Signed-off-by: Thinh Nguyen
---
drivers/usb/dwc3/core.c | 2 +
drivers/usb/dwc3/core.h | 13 +
drivers/usb/dwc3/gadget.c | 131 ++
3 files changed, 146 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 437816ff8860..d8cb2b1342d8 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1245,6 +1245,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
"snps,is-utmi-l1-suspend");
device_property_read_u8(dev, "snps,hird-threshold",
&hird_threshold);
+ dwc->dis_start_transfer_quirk = device_property_read_bool(dev,
+ "snps,dis-start-transfer-quirk");
dwc->usb3_lpm_capable = device_property_read_bool(dev,
"snps,usb3_lpm_capable");
device_property_read_u8(dev, "snps,rx-thr-num-pkt-prd",
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index a2de686c5460..01742ad5e9ed 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -656,6 +656,10 @@ struct dwc3_event_buffer {
* @name: a human readable name e.g. ep1out-bulk
* @direction: true for TX, false for RX
* @stream_capable: true when streams are enabled
+ * @combo_num: the test combination BIT[15:14] of the frame number to test
+ * isochronous START TRANSFER command failure workaround
+ * @start_cmd_status: the status of testing START TRANSFER command with
+ * combo_num = 'b00
*/
struct dwc3_ep {
struct usb_ep endpoint;
@@ -705,6 +709,10 @@ struct dwc3_ep {
unsigneddirection:1;
unsignedstream_capable:1;
+
+ /* For isochronous START TRANSFER workaround only */
+ u8 combo_num;
+ int start_cmd_status;
};
enum dwc3_phy {
@@ -971,6 +979,8 @@ struct dwc3_scratchpad_array {
* @pullups_connected: true when Run/Stop bit is set
* @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
* @three_stage_setup: set if we perform a three phase setup
+ * @dis_start_transfer_quirk: set if start_transfer failure SW workaround is
+ * not needed for DWC_usb31 version 1.70a-ea06 and below
* @usb3_lpm_capable: set if hadrware supports Link Power Management
* @disable_scramble_quirk: set if we enable the disable scramble quirk
* @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
@@ -1104,6 +1114,8 @@ struct dwc3 {
#define DWC3_REVISION_IS_DWC31 0x8000
#define DWC3_USB31_REVISION_110A (0x3131302a | DWC3_REVISION_IS_DWC31)
#define DWC3_USB31_REVISION_120A (0x3132302a | DWC3_REVISION_IS_DWC31)
+#define DWC3_USB31_REVISION_160A (0x3136302a | DWC3_REVISION_IS_DWC31)
+#define DWC3_USB31_REVISION_170A (0x3137302a | DWC3_REVISION_IS_DWC31)
u32 version_type;
@@ -1155,6 +1167,7 @@ struct dwc3 {
unsignedpullups_connected:1;
unsignedsetup_packet_pending:1;
un
[PATCH v6 0/3] usb: dwc3: Workaround isoc start_transfer failure
DWC_usb31 peripheral v1.70a-ea06 and prior needs a SW workaround for isoc START TRANSFER command failure. This patch series implements that workaround Change in v6: - Defined more version types in "usb: dwc3: Track DWC_usb31 VERSIONTYPE" - Minor cleanup/fix in "usb: dwc3: Add workaround for isoc start transfer failure" Change in v5: - Splitted and resent from an old patch series - Cleanup and fixed review issues Change in v4: - None Change in v3: - None Change in v2: - None Thinh Nguyen (3): usb: dwc3: Track DWC_usb31 VERSIONTYPE usb: dwc3: Add disabling of start_transfer failure quirk usb: dwc3: Add workaround for isoc start transfer failure Documentation/devicetree/bindings/usb/dwc3.txt | 3 + drivers/usb/dwc3/core.c| 3 + drivers/usb/dwc3/core.h| 23 + drivers/usb/dwc3/gadget.c | 131 + 4 files changed, 160 insertions(+) -- 2.11.0
Re: [PATCH 4/4] usb: dwc3: gadget: check if dep->frame_number is still valid
Hi Felipe,
On 11/7/2018 10:58 PM, Felipe Balbi wrote:
> Gadget driver may take an unbounded amount of time to queue requests
> after XferNotReady. This is important for isochronous endpoints which
> need to be started for a specific (micro-)frame.
>
> Before kicking the transfer, let's check how much time has elapsed
> since dep->frame_number was updated and make sure we start the request
> to the next valid interval.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/dwc3/core.h | 5 +
> drivers/usb/dwc3/gadget.c | 11 +++
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 131028501752..306a2dd75ed5 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -651,6 +651,7 @@ struct dwc3_event_buffer {
> * @number: endpoint number (1 - 15)
> * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
> * @resource_index: Resource transfer index
> + * @frame_timestamp: timestamp of most recent frame number
> * @frame_number: set to the frame number we want this transfer to start
> (ISOC)
> * @interval: the interval on which the ISOC transfer is started
> * @name: a human readable name e.g. ep1out-bulk
> @@ -697,7 +698,11 @@ struct dwc3_ep {
> u8 number;
> u8 type;
> u8 resource_index;
> +
> + u64 frame_timestamp;
> u32 frame_number;
> +#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
> +
> u32 interval;
>
> charname[20];
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index d8c7ad0c22e8..00fe01a01977 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1268,12 +1268,22 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>
> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> {
> + u64 current_timestamp;
> + u64 diff_timestamp;
> + u32 elapsed_frames;
> +
> if (list_empty(&dep->pending_list)) {
> dep->flags |= DWC3_EP_PENDING_REQUEST;
> return -EAGAIN;
> }
>
> + current_timestamp = ktime_get_ns();
> + diff_timestamp = current_timestamp - dep->frame_timestamp;
> + elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
> +
> + dep->frame_number += elapsed_frames;
> dep->frame_number = DWC3_ALIGN_FRAME(dep);
> +
> return __dwc3_gadget_kick_transfer(dep);
> }
>
> @@ -2320,6 +2330,7 @@ static void
> dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
> const struct dwc3_event_depevt *event)
> {
> dep->frame_number = event->parameters;
> + dep->frame_timestamp = ktime_get_ns();
> }
>
> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
This may not be enough. The dep->frame_timestamp may not correspond to
the frame_number from XferNotReady event. When there's system latency
(which is possible when this failure happens), the time the driver
handle the event may be a few uframes passed the time the controller's
XferInProgress uframe parameter.
Rather than starting the isoc transfer immediately on the next interval.
How about starting the transfer with some minimum buffer uframes just
like before? (e.g. frame_number + max(4, interval))
Thinh
Re: [PATCH 4/4] usb: dwc3: gadget: check if dep->frame_number is still valid
Hi,
Thinh Nguyen writes:
> On 11/7/2018 10:58 PM, Felipe Balbi wrote:
>> Gadget driver may take an unbounded amount of time to queue requests
>> after XferNotReady. This is important for isochronous endpoints which
>> need to be started for a specific (micro-)frame.
>>
>> Before kicking the transfer, let's check how much time has elapsed
>> since dep->frame_number was updated and make sure we start the request
>> to the next valid interval.
>>
>> Signed-off-by: Felipe Balbi
>> ---
>> drivers/usb/dwc3/core.h | 5 +
>> drivers/usb/dwc3/gadget.c | 11 +++
>> 2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 131028501752..306a2dd75ed5 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -651,6 +651,7 @@ struct dwc3_event_buffer {
>> * @number: endpoint number (1 - 15)
>> * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
>> * @resource_index: Resource transfer index
>> + * @frame_timestamp: timestamp of most recent frame number
>> * @frame_number: set to the frame number we want this transfer to start
>> (ISOC)
>> * @interval: the interval on which the ISOC transfer is started
>> * @name: a human readable name e.g. ep1out-bulk
>> @@ -697,7 +698,11 @@ struct dwc3_ep {
>> u8 number;
>> u8 type;
>> u8 resource_index;
>> +
>> +u64 frame_timestamp;
>> u32 frame_number;
>> +#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
>> +
>> u32 interval;
>>
>> charname[20];
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index d8c7ad0c22e8..00fe01a01977 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1268,12 +1268,22 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>
>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>> {
>> +u64 current_timestamp;
>> +u64 diff_timestamp;
>> +u32 elapsed_frames;
>> +
>> if (list_empty(&dep->pending_list)) {
>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>> return -EAGAIN;
>> }
>>
>> +current_timestamp = ktime_get_ns();
>> +diff_timestamp = current_timestamp - dep->frame_timestamp;
>> +elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>> +
>> +dep->frame_number += elapsed_frames;
>> dep->frame_number = DWC3_ALIGN_FRAME(dep);
>> +
>> return __dwc3_gadget_kick_transfer(dep);
>> }
>>
>> @@ -2320,6 +2330,7 @@ static void
>> dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
>> const struct dwc3_event_depevt *event)
>> {
>> dep->frame_number = event->parameters;
>> +dep->frame_timestamp = ktime_get_ns();
>> }
>>
>> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>
> This may not be enough. The dep->frame_timestamp may not correspond to
> the frame_number from XferNotReady event. When there's system latency
> (which is possible when this failure happens), the time the driver
> handle the event may be a few uframes passed the time the controller's
> XferInProgress uframe parameter.
>
> Rather than starting the isoc transfer immediately on the next interval.
> How about starting the transfer with some minimum buffer uframes just
> like before? (e.g. frame_number + max(4, interval))
The problem with this is cases with interval of 1ms. This will result in
a 4ms delay. I really want to start transfer as soon as possible and the
timestamp trick seems to be the best idea so far, without resorting to
4 intervals delay. We do, however, have the possibility that this will
start 2 intervals in the future because of the usage of
DIV_ROUND_UP_ULL() and because of how DWC3_ALIGN_FRAME() is implemented.
I understand what you're saying, though, but it seems like we don't
have to avoid that case completely. We can only make it less likely.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/3] usb: dwc3: Add reference clock properties
Hi, Thinh Nguyen writes: >> Thinh Nguyen writes: >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >>> b/Documentation/devicetree/bindings/usb/dwc3.txt >>> index 636630fb92d7..712b344c3a31 100644 >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>> @@ -95,6 +95,24 @@ Optional properties: >>> this and tx-thr-num-pkt-prd to a valid, >>> non-zero value >>> 1-16 (DWC_usb31 programming guide section >>> 1.2.3) to >>> enable periodic ESS TX threshold. >>> + - snps,refclk-period-ns: set to program the reference clock period. >>> The valid >>> + input periods are as follow: >>> + +-+-+ >>> + | Period (ns) | Freq (MHz) | >>> + +-+-+ >>> + | 25 | 39.7/40 | >>> + | 41 | 24.4| >>> + | 50 | 20 | >>> + | 52 | 19.2| >>> + | 58 | 17.2| >>> + | 62 | 16.1| >>> + +-+-+ >>> + - snps,enable-refclk-lpm: set to enable low power scheduling of >>> isochronous >>> + transfers by running SOF/ITP counters using the >>> + reference clock. Only valid for DWC_usb31 >>> peripheral >>> + controller v1.80a and higher. Both >>> + "snps,dis_u2_susphy_quirk" and >>> + "snps,dis_enblslpm_quirk" must not be set. >> sounds like you should rely on clk API here. Then on driver call >> clk_get_rate() to computer whatever you need to compute. >> > There's nothing to compute here. We can simply enable this feature with > "snps, enable-refclk-lpm" and the controller will use the default refclk > settings. Right, right. What I'm saying, though, is that we have a clock API for describing a clock. So why wouldn't we rely on that API for this? I think both of these new properties can be replaced with standard clock API properties: clocks = <&clk1>, ..., <&lpm_clk> clock-names = "clock1", , "lpm"; Then dwc3 core could, simply, check if we have a clock named "lpm" and if there is, use NSECS_PER_SEC / clk_get_rate() to get its period and write it to the register that needs the information. >>> There's no new clock here. We are using the ref_clk for SOF and ITP >>> counter for this feature. Also, clocks are optional on non-DT platforms. >>> To use the clock API, then we need to update the driver to allow some >>> optional clock such as "ref" clock for non-DT platforms. Do you want to >>> do it like this? >> I can't think of a problem that would arise from that. Can you? Mark, >> Rob, what do you think? >> > No problem. That can be done. This will remove the > "snps,refclk-period-ns" property. But we should have > "snps,enable-refclk-lpm" to enable this feature. not really. Just check if you have a clock named lpm. If you do, then you enable the feature. -- balbi signature.asc Description: PGP signature
Re: [PATCH 4/4] usb: dwc3: gadget: check if dep->frame_number is still valid
Hi Felipe,
On 11/8/2018 11:11 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen writes:
>> On 11/7/2018 10:58 PM, Felipe Balbi wrote:
>>> Gadget driver may take an unbounded amount of time to queue requests
>>> after XferNotReady. This is important for isochronous endpoints which
>>> need to be started for a specific (micro-)frame.
>>>
>>> Before kicking the transfer, let's check how much time has elapsed
>>> since dep->frame_number was updated and make sure we start the request
>>> to the next valid interval.
>>>
>>> Signed-off-by: Felipe Balbi
>>> ---
>>> drivers/usb/dwc3/core.h | 5 +
>>> drivers/usb/dwc3/gadget.c | 11 +++
>>> 2 files changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 131028501752..306a2dd75ed5 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -651,6 +651,7 @@ struct dwc3_event_buffer {
>>> * @number: endpoint number (1 - 15)
>>> * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
>>> * @resource_index: Resource transfer index
>>> + * @frame_timestamp: timestamp of most recent frame number
>>> * @frame_number: set to the frame number we want this transfer to start
>>> (ISOC)
>>> * @interval: the interval on which the ISOC transfer is started
>>> * @name: a human readable name e.g. ep1out-bulk
>>> @@ -697,7 +698,11 @@ struct dwc3_ep {
>>> u8 number;
>>> u8 type;
>>> u8 resource_index;
>>> +
>>> + u64 frame_timestamp;
>>> u32 frame_number;
>>> +#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
>>> +
>>> u32 interval;
>>>
>>> charname[20];
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index d8c7ad0c22e8..00fe01a01977 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1268,12 +1268,22 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>>
>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>> {
>>> + u64 current_timestamp;
>>> + u64 diff_timestamp;
>>> + u32 elapsed_frames;
>>> +
>>> if (list_empty(&dep->pending_list)) {
>>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>>> return -EAGAIN;
>>> }
>>>
>>> + current_timestamp = ktime_get_ns();
>>> + diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> + elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>> +
>>> + dep->frame_number += elapsed_frames;
>>> dep->frame_number = DWC3_ALIGN_FRAME(dep);
>>> +
>>> return __dwc3_gadget_kick_transfer(dep);
>>> }
>>>
>>> @@ -2320,6 +2330,7 @@ static void
>>> dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
>>> const struct dwc3_event_depevt *event)
>>> {
>>> dep->frame_number = event->parameters;
>>> + dep->frame_timestamp = ktime_get_ns();
>>> }
>>>
>>> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>> This may not be enough. The dep->frame_timestamp may not correspond to
>> the frame_number from XferNotReady event. When there's system latency
>> (which is possible when this failure happens), the time the driver
>> handle the event may be a few uframes passed the time the controller's
>> XferInProgress uframe parameter.
>>
>> Rather than starting the isoc transfer immediately on the next interval.
>> How about starting the transfer with some minimum buffer uframes just
>> like before? (e.g. frame_number + max(4, interval))
> The problem with this is cases with interval of 1ms. This will result in
> a 4ms delay. I really want to start transfer as soon as possible and the
> timestamp trick seems to be the best idea so far, without resorting to
> 4 intervals delay. We do, however, have the possibility that this will
> start 2 intervals in the future because of the usage of
> DIV_ROUND_UP_ULL() and because of how DWC3_ALIGN_FRAME() is implemented.
>
> I understand what you're saying, though, but it seems like we don't
> have to avoid that case completely. We can only make it less likely.
>
In the case of interval of 1ms, it will start on the next interval.
frame_number + max(4, interval) will start at least 4 uframes in the future.
In any case, what about immediately retry the START_TRANSFER command
with a new frame_number + (interval*retry) should it fail with
bus-expiry? You can set the number of retries to maybe 5 times. This
should remove the need to do time stamping.
Thinh
Re: USB Bluetooth dongle stop response with timeout error
Hello Oliver, Thank you for the reply again. On 8/11/2018 6:30 PM, Oliver Neukum wrote: > On Mi, 2018-11-07 at 13:20 +0800, Morikazu Fumita wrote: > > Hi, > >> Hello Oliver, >> >> I got rid of the network bridge but the timeout error still happens so I >> can rule out the bridge now. > > Good. > >> I also got USB packet dump and found that the error is happening >> regardless of HCI commands. >> >> One example is below. I just inserted the USB dongle when I got the >> following errors. >> [ 145.046503] Bluetooth: hci0: command 0x1002 tx timeout >> [ 147.086503] Bluetooth: hci0: command 0x0c52 tx timeout >> [ 149.121499] Bluetooth: hci0: command 0x0c45 tx timeout >> [ 151.166503] Bluetooth: hci0: command 0x0c58 tx timeout >> >> Please find the USB packet dump attached. > > I have trouble reading this. How did you make it? > What does it view with? You should be able to open it with WireShark. Sorry for not mentioning it. > > >> In frame no. 161, the host sent HCI command 0x1002 (Read Local Supported >> Commands). >> Then the USB dongle tried to respond to that from frame no. 163 but it >> did not sent all fragment packets. It seems to stop response in the middle. >> Finally, above "0x1002 tx timeout" happened. > > Did you try to repeat this from cold boot? Yes, I completely turned the target power OFF then ON and then inserted the USB BT dongle. > > >> As for frame no. 165, the host sent 0x0c52 (Write Extended Inquiry >> Response) but there was no response to it then above "0x0c52 tx timeout" >> happened. >> >> This is just one example. The USB dongle can be successfully inserted >> and working for a while but suddenly stops response to HCI commands like >> this example. >> It seems there is no specific HCI commands to cause the problem. >> >> I do not find out what is the trigger of it yet. >> Do you have any thoughts from this point? > > Have you tried ruling out LPM and other runtime PM? I disabled the following kernel options but the error still happens by power ON and inserting the BT dongle as mentioned above. * CPU Frequency scaling * CPU idle PM support * Suspend to RAM and standby * Hibernation (aka 'suspend to disk') * Device power management core functionality * Advanced Power Management Emulation I also disabled options under "Bluetooth subsystem support" (so only "Bluetooth subsystem support" is enabled) but still it is happening. I tried to reduce some USB options and enabled just only the following stuff but it did not make any difference. * Support for Host-side USB * USB announce new devices * DesignWare USB2 DRD Core Support > > This looks like a bluetooth issue, not specifically USB so we > are kind of the wrong mailing list. There is one for Bluetooth. That's right. I will organize information and re-send email on the Bluetooth mailing list including you. > > > Regards > Oliver Kind regards, Mori
Re: [PATCH 1/3] usb: dwc3: Add reference clock properties
Hi Felipe, On 11/8/2018 11:14 PM, Felipe Balbi wrote: > Hi, > > Thinh Nguyen writes: >>> Thinh Nguyen writes: diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt index 636630fb92d7..712b344c3a31 100644 --- a/Documentation/devicetree/bindings/usb/dwc3.txt +++ b/Documentation/devicetree/bindings/usb/dwc3.txt @@ -95,6 +95,24 @@ Optional properties: this and tx-thr-num-pkt-prd to a valid, non-zero value 1-16 (DWC_usb31 programming guide section 1.2.3) to enable periodic ESS TX threshold. + - snps,refclk-period-ns: set to program the reference clock period. The valid + input periods are as follow: + +-+-+ + | Period (ns) | Freq (MHz) | + +-+-+ + | 25 | 39.7/40 | + | 41 | 24.4| + | 50 | 20 | + | 52 | 19.2| + | 58 | 17.2| + | 62 | 16.1| + +-+-+ + - snps,enable-refclk-lpm: set to enable low power scheduling of isochronous + transfers by running SOF/ITP counters using the + reference clock. Only valid for DWC_usb31 peripheral + controller v1.80a and higher. Both + "snps,dis_u2_susphy_quirk" and + "snps,dis_enblslpm_quirk" must not be set. >>> sounds like you should rely on clk API here. Then on driver call >>> clk_get_rate() to computer whatever you need to compute. >>> >> There's nothing to compute here. We can simply enable this feature with >> "snps, enable-refclk-lpm" and the controller will use the default refclk >> settings. > Right, right. What I'm saying, though, is that we have a clock API for > describing a clock. So why wouldn't we rely on that API for this? I > think both of these new properties can be replaced with standard clock > API properties: > > clocks = <&clk1>, ..., <&lpm_clk> > clock-names = "clock1", , "lpm"; > > Then dwc3 core could, simply, check if we have a clock named "lpm" and > if there is, use NSECS_PER_SEC / clk_get_rate() to get its period and > write it to the register that needs the information. There's no new clock here. We are using the ref_clk for SOF and ITP counter for this feature. Also, clocks are optional on non-DT platforms. To use the clock API, then we need to update the driver to allow some optional clock such as "ref" clock for non-DT platforms. Do you want to do it like this? >>> I can't think of a problem that would arise from that. Can you? Mark, >>> Rob, what do you think? >>> >> No problem. That can be done. This will remove the >> "snps,refclk-period-ns" property. But we should have >> "snps,enable-refclk-lpm" to enable this feature. > not really. Just check if you have a clock named lpm. If you do, then > you enable the feature. > But this clock name should be "ref". The new name "lpm" would make it seem like it's a different clock. Thinh
[PATCH v2 2/3] spi: add FTDI MPSSE SPI controller driver
Add SPI bus controller driver for FTDI MPSSE mode. This driver
is supposed to be used together with the FT232H interface driver
for FPGA configuration in drivers/usb/misc/ft232h-intf.c which
adds an mpsse spi platform device describing USB SPI bus with
attached SPI slave devices.
Signed-off-by: Anatolij Gustschin
---
Changes in v2:
- fix build breakage when building with ARCH=i386 allmodconfig
- add checks for ops->lock/ops->unlock presence in pdata
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-ftdi-mpsse.c | 673 +++
3 files changed, 681 insertions(+)
create mode 100644 drivers/spi/spi-ftdi-mpsse.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7d3a5c94727e..2cb24e28c485 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -259,6 +259,13 @@ config SPI_FSL_LPSPI
help
This enables Freescale i.MX LPSPI controllers in master mode.
+config SPI_FTDI_MPSSE
+ tristate "FTDI MPSSE SPI controller"
+ depends on USB_FT232H_INTF || COMPILE_TEST
+ help
+ FT232H supports SPI in MPSSE mode. This driver provides MPSSE
+ SPI controller in master mode.
+
config SPI_GPIO
tristate "GPIO-based bitbanging SPI Master"
depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 3575205c5c27..268c42c502e2 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -113,6 +113,7 @@ obj-$(CONFIG_SPI_XILINX)+= spi-xilinx.o
obj-$(CONFIG_SPI_XLP) += spi-xlp.o
obj-$(CONFIG_SPI_XTENSA_XTFPGA)+= spi-xtensa-xtfpga.o
obj-$(CONFIG_SPI_ZYNQMP_GQSPI) += spi-zynqmp-gqspi.o
+obj-$(CONFIG_SPI_FTDI_MPSSE) += spi-ftdi-mpsse.o
# SPI slave protocol handlers
obj-$(CONFIG_SPI_SLAVE_TIME) += spi-slave-time.o
diff --git a/drivers/spi/spi-ftdi-mpsse.c b/drivers/spi/spi-ftdi-mpsse.c
new file mode 100644
index ..2507d7705e90
--- /dev/null
+++ b/drivers/spi/spi-ftdi-mpsse.c
@@ -0,0 +1,673 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FTDI FT232H MPSSE SPI controller driver
+ *
+ * Copyright (C) 2017 DENX Software Engineering
+ * Anatolij Gustschin
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+enum gpiol {
+ MPSSE_SK= BIT(0),
+ MPSSE_DO= BIT(1),
+ MPSSE_DI= BIT(2),
+ MPSSE_CS= BIT(3),
+};
+
+struct ftdi_spi {
+ struct platform_device *pdev;
+ struct usb_interface *intf;
+ struct spi_master *master;
+ const struct ft232h_intf_ops *iops;
+ struct gpiod_lookup_table *lookup[13];
+ struct gpio_desc **cs_gpios;
+
+ u8 txrx_cmd;
+ u8 rx_cmd;
+ u8 tx_cmd;
+ u8 xfer_buf[SZ_64K];
+ u16 last_mode;
+};
+
+static void ftdi_spi_chipselect(struct ftdi_spi *priv, struct spi_device *spi,
+ bool value)
+{
+ int cs = spi->chip_select;
+
+ dev_dbg(&priv->master->dev, "%s: CS %d, mode(%d), val %d\n",
+ __func__, cs, (spi->mode & SPI_CS_HIGH), value);
+
+ gpiod_set_raw_value_cansleep(priv->cs_gpios[cs], value);
+}
+
+static inline u8 ftdi_spi_txrx_byte_cmd(struct spi_device *spi)
+{
+ u8 mode = spi->mode & (SPI_CPOL | SPI_CPHA);
+ u8 cmd;
+
+ if (spi->mode & SPI_LSB_FIRST) {
+ switch (mode) {
+ case SPI_MODE_0:
+ case SPI_MODE_1:
+ cmd = TXF_RXR_BYTES_LSB;
+ break;
+ case SPI_MODE_2:
+ case SPI_MODE_3:
+ cmd = TXR_RXF_BYTES_LSB;
+ break;
+ }
+ } else {
+ switch (mode) {
+ case SPI_MODE_0:
+ case SPI_MODE_1:
+ cmd = TXF_RXR_BYTES_MSB;
+ break;
+ case SPI_MODE_2:
+ case SPI_MODE_3:
+ cmd = TXR_RXF_BYTES_MSB;
+ break;
+ }
+ }
+ return cmd;
+}
+
+static inline int ftdi_spi_loopback_cfg(struct ftdi_spi *priv, int on)
+{
+ int ret;
+
+ priv->xfer_buf[0] = on ? LOOPBACK_ON : LOOPBACK_OFF;
+
+ ret = priv->iops->write_data(priv->intf, priv->xfer_buf, 1);
+ if (ret < 0)
+ dev_warn(&priv->master->dev, "loopback %d failed\n", on);
+ return ret;
+}
+
+static int ftdi_spi_tx_rx(struct ftdi_spi *priv, struct spi_device *spi,
+ struct spi_transfer *t)
+{
+ const struct ft232h_intf_ops *ops = priv->iops;
+ struct spi_master *master = priv->master;
+ struct device *dev = &master->dev;
+ void *rx_offs;
+ const void *tx_offs;
+ size_t remaining, stride;
+ size_t rx_stride;
+ int ret, tout = 10;
+ const u8 *tx_d
Re: [RFC PATCH v1 13/14] usb:cdns3: Adds debugging function.
Hi,
On 03/11/18 19:51, Pawel Laszczak wrote:
> Patch implements some function used for debugging driver.
>
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/cdns3/Makefile | 2 +-
> drivers/usb/cdns3/debug.c | 128 +
> drivers/usb/cdns3/ep0.c| 3 +
> drivers/usb/cdns3/gadget.c | 12
> drivers/usb/cdns3/gadget.h | 13 +++-
> 5 files changed, 156 insertions(+), 2 deletions(-)
> create mode 100644 drivers/usb/cdns3/debug.c
>
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index f4cfc978626f..34e60d03c4ec 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -2,6 +2,6 @@ obj-$(CONFIG_USB_CDNS3) += cdns3.o
> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) += cdns3-pci.o
>
> cdns3-y := core.o drd.o
> -cdns3-$(CONFIG_USB_CDNS3_GADGET) += gadget.o ep0.o
> +cdns3-$(CONFIG_USB_CDNS3_GADGET) += gadget.o ep0.o debug.o
> cdns3-$(CONFIG_USB_CDNS3_HOST) += host.o
> cdns3-pci-y := cdns3-pci-wrap.o
> diff --git a/drivers/usb/cdns3/debug.c b/drivers/usb/cdns3/debug.c
> new file mode 100644
> index ..bebf22c4d18e
> --- /dev/null
> +++ b/drivers/usb/cdns3/debug.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence USBSS DRD Driver.
> + *
> + * Copyright (C) 2018 Cadence.
> + *
> + * Author: Pawel Laszczak
> + */
> +
> +#include "gadget.h"
> +
> +static inline char *cdns3_decode_ep_irq(u32 ep_sts, const char *ep_name)
> +{
> + static char str[256];
> + int ret;
> +
> + ret = sprintf(str, "IRQ for %s: %08x ", ep_name, ep_sts);
> +
> + if (ep_sts & EP_STS_SETUP)
> + ret += sprintf(str + ret, "SETUP ");
> + if (ep_sts & EP_STS_IOC)
> + ret += sprintf(str + ret, "IOC ");
> + if (ep_sts & EP_STS_ISP)
> + ret += sprintf(str + ret, "ISP ");
> + if (ep_sts & EP_STS_DESCMIS)
> + ret += sprintf(str + ret, "DESCMIS ");
> + if (ep_sts & EP_STS_STREAMR)
> + ret += sprintf(str + ret, "STREAMR ");
> + if (ep_sts & EP_STS_MD_EXIT)
> + ret += sprintf(str + ret, "MD_EXIT ");
> + if (ep_sts & EP_STS_TRBERR)
> + ret += sprintf(str + ret, "TRBERR ");
> + if (ep_sts & EP_STS_NRDY)
> + ret += sprintf(str + ret, "NRDY ");
> + if (ep_sts & EP_STS_PRIME)
> + ret += sprintf(str + ret, "PRIME ");
> + if (ep_sts & EP_STS_SIDERR)
> + ret += sprintf(str + ret, "SIDERRT ");
> + if (ep_sts & EP_STS_OUTSMM)
> + ret += sprintf(str + ret, "OUTSMM ");
> + if (ep_sts & EP_STS_ISOERR)
> + ret += sprintf(str + ret, "ISOERR ");
> + if (ep_sts & EP_STS_IOT)
> + ret += sprintf(str + ret, "IOT ");
> +
> + return str;
> +}
> +
> +char *cdns3_decode_epx_irq(struct cdns3_endpoint *priv_ep)
> +{
> + struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
> +
> + return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
> +priv_ep->name);
> +}
> +
> +char *cdns3_decode_ep0_irq(struct cdns3_device *priv_dev, int dir)
> +{
> + if (dir)
> + return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
> +"ep0IN");
> + else
> + return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
> +"ep0OUT");
> +}
> +
> +void cdns3_dbg_setup(struct cdns3_device *priv_dev)
> +{
> + struct usb_ctrlrequest *setup = priv_dev->setup;
> +
> + dev_dbg(&priv_dev->dev,
> + "SETUP BRT: %02x BR: %02x V: %04x I: %04x L: %04x\n",
> + setup->bRequestType,
> + setup->bRequest,
> + le16_to_cpu(setup->wValue),
> + le16_to_cpu(setup->wIndex),
> + le16_to_cpu(setup->wLength));
> +}
> +
> +/**
> + * Debug a transfer ring.
> + *
> + * Prints out all TRBs in the endpoint ring, even those after the Link TRB.
> + *.
> + */
> +void cdns3_dbg_ring(struct cdns3_device *priv_dev,
> + struct cdns3_endpoint *priv_ep)
> +{
> + u64 addr = priv_ep->trb_pool_dma;
> + struct cdns3_trb *trb;
> + int i;
> +
> + for (i = 0; i < TRBS_PER_SEGMENT; ++i) {
> + trb = &priv_ep->trb_pool[i];
> + dev_dbg(&priv_dev->dev, "@%016llx %08x %08x %08x\n", addr,
> + le32_to_cpu(trb->buffer),
> + le32_to_cpu(trb->length),
> + le32_to_cpu(trb->control));
> + addr += sizeof(*trb);
> + }
> +}
> +
> +void cdns3_dbg_ring_ptrs(struct cdns3_device *priv_dev,
> + struct cdns3_endpoint *priv_ep)
> +{
> + struct cdns3_trb *trb;
> +
> + trb = &priv_ep->trb_pool[priv_ep->dequeue];
> + dev_dbg(&priv_dev->dev,
> + "Ring deq index: %d, trb: %p (virt), 0x%llx (dma)\n",
> + priv_ep->
Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date
On Wed, Nov 07, 2018 at 07:17:51PM -0500, Sasha Levin wrote: > So why not just take 760db29bdc completely? It looks safer than taking a > partial backport, and will make applying future patches easier. > > I tried to do it and it doesn't look like there are any dependencies > that would cause an issue. Somehow i was convinced it didn't build on 4.4.x... can you pick it up? commit 760db29bdc97b73ff60b091315ad787b1deb5cf5 Author: Phil Elwell Date: Thu Apr 19 17:59:38 2018 +0100 lan78xx: Read MAC address from DT if present There is a standard mechanism for locating and using a MAC address from the Device Tree. Use this facility in the lan78xx driver to support applications without programmed EEPROM or OTP. At the same time, regularise the handling of the different address sources. Signed-off-by: Phil Elwell Signed-off-by: David S. Miller -- bye, p.
RE: [RFC PATCH v1 04/14] usb:cdns3: Added DRD support
Hi Roger,
>On 03/11/18 19:51, Pawel Laszczak wrote:
>> Patch adds supports for detecting Host/Device mode.
>> Controller has additional OTG register that allow
>> implement even whole OTG functionality.
>> At this moment patch adds support only for detecting
>> the appropriate mode based on strap pins and ID pin.
>>
>> Signed-off-by: Pawel Laszczak
>> ---
>> drivers/usb/cdns3/Makefile | 2 +-
>> drivers/usb/cdns3/core.c | 22 ++--
>> drivers/usb/cdns3/drd.c| 219 +
>> drivers/usb/cdns3/drd.h| 122 +
>> 4 files changed, 356 insertions(+), 9 deletions(-)
>> create mode 100644 drivers/usb/cdns3/drd.c
>> create mode 100644 drivers/usb/cdns3/drd.h
>>
>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> index 083676c7748f..d4ccfb49d844 100644
>> --- a/drivers/usb/cdns3/Makefile
>> +++ b/drivers/usb/cdns3/Makefile
>> @@ -1,7 +1,7 @@
>> obj-$(CONFIG_USB_CDNS3) += cdns3.o
>> obj-$(CONFIG_USB_CDNS3_PCI_WRAP)+= cdns3-pci.o
>>
>> -cdns3-y := core.o
>> +cdns3-y := core.o drd.o
>> cdns3-$(CONFIG_USB_CDNS3_GADGET)+= gadget.o
>> cdns3-$(CONFIG_USB_CDNS3_HOST) += host.o
>> cdns3-pci-y := cdns3-pci-wrap.o
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index 727136235957..20ae9e76940e 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -18,6 +18,7 @@
>> #include "core.h"
>> #include "host-export.h"
>> #include "gadget-export.h"
>> +#include "drd.h"
>>
>> static inline struct cdns3_role_driver *cdns3_role(struct cdns3 *cdns)
>> {
>> @@ -70,8 +71,10 @@ static void cdns3_set_role(struct cdns3 *cdns, enum
>> cdns3_roles role)
>> static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>> {
>> if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>> -//TODO: implements selecting device/host mode
>> -return CDNS3_ROLE_HOST;
>> +if (cdns3_is_host(cdns))
>> +return CDNS3_ROLE_HOST;
>> +if (cdns3_is_device(cdns))
>> +return CDNS3_ROLE_GADGET;
>> }
>> return cdns->roles[CDNS3_ROLE_HOST]
>> ? CDNS3_ROLE_HOST
>> @@ -141,6 +144,12 @@ static irqreturn_t cdns3_irq(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>>
>> +if (cdns->dr_mode == USB_DR_MODE_OTG) {
>> +ret = cdns3_drd_irq(cdns);
>> +if (ret == IRQ_HANDLED)
>> +return ret;
>> +}
>> +
>> /* Handle device/host interrupt */
>> if (cdns->role != CDNS3_ROLE_END)
>> ret = cdns3_role(cdns)->irq(cdns);
>> @@ -202,12 +211,8 @@ static void cdns3_role_switch(struct work_struct *work)
>> bool device, host;
>>
>> cdns = container_of(work, struct cdns3, role_switch_wq);
>> -
>> -//TODO: implements this functions.
>> -//host = cdns3_is_host(cdns);
>> -//device = cdns3_is_device(cdns);
>> -host = 1;
>> -device = 0;
>> +host = cdns3_is_host(cdns);
>> +device = cdns3_is_device(cdns);
>>
>> if (host) {
>> if (cdns->roles[CDNS3_ROLE_HOST])
>> @@ -286,6 +291,7 @@ static int cdns3_probe(struct platform_device *pdev)
>> if (ret)
>> goto err3;
>>
>> +ret = cdns3_drd_probe(cdns);
>> if (ret)
>> goto err3;
>>
>> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
>> new file mode 100644
>> index ..5d988432edb8
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/drd.c
>> @@ -0,0 +1,219 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Driver.
>> + *
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Author: Pawel Laszczak > + *
>> + */
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "gadget.h"
>> +#include "drd.h"
>> +
>> +/**
>> + * cdns3_set_mode - change mode of OTG Core
>> + * @cdns: pointer to context structure
>> + * @mode: selected mode from cdns_role
>> + */
>> +void cdns3_set_mode(struct cdns3 *cdns, u32 mode)
>> +{
>> +u32 reg;
>> +
>> +switch (mode) {
>> +case CDNS3_ROLE_GADGET:
>
>I think we need to make a clear distinction between mode and role.
>
>Mode is the controller mode. (Host-only, Device-only, dual-role[otg])
>Role is the USB controller state (A-Host, B-Device, Idle)
I agree with you, In next set patch distinction between mode and role will be
clear.
>
>cnds->dr_mode should correspond to enum usb_dr_mode which should be the
>argument
>for this function if you want to set mode.
>
>> +dev_info(cdns->dev, "Set controller to Gadget mode\n");
>> +writel(OTGCMD_DEV_BUS_REQ | OTGCMD_OTG_DIS,
>> + &cdns->otg_regs->cmd);
>> +break;
>> +case CDNS3_ROLE_HOST:
>> +dev_info(cdns->dev, "Set controller to Host mode\n");
>> +writel(
RE: [RFC PATCH v1 03/14] usb:cdns3: Driver initialization code.
>On 03/11/18 19:51, Pawel Laszczak wrote:
>> Patch adds core.c and core.h file that implements initialization
>> of platform driver and adds function responsible for selecting,
>> switching and running appropriate Device/Host mode.
>>
>> Patch also adds gadget.c, host.c, gadget-export.h, host-export.h.
>> These files contains templates functions used during initialization.
>> The implementation will be added in next patches.
>>
>> Signed-off-by: Pawel Laszczak
>> ---
>> drivers/usb/cdns3/Kconfig | 20 ++
>> drivers/usb/cdns3/Makefile| 4 +
>> drivers/usb/cdns3/core.c | 373 ++
>> drivers/usb/cdns3/core.h | 88 +++
>> drivers/usb/cdns3/gadget-export.h | 27 +++
>> drivers/usb/cdns3/gadget.c| 36 +++
>> drivers/usb/cdns3/host-export.h | 30 +++
>> drivers/usb/cdns3/host.c | 28 +++
>> 8 files changed, 606 insertions(+)
>> create mode 100644 drivers/usb/cdns3/core.c
>> create mode 100644 drivers/usb/cdns3/core.h
>> create mode 100644 drivers/usb/cdns3/gadget-export.h
>> create mode 100644 drivers/usb/cdns3/gadget.c
>> create mode 100644 drivers/usb/cdns3/host-export.h
>> create mode 100644 drivers/usb/cdns3/host.c
>>
>
>
>
>
>> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>> new file mode 100644
>> index ..e7159c474308
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/core.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Cadence USBSS DRD Driver.
>> + *
>> + * Copyright (C) 2017 NXP
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Authors: Peter Chen
>> + * Pawel Laszczak
>> + */
>> +#include
>> +
>> +#ifndef __LINUX_CDNS3_CORE_H
>> +#define __LINUX_CDNS3_CORE_H
>> +
>> +struct cdns3;
>> +enum cdns3_roles {
>> +CDNS3_ROLE_HOST = 0,
>> +CDNS3_ROLE_GADGET,
>> +CDNS3_ROLE_END,
>> +CDNS3_ROLE_OTG,
>> +};
>> +
>> +/**
>> + * struct cdns3_role_driver - host/gadget role driver
>> + * @start: start this role
>> + * @stop: stop this role
>> + * @suspend: suspend callback for this role
>> + * @resume: resume callback for this role
>> + * @irq: irq handler for this role
>> + * @name: role name string (host/gadget)
>> + */
>> +struct cdns3_role_driver {
>> +int (*start)(struct cdns3 *cdns);
>> +void (*stop)(struct cdns3 *cdns);
>> +int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
>> +int (*resume)(struct cdns3 *cdns, bool hibernated);
>> +irqreturn_t (*irq)(struct cdns3 *cdns);
>
>Why does role driver need hook to irq handler?
>
>Can't each driver host or gadget handle it's respective irq
>on its own? If the same IRQ line is used it could be requested
>as a shared IRQ.
When controller is working as Device then host part of controller is held in
reset and
vice versa, so driver has restricted access to registers.
>> +const char *name;
>> +};
>> +
>
>cheers,
>-roger
>
>--
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Thanks,
Cheers,
Pawel Laszczak
RE: [RFC PATCH v1 14/14] usb:cdns3: Feature for changing role
>On 03/11/18 19:51, Pawel Laszczak wrote:
>> Patch adds feature that allow to change role from user space.
>>
>> Signed-off-by: Pawel Laszczak
>> ---
>> drivers/usb/cdns3/Makefile | 2 +-
>> drivers/usb/cdns3/core.c| 2 +
>> drivers/usb/cdns3/debugfs.c | 94 +
>> drivers/usb/cdns3/drd.h | 3 ++
>> 4 files changed, 100 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/usb/cdns3/debugfs.c
>>
>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> index 34e60d03c4ec..08e6cdbebd46 100644
>> --- a/drivers/usb/cdns3/Makefile
>> +++ b/drivers/usb/cdns3/Makefile
>> @@ -1,7 +1,7 @@
>> obj-$(CONFIG_USB_CDNS3) += cdns3.o
>> obj-$(CONFIG_USB_CDNS3_PCI_WRAP)+= cdns3-pci.o
>>
>> -cdns3-y := core.o drd.o
>> +cdns3-y := core.o drd.o debugfs.o
>> cdns3-$(CONFIG_USB_CDNS3_GADGET)+= gadget.o ep0.o debug.o
>> cdns3-$(CONFIG_USB_CDNS3_HOST) += host.o
>> cdns3-pci-y := cdns3-pci-wrap.o
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index 20ae9e76940e..4012f1007da9 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -309,6 +309,7 @@ static int cdns3_probe(struct platform_device *pdev)
>> if (ret)
>> goto err4;
>>
>> +cdns3_debugfs_init(cdns);
>> device_set_wakeup_capable(dev, true);
>> pm_runtime_set_active(dev);
>> pm_runtime_enable(dev);
>> @@ -346,6 +347,7 @@ static int cdns3_remove(struct platform_device *pdev)
>> pm_runtime_get_sync(&pdev->dev);
>> pm_runtime_disable(&pdev->dev);
>> pm_runtime_put_noidle(&pdev->dev);
>> +cdns3_debugfs_exit(cdns);
>> cdns3_remove_roles(cdns);
>> usb_phy_shutdown(cdns->usbphy);
>>
>> diff --git a/drivers/usb/cdns3/debugfs.c b/drivers/usb/cdns3/debugfs.c
>> new file mode 100644
>> index ..d4871bc1a69d
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/debugfs.c
>> @@ -0,0 +1,94 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Controller DebugFS filer.
>> + *
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Author: Pawel Laszczak
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "core.h"
>> +#include "gadget.h"
>> +
>> +static int cdns3_mode_show(struct seq_file *s, void *unused)
>> +{
>> +struct cdns3*cdns = s->private;
>> +
>> +switch (cdns->role) {
>> +case CDNS3_ROLE_HOST:
>> +seq_puts(s, "host\n");
>> +break;
>> +case CDNS3_ROLE_GADGET:
>> +seq_puts(s, "device\n");
>> +break;
>> +case CDNS3_ROLE_OTG:
>> +case CDNS3_ROLE_END:
>> +seq_puts(s, "otg\n");
>> +break;
>> +default:
>> +seq_puts(s, "UNKNOWN mode\n");
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static int cdns3_mode_open(struct inode *inode, struct file *file)
>> +{
>> +return single_open(file, cdns3_mode_show, inode->i_private);
>> +}
>> +
>> +static ssize_t cdns3_mode_write(struct file *file,
>> +const char __user *ubuf,
>> +size_t count, loff_t *ppos)
>> +{
>> +struct seq_file *s = file->private_data;
>> +struct cdns3*cdns = s->private;
>> +u32 mode = 0;
>> +charbuf[32];
>> +
>> +if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
>> +return -EFAULT;
>> +
>> +if (!strncmp(buf, "host", 4))
>> +mode = USB_DR_MODE_HOST;
>> +
>> +if (!strncmp(buf, "device", 6))
>> +mode = USB_DR_MODE_PERIPHERAL;
>> +
>> +if (!strncmp(buf, "otg", 3))
>> +mode = USB_DR_MODE_OTG;
>> +
>> +cdns->desired_role = mode;
>> +queue_work(system_freezable_wq, &cdns->role_switch_wq);
>
>If we start with OTG mode and user says change mode to device will we still
>switch to host based on ID pin change?
>
>If it does then this isn't working correctly.
>We need to stop processing ID interrupts and keep the role static till
>the user switches it back to otg.
Switching role form user space will limited driver only to selected mode.
Only for USB_DR_MODE_OTG driver should base on ID pin.
That's my intension.
>> +return count;
>> +}
>> +
>> +static const struct file_operations cdns3_mode_fops = {
>> +.open = cdns3_mode_open,
>> +.write = cdns3_mode_write,
>> +.read = seq_read,
>> +.llseek = seq_lseek,
>> +.release= single_release,
>> +};
>> +
>> +void cdns3_debugfs_init(struct cdns3 *cdns)
>> +{
>> +struct dentry *root;
>> +
>> +root = debugfs_create_dir(dev_name(cdns->dev), NULL);
>> +cdns->root = root;
>> +if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET) &&
>> +IS_ENABLED(CONFIG_USB_CDNS3_HOST))
RE: [RFC PATCH v1 13/14] usb:cdns3: Adds debugging function.
>Hi,
>
>On 03/11/18 19:51, Pawel Laszczak wrote:
>> Patch implements some function used for debugging driver.
>>
>> Signed-off-by: Pawel Laszczak
>> ---
>> drivers/usb/cdns3/Makefile | 2 +-
>> drivers/usb/cdns3/debug.c | 128 +
>> drivers/usb/cdns3/ep0.c| 3 +
>> drivers/usb/cdns3/gadget.c | 12
>> drivers/usb/cdns3/gadget.h | 13 +++-
>> 5 files changed, 156 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/usb/cdns3/debug.c
>>
>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> index f4cfc978626f..34e60d03c4ec 100644
>> --- a/drivers/usb/cdns3/Makefile
>> +++ b/drivers/usb/cdns3/Makefile
>> @@ -2,6 +2,6 @@ obj-$(CONFIG_USB_CDNS3) += cdns3.o
>> obj-$(CONFIG_USB_CDNS3_PCI_WRAP)+= cdns3-pci.o
>>
>> cdns3-y := core.o drd.o
>> -cdns3-$(CONFIG_USB_CDNS3_GADGET)+= gadget.o ep0.o
>> +cdns3-$(CONFIG_USB_CDNS3_GADGET)+= gadget.o ep0.o debug.o
>> cdns3-$(CONFIG_USB_CDNS3_HOST) += host.o
>> cdns3-pci-y := cdns3-pci-wrap.o
>> diff --git a/drivers/usb/cdns3/debug.c b/drivers/usb/cdns3/debug.c
>> new file mode 100644
>> index ..bebf22c4d18e
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/debug.c
>> @@ -0,0 +1,128 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Driver.
>> + *
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Author: Pawel Laszczak
>> + */
>> +
>> +#include "gadget.h"
>> +
>> +static inline char *cdns3_decode_ep_irq(u32 ep_sts, const char *ep_name)
>> +{
>> +static char str[256];
>> +int ret;
>> +
>> +ret = sprintf(str, "IRQ for %s: %08x ", ep_name, ep_sts);
>> +
>> +if (ep_sts & EP_STS_SETUP)
>> +ret += sprintf(str + ret, "SETUP ");
>> +if (ep_sts & EP_STS_IOC)
>> +ret += sprintf(str + ret, "IOC ");
>> +if (ep_sts & EP_STS_ISP)
>> +ret += sprintf(str + ret, "ISP ");
>> +if (ep_sts & EP_STS_DESCMIS)
>> +ret += sprintf(str + ret, "DESCMIS ");
>> +if (ep_sts & EP_STS_STREAMR)
>> +ret += sprintf(str + ret, "STREAMR ");
>> +if (ep_sts & EP_STS_MD_EXIT)
>> +ret += sprintf(str + ret, "MD_EXIT ");
>> +if (ep_sts & EP_STS_TRBERR)
>> +ret += sprintf(str + ret, "TRBERR ");
>> +if (ep_sts & EP_STS_NRDY)
>> +ret += sprintf(str + ret, "NRDY ");
>> +if (ep_sts & EP_STS_PRIME)
>> +ret += sprintf(str + ret, "PRIME ");
>> +if (ep_sts & EP_STS_SIDERR)
>> +ret += sprintf(str + ret, "SIDERRT ");
>> +if (ep_sts & EP_STS_OUTSMM)
>> +ret += sprintf(str + ret, "OUTSMM ");
>> +if (ep_sts & EP_STS_ISOERR)
>> +ret += sprintf(str + ret, "ISOERR ");
>> +if (ep_sts & EP_STS_IOT)
>> +ret += sprintf(str + ret, "IOT ");
>> +
>> +return str;
>> +}
>> +
>> +char *cdns3_decode_epx_irq(struct cdns3_endpoint *priv_ep)
>> +{
>> +struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
>> +
>> +return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
>> + priv_ep->name);
>> +}
>> +
>> +char *cdns3_decode_ep0_irq(struct cdns3_device *priv_dev, int dir)
>> +{
>> +if (dir)
>> +return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
>> + "ep0IN");
>> +else
>> +return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
>> + "ep0OUT");
>> +}
>> +
>> +void cdns3_dbg_setup(struct cdns3_device *priv_dev)
>> +{
>> +struct usb_ctrlrequest *setup = priv_dev->setup;
>> +
>> +dev_dbg(&priv_dev->dev,
>> +"SETUP BRT: %02x BR: %02x V: %04x I: %04x L: %04x\n",
>> +setup->bRequestType,
>> +setup->bRequest,
>> +le16_to_cpu(setup->wValue),
>> +le16_to_cpu(setup->wIndex),
>> +le16_to_cpu(setup->wLength));
>> +}
>> +
>> +/**
>> + * Debug a transfer ring.
>> + *
>> + * Prints out all TRBs in the endpoint ring, even those after the Link TRB.
>> + *.
>> + */
>> +void cdns3_dbg_ring(struct cdns3_device *priv_dev,
>> +struct cdns3_endpoint *priv_ep)
>> +{
>> +u64 addr = priv_ep->trb_pool_dma;
>> +struct cdns3_trb *trb;
>> +int i;
>> +
>> +for (i = 0; i < TRBS_PER_SEGMENT; ++i) {
>> +trb = &priv_ep->trb_pool[i];
>> +dev_dbg(&priv_dev->dev, "@%016llx %08x %08x %08x\n", addr,
>> +le32_to_cpu(trb->buffer),
>> +le32_to_cpu(trb->length),
>> +le32_to_cpu(trb->control));
>> +addr += sizeof(*trb);
>> +}
>> +}
>> +
>> +void cdns3_dbg_ring_ptrs(struct cdns3_device *priv_dev,
>> + struct cdns3_endpoint *priv_ep)
>> +{
>> +struct cdns3_trb *trb;
>> +
>> +trb = &priv_ep->trb_pool[priv_ep->dequeue];
>> +dev_dbg(&priv_dev->dev,
>> +"Ring deq i
Re: [PATCH 0/5 v7] Keep rtsx_usb suspended when there's no card
This is based on Ulf's work [1] [2]. This patch series can keep rtsx_usb suspended, to save ~0.5W on Intel platforms and ~1.5W on AMD platforms. [1] https://patchwork.kernel.org/patch/10440583/ [2] https://patchwork.kernel.org/patch/10445725/ Tested-by: Oleksandr Natalenko -- Oleksandr Natalenko (post-factum)
RE: [PATCH] usb: gadget: fix spelling mistakeis "[En]queing" -> "[En]queuing"
From: Andrew Jeffery > Sent: 08 November 2018 01:55 > > - EPDBG(ep,"Enqueing request on wrong or disabled EP\n"); > > + EPDBG(ep, "Enqueuing request on wrong or disabled EP\n"); Shouldn't it be Enqueueing ? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [RFC PATCH v1 14/14] usb:cdns3: Feature for changing role
On 08/11/18 13:51, Pawel Laszczak wrote:
>> On 03/11/18 19:51, Pawel Laszczak wrote:
>>> Patch adds feature that allow to change role from user space.
>>>
>>> Signed-off-by: Pawel Laszczak
>>> ---
>>> drivers/usb/cdns3/Makefile | 2 +-
>>> drivers/usb/cdns3/core.c| 2 +
>>> drivers/usb/cdns3/debugfs.c | 94 +
>>> drivers/usb/cdns3/drd.h | 3 ++
>>> 4 files changed, 100 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/usb/cdns3/debugfs.c
>>>
>>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>>> index 34e60d03c4ec..08e6cdbebd46 100644
>>> --- a/drivers/usb/cdns3/Makefile
>>> +++ b/drivers/usb/cdns3/Makefile
>>> @@ -1,7 +1,7 @@
>>> obj-$(CONFIG_USB_CDNS3)+= cdns3.o
>>> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) += cdns3-pci.o
>>>
>>> -cdns3-y:= core.o drd.o
>>> +cdns3-y:= core.o drd.o debugfs.o
>>> cdns3-$(CONFIG_USB_CDNS3_GADGET) += gadget.o ep0.o debug.o
>>> cdns3-$(CONFIG_USB_CDNS3_HOST) += host.o
>>> cdns3-pci-y:= cdns3-pci-wrap.o
>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>> index 20ae9e76940e..4012f1007da9 100644
>>> --- a/drivers/usb/cdns3/core.c
>>> +++ b/drivers/usb/cdns3/core.c
>>> @@ -309,6 +309,7 @@ static int cdns3_probe(struct platform_device *pdev)
>>> if (ret)
>>> goto err4;
>>>
>>> + cdns3_debugfs_init(cdns);
>>> device_set_wakeup_capable(dev, true);
>>> pm_runtime_set_active(dev);
>>> pm_runtime_enable(dev);
>>> @@ -346,6 +347,7 @@ static int cdns3_remove(struct platform_device *pdev)
>>> pm_runtime_get_sync(&pdev->dev);
>>> pm_runtime_disable(&pdev->dev);
>>> pm_runtime_put_noidle(&pdev->dev);
>>> + cdns3_debugfs_exit(cdns);
>>> cdns3_remove_roles(cdns);
>>> usb_phy_shutdown(cdns->usbphy);
>>>
>>> diff --git a/drivers/usb/cdns3/debugfs.c b/drivers/usb/cdns3/debugfs.c
>>> new file mode 100644
>>> index ..d4871bc1a69d
>>> --- /dev/null
>>> +++ b/drivers/usb/cdns3/debugfs.c
>>> @@ -0,0 +1,94 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Cadence USBSS DRD Controller DebugFS filer.
>>> + *
>>> + * Copyright (C) 2018 Cadence.
>>> + *
>>> + * Author: Pawel Laszczak
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include "core.h"
>>> +#include "gadget.h"
>>> +
>>> +static int cdns3_mode_show(struct seq_file *s, void *unused)
>>> +{
>>> + struct cdns3*cdns = s->private;
>>> +
>>> + switch (cdns->role) {
>>> + case CDNS3_ROLE_HOST:
>>> + seq_puts(s, "host\n");
>>> + break;
>>> + case CDNS3_ROLE_GADGET:
>>> + seq_puts(s, "device\n");
>>> + break;
>>> + case CDNS3_ROLE_OTG:
>>> + case CDNS3_ROLE_END:
>>> + seq_puts(s, "otg\n");
>>> + break;
>>> + default:
>>> + seq_puts(s, "UNKNOWN mode\n");
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int cdns3_mode_open(struct inode *inode, struct file *file)
>>> +{
>>> + return single_open(file, cdns3_mode_show, inode->i_private);
>>> +}
>>> +
>>> +static ssize_t cdns3_mode_write(struct file *file,
>>> + const char __user *ubuf,
>>> + size_t count, loff_t *ppos)
>>> +{
>>> + struct seq_file *s = file->private_data;
>>> + struct cdns3*cdns = s->private;
>>> + u32 mode = 0;
>>> + charbuf[32];
>>> +
>>> + if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
>>> + return -EFAULT;
>>> +
>>> + if (!strncmp(buf, "host", 4))
>>> + mode = USB_DR_MODE_HOST;
>>> +
>>> + if (!strncmp(buf, "device", 6))
>>> + mode = USB_DR_MODE_PERIPHERAL;
>>> +
>>> + if (!strncmp(buf, "otg", 3))
>>> + mode = USB_DR_MODE_OTG;
>>> +
>>> + cdns->desired_role = mode;
>>> + queue_work(system_freezable_wq, &cdns->role_switch_wq);
>>
>> If we start with OTG mode and user says change mode to device will we still
>> switch to host based on ID pin change?
>>
>> If it does then this isn't working correctly.
>> We need to stop processing ID interrupts and keep the role static till
>> the user switches it back to otg.
>
> Switching role form user space will limited driver only to selected mode.
> Only for USB_DR_MODE_OTG driver should base on ID pin.
> That's my intension.
>
User space setting should override ID if it was OTG mode. At least this is how
it is for dwc3. That way it is useful for testing role swap.
cheers,
-roger
>>> + return count;
>>> +}
>>> +
>>> +static const struct file_operations cdns3_mode_fops = {
>>> + .open = cdns3_mode_open,
>>> + .write = cdns3_mode_write,
>>> + .read = seq_read,
>>> + .llseek = seq_lseek,
>>> + .release= sin
Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date
On Thu, Nov 08, 2018 at 12:01:27PM +0100, Paolo Pisati wrote: On Wed, Nov 07, 2018 at 07:17:51PM -0500, Sasha Levin wrote: So why not just take 760db29bdc completely? It looks safer than taking a partial backport, and will make applying future patches easier. I tried to do it and it doesn't look like there are any dependencies that would cause an issue. Somehow i was convinced it didn't build on 4.4.x... can you pick it up? commit 760db29bdc97b73ff60b091315ad787b1deb5cf5 Author: Phil Elwell Date: Thu Apr 19 17:59:38 2018 +0100 lan78xx: Read MAC address from DT if present There is a standard mechanism for locating and using a MAC address from the Device Tree. Use this facility in the lan78xx driver to support applications without programmed EEPROM or OTP. At the same time, regularise the handling of the different address sources. Signed-off-by: Phil Elwell Signed-off-by: David S. Miller Can you confirm it actually works on 4.4? -- Thanks, Sasha
Re: [PATCH] usb: dwc2: gadget: fix ISOC frame overflow handling
Hi Minas,
On Mon, 5 Nov 2018 08:28:07 +
Minas Harutyunyan wrote:
> On 10/23/2018 5:43 PM, John Keeping wrote:
> > By clearing the overrun flag as soon as the target frame is next
> > incremented, we can end up incrementing the target frame more than
> > expected in dwc2_gadget_handle_ep_disabled() when the endpoint's
> > interval is greater than 1. This happens if the target frame has
> > just wrapped at the point when the endpoint is disabled and the
> > frame number has not yet done so.
> >
> > Instead, wait until the frame number also wraps and then clear the
> > overrun flag.
> >
> > Signed-off-by: John Keeping
> > ---
> > drivers/usb/dwc2/gadget.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 2d6d2c8244de..8da2c052dfa1 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -117,7 +117,7 @@ static inline void
> > dwc2_gadget_incr_frame_num(struct dwc2_hsotg_ep *hs_ep) if
> > (hs_ep->target_frame > DSTS_SOFFN_LIMIT) { hs_ep->frame_overrun =
> > true; hs_ep->target_frame &= DSTS_SOFFN_LIMIT;
> > - } else {
> > + } else if (hs_ep->parent->frame_number <
> > hs_ep->target_frame) { hs_ep->frame_overrun = false;
> > }
> > }
> >
> Did you tested mentioned by you scenario? If you see issue can you
> provide debug log and point the issue line in the log.
It only reproduces very occasionally so it's difficult to capture a full
debug log containing the error.
I applied this patch to capture logging specifically around this
scenario:
-- >8 --
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 220c0f9b89b0..3770b9d3b523 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2722,13 +2722,20 @@ static void dwc2_gadget_handle_ep_disabled(struct
dwc2_hsotg_ep *hs_ep)
}
do {
+ unsigned int target_frame = hs_ep->target_frame;
+ bool frame_overrun = hs_ep->frame_overrun;
+
hs_req = get_ep_head(hs_ep);
if (hs_req)
dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req,
-ENODATA);
+
dwc2_gadget_incr_frame_num(hs_ep);
/* Update current frame number value. */
hsotg->frame_number = dwc2_hsotg_read_frameno(hsotg);
+
+ dev_warn(hsotg->dev, "%s: expiring request frame_number=0x%04x
target_frame=0x%04x overrun=%u\n",
+__func__, hsotg->frame_number, target_frame,
frame_overrun);
} while (dwc2_gadget_target_frame_elapsed(hs_ep));
dwc2_gadget_start_next_request(hs_ep);
-- 8< --
and I captured this log (the first entry is a separate error and then
the remaining ones show this bug being triggered):
[ 562.571227] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3eb9 target_frame=0x3ec0
[ 562.611213] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff8 target_frame=0x0008
[ 562.611219] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff8 target_frame=0x0010
[ 562.611223] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0018
[ 562.611228] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0020
[ 562.611232] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0028
[ 562.611236] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0030
[ 562.611240] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0038
[ 562.611244] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0040
[ 562.611249] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0048
[ 562.611253] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0050
[ 562.611257] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0058
[ 562.611261] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0060
[ 562.611265] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0068
[ 562.611269] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0070
[ 562.611274] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0078
[ 562.611278] dwc2 ff58.usb: dwc2_gadget_handle_ep_disabled: expiring
request frame_number=0x3ff9 target_frame=0x0080
[ 562.611282] dwc2
[PATCH] net: smsc95xx: Fix MTU range
The commit f77f0aee4da4 ("net: use core MTU range checking in USB NIC
drivers") introduce a common MTU handling for usbnet. But it's missing
the necessary changes for smsc95xx. So set the MTU range accordingly.
This patch has been tested on a Raspberry Pi 3.
Fixes: f77f0aee4da4 ("net: use core MTU range checking in USB NIC drivers")
Signed-off-by: Stefan Wahren
---
drivers/net/usb/smsc95xx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 262e7a3..5974478 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1321,6 +1321,8 @@ static int smsc95xx_bind(struct usbnet *dev, struct
usb_interface *intf)
dev->net->ethtool_ops = &smsc95xx_ethtool_ops;
dev->net->flags |= IFF_MULTICAST;
dev->net->hard_header_len += SMSC95XX_TX_OVERHEAD_CSUM;
+ dev->net->min_mtu = ETH_MIN_MTU;
+ dev->net->max_mtu = ETH_DATA_LEN;
dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
pdata->dev = dev;
--
2.7.4
Re: [PATCH V2 2/6] usb: core: Add ability to skip phy exit on suspend and init on resume
On Wed, Nov 7, 2018 at 8:43 PM Chunfeng Yun wrote: > > hi, > On Tue, 2018-10-30 at 18:30 -0400, Alan Cooper wrote: > > On 10/17/18 9:46 PM, Chunfeng Yun wrote:> hi, > > > > > > On Wed, 2018-10-17 at 18:29 -0400, Al Cooper wrote: > > >> Add the ability to skip calling the PHY's exit routine on suspend > > >> and the PHY's init routine on resume. This is to handle a USB PHY > > >> that should have it's power_off function called on suspend but > > cannot > > >> have it's exit function called because on exit it will disable the > > >> PHY to the point where register accesses to the Host Controllers > > >> using the PHY will be disabled and the host drivers will crash. > > >> > > >> This is enabled with the HCD flag "suspend_without_phy_exit" which > > >> can be set from any HCD driver. > > >> > > >> Signed-off-by: Al Cooper > > >> --- > > >> drivers/usb/core/hcd.c | 8 > > >> drivers/usb/core/phy.c | 18 -- > > >> drivers/usb/core/phy.h | 9 ++--- > > >> include/linux/usb/hcd.h | 3 +++ > > >> 4 files changed, 25 insertions(+), 13 deletions(-) > > >> > > > > >> unsignedskip_phy_initialization:1; > > >> > > >> +/* Some phys don't want the phy's exit/init called on > > suspend/resume */ > > >> +unsignedsuspend_without_phy_exit:1; > > > As suggested before, you can skip phy's exit/init during > > suspend/resume > > > by enabling wakeup of hcd, so needn't add a new variable for it. > > > > I still need to be able to enable and disable wakeup for this driver. > Just use device_init_wakeup(dev, true) instead of > device_wakeup_enable(dev) for your controller driver, you can try it. > > Sorry for the late replay What about the case where sysfs is used to disable wakeup? Al > > > > > > > >> + > > >> /* The next flag is a stopgap, to be removed when all the > > HCDs > > >>* support the new root-hub polling mechanism. */ > > >> unsigneduses_new_polling:1; > > > > >
Re: [PATCH] net: smsc95xx: Fix MTU range
From: Stefan Wahren
Date: Thu, 8 Nov 2018 20:38:26 +0100
> The commit f77f0aee4da4 ("net: use core MTU range checking in USB NIC
> drivers") introduce a common MTU handling for usbnet. But it's missing
> the necessary changes for smsc95xx. So set the MTU range accordingly.
>
> This patch has been tested on a Raspberry Pi 3.
>
> Fixes: f77f0aee4da4 ("net: use core MTU range checking in USB NIC drivers")
> Signed-off-by: Stefan Wahren
Applied and queued up for -stable, thanks.
