From c79687f5f4e31492ef68207d80d0b008b9035366 Mon Sep 17 00:00:00 2001 From: Sebastian Sumpf Date: Mon, 5 Oct 2020 09:17:58 +0200 Subject: [PATCH] gpio: introduce Pin and '_with_gpio' - make GPIO server more robust on imx by not throwing exceptions for unknown pins, use '_with_gpio' instead - use 'Gpio::Pin' data type instead of POD 'unsigned' issue #3900 --- repos/os/include/gpio/component.h | 14 +-- repos/os/include/gpio/config.h | 10 +- repos/os/include/gpio/driver.h | 40 +++++--- repos/os/src/drivers/gpio/imx/driver.h | 134 +++++++++++++++---------- repos/os/src/drivers/gpio/rpi/driver.h | 65 ++++++------ 5 files changed, 153 insertions(+), 110 deletions(-) diff --git a/repos/os/include/gpio/component.h b/repos/os/include/gpio/component.h index 27aaeb9165..05326f5ea4 100644 --- a/repos/os/include/gpio/component.h +++ b/repos/os/include/gpio/component.h @@ -32,9 +32,9 @@ class Gpio::Session_component : public Genode::Rpc_object struct Irq_session_component : public Genode::Rpc_object { Driver &_driver; - unsigned long _pin; + Pin const &_pin; - Irq_session_component(Driver &driver, unsigned long pin) + Irq_session_component(Driver &driver, Pin const &pin) : _driver(driver), _pin(pin) { } @@ -51,7 +51,7 @@ class Gpio::Session_component : public Genode::Rpc_object Genode::Rpc_entrypoint &_ep; Driver &_driver; - unsigned long _pin; + Pin const _pin; Irq_session_component _irq_component; Genode::Irq_session_capability _irq_cap; @@ -61,8 +61,8 @@ class Gpio::Session_component : public Genode::Rpc_object Session_component(Genode::Rpc_entrypoint &ep, Driver &driver, - unsigned long gpio_pin) - : _ep(ep), _driver(driver), _pin(gpio_pin), + unsigned gpio_pin) + : _ep(ep), _driver(driver), _pin(Pin { gpio_pin }), _irq_component(_driver, _pin), _irq_cap(_ep.manage(&_irq_component)) { } @@ -120,12 +120,12 @@ class Gpio::Root : public Genode::Root_component Session_component *_create_session(const char *args) override { - unsigned long pin = + unsigned pin = Genode::Arg_string::find_arg(args, "gpio").ulong_value(0); Genode::size_t ram_quota = Genode::Arg_string::find_arg(args, "ram_quota").ulong_value(0); - if (!_driver.gpio_valid(pin)) + if (!_driver.gpio_valid(Pin { pin })) throw Genode::Service_denied(); if (ram_quota < sizeof(Session_component)) { diff --git a/repos/os/include/gpio/config.h b/repos/os/include/gpio/config.h index ff32274bb5..cb18716870 100644 --- a/repos/os/include/gpio/config.h +++ b/repos/os/include/gpio/config.h @@ -48,7 +48,9 @@ void Gpio::process_config(Genode::Xml_node const &config, Gpio::Driver &driver) config.for_each_sub_node("gpio", [&] (Genode::Xml_node const &gpio_node) { unsigned const num = gpio_node.attribute_value("num", 0U); - if (!driver.gpio_valid(num)) { + Pin gpio { num }; + + if (!driver.gpio_valid(gpio)) { Genode::warning("invalid GPIO number ", num, ", ignore node"); return; } @@ -60,11 +62,11 @@ void Gpio::process_config(Genode::Xml_node const &config, Gpio::Driver &driver) if (mode == "O" || mode == "o") { value = gpio_node.attribute_value("value", value); - driver.write(num, value); - driver.direction(num, false); + driver.write(gpio, value); + driver.direction(Pin {num}, false); } else if (mode == "I" || mode == "i") { - driver.direction(num, true); + driver.direction(Pin {num}, true); } else { Genode::error("gpio ", num, " has invalid mode, must be I or O"); diff --git a/repos/os/include/gpio/driver.h b/repos/os/include/gpio/driver.h index a442ddd464..78bd9ef1e6 100644 --- a/repos/os/include/gpio/driver.h +++ b/repos/os/include/gpio/driver.h @@ -15,10 +15,20 @@ #define _INCLUDE__GPIO__DRIVER_H_ /* Genode includes */ +#include #include -namespace Gpio { struct Driver; } +namespace Gpio { + struct Driver; + struct Pin; +} +struct Gpio::Pin +{ + unsigned value; + + void print(Genode::Output &out) const { Genode::print(out, value); } +}; struct Gpio::Driver : Genode::Interface { @@ -27,84 +37,84 @@ struct Gpio::Driver : Genode::Interface * *\param gpio corresponding gpio pin number */ - virtual void direction(unsigned gpio, bool input) = 0; + virtual void direction(Pin gpio, bool input) = 0; /** * Set output level (high or low) * *\param gpio corresponding gpio pin number */ - virtual void write(unsigned gpio, bool enable) = 0; + virtual void write(Pin gpio, bool enable) = 0; /** * Read input level (high or low) * *\param gpio corresponding gpio pin number */ - virtual bool read(unsigned gpio) = 0; + virtual bool read(Pin gpio) = 0; /** * Enable/disable debounce logic for the GPIO pin * *\param gpio corresponding gpio pin number */ - virtual void debounce_enable(unsigned gpio, bool enable) = 0; + virtual void debounce_enable(Pin gpio, bool enable) = 0; /** * Set delay for debounce logic for the GPIO pin * *\param gpio corresponding gpio pin number */ - virtual void debounce_time(unsigned gpio, unsigned long us) = 0; + virtual void debounce_time(Pin gpio, unsigned long us) = 0; /** * Set IRQ trigger state to falling edge-triggered * *\param gpio corresponding gpio pin number */ - virtual void falling_detect(unsigned gpio) = 0; + virtual void falling_detect(Pin gpio) = 0; /** * Set IRQ trigger state to rising edge-triggered * *\param gpio corresponding gpio pin number */ - virtual void rising_detect(unsigned gpio) = 0; + virtual void rising_detect(Pin gpio) = 0; /** * Set IRQ trigger state to high level-triggered * *\param gpio corresponding gpio pin number */ - virtual void high_detect(unsigned gpio) = 0; + virtual void high_detect(Pin gpio) = 0; /** * Set IRQ trigger state to low level-triggered * *\param gpio corresponding gpio pin number */ - virtual void low_detect(unsigned gpio) = 0; + virtual void low_detect(Pin gpio) = 0; /** * Enable/disable IRQ for specified GPIO pin * *\param gpio corresponding gpio pin number */ - virtual void irq_enable(unsigned gpio, bool enable) = 0; + virtual void irq_enable(Pin gpio, bool enable) = 0; /** * Acknowledge IRQ for specified GPIO pin * * \param gpio corresponding gpio pin number */ - virtual void ack_irq(unsigned gpio) = 0; + virtual void ack_irq(Pin gpio) = 0; /** * Register signal handler for interrupts * *\param gpio corresponding gpio pin number */ - virtual void register_signal(unsigned gpio, + virtual void register_signal(Pin gpio, Genode::Signal_context_capability cap) = 0; /** @@ -112,14 +122,14 @@ struct Gpio::Driver : Genode::Interface * *\param gpio corresponding gpio pin number */ - virtual void unregister_signal(unsigned gpio) = 0; + virtual void unregister_signal(Pin gpio) = 0; /** * Check whether GPIO number is valid * *\param gpio corresponding gpio pin number */ - virtual bool gpio_valid(unsigned gpio) = 0; + virtual bool gpio_valid(Pin gpio) = 0; }; #endif /* _INCLUDE__GPIO__DRIVER_H_ */ diff --git a/repos/os/src/drivers/gpio/imx/driver.h b/repos/os/src/drivers/gpio/imx/driver.h index 4a9b9879e0..16f376863f 100644 --- a/repos/os/src/drivers/gpio/imx/driver.h +++ b/repos/os/src/drivers/gpio/imx/driver.h @@ -28,6 +28,7 @@ class Imx_driver : public Gpio::Driver { + using Pin = Gpio::Pin; private: enum { @@ -102,7 +103,7 @@ class Imx_driver : public Gpio::Driver _irqh_low(env, irq_low, this), _irqh_high(env, irq_high, this) { } - Gpio_reg* regs() { return &_reg; } + Gpio_reg& regs() { return _reg; } void irq(int pin, bool enable) { @@ -123,19 +124,20 @@ class Imx_driver : public Gpio::Driver Platform::Connection _platform_connection; Genode::Constructible _gpio_banks[MAX_BANKS]; - Gpio_bank &_gpio_bank(int gpio) + template + void _with_gpio(Pin gpio, FN const &fn) { - int bank = gpio >> PIN_SHIFT; + unsigned bank = gpio.value >> PIN_SHIFT; if (bank >= MAX_BANKS) { - Genode::error("no Gpio_bank for pin ", gpio, " available"); - throw -1; + Genode::warning("no Gpio_bank for pin ", gpio, " ignoring"); + return; } - return *_gpio_banks[bank]; + fn(*_gpio_banks[bank]); } - int _gpio_index(int gpio) { return gpio & 0x1f; } + unsigned _gpio_index(Pin gpio) const { return gpio.value & 0x1f; } public: @@ -175,12 +177,12 @@ class Imx_driver : public Gpio::Driver _gpio_banks[i].construct(env, io_mem, irq_low, irq_high); - Gpio_reg *regs = _gpio_banks[i]->regs(); + Gpio_reg ®s = _gpio_banks[i]->regs(); for (unsigned j = 0; j < MAX_PINS; j++) { - regs->write(Gpio_reg::Int_conf::LOW_LEVEL, j); - regs->write(0, j); + regs.write(Gpio_reg::Int_conf::LOW_LEVEL, j); + regs.write(0, j); } - regs->write(0xffffffff); + regs.write(0xffffffff); i++; }); @@ -191,87 +193,113 @@ class Imx_driver : public Gpio::Driver ** Gpio::Driver interface ** ******************************/ - void direction(unsigned gpio, bool input) override + void direction(Pin gpio, bool input) override { - Gpio_reg *gpio_reg = _gpio_bank(gpio).regs(); - gpio_reg->write(input ? 0 : 1, - _gpio_index(gpio)); + _with_gpio(gpio, [&](Gpio_bank &bank) { + Gpio_reg &gpio_reg = bank.regs(); + gpio_reg.write(input ? 0 : 1, + _gpio_index(gpio)); + }); } - void write(unsigned gpio, bool level) override + void write(Pin gpio, bool level) override { - Gpio_reg *gpio_reg = _gpio_bank(gpio).regs(); - - gpio_reg->write(level ? 1 : 0, - _gpio_index(gpio)); + _with_gpio(gpio, [&](Gpio_bank &bank) { + Gpio_reg &gpio_reg = bank.regs(); + gpio_reg.write(level ? 1 : 0, + _gpio_index(gpio)); + }); } - bool read(unsigned gpio) override + bool read(Pin gpio) override { - Gpio_reg *gpio_reg = _gpio_bank(gpio).regs(); - return gpio_reg->read(_gpio_index(gpio)); + bool ret = false; + + _with_gpio(gpio, [&](Gpio_bank &bank) { + Gpio_reg &gpio_reg = bank.regs(); + ret = gpio_reg.read(_gpio_index(gpio)); + }); + + return ret; } - void debounce_enable(unsigned /* gpio */, bool /* enable */) override + void debounce_enable(Pin /* gpio */, bool /* enable */) override { Genode::warning("debounce enable not supported"); } - void debounce_time(unsigned /* gpio */, unsigned long /* us */) override + void debounce_time(Pin /* gpio */, unsigned long /* us */) override { Genode::warning("debounce time not supported"); } - void falling_detect(unsigned gpio) override + void falling_detect(Pin gpio) override { - Gpio_reg *gpio_reg = _gpio_bank(gpio).regs(); - gpio_reg->write(Gpio_reg::Int_conf::FAL_EDGE, - _gpio_index(gpio)); + _with_gpio(gpio, [&](Gpio_bank &bank) { + Gpio_reg &gpio_reg = bank.regs(); + gpio_reg.write(Gpio_reg::Int_conf::FAL_EDGE, + _gpio_index(gpio)); + }); } - void rising_detect(unsigned gpio) override + void rising_detect(Pin gpio) override { - Gpio_reg *gpio_reg = _gpio_bank(gpio).regs(); - gpio_reg->write(Gpio_reg::Int_conf::RIS_EDGE, - _gpio_index(gpio)); + _with_gpio(gpio, [&](Gpio_bank &bank) { + Gpio_reg &gpio_reg = bank.regs(); + gpio_reg.write(Gpio_reg::Int_conf::RIS_EDGE, + _gpio_index(gpio)); + }); } - void high_detect(unsigned gpio) override + void high_detect(Pin gpio) override { - Gpio_reg *gpio_reg = _gpio_bank(gpio).regs(); - gpio_reg->write(Gpio_reg::Int_conf::HIGH_LEVEL, - _gpio_index(gpio)); + _with_gpio(gpio, [&](Gpio_bank &bank) { + Gpio_reg &gpio_reg = bank.regs(); + gpio_reg.write(Gpio_reg::Int_conf::HIGH_LEVEL, + _gpio_index(gpio)); + }); } - void low_detect(unsigned gpio) override + void low_detect(Pin gpio) override { - Gpio_reg *gpio_reg = _gpio_bank(gpio).regs(); - gpio_reg->write(Gpio_reg::Int_conf::LOW_LEVEL, - _gpio_index(gpio)); + _with_gpio(gpio, [&](Gpio_bank &bank) { + Gpio_reg &gpio_reg = bank.regs(); + gpio_reg.write(Gpio_reg::Int_conf::LOW_LEVEL, + _gpio_index(gpio)); + }); } - void irq_enable(unsigned gpio, bool enable) override + void irq_enable(Pin gpio, bool enable) override { - _gpio_bank(gpio).irq(_gpio_index(gpio), enable); + _with_gpio(gpio, [&](Gpio_bank &bank) { + bank.irq(_gpio_index(gpio), enable); + }); } - void ack_irq(unsigned gpio) override + void ack_irq(Pin gpio) override { - _gpio_bank(gpio).ack_irq(_gpio_index(gpio)); + _with_gpio(gpio, [&](Gpio_bank &bank) { + bank.ack_irq(_gpio_index(gpio)); + }); } - void register_signal(unsigned gpio, + void register_signal(Pin gpio, Genode::Signal_context_capability cap) override { - _gpio_bank(gpio).sigh(_gpio_index(gpio), cap); } - - void unregister_signal(unsigned gpio) override - { - Genode::Signal_context_capability cap; - _gpio_bank(gpio).sigh(_gpio_index(gpio), cap); + _with_gpio(gpio, [&](Gpio_bank &bank) { + bank.sigh(_gpio_index(gpio), cap); + }); } - bool gpio_valid(unsigned gpio) override { return gpio < (MAX_PINS*MAX_BANKS); } + void unregister_signal(Pin gpio) override + { + Genode::Signal_context_capability cap; + _with_gpio(gpio, [&](Gpio_bank &bank) { + bank.sigh(_gpio_index(gpio), cap); + }); + } + + bool gpio_valid(Pin gpio) override { return gpio.value < (MAX_PINS*MAX_BANKS); } }; #endif /* _DRIVERS__GPIO__IMX__DRIVER_H_ */ diff --git a/repos/os/src/drivers/gpio/rpi/driver.h b/repos/os/src/drivers/gpio/rpi/driver.h index 5fb0a3ce4a..3ed90c5a50 100644 --- a/repos/os/src/drivers/gpio/rpi/driver.h +++ b/repos/os/src/drivers/gpio/rpi/driver.h @@ -30,8 +30,11 @@ static int verbose = 1; namespace Gpio { class Rpi_driver; } + class Gpio::Rpi_driver : public Driver { + using Pin = Gpio::Pin; + private: enum { @@ -54,7 +57,7 @@ class Gpio::Rpi_driver : public Driver }); } - void _invalid_gpio(unsigned gpio) { + void _invalid_gpio(Pin gpio) { Genode::error("invalid GPIO pin number ", gpio); } public: @@ -85,9 +88,9 @@ class Gpio::Rpi_driver : public Driver ** Driver interface ** ******************************/ - bool gpio_valid(unsigned gpio) override { return gpio < MAX_PINS; } + bool gpio_valid(Pin gpio) override { return gpio.value < MAX_PINS; } - void direction(unsigned gpio, bool input) override + void direction(Pin gpio, bool input) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } @@ -95,113 +98,113 @@ class Gpio::Rpi_driver : public Driver Genode::log("direction: gpio=", gpio, " input=", input); Reg::Function f = input ? Reg::FSEL_INPUT : Reg::FSEL_OUTPUT; - _reg.set_gpio_function(gpio, f); + _reg.set_gpio_function(gpio.value, f); } - void write(unsigned gpio, bool level) override + void write(Pin gpio, bool level) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("write: gpio=", gpio, " level=", level); - if (_reg.get_gpio_function(gpio)!=Reg::FSEL_OUTPUT) + if (_reg.get_gpio_function(gpio.value)!=Reg::FSEL_OUTPUT) warning("GPIO pin ", gpio, " is not configured for output"); if (level) - _reg.set_gpio_level(gpio); + _reg.set_gpio_level(gpio.value); else - _reg.clear_gpio_level(gpio); + _reg.clear_gpio_level(gpio.value); } - bool read(unsigned gpio) override + bool read(Pin gpio) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return 0; } - if(_reg.get_gpio_function(gpio) != Reg::FSEL_INPUT) + if(_reg.get_gpio_function(gpio.value) != Reg::FSEL_INPUT) warning("GPIO pin ", gpio, " is not configured for input"); - return _reg.get_gpio_level(gpio); + return _reg.get_gpio_level(gpio.value); } - void debounce_enable(unsigned, bool) override { + void debounce_enable(Pin, bool) override { Genode::warning("debounce_enable not supported!"); } - void debounce_time(unsigned, unsigned long) override { + void debounce_time(Pin, unsigned long) override { Genode::warning("debounce_time not supported!"); } - void falling_detect(unsigned gpio) override + void falling_detect(Pin gpio) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("falling_detect: gpio=", gpio); if(_async) - _reg.set_gpio_async_falling_detect(gpio); + _reg.set_gpio_async_falling_detect(gpio.value); else - _reg.set_gpio_falling_detect(gpio); + _reg.set_gpio_falling_detect(gpio.value); } - void rising_detect(unsigned gpio) override + void rising_detect(Pin gpio) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("rising_detect: gpio=", gpio); if(_async) - _reg.set_gpio_async_rising_detect(gpio); + _reg.set_gpio_async_rising_detect(gpio.value); else - _reg.set_gpio_rising_detect(gpio); + _reg.set_gpio_rising_detect(gpio.value); } - void high_detect(unsigned gpio) override + void high_detect(Pin gpio) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("high_detect: gpio=", gpio); - _reg.set_gpio_high_detect(gpio); + _reg.set_gpio_high_detect(gpio.value); } - void low_detect(unsigned gpio) override + void low_detect(Pin gpio) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("low_detect: gpio=", gpio); - _reg.set_gpio_low_detect(gpio); + _reg.set_gpio_low_detect(gpio.value); } - void irq_enable(unsigned gpio, bool enable) override + void irq_enable(Pin gpio, bool enable) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("irq_enable: gpio=", gpio, " enable=", enable); - _irq_enabled[gpio] = enable; + _irq_enabled[gpio.value] = enable; } - void ack_irq(unsigned gpio) override + void ack_irq(Pin gpio) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("ack_irq: gpio=", gpio); - _reg.clear_event(gpio); + _reg.clear_event(gpio.value); _irq.ack_irq(); } - void register_signal(unsigned gpio, + void register_signal(Pin gpio, Genode::Signal_context_capability cap) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("register_signal: gpio=", gpio); - _sig_cap[gpio] = cap; + _sig_cap[gpio.value] = cap; } - void unregister_signal(unsigned gpio) override + void unregister_signal(Pin gpio) override { if (!gpio_valid(gpio)) { _invalid_gpio(gpio); return; } if (verbose) Genode::log("unregister_signal: gpio=", gpio); Genode::Signal_context_capability cap; - _sig_cap[gpio] = cap; + _sig_cap[gpio.value] = cap; } };