device_get_property: add a HANDLE case

[bz]

Summary

This will resolve a reference and return the appropriate handle, a node
on the simplebus or an ACPI_HANDLE for ACPI. It is harder to further
abstract this for me (and frankly the xref in FDT seems a lot more handy
than a node, but that propbably depends on the use case).

[mw]

Overall, LGTM - thank you for this patch! One nit/suggestion inline. Do you have an example code locally that shows real-life usage - I would like to see a code snippet/diff.

sys/dev/fdt/simplebus.c

411

How about return (-EINVAL)?

[dsl]

@mw, this diff is supposed to substitute some pieces from sys/dev/dpaa2/dpaa2_mc_fdt.c and sys/dev/dpaa2/dpaa2_mc_acpi.c (https://reviews.freebsd.org/D36638):

static int
dpaa2_mac_dev_attach(device_t dev)
{
...
hobj = NULL;
	s = device_get_property(dev, "phy-handle", &hobj, sizeof(hobj),
	    DEVICE_PROP_ANY);
	if (s > 0) {
		if (hobj->Type != ACPI_TYPE_PACKAGE) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package obj, not ACPI_TYPE_PACKAGE\n");
			goto out;
		}
		if (hobj->Package.Count != 1) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package obj, count %d\n", hobj->Package.Count);
			goto out;
		}
		pobj = &hobj->Package.Elements[0];
		if (pobj == NULL) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package pobj is NULL\n");
			goto out;
		}
		if (pobj->Type != ACPI_TYPE_LOCAL_REFERENCE) {
			device_printf(dev, "Cannot get 'phy-handle' package "
			    "pobj, no ACPI_TYPE_LOCAL_REFERENCE\n");
			goto out;
		}
		sc->phy_channel = acpi_GetReference(NULL, pobj);
		if (sc->phy_channel == NULL) {
			device_printf(dev, "Cannot get 'phy-handle' reference "
			    "handle ph is NULL\n");
			goto out;
		}
	}
...
}

You've requested such change in https://reviews.freebsd.org/D36677#833135, haven't you?

[mw]

In D36793#837060, @dsl wrote:

@mw, this diff is supposed to substitute some pieces from sys/dev/dpaa2/dpaa2_mc_fdt.c and sys/dev/dpaa2/dpaa2_mc_acpi.c (https://reviews.freebsd.org/D36638):

static int
dpaa2_mac_dev_attach(device_t dev)
{
...
hobj = NULL;
	s = device_get_property(dev, "phy-handle", &hobj, sizeof(hobj),
	    DEVICE_PROP_ANY);
	if (s > 0) {
		if (hobj->Type != ACPI_TYPE_PACKAGE) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package obj, not ACPI_TYPE_PACKAGE\n");
			goto out;
		}
		if (hobj->Package.Count != 1) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package obj, count %d\n", hobj->Package.Count);
			goto out;
		}
		pobj = &hobj->Package.Elements[0];
		if (pobj == NULL) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package pobj is NULL\n");
			goto out;
		}
		if (pobj->Type != ACPI_TYPE_LOCAL_REFERENCE) {
			device_printf(dev, "Cannot get 'phy-handle' package "
			    "pobj, no ACPI_TYPE_LOCAL_REFERENCE\n");
			goto out;
		}
		sc->phy_channel = acpi_GetReference(NULL, pobj);
		if (sc->phy_channel == NULL) {
			device_printf(dev, "Cannot get 'phy-handle' reference "
			    "handle ph is NULL\n");
			goto out;
		}
	}
...
}

You've requested such change in https://reviews.freebsd.org/D36677#833135, haven't you?

I have, indeed. What I asked for, basically, is a rebased version of https://reviews.freebsd.org/D36638 on top of this change. We'd be sure it's working and has really an outcome of unifying the API.

[bz]

In D36793#837060, @dsl wrote:

@mw, this diff is supposed to substitute some pieces from sys/dev/dpaa2/dpaa2_mc_fdt.c and sys/dev/dpaa2/dpaa2_mc_acpi.c (https://reviews.freebsd.org/D36638):

static int
dpaa2_mac_dev_attach(device_t dev)
{
...
hobj = NULL;
	s = device_get_property(dev, "phy-handle", &hobj, sizeof(hobj),
	    DEVICE_PROP_ANY);
	if (s > 0) {
		if (hobj->Type != ACPI_TYPE_PACKAGE) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package obj, not ACPI_TYPE_PACKAGE\n");
			goto out;
		}
		if (hobj->Package.Count != 1) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package obj, count %d\n", hobj->Package.Count);
			goto out;
		}
		pobj = &hobj->Package.Elements[0];
		if (pobj == NULL) {
			device_printf(dev, "Cannot get 'phy-handle' "
			    "package pobj is NULL\n");
			goto out;
		}
		if (pobj->Type != ACPI_TYPE_LOCAL_REFERENCE) {
			device_printf(dev, "Cannot get 'phy-handle' package "
			    "pobj, no ACPI_TYPE_LOCAL_REFERENCE\n");
			goto out;
		}
		sc->phy_channel = acpi_GetReference(NULL, pobj);
		if (sc->phy_channel == NULL) {
			device_printf(dev, "Cannot get 'phy-handle' reference "
			    "handle ph is NULL\n");
			goto out;
		}
	}
...
}

You've requested such change in https://reviews.freebsd.org/D36677#833135, haven't you?

Sorry, I'll try to put that diff up later as well as I have dpaa2 compiling with this.

[bz]

Here's the minimal changes to dpaa2; there's still a disconnect between ACPI and FDT in that they do express different things (also partially with different names) so I haven't tried to completely harmonify them yet. There is also still phandle_t node vs. ACPIHANDLE but that seems the least of the problems. https://people.freebsd.org/~bz/tmp/20221004-01-dpaa2-prop-handle.diff

[bz]

sys/dev/fdt/simplebus.c

411

Just following the OF_ conventions and the above example for the switch on the PROP types here; I am not sure adding the Linux -Exxx is a good idea.

[mw]

That looks really good, thanks a lot!

One idea:
OF_device_from_xref(OF_xref_from_node(sc->sfp))
I think it would be useful to have a wrapper: OF_device_from_node

[bz]

In D36793#837249, @mw wrote:

That looks really good, thanks a lot!

One idea:
OF_device_from_xref(OF_xref_from_node(sc->sfp))
I think it would be useful to have a wrapper: OF_device_from_node

I was thinking long about that and I had actually done an OF_dev_from_xref_node() to not lose the xref but I decided it's a few one-time calls ... If we do that we should do it seperately and fix the entire tree (as this is a more-than-once pattern).

[mw]

In D36793#837252, @bz wrote:

In D36793#837249, @mw wrote:

That looks really good, thanks a lot!

One idea:
OF_device_from_xref(OF_xref_from_node(sc->sfp))
I think it would be useful to have a wrapper: OF_device_from_node

I was thinking long about that and I had actually done an OF_dev_from_xref_node() to not lose the xref but I decided it's a few one-time calls ... If we do that we should do it seperately and fix the entire tree (as this is a more-than-once pattern).

1. Of course, I mean a separate change. This one is IMO good to go and land in the tree.

2. Well, I'd go with a shorter name for the sake of simplicity - we transform phandle_t to dev, I'm not convinced we should mention xref (if someone wants to use the latter, the API is already there). We can discuss under patch. I thought of doing it in 2 or more commits: a. add API, b. update existing code.

What do you think?