Re: [Minios-devel] [UNIKRAFT PATCHv3 4/7] plat/common: Share arch_timer fdt node among functions

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

Hi Santiago

> -----Original Message-----
> From: Santiago Pagani <Santiago.Pagani@xxxxxxxxx>
> Sent: 2019年9月10日 15:11
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>;
> Sharan Santhanam <Sharan.Santhanam@xxxxxxxxx>
> Cc: Julien Grall <Julien.Grall@xxxxxxx>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@xxxxxxx>; Wei Chen (Arm Technology China)
> <Wei.Chen@xxxxxxx>; Jianyong Wu (Arm Technology China)
> <Jianyong.Wu@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv3 4/7] plat/common: Share arch_timer fdt
> node among functions
>
> Hi all,
>
> Thanks for the patch. Please find my comments inline.
>
> On 30.07.19, 16:27, "Jia He" <justin.he@xxxxxxx> wrote:
>
>     Several function will use the arch_timer fdt node to get information
>     from device tree. We find it once, and share it among functions. this
>     will avoid find arch_timer fdt everywhere.
>
>     Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
>     Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
>     Signed-off-by: Jia He <justin.he@xxxxxxx>
>     ---
>      plat/common/arm/time.c | 53 +++++++++++++++++++++--------------------
> -
>      1 file changed, 27 insertions(+), 26 deletions(-)
>
>     diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c
>     index aea379c..107fdf0 100644
>     --- a/plat/common/arm/time.c
>     +++ b/plat/common/arm/time.c
>     @@ -33,6 +33,7 @@
>       */
>      #include <stdlib.h>
>      #include <libfdt.h>
>     +#include <ofw/fdt.h>
>      #include <uk/assert.h>
>      #include <uk/plat/time.h>
>      #include <uk/plat/irq.h>
>     @@ -44,6 +45,12 @@
>       * portable way to handover the DTB entry point to common platform
> code */
>      #include <kvm/config.h>
>
>     +static const char * const arch_timer_list[] = {
>     + "arm,armv8-timer",
>     + "arm,armv7-timer",
>     + NULL
>     +};
>     +
>      static uint64_t boot_ticks;
>      static uint32_t counter_freq;
>
>     @@ -126,32 +133,18 @@ static void calculate_mult_shift(uint32_t
> *mult, uint8_t *shift,
>       *shift = sft;
>      }
>
>     -/*
>     - * On a few platforms the frequency is not configured correctly
>     - * by the firmware. A property in the DT (clock-frequency) has
>     - * been introduced to workaround those firmware. So, we will try
>     - * to get clock-frequency from DT first, if failed we will read
>     - * the register directly.
>     - */
>     -static uint32_t get_counter_frequency(void)
>     +static uint32_t generic_timer_get_frequency(int fdt_timer)
>      {
>     - int fdt_archtimer, len;
>     + int len;
>       const uint64_t *fdt_freq;
>
>     - /* Try to find arm,armv8-timer first */
>     - fdt_archtimer =
> fdt_node_offset_by_compatible(_libkvmplat_cfg.dtb,
>     -                                         -1, "arm,armv8-timer");
>     - /* If failed, try to find arm,armv7-timer */
>     - if (fdt_archtimer < 0)
>     -         fdt_archtimer = fdt_node_offset_by_compatible(
>     -                                                 _libkvmplat_cfg.dtb,
>     -                                                 -1, "arm,armv7-
> timer");
>     - /* DT doesn't provide arch timer information */
>     - if (fdt_archtimer < 0)
>     -         goto endnofreq;
>     -
>     + /*
>     +  * On a few platforms the frequency is not configured correctly
>     +  * by the firmware. A property in the DT (clock-frequency) has
>     +  * been introduced to workaround those firmware.
>     +  */
>       fdt_freq = fdt_getprop(_libkvmplat_cfg.dtb,
>     -                 fdt_archtimer, "clock-frequency", &len);
>     +                 fdt_timer, "clock-frequency", &len);
>       if (!fdt_freq || (len <= 0)) {
>               uk_pr_info("No clock-frequency found, reading from
> register directly.\n");
>               goto endnofreq;
> COMMENT: No need to use goto in this case. Simply move the comment
> and return statement in the goto here and then remove the endnogreq
> label.

Yes, thanks

--
Cheers,
Justin (Jia He)


>     @@ -160,6 +153,7 @@ static uint32_t get_counter_frequency(void)
>       return fdt32_to_cpu(fdt_freq[0]);
>
>      endnofreq:
>     + /* No workaround, get from register directly */
>       return SYSREG_READ32(cntfrq_el0);
>      }
>
>     @@ -205,9 +199,10 @@ static uint64_t generic_timer_epochoffset(void)
>       return 0;
>      }
>
>     -static int generic_timer_init(void)
>     +static int generic_timer_init(int fdt_timer)
>      {
>     - counter_freq = get_counter_frequency();
>     + /* Get counter frequency from DTB or register */
>     + counter_freq = generic_timer_get_frequency(fdt_timer);
>
>       /*
>        * Calculate the shift factor and scaling multiplier for
>     @@ -268,7 +263,7 @@ static int timer_handler(void *arg __unused)
>      /* must be called before interrupts are enabled */
>      void ukplat_time_init(void)
>      {
>     - int rc;
>     + int rc, fdt_timer;
>
>       /*
>        * Monotonic time begins at boot_ticks (first read of counter
>     @@ -276,11 +271,17 @@ void ukplat_time_init(void)
>        */
>       boot_ticks = generic_timer_get_ticks();
>
>     + /* Currently, we only support 1 timer per system */
>     + fdt_timer =
> fdt_node_offset_by_compatible_list(_libkvmplat_cfg.dtb,
>     +                         -1, arch_timer_list);
>     + if (fdt_timer < 0)
>     +         UK_CRASH("Could not find arch timer!\n");
>     +
>       rc = ukplat_irq_register(0, timer_handler, NULL);
>       if (rc < 0)
>               UK_CRASH("Failed to register timer interrupt handler\n");
>
>     - rc = generic_timer_init();
>     + rc = generic_timer_init(fdt_timer);
>       if (rc < 0)
>               UK_CRASH("Failed to initialize platform time\n");
>      }
>     --
>     2.17.1
>
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel