[v7,7/9] acpi/arm64: Add memory-mapped timer support in GTDT driver

Commit Message

[e-mail] July 13, 2016, 5:53 p.m. UTC

From: Fu Wei <[e-mail]>

This driver adds support for parsing memory-mapped timer in GTDT:
provide a kernel APIs to parse GT Block Structure in GTDT,
export all the info by filling the struct which provided
by parameter(pointer of the struct).

By this driver, we can add ACPI support for memory-mapped timer in
arm_arch_timer drivers, and separate the ACPI GTDT knowledge from it.

Signed-off-by: Fu Wei <[e-mail]>
Signed-off-by: Hanjun Guo <[e-mail]>
---
 drivers/acpi/arm64/acpi_gtdt.c       | 90 ++++++++++++++++++++++++++++++++++++
 include/clocksource/arm_arch_timer.h | 15 ++++++
 include/linux/acpi.h                 |  1 +
 3 files changed, 106 insertions(+)

Comments

Lorenzo Pieralisi July 14, 2016, 1:42 p.m. UTC | #1

On Thu, Jul 14, 2016 at 01:53:20AM +0800, [e-mail] wrote:
> From: Fu Wei <[e-mail]>
> 
> This driver adds support for parsing memory-mapped timer in GTDT:
> provide a kernel APIs to parse GT Block Structure in GTDT,
> export all the info by filling the struct which provided
> by parameter(pointer of the struct).
> 
> By this driver, we can add ACPI support for memory-mapped timer in
> arm_arch_timer drivers, and separate the ACPI GTDT knowledge from it.
> 
> Signed-off-by: Fu Wei <[e-mail]>
> Signed-off-by: Hanjun Guo <[e-mail]>
> ---
>  drivers/acpi/arm64/acpi_gtdt.c       | 90 ++++++++++++++++++++++++++++++++++++
>  include/clocksource/arm_arch_timer.h | 15 ++++++
>  include/linux/acpi.h                 |  1 +
>  3 files changed, 106 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
> index 9ee977d..ff62953 100644
> --- a/drivers/acpi/arm64/acpi_gtdt.c
> +++ b/drivers/acpi/arm64/acpi_gtdt.c
> @@ -168,3 +168,93 @@ int __init gtdt_arch_timer_init(struct acpi_table_header *table)
>  
>  	return -EINVAL;
>  }
> +
> +/*
> + * Helper function for getting the pointer of a timer frame in GT block.
> + */
> +static void __init *gtdt_gt_timer_frame(struct acpi_gtdt_timer_block *gt_block,
> +					int index)
> +{
> +	void *timer_frame = (void *)gt_block + gt_block->timer_offset +
> +			    sizeof(struct acpi_gtdt_timer_entry) * index;
> +
> +	if (timer_frame <= (void *)gt_block + gt_block->header.length -
> +			   sizeof(struct acpi_gtdt_timer_entry))
> +		return timer_frame;

Nit: gt_block is an array, right ? I think it would be much simpler
if you treat is as such, so that indexing into it would be done
automatically by the compiler. Actually, I do not even think you
would need this function at all if you treat gt_block as an array,
the length check could be done in gtdt_parse_gt_block() straight
away.

> +
> +	return NULL;
> +}
> +
> +static int __init gtdt_parse_gt_block(void *platform_timer, int index,
> +				      void *data)
> +{
> +	struct acpi_gtdt_timer_block *block;
> +	struct acpi_gtdt_timer_entry *frame;
> +	struct gt_block_data *block_data;
> +	int i, j;
> +
> +	if (!platform_timer || !data)
> +		return -EINVAL;
> +
> +	block = platform_timer;
> +	block_data = data + sizeof(struct gt_block_data) * index;

Nit: See above, data is a struct gt_block_data[] right ? These void
pointers parameters are not really great, the caller context
knows what they are and it can pass them as pointer to typed
array elements anyway unless I am missing something.

> +	if (!block->block_address || !block->timer_count) {
> +		pr_err(FW_BUG "invalid GT Block data.\n");
> +		return -EINVAL;
> +	}
> +	block_data->cntctlbase_phy = (phys_addr_t)block->block_address;
> +	block_data->timer_count = block->timer_count;
> +
> +	/*
> +	 * Get the GT timer Frame data for every GT Block Timer
> +	 */
> +	for (i = 0, j = 0; i < block->timer_count; i++) {

What's j needed for (ie can't you use just i instead ?) ?

> +		frame = gtdt_gt_timer_frame(block, i);
> +		if (!frame || !frame->base_address || !frame->timer_interrupt) {
> +			pr_err(FW_BUG "invalid GT Block Timer data.\n");
> +			return -EINVAL;
> +		}
> +		block_data->timer[j].frame_nr = frame->frame_number;
> +		block_data->timer[j].cntbase_phy = frame->base_address;
> +		block_data->timer[j].irq = map_generic_timer_interrupt(
> +						   frame->timer_interrupt,
> +						   frame->timer_flags);
> +		if (frame->virtual_timer_interrupt)
> +			block_data->timer[j].virt_irq =
> +				map_generic_timer_interrupt(
> +					frame->virtual_timer_interrupt,
> +					frame->virtual_timer_flags);
> +		j++;
> +	}
> +
> +	if (j)
> +		return 0;
> +
> +	block_data->cntctlbase_phy = (phys_addr_t)NULL;

