On Thu, Feb 14, 2019 at 08:22:34PM +0530, Nagarjuna Kristam wrote:
>
>
> On 04-02-2019 19:26, Thierry Reding wrote:
> > On Thu, Jan 03, 2019 at 03:34:58PM +0530, Nagarjuna Kristam wrote:
> >> 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 <mkuo@xxxxxxxxxx>
> >> Andrew Bresticker <abrestic@xxxxxxxxxxxx>
> >>
> >> Signed-off-by: Nagarjuna Kristam <nkristam@xxxxxxxxxx>
> >> ---
> >> 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.c
> >>
> >> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> >> index 0a16cbd..31665e7 100644
> >> --- a/drivers/usb/gadget/udc/Kconfig
> >> +++ b/drivers/usb/gadget/udc/Kconfig
> >> @@ -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.
> >
> >> + 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.
> >
>
> 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.
> >> + 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)
> >> +{
> >> + 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_mode in the
> > critical section.
> >
> > Once you unlock here, xudc->device_mode could 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 set xudc->device_mode later on.
> >
>
> reason for split spin_lock usage is to avoid creating dead lock,
> where pm_runtime_get_sync also takes the same lock.
> Will re-check and update the logic accordingly, maybe use mutex to syncronize
> device_mode on 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.
> >> +
> >> + 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()?
> >
> >> + 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 a
> > phy_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.
> >
> > 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
>
> >> +
> >> + 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_on
>
> >> +}
> >> +
> >> +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);
> >> + if (!xudc->device_mode) {
> >> + spin_unlock_irqrestore(&xudc->lock, flags);
> >> + return;
> >> + }
> >> +
> >> + dev_info(xudc->dev, "device mode off\n");
> >
> > dev_dbg()
> >
>
> 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.
> >> +
> >> + 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);
> >> + }
> >> +
> >> + 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.
>
> >> + 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()?
> >
> >> +
> >> + /* 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);
> >> +
> >> + pm_runtime_put(xudc->dev);
> >> +}
> >
> > Also, you seem to be using phy_power_on() and phy_power_off()
> > symmetrically with pm_runtime_get() and pm_runtime_put(), so I'm
> > wondering if phy_power_on() and phy_power_off() can't be moved into the
> > runtime PM callbacks.
> >
>
> phy_power is coupled with device_mode on/off sequence and runtime_get and 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.
> >> +
> >> +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) {
> >> + 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?
> >
> 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.
>
> >> + 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 =
> >> + 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
> >> + 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
> >> + pls = (xudc_readl(xudc, PORTSC) >> PORTSC_PLS_SHIFT) &
> >> + PORTSC_PLS_MASK;
> >> + dev_info(xudc->dev, "pls = %x\n", pls);
> >
> > dev_dbg()
> >
> Will udpate
> >> +
> >> + if (pls == PORTSC_PLS_DISABLED) {
> >> + dev_info(xudc->dev, "toggle vbus\n");
> >
> > dev_dbg()
> >
> Will udpate
> >> + /* 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.
> >
>
> As discussed earlier, phy_reset and 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. =)
[...]
> >> +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);
> >> + 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?
> >
>
> 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.
[...]
> >> + 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);
> >> + }
> >> + }
> >> +
> >> +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()
> >
>
> 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.
[...]
> >> +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,
> >> + .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?
> >
>
> 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?
[...]
> >> + 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;
> >> + }
> >> +
> >> + 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.
> >
>
> As mentioned earlier, we need this for port operations
Okay, let's keep it, then.
[...]
> >> +#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?
> >
> > 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
Attachment:
signature.asc
Description: PGP signature
