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