From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AD683C54EBC for ; Tue, 10 Jan 2023 13:39:57 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9D80885413; Tue, 10 Jan 2023 14:39:55 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=baylibre-com.20210112.gappssmtp.com header.i=@baylibre-com.20210112.gappssmtp.com header.b="8BFb5GS7"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4CFE582131; Tue, 10 Jan 2023 14:39:54 +0100 (CET) Received: from mail-wm1-x32d.google.com (mail-wm1-x32d.google.com [IPv6:2a00:1450:4864:20::32d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 959E98544D for ; Tue, 10 Jan 2023 14:39:47 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=mkorpershoek@baylibre.com Received: by mail-wm1-x32d.google.com with SMTP id j16-20020a05600c1c1000b003d9ef8c274bso5585850wms.0 for ; Tue, 10 Jan 2023 05:39:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=MvoIuZIHEZG9cOF/G2zZxKiGS55KHcbjRFeQ3f1dEZA=; b=8BFb5GS7Yo0r2d5tSrcuZ9xFVPxUI8b0UMuh49PuGGq0LBQKPLy0xUcKsDipuHXNhz Ssk1s3M5pufP+2BJDJPsxSMNvkukzErHwwOqYwMBkhl5m7TLV73nQCt0oIVAtV06Hh7A JevkkH//b42HXJVTInQhieu/Zs9PqV70sRf0NFRcdJRVcRpg+dvYuVVqFwv3U4wIw+n3 XVeGgX88Z/WiTOs80mtoUiSdXTSO3iLoNdItvubNHEK099sPfTYRlbh/xr/tKcLamAXF IKOeTYfg/X2dVEDHTR98T87wbBr+oKu8cJaKjBRoClJg/zL/tC0RcVt+Twht1UiqAWIF 3Aeg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=MvoIuZIHEZG9cOF/G2zZxKiGS55KHcbjRFeQ3f1dEZA=; b=H/2CZkQ3r6Ahvr4zRGdSBdV51wIIZJs2ZCyHwvHUpNE7LiiYIlZguYYguq76FAi93g 1Dw9OOmMxQ+TD5nWylGaArRqGbtOHc0AUTj3rL1Hcv6LzmDXCzcbLvByS/XIbduRVwUC w8jrv93VmdJucgeVH+PpXhWLs2jyI/6p4YalX9Ble7H1Ni4llsHXl/2yJ37HIiOVOWYA lg0fqbXuu3QyR6qQQa2Sp7h8PCsRsNFQVzwvjGXS9m7us3mA0DQ5VUrVSpMuD6w7sluf DlTE3NHdNrA0nlhhMGQPySo14KfrH2EsYHmy5M6PU7ES7WfjZGuYnmfq6V/lPPMttlk3 SIEQ== X-Gm-Message-State: AFqh2krSHwltCnaetXapmGXC4zL5iEzYfzFD3l9tYFpRZSdUC2P0WJsA AnD77B6iwaA6mXofLd3JYIyTUQ== X-Google-Smtp-Source: AMrXdXvXHtEmzvXsCggcpwNUT09Pr7J6C+EL24tfgjfhUrxlDI0Jzj204DsLcLik+8XvZIT5a1Gy8w== X-Received: by 2002:a05:600c:1d89:b0:3d3:5cd6:781 with SMTP id p9-20020a05600c1d8900b003d35cd60781mr48723365wms.37.1673357986826; Tue, 10 Jan 2023 05:39:46 -0800 (PST) Received: from localhost ([2a01:cb19:85e6:1900:2bf7:7388:731d:c4e1]) by smtp.gmail.com with ESMTPSA id l27-20020a05600c2cdb00b003a84375d0d1sm21213417wmc.44.2023.01.10.05.39.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 10 Jan 2023 05:39:46 -0800 (PST) From: Mattijs Korpershoek To: Harald Seiler , Lukasz Majewski , Marek Vasut Cc: Dario Binacchi , Michael Trimarchi , u-boot@lists.denx.de Subject: Re: [PATCH] usb: gadget: dwc2_udc_otg: implement pullup() In-Reply-To: <45c496c31b29671a1f3d38c95362db0d238c70a6.camel@denx.de> References: <20230110-dwc2-pullup-v1-1-2bf9743e59e5@baylibre.com> <45c496c31b29671a1f3d38c95362db0d238c70a6.camel@denx.de> Date: Tue, 10 Jan 2023 14:39:45 +0100 Message-ID: <87eds28q5a.fsf@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi Harald, Thank you for your review. On Tue, Jan 10, 2023 at 14:30, Harald Seiler wrote: > Hi, > > On Tue, 2023-01-10 at 10:11 +0100, Mattijs Korpershoek wrote: >> Pullup is used by the usb framework in order to do software-controlled >> usb_gadget_connect() and usb_gadget_disconnect(). >> >> Implement pullup() for dwc2 using the SOFT_DISCONNECT bit in the dctl >> register. >> >> This is especially useful when a gadget disconnection is initiated but >> no board_usb_cleanup() is called. >> >> Signed-off-by: Mattijs Korpershoek >> --- >> On some boards using the dwc2 controller, like the Khadas VIM3L, whenever >> usb_gadget_release() is called, the D+ and D- lines are in an unknown state. >> >> Because of that, the host can't detect usb disconnection. >> >> It was attempted to be be fixed with [1] but ended up doing the gadget disconnection >> too early, creating issues on NXP-based boards which use uuu [2]. >> >> By implementing pullup() in the controller driver, we ensure that the disconnection will >> only be done when the framework calls usb_gadget_disconnect(). >> >> [1] https://lore.kernel.org/all/20220728-reset-usb-controller-v2-1-ef7657ce78b1@baylibre.com/ >> [2] https://lore.kernel.org/all/20230107164807.3597020-1-dario.binacchi@amarulasolutions.com/ >> --- >> drivers/usb/gadget/dwc2_udc_otg.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c >> index 77988f78ab30..93ed9712d18a 100644 >> --- a/drivers/usb/gadget/dwc2_udc_otg.c >> +++ b/drivers/usb/gadget/dwc2_udc_otg.c >> @@ -236,6 +236,21 @@ static int udc_enable(struct dwc2_udc *dev) >> return 0; >> } >> >> +static int dwc2_gadget_pullup(struct usb_gadget *g, int is_on) >> +{ >> + unsigned int uTemp; > > Just some minor points about style... > > I think the `u32` type should be used for 32-bit registers instead. > More explicit and no room for accidentally choosing the wrong size. > > Also, U-Boot and Linux don't use hungarian notation for variable names, > just call it `tmp` or even better `dctl` to be explicit. I agree with this. I actually picked up some of the code from reconfig_usbd() where hungarian nototation is used. That's also where the unsigned int comes from. Sorry I did not convert it to some more Linux/U-Boot oriented style before submitting. > >> + >> + is_on = !!is_on; > > This is superfluous, the if condition works either way. Ack. The maintainer (Marek) already picked this up in his usb tree: https://lore.kernel.org/all/1f2c5d74-faae-42f8-5d4d-dfc08cb093aa@denx.de/ Should I send a clean-up/follow-up patch for this? > >> + uTemp = readl(®->dctl); >> + if (is_on) >> + uTemp = uTemp & ~SOFT_DISCONNECT; >> + else >> + uTemp |= SOFT_DISCONNECT; >> + writel(uTemp, ®->dctl); >> + >> + return 0; >> +} >> + >> #if !CONFIG_IS_ENABLED(DM_USB_GADGET) >> /* >> Register entry point for the peripheral controller driver. >> @@ -805,6 +820,7 @@ static void dwc2_fifo_flush(struct usb_ep *_ep) >> } >> >> static const struct usb_gadget_ops dwc2_udc_ops = { >> + .pullup = dwc2_gadget_pullup, >> /* current versions must always be self-powered */ >> #if CONFIG_IS_ENABLED(DM_USB_GADGET) >> .udc_start = dwc2_gadget_start, >> >> --- >> base-commit: 7b84c973b96775576dcff228d865e8570be26c82 >> change-id: 20230110-dwc2-pullup-5b0f5a073d6b >> >> Best regards, > > Regards, > > Harald