[PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help

[PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help

[Miquel Raynal]

While it may sound totally obvious for the regular U-Boot developer to
get the parameters of the bind/unbind commands from the output of 'dm
tree', it did not felt straightforward to me until I was explicitly
told to look there. And even when I knew the command, I did not make a
direct link between the arguments of this command and the columns
returned by 'dm tree'.

Several of us lost a lot of time because of that, I would like to kindly
help other users by slightly improving this textual line. Unfortunately,
because of how this string is used (like within the 'help' command) I
cannot detail much more, but at least the pointer is there.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
Resend: no change.

Changes in v2:
* Moved the details in the long text section as suggested by Heinrich.
* Rephrased the help text as well, not fully following Heinrich
  suggestion, but taking inspiration from it.
---
 cmd/bind.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cmd/bind.c b/cmd/bind.c
index 4d1b7885e60..be0d4d2a711 100644
--- a/cmd/bind.c
+++ b/cmd/bind.c
@@ -246,6 +246,8 @@ U_BOOT_CMD(
 	"Bind a device to a driver",
 	"<node path> <driver>\n"
 	"bind <class> <index> <driver>\n"
+	"Use 'dm tree' to list all devices registered in the driver model,\n"
+	"their path, class, index and current driver.\n"
 );
 
 U_BOOT_CMD(
@@ -254,4 +256,6 @@ U_BOOT_CMD(
 	"<node path>\n"
 	"unbind <class> <index>\n"
 	"unbind <class> <index> <driver>\n"
+	"Use 'dm tree' to list all devices registered in the driver model,\n"
+	"their path, class, index and current driver.\n"
 );
-- 
2.34.1

[PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Miquel Raynal]

At some point when trying to use USB gadgets, two situations may arise
and lead to a failure. Either the UDC (USB Device Controller) is not
available at all (not described or not probed) or the UDC is already in
use. For instance, as the USB Ethernet gadget remains bound to the UDC,
the use of any other USB gadget (fastboot, dfu, etc) *after* will always
fail with the "couldn't find an available UDC" error.

Let's give a more helpful message by making a difference between the two
cases. Let's also hint people who would get this error and grep it into
the sources a better explanation of what's wrong with their workflow.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
While doing this I really wanted to add "much more" error comments but I
faced another reality: often the messages are there but use
pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
I consider this unnecessary, as decreasing the loglevel will make these
messages appear. I would have expected errors to be displayed, but I
understand it makes the binaries even bigger.

Resend: no change.

Changes in v2:
* s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
  not sound well at all when there is only one UDC.
---
 drivers/usb/gadget/udc/udc-core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index 7f73926cb3e..8405b03462e 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -323,6 +323,7 @@ err1:
 int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
 {
 	struct usb_udc		*udc = NULL;
+	unsigned int		udc_count = 0;
 	int			ret;
 
 	if (!driver || !driver->bind || !driver->setup)
@@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
 
 	mutex_lock(&udc_lock);
 	list_for_each_entry(udc, &udc_list, list) {
+		udc_count++;
+
 		/* For now we take the first one */
 		if (!udc->driver)
 			goto found;
 	}
 
-	printf("couldn't find an available UDC\n");
+	if (!udc_count)
+		printf("No UDC available in the system\n");
+	else
+		/* When this happens, users should 'unbind <class> <index>'
+		 * using the output of 'dm tree' and looking at the line right
+		 * after the USB peripheral/device controller.
+		 */
+		printf("All UDCs in use (%d available), use the unbind command\n",
+		       udc_count);
 	mutex_unlock(&udc_lock);
 	return -ENODEV;
 found:
-- 
2.34.1

Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Massimo Pegorer]

Hi Miquel,

Il giorno lun 2 ott 2023 alle ore 15:46 Miquel Raynal
<miquel.raynal@bootlin.com> ha scritto:
>
> At some point when trying to use USB gadgets, two situations may arise

[...cut...]

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> While doing this I really wanted to add "much more" error comments but I
> faced another reality: often the messages are there but use
> pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
> I consider this unnecessary, as decreasing the loglevel will make these
> messages appear. I would have expected errors to be displayed, but I
> understand it makes the binaries even bigger.

This is how it works for pr_err but not for log_err: if you are not
using CONFIG_LOG, all log_xxx with level less than or equal to INFO
are in the binary. On the other hand, if you have CONFIG_LOG=y,
log_xxx are left out based on LOG_MAX_LEVEL value, while pr_xxx are
left out based on both LOGLEVEL and LOG_MAX_LEVEL too (due to the fact
that pr_xxx relies on log_xxx). This is quite confusing IMO. I'm
working on a proposal for a simpler and clearer unified way to log in
U-Boot.

Regards,
Massimo

> Resend: no change.
>

[...cut...]

Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Miquel Raynal]

Hi Massimo,

massimo.pegorer+oss@gmail.com wrote on Mon, 2 Oct 2023 16:37:10 +0200:

> Hi Miquel,
> 
> Il giorno lun 2 ott 2023 alle ore 15:46 Miquel Raynal
> <miquel.raynal@bootlin.com> ha scritto:
> >
> > At some point when trying to use USB gadgets, two situations may arise  
> 
> [...cut...]
> 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > While doing this I really wanted to add "much more" error comments but I
> > faced another reality: often the messages are there but use
> > pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
> > I consider this unnecessary, as decreasing the loglevel will make these
> > messages appear. I would have expected errors to be displayed, but I
> > understand it makes the binaries even bigger.  
> 
> This is how it works for pr_err but not for log_err: if you are not
> using CONFIG_LOG, all log_xxx with level less than or equal to INFO
> are in the binary. On the other hand, if you have CONFIG_LOG=y,
> log_xxx are left out based on LOG_MAX_LEVEL value, while pr_xxx are
> left out based on both LOGLEVEL and LOG_MAX_LEVEL too (due to the fact
> that pr_xxx relies on log_xxx). This is quite confusing IMO. I'm
> working on a proposal for a simpler and clearer unified way to log in
> U-Boot.

Very interesting (and, as you said, way too confusing). I did not even
notice the difference, thanks for the explanation. Indeed, it would be
nice to simplify this a bit.

Thanks,
Miquèl

Re: [PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help

[Simon Glass]

On Mon, 2 Oct 2023 at 07:46, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> While it may sound totally obvious for the regular U-Boot developer to
> get the parameters of the bind/unbind commands from the output of 'dm
> tree', it did not felt straightforward to me until I was explicitly
> told to look there. And even when I knew the command, I did not make a
> direct link between the arguments of this command and the columns
> returned by 'dm tree'.
>
> Several of us lost a lot of time because of that, I would like to kindly
> help other users by slightly improving this textual line. Unfortunately,
> because of how this string is used (like within the 'help' command) I
> cannot detail much more, but at least the pointer is there.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> Resend: no change.
>
> Changes in v2:
> * Moved the details in the long text section as suggested by Heinrich.
> * Rephrased the help text as well, not fully following Heinrich
>   suggestion, but taking inspiration from it.
> ---
>  cmd/bind.c | 4 ++++
>  1 file changed, 4 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help

[Mattijs Korpershoek]

On lun., oct. 02, 2023 at 15:46, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> While it may sound totally obvious for the regular U-Boot developer to
> get the parameters of the bind/unbind commands from the output of 'dm
> tree', it did not felt straightforward to me until I was explicitly
> told to look there. And even when I knew the command, I did not make a
> direct link between the arguments of this command and the columns
> returned by 'dm tree'.
>
> Several of us lost a lot of time because of that, I would like to kindly
> help other users by slightly improving this textual line. Unfortunately,
> because of how this string is used (like within the 'help' command) I
> cannot detail much more, but at least the pointer is there.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
> Resend: no change.
>
> Changes in v2:
> * Moved the details in the long text section as suggested by Heinrich.
> * Rephrased the help text as well, not fully following Heinrich
>   suggestion, but taking inspiration from it.
> ---
>  cmd/bind.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/cmd/bind.c b/cmd/bind.c
> index 4d1b7885e60..be0d4d2a711 100644
> --- a/cmd/bind.c
> +++ b/cmd/bind.c
> @@ -246,6 +246,8 @@ U_BOOT_CMD(
>  	"Bind a device to a driver",
>  	"<node path> <driver>\n"
>  	"bind <class> <index> <driver>\n"
> +	"Use 'dm tree' to list all devices registered in the driver model,\n"
> +	"their path, class, index and current driver.\n"
>  );
>  
>  U_BOOT_CMD(
> @@ -254,4 +256,6 @@ U_BOOT_CMD(
>  	"<node path>\n"
>  	"unbind <class> <index>\n"
>  	"unbind <class> <index> <driver>\n"
> +	"Use 'dm tree' to list all devices registered in the driver model,\n"
> +	"their path, class, index and current driver.\n"
>  );
> -- 
> 2.34.1

Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Mattijs Korpershoek]

On lun., oct. 02, 2023 at 15:46, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> At some point when trying to use USB gadgets, two situations may arise
> and lead to a failure. Either the UDC (USB Device Controller) is not
> available at all (not described or not probed) or the UDC is already in
> use. For instance, as the USB Ethernet gadget remains bound to the UDC,
> the use of any other USB gadget (fastboot, dfu, etc) *after* will always
> fail with the "couldn't find an available UDC" error.
>
> Let's give a more helpful message by making a difference between the two
> cases. Let's also hint people who would get this error and grep it into
> the sources a better explanation of what's wrong with their workflow.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
> While doing this I really wanted to add "much more" error comments but I
> faced another reality: often the messages are there but use
> pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
> I consider this unnecessary, as decreasing the loglevel will make these
> messages appear. I would have expected errors to be displayed, but I
> understand it makes the binaries even bigger.
>
> Resend: no change.
>
> Changes in v2:
> * s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
>   not sound well at all when there is only one UDC.
> ---
>  drivers/usb/gadget/udc/udc-core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index 7f73926cb3e..8405b03462e 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -323,6 +323,7 @@ err1:
>  int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>  {
>  	struct usb_udc		*udc = NULL;
> +	unsigned int		udc_count = 0;
>  	int			ret;
>  
>  	if (!driver || !driver->bind || !driver->setup)
> @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>  
>  	mutex_lock(&udc_lock);
>  	list_for_each_entry(udc, &udc_list, list) {
> +		udc_count++;
> +
>  		/* For now we take the first one */
>  		if (!udc->driver)
>  			goto found;
>  	}
>  
> -	printf("couldn't find an available UDC\n");
> +	if (!udc_count)
> +		printf("No UDC available in the system\n");
> +	else
> +		/* When this happens, users should 'unbind <class> <index>'
> +		 * using the output of 'dm tree' and looking at the line right
> +		 * after the USB peripheral/device controller.
> +		 */
> +		printf("All UDCs in use (%d available), use the unbind command\n",
> +		       udc_count);
>  	mutex_unlock(&udc_lock);
>  	return -ENODEV;
>  found:
> -- 
> 2.34.1

Re: [PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help

[Marek Vasut]

On 10/2/23 15:46, Miquel Raynal wrote:
> While it may sound totally obvious for the regular U-Boot developer to
> get the parameters of the bind/unbind commands from the output of 'dm
> tree', it did not felt straightforward to me until I was explicitly
> told to look there. And even when I knew the command, I did not make a
> direct link between the arguments of this command and the columns
> returned by 'dm tree'.
> 
> Several of us lost a lot of time because of that, I would like to kindly
> help other users by slightly improving this textual line. Unfortunately,
> because of how this string is used (like within the 'help' command) I
> cannot detail much more, but at least the pointer is there.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> Resend: no change.
> 
> Changes in v2:
> * Moved the details in the long text section as suggested by Heinrich.
> * Rephrased the help text as well, not fully following Heinrich
>    suggestion, but taking inspiration from it.
> ---
>   cmd/bind.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/cmd/bind.c b/cmd/bind.c
> index 4d1b7885e60..be0d4d2a711 100644
> --- a/cmd/bind.c
> +++ b/cmd/bind.c
> @@ -246,6 +246,8 @@ U_BOOT_CMD(
>   	"Bind a device to a driver",
>   	"<node path> <driver>\n"
>   	"bind <class> <index> <driver>\n"
> +	"Use 'dm tree' to list all devices registered in the driver model,\n"
> +	"their path, class, index and current driver.\n"
>   );
>   
>   U_BOOT_CMD(
> @@ -254,4 +256,6 @@ U_BOOT_CMD(
>   	"<node path>\n"
>   	"unbind <class> <index>\n"
>   	"unbind <class> <index> <driver>\n"
> +	"Use 'dm tree' to list all devices registered in the driver model,\n"
> +	"their path, class, index and current driver.\n"

'dm tree' depends on CMD_DM , you might need some if (IS_ENABLED()) here.

Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Marek Vasut]

On 10/2/23 15:46, Miquel Raynal wrote:
> At some point when trying to use USB gadgets, two situations may arise
> and lead to a failure. Either the UDC (USB Device Controller) is not
> available at all (not described or not probed) or the UDC is already in
> use. For instance, as the USB Ethernet gadget remains bound to the UDC,
> the use of any other USB gadget (fastboot, dfu, etc) *after* will always
> fail with the "couldn't find an available UDC" error.
> 
> Let's give a more helpful message by making a difference between the two
> cases. Let's also hint people who would get this error and grep it into
> the sources a better explanation of what's wrong with their workflow.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> While doing this I really wanted to add "much more" error comments but I
> faced another reality: often the messages are there but use
> pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
> I consider this unnecessary, as decreasing the loglevel will make these
> messages appear. I would have expected errors to be displayed, but I
> understand it makes the binaries even bigger.
> 
> Resend: no change.
> 
> Changes in v2:
> * s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
>    not sound well at all when there is only one UDC.
> ---
>   drivers/usb/gadget/udc/udc-core.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index 7f73926cb3e..8405b03462e 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -323,6 +323,7 @@ err1:
>   int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>   {
>   	struct usb_udc		*udc = NULL;
> +	unsigned int		udc_count = 0;
>   	int			ret;
>   
>   	if (!driver || !driver->bind || !driver->setup)
> @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>   
>   	mutex_lock(&udc_lock);
>   	list_for_each_entry(udc, &udc_list, list) {
> +		udc_count++;
> +
>   		/* For now we take the first one */
>   		if (!udc->driver)
>   			goto found;
>   	}
>   
> -	printf("couldn't find an available UDC\n");
> +	if (!udc_count)
> +		printf("No UDC available in the system\n");
> +	else
> +		/* When this happens, users should 'unbind <class> <index>'
> +		 * using the output of 'dm tree' and looking at the line right
> +		 * after the USB peripheral/device controller.
> +		 */
> +		printf("All UDCs in use (%d available), use the unbind command\n",
> +		       udc_count);

Users should really use 'bind' command in the first place, to avoid this 
guesswork to which UDC (on systems with multiple UDCs) a gadget gets 
bound to, and instead bind the gadget to specific UDC they want to bind 
it to. This code should then be removed entirely.

Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Miquel Raynal]

Hi Marek,

marex@denx.de wrote on Thu, 5 Oct 2023 15:04:25 +0200:

> On 10/2/23 15:46, Miquel Raynal wrote:
> > At some point when trying to use USB gadgets, two situations may arise
> > and lead to a failure. Either the UDC (USB Device Controller) is not
> > available at all (not described or not probed) or the UDC is already in
> > use. For instance, as the USB Ethernet gadget remains bound to the UDC,
> > the use of any other USB gadget (fastboot, dfu, etc) *after* will always
> > fail with the "couldn't find an available UDC" error.
> > 
> > Let's give a more helpful message by making a difference between the two
> > cases. Let's also hint people who would get this error and grep it into
> > the sources a better explanation of what's wrong with their workflow.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > While doing this I really wanted to add "much more" error comments but I
> > faced another reality: often the messages are there but use
> > pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
> > I consider this unnecessary, as decreasing the loglevel will make these
> > messages appear. I would have expected errors to be displayed, but I
> > understand it makes the binaries even bigger.
> > 
> > Resend: no change.
> > 
> > Changes in v2:
> > * s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
> >    not sound well at all when there is only one UDC.
> > ---
> >   drivers/usb/gadget/udc/udc-core.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> > index 7f73926cb3e..8405b03462e 100644
> > --- a/drivers/usb/gadget/udc/udc-core.c
> > +++ b/drivers/usb/gadget/udc/udc-core.c
> > @@ -323,6 +323,7 @@ err1:
> >   int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
> >   {
> >   	struct usb_udc		*udc = NULL;
> > +	unsigned int		udc_count = 0;
> >   	int			ret;  
> >   >   	if (!driver || !driver->bind || !driver->setup)  
> > @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)  
> >   >   	mutex_lock(&udc_lock);  
> >   	list_for_each_entry(udc, &udc_list, list) {
> > +		udc_count++;
> > +
> >   		/* For now we take the first one */
> >   		if (!udc->driver)
> >   			goto found;
> >   	}  
> >   > -	printf("couldn't find an available UDC\n");  
> > +	if (!udc_count)
> > +		printf("No UDC available in the system\n");
> > +	else
> > +		/* When this happens, users should 'unbind <class> <index>'
> > +		 * using the output of 'dm tree' and looking at the line right
> > +		 * after the USB peripheral/device controller.
> > +		 */
> > +		printf("All UDCs in use (%d available), use the unbind command\n",
> > +		       udc_count);  
> 
> Users should really use 'bind' command in the first place, to avoid this guesswork to which UDC (on systems with multiple UDCs) a gadget gets bound to, and instead bind the gadget to specific UDC they want to bind it to. This code should then be removed entirely.

There are two cases when this can happen:
* a single UDC (common) but a function already bound, there is no
  guessing here
* two (or more) UDC and there is some guessing

Before your cleanup, drivers would get automatically unbound from the
UDC to let the one being needed to bind. This is no longer the case,
so there is a change in the CLI and I want to help people facing
this new problem with a slightly more comprehensive message because
people don't fully understand what USB is all about. We cannot
ask all U-Boot USB users to be U-Boot experts and USB experts. This
is just a little help.

In an ideal world, we will have the possibility to create
composite gadgets and all this can go away once it is well
integrated, I guess?

Thanks,
Miquèl

Re: [PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help

[Miquel Raynal]

Hi Marek,

marex@denx.de wrote on Thu, 5 Oct 2023 15:01:51 +0200:

> On 10/2/23 15:46, Miquel Raynal wrote:
> > While it may sound totally obvious for the regular U-Boot developer to
> > get the parameters of the bind/unbind commands from the output of 'dm
> > tree', it did not felt straightforward to me until I was explicitly
> > told to look there. And even when I knew the command, I did not make a
> > direct link between the arguments of this command and the columns
> > returned by 'dm tree'.
> > 
> > Several of us lost a lot of time because of that, I would like to kindly
> > help other users by slightly improving this textual line. Unfortunately,
> > because of how this string is used (like within the 'help' command) I
> > cannot detail much more, but at least the pointer is there.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > Resend: no change.
> > 
> > Changes in v2:
> > * Moved the details in the long text section as suggested by Heinrich.
> > * Rephrased the help text as well, not fully following Heinrich
> >    suggestion, but taking inspiration from it.
> > ---
> >   cmd/bind.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/cmd/bind.c b/cmd/bind.c
> > index 4d1b7885e60..be0d4d2a711 100644
> > --- a/cmd/bind.c
> > +++ b/cmd/bind.c
> > @@ -246,6 +246,8 @@ U_BOOT_CMD(
> >   	"Bind a device to a driver",
> >   	"<node path> <driver>\n"
> >   	"bind <class> <index> <driver>\n"
> > +	"Use 'dm tree' to list all devices registered in the driver model,\n"
> > +	"their path, class, index and current driver.\n"
> >   );  
> >   >   U_BOOT_CMD(  
> > @@ -254,4 +256,6 @@ U_BOOT_CMD(
> >   	"<node path>\n"
> >   	"unbind <class> <index>\n"
> >   	"unbind <class> <index> <driver>\n"
> > +	"Use 'dm tree' to list all devices registered in the driver model,\n"
> > +	"their path, class, index and current driver.\n"  
> 
> 'dm tree' depends on CMD_DM , you might need some if (IS_ENABLED()) here.

Well, no, and that's my point actually. People need to know that this
command exist and should be used for this purpose. If you only tell
them "use 'dm tree'" conditionally, you loose part of the audience:
people not aware of this command or its usefulness.

Thanks,
Miquèl

Re: [PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help

[Marek Vasut]

On 10/5/23 17:54, Miquel Raynal wrote:
> Hi Marek,
> 
> marex@denx.de wrote on Thu, 5 Oct 2023 15:01:51 +0200:
> 
>> On 10/2/23 15:46, Miquel Raynal wrote:
>>> While it may sound totally obvious for the regular U-Boot developer to
>>> get the parameters of the bind/unbind commands from the output of 'dm
>>> tree', it did not felt straightforward to me until I was explicitly
>>> told to look there. And even when I knew the command, I did not make a
>>> direct link between the arguments of this command and the columns
>>> returned by 'dm tree'.
>>>
>>> Several of us lost a lot of time because of that, I would like to kindly
>>> help other users by slightly improving this textual line. Unfortunately,
>>> because of how this string is used (like within the 'help' command) I
>>> cannot detail much more, but at least the pointer is there.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>> Resend: no change.
>>>
>>> Changes in v2:
>>> * Moved the details in the long text section as suggested by Heinrich.
>>> * Rephrased the help text as well, not fully following Heinrich
>>>     suggestion, but taking inspiration from it.
>>> ---
>>>    cmd/bind.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/cmd/bind.c b/cmd/bind.c
>>> index 4d1b7885e60..be0d4d2a711 100644
>>> --- a/cmd/bind.c
>>> +++ b/cmd/bind.c
>>> @@ -246,6 +246,8 @@ U_BOOT_CMD(
>>>    	"Bind a device to a driver",
>>>    	"<node path> <driver>\n"
>>>    	"bind <class> <index> <driver>\n"
>>> +	"Use 'dm tree' to list all devices registered in the driver model,\n"
>>> +	"their path, class, index and current driver.\n"
>>>    );
>>>    >   U_BOOT_CMD(
>>> @@ -254,4 +256,6 @@ U_BOOT_CMD(
>>>    	"<node path>\n"
>>>    	"unbind <class> <index>\n"
>>>    	"unbind <class> <index> <driver>\n"
>>> +	"Use 'dm tree' to list all devices registered in the driver model,\n"
>>> +	"their path, class, index and current driver.\n"
>>
>> 'dm tree' depends on CMD_DM , you might need some if (IS_ENABLED()) here.
> 
> Well, no, and that's my point actually. People need to know that this
> command exist and should be used for this purpose. If you only tell
> them "use 'dm tree'" conditionally, you loose part of the audience:
> people not aware of this command or its usefulness.

If you tell them 'Use "dm tree"' and it prints "no command found" , then 
it's a bug.

You likely want to add 'imply CMD_DM' into CMD_BIND Kconfig to make sure 
CMD_DM is available by default, but also that users can turn it off if 
they really want to.

Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Marek Vasut]

On 10/5/23 17:51, Miquel Raynal wrote:
> Hi Marek,
> 
> marex@denx.de wrote on Thu, 5 Oct 2023 15:04:25 +0200:
> 
>> On 10/2/23 15:46, Miquel Raynal wrote:
>>> At some point when trying to use USB gadgets, two situations may arise
>>> and lead to a failure. Either the UDC (USB Device Controller) is not
>>> available at all (not described or not probed) or the UDC is already in
>>> use. For instance, as the USB Ethernet gadget remains bound to the UDC,
>>> the use of any other USB gadget (fastboot, dfu, etc) *after* will always
>>> fail with the "couldn't find an available UDC" error.
>>>
>>> Let's give a more helpful message by making a difference between the two
>>> cases. Let's also hint people who would get this error and grep it into
>>> the sources a better explanation of what's wrong with their workflow.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>> While doing this I really wanted to add "much more" error comments but I
>>> faced another reality: often the messages are there but use
>>> pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
>>> I consider this unnecessary, as decreasing the loglevel will make these
>>> messages appear. I would have expected errors to be displayed, but I
>>> understand it makes the binaries even bigger.
>>>
>>> Resend: no change.
>>>
>>> Changes in v2:
>>> * s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
>>>     not sound well at all when there is only one UDC.
>>> ---
>>>    drivers/usb/gadget/udc/udc-core.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>>> index 7f73926cb3e..8405b03462e 100644
>>> --- a/drivers/usb/gadget/udc/udc-core.c
>>> +++ b/drivers/usb/gadget/udc/udc-core.c
>>> @@ -323,6 +323,7 @@ err1:
>>>    int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>>>    {
>>>    	struct usb_udc		*udc = NULL;
>>> +	unsigned int		udc_count = 0;
>>>    	int			ret;
>>>    >   	if (!driver || !driver->bind || !driver->setup)
>>> @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
>>>    >   	mutex_lock(&udc_lock);
>>>    	list_for_each_entry(udc, &udc_list, list) {
>>> +		udc_count++;
>>> +
>>>    		/* For now we take the first one */
>>>    		if (!udc->driver)
>>>    			goto found;
>>>    	}
>>>    > -	printf("couldn't find an available UDC\n");
>>> +	if (!udc_count)
>>> +		printf("No UDC available in the system\n");
>>> +	else
>>> +		/* When this happens, users should 'unbind <class> <index>'
>>> +		 * using the output of 'dm tree' and looking at the line right
>>> +		 * after the USB peripheral/device controller.
>>> +		 */
>>> +		printf("All UDCs in use (%d available), use the unbind command\n",
>>> +		       udc_count);
>>
>> Users should really use 'bind' command in the first place, to avoid this guesswork to which UDC (on systems with multiple UDCs) a gadget gets bound to, and instead bind the gadget to specific UDC they want to bind it to. This code should then be removed entirely.
> 
> There are two cases when this can happen:
> * a single UDC (common) but a function already bound, there is no
>    guessing here
> * two (or more) UDC and there is some guessing
> 
> Before your cleanup, drivers would get automatically unbound from the
> UDC to let the one being needed to bind.

How did that ever work ?

> This is no longer the case,
> so there is a change in the CLI and I want to help people facing
> this new problem with a slightly more comprehensive message because
> people don't fully understand what USB is all about. We cannot
> ask all U-Boot USB users to be U-Boot experts and USB experts. This
> is just a little help.

In that case, you have to handle the details like CMD_BIND may not be 
enabled, and CMD_DM may not be enabled. But then, maybe what we want to 
do is fix the automatic switching of gadgets to make it work again ?

> In an ideal world, we will have the possibility to create
> composite gadgets and all this can go away once it is well
> integrated, I guess?

I believe you would still have to do a disconnect/connect cycle even 
with a composite driver, you cannot just add functions to existing 
composite at runtime.

Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Miquel Raynal]

Hi Marek,

> >>> --- a/drivers/usb/gadget/udc/udc-core.c
> >>> +++ b/drivers/usb/gadget/udc/udc-core.c
> >>> @@ -323,6 +323,7 @@ err1:
> >>>    int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
> >>>    {
> >>>    	struct usb_udc		*udc = NULL;
> >>> +	unsigned int		udc_count = 0;
> >>>    	int			ret;  
> >>>    >   	if (!driver || !driver->bind || !driver->setup)  
> >>> @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)  
> >>>    >   	mutex_lock(&udc_lock);  
> >>>    	list_for_each_entry(udc, &udc_list, list) {
> >>> +		udc_count++;
> >>> +
> >>>    		/* For now we take the first one */
> >>>    		if (!udc->driver)
> >>>    			goto found;
> >>>    	}  
> >>>    > -	printf("couldn't find an available UDC\n");  
> >>> +	if (!udc_count)
> >>> +		printf("No UDC available in the system\n");
> >>> +	else
> >>> +		/* When this happens, users should 'unbind <class> <index>'
> >>> +		 * using the output of 'dm tree' and looking at the line right
> >>> +		 * after the USB peripheral/device controller.
> >>> +		 */
> >>> +		printf("All UDCs in use (%d available), use the unbind command\n",
> >>> +		       udc_count);  
> >>
> >> Users should really use 'bind' command in the first place, to avoid this guesswork to which UDC (on systems with multiple UDCs) a gadget gets bound to, and instead bind the gadget to specific UDC they want to bind it to. This code should then be removed entirely.  
> > 
> > There are two cases when this can happen:
> > * a single UDC (common) but a function already bound, there is no
> >    guessing here
> > * two (or more) UDC and there is some guessing
> > 
> > Before your cleanup, drivers would get automatically unbound from the
> > UDC to let the one being needed to bind.  
> 
> How did that ever work ?

I believe both commands (fastboot vs. network commands like dhcp, tftp,
etc) would automatically bind to the first UDC and unbind once the
command done. But you recently changed that.

> > This is no longer the case,
> > so there is a change in the CLI and I want to help people facing
> > this new problem with a slightly more comprehensive message because
> > people don't fully understand what USB is all about. We cannot
> > ask all U-Boot USB users to be U-Boot experts and USB experts. This
> > is just a little help.  
> 
> In that case, you have to handle the details like CMD_BIND may not be enabled, and CMD_DM may not be enabled.

If I understand correctly you would like this to be described:

CMD_BIND imply CMD_DM
USB_GADGET imply CMD_BIND

Currently there is:

CMD_BIND default y if USB_ETHER

Maybe we should instead have:

CMD_BIND default y if USB_GADGET

And perhaps I would even go further and change the dependency
direction, so:

USB_GADGET imply CMD_BIND
CMD_BIND imply CMD_DM

Would this work for you?

> But then, maybe what we want to do is fix the automatic switching of
> gadgets to make it work again ?

This is a longer term goal I guess.

> > In an ideal world, we will have the possibility to create
> > composite gadgets and all this can go away once it is well
> > integrated, I guess?  
> 
> I believe you would still have to do a disconnect/connect cycle even with a composite driver, you cannot just add functions to existing composite at runtime.

I have in mind an environment variable which could define the "default"
composite gadget that we want at boot time, but then if people want to
change the gadgets exposed they would indeed need some kind of
disconnect/connect machinery, agreed.

Thanks, Miquèl