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

Fix HWPE subsystem for N_HWPE={0,1} #88

Open
wants to merge 3 commits into
base: ab/merge
Choose a base branch
from

Conversation

LuigiGhionda
Copy link

@LuigiGhionda LuigiGhionda commented Jan 23, 2025

This PR fixes the HWPE subsystem of Astral-based pulp cluster in the following non-default scenarios:

  • N_HWPE = 1:
  • N_HWPE = 0 (HwpePresent=0)

However, it is important to note that the order in which HWPEs are listed in the configuration must reflect the bitmask used in the corresponding HAL function. Otherwise, the HALs will need to be changed. It might be helpful to make the user aware of this requirement somehow.

In addition, it also removes a duplicated call to vopt in the Makefile

@LuigiGhionda LuigiGhionda requested a review from belanoa January 23, 2025 16:30
@LuigiGhionda
Copy link
Author

@ricted98 Since you redesigned the HWPE subsystem for Astral maybe you have some comments/suggestions

@ricted98
Copy link

ricted98 commented Jan 23, 2025

Hi @LuigiGhionda , two questions:

  • What was broken in these scenarios?
  • Does the static mux not work in the "degenerate" case of a single output?

@LuigiGhionda
Copy link
Author

LuigiGhionda commented Jan 23, 2025

@ricted98

* What was broken in these scenarios?

The problem is with hwpe_sel_int signal definiton which fails when N_HWPE=1 (https://github.com/pulp-platform/pulp_cluster/pull/88/files#diff-6bfef46bbce2076251409cb7cccedbf816d10bcf9dbd66fdb552501a62fd7837L69)

* Does the static mux not work in the "degenerate" case of a single output?

Yes, it can handle the degenerate case, but since its select signal is hwpe_sel_int, I thought to separate the two cases in order only to define it for N_HWPE!=1. However, that part could be modified

@ricted98
Copy link

ricted98 commented Jan 23, 2025

So the problem is the +: operator? If so, we could also think of just adding a parameter to use the mux in all cases.
HWPE_SEL_BITS = N_HWPE == 1 ? 1 : $clog2(N_HWPES). What do you think?

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