[U-Boot] [PATCH 0/4] Solve issue with serial rx buffer when MUX is deactivated

[U-Boot] [PATCH 0/4] Solve issue with serial rx buffer when MUX is deactivated

[Patrick Delaunay]

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(-)

-- 
2.7.4

[U-Boot] [PATCH 1/4] stm32mp1: activate serial rx buffer

[Patrick Delaunay]

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
-- 
2.7.4

[U-Boot] [U-Boot,1/4] stm32mp1: activate serial rx buffer

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180911/c759c703/attachment.sig>

[U-Boot] [PATCH 2/4] serial: protect access to serial rx buffer

[Patrick Delaunay]

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

[U-Boot] [PATCH 2/4] serial: protect access to serial rx buffer

[Simon Glass]

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

[U-Boot] [PATCH 2/4] serial: protect access to serial rx buffer

[Patrick DELAUNAY]

Hi Simon and Stefan,

Sorry for my late answer (I just come back from my summer holiday)

> From: sjg at 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

[U-Boot] [PATCH 2/4] serial: protect access to serial rx buffer

[Stefan]

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 at 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

[U-Boot] [PATCH 2/4] serial: protect access to serial rx buffer

[Patrick DELAUNAY]

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 at 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 

[U-Boot] [PATCH 2/4] serial: protect access to serial rx buffer

[Stefan]

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 at 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

[U-Boot] [PATCH 2/4] serial: protect access to serial rx buffer

[Patrick DELAUNAY]

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 at 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

[U-Boot] [U-Boot, 2/4] serial: protect access to serial rx buffer

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180911/ec64cd2f/attachment.sig>

[U-Boot] [PATCH 3/4] console: unify fgetc function when console MUX is deactivated

[Patrick Delaunay]

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;
-- 
2.7.4

[U-Boot] [PATCH 3/4] console: unify fgetc function when console MUX is deactivated

[Simon Glass]

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...

[U-Boot] [PATCH 3/4] console: unify fgetc function when console MUX is deactivated

[Patrick DELAUNAY]

Hi Simon,

> From: sjg at 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

[U-Boot] [U-Boot, 3/4] console: unify fgetc function when console MUX is deactivated

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180911/0e8cd078/attachment.sig>

[U-Boot] [PATCH 4/4] cli: handle getch error

[Patrick Delaunay]

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;
-- 
2.7.4

[U-Boot] [U-Boot,4/4] cli: handle getch error

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180911/107eb1bb/attachment.sig>