Re: [PATCH RESEND] PCI: Check for USB xHCI class for HAPS platform
On 02/05/2019 03:32 PM, Bjorn Helgaas wrote: [+cc Richard, Lucas] On Tue, Feb 05, 2019 at 01:04:28PM -0800, Thinh Nguyen wrote: The Synopsys HAPS USB controller has a VID PID (16c3,abcd) that matches to an existing PCIe controller. This quirk is intended for USB HAPS devices only. To fix this, check for the PCI class USB xHCI to prevent matching the PCIe controllers. So there are at least three different parts with the same Vendor & Device ID ([16c3:abcd]): 1) Synopsys HAPS USB3 controller 2) Synopsys PCIe IP in the NXP i.MX6QP (reported by Lukas) 3) Synopsys PCIe IP in the NXP i.MX7D (reported by Trent) I don't know if Synopsys is to blame for 2 & 3, or if NXP was expected to change the Vendor ID when incorporating the Synopsys IP into the i.MX designs. But even leaving the default Device ID of the PCIe IP the same as another Synopsys device was probably a bad idea. Hi Bjorn, From talking with our PCIe folks, our best guess is a vendor misconfiguration. The PCIe IP ships with a different PID by default and does not collide with USB. dwc3-haps claims by Vendor & Device ID; it doesn't look at the Class Code. What happens when it tries to claim the PCIe IP in i.MX? We can add the class code to dwc3-haps to account for this. John
Re: [PATCH] usb: dwc2: gadget: Use true and false for boolean values
On 01/23/2018 07:45 AM, Gustavo A. R. Silva wrote:
Assign true or false to boolean variables instead of an integer value.
This issue was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/dwc2/gadget.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index e4c3ce0..1f684dd 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -116,10 +116,10 @@ static inline void dwc2_gadget_incr_frame_num(struct
dwc2_hsotg_ep *hs_ep)
{
hs_ep->target_frame += hs_ep->interval;
if (hs_ep->target_frame > DSTS_SOFFN_LIMIT) {
- hs_ep->frame_overrun = 1;
+ hs_ep->frame_overrun = true;
hs_ep->target_frame &= DSTS_SOFFN_LIMIT;
} else {
- hs_ep->frame_overrun = 0;
+ hs_ep->frame_overrun = false;
}
}
+Felipe
Acked-by: John Youn
Regards,
John
Re: [PATCH] usb: dwc2: add optional usb ecc reset bit
On 11/01/2017 08:35 AM, Dinh Nguyen wrote:
> The dwc2 USB controller in Stratix10 has an additional ECC reset bit that
> needs to get de-asserted in order for the controller to work properly.
>
> Signed-off-by: Dinh Nguyen
> ---
> drivers/usb/dwc2/core.h | 1 +
> drivers/usb/dwc2/platform.c | 10 ++
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 8367d4f9..a4b5f4e 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -920,6 +920,7 @@ struct dwc2_hsotg {
> int irq;
> struct clk *clk;
> struct reset_control *reset;
> + struct reset_control *reset_ecc;
>
> unsigned int queuing_high_bandwidth:1;
> unsigned int srp_success:1;
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index daf0d37..d466e03 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -220,6 +220,15 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
> *hsotg)
>
> reset_control_deassert(hsotg->reset);
>
> + hsotg->reset_ecc = devm_reset_control_get_optional(hsotg->dev,
> "dwc2-ecc");
> + if (IS_ERR(hsotg->reset_ecc)) {
> + ret = PTR_ERR(hsotg->reset_ecc);
> + dev_err(hsotg->dev, "error getting reset control for ecc %d\n",
> ret);
> + return ret;
> + }
> +
> + reset_control_deassert(hsotg->reset_ecc);
> +
> /* Set default UTMI width */
> hsotg->phyif = GUSBCFG_PHYIF16;
>
> @@ -318,6 +327,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
> dwc2_lowlevel_hw_disable(hsotg);
>
> reset_control_assert(hsotg->reset);
> + reset_control_assert(hsotg->reset_ecc);
>
> return 0;
> }
>
Acked-by: John Youn
John
Re: [PATCH] usb: dwc2: Fix TxFIFOn sizes and total TxFIFO size issues
otal FIFO depth available for
> * device mode TX FIFOs
> */
> int dwc2_hsotg_tx_fifo_total_depth(struct dwc2_hsotg *hsotg)
> {
> - int ep_info_size;
> int addr;
> int tx_addr_max;
> u32 np_tx_fifo_size;
> @@ -255,8 +218,7 @@ int dwc2_hsotg_tx_fifo_total_depth(struct dwc2_hsotg
> *hsotg)
> hsotg->params.g_np_tx_fifo_size);
>
> /* Get Endpoint Info Control block size in DWORDs. */
> - ep_info_size = dwc2_hsotg_ep_info_size(hsotg);
> - tx_addr_max = hsotg->hw_params.total_fifo_size - ep_info_size;
> + tx_addr_max = hsotg->hw_params.total_fifo_size;
>
> addr = hsotg->params.g_rx_fifo_size + np_tx_fifo_size;
> if (tx_addr_max <= addr)
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index a3ffe97170ff..ad2ebff74924 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -469,8 +469,7 @@ static void dwc2_check_param_tx_fifo_sizes(struct
> dwc2_hsotg *hsotg)
> }
>
> for (fifo = 1; fifo <= fifo_count; fifo++) {
> - dptxfszn = (dwc2_readl(hsotg->regs + DPTXFSIZN(fifo)) &
> - FIFOSIZE_DEPTH_MASK) >> FIFOSIZE_DEPTH_SHIFT;
> + dptxfszn = hsotg->hw_params.g_tx_fifo_size[fifo];
>
> if (hsotg->params.g_tx_fifo_size[fifo] < min ||
> hsotg->params.g_tx_fifo_size[fifo] > dptxfszn) {
> @@ -594,6 +593,7 @@ static void dwc2_get_dev_hwparams(struct dwc2_hsotg
> *hsotg)
> struct dwc2_hw_params *hw = &hsotg->hw_params;
> bool forced;
> u32 gnptxfsiz;
> + int fifo, fifo_count;
>
> if (hsotg->dr_mode == USB_DR_MODE_HOST)
> return;
> @@ -602,6 +602,14 @@ static void dwc2_get_dev_hwparams(struct dwc2_hsotg
> *hsotg)
>
> gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
>
> + fifo_count = dwc2_hsotg_tx_fifo_count(hsotg);
> +
> + for (fifo = 1; fifo <= fifo_count; fifo++) {
> + hw->g_tx_fifo_size[fifo] =
> + (dwc2_readl(hsotg->regs + DPTXFSIZN(fifo)) &
> + FIFOSIZE_DEPTH_MASK) >> FIFOSIZE_DEPTH_SHIFT;
> + }
> +
> if (forced)
> dwc2_clear_force_mode(hsotg);
>
> @@ -646,14 +654,6 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
> hwcfg4 = dwc2_readl(hsotg->regs + GHWCFG4);
> grxfsiz = dwc2_readl(hsotg->regs + GRXFSIZ);
>
> - /*
> - * Host specific hardware parameters. Reading these parameters
> - * requires the controller to be in host mode. The mode will
> - * be forced, if necessary, to read these values.
> - */
> - dwc2_get_host_hwparams(hsotg);
> - dwc2_get_dev_hwparams(hsotg);
> -
> /* hwcfg1 */
> hw->dev_ep_dirs = hwcfg1;
>
> @@ -696,6 +696,8 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
> hw->en_multiple_tx_fifo = !!(hwcfg4 & GHWCFG4_DED_FIFO_EN);
> hw->num_dev_perio_in_ep = (hwcfg4 & GHWCFG4_NUM_DEV_PERIO_IN_EP_MASK) >>
> GHWCFG4_NUM_DEV_PERIO_IN_EP_SHIFT;
> + hw->num_dev_in_eps = (hwcfg4 & GHWCFG4_NUM_IN_EPS_MASK) >>
> + GHWCFG4_NUM_IN_EPS_SHIFT;
> hw->dma_desc_enable = !!(hwcfg4 & GHWCFG4_DESC_DMA);
> hw->power_optimized = !!(hwcfg4 & GHWCFG4_POWER_OPTIMIZ);
> hw->utmi_phy_data_width = (hwcfg4 & GHWCFG4_UTMI_PHY_DATA_WIDTH_MASK) >>
> @@ -704,6 +706,13 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
> /* fifo sizes */
> hw->rx_fifo_size = (grxfsiz & GRXFSIZ_DEPTH_MASK) >>
> GRXFSIZ_DEPTH_SHIFT;
> + /*
> + * Host specific hardware parameters. Reading these parameters
> + * requires the controller to be in host mode. The mode will
> + * be forced, if necessary, to read these values.
> + */
> + dwc2_get_host_hwparams(hsotg);
> + dwc2_get_dev_hwparams(hsotg);
>
> return 0;
> }
>
Acked-by: John Youn
John
Re: [PATCH v3 1/2] usb: dwc2: host: Don't retry NAKed transactions right away
On 12/05/2017 08:18 AM, Stefan Wahren wrote: > Hi Felipe, > Hi John, > > > Am 30.10.2017 um 18:08 schrieb Douglas Anderson: >> On rk3288-veyron devices on Chrome OS it was found that plugging in an >> Arduino-based USB device could cause the system to lockup, especially >> if the CPU Frequency was at one of the slower operating points (like >> 100 MHz / 200 MHz). >> >> Upon tracing, I found that the following was happening: >> * The USB device (full speed) was connected to a high speed hub and >>then to the rk3288. Thus, we were dealing with split transactions, >>which is all handled in software on dwc2. >> * Userspace was initiating a BULK IN transfer >> * When we sent the SSPLIT (to start the split transaction), we got an >>ACK. Good. Then we issued the CSPLIT. >> * When we sent the CSPLIT, we got back a NAK. We immediately (from >>the interrupt handler) started to retry and sent another SSPLIT. >> * The device kept NAKing our CSPLIT, so we kept ping-ponging between >>sending a SSPLIT and a CSPLIT, each time sending from the interrupt >>handler. >> * The handling of the interrupts was (because of the low CPU speed and >>the inefficiency of the dwc2 interrupt handler) was actually taking >>_longer_ than it took the other side to send the ACK/NAK. Thus we >>were _always_ in the USB interrupt routine. >> * The fact that USB interrupts were always going off was preventing >>other things from happening in the system. This included preventing >>the system from being able to transition to a higher CPU frequency. >> >> As I understand it, there is no requirement to retry super quickly >> after a NAK, we just have to retry sometime in the future. Thus one >> solution to the above is to just add a delay between getting a NAK and >> retrying the transmission. If this delay is sufficiently long to get >> out of the interrupt routine then the rest of the system will be able >> to make forward progress. Even a 25 us delay would probably be >> enough, but we'll be extra conservative and try to delay 1 ms (the >> exact amount depends on HZ and the accuracy of the jiffy and how close >> the current jiffy is to ticking, but could be as much as 20 ms or as >> little as 1 ms). >> >> Presumably adding a delay like this could impact the USB throughput, >> so we only add the delay with repeated NAKs. >> >> NOTE: Upon further testing of a pl2303 serial adapter, I found that >> this fix may help with problems there. Specifically I found that the >> pl2303 serial adapters tend to respond with a NAK when they have >> nothing to say and thus we end with this same sequence. >> >> Signed-off-by: Douglas Anderson >> Cc: sta...@vger.kernel.org >> Reviewed-by: Julius Werner >> Tested-by: Stefan Wahren >> --- >> >> Changes in v3: >> - Add tested-by for Stefan Wahren >> - Sent to Felipe Balbi as candiate to land this. >> - Add Cc for stable (it's always been broken so go as far is as easy) >> >> Changes in v2: >> - Address >> https://urldefense.proofpoint.com/v2/url?u=http-3A__crosreview.com_737520&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=Y_xpJ6Ks0XAK5_bQgmeQEvgKThZtPBQJ3cejNCGfEvM&s=olyPwyYvn_072esVwYxrCduKOKKJPUgc1YHX-CNhM1s&e= >> feedback >> > > does it need a resend? > You can add my acked-by: Acked-by: John Youn Regards, John
Re: [PATCHv2 2/7] usb: dwc2: add support for STM32F7 USB OTG HS
On 09/29/2017 07:22 AM, Amelie DELAUNAY wrote:
> Hi,
>
> Gentle ping for driver review submitted on August 28th.
>
> Thanks,
> Amelie
>
> On 08/28/2017 04:20 PM, Amelie Delaunay wrote:
>> This patch adds the dwc2_set_params function for STM32F7 USB OTG HS.
>>
>> Signed-off-by: Amelie Delaunay
>> ---
>> drivers/usb/dwc2/params.c | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>> index a3ffe97..d5b672d 100644
>> --- a/drivers/usb/dwc2/params.c
>> +++ b/drivers/usb/dwc2/params.c
>> @@ -136,6 +136,15 @@ static void dwc2_set_stm32f4x9_fsotg_params(struct
>> dwc2_hsotg *hsotg)
>> p->activate_stm_fs_transceiver = true;
>> }
>>
>> +static void dwc2_set_stm32f7_hsotg_params(struct dwc2_hsotg *hsotg)
>> +{
>> +struct dwc2_core_params *p = &hsotg->params;
>> +
>> +p->host_rx_fifo_size = 622;
>> +p->host_nperio_tx_fifo_size = 128;
>> +p->host_perio_tx_fifo_size = 256;
>> +}
>> +
>> const struct of_device_id dwc2_of_match_table[] = {
>> { .compatible = "brcm,bcm2835-usb", .data = dwc2_set_bcm_params },
>> { .compatible = "hisilicon,hi6220-usb", .data = dwc2_set_his_params },
>> @@ -154,6 +163,8 @@ const struct of_device_id dwc2_of_match_table[] = {
>> { .compatible = "st,stm32f4x9-fsotg",
>>.data = dwc2_set_stm32f4x9_fsotg_params },
>> { .compatible = "st,stm32f4x9-hsotg" },
>> +{ .compatible = "st,stm32f7-hsotg",
>> + .data = dwc2_set_stm32f7_hsotg_params },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
>>
Acked-by: John Youn
John
Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey
On 09/20/2017 12:57 PM, John Stultz wrote: > So here are a few dwc2 fixes that I've been using with HiKey. > I'm not totally sure these are all ideal, but they avoid edge case > issues that we have been running into with switching between > gadget mode and host mode. > > I'd guess the first two are potentially -stable material, and > the last might be worth sending to -stable too, as its a relatively > simple fix, but to my understanding the UDC state tracking has > always been broken so its not really a regression. But still. > > I'd love to get some feedback on the patches and consideration > to be merged upstream. > > thanks > -john > > Cc: Wei Xu > Cc: Guodong Xu > Cc: Amit Pundir > Cc: YongQin Liu > Cc: John Youn > Cc: Minas Harutyunyan > Cc: Douglas Anderson > Cc: Chen Yu > Cc: Felipe Balbi > Cc: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org > > John Stultz (3): > usb: dwc2: Improve gadget state disconnection handling > usb: dwc2: Error out of dwc2_hsotg_ep_disable() if we're in host mode > usb: dwc2: Fix UDC state tracking > > drivers/usb/dwc2/gadget.c | 7 +++ > drivers/usb/dwc2/hcd.c| 8 ++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > Hi John, I think we have something that fixes these issues. Minas, Could you take a look at this? I was not able to find the patches we talked about. If possible, please post them so that John can try them out. Thanks, John
Re: [PATCH] usb: dwc2: gadget: On USB RESET reset device address to zero
On 07/11/2017 03:25 AM, Minas Harutyunyan wrote: > Reseted DEVADDR field in DCFG to zero on USB RESET. > > Device address in DCFG register does not reset to zero, > which required to pass enumeration, after disconnect and > reconnect. > > Signed-off-by: Minas Harutyunyan > --- > drivers/usb/dwc2/gadget.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 98a4a79e7f6e..c5c0a26a4d66 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -3561,6 +3561,9 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw) > /* Report disconnection if it is not already done. */ > dwc2_hsotg_disconnect(hsotg); > > + /* Reset device address to zero */ > + __bic32(hsotg->regs + DCFG, DCFG_DEVADDR_MASK); > + > if (usb_status & GOTGCTL_BSESVLD && connected) > dwc2_hsotg_core_init_disconnected(hsotg, true); > } > Acked-by: John Youn John
Re: bug? dwc2: insufficient fifo memory
On 6/5/2017 5:32 AM, Minas Harutyunyan wrote:
> On 6/2/2017 10:20 PM, John Stultz wrote:
>> On Mon, Apr 17, 2017 at 3:36 PM, John Stultz wrote:
>>> On Fri, Feb 24, 2017 at 2:46 PM, John Stultz wrote:
Hey John,
So after the USB tree landed in 4.11-rc, I've been seeing the
following warning at bootup.
It seems the fifo_mem/total_fifo_size value on hikey is 1920, but I'm
seeing the addresses zip upward quickly as the txfsz values are all
2048. Not exactly sure whats wrong here. Things still seem to work
properly.
thanks
-john
[8.944987] dwc2 f72c.usb: bound driver configfs-gadget
[8.956651] insufficient fifo memory
[8.956703] [ cut here ]
[8.964906] WARNING: CPU: 7 PID: 1 at drivers/usb/dwc2/gadget.c:330
dwc2_hsotg_init_fifo+0x1a8/0x1c8
>>>
>>>
>>> Hey John,
>>>So I finally got a bit of time to look deeper into this, and it
>>> seems like this issue was introduced by commit 3c6aea7344c3 ("usb:
>>> dwc2: gadget: Add checking for g-tx-fifo-size parameter"), as that
>>> change added the following snippit:
>>>
>>>if (hsotg->params.g_tx_fifo_size[fifo] < min ||
>>>hsotg->params.g_tx_fifo_size[fifo] > dptxfszn) {
>>>dev_warn(hsotg->dev, "%s: Invalid parameter
>>> g_tx_fifo_size[%d]=%d\n",
>>> __func__, fifo,
>>> hsotg->params.g_tx_fifo_size[fifo]);
>>>hsotg->params.g_tx_fifo_size[fifo] = dptxfszn;
>>>}
>>>
>>> On HiKey, we have g-tx-fifo-size = <128 128 128 128 128 128> in the
>>> dtsi, and the fifo_mem value ends up initialized at 1920.
>>>
>>> Unfortunately, in the above, it sees other entries in the
>>> g_tx_fifo_size[] array are initialized to zero, which then causes them
>>> to be set to the "default" value of dptxfszn which is 2048. So then
>>> later in dwc2_hsotg_init_fifo() the addr value (which adds the
>>> fifo_size array value each step) quickly grows beyond the fifo_mem
>>> value, causing the warning.
>>>
>>> Not sure what the right fix is here? Should the min value be used
>>> instead of the "default" dptxfszn value? Again, I'm not sure I see
>>> any side-effects from this warning, but wanted to try to figure out
>>> how to fix it properly.
>>
>> Hey John, Minas,
>> Wanted to follow up on this again. I'm using a patch that looks as
>> follows (sorry for the copy-paste whitespace damage) in the meantime
>> to work around this issue:
>>
>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>> index 9cd8722..13a911b 100644
>> --- a/drivers/usb/dwc2/params.c
>> +++ b/drivers/usb/dwc2/params.c
>> @@ -475,7 +475,7 @@ static void dwc2_check_param_tx_fifo_sizes(struct
>> dwc2_hsotg *hsotg)
>> dev_warn(hsotg->dev, "%s: Invalid parameter
>> g_tx_fifo_size[%d]=%d\n",
>> __func__, fifo,
>> hsotg->params.g_tx_fifo_size[fifo]);
>> - hsotg->params.g_tx_fifo_size[fifo] = dptxfszn;
>> + hsotg->params.g_tx_fifo_size[fifo] = min;
>> }
>> }
>> }
>>
>> Any suggestions on what a proper fix would look like?
>>
>> thanks
>> -john
>>
>
> Hi John,
>
> The patch series: "[PATCH 0/4] usb: dwc2: gadget: Fix dynamic FIFO
> initialization flow" from Sevak Arakelyan submitted to LKML at 4/26/2017
> should resolve this issue.
>
> Thanks,
> Minas
>
Hi John,
See if this patch helps and let us know.
Unfortunately, the author of this patch (and expert on this part of
the code) has left Synopsys. So it might take us a bit to look into
this.
Regards,
John
Re: [PATCH v2 3/4] usb: dwc3: add dual-role support
On 03/30/2017 02:27 AM, Felipe Balbi wrote: > > Hi > > Roger Quadros writes: > For something that simple, we wouldn't even need to use OTG FSM layer > because that brings no benefit for such a simple requirement. no no. I think you got it wrong. I'm not using the OTG FSM layer at all :). >>> >>> what are all the otg_fsm mentions then? Also you have: >>> >>> + struct usb_otg otg; >>> + struct otg_fsm otg_fsm; >>> >> >> I'll get rid of them. They aren't really needed. >> Now I see why you thought I was using the otg_fsm layer. :) > > fair enough > > Can you either confirm or refute the statement above? The real problem was that if host adapter was removed during a system suspend then while resuming XHCI was locking up like below. This is probably because we're trying to remove/Halt the HCD in the otg_irq_handler before XHCI driver has resumed? How can we ensure that we call dwc3_host_exit() only *after* XHCI driver has resumed? >>> >>> Well, xHCI is our child, so driver model should *already* be >>> guaranteeing that, no? Specially since you're calling this from >>> ->complete() which happens after ->resume() has been called for the >>> entire tree. It's true, however, that dwc3's ->complete() will be called >>> before xhci's ->complete(). Is this the problem you're pointing at? Or >>> do you mean xHCI is runtime suspended (or runtime resuming) while you >>> call dwc3_host_exit()? If that's the case, then there's a bug in xHCI >>> itself. >> >> Yes dwc3->complete() being called before xhci->complete(), and so HCD being >> removed before xhci->complete() causes the lockup. >> >> It could be a bug in xHCI driver as well. > > I see... > We need a way to mask the OTG events without loosing them while they are masked. Do you know how that could be achieved? >>> >>> masking doesn't clear events. It just masks them. Look at gadget.c for >>> how we do it. Basically, the code we have here is racy, really racy and >>> will cause hard-to-debug lockups. Your code can only work with >>> IRQF_ONESHOT, which we don't want to add back. >>> >>> In any case, to mask events, you set BIT 31 in GEVNTSIZ register. Just >>> copy it from gadget.c ;-) >> >> Isn't GEVNTSIZ specific to device side? We're talking about the OTG block >> here. > > that's true, sorry. > >> Are you sure that setting bit 31 of GEVNTSIZ will mask OTG_irq? I don't >> think so. >> >> Note that OTG_IRQ is a separate IRQ line than PERIPHERAL_IRQ. > > man, there's really no way to mask OTG events. This can be bad :-) > > John, can you confirm if there's really no way of masking OTG events > without loosing them? Not sure. I'll have to verify. If the IP version is 3.00a or higher you could experiment with using interrupt moderation as a global mask. John
Re: dwc2_hc_chhltd_intr_dma - ChHltd set errors?
On 03/31/2017 04:04 PM, John Stultz wrote: > On Thu, Mar 2, 2017 at 12:00 PM, John Stultz wrote: >> Hey John, >> We've noticed that when using usb ethernet adapters on HiKey, we >> occasionally see errors like: >> >> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 0 - ChHltd set, >> but reason is unknown >> dwc2 f72c.usb: hcint 0x0002, intsts 0x06200029 >> >> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 11 - ChHltd set, >> but reason is unknown >> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029 >> >> Sometimes followed up by a usb error in the driver, something like: >> asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length 0x36000807, offset 68 >> >> Curious if you've seen any reports like this? > > Hey John, > Just wanted to check in again on this to see if you've seen anything > like it? I've not had too much time to debug it, but my attempts so > far haven't been productive. > I don't think I've seen that. We'll try to take a look this week. Thanks, John
Re: [PATCH v2] usb: dwc2: Make sure we disconnect the gadget state
On 03/29/2017 08:23 PM, John Stultz wrote: > I had seen some odd behavior with HiKey's usb-gadget interface > that I finally seemed to have chased down. Basically every other > time I plugged in the OTG port, the gadget interface would > properly initialize. The other times, I'd get a big WARN_ON > in dwc2_hsotg_init_fifo() about the fifo_map not being clear. > > Ends up if we don't disconnect the gadget state, the fifo-map > doesn't get cleared properly, which causes WARN_ON messages and > also results in the device not properly being setup as a gadget > every other time the OTG port is connected. > > So this patch adds a call to dwc2_hsotg_disconnect() in the > reset path so the state is properly cleared. > > With it, the gadget interface initializes properly on every > plug in. > > Cc: Wei Xu > Cc: Guodong Xu > Cc: Amit Pundir > Cc: Rob Herring > Cc: John Youn > Cc: Douglas Anderson > Cc: Chen Yu > Cc: Felipe Balbi > Cc: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org > Acked-by: John Youn > Signed-off-by: John Stultz > --- > v2: Minor typo fix suggested by Sergei Shtylyov > --- > drivers/usb/dwc2/hcd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index a73722e..91ed5b6 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -3264,6 +3264,7 @@ static void dwc2_conn_id_status_change(struct > work_struct *work) > dwc2_core_init(hsotg, false); > dwc2_enable_global_interrupts(hsotg); > spin_lock_irqsave(&hsotg->lock, flags); > + dwc2_hsotg_disconnect(hsotg); > dwc2_hsotg_core_init_disconnected(hsotg, false); > spin_unlock_irqrestore(&hsotg->lock, flags); > dwc2_hsotg_core_connect(hsotg); > Hi Felipe, You may want to use this patch to replace the one in your testing/next as this has a cleaner commit message. Thanks, John
Re: bug? dwc2: insufficient fifo memory
+linux-usb On 2/24/2017 2:46 PM, John Stultz wrote: > Hey John, > So after the USB tree landed in 4.11-rc, I've been seeing the > following warning at bootup. > > It seems the fifo_mem/total_fifo_size value on hikey is 1920, but I'm > seeing the addresses zip upward quickly as the txfsz values are all > 2048. Not exactly sure whats wrong here. Things still seem to work > properly. > Hi John, Could you send us a register dump before and after you load the function? You can get this through debugfs. Regards, John
Re: bug? dwc2: insufficient fifo memory
On 2/24/2017 2:46 PM, John Stultz wrote: > Hey John, > So after the USB tree landed in 4.11-rc, I've been seeing the > following warning at bootup. > > It seems the fifo_mem/total_fifo_size value on hikey is 1920, but I'm > seeing the addresses zip upward quickly as the txfsz values are all > 2048. Not exactly sure whats wrong here. Things still seem to work > properly. Thanks John. I'm not sure what could be the problem at the moment but sounds like something we broke with the default fifo initialization. We'll take a look at it. Regards, John > > > [8.944987] dwc2 f72c.usb: bound driver configfs-gadget > [8.956651] insufficient fifo memory > [8.956703] [ cut here ] > [8.964906] WARNING: CPU: 7 PID: 1 at drivers/usb/dwc2/gadget.c:330 > dwc2_hsotg_init_fifo+0x1a8/0x1c8 > [8.974041] > [8.975536] CPU: 7 PID: 1 Comm: init Not tainted > 4.10.0-09619-gcf8ef33-dirty #2280 > [8.983115] Hardware name: HiKey Development Board (DT) > [8.988344] task: ffc005f38000 task.stack: ffc005f34000 > [8.994283] PC is at dwc2_hsotg_init_fifo+0x1a8/0x1c8 > [8.999341] LR is at dwc2_hsotg_init_fifo+0x1a8/0x1c8 > [9.004408] pc : [] lr : [] > pstate: 61c5 > [9.011819] sp : ffc005f37be0 > [9.015138] x29: ffc005f37be0 x28: ffc005f38000 > [9.020471] x27: ff8008a52000 x26: 0001 > [9.025797] x25: ff8008c4d3f0 x24: ff8008dbdd3e > [9.031118] x23: 08000580 x22: 0580 > [9.036454] x21: ffc074ba7098 x20: ffc074ba7018 > [9.041781] x19: 011c x18: 0010 > [9.047116] x17: x16: ff80081d71c8 > [9.052443] x15: ff8088de639f x14: 0006 > [9.057763] x13: ff8008de63ad x12: ff8008d68820 > [9.063084] x11: x10: ffc005f37be0 > [9.068404] x9 : ffc005f37be0 x8 : 66696620746e6569 > [9.073726] x7 : 636975736e69 x6 : 034e > [9.079047] x5 : 0015 x4 : > [9.084382] x3 : 0002 x2 : 0002 > [9.089722] x1 : 0001 x0 : 0018 > [9.095051] > [9.096545] ---[ end trace 7e67e3086c7527eb ]--- > [9.101173] Call trace: > [9.103641] Exception stack(0xffc005f37a10 to 0xffc005f37b40) > [9.110112] 7a00: > 011c 0080 > [9.117954] 7a20: ffc005f37be0 ff8008699640 > ff8008c4d3f0 0001 > [9.125809] 7a40: ff8008a52000 ffc005f38000 > ffc005f37a90 > [9.133667] 7a60: ffc005f37be0 ffc005f37be0 > ffc005f37ba0 ffc8 > [9.141530] 7a80: ffc005f37ab0 ff8008105114 > ffc005f37be0 ffc005f37be0 > [9.149390] 7aa0: ffc005f37ba0 ffc8 > 0018 0001 > [9.157252] 7ac0: 0002 0002 > 0015 > [9.165114] 7ae0: 034e 636975736e69 > 66696620746e6569 ffc005f37be0 > [9.172957] 7b00: ffc005f37be0 > ff8008d68820 ff8008de63ad > [9.180811] 7b20: 0006 ff8088de639f > ff80081d71c8 > [9.188673] [] dwc2_hsotg_init_fifo+0x1a8/0x1c8 > [9.194803] [] > dwc2_hsotg_core_init_disconnected+0x7c/0x3d0 > [9.201970] [] dwc2_hsotg_pullup+0x90/0x100 > [9.207751] [] usb_udc_connect_control.isra.0+0x78/0x90 > [9.214572] [] udc_bind_to_driver+0xc4/0xe8 > [9.220349] [] usb_gadget_probe_driver+0xa0/0x150 > [9.226629] [] gadget_dev_desc_UDC_store+0xdc/0xf8 > [9.233015] [] configfs_write_file+0xc0/0x198 > [9.238969] [] __vfs_write+0x1c/0x100 > [9.244206] [] vfs_write+0xa0/0x1b0 > [9.249269] [] SyS_write+0x44/0xa0 > [9.254245] [] el0_svc_naked+0x24/0x28 >
Re: [RESEND PATCH v2 0/2] add multiple clock handling for dwc2 driver
On 2/21/2017 5:30 PM, Frank Wang wrote: > Hi John and Greg, > > Friendly ping... :-) > Hi Frank, We'll take a look at this and get back to you soon. Regards, John > On 2017/2/10 11:06, Frank Wang wrote: >> Resend this series to involve device tree maintainer and add 'Reviewed-by' >> tag for driver. >> >> The Current default dwc2 just handle one clock named otg, however, it may >> have >> two or more clock need to manage for some new SoCs(such as RK3328), so this >> adds change clk to clk's array of dwc2_hsotg to handle more clocks operation. >> >> Changes in v2: >> - amend dwc2 clocks property in DT. >> - change DWC2_MAX_CLKS to 5 and amend commit message. >> >> Frank Wang (2): >>Documentation: dt: dwc2: amend clocks property >>usb: dwc2: add multiple clocks handling >> >> Documentation/devicetree/bindings/usb/dwc2.txt | 13 +++-- >> drivers/usb/dwc2/core.h| 5 +++- >> drivers/usb/dwc2/platform.c| 39 >> +- >> 3 files changed, 40 insertions(+), 17 deletions(-) >> > > BR. > Frank > >
Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state
On 2/13/2017 8:08 PM, John Stultz wrote: > Just wanted to get some early feedback on this before I submit > it for real for the 4.12 timeframe. This is the last patch, that > isn't already queued, which I need to get hikey's USB working > properly. > > Feedback would be greatly appreciated! > It looks good to me. Acked-by: John Youn Regards, John > thanks > -john > > > I had seen some odd behavior with HiKey's usb-gadget interface > that I finally seemed to have chased down. Basically every other > time I pluged in the OTG port, the gadget interface would > properly initialize. The other times, I'd get a big WARN_ON > in dwc2_hsotg_init_fifo() about the fifo_map not being clear. > > Ends up if we don't disconnect the gadget state, the fifo-map > doesn't get cleared properly, which causes WARN_ON messages and > also results in the device not properly being setup as a gadget > every other time the OTG port is connected. > > So this patch adds a call to dwc2_hsotg_disconnect() in the > reset path so the state is properly cleared. > > With it, the gadget interface initializes properly on every > plug in. > > Cc: Wei Xu > Cc: Guodong Xu > Cc: Amit Pundir > Cc: Rob Herring > Cc: John Youn > Cc: Douglas Anderson > Cc: Chen Yu > Cc: Felipe Balbi > Cc: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org > Signed-off-by: John Stultz > --- > drivers/usb/dwc2/hcd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index 90bd248..067202d 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -3266,6 +3266,7 @@ static void dwc2_conn_id_status_change(struct > work_struct *work) > dwc2_core_init(hsotg, false); > dwc2_enable_global_interrupts(hsotg); > spin_lock_irqsave(&hsotg->lock, flags); > + dwc2_hsotg_disconnect(hsotg); > dwc2_hsotg_core_init_disconnected(hsotg, false); > spin_unlock_irqrestore(&hsotg->lock, flags); > dwc2_hsotg_core_connect(hsotg); >
Re: [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling
On 2/6/2017 7:18 PM, Frank Wang wrote:
> Hi Heiko, John and Greg,
>
> On 2017/2/7 8:06, Heiko Stuebner wrote:
>> Hi Frank,
>>
>> Am Sonntag, 5. Februar 2017, 10:51:01 CET schrieb Frank Wang:
>>> Originally, dwc2 just handle one clock named otg, however, it may have
>>> two or more clock need to manage for some new SoCs, so this adds
>>> change clk to clk's array of dwc2_hsotg to handle more clocks operation.
>>>
>>> Signed-off-by: Frank Wang
>>> ---
>>> drivers/usb/dwc2/core.h | 5 -
>>> drivers/usb/dwc2/platform.c | 39 ++-
>>> 2 files changed, 30 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index 1a7e830..d10a466 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -121,6 +121,9 @@ static inline void dwc2_writel(u32 value, void __iomem
>>> *addr) /* Maximum number of Endpoints/HostChannels */
>>> #define MAX_EPS_CHANNELS 16
>>>
>>> +/* Maximum number of dwc2 clocks */
>>> +#define DWC2_MAX_CLKS 3
>> why 3 clocks?
>
> Showing from the chapter 2.4 of dwc otg databook v3.10, it seems there
> should be five clocks, am I right?
> hclk/pmu_hclk/utmi_clk/ulpi_clk/utmifs_clk48
>
>> I.e. the binding currently only specifies the "otg" clock, so you should
>> definitly amend it to also specifiy this "pmu" clock - in a separate patch
>> before this one of course :-) .
>> "pmu" also looks like a good name for that clock-binding and these new clocks
>> of course should be optional in the binding.
>
> No problem, I will amend the binding when the implementation scheme is
> clear.
>
>> And ideally also just specify this mysterious third clock as well while
>> you're
>> at it.
>>
>>> +
>>> /* dwc2-hsotg declarations */
>>> static const char * const dwc2_hsotg_supply_names[] = {
>>> "vusb_d", /* digital USB supply, 1.2V */
>>> @@ -913,7 +916,7 @@ struct dwc2_hsotg {
>>> spinlock_t lock;
>>> void *priv;
>>> int irq;
>>> - struct clk *clk;
>>> + struct clk *clks[DWC2_MAX_CLKS];
>>> struct reset_control *reset;
>>>
>>> unsigned int queuing_high_bandwidth:1;
>> [...]
>>
>>> @@ -123,17 +123,20 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
>>> static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
>>> {
>>> struct platform_device *pdev = to_platform_device(hsotg->dev);
>>> - int ret;
>>> + int clk, ret;
>>>
>>> ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
>>> hsotg->supplies);
>>> if (ret)
>>> return ret;
>>>
>>> - if (hsotg->clk) {
>>> - ret = clk_prepare_enable(hsotg->clk);
>>> - if (ret)
>>> + for (clk = 0; clk < DWC2_MAX_CLKS && hsotg->clks[clk]; clk++) {
>>> + ret = clk_prepare_enable(hsotg->clks[clk]);
>>> + if (ret) {
>>> + while (--clk >= 0)
>>> + clk_disable_unprepare(hsotg->clks[clk]);
>>> return ret;
>>> + }
>>> }
>>>
>>> if (hsotg->uphy) {
>>> @@ -168,7 +171,7 @@ int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
>>> static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>>> {
>>> struct platform_device *pdev = to_platform_device(hsotg->dev);
>>> - int ret = 0;
>>> + int clk, ret = 0;
>>>
>>> if (hsotg->uphy) {
>>> usb_phy_shutdown(hsotg->uphy);
>>> @@ -182,8 +185,9 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg
>>> *hsotg) if (ret)
>>> return ret;
>>>
>>> - if (hsotg->clk)
>>> - clk_disable_unprepare(hsotg->clk);
>>> + for (clk = DWC2_MAX_CLKS - 1; clk >= 0; clk--)
>>> + if (hsotg->clks[clk])
>>> + clk_disable_unprepare(hsotg->clks[clk]);
>>> ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
>>> hsotg->supplies);
>>> @@ -209,7 +213,7 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>>>
>>> static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
>>> {
>>> - int i, ret;
>>> + int i, clk, ret;
>>>
>>> hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
>>> if (IS_ERR(hsotg->reset)) {
>>> @@ -282,11 +286,20 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
>>> *hsotg) hsotg->phyif = GUSBCFG_PHYIF8;
>>> }
>>>
>>> - /* Clock */
>>> - hsotg->clk = devm_clk_get(hsotg->dev, "otg");
>>> - if (IS_ERR(hsotg->clk)) {
>>> - hsotg->clk = NULL;
>>> - dev_dbg(hsotg->dev, "cannot get otg clock\n");
>>> + /* Clocks */
>>> + for (clk = 0; clk < DWC2_MAX_CLKS; clk++) {
>>> + hsotg->clks[clk] = of_clk_get(hsotg->dev->of_node, clk);
>>> + if (IS_ERR(hsotg->clks[clk])) {
>>> + ret = PTR_ERR(hsotg->clks[clk]);
>>> + if (ret == -EPROBE_DEFER) {
>>> + while (--clk >= 0)
>>> + clk_put(hsotg->c
Re: [PATCH 12/14] usb: dwc2: simplify optional reset handling
On 1/30/2017 4:28 PM, John Youn wrote:
> Hi Philipp,
>
> On 1/30/2017 3:42 AM, Philipp Zabel wrote:
>> As of commit bb475230b8e5 ("reset: make optional functions really
>
> Where can I find this? It's not in mainline.
>
Never mind. I found on arm-soc per your reply elsewhere.
Acked-by: John Youn
Regards,
John
>
>
>> optional"), the reset framework API calls use NULL pointers to describe
>> optional, non-present reset controls.
>>
>> This allows to return errors from devm_reset_control_get_optional and to
>> call reset_control_(de)assert unconditionally.
>>
>> Signed-off-by: Philipp Zabel
>> Cc: Dinh Nguyen
>> Cc: John Youn
>> Cc: Felipe Balbi
>> Cc: Greg Kroah-Hartman
>> ---
>> drivers/usb/dwc2/platform.c | 18 --
>> 1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 4fc8c603afb8b..c6aa2710cecfe 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -214,20 +214,11 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
>> *hsotg)
>> hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
>> if (IS_ERR(hsotg->reset)) {
>> ret = PTR_ERR(hsotg->reset);
>> -switch (ret) {
>> -case -ENOENT:
>> -case -ENOTSUPP:
>> -hsotg->reset = NULL;
>> -break;
>> -default:
>> -dev_err(hsotg->dev, "error getting reset control %d\n",
>> -ret);
>> -return ret;
>> -}
>> +dev_err(hsotg->dev, "error getting reset control %d\n", ret);
>> +return ret;
>> }
>>
>> -if (hsotg->reset)
>> -reset_control_deassert(hsotg->reset);
>> +reset_control_deassert(hsotg->reset);
>>
>> /* Set default UTMI width */
>> hsotg->phyif = GUSBCFG_PHYIF16;
>> @@ -326,8 +317,7 @@ static int dwc2_driver_remove(struct platform_device
>> *dev)
>> if (hsotg->ll_hw_enabled)
>> dwc2_lowlevel_hw_disable(hsotg);
>>
>> -if (hsotg->reset)
>> -reset_control_assert(hsotg->reset);
>> +reset_control_assert(hsotg->reset);
>>
>> return 0;
>> }
>>
>
>
Re: [PATCH 12/14] usb: dwc2: simplify optional reset handling
Hi Philipp,
On 1/30/2017 3:42 AM, Philipp Zabel wrote:
> As of commit bb475230b8e5 ("reset: make optional functions really
Where can I find this? It's not in mainline.
John
> optional"), the reset framework API calls use NULL pointers to describe
> optional, non-present reset controls.
>
> This allows to return errors from devm_reset_control_get_optional and to
> call reset_control_(de)assert unconditionally.
>
> Signed-off-by: Philipp Zabel
> Cc: Dinh Nguyen
> Cc: John Youn
> Cc: Felipe Balbi
> Cc: Greg Kroah-Hartman
> ---
> drivers/usb/dwc2/platform.c | 18 --
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 4fc8c603afb8b..c6aa2710cecfe 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -214,20 +214,11 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
> *hsotg)
> hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
> if (IS_ERR(hsotg->reset)) {
> ret = PTR_ERR(hsotg->reset);
> - switch (ret) {
> - case -ENOENT:
> - case -ENOTSUPP:
> - hsotg->reset = NULL;
> - break;
> - default:
> - dev_err(hsotg->dev, "error getting reset control %d\n",
> - ret);
> - return ret;
> - }
> + dev_err(hsotg->dev, "error getting reset control %d\n", ret);
> + return ret;
> }
>
> - if (hsotg->reset)
> - reset_control_deassert(hsotg->reset);
> + reset_control_deassert(hsotg->reset);
>
> /* Set default UTMI width */
> hsotg->phyif = GUSBCFG_PHYIF16;
> @@ -326,8 +317,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
> if (hsotg->ll_hw_enabled)
> dwc2_lowlevel_hw_disable(hsotg);
>
> - if (hsotg->reset)
> - reset_control_assert(hsotg->reset);
> + reset_control_assert(hsotg->reset);
>
> return 0;
> }
>
Re: [PATCH] usb: dwc3: core: Disable USB2.0 phy suspend when dwc3 acts as host role
On 1/18/2017 7:12 PM, Baolin Wang wrote: > Hi John, > > On 19 January 2017 at 09:33, John Youn wrote: >> On 1/16/2017 2:38 AM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> John Youn writes: >>>>> Baolin Wang writes: >>>>>>> Baolin Wang writes: >>>>>>>> When dwc3 controller acts as host role with attaching slow speed device >>>>>>>> (like mouse or keypad). Then if we plugged out the slow speed device, >>>>>>>> it will timeout to run the deconfiguration endpoint command to drop the >>>>>>>> endpoint's resources. Some xHCI command timeout log as below when >>>>>>>> disconnecting one slow device: >>>>>>>> >>>>>>>> [ 99.807739] c0 xhci-hcd.0.auto: Port Status Change Event for port 1 >>>>>>>> [ 99.814699] c0 xhci-hcd.0.auto: resume root hub >>>>>>>> [ 99.819992] c0 xhci-hcd.0.auto: handle_port_status: starting port >>>>>>>> polling. >>>>>>>> [ 99.827808] c0 xhci-hcd.0.auto: get port status, actual port 0 >>>>>>>> status >>>>>>>> = 0x202a0 >>>>>>>> [ 99.835903] c0 xhci-hcd.0.auto: Get port status returned 0x10100 >>>>>>>> [ 99.850052] c0 xhci-hcd.0.auto: clear port connect change, actual >>>>>>>> port 0 status = 0x2a0 >>>>>>>> [ 99.859313] c0 xhci-hcd.0.auto: Cancel URB ffc01ed6cd00, dev 1, >>>>>>>> ep 0x81, starting at offset 0xc406d210 >>>>>>>> [ 99.869645] c0 xhci-hcd.0.auto: // Ding dong! >>>>>>>> [ 99.874776] c0 xhci-hcd.0.auto: Stopped on Transfer TRB >>>>>>>> [ 99.880713] c0 xhci-hcd.0.auto: Removing canceled TD starting at >>>>>>>> 0xc406d210 (dma). >>>>>>>> [ 99.889012] c0 xhci-hcd.0.auto: Finding endpoint context >>>>>>>> [ 99.895069] c0 xhci-hcd.0.auto: Cycle state = 0x1 >>>>>>>> [ 99.900519] c0 xhci-hcd.0.auto: New dequeue segment = >>>>>>>> ffc1112f0880 (virtual) >>>>>>>> [ 99.908655] c0 xhci-hcd.0.auto: New dequeue pointer = 0xc406d220 >>>>>>>> (DMA) >>>>>>>> [ 99.915927] c0 xhci-hcd.0.auto: Set TR Deq Ptr cmd, new deq seg = >>>>>>>> ffc1112f0880 (0xc406d000 dma), >>>>>>>> new deq ptr = ff8002175220 >>>>>>>> (0xc406d220 dma), new cycle = 1 >>>>>>>> [ 99.931242] c0 xhci-hcd.0.auto: // Ding dong! >>>>>>>> [ 99.936360] c0 xhci-hcd.0.auto: Successful Set TR Deq Ptr cmd, >>>>>>>> deq = @c406d220 >>>>>>>> [ 99.944458] c0 xhci-hcd.0.auto: xhci_hub_status_data: stopping port >>>>>>>> polling. >>>>>>>> [ 100.047619] c0 xhci-hcd.0.auto: xhci_drop_endpoint called for udev >>>>>>>> ffc01ae08800 >>>>>>>> [ 100.057002] c0 xhci-hcd.0.auto: drop ep 0x81, slot id 1, new drop >>>>>>>> flags = 0x8, new add flags = 0x0 >>>>>>>> [ 100.067878] c0 xhci-hcd.0.auto: xhci_check_bandwidth called for udev >>>>>>>> ffc01ae08800 >>>>>>>> [ 100.076868] c0 xhci-hcd.0.auto: New Input Control Context: >>>>>>>> >>>>>>>> .. >>>>>>>> >>>>>>>> [ 100.427252] c0 xhci-hcd.0.auto: // Ding dong! >>>>>>>> [ 105.430728] c0 xhci-hcd.0.auto: Command timeout >>>>>>>> [ 105.436029] c0 xhci-hcd.0.auto: Abort command ring >>>>>>>> [ 113.558223] c0 xhci-hcd.0.auto: Command completion event does not >>>>>>>> match >>>>>>>> command >>>>>>>> [ 113.569778] c0 xhci-hcd.0.auto: Timeout while waiting for configure >>>>&g
Re: [PATCH] usb: dwc3: core: Disable USB2.0 phy suspend when dwc3 acts as host role
On 1/16/2017 2:38 AM, Felipe Balbi wrote: > > Hi, > > John Youn writes: >>> Baolin Wang writes: >>>>> Baolin Wang writes: >>>>>> When dwc3 controller acts as host role with attaching slow speed device >>>>>> (like mouse or keypad). Then if we plugged out the slow speed device, >>>>>> it will timeout to run the deconfiguration endpoint command to drop the >>>>>> endpoint's resources. Some xHCI command timeout log as below when >>>>>> disconnecting one slow device: >>>>>> >>>>>> [ 99.807739] c0 xhci-hcd.0.auto: Port Status Change Event for port 1 >>>>>> [ 99.814699] c0 xhci-hcd.0.auto: resume root hub >>>>>> [ 99.819992] c0 xhci-hcd.0.auto: handle_port_status: starting port >>>>>> polling. >>>>>> [ 99.827808] c0 xhci-hcd.0.auto: get port status, actual port 0 status >>>>>> = 0x202a0 >>>>>> [ 99.835903] c0 xhci-hcd.0.auto: Get port status returned 0x10100 >>>>>> [ 99.850052] c0 xhci-hcd.0.auto: clear port connect change, actual >>>>>> port 0 status = 0x2a0 >>>>>> [ 99.859313] c0 xhci-hcd.0.auto: Cancel URB ffc01ed6cd00, dev 1, >>>>>> ep 0x81, starting at offset 0xc406d210 >>>>>> [ 99.869645] c0 xhci-hcd.0.auto: // Ding dong! >>>>>> [ 99.874776] c0 xhci-hcd.0.auto: Stopped on Transfer TRB >>>>>> [ 99.880713] c0 xhci-hcd.0.auto: Removing canceled TD starting at >>>>>> 0xc406d210 (dma). >>>>>> [ 99.889012] c0 xhci-hcd.0.auto: Finding endpoint context >>>>>> [ 99.895069] c0 xhci-hcd.0.auto: Cycle state = 0x1 >>>>>> [ 99.900519] c0 xhci-hcd.0.auto: New dequeue segment = >>>>>> ffc1112f0880 (virtual) >>>>>> [ 99.908655] c0 xhci-hcd.0.auto: New dequeue pointer = 0xc406d220 (DMA) >>>>>> [ 99.915927] c0 xhci-hcd.0.auto: Set TR Deq Ptr cmd, new deq seg = >>>>>> ffc1112f0880 (0xc406d000 dma), >>>>>> new deq ptr = ff8002175220 >>>>>> (0xc406d220 dma), new cycle = 1 >>>>>> [ 99.931242] c0 xhci-hcd.0.auto: // Ding dong! >>>>>> [ 99.936360] c0 xhci-hcd.0.auto: Successful Set TR Deq Ptr cmd, >>>>>> deq = @c406d220 >>>>>> [ 99.944458] c0 xhci-hcd.0.auto: xhci_hub_status_data: stopping port >>>>>> polling. >>>>>> [ 100.047619] c0 xhci-hcd.0.auto: xhci_drop_endpoint called for udev >>>>>> ffc01ae08800 >>>>>> [ 100.057002] c0 xhci-hcd.0.auto: drop ep 0x81, slot id 1, new drop >>>>>> flags = 0x8, new add flags = 0x0 >>>>>> [ 100.067878] c0 xhci-hcd.0.auto: xhci_check_bandwidth called for udev >>>>>> ffc01ae08800 >>>>>> [ 100.076868] c0 xhci-hcd.0.auto: New Input Control Context: >>>>>> >>>>>> .. >>>>>> >>>>>> [ 100.427252] c0 xhci-hcd.0.auto: // Ding dong! >>>>>> [ 105.430728] c0 xhci-hcd.0.auto: Command timeout >>>>>> [ 105.436029] c0 xhci-hcd.0.auto: Abort command ring >>>>>> [ 113.558223] c0 xhci-hcd.0.auto: Command completion event does not >>>>>> match >>>>>> command >>>>>> [ 113.569778] c0 xhci-hcd.0.auto: Timeout while waiting for configure >>>>>> endpoint command >>>>>> >>>>>> The reason is it will suspend USB phy to disable phy clock when >>>>>> disconnecting the slow USB decice, that will hang on the xHCI commands >>>>>> executing which depends on the phy clock. >>>>>> >>>>>> Thus we should disable USB2.0 phy suspend feature when dwc3 acts as host >>>>>> role. >>>>>> >>>>>> Signed-off-by: Baolin Wang >>>>>> --- >>>>>> drivers/usb/dwc3/core.c | 14 ++
Re: [PATCH 1/4 v3] usb: dwc2: Avoid sleeping while holding hsotg->lock
> On Jan 16, 2017, at 12:37 PM, John Stultz wrote: > > On Mon, Jan 16, 2017 at 2:36 AM, Felipe Balbi > wrote: >> >> Hi, >> >> John Stultz writes: >>> Basically when plugging in various cables in different orders, I'm >>> occasionally seeing the following BUG splat: >>> >>> [ 86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x0002 >>> [ 86.219164] usb 1-1: USB disconnect, device number 9 >>> [ 86.226845] Preemption disabled at:[ 86.230218] >>> [] dwc2_conn_id_status_change+0x120/0x250 >>> [ 86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: GW >>> 4.9.0-rc8-00051-gd5a7979-dirty #1702 >>> [ 86.246836] Hardware name: HiKey Development Board (DT) >>> [ 86.252100] Workqueue: dwc2 dwc2_conn_id_status_change >>> [ 86.257279] Call trace: >>> [ 86.259771] [] dump_backtrace+0x0/0x1a0 >>> [ 86.265210] [] show_stack+0x14/0x20 >>> [ 86.270308] [] dump_stack+0x90/0xb0 >>> [ 86.275401] [] __schedule_bug+0x6c/0xb8 >>> [ 86.280841] [] __schedule+0x4f8/0x5b0 >>> [ 86.286099] [] schedule+0x38/0xa0 >>> [ 86.291017] [] schedule_hrtimeout_range_clock+0x8c/0xf0 >>> [ 86.297846] [] schedule_hrtimeout_range+0x10/0x18 >>> [ 86.304150] [] usleep_range+0x50/0x58 >>> [ 86.309418] [] dwc2_wait_for_mode.isra.4+0x54/0xd0 >>> [ 86.315815] [] dwc2_core_reset+0xe0/0x168 >>> [ 86.321431] [] >>> dwc2_hsotg_core_init_disconnected+0x2c/0x310 >>> [ 86.328602] [] dwc2_conn_id_status_change+0x130/0x250 >>> [ 86.335254] [] process_one_work+0x118/0x370 >>> [ 86.341035] [] worker_thread+0x48/0x498 >>> [ 86.346473] [] kthread+0xd0/0xe8 >>> [ 86.351299] [] ret_from_fork+0x10/0x50 >>> >>> This seems to be caused by the dwc2_wait_for_mode() calling >>> usleep_range() while the hstog->lock spinlock is held, since >>> we take that before calling dwc2_hsotg_core_init_disconnected(). >>> >>> This patch avoids the issue by adding an extra argument to >>> dwc2_core_reset(), as suggested by John Youn, which allows us to >>> skip the waiting, which should be unnecessary when calling from >>> dwc2_hsotg_core_init_disconnected(). >>> >>> Cc: Wei Xu >>> Cc: Guodong Xu >>> Cc: Amit Pundir >>> Cc: Rob Herring >>> Cc: John Youn >>> Cc: Douglas Anderson >>> Cc: Chen Yu >>> Cc: Vardan Mikayelyan >>> Cc: Kishon Vijay Abraham I >>> Cc: Felipe Balbi >>> Cc: Greg Kroah-Hartman >>> Cc: linux-...@vger.kernel.org >>> Signed-off-by: John Stultz >>> --- >> >> doesn't apply to my testing/next. Please rebase > > So these were rebased onto JohnY's tree here: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_synopsys-2Dusb_linux.git&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=eBJTBuJyU21iKJvHdy5FxqtxsVARo0iIqJGxMHrlbyQ&s=VIiAT32aG7s04G5NoOOthNdm2JX0eWjJpg62neY_-KI&e= > next > > And apparently have been merged there. I suspect he's going to submit > his entire tree there to you? > > JohnY: Is this right? Yeah I'll get these issues sorted out with Felipe. Which may mean resubmitting everything the the proper order. Thanks, John
Re: [PATCH] usb: dwc2: host: use true/false for boolean
On 1/12/2017 7:53 AM, Nicholas Mc Guire wrote:
> For boolean variables true/false is preferred over 1/0 for readability.
>
> Signed-off-by: Nicholas Mc Guire
> ---
> Problem reported by scripts/coccinelle/misc/boolinit.cocci:
> ./drivers/usb/dwc2/hcd.c:5003:2-24: WARNING: Assignment of bool to 0/1
> ./drivers/usb/dwc2/hcd.c:3397:1-21: WARNING: Assignment of bool to 0/1
> ./drivers/usb/dwc2/hcd.c:3335:1-21: WARNING: Assignment of bool to 0/1
> ./drivers/usb/dwc2/hcd.c:2970:3-17: WARNING: Assignment of bool to 0/1
> ./drivers/usb/dwc2/hcd.c:2999:3-16: WARNING: Assignment of bool to 0/1
> ./drivers/usb/dwc2/hcd.c:3299:1-21: WARNING: Assignment of bool to 0/1
>
> Patch was compile tested with: x86_64_defconfig + CONFIG_USB_DWC2
>
> Patch is against 4.10-rc3 (localversion-next is next-20170112)
>
> drivers/usb/dwc2/hcd.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 9f66777..7467a37 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -2967,7 +2967,7 @@ static void dwc2_process_periodic_channels(struct
> dwc2_hsotg *hsotg)
> qspcavail = (tx_status & TXSTS_QSPCAVAIL_MASK) >>
> TXSTS_QSPCAVAIL_SHIFT;
> if (qspcavail == 0) {
> - no_queue_space = 1;
> + no_queue_space = true;
> break;
> }
>
> @@ -2996,7 +2996,7 @@ static void dwc2_process_periodic_channels(struct
> dwc2_hsotg *hsotg)
> TXSTS_FSPCAVAIL_SHIFT;
> status = dwc2_queue_transaction(hsotg, qh->channel, fspcavail);
> if (status < 0) {
> - no_fifo_space = 1;
> + no_fifo_space = true;
> break;
> }
>
> @@ -3296,7 +3296,7 @@ static void dwc2_wakeup_detected(unsigned long data)
> dwc2_readl(hsotg->regs + HPRT0));
>
> dwc2_hcd_rem_wakeup(hsotg);
> - hsotg->bus_suspended = 0;
> + hsotg->bus_suspended = false;
>
> /* Change to L0 state */
> hsotg->lx_state = DWC2_L0;
> @@ -3332,7 +3332,7 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg,
> u16 windex)
> hprt0 |= HPRT0_SUSP;
> dwc2_writel(hprt0, hsotg->regs + HPRT0);
>
> - hsotg->bus_suspended = 1;
> + hsotg->bus_suspended = true;
>
> /*
>* If hibernation is supported, Phy clock will be suspended
> @@ -3394,7 +3394,7 @@ static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
> hprt0 = dwc2_read_hprt0(hsotg);
> hprt0 &= ~(HPRT0_RES | HPRT0_SUSP);
> dwc2_writel(hprt0, hsotg->regs + HPRT0);
> - hsotg->bus_suspended = 0;
> + hsotg->bus_suspended = false;
> spin_unlock_irqrestore(&hsotg->lock, flags);
> }
>
> @@ -5000,7 +5000,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
> hsotg->dev->dma_mask == NULL) {
> dev_warn(hsotg->dev,
>"dma_mask not set, disabling DMA\n");
> - hsotg->params.host_dma = 0;
> + hsotg->params.host_dma = false;
> hsotg->params.dma_desc_enable = 0;
> }
>
>
+Felipe
Acked-by: John Youn
This one also required fix-up for next.
Regards,
John
Re: [PATCH] usb: dwc2: host: use msleep() for long delays
On 1/12/2017 7:52 AM, Nicholas Mc Guire wrote:
> ulseep_range() uses hrtimers and provides no advantage over msleep()
> for larger delays. Fix up the 20+ ms delays here passing the adjusted "min"
> value to msleep(). This helps reduce the load on the hrtimer subsystem.
>
> Signed-off-by: Nicholas Mc Guire
> ---
>
> Patch was compile tested with: x86_64_defconfig + CONFIG_USB_DWC2
> (1 sparse and 6 coccinelle warning - preparing separate patches for that)
>
> Patch is against 4.10-rc3 (localversion-next is next-20170112)
>
> drivers/usb/dwc2/hcd.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 911c3b3..37ee72d 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -2150,7 +2150,7 @@ static int dwc2_hcd_endpoint_disable(struct dwc2_hsotg
> *hsotg,
> }
>
> spin_unlock_irqrestore(&hsotg->lock, flags);
> - usleep_range(2, 4);
> + msleep(20);
> spin_lock_irqsave(&hsotg->lock, flags);
> qh = ep->hcpriv;
> if (!qh) {
> @@ -3240,7 +3240,7 @@ static void dwc2_conn_id_status_change(struct
> work_struct *work)
>"Waiting for Peripheral Mode, Mode=%s\n",
>dwc2_is_host_mode(hsotg) ? "Host" :
>"Peripheral");
> - usleep_range(2, 4);
> + msleep(20);
> if (++count > 250)
> break;
> }
> @@ -3261,7 +3261,7 @@ static void dwc2_conn_id_status_change(struct
> work_struct *work)
> dev_info(hsotg->dev, "Waiting for Host Mode, Mode=%s\n",
>dwc2_is_host_mode(hsotg) ?
>"Host" : "Peripheral");
> - usleep_range(2, 4);
> + msleep(20);
> if (++count > 250)
> break;
> }
> @@ -3354,7 +3354,7 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg,
> u16 windex)
>
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
> - usleep_range(20, 25);
> + msleep(200);
> } else {
> spin_unlock_irqrestore(&hsotg->lock, flags);
> }
> @@ -3378,7 +3378,7 @@ static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
> pcgctl &= ~PCGCTL_STOPPCLK;
> dwc2_writel(pcgctl, hsotg->regs + PCGCTL);
> spin_unlock_irqrestore(&hsotg->lock, flags);
> - usleep_range(2, 4);
> + msleep(20);
> spin_lock_irqsave(&hsotg->lock, flags);
> }
>
> @@ -3691,7 +3691,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg
> *hsotg, u16 typereq,
> }
>
> /* Clear reset bit in 10ms (FS/LS) or 50ms (HS) */
> - usleep_range(5, 7);
> + msleep(50);
> hprt0 &= ~HPRT0_RST;
> dwc2_writel(hprt0, hsotg->regs + HPRT0);
> hsotg->lx_state = DWC2_L0; /* Now back to On state */
>
+Felipe
Acked-by: John Youn
I've also fixed this up locally to apply against recent dwc2 patches
for next.
Hi Felipe,
If/when you queue the pending dwc2 patches on testing/next I can
resend this to you.
Regards,
John
Re: [PATCH RFC] usb: dwc2: host: use msleep() for long delay
On 1/12/2017 7:15 AM, Nicholas Mc Guire wrote:
> ulseep_range() uses hrtimers and provides no advantage over msleep()
> for larger delays. Fix up the 100ms delays here passing the adjusted "min"
> value to msleep(). This helps reduce the load on the hrtimer subsystem.
>
> Link:
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.org_lkml_2017_1_11_377&d=DgIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=dvQDbbH_QF2kZjARIDPimFAK95z8eOnw04feJuHCrJg&s=Prc0m1oUG4u8n8TFK-fdgzitMzE2ep6pM3L6y_DQeRs&e=
>
> Fixes: commit 2938fc63e0c2 ("usb: dwc2: Properly account for the force mode
> delays")
> Signed-off-by: Nicholas Mc Guire
> ---
> Problem was found by cocinelle script.
>
> Note that this originally was an msleep(25) then commit 2938fc63e0c2
> ("usb: dwc2: Properly account for the force mode delays") changed this
> to usleep_range(10,11) with the comment in the function head:
>
> After clearing the bits, wait up to 100 ms to account for any
> potential IDDIG filter delay...
>
> but without explaining why the API was switched to usleep_range() here
> It does not look like there is any reason for using a high resolution
> timer here - the stated requirement is clearly satisfied by msleep(100)
> as well. But it originally was usleep_range(15, 16) in
> commit 09c96980dc72 ("usb: dwc2: Add functions to set and clear force mode")
> so not sure if I am not overlooking some reason for the highrestimer here ?
>
> This needs to be clarified by someone that knows the details and history
> of this device/driver.
>
> Patch was compile tested with: x86_64_defconfig + CONFIG_USB_DWC2
>
> Patch is aginast 4.10-rc3 (localversion-next is next-20170112)
>
> drivers/usb/dwc2/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 11d8ae9..439a21b 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -455,7 +455,7 @@ void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
> dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
>
> if (dwc2_iddig_filter_enabled(hsotg))
> - usleep_range(10, 11);
> + msleep(100);
> }
>
> /*
>
+Felipe
Acked-by: John Youn
John
Re: [PATCH V2] usb: dwc2: host: fix Wmaybe-uninitialized warning
On 1/12/2017 8:32 AM, Nicholas Mc Guire wrote:
> Uninitialized char* causes a sparse build-warning, fix it up by
> initializing it to NULL.
>
> Signed-off-by: Nicholas Mc Guire
> ---
>
> V2: add missing change-log as requested by Greg Kroah-Hartman
>
>
> Problem reported by sparse
> drivers/usb/dwc2/hcd.c: In function 'dwc2_dump_urb_info':
> ./include/linux/dynamic_debug.h:134:3: warning: 'pipetype' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>__dynamic_dev_dbg(&descriptor, dev, fmt, \
>^
> drivers/usb/dwc2/hcd.c:4492:8: note: 'pipetype' was declared here
> char *pipetype;
> ^
> Patch was compile tested with: x86_64_defconfig + CONFIG_USB_DWC2=m +
> CONFIG_USB_DWC2_VERBOSE=y
>
> Patch is against 4.10-rc3 (localversion-next is next-20170112)
>
> drivers/usb/dwc2/hcd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 911c3b3..9f66777 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4489,8 +4489,8 @@ static void dwc2_dump_urb_info(struct usb_hcd *hcd,
> struct urb *urb,
> {
> #ifdef VERBOSE_DEBUG
> struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> - char *pipetype;
> - char *speed;
> + char *pipetype = NULL;
> + char *speed = NULL;
>
> dev_vdbg(hsotg->dev, "%s, urb %p\n", fn_name, urb);
> dev_vdbg(hsotg->dev, " Device address: %d\n",
>
+Felipe
Acked-by: John Youn
John
Re: [PATCH] usb: dwc2: gadget: Fix GUSBCFG.USBTRDTIM value
On 1/12/2017 7:41 AM, Amelie DELAUNAY wrote: > Hi all, > Sorry, I did not see Pengcheng Li patch which is exactly the same: > https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9347979_&d=DgIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=6VylcKBQDS2RlneASNqb6vUEW48snVAZ1f_mhtzcSaE&s=iYEesI5W2GVN4jBs6pzyRC7Sb0RAng4gpapDsoZQG_s&e= > > > Regards > Hi Felipe, Could you take either one? Regards, John > On 01/12/2017 04:36 PM, Amelie Delaunay wrote: >> USBTrdTim must be programmed to 0x5 when phy has a UTMI+ 16-bit wide >> interface or 0x9 when it has a 8-bit wide interface. >> GUSBCFG reset value (Value After Reset: 0x1400) sets USBTrdTim to 0x5. >> In case of 8-bit UTMI+, without clearing GUSBCFG.USBTRDTIM mask, USBTrdTim >> results in 0xD (0x5 | 0x9). >> That's why we need to clear GUSBCFG.USBTRDTIM mask before setting USBTrdTim >> value, to ensure USBTrdTim is correctly set in case of 8-bit UTMI+. >> >> Signed-off-by: Amelie Delaunay >> --- >> drivers/usb/dwc2/gadget.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index c55db4a..86b2076 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -3169,7 +3169,7 @@ void dwc2_hsotg_core_init_disconnected(struct >> dwc2_hsotg *hsotg, >> /* keep other bits untouched (so e.g. forced modes are not lost) */ >> usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); >> usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_PHYIF16 | GUSBCFG_SRPCAP | >> -GUSBCFG_HNPCAP); >> +GUSBCFG_HNPCAP | GUSBCFG_USBTRDTIM_MASK); >> >> if (hsotg->params.phy_type == DWC2_PHY_TYPE_PARAM_FS && >> (hsotg->params.speed == DWC2_SPEED_PARAM_FULL || >> @@ -4131,7 +4131,7 @@ static void dwc2_hsotg_init(struct dwc2_hsotg *hsotg) >> /* keep other bits untouched (so e.g. forced modes are not lost) */ >> usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); >> usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_PHYIF16 | GUSBCFG_SRPCAP | >> -GUSBCFG_HNPCAP); >> +GUSBCFG_HNPCAP | GUSBCFG_USBTRDTIM_MASK); >> >> /* set the PLL on, remove the HNP/SRP and set the PHY */ >> trdtim = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5; >> >
Re: [PATCH] usb: dwc3: core: Disable USB2.0 phy suspend when dwc3 acts as host role
On 1/11/2017 11:51 PM, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >>> Baolin Wang writes: When dwc3 controller acts as host role with attaching slow speed device (like mouse or keypad). Then if we plugged out the slow speed device, it will timeout to run the deconfiguration endpoint command to drop the endpoint's resources. Some xHCI command timeout log as below when disconnecting one slow device: [ 99.807739] c0 xhci-hcd.0.auto: Port Status Change Event for port 1 [ 99.814699] c0 xhci-hcd.0.auto: resume root hub [ 99.819992] c0 xhci-hcd.0.auto: handle_port_status: starting port polling. [ 99.827808] c0 xhci-hcd.0.auto: get port status, actual port 0 status = 0x202a0 [ 99.835903] c0 xhci-hcd.0.auto: Get port status returned 0x10100 [ 99.850052] c0 xhci-hcd.0.auto: clear port connect change, actual port 0 status = 0x2a0 [ 99.859313] c0 xhci-hcd.0.auto: Cancel URB ffc01ed6cd00, dev 1, ep 0x81, starting at offset 0xc406d210 [ 99.869645] c0 xhci-hcd.0.auto: // Ding dong! [ 99.874776] c0 xhci-hcd.0.auto: Stopped on Transfer TRB [ 99.880713] c0 xhci-hcd.0.auto: Removing canceled TD starting at 0xc406d210 (dma). [ 99.889012] c0 xhci-hcd.0.auto: Finding endpoint context [ 99.895069] c0 xhci-hcd.0.auto: Cycle state = 0x1 [ 99.900519] c0 xhci-hcd.0.auto: New dequeue segment = ffc1112f0880 (virtual) [ 99.908655] c0 xhci-hcd.0.auto: New dequeue pointer = 0xc406d220 (DMA) [ 99.915927] c0 xhci-hcd.0.auto: Set TR Deq Ptr cmd, new deq seg = ffc1112f0880 (0xc406d000 dma), new deq ptr = ff8002175220 (0xc406d220 dma), new cycle = 1 [ 99.931242] c0 xhci-hcd.0.auto: // Ding dong! [ 99.936360] c0 xhci-hcd.0.auto: Successful Set TR Deq Ptr cmd, deq = @c406d220 [ 99.944458] c0 xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling. [ 100.047619] c0 xhci-hcd.0.auto: xhci_drop_endpoint called for udev ffc01ae08800 [ 100.057002] c0 xhci-hcd.0.auto: drop ep 0x81, slot id 1, new drop flags = 0x8, new add flags = 0x0 [ 100.067878] c0 xhci-hcd.0.auto: xhci_check_bandwidth called for udev ffc01ae08800 [ 100.076868] c0 xhci-hcd.0.auto: New Input Control Context: .. [ 100.427252] c0 xhci-hcd.0.auto: // Ding dong! [ 105.430728] c0 xhci-hcd.0.auto: Command timeout [ 105.436029] c0 xhci-hcd.0.auto: Abort command ring [ 113.558223] c0 xhci-hcd.0.auto: Command completion event does not match command [ 113.569778] c0 xhci-hcd.0.auto: Timeout while waiting for configure endpoint command The reason is it will suspend USB phy to disable phy clock when disconnecting the slow USB decice, that will hang on the xHCI commands executing which depends on the phy clock. Thus we should disable USB2.0 phy suspend feature when dwc3 acts as host role. Signed-off-by: Baolin Wang --- drivers/usb/dwc3/core.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 9a4a5e4..0b646cf 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -565,6 +565,20 @@ static int dwc3_phy_setup(struct dwc3 *dwc) if (dwc->revision > DWC3_REVISION_194A) reg |= DWC3_GUSB2PHYCFG_SUSPHY; + /* + * When dwc3 controller acts as host role with attaching one slow speed + * device (like mouse or keypad). Then if we plugged out the slow speed + * device, it will timeout to run the deconfiguration endpoint command. + * The reason is it will suspend USB phy to disable phy clock when + * disconnecting slow speed decice, which will affect the xHCI commands + * executing. + * + * Thus we should disable USB 2.0 phy suspend feature when dwc3 acts as + * host role. + */ + if (dwc->dr_mode == USB_DR_MODE_HOST || dwc->dr_mode == USB_DR_MODE_OTG) + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; >>> >>> which version of the core you're using? Recent version (since 1.94A, >> >> My version is 2.80a. >> >>> IIRC) can manage core suspend automatically. Also, this patch of yours >>> w
Re: [PATCH 0/4 v3] Fixes and workarounds for dwc2 on HiKey board (rebased to synopsys-usb/next)
On 1/12/2017 12:07 AM, Felipe Balbi wrote: > > Hi, > > John Youn writes: >> On 1/11/2017 4:22 PM, John Stultz wrote: >>> Just wanted to resend my patches for dwc2 controller on the >>> HiKey board for consideration for the 4.11 merge window. >>> >>> This patchset is the same as v2, only rebased against >>> John's synopsys-usb/next branch. >>> >>> This does still exclude my patchset[1] to add extcon support to >>> dwc2, which John Youn suspects a pending rework of the dwc2 fifo >>> init logic might make unnecssary. >>> >>> thanks >>> -john >>> >>> [1] >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2016_12_6_69&d=DgIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=6QmpEaovCIrw3iVcLB9lyZ2hPpxb_n6SWfdUDBIINFU&s=JoQtFGWd8y_xhRErt3j8U8uoUw_kq0AStxhqSPwEa3Q&e= >>> >>> >>> v2: >>> * Reworked goto logic in patch #2, as suggested by Vardan >>> v3: >>> * Rebased to synopsys-usb/next >>> >> >> Thanks >> >> For this series: >> Acked-by: John Youn > > Do we need these in the -rc cycle? > This series should be applied after my param rework series for next [1]. If you have any issues with applying I can resend everything together. Regards, John [1] https://www.spinics.net/lists/linux-usb/msg151693.html
Re: [PATCH 0/4 v3] Fixes and workarounds for dwc2 on HiKey board (rebased to synopsys-usb/next)
On 1/11/2017 4:22 PM, John Stultz wrote: > Just wanted to resend my patches for dwc2 controller on the > HiKey board for consideration for the 4.11 merge window. > > This patchset is the same as v2, only rebased against > John's synopsys-usb/next branch. > > This does still exclude my patchset[1] to add extcon support to > dwc2, which John Youn suspects a pending rework of the dwc2 fifo > init logic might make unnecssary. > > thanks > -john > > [1] > https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2016_12_6_69&d=DgIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=6QmpEaovCIrw3iVcLB9lyZ2hPpxb_n6SWfdUDBIINFU&s=JoQtFGWd8y_xhRErt3j8U8uoUw_kq0AStxhqSPwEa3Q&e= > > > v2: > * Reworked goto logic in patch #2, as suggested by Vardan > v3: > * Rebased to synopsys-usb/next > Thanks For this series: Acked-by: John Youn Regards, John
Re: [PATCH 0/5 v2] Fixes and workarounds for dwc2 on HiKey board
On 1/11/2017 3:10 PM, John Stultz wrote: > On Wed, Jan 11, 2017 at 2:58 PM, John Youn wrote: >> On 1/11/2017 2:39 PM, John Stultz wrote: >>> On Wed, Jan 11, 2017 at 2:08 PM, John Youn wrote: >>>> On 1/3/2017 11:52 AM, John Stultz wrote: >>>>> Hope everyone had a happy new years! >>>>> >>>>> I just wanted to send out my current queue of patches for dwc2 >>>>> controller on the HiKey board for consideration for the 4.11 >>>>> merge window. >>>>> >>>>> This does exclude my patchset[1] to add extcon support to dwc2, >>>>> which John Youn suspects a pending rework of the dwc2 fifo init >>>>> logic might make unnecssary. >>>>> >>>>> Any feedback would be greatly appreciated! >>>>> >>>> >>>> Hi John, >>>> >>>> Do you need these in 4.10-rc or is 4.11 ok? >>> >>> So I was submitting these for 4.11. >>> >>> The only one which was sort of a regression fix (though introduced in >>> 4.9 rather then 4.10) was the "Avoid suspending if we're in gadget >>> mode" one, which you seemed to submit for 4.10-rc already. >>> >>> Let me know if there is anything you need prior to queuing the rest for >>> 4.11. >>> >> >> Ok. The only one that will have a problem will be patch 5 due to my >> recent series [1]. >> >> Can you rebase against that? > > Do you have a git branch for that series to rebase against? > https://github.com/synopsys-usb/linux.git branch 'next' Regards, John
Re: [PATCH 0/5 v2] Fixes and workarounds for dwc2 on HiKey board
On 1/11/2017 2:39 PM, John Stultz wrote: > On Wed, Jan 11, 2017 at 2:08 PM, John Youn wrote: >> On 1/3/2017 11:52 AM, John Stultz wrote: >>> Hope everyone had a happy new years! >>> >>> I just wanted to send out my current queue of patches for dwc2 >>> controller on the HiKey board for consideration for the 4.11 >>> merge window. >>> >>> This does exclude my patchset[1] to add extcon support to dwc2, >>> which John Youn suspects a pending rework of the dwc2 fifo init >>> logic might make unnecssary. >>> >>> Any feedback would be greatly appreciated! >>> >> >> Hi John, >> >> Do you need these in 4.10-rc or is 4.11 ok? > > So I was submitting these for 4.11. > > The only one which was sort of a regression fix (though introduced in > 4.9 rather then 4.10) was the "Avoid suspending if we're in gadget > mode" one, which you seemed to submit for 4.10-rc already. > > Let me know if there is anything you need prior to queuing the rest for 4.11. > Ok. The only one that will have a problem will be patch 5 due to my recent series [1]. Can you rebase against that? Patch 1-4 of this series: Acked-by: John Youn I'll also add these to our 4.11 branch locally as there are a lot of interdependent changes. I'll send everything at once to Felipe to make it easier for him. Regards, John [1] https://www.spinics.net/lists/linux-usb/msg151693.html
Re: [PATCH 0/5 v2] Fixes and workarounds for dwc2 on HiKey board
On 1/3/2017 11:52 AM, John Stultz wrote: > Hope everyone had a happy new years! > > I just wanted to send out my current queue of patches for dwc2 > controller on the HiKey board for consideration for the 4.11 > merge window. > > This does exclude my patchset[1] to add extcon support to dwc2, > which John Youn suspects a pending rework of the dwc2 fifo init > logic might make unnecssary. > > Any feedback would be greatly appreciated! > Hi John, Do you need these in 4.10-rc or is 4.11 ok? John
Re: [PATCH] usb: dwc2: use u32 for DT binding parameters
On 1/6/2017 1:52 PM, John Stultz wrote:
> On Fri, Jan 6, 2017 at 4:45 AM, Leo Yan wrote:
>> Commit 05ee799f2021 ("usb: dwc2: Move gadget settings into core_params")
>> changes to type u16 for DT binding "g-rx-fifo-size" and
>> "g-np-tx-fifo-size" but use type u32 for "g-tx-fifo-size". Finally the
>> the first two parameters cannot be passed successfully with wrong data
>> format. This is found the data transferring broken on 96boards Hikey.
>>
>> This patch is to change all parameters to u32 type, and verified on
>> Hikey board the DT parameters can pass successfully.
>>
>> Signed-off-by: Leo Yan
>
> Nice!
>
> This patch (while it doesn't apply cleanly to v4.10-rc2) does resolve
> the regression I reported earlier here:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dusb_msg150766.html&d=DgIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=pIpbe7JcuPH0X7-CYbBt2U3yE2WPSPtrGaMQRtcvQM0&s=hYurvvfAzlq9WTDnKKUznSRb5thSAvIO9doLJOzfUSo&e=
>
>
> I didn't see the slight shift to u16s there.
>
> Leo, does the patch need a respin, or is it against Linus' HEAD and my
> tree is stale?
>
> Anyway, after re-applying it to my tree:
> Tested-by: John Stultz
>
Thanks Leo and John.
I just sent out a rebased version for Felipe to pick up.
Regards,
John
[PATCH] usb: dwc2: use u32 for DT binding parameters
From: Leo Yan
Commit 05ee799f2021 ("usb: dwc2: Move gadget settings into core_params")
changes to type u16 for DT binding "g-rx-fifo-size" and
"g-np-tx-fifo-size" but use type u32 for "g-tx-fifo-size". Finally the
the first two parameters cannot be passed successfully with wrong data
format. This is found the data transferring broken on 96boards Hikey.
This patch is to change all parameters to u32 type, and verified on
Hikey board the DT parameters can pass successfully.
[johnyoun: minor rebase]
Signed-off-by: Leo Yan
Signed-off-by: John Youn
Tested-by: John Stultz
---
drivers/usb/dwc2/core.h | 4 ++--
drivers/usb/dwc2/params.c | 10 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 9548d3e03453..302b8f5f7d27 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -513,8 +513,8 @@ struct dwc2_core_params {
/* Gadget parameters */
bool g_dma;
bool g_dma_desc;
- u16 g_rx_fifo_size;
- u16 g_np_tx_fifo_size;
+ u32 g_rx_fifo_size;
+ u32 g_np_tx_fifo_size;
u32 g_tx_fifo_size[MAX_EPS_CHANNELS];
};
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 11fe68a4627b..bcd1e19b4076 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -385,16 +385,16 @@ static void dwc2_set_param(struct dwc2_hsotg *hsotg, void
*param,
}
/**
- * dwc2_set_param_u16() - Set a u16 parameter
+ * dwc2_set_param_u32() - Set a u32 parameter
*
* See dwc2_set_param().
*/
-static void dwc2_set_param_u16(struct dwc2_hsotg *hsotg, u16 *param,
+static void dwc2_set_param_u32(struct dwc2_hsotg *hsotg, u32 *param,
bool lookup, char *property, u16 legacy,
u16 def, u16 min, u16 max)
{
dwc2_set_param(hsotg, param, lookup, property,
- legacy, def, min, max, 2);
+ legacy, def, min, max, 4);
}
/**
@@ -1178,12 +1178,12 @@ static void dwc2_set_parameters(struct dwc2_hsotg
*hsotg,
* auto-detect if the hardware does not support the
* default.
*/
- dwc2_set_param_u16(hsotg, &p->g_rx_fifo_size,
+ dwc2_set_param_u32(hsotg, &p->g_rx_fifo_size,
true, "g-rx-fifo-size", 2048,
hw->rx_fifo_size,
16, hw->rx_fifo_size);
- dwc2_set_param_u16(hsotg, &p->g_np_tx_fifo_size,
+ dwc2_set_param_u32(hsotg, &p->g_np_tx_fifo_size,
true, "g-np-tx-fifo-size", 1024,
hw->dev_nperio_tx_fifo_size,
16, hw->dev_nperio_tx_fifo_size);
--
2.11.0
Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
On 12/28/2016 5:29 PM, John Youn wrote:
>
>
>> Janusz Dziedzic writes:
>>>>>> On some platfroms(like x86 platform), when one core is running the
>> USB gadget
>>>>>> irq thread handler by dwc3_thread_interrupt(), meanwhile another
>> core also can
>>>>>> respond other interrupts from dwc3 controller and modify the event
>> buffer by
>>>>>> dwc3_interrupt() function, that will cause getting the wrong event
>> count in
>>>>>> irq thread handler to make the USB function abnormal.
>>>>>>
>>>>>> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid
>> this race.
>>>>>
>>>>> Why not spin_lock_irq ones? This lock seems to be used in both
>>>>> normal and interrupt threads. Or, I missed anything?
>>>>
>>>> this is top half handler. Interrupts are already disabled.
>>>>
>>> BTW,
>>> We don't use spin_lock in top half handler.
>>> Maybe we should/can switch all spin_lock_irqsave() to simple
>>> spin_lock() in the thread/callbacks?
>>
>> in theory, yes we've masked all interrupts from this controller for the
>> duration of the thread handler. However this breaks networking
>> gadgets. I can only guess network stack has a hard requirement to run
>> with IRQs disabled.
>>
>
> Hi,
>
> Is this version 3.00a of the core?
>
> That version has a STAR where the interrupts cannot be masked. That
> results in similar symptoms to what you're seeing here.
> Regards,
> John
>
Didn't see any response to this. Just want to make sure this
possibility is addressed as there is a workaround for it on mainline.
See the following commits for details:
d9fa4c63f766 ("usb: dwc3: core: add a event buffer cache")
ebbb2d59398f ("usb: dwc3: gadget: use evt->cache for processing events")
65aca3205046 ("usb: dwc3: gadget: clear events in top-half handler")
cf40b86b6ef6 ("usb: dwc3: Implement interrupt moderation")
28632b44d129 ("usb: dwc3: Workaround for irq mask issue")
Regards,
John
Re: [PATCH 0/5 v2] Fixes and workarounds for dwc2 on HiKey board
On 1/3/2017 12:05 PM, John Stultz wrote: > On Tue, Jan 3, 2017 at 11:52 AM, John Stultz wrote: >> Hope everyone had a happy new years! >> >> I just wanted to send out my current queue of patches for dwc2 >> controller on the HiKey board for consideration for the 4.11 >> merge window. >> >> This does exclude my patchset[1] to add extcon support to dwc2, >> which John Youn suspects a pending rework of the dwc2 fifo init >> logic might make unnecssary. > > John/Vardan: Just as a heads up, along with these patches, I'm also > still using the "fix flags for DMA descriptor allocation in > dwc2_hsotg_ep_enable" from Marek Szyprowski, as its not landed This just hit Greg's tree and should be in -rc3. > upstream, and I'm also still seeing the matching warning on > dwc2_hsotg_ep_disable() on the free side, as I've not seen a fix for > that yet. Most of the team is out this week, so it might not be ready until next week. Unfortunately we had some infrastructure issues which prevented us from getting this out before the holidays. Regards, John
RE: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
> -Original Message- > From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb- > ow...@vger.kernel.org] On Behalf Of Felipe Balbi > Sent: Wednesday, December 28, 2016 8:19 AM > To: Janusz Dziedzic > Cc: Lu Baolu ; Baolin Wang > ; Greg KH ; USB > ; LKML ; Linaro > Kernel Mailman List ; Mark Brown > > Subject: Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt > handler and irq thread handler > > > Hi, > > Janusz Dziedzic writes: > On some platfroms(like x86 platform), when one core is running the > USB gadget > irq thread handler by dwc3_thread_interrupt(), meanwhile another > core also can > respond other interrupts from dwc3 controller and modify the event > buffer by > dwc3_interrupt() function, that will cause getting the wrong event > count in > irq thread handler to make the USB function abnormal. > > We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid > this race. > >>> > >>> Why not spin_lock_irq ones? This lock seems to be used in both > >>> normal and interrupt threads. Or, I missed anything? > >> > >> this is top half handler. Interrupts are already disabled. > >> > > BTW, > > We don't use spin_lock in top half handler. > > Maybe we should/can switch all spin_lock_irqsave() to simple > > spin_lock() in the thread/callbacks? > > in theory, yes we've masked all interrupts from this controller for the > duration of the thread handler. However this breaks networking > gadgets. I can only guess network stack has a hard requirement to run > with IRQs disabled. > Hi, Is this version 3.00a of the core? That version has a STAR where the interrupts cannot be masked. That results in similar symptoms to what you're seeing here. Regards, John
Re: [PATCH 1/2] usb: dwc3: gadget: Fix full speed mode
On 12/7/2016 7:06 PM, John Youn wrote: > On 12/7/2016 4:44 AM, Felipe Balbi wrote: >> >> Hi, >> >> Roger Quadros writes: >>>>> Roger Quadros writes: >>>>>> DCFG.DEVSPD == 0x3 is not valid and we need to set >>>>>> DCFG.DEVSPD to 0x1 for full speed mode. >>>>> >>>>> seems like it has been made invalid somewhere between 1.73a and >>>>> 2.60a. Can you figure it out from Documentation why and when it was made >>>>> invalid? We might need revision checks here. >>>>> >>>> >>>> I'll try to dig out more. >>> >>> I couldn't figure out more information on this. The changelogs in the TRMs >>> don't capture this change and I don't have access to all TRM versions >>> so I can't say which version it got changed and why. >>> >>> Can we please involve someone from Synopsis to provide this information? >>> Thanks. >> >> John, could you help us with this query? We'd like to understand why one >> of the FULLSPEED modes got removed. Do we need a revision check or can >> we assume that the other mode was never supposed to be used? >> > > Full speed is 0x1. 0x3 may still work due to how the bits are > checked. But it definitely should be 0x1. > > I'm not sure if it was 0x3 before. I still need to confirm whether > that was the case or not and if so why. > Hi Felipe, According to the old databook, 0x3 was for FS in 48MHz mode for 1.1 transceiver, which was never supported. UTMI FS was still specified as 0x1 in those old databooks. Regards, John
Re: [RFC][PATCH] HACK: usb: dwc2: Workaround case where GOTGCTL state is wrong
On 12/8/2016 2:43 PM, John Stultz wrote: > On Tue, Dec 6, 2016 at 7:52 PM, John Youn wrote: >> On 12/6/2016 5:48 PM, John Stultz wrote: >>> Hey John, >>> Just wanted to send this by you, as it seems something is >>> slightly off with the GOTGCTL state when removing a otg adapter >>> cable. The following seems to work around the issue I'm seeing. >>> >>> >>> When removing a USB-A to USB-otg adapter cable, we get a change >>> status irq, and then in dwc2_conn_id_status_change, we >>> erroniously see the GOTGCTL_CONID_B flag set. This causes us to >> >> This is the correct behavior for an OTG controller. When you unplug a >> cable or plug in the B end of a cable, the ID pin floats, indicating >> it is a B-Device. >> >> When you plug in an A-cable, which is what your adapter is, it will >> ground the pin, meaning A-device. > > Hrm... So normally, when I plug in the gadget cable into the OTG port, > I see the change_status irq comes in and the function sees: > > dwc2 f72c.usb: gotgctl=401 > dwc2 f72c.usb: gotgctl.b.conidsts=1 > dwc2 f72c.usb: Do port resume before switching to device mode > dwc2 f72c.usb: dwc2_hsotg_enqueue_setup: failed queue (-11) > dwc2 f72c.usb: new device is high-speed > dwc2 f72c.usb: new device is high-speed > dwc2 f72c.usb: new device is high-speed > dwc2 f72c.usb: new address 37 > configfs-gadget gadget: high-speed config #1: b > > Then when I unplug the cable: > > dwc2 f72c.usb: gotgctl=220 > dwc2 f72c.usb: gotgctl.b.conidsts=0 > usb 1-1: reset high-speed USB device number 13 using dwc2 > > > > When I plug in the OTG to USB-A adapter cable w/ a mouse plugged in > (note I see no change interrupt): > > usb 1-1: USB disconnect, device number 13 > usb 1-1: new low-speed USB device number 14 using dwc2 > input: Logitech USB Optical Mouse as > /devices/platform/soc/f72c.usb/usb1/1-1/1-1:1.0/0003:046D:C058.0003/input/input3 > hid-generic 0003:046D:C058.0003: input,hidraw0: USB HID v1.11 Mouse > [Logitech USB Optical Mouse] on usb-f72c.usb-1/input0 > > > Then unplugging the OTG to USB-A adapter cable w/ mouse: > > dwc2 f72c.usb: gotgctl=401 > dwc2 f72c.usb: gotgctl.b.conidsts=1 > dwc2 f72c.usb: Do port resume before switching to device mode > dwc2 f72c.usb: Waiting for Peripheral Mode, Mode=Host > > patch from this thread> > > usb 1-1: USB disconnect, device number 14 > dwc2 f72c.usb: gotgctl=220 > dwc2 f72c.usb: gotgctl.b.conidsts=0 > usb 1-1: new high-speed USB device number 15 using dwc2 > hub 1-1:1.0: USB hub found > hub 1-1:1.0: 3 ports detected > > > So I only get the change irq when: > * I plug in a micro-usb-B cable for gadget mode > * I remove the micro-usb-B cable being used for gadget mode > * I remove a OTG to USB-A adapter > That's very strange. It's opposite of how it's supposed to work. > One slight quirk, is that I don't always see the change irq when > removing the OTG to USB, as if I plug in a highspeed mass-storage > device, instead of the low-speed mouse, I don't see the change > interrupt and the device shows up and disappears the same as when I > plug into the normal USB-A host ports on the board. > > >>> get stuck in the "while (!dwc2_is_device_mode(hsotg))" loop, >>> spitting out "Waiting for Peripheral Mode, Mode=Host" warnings >>> until it fails out many seconds later. >> >> This is weird. Once the ID pin goes to B, the core should become a >> peripheral and this should be reflected in the status registers. >> >>> >>> This patch works around the issue by re-reading the GOTGCTL >>> state to check if the GOTGCTL_CONID_B is still set and if not >>> restarting the change status logic. >> >> This also seems weird. The connector id status shouldn't go back to A, >> assuming you've left the cable unplugged. > > So I suspect this has something to do with the way the USB-A host > ports on the board are wired up. As removing the usb-b plug seems to > switch the device back into A mode. > > One quirk with this board is that the USB-A ports on the board do not > function if anything is in the OTG/B plug (which is frustrating to use > at times). > Do you mean there are multiple A-ports on the board hooked up to the same controller? If so, that would go a long way towards explaining things. Because the hsotg is a single-port OTG controller. If there are multiple A-ports, that means a hub has to be hard-wired internally to the port. But if that's the case
Re: [PATCH 1/2] usb: dwc3: gadget: Fix full speed mode
On 12/7/2016 4:44 AM, Felipe Balbi wrote: > > Hi, > > Roger Quadros writes: Roger Quadros writes: > DCFG.DEVSPD == 0x3 is not valid and we need to set > DCFG.DEVSPD to 0x1 for full speed mode. seems like it has been made invalid somewhere between 1.73a and 2.60a. Can you figure it out from Documentation why and when it was made invalid? We might need revision checks here. >>> >>> I'll try to dig out more. >> >> I couldn't figure out more information on this. The changelogs in the TRMs >> don't capture this change and I don't have access to all TRM versions >> so I can't say which version it got changed and why. >> >> Can we please involve someone from Synopsis to provide this information? >> Thanks. > > John, could you help us with this query? We'd like to understand why one > of the FULLSPEED modes got removed. Do we need a revision check or can > we assume that the other mode was never supposed to be used? > Full speed is 0x1. 0x3 may still work due to how the bits are checked. But it definitely should be 0x1. I'm not sure if it was 0x3 before. I still need to confirm whether that was the case or not and if so why. Regards, John
Re: [RFC][PATCH] HACK: usb: dwc2: Workaround case where GOTGCTL state is wrong
On 12/6/2016 5:48 PM, John Stultz wrote:
> Hey John,
> Just wanted to send this by you, as it seems something is
> slightly off with the GOTGCTL state when removing a otg adapter
> cable. The following seems to work around the issue I'm seeing.
>
> Let me know if you have any thoughts on this.
> thanks
> -john
>
>
> When removing a USB-A to USB-otg adapter cable, we get a change
> status irq, and then in dwc2_conn_id_status_change, we
> erroniously see the GOTGCTL_CONID_B flag set. This causes us to
This is the correct behavior for an OTG controller. When you unplug a
cable or plug in the B end of a cable, the ID pin floats, indicating
it is a B-Device.
When you plug in an A-cable, which is what your adapter is, it will
ground the pin, meaning A-device.
> get stuck in the "while (!dwc2_is_device_mode(hsotg))" loop,
> spitting out "Waiting for Peripheral Mode, Mode=Host" warnings
> until it fails out many seconds later.
This is weird. Once the ID pin goes to B, the core should become a
peripheral and this should be reflected in the status registers.
>
> This patch works around the issue by re-reading the GOTGCTL
> state to check if the GOTGCTL_CONID_B is still set and if not
> restarting the change status logic.
This also seems weird. The connector id status shouldn't go back to A,
assuming you've left the cable unplugged.
Is the controller supposed to work in both peripheral and host modes?
>
> I suspect this isn't the best solution, but it seems to work
> well for me.
>
The workaround seems fine, but still, this indicates that something
wrong is going on somwhere.
You can add my ack:
Acked-by: John Youn
Regards,
John
> Feedback would be greatly appreciated!
>
> Cc: Wei Xu
> Cc: Guodong Xu
> Cc: Amit Pundir
> Cc: Rob Herring
> Cc: John Youn
> Cc: Douglas Anderson
> Cc: Chen Yu
> Cc: Kishon Vijay Abraham I
> Cc: Felipe Balbi
> Cc: Greg Kroah-Hartman
> Cc: linux-...@vger.kernel.org
> Signed-off-by: John Stultz
> ---
> drivers/usb/dwc2/hcd.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 143da47..6d6802a 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3203,7 +3203,7 @@ static void dwc2_conn_id_status_change(struct
> work_struct *work)
> dev_dbg(hsotg->dev, "gotgctl=%0x\n", gotgctl);
> dev_dbg(hsotg->dev, "gotgctl.b.conidsts=%d\n",
> !!(gotgctl & GOTGCTL_CONID_B));
> -
> +again:
> /* B-Device connector (Device Mode) */
> if (gotgctl & GOTGCTL_CONID_B) {
> /* Wait for switch to device mode */
> @@ -3219,6 +3219,9 @@ static void dwc2_conn_id_status_change(struct
> work_struct *work)
>dwc2_is_host_mode(hsotg) ? "Host" :
>"Peripheral");
> usleep_range(2, 4);
> + gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
> + if (!(gotgctl & GOTGCTL_CONID_B))
> + goto again;
> if (++count > 250)
> break;
> }
>
Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver
On 12/6/2016 4:05 PM, John Stultz wrote:
> On Tue, Dec 6, 2016 at 3:17 PM, John Youn wrote:
>> On 12/6/2016 12:06 AM, John Stultz wrote:
>>> This patch wires up extcon support to the dwc2 driver
>>> so that devices that use a modern generic phy driver
>>> and don't use the usb-phy infrastructure can still
>>> signal connect/disconnect events.
>>>
>>> Cc: Wei Xu
>>> Cc: Guodong Xu
>>> Cc: Amit Pundir
>>> Cc: Rob Herring
>>> Cc: John Youn
>>> Cc: Douglas Anderson
>>> Cc: Chen Yu
>>> Cc: Kishon Vijay Abraham I
>>> Cc: Felipe Balbi
>>> Cc: Greg Kroah-Hartman
>>> Cc: linux-...@vger.kernel.org
>>> Signed-off-by: John Stultz
>>> ---
>>> v2:
>>> * Move extcon logic from generic phy to dwc2 driver, as
>>> suggested by Kishon.
>>> ---
>>> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 11 +
>>> drivers/usb/dwc2/core.h | 16
>>> drivers/usb/dwc2/core_intr.c | 25 +++
>>> drivers/usb/dwc2/hcd.c| 23 +
>>> drivers/usb/dwc2/platform.c | 41
>>> +++
>>> 5 files changed, 116 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> index 17839db..8a86a11 100644
>>> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> @@ -732,6 +732,16 @@
>>> regulator-always-on;
>>> };
>>>
>>> + usb_vbus: usb-vbus {
>>> + compatible = "linux,extcon-usb-gpio";
>>> + id-gpio = <&gpio2 6 1>;
>>> + };
>>> +
>>> + usb_id: usb-id {
>>> + compatible = "linux,extcon-usb-gpio";
>>> + id-gpio = <&gpio2 5 1>;
>>> + };
>>> +
>>
>> So you are using extcon-usb-gpio driver to detect both the ID pin and
>> VBUS status correct? Do you need the VBUS one? It doesn't look like
>> you are using it.
>
> Not at the moment. Apologies for my ignorance, I'm not totally
> familiar with the usage of the vbus vs id pins, so I'm somewhat
> following what I was seeing from other drivers. I know with a usb OTG
> to usb A adapter, you get the vbus signal but not the id signal, but I
> don't quite see what should be done differently in that case (as it
> seems to work ok).
Hi John,
The ID pin indicates which end of the cable is connected to the
controller port, determining whether it should take the role of an
A-device or B-device. If this is not visible to the controller (thus
the controller would not give CONNIDSTSCHNG interrupt), that is why
you would need to hook up the extcon module.
I'm thinking this shouldn't be needed for you since you can see this
interrupt.
>
>> Also, do you really need this at all? Wasn't your system previously
>> able to detect the ID pin change correctly via the connection id
>> status change interrupt? This would only be needed if that were not
>> the case.
>
> So it can be made work w/o this, but we needed other hacks because the
> usb-gadget disconnect logic never triggered when the cable was
> unplugged. The controller would jump over to host mode, then when we
> re-plugged in the usb-gadget cable, it would fail often as we never
> got a disconnect signal. That's why earlier I was using this hack to
> force gadget disconnect before the reset was called:
> https://lkml.org/lkml/2016/10/20/26
Other than the triggering WARN_ON() in fifo init, is there any other
negative effects?
We are revisiting this fifo init code and I think the fifo init is not
necessary for USB_RESET purposes. This should get rid of a race
condition where the EP's are not disabled before attempting to
initialize their FIFO's. Which should get rid of the WARN_ON().
If this is the only issue, then this will probably resolve it.
Regards,
John
Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver
On 12/6/2016 12:06 AM, John Stultz wrote:
> This patch wires up extcon support to the dwc2 driver
> so that devices that use a modern generic phy driver
> and don't use the usb-phy infrastructure can still
> signal connect/disconnect events.
>
> Cc: Wei Xu
> Cc: Guodong Xu
> Cc: Amit Pundir
> Cc: Rob Herring
> Cc: John Youn
> Cc: Douglas Anderson
> Cc: Chen Yu
> Cc: Kishon Vijay Abraham I
> Cc: Felipe Balbi
> Cc: Greg Kroah-Hartman
> Cc: linux-...@vger.kernel.org
> Signed-off-by: John Stultz
> ---
> v2:
> * Move extcon logic from generic phy to dwc2 driver, as
> suggested by Kishon.
> ---
> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 11 +
> drivers/usb/dwc2/core.h | 16
> drivers/usb/dwc2/core_intr.c | 25 +++
> drivers/usb/dwc2/hcd.c| 23 +
> drivers/usb/dwc2/platform.c | 41
> +++
> 5 files changed, 116 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 17839db..8a86a11 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -732,6 +732,16 @@
> regulator-always-on;
> };
>
> + usb_vbus: usb-vbus {
> + compatible = "linux,extcon-usb-gpio";
> + id-gpio = <&gpio2 6 1>;
> + };
> +
> + usb_id: usb-id {
> + compatible = "linux,extcon-usb-gpio";
> + id-gpio = <&gpio2 5 1>;
> + };
> +
So you are using extcon-usb-gpio driver to detect both the ID pin and
VBUS status correct? Do you need the VBUS one? It doesn't look like
you are using it.
Also, do you really need this at all? Wasn't your system previously
able to detect the ID pin change correctly via the connection id
status change interrupt? This would only be needed if that were not
the case.
> usb_phy: usbphy {
> compatible = "hisilicon,hi6220-usb-phy";
> #phy-cells = <0>;
> @@ -745,6 +755,7 @@
> phys = <&usb_phy>;
> phy-names = "usb2-phy";
> clocks = <&sys_ctrl HI6220_USBOTG_HCLK>;
> + extcon = <&usb_vbus>, <&usb_id>;
You should document what should be set for "extcon" in
/Documentation/devicetree/bindings/usb/dwc2.txt. And make that a
separate commit before using the binding.
> clock-names = "otg";
> dr_mode = "otg";
> g-use-dma;
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2a21a04..4cfce62 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -623,6 +623,13 @@ struct dwc2_hregs_backup {
> bool valid;
> };
>
> +struct dwc2_extcon {
> + struct notifier_block nb;
> + struct extcon_dev *extcon;
> + int state;
> +};
> +
> +
Don't use multiple blank lines. Please run "checkpatch.pl --strict"
and fix other issues it reports, if possible.
[snip]
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 8e1728b..2e4545f 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -46,6 +46,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -543,6 +544,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
> struct dwc2_core_params defparams;
> struct dwc2_hsotg *hsotg;
> struct resource *res;
> + struct extcon_dev *ext_id, *ext_vbus;
> int retval;
>
> match = of_match_device(dwc2_of_match_table, &dev->dev);
> @@ -620,6 +622,45 @@ static int dwc2_driver_probe(struct platform_device *dev)
> if (retval)
> goto error;
>
> + ext_id = ERR_PTR(-ENODEV);
> + ext_vbus = ERR_PTR(-ENODEV);
> + if (of_property_read_bool(dev->dev.of_node, "extcon")) {
You can omit this check since it's done in the api function.
> + /* Each one of them is not mandatory */
> + ext_vbus = extcon_get_edev_by_phandle(&dev->dev, 0);
> + if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
> + return PTR_ERR(ext_vbus);
> +
> + ext_id = extcon_get_edev_by_phandle(&dev->dev, 1);
> + if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
> + return PTR_ERR(ext_id);
> + }
Ensure when you initialize hsotg->extcon_vbus/id that they are either
valid and set or NULL so that you don't have to do IS_ERR() all the
time.
Regards,
John
Re: [PATCH v9 1/2] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 12/6/2016 4:00 AM, Ayaka wrote:
> Hello John
> I still waiting them be merged, but I still can't find it at next-20161206.
>
Can you resubmit this fixing the checkpatch issues?
You can add my ack:
Acked-by: John Youn
Regards,
John
> 從我的 iPad 傳送
>
>> John Youn 於 2016年10月25日 上午9:30 寫道：
>>
>>> On 10/23/2016 2:33 AM, ayaka wrote:
>>>
>>>
>>>> On 10/22/2016 03:27 AM, John Youn wrote:
>>>>> On 10/20/2016 11:38 AM, Randy Li wrote:
>>>>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>>>>> port) the PHY can get into a bad state when a wakeup is asserted (not
>>>>> just a wakeup from full system suspend but also a wakeup from
>>>>> autosuspend).
>>>>>
>>>>> We can get the PHY out of its bad state by asserting its "port reset",
>>>>> but unfortunately that seems to assert a reset onto the USB bus so it
>>>>> could confuse things if we don't actually deenumerate / reenumerate the
>>>>> device.
>>>>>
>>>>> We can also get the PHY out of its bad state by fully resetting it using
>>>>> the reset from the CRU (clock reset unit) in chip, which does a more full
>>>>> reset. The CRU-based reset appears to actually cause devices on the bus
>>>>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>>>>> way).
>>>>>
>>>>> It's unfortunate that we need to do a full re-enumeration of devices at
>>>>> wakeup time, but this is better than alternative of letting the bus get
>>>>> wedged.
>>>>>
>>>>> Signed-off-by: Randy Li
>>>>> ---
>>>>> drivers/usb/dwc2/core.h | 1 +
>>>>> drivers/usb/dwc2/core_intr.c | 11 +++
>>>>> drivers/usb/dwc2/platform.c | 9 +
>>>>> 3 files changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>>> index 2a21a04..e91ddbc 100644
>>>>> --- a/drivers/usb/dwc2/core.h
>>>>> +++ b/drivers/usb/dwc2/core.h
>>>>> @@ -859,6 +859,7 @@ struct dwc2_hsotg {
>>>>> unsigned int ll_hw_enabled:1;
>>>>>
>>>>> struct phy *phy;
>>>>> +struct work_struct phy_rst_work;
>>>>> struct usb_phy *uphy;
>>>>> struct dwc2_hsotg_plat *plat;
>>>>> struct regulator_bulk_data
>>>>> supplies[ARRAY_SIZE(dwc2_hsotg_supply_names)];
>>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>>> index d85c5c9..c3d2168 100644
>>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>>> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
>>>>> dwc2_hsotg *hsotg)
>>>>> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>>> {
>>>>> int ret;
>>>>> +struct device_node *np = hsotg->dev->of_node;
>>>>>
>>>>> /* Clear interrupt */
>>>>> dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>>>>> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct
>>>>> dwc2_hsotg *hsotg)
>>>>> /* Restart the Phy Clock */
>>>>> pcgcctl &= ~PCGCTL_STOPPCLK;
>>>>> dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
>>>>> +
>>>>> +/*
>>>>> + * It is a quirk in Rockchip RK3288, causing by
>>>>> + * a hardware bug. This will propagate out and
>>>>> + * eventually we'll re-enumerate the device.
>>>>> + * Not great but the best we can do
>>>>> + */
>>>>> +if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
>>>>> +schedule_work(&hsotg->phy_rst_work);
>>>>> +
>>>>> mod_timer(&hsotg->wkp_timer,
>>>>>jiffies + msecs_to_jiffies(71));
>>>>> } else {
>>>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>>>> index 8e1728b..65953cf 100644
>>>>> --- a/drivers/
Re: [PATCH] usb: dwc2: fix flags for DMA descriptor allocation in dwc2_hsotg_ep_enable
On 12/1/2016 1:02 AM, Marek Szyprowski wrote:
> dwc2_hsotg_ep_enable can be called from interrupt context, so all
> allocations should be done with GFP_ATOMIC flags. This fixes following
> issue on ARM architecture:
>
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0x74/0x94)
> [] (dump_stack) from [] (__warn+0xd4/0x100)
> [] (__warn) from [] (warn_slowpath_null+0x20/0x28)
> [] (warn_slowpath_null) from []
> (smp_call_function_many+0xcc/0x2a4)
> [] (smp_call_function_many) from []
> (on_each_cpu_mask+0x38/0xa8)
> [] (on_each_cpu_mask) from []
> (start_isolate_page_range+0x134/0x1b8)
> [] (start_isolate_page_range) from []
> (alloc_contig_range+0xac/0x2f8)
> [] (alloc_contig_range) from [] (cma_alloc+0xe0/0x1a8)
> [] (cma_alloc) from [] (__alloc_from_contiguous+0x38/0xe0)
> [] (__alloc_from_contiguous) from []
> (cma_allocator_alloc+0x30/0x38)
> [] (cma_allocator_alloc) from [] (__dma_alloc+0x1c0/0x2c8)
> [] (__dma_alloc) from [] (arm_dma_alloc+0x3c/0x48)
> [] (arm_dma_alloc) from []
> (dwc2_hsotg_ep_enable+0xec/0x46c)
> [] (dwc2_hsotg_ep_enable) from []
> (usb_ep_enable+0x2c/0x3c)
> [] (usb_ep_enable) from [] (ecm_set_alt+0xa8/0x154)
> [] (ecm_set_alt) from [] (composite_setup+0xd74/0x1540)
> [] (composite_setup) from []
> (dwc2_hsotg_complete_setup+0xb8/0x370)
> [] (dwc2_hsotg_complete_setup) from []
> (usb_gadget_giveback_request+0xc/0x10)
> [] (usb_gadget_giveback_request) from []
> (dwc2_hsotg_complete_request+0x78/0x128)
> [] (dwc2_hsotg_complete_request) from []
> (dwc2_hsotg_epint+0x69c/0x81c)
> [] (dwc2_hsotg_epint) from [] (dwc2_hsotg_irq+0xfc/0x748)
> [] (dwc2_hsotg_irq) from []
> (__handle_irq_event_percpu+0x58/0x140)
> [] (__handle_irq_event_percpu) from []
> (handle_irq_event_percpu+0x1c/0x58)
> [] (handle_irq_event_percpu) from []
> (handle_irq_event+0x38/0x5c)
> [] (handle_irq_event) from []
> (handle_fasteoi_irq+0xc4/0x19c)
> [] (handle_fasteoi_irq) from []
> (generic_handle_irq+0x18/0x28)
> [] (generic_handle_irq) from []
> (__handle_domain_irq+0x6c/0xe4)
> [] (__handle_domain_irq) from []
> (gic_handle_irq+0x50/0x9c)
> [] (gic_handle_irq) from [] (__irq_svc+0x6c/0xa8)
>
> Fixes: 5f54c54b0ba83 ("usb: dwc2: gadget: Add DDMA chain pointers to
> dwc2_hsotg_ep structure")
> Signed-off-by: Marek Szyprowski
> ---
> 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 b95930f20d90..c55db4aa54d6 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3753,7 +3753,7 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
> hs_ep->desc_list = dma_alloc_coherent(hsotg->dev,
> MAX_DMA_DESC_NUM_GENERIC *
> sizeof(struct dwc2_dma_desc),
> - &hs_ep->desc_list_dma, GFP_KERNEL);
> + &hs_ep->desc_list_dma, GFP_ATOMIC);
> if (!hs_ep->desc_list) {
> ret = -ENOMEM;
> goto error2;
>
Acked-by: John Youn
Regards,
John
Re: [RFC][PATCH 1/3] phy: phy-hi6220-usb: Wire up extconn support to hikey's phy driver
On 12/1/2016 12:12 PM, John Stultz wrote:
> On Thu, Dec 1, 2016 at 12:23 AM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 23 November 2016 09:16 AM, John Stultz wrote:
>>> This wires extconn support to hikey's phy driver, and
>>> connects it to the usb UDC layer via a usb_phy structure.
>>>
>>> Not sure if this is the right way to connect phy -> UDC,
>>> but I'm lacking a clear example.
>>>
>>> Cc: Wei Xu
>>> Cc: Guodong Xu
>>> Cc: Amit Pundir
>>> Cc: Rob Herring
>>> Cc: John Youn
>>> Cc: Douglas Anderson
>>> Cc: Chen Yu
>>> Cc: Kishon Vijay Abraham I
>>> Cc: Felipe Balbi
>>> Cc: Greg Kroah-Hartman
>>> Cc: linux-...@vger.kernel.org
>>> Signed-off-by: John Stultz
>>> ---
>>> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 11 +++
>>> drivers/phy/Kconfig | 2 +
>>> drivers/phy/phy-hi6220-usb.c | 139
>>> ++
>>> 3 files changed, 152 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> index 17839db..171fbb2 100644
>>> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> @@ -732,10 +732,21 @@
>>> regulator-always-on;
>>> };
>>>
>>> + usb_vbus: usb-vbus {
>>> + compatible = "linux,extcon-usb-gpio";
>>> + id-gpio = <&gpio2 6 1>;
>>> + };
>>> +
>>> + usb_id: usb-id {
>>> + compatible = "linux,extcon-usb-gpio";
>>> + id-gpio = <&gpio2 5 1>;
>>> + };
>>> +
>>> usb_phy: usbphy {
>>> compatible = "hisilicon,hi6220-usb-phy";
>>> #phy-cells = <0>;
>>> phy-supply = <&fixed_5v_hub>;
>>> + extcon = <&usb_vbus>, <&usb_id>;
>>> hisilicon,peripheral-syscon = <&sys_ctrl>;
>>> };
>>>
>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>> index fe00f91..76f4f17 100644
>>> --- a/drivers/phy/Kconfig
>>> +++ b/drivers/phy/Kconfig
>>> @@ -254,8 +254,10 @@ config PHY_MT65XX_USB3
>>> config PHY_HI6220_USB
>>> tristate "hi6220 USB PHY support"
>>> depends on (ARCH_HISI && ARM64) || COMPILE_TEST
>>> + depends on EXTCON
>>> select GENERIC_PHY
>>> select MFD_SYSCON
>>> + select USB_PHY
>>> help
>>> Enable this to support the HISILICON HI6220 USB PHY.
>>>
>>> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
>>> index b2141cb..89d8475 100644
>>> --- a/drivers/phy/phy-hi6220-usb.c
>>> +++ b/drivers/phy/phy-hi6220-usb.c
>>> @@ -12,7 +12,12 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> #include
>>> +#include
>>>
>>> #define SC_PERIPH_CTRL4 0x00c
>>>
>>> @@ -44,9 +49,21 @@
>>>
>>> #define EYE_PATTERN_PARA 0x7053348c
>>>
>>> +
>>> +struct hi6220_usb_cable {
>>> + struct notifier_block nb;
>>> + struct extcon_dev *extcon;
>>> + int state;
>>> +};
>>> +
>>> struct hi6220_priv {
>>> struct regmap *reg;
>>> struct device *dev;
>>> + struct usb_phy phy;
>>> +
>>> + struct delayed_work work;
>>> + struct hi6220_usb_cable vbus;
>>> + struct hi6220_usb_cable id;
>>> };
>>>
>>> static void hi6220_phy_init(struct hi6220_priv *priv)
>>> @@ -112,23 +129,85 @@ static int hi6220_phy_exit(struct phy *phy)
>>> return hi6220_phy_setup(priv, false);
>>> }
>>>
>>> +
>>> static struct phy_ops hi6220_phy_ops = {
>>> .init = hi6220_phy_start,
>>> .exit = hi6220_phy_exit,
>>> .owner = THIS
Re: [PATCH 2/2] Synopsys USB 2.0 Device Controller (UDC) Driver
On 11/30/2016 4:47 AM, Felipe Balbi wrote: > > Hi, > > Raviteja Garimella writes: >> Hi Balbi, >> >> On Wed, Nov 30, 2016 at 4:10 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Raviteja Garimella writes: This is driver for Synopsys Designware Cores USB Device Controller (UDC) Subsystem with the AMBA Advanced High-Performance Bus (AHB). This driver works with Synopsys UDC20 products. Signed-off-by: Raviteja Garimella >>> >>> use drivers/usb/dwc2 instead of duplicating it. >> >> The ones we have in drivers/usb/dwc2 is for Designware high speed OTG >> controller IP. The one that I submitted for review is for USB Device >> controller IP (UDC). The IPs are different. > > I'll wait for John's confirmation that this really isn't compatible with > dwc2. John? > Hi Felipe, This is our older UDC IP, not compatible with HSOTG. It is also no longer supported by Synopsys and considered EOL. Regards, John
Re: [RFC][PATCH 3/3] usb: dwc2: Make sure we disconnect the gadget state
On 11/15/2016 1:47 PM, John Stultz wrote: > I had seen some odd behavior with HiKey's usb-gadget interface > that I finally seemed to have chased down. Basically every other > time I pluged in the OTG port, the gadget interface would > properly initialize. The other times, I'd get a big WARN_ON > in dwc2_hsotg_init_fifo() about the fifo_map not being clear. Hi, The fifo_map could end up not being clear when disconnect is never sent to the UDC framework. That unsets the configuration and the endpoints get disabled, which clears the FIFO map. Looks like the problem happens when going from A-device to B-device. If you come up as an A-Device, the gadget wouldn't have been configured so it shouldn't warn going A->B. If you go B->A, you will get a session end detected, which triggers the udc disconnect. Then A->B should not warn here either. Can you determine why this doesn't happen on your system? It sounds like there might be some race condition that we need to identify. If you can provide logs with DEBUG enabled that would be helpful too. Regards, John > > Ends up if we don't disconnect the gadget state, the fifo-map > doesn't get cleared properly, which causes WARN_ON messages and > also results in the device not properly being setup as a gadget > every other time the OTG port is connected. > > So this patch adds a call to dwc2_hsotg_disconnect() in the > reset path so the state is properly cleared. > > With it, the gadget interface initializes properly on every > plug in. > > Cc: Wei Xu > Cc: Guodong Xu > Cc: Amit Pundir > Cc: Rob Herring > Cc: John Youn > Cc: Douglas Anderson > Cc: Chen Yu > Cc: Felipe Balbi > Cc: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org > Signed-off-by: John Stultz > --- > drivers/usb/dwc2/hcd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index 8c980fd..d2557b7 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -3228,6 +3228,7 @@ static void dwc2_conn_id_status_change(struct > work_struct *work) > dwc2_core_init(hsotg, false); > dwc2_enable_global_interrupts(hsotg); > spin_lock_irqsave(&hsotg->lock, flags); > + dwc2_hsotg_disconnect(hsotg); > dwc2_hsotg_core_init_disconnected(hsotg, false); > spin_unlock_irqrestore(&hsotg->lock, flags); > dwc2_hsotg_core_connect(hsotg); >
Re: [PATCH 0/5] usb: dwc2: fix parameter handling
On 11/20/2016 1:26 PM, Stefan Wahren wrote: > This patch series fixes several parameter handling issues > found on bcm2835 in gadget mode. It's based on Felipe's USB next. > > Stefan Wahren (5): > usb: dwc2: Do not set host parameter in peripheral mode > usb: dwc2: fix dwc2_get_device_property for u8 and u16 > usb: dwc2: fix default value for DMA support > usb: dwc2: gadget: fix default value for gadget-dma-desc > usb: dwc2: fix kernel-doc for dwc2_set_param > > drivers/usb/dwc2/params.c | 32 ++-- > 1 file changed, 10 insertions(+), 22 deletions(-) > For this series: Acked-by: John Youn Felipe, This is too late for 4.10-rc1 right? Can you queue for 4.10 fixes. I can remind you after 4.10-rc1 if it's too early for that. Regards, John
[PATCH v3] usb: dwc2: add amcc,dwc-otg support
From: Christian Lamparter
This patch adds support for the "amcc,usb-otg" device
which is found in the PowerPC Canyonlands' dts.
The device definition was added by:
commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands
board")
but without any driver support as the dwc2 driver wasn't available at
that time.
Note: The system can't use the generic "snps,dwc2" compatible
because of the special ahbcfg configuration. The default
GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
when the USB and SATA is used concurrently.
Signed-off-by: Christian Lamparter
Signed-off-by: John Youn
---
Sorry, the previous one broke compilation. This fixes it.
Regards,
John
v3 [johnyoun]:
* Fixed compilation issue
v2 [johnyoun]:
* Removed params struct
* Minor commit message formatting
Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
drivers/usb/dwc2/params.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt
b/Documentation/devicetree/bindings/usb/dwc2.txt
index ad8f7ff..6c7c2bce 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -12,6 +12,7 @@ Required properties:
- "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
- "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlogic Meson8b
SoCs;
- "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic S905
SoCs;
+ - "amcc,dwc-otg": The DWC2 USB controller instance in AMCC Canyonlands 460EX
SoCs;
- snps,dwc2: A generic DWC2 USB controller with default parameters.
- reg : Should contain 1 register range (address and length)
- interrupts : Should contain 1 interrupt
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 513556a..a786256 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -239,6 +239,7 @@ const struct of_device_id dwc2_of_match_table[] = {
{ .compatible = "samsung,s3c6400-hsotg", .data = NULL},
{ .compatible = "amlogic,meson8b-usb", .data = ¶ms_amlogic },
{ .compatible = "amlogic,meson-gxbb-usb", .data = ¶ms_amlogic },
+ { .compatible = "amcc,dwc-otg", .data = NULL },
{},
};
MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
--
2.10.0
[PATCH v2] usb: dwc2: add amcc,dwc-otg support
From: Christian Lamparter
This patch adds support for the "amcc,usb-otg" device
which is found in the PowerPC Canyonlands' dts.
The device definition was added by:
commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands
board")
but without any driver support as the dwc2 driver wasn't available at
that time.
Note: The system can't use the generic "snps,dwc2" compatible
because of the special ahbcfg configuration. The default
GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
when the USB and SATA is used concurrently.
Signed-off-by: Christian Lamparter
Signed-off-by: John Youn
---
v2 [johnyoun]:
* Removed params struct
* Minor commit message formatting
Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
drivers/usb/dwc2/params.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt
b/Documentation/devicetree/bindings/usb/dwc2.txt
index ad8f7ff..6c7c2bce 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -12,6 +12,7 @@ Required properties:
- "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
- "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlogic Meson8b
SoCs;
- "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic S905
SoCs;
+ - "amcc,dwc-otg": The DWC2 USB controller instance in AMCC Canyonlands 460EX
SoCs;
- snps,dwc2: A generic DWC2 USB controller with default parameters.
- reg : Should contain 1 register range (address and length)
- interrupts : Should contain 1 interrupt
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 513556a..7991c21 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -239,6 +239,7 @@ const struct of_device_id dwc2_of_match_table[] = {
{ .compatible = "samsung,s3c6400-hsotg", .data = NULL},
{ .compatible = "amlogic,meson8b-usb", .data = ¶ms_amlogic },
{ .compatible = "amlogic,meson-gxbb-usb", .data = ¶ms_amlogic },
+ { .compatible = "amcc,dwc-otg", .data = ¶ms_amcc_dwc_otg },
{},
};
MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
--
2.10.0
Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support
On 11/14/2016 3:00 PM, John Youn wrote:
> On 11/11/2016 3:12 PM, Christian Lamparter wrote:
>> On Friday, November 11, 2016 2:20:42 PM CET John Youn wrote:
>>> On 11/11/2016 2:05 PM, Christian Lamparter wrote:
>>>> On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
>>>>> On 11/11/2016 12:59 PM, Christian Lamparter wrote:
>>>>>> This patch adds support for the "amcc,usb-otg" device
>>>>>> which is found in the PowerPC Canyonlands' dts.
>>>>>>
>>>>>> The device definition was added by:
>>>>>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands
>>>>>> board")'
>>>>>> but without any driver support as the dwc2 driver wasn't
>>>>>> available at that time.
>>>>>>
>>>>>> Note: The system can't use the generic "snps,dwc2" compatible
>>>>>> because of the special ahbcfg configuration. The default
>>>>>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
>>>>>> when the USB and SATA is used concurrently.
>>>>>
>>>>> I don't want to add any more of these param structures to the driver
>>>>> unless really necessary. We're trying to remove usage of them in favor
>>>>> of using auto-detected defaults and device properties to override
>>>>> them.
>>>> Ok, thanks. I think that would work. I've attached an updated patch.
>>>> Can it be applied/queued now? Or do you want me to resent it later?
>>>>
>>>>> The AHB Burst is actually one of the ones we were going to do next
>>>>> because our platform also doesn't work well with INCR4. In fact I'm
>>>>> thinking of making the default INCR.
>>>> Is that actually possible to change the default still? This would
>>>> require to re-evaluate all existing archs/platforms that use
>>>> "snps,dwc2" for INCR16 compatibility.
>>>
>>> INCR, not INCR16, but you're right, so we may not change it even
>>> though though INCR is usually the right choice over INCR4.
>> What about making a device-tree property?
>
> Yes, that's what I meant. I'll send a change for this shortly.
>
>>
>> Recommended properties:
>> - g-ahb-bursts : specifies the ahb bursts length, should be one of
>>"single", "INCRx", "INCR4", "INCR8", or "INCR16". If not specified
>>the safer but inefficient "INCR4" is used. The optimal setting is
>>"INCRx".
>>
>> Would this work? If so, I can make a patch over the weekend.
>>> Anyways, with the binding, can't you just set the compatible string to
>>> snps,dwc2?
>>
>> Ah, let me explain. I had a discussion with Mark Rutland and Rob Herring
>> a while back about device-tree bindings.
>>
>> They made it very clear to me, that they don't want any generic "catch all
>> compatible" strings:
>>
>> "Bindings should be for hardware (either specific device models, or for
>> classes), and not for Linux drivers. The latter is subject to arbitrary
>> changes while the former is not, as old hardware continues to exist and
>> does not change while drivers get completely reworked." [0]
>>
>> Furthermore, this is an existing binding in kernel's canyonlands.dts [1]
>> and this binding can't be easily changed. Rob Herring explained this in
>> the context of the "basic-mmio-gpio" patch [2] when I was editing the dts
>> to make them work with the changes I made:
>>
>> "You can't remove the old drivers as they are needed to work with
>> old dtbs, so there is no gain.
>>
>> You would need to match on existing compatibles such as
>> moxa,moxart-gpio and provide a match data struct that has all the info
>> you are adding here (e.g. data register offset). Then additionally you
>> could add "basic-mmio-gpio" (I would drop "basic" part) and the
>> additional data associated with it. But it has to be new properties,
>> not changing properties. Changing the reg values doesn't work."
>>
>> So, for this to work with the existing canyonlands.dts, I need to have
>> the "amcc,dwc-otg" compatible string.
>
> Ok, if that's the case. But still a bit confused as to what driver was
> working with it before since the binding was not defined fo
Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support
On 11/11/2016 3:12 PM, Christian Lamparter wrote:
> On Friday, November 11, 2016 2:20:42 PM CET John Youn wrote:
>> On 11/11/2016 2:05 PM, Christian Lamparter wrote:
>>> On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
>>>> On 11/11/2016 12:59 PM, Christian Lamparter wrote:
>>>>> This patch adds support for the "amcc,usb-otg" device
>>>>> which is found in the PowerPC Canyonlands' dts.
>>>>>
>>>>> The device definition was added by:
>>>>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands
>>>>> board")'
>>>>> but without any driver support as the dwc2 driver wasn't
>>>>> available at that time.
>>>>>
>>>>> Note: The system can't use the generic "snps,dwc2" compatible
>>>>> because of the special ahbcfg configuration. The default
>>>>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
>>>>> when the USB and SATA is used concurrently.
>>>>
>>>> I don't want to add any more of these param structures to the driver
>>>> unless really necessary. We're trying to remove usage of them in favor
>>>> of using auto-detected defaults and device properties to override
>>>> them.
>>> Ok, thanks. I think that would work. I've attached an updated patch.
>>> Can it be applied/queued now? Or do you want me to resent it later?
>>>
>>>> The AHB Burst is actually one of the ones we were going to do next
>>>> because our platform also doesn't work well with INCR4. In fact I'm
>>>> thinking of making the default INCR.
>>> Is that actually possible to change the default still? This would
>>> require to re-evaluate all existing archs/platforms that use
>>> "snps,dwc2" for INCR16 compatibility.
>>
>> INCR, not INCR16, but you're right, so we may not change it even
>> though though INCR is usually the right choice over INCR4.
> What about making a device-tree property?
Yes, that's what I meant. I'll send a change for this shortly.
>
> Recommended properties:
> - g-ahb-bursts : specifies the ahb bursts length, should be one of
>"single", "INCRx", "INCR4", "INCR8", or "INCR16". If not specified
>the safer but inefficient "INCR4" is used. The optimal setting is
>"INCRx".
>
> Would this work? If so, I can make a patch over the weekend.
>> Anyways, with the binding, can't you just set the compatible string to
>> snps,dwc2?
>
> Ah, let me explain. I had a discussion with Mark Rutland and Rob Herring
> a while back about device-tree bindings.
>
> They made it very clear to me, that they don't want any generic "catch all
> compatible" strings:
>
> "Bindings should be for hardware (either specific device models, or for
> classes), and not for Linux drivers. The latter is subject to arbitrary
> changes while the former is not, as old hardware continues to exist and
> does not change while drivers get completely reworked." [0]
>
> Furthermore, this is an existing binding in kernel's canyonlands.dts [1]
> and this binding can't be easily changed. Rob Herring explained this in
> the context of the "basic-mmio-gpio" patch [2] when I was editing the dts
> to make them work with the changes I made:
>
> "You can't remove the old drivers as they are needed to work with
> old dtbs, so there is no gain.
>
> You would need to match on existing compatibles such as
> moxa,moxart-gpio and provide a match data struct that has all the info
> you are adding here (e.g. data register offset). Then additionally you
> could add "basic-mmio-gpio" (I would drop "basic" part) and the
> additional data associated with it. But it has to be new properties,
> not changing properties. Changing the reg values doesn't work."
>
> So, for this to work with the existing canyonlands.dts, I need to have
> the "amcc,dwc-otg" compatible string.
Ok, if that's the case. But still a bit confused as to what driver was
working with it before since the binding was not defined for dwc2.
>
> Of course, it would be great to hear from Rob Herring and/or Mark Rutland
> about this case.
>
> Regards,
> Christian
>
> [0] <https://patchwork.kernel.org/patch/8976221/>
> [1]
> <http://lxr.free-electrons.com/source/arch/powerpc/boot/dts/canyonlands.dts#L181>
> [2] <http://www.spinics.net/lists/
Re: [PATCH 2/2] usb: dwc2: fixes host_dma logic
On 11/11/2016 12:59 PM, Christian Lamparter wrote:
> This patch moves the the host_dma initialization
> before dwc2_set_param_dma_desc_enable and
> dwc2_set_param_dma_desc_fs_enable. The reason being
> that both function need it.
>
> Fixes: 1205489cee75bf39 ("usb: dwc2: Get host DMA device properties")
This should probably be omitted since it's only in Felipe's
testing/next.
Otherwise looks good.
Acked-by: John Youn
Regards,
John
>
> Cc: John Youn
> Cc: Felipe Balbi
> Signed-off-by: Christian Lamparter
> ---
> drivers/usb/dwc2/params.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 5d822c5..222a83c 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -1157,9 +1157,6 @@ static void dwc2_set_parameters(struct dwc2_hsotg
> *hsotg,
> bool dma_capable = !(hw->arch == GHWCFG2_SLAVE_ONLY_ARCH);
>
> dwc2_set_param_otg_cap(hsotg, params->otg_cap);
> - dwc2_set_param_dma_desc_enable(hsotg, params->dma_desc_enable);
> - dwc2_set_param_dma_desc_fs_enable(hsotg, params->dma_desc_fs_enable);
> -
> if ((hsotg->dr_mode == USB_DR_MODE_HOST) ||
> (hsotg->dr_mode == USB_DR_MODE_OTG)) {
> bool disable;
> @@ -1174,6 +1171,8 @@ static void dwc2_set_parameters(struct dwc2_hsotg
> *hsotg,
> !disable, false,
> dma_capable);
> }
> + dwc2_set_param_dma_desc_enable(hsotg, params->dma_desc_enable);
> + dwc2_set_param_dma_desc_fs_enable(hsotg, params->dma_desc_fs_enable);
>
> dwc2_set_param_host_support_fs_ls_low_power(hsotg,
> params->host_support_fs_ls_low_power);
>
Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support
On 11/11/2016 2:05 PM, Christian Lamparter wrote:
> Hello,
>
> On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
>> On 11/11/2016 12:59 PM, Christian Lamparter wrote:
>>> This patch adds support for the "amcc,usb-otg" device
>>> which is found in the PowerPC Canyonlands' dts.
>>>
>>> The device definition was added by:
>>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands
>>> board")'
>>> but without any driver support as the dwc2 driver wasn't
>>> available at that time.
>>>
>>> Note: The system can't use the generic "snps,dwc2" compatible
>>> because of the special ahbcfg configuration. The default
>>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
>>> when the USB and SATA is used concurrently.
>>
>> I don't want to add any more of these param structures to the driver
>> unless really necessary. We're trying to remove usage of them in favor
>> of using auto-detected defaults and device properties to override
>> them.
> Ok, thanks. I think that would work. I've attached an updated patch.
> Can it be applied/queued now? Or do you want me to resent it later?
>
>> The AHB Burst is actually one of the ones we were going to do next
>> because our platform also doesn't work well with INCR4. In fact I'm
>> thinking of making the default INCR.
> Is that actually possible to change the default still? This would
> require to re-evaluate all existing archs/platforms that use
> "snps,dwc2" for INCR16 compatibility.
INCR, not INCR16, but you're right, so we may not change it even
though though INCR is usually the right choice over INCR4.
Anyways, with the binding, can't you just set the compatible string to
snps,dwc2?
Regards,
John
>
> From what I can tell based would be:
> bcm11351, bcm21664, bcm23550, exynos3250, stm32f429, rk3xxx,
> stratix10, meson-gxbb, rt3050 and some Altera FPGAs.
>
>> If that's all you need then a devicetree binding should be enough
>> right?
> Yes. The device is working fine so far.
>
> Regards,
> Christian
>
> ---
> From 70dd4be016b89655a56bc8260f04683b50f07644 Mon Sep 17 00:00:00 2001
> From: Christian Lamparter
> Date: Sun, 6 Nov 2016 00:39:24 +0100
> Subject: [PATCH] usb: dwc2: add amcc,dwc-otg support
>
> This patch adds support for the "amcc,usb-otg" device
> which is found in the PowerPC Canyonlands' dts.
>
> The device definition was added by:
> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands
> board")'
> but without any driver support as the dwc2 driver wasn't
> available at that time.
>
> Note: The system can't use the generic "snps,dwc2" compatible
> because of the special ahbcfg configuration. The default
> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
> when the USB and SATA is used concurrently.
>
> Cc: Felipe Balbi
> Cc: John Youn
> Signed-off-by: Christian Lamparter
> ---
> v1->v2:
> - moved definitons to params.c
> - removed dma_enable / host_dma parameter
> - added dma_desc_fs_enable parameter
> v2->v3:
> - removed parameters
>
> Please queue this patch until GAHBCFG_HBSTLEN_INCR16 is the default
> for ahbcfg.
> ---
> Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
> drivers/usb/dwc2/params.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt
> b/Documentation/devicetree/bindings/usb/dwc2.txt
> index 10a2a4b..6ccfe85 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> @@ -12,6 +12,7 @@ Required properties:
>- "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
>- "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlogic
> Meson8b SoCs;
>- "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic
> S905 SoCs;
> + - "amcc,dwc-otg": The DWC2 USB controller instance in AMCC Canyonlands
> 460EX SoCs;
>- snps,dwc2: A generic DWC2 USB controller with default parameters.
> - reg : Should contain 1 register range (address and length)
> - interrupts : Should contain 1 interrupt
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 64d5c66..9506ab0 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -239,6 +239,7 @@ const struct of_device_id dwc2_of_match_table[] = {
> { .compatible = "samsung,s3c6400-hsotg", .data = NULL},
> { .compatible = "amlogic,meson8b-usb", .data = ¶ms_amlogic },
> { .compatible = "amlogic,meson-gxbb-usb", .data = ¶ms_amlogic },
> + { .compatible = "amcc,dwc-otg", .data = NULL },
> {},
> };
> MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
>
Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support
On 11/11/2016 12:59 PM, Christian Lamparter wrote:
> This patch adds support for the "amcc,usb-otg" device
> which is found in the PowerPC Canyonlands' dts.
>
> The device definition was added by:
> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands
> board")'
> but without any driver support as the dwc2 driver wasn't
> available at that time.
>
> Note: The system can't use the generic "snps,dwc2" compatible
> because of the special ahbcfg configuration. The default
> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
> when the USB and SATA is used concurrently.
Hi,
I don't want to add any more of these param structures to the driver
unless really necessary. We're trying to remove usage of them in favor
of using auto-detected defaults and device properties to override
them.
The AHB Burst is actually one of the ones we were going to do next
because our platform also doesn't work well with INCR4. In fact I'm
thinking of making the default INCR.
If that's all you need then a devicetree binding should be enough
right?
Regards,
John
Re: [PATCH] usb: dwc2: gadget: simplify list handling
On 11/7/2016 11:06 AM, Nicholas Mc Guire wrote:
> The current code is effectively equivalent to list_first_entry_or_null()
> so simply switch and simplify the code.
>
> Fixes: 9c39ddc60ee9 ("USB: s3c-hsotg: Fix stall condition processing")
> Signed-off-by: Nicholas Mc Guire
> ---
> Found by simple coccinelle scanner
>
> Compile tested with: x86_64_defconfig + CONFIG_USB_DWC2=m,
> CONFIG_USB_DWC2_PERIPHERAL=y
>
> Patch is against 4.9.0-rc2 (localversion-next is next-20161028)
>
> drivers/usb/dwc2/gadget.c |6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 24fbebc..9ac8ca0 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1099,10 +1099,8 @@ static int dwc2_hsotg_process_req_status(struct
> dwc2_hsotg *hsotg,
> */
> static struct dwc2_hsotg_req *get_ep_head(struct dwc2_hsotg_ep *hs_ep)
> {
> - if (list_empty(&hs_ep->queue))
> - return NULL;
> -
> - return list_first_entry(&hs_ep->queue, struct dwc2_hsotg_req, queue);
> + return list_first_entry_or_null(&hs_ep->queue,
> + struct dwc2_hsotg_req, queue);
> }
>
> /**
>
Hi,
The same is already queued in Felipe's testing/next.
Regards,
John
Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset
On 10/31/2016 4:19 AM, Felipe Balbi wrote:
>
> Hi,
>
> John Stultz writes:
>> I had seen some odd behavior with HiKey's usb-gadget interface
>> that I finally seemed to have chased down. Basically every other
>> time I pluged in the OTG port, the gadget interface would
>> properly initialize. The other times, I'd get a big WARN_ON
>> in dwc2_hsotg_init_fifo() about the fifo_map not being clear.
>>
>> Ends up If we don't disconnect the gadget state on reset, the
>> fifo-map doesn't get cleared properly, which causes WARN_ON
>> messages and also results in the device not properly being
>> setup as a gadget every other time the OTG port is connected.
>>
>> So this patch adds a call to dwc2_hsotg_disconnect() in the
>> reset path so the state is properly cleared.
>>
>> With it, the gadget interface initializes properly on every
>> plug in.
>>
>> I don't know if this is actually the right fix, but it seems
>> to work well. Feedback would be greatly appreciated!
>>
>> Cc: Wei Xu
>> Cc: Guodong Xu
>> Cc: Chen Yu
>> Cc: Amit Pundir
>> Cc: Rob Herring
>> Cc: Mark Rutland
>> Cc: John Youn
>> Cc: Douglas Anderson
>> Cc: Greg Kroah-Hartman
>> Cc: linux-...@vger.kernel.org
>> Signed-off-by: John Stultz
>> ---
>> drivers/usb/dwc2/gadget.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 24fbebc..5505001 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>>
>> /* Kill any ep0 requests as controller will be reinitialized */
>> kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
>> +/* Make sure everything is disconnected */
>> +dwc2_hsotg_disconnect(hsotg);
>
> Dunno, seems like you're actually working around a different
> bug. Looking at USB Reset handler we have:
>
> if (gintsts & (GINTSTS_USBRST | GINTSTS_RESETDET)) {
>
> u32 usb_status = dwc2_readl(hsotg->regs + GOTGCTL);
> u32 connected = hsotg->connected;
>
> dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);
> dev_dbg(hsotg->dev, "GNPTXSTS=%08x\n",
> dwc2_readl(hsotg->regs + GNPTXSTS));
>
> dwc2_writel(GINTSTS_USBRST, hsotg->regs + GINTSTS);
>
> /* Report disconnection if it is not already done. */
> dwc2_hsotg_disconnect(hsotg);
>
> if (usb_status & GOTGCTL_BSESVLD && connected)
> dwc2_hsotg_core_init_disconnected(hsotg, true);
> }
>
> so, dwc2_hsotg_disconnect() is already called from Reset path. What
> you're doing here is that you could, potentially, call
> driver->disconnect() twice.
>
> The real problem could be your implementation for ->pullup() which
> duplicates part of what ->udc_start() does:
>
> static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> {
> struct dwc2_hsotg *hsotg = to_hsotg(gadget);
> unsigned long flags = 0;
>
> dev_dbg(hsotg->dev, "%s: is_on: %d op_state: %d\n", __func__, is_on,
> hsotg->op_state);
>
> /* Don't modify pullup state while in host mode */
> if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
> hsotg->enabled = is_on;
> return 0;
> }
>
> spin_lock_irqsave(&hsotg->lock, flags);
> if (is_on) {
> hsotg->enabled = 1;
> dwc2_hsotg_core_init_disconnected(hsotg, false);
> dwc2_hsotg_core_connect(hsotg);
> } else {
> dwc2_hsotg_core_disconnect(hsotg);
> dwc2_hsotg_disconnect(hsotg);
> hsotg->enabled = 0;
> }
>
> hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
> return 0;
> }
>
> Here's what I think dwc2_hsotg_pullup() and dwc2_hsotg_udc_start()
> should contain:
>
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 9dc6c482b89e..dbe28947d3a9 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3388,6 +3388,7 @@ static void dwc2_hsotg_init(struct dwc2_hsotg *hsotg)
> dwc2_writel(0, hsotg->regs + DAINTMSK);
>
> /* Be in disconnected state unt
Re: [PATCH] Revert "usb: dwc2: gadget: fix TX FIFO size and address initialization"
On 10/28/2016 8:52 AM, Leo Yan wrote: > This reverts commit aa381a7259c3f53727bcaa8c5f9359e940a0e3fd. > > Reverting this patch, as it incorrectly assumes TX FIFO size is fixed > and cannot change FIFO size; it removes all related dt binding code > and have no chance to set FIFO size at init phase. > > As result, Hi6220 cannot set correct FIFO size for gadget driver and > ethernet on USB function driver is easily to hang. And in the kernel > there also have other platforms set FIFO size for usb controller, > e.g. arch/arm64/boot/dts/rockchip/rk3368.dtsi. > > This patch re-enables dt binding to set FIFO size. > > Signed-off-by: Leo Yan This is already queued for 4.9-rc in Felipe's tree. Hi Felipe, It seems like your fixes branch for 4.9-rc2 didn't get merged upstream. Can we get them pulled for the next -rc? There are platforms that are broken on 4.9 without them. Thanks, John
Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset
On 10/25/2016 2:56 PM, John Stultz wrote: > On Tue, Oct 25, 2016 at 2:29 PM, John Youn wrote: >> On 10/19/2016 11:00 PM, John Stultz wrote: >>> I had seen some odd behavior with HiKey's usb-gadget interface >>> that I finally seemed to have chased down. Basically every other >>> time I pluged in the OTG port, the gadget interface would >>> properly initialize. The other times, I'd get a big WARN_ON >>> in dwc2_hsotg_init_fifo() about the fifo_map not being clear. >>> >>> Ends up If we don't disconnect the gadget state on reset, the >>> fifo-map doesn't get cleared properly, which causes WARN_ON >>> messages and also results in the device not properly being >>> setup as a gadget every other time the OTG port is connected. >>> >>> So this patch adds a call to dwc2_hsotg_disconnect() in the >>> reset path so the state is properly cleared. >>> >>> With it, the gadget interface initializes properly on every >>> plug in. >>> >>> I don't know if this is actually the right fix, but it seems >>> to work well. Feedback would be greatly appreciated! >>> >>> Cc: Wei Xu >>> Cc: Guodong Xu >>> Cc: Chen Yu >>> Cc: Amit Pundir >>> Cc: Rob Herring >>> Cc: Mark Rutland >>> Cc: John Youn >>> Cc: Douglas Anderson >>> Cc: Greg Kroah-Hartman >>> Cc: linux-...@vger.kernel.org >>> Signed-off-by: John Stultz >>> --- >>> drivers/usb/dwc2/gadget.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>> index 24fbebc..5505001 100644 >>> --- a/drivers/usb/dwc2/gadget.c >>> +++ b/drivers/usb/dwc2/gadget.c >>> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct >>> dwc2_hsotg *hsotg, >>> >>> /* Kill any ep0 requests as controller will be reinitialized */ >>> kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET); >>> + /* Make sure everything is disconnected */ >>> + dwc2_hsotg_disconnect(hsotg); >>> >>> if (!is_usb_reset) >>> if (dwc2_core_reset(hsotg)) >>> >> >> Seems fine with our testing. >> >> Acked-by: John Youn > > Awesome! Thanks so much for the review and testing! > > I'm curious, did you happen to have any thoughts or objections on the > "dwc2: Force port resume on switching to device mode" patch > (https://patchwork.kernel.org/patch/9375965/ ) as well? Sorry, must have missed that one. We'll take a look. Regards, John
Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset
On 10/19/2016 11:00 PM, John Stultz wrote: > I had seen some odd behavior with HiKey's usb-gadget interface > that I finally seemed to have chased down. Basically every other > time I pluged in the OTG port, the gadget interface would > properly initialize. The other times, I'd get a big WARN_ON > in dwc2_hsotg_init_fifo() about the fifo_map not being clear. > > Ends up If we don't disconnect the gadget state on reset, the > fifo-map doesn't get cleared properly, which causes WARN_ON > messages and also results in the device not properly being > setup as a gadget every other time the OTG port is connected. > > So this patch adds a call to dwc2_hsotg_disconnect() in the > reset path so the state is properly cleared. > > With it, the gadget interface initializes properly on every > plug in. > > I don't know if this is actually the right fix, but it seems > to work well. Feedback would be greatly appreciated! > > Cc: Wei Xu > Cc: Guodong Xu > Cc: Chen Yu > Cc: Amit Pundir > Cc: Rob Herring > Cc: Mark Rutland > Cc: John Youn > Cc: Douglas Anderson > Cc: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org > Signed-off-by: John Stultz > --- > drivers/usb/dwc2/gadget.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 24fbebc..5505001 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct > dwc2_hsotg *hsotg, > > /* Kill any ep0 requests as controller will be reinitialized */ > kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET); > + /* Make sure everything is disconnected */ > + dwc2_hsotg_disconnect(hsotg); > > if (!is_usb_reset) > if (dwc2_core_reset(hsotg)) > Seems fine with our testing. Acked-by: John Youn Regards, John
Re: [PATCH] phy: Add reset callback for not generic phy
On 10/25/2016 7:15 AM, Randy Li wrote:
> I forget to add a dummy function in case the CONFIG_GENERIC_PHY
> is disabled.
>
> Signed-off-by: Randy Li
Fixes: cac18ecb6f44 ("phy: Add reset callback")
Tested-by: John Youn
Hi Kishon,
Can you take this for 4.9-rc?
Thanks,
John
> ---
> include/linux/phy/phy.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index ee1bed7..78bb0d7 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -253,6 +253,13 @@ static inline int phy_set_mode(struct phy *phy, enum
> phy_mode mode)
> return -ENOSYS;
> }
>
> +static inline int phy_reset(struct phy *phy)
> +{
> + if (!phy)
> + return 0;
> + return -ENOSYS;
> +}
> +
> static inline int phy_get_bus_width(struct phy *phy)
> {
> return -ENOSYS;
>
>
Re: [PATCH v9 1/2] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 10/23/2016 2:33 AM, ayaka wrote:
>
>
> On 10/22/2016 03:27 AM, John Youn wrote:
>> On 10/20/2016 11:38 AM, Randy Li wrote:
>>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>>> port) the PHY can get into a bad state when a wakeup is asserted (not
>>> just a wakeup from full system suspend but also a wakeup from
>>> autosuspend).
>>>
>>> We can get the PHY out of its bad state by asserting its "port reset",
>>> but unfortunately that seems to assert a reset onto the USB bus so it
>>> could confuse things if we don't actually deenumerate / reenumerate the
>>> device.
>>>
>>> We can also get the PHY out of its bad state by fully resetting it using
>>> the reset from the CRU (clock reset unit) in chip, which does a more full
>>> reset. The CRU-based reset appears to actually cause devices on the bus
>>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>>> way).
>>>
>>> It's unfortunate that we need to do a full re-enumeration of devices at
>>> wakeup time, but this is better than alternative of letting the bus get
>>> wedged.
>>>
>>> Signed-off-by: Randy Li
>>> ---
>>> drivers/usb/dwc2/core.h | 1 +
>>> drivers/usb/dwc2/core_intr.c | 11 +++
>>> drivers/usb/dwc2/platform.c | 9 +
>>> 3 files changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index 2a21a04..e91ddbc 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -859,6 +859,7 @@ struct dwc2_hsotg {
>>> unsigned int ll_hw_enabled:1;
>>>
>>> struct phy *phy;
>>> + struct work_struct phy_rst_work;
>>> struct usb_phy *uphy;
>>> struct dwc2_hsotg_plat *plat;
>>> struct regulator_bulk_data
>>> supplies[ARRAY_SIZE(dwc2_hsotg_supply_names)];
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index d85c5c9..c3d2168 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
>>> dwc2_hsotg *hsotg)
>>> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>> {
>>> int ret;
>>> + struct device_node *np = hsotg->dev->of_node;
>>>
>>> /* Clear interrupt */
>>> dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>>> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct
>>> dwc2_hsotg *hsotg)
>>> /* Restart the Phy Clock */
>>> pcgcctl &= ~PCGCTL_STOPPCLK;
>>> dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
>>> +
>>> + /*
>>> +* It is a quirk in Rockchip RK3288, causing by
>>> +* a hardware bug. This will propagate out and
>>> +* eventually we'll re-enumerate the device.
>>> +* Not great but the best we can do
>>> +*/
>>> + if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
>>> + schedule_work(&hsotg->phy_rst_work);
>>> +
>>> mod_timer(&hsotg->wkp_timer,
>>> jiffies + msecs_to_jiffies(71));
>>> } else {
>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>> index 8e1728b..65953cf 100644
>>> --- a/drivers/usb/dwc2/platform.c
>>> +++ b/drivers/usb/dwc2/platform.c
>>> @@ -366,6 +366,14 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>>> return ret;
>>> }
>>>
>>> +/* Only used to reset usb phy at interrupter runtime */
>>> +static void dwc2_reset_phy_work(struct work_struct *data)
>>> +{
>>> + struct dwc2_hsotg *hsotg = container_of(data, struct dwc2_hsotg,
>>> + phy_rst_work);
>>> + phy_reset(hsotg->phy);
>>> +}
>>> +
>>> static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
>>> {
>>> int i, ret;
>>> @@ -410,6 +418,7 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
>>> *hsotg)
>>> return ret;
>>> }
>>> }
>>> + INIT_WORK(&hsotg->phy_rst_work, dwc2_reset_phy_work);
>>>
>>> if (!hsotg->phy) {
>>> hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);
>>>
>> Hi Randy,
>>
>> This fails compile if CONFIG_GENERIC_PHY is disabled. I think you need
>> to make a fix to your phy_reset patch first.
> In the last time, cac18ecb6f44b11bc303d7afbae3887b27938fa4 have not been
> merged, I though the
> [PATCH v8 1/3] phy: Add reset callback for not generic phy have been
> merged before that. when the rebase abandon it.
> Should re-send that patch? As the mainline have not been affected, could
> you arrange a squash for it?
Hi Randy,
Can you resend the fix to Kishon?
The phy_reset patch landed in 4.9-rc so I think he should take the fix
for the next -rc. After that we can send the dwc2 change through
Felipe.
Regards,
John
Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220
On 10/20/2016 5:43 PM, Chen Yu wrote:
> On 2016/10/19 6:21, John Youn wrote:
>> On 10/16/2016 7:42 PM, Chen Yu wrote:
>>>
>>>
>>> On 2016/10/15 3:37, John Youn wrote:
>>>> On 10/13/2016 4:36 PM, John Stultz wrote:
>>>>> From: Chen Yu
>>>>>
>>>>> The Hi6220's usb controller is limited in that it does not
>>>>> automatically autonegotiate the usb speed. Thus it requires a
>>>>> quirk so that we can manually negotiate the best usb speed for
>>>>> the attached device.
>>>>
>>>> Hi,
>>>>
>>>> Could you expand more on this by explaining what exactly is the
>>>> limitation and the workaround?
>>>>
>>>
>>> The USB host limitation of Hisilicon Hi6220 is full-speed and low-speed
>>> devices can not be enumerated when gets plugged behind a hub.
>>>
>>>> [snip]
>>>>
>>>>> +/*
>>>>> + * HPRT0_SPD_HIGH_SPEED: high speed
>>>>> + * HPRT0_SPD_FULL_SPEED: full speed
>>>>> + */
>>>>> +static void dwc2_change_bus_speed(struct usb_hcd *hcd, int speed)
>>>>> +{
>>>>> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>>>> +
>>>>> + if (hsotg->core_params->speed == speed)
>>>>> + return;
>>>>> +
>>>>> + hsotg->core_params->speed = speed;
>>>>> + queue_work(hsotg->wq_otg, &hsotg->wf_otg);
>>>>> +}
>>>>> +
>>>>> +static int dwc2_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
>>>>> +{
>>>>> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>>>> +
>>>>> + if (!hsotg->change_speed_quirk)
>>>>> + return 1;
>>>>> +
>>>>> + hsotg->device_count++;
>>>>
>>>> Why do you need to track the device count?
>>>>
>>>>> + dev_info(hsotg->dev, "Device count is %u after alloc dev\n",
>>>>> + hsotg->device_count);
>>>>> +
>>>>> + return 1;
>>>>> +}
>>>>> +
>>>>> +static void dwc2_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
>>>>> +{
>>>>> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>>>> +
>>>>> + if (!hsotg->change_speed_quirk)
>>>>> + return;
>>>>> +
>>>>> + if (hsotg->device_count)
>>>>> + hsotg->device_count--;
>>>>> +
>>>>> + dev_info(hsotg->dev, "Device count is %u after free dev\n",
>>>>> + hsotg->device_count);
>>>>> +
>>>>> + if (hsotg->device_count == 1 && udev->parent &&
>>>>> + udev->parent->speed > USB_SPEED_UNKNOWN &&
>>>>> + udev->parent->speed < USB_SPEED_HIGH) {
>>>>> + dev_info(hsotg->dev, "Set speed to default high-speed\n");
>>>>> + dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static int dwc2_reset_device(struct usb_hcd *hcd, struct usb_device
>>>>> *udev)
>>>>> +{
>>>>> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>>>> +
>>>>> + if (!hsotg->change_speed_quirk)
>>>>> + return 0;
>>>>> +
>>>>> + if (udev->speed == USB_SPEED_HIGH) {
>>>>> + dev_info(hsotg->dev, "Set speed to high-speed\n");
>>>>> + dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
>>>>> + } else if (udev->speed == USB_SPEED_FULL
>>>>> + || udev->speed == USB_SPEED_LOW) {
>>>>> + dev_info(hsotg->dev, "Set speed to full-speed\n");
>>>>> + dwc2_change_bus_speed(hcd, HPRT0_SPD_FULL_SPEED);
>>>>> + }
>>>>
>>>> It seems you are reinitializing the core every time a device is reset
>>>> and the udev->speed does not match the core_param speed. But how is
>>>> the udev->speed being set correctly if the hw cannot negotiate the
>>>> speed in the first place?
>>>>
>>>
>>> The hardware can n
Re: [PATCH v9 1/2] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 10/20/2016 11:38 AM, Randy Li wrote:
> On the rk3288 USB host-only port (the one that's not the OTG-enabled
> port) the PHY can get into a bad state when a wakeup is asserted (not
> just a wakeup from full system suspend but also a wakeup from
> autosuspend).
>
> We can get the PHY out of its bad state by asserting its "port reset",
> but unfortunately that seems to assert a reset onto the USB bus so it
> could confuse things if we don't actually deenumerate / reenumerate the
> device.
>
> We can also get the PHY out of its bad state by fully resetting it using
> the reset from the CRU (clock reset unit) in chip, which does a more full
> reset. The CRU-based reset appears to actually cause devices on the bus
> to be removed and reinserted, which fixes the problem (albeit in a hacky
> way).
>
> It's unfortunate that we need to do a full re-enumeration of devices at
> wakeup time, but this is better than alternative of letting the bus get
> wedged.
>
> Signed-off-by: Randy Li
> ---
> drivers/usb/dwc2/core.h | 1 +
> drivers/usb/dwc2/core_intr.c | 11 +++
> drivers/usb/dwc2/platform.c | 9 +
> 3 files changed, 21 insertions(+)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2a21a04..e91ddbc 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -859,6 +859,7 @@ struct dwc2_hsotg {
> unsigned int ll_hw_enabled:1;
>
> struct phy *phy;
> + struct work_struct phy_rst_work;
> struct usb_phy *uphy;
> struct dwc2_hsotg_plat *plat;
> struct regulator_bulk_data
> supplies[ARRAY_SIZE(dwc2_hsotg_supply_names)];
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index d85c5c9..c3d2168 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
> dwc2_hsotg *hsotg)
> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> {
> int ret;
> + struct device_node *np = hsotg->dev->of_node;
>
> /* Clear interrupt */
> dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct
> dwc2_hsotg *hsotg)
> /* Restart the Phy Clock */
> pcgcctl &= ~PCGCTL_STOPPCLK;
> dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
> +
> + /*
> + * It is a quirk in Rockchip RK3288, causing by
> + * a hardware bug. This will propagate out and
> + * eventually we'll re-enumerate the device.
> + * Not great but the best we can do
> + */
> + if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
> + schedule_work(&hsotg->phy_rst_work);
> +
> mod_timer(&hsotg->wkp_timer,
> jiffies + msecs_to_jiffies(71));
> } else {
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 8e1728b..65953cf 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -366,6 +366,14 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
> return ret;
> }
>
> +/* Only used to reset usb phy at interrupter runtime */
> +static void dwc2_reset_phy_work(struct work_struct *data)
> +{
> + struct dwc2_hsotg *hsotg = container_of(data, struct dwc2_hsotg,
> + phy_rst_work);
> + phy_reset(hsotg->phy);
> +}
> +
> static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
> {
> int i, ret;
> @@ -410,6 +418,7 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
> return ret;
> }
> }
> + INIT_WORK(&hsotg->phy_rst_work, dwc2_reset_phy_work);
>
> if (!hsotg->phy) {
> hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);
>
Hi Randy,
This fails compile if CONFIG_GENERIC_PHY is disabled. I think you need
to make a fix to your phy_reset patch first.
Regards,
John
Re: [PATCH v8 2/3] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 10/15/2016 8:07 AM, 陈豪 wrote:
> 2016-09-25 2:50 GMT+08:00 Randy Li :
>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>> port) the PHY can get into a bad state when a wakeup is asserted (not
>> just a wakeup from full system suspend but also a wakeup from
>> autosuspend).
>>
>> We can get the PHY out of its bad state by asserting its "port reset",
>> but unfortunately that seems to assert a reset onto the USB bus so it
>> could confuse things if we don't actually deenumerate / reenumerate the
>> device.
>>
>> We can also get the PHY out of its bad state by fully resetting it using
>> the reset from the CRU (clock reset unit) in chip, which does a more full
>> reset. The CRU-based reset appears to actually cause devices on the bus
>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>> way).
>>
>> It's unfortunate that we need to do a full re-enumeration of devices at
>> wakeup time, but this is better than alternative of letting the bus get
>> wedged.
>>
>> Signed-off-by: Randy Li
>> ---
>> drivers/usb/dwc2/core_intr.c | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index d85c5c9..af27edc 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
>> dwc2_hsotg *hsotg)
>> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>> {
>> int ret;
>> + struct device_node *np = hsotg->dev->of_node;
>>
>> /* Clear interrupt */
>> dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct
>> dwc2_hsotg *hsotg)
>> /* Restart the Phy Clock */
>> pcgcctl &= ~PCGCTL_STOPPCLK;
>> dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
>> +
>> + /*
>> +* It is a quirk in Rockchip RK3288, causing by
>> +* a hardware bug. This will propagate out and
>> +* eventually we'll re-enumerate the device.
>> +* Not great but the best we can do.
>> +*/
>> + if (of_device_is_compatible(np,
>> "rockchip,rk3288-usb"))
>> + phy_reset(hsotg->phy);
>
> It will call mutex_lock in phy_reset.
>
>
>> +
>> mod_timer(&hsotg->wkp_timer,
>> jiffies + msecs_to_jiffies(71));
>> } else {
>> --
>> 2.7.4
>>
>
> What is the status of this patch？
> Sleeping calls(phy_reset ==> mutex) shouldn't be used in irq handler.
> Randy will correct that？
>
Yes, that's right, I missed that.
It will have to be fixed.
Regards,
John
Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220
On 10/16/2016 7:42 PM, Chen Yu wrote:
>
>
> On 2016/10/15 3:37, John Youn wrote:
>> On 10/13/2016 4:36 PM, John Stultz wrote:
>>> From: Chen Yu
>>>
>>> The Hi6220's usb controller is limited in that it does not
>>> automatically autonegotiate the usb speed. Thus it requires a
>>> quirk so that we can manually negotiate the best usb speed for
>>> the attached device.
>>
>> Hi,
>>
>> Could you expand more on this by explaining what exactly is the
>> limitation and the workaround?
>>
>
> The USB host limitation of Hisilicon Hi6220 is full-speed and low-speed
> devices can not be enumerated when gets plugged behind a hub.
>
>> [snip]
>>
>>> +/*
>>> + * HPRT0_SPD_HIGH_SPEED: high speed
>>> + * HPRT0_SPD_FULL_SPEED: full speed
>>> + */
>>> +static void dwc2_change_bus_speed(struct usb_hcd *hcd, int speed)
>>> +{
>>> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>> +
>>> + if (hsotg->core_params->speed == speed)
>>> + return;
>>> +
>>> + hsotg->core_params->speed = speed;
>>> + queue_work(hsotg->wq_otg, &hsotg->wf_otg);
>>> +}
>>> +
>>> +static int dwc2_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
>>> +{
>>> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>> +
>>> + if (!hsotg->change_speed_quirk)
>>> + return 1;
>>> +
>>> + hsotg->device_count++;
>>
>> Why do you need to track the device count?
>>
>>> + dev_info(hsotg->dev, "Device count is %u after alloc dev\n",
>>> + hsotg->device_count);
>>> +
>>> + return 1;
>>> +}
>>> +
>>> +static void dwc2_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
>>> +{
>>> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>> +
>>> + if (!hsotg->change_speed_quirk)
>>> + return;
>>> +
>>> + if (hsotg->device_count)
>>> + hsotg->device_count--;
>>> +
>>> + dev_info(hsotg->dev, "Device count is %u after free dev\n",
>>> + hsotg->device_count);
>>> +
>>> + if (hsotg->device_count == 1 && udev->parent &&
>>> + udev->parent->speed > USB_SPEED_UNKNOWN &&
>>> + udev->parent->speed < USB_SPEED_HIGH) {
>>> + dev_info(hsotg->dev, "Set speed to default high-speed\n");
>>> + dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
>>> + }
>>> +}
>>> +
>>> +static int dwc2_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
>>> +{
>>> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>>> +
>>> + if (!hsotg->change_speed_quirk)
>>> + return 0;
>>> +
>>> + if (udev->speed == USB_SPEED_HIGH) {
>>> + dev_info(hsotg->dev, "Set speed to high-speed\n");
>>> + dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
>>> + } else if (udev->speed == USB_SPEED_FULL
>>> + || udev->speed == USB_SPEED_LOW) {
>>> + dev_info(hsotg->dev, "Set speed to full-speed\n");
>>> + dwc2_change_bus_speed(hcd, HPRT0_SPD_FULL_SPEED);
>>> + }
>>
>> It seems you are reinitializing the core every time a device is reset
>> and the udev->speed does not match the core_param speed. But how is
>> the udev->speed being set correctly if the hw cannot negotiate the
>> speed in the first place?
>>
>
> The hardware can negotiate the speed, but communication with a full-speed or
> low-speed device behind a hub is the problem.
>
>> Also should it be for every device? What about if a device gets
>> plugged in behind a hub? I don't think you want to execute this code
>> in that case.
>>
>> This should only affect devices plugged into the root hub, correct?
>> And the hsotg controller only has one root hub port. It seems things
>> could be simplified a bit.
>>
>
> The patch is initially written for Hikey Hi6220 board, and there is a
> hub always connected to root hub, so the patch sets the configuration to
> HPRT0_SPD_HIGH_SPEED when there is only one device(the hub).
Ok, I see.
>
> Thanks for your suggestions, the patch needs modified in these aspect:
> 1. Change the speed setting only when the device is behind a hub in
> dwc2_reset_device.
I still think you will have issues with multiple devices. Since you
have a built-in hub after root hub, it will always be behind the
hub. So whenver you need to change speeds, it will always reset every
device in the tree. Have you tested with multiple devices and also
multiple levels of hubs?
> 2. Change the speed to HPRT0_SPD_HIGH_SPEED only when the last device is a
> hub.
>
> What do you think about the fix? Any suggestions will be appreciate!
I'm not sure if any fix can work for all cases. Has this problem
always been there?
Regards,
John
Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220
On 10/13/2016 4:36 PM, John Stultz wrote:
> From: Chen Yu
>
> The Hi6220's usb controller is limited in that it does not
> automatically autonegotiate the usb speed. Thus it requires a
> quirk so that we can manually negotiate the best usb speed for
> the attached device.
Hi,
Could you expand more on this by explaining what exactly is the
limitation and the workaround?
[snip]
> +/*
> + * HPRT0_SPD_HIGH_SPEED: high speed
> + * HPRT0_SPD_FULL_SPEED: full speed
> + */
> +static void dwc2_change_bus_speed(struct usb_hcd *hcd, int speed)
> +{
> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> +
> + if (hsotg->core_params->speed == speed)
> + return;
> +
> + hsotg->core_params->speed = speed;
> + queue_work(hsotg->wq_otg, &hsotg->wf_otg);
> +}
> +
> +static int dwc2_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> +
> + if (!hsotg->change_speed_quirk)
> + return 1;
> +
> + hsotg->device_count++;
Why do you need to track the device count?
> + dev_info(hsotg->dev, "Device count is %u after alloc dev\n",
> + hsotg->device_count);
> +
> + return 1;
> +}
> +
> +static void dwc2_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> +
> + if (!hsotg->change_speed_quirk)
> + return;
> +
> + if (hsotg->device_count)
> + hsotg->device_count--;
> +
> + dev_info(hsotg->dev, "Device count is %u after free dev\n",
> + hsotg->device_count);
> +
> + if (hsotg->device_count == 1 && udev->parent &&
> + udev->parent->speed > USB_SPEED_UNKNOWN &&
> + udev->parent->speed < USB_SPEED_HIGH) {
> + dev_info(hsotg->dev, "Set speed to default high-speed\n");
> + dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
> + }
> +}
> +
> +static int dwc2_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> +
> + if (!hsotg->change_speed_quirk)
> + return 0;
> +
> + if (udev->speed == USB_SPEED_HIGH) {
> + dev_info(hsotg->dev, "Set speed to high-speed\n");
> + dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
> + } else if (udev->speed == USB_SPEED_FULL
> + || udev->speed == USB_SPEED_LOW) {
> + dev_info(hsotg->dev, "Set speed to full-speed\n");
> + dwc2_change_bus_speed(hcd, HPRT0_SPD_FULL_SPEED);
> + }
It seems you are reinitializing the core every time a device is reset
and the udev->speed does not match the core_param speed. But how is
the udev->speed being set correctly if the hw cannot negotiate the
speed in the first place?
Also should it be for every device? What about if a device gets
plugged in behind a hub? I don't think you want to execute this code
in that case.
This should only affect devices plugged into the root hub, correct?
And the hsotg controller only has one root hub port. It seems things
could be simplified a bit.
Regards,
John
Re: [REGRESSION] "dwc2: gadget: fix TX FIFO size and address initialization" breaks "adb logcat" on hikey
On 10/5/2016 11:33 PM, John Stultz wrote:
> Hey folks,
>So I've run into some trouble that I've narrowed down to commit
> aa381a7259c3 ("usb: dwc2: gadget: fix TX FIFO size and address
> initialization").
>
> After booting up android on HiKey, if I run "adb logcat", I get about
> a page of logs and then I get disconnected. On the device side, I see
> the following in dmesg:
>
> [ 35.652076] dwc2 f72c.usb: dwc2_hsotg_ep_sethalt(ep
> ffc078a4c218 ep1in, 0)
> [ 35.660334] dwc2 f72c.usb: dwc2_hsotg_ep_sethalt(ep
> ffc078a4c018 ep1out, 0)
>
> After which if I stop adbd I see:
>
> [ 99.471363] dwc2 f72c.usb: dwc2_hsotg_ep_stop_xfr: timeout flushing
> fifos
>
> and if I then restart adbd on the device it will sometimes work, but not
> always.
>
> Reverting
> ba48eab8866c ("usb: dwc2: gadget: change variable name to more meaningful")
> and
> aa381a7259c3 ("usb: dwc2: gadget: fix TX FIFO size and address
> initialization")
>
> makes things work again.
>
> Any thoughts as to what might be going wrong here? I'd be happy to
> help debug things.
>
> thanks
> -john
>
Hi John,
Yup. I've already submitted patches to revert these last week, to be
applied sometime during the 4.9-rc cycle. We're working on a more
robust fix for the FIFOs too.
Regards,
John
[PATCH] include/linux/property.h: fix typo/compile error
This fixes commit d76eebfa175e ("include/linux/property.h: fix build
issues with gcc-4.4.4").
With that commit we get the following compile error when using the
PROPERTY_ENTRY_INTEGER_ARRAY macro.
../include/linux/property.h:201:39: error: ‘u32_data’ undeclared (first
use in this function)
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u32, _val_)
^
../include/linux/property.h:193:17: note: in definition of macro
‘PROPERTY_ENTRY_INTEGER_ARRAY’
{ .pointer = { _type_##_data = _val_ } }, \
^
This needs a '.' to reference the union member. It seems this was just
overlooked here since it is done correctly in similar constructs in
other parts of the original commit.
This fix is in preparation of upcoming commits that will use this macro.
Fixes: commit d76eebfa175e ("include/linux/property.h: fix build issues with
gcc-4.4.4")
Signed-off-by: John Youn
---
include/linux/property.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/property.h b/include/linux/property.h
index 3a2f9ae..856e50b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -190,7 +190,7 @@ struct property_entry {
.length = ARRAY_SIZE(_val_) * sizeof(_type_), \
.is_array = true, \
.is_string = false, \
- { .pointer = { _type_##_data = _val_ } }, \
+ { .pointer = { ._type_##_data = _val_ } }, \
}
#define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_) \
--
2.10.0
Re: [PATCH v8 2/3] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 9/24/2016 11:51 AM, Randy Li wrote:
> On the rk3288 USB host-only port (the one that's not the OTG-enabled
> port) the PHY can get into a bad state when a wakeup is asserted (not
> just a wakeup from full system suspend but also a wakeup from
> autosuspend).
>
> We can get the PHY out of its bad state by asserting its "port reset",
> but unfortunately that seems to assert a reset onto the USB bus so it
> could confuse things if we don't actually deenumerate / reenumerate the
> device.
>
> We can also get the PHY out of its bad state by fully resetting it using
> the reset from the CRU (clock reset unit) in chip, which does a more full
> reset. The CRU-based reset appears to actually cause devices on the bus
> to be removed and reinserted, which fixes the problem (albeit in a hacky
> way).
>
> It's unfortunate that we need to do a full re-enumeration of devices at
> wakeup time, but this is better than alternative of letting the bus get
> wedged.
>
> Signed-off-by: Randy Li
> ---
> drivers/usb/dwc2/core_intr.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index d85c5c9..af27edc 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
> dwc2_hsotg *hsotg)
> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> {
> int ret;
> + struct device_node *np = hsotg->dev->of_node;
>
> /* Clear interrupt */
> dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct
> dwc2_hsotg *hsotg)
> /* Restart the Phy Clock */
> pcgcctl &= ~PCGCTL_STOPPCLK;
> dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
> +
> + /*
> + * It is a quirk in Rockchip RK3288, causing by
> + * a hardware bug. This will propagate out and
> + * eventually we'll re-enumerate the device.
> + * Not great but the best we can do.
> + */
> + if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
> + phy_reset(hsotg->phy);
> +
> mod_timer(&hsotg->wkp_timer,
> jiffies + msecs_to_jiffies(71));
> } else {
>
Acked-by: John Youn
Regards,
John
Re: [PATCH] usb: dwc2: add USBTrdTim to initial value
On 9/21/2016 6:43 PM, Pengcheng Li wrote: > After dwc2_core_reset，register is to the initial value, and the USBTrdTim > vale is 0x5. If hsotg->phyif = GUSBCFG_PHYIF8, after the dwc2_writel,the > value is 0xd.So we need to set the USBTrdTim to 0. [++ Felipe] Looks good. But please clean up the subject and message Subject: usb: dwc2: Clear GUSBCFG.UsbTrdTim before setting Description: The USBTRDTIM field needs to be cleared before setting a new value. Otherwise it will result in an incorrect value if phyif == GUSBCFG_PHYIF8. With that Acked-by: John Youn Thanks, John > > Signed-off-by: Pengcheng Li > --- > drivers/usb/dwc2/gadget.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index af46adf..9e52e4f 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -2531,7 +2531,7 @@ void dwc2_hsotg_core_init_disconnected(struct > dwc2_hsotg *hsotg, > /* keep other bits untouched (so e.g. forced modes are not lost) */ > usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); > usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_PHYIF16 | GUSBCFG_SRPCAP | > - GUSBCFG_HNPCAP); > + GUSBCFG_HNPCAP | GUSBCFG_USBTRDTIM_MASK); > > /* set the PLL on, remove the HNP/SRP and set the PHY */ > val = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5; > @@ -3413,7 +3413,7 @@ static void dwc2_hsotg_init(struct dwc2_hsotg *hsotg) > /* keep other bits untouched (so e.g. forced modes are not lost) */ > usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); > usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_PHYIF16 | GUSBCFG_SRPCAP | > - GUSBCFG_HNPCAP); > + GUSBCFG_HNPCAP | GUSBCFG_USBTRDTIM_MASK); > > /* set the PLL on, remove the HNP/SRP and set the PHY */ > trdtim = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5; >
Re: [PATCH] usb: cleanup with list_first_entry_or_null()
On 9/12/2016 10:52 PM, Felipe Balbi wrote: > > Hi Masahiro, > > Masahiro Yamada writes: >> The combo of list_empty() check and return list_first_entry() >> can be replaced with list_first_entry_or_null(). >> >> Signed-off-by: Masahiro Yamada >> --- > > Care to split this into two patches (one for dwc2 and one for dwc3)? > > thanks > For the dwc2 portion, you can add my acked-by. Acked-by: John Youn John
Re: [PATCH 2/2] usb: dwc3: Added a property to set GFLADJ register
On 9/12/2016 8:30 AM, Rob Herring wrote: > On Thu, Sep 01, 2016 at 02:32:33PM -0700, John Youn wrote: >> From: Thinh Nguyen >> >> Added gfladj variable to control the core behavior with respect to >> SOF, ITP, and frame timer functionality. >> >> Currently there is dwc->fladj that holds a single field in GFLADJ >> register (GFLADJ.GFLADJ_30MHZ). A new variable gfladj is added to >> dwc structure to allow setting of the entire GFLADJ register. If >> dwc->gfladj is set, then it has a higher priority than dwc->fladj >> when writing to the GFLADJ register. > > I'm not a fan of magic register values for DT properties. > Sure. Felipe gave the same feedback. We'll fix it. > How many fields in this register that you will ever need to touch? > >> Synopsys HW setup (HAPS DX and phy board) requires a preset to this >> register to improve interoperablitity. For example, the value for >> GFLADJ_REFCLK_LPM_SEL should be set to 0 with ref_clk period of 50. > > This sounds like it should be handled in the driver. Is it a simple, > constant correlation of ref_clk period to this value? I don't know. I'll look into it. Regards, John
Re: [PATCH 1/2] usb: dwc3: Add ref clock period setting
On 9/12/2016 7:09 AM, Rob Herring wrote: > On Thu, Sep 01, 2016 at 02:32:30PM -0700, John Youn wrote: >> From: Thinh Nguyen >> >> Added ref_clk_per for writing to GUCTL.RefClkPer which >> sets the period of ref_clk in nano second. >> >> Signed-off-by: Thinh Nguyen >> Signed-off-by: John Youn >> --- >> Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++ >> drivers/usb/dwc3/core.c| 21 + >> drivers/usb/dwc3/core.h| 5 + >> drivers/usb/dwc3/dwc3-pci.c| 1 + >> 4 files changed, 29 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >> b/Documentation/devicetree/bindings/usb/dwc3.txt >> index e3e6983..aa54ba7 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -50,6 +50,8 @@ Optional properties: >> - snps,hird-threshold: HIRD threshold >> - snps,hsphy_interface: High-Speed PHY interface selection between "utmi" >> for >> UTMI+ and "ulpi" for ULPI when the DWC_USB3_HSPHY_INTERFACE has value 3. >> + - snps,ref_clk_per: value for GUTCL.RefClkPer field that sets the period of >> +ref_clk in nano seconds. > > Use '-' rather than '_' and add a unit suffix (property-units.txt). > Ok. We'll change it to: snps,ref-clk-period-ns John
Re: [PATCH v2 1/1] usb: dwc3: fix Clear Stall EP command failure
On 9/9/2016 12:47 AM, Felipe Balbi wrote:
>
> Hi John,
>
> Lu Baolu writes:
>> Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
>> Stall EP command") sets ClearPendIN bit for all IN endpoints of
>> v2.60a+ cores. This causes ClearStall command fails on 2.60+ cores
>> operating in HighSpeed mode.
>>
>> In page 539 of 2.60a specification:
>>
>> "When issuing Clear Stall command for IN endpoints in SuperSpeed
>> mode, the software must set the "ClearPendIN" bit to '1' to
>> clear any pending IN transcations, so that the device does not
>> expect any ACK TP from the host for the data sent earlier."
>>
>> It's obvious that we only need to apply this rule to those IN
>> endpoints that currently operating in SuperSpeed mode.
>>
>> Fixes: 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear Stall EP
>> command")
>> Cc: # v4.7+
>> Signed-off-by: Lu Baolu
>> ---
>> v1->v2:
>> - check current instead of maximum speed
>> - improve the commit message
>>
>> drivers/usb/dwc3/gadget.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 7a8d3d8..4fd9fda 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -348,7 +348,8 @@ static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep
>> *dep)
>> * IN transfers due to a mishandled error condition. Synopsys
>> * STAR 9000614252.
>> */
>> -if (dep->direction && (dwc->revision >= DWC3_REVISION_260A))
>> +if (dep->direction && (dwc->revision >= DWC3_REVISION_260A) &&
>> +(dwc->gadget.speed >= USB_SPEED_SUPER))
>> cmd |= DWC3_DEPCMD_CLEARPENDIN;
>>
>> memset(¶ms, 0, sizeof(params));
>
> since this is patching what you changed, can you have a look? It makes
> sense to me.
>
Looks good to me
John
Re: [PATCH 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 9/8/2016 7:27 AM, Randy Li wrote:
> On the rk3288 USB host-only port (the one that's not the OTG-enabled
> port) the PHY can get into a bad state when a wakeup is asserted (not
> just a wakeup from full system suspend but also a wakeup from
> autosuspend).
>
> We can get the PHY out of its bad state by asserting its "port reset",
> but unfortunately that seems to assert a reset onto the USB bus so it
> could confuse things if we don't actually deenumerate / reenumerate the
> device.
>
> We can also get the PHY out of its bad state by fully resetting it using
> the reset from the CRU (clock reset unit) in chip, which does a more full
> reset. The CRU-based reset appears to actually cause devices on the bus
> to be removed and reinserted, which fixes the problem (albeit in a hacky
> way).
>
> It's unfortunate that we need to do a full re-enumeration of devices at
> wakeup time, but this is better than alternative of letting the bus get
> wedged.
>
> Signed-off-by: Randy Li
> ---
> drivers/usb/dwc2/core_intr.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index d85c5c9..5b9b671 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
> dwc2_hsotg *hsotg)
> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> {
> int ret;
> + struct device_node *np = hsotg->dev->of_node;
>
> /* Clear interrupt */
> dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct
> dwc2_hsotg *hsotg)
> /* Restart the Phy Clock */
> pcgcctl &= ~PCGCTL_STOPPCLK;
> dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
> +
> + /*
> + * It is a quirk in Rockchip RK3288, causing by
> + * a hardware bug. This will propagate out and
> + * eventually we'll re-enumerate the device.
> + * Not great but the best we can do
Hi Randy,
Please run ./scripts/checkpatch.pl on your patches and fix trailing
whitespace in above two lines.
Thanks,
John
[PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays
When a force mode bit is set and the IDDIG debounce filter is enabled,
there is a delay for the forced mode to take effect. This delay is due
to the IDDIG debounce filter and is variable depending on the platform's
PHY clock speed. To account for this delay we can poll for the expected
mode.
On a clear force mode, since we don't know what mode to poll for, delay
for a fixed 100 ms. This is the maximum delay based on the slowest PHY
clock speed.
Tested-by: Stefan Wahren
Signed-off-by: John Youn
---
drivers/usb/dwc2/core.c | 31 ++-
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index a3068e0..fa9b26b 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -395,9 +395,9 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
* Checks are done in this function to determine whether doing a force
* would be valid or not.
*
- * If a force is done, it requires a 25ms delay to take effect.
- *
- * Returns true if the mode was forced.
+ * If a force is done, it requires a IDDIG debounce filter delay if
+ * the filter is configured and enabled. We poll the current mode of
+ * the controller to account for this delay.
*/
static bool dwc2_force_mode(struct dwc2_hsotg *hsotg, bool host)
{
@@ -432,12 +432,18 @@ static bool dwc2_force_mode(struct dwc2_hsotg *hsotg,
bool host)
gusbcfg |= set;
dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
- msleep(25);
+ dwc2_wait_for_mode(hsotg, host);
return true;
}
-/*
- * Clears the force mode bits.
+/**
+ * dwc2_clear_force_mode() - Clears the force mode bits.
+ *
+ * After clearing the bits, wait up to 100 ms to account for any
+ * potential IDDIG filter delay. We can't know if we expect this delay
+ * or not because the value of the connector ID status is affected by
+ * the force mode. We only need to call this once during probe if
+ * dr_mode == OTG.
*/
static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
{
@@ -448,11 +454,8 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
- /*
-* NOTE: This long sleep is _very_ important, otherwise the core will
-* not stay in host mode after a connector ID change!
-*/
- msleep(25);
+ if (dwc2_iddig_filter_enabled(hsotg))
+ usleep_range(10, 11);
}
/*
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
__func__, hsotg->dr_mode);
break;
}
-
- /*
-* NOTE: This is required for some rockchip soc based
-* platforms.
-*/
- msleep(50);
}
/*
--
2.9.0
[PATCH v5 0/3] usb: dwc2: Fix core reset and force mode delays
This series accounts for the delay from the IDDIG debounce filter when switching modes. This delay is a function of the PHY clock speed and can range from 5-50 ms. This delay must be taken into account on core reset and force modes. A full explanation is provided in the patch commit log and code comments. This revision of the series increases the IDDIG delay to 100 ms. Some rockchip platforms seem to timeout even with 50 ms so I have doubled this. Appreciate any testing on RK3188 platforms. v5: * Added Stefan Wahren's tested-by * Dropped RFT v4: * Increased the IDDIG delay to 110ms. * Removed tested-by for patch 2 since I have changed the delays. v3: * Added tested-bys for patch 1-2 * Fixed an issue where a function was not returning a value * Dropped patch 4 v2: * Broke up the last patch of the original series Regards, John John Youn (3): usb: dwc2: gadget: Only initialize device if in device mode usb: dwc2: Add delay to core soft reset usb: dwc2: Properly account for the force mode delays drivers/usb/dwc2/core.c | 126 +++--- drivers/usb/dwc2/core.h | 1 + drivers/usb/dwc2/gadget.c | 7 ++- drivers/usb/dwc2/hw.h | 1 + 4 files changed, 116 insertions(+), 19 deletions(-) -- 2.9.0
[PATCH v5 2/3] usb: dwc2: Add delay to core soft reset
Add a delay to the core soft reset function to account for the IDDIG
debounce filter.
If the current mode is host, either due to the force mode bit being
set (which persists after core reset) or the connector id pin, a core
soft reset will temporarily reset the mode to device and a delay from
the IDDIG debounce filter will occur before going back to host mode.
Tested-by: Stefan Wahren
Signed-off-by: John Youn
---
drivers/usb/dwc2/core.c | 95 +
drivers/usb/dwc2/core.h | 1 +
drivers/usb/dwc2/hw.h | 1 +
3 files changed, 97 insertions(+)
diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 4135a5f..a3068e0 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -238,6 +238,77 @@ int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg)
return ret;
}
+/**
+ * dwc2_wait_for_mode() - Waits for the controller mode.
+ * @hsotg: Programming view of the DWC_otg controller.
+ * @host_mode: If true, waits for host mode, otherwise device mode.
+ */
+static void dwc2_wait_for_mode(struct dwc2_hsotg *hsotg,
+ bool host_mode)
+{
+ ktime_t start;
+ ktime_t end;
+ unsigned int timeout = 110;
+
+ dev_vdbg(hsotg->dev, "Waiting for %s mode\n",
+host_mode ? "host" : "device");
+
+ start = ktime_get();
+
+ while (1) {
+ s64 ms;
+
+ if (dwc2_is_host_mode(hsotg) == host_mode) {
+ dev_vdbg(hsotg->dev, "%s mode set\n",
+host_mode ? "Host" : "Device");
+ break;
+ }
+
+ end = ktime_get();
+ ms = ktime_to_ms(ktime_sub(end, start));
+
+ if (ms >= (s64)timeout) {
+ dev_warn(hsotg->dev, "%s: Couldn't set %s mode\n",
+__func__, host_mode ? "host" : "device");
+ break;
+ }
+
+ usleep_range(1000, 2000);
+ }
+}
+
+/**
+ * dwc2_iddig_filter_enabled() - Returns true if the IDDIG debounce
+ * filter is enabled.
+ */
+static bool dwc2_iddig_filter_enabled(struct dwc2_hsotg *hsotg)
+{
+ u32 gsnpsid;
+ u32 ghwcfg4;
+
+ if (!dwc2_hw_is_otg(hsotg))
+ return false;
+
+ /* Check if core configuration includes the IDDIG filter. */
+ ghwcfg4 = dwc2_readl(hsotg->regs + GHWCFG4);
+ if (!(ghwcfg4 & GHWCFG4_IDDIG_FILT_EN))
+ return false;
+
+ /*
+* Check if the IDDIG debounce filter is bypassed. Available
+* in core version >= 3.10a.
+*/
+ gsnpsid = dwc2_readl(hsotg->regs + GSNPSID);
+ if (gsnpsid >= DWC2_CORE_REV_3_10a) {
+ u32 gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+
+ if (gotgctl & GOTGCTL_DBNCE_FLTR_BYPASS)
+ return false;
+ }
+
+ return true;
+}
+
/*
* Do core a soft reset of the core. Be careful with this because it
* resets all the internal state machines of the core.
@@ -246,9 +317,30 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
{
u32 greset;
int count = 0;
+ bool wait_for_host_mode = false;
dev_vdbg(hsotg->dev, "%s()\n", __func__);
+ /*
+* If the current mode is host, either due to the force mode
+* bit being set (which persists after core reset) or the
+* connector id pin, a core soft reset will temporarily reset
+* the mode to device. A delay from the IDDIG debounce filter
+* will occur before going back to host mode.
+*
+* Determine whether we will go back into host mode after a
+* reset and account for this delay after the reset.
+*/
+ if (dwc2_iddig_filter_enabled(hsotg)) {
+ u32 gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+ u32 gusbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
+
+ if (!(gotgctl & GOTGCTL_CONID_B) ||
+ (gusbcfg & GUSBCFG_FORCEHOSTMODE)) {
+ wait_for_host_mode = true;
+ }
+ }
+
/* Core Soft Reset */
greset = dwc2_readl(hsotg->regs + GRSTCTL);
greset |= GRSTCTL_CSFTRST;
@@ -277,6 +369,9 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
}
} while (!(greset & GRSTCTL_AHBIDLE));
+ if (wait_for_host_mode)
+ dwc2_wait_for_mode(hsotg, true);
+
return 0;
}
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 78526ee..466c220 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -882,6 +882,7 @@ struct dwc2_hsotg {
#define DWC2_CORE_REV_2_92a0x4f54292a
#define DWC2_CORE_REV_2_94a0x4f54294a
#define DWC2_CO
[PATCH v5 1/3] usb: dwc2: gadget: Only initialize device if in device mode
In dwc2_hsotg_udc_start(), don't initialize the controller for device
mode unless we are actually in device mode.
Tested-by: Heiko Stuebner
Tested-by: Stefan Wahren
Signed-off-by: John Youn
---
drivers/usb/dwc2/gadget.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 94bd19a..4cd6403 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3466,8 +3466,11 @@ static int dwc2_hsotg_udc_start(struct usb_gadget
*gadget,
otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);
spin_lock_irqsave(&hsotg->lock, flags);
- dwc2_hsotg_init(hsotg);
- dwc2_hsotg_core_init_disconnected(hsotg, false);
+ if (dwc2_hw_is_device(hsotg)) {
+ dwc2_hsotg_init(hsotg);
+ dwc2_hsotg_core_init_disconnected(hsotg, false);
+ }
+
hsotg->enabled = 0;
spin_unlock_irqrestore(&hsotg->lock, flags);
--
2.9.0
[PATCH v2] usb: dwc3: Fix dr_mode validation
From: Thinh Nguyen
This patch follows the similar fix in dwc2. See
commit 5268ed9d2e3b ("usb: dwc2: Fix dr_mode validation")
Currently, the dr_mode is only checked against the module configuration.
It also needs to be checked against the hardware capablities.
The driver now checks if both the module configuration and hardware are
capable of the dr_mode value. If not, then it will issue a warning and
fall back to a supported value. If it is unable to fall back to a
suitable value, then the probe will fail.
Behavior summary:
module : actual
HW config dr_mode : dr_mode
-
host host any : host
host devany : INVALID
host otgany : host
dev host any : INVALID
dev devany : dev
dev otgany : dev
otg host any : host
otg devany : dev
otg otgany : dr_mode
Signed-off-by: Thinh Nguyen
Signed-off-by: John Youn
---
v2:
* Use the cached hwparams
* Use a switch statement
drivers/usb/dwc3/core.c | 65 -
drivers/usb/dwc3/core.h | 5 +++-
2 files changed, 58 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index d6d3fa0..dcacc70 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -49,6 +49,57 @@
#define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */
+/**
+ * dwc3_get_dr_mode - Validates and sets dr_mode
+ * @dwc: pointer to our context structure
+ */
+static int dwc3_get_dr_mode(struct dwc3 *dwc)
+{
+ enum usb_dr_mode mode;
+ struct device *dev = dwc->dev;
+ unsigned int hw_mode;
+
+ if (dwc->dr_mode == USB_DR_MODE_UNKNOWN)
+ dwc->dr_mode = USB_DR_MODE_OTG;
+
+ mode = dwc->dr_mode;
+ hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+
+ switch (hw_mode) {
+ case DWC3_GHWPARAMS0_MODE_GADGET:
+ if (IS_ENABLED(CONFIG_USB_DWC3_HOST)) {
+ dev_err(dev,
+ "Controller does not support host mode.\n");
+ return -EINVAL;
+ }
+ mode = USB_DR_MODE_PERIPHERAL;
+ break;
+ case DWC3_GHWPARAMS0_MODE_HOST:
+ if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) {
+ dev_err(dev,
+ "Controller does not support device mode.\n");
+ return -EINVAL;
+ }
+ mode = USB_DR_MODE_HOST;
+ break;
+ default:
+ if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
+ mode = USB_DR_MODE_HOST;
+ else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
+ mode = USB_DR_MODE_PERIPHERAL;
+ }
+
+ if (mode != dwc->dr_mode) {
+ dev_warn(dev,
+"Configuration mismatch. dr_mode forced to %s\n",
+mode == USB_DR_MODE_HOST ? "host" : "gadget");
+
+ dwc->dr_mode = mode;
+ }
+
+ return 0;
+}
+
void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
{
u32 reg;
@@ -1023,17 +1074,9 @@ static int dwc3_probe(struct platform_device *pdev)
goto err2;
}
- if (IS_ENABLED(CONFIG_USB_DWC3_HOST) &&
- (dwc->dr_mode == USB_DR_MODE_OTG ||
- dwc->dr_mode == USB_DR_MODE_UNKNOWN))
- dwc->dr_mode = USB_DR_MODE_HOST;
- else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
- (dwc->dr_mode == USB_DR_MODE_OTG ||
- dwc->dr_mode == USB_DR_MODE_UNKNOWN))
- dwc->dr_mode = USB_DR_MODE_PERIPHERAL;
-
- if (dwc->dr_mode == USB_DR_MODE_UNKNOWN)
- dwc->dr_mode = USB_DR_MODE_OTG;
+ ret = dwc3_get_dr_mode(dwc);
+ if (ret)
+ goto err3;
ret = dwc3_alloc_scratch_buffers(dwc);
if (ret)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b2317e7..6b60e42 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -245,7 +245,10 @@
#define DWC3_GEVNTSIZ_SIZE(n) ((n) & 0x)
/* Global HWPARAMS0 Register */
-#define DWC3_GHWPARAMS0_USB3_MODE(n) ((n) & 0x3)
+#define DWC3_GHWPARAMS0_MODE(n)((n) & 0x3)
+#define DWC3_GHWPARAMS0_MODE_GADGET0
+#define DWC3_GHWPARAMS0_MODE_HOST 1
+#define DWC3_GHWPARAMS0_MODE_DRD 2
#define DWC3_GHWPARAMS0_MBUS_TYPE(n) (((n) >> 3) & 0x7)
#define DWC3_GHWPARAMS0_SBUS_TYPE(n) (((n) >> 6) & 0x3)
#define DWC3_GHWPARAMS0_MDWIDTH(n) (((n) >> 8) & 0xff)
--
2.9.0
Re: [PATCH v7 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 9/6/2016 6:19 PM, Ayaka wrote:
>
>
> 從我的 iPad 傳送
>
>> John Youn 於 2016年9月7日 上午2:54 寫道：
>>
>>> On 9/5/2016 10:15 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>>> On Sunday 04 September 2016 03:25 AM, Randy Li wrote:
>>>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>>>> port) the PHY can get into a bad state when a wakeup is asserted (not
>>>> just a wakeup from full system suspend but also a wakeup from
>>>> autosuspend).
>>>>
>>>> We can get the PHY out of its bad state by asserting its "port reset",
>>>> but unfortunately that seems to assert a reset onto the USB bus so it
>>>> could confuse things if we don't actually deenumerate / reenumerate the
>>>> device.
>>>>
>>>> We can also get the PHY out of its bad state by fully resetting it using
>>>> the reset from the CRU (clock reset unit) in chip, which does a more full
>>>> reset. The CRU-based reset appears to actually cause devices on the bus
>>>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>>>> way).
>>>>
>>>> It's unfortunate that we need to do a full re-enumeration of devices at
>>>> wakeup time, but this is better than alternative of letting the bus get
>>>> wedged.
>>>>
>>>> Signed-off-by: Randy Li
>>>> ---
>>>> drivers/usb/dwc2/core_intr.c | 12
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>> index d85c5c9..08485b7 100644
>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
>>>> dwc2_hsotg *hsotg)
>>>> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>> {
>>>>int ret;
>>>> +struct device_node *np = hsotg->dev->of_node;
>>>>
>>>>/* Clear interrupt */
>>>>dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>>>> @@ -379,6 +380,17 @@ static void dwc2_handle_wakeup_detected_intr(struct
>>>> dwc2_hsotg *hsotg)
>>>>/* Restart the Phy Clock */
>>>>pcgcctl &= ~PCGCTL_STOPPCLK;
>>>>dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
>>>> +
>>>> +/*
>>>> + * It is a quirk in Rockchip RK3288, causing by
>>>> + * a hardware bug. This will propagate out and
>>>> + * eventually we'll re-enumerate the device.
>>>> + * Not great but the best we can do
>>>> + */
>>>> +if (of_device_is_compatible(np, "rockchip,rk3288-usb")
>>>> +&& (NULL != hsotg->phy->ops->reset))
>>>> +hsotg->phy->ops->reset(hsotg->phy);
>>>
>>> never call the phy ops directly from the controller driver. It has to be
>>> protected as well.
>>
>> It looks like we should be calling an API function instead, correct?
>>
> Could you give me a example for the wrapper of phy ops?
Check these files:
* include/linux/phy/phy.h
* drivers/phy/phy-core.c
Search for "phy_set_mode".
I'm thinking there should be a "phy_reset" API function which calls
the phy->ops->reset in a similar manner. With the same runtime checks
and stub function if CONFIG_GENERIC_PHY is not enabled.
Regards,
John
Re: [PATCH v7 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 9/5/2016 10:15 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Sunday 04 September 2016 03:25 AM, Randy Li wrote:
>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>> port) the PHY can get into a bad state when a wakeup is asserted (not
>> just a wakeup from full system suspend but also a wakeup from
>> autosuspend).
>>
>> We can get the PHY out of its bad state by asserting its "port reset",
>> but unfortunately that seems to assert a reset onto the USB bus so it
>> could confuse things if we don't actually deenumerate / reenumerate the
>> device.
>>
>> We can also get the PHY out of its bad state by fully resetting it using
>> the reset from the CRU (clock reset unit) in chip, which does a more full
>> reset. The CRU-based reset appears to actually cause devices on the bus
>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>> way).
>>
>> It's unfortunate that we need to do a full re-enumeration of devices at
>> wakeup time, but this is better than alternative of letting the bus get
>> wedged.
>>
>> Signed-off-by: Randy Li
>> ---
>> drivers/usb/dwc2/core_intr.c | 12
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index d85c5c9..08485b7 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
>> dwc2_hsotg *hsotg)
>> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>> {
>> int ret;
>> +struct device_node *np = hsotg->dev->of_node;
>>
>> /* Clear interrupt */
>> dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>> @@ -379,6 +380,17 @@ static void dwc2_handle_wakeup_detected_intr(struct
>> dwc2_hsotg *hsotg)
>> /* Restart the Phy Clock */
>> pcgcctl &= ~PCGCTL_STOPPCLK;
>> dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
>> +
>> +/*
>> + * It is a quirk in Rockchip RK3288, causing by
>> + * a hardware bug. This will propagate out and
>> + * eventually we'll re-enumerate the device.
>> + * Not great but the best we can do
>> + */
>> +if (of_device_is_compatible(np, "rockchip,rk3288-usb")
>> +&& (NULL != hsotg->phy->ops->reset))
>> +hsotg->phy->ops->reset(hsotg->phy);
>
> never call the phy ops directly from the controller driver. It has to be
> protected as well.
It looks like we should be calling an API function instead, correct?
Similar to the wrappers for the other ops.
Regards,
John
[PATCH] usb: dwc3: Fix dr_mode validation
From: Thinh Nguyen
This patch follows the similar fix in dwc2. See
commit 5268ed9d2e3b ("usb: dwc2: Fix dr_mode validation")
Currently, the dr_mode is only checked against the module configuration.
It also needs to be checked against the hardware capablities.
The driver now checks if both the module configuration and hardware are
capable of the dr_mode value. If not, then it will issue a warning and
fall back to a supported value. If it is unable to fall back to a
suitable value, then the probe will fail.
Behavior summary:
module : actual
HW config dr_mode : dr_mode
-
host host any : host
host devany : INVALID
host otgany : host
dev host any : INVALID
dev devany : dev
dev otgany : dev
otg host any : host
otg devany : dev
otg otgany : dr_mode
Signed-off-by: Thinh Nguyen
Signed-off-by: John Youn
---
drivers/usb/dwc3/core.c | 84 ++---
drivers/usb/dwc3/core.h | 5 ++-
2 files changed, 77 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index d6d3fa0..f30cffa 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -49,6 +49,76 @@
#define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */
+/* Returns the controller's GHWPARAMS0.DWC_USB3_MODE. */
+static unsigned int dwc3_usb3_mode(struct dwc3 *dwc)
+{
+ u32 ghwparams0_2_0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
+
+ return DWC3_GHWPARAMS0_MODE(ghwparams0_2_0);
+}
+
+/* Returns true if the controller is host-only. */
+static bool dwc3_hw_is_host(struct dwc3 *dwc)
+{
+ unsigned int mode = dwc3_usb3_mode(dwc);
+
+ return mode == DWC3_GHWPARAMS0_MODE_HOST;
+}
+
+/* Returns true if the controller is device-only. */
+static bool dwc3_hw_is_gadget(struct dwc3 *dwc)
+{
+ unsigned int mode = dwc3_usb3_mode(dwc);
+
+ return mode == DWC3_GHWPARAMS0_MODE_GADGET;
+}
+
+/**
+ * dwc3_get_dr_mode - Validates and sets dr_mode
+ * @dwc: pointer to our context structure
+ */
+static int dwc3_get_dr_mode(struct dwc3 *dwc)
+{
+ enum usb_dr_mode mode;
+ struct device *dev = dwc->dev;
+
+ if (dwc->dr_mode == USB_DR_MODE_UNKNOWN)
+ dwc->dr_mode = USB_DR_MODE_OTG;
+
+ mode = dwc->dr_mode;
+
+ if (dwc3_hw_is_gadget(dwc)) {
+ if (IS_ENABLED(CONFIG_USB_DWC3_HOST)) {
+ dev_err(dev,
+ "Controller does not support host mode.\n");
+ return -EINVAL;
+ }
+ mode = USB_DR_MODE_PERIPHERAL;
+ } else if (dwc3_hw_is_host(dwc)) {
+ if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) {
+ dev_err(dev,
+ "Controller does not support device mode.\n");
+ return -EINVAL;
+ }
+ mode = USB_DR_MODE_HOST;
+ } else {
+ if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
+ mode = USB_DR_MODE_HOST;
+ else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
+ mode = USB_DR_MODE_PERIPHERAL;
+ }
+
+ if (mode != dwc->dr_mode) {
+ dev_warn(dev,
+"Configuration mismatch. dr_mode forced to %s\n",
+mode == USB_DR_MODE_HOST ? "host" : "gadget");
+
+ dwc->dr_mode = mode;
+ }
+
+ return 0;
+}
+
void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
{
u32 reg;
@@ -1023,17 +1093,9 @@ static int dwc3_probe(struct platform_device *pdev)
goto err2;
}
- if (IS_ENABLED(CONFIG_USB_DWC3_HOST) &&
- (dwc->dr_mode == USB_DR_MODE_OTG ||
- dwc->dr_mode == USB_DR_MODE_UNKNOWN))
- dwc->dr_mode = USB_DR_MODE_HOST;
- else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
- (dwc->dr_mode == USB_DR_MODE_OTG ||
- dwc->dr_mode == USB_DR_MODE_UNKNOWN))
- dwc->dr_mode = USB_DR_MODE_PERIPHERAL;
-
- if (dwc->dr_mode == USB_DR_MODE_UNKNOWN)
- dwc->dr_mode = USB_DR_MODE_OTG;
+ ret = dwc3_get_dr_mode(dwc);
+ if (ret)
+ goto err3;
ret = dwc3_alloc_scratch_buffers(dwc);
if (ret)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b2317e7..6b60e42 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -245,7 +245,10 @@
#define DWC3_GEVNTSIZ_SIZE(n) ((n) & 0x)
/* Global HWPARAMS0 Register */
-#define DWC3_GHWPARAMS0_USB3_MODE(n) ((n) & 0x3)
+#define DWC3_GHWPARAMS0_MODE(n)((n) & 0
[PATCH 1/2] usb: dwc3: Add ref clock period setting
From: Thinh Nguyen
Added ref_clk_per for writing to GUCTL.RefClkPer which
sets the period of ref_clk in nano second.
Signed-off-by: Thinh Nguyen
Signed-off-by: John Youn
---
Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
drivers/usb/dwc3/core.c| 21 +
drivers/usb/dwc3/core.h| 5 +
drivers/usb/dwc3/dwc3-pci.c| 1 +
4 files changed, 29 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index e3e6983..aa54ba7 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -50,6 +50,8 @@ Optional properties:
- snps,hird-threshold: HIRD threshold
- snps,hsphy_interface: High-Speed PHY interface selection between "utmi" for
UTMI+ and "ulpi" for ULPI when the DWC_USB3_HSPHY_INTERFACE has value 3.
+ - snps,ref_clk_per: value for GUTCL.RefClkPer field that sets the period of
+ ref_clk in nano seconds.
- snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
register for post-silicon frame length adjustment when the
fladj_30mhz_sdbnd signal is invalid or incorrect.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index d6d3fa0..b96bf69 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -145,6 +145,23 @@ static int dwc3_soft_reset(struct dwc3 *dwc)
return 0;
}
+/*
+ * dwc3_ref_clock_period - Sets the reference clock period
+ * @dwc3: Pointer to our controller context structure
+ */
+static void dwc3_ref_clock_period(struct dwc3 *dwc)
+{
+ u32 reg;
+
+ if (dwc->ref_clk_per == 0)
+ return;
+
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
+ reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
+ reg |= DWC3_GUCTL_REFCLKPER(dwc->ref_clk_per);
+ dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
+}
+
/*
* dwc3_frame_length_adjustment - Adjusts frame length if required
* @dwc3: Pointer to our controller context structure
@@ -670,6 +687,9 @@ static int dwc3_core_init(struct dwc3 *dwc)
if (ret)
goto err1;
+ /* Initialize ref clock period */
+ dwc3_ref_clock_period(dwc);
+
/* Adjust Frame Length */
dwc3_frame_length_adjustment(dwc);
@@ -984,6 +1004,7 @@ static int dwc3_probe(struct platform_device *pdev)
&dwc->hsphy_interface);
device_property_read_u32(dev, "snps,quirk-frame-length-adjustment",
&dwc->fladj);
+ device_property_read_u32(dev, "snps,ref_clk_per", &dwc->ref_clk_per);
dwc->lpm_nyet_threshold = lpm_nyet_threshold;
dwc->tx_de_emphasis = tx_de_emphasis;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b2317e7..ab58334 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -198,6 +198,10 @@
#define DWC3_GCTL_GBLHIBERNATIONEN (1 << 1)
#define DWC3_GCTL_DSBLCLKGTNG (1 << 0)
+/* Global USB3 user Control Register */
+#define DWC3_GUCTL_REFCLKPER(n)((n) << 22)
+#define DWC3_GUCTL_REFCLKPER_MASK DWC3_GUCTL_REFCLKPER(0x3ff)
+
/* Global USB2 PHY Configuration Register */
#define DWC3_GUSB2PHYCFG_PHYSOFTRST(1 << 31)
#define DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS (1 << 30)
@@ -872,6 +876,7 @@ struct dwc3 {
enum usb_dr_modedr_mode;
enum usb_phy_interface hsphy_mode;
+ u32 ref_clk_per;
u32 fladj;
u32 irq_gadget;
u32 nr_scratch;
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 2eb84d6..254788b 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -127,6 +127,7 @@ static int dwc3_pci_quirks(struct pci_dev *pdev, struct
platform_device *dwc3)
PROPERTY_ENTRY_BOOL("snps,usb3_lpm_capable"),
PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
PROPERTY_ENTRY_BOOL("snps,dis_enblslpm_quirk"),
+ PROPERTY_ENTRY_U32("snps,ref_clk_per", 0x32),
{ },
};
--
2.9.0
[PATCH 2/2] usb: dwc3: Added a property to set GFLADJ register
From: Thinh Nguyen
Added gfladj variable to control the core behavior with respect to
SOF, ITP, and frame timer functionality.
Currently there is dwc->fladj that holds a single field in GFLADJ
register (GFLADJ.GFLADJ_30MHZ). A new variable gfladj is added to
dwc structure to allow setting of the entire GFLADJ register. If
dwc->gfladj is set, then it has a higher priority than dwc->fladj
when writing to the GFLADJ register.
Synopsys HW setup (HAPS DX and phy board) requires a preset to this
register to improve interoperablitity. For example, the value for
GFLADJ_REFCLK_LPM_SEL should be set to 0 with ref_clk period of 50.
Signed-off-by: Thinh Nguyen
Signed-off-by: John Youn
---
Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
drivers/usb/dwc3/core.c| 5 +
drivers/usb/dwc3/core.h| 1 +
drivers/usb/dwc3/dwc3-pci.c| 1 +
4 files changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index aa54ba7..cad4bf6 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -52,6 +52,8 @@ Optional properties:
UTMI+ and "ulpi" for ULPI when the DWC_USB3_HSPHY_INTERFACE has value 3.
- snps,ref_clk_per: value for GUTCL.RefClkPer field that sets the period of
ref_clk in nano seconds.
+ - snps,gfladj: if set, overides the value in the GFLADJ register. Takes
+ precedence over snps,quirk-frame-length-adjustment.
- snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
register for post-silicon frame length adjustment when the
fladj_30mhz_sdbnd signal is invalid or incorrect.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b96bf69..dfe1b1f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -693,6 +693,9 @@ static int dwc3_core_init(struct dwc3 *dwc)
/* Adjust Frame Length */
dwc3_frame_length_adjustment(dwc);
+ if (dwc->gfladj != 0x)
+ dwc3_writel(dwc->regs, DWC3_GFLADJ, dwc->gfladj);
+
usb_phy_set_suspend(dwc->usb2_phy, 0);
usb_phy_set_suspend(dwc->usb3_phy, 0);
ret = phy_power_on(dwc->usb2_generic_phy);
@@ -955,6 +958,7 @@ static int dwc3_probe(struct platform_device *pdev)
dwc->maximum_speed = usb_get_maximum_speed(dev);
dwc->dr_mode = usb_get_dr_mode(dev);
dwc->hsphy_mode = of_usb_get_phy_mode(dev->of_node);
+ dwc->gfladj = 0x;
dwc->has_lpm_erratum = device_property_read_bool(dev,
"snps,has-lpm-erratum");
@@ -1004,6 +1008,7 @@ static int dwc3_probe(struct platform_device *pdev)
&dwc->hsphy_interface);
device_property_read_u32(dev, "snps,quirk-frame-length-adjustment",
&dwc->fladj);
+ device_property_read_u32(dev, "snps,gfladj", &dwc->gfladj);
device_property_read_u32(dev, "snps,ref_clk_per", &dwc->ref_clk_per);
dwc->lpm_nyet_threshold = lpm_nyet_threshold;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index ab58334..61336a9 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -878,6 +878,7 @@ struct dwc3 {
u32 ref_clk_per;
u32 fladj;
+ u32 gfladj;
u32 irq_gadget;
u32 nr_scratch;
u32 u1u2;
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 254788b..71a52db 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -128,6 +128,7 @@ static int dwc3_pci_quirks(struct pci_dev *pdev, struct
platform_device *dwc3)
PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
PROPERTY_ENTRY_BOOL("snps,dis_enblslpm_quirk"),
PROPERTY_ENTRY_U32("snps,ref_clk_per", 0x32),
+ PROPERTY_ENTRY_U32("snps,gfladj", 0xc80),
{ },
};
--
2.9.0
[RFT PATCH v4 1/3] usb: dwc2: gadget: Only initialize device if in device mode
In dwc2_hsotg_udc_start(), don't initialize the controller for device
mode unless we are actually in device mode.
Tested-by: Heiko Stuebner
Signed-off-by: John Youn
---
drivers/usb/dwc2/gadget.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 94bd19a..4cd6403 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3466,8 +3466,11 @@ static int dwc2_hsotg_udc_start(struct usb_gadget
*gadget,
otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);
spin_lock_irqsave(&hsotg->lock, flags);
- dwc2_hsotg_init(hsotg);
- dwc2_hsotg_core_init_disconnected(hsotg, false);
+ if (dwc2_hw_is_device(hsotg)) {
+ dwc2_hsotg_init(hsotg);
+ dwc2_hsotg_core_init_disconnected(hsotg, false);
+ }
+
hsotg->enabled = 0;
spin_unlock_irqrestore(&hsotg->lock, flags);
--
2.9.0
[RFT PATCH v4 3/3] usb: dwc2: Properly account for the force mode delays
When a force mode bit is set and the IDDIG debounce filter is enabled,
there is a delay for the forced mode to take effect. This delay is due
to the IDDIG debounce filter and is variable depending on the platform's
PHY clock speed. To account for this delay we can poll for the expected
mode.
On a clear force mode, since we don't know what mode to poll for, delay
for a fixed 100 ms. This is the maximum delay based on the slowest PHY
clock speed.
Signed-off-by: John Youn
---
drivers/usb/dwc2/core.c | 31 ++-
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index a3068e0..fa9b26b 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -395,9 +395,9 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
* Checks are done in this function to determine whether doing a force
* would be valid or not.
*
- * If a force is done, it requires a 25ms delay to take effect.
- *
- * Returns true if the mode was forced.
+ * If a force is done, it requires a IDDIG debounce filter delay if
+ * the filter is configured and enabled. We poll the current mode of
+ * the controller to account for this delay.
*/
static bool dwc2_force_mode(struct dwc2_hsotg *hsotg, bool host)
{
@@ -432,12 +432,18 @@ static bool dwc2_force_mode(struct dwc2_hsotg *hsotg,
bool host)
gusbcfg |= set;
dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
- msleep(25);
+ dwc2_wait_for_mode(hsotg, host);
return true;
}
-/*
- * Clears the force mode bits.
+/**
+ * dwc2_clear_force_mode() - Clears the force mode bits.
+ *
+ * After clearing the bits, wait up to 100 ms to account for any
+ * potential IDDIG filter delay. We can't know if we expect this delay
+ * or not because the value of the connector ID status is affected by
+ * the force mode. We only need to call this once during probe if
+ * dr_mode == OTG.
*/
static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
{
@@ -448,11 +454,8 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
- /*
-* NOTE: This long sleep is _very_ important, otherwise the core will
-* not stay in host mode after a connector ID change!
-*/
- msleep(25);
+ if (dwc2_iddig_filter_enabled(hsotg))
+ usleep_range(10, 11);
}
/*
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
__func__, hsotg->dr_mode);
break;
}
-
- /*
-* NOTE: This is required for some rockchip soc based
-* platforms.
-*/
- msleep(50);
}
/*
--
2.9.0
[RFT PATCH v4 2/3] usb: dwc2: Add delay to core soft reset
Add a delay to the core soft reset function to account for the IDDIG
debounce filter.
If the current mode is host, either due to the force mode bit being
set (which persists after core reset) or the connector id pin, a core
soft reset will temporarily reset the mode to device and a delay from
the IDDIG debounce filter will occur before going back to host mode.
Signed-off-by: John Youn
---
drivers/usb/dwc2/core.c | 95 +
drivers/usb/dwc2/core.h | 1 +
drivers/usb/dwc2/hw.h | 1 +
3 files changed, 97 insertions(+)
diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 4135a5f..a3068e0 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -238,6 +238,77 @@ int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg)
return ret;
}
+/**
+ * dwc2_wait_for_mode() - Waits for the controller mode.
+ * @hsotg: Programming view of the DWC_otg controller.
+ * @host_mode: If true, waits for host mode, otherwise device mode.
+ */
+static void dwc2_wait_for_mode(struct dwc2_hsotg *hsotg,
+ bool host_mode)
+{
+ ktime_t start;
+ ktime_t end;
+ unsigned int timeout = 110;
+
+ dev_vdbg(hsotg->dev, "Waiting for %s mode\n",
+host_mode ? "host" : "device");
+
+ start = ktime_get();
+
+ while (1) {
+ s64 ms;
+
+ if (dwc2_is_host_mode(hsotg) == host_mode) {
+ dev_vdbg(hsotg->dev, "%s mode set\n",
+host_mode ? "Host" : "Device");
+ break;
+ }
+
+ end = ktime_get();
+ ms = ktime_to_ms(ktime_sub(end, start));
+
+ if (ms >= (s64)timeout) {
+ dev_warn(hsotg->dev, "%s: Couldn't set %s mode\n",
+__func__, host_mode ? "host" : "device");
+ break;
+ }
+
+ usleep_range(1000, 2000);
+ }
+}
+
+/**
+ * dwc2_iddig_filter_enabled() - Returns true if the IDDIG debounce
+ * filter is enabled.
+ */
+static bool dwc2_iddig_filter_enabled(struct dwc2_hsotg *hsotg)
+{
+ u32 gsnpsid;
+ u32 ghwcfg4;
+
+ if (!dwc2_hw_is_otg(hsotg))
+ return false;
+
+ /* Check if core configuration includes the IDDIG filter. */
+ ghwcfg4 = dwc2_readl(hsotg->regs + GHWCFG4);
+ if (!(ghwcfg4 & GHWCFG4_IDDIG_FILT_EN))
+ return false;
+
+ /*
+* Check if the IDDIG debounce filter is bypassed. Available
+* in core version >= 3.10a.
+*/
+ gsnpsid = dwc2_readl(hsotg->regs + GSNPSID);
+ if (gsnpsid >= DWC2_CORE_REV_3_10a) {
+ u32 gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+
+ if (gotgctl & GOTGCTL_DBNCE_FLTR_BYPASS)
+ return false;
+ }
+
+ return true;
+}
+
/*
* Do core a soft reset of the core. Be careful with this because it
* resets all the internal state machines of the core.
@@ -246,9 +317,30 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
{
u32 greset;
int count = 0;
+ bool wait_for_host_mode = false;
dev_vdbg(hsotg->dev, "%s()\n", __func__);
+ /*
+* If the current mode is host, either due to the force mode
+* bit being set (which persists after core reset) or the
+* connector id pin, a core soft reset will temporarily reset
+* the mode to device. A delay from the IDDIG debounce filter
+* will occur before going back to host mode.
+*
+* Determine whether we will go back into host mode after a
+* reset and account for this delay after the reset.
+*/
+ if (dwc2_iddig_filter_enabled(hsotg)) {
+ u32 gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+ u32 gusbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
+
+ if (!(gotgctl & GOTGCTL_CONID_B) ||
+ (gusbcfg & GUSBCFG_FORCEHOSTMODE)) {
+ wait_for_host_mode = true;
+ }
+ }
+
/* Core Soft Reset */
greset = dwc2_readl(hsotg->regs + GRSTCTL);
greset |= GRSTCTL_CSFTRST;
@@ -277,6 +369,9 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
}
} while (!(greset & GRSTCTL_AHBIDLE));
+ if (wait_for_host_mode)
+ dwc2_wait_for_mode(hsotg, true);
+
return 0;
}
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 78526ee..466c220 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -882,6 +882,7 @@ struct dwc2_hsotg {
#define DWC2_CORE_REV_2_92a0x4f54292a
#define DWC2_CORE_REV_2_94a0x4f54294a
#define DWC2_CORE_REV_3_00a0x4f54300a
+#define DWC
[RFT PATCH v4 0/3] usb: dwc2: Fix core reset and force mode delays
This series accounts for the delay from the IDDIG debounce filter when switching modes. This delay is a function of the PHY clock speed and can range from 5-50 ms. This delay must be taken into account on core reset and force modes. A full explanation is provided in the patch commit log and code comments. This revision of the series increases the IDDIG delay to 100 ms. Some rockchip platforms seem to timeout even with 50 ms so I have doubled this. Appreciate any testing on RK3188 and RPi platforms. v4: * Increased the IDDIG delay to 110ms. * Removed tested-by for patch 2 since I have changed the delays. v3: * Added tested-bys for patch 1-2 * Fixed an issue where a function was not returning a value * Dropped patch 4 v2: * Broke up the last patch of the original series Regards, John John Youn (3): usb: dwc2: gadget: Only initialize device if in device mode usb: dwc2: Add delay to core soft reset usb: dwc2: Properly account for the force mode delays drivers/usb/dwc2/core.c | 126 +++--- drivers/usb/dwc2/core.h | 1 + drivers/usb/dwc2/gadget.c | 7 ++- drivers/usb/dwc2/hw.h | 1 + 4 files changed, 116 insertions(+), 19 deletions(-) -- 2.9.0
[PATCH 0/6] usb: dwc2: gadget: Fix TX FIFO handling
Hi, This is a resend of a patch series originally submitted by Robert Baldyga back in February. Somehow I missed it back then. Sorry about that. It should fix some FIFO programming related issues on RPi and maybe other platforms. Also, this series makes the g-tx-fifo-size DT property obsolete. I've added a patch at the end of the series to document that. Regards, John John Youn (1): Documentation: devicetree: dwc2: Deprecate g-tx-fifo-size Robert Baldyga (5): usb: dwc2: gadget: use ep->fifo_index in context of FIFO registers usb: dwc2: gadget: fix TX FIFO size and address initialization usb: dwc2: gadget: change variable name to more meaningful usb: dwc2: gadget: remove dead code from dwc2_hsotg_ep_enable() usb: dwc2: gadget: free TX FIFO after killing requests Documentation/devicetree/bindings/usb/dwc2.txt | 5 +- drivers/usb/dwc2/core.h| 7 --- drivers/usb/dwc2/gadget.c | 79 +++--- 3 files changed, 23 insertions(+), 68 deletions(-) -- 2.9.0
[PATCH 3/6] usb: dwc2: gadget: change variable name to more meaningful
From: Robert Baldyga
Since we handle FIFOs and endpoint separately, using variable named 'ep'
in context of FIFO is misleading, hence we rename it to 'fifo'.
Signed-off-by: Robert Baldyga
Signed-off-by: John Youn
---
drivers/usb/dwc2/gadget.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 5c24c85..ac267fd 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -186,7 +186,7 @@ static void dwc2_hsotg_ctrl_epint(struct dwc2_hsotg *hsotg,
*/
static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
{
- unsigned int ep;
+ unsigned int fifo;
unsigned int addr;
int timeout;
u32 dptxfsizn;
@@ -217,8 +217,8 @@ static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
* them to endpoints dynamically according to maxpacket size value of
* given endpoint.
*/
- for (ep = 1; ep < MAX_EPS_CHANNELS; ep++) {
- dptxfsizn = dwc2_readl(hsotg->regs + DPTXFSIZN(ep));
+ for (fifo = 1; fifo < MAX_EPS_CHANNELS; fifo++) {
+ dptxfsizn = dwc2_readl(hsotg->regs + DPTXFSIZN(fifo));
val = (dptxfsizn & FIFOSIZE_DEPTH_MASK) | addr;
addr += dptxfsizn >> FIFOSIZE_DEPTH_SHIFT;
@@ -226,7 +226,7 @@ static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
if (addr > hsotg->fifo_mem)
break;
- dwc2_writel(val, hsotg->regs + DPTXFSIZN(ep));
+ dwc2_writel(val, hsotg->regs + DPTXFSIZN(fifo));
}
/*
--
2.9.0
[PATCH 5/6] usb: dwc2: gadget: free TX FIFO after killing requests
From: Robert Baldyga As kill_all_requests() potentially flushes TX FIFO, we should should free FIFO after calling it. Otherwise FIFO could stay unflushed properly. Signed-off-by: Robert Baldyga Signed-off-by: John Youn --- drivers/usb/dwc2/gadget.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index c8a182c..4d84c32 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3120,10 +3120,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) spin_lock_irqsave(&hsotg->lock, flags); - hsotg->fifo_map &= ~(1<fifo_index); - hs_ep->fifo_index = 0; - hs_ep->fifo_size = 0; - ctrl = dwc2_readl(hsotg->regs + epctrl_reg); ctrl &= ~DXEPCTL_EPENA; ctrl &= ~DXEPCTL_USBACTEP; @@ -3138,6 +3134,10 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) /* terminate all requests with shutdown */ kill_all_requests(hsotg, hs_ep, -ESHUTDOWN); + hsotg->fifo_map &= ~(1 << hs_ep->fifo_index); + hs_ep->fifo_index = 0; + hs_ep->fifo_size = 0; + spin_unlock_irqrestore(&hsotg->lock, flags); return 0; } -- 2.9.0
[PATCH 6/6] Documentation: devicetree: dwc2: Deprecate g-tx-fifo-size
This property is not needed because the periodic fifos are not configurable. So it was incorrect to add this property in the first place. Signed-off-by: John Youn --- Documentation/devicetree/bindings/usb/dwc2.txt | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt index 20a68bf..7d16ebf 100644 --- a/Documentation/devicetree/bindings/usb/dwc2.txt +++ b/Documentation/devicetree/bindings/usb/dwc2.txt @@ -26,7 +26,10 @@ Refer to phy/phy-bindings.txt for generic phy consumer properties - g-use-dma: enable dma usage in gadget driver. - g-rx-fifo-size: size of rx fifo size in gadget mode. - g-np-tx-fifo-size: size of non-periodic tx fifo size in gadget mode. -- g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) in gadget mode. + +Deprecated properties: +- g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) + in gadget mode. Example: -- 2.9.0
[PATCH 4/6] usb: dwc2: gadget: remove dead code from dwc2_hsotg_ep_enable()
From: Robert Baldyga
Since FIFO is always freed in dwc2_hsotg_ep_disable(), ep->fifo_index
is always 0 in dwc2_hsotg_ep_enable(), hence code inside if() block is
never executed.
Signed-off-by: Robert Baldyga
Signed-off-by: John Youn
---
drivers/usb/dwc2/gadget.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index ac267fd..c8a182c 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3043,22 +3043,11 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
break;
}
- /* If fifo is already allocated for this ep */
- if (hs_ep->fifo_index) {
- size = hs_ep->ep.maxpacket * hs_ep->mc;
- /* If bigger fifo is required deallocate current one */
- if (size > hs_ep->fifo_size) {
- hsotg->fifo_map &= ~(1 << hs_ep->fifo_index);
- hs_ep->fifo_index = 0;
- hs_ep->fifo_size = 0;
- }
- }
-
/*
* if the hardware has dedicated fifos, we must give each IN EP
* a unique tx-fifo even if it is non-periodic.
*/
- if (dir_in && hsotg->dedicated_fifos && !hs_ep->fifo_index) {
+ if (dir_in && hsotg->dedicated_fifos) {
u32 fifo_index = 0;
u32 fifo_size = UINT_MAX;
size = hs_ep->ep.maxpacket*hs_ep->mc;
--
2.9.0
[PATCH 2/6] usb: dwc2: gadget: fix TX FIFO size and address initialization
From: Robert Baldyga
According to DWC2 documentation, DPTxFSize field of DPTXFSIZn register
is read only, which means that software cannot change FIFO size.
Register description says:
"The value of this register is the Largest Device Mode Periodic Tx Data
FIFO Depth (parameter OTG_TX_DPERIO_DFIFO_DEPTH_n), as specified during
coreConsultant configuration."
That means, that we have to setup only FIFO start addresses (DPTxFStAddr),
taking into account reset values of DPTxFSize.
Initialize FIFO start addresses properly and remove unneeded core related
to incorrect FIFO size initialization.
Signed-off-by: Robert Baldyga
Signed-off-by: John Youn
---
drivers/usb/dwc2/core.h | 7 ---
drivers/usb/dwc2/gadget.c | 47 ---
2 files changed, 8 insertions(+), 46 deletions(-)
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 9fae029..78526ee 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -259,13 +259,6 @@ enum dwc2_lx_state {
DWC2_L3,/* Off state */
};
-/*
- * Gadget periodic tx fifo sizes as used by legacy driver
- * EP0 is not included
- */
-#define DWC2_G_P_LEGACY_TX_FIFO_SIZE {256, 256, 256, 256, 768, 768, 768, \
- 768, 0, 0, 0, 0, 0, 0, 0}
-
/* Gadget ep0 states */
enum dwc2_ep0_state {
DWC2_EP0_SETUP,
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index f6086d6..5c24c85 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -189,6 +189,7 @@ static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
unsigned int ep;
unsigned int addr;
int timeout;
+ u32 dptxfsizn;
u32 val;
/* Reset fifo map if not correctly cleared during previous session */
@@ -217,13 +218,13 @@ static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
* given endpoint.
*/
for (ep = 1; ep < MAX_EPS_CHANNELS; ep++) {
- if (!hsotg->g_tx_fifo_sz[ep])
- continue;
- val = addr;
- val |= hsotg->g_tx_fifo_sz[ep] << FIFOSIZE_DEPTH_SHIFT;
- WARN_ONCE(addr + hsotg->g_tx_fifo_sz[ep] > hsotg->fifo_mem,
- "insufficient fifo memory");
- addr += hsotg->g_tx_fifo_sz[ep];
+ dptxfsizn = dwc2_readl(hsotg->regs + DPTXFSIZN(ep));
+
+ val = (dptxfsizn & FIFOSIZE_DEPTH_MASK) | addr;
+ addr += dptxfsizn >> FIFOSIZE_DEPTH_SHIFT;
+
+ if (addr > hsotg->fifo_mem)
+ break;
dwc2_writel(val, hsotg->regs + DPTXFSIZN(ep));
}
@@ -3814,36 +3815,10 @@ static void dwc2_hsotg_dump(struct dwc2_hsotg *hsotg)
static void dwc2_hsotg_of_probe(struct dwc2_hsotg *hsotg)
{
struct device_node *np = hsotg->dev->of_node;
- u32 len = 0;
- u32 i = 0;
/* Enable dma if requested in device tree */
hsotg->g_using_dma = of_property_read_bool(np, "g-use-dma");
- /*
- * Register TX periodic fifo size per endpoint.
- * EP0 is excluded since it has no fifo configuration.
- */
- if (!of_find_property(np, "g-tx-fifo-size", &len))
- goto rx_fifo;
-
- len /= sizeof(u32);
-
- /* Read tx fifo sizes other than ep0 */
- if (of_property_read_u32_array(np, "g-tx-fifo-size",
- &hsotg->g_tx_fifo_sz[1], len))
- goto rx_fifo;
-
- /* Add ep0 */
- len++;
-
- /* Make remaining TX fifos unavailable */
- if (len < MAX_EPS_CHANNELS) {
- for (i = len; i < MAX_EPS_CHANNELS; i++)
- hsotg->g_tx_fifo_sz[i] = 0;
- }
-
-rx_fifo:
/* Register RX fifo size */
of_property_read_u32(np, "g-rx-fifo-size", &hsotg->g_rx_fifo_sz);
@@ -3865,13 +3840,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
struct device *dev = hsotg->dev;
int epnum;
int ret;
- int i;
- u32 p_tx_fifo[] = DWC2_G_P_LEGACY_TX_FIFO_SIZE;
/* Initialize to legacy fifo configuration values */
hsotg->g_rx_fifo_sz = 2048;
hsotg->g_np_g_tx_fifo_sz = 1024;
- memcpy(&hsotg->g_tx_fifo_sz[1], p_tx_fifo, sizeof(p_tx_fifo));
/* Device tree specific probe */
dwc2_hsotg_of_probe(hsotg);
@@ -3889,9 +3861,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
dev_dbg(dev, "NonPeriodic TXFIFO size: %d\n",
hsotg->g_np_g_tx_fifo_sz);
dev_dbg(dev, "RXFIFO size: %d\n", hsotg->g_rx_fifo_sz);
- for (i = 0; i < MAX_EPS_CHANNELS; i++)
- dev_dbg(dev, "Periodic TXFIFO%2d size: %d\n", i,
- hsotg->g_tx_fifo_sz[i]);
hsotg->gadget.max_speed = USB_SPEED_HIGH;
hsotg->gadget.ops = &dwc2_hsotg_gadget_ops;
--
2.9.0
[PATCH 1/6] usb: dwc2: gadget: use ep->fifo_index in context of FIFO registers
From: Robert Baldyga
In context of FIFO registers we use ep->fifo_index instead of ep->index.
Signed-off-by: Robert Baldyga
Signed-off-by: John Youn
---
drivers/usb/dwc2/gadget.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index af46adf..f6086d6 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -388,7 +388,8 @@ static int dwc2_hsotg_write_fifo(struct dwc2_hsotg *hsotg,
return -ENOSPC;
}
} else if (hsotg->dedicated_fifos && hs_ep->index != 0) {
- can_write = dwc2_readl(hsotg->regs + DTXFSTS(hs_ep->index));
+ can_write = dwc2_readl(hsotg->regs +
+ DTXFSTS(hs_ep->fifo_index));
can_write &= 0x;
can_write *= 4;
@@ -2432,7 +2433,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
if (!hsotg->dedicated_fifos)
return;
- size = (dwc2_readl(hsotg->regs + DTXFSTS(ep->index)) & 0x) * 4;
+ size = (dwc2_readl(hsotg->regs + DTXFSTS(ep->fifo_index)) & 0x) * 4;
if (size < ep->fifo_size)
dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
}
--
2.9.0
Re: [RESEND PATCH 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform
On 8/29/2016 12:51 AM, Felipe Balbi wrote:
>
> Hi,
>
> John Youn writes:
>
> [...]
>
>>>>> + */
>>>>> + if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
>>>>> + hsotg->phy->ops->reset(hsotg->phy);
>>>>> +
>>>>
>>>> You should probably check for NULL before calling the reset()
>>>> callback.
>>> Sure.
>>>>
>>>> Also, I'd rather see a generic quirk property that you set for your
>>>> platform.
>>>>
>>>> Something like "phy_reset_on_wakeup_quirk".
>>> But Rob Herring want me to implied by the SoC specific compatible
>>> string. I agree with him. It is certainly bug in RK3288 platform.
>>> It is no found no the other platform.
>>
>> Ok, I missed that before.
>>
>> Based on the drivers I'm familiar with (like dwc3), you would
>> typically add a "quirk" anyways.
>>
>> Felipe,
>>
>> Do you have some policy or preference on this?
>
> if it's not a dwc2-generic feature, then let's do it via compatible
> flag, sure. What we don't want is for things like:
>
> if (is_compatible('synopsys') || is_compatible('rockchip') ||
> is_compatible('foobar') ... )
>
> For that, we'd be better of adding a generic quirk flag which several
> can use.
>
Alright sounds reasonable.
Randy, could you respin with the other feedback? There's no need to
add a quirk.
Regards,
John
