| Message ID | 1447940895-7763-6-git-send-email-thomas@wytron.com.tw |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Thomas, On 19 November 2015 at 06:48, Thomas Chou <thomas@wytron.com.tw> wrote: > Unify serial_ppc, and use the generic binding. > > Signed-off-by: Thomas Chou <thomas@wytron.com.tw> > Reviewed-by: Tom Rini <trini@konsulko.com> > --- > arch/powerpc/include/asm/config.h | 4 ++++ > drivers/serial/Kconfig | 2 +- > drivers/serial/Makefile | 1 - > drivers/serial/serial_ppc.c | 40 --------------------------------------- > 4 files changed, 5 insertions(+), 42 deletions(-) > delete mode 100644 drivers/serial/serial_ppc.c > > diff --git a/arch/powerpc/include/asm/config.h b/arch/powerpc/include/asm/config.h > index 65496d0..7391066 100644 > --- a/arch/powerpc/include/asm/config.h > +++ b/arch/powerpc/include/asm/config.h > @@ -104,4 +104,8 @@ > /* All PPC boards must swap IDE bytes */ > #define CONFIG_IDE_SWAP_IO > > +#if defined(CONFIG_DM_SERIAL) > +#define CONFIG_SYS_NS16550_CLK get_serial_clock() > +#endif > + > #endif /* _ASM_CONFIG_H_ */ > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig > index 93faa4c..b41f508 100644 > --- a/drivers/serial/Kconfig > +++ b/drivers/serial/Kconfig > @@ -198,7 +198,7 @@ config ROCKCHIP_SERIAL > config NS16550_SERIAL > bool "NS16550 UART or compatible" > depends on DM_SERIAL > - default y if X86 > + default y if X86 || PPC > help > Support NS16550 UART or compatible with driver model. This can be > enabled in the device tree with the correct input clock frequency. > diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile > index 9036a8e..9f61113 100644 > --- a/drivers/serial/Makefile > +++ b/drivers/serial/Makefile > @@ -8,7 +8,6 @@ > ifdef CONFIG_DM_SERIAL > obj-y += serial-uclass.o > obj-$(CONFIG_PL01X_SERIAL) += serial_pl01x.o > -obj-$(CONFIG_PPC) += serial_ppc.o > else > obj-y += serial.o > obj-$(CONFIG_PL010_SERIAL) += serial_pl01x.o > diff --git a/drivers/serial/serial_ppc.c b/drivers/serial/serial_ppc.c > deleted file mode 100644 > index 47141c6..0000000 > --- a/drivers/serial/serial_ppc.c > +++ /dev/null > @@ -1,40 +0,0 @@ > -/* > - * Copyright (c) 2014 Google, Inc > - * > - * SPDX-License-Identifier: GPL-2.0+ > - */ > - > -#include <common.h> > -#include <dm.h> > -#include <ns16550.h> > -#include <serial.h> > - > -static const struct udevice_id ppc_serial_ids[] = { > - { .compatible = "ns16550" }, > - { } > -}; > - > -static int ppc_serial_ofdata_to_platdata(struct udevice *dev) > -{ > - struct ns16550_platdata *plat = dev_get_platdata(dev); > - int ret; > - > - ret = ns16550_serial_ofdata_to_platdata(dev); > - if (ret) > - return ret; > - plat->clock = get_serial_clock(); You are dropping this call. We certainly don't want it for driver model, but I suspect it will break some PPC boards if they don't have the clock-frequency in the device tree. Do they? > - > - return 0; > -} > - > -U_BOOT_DRIVER(serial_ns16550) = { > - .name = "serial_ppc", > - .id = UCLASS_SERIAL, > - .of_match = ppc_serial_ids, > - .ofdata_to_platdata = ppc_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, > -}; > -- > 2.5.0 > Regards, Simon
Hi Simon, On 2015年11月21日 01:18, Simon Glass wrote: > Hi Thomas, > > On 19 November 2015 at 06:48, Thomas Chou <thomas@wytron.com.tw> wrote: >> Unify serial_ppc, and use the generic binding. >> >> Signed-off-by: Thomas Chou <thomas@wytron.com.tw> >> Reviewed-by: Tom Rini <trini@konsulko.com> >> --- >> arch/powerpc/include/asm/config.h | 4 ++++ >> drivers/serial/Kconfig | 2 +- >> drivers/serial/Makefile | 1 - >> drivers/serial/serial_ppc.c | 40 --------------------------------------- >> 4 files changed, 5 insertions(+), 42 deletions(-) >> delete mode 100644 drivers/serial/serial_ppc.c >> >> diff --git a/arch/powerpc/include/asm/config.h b/arch/powerpc/include/asm/config.h >> index 65496d0..7391066 100644 >> --- a/arch/powerpc/include/asm/config.h >> +++ b/arch/powerpc/include/asm/config.h >> @@ -104,4 +104,8 @@ >> /* All PPC boards must swap IDE bytes */ >> #define CONFIG_IDE_SWAP_IO >> >> +#if defined(CONFIG_DM_SERIAL) >> +#define CONFIG_SYS_NS16550_CLK get_serial_clock() >> +#endif >> + I move the get_serial_clock here. >> #endif /* _ASM_CONFIG_H_ */ >> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig >> index 93faa4c..b41f508 100644 >> --- a/drivers/serial/Kconfig >> +++ b/drivers/serial/Kconfig >> @@ -198,7 +198,7 @@ config ROCKCHIP_SERIAL >> config NS16550_SERIAL >> bool "NS16550 UART or compatible" >> depends on DM_SERIAL >> - default y if X86 >> + default y if X86 || PPC >> help >> Support NS16550 UART or compatible with driver model. This can be >> enabled in the device tree with the correct input clock frequency. >> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile >> index 9036a8e..9f61113 100644 >> --- a/drivers/serial/Makefile >> +++ b/drivers/serial/Makefile >> @@ -8,7 +8,6 @@ >> ifdef CONFIG_DM_SERIAL >> obj-y += serial-uclass.o >> obj-$(CONFIG_PL01X_SERIAL) += serial_pl01x.o >> -obj-$(CONFIG_PPC) += serial_ppc.o >> else >> obj-y += serial.o >> obj-$(CONFIG_PL010_SERIAL) += serial_pl01x.o >> diff --git a/drivers/serial/serial_ppc.c b/drivers/serial/serial_ppc.c >> deleted file mode 100644 >> index 47141c6..0000000 >> --- a/drivers/serial/serial_ppc.c >> +++ /dev/null >> @@ -1,40 +0,0 @@ >> -/* >> - * Copyright (c) 2014 Google, Inc >> - * >> - * SPDX-License-Identifier: GPL-2.0+ >> - */ >> - >> -#include <common.h> >> -#include <dm.h> >> -#include <ns16550.h> >> -#include <serial.h> >> - >> -static const struct udevice_id ppc_serial_ids[] = { >> - { .compatible = "ns16550" }, >> - { } >> -}; >> - >> -static int ppc_serial_ofdata_to_platdata(struct udevice *dev) >> -{ >> - struct ns16550_platdata *plat = dev_get_platdata(dev); >> - int ret; >> - >> - ret = ns16550_serial_ofdata_to_platdata(dev); >> - if (ret) >> - return ret; >> - plat->clock = get_serial_clock(); > > You are dropping this call. We certainly don't want it for driver > model, but I suspect it will break some PPC boards if they don't have > the clock-frequency in the device tree. Do they? I moved it to a macro. #define CONFIG_SYS_NS16550_CLK get_serial_clock() Best regards, Thomas
Hi Thomas, On 20 November 2015 at 17:19, Thomas Chou <thomas@wytron.com.tw> wrote: > > Hi Simon, > > On 2015年11月21日 01:18, Simon Glass wrote: >> >> Hi Thomas, >> >> On 19 November 2015 at 06:48, Thomas Chou <thomas@wytron.com.tw> wrote: >>> >>> Unify serial_ppc, and use the generic binding. >>> >>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw> >>> Reviewed-by: Tom Rini <trini@konsulko.com> >>> --- >>> arch/powerpc/include/asm/config.h | 4 ++++ >>> drivers/serial/Kconfig | 2 +- >>> drivers/serial/Makefile | 1 - >>> drivers/serial/serial_ppc.c | 40 --------------------------------------- >>> 4 files changed, 5 insertions(+), 42 deletions(-) >>> delete mode 100644 drivers/serial/serial_ppc.c >>> >>> diff --git a/arch/powerpc/include/asm/config.h b/arch/powerpc/include/asm/config.h >>> index 65496d0..7391066 100644 >>> --- a/arch/powerpc/include/asm/config.h >>> +++ b/arch/powerpc/include/asm/config.h >>> @@ -104,4 +104,8 @@ >>> /* All PPC boards must swap IDE bytes */ >>> #define CONFIG_IDE_SWAP_IO >>> >>> +#if defined(CONFIG_DM_SERIAL) >>> +#define CONFIG_SYS_NS16550_CLK get_serial_clock() >>> +#endif >>> + > > > I move the get_serial_clock here. > > >>> #endif /* _ASM_CONFIG_H_ */ >>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig >>> index 93faa4c..b41f508 100644 >>> --- a/drivers/serial/Kconfig >>> +++ b/drivers/serial/Kconfig >>> @@ -198,7 +198,7 @@ config ROCKCHIP_SERIAL >>> config NS16550_SERIAL >>> bool "NS16550 UART or compatible" >>> depends on DM_SERIAL >>> - default y if X86 >>> + default y if X86 || PPC >>> help >>> Support NS16550 UART or compatible with driver model. This can be >>> enabled in the device tree with the correct input clock frequency. >>> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile >>> index 9036a8e..9f61113 100644 >>> --- a/drivers/serial/Makefile >>> +++ b/drivers/serial/Makefile >>> @@ -8,7 +8,6 @@ >>> ifdef CONFIG_DM_SERIAL >>> obj-y += serial-uclass.o >>> obj-$(CONFIG_PL01X_SERIAL) += serial_pl01x.o >>> -obj-$(CONFIG_PPC) += serial_ppc.o >>> else >>> obj-y += serial.o >>> obj-$(CONFIG_PL010_SERIAL) += serial_pl01x.o >>> diff --git a/drivers/serial/serial_ppc.c b/drivers/serial/serial_ppc.c >>> deleted file mode 100644 >>> index 47141c6..0000000 >>> --- a/drivers/serial/serial_ppc.c >>> +++ /dev/null >>> @@ -1,40 +0,0 @@ >>> -/* >>> - * Copyright (c) 2014 Google, Inc >>> - * >>> - * SPDX-License-Identifier: GPL-2.0+ >>> - */ >>> - >>> -#include <common.h> >>> -#include <dm.h> >>> -#include <ns16550.h> >>> -#include <serial.h> >>> - >>> -static const struct udevice_id ppc_serial_ids[] = { >>> - { .compatible = "ns16550" }, >>> - { } >>> -}; >>> - >>> -static int ppc_serial_ofdata_to_platdata(struct udevice *dev) >>> -{ >>> - struct ns16550_platdata *plat = dev_get_platdata(dev); >>> - int ret; >>> - >>> - ret = ns16550_serial_ofdata_to_platdata(dev); >>> - if (ret) >>> - return ret; >>> - plat->clock = get_serial_clock(); >> >> >> You are dropping this call. We certainly don't want it for driver >> model, but I suspect it will break some PPC boards if they don't have >> the clock-frequency in the device tree. Do they? > > > I moved it to a macro. > #define CONFIG_SYS_NS16550_CLK get_serial_clock() Ah, we shouldn't do that with driver model. This should be in some sort of clock driver. Perhaps we should leave this serial driver along for now? Regards, Simon
Hi Simon, On 2015年11月21日 08:27, Simon Glass wrote: >> I moved it to a macro. >> #define CONFIG_SYS_NS16550_CLK get_serial_clock() > > Ah, we shouldn't do that with driver model. This should be in some > sort of clock driver. Perhaps we should leave this serial driver along > for now? I understand a clock driver should be used for driver model. But I would suggest that we use this get_serial_clock macro until the clock driver for powerpc is ready to get UART clock. This will help things moving forwards to driver model. Best regards, Thomas
On Sat, Nov 21, 2015 at 10:39:54AM +0800, Thomas Chou wrote: > Hi Simon, > > On 2015年11月21日 08:27, Simon Glass wrote: > >>I moved it to a macro. > >>#define CONFIG_SYS_NS16550_CLK get_serial_clock() > > > >Ah, we shouldn't do that with driver model. This should be in some > >sort of clock driver. Perhaps we should leave this serial driver along > >for now? > > I understand a clock driver should be used for driver model. But I > would suggest that we use this get_serial_clock macro until the > clock driver for powerpc is ready to get UART clock. This will help > things moving forwards to driver model. Yes, how do you want to proceed here Simon? I have all of these (and a few other things) build-tested now and will be doing some quick sanity run time testing later. Thanks!
Hi Tom, On 21 November 2015 at 08:22, Tom Rini <trini@konsulko.com> wrote: > On Sat, Nov 21, 2015 at 10:39:54AM +0800, Thomas Chou wrote: >> Hi Simon, >> >> On 2015年11月21日 08:27, Simon Glass wrote: >> >>I moved it to a macro. >> >>#define CONFIG_SYS_NS16550_CLK get_serial_clock() >> > >> >Ah, we shouldn't do that with driver model. This should be in some >> >sort of clock driver. Perhaps we should leave this serial driver along >> >for now? >> >> I understand a clock driver should be used for driver model. But I >> would suggest that we use this get_serial_clock macro until the >> clock driver for powerpc is ready to get UART clock. This will help >> things moving forwards to driver model. > > Yes, how do you want to proceed here Simon? I have all of these (and a > few other things) build-tested now and will be doing some quick sanity > run time testing later. Thanks! My first thought would be to leave serial_ppc.c out of the conversion, but that would be tricky given how the series is structured (with the temporary CONFIG). Perhaps a TODO on the #define above since it should be cleaned up, and we want to make sure no one copies it. Since you have it ready I think it should go in, with the TODO if you can manage it. Regards, Simon
On Sat, Nov 21, 2015 at 09:10:07AM -0700, Simon Glass wrote: > Hi Tom, > > On 21 November 2015 at 08:22, Tom Rini <trini@konsulko.com> wrote: > > On Sat, Nov 21, 2015 at 10:39:54AM +0800, Thomas Chou wrote: > >> Hi Simon, > >> > >> On 2015年11月21日 08:27, Simon Glass wrote: > >> >>I moved it to a macro. > >> >>#define CONFIG_SYS_NS16550_CLK get_serial_clock() > >> > > >> >Ah, we shouldn't do that with driver model. This should be in some > >> >sort of clock driver. Perhaps we should leave this serial driver along > >> >for now? > >> > >> I understand a clock driver should be used for driver model. But I > >> would suggest that we use this get_serial_clock macro until the > >> clock driver for powerpc is ready to get UART clock. This will help > >> things moving forwards to driver model. > > > > Yes, how do you want to proceed here Simon? I have all of these (and a > > few other things) build-tested now and will be doing some quick sanity > > run time testing later. Thanks! > > My first thought would be to leave serial_ppc.c out of the conversion, > but that would be tricky given how the series is structured (with the > temporary CONFIG). > > Perhaps a TODO on the #define above since it should be cleaned up, and > we want to make sure no one copies it. > > Since you have it ready I think it should go in, with the TODO if you > can manage it. OK, I'll add the TODO comment.
On Thu, Nov 19, 2015 at 09:48:07PM +0800, Thomas Chou wrote: > Unify serial_ppc, and use the generic binding. > > Signed-off-by: Thomas Chou <thomas@wytron.com.tw> > Reviewed-by: Tom Rini <trini@konsulko.com> Applied to u-boot/master, thanks!
diff --git a/arch/powerpc/include/asm/config.h b/arch/powerpc/include/asm/config.h index 65496d0..7391066 100644 --- a/arch/powerpc/include/asm/config.h +++ b/arch/powerpc/include/asm/config.h @@ -104,4 +104,8 @@ /* All PPC boards must swap IDE bytes */ #define CONFIG_IDE_SWAP_IO +#if defined(CONFIG_DM_SERIAL) +#define CONFIG_SYS_NS16550_CLK get_serial_clock() +#endif + #endif /* _ASM_CONFIG_H_ */ diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index 93faa4c..b41f508 100644 --- a/drivers/serial/Kconfig +++ b/drivers/serial/Kconfig @@ -198,7 +198,7 @@ config ROCKCHIP_SERIAL config NS16550_SERIAL bool "NS16550 UART or compatible" depends on DM_SERIAL - default y if X86 + default y if X86 || PPC help Support NS16550 UART or compatible with driver model. This can be enabled in the device tree with the correct input clock frequency. diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile index 9036a8e..9f61113 100644 --- a/drivers/serial/Makefile +++ b/drivers/serial/Makefile @@ -8,7 +8,6 @@ ifdef CONFIG_DM_SERIAL obj-y += serial-uclass.o obj-$(CONFIG_PL01X_SERIAL) += serial_pl01x.o -obj-$(CONFIG_PPC) += serial_ppc.o else obj-y += serial.o obj-$(CONFIG_PL010_SERIAL) += serial_pl01x.o diff --git a/drivers/serial/serial_ppc.c b/drivers/serial/serial_ppc.c deleted file mode 100644 index 47141c6..0000000 --- a/drivers/serial/serial_ppc.c +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright (c) 2014 Google, Inc - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -#include <common.h> -#include <dm.h> -#include <ns16550.h> -#include <serial.h> - -static const struct udevice_id ppc_serial_ids[] = { - { .compatible = "ns16550" }, - { } -}; - -static int ppc_serial_ofdata_to_platdata(struct udevice *dev) -{ - struct ns16550_platdata *plat = dev_get_platdata(dev); - int ret; - - ret = ns16550_serial_ofdata_to_platdata(dev); - if (ret) - return ret; - plat->clock = get_serial_clock(); - - return 0; -} - -U_BOOT_DRIVER(serial_ns16550) = { - .name = "serial_ppc", - .id = UCLASS_SERIAL, - .of_match = ppc_serial_ids, - .ofdata_to_platdata = ppc_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, -};