[PATCH 0/4] serial: s5p: Cleanups

[PATCH 0/4] serial: s5p: Cleanups

[Sam Protsenko]

A small collection of cleanup patches for serial_s5p.c driver. Should
induce no functional changes.

Sam Protsenko (4):
  serial: s5p: Remove common.h inclusion
  serial: s5p: Use livetree API to get "id" property
  serial: s5p: Use named constants for register values
  serial: s5p: Improve coding style

 drivers/serial/serial_s5p.c | 71 +++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 31 deletions(-)

-- 
2.39.2

[PATCH 1/4] serial: s5p: Remove common.h inclusion

[Sam Protsenko]

It's not really needed here anymore. Remove it, as common.h is going
away at some point.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/serial/serial_s5p.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
index fe52580d64de..177215535676 100644
--- a/drivers/serial/serial_s5p.c
+++ b/drivers/serial/serial_s5p.c
@@ -7,7 +7,6 @@
  * based on drivers/serial/s3c64xx.c
  */
 
-#include <common.h>
 #include <dm.h>
 #include <errno.h>
 #include <fdtdec.h>
-- 
2.39.2

[PATCH 2/4] serial: s5p: Use livetree API to get "id" property

[Sam Protsenko]

Use dev_read_u8_default() instead of fdtdec_get_int() to read the "id"
property from device tree, as suggested in [1]. dev_* API is already
used in this driver, so there is no reason to stick to fdtdec_* API.
This also fixes checkpatch warning:

    WARNING: Use the livetree API (dev_read_...)

[1] doc/develop/driver-model/livetree.rst

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/serial/serial_s5p.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
index 177215535676..c57bdd059ea6 100644
--- a/drivers/serial/serial_s5p.c
+++ b/drivers/serial/serial_s5p.c
@@ -20,8 +20,6 @@
 #include <serial.h>
 #include <clk.h>
 
