Change p9_mss_freq_system to write attributes, errors for Cronus

  Honor the maximum support frequencies based on rank configs
  Remove the MEMVPD_FREQ attribute
  Fixup VPD tooling and accessors for new freq attrs
  Fix test case handling of master ranks
  Fix handling of empty MCS in Cronus

Change-Id: I669ad0e81454f12368b484e826461ee76f7b9079
Reviewed-on: http://ralgit01.raleigh.ibm.com/gerrit1/29878
Reviewed-by: Daniel M. Crowell <dcrowell@us.ibm.com>
Tested-by: Jenkins Server <pfd-jenkins+hostboot@us.ibm.com>
Tested-by: PPE CI <ppe-ci+hostboot@us.ibm.com>
Reviewed-by: ANDRE A. MARIN <aamarin@us.ibm.com>
Reviewed-by: Matt K. Light <mklight@us.ibm.com>
Tested-by: Hostboot CI <hostboot-ci+hostboot@us.ibm.com>
Reviewed-by: Jennifer A. Stofer <stofer@us.ibm.com>
Reviewed-on: http://ralgit01.raleigh.ibm.com/gerrit1/29907
Tested-by: FSP CI Jenkins <fsp-CI-jenkins+hostboot@us.ibm.com>
Reviewed-by: Christian R. Geddes <crgeddes@us.ibm.com>

4 files changed, 356 insertions, 77 deletions

