SCMI support

[br]

Summary

SCMI v3.1 support described in DEN0056D_System_Control_and_Management_Interface_v3_1-6.pdf

This implements Shared Memory Transfer type only for now.

Test Plan

Works on ARM Morello platform (Controls HDMI pixel clock frequency.)

[br]

add a check

[andrew]

I think the extres changes should be split out to a new file.

Can you add scmi_ to the start of all the scmi files. The build system puts all the .o files in the same directory so we need to make sure the source files have a unique name.

sys/arm64/scmi/scmi_protocols.h

2

Where did you get this file from?

[br]

Use MAX_SETTINGS macro from cpu.h.
Split-out extres/clk changes.

[br]

In D37316#848047, @andrew wrote:

I think the extres changes should be split out to a new file.

Can you add scmi_ to the start of all the scmi files. The build system puts all the .o files in the same directory so we need to make sure the source files have a unique name.

Yes, I can add. I only worry that those files are generic drivers used by SCMI. They are not part of SCMI spec I think. I.e. doorbell, cpufreq and mmio.

[br]

sys/arm64/scmi/scmi_protocols.h

2

This file comes from u-boot. I extended it with a several structs and macroses.
But I can re-implement this file completely if needed.

[jrtc27]

Your stack is the wrong way round; this needs to depend on D37325 not the other way round

sys/arm64/scmi/dt_cpufreq.c

77 ↗

