dm: serial: Add .pre_probe() function to to wait for previous transmission end

PATCH New Lukasz Majewski 3 participants 8 messages Oct 26, 2017 – Nov 5, 2017

Share & tools

Lukasz Majewski

author

2017-10-26 22:13 UTC #0 lore raw

PATCH 1/1 dm: serial: Add .pre_probe() function to to wait for previous transmission end

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.

Signed-off-by Lukasz Majewski <[email protected]>

---

 drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

drivers/serial/serial-uclass.c +20 -0

index 2e5116f..5e6964d 100644
@@ -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),

Signature · 1 line

2.1.4

Dr. Philipp Tomsich

2017-10-26 22:19 UTC #1 lore raw

PATCH 1/1 dm: serial: Add .pre_probe() function to to wait for previous transmission end

Quoted 21 lines · Lukasz Majewski

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

Quoted 6 lines

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?

Quoted 46 lines

Signed-off-by Lukasz Majewski <[email protected]>

---

drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++

1 file changed, 20 insertions(+)

drivers/serial/serial-uclass.c +20 -0

index 2e5116f..5e6964d 100644
@@ -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),

Signature · 1 line

2.1.4

Lukasz Majewski

author

2017-10-26 22:51 UTC #2 lore raw

PATCH 1/1 dm: serial: Add .pre_probe() function to to wait for previous transmission end

Hi Philipp,

Quoted 29 lines

Quoted 22 lines · Lukasz Majewski
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
Quoted 6 lines
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.

Quoted 3 lines

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.

[1] - https://patchwork.ozlabs.org/patch/820824/

Quoted 40 lines

Signed-off-by Lukasz Majewski <[email protected]>

---

drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++

1 file changed, 20 insertions(+)

(quoted diff) +0 -0

b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644

drivers/serial/serial-uclass.c +7 -0

@@ -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

drivers/serial/serial-uclass.c +12 -0

+	 * 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",

Quoted 9 lines

Quoted 8 lines
	.flags		= DM_UC_FLAG_SEQ_ALIAS,
drivers/serial/serial-uclass.c +1 -0
+	.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

Dr. Philipp Tomsich

2017-10-27 13:34 UTC #3 lore raw

PATCH 1/1 dm: serial: Add .pre_probe() function to to wait for previous transmission end

Hi Lukasz,

Quoted 42 lines · Lukasz Majewski

Quoted 29 lines
Quoted 22 lines · Lukasz Majewski
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
Quoted 6 lines
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.
Quoted 3 lines
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.

Quoted 6 lines

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.

Quoted 65 lines

[1] - https://patchwork.ozlabs.org/patch/820824/
Quoted 40 lines
Quoted 40 lines
Signed-off-by Lukasz Majewski <[email protected]>
---
drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
(quoted diff) +0 -0
b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
drivers/serial/serial-uclass.c +7 -0
@@ -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
drivers/serial/serial-uclass.c +12 -0
+	 * 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",
Quoted 9 lines
Quoted 8 lines
	.flags		= DM_UC_FLAG_SEQ_ALIAS,
drivers/serial/serial-uclass.c +1 -0
+	.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

Lukasz Majewski

author

2017-10-27 13:57 UTC #4 lore raw

PATCH 1/1 dm: serial: Add .pre_probe() function to to wait for previous transmission end

Hi Philipp,

Quoted 49 lines

Hi Lukasz,
Quoted 47 lines · Lukasz Majewski
Quoted 30 lines
Quoted 22 lines · Lukasz Majewski
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
Quoted 7 lines
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.
Quoted 3 lines
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).

Quoted 7 lines

Quoted 2 lines
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.

Quoted 6 lines

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.

Quoted 59 lines

Quoted 59 lines
[1] - https://patchwork.ozlabs.org/patch/820824/
Quoted 40 lines
Quoted 40 lines
Signed-off-by Lukasz Majewski <[email protected]>
---
drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
(quoted diff) +0 -0
b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
drivers/serial/serial-uclass.c +7 -0
@@ -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
drivers/serial/serial-uclass.c +12 -0
+	 * 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",
Quoted 9 lines
Quoted 8 lines
	.flags		= DM_UC_FLAG_SEQ_ALIAS,
