Re: [PATCH v2 04/12] phy: tegra: xusb: t210: add lane_iddq operations

From: Thierry Reding
Date: Mon Aug 31 2020 - 07:53:43 EST

Next message: Christian Borntraeger: "Re: [RFC PATCH 0/2] mm/gup: fix gup_fast with dynamic page table folding"
Previous message: Jason Gunthorpe: "Re: [PATCH] nouveau: fix the start/end range for migration"
In reply to: JC Kuo: "[PATCH v2 04/12] phy: tegra: xusb: t210: add lane_iddq operations"
Next in thread: JC Kuo: "[PATCH v2 05/12] phy: tegra: xusb: add sleepwalk and suspend/resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> ---
> 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: signature.asc
Description: PGP signature

Next message: Christian Borntraeger: "Re: [RFC PATCH 0/2] mm/gup: fix gup_fast with dynamic page table folding"
Previous message: Jason Gunthorpe: "Re: [PATCH] nouveau: fix the start/end range for migration"
In reply to: JC Kuo: "[PATCH v2 04/12] phy: tegra: xusb: t210: add lane_iddq operations"
Next in thread: JC Kuo: "[PATCH v2 05/12] phy: tegra: xusb: add sleepwalk and suspend/resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]