Add serial adapter interface code

[zhemao]

This is the new serial interface I've been prototyping for use in our FPGA testing and research chips. I'm calling it Serial Adapter Interface (SAI) for now. Open to suggestions for better names.

This interface basically allows the host to write into any arbitrary location in the target memory. So it writes the ELF directly into memory and then "resets" the tiles by writing to their msip registers. The tohost/fromhost communication is similarly handled by reading/writing directly to the memory locations.

One limitation is that this will only work for RV64 systems because ADDR_CHUNKS and LEN_CHUNKS is hardcoded. Maybe this should be derived from the config string?

[colinschmidt]

Maybe call it something like Tether Serial Interface (TSI)?

[aswaterman]

This seems to conflate two things (SAI is the transport; IPIs for booting is a higher-level concept that is independent of transport).

On Thu, Oct 6, 2016 at 12:57 PM, Colin Schmidt notifications@github.com wrote:

@colinschmidt requested changes on this pull request.

Pretty good. A couple of questions and one fix, in my comments.

I believe you told me that you have already tested this code with your

tether but I just want to confirm that I'm remembering correctly.

In fesvr/sai.cc https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465:

+void sai_t::push_len(size_t len) +{

• for (int i = 0; i < SAI_LEN_CHUNKS; i++) {
• in_data.push_back(len & 0xffffffff);
• len = len >> 32;
• } +} + +void sai_t::read_chunk(addr_t taddr, size_t nbytes, void* dst) +{
• uint32_t _result = static_cast(dst);
• size_t len = nbytes / sizeof(uint32_t); +
• in_data.push_back(SAI_CMD_READ);
• push_addr(taddr);
• push_len(len - 1);

why do you use len - 1? You using integer division above which will

truncate so don't need to add 1 to guarantee that all nbytes get sent?

In fesvr/sai.cc https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465:

• target->switch_to();
• result[i] = out_data.front();
• out_data.pop_front();
• } +} + +void sai_t::write_chunk(addr_t taddr, size_t nbytes, const void* src) +{
• const uint32_t _src_data = static_cast(src);
• size_t len = nbytes / sizeof(uint32_t); +
• in_data.push_back(SAI_CMD_WRITE);
• push_addr(taddr);
• push_len(len - 1); +
• in_data.insert(in_data.end(), src_data, src_data + len);

Does src_data need padding if nbytes doesn't align with sizeof(uint32_t)?

In fesvr/sai.cc https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465:

• return !in_data.empty(); +} + +void sai_t::switch_to_host(void) +{
• host.switch_to(); +} + +int sai_t::get_ipi_addrs(addr_t *ipis) +{
• const char *cfgstr = config_string.c_str();
• query_result res;
• char key[32]; +
• for (int core = 0; ; core++) {
• snprintf(key, sizeof(key), "core{%d{0{ipi", core);

I've never used this interface and there aren't any comments in the implementation so I just want to make sure you don't need newlines here,

which is the way rocket-chip prints out the string.

In fesvr/sai.cc https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465:

+void sai_t::switch_to_host(void) +{

• host.switch_to(); +} + +int sai_t::get_ipi_addrs(addr_t *ipis) +{
• const char *cfgstr = config_string.c_str();
• query_result res;
• char key[32]; +
• for (int core = 0; ; core++) {
• snprintf(key, sizeof(key), "core{%d{0{ipi", core);
• res = query_config_string(cfgstr, key);
• if (res.start == NULL)
• return core;

This will just continue if we can't find the ipi address? It seems like this interface won't work if that can't be found so why don't we just abort or something to let the user know that something is broken.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-7wi05tezS1GfLuOLPFOGqnJN6LnO4ks5qxVLFgaJpZM4KOUrJ .

[aswaterman]

Why not do the IPIs in target-machine software?

On Thu, Oct 6, 2016 at 2:01 PM, Andrew Waterman waterman@eecs.berkeley.edu wrote:

This seems to conflate two things (SAI is the transport; IPIs for booting is a higher-level concept that is independent of transport).

On Thu, Oct 6, 2016 at 12:57 PM, Colin Schmidt notifications@github.com wrote:

@colinschmidt requested changes on this pull request.

Pretty good. A couple of questions and one fix, in my comments.

I believe you told me that you have already tested this code with your

tether but I just want to confirm that I'm remembering correctly.

In fesvr/sai.cc https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465:

+void sai_t::push_len(size_t len) +{

• for (int i = 0; i < SAI_LEN_CHUNKS; i++) {
• in_data.push_back(len & 0xffffffff);
• len = len >> 32;
• } +} + +void sai_t::read_chunk(addr_t taddr, size_t nbytes, void* dst) +{
• uint32_t _result = static_cast(dst);
• size_t len = nbytes / sizeof(uint32_t); +
• in_data.push_back(SAI_CMD_READ);
• push_addr(taddr);
• push_len(len - 1);

why do you use len - 1? You using integer division above which will

truncate so don't need to add 1 to guarantee that all nbytes get sent?

In fesvr/sai.cc https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465:

• target->switch_to();
• result[i] = out_data.front();
• out_data.pop_front();
• } +} + +void sai_t::write_chunk(addr_t taddr, size_t nbytes, const void* src) +{
• const uint32_t _src_data = static_cast(src);
• size_t len = nbytes / sizeof(uint32_t); +
• in_data.push_back(SAI_CMD_WRITE);
• push_addr(taddr);
• push_len(len - 1); +
• in_data.insert(in_data.end(), src_data, src_data + len);

Does src_data need padding if nbytes doesn't align with sizeof(uint32_t)?

In fesvr/sai.cc https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465:

• return !in_data.empty(); +} + +void sai_t::switch_to_host(void) +{
• host.switch_to(); +} + +int sai_t::get_ipi_addrs(addr_t *ipis) +{
• const char *cfgstr = config_string.c_str();
• query_result res;
• char key[32]; +
• for (int core = 0; ; core++) {
• snprintf(key, sizeof(key), "core{%d{0{ipi", core);

I've never used this interface and there aren't any comments in the implementation so I just want to make sure you don't need newlines here,

which is the way rocket-chip prints out the string.

In fesvr/sai.cc https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465:

+void sai_t::switch_to_host(void) +{

• host.switch_to(); +} + +int sai_t::get_ipi_addrs(addr_t *ipis) +{
• const char *cfgstr = config_string.c_str();
• query_result res;
• char key[32]; +
• for (int core = 0; ; core++) {
• snprintf(key, sizeof(key), "core{%d{0{ipi", core);
• res = query_config_string(cfgstr, key);
• if (res.start == NULL)
• return core;

This will just continue if we can't find the ipi address? It seems like this interface won't work if that can't be found so why don't we just abort or something to let the user know that something is broken.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/riscv/riscv-fesvr/pull/16#pullrequestreview-3186465, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-7wi05tezS1GfLuOLPFOGqnJN6LnO4ks5qxVLFgaJpZM4KOUrJ .

[zhemao]

@aswaterman This "reset" function in which I write the IPIs is defined in HTIF itself. I have to do something to get the cores to reset and start executing the code that's just been loaded.

What do you mean by "do the IPIs in target machine software"? Do you mean do this in pk/bbl? How would they know when to send the IPIs?

[aswaterman]

I don't know; how does the fesvr know when to send them?

[zhemao]

Fesvr knows because it's the one loading the program. So once it's done loading the program it sends the IPIs.

[aswaterman]

I'm fine with merging this once @davidbiancolin approves the zynq changes

[colinschmidt]

Sure

[zhemao]

I've renamed SAI to TSI: Tethered Serial Interface. Updated both PRs. @davidbiancolin @colinschmidt is this acceptable?

[davidbiancolin]

Yes, most acceptable.

I have a slew of other wonderful names i can acronym to TSI when i get to play with this on the FPGA.

[zhemao]

Okay, I'm going to merge this and then bump things up the submodule hierarchy.