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

[Johan Hovold]

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