Hi Simon, > Hi Lukasz, > > On 4 November 2017 at 15:42, Lukasz Majewski <[email protected]> wrote: > > > > 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? > > 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 <[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] 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]
pgpG4_h269uzA.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
