[PATCH 0/2] phy: ti: j721e-wiz: Add support to manage type-C swap on Lane2 and lane3

[PATCH 0/2] phy: ti: j721e-wiz: Add support to manage type-C swap on Lane2 and lane3

[Sinthu Raja]

Hi All,
This series of patch add support to enable lane2 and lane3 swap by
configuring the LN23 bit. Also, it's possible that the Type-C plug orientation
on the DIR line will be implemented through hardware design. In that
situation, there won't be an external GPIO line available, but the
driver still needs to address this since the DT won't use the
typec-gpio-dir property. Update code to handle if typec-gpio-dir property
is not specified in DT.

Sinthu Raja (2):
  phy: ti: j721e-wiz: Manage TypeC lane swap if typec-gpio-dir not
    specified
  phy: ti: j721e-wiz: Add support to enable LN23 Type-C swap

 drivers/phy/ti/phy-j721e-wiz.c | 90 ++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 25 deletions(-)

-- 
2.36.1

[PATCH 1/2] phy: ti: j721e-wiz: Manage TypeC lane swap if typec-gpio-dir not specified

[Sinthu Raja]

It's possible that the Type-C plug orientation on the DIR line will be
implemented through hardware design. In that situation, there won't be
an external GPIO line available, but the driver still needs to address
this since the DT won't use the typec-gpio-dir property.

Add code to handle LN10 Type-C swap if typec-gpio-dir property is not
specified in DT.

Remove typec-gpio-dir check to use minimum debounce from Type-C spec if
it is not provided in DT

