Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 May 2023 12:54:02 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Cc:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Felipe Balbi <balbi@kernel.org>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, quic_pkondeti@quicinc.com,
        quic_ppratap@quicinc.com, quic_wcheng@quicinc.com,
        quic_jackp@quicinc.com, quic_harshq@quicinc.com,
        ahalaney@redhat.com
Subject: Re: [PATCH v8 7/9] arm64: dts: qcom: sc8280xp: Add multiport
 controller node for SC8280
Message-ID: <ZGNgyo8ZRcC26W5l@hovoldconsulting.com>
References: <20230514054917.21318-1-quic_kriskura@quicinc.com>
 <20230514054917.21318-8-quic_kriskura@quicinc.com>
 <ZGJBLUsPcbsxj989@hovoldconsulting.com>
 <d14567fd-0576-55bb-40c0-442e060c28ba@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d14567fd-0576-55bb-40c0-442e060c28ba@quicinc.com>
Precedence: bulk

On Mon, May 15, 2023 at 09:02:13PM +0530, Krishna Kurapati PSSNV wrote:
> On 5/15/2023 7:56 PM, Johan Hovold wrote:
> > On Sun, May 14, 2023 at 11:19:15AM +0530, Krishna Kurapati wrote:

> >> @@ -3133,6 +3133,72 @@ usb_1_role_switch: endpoint {
> >>   			};
> >>   		};
> >>   
> >> +		usb_2: usb@a4f8800 {
> > 
> > As I believe someone already pointed out, this node is not in sort order
> > (i.e. it should go before usb@a6f8800).

>    I missed that message, but since I named it usb_2, so I placed it in 
> order after usb_1. Hope that is fine !!

No, the nodes should be sorted by unit address so you need to move it.

> >> +			interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
> >> +					      <&pdc 126 IRQ_TYPE_EDGE_RISING>,
> >> +					      <&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
> >> +					      <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
> >> +					      <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
> >> +					      <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
> >> +					      <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>;
> >> +
> >> +			interrupt-names = "dp_hs_phy_irq",
> >> +					  "dm_hs_phy_irq",
> >> +					  "ss_phy_irq",
> >> +					  "pwr_event_1",
> >> +					  "pwr_event_2",
> >> +					  "pwr_event_3",
> >> +					  "pwr_event_4";

> >> +			interconnect-names = "usb-ddr", "apps-usb";
> > 
> > Looks like 'wakeup-source' is missing here too.
> > 
> 
> I believe this property was added to enable wakeup from system suspend 
> in host mode. I didn't add this property as currently I don't need to 
> support wakeup. If any requirement comes in future, then I might need to 
> add dp/dm interrupts (if any) for other ports as well and then need to 
> change driver code to enable/disable them on suspend/resume.

If there are dp/dm/ss interrupts per ports then those need to be defined
in the binding and devicetree from the start.

Similar for 'wakeup-source' which indicates that the controller *can* be
used to wakeup the system from suspend (which those pdc interrupts
indicates).

Remember that the devicetree is supposed to describe the hardware, and
which features are currently supported in some version of software is
mostly irrelevant.

Johan