This is wrong. NULL is not meant to be used as a physical address,
you must not do that. Is not zeroeing timer_count enough ? I have
to understand why you need this because casting NULL into it is
not safe, at all.

> +	block_data->timer_count = 0;
> +
> +	return -EINVAL;
> +}
> +
> +/*
> + * Get the GT block info for memory-mapped timer from GTDT table.
> + * Please make sure we have called gtdt_arch_timer_init, because it helps to
> + * init the global variables.

It is a helper function that you call once at boot, you easily
determine when it is called, it is not meant to be used in different
contexts from different subsystems; I think that this comment
is not clear so either you make it clearer or you remove it.

> + */
> +int __init gtdt_arch_timer_mem_init(struct gt_block_data *data)
> +{
> +	void *platform_timer;
> +	int index = 0;
> +
> +	for_each_platform_timer(platform_timer) {
> +		if (is_timer_block(platform_timer) &&
> +		    !gtdt_parse_gt_block(platform_timer, index, data))

Passing around these opaque void* is a tad ugly, see my comments
above about this.

Apart from the NULL pointer assignment, everything else is just code
clean-up.

Thanks,
Lorenzo

> +			index++;
> +	}
> +
> +	if (index)
> +		pr_info("found %d memory-mapped timer block.\n", index);
> +
> +	return index;
> +}
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 16dcd10..ece6b3b 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -56,6 +56,8 @@ enum spi_nr {
>  #define ARCH_TIMER_MEM_PHYS_ACCESS	2
>  #define ARCH_TIMER_MEM_VIRT_ACCESS	3
>  
> +#define ARCH_TIMER_MEM_MAX_FRAME	8
> +
>  #define ARCH_TIMER_USR_PCT_ACCESS_EN	(1 << 0) /* physical counter */
>  #define ARCH_TIMER_USR_VCT_ACCESS_EN	(1 << 1) /* virtual counter */
>  #define ARCH_TIMER_VIRT_EVT_EN		(1 << 2)
> @@ -71,6 +73,19 @@ struct arch_timer_kvm_info {
>  	int virtual_irq;
>  };
>  
> +struct gt_timer_data {
> +	int frame_nr;
> +	phys_addr_t cntbase_phy;
> +	int irq;
> +	int virt_irq;
> +};
> +
> +struct gt_block_data {
> +	phys_addr_t cntctlbase_phy;
> +	int timer_count;
> +	struct gt_timer_data timer[ARCH_TIMER_MEM_MAX_FRAME];
> +};
> +
>  #ifdef CONFIG_ARM_ARCH_TIMER
>  
>  extern u32 arch_timer_get_rate(void);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 8439579..b1cacbc 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -536,6 +536,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *);
>  int __init gtdt_arch_timer_init(struct acpi_table_header *table);
>  int __init acpi_gtdt_map_ppi(int type);
>  int __init acpi_gtdt_c3stop(void);
> +int __init gtdt_arch_timer_mem_init(struct gt_block_data *data);
>  #endif
>  
>  #else	/* !CONFIG_ACPI */
> -- 
> 2.5.5
>

[e-mail] July 19, 2016, 6:28 p.m. UTC | #2

Hi Lorenzo,

I have updated my patchset following all your comments.
I have just posted v8 patchset, please check.

Great thanks for your comments, they are very helpful :-)

