From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: Konrad Dybcio <konradybcio@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
usb4-upstream@oss.qualcomm.com,
Raghavendra Thoorpu <rthoorpu@qti.qualcomm.com>,
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>,
Jack Pham <jack.pham@oss.qualcomm.com>
Subject: Re: [PATCH] usb: typec: mux: ps883x: Power the retimer off when not in use
Date: Mon, 27 Apr 2026 12:22:39 +0300 [thread overview]
Message-ID: <ae8q3yLta16oS5At@kuha> (raw)
In-Reply-To: <20260420-topic-ps883x_unused_reset-v1-1-7aabf7004d2a@oss.qualcomm.com>
On Mon, Apr 20, 2026 at 01:40:28PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> When there's nothing going through the retimer, there's no reason to
> keep it online. Put it in reset when possible to save power.
>
> Also, remove the register cache-compare optimization as it makes little
> sense now that the chip resets during almost all transitions and
> tracking the validity of that cache becomes a headache.
>
> Suggested-by: Jack Pham <jack.pham@oss.qualcomm.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Note most of the diff happens to be there because I need to move
> ps883x_(en/dis)able_vregs() around..
> ---
> drivers/usb/typec/mux/ps883x.c | 196 ++++++++++++++++++++++++-----------------
> 1 file changed, 114 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
> index 1256252eceed..f52443638ee2 100644
> --- a/drivers/usb/typec/mux/ps883x.c
> +++ b/drivers/usb/typec/mux/ps883x.c
> @@ -61,19 +61,110 @@ struct ps883x_retimer {
> struct mutex lock; /* protect non-concurrent retimer & switch */
>
> enum typec_orientation orientation;
> - u8 cfg0;
> - u8 cfg1;
> - u8 cfg2;
> + bool in_reset;
> };
>
> +static int ps883x_enable_vregs(struct ps883x_retimer *retimer)
> +{
> + struct device *dev = &retimer->client->dev;
> + int ret;
> +
> + ret = regulator_enable(retimer->vdd33_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD 3.3V regulator: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regulator_enable(retimer->vdd33_cap_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD 3.3V CAP regulator: %d\n", ret);
> + goto err_vdd33_disable;
> + }
> +
> + usleep_range(4000, 10000);
> +
> + ret = regulator_enable(retimer->vdd_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD regulator: %d\n", ret);
> + goto err_vdd33_cap_disable;
> + }
> +
> + ret = regulator_enable(retimer->vddar_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD AR regulator: %d\n", ret);
> + goto err_vdd_disable;
> + }
> +
> + ret = regulator_enable(retimer->vddat_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD AT regulator: %d\n", ret);
> + goto err_vddar_disable;
> + }
> +
> + ret = regulator_enable(retimer->vddio_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD IO regulator: %d\n", ret);
> + goto err_vddat_disable;
> + }
> +
> + return 0;
> +
> +err_vddat_disable:
> + regulator_disable(retimer->vddat_supply);
> +err_vddar_disable:
> + regulator_disable(retimer->vddar_supply);
> +err_vdd_disable:
> + regulator_disable(retimer->vdd_supply);
> +err_vdd33_cap_disable:
> + regulator_disable(retimer->vdd33_cap_supply);
> +err_vdd33_disable:
> + regulator_disable(retimer->vdd33_supply);
> +
> + return ret;
> +}
> +
> +static void ps883x_disable_vregs(struct ps883x_retimer *retimer)
> +{
> + regulator_disable(retimer->vddio_supply);
> + regulator_disable(retimer->vddat_supply);
> + regulator_disable(retimer->vddar_supply);
> + regulator_disable(retimer->vdd_supply);
> + regulator_disable(retimer->vdd33_cap_supply);
> + regulator_disable(retimer->vdd33_supply);
> +}
> +
> +static void ps883x_reset(struct ps883x_retimer *retimer)
> +{
> + if (retimer->in_reset)
> + return;
> +
> + gpiod_set_value(retimer->reset_gpio, 1);
> + ps883x_disable_vregs(retimer);
> + retimer->in_reset = true;
> +}
> +
> static int ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
> - int cfg1, int cfg2)
> + int cfg1, int cfg2, bool reset)
> {
> struct device *dev = &retimer->client->dev;
> int ret;
>
> - if (retimer->cfg0 == cfg0 && retimer->cfg1 == cfg1 && retimer->cfg2 == cfg2)
> + if (reset) {
> + ps883x_reset(retimer);
> +
> return 0;
> + } else if (retimer->in_reset) {
> + ret = ps883x_enable_vregs(retimer);
> + if (ret)
> + return ret;
> +
> + gpiod_set_value(retimer->reset_gpio, 0);
> +
> + /* firmware initialization delay */
> + msleep(60);
> +
> + retimer->in_reset = false;
> + }
>
> ret = regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_0, cfg0);
> if (ret) {
> @@ -93,10 +184,6 @@ static int ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
> return ret;
> }
>
> - retimer->cfg0 = cfg0;
> - retimer->cfg1 = cfg1;
> - retimer->cfg2 = cfg2;
> -
> return 0;
> }
>
> @@ -107,6 +194,7 @@ static int ps883x_set(struct ps883x_retimer *retimer, struct typec_retimer_state
> int cfg0 = CONN_STATUS_0_CONNECTION_PRESENT;
> int cfg1 = 0x00;
> int cfg2 = 0x00;
> + bool reset = false;
>
> if (retimer->orientation == TYPEC_ORIENTATION_REVERSE)
> cfg0 |= CONN_STATUS_0_ORIENTATION_REVERSED;
> @@ -148,9 +236,13 @@ static int ps883x_set(struct ps883x_retimer *retimer, struct typec_retimer_state
> }
> } else {
> switch (state->mode) {
> + /* SAFE can be transient or point to an actual disconnect */
> case TYPEC_STATE_SAFE:
> + reset = retimer->orientation == TYPEC_ORIENTATION_NONE;
> + break;
> /* USB2 pins don't even go through this chip */
> case TYPEC_MODE_USB2:
> + reset = true;
> break;
> case TYPEC_STATE_USB:
> case TYPEC_MODE_USB3:
> @@ -171,7 +263,7 @@ static int ps883x_set(struct ps883x_retimer *retimer, struct typec_retimer_state
> }
> }
>
> - return ps883x_configure(retimer, cfg0, cfg1, cfg2);
> + return ps883x_configure(retimer, cfg0, cfg1, cfg2, reset);
> }
>
> static int ps883x_sw_set(struct typec_switch_dev *sw,
> @@ -184,11 +276,19 @@ static int ps883x_sw_set(struct typec_switch_dev *sw,
> if (ret)
> return ret;
>
> - mutex_lock(&retimer->lock);
> + guard(mutex)(&retimer->lock);
>
> if (retimer->orientation != orientation) {
> retimer->orientation = orientation;
>
> + /*
> + * Orientation notifications usually come prior to mode switch
> + * events. If the retimer is already in reset, we still want to
> + * cache the new orientation value for the subsequent ps883x_set().
> + */
> + if (retimer->in_reset)
> + return 0;
> +
> ret = regmap_assign_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> CONN_STATUS_0_ORIENTATION_REVERSED,
> orientation == TYPEC_ORIENTATION_REVERSE);
> @@ -196,8 +296,6 @@ static int ps883x_sw_set(struct typec_switch_dev *sw,
> dev_err(&retimer->client->dev, "failed to set orientation: %d\n", ret);
> }
>
> - mutex_unlock(&retimer->lock);
> -
> return ret;
> }
>
> @@ -222,75 +320,6 @@ static int ps883x_retimer_set(struct typec_retimer *rtmr,
> return typec_mux_set(retimer->typec_mux, &mux_state);
> }
>
> -static int ps883x_enable_vregs(struct ps883x_retimer *retimer)
> -{
> - struct device *dev = &retimer->client->dev;
> - int ret;
> -
> - ret = regulator_enable(retimer->vdd33_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD 3.3V regulator: %d\n", ret);
> - return ret;
> - }
> -
> - ret = regulator_enable(retimer->vdd33_cap_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD 3.3V CAP regulator: %d\n", ret);
> - goto err_vdd33_disable;
> - }
> -
> - usleep_range(4000, 10000);
> -
> - ret = regulator_enable(retimer->vdd_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD regulator: %d\n", ret);
> - goto err_vdd33_cap_disable;
> - }
> -
> - ret = regulator_enable(retimer->vddar_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD AR regulator: %d\n", ret);
> - goto err_vdd_disable;
> - }
> -
> - ret = regulator_enable(retimer->vddat_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD AT regulator: %d\n", ret);
> - goto err_vddar_disable;
> - }
> -
> - ret = regulator_enable(retimer->vddio_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD IO regulator: %d\n", ret);
> - goto err_vddat_disable;
> - }
> -
> - return 0;
> -
> -err_vddat_disable:
> - regulator_disable(retimer->vddat_supply);
> -err_vddar_disable:
> - regulator_disable(retimer->vddar_supply);
> -err_vdd_disable:
> - regulator_disable(retimer->vdd_supply);
> -err_vdd33_cap_disable:
> - regulator_disable(retimer->vdd33_cap_supply);
> -err_vdd33_disable:
> - regulator_disable(retimer->vdd33_supply);
> -
> - return ret;
> -}
> -
> -static void ps883x_disable_vregs(struct ps883x_retimer *retimer)
> -{
> - regulator_disable(retimer->vddio_supply);
> - regulator_disable(retimer->vddat_supply);
> - regulator_disable(retimer->vddar_supply);
> - regulator_disable(retimer->vdd_supply);
> - regulator_disable(retimer->vdd33_cap_supply);
> - regulator_disable(retimer->vdd33_supply);
> -}
> -
> static int ps883x_get_vregs(struct ps883x_retimer *retimer)
> {
> struct device *dev = &retimer->client->dev;
> @@ -422,6 +451,9 @@ static int ps883x_retimer_probe(struct i2c_client *client)
> }
> }
>
> + /* Keep the retimer in reset until a Type-C notification comes */
> + ps883x_reset(retimer);
> +
> sw_desc.drvdata = retimer;
> sw_desc.fwnode = dev_fwnode(dev);
> sw_desc.set = ps883x_sw_set;
>
> ---
> base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
> change-id: 20260420-topic-ps883x_unused_reset-9af0909cefac
>
> Best regards,
> --
> Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
--
heikki
next prev parent reply other threads:[~2026-04-27 9:22 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-20 11:40 [PATCH] usb: typec: mux: ps883x: Power the retimer off when not in use Konrad Dybcio
2026-04-24 16:31 ` Anthony Ruhier
2026-04-27 9:22 ` Heikki Krogerus [this message]
2026-04-28 21:35 ` Sebastian Reichel
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=ae8q3yLta16oS5At@kuha \
--to=heikki.krogerus@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=jack.pham@oss.qualcomm.com \
--cc=konrad.dybcio@oss.qualcomm.com \
--cc=konradybcio@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=rthoorpu@qti.qualcomm.com \
--cc=usb4-upstream@oss.qualcomm.com \
/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.