[PATCH v5 0/2] Refine USB interrupt vectors on Qualcomm platforms

[PATCH v5 0/2] Refine USB interrupt vectors on Qualcomm platforms

[Krishna Kurapati]

Qualcomm targets define the following interrupts for usb wakeup:
{dp/dm}_hs_phy_irq, hs_phy_irq, pwr_event, ss_phy_irq.

But QUSB2 Phy based targets have another interrupt which gets triggered
in response to J/K states on dp/dm pads. Its functionality is replaced
by dp/dm interrupts on Femto/m31/eusb2 phy based targets for wakeup
purposes. Exceptions are some targets like SDM845/SDM670/SM6350 where
dp/dm irq's are used although they are qusb2 phy targets.

Currently in QUSB2 Phy based DT's, te qusb2_phy interrupt is named and
used as "hs_phy_irq" when in fact it is a different interrupt (used by
HW validation folks for debug purposes and not used on any downstream
target qusb/non-qusb).

On some non-QUSB2 targets (like sm8450/sm8550), the pwr_event IRQ was
named as hs_phy_irq and actual pwr_event_irq was skipped.

This series tries to address the discrepancies in the interrupt numbering
adding the missing interrupts and correcting the existing ones.

This series has been compared with downstream counter part and hw specifics
to ensure the numbering is right. Since there is not functionality change
the code has been only compile tested.

Changes in v5:
Fixed commit header on v4-1 bindings patch.
Provide lore link instead of patchwork link for v3.

Changes in v4:
Udpated commit text indicating why pwr_event interrupt was added as the first
one and fixed some typos present in v3.
While at it, rebase on top of latest linux-next fixing merge conflicts.

Changes in v3:
Separated out the DT changes and pushed only bindings and driver update.
Modified order of irq descriptions to match them with permutations defined.
Fixed nitpicks mentioned by reviewers in v2.

Changes in v2:
Removed additional compatibles added for different targets in v1.
Specified permuations of interrupts possible for QC targets and regrouped
interrupts for most of the DT's.

Link to v4:
https://lore.kernel.org/all/20231222062720.10128-1-quic_kriskura@quicinc.com/

Link to v3:
https://lore.kernel.org/all/20231211121124.4194-1-quic_kriskura@quicinc.com/

Link to v2:
https://lore.kernel.org/all/20231204100950.28712-1-quic_kriskura@quicinc.com/

Link to v1: (providing patchwork link since threading was broken in v1)
https://patchwork.kernel.org/project/linux-arm-msm/cover/20231122191259.3021-1-quic_kriskura@quicinc.com/

Krishna Kurapati (2):
  The high speed related interrupts present on QC targets are as
    follows:
  usb: dwc3: qcom: Rename hs_phy_irq to qusb2_phy_irq

 .../devicetree/bindings/usb/qcom,dwc3.yaml    | 138 ++++++++----------
 drivers/usb/dwc3/dwc3-qcom.c                  |  22 +--
 2 files changed, 70 insertions(+), 90 deletions(-)

-- 
2.42.0

[PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krishna Kurapati]

The high speed related interrupts present on QC targets are as follows:

dp/dm irq's
These IRQ's directly reflect changes on the DP/DM pads of the SoC. These
are used as wakeup interrupts only on SoCs with non-QUSB2 targets with
exception of SDM670/SDM845/SM6350.

qusb2_phy irq
SoCs with QUSB2 PHY do not have separate DP/DM IRQs and expose only a
single IRQ whose behavior can be modified by the QUSB2PHY_INTR_CTRL
register. The required DPSE/DMSE configuration is done in
QUSB2PHY_INTR_CTRL register of phy address space.

hs_phy_irq
This is completely different from the above two and is present on all
targets with exception of a few IPQ ones. The interrupt is not enabled by
default and its functionality is mutually exclusive of qusb2_phy on QUSB
targets and DP/DM on femto phy targets.

The DTs of several QUSB2 PHY based SoCs incorrectly define "hs_phy_irq"
when they should have been "qusb2_phy_irq". On Femto phy targets, the
"hs_phy_irq" mentioned is either the actual "hs_phy_irq" or "pwr_event",
neither of which would never be triggered directly are non-functional
currently. The implementation tries to clean up this issue by addressing
the discrepencies involved and fixing the hs_phy_irq's in respective DT's.

Classify SoC's into four groups based on whether qusb2_phy interrupt
or {dp/dm}_hs_phy_irq is used for wakeup in high speed and whether the
SoCs have hs_phy_irq present in them or not.

The ss_phy_irq is optional interrupt because there are mutliple SoC's
which either support only High Speed or there are multiple controllers
within same Soc and the secondary controller is High Speed only capable.

This breaks ABI on targets running older kernels, but since the interrupt
definitions are given wrong on many targets and to establish proper rules
for usage of DWC3 interrupts on Qualcomm platforms, DT binding update is
necessary.

The bindings put pwr_event as the first interrupt and ss_phy as the last.
Since all SoCs have the pwr_event (HS) interrupt, but not all
controllers have the SS PHY interrupt, this would prevent expressing
that the SS PHY is optional by keeping it last in the binding schema and
making sure that minItem = maxItems - 1.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 .../devicetree/bindings/usb/qcom,dwc3.yaml    | 138 ++++++++----------
 1 file changed, 59 insertions(+), 79 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index 473c4bfaf8a2..1964b5ad83b7 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -99,12 +99,29 @@ properties:
       - const: apps-usb
 
   interrupts:
-    minItems: 1
-    maxItems: 4
+    description: |
+      Different types of interrupts are used based on HS PHY used on target:
+        - pwr_event: Used for wakeup based on other power events.
+        - hs_phY_irq: Apart from DP/DM/QUSB2 PHY interrupts, there is
+                       hs_phy_irq which is not triggered by default and its
+                       functionality is mutually exclusive to that of
+                       {dp/dm}_hs_phy_irq and qusb2_phy_irq.
+        - qusb2_phy: SoCs with QUSB2 PHY do not have separate DP/DM IRQs and
+                      expose only a single IRQ whose behavior can be modified
+                      by the QUSB2PHY_INTR_CTRL register. The required DPSE/
+                      DMSE configuration is done in QUSB2PHY_INTR_CTRL register
+                      of PHY address space.
+        - {dp/dm}_hs_phy_irq: These IRQ's directly reflect changes on the DP/
+                               DM pads of the SoC. These are used for wakeup
+                               only on SoCs with non-QUSB2 targets with
+                               exception of SDM670/SDM845/SM6350.
+        - ss_phy_irq: Used for remote wakeup in Super Speed mode of operation.
+    minItems: 2
+    maxItems: 5
 
   interrupt-names:
