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 04/12] phy: tegra: xusb: t210: add lane_iddq operations
Date: Mon, 31 Aug 2020 13:53:30 +0200 [thread overview]
Message-ID: <20200831115330.GB1689119@ulmo> (raw)
In-Reply-To: <20200831044043.1561074-5-jckuo@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 9236 bytes --]
On Mon, Aug 31, 2020 at 12:40:35PM +0800, JC Kuo wrote:
> As per Tegra210 TRM, before changing lane assignments, driver should
> keep lanes in IDDQ and sleep state; after changing lane assignments,
> driver should bring lanes out of IDDQ.
> This commit implements the required operations.
>
> Signed-off-by: JC Kuo <jckuo@nvidia.com>
> ---
> drivers/phy/tegra/xusb-tegra210.c | 94 +++++++++++++++++++++++++++++++
> drivers/phy/tegra/xusb.c | 6 ++
> drivers/phy/tegra/xusb.h | 4 +-
> 3 files changed, 103 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
> index 3a2d9797fb9f..fe1ab440424d 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -198,6 +198,18 @@
> #define XUSB_PADCTL_UPHY_MISC_PAD_CTL1_AUX_RX_TERM_EN BIT(18)
> #define XUSB_PADCTL_UPHY_MISC_PAD_CTL1_AUX_RX_MODE_OVRD BIT(13)
>
> +#define XUSB_PADCTL_UPHY_MISC_PAD_PX_CTL2(x) (0x464 + (x) * 0x40)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ BIT(0)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ_OVRD BIT(1)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_MASK (0x3 << 4)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_VAL (0x3 << 4)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_PWR_OVRD BIT(24)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ BIT(8)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ_OVRD BIT(9)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_MASK (0x3 << 12)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_VAL (0x3 << 12)
> +#define XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_PWR_OVRD BIT(25)
> +
> #define XUSB_PADCTL_UPHY_PLL_S0_CTL1 0x860
>
> #define XUSB_PADCTL_UPHY_PLL_S0_CTL2 0x864
> @@ -209,6 +221,7 @@
> #define XUSB_PADCTL_UPHY_PLL_S0_CTL8 0x87c
>
> #define XUSB_PADCTL_UPHY_MISC_PAD_S0_CTL1 0x960
> +#define XUSB_PADCTL_UPHY_MISC_PAD_S0_CTL2 0x964
>
> #define XUSB_PADCTL_UPHY_USB3_PADX_ECTL1(x) (0xa60 + (x) * 0x40)
> #define XUSB_PADCTL_UPHY_USB3_PAD_ECTL1_TX_TERM_CTRL_SHIFT 16
> @@ -1636,6 +1649,63 @@ static const struct tegra_xusb_pad_soc tegra210_hsic_pad = {
> .ops = &tegra210_hsic_ops,
> };
>
> +static void
> +tegra210_uphy_lane_iddq_enable(struct tegra_xusb_padctl *padctl, unsigned lane)
> +{
> + u32 val, offset;
> +
> + if (lane <= 6)
> + offset = XUSB_PADCTL_UPHY_MISC_PAD_PX_CTL2(lane);
> + else if (lane == 7)
> + offset = XUSB_PADCTL_UPHY_MISC_PAD_S0_CTL2;
> + else {
> + WARN(true, "invalid lane %u\n", lane);
> + return;
> + }
> +
> + val = padctl_readl(padctl, offset);
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ_OVRD;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ_OVRD;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_PWR_OVRD;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_PWR_OVRD;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ;
> + val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_MASK;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_VAL;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ;
> + val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_MASK;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_VAL;
> + padctl_writel(padctl, val, offset);
> +}
> +
> +static void
> +tegra210_uphy_lane_iddq_disable(struct tegra_xusb_padctl *padctl, unsigned lane)
> +{
> + u32 val, offset;
> +
> + if (lane <= 6)
> + offset = XUSB_PADCTL_UPHY_MISC_PAD_PX_CTL2(lane);
> + else if (lane == 7)
> + offset = XUSB_PADCTL_UPHY_MISC_PAD_S0_CTL2;
> + else {
> + WARN(true, "invalid lane %d\n", lane);
> + return;
> + }
> +
> + val = padctl_readl(padctl, offset);
> + val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ_OVRD;
> + val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ_OVRD;
> + val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_PWR_OVRD;
> + val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_PWR_OVRD;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_IDDQ;
> + val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_MASK;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_TX_SLEEP_VAL;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_IDDQ;
> + val &= ~XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_MASK;
> + val |= XUSB_PADCTL_UPHY_MISC_PAD_CTL2_RX_SLEEP_VAL;
> + padctl_writel(padctl, val, offset);
> +}
> +
> +
> static const char *tegra210_pcie_functions[] = {
> "pcie-x1",
> "usb3-ss",
> @@ -1808,9 +1878,21 @@ static void tegra210_pcie_lane_remove(struct tegra_xusb_lane *lane)
> kfree(pcie);
> }
>
> +static void tegra210_pcie_lane_iddq_enable(struct tegra_xusb_lane *lane)
> +{
> + tegra210_uphy_lane_iddq_enable(lane->pad->padctl, lane->index);
> +}
> +
> +static void tegra210_pcie_lane_iddq_disable(struct tegra_xusb_lane *lane)
> +{
> + tegra210_uphy_lane_iddq_disable(lane->pad->padctl, lane->index);
> +}
> +
> static const struct tegra_xusb_lane_ops tegra210_pcie_lane_ops = {
> .probe = tegra210_pcie_lane_probe,
> .remove = tegra210_pcie_lane_remove,
> + .iddq_enable = tegra210_pcie_lane_iddq_enable,
> + .iddq_disable = tegra210_pcie_lane_iddq_disable,
> };
>
> static int tegra210_pcie_phy_init(struct phy *phy)
> @@ -1971,9 +2053,21 @@ static void tegra210_sata_lane_remove(struct tegra_xusb_lane *lane)
> kfree(sata);
> }
>
> +static void tegra210_sata_lane_iddq_enable(struct tegra_xusb_lane *lane)
> +{
> + tegra210_uphy_lane_iddq_enable(lane->pad->padctl, lane->index + 7);
> +}
> +
> +static void tegra210_sata_lane_iddq_disable(struct tegra_xusb_lane *lane)
> +{
> + tegra210_uphy_lane_iddq_disable(lane->pad->padctl, lane->index + 7);
> +}
This looks like abstraction at the wrong level. You introduce this
arbtitrary offset 7 to differentiate between the two types, whereas what
you really only seem to be after is to get the correct offset.
Can't we instead make tegra210_uphy_lane_iddq_{enable,disable}() take
the offset instead and push the logic to pick the offset into the
callers? We could then have an extra helper that determines the offset
from the lane if we want to avoid duplicating that logic.
Or perhaps an even better way would be to store the offset to this MISC
register in struct tegra_xusb_lane_soc? Something like this perhaps:
struct tegra_xusb_lane_soc {
...
struct {
unsigned int misc;
} regs;
};
That way we don't even have to go through two layers but instead can
operate on the struct tegra_xusb_lane directly.
> +
> static const struct tegra_xusb_lane_ops tegra210_sata_lane_ops = {
> .probe = tegra210_sata_lane_probe,
> .remove = tegra210_sata_lane_remove,
> + .iddq_enable = tegra210_sata_lane_iddq_enable,
> + .iddq_disable = tegra210_sata_lane_iddq_disable,
> };
>
> static int tegra210_sata_phy_init(struct phy *phy)
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index b48b590aa0c1..74abd67e3a31 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -321,11 +321,17 @@ static void tegra_xusb_lane_program(struct tegra_xusb_lane *lane)
> if (soc->num_funcs < 2)
> return;
>
> + if (lane->pad->ops->iddq_enable && lane->pad->ops->iddq_disable)
> + lane->pad->ops->iddq_enable(lane);
You can drop the second check because it isn't relevant here.
> +
> /* choose function */
> value = padctl_readl(padctl, soc->offset);
> value &= ~(soc->mask << soc->shift);
> value |= lane->function << soc->shift;
> padctl_writel(padctl, value, soc->offset);
> +
> + if (lane->pad->ops->iddq_enable && lane->pad->ops->iddq_disable)
> + lane->pad->ops->iddq_disable(lane);
Similarly, the first check can be dropped here because only the second
is relevant. It might make sense to only support IDDQ if both callbacks
are implemented, but that's not something we need to check at this
level. The check here is only to avoid calling a NULL function. If you
absolutely want to do sanity checks, do them at ->probe() time. But I
don't think we need that here. It's up to the developer to get this
right.
> }
>
> static void tegra_xusb_pad_program(struct tegra_xusb_pad *pad)
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index 0c828694cf2d..485b692a3b15 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -22,6 +22,7 @@ struct phy;
> struct phy_provider;
> struct platform_device;
> struct regulator;
> +struct tegra_xusb_padctl;
>
> /*
> * lanes
> @@ -126,6 +127,8 @@ struct tegra_xusb_lane_ops {
> struct device_node *np,
> unsigned int index);
> void (*remove)(struct tegra_xusb_lane *lane);
> + void (*iddq_enable)(struct tegra_xusb_lane *lane);
> + void (*iddq_disable)(struct tegra_xusb_lane *lane);
> };
>
> bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
> @@ -134,7 +137,6 @@ bool tegra_xusb_lane_check(struct tegra_xusb_lane *lane, const char *function);
> * pads
> */
> struct tegra_xusb_pad_soc;
> -struct tegra_xusb_padctl;
>
> struct tegra_xusb_pad_ops {
> struct tegra_xusb_pad *(*probe)(struct tegra_xusb_padctl *padctl,
These last two hunks look like leftovers. I don't see why they are
needed here.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-08-31 11:53 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
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 [this message]
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=20200831115330.GB1689119@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).