[U-Boot] [PATCH v2 5/7] serial: ns16550: Support ns16550 compatible pci uart devices

[Simon Glass]

Hi Bin,

On 28 December 2014 at 23:15, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Sun, Dec 28, 2014 at 9:55 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 27 December 2014 at 05:10, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> There are many pci uart devices which are ns16550 compatible. We can
>>> describe them in the board dts file and use it as the U-Boot serial
>>> console as specified in the chosen node 'stdout-path' property.
>>>
>>> Those pci uart devices can have their register be memory mapped, or
>>
>> memory-mapped
>>
>>> i/o mapped. The driver will try to use memory mapped register if the
>>
>> i/o-mapped
>>
>> s/memory mapped/the memory-mapped/
>>
>>> reg property in the node has an entry to describe the memory mapped
>>
>> memory-mapped
>>
>>> register, otherwise i/o mapped register will be used.
>>
>> i/o-mapped
>>
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - New patch to support ns16550 compatible pci uart devices
>>>
>>>  drivers/serial/ns16550.c | 33 ++++++++++++++++++++++++++++++++-
>>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>> index af5beba..2001fac 100644
>>> --- a/drivers/serial/ns16550.c
>>> +++ b/drivers/serial/ns16550.c
>>> @@ -289,10 +289,41 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
>>>         struct ns16550_platdata *plat = dev->platdata;
>>>         fdt_addr_t addr;
>>>
>>> +       /* try plb device first */
>>
>> What is plb?
>
> I mean Processor Local Bus, a concept I believe is popular in the
> embedded powerpc world, but I realized it might not be that popuar to
> other architectures. Maybe I could just remove it.

Seems fine, but might want to write it out in full.

>
>>>         addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>>> -       if (addr == FDT_ADDR_T_NONE)
>>> +       if (addr == FDT_ADDR_T_NONE) {
>>> +#ifdef CONFIG_PCI
>>> +               /* then try pci device */
>>> +               struct fdt_pci_addr pci_addr;
>>> +               u32 bar;
>>> +               int ret;
>>> +
>>> +               /* we prefer to use memory mapped register */
>>
>> a memory-mapped
>>
>>> +               ret = fdtdec_get_pci_addr(gd->fdt_blob, dev->of_offset,
>>> +                                         FDT_PCI_SPACE_MEM32, "reg",
>>> +                                         &pci_addr);
>>> +               if (ret) {
>>> +                       /* try if there is any io mapped register */
>>> +                       ret = fdtdec_get_pci_addr(gd->fdt_blob,
>>> +                                                 dev->of_offset,
>>> +                                                 FDT_PCI_SPACE_IO,
>>> +                                                 "reg", &pci_addr);
>>> +                       if (ret)
>>> +                               return ret;
>>> +               }
>>> +
>>> +               ret = fdtdec_get_pci_bar32(gd->fdt_blob, dev->of_offset,
>>> +                                          &pci_addr, &bar);
>>> +               if (ret)
>>> +                       return ret;
>>> +
>>> +               addr = bar;
>>> +               goto cont;
>>> +#endif
>>>                 return -EINVAL;
>>> +       }
>>
>> Instead of the above 4 lines, move the #ifdef CONFIG_PCI up one line, then here:
>>
>>    }
>> if (addr == FDT_ADDR_T_NONE)
>>    return -EINVAL;
>>
>> This avoids the goto.
>
> Yep, this is better. Will fix it.
>
>>>
>>> +cont:
>>>         plat->base = addr;
>>>         plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>>>                                          "reg-shift", 1);
>>> --
>>> 1.8.2.1

Regards,
Simon