-    minItems: 1
-    maxItems: 4
+    minItems: 2
+    maxItems: 5
 
   qcom,select-utmi-as-pipe-clk:
     description:
@@ -361,60 +378,21 @@ allOf:
         compatible:
           contains:
             enum:
-              - qcom,ipq4019-dwc3
+              - qcom,ipq5018-dwc3
               - qcom,ipq6018-dwc3
-              - qcom,ipq8064-dwc3
               - qcom,ipq8074-dwc3
-              - qcom,msm8994-dwc3
-              - qcom,qcs404-dwc3
-              - qcom,sc7180-dwc3
-              - qcom,sdm670-dwc3
-              - qcom,sdm845-dwc3
-              - qcom,sdx55-dwc3
-              - qcom,sdx65-dwc3
-              - qcom,sdx75-dwc3
-              - qcom,sm4250-dwc3
-              - qcom,sm6350-dwc3
-              - qcom,sm8150-dwc3
-              - qcom,sm8250-dwc3
-              - qcom,sm8350-dwc3
-              - qcom,sm8450-dwc3
-              - qcom,sm8550-dwc3
-              - qcom,sm8650-dwc3
-    then:
-      properties:
-        interrupts:
-          items:
-            - description: The interrupt that is asserted
-                when a wakeup event is received on USB2 bus.
-            - description: The interrupt that is asserted
-                when a wakeup event is received on USB3 bus.
-            - description: Wakeup event on DM line.
-            - description: Wakeup event on DP line.
-        interrupt-names:
-          items:
-            - const: hs_phy_irq
-            - const: ss_phy_irq
-            - const: dm_hs_phy_irq
-            - const: dp_hs_phy_irq
-
-  - if:
-      properties:
-        compatible:
-          contains:
-            enum:
               - qcom,msm8953-dwc3
-              - qcom,msm8996-dwc3
               - qcom,msm8998-dwc3
-              - qcom,sm6115-dwc3
-              - qcom,sm6125-dwc3
+              - qcom,qcm2290-dwc3
     then:
       properties:
         interrupts:
-          maxItems: 2
+          minItems: 2
+          maxItems: 3
         interrupt-names:
           items:
-            - const: hs_phy_irq
+            - const: pwr_event
+            - const: qusb2_phy
             - const: ss_phy_irq
 
   - if:
@@ -422,37 +400,21 @@ allOf:
         compatible:
           contains:
             enum:
-              - qcom,ipq5018-dwc3
-              - qcom,ipq5332-dwc3
+              - qcom,msm8996-dwc3
+              - qcom,qcs404-dwc3
               - qcom,sdm660-dwc3
-    then:
-      properties:
-        interrupts:
-          minItems: 1
-          maxItems: 2
-        interrupt-names:
-          minItems: 1
-          items:
-            - const: hs_phy_irq
-            - const: ss_phy_irq
-
-  - if:
-      properties:
-        compatible:
-          contains:
-            enum:
-              - qcom,sc7280-dwc3
+              - qcom,sm6115-dwc3
+              - qcom,sm6125-dwc3
     then:
       properties:
         interrupts:
           minItems: 3
           maxItems: 4
         interrupt-names:
-          minItems: 3
           items:
+            - const: pwr_event
             - const: hs_phy_irq
-            - const: dp_hs_phy_irq
-            - const: dm_hs_phy_irq
+            - const: qusb2_phy
             - const: ss_phy_irq
 
   - if:
@@ -460,11 +422,13 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,ipq5332-dwc3
               - qcom,sc8280xp-dwc3
               - qcom,x1e80100-dwc3
     then:
       properties:
         interrupts:
+          minItems: 3
           maxItems: 4
         interrupt-names:
           items:
@@ -479,15 +443,30 @@ allOf:
           contains:
             enum:
               - qcom,sa8775p-dwc3
+              - qcom,sc7180-dwc3
+              - qcom,sc7280-dwc3
+              - qcom,sdm670-dwc3
+              - qcom,sdm845-dwc3
+              - qcom,sdx55-dwc3
+              - qcom,sdx65-dwc3
+              - qcom,sdx75-dwc3
+              - qcom,sm6350-dwc3
+              - qcom,sm6375-dwc3
+              - qcom,sm8150-dwc3
+              - qcom,sm8250-dwc3
+              - qcom,sm8350-dwc3
+              - qcom,sm8450-dwc3
+              - qcom,sm8550-dwc3
+              - qcom,sm8650-dwc3
     then:
       properties:
         interrupts:
-          minItems: 3
-          maxItems: 4
+          minItems: 4
+          maxItems: 5
         interrupt-names:
-          minItems: 3
           items:
             - const: pwr_event
+            - const: hs_phy_irq
             - const: dp_hs_phy_irq
             - const: dm_hs_phy_irq
             - const: ss_phy_irq
@@ -525,12 +504,13 @@ examples:
                           <&gcc GCC_USB30_PRIM_MASTER_CLK>;
             assigned-clock-rates = <19200000>, <150000000>;
 
-            interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
+            interrupts = <GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 489 IRQ_TYPE_EDGE_BOTH>,
                          <GIC_SPI 488 IRQ_TYPE_EDGE_BOTH>,
-                         <GIC_SPI 489 IRQ_TYPE_EDGE_BOTH>;
-            interrupt-names = "hs_phy_irq", "ss_phy_irq",
-                          "dm_hs_phy_irq", "dp_hs_phy_irq";
+                         <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "pwr_event", "hs_phy_irq",
+                          "dp_hs_phy_irq", "dm_hs_phy_irq", "ss_phy_irq";
 
             power-domains = <&gcc USB30_PRIM_GDSC>;
 
-- 
2.42.0

