[PATCH v2 0/2] phy: rockchip: usbdp: improve typec orientation handling

[PATCH v2 0/2] phy: rockchip: usbdp: improve typec orientation handling

[Heiko Stuebner]

Reinit the phy if it's already running when a type-orientation
event happens.

changes in v2:
- fix an error I made when splitting into code-move + re-init
  function should still return 0 after first patch
- use rk_udphy_init() instead of disable + setup combo
  We don't need to disable + reenable clocks

Heiko Stuebner (2):
  phy: rockchip: usbdp: move type-orientation-switch further down
  phy: rockchip: usbdp: re-init the phy on orientation-change

 drivers/phy/rockchip/phy-rockchip-usbdp.c | 171 +++++++++++-----------
 1 file changed, 88 insertions(+), 83 deletions(-)

-- 
2.47.2

[PATCH v2 1/2] phy: rockchip: usbdp: move type-orientation-switch further down

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@cherry.de>

Move the typec-orientation-switch functionality further down, next to
the typec-mux code. Not only brings this the typec-related functionality
closer together, but also the following change needs access to other
driver functions, that are below the current position.

No functional change.

Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/phy/rockchip/phy-rockchip-usbdp.c | 166 +++++++++++-----------
 1 file changed, 83 insertions(+), 83 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
index 5b1e8a3806ed..960cad5b01a9 100644
--- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
+++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
@@ -616,89 +616,6 @@ static void rk_udphy_dp_hpd_event_trigger(struct rk_udphy *udphy, bool hpd)
 	rk_udphy_grfreg_write(udphy->vogrf, &cfg->vogrfcfg[udphy->id].hpd_trigger, hpd);
 }
 
