Hi Lorenzo,

On 28 March 2017 at 23:41, Lorenzo Pieralisi [off-list ref] wrote:

On Wed, Mar 22, 2017 at 12:31:22AM +0800, fu.wei at linaro.org wrote:

quoted

From: Fu Wei <redacted>

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 <redacted>
Signed-off-by: Hanjun Guo <redacted>
Tested-by: Xiongfeng Wang <redacted>
---
 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);

quoted

+             return -EINVAL;

You should unmap the gsi here.

yes, you are right, fixed it.

quoted

+     }
+
+     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

quoted

+      * 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 <redacted>

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! :-)

quoted

+     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