phy: tegra: xusb: t210: add XUSB device mode support
PATCH 1/8 phy: tegra: xusb: t210: add XUSB device mode support
Quoted 18 lines · Nagarjuna Kristam
Hi Thierry,
Quoted 6 lines · Thierry Reding
Quoted 4 lines · Nagarjuna Kristam
Add support for XUSB device mode controller on Tegra210. Update PADCTL driver to set port cap based on DT config. Add code to handle property "nvidia,usb3-port-fake" Provide API's to control vbus override and utmi pad power control
Quoted 8 lines
You essentially list three features additions here, so it makes sense to provide each of them in a separate patch. That helps reviewers concentrate on one feature at a time without having to load context for three different things in parallel.
Also, for each feature please provide a bit more background information as well as reasons for why they are added.
Quoted 7 lines
For XUSB device control Feature support, we needed all three and hence added them as part of one change. If we can have group of individuals CL's for providing one feature support, will split them accordingly. Doing above will mark current changes as a different patch series, considering new patch series will be made, is it fine to move USB gadget driver changes to a new series ?
Please share your inputs
Yes, it's generally fine to rearrange patches in a series or add new ones as the need arises. One thing to consider is to keep the subject of the cover letter intact and use proper versioning of patches. Both of these should help track the series as it evolves.
When splitting up patches, though, make sure that all patches can be successfully built in the order that you send them. This is important in order to be able to run git bisections later on. You can easily verify this by doing something like:
$ git rebase -i --exec '/bin/sh'
once the series is ready. That will automatically drop you into a shell after each commit and you can run your build commands to verify that each commit builds. You can also replace "/bin/sh" with the build command directly to fully automate this process. However you have to make sure that your build command returns proper error codes on failure for this to work.
[...]
Quoted 22 lines
Quoted 22 lines
Quoted 5 lines
(quoted diff) +5
-0+ reg = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index)); + reg |= XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD; + padctl_writel(padctl, reg, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index)); + + reg = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));Review
Quoted 10 lines
(quoted diff) +10
-0+ reg |= XUSB_PADCTL_USB2_OTG_PAD_CTL1_PD_DR; + padctl_writel(padctl, reg, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index)); + + udelay(2); + + tegra210_utmi_bias_pad_power_off(padctl, pad); + usb2->powered_on = false; + + mutex_unlock(&padctl->lock); +}I'm not sure I understand this correctly. This seems to implement some reference counting of its own. Why is that? Is it because it is shared between multiple pads? But then, you're writing to indexed registers, which indicates to me that these are in fact separate pads and not a shared resource. So why the reference counting?
Quoted 8 lines
Quoted 3 lines
I'm also not sure why these functions need to be accessible outside this file. They are not used outside, as far as I can tell.
Lock was added, since the API area called from external context(XUDC driver) and to protect against on/off,
mutex_lockwas done.
Based on later suggestion in the review, will move utmi pad and utmi bias pad power on functions tophy_power_on, with that, mutex lock is not needed any more.
Sounds great!
[...]
Quoted 30 lines
Quoted 21 lines
Quoted 8 lines
(quoted diff) +2
-0@@ -546,9 +546,26 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2) { struct tegra_xusb_port *port = &usb2->base; struct device_node *np = port->dev.of_node; + const char *prop_string; + u32 value; usb2->internal = of_property_read_bool(np, "nvidia,internal");Review static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2) · line 562
Quoted 10 lines
(quoted diff) +9
-0+ usb2->port_cap = USB_PORT_DISABLED; /* default */ + if (!of_property_read_string(np, "mode", &prop_string)) { + if (!strcmp("host", prop_string)) + usb2->port_cap = USB_HOST_CAP; + else if (!strcmp("device", prop_string)) + usb2->port_cap = USB_DEVICE_CAP; + else if (!strcmp("otg", prop_string)) + usb2->port_cap = USB_OTG_CAP; + }This seems to fulfill the same purpose as this:
Quoted 8 lines
http://patchwork.ozlabs.org/patch/1031014/Although, looking more closely at that, we can probably just the
usb_get_dr_mode()helper for this. There's also a standard property for the dual-role mode, named "dr_mode", albeit the name does not seem to match standard best practices. Seems like it would still make sense to move to that standard property name.
Quoted 6 lines
Yes, this part of change is what below patch is doing
http://patchwork.ozlabs.org/patch/1031014/w.r.t using
usb_get_dr_mode(), it parsesdr_modestring if present in fwnode But in our case, we have mode part of port setting and is not inline with expectation of APIusb_get_dr_mode(). I believe we need to stay with implementation as in above link.
Yeah, I realized the same thing when I tried to convert the above patch to use usb_get_dr_mode() instead. I agree, I think we need to stick with the current implementation.
Quoted 3 lines
Considering current patch and changes in patch/1031014/ are same, please suggest, if changes w.r.t mode reading will be taken as part of patch/1031014/ or shall i continue in current patch series
I would suggest that you either stick with this patch or replace it with the above for now. If the above series goes in first, you can rebase your series on top of it and drop the duplicate patch. If your series goes in first, then the above series will need to be rebased and drop the duplicate patch.
Quoted 25 lines
Quoted 15 lines
Quoted 15 lines
(quoted diff) +15
-0+void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy) +{ + struct tegra_xusb_lane *lane = phy_get_drvdata(phy); + struct tegra_xusb_padctl *padctl = lane->pad->padctl; + + if (padctl->soc->ops->utmi_pad_power_on) + padctl->soc->ops->utmi_pad_power_on(phy); +} +EXPORT_SYMBOL_GPL(tegra_phy_xusb_utmi_pad_power_on); + +void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy) +{ + struct tegra_xusb_lane *lane = phy_get_drvdata(phy); + struct tegra_xusb_padctl *padctl = lane->pad->padctl; +Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ if (padctl->soc->ops->utmi_pad_power_down) + padctl->soc->ops->utmi_pad_power_down(phy); +} +EXPORT_SYMBOL_GPL(tegra_phy_xusb_utmi_pad_power_down);I already briefly touched on this earlier, bu why do these have to be exported functions? Can't these simply be abstracted behind the PHY abstraction?
Quoted 2 lines
yes, they can be part of
phy_power_xxx(). Will removetegra_phy_xusb_utmi_pad_power_xxexport functions
Excellent!
[...]
Quoted 35 lines
Quoted 25 lines
Quoted 12 lines
include/linux/phy/tegra/xusb.h +1
-1@@ -1,5 +1,5 @@/* -* Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved. +* Copyright (c) 2016-2018, NVIDIA CORPORATION. All rights reserved. * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, include/linux/phy/tegra/xusb.h int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl, · line 34
Quoted 9 lines
include/linux/phy/tegra/xusb.h +5
-0@@ -26,5 +26,10 @@ int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl, unsigned int port, bool idle);int tegra_xusb_padctl_usb3_set_lfps_detect (struct tegra_xusb_padctl *padctl,unsigned int port,bool enable); +int tegra_xusb_padctl_set_vbus_override (struct tegra_xusb_padctl *padctl); +int tegra_xusb_padctl_clear_vbus_override (struct tegra_xusb_padctl *padctl); +void tegra_phy_xusb_utmi_pad_power_on (struct phy *phy); +void tegra_phy_xusb_utmi_pad_power_down (struct phy *phy); +int tegra_phy_xusb_utmi_port_reset_quirk (struct phy *phy);We already have more custom APIs here than I'd like, so I really want to make sure that we're not adding to these if not absolutely necessary.
Quoted 9 lines
Are there any reasons why we can't somehow hide these behind the existing PHY framework?
tegra_phy_xusb_utmi_port_reset_quirk()seems like something that we may be able to implement using thephy_ops->reset()callback. Thepower_on()andpower_off()for the UTMI pad seem like they should be just part of the regular pad ->power_on() and ->power_off() callback implementations (perhaps this means that we need to introduce another pad for this?).
Quoted 5 lines
As mentioned earlier, will move utmi pad and utmi bias pad power function can be to phy power functions.
tegra_phy_xusb_utmi_port_reset_quirk()does a vbus override only on a specific condition, so it cannot be direct replacement ofphy_ops->reset.
If XUDC is the only user of the PHY, then it can call phy_reset() on the PHY at any time that it wants, so it could be a replacement for the custom API if we choose to.
That said, it seems like this actually operates on the "port" registers rather than the "pad" registers, so phy_reset() might not be the best candidate after all, since the PHYs really only deal with pads.
Quoted 8 lines
Quoted 5 lines
Set/clear VBUS override seems like maybe something that could be hidden behind
phy_ops->configure(). This is a fairly recent addition, but sounds like we could also push things like HSIC idle and USB3 LPFS detection into that.AFAIU, we can not use
phy_ops->configure(), since the same has enum argumentphy_configure_opts_mipi_dphy, which is not inline with needs of vbus override. Also, API is not present in current kernel.
The API is part of linux-next, so you could rebase the series on that and use it. Also, what I was suggesting is that we could extend the new configure hook with options that can model the VBUS override functionality. But I also realize that it may be a bit of a stretch to consider the VBUS override configuration. Again, it's also dealing with a property that's more in the domain of the port rather than the PHY, so phy_configure() may not be a good candidate after all.
Quoted 2 lines
So i beleive we need to stay with vbus override API, which instead of 2, will export 1(and take argument as set/clear) and other
port_reset_quirk
Sounds good. Although I'm wondering if perhaps the name could be made a bit more generic. What if we called the VBUS override APIs something like:
int tegra_xusb_padctl_set_otg_mode(struct tegra_xusb_padctl *padctl, enum usb_dr_mode mode);
That way we avoid having a boolean argument. I find those somewhat annoying because you can't immediately see from the callsite what they mean. It's fairly clear in this case, but:
tegra_xusb_padctl_set_otg_mode(padctl, USB_DR_MODE_PERIPHERAL);
is even clearer. And it has the added benefit that we can add more code to it if we need to without making the name ambiguous.
Again, this isn't a strong objection, so if you prefer to keep the tegra_xusb_padctl_set_vbus_override(padctl, true/false), feel free to do so.
Thierry