From: fu.wei@linaro.org (Fu Wei)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v22 07/11] acpi/arm64: Add GTDT table parse driver
Date: Wed, 29 Mar 2017 21:42:39 +0800 [thread overview]
Message-ID: <CADyBb7snT+fvZYDyjUW7ZCVLX-ha4VXYBhfZsi8a3wOeYtdHkQ@mail.gmail.com> (raw)
In-Reply-To: <20170329113305.GA10960@red-moon>
Hi Lorenzo,
On 29 March 2017 at 19:33, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Wed, Mar 29, 2017 at 06:48:26PM +0800, Fu Wei wrote:
>> Hi Lorenzo,
>>
>> On 29 March 2017 at 18:21, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> > On Wed, Mar 29, 2017 at 05:48:17PM +0800, Fu Wei wrote:
>> >
>> > [...]
>> >
>> >> * @platform_timer_count: It points to a integer variable which is used
>> >> * for storing the number of platform timers.
>> >> * This pointer could be NULL, if the caller
>> >> * doesn't need this info.
>> >>
>> >> >
>> >> >> + *
>> >> >> + * Return: 0 if success, -EINVAL if error.
>> >> >> + */
>> >> >> +int __init acpi_gtdt_init(struct acpi_table_header *table,
>> >> >> + int *platform_timer_count)
>> >> >> +{
>> >> >> + int ret = 0;
>> >> >> + int timer_count = 0;
>> >> >> + void *platform_timer = NULL;
>> >> >> + struct acpi_table_gtdt *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_warn("Revision:%d doesn't support Platform Timers.\n",
>> >> >> + table->revision);
>> >> >
>> >> > Ok, two points here. First, I am not sure why you should warn if the
>> >> > table revision is < 2, is that a FW bug ? I do not think it is, you
>> >> > can just return 0.
>> >>
>> >> I used pr_debug here before v20, then I got Hanjun's suggestion:
>> >> -------
>> >> GTDT table revision is updated to 2 in ACPI 5.1, we will
>> >> not support ACPI version under 5.1 and disable ACPI in FADT
>> >> parse before this code is called, so if we get revision
>> >> <2 here, I think we need to print warning (we need to keep
>> >> the firmware stick to the spec on ARM64).
>> >> -------
>> >> https://lkml.org/lkml/2017/1/19/82
>> >>
>> >> So I started to use pr_warn.
>> >
>> > Thanks for the explanation, so it is a FW bug and the warning
>> > is granted :) just leave it there.
>> >
>> > Still, please check my comment on acpi_gtdt_init() being called
>> > multiple times on patch 11.
>>
>> Thanks
>>
>> For calling acpi_gtdt_init() twice:
>> (1) 1st time: in early boot(bootmem), for init arch_timer and
>> memory-mapped timer, we initialize the acpi_gtdt_desc.
>> you can see that all the items in this struct are pointer.
>> (2) 2nd time: when system switch from bootmem to slab, all the
>> pointers in the acpi_gtdt_desc are invalid, so we have to
>> re-initialize(re-map) them.
>>
>> I have tested it, if we don't re-initialize the acpi_gtdt_desc,
>> system will go wrong.
>
> Ok, that's what I feared. My complaint on patch 11 is that:
>
> 1) Stashing the GTDT pointer in acpi_gtdt_desc is not needed to
> parse SBSA watchdogs
The acpi_gtdt_desc is for sharing the info between acpi_gtdt_init and
acpi_gtdt_c3stop, ;acpi_gtdt_map_ppi
I re-use it in parsing SBSA watchdogs, because I try to re-use acpi_gtdt_init.
> 2) It is not clear at all from the code or the commit log _why_ you
> need to call acpi_gtdt_init() again (ie technically you don't need
> to call it - you grab a valid pointer to the table and parse the
> watchdogs in the _same_ function gtdt_sbsa_gwdt_init())
yes, we can avoid calling acpi_gtdt_init(), do the same thing in
parsing SBSA watchdogs info.
But if we will do the same thing(getting gtdt, platform_timer,
timer_count), why not just re-using the same function?
So my suggestion is that:
Could we re-use acpi_gtdt_init, and a comment at the head of SBSA
watchdogs info parsing function to summarize this issue?
The comment like this
Note: although the global variable acpi_gtdt_desc has been initialized
by acpi_gtdt_init, when we initialized arch_timer. But when we call this
function to get SBSA watchdogs info from GTDT, the system has switch
from bootmem to slab, so the pointers in acpi_gtdt_desc are dated, we
need to re-initialize(remap) them. So we call acpi_gtdt_init again here.
Is that OK for you? :-)
>
> I do not think there is much you can do to improve the arch timer gtdt
> interface (and it is too late for that anyway) but in patch 11 it would
> be ideal if you avoid calling acpi_gtdt_init() again, just parse GTDT
> entries and initialize the corresponding watchdogs (ie pointers stashed
> in acpi_gtdt_desc are stale anyway but that's __initdata so I can live
> with that).
>
> You should add comments to summarize this issue so that it can be
> easily understood by anyone maintaining this code, it is not crystal
> clear by reading the code why you need to multiple acpi_gtdt_init()
> calls and that's not a piffling detail.
>
> The ACPI patches are fine with me otherwise, I will complete the
> review shortly.
>
> Thanks !
> Lorenzo
--
Best regards,
Fu Wei
Software Engineer
Red Hat
next prev parent reply other threads:[~2017-03-29 13:42 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-21 16:31 [PATCH v22 00/11] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer fu.wei at linaro.org
2017-03-21 16:31 ` [PATCH v22 01/11] clocksource: arm_arch_timer: introduce a wrapper function to get the frequency from mmio fu.wei at linaro.org
2017-03-21 16:31 ` [PATCH v22 02/11] clocksource: arm_arch_timer: separate out device-tree code and remove arch_timer_detect_rate fu.wei at linaro.org
2017-03-28 14:58 ` Daniel Lezcano
2017-03-29 3:41 ` Fu Wei
2017-03-29 5:11 ` Fu Wei
2017-03-29 14:41 ` Daniel Lezcano
2017-03-29 15:01 ` Fu Wei
2017-03-21 16:31 ` [PATCH v22 03/11] clocksource: arm_arch_timer: refactor arch_timer_needs_probing fu.wei at linaro.org
2017-03-28 15:02 ` Daniel Lezcano
2017-03-29 15:24 ` Mark Rutland
2017-03-29 15:32 ` Daniel Lezcano
2017-03-21 16:31 ` [PATCH v22 04/11] clocksource: arm_arch_timer: move arch_timer_needs_of_probing into DT init call fu.wei at linaro.org
2017-03-21 16:31 ` [PATCH v22 05/11] clocksource: arm_arch_timer: introduce some new structs to prepare for GTDT fu.wei at linaro.org
2017-03-21 16:31 ` [PATCH v22 06/11] clocksource: arm_arch_timer: refactor MMIO timer probing fu.wei at linaro.org
2017-03-21 16:31 ` [PATCH v22 07/11] acpi/arm64: Add GTDT table parse driver fu.wei at linaro.org
2017-03-28 11:35 ` Lorenzo Pieralisi
2017-03-29 9:48 ` Fu Wei
2017-03-29 10:21 ` Lorenzo Pieralisi
2017-03-29 10:48 ` Fu Wei
2017-03-29 11:33 ` Lorenzo Pieralisi
2017-03-29 13:42 ` Fu Wei [this message]
2017-03-29 16:02 ` Lorenzo Pieralisi
2017-03-29 14:29 ` Fu Wei
2017-03-29 14:31 ` Fu Wei
2017-03-29 15:19 ` Lorenzo Pieralisi
2017-03-21 16:31 ` [PATCH v22 08/11] clocksource: arm_arch_timer: simplify ACPI support code fu.wei at linaro.org
2017-03-21 16:31 ` [PATCH v22 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver fu.wei at linaro.org
2017-03-29 16:47 ` Lorenzo Pieralisi
2017-03-30 7:54 ` Fu Wei
2017-03-21 16:31 ` [PATCH v22 10/11] clocksource: arm_arch_timer: add GTDT support for memory-mapped timer fu.wei at linaro.org
2017-03-21 16:31 ` [PATCH v22 11/11] acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver fu.wei at linaro.org
2017-03-28 15:41 ` Lorenzo Pieralisi
2017-03-31 8:10 ` Fu Wei
2017-03-31 11:54 ` Lorenzo Pieralisi
2017-03-28 11:32 ` [PATCH v22 00/11] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer Jon Masters
2017-03-28 12:34 ` Fu Wei
2017-03-28 13:05 ` Mark Rutland
2017-03-28 14:29 ` Fu Wei
2017-03-28 14:53 ` Mark Rutland
2017-03-31 17:55 ` Fu Wei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADyBb7snT+fvZYDyjUW7ZCVLX-ha4VXYBhfZsi8a3wOeYtdHkQ@mail.gmail.com \
--to=fu.wei@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).