Page MenuHomeFreeBSD
Differential D35643

ithreads: Support priority adjustment by schedulers.
ClosedPublic
Actions

Authored by jhb on Jun 28 2022, 10:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 3 2025, 1:28 PM
Unknown Object (File)
Feb 1 2025, 2:36 AM
Unknown Object (File)
Jan 25 2025, 4:26 PM
Unknown Object (File)
Jan 24 2025, 11:16 AM
Unknown Object (File)
Jan 23 2025, 11:47 PM
Unknown Object (File)
Jan 19 2025, 7:21 AM
Unknown Object (File)
Dec 2 2024, 10:42 PM
Unknown Object (File)
Nov 21 2024, 6:48 AM
View All 54 Files
Subscribers
imp

Details

Reviewers
kib
markj
Commits
rGed998d1c24db: ithreads: Support priority adjustment by schedulers.
Summary

Use sched_wakeup instead of sched_add when marking an ithread
runnable. This allows schedulers to reset their internal time slice
tracking state and restore the base ithread priority when an ithread
resumes from idle.

Sponsored by: Netflix

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 46166
Build 43055: arc lint + arc unit

Event Timeline

jhb created this revision.Jun 28 2022, 10:31 PM
Herald added a subscriber: imp. · View Herald TranscriptJun 28 2022, 10:31 PM
jhb requested review of this revision.Jun 28 2022, 10:31 PM
Harbormaster completed remote builds in B46166: Diff 107505.Jun 28 2022, 10:31 PM
jhb added a parent revision: D35642: Add sched_ithread_prio to set the base priority of an interrupt thread..Jun 28 2022, 10:31 PM
jhb added a child revision: D35644: ule: Simplistic time-sharing for interrupt threads..
kib added a comment.Jul 2 2022, 9:36 AM

So this change would be not needed if you do the prio adjustments in sched_add(). I think more explanations are needed if there is a reason to handle ithreads prio in sched_wakeup(). Is it just a micro-optimization to avoid one more if() in sched_add()?

jhb added a comment.Jul 6 2022, 6:23 PM

One more if that would end up duplicating logic. However, it's also true that you don't want to do it on every sched_add(). For example, if an ithread is preempted the old thread gets put on the runqueues via sched_add() and you don't want that step to reset the scheduler's internal notion of CPU usage (how much of a time slice has been used). You only want to reset the usage back to zero when the ithread goes idle. In practice though it is simpler to mimic what happens now for other threads which is to reset it not when going idle, but when resumed from being idle. This is how it works today for other threads that only reset their time slice usage when resumed from sleeping, not every time they are put on a run queue.

markj accepted this revision.Jul 12 2022, 1:31 PM
This revision is now accepted and ready to land.Jul 12 2022, 1:31 PM
Closed by commit rGed998d1c24db: ithreads: Support priority adjustment by schedulers. (authored by jhb). · Explain WhyJul 14 2022, 8:15 PM
This revision was automatically updated to reflect the committed changes.
jhb added a commit: rGed998d1c24db: ithreads: Support priority adjustment by schedulers..

Revision Contents
Changeset List

PathSize
sys/
kern/
4 lines
2 lines

Diff 107505

View Options

sys/kern/kern_intr.c

  • This file was added.
/*-
* SPDX-License-Identifier: BSD-2-Clause-FreeBSD
*
* Copyright (c) 2021 Jessica Clarke <jrtc27@FreeBSD.org>
*
* 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.
*
*/
/* TODO: Provide interrupt controller interface */
mhorneUnsubmitted
Not Done
Inline Actions

I would be interested in implementing this, but not sure how soon that would be...

