[Skiboot] [PATCH v4 9/9] npu: Add CONFIG_NPU to optionally skip NPU code
Cédric Le Goater
clg at kaod.org
Sat Dec 18 03:04:18 AEDT 2021
On 12/17/21 03:36, Nicholas Piggin wrote:
> From: Stewart Smith <stewart at flamingspork.com>
>
> Saves a whopping 39kb of skiboot.lid.xz.
ok, so CONFIG_NPU includes also NPU2 support. This patch is confusing.
I think this is because the code base has changed (P10) and because
it is doing more that it says. Could we split it please ?
> Reviewed-by: Dan Horák <dan at danny.cz>
> Signed-off-by: Stewart Smith <stewart at flamingspork.com>
> ---
> Makefile | 2 ++
> Makefile.main | 4 ++++
> core/hmi.c | 10 +++++++++-
> core/platform.c | 1 -
> hw/Makefile.inc | 18 ++++++++++++++----
> hw/npu2-common.c | 3 ++-
> hw/npu2.c | 1 +
> include/npu2.h | 8 +++++++-
> include/pci.h | 3 +++
> platforms/astbmc/Makefile.inc | 15 +++++++++++----
> 10 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 625f212ea..ce61e3af0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -61,6 +61,8 @@ DEAD_CODE_ELIMINATION ?= 0
> CONFIG_FSP?=1
> # Try to build without POWER8 support
> CONFIG_P8?=1
> +# Try and build without any NPU support
> +CONFIG_NPU?=1
>
> #
> # Where is the source directory, must be a full path (no ~)
> diff --git a/Makefile.main b/Makefile.main
> index 2a346a6c9..dce0338da 100644
> --- a/Makefile.main
> +++ b/Makefile.main
> @@ -165,6 +165,10 @@ ifeq ($(CONFIG_P8),1)
> CFLAGS += -DCONFIG_P8=1
> endif
>
> +ifeq ($(CONFIG_NPU),1)
> +CFLAGS += -DCONFIG_NPU=1
> +endif
> +
> CFLAGS += $(call try-cflag,$(CC),-Wjump-misses-init) \
> $(call try-cflag,$(CC),-Wsuggest-attribute=const) \
> $(call try-cflag,$(CC),-Wsuggest-attribute=noreturn) \
> diff --git a/core/hmi.c b/core/hmi.c
> index fe3d82529..1af9971d2 100644
> --- a/core/hmi.c
> +++ b/core/hmi.c
> @@ -719,6 +719,7 @@ static void find_nx_checkstop_reason(int flat_chip_id,
> queue_hmi_event(hmi_evt, 0, out_flags);
> }
>
> +#ifdef CONFIG_NPU
> static void add_npu_xstop_reason(uint32_t *xstop_reason, uint8_t reason)
> {
> int i, reason_count;
> @@ -854,7 +855,7 @@ static bool npu_fir_errors(struct phb *phb, int flat_chip_id,
> switch (phb->phb_type) {
> case phb_type_npu_v2:
> case phb_type_npu_v2_opencapi:
> - npu2_dump_scoms(npu2, flat_chip_id);
> + phb->ops->dump_debug_data(phb, flat_chip_id);
Can't we add an empty stub for npu2_dump_scoms() ?
hmm, do we even need a stub since both add_npu_xstop_reason() and
npu2_dump_scoms() are not compiled when CONFIG_NPU=0 ?
> break;
> case phb_type_pau_opencapi:
> pau_opencapi_dump_scoms(pau);
> @@ -922,6 +923,13 @@ static void find_npu_checkstop_reason(int flat_chip_id,
> queue_hmi_event(hmi_evt, 1, out_flags);
> }
> }
> +#else
> +static void find_npu_checkstop_reason(int flat_chip_id __unused,
> + struct OpalHMIEvent *hmi_evt __unused,
> + uint64_t *out_flags __unused)
> +{
> +}
> +#endif
This is stripping PAU support also :/
>
> static void decode_malfunction(struct OpalHMIEvent *hmi_evt, uint64_t *out_flags)
> {
> diff --git a/core/platform.c b/core/platform.c
> index 320fdea03..3f4c8bdd5 100644
> --- a/core/platform.c
> +++ b/core/platform.c
> @@ -226,7 +226,6 @@ static struct platform generic_platform = {
> .start_preload_resource = generic_start_preload_resource,
> .resource_loaded = generic_resource_loaded,
> .ocapi = &generic_ocapi,
> - .npu2_device_detect = npu2_i2c_presence_detect, /* Assumes ZZ */
That change belongs to another patch.
> };
>
> const struct bmc_platform *bmc_platform = &generic_bmc;
> diff --git a/hw/Makefile.inc b/hw/Makefile.inc
> index 302718885..7fb7c38f8 100644
> --- a/hw/Makefile.inc
> +++ b/hw/Makefile.inc
> @@ -5,14 +5,24 @@ HW_OBJS += homer.o slw.o occ.o fsi-master.o centaur.o imc.o
> HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-compress.o nx-842.o nx-gzip.o
> HW_OBJS += sfc-ctrl.o fake-rtc.o bt.o p8-i2c.o prd.o
> HW_OBJS += dts.o lpc-rtc.o xive.o phb4.o
> -HW_OBJS += fake-nvram.o lpc-mbox.o npu2.o npu2-hw-procedures.o
> -HW_OBJS += npu2-common.o npu2-opencapi.o phys-map.o sbe-p9.o capp.o
> +HW_OBJS += fake-nvram.o lpc-mbox.o
> +HW_OBJS += phys-map.o sbe-p9.o capp.o
> HW_OBJS += occ-sensor.o vas.o sbe-p8.o dio-p9.o lpc-port80h.o cache-p9.o
> -HW_OBJS += npu-opal.o ocmb.o xive2.o pau.o pau-hw-procedures.o
> +HW_OBJS += ocmb.o xive2.o pau.o pau-hw-procedures.o
> +
> +ifeq ($(CONFIG_NPU),1)
> +HW_OBJS += npu2.o npu2-hw-procedures.o
> +HW_OBJS += npu2-common.o npu2-opencapi.o
> +HW_OBJS += npu-opal.o
> ifeq ($(CONFIG_P8),1)
> -HW_OBJS += phb3.o
> HW_OBJS += npu.o npu-hw-procedures.o
> endif
> +endif
> +
> +ifeq ($(CONFIG_P8),1)
> +HW_OBJS += phb3.o
> +endif
> +
> HW=hw/built-in.a
>
> include $(SRC)/hw/fsp/Makefile.inc
> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
> index 02f102fe5..33339ffa1 100644
> --- a/hw/npu2-common.c
> +++ b/hw/npu2-common.c
> @@ -296,8 +296,9 @@ static void show_all_regs(struct npu2 *npu, int brick_index)
> }
> }
>
> -void npu2_dump_scoms(struct npu2 *npu, int chip_id)
> +void npu2_dump_scoms(struct phb *phb, int chip_id)
> {
> + struct npu2 *npu = phb_to_npu2_nvlink(phb);
> if (npu && npu->chip_id == chip_id)
> show_all_regs(npu, -1 /* all bricks */);
> }
> diff --git a/hw/npu2.c b/hw/npu2.c
> index cf57eeb0c..e18a1b7b1 100644
> --- a/hw/npu2.c
> +++ b/hw/npu2.c
> @@ -1316,6 +1316,7 @@ static const struct phb_ops npu_ops = {
> .set_capi_mode = NULL,
> .set_capp_recovery = NULL,
> .tce_kill = npu2_tce_kill,
> + .dump_debug_data = npu2_dump_scoms,
> };
>
> static void assign_mmio_bars(uint64_t gcid, uint32_t scom, uint64_t reg[2], uint64_t mm_win[2])
> diff --git a/include/npu2.h b/include/npu2.h
> index b302108b9..374c20779 100644
> --- a/include/npu2.h
> +++ b/include/npu2.h
> @@ -212,7 +212,13 @@ static inline struct phb *npu2_dev_to_phb(struct npu2_dev *ndev)
> }
> }
>
> +#ifdef CONFIG_NPU
> void npu2_i2c_presence_detect(struct npu2 *npu);
> +#else
> +static inline void npu2_i2c_presence_detect(struct npu2 *npu __unused)
> +{
> +}
> +#endif
This looks weird. can't we simply add #ifdef CONFIG_NPU under the
platform definition to drop the .npu2_device_detect handler ?
> int npu2_opencapi_init_npu(struct npu2 *npu);
> int npu2_nvlink_init_npu(struct npu2 *npu);
> void npu2_nvlink_create_phb(struct npu2 *npu, struct dt_node *dn);
> @@ -241,7 +247,7 @@ int64_t npu2_freeze_status(struct phb *phb __unused,
> uint8_t *freeze_state,
> uint16_t *pci_error_type __unused,
> uint16_t *severity __unused);
> -void npu2_dump_scoms(struct npu2 *npu, int chip_id);
> +void npu2_dump_scoms(struct phb *phb, int chip_id);
>
> int64_t npu2_init_context(struct phb *phb, uint64_t msr, uint64_t bdf);
> int64_t npu2_destroy_context(struct phb *phb, uint64_t bdf);
> diff --git a/include/pci.h b/include/pci.h
> index 101442490..46d3d0ace 100644
> --- a/include/pci.h
> +++ b/include/pci.h
> @@ -343,6 +343,9 @@ struct phb_ops {
>
> /* Currently only used by NPU HMI code */
> void (*set_fence_state)(struct phb *phb, bool fence);
> +
> + /* The most terrible of situtions, dump debug data to console. */
> + void (*dump_debug_data)(struct phb *phb, int flat_chip_id);
> };
>
> enum phb_type {
> diff --git a/platforms/astbmc/Makefile.inc b/platforms/astbmc/Makefile.inc
> index 1cdf37f2a..be2267d3f 100644
> --- a/platforms/astbmc/Makefile.inc
> +++ b/platforms/astbmc/Makefile.inc
> @@ -1,16 +1,23 @@
> SUBDIRS += $(PLATDIR)/astbmc
>
> ASTBMC_OBJS = pnor.o common.o slots.o \
> - witherspoon.o zaius.o romulus.o p9dsu.o \
> - nicole.o mihawk.o mowgli.o \
> + witherspoon.o romulus.o p9dsu.o \
> + nicole.o mowgli.o \
> talos.o blackbird.o \
> - swift.o rainier.o
> + rainier.o
> +
> +ifeq ($(CONFIG_NPU),1)
> +ASTBMC_OBJS += zaius.o mihawk.o swift.o
> +endif
Ah ! this is not disabling the NPU support but support for whole platforms.
Hmm, this is not exactly what was expected.
Thanks,
C.
> ifeq ($(CONFIG_P8),1)
> ASTBMC_OBJS += palmetto.o habanero.o firestone.o \
> p8dtu.o p8dnu.o \
> - garrison.o barreleye.o \
> + barreleye.o \
> vesnin.o
> +ifeq ($(CONFIG_NPU),1)
> +ASTBMC_OBJS += garrison.o
> +endif
> endif
>
> ASTBMC = $(PLATDIR)/astbmc/built-in.a
>
More information about the Skiboot
mailing list