Changeset View
Standalone View
sys/arm/arm/pmu_acpi.c
- This file was added.
/*- | |||||||||
* SPDX-License-Identifier: BSD-2-Clause-FreeBSD | |||||||||
* | |||||||||
* Copyright (c) 2020 Greg V <greg@unrelenting.technology> | |||||||||
* Copyright (c) 2021 Ruslan Bukin <br@bsdpad.com> | |||||||||
* | |||||||||
jrtc27: Should probably be the other way round? | |||||||||
* Redistribution and use in source and binary forms, with or without | |||||||||
* modification, are permitted provided that the following conditions | |||||||||
* are met: | |||||||||
* 1. Redistributions of source code must retain the above copyright | |||||||||
* notice, this list of conditions and the following disclaimer. | |||||||||
* 2. Redistributions in binary form must reproduce the above copyright | |||||||||
* notice, this list of conditions and the following disclaimer in the | |||||||||
* documentation and/or other materials provided with the distribution. | |||||||||
* | |||||||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND | |||||||||
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |||||||||
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | |||||||||
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE | |||||||||
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |||||||||
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | |||||||||
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | |||||||||
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | |||||||||
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | |||||||||
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | |||||||||
* SUCH DAMAGE. | |||||||||
*/ | |||||||||
#include <sys/cdefs.h> | |||||||||
#include <sys/param.h> | |||||||||
#include <sys/bus.h> | |||||||||
#include <sys/kernel.h> | |||||||||
#include <sys/module.h> | |||||||||
#include <sys/rman.h> | |||||||||
#include <contrib/dev/acpica/include/acpi.h> | |||||||||
#include <dev/acpica/acpivar.h> | |||||||||
#include "acpi_bus_if.h" | |||||||||
#include "pmu.h" | |||||||||
struct madt_ctx { | |||||||||
struct pmu_softc *sc; | |||||||||
int error; | |||||||||
int i; | |||||||||
}; | |||||||||
static void | |||||||||
madt_handler(ACPI_SUBTABLE_HEADER *entry, void *arg) | |||||||||
{ | |||||||||
ACPI_MADT_GENERIC_INTERRUPT *intr; | |||||||||
struct intr_map_data_acpi *ad; | |||||||||
struct intr_map_data *data; | |||||||||
struct madt_ctx *ctx; | |||||||||
struct pmu_softc *sc; | |||||||||
struct pcpu *pcpu; | |||||||||
int rid; | |||||||||
Not Done Inline ActionsThis should be /* * NOTE: ... andrew: This should be
```
/*
* NOTE: ...
``` | |||||||||
int cpuid; | |||||||||
int i; | |||||||||
ctx = arg; | |||||||||
sc = ctx->sc; | |||||||||
rid = ctx->i; | |||||||||
cpuid = -1; | |||||||||
if (ctx->error) | |||||||||
return; | |||||||||
if (entry->Type != ACPI_MADT_TYPE_GENERIC_INTERRUPT) | |||||||||
return; | |||||||||
intr = (ACPI_MADT_GENERIC_INTERRUPT *)entry; | |||||||||
for (i = 0; i < MAXCPU; i++) { | |||||||||
Not Done Inline ActionsThis is nothing but an unacceptable gross hack. In addition, it is a clear sign that there is something very broken in the ACPI crap. The interrupt resource for PMU should be parsed and instantiated in exactly same way that all other interrupts. If something is missing in MADT handling in ACPI driver, it should be fixed within it, not in some other driver. mmel: This is nothing but an unacceptable gross hack. In addition, it is a clear sign that there is… | |||||||||
Done Inline ActionsIn ACPI, the PMU is *not* described as a normal device with resources (that would be in DSDT). Here's an example of what MADT is: val_packett.cool: In ACPI, the PMU is *not* described as a normal device with resources (that would be in DSDT). | |||||||||
Not Done Inline ActionsI understand. Please see how are the irregularities are handled in i386/amd64 world and where the code lives -> sys/x86/acpica/madt.c. At my best you should move PMU enumeration into appropriate arm64 directory (/sys/arm64/acpica/) and split it from real device driver. mmel: I understand. Please see how are the irregularities are handled in i386/amd64 world and… | |||||||||
Done Inline Actions
hm, all "irregularities" there seem to be interrupt controller quirks, not creating an unrelated device from data that exists in MADT.
Why? gic_acpi.c lives in sys/arm/arm, and it's an arm64 thing. sys/arm/arm/generic_timer.c also has ACPI inside. So from these two examples it seems like the convention is for devices that share anything between 32 and 64 bit, everything lives in arm/arm. val_packett.cool: > sys/x86/acpica/madt.c
hm, all "irregularities" there seem to be interrupt controller quirks… | |||||||||
Not Done Inline ActionsWell, in attempt to unlock this "clash", my biggest problem is usage of ACPI_BUS_MAP_INTR() in this context. This method is part of resources allocation protocol and should be called only within it contract. (As you can see nobody use this method in similar context). Also accessing parent device ivars from child is clear layering violation. mmel: Well, in attempt to unlock this "clash", my biggest problem is usage of ACPI_BUS_MAP_INTR() in… | |||||||||
pcpu = pcpu_find(i); | |||||||||
if (pcpu != NULL && pcpu->pc_mpidr == intr->ArmMpidr) { | |||||||||
Not Done Inline ActionsWhat if we can't find the CPU? Seems to me that should either be an error that fails the attachment or a warning and early return in order to at least attach for the other CPUs (I prefer the former though otherwise you could end up reporting success but be unable to attach for any CPU) jrtc27: What if we can't find the CPU? Seems to me that should either be an error that fails the… | |||||||||
cpuid = i; | |||||||||
break; | |||||||||
} | |||||||||
} | |||||||||
Not Done Inline ActionsIt would be useful to create a general helper function somewhere for this mpidr -> cpuid conversion, but this is a TODO item at best. mhorne: It would be useful to create a general helper function somewhere for this mpidr -> cpuid… | |||||||||
if (cpuid == -1) { | |||||||||
/* pcpu not found. */ | |||||||||
device_printf(sc->dev, "MADT: could not find pcpu, " | |||||||||
"ArmMpidr %lx\n", intr->ArmMpidr); | |||||||||
Not Done Inline Actions
jrtc27: | |||||||||
ctx->error = ENODEV; | |||||||||
return; | |||||||||
} | |||||||||
Not Done Inline ActionsNit: most drivers prefer the bus_set_resource() wrapper. mhorne: Nit: most drivers prefer the `bus_set_resource()` wrapper. | |||||||||
if (bootverbose) | |||||||||
device_printf(sc->dev, "MADT: cpu %d (mpidr %lu) irq %d " | |||||||||
"%s-triggered\n", cpuid, intr->ArmMpidr, | |||||||||
intr->PerformanceInterrupt, | |||||||||
(intr->Flags & ACPI_MADT_PERFORMANCE_IRQ_MODE) ? | |||||||||
Not Done Inline ActionsThis needs to be a proper error, currently it (a) continues on to rman_get_virtual of the NULL pointer (b) doesn't return an error from the attach function. jrtc27: This needs to be a proper error, currently it (a) continues on to rman_get_virtual of the NULL… | |||||||||
"edge" : "level"); | |||||||||
bus_set_resource(sc->dev, SYS_RES_IRQ, ctx->i, | |||||||||
intr->PerformanceInterrupt, 1); | |||||||||
sc->irq[ctx->i].res = bus_alloc_resource_any(sc->dev, SYS_RES_IRQ, | |||||||||
&rid, RF_ACTIVE | RF_SHAREABLE); | |||||||||
Not Done Inline ActionsAre there cases where this is not true, and if so is silently continuing the right thing to do? I'm assuming this should always be true and thus should be a KASSERT, or maybe even conditional panic. That or fail the attachment with an error like any other "could not map resource" kind of error. jrtc27: Are there cases where this is not true, and if so is silently continuing the right thing to do? | |||||||||
if (sc->irq[ctx->i].res == NULL) { | |||||||||
device_printf(sc->dev, "Failed to allocate IRQ %d\n", ctx->i); | |||||||||
ctx->error = ENXIO; | |||||||||
return; | |||||||||
} | |||||||||
Not Done Inline ActionsThis is tricky, and at first I thought it did not work. But it seems like an okay way to deal with this special case :) mhorne: This is tricky, and at first I thought it did not work. But it seems like an okay way to deal… | |||||||||
/* | |||||||||
* BUS_CONFIG_INTR does nothing on arm64, so we manually set trigger | |||||||||
* mode. | |||||||||
*/ | |||||||||
Not Done Inline ActionsThis isn't quite right for GICv3.1, which adds more "extended" PPIs (1056-1119) and SPIs (4096-5119), so it'd be nice to centralise this logic in the GIC driver itself rather than also baked in here such that adding GICv3.1 support is localised to the GIC driver; maybe a macro or inline function in gic_common.h where this macro comes from? jrtc27: This isn't quite right for GICv3.1, which adds more "extended" PPIs (1056-1119) and SPIs (4096… | |||||||||
data = rman_get_virtual(sc->irq[ctx->i].res); | |||||||||
KASSERT(data->type == INTR_MAP_DATA_ACPI, ("Wrong data type")); | |||||||||
ad = (struct intr_map_data_acpi *)data; | |||||||||
ad->trig = (intr->Flags & ACPI_MADT_PERFORMANCE_IRQ_MODE) ? | |||||||||
INTR_TRIGGER_EDGE : INTR_TRIGGER_LEVEL; | |||||||||
ad->pol = INTR_POLARITY_HIGH; | |||||||||
if (!intr_is_per_cpu(sc->irq[ctx->i].res)) | |||||||||
sc->irq[ctx->i].cpuid = cpuid; | |||||||||
ctx->i++; | |||||||||
} | |||||||||
Not Done Inline ActionsCould we use intr_is_per_cpu here? This driver shouldn't need to know about GIC internal details. andrew: Could we use `intr_is_per_cpu` here? This driver shouldn't need to know about GIC internal… | |||||||||
static void | |||||||||
pmu_acpi_identify(driver_t *driver, device_t parent) | |||||||||
{ | |||||||||
device_t dev; | |||||||||
if (acpi_find_table(ACPI_SIG_MADT) == 0) | |||||||||
return; | |||||||||
dev = BUS_ADD_CHILD(parent, BUS_PASS_INTERRUPT + BUS_PASS_ORDER_LAST, | |||||||||
"pmu", -1); | |||||||||
if (dev == NULL) | |||||||||
device_printf(parent, "pmu: Unable to add pmu child\n"); | |||||||||
} | |||||||||
static int | |||||||||
pmu_acpi_probe(device_t dev) | |||||||||
{ | |||||||||
device_set_desc(dev, "Performance Monitoring Unit"); | |||||||||
return (BUS_PROBE_NOWILDCARD); | |||||||||
} | |||||||||
static int | |||||||||
pmu_acpi_attach(device_t dev) | |||||||||
{ | |||||||||
struct pmu_softc *sc; | |||||||||
struct madt_ctx ctx; | |||||||||
ACPI_TABLE_MADT *madt; | |||||||||
int i; | |||||||||
sc = device_get_softc(dev); | |||||||||
sc->dev = dev; | |||||||||
madt = acpi_map_table(acpi_find_table(ACPI_SIG_MADT), ACPI_SIG_MADT); | |||||||||
if (madt == NULL) { | |||||||||
device_printf(dev, "Unable to map the MADT table\n"); | |||||||||
return (ENXIO); | |||||||||
} | |||||||||
/* We have to initialize cpuid to -1. */ | |||||||||
for (i = 0; i < MAX_RLEN; i++) | |||||||||
sc->irq[i].cpuid = -1; | |||||||||
ctx.sc = sc; | |||||||||
ctx.i = 0; | |||||||||
ctx.error = 0; | |||||||||
acpi_walk_subtables(madt + 1, (char *)madt + madt->Header.Length, | |||||||||
madt_handler, &ctx); | |||||||||
acpi_unmap_table(madt); | |||||||||
if (ctx.error) | |||||||||
Not Done Inline ActionsIt's extremely underused, but this can be written more succinctly as: DEFINE_CLASS_0(pmu, pmu_acpi_driver, pmu_acpi_methods, sizeof(struct pmu_softc)); jrtc27: It's extremely underused, but this can be written more succinctly as:
```DEFINE_CLASS_0(pmu… | |||||||||
return (ctx.error); | |||||||||
return (pmu_attach(dev)); | |||||||||
} | |||||||||
Not Done Inline ActionsWhy is this in the ACPI function? It looks like it would be safe in the parent as the flag will never be set in the FDT code. andrew: Why is this in the ACPI function? It looks like it would be safe in the parent as the flag will… | |||||||||
static device_method_t pmu_acpi_methods[] = { | |||||||||
DEVMETHOD(device_identify, pmu_acpi_identify), | |||||||||
DEVMETHOD(device_probe, pmu_acpi_probe), | |||||||||
DEVMETHOD(device_attach, pmu_acpi_attach), | |||||||||
DEVMETHOD_END, | |||||||||
}; | |||||||||
DEFINE_CLASS_0(pmu, pmu_acpi_driver, pmu_acpi_methods, | |||||||||
sizeof(struct pmu_softc)); | |||||||||
static devclass_t pmu_acpi_devclass; | |||||||||
DRIVER_MODULE(pmu, acpi, pmu_acpi_driver, pmu_acpi_devclass, 0, 0); |
Should probably be the other way round?