diff --git a/src/import/chips/p9/procedures/hwp/memory/lib/freq/cas_latency.C b/src/import/chips/p9/procedures/hwp/memory/lib/freq/cas_latency.C
index ce92c500a..07e4ca4d2 100644
--- a/src/import/chips/p9/procedures/hwp/memory/lib/freq/cas_latency.C
+++ b/src/import/chips/p9/procedures/hwp/memory/lib/freq/cas_latency.C
@@ -66,17 +66,17 @@ namespace mss
 /// @param[in]  i_caches decoder caches
 ///
 cas_latency::cas_latency(const fapi2::Target<fapi2::TARGET_TYPE_MCS>& i_target,
-                         const std::map<uint32_t, std::shared_ptr<spd::decoder> >& i_caches)
+                         const std::map<uint32_t, std::shared_ptr<spd::decoder> >& i_caches):
+    iv_dimm_list_empty(false),
+    iv_largest_taamin(0),
+    iv_proposed_tck(0),
+    iv_common_CL(UINT64_MAX) // Masks out supported CLs
 {
-    iv_dimm_list_empty = false;
-    iv_common_CL = UINT64_MAX; // Masks out supported CLs
-    iv_largest_taamin = 0;
-    iv_proposed_tck = 0;
-
     const auto l_dimm_list = find_targets<TARGET_TYPE_DIMM>(i_target);
 
     if(l_dimm_list.empty())
     {
+        FAPI_INF("cas latency ctor seeing no DIMM on %s", mss::c_str(i_target));
         iv_dimm_list_empty = true;
         return;
     }
diff --git a/src/import/chips/p9/procedures/hwp/memory/lib/freq/cycle_time.H b/src/import/chips/p9/procedures/hwp/memory/lib/freq/cycle_time.H
index 860cf3fca..2352d14df 100644
--- a/src/import/chips/p9/procedures/hwp/memory/lib/freq/cycle_time.H
+++ b/src/import/chips/p9/procedures/hwp/memory/lib/freq/cycle_time.H
@@ -22,6 +22,7 @@
 /* permissions and limitations under the License.                         */
 /*                                                                        */
 /* IBM_PROLOG_END_TAG                                                     */
+
 ///
 /// @file cycle_time.H
 /// @brief cycle time (tCK) data & functions needed for CAS latency algorithm
@@ -37,19 +38,11 @@
 
 #include <fapi2.H>
 #include <memory>
+#include <lib/freq/sync.H>
 
 namespace mss
 {
 
-enum FREQ_LIMITS
-{
-    // Can't add these limits in xml because
-    // Cronus won't know what ENUM to print out
-    // if there are two enums for the same value.
-    MAX_SUPPORTED_FREQ = fapi2::ENUM_ATTR_MSS_FREQ_MT2666,
-    MIN_SUPPORTED_FREQ = fapi2::ENUM_ATTR_MSS_FREQ_MT1866
-};
-
 enum FREQ_MARGIN : std::uint64_t
 {
     // -5% of 1866 MT/s
@@ -59,17 +52,6 @@ enum FREQ_MARGIN : std::uint64_t
     MAX_FREQ_MARGIN = 2799,
 };
 
-static const std::vector<uint64_t> P9_FREQS
-{
-    // P9 supported frequencies in addition to
-    // TK - How make sure thes vector is always updated if enums are changed?
-    fapi2::ENUM_ATTR_MSS_FREQ_MT1866,
-    fapi2::ENUM_ATTR_MSS_FREQ_MT2133,
-    fapi2::ENUM_ATTR_MSS_FREQ_MT2400,
-    fapi2::ENUM_ATTR_MSS_FREQ_MT2666,
-};
-
-
 // Proposed DDR4 Full spec update(79-4A)
 // Item No. 1716.78C
 // Page 217
@@ -86,7 +68,10 @@ static const std::vector<uint64_t> tckmin_avg_ddr4
     /* 1250, // 1600 MT/s - don't think we support this one - AAM */
 };
 
+///
 /// @brief          Selects DIMM frequency to run based on supported system frequencies
+/// @tparam         T the fapi2::TargetType for which the DIMM information exists.
+/// @param[in]      target from which to get the dimm information
 /// @param[in,out]  io_dimm_freq input is current dimm freq & output is next
 ///                 lowest dimm freq between 1866 MT/s (-5% margin)  - 2666 MT/s (+5% margin)
 /// @return         FAPI2_RC_SUCCESS iff ok
@@ -94,8 +79,13 @@ static const std::vector<uint64_t> tckmin_avg_ddr4
 /// @note           From P9 User's Manual Version 0.8 - June 08, 2015
 /// @note           Memory Controller Unit - page 199 of 456
 ///
-inline fapi2::ReturnCode select_supported_freq(uint64_t& io_dimm_freq)
+template< fapi2::TargetType T >
+inline fapi2::ReturnCode select_supported_freq(const fapi2::Target<T>& i_target, uint64_t& io_dimm_freq)
 {
+    uint32_t l_min_freq = 0;
+    uint32_t l_max_freq = 0;
+    std::vector<uint32_t> l_freqs;
+
     //TK - This 5% margin was done in P8, continue for P9 ?? - AAMARIN
 
     // Speed bins for P9 DDR4 are 1866 MT/s -  2666 MT/s
@@ -108,25 +98,31 @@ inline fapi2::ReturnCode select_supported_freq(uint64_t& io_dimm_freq)
                  "Unsupported frequency: %d.  DIMM Freq calculated is < 1773 or > 2799 MT/s",
                  io_dimm_freq);
 
+    FAPI_TRY( mss::supported_freqs(i_target, l_freqs) );
+    l_min_freq = *(l_freqs.begin());
+    l_max_freq = *(l_freqs.end() - 1);
+
     // Corner case to take into account -5% margin
-    if(io_dimm_freq  < MIN_SUPPORTED_FREQ)
+    if(io_dimm_freq  < l_min_freq)
     {
-        io_dimm_freq = MIN_SUPPORTED_FREQ;
+        io_dimm_freq = l_min_freq;
         return fapi2::FAPI2_RC_SUCCESS;
     }
 
     // Corner case to take into account +5% margin
-    if(io_dimm_freq > MAX_SUPPORTED_FREQ)
+    if(io_dimm_freq > l_max_freq)
     {
-        io_dimm_freq = MAX_SUPPORTED_FREQ;
+        io_dimm_freq = l_max_freq;
         return fapi2::FAPI2_RC_SUCCESS;
     }
 
     // Selects next lowest freq btw 1866 - 2666 [MT/s]
     {
-        auto iterator = std::upper_bound(P9_FREQS.begin(), P9_FREQS.end(), io_dimm_freq);
+        // supported_freqs sorts so upper_bound is valid.
+        auto iterator = std::upper_bound(l_freqs.begin(), l_freqs.end(), io_dimm_freq);
         io_dimm_freq = *(--iterator);
     }
+
 fapi_try_exit:
     return fapi2::current_err;
 }