[PATCH v5 2/2] usb: dwc3: qcom: Rename hs_phy_irq to qusb2_phy_irq

[Krishna Kurapati]

For wakeup to work, driver needs to enable interrupts that depict what is
happening on the DP/DM lines. On QUSB targets, this is identified by
qusb2_phy whereas on SoCs using Femto PHY, separate {dp,dm}_hs_phy_irq's
are used instead.

The implementation incorrectly names qusb2_phy interrupts as "hs_phy_irq".
Clean this up so that driver would be using only qusb2/(dp & dm) for wakeup
purposes.

For devices running older kernels, this won't break any functionality
because the interrupt configurations in QUSB2 PHY based SoCs is done
by configuring QUSB2PHY_INTR_CTRL register in PHY address space and it was
never armed properly right from the start.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index fdf6d5d3c2ad..dbd6a5b2b289 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -57,7 +57,7 @@ struct dwc3_acpi_pdata {
 	u32			qscratch_base_offset;
 	u32			qscratch_base_size;
 	u32			dwc3_core_base_size;
-	int			hs_phy_irq_index;
+	int			qusb2_phy_irq_index;
 	int			dp_hs_phy_irq_index;
 	int			dm_hs_phy_irq_index;
 	int			ss_phy_irq_index;
@@ -73,7 +73,7 @@ struct dwc3_qcom {
 	int			num_clocks;
 	struct reset_control	*resets;
 
-	int			hs_phy_irq;
+	int			qusb2_phy_irq;
 	int			dp_hs_phy_irq;
 	int			dm_hs_phy_irq;
 	int			ss_phy_irq;
@@ -372,7 +372,7 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
 
 static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 {
-	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
+	dwc3_qcom_disable_wakeup_irq(qcom->qusb2_phy_irq);
 
 	if (qcom->usb2_speed == USB_SPEED_LOW) {
 		dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
@@ -389,7 +389,7 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 
 static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 {
-	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq, 0);
+	dwc3_qcom_enable_wakeup_irq(qcom->qusb2_phy_irq, 0);
 
 	/*
 	 * Configure DP/DM line interrupts based on the USB2 device attached to
@@ -542,19 +542,19 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 	int irq;
 	int ret;
 
-	irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
-				pdata ? pdata->hs_phy_irq_index : -1);
+	irq = dwc3_qcom_get_irq(pdev, "qusb2_phy",
+				pdata ? pdata->qusb2_phy_irq_index : -1);
 	if (irq > 0) {
 		/* Keep wakeup interrupts disabled until suspend */
 		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
 					qcom_dwc3_resume_irq,
 					IRQF_ONESHOT | IRQF_NO_AUTOEN,
-					"qcom_dwc3 HS", qcom);
+					"qcom_dwc3 QUSB2", qcom);
 		if (ret) {
-			dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret);
+			dev_err(qcom->dev, "qusb2_phy_irq failed: %d\n", ret);
 			return ret;
 		}
-		qcom->hs_phy_irq = irq;
+		qcom->qusb2_phy_irq = irq;
 	}
 
 	irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
@@ -1058,7 +1058,7 @@ static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
 	.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
 	.qscratch_base_size = SDM845_QSCRATCH_SIZE,
 	.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
-	.hs_phy_irq_index = 1,
+	.qusb2_phy_irq_index = 1,
 	.dp_hs_phy_irq_index = 4,
 	.dm_hs_phy_irq_index = 3,
 	.ss_phy_irq_index = 2