mhorne: I would be interested in implementing this, but not sure how soon that would be...
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/bus.h>
#include <sys/kernel.h>
#include <sys/module.h>
#include <sys/rman.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/gpio.h>
#include <dev/gpio/gpiobusvar.h>
#include <dev/ofw/ofw_bus.h>
#include <dev/ofw/ofw_bus_subr.h>
#include <machine/bus.h>
/* Registers are 32-bit so can only fit 32 pins */
#define SFGPIO_MAX_PINS 32
#define SFGPIO_DEFAULT_CAPS (GPIO_PIN_INPUT | GPIO_PIN_OUTPUT)
#define SFGPIO_INPUT_VAL 0x0
#define SFGPIO_INPUT_EN 0x4
#define SFGPIO_OUTPUT_EN 0x8
#define SFGPIO_OUTPUT_VAL 0xc
#define SFGPIO_RISE_IE 0x18
#define SFGPIO_RISE_IP 0x1c
#define SFGPIO_FALL_IE 0x20
#define SFGPIO_FALL_IP 0x24
#define SFGPIO_HIGH_IE 0x28
#define SFGPIO_HIGH_IP 0x2c
#define SFGPIO_LOW_IE 0x30
#define SFGPIO_LOW_IP 0x34
struct sfgpio_softc {
device_t dev;
device_t busdev;
struct mtx mtx;
struct resource *mem_res;
int mem_rid;
struct resource *irq_res;
int irq_rid;
int npins;
struct gpio_pin gpio_pins[SFGPIO_MAX_PINS];
};
#define SFGPIO_LOCK(_sc) mtx_lock(&(_sc)->mtx)
#define SFGPIO_UNLOCK(_sc) mtx_unlock(&(_sc)->mtx)
#define SFGPIO_READ(_sc, _off) \
bus_read_4((_sc)->mem_res, (_off))
#define SFGPIO_WRITE(_sc, _off, _val) \
bus_write_4((_sc)->mem_res, (_off), (_val))
static struct ofw_compat_data compat_data[] = {
{ "sifive,gpio0", 1 },
{ NULL, 0 },
};
static int
sfgpio_probe(device_t dev)
{
if (!ofw_bus_status_okay(dev))
return (ENXIO);
if (ofw_bus_search_compatible(dev, compat_data)->ocd_data == 0)
return (ENXIO);
device_set_desc(dev, "SiFive GPIO Controller");
return (BUS_PROBE_DEFAULT);
}
static int
sfgpio_attach(device_t dev)
{
struct sfgpio_softc *sc;
phandle_t node;
int error, i;
pcell_t npins;
uint32_t input_en, output_en;
sc = device_get_softc(dev);
sc->dev = dev;
node = ofw_bus_get_node(dev);
mtx_init(&sc->mtx, device_get_nameunit(sc->dev), NULL, MTX_DEF);
sc->mem_rid = 0;
sc->mem_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY,
&sc->mem_rid, RF_ACTIVE);
if (sc->mem_res == NULL) {
device_printf(dev, "Cannot allocate memory resource\n");
error = ENXIO;
goto fail;
}
if (OF_getencprop(node, "ngpios", &npins, sizeof(npins)) <= 0) {
/* Optional; defaults to 16 */
npins = 16;
} else if (npins > SFGPIO_MAX_PINS) {
device_printf(dev, "Too many pins: %d\n", npins);
error = ENXIO;
goto fail;
}
sc->npins = npins;
sc->irq_rid = 0;
sc->irq_res = bus_alloc_resource_any(dev, SYS_RES_IRQ, &sc->irq_rid,
RF_ACTIVE);
if (sc->irq_res == NULL) {
device_printf(dev, "Cannot allocate IRQ resource\n");
error = ENXIO;
goto fail;
}
input_en = SFGPIO_READ(sc, SFGPIO_INPUT_EN);
output_en = SFGPIO_READ(sc, SFGPIO_OUTPUT_EN);
for (i = 0; i < sc->npins; ++i) {
sc->gpio_pins[i].gp_pin = i;
sc->gpio_pins[i].gp_caps = SFGPIO_DEFAULT_CAPS;
sc->gpio_pins[i].gp_flags =
((input_en & (1u << i)) ? GPIO_PIN_INPUT : 0) |
((output_en & (1u << i)) ? GPIO_PIN_OUTPUT : 0);
snprintf(sc->gpio_pins[i].gp_name, GPIOMAXNAME, "GPIO%d", i);
sc->gpio_pins[i].gp_name[GPIOMAXNAME - 1] = '\0';
}
manuUnsubmitted
Done
Inline Actions

Is the device_get_unit really necessary ?
That would mean that a sifive_gpio1 driver expose sifive_gpio1.X and since you would need to -f /dev/gpioc1 too that's a bit redondant.
Same for the 'sifive_gpio' prefix, for Allwinner and Rockchip we name them after the pad/bank-pin name as this is what is used in the schematics of the different boards.

manu: Is the device_get_unit really necessary ? That would mean that a sifive_gpio1 driver expose…
jrtc27AuthorUnsubmitted
Done
Inline Actions

