Skip to content

Commit

Permalink
NVDIMM: Fix issue where a deviceRead of a register is returning bad data
Browse files Browse the repository at this point in the history 
The reason the deviceRead is returning bad data, from an NVDIMM register,
is because the register data is segmented across several pages.  One must
first set the page correctly before attempting to do a read of a register.
This fix will set the page up correctly before doing a read/write to an
NVDIMM register.

Change-Id: Ic63c8671c0083f1ca243f255a29d1ef4606b7ce5
CQ: SW465917
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/81542
Tested-by: Jenkins Server <pfd-jenkins+hostboot@us.ibm.com>
Tested-by: FSP CI Jenkins <fsp-CI-jenkins+hostboot@us.ibm.com>
Reviewed-by: Matthew Raybuck <matthew.raybuck@ibm.com>
Tested-by: Jenkins OP Build CI <op-jenkins+hostboot@us.ibm.com>
Tested-by: Jenkins OP HW <op-hw-jenkins+hostboot@us.ibm.com>
Reviewed-by: Matt Derksen <mderkse1@us.ibm.com>
Reviewed-by: Daniel M Crowell <dcrowell@us.ibm.com>
  • Loading branch information
@velozr @dcrowell77
velozr authored and dcrowell77 committed Aug 9, 2019
1 parent 86e12de commit 7e50071
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 7 deletions.
25 changes: 22 additions & 3 deletions src/include/usr/devicefw/userif.H
View file
Expand Up @@ -68,7 +68,8 @@ namespace DeviceFW
AHB_SIO, // AST Hostbridge via SIO
DVPD, // Direct access memory VPD
NODECOMM, // Internode communication
NVDIMM, // Non-volatile DIMM controller access
NVDIMM, // Routes message to non-volatile DIMM controller access
NVDIMM_RAW, // Non-volatile DIMM controller access
FAPI_I2C, // FAPI2-triggered i2c accesses
MMIO, // Memory Mapped I/O
IDEC, // Read and set EC and CHIPID values
Expand Down Expand Up @@ -379,12 +380,30 @@ namespace DeviceFW
static_cast<uint64_t>(( i_mailbox_id ))

/**
* Construct the device addressing parameters for the NVDIMM device ops.
* @param[i] i_address - NVDIMM address to internal register
* @brief Construct the device addressing parameters for the
* NVDIMM device ops.
* @details This call includes setting the page based off the address
* and then performing the read/write of that NVDIMM address.
* @see DEVICE_NVDIMM_RAW_ADDRESS for an NVDIMM read/write call without
* page setting.
* @param[i] i_address - NVDIMM address to an internal register
*/
#define DEVICE_NVDIMM_ADDRESS(i_address)\
DeviceFW::NVDIMM, static_cast<uint64_t>((i_address))

/**
* @brief Construct the device addressing parameters for the
* NVDIMM RAW device ops.
* @details This a raw call to read/write a NVDIMM address which means it
* will not set the page before it does the read/write call. Hence,
* for this call to work properly, the page must have been set
* properly beforehand.
* @see DEVICE_NVDIMM_ADDRESS for a NVDIMM read/write call with
* page setting.
* @param[i] i_address - NVDIMM address to an internal register
*/
#define DEVICE_NVDIMM_RAW_ADDRESS(i_address)\
DeviceFW::NVDIMM_RAW, static_cast<uint64_t>((i_address))

