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

[Santiago Pagani <Santiago.Pagani@xxxxxxxxx>]

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.
    @@ -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
    
    
    

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel