From: fu.wei@linaro.org (Fu Wei)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v22 11/11] acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver
Date: Fri, 31 Mar 2017 16:10:43 +0800 [thread overview]
Message-ID: <CADyBb7uDyjhP2Pjtzh6pTN-0hkokB=KevY7V76G9qZOF-9-zoQ@mail.gmail.com> (raw)
In-Reply-To: <20170328154111.GC18046@red-moon>
Hi Lorenzo,
On 28 March 2017 at 23:41, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Wed, Mar 22, 2017 at 12:31:22AM +0800, fu.wei at linaro.org wrote:
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> This driver adds support for parsing SBSA Generic Watchdog timer
>> in GTDT, parse all info in SBSA Generic Watchdog Structure in GTDT,
>> and creating a platform device with that information.
>>
>> This allows the operating system to obtain device data from the
>> resource of platform device. The platform device named "sbsa-gwdt"
>> can be used by the ARM SBSA Generic Watchdog driver.
>>
>> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Tested-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>> ---
>> drivers/acpi/arm64/gtdt.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 94 insertions(+)
>>
>> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
>> index f471873..5d167f0 100644
>> --- a/drivers/acpi/arm64/gtdt.c
>> +++ b/drivers/acpi/arm64/gtdt.c
>> @@ -14,6 +14,7 @@
>> #include <linux/acpi.h>
>> #include <linux/init.h>
>> #include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>>
>> #include <clocksource/arm_arch_timer.h>
>>
>> @@ -59,6 +60,17 @@ static inline bool is_timer_block(void *platform_timer)
>> return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
>> }
>>
>> +static inline bool is_non_secure_watchdog(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> + struct acpi_gtdt_watchdog *wd = platform_timer;
>> +
>> + if (gh->type != ACPI_GTDT_TYPE_WATCHDOG)
>> + return false;
>> +
>> + return !(wd->timer_flags & ACPI_GTDT_WATCHDOG_SECURE);
>> +}
>> +
>> static int __init map_gt_gsi(u32 interrupt, u32 flags)
>> {
>> int trigger, polarity;
>> @@ -285,3 +297,85 @@ int __init acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem,
>>
>> return 0;
>> }
>> +
>> +/*
>> + * Initialize a SBSA generic Watchdog platform device info from GTDT
>> + */
>> +static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd,
>> + int index)
>> +{
>> + struct platform_device *pdev;
>> + int irq = map_gt_gsi(wd->timer_interrupt, wd->timer_flags);
>> +
>> + /*
>> + * According to SBSA specification the size of refresh and control
>> + * frames of SBSA Generic Watchdog is SZ_4K(Offset 0x000 ? 0xFFF).
>> + */
>> + struct resource res[] = {
>> + DEFINE_RES_MEM(wd->control_frame_address, SZ_4K),
>> + DEFINE_RES_MEM(wd->refresh_frame_address, SZ_4K),
>> + DEFINE_RES_IRQ(irq),
>> + };
>> + int nr_res = ARRAY_SIZE(res);
>> +
>> + pr_debug("found a Watchdog (0x%llx/0x%llx gsi:%u flags:0x%x).\n",
>> + wd->refresh_frame_address, wd->control_frame_address,
>> + wd->timer_interrupt, wd->timer_flags);
>> +
>> + if (!(wd->refresh_frame_address && wd->control_frame_address)) {
>> + pr_err(FW_BUG "failed to get the Watchdog base address.\n");
+ acpi_unregister_gsi(wd->timer_interrupt);
>> + return -EINVAL;
>
> You should unmap the gsi here.
yes, you are right, fixed it.
>
>> + }
>> +
>> + if (irq <= 0) {
>> + pr_warn("failed to map the Watchdog interrupt.\n");
>> + nr_res--;
>> + }
>> +
>> + /*
>> + * Add a platform device named "sbsa-gwdt" to match the platform driver.
>> + * "sbsa-gwdt": SBSA(Server Base System Architecture) Generic Watchdog
>> + * The platform driver (like drivers/watchdog/sbsa_gwdt.c)can get device
+ * The platform driver can get device info below by matching this name.
>
> Nit: I would not hardcode drivers paths in comments.
OK, deleted it
>
>> + * info below by matching this name.
>> + */
>> + pdev = platform_device_register_simple("sbsa-gwdt", index, res, nr_res);
>> + if (IS_ERR(pdev)) {
>> + acpi_unregister_gsi(wd->timer_interrupt);
>> + return PTR_ERR(pdev);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int __init gtdt_sbsa_gwdt_init(void)
>> +{
>> + int ret, i = 0;
>> + void *platform_timer;
>> + struct acpi_table_header *table;
>> +
>> + if (acpi_disabled)
>> + return 0;
>> +
>> + if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_GTDT, 0, &table)))
>> + return -EINVAL;
>> +
>> + ret = acpi_gtdt_init(table, NULL);
>> + if (ret)
>> + return ret;
>
> Ok, I missed previous versions reviews so I miss the background
> here and I apologise.
>
> I do not understand why you call acpi_gtdt_init() again (or better,
> why acpi_gtdt_init() does not return straight away) here if the
> stashed pointer in acpi_gtdt_desc is already set. I am not a big fan
> of the for_each_platform_timer macro either, here you can just read
> the ACPI_SIG_GTDT and parse its entries, I see no point in calling
> acpi_gtdt_init() again, there is nothing to stash for later probing,
> is there ?
>
> Or it is just to reuse common parsing code ? Regardless, if the
> acpi_gtdt_desc struct is already initialized acpi_gtdt_init() should
> just return or I am missing the point.
>
> I would like clarifications on the acpi_gtdt_init() call above please
> (plus GSI unmap fix), apart from that:
>
> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Actually, after the discussion in the 7th patch, I have tried to do
this, see if that can make the code simpler.
Then I found out that
(1) If we would like to reuse "for_each_platform_timer" (and
"next_platform_timer") to scan the GTDT for SBSA watchdog, we have to
re-initialize acpi_gtdt_desc, too.
So we still need to explain that why we have to re-initialize
acpi_gtdt_desc.
(2) If we totally ignore acpi_gtdt_desc, and acpi_gtdt_init(), we
can't re-use "for_each_platform_timer" (and "next_platform_timer") .
we have to re-write the loop of scanning the GTDT for SBSA watchdog,
So may I suggest that maybe we can keep using acpi_gtdt_init, only add
the comment to explain why we have to re-initialize
acpi_gtdt_desc(Just what you have said in the 7th patch. Thanks for
your correction for the comment, I will use that ), this way maybe can
make the code simpler.
But If you still concern about this, we still can discuss this at -rc1.
Great thanks for your help! :-)
>
>> + for_each_platform_timer(platform_timer) {
>> + if (is_non_secure_watchdog(platform_timer)) {
>> + ret = gtdt_import_sbsa_gwdt(platform_timer, i);
>> + if (ret)
>> + break;
>> + i++;
>> + }
>> + }
>> +
>> + if (i)
>> + pr_info("found %d SBSA generic Watchdog(s).\n", i);
>> +
>> + return ret;
>> +}
>> +
>> +device_initcall(gtdt_sbsa_gwdt_init);
>> --
>> 2.9.3
>>
--
Best regards,
Fu Wei
Software Engineer
Red Hat
next prev parent reply other threads:[~2017-03-31 8:10 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
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 [this message]
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='CADyBb7uDyjhP2Pjtzh6pTN-0hkokB=KevY7V76G9qZOF-9-zoQ@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).