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

[Simon Glass]

Hi Bin,

On 17 August 2015 at 20:25, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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.

However it gets changed it will need to work correctly so you don't
need to worry about that.

>
>>>
>>>>> +       {}
>>>>> +};
>>>>> +
>>>>> +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?

Well if it is in the device tree you don't need to use
U_BOOT_PCI_DEVICE. In pci_bind_bus_devices()  it searches the device
tree first and only resorts to pci_find_and_bind_driver() if that
fails.

[snip]

Regards,
Simon