| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Apr 2023 15:33:28 -0500
From: Andrew Halaney <ahalaney@...hat.com>
To: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
Cc: Johan Hovold <johan+linaro@...nel.org>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
quic_pkondeti@...cinc.com, quic_ppratap@...cinc.com,
quic_wcheng@...cinc.com, quic_jackp@...cinc.com,
quic_harshq@...cinc.com, quic_shazhuss@...cinc.com
Subject: Re: [PATCH v6 6/8] arm64: dts: qcom: sc8280xp: Add multiport
controller node for SC8280
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@...cinc.com>
> > > > ---
> > > > Link to v5: https://lore.kernel.org/all/20230310163420.7582-7-quic_kriskura@quicinc.com/
> > > >
> > > > 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@...8800 {
> > > > + 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@...aney-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@...8800: 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@...8800: 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@...8800: 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@...8800: 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@...8800: 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@...aney-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@kernel.org/
> [2]:
> https://lore.kernel.org/all/20220713131340.29401-6-johan+linaro@kernel.org/
>
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
Powered by blists - more mailing lists