From 8e72b20e351535887e05c0c87c4f4ec77bfd14bf Mon Sep 17 00:00:00 2001
From: Eric Kooistra <kooistra@astron.nl>
Date: Fri, 13 Mar 2020 15:15:24 +0100
Subject: [PATCH] Improved the descriptions.

---
 libraries/base/mm/src/vhdl/mm_bus.vhd        | 49 +++++++++++++++-----
 libraries/base/mm/src/vhdl/mm_bus_comb.vhd   | 17 ++++---
 libraries/base/mm/src/vhdl/mm_bus_pipe.vhd   | 43 +++++++++++++----
 libraries/base/mm/src/vhdl/mm_master_mux.vhd |  2 +-
 libraries/base/mm/src/vhdl/mm_pipeline.vhd   | 16 ++++---
 5 files changed, 92 insertions(+), 35 deletions(-)

diff --git a/libraries/base/mm/src/vhdl/mm_bus.vhd b/libraries/base/mm/src/vhdl/mm_bus.vhd
index 75e5fdb98f..8e277ea397 100644
--- a/libraries/base/mm/src/vhdl/mm_bus.vhd
+++ b/libraries/base/mm/src/vhdl/mm_bus.vhd
@@ -27,11 +27,29 @@
 --   In addition to mm_bus_pipe this mm_bus takes care of:
 --   - not connected slaves
 --   - slaves that do not need flow control
--- 
---   * MM bus --> see mm_bus_comb
---   * Slave allocation --> see mm_bus_comb
---   * Read latency --> see mm_bus_comb
---   * Pipelining --> see mm_bus_pipe
+--                                    
+--                                     FOR g_nof_slaves:
+--                             g_waitrequest_arr
+--                             g_rd_latency_arr 
+--                              |          |
+--                              |          |
+--        g_base_arr            |          |
+--        g_width_arr           |          |
+--        g_pipeline_mosi       |          |
+--        g_pipeline_miso_rdval |          |
+--        g_pipeline_miso_wait  |          |
+--                          |   |          |              
+--                        __v___v_     ____v___
+--   master_miso <-------| mm_bus |<--| mm     |<-- slave_miso_arr
+--   master_mosi ------->| pipe   |-->| slave  |--> slave_mosi_arr
+--                       |________|   | enable |
+--                                    |________|
+--   The mm_bus_comb takes care of:
+--   - MM bus multiplexer between master and slaves
+--   - MM slave address allocation on the MM bus
+--   The mm_bus_pipe takes care of:
+--   - Pipelining the MM bus, the mm_bus_comb and mm_slave_enable are
+--     combinatorial.
 --
 -- Usage:
 --   The ascii drawing shows how this mm_bus can be used in combination
@@ -53,6 +71,9 @@
 --                    |
 --       M -----------|
 --                mm_master_mux
+-- 
+--     The mm_slave_mux and mm_master_mux should typically be combinatorial,
+--     because all MM bus pipelining is concentrated in mm_bus_pipe.
 --
 --   * The mm_slave_mux is useful to present an array of equal slave MM
 --     ports via a single port on the MM bus. Otherwise the mm_bus could
@@ -67,13 +88,17 @@
 -- 
 -- Limitations --> see mm_bus_comb
 --
--- Todo:
--- * The mm_bus assumes that the MM slave will pull miso.waitrequest low.
---   To avoid that the MM bus accesss will stall, a MM slave port that uses
---   mosi flow control should also support a waitrequest timeout mechanism.
---   The master can then be informed about the failing mosi access using
---   the miso.response field of the Avalon bus. Such a timeout mechanism
---   could also be supported via mm_slave_enable.
+-- Todo (only if necessary):
+-- * The mm_bus assumes that the MM slave will eventually acknowledge an
+--   mosi.wr or rd access by pulling miso.waitrequest low. If the MM slave
+--   malfunctions then the MM bus access will stall. Therefore a MM slave
+--   port that uses mosi flow control should also support a waitrequest
+--   timeout mechanism. Such a waitrequest timeout mechanism could be made
+--   part of mm_slave_enable.
+-- * The master can then be informed about a failing mosi access using
+--   the miso.response field of the Avalon bus. A failing mosi access can
+--   be due to a not connected slave, an out-of-range address, a slave that
+--   times out on flow control.
 --
 -------------------------------------------------------------------------------
 
diff --git a/libraries/base/mm/src/vhdl/mm_bus_comb.vhd b/libraries/base/mm/src/vhdl/mm_bus_comb.vhd
index 8befe54e9c..14c887d118 100644
--- a/libraries/base/mm/src/vhdl/mm_bus_comb.vhd
+++ b/libraries/base/mm/src/vhdl/mm_bus_comb.vhd
@@ -42,8 +42,14 @@
 --   . The base address of a slave has to be a power of 2 multiple of the
 --     slave span.
 --