I was using other drivers like dwgpio and qoriq_gpio as a reference. If what they're doing also doesn't make a huge amount of sense then this I can just change this.

The schematics just name them GPIOn, with a note saying what some of them are connected to.

jrtc27: I was using other drivers like dwgpio and qoriq_gpio as a reference. If what they're doing also…
sc->busdev = gpiobus_attach_bus(dev);
if (sc->busdev == NULL) {
device_printf(dev, "Cannot attach gpiobus\n");
error = ENXIO;
goto fail;
}
return (0);
fail:
if (sc->busdev != NULL)
gpiobus_detach_bus(dev);
if (sc->irq_res != NULL)
bus_release_resource(dev, SYS_RES_IRQ, sc->irq_rid,
sc->irq_res);
if (sc->mem_res != NULL)
bus_release_resource(dev, SYS_RES_MEMORY, sc->mem_rid,
sc->mem_res);
mtx_destroy(&sc->mtx);
return (error);
}
static device_t
sfgpio_get_bus(device_t dev)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
return (sc->busdev);
}
static int
sfgpio_pin_max(device_t dev, int *maxpin)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
*maxpin = sc->npins - 1;
return (0);
}
static int
sfgpio_pin_set(device_t dev, uint32_t pin, unsigned int val)
{
struct sfgpio_softc *sc;
uint32_t reg;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
mhorneUnsubmitted
Done
Inline Actions

Is it ever the case that gp_pin != i? Seems like these loops could be removed, if not.

mhorne: Is it ever the case that `gp_pin != i`? Seems like these loops could be removed, if not.
jrtc27AuthorUnsubmitted
Done
Inline Actions

This code is indeed weird. A bunch of other drivers have this exact pattern (basically most drivers using struct gpio_pin, there are some exceptions that do things more simply) and as far as I can tell will also never have the pins not match, I just copied this idiom from them... So maybe I should drop these loops and just assert that sc->gpio_pins[pin].gp_pin == pin?

jrtc27: This code is indeed weird. A bunch of other drivers have this exact pattern (basically most…
mhorneUnsubmitted
Done
Inline Actions

Yeah, seems okay to me to drop the loops here.

Looks like ar5315_gpio and ar71xx_gpio are drivers where this mapping isn't 1:1, but for most it doesn't matter.

mhorne: Yeah, seems okay to me to drop the loops here. Looks like `ar5315_gpio` and `ar71xx_gpio` are…
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_VAL);
if (val)
reg |= (1u << pin);
else
reg &= ~(1u << pin);
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_VAL, reg);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_get(device_t dev, uint32_t pin, unsigned int *val)
{
struct sfgpio_softc *sc;
uint32_t reg;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
if (sc->gpio_pins[pin].gp_flags & GPIO_PIN_OUTPUT)
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_VAL);
else
reg = SFGPIO_READ(sc, SFGPIO_INPUT_VAL);
*val = (reg & (1u << pin)) ? 1 : 0;
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_toggle(device_t dev, uint32_t pin)
{
struct sfgpio_softc *sc;
uint32_t reg;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_VAL);
reg ^= (1u << pin);
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_VAL, reg);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_getcaps(device_t dev, uint32_t pin, uint32_t *caps)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
*caps = sc->gpio_pins[pin].gp_caps;
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_getflags(device_t dev, uint32_t pin, uint32_t *flags)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
*flags = sc->gpio_pins[pin].gp_flags;
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_getname(device_t dev, uint32_t pin, char *name)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
memcpy(name, sc->gpio_pins[pin].gp_name, GPIOMAXNAME);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_setflags(device_t dev, uint32_t pin, uint32_t flags)
{
struct sfgpio_softc *sc;
uint32_t reg;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
reg = SFGPIO_READ(sc, SFGPIO_INPUT_EN);
if (flags & GPIO_PIN_INPUT) {
reg |= (1u << pin);
sc->gpio_pins[pin].gp_flags |= GPIO_PIN_INPUT;
} else {
reg &= ~(1u << pin);
sc->gpio_pins[pin].gp_flags &= ~GPIO_PIN_INPUT;
}
SFGPIO_WRITE(sc, SFGPIO_INPUT_EN, reg);
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_EN);
if (flags & GPIO_PIN_OUTPUT) {
reg |= (1u << pin);
sc->gpio_pins[pin].gp_flags |= GPIO_PIN_OUTPUT;
} else {
reg &= ~(1u << pin);
sc->gpio_pins[pin].gp_flags &= ~GPIO_PIN_OUTPUT;
}
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_EN, reg);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_access_32(device_t dev, uint32_t first_pin, uint32_t clear_pins,
uint32_t change_pins, uint32_t *orig_pins)
{
struct sfgpio_softc *sc;
uint32_t reg;
if (first_pin != 0)
return (EINVAL);
sc = device_get_softc(dev);
SFGPIO_LOCK(sc);
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_VAL);
if (orig_pins != NULL)
/* Only input_val is implicitly masked by input_en */
*orig_pins = SFGPIO_READ(sc, SFGPIO_INPUT_VAL) |
(reg & SFGPIO_READ(sc, SFGPIO_OUTPUT_EN));
skiboUnsubmitted
Done
Inline Actions

