[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 at denx.de> wrote:
>> 
>>> On 27 Oct 2017, at 00:13, Lukasz Majewski <lukma at 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 at 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