[PATCH v2 1/3] lib: utils/timer: Allow ACLINT MTIMER driver to setup quirks

[Anup Patel]

On Fri, Oct 13, 2023 at 12:17 PM Inochi Amaoto <inochiama at outlook.com> wrote:
>
> The quirks checking will cause ACLINT step into a CLINT code path, this
> is not expected when ACLINT needs a quirks.
>
> Add more check to ensure the timer is CLINT before applying quirks.
> And now apply the general quirks after applying CLINT specific quirks.
>
> Signed-off-by: Inochi Amaoto <inochiama at outlook.com>
> ---
>  lib/utils/timer/fdt_timer_mtimer.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/lib/utils/timer/fdt_timer_mtimer.c b/lib/utils/timer/fdt_timer_mtimer.c
> index 9eaa11d..fb5917f 100644
> --- a/lib/utils/timer/fdt_timer_mtimer.c
> +++ b/lib/utils/timer/fdt_timer_mtimer.c
> @@ -29,6 +29,12 @@ static SBI_LIST_HEAD(mtn_list);
>
>  static struct aclint_mtimer_data *mt_reference = NULL;
>
> +static int clint_contains_timer(const void *data)
> +{
> +       const struct timer_mtimer_quirks *quirks = data;
> +       return quirks && quirks->mtime_offset;

This is already broken because you are not allowing mtime_offset
to be zero in quirks.

Drop clint_contains_timer() function.

> +}
> +
>  static int timer_mtimer_cold_init(void *fdt, int nodeoff,
>                                   const struct fdt_match *match)
>  {
> @@ -58,7 +64,7 @@ static int timer_mtimer_cold_init(void *fdt, int nodeoff,
>                 return rc;
>         }
>
> -       if (match->data) { /* SiFive CLINT */
> +       if (clint_contains_timer(match->data)) {
>                 const struct timer_mtimer_quirks *quirks = match->data;
>
>                 /* Set CLINT addresses */
> @@ -74,8 +80,6 @@ static int timer_mtimer_cold_init(void *fdt, int nodeoff,
>                         mt->mtime_addr = mt->mtime_size = 0;
>                 }
>                 mt->mtimecmp_addr += quirks->mtime_offset;
> -               /* Apply additional CLINT quirks */
> -               mt->has_64bit_mmio = quirks->has_64bit_mmio;
>         } else { /* RISC-V ACLINT MTIMER */
>                 /* Set ACLINT MTIMER addresses */
>                 mt->mtime_addr = addr[0];
> @@ -84,6 +88,13 @@ static int timer_mtimer_cold_init(void *fdt, int nodeoff,
>                 mt->mtimecmp_size = size[1];
>         }
>
> +       /* Apply additional quirks for CLINT and ACLINT */
> +       if (match->data) {
> +               const struct timer_mtimer_quirks *quirks = match->data;
> +
> +               mt->has_64bit_mmio = quirks->has_64bit_mmio;
> +       }
> +
>         /* Check if MTIMER device has shared MTIME address */
>         if (mt->mtime_size) {
>                 mt->has_shared_mtime = false;
> --
> 2.42.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Instead of above, I suggest to do as follows:

diff --git a/lib/utils/timer/fdt_timer_mtimer.c
b/lib/utils/timer/fdt_timer_mtimer.c
index 9eaa11d..0187272 100644
--- a/lib/utils/timer/fdt_timer_mtimer.c
+++ b/lib/utils/timer/fdt_timer_mtimer.c
@@ -16,6 +16,7 @@
 #include <sbi_utils/timer/aclint_mtimer.h>

 struct timer_mtimer_quirks {
+    bool        custom_aclint;
     unsigned int    mtime_offset;
     bool        has_64bit_mmio;
     bool        without_mtime;
@@ -36,6 +37,7 @@ static int timer_mtimer_cold_init(void *fdt, int nodeoff,
     unsigned long addr[2], size[2];
     struct timer_mtimer_node *mtn, *n;
     struct aclint_mtimer_data *mt;
+    const struct timer_mtimer_quirks *quirks = match->data;

     mtn = sbi_zalloc(sizeof(*mtn));
     if (!mtn)
@@ -58,9 +60,7 @@ static int timer_mtimer_cold_init(void *fdt, int nodeoff,
         return rc;
     }

-    if (match->data) { /* SiFive CLINT */
-        const struct timer_mtimer_quirks *quirks = match->data;
-
+    if (quirks && !quirks->custom_aclint) { /* SiFive CLINT */
         /* Set CLINT addresses */
         mt->mtimecmp_addr = addr[0] + ACLINT_DEFAULT_MTIMECMP_OFFSET;
         mt->mtimecmp_size = ACLINT_DEFAULT_MTIMECMP_SIZE;
@@ -74,8 +74,6 @@ static int timer_mtimer_cold_init(void *fdt, int nodeoff,
             mt->mtime_addr = mt->mtime_size = 0;
         }
         mt->mtimecmp_addr += quirks->mtime_offset;
-        /* Apply additional CLINT quirks */
-        mt->has_64bit_mmio = quirks->has_64bit_mmio;
     } else { /* RISC-V ACLINT MTIMER */
         /* Set ACLINT MTIMER addresses */
         mt->mtime_addr = addr[0];
@@ -84,6 +82,11 @@ static int timer_mtimer_cold_init(void *fdt, int nodeoff,
         mt->mtimecmp_size = size[1];
     }

+    if (quirks) {
+        /* Apply additional quirks */
+        mt->has_64bit_mmio = quirks->has_64bit_mmio;
+    }
+
     /* Check if MTIMER device has shared MTIME address */
     if (mt->mtime_size) {
         mt->has_shared_mtime = false;

Regards,
Anup