Default to enable all clock outputs
In the current code flow, as and when Entity manager detects the
HSBP config, and the hardware is initialized, the Clock Buffers are
set to a default value, which is to disable all outputs. Even the IO
controller is initialized to disable the clock output.
This approach leads to two issues
1. The code assumes HSBP is the only consumer of the MCIO port. In the
event of attaching any other card to the MCIO port, the clock will be
disabled by default which is incorrect.
2. During the scenario when bmc goes for reboot, or even if
entity-manager service reloads, the clock buffers are momentorily
initialized to disable clocks until the code finds that there are
drives connected. This leads to an issue when, HOST is accessing the
drive and clock buffers are re-initialized, the HOST loses the control
over the drive for that moment.
This commit fixes both of these issues by defaulting the clocks to be
enabled, and upon detection of HSBP, determine the drives connected and
accordingly disable clocks which are not required.
TESTED:
- Restarted Entity manager to simulate the bmc reboot scenario and
verified the Clock buffer status
- With older code, the buffer value was reset and then
re-initialized
- With the fix implemented, the buffer retained its value.
- On removing/inserting a drive, the corresponding clock was
enabled/disabled
Change-Id: I2e85fb50de33ba763a6ea05076fb99d53d90d859
Signed-off-by: Rashmi R V <rashmi.r.v@intel.com>
diff --git a/hsbp-manager/src/hsbp_manager.cpp b/hsbp-manager/src/hsbp_manager.cpp
index 4f3f3c5..6e22d2d 100644
--- a/hsbp-manager/src/hsbp_manager.cpp
+++ b/hsbp-manager/src/hsbp_manager.cpp
@@ -120,9 +120,9 @@
{
/* Execute below operation only when mode of operation is SMBus. By
* default the clock buffer is configured to follow OE pin output, so we
- * need to set the output value to 0 to disable the clock outputs. If
- * mode of operation is IO, then the IO value will determine the
- * disable/enable of clock output */
+ * need to set the output value to 0 to disable the clock outputs and 1
+ * to enable clock output. If mode of operation is IO, then the IO value
+ * will determine the disable/enable of clock output */
if (modeOfOperation == "SMBus")
{
if (file < 0)
@@ -172,7 +172,7 @@
std::bitset<8> currByte(read);
bool writeRequired = false;
- /* Set zero only at bit position that we have a NVMe drive (i.e.
+ /* Set 0/1 only at bit position that we have a NVMe drive (i.e.
* ignore where byteMap is "-"). We do not want to touch other
* bits */
for (uint8_t bit = 0; bit < 8; bit++)
@@ -180,7 +180,12 @@
if (byte->second.at(bit) != "-")
{
writeRequired = true;
- currByte.reset(bit);
+ /* Default to enabling the clock output and once the
+ * HSBP's are detected the clocks will be
+ * enabled/disabled depending on the drive status */
+ /* TODO: This code might require a re-visit in case of
+ * any signal integrity issues in the future */
+ currByte.set(bit);
}
}
@@ -347,7 +352,7 @@
void initialize()
{
/* Initialize the IO expander Control register to configure the IO ports
- * as outputs and set the output to low by default */
+ * as outputs and set the output*/
if (file < 0)
{
file = open(("/dev/i2c-" + std::to_string(bus)).c_str(),
@@ -406,7 +411,7 @@
std::bitset<8> currCtrlVal(read1);
std::bitset<8> currOutVal(read2);
- /* Set zero only at bit position that we have a NVMe drive (i.e.
+ /* Set 0/1 only at bit position that we have a NVMe drive (i.e.
* ignore where ioMap is "-"). We do not want to touch other
* bits */
for (uint8_t bit = 0; bit < 8; bit++)
@@ -415,7 +420,11 @@
{
writeRequired = true;
currCtrlVal.reset(bit);
- currOutVal.reset(bit);
+ /* Set the output register to drive the OE pin high thereby
+ * enabling the clock. Default to enabling the clock output
+ * and once the HSBP's are detected the clocks will be
+ * enabled/disabled depending on the drive status */
+ currOutVal.set(bit);
}
}