Bug 519 - Get output from ulx3s serial port to show up in minicom
Summary: Get output from ulx3s serial port to show up in minicom
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: Normal normal
Assignee: Cole Poirier
URL:
Depends on:
Blocks: 383
  Show dependency treegraph
 
Reported: 2020-10-18 01:09 BST by Cole Poirier
Modified: 2020-10-23 00:10 BST (History)
2 users (show)

See Also:
NLnet milestone: NLNet.2019.10.Wishbone
total budget (EUR) for completion of task and all subtasks: 0
budget (EUR) for this task, excluding subtasks' budget: 0
parent task for budget allocation: 383
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:


Attachments
Patch that adds ability to build using versa_ecp5.py with ulx3s (5.64 KB, patch)
2020-10-20 22:15 BST, Cole Poirier
Details | Diff
Use OO instead of string testing in cofiguration (10.77 KB, patch)
2020-10-22 22:36 BST, Cole Poirier
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cole Poirier 2020-10-18 01:09:53 BST
Started on the mailing list here: http://lists.libre-soc.org/pipermail/libre-soc-dev/2020-October/000972.html, but I realized today I should have created a bug report instead of stupidly continuing to correspond about this on the daily update thread. Sorry everyone!

Will post my corrected patch here shortly.
Comment 1 Cole Poirier 2020-10-18 01:19:46 BST
Hi Luke, having some difficulty. There is no longer a cpu_variant standardjtag, it has become standardjtaggpiotest. You have instructed me to not make the below change (versa_ecp5.py), but to make the bottom-most change that makes standardjtag into standardjtaggpiotest (sim.py). This seems like a contradiction to me, but I may be misunderstanding your instructions. How would you like me to resolve this?

diff --git a/src/soc/litex/florent/versa_ecp5.py
b/src/soc/litex/florent/versa_ecp5.py
index 8774b849..a001476e 100755
--- a/src/soc/litex/florent/versa_ecp5.py
+++ b/src/soc/litex/florent/versa_ecp5.py
@@ -69,7 +69,7 @@ class ULX3S85FTestSoC(ulx3s.BaseSoC):
             cpu_cls      = LibreSoC,
-            cpu_variant  = "standardjtag",
+            cpu_variant  = "standardjtaggpiotest",

