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

BEU <-> HART link added in DTS #2373

Merged
merged 2 commits into from
Mar 28, 2020
Merged

BEU <-> HART link added in DTS #2373

merged 2 commits into from
Mar 28, 2020

Conversation

mhtwn
Copy link
Contributor

@mhtwn mhtwn commented Mar 27, 2020

Related issue:

Type of change: other enhancement

Impact: no functional change

Development Phase:implementation

Release Notes
The CPU section in the generated DTS file will have link to the associated BEU. This should help downstream software flows.

Example of the emitted DTS (updated):

{
...
 model = "SiFive,FU500";
....
 L34: cpus {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	L9: cpu@0 {
 		clock-frequency = <0>;
 		...
                      ...
                      ...
 		device_type = "cpu";
 		....
 		sifive,buserror = <&L8>;
 		....
 	};
 };
 L33: soc {
             ....
             ....
 	L8: bus-error-unit@1700000 {
 		compatible = "sifive,buserror0";
 		interrupt-parent = <&L3>;
 		interrupts = <303>;
 		reg = <0x1700000 0x1000>;
 		reg-names = "control";
 	};
             ....
             ....
  }

@mwachs5
Copy link
Contributor

mwachs5 commented Mar 28, 2020

Can you put an example of the emitted DTS in the PR notes?

@mhtwn
Copy link
Contributor Author

mhtwn commented Mar 28, 2020

Can you put an example of the emitted DTS in the PR notes?

Done. (kept only relevant-to-the-PR content)

@mwachs5
Copy link
Contributor

mwachs5 commented Mar 28, 2020

Thank you! I assume in that example <L3> points to the PLIC, for example?

@mhtwn
Copy link
Contributor Author

mhtwn commented Mar 28, 2020

Thank you! I assume in that example <L3> points to the PLIC, for example?

Yes, that is right.

@@ -88,11 +88,15 @@ class RocketTile private(

val itimProperty = frontend.icache.itimProperty.toSeq.flatMap(p => Map("sifive,itim" -> p))

val beuProperty = bus_error_unit.map(d => Map(
"sifive,buserror0" -> d.device.asProperty)).getOrElse(Nil)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more idiomatic and readable to call .toSeq instead of .getOrElse(Nil).

Also, do we actually want to call this sifive,buserror0 or should it just be sifive,buserror?

Copy link

Choose a reason for hiding this comment

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

It should be not be "sifive,buserror0", but simply be "sifive,buserror" as the version as described in the compatible string.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, i suggested the change, but would like @mhtwn to show what the outputted DTS looks like with this change and get @bsousi5 's final approval before approving the PR

Copy link
Contributor Author

@mhtwn mhtwn Mar 28, 2020

Choose a reason for hiding this comment

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

@mwachs5 its a plain string - it will show up in the dts as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

well i see that string in the DTS above in 2 places. I thought that @bsousi5 was saying in the "compat" he expected it to say sifive,buserror0 but the device type to be sifive,buserror

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, just to not create any confusion, the dts in the release notes (updated above) is how the generated dts looks:

  1. In the cpu component listing: buserror0 is now -> buserror
  2. Compatible field in the 'bus_error_unit' component still lists sifive_buserror0

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks! @bsousi5 if that looks good to you can you please approve the PR? If that's not enough to make it mergable i will approve it

Copy link

@bsousi5 bsousi5 Mar 28, 2020

Choose a reason for hiding this comment

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

I am good with what @mhtwn commented and shown in the release note above.

Copy link

Choose a reason for hiding this comment

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

It seems I don't have the permission to hit the approve button. So @mwachs5, you need to approve.

Choose a reason for hiding this comment

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

I do. So I’ve approved based on your, @richardxia and @mwachs5’s reviews.

@richardxia
Copy link
Member

cc @bsousi5 @nategraff-sifive, who should comment on the actual produced DTS.

BEU name update

Co-Authored-By: Megan Wachs <megan@sifive.com>
@mhtwn mhtwn merged commit 833ec53 into master Mar 28, 2020
@mhtwn mhtwn deleted the beu_dts branch March 28, 2020 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants