| Message ID | 20231107190601.17151-3-semen.protsenko@linaro.org |
|---|---|
| State | Accepted |
| Commit | 5ad21de6bae0a6d51d4b0ff8eedfb795ed2d4581 |
| Headers | show |
| Series | serial: s5p: Cleanups | expand |
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
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
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
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
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>
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;
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(-)