[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 #23 from Andrey Miroshnikov <andrey at technepisteme.xyz> ---
(In reply to Luke Kenneth Casson Leighton from comment #22)
> 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)

It's a good time to clarify this.
The GPIO config byte sent via the WB bus has the following format:
b7 | b6 | b5 | b4 | b3 | b2 | b1 | b0
  bank[2:0]  | io | pd | pu | ie | oe

The "io" signal either has the input or output value.
When the user wants to read, it will contain the current output value when "oe"
is high, otherwise the input value.

(If this is not the behaviour that we want, let me know what we should do
instead).

The GPIO port signals:
b7 | b6 | b5 | b4 | b3 | b2 | b1 | b0
  bank[2:0]  | pd | pu | o  | oe | i

The "ie" signal is not used at the GPIO port (since we only have an i/o/oe type
of IOpad), and I left it unused for now. Should "ie" be brought out to the GPIO
port?

> 
> 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.
> 

"rd_multi" is only an intermediate signal and is not a part of GPIO port
registers.
However I will remove those lines if they are unnecessary.

> 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.
I assumed that as long as WB ack is not asserted, I can delay the transaction
by one cycle. Is this a wrong assumption? Do you expect this block to support
pipelined transactions and thus the operation has be complete in one cycle?


> basically you probably want this:
> 
>  113                         comb +=
> rd_multi[i].oe.eq(gpio_ports[GPIO_num].oe)
>  114 etc. etc.

Ok, will do.

> 
> 
> also, you've made a classic error of using an Array unnecessarily:
> ...
> and using it with a *static* index:
> ...
> Array() is only necessary on a *DYNAMIC* index, like GPIO_num.

Ok, now it's starting to make more sense haha!

> 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))

What's wrong with format()?
Is it wasteful and does it reduce clarity?

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