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 61192C001DE for ; Fri, 4 Aug 2023 16:00:21 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 92B75866B9; Fri, 4 Aug 2023 18:00:19 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=bootlin.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=bootlin.com header.i=@bootlin.com header.b="Ac7EFVek"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id EC306866B9; Fri, 4 Aug 2023 18:00:17 +0200 (CEST) Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id BCCA5863E8 for ; Fri, 4 Aug 2023 18:00:14 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=miquel.raynal@bootlin.com Received: by mail.gandi.net (Postfix) with ESMTPSA id 908ACE0005; Fri, 4 Aug 2023 16:00:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1691164814; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=l7bwoijaMpeeshlICbacvA+A8A/wCQCPJQjajKCLP+I=; b=Ac7EFVekcO/P8APCj71+tc2Pyc3cOP+NojUG3x3m+TZMjgyeEQFDECzUMv7WWvo2VP1Kya GQPCQIRCYsL6hqDapVNVdqNjOGap67JSPh8Uz9j+MmkGayf7XdfC4NWZm3Iz+FM5CBMZwE 8NGHIW/EwB34ItBivP3XA/pRJ7yTyFZyZGugmhdwMXR4UqGug/RhNjwsmA9g8fm7leCRKf Uc30DMFvoRvmoYJK/AACcx8kEfK0+X056qhTYV114YT/g/iL0SKtvPHdmpqnLzELf+wyf4 U6gJW97S7fk4H16z1n378846bG52ovf8r/KYiWXtZhPIMw6+PzVdfoZoVydR8A== Date: Fri, 4 Aug 2023 18:00:12 +0200 From: Miquel Raynal To: Marek Vasut Cc: Tom Rini , u-boot@lists.denx.de, Kevin Hilman , Lukasz Majewski , Simon Glass Subject: Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter Message-ID: <20230804180012.79c4f9ec@xps-13> In-Reply-To: <0eb7f3fe-42da-6252-3b77-50eb3276f2ee@denx.de> References: <20230802124657.31184-1-marex@denx.de> <20230804090028.70aa780d@xps-13> <4833722f-18ab-e885-291a-988115fe39f8@denx.de> <20230804150100.GN3630934@bill-the-cat> <8902cf54-acba-cfb4-571b-09ba807e7a89@denx.de> <20230804171206.07faade4@xps-13> <0eb7f3fe-42da-6252-3b77-50eb3276f2ee@denx.de> Organization: Bootlin X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Sasl: miquel.raynal@bootlin.com 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.8 at phobos.denx.de X-Virus-Status: Clean Hi Marek, marex@denx.de wrote on Fri, 4 Aug 2023 17:40:07 +0200: > On 8/4/23 17:12, Miquel Raynal wrote: > > Hi, > >=20 > > marex@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200: > > =20 > >> On 8/4/23 17:01, Tom Rini wrote: =20 > >>> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote: =20 > >>>> On 8/4/23 09:00, Miquel Raynal wrote: =20 > >>>>> Hi Marek, > >>>>> > >>>>> marex@denx.de wrote on Wed, 2 Aug 2023 14:46:54 +0200: =20 > >>>>> >>>>>> Extend the driver core to perform lookup by both OF node a= nd driver =20 > >>>>>> bound to the node. Use this to look up specific device instances to > >>>>>> unbind from nodes in the unbind command. One example where this is > >>>>>> needed is USB peripheral controller, which may have multiple gadget > >>>>>> drivers bound to it. The unbind command has to select that specific > >>>>>> gadget driver instance to unbind from the controller, not unbind t= he > >>>>>> controller driver itself from the controller. > >>>>>> > >>>>>> USB ethernet gadget usage looks as follows with this change. Notice > >>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end. > >>>>>> " > >>>>>> bind /soc/usb-otg@49000000 usb_ether > >>>>>> setenv ethact usb_ether > >>>>>> setenv loadaddr 0xc2000000 > >>>>>> setenv ipaddr 10.0.0.2 > >>>>>> setenv serverip 10.0.0.1 > >>>>>> setenv netmask 255.255.255.0 > >>>>>> tftpboot 0xc2000000 10.0.0.1:test.file > >>>>>> unbind /soc/usb-otg@49000000 usb_ether =20 > >>>>> > >>>>> This extra parameter does not seem to work on the BBBW. I cannot se= lect > >>>>> the "usb_ether" driver like you do. > >>>>> > >>>>> Good news though, I am now able to use fastboot, but it is not > >>>>> straightforward: > >>>>> > >>>>> Here is my sequence right after the boot (reducing the dm tree outp= ut > >>>>> to the usb nodes for clarity): =20 > >>>>> >>>>> =3D> dm tree =20 > >>>>> misc 0 [ + ] ti-musb-wrapper | |-- usb@4740= 0000 > >>>>> usb 0 [ + ] ti-musb-peripheral | | |-- usb@= 47401000 > >>>>> ethernet 1 [ + ] usb_ether | | | `-- = usb_ether > >>>>> bootdev 3 [ ] eth_bootdev | | | = `-- usb_ether.bootdev > >>>>> usb 0 [ ] ti-musb-host | | `-- usb@= 47401800 =20 > >>>>> =3D> fastboot usb 0 =20 > >>>>> couldn't find an available UDC > >>>>> g_dnl_register: failed!, error: -19 =20 > >>>> > >>>> That is expected and not a bug, since the beagle explicitly binds USB > >>>> ethernet to MUSB gadget in board file, which is legacy deprecated wa= y. =20 > >>> > >>> So, we should do away with, probably all of arch_misc_init() in > >>> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case. =20 > >> > >> Yes > >> =20 > >>>>> exit not allowed from main input shell. =20 > >>>>> =3D> unbind /ocp/usb@47400000/usb@47401000 usb_ether =20 > >>>> > >>>> Does =20 > >>>> >>>> =3D> unbind ethernet 0 =20 > >>>> > >>>> work ? > >>>> > >>>> If so, 1/4 in this series can be skipped altogether. > >>>> > >>>> You likely won't even need the rebinding of ti-musb-peripheral anymo= re. =20 >=20 > Did you test this yet ? Not yet, I'll send you the feedback once done. > >>>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000 =20 > >>>>> =3D> unbind /ocp/usb@47400000/usb@47401000 > >>>>> =3D> dm tree =20 > >>>>> misc 0 [ + ] ti-musb-wrapper | |-- usb@4740= 0000 > >>>>> usb 0 [ ] ti-musb-host | | `-- usb@= 47401800 =20 > >>>>> =3D> fastboot usb 0 > >>>>> =3D> bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral > >>>>> =3D> dm tree =20 > >>>>> misc 0 [ + ] ti-musb-wrapper | |-- usb@4740= 0000 > >>>>> usb 0 [ ] ti-musb-host | | |-- usb@= 47401800 > >>>>> usb 0 [ ] ti-musb-peripheral | | `-- usb@= 47401000 =20 > >>>>> =3D> fastboot usb 0 =20 > >>>>> musb-hdrc: peripheral reset irq lost! > >>>>> # works! (the irq-related line above as always been there) > >>>>> > >>>>> So now, how do we make this process easy/understandable? =20 > >>>> > >>>> What would be your proposal ? =20 > >=20 > > At least I would appreciate: > > - to select CMD_BIND "by default" when relevant > > - to make the fastboot error more readable for the regular user =20 >=20 > Since with this 'unbind ethernet 0' this is orthogonal to this series, se= nd separate patches, thanks. This is not orthogonal, I am sorry. version X: - tftp works "out of the box" - fastboot works "out of the box" version X+1: - tftp works "out of the box" - fastboot returns an obscure error 1/ If we now *need* the bind/unbind commands, the series must take care of it. 2/ Without proper error message you just break fastboot for most regular users (basically everyone but few U-Boot devs). > > If you want to get rid of all the legacy code, I am not opposed, > > really, but that must not be the user who is responsible for > > understanding what changed in the "core". It must be very easily > > accessible to understand that now: > > - manual binding/unbinding is needed > > - which driver must be used when =20 >=20 > When my spare time permits. I understand. But I strongly disagree with this approach. We want to make U-Boot better. I am fine with all internal changes you wish, even if it breaks the CLI at some point because it is more accurate and drops a ton of legacy code. Again, this is okay as long as the CLI tells the user what changed in the behavior. So when I run tftp/dhcp/ping/whatever, if you don't want to automatically bind the ethernet gadget I'm okay, but just tell the user "Please make sure the Ethernet gadget is bound, eg: unbind xxx; bind yyy" (possibly giving automatic shortcuts to avoid the pain of finding the path of the controller). And when fastboot fails, same idea. Could you please consider enhancing these parts as well? > >>> Well, what's needed / is it possible to get to the point where we don= 't > >>> _need_ to call bind/unbind for each of these cases? Is there something > >>> we're supposed to be setting in the DT that we aren't? =20 > >> > >> You do need to unbind the ethernet before using fastboot, always, with= the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, = so it should behave like before. =20 > >=20 > > And for my own understanding, why don't we need to bind a fastboot > > gadget? =20 >=20 > The fastboot command does that internally . This is not visible with dm tree and I did not find how to bind the fastboot gadget manually. This makes the CLI clearly uneven and the internal code badly different between gadgets/commands. Why can't we have them both autoloaded/unloaded like before? I think I missed something which explains the rationale behind this series. Thanks, Miqu=C3=A8l