I think you need to update sc->gpio_pins[i].gp_flags here.

skibo: I think you need to update sc->gpio_pins[i].gp_flags here.
if ((clear_pins | change_pins) != 0)
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_VAL,
(reg & ~clear_pins) ^ change_pins);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_config_32(device_t dev, uint32_t first_pin, uint32_t num_pins,
uint32_t *pin_flags)
{
struct sfgpio_softc *sc;
uint32_t ireg, oreg;
mhorneUnsubmitted
Not Done
Inline Actions

No detach method?

mhorne: No detach method?
manuUnsubmitted
Not Done
Inline Actions

Detach for gpio controller is complicated as it would mean to track any driver that uses a gpio. Also if there is any driver (which on embedded is more than likely as you often have reset lines etc ...) you couldn't detach.

manu: Detach for gpio controller is complicated as it would mean to track any driver that uses a gpio.
int i;
sc = device_get_softc(dev);
if (first_pin != 0 || num_pins > sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
ireg = SFGPIO_READ(sc, SFGPIO_INPUT_EN);
oreg = SFGPIO_READ(sc, SFGPIO_OUTPUT_EN);
for (i = 0; i < num_pins; ++i) {
mhorneUnsubmitted
Not Done
Inline Actions

If you did the reads after the loop, you could reduce the time spent holding the lock. I don't think it's a big deal.

mhorne: If you did the reads after the loop, you could reduce the time spent holding the lock. I don't…
jrtc27AuthorUnsubmitted
Done
Inline Actions

That's true, though I'd then need [io]{set,clear} variables, and if this driver's lock is contended then I have serious questions... :)

jrtc27: That's true, though I'd then need [io]{set,clear} variables, and if this driver's lock is…
jrtc27AuthorUnsubmitted
Done
Inline Actions

Actually no, the sc->gpio_pins[i].gp_flags all need protecting by the lock too, so reordering the loop with the I/O reads doesn't let you delay taking the lock

jrtc27: Actually no, the sc->gpio_pins[i].gp_flags all need protecting by the lock too, so reordering…
if (pin_flags[i] & GPIO_PIN_INPUT) {
ireg |= (1u << i);
oreg &= ~(1u << i);
sc->gpio_pins[i].gp_flags |= GPIO_PIN_INPUT;
sc->gpio_pins[i].gp_flags &= ~GPIO_PIN_OUTPUT;
} else if (pin_flags[i] & GPIO_PIN_OUTPUT) {
ireg &= ~(1u << i);
oreg |= (1u << i);
sc->gpio_pins[i].gp_flags &= ~GPIO_PIN_INPUT;
sc->gpio_pins[i].gp_flags |= GPIO_PIN_OUTPUT;
}
}
SFGPIO_WRITE(sc, SFGPIO_INPUT_EN, ireg);
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_EN, oreg);
SFGPIO_UNLOCK(sc);
return (0);
}
static phandle_t
sfgpio_get_node(device_t bus, device_t dev)
{
return (ofw_bus_get_node(bus));
}
static device_method_t sfgpio_methods[] = {
/* Device interface */
DEVMETHOD(device_probe, sfgpio_probe),
DEVMETHOD(device_attach, sfgpio_attach),
/* GPIO protocol */
DEVMETHOD(gpio_get_bus, sfgpio_get_bus),
DEVMETHOD(gpio_pin_max, sfgpio_pin_max),
DEVMETHOD(gpio_pin_set, sfgpio_pin_set),
DEVMETHOD(gpio_pin_get, sfgpio_pin_get),
DEVMETHOD(gpio_pin_toggle, sfgpio_pin_toggle),
DEVMETHOD(gpio_pin_getcaps, sfgpio_pin_getcaps),
DEVMETHOD(gpio_pin_getflags, sfgpio_pin_getflags),
DEVMETHOD(gpio_pin_getname, sfgpio_pin_getname),
DEVMETHOD(gpio_pin_setflags, sfgpio_pin_setflags),
DEVMETHOD(gpio_pin_access_32, sfgpio_pin_access_32),
DEVMETHOD(gpio_pin_config_32, sfgpio_pin_config_32),
/* ofw_bus interface */
DEVMETHOD(ofw_bus_get_node, sfgpio_get_node),
DEVMETHOD_END
};
static devclass_t sfgpio_devclass;
DEFINE_CLASS_0(gpio, sfgpio_driver, sfgpio_methods,
sizeof(struct sfgpio_softc));
EARLY_DRIVER_MODULE(gpio, simplebus, sfgpio_driver, sfgpio_devclass, 0, 0,
BUS_PASS_INTERRUPT + BUS_PASS_ORDER_LATE);
MODULE_DEPEND(sfgpio, gpiobus, 1, 1, 1);
View Options

