lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 28 Sep 2020 15:50:21 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     JC Kuo <jckuo@...dia.com>
Cc:     gregkh@...uxfoundation.org, robh@...nel.org, jonathanh@...dia.com,
        kishon@...com, linux-tegra@...r.kernel.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, nkristam@...dia.com
Subject: Re: [PATCH v3 12/15] phy: tegra: xusb: Add wake/sleepwalk for
 Tegra186

On Wed, Sep 09, 2020 at 04:10:38PM +0800, JC Kuo wrote:
> This commit implements Tegra186/Tegra194 XUSB PADCTL/AO wake and
> sleepwalk operations.
> 
> Signed-off-by: JC Kuo <jckuo@...dia.com>
> ---
> v3:
>    move 'ao_regs' to the top of 'struct tegra186_xusb_padctl'
>    change return data of .phy_remote_wake_detected() to 'bool'
>    change input parameter of .phy_remote_wake_detected() to 'struct phy*'
>    remove unnecessary 'else'
>    rename 'val' with 'value'
> 
>  drivers/phy/tegra/xusb-tegra186.c | 626 ++++++++++++++++++++++++++++++
>  1 file changed, 626 insertions(+)
> 
> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
> index 5d64f69b39a9..104e2a8496b4 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -113,6 +113,117 @@
>  #define  ID_OVERRIDE_FLOATING			ID_OVERRIDE(8)
>  #define  ID_OVERRIDE_GROUNDED			ID_OVERRIDE(0)
>  
> +/* XUSB AO registers */
> +#define XUSB_AO_USB_DEBOUNCE_DEL		(0x4)
> +#define   UHSIC_LINE_DEB_CNT(x)			(((x) & 0xf) << 4)
> +#define   UTMIP_LINE_DEB_CNT(x)			((x) & 0xf)
> +
> +#define XUSB_AO_UTMIP_TRIGGERS(x)		(0x40 + (x) * 4)
> +#define   CLR_WALK_PTR				(1 << 0)
> +#define   CAP_CFG				(1 << 1)
> +#define   CLR_WAKE_ALARM			(1 << 3)
> +
> +#define XUSB_AO_UHSIC_TRIGGERS(x)		(0x60 + (x) * 4)
> +#define   HSIC_CLR_WALK_PTR			(1 << 0)
> +#define   HSIC_CLR_WAKE_ALARM			(1 << 3)
> +#define   HSIC_CAP_CFG				(1 << 4)
> +
> +#define XUSB_AO_UTMIP_SAVED_STATE(x)		(0x70 + (x) * 4)
> +#define   SPEED(x)				((x) & 0x3)
> +#define     UTMI_HS				SPEED(0)
> +#define     UTMI_FS				SPEED(1)
> +#define     UTMI_LS				SPEED(2)
> +#define     UTMI_RST				SPEED(3)
> +
> +#define XUSB_AO_UHSIC_SAVED_STATE(x)		(0x90 + (x) * 4)
> +#define   MODE(x)				((x) & 0x1)
> +#define   MODE_HS				MODE(0)
> +#define   MODE_RST				MODE(1)
> +
> +#define XUSB_AO_UTMIP_SLEEPWALK_CFG(x)		(0xd0 + (x) * 4)
> +#define XUSB_AO_UHSIC_SLEEPWALK_CFG(x)		(0xf0 + (x) * 4)
> +#define   FAKE_USBOP_VAL			(1 << 0)
> +#define   FAKE_USBON_VAL			(1 << 1)
> +#define   FAKE_USBOP_EN				(1 << 2)
> +#define   FAKE_USBON_EN				(1 << 3)
> +#define   FAKE_STROBE_VAL			(1 << 0)
> +#define   FAKE_DATA_VAL				(1 << 1)
> +#define   FAKE_STROBE_EN			(1 << 2)
> +#define   FAKE_DATA_EN				(1 << 3)
> +#define   WAKE_WALK_EN				(1 << 14)
> +#define   MASTER_ENABLE				(1 << 15)
> +#define   LINEVAL_WALK_EN			(1 << 16)
> +#define   WAKE_VAL(x)				(((x) & 0xf) << 17)
> +#define     WAKE_VAL_NONE			WAKE_VAL(12)
> +#define     WAKE_VAL_ANY			WAKE_VAL(15)
> +#define     WAKE_VAL_DS10			WAKE_VAL(2)
> +#define   LINE_WAKEUP_EN			(1 << 21)
> +#define   MASTER_CFG_SEL			(1 << 22)
> +
> +#define XUSB_AO_UTMIP_SLEEPWALK(x)		(0x100 + (x) * 4)
> +/* phase A */
> +#define   USBOP_RPD_A				(1 << 0)
> +#define   USBON_RPD_A				(1 << 1)
> +#define   AP_A					(1 << 4)
> +#define   AN_A					(1 << 5)
> +#define   HIGHZ_A				(1 << 6)
> +/* phase B */
> +#define   USBOP_RPD_B				(1 << 8)
> +#define   USBON_RPD_B				(1 << 9)
> +#define   AP_B					(1 << 12)
> +#define   AN_B					(1 << 13)
> +#define   HIGHZ_B				(1 << 14)
> +/* phase C */
> +#define   USBOP_RPD_C				(1 << 16)
> +#define   USBON_RPD_C				(1 << 17)
> +#define   AP_C					(1 << 20)
> +#define   AN_C					(1 << 21)
> +#define   HIGHZ_C				(1 << 22)
> +/* phase D */
> +#define   USBOP_RPD_D				(1 << 24)
> +#define   USBON_RPD_D				(1 << 25)
> +#define   AP_D					(1 << 28)
> +#define   AN_D					(1 << 29)
> +#define   HIGHZ_D				(1 << 30)
> +
> +#define XUSB_AO_UHSIC_SLEEPWALK(x)		(0x120 + (x) * 4)
> +/* phase A */
> +#define   RPD_STROBE_A				(1 << 0)
> +#define   RPD_DATA0_A				(1 << 1)
> +#define   RPU_STROBE_A				(1 << 2)
> +#define   RPU_DATA0_A				(1 << 3)
> +/* phase B */
> +#define   RPD_STROBE_B				(1 << 8)
> +#define   RPD_DATA0_B				(1 << 9)
> +#define   RPU_STROBE_B				(1 << 10)
> +#define   RPU_DATA0_B				(1 << 11)
> +/* phase C */
> +#define   RPD_STROBE_C				(1 << 16)
> +#define   RPD_DATA0_C				(1 << 17)
> +#define   RPU_STROBE_C				(1 << 18)
> +#define   RPU_DATA0_C				(1 << 19)
> +/* phase D */
> +#define   RPD_STROBE_D				(1 << 24)
> +#define   RPD_DATA0_D				(1 << 25)
> +#define   RPU_STROBE_D				(1 << 26)
> +#define   RPU_DATA0_D				(1 << 27)
> +
> +#define XUSB_AO_UTMIP_PAD_CFG(x)		(0x130 + (x) * 4)
> +#define   FSLS_USE_XUSB_AO			(1 << 3)
> +#define   TRK_CTRL_USE_XUSB_AO			(1 << 4)
> +#define   RPD_CTRL_USE_XUSB_AO			(1 << 5)
> +#define   RPU_USE_XUSB_AO			(1 << 6)
> +#define   VREG_USE_XUSB_AO			(1 << 7)
> +#define   USBOP_VAL_PD				(1 << 8)
> +#define   USBON_VAL_PD				(1 << 9)
> +#define   E_DPD_OVRD_EN				(1 << 10)
> +#define   E_DPD_OVRD_VAL			(1 << 11)
> +
> +#define XUSB_AO_UHSIC_PAD_CFG(x)		(0x150 + (x) * 4)
> +#define   STROBE_VAL_PD				(1 << 0)
> +#define   DATA0_VAL_PD				(1 << 1)
> +#define   USE_XUSB_AO				(1 << 4)
> +
>  #define TEGRA186_LANE(_name, _offset, _shift, _mask, _type)		\
>  	{								\
>  		.name = _name,						\
> @@ -130,7 +241,15 @@ struct tegra_xusb_fuse_calibration {
>  	u32 rpd_ctrl;
>  };
>  
> +struct tegra186_xusb_padctl_context {
> +	u32 vbus_id;
> +	u32 usb2_pad_mux;
> +	u32 usb2_port_cap;
> +	u32 ss_port_cap;
> +};
> +
>  struct tegra186_xusb_padctl {
> +	void __iomem *ao_regs;
>  	struct tegra_xusb_padctl base;

base should always be the first element in the structure to optimize
container_of().

>  
>  	struct tegra_xusb_fuse_calibration calib;
> @@ -138,8 +257,25 @@ struct tegra186_xusb_padctl {
>  	/* UTMI bias and tracking */
>  	struct clk *usb2_trk_clk;
>  	unsigned int bias_pad_enable;
> +
> +	/* padctl context */
> +	struct tegra186_xusb_padctl_context context;
>  };
>  
> +static inline void ao_writel(struct tegra186_xusb_padctl *priv, u32 value, unsigned long offset)

I prefer offsets to be unsigned int because the _l_ in read_l_() and
write_l_() was originally meant to be "long" (from back when long meant
32-bit). An unsigned long parameter can therefore be easily mistaken for
the value. That's arguably less of an issue with 64-bit because u32 is
an unsigned int. I guess making the offset unsigned int could also be
confusing on 64-bit because now both the value and the offset are
unsigned int, but for compatibility's sake I think that's okay.

Also, offsets are usually pretty small, so a full 64-bit integer isn't
really warranted.

[...]
> +static int tegra186_xusb_padctl_enable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
> +						     struct phy *phy,
> +						     enum usb_device_speed speed)
> +{
> +	if (is_usb3_phy(phy))
> +		return tegra186_usb3_phy_enable_sleepwalk(phy);
> +
> +	if (is_utmi_phy(phy))
> +		return tegra186_utmi_phy_enable_sleepwalk(phy, speed);
> +
> +	return -EINVAL;
> +}
> +
> +static int tegra186_xusb_padctl_disable_phy_sleepwalk(struct tegra_xusb_padctl *padctl,
> +						      struct phy *phy)
> +{
> +	if (is_usb3_phy(phy))
> +		return tegra186_usb3_phy_disable_sleepwalk(phy);
> +
> +	if (is_utmi_phy(phy))
> +		return tegra186_utmi_phy_disable_sleepwalk(phy);
> +
> +	return -EINVAL;
> +}
> +
> +static int tegra186_xusb_padctl_enable_phy_wake(struct tegra_xusb_padctl *padctl, struct phy *phy)
> +{
> +	if (is_usb3_phy(phy))
> +		return tegra186_usb3_phy_enable_wake(phy);
> +
> +	if (is_utmi_phy(phy))
> +		return tegra186_utmi_phy_enable_wake(phy);
> +
> +	return -EINVAL;
> +}
> +
> +static int tegra186_xusb_padctl_disable_phy_wake(struct tegra_xusb_padctl *padctl, struct phy *phy)
> +{
> +	if (is_usb3_phy(phy))
> +		return tegra186_usb3_phy_disable_wake(phy);
> +
> +	if (is_utmi_phy(phy))
> +		return tegra186_utmi_phy_disable_wake(phy);
> +
> +	return -EINVAL;
> +}
> +
> +static bool tegra186_xusb_padctl_remote_wake_detected(struct phy *phy)
> +{
> +	if (is_utmi_phy(phy))
> +		return tegra186_utmi_phy_remote_wake_detected(phy);
> +
> +	if (is_usb3_phy(phy))
> +		return tegra186_usb3_phy_remote_wake_detected(phy);
> +
> +	return false;
> +}

These are pretty much the same multiplexers as for Tegra210. If we had
lane ops function pointers we could make this code more generic.

> +
>  static void tegra186_xusb_padctl_remove(struct tegra_xusb_padctl *padctl)
>  {
>  }
> @@ -937,7 +1556,14 @@ static void tegra186_xusb_padctl_remove(struct tegra_xusb_padctl *padctl)
>  static const struct tegra_xusb_padctl_ops tegra186_xusb_padctl_ops = {
>  	.probe = tegra186_xusb_padctl_probe,
>  	.remove = tegra186_xusb_padctl_remove,
> +	.suspend_noirq = tegra186_xusb_padctl_suspend_noirq,
> +	.resume_noirq = tegra186_xusb_padctl_resume_noirq,
>  	.vbus_override = tegra186_xusb_padctl_vbus_override,
> +	.enable_phy_sleepwalk = tegra186_xusb_padctl_enable_phy_sleepwalk,
> +	.disable_phy_sleepwalk = tegra186_xusb_padctl_disable_phy_sleepwalk,
> +	.enable_phy_wake = tegra186_xusb_padctl_enable_phy_wake,
> +	.disable_phy_wake = tegra186_xusb_padctl_disable_phy_wake,
> +	.remote_wake_detected = tegra186_xusb_padctl_remote_wake_detected,

We might even be able to get rid of these callbacks because the code
that needs to call these can directly call into the lane ops.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