Received-SPF: pass (google.com: domain of linux-kernel+bounces-120492-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240325164021.3229-1-jszhang@kernel.org> <20240325164021.3229-3-jszhang@kernel.org>
 <b063df9b-90b6-4f06-8be5-5a8c267e6c8d@sifive.com> <ZgL31BFWvaLwYQrN@xhacker>
In-Reply-To: <ZgL31BFWvaLwYQrN@xhacker>
From: yunhui cui <cuiyunhui@bytedance.com>
Date: Wed, 27 Mar 2024 15:58:01 +0800
Message-ID: <CAEEQ3w=NZ5YR8bsOKdhLfeGk7u-7rUHGYA2mvcoO1o-ZaZT-jQ@mail.gmail.com>
Subject: Re: [External] Re: [PATCH 2/5] riscv: nommu: use CSR_TIME* for
 get_cycles* implementation
To: Jisheng Zhang <jszhang@kernel.org>
Cc: Samuel Holland <samuel.holland@sifive.com>, Paul Walmsley <paul.walmsley@sifive.com>, 
	Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, 
	Daniel Lezcano <daniel.lezcano@linaro.org>, Thomas Gleixner <tglx@linutronix.de>, 
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Jisheng,

On Wed, Mar 27, 2024 at 12:43=E2=80=AFAM Jisheng Zhang <jszhang@kernel.org>=
 wrote:
>
> On Mon, Mar 25, 2024 at 09:39:26PM -0500, Samuel Holland wrote:
> > Hi Jisheng,
> >
> > On 2024-03-25 11:40 AM, Jisheng Zhang wrote:
> > > Per riscv privileged spec, "The time CSR is a read-only shadow of the
> > > memory-mapped mtime register", "On RV32I the timeh CSR is a read-only
> > > shadow of the upper 32 bits of the memory-mapped mtime register, whil=
e
> > > time shadows only the lower 32 bits of mtime." Since get_cycles() onl=
y
> > > reads the timer, it's fine to use CSR_TIME to implement get_cycles().
> >
> > Unfortunately there are various implementations (e.g. FU740/Unmatched, =
probably
> > K210 which this code was originally used for) which do not implement th=
e time
> > CSR, relying on M-mode software to emulate the CSR so S-mode software d=
oesn't
> > notice. So this code is needed to support those platforms when running =
Linux in
> > M-mode.
>
> OOPS, I knew this for the first time there are such implementations
> which doesn't implement the TIME CSR :(
>
> >
> > Maybe there should be an option to assume the time CSR is/is not implem=
ented,
> > like there is for misaligned access?
>
> Yep, this seems the only solution. Then which should be the default
> choice? I.E
>
> Assume all NOMMU goes through TIME CSR, and provide an option for
> platform lacking of TIME CSR. This prefers TIME CSR.
>
> VS.
>
> By default, MTIME is used, and provide one Kconfig option for TIME CSR
> usage. This prefers MTIME
>
> which choice is better? Any suggestion?
>
> Thanks in advance
>
> >
> > Regards,
> > Samuel
> >
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > ---
> > >  arch/riscv/include/asm/timex.h | 40 --------------------------------=
--
> > >  1 file changed, 40 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/=
timex.h
> > > index a06697846e69..a3fb85d505d4 100644
> > > --- a/arch/riscv/include/asm/timex.h
> > > +++ b/arch/riscv/include/asm/timex.h
> > > @@ -10,44 +10,6 @@
> > >
> > >  typedef unsigned long cycles_t;
> > >
> > > -#ifdef CONFIG_RISCV_M_MODE
> > > -
> > > -#include <asm/clint.h>
> > > -
> > > -#ifdef CONFIG_64BIT
> > > -static inline cycles_t get_cycles(void)
> > > -{
> > > -   return readq_relaxed(clint_time_val);
> > > -}
> > > -#else /* !CONFIG_64BIT */
> > > -static inline u32 get_cycles(void)
> > > -{
> > > -   return readl_relaxed(((u32 *)clint_time_val));
> > > -}
> > > -#define get_cycles get_cycles
> > > -
> > > -static inline u32 get_cycles_hi(void)
> > > -{
> > > -   return readl_relaxed(((u32 *)clint_time_val) + 1);
> > > -}
> > > -#define get_cycles_hi get_cycles_hi
> > > -#endif /* CONFIG_64BIT */
> > > -
> > > -/*
> > > - * Much like MIPS, we may not have a viable counter to use at an ear=
ly point
> > > - * in the boot process. Unfortunately we don't have a fallback, so i=
nstead
> > > - * we just return 0.
> > > - */
> > > -static inline unsigned long random_get_entropy(void)
> > > -{
> > > -   if (unlikely(clint_time_val =3D=3D NULL))
> > > -           return random_get_entropy_fallback();
> > > -   return get_cycles();
> > > -}
> > > -#define random_get_entropy()       random_get_entropy()
> > > -
> > > -#else /* CONFIG_RISCV_M_MODE */
> > > -
> > >  static inline cycles_t get_cycles(void)
> > >  {
> > >     return csr_read(CSR_TIME);
> > > @@ -60,8 +22,6 @@ static inline u32 get_cycles_hi(void)
> > >  }
> > >  #define get_cycles_hi get_cycles_hi
> > >
> > > -#endif /* !CONFIG_RISCV_M_MODE */
> > > -
> > >  #ifdef CONFIG_64BIT
> > >  static inline u64 get_cycles64(void)
> > >  {
> >
>

I'd like to take this thread to ask:
csr_read(CSR_TIME) returns a timestamp, right? So why is the function
name get_cycles()? Instead of get_timestamp()?

Thanks,
Yunhui