| Message ID | 20200831044043.1561074-1-jckuo@nvidia.com |
|---|---|
| Headers | show |
| Series | Tegra XHCI controller ELPG support | expand |
31.08.2020 07:40, JC Kuo пишет: > + err = devm_request_threaded_irq(&pdev->dev, tegra->padctl_irq, > + NULL, > + tegra_xusb_padctl_irq, > + IRQF_ONESHOT | > + IRQF_TRIGGER_HIGH, Specifying trigger levels is meaningless for interrupts coming from a device-tree because DT levels always take precedence.
Hi Thierry, Thanks for review. On 8/31/20 7:42 PM, Thierry Reding wrote: > Please start commit subjects with a capital letter after the prefix. > Also, please avoid t210 as abbreviation and use tegra210 instead. > > The above should be something like: > > phy: tegra: xusb: tegra210: Rearrange UPHY init > > Or perhaps: > > phy: tegra: xusb: Rearrange UPHY init on Tegra210 I will take this one. Thanks. > > On Mon, Aug 31, 2020 at 12:40:34PM +0800, JC Kuo wrote: >> This commit is a preparation for enabling XUSB SC7 support. >> It rearranges Tegra210 XUSB PADCTL UPHY initialization sequence, >> for the following reasons: >> >> 1. PLLE hardware power sequencer has to be enabled only after both >> PEX UPHY PLL and SATA UPHY PLL are initialized. >> tegra210_uphy_init() -> tegra210_pex_uphy_enable() >> -> tegra210_sata_uphy_enable() >> -> tegra210_plle_hw_sequence_start() >> -> tegra210_aux_mux_lp0_clamp_disable() >> >> 2. Once UPHY PLL hardware power sequencer is enabled, do not assert >> reset to PEX/SATA PLLs, otherwise UPHY PLL operation will be >> broken. >> reset_control_assert(pcie->rst) and reset_control_assert(sata->rst) >> are removed from PEX/SATA UPHY disable procedure. >> >> 3. At cold boot and SC7 exit, the following bits must be cleared after >> PEX/SATA lanes are out of IDDQ (IDDQ_DISABLE=1). >> a. XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN, >> b. XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN_EARLY >> c. XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN >> >> tegra210_pex_uphy_enable() and tegra210_sata_uphy_enable() are in >> charge of bringing lanes out of IDDQ, and then AUX_MUX_LP0_* bits >> will be cleared by tegra210_aux_mux_lp0_clamp_disable(). >> >> 4. The programming sequence in tegra210_usb3_port_enable() is required >> for both cold boot and SC7 exit, and must be performed only after >> PEX/SATA UPHY is initialized. Therefore, this commit moves the >> programming sequence to .power_on() stub which is invoked after >> .init(). PEX/SATA UPHY is initialzied in .init(). >> >> Signed-off-by: JC Kuo <jckuo@nvidia.com> >> --- >> drivers/phy/tegra/xusb-tegra210.c | 495 ++++++++++++++++-------------- >> drivers/phy/tegra/xusb.c | 2 +- >> drivers/phy/tegra/xusb.h | 6 +- >> 3 files changed, 270 insertions(+), 233 deletions(-) > > You've listed 4 logically separate changes in the commit message, so I'm > wondering if it's possible to split this patch into 4 different ones. It > might not be worth doing that if they all basically fix the sequence in > one go, but it's pretty difficult to review this as-is. I found #1 and #3 are not possible to be split. I will submit #2 and #4 as separate changes. > >> >> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c >> index 66bd4613835b..3a2d9797fb9f 100644 >> --- a/drivers/phy/tegra/xusb-tegra210.c >> +++ b/drivers/phy/tegra/xusb-tegra210.c >> @@ -256,23 +256,52 @@ to_tegra210_xusb_padctl(struct tegra_xusb_padctl *padctl) >> return container_of(padctl, struct tegra210_xusb_padctl, base); >> } >> >> +static const struct tegra_xusb_lane_map tegra210_usb3_map[] = { >> + { 0, "pcie", 6 }, >> + { 1, "pcie", 5 }, >> + { 2, "pcie", 0 }, >> + { 2, "pcie", 3 }, >> + { 3, "pcie", 4 }, >> + { 3, "pcie", 4 }, >> + { 0, NULL, 0 } >> +}; >> + >> +static int tegra210_usb3_lane_map(struct tegra_xusb_lane *lane) >> +{ >> + const struct tegra_xusb_lane_map *map; >> + >> + for (map = tegra210_usb3_map; map->type; map++) { >> + if (map->index == lane->index && >> + strcmp(map->type, lane->pad->soc->name) == 0) { >> + dev_dbg(lane->pad->padctl->dev, >> + "lane = %s map to port = usb3-%d\n", > > "mapped to port"? Yes, each PEX/SATA lane maps to an USB3 (super-speed) port. > >> + lane->pad->soc->lanes[lane->index].name, >> + map->port); >> + return map->port; >> + } >> + } >> + >> + return -EINVAL; >> +} >> + >> /* must be called under padctl->lock */ >> static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl) >> { >> struct tegra_xusb_pcie_pad *pcie = to_pcie_pad(padctl->pcie); >> unsigned long timeout; >> u32 value; >> - int err; >> + int err, i; > > i should be unsigned to match the type of padctl->pcie->soc->num_lanes. I will fix this. Thanks. > >> >> - if (pcie->enable > 0) { >> - pcie->enable++; >> + if (pcie->enable) >> return 0; >> - } >> >> err = clk_prepare_enable(pcie->pll); >> if (err < 0) >> return err; >> >> + if (tegra210_plle_hw_sequence_is_enabled()) >> + goto skip_pll_init; >> + >> err = reset_control_deassert(pcie->rst); > > Is it guaranteed that the reset is asserted if the PLLE HW sequencer is > enabled? I suppose with the change to not enable the sequencer by > default in one of the earlier patches this may indeed be a valid > assumption. Yes, reset is de-asserted before PLLE initialization happens. > >> if (err < 0) >> goto disable; >> @@ -455,7 +484,14 @@ static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl) >> >> tegra210_xusb_pll_hw_sequence_start(); >> >> - pcie->enable++; >> +skip_pll_init: >> + pcie->enable = true; >> + >> + for (i = 0; i < padctl->pcie->soc->num_lanes; i++) { >> + value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX); >> + value |= XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(i); >> + padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX); >> + } >> >> return 0; >> >> @@ -469,34 +505,42 @@ static int tegra210_pex_uphy_enable(struct tegra_xusb_padctl *padctl) >> static void tegra210_pex_uphy_disable(struct tegra_xusb_padctl *padctl) >> { >> struct tegra_xusb_pcie_pad *pcie = to_pcie_pad(padctl->pcie); >> + u32 value; >> + int i; > > Same as above. I will fix this. Thanks. > >> >> - mutex_lock(&padctl->lock); >> - >> - if (WARN_ON(pcie->enable == 0)) >> - goto unlock; >> + if (WARN_ON(!pcie->enable)) >> + return; >> >> - if (--pcie->enable > 0) >> - goto unlock; >> + pcie->enable = false; >> >> - reset_control_assert(pcie->rst); >> + for (i = 0; i < padctl->pcie->soc->num_lanes; i++) { >> + value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX); >> + value &= ~XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(i); >> + padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX); >> + } >> clk_disable_unprepare(pcie->pll); > > Please leave a blank line after a block for better readability. I will fix this. Thanks. > >> - >> -unlock: >> - mutex_unlock(&padctl->lock); >> } >> >> /* must be called under padctl->lock */ >> -static int tegra210_sata_uphy_enable(struct tegra_xusb_padctl *padctl, bool usb) >> +static int tegra210_sata_uphy_enable(struct tegra_xusb_padctl *padctl) >> { >> struct tegra_xusb_sata_pad *sata = to_sata_pad(padctl->sata); >> + struct tegra_xusb_lane *lane = tegra_xusb_find_lane(padctl, "sata", 0); >> unsigned long timeout; >> u32 value; >> - int err; >> + int err, i; > > Same comment as above for "i". I will fix this. Thanks. > >> + bool usb; >> >> - if (sata->enable > 0) { >> - sata->enable++; >> + if (sata->enable) > > Do we want a WARN_ON() here for symmetry with the implementation of > tegra210_sata_uphy_disable() below? Yes, I can add this. > >> return 0; >> - } >> + >> + if (!lane) >> + return 0; >> + >> + if (tegra210_plle_hw_sequence_is_enabled()) >> + goto skip_pll_init; >> + >> + usb = tegra_xusb_lane_check(lane, "usb3-ss"); >> >> err = clk_prepare_enable(sata->pll); >> if (err < 0) >> @@ -697,7 +741,14 @@ static int tegra210_sata_uphy_enable(struct tegra_xusb_padctl *padctl, bool usb) >> >> tegra210_sata_pll_hw_sequence_start(); >> >> - sata->enable++; >> +skip_pll_init: >> + sata->enable = true; >> + >> + for (i = 0; i < padctl->sata->soc->num_lanes; i++) { >> + value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX); >> + value |= XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(i); >> + padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX); >> + } >> >> return 0; >> >> @@ -711,31 +762,26 @@ static int tegra210_sata_uphy_enable(struct tegra_xusb_padctl *padctl, bool usb) >> static void tegra210_sata_uphy_disable(struct tegra_xusb_padctl *padctl) >> { >> struct tegra_xusb_sata_pad *sata = to_sata_pad(padctl->sata); >> + u32 value; >> + int i; > > unsigned int, please. I will fix this. Thanks. > >> >> - mutex_lock(&padctl->lock); >> - >> - if (WARN_ON(sata->enable == 0)) >> - goto unlock; >> + if (WARN_ON(!sata->enable)) >> + return; >> >> - if (--sata->enable > 0) >> - goto unlock; >> + sata->enable = false; >> >> - reset_control_assert(sata->rst); >> + for (i = 0; i < padctl->sata->soc->num_lanes; i++) { >> + value = padctl_readl(padctl, XUSB_PADCTL_USB3_PAD_MUX); >> + value &= ~XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(i); >> + padctl_writel(padctl, value, XUSB_PADCTL_USB3_PAD_MUX); >> + } >> clk_disable_unprepare(sata->pll); >> - >> -unlock: >> - mutex_unlock(&padctl->lock); >> } >> >> -static int tegra210_xusb_padctl_enable(struct tegra_xusb_padctl *padctl) >> +static void tegra210_aux_mux_lp0_clamp_disable(struct tegra_xusb_padctl *padctl) >> { >> u32 value; >> >> - mutex_lock(&padctl->lock); >> - >> - if (padctl->enable++ > 0) >> - goto out; >> - >> value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1); >> value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN; >> padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1); >> @@ -751,24 +797,12 @@ static int tegra210_xusb_padctl_enable(struct tegra_xusb_padctl *padctl) >> value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1); >> value &= ~XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN; >> padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1); >> - >> -out: >> - mutex_unlock(&padctl->lock); >> - return 0; >> } >> >> -static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl) >> +static void tegra210_aux_mux_lp0_clamp_enable(struct tegra_xusb_padctl *padctl) >> { >> u32 value; >> >> - mutex_lock(&padctl->lock); >> - >> - if (WARN_ON(padctl->enable == 0)) >> - goto out; >> - >> - if (--padctl->enable > 0) >> - goto out; >> - >> value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1); >> value |= XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN; >> padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1); >> @@ -784,12 +818,36 @@ static int tegra210_xusb_padctl_disable(struct tegra_xusb_padctl *padctl) >> value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1); >> value |= XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_CLAMP_EN; >> padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM1); >> +} >> + >> +static int tegra210_uphy_init(struct tegra_xusb_padctl *padctl) >> +{ >> + if (padctl->pcie) >> + tegra210_pex_uphy_enable(padctl); >> + if (padctl->sata) >> + tegra210_sata_uphy_enable(padctl); >> + >> + if (!tegra210_plle_hw_sequence_is_enabled()) >> + tegra210_plle_hw_sequence_start(); >> + else >> + dev_dbg(padctl->dev, "PLLE is already in HW control\n"); >> + >> + tegra210_aux_mux_lp0_clamp_disable(padctl); >> >> -out: >> - mutex_unlock(&padctl->lock); >> return 0; >> } >> >> +static void __maybe_unused >> +tegra210_uphy_deinit(struct tegra_xusb_padctl *padctl) >> +{ >> + tegra210_aux_mux_lp0_clamp_enable(padctl); > > Do we need tegra210_plle_hw_sequence_stop() here? > PLLE hardware power sequencer must remain enabled at SC7 entry. >> + >> + if (padctl->pcie) >> + tegra210_pex_uphy_disable(padctl); >> + if (padctl->sata) >> + tegra210_sata_uphy_disable(padctl); > > Maybe reverse the order of these two so that they are symmetrical with > tegra210_uphy_init()? Also, single blank lines between the two blocks > make this easier to read, in my opinion. I can do this. Thanks. > > Thierry >
Hi Thierry, Thanks for review. I will amend accordingly and submit a new revision. JC On 8/31/20 8:50 PM, Thierry Reding wrote: > On Mon, Aug 31, 2020 at 12:40:43PM +0800, JC Kuo wrote: >> This commit implements the complete programming sequence for ELPG >> entry and exit. >> >> 1. At ELPG entry, invokes tegra_xusb_padctl_enable_phy_sleepwalk() >> and tegra_xusb_padctl_enable_phy_wake() to configure XUSB PADCTL >> sleepwalk and wake detection circuits to maintain USB lines level >> and respond to wake events (wake-on-connect, wake-on-disconnect, >> device-initiated-wake). >> >> 2. At ELPG exit, invokes tegra_xusb_padctl_disable_phy_sleepwalk() >> and tegra_xusb_padctl_disable_phy_wake() to disarm sleepwalk and >> wake detection circuits. >> >> At runtime suspend, XUSB host controller can enter ELPG to reduce >> power consumption. When XUSB PADCTL wake detection circuit detects >> a wake event, an interrupt will be raised. xhci-tegra driver then >> will invoke pm_runtime_resume() for xhci-tegra. >> >> Runtime resume could also be triggered by protocol drivers, this is >> the host-initiated-wake event. At runtime resume, xhci-tegra driver >> brings XUSB host controller out of ELPG to handle the wake events. >> >> The same ELPG enter/exit procedure will be performed for system >> suspend/resume path so USB devices can remain connected across SC7. >> >> Signed-off-by: JC Kuo <jckuo@nvidia.com> >> --- >> drivers/usb/host/xhci-tegra.c | 391 +++++++++++++++++++++++++++++++--- >> 1 file changed, 361 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c >> index ce6526c2caf6..9530cfc83f45 100644 >> --- a/drivers/usb/host/xhci-tegra.c >> +++ b/drivers/usb/host/xhci-tegra.c >> @@ -15,9 +15,11 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/of_device.h> >> +#include <linux/of_irq.h> >> #include <linux/phy/phy.h> >> #include <linux/phy/tegra/xusb.h> >> #include <linux/platform_device.h> >> +#include <linux/usb/ch9.h> >> #include <linux/pm.h> >> #include <linux/pm_domain.h> >> #include <linux/pm_runtime.h> >> @@ -224,6 +226,7 @@ struct tegra_xusb { >> >> int xhci_irq; >> int mbox_irq; >> + int padctl_irq; >> >> void __iomem *ipfs_base; >> void __iomem *fpci_base; >> @@ -268,10 +271,13 @@ struct tegra_xusb { >> dma_addr_t phys; >> } fw; >> >> + bool suspended; >> struct tegra_xusb_context context; >> }; >> >> static struct hc_driver __read_mostly tegra_xhci_hc_driver; >> +static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime); >> +static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime); >> >> static inline u32 fpci_readl(struct tegra_xusb *tegra, unsigned int offset) >> { >> @@ -657,6 +663,9 @@ static irqreturn_t tegra_xusb_mbox_thread(int irq, void *data) >> >> mutex_lock(&tegra->lock); >> >> + if (pm_runtime_suspended(tegra->dev) || tegra->suspended) >> + goto out; >> + >> value = fpci_readl(tegra, tegra->soc->mbox.data_out); >> tegra_xusb_mbox_unpack(&msg, value); >> >> @@ -670,6 +679,7 @@ static irqreturn_t tegra_xusb_mbox_thread(int irq, void *data) >> >> tegra_xusb_mbox_handle(tegra, &msg); >> >> +out: >> mutex_unlock(&tegra->lock); >> return IRQ_HANDLED; >> } >> @@ -812,12 +822,27 @@ static void tegra_xusb_phy_disable(struct tegra_xusb *tegra) >> >> static int tegra_xusb_runtime_suspend(struct device *dev) >> { >> - return 0; >> + struct tegra_xusb *tegra = dev_get_drvdata(dev); >> + int ret; >> + >> + synchronize_irq(tegra->mbox_irq); >> + mutex_lock(&tegra->lock); >> + ret = tegra_xusb_enter_elpg(tegra, true); >> + mutex_unlock(&tegra->lock); >> + >> + return ret; >> } >> >> static int tegra_xusb_runtime_resume(struct device *dev) >> { >> - return 0; >> + struct tegra_xusb *tegra = dev_get_drvdata(dev); >> + int err; >> + >> + mutex_lock(&tegra->lock); >> + err = tegra_xusb_exit_elpg(tegra, true); >> + mutex_unlock(&tegra->lock); >> + >> + return err; >> } >> >> #ifdef CONFIG_PM_SLEEP >> @@ -1121,6 +1146,22 @@ static int __tegra_xusb_enable_firmware_messages(struct tegra_xusb *tegra) >> return err; >> } >> >> +static irqreturn_t tegra_xusb_padctl_irq(int irq, void *data) >> +{ >> + struct tegra_xusb *tegra = data; >> + >> + mutex_lock(&tegra->lock); >> + if (tegra->suspended) { >> + mutex_unlock(&tegra->lock); >> + return IRQ_HANDLED; >> + } >> + mutex_unlock(&tegra->lock); > > Blank lines before and after a block can help make this less cluttered. > >> + >> + pm_runtime_resume(tegra->dev); >> + >> + return IRQ_HANDLED; >> +} >> + >> static int tegra_xusb_enable_firmware_messages(struct tegra_xusb *tegra) >> { >> int err; >> @@ -1244,6 +1285,51 @@ static void tegra_xhci_id_work(struct work_struct *work) >> } >> } >> >> +static bool is_usb2_otg_phy(struct tegra_xusb *tegra, int index) > > unsigned int index? > >> +{ >> + return (tegra->usbphy[index] != NULL); >> +} >> + >> +static bool is_usb3_otg_phy(struct tegra_xusb *tegra, int index) > > Here too. > >> +{ >> + struct tegra_xusb_padctl *padctl = tegra->padctl; >> + int i, port; > > These can also be unsigned. > >> + >> + for (i = 0; i < tegra->num_usb_phys; i++) { >> + if (is_usb2_otg_phy(tegra, i)) { >> + port = tegra_xusb_padctl_get_usb3_companion(padctl,i); > > Space after ",". > >> + if (index == port) >> + return true; >> + } >> + } >> + >> + return false; >> +} >> + >> +static bool is_host_mode_phy(struct tegra_xusb *tegra, int phy_type, int index) >> +{ >> + if (strcmp(tegra->soc->phy_types[phy_type].name, "hsic") == 0) >> + return true; >> + >> + if (strcmp(tegra->soc->phy_types[phy_type].name, "usb2") == 0) { >> + if (is_usb2_otg_phy(tegra, index)) { >> + return ((index == tegra->otg_usb2_port) && >> + tegra->host_mode); >> + } else >> + return true; >> + } >> + >> + if (strcmp(tegra->soc->phy_types[phy_type].name, "usb3") == 0) { >> + if (is_usb3_otg_phy(tegra, index)) { >> + return ((index == tegra->otg_usb3_port) && >> + tegra->host_mode); >> + } else >> + return true; >> + } >> + >> + return false; >> +} >> + >> static int tegra_xusb_get_usb2_port(struct tegra_xusb *tegra, >> struct usb_phy *usbphy) >> { >> @@ -1336,6 +1422,7 @@ static void tegra_xusb_deinit_usb_phy(struct tegra_xusb *tegra) >> static int tegra_xusb_probe(struct platform_device *pdev) >> { >> struct tegra_xusb *tegra; >> + struct device_node *np; >> struct resource *regs; >> struct xhci_hcd *xhci; >> unsigned int i, j, k; >> @@ -1383,6 +1470,14 @@ static int tegra_xusb_probe(struct platform_device *pdev) >> if (IS_ERR(tegra->padctl)) >> return PTR_ERR(tegra->padctl); >> >> + np = of_parse_phandle(pdev->dev.of_node, "nvidia,xusb-padctl", 0); >> + if (!np) >> + return -ENODEV; >> + >> + tegra->padctl_irq = of_irq_get(np, 0); >> + if (tegra->padctl_irq < 0) >> + return tegra->padctl_irq; >> + >> tegra->host_clk = devm_clk_get(&pdev->dev, "xusb_host"); >> if (IS_ERR(tegra->host_clk)) { >> err = PTR_ERR(tegra->host_clk); >> @@ -1527,6 +1622,7 @@ static int tegra_xusb_probe(struct platform_device *pdev) >> goto put_powerdomains; >> } >> >> + tegra->hcd->skip_phy_initialization = 1; >> tegra->hcd->regs = tegra->regs; >> tegra->hcd->rsrc_start = regs->start; >> tegra->hcd->rsrc_len = resource_size(regs); >> @@ -1609,12 +1705,6 @@ static int tegra_xusb_probe(struct platform_device *pdev) >> goto put_usb3; >> } >> >> - err = tegra_xusb_enable_firmware_messages(tegra); >> - if (err < 0) { >> - dev_err(&pdev->dev, "failed to enable messages: %d\n", err); >> - goto remove_usb3; >> - } >> - >> err = devm_request_threaded_irq(&pdev->dev, tegra->mbox_irq, >> tegra_xusb_mbox_irq, >> tegra_xusb_mbox_thread, 0, >> @@ -1624,12 +1714,40 @@ static int tegra_xusb_probe(struct platform_device *pdev) >> goto remove_usb3; >> } >> >> + err = devm_request_threaded_irq(&pdev->dev, tegra->padctl_irq, >> + NULL, >> + tegra_xusb_padctl_irq, >> + IRQF_ONESHOT | >> + IRQF_TRIGGER_HIGH, >> + dev_name(&pdev->dev), tegra); > > The alignment here is off. Also, try to make good use of those 100 > columns. > >> + if (err < 0) { >> + dev_err(&pdev->dev, "failed to request padctl IRQ: %d\n", err); >> + goto remove_usb3; >> + } >> + >> + err = tegra_xusb_enable_firmware_messages(tegra); >> + if (err < 0) { >> + dev_err(&pdev->dev, "failed to enable messages: %d\n", err); >> + goto remove_usb3; >> + } >> + >> err = tegra_xusb_init_usb_phy(tegra); >> if (err < 0) { >> dev_err(&pdev->dev, "failed to init USB PHY: %d\n", err); >> goto remove_usb3; >> } >> >> + /* Enable wake for both USB 2.0 and USB 3.0 roothubs */ >> + device_init_wakeup(&tegra->hcd->self.root_hub->dev, true); >> + device_init_wakeup(&xhci->shared_hcd->self.root_hub->dev, true); >> + device_init_wakeup(tegra->dev, true); >> + >> + pm_runtime_use_autosuspend(tegra->dev); >> + pm_runtime_set_autosuspend_delay(tegra->dev, 2000); >> + pm_runtime_mark_last_busy(tegra->dev); >> + pm_runtime_set_active(tegra->dev); >> + pm_runtime_enable(tegra->dev); >> + >> return 0; >> >> remove_usb3: >> @@ -1665,6 +1783,7 @@ static int tegra_xusb_remove(struct platform_device *pdev) >> >> tegra_xusb_deinit_usb_phy(tegra); >> >> + pm_runtime_get_sync(&pdev->dev); >> usb_remove_hcd(xhci->shared_hcd); >> usb_put_hcd(xhci->shared_hcd); >> xhci->shared_hcd = NULL; >> @@ -1674,8 +1793,8 @@ static int tegra_xusb_remove(struct platform_device *pdev) >> dma_free_coherent(&pdev->dev, tegra->fw.size, tegra->fw.virt, >> tegra->fw.phys); >> >> - pm_runtime_put_sync(&pdev->dev); >> pm_runtime_disable(&pdev->dev); >> + pm_runtime_put(&pdev->dev); >> >> tegra_xusb_powergate_partitions(tegra); >> >> @@ -1717,9 +1836,17 @@ static bool xhci_hub_ports_suspended(struct xhci_hub *hub) >> static int tegra_xusb_check_ports(struct tegra_xusb *tegra) >> { >> struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); >> + struct xhci_hub *rhub = xhci_get_rhub(xhci->main_hcd); >> + struct xhci_bus_state *bus_state = &rhub->bus_state; >> unsigned long flags; >> int err = 0; >> >> + if (bus_state->bus_suspended) { >> + /* xusb_hub_suspend() has just directed one or more USB2 port(s) >> + * to U3 state, it takes 3ms to enter U3. */ >> + usleep_range(3000, 4000); >> + } >> + >> spin_lock_irqsave(&xhci->lock, flags); >> >> if (!xhci_hub_ports_suspended(&xhci->usb2_rhub) || >> @@ -1765,45 +1892,184 @@ static void tegra_xusb_restore_context(struct tegra_xusb *tegra) >> } >> } >> >> -static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool wakeup) >> +static enum usb_device_speed >> +tegra_xhci_portsc_to_speed(struct tegra_xusb *tegra, u32 portsc) >> +{ >> + if (DEV_LOWSPEED(portsc)) >> + return USB_SPEED_LOW; >> + else if (DEV_HIGHSPEED(portsc)) >> + return USB_SPEED_HIGH; >> + else if (DEV_FULLSPEED(portsc)) >> + return USB_SPEED_FULL; >> + else if (DEV_SUPERSPEED_ANY(portsc)) >> + return USB_SPEED_SUPER; >> + else >> + return USB_SPEED_UNKNOWN; >> +} > > As in a prior patch you can make this simpler by dropping the elses. > >> + >> +static void tegra_xhci_enable_phy_sleepwalk_wake(struct tegra_xusb *tegra) >> +{ >> + struct tegra_xusb_padctl *padctl = tegra->padctl; >> + struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); >> + enum usb_device_speed speed; >> + struct phy *phy; >> + int index, offset; >> + int i, j, k; > > It looks like these can all be unsigned int. > >> + struct xhci_hub *rhub; >> + u32 portsc; >> + >> + for (i = 0, k = 0; i < tegra->soc->num_types; i++) { >> + if (strcmp(tegra->soc->phy_types[i].name, "usb3") == 0) >> + rhub = &xhci->usb3_rhub; >> + else >> + rhub = &xhci->usb2_rhub; >> + >> + if (strcmp(tegra->soc->phy_types[i].name, "hsic") == 0) >> + offset = tegra->soc->ports.usb2.count; >> + else >> + offset = 0; >> + >> + for (j = 0; j < tegra->soc->phy_types[i].num; j++) { >> + phy = tegra->phys[k++]; >> + >> + if (!phy) >> + continue; >> + >> + index = j + offset; >> + >> + if (index >= rhub->num_ports) >> + continue; >> + >> + if (!is_host_mode_phy(tegra, i, j)) >> + continue; >> + >> + portsc = readl(rhub->ports[index]->addr); >> + speed = tegra_xhci_portsc_to_speed(tegra, portsc); >> + tegra_xusb_padctl_enable_phy_sleepwalk(padctl, phy, >> + speed); >> + tegra_xusb_padctl_enable_phy_wake(padctl, phy); >> + } >> + } >> +} >> + >> +static void tegra_xhci_disable_phy_wake(struct tegra_xusb *tegra) >> +{ >> + struct tegra_xusb_padctl *padctl = tegra->padctl; >> + int i; > > Same here. > >> + >> + for (i = 0; i < tegra->num_phys; i++) { >> + if (!tegra->phys[i]) >> + continue; >> + >> + tegra_xusb_padctl_disable_phy_wake(padctl, tegra->phys[i]); >> + } >> +} >> + >> +static void tegra_xhci_disable_phy_sleepwalk(struct tegra_xusb *tegra) >> +{ >> + struct tegra_xusb_padctl *padctl = tegra->padctl; >> + int i; > > And here. > >> + >> + for (i = 0; i < tegra->num_phys; i++) { >> + if (!tegra->phys[i]) >> + continue; >> + >> + tegra_xusb_padctl_disable_phy_sleepwalk(padctl, tegra->phys[i]); >> + } >> +} >> + >> +static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) >> { >> struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); >> + struct device *dev = tegra->dev; >> + bool wakeup = runtime ? true : device_may_wakeup(dev); >> + unsigned int i; >> int err; >> + u32 usbcmd; >> + >> + dev_dbg(dev, "entering ELPG\n"); >> + >> + usbcmd = readl(&xhci->op_regs->command); >> + usbcmd &= ~CMD_EIE; >> + writel(usbcmd, &xhci->op_regs->command); >> >> err = tegra_xusb_check_ports(tegra); >> if (err < 0) { >> dev_err(tegra->dev, "not all ports suspended: %d\n", err); >> - return err; >> + goto out; >> } >> >> err = xhci_suspend(xhci, wakeup); >> if (err < 0) { >> dev_err(tegra->dev, "failed to suspend XHCI: %d\n", err); >> - return err; >> + goto out; >> } >> >> tegra_xusb_save_context(tegra); >> - tegra_xusb_phy_disable(tegra); >> + >> + if (wakeup) >> + tegra_xhci_enable_phy_sleepwalk_wake(tegra); >> + >> + tegra_xusb_powergate_partitions(tegra); >> + >> + for (i = 0; i < tegra->num_phys; i++) { >> + if (!tegra->phys[i]) >> + continue; >> + >> + phy_power_off(tegra->phys[i]); >> + if (!wakeup) >> + phy_exit(tegra->phys[i]); >> + } >> tegra_xusb_clk_disable(tegra); > > Use blank lines to separate blocks of code. > >> >> - return 0; >> +out: >> + if (!err) >> + dev_dbg(tegra->dev, "entering ELPG done\n"); >> + else { >> + usbcmd = readl(&xhci->op_regs->command); >> + usbcmd |= CMD_EIE; >> + writel(usbcmd, &xhci->op_regs->command); >> + >> + dev_dbg(tegra->dev, "entering ELPG failed\n"); >> + pm_runtime_mark_last_busy(tegra->dev); >> + } >> + >> + return err; >> } >> >> -static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool wakeup) >> +static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime) >> { >> struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); >> + struct device *dev = tegra->dev; >> + bool wakeup = runtime ? true : device_may_wakeup(dev); >> + unsigned int i; >> + u32 usbcmd; >> int err; >> >> + dev_dbg(dev, "exiting ELPG\n"); >> + pm_runtime_mark_last_busy(tegra->dev); >> + >> err = tegra_xusb_clk_enable(tegra); >> if (err < 0) { >> dev_err(tegra->dev, "failed to enable clocks: %d\n", err); >> - return err; >> + goto out; >> } >> >> - err = tegra_xusb_phy_enable(tegra); >> - if (err < 0) { >> - dev_err(tegra->dev, "failed to enable PHYs: %d\n", err); >> - goto disable_clk; >> + err = tegra_xusb_unpowergate_partitions(tegra); >> + if (err) >> + goto disable_clks; >> + >> + if (wakeup) >> + tegra_xhci_disable_phy_wake(tegra); >> + >> + for (i = 0; i < tegra->num_phys; i++) { >> + if (!tegra->phys[i]) >> + continue; >> + >> + if (!wakeup) >> + phy_init(tegra->phys[i]); >> + >> + phy_power_on(tegra->phys[i]); >> } >> >> tegra_xusb_config(tegra); >> @@ -1821,31 +2087,78 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool wakeup) >> goto disable_phy; >> } >> >> - err = xhci_resume(xhci, true); >> + if (wakeup) >> + tegra_xhci_disable_phy_sleepwalk(tegra); >> + >> + err = xhci_resume(xhci, 0); >> if (err < 0) { >> dev_err(tegra->dev, "failed to resume XHCI: %d\n", err); >> goto disable_phy; >> } >> >> - return 0; >> + usbcmd = readl(&xhci->op_regs->command); >> + usbcmd |= CMD_EIE; >> + writel(usbcmd, &xhci->op_regs->command); >> + >> + goto out; >> >> disable_phy: >> - tegra_xusb_phy_disable(tegra); >> -disable_clk: >> + for (i = 0; i < tegra->num_phys; i++) { >> + if (!tegra->phys[i]) >> + continue; >> + >> + phy_power_off(tegra->phys[i]); >> + if (!wakeup) >> + phy_exit(tegra->phys[i]); >> + } >> + tegra_xusb_powergate_partitions(tegra); >> +disable_clks: >> tegra_xusb_clk_disable(tegra); >> +out: >> + if (!err) >> + dev_dbg(dev, "exiting ELPG done\n"); >> + else >> + dev_dbg(dev, "exiting ELPG failed\n"); >> + >> return err; >> } >> >> static int tegra_xusb_suspend(struct device *dev) >> { >> struct tegra_xusb *tegra = dev_get_drvdata(dev); >> - bool wakeup = device_may_wakeup(dev); >> int err; >> >> synchronize_irq(tegra->mbox_irq); >> - > > I think that blank line actually helped with readability. > > Thierry > >> mutex_lock(&tegra->lock); >> - err = tegra_xusb_enter_elpg(tegra, wakeup); >> + >> + if (pm_runtime_suspended(dev)) { >> + err = tegra_xusb_exit_elpg(tegra, true); >> + if (err < 0) >> + goto out; >> + } >> + >> + err = tegra_xusb_enter_elpg(tegra, false); >> + if (err < 0) { >> + if (pm_runtime_suspended(dev)) { >> + pm_runtime_disable(dev); >> + pm_runtime_set_active(dev); >> + pm_runtime_enable(dev); >> + } >> + >> + goto out; >> + } >> + >> +out: >> + if (!err) { >> + tegra->suspended = true; >> + pm_runtime_disable(dev); >> + >> + if (device_may_wakeup(dev)) { >> + if (enable_irq_wake(tegra->padctl_irq)) >> + dev_err(dev, "failed to enable padctl wakes\n"); >> + } >> + } >> + >> mutex_unlock(&tegra->lock); >> >> return err; >> @@ -1854,14 +2167,32 @@ static int tegra_xusb_suspend(struct device *dev) >> static int tegra_xusb_resume(struct device *dev) >> { >> struct tegra_xusb *tegra = dev_get_drvdata(dev); >> - bool wakeup = device_may_wakeup(dev); >> int err; >> >> mutex_lock(&tegra->lock); >> - err = tegra_xusb_exit_elpg(tegra, wakeup); >> + >> + if (!tegra->suspended) { >> + mutex_unlock(&tegra->lock); >> + return 0; >> + } >> + >> + err = tegra_xusb_exit_elpg(tegra, false); >> + if (err < 0) { >> + mutex_unlock(&tegra->lock); >> + return err; >> + } >> + >> + if (device_may_wakeup(dev)) { >> + if (disable_irq_wake(tegra->padctl_irq)) >> + dev_err(dev, "failed to disable padctl wakes\n"); >> + } >> + tegra->suspended = false; >> mutex_unlock(&tegra->lock); >> >> - return err; >> + pm_runtime_set_active(dev); >> + pm_runtime_enable(dev); >> + >> + return 0; >> } >> #endif >> >> -- >> 2.25.1 >>
Thanks Dmitry. I will remove this. On 9/2/20 4:33 AM, Dmitry Osipenko wrote: > 31.08.2020 07:40, JC Kuo пишет: >> + err = devm_request_threaded_irq(&pdev->dev, tegra->padctl_irq, >> + NULL, >> + tegra_xusb_padctl_irq, >> + IRQF_ONESHOT | > >> + IRQF_TRIGGER_HIGH, > > Specifying trigger levels is meaningless for interrupts coming from a > device-tree because DT levels always take precedence. >
Tegra XHCI controler can be placed in ELPG (Engine Level PowerGated) state for power saving when all of the connected USB devices are in suspended state. This patch series includes clk, phy and pmc changes that are required for properly place controller in ELPG and bring controller out of ELPG. JC Kuo (12): clk: tegra: Add PLLE HW power sequencer control v2: no change clk: tegra: don't enable PLLE HW sequencer at init v2: no change phy: tegra: xusb: t210: rearrange UPHY init v2: no change phy: tegra: xusb: t210: add lane_iddq operations v2: no change phy: tegra: xusb: add sleepwalk and suspend/resume v2: no change soc/tegra: pmc: provide usb sleepwalk register map v2: make tegra_pmc_regmap_readl() and tegra_pmc_regmap_writel() static arm64: tegra210: XUSB PADCTL add "nvidia,pmc" prop v2: no change phy: tegra: xusb: t210: support wake and sleepwalk v2: no change phy: tegra: xusb: t186: support wake and sleepwalk v2: no change arm64: tegra210/tegra186/tegra194: XUSB PADCTL irq v2: no change usb: host: xhci-tegra: unlink power domain devices v2: no change xhci: tegra: enable ELPG for runtime/system PM v2: no change arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 + arch/arm64/boot/dts/nvidia/tegra194.dtsi | 1 + arch/arm64/boot/dts/nvidia/tegra210.dtsi | 2 + drivers/clk/tegra/clk-pll.c | 12 - drivers/clk/tegra/clk-tegra210.c | 51 + drivers/phy/tegra/xusb-tegra186.c | 656 ++++++++ drivers/phy/tegra/xusb-tegra210.c | 1953 +++++++++++++++++----- drivers/phy/tegra/xusb.c | 86 +- drivers/phy/tegra/xusb.h | 23 +- drivers/soc/tegra/pmc.c | 89 + drivers/usb/host/xhci-tegra.c | 577 +++++-- include/linux/clk/tegra.h | 2 + include/linux/phy/tegra/xusb.h | 13 + 13 files changed, 2957 insertions(+), 509 deletions(-)