sys/kern/kern_timeout.c

  • This file was added.
/*-
* SPDX-License-Identifier: BSD-2-Clause-FreeBSD
*
* Copyright (c) 2021 Jessica Clarke <jrtc27@FreeBSD.org>
*
* 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.
*
*/
/* TODO: Provide interrupt controller interface */
mhorneUnsubmitted
Not Done
Inline Actions

I would be interested in implementing this, but not sure how soon that would be...

mhorne: I would be interested in implementing this, but not sure how soon that would be...
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/bus.h>
#include <sys/kernel.h>
#include <sys/module.h>
#include <sys/rman.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/gpio.h>
#include <dev/gpio/gpiobusvar.h>
#include <dev/ofw/ofw_bus.h>
#include <dev/ofw/ofw_bus_subr.h>
#include <machine/bus.h>
/* Registers are 32-bit so can only fit 32 pins */
#define SFGPIO_MAX_PINS 32
#define SFGPIO_DEFAULT_CAPS (GPIO_PIN_INPUT | GPIO_PIN_OUTPUT)
#define SFGPIO_INPUT_VAL 0x0
#define SFGPIO_INPUT_EN 0x4
#define SFGPIO_OUTPUT_EN 0x8
#define SFGPIO_OUTPUT_VAL 0xc
#define SFGPIO_RISE_IE 0x18
#define SFGPIO_RISE_IP 0x1c
#define SFGPIO_FALL_IE 0x20
#define SFGPIO_FALL_IP 0x24
#define SFGPIO_HIGH_IE 0x28
#define SFGPIO_HIGH_IP 0x2c
#define SFGPIO_LOW_IE 0x30
#define SFGPIO_LOW_IP 0x34
struct sfgpio_softc {
device_t dev;
device_t busdev;
struct mtx mtx;
struct resource *mem_res;
int mem_rid;
struct resource *irq_res;
int irq_rid;
int npins;
struct gpio_pin gpio_pins[SFGPIO_MAX_PINS];
};
#define SFGPIO_LOCK(_sc) mtx_lock(&(_sc)->mtx)
#define SFGPIO_UNLOCK(_sc) mtx_unlock(&(_sc)->mtx)
#define SFGPIO_READ(_sc, _off) \
bus_read_4((_sc)->mem_res, (_off))
#define SFGPIO_WRITE(_sc, _off, _val) \
bus_write_4((_sc)->mem_res, (_off), (_val))
static struct ofw_compat_data compat_data[] = {
{ "sifive,gpio0", 1 },
{ NULL, 0 },
};
static int
sfgpio_probe(device_t dev)
{
if (!ofw_bus_status_okay(dev))
return (ENXIO);
if (ofw_bus_search_compatible(dev, compat_data)->ocd_data == 0)
return (ENXIO);
device_set_desc(dev, "SiFive GPIO Controller");
return (BUS_PROBE_DEFAULT);
}
static int
sfgpio_attach(device_t dev)
{
struct sfgpio_softc *sc;
phandle_t node;
int error, i;
pcell_t npins;
uint32_t input_en, output_en;
sc = device_get_softc(dev);
sc->dev = dev;
node = ofw_bus_get_node(dev);
mtx_init(&sc->mtx, device_get_nameunit(sc->dev), NULL, MTX_DEF);
sc->mem_rid = 0;
sc->mem_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY,
&sc->mem_rid, RF_ACTIVE);
if (sc->mem_res == NULL) {
device_printf(dev, "Cannot allocate memory resource\n");
error = ENXIO;
goto fail;
}
if (OF_getencprop(node, "ngpios", &npins, sizeof(npins)) <= 0) {
/* Optional; defaults to 16 */
npins = 16;
} else if (npins > SFGPIO_MAX_PINS) {
device_printf(dev, "Too many pins: %d\n", npins);
error = ENXIO;
goto fail;
}
sc->npins = npins;
sc->irq_rid = 0;
sc->irq_res = bus_alloc_resource_any(dev, SYS_RES_IRQ, &sc->irq_rid,
RF_ACTIVE);
if (sc->irq_res == NULL) {
device_printf(dev, "Cannot allocate IRQ resource\n");
error = ENXIO;
goto fail;
}
input_en = SFGPIO_READ(sc, SFGPIO_INPUT_EN);
output_en = SFGPIO_READ(sc, SFGPIO_OUTPUT_EN);
for (i = 0; i < sc->npins; ++i) {
sc->gpio_pins[i].gp_pin = i;
sc->gpio_pins[i].gp_caps = SFGPIO_DEFAULT_CAPS;
sc->gpio_pins[i].gp_flags =
((input_en & (1u << i)) ? GPIO_PIN_INPUT : 0) |
((output_en & (1u << i)) ? GPIO_PIN_OUTPUT : 0);
snprintf(sc->gpio_pins[i].gp_name, GPIOMAXNAME, "GPIO%d", i);
sc->gpio_pins[i].gp_name[GPIOMAXNAME - 1] = '\0';
}
manuUnsubmitted
Done
Inline Actions

