GPIO document updates#340
Conversation
MikeOpenHWGroup
left a comment
MikeOpenHWGroup
left a comment
There was a problem hiding this comment.
Fantastic work @cst-jayesht. Just a few comments.
docs/doc-src/ip-blocks/apb_gpio.rst
Outdated
| Pin Direction Control | ||
| ~~~~~~~~~~~~~~~~~~~~~ | ||
| GPIO have input, output, and open-drain pins. The GPIO module allows output and open-drain to be configured individually for each output pin. | ||
|
|
There was a problem hiding this comment.
Is there a fourth option? If the IO PAD is configured with open-drain == 0 does that imply a totem (push-pull) IO driver?
There was a problem hiding this comment.
Hey Mike,
If the Open-drain is disabled, then the gpio_dir pin, simply specifies whether output is enabled for that pin or not.
We've inferred the open-drain behavior for the GPIO based on the following line of the RTL code: -
gpio_dir[i] = r_gpio_dir[i][1] ? ~r_gpio_out[i] : r_gpio_dir[i][0]; // Open Drain
Could you please review the RTL and confirm if our understanding is correct?
There was a problem hiding this comment.
Hi @MikeOpenHWGroup, awaiting your response. Please let us know whenever you have time.
There was a problem hiding this comment.
This is getting confusing! We allowed ourselves to get distracted by the "Open Drain" comment in the source code. While is it almost certainly true that the IO PADs used by a specific ASIC or FPGA implementation of the CORE-V-MCU will support open-drain drivers, there is nothing in the RTL code that requires this.
I have attempted to re-write the Pin Direction Control section of this file, see the attachment. If you agree with this content, you can push it into this PR. If you do, you'll need to update the SETDIR CSR table as well.
There was a problem hiding this comment.
Thanks for the clarification and for revising the Pin Direction Control section—what you've provided looks great! I've incorporated your changes into the document and updated the SETDIR CSR table as well. Please review.
| - Level detection for active-high and active-low signals | ||
| - Interrupt blocking mechanism to prevent repeated level interrupts | ||
| The interrupt signal is captured by the APB Event Controller for further processing. | ||
| Refer to the `APB Event Controller documentation <https://docs.openhwgroup.org/projects/core-v-mcu/doc-src/ip-blocks/apb_event_cntrl.html>`_ for more details. |
There was a problem hiding this comment.
I think it would be better to make this a relative, not absolute path for the URL. If this document gets hosted at (say) eclipse.org, that link will break. Check https://sublime-and-sphinx-guide.readthedocs.io/en/latest/references.html
There was a problem hiding this comment.
Sure, we'll make all the links to be relative
| - To configure gpio_in as input: All pins are input by default and the input cannot be disabled. | ||
| - To configure gpio_out as output: Place a value of 1 in bit [24] along with the pin number in bits [6:0]. | ||
| - To configure gpio_out as open-drain: Place a value of 1 in bit [25] along with the pin number in bits [6:0]. | ||
|
|
There was a problem hiding this comment.
What happens if bit [25] is set to 0?
There was a problem hiding this comment.
This is similar to the previous comment regarding the open-drain functionality.
There was a problem hiding this comment.
Yes, you are right, but this section will be mostly by software developers who may not have read the other text about open-drain, so it might be a good idea to refer to that section here.
There was a problem hiding this comment.
Sure, will refer to that section here.
MikeOpenHWGroup
left a comment
MikeOpenHWGroup
left a comment
There was a problem hiding this comment.
Thanks for this @cst-jayesht, looks good. My apologies for the long delay to complete the reivew.
2 participants
This PR includes documentation updates for the GPIO module. The document has been updated for clarity and also consist of some fixes some of the functional working of the module.