From 7e5007180bf32c840af3da04d90a398eb02099dd Mon Sep 17 00:00:00 2001 From: Roland Veloz Date: Thu, 1 Aug 2019 15:20:55 -0500 Subject: [PATCH] NVDIMM: Fix issue where a deviceRead of a register is returning bad data 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 Tested-by: FSP CI Jenkins Reviewed-by: Matthew Raybuck Tested-by: Jenkins OP Build CI Tested-by: Jenkins OP HW Reviewed-by: Matt Derksen Reviewed-by: Daniel M Crowell --- src/include/usr/devicefw/userif.H | 25 ++++++++-- src/usr/isteps/nvdimm/nvdimm.C | 4 +- src/usr/isteps/nvdimm/nvdimm_update.C | 2 +- src/usr/isteps/nvdimm/nvdimmdd.C | 72 ++++++++++++++++++++++++++- src/usr/isteps/nvdimm/nvdimmdd.H | 43 ++++++++++++++++ 5 files changed, 139 insertions(+), 7 deletions(-) diff --git a/src/include/usr/devicefw/userif.H b/src/include/usr/devicefw/userif.H index d626d49c025..7725115664b 100644 --- a/src/include/usr/devicefw/userif.H +++ b/src/include/usr/devicefw/userif.H @@ -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 @@ -379,12 +380,30 @@ namespace DeviceFW static_cast(( 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((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((i_address)) /** * Construct the device addressing parameters for the FAPI I2C operation diff --git a/src/usr/isteps/nvdimm/nvdimm.C b/src/usr/isteps/nvdimm/nvdimm.C index 852a65a88c9..db26eb18414 100644 --- a/src/usr/isteps/nvdimm/nvdimm.C +++ b/src/usr/isteps/nvdimm/nvdimm.C @@ -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", @@ -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", diff --git a/src/usr/isteps/nvdimm/nvdimm_update.C b/src/usr/isteps/nvdimm/nvdimm_update.C index 3a732ef663a..1f42228ced0 100644 --- a/src/usr/isteps/nvdimm/nvdimm_update.C +++ b/src/usr/isteps/nvdimm/nvdimm_update.C @@ -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: " diff --git a/src/usr/isteps/nvdimm/nvdimmdd.C b/src/usr/isteps/nvdimm/nvdimmdd.C index b2c8909d333..695a60b9346 100755 --- a/src/usr/isteps/nvdimm/nvdimmdd.C +++ b/src/usr/isteps/nvdimm/nvdimmdd.C @@ -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(va_arg(i_args, uint64_t)); + + // Get a handle to the data buffer for easy referencing + uint8_t* l_data = static_cast(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(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 // ------------------------------------------------------------------ diff --git a/src/usr/isteps/nvdimm/nvdimmdd.H b/src/usr/isteps/nvdimm/nvdimmdd.H index 4d599b38a18..1e299f2deac 100755 --- a/src/usr/isteps/nvdimm/nvdimmdd.H +++ b/src/usr/isteps/nvdimm/nvdimmdd.H @@ -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