Page MenuHomeFreeBSD

device_get_property: add a HANDLE case
ClosedPublic

Authored by bz on Sep 29 2022, 12:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
May 8 2024, 5:48 AM
Unknown Object (File)
May 8 2024, 5:48 AM
Unknown Object (File)
May 8 2024, 5:48 AM
Unknown Object (File)
May 8 2024, 2:17 AM
Unknown Object (File)
Dec 28 2023, 1:07 PM
Unknown Object (File)
Dec 23 2023, 10:38 AM
Unknown Object (File)
Dec 10 2023, 9:18 PM
Unknown Object (File)
Nov 12 2023, 1:45 AM
Subscribers

Details

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).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bz requested review of this revision.Sep 29 2022, 12:45 PM
mw requested changes to this revision.Sep 29 2022, 3:14 PM

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)?

This revision now requires changes to proceed.Sep 29 2022, 3:14 PM

@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?

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.

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.

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

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.

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

This revision is now accepted and ready to land.Oct 4 2022, 10:03 PM
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).

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.
  1. 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?

This revision was automatically updated to reflect the committed changes.