|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH 1/1] plat/common: UKTIME supported in the Raspberry Pi 3.
Hi Lupu,
I can split the patch into multiple patches, sure.
In regards to the second question, yes, the Raspberry Pi 3 is a new platform (I
have not sent my patches for that yet). My only concern with moving this timer
code to a Raspi specific folder is that only a couple of things are different
in the Pi, while the rest of the code still remains common. The best choice
would then probably be to split up the time.c file in two, so all current
platforms can use the current version, while I only include one of the files in
the Pi 3 and generate a new one for the 4 functions that need their own
versions.
Best,
Santiago
On 19.10.19, 07:37, "Costin Lupu" <costin.lup@xxxxxxxxx> wrote:
Hi Santiago,
It looks to me that this patch should be split into more patches (e.g.
add ukarch_{fls,flsl} for arm64, add timer support for new platform, etc).
Btw, isn't raspi a new platform actually? This would mean you should put
the platform-specific code under a new raspi/ subdir in plat/, along
with kvm/, xen/ and linuxu/.
Please see inline other comments.
On 10/18/19 3:17 PM, Santiago Pagani wrote:
> Added and guarded Raspberry Pi 3's SoC specific
> things related to the ARM generic timer in order
> to support UKTIME.
>
> Signed-off-by: Santiago Pagani <santiago.pagani@xxxxxxxxx>
> ---
> arch/arm/arm64/include/uk/asm/atomic.h | 53 ++++++++++
> lib/nolibc/include/unistd.h | 2 +
> plat/common/arm/time.c | 134 ++++++++++++++++---------
> 3 files changed, 143 insertions(+), 46 deletions(-)
>
> diff --git a/arch/arm/arm64/include/uk/asm/atomic.h
b/arch/arm/arm64/include/uk/asm/atomic.h
> index 7ee66677..98b5ed2b 100644
> --- a/arch/arm/arm64/include/uk/asm/atomic.h
> +++ b/arch/arm/arm64/include/uk/asm/atomic.h
> @@ -6,6 +6,7 @@
> * Authors: Karim Allah Ahmed <karim.allah.ahmed@xxxxxxxxx>
> * Thomas Leonard <talex5@xxxxxxxxx>
> * Wei Chen <Wei.Chen@xxxxxxx>
> + * Santiago Pagani <santiago.pagani@xxxxxxxxx
> *
> * Copyright (c) 2014 Karim Allah Ahmed
> * Copyright (c) 2014 Thomas Leonard
> @@ -37,6 +38,32 @@
> #error Do not include this header directly
> #endif
>
> +/**
> + * ukarch_fls - find last (highest) set bit in word.
> + * @word: The word to search
> + *
> + * Undefined if no bit exists, so code should check against 0 first.
> + */
> +static inline unsigned int ukarch_fls(unsigned int word)
> +{
> + int clz;
> +
> + /* 000001xxxx = word
> + * 4 = 63 - clz(word)
> + */
> +
> + __asm__("clz %[clz], %[word]\n"
> + :
> + /* Outputs: */
> + [clz] "=r"(clz)
> + :
> + /* Inputs: */
> + [word] "r"(word)
> + );
> +
> + return 63 - clz;
> +}
> +
> /**
> * ukarch_ffsl - find first (lowest) set bit in word.
> * @word: The word to search
> @@ -68,3 +95,29 @@ static inline unsigned long ukarch_ffsl(unsigned long
word)
>
> return 63 - clz;
> }
> +
> +/**
> + * ukarch_flsl - find last (highest) set bit in word.
> + * @word: The word to search
> + *
> + * Undefined if no bit exists, so code should check against 0 first.
> + */
> +static inline unsigned long ukarch_flsl(unsigned long word)
> +{
> + int clz;
> +
> + /* 000001xxxx = word
> + * 4 = 63 - clz(word)
> + */
> +
> + __asm__("clz %[clz], %[word]\n"
> + :
> + /* Outputs: */
> + [clz] "=r"(clz)
> + :
> + /* Inputs: */
> + [word] "r"(word)
> + );
> +
> + return 63 - clz;
> +}
> diff --git a/lib/nolibc/include/unistd.h b/lib/nolibc/include/unistd.h
> index a6986873..263a9f49 100644
> --- a/lib/nolibc/include/unistd.h
> +++ b/lib/nolibc/include/unistd.h
> @@ -55,7 +55,9 @@ extern "C" {
>
> #include <nolibc-internal/shareddefs.h>
>
> +#if CONFIG_LIBUKTIME
Actually, we should check if CONFIG_HAVE_TIME is set (it's a new
addition). The thing is you might have some lib other than uktime
implementing the same thing, but this config flag (CONFIG_HAVE_TIME)
will always tell you that time-related functionality is enabled. You can
even create a patch only for this change.
> unsigned int sleep(unsigned int seconds);
> +#endif
>
> #if CONFIG_LIBVFSCORE
> int close(int fd);
> diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c
> index 56d9d6e1..3ed33e8d 100644
> --- a/plat/common/arm/time.c
> +++ b/plat/common/arm/time.c
> @@ -32,8 +32,10 @@
> * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> */
> #include <stdlib.h>
> -#include <libfdt.h>
> -#include <ofw/fdt.h>
> +#if !CONFIG_PLAT_RASPI
> + #include <libfdt.h>
> + #include <ofw/fdt.h>
> +#endif
> #include <uk/assert.h>
> #include <uk/plat/time.h>
> #include <uk/plat/lcpu.h>
> @@ -44,17 +46,25 @@
> #include <irq.h>
> #include <gic/gic-v2.h>
> #include <arm/time.h>
> +#if CONFIG_PLAT_RASPI
> + #include <raspi/time.h>
> + #include <raspi/irq.h>
> +#endif
>
> /* TODO: For now this file is KVM dependent. As soon as we have more
> * Arm platforms that are using this file, we need to introduce a
> * portable way to handover the DTB entry point to common platform code
*/
> -#include <kvm/config.h>
> +#if CONFIG_PLAT_KVM
> + #include <kvm/config.h>
> +#endif
>
> -static const char * const arch_timer_list[] = {
> - "arm,armv8-timer",
> - "arm,armv7-timer",
> - NULL
> -};
> +#if !CONFIG_PLAT_RASPI
> + static const char * const arch_timer_list[] = {
> + "arm,armv8-timer",
> + "arm,armv7-timer",
> + NULL
> + };
> +#endif
>
> static uint64_t boot_ticks;
> static uint32_t counter_freq;
> @@ -163,6 +173,9 @@ static inline void generic_timer_disable(void)
> static inline void generic_timer_mask_irq(void)
> {
> set_el0(cntv_ctl, get_el0(cntv_ctl) | GT_TIMER_MASK_IRQ);
> + #if CONFIG_PLAT_RASPI
> + *RASPI_ARM_C0_TIMER_IRQ_CTL = *RASPI_ARM_C0_TIMER_IRQ_CTL & ~(1
<< 3);
> + #endif
>
> /* Ensure the write of sys register is visible */
> isb();
> @@ -171,7 +184,18 @@ static inline void generic_timer_mask_irq(void)
> static inline void generic_timer_unmask_irq(void)
> {
> set_el0(cntv_ctl, get_el0(cntv_ctl) & ~GT_TIMER_MASK_IRQ);
> + #if CONFIG_PLAT_RASPI
> + *RASPI_ARM_C0_TIMER_IRQ_CTL = *RASPI_ARM_C0_TIMER_IRQ_CTL | (1
<< 3);
> + #endif
> +
> + /* Ensure the write of sys register is visible */
> + isb();
> +}
>
> +static inline void generic_timer_clear_irq(void)
> +{
> + set_el0(cntv_ctl, get_el0(cntv_ctl) & ~GT_TIMER_IRQ_STATUS);
> +
> /* Ensure the write of sys register is visible */
> isb();
> }
> @@ -209,27 +233,34 @@ static inline uint64_t generic_timer_get_ticks(void)
> }
> #endif
>
> -static uint32_t generic_timer_get_frequency(int fdt_timer)
> -{
> - int len;
> - const uint64_t *fdt_freq;
> -
> - /*
> - * 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_timer, "clock-frequency", &len);
> - if (!fdt_freq || (len <= 0)) {
> - uk_pr_info("No clock-frequency found, reading from register
directly.\n");
> -
> - /* No workaround, get from register directly */
> +#if CONFIG_PLAT_RASPI
> + static uint32_t generic_timer_get_frequency(int fdt_timer __unused)
> + {
> return get_el0(cntfrq);
> }
> -
> - return fdt32_to_cpu(fdt_freq[0]);
> -}
> +#else
> + static uint32_t generic_timer_get_frequency(int fdt_timer)
> + {
> + int len;
> + const uint64_t *fdt_freq;
> +
> + /*
> + * 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_timer, "clock-frequency", &len);
> + if (!fdt_freq || (len <= 0)) {
> + uk_pr_info("No clock-frequency found, reading from
register directly.\n");
> +
> + /* No workaround, get from register directly */
> + return get_el0(cntfrq);
> + }
> +
> + return fdt32_to_cpu(fdt_freq[0]);
> + }
> +#endif
>
> /*
> * monotonic_clock(): returns # of nanoseconds passed since
> @@ -263,8 +294,6 @@ static void generic_timer_cpu_block_until(uint64_t
until_ns)
> {
> uint64_t now_ns, until_ticks;
>
> - UK_ASSERT(ukplat_lcpu_irqs_disabled());
> -
> /* Record current ns and until_ticks for timer */
> now_ns = ukplat_monotonic_clock();
> until_ticks = generic_timer_get_ticks()
> @@ -322,6 +351,7 @@ static int generic_timer_irq_handler(void *arg
__unused)
> * generic_timer_cpu_block_until, and then unmask the IRQ.
> */
> generic_timer_mask_irq();
> + generic_timer_clear_irq();
>
> /* Yes, we handled the irq. */
> return 1;
> @@ -333,8 +363,10 @@ void time_block_until(__snsec until)
> {
> while ((__snsec) ukplat_monotonic_clock() < until) {
> generic_timer_cpu_block_until(until);
> - if (__uk_test_and_clear_bit(0, &sched_have_pending_events))
> - break;
> + #if !CONFIG_PLAT_RASPI
> + if (__uk_test_and_clear_bit(0,
&sched_have_pending_events))
> + break;
> + #endif
> }
> }
>
> @@ -354,8 +386,10 @@ __nsec ukplat_wall_clock(void)
> void ukplat_time_init(void)
> {
> int rc, irq, fdt_timer;
> - uint32_t irq_type, hwirq;
> - uint32_t trigger_type;
> + #if !CONFIG_PLAT_RASPI
> + uint32_t irq_type, hwirq;
> + uint32_t trigger_type;
> + #endif
>
> /*
> * Monotonic time begins at boot_ticks (first read of counter
> @@ -364,24 +398,32 @@ 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");
> + #if CONFIG_PLAT_RASPI
> + fdt_timer = 0;
> + #else
> + 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");
> + #endif
>
> rc = generic_timer_init(fdt_timer);
> if (rc < 0)
> UK_CRASH("Failed to initialize platform time\n");
>
> - rc = gic_get_irq_from_dtb(_libkvmplat_cfg.dtb, fdt_timer, 2,
> - &irq_type, &hwirq, &trigger_type);
> - if (rc < 0)
> - UK_CRASH("Failed to find IRQ number from DTB\n");
> -
> - irq = gic_irq_translate(irq_type, hwirq);
> - if (irq < 0 || irq >= __MAX_IRQ)
> - UK_CRASH("Failed to translate IRQ number, type=%u, hwirq=%u\n",
> - irq_type, hwirq);
> + #if CONFIG_PLAT_RASPI
> + irq = IRQ_ID_ARM_GENERIC_TIMER;
> + #else
> + rc = gic_get_irq_from_dtb(_libkvmplat_cfg.dtb, fdt_timer, 2,
> + &irq_type, &hwirq, &trigger_type);
> + if (rc < 0)
> + UK_CRASH("Failed to find IRQ number from DTB\n");
> +
> + irq = gic_irq_translate(irq_type, hwirq);
> + if (irq < 0 || irq >= __MAX_IRQ)
> + UK_CRASH("Failed to translate IRQ number, type=%u,
hwirq=%u\n",
> + irq_type, hwirq);
> + #endif
>
> rc = ukplat_irq_register(irq, generic_timer_irq_handler, NULL);
> if (rc < 0)
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |