serial: zynq: Fix serial driver

PATCH New Michal Simek 2 participants 15 messages Jun 14, 2018 – Jun 15, 2018 Arm/Zynq architecture

lore

Share & tools

lore

Michal Simek

author

2018-06-14 09:32 UTC #0 lore raw

cover serial: zynq: Fix serial driver

Hi,

this series contain some changes in this driver.
1. Coding style fixes - using BIT macro
2. Using output fifo more effectively
3. Avoid initialization after relocation again
4. Make sure that priv is valid all the time
5. Keep sparse happy

Thanks,
Michal

Michal Simek (5):
serial: zynq: Use BIT macros instead of shifts and full hex numbers
serial: zynq: Write chars till output fifo is full
serial: zynq: Initialize uart only before relocation
serial: zynq: Check priv pointer get via dev_get_priv()
serial: zynq: Make zynq_serial_setbrg static

 drivers/serial/serial_zynq.c | 51 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 12 deletions(-)

Signature · 1 line

1.9.1

Michal Simek

author

2018-06-14 09:32 UTC #1 lore raw

PATCH 1/5 serial: zynq: Use BIT macros instead of shifts and full hex numbers

Coding style is checking to use BIT macros instead of shifts. The patch is also fixing the rest of macros which should be BITs instead of hex numbers.

Signed-off-by Michal Simek <[email protected]>

---

 drivers/serial/serial_zynq.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/serial/serial_zynq.c +8 -8

index 3650af215731..7a6f822c26ac 100644
@@ -15,14 +15,14 @@
 #include <linux/compiler.h>
 #include <serial.h>
 
-#define ZYNQ_UART_SR_TXEMPTY	(1 << 3) /* TX FIFO empty */
-#define ZYNQ_UART_SR_TXACTIVE	(1 << 11)  /* TX active */
-#define ZYNQ_UART_SR_RXEMPTY	0x00000002 /* RX FIFO empty */
-
-#define ZYNQ_UART_CR_TX_EN	0x00000010 /* TX enabled */
-#define ZYNQ_UART_CR_RX_EN	0x00000004 /* RX enabled */
-#define ZYNQ_UART_CR_TXRST	0x00000002 /* TX logic reset */
-#define ZYNQ_UART_CR_RXRST	0x00000001 /* RX logic reset */
+#define ZYNQ_UART_SR_TXEMPTY	BIT(3) /* TX FIFO empty */
+#define ZYNQ_UART_SR_TXACTIVE	BIT(11) /* TX active */
+#define ZYNQ_UART_SR_RXEMPTY	BIT(1) /* RX FIFO empty */
+
+#define ZYNQ_UART_CR_TX_EN	BIT(4) /* TX enabled */
+#define ZYNQ_UART_CR_RX_EN	BIT(2) /* RX enabled */
+#define ZYNQ_UART_CR_TXRST	BIT(1) /* TX logic reset */
+#define ZYNQ_UART_CR_RXRST	BIT(0) /* RX logic reset */
 
 #define ZYNQ_UART_MR_PARITY_NONE	0x00000020  /* No parity mode */

Signature · 1 line

1.9.1

Simon Glass

2018-06-14 12:58 UTC #2 lore raw

PATCH 1/5 serial: zynq: Use BIT macros instead of shifts and full hex numbers

Quoted 9 lines · Michal Simek

Coding style is checking to use BIT macros instead of shifts. The patch is also fixing the rest of macros which should be BITs instead of hex numbers.
Signed-off-by Michal Simek <[email protected]>
---
 drivers/serial/serial_zynq.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Michal Simek

author

2018-06-14 09:32 UTC #3 lore raw

PATCH 2/5 serial: zynq: Write chars till output fifo is full

In reply to Michal Simek

Change logic and put char to fifo till there is a space in output fifo. Origin logic was that output fifo needs to be empty. It means only one char was in output queue.
Also remove unused ZYNQ_UART_SR_TXEMPTY macro.

Signed-off-by Michal Simek <[email protected]>

---

 drivers/serial/serial_zynq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/serial/serial_zynq.c +2 -2

index 7a6f822c26ac..4ae24939ab23 100644
@@ -15,8 +15,8 @@
 #include <linux/compiler.h>
 #include <serial.h>
 
-#define ZYNQ_UART_SR_TXEMPTY	BIT(3) /* TX FIFO empty */
 #define ZYNQ_UART_SR_TXACTIVE	BIT(11) /* TX active */
