[U-Boot] [PATCH 5/8] drivers: serial: Add ns16550 compatible pci uart driver

Bin Meng bmeng.cn at gmail.com
Tue Aug 18 04:25:19 CEST 2015


Hi Simon,

On Tue, Aug 18, 2015 at 10:00 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 17 August 2015 at 18:20, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Simon,
>>
>> On Tue, Aug 18, 2015 at 6:14 AM, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Bin,
>>>
>>> On 15 August 2015 at 01:07, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> This adds a new driver to support National Semiconductor 16550
>>>> compatible UART device with PCI interface. The initial support
>>>> only adds device IDs for Intel Topcliff chipset UART devices.
>>>>
>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>> ---
>>>>
>>>>  drivers/serial/Kconfig      |  9 ++++++
>>>>  drivers/serial/Makefile     |  1 +
>>>>  drivers/serial/serial_pci.c | 75 +++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 85 insertions(+)
>>>>  create mode 100644 drivers/serial/serial_pci.c
>>>>
>>>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>>>> index fd126a8..f2eccdd 100644
>>>> --- a/drivers/serial/Kconfig
>>>> +++ b/drivers/serial/Kconfig
>>>> @@ -128,3 +128,12 @@ config X86_SERIAL
>>>>           enabled in the device tree with the correct input clock frequency
>>>>           provided (default 1843200). Enable this to obtain serial console
>>>>           output.
>>>> +
>>>> +config PCI_SERIAL
>>>> +       bool "Support for 16550 serial port on PCI bus"
>>>> +       depends on DM_PCI
>>>> +       default n
>>>> +       help
>>>> +         This is the UART driver for ns16550 compatible chipset with PCI
>>>> +         interface. This can be enabled in the device tree with the correct
>>>> +         properties provided. If unsure, say N.
>>>> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
>>>> index 1d1f036..a7e2cd2 100644
>>>> --- a/drivers/serial/Makefile
>>>> +++ b/drivers/serial/Makefile
>>>> @@ -45,6 +45,7 @@ obj-$(CONFIG_TEGRA_SERIAL) += serial_tegra.o
>>>>  obj-$(CONFIG_UNIPHIER_SERIAL) += serial_uniphier.o
>>>>  obj-$(CONFIG_OMAP_SERIAL) += serial_omap.o
>>>>  obj-$(CONFIG_X86_SERIAL) += serial_x86.o
>>>> +obj-$(CONFIG_PCI_SERIAL) += serial_pci.o
>>>>  obj-$(CONFIG_STM32_SERIAL) += serial_stm32.o
>>>>
>>>>  ifndef CONFIG_SPL_BUILD
>>>> diff --git a/drivers/serial/serial_pci.c b/drivers/serial/serial_pci.c
>>>> new file mode 100644
>>>> index 0000000..bc87c9a
>>>> --- /dev/null
>>>> +++ b/drivers/serial/serial_pci.c
>>>> @@ -0,0 +1,75 @@
>>>> +/*
>>>> + * Copyright (C) 2015, Bin Meng <bmeng.cn at gmail.com>
>>>> + *
>>>> + * This driver aims to support National Semiconductor 16550 compatible
>>>> + * UART device with PCI interface.
>>>> + *
>>>> + * SPDX-License-Identifier:    GPL-2.0+
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <dm.h>
>>>> +#include <fdtdec.h>
>>>> +#include <ns16550.h>
>>>> +#include <serial.h>
>>>> +
>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>> +
>>>> +static struct pci_device_id supported[] = {
>>>> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TCF_UART_0) },
>>>> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TCF_UART_1) },
>>>> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TCF_UART_2) },
>>>> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TCF_UART_3) },
>>>
>>> Ick, did I miss the discussion on this? I really want us to keep this
>>> stuff in the device tree.
>>>
>>
>> I don't think you missed any discussion. But I think you might misread
>> this code? Do you mean there is no need to declare what devices are
>> supported by this driver using U_BOOT_PCI_DEVICE?
>
> OK but we have gone from a generic driver that worked with anything to
> one where you have to enable it for every ID on every board. How is
> that an improvement?
>

Agreed, but given fdtdec_get_addr() is broken (see previous
discussion), I decided to split that from the generic 16550 driver.
Although I see you revert that commit, I am not sure how
fdtdec_get_addr() will be changed in the future.

>>
>>>> +       {}
>>>> +};
>>>> +
>>>> +static const struct udevice_id pci_serial_ids[] = {
>>>> +       { .compatible = "pci-uart" },
>>>> +       { }
>>>> +};
>>>> +
>>>> +static int pci_serial_ofdata_to_platdata(struct udevice *dev)
>>>> +{
>>>> +       struct ns16550_platdata *plat = dev_get_platdata(dev);
>>>> +       struct fdt_pci_addr pci_addr;
>>>> +       u32 bar;
>>>> +       int ret;
>
> If we have a device tree node, why do we need the U_BOOT_PCI_DEVICE() stuff?
>

This is to have pci bus to filter the device creation. I see if I
define U_BOOT_PCI_DEVICE() and with a device node at the same time, it
will show twice in the 'dm tree' output. One with a driver name
'pci_serial' and the other one with 'serial'. Maybe I am using it
wrong?

>>>> +
>>>> +       /* we prefer to use a memory-mapped register */
>>>> +       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 i/o-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;
>>>> +
>>>> +       plat->base = bar;
>>>> +       plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>>>> +                                        "reg-shift", 1);
>>>> +       plat->clock = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>>>> +                                    "clock-frequency", 1843200);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +U_BOOT_DRIVER(serial_pci) = {
>>>> +       .name = "serial_pci",
>>>> +       .id = UCLASS_SERIAL,
>>>> +       .of_match = pci_serial_ids,
>>>> +       .ofdata_to_platdata = pci_serial_ofdata_to_platdata,
>>>> +       .platdata_auto_alloc_size = sizeof(struct ns16550_platdata),
>>>> +       .priv_auto_alloc_size = sizeof(struct NS16550),
>>>> +       .probe = ns16550_serial_probe,
>>>> +       .ops = &ns16550_serial_ops,
>>>> +       .flags = DM_FLAG_PRE_RELOC,
>>>> +};
>>>> +
>>>> +U_BOOT_PCI_DEVICE(serial_pci, supported);
>>>> --
>>

Regards,
Bin


More information about the U-Boot mailing list