--- * The mm_clk is only used when there is a slave with read latency > 0 or
---   when the MM bus uses pipelining.
+-- * The mm_clk is only used when there is a slave with read latency > 0, to
+--   pipeline the slave_index_arr for the master_miso.rddata/rdval.
+--   Typically a master will wait for the last rdval, before accessing
+--   another slave port, so then it is not benecessary to pipeline the
+--   slave_index_arr. However registering the slave_index_arr eases timing
+--   closure on the miso part and will allow reading from different slave
+--   ports without waiting, provided that both slaves have the same read
+--   latency.
 --
 -- * Read latency
 --   For read accesses a slave will typically have a read latency > 0, which
@@ -65,10 +71,12 @@
 --                                   rd               |
 --                                                    v
 --    master_miso <--------------------slave_miso_arr[ ]<-- slave_miso_arr
+--
 --   
 -- * No pipelining
 --   The mm_bus_comb is combinatorial, so there is no pipelining between
---   the master interface and the slave interfaces.
+--   the master interface and the slave interfaces. Use mm_bus_pipe to add
+--   pipelining.
 --     
 -- Usage:
 --   See mm_bus.vhd.
@@ -91,9 +99,6 @@
 --   spaced without address gaps. It is possible to use common_mem_mux in
 --   series with mm_bus_comb to provide hierarchy by reprensenting an array
 --   of slave ports via a single slave port on the MM bus.
--- . In simulation selecting an unused element address will cause a simulation
---   failure. Therefore the element index is only accepted when it is in the
---   0 TO g_nof_slaves-1 range.
 --
 -------------------------------------------------------------------------------
 
diff --git a/libraries/base/mm/src/vhdl/mm_bus_pipe.vhd b/libraries/base/mm/src/vhdl/mm_bus_pipe.vhd
index d788fe39db..4f1b6fe1bf 100644
--- a/libraries/base/mm/src/vhdl/mm_bus_pipe.vhd
+++ b/libraries/base/mm/src/vhdl/mm_bus_pipe.vhd
@@ -25,9 +25,28 @@
 -- Description:
 --   The mm_bus_comb is combinatorial, so there is no pipelining between
 --   the master interface and the slave interfaces. If possible do not
---   use pipelining of mosi and miso to avoid increasing the read latency and
---   achieve timing closure by lower clock rate for the MM bus. Pipelining the
---   MM bus can be necessary to achieve timing closure:
+--   use pipelining of mosi and miso to avoid extra logic and to avoid
+--   increasing the read latency. Instead first try achieve timing closure
+--   by lower clock rate for the MM bus. Pipelining the MM bus can be
+--   necessary to achieve timing closure. Thanks to mm_bus_comb the 
+--   pipelining is clearly separated from the MM bus multiplexer. The
+--   pipelining is placed at the output of the bus, so at the slave side
+--   for mosi and at the master side for miso:
+--
+--                                   FOR g_nof_slaves:
+--      g_pipeline_miso_rdval        g_pipeline_mosi
+--      g_pipeline_miso_wait              |   g_pipeline_miso_wait
+--               |                        |           |      
+--               v       ________     ____v___     ___v___
+--   <-- p_miso_pipe <--| mm_bus |<--|mm      |<--|mm     |<-------- 
+--   ------------------>|  comb  |-->|pipeline|-->|latency|--------> 
+--     .              . |________| . |________|   |adapter| .    .
+--   master_miso      .            .            . |_______| . slave_miso_arr
+--   master_mosi      .            .            .           . slave_mosi_arr
+--                  m_miso   bus_miso_arr  pipe_miso_arr  adapt_miso_arr
+--                  m_mosi   bus_mosi_arr  pipe_mosi_arr  adapt_mosi_arr
+--
+--   The MM bus pipelining is defined by:
 --
 --   * g_pipeline_mosi
 --     Pipelining mosi write accesses introduces an extra latency from master
@@ -35,8 +54,8 @@
 --     accesses increases the read latency between accessing the slave and
 --     getting the rddata. Using a different pipelining for the wr and the rd
 --     pulse would yield a different pipelining of the address for write and
---     for read, which is akward. Therefore assume that both mosi write and
---     mosi read have the same pipelining.
+--     for read, which is akward. Therefore both mosi write and mosi read
+--     use the same g_pipeline_mosi pipelining.
 --
 --   * g_pipeline_miso_rdval
 --     Pipelining the miso read data increases the read latency.
@@ -46,10 +65,14 @@
 --     for slaves that need MM flow control. Only applies to slave that
 --     have g_waitrequest_arr is TRUE.
 --
