Gpio straps doc #25971
Gpio straps doc #25971
Conversation
marcelocarvalhoLowRisc
requested review from
marnovandermaas
and removed request for
a team
marcelocarvalhoLowRisc
force-pushed
the
gpio_straps_doc
branch
from
1ce170c to
8d73e06
Compare
marcelocarvalhoLowRisc
assigned marcelocarvalhoLowRisc and unassigned rswarbrick, martin-velay and marcelocarvalhoLowRisc
marcelocarvalhoLowRisc
force-pushed
the
gpio_straps_doc
branch
6 times, most recently
from
118b2be to
72fa1b1
Compare
Signed-off-by: Marcelo Carvalho Faleiro de Almeida <[email protected]>
marcelocarvalhoLowRisc
force-pushed
the
gpio_straps_doc
branch
from
72fa1b1 to
68f85e5
Compare
| @@ -111,6 +111,7 @@ | |||
| package: "", | |||
| desc: ''' | |||
| This signal is pulsed high by the power manager after reset in order to sample the HW straps. | |||
| It can only be pulsed high one-time after reset. The following pulses will not be taken into consideration. | |||
There was a problem hiding this comment.
I think this sounds like a requirement on the integrator. Don't we want something more like "This will only have an effect the first time it goes high after reset" ? (And I'm not sure about "pulse" in the line: we probably don't have an opinion about whether it has to drop again)
| @@ -390,7 +391,14 @@ | |||
| ], | |||
| }, | |||
| { name: "HW_STRAPS_DATA_IN", | |||
| desc: "GPIO Input data sampled as straps during cold boot read value", | |||
| desc: ''' | |||
| GPIO input data that was sampled as straps as the block came out of reset. | |||
There was a problem hiding this comment.
Nit: I'd avoid the double "as" with something like "... sampled as straps when the block ..."
| desc: ''' | ||
| GPIO input data that was sampled as straps as the block came out of reset. | ||
|
|
||
| This register depends on the GpioAsHwStrapsEn parameter. |
There was a problem hiding this comment.
I guess the register exists either way, so maybe "The behaviour of this register depends ..." ?
| - If the parameter is true then GPIO input data is sampled on the first cycle after reset | ||
| where the strap_en_i input is high. That sampled data is stored in this register. |
There was a problem hiding this comment.
I'd suggest reordering this to "... is sampled after reset on the first cycle where the strap_en_i input is high."
That avoids it sounding like we only do this for special sorts of reset.
| The snapshot value of the GPIO inputs is available in the [`HW_STRAPS_DATA_IN`](../data/gpio.hjson#hw_straps_data_in) register for firmware read out | ||
| Note that all 32 bits of input data to the GPIO controller are sampled in this mode regardless of the gpio output enable status | ||
|
|
||
| The GPIO controller includes an optional feature to sample GPIO input values as hardware configuration straps. After the reset (cold boot), when the strap_en signal is asserted, the GPIO detects the rising edge of the signal and triggers the sampling of GPIO input values one clock cycle later. These sampled values are stored in the HW_STRAPS_DATA_IN register. |
There was a problem hiding this comment.
A couple of style nits:
- The line is really long because there are missing newlines after the full stops.
- As with my comment on the hjson change, I think you might need to reorder things to avoid it sounding like "a reset when strap_en is asserted", which isn't what we want.
- And the sentence that starts "After the reset..." is rather long. Probably worth breaking.
- I'd also avoid talking about "After the reset...". Can't this be more operational? "After each reset, on the first cycle when strap_en is asserted, ..." ?
- Also "rising edge" is dangerous: it sounds a bit like we're talking about a clock.
- Don't talk about "cold boot" in this bit: that's a system-level concern. (I realise that the previous text did so, but we can do better!)
|
|
||
| The GPIO controller includes an optional feature to sample GPIO input values as hardware configuration straps. After the reset (cold boot), when the strap_en signal is asserted, the GPIO detects the rising edge of the signal and triggers the sampling of GPIO input values one clock cycle later. These sampled values are stored in the HW_STRAPS_DATA_IN register. | ||
|
|
||
| The strap_en signal is driven by the power manager (pwrmgr) and is expected to toggle only once after the reset process. Sampling occurs only during this period, and subsequent changes to the GPIO input configuration will not be captured. |
There was a problem hiding this comment.
Is this chunk actually necessary? I think I'd be inclined to drop it, since that's very much a concern of stuff outside of the block.
|
Nuts: I clicked in the wrong place and didn't leave a summary note! Thanks for documenting this carefully: I think it looks like a great improvement. Once it's stable, we can get someone from Rivos to check it matches the design they intended. |
Documentation adjustments for GPIO Straps feature.