| 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 |
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 <quic_kriskura@quicinc.com> > --- > .../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
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 <quic_kriskura@quicinc.com> >>> --- >>> .../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
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
> > 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,
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
>> >> 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,
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
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
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,
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
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
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 --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>;
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(-)