Dear All, > Hi Philipp, > > > Hi Lukasz, > > > > > On 27 Oct 2017, at 00:51, Lukasz Majewski <[email protected]> wrote: > > >> > > >>> On 27 Oct 2017, at 00:13, Lukasz Majewski <[email protected]> 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 <[email protected]> > > >>> --- > > >>> > > >>> 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: > > > [email protected] > > > > > > 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: [email protected] 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: [email protected]
pgpw5tuaVaCIH.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
