[U-Boot] [PATCH 0/4] Solve issue with serial rx buffer when MUX is deactivated
When I activate CONFIG_SERIAL_RX_BUFFER on my board without CONSOLE_MUX, I have a strange behavior in console: a new prompt is displayed continuously
I check the call stack
cread_line (common/cli_readline.c) => getcmd_getch (common/cli_readline.c) ==> fgetc (common/console.c) ===> console_tstc (common/console.c) ====> stdio_devices[file]->get() =====> _serial_getc() (drivers/serial/serial-uclass.c) and the data reads from rx buffer but it is garbage as tstc is not called
PS: I have no issue when CONSOLE_MUX is activated because in this case the tstc() is always called in fgetc loop.
My first solution to update the rx buffer management, to avoid the issue:
static int _serial_getc(struct udevice *dev) { struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); char val;
if (upriv->rd_ptr == upriv->wr_ptr) __serial_tstc(dev);
if (upriv->rd_ptr == upriv->wr_ptr) return 0; /* error : no data to read */
val = upriv->buf[upriv->rd_ptr++]; upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;
return val; }
With this first patch, I see that that WATCHDOG is not reloaded and the getc() error value 0x0 wasn't correctly handle in cli; the issue is alway present and the behavior change (getc function is no more blocking): the caller needs to handle the error and reload the watchdog.
To summarize, I see several issues in the current U-Boot code: - No protection on the rx buffer in _serial_getc(): the current code assumed that tstc() is alway called but it is dangerous - error for getc() (read value = 0x0) is not handled in cread_line() - in console.c, tstc() is not called when CONSOLE_MUX is not activated
In this patchset I try to solve all these issues by separate patch but perhaps only the first correction on the rx buffer is really mandatory.
On my board stm32mp1 ev1 the issue is solved.
Patrick Delaunay (4): stm32mp1: activate serial rx buffer serial: protect access to serial rx buffer console: unify fgetc function when console MUX is deactivated cli: handle getch error
common/cli_readline.c | 4 ++++ common/console.c | 9 +++++---- configs/stm32mp15_basic_defconfig | 1 + drivers/serial/serial-uclass.c | 3 +++ 4 files changed, 13 insertions(+), 4 deletions(-)
Activate the serial rx buffer. Prepare console MUX activation with vidconsole, and avoid console performance issue (missing character for copy-paste).
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
configs/stm32mp15_basic_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/configs/stm32mp15_basic_defconfig b/configs/stm32mp15_basic_defconfig index c72a440..9792e49 100644 --- a/configs/stm32mp15_basic_defconfig +++ b/configs/stm32mp15_basic_defconfig @@ -41,4 +41,5 @@ CONFIG_DM_REGULATOR_FIXED=y CONFIG_DM_REGULATOR_GPIO=y CONFIG_DM_REGULATOR_STM32_VREFBUF=y CONFIG_DM_REGULATOR_STPMU1=y +CONFIG_SERIAL_RX_BUFFER=y CONFIG_STM32_SERIAL=y
On Fri, Aug 03, 2018 at 01:38:42PM +0200, Patrick Delaunay wrote:
Activate the serial rx buffer. Prepare console MUX activation with vidconsole, and avoid console performance issue (missing character for copy-paste).
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Applied to u-boot/master, thanks!
Add test to avoid access to rx buffer when this buffer is empty. In this case directly call getc() function to avoid issue when tstc() is not called.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
drivers/serial/serial-uclass.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23e..4121a37 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -228,6 +228,9 @@ static int _serial_getc(struct udevice *dev) struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); char val;
+ if (upriv->rd_ptr == upriv->wr_ptr) + return __serial_getc(dev); + val = upriv->buf[upriv->rd_ptr++]; upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;
On 3 August 2018 at 05:38, Patrick Delaunay patrick.delaunay@st.com wrote:
Add test to avoid access to rx buffer when this buffer is empty. In this case directly call getc() function to avoid issue when tstc() is not called.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
drivers/serial/serial-uclass.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23e..4121a37 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -228,6 +228,9 @@ static int _serial_getc(struct udevice *dev) struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); char val;
if (upriv->rd_ptr == upriv->wr_ptr)return __serial_getc(dev);val = upriv->buf[upriv->rd_ptr++]; upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;-- 2.7.4
Reviewed-by: Simon Glass sjg@chromium.org
BTW I think the code in _serial_tstc() is wrong, or at least sub-optimal:
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
It should call serial_getc() until it returns -EAGAIN. There should be no need to call __serial_tstc() first,
Regards, Simon
Hi Simon and Stefan,
Sorry for my late answer (I just come back from my summer holiday)
From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: mercredi 8 août 2018 11:56
On 3 August 2018 at 05:38, Patrick Delaunay patrick.delaunay@st.com wrote:
Add test to avoid access to rx buffer when this buffer is empty. In this case directly call getc() function to avoid issue when tstc() is not called.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
drivers/serial/serial-uclass.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23e..4121a37 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -228,6 +228,9 @@ static int _serial_getc(struct udevice *dev) struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); char val;
if (upriv->rd_ptr == upriv->wr_ptr)return __serial_getc(dev);val = upriv->buf[upriv->rd_ptr++]; upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;-- 2.7.4
Reviewed-by: Simon Glass sjg@chromium.org
BTW I think the code in _serial_tstc() is wrong, or at least sub-optimal:
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
It should call serial_getc() until it returns -EAGAIN. There should be no need to call __serial_tstc() first,
This part had be added by Stefan Roese in SHA1 3ca7a06afb7cbc867b7861a8b9aada74e5bbb559
But I check again the code, and I think the code is correct.... but I agree it is not optimal.
we can directly ops->getc(dev) and no more __serial_getc() or __serial_tstc() : the behavior don't change but the access to serial driver is reduced. When no char is available, ops->getc witll return -EAGAIN
static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
If you are OK I will push a other patchset for these otpimisation.
Regards, Simon
Regards Patrick
Hi Patrick,
On 03.09.2018 15:35, Patrick DELAUNAY wrote:
Hi Simon and Stefan,
Sorry for my late answer (I just come back from my summer holiday)
From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: mercredi 8 août 2018 11:56
On 3 August 2018 at 05:38, Patrick Delaunay patrick.delaunay@st.com wrote:
Add test to avoid access to rx buffer when this buffer is empty. In this case directly call getc() function to avoid issue when tstc() is not called.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
drivers/serial/serial-uclass.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23e..4121a37 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -228,6 +228,9 @@ static int _serial_getc(struct udevice *dev) struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); char val;
if (upriv->rd_ptr == upriv->wr_ptr)return __serial_getc(dev);val = upriv->buf[upriv->rd_ptr++]; upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;-- 2.7.4
Reviewed-by: Simon Glass sjg@chromium.org
BTW I think the code in _serial_tstc() is wrong, or at least sub-optimal:
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
It should call serial_getc() until it returns -EAGAIN. There should be no need to call __serial_tstc() first,
This part had be added by Stefan Roese in SHA1 3ca7a06afb7cbc867b7861a8b9aada74e5bbb559
But I check again the code, and I think the code is correct....
I really hope so. I did test this implementation quite heavily at that time.
but I agree it is not optimal.
we can directly ops->getc(dev) and no more __serial_getc() or __serial_tstc() : the behavior don't change but the access to serial driver is reduced. When no char is available, ops->getc witll return -EAGAIN
static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
If you are OK I will push a other patchset for these otpimisation.
I'm not 100% sure, if this new implementation is "better". Lets compare the code:
Current version: static int _serial_tstc(struct udevice *dev) { struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
New version: static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
The new version has more code and will most likely produce a larger binary. You are removing the calls to the __foo() functions - making the call chain a bit smaller. But this will only affect the performance which is most likely negligible in this case.
I any case, I don't object against any "optimizations" here. But please make sure to test any changes very thorough - please also on platforms with limited RX FIFO sizes.
Thanks, Stefan
Hi Stefan
From: Stefan sr@denx.de Sent: lundi 3 septembre 2018 16:03
Hi Patrick,
On 03.09.2018 15:35, Patrick DELAUNAY wrote:
Hi Simon and Stefan,
Sorry for my late answer (I just come back from my summer holiday)
From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: mercredi 8 août 2018 11:56
On 3 August 2018 at 05:38, Patrick Delaunay patrick.delaunay@st.com
wrote:
Add test to avoid access to rx buffer when this buffer is empty. In this case directly call getc() function to avoid issue when tstc() is not called.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
drivers/serial/serial-uclass.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23e..4121a37 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -228,6 +228,9 @@ static int _serial_getc(struct udevice *dev) struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); char val;
if (upriv->rd_ptr == upriv->wr_ptr)return __serial_getc(dev);val = upriv->buf[upriv->rd_ptr++]; upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;-- 2.7.4
Reviewed-by: Simon Glass sjg@chromium.org
BTW I think the code in _serial_tstc() is wrong, or at least sub-optimal:
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
It should call serial_getc() until it returns -EAGAIN. There should be no need to call __serial_tstc() first,
This part had be added by Stefan Roese in SHA1 3ca7a06afb7cbc867b7861a8b9aada74e5bbb559
But I check again the code, and I think the code is correct....
I really hope so. I did test this implementation quite heavily at that time.
but I agree it is not optimal.
we can directly ops->getc(dev) and no more __serial_getc() or __serial_tstc() : the behavior don't change but the access to serial driver is reduced. When no char is available, ops->getc witll return -EAGAIN
static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
If you are OK I will push a other patchset for these otpimisation.
I'm not 100% sure, if this new implementation is "better". Lets compare the code:
Current version: static int _serial_tstc(struct udevice *dev) { struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
New version: static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
The new version has more code and will most likely produce a larger binary. You are removing the calls to the __foo() functions - making the call chain a bit smaller. But this will only affect the performance which is most likely negligible in this case.
Yes, perhaps a larger code but no more "pending" ops call of serial driver. I only directly use getc ops => that avoid one access to HW register I think.
Than can improve the execution time, but I agree that seems marginal in most the case.
I any case, I don't object against any "optimizations" here. But please make sure to test any changes very thorough - please also on platforms with limited RX FIFO sizes.
Unfortunately I have no platform with limited FIFO size, So I don't known how test it deeply.
And the impact depends also how is implemented the serial driver (gets and pending ops can use several HW register access and is depending on the access time to the IP).
But if you and Simon think that "optimization" is not needed, you can forget this proposal because I think also the gain should be very low. This proposal is only a reaction on the Simon comment (at least sub-optimal)
Thanks, Stefan
Regards Patrick
Hi Patrick,
On 04.09.2018 09:56, Patrick DELAUNAY wrote:
Hi Stefan
From: Stefan sr@denx.de Sent: lundi 3 septembre 2018 16:03
Hi Patrick,
On 03.09.2018 15:35, Patrick DELAUNAY wrote:
Hi Simon and Stefan,
Sorry for my late answer (I just come back from my summer holiday)
From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: mercredi 8 août 2018 11:56
On 3 August 2018 at 05:38, Patrick Delaunay patrick.delaunay@st.com
wrote:
Add test to avoid access to rx buffer when this buffer is empty. In this case directly call getc() function to avoid issue when tstc() is not called.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
drivers/serial/serial-uclass.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23e..4121a37 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -228,6 +228,9 @@ static int _serial_getc(struct udevice *dev) struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); char val;
if (upriv->rd_ptr == upriv->wr_ptr)return __serial_getc(dev);val = upriv->buf[upriv->rd_ptr++]; upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;-- 2.7.4
Reviewed-by: Simon Glass sjg@chromium.org
BTW I think the code in _serial_tstc() is wrong, or at least sub-optimal:
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
It should call serial_getc() until it returns -EAGAIN. There should be no need to call __serial_tstc() first,
This part had be added by Stefan Roese in SHA1 3ca7a06afb7cbc867b7861a8b9aada74e5bbb559
But I check again the code, and I think the code is correct....
I really hope so. I did test this implementation quite heavily at that time.
but I agree it is not optimal.
we can directly ops->getc(dev) and no more __serial_getc() or __serial_tstc() : the behavior don't change but the access to serial driver is reduced. When no char is available, ops->getc witll return -EAGAIN
static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
If you are OK I will push a other patchset for these otpimisation.
I'm not 100% sure, if this new implementation is "better". Lets compare the code:
Current version: static int _serial_tstc(struct udevice *dev) { struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
New version: static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
The new version has more code and will most likely produce a larger binary. You are removing the calls to the __foo() functions - making the call chain a bit smaller. But this will only affect the performance which is most likely negligible in this case.
Yes, perhaps a larger code but no more "pending" ops call of serial driver. I only directly use getc ops => that avoid one access to HW register I think.
Than can improve the execution time, but I agree that seems marginal in most the case.
I any case, I don't object against any "optimizations" here. But please make sure to test any changes very thorough - please also on platforms with limited RX FIFO sizes.
Unfortunately I have no platform with limited FIFO size, So I don't known how test it deeply.
And the impact depends also how is implemented the serial driver (gets and pending ops can use several HW register access and is depending on the access time to the IP).
But if you and Simon think that "optimization" is not needed, you can forget this proposal because I think also the gain should be very low. This proposal is only a reaction on the Simon comment (at least sub-optimal)
I would prefer not to change this code without any real need (bug fix etc). As the current code has undergone many tests and has proven stable - at least for me in all my test cases. And I can't test any changes very easily, as the platform setup will take a while for me.
Thanks, Stefan
Hi Stefan,
From: Stefan sr@denx.de Sent: mardi 4 septembre 2018 14:08
Hi Patrick,
On 04.09.2018 09:56, Patrick DELAUNAY wrote:
Hi Stefan
From: Stefan sr@denx.de Sent: lundi 3 septembre 2018 16:03
Hi Patrick,
On 03.09.2018 15:35, Patrick DELAUNAY wrote:
Hi Simon and Stefan,
Sorry for my late answer (I just come back from my summer holiday)
From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: mercredi 8 août 2018 11:56
On 3 August 2018 at 05:38, Patrick Delaunay patrick.delaunay@st.com
wrote:
Add test to avoid access to rx buffer when this buffer is empty. In this case directly call getc() function to avoid issue when tstc() is not called.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
drivers/serial/serial-uclass.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23e..4121a37 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -228,6 +228,9 @@ static int _serial_getc(struct udevice *dev) struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); char val;
if (upriv->rd_ptr == upriv->wr_ptr)return __serial_getc(dev);val = upriv->buf[upriv->rd_ptr++]; upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;-- 2.7.4
Reviewed-by: Simon Glass sjg@chromium.org
BTW I think the code in _serial_tstc() is wrong, or at least sub-optimal:
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
It should call serial_getc() until it returns -EAGAIN. There should be no need to call __serial_tstc() first,
This part had be added by Stefan Roese in SHA1 3ca7a06afb7cbc867b7861a8b9aada74e5bbb559
But I check again the code, and I think the code is correct....
I really hope so. I did test this implementation quite heavily at that time.
but I agree it is not optimal.
we can directly ops->getc(dev) and no more __serial_getc() or __serial_tstc() : the behavior don't change but the access to serial driver is reduced. When no char is available, ops->getc witll return -EAGAIN
static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
If you are OK I will push a other patchset for these otpimisation.
I'm not 100% sure, if this new implementation is "better". Lets compare the code:
Current version: static int _serial_tstc(struct udevice *dev) { struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);
/* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) { upriv->buf[upriv->wr_ptr++] = __serial_getc(dev); upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
New version: static int _serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev); struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); int err;
/* Read all available chars into the RX buffer */ while(1) { err = ops->getc(dev); if (err < 0) break; upriv->buf[upriv->wr_ptr++] = err; upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0; }
The new version has more code and will most likely produce a larger binary. You are removing the calls to the __foo() functions - making the call chain a bit smaller. But this will only affect the performance which is most likely negligible in this case.
Yes, perhaps a larger code but no more "pending" ops call of serial driver. I only directly use getc ops => that avoid one access to HW register I think.
Than can improve the execution time, but I agree that seems marginal in most
the case.
I any case, I don't object against any "optimizations" here. But please make sure to test any changes very thorough - please also on platforms with limited RX FIFO sizes.
Unfortunately I have no platform with limited FIFO size, So I don't known how
test it deeply.
And the impact depends also how is implemented the serial driver (gets and
pending ops can use several HW register access and is depending on the access time to the IP).
But if you and Simon think that "optimization" is not needed, you can forget
this proposal because I think also the gain should be very low.
This proposal is only a reaction on the Simon comment (at least sub-optimal)
I would prefer not to change this code without any real need (bug fix etc). As the current code has undergone many tests and has proven stable - at least for me in all my test cases. And I can't test any changes very easily, as the platform setup will take a while for me.
Ok thanks for the feedback, so I forget my "optimization" proposal. But the fist patch to protect the serial rx buffer access is needed (the initial patch reviewed by Simon).
Thanks, Stefan
Regards Patrick
On Fri, Aug 03, 2018 at 01:38:43PM +0200, Patrick Delaunay wrote:
Add test to avoid access to rx buffer when this buffer is empty. In this case directly call getc() function to avoid issue when tstc() is not called.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
Unify the fgetc function when MUX is activated or not: - always call tstc() : it is the normal behavior expected by serial uclass (call tstc then getc) and that avoids issue when SERIAL_RX_BUFFER is activated - reload WATCHDOG in the char waiting loop
This patch allow to have the same behavior when CONSOLE_MUX is activated or not and avoid regression when this feature is deactivated.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
common/console.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/common/console.c b/common/console.c index 7aa58d0..9a94f32 100644 --- a/common/console.c +++ b/common/console.c @@ -311,12 +311,12 @@ int serial_printf(const char *fmt, ...) int fgetc(int file) { if (file < MAX_FILES) { -#if CONFIG_IS_ENABLED(CONSOLE_MUX) /* * Effectively poll for input wherever it may be available. */ for (;;) { WATCHDOG_RESET(); +#if CONFIG_IS_ENABLED(CONSOLE_MUX) /* * Upper layer may have already called tstc() so * check for that first. @@ -324,6 +324,10 @@ int fgetc(int file) if (tstcdev != NULL) return console_getc(file); console_tstc(file); +#else + if (console_tstc(file)) + return console_getc(file); +#endif #ifdef CONFIG_WATCHDOG /* * If the watchdog must be rate-limited then it should @@ -332,9 +336,6 @@ int fgetc(int file) udelay(1); #endif } -#else - return console_getc(file); -#endif }
return -1;
On 3 August 2018 at 05:38, Patrick Delaunay patrick.delaunay@st.com wrote:
Unify the fgetc function when MUX is activated or not:
- always call tstc() : it is the normal behavior expected by serial uclass (call tstc then getc) and that avoids issue when SERIAL_RX_BUFFER is activated
- reload WATCHDOG in the char waiting loop
This patch allow to have the same behavior when CONSOLE_MUX is activated or not and avoid regression when this feature is deactivated.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
common/console.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
I'm not sure how to test this with the various cases...
Hi Simon,
From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: mercredi 8 août 2018 11:56 To: Patrick DELAUNAY patrick.delaunay@st.com On 3 August 2018 at 05:38, Patrick Delaunay patrick.delaunay@st.com wrote:
Unify the fgetc function when MUX is activated or not:
- always call tstc() : it is the normal behavior expected by serial uclass (call tstc then getc) and that avoids issue when SERIAL_RX_BUFFER is activated
- reload WATCHDOG in the char waiting loop
This patch allow to have the same behavior when CONSOLE_MUX is activated or not and avoid regression when this feature is deactivated.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
common/console.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
I'm not sure how to test this with the various cases...
Yes it is a difficully and I don't know the process in this case, but at least the behavior (testc() function call) will be shared.
If this change seens too risky, it can be dropped as only the PATCH 1/4 is mandatory to solve my issue.
Regards, Patrick
On Fri, Aug 03, 2018 at 01:38:44PM +0200, Patrick Delaunay wrote:
Unify the fgetc function when MUX is activated or not:
- always call tstc() : it is the normal behavior expected by serial uclass (call tstc then getc) and that avoids issue when SERIAL_RX_BUFFER is activated
- reload WATCHDOG in the char waiting loop
This patch allow to have the same behavior when CONSOLE_MUX is activated or not and avoid regression when this feature is deactivated.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
Handle getch error (when getch return 0x0) to avoid display issue in the console.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
common/cli_readline.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/common/cli_readline.c b/common/cli_readline.c index 60a232b..99b6317 100644 --- a/common/cli_readline.c +++ b/common/cli_readline.c @@ -273,6 +273,10 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len,
ichar = getcmd_getch();
+ /* ichar=0x0 when error occurs in U-Boot getc */ + if (!ichar) + continue; + if ((ichar == '\n') || (ichar == '\r')) { putc('\n'); break;
On Fri, Aug 03, 2018 at 01:38:45PM +0200, Patrick Delaunay wrote:
Handle getch error (when getch return 0x0) to avoid display issue in the console.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Applied to u-boot/master, thanks!
participants (5)
-
Patrick DELAUNAY -
Patrick Delaunay
-
Simon Glass -
Stefan -
Tom Rini