| Message ID | 1439622470-10803-5-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Simon, On Sat, Aug 15, 2015 at 3:07 PM, Bin Meng <bmeng.cn@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@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@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) }, > + {} > +}; > + > +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; > + > + /* 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); > -- Just found that this patch needs to be rebased on latest u-boot-x86/master in order to apply cleanly. Will send v2 after the review. Regards, Bin
Hi Bin, On 15 August 2015 at 01:07, Bin Meng <bmeng.cn@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@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@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. > + {} > +}; > + > +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; > + > + /* 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); > -- > 1.8.2.1 > Regards, Simon
Hi Simon, On Tue, Aug 18, 2015 at 6:14 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 15 August 2015 at 01:07, Bin Meng <bmeng.cn@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@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@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? >> + {} >> +}; >> + >> +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; >> + >> + /* 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
Hi Bin, On 17 August 2015 at 18:20, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Tue, Aug 18, 2015 at 6:14 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 15 August 2015 at 01:07, Bin Meng <bmeng.cn@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@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@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? > >>> + {} >>> +}; >>> + >>> +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? >>> + >>> + /* 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 Regards, Simon
Hi Simon, On Tue, Aug 18, 2015 at 10:00 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 17 August 2015 at 18:20, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Tue, Aug 18, 2015 at 6:14 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> On 15 August 2015 at 01:07, Bin Meng <bmeng.cn@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@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@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
Hi Bin, On 17 August 2015 at 20:25, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Tue, Aug 18, 2015 at 10:00 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 17 August 2015 at 18:20, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Tue, Aug 18, 2015 at 6:14 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> On 15 August 2015 at 01:07, Bin Meng <bmeng.cn@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@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@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
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@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) }, + {} +}; + +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; + + /* 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);
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@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