<somlo>
just dumped a whole bunch of stuff into github, getting readyo to go to sleep over here...
<somlo>
*ready
<somlo>
but long story short, I got sdcard to properly pass testing on rocket (with either 8 or 32bit simple CSRs)
<benh>
somlo: hey
<benh>
I very much disagree with your "fix" for the CSRs, this brings back a breakage I fixed actually :)
<benh>
_florent_ somlo: see my comment on the github issue. I can try to investigate why it broke on Mor1kx but making the access size match the actual CSR size was done on purpose, otherwise we end up generating multi-beat transfers on every CSR access
<benh>
when the unsigned long is wider than the CSR size
<benh>
my patch *should* have taken the alignment into account via CSR_OFFSET_BYTES and it works on microwatt which has a 64-bit bus and 32-bit CSRs
<benh>
so I don't know why Mor1kx broke at this point, if you have info, please share, otherwise I'll try to investigate this week-end
<benh>
but I very very much disagree with your "fix"
Degi has quit [Ping timeout: 246 seconds]
Degi has joined #litex
Skip has quit [Remote host closed the connection]
kgugala_ has quit [Ping timeout: 256 seconds]
kgugala has joined #litex
scanakci has joined #litex
_whitelogger has joined #litex
CarlFK has joined #litex
CarlFK has quit [Ping timeout: 256 seconds]
kgugala_ has joined #litex
kgugala__ has joined #litex
kgugala has quit [Ping timeout: 264 seconds]
kgugala_ has quit [Ping timeout: 265 seconds]
proteusguy has quit [Ping timeout: 256 seconds]
proteusguy has joined #litex
wizzy has joined #litex
<_florent_>
somlo: good for the sdcard, i will look at your changes
<_florent_>
somlo, benh: we could discuss the right solution for the CSR fix in the issue and should also probably limit the supported CSR configurations to simplify things
_whitelogger has joined #litex
<benh>
_florent_: I agree, there is no point in supporting every combination under the sun, it's just a pain to maintain & test long run
<benh>
_florent_: and if we support them today & people use them it gets harder to get rid of
<benh>
_florent_: so yes, I wholeheartedly agree :) 32-bit and 8-bit CSR both on a 32-bit wishbone makes sense
<benh>
not sure if there's real advantage to the 8-bit case but we can keep it around if it really saves resources (it does add muxes and uses less registers, I tend to find that we run out of muxes/timing faster than we run out of registers but that's my use cases)
<benh>
if 64-bit systems always have a proper down-converter to 32-bit then alignment can be removed
<benh>
and such systems can just use normal 32-bit with 32-bit alignment
<benh>
we still need to decide on endian :) but that's less urgent
<_florent_>
yes this seems fine and easier to support in the long term
<benh>
somlo: opinions ? :)
<benh>
_florent_: if we go down that path, we could simplify hw/common.h a lot too
<benh>
we can make csr_*simple just open-code the 4 read/write for the 8-bit case and make everything else operate on 32-bit quantities
<somlo>
_florent_, benh: I honestly wish the CSR bus had some concept of strobes :)
<somlo>
because that's the main reason we have to have 64bit CSR alignment right now
<somlo>
i.e., the 64bit CPU makes 64-bit accesses for *everything* (even an 8bit read/write), which then gets translated into two 32-bit accesses by the wishbone down-shifter, which then touches two adjacent 32-bit CSRs (if they're not 64-bit aligned), regardless of whether one half of the original 64-bit request is completely strobed out
<_florent_>
somlo: we could handle the strobes in the Wishbone2CSR converter
<_florent_>
somlo: ie, if the strobes are not set for the the 32 LSB or MSB, we could just avoid the access on the CSR bus for this
<somlo>
right, and then we could get rid of 64-bit alignment. I'm just saying, with the way things are right now, the MMIO (simple) accessors are what they need to be. They're wrong on some "ideal" hardware we don't (yet) have :D
<_florent_>
somlo: ok, so we could work on this together
<somlo>
and I'll take a pass at fixing the "buffer" accessors for non-multiple cases, unless whatever we're about to do changes *that*, in which case I'd wait until the dust settles :D
_whitelogger has joined #litex
<benh>
wait...
<benh>
why would it do that ?
<benh>
I mean, why would the 64->32 downconverter generate cycles for the lanes that have no byte enable ?
<benh>
mine doesn't :-)
<benh>
(the one I have in microwatt)
<benh>
and yes, the Wishbone2CSR converter should definitely check the enables
<benh>
for 32-bit and 8-bit
<benh>
_florent_: ok so I see two issues indeed with the approach I want for the accessors
<benh>
_florent_: indeed it looks like the wishbone downconverter will turn any access from a 64-bit master into 2x32-bit access downstream even if sel's are all 0
<benh>
that should/could probably be optimized :)
<benh>
(unless I'm misreading the migen)
<benh>
_florent_: and yes Wishbone2CSR should probably check that relevant sel bits are set for the CSR size
_whitelogger has joined #litex
scanakci has quit [Quit: Connection closed for inactivity]
<benh>
_florent_: if we start making the main wishbone 64-bit, we'll have more 64->32 downconverters for random IO parts (not a bad idea for utilization anyway)
<benh>
_florent_: might be worth looking at avoiding the downstream phases for unused sels
<benh>
_florent_: in microwatt it's easy bcs the downconverter is hard wired for 64-32 :)
<benh>
_florent_: the general case is harder but we could special case the "half size" case in the generator
<benh>
_florent_: I basically have two signals "have_top" and "have_bottom" and define the next state based on them with a state for each half
<somlo>
_florent_: seems to work in sim with rocket and 32-bit csr data width; I'll run some more tests, and try on the nexys4ddr after that
<somlo>
then I'll pretty much have to overhaul the 64-bit linux drivers in litex-hub/linux :)
<somlo>
but writing to the scratch register no longer causes the SoC to reset (because of the preceding reset register contained within the same 64-bit aligned pair :)
kgugala__ has quit [Quit: -a- Connection Timed Out]
kgugala has joined #litex
<somlo>
32-bit alignment appears to work on nexys4ddr with rocket and the updated sdcard code from PR 547
<_florent_>
somlo: thanks for the test, yes i was planning to remove the CSR re-alignment on the CPU data width if it was working without for you
<_florent_>
somlo: maybe we should also remove 64-bit CSR alignment at the same time
<_florent_>
benh: i agree we should avoid downstream access if sel is not set, i will look at implementing it. Here i just wanted to see if we could avoid the 64-bit CSR alignment support
<somlo>
_florent_: I think I'm ok with removing 64bit alignment altogether
<somlo>
I assume the linux drivers will be more or less the same in that case for both vexriscv and rocket
<somlo>
but I can check (and clean things up if needed)
<somlo>
I have 15 days of time off and nowhere to go (my flight to Budapest just got canceled, so no vacation this summer for the Somlos :(
<somlo>
on the bright side, I can spend some fun time with LiteX now that $DAYJOB won't pay for it anymore :D