@@ -1068,7 +1068,7 @@ static const struct dwc3_acpi_pdata sdm845_acpi_urs_pdata = {
 	.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
 	.qscratch_base_size = SDM845_QSCRATCH_SIZE,
 	.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
-	.hs_phy_irq_index = 1,
+	.qusb2_phy_irq_index = 1,
 	.dp_hs_phy_irq_index = 4,
 	.dm_hs_phy_irq_index = 3,
 	.ss_phy_irq_index = 2,
-- 
2.42.0

Re: [PATCH v5 2/2] usb: dwc3: qcom: Rename hs_phy_irq to qusb2_phy_irq

[Thinh Nguyen]

On Fri, Dec 22, 2023, Krishna Kurapati wrote:
> For wakeup to work, driver needs to enable interrupts that depict what is
> happening on the DP/DM lines. On QUSB targets, this is identified by
> qusb2_phy whereas on SoCs using Femto PHY, separate {dp,dm}_hs_phy_irq's
> are used instead.
> 
> The implementation incorrectly names qusb2_phy interrupts as "hs_phy_irq".
> Clean this up so that driver would be using only qusb2/(dp & dm) for wakeup
> purposes.
> 
> For devices running older kernels, this won't break any functionality
> because the interrupt configurations in QUSB2 PHY based SoCs is done
> by configuring QUSB2PHY_INTR_CTRL register in PHY address space and it was
> never armed properly right from the start.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index fdf6d5d3c2ad..dbd6a5b2b289 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -57,7 +57,7 @@ struct dwc3_acpi_pdata {
>  	u32			qscratch_base_offset;
>  	u32			qscratch_base_size;
>  	u32			dwc3_core_base_size;
> -	int			hs_phy_irq_index;
> +	int			qusb2_phy_irq_index;
>  	int			dp_hs_phy_irq_index;
>  	int			dm_hs_phy_irq_index;
>  	int			ss_phy_irq_index;
> @@ -73,7 +73,7 @@ struct dwc3_qcom {
>  	int			num_clocks;
>  	struct reset_control	*resets;
>  
> -	int			hs_phy_irq;
> +	int			qusb2_phy_irq;
>  	int			dp_hs_phy_irq;
>  	int			dm_hs_phy_irq;
>  	int			ss_phy_irq;
> @@ -372,7 +372,7 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
>  
>  static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
>  {
> -	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
> +	dwc3_qcom_disable_wakeup_irq(qcom->qusb2_phy_irq);
>  
>  	if (qcom->usb2_speed == USB_SPEED_LOW) {
>  		dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
> @@ -389,7 +389,7 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
>  
>  static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
>  {
> -	dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq, 0);
> +	dwc3_qcom_enable_wakeup_irq(qcom->qusb2_phy_irq, 0);
>  
>  	/*
>  	 * Configure DP/DM line interrupts based on the USB2 device attached to
> @@ -542,19 +542,19 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>  	int irq;
>  	int ret;
>  
> -	irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
> -				pdata ? pdata->hs_phy_irq_index : -1);
> +	irq = dwc3_qcom_get_irq(pdev, "qusb2_phy",
> +				pdata ? pdata->qusb2_phy_irq_index : -1);
>  	if (irq > 0) {
>  		/* Keep wakeup interrupts disabled until suspend */
>  		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>  					qcom_dwc3_resume_irq,
>  					IRQF_ONESHOT | IRQF_NO_AUTOEN,
> -					"qcom_dwc3 HS", qcom);
> +					"qcom_dwc3 QUSB2", qcom);
>  		if (ret) {
> -			dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret);
> +			dev_err(qcom->dev, "qusb2_phy_irq failed: %d\n", ret);
>  			return ret;
>  		}
> -		qcom->hs_phy_irq = irq;
> +		qcom->qusb2_phy_irq = irq;
>  	}
>  
>  	irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
> @@ -1058,7 +1058,7 @@ static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
>  	.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
>  	.qscratch_base_size = SDM845_QSCRATCH_SIZE,
>  	.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
> -	.hs_phy_irq_index = 1,
> +	.qusb2_phy_irq_index = 1,
>  	.dp_hs_phy_irq_index = 4,
>  	.dm_hs_phy_irq_index = 3,
>  	.ss_phy_irq_index = 2
> @@ -1068,7 +1068,7 @@ static const struct dwc3_acpi_pdata sdm845_acpi_urs_pdata = {
>  	.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
>  	.qscratch_base_size = SDM845_QSCRATCH_SIZE,
>  	.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
> -	.hs_phy_irq_index = 1,
> +	.qusb2_phy_irq_index = 1,
>  	.dp_hs_phy_irq_index = 4,
>  	.dm_hs_phy_irq_index = 3,
>  	.ss_phy_irq_index = 2,
> -- 
> 2.42.0
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krzysztof Kozlowski]

On 22/12/2023 07:36, Krishna Kurapati wrote:
> The high speed related interrupts present on QC targets are as follows:
> 


>  
>    interrupt-names:
> -    minItems: 1
> -    maxItems: 4
> +    minItems: 2
> +    maxItems: 5
>  
>    qcom,select-utmi-as-pipe-clk:
>      description:
> @@ -361,60 +378,21 @@ allOf:
>          compatible:
>            contains:
>              enum:
> -              - qcom,ipq4019-dwc3

Why do you remove it, without adding it somewhere else. Nothing in the
commit msg explains it.

> +              - qcom,ipq5018-dwc3
>                - qcom,ipq6018-dwc3
> -              - qcom,ipq8064-dwc3
>                - qcom,ipq8074-dwc3
> -              - qcom,msm8994-dwc3
> -              - qcom,qcs404-dwc3
> -              - qcom,sc7180-dwc3
> -              - qcom,sdm670-dwc3
> -              - qcom,sdm845-dwc3
> -              - qcom,sdx55-dwc3
> -              - qcom,sdx65-dwc3
> -              - qcom,sdx75-dwc3
> -              - qcom,sm4250-dwc3
> -              - qcom,sm6350-dwc3
> -              - qcom,sm8150-dwc3
> -              - qcom,sm8250-dwc3
> -              - qcom,sm8350-dwc3
> -              - qcom,sm8450-dwc3
> -              - qcom,sm8550-dwc3
> -              - qcom,sm8650-dwc3
> -    then:
> -      properties:
> -        interrupts:
> -          items:
> -            - description: The interrupt that is asserted
> -                when a wakeup event is received on USB2 bus.
> -            - description: The interrupt that is asserted
> -                when a wakeup event is received on USB3 bus.
> -            - description: Wakeup event on DM line.
> -            - description: Wakeup event on DP line.
> -        interrupt-names:
> -          items:
> -            - const: hs_phy_irq
> -            - const: ss_phy_irq
> -            - const: dm_hs_phy_irq
> -            - const: dp_hs_phy_irq
> -
> -  - if:
> -      properties:
> -        compatible:
> -          contains:
> -            enum:
>                - qcom,msm8953-dwc3
> -              - qcom,msm8996-dwc3
>                - qcom,msm8998-dwc3
> -              - qcom,sm6115-dwc3
> -              - qcom,sm6125-dwc3
> +              - qcom,qcm2290-dwc3
>      then:
>        properties:
>          interrupts:
> -          maxItems: 2
> +          minItems: 2
> +          maxItems: 3
>          interrupt-names:
>            items:
> -            - const: hs_phy_irq
> +            - const: pwr_event
> +            - const: qusb2_phy
>              - const: ss_phy_irq
>  
>    - if:
> @@ -422,37 +400,21 @@ allOf:
>          compatible:
>            contains:
>              enum:
> -              - qcom,ipq5018-dwc3
> -              - qcom,ipq5332-dwc3
> +              - qcom,msm8996-dwc3
> +              - qcom,qcs404-dwc3
>                - qcom,sdm660-dwc3
> -    then:
> -      properties:
> -        interrupts:
> -          minItems: 1
> -          maxItems: 2
> -        interrupt-names:
> -          minItems: 1
> -          items:
> -            - const: hs_phy_irq
> -            - const: ss_phy_irq
> -
> -  - if:
> -      properties:
> -        compatible:
> -          contains:
> -            enum:
> -              - qcom,sc7280-dwc3
> +              - qcom,sm6115-dwc3
> +              - qcom,sm6125-dwc3
>      then:
>        properties:
>          interrupts:
>            minItems: 3
>            maxItems: 4
>          interrupt-names:
> -          minItems: 3
>            items:
> +            - const: pwr_event
>              - const: hs_phy_irq
> -            - const: dp_hs_phy_irq
> -            - const: dm_hs_phy_irq
> +            - const: qusb2_phy

Why qusb2_phy is after hs_phy_irq? In the earlier if:then: it is the
second one.


>              - const: ss_phy_irq
>  
>    - if:
> @@ -460,11 +422,13 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - qcom,ipq5332-dwc3
>                - qcom,sc8280xp-dwc3
>                - qcom,x1e80100-dwc3
>      then:
>        properties:
>          interrupts:
> +          minItems: 3

Hm, why? This commit is unmanageable. Your commit msg is already huge
but still does not explain this. Are you sure you are fixing only one
logical thing per patch? Does not look like.

Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krishna Kurapati PSSNV]