Signed-off-by: Sinthu Raja <sinthu.raja@ti.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 65 +++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 25 deletions(-)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index 141b51af4427..b17eec632d49 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -375,6 +375,7 @@ struct wiz {
 	struct gpio_desc	*gpio_typec_dir;
 	int			typec_dir_delay;
 	u32 lane_phy_type[WIZ_MAX_LANES];
+	u32 lane_phy_reg[WIZ_MAX_LANES];
 	struct clk		*input_clks[WIZ_MAX_INPUT_CLOCKS];
 	struct clk		*output_clks[WIZ_MAX_OUTPUT_CLOCKS];
 	struct clk_onecell_data	clk_data;
@@ -1231,14 +1232,28 @@ static int wiz_phy_reset_deassert(struct reset_controller_dev *rcdev,
 	int ret;
 
 	/* if typec-dir gpio was specified, set LN10 SWAP bit based on that */
-	if (id == 0 && wiz->gpio_typec_dir) {
-		if (wiz->typec_dir_delay)
-			msleep_interruptible(wiz->typec_dir_delay);
-
-		if (gpiod_get_value_cansleep(wiz->gpio_typec_dir))
-			regmap_field_write(wiz->typec_ln10_swap, 1);
-		else
-			regmap_field_write(wiz->typec_ln10_swap, 0);
+	if (id == 0 && wiz->typec_dir_delay) {
+		msleep_interruptible(wiz->typec_dir_delay);
+
+		if (wiz->gpio_typec_dir) {
+			if (gpiod_get_value_cansleep(wiz->gpio_typec_dir))
+				regmap_field_write(wiz->typec_ln10_swap, 1);
+			else
+				regmap_field_write(wiz->typec_ln10_swap, 0);
+		} else {
+			/* if no typec-dir gpio was specified, and USB lines
+			 * are connected to Lane 0 then set LN10 SWAP bit to 1.
+			 */
+			u32 num_lanes = wiz->num_lanes;
+			int i;
+
+			for (i = 0; i < num_lanes; i++) {
+				if ((wiz->lane_phy_type[i] == PHY_TYPE_USB3) \
+						&& wiz->lane_phy_reg[i] == 0) {
+					regmap_field_write(wiz->typec_ln10_swap, 1);
+				}
+			}
+		}
 	}
 
 	if (id == 0) {
@@ -1370,8 +1385,10 @@ static int wiz_get_lane_phy_types(struct device *dev, struct wiz *wiz)
 		dev_dbg(dev, "%s: Lanes %u-%u have phy-type %u\n", __func__,
 			reg, reg + num_lanes - 1, phy_type);
 
-		for (i = reg; i < reg + num_lanes; i++)
+		for (i = reg; i < reg + num_lanes; i++) {
+			wiz->lane_phy_reg[i] = reg;
 			wiz->lane_phy_type[i] = phy_type;
+		}
 	}
 
 	return 0;
@@ -1464,24 +1481,22 @@ static int wiz_probe(struct platform_device *pdev)
 		goto err_addr_to_resource;
 	}
 
-	if (wiz->gpio_typec_dir) {
-		ret = of_property_read_u32(node, "typec-dir-debounce-ms",
-					   &wiz->typec_dir_delay);
-		if (ret && ret != -EINVAL) {
-			dev_err(dev, "Invalid typec-dir-debounce property\n");
-			goto err_addr_to_resource;
-		}
+	ret = of_property_read_u32(node, "typec-dir-debounce-ms",
+				   &wiz->typec_dir_delay);
+	if (ret && ret != -EINVAL) {
+		dev_err(dev, "Invalid typec-dir-debounce property\n");
+		goto err_addr_to_resource;
+	}
 
-		/* use min. debounce from Type-C spec if not provided in DT  */
-		if (ret == -EINVAL)
-			wiz->typec_dir_delay = WIZ_TYPEC_DIR_DEBOUNCE_MIN;
+	/* use min. debounce from Type-C spec if not provided in DT  */
+	if (ret == -EINVAL)
+		wiz->typec_dir_delay = WIZ_TYPEC_DIR_DEBOUNCE_MIN;
 
-		if (wiz->typec_dir_delay < WIZ_TYPEC_DIR_DEBOUNCE_MIN ||
-		    wiz->typec_dir_delay > WIZ_TYPEC_DIR_DEBOUNCE_MAX) {
-			ret = -EINVAL;
-			dev_err(dev, "Invalid typec-dir-debounce property\n");
-			goto err_addr_to_resource;
-		}
+	if (wiz->typec_dir_delay < WIZ_TYPEC_DIR_DEBOUNCE_MIN ||
+	    wiz->typec_dir_delay > WIZ_TYPEC_DIR_DEBOUNCE_MAX) {
+		ret = -EINVAL;
+		dev_err(dev, "Invalid typec-dir-debounce property\n");
+		goto err_addr_to_resource;
 	}
 
 	ret = wiz_get_lane_phy_types(dev, wiz);
-- 
2.36.1

[PATCH 2/2] phy: ti: j721e-wiz: Add support to enable LN23 Type-C swap

[Sinthu Raja]

Serdes wiz supports both LN23 and LN10 Type-C swap. Add support to
configure LN23 bit to swap between lane2 or lane3 if required.

Signed-off-by: Sinthu Raja <sinthu.raja@ti.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index b17eec632d49..0091892af0b0 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -58,6 +58,11 @@ enum wiz_lane_standard_mode {
 	LANE_MODE_GEN4,
 };
 
+enum wiz_lane_typec_swap_mode {
+	LANE10_SWAP = 0,
+	LANE23_SWAP = 2,
+};
+
 enum wiz_refclk_mux_sel {
 	PLL0_REFCLK,
 	PLL1_REFCLK,
@@ -194,6 +199,9 @@ static const struct reg_field p_mac_div_sel1[WIZ_MAX_LANES] = {
 static const struct reg_field typec_ln10_swap =
 					REG_FIELD(WIZ_SERDES_TYPEC, 30, 30);
 
+static const struct reg_field typec_ln23_swap =
+					REG_FIELD(WIZ_SERDES_TYPEC, 31, 31);
+
 struct wiz_clk_mux {
 	struct clk_hw		hw;
 	struct regmap_field	*field;
@@ -366,6 +374,7 @@ struct wiz {
 	struct regmap_field	*mux_sel_field[WIZ_MUX_NUM_CLOCKS];
 	struct regmap_field	*div_sel_field[WIZ_DIV_NUM_CLOCKS_16G];
 	struct regmap_field	*typec_ln10_swap;
+	struct regmap_field	*typec_ln23_swap;
 	struct regmap_field	*sup_legacy_clk_override;
 
 	struct device		*dev;
@@ -675,6 +684,13 @@ static int wiz_regfield_init(struct wiz *wiz)
 		return PTR_ERR(wiz->typec_ln10_swap);
 	}
 
+	wiz->typec_ln23_swap = devm_regmap_field_alloc(dev, regmap,
+						       typec_ln23_swap);
+	if (IS_ERR(wiz->typec_ln23_swap)) {
+		dev_err(dev, "LN23_SWAP reg field init failed\n");
+		return PTR_ERR(wiz->typec_ln23_swap);
+	}
+
 	wiz->phy_en_refclk = devm_regmap_field_alloc(dev, regmap, phy_en_refclk);
 	if (IS_ERR(wiz->phy_en_refclk)) {
 		dev_err(dev, "PHY_EN_REFCLK reg field init failed\n");
@@ -1242,15 +1258,24 @@ static int wiz_phy_reset_deassert(struct reset_controller_dev *rcdev,
 				regmap_field_write(wiz->typec_ln10_swap, 0);
 		} else {
 			/* if no typec-dir gpio was specified, and USB lines
-			 * are connected to Lane 0 then set LN10 SWAP bit to 1.
+			 * are connected to SWAP lanes '0' or '2' then set LN10 SWAP
+			 * or LN23 bit to 1 respectively.
 			 */
 			u32 num_lanes = wiz->num_lanes;
 			int i;
 
 			for (i = 0; i < num_lanes; i++) {
-				if ((wiz->lane_phy_type[i] == PHY_TYPE_USB3) \
-						&& wiz->lane_phy_reg[i] == 0) {
-					regmap_field_write(wiz->typec_ln10_swap, 1);
+				if (wiz->lane_phy_type[i] == PHY_TYPE_USB3) {
+					switch (wiz->lane_phy_reg[i]) {
+					case LANE10_SWAP:
+						regmap_field_write(wiz->typec_ln10_swap, 1);
+						break;
+					case LANE23_SWAP:
+						regmap_field_write(wiz->typec_ln23_swap, 1);
+						break;
+					default:
+						break;
+					}
 				}
 			}
 		}
-- 
2.36.1

Re: [PATCH 1/2] phy: ti: j721e-wiz: Manage TypeC lane swap if typec-gpio-dir not specified

[Roger Quadros]

Hi Sinthu,

On 13/12/2022 14:48, Sinthu Raja wrote:
> It's possible that the Type-C plug orientation on the DIR line will be
> implemented through hardware design. In that situation, there won't be
> an external GPIO line available, but the driver still needs to address
> this since the DT won't use the typec-gpio-dir property.

The property is actually "typec-dir-gpios"

> 
> Add code to handle LN10 Type-C swap if typec-gpio-dir property is not
> specified in DT.
> 
> Remove typec-gpio-dir check to use minimum debounce from Type-C spec if
> it is not provided in DT

Why?

> 
> Signed-off-by: Sinthu Raja <sinthu.raja@ti.com>
> ---
>  drivers/phy/ti/phy-j721e-wiz.c | 65 +++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> index 141b51af4427..b17eec632d49 100644
> --- a/drivers/phy/ti/phy-j721e-wiz.c
> +++ b/drivers/phy/ti/phy-j721e-wiz.c
> @@ -375,6 +375,7 @@ struct wiz {
>  	struct gpio_desc	*gpio_typec_dir;
>  	int			typec_dir_delay;
>  	u32 lane_phy_type[WIZ_MAX_LANES];
> +	u32 lane_phy_reg[WIZ_MAX_LANES];
>  	struct clk		*input_clks[WIZ_MAX_INPUT_CLOCKS];
>  	struct clk		*output_clks[WIZ_MAX_OUTPUT_CLOCKS];
>  	struct clk_onecell_data	clk_data;
> @@ -1231,14 +1232,28 @@ static int wiz_phy_reset_deassert(struct reset_controller_dev *rcdev,
>  	int ret;
>  
>  	/* if typec-dir gpio was specified, set LN10 SWAP bit based on that */
> -	if (id == 0 && wiz->gpio_typec_dir) {
> -		if (wiz->typec_dir_delay)
> -			msleep_interruptible(wiz->typec_dir_delay);
> -
> -		if (gpiod_get_value_cansleep(wiz->gpio_typec_dir))
> -			regmap_field_write(wiz->typec_ln10_swap, 1);
> -		else
> -			regmap_field_write(wiz->typec_ln10_swap, 0);
> +	if (id == 0 && wiz->typec_dir_delay) {
> +		msleep_interruptible(wiz->typec_dir_delay);

Why do you need to have this debounce delay if there was no GPIO to begin with.
You need to move the msleep call within the next if {} block.

> +
> +		if (wiz->gpio_typec_dir) {
> +			if (gpiod_get_value_cansleep(wiz->gpio_typec_dir))
> +				regmap_field_write(wiz->typec_ln10_swap, 1);
> +			else
> +				regmap_field_write(wiz->typec_ln10_swap, 0);
> +		} else {
> +			/* if no typec-dir gpio was specified, and USB lines
> +			 * are connected to Lane 0 then set LN10 SWAP bit to 1.
> +			 */
> +			u32 num_lanes = wiz->num_lanes;
> +			int i;
> +
> +			for (i = 0; i < num_lanes; i++) {typec-dir-gpios:
> +				if ((wiz->lane_phy_type[i] == PHY_TYPE_USB3) \
> +						&& wiz->lane_phy_reg[i] == 0) {
> +					regmap_field_write(wiz->typec_ln10_swap, 1);
> +				}
> +			}
> +		}
>  	}
>  
>  	if (id == 0) {
> @@ -1370,8 +1385,10 @@ static int wiz_get_lane_phy_types(struct device *dev, struct wiz *wiz)
>  		dev_dbg(dev, "%s: Lanes %u-%u have phy-type %u\n", __func__,
>  			reg, reg + num_lanes - 1, phy_type);
>  
> -		for (i = reg; i < reg + num_lanes; i++)
> +		for (i = reg; i < reg + num_lanes; i++) {
> +			wiz->lane_phy_reg[i] = reg;
>  			wiz->lane_phy_type[i] = phy_type;
> +		}
>  	}
>  
>  	return 0;
> @@ -1464,24 +1481,22 @@ static int wiz_probe(struct platform_device *pdev)
>  		goto err_addr_to_resource;
>  	}
>  
> -	if (wiz->gpio_typec_dir) {
> -		ret = of_property_read_u32(node, "typec-dir-debounce-ms",
> -					   &wiz->typec_dir_delay);
> -		if (ret && ret != -EINVAL) {
> -			dev_err(dev, "Invalid typec-dir-debounce property\n");
> -			goto err_addr_to_resource;
> -		}
> +	ret = of_property_read_u32(node, "typec-dir-debounce-ms",
> +				   &wiz->typec_dir_delay);
> +	if (ret && ret != -EINVAL) {
> +		dev_err(dev, "Invalid typec-dir-debounce property\n");
> +		goto err_addr_to_resource;
> +	}

Why do you need to know this debounce value if you don't have a valid GPIO line?

>  
> -		/* use min. debounce from Type-C spec if not provided in DT  */
> -		if (ret == -EINVAL)
> -			wiz->typec_dir_delay = WIZ_TYPEC_DIR_DEBOUNCE_MIN;
> +	/* use min. debounce from Type-C spec if not provided in DT  */
> +	if (ret == -EINVAL)
> +		wiz->typec_dir_delay = WIZ_TYPEC_DIR_DEBOUNCE_MIN;
>  
> -		if (wiz->typec_dir_delay < WIZ_TYPEC_DIR_DEBOUNCE_MIN ||
> -		    wiz->typec_dir_delay > WIZ_TYPEC_DIR_DEBOUNCE_MAX) {
> -			ret = -EINVAL;
> -			dev_err(dev, "Invalid typec-dir-debounce property\n");
> -			goto err_addr_to_resource;
> -		}
> +	if (wiz->typec_dir_delay < WIZ_TYPEC_DIR_DEBOUNCE_MIN ||
> +	    wiz->typec_dir_delay > WIZ_TYPEC_DIR_DEBOUNCE_MAX) {
> +		ret = -EINVAL;
> +		dev_err(dev, "Invalid typec-dir-debounce property\n");
> +		goto err_addr_to_resource;
>  	}

All these changes are unnecessary.

>  
>  	ret = wiz_get_lane_phy_types(dev, wiz);

cheers,
-roger

Re: [PATCH 2/2] phy: ti: j721e-wiz: Add support to enable LN23 Type-C swap

[Roger Quadros]

On 13/12/2022 14:48, Sinthu Raja wrote:
> Serdes wiz supports both LN23 and LN10 Type-C swap. Add support to

SerDes?

what is wiz?

It has nothing to do with Type-C. It is just a lane swap.
There may or may not be a Type-C port.

> configure LN23 bit to swap between lane2 or lane3 if required.

What do you mean by "swap between lane2 or lane3"?

Do you mean "swap lanes 2 and 3"?

Is LN23 bit supported on all variants?

> 
> Signed-off-by: Sinthu Raja <sinthu.raja@ti.com>
> ---
>  drivers/phy/ti/phy-j721e-wiz.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> index b17eec632d49..0091892af0b0 100644
> --- a/drivers/phy/ti/phy-j721e-wiz.c
> +++ b/drivers/phy/ti/phy-j721e-wiz.c
> @@ -58,6 +58,11 @@ enum wiz_lane_standard_mode {
>  	LANE_MODE_GEN4,
>  };
>  
> +enum wiz_lane_typec_swap_mode {
> +	LANE10_SWAP = 0,
> +	LANE23_SWAP = 2,
> +};

What is this? Is it a register setting?

> +
>  enum wiz_refclk_mux_sel {
>  	PLL0_REFCLK,
>  	PLL1_REFCLK,
> @@ -194,6 +199,9 @@ static const struct reg_field p_mac_div_sel1[WIZ_MAX_LANES] = {
>  static const struct reg_field typec_ln10_swap =
>  					REG_FIELD(WIZ_SERDES_TYPEC, 30, 30);
>  
> +static const struct reg_field typec_ln23_swap =
> +					REG_FIELD(WIZ_SERDES_TYPEC, 31, 31);
> +
>  struct wiz_clk_mux {
>  	struct clk_hw		hw;
>  	struct regmap_field	*field;
> @@ -366,6 +374,7 @@ struct wiz {
>  	struct regmap_field	*mux_sel_field[WIZ_MUX_NUM_CLOCKS];
>  	struct regmap_field	*div_sel_field[WIZ_DIV_NUM_CLOCKS_16G];
>  	struct regmap_field	*typec_ln10_swap;
> +	struct regmap_field	*typec_ln23_swap;
>  	struct regmap_field	*sup_legacy_clk_override;
>  
>  	struct device		*dev;
> @@ -675,6 +684,13 @@ static int wiz_regfield_init(struct wiz *wiz)
>  		return PTR_ERR(wiz->typec_ln10_swap);
>  	}
>  
> +	wiz->typec_ln23_swap = devm_regmap_field_alloc(dev, regmap,
> +						       typec_ln23_swap);
> +	if (IS_ERR(wiz->typec_ln23_swap)) {
> +		dev_err(dev, "LN23_SWAP reg field init failed\n");
> +		return PTR_ERR(wiz->typec_ln23_swap);
> +	}
> +
>  	wiz->phy_en_refclk = devm_regmap_field_alloc(dev, regmap, phy_en_refclk);
>  	if (IS_ERR(wiz->phy_en_refclk)) {
>  		dev_err(dev, "PHY_EN_REFCLK reg field init failed\n");
> @@ -1242,15 +1258,24 @@ static int wiz_phy_reset_deassert(struct reset_controller_dev *rcdev,
>  				regmap_field_write(wiz->typec_ln10_swap, 0);
>  		} else {
>  			/* if no typec-dir gpio was specified, and USB lines
> -			 * are connected to Lane 0 then set LN10 SWAP bit to 1.
> +			 * are connected to SWAP lanes '0' or '2' then set LN10 SWAP
> +			 * or LN23 bit to 1 respectively.
>  			 */
>  			u32 num_lanes = wiz->num_lanes;
>  			int i;
>  
>  			for (i = 0; i < num_lanes; i++) {
> -				if ((wiz->lane_phy_type[i] == PHY_TYPE_USB3) \
> -						&& wiz->lane_phy_reg[i] == 0) {
> -					regmap_field_write(wiz->typec_ln10_swap, 1);
> +				if (wiz->lane_phy_type[i] == PHY_TYPE_USB3) {
> +					switch (wiz->lane_phy_reg[i]) {
> +					case LANE10_SWAP:
> +						regmap_field_write(wiz->typec_ln10_swap, 1);
> +						break;
> +					case LANE23_SWAP:
> +						regmap_field_write(wiz->typec_ln23_swap, 1);
> +						break;
> +					default:
> +						break;
> +					}

Could you please explain what is going on here?
What is the basis for deciding if LN10 or LN23 bit must be set or not?

What about clearing those bits?

>  				}
>  			}
>  		}

cheers,
-roger

Re: [PATCH 1/2] phy: ti: j721e-wiz: Manage TypeC lane swap if typec-gpio-dir not specified

[Roger Quadros]

Hi,

Some more comments below.

On 13/12/2022 14:48, Sinthu Raja wrote:
> It's possible that the Type-C plug orientation on the DIR line will be
> implemented through hardware design. In that situation, there won't be
> an external GPIO line available, but the driver still needs to address
> this since the DT won't use the typec-gpio-dir property.
> 
> Add code to handle LN10 Type-C swap if typec-gpio-dir property is not
> specified in DT.
> 
> Remove typec-gpio-dir check to use minimum debounce from Type-C spec if
> it is not provided in DT
> 
> Signed-off-by: Sinthu Raja <sinthu.raja@ti.com>
> ---
>  drivers/phy/ti/phy-j721e-wiz.c | 65 +++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> index 141b51af4427..b17eec632d49 100644
> --- a/drivers/phy/ti/phy-j721e-wiz.c
> +++ b/drivers/phy/ti/phy-j721e-wiz.c
> @@ -375,6 +375,7 @@ struct wiz {
>  	struct gpio_desc	*gpio_typec_dir;
>  	int			typec_dir_delay;
>  	u32 lane_phy_type[WIZ_MAX_LANES];
> +	u32 lane_phy_reg[WIZ_MAX_LANES];

This name looks misleading. I'll discuss about it where you are setting it.

>  	struct clk		*input_clks[WIZ_MAX_INPUT_CLOCKS];
>  	struct clk		*output_clks[WIZ_MAX_OUTPUT_CLOCKS];
>  	struct clk_onecell_data	clk_data;
> @@ -1231,14 +1232,28 @@ static int wiz_phy_reset_deassert(struct reset_controller_dev *rcdev,
>  	int ret;
>  
>  	/* if typec-dir gpio was specified, set LN10 SWAP bit based on that */
> -	if (id == 0 && wiz->gpio_typec_dir) {
> -		if (wiz->typec_dir_delay)
> -			msleep_interruptible(wiz->typec_dir_delay);
> -
> -		if (gpiod_get_value_cansleep(wiz->gpio_typec_dir))
> -			regmap_field_write(wiz->typec_ln10_swap, 1);
> -		else
> -			regmap_field_write(wiz->typec_ln10_swap, 0);
> +	if (id == 0 && wiz->typec_dir_delay) {
> +		msleep_interruptible(wiz->typec_dir_delay);
> +
> +		if (wiz->gpio_typec_dir) {
> +			if (gpiod_get_value_cansleep(wiz->gpio_typec_dir))
> +				regmap_field_write(wiz->typec_ln10_swap, 1);
> +			else
> +				regmap_field_write(wiz->typec_ln10_swap, 0);
> +		} else {
> +			/* if no typec-dir gpio was specified, and USB lines
> +			 * are connected to Lane 0 then set LN10 SWAP bit to 1.
> +			 */

Why should lanes 1 and 0 be swapped if USB is connected to lane 0?

> +			u32 num_lanes = wiz->num_lanes;
> +			int i;
> +
> +			for (i = 0; i < num_lanes; i++) {
> +				if ((wiz->lane_phy_type[i] == PHY_TYPE_USB3) \
> +						&& wiz->lane_phy_reg[i] == 0) {
> +					regmap_field_write(wiz->typec_ln10_swap, 1);
> +				}
> +			}

I really don't understand what you are doing here.
It definitely doesn't match your comment.
As an example. If num_lanes = 2 then wiz->lane_phy_reg[1] is being used without being
really initialized (see later). Just because of kzalloc, it would of course be 0.

> +		}
>  	}
>  
>  	if (id == 0) {
> @@ -1370,8 +1385,10 @@ static int wiz_get_lane_phy_types(struct device *dev, struct wiz *wiz)
>  		dev_dbg(dev, "%s: Lanes %u-%u have phy-type %u\n", __func__,
>  			reg, reg + num_lanes - 1, phy_type);
>  
> -		for (i = reg; i < reg + num_lanes; i++)
> +		for (i = reg; i < reg + num_lanes; i++) {
> +			wiz->lane_phy_reg[i] = reg;

As per DT binding
      reg:
        description:
          The master lane number. This is the lowest numbered lane in the lane group.

So you are in fact storing the Master lane number of every Link (or lane group).
A link may have 1 or more lanes in it.

Also notice that if num_lanes has been 2 then wiz->lane_phy_reg[1] is not initialized.

>  			wiz->lane_phy_type[i] = phy_type;
> +		}
>  	}
>  
>  	return 0;
> @@ -1464,24 +1481,22 @@ static int wiz_probe(struct platform_device *pdev)
>  		goto err_addr_to_resource;
>  	}
>  
> -	if (wiz->gpio_typec_dir) {
> -		ret = of_property_read_u32(node, "typec-dir-debounce-ms",
> -					   &wiz->typec_dir_delay);
> -		if (ret && ret != -EINVAL) {
> -			dev_err(dev, "Invalid typec-dir-debounce property\n");
> -			goto err_addr_to_resource;
> -		}
> +	ret = of_property_read_u32(node, "typec-dir-debounce-ms",
> +				   &wiz->typec_dir_delay);
> +	if (ret && ret != -EINVAL) {
> +		dev_err(dev, "Invalid typec-dir-debounce property\n");
> +		goto err_addr_to_resource;
> +	}
>  
> -		/* use min. debounce from Type-C spec if not provided in DT  */
> -		if (ret == -EINVAL)
> -			wiz->typec_dir_delay = WIZ_TYPEC_DIR_DEBOUNCE_MIN;
> +	/* use min. debounce from Type-C spec if not provided in DT  */
> +	if (ret == -EINVAL)
> +		wiz->typec_dir_delay = WIZ_TYPEC_DIR_DEBOUNCE_MIN;
>  
> -		if (wiz->typec_dir_delay < WIZ_TYPEC_DIR_DEBOUNCE_MIN ||
> -		    wiz->typec_dir_delay > WIZ_TYPEC_DIR_DEBOUNCE_MAX) {
> -			ret = -EINVAL;
> -			dev_err(dev, "Invalid typec-dir-debounce property\n");
> -			goto err_addr_to_resource;
> -		}
> +	if (wiz->typec_dir_delay < WIZ_TYPEC_DIR_DEBOUNCE_MIN ||
> +	    wiz->typec_dir_delay > WIZ_TYPEC_DIR_DEBOUNCE_MAX) {
> +		ret = -EINVAL;
> +		dev_err(dev, "Invalid typec-dir-debounce property\n");
> +		goto err_addr_to_resource;
>  	}
>  
>  	ret = wiz_get_lane_phy_types(dev, wiz);

cheers,
-roger

Re: [PATCH 2/2] phy: ti: j721e-wiz: Add support to enable LN23 Type-C swap

[Sinthu Raja M]

Hi Roger,
On Wed, Dec 14, 2022 at 2:47 PM Roger Quadros <rogerq@kernel.org> wrote:
>
>
>
> On 13/12/2022 14:48, Sinthu Raja wrote:
> > Serdes wiz supports both LN23 and LN10 Type-C swap. Add support to
>
> SerDes?
>
> what is wiz?
The WIZ acts as a wrapper for the SerDes and can send control signals
to and report status signals from the SerDes, and muxes SerDes to
peripherals.
>
> It has nothing to do with Type-C. It is just a lane swap.
> There may or may not be a Type-C port.
According to the SerDes design, in the case of 4 lanes SerDes, Lane 0
and Lane 2 are reserved for USB for type-C lane swap if Lane 1 and
Lane 3 are integrated into USB3 PHY. The C-type lane swap is
responsible for swapping lanes 0 and 1 or lanes 2 and 3 based on the
configuration register. This allows a Type C USB connector to deal
with the connector orientation.
>
> > configure LN23 bit to swap between lane2 or lane3 if required.
>
> What do you mean by "swap between lane2 or lane3"?
>
> Do you mean "swap lanes 2 and 3"?
Yes.
>
> Is LN23 bit supported on all variants?
Yes, it is supported in all J7 variants if it is a 4 Lanes SerDes and
USB3 PHY is supported.
>
> >
> > Signed-off-by: Sinthu Raja <sinthu.raja@ti.com>
> > ---
> >  drivers/phy/ti/phy-j721e-wiz.c | 33 +++++++++++++++++++++++++++++----
> >  1 file changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> > index b17eec632d49..0091892af0b0 100644
> > --- a/drivers/phy/ti/phy-j721e-wiz.c
> > +++ b/drivers/phy/ti/phy-j721e-wiz.c
> > @@ -58,6 +58,11 @@ enum wiz_lane_standard_mode {
> >       LANE_MODE_GEN4,
> >  };
> >
> > +enum wiz_lane_typec_swap_mode {
> > +     LANE10_SWAP = 0,
> > +     LANE23_SWAP = 2,
> > +};
>
> What is this? Is it a register setting?
These are the master lane numbers that support the C-type lane swap.
Will change the enum name relatable and also shall add a comment for
more clarification.
>
> > +
> >  enum wiz_refclk_mux_sel {
> >       PLL0_REFCLK,
> >       PLL1_REFCLK,
> > @@ -194,6 +199,9 @@ static const struct reg_field p_mac_div_sel1[WIZ_MAX_LANES] = {
> >  static const struct reg_field typec_ln10_swap =
> >                                       REG_FIELD(WIZ_SERDES_TYPEC, 30, 30);
> >
> > +static const struct reg_field typec_ln23_swap =
> > +                                     REG_FIELD(WIZ_SERDES_TYPEC, 31, 31);
> > +
> >  struct wiz_clk_mux {
> >       struct clk_hw           hw;
> >       struct regmap_field     *field;
> > @@ -366,6 +374,7 @@ struct wiz {
> >       struct regmap_field     *mux_sel_field[WIZ_MUX_NUM_CLOCKS];
> >       struct regmap_field     *div_sel_field[WIZ_DIV_NUM_CLOCKS_16G];
> >       struct regmap_field     *typec_ln10_swap;
> > +     struct regmap_field     *typec_ln23_swap;
> >       struct regmap_field     *sup_legacy_clk_override;
> >
> >       struct device           *dev;
> > @@ -675,6 +684,13 @@ static int wiz_regfield_init(struct wiz *wiz)
> >               return PTR_ERR(wiz->typec_ln10_swap);
> >       }
> >
> > +     wiz->typec_ln23_swap = devm_regmap_field_alloc(dev, regmap,
> > +                                                    typec_ln23_swap);
> > +     if (IS_ERR(wiz->typec_ln23_swap)) {
> > +             dev_err(dev, "LN23_SWAP reg field init failed\n");
> > +             return PTR_ERR(wiz->typec_ln23_swap);
> > +     }
> > +
> >       wiz->phy_en_refclk = devm_regmap_field_alloc(dev, regmap, phy_en_refclk);
> >       if (IS_ERR(wiz->phy_en_refclk)) {
> >               dev_err(dev, "PHY_EN_REFCLK reg field init failed\n");
> > @@ -1242,15 +1258,24 @@ static int wiz_phy_reset_deassert(struct reset_controller_dev *rcdev,
> >                               regmap_field_write(wiz->typec_ln10_swap, 0);
> >               } else {
> >                       /* if no typec-dir gpio was specified, and USB lines
> > -                      * are connected to Lane 0 then set LN10 SWAP bit to 1.
> > +                      * are connected to SWAP lanes '0' or '2' then set LN10 SWAP
> > +                      * or LN23 bit to 1 respectively.
> >                        */
> >                       u32 num_lanes = wiz->num_lanes;
> >                       int i;
> >
> >                       for (i = 0; i < num_lanes; i++) {
> > -                             if ((wiz->lane_phy_type[i] == PHY_TYPE_USB3) \
> > -                                             && wiz->lane_phy_reg[i] == 0) {
> > -                                     regmap_field_write(wiz->typec_ln10_swap, 1);
> > +                             if (wiz->lane_phy_type[i] == PHY_TYPE_USB3) {
> > +                                     switch (wiz->lane_phy_reg[i]) {
> > +                                     case LANE10_SWAP:
> > +                                             regmap_field_write(wiz->typec_ln10_swap, 1);
> > +                                             break;
> > +                                     case LANE23_SWAP:
> > +                                             regmap_field_write(wiz->typec_ln23_swap, 1);
> > +                                             break;
> > +                                     default:
> > +                                             break;
> > +                                     }
>
> Could you please explain what is going on here?
> What is the basis for deciding if LN10 or LN23 bit must be set or not?
This snippet is used to configure the SerDes Type C control register
that allows the external lanes selection to be swapped. Based on the
master lane number and if the PHY type is USB3, we are configuring the
lane swap bits.
>

> What about clearing those bits?
According to the design this does not need to be cleared if it is set.
By default, it is set to 0.
>
> >                               }
> >                       }
> >               }
>
> cheers,
> -roger



-- 
With Regards
Sinthu Raja

Re: [PATCH 1/2] phy: ti: j721e-wiz: Manage TypeC lane swap if typec-gpio-dir not specified

[Sinthu Raja M]

Hi Roger,

On Wed, Dec 14, 2022 at 3:12 PM Roger Quadros <rogerq@kernel.org> wrote:
>
> Hi,
>
> Some more comments below.
>
> On 13/12/2022 14:48, Sinthu Raja wrote:
> > It's possible that the Type-C plug orientation on the DIR line will be
> > implemented through hardware design. In that situation, there won't be
> > an external GPIO line available, but the driver still needs to address
> > this since the DT won't use the typec-gpio-dir property.
> >
> > Add code to handle LN10 Type-C swap if typec-gpio-dir property is not
> > specified in DT.
> >
> > Remove typec-gpio-dir check to use minimum debounce from Type-C spec if
> > it is not provided in DT
> >
> > Signed-off-by: Sinthu Raja <sinthu.raja@ti.com>
> > ---
> >  drivers/phy/ti/phy-j721e-wiz.c | 65 +++++++++++++++++++++-------------
> >  1 file changed, 40 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> > index 141b51af4427..b17eec632d49 100644
> > --- a/drivers/phy/ti/phy-j721e-wiz.c
> > +++ b/drivers/phy/ti/phy-j721e-wiz.c
> > @@ -375,6 +375,7 @@ struct wiz {
> >       struct gpio_desc        *gpio_typec_dir;
> >       int                     typec_dir_delay;
> >       u32 lane_phy_type[WIZ_MAX_LANES];
> > +     u32 lane_phy_reg[WIZ_MAX_LANES];
>
> This name looks misleading. I'll discuss about it where you are setting it.
Will change the name to master_lane_num[];
>
> >       struct clk              *input_clks[WIZ_MAX_INPUT_CLOCKS];
> >       struct clk              *output_clks[WIZ_MAX_OUTPUT_CLOCKS];
> >       struct clk_onecell_data clk_data;
> > @@ -1231,14 +1232,28 @@ static int wiz_phy_reset_deassert(struct reset_controller_dev *rcdev,
> >       int ret;
> >
> >       /* if typec-dir gpio was specified, set LN10 SWAP bit based on that */
> > -     if (id == 0 && wiz->gpio_typec_dir) {
> > -             if (wiz->typec_dir_delay)
> > -                     msleep_interruptible(wiz->typec_dir_delay);
> > -
> > -             if (gpiod_get_value_cansleep(wiz->gpio_typec_dir))
> > -                     regmap_field_write(wiz->typec_ln10_swap, 1);
> > -             else
> > -                     regmap_field_write(wiz->typec_ln10_swap, 0);
> > +     if (id == 0 && wiz->typec_dir_delay) {
> > +             msleep_interruptible(wiz->typec_dir_delay);
> > +
> > +             if (wiz->gpio_typec_dir) {
> > +                     if (gpiod_get_value_cansleep(wiz->gpio_typec_dir))
> > +                             regmap_field_write(wiz->typec_ln10_swap, 1);
> > +                     else
> > +                             regmap_field_write(wiz->typec_ln10_swap, 0);
> > +             } else {
> > +                     /* if no typec-dir gpio was specified, and USB lines
> > +                      * are connected to Lane 0 then set LN10 SWAP bit to 1.
> > +                      */
>
> Why should lanes 1 and 0 be swapped if USB is connected to lane 0?
My Bad! I should have been more precise in mentioning the USB3 Type C.
Lanes 0 and 2 are reserved for USB3 for type-C connector lane swap.
>
> > +                     u32 num_lanes = wiz->num_lanes;
> > +                     int i;
> > +
> > +                     for (i = 0; i < num_lanes; i++) {
> > +                             if ((wiz->lane_phy_type[i] == PHY_TYPE_USB3) \
> > +                                             && wiz->lane_phy_reg[i] == 0) {
> > +                                     regmap_field_write(wiz->typec_ln10_swap, 1);
> > +                             }
> > +                     }
>
> I really don't understand what you are doing here.
> It definitely doesn't match your comment.
> As an example. If num_lanes = 2 then wiz->lane_phy_reg[1] is being used without being
> really initialized (see later). Just because of kzalloc, it would of course be 0.
This register is used to configure the external lanes selections that
need to be swapped for SerDes type C. The initialization of the lanes
is done separately. This bit is set to configure that all control for
lane 0 will apply to lane 1 and vice versa.  Will update the commit
description.
>
> > +             }
> >       }
> >
> >       if (id == 0) {
> > @@ -1370,8 +1385,10 @@ static int wiz_get_lane_phy_types(struct device *dev, struct wiz *wiz)
> >               dev_dbg(dev, "%s: Lanes %u-%u have phy-type %u\n", __func__,
> >                       reg, reg + num_lanes - 1, phy_type);
> >
> > -             for (i = reg; i < reg + num_lanes; i++)
> > +             for (i = reg; i < reg + num_lanes; i++) {
> > +                     wiz->lane_phy_reg[i] = reg;
>
> As per DT binding
>       reg:
>         description:
>           The master lane number. This is the lowest numbered lane in the lane group.
>
> So you are in fact storing the Master lane number of every Link (or lane group).
> A link may have 1 or more lanes in it.
>
> Also notice that if num_lanes has been 2 then wiz->lane_phy_reg[1] is not initialized.
Irrespective of the number of lanes that are connected to the link, if
the master lane is '0' or '2'  and the PHY type is USB3, then this bit
needs to be set in the SerDes WIZ control register (according to the
design).
>
> >                       wiz->lane_phy_type[i] = phy_type;
> > +             }
> >       }
> >
> >       return 0;
> > @@ -1464,24 +1481,22 @@ static int wiz_probe(struct platform_device *pdev)
> >               goto err_addr_to_resource;
> >       }
> >
> > -     if (wiz->gpio_typec_dir) {
> > -             ret = of_property_read_u32(node, "typec-dir-debounce-ms",
> > -                                        &wiz->typec_dir_delay);
> > -             if (ret && ret != -EINVAL) {
> > -                     dev_err(dev, "Invalid typec-dir-debounce property\n");
> > -                     goto err_addr_to_resource;
> > -             }
> > +     ret = of_property_read_u32(node, "typec-dir-debounce-ms",
> > +                                &wiz->typec_dir_delay);
> > +     if (ret && ret != -EINVAL) {
> > +             dev_err(dev, "Invalid typec-dir-debounce property\n");
> > +             goto err_addr_to_resource;
> > +     }
> >
> > -             /* use min. debounce from Type-C spec if not provided in DT  */
> > -             if (ret == -EINVAL)
> > -                     wiz->typec_dir_delay = WIZ_TYPEC_DIR_DEBOUNCE_MIN;
> > +     /* use min. debounce from Type-C spec if not provided in DT  */
> > +     if (ret == -EINVAL)
> > +             wiz->typec_dir_delay = WIZ_TYPEC_DIR_DEBOUNCE_MIN;
> >
> > -             if (wiz->typec_dir_delay < WIZ_TYPEC_DIR_DEBOUNCE_MIN ||
> > -                 wiz->typec_dir_delay > WIZ_TYPEC_DIR_DEBOUNCE_MAX) {
> > -                     ret = -EINVAL;
> > -                     dev_err(dev, "Invalid typec-dir-debounce property\n");
> > -                     goto err_addr_to_resource;
> > -             }
> > +     if (wiz->typec_dir_delay < WIZ_TYPEC_DIR_DEBOUNCE_MIN ||
> > +         wiz->typec_dir_delay > WIZ_TYPEC_DIR_DEBOUNCE_MAX) {
> > +             ret = -EINVAL;
> > +             dev_err(dev, "Invalid typec-dir-debounce property\n");
> > +             goto err_addr_to_resource;
> >       }
> >
> >       ret = wiz_get_lane_phy_types(dev, wiz);
>
> cheers,
> -roger



-- 
With Regards
Sinthu Raja