On 14 July 2016 at 21:42, Lorenzo Pieralisi <[e-mail]> wrote:
> On Thu, Jul 14, 2016 at 01:53:20AM +0800, [e-mail] wrote:
>> From: Fu Wei <[e-mail]>
>>
>> This driver adds support for parsing memory-mapped timer in GTDT:
>> provide a kernel APIs to parse GT Block Structure in GTDT,
>> export all the info by filling the struct which provided
>> by parameter(pointer of the struct).
>>
>> By this driver, we can add ACPI support for memory-mapped timer in
>> arm_arch_timer drivers, and separate the ACPI GTDT knowledge from it.
>>
>> Signed-off-by: Fu Wei <[e-mail]>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> ---
>>  drivers/acpi/arm64/acpi_gtdt.c       | 90 ++++++++++++++++++++++++++++++++++++
>>  include/clocksource/arm_arch_timer.h | 15 ++++++
>>  include/linux/acpi.h                 |  1 +
>>  3 files changed, 106 insertions(+)
>>
>> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
>> index 9ee977d..ff62953 100644
>> --- a/drivers/acpi/arm64/acpi_gtdt.c
>> +++ b/drivers/acpi/arm64/acpi_gtdt.c
>> @@ -168,3 +168,93 @@ int __init gtdt_arch_timer_init(struct acpi_table_header *table)
>>
>>       return -EINVAL;
>>  }
>> +
>> +/*
>> + * Helper function for getting the pointer of a timer frame in GT block.
>> + */
>> +static void __init *gtdt_gt_timer_frame(struct acpi_gtdt_timer_block *gt_block,
>> +                                     int index)
>> +{
>> +     void *timer_frame = (void *)gt_block + gt_block->timer_offset +
>> +                         sizeof(struct acpi_gtdt_timer_entry) * index;
>> +
>> +     if (timer_frame <= (void *)gt_block + gt_block->header.length -
>> +                        sizeof(struct acpi_gtdt_timer_entry))
>> +             return timer_frame;
>
> Nit: gt_block is an array, right ? I think it would be much simpler
> if you treat is as such, so that indexing into it would be done
> automatically by the compiler. Actually, I do not even think you
> would need this function at all if you treat gt_block as an array,
> the length check could be done in gtdt_parse_gt_block() straight
> away.
>
>> +
>> +     return NULL;
>> +}
>> +
>> +static int __init gtdt_parse_gt_block(void *platform_timer, int index,
>> +                                   void *data)
>> +{
>> +     struct acpi_gtdt_timer_block *block;
>> +     struct acpi_gtdt_timer_entry *frame;
>> +     struct gt_block_data *block_data;
>> +     int i, j;
>> +
>> +     if (!platform_timer || !data)
>> +             return -EINVAL;
>> +
>> +     block = platform_timer;
>> +     block_data = data + sizeof(struct gt_block_data) * index;
>
> Nit: See above, data is a struct gt_block_data[] right ? These void
> pointers parameters are not really great, the caller context
> knows what they are and it can pass them as pointer to typed
> array elements anyway unless I am missing something.
>
>> +     if (!block->block_address || !block->timer_count) {
>> +             pr_err(FW_BUG "invalid GT Block data.\n");
>> +             return -EINVAL;
>> +     }
>> +     block_data->cntctlbase_phy = (phys_addr_t)block->block_address;
>> +     block_data->timer_count = block->timer_count;
>> +
>> +     /*
>> +      * Get the GT timer Frame data for every GT Block Timer
>> +      */
>> +     for (i = 0, j = 0; i < block->timer_count; i++) {
>
> What's j needed for (ie can't you use just i instead ?) ?
>
>> +             frame = gtdt_gt_timer_frame(block, i);
>> +             if (!frame || !frame->base_address || !frame->timer_interrupt) {
>> +                     pr_err(FW_BUG "invalid GT Block Timer data.\n");
>> +                     return -EINVAL;
>> +             }
>> +             block_data->timer[j].frame_nr = frame->frame_number;
>> +             block_data->timer[j].cntbase_phy = frame->base_address;
>> +             block_data->timer[j].irq = map_generic_timer_interrupt(
>> +                                                frame->timer_interrupt,
>> +                                                frame->timer_flags);
>> +             if (frame->virtual_timer_interrupt)
>> +                     block_data->timer[j].virt_irq =
>> +                             map_generic_timer_interrupt(
>> +                                     frame->virtual_timer_interrupt,
>> +                                     frame->virtual_timer_flags);
>> +             j++;
>> +     }
>> +
>> +     if (j)
>> +             return 0;
>> +
>> +     block_data->cntctlbase_phy = (phys_addr_t)NULL;
>
> This is wrong. NULL is not meant to be used as a physical address,
> you must not do that. Is not zeroeing timer_count enough ? I have
> to understand why you need this because casting NULL into it is
> not safe, at all.
>
>> +     block_data->timer_count = 0;
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +/*
>> + * Get the GT block info for memory-mapped timer from GTDT table.
>> + * Please make sure we have called gtdt_arch_timer_init, because it helps to
>> + * init the global variables.
>
> It is a helper function that you call once at boot, you easily
> determine when it is called, it is not meant to be used in different
> contexts from different subsystems; I think that this comment
> is not clear so either you make it clearer or you remove it.
>
>> + */
>> +int __init gtdt_arch_timer_mem_init(struct gt_block_data *data)
>> +{
>> +     void *platform_timer;
>> +     int index = 0;
>> +
>> +     for_each_platform_timer(platform_timer) {
>> +             if (is_timer_block(platform_timer) &&
>> +                 !gtdt_parse_gt_block(platform_timer, index, data))
>
> Passing around these opaque void* is a tad ugly, see my comments
> above about this.
>
> Apart from the NULL pointer assignment, everything else is just code
> clean-up.
>
> Thanks,
> Lorenzo
>
>> +                     index++;
>> +     }
>> +
>> +     if (index)
>> +             pr_info("found %d memory-mapped timer block.\n", index);
>> +
>> +     return index;
>> +}
>> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
>> index 16dcd10..ece6b3b 100644
>> --- a/include/clocksource/arm_arch_timer.h
>> +++ b/include/clocksource/arm_arch_timer.h
>> @@ -56,6 +56,8 @@ enum spi_nr {
>>  #define ARCH_TIMER_MEM_PHYS_ACCESS   2
>>  #define ARCH_TIMER_MEM_VIRT_ACCESS   3
>>
>> +#define ARCH_TIMER_MEM_MAX_FRAME     8
>> +
>>  #define ARCH_TIMER_USR_PCT_ACCESS_EN (1 << 0) /* physical counter */
>>  #define ARCH_TIMER_USR_VCT_ACCESS_EN (1 << 1) /* virtual counter */
>>  #define ARCH_TIMER_VIRT_EVT_EN               (1 << 2)
>> @@ -71,6 +73,19 @@ struct arch_timer_kvm_info {
>>       int virtual_irq;
>>  };
>>
>> +struct gt_timer_data {
>> +     int frame_nr;
>> +     phys_addr_t cntbase_phy;
>> +     int irq;
>> +     int virt_irq;
>> +};
>> +
>> +struct gt_block_data {
>> +     phys_addr_t cntctlbase_phy;
>> +     int timer_count;
>> +     struct gt_timer_data timer[ARCH_TIMER_MEM_MAX_FRAME];
>> +};
>> +
>>  #ifdef CONFIG_ARM_ARCH_TIMER
>>
>>  extern u32 arch_timer_get_rate(void);
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 8439579..b1cacbc 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -536,6 +536,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *);
>>  int __init gtdt_arch_timer_init(struct acpi_table_header *table);
>>  int __init acpi_gtdt_map_ppi(int type);
>>  int __init acpi_gtdt_c3stop(void);
>> +int __init gtdt_arch_timer_mem_init(struct gt_block_data *data);
>>  #endif
>>
>>  #else        /* !CONFIG_ACPI */
>> --
>> 2.5.5
>>

