[Skiboot] [PATCH v3 2/6] VAS: Initialize the basic VAS internal registers
Sukadev Bhattiprolu
sukadev at linux.vnet.ibm.com
Fri Dec 2 06:21:18 AEDT 2016
Balbir Singh [bsingharora at gmail.com] wrote:
>
>
> On 30/11/16 16:36, Sukadev Bhattiprolu wrote:
> > Initialize the basic VAS internal registers that are needed for a
> > functioning VAS. Initializing VAS involves writing either pre-defined
> > values or allocated addresses to appropriate SCOM addresses.
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev at linux.vnet.ibm.com>
> > ---
> > Changelog [v3]
> > - [Oliver O'Halloran] Fold changes to vas.c and Makefile.inc from
> > patch 1 into current patch; use constant 'true' for the
> > (unnecessary) macros that were removed by earlier patch;
> > free wcbs memory if any chip fails initialization;use
> > prolog()/prerror() instead of printf; Use out_be64() to
> > write to the MMIO address
> > - [Oliver O'Halloran, Alistair Popple] Use proc_gen to check for P9
> > - Move vas_vdbg() macro from later patch into this.
> > ---
> > core/Makefile.inc | 2 +-
> > core/init.c | 4 +
> > core/vas.c | 297 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/chip.h | 2 +
> > 4 files changed, 304 insertions(+), 1 deletion(-)
> > create mode 100644 core/vas.c
> >
> > diff --git a/core/Makefile.inc b/core/Makefile.inc
> > index 9223af1..f41617d 100644
> > --- a/core/Makefile.inc
> > +++ b/core/Makefile.inc
> > @@ -8,7 +8,7 @@ CORE_OBJS += pci-opal.o fast-reboot.o device.o exceptions.o trace.o affinity.o
> > CORE_OBJS += vpd.o hostservices.o platform.o nvram.o nvram-format.o hmi.o
> > CORE_OBJS += console-log.o ipmi.o time-utils.o pel.o pool.o errorlog.o
> > CORE_OBJS += timer.o i2c.o rtc.o flash.o sensor.o ipmi-opal.o
> > -CORE_OBJS += flash-subpartition.o
> > +CORE_OBJS += flash-subpartition.o vas.o
> >
> > ifeq ($(SKIBOOT_GCOV),1)
> > CORE_OBJS += gcov-profiling.o
> > diff --git a/core/init.c b/core/init.c
> > index 9d4ab60..2af5427 100644
> > --- a/core/init.c
> > +++ b/core/init.c
> > @@ -45,6 +45,7 @@
> > #include <sensor.h>
> > #include <xive.h>
> > #include <nvram.h>
> > +#include <vas.h>
> > #include <libstb/stb.h>
> > #include <libstb/container.h>
> >
> > @@ -909,6 +910,9 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
> > phb3_preload_capp_ucode();
> > start_preload_kernel();
> >
> > + /* Virtual Accelerator Switchboard */
> > + init_vas();
>
> vas_init() -- along the lines of nx_init(). I think this will end up
> affecting a lot of functions that follow
Will change to vas_init() but not sure what you mean by "affecting lot of
functions"
>
> > +
> > /* NX init */
> > nx_init();
> >
> > diff --git a/core/vas.c b/core/vas.c
> > new file mode 100644
> > index 0000000..2b3d8dd
> > --- /dev/null
> > +++ b/core/vas.c
> > @@ -0,0 +1,297 @@
> > +/* Copyright 2013-2016 IBM Corp.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > + * implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +#include <skiboot.h>
> > +#include <chip.h>
> > +#include <xscom.h>
> > +#include <io.h>
> > +#include <vas.h>
> > +
> > +#undef VAS_VERBOSE_DEBUG
> > +#ifdef VAS_VERBOSE_DEBUG
> > +#define vas_vdbg(__x,__fmt,...) prlog(PR_DEBUG,"VAS: " __fmt, ##__VA_ARGS)
> > +#else
> > +#define vas_vdbg(__x,__fmt,...) do { } while (0)
> > +#endif
> > +
> > +/*
> > + * TODO: Set to 64K after initial development.
> > + */
> > +static int max_win_per_chip = 16;
> > +static int vas_initialized;
> > +
> > +static uint64_t get_hvwc_mmio_bar(const int chipid)
> > +{
> > + return VAS_HVWC_MMIO_BAR_BASE + chipid * VAS_HVWC_MMIO_BAR_SIZE;
> > +}
> > +
> > +static uint64_t get_uwc_mmio_bar(int chipid)
> > +{
> > + return VAS_UWC_MMIO_BAR_BASE + chipid * VAS_UWC_MMIO_BAR_SIZE;
> > +}
> > +
>
> Is there a reason these are not inline?
Will make them inline.
>
> > +static inline uint64_t compute_vas_scom_addr(uint64_t ring_sat_offset)
> > +{
> > + return VAS_SCOM_BASE_ADDR + ring_sat_offset;
> > +}
> > +
> > +static int vas_scom_write(struct proc_chip *chip, uint64_t reg, uint64_t val)
> > +{
> > + return xscom_write(chip->id, compute_vas_scom_addr(reg), val);
> > +}
> > +
> > +static inline int vas_scom_read(struct proc_chip *chip, uint64_t reg,
> > + uint64_t *val)
> > +{
> > + return xscom_read(chip->id, compute_vas_scom_addr(reg), val);
> > +}
> > +
> > +static int init_north_ctl(struct proc_chip *chip)
> > +{
>
> north_ctl -- north_ctrl?
I have been trying to use ctl consistently.
>
> > + uint64_t val = 0ULL;
> > +
> > + val = SETFIELD(VAS_64K_MODE_MASK, val, true);
> > + val = SETFIELD(VAS_ACCEPT_PASTE_MASK, val, true);
> > +
> > + return vas_scom_write(chip, VAS_MISC_N_CTL, val);
> > +}
> > +
> > +static int reset_north_ctl(struct proc_chip *chip)
> > +{
> > + return vas_scom_write(chip, VAS_MISC_N_CTL, 0ULL);
>
> I think this may be incorrect. Ideally we want to set it to
> a value that we read at init time from VAS_MISC_N_CTRL register.
> I am saying this because I see some reserved fields in the
> register and I am not sure we can write 0's to them on reset
Confirmed with the hardware guys that writing 0s to the reserved bits
does not matter.
>
> > +}
> > +
> > +static void reset_fir(struct proc_chip *chip)
> > +{
> > + uint64_t val;
> > +
> > + val = 0x0ULL;
> > + vas_scom_write(chip, VAS_FIR0, val);
> > + vas_scom_write(chip, VAS_FIR1, val);
> > + vas_scom_write(chip, VAS_FIR2, val);
> > + vas_scom_write(chip, VAS_FIR3, val);
> > + vas_scom_write(chip, VAS_FIR4, val);
> > + vas_scom_write(chip, VAS_FIR5, val);
> > + vas_scom_write(chip, VAS_FIR6, val);
> > + vas_scom_write(chip, VAS_FIR7, val);
> > +}
> > +
> > +/*
> > + * Initialize RMA BAR and RMA Base Address Mask Register (BAMR) to their
> > + * default values. This will cause VAS to accept paste commands to all
> > + * chips in the system.
> > + */
> > +static int init_rma(struct proc_chip *chip)
> > +{
> > + int rc;
> > + uint64_t val;
> > +
> > + val = 0ULL;
> > + val = SETFIELD(VAS_RMA_BAR_ADDR_MASK, val, 0x08000000000ULL);
> > +
>
> Isn't it simpler to say
> val = SETFIELD(VAS_RMA_BAR_ADDR_MASK, 0ULL, 0x08000000000ULL);
Sure
>
> If I am reading the document correctly, this is also the reset value,
> but I presume this function assumes that we can write to VAS_RMA_BAR_ADDR_MASK
> more than once after init? Don't we also need to write the chipid bits
> in the mask?
The RMA_BAR and RMA_BAMR can be used to enable VAS on a subset of the chips.
If we are allowing access to all instances of VAS, I don't think we need to
write the chip ids.
>
> > + rc = vas_scom_write(chip, VAS_RMA_BAR, val);
> > + if (rc)
> > + return rc;
> > +
> > + val = 0ULL;
> > + val = SETFIELD(VAS_RMA_BAMR_ADDR_MASK, val, 0xFFFC0000000ULL);
> > +
> Ditto
>
> > + rc = vas_scom_write(chip, VAS_RMA_BAMR, val);
> Same as above.
>
> > +
> > + return rc;
> > +}
> > +
> > +/*
> > + * Window Context MMIO (WCM) Region for each chip is assigned in the P9
> > + * MMIO MAP spreadsheet. Write this value to the SCOM address associated
> > + * with WCM_BAR.
> > + */
> > +static int init_wcm(struct proc_chip *chip)
> > +{
> > + uint64_t wcmbar;
> > +
> > + wcmbar = get_hvwc_mmio_bar(chip->id);
> gr> +
> > + /*
> > + * Write the entire WCMBAR address to the SCOM address. VAS will
> > + * extract bits that it thinks are relevant i.e bits 8..38
> > + */
> > + return vas_scom_write(chip, VAS_WCM_BAR, wcmbar);
> > +}
> > +
> > +/*
> > + * OS/User Window Context MMIO (UWCM) Region for each is assigned in the
> > + * P9 MMIO MAP spreadsheet. Write this value to the SCOM address associated
> > + * with UWCM_BAR.
> > + */
> > +static int init_uwcm(struct proc_chip *chip)
> > +{
> > + uint64_t uwcmbar;
> > +
> > + uwcmbar = get_uwc_mmio_bar(chip->id);
> > +
> > + /*
> > + * Write the entire UWCMBAR address to the SCOM address. VAS will
> > + * extract bits that it thinks are relevant i.e bits 8..35.
> > + */
> > + return vas_scom_write(chip, VAS_UWCM_BAR, uwcmbar);
> > +}
> > +
> > +static inline void free_wcbs(struct proc_chip *chip)
> > +{
> > + free((void *)chip->vas_wcbs);
> > +}
> > +
> > +/*
> > + * VAS needs a backing store for the 64K window contexts on a chip.
> > + * (64K times 512 = 8MB). This region needs to be contiguous, so
> > + * allocate during early boot. Then write the allocated address to
> > + * the SCOM address for the Backing store BAR.
> > + */
> > +static int alloc_init_wcbs(struct proc_chip *chip)
> > +{
> > + int rc;
> > + uint64_t wcbs;
> > + size_t size;
> > +
> > + /* align to the backing store size */
> > + size = (size_t)VAS_WCBS_SIZE;
> > + wcbs = (uint64_t)local_alloc(chip->id, size, size);
>
> Should this be zalloc? so that the window is seen as closed?
Guess it doesn't hurt, but we always initialize all the valid registers in
the context before allowing user space to access the window (we must do
that anyway since the window could have been released by another process).
>
> > + if (!wcbs) {
> > + prerror("VAS: Unable to allocate memory for backing store\n");
>
> At this point is it possible to use lesster than 512 64K windows and retry?
Possibly, but how lower should we go? One of the reasons to allocate
memory here is that we would have it available. Another problem is if
we get 64K windows on one chip and 16K windows on another, we have to
do more bookkeeping to keep track of which chip has how many windows?
Thanks again for the detailed review.
Sukadev
More information about the Skiboot
mailing list