[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 #24 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Andrey Miroshnikov from comment #23)
> (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.

no it's good.  with the PU/PD you can actually wiggle OE rather than IO
and get that SCL/SDA behaviour that Cesar was talking about (the
"on" or "high-impededance" thing rather than "on" and "off" which you
would get by wiggling io)

> 
> (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?

no need, let's do that later when there's an actual IOpad to match it.

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

nono, don't do that, but if they are intermediary then move them actually
*into* the elaborate() function and remove "self.".  they're not intended
to be accessed externally, so why are they made public members?  just
move the initialisation wr_multi = blahblah actually directly into elaborate().

basically you can use wr_multi and rd_multi as a means to "re-format"
the data (match the data structure with what needs to go in/out of the
GPIO registers), and you can do that combinatorially.

but you *must* ensure that the GPIO_index (and associated
"sel") triggers only the storage into the GPIO_reg (for write - we=HI)
or copying-out (for read).

> I assumed that as long as WB ack is not asserted, I can delay the
> transaction by one cycle. Is this a wrong assumption?

yes.  what you've done requires two cycles, not one. the we needs to
be held for *two* cycles, which is not going to happen.  therefore you
*will* end up with data corruption, guaranteed.

> Do you expect this
> block to support pipelined transactions and thus the operation has be
> complete in one cycle?

yes. no.  complete on the *next* cycle.  the mistake you made earlier was
to make the acknowledgement combinatorial (which is defined as "complete in
one cycle i.e. complete in *this* cycle).  WB4 pipeline has to have the
acknowledgement be on the *NEXT* cycle, and if you get that strictly correct
it's "compatible" with WB classic.

> > Array() is only necessary on a *DYNAMIC* index, like GPIO_num.
>  
> Ok, now it's starting to make more sense haha!

i did exactly the same thing, early on. wrote code that had a
static for-loop but allocated an Array for each element. total
waste of time.  basically, Array() is a compact way to not have
to do this:

   with m.Switch(index):
      for i in range(1<<max(index)):
          with m.Case(i):
             sync += element[i].eq(something)

you can replace all that bullshit with a *one-line*:

   sync += element[index].eq(something)

but if you are instead doing this, you *do not* need the Array()

   for i in range(fixed_compiletime_constant_number):
        sync += element[i].eq(something)

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

it's nowhere near as compact.  printf formatting has been around
literally for decades, it's extremely well-known, isn't going away,
and is blindingly-obvious

  (first letter always has a clear meaning: (d)ecimal, he(x)adecimal,
    (f)loat (s)tring i mean it's not hard)

even has a wikipedia page https://en.wikipedia.org/wiki/Printf_format_string

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