diff mbox series

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

Message ID 20231122191335.3058-1-quic_kriskura@quicinc.com
State New
Headers show
Series [1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings | expand

Commit Message

Krishna Kurapati Nov. 22, 2023, 7:13 p.m. UTC
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 <quic_kriskura@quicinc.com>
---
 .../devicetree/bindings/usb/qcom,dwc3.yaml    | 125 ++++++++++--------
 1 file changed, 69 insertions(+), 56 deletions(-)

Comments

Johan Hovold Nov. 23, 2023, 2:10 p.m. UTC | #1
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
Krishna Kurapati Nov. 24, 2023, 12:02 p.m. UTC | #2
> 
> 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,
Johan Hovold Nov. 24, 2023, 1:46 p.m. UTC | #3
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
Krishna Kurapati Nov. 24, 2023, 5:39 p.m. UTC | #4
>>
>> 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,
Krzysztof Kozlowski Nov. 29, 2023, 9:28 a.m. UTC | #5
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
Krishna Kurapati Nov. 29, 2023, 10:50 a.m. UTC | #6
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,
Krzysztof Kozlowski Nov. 30, 2023, 8:08 a.m. UTC | #7
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
Krzysztof Kozlowski Nov. 30, 2023, 8:16 a.m. UTC | #8
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
Johan Hovold Nov. 30, 2023, 8:34 a.m. UTC | #9
On Thu, Nov 30, 2023 at 09:16:41AM +0100, Krzysztof Kozlowski wrote:
> 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.

The more if-then clauses you have, the harder it gets for a human to
make sense of the binding documents.

By cleaning up the current clauses in four groups which reflect actual
classes of hardware and not just arbitrary reordering and omission, it
will make it much easier next time a new SoC is added. Most likely it
belongs in the latest category, and a reviewer can more easily spot new
mistakes if someone tries to add 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.
> 
> 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.

So it's not so much about the code as the messy binding schema this
results in and that that makes it harder to spot mistakes next time an
SoC is upstreamed.

Johan
diff mbox series

Patch

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
           - 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
           - qcom,sc7180-dwc3
           - qcom,sc7280-dwc3
+          - qcom,sc7280-dwc3-sec
           - qcom,sc8280xp-dwc3
           - qcom,sdm660-dwc3
+          - qcom,sdm660-dwc3-sec
           - qcom,sdm670-dwc3
           - qcom,sdm845-dwc3
           - qcom,sdx55-dwc3
@@ -98,11 +103,11 @@  properties:
 
   interrupts:
     minItems: 1
-    maxItems: 4
+    maxItems: 5
 
   interrupt-names:
     minItems: 1
-    maxItems: 4
+    maxItems: 5
 
   qcom,select-utmi-as-pipe-clk:
     description:
@@ -175,10 +180,13 @@  allOf:
               - qcom,ipq9574-dwc3
               - qcom,msm8953-dwc3
               - qcom,msm8996-dwc3
+              - qcom,msm8996-dwc3-sec
               - qcom,msm8998-dwc3
               - qcom,sa8775p-dwc3
+              - qcom,sa8775p-dwc3-ter
               - qcom,sc7180-dwc3
               - qcom,sc7280-dwc3
+              - qcom,sc7280-dwc3-sec
               - qcom,sdm670-dwc3
               - qcom,sdm845-dwc3
               - qcom,sdx55-dwc3
@@ -203,6 +211,7 @@  allOf:
           contains:
             enum:
               - qcom,ipq6018-dwc3
+              - qcom,ipq6018-dwc3-sec
     then:
       properties:
         clocks:
@@ -285,6 +294,7 @@  allOf:
           contains:
             enum:
               - qcom,sdm660-dwc3
+              - qcom,sdm660-dwc3-sec
     then:
       properties:
         clocks:
@@ -357,20 +367,15 @@  allOf:
         compatible:
           contains:
             enum:
-              - qcom,ipq4019-dwc3
-              - qcom,ipq6018-dwc3
-              - qcom,ipq8064-dwc3
-              - qcom,ipq8074-dwc3
-              - qcom,msm8994-dwc3
-              - qcom,qcs404-dwc3
+              - qcom,sc8280xp-dwc3
+              - 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,sm4250-dwc3
-              - qcom,sm6125-dwc3
               - qcom,sm6350-dwc3
               - qcom,sm8150-dwc3
               - qcom,sm8250-dwc3
@@ -381,16 +386,37 @@  allOf:
       properties:
         interrupts:
           items:
+            - description: Wakeup event on DM line.
+            - description: Wakeup event on DP line.
             - description: The interrupt that is asserted
-                when a wakeup event is received on USB2 bus.
+                based on linestates. Is enabled if qscratch
+                registers are configured appropirately. This
+                interrupt functionality is mutually exclusive
+                to that of {dp/d}_hs_phy_irq)
+            - description: Wakeup based on power events.
             - 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: dm_hs_phy_irq
