[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[Simon Glass]

We should aim for a single point of entry to the commands, whichever
parser is used.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/command.c  |   10 ++++++++++
 common/hush.c     |    9 +++------
 common/main.c     |    3 +--
 include/command.h |   12 ++++++++++++
 4 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/common/command.c b/common/command.c
index c5cecd3..acc1c15 100644
--- a/common/command.c
+++ b/common/command.c
@@ -487,3 +487,13 @@ void fixup_cmdtable(cmd_tbl_t *cmdtp, int size)
 	}
 }
 #endif
+
+int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	int result;
+
+	result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
+	if (result)
+		debug("Command failed, result=%d", result);
+	return result;
+}
diff --git a/common/hush.c b/common/hush.c
index 940889b..8106a91 100644
--- a/common/hush.c
+++ b/common/hush.c
@@ -1695,13 +1695,10 @@ static int run_pipe_real(struct pipe *pi)
 				rcode = x->function(child);
 #else
 				/* OK - call function to do the command */
-
-				rcode = (cmdtp->cmd)
-(cmdtp, flag,child->argc-i,&child->argv[i]);
-				if ( !cmdtp->repeatable )
+				rcode = cmd_call(cmdtp, flag,  child->argc-i,
+						 &child->argv[i]);
+				if (!cmdtp->repeatable)
 					flag_repeat = 0;
-
-
 #endif
 				child->argv-=i;  /* XXX restore hack so free() can work right */
 #ifndef __U_BOOT__
diff --git a/common/main.c b/common/main.c
index e96c95a..3b60d27 100644
--- a/common/main.c
+++ b/common/main.c
@@ -1376,9 +1376,8 @@ int run_command (const char *cmd, int flag)
 #endif
 
 		/* OK - call function to do the command */
-		if ((cmdtp->cmd) (cmdtp, flag, argc, argv) != 0) {
+		if (cmd_call(cmdtp, flag, argc, argv) != 0)
 			rc = -1;
-		}
 
 		repeatable &= cmdtp->repeatable;
 
diff --git a/include/command.h b/include/command.h
index c270110..3eeef7c 100644
--- a/include/command.h
+++ b/include/command.h
@@ -147,4 +147,16 @@ extern int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 #if defined(CONFIG_NEEDS_MANUAL_RELOC)
 void fixup_cmdtable(cmd_tbl_t *cmdtp, int size);
 #endif
+
+/**
+ * Call a command function. This should be the only route in U-Boot to call
+ * a command, so that we can track whether we are waiting for input or
+ * executing a command.
+ *
+ * @param cmdtp		Pointer to the command to execute
+ * @param flag		Some flags normally 0 (see CMD_FLAG_.. above)
+ * @param argc		Number of arguments (arg 0 must be the command text)
+ * @param argv		Arguments
+ */
+int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 #endif	/* __COMMAND_H */
-- 
1.7.3.1

[U-Boot] [PATCH 2/2] RFC: Add XON/XOFF support

[Simon Glass]

This is just for testing - please try it out and report back with results.
For me it works on Minicom but not ser2net.

This needs to be controlled by an environment variable, CONFIG option or
both. We may need a way of specifying flow control on a per-device basis.
We may need to do something special for X-modem transfers, etc. This might
turn out to be a can of worms.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/command.c |    1 +
 common/console.c |   12 ++++++++++++
 common/main.c    |    3 +++
 include/common.h |    2 ++
 4 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/common/command.c b/common/command.c
index acc1c15..7b7c24e 100644
--- a/common/command.c
+++ b/common/command.c
@@ -492,6 +492,7 @@ int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int result;
 
+	console_suspend_input();
 	result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
 	if (result)
 		debug("Command failed, result=%d", result);
diff --git a/common/console.c b/common/console.c
index f17875e..eeb58e6 100644
--- a/common/console.c
+++ b/common/console.c
@@ -199,6 +199,18 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
 }
 #endif /* defined(CONFIG_CONSOLE_MUX) */
 
+void console_suspend_input(void)
+{
+	/* Send XOFF to tell the other end to stop sending */
+	console_putc(stdout, 'S' - '@');
+}
+
+void console_resume_input(void)
+{
+	/* Send XON to tell the other end to start sending */
+	console_putc(stdout, 'Q' - '@');
+}
+
 /** U-Boot INITIAL CONSOLE-NOT COMPATIBLE FUNCTIONS *************************/
 
 int serial_printf(const char *fmt, ...)