+#define ZYNQ_UART_SR_TXFULL	BIT(4) /* TX FIFO full */
 #define ZYNQ_UART_SR_RXEMPTY	BIT(1) /* RX FIFO empty */
 
 #define ZYNQ_UART_CR_TX_EN	BIT(4) /* TX enabled */

@@ -93,7 +93,7 @@ static void _uart_zynq_serial_init(struct uart_zynq *regs)
 
 static int _uart_zynq_serial_putc(struct uart_zynq *regs, const char c)
 {
-	if (!(readl(&regs->channel_sts) & ZYNQ_UART_SR_TXEMPTY))
+	if (readl(&regs->channel_sts) & ZYNQ_UART_SR_TXFULL)
 		return -EAGAIN;
 
 	writel(c, &regs->tx_rx_fifo);

Signature · 1 line

1.9.1

Simon Glass

2018-06-14 12:58 UTC #4 lore raw

PATCH 2/5 serial: zynq: Write chars till output fifo is full

Quoted 10 lines · Michal Simek

Change logic and put char to fifo till there is a space in output fifo. Origin logic was that output fifo needs to be empty. It means only one char was in output queue.
Also remove unused ZYNQ_UART_SR_TXEMPTY macro.
Signed-off-by Michal Simek <[email protected]>
---
 drivers/serial/serial_zynq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Michal Simek

author

2018-06-14 09:32 UTC #5 lore raw

PATCH 3/5 serial: zynq: Initialize uart only before relocation

In reply to Michal Simek

This issue was found when OF_LIVE was enabled that there are scrambled chars on the console like this:
Chip ID: zu3eg
Watchdog: Started��j� sdhci at ff160000: 0, sdhci at ff170000: 1
In: serial at ff010000

I found a solution for this problem exactly the same as I found later in serial_msm fixed by:
"serial: serial_msm: initialize uart only before relocation" (sha1: 7e5ad796bcd65772a87da236ae21cd536ae3a4d2)

What it is happening is that output TX fifo still contains chars to be sent and _uart_zynq_serial_init() resets TX fifo even in the middle of transfer.

Signed-off-by Michal Simek <[email protected]>

---

 drivers/serial/serial_zynq.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/serial/serial_zynq.c +6 -0

index 4ae24939ab23..cc14bfa39cff 100644
@@ -15,6 +15,8 @@
 #include <linux/compiler.h>
 #include <serial.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 #define ZYNQ_UART_SR_TXACTIVE	BIT(11) /* TX active */
 #define ZYNQ_UART_SR_TXFULL	BIT(4) /* TX FIFO full */
 #define ZYNQ_UART_SR_RXEMPTY	BIT(1) /* RX FIFO empty */

@@ -137,6 +139,10 @@ static int zynq_serial_probe(struct udevice *dev)
 {
 	struct zynq_uart_priv *priv = dev_get_priv(dev);
 
+	/* No need to reinitialize the UART after relocation */
+	if (gd->flags & GD_FLG_RELOC)
+		return 0;
+
 	_uart_zynq_serial_init(priv->regs);
 
 	return 0;

Signature · 1 line

1.9.1

Simon Glass

2018-06-14 12:58 UTC #6 lore raw

PATCH 3/5 serial: zynq: Initialize uart only before relocation

Quoted 15 lines · Michal Simek

This issue was found when OF_LIVE was enabled that there are scrambled chars on the console like this:
Chip ID:        zu3eg
Watchdog: Started��j�   sdhci at ff160000: 0, sdhci at ff170000: 1
In:    serial at ff010000
I found a solution for this problem exactly the same as I found later in serial_msm fixed by:
"serial: serial_msm: initialize uart only before relocation" (sha1: 7e5ad796bcd65772a87da236ae21cd536ae3a4d2)
What it is happening is that output TX fifo still contains chars to be sent and _uart_zynq_serial_init() resets TX fifo even in the middle of transfer.

Quoted 6 lines

Signed-off-by Michal Simek <[email protected]>
---
 drivers/serial/serial_zynq.c | 6 ++++++
 1 file changed, 6 insertions(+)

Reviewed-by Simon Glass <[email protected]>

Michal Simek

author

2018-06-14 09:32 UTC #7 lore raw

PATCH 4/5 serial: zynq: Check priv pointer get via dev_get_priv()

In reply to Michal Simek

Make sure that functions are working with proper strcture.

Signed-off-by Michal Simek <[email protected]>

---

Reported by: Coverity (local)

---
 drivers/serial/serial_zynq.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

drivers/serial/serial_zynq.c +24 -3

index cc14bfa39cff..0fd9e4b4a857 100644
@@ -107,10 +107,12 @@ int zynq_serial_setbrg(struct udevice *dev, int baudrate)
 {
 	struct zynq_uart_priv *priv = dev_get_priv(dev);
 	unsigned long clock;
-
 	int ret;
 	struct clk clk;
 
+	if (!priv)
+		return -EINVAL;
+
 	ret = clk_get_by_index(dev, 0, &clk);
 	if (ret < 0) {
 		dev_err(dev, "failed to get clock\n");

@@ -139,6 +141,9 @@ static int zynq_serial_probe(struct udevice *dev)
 {
 	struct zynq_uart_priv *priv = dev_get_priv(dev);
 
+	if (!priv)
+		return -EINVAL;
+
 	/* No need to reinitialize the UART after relocation */
 	if (gd->flags & GD_FLG_RELOC)
 		return 0;

@@ -150,8 +155,13 @@ static int zynq_serial_probe(struct udevice *dev)
 
 static int zynq_serial_getc(struct udevice *dev)
 {
+	struct uart_zynq *regs;
 	struct zynq_uart_priv *priv = dev_get_priv(dev);
-	struct uart_zynq *regs = priv->regs;
+
+	if (!priv)
+		return -EINVAL;
+
+	regs = priv->regs;
 
 	if (readl(&regs->channel_sts) & ZYNQ_UART_SR_RXEMPTY)
 		return -EAGAIN;

@@ -163,13 +173,21 @@ static int zynq_serial_putc(struct udevice *dev, const char ch)
 {
 	struct zynq_uart_priv *priv = dev_get_priv(dev);
 
+	if (!priv)
+		return -EINVAL;
+
 	return _uart_zynq_serial_putc(priv->regs, ch);
 }
 
 static int zynq_serial_pending(struct udevice *dev, bool input)
 {
 	struct zynq_uart_priv *priv = dev_get_priv(dev);
-	struct uart_zynq *regs = priv->regs;
+	struct uart_zynq *regs;
+
+	if (!priv)
+		return -EINVAL;
+
+	regs = priv->regs;
 
 	if (input)
 		return !(readl(&regs->channel_sts) & ZYNQ_UART_SR_RXEMPTY);

@@ -181,6 +199,9 @@ static int zynq_serial_ofdata_to_platdata(struct udevice *dev)
 {
 	struct zynq_uart_priv *priv = dev_get_priv(dev);
 
+	if (!priv)
+		return -EINVAL;
+
 	priv->regs = (struct uart_zynq *)dev_read_addr(dev);
 	if (IS_ERR(priv->regs))
 		return PTR_ERR(priv->regs);

Signature · 1 line

1.9.1

Simon Glass

2018-06-14 12:58 UTC #8 lore raw

PATCH 4/5 serial: zynq: Check priv pointer get via dev_get_priv()

Hi,

Quoted 10 lines · Michal Simek

Make sure that functions are working with proper strcture.
Signed-off-by Michal Simek <[email protected]>
---
Reported by: Coverity (local)
---
 drivers/serial/serial_zynq.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

But priv cannot be NULL if the device has been probed. So this check is confusing at best.

If this is from coverity, perhaps you can find a way to mask it?

Regards,
Simon

Michal Simek

author

2018-06-14 13:05 UTC #9 lore raw

PATCH 4/5 serial: zynq: Check priv pointer get via dev_get_priv()

Quoted 10 lines · Simon Glass

Hi,
Quoted 7 lines · Michal Simek
Make sure that functions are working with proper strcture.
Signed-off-by Michal Simek <[email protected]>
---
Reported by: Coverity (local)

Quoted 8 lines

Quoted 3 lines
---
 drivers/serial/serial_zynq.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
But priv cannot be NULL if the device has been probed. So this check is confusing at best.
If this is from coverity, perhaps you can find a way to mask it?

Let me talk to local guy how to do it.

Also can you please look at that driver and tell me if we using private structure is used properly?
Because I think we should be using platdata instead.
What was that rule for using platdate versus private data?

Thanks,
Michal

Simon Glass

2018-06-14 14:16 UTC #10 lore raw

PATCH 4/5 serial: zynq: Check priv pointer get via dev_get_priv()

Hi Michal,

Quoted 18 lines · Michal Simek

Quoted 17 lines · Simon Glass
Hi,
Quoted 10 lines · Michal Simek
Make sure that functions are working with proper strcture.
Signed-off-by Michal Simek <[email protected]>
---
Reported by: Coverity (local)
---
 drivers/serial/serial_zynq.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
But priv cannot be NULL if the device has been probed. So this check is confusing at best.

Quoted 8 lines

Quoted 1 line
If this is from coverity, perhaps you can find a way to mask it?
Let me talk to local guy how to do it.
Also can you please look at that driver and tell me if we using private structure is used properly?
Because I think we should be using platdata instead.
What was that rule for using platdate versus private data?

Private data is created when the device is probed and freed when the device is removed.

Platform data is created when the device is bound, and survives probe/remove cycles.

Strictly speaking, platform data should be used to hold the decoded device tree properties. Private data should be used for run-time things the device needs to keep track of.

Regards,
Simon

Michal Simek

author

2018-06-15 06:11 UTC #11 lore raw

PATCH 4/5 serial: zynq: Check priv pointer get via dev_get_priv()

Hi Simon,

Quoted 33 lines · Simon Glass

Hi Michal,
Quoted 18 lines · Michal Simek
Quoted 17 lines · Simon Glass
Hi,
Quoted 10 lines · Michal Simek
Make sure that functions are working with proper strcture.
Signed-off-by Michal Simek <[email protected]>
---
Reported by: Coverity (local)
---
 drivers/serial/serial_zynq.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
But priv cannot be NULL if the device has been probed. So this check is confusing at best.
Quoted 8 lines
Quoted 1 line
If this is from coverity, perhaps you can find a way to mask it?
Let me talk to local guy how to do it.
Also can you please look at that driver and tell me if we using private structure is used properly?
Because I think we should be using platdata instead.
What was that rule for using platdate versus private data?
Private data is created when the device is probed and freed when the device is removed.

Quoted 6 lines

Platform data is created when the device is bound, and survives probe/remove cycles.
Strictly speaking, platform data should be used to hold the decoded device tree properties. Private data should be used for run-time things the device needs to keep track of.

ok. It means we need to fix at least this driver.

Thanks,
Michal

Michal Simek

author

2018-06-15 07:33 UTC #12 lore raw

PATCH 4/5 serial: zynq: Check priv pointer get via dev_get_priv()

In reply to Simon Glass

Quoted 35 lines · Simon Glass

EXTERNAL EMAIL
Hi Michal,
Quoted 18 lines · Michal Simek
Quoted 17 lines · Simon Glass
Hi,
Quoted 10 lines · Michal Simek
Make sure that functions are working with proper strcture.
Signed-off-by Michal Simek <[email protected]>
---
Reported by: Coverity (local)
---
 drivers/serial/serial_zynq.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
But priv cannot be NULL if the device has been probed. So this check is confusing at best.
Quoted 8 lines
Quoted 1 line
If this is from coverity, perhaps you can find a way to mask it?
Let me talk to local guy how to do it.
Also can you please look at that driver and tell me if we using private structure is used properly?
Because I think we should be using platdata instead.
What was that rule for using platdate versus private data?
Private data is created when the device is probed and freed when the device is removed.

Quoted 6 lines

Platform data is created when the device is bound, and survives probe/remove cycles.
Strictly speaking, platform data should be used to hold the decoded device tree properties. Private data should be used for run-time things the device needs to keep track of.

wonderful. Here is the patch for serial and we will check other drivers.

https://lists.denx.de/pipermail/u-boot/2018-June/331932.html

Thanks,
Michal

Michal Simek

author

2018-06-14 09:32 UTC #13 lore raw

PATCH 5/5 serial: zynq: Make zynq_serial_setbrg static

In reply to Michal Simek

This function is used only inside this driver that's why should be static.

Signed-off-by Michal Simek <[email protected]>

---

 drivers/serial/serial_zynq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/serial/serial_zynq.c +1 -1

index 0fd9e4b4a857..d0d8aa6828f0 100644
@@ -103,7 +103,7 @@ static int _uart_zynq_serial_putc(struct uart_zynq *regs, const char c)
 	return 0;
 }
 
-int zynq_serial_setbrg(struct udevice *dev, int baudrate)
+static int zynq_serial_setbrg(struct udevice *dev, int baudrate)
 {
 	struct zynq_uart_priv *priv = dev_get_priv(dev);
 	unsigned long clock;

Signature · 1 line

1.9.1

Simon Glass

2018-06-14 12:58 UTC #14 lore raw

PATCH 5/5 serial: zynq: Make zynq_serial_setbrg static

Quoted 9 lines · Michal Simek

This function is used only inside this driver that's why should be static.
Signed-off-by Michal Simek <[email protected]>
---
 drivers/serial/serial_zynq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by Simon Glass <[email protected]>