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

TCL: ad_hpmx_interconnect #1201

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

liambeguin
Copy link
Contributor

@liambeguin liambeguin commented Oct 18, 2023

PR Description

Some custom projects might rely on other PS AXI master interfaces, rework ad_cpu_interconnect to allow users to specify other interfaces.

For now, I was only able to test this on a zynqmp project, and don't have the full list of interfaces for other platforms, but I'm happy to update the PR.

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

@CLAassistant
Copy link

CLAassistant commented Oct 18, 2023

CLA assistant check
All committers have signed the CLA.

@gastmaier gastmaier self-requested a review October 18, 2023 21:06
}
if {$sys_zynq == 2} {
ad_connect ${interconnect_name}/S00_AXI sys_cips/M_AXI_FPD
} elseif {($p_sel eq "HPM0_FPD") && ($sys_zynq == 2)} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps nest

} elseif {$sys_zynq == 2} {
  if {$p_sel eq "HPM0_FPD"} {
    (...)
  }
  (...)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing!

I went with a single line to stay consistent with ad_mem_hpx_interconnect.
I feel like it's also a bit more readable when we end up having other sys_zynq values.

projects/scripts/adi_board.tcl Outdated Show resolved Hide resolved
projects/scripts/adi_board.tcl Show resolved Hide resolved
projects/scripts/adi_board.tcl Outdated Show resolved Hide resolved
projects/scripts/adi_board.tcl Outdated Show resolved Hide resolved
projects/scripts/adi_board.tcl Outdated Show resolved Hide resolved
projects/scripts/adi_board.tcl Outdated Show resolved Hide resolved
@liambeguin
Copy link
Contributor Author

Hi @gastmaier, what do you think of this version?
I got rid of all the indexes, and cleanup a few other minor things. I also tested these changes on a zynq-7000 build.

@gastmaier
Copy link
Contributor

Hi Liam, I liked the changes.
I plan to merge #1054 first since it changes the same file, then yours.
In the next step I will be building your branch on the CI to check if there is no conflict with any project, even though it is extremely unlikely since the index is always 0 and the defaults for the ad_cpu_interconnect didn't change.

Thanks,

@liambeguin
Copy link
Contributor Author

rebased on the lastest branch that includes changes from #1054.

summary of the changes:

$ git range-diff origin/main lvb/ad_hpmx_interconnect ad_hpmx_interconnect 
1:  61c64f6497a0 ! 1:  afee12284523 adi_board: add support for other AXI master interfaces
    @@ projects/scripts/adi_board.tcl: proc ad_mem_hpx_interconnect {p_sel p_clk p_name
      
        global sys_zynq
     -  global sys_cpu_interconnect_index
    +   global use_smartconnect
      
     -  set i_str "M$sys_cpu_interconnect_index"
     -  if {$sys_cpu_interconnect_index < 10} {
    @@ projects/scripts/adi_board.tcl: proc ad_mem_hpx_interconnect {p_sel p_clk p_name
        }
     +  set i_str [format "M%02d" $interconnect_index]
      
    -   set use_smart_connect 1
    -   # SmartConnect has higher resource utilization and worse timing closure on older families
    -@@ projects/scripts/adi_board.tcl: proc ad_cpu_interconnect {p_address p_name {p_intf_name {}}} {
    -     set use_smart_connect 0
    -   }
    - 
     -  if {$sys_cpu_interconnect_index == 0} {
     +  if {$i_str eq "M00"} {
      
    -     if {$use_smart_connect == 1} {
    +     if {$use_smartconnect == 1} {
     -      ad_ip_instance smartconnect axi_cpu_interconnect [ list \
     +      ad_ip_instance smartconnect $interconnect_name [ list \
              NUM_MI 1 \
    @@ projects/scripts/adi_board.tcl: proc ad_cpu_interconnect {p_address p_name {p_in
     -  set_property CONFIG.NUM_MI $sys_cpu_interconnect_index [get_bd_cells axi_cpu_interconnect]
     +  set_property CONFIG.NUM_MI $interconnect_index [get_bd_cells $interconnect_name]
      
    -   if {$use_smart_connect == 0} {
    +   if {$use_smartconnect == 0} {
     -    ad_connect sys_cpu_clk axi_cpu_interconnect/${i_str}_ACLK
     -    ad_connect sys_cpu_resetn axi_cpu_interconnect/${i_str}_ARESETN
     +    ad_connect sys_cpu_clk $interconnect_name/${i_str}_ACLK
2:  81db033ae062 = 2:  cfeb879e32bc adi_board: ad_hpmx_interconnect: add create_bd_addr_seg logs

@gastmaier
Copy link
Contributor

@bia1708 can we trigger the CI on this one? preferably for all projects

@bia1708
Copy link
Collaborator

bia1708 commented Jul 24, 2024

@gastmaier Unfortunately, I can only trigger the CIs for branches inside the analogdevicesinc repo, and not from forks.

liambeguin and others added 3 commits September 4, 2024 14:41
Rework ad_cpu_interconnect to support other master axi interfaces.
Add create_bd_addr_seg logs.

Signed-off-by: Liam Beguin <[email protected]>
The variable sys_zynq is a global var.
Line breaks matter in TCL.

Signed-off-by: Jorge Marques <[email protected]>
With the rework of ad_cpu_interconnect to allow other PS AXI manager
ports, the interconnect IP name now reflects the AXI manager port name.
Rename axi_cpu_interconnect in projects to match the PS AXI manager port
name.

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.

Propagate approval from CI PR #1416

@gastmaier gastmaier merged commit 4971995 into analogdevicesinc:main Sep 4, 2024
2 of 5 checks passed
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.

4 participants