-static void rk_udphy_set_typec_default_mapping(struct rk_udphy *udphy)
-{
-	if (udphy->flip) {
-		udphy->dp_lane_sel[0] = 0;
-		udphy->dp_lane_sel[1] = 1;
-		udphy->dp_lane_sel[2] = 3;
-		udphy->dp_lane_sel[3] = 2;
-		udphy->lane_mux_sel[0] = PHY_LANE_MUX_DP;
-		udphy->lane_mux_sel[1] = PHY_LANE_MUX_DP;
-		udphy->lane_mux_sel[2] = PHY_LANE_MUX_USB;
-		udphy->lane_mux_sel[3] = PHY_LANE_MUX_USB;
-		udphy->dp_aux_dout_sel = PHY_AUX_DP_DATA_POL_INVERT;
-		udphy->dp_aux_din_sel = PHY_AUX_DP_DATA_POL_INVERT;
-		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 1);
-		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 0);
-	} else {
-		udphy->dp_lane_sel[0] = 2;
-		udphy->dp_lane_sel[1] = 3;
-		udphy->dp_lane_sel[2] = 1;
-		udphy->dp_lane_sel[3] = 0;
-		udphy->lane_mux_sel[0] = PHY_LANE_MUX_USB;
-		udphy->lane_mux_sel[1] = PHY_LANE_MUX_USB;
-		udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
-		udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
-		udphy->dp_aux_dout_sel = PHY_AUX_DP_DATA_POL_NORMAL;
-		udphy->dp_aux_din_sel = PHY_AUX_DP_DATA_POL_NORMAL;
-		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 0);
-		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 1);
-	}
-
-	udphy->mode = UDPHY_MODE_DP_USB;
-}
-
-static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
-				 enum typec_orientation orien)
-{
-	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
-
-	mutex_lock(&udphy->mutex);
-
-	if (orien == TYPEC_ORIENTATION_NONE) {
-		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 0);
-		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 0);
-		/* unattached */
-		rk_udphy_usb_bvalid_enable(udphy, false);
-		goto unlock_ret;
-	}
-
-	udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE) ? true : false;
-	rk_udphy_set_typec_default_mapping(udphy);
-	rk_udphy_usb_bvalid_enable(udphy, true);
-
-unlock_ret:
-	mutex_unlock(&udphy->mutex);
-	return 0;
-}
-
-static void rk_udphy_orien_switch_unregister(void *data)
-{
-	struct rk_udphy *udphy = data;
-
-	typec_switch_unregister(udphy->sw);
-}
-
-static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
-{
-	struct typec_switch_desc sw_desc = { };
-
-	sw_desc.drvdata = udphy;
-	sw_desc.fwnode = dev_fwnode(udphy->dev);
-	sw_desc.set = rk_udphy_orien_sw_set;
-
-	udphy->sw = typec_switch_register(udphy->dev, &sw_desc);
-	if (IS_ERR(udphy->sw)) {
-		dev_err(udphy->dev, "Error register typec orientation switch: %ld\n",
-			PTR_ERR(udphy->sw));
-		return PTR_ERR(udphy->sw);
-	}
-
-	return devm_add_action_or_reset(udphy->dev,
-					rk_udphy_orien_switch_unregister, udphy);
-}
-
 static int rk_udphy_refclk_set(struct rk_udphy *udphy)
 {
 	unsigned long rate;
@@ -1323,6 +1240,89 @@ static const struct phy_ops rk_udphy_usb3_phy_ops = {
 	.owner		= THIS_MODULE,
 };
 
+static void rk_udphy_set_typec_default_mapping(struct rk_udphy *udphy)
+{
+	if (udphy->flip) {
+		udphy->dp_lane_sel[0] = 0;
+		udphy->dp_lane_sel[1] = 1;
+		udphy->dp_lane_sel[2] = 3;
+		udphy->dp_lane_sel[3] = 2;
+		udphy->lane_mux_sel[0] = PHY_LANE_MUX_DP;
+		udphy->lane_mux_sel[1] = PHY_LANE_MUX_DP;
+		udphy->lane_mux_sel[2] = PHY_LANE_MUX_USB;
+		udphy->lane_mux_sel[3] = PHY_LANE_MUX_USB;
+		udphy->dp_aux_dout_sel = PHY_AUX_DP_DATA_POL_INVERT;
+		udphy->dp_aux_din_sel = PHY_AUX_DP_DATA_POL_INVERT;
+		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 1);
+		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 0);
+	} else {
+		udphy->dp_lane_sel[0] = 2;
+		udphy->dp_lane_sel[1] = 3;
+		udphy->dp_lane_sel[2] = 1;
+		udphy->dp_lane_sel[3] = 0;
+		udphy->lane_mux_sel[0] = PHY_LANE_MUX_USB;
+		udphy->lane_mux_sel[1] = PHY_LANE_MUX_USB;
+		udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
+		udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
+		udphy->dp_aux_dout_sel = PHY_AUX_DP_DATA_POL_NORMAL;
+		udphy->dp_aux_din_sel = PHY_AUX_DP_DATA_POL_NORMAL;
+		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 0);
+		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 1);
+	}
+
+	udphy->mode = UDPHY_MODE_DP_USB;
+}
+
+static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
+				 enum typec_orientation orien)
+{
+	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
+
+	mutex_lock(&udphy->mutex);
+
+	if (orien == TYPEC_ORIENTATION_NONE) {
+		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 0);
+		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 0);
+		/* unattached */
+		rk_udphy_usb_bvalid_enable(udphy, false);
+		goto unlock_ret;
+	}
+
+	udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE) ? true : false;
+	rk_udphy_set_typec_default_mapping(udphy);
+	rk_udphy_usb_bvalid_enable(udphy, true);
+
+unlock_ret:
+	mutex_unlock(&udphy->mutex);
+	return 0;
+}
+
+static void rk_udphy_orien_switch_unregister(void *data)
+{
+	struct rk_udphy *udphy = data;
+
+	typec_switch_unregister(udphy->sw);
+}
+
+static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
+{
+	struct typec_switch_desc sw_desc = { };
+
+	sw_desc.drvdata = udphy;
+	sw_desc.fwnode = dev_fwnode(udphy->dev);
+	sw_desc.set = rk_udphy_orien_sw_set;
+
+	udphy->sw = typec_switch_register(udphy->dev, &sw_desc);
+	if (IS_ERR(udphy->sw)) {
+		dev_err(udphy->dev, "Error register typec orientation switch: %ld\n",
+			PTR_ERR(udphy->sw));
+		return PTR_ERR(udphy->sw);
+	}
+
+	return devm_add_action_or_reset(udphy->dev,
+					rk_udphy_orien_switch_unregister, udphy);
+}
+
 static int rk_udphy_typec_mux_set(struct typec_mux_dev *mux,
 				  struct typec_mux_state *state)
 {
-- 
2.47.2

[PATCH v2 2/2] phy: rockchip: usbdp: re-init the phy on orientation-change

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@cherry.de>

Until now the usbdp in the orientation-handler set the new lane setup in
its internal state variables and adapted the sbu gpios as needed.
It never actually updated the phy itself though, but relied on the
controlling usb-controller to disable and re-enable the phy.

And while on the vendor-kernel, I could see that on every unplug the dwc3
did go to its suspend and woke up on the next device plug-in event,
thus toggling the phy as needed, this does not happen in all cases and we
should not rely on that behaviour.

This results in the usb2 always working, as it's not affected by the
orientation, but usb3 only working in one direction right now.

So similar to how the update works in the power-on callback, just re-init
the phy if it's already running when the orientation-event happens.

Both the power-on/-off functions as well as the orientation-set callback
work with the usbdp-mutex held, so can't conflict.

The behaviour is similar to how the qcom qmp phys handle the orientaton
re-init - by re-initting the phy.

Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/phy/rockchip/phy-rockchip-usbdp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
index 960cad5b01a9..c07b79da5b6b 100644
--- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
+++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
@@ -1277,6 +1277,7 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
 				 enum typec_orientation orien)
 {
 	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
+	int ret = 0;
 
 	mutex_lock(&udphy->mutex);
 
@@ -1292,9 +1293,13 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
 	rk_udphy_set_typec_default_mapping(udphy);
 	rk_udphy_usb_bvalid_enable(udphy, true);
 
+	/* re-init the phy if already on */
+	if (udphy->status != UDPHY_MODE_NONE)
+		ret = rk_udphy_init(udphy);
+
 unlock_ret:
 	mutex_unlock(&udphy->mutex);
-	return 0;
+	return ret;
 }
 
 static void rk_udphy_orien_switch_unregister(void *data)
