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: 34+ 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-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-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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.