[U-Boot] [PATCH] serial: fdt: add device tree support for pl01x

[vikasm <vikas.manocha@st.com>]

Hello Masahiro,

On 05/01/2015 03:32 PM, vikasm wrote:
> Thanks Simon,
>
> On 05/01/2015 03:02 PM, Simon Glass wrote:
>> +Masahiro, for my of_match_ptr() comment below.
>>
>> Hi Vikas,
>>
>> On 1 May 2015 at 15:48, Vikas Manocha <vikas.manocha@st.com> wrote:
>>> This patch adds device tree support for arm pl010/pl011 driver.
>>>
>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>>> ---
>>>  doc/device-tree-bindings/serial/pl01x.txt |    7 +++++
>>>  drivers/serial/serial_pl01x.c             |   41 ++++++++++++++++++++++++++++-
>>>  2 files changed, 47 insertions(+), 1 deletion(-)
>>>  create mode 100644 doc/device-tree-bindings/serial/pl01x.txt
>>>
>>> diff --git a/doc/device-tree-bindings/serial/pl01x.txt b/doc/device-tree-bindings/serial/pl01x.txt
>>> new file mode 100644
>>> index 0000000..61c27d1
>>> --- /dev/null
>>> +++ b/doc/device-tree-bindings/serial/pl01x.txt
>>> @@ -0,0 +1,7 @@
>>> +* ARM AMBA Primecell PL011 & PL010 serial UART
>>> +
>>> +Required properties:
>>> +- compatible: must be "arm,primecell", "arm,pl011" or "arm,pl010"
>>> +- reg: exactly one register range with length 0x1000
>>> +- clock: input clock frequency for the UART (used to calculate the baud
>>> +  rate divisor)
>>> diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c
>>> index 2124161..ea22cfd 100644
>>> --- a/drivers/serial/serial_pl01x.c
>>> +++ b/drivers/serial/serial_pl01x.c
>>> @@ -20,6 +20,9 @@
>>>  #include <dm/platform_data/serial_pl01x.h>
>>>  #include <linux/compiler.h>
>>>  #include "serial_pl01x_internal.h"
>>> +#include <fdtdec.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>>
>>>  #ifndef CONFIG_DM_SERIAL
>>>
>>> @@ -28,7 +31,6 @@ static enum pl01x_type pl01x_type __attribute__ ((section(".data")));
>>>  static struct pl01x_regs *base_regs __attribute__ ((section(".data")));
>>>  #define NUM_PORTS (sizeof(port)/sizeof(port[0]))
>>>
>>> -DECLARE_GLOBAL_DATA_PTR;
>>>  #endif
>>>
>>>  static int pl01x_putc(struct pl01x_regs *regs, char c)
>>> @@ -351,9 +353,46 @@ static const struct dm_serial_ops pl01x_serial_ops = {
>>>         .setbrg = pl01x_serial_setbrg,
>>>  };
>>>
>>> +#ifdef CONFIG_OF_CONTROL
>>> +static const struct udevice_id pl01x_serial_id[] ={
>>> +       {.compatible = "arm,pl011"},
>>> +       {.compatible = "arm,pl010"},
>> You can use:
>>
>>       {.compatible = "arm,pl011", .data = TYPE_PL011},
> Thanks for the suggestion.
>
>>> +       {}
>>> +};
>>> +
>>> +static int pl01x_serial_ofdata_to_platdata(struct udevice *dev)
>>> +{
>>> +       struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
>>> +       fdt_addr_t addr;
>>> +       const char* type_pl01x;
>>> +
>>> +       addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>>> +       if (addr == FDT_ADDR_T_NONE)
>>> +               return -EINVAL;
>>> +
>>> +       plat->base = addr;
>>> +       plat->clock = fdtdec_get_int(gd->fdt_blob, dev->of_offset, "clock", 1);
>>> +       type_pl01x = fdt_getprop(gd->fdt_blob, dev->of_offset, \
>>> +                       "compatible", NULL);
>> plat->type = dev_get_driver_data(dev);
> completely agree, it would make the code much cleaner.
>
>>> +       if(!strcmp(type_pl01x, "arm,pl011"))
>>> +               plat->type = TYPE_PL011;
>>> +       else if(!strcmp(type_pl01x, "arm,pl010"))
>>> +               plat->type = TYPE_PL010;
>>> +       else
>>> +               return -EINVAL;
>> Should be able to drop this line.
> yes, all the above block.
>
>>> +
>>> +       return 0;
>>> +}
>>> +#endif
>>> +
>>>  U_BOOT_DRIVER(serial_pl01x) = {
>>>         .name   = "serial_pl01x",
>>>         .id     = UCLASS_SERIAL,
>>> +#ifdef CONFIG_OF_CONTROL
>>> +       .of_match = pl01x_serial_id,
>>> +       .ofdata_to_platdata = pl01x_serial_ofdata_to_platdata,
>>> +       .platdata_auto_alloc_size = sizeof(struct pl01x_serial_platdata),
>>> +#endif
>> Would be good to get rid of the #ifdef.
>>
>> I think you can put the last one outside the #ifdef, since
>> device_bind() will do the right thing if there is already platform
>> data.
> ok.
>
>> For the first one you can do   .of_match = of_match_ptr(pl01x_serial_id),
> ok, i will check it.
>
> Rgds,
> Vikas
>
>> But the middle line (ofdata_to_platdata) does need an #ifdef I think.
>> Perhaps we could create something similar to of_match_ptr() for this
>> case?

Any suggestion on this point..can we use of_match_ptr() ?

Rgds,
Vikas

>>
>>>         .probe = pl01x_serial_probe,
>>>         .ops    = &pl01x_serial_ops,
>>>         .flags = DM_FLAG_PRE_RELOC,
>>> --
>>> 1.7.9.5
>>>
>> Regards,
>> Simon