| Message ID | 1389961514-13562-19-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? Arnd
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 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. Cheers, Mark.
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). Thanks, Mark.
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
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. Arnd
On Wed, Jan 22, 2014 at 02:56:05PM +0000, 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. Except for the fact that some timers / clocksources that we already have in 32-bit land will likely be reused in 64-bit SoC designs. People will want to use the same driver for both 32-bit and 64-bit kernels, and thus we need CLOCKSOURCE_OF_DECLARE in 64-bit kernels. Those platforms which will have ACPI will likely reuse existing timer IP blocks, and will want to make minimal changes to the driver, which will likely be using CLOCKSOURCE_OF_DECLARE. We can't change these to platform drivers or we can break existing systems because the timers will be registered too late. I don't see how we can share drivers between 32-bit and 64-bit kernels without sharing a common driver model, and I think it makes sense to have some uniformity across drivers (i.e. always use CLOCKSOURCE_*_DECLARE rather than sometimes using platform drivers). Thanks, Mark.
On Wednesday 22 January 2014 15:17:49 Mark Rutland wrote: > Except for the fact that some timers / clocksources that we already have > in 32-bit land will likely be reused in 64-bit SoC designs. People will > want to use the same driver for both 32-bit and 64-bit kernels, and thus > we need CLOCKSOURCE_OF_DECLARE in 64-bit kernels. > > Those platforms which will have ACPI will likely reuse existing timer IP > blocks, and will want to make minimal changes to the driver, which will > likely be using CLOCKSOURCE_OF_DECLARE. We can't change these to > platform drivers or we can break existing systems because the timers > will be registered too late. > > I don't see how we can share drivers between 32-bit and 64-bit kernels > without sharing a common driver model, and I think it makes sense to > have some uniformity across drivers (i.e. always use > CLOCKSOURCE_*_DECLARE rather than sometimes using platform drivers). This still sounds like speculation. I would defer this change until we actually have a platform that needs it. The platforms that would reuse a lot of IP blocks are most likely embedded systems and /not/ server hardware following some strict specification, so they wouldn't use ACPI. Arnd
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: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. 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. Thanks, Mark.
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. Thanks, Mark.
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?
On Fri, Jan 24, 2014 at 03:15:13PM +0000, Catalin Marinas wrote: > Do we still need the chosen node to be passed via DT for command line, > even if the kernel uses ACPI? It depends on how the kernel is booted. If not booted as a UEFI application, then the user would have to provide a dtb to pass the command line. If booted as a UEFI application, the kernel can retrieve the commandline from UEFI and handle it appropriately (this _might_ involve the EFI stub building a dtb with a chosen node and pasing that to the kernel, but that could change as it's within the kernel). In that case, the user need not build a dtb manually. However, I'd expect that we use a restricted dtb with only a /chosen node for passing additional data as an equivalent to x86's bootargs (which would contain the location of ACPI tables and possibly other stuff like initrd location if not passed on the command line). That makes the very early logic in the kernel much simpler, and it gives us an extensible format that should limit headaches in future. So if booted with ACPI we _might_ have a restricted chosen-only dtb, which _might_ be passed to the kernel explicitly by the user. Thanks, Mark.
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 */