The title and description don't match the code change!

This is not a full review, just a couple of quick notes.

Add missing whitespace.

Add "enable-methods" property to the check. According to my reading of the
spec, and our code, this should satisfy all cases.

Use ofw_bus_status_okay().

Remove warning message.

In D48122#1097216, @imp wrote:

Do we need to update arch(7)?

Thanks. I am swamped with reviews right now, but I will try to say something on the other open ones before the end of the year.

Approved. Sorry, one final note is that if Sergio doesn't speak on this change, it is just:

Approved by: mhorne (mentor)

Thanks much. I will pull this change today.

In D48058#1096833, @markj wrote:

In D48058#1096826, @mhorne wrote:

I have no objection to the current revision, although we can say a few things about how the code may look/change in the future.

Obviously, the feature-to-ISA-string code will continue to expand/balloon, as we will want to advertise the availability of the many unprivileged ISA extensions to the guest as well.

The problem is that we have no mechanism for reporting extension presence from the kernel to userspace, therefore you are using vm_cap_type here. I am working on something "official" for this, most likely it will be a sysarch exporting a bitmap, with bit definitions compatible to what can be found in Linux. Therefore one call can obtain the full set of supported extensions (sanitized), and we will be able to use this in bhyve to construct the ISA string for the guest.

So I don't think we will need to rely on vm_cap_type in the long run. We also don't need to be precious about the ABI, given the experimental state of RISC-V vmm and hypervisor hardware, and since I intend for this extension reporting stuff to be resolved before the release of 15.0. So this VM_CAP_SSTC can easily be removed later.

I'll just comment that we will probably still want some kind of indirection through vmm to fetch the set of supported extensions, as vmm might want to "mask" some extensions, e.g., because they require some specific support in the hypervisor.

I have no objection to the current revision, although we can say a few things about how the code may look/change in the future.

In D48086#1096806, @concussious.bugzilla_runbox.com wrote:

Thank you. How does it work since this is not doc? Since you have a src bit and have approved it, do I commit this? Also, in general, if you have approved it, do I still wait for Sergio? If not, do I put him as implicit or do I omit from the commit message?

Thanks! This is a good result.

Approved.

Agreed with the change. The message and "Fixes" line should actually point to aa2a0e0fc3114ecb275fba6564d92e94a7039fa9.

Committed in 71924545585, but I listed the original review.

Please, when posting/updating patches, include diff context.

In D47831#1091983, @br wrote:

I managed to convert the driver to newbus but noticed slower performance of NVME when flushing cache using bus_write_8().

• direct access to pointer: 97.5MB/s
• using bus_write_8(): 93.3MB/s

So I provided another mapping for a faster access. WDYT?
another idea is to extract the mapping from sc->res and access it directly?

BTW, I needed to add the following SYS clock in order for the STG driver to attach successfully:

diff --git a/sys/dev/clk/starfive/jh7110_clk_sys.c b/sys/dev/clk/starfive/jh7110_clk_sys.c
index e7b8aa87c48e..d4c9a33e8628 100644
--- a/sys/dev/clk/starfive/jh7110_clk_sys.c
+++ b/sys/dev/clk/starfive/jh7110_clk_sys.c
@@ -65,6 +65,7 @@ static const char *u1_dw_uart_clk_apb_p[] = { "apb0" };
 static const char *u1_dw_uart_clk_core_p[] = { "osc" };
 static const char *u1_dw_sdio_clk_ahb_p[] = { "ahb0" };
 static const char *u1_dw_sdio_clk_sdcard_p[] = { "axi_cfg0" };
+static const char *usb_125m_p[] = { "pll0_out" };
 static const char *u2_dw_uart_clk_apb_p[] = { "apb0" };
 static const char *u2_dw_uart_clk_core_p[] = { "osc" };
 static const char *u3_dw_uart_clk_apb_p[] = { "apb0" };
@@ -127,6 +128,7 @@ static const struct jh7110_clk_def sys_clks[] = {
 	    u0_dw_sdio_clk_sdcard_p, 15),
 	JH7110_GATEDIV(JH7110_SYSCLK_SDIO1_SDCARD, "u1_dw_sdio_clk_sdcard",
 	    u1_dw_sdio_clk_sdcard_p, 15),
+	JH7110_DIV(JH7110_SYSCLK_USB_125M, "usb_125m", usb_125m_p, 15),

Thanks for handling this, I was beginning to experiment with the USB controller and need the STG clocks for it.

Rebased from D35593, but this is basically unchanged.

Thanks!

Same notes here: this looks fine as a glue driver, but I am not in the best position to evaluate PCIe. I hope you will monitor the upstreaming of linux/u-boot to eventually improve the magic initialization code.

Seems pretty straightforward.

I have never written or studied an sdhci driver in depth. Maybe @manu would like to look.

I strongly think this should be a newbus device driver. Unless there is some urgent need that it should be set up at SI_SUB_CPU?

In D47854#1090987, @jrtc27 wrote:

Honestly we should instead be removing the use of intptr_t for hwreset, it's not a sensible type to use for an ID.

Makes sense to me.

LGTM

FYI, the commit message body should be wrapped at 72 characters. I don't know what you are using here but it appears too short.

I noticed that when viewing the commit, phabricator very helpfully lists which branches the commit is present on. Easy to compare against what you have here.

Maybe you saw/figured this out, but the website/tools/hardware-notes-processor.rb gives a warning for all the pages it can't find. A test run comparing the template notes against main yielded:

Processing hardware notes...
WARNING: The manual page adv does not exists
WARNING: The manual page adw does not exists
WARNING: The manual page aha does not exists
WARNING: The manual page man4.i386/aic does not exists
WARNING: The manual page amr does not exists
WARNING: The manual page bt does not exists
WARNING: The manual page dpt does not exists
WARNING: The manual page esp does not exists
WARNING: The manual page iir does not exists
WARNING: The manual page mly does not exists
WARNING: The manual page ncr does not exists
WARNING: The manual page ncv does not exists
WARNING: The manual page nsp does not exists
WARNING: The manual page stg does not exists
WARNING: The manual page trm does not exists
WARNING: The manual page twa does not exists
WARNING: The manual page vpo does not exists
WARNING: The manual page de does not exists
WARNING: The manual page ed does not exists
WARNING: The manual page man4.i386/ep does not exists
WARNING: The manual page man4.i386/ex does not exists
WARNING: The manual page man4.i386/fe does not exists
WARNING: The manual page hme does not exists
WARNING: The manual page pcn does not exists
WARNING: The manual page sf does not exists
WARNING: The manual page sn does not exists
WARNING: The manual page tl does not exists
WARNING: The manual page txp does not exists
WARNING: The manual page man4.i386/vx does not exists
WARNING: The manual page wb does not exists
WARNING: The manual page xe does not exists
WARNING: The manual page man4.i386/ce does not exists
WARNING: The manual page man4.i386/cx does not exists
WARNING: The manual page man4.i386/cp does not exists
WARNING: The manual page man4.i386/ctau does not exists
WARNING: The manual page rc does not exists
WARNING: The manual page snd_ad1816 does not exists
WARNING: The manual page snd_ds1 does not exists
WARNING: The manual page snd_ess does not exists
WARNING: The manual page snd_gusc does not exists
WARNING: The manual page snd_maestro does not exists
WARNING: The manual page snd_mss does not exists
WARNING: The manual page snd_sbc does not exists
WARNING: The manual page bktr does not exists
WARNING: The manual page ng_bt3c does not exists
WARNING: The manual page ubsec does not exists