Is the device_get_unit really necessary ?
That would mean that a sifive_gpio1 driver expose sifive_gpio1.X and since you would need to -f /dev/gpioc1 too that's a bit redondant.
Same for the 'sifive_gpio' prefix, for Allwinner and Rockchip we name them after the pad/bank-pin name as this is what is used in the schematics of the different boards.

manu: Is the device_get_unit really necessary ? That would mean that a sifive_gpio1 driver expose…
jrtc27AuthorUnsubmitted
Done
Inline Actions

I was using other drivers like dwgpio and qoriq_gpio as a reference. If what they're doing also doesn't make a huge amount of sense then this I can just change this.

The schematics just name them GPIOn, with a note saying what some of them are connected to.

jrtc27: I was using other drivers like dwgpio and qoriq_gpio as a reference. If what they're doing also…
sc->busdev = gpiobus_attach_bus(dev);
if (sc->busdev == NULL) {
device_printf(dev, "Cannot attach gpiobus\n");
error = ENXIO;
goto fail;
}
return (0);
fail:
if (sc->busdev != NULL)
gpiobus_detach_bus(dev);
if (sc->irq_res != NULL)
bus_release_resource(dev, SYS_RES_IRQ, sc->irq_rid,
sc->irq_res);
if (sc->mem_res != NULL)
bus_release_resource(dev, SYS_RES_MEMORY, sc->mem_rid,
sc->mem_res);
mtx_destroy(&sc->mtx);
return (error);
}
static device_t
sfgpio_get_bus(device_t dev)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
return (sc->busdev);
}
static int
sfgpio_pin_max(device_t dev, int *maxpin)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
*maxpin = sc->npins - 1;
return (0);
}
static int
sfgpio_pin_set(device_t dev, uint32_t pin, unsigned int val)
{
struct sfgpio_softc *sc;
uint32_t reg;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
mhorneUnsubmitted
Done
Inline Actions

Is it ever the case that gp_pin != i? Seems like these loops could be removed, if not.

mhorne: Is it ever the case that `gp_pin != i`? Seems like these loops could be removed, if not.
jrtc27AuthorUnsubmitted
Done
Inline Actions

This code is indeed weird. A bunch of other drivers have this exact pattern (basically most drivers using struct gpio_pin, there are some exceptions that do things more simply) and as far as I can tell will also never have the pins not match, I just copied this idiom from them... So maybe I should drop these loops and just assert that sc->gpio_pins[pin].gp_pin == pin?

jrtc27: This code is indeed weird. A bunch of other drivers have this exact pattern (basically most…
mhorneUnsubmitted
Done
Inline Actions

Yeah, seems okay to me to drop the loops here.

Looks like ar5315_gpio and ar71xx_gpio are drivers where this mapping isn't 1:1, but for most it doesn't matter.

mhorne: Yeah, seems okay to me to drop the loops here. Looks like `ar5315_gpio` and `ar71xx_gpio` are…
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_VAL);
if (val)
reg |= (1u << pin);
else
reg &= ~(1u << pin);
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_VAL, reg);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_get(device_t dev, uint32_t pin, unsigned int *val)
{
struct sfgpio_softc *sc;
uint32_t reg;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
if (sc->gpio_pins[pin].gp_flags & GPIO_PIN_OUTPUT)
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_VAL);
else
reg = SFGPIO_READ(sc, SFGPIO_INPUT_VAL);
*val = (reg & (1u << pin)) ? 1 : 0;
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_toggle(device_t dev, uint32_t pin)
{
struct sfgpio_softc *sc;
uint32_t reg;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_VAL);
reg ^= (1u << pin);
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_VAL, reg);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_getcaps(device_t dev, uint32_t pin, uint32_t *caps)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
*caps = sc->gpio_pins[pin].gp_caps;
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_getflags(device_t dev, uint32_t pin, uint32_t *flags)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
*flags = sc->gpio_pins[pin].gp_flags;
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_getname(device_t dev, uint32_t pin, char *name)
{
struct sfgpio_softc *sc;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
memcpy(name, sc->gpio_pins[pin].gp_name, GPIOMAXNAME);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_setflags(device_t dev, uint32_t pin, uint32_t flags)
{
struct sfgpio_softc *sc;
uint32_t reg;
sc = device_get_softc(dev);
if (pin >= sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
reg = SFGPIO_READ(sc, SFGPIO_INPUT_EN);
if (flags & GPIO_PIN_INPUT) {
reg |= (1u << pin);
sc->gpio_pins[pin].gp_flags |= GPIO_PIN_INPUT;
} else {
reg &= ~(1u << pin);
sc->gpio_pins[pin].gp_flags &= ~GPIO_PIN_INPUT;
}
SFGPIO_WRITE(sc, SFGPIO_INPUT_EN, reg);
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_EN);
if (flags & GPIO_PIN_OUTPUT) {
reg |= (1u << pin);
sc->gpio_pins[pin].gp_flags |= GPIO_PIN_OUTPUT;
} else {
reg &= ~(1u << pin);
sc->gpio_pins[pin].gp_flags &= ~GPIO_PIN_OUTPUT;
}
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_EN, reg);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_access_32(device_t dev, uint32_t first_pin, uint32_t clear_pins,
uint32_t change_pins, uint32_t *orig_pins)
{
struct sfgpio_softc *sc;
uint32_t reg;
if (first_pin != 0)
return (EINVAL);
sc = device_get_softc(dev);
SFGPIO_LOCK(sc);
reg = SFGPIO_READ(sc, SFGPIO_OUTPUT_VAL);
if (orig_pins != NULL)
/* Only input_val is implicitly masked by input_en */
*orig_pins = SFGPIO_READ(sc, SFGPIO_INPUT_VAL) |
(reg & SFGPIO_READ(sc, SFGPIO_OUTPUT_EN));
skiboUnsubmitted
Done
Inline Actions

I think you need to update sc->gpio_pins[i].gp_flags here.

skibo: I think you need to update sc->gpio_pins[i].gp_flags here.
if ((clear_pins | change_pins) != 0)
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_VAL,
(reg & ~clear_pins) ^ change_pins);
SFGPIO_UNLOCK(sc);
return (0);
}
static int
sfgpio_pin_config_32(device_t dev, uint32_t first_pin, uint32_t num_pins,
uint32_t *pin_flags)
{
struct sfgpio_softc *sc;
uint32_t ireg, oreg;
mhorneUnsubmitted
Not Done
Inline Actions

No detach method?

mhorne: No detach method?
manuUnsubmitted
Not Done
Inline Actions

Detach for gpio controller is complicated as it would mean to track any driver that uses a gpio. Also if there is any driver (which on embedded is more than likely as you often have reset lines etc ...) you couldn't detach.

manu: Detach for gpio controller is complicated as it would mean to track any driver that uses a gpio.
int i;
sc = device_get_softc(dev);
if (first_pin != 0 || num_pins > sc->npins)
return (EINVAL);
SFGPIO_LOCK(sc);
ireg = SFGPIO_READ(sc, SFGPIO_INPUT_EN);
oreg = SFGPIO_READ(sc, SFGPIO_OUTPUT_EN);
for (i = 0; i < num_pins; ++i) {
mhorneUnsubmitted
Not Done
Inline Actions

If you did the reads after the loop, you could reduce the time spent holding the lock. I don't think it's a big deal.

mhorne: If you did the reads after the loop, you could reduce the time spent holding the lock. I don't…
jrtc27AuthorUnsubmitted
Done
Inline Actions

That's true, though I'd then need [io]{set,clear} variables, and if this driver's lock is contended then I have serious questions... :)

jrtc27: That's true, though I'd then need [io]{set,clear} variables, and if this driver's lock is…
jrtc27AuthorUnsubmitted
Done
Inline Actions

Actually no, the sc->gpio_pins[i].gp_flags all need protecting by the lock too, so reordering the loop with the I/O reads doesn't let you delay taking the lock

jrtc27: Actually no, the sc->gpio_pins[i].gp_flags all need protecting by the lock too, so reordering…
if (pin_flags[i] & GPIO_PIN_INPUT) {
ireg |= (1u << i);
oreg &= ~(1u << i);
sc->gpio_pins[i].gp_flags |= GPIO_PIN_INPUT;
sc->gpio_pins[i].gp_flags &= ~GPIO_PIN_OUTPUT;
} else if (pin_flags[i] & GPIO_PIN_OUTPUT) {
ireg &= ~(1u << i);
oreg |= (1u << i);
sc->gpio_pins[i].gp_flags &= ~GPIO_PIN_INPUT;
sc->gpio_pins[i].gp_flags |= GPIO_PIN_OUTPUT;
}
}
SFGPIO_WRITE(sc, SFGPIO_INPUT_EN, ireg);
SFGPIO_WRITE(sc, SFGPIO_OUTPUT_EN, oreg);
SFGPIO_UNLOCK(sc);
return (0);
}
static phandle_t
sfgpio_get_node(device_t bus, device_t dev)
{
return (ofw_bus_get_node(bus));
}
static device_method_t sfgpio_methods[] = {
/* Device interface */
DEVMETHOD(device_probe, sfgpio_probe),
DEVMETHOD(device_attach, sfgpio_attach),
/* GPIO protocol */
DEVMETHOD(gpio_get_bus, sfgpio_get_bus),
DEVMETHOD(gpio_pin_max, sfgpio_pin_max),
DEVMETHOD(gpio_pin_set, sfgpio_pin_set),
DEVMETHOD(gpio_pin_get, sfgpio_pin_get),
DEVMETHOD(gpio_pin_toggle, sfgpio_pin_toggle),
DEVMETHOD(gpio_pin_getcaps, sfgpio_pin_getcaps),
DEVMETHOD(gpio_pin_getflags, sfgpio_pin_getflags),
DEVMETHOD(gpio_pin_getname, sfgpio_pin_getname),
DEVMETHOD(gpio_pin_setflags, sfgpio_pin_setflags),
DEVMETHOD(gpio_pin_access_32, sfgpio_pin_access_32),
DEVMETHOD(gpio_pin_config_32, sfgpio_pin_config_32),
/* ofw_bus interface */
DEVMETHOD(ofw_bus_get_node, sfgpio_get_node),
DEVMETHOD_END
};
static devclass_t sfgpio_devclass;
DEFINE_CLASS_0(gpio, sfgpio_driver, sfgpio_methods,
sizeof(struct sfgpio_softc));
EARLY_DRIVER_MODULE(gpio, simplebus, sfgpio_driver, sfgpio_devclass, 0, 0,
BUS_PASS_INTERRUPT + BUS_PASS_ORDER_LATE);
MODULE_DEPEND(sfgpio, gpiobus, 1, 1, 1);