diff --git a/common/main.c b/common/main.c
index 3b60d27..b030176 100644
--- a/common/main.c
+++ b/common/main.c
@@ -948,6 +948,7 @@ int readline_into_buffer (const char *const prompt, char * buffer)
 
 		if (prompt)
 			puts (prompt);
+		console_resume_input();
 
 		rc = cread_line(prompt, p, &len);
 		return rc < 0 ? rc : len;
@@ -967,6 +968,8 @@ int readline_into_buffer (const char *const prompt, char * buffer)
 	}
 	col = plen;
 
+	console_resume_input();
+
 	for (;;) {
 #ifdef CONFIG_BOOT_RETRY_TIME
 		while (!tstc()) {	/* while no incoming data */
diff --git a/include/common.h b/include/common.h
index a1683a2..970ec0d 100644
--- a/include/common.h
+++ b/include/common.h
@@ -738,6 +738,8 @@ int	ctrlc (void);
 int	had_ctrlc (void);	/* have we had a Control-C since last clear? */
 void	clear_ctrlc (void);	/* clear the Control-C condition */
 int	disable_ctrlc (int);	/* 1 to disable, 0 to enable Control-C detect */
+void console_suspend_input(void);
+void console_resume_input(void);
 
 /*
  * STDIO based functions (can always be used)
-- 
1.7.3.1

[U-Boot] [PATCH 2/2] RFC: Add XON/XOFF support

[Graeme Russ]

Hi Simon,

On 25/10/11 14:52, Simon Glass wrote:
> This is just for testing - please try it out and report back with results.
> For me it works on Minicom but not ser2net.
> 
> This needs to be controlled by an environment variable, CONFIG option or
> both. We may need a way of specifying flow control on a per-device basis.
> We may need to do something special for X-modem transfers, etc. This might
> turn out to be a can of worms.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  common/command.c |    1 +
>  common/console.c |   12 ++++++++++++
>  common/main.c    |    3 +++
>  include/common.h |    2 ++
>  4 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/common/command.c b/common/command.c
> index acc1c15..7b7c24e 100644
> --- a/common/command.c
> +++ b/common/command.c
> @@ -492,6 +492,7 @@ int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
>  	int result;
>  
> +	console_suspend_input();

Not the best name, but I cannot think of better right now

>  	result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
>  	if (result)
>  		debug("Command failed, result=%d", result);
> diff --git a/common/console.c b/common/console.c
> index f17875e..eeb58e6 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -199,6 +199,18 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
>  }
>  #endif /* defined(CONFIG_CONSOLE_MUX) */
>  
> +void console_suspend_input(void)
> +{
> +	/* Send XOFF to tell the other end to stop sending */
> +	console_putc(stdout, 'S' - '@');

Uugh - #define XON and XOFF please

> +}
> +
> +void console_resume_input(void)
> +{
> +	/* Send XON to tell the other end to start sending */
> +	console_putc(stdout, 'Q' - '@');
> +}
> +
>  /** U-Boot INITIAL CONSOLE-NOT COMPATIBLE FUNCTIONS *************************/
>  
>  int serial_printf(const char *fmt, ...)
> diff --git a/common/main.c b/common/main.c
> index 3b60d27..b030176 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -948,6 +948,7 @@ int readline_into_buffer (const char *const prompt, char * buffer)
>  
>  		if (prompt)
>  			puts (prompt);
> +		console_resume_input();

Not needed - move to getc()

>  		rc = cread_line(prompt, p, &len);
>  		return rc < 0 ? rc : len;
> @@ -967,6 +968,8 @@ int readline_into_buffer (const char *const prompt, char * buffer)
>  	}
>  	col = plen;
>  
> +	console_resume_input();

Not needed (in getc())

> +
>  	for (;;) {
>  #ifdef CONFIG_BOOT_RETRY_TIME
>  		while (!tstc()) {	/* while no incoming data */
> diff --git a/include/common.h b/include/common.h
> index a1683a2..970ec0d 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -738,6 +738,8 @@ int	ctrlc (void);
>  int	had_ctrlc (void);	/* have we had a Control-C since last clear? */
>  void	clear_ctrlc (void);	/* clear the Control-C condition */
>  int	disable_ctrlc (int);	/* 1 to disable, 0 to enable Control-C detect */
> +void console_suspend_input(void);
> +void console_resume_input(void);
>  
>  /*
>   * STDIO based functions (can always be used)

What about the secondart 'rx_flush()' I mentioned?

Regards,

Graeme

[U-Boot] [PATCH 2/2] RFC: Add XON/XOFF support

[Simon Glass]

Hi Graeme,

On Tue, Oct 25, 2011 at 1:09 AM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Simon,
>
> On 25/10/11 14:52, Simon Glass wrote:
>> This is just for testing - please try it out and report back with results.
>> For me it works on Minicom but not ser2net.
>>
>> This needs to be controlled by an environment variable, CONFIG option or
>> both. We may need a way of specifying flow control on a per-device basis.
>> We may need to do something special for X-modem transfers, etc. This might
>> turn out to be a can of worms.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> ?common/command.c | ? ?1 +
>> ?common/console.c | ? 12 ++++++++++++
>> ?common/main.c ? ?| ? ?3 +++
>> ?include/common.h | ? ?2 ++
>> ?4 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/common/command.c b/common/command.c
>> index acc1c15..7b7c24e 100644
>> --- a/common/command.c
>> +++ b/common/command.c
>> @@ -492,6 +492,7 @@ int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> ?{
>> ? ? ? int result;
>>
>> + ? ? console_suspend_input();
>
> Not the best name, but I cannot think of better right now
>
>> ? ? ? result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
>> ? ? ? if (result)
>> ? ? ? ? ? ? ? debug("Command failed, result=%d", result);
>> diff --git a/common/console.c b/common/console.c
>> index f17875e..eeb58e6 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -199,6 +199,18 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
>> ?}
>> ?#endif /* defined(CONFIG_CONSOLE_MUX) */
>>
>> +void console_suspend_input(void)
>> +{
>> + ? ? /* Send XOFF to tell the other end to stop sending */
>> + ? ? console_putc(stdout, 'S' - '@');
>
> Uugh - #define XON and XOFF please

OK

>
>> +}
>> +
>> +void console_resume_input(void)
>> +{
>> + ? ? /* Send XON to tell the other end to start sending */
>> + ? ? console_putc(stdout, 'Q' - '@');
>> +}
>> +
>> ?/** U-Boot INITIAL CONSOLE-NOT COMPATIBLE FUNCTIONS *************************/
>>
>> ?int serial_printf(const char *fmt, ...)
>> diff --git a/common/main.c b/common/main.c
>> index 3b60d27..b030176 100644
>> --- a/common/main.c
>> +++ b/common/main.c
>> @@ -948,6 +948,7 @@ int readline_into_buffer (const char *const prompt, char * buffer)
>>
>> ? ? ? ? ? ? ? if (prompt)
>> ? ? ? ? ? ? ? ? ? ? ? puts (prompt);
>> + ? ? ? ? ? ? console_resume_input();
>
> Not needed - move to getc()

Well we need to thing about the other issues mentioned in my commit
message also.

>
>> ? ? ? ? ? ? ? rc = cread_line(prompt, p, &len);
>> ? ? ? ? ? ? ? return rc < 0 ? rc : len;
>> @@ -967,6 +968,8 @@ int readline_into_buffer (const char *const prompt, char * buffer)
>> ? ? ? }
>> ? ? ? col = plen;
>>
>> + ? ? console_resume_input();
>
> Not needed (in getc())
>
>> +
>> ? ? ? for (;;) {
>> ?#ifdef CONFIG_BOOT_RETRY_TIME
>> ? ? ? ? ? ? ? while (!tstc()) { ? ? ? /* while no incoming data */
>> diff --git a/include/common.h b/include/common.h
>> index a1683a2..970ec0d 100644
>> --- a/include/common.h
>> +++ b/include/common.h
>> @@ -738,6 +738,8 @@ int ? ? ? ctrlc (void);
>> ?int ?had_ctrlc (void); ? ? ? /* have we had a Control-C since last clear? */
>> ?void clear_ctrlc (void); ? ? /* clear the Control-C condition */
>> ?int ?disable_ctrlc (int); ? ?/* 1 to disable, 0 to enable Control-C detect */
>> +void console_suspend_input(void);
>> +void console_resume_input(void);
>>
>> ?/*
>> ? * STDIO based functions (can always be used)
>
> What about the secondart 'rx_flush()' I mentioned?

Will await discussions before doing a new patch :-)

Regards,
Simon

>
> Regards,
>
> Graeme
>

[U-Boot] [PATCH 2/2] RFC: Add XON/XOFF support

[Mike Frysinger]

On Monday 24 October 2011 23:52:24 Simon Glass wrote:
> This is just for testing - please try it out and report back with results.
> For me it works on Minicom but not ser2net.
> 
> This needs to be controlled by an environment variable, CONFIG option or
> both. We may need a way of specifying flow control on a per-device basis.
> We may need to do something special for X-modem transfers, etc. This might
> turn out to be a can of worms.

we have CONFIG_HWFLOW already, so CONFIG_SWFLOW would probably be the natural 
choice.  and then have the funcs be named swflow_xxx (although the hwflow code 
does it via one and uses its arg to figure out what to do).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111030/e2f12354/attachment.pgp 

[U-Boot] [PATCH 2/2] RFC: Add XON/XOFF support

[Albert ARIBAUD]

Hi Simon,

Le 25/10/2011 05:52, Simon Glass a ?crit :
> This is just for testing - please try it out and report back with results.
> For me it works on Minicom but not ser2net.

Exactly how does it not work, i.e. what happens that you did not expect, 
or what does not happen that you did expect?

> This needs to be controlled by an environment variable, CONFIG option or
> both. We may need a way of specifying flow control on a per-device basis.

Hmm... We want XON/XOFF on the console only. What other device would 
require this?

> We may need to do something special for X-modem transfers, etc. This might
> turn out to be a can of worms.

I think any U-Boot command that uses the console as a means of data 
transfer should 'resume console' when starting, and can safely do so 
because if the command was invoked, then most certainly the other end of 
the console is not going to fire away; it is going to wait for U-Boot to 
get ready for transfer. Suspending the console after transfer should be 
done if resume was done.

BTW:

> Signed-off-by: Simon Glass<sjg@chromium.org>
> ---
>   common/command.c |    1 +
>   common/console.c |   12 ++++++++++++
>   common/main.c    |    3 +++
>   include/common.h |    2 ++
>   4 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/common/command.c b/common/command.c
> index acc1c15..7b7c24e 100644
> --- a/common/command.c
> +++ b/common/command.c
> @@ -492,6 +492,7 @@ int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   {
>   	int result;
>
> +	console_suspend_input();
>   	result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
>   	if (result)
>   		debug("Command failed, result=%d", result);
> diff --git a/common/console.c b/common/console.c
> index f17875e..eeb58e6 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -199,6 +199,18 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
>   }
>   #endif /* defined(CONFIG_CONSOLE_MUX) */
>
> +void console_suspend_input(void)
> +{
> +	/* Send XOFF to tell the other end to stop sending */
> +	console_putc(stdout, 'S' - '@');
> +}

Please define XOFF and XON.

> +void console_resume_input(void)
> +{
> +	/* Send XON to tell the other end to start sending */
> +	console_putc(stdout, 'Q' - '@');
> +}
> +
>   /** U-Boot INITIAL CONSOLE-NOT COMPATIBLE FUNCTIONS *************************/
>
>   int serial_printf(const char *fmt, ...)
> diff --git a/common/main.c b/common/main.c
> index 3b60d27..b030176 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -948,6 +948,7 @@ int readline_into_buffer (const char *const prompt, char * buffer)
>
>   		if (prompt)
>   			puts (prompt);
> +		console_resume_input();

This makes XON sent on each readline. XOFF should be sent only if XOFF 
was sent before, IMO (and then, XOFF should only be sent if it was not 
sent already.

Plus, why send it on readlines? Why not just surround the 'result = 
(cmdtp->cmd)(cmdtp, flag, argc, argv);' above with a 'suspend' before 
and a 'resume' after?

>   		rc = cread_line(prompt, p,&len);
>   		return rc<  0 ? rc : len;
> @@ -967,6 +968,8 @@ int readline_into_buffer (const char *const prompt, char * buffer)
>   	}
>   	col = plen;
>
> +	console_resume_input();
> +
>   	for (;;) {
>   #ifdef CONFIG_BOOT_RETRY_TIME
>   		while (!tstc()) {	/* while no incoming data */
> diff --git a/include/common.h b/include/common.h
> index a1683a2..970ec0d 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -738,6 +738,8 @@ int	ctrlc (void);
>   int	had_ctrlc (void);	/* have we had a Control-C since last clear? */
>   void	clear_ctrlc (void);	/* clear the Control-C condition */
>   int	disable_ctrlc (int);	/* 1 to disable, 0 to enable Control-C detect */
> +void console_suspend_input(void);
> +void console_resume_input(void);
>
>   /*
>    * STDIO based functions (can always be used)

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[Wolfgang Denk]

Dear Simon Glass,

In message <1319514744-18697-1-git-send-email-sjg@chromium.org> you wrote:
> We should aim for a single point of entry to the commands, whichever
> parser is used.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  common/command.c  |   10 ++++++++++
>  common/hush.c     |    9 +++------
>  common/main.c     |    3 +--
>  include/command.h |   12 ++++++++++++
>  4 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/common/command.c b/common/command.c
> index c5cecd3..acc1c15 100644
> --- a/common/command.c
> +++ b/common/command.c
> @@ -487,3 +487,13 @@ void fixup_cmdtable(cmd_tbl_t *cmdtp, int size)
>  	}
>  }
>  #endif
> +
> +int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	int result;
> +
> +	result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
> +	if (result)
> +		debug("Command failed, result=%d", result);
> +	return result;
> +}

What exactly is the purpose of this additional function?  Except for
the debug() it provides only overhead and no benefit.

I don't think I want to have that.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Mr. Cole's Axiom:
        The sum of the intelligence on the planet is a constant;
        the population is growing.

[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[Graeme Russ]

Hi Wolfgang,

On 25/10/11 18:46, Wolfgang Denk wrote:
> Dear Simon Glass,
> 
> In message <1319514744-18697-1-git-send-email-sjg@chromium.org> you wrote:
>> We should aim for a single point of entry to the commands, whichever
>> parser is used.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>  common/command.c  |   10 ++++++++++
>>  common/hush.c     |    9 +++------
>>  common/main.c     |    3 +--
>>  include/command.h |   12 ++++++++++++
>>  4 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/common/command.c b/common/command.c
>> index c5cecd3..acc1c15 100644
>> --- a/common/command.c
>> +++ b/common/command.c
>> @@ -487,3 +487,13 @@ void fixup_cmdtable(cmd_tbl_t *cmdtp, int size)
>>  	}
>>  }
>>  #endif
>> +
>> +int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +	int result;
>> +
>> +	result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
>> +	if (result)
>> +		debug("Command failed, result=%d", result);
>> +	return result;
>> +}
> 
> What exactly is the purpose of this additional function?  Except for
> the debug() it provides only overhead and no benefit.

It provides a single location to issue an XOFF immediately prior to running
a (potentially long running) command

> I don't think I want to have that.

Well it does make things cleaner if we do end up implementing software flow
control

Regards,

Graeme

[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[Simon Glass]

Hi Wolfgang,

On Tue, Oct 25, 2011 at 1:03 AM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Wolfgang,
>
> On 25/10/11 18:46, Wolfgang Denk wrote:
>> Dear Simon Glass,
>>
>> In message <1319514744-18697-1-git-send-email-sjg@chromium.org> you wrote:
>>> We should aim for a single point of entry to the commands, whichever
>>> parser is used.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>> ?common/command.c ?| ? 10 ++++++++++
>>> ?common/hush.c ? ? | ? ?9 +++------
>>> ?common/main.c ? ? | ? ?3 +--
>>> ?include/command.h | ? 12 ++++++++++++
>>> ?4 files changed, 26 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/common/command.c b/common/command.c
>>> index c5cecd3..acc1c15 100644
>>> --- a/common/command.c
>>> +++ b/common/command.c
>>> @@ -487,3 +487,13 @@ void fixup_cmdtable(cmd_tbl_t *cmdtp, int size)
>>> ? ? ?}
>>> ?}
>>> ?#endif
>>> +
>>> +int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>> +{
>>> + ? ?int result;
>>> +
>>> + ? ?result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
>>> + ? ?if (result)
>>> + ? ? ? ? ? ?debug("Command failed, result=%d", result);
>>> + ? ?return result;
>>> +}
>>
>> What exactly is the purpose of this additional function? ?Except for
>> the debug() it provides only overhead and no benefit.
>
> It provides a single location to issue an XOFF immediately prior to running
> a (potentially long running) command
>
>> I don't think I want to have that.
>
> Well it does make things cleaner if we do end up implementing software flow
> control

What Graeme said :-)

It could probably be done in an inline fashion though to avoid overhead.

Regards,
Simon

>
> Regards,
>
> Graeme
>

[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[Wolfgang Denk]

Dear Graeme Russ,

In message <4EA66D4C.8010506@gmail.com> you wrote:
> 
> > What exactly is the purpose of this additional function?  Except for
> > the debug() it provides only overhead and no benefit.
> 
> It provides a single location to issue an XOFF immediately prior to running
> a (potentially long running) command
> 
> > I don't think I want to have that.
> 
> Well it does make things cleaner if we do end up implementing software flow
> control

If, and only if.  _And_ if we agree that we should hook into command
execution, which I don't consider a good idea.

Serial flow control has nothing to do with that.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You can fool some of the people all of the time, and You can fool all
of the people some of the time, but You can't fool mom.

[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[Mike Frysinger]

On Mon, Oct 24, 2011 at 23:52, Simon Glass wrote:
> +int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> + ? ? ? int result;
> +
> + ? ? ? result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
> + ? ? ? if (result)
> + ? ? ? ? ? ? ? debug("Command failed, result=%d", result);
> + ? ? ? return result;
> +}

i don't think this goes for enough.  it should integrate the "if (argc
> cmdtp->maxargs) return cmd_usage(cmdtp);".

and perhaps even the find_cmd(argv[0]) lookup ...
-mike

[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[Simon Glass]

Hi Mike,

On Tue, Oct 25, 2011 at 6:57 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Mon, Oct 24, 2011 at 23:52, Simon Glass wrote:
>> +int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + ? ? ? int result;
>> +
>> + ? ? ? result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
>> + ? ? ? if (result)
>> + ? ? ? ? ? ? ? debug("Command failed, result=%d", result);
>> + ? ? ? return result;
>> +}
>
> i don't think this goes for enough. ?it should integrate the "if (argc
>> cmdtp->maxargs) return cmd_usage(cmdtp);".
>

Yes, that might turn this into a patch worth accepting on its merits.

> and perhaps even the find_cmd(argv[0]) lookup ...
> -mike
>

How about if the commands return error codes, one of which means
'print usage'? Then we might remove 265 calls to cmd_usage and even
reduce code size :-O

Continuing down the slippery slope, for find_cmd() there is sometimes
a secondary command, and there are also argument to decode. We could
devise a printf-style command description to specify the arguments for
many/most commands. Even less code. More complicated.

Regards,
Simon

[U-Boot] [PATCH 1/2] Create a single cmd_call() function to handle command execution

[Mike Frysinger]

On Tuesday 25 October 2011 19:05:02 Simon Glass wrote:
> On Tue, Oct 25, 2011 at 6:57 AM, Mike Frysinger wrote:
> > On Mon, Oct 24, 2011 at 23:52, Simon Glass wrote:
> >> +int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >> +{
> >> +       int result;
> >> +
> >> +       result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
> >> +       if (result)
> >> +               debug("Command failed, result=%d", result);
> >> +       return result;
> >> +}
> > 
> > i don't think this goes for enough.  it should integrate the "if (argc
> > 
> >> cmdtp->maxargs) return cmd_usage(cmdtp);".
> 
> Yes, that might turn this into a patch worth accepting on its merits.
> 
> > and perhaps even the find_cmd(argv[0]) lookup ...
> > -mike
> 
> How about if the commands return error codes, one of which means
> 'print usage'? Then we might remove 265 calls to cmd_usage and even
> reduce code size :-O

i'm not sure hardcoding a specific value across all commands is possible.  for 
most commands, we do just return -1/0/1, which probably should be normalized 
into 0/1 ...

but the overhead with these calls is probably not that bad since we return via 
it.  so the asm isn't a matter of setting up the args, making the call, moving 
the return into the right place, and then returning ... we set up the args and 
jump to cmd_usage.  for some arches (depending on the calling convention), we 
might not even have to touch the args since the one arg is already in the 
right place.

you could prototype the overhead:
-	return cmd_usage(cmdtp);
+	return -500;
but i wouldn't be surprised if there was no difference, or even if this was 
actually more overhead ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111030/9afbb393/attachment.pgp