Message ID | 1549588593-4856-1-git-send-email-lkml@metux.net (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | [1/2] x86: gpio: AMD G-Series pch gpio platform driver | expand |
Hi Enrico! Thanks for your patch! I would really like Andy to have a look at it too, he's more on top of things in the x86 world. On Fri, Feb 8, 2019 at 2:16 AM Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > From: "Enrico Weigelt, metux IT consult" <info@metux.net> > > GPIO platform driver for the AMD G-series PCH (eg. on GX-412TC) > > This driver doesn't registers itself automatically, as it needs to > be provided with platform specific configuration, provided by some > board driver setup code. > > Didn't implement oftree probing yet, as it's rarely found on x86. > > Cc: linux-gpio@vger.kernel.org > Cc: linus.walleij@linaro.org > Cc: bgolaszewski@baylibre.com > Cc: dvhart@infradead.org > Cc: andy@infradead.org > Cc: platform-driver-x86@vger.kernel.org > > Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> (...) > +config GPIO_AMD_FCH > + tristate "GPIO support for AMD Fusion Controller Hub (G-series SOCs)" > + select GPIO_GENERIC You are selecting GPIO_GENERIC, is this necessary? I thought X86 was already selecting this. > +/* > + * GPIO driver for the AMD G series FCH (eg. GX-412TC) > + * > + * Copyright (C) 2018 metux IT consult > + * Author: Enrico Weigelt <info@metux.net> > + * > + * SPDX-License-Identifier: GPL+ > + */ I think checkpatch will complain on that SPDX thing. Copy something from one of the other drivers, it should be on the first line of the file. > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/gpio/driver.h> > +#include <linux/platform_data/x86/amd-fch-gpio-pdata.h> > + > + > +#define GPIO_BIT_DIR 23 > +#define GPIO_BIT_WRITE 22 > +#define GPIO_BIT_READ 16 > + > + Cut down the excessive newlines. > +static void amd_fch_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc) > +{ > + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); > + (void)priv; > + > + seq_printf(s, "debug info not implemented yet\n"); > +} I think you can just skip implementing this then. > +static int amd_fch_gpio_request(struct gpio_chip *chip, unsigned gpio_pin) > +{ > + if (gpio_pin < chip->ngpio) > + return 0; > + > + return -EINVAL; > +} You can probably skip this too. The core already does this check. > + priv->gc.owner = THIS_MODULE; > + priv->gc.parent = &pdev->dev; > + priv->gc.label = dev_name(&pdev->dev); > + priv->gc.base = priv->pdata->gpio_base; No please, use priv->gc.base = -1; > + dev_info(&pdev->dev, "initializing on my own II\n"); Drop this. > + if (IS_ENABLED(CONFIG_DEBUG_FS)) { > + dev_info(&pdev->dev, "enabling debugfs\n"); > + priv->gc.dbg_show = amd_fch_gpio_dbg_show; > + } I think you can drop this too. > + platform_set_drvdata(pdev, priv); > + > + err = devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv); > + dev_info(&pdev->dev, "probe finished\n"); If you keep this info, write something more helpful. > +/* > + * struct amd_fch_gpio_reg - GPIO register definition > + * @reg: register index > + * @name: signal name > + */ > +struct amd_fch_gpio_reg { > + int reg; > + const char* name; > +}; Can't you put this in the driver file? > +struct amd_fch_gpio_pdata { > + struct resource res; > + int gpio_num; > + struct amd_fch_gpio_reg *gpio_reg; > + int gpio_base; > +}; Drop gpio_base. We don't hardcode the GPIO base anymore. Use the character device instead if you want it because of userspace thingies. (See tools/gpio/*) Yours, Linus Walleij
On Fri, Feb 8, 2019 at 4:26 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > Hi Enrico! > > Thanks for your patch! I would really like Andy to have a look at it > too, he's more on top of things in the x86 world. Seems that is the same version sent twice. I already gave a long review on it.
On Fri, 2019-02-08 at 15:25 +0100, Linus Walleij wrote: > > +/* > > + * GPIO driver for the AMD G series FCH (eg. GX-412TC) > > + * > > + * Copyright (C) 2018 metux IT consult > > + * Author: Enrico Weigelt <info@metux.net> > > + * > > + * SPDX-License-Identifier: GPL+ > > + */ > > I think checkpatch will complain on that SPDX thing. > Copy something from one of the other drivers, it should be > on the first line of the file. Actually, checkpatch will complain that there isn't an SPDX license on line 1 but will not complain about the existence of an SPDX license on other line numbers. Perhaps checkpatch _should_ complain when it sees an SPDX license identifier outside the expected line.
On 08.02.19 15:25, Linus Walleij wrote:
Hi,
<snip>
>> +/*>> + * struct amd_fch_gpio_reg - GPIO register definition>> + * @reg:
register index>> + * @name: signal name>> + */>> +struct
amd_fch_gpio_reg {>> + int reg;>> + const char* name;>>
+};> > Can't you put this in the driver file?
I'm afraid, I can't. Because the board driver needs to tell the
gpio driver which gpio's we actually have. This seems to be really
board specific, and the register layout of the FCHs gpio bank seems
to be not actually linear (or maybe the pin naming is just weird)
What I really don't want is undocumented registers or lines being
exposed as gpio somewhere (not knowing what they actually do).
Therefore, I'd like to add only those gpio registers that I can
confirm being safe - at least until I've managed to get more
information.
--mtx
On Mon, Feb 11, 2019 at 11:46 AM Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > On 08.02.19 15:25, Linus Walleij wrote: > >> +/*>> + * struct amd_fch_gpio_reg - GPIO register definition>> + * @reg: > register index>> + * @name: signal name>> + */>> +struct > amd_fch_gpio_reg {>> + int reg;>> + const char* name;>> > +};> > Can't you put this in the driver file? > > I'm afraid, I can't. Because the board driver needs to tell the > gpio driver which gpio's we actually have. This seems to be really > board specific, and the register layout of the FCHs gpio bank seems > to be not actually linear (or maybe the pin naming is just weird) > > What I really don't want is undocumented registers or lines being > exposed as gpio somewhere (not knowing what they actually do). > Therefore, I'd like to add only those gpio registers that I can > confirm being safe - at least until I've managed to get more > information. What we normally do is expose all the lines from a gpio chip as kernel abstraction. This is because at least in theory this chip can be used by several boards. The chip abstraction does have facilities for masking off unavailable lines: in struct gpio_chip there is .valid_mask and even a callback .init_valid_mask() to set it up. This is currently used on ACPI systems to mask off an make unavailable lines that are handled by firmware/BIOS. So this is what you should use to make existing lines unavailable, do not try to hide them by not exposing registers or line offsets. The abstraction should model the hardware, not the usecase. Imposing a restriction from a board is a new thing, and would involve setting these masks somehow using machine data from <linux/gpio/machine.h>, I am sure we can figure something out if you absolutely need this. FYI there are tons of systems out there that expose a whole range of GPIOs that the developer have no clue where they are connected on the PCB, the most common case is that they are not connected (sometimes not even leaving the chip die) or go to test points. They are very seldom dangerous, in fact I've never seen dangerous GPIOs, just dangerous set-ups of pins already known to be in use for something. Yours, Linus Walleij
On 13.02.19 10:02, Linus Walleij wrote: Hi, > What we normally do is expose all the lines from a gpio chip> as kernel abstraction.> > This is because at least in theory this chip can be used by> several boards. I'm afraid it's a bit more complicated. The register set doesn't seem to be linear. I have absolutely no idea what the undocumented registers do. For the sake of security, I would hate to see them accessible from userland. > The chip abstraction does have facilities for masking off > unavailable lines: in struct gpio_chip there is .valid_mask > and even a callback .init_valid_mask() to set it up. Okay, but then again I'd need to pass this from the board driver into the gpio driver. Feels much easier and safer to just pass the registers. What I perhaps could do: add all documented registers as gpio's in the gpio driver and make the board driver a dummy consumer for the unused ones. > So this is what you should use to make existing lines > unavailable, do not try to hide them by not exposing > registers or line offsets. The abstraction should model > the hardware, not the usecase. According to my information, it is the hardware who dictates that non-linear layout. Even the PIN naming is non-linear. And I don't like to see this passed to upper layers, or even userland. So, at least the gpio driver should only serve the documented gpios and present them lineary. Maybe the holes in the register set are technically also gpios, but used by some internal logic and not properly masked out in hw - really weird things (possibly damage) could happen. > FYI there are tons of systems out there that expose a > whole range of GPIOs that the developer have no clue > where they are connected on the PCB, the most common > case is that they are not connected (sometimes not > even leaving the chip die) or go to test points. They are > very seldom dangerous, in fact I've never seen dangerous > GPIOs, just dangerous set-ups of pins already known > to be in use for something. Can't speak about standard ICs/SoCs, but i've seen such things in some of my client's fpga designs. For example incomplete decoders, gpios attached to internal state machines, etc, etc So, I've learned to be *very* cautious with undocumented registers. --mtx
On 08.02.19 15:25, Linus Walleij wrote: Hi, >> +config GPIO_AMD_FCH >> + tristate "GPIO support for AMD Fusion Controller Hub (G-series SOCs)" >> + select GPIO_GENERIC > > You are selecting GPIO_GENERIC, is this necessary? I thought > X86 was already selecting this. Doesn't look so - at least haven't found anything where it's automatically selected on x86. OTOH, that wouldn't make much sense to me - I somewhat doubt that x86 can't run w/o that. Maybe ACPI selects it (haven't checked yet), but my driver is completely independent from ACPI (the board's BIOS doesn't provide any useful entries here), ACPI isn't x86 specific (anymore), and I'd guess x86 boards can run w/o ACPI. (actually, I'm considering hacking up a completely oftree based bootup on x86 ;-)). IMHO, dependencies should always be direct - indirect ones could suddenly change in subtle ways. > I think checkpatch will complain on that SPDX thing. > Copy something from one of the other drivers, it should be > on the first line of the file. Yeah, that's a really helpful tool. Maybe I should do some more automatisation on handling/editing whole patch queues ... (unless somebody else already did it). > Cut down the excessive newlines. Is there some standard rule on this ? Maybe something that checkpatch.pl could automatically catch ? >> +static void amd_fch_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc) >> +{ >> + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); >> + (void)priv; >> + >> + seq_printf(s, "debug info not implemented yet\n"); >> +} > > I think you can just skip implementing this then. Yep, forgot to clean that up. >> +static int amd_fch_gpio_request(struct gpio_chip *chip, unsigned gpio_pin) >> +{ >> + if (gpio_pin < chip->ngpio) >> + return 0; >> + >> + return -EINVAL; >> +} > > You can probably skip this too. The core already does this check. Okay, wasn't sure about that, so I preferred defensive programming. >> + priv->gc.owner = THIS_MODULE; >> + priv->gc.parent = &pdev->dev; >> + priv->gc.label = dev_name(&pdev->dev); >> + priv->gc.base = priv->pdata->gpio_base; > > No please, use priv->gc.base = -1; Could I also leave that field untouched (IOW: =0) ? >> +/* >> + * struct amd_fch_gpio_reg - GPIO register definition >> + * @reg: register index >> + * @name: signal name >> + */ >> +struct amd_fch_gpio_reg { >> + int reg; >> + const char* name; >> +}; > > Can't you put this in the driver file? See other mails. Need to pass in the board specific register assignments. >> +struct amd_fch_gpio_pdata { >> + struct resource res; >> + int gpio_num; >> + struct amd_fch_gpio_reg *gpio_reg; >> + int gpio_base; >> +}; > > Drop gpio_base. We don't hardcode the GPIO base anymore. Done. I had to patch gpio-keys-polled driver first. --mtx
On Thu, Feb 14, 2019 at 10:56 AM Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > On 08.02.19 15:25, Linus Walleij wrote: > > >> +config GPIO_AMD_FCH > >> + tristate "GPIO support for AMD Fusion Controller Hub (G-series SOCs)" > >> + select GPIO_GENERIC > > > > You are selecting GPIO_GENERIC, is this necessary? I thought > > X86 was already selecting this. > > Doesn't look so - at least haven't found anything where it's > automatically selected on x86. OTOH, that wouldn't make much > sense to me - I somewhat doubt that x86 can't run w/o that. Sorry my bad. You should select this if you use it, but are you using the GPIO MMIO abstractions? The hallmark of those implementations are that they call bgpio_init() and this driver does not, and it seems with the funny layout of the registers it can't even use it anyway. > IMHO, dependencies should always be direct - indirect ones could > suddenly change in subtle ways. Agreed. > >> + priv->gc.owner = THIS_MODULE; > >> + priv->gc.parent = &pdev->dev; > >> + priv->gc.label = dev_name(&pdev->dev); > >> + priv->gc.base = priv->pdata->gpio_base; > > > > No please, use priv->gc.base = -1; > > Could I also leave that field untouched (IOW: =0) ? No unfortunately not, because 0 is a valid GPIO base. Yours, Linus Walleij
diff --git a/MAINTAINERS b/MAINTAINERS index 8c68de3c..b9bc500 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -766,6 +766,13 @@ S: Supported F: Documentation/hwmon/fam15h_power F: drivers/hwmon/fam15h_power.c +AMD FCH GPIO DRIVER +M: Enrico Weigelt, metux IT consult <info@metux.net> +L: linux-gpio@vger.kernel.org +S: Maintained +F: drivers/gpio/gpio-amd-fch.c +F: include/linux/platform_data/x86/amd-fch-gpio-pdata.h + AMD GEODE CS5536 USB DEVICE CONTROLLER DRIVER L: linux-geode@lists.infradead.org (moderated for non-subscribers) S: Orphan diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index b5a2845..a3e47c8 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -654,6 +654,16 @@ config GPIO_LOONGSON1 help Say Y or M here to support GPIO on Loongson1 SoCs. +config GPIO_AMD_FCH + tristate "GPIO support for AMD Fusion Controller Hub (G-series SOCs)" + select GPIO_GENERIC + help + This option enables driver for GPIO on AMDs Fusion Controller Hub, + as found on G-series SOCs (eg. GX-412TC) + + Note: This driver doesn't registers itself automatically, as it + needs to be provided with platform specific configuration. + (See eg. CONFIG_PCENGINES_APU2.) endmenu menu "Port-mapped I/O GPIO drivers" diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 37628f8..bb48fd2 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o obj-$(CONFIG_GPIO_ALTERA_A10SR) += gpio-altera-a10sr.o +obj-$(CONFIG_GPIO_AMD_FCH) += gpio-amd-fch.o obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o obj-$(CONFIG_GPIO_AMDPT) += gpio-amdpt.o obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o diff --git a/drivers/gpio/gpio-amd-fch.c b/drivers/gpio/gpio-amd-fch.c new file mode 100644 index 0000000..356bb21 --- /dev/null +++ b/drivers/gpio/gpio-amd-fch.c @@ -0,0 +1,169 @@ +/* + * GPIO driver for the AMD G series FCH (eg. GX-412TC) + * + * Copyright (C) 2018 metux IT consult + * Author: Enrico Weigelt <info@metux.net> + * + * SPDX-License-Identifier: GPL+ + */ + +#include <linux/err.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/gpio/driver.h> +#include <linux/platform_data/x86/amd-fch-gpio-pdata.h> + + +#define GPIO_BIT_DIR 23 +#define GPIO_BIT_WRITE 22 +#define GPIO_BIT_READ 16 + + +struct amd_fch_gpio_priv { + struct platform_device *pdev; + struct gpio_chip gc; + void __iomem *base; + struct amd_fch_gpio_pdata *pdata; +}; + +static uint32_t *amd_fch_gpio_addr(struct gpio_chip *gc, unsigned gpio) +{ + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); + + if (gpio > priv->pdata->gpio_num) { + dev_err(&priv->pdev->dev, "gpio number %d out of range\n", gpio); + return NULL; + } + + return priv->base + priv->pdata->gpio_reg[gpio].reg*sizeof(u32); +} + +static int amd_fch_gpio_direction_input(struct gpio_chip *gc, unsigned offset) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, offset); + if (!ptr) return -EINVAL; + + *ptr &= ~(1 << GPIO_BIT_DIR); + return 0; +} + +static int amd_fch_gpio_direction_output(struct gpio_chip *gc, unsigned gpio, int value) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, gpio); + if (!ptr) return -EINVAL; + + *ptr |= (1 << GPIO_BIT_DIR); + return 0; +} + +static int amd_fch_gpio_get_direction(struct gpio_chip *gc, unsigned gpio) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, gpio); + if (!ptr) return -EINVAL; + + return (*ptr >> GPIO_BIT_DIR) & 1; +} + +static void amd_fch_gpio_set(struct gpio_chip *gc, unsigned gpio, int value) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, gpio); + if (!ptr) return; + + if (value) + *ptr |= (1 << GPIO_BIT_WRITE); + else + *ptr &= ~(1 << GPIO_BIT_WRITE); +} + +static int amd_fch_gpio_get(struct gpio_chip *gc, unsigned offset) +{ + volatile uint32_t *ptr = amd_fch_gpio_addr(gc, offset); + if (!ptr) return -EINVAL; + + return ((*ptr) >> GPIO_BIT_READ) & 1; +} + +static void amd_fch_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc) +{ + struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc); + (void)priv; + + seq_printf(s, "debug info not implemented yet\n"); +} + +static int amd_fch_gpio_request(struct gpio_chip *chip, unsigned gpio_pin) +{ + if (gpio_pin < chip->ngpio) + return 0; + + return -EINVAL; +} + +static int amd_fch_gpio_probe(struct platform_device *pdev) +{ + struct amd_fch_gpio_priv *priv; + struct amd_fch_gpio_pdata *pdata = pdev->dev.platform_data; + int err; + + if (!pdata) { + dev_err(&pdev->dev, "no platform_data\n"); + return -ENOENT; + } + + if (!(priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL))) { + dev_err(&pdev->dev, "failed to allocate priv struct\n"); + return -ENOMEM; + } + + priv->pdata = pdata; + priv->pdev = pdev; + + priv->gc.owner = THIS_MODULE; + priv->gc.parent = &pdev->dev; + priv->gc.label = dev_name(&pdev->dev); + priv->gc.base = priv->pdata->gpio_base; + priv->gc.ngpio = priv->pdata->gpio_num; + priv->gc.request = amd_fch_gpio_request; + priv->gc.direction_input = amd_fch_gpio_direction_input; + priv->gc.direction_output = amd_fch_gpio_direction_output; + priv->gc.get_direction = amd_fch_gpio_get_direction; + priv->gc.get = amd_fch_gpio_get; + priv->gc.set = amd_fch_gpio_set; + + spin_lock_init(&priv->gc.bgpio_lock); + + if (IS_ERR(priv->base = devm_ioremap_resource(&pdev->dev, &priv->pdata->res))) { + dev_err(&pdev->dev, "failed to map iomem\n"); + return -ENXIO; + } + + dev_info(&pdev->dev, "initializing on my own II\n"); + + if (IS_ENABLED(CONFIG_DEBUG_FS)) { + dev_info(&pdev->dev, "enabling debugfs\n"); + priv->gc.dbg_show = amd_fch_gpio_dbg_show; + } + + platform_set_drvdata(pdev, priv); + + err = devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv); + dev_info(&pdev->dev, "probe finished\n"); + return err; +} + +static struct platform_driver amd_fch_gpio_driver = { + .driver = { + .name = AMD_FCH_GPIO_DRIVER_NAME, + }, + .probe = amd_fch_gpio_probe, +}; + +module_platform_driver(amd_fch_gpio_driver); + +MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>"); +MODULE_DESCRIPTION("AMD G-series FCH GPIO driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:gpio_amd_fch"); diff --git a/include/linux/platform_data/x86/amd-fch-gpio-pdata.h b/include/linux/platform_data/x86/amd-fch-gpio-pdata.h new file mode 100644 index 0000000..68c1730 --- /dev/null +++ b/include/linux/platform_data/x86/amd-fch-gpio-pdata.h @@ -0,0 +1,41 @@ +/* + * AMD FCH gpio driver platform-data + * + * Copyright (C) 2018 metux IT consult + * Author: Enrico Weigelt <info@metux.net> + * + * SPDX-License-Identifier: GPL + */ + +#ifndef AMD_FCH_PDATA_H +#define AMD_FCH_PDATA_H + + +#include <linux/ioport.h> + +#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch" + +/* + * struct amd_fch_gpio_reg - GPIO register definition + * @reg: register index + * @name: signal name + */ +struct amd_fch_gpio_reg { + int reg; + const char* name; +}; + +/* + * struct amd_fch_gpio_pdata - GPIO chip platform data + * @resource: iomem range + * @gpio_reg: array of gpio registers + * @gpio_num: number of entries + */ +struct amd_fch_gpio_pdata { + struct resource res; + int gpio_num; + struct amd_fch_gpio_reg *gpio_reg; + int gpio_base; +}; + +#endif /* AMD_FCH_PDATA_H */