On Sat, Apr 22, 2023 at 09:38:44PM +0530, Krishna Kurapati PSSNV wrote: > > > On 4/16/2023 12:34 AM, Krishna Kurapati PSSNV wrote: > > > > > > On 4/14/2023 9:15 PM, Andrew Halaney wrote: > > > On Wed, Apr 05, 2023 at 06:27:57PM +0530, Krishna Kurapati wrote: > > > > Add USB and DWC3 node for tertiary port of SC8280 along with multiport > > > > IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride > > > > platforms. > > > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx> > > > > --- > > > > Link to v5: https://lore.kernel.org/all/20230310163420.7582-7-quic_kriskura@xxxxxxxxxxx/ > > > > > > > > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 58 ++++++++++++++++++++++++++ > > > > 1 file changed, 58 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > > > b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > > > index 42bfa9fa5b96..7b81f2b0449d 100644 > > > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > > > @@ -3108,6 +3108,64 @@ usb_1_role_switch: endpoint { > > > > }; > > > > }; > > > > + usb_2: usb@a4f8800 { > > > > + compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3"; > > > > + reg = <0 0x0a4f8800 0 0x400>; > > > > + #address-cells = <2>; > > > > + #size-cells = <2>; > > > > + ranges; > > > > + > > > > + clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>, > > > > + <&gcc GCC_USB30_MP_MASTER_CLK>, > > > > + <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>, > > > > + <&gcc GCC_USB30_MP_SLEEP_CLK>, > > > > + <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, > > > > + <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>, > > > > + <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>, > > > > + <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>, > > > > + <&gcc GCC_SYS_NOC_USB_AXI_CLK>; > > > > + clock-names = "cfg_noc", "core", "iface", "sleep", > > > > "mock_utmi", > > > > + "noc_aggr", "noc_aggr_north", > > > > "noc_aggr_south", "noc_sys"; > > > > + > > > > + assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, > > > > + <&gcc GCC_USB30_MP_MASTER_CLK>; > > > > + assigned-clock-rates = <19200000>, <200000000>; > > > > + > > > > + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, > > > > + <&pdc 126 IRQ_TYPE_EDGE_RISING>, > > > > + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>; > > > > + > > > > + interrupt-names = "dp_hs_phy_irq", "dm_hs_phy_irq", > > > > + "ss_phy_irq"; > > > > + > > > > > > This is breaking the current schema (with the full series applied), > > > I am not sure if a pwr_event IRQ exists or but it maybe necessary to > > > modify qcom,dwc3.yaml in order to explain hardware if it doesn't exist: > > > > > > (dtschema) ahalaney@halaney-x13s ~/git/linux-next > > > (git)-[718f2024524f] % make CHECK_DTBS=y > > > DT_SCHEMA_FILES=/usb/qcom,dwc3.yaml qcom/sa8540p-ride.dtb > > > :( > > > LINT Documentation/devicetree/bindings > > > CHKDT Documentation/devicetree/bindings/processed-schema.json > > > SCHEMA Documentation/devicetree/bindings/processed-schema.json > > > DTC_CHK arch/arm64/boot/dts/qcom/sa8540p-ride.dtb > > > /home/ahalaney/git/linux-next/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: usb@a4f8800: interrupt-names:0: 'pwr_event' was expected > > > From schema: /home/ahalaney/git/linux-next/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > > > /home/ahalaney/git/linux-next/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: usb@a4f8800: interrupt-names:1: 'dp_hs_phy_irq' was expected > > > From schema: /home/ahalaney/git/linux-next/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > > > /home/ahalaney/git/linux-next/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: usb@a4f8800: interrupt-names:2: 'dm_hs_phy_irq' was expected > > > From schema: /home/ahalaney/git/linux-next/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > > > /home/ahalaney/git/linux-next/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: usb@a4f8800: interrupt-names: ['dp_hs_phy_irq', 'dm_hs_phy_irq', 'ss_phy_irq'] is too short > > > From schema: /home/ahalaney/git/linux-next/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > > > /home/ahalaney/git/linux-next/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: usb@a4f8800: interrupts-extended: [[99, 127, 1], [99, 126, 1], [99, 16, 4]] is too short > > > From schema: /home/ahalaney/git/linux-next/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > > > make CHECK_DTBS=y DT_SCHEMA_FILES=/usb/qcom,dwc3.yaml > > > qcom/sa8540p-ride.dtb 22.61s user 0.54s system 99% cpu 23.172 total > > > (dtschema) ahalaney@halaney-x13s ~/git/linux-next (git)-[718f2024524f] % > > > > > > Thanks, > > > Andrew > > > > > > > Hi Andrew, > > > > Thanks for pointing it out. Let me check and get back on the > > pwr_event_irq. > > > > Probably I might have missed it 😅. If so, will make sure to add it in > > next version. > > > > Regards, > > Krishna, > > > Hi Andrew, Johan, > > I was looking at the pwr_event_irq interrupts for Multiport controller and > see that there are two of them as per HW specs. All targets till date have > only 1 pwr_event_irq required. > > The reason I thought I missed pwr_event_irq in my patches is because in > downstream this is a required IRQ for all targets, so I was under assumption > that we need it for upstream targets as well. But upstream qcom driver > doesn't have support for this IRQ yet. And this has been made a required one > only for SC8280 [1]/[2]. > > Probably we can proceed in one of the following ways: > 1. Remove pwr_event_irq in both bindings and DT as driver support is not > present currently. > 2. Update the bindings for SC8280 to include an optional secondary > pwr_event_irq for multiport controller. > > I would prefer option-1 as removing them would be better because they are > not being used. Please let me know your thoughts on this. > > [1]: > https://lore.kernel.org/all/20220713131340.29401-2-johan+linaro@xxxxxxxxxx/ > [2]: > https://lore.kernel.org/all/20220713131340.29401-6-johan+linaro@xxxxxxxxxx/ > Personally, I prefer option 2 since the IRQ does exist technically (although it isn't currently used), I like it being described... it makes the dt-binding a more complete description of the hardware. I am unsure of the rules wrt dt-bindings and usage in drivers, but I always like to view it as "this is a description of the hardware", and the driver bit is just nice to have to ensure that whoever is adding the binding is actually describing things sufficiently. You could probably add a new compatible string for the multiport sc8280xp IP as well, and make the second IRQ non-optional in dt-binding evaluation? That seems more inline with reality, the regular IP has 1 pwr_event_irq, multiport on this platform has 2, they behave slightly differently and thus they deserve their own string to match on despite being on the same platform. Please note, I'm just a contributor -- I would not be surprised to find out that my view is not aligned with what maintainers here think, but that's my interpretation of things! Hope that helps, Andrew