On 12/25/2023 6:35 PM, Krzysztof Kozlowski wrote:
> On 22/12/2023 07:36, Krishna Kurapati wrote:
>> The high speed related interrupts present on QC targets are as follows:
>>
> 
> 
>>   
>>     interrupt-names:
>> -    minItems: 1
>> -    maxItems: 4
>> +    minItems: 2
>> +    maxItems: 5
>>   
>>     qcom,select-utmi-as-pipe-clk:
>>       description:
>> @@ -361,60 +378,21 @@ allOf:
>>           compatible:
>>             contains:
>>               enum:
>> -              - qcom,ipq4019-dwc3
> 
> Why do you remove it, without adding it somewhere else. Nothing in the
> commit msg explains it.
> 

Apologies, Will check and add it back.

>> +              - qcom,ipq5018-dwc3
>>                 - qcom,ipq6018-dwc3
>> -              - qcom,ipq8064-dwc3
>>                 - qcom,ipq8074-dwc3
>> -              - qcom,msm8994-dwc3
>> -              - qcom,qcs404-dwc3
>> -              - qcom,sc7180-dwc3
>> -              - qcom,sdm670-dwc3
>> -              - qcom,sdm845-dwc3
>> -              - qcom,sdx55-dwc3
>> -              - qcom,sdx65-dwc3
>> -              - qcom,sdx75-dwc3
>> -              - qcom,sm4250-dwc3
>> -              - qcom,sm6350-dwc3
>> -              - qcom,sm8150-dwc3
>> -              - qcom,sm8250-dwc3
>> -              - qcom,sm8350-dwc3
>> -              - qcom,sm8450-dwc3
>> -              - qcom,sm8550-dwc3
>> -              - qcom,sm8650-dwc3
>> -    then:
>> -      properties:
>> -        interrupts:
>> -          items:
>> -            - description: The interrupt that is asserted
>> -                when a wakeup event is received on USB2 bus.
>> -            - description: The interrupt that is asserted
>> -                when a wakeup event is received on USB3 bus.
>> -            - description: Wakeup event on DM line.
>> -            - description: Wakeup event on DP line.
>> -        interrupt-names:
>> -          items:
>> -            - const: hs_phy_irq
>> -            - const: ss_phy_irq
>> -            - const: dm_hs_phy_irq
>> -            - const: dp_hs_phy_irq
>> -
>> -  - if:
>> -      properties:
>> -        compatible:
>> -          contains:
>> -            enum:
>>                 - qcom,msm8953-dwc3
>> -              - qcom,msm8996-dwc3
>>                 - qcom,msm8998-dwc3
>> -              - qcom,sm6115-dwc3
>> -              - qcom,sm6125-dwc3
>> +              - qcom,qcm2290-dwc3
>>       then:
>>         properties:
>>           interrupts:
>> -          maxItems: 2
>> +          minItems: 2
>> +          maxItems: 3
>>           interrupt-names:
>>             items:
>> -            - const: hs_phy_irq
>> +            - const: pwr_event
>> +            - const: qusb2_phy
>>               - const: ss_phy_irq
>>   
>>     - if:
>> @@ -422,37 +400,21 @@ allOf:
>>           compatible:
>>             contains:
>>               enum:
>> -              - qcom,ipq5018-dwc3
>> -              - qcom,ipq5332-dwc3
>> +              - qcom,msm8996-dwc3
>> +              - qcom,qcs404-dwc3
>>                 - qcom,sdm660-dwc3
>> -    then:
>> -      properties:
>> -        interrupts:
>> -          minItems: 1
>> -          maxItems: 2
>> -        interrupt-names:
>> -          minItems: 1
>> -          items:
>> -            - const: hs_phy_irq
>> -            - const: ss_phy_irq
>> -
>> -  - if:
>> -      properties:
>> -        compatible:
>> -          contains:
>> -            enum:
>> -              - qcom,sc7280-dwc3
>> +              - qcom,sm6115-dwc3
>> +              - qcom,sm6125-dwc3
>>       then:
>>         properties:
>>           interrupts:
>>             minItems: 3
>>             maxItems: 4
>>           interrupt-names:
>> -          minItems: 3
>>             items:
>> +            - const: pwr_event
>>               - const: hs_phy_irq
>> -            - const: dp_hs_phy_irq
>> -            - const: dm_hs_phy_irq
>> +            - const: qusb2_phy
> 
> Why qusb2_phy is after hs_phy_irq? In the earlier if:then: it is the
> second one.
> 

In v3 as well, the hs_phy_irq is before qusb2_phy interrupt:
https://lore.kernel.org/all/20231211121124.4194-2-quic_kriskura@quicinc.com/

> 
>>               - const: ss_phy_irq
>>   
>>     - if:
>> @@ -460,11 +422,13 @@ allOf:
>>           compatible:
>>             contains:
>>               enum:
>> +              - qcom,ipq5332-dwc3
>>                 - qcom,sc8280xp-dwc3
>>                 - qcom,x1e80100-dwc3
>>       then:
>>         properties:
>>           interrupts:
>> +          minItems: 3
> 
> Hm, why? This commit is unmanageable. Your commit msg is already huge
> but still does not explain this. Are you sure you are fixing only one
> logical thing per patch? Does not look like.
> 

This is reordering the targets based on interrupts they have. I put it 
in one commit because splitting this into multiple patches breaks one 
thing or other. Also once I am defining permutations, I have to group 
targets into these combinations in the same patch. I know this is a big 
commit but it solves the interrupt cleanup and defines a way for future 
targets.

Regards,
Krishna,

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krzysztof Kozlowski]

