-
Notifications
You must be signed in to change notification settings - Fork 586
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
Allow BoringUtils.bore to work on probes #3908
Conversation
Extends BorginUtils.bore (and related) to work where the source is a probe. Currently, if a user tries to do this, they will get an error about being unable to probe a probe because the old code would unconditionally wrap whatever the source was in `Probe`/`RWProbe`. Change this to not wrap if the source is already a probe. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a useful extension, although I'm a little uncertain about this method's behavior in all the various cases.
Don't see this breaking things that work today AFAIK, so LGTM.
case None => purePortTypeBase | ||
case Some(pi) => | ||
// If the source is already a probe, don't double wrap it in a probe. | ||
purePortTypeBase.probeInfo match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things get strange if the bore/tap target is mixed probe/non-probe, but that's already true.
(or if has incompatible probe type vs requested kind)
Not sure what the errors look like if this is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not something to be addressed here unless it seems easy/reasonable to address while visiting (?), just a note.
case None if pi.writable => RWProbe(purePortTypeBase) | ||
case None => Probe(purePortTypeBase) | ||
} | ||
case None => purePortTypeBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the below logic maybe can/should be tweaked, not sure and maybe better to defer that to a later simplification.
For example, if the target is a rwprobe port (new L251, old L246), and this is used by rwTap, it'll add another RWProbe for that. Possibly error but haven't traced all the paths here...
Extends BorginUtils.bore (and related) to work where the source is a probe. Currently, if a user tries to do this, they will get an error about being unable to probe a probe because the old code would unconditionally wrap whatever the source was in `Probe`/`RWProbe`. Change this to not wrap if the source is already a probe. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com> (cherry picked from commit 67f51b8)
Extends BorginUtils.bore (and related) to work where the source is a probe. Currently, if a user tries to do this, they will get an error about being unable to probe a probe because the old code would unconditionally wrap whatever the source was in `Probe`/`RWProbe`. Change this to not wrap if the source is already a probe. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com> (cherry picked from commit 67f51b8) Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Extends BorginUtils.bore (and related) to work where the source is a probe. Currently, if a user tries to do this, they will get an error about being unable to probe a probe because the old code would unconditionally wrap whatever the source was in
Probe
/RWProbe
. Change this to not wrap if the source is already a probe.Release Notes
Allow boring from a source which is already a probe.