Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add docs/library/cn0363/* specific IPs #1550

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

docs: add docs/library/cn0363/* specific IPs #1550

wants to merge 5 commits into from

Conversation

caosjr
Copy link
Contributor

@caosjr caosjr commented Jan 2, 2025

Adds documentation for the CN0363 specific IPs:

  • CN0363 DMA Sequencer;
  • CN0363 Phase Data Sync.

PR Description

Please replace this comment with summary, motivation and context of the changes.
List any dependencies required for this change.

You can check the checkboxes below by inserting a 'x' between square brackets
(without any other characters or spaces) or just check them after publishing the PR.

If there is a breaking change, specify dependent PRs in description and
try to push all related PRs at the same time.

PR Type

  • Bug fix (change that fixes an issue)
  • New feature (change that adds new functionality)
  • Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

  • I have followed the code style guidelines
  • I have performed a self-review of changes
  • I have compiled all hdl projects and libraries affected by this PR
  • I have tested in hardware affected projects, at least on relevant boards
  • I have commented my code, at least hard-to-understand parts
  • I have signed off all commits from this PR
  • I have updated the documentation (wiki pages, ReadMe files, Copyright etc)
  • I have not introduced new Warnings/Critical Warnings on compilation
  • I have added new hdl testbenches or updated existing ones

Adds documentation for the CN0363 specific IPs:
- CN0363 DMA Sequencer;
- CN0363 Phase Data Sync.

Signed-off-by: Carlos Souza <[email protected]>
Copy link
Contributor

@gastmaier gastmaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review done using the GitHub GUI

docs/library/cn0363/cn0363_phase_data_sync/index.rst Outdated Show resolved Hide resolved
docs/library/cn0363/index.rst Outdated Show resolved Hide resolved
caosjr and others added 4 commits January 7, 2025 22:04
fixes some cosmetic changes

Signed-off-by: Carlos Souza <[email protected]>
Remove trailing whitespace.
Use local project ref.
Fix leading backslash.
Rename ip labels.

Signed-off-by: Jorge Marques <[email protected]>
Mostly better PDF output.

Signed-off-by: Jorge Marques <[email protected]>
Copy link
Contributor

@gastmaier gastmaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional review now using the proper tools...

Please do not commit trailing whitespace.
I sent patches privately to help applying the requested changes.

@@ -0,0 +1,97 @@
.. _cn0363_phase_data_sync:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you have plain pages, without additional resources, you can include in the parent simply as

index.rst
cn0363_dma_sequencer.rst
cn0363_phase_data_sync.rst

Remember, the documentation is highly hierarchical, so docs/library/cn0363/cn0363_phase_data_sync/index.rst ends up being redundant.

Comment on lines 19 to 23
.. toctree::
:maxdepth: 2

CN0363 DMA Sequencer <cn0363_dma_sequencer/index>
CN0363 Phase Data Sync <cn0363_phase_data_sync/index>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my suggestion, this could be simply:

.. toctree::
   :maxdepth: 2
   :glob:

   *

@@ -0,0 +1,97 @@
.. _cn0363_phase_data_sync:

CN0363 Phase Data Sync FPGA Peripheral
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CN0363 Phase Data Sync or Phase Data Sync

On my side I want to add to the theme the level navigation on top, which completely removes the desire of providing a "full name" for the page, like in the developer.analog.com :
image

(change included in the review commit)

@@ -0,0 +1,104 @@
.. _cn0363_dma_sequencer:

CN0363 DMA Sequencer FPGA Peripheral
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just CN0363 DMA Sequencer

(change included in the review commit)

.. hdl-component-diagram::

The CN0363 Sequencer FPGA Peripheral is part of the
:dokuwiki:`CN0363 HDL project documentation <resources/eval/user-guides/eval-cn0363-pmdz/reference_hdl>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this page already exists in our repo. (change included in the review commit)

I see however that the wiki page seems to be more complete, could you work on improving the imported one?

Comment on lines 21 to 23
* - :git-hdl:`/library/cn0363/cn0363_phase_data_sync/cn0363_phase_data_sync.v`
- Verilog source for the peripheral.
* - :git-hdl:`/library/cn0363/cn0363_phase_data_sync/cn0363_phase_data_sync_ip.tcl`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leading backslash on git-hdl
(change included in the review commit)

More Information
--------------------------------------------------------------------------------

- :dokuwiki:`[Wiki] CN0363 HDL project documentation <resources/eval/user-guides/eval-cn0363-pmdz/reference_hdl>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link local doc (change included in the review commit)

More Information
--------------------------------------------------------------------------------

- :ref:`EVAL-CN0363-PMDZ HDL reference design <cn0363>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you used the local ref


CN0363 library comprises two IPs:

- **CN0363 DMA Sequencer**;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

; is not used in lists.

Comment on lines 16 to 18
Contents
--------

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useless, better ref the Contents list above and leave the toctree hidden.

@gastmaier
Copy link
Contributor

output.pdf
Example unified doc after patches

Copy link
Contributor

@gastmaier gastmaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page does not fully follow the template https://github.com/analogdevicesinc/hdl/blob/main/docs/library/template_ip/index.rst?plain=1 .
It is important to follow the template, one handy trick of the doc is to cycle the pages anchored on a particular section using Ctrl+Alt+Shift+Left/Right so we can "batch" check on particular section.

- TCL script to generate the Vivado IP-integrator project for the
peripheral.

Signal and Interface Pins
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface

- TCL script to generate the Vivado IP-integrator project for the
peripheral.

Signal and Interface Pins
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface

- Output
- Reset signal for the processing pipeline

Theory of Operation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detailed Description is more used.

- The overflow signal is asserted if a new sample arrives before the
previous one has been consumed.

Theory of Operation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detailed Description is more used.

is inserted into the processing pipeline after the reset belongs to the first
channel.

More Information
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

References

discarded. This allows an application to select which data channels it wants to
capture.

More Information
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

References

bit is set the channel is sent to the ``dma_wr`` interface, otherwise it is
discarded. This allows an application to select which data channels it wants to
capture.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Software Support section?

resets the channel swap detection logic and makes sure that the next sample that
is inserted into the processing pipeline after the reset belongs to the first
channel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Software Support section?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants