| Message ID | 7d1a2156ddabe0b72964e88734adba307a472067.1440093298.git.plr.vincent@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 20, 2015 at 08:03:26PM +0200, Vincent Pelletier wrote: Hi Vincent, This patch breaks GPIO support for the Super-I/Os f71882fg and f71889f. > For some (unclear to me) reason, superio_enter/superio_exit, although > calling request_muxed_region/release_region (respectively), are not > sufficient on an multi-processor system to guarantee serialised access, > leading to the following errors: > [ 1210.586990] Trying to free nonexistent resource <000000000000002e-000000000000002f> > [ 1211.227414] Trying to free nonexistent resource <000000000000002e-000000000000002f> > [ 1211.867890] Trying to free nonexistent resource <000000000000002e-000000000000002f> > [ 1212.508299] Trying to free nonexistent resource <000000000000002e-000000000000002f> > [ 1213.148734] Trying to free nonexistent resource <000000000000002e-000000000000002f> > [ 1213.789172] Trying to free nonexistent resource <000000000000002e-000000000000002f> > [ 1214.429607] Trying to free nonexistent resource <000000000000002e-000000000000002f> I am quite surprised by this warnings. Although I have been using this driver intensively with the f71882fg and f71889f Super-I/Os on a large range of boards, I have never experimented such warnings. IMHO, understand *clearly* the issue could be a good start in order to fix it efficiently. Please, could you describe a setup (as simple as possible) allowing to reproduce the issue ? I'll try it on my side. > > Disabling all but one core make the error disappear, and re-enabling it > make it reappear. > > Loaded modules for this SuperIO chip on this system are: > 8250_fintek (not actively used) > fintek_cir (not actively used) > f71882fg (polled every 2 seconds from userland) > gpio_f7188x > > Loaded modules using GPIO functionality: > gpio_keys_polled (20ms polling period) > leds_gpio (usb-host and 500ms timer triggers) Please try to make the module list needed to reproduce the issue as short as possible. Ideally only gpio_f7188x would be needed. > > This change replaces frequent superio_enter/superio_exit and accesses via the > common GPIO IO region (0x2e..0x2f or 0x4e..0x4f) with mutex locking and > accesses via GPIO-dedicated IO region (pre-configured on chip). Unfortunately the f71882fg and f71889f Super-I/Os don't provide an I/O region dedicated to GPIOs. If it was the case, I would have used this way. But for this Super-I/O models, the GPIOs have to be configured through the global registers. That's why your patch breaks support with this models. However, accordingly to the datasheet, this feature seems to be supported by the f71869a model. Thanks, Simon > > Code inspired from hwmon/f71882fg . > > Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com> > --- > drivers/gpio/gpio-f7188x.c | 126 ++++++++++++++++++++++++++++----------------- > 1 file changed, 80 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c > index dbda843..2915f8d 100644 > --- a/drivers/gpio/gpio-f7188x.c > +++ b/drivers/gpio/gpio-f7188x.c > @@ -16,6 +16,8 @@ > #include <linux/platform_device.h> > #include <linux/io.h> > #include <linux/gpio.h> > +#include <linux/mutex.h> > +#include <linux/acpi.h> > > #define DRVNAME "gpio-f7188x" > > @@ -26,11 +28,17 @@ > #define SIO_DEVID 0x20 /* Device ID (2 bytes) */ > #define SIO_DEVREV 0x22 /* Device revision */ > #define SIO_MANID 0x23 /* Fintek ID (2 bytes) */ > +#define SIO_ENABLE 0x30 /* Logical device enable */ > +#define SIO_ADDR 0x60 /* Logical device address (2 bytes) */ > > #define SIO_LD_GPIO 0x06 /* GPIO logical device */ > #define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ > #define SIO_LOCK_KEY 0xAA /* Key to disable Super-I/O */ > > +#define REGION_LENGTH 8 > +#define ADDR_REG_OFFSET 5 > +#define DATA_REG_OFFSET 6 > + > #define SIO_FINTEK_ID 0x1934 /* Manufacturer ID */ > #define SIO_F71869_ID 0x0814 /* F71869 chipset ID */ > #define SIO_F71869A_ID 0x1007 /* F71869A chipset ID */ > @@ -47,8 +55,9 @@ static const char * const f7188x_names[] = { > }; > > struct f7188x_sio { > - int addr; > + u16 addr; > enum chips type; > + struct mutex lock; > }; > > struct f7188x_gpio_bank { > @@ -118,6 +127,22 @@ static inline void superio_exit(int base) > release_region(base, 2); > } > > +static u8 f7188x_read8(struct f7188x_sio *data, u8 reg) > +{ > + u8 val; > + > + outb(reg, data->addr + ADDR_REG_OFFSET); > + val = inb(data->addr + DATA_REG_OFFSET); > + > + return val; > +} > + > +static void f7188x_write8(struct f7188x_sio *data, u8 reg, u8 val) > +{ > + outb(reg, data->addr + ADDR_REG_OFFSET); > + outb(val, data->addr + DATA_REG_OFFSET); > +} > + > /* > * GPIO chip. > */ > @@ -192,47 +217,35 @@ static struct f7188x_gpio_bank f71889_gpio_bank[] = { > > static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset) > { > - int err; > struct f7188x_gpio_bank *bank = > container_of(chip, struct f7188x_gpio_bank, chip); > struct f7188x_sio *sio = bank->data->sio; > u8 dir; > > - err = superio_enter(sio->addr); > - if (err) > - return err; > - superio_select(sio->addr, SIO_LD_GPIO); > - > - dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); > + mutex_lock(&sio->lock); > + dir = f7188x_read8(sio, gpio_dir(bank->regbase)); > dir &= ~(1 << offset); > - superio_outb(sio->addr, gpio_dir(bank->regbase), dir); > - > - superio_exit(sio->addr); > + f7188x_write8(sio, gpio_dir(bank->regbase), dir); > + mutex_unlock(&sio->lock); > > return 0; > } > > static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset) > { > - int err; > struct f7188x_gpio_bank *bank = > container_of(chip, struct f7188x_gpio_bank, chip); > struct f7188x_sio *sio = bank->data->sio; > u8 dir, data; > > - err = superio_enter(sio->addr); > - if (err) > - return err; > - superio_select(sio->addr, SIO_LD_GPIO); > - > - dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); > + mutex_lock(&sio->lock); > + dir = f7188x_read8(sio, gpio_dir(bank->regbase)); > dir = !!(dir & (1 << offset)); > if (dir) > - data = superio_inb(sio->addr, gpio_data_out(bank->regbase)); > + data = f7188x_read8(sio, gpio_data_out(bank->regbase)); > else > - data = superio_inb(sio->addr, gpio_data_in(bank->regbase)); > - > - superio_exit(sio->addr); > + data = f7188x_read8(sio, gpio_data_in(bank->regbase)); > + mutex_unlock(&sio->lock); > > return !!(data & 1 << offset); > } > @@ -240,54 +253,42 @@ static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset) > static int f7188x_gpio_direction_out(struct gpio_chip *chip, > unsigned offset, int value) > { > - int err; > struct f7188x_gpio_bank *bank = > container_of(chip, struct f7188x_gpio_bank, chip); > struct f7188x_sio *sio = bank->data->sio; > u8 dir, data_out; > > - err = superio_enter(sio->addr); > - if (err) > - return err; > - superio_select(sio->addr, SIO_LD_GPIO); > - > - data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase)); > + mutex_lock(&sio->lock); > + data_out = f7188x_read8(sio, gpio_data_out(bank->regbase)); > if (value) > data_out |= (1 << offset); > else > data_out &= ~(1 << offset); > - superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out); > + f7188x_write8(sio, gpio_data_out(bank->regbase), data_out); > > - dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); > + dir = f7188x_read8(sio, gpio_dir(bank->regbase)); > dir |= (1 << offset); > - superio_outb(sio->addr, gpio_dir(bank->regbase), dir); > - > - superio_exit(sio->addr); > + f7188x_write8(sio, gpio_dir(bank->regbase), dir); > + mutex_unlock(&sio->lock); > > return 0; > } > > static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value) > { > - int err; > struct f7188x_gpio_bank *bank = > container_of(chip, struct f7188x_gpio_bank, chip); > struct f7188x_sio *sio = bank->data->sio; > u8 data_out; > > - err = superio_enter(sio->addr); > - if (err) > - return; > - superio_select(sio->addr, SIO_LD_GPIO); > - > - data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase)); > + mutex_lock(&sio->lock); > + data_out = f7188x_read8(sio, gpio_data_out(bank->regbase)); > if (value) > data_out |= (1 << offset); > else > data_out &= ~(1 << offset); > - superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out); > - > - superio_exit(sio->addr); > + f7188x_write8(sio, gpio_data_out(bank->regbase), data_out); > + mutex_unlock(&sio->lock); > } > > /* > @@ -326,6 +327,7 @@ static int f7188x_gpio_probe(struct platform_device *pdev) > return -ENODEV; > } > data->sio = sio; > + mutex_init(&sio->lock); > > platform_set_drvdata(pdev, data); > > @@ -372,6 +374,7 @@ static int f7188x_gpio_remove(struct platform_device *pdev) > static int __init f7188x_find(int addr, struct f7188x_sio *sio) > { > int err; > + u16 logical_device_address; > u16 devid; > > err = superio_enter(addr); > @@ -403,12 +406,25 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio) > pr_info(DRVNAME ": Unsupported Fintek device 0x%04x\n", devid); > goto err; > } > - sio->addr = addr; > + superio_select(addr, SIO_LD_GPIO); > + > + if (!(superio_inb(addr, SIO_ENABLE) & 0x01)) { > + pr_warn("Device not activated\n"); > + goto err; > + } > + > + logical_device_address = superio_inw(addr, SIO_ADDR); > + if (logical_device_address == 0) { > + pr_warn("Base address not set\n"); > + goto err; > + } > + logical_device_address &= ~(REGION_LENGTH - 1); /* Ignore 3 LSB */ > + sio->addr = logical_device_address; > err = 0; > > pr_info(DRVNAME ": Found %s at %#x, revision %d\n", > f7188x_names[sio->type], > - (unsigned int) addr, > + (unsigned int) logical_device_address, > (int) superio_inb(addr, SIO_DEVREV)); > > err: > @@ -421,12 +437,28 @@ static struct platform_device *f7188x_gpio_pdev; > static int __init > f7188x_gpio_device_add(const struct f7188x_sio *sio) > { > + struct resource res = { > + .start = sio->addr, > + .end = sio->addr + REGION_LENGTH - 1, > + .flags = IORESOURCE_IO, > + }; > int err; > > f7188x_gpio_pdev = platform_device_alloc(DRVNAME, -1); > if (!f7188x_gpio_pdev) > return -ENOMEM; > > + res.name = f7188x_gpio_pdev->name; > + err = acpi_check_resource_conflict(&res); > + if (err) > + goto err; > + > + err = platform_device_add_resources(f7188x_gpio_pdev, &res, 1); > + if (err) { > + pr_err("Device resource addition failed\n"); > + goto err; > + } > + > err = platform_device_add_data(f7188x_gpio_pdev, > sio, sizeof(*sio)); > if (err) { > @@ -467,6 +499,8 @@ static int __init f7188x_gpio_init(void) > int err; > struct f7188x_sio sio; > > + memset(&sio, 0, sizeof(sio)); > + > if (f7188x_find(0x2e, &sio) && > f7188x_find(0x4e, &sio)) > return -ENODEV; > -- > 2.5.0
Hello Simon, Thanks for reviewing my patch. On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot <simon.guinot@sequanux.org> wrote: > IMHO, understand *clearly* the issue could be a good start in order to > fix it efficiently. I totally agree, but have no idea how to debug further. Could you suggest any mechanism to debug *_resource ? > Please, could you describe a setup (as simple as possible) allowing to > reproduce the issue ? I'll try it on my side. [...] > Please try to make the module list needed to reproduce the issue as > short as possible. Ideally only gpio_f7188x would be needed. The simplest I could find so far needs gpio-input-polled with 20ms polling period (I didn't try to change polling period), and a shell loop writing to gpioXX/value. I'm using the following platform driver to declare a single gpio-input-polled key: https://github.com/vpelletier/linux/blob/free_nonexistent_resource/drivers/platform/x86/qnap-tsx51.c Full version of this driver: https://github.com/vpelletier/linux/blob/ts651/drivers/platform/x86/qnap-tsx51.c (ignore code surrounded with '#if QNAP_TSX51_GPIOD', it is still broken). It should be easy to create a variant fitting another board, as long as you can recycle two GPIOs. Once I've unloaded all other listed drivers for this chip and loaded the stripped-down qnap-tsx51 version: Terminal 0: echo 0 > /sys/devices/system/cpu/cpu1/online Terminal 1: cd /sys/class/gpio/ echo 62 > export cd gpio62 echo "out" > direction while :; echo 0 > value; echo 1 > value; done No error so far. Terminal 1: echo 1 > /sys/devices/cpu/cpu1/online After a few (10 to 30) seconds, tty is flooded with the error I reported. Stopping the loop stop the error. I have seen rare cases where a few extra messages could occur within the next minute or so, but this was before I stripped most out of the platform driver. With just two such loops poking at (out) GPIOs, it does not happen. I'm testing this on v4.1.4 with the following patch applied (the error happened before and after that patch, so it likely does not matter): https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7e08117de6ee17ae6c8f2983999a98cb95eb9bc2 CPU is an intel celeron J1800, dual-core at 2.41GHz. Exact board is this machine's motherboard: https://www.qnap.com/i/fr/product/model.php?II=144 > Unfortunately the f71882fg and f71889f Super-I/Os don't provide an > I/O region dedicated to GPIOs. If it was the case, I would have used > this way. But for this Super-I/O models, the GPIOs have to be configured > through the global registers. That's why your patch breaks support with > this models. Wow, I didn't expect such difference between models otherwise handled the same way in hwmon/f71882fg.c . I guess the GPIO function works differently from hwmon function in these models (hwmon still having dedicated IO range, not GPIO). Regards,
On Fri, 21 Aug 2015 22:48:24 +0200, Vincent Pelletier
<plr.vincent@gmail.com> wrote:
> With just two such loops poking at (out) GPIOs, it does not happen.
Re-reading my mail, I realise this was not clear: this is without
input-gpio-polled but just two shell loops switching a GPIO each.
Regards,
Hi, On Fri, 21 Aug 2015 22:48:24 +0200, Vincent Pelletier <plr.vincent@gmail.com> wrote: > On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot > <simon.guinot@sequanux.org> wrote: > > Please, could you describe a setup (as simple as possible) allowing to > > reproduce the issue ? I'll try it on my side. > [...] > > Please try to make the module list needed to reproduce the issue as > > short as possible. Ideally only gpio_f7188x would be needed. > > The simplest I could find so far needs gpio-input-polled with 20ms > polling period (I didn't try to change polling period), and a shell loop > writing to gpioXX/value. Have you had a chance to reproduce this issue ? Can I do something to help ? Regards,
On Thu, Sep 03, 2015 at 08:05:40PM +0200, Vincent Pelletier wrote: > Hi, > > On Fri, 21 Aug 2015 22:48:24 +0200, Vincent Pelletier > <plr.vincent@gmail.com> wrote: > > On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot > > <simon.guinot@sequanux.org> wrote: > > > Please, could you describe a setup (as simple as possible) allowing to > > > reproduce the issue ? I'll try it on my side. > > [...] > > > Please try to make the module list needed to reproduce the issue as > > > short as possible. Ideally only gpio_f7188x would be needed. > > > > The simplest I could find so far needs gpio-input-polled with 20ms > > polling period (I didn't try to change polling period), and a shell loop > > writing to gpioXX/value. > > Have you had a chance to reproduce this issue ? > Can I do something to help ? Still not but it is on my todo list. Regards, Simon
Hi Vincent. I'm working with Simon and I'm trying to reproduce the error you encountered with the f7188x driver. On Fri, Aug 21, 2015 at 10:48:24PM +0200, Vincent Pelletier wrote: > Hello Simon, > > Thanks for reviewing my patch. > > On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot > <simon.guinot@sequanux.org> wrote: > > IMHO, understand *clearly* the issue could be a good start in order to > > fix it efficiently. > > I totally agree, but have no idea how to debug further. > Could you suggest any mechanism to debug *_resource ? > > > Please, could you describe a setup (as simple as possible) allowing to > > reproduce the issue ? I'll try it on my side. > [...] > > Please try to make the module list needed to reproduce the issue as > > short as possible. Ideally only gpio_f7188x would be needed. > > The simplest I could find so far needs gpio-input-polled with 20ms > polling period (I didn't try to change polling period), and a shell loop > writing to gpioXX/value. > > I'm using the following platform driver to declare a single > gpio-input-polled key: > https://github.com/vpelletier/linux/blob/free_nonexistent_resource/drivers/platform/x86/qnap-tsx51.c > Full version of this driver: > https://github.com/vpelletier/linux/blob/ts651/drivers/platform/x86/qnap-tsx51.c > (ignore code surrounded with '#if QNAP_TSX51_GPIOD', it is > still broken). > > It should be easy to create a variant fitting another board, as long > as you can recycle two GPIOs. > > Once I've unloaded all other listed drivers for this chip and loaded > the stripped-down qnap-tsx51 version: > Terminal 0: > echo 0 > /sys/devices/system/cpu/cpu1/online > Terminal 1: > cd /sys/class/gpio/ > echo 62 > export > cd gpio62 > echo "out" > direction > while :; echo 0 > value; echo 1 > value; done > No error so far. > Terminal 1: > echo 1 > /sys/devices/cpu/cpu1/online > After a few (10 to 30) seconds, tty is flooded with the error I > reported. > > Stopping the loop stop the error. > I have seen rare cases where a few extra messages could occur within > the next minute or so, but this was before I stripped most out of the > platform driver. > I didn't manage to see the error you were talking about. To make sure I understood correctly your previous e-mail and since I saw some problems with commands you gave, here's what I tested on a 4.2-rc7: $ echo 62 > /sys/class/gpio/export $ cd /sys/class/gpio/gpio62 $ while true;do echo 0 > value; echo 1 > value; done ... I let the loop running during several minutes without having any error. Let me know if these commands are enough for you to reproduce the issue. I also tried as you, to remove/add CPUs but it didn't triggered anything more. $ for i in $(seq 1 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done $ echo 1 > /sys/devices/system/cpu/cpu1/online The last thing I tested is to perform SuperI/O access through the hmon driver while running the gpio loop. But once again no problem appeared. > With just two such loops poking at (out) GPIOs, it does not happen. > > I'm testing this on v4.1.4 with the following patch applied (the error > happened before and after that patch, so it likely does not matter): > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7e08117de6ee17ae6c8f2983999a98cb95eb9bc2 > > CPU is an intel celeron J1800, dual-core at 2.41GHz. > Exact board is this machine's motherboard: > https://www.qnap.com/i/fr/product/model.php?II=144 > > > Unfortunately the f71882fg and f71889f Super-I/Os don't provide an > > I/O region dedicated to GPIOs. If it was the case, I would have used > > this way. But for this Super-I/O models, the GPIOs have to be configured > > through the global registers. That's why your patch breaks support with > > this models. > > Wow, I didn't expect such difference between models otherwise handled > the same way in hwmon/f71882fg.c . I guess the GPIO function works > differently from hwmon function in these models (hwmon still having > dedicated IO range, not GPIO). > > Regards, > -- > Vincent Pelletier -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vincent, On Fri, 4 Sep 2015 15:48:54 +0200, Vincent Donnefort <vdonnefort@gmail.com> wrote: > ... I let the loop running during several minutes without having any error. > Let me know if these commands are enough for you to reproduce the issue. Did you also have gpio-input-polled active on another pin on the same chip ? Regards,
On Thu, Sep 03, 2015 at 08:05:40PM +0200, Vincent Pelletier wrote: > Hi, > > On Fri, 21 Aug 2015 22:48:24 +0200, Vincent Pelletier > <plr.vincent@gmail.com> wrote: > > On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot > > <simon.guinot@sequanux.org> wrote: > > > Please, could you describe a setup (as simple as possible) allowing to > > > reproduce the issue ? I'll try it on my side. > > [...] > > > Please try to make the module list needed to reproduce the issue as > > > short as possible. Ideally only gpio_f7188x would be needed. > > > > The simplest I could find so far needs gpio-input-polled with 20ms > > polling period (I didn't try to change polling period), and a shell loop > > writing to gpioXX/value. > > Have you had a chance to reproduce this issue ? > Can I do something to help ? Hi Vincent, Vincent (Donnefort) finally succeeds to reproduce the issue. The setup is quite simple. You only have to flood the gpio-f7188x driver via the sysfs GPIO interface. Nothing more is needed. After some debugging we discovered that the problem comes from the __request_region function which don't handle very well concurrent requests on a muxed region. I will send a patch as a reply to this email. Please, can you test it ? Thanks, Simon
Hello Simon, On Thu, 10 Sep 2015 00:01:40 +0200, Simon Guinot <simon.guinot@sequanux.org> wrote: > Vincent (Donnefort) finally succeeds to reproduce the issue. The setup > is quite simple. You only have to flood the gpio-f7188x driver via the > sysfs GPIO interface. Nothing more is needed. > > After some debugging we discovered that the problem comes from the > __request_region function which don't handle very well concurrent > requests on a muxed region. > > I will send a patch as a reply to this email. Please, can you test it ? I reverted my mutex-adding commit, applied given patch, and could not reproduce the error after a few minutes with my test-case, so I think this solves the issue. Tested-by: Vincent Pelletier <plr.vincent@gmail.com> I rebased my others gpio patches, unrelated to this issue: gpio: gpio-f7188x: Implement get_direction. gpio: gpio-f7188x: "get" should retrieve sensed level when available. gpio: gpio-f7188x: GPIO bank 0 bit 0 is not available on f71869a Should I resend ? I have not checked other model's datasheets, FWIW. Regards,
diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c index dbda843..2915f8d 100644 --- a/drivers/gpio/gpio-f7188x.c +++ b/drivers/gpio/gpio-f7188x.c @@ -16,6 +16,8 @@ #include <linux/platform_device.h> #include <linux/io.h> #include <linux/gpio.h> +#include <linux/mutex.h> +#include <linux/acpi.h> #define DRVNAME "gpio-f7188x" @@ -26,11 +28,17 @@ #define SIO_DEVID 0x20 /* Device ID (2 bytes) */ #define SIO_DEVREV 0x22 /* Device revision */ #define SIO_MANID 0x23 /* Fintek ID (2 bytes) */ +#define SIO_ENABLE 0x30 /* Logical device enable */ +#define SIO_ADDR 0x60 /* Logical device address (2 bytes) */ #define SIO_LD_GPIO 0x06 /* GPIO logical device */ #define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ #define SIO_LOCK_KEY 0xAA /* Key to disable Super-I/O */ +#define REGION_LENGTH 8 +#define ADDR_REG_OFFSET 5 +#define DATA_REG_OFFSET 6 + #define SIO_FINTEK_ID 0x1934 /* Manufacturer ID */ #define SIO_F71869_ID 0x0814 /* F71869 chipset ID */ #define SIO_F71869A_ID 0x1007 /* F71869A chipset ID */ @@ -47,8 +55,9 @@ static const char * const f7188x_names[] = { }; struct f7188x_sio { - int addr; + u16 addr; enum chips type; + struct mutex lock; }; struct f7188x_gpio_bank { @@ -118,6 +127,22 @@ static inline void superio_exit(int base) release_region(base, 2); } +static u8 f7188x_read8(struct f7188x_sio *data, u8 reg) +{ + u8 val; + + outb(reg, data->addr + ADDR_REG_OFFSET); + val = inb(data->addr + DATA_REG_OFFSET); + + return val; +} + +static void f7188x_write8(struct f7188x_sio *data, u8 reg, u8 val) +{ + outb(reg, data->addr + ADDR_REG_OFFSET); + outb(val, data->addr + DATA_REG_OFFSET); +} + /* * GPIO chip. */ @@ -192,47 +217,35 @@ static struct f7188x_gpio_bank f71889_gpio_bank[] = { static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset) { - int err; struct f7188x_gpio_bank *bank = container_of(chip, struct f7188x_gpio_bank, chip); struct f7188x_sio *sio = bank->data->sio; u8 dir; - err = superio_enter(sio->addr); - if (err) - return err; - superio_select(sio->addr, SIO_LD_GPIO); - - dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); + mutex_lock(&sio->lock); + dir = f7188x_read8(sio, gpio_dir(bank->regbase)); dir &= ~(1 << offset); - superio_outb(sio->addr, gpio_dir(bank->regbase), dir); - - superio_exit(sio->addr); + f7188x_write8(sio, gpio_dir(bank->regbase), dir); + mutex_unlock(&sio->lock); return 0; } static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset) { - int err; struct f7188x_gpio_bank *bank = container_of(chip, struct f7188x_gpio_bank, chip); struct f7188x_sio *sio = bank->data->sio; u8 dir, data; - err = superio_enter(sio->addr); - if (err) - return err; - superio_select(sio->addr, SIO_LD_GPIO); - - dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); + mutex_lock(&sio->lock); + dir = f7188x_read8(sio, gpio_dir(bank->regbase)); dir = !!(dir & (1 << offset)); if (dir) - data = superio_inb(sio->addr, gpio_data_out(bank->regbase)); + data = f7188x_read8(sio, gpio_data_out(bank->regbase)); else - data = superio_inb(sio->addr, gpio_data_in(bank->regbase)); - - superio_exit(sio->addr); + data = f7188x_read8(sio, gpio_data_in(bank->regbase)); + mutex_unlock(&sio->lock); return !!(data & 1 << offset); } @@ -240,54 +253,42 @@ static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset) static int f7188x_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value) { - int err; struct f7188x_gpio_bank *bank = container_of(chip, struct f7188x_gpio_bank, chip); struct f7188x_sio *sio = bank->data->sio; u8 dir, data_out; - err = superio_enter(sio->addr); - if (err) - return err; - superio_select(sio->addr, SIO_LD_GPIO); - - data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase)); + mutex_lock(&sio->lock); + data_out = f7188x_read8(sio, gpio_data_out(bank->regbase)); if (value) data_out |= (1 << offset); else data_out &= ~(1 << offset); - superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out); + f7188x_write8(sio, gpio_data_out(bank->regbase), data_out); - dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); + dir = f7188x_read8(sio, gpio_dir(bank->regbase)); dir |= (1 << offset); - superio_outb(sio->addr, gpio_dir(bank->regbase), dir); - - superio_exit(sio->addr); + f7188x_write8(sio, gpio_dir(bank->regbase), dir); + mutex_unlock(&sio->lock); return 0; } static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { - int err; struct f7188x_gpio_bank *bank = container_of(chip, struct f7188x_gpio_bank, chip); struct f7188x_sio *sio = bank->data->sio; u8 data_out; - err = superio_enter(sio->addr); - if (err) - return; - superio_select(sio->addr, SIO_LD_GPIO); - - data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase)); + mutex_lock(&sio->lock); + data_out = f7188x_read8(sio, gpio_data_out(bank->regbase)); if (value) data_out |= (1 << offset); else data_out &= ~(1 << offset); - superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out); - - superio_exit(sio->addr); + f7188x_write8(sio, gpio_data_out(bank->regbase), data_out); + mutex_unlock(&sio->lock); } /* @@ -326,6 +327,7 @@ static int f7188x_gpio_probe(struct platform_device *pdev) return -ENODEV; } data->sio = sio; + mutex_init(&sio->lock); platform_set_drvdata(pdev, data); @@ -372,6 +374,7 @@ static int f7188x_gpio_remove(struct platform_device *pdev) static int __init f7188x_find(int addr, struct f7188x_sio *sio) { int err; + u16 logical_device_address; u16 devid; err = superio_enter(addr); @@ -403,12 +406,25 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio) pr_info(DRVNAME ": Unsupported Fintek device 0x%04x\n", devid); goto err; } - sio->addr = addr; + superio_select(addr, SIO_LD_GPIO); + + if (!(superio_inb(addr, SIO_ENABLE) & 0x01)) { + pr_warn("Device not activated\n"); + goto err; + } + + logical_device_address = superio_inw(addr, SIO_ADDR); + if (logical_device_address == 0) { + pr_warn("Base address not set\n"); + goto err; + } + logical_device_address &= ~(REGION_LENGTH - 1); /* Ignore 3 LSB */ + sio->addr = logical_device_address; err = 0; pr_info(DRVNAME ": Found %s at %#x, revision %d\n", f7188x_names[sio->type], - (unsigned int) addr, + (unsigned int) logical_device_address, (int) superio_inb(addr, SIO_DEVREV)); err: @@ -421,12 +437,28 @@ static struct platform_device *f7188x_gpio_pdev; static int __init f7188x_gpio_device_add(const struct f7188x_sio *sio) { + struct resource res = { + .start = sio->addr, + .end = sio->addr + REGION_LENGTH - 1, + .flags = IORESOURCE_IO, + }; int err; f7188x_gpio_pdev = platform_device_alloc(DRVNAME, -1); if (!f7188x_gpio_pdev) return -ENOMEM; + res.name = f7188x_gpio_pdev->name; + err = acpi_check_resource_conflict(&res); + if (err) + goto err; + + err = platform_device_add_resources(f7188x_gpio_pdev, &res, 1); + if (err) { + pr_err("Device resource addition failed\n"); + goto err; + } + err = platform_device_add_data(f7188x_gpio_pdev, sio, sizeof(*sio)); if (err) { @@ -467,6 +499,8 @@ static int __init f7188x_gpio_init(void) int err; struct f7188x_sio sio; + memset(&sio, 0, sizeof(sio)); + if (f7188x_find(0x2e, &sio) && f7188x_find(0x4e, &sio)) return -ENODEV;
For some (unclear to me) reason, superio_enter/superio_exit, although calling request_muxed_region/release_region (respectively), are not sufficient on an multi-processor system to guarantee serialised access, leading to the following errors: [ 1210.586990] Trying to free nonexistent resource <000000000000002e-000000000000002f> [ 1211.227414] Trying to free nonexistent resource <000000000000002e-000000000000002f> [ 1211.867890] Trying to free nonexistent resource <000000000000002e-000000000000002f> [ 1212.508299] Trying to free nonexistent resource <000000000000002e-000000000000002f> [ 1213.148734] Trying to free nonexistent resource <000000000000002e-000000000000002f> [ 1213.789172] Trying to free nonexistent resource <000000000000002e-000000000000002f> [ 1214.429607] Trying to free nonexistent resource <000000000000002e-000000000000002f> Disabling all but one core make the error disappear, and re-enabling it make it reappear. Loaded modules for this SuperIO chip on this system are: 8250_fintek (not actively used) fintek_cir (not actively used) f71882fg (polled every 2 seconds from userland) gpio_f7188x Loaded modules using GPIO functionality: gpio_keys_polled (20ms polling period) leds_gpio (usb-host and 500ms timer triggers) This change replaces frequent superio_enter/superio_exit and accesses via the common GPIO IO region (0x2e..0x2f or 0x4e..0x4f) with mutex locking and accesses via GPIO-dedicated IO region (pre-configured on chip). Code inspired from hwmon/f71882fg . Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com> --- drivers/gpio/gpio-f7188x.c | 126 ++++++++++++++++++++++++++++----------------- 1 file changed, 80 insertions(+), 46 deletions(-)