| Message ID | AM5PR0701MB26576FCB30CDCA6F989C9B80E4420@AM5PR0701MB2657.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | GPIO driver and bindings for Altera FPGA Manager I/O | expand |
On Thu, Oct 19, 2017 at 6:29 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: Does this really work? > This is an internal 32-bit input and 32-bit output port to the FPGA logic. > > Instantiate this in the device tree as: > > gpio3: gpio@ff706010 { > #address-cells = <1>; > #size-cells = <0>; > compatible = "altr,fpgamgr-gpio"; > reg = <0xff706010 0x8>; > status = "okay"; > > portd: gpio-controller@0 { > compatible = "altr,fpgamgr-gpio-output"; > gpio-controller; > #gpio-cells = <2>; > reg = <0>; > }; > > porte: gpio-controller@1 { > compatible = "altr,fpgamgr-gpio-input"; > gpio-controller; > #gpio-cells = <2>; > reg = <1>; > }; So you have output-only and input-only ports.... > +static int fpgamgr_gpio_add_port(struct fpgamgr_gpio *gpio, > + struct fpgamgr_port_property *pp, > + unsigned int offs) > +{ > + struct fpgamgr_gpio_port *port; > + void __iomem *dat; > + int err; > + > + port = &gpio->ports[offs]; > + port->gpio = gpio; > + port->idx = pp->idx; > + > + dat = gpio->regs + (pp->idx * 4); > + > + err = bgpio_init(&port->bgc, gpio->dev, 4, dat, NULL, NULL, > + NULL, NULL, 0); But all you add is input-only GPIO chips. Also for the output ports. Yours, Linus Walleij -- 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
T24gMTAvMjAvMTcgMTA6MTAsIExpbnVzIFdhbGxlaWogd3JvdGU6DQo+IE9uIFRodSwgT2N0IDE5 LCAyMDE3IGF0IDY6MjkgUE0sIEJlcm5kIEVkbGluZ2VyDQo+IDxiZXJuZC5lZGxpbmdlckBob3Rt YWlsLmRlPiB3cm90ZToNCj4gDQo+IERvZXMgdGhpcyByZWFsbHkgd29yaz8NCj4gDQoNClllcywg aXQgZG9lcyByZWFsbHkgd29yay4NCg0KSSB1c2UgaXQgdG8gZ2F0ZSBhIGNsb2NrIHNpZ25hbCB1 c2luZyBmcGdhIGxvZ2ljDQppbnN0ZWFkIG9mIGFuIGV4dGVybmFsIGFuZCBnYXRlIG9uIHRoZSBQ Q0IuDQoNCkFuZCBhbm90aGVyIGRyaXZlciB1c2VzIHRoaXMgZ3BpbyB3aGljaA0KaXMgY29uZmln dXJlZCBhcyAic2VsZWN0LWdwaW8gPSA8JnBvcnRkIDIgMD47Ig0KaW4gdGhlIGRldmljZSB0cmVl LiAgVGhlIGFjdHVhbCBJL08gaXMgc2ltcGx5DQpjb250cm9sbGVkIHdpdGggZ3Bpb2Rfc2V0X3Zh bHVlKCksIHdoaWNoIHdvcmtzDQp3aXRob3V0IHRoZSBvdGhlciBkcml2ZXIgZXZlciBrbm93aW5n IGhvdyB0aGUNCkkvTyBpcyBjb25uZWN0ZWQuDQoNCkkgd2FzIGFsc28gYWJsZSB0byBleGVyY2lz ZSB0aGUgZGV2aWNlIHVubG9hZCBjb2RlIHBhdGgsDQp1c2luZyB5b3VyIGhpbnQgd2l0aCB0aGUg Z3BpbyB1bmJpbmQgZmlsZSBpbiB0aGUgc3lzZnMuDQoNCj4+IFRoaXMgaXMgYW4gaW50ZXJuYWwg MzItYml0IGlucHV0IGFuZCAzMi1iaXQgb3V0cHV0IHBvcnQgdG8gdGhlIEZQR0EgbG9naWMuDQo+ Pg0KPj4gSW5zdGFudGlhdGUgdGhpcyBpbiB0aGUgZGV2aWNlIHRyZWUgYXM6DQo+Pg0KPj4gICAg Z3BpbzM6IGdwaW9AZmY3MDYwMTAgew0KPj4gICAgICNhZGRyZXNzLWNlbGxzID0gPDE+Ow0KPj4g ICAgICNzaXplLWNlbGxzID0gPDA+Ow0KPj4gICAgIGNvbXBhdGlibGUgPSAiYWx0cixmcGdhbWdy LWdwaW8iOw0KPj4gICAgIHJlZyA9IDwweGZmNzA2MDEwIDB4OD47DQo+PiAgICAgc3RhdHVzID0g Im9rYXkiOw0KPj4NCj4+ICAgICBwb3J0ZDogZ3Bpby1jb250cm9sbGVyQDAgew0KPj4gICAgICBj b21wYXRpYmxlID0gImFsdHIsZnBnYW1nci1ncGlvLW91dHB1dCI7DQo+PiAgICAgIGdwaW8tY29u dHJvbGxlcjsNCj4+ICAgICAgI2dwaW8tY2VsbHMgPSA8Mj47DQo+PiAgICAgIHJlZyA9IDwwPjsN Cj4+ICAgICB9Ow0KPj4NCj4+ICAgICBwb3J0ZTogZ3Bpby1jb250cm9sbGVyQDEgew0KPj4gICAg ICBjb21wYXRpYmxlID0gImFsdHIsZnBnYW1nci1ncGlvLWlucHV0IjsNCj4+ICAgICAgZ3Bpby1j b250cm9sbGVyOw0KPj4gICAgICAjZ3Bpby1jZWxscyA9IDwyPjsNCj4+ICAgICAgcmVnID0gPDE+ Ow0KPj4gICAgIH07DQo+IA0KPiBTbyB5b3UgaGF2ZSBvdXRwdXQtb25seSBhbmQgaW5wdXQtb25s eSBwb3J0cy4uLi4NCj4gDQoNClllcy4NCg0KPj4gK3N0YXRpYyBpbnQgZnBnYW1ncl9ncGlvX2Fk ZF9wb3J0KHN0cnVjdCBmcGdhbWdyX2dwaW8gKmdwaW8sDQo+PiArICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICBzdHJ1Y3QgZnBnYW1ncl9wb3J0X3Byb3BlcnR5ICpwcCwNCj4+ICsgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgIHVuc2lnbmVkIGludCBvZmZzKQ0KPj4gK3sNCj4+ ICsgICAgICAgc3RydWN0IGZwZ2FtZ3JfZ3Bpb19wb3J0ICpwb3J0Ow0KPj4gKyAgICAgICB2b2lk IF9faW9tZW0gKmRhdDsNCj4+ICsgICAgICAgaW50IGVycjsNCj4+ICsNCj4+ICsgICAgICAgcG9y dCA9ICZncGlvLT5wb3J0c1tvZmZzXTsNCj4+ICsgICAgICAgcG9ydC0+Z3BpbyA9IGdwaW87DQo+ PiArICAgICAgIHBvcnQtPmlkeCA9IHBwLT5pZHg7DQo+PiArDQo+PiArICAgICAgIGRhdCA9IGdw aW8tPnJlZ3MgKyAocHAtPmlkeCAqIDQpOw0KPj4gKw0KPj4gKyAgICAgICBlcnIgPSBiZ3Bpb19p bml0KCZwb3J0LT5iZ2MsIGdwaW8tPmRldiwgNCwgZGF0LCBOVUxMLCBOVUxMLA0KPj4gKyAgICAg ICAgICAgICAgICAgICAgICAgIE5VTEwsIE5VTEwsIDApOw0KPiANCj4gQnV0IGFsbCB5b3UgYWRk IGlzIGlucHV0LW9ubHkgR1BJTyBjaGlwcy4gQWxzbyBmb3IgdGhlIG91dHB1dCBwb3J0cy4NCj4g DQoNCldlbGwgYWN0dWFsbHkgdGhpcyBhbGxvd3MgYm90aCBpbnB1dCBhbmQgb3V0cHV0LA0KYnV0 IHdoaWxlIHRoZSBpbnB1dCBwb3J0IGNvdWxkIGJlIHdyaXR0ZW4gYnkgYWNjaWRlbnQsDQp0aGUg aGFyZHdhcmUgZG9lcyB0aGUgcmlnaHQgdGhpbmcgYW5kIGlnbm9yZXMgdGhlIHdyaXRlIGN5Y2xl Lg0KDQpUaGUgb3RoZXIgcGFyYW1ldGVycyBvZiBiZ3Bpb19pbnB1dCBhcmUgZm9yIGRldmljZXMg d2l0aA0KZXhwbGljaXQgc2V0LSBhbmQgcmVzZXQtc2lnbmFscywgYW5kIGNvbmZpZ3VyYWJsZQ0K b3V0cHV0LWRpcmVjdGlvbiBldGMsIHdoaWNoIEkgZG9uJ3QgaGF2ZS4NCg0KDQoNCkJlcm5kLg0K -- 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
On Fri, Oct 20, 2017 at 4:40 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > On 10/20/17 10:10, Linus Walleij wrote: >>> + err = bgpio_init(&port->bgc, gpio->dev, 4, dat, NULL, NULL, >>> + NULL, NULL, 0); >> >> But all you add is input-only GPIO chips. Also for the output ports. >> > > Well actually this allows both input and output, > but while the input port could be written by accident, > the hardware does the right thing and ignores the write cycle. > > The other parameters of bgpio_input are for devices with > explicit set- and reset-signals, and configurable > output-direction etc, which I don't have. Ah! You're right. With just one register it becomes one of these that read and write into that register, simply. Sorry for my ignorance. I would prefer if you set the flag BGPIOF_NO_OUTPUT on the input only version though. Semantics may matter to the mmio gpio core. Yours, Linus Walleij -- 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
On 10/20/17 18:45, Linus Walleij wrote: > On Fri, Oct 20, 2017 at 4:40 PM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> On 10/20/17 10:10, Linus Walleij wrote: > >>>> + err = bgpio_init(&port->bgc, gpio->dev, 4, dat, NULL, NULL, >>>> + NULL, NULL, 0); >>> >>> But all you add is input-only GPIO chips. Also for the output ports. >>> >> >> Well actually this allows both input and output, >> but while the input port could be written by accident, >> the hardware does the right thing and ignores the write cycle. >> >> The other parameters of bgpio_input are for devices with >> explicit set- and reset-signals, and configurable >> output-direction etc, which I don't have. > > Ah! You're right. With just one register it becomes one of these that > read and write into that register, simply. Sorry for my ignorance. > > I would prefer if you set the flag BGPIOF_NO_OUTPUT on the input > only version though. Semantics may matter to the mmio gpio core. > Ah, thanks, that's a good suggestion. I will do that and send an updated patch. Bernd.
On Friday, October 20, 2017 5:48:11 PM CEST Bernd Edlinger wrote: > On 10/20/17 18:45, Linus Walleij wrote: > > On Fri, Oct 20, 2017 at 4:40 PM, Bernd Edlinger > > <bernd.edlinger@hotmail.de> wrote: > >> On 10/20/17 10:10, Linus Walleij wrote: > > > >>>> + err = bgpio_init(&port->bgc, gpio->dev, 4, dat, NULL, NULL, > >>>> + NULL, NULL, 0); > >>> > >>> But all you add is input-only GPIO chips. Also for the output ports. > >>> > >> > >> Well actually this allows both input and output, > >> but while the input port could be written by accident, > >> the hardware does the right thing and ignores the write cycle. > >> > >> The other parameters of bgpio_input are for devices with > >> explicit set- and reset-signals, and configurable > >> output-direction etc, which I don't have. > > > > Ah! You're right. With just one register it becomes one of these that > > read and write into that register, simply. Sorry for my ignorance. > > > > I would prefer if you set the flag BGPIOF_NO_OUTPUT on the input > > only version though. Semantics may matter to the mmio gpio core. > > > > Ah, thanks, that's a good suggestion. > > I will do that and send an updated patch. Wait. I had the same comment about this earlier and a few more suggestions to move the driver to gpio-mmio's dt. Didn't you read the mail? <https://marc.info/?l=linux-gpio&m=150678737724132&w=2> It made it to the list archive, so I'm pretty sure this got delivered to you as well. Regards, Christian -- 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
On 10/20/17 21:08, Christian Lamparter wrote: > Wait. > > I had the same comment about this earlier and a few more suggestions > to move the driver to gpio-mmio's dt. Didn't you read the mail? > > <https://marc.info/?l=linux-gpio&m=150678737724132&w=2> > > It made it to the list archive, so I'm pretty sure this got delivered > to you as well. > Hi, looks like I overlooked your message, sorry. > Hm, Is it possible to differenciate it from a "syscon" device? This is not a register region containing a set of miscellaneous registers, which are not cohesive enough to represent as any specific type of device. At least not how I understand the term. That would mean unrelated but hardwired functions that share the same register. The various functions would be fixed and documented in the manual. The difference is that there is no fixed function at all, like in the case of a I/O pin where the signal is routed on the PCB to some external device. Instead the signals are routed through the internal interconnect to logic functions on the FPGA. > The driver seemd fairly simple. So, You could actually get > away with just adding the "altr,fpgamgr-gpio" compatible string > to gpio-mmio.c's bgpio_of_match struct at [0] and change the dt > to something like this: This structure does not reflect the internal register structure of the FPGA Manager's I/O register block any more. I think this could of course be a completely generic driver, in that case I see no need to use a string like "altr,fpgamgr-gpio" at all, maybe just "generic-gpio". I would still see a value in having a specific driver for this specific hardware. But is this driver is too simple, to be acceptable? Bernd.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 3388d54..0bec903 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -97,6 +97,12 @@ config GPIO_ALTERA If driver is built as a module it will be called gpio-altera. +config GPIO_ALTERA_FPGAMGR + tristate "Altera FPGAMGR GPIO" + depends on OF_GPIO + help + Say yes here to support the Altera FPGAMGR GPIO device. + config GPIO_AMDPT tristate "AMD Promontory GPIO support" depends on ACPI diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index aeb70e9d..3eb73d4 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -26,6 +26,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_ALTERA_FPGAMGR) += gpio-altera-fpgamgr.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-altera-fpgamgr.c b/drivers/gpio/gpio-altera-fpgamgr.c new file mode 100644 index 0000000..76eff81 --- /dev/null +++ b/drivers/gpio/gpio-altera-fpgamgr.c @@ -0,0 +1,219 @@ +/* + * This is a GPIO driver for the internal FPGA Manager I/O ports + * connecting the HPS to the FPGA logic on certain Altera parts. + * + * Copyright (c) 2015 Softing Industrial Automation GmbH + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + */ +#include <linux/gpio/driver.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/ioport.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> +#include <linux/slab.h> + +struct fpgamgr_port_property { + struct device_node *node; + const char *name; + unsigned int idx; +}; + +struct fpgamgr_platform_data { + struct fpgamgr_port_property *properties; + unsigned int nports; +}; + +struct fpgamgr_gpio_port { + struct gpio_chip bgc; + struct fpgamgr_gpio *gpio; + unsigned int idx; +}; + +struct fpgamgr_gpio { + struct device *dev; + void __iomem *regs; + struct fpgamgr_gpio_port *ports; + unsigned int nr_ports; +}; + +static int fpgamgr_gpio_add_port(struct fpgamgr_gpio *gpio, + struct fpgamgr_port_property *pp, + unsigned int offs) +{ + struct fpgamgr_gpio_port *port; + void __iomem *dat; + int err; + + port = &gpio->ports[offs]; + port->gpio = gpio; + port->idx = pp->idx; + + dat = gpio->regs + (pp->idx * 4); + + err = bgpio_init(&port->bgc, gpio->dev, 4, dat, NULL, NULL, + NULL, NULL, 0); + if (err) { + dev_err(gpio->dev, "failed to init gpio chip for %s\n", + pp->name); + return err; + } + + port->bgc.of_node = pp->node; + + err = devm_gpiochip_add_data(gpio->dev, &port->bgc, NULL); + if (err) + dev_err(gpio->dev, "failed to register gpiochip for %s\n", + pp->name); + + return err; +} + +static struct fpgamgr_platform_data * +fpgamgr_gpio_get_pdata_of(struct device *dev) +{ + struct device_node *np = dev->of_node, *port_np; + struct fpgamgr_platform_data *pdata; + struct fpgamgr_port_property *pp; + int nports; + int i; + + if (!np) + return ERR_PTR(-ENODEV); + + nports = of_get_child_count(np); + if (nports == 0) + return ERR_PTR(-ENODEV); + + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return ERR_PTR(-ENOMEM); + + pdata->properties = kcalloc(nports, sizeof(*pp), GFP_KERNEL); + if (!pdata->properties) { + kfree(pdata); + return ERR_PTR(-ENOMEM); + } + + pdata->nports = nports; + + i = 0; + for_each_child_of_node(np, port_np) { + pp = &pdata->properties[i++]; + pp->node = port_np; + + if (of_property_read_u32(port_np, "reg", &pp->idx) || + pp->idx > 1) { + dev_err(dev, "missing/invalid port index for %s\n", + port_np->full_name); + kfree(pdata->properties); + kfree(pdata); + return ERR_PTR(-EINVAL); + } + + pp->name = port_np->full_name; + } + + return pdata; +} + +static inline void fpgamgr_free_pdata_of(struct fpgamgr_platform_data *pdata) +{ + if (!pdata) + return; + + kfree(pdata->properties); + kfree(pdata); +} + +static int fpgamgr_gpio_probe(struct platform_device *pdev) +{ + unsigned int i; + struct resource *res; + struct fpgamgr_gpio *fgpio; + int err; + struct device *dev = &pdev->dev; + struct fpgamgr_platform_data *pdata = dev_get_platdata(dev); + bool is_pdata_alloc = !pdata; + + if (is_pdata_alloc) { + pdata = fpgamgr_gpio_get_pdata_of(dev); + if (IS_ERR(pdata)) + return PTR_ERR(pdata); + } + + if (!pdata->nports) { + err = -ENODEV; + goto out_err; + } + + fgpio = devm_kzalloc(dev, sizeof(*fgpio), GFP_KERNEL); + if (!fgpio) { + err = -ENOMEM; + goto out_err; + } + fgpio->dev = dev; + fgpio->nr_ports = pdata->nports; + + fgpio->ports = devm_kcalloc(dev, fgpio->nr_ports, + sizeof(*fgpio->ports), GFP_KERNEL); + if (!fgpio->ports) { + err = -ENOMEM; + goto out_err; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + fgpio->regs = devm_ioremap_resource(dev, res); + if (IS_ERR(fgpio->regs)) { + err = PTR_ERR(fgpio->regs); + goto out_err; + } + + for (i = 0; i < fgpio->nr_ports; i++) { + err = fpgamgr_gpio_add_port(fgpio, &pdata->properties[i], i); + if (err) + goto out_unregister; + } + platform_set_drvdata(pdev, fgpio); + + goto out_err; + +out_unregister: + while (i > 0) + devm_gpiochip_remove(dev, &fgpio->ports[--i].bgc); + +out_err: + if (is_pdata_alloc) + fpgamgr_free_pdata_of(pdata); + + return err; +} + +static const struct of_device_id fpgamgr_of_match[] = { + { .compatible = "altr,fpgamgr-gpio" }, + { /* Sentinel */ } +}; +MODULE_DEVICE_TABLE(of, fpgamgr_of_match); + +static struct platform_driver fpgamgr_gpio_driver = { + .driver = { + .name = "gpio-altera-fpgamgr", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(fpgamgr_of_match), + }, + .probe = fpgamgr_gpio_probe, +}; + +module_platform_driver(fpgamgr_gpio_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Bernd Edlinger"); +MODULE_DESCRIPTION("Altera fpgamgr GPIO driver");
This is an internal 32-bit input and 32-bit output port to the FPGA logic. Instantiate this in the device tree as: gpio3: gpio@ff706010 { #address-cells = <1>; #size-cells = <0>; compatible = "altr,fpgamgr-gpio"; reg = <0xff706010 0x8>; status = "okay"; portd: gpio-controller@0 { compatible = "altr,fpgamgr-gpio-output"; gpio-controller; #gpio-cells = <2>; reg = <0>; }; porte: gpio-controller@1 { compatible = "altr,fpgamgr-gpio-input"; gpio-controller; #gpio-cells = <2>; reg = <1>; }; }; Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de> --- drivers/gpio/Kconfig | 6 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-altera-fpgamgr.c | 219 +++++++++++++++++++++++++++++++++++++ 3 files changed, 226 insertions(+) create mode 100644 drivers/gpio/gpio-altera-fpgamgr.c