| Message ID | 1389961514-13562-19-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 2014-1-17 22:21, Arnd Bergmann wrote: > On Friday 17 January 2014, Hanjun Guo wrote: >> >> From: Amit Daniel Kachhap <amit.daniel@samsung.com> >> >> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device >> name from the ACPI timer table is matched with all the registered >> timer controllers and matching initialisation routine is invoked. > > I wouldn't anticipate this infrastructure to be required. Shouldn't all > ARMv8 machines have an architected timer? I not sure of this, could anyone can give some guidance? if only arch timer is available for ARM64, this will make thing very simple. Thanks Hanjun
On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > From: Amit Daniel Kachhap <amit.daniel@samsung.com> > > This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device > name from the ACPI timer table is matched with all the registered > timer controllers and matching initialisation routine is invoked. > > Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE() to TIMER_OF_DECLARE() and I think this macro, if needed, should be named TIMER_ACPI_DECLARE(). The reason is that "clocksource" is a Linux-internal name and this macro pertains to the hardware name in respective system description type. > +#ifdef CONFIG_ACPI > +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) \ > + static const struct acpi_device_id __clksrc_acpi_table_##name \ > + __used __section(__clksrc_acpi_table) \ > + = { .id = compat, \ > + .driver_data = (kernel_ulong_t)fn } > +#else > +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) > +#endif This hammers down the world to compile one binary for ACPI and one binary for device tree. Maybe that's fine, I don't know. Yours, Linus Walleij
On Wed, Jan 22, 2014 at 12:45 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote: >> This hammers down the world to compile one binary for ACPI >> and one binary for device tree. Maybe that's fine, I don't know. > > How does it do that? Bah it doesn't I misread the patch. Forget that comment. Yours, Linus Walleij
Hi Linus, Sorry for the late reply. On 2014年01月22日 16:26, Linus Walleij wrote: > On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > >> From: Amit Daniel Kachhap <amit.daniel@samsung.com> >> >> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device >> name from the ACPI timer table is matched with all the registered >> timer controllers and matching initialisation routine is invoked. >> >> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE() > to TIMER_OF_DECLARE() and I think this macro, if needed, should > be named TIMER_ACPI_DECLARE(). > > The reason is that "clocksource" is a Linux-internal name and this > macro pertains to the hardware name in respective system > description type. That make sense to me too, I will update in next version if this patch is still needed. > >> +#ifdef CONFIG_ACPI >> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) \ >> + static const struct acpi_device_id __clksrc_acpi_table_##name \ >> + __used __section(__clksrc_acpi_table) \ >> + = { .id = compat, \ >> + .driver_data = (kernel_ulong_t)fn } >> +#else >> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) >> +#endif > This hammers down the world to compile one binary for ACPI > and one binary for device tree. Maybe that's fine, I don't know. This is a problem we can have some discussion on it. I prefer mutually exclusive ACPI and DT support. Thanks Hanjun
On 2014年01月22日 19:45, Mark Rutland wrote: > On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote: >> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >> >>> From: Amit Daniel Kachhap <amit.daniel@samsung.com> >>> >>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device >>> name from the ACPI timer table is matched with all the registered >>> timer controllers and matching initialisation routine is invoked. >>> >>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE() >> to TIMER_OF_DECLARE() and I think this macro, if needed, should >> be named TIMER_ACPI_DECLARE(). >> >> The reason is that "clocksource" is a Linux-internal name and this >> macro pertains to the hardware name in respective system >> description type. >> >>> +#ifdef CONFIG_ACPI >>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) \ >>> + static const struct acpi_device_id __clksrc_acpi_table_##name \ >>> + __used __section(__clksrc_acpi_table) \ >>> + = { .id = compat, \ >>> + .driver_data = (kernel_ulong_t)fn } >>> +#else >>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) >>> +#endif >> This hammers down the world to compile one binary for ACPI >> and one binary for device tree. Maybe that's fine, I don't know. > How does it do that? > > As far as I could tell CONFIG_ACPI and CONFIG_OF are not mutually > exclusive, and this just means that we only build the datastructures for > matching from ACPI when CONFIG_ACPI is enabled. > > Have I missed something? > > I definitely don't want to see mutually exclusive ACPI and DT support. ACPI and DT did the same job so I think they should mutually exclusive. if we enable both DT and ACPI in one system, this will leading confusions. Further more, firmware guys will be happy to present all the devices in one way, not both of them. Thanks Hanjun
On 2014年01月22日 19:46, Mark Rutland wrote: > On Mon, Jan 20, 2014 at 09:08:32AM +0000, Hanjun Guo wrote: >> On 2014-1-17 22:21, Arnd Bergmann wrote: >>> On Friday 17 January 2014, Hanjun Guo wrote: >>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>> >>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device >>>> name from the ACPI timer table is matched with all the registered >>>> timer controllers and matching initialisation routine is invoked. >>> I wouldn't anticipate this infrastructure to be required. Shouldn't all >>> ARMv8 machines have an architected timer? >> I not sure of this, could anyone can give some guidance? if only arch >> timer is available for ARM64, this will make thing very simple. > All ARMv8 systems should have an architected timer. Thanks for the clarification :) > > However, they may also have other timers (e.g. global timers for use > when CPUs are in low power states and their local architected timers > aren't active). Only arch timer (generic timer) is defined as a table in ACPI 5.0 spec at now, so other timers will not described as table and will described as device object in DSDT table, so do you think we need this framework or not? Thanks Hanjun
On 2014年01月22日 22:56, Arnd Bergmann wrote: > On Wednesday 22 January 2014 11:46:16 Mark Rutland wrote: >> On Mon, Jan 20, 2014 at 09:08:32AM +0000, Hanjun Guo wrote: >>> On 2014-1-17 22:21, Arnd Bergmann wrote: >>>> On Friday 17 January 2014, Hanjun Guo wrote: >>>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>>> >>>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device >>>>> name from the ACPI timer table is matched with all the registered >>>>> timer controllers and matching initialisation routine is invoked. >>>> I wouldn't anticipate this infrastructure to be required. Shouldn't all >>>> ARMv8 machines have an architected timer? >>> I not sure of this, could anyone can give some guidance? if only arch >>> timer is available for ARM64, this will make thing very simple. >> All ARMv8 systems should have an architected timer. >> >> However, they may also have other timers (e.g. global timers for use >> when CPUs are in low power states and their local architected timers >> aren't active). > But all other timers could be regular platform drivers I suppose. No > need for special infrastructure for those. One more information, acpi_early_init() is run before timekeeping_init() now, and that patch was merged into 3.14-rc1. So we can initialize timers for scanning the DSDT table, and it also means that timer defined in DSDT can be initialized early enough. Thanks Hanjun
On Fri, Jan 24, 2014 at 12:08:15PM +0000, Mark Rutland wrote: > On Fri, Jan 24, 2014 at 12:20:46AM +0000, Hanjun Guo wrote: > > On 2014年01月22日 19:45, Mark Rutland wrote: > > > On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote: > > >> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: > > >> > > >>> From: Amit Daniel Kachhap <amit.daniel@samsung.com> > > >>> > > >>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device > > >>> name from the ACPI timer table is matched with all the registered > > >>> timer controllers and matching initialisation routine is invoked. > > >>> > > >>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> > > >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > >> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE() > > >> to TIMER_OF_DECLARE() and I think this macro, if needed, should > > >> be named TIMER_ACPI_DECLARE(). > > >> > > >> The reason is that "clocksource" is a Linux-internal name and this > > >> macro pertains to the hardware name in respective system > > >> description type. > > >> > > >>> +#ifdef CONFIG_ACPI > > >>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) \ > > >>> + static const struct acpi_device_id __clksrc_acpi_table_##name \ > > >>> + __used __section(__clksrc_acpi_table) \ > > >>> + = { .id = compat, \ > > >>> + .driver_data = (kernel_ulong_t)fn } > > >>> +#else > > >>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) > > >>> +#endif > > >> This hammers down the world to compile one binary for ACPI > > >> and one binary for device tree. Maybe that's fine, I don't know. > > > How does it do that? > > > > > > As far as I could tell CONFIG_ACPI and CONFIG_OF are not mutually > > > exclusive, and this just means that we only build the datastructures for > > > matching from ACPI when CONFIG_ACPI is enabled. > > > > > > Have I missed something? > > > > > > I definitely don't want to see mutually exclusive ACPI and DT support. > > > > ACPI and DT did the same job so I think they should mutually exclusive. > > if we enable both DT and ACPI in one system, this will leading confusions. > > ACPI and DT do similar jobs, and we should be mutually exclusive at > runtime. However, they should not be mutually exclusive at compile-time. > > Being mutually exclusive at compile-time is just broken. It creates more > work for distributions (who need to ship double the number of kernels), > it increases the number of configurations requiring testing, and it > makes it easier for bugs to be introduced. It's just painful, and > there's no reason for it. I fully agree (IOW, I'll NAK patches that break this assumption; we want single kernel image whether it uses DT or ACPI). > At boot time the kernel needs to decide which to use for hardware > description, and completely ignore the other (which should not be > present, but lets not assume that or inevitably someone will break that > assumption for a quick hack). > > The same kernel should boot on a system that has a DTB or a system that > has ACPI tables. On a system that's provided both it should use one or > the other, but not both. Do we still need the chosen node to be passed via DT for command line, even if the kernel uses ACPI?
Hi Mark, On 2014年01月24日 20:32, Mark Rutland wrote: > On Fri, Jan 24, 2014 at 12:12:24AM +0000, Hanjun Guo wrote: >> Hi Linus, >> >> Sorry for the late reply. >> >> On 2014年01月22日 16:26, Linus Walleij wrote: >>> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >>> >>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>> >>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device >>>> name from the ACPI timer table is matched with all the registered >>>> timer controllers and matching initialisation routine is invoked. >>>> >>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE() >>> to TIMER_OF_DECLARE() and I think this macro, if needed, should >>> be named TIMER_ACPI_DECLARE(). >>> >>> The reason is that "clocksource" is a Linux-internal name and this >>> macro pertains to the hardware name in respective system >>> description type. >> That make sense to me too, I will update in next version if >> this patch is still needed. >> >>>> +#ifdef CONFIG_ACPI >>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) \ >>>> + static const struct acpi_device_id __clksrc_acpi_table_##name \ >>>> + __used __section(__clksrc_acpi_table) \ >>>> + = { .id = compat, \ >>>> + .driver_data = (kernel_ulong_t)fn } >>>> +#else >>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) >>>> +#endif >>> This hammers down the world to compile one binary for ACPI >>> and one binary for device tree. Maybe that's fine, I don't know. >> This is a problem we can have some discussion on it. >> I prefer mutually exclusive ACPI and DT support. > A lot of work has been put into making a single kernel boot everywhere. > It's forced duplicated code to be factored out, and it's made the kernel > more flexible. While it has been painful, it's forced a far higher > quality standard across the board(s). > > Having a separate ACPI-capable or DT-capable kernels goes completely > against that, and it's completely broken: > > * It doubles the testing effort required for a particular kernel. I can > guarantee that we will miss bugs (even amazingly bad build bugs) > because no-one will be able to test a full suite of kernels. > > * It introduces the possibility of completely pointles arbitrary > differences between the two. How long until we see the first bug-fix > that only works in one configuration? > > * It creates additional work for distributions, which need to build more > kernels test them, distribute them, and document which platforms which > kernels are supported on. This creates more pain for end-users too. > > Eventually we _will_ get fed up with all of those, and we'll have to do > painful invasive work to make the kernel decide at runtime. > > Having separate kernels is a lazy shortcut. It's painful for everyone, > leads to a greater maintenance overhead, it's not what we want now and > not what we want in future. > > No thanks. > > Either the kernel figures out whether or not to deal with ACPI at > runtime, or it doesn't deal with it at all. I fully agree with you for the single kernel image, I didn't notice this before, sorry for my noise about the exclusive ACPI and DT support. Thank you very much to let things much more clearer :) Hanjun
On 2014年01月24日 23:15, Catalin Marinas wrote: > On Fri, Jan 24, 2014 at 12:08:15PM +0000, Mark Rutland wrote: >> On Fri, Jan 24, 2014 at 12:20:46AM +0000, Hanjun Guo wrote: >>> On 2014年01月22日 19:45, Mark Rutland wrote: >>>> On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote: >>>>> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote: >>>>> >>>>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>>>> >>>>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device >>>>>> name from the ACPI timer table is matched with all the registered >>>>>> timer controllers and matching initialisation routine is invoked. >>>>>> >>>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>>> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE() >>>>> to TIMER_OF_DECLARE() and I think this macro, if needed, should >>>>> be named TIMER_ACPI_DECLARE(). >>>>> >>>>> The reason is that "clocksource" is a Linux-internal name and this >>>>> macro pertains to the hardware name in respective system >>>>> description type. >>>>> >>>>>> +#ifdef CONFIG_ACPI >>>>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) \ >>>>>> + static const struct acpi_device_id __clksrc_acpi_table_##name \ >>>>>> + __used __section(__clksrc_acpi_table) \ >>>>>> + = { .id = compat, \ >>>>>> + .driver_data = (kernel_ulong_t)fn } >>>>>> +#else >>>>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) >>>>>> +#endif >>>>> This hammers down the world to compile one binary for ACPI >>>>> and one binary for device tree. Maybe that's fine, I don't know. >>>> How does it do that? >>>> >>>> As far as I could tell CONFIG_ACPI and CONFIG_OF are not mutually >>>> exclusive, and this just means that we only build the datastructures for >>>> matching from ACPI when CONFIG_ACPI is enabled. >>>> >>>> Have I missed something? >>>> >>>> I definitely don't want to see mutually exclusive ACPI and DT support. >>> ACPI and DT did the same job so I think they should mutually exclusive. >>> if we enable both DT and ACPI in one system, this will leading confusions. >> ACPI and DT do similar jobs, and we should be mutually exclusive at >> runtime. However, they should not be mutually exclusive at compile-time. >> >> Being mutually exclusive at compile-time is just broken. It creates more >> work for distributions (who need to ship double the number of kernels), >> it increases the number of configurations requiring testing, and it >> makes it easier for bugs to be introduced. It's just painful, and >> there's no reason for it. > I fully agree (IOW, I'll NAK patches that break this assumption; we want > single kernel image whether it uses DT or ACPI). I will not break this in next version, because I totally agree with Mark too :) > >> At boot time the kernel needs to decide which to use for hardware >> description, and completely ignore the other (which should not be >> present, but lets not assume that or inevitably someone will break that >> assumption for a quick hack). >> >> The same kernel should boot on a system that has a DTB or a system that >> has ACPI tables. On a system that's provided both it should use one or >> the other, but not both. > Do we still need the chosen node to be passed via DT for command line, > even if the kernel uses ACPI? It depends, but I would prefer not. I prefer UEFI+ACPI and then we don't need the chosen node to be passed via DT. Thanks Hanjun
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index bc2121f..3216bee 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -148,6 +148,15 @@ #define CLKSRC_OF_TABLES() #endif +#ifdef CONFIG_ACPI +#define CLKSRC_ACPI_TABLES() . = ALIGN(8); \ + VMLINUX_SYMBOL(__clksrc_acpi_table) = .; \ + *(__clksrc_acpi_table) \ + *(__clksrc_acpi_table_end) +#else +#define CLKSRC_ACPI_TABLES() +#endif + #ifdef CONFIG_IRQCHIP #define IRQCHIP_OF_MATCH_TABLE() \ . = ALIGN(8); \ @@ -491,6 +500,7 @@ MEM_DISCARD(init.rodata) \ CLK_OF_TABLES() \ CLKSRC_OF_TABLES() \ + CLKSRC_ACPI_TABLES() \ KERNEL_DTB() \ IRQCHIP_OF_MATCH_TABLE() diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h index 67301a4..1d500cf 100644 --- a/include/linux/clocksource.h +++ b/include/linux/clocksource.h @@ -341,6 +341,8 @@ extern int clocksource_i8253_init(void); struct device_node; typedef void(*clocksource_of_init_fn)(struct device_node *); +typedef void(*clocksource_acpi_init_fn)(void); + #ifdef CONFIG_CLKSRC_OF extern void clocksource_of_init(void); @@ -358,4 +360,14 @@ static inline void clocksource_of_init(void) {} .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn } #endif +#ifdef CONFIG_ACPI +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) \ + static const struct acpi_device_id __clksrc_acpi_table_##name \ + __used __section(__clksrc_acpi_table) \ + = { .id = compat, \ + .driver_data = (kernel_ulong_t)fn } +#else +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn) +#endif + #endif /* _LINUX_CLOCKSOURCE_H */