diff --git a/src/soc/litex/florent/libresoc/core.py
b/src/soc/litex/florent/libresoc/core.py
index 81bd0dfc..10ad9396 100644
--- a/src/soc/litex/florent/libresoc/core.py
+++ b/src/soc/litex/florent/libresoc/core.py
@@ -13,7 +13,7 @@ from libresoc.ls180 import io
-CPU_VARIANTS = ["standard", "standard32", "standardjtag", "ls180",
+CPU_VARIANTS = ["standard", "standard32", "standardjtaggpiotest", "ls180",
                 "standardjtagnoirq"]
Comment 2 Luke Kenneth Casson Leighton 2020-10-18 05:13:55 BST
(In reply to Cole Poirier from comment #1)
> Hi Luke, having some difficulty. There is no longer a cpu_variant
> standardjtag, 

why do you feel it is necessary to remove a variant? do you think it would be a good idea to stop code from working by removing one of the options?


> it has become standardjtaggpiotest.

why?

> You have instructed me to
> not make the below change (versa_ecp5.py), but to make the bottom-most
> change that makes standardjtag into standardjtaggpiotest (sim.py). 

no i did not.

> This
> seems like a contradiction to me,

where did i say "remove it"?
Comment 3 Luke Kenneth Casson Leighton 2020-10-18 06:58:20 BST
so, to reiterate:

* variants specify what is permitted
* sim.py, versa_cp5.py, may request a variant
* verilog file must provide the signals used by the variant.

you need to check *the full chain* by looking at the source and the output from each command.
Comment 4 Cole Poirier 2020-10-18 07:50:10 BST
(In reply to Luke Kenneth Casson Leighton from comment #2)
> (In reply to Cole Poirier from comment #1)
> > Hi Luke, having some difficulty. There is no longer a cpu_variant
> > standardjtag, 
> 
> why do you feel it is necessary to remove a variant? do you think it would
> be a good idea to stop code from working by removing one of the options?

Nope, I’m being very cautious, that’s why I did’t commit it even though you said “commit this line”, because I knew doing so had the potential to royally mess up the code. I remember the rule. Don’t commit something that could break code that is depended on. That’s why I was planning on getting your explicit approval of the whole diff again after I had removed the changes you denied but kept the ones you approved. That’s actually why I created this bug report. Because it’s the appropriate place to review code and has the ability to upload patch files. I was mistakenly using the mailing list for something other than it’s intended purpose yesterday because I forgot/didn’t realize that this was meant to be done as a bug :) 

After making the changes I then tested it to make sure it still worked and thinking carefully about how changing one of the variants could have ramifications for other code was planning on doing a series of tests to make sure the code still worked as it did before my changes. Perhaps I should write a unit test/some asserts... Anyhow, it failed with the assertion error “standardjtag not in CPU_VARIANTS” on the first test which was running “./versa_ecp5 —-build —-fpga ulx3s85f” because you approved the change where standardjtag was changed to standardjtaggpiotest in litex/florent/libresoc/core.py and litex/florent/sim.py but denied the change where the same was done in versa_ecp5.py (see exact email where you said this below). I was pretty sure you didn’t actually approve the changes I made that you said commit because you misread my diff to be adding a variant not modifying a variant. That’s why I’m coming to you for clarification because of this miscommunication.

> > it has become standardjtaggpiotest.
> 
> why?

Because you said that that diff was ok to commit, however I didn’t commit it because I tried testing it and it didn’t work, because as it stands there’s an inconsistency.

> > You have instructed me to
> > not make the below change (versa_ecp5.py), but to make the bottom-most
> > change that makes standardjtag into standardjtaggpiotest (sim.py). 
> 
> no i did not.

If so I am confused see below in response to your ‘where did I say “remove it”?’ You didn’t explicitly say remove it because it was a diff you were commenting on. But approval of a diff where the string was removed and replaced with the same string but new characters appended to it, is strong implication (not implicature as most people use ‘implies’ to refer to) of this. In one of the files you approved the change, in the other you denied it. That’s the part I’m finding confusing and trying to resolve.

> > This
> > seems like a contradiction to me,
> 
> where did i say "remove it"?

This email http://lists.libre-riscv.org/pipermail/libre-soc-dev/2020-October/000963.html

Are we on the same page now or is there still a misunderstanding/miscommunication?

```

read carefully.

> ```
> diff --git a/src/soc/litex/florent/libresoc/core.py
> b/src/soc/litex/florent/libresoc/core.py
> index 81bd0dfc..10ad9396 100644
> --- a/src/soc/litex/florent/libresoc/core.py
> +++ b/src/soc/litex/florent/libresoc/core.py
> @@ -13,7 +13,7 @@ from libresoc.ls180 import io
>
> -CPU_VARIANTS = ["standard", "standard32", "standardjtag", "ls180",
> +CPU_VARIANTS = ["standard", "standard32", "standardjtaggpiotest", "ls180",
>                  "standardjtagnoirq"]

yes.  commit this one line change with an explanation (and the sim.py
below) in the same commit.

do NOT use "git commit -a".

>
> diff --git a/src/soc/litex/florent/sim.py b/src/soc/litex/florent/sim.py
> index c797a43d..bce505d2 100755
> --- a/src/soc/litex/florent/sim.py
> +++ b/src/soc/litex/florent/sim.py
> @@ -49,7 +49,7 @@ class LibreSoCSim(SoCSDRAM):
>          if cpu_data_width == 32:
>              variant = "standard32"
>          else:
> -            variant = "standardjtag"
> +            variant = "standardjtaggpiotest"

yes.

> diff --git a/src/soc/litex/florent/versa_ecp5.py
> b/src/soc/litex/florent/versa_ecp5.py
> index 8774b849..a001476e 100755
> --- a/src/soc/litex/florent/versa_ecp5.py
> +++ b/src/soc/litex/florent/versa_ecp5.py
> @@ -69,7 +69,7 @@ class ULX3S85FTestSoC(ulx3s.BaseSoC):
>              cpu_cls      = LibreSoC,
> -            cpu_variant  = "standardjtag",
> +            cpu_variant  = "standardjtaggpiotest",

no.
```
Comment 5 Luke Kenneth Casson Leighton 2020-10-18 11:47:01 BST
(In reply to Cole Poirier from comment #4)

> Nope, I’m being very cautious, that’s why I did’t commit it even though you
> said “commit this line”, because I knew doing so had the potential to
> royally mess up the code. I remember the rule.

brilliant.

cole, we have a bit of a problem: i'm doing code-review effectively in the middle of the night, or extremely late at night.  consequently i can't even necessarily remember what it is that i said from one day to the next.  i'm not going to be able to spot absolutely everything, so it's a huge relief that you're getting it and spotting discrepancies.
Comment 6 Cole Poirier 2020-10-18 13:47:03 BST
(In reply to Luke Kenneth Casson Leighton from comment #5)
> (In reply to Cole Poirier from comment #4)
> 
> > Nope, I’m being very cautious, that’s why I did’t commit it even though you
> > said “commit this line”, because I knew doing so had the potential to
> > royally mess up the code. I remember the rule.
> 
> brilliant.

Fun when I start to get things eh? I mean do I understand simpleV yet? Nope. But with good foundations the rest will come in time ;)

> cole, we have a bit of a problem: i'm doing code-review effectively in the
> middle of the night, or extremely late at night.  consequently i can't even
> necessarily remember what it is that i said from one day to the next.  i'm
> not going to be able to spot absolutely everything, so it's a huge relief
> that you're getting it and spotting discrepancies.

:) Trying to take responsibility.

Yes, apologies for my working later this week than normal. I’m used to following your lead, i.e. you do stuff while I’m asleep then I catch up the next day, rather than I do stuff while you should be asleep and you help me anyway. We can explicitly switch back to the previous order, and you just won’t respond to me after midnight your time until the morning? I would suggest an earlier cutoff point but... having worked with you for six months now I know you’re often up and fully functioning past then, and that, as well, you’re a bit of a night owl.
Comment 7 Cole Poirier 2020-10-20 22:15:32 BST
Created attachment 109 [details]
Patch that adds ability to build using versa_ecp5.py with ulx3s

Hi Luke, I needed to change a couple of things in order to get this to work. For example I had to add the command line option --variant in litex/florent/sim.py and remove the hard-coded cpu_data_width, but I think this is actually what you wanted. i.e. replacing hard coded compilation options with commandline options for the same. I've tested it and it runs the simulation, builds successfully for ulx3s85f and successfully loads onto my ulx3s85f fpga. Additionally, I tested that it builds successfully for versa_ecp5 but I obviously can't test that it loads successfully as I don't have one. Let me know what changes are necessary and I'll do another 'cycle' of this, then come back here for approval to commit, or if it's correct now then let me know that too :)
Comment 8 Luke Kenneth Casson Leighton 2020-10-21 22:07:27 BST
(In reply to Cole Poirier from comment #7)
> Created attachment 109 [details]
> Patch that adds ability to build using versa_ecp5.py with ulx3s
> 
> Hi Luke, I needed to change a couple of things in order to get this to work.

looks really good.
Comment 9 Cole Poirier 2020-10-21 22:32:20 BST
(In reply to Luke Kenneth Casson Leighton from comment #8)
> (In reply to Cole Poirier from comment #7)
> > Created attachment 109 [details]
> > Patch that adds ability to build using versa_ecp5.py with ulx3s
> > 
> > Hi Luke, I needed to change a couple of things in order to get this to work.
> 
> looks really good.

Ok committed and pushed. Next question before we can close this bug. Is running `./versa_ecp5.py --load` supposed to show output in the minicom terminal?
Comment 10 Luke Kenneth Casson Leighton 2020-10-21 22:51:20 BST
> Ok committed and pushed. Next question before we can close this bug. Is
> running
> `./versa_ecp5.py --load` supposed to show output in the minicom terminal?

if everything is wired up correctly yes.

a good test is to run picorv32 litex.  ask  on the #litex freenode channel how to do that.
Comment 11 Cole Poirier 2020-10-22 00:24:14 BST
(In reply to Luke Kenneth Casson Leighton from comment #10)
> > Ok committed and pushed. Next question before we can close this bug. Is
> > running
> > `./versa_ecp5.py --load` supposed to show output in the minicom terminal?
> 
> if everything is wired up correctly yes.
> 
> a good test is to run picorv32 litex.  ask  on the #litex freenode channel
> how to do that.

Looks like it's a configuration issue in our setup because I get the following interactive output in minicom from running: 
`litex-boards/litex_boards/targets$ ./ulx3s.py --load --device LFE5U-85F`

An ideas of what I can do to connect our 'stuff' properly?


```
Welcome to minicom 2.7.1

OPTIONS: I18n 
Compiled on May  6 2018, 08:02:47.
Port /dev/ttyUSB0, 16:19:20

Press CTRL-A Z for help on special keys                 
                                                        
   Read speed: 10MiB/s                                  
                                                        
--============== Boot ==================--              
Booting from serial...
Press Q or ESC to abort boot completely.
sL5DdSMmkekro
             Timeout
No boot medium found

--============= Console ================--

litex> 
litex> 
litex> ls
Command not found
litex> halt
Command not found
litex> help

LiteX BIOS, available commands:

flush_l2_cache           - Flush L2 cache
flush_cpu_dcache         - Flush CPU data cache
crc                      - Compute CRC32 of a part of the address space
ident                    - Identifier of the system
help                     - Print this help

serialboot               - Boot from Serial (SFL)
reboot                   - Reboot

mem_speed                - Test memory speed
mem_test                 - Test memory access
mem_copy                 - Copy address space
mem_write                - Write address space
mem_read                 - Read address space

sdram_test               - Test SDRAM


litex> 
```
Comment 12 Luke Kenneth Casson Leighton 2020-10-22 03:32:34 BST
(In reply to Cole Poirier from comment #11)
> (In reply to Luke Kenneth Casson Leighton from comment #10)
> > > Ok committed and pushed. Next question before we can close this bug. Is
> > > running
> > > `./versa_ecp5.py --load` supposed to show output in the minicom terminal?
> > 
> > if everything is wired up correctly yes.
> > 
> > a good test is to run picorv32 litex.  ask  on the #litex freenode channel
> > how to do that.
> 
> Looks like it's a configuration issue in our setup because I get the
> following interactive output in minicom from running: 
> `litex-boards/litex_boards/targets$ ./ulx3s.py --load --device LFE5U-85F`
> 
> An ideas of what I can do to connect our 'stuff' properly?

https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/litex/florent/versa_ecp5.py;hb=HEAD

  32             cpu_variant = "standardjtagnoirq",

check the ulx3s class, line 72.  see how duplication means the two near-identical classes get out of sync?


> 
> ```
> Welcome to minicom 2.7.1
> 
> OPTIONS: I18n 
> Compiled on May  6 2018, 08:02:47.
> Port /dev/ttyUSB0, 16:19:20
> 
> Press CTRL-A Z for help on special keys                 
>                                                         
>    Read speed: 10MiB/s                                  
>                                                         
> --============== Boot ==================--              
> Booting from serial...

you could have stopped around here (maybe a few more lines).

it showed success, the rest was effectively spam, because nobody is intwrested in the *contents* of the fact that picorv32 succeeded, just that it did in fact succeed.
Comment 13 Cole Poirier 2020-10-22 22:36:36 BST
Created attachment 111 [details]
Use OO instead of string testing in cofiguration

(In reply to Luke Kenneth Casson Leighton from comment #12)

> https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/litex/florent/
> versa_ecp5.py;hb=HEAD
> 
>   32             cpu_variant = "standardjtagnoirq",
> 
> check the ulx3s class, line 72.  see how duplication means the two
> near-identical classes get out of sync?

I have an idea of how this might be solved through using OO instead of strings. See my attached patch. Is this a good approach? If not, why not? (I do understand that the string testing in the other files will have to be changed to work on the new classes instead, but I wanted your input on this before I do all of that work)

[snip]
> you could have stopped around here (maybe a few more lines).
> 
> it showed success, the rest was effectively spam, because nobody is
> intwrested in the *contents* of the fact that picorv32 succeeded, just that
> it did in fact succeed.

Understood. Thanks for the guidance.
Comment 14 Luke Kenneth Casson Leighton 2020-10-22 23:09:46 BST
if the idea is to have a class which overrides __in__ and __str__ then yes, good one.

however the priority is to get ulx3s working.

code improvements are not high priority unless they seriously interfere with readability or useability.

and this one although particularly odd doesn't to my mind hit that "threshold of painfulness".  if however it does for you then go for it.
Comment 15 Cole Poirier 2020-10-22 23:39:23 BST
(In reply to Luke Kenneth Casson Leighton from comment #14)
> if the idea is to have a class which overrides __in__ and __str__ then yes,
> good one.
> 
> however the priority is to get ulx3s working.
> 
> code improvements are not high priority unless they seriously interfere with
> readability or useability.
> 
> and this one although particularly odd doesn't to my mind hit that
> "threshold of painfulness".  if however it does for you then go for it.

Ok understood. Will probably leave it for now. No my idea was not overrides, that's why I came here to ask for feedback because I wasn't sure if my approach was sensible. It works with variant=standardjtagnoirqtestgpio... just makes me sad to have a janky and string based api for this.

Oh well. Will close this as now a litex prompt shows up in the minicom terminal and will take commands... but since I don't think we have set up any commands it will respond 'command not found' to all input including 'help'. Now is time for me to work on the jtag pins from ulx3s gpio pins on bug 517.
Comment 16 Luke Kenneth Casson Leighton 2020-10-22 23:42:31 BST
(In reply to Cole Poirier from comment #15)

> Oh well. Will close this as now a litex prompt shows up in the minicom
> terminal

hooraay!  Big Deal!  remember when closing make sure to track on your "about" page.

>  and will take commands... but since I don't think we have set up
> any commands it will respond 'command not found' to all input including
> 'help'.

correct.

> Now is time for me to work on the jtag pins from ulx3s gpio pins on
> bug 517.

excellent.
Comment 17 Cole Poirier 2020-10-22 23:44:41 BST
(In reply to Luke Kenneth Casson Leighton from comment #16)
>
> hooraay!  Big Deal!  remember when closing make sure to track on your
> "about" page.

:) Will do!
Comment 18 Luke Kenneth Casson Leighton 2020-10-23 00:10:50 BST
the other thing is: i'd like the developer of ulx3s to be able to post about this.  can you please tar -cvzf the build directory and sftp it up to libre-soc.org?  (don't ask "how do i do that", the answer's "search online how to do it)