From: Thierry Reding <thierry.reding@gmail.com>
To: JC Kuo <jckuo@nvidia.com>
Cc: gregkh@linuxfoundation.org, robh@kernel.org,
jonathanh@nvidia.com, kishon@ti.com, linux-tegra@vger.kernel.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, nkristam@nvidia.com
Subject: Re: [PATCH v2 03/12] phy: tegra: xusb: t210: rearrange UPHY init
Date: Mon, 31 Aug 2020 13:42:12 +0200 [thread overview]
Message-ID: <20200831114212.GA1689119@ulmo> (raw)
In-Reply-To: <20200831044043.1561074-4-jckuo@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 11204 bytes --]
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
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.
>
> 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"?
> + 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.
>
> - 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.
> 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.
>
> - 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.
> -
> -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".
> + 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?
> 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.
>
> - 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?
> +
> + 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.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-08-31 11:42 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-31 4:40 [PATCH v2 00/12] Tegra XHCI controller ELPG support JC Kuo
2020-08-31 4:40 ` [PATCH v2 01/12] clk: tegra: Add PLLE HW power sequencer control JC Kuo
2020-08-31 4:40 ` [PATCH v2 02/12] clk: tegra: don't enable PLLE HW sequencer at init JC Kuo
2020-08-31 4:40 ` [PATCH v2 03/12] phy: tegra: xusb: t210: rearrange UPHY init JC Kuo
2020-08-31 11:42 ` Thierry Reding [this message]
2020-09-04 9:10 ` JC Kuo
2020-08-31 4:40 ` [PATCH v2 04/12] phy: tegra: xusb: t210: add lane_iddq operations JC Kuo
2020-08-31 11:53 ` Thierry Reding
2020-09-07 2:26 ` JC Kuo
2020-08-31 4:40 ` [PATCH v2 05/12] phy: tegra: xusb: add sleepwalk and suspend/resume JC Kuo
2020-08-31 11:58 ` Thierry Reding
2020-09-07 2:34 ` JC Kuo
2020-08-31 4:40 ` [PATCH v2 06/12] soc/tegra: pmc: provide usb sleepwalk register map JC Kuo
2020-08-31 12:09 ` Thierry Reding
2020-09-07 3:07 ` JC Kuo
2020-08-31 4:40 ` [PATCH v2 07/12] arm64: tegra210: XUSB PADCTL add "nvidia,pmc" prop JC Kuo
2020-08-31 4:40 ` [PATCH v2 08/12] phy: tegra: xusb: t210: support wake and sleepwalk JC Kuo
2020-08-31 12:37 ` Thierry Reding
2020-09-08 1:14 ` JC Kuo
2020-09-01 15:14 ` kernel test robot
2020-08-31 4:40 ` [PATCH v2 09/12] phy: tegra: xusb: t186: " JC Kuo
2020-08-31 12:38 ` Thierry Reding
2020-09-01 16:51 ` kernel test robot
2020-08-31 4:40 ` [PATCH v2 10/12] arm64: tegra210/tegra186/tegra194: XUSB PADCTL irq JC Kuo
2020-08-31 4:40 ` [PATCH v2 11/12] usb: host: xhci-tegra: unlink power domain devices JC Kuo
2020-08-31 12:42 ` Thierry Reding
2020-09-08 2:19 ` JC Kuo
2020-08-31 4:40 ` [PATCH v2 12/12] xhci: tegra: enable ELPG for runtime/system PM JC Kuo
2020-08-31 12:50 ` Thierry Reding
2020-09-08 2:29 ` JC Kuo
2020-09-01 20:33 ` Dmitry Osipenko
2020-09-08 2:42 ` JC Kuo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200831114212.GA1689119@ulmo \
--to=thierry.reding@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jckuo@nvidia.com \
--cc=jonathanh@nvidia.com \
--cc=kishon@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=nkristam@nvidia.com \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).