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

Lint

Lint Skipped

Unit

Tests Skipped

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	So if the property doesn't exist, we assume it's OK?
194	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	Yes. Node is enabled in 3 cases: lack of status property status = "ok" status = "okay"
194	Im ok with both fixing len and bcmp instead of strncmp.