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 E2D47C433F5 for ; Fri, 14 Jan 2022 00:50:08 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 2A054830EA; Fri, 14 Jan 2022 01:50:06 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.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=gmail.com header.i=@gmail.com header.b="oIsarOrj"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id DF24F80031; Fri, 14 Jan 2022 01:50:03 +0100 (CET) Received: from mail-ed1-x535.google.com (mail-ed1-x535.google.com [IPv6:2a00:1450:4864:20::535]) (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 0946D832CD for ; Fri, 14 Jan 2022 01:49:59 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=aford173@gmail.com Received: by mail-ed1-x535.google.com with SMTP id m4so29210561edb.10 for ; Thu, 13 Jan 2022 16:49:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=gaeCgLnLGmLAK1Qc4QpZ+INBzdYuylT6QAdXe1loYjk=; b=oIsarOrjLTBqglpaHV9uC9M001HtePz8XTx/onfyR2tMP1X3nuxJYbMmUNke4/tukX vjFfdUE2QOP3RKDL+6fnuyu+bSrJ44/M/1UnpNNjan0d2m1MVspn3ScO6CZ2FeN01HvC eWoT+0FrX3f59Z1FmQS3Me59jUDymIqzW8GJR61w4FSqK/dUpgpe5YEohmsvyttQ3s5C mSlrUM73tfuWIG2/ZGLeK+WMfmWAW0YGkbyELg0KWRbII+LLgCOG4IeVtBoxnUKsfjUC HrtLMsQGGHGDG9cruZprHXGL98oKELmdsEOBpLngF8d2lUNt1n+H2e+nk3Hx79ErAtkz 0e0Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gaeCgLnLGmLAK1Qc4QpZ+INBzdYuylT6QAdXe1loYjk=; b=kWH+wbmK0pwozY3zDmF0czuuS48Krx1QHUaimsHjRvdjMwa7kNqW20sl3uc4cHB0Ve MHiPtXGBLXFjCSTAOJtZFDZcQq0QoXHxcEhT2rwUVWhrs9IQ9uzOE+gbVRvprZk4VVEq i3ZD1sORWN7uobRfxSM4bwOu0xgZKYlfLKQZc5h9RqdvyP/Q9a+zvoqT20ZHWgVjkp3J vM7s7T6S919CXnRZzdG7328WCEGy4hnUW+FU6VVPG9OhXi5gAEHtYscE54kGjbxjUvWD KHLZWkEkw8obxZuqHpmJMq3uveneSoFNZ0qktER/P+Qi3El6GMux1NEyMUa6d1NDKnXC 2SHw== X-Gm-Message-State: AOAM531zY5TM74QJTpD45yrJvfPL/YoOhtZUNIKuzmMGduZo0p4faX1H G78AJ+E97vny5U7TrWU0ngRR0J9Thq7i2vX/Mrg= X-Google-Smtp-Source: ABdhPJzvpy0FE2PjSh1/rnX1p81SG9iRMjousYsGhW3xGpnqmnfO8BU6htqSvOucfdpK8ikTafQ8SZbZU2DfyYG8YFw= X-Received: by 2002:a17:907:3f1b:: with SMTP id hq27mr5307471ejc.613.1642121398382; Thu, 13 Jan 2022 16:49:58 -0800 (PST) MIME-Version: 1.0 References: <20211223140756.304527-1-aford173@gmail.com> <20220104083148.1328623-1-michael@walle.cc> <6f8b8bccf3eb19742d386acaa9bf59e3@walle.cc> In-Reply-To: From: Adam Ford Date: Thu, 13 Jan 2022 18:49:47 -0600 Message-ID: Subject: Re: [PATCH V2] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn To: Michael Walle Cc: Fabio Estevam , Marek Vasut , Simon Glass , U-Boot Mailing List , Tim Harvey Content-Type: text/plain; charset="UTF-8" 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.2 at phobos.denx.de X-Virus-Status: Clean On Thu, Jan 13, 2022 at 6:35 PM Adam Ford wrote: > > On Wed, Jan 12, 2022 at 2:17 AM Michael Walle wrote: > > > > Am 2022-01-11 17:52, schrieb Adam Ford: > > > On Tue, Jan 11, 2022 at 9:31 AM Michael Walle wrote: > > >> > > >> Hi, > > >> > > >> Am 2022-01-11 15:20, schrieb Adam Ford: > > >> > On Tue, Jan 4, 2022 at 2:32 AM Michael Walle wrote: > > >> >> > The imx8mm and imx8mn appear compatible with imx7d-usb > > >> >> > flags in the OTG driver. If the dr_mode is defined as > > >> >> > host or peripheral, the device appears to operate correctly, > > >> >> > however the auto host/peripheral detection results in an error. > > >> >> > > > >> >> > The solution isn't just adding checks for imx8mm and imx8mn to > > >> >> > the check for imx7, because the USB clock needs to be running > > >> >> > to read from the USBNC_PHY_STATUS_OFFSET register or it will hang. > > >> >> > > > >> >> > The init_type in both priv and plat data are the same, so it doesn't > > >> >> > make sense to configure the data in the plat data and copy the > > >> >> > data to priv when priv can be configured directly. Instead, rename > > >> >> > ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the > > >> >> > probe functions after the clocks are enabled, but before the data > > >> >> > is required. > > >> >> > > > >> >> > With that added, the additional checks for imx8mm and imx8mn will > > >> >> > allow reading the register to automatically determine the state > > >> >> > (host or device) of the OTG controller. > > >> >> > > > >> >> > Signed-off-by: Adam Ford > > >> >> > --- > > >> >> > V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it > > >> >> > from the probe after the clocks are enabled, but before > > >> >> > the data is needed. > > >> >> > > > >> >> > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c > > >> >> > index 1bd6147c76..f2a34b7f06 100644 > > >> >> > --- a/drivers/usb/host/ehci-mx6.c > > >> >> > +++ b/drivers/usb/host/ehci-mx6.c > > >> >> > > >> >> .. > > >> >> > > >> >> > @@ -639,10 +639,8 @@ static int mx6_parse_dt_addrs(struct udevice *dev) > > >> >> > > > >> >> > static int ehci_usb_probe(struct udevice *dev) > > >> >> > { > > >> >> > - struct usb_plat *plat = dev_get_plat(dev); > > >> >> > struct usb_ehci *ehci = dev_read_addr_ptr(dev); > > >> >> > struct ehci_mx6_priv_data *priv = dev_get_priv(dev); > > >> >> > - enum usb_init_type type = plat->init_type; > > >> >> > struct ehci_hccr *hccr; > > >> >> > struct ehci_hcor *hcor; > > >> >> > int ret; > > >> >> > @@ -660,8 +658,6 @@ static int ehci_usb_probe(struct udevice *dev) > > >> >> > return ret; > > >> >> > > > >> >> > priv->ehci = ehci; > > >> >> > - priv->init_type = type; > > >> >> > > >> > > > >> > Michael, > > >> > > > >> >> I'm not sure this is correct. There is also this: > > >> >> https://elixir.bootlin.com/u-boot/v2022.01-rc4/source/drivers/usb/host/usb-uclass.c#L407 > > >> >> > > >> > > > >> > Further down in the code, you should see: > > >> > priv->phy_type = usb_get_phy_mode(dev_ofnode(dev)); > > >> > > >> But that is just fetching the mode from the device tree, the > > >> usb_setup_ehci_gadget() referenced above, change the mode during > > >> runtime by writing the plat->init_type and reprobing the device. > > >> > > >> >> Which won't work anymore. usb_setup_ehci_gadget() is called from > > >> >> usb_gadget_register_driver() in ci_udc.c. The latter is the one used > > >> >> on the imx, right? But I might be wrong. I'm still trying to figure > > >> >> out how this all works together, because I also try to get OTG > > >> >> running on the dwc3 driver. It looks like the ci_udc.c is special > > >> >> here, and I wonder how a transition to UCLASS_USB_GADGET_GENERIC > > >> >> might look like for this driver. > > >> > > > >> > This driver really isn't changing anything, it's just an order of > > >> > operations. > > >> > > >> It doesn't use the plat->init_type at all anymore, no? > > > > > > From what I could tell, the only thing that plat->init_type did was to > > > set priv->init_type. > > > The priv structure appears to do most of the work. > > > > but plat->init_type seems to change during runtime (by > > usb_setup_ehci_gadget()) and with this patch applied, it doesn't > > do that anymore. > > > > >> > Previously there was a separate that was being called to > > >> > determine the init_type as being either a peripheral or host. If it > > >> > wasn't explicitly set in the device tree, the helper function would > > >> > query a register, however, on the imx8mm and imx8mn platforms, the > > >> > clocks were not running which resulted in a hang. What I've done is > > >> > simply move the helper function into the probe and have it run after > > >> > the clocks start. In theory the register values will be the same as > > >> > they would be with the help function still in place. > > >> > > > >> > Both Tim Harvey and I have tested on an imx8mm and Fabio tested it on > > >> > an imx7. For me, the peripheral mode worked when the ID pin of the > > >> > USB-OTG was not grounded. When it was grounded, the system corrected > > >> > identified it as a host and I was able to mount a USB drive. > > >> > > >> Again, I'm missing something here, but the only user of > > >> usb_setup_ehci_gadget() is usb/gadget/ci_udc.c. > > > > > > I can't speak to what those functions do. What are you suggesting > > > that I do differently? > > > > I'd start by seeing if/when usb_setup_ehci_gadget() is called and > > see if plat->init_type changes. If its not called, is it dead code > > then? > > I commented out the call to usb_setup_ehci_gadget(), and the gadget > driver still enumerated. > I'll push a V2 to remove it along with some subsequent patches to > update the defconfig, so others who want to use/test it don't have to > guess what config options are necessary. > > Thanks for catching that. > Simon, i went go to remove usb_setup_ehci_gadget(), but the header file has a TODO with your name on it, but the ci_udc river appears to be the only function to call it, and from what I can tell, the CI-UDC driver doesn't need this. /** * usb_setup_ehci_gadget() - Set up a USB device as a gadget * * TODO(sjg@chromium.org): Tidy this up when USB gadgets can use driver model * * This provides a way to tell a controller to start up as a USB device * instead of as a host. It is untested. */ If I remove the function call from the ci_udc.c driver, do you want this left in the header? As it's written, the usb_setup_ehci_gadget() appears to attempt to override the plat->init_type, but this change is eliminating the need for the plat structure since it is pointless. >From what I can tell the function is trying to force a re-probe with the plat->init_type redefined as a peripheral, but the code I want to use auto-detects the host/peripheral mode, rendering this pointless. adam > adam > > > Sorry I just noticed this, while reviewing how this particular driver > > works, because I still like to find a working OTG driver in u-boot. > > I don't have any imx boards. > > > > -michael