Date: Mon, 18 Jul 2016 17:11:43 -0700 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Svatopluk Kraus <skra@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r301453 - in head/sys: arm/arm arm64/arm64 dev/fdt dev/gpio dev/iicbus dev/ofw dev/pci dev/vnic kern mips/mips sys Message-ID: <b9606755-69cb-2cb0-04d7-6be32e4cb89e@freebsd.org> In-Reply-To: <201606051620.u55GKD5S066398@repo.freebsd.org> References: <201606051620.u55GKD5S066398@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Could you please describe what this change is in more detail? It breaks a lot of encapsulations we have worked very hard to maintain, moves ARM code into MI parts of the kernel, and the OFW parts violate IEEE 1275 (the Open Firmware standard). In particular, there is no guarantee that the interrupts for a newbus (or OF) device are encoded in a property called "interrupts" (or, indeed, in any property at all) on that node and there are many, many device trees where that is not the case (e.g. ones with interrupt maps, as well as Apple hardware). By putting that knowledge into the OF root bus device, which we have tried to keep it out of, this enforces a standard that doesn't actually exist. I'm hesitant to ask for reversion on something that landed 6 weeks ago without me noticing, but this needs a lot more architectural work before any parts of the kernel should use it. -Nathan On 06/05/16 09:20, Svatopluk Kraus wrote: > Author: skra > Date: Sun Jun 5 16:20:12 2016 > New Revision: 301453 > URL: https://svnweb.freebsd.org/changeset/base/301453 > > Log: > INTRNG - change the way how an interrupt mapping data are provided > to the framework in OFW (FDT) case. > > This is a follow-up to r301451. > > Differential Revision: https://reviews.freebsd.org/D6634 > > Modified: > head/sys/arm/arm/nexus.c > head/sys/arm64/arm64/gic_v3.c > head/sys/arm64/arm64/nexus.c > head/sys/dev/fdt/simplebus.c > head/sys/dev/gpio/ofw_gpiobus.c > head/sys/dev/iicbus/ofw_iicbus.c > head/sys/dev/ofw/ofw_bus_subr.c > head/sys/dev/ofw/ofw_bus_subr.h > head/sys/dev/ofw/ofwbus.c > head/sys/dev/pci/pci_host_generic.c > head/sys/dev/vnic/mrml_bridge.c > head/sys/dev/vnic/thunder_mdio_fdt.c > head/sys/kern/subr_intr.c > head/sys/mips/mips/nexus.c > head/sys/sys/intr.h > > Modified: head/sys/arm/arm/nexus.c > ============================================================================== > --- head/sys/arm/arm/nexus.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/arm/arm/nexus.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -412,6 +412,10 @@ nexus_ofw_map_intr(device_t dev, device_ > pcell_t *intr) > { > > +#ifdef INTRNG > + return (INTR_IRQ_INVALID); > +#else > return (intr_fdt_map_irq(iparent, intr, icells)); > +#endif > } > #endif > > Modified: head/sys/arm64/arm64/gic_v3.c > ============================================================================== > --- head/sys/arm64/arm64/gic_v3.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/arm64/arm64/gic_v3.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -58,6 +58,10 @@ __FBSDID("$FreeBSD$"); > #include <machine/cpu.h> > #include <machine/intr.h> > > +#ifdef FDT > +#include <dev/ofw/ofw_bus_subr.h> > +#endif > + > #include "pic_if.h" > > #include "gic_v3_reg.h" > > Modified: head/sys/arm64/arm64/nexus.c > ============================================================================== > --- head/sys/arm64/arm64/nexus.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/arm64/arm64/nexus.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -448,7 +448,7 @@ nexus_ofw_map_intr(device_t dev, device_ > pcell_t *intr) > { > #ifdef INTRNG > - return (intr_fdt_map_irq(iparent, intr, icells)); > + return (INTR_IRQ_INVALID); > #else > int irq; > > > Modified: head/sys/dev/fdt/simplebus.c > ============================================================================== > --- head/sys/dev/fdt/simplebus.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/fdt/simplebus.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -251,7 +251,9 @@ simplebus_setup_dinfo(device_t dev, phan > > resource_list_init(&ndi->rl); > ofw_bus_reg_to_rl(dev, node, sc->acells, sc->scells, &ndi->rl); > +#ifndef INTRNG > ofw_bus_intr_to_rl(dev, node, &ndi->rl, NULL); > +#endif > > return (ndi); > } > > Modified: head/sys/dev/gpio/ofw_gpiobus.c > ============================================================================== > --- head/sys/dev/gpio/ofw_gpiobus.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/gpio/ofw_gpiobus.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -321,11 +321,13 @@ ofw_gpiobus_setup_devinfo(device_t bus, > devi->pins[i] = pins[i].pin; > } > free(pins, M_DEVBUF); > +#ifndef INTRNG > /* Parse the interrupt resources. */ > if (ofw_bus_intr_to_rl(bus, node, &dinfo->opd_dinfo.rl, NULL) != 0) { > ofw_gpiobus_destroy_devinfo(bus, dinfo); > return (NULL); > } > +#endif > device_set_ivars(child, dinfo); > > return (dinfo); > > Modified: head/sys/dev/iicbus/ofw_iicbus.c > ============================================================================== > --- head/sys/dev/iicbus/ofw_iicbus.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/iicbus/ofw_iicbus.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -187,8 +187,10 @@ ofw_iicbus_attach(device_t dev) > > childdev = device_add_child(dev, NULL, -1); > resource_list_init(&dinfo->opd_dinfo.rl); > +#ifndef INTRNG > ofw_bus_intr_to_rl(childdev, child, > &dinfo->opd_dinfo.rl, NULL); > +#endif > device_set_ivars(childdev, dinfo); > } > > > Modified: head/sys/dev/ofw/ofw_bus_subr.c > ============================================================================== > --- head/sys/dev/ofw/ofw_bus_subr.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/ofw/ofw_bus_subr.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -516,6 +516,7 @@ ofw_bus_find_iparent(phandle_t node) > return (iparent); > } > > +#ifndef INTRNG > int > ofw_bus_intr_to_rl(device_t dev, phandle_t node, > struct resource_list *rl, int *rlen) > @@ -581,6 +582,78 @@ ofw_bus_intr_to_rl(device_t dev, phandle > free(intr, M_OFWPROP); > return (err); > } > +#endif > + > +int > +ofw_bus_intr_by_rid(device_t dev, phandle_t node, int wanted_rid, > + phandle_t *producer, int *ncells, pcell_t **cells) > +{ > + phandle_t iparent; > + uint32_t icells, *intr; > + int err, i, nintr, rid; > + boolean_t extended; > + > + nintr = OF_getencprop_alloc(node, "interrupts", sizeof(*intr), > + (void **)&intr); > + if (nintr > 0) { > + iparent = ofw_bus_find_iparent(node); > + if (iparent == 0) { > + device_printf(dev, "No interrupt-parent found, " > + "assuming direct parent\n"); > + iparent = OF_parent(node); > + iparent = OF_xref_from_node(iparent); > + } > + if (OF_searchencprop(OF_node_from_xref(iparent), > + "#interrupt-cells", &icells, sizeof(icells)) == -1) { > + device_printf(dev, "Missing #interrupt-cells " > + "property, assuming <1>\n"); > + icells = 1; > + } > + if (icells < 1 || icells > nintr) { > + device_printf(dev, "Invalid #interrupt-cells property " > + "value <%d>, assuming <1>\n", icells); > + icells = 1; > + } > + extended = false; > + } else { > + nintr = OF_getencprop_alloc(node, "interrupts-extended", > + sizeof(*intr), (void **)&intr); > + if (nintr <= 0) > + return (ESRCH); > + extended = true; > + } > + err = ESRCH; > + rid = 0; > + for (i = 0; i < nintr; i += icells, rid++) { > + if (extended) { > + iparent = intr[i++]; > + if (OF_searchencprop(OF_node_from_xref(iparent), > + "#interrupt-cells", &icells, sizeof(icells)) == -1) { > + device_printf(dev, "Missing #interrupt-cells " > + "property\n"); > + err = ENOENT; > + break; > + } > + if (icells < 1 || (i + icells) > nintr) { > + device_printf(dev, "Invalid #interrupt-cells " > + "property value <%d>\n", icells); > + err = ERANGE; > + break; > + } > + } > + if (rid == wanted_rid) { > + *cells = malloc(icells * sizeof(**cells), M_OFWPROP, > + M_WAITOK); > + *producer = iparent; > + *ncells= icells; > + memcpy(*cells, intr + i, icells * sizeof(**cells)); > + err = 0; > + break; > + } > + } > + free(intr, M_OFWPROP); > + return (err); > +} > > phandle_t > ofw_bus_find_child(phandle_t start, const char *child_name) > > Modified: head/sys/dev/ofw/ofw_bus_subr.h > ============================================================================== > --- head/sys/dev/ofw/ofw_bus_subr.h Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/ofw/ofw_bus_subr.h Sun Jun 5 16:20:12 2016 (r301453) > @@ -52,6 +52,13 @@ struct ofw_compat_data { > uintptr_t ocd_data; > }; > > +struct intr_map_data_fdt { > + struct intr_map_data hdr; > + phandle_t iparent; > + u_int ncells; > + pcell_t *cells; > +}; > + > #define SIMPLEBUS_PNP_DESCR "Z:compat;P:private;" > #define SIMPLEBUS_PNP_INFO(t) \ > MODULE_PNP_INFO(SIMPLEBUS_PNP_DESCR, simplebus, t, t, sizeof(t[0]), sizeof(t) / sizeof(t[0])); > @@ -82,7 +89,11 @@ int ofw_bus_msimap(phandle_t, uint16_t, > /* Routines for parsing device-tree data into resource lists. */ > int ofw_bus_reg_to_rl(device_t, phandle_t, pcell_t, pcell_t, > struct resource_list *); > +#ifndef INTRNG > int ofw_bus_intr_to_rl(device_t, phandle_t, struct resource_list *, int *); > +#endif > +int ofw_bus_intr_by_rid(device_t, phandle_t, int, phandle_t *, int *, > + pcell_t **); > > /* Helper to get device status property */ > const char *ofw_bus_get_status(device_t dev); > > Modified: head/sys/dev/ofw/ofwbus.c > ============================================================================== > --- head/sys/dev/ofw/ofwbus.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/ofw/ofwbus.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -43,6 +43,9 @@ __FBSDID("$FreeBSD$"); > #include <sys/module.h> > #include <sys/pcpu.h> > #include <sys/rman.h> > +#ifdef INTRNG > +#include <sys/intr.h> > +#endif > > #include <vm/vm.h> > #include <vm/pmap.h> > @@ -77,6 +80,9 @@ static device_attach_t ofwbus_attach; > static bus_alloc_resource_t ofwbus_alloc_resource; > static bus_adjust_resource_t ofwbus_adjust_resource; > static bus_release_resource_t ofwbus_release_resource; > +#ifdef INTRNG > +static bus_map_intr_t ofwbus_map_intr; > +#endif > > static device_method_t ofwbus_methods[] = { > /* Device interface */ > @@ -90,6 +96,9 @@ static device_method_t ofwbus_methods[] > DEVMETHOD(bus_alloc_resource, ofwbus_alloc_resource), > DEVMETHOD(bus_adjust_resource, ofwbus_adjust_resource), > DEVMETHOD(bus_release_resource, ofwbus_release_resource), > +#ifdef INTRNG > + DEVMETHOD(bus_map_intr, ofwbus_map_intr), > +#endif > > DEVMETHOD_END > }; > @@ -290,3 +299,53 @@ ofwbus_release_resource(device_t bus, de > } > return (rman_release_resource(r)); > } > + > +#ifdef INTRNG > +static void > +ofwbus_destruct_map_data(struct intr_map_data *map_data) > +{ > + struct intr_map_data_fdt *fdt_map_data; > + > + KASSERT(map_data->type == INTR_MAP_DATA_FDT, > + ("%s: bad map_data type %d", __func__, map_data->type)); > + > + fdt_map_data = (struct intr_map_data_fdt *)map_data; > + OF_prop_free(fdt_map_data->cells); > + free(fdt_map_data, M_OFWPROP); > +} > + > +static int > +ofwbus_map_intr(device_t bus, device_t child, int *rid, rman_res_t *start, > + rman_res_t *end, rman_res_t *count, struct intr_map_data **imd) > +{ > + phandle_t iparent, node; > + pcell_t *cells; > + int ncells, rv; > + u_int irq; > + struct intr_map_data_fdt *fdt_data; > + > + node = ofw_bus_get_node(child); > + rv = ofw_bus_intr_by_rid(child, node, *rid, &iparent, &ncells, &cells); > + if (rv != 0) > + return (rv); > + > + fdt_data = malloc(sizeof(*fdt_data), M_OFWPROP, M_WAITOK | M_ZERO); > + fdt_data->hdr.type = INTR_MAP_DATA_FDT; > + fdt_data->hdr.destruct = ofwbus_destruct_map_data; > + fdt_data->iparent = iparent; > + fdt_data->ncells = ncells; > + fdt_data->cells = cells; > + rv = intr_map_irq(NULL, iparent, (struct intr_map_data *)fdt_data, > + &irq); > + if (rv != 0) { > + ofwbus_destruct_map_data((struct intr_map_data *)fdt_data); > + return (rv); > + } > + > + *start = irq; > + *end = irq; > + *count = 1; > + *imd = (struct intr_map_data *)fdt_data; > + return (0); > +} > +#endif > > Modified: head/sys/dev/pci/pci_host_generic.c > ============================================================================== > --- head/sys/dev/pci/pci_host_generic.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/pci/pci_host_generic.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -949,7 +949,9 @@ generic_pcie_ofw_bus_attach(device_t dev > resource_list_init(&di->di_rl); > ofw_bus_reg_to_rl(dev, node, addr_cells, size_cells, > &di->di_rl); > +#ifndef INTRNG > ofw_bus_intr_to_rl(dev, node, &di->di_rl, NULL); > +#endif > > /* Add newbus device for this FDT node */ > child = device_add_child(dev, NULL, -1); > > Modified: head/sys/dev/vnic/mrml_bridge.c > ============================================================================== > --- head/sys/dev/vnic/mrml_bridge.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/vnic/mrml_bridge.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -263,7 +263,9 @@ mrmlb_ofw_bus_attach(device_t dev) > resource_list_init(&di->di_rl); > ofw_bus_reg_to_rl(dev, node, sc->acells, sc->scells, > &di->di_rl); > +#ifndef INTRNG > ofw_bus_intr_to_rl(dev, node, &di->di_rl, NULL); > +#endif > > /* Add newbus device for this FDT node */ > child = device_add_child(dev, NULL, -1); > > Modified: head/sys/dev/vnic/thunder_mdio_fdt.c > ============================================================================== > --- head/sys/dev/vnic/thunder_mdio_fdt.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/dev/vnic/thunder_mdio_fdt.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -271,7 +271,9 @@ mdionexus_ofw_bus_attach(device_t dev) > resource_list_init(&di->di_rl); > ofw_bus_reg_to_rl(dev, node, sc->acells, sc->scells, > &di->di_rl); > +#ifndef INTRNG > ofw_bus_intr_to_rl(dev, node, &di->di_rl, NULL); > +#endif > > /* Add newbus device for this FDT node */ > child = device_add_child(dev, NULL, -1); > > Modified: head/sys/kern/subr_intr.c > ============================================================================== > --- head/sys/kern/subr_intr.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/kern/subr_intr.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -64,12 +64,6 @@ __FBSDID("$FreeBSD$"); > #include <machine/smp.h> > #include <machine/stdarg.h> > > -#ifdef FDT > -#include <dev/ofw/openfirm.h> > -#include <dev/ofw/ofw_bus.h> > -#include <dev/ofw/ofw_bus_subr.h> > -#endif > - > #ifdef DDB > #include <ddb/ddb.h> > #endif > @@ -625,33 +619,6 @@ intr_acpi_map_irq(device_t dev, u_int ir > return (ddata->idd_irq); > } > #endif > -#ifdef FDT > -/* > - * Map interrupt source according to FDT data into framework. If such mapping > - * does not exist, create it. Return unique interrupt number (resource handle) > - * associated with mapped interrupt source. > - */ > -u_int > -intr_fdt_map_irq(phandle_t node, pcell_t *cells, u_int ncells) > -{ > - size_t cellsize; > - struct intr_dev_data *ddata; > - struct intr_map_data_fdt *daf; > - > - cellsize = ncells * sizeof(*cells); > - ddata = intr_ddata_alloc(sizeof(struct intr_map_data_fdt) + cellsize); > - if (ddata == NULL) > - return (INTR_IRQ_INVALID); /* no space left */ > - > - ddata->idd_xref = (intptr_t)node; > - ddata->idd_data->type = INTR_MAP_DATA_FDT; > - > - daf = (struct intr_map_data_fdt *)ddata->idd_data; > - daf->ncells = ncells; > - memcpy(daf->cells, cells, cellsize); > - return (ddata->idd_irq); > -} > -#endif > > /* > * Store GPIO interrupt decription in framework and return unique interrupt > @@ -1107,7 +1074,11 @@ intr_alloc_irq(device_t dev, struct reso > KASSERT(rman_get_start(res) == rman_get_end(res), > ("%s: more interrupts in resource", __func__)); > > - isrc = intr_ddata_lookup(rman_get_start(res), &data); > + data = rman_get_virtual(res); > + if (data == NULL) > + isrc = intr_ddata_lookup(rman_get_start(res), &data); > + else > + isrc = isrc_lookup(rman_get_start(res)); > if (isrc == NULL) > return (EINVAL); > > @@ -1123,7 +1094,11 @@ intr_release_irq(device_t dev, struct re > KASSERT(rman_get_start(res) == rman_get_end(res), > ("%s: more interrupts in resource", __func__)); > > - isrc = intr_ddata_lookup(rman_get_start(res), &data); > + data = rman_get_virtual(res); > + if (data == NULL) > + isrc = intr_ddata_lookup(rman_get_start(res), &data); > + else > + isrc = isrc_lookup(rman_get_start(res)); > if (isrc == NULL) > return (EINVAL); > > @@ -1142,7 +1117,11 @@ intr_setup_irq(device_t dev, struct reso > KASSERT(rman_get_start(res) == rman_get_end(res), > ("%s: more interrupts in resource", __func__)); > > - isrc = intr_ddata_lookup(rman_get_start(res), &data); > + data = rman_get_virtual(res); > + if (data == NULL) > + isrc = intr_ddata_lookup(rman_get_start(res), &data); > + else > + isrc = isrc_lookup(rman_get_start(res)); > if (isrc == NULL) > return (EINVAL); > > @@ -1202,7 +1181,11 @@ intr_teardown_irq(device_t dev, struct r > KASSERT(rman_get_start(res) == rman_get_end(res), > ("%s: more interrupts in resource", __func__)); > > - isrc = intr_ddata_lookup(rman_get_start(res), &data); > + data = rman_get_virtual(res); > + if (data == NULL) > + isrc = intr_ddata_lookup(rman_get_start(res), &data); > + else > + isrc = isrc_lookup(rman_get_start(res)); > if (isrc == NULL || isrc->isrc_handlers == 0) > return (EINVAL); > > > Modified: head/sys/mips/mips/nexus.c > ============================================================================== > --- head/sys/mips/mips/nexus.c Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/mips/mips/nexus.c Sun Jun 5 16:20:12 2016 (r301453) > @@ -524,7 +524,11 @@ nexus_ofw_map_intr(device_t dev, device_ > pcell_t *intr) > { > > +#ifdef INTRNG > + return (INTR_IRQ_INVALID); > +#else > return (intr_fdt_map_irq(iparent, intr, icells)); > +#endif > } > #endif > #endif /* INTRNG */ > > Modified: head/sys/sys/intr.h > ============================================================================== > --- head/sys/sys/intr.h Sun Jun 5 16:09:31 2016 (r301452) > +++ head/sys/sys/intr.h Sun Jun 5 16:20:12 2016 (r301453) > @@ -42,13 +42,6 @@ struct intr_map_data_acpi { > enum intr_trigger trig; > }; > #endif > -#ifdef FDT > -struct intr_map_data_fdt { > - struct intr_map_data hdr; > - u_int ncells; > - pcell_t cells[0]; > -}; > -#endif > > struct intr_map_data_gpio { > struct intr_map_data hdr; > @@ -135,9 +128,7 @@ int intr_release_msix(device_t, device_t > u_int intr_acpi_map_irq(device_t, u_int, enum intr_polarity, > enum intr_trigger); > #endif > -#ifdef FDT > -u_int intr_fdt_map_irq(phandle_t, pcell_t *, u_int); > -#endif > + > u_int intr_gpio_map_irq(device_t dev, u_int pin_num, u_int pin_flags, > u_int intr_mode); > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?b9606755-69cb-2cb0-04d7-6be32e4cb89e>