usb: gadget: Add UDC driver for tegra XUSB device mode controller
PATCH 7/8 usb: gadget: Add UDC driver for tegra XUSB device mode controller
Quoted 43 lines · Nagarjuna Kristam
Quoted 31 lines · Thierry Reding
Quoted 20 lines · Nagarjuna Kristam
This patch adds UDC driver for tegra XUSB 3.0 device mode controller. XUSB device mode controller support SS, HS and FS modes
Based on work by:
Mark Kuo <[email protected]>
Andrew Bresticker <[email protected]>Signed-off-by Nagarjuna Kristam <[email protected]>--- drivers/usb/gadget/udc/Kconfig | 11 + drivers/usb/gadget/udc/Makefile | 1 + drivers/usb/gadget/udc/tegra_xudc.c | 3821 +++++++++++++++++++++++++++++++++++ 3 files changed, 3833 insertions(+) create mode 100644 drivers/usb/gadget/udc/tegra_xudc.cdrivers/usb/gadget/udc/Kconfig +0
-0drivers/usb/gadget/udc/Kconfig config USB_GADGET_XILINX · line 445
Quoted 7 lines
drivers/usb/gadget/udc/Kconfig +4
-0@@ -439,6 +439,17 @@ config USB_GADGET_XILINX dynamically linked module called "udc-xilinx" and force all gadget drivers to also be dynamically linked. +config USB_TEGRA_XUDC + tristate "NVIDIA Superspeed USB 3.0 Device Controller" + depends on ARCH_TEGRA + default n"default n" is the default, so you can drop this line.
Quoted 8 lines
drivers/usb/gadget/udc/Kconfig config USB_GADGET_XILINX · line 450
Quoted 5 lines
drivers/usb/gadget/udc/Kconfig +5
-0+ help + Enables TEGRA USB 3.0 device mode controller driver. + + Say "y" to link the driver statically, or "m" to build a + dynamically linked module called "tegra_xudc" and force all"tegra-xudc", see below.
Quoted 2 lines
all files in udc folder uses _ and hence used _. If policy for tegra overrides folder naming convention, will rename accordingly. Please share your views.
Felipe would have to answer whether there's a strict rule in the gadget subsystem. Let's assume for now that there is and keep the underscore.
Quoted 29 lines
Quoted 22 lines
Quoted 6 lines
drivers/usb/gadget/udc/Kconfig +3
-0+ gadget drivers to also be dynamically linked. + source "drivers/usb/gadget/udc/aspeed-vhub/Kconfig" # +static void tegra_xudc_device_mode_on(struct tegra_xudc *xudc)drivers/usb/gadget/udc/Kconfig config USB_GADGET_XILINX
Quoted 10 lines
drivers/usb/gadget/udc/Kconfig +10
-0+{ + unsigned long flags; + int err; + + spin_lock_irqsave(&xudc->lock, flags); + if (xudc->device_mode) { + spin_unlock_irqrestore(&xudc->lock, flags); + return; + } + spin_unlock_irqrestore(&xudc->lock, flags);I'm not sure I understand what you're trying to achive here. This locking doesn't buy you anything at all. First, you're not protecting a race condition here since you're not modifying
xudc->device_modein the critical section.Quoted 6 lines
Once you unlock here,
xudc->device_modecould be modified while you're executing the rest of the code, exposing you to a potential race condition. So either you already have proper locking in place and can drop the locking from here, or you need to make sure to keep the lock until you are in device mode and setxudc->device_modelater on.
Quoted 4 lines
reason for split
spin_lockusage is to avoid creating dead lock, wherepm_runtime_get_syncalso takes the same lock.
Will re-check and update the logic accordingly, maybe use mutex to syncronizedevice_modeon and off functionality.
As-is, the above doesn't make any sense because you lock, read a variable and immediately unlock again, irrespective of what the value was and without doing anything else in between. Reading the variable is already an atomic operation, meaning you'll read either true or false. There's no potential for any race conditions in the above, so the locking here is not needed.
Quoted 60 lines
Quoted 23 lines
drivers/usb/gadget/udc/Kconfig
Quoted 12 lines
drivers/usb/gadget/udc/Kconfig +12
-0+ + pm_runtime_get_sync(xudc->dev); + + err = phy_power_on(xudc->utmi_phy); + if (err < 0) + dev_err(xudc->dev, "utmi power on failed %d\n", err); + err = phy_power_on(xudc->usb3_phy); + if (err < 0) + dev_err(xudc->dev, "usb3 phy power on failed %d\n", err); + + spin_lock_irqsave(&xudc->lock, flags); + dev_info(xudc->dev, "device mode on\n");
dev_dbg()?drivers/usb/gadget/udc/Kconfig
Quoted 1 line
drivers/usb/gadget/udc/Kconfig +1
-0+ tegra_xusb_padctl_set_vbus_override(xudc->padctl);What exactly does that do here? Could we automatically run this as part of
phy_power_on(xudc->utmi_phy)? Perhaps we need to combine this with aphy_configure()call that can be used to configure the PHY into device mode? Based on the mode,phy_power_on()could then set or clear the VBUS override.Quoted 3 lines
Or I suppose
phy_configure()itself could set or clear the VBUS override if the sequencing is important.As discussed over other patch discussion, we will keep vbus override functionality
Quoted 8 lines
drivers/usb/gadget/udc/Kconfig
Quoted 4 lines
drivers/usb/gadget/udc/Kconfig +4
-0+ + xudc->device_mode = true; + spin_unlock_irqrestore(&xudc->lock, flags); + tegra_phy_xusb_utmi_pad_power_on(xudc->utmi_phy);The way this is used I'm wondering if this shouldn't be part of the
xudc->utmi_phy->power_on() implementation.Will handle as part of
phy_power_onQuoted 10 lines
Quoted 10 lines
drivers/usb/gadget/udc/Kconfig +10
-0+} + +static void tegra_xudc_device_mode_off(struct tegra_xudc *xudc) +{ + bool connected = false; + unsigned long flags; + u32 pls, val; + int err; + + spin_lock_irqsave(&xudc->lock, flags);Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 6 lines
drivers/usb/gadget/udc/Kconfig +6
-0+ if (!xudc->device_mode) { + spin_unlock_irqrestore(&xudc->lock, flags); + return; + } + + dev_info(xudc->dev, "device mode off\n");
dev_dbg()
Quoted 1 line
Marked info to get run time update on mode changes.
Aren't there any other mechanisms to check mode changes? I would expect there to be files in sysfs or debugfs that would allow one to read the mode out of.
As a general rule, only output things that are of significance to the user. In this particular case, it's reasonable for the user to expect a mode change to just work, in which case there's no use in informing the user. If you want to leave this in for debugging, that's fine, but then please turn this into dev_dbg() so that it is only shown when needed, which is while debugging.
Quoted 62 lines
Quoted 21 lines
Quoted 21 lines
drivers/usb/gadget/udc/Kconfig +21
-0+ + connected = !!(xudc_readl(xudc, PORTSC) & PORTSC_CCS); + reinit_completion(&xudc->disconnect_complete); + + tegra_xusb_padctl_clear_vbus_override(xudc->padctl); + + pls = (xudc_readl(xudc, PORTSC) >> PORTSC_PLS_SHIFT) & + PORTSC_PLS_MASK; + + /* Direct link to U0 if disconnected in RESUME or U2. */ + if (xudc->soc->pls_quirk && xudc->gadget.speed == USB_SPEED_SUPER && + (pls == PORTSC_PLS_RESUME || pls == PORTSC_PLS_U2)) { + val = xudc_readl(xudc, PORTPM); + val |= PORTPM_FRWE; + xudc_writel(xudc, val, PORTPM); + + val = xudc_readl(xudc, PORTSC); + val &= ~(PORTSC_CHANGE_MASK | + (PORTSC_PLS_MASK << PORTSC_PLS_SHIFT)); + val |= PORTSC_LWS | (PORTSC_PLS_U0 << PORTSC_PLS_SHIFT); + xudc_writel(xudc, val, PORTSC);Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 4 lines
drivers/usb/gadget/udc/Kconfig +4
-0+ } + + xudc->device_mode = false; + spin_unlock_irqrestore(&xudc->lock, flags);The locking is again somewhat weird here. Technically someone else could jump right in at this point and reconfigure to device mode again. Would that not interfere with the operations below?
As mentioned above, will check possibility to protect this using a mutex instead.
Quoted 19 lines
drivers/usb/gadget/udc/Kconfig
Quoted 1 line
drivers/usb/gadget/udc/Kconfig +1
-0+ tegra_phy_xusb_utmi_pad_power_down(xudc->utmi_phy);Again, if sequencing isn't crucial here, couldn't this be moved into
phy_power_off()?Quoted 14 lines
drivers/usb/gadget/udc/Kconfig +14
-0+ + /* Wait for disconnect event. */ + if (connected) + wait_for_completion(&xudc->disconnect_complete); + + /* Make sure interrupt handler has completed before powergating. */ + synchronize_irq(xudc->irq); + err = phy_power_off(xudc->utmi_phy); + if (err < 0) + dev_err(xudc->dev, "utmi_phy power off failed %d\n", err); + + err = phy_power_off(xudc->usb3_phy); + if (err < 0) + dev_err(xudc->dev, "usb3_phy power off failed %d\n", err);Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 3 lines
drivers/usb/gadget/udc/Kconfig +3
-0+ + pm_runtime_put(xudc->dev); +}Also, you seem to be using
phy_power_on()andphy_power_off()symmetrically withpm_runtime_get()andpm_runtime_put(), so I'm wondering ifphy_power_on()andphy_power_off()can't be moved into the runtime PM callbacks.
Quoted 3 lines
phy_poweris coupled withdevice_modeon/off sequence andruntime_getand put are intermittently called along with gadget driver callbacks also and hence they are separated out
Is there any reason to keep a runtime PM reference if we disable device mode? I'm not sure I understand why PHY and runtime PM are separated here.
If PHY power on/off are called in other places, does it perhaps make sense to call runtime PM get/put in those same places as well? Or are there any reasons to keep XUDC enabled while the PHY is off?
If so, it's fine to keep these separate.
Quoted 50 lines
Quoted 41 lines
Quoted 41 lines
drivers/usb/gadget/udc/Kconfig +41
-0+ +static void tegra_xudc_update_data_role(struct tegra_xudc *xudc) +{ + if (extcon_get_state(xudc->data_role_extcon, EXTCON_USB)) + tegra_xudc_device_mode_on(xudc); + else + tegra_xudc_device_mode_off(xudc); +} + +static void tegra_xudc_data_role_work(struct work_struct *work) +{ + struct tegra_xudc *xudc = container_of(work, struct tegra_xudc, + data_role_work); + + tegra_xudc_update_data_role(xudc); +} + +static int tegra_xudc_data_role_notifier(struct notifier_block *nb, + unsigned long event, void *unused) +{ + struct tegra_xudc *xudc = container_of(nb, struct tegra_xudc, + data_role_nb); + unsigned long flags; + + spin_lock_irqsave(&xudc->lock, flags); + if (!xudc->suspended) + schedule_work(&xudc->data_role_work); + spin_unlock_irqrestore(&xudc->lock, flags); + + return NOTIFY_DONE; +} + +static void tegra_xudc_plc_reset_work(struct work_struct *work) +{ + struct delayed_work *dwork = to_delayed_work(work); + struct tegra_xudc *xudc = container_of(dwork, struct tegra_xudc, + plc_reset_work); + unsigned long flags; + + spin_lock_irqsave(&xudc->lock, flags); + if (xudc->wait_csc) {Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 6 lines
drivers/usb/gadget/udc/Kconfig +6
-0+ u32 pls = (xudc_readl(xudc, PORTSC) >> PORTSC_PLS_SHIFT) & + PORTSC_PLS_MASK; + if (pls == PORTSC_PLS_INACTIVE) { + dev_info(xudc->dev, "PLS = Inactive. Toggle VBUS\n"); + tegra_xusb_padctl_clear_vbus_override(xudc->padctl); + tegra_xusb_padctl_set_vbus_override(xudc->padctl);Isn't this the same as the reset quirk?
Quoted 3 lines
Both plc reset and port reset quirk are triggered on different port status. Even though they need to toggle vbus, conditions on which they do have different conditions and hence separated out
Okay.
Quoted 52 lines
Quoted 11 lines
Quoted 11 lines
drivers/usb/gadget/udc/Kconfig +10
-0+ xudc->wait_csc = false; + } + } + spin_unlock_irqrestore(&xudc->lock, flags); +} + +static void tegra_xudc_port_reset_war_work(struct work_struct *work) +{ + struct delayed_work *dwork = to_delayed_work(work); + struct tegra_xudc *xudc =Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 6 lines
drivers/usb/gadget/udc/Kconfig +6
-0+ container_of(dwork, struct tegra_xudc, port_reset_war_work); + unsigned long flags; + u32 pls; + int ret; + + dev_info(xudc->dev, "port_reset_war_work\n");
dev_dbg(), though you may also want to reword this to be more useful.Will udpate
Quoted 7 lines
drivers/usb/gadget/udc/Kconfig
Quoted 4 lines
drivers/usb/gadget/udc/Kconfig +4
-0+ spin_lock_irqsave(&xudc->lock, flags); + if (xudc->data_role_extcon && + extcon_get_state(xudc->data_role_extcon, EXTCON_USB) && + xudc->wait_for_sec_prc) {This could use a local variable to make the conditional easier to read.
Will udpate
Quoted 6 lines
drivers/usb/gadget/udc/Kconfig
Quoted 3 lines
drivers/usb/gadget/udc/Kconfig +3
-0+ pls = (xudc_readl(xudc, PORTSC) >> PORTSC_PLS_SHIFT) & + PORTSC_PLS_MASK; + dev_info(xudc->dev, "pls = %x\n", pls);
dev_dbg()Will udpate
Quoted 6 lines
drivers/usb/gadget/udc/Kconfig
Quoted 3 lines
drivers/usb/gadget/udc/Kconfig +3
-0+ + if (pls == PORTSC_PLS_DISABLED) { + dev_info(xudc->dev, "toggle vbus\n");
dev_dbg()Will udpate
Quoted 8 lines
drivers/usb/gadget/udc/Kconfig
Quoted 3 lines
drivers/usb/gadget/udc/Kconfig +3
-0+ /* PRC doesn't complete in 100ms, toggle the vbus */ + ret = + tegra_phy_xusb_utmi_port_reset_quirk(xudc->utmi_phy);Would
phy_reset()work in this case? That would have the nice side-effect of making the expression short enough for it all to fit on one line.
Quoted 1 line
As discussed earlier,
phy_resetand port reset need to be separated out
Okay then. Perhaps we can leave out _quirk from the function name to make this somewhat shorter? The way the above is wrapped makes my eyes hurt. =)
[...]
Quoted 23 lines
Quoted 13 lines
Quoted 13 lines
drivers/usb/gadget/udc/Kconfig +13
-0+static int +tegra_xudc_ep_queue(struct usb_ep *usb_ep, struct usb_request *usb_req, + gfp_t gfp) +{ + struct tegra_xudc_request *req; + struct tegra_xudc_ep *ep; + struct tegra_xudc *xudc; + unsigned long flags; + int ret; + + if (!usb_ep || !usb_req) + return -EINVAL; + ep = to_xudc_ep(usb_ep);Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 5 lines
drivers/usb/gadget/udc/Kconfig +5
-0+ req = to_xudc_req(usb_req); + xudc = ep->xudc; + + spin_lock_irqsave(&xudc->lock, flags); + if (xudc->powergated || !ep->desc) {Does this even happen? Shouldn't we make sure that this can never be called when powergated?
Quoted 3 lines
During fast plug and unplug, In the disconnect path,
there can be a race between XUDC entering runtime suspend
and the gadget core resetting (unbinding and re-binding), resulting in this access.
Okay, sounds fine then.
[...]
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
drivers/usb/gadget/udc/Kconfig +10
-0+ ep_reload(xudc, ep->index); + ep_unpause(xudc, ep->index); + ep_unhalt(xudc, ep->index); + + if (usb_endpoint_xfer_isoc(desc)) { + for (i = 0; i < ARRAY_SIZE(xudc->ep); i++) { + if (xudc->ep[i].desc && + usb_endpoint_xfer_bulk(xudc->ep[i].desc)) + ep_unpause(xudc, i); + }Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 6 lines
drivers/usb/gadget/udc/Kconfig +6
-0+ } + +out: + dev_info(xudc->dev, "ep %u (type: %d, dir: %s) enabled\n", ep->index, + usb_endpoint_type(ep->desc), + usb_endpoint_dir_in(ep->desc) ? "in" : "out");
dev_dbg()
Quoted 1 line
Maked info, to get run time udpated on enabled end points
Again, can this information not be obtained elsewhere? The kernel log is a precious tool to quickly see when unexpected things happen. If we keep spamming the log with information that's not useful, we make it more difficult to see the really important bits.
[...]
Quoted 24 lines
Quoted 14 lines
Quoted 14 lines
drivers/usb/gadget/udc/Kconfig +14
-0+static int tegra_xudc_ep0_enable(struct usb_ep *usb_ep, + const struct usb_endpoint_descriptor *desc) +{ + return -EINVAL; +} + +static int tegra_xudc_ep0_disable(struct usb_ep *usb_ep) +{ + return -EINVAL; +} + +static struct usb_ep_ops tegra_xudc_ep0_ops = { + .enable = tegra_xudc_ep0_enable, + .disable = tegra_xudc_ep0_disable,Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 6 lines
drivers/usb/gadget/udc/Kconfig +6
-0+ .alloc_request = tegra_xudc_ep_alloc_request, + .free_request = tegra_xudc_ep_free_request, + .queue = tegra_xudc_ep_queue, + .dequeue = tegra_xudc_ep_dequeue, + .set_halt = tegra_xudc_ep_set_halt, +};What makes EP0 special?
Quoted 2 lines
EP0 is always enabled and hence any calls to enable/disable are not handled hence separate ops for EP0 to reject request if any
-EINVAL sounds suboptimal as an error message in this case. Perhaps something like -EBUSY would be more appropriate?
[...]
Quoted 31 lines
Quoted 21 lines
Quoted 21 lines
drivers/usb/gadget/udc/Kconfig +21
-0+ return err; + } + + err = tegra_xudc_clk_init(xudc); + if (err < 0) { + dev_err(xudc->dev, "failed to init clocks %d\n", err); + return err; + } + + xudc->supplies = devm_kcalloc(&pdev->dev, xudc->soc->num_supplies, + sizeof(*xudc->supplies), GFP_KERNEL); + if (!xudc->supplies) + return -ENOMEM; + for (i = 0; i < xudc->soc->num_supplies; i++) + xudc->supplies[i].supply = xudc->soc->supply_names[i]; + err = devm_regulator_bulk_get(&pdev->dev, xudc->soc->num_supplies, + xudc->supplies); + if (err) { + dev_err(xudc->dev, "failed to request regulators %d\n", err); + return err; + }Quoted 9 lines
drivers/usb/gadget/udc/Kconfig
Quoted 4 lines
drivers/usb/gadget/udc/Kconfig +4
-0+ + xudc->padctl = tegra_xusb_padctl_get(&pdev->dev); + if (IS_ERR(xudc->padctl)) + return PTR_ERR(xudc->padctl);It seems like we only need this for the VBUS override functionality. If we can get rid of that by incorporating it into phy API functions, we could remove the need to get a reference to it here.
Quoted 1 line
As mentioned earlier, we need this for port operations
Okay, let's keep it, then.
[...]
Quoted 16 lines
Quoted 16 lines
drivers/usb/gadget/udc/Kconfig
Quoted 12 lines
drivers/usb/gadget/udc/Kconfig +12
-0+#ifdef CONFIG_PM +static int tegra_xudc_runtime_suspend(struct device *dev) +{ + struct tegra_xudc *xudc = dev_get_drvdata(dev); + unsigned long flags; + + spin_lock_irqsave(&xudc->lock, flags); + if (WARN_ON(xudc->device_mode)) { + spin_unlock_irqrestore(&xudc->lock, flags); + return -EBUSY; + } + spin_unlock_irqrestore(&xudc->lock, flags);This seems overly restrictive. Why reject suspend mode when we're in device mode?
Quoted 6 lines
Quoted 4 lines
Also, it seems to me like there really aren't any differences between system sleep and runtime PM, so why not just leave away the system sleep functions and just let runtime PM deal with everything?
During system sleep we force off the device mode and not in runtime suspend and viceversa during resume, in runtime, its more of powergaring
Come to think of it, why would we even attempt to runtime suspend if we're in device mode? Can't we just make sure that we call pm_runtime_get() and pm_runtime_put() in the right places so that this doesn't happen? Or is this again the race condition that can happen if we quickly plug/unplug?
Thierry