Re: [PATCH] usb: dwc2: add optional usb ecc reset bit
Hi John, was wondering if you have gotten a chance to review this? Thank, Dinh On 11/01/2017 10:34 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 <dingu...@kernel.org> > --- > 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; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Suspend to disk with usb connected fails to detect usb(reset) during resume
On 11/08/2017 06:29 AM, Ivid Suvarna wrote: > On Tue, Nov 7, 2017 at 9:19 PM, Alan Sternwrote: >> On Tue, 7 Nov 2017, Ivid Suvarna wrote: >> >>> Hi, >>> >>> I am trying to support suspend to disk(hibernate) on Hikey with 4.4 >>> kernel. During suspend, I could see the usb devices getting reset and I >>> can access them properly. Message can be seen as below: >>> >>> usb 1-1: reset high-speed USB device number 2 using dwc2 >>> usb 1-1.2: reset high-speed USB device number 3 using dwc2 >>> usb 1-1.2.4: reset high-speed USB device number 4 using dwc2 >> >> You might start by asking why the devices need to be reset during >> suspend. I can't see any reason for doing this. >> >> Alan Stern >> > > Yes please. Could you explain as to why the usb needs to reset while > suspending? > This scenario is also observed in normal suspend to ram. > > And why the USB gets disconnected on resume from hibernation. > I checked the register dump of usb in /sys/kernel/debug and all > registers after resume have zero value. > Registers with zero value is indicative of a clock not getting turned on, at least that I have encountered with DWC2 on the SoCFPGA platform. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: dwc2: add optional usb ecc reset bit
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 <dingu...@kernel.org> --- 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; } -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dwc2: usb: Unable to clear channel error
On 10/23/2017 09:03 AM, Minas Harutyunyan wrote: > On 10/19/2017 5:35 PM, Dinh Nguyen wrote: >> >> >> On 10/19/2017 06:55 AM, Grigor Tovmasyan wrote: >>> On 10/18/2017 6:07 PM, Marek Vasut wrote: >>>> On 10/18/2017 04:05 PM, Dinh Nguyen wrote: >>>>> Hi, >>>>> >>>>> I'm trying to bringup the DWC2 USB IP version 330A on a new Stratix10 >>>>> SoC and have encountered this error in both Linux and U-Boot: >>>>> >>>>> U-Boot(version v2017.09) >>>>> >>>>> # usb start >>>>> starting USB... >>>>> USB0: Core Release: 3.30a >>>>> dwc_otg_core_host_init: Timeout! >>>>> dwc_otg_core_host_init: Timeout! >>>>> >>>>> Linux(kernel v4.13) >>>>> >>>>> [1.299891] dwc2 ffb0.usb: DWC OTG Controller >>>>> [1.304628] dwc2 ffb0.usb: new USB bus registered, assigned bus >>>>> number 1 >>>>> [1.311698] dwc2 ffb0.usb: irq 13, io mem 0xffb0 >>>>> [1.318309] dwc2 ffb0.usb: Unable to clear enable on channel 0 >>>>> [1.325749] dwc2 ffb0.usb: Unable to clear enable on channel 1 >>>>> [1.333187] dwc2 ffb0.usb: Unable to clear enable on channel 2 >>>>> [1.340626] dwc2 ffb0.usb: Unable to clear enable on channel 3 >>>>> [1.348064] dwc2 ffb0.usb: Unable to clear enable on channel 4 >>>>> [1.355503] dwc2 ffb0.usb: Unable to clear enable on channel 5 >>>>> [1.362941] dwc2 ffb0.usb: Unable to clear enable on channel 6 >>>>> [1.370379] dwc2 ffb0.usb: Unable to clear enable on channel 7 >>>>> [1.377818] dwc2 ffb0.usb: Unable to clear enable on channel 8 >>>>> [1.385256] dwc2 ffb0.usb: Unable to clear enable on channel 9 >>>>> [1.392694] dwc2 ffb0.usb: Unable to clear enable on channel 10 >>>>> [1.400218] dwc2 ffb0.usb: Unable to clear enable on channel 11 >>>>> [1.407743] dwc2 ffb0.usb: Unable to clear enable on channel 12 >>>>> [1.415269] dwc2 ffb0.usb: Unable to clear enable on channel 13 >>>>> [1.422794] dwc2 ffb0.usb: Unable to clear enable on channel 14 >>>>> >>>>> Just wondering if anyone might have an idea on what could be causing >>>>> this error? >>>> >>>> Maybe some clock are not enabled ? >>>> >>> >>> Hi , >>> >>> Are you following board/hisilicon/hikey/README file instructions when >>> using U-Boot? Specially paragraph FLASHING point 4, where discussed >>> "dwc_otg_core_host_init: Timeout!" message. >>> >> >> I saw that, but I don't know how that applies to a Stratix10 platform? >> >> Dinh >> >> > Hi, > > Did you able to resolve this issue? If not, please provide verbose debug > log and register dump. > I double-checked the clocks and it looks like all of the necessary clocks are enabled. Here are the log and register dump for U-Boot, I'll follow-up with the Linux log shortly. For U-Boot: SOCFPGA_STRATIX10 # usb start starting USB... USB0: Core Release: 3.30a dwc_otg_core_host_init: Timeout (reg=ffb00500 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb00520 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb00540 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb00560 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb00580 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb005a0 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb005c0 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb005e0 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb00600 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb00620 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb00640 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ffb00660 mask=8000 wait_set=0) dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout (reg=ff
Re: dwc2: usb: Unable to clear channel error
On 10/19/2017 06:55 AM, Grigor Tovmasyan wrote: > On 10/18/2017 6:07 PM, Marek Vasut wrote: >> On 10/18/2017 04:05 PM, Dinh Nguyen wrote: >>> Hi, >>> >>> I'm trying to bringup the DWC2 USB IP version 330A on a new Stratix10 >>> SoC and have encountered this error in both Linux and U-Boot: >>> >>> U-Boot(version v2017.09) >>> >>> # usb start >>> starting USB... >>> USB0: Core Release: 3.30a >>> dwc_otg_core_host_init: Timeout! >>> dwc_otg_core_host_init: Timeout! >>> >>> Linux(kernel v4.13) >>> >>> [1.299891] dwc2 ffb0.usb: DWC OTG Controller >>> [1.304628] dwc2 ffb0.usb: new USB bus registered, assigned bus >>> number 1 >>> [1.311698] dwc2 ffb0.usb: irq 13, io mem 0xffb0 >>> [1.318309] dwc2 ffb0.usb: Unable to clear enable on channel 0 >>> [1.325749] dwc2 ffb0.usb: Unable to clear enable on channel 1 >>> [1.333187] dwc2 ffb0.usb: Unable to clear enable on channel 2 >>> [1.340626] dwc2 ffb0.usb: Unable to clear enable on channel 3 >>> [1.348064] dwc2 ffb0.usb: Unable to clear enable on channel 4 >>> [1.355503] dwc2 ffb0.usb: Unable to clear enable on channel 5 >>> [1.362941] dwc2 ffb0.usb: Unable to clear enable on channel 6 >>> [1.370379] dwc2 ffb0.usb: Unable to clear enable on channel 7 >>> [1.377818] dwc2 ffb0.usb: Unable to clear enable on channel 8 >>> [1.385256] dwc2 ffb0.usb: Unable to clear enable on channel 9 >>> [1.392694] dwc2 ffb0.usb: Unable to clear enable on channel 10 >>> [1.400218] dwc2 ffb0.usb: Unable to clear enable on channel 11 >>> [1.407743] dwc2 ffb0.usb: Unable to clear enable on channel 12 >>> [1.415269] dwc2 ffb0.usb: Unable to clear enable on channel 13 >>> [1.422794] dwc2 ffb0.usb: Unable to clear enable on channel 14 >>> >>> Just wondering if anyone might have an idea on what could be causing >>> this error? >> >> Maybe some clock are not enabled ? >> > > Hi , > > Are you following board/hisilicon/hikey/README file instructions when > using U-Boot? Specially paragraph FLASHING point 4, where discussed > "dwc_otg_core_host_init: Timeout!" message. > I saw that, but I don't know how that applies to a Stratix10 platform? Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dwc2: usb: Unable to clear channel error
On 10/18/2017 09:07 AM, Marek Vasut wrote: > On 10/18/2017 04:05 PM, Dinh Nguyen wrote: >> Hi, >> >> I'm trying to bringup the DWC2 USB IP version 330A on a new Stratix10 >> SoC and have encountered this error in both Linux and U-Boot: >> >> U-Boot(version v2017.09) >> >> # usb start >> starting USB... >> USB0: Core Release: 3.30a >> dwc_otg_core_host_init: Timeout! >> dwc_otg_core_host_init: Timeout! >> >> Linux(kernel v4.13) >> >> [1.299891] dwc2 ffb0.usb: DWC OTG Controller >> [1.304628] dwc2 ffb0.usb: new USB bus registered, assigned bus >> number 1 >> [1.311698] dwc2 ffb0.usb: irq 13, io mem 0xffb0 >> [1.318309] dwc2 ffb0.usb: Unable to clear enable on channel 0 >> [1.325749] dwc2 ffb0.usb: Unable to clear enable on channel 1 >> [1.333187] dwc2 ffb0.usb: Unable to clear enable on channel 2 >> [1.340626] dwc2 ffb0.usb: Unable to clear enable on channel 3 >> [1.348064] dwc2 ffb0.usb: Unable to clear enable on channel 4 >> [1.355503] dwc2 ffb0.usb: Unable to clear enable on channel 5 >> [1.362941] dwc2 ffb0.usb: Unable to clear enable on channel 6 >> [1.370379] dwc2 ffb0.usb: Unable to clear enable on channel 7 >> [1.377818] dwc2 ffb0.usb: Unable to clear enable on channel 8 >> [1.385256] dwc2 ffb0.usb: Unable to clear enable on channel 9 >> [1.392694] dwc2 ffb0.usb: Unable to clear enable on channel 10 >> [1.400218] dwc2 ffb0.usb: Unable to clear enable on channel 11 >> [1.407743] dwc2 ffb0.usb: Unable to clear enable on channel 12 >> [1.415269] dwc2 ffb0.usb: Unable to clear enable on channel 13 >> [1.422794] dwc2 ffb0.usb: Unable to clear enable on channel 14 >> >> Just wondering if anyone might have an idea on what could be causing >> this error? > > Maybe some clock are not enabled ? > Thanks...I'll check. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
dwc2: usb: Unable to clear channel error
Hi, I'm trying to bringup the DWC2 USB IP version 330A on a new Stratix10 SoC and have encountered this error in both Linux and U-Boot: U-Boot(version v2017.09) # usb start starting USB... USB0: Core Release: 3.30a dwc_otg_core_host_init: Timeout! dwc_otg_core_host_init: Timeout! Linux(kernel v4.13) [1.299891] dwc2 ffb0.usb: DWC OTG Controller [1.304628] dwc2 ffb0.usb: new USB bus registered, assigned bus number 1 [1.311698] dwc2 ffb0.usb: irq 13, io mem 0xffb0 [1.318309] dwc2 ffb0.usb: Unable to clear enable on channel 0 [1.325749] dwc2 ffb0.usb: Unable to clear enable on channel 1 [1.333187] dwc2 ffb0.usb: Unable to clear enable on channel 2 [1.340626] dwc2 ffb0.usb: Unable to clear enable on channel 3 [1.348064] dwc2 ffb0.usb: Unable to clear enable on channel 4 [1.355503] dwc2 ffb0.usb: Unable to clear enable on channel 5 [1.362941] dwc2 ffb0.usb: Unable to clear enable on channel 6 [1.370379] dwc2 ffb0.usb: Unable to clear enable on channel 7 [1.377818] dwc2 ffb0.usb: Unable to clear enable on channel 8 [1.385256] dwc2 ffb0.usb: Unable to clear enable on channel 9 [1.392694] dwc2 ffb0.usb: Unable to clear enable on channel 10 [1.400218] dwc2 ffb0.usb: Unable to clear enable on channel 11 [1.407743] dwc2 ffb0.usb: Unable to clear enable on channel 12 [1.415269] dwc2 ffb0.usb: Unable to clear enable on channel 13 [1.422794] dwc2 ffb0.usb: Unable to clear enable on channel 14 Just wondering if anyone might have an idea on what could be causing this error? Thanks, Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: dwc2: disable erroneous overcurrent condition
For the case where an external VBUS is used, we should enable the external VBUS comparator in the driver. This would prevent an unnecessary overcurrent error which would then disable the host port. This patch uses the standard 'disable-over-current' binding to allow of the option of disabling the over-current condition. Signed-off-by: Dinh Nguyen <dingu...@kernel.org> --- drivers/usb/dwc2/core.h | 4 drivers/usb/dwc2/hcd.c| 5 + drivers/usb/dwc2/params.c | 3 +++ 3 files changed, 12 insertions(+) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 8367d4f9..730d7eb 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -395,6 +395,9 @@ enum dwc2_ep0_state { * (default when phy_type is UTMI+ or ULPI) * 1 - 6 MHz * (default when phy_type is Full Speed) + * @oc_disable:Flag to disable overcurrent condition. + * 0 - Allow overcurrent condition to get detected + * 1 - Disable overcurrent condtion to get detected * @ts_dline: Enable Term Select Dline pulsing * 0 - No (default) * 1 - Yes @@ -492,6 +495,7 @@ struct dwc2_core_params { bool dma_desc_fs_enable; bool host_support_fs_ls_low_power; bool host_ls_low_power_phy_clk; + bool oc_disable; u8 host_channels; u16 host_rx_fifo_size; diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index c263114..5e20336 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -213,6 +213,11 @@ static int dwc2_hs_phy_init(struct dwc2_hsotg *hsotg, bool select_phy) usbcfg &= ~(GUSBCFG_PHYIF16 | GUSBCFG_DDRSEL); if (hsotg->params.phy_ulpi_ddr) usbcfg |= GUSBCFG_DDRSEL; + + /* Set external VBUS indicator as needed. */ + if (hsotg->params.oc_disable) + usbcfg |= (GUSBCFG_ULPI_INT_VBUS_IND | + GUSBCFG_INDICATORPASSTHROUGH); break; case DWC2_PHY_TYPE_PARAM_UTMI: /* UTMI+ interface */ diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c index a3ffe97..39e02cd 100644 --- a/drivers/usb/dwc2/params.c +++ b/drivers/usb/dwc2/params.c @@ -335,6 +335,9 @@ static void dwc2_get_device_properties(struct dwc2_hsotg *hsotg) num); } } + + if (of_find_property(hsotg->dev->of_node, "disable-over-current", NULL)) + p->oc_disable = true; } static void dwc2_check_param_otg_cap(struct dwc2_hsotg *hsotg) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc2: Add reset control to dwc2
On 08/09/2016 02:05 PM, dingu...@opensource.altera.com wrote: > From: Dinh Nguyen <dingu...@opensource.altera.com> > > Allow for platforms that have a reset controller driver in place to bring > the USB IP out of reset. > > Signed-off-by: Dinh Nguyen <dingu...@opensource.altera.com> > Acked-by: John Youn <johny...@synopsys.com> > Tested-by: Stefan Wahren <stefan.wah...@i2se.com> > --- > Hi Felipe, > > Can you please take this patch through your USB tree? This patch was dependent > on "168d7c4e8bb2 reset: Return -ENOTSUPP when not configured" and is now in > v3.8-rc1. Sorry, but I mean v4.8-rc1. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: dwc2: Add reset control to dwc2
From: Dinh Nguyen <dingu...@opensource.altera.com> Allow for platforms that have a reset controller driver in place to bring the USB IP out of reset. Signed-off-by: Dinh Nguyen <dingu...@opensource.altera.com> Acked-by: John Youn <johny...@synopsys.com> Tested-by: Stefan Wahren <stefan.wah...@i2se.com> --- Hi Felipe, Can you please take this patch through your USB tree? This patch was dependent on "168d7c4e8bb2 reset: Return -ENOTSUPP when not configured" and is now in v3.8-rc1. The discussion is highlight here: https://lkml.org/lkml/2016/7/7/291 Thanks, Dinh --- v7: Use devm_reset_control_get_optional() v6: fix 80 line checkpatch warning in dev_err print v5: updated error conditions for not finding the reset property v4: use dev_dbg() if not a -EPROBE_DEFER v3: fix compile error v2: move to lowlevel_hw_init() --- drivers/usb/dwc2/core.h | 1 + drivers/usb/dwc2/platform.c | 22 ++ 2 files changed, 23 insertions(+) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 9fae029..d645512 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -868,6 +868,7 @@ struct dwc2_hsotg { void *priv; int irq; struct clk *clk; + struct reset_control *reset; 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 fc6f525..530959a 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -45,6 +45,7 @@ #include #include #include +#include #include @@ -337,6 +338,24 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg) { int i, ret; + 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; + } + } + + if (hsotg->reset) + reset_control_deassert(hsotg->reset); + /* Set default UTMI width */ hsotg->phyif = GUSBCFG_PHYIF16; @@ -434,6 +453,9 @@ 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); + return 0; } -- 2.8.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] usb: dwc2: Add reset control to dwc2
On 06/21/2016 02:15 PM, John Youn wrote: > > Can you resend this to Philipp Zabel. > > Hi Philipp, > > Could you take this in your reset/next tree? It depends on the > following patch there: > > http://marc.info/?l=linux-usb=146473891018262=2 > I've resent that patch to Philipp. Thanks, Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Documentation: dt-bindings: dwc2: add the resets and reset-names property
Hi Rob, On 04/14/2016 11:40 AM, Rob Herring wrote: > On Wed, Apr 13, 2016 at 05:44:01PM -0500, dingu...@opensource.altera.com > wrote: >> From: Dinh Nguyen <dingu...@opensource.altera.com> >> >> Document the optional 'resets' and 'reset-names' property for the DWC2 usb >> core. >> >> Signed-off-by: Dinh Nguyen <dingu...@opensource.altera.com> >> --- >> Cc: Rob Herring <robh...@kernel.org> >> Cc: Pawel Moll <pawel.m...@arm.com> >> Cc: Mark Rutland <mark.rutl...@arm.com> >> Cc: Ian Campbell <ijc+devicet...@hellion.org.uk> >> Cc: Kumar Gala <ga...@codeaurora.org> >> --- >> Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++ >> 1 file changed, 3 insertions(+) > > Acked-by: Rob Herring <r...@kernel.org> > Can you please apply this to your tree? Thanks, Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 2/2] usb: dwc2: Add reset control to dwc2
On 04/14/2016 01:23 PM, John Youn wrote: > On 4/13/2016 7:04 PM, Arnd Bergmann wrote: >> On Thursday 14 April 2016, dingu...@opensource.altera.com wrote: >>> @@ -337,6 +338,17 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg >>> *hsotg) >>> { >>> int i, ret; >>> >>> + hsotg->reset = devm_reset_control_get(hsotg->dev, "dwc2"); >>> + if (IS_ERR(hsotg->reset)) { >>> + if (PTR_ERR(hsotg->reset) == -EPROBE_DEFER) >>> + return -EPROBE_DEFER; >>> + dev_dbg(hsotg->dev, "Could not get reset control.\n"); >>> + hsotg->reset = NULL; >>> + } >>> + >>> + if(hsotg->reset) >>> + reset_control_deassert(hsotg->reset); >> >> >> The error handling seems a bit odd here. If there is a failure to get the >> reset control and it's actually needed, I would argue the init function >> should >> not continue. Conversely, if there was no reset line specified in the >> device, why even print a message about it? > > Yes it's optional. I think this needs to be checked like the PHY's are > checked in the same function. That is, by checking for specific error > codes that indicate a reset control is not specified and continue only > in that case. All other errors should be returned. > > Dinh, care to make these changes? Sure, I can make the change. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] usb: dwc2: Add reset control to dwc2
On 04/13/2016 11:23 AM, Stefan Wahren wrote: >> +hsotg->reset = devm_reset_control_get(hsotg->dev, "dwc2"); > > shouldn't this be documented in > Documentation/devicetree/bindings/usb/dwc2.txt ? > Sure, I can document it. >> +if (IS_ERR(hsotg->reset)) { >> +dev_info(hsotg->dev, "Could not get reset control!\n"); >> +if (PTR_ERR(hsotg->reset) == -EPROBE_DEFER) >> +return -EPROBE_DEFER; > > If this log message is really necessary, please move it down here and add the > error code into the message. > Will do. Thanks for the review. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: dwc2: Add reset control to dwc2
On Thu, 7 Apr 2016, John Youn wrote: > Can you move into the lowlevel_hw_init()? The probe is a bit bloated > and we have these existing "lowlevel" functions where we consolidate > stuff like this. Same with the assert/deassert if possible. Yes, I can do that. Thanks for the review. BR, Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb: dwc2: Kernel 4.5 and socfpga problem
On 03/17/2016 09:35 PM, Phil Reid wrote: > G'day, > > Has anyone successful run the usb dwc2 from kenerl 4.5 on an socfpga. > Yes. I just tested on the devkit and USB is working fine: [0.655761] ffb4.usb supply vusb_d not found, using dummy regulator [0.662384] ffb4.usb supply vusb_a not found, using dummy regulator [0.943627] dwc2 ffb4.usb: EPs: 16, dedicated fifos, 8064 entries in SPRAM [0.951145] dwc2 ffb4.usb: DWC OTG Controller [0.955872] dwc2 ffb4.usb: new USB bus registered, assigned bus number 1 [0.962908] dwc2 ffb4.usb: irq 37, io mem 0x [0.968877] hub 1-0:1.0: USB hub found [0.972641] hub 1-0:1.0: 1 port detected [...] root@socfpga_cyclone5:~# uname -a Linux socfpga_cyclone5 4.5.0 #1 SMP Fri Mar 18 09:36:26 CDT 2016 armv7l GNU/Linux root@socfpga_cyclone5:~# [ 21.793687] usb 1-1: USB disconnect, device number 2 [ 24.173612] usb 1-1: new high-speed USB device number 3 using dwc2 > Inital I've had to remove the phys & phy-names property from the DT for > it to even probe. > Otherwise it was returning PROBE_DEFERED. > The 4.4 driver seems to be getting the same error but it continued > loading regardless. > What hardware are you using? Sockit, Atlas board, socrates, etc? > After fixing that and getting it to load its going into overcurrent fault. > Applying Dinh's patch from the Altera 4.4 tree > FogBugz #198256: Fix unnecessary USB overcurrent condition > hasn't help. The driver immediately goes into an overcurrent condition > at boot. I'm still trying to find a way to upstream this patch. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/32] usb: dwc2: various bug fixes
On 09/22/2015 08:16 AM, Mian Yousaf Kaukab wrote: > Hi, > This series consists of various bug fixes for both host and gadget > sides. All patches are verified on dwc2 v3.0a with dedicated fifos. > It would be good to get some Tested-bys for other platforms. > > It is based on testing/next branch in Felipe's git. > > Thank you, > I tested this series on 2.93a and 3.20a versions of the IP and did not see any issues. Tested-by: Dinh Nguyen <dingu...@opensource.altera.com> Thanks, Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] usb: second series of updates for dwc2 driver
On 01/21/2015 02:54 AM, Kaukab, Yousaf wrote: Hi John, -Original Message- From: Kaukab, Yousaf Sent: Thursday, January 15, 2015 6:09 PM To: linux-usb@vger.kernel.org; ba...@ti.com Cc: john.y...@synopsys.com; Herrero, Gregory; pa...@synopsys.com; r.bald...@samsung.com; dingu...@opensource.altera.com; Kaukab, Yousaf Subject: [PATCH 00/13] usb: second series of updates for dwc2 driver Hi, This patchset consists of some bug fixes, feature enhancements and cosmetic changes for the dwc2 driver. All the patches are verified on dwc2 v3.0a with dedicated fifos. Main focus of testing was with dma enabled. Although basic testing without dma was also done. This is based on testing/next branch in Felipe's git. Thank you, Best regards, Yousaf Gregory Herrero (8): usb: dwc2: host: register hcd handle to the phy usb: dwc2: host: resume root hub on remote wakeup usb: dwc2: gadget: fix clear halt feature handling usb: dwc2: gadget: add TEST_MODE feature support usb: dwc2: gadget: fix a typo in comment usb: dwc2: gadget: add reset flag in init function usb: dwc2: gadget: don't modify pullup status during reset usb: dwc2: gadget: initialize controller in pullup callback Mian Yousaf Kaukab (5): usb: dwc2: gadget: remove hardcoded if (0) and if (1) checks usb: dwc2: gadget: add unaligned buffers support usb: dwc2: gadget: fix debug message for zlp usb: dwc2: gadget: fix phy interface configuration usb: dwc2: gadget: replace constants with defines drivers/usb/dwc2/core.h | 12 +- drivers/usb/dwc2/gadget.c | 358 ++ drivers/usb/dwc2/hcd.c| 43 -- drivers/usb/dwc2/hw.h | 1 + 4 files changed, 348 insertions(+), 66 deletions(-) Did you get a chance to review this patchset? Can you please ACK it if you don't have any objections? BR, Yousaf I have not had a chance to test this series on my v2.93a hardware, and most likely won't be able to get to it until early next week. Just a note in case you wanted additional testing. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/30] usb: updates for dwc2 gadget driver
On 01/07/2015 09:34 AM, Kaukab, Yousaf wrote: Hi, -Original Message- From: Dinh Nguyen [mailto:dingu...@opensource.altera.com] Sent: Wednesday, January 7, 2015 1:01 AM To: Paul Zimmerman; Kaukab, Yousaf; linux-usb@vger.kernel.org; ba...@ti.com Cc: Herrero, Gregory; sergei.shtyl...@cogentembedded.com; r.bald...@samsung.com Subject: Re: [PATCH v2 00/30] usb: updates for dwc2 gadget driver On 01/06/2015 05:47 PM, Dinh Nguyen wrote: On 01/05/2015 01:16 PM, Paul Zimmerman wrote: Adding Dinh to CC. Robert, Dinh, I would like to have your tested-bys for this series, please. This patch series is producing this error on the SOCFPGA platform. I'll try to bisect to a specific patch. root@socfpga_cyclone5:~# [ 47.929743] dwc2 ffb4.usb: new device is low-speed [ 63.873596] dwc2 ffb4.usb: new device is low-speed [ 64.219220] dwc2 ffb4.usb: new device is low-speed [ 64.421495] dwc2 ffb4.usb: new device is low-speed [ 64.834505] dwc2 ffb4.usb: new device is high-speed [ 69.899695] [ cut here ] [ 69.904315] WARNING: CPU: 0 PID: 0 at drivers/usb/dwc2/gadget.c:1352 s3c_hsotg_rx_data+0xc0/0xd4() [ 69.913230] Modules linked in: g_mass_storage usb_f_mass_storage libcomposite [ 69.920385] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.19.0-rc1-00090-g4864f14 #4 [ 69.927917] Hardware name: Altera SOCFPGA [ 69.931936] [c0014d24] (unwind_backtrace) from [c001173c] (show_stack+0x10/0x14) [ 69.939654] [c001173c] (show_stack) from [c0461c74] (dump_stack+0x74/0x90) [ 69.946857] [c0461c74] (dump_stack) from [c0021138] (warn_slowpath_common+0x78/0xb4) [ 69.954916] [c0021138] (warn_slowpath_common) from [c0021190] (warn_slowpath_null+0x1c/0x24) [ 69.963669] [c0021190] (warn_slowpath_null) from [c030278c] (s3c_hsotg_rx_data+0xc0/0xd4) [ 69.972162] [c030278c] (s3c_hsotg_rx_data) from [c0304ac0] (s3c_hsotg_irq+0x454/0x520) [ 69.980394] [c0304ac0] (s3c_hsotg_irq) from [c0058c3c] (handle_irq_event_percpu+0x5c/0x1f4) [ 69.989056] [c0058c3c] (handle_irq_event_percpu) from [c0058e18] (handle_irq_event+0x44/0x64) [ 69.997892] [c0058e18] (handle_irq_event) from [c005b8d8] (handle_fasteoi_irq+0xa8/0x180) [ 70.006382] [c005b8d8] (handle_fasteoi_irq) from [c005856c] (generic_handle_irq+0x20/0x30) [ 70.014958] [c005856c] (generic_handle_irq) from [c0058668] (__handle_domain_irq+0x54/0xb4) [ 70.023619] [c0058668] (__handle_domain_irq) from [c0008638] (gic_handle_irq+0x20/0x5c) [ 70.031937] [c0008638] (gic_handle_irq) from [c0012240] (__irq_svc+0x40/0x54) [ 70.039384] Exception stack(0xc0635f60 to 0xc0635fa8) [ 70.044415] 5f60: c0635fb0 c001cee0 c066cca0 c063c498 c046908c c066c4b2 [ 70.052556] 5f80: c066cca0 0001 ef7fccc0 0100 c0635fa8 c000f0fc c000f100 [ 70.060694] 5fa0: 600f0013 [ 70.064174] [c0012240] (__irq_svc) from [c000f100] (arch_cpu_idle+0x30/0x3c) [ 70.071545] [c000f100] (arch_cpu_idle) from [c0050364] (cpu_startup_entry+0x138/0x224) [ 70.079783] [c0050364] (cpu_startup_entry) from [c05ebbb0] (start_kernel+0x304/0x35c) [ 70.087921] ---[ end trace 66b76f6dcbbda3bf ]--- [ 79.911589] dwc2 ffb4.usb: new device is low-speed Dinh It looks like this patch is producing the above error. usb: dwc2: gadget: manage ep0 state in software Manage ep0 state in software to add handling of status OUT stage. Just toggling hsotg-setup in s3c_hsotg_handle_outdone leaves it in wrong state in 2-stage control transfers. Moreover, don't call s3c_hsotg_handle_outdone both from SetupDone and OutDone. Only use one of them, as for a setup packet we get both. Signed-off-by: Mian Yousaf Kaukab yousaf.kau...@intel.com Thank you for testing this patchset. I have tested g_mass_storage without dma in my setup and it is working fine. Did you also test with DMA enabled? As I don't see this problem, I need some help in figuring out what's going on. Would it be possible for you to provide me following? 1) Console output after enabling CONFIG_USB_DWC2_DEBUG 2) Function trace echo s3c_* /sys/kernel/debug/tracing/set_ftrace_filter echo function /sys/kernel/debug/tracing/current_tracer 3) Values of following registers GSNPSID, GHWCFG1-GHWCFG4 I'll try to get these out for your shortly. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/30] usb: updates for dwc2 gadget driver
Hi Yousaf, On 01/07/2015 09:47 AM, Kaukab, Yousaf wrote: -Original Message- From: Dinh Nguyen [mailto:dingu...@opensource.altera.com] Sent: Wednesday, January 7, 2015 4:39 PM To: Kaukab, Yousaf; Paul Zimmerman; linux-usb@vger.kernel.org; ba...@ti.com Cc: Herrero, Gregory; sergei.shtyl...@cogentembedded.com; r.bald...@samsung.com Subject: Re: [PATCH v2 00/30] usb: updates for dwc2 gadget driver On 01/07/2015 09:34 AM, Kaukab, Yousaf wrote: Hi, -Original Message- From: Dinh Nguyen [mailto:dingu...@opensource.altera.com] Sent: Wednesday, January 7, 2015 1:01 AM To: Paul Zimmerman; Kaukab, Yousaf; linux-usb@vger.kernel.org; ba...@ti.com Cc: Herrero, Gregory; sergei.shtyl...@cogentembedded.com; r.bald...@samsung.com Subject: Re: [PATCH v2 00/30] usb: updates for dwc2 gadget driver On 01/06/2015 05:47 PM, Dinh Nguyen wrote: On 01/05/2015 01:16 PM, Paul Zimmerman wrote: Adding Dinh to CC. Robert, Dinh, I would like to have your tested-bys for this series, please. This patch series is producing this error on the SOCFPGA platform. I'll try to bisect to a specific patch. root@socfpga_cyclone5:~# [ 47.929743] dwc2 ffb4.usb: new device is low-speed [ 63.873596] dwc2 ffb4.usb: new device is low-speed [ 64.219220] dwc2 ffb4.usb: new device is low-speed [ 64.421495] dwc2 ffb4.usb: new device is low-speed [ 64.834505] dwc2 ffb4.usb: new device is high-speed [ 69.899695] [ cut here ] [ 69.904315] WARNING: CPU: 0 PID: 0 at drivers/usb/dwc2/gadget.c:1352 s3c_hsotg_rx_data+0xc0/0xd4() [ 69.913230] Modules linked in: g_mass_storage usb_f_mass_storage libcomposite [ 69.920385] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.19.0-rc1-00090-g4864f14 #4 [ 69.927917] Hardware name: Altera SOCFPGA [ 69.931936] [c0014d24] (unwind_backtrace) from [c001173c] (show_stack+0x10/0x14) [ 69.939654] [c001173c] (show_stack) from [c0461c74] (dump_stack+0x74/0x90) [ 69.946857] [c0461c74] (dump_stack) from [c0021138] (warn_slowpath_common+0x78/0xb4) [ 69.954916] [c0021138] (warn_slowpath_common) from [c0021190] (warn_slowpath_null+0x1c/0x24) [ 69.963669] [c0021190] (warn_slowpath_null) from [c030278c] (s3c_hsotg_rx_data+0xc0/0xd4) [ 69.972162] [c030278c] (s3c_hsotg_rx_data) from [c0304ac0] (s3c_hsotg_irq+0x454/0x520) [ 69.980394] [c0304ac0] (s3c_hsotg_irq) from [c0058c3c] (handle_irq_event_percpu+0x5c/0x1f4) [ 69.989056] [c0058c3c] (handle_irq_event_percpu) from [c0058e18] (handle_irq_event+0x44/0x64) [ 69.997892] [c0058e18] (handle_irq_event) from [c005b8d8] (handle_fasteoi_irq+0xa8/0x180) [ 70.006382] [c005b8d8] (handle_fasteoi_irq) from [c005856c] (generic_handle_irq+0x20/0x30) [ 70.014958] [c005856c] (generic_handle_irq) from [c0058668] (__handle_domain_irq+0x54/0xb4) [ 70.023619] [c0058668] (__handle_domain_irq) from [c0008638] (gic_handle_irq+0x20/0x5c) [ 70.031937] [c0008638] (gic_handle_irq) from [c0012240] (__irq_svc+0x40/0x54) [ 70.039384] Exception stack(0xc0635f60 to 0xc0635fa8) [ 70.044415] 5f60: c0635fb0 c001cee0 c066cca0 c063c498 c046908c c066c4b2 [ 70.052556] 5f80: c066cca0 0001 ef7fccc0 0100 c0635fa8 c000f0fc c000f100 [ 70.060694] 5fa0: 600f0013 [ 70.064174] [c0012240] (__irq_svc) from [c000f100] (arch_cpu_idle+0x30/0x3c) [ 70.071545] [c000f100] (arch_cpu_idle) from [c0050364] (cpu_startup_entry+0x138/0x224) [ 70.079783] [c0050364] (cpu_startup_entry) from [c05ebbb0] (start_kernel+0x304/0x35c) [ 70.087921] ---[ end trace 66b76f6dcbbda3bf ]--- [ 79.911589] dwc2 ffb4.usb: new device is low-speed Dinh It looks like this patch is producing the above error. usb: dwc2: gadget: manage ep0 state in software Manage ep0 state in software to add handling of status OUT stage. Just toggling hsotg-setup in s3c_hsotg_handle_outdone leaves it in wrong state in 2-stage control transfers. Moreover, don't call s3c_hsotg_handle_outdone both from SetupDone and OutDone. Only use one of them, as for a setup packet we get both. Signed-off-by: Mian Yousaf Kaukab yousaf.kau...@intel.com Thank you for testing this patchset. I have tested g_mass_storage without dma in my setup and it is working fine. Did you also test with DMA enabled? Yes, it's working as well. No issue. s3c_hsotg_rx_data() should not be called when DMA enabled. That's why I only mentioned without-dma case. As I don't see this problem, I need some help in figuring out what's going on. Would it be possible for you to provide me following? 1) Console output after enabling CONFIG_USB_DWC2_DEBUG 2) Function trace echo s3c_* /sys/kernel/debug/tracing/set_ftrace_filter echo function /sys/kernel/debug/tracing/current_tracer 3) Values of following registers GSNPSID, GHWCFG1-GHWCFG4 I'll try to get these out for your shortly. Thanks! I've also attached a text file
Re: [PATCH v3] usb: dwc2: add bus suspend/resume for dwc2
On 01/05/2015 09:02 PM, Paul Zimmerman wrote: From: Kever Yang [mailto:kever.y...@rock-chips.com] Sent: Monday, January 05, 2015 5:42 PM Hi Paul, I think you need this patch to fix the problem: usb: dwc2: resume root hub when device detect with suspend state https://patchwork.kernel.org/patch/5325111/ This patch may have fixed hotplug when a device is connected directly to the USB port, but it does not seem to fix a hotplug when a device is connected to a hub. I booted the board up with a device connected to a 4-port hub, the device is detected, unplug the device from a hub, wait 5-sec, re-plug the device into hub. The device is not detected. Thanks, - Kever On 01/06/2015 09:23 AM, Paul Zimmerman wrote: From: Kever Yang [mailto:kever.y...@rock-chips.com] Sent: Wednesday, November 12, 2014 4:42 PM On 11/13/2014 07:22 AM, Doug Anderson wrote: Kever, On Mon, Nov 10, 2014 at 5:09 AM, Kever Yang kever.y...@rock-chips.com wrote: Hcd controller needs bus_suspend/resume, dwc2 controller make root hub generate suspend/resume signal with hprt0 register when work in host mode. After the root hub enter suspend, we can make controller enter low power state with PCGCTL register. We also update the lx_state for hsotg state. This patch has tested on rk3288 with suspend/resume. Signed-off-by: Kever Yang kever.y...@rock-chips.com Acked-by: Paul Zimmerman pa...@synopsys.com --- Changes in v3: - remove CONFIG_PM macro for bus_suspend/resume - add PCGCTL operation for no device connect case Changes in v2: - update commit message - make dwc2 suspend/resume sourcecode work drivers/usb/dwc2/hcd.c | 88 +++--- 1 file changed, 77 insertions(+), 11 deletions(-) I would certainly appreciate confirmation, but my inclination is to NAK this change due to the fact that it regresses functionality. I haven't done any serious review of it, but I've been testing it and it appears to break hotplug. Said another way, I did this: 1. Without this patch, I booted with a USB stick in. It was detected. I unplugged it, waited 5 seconds, and then plugged it back in. The USB stick was redetcted. 2. With this patch, I did the same thing. The USB not redected after plugging it back in. With this patch, the dwc2 hcd/root hub will be auto suspend after device on port is disconnected, and it can't detect the device connect any more, I think that's the problem. I will figure out how to make dwc2 detect the device connect after auto suspend, or disable the auto suspend feature for the dwc2 hcd. Kever, This patch has made it into Linus' kernel as commit 0cf884e819e0, and it breaks disconnect/connect on at least the Altera SOCFPGA platform. I haven't been able to test it on any other platforms. You need to submit a patch to either fix this, or to only enable this feature for the Rock-chip platform. Otherwise the patch has to be reverted. Unfortunately, after applying that patch there is another problem. If I connect a device that causes an overcurrent condition to the root port, then the port is dead after that. No further devices are detected until after I reboot. I tried adding another call to usb_hcd_resume_root_hub() in the if (hprt0 HPRT0_OVRCURRCHG) branch, but then an overcurrent condition causes a continuous stream of these messages: [ 133.348776] dwc2 ffb4.usb: GetPortStatus wIndex=0x0001 flags=0x0022 [ 133.355717] dwc2 ffb4.usb: Overcurrent change detected [ 133.361179] dwc2 ffb4.usb: HPRT0: 0x0002 [ 133.365960] dwc2 ffb4.usb: port_status=0008 [ 133.373236] hub 1-0:1.0: state 7 ports 1 chg evt [ 133.380038] hub 1-0:1.0: hub_suspend [ 133.384237] usb usb1: bus auto-suspend, wakeup 1 [ 133.393631] dwc2 ffb4.usb: DWC OTG HCD HUB STATUS DATA: Root port status changed [ 133.401341] dwc2 ffb4.usb: port_connect_status_change: 0 [ 133.407157] dwc2 ffb4.usb: port_reset_change: 0 [ 133.412186] dwc2 ffb4.usb: port_enable_change: 0 [ 133.417310] dwc2 ffb4.usb: port_suspend_change: 0 [ 133.422519] dwc2 ffb4.usb: port_over_current_change: 1 [ 133.428154] usb usb1: suspend raced with wakeup event [ 133.433191] usb usb1: usb auto-resume [ 133.441522] hub 1-0:1.0: hub_resume [ 133.455505] dwc2 ffb4.usb: GetPortStatus wIndex=0x0001 flags=0x0022 [ 133.462443] dwc2 ffb4.usb: Overcurrent change detected [ 133.467907] dwc2 ffb4.usb: HPRT0: 0x0002 [ 133.472684] dwc2 ffb4.usb: port_status=0008 [ 133.480088] hub 1-0:1.0: state 7 ports 1 chg evt [ 133.485578] hub 1-0:1.0: hub_suspend [ 133.489157] usb usb1: bus auto-suspend, wakeup 1 [ 133.493768] dwc2 ffb4.usb: _dwc2_hcd_suspend() [ 133.498540] dwc2 ffb4.usb: DWC OTG HCD HUB STATUS DATA: Root port status changed [ 133.506257] dwc2 ffb4.usb: port_connect_status_change: 0 [ 133.512065] dwc2
Re: [PATCH v2 00/30] usb: updates for dwc2 gadget driver
On 01/05/2015 01:16 PM, Paul Zimmerman wrote: Adding Dinh to CC. Robert, Dinh, I would like to have your tested-bys for this series, please. This patch series is producing this error on the SOCFPGA platform. I'll try to bisect to a specific patch. root@socfpga_cyclone5:~# [ 47.929743] dwc2 ffb4.usb: new device is low-speed [ 63.873596] dwc2 ffb4.usb: new device is low-speed [ 64.219220] dwc2 ffb4.usb: new device is low-speed [ 64.421495] dwc2 ffb4.usb: new device is low-speed [ 64.834505] dwc2 ffb4.usb: new device is high-speed [ 69.899695] [ cut here ] [ 69.904315] WARNING: CPU: 0 PID: 0 at drivers/usb/dwc2/gadget.c:1352 s3c_hsotg_rx_data+0xc0/0xd4() [ 69.913230] Modules linked in: g_mass_storage usb_f_mass_storage libcomposite [ 69.920385] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.19.0-rc1-00090-g4864f14 #4 [ 69.927917] Hardware name: Altera SOCFPGA [ 69.931936] [c0014d24] (unwind_backtrace) from [c001173c] (show_stack+0x10/0x14) [ 69.939654] [c001173c] (show_stack) from [c0461c74] (dump_stack+0x74/0x90) [ 69.946857] [c0461c74] (dump_stack) from [c0021138] (warn_slowpath_common+0x78/0xb4) [ 69.954916] [c0021138] (warn_slowpath_common) from [c0021190] (warn_slowpath_null+0x1c/0x24) [ 69.963669] [c0021190] (warn_slowpath_null) from [c030278c] (s3c_hsotg_rx_data+0xc0/0xd4) [ 69.972162] [c030278c] (s3c_hsotg_rx_data) from [c0304ac0] (s3c_hsotg_irq+0x454/0x520) [ 69.980394] [c0304ac0] (s3c_hsotg_irq) from [c0058c3c] (handle_irq_event_percpu+0x5c/0x1f4) [ 69.989056] [c0058c3c] (handle_irq_event_percpu) from [c0058e18] (handle_irq_event+0x44/0x64) [ 69.997892] [c0058e18] (handle_irq_event) from [c005b8d8] (handle_fasteoi_irq+0xa8/0x180) [ 70.006382] [c005b8d8] (handle_fasteoi_irq) from [c005856c] (generic_handle_irq+0x20/0x30) [ 70.014958] [c005856c] (generic_handle_irq) from [c0058668] (__handle_domain_irq+0x54/0xb4) [ 70.023619] [c0058668] (__handle_domain_irq) from [c0008638] (gic_handle_irq+0x20/0x5c) [ 70.031937] [c0008638] (gic_handle_irq) from [c0012240] (__irq_svc+0x40/0x54) [ 70.039384] Exception stack(0xc0635f60 to 0xc0635fa8) [ 70.044415] 5f60: c0635fb0 c001cee0 c066cca0 c063c498 c046908c c066c4b2 [ 70.052556] 5f80: c066cca0 0001 ef7fccc0 0100 c0635fa8 c000f0fc c000f100 [ 70.060694] 5fa0: 600f0013 [ 70.064174] [c0012240] (__irq_svc) from [c000f100] (arch_cpu_idle+0x30/0x3c) [ 70.071545] [c000f100] (arch_cpu_idle) from [c0050364] (cpu_startup_entry+0x138/0x224) [ 70.079783] [c0050364] (cpu_startup_entry) from [c05ebbb0] (start_kernel+0x304/0x35c) [ 70.087921] ---[ end trace 66b76f6dcbbda3bf ]--- [ 79.911589] dwc2 ffb4.usb: new device is low-speed Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/30] usb: updates for dwc2 gadget driver
On 01/06/2015 05:47 PM, Dinh Nguyen wrote: On 01/05/2015 01:16 PM, Paul Zimmerman wrote: Adding Dinh to CC. Robert, Dinh, I would like to have your tested-bys for this series, please. This patch series is producing this error on the SOCFPGA platform. I'll try to bisect to a specific patch. root@socfpga_cyclone5:~# [ 47.929743] dwc2 ffb4.usb: new device is low-speed [ 63.873596] dwc2 ffb4.usb: new device is low-speed [ 64.219220] dwc2 ffb4.usb: new device is low-speed [ 64.421495] dwc2 ffb4.usb: new device is low-speed [ 64.834505] dwc2 ffb4.usb: new device is high-speed [ 69.899695] [ cut here ] [ 69.904315] WARNING: CPU: 0 PID: 0 at drivers/usb/dwc2/gadget.c:1352 s3c_hsotg_rx_data+0xc0/0xd4() [ 69.913230] Modules linked in: g_mass_storage usb_f_mass_storage libcomposite [ 69.920385] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.19.0-rc1-00090-g4864f14 #4 [ 69.927917] Hardware name: Altera SOCFPGA [ 69.931936] [c0014d24] (unwind_backtrace) from [c001173c] (show_stack+0x10/0x14) [ 69.939654] [c001173c] (show_stack) from [c0461c74] (dump_stack+0x74/0x90) [ 69.946857] [c0461c74] (dump_stack) from [c0021138] (warn_slowpath_common+0x78/0xb4) [ 69.954916] [c0021138] (warn_slowpath_common) from [c0021190] (warn_slowpath_null+0x1c/0x24) [ 69.963669] [c0021190] (warn_slowpath_null) from [c030278c] (s3c_hsotg_rx_data+0xc0/0xd4) [ 69.972162] [c030278c] (s3c_hsotg_rx_data) from [c0304ac0] (s3c_hsotg_irq+0x454/0x520) [ 69.980394] [c0304ac0] (s3c_hsotg_irq) from [c0058c3c] (handle_irq_event_percpu+0x5c/0x1f4) [ 69.989056] [c0058c3c] (handle_irq_event_percpu) from [c0058e18] (handle_irq_event+0x44/0x64) [ 69.997892] [c0058e18] (handle_irq_event) from [c005b8d8] (handle_fasteoi_irq+0xa8/0x180) [ 70.006382] [c005b8d8] (handle_fasteoi_irq) from [c005856c] (generic_handle_irq+0x20/0x30) [ 70.014958] [c005856c] (generic_handle_irq) from [c0058668] (__handle_domain_irq+0x54/0xb4) [ 70.023619] [c0058668] (__handle_domain_irq) from [c0008638] (gic_handle_irq+0x20/0x5c) [ 70.031937] [c0008638] (gic_handle_irq) from [c0012240] (__irq_svc+0x40/0x54) [ 70.039384] Exception stack(0xc0635f60 to 0xc0635fa8) [ 70.044415] 5f60: c0635fb0 c001cee0 c066cca0 c063c498 c046908c c066c4b2 [ 70.052556] 5f80: c066cca0 0001 ef7fccc0 0100 c0635fa8 c000f0fc c000f100 [ 70.060694] 5fa0: 600f0013 [ 70.064174] [c0012240] (__irq_svc) from [c000f100] (arch_cpu_idle+0x30/0x3c) [ 70.071545] [c000f100] (arch_cpu_idle) from [c0050364] (cpu_startup_entry+0x138/0x224) [ 70.079783] [c0050364] (cpu_startup_entry) from [c05ebbb0] (start_kernel+0x304/0x35c) [ 70.087921] ---[ end trace 66b76f6dcbbda3bf ]--- [ 79.911589] dwc2 ffb4.usb: new device is low-speed Dinh It looks like this patch is producing the above error. usb: dwc2: gadget: manage ep0 state in software Manage ep0 state in software to add handling of status OUT stage. Just toggling hsotg-setup in s3c_hsotg_handle_outdone leaves it in wrong state in 2-stage control transfers. Moreover, don't call s3c_hsotg_handle_outdone both from SetupDone and OutDone. Only use one of them, as for a setup packet we get both. Signed-off-by: Mian Yousaf Kaukab yousaf.kau...@intel.com Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 00/30] usb: updates for dwc2 gadget driver
On Mon, 5 Jan 2015, Paul Zimmerman wrote: Adding Dinh to CC. Robert, Dinh, I would like to have your tested-bys for this series, please. I'll try to test this by the end of this week. Dinh From: Mian Yousaf Kaukab [mailto:yousaf.kau...@intel.com] Sent: Friday, January 02, 2015 6:42 AM Hi, This patchset consists of various bug fixes and feature enhancements for the dwc2 gadget driver. All the patches are verified on dwc2 v3.0a with dedicated fifos. Main focus of testing was with dma enabled. Although basic testing without dma was also done. It is based on testing/next branch in Felipe's git with https://lkml.org/lkml/2014/12/16/135 applied on top. Thank you, Best regards, Yousaf History: v2: - Rebased to Felipe's testing/next with https://lkml.org/lkml/2014/12/16/135 applied on top. - Fixed comments from Robert Baldyga - Some cosmetic changes - Replaced usb: dwc2: gadget: process setup packet on transfer complete with usb: dwc2: gadget: don't process XferCompl on setup packet - Updated usb: dwc2: gadget: provide gadget handle to the phy so that otg_set_peripheral is called in both udc_start and udc_stop. v1: - Addressed comments from Sergei Shtylyov Gregory Herrero (13): usb: dwc2: gadget: register gadget handle to the phy usb: dwc2: gadget: write correct value in ahbcfg register usb: dwc2: gadget: don't erase gahbcfg register when enabling dma usb: dwc2: gadget: add device tree property to enable dma Documentation: dt-bindings: add dt binding info for dwc2 g-use-dma usb: dwc2: gadget: configure fifos from device tree Documentation: dt-bindings: add dt binding info for dwc2 fifo resizing usb: dwc2: gadget: don't block after fifo flush timeout usb: dwc2: gadget: add vbus_session support usb: dwc2: gadget: reset fifo_map when initializing fifos usb: dwc2: gadget: fix pullup handling usb: dwc2: gadget: add vbus_draw support usb: dwc2: gadget: force gadget initialization in dev mode Mian Yousaf Kaukab (17): usb: dwc2: gadget: mask fifo empty irq with dma usb: dwc2: gadget: don't process XferCompl on setup packet usb: dwc2: gadget: don't embed ep0 buffers usb: dwc2: gadget: fix error path in dwc2_gadget_init usb: dwc2: gadget: add bi-directional endpoint support usb: dwc2: gadget: check interrupts for all endpoints usb: dwc2: gadget: remove unused members from hsotg_req usb: dwc2: gadget: fix debug loop limits usb: dwc2: gadget: consider all tx fifos usb: dwc2: gadget: kill requests after disabling ep usb: dwc2: gadget: manage ep0 state in software usb: dwc2: gadget: fix zero length packet transfers usb: dwc2: gadget: dont warn if endpoint is not enabled usb: dwc2: gadget: rename sent_zlp to send_zlp usb: dwc2: gadget: pick smallest acceptable fifo usb: dwc2: gadget: fix fifo allocation leak usb: dwc2: gadget: report disconnection after reset Documentation/devicetree/bindings/usb/dwc2.txt | 4 + drivers/usb/dwc2/core.h| 46 +- drivers/usb/dwc2/gadget.c | 786 - drivers/usb/dwc2/hw.h | 1 + 4 files changed, 550 insertions(+), 287 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html BR, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb: dwc2: gadget: Do not fail probe if there isn't a clock node
+CC: Felipe Balbi On 11/24/2014 04:48 AM, Dan Carpenter wrote: On Mon, Nov 24, 2014 at 01:46:56PM +0300, Dan Carpenter wrote: Hello Dinh Nguyen, The patch 8d736d8a9c44: usb: dwc2: gadget: Do not fail probe if there isn't a clock node from Nov 11, 2014, leads to the following static checker warning: drivers/usb/dwc2/gadget.c:3436 dwc2_gadget_init() warn: passing zero to 'PTR_ERR' drivers/usb/dwc2/gadget.c 3432 hsotg-clk = devm_clk_get(dev, otg); 3433 if (IS_ERR(hsotg-clk)) { 3434 hsotg-clk = NULL; You need to preserve the error code. NULL means zero means success. Oh, wait. You are returning success deliberately? Just return 0; in that case instead of obfuscating it this way. But shouldn't we continue with the rest of the function anyway? This patch is confusing to me. Yes, I believe that we can remove the return, and that was Felipe's comment. 3435 dev_err(dev, cannot get otg clock\n); Do we need to print this error if it's a success path? A perhaps a follow up patch to something like this? --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3451,8 +3451,7 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) hsotg-clk = devm_clk_get(dev, otg); if (IS_ERR(hsotg-clk)) { hsotg-clk = NULL; - dev_err(dev, cannot get otg clock\n); - return PTR_ERR(hsotg-clk); + dev_dbg(dev, cannot get otg clock\n); } hsotg-gadget.max_speed = USB_SPEED_HIGH; @@ -3461,7 +3460,12 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) /* reset the system */ - clk_prepare_enable(hsotg-clk); + ret = clk_prepare_enable(hsotg-clk); + if (ret) { + dev_err(dev, failed to enable otg clk\n); + goto err_clk; + } + -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc2: allow dwc2 to get built when USB_GADGET=m
Hi Felipe, On 10/22/2014 01:29 PM, Paul Zimmerman wrote: From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com] Sent: Tuesday, October 21, 2014 1:32 PM From: Dinh Nguyen dingu...@opensource.altera.com This patch allows the gadget portion of the DWC2 driver to get built when (!USB USB_GADGET) condition is encountered. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com --- drivers/usb/dwc2/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index f93807b..4d02718 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -1,6 +1,6 @@ config USB_DWC2 bool DesignWare USB2 DRD Core Support -depends on USB +depends on USB || USB_GADGET help Say Y here if your system has a Dual Role Hi-Speed USB controller based on the DesignWare HSOTG IP Core. Acked-by: Paul Zimmerman pa...@synopsys.com Can you please apply this patch to your -next tree? Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 2/8] usb: dwc2: Move gadget probe function into platform code
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 10/30/2014 08:57 AM, Felipe Balbi wrote: On Tue, Oct 28, 2014 at 06:25:43PM -0500, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com This patch will aggregate the probing of gadget/hcd driver into platform.c. The gadget probe funtion is converted into gadget_init that is now only responsible for gadget only initialization. All the gadget resources is now s/resources is/resources are handled by platform.c Since the host workqueue will not get initialized if the driver is configured for peripheral mode only. Thus we need to check for wq_otg before calling queue_work(). this period character in the middle of the sentence doesn't make sense, perhaps a comma is what you want ? The sentence can be improved too: Since the host workqueue will not get initialized if the drier is configured for peripheral mode only, we add a check for wq_otg before calling queue_work(). Also, we move spin_lock_init to common location for both host and gadget that is either in platform.c or pci.c. We also ove suspend/resume code to common platform code, and update it to use the new PM API (struct dev_pm_ops). updating to dev_pm_ops should really be a separate patch. All fixed... Thanks, Dinh -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJUU6PAAAoJEBmUBAuBoyj0KJUP/RWpP9YkctpmZQgrb2xq0JaG eLCpOnKVYaTBCAhydJOKBBx0odYyxORlxTWbJCbZPvPhrZ7NDTINqEVWJOqYmtqe Xl8aEPYm+ckcfTliONK3yIP/MA49he/qbGOmqXFwFMbcAvKFN/kpq05cjzZ6T8bO hILDO2Yy2HVQHonq3uKppmTB9DSnCNfCR0Cuum2fpOzmVLY/X46EM5UD2e+XmVgo dsINAGe40FwFIoLZzAradn12MrmCjOM6MPpkkGZ3SMGjKXY7yQX38c9WbcCvsm6g y1/5fDqdRlsq+Weoo9N3H03LUX8MlGCpZ7dgLrABXMEjdySj3eVTj4bHgPxRjRY/ +SfI2YR8RyEcHj2UTqgMUp4JmRl2CNiB9fsvZHMQQm2MTeFkcUpZnSUXWHB9Vtwv s1I5nMOSoT2NDBg6QS9a1T1s1gdSywOnDBd+/SK7mf4QkQumvf2Nvu6avQB1Rxxm sqpGWztuhuycg332CXc9W0EBXcg5t8SRyk9SFgksH3MezS85gAQxZtXyv44NuCUM ciTru8tKh3ncszws2NmX9yyQgC4fM3kHDytOwDXFt23f3xHyjg+2m3O1TLErAwig yqj2MfYTfqfYVrs/hOvOpEbUgUSy5MQY/EOMIsI6aGfB2Nba72wrYQP79ugb84u9 Ftd1XH47lvXX/R8VOV2x =ZAkf -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/8] usb: dwc2: Update common interrupt handler to call gadget interrupt handler
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 10/30/2014 09:00 AM, Felipe Balbi wrote: Hi, On Tue, Oct 28, 2014 at 06:25:45PM -0500, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Make dwc2_handle_common_intr call the gadget interrupt function when operating in peripheral mode. Remove the spinlock functions in s3c_hsotg_irq as dwc2_handle_common_intr() already has the spinlocks. Move the registeration of the IRQ to common code for platform and PCI. Remove duplicate interrupt conditions that was in gadget, as those are handled by dwc2 common interrupt handler. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- v5: remove individual devm_request_irq from gadget and hcd, and place a single devm_request_irq in platform and pci. v2: Keep interrupt handler for host and peripheral modes separate --- drivers/usb/dwc2/core.c | 10 drivers/usb/dwc2/core.h | 3 +++ drivers/usb/dwc2/core_intr.c | 3 +++ drivers/usb/dwc2/gadget.c | 57 ++-- drivers/usb/dwc2/pci.c | 6 + drivers/usb/dwc2/platform.c | 9 +++ 6 files changed, 23 insertions(+), 65 deletions(-) diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index d926945..7605850b 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -458,16 +458,6 @@ int dwc2_core_init(struct dwc2_hsotg *hsotg, bool select_phy, int irq) /* Clear the SRP success bit for FS-I2c */ hsotg-srp_success = 0; -if (irq = 0) { - dev_dbg(hsotg-dev, registering common handler for irq%d\n, - irq); - retval = devm_request_irq(hsotg-dev, irq, - dwc2_handle_common_intr, IRQF_SHARED, - dev_name(hsotg-dev), hsotg); - if (retval) - return retval; - } - /* Enable common interrupts */ dwc2_enable_common_interrupts(hsotg); diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 80d29c7..ec70862 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -967,6 +967,7 @@ extern int s3c_hsotg_suspend(struct dwc2_hsotg *dwc2); extern int s3c_hsotg_resume(struct dwc2_hsotg *dwc2); extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq); extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2); +irqreturn_t s3c_hsotg_irq(int irq, void *pw); #else static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2) { return 0; } @@ -977,6 +978,8 @@ static inline int s3c_hsotg_resume(struct dwc2_hsotg *dwc2) static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) { return 0; } static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {} +static inline irqreturn_t s3c_hsotg_irq(int irq, void *pw) +{ return IRQ_HANDLED; } #endif #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index b176c2f..b0c14e0 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -474,6 +474,9 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev) spin_lock(hsotg-lock); +if (dwc2_is_device_mode(hsotg)) + retval = s3c_hsotg_irq(irq, dev); + gintsts = dwc2_read_common_intr(hsotg); if (gintsts ~GINTSTS_PRTINT) retval = IRQ_HANDLED; diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 19d1b03..202f8cc 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -2257,14 +2257,13 @@ void s3c_hsotg_core_init(struct dwc2_hsotg *hsotg) * @irq: The IRQ number triggered * @pw: The pw value when registered the handler. */ -static irqreturn_t s3c_hsotg_irq(int irq, void *pw) +irqreturn_t s3c_hsotg_irq(int irq, void *pw) why ? It would've been a lot easier to just make the IRQ line shared. Reworked to use IRQF_SHARED... Thanks, Dinh -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJUU6P5AAoJEBmUBAuBoyj0vWwP/1Qq0Gt9U1MF6LHJU853+P+I jx6xVVanPSuK8yTWprh7M/endv+UkVwwt+2sGZ0MMzLDH1T7Kf6FdqhjlNqvFeRK qK5LUkpMKvMJKcw6tQNGsjB75X93GRkWLxuZEQ9BpGjfy1fct6Wic/kv66dUVCCI DevfZZFPGdSmiichVup0sO/cfTJ84R8xyQVuF77LvZ/Wbm606ypV4PTMEjKcFhmr 5s6BeOINm7p1NMuoDaWbmjjOHAfTwy06W3p81LMsWH5yfVCq4TgJSwMbdo8s/i/Y po/1Nu0x5lwwao1J2OZA7b30i1vbtztCzVPtFcO5quN9rOlsaXRNBGLf9jjHP1OO PzRSsh1EbAinc4Ohbnii/bdx2g7lWP2ktf5qqoeVocgZkrKA/k5w/QK1qL1z3Onl ZkylCf5VMd06eOkeGbdce8UFhbceNLw0ryqAVN0bsxiD0u78aH6KvhYhBo29p/1R VMlKPrsYVDp9xjsp24S/u7o4NvNx8VeIfaKA5JN0Hnzncy03ByaO/o5iMPLb231l /yCoVyt2A9q7LQs/HeyCZYOFAPpG8ZbTsZrucKB9rRWXizDIrpIfJdcH3mIp71ab fhhf8sE1kDMrqADLTd1UL96DKCJnU2N8qJLSll6uWNi95J6OogIkj+H4YFPXXWiM +wUVqVAyYzPKsCaMRNTF =5+gL -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 5/8] usb: dwc2: Add call_gadget functions for perpheral mode interrupts
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 10/30/2014 09:01 AM, Felipe Balbi wrote: On Tue, Oct 28, 2014 at 06:25:46PM -0500, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Update the dwc2 wakeup and suspend interrupt functions to use call_gadget when the IP is in peripheral mode. it seems like you're actually fixing a bug here. Those calls weren't there before. This patch can be sent separately from this series. Thanks, Dinh -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJUU6QzAAoJEBmUBAuBoyj0XpIQAKLQxsksyfEBAOWNwapO1Grx jMK9+QpqZuGwo5Gha905iAO1Q/XP5tHvkLBzGyeEpNhEqBWXTTAvu1i2EToSiNId JTtae/eJA5xfxoljULqL3Vuy1SfOt+EdBHoMdn/vQQfBp9kUiqfJNgZeUsDgvbBK 7xNfdocMQjqPPbseNGL3193aasOFyZdc9sI+ZR3XjeXjNPgzwJzfB9eMZH+zpRKU nVcWY5UBkw05UnvDNwc4e/+VNpBRj4NLBBLGPleUJ1KXyunUet5r4oCLYLasZaFn zk6VvovQMUDk9DhnK2d8w6oosmSA/eD8zKn3K3++LV/XGwb5rHP+zC5jpAkbjN/o 4ZudegACKNUiwvo0tfbgtI5no/VzlaArSM6n3WT6Fz5cDveibZbU7z78AqIqRkwF Ai4+bn510OCgfEEu2rVRXl21r/TAt9r3ujh1mPPUwJ+nWdoxjmm7+0MyKNSeB/Ab MmVZfQaELLGTOMHH0T0/yr6XX8Bc4LxGtbjVQRlmW7mYfafnzhE+aWAeLnyF4V9r cA0YE09jOQoXh8WU6k18SKbapF8HDaXSLBcTWnXBR93TQu4U05qx5iB8hEZ7S9NT z3uQdfgjk8LjhaYWTtSsT29E6oDbvho8h5TF48KiwN4Ud0wagqJq1W3vlGhrgKUd wy94/mOPpifATM0S4KUr =qCZw -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 1/8] usb: dwc2: Update the gadget driver to use common dwc2_hsotg structure
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 10/30/2014 08:54 AM, Felipe Balbi wrote: On Tue, Oct 28, 2014 at 06:25:42PM -0500, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Adds the gadget data structure and appropriate data structure pointers to the common dwc2_hsotg data structure. To keep the driver data dereference code looking clean, the gadget variable declares are only available for peripheral and dual-role mode. This is needed so that the dwc2_hsotg data structure can be used by the hcd and gadget drivers. Updates gadget.c to use the dwc2_hsotg data structure and gadget pointers that have been moved into the common dwc2_hsotg structure. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Signed-off-by: Paul Zimmerman pa...@synopsys.com --- v5: Keep the changes to mininum and maintain hcd and gadget driver to build and work separately. Use IS_ENABLED() instead of #if defined v3: Updated with paulz's suggestion to avoid double pointers. v2: Left the function parameter name as 'hsotg' and just changed its type. --- drivers/usb/dwc2/core.h | 156 -- drivers/usb/dwc2/gadget.c | 145 +- 2 files changed, 154 insertions(+), 147 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 55c90c5..96c283d 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -84,7 +84,7 @@ static const char * const s3c_hsotg_supply_names[] = { */ #define EP0_MPS_LIMIT 64 -struct s3c_hsotg; +struct dwc2_hsotg; struct s3c_hsotg_req; /** @@ -130,7 +130,7 @@ struct s3c_hsotg_req; struct s3c_hsotg_ep { struct usb_ep ep; struct list_headqueue; - struct s3c_hsotg*parent; + struct dwc2_hsotg *parent; struct s3c_hsotg_req*req; struct dentry *debugfs; @@ -155,67 +155,6 @@ struct s3c_hsotg_ep { }; /** - * struct s3c_hsotg - driver state. - * @dev: The parent device supplied to the probe function - * @driver: USB gadget driver - * @phy: The otg phy transceiver structure for phy control. - * @uphy: The otg phy transceiver structure for old USB phy control. - * @plat: The platform specific configuration data. This can be removed once - * all SoCs support usb transceiver. - * @regs: The memory area mapped for accessing registers. - * @irq: The IRQ number we are using - * @supplies: Definition of USB power supplies - * @phyif: PHY interface width - * @dedicated_fifos: Set if the hardware has dedicated IN-EP fifos. - * @num_of_eps: Number of available EPs (excluding EP0) - * @debug_root: root directrory for debugfs. - * @debug_file: main status file for debugfs. - * @debug_fifo: FIFO status file for debugfs. - * @ep0_reply: Request used for ep0 reply. - * @ep0_buff: Buffer for EP0 reply data, if needed. - * @ctrl_buff: Buffer for EP0 control requests. - * @ctrl_req: Request for EP0 control packets. - * @setup: NAK management for EP0 SETUP - * @last_rst: Time of last reset - * @eps: The endpoints being supplied to the gadget framework - */ -struct s3c_hsotg { - struct device*dev; - struct usb_gadget_driver *driver; - struct phy *phy; -struct usb_phy *uphy; - struct s3c_hsotg_plat*plat; - - spinlock_t lock; - -void __iomem*regs; -int irq; - struct clk *clk; - - struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)]; - -u32 phyif; - int fifo_mem; - unsigned int dedicated_fifos:1; - unsigned char num_of_eps; - u32 fifo_map; - -struct dentry *debug_root; - struct dentry *debug_file; - struct dentry *debug_fifo; - - struct usb_request *ep0_reply; - struct usb_request *ctrl_req; -u8 ep0_buff[8]; - u8 ctrl_buff[8]; - - struct usb_gadget gadget; - unsigned intsetup; - unsigned long last_rst; - struct s3c_hsotg_ep *eps; -}; - -/** * struct s3c_hsotg_req - data transfer request * @req: The USB gadget request * @queue: The list of requests for the endpoint this is queued for. @@ -229,6 +168,7 @@ struct s3c_hsotg_req { unsigned char mapped; }; +#if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE) #define call_gadget(_hs, _entry) \ do { \ if ((_hs)-gadget.speed != USB_SPEED_UNKNOWN \ @@ -238,6 +178,9 @@ do { \ spin_lock(_hs-lock); \ } \ } while (0) +#else +#define call_gadget(_hs, _entry) do {} while (0) +#endif struct dwc2_hsotg; struct dwc2_host_chan; @@ -495,11 +438,13 @@ struct dwc2_hw_params { * struct dwc2_hsotg - Holds the state of the driver, including the non-periodic * and periodic schedules * + * These are common for both host and peripheral modes: + * * @dev:The struct device pointer * @regs
Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node
On 10/30/2014 09:04 AM, Felipe Balbi wrote: Hi, On Tue, Oct 28, 2014 at 06:25:47PM -0500, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Since the dwc2 hcd driver is currently not looking for a clock node during init, we should not completely fail if there isn't a clock provided. For dual-role mode, we will only fail init for a non-clock node error. We then update the HCD to only call gadget funtions if there is a proper clock node. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com --- v5: reworked to not access gadget functions from the hcd. --- drivers/usb/dwc2/core.h | 3 +-- drivers/usb/dwc2/core_intr.c | 9 ++--- drivers/usb/dwc2/hcd.c | 3 ++- drivers/usb/dwc2/platform.c | 19 +++ 4 files changed, 24 insertions(+), 10 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index ec70862..48120c8 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -660,6 +660,7 @@ struct dwc2_hsotg { #endif #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */ +struct clk *clk; #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE) /* Gadget structures */ struct usb_gadget_driver *driver; @@ -667,8 +668,6 @@ struct dwc2_hsotg { struct usb_phy *uphy; struct s3c_hsotg_plat *plat; -struct clk *clk; - struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)]; u32 phyif; diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index 1240875..1608037 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -339,7 +339,8 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) } /* Change to L0 state */ hsotg-lx_state = DWC2_L0; -call_gadget(hsotg, resume); +if (!IS_ERR(hsotg-clk)) +call_gadget(hsotg, resume); instead of exposing the clock detail to the entire driver, add IS_ERR() checks to resume and suspend instead. In fact, NULL is a valid clock, so you might as well: clk = clk_get(foo, bar); if (IS_ERR(clk)) dwc-clk = NULL; else dwc-clk = clk; Then you don't need any IS_ERR() checks sprinkled around the driver. But we would still need to check for the clock before accessing gadget functionality right? if (dwc2-clk) call_gadget(); @@ -400,7 +401,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg) DSTS.Suspend Status=%d HWCFG4.Power Optimize=%d\n, !!(dsts DSTS_SUSPSTS), hsotg-hw_params.power_optimized); -call_gadget(hsotg, suspend); +if (!IS_ERR(hsotg-clk)) +call_gadget(hsotg, suspend); } else { if (hsotg-op_state == OTG_STATE_A_PERIPHERAL) { dev_dbg(hsotg-dev, a_peripheral-a_host\n); @@ -477,7 +479,8 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev) spin_lock(hsotg-lock); if (dwc2_is_device_mode(hsotg)) -retval = s3c_hsotg_irq(irq, dev); +if (!IS_ERR(hsotg-clk)) +retval = s3c_hsotg_irq(irq, dev); wait a minute, if there is no clock we don't call the gadget interrupt handler ? Why ? Who will disable the IRQ line ? This portion is no longer needed when I use shared IRQ lines. diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 44c609f..fa49c72 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -1371,7 +1371,8 @@ static void dwc2_conn_id_status_change(struct work_struct *work) hsotg-op_state = OTG_STATE_B_PERIPHERAL; dwc2_core_init(hsotg, false, -1); dwc2_enable_global_interrupts(hsotg); -s3c_hsotg_core_init(hsotg); +if (!IS_ERR(hsotg-clk)) +s3c_hsotg_core_init(hsotg); } else { /* A-Device connector (Host Mode) */ dev_dbg(hsotg-dev, connId A\n); diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index 72f32f7..77c8417 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -217,8 +217,17 @@ static int dwc2_driver_probe(struct platform_device *dev) spin_lock_init(hsotg-lock); retval = dwc2_gadget_init(hsotg, irq); -if (retval) -return retval; +if (retval) { +/* + * We will not fail the driver initialization for dual-role + * if no clock node is supplied. However, all gadget + * functionality will be disabled if a clock node is not + * provided. Host functionality will continue. + * TO-DO: make clock node a requirement for the HCD
Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node
On 10/31/2014 12:42 PM, Felipe Balbi wrote: Hi, On Fri, Oct 31, 2014 at 10:20:06AM -0500, Dinh Nguyen wrote: @@ -339,7 +339,8 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) } /* Change to L0 state */ hsotg-lx_state = DWC2_L0; - call_gadget(hsotg, resume); + if (!IS_ERR(hsotg-clk)) + call_gadget(hsotg, resume); instead of exposing the clock detail to the entire driver, add IS_ERR() checks to resume and suspend instead. In fact, NULL is a valid clock, so you might as well: clk = clk_get(foo, bar); if (IS_ERR(clk)) dwc-clk = NULL; else dwc-clk = clk; Then you don't need any IS_ERR() checks sprinkled around the driver. But we would still need to check for the clock before accessing gadget functionality right? if (dwc2-clk) call_gadget(); Read my comment again. NULL is a valid clock. Look at what clk_enable() does when a NULL pointer is passed: static int __clk_enable(struct clk *clk) { int ret = 0; if (!clk) return 0; if (WARN_ON(clk-prepare_count == 0)) return -ESHUTDOWN; if (clk-enable_count == 0) { ret = __clk_enable(clk-parent); if (ret) return ret; if (clk-ops-enable) { ret = clk-ops-enable(clk-hw); if (ret) { __clk_disable(clk-parent); return ret; } } } clk-enable_count++; return 0; } int clk_enable(struct clk *clk) { unsigned long flags; int ret; flags = clk_enable_lock(); ret = __clk_enable(clk); clk_enable_unlock(flags); return ret; } EXPORT_SYMBOL_GPL(clk_enable); Ah yes, thanks for the explanation. So if clk=NULL, it just return 0. But what I'm saying is that if the driver is configured for dual-role mode, and no clock is specified, then the driver should not be accessing any gadget functionality. So as the patch series stands right now, if I swap out an A connector to a B-connector, then I get a connect_id_status change interrupt. The status would show a device and I would initialize the gadget portion of the driver. diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 44c609f..96810f7 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -1371,7 +1371,8 @@ static void dwc2_conn_id_status_change(struct work_struct *work) hsotg-op_state = OTG_STATE_B_PERIPHERAL; dwc2_core_init(hsotg, false, -1); dwc2_enable_global_interrupts(hsotg); - s3c_hsotg_core_init(hsotg); + if (hsotg-clk) + s3c_hsotg_core_init(hsotg); So if I don't have a valid clock, I'll be accessing the peripheral portion of the IP. But I guess not having the check for the valid clock here should be fine as I don't see a case where there can be 2 different clocks for host and peripheral? @@ -400,7 +401,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg) DSTS.Suspend Status=%d HWCFG4.Power Optimize=%d\n, !!(dsts DSTS_SUSPSTS), hsotg-hw_params.power_optimized); - call_gadget(hsotg, suspend); + if (!IS_ERR(hsotg-clk)) + call_gadget(hsotg, suspend); } else { if (hsotg-op_state == OTG_STATE_A_PERIPHERAL) { dev_dbg(hsotg-dev, a_peripheral-a_host\n); @@ -477,7 +479,8 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev) spin_lock(hsotg-lock); if (dwc2_is_device_mode(hsotg)) - retval = s3c_hsotg_irq(irq, dev); + if (!IS_ERR(hsotg-clk)) + retval = s3c_hsotg_irq(irq, dev); wait a minute, if there is no clock we don't call the gadget interrupt handler ? Why ? Who will disable the IRQ line ? This portion is no static int __clk_enable(struct clk *clk) huh ? What I mean is that this has the potential of leaving that IRQ line enabled. Imagine you don't have a clock and s3c_hsotg_irq() isn't called, then a peripheral IRQ fires, since the handler isn't called, who will clear the interrupt ? Yes, right. This portion of the code is no longer needed when I switched to use a shared IRQ. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node
On 10/31/2014 02:31 PM, Dinh Nguyen wrote: On 10/31/2014 12:42 PM, Felipe Balbi wrote: Hi, On Fri, Oct 31, 2014 at 10:20:06AM -0500, Dinh Nguyen wrote: @@ -339,7 +339,8 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) } /* Change to L0 state */ hsotg-lx_state = DWC2_L0; - call_gadget(hsotg, resume); + if (!IS_ERR(hsotg-clk)) + call_gadget(hsotg, resume); instead of exposing the clock detail to the entire driver, add IS_ERR() checks to resume and suspend instead. In fact, NULL is a valid clock, so you might as well: clk = clk_get(foo, bar); if (IS_ERR(clk)) dwc-clk = NULL; else dwc-clk = clk; Then you don't need any IS_ERR() checks sprinkled around the driver. But we would still need to check for the clock before accessing gadget functionality right? if (dwc2-clk) call_gadget(); Read my comment again. NULL is a valid clock. Look at what clk_enable() does when a NULL pointer is passed: static int __clk_enable(struct clk *clk) { int ret = 0; if (!clk) return 0; if (WARN_ON(clk-prepare_count == 0)) return -ESHUTDOWN; if (clk-enable_count == 0) { ret = __clk_enable(clk-parent); if (ret) return ret; if (clk-ops-enable) { ret = clk-ops-enable(clk-hw); if (ret) { __clk_disable(clk-parent); return ret; } } } clk-enable_count++; return 0; } int clk_enable(struct clk *clk) { unsigned long flags; int ret; flags = clk_enable_lock(); ret = __clk_enable(clk); clk_enable_unlock(flags); return ret; } EXPORT_SYMBOL_GPL(clk_enable); Ah yes, thanks for the explanation. So if clk=NULL, it just return 0. But what I'm saying is that if the driver is configured for dual-role mode, and no clock is specified, then the driver should not be accessing any gadget functionality. So as the patch series stands right now, if I swap out an A connector to a B-connector, then I get a connect_id_status change interrupt. The status would show a device and I would initialize the gadget portion of the driver. diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 44c609f..96810f7 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -1371,7 +1371,8 @@ static void dwc2_conn_id_status_change(struct work_struct *work) hsotg-op_state = OTG_STATE_B_PERIPHERAL; dwc2_core_init(hsotg, false, -1); dwc2_enable_global_interrupts(hsotg); - s3c_hsotg_core_init(hsotg); + if (hsotg-clk) + s3c_hsotg_core_init(hsotg); So if I don't have a valid clock, I'll be accessing the peripheral portion of the IP. But I guess not having the check for the valid clock here should be fine as I don't see a case where there can be 2 different clocks for host and peripheral? Ah...nevermind. I don't need to check for clocks at all because in dwc2_gadget_init(), the clock node check comes before usb_add_gadget_udc(). Thus without a clock node, gadget functionality is disabled already. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 8/8] usb: dwc2: move usb_disabled() call to host driver only
On 10/28/14, 8:26 PM, Paul Zimmerman wrote: From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com] Sent: Tuesday, October 28, 2014 4:26 PM Now that platform.c will get built for both Host and Gadget, if we leave the usb_disabled() call in platform.c, it results in the following build error when (!USB USB_GADGET) condition is met. ERROR: usb_disabled [drivers/usb/dwc2/dwc2_platform.ko] undefined! Since usb_disabled() is mostly used to disable USB host functionality, move the call the host portion for the DWC2 driver. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com --- drivers/usb/dwc2/hcd.c | 3 +++ drivers/usb/dwc2/platform.c | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index fa49c72..b741997 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2780,6 +2780,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, int i, num_channels; int retval; +if (usb_disabled()) +return -ENODEV; + dev_dbg(hsotg-dev, DWC OTG HCD INIT\n); /* Detect config values from hardware */ diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index 77c8417..123cf54 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -157,9 +157,6 @@ static int dwc2_driver_probe(struct platform_device *dev) int retval; int irq; -if (usb_disabled()) -return -ENODEV; - match = of_match_device(dwc2_of_match_table, dev-dev); if (match match-data) { params = match-data; I'm confused. You are saying the build is broken until patch 8/8 is applied? As always, that is not acceptable. You need to fix the breakage at the point where it was introduced, not leave it broken until the last patch in the series. The build gets broken when patch 7/8 of is applied. That is the patch that finally allows platform.c to get built for host and gadget. I can fold this patch into patch 7/8. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 7/7] usb: dwc2: Update Kconfig to support dual-role
On 10/22/2014 03:27 PM, Paul Bolle wrote: On Tue, 2014-10-21 at 15:47 -0500, Dinh Nguyen wrote: On 10/20/2014 02:42 PM, Paul Bolle wrote: (Side note: drivers/usb/dwc2/Kconfig is sourced (in drivers/usb/Kconfig) even if USB is _not_ set. But USB_DCW2 still depends on USB. Why is that?) Because USB is for Host-Side support. DWC2 driver should only get built for when USB is enabled. I think you're getting USB and USB_SUPPORT mixed up. No, I don't think I did. Because in drivers/usb/Kconfig we see if USB [...] endif [...] source drivers/usb/dwc2/Kconfig [...] Well, CONFIG_USB enables the host stack that is needed by the DWC2 driver, without CONFIG_USB, the DWC2 driver will not be able to build. In addition, thanks to your comment, I realized that DWC2 should also be available to build when USB_GADGET is enabled. A patch has been sent: http://marc.info/?l=linux-usbm=141392377124818w=2 I haven't checked, but doesn't this mean USB_DWC2 could just depend on USB_SUPPORT? I don't think so because USB_SUPPORT will not select USB or USB_GAGDGET, and the DWC2 driver will need either 1 or both for it to build correctly. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 7/7] usb: dwc2: Update Kconfig to support dual-role
On 10/23/2014 01:28 PM, Paul Zimmerman wrote: From: Bartlomiej Zolnierkiewicz [mailto:b.zolnier...@samsung.com] Sent: Wednesday, October 22, 2014 5:26 AM On Monday, October 20, 2014 01:52:06 PM dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com config USB_DWC2_PLATFORM bool DWC2 Platform - depends on USB_DWC2_HOST default USB_DWC2_HOST It should be default USB_DWC2_HOST || USB_DWC2_PERIPHERAL because USB_DWC2_PLATFORM replaces current USB_DWC2_PERIPHERAL functionality. Additionaly USB_DWC2_PLATFORM should be changed to tristate (USB_DWC2_PERIPHERAL was a tristeate before your changes). Dinh, I think this is a good point. Is there any reason why USB_DWC2_PLATFORM (and USB_DWC2_PCI for that matter) can't be tristate? That's what DWC3 does. Yes, in my v6, I have made this change. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 7/7] usb: dwc2: Update Kconfig to support dual-role
On 10/20/2014 02:42 PM, Paul Bolle wrote: dingu...@opensource.altera.com schreef op ma 20-10-2014 om 13:52 [-0500]: From: Dinh Nguyen dingu...@opensource.altera.com Update DWC2 kconfig and makefile to support dual-role mode. The platform file will always get compiled for the case where the controller is directly connected to the CPU. So for loadable modules, dwc2.ko is built for host, peripheral, and dual-role mode. The PCI bus interface will be called dwc2_pci.ko and the platform interface module will be called dwc2_platform.ko. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- v5: dwc2.ko for all modes along with dwc2_platform.ko and dwc2_pci.ko will get built for kernel modules. v3: Add USB_GADGET=y and USB_GADGET=USB_DWC2 for peripheral and dual-role config options. v2: Remove reference to dwc2_gadget --- drivers/usb/dwc2/Kconfig | 61 --- drivers/usb/dwc2/Makefile | 32 - 2 files changed, 53 insertions(+), 40 deletions(-) diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index f93807b..1ea702e 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -1,5 +1,5 @@ config USB_DWC2 -bool DesignWare USB2 DRD Core Support +tristate DesignWare USB2 DRD Core Support depends on USB (Side note: drivers/usb/dwc2/Kconfig is sourced (in drivers/usb/Kconfig) even if USB is _not_ set. But USB_DCW2 still depends on USB. Why is that?) Because USB is for Host-Side support. DWC2 driver should only get built for when USB is enabled. I think you're getting USB and USB_SUPPORT mixed up. In addition, thanks to your comment, I realized that DWC2 should also be available to build when USB_GADGET is enabled. A patch has been sent: http://marc.info/?l=linux-usbm=141392377124818w=2 help Say Y here if your system has a Dual Role Hi-Speed USB @@ -10,31 +10,53 @@ config USB_DWC2 bus interface module (if you have a PCI bus system) will be called dwc2_pci.ko, and the platform interface module (for controllers directly connected to the CPU) will be called - dwc2_platform.ko. For gadget mode, there will be a single - module called dwc2_gadget.ko. - - NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The - host and gadget drivers are still currently separate drivers. - There are plans to merge the dwc2_gadget driver with the dwc2 - host driver in the near future to create a dual-role driver. + dwc2_platform.ko. For all modes(host, gadget and dual-role), there + will be a single module called dwc2.ko. if USB_DWC2 +choice +bool DWC2 Mode Selection +default USB_DWC2_DUAL_ROLE if (USB USB_GADGET) +default USB_DWC2_HOST if (USB !USB_GADGET) +default USB_DWC2_PERIPHERAL if (!USB USB_GADGET) + Juggling kconfig tristate logic is still rather hard for me, but don't the above three if rules all evaluate to non-zero if both USB and USB_GADGET are 'm'? If that's correct perhaps USB_DWC2_DUAL_ROLE should be the last default (assuming the last default wins, that is). As the way it is, the functionality is correct. As this is the same as DWC3's Kconfig, perhaps Felipe can comment if something doesn't seem correct. Besides, will the default USB_DWC2_PERIPHERAL ever trigger as !USB can't be true at this point, can it? Yes it can. USB is for Host-side support, so you can have a condition where you just want to build for GADGET and !USB. config USB_DWC2_HOST -tristate Host only mode +bool Host only mode depends on USB help The Designware USB2.0 high-speed host controller - integrated into many SoCs. + integrated into many SoCs. Select this option if you want the + driver to operate in Host-only mode. + +comment Gadget/Dual-role mode requires USB Gadget support to be enabled + +config USB_DWC2_PERIPHERAL +bool Gadget only mode +depends on USB_GADGET=y || USB_GADGET=USB_DWC2 +help + The Designware USB2.0 high-speed gadget controller + integrated into many SoCs. Select this option if you want the + driver to operate in Peripheral-only mode. This option requires + USB_GADGET=y. + +config USB_DWC2_DUAL_ROLE +bool Dual Role mode +depends on (USB=y || USB=USB_DWC2) (USB_GADGET=y || USB_GADGET=USB_DWC2) +help + Select this option if you want the driver to work in a dual-role + mode. In this mode both host and gadget features are enabled, and + the role will be determined by the cable that gets plugged-in. This + option requires USB_GADGET=y. +endchoice (I wonder how the dependencies of these three config entries interact with the three defaults of this choice. Since we're dealing with three options here this requires a piece of paper, a pencil, and clear mind
Re: [PATCHv4 08/12] usb: dwc2: gadget: Do not fail probe if there isn't a clock node
On 9/12/14, 11:28 AM, Bartlomiej Zolnierkiewicz wrote: [ added linux-kernel ML to cc: ] Hi, On Tuesday, August 26, 2014 11:19:59 AM dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Since the dwc2 hcd driver is currently not looking for a clock node during init, we should not completely fail if there isn't a clock provided. Add a check for a valid clock before calling clock functions. This doesn't look correct at least for the case when we are really missing the clock and USB_DWC2_PERIPHERAL=y (moreover it just looks wrong to access gadget functionalities when clock is disabled). It seems that it would be better to just disable gadget functionality on dwc2_gadget_init() failure in hcd and not call gadget functions later from hcd if gadget functionality is disabled. Yes...this is correct. Will fix up in v5. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 04/12] usb: dwc2: Add the appropriate init calls in platform code
Hi Robert, On 09/17/2014 03:52 PM, Stephen Warren wrote: On 09/17/2014 02:47 PM, Dinh Nguyen wrote: Hi Robert, On 9/12/14, 7:13 AM, Robert Baldyga wrote: Hi Dinh, On 08/26/2014 06:19 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Add the proper init calls for either host, gadget or both in platform.c Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- drivers/usb/dwc2/core.h | 13 + drivers/usb/dwc2/gadget.c | 2 +- drivers/usb/dwc2/platform.c | 28 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index f55e62d..3a49a00 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -960,6 +960,19 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg); */ extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg); +/* Gadget defines */ +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE) +extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg); +extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2); Function s3c_hsotg_core_init() is used only inside file gadget.c so exporting it makes no sense. By the way it should be static. Yes, I agree here. Fixed up in v5. I went back to look at the code and realized that I had to use s3c_hsotg_core_init() in patch 5/12 - usb: dwc2: Initialize the USB core for peripheral mode. I need to add the call to s3c_hsotg_core_init() for a B-cable insert event, and this is in hcd.c. So I need to export it. +extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq); +#else +static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {} +static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2) +{ return 0; } +static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) +{ return 0; } It also makes no sense to have this functions declared if you don't have to use them. They are called in one place in code, inside dwc2_driver_probe() function, so you can rather use if defined() there. I'm not sure I agree here. This is necessary for the current runtime implementation of the role initialization. This is probably relevant with your next 2 comments. +#endif + #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE) /** * dwc2_hcd_get_frame_number() - Returns current frame number diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 96f868f..efa68a0 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3572,7 +3572,7 @@ err_clk: * s3c_hsotg_remove - remove function for hsotg driver * @pdev: The platform information for the driver */ -static int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) +int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) { usb_del_gadget_udc(hsotg-gadget); diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index dd2f8f5..2871f351 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -92,7 +92,14 @@ static int dwc2_driver_remove(struct platform_device *dev) { struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); -dwc2_hcd_remove(hsotg); +if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL)) +s3c_hsotg_remove(hsotg); +else if (IS_ENABLED(CONFIG_USB_DWC2_HOST)) +dwc2_hcd_remove(hsotg); +else { /* dual role */ +dwc2_hcd_remove(hsotg); +s3c_hsotg_remove(hsotg); +} Why don't make this checks compile-time? Do you have a reason for a compile-time versus runtime here? It just seems that from a few discussion threads on lkml that there is a general biased towards using IS_ENABLED() as it looks a bit cleaner than littering the code with a bunch of #ifdefs. With typical compiler optimization, if (IS_ENABLED(...)) *is* a compile-time check. Yet, it allows the code within the if block body to be parsed, so that even if the code doesn't make it into the binary, it still gets syntax checking etc. Thanks Stephen for this information! Robert, are your comments properly addressed for this patch? Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 04/12] usb: dwc2: Add the appropriate init calls in platform code
On Thu, Sep 18, 2014 at 10:17 AM, Dinh Nguyen dingu...@opensource.altera.com wrote: Hi Robert, On 09/17/2014 03:52 PM, Stephen Warren wrote: On 09/17/2014 02:47 PM, Dinh Nguyen wrote: Hi Robert, On 9/12/14, 7:13 AM, Robert Baldyga wrote: Hi Dinh, On 08/26/2014 06:19 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Add the proper init calls for either host, gadget or both in platform.c Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- drivers/usb/dwc2/core.h | 13 + drivers/usb/dwc2/gadget.c | 2 +- drivers/usb/dwc2/platform.c | 28 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index f55e62d..3a49a00 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -960,6 +960,19 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg); */ extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg); +/* Gadget defines */ +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE) +extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg); +extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2); Function s3c_hsotg_core_init() is used only inside file gadget.c so exporting it makes no sense. By the way it should be static. Yes, I agree here. Fixed up in v5. I went back to look at the code and realized that I had to use s3c_hsotg_core_init() in patch 5/12 - usb: dwc2: Initialize the USB core for peripheral mode. I need to add the call to s3c_hsotg_core_init() for a B-cable insert event, and this is in hcd.c. So I need to export it. I can see your reason for the comment. So I'll move the export of the function into the next patch that needs it, to make it clearer. Dinh +extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq); +#else +static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {} +static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2) +{ return 0; } +static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) +{ return 0; } It also makes no sense to have this functions declared if you don't have to use them. They are called in one place in code, inside dwc2_driver_probe() function, so you can rather use if defined() there. I'm not sure I agree here. This is necessary for the current runtime implementation of the role initialization. This is probably relevant with your next 2 comments. +#endif + #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE) /** * dwc2_hcd_get_frame_number() - Returns current frame number diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 96f868f..efa68a0 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3572,7 +3572,7 @@ err_clk: * s3c_hsotg_remove - remove function for hsotg driver * @pdev: The platform information for the driver */ -static int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) +int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) { usb_del_gadget_udc(hsotg-gadget); diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index dd2f8f5..2871f351 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -92,7 +92,14 @@ static int dwc2_driver_remove(struct platform_device *dev) { struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); -dwc2_hcd_remove(hsotg); +if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL)) +s3c_hsotg_remove(hsotg); +else if (IS_ENABLED(CONFIG_USB_DWC2_HOST)) +dwc2_hcd_remove(hsotg); +else { /* dual role */ +dwc2_hcd_remove(hsotg); +s3c_hsotg_remove(hsotg); +} Why don't make this checks compile-time? Do you have a reason for a compile-time versus runtime here? It just seems that from a few discussion threads on lkml that there is a general biased towards using IS_ENABLED() as it looks a bit cleaner than littering the code with a bunch of #ifdefs. With typical compiler optimization, if (IS_ENABLED(...)) *is* a compile-time check. Yet, it allows the code within the if block body to be parsed, so that even if the code doesn't make it into the binary, it still gets syntax checking etc. Thanks Stephen for this information! Robert, are your comments properly addressed for this patch? Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role
Hi Bartlomiej, On 09/12/2014 10:49 AM, Bartlomiej Zolnierkiewicz wrote: [ added linux-kernel ML to cc: ] Hi, On Tuesday, August 26, 2014 11:19:52 AM dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Update DWC2 kconfig and makefile to support dual-role mode. The platform file will always get compiled for the case where the controller is directly connected to the CPU. So for loadable modules, only dwc2.ko is needed. Kconfig and Makefile changes should be done after (or at the same time as) driver code itself is modified to support dual-role mode. Each individual patch of the patchset should be correct itself (not cause any breakages) in order to keep the whole patchset bisectable. Paulz mentioned this in v1 of this patch series and ever since then, I have been careful to test each patch on it's own, and each version since then has passed 0-Day kbuild testing. But I may have missed something in v4. Will try to move the edits to Kconfig/Makefile to end for v5. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- v3: Add USB_GADGET=y and USB_GADGET=USB_DWC2 for peripheral and dual-role config options. v2: Remove reference to dwc2_gadget --- drivers/usb/dwc2/Kconfig | 63 +++ drivers/usb/dwc2/Makefile | 21 2 files changed, 47 insertions(+), 37 deletions(-) diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index f93807b..4396a1f 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -1,40 +1,29 @@ config USB_DWC2 -bool DesignWare USB2 DRD Core Support +tristate DesignWare USB2 DRD Core Support depends on USB help Say Y here if your system has a Dual Role Hi-Speed USB controller based on the DesignWare HSOTG IP Core. - For host mode, if you choose to build the driver as dynamically - linked modules, the core module will be called dwc2.ko, the PCI - bus interface module (if you have a PCI bus system) will be - called dwc2_pci.ko, and the platform interface module (for - controllers directly connected to the CPU) will be called - dwc2_platform.ko. For gadget mode, there will be a single - module called dwc2_gadget.ko. - - NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The - host and gadget drivers are still currently separate drivers. - There are plans to merge the dwc2_gadget driver with the dwc2 - host driver in the near future to create a dual-role driver. + If you choose to build the driver as dynamically + linked modules, a single dwc2.ko(regardless of mode of operation) minort nitpick: is missing after dwc2.ko + will get built for both platform IPs and PCI. Why do you want ot merge both platform and PCI drivers into one? To do it properly you need to modify module_init/exit() of the final module to properly handle both PCI and platform devices. It should be easier to leave separate dwc2_pci/platform drivers and just put the common code into dwc2.ko. I need to rework to the comment. I think it should say, will get built for either platform IPs or PCI. I am not merging both platform and PCI drivers into one. if USB_DWC2 +choice +bool DWC2 Mode Selection +default USB_DWC2_DUAL_ROLE if (USB USB_GADGET) +default USB_DWC2_HOST if (USB !USB_GADGET) +default USB_DWC2_PERIPHERAL if (!USB USB_GADGET) + config USB_DWC2_HOST -tristate Host only mode +bool Host only mode depends on USB help The Designware USB2.0 high-speed host controller - integrated into many SoCs. - -config USB_DWC2_PLATFORM -bool DWC2 Platform -depends on USB_DWC2_HOST -default USB_DWC2_HOST -help - The Designware USB2.0 platform interface module for - controllers directly connected to the CPU. This is only - used for host mode. + integrated into many SoCs. Select this option if you want the + driver to operate in Host-only mode. config USB_DWC2_PCI bool DWC2 PCI Why have you left this option into middle of 'choice' selection? Will move... You've also left the depends on USB_DWC2_HOST PCI unmodified which causes DWC2 PCI support to show up only if Host only mode is selected (it is not available if Dual Role mode is selected). Does PCI support gadget? I left it unmodified because I didn't think the PCI driver supported Gadget. @@ -47,11 +36,31 @@ config USB_DWC2_PCI comment Gadget mode requires USB Gadget support to be enabled config USB_DWC2_PERIPHERAL -tristate Gadget only mode -depends on USB_GADGET +bool Gadget only mode +depends on USB_GADGET=y || USB_GADGET=USB_DWC2 help The Designware USB2.0 high-speed gadget controller - integrated into many SoCs. + integrated
Re: [PATCHv4 04/12] usb: dwc2: Add the appropriate init calls in platform code
On 09/12/2014 11:18 AM, Bartlomiej Zolnierkiewicz wrote: [ added linux-kernel ML to cc: ] Hi, On Tuesday, August 26, 2014 11:19:55 AM dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Add the proper init calls for either host, gadget or both in platform.c Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- drivers/usb/dwc2/core.h | 13 + drivers/usb/dwc2/gadget.c | 2 +- drivers/usb/dwc2/platform.c | 28 Where are correspoding changes to pci.c? I cannot find them in your patchset. The current PCI driver only supports HCD. If you want PCI to support gadget, then it can be done in a separate patchset. 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index f55e62d..3a49a00 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -960,6 +960,19 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg); */ extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg); +/* Gadget defines */ +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE) +extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg); +extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2); +extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq); +#else +static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {} +static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2) +{ return 0; } +static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) +{ return 0; } +#endif + #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE) /** * dwc2_hcd_get_frame_number() - Returns current frame number diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 96f868f..efa68a0 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3572,7 +3572,7 @@ err_clk: * s3c_hsotg_remove - remove function for hsotg driver * @pdev: The platform information for the driver */ -static int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) +int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) { usb_del_gadget_udc(hsotg-gadget); diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index dd2f8f5..2871f351 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -92,7 +92,14 @@ static int dwc2_driver_remove(struct platform_device *dev) { struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); -dwc2_hcd_remove(hsotg); +if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL)) +s3c_hsotg_remove(hsotg); +else if (IS_ENABLED(CONFIG_USB_DWC2_HOST)) +dwc2_hcd_remove(hsotg); +else { /* dual role */ +dwc2_hcd_remove(hsotg); +s3c_hsotg_remove(hsotg); Why not simply always call: dwc2_hcd_remove(hsotg); s3c_hsotg_remove(hsotg); and add appropriate stub for dwc2_hcd_remove() for CONFIG_USB_DWC2_PERIPHERAL=y? Yes...I'll do this for v5. Same for the init(). Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 04/12] usb: dwc2: Add the appropriate init calls in platform code
Hi Robert, On 9/12/14, 7:13 AM, Robert Baldyga wrote: Hi Dinh, On 08/26/2014 06:19 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Add the proper init calls for either host, gadget or both in platform.c Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- drivers/usb/dwc2/core.h | 13 + drivers/usb/dwc2/gadget.c | 2 +- drivers/usb/dwc2/platform.c | 28 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index f55e62d..3a49a00 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -960,6 +960,19 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg); */ extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg); +/* Gadget defines */ +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE) +extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg); +extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2); Function s3c_hsotg_core_init() is used only inside file gadget.c so exporting it makes no sense. By the way it should be static. Yes, I agree here. Fixed up in v5. +extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq); +#else +static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {} +static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2) +{ return 0; } +static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) +{ return 0; } It also makes no sense to have this functions declared if you don't have to use them. They are called in one place in code, inside dwc2_driver_probe() function, so you can rather use if defined() there. I'm not sure I agree here. This is necessary for the current runtime implementation of the role initialization. This is probably relevant with your next 2 comments. +#endif + #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE) /** * dwc2_hcd_get_frame_number() - Returns current frame number diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 96f868f..efa68a0 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3572,7 +3572,7 @@ err_clk: * s3c_hsotg_remove - remove function for hsotg driver * @pdev: The platform information for the driver */ -static int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) +int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) { usb_del_gadget_udc(hsotg-gadget); diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index dd2f8f5..2871f351 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -92,7 +92,14 @@ static int dwc2_driver_remove(struct platform_device *dev) { struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); -dwc2_hcd_remove(hsotg); +if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL)) +s3c_hsotg_remove(hsotg); +else if (IS_ENABLED(CONFIG_USB_DWC2_HOST)) +dwc2_hcd_remove(hsotg); +else { /* dual role */ +dwc2_hcd_remove(hsotg); +s3c_hsotg_remove(hsotg); +} Why don't make this checks compile-time? Do you have a reason for a compile-time versus runtime here? It just seems that from a few discussion threads on lkml that there is a general biased towards using IS_ENABLED() as it looks a bit cleaner than littering the code with a bunch of #ifdefs. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 06/12] usb: dwc2: Update common interrupt handler to call gadget interrupt handler
On 9/12/14, 7:50 AM, Robert Baldyga wrote: Hi, On 08/26/2014 06:19 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Make dwc2_handle_common_intr call the gadget interrupt function when operating in peripheral mode. Remove the spinlock functions in s3c_hsotg_irq as dwc2_handle_common_intr() already has the spinlocks. Remove duplicate interrupt conditions that was in gadget, as those are handled by dwc2 common interrupt handler. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- v2: Keep interrupt handler for host and peripheral modes separate --- drivers/usb/dwc2/core.h | 3 +++ drivers/usb/dwc2/core_intr.c | 3 +++ drivers/usb/dwc2/gadget.c| 50 3 files changed, 10 insertions(+), 46 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 3a49a00..bbb0f52 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -965,12 +965,15 @@ extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg); extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg); extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2); extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq); +irqreturn_t s3c_hsotg_irq(int irq, void *pw); #else static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {} static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2) { return 0; } static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) { return 0; } +static inline irqreturn_t s3c_hsotg_irq(int irq, void *pw) +{ return IRQ_HANDLED; } #endif #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index c93918b..24d4c0d 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -472,6 +472,9 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev) spin_lock(hsotg-lock); +if (dwc2_is_device_mode(hsotg)) +retval = s3c_hsotg_irq(irq, dev); + gintsts = dwc2_read_common_intr(hsotg); if (gintsts ~GINTSTS_PRTINT) retval = IRQ_HANDLED; diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index efa68a0..a1c93bf 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -2257,14 +2257,13 @@ void s3c_hsotg_core_init(struct dwc2_hsotg *hsotg) * @irq: The IRQ number triggered * @pw: The pw value when registered the handler. */ -static irqreturn_t s3c_hsotg_irq(int irq, void *pw) +irqreturn_t s3c_hsotg_irq(int irq, void *pw) { struct dwc2_hsotg *hsotg = pw; int retry_count = 8; u32 gintsts; u32 gintmsk; -spin_lock(hsotg-lock); irq_retry: gintsts = readl(hsotg-regs + GINTSTS); gintmsk = readl(hsotg-regs + GINTMSK); @@ -2274,33 +2273,12 @@ irq_retry: gintsts = gintmsk; -if (gintsts GINTSTS_OTGINT) { -u32 otgint = readl(hsotg-regs + GOTGINT); - -dev_info(hsotg-dev, OTGInt: %08x\n, otgint); - -writel(otgint, hsotg-regs + GOTGINT); -} - -if (gintsts GINTSTS_SESSREQINT) { -dev_dbg(hsotg-dev, %s: SessReqInt\n, __func__); -writel(GINTSTS_SESSREQINT, hsotg-regs + GINTSTS); -} - if (gintsts GINTSTS_ENUMDONE) { writel(GINTSTS_ENUMDONE, hsotg-regs + GINTSTS); s3c_hsotg_irq_enumdone(hsotg); } -if (gintsts GINTSTS_CONIDSTSCHNG) { -dev_dbg(hsotg-dev, ConIDStsChg (DSTS=0x%08x, GOTCTL=%08x)\n, -readl(hsotg-regs + DSTS), -readl(hsotg-regs + GOTGCTL)); - -writel(GINTSTS_CONIDSTSCHNG, hsotg-regs + GINTSTS); -} - if (gintsts (GINTSTS_OEPINT | GINTSTS_IEPINT)) { u32 daint = readl(hsotg-regs + DAINT); u32 daintmsk = readl(hsotg-regs + DAINTMSK); @@ -2381,25 +2359,6 @@ irq_retry: s3c_hsotg_handle_rx(hsotg); } -if (gintsts GINTSTS_MODEMIS) { -dev_warn(hsotg-dev, warning, mode mismatch triggered\n); -writel(GINTSTS_MODEMIS, hsotg-regs + GINTSTS); -} - -if (gintsts GINTSTS_USBSUSP) { -dev_info(hsotg-dev, GINTSTS_USBSusp\n); -writel(GINTSTS_USBSUSP, hsotg-regs + GINTSTS); - -call_gadget(hsotg, suspend); -} - -if (gintsts GINTSTS_WKUPINT) { -dev_info(hsotg-dev, GINTSTS_WkUpIn\n); -writel(GINTSTS_WKUPINT, hsotg-regs + GINTSTS); - -call_gadget(hsotg, resume); -} - if (gintsts GINTSTS_ERLYSUSP) { dev_dbg(hsotg-dev, GINTSTS_ErlySusp\n); writel(GINTSTS_ERLYSUSP, hsotg-regs + GINTSTS); @@ -2435,10 +2394,9 @@ irq_retry: if (gintsts IRQ_RETRY_MASK --retry_count 0
Re: [RFC PATCH 0/2] usb: dwc2: Enable URB giveback in a tasklet context
On 09/15/2014 11:08 AM, Alan Stern wrote: On Mon, 15 Sep 2014 dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Hi Ming-Lei, Thanks for your patch to enable the URB giveback in a tasklet context for the EHCI driver. I found your patch to fix a USB webcam timeout/stutter issue on the DWC2 HCD in the SOCFPGA platform. However, I need your help trying to figure out why I need the 2nd patch to get your URB giveback patch to fully work. Maybe you don't need it. Did you try testing with only the first patch? Yes, I did. 2nd patch was needed. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 00/12] usb: dwc2: Add support for dual role
On 09/12/2014 11:44 AM, Bartlomiej Zolnierkiewicz wrote: [ added linux-kernel ML to cc: ] Hi, On Tuesday, August 26, 2014 11:19:51 AM dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Hello, This is version 4 of the patch series that combines the dwc2 gadget and host driver into a single dual role driver. Here are the main differences from V2: - Patch 9/12 : Move spin_lock_init() earlier up in the function to guarantee no locks can be taken before the initializing the spin_lock. - Patch 12/12 : Same as patch 9/12. - Added Acked-by: paulz for all patches except patch 9/12 and 12/12. For v4, I have rebased the series on top of Greg KH's USB usb-linus tree [9b2667f usb: dwc2: gadget: Set the default EP max packet value as 8 bytes] and on top of the following patches that have not yet been applied: Doug Anderson: usb: dwc2: Read GNPTXFSIZ when in forced HOST mode Kever Yang : usb: dwc2: add 'mode' which based on Kconfig select or dts setting Robert Baldyga : usb: dwc2/gadget: fix series - 12 patches As usual, tested on SOCFPGA(host, gadget, and dual-role) and on Rpi-B (host mode only). I have pushed this series to a git repo to make it more convenient for people to test/review. git://git.rocketboards.org/linux-socfpga-next.git dwc2_dual_role_v4 Please also add linux-kernel ML to next revision of the patchset (helps to get reviewers). Also sorry for the late review but people are not yet used to the fact that dwc2 now also means our good old s3c-hsotg. ;-) Thanks for the review. I'll try to address all of your comments in the next version. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 03/12] usb: dwc2: Update the gadget driver to use common dwc2_hsotg structure
Hi Paul, On 09/08/2014 05:50 PM, Greg KH wrote: On Tue, Aug 26, 2014 at 11:19:54AM -0500, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Adds the gadget data structure and appropriate data structure pointers to the common dwc2_hsotg data structure. To keep the driver data dereference code looking clean, the gadget variable declares are only available for peripheral and dual-role mode. This is needed so that the dwc2_hsotg data structure can be used by the hcd and gadget drivers. Updates gadget.c to use the dwc2_hsotg data structure and gadget pointers that have been moved into the common dwc2_hsotg structure. Along with the updating the gadget driver to use the common dwc2_hsotg structure, a few other things are required in order for this patch to build properly. Those are: - Remove gadget module defines. Since the driver probing will be handled by either the platform or pci code. - Change the gadget probe function into gadget_init. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Signed-off-by: Paul Zimmerman pa...@synopsys.com --- v3: Updated with paulz's suggestion to avoid double pointers. v2: Left the function parameter name as 'hsotg' and just changed its type. --- drivers/usb/dwc2/core.h | 176 +--- drivers/usb/dwc2/gadget.c | 226 +- drivers/usb/dwc2/hcd.h| 10 -- 3 files changed, 184 insertions(+), 228 deletions(-) This doesn't apply as I couldn't take the patches you depended on here :( Can you work with Paul to get this into a series that I can take? Paul, you can just take it and resend it with any previous patches that are needed as well. This patch series was based on top of Robert Baldyga : usb: dwc2/gadget: fix series - 12 patches. It looks like that series is not in Greg's tree yet. Let me know if you need me to rework that series without Robert Baldyga's patches. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 03/12] usb: dwc2: Update the gadget driver to use common dwc2_hsotg structure
On 09/09/2014 02:06 PM, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dingu...@opensource.altera.com] Sent: Tuesday, September 09, 2014 9:26 AM On 09/08/2014 05:50 PM, Greg KH wrote: On Tue, Aug 26, 2014 at 11:19:54AM -0500, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Adds the gadget data structure and appropriate data structure pointers to the common dwc2_hsotg data structure. To keep the driver data dereference code looking clean, the gadget variable declares are only available for peripheral and dual-role mode. This is needed so that the dwc2_hsotg data structure can be used by the hcd and gadget drivers. Updates gadget.c to use the dwc2_hsotg data structure and gadget pointers that have been moved into the common dwc2_hsotg structure. Along with the updating the gadget driver to use the common dwc2_hsotg structure, a few other things are required in order for this patch to build properly. Those are: - Remove gadget module defines. Since the driver probing will be handled by either the platform or pci code. - Change the gadget probe function into gadget_init. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Signed-off-by: Paul Zimmerman pa...@synopsys.com --- v3: Updated with paulz's suggestion to avoid double pointers. v2: Left the function parameter name as 'hsotg' and just changed its type. --- drivers/usb/dwc2/core.h | 176 +--- drivers/usb/dwc2/gadget.c | 226 +- drivers/usb/dwc2/hcd.h| 10 -- 3 files changed, 184 insertions(+), 228 deletions(-) This doesn't apply as I couldn't take the patches you depended on here :( Can you work with Paul to get this into a series that I can take? Paul, you can just take it and resend it with any previous patches that are needed as well. This patch series was based on top of Robert Baldyga : usb: dwc2/gadget: fix series - 12 patches. It looks like that series is not in Greg's tree yet. Let me know if you need me to rework that series without Robert Baldyga's patches. Robert is currently respinning that series, so I suggest you wait for that to be applied, and then rebase on top of it. Will do... BTW..Did you get a chance to review patches 9/12 and 12/12 of this series regarding the spinlock placement? thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 03/12] usb: dwc2: Update the gadget driver to use common dwc2_hsotg structure
On 09/09/2014 02:56 PM, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dingu...@opensource.altera.com] Sent: Tuesday, September 09, 2014 12:13 PM On 09/09/2014 02:06 PM, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dingu...@opensource.altera.com] Sent: Tuesday, September 09, 2014 9:26 AM On 09/08/2014 05:50 PM, Greg KH wrote: On Tue, Aug 26, 2014 at 11:19:54AM -0500, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com Adds the gadget data structure and appropriate data structure pointers to the common dwc2_hsotg data structure. To keep the driver data dereference code looking clean, the gadget variable declares are only available for peripheral and dual-role mode. This is needed so that the dwc2_hsotg data structure can be used by the hcd and gadget drivers. Updates gadget.c to use the dwc2_hsotg data structure and gadget pointers that have been moved into the common dwc2_hsotg structure. Along with the updating the gadget driver to use the common dwc2_hsotg structure, a few other things are required in order for this patch to build properly. Those are: - Remove gadget module defines. Since the driver probing will be handled by either the platform or pci code. - Change the gadget probe function into gadget_init. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Signed-off-by: Paul Zimmerman pa...@synopsys.com --- v3: Updated with paulz's suggestion to avoid double pointers. v2: Left the function parameter name as 'hsotg' and just changed its type. --- drivers/usb/dwc2/core.h | 176 +--- drivers/usb/dwc2/gadget.c | 226 +- drivers/usb/dwc2/hcd.h| 10 -- 3 files changed, 184 insertions(+), 228 deletions(-) This doesn't apply as I couldn't take the patches you depended on here :( Can you work with Paul to get this into a series that I can take? Paul, you can just take it and resend it with any previous patches that are needed as well. This patch series was based on top of Robert Baldyga : usb: dwc2/gadget: fix series - 12 patches. It looks like that series is not in Greg's tree yet. Let me know if you need me to rework that series without Robert Baldyga's patches. Robert is currently respinning that series, so I suggest you wait for that to be applied, and then rebase on top of it. Will do... BTW..Did you get a chance to review patches 9/12 and 12/12 of this series regarding the spinlock placement? Yes, I think I forgot to give my ack for those. But see Felipe's response to 12/12. I think he's right, you should combine those two patches. After that you can add my Acked-by. Thanks. Yes, Felipe's comment for 12/12 makes sense. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 10/13] usb: dwc2: initialize the spin_lock for both host and gadget
On 8/22/14, 3:57 PM, Felipe Balbi wrote: Hi, On Fri, Aug 22, 2014 at 08:52:23PM +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 30, 2014 8:21 AM Move spin_lock_init to common location for both host and gadget. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/hcd.c |1 - drivers/usb/dwc2/platform.c |1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 07a7bcd..c6778d9 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2824,7 +2824,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, hcd-has_tt = 1; - spin_lock_init(hsotg-lock); ((struct wrapper_priv_data *) hcd-hcd_priv)-hsotg = hsotg; hsotg-priv = hcd; diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index eb2a131..4898268 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -197,6 +197,7 @@ static int dwc2_driver_probe(struct platform_device *dev) } platform_set_drvdata(dev, hsotg); + spin_lock_init(hsotg-lock); return retval; } Hi Dinh, I don't have a copy of your v3 patches in my mailbox anymore, so I am replying to the v2 one instead. Are you absolutely sure that no code that takes the spinlock can be called before this point? This is the last line in the probe() function, so I have a hard time believing it is safe to initialize the spinlock this late. In particular, the IRQ has already been attached, and usb_add_gadget_udc() has already been called. So it seems entirely possible that some other entity could try to access the driver before this point. you're right with this comment. request_irq() enables the IRQ line and it could be that we already have a pending event to handle which fires as soon as we enable that IRQ line. Yes, thanks for catching this. I will move the call up in v4. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 07/13] usb: dwc2: Update common interrupt handler to call gadget interrupt handler
On Wed, Jul 30, 2014 at 1:38 PM, Paul Zimmerman paul.zimmer...@synopsys.com wrote: From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of dingu...@altera.com Sent: Wednesday, July 30, 2014 8:21 AM Make dwc2_handle_common_intr call the gadget interrupt function when operating in peripheral mode. Remove the spinlock functions in s3c_hsotg_irq as dwc2_handle_common_intr() already has the spinlocks. Remove duplicate interrupt conditions that was in gadget, as those are handled by dwc2 common interrupt handler. Signed-off-by: Dinh Nguyen dingu...@altera.com --- v2: Keep interrupt handler for host and peripheral modes separate --- drivers/usb/dwc2/core.h |3 +++ drivers/usb/dwc2/core_intr.c |3 +++ drivers/usb/dwc2/gadget.c| 50 -- 3 files changed, 10 insertions(+), 46 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 28541a8..b86673c 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -961,12 +961,15 @@ extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg); extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg); extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2); extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq); +irqreturn_t s3c_hsotg_irq(int irq, void *pw); #else static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {} static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2) { return 0; } static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) { return 0; } +static inline irqreturn_t s3c_hsotg_irq(int irq, void *pw) +{ return IRQ_HANDLED; } #endif #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index c93918b..24d4c0d 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -472,6 +472,9 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev) spin_lock(hsotg-lock); + if (dwc2_is_device_mode(hsotg)) + retval = s3c_hsotg_irq(irq, dev); + Don't you want to release the spinlock and exit the function here if in device mode? Otherwise it continues on and does a bunch of checks for things that should only happen in host mode. This part handles common interrupts for the core that can happen regardless of what mode it's operating in. Things such as disconnect, connect ID change are handle here. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 01/13] usb: dwc2: Update Kconfig to support dual-role
On 8/1/14, 4:41 PM, Dinh Nguyen wrote: On Fri, 2014-08-01 at 20:42 +, Paul Zimmerman wrote: From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of dingu...@altera.com Sent: Wednesday, July 30, 2014 8:21 AM Update DWC2 kconfig and makefile to support dual-role mode. The platform file will always get compiled for the case where the controller is directly connected to the CPU. So for loadable modules, only dwc2.ko is needed. Signed-off-by: Dinh Nguyen dingu...@altera.com --- v2: Remove reference to dwc2_gadget --- drivers/usb/dwc2/Kconfig | 59 + drivers/usb/dwc2/Makefile | 21 2 files changed, 44 insertions(+), 36 deletions(-) diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index f93807b..3d69928 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -1,40 +1,29 @@ config USB_DWC2 - bool DesignWare USB2 DRD Core Support + tristate DesignWare USB2 DRD Core Support depends on USB help Say Y here if your system has a Dual Role Hi-Speed USB controller based on the DesignWare HSOTG IP Core. - For host mode, if you choose to build the driver as dynamically - linked modules, the core module will be called dwc2.ko, the PCI - bus interface module (if you have a PCI bus system) will be - called dwc2_pci.ko, and the platform interface module (for - controllers directly connected to the CPU) will be called - dwc2_platform.ko. For gadget mode, there will be a single - module called dwc2_gadget.ko. - - NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The - host and gadget drivers are still currently separate drivers. - There are plans to merge the dwc2_gadget driver with the dwc2 - host driver in the near future to create a dual-role driver. + If you choose to build the driver as dynamically + linked modules, a single dwc2.ko(regardless of mode of operation) + will get built for both platform IPs and PCI. if USB_DWC2 +choice + bool DWC2 Mode Selection + default USB_DWC2_DUAL_ROLE if (USB USB_GADGET) + default USB_DWC2_HOST if (USB !USB_GADGET) + default USB_DWC2_PERIPHERAL if (!USB USB_GADGET) + config USB_DWC2_HOST - tristate Host only mode + bool Host only mode depends on USB help The Designware USB2.0 high-speed host controller - integrated into many SoCs. - -config USB_DWC2_PLATFORM - bool DWC2 Platform - depends on USB_DWC2_HOST - default USB_DWC2_HOST - help - The Designware USB2.0 platform interface module for - controllers directly connected to the CPU. This is only - used for host mode. + integrated into many SoCs. Select this option if you want the + driver to operate in Host-only mode. config USB_DWC2_PCI bool DWC2 PCI @@ -47,11 +36,29 @@ config USB_DWC2_PCI comment Gadget mode requires USB Gadget support to be enabled config USB_DWC2_PERIPHERAL - tristate Gadget only mode + bool Gadget only mode depends on USB_GADGET help The Designware USB2.0 high-speed gadget controller - integrated into many SoCs. + integrated into many SoCs. Select this option if you want the + driver to operate in Peripheral-only mode. + +config USB_DWC2_DUAL_ROLE + bool Dual Role mode + depends on ((USB=y || USB=USB_DWC2) (USB_GADGET=y)) Hi Dinh, I just noticed that for dual-role mode, you are not allowing USB_GADGET to be modular. Is there a reason for that? If so, please mention it in the commit message. It should also be explained in the help text. Or maybe add another comment line saying Dual-role mode requires USB Gadget = y or something like that. I think it was an oversight on my part and there's not reason why USB_GADGET can't be modular. I went back to look this for v3 and it appears that I need USB_GADGET=y to avoid a build error when building the new driver for Gadget or Dual-role. drivers/built-in.o: In function `dwc2_gadget_init': /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3516: undefined reference to `usb_add_gadget_udc' drivers/built-in.o: In function `s3c_hsotg_remove': /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3543: undefined reference to `usb_del_gadget_udc' /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3549: undefined reference to `usb_gadget_unregister_driver' So just like the DWC3 driver, I need to add a dependency on USB_GADGET=y. I'll add a note to the help text. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 01/13] usb: dwc2: Update Kconfig to support dual-role
On Thu, Aug 7, 2014 at 3:55 PM, Paul Zimmerman paul.zimmer...@synopsys.com wrote: From: Dinh Nguyen [mailto:dinh.li...@gmail.com] Sent: Thursday, August 07, 2014 5:12 AM On 8/1/14, 4:41 PM, Dinh Nguyen wrote: On Fri, 2014-08-01 at 20:42 +, Paul Zimmerman wrote: From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of dingu...@altera.com Sent: Wednesday, July 30, 2014 8:21 AM ... config USB_DWC2_PERIPHERAL - tristate Gadget only mode + bool Gadget only mode depends on USB_GADGET help The Designware USB2.0 high-speed gadget controller - integrated into many SoCs. + integrated into many SoCs. Select this option if you want the + driver to operate in Peripheral-only mode. + +config USB_DWC2_DUAL_ROLE + bool Dual Role mode + depends on ((USB=y || USB=USB_DWC2) (USB_GADGET=y)) Hi Dinh, I just noticed that for dual-role mode, you are not allowing USB_GADGET to be modular. Is there a reason for that? If so, please mention it in the commit message. It should also be explained in the help text. Or maybe add another comment line saying Dual-role mode requires USB Gadget = y or something like that. I think it was an oversight on my part and there's not reason why USB_GADGET can't be modular. I went back to look this for v3 and it appears that I need USB_GADGET=y to avoid a build error when building the new driver for Gadget or Dual-role. drivers/built-in.o: In function `dwc2_gadget_init': /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3516: undefined reference to `usb_add_gadget_udc' drivers/built-in.o: In function `s3c_hsotg_remove': /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3543: undefined reference to `usb_del_gadget_udc' /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3549: undefined reference to `usb_gadget_unregister_driver' I don't see why that shouldn't work. usb_add_gadget_udc is defined in udc-core.c, which gets built if CONFIG_USB_GADGET is set. What Kconfig line did you use when you got this build error? I think it should be: config USB_DWC2_DUAL_ROLE bool Dual Role mode depends on (USB=y || USB=USB_DWC2) (USB_GADGET=y || USB_GADGET=USB_DWC2) Right, I think your original comment was why I had USB_GADGET=y as a dependency for DWC2_DUAL_ROLE. If I replace USB_GADGET=y with just USB_GADGET, and make USB_GADGET a module. This is fine if I build DWC as module, but If I build DWC2 into the kernel, then the driver cannot link those gadget functions in. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 01/13] usb: dwc2: Update Kconfig to support dual-role
On Thu, Aug 7, 2014 at 4:04 PM, Paul Zimmerman paul.zimmer...@synopsys.com wrote: From: Dinh Nguyen [mailto:dinh.li...@gmail.com] Sent: Thursday, August 07, 2014 2:01 PM On Thu, Aug 7, 2014 at 3:55 PM, Paul Zimmerman paul.zimmer...@synopsys.com wrote: From: Dinh Nguyen [mailto:dinh.li...@gmail.com] Sent: Thursday, August 07, 2014 5:12 AM On 8/1/14, 4:41 PM, Dinh Nguyen wrote: On Fri, 2014-08-01 at 20:42 +, Paul Zimmerman wrote: From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of dingu...@altera.com Sent: Wednesday, July 30, 2014 8:21 AM ... config USB_DWC2_PERIPHERAL - tristate Gadget only mode + bool Gadget only mode depends on USB_GADGET help The Designware USB2.0 high-speed gadget controller - integrated into many SoCs. + integrated into many SoCs. Select this option if you want the + driver to operate in Peripheral-only mode. + +config USB_DWC2_DUAL_ROLE + bool Dual Role mode + depends on ((USB=y || USB=USB_DWC2) (USB_GADGET=y)) Hi Dinh, I just noticed that for dual-role mode, you are not allowing USB_GADGET to be modular. Is there a reason for that? If so, please mention it in the commit message. It should also be explained in the help text. Or maybe add another comment line saying Dual-role mode requires USB Gadget = y or something like that. I think it was an oversight on my part and there's not reason why USB_GADGET can't be modular. I went back to look this for v3 and it appears that I need USB_GADGET=y to avoid a build error when building the new driver for Gadget or Dual-role. drivers/built-in.o: In function `dwc2_gadget_init': /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3516: undefined reference to `usb_add_gadget_udc' drivers/built-in.o: In function `s3c_hsotg_remove': /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3543: undefined reference to `usb_del_gadget_udc' /home/dinguyen/linux_dev/linux-socfpga/drivers/usb/dwc2/gadget.c:3549: undefined reference to `usb_gadget_unregister_driver' I don't see why that shouldn't work. usb_add_gadget_udc is defined in udc-core.c, which gets built if CONFIG_USB_GADGET is set. What Kconfig line did you use when you got this build error? I think it should be: config USB_DWC2_DUAL_ROLE bool Dual Role mode depends on (USB=y || USB=USB_DWC2) (USB_GADGET=y || USB_GADGET=USB_DWC2) Right, I think your original comment was why I had USB_GADGET=y as a dependency for DWC2_DUAL_ROLE. If I replace USB_GADGET=y with just USB_GADGET, and make USB_GADGET a module. This is fine if I build DWC as module, but If I build DWC2 into the kernel, then the driver cannot link those gadget functions in. I don't think just USB_GADGET will work. Please try it with the line I showed above. Yes, your suggestion works fine. It's an additional USB_GADGET=USB_DWC2 from what I had in v2. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] usb: dwc2: add 'mode' which based on Kconfig select or dts setting
On Wed, Aug 6, 2014 at 5:24 PM, Doug Anderson diand...@chromium.org wrote: Kever, On Tue, Aug 5, 2014 at 6:01 PM, Kever Yang kever.y...@rock-chips.com wrote: According to the dr_mode, the otg controller can work as device role and host role. Some boards always want to use host mode and some other boards want to use gadget mode. We use the dts setting to set dwc2's mode, rather than fixing it to whatever hardware says. Signed-off-by: Kever Yang kever.y...@rock-chips.com Acked-by: Paul Zimmerman pa...@synopsys.com --- Changes in v4: - From Doug's suggestion: -- remove dr_mode init from Kconfig code -- change the commit meesage Changes in v3: - fix the odd spacing in dwc2_hsotg struct - From Jingoo's suggestion: change the commit message - add dr_mode init from Kconfig Changes in v2: - put spaces around '+' operator - expand the comment for dr_mode - handle dr_mode is USB_DR_MODE_OTG drivers/usb/dwc2/core.c | 18 ++ drivers/usb/dwc2/core.h | 5 + drivers/usb/dwc2/platform.c | 4 3 files changed, 27 insertions(+) I think this patch still makes sense even though we don't have a combined driver yet. Perhaps Paul or Dihn can confirm. I think this looks fine to me. I will rebase my v3 on top of this patch. Dinh We could potentially do something based on KConfig (like you did in patch set #3), but it wouldn't really make sense to do that until after Dihn's work lands. diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index 27d2c9b..738bec2 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -118,6 +118,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg) { u32 greset; int count = 0; + u32 gusbcfg; dev_vdbg(hsotg-dev, %s()\n, __func__); @@ -148,6 +149,23 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg) } } while (greset GRSTCTL_CSFTRST); + if (hsotg-dr_mode == USB_DR_MODE_HOST) { + gusbcfg = readl(hsotg-regs + GUSBCFG); + gusbcfg = ~GUSBCFG_FORCEDEVMODE; + gusbcfg |= GUSBCFG_FORCEHOSTMODE; + writel(gusbcfg, hsotg-regs + GUSBCFG); + } else if (hsotg-dr_mode == USB_DR_MODE_PERIPHERAL) { + gusbcfg = readl(hsotg-regs + GUSBCFG); + gusbcfg = ~GUSBCFG_FORCEHOSTMODE; + gusbcfg |= GUSBCFG_FORCEDEVMODE; + writel(gusbcfg, hsotg-regs + GUSBCFG); + } else if (hsotg-dr_mode == USB_DR_MODE_OTG) { + gusbcfg = readl(hsotg-regs + GUSBCFG); + gusbcfg = ~GUSBCFG_FORCEHOSTMODE; + gusbcfg = ~GUSBCFG_FORCEDEVMODE; + writel(gusbcfg, hsotg-regs + GUSBCFG); I think the third case here won't be too useful until the combined driver, but it shouldn't hurt, right? -Doug -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] usb: dwc2: add 'mode' which based on Kconfig select or dts setting
On 8/6/14, 5:57 PM, Paul Zimmerman wrote: From: diand...@google.com [mailto:diand...@google.com] On Behalf Of Doug Anderson Sent: Wednesday, August 06, 2014 3:25 PM On Tue, Aug 5, 2014 at 6:01 PM, Kever Yang kever.y...@rock-chips.com wrote: According to the dr_mode, the otg controller can work as device role and host role. Some boards always want to use host mode and some other boards want to use gadget mode. We use the dts setting to set dwc2's mode, rather than fixing it to whatever hardware says. Signed-off-by: Kever Yang kever.y...@rock-chips.com Acked-by: Paul Zimmerman pa...@synopsys.com --- Changes in v4: - From Doug's suggestion: -- remove dr_mode init from Kconfig code -- change the commit meesage Changes in v3: - fix the odd spacing in dwc2_hsotg struct - From Jingoo's suggestion: change the commit message - add dr_mode init from Kconfig Changes in v2: - put spaces around '+' operator - expand the comment for dr_mode - handle dr_mode is USB_DR_MODE_OTG drivers/usb/dwc2/core.c | 18 ++ drivers/usb/dwc2/core.h | 5 + drivers/usb/dwc2/platform.c | 4 3 files changed, 27 insertions(+) I think this patch still makes sense even though we don't have a combined driver yet. Perhaps Paul or Dihn can confirm. Yes, it should be fine. We could potentially do something based on KConfig (like you did in patch set #3), but it wouldn't really make sense to do that until after Dihn's work lands. diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index 27d2c9b..738bec2 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -118,6 +118,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg) { u32 greset; int count = 0; + u32 gusbcfg; dev_vdbg(hsotg-dev, %s()\n, __func__); @@ -148,6 +149,23 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg) } } while (greset GRSTCTL_CSFTRST); + if (hsotg-dr_mode == USB_DR_MODE_HOST) { + gusbcfg = readl(hsotg-regs + GUSBCFG); + gusbcfg = ~GUSBCFG_FORCEDEVMODE; + gusbcfg |= GUSBCFG_FORCEHOSTMODE; + writel(gusbcfg, hsotg-regs + GUSBCFG); + } else if (hsotg-dr_mode == USB_DR_MODE_PERIPHERAL) { + gusbcfg = readl(hsotg-regs + GUSBCFG); + gusbcfg = ~GUSBCFG_FORCEHOSTMODE; + gusbcfg |= GUSBCFG_FORCEDEVMODE; + writel(gusbcfg, hsotg-regs + GUSBCFG); + } else if (hsotg-dr_mode == USB_DR_MODE_OTG) { + gusbcfg = readl(hsotg-regs + GUSBCFG); + gusbcfg = ~GUSBCFG_FORCEHOSTMODE; + gusbcfg = ~GUSBCFG_FORCEDEVMODE; + writel(gusbcfg, hsotg-regs + GUSBCFG); I think the third case here won't be too useful until the combined driver, but it shouldn't hurt, right? Right. For the existing drivers, it shouldn't change anything, since nothing currently sets dr_mode. And with Kever's other patches, it forces host mode on the Rockchip platform, which is all that is required for now, if I understand correctly. I managed to test this patch on the SOCFPGA platform. So Tested-by: Dinh Nguyen dingu...@opensource.altera.com Thanks, DInh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2 02/13] usb: dwc2: Moves s3c_hsotg gadget data structure into dwc2_hsotg
On Fri, 2014-08-01 at 20:31 +, Paul Zimmerman wrote: From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Paul Zimmerman Sent: Friday, August 01, 2014 11:49 AM From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 30, 2014 8:21 AM Adds the gadget data structure and appropriate data structure pointers to the common dwc2_hsotg data structure. This is needed so that the dwc2_hsotg data structure can be used by the hcd and gadget drivers. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/core.h |6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 3b4bd4c..ee34ee1 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -604,6 +604,12 @@ struct dwc2_hsotg { struct timer_list wkp_timer; enum dwc2_lx_state lx_state; + /* Gadget structures */ + struct s3c_hsotg *s3c_hsotg; + struct usb_gadget gadget; + struct usb_gadget_driver *driver; + struct s3c_hsotg_ep *eps; + Hi Dinh, After looking at this some more, I'm not really happy with including a pointer to the s3c_hsotg struct inside the dwc2_hsotg struct. It makes the peripheral mode kind of a second class citizen, and requires a bunch of double pointer indirections in gadget.c (hsotg-s3c_hsotg-foo). Plus, when building for peripheral-only mode, there are a lot of unused fields in the dwc2_hsotg struct. So how about something like the below instead? This moves all of the s3c_hsotg struct fields into the dwc2_hsotg struct, and adds ifdefs around the host-only and peripheral-only fields. Doing this should make the diff to gadget.c even smaller, since it eliminates the double indirections. This patch is on top of your series. And I'm only showing the changes to core.h. And here is a patch which actually compiles in all three modes. I am including the full patch, including the changes to gadget.c, this time. Thanks Paul. I agree that having the double pointers looked messy. I like this suggestion. Can I add your Signed-by when I fold your patch into mine? Thanks, Dinh These changes should really be folded into your series, instead of being tacked onto the end like this. I have no way to test this, since I don't have non-PCI hardware that works in peripheral mode. (After this series goes in, I plan to add support for that on PCI.) -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2 01/13] usb: dwc2: Update Kconfig to support dual-role
On Fri, 2014-08-01 at 20:42 +, Paul Zimmerman wrote: From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of dingu...@altera.com Sent: Wednesday, July 30, 2014 8:21 AM Update DWC2 kconfig and makefile to support dual-role mode. The platform file will always get compiled for the case where the controller is directly connected to the CPU. So for loadable modules, only dwc2.ko is needed. Signed-off-by: Dinh Nguyen dingu...@altera.com --- v2: Remove reference to dwc2_gadget --- drivers/usb/dwc2/Kconfig | 59 + drivers/usb/dwc2/Makefile | 21 2 files changed, 44 insertions(+), 36 deletions(-) diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index f93807b..3d69928 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -1,40 +1,29 @@ config USB_DWC2 - bool DesignWare USB2 DRD Core Support + tristate DesignWare USB2 DRD Core Support depends on USB help Say Y here if your system has a Dual Role Hi-Speed USB controller based on the DesignWare HSOTG IP Core. - For host mode, if you choose to build the driver as dynamically - linked modules, the core module will be called dwc2.ko, the PCI - bus interface module (if you have a PCI bus system) will be - called dwc2_pci.ko, and the platform interface module (for - controllers directly connected to the CPU) will be called - dwc2_platform.ko. For gadget mode, there will be a single - module called dwc2_gadget.ko. - - NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The - host and gadget drivers are still currently separate drivers. - There are plans to merge the dwc2_gadget driver with the dwc2 - host driver in the near future to create a dual-role driver. + If you choose to build the driver as dynamically + linked modules, a single dwc2.ko(regardless of mode of operation) + will get built for both platform IPs and PCI. if USB_DWC2 +choice + bool DWC2 Mode Selection + default USB_DWC2_DUAL_ROLE if (USB USB_GADGET) + default USB_DWC2_HOST if (USB !USB_GADGET) + default USB_DWC2_PERIPHERAL if (!USB USB_GADGET) + config USB_DWC2_HOST - tristate Host only mode + bool Host only mode depends on USB help The Designware USB2.0 high-speed host controller - integrated into many SoCs. - -config USB_DWC2_PLATFORM - bool DWC2 Platform - depends on USB_DWC2_HOST - default USB_DWC2_HOST - help - The Designware USB2.0 platform interface module for - controllers directly connected to the CPU. This is only - used for host mode. + integrated into many SoCs. Select this option if you want the + driver to operate in Host-only mode. config USB_DWC2_PCI bool DWC2 PCI @@ -47,11 +36,29 @@ config USB_DWC2_PCI comment Gadget mode requires USB Gadget support to be enabled config USB_DWC2_PERIPHERAL - tristate Gadget only mode + bool Gadget only mode depends on USB_GADGET help The Designware USB2.0 high-speed gadget controller - integrated into many SoCs. + integrated into many SoCs. Select this option if you want the + driver to operate in Peripheral-only mode. + +config USB_DWC2_DUAL_ROLE + bool Dual Role mode + depends on ((USB=y || USB=USB_DWC2) (USB_GADGET=y)) Hi Dinh, I just noticed that for dual-role mode, you are not allowing USB_GADGET to be modular. Is there a reason for that? If so, please mention it in the commit message. It should also be explained in the help text. Or maybe add another comment line saying Dual-role mode requires USB Gadget = y or something like that. I think it was an oversight on my part and there's not reason why USB_GADGET can't be modular. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 02/13] usb: dwc2: Moves s3c_hsotg gadget data structure into dwc2_hsotg
On 7/31/14, 5:00 PM, Paul Zimmerman wrote: From: Paul Zimmerman Sent: Wednesday, July 30, 2014 11:29 AM From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 30, 2014 8:21 AM Adds the gadget data structure and appropriate data structure pointers to the common dwc2_hsotg data structure. This is needed so that the dwc2_hsotg data structure can be used by the hcd and gadget drivers. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/core.h |6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 3b4bd4c..ee34ee1 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -604,6 +604,12 @@ struct dwc2_hsotg { struct timer_list wkp_timer; enum dwc2_lx_state lx_state; + /* Gadget structures */ + struct s3c_hsotg *s3c_hsotg; + struct usb_gadget gadget; + struct usb_gadget_driver *driver; + struct s3c_hsotg_ep *eps; What's up with the funny spacing here? Can you neaten that up, please? Also, you need another patch later in the series, to remove the 'gadget', 'driver', and 'eps' members from the s3c_hsotg struct, once they are no longer used. Or did you do that later and I missed it? Also, I think there are some more members, like 'regs', that are unused at the end of the series, right? I believe so. I'll make sure to address this in V3. And, I think the 'dedicated_fifos' member is the same thing as the 'en_multiple_tx_fifo' member on the host side, so perhaps that could be removed as well? That could be done in a future patch, though. I'll see if it can be removed cleanly in this series. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dwc2: usb/ethernet dongle does not work
Hi Roy, On Mon, 2014-07-28 at 15:05 +0800, Roy wrote: Hi Dinh: Do you think it might be a problem of a un-aligned DMA buffer address? We know that the usbnet driver submits lots of URBs with a 2-byte aligned buffer address. But in Buffer DMA mode, the starting DMA address must be DWORD-aligned, only if turn on Descriptor DMA mode this this 4-byte aligned limit disappear. We met this kind of problem in our platform which powered by a arm based SoC, RK3288, using the DWC_OTG 3.10a usb IP. Yes, I think this is the issue. But even in decscriptor DMA mode the USB/ethernet is only able to get an IP address from DHCP, pings sometime work and when pings work, it is extremely slow. I still cannot mount an NFS using a the adapter. Does it work perfectly for you? Would you care to share a patch? Thanks, Dinh 发自我的 iPad on 7-15-2014,10:39,Dinh Nguyen dinh.li...@gmail.com wrote: Hi Paul, On 6/25/14, 1:24 PM, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dinh.li...@gmail.com] Sent: Wednesday, June 25, 2014 8:52 AM I was wondering if you have ever tested this driver with a USB/ethernet dongle? I'm using the apple usb/ethernet dongle, which is basically just the ASIX AX88xxx Based Ethernet Adapter. From brief debugging, it appears that Bulk and Interrupt endpoint data are getting corrupted during the data transfer. Will continue to debug, but was just curious if you have tested this kind of device? Hi Dinh, Yes, I have tested the driver with a couple of different Ethernet dongles. That was a while ago though, so I don't know if anything has changed lately to break that. I don't remember what brand of Ethernet dongle I used, unfortunately. I managed to debug this a bit further. The ethernet dongles all work fine on my raspberry pi. But on the SOCFPGA platform, which has version 2.93a of the USB IP, the ethernet dongle seems to only work if I turn on Descriptor DMA and uframe_sched=0. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] usb: dwc2: update gadget portion to use common dwc2_hsotg structure
On 07/16/2014 04:26 PM, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 16, 2014 1:33 PM Update gadget.c to use the dwc2_hsotg and new placement of the gadget data structure that has been moved into the common structure. Along with the updating the gadget to use the common dwc2_hsotg data structure, a few other things are required in order for this patch to build properly. Those are: - Add proper externs for functions that need to be shared between host and peripheral drivers. - Remove gadget module defines. Since the driver probing will be handled by either the platform or pci code. - Change the gadget probe function into gadget_init. - Move the allocation of the core_params to a share place for use by both host and gadget. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/core.h | 61 ++- drivers/usb/dwc2/gadget.c | 1238 --- drivers/usb/dwc2/hcd.c |5 - drivers/usb/dwc2/hcd.h | 10 - drivers/usb/dwc2/platform.c |4 + 5 files changed, 649 insertions(+), 669 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 0c6efbf..0fc4e41 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -84,7 +84,7 @@ static const char * const s3c_hsotg_supply_names[] = { */ #define EP0_MPS_LIMIT 64 -struct s3c_hsotg; +struct dwc2_hsotg; struct s3c_hsotg_req; /** @@ -130,7 +130,7 @@ struct s3c_hsotg_req; struct s3c_hsotg_ep { struct usb_ep ep; struct list_headqueue; - struct s3c_hsotg*parent; + struct dwc2_hsotg *parent; struct s3c_hsotg_req*req; struct dentry *debugfs; At some point we should rename s3c_hsotg_ep and s3c_hsotg_req as well. Same for all the s3c_* function names. That could be done as an additional patch. @@ -953,4 +953,61 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg); */ extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg); +/* Gadget defines */ +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE) +extern void s3c_hsotg_irq_enumdone(struct dwc2_hsotg *dwc2); +extern void s3c_hsotg_epint(struct dwc2_hsotg *dwc2, unsigned int idx, + int dir_in); ... diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index f3c56a2..42d1d60 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -38,6 +38,7 @@ #include linux/platform_data/s3c-hsotg.h #include core.h +#include hw.h /* conversion functions */ static inline struct s3c_hsotg_req *our_req(struct usb_request *req) @@ -50,9 +51,9 @@ static inline struct s3c_hsotg_ep *our_ep(struct usb_ep *ep) return container_of(ep, struct s3c_hsotg_ep, ep); } -static inline struct s3c_hsotg *to_hsotg(struct usb_gadget *gadget) +static inline struct dwc2_hsotg *to_hsotg(struct usb_gadget *gadget) { - return container_of(gadget, struct s3c_hsotg, gadget); + return container_of(gadget, struct dwc2_hsotg, gadget); } static inline void __orr32(void __iomem *ptr, u32 val) @@ -65,9 +66,6 @@ static inline void __bic32(void __iomem *ptr, u32 val) writel(readl(ptr) ~val, ptr); } -/* forward decleration of functions */ -static void s3c_hsotg_dump(struct s3c_hsotg *hsotg); - /** * using_dma - return the DMA status of the driver. * @hsotg: The driver state. @@ -97,16 +95,16 @@ static inline bool using_dma(struct s3c_hsotg *hsotg) * @hsotg: The device state * @ints: A bitmask of the interrupts to enable */ -static void s3c_hsotg_en_gsint(struct s3c_hsotg *hsotg, u32 ints) +static void s3c_hsotg_en_gsint(struct dwc2_hsotg *dwc2, u32 ints) { - u32 gsintmsk = readl(hsotg-regs + GINTMSK); + u32 gsintmsk = readl(dwc2-regs + GINTMSK); u32 new_gsintmsk; new_gsintmsk = gsintmsk | ints; if (new_gsintmsk != gsintmsk) { - dev_dbg(hsotg-dev, gsintmsk now 0x%08x\n, new_gsintmsk); - writel(new_gsintmsk, hsotg-regs + GINTMSK); + dev_dbg(dwc2-dev, gsintmsk now 0x%08x\n, new_gsintmsk); + writel(new_gsintmsk, dwc2-regs + GINTMSK); } } As a general comment for the entire patch, if you left the function parameter names as 'hsotg' and just changed their type, it would simplify this entire patch a *lot*. Care to respin this patch like that? The renaming could always be done later in a separate patch, but right now I'm not sure there's a need for it. Yes, I can do that. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] usb: dwc2: Add DTS compatible string to dwc2_of_match_table
On 07/16/2014 04:27 PM, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 16, 2014 1:33 PM Puts back samsung,s3c6400-hsotg into the dwc2_of_match_table[]. Why? This compatible binding was originally in the gadget driver. Don't I want to put it back into the platform binding table? Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] usb: dwc2: Add DTS compatible string to dwc2_of_match_table
On 7/23/14, 1:03 PM, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dinh.li...@gmail.com] Sent: Wednesday, July 23, 2014 8:12 AM On 07/16/2014 04:27 PM, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 16, 2014 1:33 PM Puts back samsung,s3c6400-hsotg into the dwc2_of_match_table[]. Why? This compatible binding was originally in the gadget driver. Don't I want to put it back into the platform binding table? Do you mean you accidently deleted it in one of your previous patches? If so, then yes you should put it back. But your commit message should explain that, so other people know why it is being done. Yes, I had to removed s3c_hsotg_of_ids[] in patch 3/12. I think I had to do this to enable that patch to build. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/12] usb: dwc2: Update Kconfig to support dual-role
On Mon, 2014-07-21 at 21:39 +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 16, 2014 1:33 PM Update DWC2 kconfig and makefile to support dual-role mode. The platform file will always get compiled for the case where the controller is directly connected to the CPU. So for loadable modules, only dwc2.ko is needed. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/Kconfig | 59 ++--- drivers/usb/dwc2/Makefile | 21 2 files changed, 45 insertions(+), 35 deletions(-) diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index f93807b..5f32e62 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -1,40 +1,31 @@ config USB_DWC2 - bool DesignWare USB2 DRD Core Support + tristate DesignWare USB2 DRD Core Support depends on USB help Say Y here if your system has a Dual Role Hi-Speed USB controller based on the DesignWare HSOTG IP Core. - For host mode, if you choose to build the driver as dynamically - linked modules, the core module will be called dwc2.ko, the PCI - bus interface module (if you have a PCI bus system) will be - called dwc2_pci.ko, and the platform interface module (for - controllers directly connected to the CPU) will be called - dwc2_platform.ko. For gadget mode, there will be a single - module called dwc2_gadget.ko. + If you choose to build the driver as dynamically + linked modules, a single dwc2.ko(regardless of mode of operation) + will get built for both platform IPs and PCI. - NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The - host and gadget drivers are still currently separate drivers. - There are plans to merge the dwc2_gadget driver with the dwc2 - host driver in the near future to create a dual-role driver. + NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. Looks like this line is incorrect, dwc2_gadget.ko no longer exists, right? Yes, that is correct. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dwc2: usb/ethernet dongle does not work
Hi Paul, On 6/25/14, 1:24 PM, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dinh.li...@gmail.com] Sent: Wednesday, June 25, 2014 8:52 AM I was wondering if you have ever tested this driver with a USB/ethernet dongle? I'm using the apple usb/ethernet dongle, which is basically just the ASIX AX88xxx Based Ethernet Adapter. From brief debugging, it appears that Bulk and Interrupt endpoint data are getting corrupted during the data transfer. Will continue to debug, but was just curious if you have tested this kind of device? Hi Dinh, Yes, I have tested the driver with a couple of different Ethernet dongles. That was a while ago though, so I don't know if anything has changed lately to break that. I don't remember what brand of Ethernet dongle I used, unfortunately. I managed to debug this a bit further. The ethernet dongles all work fine on my raspberry pi. But on the SOCFPGA platform, which has version 2.93a of the USB IP, the ethernet dongle seems to only work if I turn on Descriptor DMA and uframe_sched=0. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
dwc2: usb/ethernet dongle does not work
Hi Paul, I was wondering if you have ever tested this driver with a USB/ethernet dongle? I'm using the apple usb/ethernet dongle, which is basically just the ASIX AX88xxx Based Ethernet Adapter. From brief debugging, it appears that Bulk and Interrupt endpoint data are getting corrupted during the data transfer. Will continue to debug, but was just curious if you have tested this kind of device? Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RESEND PATCH 00/15] usb: dwc2: Add support for dual-role
On Fri, 2014-06-20 at 23:10 +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Friday, June 20, 2014 8:35 AM From: Dinh Nguyen dingu...@altera.com Apologies for the noise, but I got Paul Zimmerman's address wrong on the first send. Hi, This patch series combines the dwc2 host and gadget driver into a single dual-role driver. I have tested this on the SOCFPGA platform. I compile tested for bcm2835_defconfig and a PCI platform. I split up the patches to make the review a bit easier, but each individual patch can't really stand on it's own. Any comments/testing is greatly appreciated. Hi Dinh, I really appreciate your work on this, and from a quick glance, it looks like this will be acceptable once a few issues are taken care of. However, I am not going to do an in-depth review, or do any testing, until you submit a patch series that can be applied. Why? If I do a review and test of this series, then I will have to ask you to redo it into a form that can be applied. Then I would have to do a full review and test of _that_ series as well, to make sure that nothing got broken in the translation. So you are creating more work for me, yourself, and anyone else who might want to review/test this. Also, please do not post a patch series titled [PATCH] if you know that it should not be applied as-is. At least put an RFC in there so people will know it shouldn't be applied. Yes, apologies. I didn't think this patch series needed an RFC as the the idea of this patch has been floating around for a while. And because this patch series is just a respin of earlier efforts. Each patch can be applied cleanly, but will not compile for different Kconfig options, as I'm finding out from kbuild test robot. I need to rework this series so that each patch will compile and maintain bisectability. Really, you should learn to use git properly, instead of doing a bunch of work without it, and then throwing everything in at the last minute so you can create a patch series. Git should be your friend, not your enemy ;) You'd think that if I knew to add RESEND to my patch, I'd know to add RFC. I've been using git for quite some time now, and this is the 2nd time that called out my git skills. Not sure how this oversight and haste in my part could have been solved by git? Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: dwc2: Option to disable dma descriptor mode
On Tue, 2014-05-06 at 19:39 +0400, Sergei Shtylyov wrote: Hello. On 05/06/2014 02:23 AM, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com Even though the IP supports Descriptor DMA mode, it does not support SPLIT transactions in this mode. Since the driver can get the Descriptor DMA mode support from hardware, the driver in its currently form cannot fully support LS/FS devices connected to a HS Hub when Descriptor DMA mode is enabled in the driver. Thus, we give the option to disable Descriptor DMA from device tree. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/platform.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index eaba547..cb2133c 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -123,6 +123,7 @@ static int dwc2_driver_probe(struct platform_device *dev) struct resource *res; int retval; int irq; + u32 prop; if (usb_disabled()) return -ENODEV; @@ -135,6 +136,8 @@ static int dwc2_driver_probe(struct platform_device *dev) dwc2_set_all_params(defparams, -1); params = defparams; } + if (!of_property_read_u32(dev-dev.of_node, dma-desc-enable, prop)) + defparams.dma_desc_enable = prop; You don't really need a helper local variable here, the variable receiving the property value won't be changed if there was an error retrieving it. Ah yes, you're right. Thanks for the review. Dinh WBR, Sergei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] usb: dwc2: Option to disable dma descriptor mode
On Tue, 2014-05-06 at 18:21 +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Monday, May 05, 2014 3:23 PM Even though the IP supports Descriptor DMA mode, it does not support SPLIT transactions in this mode. Since the driver can get the Descriptor DMA mode support from hardware, the driver in its currently form cannot fully support LS/FS devices connected to a HS Hub when Descriptor DMA mode is enabled in the driver. Thus, we give the option to disable Descriptor DMA from device tree. Hi Dinh, Instead of this patch, how about if we make the driver default to buffer DMA mode instead, and if anyone wants to use descriptor DMA mode, they can add a DT property for that? I don't think anyone will want that, since as you say it doesn't fully support LS/FS devices in that mode. Yes, I think that's a good idea. Let me spin up a patchset to do that. Dinh The driver originally worked like that. I guess the default got changed by one of Matthijs' patches, and I didn't notice it. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCHv5] usb: dwc2: Add function to calculate correct FIFO sizes
On 4/14/14 4:11 PM, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com The dwc2 IP on the SOCFPGA cannot use the default HW configured FIFO sizes. The total FIFO depth as read from GHWCFG3 reports 0x1f80 or 8064 32-bit words. But the GRXFSIZ, GNPTXFSIZ, and HPTXFSIZ register defaults to 0x2000 or 8192 32-bit words. So the driver cannot just use the fifo sizes as read from those registers. For platforms that face the same issue, this commits sets the RX, periodic TX, and non-periodic TX fifo size to those that are recommended v2.93a spec for the DWC2 IP. Implements Method #2 from the Synopsys v2.93a spec for the DWC2. Signed-off-by: Dinh Nguyen dingu...@altera.com Acked-by: Paul Zimmerman pa...@synopsys.com Reviewed-by: Felipe Balbi ba...@ti.com --- v5: - Fix build error around dev_err() v4: - Fix comment style errors - Use comment suggestion from Felipe to clearly commmunicate invalid fifo sizes - Removed useless comment v3: - Address comments from Felipe - Used Method 2 as recommended by Synopsys to high-bandwidth endpoints. v2: - Fix coding style with braces around both if() branches --- drivers/usb/dwc2/core.c | 68 +++ 1 file changed, 68 insertions(+) diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index 1d12988..27d2c9b 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -507,6 +507,72 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg) writel(intmsk, hsotg-regs + GINTMSK); } +/* + * dwc2_calculate_dynamic_fifo() - Calculates the default fifo size + * For system that have a total fifo depth that is smaller than the default + * RX + TX fifo size. + * + * @hsotg: Programming view of DWC_otg controller + */ +static void dwc2_calculate_dynamic_fifo(struct dwc2_hsotg *hsotg) +{ + struct dwc2_core_params *params = hsotg-core_params; + struct dwc2_hw_params *hw = hsotg-hw_params; + u32 rxfsiz, nptxfsiz, ptxfsiz, total_fifo_size; + + total_fifo_size = hw-total_fifo_size; + rxfsiz = params-host_rx_fifo_size; + nptxfsiz = params-host_nperio_tx_fifo_size; + ptxfsiz = params-host_perio_tx_fifo_size; + + /* + * Will use Method 2 defined in the DWC2 spec: minimum FIFO depth + * allocation with support for high bandwidth endpoints. Synopsys + * defines MPS(Max Packet size) for a periodic EP=1024, and for + * non-periodic as 512. + */ + if (total_fifo_size (rxfsiz + nptxfsiz + ptxfsiz)) { + /* + * For Buffer DMA mode/Scatter Gather DMA mode + * 2 * ((Largest Packet size / 4) + 1 + 1) + n + * with n = number of host channel. + * 2 * ((1024/4) + 2) = 516 + */ + rxfsiz = 516 + hw-host_channels; + + /* + * min non-periodic tx fifo depth + * 2 * (largest non-periodic USB packet used / 4) + * 2 * (512/4) = 256 + */ + nptxfsiz = 256; + + /* + * min periodic tx fifo depth + * (largest packet size*MC)/4 + * (1024 * 3)/4 = 768 + */ + ptxfsiz = 768; + + params-host_rx_fifo_size = rxfsiz; + params-host_nperio_tx_fifo_size = nptxfsiz; + params-host_perio_tx_fifo_size = ptxfsiz; + } + + /* + * If the summation of RX, NPTX and PTX fifo sizes is still + * bigger than the total_fifo_size, then we have a problem. + * + * We won't be able to allocate as many endpoints. Right now, + * we're just printing an error message, but ideally this FIFO + * allocation algorithm would be improved in the future. + * + * FIXME improve this FIFO allocation algorithm. + */ + if (unlikely(total_fifo_size (rxfsiz + nptxfsiz + ptxfsiz))) + dev_err(hsotg-dev, invalid fifo sizes\n); +} + static void dwc2_config_fifos(struct dwc2_hsotg *hsotg) { struct dwc2_core_params *params = hsotg-core_params; @@ -515,6 +581,8 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg) if (!params-enable_dynamic_fifo) return; + dwc2_calculate_dynamic_fifo(hsotg); + /* Rx FIFO */ grxfsiz = readl(hsotg-regs + GRXFSIZ); dev_dbg(hsotg-dev, initial grxfsiz=%08x\n, grxfsiz); Hi Greg, If you're ok with this patch, can you please apply this to usb-next? Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 4/4] usb: dwc2: Edit the Kconfig and Makefile to build dwc2_gadget driver
On 04/10/2014 09:02 PM, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Friday, March 14, 2014 11:52 AM From: Dinh Nguyen dingu...@altera.com Modify the dwc2 Kconfig and Makefile so that it will build the dwc2_gadget driver when peripheral only mode is selected. The dwc_platform.ko and dwc2.ko will get built when dynamic linked modules are selected for Host mode. For peripheral mode only dwc2_gadget.ko will get built. NOTE: You can build both host and peripheral as a dynamically linked module, but be aware that if you insmod dwc2_gadget, then rmmod it, then insmod dwc2 and dwc2_platform for host mode, this will not work. As the step to rmmod dwc2_gadget.ko will turn off the clock to the USB IP. The dwc2 host driver currently does not look to turn on a clock yet. A patch to fix that will be coming soon. Hi Dinh, Sorry for the late response to this. The patch series looks OK to me, except I didn't quite like the Kconfig and Makefile changes, so I have redone those a little bit. Once the commit window for 3.15 opens, I will send these patches on to Greg with my and Robert's acked-bys. Thanks Paul! I was waiting for the 3.15 to open and resend. No longer necessary now. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv4 1/4] usb: dwc2: Add defines to support the s3c-hsotg driver
On Tue, 2014-03-11 at 22:42 +, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dinh.li...@gmail.com] Sent: Monday, March 10, 2014 5:52 AM In preparation of combining the dwc2/s3c-hsotg driver in a single DRD driver, the defines in dwc2/hw.h needs to get updated so that the s3c-hsotg driver can use them. Signed-off-by: Dinh Nguyen dingu...@altera.com Tested-by: Jingoo Han jg1@samsung.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Paul Zimmerman pa...@synopsys.com Cc: Felipe Balbi ba...@ti.com Cc: Ben Dooks ben-li...@fluff.org Cc: Matt Porter mpor...@linaro.org Cc: Kukjin Kim kgene@samsung.com Cc: Stephen Warren swar...@wwwdotorg.org Cc: Matthijs Kooijman matth...@stdin.nl Cc: Sachin Kamat sachin.ka...@linaro.org Cc: Robert Baldyga r.bald...@samsung.com --- v4: none v3: - Remove unused DXEPCTL_EPTYPE_SHIFT define v2: - No need to redo the GRXSTS_PKTSTS defines - Add a FIFOSIZE_DEPTH_GET macro Hi Dinh, I tested these patches on top of mainline on my Synopsys FPGA board, and I don't see any functional issues with host mode. Well, I do see issues with webcams and Ethernet adapters, but they happen even without your patches, so they are unrelated to your changes. So: Tested-by: Paul Zimmerman pa...@synopsys.com Thanks. However, I wonder why this patch series does not allow to build both host mode and device mode drivers at the same time? That is a regression from the current kernel, so I think it is unacceptable. And I see no reason why both drivers can't be built at once. The reason why both drivers can't be built the same time is that both drivers are sharing the same compatible string snps,dwc2. If I enable both drivers, then it appears that only the gadget portion gets loaded and the host driver does not. I'm guessing it would make more sense to be able build both only for dynamically loaded modules? Once that is fixed, we should try also to get host mode tested by Stephen Warren (raspberry pi), Matthijs Kooijman (Ralink rt3052), and Federico Vaga (STMicro STA2X11). Those are the only in-tree users of dwc2 host mode that I am aware of. I am having issues with Stephen Warren's email from my altera.com address, so I am sending from my gmail. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4 4/4] usb: dwc2: Edit the Kconfig and Makefile to build dwc2_gadget driver
From: Dinh Nguyen dingu...@altera.com Modify the dwc2 Kconfig and Makefile so that it will build the dwc2_gadget driver when peripheral only mode is selected. The dwc_platform.ko and dwc2.ko will get built then dynamic linked modules are selected for Host mode. For peripheral mode only dwc2_gadget.ko will get built. Signed-off-by: Dinh Nguyen dingu...@altera.com Tested-by: Jingoo Han jg1@samsung.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Paul Zimmerman pa...@synopsys.com Cc: Felipe Balbi ba...@ti.com Cc: Ben Dooks ben-li...@fluff.org Cc: Matt Porter mpor...@linaro.org Cc: Kukjin Kim kgene@samsung.com Cc: Stephen Warren swar...@wwwdotorg.org Cc: Matthijs Kooijman matth...@stdin.nl Cc: Sachin Kamat sachin.ka...@linaro.org Cc: Robert Baldyga r.bald...@samsung.com --- v4: Remove PCI dependency for USB_DWC2_PLATFORM v3: Created v2: none --- drivers/usb/dwc2/Kconfig| 36 +--- drivers/usb/dwc2/Makefile | 27 +++ drivers/usb/gadget/Kconfig | 7 --- drivers/usb/gadget/Makefile | 1 - 4 files changed, 44 insertions(+), 27 deletions(-) diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index be947d6..e85e6e1 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -5,21 +5,43 @@ config USB_DWC2 Say Y or M here if your system has a Dual Role HighSpeed USB controller based on the DesignWare HSOTG IP Core. - If you choose to build this driver as dynamically linked - modules, the core module will be called dwc2.ko, the + For host mode, if you choose to build this driver as dynamically + linked modules, the core module will be called dwc2.ko, the PCI bus interface module (if you have a PCI bus system) will be called dwc2_pci.ko and the platform interface module (for controllers directly connected to the CPU) will be called - dwc2_platform.ko. + dwc2_platform.ko. For gadget, it will be only be dwc2_gadget.ko. - NOTE: This driver at present only implements the Host mode - of the controller. The existing s3c-hsotg driver supports - Peripheral mode, but only for the Samsung S3C platforms. - There are plans to merge the s3c-hsotg driver with this + NOTE: The s3c-hsotg is now renamed as dwc2_gadget. The host + and gadget driver are still currently separate drivers. There + are plans to merge the dwc2_gadget driver with the dwc2 host driver in the near future to create a dual-role driver. if USB_DWC2 +config USB_DWC2_HOST + bool Host only mode + depends on USB=y || USB=USB_DWC2 + default y + help + Select this when you want to use DWC2 in host mode only, + thereby the gadget feature will be regressed. + +config USB_DWC2_PERIPHERAL + depends on (ARM || USB_GADGET=y) !USB_DWC2_HOST + tristate Gadget only mode + help + The Designware USB2.0 high-speed gadget controller + integrated into many SoCs. + +config USB_DWC2_PLATFORM + depends on USB_DWC2_HOST + tristate DWC2 Platform + default y + help + The Designware USB2.0 platform interface module for controllers + directly connected to the CPU. This is only used for Host mode. + config USB_DWC2_DEBUG bool Enable Debugging Messages help diff --git a/drivers/usb/dwc2/Makefile b/drivers/usb/dwc2/Makefile index 11529d3..1509032 100644 --- a/drivers/usb/dwc2/Makefile +++ b/drivers/usb/dwc2/Makefile @@ -1,25 +1,28 @@ ccflags-$(CONFIG_USB_DWC2_DEBUG) += -DDEBUG ccflags-$(CONFIG_USB_DWC2_VERBOSE) += -DVERBOSE_DEBUG +ifeq ($(CONFIG_USB_DWC2_HOST),y) obj-$(CONFIG_USB_DWC2) += dwc2.o - dwc2-y += core.o core_intr.o - -# NOTE: This driver at present only implements the Host mode -# of the controller. The existing s3c-hsotg driver supports -# Peripheral mode, but only for the Samsung S3C platforms. -# There are plans to merge the s3c-hsotg driver with this -# driver in the near future to create a dual-role driver. Once -# that is done, Host mode will become an optional feature that -# is selected with a config option. - dwc2-y += hcd.o hcd_intr.o dwc2-y += hcd_queue.o hcd_ddma.o +endif +obj-$(CONFIG_USB_DWC2_PERIPHERAL) += dwc2_gadget.o +dwc2_gadget-objs += gadget.o + +# NOTE: The previous s3c-hsotg peripheral mode only driver has been moved to +# this location and renamed gadget.c. When building for dynamically linked +# modules, dwc2_gadget.ko will get built for peripheral mode. For host mode, +# the core module will be dwc2.ko, the PCI bus interface module will called +# dwc2_pci.ko and the platform interface module will be called dwc2_platform.ko. +# At present the host and gadget driver will be separate drivers
[PATCHv4 2/4] usb: s3c-hsotg: Move s3c-hsotg into dwc2 folder
From: Dinh Nguyen dingu...@altera.com Moves the s3c-hsotg driver into the dwc2 folder and use the dwc2 defines in hw.h. Renamed the s3c-hsotg.c to gadget.c. Signed-off-by: Dinh Nguyen dingu...@altera.com Tested-by: Jingoo Han jg1@samsung.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Paul Zimmerman pa...@synopsys.com Cc: Felipe Balbi ba...@ti.com Cc: Ben Dooks ben-li...@fluff.org Cc: Matt Porter mpor...@linaro.org Cc: Kukjin Kim kgene@samsung.com Cc: Stephen Warren swar...@wwwdotorg.org Cc: Matthijs Kooijman matth...@stdin.nl Cc: Sachin Kamat sachin.ka...@linaro.org Cc: Robert Baldyga r.bald...@samsung.com --- v4: none v3: - Renamed s3c-hsotg to dwc2/gadget - Remove the edits to the Kconfig and Makefile as that will be in a separate patch for v4. v2: - Fix whitespace damage - Redo s3c_hsotg_handle_rx() to use dwc2 definitions - Use FIFOSIZE_DEPTH_GET --- drivers/usb/{gadget/s3c-hsotg.c = dwc2/gadget.c} | 415 +++--- drivers/usb/gadget/s3c-hsotg.h| 378 2 files changed, 206 insertions(+), 587 deletions(-) rename drivers/usb/{gadget/s3c-hsotg.c = dwc2/gadget.c} (91%) delete mode 100644 drivers/usb/gadget/s3c-hsotg.h diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/dwc2/gadget.c similarity index 91% rename from drivers/usb/gadget/s3c-hsotg.c rename to drivers/usb/dwc2/gadget.c index 1172eae..9bb1ed7 100644 --- a/drivers/usb/gadget/s3c-hsotg.c +++ b/drivers/usb/dwc2/gadget.c @@ -37,7 +37,7 @@ #include linux/usb/phy.h #include linux/platform_data/s3c-hsotg.h -#include s3c-hsotg.h +#include hw.h static const char * const s3c_hsotg_supply_names[] = { vusb_d, /* digital USB supply, 1.2V */ @@ -340,9 +340,8 @@ static void s3c_hsotg_init_fifo(struct s3c_hsotg *hsotg) /* set FIFO sizes to 2048/1024 */ writel(2048, hsotg-regs + GRXFSIZ); - writel(GNPTXFSIZ_NPTxFStAddr(2048) | - GNPTXFSIZ_NPTxFDep(1024), - hsotg-regs + GNPTXFSIZ); + writel((2048 FIFOSIZE_STARTADDR_SHIFT) | + (1024 FIFOSIZE_DEPTH_SHIFT), hsotg-regs + GNPTXFSIZ); /* * arange all the rest of the TX FIFOs, as some versions of this @@ -362,10 +361,10 @@ static void s3c_hsotg_init_fifo(struct s3c_hsotg *hsotg) for (ep = 1; ep = 15; ep++) { val = addr; - val |= size DPTXFSIZn_DPTxFSize_SHIFT; + val |= size FIFOSIZE_DEPTH_SHIFT; addr += size; - writel(val, hsotg-regs + DPTXFSIZn(ep)); + writel(val, hsotg-regs + DPTXFSIZN(ep)); } /* @@ -373,15 +372,15 @@ static void s3c_hsotg_init_fifo(struct s3c_hsotg *hsotg) * all fifos are flushed before continuing */ - writel(GRSTCTL_TxFNum(0x10) | GRSTCTL_TxFFlsh | - GRSTCTL_RxFFlsh, hsotg-regs + GRSTCTL); + writel(GRSTCTL_TXFNUM(0x10) | GRSTCTL_TXFFLSH | + GRSTCTL_RXFFLSH, hsotg-regs + GRSTCTL); /* wait until the fifos are both flushed */ timeout = 100; while (1) { val = readl(hsotg-regs + GRSTCTL); - if ((val (GRSTCTL_TxFFlsh | GRSTCTL_RxFFlsh)) == 0) + if ((val (GRSTCTL_TXFFLSH | GRSTCTL_RXFFLSH)) == 0) break; if (--timeout == 0) { @@ -495,14 +494,14 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg, * how much data is left in the fifo. */ - size_left = DxEPTSIZ_XferSize_GET(epsize); + size_left = DXEPTSIZ_XFERSIZE_GET(epsize); /* * if shared fifo, we cannot write anything until the * previous data has been completely sent. */ if (hs_ep-fifo_load != 0) { - s3c_hsotg_en_gsint(hsotg, GINTSTS_PTxFEmp); + s3c_hsotg_en_gsint(hsotg, GINTSTS_PTXFEMP); return -ENOSPC; } @@ -523,7 +522,7 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg, __func__, can_write); if (can_write = 0) { - s3c_hsotg_en_gsint(hsotg, GINTSTS_PTxFEmp); + s3c_hsotg_en_gsint(hsotg, GINTSTS_PTXFEMP); return -ENOSPC; } } else if (hsotg-dedicated_fifos hs_ep-index != 0) { @@ -532,16 +531,16 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg, can_write = 0x; can_write *= 4; } else { - if (GNPTXSTS_NPTxQSpcAvail_GET(gnptxsts) == 0) { + if (GNPTXSTS_NP_TXQ_SPC_AVAIL_GET(gnptxsts) == 0) { dev_dbg(hsotg-dev, %s: no queue slots available (0x%08x)\n, __func__, gnptxsts); - s3c_hsotg_en_gsint(hsotg
[PATCHv4 3/4] usb: s3c-hsotg: Move s3c-hsotg data structures
From: Dinh Nguyen dingu...@altera.com This patch moves the data structures that are in the s3c-hsotg source into core.h. This is a necessary step towards unifying the s3c-hsotg and dwc2 into a single DRD. Signed-off-by: Dinh Nguyen dingu...@altera.com Tested-by: Jingoo Han jg1@samsung.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Paul Zimmerman pa...@synopsys.com Cc: Felipe Balbi ba...@ti.com Cc: Ben Dooks ben-li...@fluff.org Cc: Matt Porter mpor...@linaro.org Cc: Kukjin Kim kgene@samsung.com Cc: Stephen Warren swar...@wwwdotorg.org Cc: Matthijs Kooijman matth...@stdin.nl Cc: Sachin Kamat sachin.ka...@linaro.org Cc: Robert Baldyga r.bald...@samsung.com --- v4: none v3: none v2: none --- drivers/usb/dwc2/core.h | 182 ++ drivers/usb/dwc2/gadget.c | 180 + 2 files changed, 183 insertions(+), 179 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 648519c..1efd10c 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -37,6 +37,10 @@ #ifndef __DWC2_CORE_H__ #define __DWC2_CORE_H__ +#include linux/phy/phy.h +#include linux/regulator/consumer.h +#include linux/usb/gadget.h +#include linux/usb/otg.h #include linux/usb/phy.h #include hw.h @@ -54,6 +58,184 @@ static inline void do_write(u32 value, void *addr) /* Maximum number of Endpoints/HostChannels */ #define MAX_EPS_CHANNELS 16 +/* s3c-hsotg declarations */ +static const char * const s3c_hsotg_supply_names[] = { + vusb_d, /* digital USB supply, 1.2V */ + vusb_a, /* analog USB supply, 1.1V */ +}; + +/* + * EP0_MPS_LIMIT + * + * Unfortunately there seems to be a limit of the amount of data that can + * be transferred by IN transactions on EP0. This is either 127 bytes or 3 + * packets (which practically means 1 packet and 63 bytes of data) when the + * MPS is set to 64. + * + * This means if we are wanting to move 127 bytes of data, we need to + * split the transactions up, but just doing one packet at a time does + * not work (this may be an implicit DATA0 PID on first packet of the + * transaction) and doing 2 packets is outside the controller's limits. + * + * If we try to lower the MPS size for EP0, then no transfers work properly + * for EP0, and the system will fail basic enumeration. As no cause for this + * has currently been found, we cannot support any large IN transfers for + * EP0. + */ +#define EP0_MPS_LIMIT 64 + +struct s3c_hsotg; +struct s3c_hsotg_req; + +/** + * struct s3c_hsotg_ep - driver endpoint definition. + * @ep: The gadget layer representation of the endpoint. + * @name: The driver generated name for the endpoint. + * @queue: Queue of requests for this endpoint. + * @parent: Reference back to the parent device structure. + * @req: The current request that the endpoint is processing. This is + * used to indicate an request has been loaded onto the endpoint + * and has yet to be completed (maybe due to data move, or simply + * awaiting an ack from the core all the data has been completed). + * @debugfs: File entry for debugfs file for this endpoint. + * @lock: State lock to protect contents of endpoint. + * @dir_in: Set to true if this endpoint is of the IN direction, which + * means that it is sending data to the Host. + * @index: The index for the endpoint registers. + * @mc: Multi Count - number of transactions per microframe + * @interval - Interval for periodic endpoints + * @name: The name array passed to the USB core. + * @halted: Set if the endpoint has been halted. + * @periodic: Set if this is a periodic ep, such as Interrupt + * @isochronous: Set if this is a isochronous ep + * @sent_zlp: Set if we've sent a zero-length packet. + * @total_data: The total number of data bytes done. + * @fifo_size: The size of the FIFO (for periodic IN endpoints) + * @fifo_load: The amount of data loaded into the FIFO (periodic IN) + * @last_load: The offset of data for the last start of request. + * @size_loaded: The last loaded size for DxEPTSIZE for periodic IN + * + * This is the driver's state for each registered enpoint, allowing it + * to keep track of transactions that need doing. Each endpoint has a + * lock to protect the state, to try and avoid using an overall lock + * for the host controller as much as possible. + * + * For periodic IN endpoints, we have fifo_size and fifo_load to try + * and keep track of the amount of data in the periodic FIFO for each + * of these as we don't have a status register that tells us how much + * is in each of them. (note, this may actually be useless information + * as in shared-fifo mode periodic in acts like a single-frame packet + * buffer than a fifo) + */ +struct s3c_hsotg_ep { + struct usb_ep ep; + struct list_headqueue; + struct s3c_hsotg*parent; + struct s3c_hsotg_req*req; + struct dentry
[PATCHv4 1/4] usb: dwc2: Add defines to support the s3c-hsotg driver
From: Dinh Nguyen dingu...@altera.com In preparation of combining the dwc2/s3c-hsotg driver in a single DRD driver, the defines in dwc2/hw.h needs to get updated so that the s3c-hsotg driver can use them. Signed-off-by: Dinh Nguyen dingu...@altera.com Tested-by: Jingoo Han jg1@samsung.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Paul Zimmerman pa...@synopsys.com Cc: Felipe Balbi ba...@ti.com Cc: Ben Dooks ben-li...@fluff.org Cc: Matt Porter mpor...@linaro.org Cc: Kukjin Kim kgene@samsung.com Cc: Stephen Warren swar...@wwwdotorg.org Cc: Matthijs Kooijman matth...@stdin.nl Cc: Sachin Kamat sachin.ka...@linaro.org Cc: Robert Baldyga r.bald...@samsung.com --- v4: none v3: - Remove unused DXEPCTL_EPTYPE_SHIFT define v2: - No need to redo the GRXSTS_PKTSTS defines - Add a FIFOSIZE_DEPTH_GET macro --- drivers/usb/dwc2/hw.h | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h index 9c92a3c..51248b9 100644 --- a/drivers/usb/dwc2/hw.h +++ b/drivers/usb/dwc2/hw.h @@ -109,6 +109,7 @@ #define GUSBCFG_FSINTF (1 5) #define GUSBCFG_ULPI_UTMI_SEL (1 4) #define GUSBCFG_PHYIF16(1 3) +#define GUSBCFG_PHYIF8 (0 3) #define GUSBCFG_TOUTCAL_MASK (0x7 0) #define GUSBCFG_TOUTCAL_SHIFT 0 #define GUSBCFG_TOUTCAL_LIMIT 0x7 @@ -403,6 +404,7 @@ #define FIFOSIZE_DEPTH_SHIFT 16 #define FIFOSIZE_STARTADDR_MASK(0x 0) #define FIFOSIZE_STARTADDR_SHIFT 0 +#define FIFOSIZE_DEPTH_GET(_x) (((_x) 16) 0x) /* Device mode registers */ @@ -519,11 +521,11 @@ #define DXEPCTL_STALL (1 21) #define DXEPCTL_SNP(1 20) #define DXEPCTL_EPTYPE_MASK(0x3 18) -#define DXEPCTL_EPTYPE_SHIFT 18 -#define DXEPCTL_EPTYPE_CONTROL 0 -#define DXEPCTL_EPTYPE_ISO 1 -#define DXEPCTL_EPTYPE_BULK2 -#define DXEPCTL_EPTYPE_INTTERUPT 3 +#define DXEPCTL_EPTYPE_CONTROL (0x0 18) +#define DXEPCTL_EPTYPE_ISO (0x1 18) +#define DXEPCTL_EPTYPE_BULK(0x2 18) +#define DXEPCTL_EPTYPE_INTERRUPT (0x3 18) + #define DXEPCTL_NAKSTS (1 17) #define DXEPCTL_DPID (1 16) #define DXEPCTL_EOFRNUM(1 16) -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Trouble with stephen warren's wwwdotorg email
Hi Stephen, I tried sending you this patch series to swar...@wwwdotorg.org, but it's bouncing. http://thread.gmane.org/gmane.linux.usb.general/104611 Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] usb: dwc2: Add function to calculate correct FIFO sizes
On Tue, 2014-03-04 at 09:18 -0600, Felipe Balbi wrote: On Mon, Mar 03, 2014 at 09:25:13PM -0600, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com The dwc2 IP on the SOCFPGA cannot use the default HW configured FIFO sizes. The total FIFO depth as read from GHWCFG3 reports 0x1f80 or 8064 32-bit words. But the GRXFSIZ, GNPTXFSIZ, and HPTXFSIZ register defaults to 0x2000 or 8192 32-bit words. So the driver cannot just use the fifo sizes as read from those registers. For platforms that face the same issue, this commits sets the RX, periodic TX, and non-periodic TX fifo size to those that are recommended v2.93a spec for the DWC2 IP. Signed-off-by: Dinh Nguyen dingu...@altera.com Acked-by: Paul Zimmerman pa...@synopsys.com --- v2: Fix coding style with braces around both if() branches --- drivers/usb/dwc2/core.c | 41 + 1 file changed, 41 insertions(+) diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index 1d12988..efa7a45 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -507,6 +507,44 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg) writel(intmsk, hsotg-regs + GINTMSK); } +/* + * dwc2_calculate_dynamic_fifo() - Calculates the default fifo size + * For system that have a total fifo depth that is smaller than the default + * RX + TX fifo size. + * + * @hsotg: Programming view of DWC_otg controller + */ +static void dwc2_calculate_dynamic_fifo(struct dwc2_hsotg *hsotg) +{ + struct dwc2_core_params *params = hsotg-core_params; + struct dwc2_hw_params *hw = hsotg-hw_params; + u32 rxfsiz, nptxfsiz, ptxfsiz, total_fifo_size; + + total_fifo_size = hw-total_fifo_size; + rxfsiz = params-host_rx_fifo_size; + nptxfsiz = params-host_nperio_tx_fifo_size; + ptxfsiz = params-host_perio_tx_fifo_size; + + if (total_fifo_size = (rxfsiz + nptxfsiz + ptxfsiz)) { + /* Params are valid, nothing to do */ + return; + } else { + /* min rx fifo size = ((largest packet/4)*2)+2 */ + rxfsiz = (512/4) * 2 + 2; + /* min non-periodic tx fifo depth */ + nptxfsiz = 2 * (512/4); + /* min periodic tx fifo depth */ + ptxfsiz = (512 * 3)/4; + } + + if (total_fifo_size (rxfsiz + nptxfsiz + ptxfsiz)) + dev_err(hsotg-dev, invalid fifo sizes\n); my comments were silently ignored. NAK. I did not mean to ignore your comments, except I did not see any for this patch. http://thread.gmane.org/gmane.linux.usb.general/104157 Except for the checkpatch --strict part. Which I reran with v1 of the patch and it still was clean. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove fifo sizes from dwc2 usb driver from socfpga.dtsi
On 03/03/2014 08:42 PM, Felipe Balbi wrote: Hi, On Mon, Mar 03, 2014 at 05:09:27PM -0600, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com Remove reading the fifo sizes from dts in platform.c Add dwc2_calculate_dynamic_fifo Conflicts: arch/arm/boot/dts/socfpga.dtsi drivers/staging/dwc2/core.c snip diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index 6d001b5..3acb7ee 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -478,6 +478,38 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg) writel(intmsk, hsotg-regs + GINTMSK); } +static void dwc2_calculate_dynamic_fifo(struct dwc2_hsotg *hsotg) +{ + struct dwc2_core_params *params = hsotg-core_params; + struct dwc2_hw_params *hw = hsotg-hw_params; + u32 rxfsiz, nptxfsiz, ptxfsiz, total_fifo_size; + + total_fifo_size = hw-total_fifo_size; + rxfsiz = params-host_rx_fifo_size; + nptxfsiz = params-host_nperio_tx_fifo_size; + ptxfsiz = params-host_perio_tx_fifo_size; + + if (total_fifo_size = (rxfsiz + nptxfsiz + ptxfsiz)) + /* Params are valid, nothing to do */ + return; You really don't need this branch but in any case, if one branch has curly braces, both branches should have it too. Will fix... + else { + /* min rx fifo size = ((largest packet/4)*2)+2 */ + rxfsiz = (((1024/4) + 1 + 1) * 3) + 1; + /* min non-periodic tx fifo depth */ + nptxfsiz = 3 * (1024/4); + /* min periodic tx fifo depth */ + ptxfsiz = ((1024*3)/4) * 3; + } + + if (total_fifo_size (rxfsiz + nptxfsiz + ptxfsiz)) hmm, so in the if condition above you do: if (total_fifo_size = (rxfsiz + nptxfsiz + ptxfsiz)) then you have an else branch following it and the only way for that else branch to be reached is if this other condition is true. I think this code could be rewritten as: if (total_fifo_size (rxfsiz + nptxfsiz + ptxfsiz)) { /* min rx fifo size = ((largest packet / 4) * 2) + 2 */ rxfsiz = (((1024 / 4) + 1 + 1) * 3) + 1; /* min non-periodic tx fifo depth */ nptxfsiz = 3 * (1024 / 4); /* min periodic tx fifo depth */ ptxfsiz = ((1024 * 3) / 4) * 3; } Ok.. will re-write... Also, you could be a bit more verbose on your comments above. One thing I see is that your fifo allocation, apparently, won't work for high-bandwidth ISOC endpoints. Is that correct ? Another thing is that pre-allocating FIFO sizes might not be the best choice. I mean, if you only have two bulk endpoints enabled, you would be better off allocating bigger FIFOs for those endpoints in order to allow for double- or triple-buffering as that would give SW more time to program DMA to read out the data. yes, I am just coding this to the minimum fifo settings. Let me see if the function can be smarter about the fifo allocation. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove fifo sizes from dwc2 usb driver from socfpga.dtsi
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 On 3/3/14 8:42 PM, Felipe Balbi wrote: Hi, On Mon, Mar 03, 2014 at 05:09:27PM -0600, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com Remove reading the fifo sizes from dts in platform.c Add dwc2_calculate_dynamic_fifo Conflicts: arch/arm/boot/dts/socfpga.dtsi drivers/staging/dwc2/core.c if you rebased the patch and got conflicts, when sending the patch upstream, you should remove this section. Here's a few things for you to fix before getting this patch merged: Apologies! But this patch was not the intended patch I wanted to send out. Let me resend the series with the correct 1st patch. Dinh -BEGIN PGP SIGNATURE- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) Comment: GPGTools - https://gpgtools.org Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBCgAGBQJTFUBmAAoJEBmUBAuBoyj0gg4P/jM/VGTJt2Y2cZX1OZroJ7AV l/oj2iP78psVk1YX2v+taprsWEmKNFztdM8b2JwTlkD95QW6JSnm4fmazmrH5ZSG irFuJpbtKydrGRJy+Uf3jMAHuEnpTtfhF9Bj4NPdXt5phMG7zlKfhNquROhPc8GU /+VD2NfaNa5qRtouyxqfr9cwfo42ihc5O53Y/fLbJrIC1f7VpsZ73VAax3mf3var NDdGhTO8QmSBhbI+59PR99JHQ9ZKjQyQ8H8tbmuuB1133T+82sKfW6to+mVPYi3e r7nHTPVf61xNdZUQydG+GRiDZjUzLzkAMyoDiavQxoksD8VHsJPhwwK7rMMHw6Wi zgLoWfrkqY/xtgVy8wpZuoFevL865m2B4floO0dYS8aJBdXKwfAz4fh+oEgJ3WWu VaG3/ocA+V95HGqbeAX3tBYc8Bu/bE39ZSwxzj9mtp0EFon3HJ4wgVPRJMqptmX/ kekgKshp9cUINXYWfaeMgf1Wlf27+THuKMsFwq0KA2c7s1VvIUOEZlgjBQn5sFAj uucD9Ak7ci9GNH2L6nbNUk3xMD+Y+l842eifeyrtxTXsYV/t3Mi8WNJVTgyWU/Eu zOzCD4Fw62OrvFfQARc+ejyR7yiT4Ux736qTyH7bThoNFaf4qkFp4loPGRxeMFxW 7tc0CD0JWcLJ0RlUVtD5 =oTcJ -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 2/3] usb: s3c-hsotg: Move s3c-hsotg into dwc2 folder
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Hi Felipe, On 3/3/14 8:46 PM, Felipe Balbi wrote: On Mon, Mar 03, 2014 at 05:09:28PM -0600, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com Moves the s3c-hsotg driver into the dwc2 folder and use the dwc2 defines in hw.h. The s3c-hostg driver will now be built with a kconfig option under the dwc2 kconfig. USB_DWC2_HOST and USB_S3C_HSOTG are mutually exclusive build options. Signed-off-by: Dinh Nguyen dingu...@altera.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Paul Zimmerman pa...@synopsys.com Cc: Felipe Balbi ba...@ti.com Cc: Ben Dooks ben-li...@fluff.org Cc: Matt Porter mpor...@linaro.org Cc: Kukjin Kim kgene@samsung.com Cc: Stephen Warren swar...@wwwdotorg.org Cc: Matthijs Kooijman matth...@stdin.nl Cc: Jingoo Han jg1@samsung.com Cc: Sachin Kamat sachin.ka...@linaro.org Cc: Robert Baldyga r.bald...@samsung.com --- v2: - Fix whitespace damage - Redo s3c_hsotg_handle_rx() to use dwc2 definitions - Use FIFOSIZE_DEPTH_GET --- drivers/usb/dwc2/Kconfig | 15 ++ drivers/usb/dwc2/Makefile| 15 +- drivers/usb/{gadget = dwc2}/s3c-hsotg.c | 415 +++--- drivers/usb/gadget/Kconfig |7 - drivers/usb/gadget/Makefile |1 - drivers/usb/gadget/s3c-hsotg.h | 378 --- 6 files changed, 231 insertions(+), 600 deletions(-) rename drivers/usb/{gadget = dwc2}/s3c-hsotg.c (91%) delete mode 100644 drivers/usb/gadget/s3c-hsotg.h diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index be947d6..ce14859 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -20,6 +20,21 @@ config USB_DWC2 if USB_DWC2 +config USB_DWC2_HOST +bool Host only mode +depends on USB=y || USB=USB_DWC2 +default y +help + Select this when you want to use DWC2 in host mode only, + thereby the gadget feature will be regressed. + +config USB_S3C_HSOTG while at that, you really want to rename this to USB_DWC2_PERIPHERAL +depends on (ARM || USB_GADGET=y) !USB_DWC2_HOST +tristate Designware/S3C HS/OtG USB Device controller +help + The Designware USB2.0 high-speed gadget controller + integrated into many SoCs. + config USB_DWC2_DEBUG bool Enable Debugging Messages help diff --git a/drivers/usb/dwc2/Makefile b/drivers/usb/dwc2/Makefile index 11529d3..2778e16 100644 --- a/drivers/usb/dwc2/Makefile +++ b/drivers/usb/dwc2/Makefile @@ -1,9 +1,15 @@ ccflags-$(CONFIG_USB_DWC2_DEBUG)+= -DDEBUG ccflags-$(CONFIG_USB_DWC2_VERBOSE)+= -DVERBOSE_DEBUG +ifeq ($(CONFIG_USB_DWC2_HOST),y) obj-$(CONFIG_USB_DWC2)+= dwc2.o - dwc2-y+= core.o core_intr.o +dwc2-y+= hcd.o hcd_intr.o +dwc2-y+= hcd_queue.o hcd_ddma.o +endif +obj-$(CONFIG_USB_S3C_HSOTG)+= s3c_hsotg.o +s3c_hsotg-y+= s3c-hsotg.o also rename the file to gadget.c or peripheral.c. @@ -13,13 +19,12 @@ dwc2-y+= core.o core_intr.o # that is done, Host mode will become an optional feature that # is selected with a config option. -dwc2-y+= hcd.o hcd_intr.o -dwc2-y+= hcd_queue.o hcd_ddma.o - ifneq ($(CONFIG_PCI),) obj-$(CONFIG_USB_DWC2)+= dwc2_pci.o endif -obj-$(CONFIG_USB_DWC2)+= dwc2_platform.o +ifneq ($(CONFIG_USB_DWC2_HOST),) +obj-$(CONFIG_USB_DWC2)+= dwc2_platform.o this is wrong. You actually want to build dwc2_platform.o separately and want to give it its own Kconfig symbol so that you can: obj-$(CONFIG_USB_DWC2_PLATFORM)+= dwc2_platform.o diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/dwc2/s3c-hsotg.c similarity index 91% rename from drivers/usb/gadget/s3c-hsotg.c rename to drivers/usb/dwc2/s3c-hsotg.c index 1172eae..9bb1ed7 100644 --- a/drivers/usb/gadget/s3c-hsotg.c +++ b/drivers/usb/dwc2/s3c-hsotg.c before accepting this, I need lots of Tested-by tags from a considerable % of people involved with s3c-hsotg.c Your comments are noted and appreciated, but let me resend out the series again with the correct 1st patch in the series. Thanks, Dinh -BEGIN PGP SIGNATURE- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) Comment: GPGTools - https://gpgtools.org Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBCgAGBQJTFUC9AAoJEBmUBAuBoyj0Hs4P/0wzy9n4ltU2jLW/1vHrnBSB 9nBoWZ5rAWgEMGWrl0GGUos1bCXtLpUSXIoIIzqofMlPszEtSmBJqoxvz0AQEbtA 68AUvmbbzrjguv2yATZWv/ybgpqyK9vX1nh9Pp6m6onjo4FUSxpm6lVB4oibuFgY 5dP7UjTqsnrFPxHBGTF+JNpMuj85MGuuiHrXBtdzwVhJiYqjtQ/SVBrE7u4W7HPq pq/i2M5lf+MKAJ3hh4qkCtFS8Ci0u+8z+gc1oiqUyP05JJr5GZg+sCnXjSvtDkO6 p9KBgDEjy/ZN0icRH8eES+4EQE6nmcok9sBHIcSXKfbNCOy0tC4iuSNTOA5NgBKf 7uaadi1p4hCw92ViqQMtvsSkaaynqF7iWs+HgWQEDW8fUKMQhkurlHRQDnZA2UzE
Re: [PATCHv2 0/3] usb: dwc2/s3c-hsotg: Move the s3c-hsotg driver into dwc2
On 3/3/14 5:09 PM, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com Hi, This is a shortened version of the v1 patch to combine the dwc2/s3c-hsotg into a single dual-role driver. The series will only move the s3c-hsotg driver into the dwc2 folder, use the defines in the dwc2 hw.h, and removes the s3c-hsotg.h defines. This will make the dual-role combining work a bit easier to review in the future. For now, the dwc2 and s3c-hsotg will be separate drivers. Thanks, Dinh Nguyen (3): usb: dwc2: Add defines to support the s3c-hsotg driver usb: s3c-hsotg: Move s3c-hsotg into dwc2 folder usb: s3c-hsotg: Move s3c-hsotg data structures Apologies, but please disregard this series. I sent out the wrong 1st patch that should go with this series. Dinh drivers/usb/dwc2/Kconfig | 15 + drivers/usb/dwc2/Makefile| 15 +- drivers/usb/dwc2/core.h | 182 + drivers/usb/dwc2/hw.h| 11 +- drivers/usb/{gadget = dwc2}/s3c-hsotg.c | 593 +++--- drivers/usb/gadget/Kconfig |7 - drivers/usb/gadget/Makefile |1 - drivers/usb/gadget/s3c-hsotg.h | 378 --- 8 files changed, 420 insertions(+), 782 deletions(-) rename drivers/usb/{gadget = dwc2}/s3c-hsotg.c (85%) delete mode 100644 drivers/usb/gadget/s3c-hsotg.h --- Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Paul Zimmerman pa...@synopsys.com Cc: Felipe Balbi ba...@ti.com Cc: Ben Dooks ben-li...@fluff.org Cc: Matt Porter mpor...@linaro.org Cc: Kukjin Kim kgene@samsung.com Cc: Stephen Warren swar...@wwwdotorg.org Cc: Matthijs Kooijman matth...@stdin.nl Cc: Jingoo Han jg1@samsung.com Cc: Sachin Kamat sachin.ka...@linaro.org Cc: Robert Baldyga r.bald...@samsung.com -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc2: Add function to calculate correct FIFO sizes
Hi Paul, On 3/3/14 9:14 PM, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Monday, March 03, 2014 2:20 PM From: Dinh Nguyen dingu...@altera.com The dwc2 IP on the SOCFPGA cannot use the default HW configured FIFO sizes. The total FIFO depth as read from GHWCFG3 reports 0x1f80 or 8064 32-bit words. But the GRXFSIZ, GNPTXFSIZ, and HPTXFSIZ register defaults to 0x2000 or 8192 32-bit words. So the driver cannot just use the fifo sizes as read from those registers. For platforms that face the same issue, this commits sets the RX, periodic TX, and non-periodic TX fifo size to those that are recommended v2.93a spec for the DWC2 IP. Signed-off-by: Dinh Nguyen dingu...@altera.com Cc: Paul Zimmerman pa...@synopsys.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org --- drivers/usb/dwc2/core.c | 41 + 1 file changed, 41 insertions(+) diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index 1d12988..e8e394c 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -507,6 +507,44 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg) writel(intmsk, hsotg-regs + GINTMSK); } +/* + * dwc2_calculate_dynamic_fifo() - Calculates the default fifo size + * For system that have a total fifo depth that is smaller than the default + * RX + TX fifo size. + * + * @hsotg: Programming view of DWC_otg controller + */ +static void dwc2_calculate_dynamic_fifo(struct dwc2_hsotg *hsotg) +{ +struct dwc2_core_params *params = hsotg-core_params; +struct dwc2_hw_params *hw = hsotg-hw_params; +u32 rxfsiz, nptxfsiz, ptxfsiz, total_fifo_size; + +total_fifo_size = hw-total_fifo_size; +rxfsiz = params-host_rx_fifo_size; +nptxfsiz = params-host_nperio_tx_fifo_size; +ptxfsiz = params-host_perio_tx_fifo_size; + +if (total_fifo_size = (rxfsiz + nptxfsiz + ptxfsiz)) +/* Params are valid, nothing to do */ +return; +else { Kernel style says both branches of the if() statement should have parentheses here. (Checkpatch doesn't warn about that, I thought it did.) No, checkpatch was clean. Other than that it looks OK. Once you fix the parentheses, you can add my acked-by. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCHv1] usb: dwc2: Combine the dwc2 and s3c_hsotg into a single USB DRD driver.
On 2/11/14 11:56 PM, Jingoo Han wrote: On Wednesday, February 12, 2014 2:34 PM, Stephen Warren wrote: On 02/04/2014 02:45 PM, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com This means that the driver can be in host or peripheral mode when the appropriate connector is used. When an A-cable is plugged in, the driver behaves in host mode, and when a B-cable is used, the driver will be in peripheral mode. Sorry for the slow response. When building ARCH=arm bcm2835_defconfig, I get build errors: Thanks for testing. drivers/built-in.o: In function `dwc2_gadget_init': drivers/usb/dwc2/s3c-hsotg.c:3335: undefined reference to `usb_add_gadget_udc' drivers/built-in.o: In function `s3c_hsotg_remove': drivers/usb/dwc2/s3c-hsotg.c:3358: undefined reference to `usb_del_gadget_udc' drivers/usb/dwc2/s3c-hsotg.c:3364: undefined reference to `usb_gadget_unregister_driver' These errors happen when CONFIG_USB_GADGET=n. 's3c-hsotg.c' supports only gadget mode. In the case of USB_DWC2_HOST mode, CONFIG_USB_GADGET is NOT enabled. I don't know how to solve it. I'm working on v2 of this patch. I think I can make the Kconfig more flexible by selecting USB_GADGET for dual-role or gadget. Or I can make the new driver more independent for the various modes, meaning if the driver is built for HOST mode, it shouldn't be dependent on gadget functions. Dinh Best regards, Jingoo Han -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: dwc2: commit beb7e592bc is causing a disconnected device to not get detected again
On Tue, 2014-02-04 at 02:22 +, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dingu...@altera.com] Sent: Monday, February 03, 2014 3:42 PM On Mon, 2014-02-03 at 23:10 +, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dingu...@altera.com] Sent: Monday, February 03, 2014 2:53 PM While I was testing my patch to combine the dwc2/s3c-hsotg into a DRD driver, I found that after disconnecting a USB HDD from an OTG A-connector, then reconnecting it, the driver would no longer detect the USB device. I was able to track this issue down to this commit: commit beb7e592bcfd750951c47580494f13065f0fd44c Author: Julien DELACOU julien.dela...@st.com Date: Wed Nov 20 17:29:49 2013 +0100 staging: dwc2: add check on dwc2_core_reset return If the GRSTCTL_CSFTRST self-clearing bit never comes back to 0 for any reason, the controller is under reset state and cannot be used. It's preferable to abort initialization in such case. Signed-off-by: Julien Delacou julien.dela...@st.com Acked-by: Paul Zimmerman pa...@synopsys.com Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org Has anyone else seen this issue with the dwc2 driver on 3.14-rc1? Hi Dinh, I haven't seen it. Do either of the HANG messages in dwc2_core_reset() show up in your dmesg when this happens? If so, what happens if you try increasing the timeout values in there? i.e. try changing the two if (++count 50) to if (++count 250) or so. I think it's because of this change: static int dwc2_hs_phy_init(struct dwc2_hsotg *hsotg, bool select_phy) { u32 usbcfg; + int retval = 0; if (!select_phy) - return; + return -ENODEV; My select_phy is NULL. Not sure why, but I'll debug it some more. Hi Dinh, That looks like just a silly error in that patch. Does the attached patch fix it? Hi Paul, Yes, the attached patch does fix it. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] usb: dwc2: Handle the Host Port Interrupt when it occurs in device mode
On Tue, 2014-02-04 at 21:43 +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Tuesday, February 04, 2014 8:11 AM According to the spec for the DWC2 controller, when the PRTINT interrupt fires, the application must clear the appropriate status bit in the Host Port Control and Status register to clear this bit. When disconnecting an A-cable when the dwc2 host driver, the PRTINT fires, but only the GINTSTS_PRTINT status is cleared, no action is done with the HPRT0 register. The HPRT0_ENACHG bit in the HPRT0 must also be poked to correctly clear the GINTSTS_PRTINT interrupt. I am seeing this behavoir on v2.93 of the DWC2 IP. When I disconnect an OTG A-cable adapter, the PRTINT interrupt fires when the DWC2 is in device mode and is never cleared. This patch adds the function to read the HPRT0 register when the PRTINT fires and the dwc2 IP has already transitioned to device mode. This function is only clearing the HPRT0_ENACHG bit for now, but can be modified to handle more. Signed-off-by: Dinh Nguyen dingu...@altera.com Cc: Paul Zimmerman pa...@synopsys.com Cc: Matt Porter mpor...@linaro.org Cc: Matthijs Kooijman matth...@stdin.nl --- v2: only need to call dwc2_handle_usb_port_intr() once --- drivers/usb/dwc2/core_intr.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index bad298a..e037ad5 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -72,6 +72,23 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg) } /** + * dwc2_handle_usb_port_intr - handles OTG PRTINT interrupts. + * When the PRTINT interrupt fires, there are certain status bits in the Host + * Port that needs to get cleared. + * + * @hsotg: Programming view of DWC_otg controller + */ +static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) +{ + u32 hprt0 = readl(hsotg-regs + HPRT0); + + if (hprt0 HPRT0_ENACHG) { + hprt0 |= HPRT0_ENACHG; + writel(hprt0, hsotg-regs + HPRT0); + } +} Hi Dinh, On second thought, I'm not sure it is safe to blindly write to HPRT0 like this. There is the write-1-to-clear HPRT0_ENA bit in there that could cause the port to get disabled by this write. +/** * dwc2_handle_mode_mismatch_intr() - Logs a mode mismatch warning message * * @hsotg: Programming view of DWC_otg controller @@ -572,6 +589,7 @@ irq_retry: if (dwc2_is_device_mode(hsotg)) { dev_dbg(hsotg-dev, --Port interrupt received in Device mode--\n); + dwc2_handle_usb_port_intr(hsotg); gintsts = GINTSTS_PRTINT; writel(gintsts, hsotg-regs + GINTSTS); retval = 1; Also, for consistency the write to GINTSTS should be done inside dwc2_handle_usb_port_intr() like is done for all the other handlers. So, something like the attached alternate patch. Does it also work for you? Yes, the attached patch also works for me. Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCHv1] usb: dwc2: Combine the dwc2 and s3c_hsotg into a single USB DRD driver.
On Wed, 2014-02-05 at 00:42 +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Tuesday, February 04, 2014 1:46 PM From: Dinh Nguyen dingu...@altera.com This means that the driver can be in host or peripheral mode when the appropriate connector is used. When an A-cable is plugged in, the driver behaves in host mode, and when a B-cable is used, the driver will be in peripheral mode. This commit: - Replaces in the defines used in s3c_hsotg.h with the defines used in the dwc2 hw.h defines. - Use the dw2_hsotg as the unified data structure for the host/gadget. - Uses the dwc2 IRQ handler for host/gadget. - A single spinlock. Hi Dinh, Putting all of these changes into a single patch makes them unreviewable as far I am concerned. You need to break this into a series of smaller patches. I would suggest something like this: 1 of n: Make the minimum changes to the dwc2 header files needed to support s3c-hsotg as a standalone driver. 2 of n: Make the spelling changes to s3c-hsotg.c needed to use the dwc2 headers, and move it to the dwc2/ directory. Make the Kconfig and Makefile changes needed for the move. Delete s3c-hsotg.h. 3 of n: Move the struct defines etc. from s3c-hsotg.c to the dwc2 header files. .. of n: Make the changes required to combine the functionality of both drivers into one. Preferably this would also be a series of patches instead of one big one. At each step of the series, both drivers should still compile and work. I agree. My original thought was to also split this patch, but I just didn't know how to split it. This is why I designated as an RFC. I was really looking for feedback as this is the correct way to combine this driver. I was also looking for testing purpose to make sure I did not break anything for the s3c platform. Also, please follow the patch style used on the linux lists. 'git format-patch --cover-letter' should do most of this for you automatically. I did use --cover-letter on this patch series. And you should probably trim the Cc list to something more reasonable. I looked through all the commits for the dwc2 driver for the cc list. I also CC a bunch of the Samsung people as I figured that the biggest impact of the work would affect the s3c folks. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] usb: dwc2: Handle the Host Port Interrupt when it occurs in device mode
On Mon, 2014-02-03 at 21:13 +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Monday, February 03, 2014 9:00 AM Hi Dinh, According to the spec for the DWC2 controller, when the PRTINT interrupt fires, the application must clear the appropriate status bit in the Host Port Control and Status register to clear this bit. When disconnecting an A-cable when the dwc2 host driver, the PRTINT fires, but only the GINTSTS_PRTINT status is cleared, no action is done with the HPRT0 register. The HPRT0_ENACHG bit in the HPRT0 must also be poked to correctly clear the GINTSTS_PRTINT interrupt. I am seeing this behavoir on v2.93 of the DWC2 IP. When I disconnect an OTG A-cable adapter, the PRTINT interrupt fires when the DWC2 is in device mode and is never cleared. This patch adds the function to read the HPRT0 register when the PRTINT fires and the dwc2 IP has already transitioned to device mode. This function is only clearing the HPRT0_ENACHG bit for now, but can be modified to handle more. Signed-off-by: Dinh Nguyen dingu...@altera.com Cc: Paul Zimmerman pa...@synopsys.com Cc: Matt Porter mpor...@linaro.org Cc: Matthijs Kooijman matth...@stdin.nl --- drivers/usb/dwc2/core_intr.c | 20 1 file changed, 20 insertions(+) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index 12dde73..64fee902 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -76,6 +76,24 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg) } /** + * dwc2_handle_usb_port_intr - handles OTG PRTINT interrupts. + * When the PRTINT interrupt fires, there are certain status bits in the Host + * Port that needs to get cleared. + * + * @hsotg: Programming view of DWC_otg controller + */ +static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) +{ + u32 hprt0; + + hprt0 = readl(hsotg-regs + HPRT0); + if (hprt0 HPRT0_ENACHG) { This would be a little cleaner like this: u32 hprt0 = readl(hsotg-regs + HPRT0); if (hprt0 HPRT0_ENACHG) { Ok... + hprt0 |= HPRT0_ENACHG; + writel(hprt0, hsotg-regs + HPRT0); + } +} + +/** * dwc2_handle_mode_mismatch_intr() - Logs a mode mismatch warning message * * @hsotg: Programming view of DWC_otg controller @@ -583,8 +601,10 @@ irq_retry: if (dwc2_is_device_mode(hsotg)) { dev_dbg(hsotg-dev, --Port interrupt received in Device mode--\n); + dwc2_handle_usb_port_intr(hsotg); gintsts = GINTSTS_PRTINT; writel(gintsts, hsotg-regs + GINTSTS); + dwc2_handle_usb_port_intr(hsotg); Why do you have two calls to dwc2_handle_usb_port_intr() here? Does it still work if you remove the first call? I don't see this problem on our internal FPGA platform, so I will just have to take your word that this fixes a problem. If you resubmit the patch with just a single call to dwc2_handle_usb_port_intr(), I will ack it. Yes, sorry for that. It must have been a merge error on my part between my branches. Yes, a single call to dwc2_handle_usb_port_intr() is enough. will send out v2. Thanks, DInh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: dwc2: commit beb7e592bc is causing a disconnected device to not get detected again
On Mon, 2014-02-03 at 23:10 +, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dingu...@altera.com] Sent: Monday, February 03, 2014 2:53 PM While I was testing my patch to combine the dwc2/s3c-hsotg into a DRD driver, I found that after disconnecting a USB HDD from an OTG A-connector, then reconnecting it, the driver would no longer detect the USB device. I was able to track this issue down to this commit: commit beb7e592bcfd750951c47580494f13065f0fd44c Author: Julien DELACOU julien.dela...@st.com Date: Wed Nov 20 17:29:49 2013 +0100 staging: dwc2: add check on dwc2_core_reset return If the GRSTCTL_CSFTRST self-clearing bit never comes back to 0 for any reason, the controller is under reset state and cannot be used. It's preferable to abort initialization in such case. Signed-off-by: Julien Delacou julien.dela...@st.com Acked-by: Paul Zimmerman pa...@synopsys.com Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org Has anyone else seen this issue with the dwc2 driver on 3.14-rc1? Hi Dinh, I haven't seen it. Do either of the HANG messages in dwc2_core_reset() show up in your dmesg when this happens? No, I do not see these messages when I re-insert the OTG adapter. Also, this is my setup. I am using an OTG Mini-A Male to Female A adapter, then I have a USB HDD connected to the Female A adapter. Disconnect/reconnecting the HDD to the adapter works. But if I disconnect the adapter connected to the board, then reconnecting fails. Thanks, Dinh If so, what happens if you try increasing the timeout values in there? i.e. try changing the two if (++count 50) to if (++count 250) or so. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: dwc2: commit beb7e592bc is causing a disconnected device to not get detected again
On Mon, 2014-02-03 at 23:10 +, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dingu...@altera.com] Sent: Monday, February 03, 2014 2:53 PM While I was testing my patch to combine the dwc2/s3c-hsotg into a DRD driver, I found that after disconnecting a USB HDD from an OTG A-connector, then reconnecting it, the driver would no longer detect the USB device. I was able to track this issue down to this commit: commit beb7e592bcfd750951c47580494f13065f0fd44c Author: Julien DELACOU julien.dela...@st.com Date: Wed Nov 20 17:29:49 2013 +0100 staging: dwc2: add check on dwc2_core_reset return If the GRSTCTL_CSFTRST self-clearing bit never comes back to 0 for any reason, the controller is under reset state and cannot be used. It's preferable to abort initialization in such case. Signed-off-by: Julien Delacou julien.dela...@st.com Acked-by: Paul Zimmerman pa...@synopsys.com Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org Has anyone else seen this issue with the dwc2 driver on 3.14-rc1? Hi Dinh, I haven't seen it. Do either of the HANG messages in dwc2_core_reset() show up in your dmesg when this happens? If so, what happens if you try increasing the timeout values in there? i.e. try changing the two if (++count 50) to if (++count 250) or so. I think it's because of this change: static int dwc2_hs_phy_init(struct dwc2_hsotg *hsotg, bool select_phy) { u32 usbcfg; + int retval = 0; if (!select_phy) - return; + return -ENODEV; My select_phy is NULL. Not sure why, but I'll debug it some more. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/2] dwc2/s3c-hsotg: Initial steps to combine the 2 driver
On Tue, 2014-01-14 at 06:21 -0800, Greg KH wrote: On Tue, Jan 14, 2014 at 05:01:00AM -0600, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com Hi, I'm starting work downstream on combining the DWC2 host driver and the s3c-hsotg gadget driver into a dual-role OTG driver. Before I go further, I was hoping to solicit comments on whether or not my initial approach is correct? I know there are plans to combine the 2, so would like to solicit comments/suggestions so that I can also upstream it as well. These 2 patches: * Moves the DWC2 driver out of drivers/staging into drivers/usb/dwc2/ This already happened yesterday in my tree, so you should see this in linux-next by now, no need to do it again :) I see it now. Thanks for the pointer. * Moves the s3c-hsotg driver into drivers/usb/dwc2/ * Delete s3c-hsotg.h * Make the s3c-hsotg.c file use the defines in hw.h from the DWC2 driver. This initial patch has been tested on the SOCFPGA platform only in Host-only and Gadget-only mode. The next step would be to do the combining of the driver into a dual-role OTG driver. I was told that merging the two of these isn't going to work as the silicon is just too different, which is why I allowed the code to move out of staging. If you feel differently, and think you can combine the two drivers, that's wonderful, I'll gladly take patches to do so, but be sure to test on the proper platforms to make sure nothing breaks. I wasn't aware of the silicon differences. I just took the s3c-hsotg driver as is and it worked fine on my version 2.93a of the USB IP. I'll search the ML for information, or perhaps Paul can comment? Thanks, Dinh thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/2] dwc2/s3c-hsotg: Initial steps to combine the 2 driver
On Tue, 2014-01-14 at 13:14 -0800, Greg KH wrote: On Tue, Jan 14, 2014 at 08:57:12PM +, Paul Zimmerman wrote: From: Dinh Nguyen [mailto:dingu...@altera.com] Sent: Tuesday, January 14, 2014 12:46 PM On Tue, 2014-01-14 at 06:21 -0800, Greg KH wrote: On Tue, Jan 14, 2014 at 05:01:00AM -0600, dingu...@altera.com wrote: From: Dinh Nguyen dingu...@altera.com Hi, I'm starting work downstream on combining the DWC2 host driver and the s3c-hsotg gadget driver into a dual-role OTG driver. Before I go further, I was hoping to solicit comments on whether or not my initial approach is correct? I know there are plans to combine the 2, so would like to solicit comments/suggestions so that I can also upstream it as well. These 2 patches: * Moves the DWC2 driver out of drivers/staging into drivers/usb/dwc2/ This already happened yesterday in my tree, so you should see this in linux-next by now, no need to do it again :) I see it now. Thanks for the pointer. * Moves the s3c-hsotg driver into drivers/usb/dwc2/ * Delete s3c-hsotg.h * Make the s3c-hsotg.c file use the defines in hw.h from the DWC2 driver. This initial patch has been tested on the SOCFPGA platform only in Host-only and Gadget-only mode. The next step would be to do the combining of the driver into a dual-role OTG driver. I was told that merging the two of these isn't going to work as the silicon is just too different, which is why I allowed the code to move out of staging. If you feel differently, and think you can combine the two drivers, that's wonderful, I'll gladly take patches to do so, but be sure to test on the proper platforms to make sure nothing breaks. I wasn't aware of the silicon differences. I just took the s3c-hsotg driver as is and it worked fine on my version 2.93a of the USB IP. I'll search the ML for information, or perhaps Paul can comment? I think Greg is thinking of the octeon-usb driver in staging [1], not the s3c-hsotg driver. The plan was always to eventually merge dwc2 with s3c-hsotg. Yes, I'm totally confused, you are right. Nevermind then, Dinh, if you want to redo your patch after 3.14-rc1 is out, that would be great as merging the drivers together can be done easier after that development point. Unless, Paul has already started the work, I can take a crack at it. Dinh thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/9] usb: gadget: s3c-hsotg: enable build for other platforms
Hi Matt, On 12/12/13 7:26 AM, Matt Porter wrote: Remove unused Samsung-specific machine include and Kconfig dependency on S3C. Signed-off-by: Matt Porter mpor...@linaro.org Reviewed-by: Markus Mayer markus.ma...@linaro.org Reviewed-by: Tim Kryger tim.kry...@linaro.org --- drivers/usb/gadget/Kconfig | 7 +++ drivers/usb/gadget/s3c-hsotg.c | 2 -- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index a91e642..970bd1a 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -294,11 +294,10 @@ config USB_PXA27X gadget drivers to also be dynamically linked. config USB_S3C_HSOTG - tristate S3C HS/OtG USB Device controller - depends on S3C_DEV_USB_HSOTG + tristate Designware/S3C HS/OtG USB Device controller help - The Samsung S3C64XX USB2.0 high-speed gadget controller - integrated into the S3C64XX series SoC. + The Designware USB2.0 high-speed gadget controller + integrated into the S3C64XX and BCM281xx series SoC. Thanks for doing this. The SOCFPGA platform is also using this driver. So perhaps a more generic message here? Dinh config USB_S3C2410 tristate S3C2410 USB Device Controller diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c index 33eb690..8ceb5ef 100644 --- a/drivers/usb/gadget/s3c-hsotg.c +++ b/drivers/usb/gadget/s3c-hsotg.c @@ -37,8 +37,6 @@ #include linux/usb/phy.h #include linux/platform_data/s3c-hsotg.h -#include mach/map.h - #include s3c-hsotg.h static const char * const s3c_hsotg_supply_names[] = { -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/9] usb: gadget: s3c-hsotg: enable build for other platforms
On 12/12/13 4:07 PM, Matt Porter wrote: On Thu, Dec 12, 2013 at 03:51:31PM -0600, Dinh Nguyen wrote: Hi Matt, On 12/12/13 7:26 AM, Matt Porter wrote: Remove unused Samsung-specific machine include and Kconfig dependency on S3C. Signed-off-by: Matt Porter mpor...@linaro.org Reviewed-by: Markus Mayer markus.ma...@linaro.org Reviewed-by: Tim Kryger tim.kry...@linaro.org --- drivers/usb/gadget/Kconfig | 7 +++ drivers/usb/gadget/s3c-hsotg.c | 2 -- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index a91e642..970bd1a 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -294,11 +294,10 @@ config USB_PXA27X gadget drivers to also be dynamically linked. config USB_S3C_HSOTG - tristate S3C HS/OtG USB Device controller - depends on S3C_DEV_USB_HSOTG + tristate Designware/S3C HS/OtG USB Device controller help - The Samsung S3C64XX USB2.0 high-speed gadget controller - integrated into the S3C64XX series SoC. + The Designware USB2.0 high-speed gadget controller + integrated into the S3C64XX and BCM281xx series SoC. Thanks for doing this. The SOCFPGA platform is also using this driver. So perhaps a more generic message here? Ok, I wasn't aware since I didn't see anybody else try to fix this driver in mainline. How about this? The Designware USB2.0 high-speed gadget controller integrated into many SoCS. That looks fine to me. Yeah, I didn't realize the s3c driver was just the Synopsys DWC2 peripheral driver until I saw drivers/staging/dwc2. I was in process of switching over to use it and then I saw your patchset. Thanks, Dinh If that looks better to you I'll plan to add it in an update. I would like to see if we can gather a couple acks at this point or if there are any additional DT comments before sending another update. -Matt config USB_S3C2410 tristate S3C2410 USB Device Controller diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c index 33eb690..8ceb5ef 100644 --- a/drivers/usb/gadget/s3c-hsotg.c +++ b/drivers/usb/gadget/s3c-hsotg.c @@ -37,8 +37,6 @@ #include linux/usb/phy.h #include linux/platform_data/s3c-hsotg.h -#include mach/map.h - #include s3c-hsotg.h static const char * const s3c_hsotg_supply_names[] = { -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels
Hi Dan, On 10/1/13 3:23 AM, Dan Carpenter wrote: On Tue, Oct 01, 2013 at 09:51:07AM +0200, Matthijs Kooijman wrote: On Tue, Oct 01, 2013 at 10:05:17AM +0300, Dan Carpenter wrote: On Tue, Oct 01, 2013 at 01:21:28AM +, Paul Zimmerman wrote: From: Dan Carpenter [mailto:dan.carpen...@oracle.com] Sent: Monday, September 30, 2013 6:09 PM Yeah. I guess it's fine... I was going to suggest adding the + 1 in a different place but actually it doesn't matter. The key to understanding dwc2_set_param_host_channels() is that the val parameter is always -1. That means it always returns -EINVAL and the caller jumbles the error code in with some other error codes and then ignores any errors. :/ The intent of this was that the value can be overridden by the platform code if required, in which case val would not be -1. However, to my knowledge, no in-tree platforms do that, so I guess it would be fine to redo this as you suggest below. We can always add it back if needed. If we redo the dwc2_set_param_host_channels function, we should probably redo _all_ of the dwc2_set_param_* functions, since they all do this. Yeah... Generally one of the early steps in staging drivers is to delete as much code as possible. The rule in the kernel is to not include any functionality which doesn't have a user. Why are we even adding one to the number of channels that the hardware reports? Because that's how the hardware register is defined. I presume it never makes sense to have 0 host channels, this allows a range of 1-16 instead of 0-15. In any case, for this particular problem, I would think that simply making the host_channels field in dcw2_hw_params bigger is the better solution here. E.g., something like: struct dwc2_hw_params { ... -unsigned host_channels:4; +unsigned host_channels:5; } Moving the +1 calculation reverts the code back to what it was before one of my patches. I moved the +1 to this place, so that the off-by-one detail of the hardware register is only known at the place where hardware registers are read. All other code can simply assume that the host_channels variable contains just that: the number of host channels present. However, in that patch I apparently chose the wrong size for the host_channels field, which I think should be problem to fix here. Somehow I assumed that was fixed by the hardware, but I see now that you are right. Yes, making the definition larger is better than moving the + 1. This was my original fix to the problem, but I thought that it would be confusing when reading the code. I also thought about the +1 for host_channels was strange too. For debug outputs, it would be more accurate to display 16 channels, in code-wise, I see that host_channels is used in 2 for loops. Does it make sense to just fix the for loops to include channels 0-15? Dinh, do you want to do that? The other option is that Matthijs could fix it and give you the Reported-by credit. I'm fine with that, if Matthijs wants to submit the fix. I can test it on my hardware too. Dinh regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels
Hi Matthijs, On 10/1/13 6:08 AM, Matthijs Kooijman wrote: Hi Dinh, Somehow I assumed that was fixed by the hardware, but I see now that you are right. Yes, making the definition larger is better than moving the + 1. This was my original fix to the problem, but I thought that it would be confusing when reading the code. I also thought about the +1 for host_channels was strange too. For debug outputs, it would be more accurate to display 16 channels, in code-wise, I see that host_channels is used in 2 for loops. Does it make sense to just fix the for loops to include channels 0-15? I think that fixing this in the places where the value is used is moving the complexity the wrong way. Not sure if I'm understanding you correctly, thoguh. - for (i = 0; i num_channels; i++) { + for (i = 0; i = num_channels; i++) { This way you can keep the host_channels at 4 bits, to more accurately represent the hardware. Dinh, do you want to do that? The other option is that Matthijs could fix it and give you the Reported-by credit. I'm fine with that, if Matthijs wants to submit the fix. I can test it on my hardware too. I'll prepare a patch in a few hours. Would be great if you could test, since my hardware only has a meagre 4 host channels ;-) Sure... Dinh Gr. Matthijs -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels
Hi Matthijs, On 10/1/13 6:08 AM, Matthijs Kooijman wrote: Hi Dinh, Somehow I assumed that was fixed by the hardware, but I see now that you are right. Yes, making the definition larger is better than moving the + 1. This was my original fix to the problem, but I thought that it would be confusing when reading the code. I also thought about the +1 for host_channels was strange too. For debug outputs, it would be more accurate to display 16 channels, in code-wise, I see that host_channels is used in 2 for loops. Does it make sense to just fix the for loops to include channels 0-15? I think that fixing this in the places where the value is used is moving the complexity the wrong way. Not sure if I'm understanding you correctly, thoguh. [resend]: previous reply didn't include Matthijs - for (i = 0; i num_channels; i++) { + for (i = 0; i = num_channels; i++) { That way, host_channels is correct at 4 bits. Just a thought.. Dinh, do you want to do that? The other option is that Matthijs could fix it and give you the Reported-by credit. I'm fine with that, if Matthijs wants to submit the fix. I can test it on my hardware too. I'll prepare a patch in a few hours. Would be great if you could test, since my hardware only has a meagre 4 host channels ;-) Sure, thanks... Dinh Gr. Matthijs -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels
On 10/1/13 6:29 AM, Dan Carpenter wrote: On Tue, Oct 01, 2013 at 06:22:35AM -0500, Dinh Nguyen wrote: Hi Matthijs, On 10/1/13 6:08 AM, Matthijs Kooijman wrote: Hi Dinh, Somehow I assumed that was fixed by the hardware, but I see now that you are right. Yes, making the definition larger is better than moving the + 1. This was my original fix to the problem, but I thought that it would be confusing when reading the code. I also thought about the +1 for host_channels was strange too. For debug outputs, it would be more accurate to display 16 channels, in code-wise, I see that host_channels is used in 2 for loops. Does it make sense to just fix the for loops to include channels 0-15? I think that fixing this in the places where the value is used is moving the complexity the wrong way. Not sure if I'm understanding you correctly, thoguh. - for (i = 0; i num_channels; i++) { + for (i = 0; i = num_channels; i++) { This way you can keep the host_channels at 4 bits, to more accurately represent the hardware. No. num_channels should be the actual number of channels instead of number of channels - 1. Matthijs's fix is the best. CC: Matthijs Not sure why my mail client leaves Matthijs's contact out of reply-all. Dinh regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
staging: dwc2: Issue with isochronous transfers through a hub?
Hi, Just wondering if anyone tried using a webcam with the DWC2 driver? The webcam works great when plugged in directly to the USB port, but appears to have alot of dropped frames when used with a HS hub. I'll debug it of course, but just wondering if anyone has had a similar experience? Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: dwc2: Make dwc2_hw_params.host_channels large enough
On 10/1/13 4:42 PM, Matthijs Kooijman wrote: The hardware offers a 4-bit register containing the number of host channels. However, the values of these register mean 1-16 host channels, not 0-15. Since the dwc2_hw_params struct stores the actual number of host channels supported instead of the raw register value, it should be 5 bits wide instead of 4. Before this commit, hardware with 16 host channels would overflow the field, making it appear as 0 channels. This bug was introduced in commit 9badec2 (staging: dwc2: interpret all hwcfg and related register at init time). Reported-by: Dinh Nguyen dinh.li...@gmail.com Can you please use: Dinh Nguyen dingu...@altera.com Signed-off-by: Matthijs Kooijman matth...@stdin.nl --- drivers/staging/dwc2/core.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h index f7ba34b..fab718d 100644 --- a/drivers/staging/dwc2/core.h +++ b/drivers/staging/dwc2/core.h @@ -294,7 +294,7 @@ struct dwc2_hw_params { unsigned dev_token_q_depth:5; unsigned max_transfer_size:26; unsigned max_packet_count:11; - unsigned host_channels:4; + unsigned host_channels:5; unsigned hs_phy_type:2; unsigned fs_phy_type:2; unsigned i2c_enable:1; Tested-by: Dinh Nguyen dingu...@altera.com Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html