---   The total write latency from master to slave is c_pipeline_mosi.
+--   The pipelining generics are defined as BOOLEAN (rather than NATURAL),
+--   because the pipelining only needs to be 0 or 1.
+--
+--   The total write latency from master to slave is 1 when either 
+--   g_pipeline_mosi or g_pipeline_miso_wait.
 --   The total read latency from master via slave back to master is
---   c_pipeline_mosi + g_rd_latency_arr of the selected slave + 
---   c_pipeline_miso_rdval. 
+--   write latency + g_rd_latency_arr of the selected slave + 1 or 0
+--   dependend on g_pipeline_miso_rdval. 
 --     
 -- Usage:
 --   See mm_bus.vhd
@@ -60,14 +83,14 @@
 --     of the miso.waitrequest that is used at the output of the mm_pipeline
 --     and at the input of the mm_latency adapter:
 --     - at the mm_pipeline output the waitrequest gates the mosi.wr and rd
---     - at the mm_latency_adapter input in common_rl_decrease in the wr or
+--     - at the mm_latency_adapter input in common_rl_decrease the wr or
 --       rd strobe is used to set the waitrequest.
 --     This combinatorial loop seems unavoidable when the interface between
 --     mm_pipeline and mm_latency_adpater is at RL = 0. A solution could be
 --     to increase the RL at the output of the mm_pipeline to RL = 1 by
 --     registering the waitrequest from the mm_latency_adapter. The total
 --     RL for the input of the MM latency adapter then becomes RL = 2, so
---     then the mm_latency_adapter needs t oadapt from RL = 2 to 0.
+--     then the mm_latency_adapter needs to adapt from RL = 2 to 0.
 --     Currently the mm_latency_adapter only supports RL 1 to 0. Is possible
 --     to extent this to RL = N to 0, similar as in dp_latency_adapter.
 --     However fortunately it is not necessary to support g_pipeline_mosi =
diff --git a/libraries/base/mm/src/vhdl/mm_master_mux.vhd b/libraries/base/mm/src/vhdl/mm_master_mux.vhd
index 2e9d4453d5..20d31d78bc 100644
--- a/libraries/base/mm/src/vhdl/mm_master_mux.vhd
+++ b/libraries/base/mm/src/vhdl/mm_master_mux.vhd
@@ -37,7 +37,7 @@
 --
 --   The mm_master_mux operates combinatorially, so it introduces no
 --   extra latency. The mm_clk is needed to hold the index of the master that
---   is currently active, to ensure that the read data.is passed on to the
+--   is currently active, to ensure that the read data is passed on to the
 --   master that did the rd access.
 --
 -- Remarks:
diff --git a/libraries/base/mm/src/vhdl/mm_pipeline.vhd b/libraries/base/mm/src/vhdl/mm_pipeline.vhd
index f4d435b08f..9ef80d3a57 100644
--- a/libraries/base/mm/src/vhdl/mm_pipeline.vhd
+++ b/libraries/base/mm/src/vhdl/mm_pipeline.vhd
@@ -31,7 +31,8 @@
 --   ready for ready latency RL = 0. For RL = 0 the ready acts as an
 --   acknowledge to pending data. For RL > 0 the ready acts as a request for
 --   new data. The miso.waitrequest is defined for RL = 0 but for analysis
---   the timing diagrams below show an example of both RL = 0 and RL = 1.
+--   the timing diagrams below show an example of both RL = 0 and RL = 1. The
+--   miso.waitrequest is equivalent to NOT sosi.ready.
 --
 --   * RL=1
 --                 _   _   _   _   _   _   _   _   _   _   _   _
@@ -74,17 +75,20 @@
 --   out_val. The ready/reg_ready is used and not the in_val, because by
 --   using the ready/reg_ready the pipeline register is emptied as soon
 --   as the ready is active, rather than to wait for a next in_val to push
---   it out.
+--   it out. The ready/reg_ready have the same latency as the in_val,
+--   because they are both derived using the same RL.
 --
 -- Remark:
 -- * The mm_pipeline could be optimized regarding the miso.waitrequest flow
 --   control if it would be implemented similar as dp_pipeline.vhd. This
 --   involves using the pipeline register to accept an access when it is
 --   empty. In this way the waitrequest to the in_mosi only needs to apply
---   when the out_miso is not ready and the pipeline is full. The advantage
---   of simply registering in_mosi and wiring in_miso is that it is simpler
---   and does not put extra logic into the combinatorial miso.waitrequest
---   path.
+--   when the out_miso is not ready and the pipeline is full. This would
+--   achieve the maximum throughput. The advantage of simply registering
+--   in_mosi and wiring in_miso is that it is simpler and does not put extra
+--   logic into the combinatorial miso.waitrequest path. It is better to
+--   keep it simpler and with less logic, then to try to win the last few
+--   percent of throughput.
 
 LIBRARY IEEE, common_lib;
 USE IEEE.STD_LOGIC_1164.ALL;
-- 
GitLab