diff --git a/src/import/chips/p9/procedures/hwp/memory/lib/freq/sync.C b/src/import/chips/p9/procedures/hwp/memory/lib/freq/sync.C
index c3acf1070..eb1cb3661 100644
--- a/src/import/chips/p9/procedures/hwp/memory/lib/freq/sync.C
+++ b/src/import/chips/p9/procedures/hwp/memory/lib/freq/sync.C
@@ -32,6 +32,7 @@
 // *HWP Team: Memory
 // *HWP Level: 2
 // *HWP Consumed by: HB:FSP
+
 #include  <vector>
 #include <map>
 
@@ -39,15 +40,23 @@
 #include <mss.H>
 #include <lib/freq/sync.H>
 #include <lib/utils/find.H>
+#include <lib/utils/assert_noexit.H>
 
 using fapi2::TARGET_TYPE_DIMM;
 using fapi2::TARGET_TYPE_MCS;
+using fapi2::TARGET_TYPE_MCA;
 using fapi2::TARGET_TYPE_MCBIST;
 using fapi2::TARGET_TYPE_SYSTEM;
 
 namespace mss
 {
 
+// This doesn't seem to be available from the XML, so we'll kind of just hard-wire it. They're
+// here as I don't think these values are generally useful and I really don't want them used
+// other places.
+constexpr size_t NUM_VPD_FREQS = 4;
+constexpr size_t NUM_MAX_FREQS = 5;
+
 ///
 /// @brief Retrieves a mapping of MSS frequency values per mcbist target
 /// @param[in] i_targets vector of controller targets
@@ -148,7 +157,6 @@ bool deconfigure(const fapi2::Target<TARGET_TYPE_MCBIST>& i_target,
     FAPI_INF("---- In deconfigure ----");
     bool l_is_hw_deconfigured = false;
 
-    // TODO - RTC 155347 fix dimm speed & nest comparison if needed
     if(i_dimm_speed != i_nest_freq)
     {
         // Deconfigure MCSes
@@ -156,13 +164,13 @@ bool deconfigure(const fapi2::Target<TARGET_TYPE_MCBIST>& i_target,
         {
             l_is_hw_deconfigured = true;
 
-            PLAT_FAPI_ASSERT_NOEXIT(false,
-                                    fapi2::MSS_FREQ_NOT_EQUAL_NEST_FREQ()
-                                    .set_MSS_FREQ(i_dimm_speed)
-                                    .set_NEST_FREQ(i_nest_freq)
-                                    .set_MCS_TARGET(l_mcs),
-                                    "Deconfiguring %s",
-                                    mss::c_str(l_mcs) );
+            MSS_ASSERT_NOEXIT(false,
+                              fapi2::MSS_FREQ_NOT_EQUAL_NEST_FREQ()
+                              .set_MSS_FREQ(i_dimm_speed)
+                              .set_NEST_FREQ(i_nest_freq)
+                              .set_MCS_TARGET(l_mcs),
+                              "Deconfiguring %s",
+                              mss::c_str(l_mcs) );
         }// end for
     }// end if
 
@@ -176,63 +184,99 @@ bool deconfigure(const fapi2::Target<TARGET_TYPE_MCBIST>& i_target,
 /// @param[in] i_nest_freq nest frequency
 /// @param[in] i_required_sync_mode system policy to enforce synchronous mode
 /// @param[out] o_selected_sync_mode final synchronous mode
+/// @param[out] o_selected_freq final freq selected, only valid if final sync mode is in-sync
 /// @return FAPI2_RC_SUCCESS iff successful
 ///
 fapi2::ReturnCode select_sync_mode(const std::map< fapi2::Target<TARGET_TYPE_MCBIST>, uint64_t >& i_freq_map,
                                    const speed_equality i_equal_dimm_speed,
                                    const uint32_t i_nest_freq,
                                    const uint8_t i_required_sync_mode,
-                                   sync_mode& o_selected_sync_mode)
+                                   uint8_t& o_selected_sync_mode,
+                                   uint64_t& o_selected_freq)
 {
     FAPI_INF("---- In select_sync_mode ----");
 
-    // Implementing frequency handling discussion:
-    // https://w3-connections.ibm.com/forums/html/topic?id=1222318f-5992-4342-a858-b75594df1be3&ps=
-    // Summary: We will always use async mode if we don't see a perfect match of dimm frequencies that match the nest
+    // If we're in SYNC_MODE_NEVER, then we're done and we tell the caller we're not in sync mode
+    if (fapi2::ENUM_ATTR_REQUIRED_SYNCH_MODE_NEVER == i_required_sync_mode)
+    {
+        o_selected_sync_mode = fapi2::ENUM_ATTR_MC_SYNC_MODE_NOT_IN_SYNC;
+        return fapi2::FAPI2_RC_SUCCESS;
+    }
+
     switch(i_equal_dimm_speed)
     {
+        // If we have MCBIST which have resolved to equal speeds ...
         case speed_equality::EQUAL_DIMM_SPEEDS:
 
-            // Do not run synchronously, even if the frequencies match
-            if (i_required_sync_mode == fapi2::ENUM_ATTR_REQUIRED_SYNCH_MODE_NEVER)
-            {
-                o_selected_sync_mode = sync_mode::MC_NOT_IN_SYNC;
-                break;
-            }
+            // Return back the resulting speed. It doesn't matter which we select from the map as they're all equal
+            // If we end up not in sync in the conditional below, thats ok - this parameter is ignored by the
+            // caller if we're not in sync mode
+            o_selected_freq = i_freq_map.begin()->second;
 
-            // Run synchronously if the dimm and nest freq matches
-            // Implicitly covers case when ATTR_REQUIRED_SYNCH_MODE is
-            // ALWAYS and UNDETERMINED
-            if( i_freq_map.begin()->second  == i_nest_freq)
-            {
-                o_selected_sync_mode = sync_mode::MC_IN_SYNC;
-            }
-            else
+            // When we selected ATTR_MSS_FREQ, we made sure that for forced sync mode cases we didn't
+            // select a DIMM freq the nest couldn't support. So if we're in forced sync mode, we're done.
+            if (i_required_sync_mode == fapi2::ENUM_ATTR_REQUIRED_SYNCH_MODE_ALWAYS)
             {
-                o_selected_sync_mode =  sync_mode::MC_NOT_IN_SYNC;
+                o_selected_sync_mode = fapi2::ENUM_ATTR_MC_SYNC_MODE_IN_SYNC;
+                // On Cronus if the o_selected_freq != i_nest_freq we've got a mismatch. Note that p9_mss_freq ensures
+                // we don't select an invalid freq, but doesn't ensure we select the current nest freq.
+#ifndef __HOSTBOOT_MODULE
+                FAPI_ASSERT(o_selected_freq == i_nest_freq,
+                            fapi2::MSS_FAILED_SYNC_MODE().set_NEST_FREQ(i_nest_freq),
+                            "Configured in sync mode, but the DIMM freq (%d) and the nest freq (%d) don't align",
+                            o_selected_freq, i_nest_freq);
+#endif
+                return fapi2::FAPI2_RC_SUCCESS;
+
             }
 
+            // So we need to decide. We know the DIMM speeds are equal and we know we picked the fastest supportable
+            // speed. So, if we're within the nest frequencies then we can run sync mode (lowest latency.) If we're
+            // outside of the nest frequencies we'll run async (higest DIMM speed.)
+            o_selected_sync_mode = (i_nest_freq <= fapi2::ENUM_ATTR_MSS_FREQ_MT2400) ?
+                                   fapi2::ENUM_ATTR_MC_SYNC_MODE_IN_SYNC : fapi2::ENUM_ATTR_MC_SYNC_MODE_NOT_IN_SYNC;
+            return fapi2::FAPI2_RC_SUCCESS;
             break;
 
         case speed_equality::NOT_EQUAL_DIMM_SPEEDS:
 
-            // Require matching frequencies and deconfigure memory that does not match the nest
+            // When we selected ATTR_MSS_FREQ, we made sure that for forced sync mode cases we didn't
+            // select a DIMM freq the nest couldn't support. That means that the fastest of the MCBIST
+            // is the one that rules the roost (the nest can support it too.) So find that, and set it to
+            // the selected frequency. Then deconfigure the slower MCBIST (unless we're in Cronus in which
+            // case we just bomb out.)
+#ifdef __HOSTBOOT_MODULE
             if( i_required_sync_mode == fapi2::ENUM_ATTR_REQUIRED_SYNCH_MODE_ALWAYS )
             {
-                for(const auto& l_it : i_freq_map)
+                uint64_t l_max_dimm_speed = 0;
+                std::for_each(i_freq_map.begin(), i_freq_map.end(),
+                              [&l_max_dimm_speed](const std::pair<fapi2::Target<TARGET_TYPE_MCBIST>, uint64_t>& m)
                 {
-                    // This can be refactored to return info people might
-                    // need.  Until then this returns true if hw was deconfigured
-                    // FFDC prints out location
-                    deconfigure(l_it.first, l_it.second, i_nest_freq);
+                    l_max_dimm_speed = std::max(l_max_dimm_speed, m.second);
+                });
 
-                }// end for
-            }// end if
+                std::for_each(i_freq_map.begin(), i_freq_map.end(),
+                              [&l_max_dimm_speed](const std::pair<fapi2::Target<TARGET_TYPE_MCBIST>, uint64_t>& m)
+                {
+                    deconfigure(m.first, m.second, l_max_dimm_speed);
+                });
 
-            // Implicitly covers case when ATTR_REQUIRED_SYNCH_MODE is
-            // NEVER, ALWAYS, and UNDETERMINED
-            o_selected_sync_mode = sync_mode::MC_NOT_IN_SYNC;
+                o_selected_sync_mode = fapi2::ENUM_ATTR_MC_SYNC_MODE_IN_SYNC;
+                o_selected_freq = l_max_dimm_speed;
+                return fapi2::FAPI2_RC_SUCCESS;
+            }
 
+#else
+            // Cronus only
+            FAPI_ASSERT(i_required_sync_mode != fapi2::ENUM_ATTR_REQUIRED_SYNCH_MODE_ALWAYS,
+                        fapi2::MSS_FAILED_SYNC_MODE().set_NEST_FREQ(i_nest_freq),
+                        "Seeing forced nest/memory sync mode but DIMM speeds differ from nest speed %d", i_nest_freq);
+#endif
+
+            // Notce that if we don't have equal DIMM speeds we're never in sync. We either error out (Cronus)
+            // or we toss a bunch of DIMM off the boat (f/w) or they didn't care wether we were sync or not.
+            o_selected_sync_mode = fapi2::ENUM_ATTR_MC_SYNC_MODE_NOT_IN_SYNC;
+            return fapi2::FAPI2_RC_SUCCESS;
             break;
 
         default:
@@ -242,6 +286,210 @@ fapi2::ReturnCode select_sync_mode(const std::map< fapi2::Target<TARGET_TYPE_MCB
     }// end switch
 
     return fapi2::FAPI2_RC_SUCCESS;
+
+#ifndef __HOSTBOOT_MODULE
+fapi_try_exit:
+    return fapi2::current_err;
+#endif
+}
+
+///
+/// @brief Create and sort a vector of supported MT/s (freq)
+/// @param[in] MCS target for which to get the DIMM configs
+/// @param[in,out] reference to a std::vector<uint32_t> space to put the sorted vector
+/// @return FAPI2_RC_SUCCESS iff ok
+/// @note Taken from ATTR_MSS_MRW_SUPPORTED_FREQ. The result is sorted so such that the min
+/// supported freq is std::vector<>.begin and the max is std::vector<>.end - 1. You can
+/// search the resulting vector for valid frequencies as it is sorted.
+///
+fapi2::ReturnCode supported_freqs(const fapi2::Target<TARGET_TYPE_MCS>& i_target, std::vector<uint32_t>& io_freqs)
+{
+    std::vector<uint32_t> l_freqs(NUM_VPD_FREQS, 0);
+    std::vector<uint32_t> l_max_freqs(NUM_MAX_FREQS, 0);
+    uint8_t l_req_sync_mode = 0;
+
+    FAPI_TRY( mss::mrw_supported_freq(l_freqs.data()) );
+    FAPI_TRY( mss::max_allowed_dimm_freq(l_max_freqs.data()) );
+    FAPI_TRY( mss::required_synch_mode(l_req_sync_mode) );
+
+    FAPI_INF("attribute supported freqs %d %d %d %d",
+             l_freqs[0], l_freqs[1], l_freqs[2], l_freqs[3]);
+
+    FAPI_INF("attribute supported max freqs %d %d %d %d %d",
+             l_max_freqs[0], l_max_freqs[1], l_max_freqs[2], l_max_freqs[3], l_max_freqs[4]);
+
+    FAPI_INF("attribute required sync mode %d", l_req_sync_mode);
+
+    FAPI_TRY( supported_freqs_helper(i_target,
+                                     l_freqs,
+                                     l_max_freqs,
+                                     l_req_sync_mode == fapi2::ENUM_ATTR_REQUIRED_SYNCH_MODE_ALWAYS,
+                                     io_freqs) );
+
+fapi_try_exit:
+    return fapi2::current_err;
+}
+
+///
+/// @brief Create and sort a vector of supported MT/s (freq) - helper for testing purposes
+/// @param[in] MCS target for which to get the DIMM configs
+/// @param[in] vector of MVPD freqs
+/// @param[in] vector of max allowed freqs
+/// @param[in] bool whether or not we're forced into sync mode
+/// @param[in,out] reference to a std::vector<uint32_t> space to put the sorted vector
+/// @return FAPI2_RC_SUCCESS iff ok
+/// @note the attributes which drive this are read-only so they're hard to change when
+/// testing. So this helper allows us to use the attributes for the main path but
+/// have a path for testing (DFT I think the cool kids call it.)
+///
+fapi2::ReturnCode supported_freqs_helper(const fapi2::Target<TARGET_TYPE_MCS>& i_target,
+        const std::vector<uint32_t>& i_freqs,
+        const std::vector<uint32_t>& i_max_freqs,
+        const bool i_req_sync_mode,
+        std::vector<uint32_t>& io_freqs)
+{
+    // Indexes into the ATTR_MAX_ALLOWED_DIMM_FREQ arrary. e.g., [0][0] is 1R 1 drop
+    constexpr size_t l_indexes[MAX_DIMM_PER_PORT][MAX_PRIMARY_RANKS_PER_PORT] =
+    {
+        {0, 1, 0xFF, 2},
+        {3, 4, 0xFF, 0xFF}
+    };
+
+    // Holds the max freq allowed for this configuration of DIMMs. This is the minimum of maximum
+    // frequencies allowed by the DIMM. So, we start way off the charts so std::min can do the lifting for us.
+    uint32_t l_our_max_freq = ~(0);
+
+    io_freqs.clear();
+
+    // This magic number isn't the number of frequencies supported by the hardware, it's the number
+    // of frequencies in the attribute or VPD. They may be different (see *1 below)
+    if (i_freqs.size() != NUM_VPD_FREQS)
+    {
+        FAPI_ERR("incorrect number of frequencies for %s (%d)", mss::c_str(i_target), i_freqs.size());
+        fapi2::Assert(false);
+    }
+
+    FAPI_INF("unsorted supported freqs %d %d %d %d",
+             i_freqs[0], i_freqs[1], i_freqs[2], i_freqs[3]);
+
+    // (*1) This is the number of elelments in the max_allowed_dimm_freq attribute, not the frequencies of
+    // the system. Hence the magic number.
+    if (i_max_freqs.size() != NUM_MAX_FREQS)
+    {
+        FAPI_ERR("incorrect number of max frequencies for %s (%d)", mss::c_str(i_target), i_max_freqs.size());
+        fapi2::Assert(false);
+    }
+
+    FAPI_INF("max supported freqs %d %d %d %d %d",
+             i_max_freqs[0], i_max_freqs[1], i_max_freqs[2], i_max_freqs[3], i_max_freqs[4]);
+
+    // Given the max supported freqs, find the config based on our DIMMs. There's no great way to do this ...
+    for (const auto& p : mss::find_targets<TARGET_TYPE_MCA>(i_target))
+    {
+        const auto l_dimms = mss::find_targets<TARGET_TYPE_DIMM>(p);
+        uint64_t l_dimms_on_port = l_dimms.size();
+
+        // Just a quick check but we're in deep yogurt if this triggers
+        if (l_dimms_on_port > MAX_DIMM_PER_PORT)
+        {
+            FAPI_ERR("seeing %d DIMM on port %s", l_dimms_on_port, mss::c_str(p));
+            fapi2::Assert(false);
+        }
+
+        for (const auto& d : l_dimms)
+        {
+            uint8_t l_num_master_ranks = 0;
+            size_t l_index = 0xFF;
+
+            FAPI_TRY( mss::eff_num_master_ranks_per_dimm(d, l_num_master_ranks) );
+
+            // Just a quick check but we're in deep yogurt if this triggers
+            if (l_num_master_ranks > MAX_PRIMARY_RANKS_PER_PORT)
+            {
+                FAPI_ERR("seeing %d primary ranks on DIMM %s", l_num_master_ranks, mss::c_str(d));
+                fapi2::Assert(false);
+            }
+
+            FAPI_INF("%s rank config %d drop %d yields max freq attribute index of %d (%d)",
+                     mss::c_str(d), l_num_master_ranks, l_dimms_on_port,
+                     l_indexes[l_dimms_on_port - 1][l_num_master_ranks - 1],
+                     i_max_freqs[l_indexes[l_dimms_on_port - 1][l_num_master_ranks - 1]] );
+
+            l_index = l_indexes[l_dimms_on_port - 1][l_num_master_ranks - 1];
+
+            // Just a quick check but we're in deep yogurt if this triggers
+            if (l_index > NUM_MAX_FREQS)
+            {
+                FAPI_ERR("seeing %d index for %d DIMM and %d ranks on DIMM %s",
+                         l_index, l_dimms_on_port, l_num_master_ranks, mss::c_str(d));
+                fapi2::Assert(false);
+            }
+
+            l_our_max_freq = std::min(l_our_max_freq, i_max_freqs[l_index]);
+        }
+    }
+
+    FAPI_INF("after processing DIMM, max freq is %d", l_our_max_freq);
+
+    // We need to push things as the memcpy doesn't update the vector's count, etc. and we don't
+    // create the vector, we get it passed in. It's not a big deal as we want to touch all the elements
+    // to check for 0's anyway.
+    for (size_t i = 0; i < NUM_VPD_FREQS; ++i)
+    {
+        // Funky if-tree makes things clearer than a combinatorialy explosive conditional
+        if (i_freqs[i] == 0)
+        {
+            // Skip 0's
+            continue;
+        }
+
+        if (i_freqs[i] > l_our_max_freq)
+        {
+            // Skip freqs larger than our max
+            continue;
+        }
+
+        // Add this freq if we're not in sync mode, or, if we are add it if it matches a nest freq
+        if (!i_req_sync_mode || (i_req_sync_mode && (i_freqs[i] <= fapi2::ENUM_ATTR_MSS_FREQ_MT2400)))
+        {
+            io_freqs.push_back(i_freqs[i]);
+        }
+    }
+
+    // We now know io_freqs contains valid frequencies for this DIMM config, system contraints, and sync mode.
+    // Sort it so we know supported min is io_freq.begin and supported max is io_freq.end - 1
+    std::sort(io_freqs.begin(), io_freqs.end());
+
+    // If we have an empty set, we have a pro'lem
+    FAPI_ASSERT(io_freqs.size() != 0,
+                fapi2::MSS_VPD_FREQ_MAX_FREQ_EMPTY_SET()
+                .set_MSS_VPD_FREQ_0(i_freqs[0])
+                .set_MSS_VPD_FREQ_1(i_freqs[1])
+                .set_MSS_VPD_FREQ_2(i_freqs[2])
+                .set_MSS_VPD_FREQ_3(i_freqs[3])
+                .set_MSS_MAX_FREQ_0(i_max_freqs[0])
+                .set_MSS_MAX_FREQ_1(i_max_freqs[1])
+                .set_MSS_MAX_FREQ_2(i_max_freqs[2])
+                .set_MSS_MAX_FREQ_3(i_max_freqs[3])
+                .set_MSS_MAX_FREQ_4(i_max_freqs[4])
+                .set_REQUIRED_SYNC_MODE(i_req_sync_mode)
+                .set_MAX_FREQ_FROM_DIMM(l_our_max_freq)
+                .set_MCS_TARGET(i_target),
+                "%s didn't find a frequency which was in VPD and was allowable max", mss::c_str(i_target));
+
+fapi_try_exit:
+    return fapi2::current_err;
+}
+
+///
+/// @brief Return whether a given freq is supported
+/// @param[in] a freq to check for
+/// @param[in] reference to a std::vector<uint32_t> of freqs
+/// @return bool, true iff input freq is supported
+///
+fapi2::ReturnCode is_freq_supported(const uint32_t i_freq, const std::vector<uint32_t>& i_freqs)
+{
+    return std::binary_search(i_freqs.begin(), i_freqs.end(), i_freq);
 }
 
 }// mss
diff --git a/src/import/chips/p9/procedures/hwp/memory/lib/freq/sync.H b/src/import/chips/p9/procedures/hwp/memory/lib/freq/sync.H
index 51a7fe83f..beae6c17a 100644
--- a/src/import/chips/p9/procedures/hwp/memory/lib/freq/sync.H
+++ b/src/import/chips/p9/procedures/hwp/memory/lib/freq/sync.H
@@ -49,12 +49,6 @@ enum class speed_equality : uint8_t
     EQUAL_DIMM_SPEEDS = 1,
 };
 
