Message ID | 20230621043628.21485-5-quic_kriskura@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp4126603vqr; Tue, 20 Jun 2023 22:03:20 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5SAM3SIWoLhVCRRfWIqgXnm+EnQrA+uq36tqfEC7Umf4RDzObAopaf2J7D/dGLoqTOHFEV X-Received: by 2002:a05:6214:764:b0:5f7:a9e1:bbbf with SMTP id f4-20020a056214076400b005f7a9e1bbbfmr6186212qvz.44.1687323800165; Tue, 20 Jun 2023 22:03:20 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1687323800; cv=none; d=google.com; s=arc-20160816; b=WkQ5OGqhZ274P6uXOK6YbVZy2VgYGv6lzbGfmjeAQl/TO7xRf8iZKCeAOTUuot2oXO EofrJDelyvsWanULvEyhyhciecIGhONQZwFLzZm0b6QV+pngL2zBWKq3vnlznbmeRYx1 f52LKVjksCfXjePg8rALeBVUxMm2NEWbUqlzkBioe3tLrJN30hDND44FNxmpAXj05Und pq3QkjO/TNY2k6XtCtg7bnMmSrs58sDyGPc7/rcnZYFnNP7fmzEkW73p1hrHAKKPqG8Q LSvKqoB163iJRdcZDi8B99uXlkHya9J22Zu0I1eAY5VqiZ4JgK9V28H64wLmPRKTRzxB DzLA== 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=D6FIyIgMKwSgrq83FPPJUG1q91y6mUgAst2KQA5Z7VM=; b=UCCJJMnevXOSai1q/mp4nSSAm4l8T/k3OaDavUIui97dWpaYFsSY6GocOw2aezG3+V o0PcQhmz8WoACzBhCVMjDu53mLeW3uUT2dkKr9MH/LGnd0pmxXuJqeJ52zwKdcRL3q7v o+BaQtq977+IuGhI/x6Td/46ykjT5Hrhck8w80bMf2q6X5+zOZ70wGxMGi62qrkUfluL JNhzt+NU/LSHdhQvRkJHAe1/MzBERPEpexk6NoAzSHYmWIHG0mG+LyV0GKbA7POEK3Lv XH7Vn14sMIv+u/ywbO2m3SFHTLQW9SP1fwAx/3CIA41b+nDgAIBg6iAd1ZKIr3KtFDQ7 kW3w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=jNMnCB3H; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 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 (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i62-20020a638741000000b0054fdcd2edbfsi3029214pge.831.2023.06.20.22.03.08; Tue, 20 Jun 2023 22:03:20 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=jNMnCB3H; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229871AbjFUEhm (ORCPT <rfc822;maxin.john@gmail.com> + 99 others); Wed, 21 Jun 2023 00:37:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53366 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229791AbjFUEhf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 21 Jun 2023 00:37:35 -0400 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8C0B91994; Tue, 20 Jun 2023 21:37:22 -0700 (PDT) Received: from pps.filterd (m0279867.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 35L2oDUM024248; Wed, 21 Jun 2023 04:37:13 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=D6FIyIgMKwSgrq83FPPJUG1q91y6mUgAst2KQA5Z7VM=; b=jNMnCB3HbAKC6NSbSmBfidM4AtXcpAs0v4ijmoO5eu3ETJZ9BNeIF3zfiWD4CjHZ825r MjUU/IGy8psjGgXZ1Q7mOPgXgPgJhFwZCfqlBNId2GI3ypn1rkHHKGQ5H85fKbKtrU2+ NriKQuSujO9RR4NmrCSEzsA3zZE8c0ARbkX3aZj32VJCAV/GUCfyU0aPx9pBQQHq60jK Y8WQePG9lJKiCedGZtd7BA5CZ5wgmuljzHgSm+cHmtZU7RNJ24VnO/5GQDXKl1DlWGLE YYj/S9r6PDIJS2yEw+tfXL6DgRvK7ktZxvxAcML5oDwhawuRJX/SqjyUeoLdswlOkygo 3w== Received: from nalasppmta04.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3rb7sutmhh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 21 Jun 2023 04:37:12 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA04.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 35L4bCbv020763 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 21 Jun 2023 04:37:12 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.986.42; Tue, 20 Jun 2023 21:37:05 -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>, <quic_harshq@quicinc.com>, <ahalaney@redhat.com>, <quic_shazhuss@quicinc.com>, Krishna Kurapati <quic_kriskura@quicinc.com> Subject: [PATCH v9 04/10] usb: dwc3: core: Skip setting event buffers for host only controllers Date: Wed, 21 Jun 2023 10:06:22 +0530 Message-ID: <20230621043628.21485-5-quic_kriskura@quicinc.com> X-Mailer: git-send-email 2.40.0 In-Reply-To: <20230621043628.21485-1-quic_kriskura@quicinc.com> References: <20230621043628.21485-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: wTmodpteoBrzbx3iq7WoNAa9eLT0gEzc X-Proofpoint-ORIG-GUID: wTmodpteoBrzbx3iq7WoNAa9eLT0gEzc X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26 definitions=2023-06-21_03,2023-06-16_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 impostorscore=0 priorityscore=1501 bulkscore=0 phishscore=0 mlxlogscore=991 suspectscore=0 spamscore=0 mlxscore=0 adultscore=0 clxscore=1015 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2305260000 definitions=main-2306210039 X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1769287241184980738?= X-GMAIL-MSGID: =?utf-8?q?1769287241184980738?= |
Series |
Add multiport support for DWC3 controllers
|
|
Commit Message
Krishna Kurapati
June 21, 2023, 4:36 a.m. UTC
On some SoC's like SA8295P where the tertiary controller is host-only
capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible.
Trying to access them leads to a crash.
For DRD/Peripheral supported controllers, event buffer setup is done
again in gadget_pullup. Skip setup or cleanup of event buffers if
controller is host-only capable.
Suggested-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
drivers/usb/dwc3/core.c | 11 +++++++++++
1 file changed, 11 insertions(+)
Comments
On Wed, Jun 21, 2023, Krishna Kurapati wrote: > On some SoC's like SA8295P where the tertiary controller is host-only > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > Trying to access them leads to a crash. > > For DRD/Peripheral supported controllers, event buffer setup is done > again in gadget_pullup. Skip setup or cleanup of event buffers if > controller is host-only capable. > > Suggested-by: Johan Hovold <johan@kernel.org> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 32ec05fc242b..e1ebae54454f 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) > static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > { > struct dwc3_event_buffer *evt; > + unsigned int hw_mode; > + > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > + return 0; This is a little awkward. Returning 0 here indicates that this function was successful, and the event buffers were allocated based on the function name. Do this check outside of dwc3_alloc_one_event_buffer() and specifically set dwc->ev_buf = NULL if that's the case. Thanks, Thinh > > evt = dwc3_alloc_one_event_buffer(dwc, length); > if (IS_ERR(evt)) { > @@ -507,6 +512,9 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc) > { > struct dwc3_event_buffer *evt; > > + if (!dwc->ev_buf) > + return 0; > + > evt = dwc->ev_buf; > evt->lpos = 0; > dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(0), > @@ -524,6 +532,9 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc) > { > struct dwc3_event_buffer *evt; > > + if (!dwc->ev_buf) > + return; > + > evt = dwc->ev_buf; > > evt->lpos = 0; > -- > 2.40.0 >
On 6/24/2023 3:57 AM, Thinh Nguyen wrote: > On Wed, Jun 21, 2023, Krishna Kurapati wrote: >> On some SoC's like SA8295P where the tertiary controller is host-only >> capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. >> Trying to access them leads to a crash. >> >> For DRD/Peripheral supported controllers, event buffer setup is done >> again in gadget_pullup. Skip setup or cleanup of event buffers if >> controller is host-only capable. >> >> Suggested-by: Johan Hovold <johan@kernel.org> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 32ec05fc242b..e1ebae54454f 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) >> static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) >> { >> struct dwc3_event_buffer *evt; >> + unsigned int hw_mode; >> + >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) >> + return 0; > > This is a little awkward. Returning 0 here indicates that this function > was successful, and the event buffers were allocated based on the > function name. Do this check outside of dwc3_alloc_one_event_buffer() > and specifically set dwc->ev_buf = NULL if that's the case. > Hi Thinh, Apologies, I didn't understand the comment properly. I thought we were supposed to return 0 here if we fulfill the goal of this function (allocate if we are drd/gadget and don't allocate if we are host mode only). If we return a non zero error here, probe would fail as this call will be done only for host only controllers during probe and nowhere else. Are you suggesting we move this check to dwc3_alloc_one_event_buffer call ? Regards, Krishna, >> >> evt = dwc3_alloc_one_event_buffer(dwc, length); >> if (IS_ERR(evt)) { >> @@ -507,6 +512,9 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc) >> { >> struct dwc3_event_buffer *evt; >> >> + if (!dwc->ev_buf) >> + return 0; >> + >> evt = dwc->ev_buf; >> evt->lpos = 0; >> dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(0), >> @@ -524,6 +532,9 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc) >> { >> struct dwc3_event_buffer *evt; >> >> + if (!dwc->ev_buf) >> + return; >> + >> evt = dwc->ev_buf; >> >> evt->lpos = 0; >> -- >> 2.40.0
On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote: > > > On 6/24/2023 3:57 AM, Thinh Nguyen wrote: > > On Wed, Jun 21, 2023, Krishna Kurapati wrote: > > > On some SoC's like SA8295P where the tertiary controller is host-only > > > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > > > Trying to access them leads to a crash. > > > > > > For DRD/Peripheral supported controllers, event buffer setup is done > > > again in gadget_pullup. Skip setup or cleanup of event buffers if > > > controller is host-only capable. > > > > > > Suggested-by: Johan Hovold <johan@kernel.org> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > --- > > > drivers/usb/dwc3/core.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 32ec05fc242b..e1ebae54454f 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) > > > static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > > > { > > > struct dwc3_event_buffer *evt; > > > + unsigned int hw_mode; > > > + > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > > > + return 0; > > > > This is a little awkward. Returning 0 here indicates that this function > > was successful, and the event buffers were allocated based on the > > function name. Do this check outside of dwc3_alloc_one_event_buffer() > > and specifically set dwc->ev_buf = NULL if that's the case. > > > Hi Thinh, > > Apologies, I didn't understand the comment properly. > > I thought we were supposed to return 0 here if we fulfill the goal of this > function (allocate if we are drd/gadget and don't allocate if we are host > mode only). The name of the function implies that it returns 0 if it allocated the event buffer. If there are multiple conditions to the function returning 0 here, then we should document it. > > If we return a non zero error here, probe would fail as this call will be > done only for host only controllers during probe and nowhere else. > > Are you suggesting we move this check to dwc3_alloc_one_event_buffer call > ? > > Regards, > Krishna, > I'm suggesting to move the check to the caller of dwc3_alloc_one_event_buffer(). Something like this so that it's self-documented: diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 0beaab932e7d..bba82792bf6f 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev) struct resource *res, dwc_res; void __iomem *regs; struct dwc3 *dwc; + unsigned int hw_mode; int ret; dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev) pm_runtime_forbid(dev); - ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); - if (ret) { - dev_err(dwc->dev, "failed to allocate event buffers\n"); - ret = -ENOMEM; - goto err_allow_rpm; + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { + dwc->ev_buf = NULL; + } else { + ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); + if (ret) { + dev_err(dwc->dev, "failed to allocate event buffers\n"); + ret = -ENOMEM; + goto err_allow_rpm; + } } dwc->edev = dwc3_get_extcon(dwc); -- Thanks, Thinh
On Mon, Jun 26, 2023, Thinh Nguyen wrote: > On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote: > > > > > > On 6/24/2023 3:57 AM, Thinh Nguyen wrote: > > > On Wed, Jun 21, 2023, Krishna Kurapati wrote: > > > > On some SoC's like SA8295P where the tertiary controller is host-only > > > > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > > > > Trying to access them leads to a crash. > > > > > > > > For DRD/Peripheral supported controllers, event buffer setup is done > > > > again in gadget_pullup. Skip setup or cleanup of event buffers if > > > > controller is host-only capable. > > > > > > > > Suggested-by: Johan Hovold <johan@kernel.org> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > --- > > > > drivers/usb/dwc3/core.c | 11 +++++++++++ > > > > 1 file changed, 11 insertions(+) > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index 32ec05fc242b..e1ebae54454f 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) > > > > static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > > > > { > > > > struct dwc3_event_buffer *evt; > > > > + unsigned int hw_mode; > > > > + > > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > > > > + return 0; > > > > > > This is a little awkward. Returning 0 here indicates that this function > > > was successful, and the event buffers were allocated based on the > > > function name. Do this check outside of dwc3_alloc_one_event_buffer() > > > and specifically set dwc->ev_buf = NULL if that's the case. > > > > > Hi Thinh, > > > > Apologies, I didn't understand the comment properly. > > > > I thought we were supposed to return 0 here if we fulfill the goal of this > > function (allocate if we are drd/gadget and don't allocate if we are host > > mode only). > > The name of the function implies that it returns 0 if it allocated the > event buffer. If there are multiple conditions to the function returning > 0 here, then we should document it. > > > > > If we return a non zero error here, probe would fail as this call will be > > done only for host only controllers during probe and nowhere else. > > > > Are you suggesting we move this check to dwc3_alloc_one_event_buffer call > > ? > > > > Regards, > > Krishna, > > > > I'm suggesting to move the check to the caller of > dwc3_alloc_one_event_buffer(). Something like this so that it's > self-documented: > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 0beaab932e7d..bba82792bf6f 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev) > struct resource *res, dwc_res; > void __iomem *regs; > struct dwc3 *dwc; > + unsigned int hw_mode; > int ret; > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev) > > pm_runtime_forbid(dev); > > - ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); > - if (ret) { > - dev_err(dwc->dev, "failed to allocate event buffers\n"); > - ret = -ENOMEM; > - goto err_allow_rpm; > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > + dwc->ev_buf = NULL; > + } else { > + ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); > + if (ret) { > + dev_err(dwc->dev, "failed to allocate event buffers\n"); > + ret = -ENOMEM; > + goto err_allow_rpm; > + } > } > > dwc->edev = dwc3_get_extcon(dwc); > Actually, please ignore. there's already a document there, I missed that for some reason. What you did is fine. Though, I don't see the condition for ev_buf = NULL anywhere. Can you add that for clarity? Thanks, Thinh
On 6/27/2023 5:16 AM, Thinh Nguyen wrote: > On Mon, Jun 26, 2023, Thinh Nguyen wrote: >> On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote: >>> >>> >>> On 6/24/2023 3:57 AM, Thinh Nguyen wrote: >>>> On Wed, Jun 21, 2023, Krishna Kurapati wrote: >>>>> On some SoC's like SA8295P where the tertiary controller is host-only >>>>> capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. >>>>> Trying to access them leads to a crash. >>>>> >>>>> For DRD/Peripheral supported controllers, event buffer setup is done >>>>> again in gadget_pullup. Skip setup or cleanup of event buffers if >>>>> controller is host-only capable. >>>>> >>>>> Suggested-by: Johan Hovold <johan@kernel.org> >>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>>> --- >>>>> drivers/usb/dwc3/core.c | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>> index 32ec05fc242b..e1ebae54454f 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>>> @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) >>>>> static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) >>>>> { >>>>> struct dwc3_event_buffer *evt; >>>>> + unsigned int hw_mode; >>>>> + >>>>> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >>>>> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) >>>>> + return 0; >>>> >>>> This is a little awkward. Returning 0 here indicates that this function >>>> was successful, and the event buffers were allocated based on the >>>> function name. Do this check outside of dwc3_alloc_one_event_buffer() >>>> and specifically set dwc->ev_buf = NULL if that's the case. >>>> >>> Hi Thinh, >>> >>> Apologies, I didn't understand the comment properly. >>> >>> I thought we were supposed to return 0 here if we fulfill the goal of this >>> function (allocate if we are drd/gadget and don't allocate if we are host >>> mode only). >> >> The name of the function implies that it returns 0 if it allocated the >> event buffer. If there are multiple conditions to the function returning >> 0 here, then we should document it. >> >>> >>> If we return a non zero error here, probe would fail as this call will be >>> done only for host only controllers during probe and nowhere else. >>> >>> Are you suggesting we move this check to dwc3_alloc_one_event_buffer call >>> ? >>> >>> Regards, >>> Krishna, >>> >> >> I'm suggesting to move the check to the caller of >> dwc3_alloc_one_event_buffer(). Something like this so that it's >> self-documented: >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 0beaab932e7d..bba82792bf6f 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev) >> struct resource *res, dwc_res; >> void __iomem *regs; >> struct dwc3 *dwc; >> + unsigned int hw_mode; >> int ret; >> >> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); >> @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev) >> >> pm_runtime_forbid(dev); >> >> - ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); >> - if (ret) { >> - dev_err(dwc->dev, "failed to allocate event buffers\n"); >> - ret = -ENOMEM; >> - goto err_allow_rpm; >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >> + dwc->ev_buf = NULL; >> + } else { >> + ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); >> + if (ret) { >> + dev_err(dwc->dev, "failed to allocate event buffers\n"); >> + ret = -ENOMEM; >> + goto err_allow_rpm; >> + } >> } >> >> dwc->edev = dwc3_get_extcon(dwc); >> > > Actually, please ignore. there's already a document there, I missed that > for some reason. What you did is fine. Though, I don't see the condition > for ev_buf = NULL anywhere. Can you add that for clarity? > > Thanks, > Thinh Hi Thinh, Did you mean adding "dwc->ev_buf = NULL" specifically ? Regards, Krishna,
On Mon, Jul 03, 2023, Krishna Kurapati PSSNV wrote: > > > On 6/27/2023 5:16 AM, Thinh Nguyen wrote: > > On Mon, Jun 26, 2023, Thinh Nguyen wrote: > > > On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote: > > > > > > > > > > > > On 6/24/2023 3:57 AM, Thinh Nguyen wrote: > > > > > On Wed, Jun 21, 2023, Krishna Kurapati wrote: > > > > > > On some SoC's like SA8295P where the tertiary controller is host-only > > > > > > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > > > > > > Trying to access them leads to a crash. > > > > > > > > > > > > For DRD/Peripheral supported controllers, event buffer setup is done > > > > > > again in gadget_pullup. Skip setup or cleanup of event buffers if > > > > > > controller is host-only capable. > > > > > > > > > > > > Suggested-by: Johan Hovold <johan@kernel.org> > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > > > --- > > > > > > drivers/usb/dwc3/core.c | 11 +++++++++++ > > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > > index 32ec05fc242b..e1ebae54454f 100644 > > > > > > --- a/drivers/usb/dwc3/core.c > > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > > @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) > > > > > > static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > > > > > > { > > > > > > struct dwc3_event_buffer *evt; > > > > > > + unsigned int hw_mode; > > > > > > + > > > > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > > > > > > + return 0; > > > > > > > > > > This is a little awkward. Returning 0 here indicates that this function > > > > > was successful, and the event buffers were allocated based on the > > > > > function name. Do this check outside of dwc3_alloc_one_event_buffer() > > > > > and specifically set dwc->ev_buf = NULL if that's the case. > > > > > > > > > Hi Thinh, > > > > > > > > Apologies, I didn't understand the comment properly. > > > > > > > > I thought we were supposed to return 0 here if we fulfill the goal of this > > > > function (allocate if we are drd/gadget and don't allocate if we are host > > > > mode only). > > > > > > The name of the function implies that it returns 0 if it allocated the > > > event buffer. If there are multiple conditions to the function returning > > > 0 here, then we should document it. > > > > > > > > > > > If we return a non zero error here, probe would fail as this call will be > > > > done only for host only controllers during probe and nowhere else. > > > > > > > > Are you suggesting we move this check to dwc3_alloc_one_event_buffer call > > > > ? > > > > > > > > Regards, > > > > Krishna, > > > > > > > > > > I'm suggesting to move the check to the caller of > > > dwc3_alloc_one_event_buffer(). Something like this so that it's > > > self-documented: > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 0beaab932e7d..bba82792bf6f 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev) > > > struct resource *res, dwc_res; > > > void __iomem *regs; > > > struct dwc3 *dwc; > > > + unsigned int hw_mode; > > > int ret; > > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > > > @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev) > > > pm_runtime_forbid(dev); > > > - ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); > > > - if (ret) { > > > - dev_err(dwc->dev, "failed to allocate event buffers\n"); > > > - ret = -ENOMEM; > > > - goto err_allow_rpm; > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > > > + dwc->ev_buf = NULL; > > > + } else { > > > + ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); > > > + if (ret) { > > > + dev_err(dwc->dev, "failed to allocate event buffers\n"); > > > + ret = -ENOMEM; > > > + goto err_allow_rpm; > > > + } > > > } > > > dwc->edev = dwc3_get_extcon(dwc); > > > > > > > Actually, please ignore. there's already a document there, I missed that > > for some reason. What you did is fine. Though, I don't see the condition > > for ev_buf = NULL anywhere. Can you add that for clarity? > > > > Thanks, > > Thinh > > Hi Thinh, > > Did you mean adding "dwc->ev_buf = NULL" specifically ? > Yes, please initialize dwc->ev_buf to NULL somewhere if it's host mode. Thanks, Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 32ec05fc242b..e1ebae54454f 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) { struct dwc3_event_buffer *evt; + unsigned int hw_mode; + + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) + return 0; evt = dwc3_alloc_one_event_buffer(dwc, length); if (IS_ERR(evt)) { @@ -507,6 +512,9 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc) { struct dwc3_event_buffer *evt; + if (!dwc->ev_buf) + return 0; + evt = dwc->ev_buf; evt->lpos = 0; dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(0), @@ -524,6 +532,9 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc) { struct dwc3_event_buffer *evt; + if (!dwc->ev_buf) + return; + evt = dwc->ev_buf; evt->lpos = 0;