[top_darjeeling] Use FuseSoC mapping for Darjeeling-specific pwrmgr dependency

[qmn]

Ridding the TODO, thanks to #23555!

[a-will]

Do we need a virtual core here? The specific implementation should be an ipgen output with a unique VLNV. Does that not work for the use case? (In other words, you can just delete the two commented out lines in the core file. They suggest a direction that seems to just unnecessarily restrict pwrmgr's use.)

[a-will]

soc_dbg_ctrl also doesn't generically depend on just any pwrmgr_pkg. It specifically must depend on darjeeling's, since it assumes a particular parameterization of pwrmgr (see how boot_status_i makes assumptions about the width of those struct members that depend on parameters). That would need to be untangled to have something usable for other tops.

[qmn]

you can just delete the two commented out lines in the core file

Thanks for having a look! The dependence on boot_status_i does exist, so it seems appropriate to state that dependence in the core file -- perhaps I'm looking at the wrong lines?

To your second comment: soc_dbg_ctrl does depend specifically on Darjeeling's parameterization of the pwrmgr_pkg. In addition to what you said, the presence of boot_status_i itself is guarded on wait_for_external_reset, currently only set to true for Darjeeling.

I guess I mean to say that it's nice to make soc_dbg_ctrl not necessarily specific to Darjeeling (I say this knowing fully well that soc_dbg_ctrl is not really meaningful in the other two tops).

[a-will]

Essentially, I think we've gone about pwrmgr's ports all wrong. Generally, ipgen'd cores should not be exporting types through packages with fixed names. We also should not be creating structs with parameterizable members and exporting that as a required type for port interfaces (especially not in said packages).

This all creates reuse problems, and it was done merely for the convenience of reducing the number of ports. It's a poor design, however, if you want to support multiple integrations with different configurations.

Because soc_dbg_ctrl necessarily depends on Darjeeling's specific ipgen output, there's no apparent reason to jump straight into defining another virtual core, and we should really avoid defining virtual cores (due to the restrictions it places on the number of configurations that can be present in a design partition).

If you happen to have an out-of-tree top that uses soc_dbg_ctrl (but it has a different name for its ipgen'd outputs), then you can use the mapping feature to override lowrisc:darjeeling_ip:pwrmgr_pkg.

Otherwise, before defining another virtual core, I think we ought to see a more careful definition of interfaces between pwrmgr and other cores, and soc_dbg_ctrl should become more flexible to handle those. I'd suggest, though, that these problems stem from how pwrmgr's ports are organized, and if that were fixed, none of this interface virtualization and additional flexibility would be necessary.

Does that make sense to you? (hopefully I haven't assumed more capability in fusesoc's mapping feature than is present, too...)

[a-will]

It would help some if topgen could deconstruct the structs and make available members for connectivity, though 🤔

[qmn]

Thanks for all this! It makes sense. To be concrete, one way forward might be just to break out the pwr_boot_status_t struct into its constituent members, and then put in an ordinary SV parameter (at the soc_dbg_ctrl and pwrmgr levels, and now visible to topgen) for the width of the rom_ctrl_status member. Seems that the only downside is that the port list gets a bit longer.

[a-will]

That's right! If you happen to want to do some refactoring in the near term, this month would be the time. soc_dbg_ctrl would go through a freeze for sign-off in August.

@Razer6 for visibility. I personally think "exploding" the struct into individual ports and removing the dependency on pwrmgr_pkg would be best for the soc_dbg_ctrl IP, but I'd defer to Robert here. This struct is also only consumed by soc_dbg_ctrl, so it shouldn't cause widespread impact to change it.

[Razer6]

Thanks @a-will for pulling me in.

Thanks for all this! It makes sense. To be concrete, one way forward might be just to break out the pwr_boot_status_t struct into its constituent members, and then put in an ordinary SV parameter (at the soc_dbg_ctrl and pwrmgr levels, and now visible to topgen) for the width of the rom_ctrl_status member. Seems that the only downside is that the port list gets a bit longer.

Agreed. Although the port list in terms of SV code is longer, the actual hardware and wiring is the same. However, it allows for a generic parametrization with existing language features. Further, the connectivity is done by topgen, so no need to worry about the larger port list.

If you can make a PR with those changes soon, it would be great for the sign-off timeline.

[a-will]

It seems like the problem here is the way pwr_boot_status_t is defined. Generally, we should not create structs for port types if they have members with widths that depend on a parameter.

[qmn]

To seek clarification on your second statement: is that to say that rom_ctrl_status (the parameterizable member in question) should be a separate block-level port?

[a-will]

To seek clarification on your second statement: is that to say that rom_ctrl_status (the parameterizable member in question) should be a separate block-level port?

Yup! This came up in the multi-top WG meeting, and that's the direction we've started heading towards. The structs can be convenient for reducing the number of ports, but members that depend on parameters are problematic for reuse.

[Razer6]

Yes, I agree with that. Moving out the parametrizable part of the struct would make the overall connectivity easier and the design more parametrizable with existing SV features.