-- 
2.47.2

Re: [PATCH v2 1/2] phy: rockchip: usbdp: move type-orientation-switch further down

[Quentin Schulz]

Hi Heiko,

On 2/26/25 11:38 AM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> Move the typec-orientation-switch functionality further down, next to
> the typec-mux code. Not only brings this the typec-related functionality
> closer together, but also the following change needs access to other
> driver functions, that are below the current position.
> 
> No functional change.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>

Checked with `meld`, the - and + diff are identical.

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 2/2] phy: rockchip: usbdp: re-init the phy on orientation-change

[Quentin Schulz]

Hi Heiko,

On 2/26/25 11:38 AM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> Until now the usbdp in the orientation-handler set the new lane setup in
> its internal state variables and adapted the sbu gpios as needed.
> It never actually updated the phy itself though, but relied on the
> controlling usb-controller to disable and re-enable the phy.
> 
> And while on the vendor-kernel, I could see that on every unplug the dwc3
> did go to its suspend and woke up on the next device plug-in event,
> thus toggling the phy as needed, this does not happen in all cases and we
> should not rely on that behaviour.
> 
> This results in the usb2 always working, as it's not affected by the
> orientation, but usb3 only working in one direction right now.
> 
> So similar to how the update works in the power-on callback, just re-init
> the phy if it's already running when the orientation-event happens.
> 
> Both the power-on/-off functions as well as the orientation-set callback
> work with the usbdp-mutex held, so can't conflict.
> 
> The behaviour is similar to how the qcom qmp phys handle the orientaton
> re-init - by re-initting the phy.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>

Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # RK3588 Jaguar

> ---
>   drivers/phy/rockchip/phy-rockchip-usbdp.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 960cad5b01a9..c07b79da5b6b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1277,6 +1277,7 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>   				 enum typec_orientation orien)
>   {
>   	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
> +	int ret = 0;
>   
>   	mutex_lock(&udphy->mutex);
>   
> @@ -1292,9 +1293,13 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,

Unrelated to this patch (but may be triggered by this patch?), I'm 
wondering how flip is really handled.

It seems like we have flip store the orientation of the cable, but also 
if rockchip,dp-lane-mux is set to <0 1>. But wouldn't that break if we 
ignore that initial flipped lane-mux whenever a USB-C cable is inserted 
in reverse? Basically, shouldn't a reserve orientation of the cable when 
rockchip,dp-lane-mux is set to <0 1> mean "normal mux"?

Cheers,
Quentin

Re: [PATCH v2 2/2] phy: rockchip: usbdp: re-init the phy on orientation-change

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

Hello Quentin,

On Wed, Feb 26, 2025 at 01:38:10PM +0100, Quentin Schulz wrote:
> Unrelated to this patch (but may be triggered by this patch?), I'm wondering
> how flip is really handled.
> 
> It seems like we have flip store the orientation of the cable, but also if
> rockchip,dp-lane-mux is set to <0 1>. But wouldn't that break if we ignore
> that initial flipped lane-mux whenever a USB-C cable is inserted in reverse?
> Basically, shouldn't a reserve orientation of the cable when
> rockchip,dp-lane-mux is set to <0 1> mean "normal mux"?

If a USB-C connector is involved, the TypeC controller is supposed to
setup the lane muxing based on the connector orientation. This
happens via the typec API and in this hardware setup the PHY should
not have the rockchip,dp-lane-mux DT property set.

The rockchip,dp-lane-mux property is required if no USB-C connector
is involved. For example if the lanes are routed to a Displayport
connector. In that case the lane setup is fixed in hardware and
there is no TypeC controller involved, which could do any setup ;)

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] phy: rockchip: usbdp: re-init the phy on orientation-change