/**
* Construct the device addressing parameters for the FAPI I2C operation
Expand Down
4 changes: 2 additions & 2 deletions src/usr/isteps/nvdimm/nvdimm.C
View file
Expand Up @@ -264,7 +264,7 @@ errlHndl_t nvdimmReadReg(Target* i_nvdimm,
i_nvdimm,
&o_data,
l_numBytes,
DEVICE_NVDIMM_ADDRESS(l_reg_addr));
DEVICE_NVDIMM_RAW_ADDRESS(l_reg_addr));
}while(0);

TRACUCOMP(g_trac_nvdimm, EXIT_MRK"NVDIMM Read HUID 0x%X, page 0x%X, addr 0x%X = 0x%X",
Expand Down Expand Up @@ -334,7 +334,7 @@ errlHndl_t nvdimmWriteReg(Target* i_nvdimm,
i_nvdimm,
&i_data,
l_numBytes,
DEVICE_NVDIMM_ADDRESS(l_reg_addr));
DEVICE_NVDIMM_RAW_ADDRESS(l_reg_addr));
}while(0);

TRACUCOMP(g_trac_nvdimm, EXIT_MRK"NVDIMM Write HUID 0x%X, page = 0x%X, addr 0x%X = 0x%X",
Expand Down
2 changes: 1 addition & 1 deletion src/usr/isteps/nvdimm/nvdimm_update.C
View file
Expand Up @@ -1432,7 +1432,7 @@ errlHndl_t NvdimmInstalledImage::byteRegionBlockTransfer(const uint8_t * i_data,
iv_dimm,
pCurrentBlockData,
l_numBytes,
DEVICE_NVDIMM_ADDRESS(l_reg_addr) );
DEVICE_NVDIMM_RAW_ADDRESS(l_reg_addr) );
if (l_err)
{
TRACFCOMP(g_trac_nvdimm_upd, ERR_MRK"byteRegionBlockTransfer: "
Expand Down
72 changes: 71 additions & 1 deletion src/usr/isteps/nvdimm/nvdimmdd.C
View file
Expand Up @@ -97,12 +97,82 @@ static bool errorIsRetryable(uint16_t reasonCode)
namespace NVDIMM
{

// Register the perform Op with the routing code for DIMMs.
// Register the perform Op router with the routing code for DIMMs.
DEVICE_REGISTER_ROUTE( DeviceFW::WILDCARD,
DeviceFW::NVDIMM,
TARGETING::TYPE_DIMM,
nvdimmPerformOpRouter );

// Register the perform Op with the routing code for DIMMs.
DEVICE_REGISTER_ROUTE( DeviceFW::WILDCARD,
DeviceFW::NVDIMM_RAW,
TARGETING::TYPE_DIMM,
nvdimmPerformOp );

// ------------------------------------------------------------------
// nvdimmPerformOpRouter
// ------------------------------------------------------------------
errlHndl_t nvdimmPerformOpRouter( DeviceFW::OperationType i_opType,
TARGETING::Target * i_target,
void * io_buffer,
size_t & io_buflen,
int64_t i_accessType,
va_list i_args )
{
errlHndl_t l_err(nullptr);

TRACDCOMP( g_trac_nvdimm,
ENTER_MRK"nvdimmPerformOpRouter()" );

// Get the NVDIMM register's address, where data will be accessed from
// Although the data is being retrieved as a 64 bit value
// it is really a 16 bit value. Data passed via an arg list
// are retrieved in 64 bit chunks.
uint16_t l_registerAddress =
static_cast<uint16_t>(va_arg(i_args, uint64_t));

// Get a handle to the data buffer for easy referencing
uint8_t* l_data = static_cast<uint8_t*>(io_buffer);

TRACUCOMP(g_trac_nvdimm, INFO_MRK"nvdimmPerformOpRouter(): "
"operation type=%d, target HUID=0x%.8X, access type=%d, "
"buffer length=%d, buffer data=0x%.8X, register address=0x%.8X",
static_cast<uint64_t>(i_opType), get_huid(i_target), i_accessType,
io_buflen, *l_data, l_registerAddress);

// Make the right read/write call based on operation type
if( i_opType == DeviceFW::READ )
{
l_err = nvdimmReadReg( i_target,
l_registerAddress,
*l_data,
PAGE_VERIFY);
if (!l_err)
{
TRACUCOMP (g_trac_nvdimm, INFO_MRK"nvdimmPerformOpRouter(): "
"Read data(0x%X) from register(0x%X)",
*l_data, l_registerAddress);
}
}
else if( i_opType == DeviceFW::WRITE )
{
TRACUCOMP (g_trac_nvdimm, INFO_MRK"nvdimmPerformOpRouter(): "
"Writing data(0x%X) to register(0x%X) ...",
*l_data, l_registerAddress);

l_err = nvdimmWriteReg( i_target,
l_registerAddress,
*l_data,
PAGE_VERIFY);
}

TRACDCOMP(g_trac_nvdimm,
EXIT_MRK"nvdimmPerformOpRouter() returning with %s",
(l_err == nullptr ? "no error, success" : "an error, failure") );

return l_err;
}

// ------------------------------------------------------------------
// nvdimmPerformOp
// ------------------------------------------------------------------
Expand Down
43 changes: 43 additions & 0 deletions src/usr/isteps/nvdimm/nvdimmdd.H
View file
Expand Up @@ -138,6 +138,49 @@ errlHndl_t nvdimmPerformOp( DeviceFW::OperationType i_opType,
int64_t i_accessType,
va_list i_args );

/**
*
* @brief Route the read/write operator (i_opType) to the correct
* nvdimmReadReg/nvdimmWriteReg call.
*
* @details This is essentially a wrapper around the nvdimmPerformOp method
* which is called via the nvdimmReadReg/nvdimmWriteReg call. This
* ensures that the page is set correctly whenever a NVDIMM register
* is accessed.
*
* @param[in] i_opType - Operation Type - See DeviceFW::OperationType in
* driververif.H
*
* @param[in] i_target - Target device.
*
* @param[in/out] io_buffer
* INPUT: Pointer to the data that will be written to the target
* device.
* OUTPUT: Pointer to the data that was read from the target device.
*
* @param[in/out] io_buflen
* INPUT: Length of the buffer to be written to target device.
* OUTPUT: Length of buffer that was written, or length of buffer
* to be read from target device.
*
* @param [in] i_accessType - Access Type - See DeviceFW::AccessType in
* usrif.H
*
* @param [in] i_args - This is an argument list for the device driver
* framework. This argument list consists of the internal offset
* to use on the slave I2C device.
*
* @return errlHndl_t - NULL if successful, otherwise a pointer to the
* error log.
*
*/
errlHndl_t nvdimmPerformOpRouter( DeviceFW::OperationType i_opType,
TARGETING::Target * i_target,
void * io_buffer,
size_t & io_buflen,
int64_t i_accessType,
va_list i_args );

/*
* @brief On the NV Controller, the page is selected by writing to offset
* 0x00 with the page you would like to switch too. e.g. to activate
Expand Down

0 comments on commit 7e50071

Please sign in to comment.