drivers/serial/serial-uclass.c +1 -0
+	.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,

Quoted 9 lines

Quoted 8 lines
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

author

2017-11-04 21:42 UTC #5 lore raw

PATCH 1/1 dm: serial: Add .pre_probe() function to to wait for previous transmission end

Dear All,

Quoted 76 lines

Hi Philipp,
Quoted 49 lines
Hi Lukasz,
Quoted 47 lines · Lukasz Majewski
Quoted 30 lines
Quoted 22 lines · Lukasz Majewski
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
Quoted 7 lines
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.
Quoted 3 lines
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).
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
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:

Quoted 6 lines

- 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?

Quoted 74 lines

Quoted 60 lines
Quoted 60 lines
[1] - https://patchwork.ozlabs.org/patch/820824/
Quoted 40 lines
Quoted 40 lines
Signed-off-by Lukasz Majewski <[email protected]>
---
drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
(quoted diff) +0 -0
b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
drivers/serial/serial-uclass.c +7 -0
@@ -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
drivers/serial/serial-uclass.c +12 -0
+	 * 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",
Quoted 9 lines
Quoted 8 lines
	.flags		= DM_UC_FLAG_SEQ_ALIAS,
drivers/serial/serial-uclass.c +1 -0
+	.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,
Quoted 9 lines
Quoted 8 lines
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,

Quoted 7 lines

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>

Simon Glass

2017-11-05 02:45 UTC #6 lore raw

PATCH 1/1 dm: serial: Add .pre_probe() function to to wait for previous transmission end

Hi Lukasz,

Quoted 86 lines · Lukasz Majewski

Dear All,
Quoted 76 lines
Hi Philipp,
Quoted 49 lines
Hi Lukasz,
Quoted 47 lines · Lukasz Majewski
Quoted 30 lines
Quoted 22 lines · Lukasz Majewski
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
Quoted 7 lines
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.
Quoted 3 lines
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).
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
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:
Quoted 6 lines
- 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.

Quoted 8 lines

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.

I have seen this problem on other boards so I do favour a generic solution if we can figure this out.

Regards,
Simon

Quoted 94 lines

Quoted 75 lines
Quoted 61 lines
Quoted 61 lines
[1] - https://patchwork.ozlabs.org/patch/820824/
Quoted 40 lines
Quoted 40 lines
Signed-off-by Lukasz Majewski <[email protected]>
---
drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
(quoted diff) +0 -0
b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644
drivers/serial/serial-uclass.c +7 -0
@@ -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
drivers/serial/serial-uclass.c +12 -0
+        * 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",
Quoted 9 lines
Quoted 8 lines
        .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,
Quoted 9 lines
Quoted 8 lines
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,
Quoted 7 lines
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

Lukasz Majewski

author

2017-11-05 22:16 UTC #7 lore raw

PATCH 1/1 dm: serial: Add .pre_probe() function to to wait for previous transmission end

Hi Simon,

Quoted 105 lines

Hi Lukasz,
Quoted 91 lines · Lukasz Majewski
Dear All,
Quoted 81 lines
Hi Philipp,
Quoted 54 lines
Hi Lukasz,
Quoted 52 lines
Quoted 32 lines · Lukasz Majewski
Quoted 23 lines
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
Quoted 8 lines
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.
Quoted 3 lines
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).
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
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:
Quoted 6 lines
- 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.
Quoted 8 lines
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.

Quoted 8 lines

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.

Quoted 103 lines

I have seen this problem on other boards so I do favour a generic solution if we can figure this out.
Regards,
Simon
Quoted 96 lines
Quoted 75 lines
Quoted 61 lines
Quoted 61 lines
[1] - https://patchwork.ozlabs.org/patch/820824/
Quoted 40 lines
Quoted 40 lines
Signed-off-by Lukasz Majewski <[email protected]>
---
drivers/serial/serial-uclass.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
(quoted diff) +0 -0
b/drivers/serial/serial-uclass.c index 2e5116f..5e6964d 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c
(quoted diff) +7 -0
@@ -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
(quoted diff) +12 -0
+        * 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",
Quoted 9 lines
Quoted 8 lines
        .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,
Quoted 9 lines
Quoted 8 lines
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,
Quoted 8 lines
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