-DECLARE_GLOBAL_DATA_PTR;
-
 enum {
 	PORT_S5P = 0,
 	PORT_S5L
@@ -220,8 +218,7 @@ static int s5p_serial_of_to_plat(struct udevice *dev)
 
 	plat->reg = (struct s5p_uart *)addr;
 	plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
-	plat->port_id = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
-					"id", dev_seq(dev));
+	plat->port_id = dev_read_u8_default(dev, "id", dev_seq(dev));
 
 	if (port_type == PORT_S5L) {
 		plat->rx_fifo_count_shift = S5L_RX_FIFO_COUNT_SHIFT;
-- 
2.39.2

[PATCH 3/4] serial: s5p: Use named constants for register values

[Sam Protsenko]

Get rid of magic numbers in s5p_serial_init() when writing to UART
registers. While at it, use BIT() macro for existing constants when
appropriate.

No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/serial/serial_s5p.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
index c57bdd059ea6..6d316ccaf31d 100644
--- a/drivers/serial/serial_s5p.c
+++ b/drivers/serial/serial_s5p.c
@@ -25,19 +25,28 @@ enum {
 	PORT_S5L
 };
 
+#define UFCON_FIFO_EN		BIT(0)
+#define UFCON_RX_FIFO_RESET	BIT(1)
+#define UMCON_RESET_VAL		0x0
+#define ULCON_WORD_8_BIT	0x3
+#define UCON_RX_IRQ_OR_POLLING	BIT(0)
+#define UCON_TX_IRQ_OR_POLLING	BIT(2)
+#define UCON_RX_ERR_IRQ_EN	BIT(6)
+#define UCON_TX_IRQ_LEVEL	BIT(9)
+
 #define S5L_RX_FIFO_COUNT_SHIFT	0
 #define S5L_RX_FIFO_COUNT_MASK	(0xf << S5L_RX_FIFO_COUNT_SHIFT)
-#define S5L_RX_FIFO_FULL	(1 << 8)
+#define S5L_RX_FIFO_FULL	BIT(8)
 #define S5L_TX_FIFO_COUNT_SHIFT	4
 #define S5L_TX_FIFO_COUNT_MASK	(0xf << S5L_TX_FIFO_COUNT_SHIFT)
-#define S5L_TX_FIFO_FULL	(1 << 9)
+#define S5L_TX_FIFO_FULL	BIT(9)
 
 #define S5P_RX_FIFO_COUNT_SHIFT	0
 #define S5P_RX_FIFO_COUNT_MASK	(0xff << S5P_RX_FIFO_COUNT_SHIFT)
-#define S5P_RX_FIFO_FULL	(1 << 8)
+#define S5P_RX_FIFO_FULL	BIT(8)
 #define S5P_TX_FIFO_COUNT_SHIFT	16
 #define S5P_TX_FIFO_COUNT_MASK	(0xff << S5P_TX_FIFO_COUNT_SHIFT)
-#define S5P_TX_FIFO_FULL	(1 << 24)
+#define S5P_TX_FIFO_FULL	BIT(24)
 
 /* Information about a serial port */
 struct s5p_serial_plat {
@@ -80,13 +89,15 @@ static const int udivslot[] = {
 
 static void __maybe_unused s5p_serial_init(struct s5p_uart *uart)
 {
-	/* enable FIFOs, auto clear Rx FIFO */
-	writel(0x3, &uart->ufcon);
-	writel(0, &uart->umcon);
-	/* 8N1 */
-	writel(0x3, &uart->ulcon);
+	/* Enable FIFOs, auto clear Rx FIFO */
+	writel(UFCON_FIFO_EN | UFCON_RX_FIFO_RESET, &uart->ufcon);
+	/* No auto flow control, disable nRTS signal */
+	writel(UMCON_RESET_VAL, &uart->umcon);
+	/* 8N1, no parity bit */
+	writel(ULCON_WORD_8_BIT, &uart->ulcon);
 	/* No interrupts, no DMA, pure polling */
-	writel(0x245, &uart->ucon);
+	writel(UCON_RX_IRQ_OR_POLLING | UCON_TX_IRQ_OR_POLLING |
+	       UCON_RX_ERR_IRQ_EN | UCON_TX_IRQ_LEVEL, &uart->ucon);
 }
 
 static void __maybe_unused s5p_serial_baud(struct s5p_uart *uart, u8 reg_width,
-- 
2.39.2

[PATCH 4/4] serial: s5p: Improve coding style

[Sam Protsenko]

Just some minor style fixes. No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/serial/serial_s5p.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
index 6d316ccaf31d..c24d9bca84c9 100644
--- a/drivers/serial/serial_s5p.c
+++ b/drivers/serial/serial_s5p.c
@@ -50,9 +50,9 @@ enum {
 
 /* Information about a serial port */
 struct s5p_serial_plat {
-	struct s5p_uart *reg;  /* address of registers in physical memory */
-	u8 reg_width;	/* register width */
-	u8 port_id;     /* uart port number */
+	struct s5p_uart *reg;	/* address of registers in physical memory */
+	u8 reg_width;		/* register width */
+	u8 port_id;		/* uart port number */
 	u8 rx_fifo_count_shift;
 	u8 tx_fifo_count_shift;
 	u32 rx_fifo_count_mask;
@@ -65,7 +65,7 @@ struct s5p_serial_plat {
  * The coefficient, used to calculate the baudrate on S5P UARTs is
  * calculated as
  * C = UBRDIV * 16 + number_of_set_bits_in_UDIVSLOT
- * however, section 31.6.11 of the datasheet doesn't recomment using 1 for 1,
+ * however, section 31.6.11 of the datasheet doesn't recommend using 1 for 1,
  * 3 for 2, ... (2^n - 1) for n, instead, they suggest using these constants:
  */
 static const int udivslot[] = {
@@ -251,10 +251,10 @@ static int s5p_serial_of_to_plat(struct udevice *dev)
 }
 
 static const struct dm_serial_ops s5p_serial_ops = {
-	.putc = s5p_serial_putc,
-	.pending = s5p_serial_pending,
-	.getc = s5p_serial_getc,
-	.setbrg = s5p_serial_setbrg,
+	.putc		= s5p_serial_putc,
+	.pending	= s5p_serial_pending,
+	.getc		= s5p_serial_getc,
+	.setbrg		= s5p_serial_setbrg,
 };
 
 static const struct udevice_id s5p_serial_ids[] = {
@@ -264,13 +264,13 @@ static const struct udevice_id s5p_serial_ids[] = {
 };
 
 U_BOOT_DRIVER(serial_s5p) = {
-	.name	= "serial_s5p",
-	.id	= UCLASS_SERIAL,
-	.of_match = s5p_serial_ids,
-	.of_to_plat = s5p_serial_of_to_plat,
+	.name		= "serial_s5p",
+	.id		= UCLASS_SERIAL,
+	.of_match	= s5p_serial_ids,
+	.of_to_plat	= s5p_serial_of_to_plat,
 	.plat_auto	= sizeof(struct s5p_serial_plat),
-	.probe = s5p_serial_probe,
-	.ops	= &s5p_serial_ops,
+	.probe		= s5p_serial_probe,
+	.ops		= &s5p_serial_ops,
 };
 #endif
 
@@ -298,10 +298,12 @@ static inline void _debug_uart_putc(int ch)
 	struct s5p_uart *uart = (struct s5p_uart *)CONFIG_VAL(DEBUG_UART_BASE);
 
 #if IS_ENABLED(CONFIG_ARCH_APPLE)
-	while (readl(&uart->ufstat) & S5L_TX_FIFO_FULL);
+	while (readl(&uart->ufstat) & S5L_TX_FIFO_FULL)
+		;
 	writel(ch, &uart->utxh);
 #else
-	while (readl(&uart->ufstat) & S5P_TX_FIFO_FULL);
+	while (readl(&uart->ufstat) & S5P_TX_FIFO_FULL)
+		;
 	writeb(ch, &uart->utxh);
 #endif
 }
-- 
2.39.2

Re: [PATCH 2/4] serial: s5p: Use livetree API to get "id" property

[Simon Glass]

Hi Sam,

On Tue, 7 Nov 2023 at 12:06, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Use dev_read_u8_default() instead of fdtdec_get_int() to read the "id"
> property from device tree, as suggested in [1]. dev_* API is already
> used in this driver, so there is no reason to stick to fdtdec_* API.
> This also fixes checkpatch warning:
>
>     WARNING: Use the livetree API (dev_read_...)
>
> [1] doc/develop/driver-model/livetree.rst
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/serial/serial_s5p.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
> index 177215535676..c57bdd059ea6 100644
> --- a/drivers/serial/serial_s5p.c
> +++ b/drivers/serial/serial_s5p.c
> @@ -20,8 +20,6 @@
>  #include <serial.h>
>  #include <clk.h>
>
> -DECLARE_GLOBAL_DATA_PTR;
> -
>  enum {
>         PORT_S5P = 0,
>         PORT_S5L
> @@ -220,8 +218,7 @@ static int s5p_serial_of_to_plat(struct udevice *dev)
>
>         plat->reg = (struct s5p_uart *)addr;
>         plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
> -       plat->port_id = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
> -                                       "id", dev_seq(dev));
> +       plat->port_id = dev_read_u8_default(dev, "id", dev_seq(dev));
>
>         if (port_type == PORT_S5L) {
>                 plat->rx_fifo_count_shift = S5L_RX_FIFO_COUNT_SHIFT;
> --
> 2.39.2
>

Really this property should not be needed anymore. Can you figure out
how to drop it?

Regards,
Simon

Re: [PATCH 4/4] serial: s5p: Improve coding style

[Simon Glass]

On Tue, 7 Nov 2023 at 12:06, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Just some minor style fixes. No functional change.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/serial/serial_s5p.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
>

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

Re: [PATCH 3/4] serial: s5p: Use named constants for register values

[Simon Glass]

Hi Sam,

On Tue, 7 Nov 2023 at 12:06, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Get rid of magic numbers in s5p_serial_init() when writing to UART
> registers. While at it, use BIT() macro for existing constants when
> appropriate.
>
> No functional change.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/serial/serial_s5p.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)

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

Re: [PATCH 1/4] serial: s5p: Remove common.h inclusion

[Simon Glass]

On Tue, 7 Nov 2023 at 12:06, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> It's not really needed here anymore. Remove it, as common.h is going
> away at some point.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/serial/serial_s5p.c | 1 -
>  1 file changed, 1 deletion(-)

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

Re: [PATCH 2/4] serial: s5p: Use livetree API to get "id" property

[Sam Protsenko]

Hi Simon,

On Tue, Nov 7, 2023 at 10:26 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sam,
>
> On Tue, 7 Nov 2023 at 12:06, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> >
> > Use dev_read_u8_default() instead of fdtdec_get_int() to read the "id"
> > property from device tree, as suggested in [1]. dev_* API is already
> > used in this driver, so there is no reason to stick to fdtdec_* API.
> > This also fixes checkpatch warning:
> >
> >     WARNING: Use the livetree API (dev_read_...)
> >
> > [1] doc/develop/driver-model/livetree.rst
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/serial/serial_s5p.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
> > index 177215535676..c57bdd059ea6 100644
> > --- a/drivers/serial/serial_s5p.c
> > +++ b/drivers/serial/serial_s5p.c
> > @@ -20,8 +20,6 @@
> >  #include <serial.h>
> >  #include <clk.h>
> >
> > -DECLARE_GLOBAL_DATA_PTR;
> > -
> >  enum {
> >         PORT_S5P = 0,
> >         PORT_S5L
> > @@ -220,8 +218,7 @@ static int s5p_serial_of_to_plat(struct udevice *dev)
> >
> >         plat->reg = (struct s5p_uart *)addr;
> >         plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
> > -       plat->port_id = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
> > -                                       "id", dev_seq(dev));
> > +       plat->port_id = dev_read_u8_default(dev, "id", dev_seq(dev));
> >
> >         if (port_type == PORT_S5L) {
> >                 plat->rx_fifo_count_shift = S5L_RX_FIFO_COUNT_SHIFT;
> > --
> > 2.39.2
> >
>
> Really this property should not be needed anymore. Can you figure out
> how to drop it?
>

The 'port_id' seems to be needed for ARCH_EXYNOS4 boards. Because
Exynos4 doesn't have proper DM clocks, it uses 'id' property to get
corresponding UART clock frequency from its mach code.

Here is what's happening in the serial driver in case of Exynos4:

  1. get_uart_clk(port_id) is called
  2. which in turn calls exynos4_get_uart_clk(port_id)
  3. which uses "port_id" value to read corresponding bits of of
CLK_SRC_PERIL0 register
  4. those bits are used to get corresponding PLL clock's frequency
  5. which is in turn used to calculate UART clock rate
  6. calculated rate is returned by get_uart_clk() to serial driver

So I don't see any *easy* way we can get rid of that id property.

The proper way of doing that would require converting Exynos4 clock
code to CCF (enabling CONFIG_CLK_EXYNOS). Which of course also means
implementing clocks in dts, akin to kernel's exynos4.dtsi. Then it'll
be possible to get rid of 'id' property.

Maybe I'm missing something, please let me know.

Thanks!

> Regards,
> Simon

Re: [PATCH 2/4] serial: s5p: Use livetree API to get "id" property

[Simon Glass]

Hi Sam,

On Fri, 10 Nov 2023 at 11:29, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, Nov 7, 2023 at 10:26 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sam,
> >
> > On Tue, 7 Nov 2023 at 12:06, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> > >
> > > Use dev_read_u8_default() instead of fdtdec_get_int() to read the "id"
> > > property from device tree, as suggested in [1]. dev_* API is already
> > > used in this driver, so there is no reason to stick to fdtdec_* API.
> > > This also fixes checkpatch warning:
> > >
> > >     WARNING: Use the livetree API (dev_read_...)
> > >
> > > [1] doc/develop/driver-model/livetree.rst
> > >
> > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > > ---
> > >  drivers/serial/serial_s5p.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
> > > index 177215535676..c57bdd059ea6 100644
> > > --- a/drivers/serial/serial_s5p.c
> > > +++ b/drivers/serial/serial_s5p.c
> > > @@ -20,8 +20,6 @@
> > >  #include <serial.h>
> > >  #include <clk.h>
> > >
> > > -DECLARE_GLOBAL_DATA_PTR;
> > > -
> > >  enum {
> > >         PORT_S5P = 0,
> > >         PORT_S5L
> > > @@ -220,8 +218,7 @@ static int s5p_serial_of_to_plat(struct udevice *dev)
> > >
> > >         plat->reg = (struct s5p_uart *)addr;
> > >         plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
> > > -       plat->port_id = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
> > > -                                       "id", dev_seq(dev));
> > > +       plat->port_id = dev_read_u8_default(dev, "id", dev_seq(dev));
> > >
> > >         if (port_type == PORT_S5L) {
> > >                 plat->rx_fifo_count_shift = S5L_RX_FIFO_COUNT_SHIFT;
> > > --
> > > 2.39.2
> > >
> >
> > Really this property should not be needed anymore. Can you figure out
> > how to drop it?
> >
>
> The 'port_id' seems to be needed for ARCH_EXYNOS4 boards. Because
> Exynos4 doesn't have proper DM clocks, it uses 'id' property to get
> corresponding UART clock frequency from its mach code.
>
> Here is what's happening in the serial driver in case of Exynos4:
>
>   1. get_uart_clk(port_id) is called
>   2. which in turn calls exynos4_get_uart_clk(port_id)
>   3. which uses "port_id" value to read corresponding bits of of
> CLK_SRC_PERIL0 register
>   4. those bits are used to get corresponding PLL clock's frequency
>   5. which is in turn used to calculate UART clock rate
>   6. calculated rate is returned by get_uart_clk() to serial driver
>
> So I don't see any *easy* way we can get rid of that id property.
>
> The proper way of doing that would require converting Exynos4 clock
> code to CCF (enabling CONFIG_CLK_EXYNOS). Which of course also means
> implementing clocks in dts, akin to kernel's exynos4.dtsi. Then it'll
> be possible to get rid of 'id' property.

That sounds good!

>
> Maybe I'm missing something, please let me know.

An easy way in the meantime would be to look at the compatible / reg
property, e.g. here is a sketch:

static int get_id(ofnode node)
{
ulong addr = (ulong)plat->reg;
if (ofnode_device_is_compatible(node, "samsung,exynos4210-uart")) {
    return (addr >> 16) & 0xf;
...

reg = <0x13800000 0x3c>;
id = <0>;
};

serail_1: serial@13810000 {
compatible = "samsung,exynos4210-uart";
reg = <0x13810000 0x3c>;
id = <1>;
};

serial_2: serial@13820000 {
compatible = "samsung,exynos4210-uart";
reg = <0x13820000 0x3c>;
id = <2>;
};

serial_3: serial@13830000 {
compatible = "samsung,exynos4210-uart";
reg = <0x13830000 0x3c>;
id = <3>;
};

serial_4: serial@13840000 {
compatible = "samsung,exynos4210-uart";
reg = <0x13840000 0x3c>;
id = <4>;

Regards,
Simon

Re: [PATCH 2/4] serial: s5p: Use livetree API to get "id" property

[Sam Protsenko]

On Mon, Nov 13, 2023 at 12:01 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > The 'port_id' seems to be needed for ARCH_EXYNOS4 boards. Because
> > Exynos4 doesn't have proper DM clocks, it uses 'id' property to get
> > corresponding UART clock frequency from its mach code.
> >
> > Here is what's happening in the serial driver in case of Exynos4:
> >
> >   1. get_uart_clk(port_id) is called
> >   2. which in turn calls exynos4_get_uart_clk(port_id)
> >   3. which uses "port_id" value to read corresponding bits of of
> > CLK_SRC_PERIL0 register
> >   4. those bits are used to get corresponding PLL clock's frequency
> >   5. which is in turn used to calculate UART clock rate
> >   6. calculated rate is returned by get_uart_clk() to serial driver
> >
> > So I don't see any *easy* way we can get rid of that id property.
> >
> > The proper way of doing that would require converting Exynos4 clock
> > code to CCF (enabling CONFIG_CLK_EXYNOS). Which of course also means
> > implementing clocks in dts, akin to kernel's exynos4.dtsi. Then it'll
> > be possible to get rid of 'id' property.
>
> That sounds good!
>
> >
> > Maybe I'm missing something, please let me know.
>
> An easy way in the meantime would be to look at the compatible / reg
> property, e.g. here is a sketch:
>
> static int get_id(ofnode node)
> {
> ulong addr = (ulong)plat->reg;
> if (ofnode_device_is_compatible(node, "samsung,exynos4210-uart")) {
>     return (addr >> 16) & 0xf;
> ...
>
> reg = <0x13800000 0x3c>;
> id = <0>;
> };
>
> serail_1: serial@13810000 {
> compatible = "samsung,exynos4210-uart";
> reg = <0x13810000 0x3c>;
> id = <1>;
> };
>
> serial_2: serial@13820000 {
> compatible = "samsung,exynos4210-uart";
> reg = <0x13820000 0x3c>;
> id = <2>;
> };
>
> serial_3: serial@13830000 {
> compatible = "samsung,exynos4210-uart";
> reg = <0x13830000 0x3c>;
> id = <3>;
> };
>
> serial_4: serial@13840000 {
> compatible = "samsung,exynos4210-uart";
> reg = <0x13840000 0x3c>;
> id = <4>;
>

To be honest, I'm a bit of busy right now working on U-Boot port for a
new Exynos dev board, which I want to finalize and upstream ASAP. Do
you think it's possible to take this one as is for now? At least it
fixes one issue (fdtdec API and global data pointer).

Thanks!

> Regards,
> Simon

Re: [PATCH 0/4] serial: s5p: Cleanups

[Sam Protsenko]

On Tue, Nov 7, 2023 at 1:06 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> A small collection of cleanup patches for serial_s5p.c driver. Should
> induce no functional changes.
>
> Sam Protsenko (4):
>   serial: s5p: Remove common.h inclusion
>   serial: s5p: Use livetree API to get "id" property
>   serial: s5p: Use named constants for register values
>   serial: s5p: Improve coding style
>
>  drivers/serial/serial_s5p.c | 71 +++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 31 deletions(-)
>
> --

+ Minkyu

Just a kind reminder to review this series.

Thanks!

> 2.39.2
>

Re: [PATCH 2/4] serial: s5p: Use livetree API to get "id" property

[Simon Glass]

On Tue, 21 Nov 2023 at 10:32, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> On Mon, Nov 13, 2023 at 12:01 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > The 'port_id' seems to be needed for ARCH_EXYNOS4 boards. Because
> > > Exynos4 doesn't have proper DM clocks, it uses 'id' property to get
> > > corresponding UART clock frequency from its mach code.
> > >
> > > Here is what's happening in the serial driver in case of Exynos4:
> > >
> > >   1. get_uart_clk(port_id) is called
> > >   2. which in turn calls exynos4_get_uart_clk(port_id)
> > >   3. which uses "port_id" value to read corresponding bits of of
> > > CLK_SRC_PERIL0 register
> > >   4. those bits are used to get corresponding PLL clock's frequency
> > >   5. which is in turn used to calculate UART clock rate
> > >   6. calculated rate is returned by get_uart_clk() to serial driver
> > >
> > > So I don't see any *easy* way we can get rid of that id property.
> > >
> > > The proper way of doing that would require converting Exynos4 clock
> > > code to CCF (enabling CONFIG_CLK_EXYNOS). Which of course also means
> > > implementing clocks in dts, akin to kernel's exynos4.dtsi. Then it'll
> > > be possible to get rid of 'id' property.
> >
> > That sounds good!
> >
> > >
> > > Maybe I'm missing something, please let me know.
> >
> > An easy way in the meantime would be to look at the compatible / reg
> > property, e.g. here is a sketch:
> >
> > static int get_id(ofnode node)
> > {
> > ulong addr = (ulong)plat->reg;
> > if (ofnode_device_is_compatible(node, "samsung,exynos4210-uart")) {
> >     return (addr >> 16) & 0xf;
> > ...
> >
> > reg = <0x13800000 0x3c>;
> > id = <0>;
> > };
> >
> > serail_1: serial@13810000 {
> > compatible = "samsung,exynos4210-uart";
> > reg = <0x13810000 0x3c>;
> > id = <1>;
> > };
> >
> > serial_2: serial@13820000 {
> > compatible = "samsung,exynos4210-uart";
> > reg = <0x13820000 0x3c>;
> > id = <2>;
> > };
> >
> > serial_3: serial@13830000 {
> > compatible = "samsung,exynos4210-uart";
> > reg = <0x13830000 0x3c>;
> > id = <3>;
> > };
> >
> > serial_4: serial@13840000 {
> > compatible = "samsung,exynos4210-uart";
> > reg = <0x13840000 0x3c>;
> > id = <4>;
> >
>
> To be honest, I'm a bit of busy right now working on U-Boot port for a
> new Exynos dev board, which I want to finalize and upstream ASAP. Do
> you think it's possible to take this one as is for now? At least it
> fixes one issue (fdtdec API and global data pointer).
>
> Thanks!

Yes, OK with me.

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

Re: [PATCH 0/4] serial: s5p: Cleanups

[Minkyu Kang]

Hi!

2023년 11월 22일 (수) 02:40, Sam Protsenko <semen.protsenko@linaro.org>님이 작성:

> On Tue, Nov 7, 2023 at 1:06 PM Sam Protsenko <semen.protsenko@linaro.org>
> wrote:
> >
> > A small collection of cleanup patches for serial_s5p.c driver. Should
> > induce no functional changes.
> >
> > Sam Protsenko (4):
> >   serial: s5p: Remove common.h inclusion
> >   serial: s5p: Use livetree API to get "id" property
> >   serial: s5p: Use named constants for register values
> >   serial: s5p: Improve coding style
> >
> >  drivers/serial/serial_s5p.c | 71 +++++++++++++++++++++----------------
> >  1 file changed, 40 insertions(+), 31 deletions(-)
> >
> > --
>
> + Minkyu
>
> Just a kind reminder to review this series.
>
> Thanks!
>
> > 2.39.2
> >


applied to u-boot-samsung.

Thanks.
Minkyu Kang.