Patch

diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
index 9ee977d..ff62953 100644
--- a/drivers/acpi/arm64/acpi_gtdt.c
+++ b/drivers/acpi/arm64/acpi_gtdt.c
@@ -168,3 +168,93 @@  int __init gtdt_arch_timer_init(struct acpi_table_header *table)
 
 	return -EINVAL;
 }
+
+/*
+ * Helper function for getting the pointer of a timer frame in GT block.
+ */
+static void __init *gtdt_gt_timer_frame(struct acpi_gtdt_timer_block *gt_block,
+					int index)
+{
+	void *timer_frame = (void *)gt_block + gt_block->timer_offset +
+			    sizeof(struct acpi_gtdt_timer_entry) * index;
+
+	if (timer_frame <= (void *)gt_block + gt_block->header.length -
+			   sizeof(struct acpi_gtdt_timer_entry))
+		return timer_frame;
+
+	return NULL;
+}
+
+static int __init gtdt_parse_gt_block(void *platform_timer, int index,
+				      void *data)
+{
+	struct acpi_gtdt_timer_block *block;
+	struct acpi_gtdt_timer_entry *frame;
+	struct gt_block_data *block_data;
+	int i, j;
+
+	if (!platform_timer || !data)
+		return -EINVAL;
+
+	block = platform_timer;
+	block_data = data + sizeof(struct gt_block_data) * index;
+
+	if (!block->block_address || !block->timer_count) {
+		pr_err(FW_BUG "invalid GT Block data.\n");
+		return -EINVAL;
+	}
+	block_data->cntctlbase_phy = (phys_addr_t)block->block_address;
+	block_data->timer_count = block->timer_count;
+
+	/*
+	 * Get the GT timer Frame data for every GT Block Timer
+	 */
+	for (i = 0, j = 0; i < block->timer_count; i++) {
+		frame = gtdt_gt_timer_frame(block, i);
+		if (!frame || !frame->base_address || !frame->timer_interrupt) {
+			pr_err(FW_BUG "invalid GT Block Timer data.\n");
+			return -EINVAL;
+		}
+		block_data->timer[j].frame_nr = frame->frame_number;
+		block_data->timer[j].cntbase_phy = frame->base_address;
+		block_data->timer[j].irq = map_generic_timer_interrupt(
+						   frame->timer_interrupt,
+						   frame->timer_flags);
+		if (frame->virtual_timer_interrupt)
+			block_data->timer[j].virt_irq =
+				map_generic_timer_interrupt(
+					frame->virtual_timer_interrupt,
+					frame->virtual_timer_flags);
+		j++;
+	}
+
+	if (j)
+		return 0;
+
+	block_data->cntctlbase_phy = (phys_addr_t)NULL;
+	block_data->timer_count = 0;
+
+	return -EINVAL;
+}
+
+/*
+ * Get the GT block info for memory-mapped timer from GTDT table.
+ * Please make sure we have called gtdt_arch_timer_init, because it helps to
+ * init the global variables.
+ */
+int __init gtdt_arch_timer_mem_init(struct gt_block_data *data)
+{
+	void *platform_timer;
+	int index = 0;
+
+	for_each_platform_timer(platform_timer) {
+		if (is_timer_block(platform_timer) &&
+		    !gtdt_parse_gt_block(platform_timer, index, data))
+			index++;
+	}
+
+	if (index)
+		pr_info("found %d memory-mapped timer block.\n", index);
+
+	return index;
+}
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 16dcd10..ece6b3b 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -56,6 +56,8 @@  enum spi_nr {
 #define ARCH_TIMER_MEM_PHYS_ACCESS	2
 #define ARCH_TIMER_MEM_VIRT_ACCESS	3
 
+#define ARCH_TIMER_MEM_MAX_FRAME	8
+
 #define ARCH_TIMER_USR_PCT_ACCESS_EN	(1 << 0) /* physical counter */
 #define ARCH_TIMER_USR_VCT_ACCESS_EN	(1 << 1) /* virtual counter */
 #define ARCH_TIMER_VIRT_EVT_EN		(1 << 2)
@@ -71,6 +73,19 @@  struct arch_timer_kvm_info {
 	int virtual_irq;
 };
 
+struct gt_timer_data {
+	int frame_nr;
+	phys_addr_t cntbase_phy;
+	int irq;
+	int virt_irq;
+};
+
+struct gt_block_data {
+	phys_addr_t cntctlbase_phy;
+	int timer_count;
+	struct gt_timer_data timer[ARCH_TIMER_MEM_MAX_FRAME];
+};
+
 #ifdef CONFIG_ARM_ARCH_TIMER
 
 extern u32 arch_timer_get_rate(void);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 8439579..b1cacbc 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -536,6 +536,7 @@  struct platform_device *acpi_create_platform_device(struct acpi_device *);
 int __init gtdt_arch_timer_init(struct acpi_table_header *table);
 int __init acpi_gtdt_map_ppi(int type);
 int __init acpi_gtdt_c3stop(void);
+int __init gtdt_arch_timer_mem_init(struct gt_block_data *data);
 #endif
 
 #else	/* !CONFIG_ACPI */