On 26/12/2023 06:37, Krishna Kurapati PSSNV wrote:
> 
> 
> On 12/25/2023 6:35 PM, Krzysztof Kozlowski wrote:
>> On 22/12/2023 07:36, Krishna Kurapati wrote:
>>> The high speed related interrupts present on QC targets are as follows:
>>>
>>
>>
>>>   
>>>     interrupt-names:
>>> -    minItems: 1
>>> -    maxItems: 4
>>> +    minItems: 2
>>> +    maxItems: 5
>>>   
>>>     qcom,select-utmi-as-pipe-clk:
>>>       description:
>>> @@ -361,60 +378,21 @@ allOf:
>>>           compatible:
>>>             contains:
>>>               enum:
>>> -              - qcom,ipq4019-dwc3
>>
>> Why do you remove it, without adding it somewhere else. Nothing in the
>> commit msg explains it.
>>
> 
> Apologies, Will check and add it back.

Please check your patch for other entries. I just took first compatible
which turns out to be gone. I did not check the reset and I don't want
to keep checking.

...

>>> -    then:
>>> -      properties:
>>> -        interrupts:
>>> -          minItems: 1
>>> -          maxItems: 2
>>> -        interrupt-names:
>>> -          minItems: 1
>>> -          items:
>>> -            - const: hs_phy_irq
>>> -            - const: ss_phy_irq
>>> -
>>> -  - if:
>>> -      properties:
>>> -        compatible:
>>> -          contains:
>>> -            enum:
>>> -              - qcom,sc7280-dwc3
>>> +              - qcom,sm6115-dwc3
>>> +              - qcom,sm6125-dwc3
>>>       then:
>>>         properties:
>>>           interrupts:
>>>             minItems: 3
>>>             maxItems: 4
>>>           interrupt-names:
>>> -          minItems: 3
>>>             items:
>>> +            - const: pwr_event
>>>               - const: hs_phy_irq
>>> -            - const: dp_hs_phy_irq
>>> -            - const: dm_hs_phy_irq
>>> +            - const: qusb2_phy
>>
>> Why qusb2_phy is after hs_phy_irq? In the earlier if:then: it is the
>> second one.
>>
> 
> In v3 as well, the hs_phy_irq is before qusb2_phy interrupt:
> https://lore.kernel.org/all/20231211121124.4194-2-quic_kriskura@quicinc.com/

? How v3 matters?

> 
>>
>>>               - const: ss_phy_irq
>>>   
>>>     - if:
>>> @@ -460,11 +422,13 @@ allOf:
>>>           compatible:
>>>             contains:
>>>               enum:
>>> +              - qcom,ipq5332-dwc3
>>>                 - qcom,sc8280xp-dwc3
>>>                 - qcom,x1e80100-dwc3
>>>       then:
>>>         properties:
>>>           interrupts:
>>> +          minItems: 3
>>
>> Hm, why? This commit is unmanageable. Your commit msg is already huge
>> but still does not explain this. Are you sure you are fixing only one
>> logical thing per patch? Does not look like.
>>
> 
> This is reordering the targets based on interrupts they have. I put it 
> in one commit because splitting this into multiple patches breaks one 
> thing or other. Also once I am defining permutations, I have to group 
> targets into these combinations in the same patch. I know this is a big 
> commit but it solves the interrupt cleanup and defines a way for future 
> targets.


This does not answer why, you sc8280xp and x1e80100 not get one optional
interrupt. I asked "why" you are doing this change. Why do you need it?
What is the rationale?

Then I grunted about unmanageable commit, because all my troubles to
review it are the effect of it: it is very difficult to read. It is also
difficult for you, because you keep making here mistakes. So if you
cannot write this commit properly and I cannot review it, then it is way
over-complicated, don't you think? But this is still second problem
here, don't ignore the fist - "why?"

Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krishna Kurapati PSSNV]

On 12/26/2023 3:03 PM, Krzysztof Kozlowski wrote:
> On 26/12/2023 06:37, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 12/25/2023 6:35 PM, Krzysztof Kozlowski wrote:
>>> On 22/12/2023 07:36, Krishna Kurapati wrote:
>>>> The high speed related interrupts present on QC targets are as follows:
>>>>
>>>
>>>
>>>>    
>>>>      interrupt-names:
>>>> -    minItems: 1
>>>> -    maxItems: 4
>>>> +    minItems: 2
>>>> +    maxItems: 5
>>>>    
>>>>      qcom,select-utmi-as-pipe-clk:
>>>>        description:
>>>> @@ -361,60 +378,21 @@ allOf:
>>>>            compatible:
>>>>              contains:
>>>>                enum:
>>>> -              - qcom,ipq4019-dwc3
>>>
>>> Why do you remove it, without adding it somewhere else. Nothing in the
>>> commit msg explains it.
>>>
>>
>> Apologies, Will check and add it back.
> 
> Please check your patch for other entries. I just took first compatible
> which turns out to be gone. I did not check the reset and I don't want
> to keep checking.
> > ...
> 
>>>> -    then:
>>>> -      properties:
>>>> -        interrupts:
>>>> -          minItems: 1
>>>> -          maxItems: 2
>>>> -        interrupt-names:
>>>> -          minItems: 1
>>>> -          items:
>>>> -            - const: hs_phy_irq
>>>> -            - const: ss_phy_irq
>>>> -
>>>> -  - if:
>>>> -      properties:
>>>> -        compatible:
>>>> -          contains:
>>>> -            enum:
>>>> -              - qcom,sc7280-dwc3
>>>> +              - qcom,sm6115-dwc3
>>>> +              - qcom,sm6125-dwc3
>>>>        then:
>>>>          properties:
>>>>            interrupts:
>>>>              minItems: 3
>>>>              maxItems: 4
>>>>            interrupt-names:
>>>> -          minItems: 3
>>>>              items:
>>>> +            - const: pwr_event
>>>>                - const: hs_phy_irq
>>>> -            - const: dp_hs_phy_irq
>>>> -            - const: dm_hs_phy_irq
>>>> +            - const: qusb2_phy
>>>
>>> Why qusb2_phy is after hs_phy_irq? In the earlier if:then: it is the
>>> second one.
>>>
>>
>> In v3 as well, the hs_phy_irq is before qusb2_phy interrupt:
>> https://lore.kernel.org/all/20231211121124.4194-2-quic_kriskura@quicinc.com/
> 
> ? How v3 matters?
> 
>>
>>>
>>>>                - const: ss_phy_irq
>>>>    
>>>>      - if:
>>>> @@ -460,11 +422,13 @@ allOf:
>>>>            compatible:
>>>>              contains:
>>>>                enum:
>>>> +              - qcom,ipq5332-dwc3
>>>>                  - qcom,sc8280xp-dwc3
>>>>                  - qcom,x1e80100-dwc3
>>>>        then:
>>>>          properties:
>>>>            interrupts:
>>>> +          minItems: 3
>>>
>>> Hm, why? This commit is unmanageable. Your commit msg is already huge
>>> but still does not explain this. Are you sure you are fixing only one
>>> logical thing per patch? Does not look like.
>>>
>>
>> This is reordering the targets based on interrupts they have. I put it
>> in one commit because splitting this into multiple patches breaks one
>> thing or other. Also once I am defining permutations, I have to group
>> targets into these combinations in the same patch. I know this is a big
>> commit but it solves the interrupt cleanup and defines a way for future
>> targets.
> 
> 
> This does not answer why, you sc8280xp and x1e80100 not get one optional
> interrupt. I asked "why" you are doing this change. Why do you need it?
> What is the rationale?
> 
> Then I grunted about unmanageable commit, because all my troubles to
> review it are the effect of it: it is very difficult to read. It is also
> difficult for you, because you keep making here mistakes. So if you
> cannot write this commit properly and I cannot review it, then it is way
> over-complicated, don't you think? But this is still second problem
> here, don't ignore the fist - "why?"

