LinuxLists.cc - [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On 22/11/2023 20:13, Krishna Kurapati wrote:
> 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-QUSBb2 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.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> .../devicetree/bindings/usb/qcom,dwc3.yaml | 125 ++++++++++--------
> 1 file changed, 69 insertions(+), 56 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index e889158ca205..4a46346e2ead 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -17,20 +17,25 @@ properties:
> - qcom,ipq5018-dwc3
> - qcom,ipq5332-dwc3
> - qcom,ipq6018-dwc3
> + - qcom,ipq6018-dwc3-sec

I could not understand from commit msg why you are adding new compatible
and what it is supposed to fix.

The entire diff is huge thus difficult to review. Why fixing hs_phy_irq
causes three new interrupts being added?
> - qcom,ipq8064-dwc3
> - qcom,ipq8074-dwc3
> - qcom,ipq9574-dwc3
> - qcom,msm8953-dwc3
> - qcom,msm8994-dwc3
> - qcom,msm8996-dwc3
> + - qcom,msm8996-dwc3-sec
> - qcom,msm8998-dwc3
> - qcom,qcm2290-dwc3
> - qcom,qcs404-dwc3
> - qcom,sa8775p-dwc3
> + - qcom,sa8775p-dwc3-ter

Ter?

Best regards,
Krzysztof

2023-11-23 07:44:37

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On 11/23/2023 1:11 PM, Krzysztof Kozlowski wrote:
> On 22/11/2023 20:13, Krishna Kurapati wrote:
>> 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-QUSBb2 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.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> .../devicetree/bindings/usb/qcom,dwc3.yaml | 125 ++++++++++--------
>> 1 file changed, 69 insertions(+), 56 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> index e889158ca205..4a46346e2ead 100644
>> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> @@ -17,20 +17,25 @@ properties:
>> - qcom,ipq5018-dwc3
>> - qcom,ipq5332-dwc3
>> - qcom,ipq6018-dwc3
>> + - qcom,ipq6018-dwc3-sec
>
> I could not understand from commit msg why you are adding new compatible
> and what it is supposed to fix.
>
> The entire diff is huge thus difficult to review. Why fixing hs_phy_irq
> causes three new interrupts being added?

Some targets have two controllers where the second one is only HS
capable and doesn't have ss_phy_irq. In such cases to make it clear in
bindings, I added a suffix "-sec" and accordingly changed in DT as well.
Should've put this in commit text.

>> - qcom,ipq8064-dwc3
>> - qcom,ipq8074-dwc3
>> - qcom,ipq9574-dwc3
>> - qcom,msm8953-dwc3
>> - qcom,msm8994-dwc3
>> - qcom,msm8996-dwc3
>> + - qcom,msm8996-dwc3-sec
>> - qcom,msm8998-dwc3
>> - qcom,qcm2290-dwc3
>> - qcom,qcs404-dwc3
>> - qcom,sa8775p-dwc3
>> + - qcom,sa8775p-dwc3-ter
>
> Ter?
>
Tertiary controller.

Regards,
Krishna,

2023-11-23 07:50:31

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On 23/11/2023 08:44, Krishna Kurapati PSSNV wrote:
>
>
> On 11/23/2023 1:11 PM, Krzysztof Kozlowski wrote:
>> On 22/11/2023 20:13, Krishna Kurapati wrote:
>>> 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-QUSBb2 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.
>>>
>>> Signed-off-by: Krishna Kurapati <[email protected]>
>>> ---
>>> .../devicetree/bindings/usb/qcom,dwc3.yaml | 125 ++++++++++--------
>>> 1 file changed, 69 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>> index e889158ca205..4a46346e2ead 100644
>>> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>> @@ -17,20 +17,25 @@ properties:
>>> - qcom,ipq5018-dwc3
>>> - qcom,ipq5332-dwc3
>>> - qcom,ipq6018-dwc3
>>> + - qcom,ipq6018-dwc3-sec
>>
>> I could not understand from commit msg why you are adding new compatible
>> and what it is supposed to fix.
>>
>> The entire diff is huge thus difficult to review. Why fixing hs_phy_irq
>> causes three new interrupts being added?
>
> Some targets have two controllers where the second one is only HS
> capable and doesn't have ss_phy_irq. In such cases to make it clear in
> bindings, I added a suffix "-sec" and accordingly changed in DT as well.
> Should've put this in commit text.

Where did you explain it in the commit msg? Why adding new compatibles
is squashed to this patch?

You need separate commit with its own justification. I am not sure if
calling things secondary and tertiary scales. Please describe all the
differences and come with some reason for the naming.

Best regards,
Krzysztof

2023-11-23 07:57:45

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On 11/23/2023 1:20 PM, Krzysztof Kozlowski wrote:
> On 23/11/2023 08:44, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 11/23/2023 1:11 PM, Krzysztof Kozlowski wrote:
>>> On 22/11/2023 20:13, Krishna Kurapati wrote:
>>>> 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-QUSBb2 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.
>>>>
>>>> Signed-off-by: Krishna Kurapati <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/usb/qcom,dwc3.yaml | 125 ++++++++++--------
>>>> 1 file changed, 69 insertions(+), 56 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>>> index e889158ca205..4a46346e2ead 100644
>>>> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>>> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>>> @@ -17,20 +17,25 @@ properties:
>>>> - qcom,ipq5018-dwc3
>>>> - qcom,ipq5332-dwc3
>>>> - qcom,ipq6018-dwc3
>>>> + - qcom,ipq6018-dwc3-sec
>>>
>>> I could not understand from commit msg why you are adding new compatible
>>> and what it is supposed to fix.
>>>
>>> The entire diff is huge thus difficult to review. Why fixing hs_phy_irq
>>> causes three new interrupts being added?
>>
>> Some targets have two controllers where the second one is only HS
>> capable and doesn't have ss_phy_irq. In such cases to make it clear in
>> bindings, I added a suffix "-sec" and accordingly changed in DT as well.
>> Should've put this in commit text.
>
> Where did you explain it in the commit msg? Why adding new compatibles
> is squashed to this patch?
>
Apologies. I meant I should've put the explanation in commit text which
I missed. Will do it while revising this patch.

> You need separate commit with its own justification. I am not sure if
> calling things secondary and tertiary scales. Please describe all the
> differences and come with some reason for the naming.
>
>

Sure, will separate out the new compatible additions and then make
different commits.

Regards,
Krishna,

2023-11-23 14:10:39

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On Thu, Nov 23, 2023 at 12:43:35AM +0530, Krishna Kurapati wrote:
> 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-QUSBb2 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.

Thanks for sorting this out.

It seems like we have a few combinations of these interrupts and we
should probably try to define the order for these once and for all and
update the current devicetrees to match (even if it means adding new
interrupts in the middle).

Instead of adding separate compatibles for the controllers without SS
support, I suggest keeping that interrupt last as an optional one.

But IIUC we essentially have something like:

qusb2-:

- const: qusb2_phy
- const: pwr_event
- const: ss_phy_irq (optional)

qusb2:

- const: hs_phy_irq
- const: qusb2_phy
- const: pwr_event
- const: ss_phy_irq (optional)

qusb2+:

- const: hs_phy_irq
- const: qusb2_phy
- const: dp_hs_phy_irq
- const: dm_hs_phy_irq
- const: pwr_event
- const: ss_phy_irq (optional)

femto-:
- const: dp_hs_phy_irq
- const: dm_hs_phy_irq
- const: pwr_event
- const: ss_phy_irq (optional)

femto:
- const: hs_phy_irq
- const: dp_hs_phy_irq
- const: dm_hs_phy_irq
- const: pwr_event
- const: ss_phy_irq (optional)

Does this look like it would cover all of our currents SoCs?

Do all of them have the pwr_event interrupt?

Note that DP comes before DM above as that seems like the natural order
of these (plus before minus).

Now if the HS interrupt is truly unusable, I guess we can consider
dropping it throughout and the above becomes just three permutations
instead, which can even be expressed along the lines of:

- anyOf:
- items:
- const: qusb2_phy
- items:
- const: dp_hs_phy_irq
- const: dm_hs_phy_irq
- const: pwr_event
- const: ss_phy_irq (optional)

Johan

2023-11-24 12:03:44

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

>
> Thanks for sorting this out.
>
> It seems like we have a few combinations of these interrupts and we
> should probably try to define the order for these once and for all and
> update the current devicetrees to match (even if it means adding new
> interrupts in the middle).
>
> Instead of adding separate compatibles for the controllers without SS
> support, I suggest keeping that interrupt last as an optional one.
>
> But IIUC we essentially have something like:
>
> qusb2-:
>
> - const: qusb2_phy
> - const: pwr_event
> - const: ss_phy_irq (optional)
>
> qusb2:
>
> - const: hs_phy_irq
> - const: qusb2_phy
> - const: pwr_event
> - const: ss_phy_irq (optional)
>
> qusb2+:
>
> - const: hs_phy_irq
> - const: qusb2_phy
> - const: dp_hs_phy_irq
> - const: dm_hs_phy_irq
> - const: pwr_event
> - const: ss_phy_irq (optional)
>

This combination doesn't exist. So we can skip this one.

> femto-:
> - const: dp_hs_phy_irq
> - const: dm_hs_phy_irq
> - const: pwr_event
> - const: ss_phy_irq (optional)
>
> femto:
> - const: hs_phy_irq
> - const: dp_hs_phy_irq
> - const: dm_hs_phy_irq
> - const: pwr_event
> - const: ss_phy_irq (optional)
>
> Does this look like it would cover all of our currents SoCs?
>
> Do all of them have the pwr_event interrupt?
>
Yes. From whatever targets I was able to find, only one of them didn't
have the power_event irq. Rest all of them had. I will recheck that
particular one again.

> Note that DP comes before DM above as that seems like the natural order
> of these (plus before minus).
>
> Now if the HS interrupt is truly unusable, I guess we can consider
> dropping it throughout and the above becomes just three permutations
> instead, which can even be expressed along the lines of:
>

Infact, I wanted to do this but since you mentioned before that if HW
has it, we must describe it, I kept it in. But since this functionality
is confirmed to be mutually exclusive of qusb2/{dp/dm}, I am aligned to
skip it in bindings and drop it in DT.

> - anyOf:
> - items:
> - const: qusb2_phy
> - items:
> - const: dp_hs_phy_irq
> - const: dm_hs_phy_irq
> - const: pwr_event
> - const: ss_phy_irq (optional)
>

This must cover all cases AFAIK. How about we keep pwr_event also
optional for time being. The ones I am not able to find also would come
up under still binding block.

Regards,
Krishna,

2023-11-24 13:47:58

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On Fri, Nov 24, 2023 at 05:32:37PM +0530, Krishna Kurapati PSSNV wrote:
> >
> > Thanks for sorting this out.
> >
> > It seems like we have a few combinations of these interrupts and we
> > should probably try to define the order for these once and for all and
> > update the current devicetrees to match (even if it means adding new
> > interrupts in the middle).
> >
> > Instead of adding separate compatibles for the controllers without SS
> > support, I suggest keeping that interrupt last as an optional one.
> >
> > But IIUC we essentially have something like:
> >
> > qusb2-:
> >
> > - const: qusb2_phy
> > - const: pwr_event
> > - const: ss_phy_irq (optional)
> >
> > qusb2:
> >
> > - const: hs_phy_irq
> > - const: qusb2_phy
> > - const: pwr_event
> > - const: ss_phy_irq (optional)
> >
> > qusb2+:
> >
> > - const: hs_phy_irq
> > - const: qusb2_phy
> > - const: dp_hs_phy_irq
> > - const: dm_hs_phy_irq
> > - const: pwr_event
> > - const: ss_phy_irq (optional)
> >
>
> This combination doesn't exist. So we can skip this one.

Ok, good. I thought you said some QUSB2 platforms used DP/DM, but I guess
that means they don't have the qusb2_phy interrupt then.

> > femto-:
> > - const: dp_hs_phy_irq
> > - const: dm_hs_phy_irq
> > - const: pwr_event
> > - const: ss_phy_irq (optional)
> >
> > femto:
> > - const: hs_phy_irq
> > - const: dp_hs_phy_irq
> > - const: dm_hs_phy_irq
> > - const: pwr_event
> > - const: ss_phy_irq (optional)
> >
> > Does this look like it would cover all of our currents SoCs?
> >
> > Do all of them have the pwr_event interrupt?
>
> Yes. From whatever targets I was able to find, only one of them didn't
> have the power_event irq. Rest all of them had. I will recheck that
> particular one again.

Please do. The driver polls the corresponding status register on all
platforms currently, and perhaps this interrupt can one day be used to
get rid of the polling.

> > Note that DP comes before DM above as that seems like the natural order
> > of these (plus before minus).
> >
> > Now if the HS interrupt is truly unusable, I guess we can consider
> > dropping it throughout and the above becomes just three permutations
> > instead, which can even be expressed along the lines of:
>
> Infact, I wanted to do this but since you mentioned before that if HW
> has it, we must describe it, I kept it in. But since this functionality
> is confirmed to be mutually exclusive of qusb2/{dp/dm}, I am aligned to
> skip it in bindings and drop it in DT.

As I mentioned elsewhere, it depends on whether it can be used at all.
Not simply whether there is some other mechanism that can be used in its
stead. Such a decision should be left up to the implementation.

That's why I said "truly unusable" above. It's still not clear to me
whether that is the case or not.

> > - anyOf:
> > - items:
> > - const: qusb2_phy
> > - items:
> > - const: dp_hs_phy_irq
> > - const: dm_hs_phy_irq
> > - const: pwr_event
> > - const: ss_phy_irq (optional)
> >
>
> This must cover all cases AFAIK. How about we keep pwr_event also
> optional for time being. The ones I am not able to find also would come
> up under still binding block.

No, we should avoid that if we can as with two many optional things,
these quickly gets messy (one optional interrupt at the end is fine and
can be expressed using min/maxItems).

If the "qusb2+" combination above isn't needed, then we're down to four
permutations, which is few enough to be spelled out explicitly even if
we decide that the hs_phy_irq should be kept in. Without hs_phy_irq, it
seems there's really only two permutations.

Johan

2023-11-24 17:39:45

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

>>
>> Yes. From whatever targets I was able to find, only one of them didn't
>> have the power_event irq. Rest all of them had. I will recheck that
>> particular one again.
>
> Please do. The driver polls the corresponding status register on all
> platforms currently, and perhaps this interrupt can one day be used to
> get rid of the polling.
>

Ok, I just rechecked and case is, I am not able to get my hands on the
doc. I can't say for sure that the target is missing the pwr_event
interrupt. I say we can safely add the target assuming pwr_event is
present for ipq9574. Every target so far even on downstream has this IRQ
present in hw.

>>> Note that DP comes before DM above as that seems like the natural order
>>> of these (plus before minus).
>>>
>>> Now if the HS interrupt is truly unusable, I guess we can consider
>>> dropping it throughout and the above becomes just three permutations
>>> instead, which can even be expressed along the lines of:
>>
>> Infact, I wanted to do this but since you mentioned before that if HW
>> has it, we must describe it, I kept it in. But since this functionality
>> is confirmed to be mutually exclusive of qusb2/{dp/dm}, I am aligned to
>> skip it in bindings and drop it in DT.
>
> As I mentioned elsewhere, it depends on whether it can be used at all.
> Not simply whether there is some other mechanism that can be used in its
> stead. Such a decision should be left up to the implementation.
>
> That's why I said "truly unusable" above. It's still not clear to me
> whether that is the case or not.
>

I looked at the code of 4.4, 4.14/ 4.19/ 5.4/ 5.10/ 5.15/ 6.1 and none
of them implement the hs_phy_irq.

>>> - anyOf:
>>> - items:
>>> - const: qusb2_phy
>>> - items:
>>> - const: dp_hs_phy_irq
>>> - const: dm_hs_phy_irq
>>> - const: pwr_event
>>> - const: ss_phy_irq (optional)
>>>
>>
>> This must cover all cases AFAIK. How about we keep pwr_event also
>> optional for time being. The ones I am not able to find also would come
>> up under still binding block.
>
> No, we should avoid that if we can as with two many optional things,
> these quickly gets messy (one optional interrupt at the end is fine and
> can be expressed using min/maxItems).
>
> If the "qusb2+" combination above isn't needed, then we're down to four
> permutations, which is few enough to be spelled out explicitly even if
> we decide that the hs_phy_irq should be kept in. Without hs_phy_irq, it
> seems there's really only two permutations.
>

My opinion would be to keep the power_event irq as mandatory and not to
include the hs_phy_irq.

Regards,
Krishna,

2023-11-28 10:20:48

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On Fri, Nov 24, 2023 at 11:09:04PM +0530, Krishna Kurapati PSSNV wrote:
> >> Yes. From whatever targets I was able to find, only one of them didn't
> >> have the power_event irq. Rest all of them had. I will recheck that
> >> particular one again.
> >
> > Please do. The driver polls the corresponding status register on all
> > platforms currently, and perhaps this interrupt can one day be used to
> > get rid of the polling.
> >
>
> Ok, I just rechecked and case is, I am not able to get my hands on the
> doc. I can't say for sure that the target is missing the pwr_event
> interrupt. I say we can safely add the target assuming pwr_event is
> present for ipq9574. Every target so far even on downstream has this IRQ
> present in hw.

Ok, good.

> >>> Now if the HS interrupt is truly unusable, I guess we can consider
> >>> dropping it throughout and the above becomes just three permutations
> >>> instead, which can even be expressed along the lines of:
> >>
> >> Infact, I wanted to do this but since you mentioned before that if HW
> >> has it, we must describe it, I kept it in. But since this functionality
> >> is confirmed to be mutually exclusive of qusb2/{dp/dm}, I am aligned to
> >> skip it in bindings and drop it in DT.
> >
> > As I mentioned elsewhere, it depends on whether it can be used at all.
> > Not simply whether there is some other mechanism that can be used in its
> > stead. Such a decision should be left up to the implementation.
> >
> > That's why I said "truly unusable" above. It's still not clear to me
> > whether that is the case or not.
>
> I looked at the code of 4.4, 4.14/ 4.19/ 5.4/ 5.10/ 5.15/ 6.1 and none
> of them implement the hs_phy_irq.

But again, that is completely irrelevant. As I've said numerous times
now, this is about what the hardware is capable of, not which
functionality a particular OS chooses to use.

> My opinion would be to keep the power_event irq as mandatory and not to
> include the hs_phy_irq.

Ok, but you still need to explain why dropping hs_phy_irq is correct.

Until there's a clear answer to that, it seems we need to include it.

Johan

2023-11-28 10:37:52

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

>>>>> Now if the HS interrupt is truly unusable, I guess we can consider
>>>>> dropping it throughout and the above becomes just three permutations
>>>>> instead, which can even be expressed along the lines of:
>>>>
>>>> Infact, I wanted to do this but since you mentioned before that if HW
>>>> has it, we must describe it, I kept it in. But since this functionality
>>>> is confirmed to be mutually exclusive of qusb2/{dp/dm}, I am aligned to
>>>> skip it in bindings and drop it in DT.
>>>
>>> As I mentioned elsewhere, it depends on whether it can be used at all.
>>> Not simply whether there is some other mechanism that can be used in its
>>> stead. Such a decision should be left up to the implementation.
>>>
>>> That's why I said "truly unusable" above. It's still not clear to me
>>> whether that is the case or not.
>>
>> I looked at the code of 4.4, 4.14/ 4.19/ 5.4/ 5.10/ 5.15/ 6.1 and none
>> of them implement the hs_phy_irq.
>
> But again, that is completely irrelevant. As I've said numerous times
> now, this is about what the hardware is capable of, not which
> functionality a particular OS chooses to use.
> >> My opinion would be to keep the power_event irq as mandatory and not to
>> include the hs_phy_irq.
>
> Ok, but you still need to explain why dropping hs_phy_irq is correct.
>
> Until there's a clear answer to that, it seems we need to include it.

Sure, I agree with you. It should describe what hardware is capable of,
not what we choose to add in driver code. In that sense we can add the
hs_phy_irq to all targets.

In my next revision, I can do the following:

- anyOf:
- items:
- const: qusb2_phy
- items:
- const: dp_hs_phy_irq
- const: dm_hs_phy_irq
- const: hs_phy_irq
- const: pwr_event
- const: ss_phy_irq (optional)

A modified version of your suggestion should help cover all cases and
describe all DT's perfectly.

Let me know your thoughts on this.

Regards,
Krishna,

2023-11-28 10:57:33

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On Tue, Nov 28, 2023 at 04:02:53PM +0530, Krishna Kurapati PSSNV wrote:

> > >> My opinion would be to keep the power_event irq as mandatory and not to
> >> include the hs_phy_irq.
> >
> > Ok, but you still need to explain why dropping hs_phy_irq is correct.
> >
> > Until there's a clear answer to that, it seems we need to include it.
>
> Sure, I agree with you. It should describe what hardware is capable of,
> not what we choose to add in driver code. In that sense we can add the
> hs_phy_irq to all targets.
>
> In my next revision, I can do the following:
>
> - anyOf:
> - items:
> - const: qusb2_phy
> - items:
> - const: dp_hs_phy_irq
> - const: dm_hs_phy_irq
> - const: hs_phy_irq
> - const: pwr_event
> - const: ss_phy_irq (optional)
>
> A modified version of your suggestion should help cover all cases and
> describe all DT's perfectly.

It may be better to spell out the permutations. Especially since there
are no platforms with both qusb2_phy and dp/dm as I thought when
mentioning the above. Using anyOf also makes it hard to specify
min/maxItems as you'll need to do.

So back to my initial proposal, with a slight modification moving
pwr_event first (e.g. as it is not a wakeup interrupt):

qusb2-:

- const: pwr_event
- const: qusb2_phy
- const: ss_phy_irq (optional)

qusb2:

- const: pwr_event
- const: hs_phy_irq
- const: qusb2_phy
- const: ss_phy_irq (optional)

femto-:
- const: pwr_event
- const: dp_hs_phy_irq
- const: dm_hs_phy_irq
- const: ss_phy_irq (optional)

femto:
- const: pwr_event
- const: hs_phy_irq
- const: dp_hs_phy_irq
- const: dm_hs_phy_irq
- const: ss_phy_irq (optional)

That ss_phy_irq is optional would be expressed as minItems being one
less than maxItems for each permutation.

Johan

2023-11-28 11:32:40

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

>
> So back to my initial proposal, with a slight modification moving
> pwr_event first (e.g. as it is not a wakeup interrupt):
>
> qusb2-:
>
> - const: pwr_event
> - const: qusb2_phy
> - const: ss_phy_irq (optional)
>
> qusb2:
>
> - const: pwr_event
> - const: hs_phy_irq
> - const: qusb2_phy
> - const: ss_phy_irq (optional)
>
> femto-:
> - const: pwr_event
> - const: dp_hs_phy_irq
> - const: dm_hs_phy_irq
> - const: ss_phy_irq (optional)
>
> femto:
> - const: pwr_event
> - const: hs_phy_irq
> - const: dp_hs_phy_irq
> - const: dm_hs_phy_irq
> - const: ss_phy_irq (optional)
>
> That ss_phy_irq is optional would be expressed as minItems being one
> less than maxItems for each permutation.

I am fine with this except that "femto-" might not be required. I looked
at
https://lore.kernel.org/all/[email protected]/
and I see that all of them have hs_phy_irq.

Regards,
Krishna,

2023-11-29 09:30:47

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On 28/11/2023 12:32, Krishna Kurapati PSSNV wrote:
>
>>
>> So back to my initial proposal, with a slight modification moving
>> pwr_event first (e.g. as it is not a wakeup interrupt):
>>
>> qusb2-:
>>
>> - const: pwr_event
>> - const: qusb2_phy
>> - const: ss_phy_irq (optional)
>>
>> qusb2:
>>
>> - const: pwr_event
>> - const: hs_phy_irq
>> - const: qusb2_phy
>> - const: ss_phy_irq (optional)
>>
>> femto-:
>> - const: pwr_event
>> - const: dp_hs_phy_irq
>> - const: dm_hs_phy_irq
>> - const: ss_phy_irq (optional)
>>
>> femto:
>> - const: pwr_event
>> - const: hs_phy_irq
>> - const: dp_hs_phy_irq
>> - const: dm_hs_phy_irq
>> - const: ss_phy_irq (optional)

I did not follow entire thread and I do not know whether you change the
order in existing bindings, but just in case: the entries in existing
bindings cannot change the order. That's a strict ABI requirement
recently also discussed with Bjorn, because we want to have stable DTB
for laptop platforms. If my comment is not relevant, then please ignore.

Best regards,
Krzysztof

2023-11-29 10:16:01

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On Wed, Nov 29, 2023 at 10:28:25AM +0100, Krzysztof Kozlowski wrote:
> On 28/11/2023 12:32, Krishna Kurapati PSSNV wrote:
> >
> >>
> >> So back to my initial proposal, with a slight modification moving
> >> pwr_event first (e.g. as it is not a wakeup interrupt):
> >>
> >> qusb2-:
> >>
> >> - const: pwr_event
> >> - const: qusb2_phy
> >> - const: ss_phy_irq (optional)
> >>
> >> qusb2:
> >>
> >> - const: pwr_event
> >> - const: hs_phy_irq
> >> - const: qusb2_phy
> >> - const: ss_phy_irq (optional)
> >>
> >> femto-:
> >> - const: pwr_event
> >> - const: dp_hs_phy_irq
> >> - const: dm_hs_phy_irq
> >> - const: ss_phy_irq (optional)
> >>
> >> femto:
> >> - const: pwr_event
> >> - const: hs_phy_irq
> >> - const: dp_hs_phy_irq
> >> - const: dm_hs_phy_irq
> >> - const: ss_phy_irq (optional)
>
> I did not follow entire thread and I do not know whether you change the
> order in existing bindings, but just in case: the entries in existing
> bindings cannot change the order. That's a strict ABI requirement
> recently also discussed with Bjorn, because we want to have stable DTB
> for laptop platforms. If my comment is not relevant, then please ignore.

Your comment is relevant, but I'm not sure I agree.

The Qualcomm bindings are a complete mess of DT snippets copied from
vendor trees and which have not been sanitised properly before being
merged upstream (partly due to there not being any public documentation
available).

This amounts to an unmaintainable mess which is reflected in the
binding schemas which similarly needs to encode every random order which
the SoC happened to use when being upstreamed. That makes the binding
documentation unreadable too, and the next time a new SoC is upstreamed
there is no clear hints of what the binding should look like, and we end
up with yet another permutation.

As part of this exercise, we've also determined that some of the
devicetrees that are already upstream are incorrect as well as
incomplete.

I really see no alternative to ripping of the plaster and cleaning this
up once and for all even if it "breaks" some imaginary OS which (unlike
Linux) relies on the current random order of these interrupts.

[ If there were any real OSes actually relying on the order, then that
would be a different thing of course. ]

Johan

2023-11-29 10:50:36

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On 11/29/2023 3:46 PM, Johan Hovold wrote:
> On Wed, Nov 29, 2023 at 10:28:25AM +0100, Krzysztof Kozlowski wrote:
>> On 28/11/2023 12:32, Krishna Kurapati PSSNV wrote:
>>>
>>>>
>>>> So back to my initial proposal, with a slight modification moving
>>>> pwr_event first (e.g. as it is not a wakeup interrupt):
>>>>
>>>> qusb2-:
>>>>
>>>> - const: pwr_event
>>>> - const: qusb2_phy
>>>> - const: ss_phy_irq (optional)
>>>>
>>>> qusb2:
>>>>
>>>> - const: pwr_event
>>>> - const: hs_phy_irq
>>>> - const: qusb2_phy
>>>> - const: ss_phy_irq (optional)
>>>>
>>>> femto-:
>>>> - const: pwr_event
>>>> - const: dp_hs_phy_irq
>>>> - const: dm_hs_phy_irq
>>>> - const: ss_phy_irq (optional)
>>>>
>>>> femto:
>>>> - const: pwr_event
>>>> - const: hs_phy_irq
>>>> - const: dp_hs_phy_irq
>>>> - const: dm_hs_phy_irq
>>>> - const: ss_phy_irq (optional)
>>
>> I did not follow entire thread and I do not know whether you change the
>> order in existing bindings, but just in case: the entries in existing
>> bindings cannot change the order. That's a strict ABI requirement
>> recently also discussed with Bjorn, because we want to have stable DTB
>> for laptop platforms. If my comment is not relevant, then please ignore.
>
> Your comment is relevant, but I'm not sure I agree.
>
> The Qualcomm bindings are a complete mess of DT snippets copied from
> vendor trees and which have not been sanitised properly before being
> merged upstream (partly due to there not being any public documentation
> available).
>
> This amounts to an unmaintainable mess which is reflected in the
> binding schemas which similarly needs to encode every random order which
> the SoC happened to use when being upstreamed. That makes the binding
> documentation unreadable too, and the next time a new SoC is upstreamed
> there is no clear hints of what the binding should look like, and we end
> up with yet another permutation.
>
> As part of this exercise, we've also determined that some of the
> devicetrees that are already upstream are incorrect as well as
> incomplete.
>
> I really see no alternative to ripping of the plaster and cleaning this
> up once and for all even if it "breaks" some imaginary OS which (unlike
> Linux) relies on the current random order of these interrupts.
>
> [ If there were any real OSes actually relying on the order, then that
> would be a different thing of course. ]
>

Hi Krzysztof, Johan,

We are modifying all the DT's in accordance to bindings as well.
Still it would be breaking ABI ?

Regards,
Krishna,

2023-11-30 08:09:11

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On 29/11/2023 11:50, Krishna Kurapati PSSNV wrote:
>
>
> On 11/29/2023 3:46 PM, Johan Hovold wrote:
>> On Wed, Nov 29, 2023 at 10:28:25AM +0100, Krzysztof Kozlowski wrote:
>>> On 28/11/2023 12:32, Krishna Kurapati PSSNV wrote:
>>>>
>>>>>
>>>>> So back to my initial proposal, with a slight modification moving
>>>>> pwr_event first (e.g. as it is not a wakeup interrupt):
>>>>>
>>>>> qusb2-:
>>>>>
>>>>> - const: pwr_event
>>>>> - const: qusb2_phy
>>>>> - const: ss_phy_irq (optional)
>>>>>
>>>>> qusb2:
>>>>>
>>>>> - const: pwr_event
>>>>> - const: hs_phy_irq
>>>>> - const: qusb2_phy
>>>>> - const: ss_phy_irq (optional)
>>>>>
>>>>> femto-:
>>>>> - const: pwr_event
>>>>> - const: dp_hs_phy_irq
>>>>> - const: dm_hs_phy_irq
>>>>> - const: ss_phy_irq (optional)
>>>>>
>>>>> femto:
>>>>> - const: pwr_event
>>>>> - const: hs_phy_irq
>>>>> - const: dp_hs_phy_irq
>>>>> - const: dm_hs_phy_irq
>>>>> - const: ss_phy_irq (optional)
>>>
>>> I did not follow entire thread and I do not know whether you change the
>>> order in existing bindings, but just in case: the entries in existing
>>> bindings cannot change the order. That's a strict ABI requirement
>>> recently also discussed with Bjorn, because we want to have stable DTB
>>> for laptop platforms. If my comment is not relevant, then please ignore.
>>
>> Your comment is relevant, but I'm not sure I agree.
>>
>> The Qualcomm bindings are a complete mess of DT snippets copied from
>> vendor trees and which have not been sanitised properly before being
>> merged upstream (partly due to there not being any public documentation
>> available).
>>
>> This amounts to an unmaintainable mess which is reflected in the
>> binding schemas which similarly needs to encode every random order which
>> the SoC happened to use when being upstreamed. That makes the binding
>> documentation unreadable too, and the next time a new SoC is upstreamed
>> there is no clear hints of what the binding should look like, and we end
>> up with yet another permutation.
>>
>> As part of this exercise, we've also determined that some of the
>> devicetrees that are already upstream are incorrect as well as
>> incomplete.
>>
>> I really see no alternative to ripping of the plaster and cleaning this
>> up once and for all even if it "breaks" some imaginary OS which (unlike
>> Linux) relies on the current random order of these interrupts.
>>
>> [ If there were any real OSes actually relying on the order, then that
>> would be a different thing of course. ]
>>
>
> Hi Krzysztof, Johan,
>
> We are modifying all the DT's in accordance to bindings as well.
> Still it would be breaking ABI ?

Yes, how can you modify DTB stored in firmware on the customer board?

Best regards,
Krzysztof

2023-11-30 08:16:57

[permalink] [raw]

Subject: Re: [PATCH 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings

On 29/11/2023 11:16, Johan Hovold wrote:
> On Wed, Nov 29, 2023 at 10:28:25AM +0100, Krzysztof Kozlowski wrote:
>> On 28/11/2023 12:32, Krishna Kurapati PSSNV wrote:
>>>
>>>>
>>>> So back to my initial proposal, with a slight modification moving
>>>> pwr_event first (e.g. as it is not a wakeup interrupt):
>>>>
>>>> qusb2-:
>>>>
>>>> - const: pwr_event
>>>> - const: qusb2_phy
>>>> - const: ss_phy_irq (optional)
>>>>
>>>> qusb2:
>>>>
>>>> - const: pwr_event
>>>> - const: hs_phy_irq
>>>> - const: qusb2_phy
>>>> - const: ss_phy_irq (optional)
>>>>
>>>> femto-:
>>>> - const: pwr_event
>>>> - const: dp_hs_phy_irq
>>>> - const: dm_hs_phy_irq
>>>> - const: ss_phy_irq (optional)
>>>>
>>>> femto:
>>>> - const: pwr_event
>>>> - const: hs_phy_irq
>>>> - const: dp_hs_phy_irq
>>>> - const: dm_hs_phy_irq
>>>> - const: ss_phy_irq (optional)
>>
>> I did not follow entire thread and I do not know whether you change the
>> order in existing bindings, but just in case: the entries in existing
>> bindings cannot change the order. That's a strict ABI requirement
>> recently also discussed with Bjorn, because we want to have stable DTB
>> for laptop platforms. If my comment is not relevant, then please ignore.
>
> Your comment is relevant, but I'm not sure I agree.
>
> The Qualcomm bindings are a complete mess of DT snippets copied from
> vendor trees and which have not been sanitised properly before being
> merged upstream (partly due to there not being any public documentation
> available).

True.

>
> This amounts to an unmaintainable mess which is reflected in the
> binding schemas which similarly needs to encode every random order which
> the SoC happened to use when being upstreamed. That makes the binding
> documentation unreadable too, and the next time a new SoC is upstreamed
> there is no clear hints of what the binding should look like, and we end
> up with yet another permutation.

While in general I agree for the bindings, but here, for order of the
interrupts, I am not really sure if this contributes to unreadable or
unmaintainable binding.

>
> As part of this exercise, we've also determined that some of the
> devicetrees that are already upstream are incorrect as well as
> incomplete.

Sure, good explanation for an ABI break.

>
> I really see no alternative to ripping of the plaster and cleaning this
> up once and for all even if it "breaks" some imaginary OS which (unlike
> Linux) relies on the current random order of these interrupts.
>
> [ If there were any real OSes actually relying on the order, then that
> would be a different thing of course. ]

The commit breaking the ABI can justify the reasons, including expected
impact (e.g. none for Linux).

While the second part probably you can justify (interrupts are taken by
name), the reason for ABI break like "I think it is poor code, so I will
ignore ABI" is not enough.

Best regards,
Krzysztof

2023-11-30 08:33:50