Skip to content

Axi4 passive vip and scoreboard#356

Closed
csabakiss-semify wants to merge 4 commits intolowRISC:mainfrom
csabakiss-semify:axi4_passive_vip
Closed

Axi4 passive vip and scoreboard#356
csabakiss-semify wants to merge 4 commits intolowRISC:mainfrom
csabakiss-semify:axi4_passive_vip

Conversation

@csabakiss-semify
Copy link
Copy Markdown
Collaborator

  1. Developed AXI4 VIP supporting passive mode only
  2. VIP integrated to the top level environment
  3. Added scoreboard to the AXI XBAR

@martin-velay
Copy link
Copy Markdown
Contributor

Thanks Csaba, before I dive into the code itself, I have some general comments. We try to make our database being more or less homogeneous with the rest of lowRISC code. My comments are a bit random, it's what I can see from a first glance:

  • Add header to each files like:
    // Copyright lowRISC contributors (COSMIC project).
    // Licensed under the Apache License, Version 2.0, see LICENSE for details.
    // SPDX-License-Identifier: Apache-2.0
  • Rename axi4_vip_config into axi4_vip_cfg
  • Rename axi4_vip_transaction into axi4_vip_item
  • Adopt external methods as much as possible like what I've done for the HMAC for example
  • Add labels at the end of each function/task/class
  • Maybe remove the UVC directory
  • The short for scoreboard is mostly scb (and not scbd)
  • The name of top_chip_dv_axi_scbd should be top_chip_dv_axi_scoreboard
  • The end else begin of if statements should be on the same line
  • It would be better to have all in lower cases here m_axi_MST0
  • The typedef are always names with _t, so this t_axis should become axis_t
  • When you name your commits you need to give between the [ ] the related block or thing you do. It can be the name of the block followed by the domain, like: [axi,dv], [axi,rtl], [top,dv], [top,script]... If you have a fix you should amend your commit. And your commit list needs to be logically arranged as most of the people will review it commit by commit in order.

I think it's enough to start with. You can also take a look at this repository and maybe ask AI to help you fixing some more things I haven't spotted yet: https://github.com/lowRISC/style-guides/tree/master

Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>
Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>
Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>
Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>
@csabakiss-semify
Copy link
Copy Markdown
Collaborator Author

I believe, the requested changes are applied. I do not know why the lint check fails. Please letme know if further modifications are required.

@martin-velay
Copy link
Copy Markdown
Contributor

martin-velay commented Mar 25, 2026

I believe, the requested changes are applied. I do not know why the lint check fails. Please letme know if further modifications are required.

Thanks Csaba, I'll take a look at the PR more in depth today.
About the CI you can see the reason by clicking here:
image

And then you'll see:
image

@csabakiss-semify
Copy link
Copy Markdown
Collaborator Author

I mean, I can see the error message, but the referenced file has exactly the same header as the others.

Copy link
Copy Markdown
Contributor

@martin-velay martin-velay left a comment

Choose a reason for hiding this comment

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

So far I have reviewed all the files under top_chip/dv except the scoreboard.

@csabakiss-semify
Copy link
Copy Markdown
Collaborator Author

Also, I rebased to the latest main branch and uart_smoke test fails due to a AXI scoreboard issue. Checking...

@csabakiss-semify csabakiss-semify closed this by deleting the head repository Mar 30, 2026
@csabakiss-semify
Copy link
Copy Markdown
Collaborator Author

Due to mocha repo went public, I lost the connection to the origin, therefore I indirectly closed this PR by deleting the head repo. New PR is expected soon with all the requested changes and passing tests.

@martin-velay
Copy link
Copy Markdown
Contributor

New PR is available here #391

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