-enum class sync_mode : uint8_t
-{
-    MC_NOT_IN_SYNC =  0,
-    MC_IN_SYNC = 1,
-};
-
 ///
 /// @brief Retrieves a mapping of MSS frequency values per mcbist target
 /// @param[in] i_targets vector of controller targets
@@ -84,13 +78,54 @@ bool deconfigure(const fapi2::Target<fapi2::TARGET_TYPE_MCBIST>& i_target,
 /// @param[in] i_nest_freq nest frequency
 /// @param[in] i_required_sync_mode system policy to enforce synchronous mode
 /// @param[out] o_selected_sync_mode final synchronous mode
+/// @param[out] o_selected_freq final freq selected, only valid if final sync mode is in-sync
 /// @return FAPI2_RC_SUCCESS iff successful
 ///
 fapi2::ReturnCode select_sync_mode(const std::map< fapi2::Target<fapi2::TARGET_TYPE_MCBIST>, uint64_t >& i_freq_map,
                                    const speed_equality i_equal_dimm_speed,
                                    const uint32_t i_nest_freq,
                                    const uint8_t i_required_sync_mode,
-                                   sync_mode& o_selected_sync_mode);
+                                   uint8_t& o_selected_sync_mode,
+                                   uint64_t& o_selected_freq);
+
+///
+/// @brief Create and sort a vector of supported MT/s (freq) - helper for testing purposes
+/// @param[in] MCS target for which to get the DIMM configs
+/// @param[in] vector of MVPD freqs
+/// @param[in] vector of max allowed freqs
+/// @param[in] bool whether or not we're forced into sync mode
+/// @param[in,out] reference to a std::vector<uint32_t> space to put the sorted vector
+/// @return FAPI2_RC_SUCCESS iff ok
+/// @note Taken from ATTR_MSS_MRW_SUPPORTED_FREQ. The result is sorted so such that the min
+/// supported freq is std::vector<>.begin and the max is std::vector<>.end - 1. You can
+/// search the resulting vector for valid frequencies as it is sorted.
+///
+fapi2::ReturnCode supported_freqs_helper(const fapi2::Target<fapi2::TARGET_TYPE_MCS>& i_target,
+        const std::vector<uint32_t>& i_freqs,
+        const std::vector<uint32_t>& i_max_freqs,
+        const bool i_req_sync_mode,
+        std::vector<uint32_t>& io_freqs);
+
+///
+/// @brief Create and sort a vector of supported MT/s (freq)
+/// @param[in] MCS target for which to get the DIMM configs
+/// @param[in,out] reference to a std::vector<uint32_t> space to put the sorted vector
+/// @return FAPI2_RC_SUCCESS iff ok
+/// @note Taken from ATTR_MSS_MRW_SUPPORTED_FREQ. The result is sorted so such that the min
+/// supported freq is std::vector<>.begin and the max is std::vector<>.end - 1. You can
+/// search the resulting vector for valid frequencies as it is sorted.
+///
+fapi2::ReturnCode supported_freqs(const fapi2::Target<fapi2::TARGET_TYPE_MCS>& i_target,
+                                  std::vector<uint32_t>& io_freqs);
+
+///
+/// @brief Return whether a given freq is supported
+/// @param[in] a freq to check for
+/// @param[in] reference to a std::vector<uint32_t> of freqs
+/// @return bool, true iff input freq is supported
+///
+fapi2::ReturnCode is_freq_supported(const uint32_t i_freq, const std::vector<uint32_t>& i_freqs);
+
 
 }// mss