Hi Bin, On 17 August 2015 at 20:25, Bin Meng <bmeng...@gmail.com> wrote: > Hi Simon, > > On Tue, Aug 18, 2015 at 10:00 AM, Simon Glass <s...@chromium.org> wrote: >> Hi Bin, >> >> On 17 August 2015 at 18:20, Bin Meng <bmeng...@gmail.com> wrote: >>> Hi Simon, >>> >>> On Tue, Aug 18, 2015 at 6:14 AM, Simon Glass <s...@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> On 15 August 2015 at 01:07, Bin Meng <bmeng...@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...@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...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot