RISCV Platform Specific Code

[nick]

Summary

Initial platform specific code for RISC-V. This currently handles a few
platform specific functions including:

• platform_probe_and_attach() for finding the best-fit platform.
• platform_devmap_init() for setting up platform specific static device

mappings

• platform_mp_setmaxid() for probing available cpu's
• platform_late_init() plaform specific function that executes after

cninit()

Sponsored by: Axiado

[mhorne]

Hi Nick, thanks for taking the time to submit this. To start, could you please update your diffs with full context? (e.g. git show -U99999)

[mhorne]

Okay, I'm not totally opposed to this interface, but I would like to better understand the motivation behind adopting it before we do so.
Does this solve a problem for some proprietary core? As I see it, this doesn't change anything for the FU540.

The arm directory is littered with these platform files, and this seems to be out of necessity due to the variety and inconsistency of standards that exists in its hardware. arm64 seems to have avoided the need for this interface so far, and I wonder if we can do the same. The Linux RISC-V world seems to be pushing hard for OpenSBI and keeping platform abstractions at the firmware level. We don't have to follow what Linux does, but I think we ought to be thoughtful in what we choose to do here. Thoughts?

[nick]

In D23877#525950, @mhorne wrote:

Okay, I'm not totally opposed to this interface, but I would like to better understand the motivation behind adopting it before we do so.
Does this solve a problem for some proprietary core? As I see it, this doesn't change anything for the FU540.

The arm directory is littered with these platform files, and this seems to be out of necessity due to the variety and inconsistency of standards that exists in its hardware. arm64 seems to have avoided the need for this interface so far, and I wonder if we can do the same. The Linux RISC-V world seems to be pushing hard for OpenSBI and keeping platform abstractions at the firmware level. We don't have to follow what Linux does, but I think we ought to be thoughtful in what we choose to do here. Thoughts?

It's definitely a fair point. Recent moves to OpenSBI has offloaded a lot of the platform specific code. Undoubtedly it's worth being thoughtful here as it will drive some of our RISC-V kernel architecture moving forward.

My justification for upstreaming platform specification in FreeBSD mainly stems from 2 things:

1. At Axiado we have use cases for platform specific devmap entries in which platform_devmap_init() seems the most flexible.
2. SiFive's cpu_check_mmu() doesn't necessarily harm our current design but it does seem a bit "inelegant" to force SiFive specific code into the general kernel and moving forward I'd argue that it's worth avoiding making compromises for one platform - especially considering there's only one supported RISC-V platform and we're already making a "compromise". I'd worry that for every platform we'd add another "compromise" until we're littered with little things like that everywhere.

Again, I'm not set on this interface being the only option, especially with OpenSBI having similar support. I definitely see why we might prefer to avoid this design. I'm curious what others think that have seen more architecture specific development progression over time.

[philip]

I agree that we should be careful not to go down the path of ARM with a plethora of platform files littering the tree. While this patch makes that easier, it doesn't actually force us in that direction.

I like the idea of having a "default" RISC-V platform that isn't vendor-specific. Currently the default RISC-V platform smells a lot like FU540. Which isn't necessarily wrong -- it *is* the only commercially available RISC-V platform to date -- but having an empty default platform could encourage people to keep more of their abstractions in firmware, where they belong.

[mhorne]

In D23877#525989, @nickisobrien_gmail.com wrote:

In D23877#525950, @mhorne wrote:

Okay, I'm not totally opposed to this interface, but I would like to better understand the motivation behind adopting it before we do so.
Does this solve a problem for some proprietary core? As I see it, this doesn't change anything for the FU540.

The arm directory is littered with these platform files, and this seems to be out of necessity due to the variety and inconsistency of standards that exists in its hardware. arm64 seems to have avoided the need for this interface so far, and I wonder if we can do the same. The Linux RISC-V world seems to be pushing hard for OpenSBI and keeping platform abstractions at the firmware level. We don't have to follow what Linux does, but I think we ought to be thoughtful in what we choose to do here. Thoughts?

It's definitely a fair point. Recent moves to OpenSBI has offloaded a lot of the platform specific code. Undoubtedly it's worth being thoughtful here as it will drive some of our RISC-V kernel architecture moving forward.

Agreed. One thing I think is unclear is how OpenSBI will fare when we start to see more platforms emerge. They provide a space for vendor-specific SBI extensions, and make it "easy" to port to new platforms, but we will have to see if vendors actually go for that. I think some of our kernel architecture for RISC-V will be driven by how well the SBI is adopted.

My justification for upstreaming platform specification in FreeBSD mainly stems from 2 things:

1. At Axiado we have use cases for platform specific devmap entries in which platform_devmap_init() seems the most flexible.

I thought this may have been a motivator. I'd suggest you be careful with this interface. From the discussion I had with a couple of the arm maintainers, they claim that the majority of its use is no longer necessary, save a few key devices that aren't initialized properly by the boot firmware. It was also their suggestion that we avoid using devmap on RISC-V if we can, since it is a legacy and pretty inflexible interface.

If you are able, can you describe the device(s) you are using devmap for and why it is required? I see how it can be useful for prototyping but IMO it's not a great long-term solution. Note that arm64 added support for devmap, but has avoided using it completely.

2. SiFive's cpu_check_mmu() doesn't necessarily harm our current design but it does seem a bit "inelegant" to force SiFive specific code into the general kernel and moving forward I'd argue that it's worth avoiding making compromises for one platform - especially considering there's only one supported RISC-V platform and we're already making a "compromise". I'd worry that for every platform we'd add another "compromise" until we're littered with little things like that everywhere.

I agree with you in the general case, we don't want platform specific hacks scattered around the kernel, it's better to keep them in one location. I believe that "mmu-type" is intended to be a standard property in all RISC-V device trees going forward, and we could reliably check it for all platforms. See: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/riscv/cpus.yaml

Again, I'm not set on this interface being the only option, especially with OpenSBI having similar support. I definitely see why we might prefer to avoid this design. I'm curious what others think that have seen more architecture specific development progression over time.

I'd like to move forward with this patch-set for now, with the agreement that we will be careful about how the platform interface gets used. @br do you have an opinion?

[nick]

Thanks for taking the time to think about it @mhorne and @philip! It's important to have these discussions. More flexibility doesn't necessarily equal "better".

One thing I think is unclear is how OpenSBI will fare when we start to see more platforms emerge. They provide a space for vendor-specific SBI extensions, and make it "easy" to port to new platforms, but we will have to see if vendors actually go for that. I think some of our kernel architecture for RISC-V will be driven by how well the SBI is adopted.

Yeah, I'm also really curious about how SBI is going to be implemented by vendors and it will undoubtedly drive our kernel development in regards to platform specificity moving forward. I'd probably prefer to see most of those "hacks" taken care of in SBI but it's not quite clear whether that's the case today.

agree with you in the general case, we don't want platform specific hacks scattered around the kernel, it's better to keep them in one location. I believe that "mmu-type" is intended to be a standard property in all RISC-V device trees going forward, and we could reliably check it for all platforms. See: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/riscv/cpus.yaml

Yeah, there's nothing necessarily harmful in this check from what I can tell. And as I said, at Axiado it's not harming us, but as Philip pointed out:

Currently the default RISC-V platform smells a lot like FU540.

It's more the principal that concerns me here rather than the direct impact of this specific check. I understand that you agree with this as well.

Lastly, I think the devmap discussion is important, and if we want to drop devmap completely there's a discussion to be had. I could see myself agreeing with your point that we should avoid it in RISC-V, but I think it's fair to keep that discussion separate as this patch is a consumer of devmap rather than the implementation of it. (Don't get me wrong, I appreciate that point and the information you're passing from the discussions with some of the ARM maintainers!).

with the agreement that we will be careful about how the platform interface gets used.

Definitely an important statement. We should learn from ARM here and be wary of the direction this takes. I'd prefer to keep everything in a "default platform" but I also like knowing we have the comfort of an elegant option to overload that if truly necessary.

Thanks again for the discussion and considerations!

[nick]

• Silence warning if PLATFORM isn't set
• Remove unnecessary headers in mp_machdep.c

[philip]

It sounds like we're reaching consensus that we should move forward with this.

I'll do another build test before committing this patch-set later today. Unless someone screams. :)

[br]

Hi Nicholas. I'm doing efforts to figure out why this is needed.
Why cpu_check_mmu() does not work for you and what are specific devmap use cases you have at Axiado?

[br]

In D23877#527473, @philip wrote:

It sounds like we're reaching consensus that we should move forward with this.

I'll do another build test before committing this patch-set later today. Unless someone screams. :)

I probably missed something, but it looks like everyone agrees that this code is temporary and we should avoid this in future and remove devmap too. Then why we are going to get this in tree now?

[nick]

Hi Ruslan,

Why cpu_check_mmu() does not work for you?

There's nothing harmful in this check long term, but from what I can tell, the reason we do that check is specific to the FU540 which, in principal, in my opinion, shouldn't be in the base kernel.

What are specific devmap use cases you have at Axiado?

My original motivation for this patch was devmap related and I was using it to ease a future PMP feature, but that's been reconsidered. I honestly don't think devmap is that relevant to this patch's viability, and I'm happy to remove those bits if we decide to drop devmap in RISC-V.

but it looks like everyone agrees that this code is temporary and we should avoid this in future and remove devmap too

Personally, I didn't necessarily take from this discussion that this code was temporary, but that we should use the default platform as much as possible but still provide a nice way to overload any of those functions for future platforms if truly needbe.

Again, my points are just points. I'm not set on them being strong enough to justify or condemn the patch, but simply points to consider. Thanks again to everyone who has participated in this discussion!

[kp]

In D23877#527818, @nickisobrien_gmail.com wrote:

but it looks like everyone agrees that this code is temporary and we should avoid this in future and remove devmap too

Personally, I didn't necessarily take from this discussion that this code was temporary, but that we should use the default platform as much as possible but still provide a nice way to overload any of those functions for future platforms if truly needbe.

I think I share your position.

If we could predict the future we'd know what varieties of RISCV platform we'd have to deal with, and we'd know if this will help or not. Sadly I have misplaced my time machine.
I hope to discuss this with manu@ in a few days to see if his ARM experience can teach us something.

[manu]

So I'm not really familiar wih risc-v eco system (if any exists) but maybe it's more safe to wait for some more risc-v SoC to show up and see if that kind of platform code is needed ?
Someone with an fpga board that have ddr3 could try to put litex (https://github.com/enjoy-digital/litex) on it and boot FreeBSD on it to see what would be needed.
On arm for some SoC we could just have a generic platform that do PSCI for cpu bringup, we don't need devmap_* if everything is done correctly.
From what I can see from sys/riscv/riscv/mp_machdep.c the code just iterate on /cpus node and do it's magic if the core have an mmu, so PLATFORM will not bring any benefit for cpu bringup unless things change in the ecosystem but I guess that in that case they will add some properties to reflect how to do cpu bring up (like enable-methods on arm/arm64).
Do you have any use for devmap_* yet ?