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

Sat Nov 4 21:42:15 UTC 2017

Dear All,

> Hi Philipp,
> 
> > 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.  
> 
> 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 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  
> >   
> 
> 
> 
> 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>