Re: [Linaro-acpi] [PATCH v3 7/7] ACPI: import watchdog info of GTDT into platform device

Hi Hanjun,

Thanks for discussing this with me about this , I have fixed some problem base on our discuss.

please check v4 patch :-)

On 13 May 2015 at 12:55, Hanjun Guo hanjun.guo@linaro.org wrote:

On 2015年05月12日 18:56, fu.wei@linaro.org wrote:

...

From: Fu Wei fu.wei@linaro.org

Import SBSA Generic watchdog info in GTDT into platform device. That make ARM SBSA watchdog driver can work on the platform which boot with ACPI.

Signed-off-by: Fu Wei fu.wei@linaro.org

arch/arm64/kernel/acpi.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..90422b5 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -23,6 +23,7 @@ #include <linux/irqdomain.h> #include <linux/memblock.h> #include <linux/of_fdt.h> +#include <linux/platform_device.h> #include <linux/smp.h>

#include <asm/cputype.h> @@ -343,3 +344,133 @@ void __init acpi_gic_init(void)

    early_acpi_os_unmap_memory((char *)table, tbl_size);

}

+static int __init acpi_gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd,

•                                        int index)
  

+{

•   struct platform_device *pdev;
  

•   struct resource *res;
  

•   u32 gsi, flags;
  

•   int trigger, polarity;
  

•   resource_size_t rf_base_phy, cf_base_phy;
  

•   int err;
  

•   /* Get SBSA Generic Watchdog info
  

•    * from a watchdog type subtable in GTDT
  

•    */
  

•   rf_base_phy = (resource_size_t)wd->refresh_frame_address;
  

•   cf_base_phy = (resource_size_t)wd->control_frame_address;
  

•   gsi = wd->timer_interrupt;
  

•   flags = wd->timer_flags;
  

•   trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
  

•                   : ACPI_LEVEL_SENSITIVE;
  

•   polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ?
  

ACPI_ACTIVE_LOW

•                   : ACPI_ACTIVE_HIGH;
  

I think we need to reuse the map_generic_timer_interrupt() in arm_arch_timer.c, but we can do that on top of this patch set.

...

•   pr_info("GTDT: found a sbsa-gwdt device @0x%llx/0x%llx gsi:%u
  

flags:0x%x\n",

•           rf_base_phy, cf_base_phy, gsi, flags);
  

...

...

•   if (!(rf_base_phy && cf_base_phy && gsi)) {
  

•           pr_err("GTDT: failed geting the device info.\n");
  

•           return -EINVAL;
  

•   }
  

Should the pr_info() put it here?

...

•   pdev = platform_device_alloc("sbsa-gwdt", index);
  

•   if (!pdev)
  

•           return -ENOMEM;
  

•   res = kcalloc(3, sizeof(*res), GFP_KERNEL);
  

•   if (!res) {
  

•           err = -ENOMEM;
  

•           goto err_free_device;
  

•   }
  

•   res[0].start = rf_base_phy;
  

•   res[0].end = rf_base_phy + SZ_4K - 1;
  

•   res[0].name = "refresh";
  

•   res[0].flags = IORESOURCE_MEM;
  

•   res[1].start = cf_base_phy;
  

•   res[1].end = cf_base_phy + SZ_4K - 1;
  

•   res[1].name = "control";
  

•   res[1].flags = IORESOURCE_MEM;
  

•   res[2].start = acpi_register_gsi(NULL, gsi, trigger, polarity);
  

•   res[2].end = res[2].start;
  

•   res[2].name = "ws0";
  

•   res[2].flags = IORESOURCE_IRQ;
  

•   err = platform_device_add_resources(pdev, res, 3);
  

•   if (err)
  

•           goto err_free_res;
  

•   err = platform_device_add(pdev);
  

•   if (err)
  

•           goto err_free_res;
  

•   return 0;
  

+err_free_res:

•   kfree(res);
  

+err_free_device:

•   platform_device_put(pdev);
  

•   return err;
  

+}

+/* Initialize SBSA generic Watchdog platform device info from GTDT */ +static int __init acpi_gtdt_sbsa_gwdt_init(struct acpi_table_header *table) +{

•   struct acpi_table_gtdt *gtdt;
  

•   struct acpi_gtdt_header *header;
  

•   void *gtdt_subtable;
  

•   int i, gwdt_index;
  

•   int ret = 0;
  

•   if (table->revision < 2) {
  

•           pr_info("GTDT: Revision:%d doesn't support Platform
  

Timers.\n",

•                   table->revision);
  

•           return 0;
  

•   }
  

No, please don't count on the table revision, lots of BIOS vendors just ignore that field, so we should count on real content in this table, which means we should scan for the table to find if there are information we needed.

...

•   gtdt = container_of(table, struct acpi_table_gtdt, header);
  

•   if (!gtdt->platform_timer_count) {
  

•           pr_info("GTDT: No Platform Timer structures.\n");
  

•           return 0;
  

•   }
  

•   gtdt_subtable = (void *)gtdt + gtdt->platform_timer_offset;
  

I like this part, it removes the introduction of new table handler for GTDT.

...

•   for (i = 0, gwdt_index = 0; i < gtdt->platform_timer_count; i++) {
  

•           if (gtdt_subtable > (void *)table + table->length) {
  

•                   pr_err("GTDT: subtable pointer overflows, bad
  

table\n");

•                   ret = -EINVAL;
  

•                   break;
  

•           }
  

•           header = (struct acpi_gtdt_header *)gtdt_subtable;
  

•           if (header->type == ACPI_GTDT_TYPE_WATCHDOG) {
  

•                   ret = acpi_gtdt_import_sbsa_gwdt(
  

•                           (struct acpi_gtdt_watchdog
  

*)gtdt_subtable,

•                           gwdt_index);
  

Can we refactor this a little bit to make this function can be reused for memory-mapped timer init?

Thanks Hanjun