Expand OpenFirmware API with ofw_bus_node_status_okay method
ClosedPublic
Actions

Authored by mw_semihalf.com on Jan 17 2017, 3:58 PM.

Details

Reviewers

nwhitehorn
zbb
jhibbits
fabient
imp
mmel
• ian

Commits

rS312745: Expand OpenFirmware API with ofw_bus_node_status_okay

Summary

Method could be used before we can access device_t structure.
As per simple phandle_t handle we can access FDT to check
if specified node has been enabled.
It will be used in Marvell's common configuration code.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mw_semihalf.com updated this revision to Diff 24122.Jan 17 2017, 3:58 PM

mw_semihalf.com retitled this revision from to Expand OpenFirmware API with ofw_bus_node_status_okay method.

mw_semihalf.com updated this object.

mw_semihalf.com edited the test plan for this revision. (Show Details)

mw_semihalf.com added reviewers: • ian, imp, zbb, fabient.

mw_semihalf.com set the repository for this revision to rS FreeBSD src repository - subversion.

mw_semihalf.com added a subscriber: ARM.

mw_semihalf.com added a child revision: D9219: Fix node detection for MBUS windows configuration.Jan 17 2017, 4:00 PM

andrew added reviewers: nwhitehorn, jhibbits.Jan 17 2017, 5:16 PM

This is a useful change. +1

This revision is now accepted and ready to land.Jan 17 2017, 6:31 PM

We have fdt_is_enabled() function, why you want new, duplicated, one?
Also note that 'ok' is also valid value for 'status' property (but yes, fdt_is_enabled() have same issue).

This revision now requires changes to proceed.Jan 18 2017, 7:17 AM

fdt_is_enabled is deprecated and should be removed as soon as there is an appropriate OpenFirmware function.

ka_semihalf.com added a subscriber: ka_semihalf.com.Jan 18 2017, 9:43 AM

It was also decided to implement new OFW function, because fdt_is_enabled would be used by Marvell platforms at an early stage and it simply fails there with:

panic: pmap_fault: pm_pt1 abort
cpuid = 0
Uptime: 1s

because malloc is not yet available.

So, once a check for 'status = "ok"' is added, are there any objections to patch?

No, of course, not. And sorry for late response.
But remember, /sys/dev/ofw is under Nathan's jurisdiction. So you still needs his positive review.

This revision is now accepted and ready to land.Jan 18 2017, 9:32 PM

Love the concept, have issues with the implementation. :)

sys/dev/ofw/ofw_bus_subr.c
189–191 ↗	(On Diff #24122)	So if the property doesn't exist, we assume it's OK?
194 ↗	(On Diff #24122)	Wouldn't it be better to do something like: if (len == 4 and bcmp(status, "okay", len) == 0) since this will match 'o', 'ok' and 'oka' which isn't quite right. And properties aren't 'C' strings so bcmp would be better, no?

mw_semihalf.com added inline comments.Jan 19 2017, 7:37 AM

sys/dev/ofw/ofw_bus_subr.c
189–191 ↗	(On Diff #24122)	Yes. Node is enabled in 3 cases: lack of status property status = "ok" status = "okay"
194 ↗	(On Diff #24122)	Im ok with both fixing len and bcmp instead of strncmp.