Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
Hi Rafael,
On 16 July 2016 at 20:35, Rafael J. Wysocki wrote:
> On Saturday, July 16, 2016 10:24:35 AM Fu Wei wrote:
>> Hi Rafeal,
>>
>> On 16 July 2016 at 05:22, Rafael J. Wysocki wrote:
>> > On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote:
>> >> Hi Rafael,
>> >>
>> >> On 15 July 2016 at 21:07, Rafael J. Wysocki wrote:
>> >> > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote:
>> >> >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote:
>> >> >> > Hi Rafael,
>> >> >> >
>> >> >
>> >> > [cut]
>> >> >
>> >> >> > >
>> >> >> > >> + return 0;
>> >> >> > >> + }
>> >> >> > >> +
>> >> >> > >> + if (!gtdt->platform_timer_count) {
>> >> >> > >> + pr_info("No Platform Timer.\n");
>> >> >> > >> + return 0;
>> >> >> > >> + }
>> >> >> > >> +
>> >> >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt +
>> >> >> > >> +
>> >> >> > >> gtdt->platform_timer_offset;
>> >> >> > >> + if (acpi_gtdt_desc.platform_timer_start <
>> >> >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) {
>> >> >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n");
>> >> >> > >
>> >> >> > > Why pr_err()?
>> >> >> >
>> >> >> > if (true), that means the GTDT table has bugs.
>> >> >> >
>> >> >>
>> >> >> And that's not a very useful piece of information unless you're
>> >> >> debugging the
>> >> >> platform, is it?
>> >> >
>> >> > FWIW, I'm not a big fan of printing "your firmware is buggy" type of
>> >> > messages
>> >> > (especially at the "error" log level or higher) unless they can be
>> >> > clearly
>> >> > connected to a specific type of functional failure.
>> >> >
>> >> > So if you want to pring an error-level message, something like "I
>> >> > cannot do X
>> >> > because of the firmware bug Y" would be better IMO.
>> >>
>> >> So can I do this:
>> >> pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the
>> >> GTDT table bug\n");
>> >>
>> >> or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of
>> >> platform_timer_offset bug in GTDT\n");
>> >>
>> >> or just delete it?
>> >>
>> >> which one do you prefer? I think maybe should provide some clue for
>> >> users to fix the problem :-)
>> >
>> > And how exactly would they fix it then?
>> >
>> >>
>> >> any thought ?
>> >
>> > If you print variable or function names and the like, the message should be
>> > a debug one, because that's information that can only be understood by
>> > developers (some developers are users too, but they are a minority).
>> >
>> > If you want to report an error, say what is not working (or not available
>> > etc) and why (if you know the reason at the time the message is printed).
>>
>> Great thanks, I guess I got you point.
>>
>> maybe just a very simple message like:
>> pr_err(FW_BUG "Failed to init table: GTDT table is buggy.\n");
>
> To understand this message one needs to know what "table" means here
> and what "GTDT" is. Also the prefix already will be something like
> "ACPI: GTDT:", so repeating part of it is not really useful IMO.
>
> Can you tell me please what's not going to work when that message is printed?
>
> Will the system boot at all then? If so, the functionality will be limited
> somehow I suppose. How is it going to be limited?
when that message is printed, all kind of platform timer(memory-mapped
timer and SBSA watchdog) can't work.
actually, I think system can boot without them.
I have updated this on my v8(just posted), let's discuss this on v8 :-)
>
>> I will also check other pr_* , if I can update them
>
> OK, great!
>
> Thanks,
> Rafael
>
--
Best regards,
Fu Wei
Software Engineer
Red Hat
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Saturday, July 16, 2016 10:24:35 AM Fu Wei wrote:
> Hi Rafeal,
>
> On 16 July 2016 at 05:22, Rafael J. Wysocki wrote:
> > On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote:
> >> Hi Rafael,
> >>
> >> On 15 July 2016 at 21:07, Rafael J. Wysocki wrote:
> >> > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote:
> >> >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote:
> >> >> > Hi Rafael,
> >> >> >
> >> >
> >> > [cut]
> >> >
> >> >> > >
> >> >> > >> + return 0;
> >> >> > >> + }
> >> >> > >> +
> >> >> > >> + if (!gtdt->platform_timer_count) {
> >> >> > >> + pr_info("No Platform Timer.\n");
> >> >> > >> + return 0;
> >> >> > >> + }
> >> >> > >> +
> >> >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt +
> >> >> > >> +
> >> >> > >> gtdt->platform_timer_offset;
> >> >> > >> + if (acpi_gtdt_desc.platform_timer_start <
> >> >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) {
> >> >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n");
> >> >> > >
> >> >> > > Why pr_err()?
> >> >> >
> >> >> > if (true), that means the GTDT table has bugs.
> >> >> >
> >> >>
> >> >> And that's not a very useful piece of information unless you're
> >> >> debugging the
> >> >> platform, is it?
> >> >
> >> > FWIW, I'm not a big fan of printing "your firmware is buggy" type of
> >> > messages
> >> > (especially at the "error" log level or higher) unless they can be
> >> > clearly
> >> > connected to a specific type of functional failure.
> >> >
> >> > So if you want to pring an error-level message, something like "I cannot
> >> > do X
> >> > because of the firmware bug Y" would be better IMO.
> >>
> >> So can I do this:
> >> pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the
> >> GTDT table bug\n");
> >>
> >> or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of
> >> platform_timer_offset bug in GTDT\n");
> >>
> >> or just delete it?
> >>
> >> which one do you prefer? I think maybe should provide some clue for
> >> users to fix the problem :-)
> >
> > And how exactly would they fix it then?
> >
> >>
> >> any thought ?
> >
> > If you print variable or function names and the like, the message should be
> > a debug one, because that's information that can only be understood by
> > developers (some developers are users too, but they are a minority).
> >
> > If you want to report an error, say what is not working (or not available
> > etc) and why (if you know the reason at the time the message is printed).
>
> Great thanks, I guess I got you point.
>
> maybe just a very simple message like:
> pr_err(FW_BUG "Failed to init table: GTDT table is buggy.\n");
To understand this message one needs to know what "table" means here
and what "GTDT" is. Also the prefix already will be something like
"ACPI: GTDT:", so repeating part of it is not really useful IMO.
Can you tell me please what's not going to work when that message is printed?
Will the system boot at all then? If so, the functionality will be limited
somehow I suppose. How is it going to be limited?
> I will also check other pr_* , if I can update them
OK, great!
Thanks,
Rafael
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
Hi Rafeal,
On 16 July 2016 at 05:22, Rafael J. Wysocki wrote:
> On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote:
>> Hi Rafael,
>>
>> On 15 July 2016 at 21:07, Rafael J. Wysocki wrote:
>> > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote:
>> >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote:
>> >> > Hi Rafael,
>> >> >
>> >
>> > [cut]
>> >
>> >> > >
>> >> > >> + return 0;
>> >> > >> + }
>> >> > >> +
>> >> > >> + if (!gtdt->platform_timer_count) {
>> >> > >> + pr_info("No Platform Timer.\n");
>> >> > >> + return 0;
>> >> > >> + }
>> >> > >> +
>> >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt +
>> >> > >> +
>> >> > >> gtdt->platform_timer_offset;
>> >> > >> + if (acpi_gtdt_desc.platform_timer_start <
>> >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) {
>> >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n");
>> >> > >
>> >> > > Why pr_err()?
>> >> >
>> >> > if (true), that means the GTDT table has bugs.
>> >> >
>> >>
>> >> And that's not a very useful piece of information unless you're debugging
>> >> the
>> >> platform, is it?
>> >
>> > FWIW, I'm not a big fan of printing "your firmware is buggy" type of
>> > messages
>> > (especially at the "error" log level or higher) unless they can be clearly
>> > connected to a specific type of functional failure.
>> >
>> > So if you want to pring an error-level message, something like "I cannot
>> > do X
>> > because of the firmware bug Y" would be better IMO.
>>
>> So can I do this:
>> pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the
>> GTDT table bug\n");
>>
>> or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of
>> platform_timer_offset bug in GTDT\n");
>>
>> or just delete it?
>>
>> which one do you prefer? I think maybe should provide some clue for
>> users to fix the problem :-)
>
> And how exactly would they fix it then?
>
>>
>> any thought ?
>
> If you print variable or function names and the like, the message should be
> a debug one, because that's information that can only be understood by
> developers (some developers are users too, but they are a minority).
>
> If you want to report an error, say what is not working (or not available
> etc) and why (if you know the reason at the time the message is printed).
Great thanks, I guess I got you point.
maybe just a very simple message like:
pr_err(FW_BUG "Failed to init table: GTDT table is buggy.\n");
I will also check other pr_* , if I can update them
>
> Thanks,
> Rafael
>
--
Best regards,
Fu Wei
Software Engineer
Red Hat
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote:
> Hi Rafael,
>
> On 15 July 2016 at 21:07, Rafael J. Wysocki wrote:
> > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote:
> >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote:
> >> > Hi Rafael,
> >> >
> >
> > [cut]
> >
> >> > >
> >> > >> + return 0;
> >> > >> + }
> >> > >> +
> >> > >> + if (!gtdt->platform_timer_count) {
> >> > >> + pr_info("No Platform Timer.\n");
> >> > >> + return 0;
> >> > >> + }
> >> > >> +
> >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt +
> >> > >> +
> >> > >> gtdt->platform_timer_offset;
> >> > >> + if (acpi_gtdt_desc.platform_timer_start <
> >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) {
> >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n");
> >> > >
> >> > > Why pr_err()?
> >> >
> >> > if (true), that means the GTDT table has bugs.
> >> >
> >>
> >> And that's not a very useful piece of information unless you're debugging
> >> the
> >> platform, is it?
> >
> > FWIW, I'm not a big fan of printing "your firmware is buggy" type of
> > messages
> > (especially at the "error" log level or higher) unless they can be clearly
> > connected to a specific type of functional failure.
> >
> > So if you want to pring an error-level message, something like "I cannot do
> > X
> > because of the firmware bug Y" would be better IMO.
>
> So can I do this:
> pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the
> GTDT table bug\n");
>
> or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of
> platform_timer_offset bug in GTDT\n");
>
> or just delete it?
>
> which one do you prefer? I think maybe should provide some clue for
> users to fix the problem :-)
And how exactly would they fix it then?
>
> any thought ?
If you print variable or function names and the like, the message should be
a debug one, because that's information that can only be understood by
developers (some developers are users too, but they are a minority).
If you want to report an error, say what is not working (or not available
etc) and why (if you know the reason at the time the message is printed).
Thanks,
Rafael
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
Hi Rafael,
On 15 July 2016 at 21:07, Rafael J. Wysocki wrote:
> On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote:
>> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote:
>> > Hi Rafael,
>> >
>
> [cut]
>
>> > >
>> > >> + return 0;
>> > >> + }
>> > >> +
>> > >> + if (!gtdt->platform_timer_count) {
>> > >> + pr_info("No Platform Timer.\n");
>> > >> + return 0;
>> > >> + }
>> > >> +
>> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt +
>> > >> +
>> > >> gtdt->platform_timer_offset;
>> > >> + if (acpi_gtdt_desc.platform_timer_start <
>> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) {
>> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n");
>> > >
>> > > Why pr_err()?
>> >
>> > if (true), that means the GTDT table has bugs.
>> >
>>
>> And that's not a very useful piece of information unless you're debugging the
>> platform, is it?
>
> FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages
> (especially at the "error" log level or higher) unless they can be clearly
> connected to a specific type of functional failure.
>
> So if you want to pring an error-level message, something like "I cannot do X
> because of the firmware bug Y" would be better IMO.
So can I do this:
pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the
GTDT table bug\n");
or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of
platform_timer_offset bug in GTDT\n");
or just delete it?
which one do you prefer? I think maybe should provide some clue for
users to fix the problem :-)
any thought ?
>
> Thanks,
> Rafael
>
--
Best regards,
Fu Wei
Software Engineer
Red Hat
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
Hi Rafael, On 15 July 2016 at 20:11, Rafael J. Wysocki wrote: > On Friday, July 15, 2016 03:45:05 PM Fu Wei wrote: >> Hi Rafael, >> >> >> On 14 July 2016 at 05:43, Rafael J. Wysocki wrote: >> > On Wed, Jul 13, 2016 at 11:08 PM, Guenter Roeck wrote: >> >> On Wed, Jul 13, 2016 at 10:30:37PM +0200, Rafael J. Wysocki wrote: >> >>> On Wed, Jul 13, 2016 at 7:53 PM, wrote: >> >>> > From: Fu Wei >> >>> > >> >>> > This patch adds support for parsing arch timer in GTDT, >> >>> > provides some kernel APIs to parse all the PPIs and >> >>> > always-on info in GTDT and export them. >> >>> > >> >>> > By this driver, we can simplify arm_arch_timer drivers, and >> >>> > separate the ACPI GTDT knowledge from it. >> >>> > >> >>> > Signed-off-by: Fu Wei >> >>> > Signed-off-by: Hanjun Guo >> >>> > --- >> >>> > drivers/acpi/Kconfig | 5 ++ >> >>> > drivers/acpi/Makefile | 1 + >> >>> > drivers/acpi/arm64/Kconfig | 15 >> >>> > drivers/acpi/arm64/Makefile| 1 + >> >>> > drivers/acpi/arm64/acpi_gtdt.c | 170 >> >>> > + >> >>> > include/linux/acpi.h | 6 ++ >> >>> > 6 files changed, 198 insertions(+) >> >>> > >> >>> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> >>> > index b7e2e77..1cdc7d2 100644 >> >>> > --- a/drivers/acpi/Kconfig >> >>> > +++ b/drivers/acpi/Kconfig >> >>> > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >> >>> > >> >>> > endif >> >>> > >> >>> > +if ARM64 >> >>> > +source "drivers/acpi/arm64/Kconfig" >> >>> > + >> >>> > +endif >> >>> > + >> >>> > endif # ACPI >> >>> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> >>> > index 251ce85..1a94ff7 100644 >> >>> > --- a/drivers/acpi/Makefile >> >>> > +++ b/drivers/acpi/Makefile >> >>> > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >> >>> > obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o >> >>> > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >> >>> > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >> >>> > +obj-$(CONFIG_ARM64)+= arm64/ >> >>> > >> >>> > video-objs += acpi_video.o video_detect.o >> >>> > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >> >>> > new file mode 100644 >> >>> > index 000..ff5c253 >> >>> > --- /dev/null >> >>> > +++ b/drivers/acpi/arm64/Kconfig >> >>> > @@ -0,0 +1,15 @@ >> >>> > +# >> >>> > +# ACPI Configuration for ARM64 >> >>> > +# >> >>> > + >> >>> > +menu "The ARM64-specific ACPI Support" >> >>> > + >> >>> > +config ACPI_GTDT >> >>> > + bool "ACPI GTDT table Support" >> >>> >> >>> This should depend on ARM64. >> >>> >> >>> Also I wonder if it needs to be user-selectable? Wouldn't it be >> >>> better to enable it by default when building for ARM64 with ACPI? >> >>> >> >> It is currently selected in patch 9, in the watchdog driver's Kconfig >> >> entry. >> > >> > Well, it still doesn't have to be user-selectable for that. :-) >> >> Actually it is also automatically selected by [PATCH v7 6/9]: > > Right. > > By user-selectable I mean "showing up in a Kconfig configurator tool". > > It just need not be visible in the configurator IMO. > >> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >> index 47352d2..71d5b30 100644 >> --- a/drivers/clocksource/Kconfig >> +++ b/drivers/clocksource/Kconfig >> @@ -8,6 +8,7 @@ config CLKSRC_OF >> config CLKSRC_ACPI >> bool >> select CLKSRC_PROBE >> + select ACPI_GTDT if ARM64 >> >> config CLKSRC_PROBE >> bool >> >> >> > >> >> Not sure if I like that; maybe the watchdog driver should depend >> >> on it instead ? >> > >> > If the watchdog is not the only user of it (and I don't think it is), >> > it would be better to arrange things this way. >> > >> >> There are two user: >> (1) arm_arch_timer(which will select CLKSRC_ACPI if ACPI, then >> CLKSRC_ACPI will select ACPI_GTDT if ARM64) >> So arm_arch_timer will automatically selecte ACPI_GTDT if ARM64 && ACPI >> >> (2) sbsa_gwdt (which will select ACPI_GTDT if ACPI in [PATCH v7 9/9]) >> So sbsa_gwdt will automatically selecte ACPI_GTDT if ARM64 && ACPI && >> ARM_ARCH_TIMER >> >> So ACPI_GTDT is automatically selected by both of two users. >> >> >> But like Timur said before: >> >> maybe we just "selecte ACPI_GTDT if ACPI" for ARM64, because ARM64 >> require GTDT if we use ACPI. > > Right. That's the way to do it then and make sbsa_gwdt depend on it instead > of selecting it. OK, will do in my v8 patchset > > Thanks, > Rafael > -- Best regards, Fu Wei Software Engineer Red Hat
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote:
> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote:
> > Hi Rafael,
> >
[cut]
> > >
> > >> + return 0;
> > >> + }
> > >> +
> > >> + if (!gtdt->platform_timer_count) {
> > >> + pr_info("No Platform Timer.\n");
> > >> + return 0;
> > >> + }
> > >> +
> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt +
> > >> +
> > >> gtdt->platform_timer_offset;
> > >> + if (acpi_gtdt_desc.platform_timer_start <
> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) {
> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n");
> > >
> > > Why pr_err()?
> >
> > if (true), that means the GTDT table has bugs.
> >
>
> And that's not a very useful piece of information unless you're debugging the
> platform, is it?
FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages
(especially at the "error" log level or higher) unless they can be clearly
connected to a specific type of functional failure.
So if you want to pring an error-level message, something like "I cannot do X
because of the firmware bug Y" would be better IMO.
Thanks,
Rafael
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote:
> Hi Rafael,
>
>
>
> On 14 July 2016 at 04:30, Rafael J. Wysocki wrote:
> > On Wed, Jul 13, 2016 at 7:53 PM, wrote:
> >> From: Fu Wei
> >>
> >> This patch adds support for parsing arch timer in GTDT,
> >> provides some kernel APIs to parse all the PPIs and
> >> always-on info in GTDT and export them.
> >>
> >> By this driver, we can simplify arm_arch_timer drivers, and
> >> separate the ACPI GTDT knowledge from it.
> >>
> >> Signed-off-by: Fu Wei
> >> Signed-off-by: Hanjun Guo
> >> ---
> >> drivers/acpi/Kconfig | 5 ++
> >> drivers/acpi/Makefile | 1 +
> >> drivers/acpi/arm64/Kconfig | 15
> >> drivers/acpi/arm64/Makefile| 1 +
> >> drivers/acpi/arm64/acpi_gtdt.c | 170
> >> +
> >> include/linux/acpi.h | 6 ++
> >> 6 files changed, 198 insertions(+)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index b7e2e77..1cdc7d2 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION
> >>
> >> endif
> >>
> >> +if ARM64
> >> +source "drivers/acpi/arm64/Kconfig"
> >> +
> >> +endif
> >> +
> >> endif # ACPI
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index 251ce85..1a94ff7 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
> >> obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o
> >> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
> >> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
> >> +obj-$(CONFIG_ARM64)+= arm64/
> >>
> >> video-objs += acpi_video.o video_detect.o
> >> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
> >> new file mode 100644
> >> index 000..ff5c253
> >> --- /dev/null
> >> +++ b/drivers/acpi/arm64/Kconfig
> >> @@ -0,0 +1,15 @@
> >> +#
> >> +# ACPI Configuration for ARM64
> >> +#
> >> +
> >> +menu "The ARM64-specific ACPI Support"
> >> +
> >> +config ACPI_GTDT
> >> + bool "ACPI GTDT table Support"
> >
> > This should depend on ARM64.
> >
> > Also I wonder if it needs to be user-selectable? Wouldn't it be
> > better to enable it by default when building for ARM64 with ACPI?
> >
> >> + help
> >> + GTDT (Generic Timer Description Table) provides information
> >> + for per-processor timers and Platform (memory-mapped) timers
> >> + for ARM platforms. Select this option to provide information
> >> + needed for the timers init.
> >> +
> >> +endmenu
> >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
> >> new file mode 100644
> >> index 000..466de6b
> >> --- /dev/null
> >> +++ b/drivers/acpi/arm64/Makefile
> >> @@ -0,0 +1 @@
> >> +obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o
> >> diff --git a/drivers/acpi/arm64/acpi_gtdt.c
> >> b/drivers/acpi/arm64/acpi_gtdt.c
> >> new file mode 100644
> >> index 000..9ee977d
> >> --- /dev/null
> >> +++ b/drivers/acpi/arm64/acpi_gtdt.c
> >> @@ -0,0 +1,170 @@
> >> +/*
> >> + * ARM Specific GTDT table Support
> >> + *
> >> + * Copyright (C) 2016, Linaro Ltd.
> >> + * Author: Daniel Lezcano
> >> + * Fu Wei
> >> + * Hanjun Guo
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include
> >> +
> >> +#undef pr_fmt
> >> +#define pr_fmt(fmt) "GTDT: " fmt
> >
> > I would add "ACPI" to the prefix too if I were you, but that's me.
>
> good idea, you are right, will do
>
> >
> >> +
> >> +typedef struct {
> >> + struct acpi_table_gtdt *gtdt;
> >> + void *platform_timer_start;
> >> + void *gtdt_end;
> >> +} acpi_gtdt_desc_t;
> >> +
> >> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata;
> >> +
> >> +static inline void *next_platform_timer(void *platform_timer)
> >> +{
> >> + struct acpi_gtdt_header *gh = platform_timer;
> >> +
> >> + platform_timer += gh->length;
> >> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
> >> + return platform_timer;
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +#define for_each_platform_timer(_g)\
> >> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \
> >> +_g = next_platform_timer(_g))
> >> +
> >> +static inline bool is_timer_block(void *platform_timer)
> >> +{
> >> + struct acpi_gtdt_header *gh = platform_timer;
> >> +
> >> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
> >> + return true;
> >> +
> >> + return false;
> >
> > This is just too much code. It would suffice to do
> >
> > return gh->type == ACPI_GTDT_TYPE_T
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Friday, July 15, 2016 03:45:05 PM Fu Wei wrote: > Hi Rafael, > > > On 14 July 2016 at 05:43, Rafael J. Wysocki wrote: > > On Wed, Jul 13, 2016 at 11:08 PM, Guenter Roeck wrote: > >> On Wed, Jul 13, 2016 at 10:30:37PM +0200, Rafael J. Wysocki wrote: > >>> On Wed, Jul 13, 2016 at 7:53 PM, wrote: > >>> > From: Fu Wei > >>> > > >>> > This patch adds support for parsing arch timer in GTDT, > >>> > provides some kernel APIs to parse all the PPIs and > >>> > always-on info in GTDT and export them. > >>> > > >>> > By this driver, we can simplify arm_arch_timer drivers, and > >>> > separate the ACPI GTDT knowledge from it. > >>> > > >>> > Signed-off-by: Fu Wei > >>> > Signed-off-by: Hanjun Guo > >>> > --- > >>> > drivers/acpi/Kconfig | 5 ++ > >>> > drivers/acpi/Makefile | 1 + > >>> > drivers/acpi/arm64/Kconfig | 15 > >>> > drivers/acpi/arm64/Makefile| 1 + > >>> > drivers/acpi/arm64/acpi_gtdt.c | 170 > >>> > + > >>> > include/linux/acpi.h | 6 ++ > >>> > 6 files changed, 198 insertions(+) > >>> > > >>> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > >>> > index b7e2e77..1cdc7d2 100644 > >>> > --- a/drivers/acpi/Kconfig > >>> > +++ b/drivers/acpi/Kconfig > >>> > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION > >>> > > >>> > endif > >>> > > >>> > +if ARM64 > >>> > +source "drivers/acpi/arm64/Kconfig" > >>> > + > >>> > +endif > >>> > + > >>> > endif # ACPI > >>> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > >>> > index 251ce85..1a94ff7 100644 > >>> > --- a/drivers/acpi/Makefile > >>> > +++ b/drivers/acpi/Makefile > >>> > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o > >>> > obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o > >>> > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o > >>> > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o > >>> > +obj-$(CONFIG_ARM64)+= arm64/ > >>> > > >>> > video-objs += acpi_video.o video_detect.o > >>> > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > >>> > new file mode 100644 > >>> > index 000..ff5c253 > >>> > --- /dev/null > >>> > +++ b/drivers/acpi/arm64/Kconfig > >>> > @@ -0,0 +1,15 @@ > >>> > +# > >>> > +# ACPI Configuration for ARM64 > >>> > +# > >>> > + > >>> > +menu "The ARM64-specific ACPI Support" > >>> > + > >>> > +config ACPI_GTDT > >>> > + bool "ACPI GTDT table Support" > >>> > >>> This should depend on ARM64. > >>> > >>> Also I wonder if it needs to be user-selectable? Wouldn't it be > >>> better to enable it by default when building for ARM64 with ACPI? > >>> > >> It is currently selected in patch 9, in the watchdog driver's Kconfig > >> entry. > > > > Well, it still doesn't have to be user-selectable for that. :-) > > Actually it is also automatically selected by [PATCH v7 6/9]: Right. By user-selectable I mean "showing up in a Kconfig configurator tool". It just need not be visible in the configurator IMO. > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index 47352d2..71d5b30 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -8,6 +8,7 @@ config CLKSRC_OF > config CLKSRC_ACPI > bool > select CLKSRC_PROBE > + select ACPI_GTDT if ARM64 > > config CLKSRC_PROBE > bool > > > > > >> Not sure if I like that; maybe the watchdog driver should depend > >> on it instead ? > > > > If the watchdog is not the only user of it (and I don't think it is), > > it would be better to arrange things this way. > > > > There are two user: > (1) arm_arch_timer(which will select CLKSRC_ACPI if ACPI, then > CLKSRC_ACPI will select ACPI_GTDT if ARM64) > So arm_arch_timer will automatically selecte ACPI_GTDT if ARM64 && ACPI > > (2) sbsa_gwdt (which will select ACPI_GTDT if ACPI in [PATCH v7 9/9]) > So sbsa_gwdt will automatically selecte ACPI_GTDT if ARM64 && ACPI && > ARM_ARCH_TIMER > > So ACPI_GTDT is automatically selected by both of two users. > > > But like Timur said before: > > maybe we just "selecte ACPI_GTDT if ACPI" for ARM64, because ARM64 > require GTDT if we use ACPI. Right. That's the way to do it then and make sbsa_gwdt depend on it instead of selecting it. Thanks, Rafael
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
Hi Paul On 15 July 2016 at 04:33, Paul Gortmaker wrote: > On Wed, Jul 13, 2016 at 1:53 PM, wrote: >> From: Fu Wei >> >> This patch adds support for parsing arch timer in GTDT, >> provides some kernel APIs to parse all the PPIs and >> always-on info in GTDT and export them. >> >> By this driver, we can simplify arm_arch_timer drivers, and >> separate the ACPI GTDT knowledge from it. >> >> Signed-off-by: Fu Wei >> Signed-off-by: Hanjun Guo >> --- >> drivers/acpi/Kconfig | 5 ++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/arm64/Kconfig | 15 >> drivers/acpi/arm64/Makefile| 1 + >> drivers/acpi/arm64/acpi_gtdt.c | 170 >> + >> include/linux/acpi.h | 6 ++ >> 6 files changed, 198 insertions(+) >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index b7e2e77..1cdc7d2 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >> >> endif >> >> +if ARM64 >> +source "drivers/acpi/arm64/Kconfig" >> + >> +endif >> + >> endif # ACPI >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> index 251ce85..1a94ff7 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >> obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o >> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >> +obj-$(CONFIG_ARM64)+= arm64/ >> >> video-objs += acpi_video.o video_detect.o >> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >> new file mode 100644 >> index 000..ff5c253 >> --- /dev/null >> +++ b/drivers/acpi/arm64/Kconfig >> @@ -0,0 +1,15 @@ >> +# >> +# ACPI Configuration for ARM64 >> +# >> + >> +menu "The ARM64-specific ACPI Support" >> + >> +config ACPI_GTDT >> + bool "ACPI GTDT table Support" >> + help >> + GTDT (Generic Timer Description Table) provides information >> + for per-processor timers and Platform (memory-mapped) timers >> + for ARM platforms. Select this option to provide information >> + needed for the timers init. >> + >> +endmenu >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile >> new file mode 100644 >> index 000..466de6b >> --- /dev/null >> +++ b/drivers/acpi/arm64/Makefile >> @@ -0,0 +1 @@ >> +obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o >> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c >> new file mode 100644 >> index 000..9ee977d >> --- /dev/null >> +++ b/drivers/acpi/arm64/acpi_gtdt.c >> @@ -0,0 +1,170 @@ >> +/* >> + * ARM Specific GTDT table Support >> + * >> + * Copyright (C) 2016, Linaro Ltd. >> + * Author: Daniel Lezcano >> + * Fu Wei >> + * Hanjun Guo >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include >> +#include >> +#include >> +#include > > Please do not use module.h in drivers that are using a > bool Kconfig setting. yes, you are right, I forget to delete it, sorry. Will do, thanks > > Thanks, > Paul. -- Best regards, Fu Wei Software Engineer Red Hat
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
Hi Rafael, On 14 July 2016 at 05:43, Rafael J. Wysocki wrote: > On Wed, Jul 13, 2016 at 11:08 PM, Guenter Roeck wrote: >> On Wed, Jul 13, 2016 at 10:30:37PM +0200, Rafael J. Wysocki wrote: >>> On Wed, Jul 13, 2016 at 7:53 PM, wrote: >>> > From: Fu Wei >>> > >>> > This patch adds support for parsing arch timer in GTDT, >>> > provides some kernel APIs to parse all the PPIs and >>> > always-on info in GTDT and export them. >>> > >>> > By this driver, we can simplify arm_arch_timer drivers, and >>> > separate the ACPI GTDT knowledge from it. >>> > >>> > Signed-off-by: Fu Wei >>> > Signed-off-by: Hanjun Guo >>> > --- >>> > drivers/acpi/Kconfig | 5 ++ >>> > drivers/acpi/Makefile | 1 + >>> > drivers/acpi/arm64/Kconfig | 15 >>> > drivers/acpi/arm64/Makefile| 1 + >>> > drivers/acpi/arm64/acpi_gtdt.c | 170 >>> > + >>> > include/linux/acpi.h | 6 ++ >>> > 6 files changed, 198 insertions(+) >>> > >>> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >>> > index b7e2e77..1cdc7d2 100644 >>> > --- a/drivers/acpi/Kconfig >>> > +++ b/drivers/acpi/Kconfig >>> > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >>> > >>> > endif >>> > >>> > +if ARM64 >>> > +source "drivers/acpi/arm64/Kconfig" >>> > + >>> > +endif >>> > + >>> > endif # ACPI >>> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >>> > index 251ce85..1a94ff7 100644 >>> > --- a/drivers/acpi/Makefile >>> > +++ b/drivers/acpi/Makefile >>> > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >>> > obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o >>> > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >>> > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >>> > +obj-$(CONFIG_ARM64)+= arm64/ >>> > >>> > video-objs += acpi_video.o video_detect.o >>> > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >>> > new file mode 100644 >>> > index 000..ff5c253 >>> > --- /dev/null >>> > +++ b/drivers/acpi/arm64/Kconfig >>> > @@ -0,0 +1,15 @@ >>> > +# >>> > +# ACPI Configuration for ARM64 >>> > +# >>> > + >>> > +menu "The ARM64-specific ACPI Support" >>> > + >>> > +config ACPI_GTDT >>> > + bool "ACPI GTDT table Support" >>> >>> This should depend on ARM64. >>> >>> Also I wonder if it needs to be user-selectable? Wouldn't it be >>> better to enable it by default when building for ARM64 with ACPI? >>> >> It is currently selected in patch 9, in the watchdog driver's Kconfig >> entry. > > Well, it still doesn't have to be user-selectable for that. :-) Actually it is also automatically selected by [PATCH v7 6/9]: diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index 47352d2..71d5b30 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -8,6 +8,7 @@ config CLKSRC_OF config CLKSRC_ACPI bool select CLKSRC_PROBE + select ACPI_GTDT if ARM64 config CLKSRC_PROBE bool > >> Not sure if I like that; maybe the watchdog driver should depend >> on it instead ? > > If the watchdog is not the only user of it (and I don't think it is), > it would be better to arrange things this way. > There are two user: (1) arm_arch_timer(which will select CLKSRC_ACPI if ACPI, then CLKSRC_ACPI will select ACPI_GTDT if ARM64) So arm_arch_timer will automatically selecte ACPI_GTDT if ARM64 && ACPI (2) sbsa_gwdt (which will select ACPI_GTDT if ACPI in [PATCH v7 9/9]) So sbsa_gwdt will automatically selecte ACPI_GTDT if ARM64 && ACPI && ARM_ARCH_TIMER So ACPI_GTDT is automatically selected by both of two users. But like Timur said before: maybe we just "selecte ACPI_GTDT if ACPI" for ARM64, because ARM64 require GTDT if we use ACPI. > Thanks, > Rafael -- Best regards, Fu Wei Software Engineer Red Hat
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
Hi Rafael,
On 14 July 2016 at 04:39, Rafael J. Wysocki wrote:
> On Wed, Jul 13, 2016 at 10:30 PM, Rafael J. Wysocki wrote:
>> On Wed, Jul 13, 2016 at 7:53 PM, wrote:
>>> From: Fu Wei
>>>
>>> This patch adds support for parsing arch timer in GTDT,
>>> provides some kernel APIs to parse all the PPIs and
>>> always-on info in GTDT and export them.
>>>
>>> By this driver, we can simplify arm_arch_timer drivers, and
>>> separate the ACPI GTDT knowledge from it.
>>>
>>> Signed-off-by: Fu Wei
>>> Signed-off-by: Hanjun Guo
>>> ---
>>> drivers/acpi/Kconfig | 5 ++
>>> drivers/acpi/Makefile | 1 +
>>> drivers/acpi/arm64/Kconfig | 15
>>> drivers/acpi/arm64/Makefile| 1 +
>>> drivers/acpi/arm64/acpi_gtdt.c | 170
>>> +
>>> include/linux/acpi.h | 6 ++
>>> 6 files changed, 198 insertions(+)
>>>
>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>> index b7e2e77..1cdc7d2 100644
>>> --- a/drivers/acpi/Kconfig
>>> +++ b/drivers/acpi/Kconfig
>>> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION
>>>
>>> endif
>>>
>>> +if ARM64
>>> +source "drivers/acpi/arm64/Kconfig"
>>> +
>>> +endif
>>> +
>>> endif # ACPI
>>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>>> index 251ce85..1a94ff7 100644
>>> --- a/drivers/acpi/Makefile
>>> +++ b/drivers/acpi/Makefile
>>> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
>>> obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o
>>> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
>>> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
>>> +obj-$(CONFIG_ARM64)+= arm64/
>>>
>>> video-objs += acpi_video.o video_detect.o
>>> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
>>> new file mode 100644
>>> index 000..ff5c253
>>> --- /dev/null
>>> +++ b/drivers/acpi/arm64/Kconfig
>>> @@ -0,0 +1,15 @@
>>> +#
>>> +# ACPI Configuration for ARM64
>>> +#
>>> +
>>> +menu "The ARM64-specific ACPI Support"
>>> +
>>> +config ACPI_GTDT
>>> + bool "ACPI GTDT table Support"
>>
>> This should depend on ARM64.
>>
>> Also I wonder if it needs to be user-selectable? Wouldn't it be
>> better to enable it by default when building for ARM64 with ACPI?
>>
>>> + help
>>> + GTDT (Generic Timer Description Table) provides information
>>> + for per-processor timers and Platform (memory-mapped) timers
>>> + for ARM platforms. Select this option to provide information
>>> + needed for the timers init.
>>> +
>>> +endmenu
>>> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
>>> new file mode 100644
>>> index 000..466de6b
>>> --- /dev/null
>>> +++ b/drivers/acpi/arm64/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o
>>> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
>>> new file mode 100644
>>> index 000..9ee977d
>>> --- /dev/null
>>> +++ b/drivers/acpi/arm64/acpi_gtdt.c
>>> @@ -0,0 +1,170 @@
>>> +/*
>>> + * ARM Specific GTDT table Support
>>> + *
>>> + * Copyright (C) 2016, Linaro Ltd.
>>> + * Author: Daniel Lezcano
>>> + * Fu Wei
>>> + * Hanjun Guo
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include
>>> +
>>> +#undef pr_fmt
>>> +#define pr_fmt(fmt) "GTDT: " fmt
>>
>> I would add "ACPI" to the prefix too if I were you, but that's me.
>>
>>> +
>>> +typedef struct {
>>> + struct acpi_table_gtdt *gtdt;
>>> + void *platform_timer_start;
>>> + void *gtdt_end;
>>> +} acpi_gtdt_desc_t;
>>> +
>>> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata;
>>> +
>>> +static inline void *next_platform_timer(void *platform_timer)
>>> +{
>>> + struct acpi_gtdt_header *gh = platform_timer;
>>> +
>>> + platform_timer += gh->length;
>>> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
>>> + return platform_timer;
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> +#define for_each_platform_timer(_g)\
>>> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \
>>> +_g = next_platform_timer(_g))
>>> +
>>> +static inline bool is_timer_block(void *platform_timer)
>>> +{
>>> + struct acpi_gtdt_header *gh = platform_timer;
>>> +
>>> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
>>> + return true;
>>> +
>>> + return false;
>>
>> This is just too much code. It would suffice to do
>>
>> return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
>>
>>> +}
>>> +
>>> +static inline bool is_watchdog(void *platform_timer)
>>> +{
>>> + struct acpi_gtdt_header *gh = platform_timer;
>>> +
>>> + if (gh->type == ACPI_GTDT_TY
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
Hi Rafael,
On 14 July 2016 at 04:30, Rafael J. Wysocki wrote:
> On Wed, Jul 13, 2016 at 7:53 PM, wrote:
>> From: Fu Wei
>>
>> This patch adds support for parsing arch timer in GTDT,
>> provides some kernel APIs to parse all the PPIs and
>> always-on info in GTDT and export them.
>>
>> By this driver, we can simplify arm_arch_timer drivers, and
>> separate the ACPI GTDT knowledge from it.
>>
>> Signed-off-by: Fu Wei
>> Signed-off-by: Hanjun Guo
>> ---
>> drivers/acpi/Kconfig | 5 ++
>> drivers/acpi/Makefile | 1 +
>> drivers/acpi/arm64/Kconfig | 15
>> drivers/acpi/arm64/Makefile| 1 +
>> drivers/acpi/arm64/acpi_gtdt.c | 170
>> +
>> include/linux/acpi.h | 6 ++
>> 6 files changed, 198 insertions(+)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index b7e2e77..1cdc7d2 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION
>>
>> endif
>>
>> +if ARM64
>> +source "drivers/acpi/arm64/Kconfig"
>> +
>> +endif
>> +
>> endif # ACPI
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 251ce85..1a94ff7 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
>> obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o
>> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
>> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
>> +obj-$(CONFIG_ARM64)+= arm64/
>>
>> video-objs += acpi_video.o video_detect.o
>> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
>> new file mode 100644
>> index 000..ff5c253
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/Kconfig
>> @@ -0,0 +1,15 @@
>> +#
>> +# ACPI Configuration for ARM64
>> +#
>> +
>> +menu "The ARM64-specific ACPI Support"
>> +
>> +config ACPI_GTDT
>> + bool "ACPI GTDT table Support"
>
> This should depend on ARM64.
>
> Also I wonder if it needs to be user-selectable? Wouldn't it be
> better to enable it by default when building for ARM64 with ACPI?
>
>> + help
>> + GTDT (Generic Timer Description Table) provides information
>> + for per-processor timers and Platform (memory-mapped) timers
>> + for ARM platforms. Select this option to provide information
>> + needed for the timers init.
>> +
>> +endmenu
>> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
>> new file mode 100644
>> index 000..466de6b
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o
>> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
>> new file mode 100644
>> index 000..9ee977d
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/acpi_gtdt.c
>> @@ -0,0 +1,170 @@
>> +/*
>> + * ARM Specific GTDT table Support
>> + *
>> + * Copyright (C) 2016, Linaro Ltd.
>> + * Author: Daniel Lezcano
>> + * Fu Wei
>> + * Hanjun Guo
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +
>> +#undef pr_fmt
>> +#define pr_fmt(fmt) "GTDT: " fmt
>
> I would add "ACPI" to the prefix too if I were you, but that's me.
good idea, you are right, will do
>
>> +
>> +typedef struct {
>> + struct acpi_table_gtdt *gtdt;
>> + void *platform_timer_start;
>> + void *gtdt_end;
>> +} acpi_gtdt_desc_t;
>> +
>> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata;
>> +
>> +static inline void *next_platform_timer(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + platform_timer += gh->length;
>> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
>> + return platform_timer;
>> +
>> + return NULL;
>> +}
>> +
>> +#define for_each_platform_timer(_g)\
>> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \
>> +_g = next_platform_timer(_g))
>> +
>> +static inline bool is_timer_block(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
>> + return true;
>> +
>> + return false;
>
> This is just too much code. It would suffice to do
>
> return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
>
>> +}
>> +
>> +static inline bool is_watchdog(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG)
>> + return true;
>> +
>> + return false;
>
> Just like above.
Thanks, this is better :-) will do
>
>> +}
>> +
>> +/*
>> + * Get some basic inf
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Wed, Jul 13, 2016 at 1:53 PM, wrote: > From: Fu Wei > > This patch adds support for parsing arch timer in GTDT, > provides some kernel APIs to parse all the PPIs and > always-on info in GTDT and export them. > > By this driver, we can simplify arm_arch_timer drivers, and > separate the ACPI GTDT knowledge from it. > > Signed-off-by: Fu Wei > Signed-off-by: Hanjun Guo > --- > drivers/acpi/Kconfig | 5 ++ > drivers/acpi/Makefile | 1 + > drivers/acpi/arm64/Kconfig | 15 > drivers/acpi/arm64/Makefile| 1 + > drivers/acpi/arm64/acpi_gtdt.c | 170 > + > include/linux/acpi.h | 6 ++ > 6 files changed, 198 insertions(+) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index b7e2e77..1cdc7d2 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION > > endif > > +if ARM64 > +source "drivers/acpi/arm64/Kconfig" > + > +endif > + > endif # ACPI > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index 251ce85..1a94ff7 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o > obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o > +obj-$(CONFIG_ARM64)+= arm64/ > > video-objs += acpi_video.o video_detect.o > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > new file mode 100644 > index 000..ff5c253 > --- /dev/null > +++ b/drivers/acpi/arm64/Kconfig > @@ -0,0 +1,15 @@ > +# > +# ACPI Configuration for ARM64 > +# > + > +menu "The ARM64-specific ACPI Support" > + > +config ACPI_GTDT > + bool "ACPI GTDT table Support" > + help > + GTDT (Generic Timer Description Table) provides information > + for per-processor timers and Platform (memory-mapped) timers > + for ARM platforms. Select this option to provide information > + needed for the timers init. > + > +endmenu > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > new file mode 100644 > index 000..466de6b > --- /dev/null > +++ b/drivers/acpi/arm64/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o > diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c > new file mode 100644 > index 000..9ee977d > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_gtdt.c > @@ -0,0 +1,170 @@ > +/* > + * ARM Specific GTDT table Support > + * > + * Copyright (C) 2016, Linaro Ltd. > + * Author: Daniel Lezcano > + * Fu Wei > + * Hanjun Guo > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include > +#include > +#include > +#include Please do not use module.h in drivers that are using a bool Kconfig setting. Thanks, Paul.
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Wed, Jul 13, 2016 at 11:08 PM, Guenter Roeck wrote: > On Wed, Jul 13, 2016 at 10:30:37PM +0200, Rafael J. Wysocki wrote: >> On Wed, Jul 13, 2016 at 7:53 PM, wrote: >> > From: Fu Wei >> > >> > This patch adds support for parsing arch timer in GTDT, >> > provides some kernel APIs to parse all the PPIs and >> > always-on info in GTDT and export them. >> > >> > By this driver, we can simplify arm_arch_timer drivers, and >> > separate the ACPI GTDT knowledge from it. >> > >> > Signed-off-by: Fu Wei >> > Signed-off-by: Hanjun Guo >> > --- >> > drivers/acpi/Kconfig | 5 ++ >> > drivers/acpi/Makefile | 1 + >> > drivers/acpi/arm64/Kconfig | 15 >> > drivers/acpi/arm64/Makefile| 1 + >> > drivers/acpi/arm64/acpi_gtdt.c | 170 >> > + >> > include/linux/acpi.h | 6 ++ >> > 6 files changed, 198 insertions(+) >> > >> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> > index b7e2e77..1cdc7d2 100644 >> > --- a/drivers/acpi/Kconfig >> > +++ b/drivers/acpi/Kconfig >> > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION >> > >> > endif >> > >> > +if ARM64 >> > +source "drivers/acpi/arm64/Kconfig" >> > + >> > +endif >> > + >> > endif # ACPI >> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> > index 251ce85..1a94ff7 100644 >> > --- a/drivers/acpi/Makefile >> > +++ b/drivers/acpi/Makefile >> > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o >> > obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o >> > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o >> > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o >> > +obj-$(CONFIG_ARM64)+= arm64/ >> > >> > video-objs += acpi_video.o video_detect.o >> > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig >> > new file mode 100644 >> > index 000..ff5c253 >> > --- /dev/null >> > +++ b/drivers/acpi/arm64/Kconfig >> > @@ -0,0 +1,15 @@ >> > +# >> > +# ACPI Configuration for ARM64 >> > +# >> > + >> > +menu "The ARM64-specific ACPI Support" >> > + >> > +config ACPI_GTDT >> > + bool "ACPI GTDT table Support" >> >> This should depend on ARM64. >> >> Also I wonder if it needs to be user-selectable? Wouldn't it be >> better to enable it by default when building for ARM64 with ACPI? >> > It is currently selected in patch 9, in the watchdog driver's Kconfig > entry. Well, it still doesn't have to be user-selectable for that. :-) > Not sure if I like that; maybe the watchdog driver should depend > on it instead ? If the watchdog is not the only user of it (and I don't think it is), it would be better to arrange things this way. Thanks, Rafael
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Wed, Jul 13, 2016 at 10:30:37PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jul 13, 2016 at 7:53 PM, wrote:
> > From: Fu Wei
> >
> > This patch adds support for parsing arch timer in GTDT,
> > provides some kernel APIs to parse all the PPIs and
> > always-on info in GTDT and export them.
> >
> > By this driver, we can simplify arm_arch_timer drivers, and
> > separate the ACPI GTDT knowledge from it.
> >
> > Signed-off-by: Fu Wei
> > Signed-off-by: Hanjun Guo
> > ---
> > drivers/acpi/Kconfig | 5 ++
> > drivers/acpi/Makefile | 1 +
> > drivers/acpi/arm64/Kconfig | 15
> > drivers/acpi/arm64/Makefile| 1 +
> > drivers/acpi/arm64/acpi_gtdt.c | 170
> > +
> > include/linux/acpi.h | 6 ++
> > 6 files changed, 198 insertions(+)
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index b7e2e77..1cdc7d2 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION
> >
> > endif
> >
> > +if ARM64
> > +source "drivers/acpi/arm64/Kconfig"
> > +
> > +endif
> > +
> > endif # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index 251ce85..1a94ff7 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
> > obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o
> > obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
> > obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
> > +obj-$(CONFIG_ARM64)+= arm64/
> >
> > video-objs += acpi_video.o video_detect.o
> > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
> > new file mode 100644
> > index 000..ff5c253
> > --- /dev/null
> > +++ b/drivers/acpi/arm64/Kconfig
> > @@ -0,0 +1,15 @@
> > +#
> > +# ACPI Configuration for ARM64
> > +#
> > +
> > +menu "The ARM64-specific ACPI Support"
> > +
> > +config ACPI_GTDT
> > + bool "ACPI GTDT table Support"
>
> This should depend on ARM64.
>
> Also I wonder if it needs to be user-selectable? Wouldn't it be
> better to enable it by default when building for ARM64 with ACPI?
>
It is currently selected in patch 9, in the watchdog driver's Kconfig
entry. Not sure if I like that; maybe the watchdog driver should depend
on it instead ?
Guenter
> > + help
> > + GTDT (Generic Timer Description Table) provides information
> > + for per-processor timers and Platform (memory-mapped) timers
> > + for ARM platforms. Select this option to provide information
> > + needed for the timers init.
> > +
> > +endmenu
> > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
> > new file mode 100644
> > index 000..466de6b
> > --- /dev/null
> > +++ b/drivers/acpi/arm64/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o
> > diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
> > new file mode 100644
> > index 000..9ee977d
> > --- /dev/null
> > +++ b/drivers/acpi/arm64/acpi_gtdt.c
> > @@ -0,0 +1,170 @@
> > +/*
> > + * ARM Specific GTDT table Support
> > + *
> > + * Copyright (C) 2016, Linaro Ltd.
> > + * Author: Daniel Lezcano
> > + * Fu Wei
> > + * Hanjun Guo
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#undef pr_fmt
> > +#define pr_fmt(fmt) "GTDT: " fmt
>
> I would add "ACPI" to the prefix too if I were you, but that's me.
>
> > +
> > +typedef struct {
> > + struct acpi_table_gtdt *gtdt;
> > + void *platform_timer_start;
> > + void *gtdt_end;
> > +} acpi_gtdt_desc_t;
> > +
> > +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata;
> > +
> > +static inline void *next_platform_timer(void *platform_timer)
> > +{
> > + struct acpi_gtdt_header *gh = platform_timer;
> > +
> > + platform_timer += gh->length;
> > + if (platform_timer < acpi_gtdt_desc.gtdt_end)
> > + return platform_timer;
> > +
> > + return NULL;
> > +}
> > +
> > +#define for_each_platform_timer(_g)\
> > + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \
> > +_g = next_platform_timer(_g))
> > +
> > +static inline bool is_timer_block(void *platform_timer)
> > +{
> > + struct acpi_gtdt_header *gh = platform_timer;
> > +
> > + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
> > + return true;
> > +
> > + return false;
>
> This is just too much code. It would suffice to do
>
> return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
>
> > +}
> > +
> > +static inline bool is_watchdog(void *platform_timer)
> > +{
>
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Wed, Jul 13, 2016 at 10:30 PM, Rafael J. Wysocki wrote:
> On Wed, Jul 13, 2016 at 7:53 PM, wrote:
>> From: Fu Wei
>>
>> This patch adds support for parsing arch timer in GTDT,
>> provides some kernel APIs to parse all the PPIs and
>> always-on info in GTDT and export them.
>>
>> By this driver, we can simplify arm_arch_timer drivers, and
>> separate the ACPI GTDT knowledge from it.
>>
>> Signed-off-by: Fu Wei
>> Signed-off-by: Hanjun Guo
>> ---
>> drivers/acpi/Kconfig | 5 ++
>> drivers/acpi/Makefile | 1 +
>> drivers/acpi/arm64/Kconfig | 15
>> drivers/acpi/arm64/Makefile| 1 +
>> drivers/acpi/arm64/acpi_gtdt.c | 170
>> +
>> include/linux/acpi.h | 6 ++
>> 6 files changed, 198 insertions(+)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index b7e2e77..1cdc7d2 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION
>>
>> endif
>>
>> +if ARM64
>> +source "drivers/acpi/arm64/Kconfig"
>> +
>> +endif
>> +
>> endif # ACPI
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 251ce85..1a94ff7 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
>> obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o
>> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
>> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
>> +obj-$(CONFIG_ARM64)+= arm64/
>>
>> video-objs += acpi_video.o video_detect.o
>> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
>> new file mode 100644
>> index 000..ff5c253
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/Kconfig
>> @@ -0,0 +1,15 @@
>> +#
>> +# ACPI Configuration for ARM64
>> +#
>> +
>> +menu "The ARM64-specific ACPI Support"
>> +
>> +config ACPI_GTDT
>> + bool "ACPI GTDT table Support"
>
> This should depend on ARM64.
>
> Also I wonder if it needs to be user-selectable? Wouldn't it be
> better to enable it by default when building for ARM64 with ACPI?
>
>> + help
>> + GTDT (Generic Timer Description Table) provides information
>> + for per-processor timers and Platform (memory-mapped) timers
>> + for ARM platforms. Select this option to provide information
>> + needed for the timers init.
>> +
>> +endmenu
>> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
>> new file mode 100644
>> index 000..466de6b
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o
>> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
>> new file mode 100644
>> index 000..9ee977d
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/acpi_gtdt.c
>> @@ -0,0 +1,170 @@
>> +/*
>> + * ARM Specific GTDT table Support
>> + *
>> + * Copyright (C) 2016, Linaro Ltd.
>> + * Author: Daniel Lezcano
>> + * Fu Wei
>> + * Hanjun Guo
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +
>> +#undef pr_fmt
>> +#define pr_fmt(fmt) "GTDT: " fmt
>
> I would add "ACPI" to the prefix too if I were you, but that's me.
>
>> +
>> +typedef struct {
>> + struct acpi_table_gtdt *gtdt;
>> + void *platform_timer_start;
>> + void *gtdt_end;
>> +} acpi_gtdt_desc_t;
>> +
>> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata;
>> +
>> +static inline void *next_platform_timer(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + platform_timer += gh->length;
>> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
>> + return platform_timer;
>> +
>> + return NULL;
>> +}
>> +
>> +#define for_each_platform_timer(_g)\
>> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \
>> +_g = next_platform_timer(_g))
>> +
>> +static inline bool is_timer_block(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
>> + return true;
>> +
>> + return false;
>
> This is just too much code. It would suffice to do
>
> return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
>
>> +}
>> +
>> +static inline bool is_watchdog(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG)
>> + return true;
>> +
>> + return false;
>
> Just like above.
>
>> +}
>> +
>> +/*
>> + * Get some basic info from GTDT table, and init the global variables above
>> + * for all timers init
Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
On Wed, Jul 13, 2016 at 7:53 PM, wrote:
> From: Fu Wei
>
> This patch adds support for parsing arch timer in GTDT,
> provides some kernel APIs to parse all the PPIs and
> always-on info in GTDT and export them.
>
> By this driver, we can simplify arm_arch_timer drivers, and
> separate the ACPI GTDT knowledge from it.
>
> Signed-off-by: Fu Wei
> Signed-off-by: Hanjun Guo
> ---
> drivers/acpi/Kconfig | 5 ++
> drivers/acpi/Makefile | 1 +
> drivers/acpi/arm64/Kconfig | 15
> drivers/acpi/arm64/Makefile| 1 +
> drivers/acpi/arm64/acpi_gtdt.c | 170
> +
> include/linux/acpi.h | 6 ++
> 6 files changed, 198 insertions(+)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index b7e2e77..1cdc7d2 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION
>
> endif
>
> +if ARM64
> +source "drivers/acpi/arm64/Kconfig"
> +
> +endif
> +
> endif # ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 251ce85..1a94ff7 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
> obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o
> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
> +obj-$(CONFIG_ARM64)+= arm64/
>
> video-objs += acpi_video.o video_detect.o
> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
> new file mode 100644
> index 000..ff5c253
> --- /dev/null
> +++ b/drivers/acpi/arm64/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# ACPI Configuration for ARM64
> +#
> +
> +menu "The ARM64-specific ACPI Support"
> +
> +config ACPI_GTDT
> + bool "ACPI GTDT table Support"
This should depend on ARM64.
Also I wonder if it needs to be user-selectable? Wouldn't it be
better to enable it by default when building for ARM64 with ACPI?
> + help
> + GTDT (Generic Timer Description Table) provides information
> + for per-processor timers and Platform (memory-mapped) timers
> + for ARM platforms. Select this option to provide information
> + needed for the timers init.
> +
> +endmenu
> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
> new file mode 100644
> index 000..466de6b
> --- /dev/null
> +++ b/drivers/acpi/arm64/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o
> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
> new file mode 100644
> index 000..9ee977d
> --- /dev/null
> +++ b/drivers/acpi/arm64/acpi_gtdt.c
> @@ -0,0 +1,170 @@
> +/*
> + * ARM Specific GTDT table Support
> + *
> + * Copyright (C) 2016, Linaro Ltd.
> + * Author: Daniel Lezcano
> + * Fu Wei
> + * Hanjun Guo
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#undef pr_fmt
> +#define pr_fmt(fmt) "GTDT: " fmt
I would add "ACPI" to the prefix too if I were you, but that's me.
> +
> +typedef struct {
> + struct acpi_table_gtdt *gtdt;
> + void *platform_timer_start;
> + void *gtdt_end;
> +} acpi_gtdt_desc_t;
> +
> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata;
> +
> +static inline void *next_platform_timer(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + platform_timer += gh->length;
> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
> + return platform_timer;
> +
> + return NULL;
> +}
> +
> +#define for_each_platform_timer(_g)\
> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \
> +_g = next_platform_timer(_g))
> +
> +static inline bool is_timer_block(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
> + return true;
> +
> + return false;
This is just too much code. It would suffice to do
return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
> +}
> +
> +static inline bool is_watchdog(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG)
> + return true;
> +
> + return false;
Just like above.
> +}
> +
> +/*
> + * Get some basic info from GTDT table, and init the global variables above
> + * for all timers initialization of Generic Timer.
> + * This function does some validation on GTDT table.
> + */
> +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table)
> +{
> + struct acpi_table_gtdt *gtdt = container_of(table,
> +
[PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver
From: Fu Wei
This patch adds support for parsing arch timer in GTDT,
provides some kernel APIs to parse all the PPIs and
always-on info in GTDT and export them.
By this driver, we can simplify arm_arch_timer drivers, and
separate the ACPI GTDT knowledge from it.
Signed-off-by: Fu Wei
Signed-off-by: Hanjun Guo
---
drivers/acpi/Kconfig | 5 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/arm64/Kconfig | 15
drivers/acpi/arm64/Makefile| 1 +
drivers/acpi/arm64/acpi_gtdt.c | 170 +
include/linux/acpi.h | 6 ++
6 files changed, 198 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index b7e2e77..1cdc7d2 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION
endif
+if ARM64
+source "drivers/acpi/arm64/Kconfig"
+
+endif
+
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 251ce85..1a94ff7 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
obj-$(CONFIG_PMIC_OPREGION)+= pmic/intel_pmic.o
obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
+obj-$(CONFIG_ARM64)+= arm64/
video-objs += acpi_video.o video_detect.o
diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
new file mode 100644
index 000..ff5c253
--- /dev/null
+++ b/drivers/acpi/arm64/Kconfig
@@ -0,0 +1,15 @@
+#
+# ACPI Configuration for ARM64
+#
+
+menu "The ARM64-specific ACPI Support"
+
+config ACPI_GTDT
+ bool "ACPI GTDT table Support"
+ help
+ GTDT (Generic Timer Description Table) provides information
+ for per-processor timers and Platform (memory-mapped) timers
+ for ARM platforms. Select this option to provide information
+ needed for the timers init.
+
+endmenu
diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
new file mode 100644
index 000..466de6b
--- /dev/null
+++ b/drivers/acpi/arm64/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_ACPI_GTDT)+= acpi_gtdt.o
diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
new file mode 100644
index 000..9ee977d
--- /dev/null
+++ b/drivers/acpi/arm64/acpi_gtdt.c
@@ -0,0 +1,170 @@
+/*
+ * ARM Specific GTDT table Support
+ *
+ * Copyright (C) 2016, Linaro Ltd.
+ * Author: Daniel Lezcano
+ * Fu Wei
+ * Hanjun Guo
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+#undef pr_fmt
+#define pr_fmt(fmt) "GTDT: " fmt
+
+typedef struct {
+ struct acpi_table_gtdt *gtdt;
+ void *platform_timer_start;
+ void *gtdt_end;
+} acpi_gtdt_desc_t;
+
+static acpi_gtdt_desc_t acpi_gtdt_desc __initdata;
+
+static inline void *next_platform_timer(void *platform_timer)
+{
+ struct acpi_gtdt_header *gh = platform_timer;
+
+ platform_timer += gh->length;
+ if (platform_timer < acpi_gtdt_desc.gtdt_end)
+ return platform_timer;
+
+ return NULL;
+}
+
+#define for_each_platform_timer(_g)\
+ for (_g = acpi_gtdt_desc.platform_timer_start; _g; \
+_g = next_platform_timer(_g))
+
+static inline bool is_timer_block(void *platform_timer)
+{
+ struct acpi_gtdt_header *gh = platform_timer;
+
+ if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
+ return true;
+
+ return false;
+}
+
+static inline bool is_watchdog(void *platform_timer)
+{
+ struct acpi_gtdt_header *gh = platform_timer;
+
+ if (gh->type == ACPI_GTDT_TYPE_WATCHDOG)
+ return true;
+
+ return false;
+}
+
+/*
+ * Get some basic info from GTDT table, and init the global variables above
+ * for all timers initialization of Generic Timer.
+ * This function does some validation on GTDT table.
+ */
+static int __init acpi_gtdt_desc_init(struct acpi_table_header *table)
+{
+ struct acpi_table_gtdt *gtdt = container_of(table,
+ struct acpi_table_gtdt,
+ header);
+
+ acpi_gtdt_desc.gtdt = gtdt;
+ acpi_gtdt_desc.gtdt_end = (void *)table + table->length;
+
+ if (table->revision < 2) {
+ pr_info("Revision:%d doesn't support Platform Timers.\n",
+ table->revision);
+ return 0;
+ }
+
+ if (!gtdt->platform_timer_count) {
+ pr_info("No Platform Timer.\n");
+ return 0;
+ }
+
+ acpi_gtdt_desc.platform_timer_start = (void *)gtdt +
+ gtdt->platform_timer_offset;
+
