opentitan icon indicating copy to clipboard operation
opentitan copied to clipboard

Published 20 hours ago • verified publisher icon lowRISC

[prim] Remove prim_clock_gp_mux2

Open rswarbrick opened this issue 9 months ago • 3 comments

This is two commits, to allow us to merge the bug fix that found the dead primitive in the first place. The first commit is also in #22964, which should probably be merged first. The second commit has the following message:

[prim] Remove prim_clock_gp_mux2

This prim actually had a bug in a generator function (found and fixed by jwnrt) and wasn't used anywhere. There's no real reason to keep the module around, especially since it hasn't been used since it was added in 2022. Remove it.

rswarbrick avatar May 03 '24 16:05 rswarbrick

Before we can merge this, we need to ensure it's also not used on the closed source side, e.g., in any of the closed source wrappers or the closed-source AST implementation. At this point, I would advise not removing stuff - even if it's not used on the open source side.

Even with the bug fixed by @jwnrt , the IP might have worked just fine in the with the tooling used on the closed source side.

vogelpi avatar May 06 '24 07:05 vogelpi

FYI, the questionable primitive got added in #13152 and was discussed / reviewed by people with visibility into the closed source side. I'll bring this up in a meeting with the PD team and will report back. Until then, please feel free to merge @jwnrt 's bugfix but please wait with this one @rswarbrick .

vogelpi avatar May 06 '24 08:05 vogelpi

Cool, that makes sense. I'm going to rebase to fix the merge conflict, but I won't hit "merge" until that's sorted out.

rswarbrick avatar May 07 '24 08:05 rswarbrick

Hi @vogelpi! Have you had a chance to talk to the PD team?

rswarbrick avatar Jun 19 '24 08:06 rswarbrick

FYI, the questionable primitive got added in #13152 and was discussed / reviewed by people with visibility into the closed source side. I'll bring this up in a meeting with the PD team and will report back. Until then, please feel free to merge @jwnrt 's bugfix but please wait with this one @rswarbrick .

For what it's worth, I suspect this was on a TODO list because of AST, but the prim never got used: https://github.com/lowRISC/opentitan/pull/8848#issuecomment-960309689

It would still be good to check with the closed-source team.

a-will avatar Jun 19 '24 14:06 a-will

@vogelpi: Repeating the question, can we drop this primitive once we take the branch on master? Or is there any evidence that it is needed somewhere in the closed source?

rswarbrick avatar Sep 04 '24 13:09 rswarbrick

Thanks for the reminder @rswarbrick , I've now reached out to the PD team to ask.

vogelpi avatar Oct 01 '24 15:10 vogelpi

Hi @rswarbrick , the PD team confirmed that the primitive is not used in the closed source and we can remove it.

vogelpi avatar Oct 07 '24 07:10 vogelpi