[Quentin Schulz]

Hi Sebastian,

On 3/1/25 10:19 PM, Sebastian Reichel wrote:
> Hello Quentin,
> 
> On Wed, Feb 26, 2025 at 01:38:10PM +0100, Quentin Schulz wrote:
>> Unrelated to this patch (but may be triggered by this patch?), I'm wondering
>> how flip is really handled.
>>
>> It seems like we have flip store the orientation of the cable, but also if
>> rockchip,dp-lane-mux is set to <0 1>. But wouldn't that break if we ignore
>> that initial flipped lane-mux whenever a USB-C cable is inserted in reverse?
>> Basically, shouldn't a reserve orientation of the cable when
>> rockchip,dp-lane-mux is set to <0 1> mean "normal mux"?
> 
> If a USB-C connector is involved, the TypeC controller is supposed to
> setup the lane muxing based on the connector orientation. This
> happens via the typec API and in this hardware setup the PHY should
> not have the rockchip,dp-lane-mux DT property set.
> 

I could see some HW routing "mistake" where the USB-C connector in 
normal orientation has DP lanes routed to RX1/TX1? Or is this expected 
to just be faulty HW we shouldn't attempt at supporting?

> The rockchip,dp-lane-mux property is required if no USB-C connector
> is involved. For example if the lanes are routed to a Displayport
> connector. In that case the lane setup is fixed in hardware and
> there is no TypeC controller involved, which could do any setup ;)
> 

Yup I've seen that for the Rock 5 ITX and the evaluation board(s) do 
this. Quite interesting :)

Cheers,
Quentin

Re: [PATCH v2 2/2] phy: rockchip: usbdp: re-init the phy on orientation-change

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]

Hi,

On Mon, Mar 03, 2025 at 10:27:31AM +0100, Quentin Schulz wrote:
> On 3/1/25 10:19 PM, Sebastian Reichel wrote:
> > On Wed, Feb 26, 2025 at 01:38:10PM +0100, Quentin Schulz wrote:
> > > Unrelated to this patch (but may be triggered by this patch?), I'm wondering
> > > how flip is really handled.
> > > 
> > > It seems like we have flip store the orientation of the cable, but also if
> > > rockchip,dp-lane-mux is set to <0 1>. But wouldn't that break if we ignore
> > > that initial flipped lane-mux whenever a USB-C cable is inserted in reverse?
> > > Basically, shouldn't a reserve orientation of the cable when
> > > rockchip,dp-lane-mux is set to <0 1> mean "normal mux"?
> > 
> > If a USB-C connector is involved, the TypeC controller is supposed to
> > setup the lane muxing based on the connector orientation. This
> > happens via the typec API and in this hardware setup the PHY should
> > not have the rockchip,dp-lane-mux DT property set.
> > 
> 
> I could see some HW routing "mistake" where the USB-C connector in normal
> orientation has DP lanes routed to RX1/TX1? Or is this expected to just be
> faulty HW we shouldn't attempt at supporting?

You mean somebody routing the RK3588 SSTX1 and SSRX1 pins to SSTX2
and SSRX2 of the TypeC connector and vice versa and thus effectively
inverting the orientation on their board? I would say let's worry
about that once somebody comes up with such a cursed hardware design.

Note, that rockchip,dp-lane-mux wouldn't be a good property for this
setup either. With USB-C you don't necessarily have 2 lanes USB3 and
2 lanes DP. You can also have 4 lanes USB3 (not supported by RK3588)
or 4 lanes DP (should be supported by RK3588 hardware). So
hardwiring the mux is a bad idea. Probably would require some flag
for the TypeC orientation switch to handle the orientation
information inverted.

> > The rockchip,dp-lane-mux property is required if no USB-C connector
> > is involved. For example if the lanes are routed to a Displayport
> > connector. In that case the lane setup is fixed in hardware and
> > there is no TypeC controller involved, which could do any setup ;)
> > 
> 
> Yup I've seen that for the Rock 5 ITX and the evaluation board(s) do this.
> Quite interesting :)
> 
> Cheers,
> Quentin

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]