Message ID | 20231007154806.605-7-quic_kriskura@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a888:0:b0:403:3b70:6f57 with SMTP id x8csp992282vqo; Sat, 7 Oct 2023 08:49:43 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEii9U70T8RUMaLb4xp1S6AEhxj9RRsCWTCOixvOIErP+mX7j6/DfYFuqiDkPlfbssLgRU0 X-Received: by 2002:a05:6a20:2445:b0:14e:3ba7:2933 with SMTP id t5-20020a056a20244500b0014e3ba72933mr12031364pzc.54.1696693783636; Sat, 07 Oct 2023 08:49:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696693783; cv=none; d=google.com; s=arc-20160816; b=iaBQ5uPravvmooBMjEpr0IKjPiRJsRWZW82r/KD9GDZuNP+R1Rlxc/hXUy9/C6DBlC yho1wAX5a4xDCWNl+5tjlGvQJn3FRJR+tWarLNVvPODvW/CAwlnlLauMh2DdU6kF6f0Z Vhee4VGG1tVDwDYbl7C20pC8pEmnEVFHdiJ25n7QI5jovf3V3vCaGqcyBddUGP5n5vDh TEcpMPgSc0I1DQh6XLEEgazO5Ex4RQzmEyip8TVennE8zWjnjm0dbKtp9o3c+mwptPSv 60P1OEajhNNbTBpare4wC905bF0z3QaEEA1AJ0eONxiy2qeY0T5VL5HfoxHC1Dae32hB 5S8Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=J4NP7JDmyt+wbcbtOopTBz9Qmtzk9G5xhxbI3dCwcv4=; fh=a++TWPJw3Cn9MYBczZLXAB+Eob7lKtyrQCXvx5EPkG8=; b=LBOcTUFzVJ/lya2Fd1OJwyu91SD2UcU9VZkIbIcHzNjxBZC4CDQroX7YYg0RUYQlXi E8vqIRujKE/NC6KJLQcbLCvmdPdVuO9Gq/dq0i3gQG+cbxFtUHP4qvScx6mYmS+A61I8 RgXZu07wV+5ZBpx63WAmxI570XvSWxHAub0Ih7EmxbT0YZsT7+K2GlXLzx0Di144an8x 06ut3QQcXT/kkRz6xNBkq5ehwrBn7bLRSObRXjX9ZSvPOFKWLWiULoN8+GYHlzdKqHpZ 1Wa3tNVWomebxj3m3MdQnqSfVemKAdoo+8vn+zPoYVJSLzIBA1cQDjkkh22mVeHBmrxX /8JA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=bq6dyV9G; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from pete.vger.email (pete.vger.email. [2620:137:e000::3:6]) by mx.google.com with ESMTPS id r29-20020a638f5d000000b0058d551255c0si1169325pgn.79.2023.10.07.08.49.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 07 Oct 2023 08:49:43 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=bq6dyV9G; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 6A4EA8027B5A; Sat, 7 Oct 2023 08:49:36 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343997AbjJGPtT (ORCPT <rfc822;pusanteemu@gmail.com> + 17 others); Sat, 7 Oct 2023 11:49:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45218 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344062AbjJGPtR (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 7 Oct 2023 11:49:17 -0400 Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2C1CEBC; Sat, 7 Oct 2023 08:49:16 -0700 (PDT) Received: from pps.filterd (m0279871.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 397FX0in019228; Sat, 7 Oct 2023 15:49:08 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : to : cc : subject : date : message-id : in-reply-to : references : mime-version : content-transfer-encoding : content-type; s=qcppdkim1; bh=J4NP7JDmyt+wbcbtOopTBz9Qmtzk9G5xhxbI3dCwcv4=; b=bq6dyV9GOmh5M+TrQhd3cxlTsvGTIF7EXTKDLNFlKN6bHt6bG58/6JrUS1j2lWIJkaMy 7g1CfuOPRSM8BrVsJnAIWmSxbpmDGJORXWiLP7ghTa5IkbN/pjNpHzVFZdRg45krczme dclSoWhnEFdONLLSkOSeGkHulsunPuKQMKkqNTGLVdeErvzuhS3lQ/0NLKcyDYjd95+V Pz7ahyAK68V46p8zEqQv45zk1wFwQ59aJIFjlor91kZQ8zNQeogpV3/grk2kYqI84Hlx IiNBk/bPkALtKEj/3k0vT9OCBNtNWY5zmHraGsLBbQFRf+aRvcL8fa0QaRZ0jxZDeW51 HQ== Received: from nalasppmta03.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3tjxx88yfk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sat, 07 Oct 2023 15:49:07 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA03.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 397Fn6tW028797 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sat, 7 Oct 2023 15:49:06 GMT Received: from hu-kriskura-hyd.qualcomm.com (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.36; Sat, 7 Oct 2023 08:49:00 -0700 From: Krishna Kurapati <quic_kriskura@quicinc.com> To: 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>, Wesley Cheng <quic_wcheng@quicinc.com>, Johan Hovold <johan@kernel.org> CC: <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_jackp@quicinc.com>, <ahalaney@redhat.com>, <quic_shazhuss@quicinc.com>, Krishna Kurapati <quic_kriskura@quicinc.com> Subject: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport Date: Sat, 7 Oct 2023 21:18:02 +0530 Message-ID: <20231007154806.605-7-quic_kriskura@quicinc.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <20231007154806.605-1-quic_kriskura@quicinc.com> References: <20231007154806.605-1-quic_kriskura@quicinc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: JdczK7afancdIcBpmVGkdTP1YJQiM0tp X-Proofpoint-ORIG-GUID: JdczK7afancdIcBpmVGkdTP1YJQiM0tp X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-07_12,2023-10-06_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 bulkscore=0 impostorscore=0 suspectscore=0 phishscore=0 priorityscore=1501 mlxscore=0 spamscore=0 mlxlogscore=779 adultscore=0 lowpriorityscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000 definitions=main-2310070143 X-Spam-Status: No, score=2.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Sat, 07 Oct 2023 08:49:36 -0700 (PDT) X-Spam-Level: ** X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779112381261639706 X-GMAIL-MSGID: 1779112381261639706 |
Series |
Add multiport support for DWC3 controllers
|
|
Commit Message
Krishna Kurapati
Oct. 7, 2023, 3:48 p.m. UTC
Currently wakeup is supported by only single port controllers. Read speed
of each port and accordingly enable IRQ's for those ports.
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
drivers/usb/dwc3/dwc3-qcom.c | 65 +++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 30 deletions(-)
Comments
On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote: > Currently wakeup is supported by only single port controllers. Read speed > of each port and accordingly enable IRQ's for those ports. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 65 +++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 30 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 863892284146..651b9775a0c2 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -90,7 +90,7 @@ struct dwc3_qcom { > */ > int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS]; > int hs_phy_irq; > - enum usb_device_speed usb2_speed; > + enum usb_device_speed usb2_speed[DWC3_MAX_PORTS]; This also belongs in a new port structure. > struct extcon_dev *edev; > struct extcon_dev *host_edev; > @@ -335,7 +335,8 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) > return dwc->xhci; > } > > -static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > +static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, > + int port_index) No need for line break (since it's a function definition). > { > struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > struct usb_device *udev; > @@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > > /* > * It is possible to query the speed of all children of > - * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code > - * currently supports only 1 port per controller. So > - * this is sufficient. > + * USB2.0 root hub via usb_hub_for_each_child(). This comment no longer makes sense with your current implementation. But perhaps this should be done using usb_hub_for_each_child() instead as that may be more efficient. Then you use this function to read out the speed for all the ports in go (and store it in the port structures I mentioned). Please determine which alternative is best. > */ > #ifdef CONFIG_USB > - udev = usb_hub_find_child(hcd->self.root_hub, 1); > + udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1); > #else > udev = NULL; > #endif > @@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq) > > static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) > { > + int i; > + > dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq); > > - if (qcom->usb2_speed == USB_SPEED_LOW) { > - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); > - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || > - (qcom->usb2_speed == USB_SPEED_FULL)) { > - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); > - } else { > - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); > - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); > - } > + for (i = 0; i < qcom->num_ports; i++) { > + if (qcom->usb2_speed[i] == USB_SPEED_LOW) { > + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]); > + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) || > + (qcom->usb2_speed[i] == USB_SPEED_FULL)) { > + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]); > + } else { > + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]); > + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]); > + } > > - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]); > + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]); > + } > } The above is hardly readable, partly because of the 2d array that I think you should drop, and partly because you add the port loop here instead of in the caller. A lot of these functions should become port operation where you either pass in a port structure directly or possibly a port index as I've mentioned before. [ I realise that the confusion around hs_phy_irq may be partly to blame for this but since that one is also a per-port interrupt, that's no longer an issue. ] > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > @@ -454,10 +461,8 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > * The role is stable during suspend as role switching is done from a > * freezable workqueue. > */ > - if (dwc3_qcom_is_host(qcom) && wakeup) { > - qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); So just let this function update the usb2 speed for all ports unless there are reasons not to. > + if (dwc3_qcom_is_host(qcom) && wakeup) > dwc3_qcom_enable_interrupts(qcom); And then iterate over the ports and enable the interrupts here as you did above for the pwr_evnt_irqs. > - } > > qcom->is_suspended = true; Johan
On 10/23/2023 9:17 PM, Johan Hovold wrote: > On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote: >> Currently wakeup is supported by only single port controllers. Read speed >> of each port and accordingly enable IRQ's for those ports. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 65 +++++++++++++++++++----------------- >> 1 file changed, 35 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 863892284146..651b9775a0c2 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -90,7 +90,7 @@ struct dwc3_qcom { >> */ >> int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS]; >> int hs_phy_irq; >> - enum usb_device_speed usb2_speed; >> + enum usb_device_speed usb2_speed[DWC3_MAX_PORTS]; > > This also belongs in a new port structure. > >> struct extcon_dev *edev; >> struct extcon_dev *host_edev; >> @@ -335,7 +335,8 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) >> return dwc->xhci; >> } >> >> -static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) >> +static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, >> + int port_index) > > No need for line break (since it's a function definition). > >> { >> struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); >> struct usb_device *udev; >> @@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) >> >> /* >> * It is possible to query the speed of all children of >> - * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code >> - * currently supports only 1 port per controller. So >> - * this is sufficient. >> + * USB2.0 root hub via usb_hub_for_each_child(). > > This comment no longer makes sense with your current implementation. > Can you help elaborate on your comment ? Do you mean that this API doesn't get speed on all ports, but this has to be called in a loop to get all the port speeds ? In that sense, I agree, I can change the comments here. > But perhaps this should be done using usb_hub_for_each_child() instead > as that may be more efficient. Then you use this function to read out > the speed for all the ports in go (and store it in the port structures I > mentioned). Please determine which alternative is best. > Either ways is fine. We would have qcom->num_ports to determine how many speeds we can read. >> */ >> #ifdef CONFIG_USB >> - udev = usb_hub_find_child(hcd->self.root_hub, 1); >> + udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1); >> #else >> udev = NULL; >> #endif >> @@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq) >> >> static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) >> { >> + int i; >> + >> dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq); >> >> - if (qcom->usb2_speed == USB_SPEED_LOW) { >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); >> - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || >> - (qcom->usb2_speed == USB_SPEED_FULL)) { >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); >> - } else { >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); >> - } >> + for (i = 0; i < qcom->num_ports; i++) { >> + if (qcom->usb2_speed[i] == USB_SPEED_LOW) { >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]); >> + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) || >> + (qcom->usb2_speed[i] == USB_SPEED_FULL)) { >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]); >> + } else { >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]); >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]); >> + } >> >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]); >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]); >> + } >> } > > The above is hardly readable, partly because of the 2d array that I > think you should drop, and partly because you add the port loop here > instead of in the caller. > > A lot of these functions should become port operation where you either > pass in a port structure directly or possibly a port index as I've > mentioned before. With your suggestion, yes, this can be refactored to be readable. > > [ I realise that the confusion around hs_phy_irq may be partly to blame > for this but since that one is also a per-port interrupt, that's no > longer an issue. ] I don't want to add support for this right away [1]. I would like to keep hs_phy_irq outside the loop for now. > >> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> @@ -454,10 +461,8 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> * The role is stable during suspend as role switching is done from a >> * freezable workqueue. >> */ >> - if (dwc3_qcom_is_host(qcom) && wakeup) { >> - qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); > > So just let this function update the usb2 speed for all ports unless > there are reasons not to. Either way is fine by me as mentioned above. Will udapte code accordingly. > >> + if (dwc3_qcom_is_host(qcom) && wakeup) >> dwc3_qcom_enable_interrupts(qcom); > > And then iterate over the ports and enable the interrupts here as you > did above for the pwr_evnt_irqs. > >> - } >> >> qcom->is_suspended = true; [1]: https://lore.kernel.org/all/fb5e5e1d-520c-4cbc-adde-f30e853421a1@quicinc.com/ Regards, Krishna,
On Mon, Oct 23, 2023 at 10:57:04PM +0530, Krishna Kurapati PSSNV wrote: > On 10/23/2023 9:17 PM, Johan Hovold wrote: > > On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote: > >> Currently wakeup is supported by only single port controllers. Read speed > >> of each port and accordingly enable IRQ's for those ports. > >> > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > >> --- > >> -static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > >> +static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, > >> + int port_index) > > > > No need for line break (since it's a function definition). > > > >> { > >> struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > >> struct usb_device *udev; > >> @@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > >> > >> /* > >> * It is possible to query the speed of all children of > >> - * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code > >> - * currently supports only 1 port per controller. So > >> - * this is sufficient. > >> + * USB2.0 root hub via usb_hub_for_each_child(). > > > > This comment no longer makes sense with your current implementation. > > > Can you help elaborate on your comment ? Do you mean that this API > doesn't get speed on all ports, but this has to be called in a loop to > get all the port speeds ? In that sense, I agree, I can change the > comments here. It does not make sense to keep only half the comment after your update as it is a suggestion for how one could go about and generalise this for multiport, which is what you are now doing. > > But perhaps this should be done using usb_hub_for_each_child() instead > > as that may be more efficient. Then you use this function to read out > > the speed for all the ports in go (and store it in the port structures I > > mentioned). Please determine which alternative is best. > > > Either ways is fine. We would have qcom->num_ports to determine how many > speeds we can read. That's not the point. I'm referring to which alternative is less computationally expensive and allows for a clean implementation. Please do try to figure it out yourself. > >> */ > >> #ifdef CONFIG_USB > >> - udev = usb_hub_find_child(hcd->self.root_hub, 1); > >> + udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1); > >> #else > >> udev = NULL; > >> #endif > >> @@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq) > >> > >> static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) > >> { > >> + int i; > >> + > >> dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq); > >> > >> - if (qcom->usb2_speed == USB_SPEED_LOW) { > >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); > >> - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || > >> - (qcom->usb2_speed == USB_SPEED_FULL)) { > >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); > >> - } else { > >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); > >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); > >> - } > >> + for (i = 0; i < qcom->num_ports; i++) { > >> + if (qcom->usb2_speed[i] == USB_SPEED_LOW) { > >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]); > >> + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) || > >> + (qcom->usb2_speed[i] == USB_SPEED_FULL)) { > >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]); > >> + } else { > >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]); > >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]); > >> + } > >> > >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]); > >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]); > >> + } > >> } > > > > The above is hardly readable, partly because of the 2d array that I > > think you should drop, and partly because you add the port loop here > > instead of in the caller. > > > > A lot of these functions should become port operation where you either > > pass in a port structure directly or possibly a port index as I've > > mentioned before. > > With your suggestion, yes, this can be refactored to be readable. > > > > > [ I realise that the confusion around hs_phy_irq may be partly to blame > > for this but since that one is also a per-port interrupt, that's no > > longer an issue. ] > > I don't want to add support for this right away [1]. I would like to > keep hs_phy_irq outside the loop for now. No. Stop trying to take shortcuts. Again, this is upstream, not Qualcomm's vendor kernel. Johan
On 10/24/2023 12:40 PM, Johan Hovold wrote: >>> >>> This comment no longer makes sense with your current implementation. >>> >> Can you help elaborate on your comment ? Do you mean that this API >> doesn't get speed on all ports, but this has to be called in a loop to >> get all the port speeds ? In that sense, I agree, I can change the >> comments here. > > It does not make sense to keep only half the comment after your update > as it is a suggestion for how one could go about and generalise this for > multiport, which is what you are now doing. > Thanks for explanation. Will update the comments. >>> But perhaps this should be done using usb_hub_for_each_child() instead >>> as that may be more efficient. Then you use this function to read out >>> the speed for all the ports in go (and store it in the port structures I >>> mentioned). Please determine which alternative is best. >>> >> Either ways is fine. We would have qcom->num_ports to determine how many >> speeds we can read. > > That's not the point. I'm referring to which alternative is less > computationally expensive and allows for a clean implementation. > > Please do try to figure it out yourself. > I don't think its much of a difference: while (loop over num_ports) { read_usb2_speed() } read_usb2_speed() { while (loop over num_ports) { hub api to read speed. } } The second one would avoid calling read_usb2_speed multiple times. Will take that path. >>> >>> [ I realise that the confusion around hs_phy_irq may be partly to blame >>> for this but since that one is also a per-port interrupt, that's no >>> longer an issue. ] >> >> I don't want to add support for this right away [1]. I would like to >> keep hs_phy_irq outside the loop for now. > > No. Stop trying to take shortcuts. Again, this is upstream, not > Qualcomm's vendor kernel. > I don't think it is a shortcut. The reason I said I would keep it out of loop is I know why we need DP/DM/SS IRQ's during wakeup. The wakeup signals come in as rising/falling edges in high speed on DP/DM lines and LFPS terminations come on SS lines. So we need these 3 interrupts for sure in wakeup context. hs_phy_irq is not mandatory for wakeup. Any particular reason why it is needed to add driver support for hs_phy_irq's of multiport now ? May be I am missing something. If there is any reason why we need to add it now, I would try to learn and see if it has any side effects (like generating spurious wakeup's) and if nothing, I would add it back to port structure. Regards, Krishna,
On Tue, Oct 24, 2023 at 02:11:31PM +0530, Krishna Kurapati PSSNV wrote: > On 10/24/2023 12:40 PM, Johan Hovold wrote: > >>> But perhaps this should be done using usb_hub_for_each_child() instead > >>> as that may be more efficient. Then you use this function to read out > >>> the speed for all the ports in go (and store it in the port structures I > >>> mentioned). Please determine which alternative is best. > >>> > >> Either ways is fine. We would have qcom->num_ports to determine how many > >> speeds we can read. > > > > That's not the point. I'm referring to which alternative is less > > computationally expensive and allows for a clean implementation. > > > > Please do try to figure it out yourself. > > > I don't think its much of a difference: > > while (loop over num_ports) { > read_usb2_speed() > } > > read_usb2_speed() { > while (loop over num_ports) { > hub api to read speed. > } > } > > The second one would avoid calling read_usb2_speed multiple times. Will > take that path. You need to look at the implementation of usb_hub_for_each_child() and usb_hub_find_child() to determine that, which you now forced me to do; and yes, you're right, this shouldn't matter from an efficiency standpoint. > >>> [ I realise that the confusion around hs_phy_irq may be partly to blame > >>> for this but since that one is also a per-port interrupt, that's no > >>> longer an issue. ] > >> > >> I don't want to add support for this right away [1]. I would like to > >> keep hs_phy_irq outside the loop for now. > > > > No. Stop trying to take shortcuts. Again, this is upstream, not > > Qualcomm's vendor kernel. > > I don't think it is a shortcut. > > The reason I said I would keep it out of loop is I know why we need > DP/DM/SS IRQ's during wakeup. The wakeup signals come in as > rising/falling edges in high speed on DP/DM lines and LFPS terminations > come on SS lines. It is a shortcut as this interrupt is per-port and some SoC's already use it. So you're making a mess of the implementation for no good reason. > So we need these 3 interrupts for sure in wakeup context. > hs_phy_irq is not mandatory for wakeup. Any particular reason why it is > needed to add driver support for hs_phy_irq's of multiport now ? May be > I am missing something. If there is any reason why we need to add it > now, I would try to learn and see if it has any side effects (like > generating spurious wakeup's) and if nothing, I would add it back to > port structure. As I've mentioned a few times now, the hs_phy_irq is already used by a few SoC's so you can't just pretend it doesn't exist and mess up the implementation for no good reason. Just find out how it is used and why only some Qualcomm SoC's use it currently. It appears to be used in parallel with the DP/DM interrupts, and it has been there from the start: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver") Sure, the wakeup implementation was incomplete and broken for a long time, but I'm not going to let you continue this practise of pushing incomplete hacks upstream which someone else will eventually be forced to clean up. You have the documentation, just use it. Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 863892284146..651b9775a0c2 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -90,7 +90,7 @@ struct dwc3_qcom { */ int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS]; int hs_phy_irq; - enum usb_device_speed usb2_speed; + enum usb_device_speed usb2_speed[DWC3_MAX_PORTS]; struct extcon_dev *edev; struct extcon_dev *host_edev; @@ -335,7 +335,8 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) return dwc->xhci; } -static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) +static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, + int port_index) { struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); struct usb_device *udev; @@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) /* * It is possible to query the speed of all children of - * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code - * currently supports only 1 port per controller. So - * this is sufficient. + * USB2.0 root hub via usb_hub_for_each_child(). */ #ifdef CONFIG_USB - udev = usb_hub_find_child(hcd->self.root_hub, 1); + udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1); #else udev = NULL; #endif @@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq) static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) { + int i; + dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq); - if (qcom->usb2_speed == USB_SPEED_LOW) { - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || - (qcom->usb2_speed == USB_SPEED_FULL)) { - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); - } else { - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); - } + for (i = 0; i < qcom->num_ports; i++) { + if (qcom->usb2_speed[i] == USB_SPEED_LOW) { + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]); + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) || + (qcom->usb2_speed[i] == USB_SPEED_FULL)) { + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]); + } else { + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]); + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]); + } - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]); + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]); + } } static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) { + int i; + dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq, 0); /* @@ -413,22 +418,24 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) * disconnect and remote wakeup. When no device is connected, configure both * DP and DM lines as rising edge to detect HS/HS/LS device connect scenario. */ - - if (qcom->usb2_speed == USB_SPEED_LOW) { - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0], + for (i = 0; i < qcom->num_ports; i++) { + qcom->usb2_speed[i] = dwc3_qcom_read_usb2_speed(qcom, i); + if (qcom->usb2_speed[i] == USB_SPEED_LOW) { + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i], IRQ_TYPE_EDGE_FALLING); - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || - (qcom->usb2_speed == USB_SPEED_FULL)) { - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0], + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) || + (qcom->usb2_speed[i] == USB_SPEED_FULL)) { + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i], IRQ_TYPE_EDGE_FALLING); - } else { - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0], + } else { + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i], IRQ_TYPE_EDGE_RISING); - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0], + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i], IRQ_TYPE_EDGE_RISING); - } + } - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0], 0); + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i], 0); + } } static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) @@ -454,10 +461,8 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) * The role is stable during suspend as role switching is done from a * freezable workqueue. */ - if (dwc3_qcom_is_host(qcom) && wakeup) { - qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); + if (dwc3_qcom_is_host(qcom) && wakeup) dwc3_qcom_enable_interrupts(qcom); - } qcom->is_suspended = true;