[U-Boot] [PATCH 0/5] serial: zynq: Fix serial driver

[U-Boot] [PATCH 0/5] serial: zynq: Fix serial driver

[Michal Simek]

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(-)

-- 
1.9.1

[U-Boot] [PATCH 1/5] serial: zynq: Use BIT macros instead of shifts and full hex numbers

[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 <michal.simek@xilinx.com>
---

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

diff --git a/drivers/serial/serial_zynq.c b/drivers/serial/serial_zynq.c
index 3650af215731..7a6f822c26ac 100644
--- a/drivers/serial/serial_zynq.c
+++ b/drivers/serial/serial_zynq.c
@@ -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 */
 
-- 
1.9.1

[U-Boot] [PATCH 2/5] serial: zynq: Write chars till output fifo is full

[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 <michal.simek@xilinx.com>
---

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

diff --git a/drivers/serial/serial_zynq.c b/drivers/serial/serial_zynq.c
index 7a6f822c26ac..4ae24939ab23 100644
--- a/drivers/serial/serial_zynq.c
+++ b/drivers/serial/serial_zynq.c
@@ -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);
-- 
1.9.1

[U-Boot] [PATCH 3/5] serial: zynq: Initialize uart only before relocation

[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 <michal.simek@xilinx.com>
---

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

diff --git a/drivers/serial/serial_zynq.c b/drivers/serial/serial_zynq.c
index 4ae24939ab23..cc14bfa39cff 100644
--- a/drivers/serial/serial_zynq.c
+++ b/drivers/serial/serial_zynq.c
@@ -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;
-- 
1.9.1

[U-Boot] [PATCH 4/5] serial: zynq: Check priv pointer get via dev_get_priv()

[Michal Simek]

Make sure that functions are working with proper strcture.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Reported by: Coverity (local)

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

diff --git a/drivers/serial/serial_zynq.c b/drivers/serial/serial_zynq.c
index cc14bfa39cff..0fd9e4b4a857 100644
--- a/drivers/serial/serial_zynq.c
+++ b/drivers/serial/serial_zynq.c
@@ -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);
-- 
1.9.1

[U-Boot] [PATCH 5/5] serial: zynq: Make zynq_serial_setbrg static

[Michal Simek]

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

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

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

diff --git a/drivers/serial/serial_zynq.c b/drivers/serial/serial_zynq.c
index 0fd9e4b4a857..d0d8aa6828f0 100644
--- a/drivers/serial/serial_zynq.c
+++ b/drivers/serial/serial_zynq.c
@@ -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;
-- 
1.9.1

[U-Boot] [PATCH 1/5] serial: zynq: Use BIT macros instead of shifts and full hex numbers

[Simon Glass]

On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
> 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 <michal.simek@xilinx.com>
> ---
>
>  drivers/serial/serial_zynq.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/5] serial: zynq: Write chars till output fifo is full

[Simon Glass]

On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
> 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 <michal.simek@xilinx.com>
> ---
>
>  drivers/serial/serial_zynq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 3/5] serial: zynq: Initialize uart only before relocation

[Simon Glass]

On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
> 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 <michal.simek@xilinx.com>
> ---
>
>  drivers/serial/serial_zynq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 4/5] serial: zynq: Check priv pointer get via dev_get_priv()

[Simon Glass]

Hi,

On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
> Make sure that functions are working with proper strcture.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> 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

[U-Boot] [PATCH 5/5] serial: zynq: Make zynq_serial_setbrg static

[Simon Glass]

On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
> This function is used only inside this driver that's why should be
> static.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  drivers/serial/serial_zynq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 4/5] serial: zynq: Check priv pointer get via dev_get_priv()

[Michal Simek]

On 14.6.2018 14:58, Simon Glass wrote:
> Hi,
> 
> On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
>> Make sure that functions are working with proper strcture.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> 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?

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

[U-Boot] [PATCH 4/5] serial: zynq: Check priv pointer get via dev_get_priv()

[Simon Glass]

Hi Michal,

On 14 June 2018 at 07:05, Michal Simek <michal.simek@xilinx.com> wrote:
> On 14.6.2018 14:58, Simon Glass wrote:
>> Hi,
>>
>> On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
>>> Make sure that functions are working with proper strcture.
>>>
>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>> ---
>>>
>>> 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?
>
> 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

[U-Boot] [PATCH 4/5] serial: zynq: Check priv pointer get via dev_get_priv()

[Michal Simek]

Hi Simon,

On 14.6.2018 16:16, Simon Glass wrote:
> Hi Michal,
> 
> On 14 June 2018 at 07:05, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 14.6.2018 14:58, Simon Glass wrote:
>>> Hi,
>>>
>>> On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
>>>> Make sure that functions are working with proper strcture.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>> ---
>>>>
>>>> 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?
>>
>> 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.

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

Thanks,
Michal

[U-Boot] [PATCH 4/5] serial: zynq: Check priv pointer get via dev_get_priv()

[Michal Simek]

On 14.6.2018 16:16, Simon Glass wrote:
> EXTERNAL EMAIL
> 
> Hi Michal,
> 
> On 14 June 2018 at 07:05, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 14.6.2018 14:58, Simon Glass wrote:
>>> Hi,
>>>
>>> On 14 June 2018 at 03:32, Michal Simek <michal.simek@xilinx.com> wrote:
>>>> Make sure that functions are working with proper strcture.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>> ---
>>>>
>>>> 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?
>>
>> 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.

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