[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[Lukasz Majewski]

It may happen that the serial IP block is performing some ongoing
transmission (started at e.g. board_init()) when the serial "probe" is
called.

As a result the serial port IP block is reset, so transmitted data is
corrupted:

    I2C:   ready
    DRAM:  1 GiB
    jSS('HH��SL_SDHC: 04 rev 0x0

This patch prevents from this situation, by defining pre_probe() callback
in which we wait till the TX buffer is empty (from previous transmission):

    I2C:   ready
    DRAM:  1 GiB
    ID:    unit type 0x4 rev 0x0

All defined ->pending callbacks at ./drivers/serial are non blocking
- just simple reading from registers and testing flags. Hence, it should
be enough to not use any timeout from timer.
One shall also note that we enable console very early - not all timers
may be ready for work - adding timeout here would impose implicit
dependency that timers are setup before serial.


Signed-off-by: Lukasz Majewski <lukma@denx.de>
---

 drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 2e5116f..5e6964d 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -420,10 +420,30 @@ static int serial_pre_remove(struct udevice *dev)
 	return 0;
 }
 
+static int serial_pre_probe(struct udevice *dev)
+{
+	struct dm_serial_ops *ops = serial_get_ops(dev);
+	int ret = 0;
+
+	/*
+	 * Wait for any ongoing transmission to finish - for example
+	 * from pre-relocation enabled UART
+	 */
+	if (ops && ops->pending)
+		do {
+			ret = ops->pending(dev, false);
+			if (ret < 0)
+				break;
+		} while (ret > 0);
+
+	return ret;
+}
+
 UCLASS_DRIVER(serial) = {
 	.id		= UCLASS_SERIAL,
 	.name		= "serial",
 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
+	.pre_probe	= serial_pre_probe,
 	.post_probe	= serial_post_probe,
 	.pre_remove	= serial_pre_remove,
 	.per_device_auto_alloc_size = sizeof(struct serial_dev_priv),
-- 
2.1.4

[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[Dr. Philipp Tomsich]

> On 27 Oct 2017, at 00:13, Lukasz Majewski <lukma@denx.de> wrote:
> 
> It may happen that the serial IP block is performing some ongoing
> transmission (started at e.g. board_init()) when the serial "probe" is
> called.
> 
> As a result the serial port IP block is reset, so transmitted data is
> corrupted:
> 
>    I2C:   ready
>    DRAM:  1 GiB
>    jSS('HH��SL_SDHC: 04 rev 0x0
> 
> This patch prevents from this situation, by defining pre_probe() callback
> in which we wait till the TX buffer is empty (from previous transmission):
> 
>    I2C:   ready
>    DRAM:  1 GiB
>    ID:    unit type 0x4 rev 0x0
> 
> All defined ->pending callbacks at ./drivers/serial are non blocking
> - just simple reading from registers and testing flags. Hence, it should
> be enough to not use any timeout from timer.
> One shall also note that we enable console very early - not all timers
> may be ready for work - adding timeout here would impose implicit
> dependency that timers are setup before serial.

Given that this is effectively a busy polling loop, why can’t this be done
from the probe-function of serial drivers that require this functionality?

> 
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
> 
> drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 2e5116f..5e6964d 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -420,10 +420,30 @@ static int serial_pre_remove(struct udevice *dev)
> 	return 0;
> }
> 
> +static int serial_pre_probe(struct udevice *dev)
> +{
> +	struct dm_serial_ops *ops = serial_get_ops(dev);
> +	int ret = 0;
> +
> +	/*
> +	 * Wait for any ongoing transmission to finish - for example
> +	 * from pre-relocation enabled UART
> +	 */
> +	if (ops && ops->pending)
> +		do {
> +			ret = ops->pending(dev, false);
> +			if (ret < 0)
> +				break;
> +		} while (ret > 0);
> +
> +	return ret;
> +}
> +
> UCLASS_DRIVER(serial) = {
> 	.id		= UCLASS_SERIAL,
> 	.name		= "serial",
> 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
> +	.pre_probe	= serial_pre_probe,
> 	.post_probe	= serial_post_probe,
> 	.pre_remove	= serial_pre_remove,
> 	.per_device_auto_alloc_size = sizeof(struct serial_dev_priv),
> -- 
> 2.1.4
> 

[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[Lukasz Majewski]

Hi Philipp,

> 
> > On 27 Oct 2017, at 00:13, Lukasz Majewski <lukma@denx.de> wrote:
> > 
> > It may happen that the serial IP block is performing some ongoing
> > transmission (started at e.g. board_init()) when the serial "probe"
> > is called.
> > 
> > As a result the serial port IP block is reset, so transmitted data
> > is corrupted:
> > 
> >    I2C:   ready
> >    DRAM:  1 GiB
> >    jSS('HH��SL_SDHC: 04 rev 0x0
> > 
> > This patch prevents from this situation, by defining pre_probe()
> > callback in which we wait till the TX buffer is empty (from
> > previous transmission):
> > 
> >    I2C:   ready
> >    DRAM:  1 GiB
> >    ID:    unit type 0x4 rev 0x0
> > 
> > All defined ->pending callbacks at ./drivers/serial are non blocking
> > - just simple reading from registers and testing flags. Hence, it
> > should be enough to not use any timeout from timer.
> > One shall also note that we enable console very early - not all
> > timers may be ready for work - adding timeout here would impose
> > implicit dependency that timers are setup before serial.
> 
> Given that this is effectively a busy polling loop, why can’t this be
> done from the probe-function of serial drivers that require this
> functionality?

That would be one of the options. 

Originally, this code was placed at iMX specific function - namely
_mxc_serial_init() [1]. 

However, Simon suggested to solve this problem globally via DM's
serial-uclass, which offers pre_probe() callback for such purpose.

The problem here is the polling loop. I've checked and ->pending on
real SoCs is just a read from the register - which should not block (a
similar approach is used in Linux kernel).

Having timeout from timer would impose dependency on timer init'ed
first - before serial.

[1] - https://patchwork.ozlabs.org/patch/820824/

> 
> > 
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> > 
> > drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/serial/serial-uclass.c
> > b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
> > --- a/drivers/serial/serial-uclass.c
> > +++ b/drivers/serial/serial-uclass.c
> > @@ -420,10 +420,30 @@ static int serial_pre_remove(struct udevice
> > *dev) return 0;
> > }
> > 
> > +static int serial_pre_probe(struct udevice *dev)
> > +{
> > +	struct dm_serial_ops *ops = serial_get_ops(dev);
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * Wait for any ongoing transmission to finish - for
> > example
> > +	 * from pre-relocation enabled UART
> > +	 */
> > +	if (ops && ops->pending)
> > +		do {
> > +			ret = ops->pending(dev, false);
> > +			if (ret < 0)
> > +				break;
> > +		} while (ret > 0);
> > +
> > +	return ret;
> > +}
> > +
> > UCLASS_DRIVER(serial) = {
> > 	.id		= UCLASS_SERIAL,
> > 	.name		= "serial",
> > 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
> > +	.pre_probe	= serial_pre_probe,
> > 	.post_probe	= serial_post_probe,
> > 	.pre_remove	= serial_pre_remove,
> > 	.per_device_auto_alloc_size = sizeof(struct
> > serial_dev_priv), -- 
> > 2.1.4
> > 
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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

[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[Dr. Philipp Tomsich]

Hi Lukasz,

> On 27 Oct 2017, at 00:51, Lukasz Majewski <lukma@denx.de> wrote:
>> 
>>> On 27 Oct 2017, at 00:13, Lukasz Majewski <lukma@denx.de> wrote:
>>> 
>>> It may happen that the serial IP block is performing some ongoing
>>> transmission (started at e.g. board_init()) when the serial "probe"
>>> is called.
>>> 
>>> As a result the serial port IP block is reset, so transmitted data
>>> is corrupted:
>>> 
>>>   I2C:   ready
>>>   DRAM:  1 GiB
>>>   jSS('HH��SL_SDHC: 04 rev 0x0
>>> 
>>> This patch prevents from this situation, by defining pre_probe()
>>> callback in which we wait till the TX buffer is empty (from
>>> previous transmission):
>>> 
>>>   I2C:   ready
>>>   DRAM:  1 GiB
>>>   ID:    unit type 0x4 rev 0x0
>>> 
>>> All defined ->pending callbacks at ./drivers/serial are non blocking
>>> - just simple reading from registers and testing flags. Hence, it
>>> should be enough to not use any timeout from timer.
>>> One shall also note that we enable console very early - not all
>>> timers may be ready for work - adding timeout here would impose
>>> implicit dependency that timers are setup before serial.
>> 
>> Given that this is effectively a busy polling loop, why can’t this be
>> done from the probe-function of serial drivers that require this
>> functionality?
> 
> That would be one of the options. 
> 
> Originally, this code was placed at iMX specific function - namely
> _mxc_serial_init() [1]. 
> 
> However, Simon suggested to solve this problem globally via DM's
> serial-uclass, which offers pre_probe() callback for such purpose.
> 
> The problem here is the polling loop. I've checked and ->pending on
> real SoCs is just a read from the register - which should not block (a
> similar approach is used in Linux kernel).
> 
> Having timeout from timer would impose dependency on timer init'ed
> first - before serial.

I worry a bit about the pending-callback being called prior to a probe.
As of today, the pending function can assume that probe() has run
and initialised the private structures accordingly—with this change,
that assumption is invalidated.

If we go down this path, then we need to clearly indicate that the
pending function can not rely on probe() to have initialised the device
state or private data structures.  If we keep this check within the
probe()-function, it should be obvious what is set up and what isn’t.

> 
> [1] - https://patchwork.ozlabs.org/patch/820824/
> 
>> 
>>> 
>>> 
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>> ---
>>> 
>>> drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>> 
>>> diff --git a/drivers/serial/serial-uclass.c
>>> b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
>>> --- a/drivers/serial/serial-uclass.c
>>> +++ b/drivers/serial/serial-uclass.c
>>> @@ -420,10 +420,30 @@ static int serial_pre_remove(struct udevice
>>> *dev) return 0;
>>> }
>>> 
>>> +static int serial_pre_probe(struct udevice *dev)
>>> +{
>>> +	struct dm_serial_ops *ops = serial_get_ops(dev);
>>> +	int ret = 0;
>>> +
>>> +	/*
>>> +	 * Wait for any ongoing transmission to finish - for
>>> example
>>> +	 * from pre-relocation enabled UART
>>> +	 */
>>> +	if (ops && ops->pending)
>>> +		do {
>>> +			ret = ops->pending(dev, false);
>>> +			if (ret < 0)
>>> +				break;
>>> +		} while (ret > 0);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> UCLASS_DRIVER(serial) = {
>>> 	.id		= UCLASS_SERIAL,
>>> 	.name		= "serial",
>>> 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
>>> +	.pre_probe	= serial_pre_probe,
>>> 	.post_probe	= serial_post_probe,
>>> 	.pre_remove	= serial_pre_remove,
>>> 	.per_device_auto_alloc_size = sizeof(struct
>>> serial_dev_priv), -- 
>>> 2.1.4
>>> 
>> 
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> 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

[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[Lukasz Majewski]

Hi Philipp,

> Hi Lukasz,
> 
> > On 27 Oct 2017, at 00:51, Lukasz Majewski <lukma@denx.de> wrote:
> >> 
> >>> On 27 Oct 2017, at 00:13, Lukasz Majewski <lukma@denx.de> wrote:
> >>> 
> >>> It may happen that the serial IP block is performing some ongoing
> >>> transmission (started at e.g. board_init()) when the serial
> >>> "probe" is called.
> >>> 
> >>> As a result the serial port IP block is reset, so transmitted data
> >>> is corrupted:
> >>> 
> >>>   I2C:   ready
> >>>   DRAM:  1 GiB
> >>>   jSS('HH��SL_SDHC: 04 rev 0x0
> >>> 
> >>> This patch prevents from this situation, by defining pre_probe()
> >>> callback in which we wait till the TX buffer is empty (from
> >>> previous transmission):
> >>> 
> >>>   I2C:   ready
> >>>   DRAM:  1 GiB
> >>>   ID:    unit type 0x4 rev 0x0
> >>> 
> >>> All defined ->pending callbacks at ./drivers/serial are non
> >>> blocking
> >>> - just simple reading from registers and testing flags. Hence, it
> >>> should be enough to not use any timeout from timer.
> >>> One shall also note that we enable console very early - not all
> >>> timers may be ready for work - adding timeout here would impose
> >>> implicit dependency that timers are setup before serial.
> >> 
> >> Given that this is effectively a busy polling loop, why can’t this
> >> be done from the probe-function of serial drivers that require this
> >> functionality?
> > 
> > That would be one of the options. 
> > 
> > Originally, this code was placed at iMX specific function - namely
> > _mxc_serial_init() [1]. 
> > 
> > However, Simon suggested to solve this problem globally via DM's
> > serial-uclass, which offers pre_probe() callback for such purpose.
> > 
> > The problem here is the polling loop. I've checked and ->pending on
> > real SoCs is just a read from the register - which should not block
> > (a similar approach is used in Linux kernel).
> > 
> > Having timeout from timer would impose dependency on timer init'ed
> > first - before serial.
> 
> I worry a bit about the pending-callback being called prior to a
> probe. As of today, the pending function can assume that probe() has
> run and initialised the private structures accordingly—with this
> change, that assumption is invalidated.

Yes. I agree.

Other issue is that we may need timer based timeout for some SoCs. And
this also implies to have timer initialized before the console.

> 
> If we go down this path, then we need to clearly indicate that the
> pending function can not rely on probe() to have initialised the
> device state or private data structures.  If we keep this check
> within the probe()-function, it should be obvious what is set up and
> what isn’t.

Those are valid arguments.

I would even go further and leave this patch as it was in the first
version [1] - since:

- It is similar to what Linux does
- for iMX it uses check on HW bit which is guarantee to be cleared in
  some point of time (of course if HW is not broken).

However, lets wait for input from Simon - how he would like to tackle
this issue.

> 
> > 
> > [1] - https://patchwork.ozlabs.org/patch/820824/
> > 
> >> 
> >>> 
> >>> 
> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >>> ---
> >>> 
> >>> drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
> >>> 1 file changed, 20 insertions(+)
> >>> 
> >>> diff --git a/drivers/serial/serial-uclass.c
> >>> b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
> >>> --- a/drivers/serial/serial-uclass.c
> >>> +++ b/drivers/serial/serial-uclass.c
> >>> @@ -420,10 +420,30 @@ static int serial_pre_remove(struct udevice
> >>> *dev) return 0;
> >>> }
> >>> 
> >>> +static int serial_pre_probe(struct udevice *dev)
> >>> +{
> >>> +	struct dm_serial_ops *ops = serial_get_ops(dev);
> >>> +	int ret = 0;
> >>> +
> >>> +	/*
> >>> +	 * Wait for any ongoing transmission to finish - for
> >>> example
> >>> +	 * from pre-relocation enabled UART
> >>> +	 */
> >>> +	if (ops && ops->pending)
> >>> +		do {
> >>> +			ret = ops->pending(dev, false);
> >>> +			if (ret < 0)
> >>> +				break;
> >>> +		} while (ret > 0);
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>> UCLASS_DRIVER(serial) = {
> >>> 	.id		= UCLASS_SERIAL,
> >>> 	.name		= "serial",
> >>> 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
> >>> +	.pre_probe	= serial_pre_probe,
> >>> 	.post_probe	= serial_post_probe,
> >>> 	.pre_remove	= serial_pre_remove,
> >>> 	.per_device_auto_alloc_size = sizeof(struct
> >>> serial_dev_priv), -- 
> >>> 2.1.4
> >>> 
> >> 
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk 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
> 



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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

[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[Lukasz Majewski]

Dear All,

> Hi Philipp,
> 
> > Hi Lukasz,
> >   
> > > On 27 Oct 2017, at 00:51, Lukasz Majewski <lukma@denx.de> wrote:  
> > >>   
> > >>> On 27 Oct 2017, at 00:13, Lukasz Majewski <lukma@denx.de> wrote:
> > >>> 
> > >>> It may happen that the serial IP block is performing some
> > >>> ongoing transmission (started at e.g. board_init()) when the
> > >>> serial "probe" is called.
> > >>> 
> > >>> As a result the serial port IP block is reset, so transmitted
> > >>> data is corrupted:
> > >>> 
> > >>>   I2C:   ready
> > >>>   DRAM:  1 GiB
> > >>>   jSS('HH��SL_SDHC: 04 rev 0x0
> > >>> 
> > >>> This patch prevents from this situation, by defining pre_probe()
> > >>> callback in which we wait till the TX buffer is empty (from
> > >>> previous transmission):
> > >>> 
> > >>>   I2C:   ready
> > >>>   DRAM:  1 GiB
> > >>>   ID:    unit type 0x4 rev 0x0
> > >>> 
> > >>> All defined ->pending callbacks at ./drivers/serial are non
> > >>> blocking
> > >>> - just simple reading from registers and testing flags. Hence,
> > >>> it should be enough to not use any timeout from timer.
> > >>> One shall also note that we enable console very early - not all
> > >>> timers may be ready for work - adding timeout here would impose
> > >>> implicit dependency that timers are setup before serial.  
> > >> 
> > >> Given that this is effectively a busy polling loop, why can’t
> > >> this be done from the probe-function of serial drivers that
> > >> require this functionality?  
> > > 
> > > That would be one of the options. 
> > > 
> > > Originally, this code was placed at iMX specific function - namely
> > > _mxc_serial_init() [1]. 
> > > 
> > > However, Simon suggested to solve this problem globally via DM's
> > > serial-uclass, which offers pre_probe() callback for such purpose.
> > > 
> > > The problem here is the polling loop. I've checked and ->pending
> > > on real SoCs is just a read from the register - which should not
> > > block (a similar approach is used in Linux kernel).
> > > 
> > > Having timeout from timer would impose dependency on timer init'ed
> > > first - before serial.  
> > 
> > I worry a bit about the pending-callback being called prior to a
> > probe. As of today, the pending function can assume that probe() has
> > run and initialised the private structures accordingly—with this
> > change, that assumption is invalidated.  
> 
> Yes. I agree.
> 
> Other issue is that we may need timer based timeout for some SoCs. And
> this also implies to have timer initialized before the console.
> 
> > 
> > If we go down this path, then we need to clearly indicate that the
> > pending function can not rely on probe() to have initialised the
> > device state or private data structures.  If we keep this check
> > within the probe()-function, it should be obvious what is set up and
> > what isn’t.  
> 
> Those are valid arguments.
> 
> I would even go further and leave this patch as it was in the first
> version [1] - since:
> 
> - It is similar to what Linux does
> - for iMX it uses check on HW bit which is guarantee to be cleared in
>   some point of time (of course if HW is not broken).
> 
> However, lets wait for input from Simon - how he would like to tackle
> this issue.

Unfortunately, I did not received any feedback from Simon.

Hence, I would opt for discarding this particular patch and use the v1
of it (please find the rationale above):

https://patchwork.ozlabs.org/patch/820824/

Has anybody disagree?

> 
> >   
> > > 
> > > [1] - https://patchwork.ozlabs.org/patch/820824/
> > >   
> > >>   
> > >>> 
> > >>> 
> > >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > >>> ---
> > >>> 
> > >>> drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
> > >>> 1 file changed, 20 insertions(+)
> > >>> 
> > >>> diff --git a/drivers/serial/serial-uclass.c
> > >>> b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
> > >>> --- a/drivers/serial/serial-uclass.c
> > >>> +++ b/drivers/serial/serial-uclass.c
> > >>> @@ -420,10 +420,30 @@ static int serial_pre_remove(struct
> > >>> udevice *dev) return 0;
> > >>> }
> > >>> 
> > >>> +static int serial_pre_probe(struct udevice *dev)
> > >>> +{
> > >>> +	struct dm_serial_ops *ops = serial_get_ops(dev);
> > >>> +	int ret = 0;
> > >>> +
> > >>> +	/*
> > >>> +	 * Wait for any ongoing transmission to finish - for
> > >>> example
> > >>> +	 * from pre-relocation enabled UART
> > >>> +	 */
> > >>> +	if (ops && ops->pending)
> > >>> +		do {
> > >>> +			ret = ops->pending(dev, false);
> > >>> +			if (ret < 0)
> > >>> +				break;
> > >>> +		} while (ret > 0);
> > >>> +
> > >>> +	return ret;
> > >>> +}
> > >>> +
> > >>> UCLASS_DRIVER(serial) = {
> > >>> 	.id		= UCLASS_SERIAL,
> > >>> 	.name		= "serial",
> > >>> 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
> > >>> +	.pre_probe	= serial_pre_probe,
> > >>> 	.post_probe	= serial_post_probe,
> > >>> 	.pre_remove	= serial_pre_remove,
> > >>> 	.per_device_auto_alloc_size = sizeof(struct
> > >>> serial_dev_priv), -- 
> > >>> 2.1.4
> > >>>   
> > >>   
> > > 
> > > 
> > > 
> > > 
> > > Best regards,
> > > 
> > > Lukasz Majewski
> > > 
> > > --
> > > 
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > Denk 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  
> >   
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> 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



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171104/d0d2606d/attachment.sig>

[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[Simon Glass]

Hi Lukasz,

On 4 November 2017 at 15:42, Lukasz Majewski <lukma@denx.de> wrote:
>
> Dear All,
>
> > Hi Philipp,
> >
> > > Hi Lukasz,
> > >
> > > > On 27 Oct 2017, at 00:51, Lukasz Majewski <lukma@denx.de> wrote:
> > > >>
> > > >>> On 27 Oct 2017, at 00:13, Lukasz Majewski <lukma@denx.de> wrote:
> > > >>>
> > > >>> It may happen that the serial IP block is performing some
> > > >>> ongoing transmission (started at e.g. board_init()) when the
> > > >>> serial "probe" is called.
> > > >>>
> > > >>> As a result the serial port IP block is reset, so transmitted
> > > >>> data is corrupted:
> > > >>>
> > > >>>   I2C:   ready
> > > >>>   DRAM:  1 GiB
> > > >>>   jSS('HH��SL_SDHC: 04 rev 0x0
> > > >>>
> > > >>> This patch prevents from this situation, by defining pre_probe()
> > > >>> callback in which we wait till the TX buffer is empty (from
> > > >>> previous transmission):
> > > >>>
> > > >>>   I2C:   ready
> > > >>>   DRAM:  1 GiB
> > > >>>   ID:    unit type 0x4 rev 0x0
> > > >>>
> > > >>> All defined ->pending callbacks at ./drivers/serial are non
> > > >>> blocking
> > > >>> - just simple reading from registers and testing flags. Hence,
> > > >>> it should be enough to not use any timeout from timer.
> > > >>> One shall also note that we enable console very early - not all
> > > >>> timers may be ready for work - adding timeout here would impose
> > > >>> implicit dependency that timers are setup before serial.
> > > >>
> > > >> Given that this is effectively a busy polling loop, why can’t
> > > >> this be done from the probe-function of serial drivers that
> > > >> require this functionality?
> > > >
> > > > That would be one of the options.
> > > >
> > > > Originally, this code was placed at iMX specific function - namely
> > > > _mxc_serial_init() [1].
> > > >
> > > > However, Simon suggested to solve this problem globally via DM's
> > > > serial-uclass, which offers pre_probe() callback for such purpose.
> > > >
> > > > The problem here is the polling loop. I've checked and ->pending
> > > > on real SoCs is just a read from the register - which should not
> > > > block (a similar approach is used in Linux kernel).
> > > >
> > > > Having timeout from timer would impose dependency on timer init'ed
> > > > first - before serial.
> > >
> > > I worry a bit about the pending-callback being called prior to a
> > > probe. As of today, the pending function can assume that probe() has
> > > run and initialised the private structures accordingly—with this
> > > change, that assumption is invalidated.
> >
> > Yes. I agree.
> >
> > Other issue is that we may need timer based timeout for some SoCs. And
> > this also implies to have timer initialized before the console.
> >
> > >
> > > If we go down this path, then we need to clearly indicate that the
> > > pending function can not rely on probe() to have initialised the
> > > device state or private data structures.  If we keep this check
> > > within the probe()-function, it should be obvious what is set up and
> > > what isn’t.
> >
> > Those are valid arguments.
> >
> > I would even go further and leave this patch as it was in the first
> > version [1] - since:
> >
> > - It is similar to what Linux does
> > - for iMX it uses check on HW bit which is guarantee to be cleared in
> >   some point of time (of course if HW is not broken).
> >
> > However, lets wait for input from Simon - how he would like to tackle
> > this issue.
>
> Unfortunately, I did not received any feedback from Simon.
>
> Hence, I would opt for discarding this particular patch and use the v1
> of it (please find the rationale above):
>
> https://patchwork.ozlabs.org/patch/820824/
>
> Has anybody disagree?

I think this patch is useful, but I agree it needs to be documented.

You could add a comment to the pending function in serial.h saying it
can be called before probe. This would only really work if the driver
was inited previously though.

Would it be better instead to have a function in board_f.c somewhere
which loops on each serial device waiting for the pending function to
return 0? Then you know that the driver has been probed, and before
relocation too, so it should be safe.

I have seen this problem on other boards so I do favour a generic
solution if we can figure this out.

Regards,
Simon

>
> >
> > >
> > > >
> > > > [1] - https://patchwork.ozlabs.org/patch/820824/
> > > >
> > > >>
> > > >>>
> > > >>>
> > > >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > > >>> ---
> > > >>>
> > > >>> drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
> > > >>> 1 file changed, 20 insertions(+)
> > > >>>
> > > >>> diff --git a/drivers/serial/serial-uclass.c
> > > >>> b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
> > > >>> --- a/drivers/serial/serial-uclass.c
> > > >>> +++ b/drivers/serial/serial-uclass.c
> > > >>> @@ -420,10 +420,30 @@ static int serial_pre_remove(struct
> > > >>> udevice *dev) return 0;
> > > >>> }
> > > >>>
> > > >>> +static int serial_pre_probe(struct udevice *dev)
> > > >>> +{
> > > >>> +       struct dm_serial_ops *ops = serial_get_ops(dev);
> > > >>> +       int ret = 0;
> > > >>> +
> > > >>> +       /*
> > > >>> +        * Wait for any ongoing transmission to finish - for
> > > >>> example
> > > >>> +        * from pre-relocation enabled UART
> > > >>> +        */
> > > >>> +       if (ops && ops->pending)
> > > >>> +               do {
> > > >>> +                       ret = ops->pending(dev, false);
> > > >>> +                       if (ret < 0)
> > > >>> +                               break;
> > > >>> +               } while (ret > 0);
> > > >>> +
> > > >>> +       return ret;
> > > >>> +}
> > > >>> +
> > > >>> UCLASS_DRIVER(serial) = {
> > > >>>         .id             = UCLASS_SERIAL,
> > > >>>         .name           = "serial",
> > > >>>         .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > >>> +       .pre_probe      = serial_pre_probe,
> > > >>>         .post_probe     = serial_post_probe,
> > > >>>         .pre_remove     = serial_pre_remove,
> > > >>>         .per_device_auto_alloc_size = sizeof(struct
> > > >>> serial_dev_priv), --
> > > >>> 2.1.4
> > > >>>
> > > >>
> > > >
> > > >
> > > >
> > > >
> > > > Best regards,
> > > >
> > > > Lukasz Majewski
> > > >
> > > > --
> > > >
> > > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > > Denk 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
> > >
> >
> >
> >
> > Best regards,
> >
> > Lukasz Majewski
> >
> > --
> >
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> > 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
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> 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

[U-Boot] [PATCH] dm: serial: Add .pre_probe() function to to wait for previous transmission end

[Lukasz Majewski]

Hi Simon,

> Hi Lukasz,
> 
> On 4 November 2017 at 15:42, Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Dear All,
> >  
> > > Hi Philipp,
> > >  
> > > > Hi Lukasz,
> > > >  
> > > > > On 27 Oct 2017, at 00:51, Lukasz Majewski <lukma@denx.de>
> > > > > wrote:  
> > > > >>  
> > > > >>> On 27 Oct 2017, at 00:13, Lukasz Majewski <lukma@denx.de>
> > > > >>> wrote:
> > > > >>>
> > > > >>> It may happen that the serial IP block is performing some
> > > > >>> ongoing transmission (started at e.g. board_init()) when the
> > > > >>> serial "probe" is called.
> > > > >>>
> > > > >>> As a result the serial port IP block is reset, so
> > > > >>> transmitted data is corrupted:
> > > > >>>
> > > > >>>   I2C:   ready
> > > > >>>   DRAM:  1 GiB
> > > > >>>   jSS('HH��SL_SDHC: 04 rev 0x0
> > > > >>>
> > > > >>> This patch prevents from this situation, by defining
> > > > >>> pre_probe() callback in which we wait till the TX buffer is
> > > > >>> empty (from previous transmission):
> > > > >>>
> > > > >>>   I2C:   ready
> > > > >>>   DRAM:  1 GiB
> > > > >>>   ID:    unit type 0x4 rev 0x0
> > > > >>>
> > > > >>> All defined ->pending callbacks at ./drivers/serial are non
> > > > >>> blocking
> > > > >>> - just simple reading from registers and testing flags.
> > > > >>> Hence, it should be enough to not use any timeout from
> > > > >>> timer. One shall also note that we enable console very
> > > > >>> early - not all timers may be ready for work - adding
> > > > >>> timeout here would impose implicit dependency that timers
> > > > >>> are setup before serial.  
> > > > >>
> > > > >> Given that this is effectively a busy polling loop, why can’t
> > > > >> this be done from the probe-function of serial drivers that
> > > > >> require this functionality?  
> > > > >
> > > > > That would be one of the options.
> > > > >
> > > > > Originally, this code was placed at iMX specific function -
> > > > > namely _mxc_serial_init() [1].
> > > > >
> > > > > However, Simon suggested to solve this problem globally via
> > > > > DM's serial-uclass, which offers pre_probe() callback for
> > > > > such purpose.
> > > > >
> > > > > The problem here is the polling loop. I've checked and
> > > > > ->pending on real SoCs is just a read from the register -
> > > > > which should not block (a similar approach is used in Linux
> > > > > kernel).
> > > > >
> > > > > Having timeout from timer would impose dependency on timer
> > > > > init'ed first - before serial.  
> > > >
> > > > I worry a bit about the pending-callback being called prior to a
> > > > probe. As of today, the pending function can assume that
> > > > probe() has run and initialised the private structures
> > > > accordingly—with this change, that assumption is invalidated.  
> > >
> > > Yes. I agree.
> > >
> > > Other issue is that we may need timer based timeout for some
> > > SoCs. And this also implies to have timer initialized before the
> > > console. 
> > > >
> > > > If we go down this path, then we need to clearly indicate that
> > > > the pending function can not rely on probe() to have
> > > > initialised the device state or private data structures.  If we
> > > > keep this check within the probe()-function, it should be
> > > > obvious what is set up and what isn’t.  
> > >
> > > Those are valid arguments.
> > >
> > > I would even go further and leave this patch as it was in the
> > > first version [1] - since:
> > >
> > > - It is similar to what Linux does
> > > - for iMX it uses check on HW bit which is guarantee to be
> > > cleared in some point of time (of course if HW is not broken).
> > >
> > > However, lets wait for input from Simon - how he would like to
> > > tackle this issue.  
> >
> > Unfortunately, I did not received any feedback from Simon.
> >
> > Hence, I would opt for discarding this particular patch and use the
> > v1 of it (please find the rationale above):
> >
> > https://patchwork.ozlabs.org/patch/820824/
> >
> > Has anybody disagree?  
> 
> I think this patch is useful, but I agree it needs to be documented.
> 
> You could add a comment to the pending function in serial.h saying it
> can be called before probe. This would only really work if the driver
> was inited previously though.
> 
> Would it be better instead to have a function in board_f.c somewhere
> which loops on each serial device waiting for the pending function to
> return 0? Then you know that the driver has been probed, and before
> relocation too, so it should be safe.

Hmm... Ok. I will try with this approach.

Thanks for the tip.

> 
> I have seen this problem on other boards so I do favour a generic
> solution if we can figure this out.
> 
> Regards,
> Simon
> 
> >  
> > >  
> > > >  
> > > > >
> > > > > [1] - https://patchwork.ozlabs.org/patch/820824/
> > > > >  
> > > > >>  
> > > > >>>
> > > > >>>
> > > > >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > > > >>> ---
> > > > >>>
> > > > >>> drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
> > > > >>> 1 file changed, 20 insertions(+)
> > > > >>>
> > > > >>> diff --git a/drivers/serial/serial-uclass.c
> > > > >>> b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d
> > > > >>> 100644 --- a/drivers/serial/serial-uclass.c
> > > > >>> +++ b/drivers/serial/serial-uclass.c
> > > > >>> @@ -420,10 +420,30 @@ static int serial_pre_remove(struct
> > > > >>> udevice *dev) return 0;
> > > > >>> }
> > > > >>>
> > > > >>> +static int serial_pre_probe(struct udevice *dev)
> > > > >>> +{
> > > > >>> +       struct dm_serial_ops *ops = serial_get_ops(dev);
> > > > >>> +       int ret = 0;
> > > > >>> +
> > > > >>> +       /*
> > > > >>> +        * Wait for any ongoing transmission to finish - for
> > > > >>> example
> > > > >>> +        * from pre-relocation enabled UART
> > > > >>> +        */
> > > > >>> +       if (ops && ops->pending)
> > > > >>> +               do {
> > > > >>> +                       ret = ops->pending(dev, false);
> > > > >>> +                       if (ret < 0)
> > > > >>> +                               break;
> > > > >>> +               } while (ret > 0);
> > > > >>> +
> > > > >>> +       return ret;
> > > > >>> +}
> > > > >>> +
> > > > >>> UCLASS_DRIVER(serial) = {
> > > > >>>         .id             = UCLASS_SERIAL,
> > > > >>>         .name           = "serial",
> > > > >>>         .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > >>> +       .pre_probe      = serial_pre_probe,
> > > > >>>         .post_probe     = serial_post_probe,
> > > > >>>         .pre_remove     = serial_pre_remove,
> > > > >>>         .per_device_auto_alloc_size = sizeof(struct
> > > > >>> serial_dev_priv), --
> > > > >>> 2.1.4
> > > > >>>  
> > > > >>  
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Best regards,
> > > > >
> > > > > Lukasz Majewski
> > > > >
> > > > > --
> > > > >
> > > > > DENX Software Engineering GmbH,      Managing Director:
> > > > > Wolfgang Denk 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  
> > > >  
> > >
> > >
> > >
> > > Best regards,
> > >
> > > Lukasz Majewski
> > >
> > > --
> > >
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > Denk 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  
> >
> >
> >
> > Best regards,
> >
> > Lukasz Majewski
> >
> > --
> >
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk 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  



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171105/0cad4378/attachment.sig>