HI Krzysztof,

  Thanks for the review.
  To answer the question,

"why ?" : The interrupts have been mis-interpreted on many platforms or 
many interrupts are missing.

Now, if I am adding the missing interrupts, I need to segregate targets 
also into respective buckets in the same patch and that is what making 
this patch a little complicated. Is it possible / acceptable to split 
this into two patches if this is the case. Can you help with suggestions 
from your end ? Or may be I am understanding your question wrong ? 😅

Regards,
Krishna,

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krzysztof Kozlowski]

On 26/12/2023 11:03, Krishna Kurapati PSSNV wrote:
>>>>>      - if:
>>>>> @@ -460,11 +422,13 @@ allOf:
>>>>>            compatible:
>>>>>              contains:
>>>>>                enum:
>>>>> +              - qcom,ipq5332-dwc3
>>>>>                  - qcom,sc8280xp-dwc3
>>>>>                  - qcom,x1e80100-dwc3
>>>>>        then:
>>>>>          properties:
>>>>>            interrupts:
>>>>> +          minItems: 3
>>>>
>>>> Hm, why? This commit is unmanageable. Your commit msg is already huge
>>>> but still does not explain this. Are you sure you are fixing only one
>>>> logical thing per patch? Does not look like.
>>>>
>>>
>>> This is reordering the targets based on interrupts they have. I put it
>>> in one commit because splitting this into multiple patches breaks one
>>> thing or other. Also once I am defining permutations, I have to group
>>> targets into these combinations in the same patch. I know this is a big
>>> commit but it solves the interrupt cleanup and defines a way for future
>>> targets.
>>
>>
>> This does not answer why, you sc8280xp and x1e80100 not get one optional
>> interrupt. I asked "why" you are doing this change. Why do you need it?
>> What is the rationale?
>>
>> Then I grunted about unmanageable commit, because all my troubles to
>> review it are the effect of it: it is very difficult to read. It is also
>> difficult for you, because you keep making here mistakes. So if you
>> cannot write this commit properly and I cannot review it, then it is way
>> over-complicated, don't you think? But this is still second problem
>> here, don't ignore the fist - "why?"
> 
> HI Krzysztof,
> 
>   Thanks for the review.
>   To answer the question,
> 
> "why ?" : The interrupts have been mis-interpreted on many platforms or 
> many interrupts are missing.

I asked about these two specific platforms. Please explain these
changes. Above is so generic that tells me nothing.

> 
> Now, if I am adding the missing interrupts, I need to segregate targets 
> also into respective buckets in the same patch and that is what making 
> this patch a little complicated. Is it possible / acceptable to split 
> this into two patches if this is the case. Can you help with suggestions 
> from your end ? Or may be I am understanding your question wrong ? 😅

Split the patch into manageable chunks.

Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krishna Kurapati PSSNV]

On 12/26/2023 5:52 PM, Krzysztof Kozlowski wrote:

>>>
>>> This does not answer why, you sc8280xp and x1e80100 not get one optional
>>> interrupt. I asked "why" you are doing this change. Why do you need it?
>>> What is the rationale?
>>>
>>> Then I grunted about unmanageable commit, because all my troubles to
>>> review it are the effect of it: it is very difficult to read. It is also
>>> difficult for you, because you keep making here mistakes. So if you
>>> cannot write this commit properly and I cannot review it, then it is way
>>> over-complicated, don't you think? But this is still second problem
>>> here, don't ignore the fist - "why?"
>>
>> HI Krzysztof,
>>
>>    Thanks for the review.
>>    To answer the question,
>>
>> "why ?" : The interrupts have been mis-interpreted on many platforms or
>> many interrupts are missing.
> 
> I asked about these two specific platforms. Please explain these
> changes. Above is so generic that tells me nothing.
> 

Is the question, "Why do x1e80100 and sc8280 don't have hs_phy_irq ?"
If so, I checked the SC8280 HW specifics and I see one small error. The 
name was printed wrong. I got it from another source. Will move sc8280 
to list having 5 interrupts. As per x1e80100, I wasn't able to get my 
hands on the hw specifics and I followed the following link by Abel Vesa:

https://lore.kernel.org/r/20231214-x1e80100-usb-v1-1-c22be5c0109e@linaro.org

As per the above patch, x1e80100 had only 4 interrupts.
For ipq5332, it has no hs_phy_irq and so I kept it under this section.

>>
>> Now, if I am adding the missing interrupts, I need to segregate targets
>> also into respective buckets in the same patch and that is what making
>> this patch a little complicated. Is it possible / acceptable to split
>> this into two patches if this is the case. Can you help with suggestions
>> from your end ? Or may be I am understanding your question wrong ? 😅
> 
> Split the patch into manageable chunks.
> 

