[Libre-soc-bugs] [Bug 762] Peripheral Pin Muxing Development

[bugzilla-daemon at libre-soc.org]

https://bugs.libre-soc.org/show_bug.cgi?id=762

--- Comment #22 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
https://git.libre-soc.org/?p=pinmux.git;a=commitdiff;h=56bf88cdb4ef409ec2d7bfb7621797ddce50857c

this looks good:

+                    GPIO_num = Signal(16) # fixed for now
+                    comb += GPIO_num.eq(bus.adr*len(bus.sel)+i)
+                    with m.If(bus.sel[i]):
+                        sync += rd_multi[i].oe.eq(gpio_ports[GPIO_num].oe)

one of these is either unnecessary or the wrong way round:
you don't want to be reading things that should be written
or writing things that should be read.

+                        with m.If (gpio_ports[GPIO_num].oe):
+                            sync += rd_multi[i].io.eq(gpio_ports[GPIO_num].o)
+                        with m.Else():
+                            sync += rd_multi[i].io.eq(gpio_ports[GPIO_num].i)
+                        sync += rd_multi[i].bank.eq(gpio_ports[GPIO_num].bank)

all of these DESTROY the registers.  the whole, entire purpose of sel
is to **not** overwrite or touch that which has a "sel" bit of zero.

+                    with m.Else():
+                        sync += rd_multi[i].oe.eq(0)
+                        sync += rd_multi[i].ie.eq(0)
+                        sync += rd_multi[i].puen.eq(0)
+                        sync += rd_multi[i].pden.eq(0)
+                        sync += rd_multi[i].io.eq(0)
+                        sync += rd_multi[i].bank.eq(0)

also i noticed that you've got this:

 103                 sync += Cat(*wr_multi).eq(wb_wr_data)

followed later by this:

 141                         sync += gpio_ports[GPIO_num].oe.eq(wr_multi[i].oe)

so what will happen there is:

* firstly, on one clock cycle (cyc & stb & we), the incoming wishbone
  write data will be captured into wr_multi. this will make that data
  available ONLY ON THE NEXT CYCLE

* secondly, on the **NEXT** cycle, "bus.we" will oh wait, whoops, it's
  now the next cycle, which has a totally new set of data, it could
  be a read at this point, and the data is hosed.

actually now that i look at it the exact same thing is happening with
rd_multi.

on one clock:

 113                         sync += rd_multi[i].oe.eq(gpio_ports[GPIO_num].oe)

and on the **NEXT** clock (when bus.we is completely invalid and/or unrelated):

 153                 sync += wb_rd_data.eq(Cat(*rd_multi))

basically you probably want this:

 113                         comb += rd_multi[i].oe.eq(gpio_ports[GPIO_num].oe)
 114 etc. etc.


also, you've made a classic error of using an Array unnecessarily:

  72         self.wr_multicsr = Array(temp)
  90         wr_multi = self.wr_multicsr
 103                 sync += Cat(*wr_multi).eq(wb_wr_data)

and using it with a *static* index:

 137                 for i in range(len(bus.sel)):

Array() is only necessary on a *DYNAMIC* index, like GPIO_num.

lose "Array()" on both rd_multi and wr_multi.

  62         self.rd_multicsr = rd_multi = []
  63         for i in range(self.wordsize):
  64             name = "rd_word%d" % i # please don't use format
  65             rd_multi.append(Record(name=name, layout=csrbus_layout))

-- 
You are receiving this mail because:
You are on the CC list for the bug.