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.
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"]
(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"?
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.
(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. ```
(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.
(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.
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 :)
(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.
(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?
> 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.
(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> ```
(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.
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.
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.
(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.
(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.
(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!
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)