I will try to split it up, but not sure if it is a good idea. I say so 
because all permutations should be added in single patch and I can't 
split that.

Regards,
Krishna,

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krzysztof Kozlowski]

On 26/12/2023 16:03, Krishna Kurapati PSSNV wrote:
> 
> 
> On 12/26/2023 5:52 PM, Krzysztof Kozlowski wrote:
> 
>>>>
>>>> This does not answer why, you sc8280xp and x1e80100 not get one optional
>>>> interrupt. I asked "why" you are doing this change. Why do you need it?
>>>> What is the rationale?
>>>>
>>>> Then I grunted about unmanageable commit, because all my troubles to
>>>> review it are the effect of it: it is very difficult to read. It is also
>>>> difficult for you, because you keep making here mistakes. So if you
>>>> cannot write this commit properly and I cannot review it, then it is way
>>>> over-complicated, don't you think? But this is still second problem
>>>> here, don't ignore the fist - "why?"
>>>
>>> HI Krzysztof,
>>>
>>>    Thanks for the review.
>>>    To answer the question,
>>>
>>> "why ?" : The interrupts have been mis-interpreted on many platforms or
>>> many interrupts are missing.
>>
>> I asked about these two specific platforms. Please explain these
>> changes. Above is so generic that tells me nothing.
>>
> 
> Is the question, "Why do x1e80100 and sc8280 don't have hs_phy_irq ?"

 No, not entirely, the question was why these have flexible number of
IRQs (last one optional)?


> If so, I checked the SC8280 HW specifics and I see one small error. The 
> name was printed wrong. I got it from another source. Will move sc8280 
> to list having 5 interrupts. As per x1e80100, I wasn't able to get my 
> hands on the hw specifics and I followed the following link by Abel Vesa:
> 
> https://lore.kernel.org/r/20231214-x1e80100-usb-v1-1-c22be5c0109e@linaro.org
> 
> As per the above patch, x1e80100 had only 4 interrupts.

Hm, ok, you say "4" but your patch says "minItems: 3". 3 != 4.

> For ipq5332, it has no hs_phy_irq and so I kept it under this section.
> 
>>>
>>> Now, if I am adding the missing interrupts, I need to segregate targets
>>> also into respective buckets in the same patch and that is what making
>>> this patch a little complicated. Is it possible / acceptable to split
>>> this into two patches if this is the case. Can you help with suggestions
>>> from your end ? Or may be I am understanding your question wrong ? 😅
>>
>> Split the patch into manageable chunks.
>>
> 
> I will try to split it up, but not sure if it is a good idea. I say so 
> because all permutations should be added in single patch and I can't 
> split that.
Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krishna Kurapati PSSNV]

On 12/27/2023 12:34 AM, Krzysztof Kozlowski wrote:
> On 26/12/2023 16:03, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 12/26/2023 5:52 PM, Krzysztof Kozlowski wrote:
>>
>>>>>
>>>>> This does not answer why, you sc8280xp and x1e80100 not get one optional
>>>>> interrupt. I asked "why" you are doing this change. Why do you need it?
>>>>> What is the rationale?
>>>>>
>>>>> Then I grunted about unmanageable commit, because all my troubles to
>>>>> review it are the effect of it: it is very difficult to read. It is also
>>>>> difficult for you, because you keep making here mistakes. So if you
>>>>> cannot write this commit properly and I cannot review it, then it is way
>>>>> over-complicated, don't you think? But this is still second problem
>>>>> here, don't ignore the fist - "why?"
>>>>
>>>> HI Krzysztof,
>>>>
>>>>     Thanks for the review.
>>>>     To answer the question,
>>>>
>>>> "why ?" : The interrupts have been mis-interpreted on many platforms or
>>>> many interrupts are missing.
>>>
>>> I asked about these two specific platforms. Please explain these
>>> changes. Above is so generic that tells me nothing.
>>>
>>
>> Is the question, "Why do x1e80100 and sc8280 don't have hs_phy_irq ?"
> 
>   No, not entirely, the question was why these have flexible number of
> IRQs (last one optional)?
> 
> 
>> If so, I checked the SC8280 HW specifics and I see one small error. The
>> name was printed wrong. I got it from another source. Will move sc8280
>> to list having 5 interrupts. As per x1e80100, I wasn't able to get my
>> hands on the hw specifics and I followed the following link by Abel Vesa:
>>
>> https://lore.kernel.org/r/20231214-x1e80100-usb-v1-1-c22be5c0109e@linaro.org
>>
>> As per the above patch, x1e80100 had only 4 interrupts.
> 
> Hm, ok, you say "4" but your patch says "minItems: 3". 3 != 4.
> 

Actually, you are right. We don't need the max/min items as we are sure 
that the targets mentioned under this have 4 interrupts definitively.

But the optional interrupt was put in just in case any target comes in 
that has no ss_phy and no hs_phy and has only the other 3 interrupts. 
Since those targets are not present currently, I will remove the max/min 
items from this.

Thanks for the catch. Sorry for bothering you with a couple of mails 
because I didn't understand the question you were trying to ask.

Regards,
Krishna,

Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krishna Kurapati PSSNV]

>>>>            interrupt-names:
>>>> -          minItems: 3
>>>>              items:
>>>> +            - const: pwr_event
>>>>                - const: hs_phy_irq
>>>> -            - const: dp_hs_phy_irq
>>>> -            - const: dm_hs_phy_irq
>>>> +            - const: qusb2_phy
>>>
>>> Why qusb2_phy is after hs_phy_irq? In the earlier if:then: it is the
>>> second one.
>>>
>>
>> In v3 as well, the hs_phy_irq is before qusb2_phy interrupt:
>> https://lore.kernel.org/all/20231211121124.4194-2-quic_kriskura@quicinc.com/
> 
> ? How v3 matters?
> 

I was thinking whether I modified the order between v3 and v5 and didn't 
mention in change log and hence I compared with v3. Thanks for the catch.

I made qusb2_phy the second one in the list and pushed v6: 
https://lore.kernel.org/all/20231227091951.685-1-quic_kriskura@quicinc.com/

Can you help provide you review on v6 as well.

Regards,
Krishna,