+            - const: dp_hs_phy_irq
             - const: hs_phy_irq
+            - const: pwr_event
             - const: ss_phy_irq
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc7280-dwc3-sec
+              - qcom,sa8775p-ter
+    then:
+      properties:
+        interrupt-names:
+          items:
+            - const: pwr_event
+            - const: hs_phy_irq
             - const: dm_hs_phy_irq
             - const: dp_hs_phy_irq
 
@@ -399,36 +425,29 @@  allOf:
         compatible:
           contains:
             enum:
-              - qcom,msm8953-dwc3
-              - qcom,msm8996-dwc3
-              - qcom,msm8998-dwc3
-              - qcom,sm6115-dwc3
+              - qcom,ipq6018-dwc3-sec
     then:
       properties:
-        interrupts:
-          maxItems: 2
         interrupt-names:
           items:
-            - const: hs_phy_irq
-            - const: ss_phy_irq
+            - const: pwr_event
+            - const: qusb2_phy
 
   - if:
       properties:
         compatible:
           contains:
             enum:
-              - qcom,ipq5018-dwc3
-              - qcom,ipq5332-dwc3
-              - qcom,sdm660-dwc3
+              - qcom,ipq6018-dwc3
+              - qcom,ipq8074-dwc3
+              - qcom,msm8953-dwc3
+              - qcom,msm8998-dwc3
     then:
       properties:
-        interrupts:
-          minItems: 1
-          maxItems: 2
         interrupt-names:
-          minItems: 1
           items:
-            - const: hs_phy_irq
+            - const: pwr_event
+            - const: qusb2_phy
             - const: ss_phy_irq
 
   - if:
@@ -436,55 +455,48 @@  allOf:
         compatible:
           contains:
             enum:
-              - qcom,sc7280-dwc3
+              - qcom,msm8996-dwc3
+              - qcom,sdm660-dwc3
+              - qcom,sm4250-dwc3
+              - qcom,sm6115-dwc3
+              - qcom,sm6125-dwc3
     then:
       properties:
-        interrupts:
-          minItems: 3
-          maxItems: 4
         interrupt-names:
-          minItems: 3
           items:
             - const: hs_phy_irq
-            - const: dp_hs_phy_irq
-            - const: dm_hs_phy_irq
+            - const: pwr_event
+            - const: qusb2_phy
             - const: ss_phy_irq
-
   - if:
       properties:
         compatible:
           contains:
             enum:
-              - qcom,sc8280xp-dwc3
+              - qcom,sdm660-dwc3-sec
+              - qcom,msm8996-dwc3-sec
+              - qcom,qcs404-dwc3
     then:
       properties:
-        interrupts:
-          maxItems: 4
         interrupt-names:
           items:
+            - const: hs_phy_irq
             - const: pwr_event
-            - const: dp_hs_phy_irq
-            - const: dm_hs_phy_irq
-            - const: ss_phy_irq
+            - const: qusb2_phy
 
   - if:
       properties:
         compatible:
           contains:
             enum:
-              - qcom,sa8775p-dwc3
+              - qcom,ipq5332-dwc3
     then:
       properties:
-        interrupts:
-          minItems: 3
-          maxItems: 4
         interrupt-names:
-          minItems: 3
           items:
-            - const: pwr_event
             - const: dp_hs_phy_irq
             - const: dm_hs_phy_irq
-            - const: ss_phy_irq
+            - const: pwr_event
 
 additionalProperties: false
 
@@ -519,12 +531,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>,
-                         <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
-            interrupt-names = "hs_phy_irq", "ss_phy_irq",
-                          "dm_hs_phy_irq", "dp_hs_phy_irq";
+            interrupts = <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "dm_hs_phy_irq", "dp_hs_phy_irq",
+                              "hs_phy_irq", "pwr_event", "ss_phy_irq";
 
             power-domains = <&gcc USB30_PRIM_GDSC>;