(On Diff #112839)

Should this really call itself an SCMI driver and live under sys/arm64/scmi when it's completely generic to whatever implements the clock methods?

98 ↗

(On Diff #112839)

If the device has no clock you really shouldn't have added this child in dt_cpufreq_identify, it'll just spew errors to the console

sys/arm64/scmi/scmi_clk.c

423

DEVMETHOD_END

sys/arm64/scmi/scmi_perf.c

503 ↗

(On Diff #112839)

DEVMETHOD_END

512 ↗

(On Diff #112839)

Does this one really need to be early?

[manu]

sys/arm64/scmi/dt_cpufreq.c

77 ↗

(On Diff #112839)

Agreed, also it's confusing to have both cpufreq_dt (from sys/dev/cpufreq/cpufreq_dt.c) and dt_cpufreq

[br]

o Remove perf protocol for now.
o Rewrite protocols.h.
o Rename doorbell.c to arm_doorbell.c, mmio.c to mmio_sram.c

[jrtc27]

sys/arm64/scmi/scmi.h

71

void *? Then you don't need to cast all the time.

sys/conf/files.arm64

93–94

This isn't the upstream context, this is with drm-subtree applied

[manu]

sys/arm64/intel/firmware.c

63 ↗

(On Diff #113611)

I was not aware of this driver.
It the point to attach to a node named "firmware" but only for some SoC ?
I couldn't find any node with the above compatible in sys/contrib/device-tree so I can't really check.
Anyway, this doesn't seems to do the proper thing and I've made a ofw_firmware driver for my ZynqMP work that should work for you, I'll open a review later today for this part (other ZynqMP part isn't ready yet).

[br]

Change in/out buf data types.
Remove unrelated code from files.arm64.

[br]

Restore firmware.c change.

[jrtc27]

sys/arm64/scmi/scmi.c

66

phandle_t *

76–82

Why not just check against the len returned by OF_getencprop_alloc_multi?

127

You shouldn't need these casts

sys/arm64/scmi/scmi.h

71

const?

80

const?

[manu]

sys/arm64/intel/firmware.c

63 ↗

(On Diff #113611)

Done in https://reviews.freebsd.org/D37612

[br]

o Make in_buf const;
o Remove unneeded casts;
o use len from OF_getencprop_alloc_multi().
o Remove firmware.c changes due to new firmware driver.

sys/arm64/intel/firmware.c

63 ↗

(On Diff #113611)

Great thank you!

[br]

o Fix length check;
o Switch to ofw_firmware bus.

[manu]

I don't think that this should live under sys/arm64/scmi.
I have sys/dev/firmware/xilinx coming soon-ish, and I think that all firmware related drivers should be under sys/dev/firmware so maybe put this under sys/dev/firmware/arm/

[manu]

Same thing for the doorbell part, creating a new sys/dev/mailbox/<vendor> might be the right way, we already have some stuff for broadcom that should go there and a lot of other SoCs have mailbox stuff.

[manu]

BTW I would argue that part of the doorbell driver should be generic, a new interface is something that would be needed and some generic function to get the doorbell device based on the mbox-names/mboxes DT properties should be written.

[manu]

sys/arm64/scmi/mmio_sram_if.m

34

This doesn't seems scmi specific so maybe put this somewhere else ?

[manu]

sys/arm64/scmi/mmio_sram.c

115

You should release the resources in sc->res here

159

Why was interrupt chosen ?
I think that SUPPORTDEV should fit better for mmio-sram

sys/arm64/scmi/scmi.c

274

you can use simplebus instead of ofw_firmware.
Does it needs to be this early ? Other cpufreq driver don't use the EARLY_ macros.

[br]

In D37316#855385, @manu wrote:

I don't think that this should live under sys/arm64/scmi.
I have sys/dev/firmware/xilinx coming soon-ish, and I think that all firmware related drivers should be under sys/dev/firmware so maybe put this under sys/dev/firmware/arm/

That directory contains 23-25 y.o. source code and a README file for FreeBSD 5x.
@andrew what do you think where SCMI code should live ?

[br]

In D37316#856924, @br wrote:

In D37316#855385, @manu wrote:

I don't think that this should live under sys/arm64/scmi.
I have sys/dev/firmware/xilinx coming soon-ish, and I think that all firmware related drivers should be under sys/dev/firmware so maybe put this under sys/dev/firmware/arm/

That directory contains 23-25 y.o. source code and a README file for FreeBSD 5x.
@andrew what do you think where SCMI code should live ?

Or sorry misspelled with firewire

[br]

Address @manu's comments. Thanks!

[manu]

,>>! In D37316#856933, @br wrote:

Address @manu's comments. Thanks!

Mhm, where ? (edit: meh, damn phab sucks, looks better, will have a better look tomorow)

[manu]

Still thinks that mmio_sram should be elsewhere, it's not firmware related and not arm (tm) specific.

sys/conf/files.arm64

213

optional fdt mmio_sram ?

215

optional fdt scmi ?

320

optional fdt arm_doorbell ?

[br]

Fix "optional" arguments.

[br]

In D37316#857228, @manu wrote:

Still thinks that mmio_sram should be elsewhere, it's not firmware related and not arm (tm) specific.

I can suggest sys/dev/misc or sys/dev/sram

[manu]

In D37316#857280, @br wrote:

In D37316#857228, @manu wrote:

Still thinks that mmio_sram should be elsewhere, it's not firmware related and not arm (tm) specific.

I can suggest sys/dev/misc or sys/dev/sram

sys/dev/sram is fine for me.
Don't forget to add the devices to the proper std.XXX in sys/arm64/conf too

[br]

Move MMIO-SRAM device to dev/sram. Add SCMI related devices to config.

[manu]

sys/dev/mailbox/arm/arm_doorbell.c

180 ↗

(On Diff #114292)

Either free the resources or just return EBUSY, it's unlikely that we will detach this device.

220 ↗

(On Diff #114292)

You should free(cells, M_OFWPROP) in the error paths below.

sys/dev/mailbox/arm/arm_doorbell.h

36 ↗

(On Diff #114292)

Leftover from earlier code ?

[br]

Address Manu's comments on ARM Doorbell:
o Release resources on detach
o Free cells in error paths
o Remove unneeded M_DOORBELL

[br]

Forgot to re-generate patch

[manu]

sys/dev/mailbox/arm/arm_doorbell.c

184 ↗

(On Diff #114296)

Sorry but we still don't have OF_device_unregister (I think I have a review open for this) so detach for this driver doesn't make sense, just return EBUSY.

234 ↗

(On Diff #114296)

And sorry again the proper way to do this is OF_free_prop like you did in scmi files.

[manu]

Haven't fully reviewed this but this looks good enough to be commited (after my last two comments) and if there is any problems we can take care of those in the tree.
Thanks.

[br]

ARM Doorbell:

o Return EBUSY on detach
o use OF_prop_free() instead of free(cells, M_OFWPROP);

[manu]

Thanks.