[PATCH 6/7] lib: sbi: Simplify HSM platform operations
Anup Patel
Anup.Patel at wdc.com
Wed Apr 28 13:14:20 BST 2021
> -----Original Message-----
> From: Alistair Francis <Alistair.Francis at wdc.com>
> Sent: 26 April 2021 11:39
> To: Atish Patra <Atish.Patra at wdc.com>; Anup Patel
> <Anup.Patel at wdc.com>
> Cc: anup at brainfault.org; opensbi at lists.infradead.org
> Subject: Re: [PATCH 6/7] lib: sbi: Simplify HSM platform operations
>
> On Thu, 2021-04-22 at 16:50 +0530, Anup Patel wrote:
> > Instead of having hsm_start(), hsm_stop() and hsm_suspend() callbacks
> > in platform operations, it will be much simpler for HSM driver to
> > directly register these operations as a device to the sbi_hsm
> > implementation.
> >
> > Signed-off-by: Anup Patel <anup.patel at wdc.com>
>
> Reviewed-by: Alistair Francis <alistair.francis at wdc.com>
Applied this patch to the riscv/opensbi repo
Thanks,
Anup
>
> Alistair
>
> > ---
> > include/sbi/sbi_hsm.h | 31 +++++++++++++
> > include/sbi/sbi_platform.h | 83
> > +---------------------------------
> > lib/sbi/sbi_hsm.c | 68 +++++++++++++++++++++++-----
> > lib/sbi/sbi_platform.c | 6 ---
> > platform/thead/c910/platform.c | 27 ++++++-----
> > platform/thead/c910/platform.h | 3 +-
> > 6 files changed, 105 insertions(+), 113 deletions(-)
> >
> > diff --git a/include/sbi/sbi_hsm.h b/include/sbi/sbi_hsm.h index
> > bf0c1a5..c16e871 100644
> > --- a/include/sbi/sbi_hsm.h
> > +++ b/include/sbi/sbi_hsm.h
> > @@ -12,9 +12,40 @@
> >
> > #include <sbi/sbi_types.h>
> >
> > +/** Hart state managment device */
> > +struct sbi_hsm_device {
> > + /** Name of the hart state managment device */
> > + char name[32];
> > +
> > + /** Start (or power-up) the given hart */
> > + int (*hart_start)(u32 hartid, ulong saddr);
> > +
> > + /**
> > + * Stop (or power-down) the current hart from running. This
> > call
> > + * doesn't expect to return if success.
> > + */
> > + int (*hart_stop)(void);
> > +
> > + /**
> > + * Put the current hart in platform specific suspend (or low-
> > power)
> > + * state.
> > + *
> > + * For successful retentive suspend, the call will return 0
> > when
> > + * the hart resumes normal execution.
> > + *
> > + * For successful non-retentive suspend, the hart will resume
> > from
> > + * specified resume address
> > + */
> > + int (*hart_suspend)(u32 suspend_type, ulong raddr); };
> > +
> > struct sbi_domain;
> > struct sbi_scratch;
> >
> > +const struct sbi_hsm_device *sbi_hsm_get_device(void);
> > +
> > +void sbi_hsm_set_device(const struct sbi_hsm_device *dev);
> > +
> > int sbi_hsm_init(struct sbi_scratch *scratch, u32 hartid, bool
> > cold_boot);
> > void __noreturn sbi_hsm_exit(struct sbi_scratch *scratch);
> >
> > diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> > index 921d39c..f8074d2 100644
> > --- a/include/sbi/sbi_platform.h
> > +++ b/include/sbi/sbi_platform.h
> > @@ -51,15 +51,11 @@ struct sbi_trap_regs;
> >
> > /** Possible feature flags of a platform */
> > enum sbi_platform_features {
> > - /** Platform has HART hotplug support */
> > - SBI_PLATFORM_HAS_HART_HOTPLUG = (1 << 0),
> > /** Platform has fault delegation support */
> > SBI_PLATFORM_HAS_MFAULTS_DELEGATION = (1 << 1),
> > - /** Platform has custom secondary hart booting support */
> > - SBI_PLATFORM_HAS_HART_SECONDARY_BOOT = (1 << 2),
> >
> > /** Last index of Platform features*/
> > - SBI_PLATFORM_HAS_LAST_FEATURE =
> > SBI_PLATFORM_HAS_HART_SECONDARY_BOOT,
> > + SBI_PLATFORM_HAS_LAST_FEATURE =
> > SBI_PLATFORM_HAS_MFAULTS_DELEGATION,
> > };
> >
> > /** Default feature set for a platform */ @@ -114,19 +110,6 @@ struct
> > sbi_platform_operations {
> > /** Exit platform timer for current HART */
> > void (*timer_exit)(void);
> >
> > - /** Bringup the given hart */
> > - int (*hart_start)(u32 hartid, ulong saddr);
> > - /**
> > - * Stop the current hart from running. This call doesn't
> > expect to
> > - * return if success.
> > - */
> > - int (*hart_stop)(void);
> > - /**
> > - * Put the current hart in platform specific suspend (or low-
> > power)
> > - * state.
> > - */
> > - int (*hart_suspend)(u32 suspend_type, ulong raddr);
> > -
> > /** platform specific SBI extension implementation probe
> > function */
> > int (*vendor_ext_check)(long extid);
> > /** platform specific SBI extension implementation provider */
> > @@ -193,15 +176,9 @@ struct sbi_platform {
> > #define sbi_platform_ops(__p) \
> > ((const struct sbi_platform_operations *)(__p)-
> > >platform_ops_addr)
> >
> > -/** Check whether the platform supports HART hotplug */ -#define
> > sbi_platform_has_hart_hotplug(__p) \
> > - ((__p)->features & SBI_PLATFORM_HAS_HART_HOTPLUG)
> > /** Check whether the platform supports fault delegation */
> > #define sbi_platform_has_mfaults_delegation(__p) \
> > ((__p)->features & SBI_PLATFORM_HAS_MFAULTS_DELEGATION)
> > -/** Check whether the platform supports custom secondary hart booting
> > support */ -#define sbi_platform_has_hart_secondary_boot(__p) \
> > - ((__p)->features & SBI_PLATFORM_HAS_HART_SECONDARY_BOOT)
> >
> > /**
> > * Get HART index for the given HART
> > @@ -316,64 +293,6 @@ static inline bool
> > sbi_platform_hart_invalid(const struct sbi_platform *plat,
> > return FALSE;
> > }
> >
> > -/**
> > - * Bringup a given hart from previous stage. Platform should
> > implement this
> > - * operation if they support a custom mechanism to start a hart.
> > Otherwise,
> > - * a generic WFI based approach will be used to start/stop a hart in
> > OpenSBI.
> > - *
> > - * @param plat pointer to struct sbi_platform
> > - * @param hartid HART id
> > - * @param saddr M-mode start physical address for the HART
> > - *
> > - * @return 0 if sucessful and negative error code on failure
> > - */
> > -static inline int sbi_platform_hart_start(const struct sbi_platform
> > *plat,
> > - u32 hartid, ulong saddr) -{
> > - if (plat && sbi_platform_ops(plat)->hart_start)
> > - return sbi_platform_ops(plat)->hart_start(hartid,
> > saddr);
> > - return SBI_ENOTSUPP;
> > -}
> > -
> > -/**
> > - * Stop the current hart in OpenSBI.
> > - *
> > - * @param plat pointer to struct sbi_platform
> > - *
> > - * @return Negative error code on failure. It doesn't return on
> > success.
> > - */
> > -static inline int sbi_platform_hart_stop(const struct sbi_platform
> > *plat)
> > -{
> > - if (plat && sbi_platform_ops(plat)->hart_stop)
> > - return sbi_platform_ops(plat)->hart_stop();
> > - return SBI_ENOTSUPP;
> > -}
> > -
> > -/**
> > - * Put the current hart in platform specific suspend (or low-power)
> > state.
> > - *
> > - * For successful retentive suspend, the call will return 0 when the
> > hart
> > - * resumes normal execution.
> > - *
> > - * For successful non-retentive suspend, the hart will resume from
> > specified
> > - * resume address
> > - *
> > - * @param plat pointer to struct sbi_platform
> > - * @param suspend_type the type of suspend
> > - * @param raddr physical address where the hart can resume in M-mode
> > after
> > - * non-retantive suspend
> > - *
> > - * @return 0 if successful and negative error code on failure
> > - */
> > -static inline int sbi_platform_hart_suspend(const struct sbi_platform
> > *plat,
> > - u32 suspend_type, ulong
> > raddr)
> > -{
> > - if (plat && sbi_platform_ops(plat)->hart_suspend)
> > - return sbi_platform_ops(plat)-
> > >hart_suspend(suspend_type,
> > - raddr);
> > - return SBI_ENOTSUPP;
> > -}
> > -
> > /**
> > * Early initialization for current HART
> > *
> > diff --git a/lib/sbi/sbi_hsm.c b/lib/sbi/sbi_hsm.c index
> > 64d299b..4662150 100644
> > --- a/lib/sbi/sbi_hsm.c
> > +++ b/lib/sbi/sbi_hsm.c
> > @@ -21,11 +21,12 @@
> > #include <sbi/sbi_hsm.h>
> > #include <sbi/sbi_init.h>
> > #include <sbi/sbi_ipi.h>
> > -#include <sbi/sbi_platform.h>
> > +#include <sbi/sbi_scratch.h>
> > #include <sbi/sbi_system.h>
> > #include <sbi/sbi_timer.h>
> > #include <sbi/sbi_console.h>
> >
> > +static const struct sbi_hsm_device *hsm_dev = NULL;
> > static unsigned long hart_data_offset;
> >
> > /** Per hart specific data to manage state transition **/ @@ -129,6
> > +130,54 @@ static void sbi_hsm_hart_wait(struct sbi_scratch *scratch,
> > u32 hartid)
> > */
> > }
> >
> > +const struct sbi_hsm_device *sbi_hsm_get_device(void) {
> > + return hsm_dev;
> > +}
> > +
> > +void sbi_hsm_set_device(const struct sbi_hsm_device *dev) {
> > + if (!dev || hsm_dev)
> > + return;
> > +
> > + hsm_dev = dev;
> > +}
> > +
> > +static bool hsm_device_has_hart_hotplug(void) {
> > + if (hsm_dev && hsm_dev->hart_start && hsm_dev->hart_stop)
> > + return true;
> > + return false;
> > +}
> > +
> > +static bool hsm_device_has_hart_secondary_boot(void)
> > +{
> > + if (hsm_dev && hsm_dev->hart_start && !hsm_dev->hart_stop)
> > + return true;
> > + return false;
> > +}
> > +
> > +static int hsm_device_hart_start(u32 hartid, ulong saddr) {
> > + if (hsm_dev && hsm_dev->hart_start)
> > + return hsm_dev->hart_start(hartid, saddr);
> > + return SBI_ENOTSUPP;
> > +}
> > +
> > +static int hsm_device_hart_stop(void) {
> > + if (hsm_dev && hsm_dev->hart_stop)
> > + return hsm_dev->hart_stop();
> > + return SBI_ENOTSUPP;
> > +}
> > +
> > +static int hsm_device_hart_suspend(u32 suspend_type, ulong raddr) {
> > + if (hsm_dev && hsm_dev->hart_suspend)
> > + return hsm_dev->hart_suspend(suspend_type, raddr);
> > + return SBI_ENOTSUPP;
> > +}
> > +
> > int sbi_hsm_init(struct sbi_scratch *scratch, u32 hartid, bool
> > cold_boot)
> > {
> > u32 i;
> > @@ -164,7 +213,6 @@ int sbi_hsm_init(struct sbi_scratch *scratch, u32
> > hartid, bool cold_boot)
> > void __noreturn sbi_hsm_exit(struct sbi_scratch *scratch)
> > {
> > u32 hstate;
> > - const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> > struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
> >
> > hart_data_offset);
> > void (*jump_warmboot)(void) = (void (*)(void))scratch-
> > >warmboot_addr;
> > @@ -174,8 +222,8 @@ void __noreturn sbi_hsm_exit(struct sbi_scratch
> > *scratch)
> > if (hstate != SBI_HSM_STATE_STOP_PENDING)
> > goto fail_exit;
> >
> > - if (sbi_platform_has_hart_hotplug(plat)) {
> > - sbi_platform_hart_stop(plat);
> > + if (hsm_device_has_hart_hotplug()) {
> > + hsm_device_hart_stop();
> > /* It should never reach here */
> > goto fail_exit;
> > }
> > @@ -201,7 +249,6 @@ int sbi_hsm_hart_start(struct sbi_scratch
> > *scratch,
> > unsigned int hstate;
> > struct sbi_scratch *rscratch;
> > struct sbi_hsm_data *hdata;
> > - const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> >
> > /* For now, we only allow start mode to be S-mode or U-mode.
> > */
> > if (smode != PRV_S && smode != PRV_U) @@ -233,10 +280,9 @@ int
> > sbi_hsm_hart_start(struct sbi_scratch *scratch,
> > rscratch->next_addr = saddr;
> > rscratch->next_mode = smode;
> >
> > - if (sbi_platform_has_hart_hotplug(plat) ||
> > - (sbi_platform_has_hart_secondary_boot(plat) &&
> > !init_count)) {
> > - return sbi_platform_hart_start(plat, hartid,
> > -
> > scratch->warmboot_addr);
> > + if (hsm_device_has_hart_hotplug() ||
> > + (hsm_device_has_hart_secondary_boot() && !init_count)) {
> > + return hsm_device_hart_start(hartid, scratch-
> > >warmboot_addr);
> > } else {
> > sbi_ipi_raw_send(hartid);
> > }
> > @@ -374,7 +420,6 @@ int sbi_hsm_hart_suspend(struct sbi_scratch
> > *scratch, u32 suspend_type,
> > {
> > int oldstate, ret;
> > const struct sbi_domain *dom = sbi_domain_thishart_ptr();
> > - const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> > struct sbi_hsm_data *hdata = sbi_scratch_offset_ptr(scratch,
> >
> > hart_data_offset);
> >
> > @@ -420,8 +465,7 @@ int sbi_hsm_hart_suspend(struct sbi_scratch
> > *scratch, u32 suspend_type,
> > hdata->suspend_type = suspend_type;
> >
> > /* Try platform specific suspend */
> > - ret = sbi_platform_hart_suspend(plat, suspend_type,
> > - scratch->warmboot_addr);
> > + ret = hsm_device_hart_suspend(suspend_type, scratch-
> > >warmboot_addr);
> > if (ret == SBI_ENOTSUPP) {
> > /* Try generic implementation of default suspend types
> > */
> > if (suspend_type == SBI_HSM_SUSPEND_RET_DEFAULT) {
> > diff --git a/lib/sbi/sbi_platform.c b/lib/sbi/sbi_platform.c index
> > e78119a..e8b94a3 100644
> > --- a/lib/sbi/sbi_platform.c
> > +++ b/lib/sbi/sbi_platform.c
> > @@ -19,15 +19,9 @@ static inline char
> > *sbi_platform_feature_id2string(unsigned long feature)
> > return NULL;
> >
> > switch (feature) {
> > - case SBI_PLATFORM_HAS_HART_HOTPLUG:
> > - fstr = "hotplug";
> > - break;
> > case SBI_PLATFORM_HAS_MFAULTS_DELEGATION:
> > fstr = "mfdeleg";
> > break;
> > - case SBI_PLATFORM_HAS_HART_SECONDARY_BOOT:
> > - fstr = "sec_boot";
> > - break;
> > default:
> > break;
> > }
> > diff --git a/platform/thead/c910/platform.c
> > b/platform/thead/c910/platform.c index 8f8069c..8a9318c 100644
> > --- a/platform/thead/c910/platform.c
> > +++ b/platform/thead/c910/platform.c
> > @@ -7,6 +7,7 @@
> > #include <sbi/sbi_console.h>
> > #include <sbi/sbi_const.h>
> > #include <sbi/sbi_hart.h>
> > +#include <sbi/sbi_hsm.h>
> > #include <sbi/sbi_platform.h>
> > #include <sbi/sbi_system.h>
> > #include <sbi_utils/irqchip/plic.h>
> > @@ -39,6 +40,19 @@ static struct sbi_system_reset_device c910_reset =
> > {
> > .system_reset = c910_system_reset
> > };
> >
> > +static int c910_hart_start(u32 hartid, ulong saddr) {
> > + csr_write(CSR_MRVBR, saddr);
> > + csr_write(CSR_MRMR, csr_read(CSR_MRMR) | (1 << hartid));
> > +
> > + return 0;
> > +}
> > +
> > +static struct sbi_hsm_device c910_hsm = {
> > + .name = "thead_c910_hsm",
> > + .hart_start = c910_hart_start
> > +};
> > +
> > static int c910_early_init(bool cold_boot)
> > {
> > if (cold_boot) {
> > @@ -63,6 +77,7 @@ static int c910_early_init(bool cold_boot)
> > c910_regs.clint_base_addr =
> > c910_regs.plic_base_addr +
> > C910_PLIC_CLINT_OFFSET;
> >
> > + sbi_hsm_set_device(&c910_hsm);
> > sbi_system_reset_set_device(&c910_reset);
> > } else {
> > /* Store to other core */ @@ -127,14 +142,6 @@ static
> > int c910_timer_init(bool cold_boot)
> > return clint_warm_timer_init();
> > }
> >
> > -int c910_hart_start(u32 hartid, ulong saddr) -{
> > - csr_write(CSR_MRVBR, saddr);
> > - csr_write(CSR_MRMR, csr_read(CSR_MRMR) | (1 << hartid));
> > -
> > - return 0;
> > -}
> > -
> > const struct sbi_platform_operations platform_ops = {
> > .early_init = c910_early_init,
> > .final_init = c910_final_init, @@ -143,9 +150,7 @@
> > const struct sbi_platform_operations platform_ops = {
> >
> > .ipi_init = c910_ipi_init,
> >
> > - .timer_init = c910_timer_init,
> > -
> > - .hart_start = c910_hart_start,
> > + .timer_init = c910_timer_init
> > };
> >
> > const struct sbi_platform platform = { diff --git
> > a/platform/thead/c910/platform.h b/platform/thead/c910/platform.h
> > index 354404e..880cb6b 100644
> > --- a/platform/thead/c910/platform.h
> > +++ b/platform/thead/c910/platform.h
> > @@ -8,8 +8,7 @@
> > #define C910_HART_COUNT 16
> >
> > #define SBI_THEAD_FEATURES \
> > - (SBI_PLATFORM_HAS_MFAULTS_DELEGATION | \
> > - SBI_PLATFORM_HAS_HART_SECONDARY_BOOT)
> > + (SBI_PLATFORM_HAS_MFAULTS_DELEGATION)
> >
> > #define CSR_MCOR 0x7c2
> > #define CSR_MHCR 0x7c1
More information about the opensbi
mailing list