devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler
@ 2025-04-16 12:01 Pengyu Luo
  2025-04-16 12:02 ` [PATCH v2 1/2] dt-bindings: phy: qcom,snps-eusb2-repeater: Add more tuning overrides Pengyu Luo
  2025-04-16 12:02 ` [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler Pengyu Luo
  0 siblings, 2 replies; 8+ messages in thread
From: Pengyu Luo @ 2025-04-16 12:01 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pengyu Luo, Stephen Rothwell,
	Abel Vesa
  Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel

In downstream tree, many registers need to be overrided, it varies
from devices and platforms, with these registers getting more, adding
a handler for this is helpful.

Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
Changes in v2:
- Drop dts changes, backwards compatible now
- Changes for dt-bindings (Krzysztof)
- Drop sequence overrides to use single property (Dmitry)
- Link to v1: https://lore.kernel.org/linux-arm-msm/20250405174319.405975-1-mitltlatltl@gmail.com

---
Pengyu Luo (2):
  dt-bindings: phy: qcom,snps-eusb2-repeater: Add more tuning overrides
  phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler

 .../phy/qcom,snps-eusb2-repeater.yaml         |  41 ++++++-
 .../phy/qualcomm/phy-qcom-eusb2-repeater.c    | 105 +++++++++++++++---
 2 files changed, 130 insertions(+), 16 deletions(-)
---
base-commit: afd30f00ef898643491ce0c0c2571b3498f1a61e

-- 
2.49.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/2] dt-bindings: phy: qcom,snps-eusb2-repeater: Add more tuning overrides
  2025-04-16 12:01 [PATCH v2 0/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler Pengyu Luo
@ 2025-04-16 12:02 ` Pengyu Luo
  2025-04-21 21:32   ` Rob Herring
  2025-04-16 12:02 ` [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler Pengyu Luo
  1 sibling, 1 reply; 8+ messages in thread
From: Pengyu Luo @ 2025-04-16 12:02 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pengyu Luo, Stephen Rothwell,
	Abel Vesa
  Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel

In downstream tree, many registers need to be overridden, it varies
from devices and platforms, not only HS trasmit amplitude(0x51),
HS disconnect threshold(0x53), Tx pre-emphasis tuning(0x57). And
some devices require different overrides for different dr_mode. So
add property entries for host mode, original entries will be used
for device mode.

Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
 .../phy/qcom,snps-eusb2-repeater.yaml         | 41 +++++++++++++++++--
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml b/Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml
index d16a543a7..d0f5e770d 100644
--- a/Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml
@@ -36,25 +36,60 @@ properties:
 
   qcom,tune-usb2-disc-thres:
     $ref: /schemas/types.yaml#/definitions/uint8
-    description: High-Speed disconnect threshold
+    description: High-Speed disconnect threshold for device mode
     minimum: 0
     maximum: 7
     default: 0
 
   qcom,tune-usb2-amplitude:
     $ref: /schemas/types.yaml#/definitions/uint8
-    description: High-Speed transmit amplitude
+    description: High-Speed transmit amplitude for device mode
     minimum: 0
     maximum: 15
     default: 8
 
   qcom,tune-usb2-preem:
     $ref: /schemas/types.yaml#/definitions/uint8
-    description: High-Speed TX pre-emphasis tuning
+    description: High-Speed TX pre-emphasis tuning for device mode
     minimum: 0
     maximum: 7
     default: 5
 
+  qcom,tune-usb2-squelch:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description: Squelch detection threshold tuning for device mode
+    minimum: 0
+    maximum: 7
+    default: 3
+
+  qcom,tune-usb2-disc-thres-host:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description: High-Speed disconnect threshold for host mode
+    minimum: 0
+    maximum: 7
+    default: 0
+
+  qcom,tune-usb2-amplitude-host:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description: High-Speed transmit amplitude for host mode
+    minimum: 0
+    maximum: 15
+    default: 8
+
+  qcom,tune-usb2-preem-host:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description: High-Speed TX pre-emphasis tuning for host mode
+    minimum: 0
+    maximum: 7
+    default: 5
+
+  qcom,tune-usb2-squelch-host:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description: Squelch detection threshold tuning for host mode
+    minimum: 0
+    maximum: 7
+    default: 3
+
 required:
   - compatible
   - reg
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler
  2025-04-16 12:01 [PATCH v2 0/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler Pengyu Luo
  2025-04-16 12:02 ` [PATCH v2 1/2] dt-bindings: phy: qcom,snps-eusb2-repeater: Add more tuning overrides Pengyu Luo
@ 2025-04-16 12:02 ` Pengyu Luo
  2025-04-25 19:41   ` Dmitry Baryshkov
  1 sibling, 1 reply; 8+ messages in thread
From: Pengyu Luo @ 2025-04-16 12:02 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pengyu Luo, Stephen Rothwell,
	Abel Vesa
  Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel

In downstream tree, many registers need to be overridden, it varies
from devices and platforms, with these registers getting more, adding
a handler for this is helpful.

Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
 .../phy/qualcomm/phy-qcom-eusb2-repeater.c    | 105 +++++++++++++++---
 1 file changed, 92 insertions(+), 13 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
index 6bd1b3c75..a4b75e70e 100644
--- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
+++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
@@ -68,12 +68,27 @@ struct eusb2_repeater_cfg {
 	int num_vregs;
 };
 
+struct tune {
+	const char * const tune_name;
+	const char * const host_tune_name;
+	enum eusb2_reg_layout init_tbl_idx;
+	u8 tune_reg;
+	u8 tune;
+	u8 host_tune;
+};
+
+struct tune_cfg {
+	struct tune *tune_tbl;
+	int tune_tbl_size;
+};
+
 struct eusb2_repeater {
 	struct device *dev;
 	struct regmap *regmap;
 	struct phy *phy;
 	struct regulator_bulk_data *vregs;
 	const struct eusb2_repeater_cfg *cfg;
+	struct tune_cfg *tune_cfg;
 	u32 base;
 	enum phy_mode mode;
 };
@@ -108,6 +123,79 @@ static const struct eusb2_repeater_cfg smb2360_eusb2_cfg = {
 	.num_vregs	= ARRAY_SIZE(pm8550b_vreg_l),
 };
 
+static struct tune tune_tbl[] = {
+	{
+		.tune_name = "qcom,tune-usb2-amplitude",
+		.host_tune_name = "qcom,tune-usb2-amplitude-host",
+		.init_tbl_idx = TUNE_IUSB2,
+		.tune_reg = EUSB2_TUNE_IUSB2,
+	},
+	{
+		.tune_name = "qcom,tune-usb2-disc-thres",
+		.host_tune_name = "qcom,tune-usb2-disc-thres-host",
+		.init_tbl_idx = TUNE_HSDISC,
+		.tune_reg = EUSB2_TUNE_HSDISC,
+
+	},
+	{
+		.tune_name = "qcom,tune-usb2-squelch",
+		.host_tune_name = "qcom,tune-usb2-squelch-host",
+		.init_tbl_idx = TUNE_SQUELCH_U,
+		.tune_reg = EUSB2_TUNE_SQUELCH_U,
+	},
+	{
+		.tune_name = "qcom,tune-usb2-preem",
+		.host_tune_name = "qcom,tune-usb2-preem-host",
+		.init_tbl_idx = TUNE_USB2_PREEM,
+		.tune_reg = EUSB2_TUNE_USB2_PREEM,
+	},
+};
+
+static struct tune_cfg tune_cfg = {
+	.tune_tbl	= tune_tbl,
+	.tune_tbl_size	= ARRAY_SIZE(tune_tbl),
+};
+
+static void eusb2_repeater_write_overrides(struct eusb2_repeater *rptr,
+					   bool is_host_mode)
+{
+	struct tune *tune_tbl;
+	int size, i;
+
+	tune_tbl = rptr->tune_cfg->tune_tbl;
+	size = rptr->tune_cfg->tune_tbl_size;
+
+	for (i = 0; i < size; ++i)
+		regmap_write(rptr->regmap, rptr->base + tune_tbl[i].tune_reg,
+			     is_host_mode ? tune_tbl[i].host_tune : tune_tbl[i].tune);
+}
+
+static void eusb2_repeater_parse_dt(struct eusb2_repeater *rptr)
+{
+	struct device_node *np = rptr->dev->of_node;
+	const u32 *init_tbl = rptr->cfg->init_tbl;
+	struct tune *tune_tbl;
+	int size, i, idx;
+
+	tune_tbl = tune_cfg.tune_tbl;
+	size = tune_cfg.tune_tbl_size;
+
+	for (i = 0; i < size; ++i) {
+		/* set default values from init_tbl */
+		idx = tune_tbl[i].init_tbl_idx;
+		tune_tbl[i].tune = init_tbl[idx];
+		tune_tbl[i].host_tune = init_tbl[idx];
+
+		/* update values from dt */
+		of_property_read_u8(np, tune_tbl[i].tune_name,
+				    &tune_tbl[i].tune);
+		of_property_read_u8(np, tune_tbl[i].host_tune_name,
+				    &tune_tbl[i].host_tune);
+	}
+
+	rptr->tune_cfg = &tune_cfg;
+}
+
 static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
 {
 	int num = rptr->cfg->num_vregs;
@@ -127,20 +215,12 @@ static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
 static int eusb2_repeater_init(struct phy *phy)
 {
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
-	struct device_node *np = rptr->dev->of_node;
 	struct regmap *regmap = rptr->regmap;
 	const u32 *init_tbl = rptr->cfg->init_tbl;
-	u8 tune_usb2_preem = init_tbl[TUNE_USB2_PREEM];
-	u8 tune_hsdisc = init_tbl[TUNE_HSDISC];
-	u8 tune_iusb2 = init_tbl[TUNE_IUSB2];
 	u32 base = rptr->base;
 	u32 val;
 	int ret;
 
-	of_property_read_u8(np, "qcom,tune-usb2-amplitude", &tune_iusb2);
-	of_property_read_u8(np, "qcom,tune-usb2-disc-thres", &tune_hsdisc);
-	of_property_read_u8(np, "qcom,tune-usb2-preem", &tune_usb2_preem);
-
 	ret = regulator_bulk_enable(rptr->cfg->num_vregs, rptr->vregs);
 	if (ret)
 		return ret;
@@ -153,14 +233,9 @@ static int eusb2_repeater_init(struct phy *phy)
 	regmap_write(regmap, base + EUSB2_TUNE_USB2_HS_COMP_CUR, init_tbl[TUNE_USB2_HS_COMP_CUR]);
 	regmap_write(regmap, base + EUSB2_TUNE_USB2_EQU, init_tbl[TUNE_USB2_EQU]);
 	regmap_write(regmap, base + EUSB2_TUNE_USB2_SLEW, init_tbl[TUNE_USB2_SLEW]);
-	regmap_write(regmap, base + EUSB2_TUNE_SQUELCH_U, init_tbl[TUNE_SQUELCH_U]);
 	regmap_write(regmap, base + EUSB2_TUNE_RES_FSDIF, init_tbl[TUNE_RES_FSDIF]);
 	regmap_write(regmap, base + EUSB2_TUNE_USB2_CROSSOVER, init_tbl[TUNE_USB2_CROSSOVER]);
 
-	regmap_write(regmap, base + EUSB2_TUNE_USB2_PREEM, tune_usb2_preem);
-	regmap_write(regmap, base + EUSB2_TUNE_HSDISC, tune_hsdisc);
-	regmap_write(regmap, base + EUSB2_TUNE_IUSB2, tune_iusb2);
-
 	ret = regmap_read_poll_timeout(regmap, base + EUSB2_RPTR_STATUS, val, val & RPTR_OK, 10, 5);
 	if (ret)
 		dev_err(rptr->dev, "initialization timed-out\n");
@@ -177,6 +252,7 @@ static int eusb2_repeater_set_mode(struct phy *phy,
 
 	switch (mode) {
 	case PHY_MODE_USB_HOST:
+		eusb2_repeater_write_overrides(rptr, true);
 		/*
 		 * CM.Lx is prohibited when repeater is already into Lx state as
 		 * per eUSB 1.2 Spec. Below implement software workaround until
@@ -186,6 +262,7 @@ static int eusb2_repeater_set_mode(struct phy *phy,
 		regmap_write(regmap, base + EUSB2_FORCE_VAL_5, V_CLK_19P2M_EN);
 		break;
 	case PHY_MODE_USB_DEVICE:
+		eusb2_repeater_write_overrides(rptr, false);
 		/*
 		 * In device mode clear host mode related workaround as there
 		 * is no repeater reset available, and enable/disable of
@@ -252,6 +329,8 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	eusb2_repeater_parse_dt(rptr);
+
 	rptr->phy = devm_phy_create(dev, np, &eusb2_repeater_ops);
 	if (IS_ERR(rptr->phy)) {
 		dev_err(dev, "failed to create PHY: %d\n", ret);
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: phy: qcom,snps-eusb2-repeater: Add more tuning overrides
  2025-04-16 12:02 ` [PATCH v2 1/2] dt-bindings: phy: qcom,snps-eusb2-repeater: Add more tuning overrides Pengyu Luo
@ 2025-04-21 21:32   ` Rob Herring
  0 siblings, 0 replies; 8+ messages in thread
From: Rob Herring @ 2025-04-21 21:32 UTC (permalink / raw)
  To: Pengyu Luo
  Cc: Vinod Koul, Kishon Vijay Abraham I, Krzysztof Kozlowski,
	Conor Dooley, Stephen Rothwell, Abel Vesa, linux-arm-msm,
	linux-phy, devicetree, linux-kernel

On Wed, Apr 16, 2025 at 08:02:00PM +0800, Pengyu Luo wrote:
> In downstream tree, many registers need to be overridden, it varies
> from devices and platforms, not only HS trasmit amplitude(0x51),

spelling

> HS disconnect threshold(0x53), Tx pre-emphasis tuning(0x57). And
> some devices require different overrides for different dr_mode. So
> add property entries for host mode, original entries will be used
> for device mode.
> 
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
>  .../phy/qcom,snps-eusb2-repeater.yaml         | 41 +++++++++++++++++--
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml b/Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml
> index d16a543a7..d0f5e770d 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml
> @@ -36,25 +36,60 @@ properties:
>  
>    qcom,tune-usb2-disc-thres:
>      $ref: /schemas/types.yaml#/definitions/uint8
> -    description: High-Speed disconnect threshold
> +    description: High-Speed disconnect threshold for device mode
>      minimum: 0
>      maximum: 7
>      default: 0
>  
>    qcom,tune-usb2-amplitude:
>      $ref: /schemas/types.yaml#/definitions/uint8
> -    description: High-Speed transmit amplitude
> +    description: High-Speed transmit amplitude for device mode
>      minimum: 0
>      maximum: 15
>      default: 8
>  
>    qcom,tune-usb2-preem:
>      $ref: /schemas/types.yaml#/definitions/uint8
> -    description: High-Speed TX pre-emphasis tuning
> +    description: High-Speed TX pre-emphasis tuning for device mode
>      minimum: 0
>      maximum: 7
>      default: 5
>  
> +  qcom,tune-usb2-squelch:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description: Squelch detection threshold tuning for device mode
> +    minimum: 0
> +    maximum: 7
> +    default: 3
> +
> +  qcom,tune-usb2-disc-thres-host:

Put this and the others after each existing property (for device mode). 
Ideally, these would all be in alphabetical order.

> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description: High-Speed disconnect threshold for host mode
> +    minimum: 0
> +    maximum: 7
> +    default: 0
> +
> +  qcom,tune-usb2-amplitude-host:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description: High-Speed transmit amplitude for host mode
> +    minimum: 0
> +    maximum: 15
> +    default: 8
> +
> +  qcom,tune-usb2-preem-host:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description: High-Speed TX pre-emphasis tuning for host mode
> +    minimum: 0
> +    maximum: 7
> +    default: 5
> +
> +  qcom,tune-usb2-squelch-host:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description: Squelch detection threshold tuning for host mode
> +    minimum: 0
> +    maximum: 7
> +    default: 3
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.49.0
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler
  2025-04-16 12:02 ` [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler Pengyu Luo
@ 2025-04-25 19:41   ` Dmitry Baryshkov
  2025-04-26  8:14     ` Pengyu Luo
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2025-04-25 19:41 UTC (permalink / raw)
  To: Pengyu Luo
  Cc: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Stephen Rothwell, Abel Vesa,
	linux-arm-msm, linux-phy, devicetree, linux-kernel

On Wed, Apr 16, 2025 at 08:02:01PM +0800, Pengyu Luo wrote:
> In downstream tree, many registers need to be overridden, it varies
> from devices and platforms, with these registers getting more, adding
> a handler for this is helpful.

It should be noted that previously all values were applied during _init
phase, before checking the status etc. Now the overrides are programmed
from the set_mode. Should you still program sane defaults at the init
stage too?

BTW, is there a real need to override those for the platform you are
working on? Could you please provide some details, maybe in the cover
letter.

> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
>  .../phy/qualcomm/phy-qcom-eusb2-repeater.c    | 105 +++++++++++++++---
>  1 file changed, 92 insertions(+), 13 deletions(-)

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler
  2025-04-25 19:41   ` Dmitry Baryshkov
@ 2025-04-26  8:14     ` Pengyu Luo
  2025-04-28 16:58       ` Dmitry Baryshkov
  0 siblings, 1 reply; 8+ messages in thread
From: Pengyu Luo @ 2025-04-26  8:14 UTC (permalink / raw)
  To: dmitry.baryshkov
  Cc: abel.vesa, conor+dt, devicetree, kishon, krzk+dt, linux-arm-msm,
	linux-kernel, linux-phy, mitltlatltl, robh, sfr, vkoul

On Sat, Apr 26, 2025 at 3:41 AM Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
> On Wed, Apr 16, 2025 at 08:02:01PM +0800, Pengyu Luo wrote:
> > In downstream tree, many registers need to be overridden, it varies
> > from devices and platforms, with these registers getting more, adding
> > a handler for this is helpful.
>
> It should be noted that previously all values were applied during _init
> phase, before checking the status etc. Now the overrides are programmed
> from the set_mode. Should you still program sane defaults at the init
> stage too?
>

I think programming in set_mode is ok. When we init(dwc3_core_init), we
set_mode(dwc3_core_init_mode) later, please check
https://elixir.bootlin.com/linux/v6.14.3/source/drivers/usb/dwc3/core.c#L2287

Actually, in the downstream, all the things are done in init, it
overrides first, then masked write the deaults, finally it set_mode,
you can check here
https://github.com/OnePlusOSS/android_kernel_oneplus_sm8650/blob/oneplus/sm8650_v_15.0.0_pad_pro/drivers/usb/repeater/repeater-qti-pmic-eusb2.c#L356

> BTW, is there a real need to override those for the platform you are
> working on? Could you please provide some details, maybe in the cover
> letter.

I am not quite sure, recently, I expirenced mode switching failure,
when I `echo device > /sys/kernel/debug/usb/a600000.usb/mode`, Ethernet
Gadget wouldn't work again, my desktop can't connect to it.

BTW, as you can see in line 356, it is most likely that overrides
related to charging feature. I have not ported charging yet, but adding
more overrides seems harmless, and if overriding, distinguishing which
mode seems necessary, even if some devices use the same sequence. So I
sent the patch.

Best wishes,
Pengyu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler
  2025-04-26  8:14     ` Pengyu Luo
@ 2025-04-28 16:58       ` Dmitry Baryshkov
  2025-04-29  9:23         ` Pengyu Luo
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2025-04-28 16:58 UTC (permalink / raw)
  To: Pengyu Luo
  Cc: abel.vesa, conor+dt, devicetree, kishon, krzk+dt, linux-arm-msm,
	linux-kernel, linux-phy, robh, sfr, vkoul

On Sat, Apr 26, 2025 at 04:14:23PM +0800, Pengyu Luo wrote:
> On Sat, Apr 26, 2025 at 3:41 AM Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
> > On Wed, Apr 16, 2025 at 08:02:01PM +0800, Pengyu Luo wrote:
> > > In downstream tree, many registers need to be overridden, it varies
> > > from devices and platforms, with these registers getting more, adding
> > > a handler for this is helpful.
> >
> > It should be noted that previously all values were applied during _init
> > phase, before checking the status etc. Now the overrides are programmed
> > from the set_mode. Should you still program sane defaults at the init
> > stage too?
> >
> 
> I think programming in set_mode is ok. When we init(dwc3_core_init), we
> set_mode(dwc3_core_init_mode) later, please check
> https://elixir.bootlin.com/linux/v6.14.3/source/drivers/usb/dwc3/core.c#L2287

Yes, but that happens after reading status regs, etc.

> Actually, in the downstream, all the things are done in init, it
> overrides first, then masked write the deaults, finally it set_mode,
> you can check here
> https://github.com/OnePlusOSS/android_kernel_oneplus_sm8650/blob/oneplus/sm8650_v_15.0.0_pad_pro/drivers/usb/repeater/repeater-qti-pmic-eusb2.c#L356

I'd stick to this approach too. Program everything in init, then
program mode-dependent regs in set_mode.

> 
> > BTW, is there a real need to override those for the platform you are
> > working on? Could you please provide some details, maybe in the cover
> > letter.
> 
> I am not quite sure, recently, I expirenced mode switching failure,
> when I `echo device > /sys/kernel/debug/usb/a600000.usb/mode`, Ethernet
> Gadget wouldn't work again, my desktop can't connect to it.

Do you have at least a list of the properties / registers that
downstream programs on your platform? I mean, it's not infrequent that
vendor kernel is more versatile than necessary, as it is being used
during bringup / etc. I'd suggest to limit supported overrides to those
necessary for your platform (and add a comment that there were other
available).

> 
> BTW, as you can see in line 356, it is most likely that overrides
> related to charging feature. I have not ported charging yet, but adding
> more overrides seems harmless, and if overriding, distinguishing which
> mode seems necessary, even if some devices use the same sequence. So I
> sent the patch.
> 
> Best wishes,
> Pengyu
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler
  2025-04-28 16:58       ` Dmitry Baryshkov
@ 2025-04-29  9:23         ` Pengyu Luo
  0 siblings, 0 replies; 8+ messages in thread
From: Pengyu Luo @ 2025-04-29  9:23 UTC (permalink / raw)
  To: dmitry.baryshkov
  Cc: abel.vesa, conor+dt, devicetree, kishon, krzk+dt, linux-arm-msm,
	linux-kernel, linux-phy, mitltlatltl, robh, sfr, vkoul

On Tue, Apr 29, 2025 at 12:58 AM Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
> On Sat, Apr 26, 2025 at 04:14:23PM +0800, Pengyu Luo wrote:
> > On Sat, Apr 26, 2025 at 3:41 AM Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
> > > On Wed, Apr 16, 2025 at 08:02:01PM +0800, Pengyu Luo wrote:
> > > > In downstream tree, many registers need to be overridden, it varies
> > > > from devices and platforms, with these registers getting more, adding
> > > > a handler for this is helpful.
> > >
> > > It should be noted that previously all values were applied during _init
> > > phase, before checking the status etc. Now the overrides are programmed
> > > from the set_mode. Should you still program sane defaults at the init
> > > stage too?
> > >
> >
> > I think programming in set_mode is ok. When we init(dwc3_core_init), we
> > set_mode(dwc3_core_init_mode) later, please check
> > https://elixir.bootlin.com/linux/v6.14.3/source/drivers/usb/dwc3/core.c#L2287
>
> Yes, but that happens after reading status regs, etc.
>
> > Actually, in the downstream, all the things are done in init, it
> > overrides first, then masked write the deaults, finally it set_mode,
> > you can check here
> > https://github.com/OnePlusOSS/android_kernel_oneplus_sm8650/blob/oneplus/sm8650_v_15.0.0_pad_pro/drivers/usb/repeater/repeater-qti-pmic-eusb2.c#L356
>
> I'd stick to this approach too. Program everything in init, then
> program mode-dependent regs in set_mode.
>

If so, I don't mind programming the defaults in init, then programming
overrides in set_mode.

> >
> > > BTW, is there a real need to override those for the platform you are
> > > working on? Could you please provide some details, maybe in the cover
> > > letter.
> >
> > I am not quite sure, recently, I expirenced mode switching failure,
> > when I `echo device > /sys/kernel/debug/usb/a600000.usb/mode`, Ethernet
> > Gadget wouldn't work again, my desktop can't connect to it.
>
> Do you have at least a list of the properties / registers that
> downstream programs on your platform? I mean, it's not infrequent that
> vendor kernel is more versatile than necessary, as it is being used
> during bringup / etc. I'd suggest to limit supported overrides to those
> necessary for your platform (and add a comment that there were other
> available).
>

Yes, I only added the necessary overrides(0x54, and mode handling).
Please check

https://github.com/OnePlusOSS/android_kernel_modules_and_devicetree_oneplus_sm8650/blob/oneplus/sm8650_v_15.0.0_pad_pro/kernel_platform/qcom/proprietary/devicetree/oplus/oplus_misc/oplus-misc-23926.dtsi#L26

Best wishes,
Pengyu

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-04-29  9:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-16 12:01 [PATCH v2 0/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler Pengyu Luo
2025-04-16 12:02 ` [PATCH v2 1/2] dt-bindings: phy: qcom,snps-eusb2-repeater: Add more tuning overrides Pengyu Luo
2025-04-21 21:32   ` Rob Herring
2025-04-16 12:02 ` [PATCH v2 2/2] phy: qualcomm: phy-qcom-eusb2-repeater: rework reg override handler Pengyu Luo
2025-04-25 19:41   ` Dmitry Baryshkov
2025-04-26  8:14     ` Pengyu Luo
2025-04-28 16:58       ` Dmitry Baryshkov
2025-04-29  9:23         ` Pengyu Luo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).