[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 at linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, Nov 7, 2023 at 10:26 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sam,
> >
> > On Tue, 7 Nov 2023 at 12:06, Sam Protsenko <semen.protsenko at 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 at 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 at 13810000 {
compatible = "samsung,exynos4210-uart";
reg = <0x13810000 0x3c>;
id = <1>;
};

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

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

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

Regards,
Simon