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

[Lukasz Majewski <lukma@denx.de>]

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>