| Message ID | 20230531151918.105223-1-nick.hawkins@hpe.com |
|---|---|
| Headers | show |
| Series | ARM: Add GPIO support | expand |
On 31/05/2023 17:19, nick.hawkins@hpe.com wrote: > From: Nick Hawkins <nick.hawkins@hpe.com> > > Provide access to the register regions and interrupt for GPIO. There > will be two drivers available. The first driver under the hpe,gxp-gpio > binding will provide GPIO information for the VUHC, CSM, and FN2 > host interfaces. The second driver under the hpe,gxp-gpio-pl will > provide GPIO information from the CPLD interface. The main difference > and need for two separate bindings is they have different interrupt > parents. The other is hpe,gxp-gpio is a combination of physical > and virtual GPIOs where as hpe,gxp-gpio-pl are all physical > GPIOs from the CPLD. > > Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com> > > --- > > v2: > *Put binding patch before the driver in the series > *Improved patch description > *Removed oneOf and items in compatible definition > *Moved additionalProperties definition to correct spot in file > *Fixed indentation on example I don't think it was fixed. > *Improved description in .yaml > --- > .../bindings/gpio/hpe,gxp-gpio.yaml | 190 ++++++++++++++++++ > 1 file changed, 190 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml > > diff --git a/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml > new file mode 100644 > index 000000000000..b92b7d72d39b > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml > @@ -0,0 +1,190 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/gpio/hpe,gxp-gpio.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: HPE GXP gpio controllers > + > +maintainers: > + - Nick Hawkins <nick.hawkins@hpe.com> > + > +description: > + Interruptable GPIO drivers for the HPE GXP that covers multiple interfaces > + of both physical and virtual GPIO pins. > + > +properties: > + compatible: > + enum: > + - hpe,gxp-gpio > + - hpe,gxp-gpio-pl > + > + reg: > + minItems: 2 > + maxItems: 6 > + > + reg-names: > + minItems: 2 > + maxItems: 6 > + > + gpio-controller: true > + > + "#gpio-cells": > + const: 2 > + > + gpio-line-names: > + minItems: 80 > + maxItems: 300 > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - reg-names > + - gpio-controller > + - "#gpio-cells" > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + enum: > + - hpe,gxp-gpio > + then: > + properties: > + reg: > + items: > + - description: CSM GPIO interface > + - description: fn2 virtual button GPIO > + - description: fn2 system status GPIO > + - description: vuhc GPIO status interface > + reg-names: > + items: > + - const: csm > + - const: fn2-vbtn > + - const: fn2-stat > + - const: vuhc > + - if: > + properties: > + compatible: > + contains: > + enum: > + - hpe,gxp-gpio-pl > + then: > + properties: > + reg: > + items: > + - description: Programmable logic device GPIO > + - description: Programmable logic device interrupt GPIO > + reg-names: > + items: > + - const: base > + - const: interrupt > + > +additionalProperties: false > + > +examples: > + - | > + gpio@0 { > + compatible = "hpe,gxp-gpio"; > + reg = <0x0 0x400>, <0x200046 0x1>, <0x200070 0x08>, <0x400064 0x80>; > + reg-names = "csm", "fn2-vbtn", "fn2-stat", "vuhc"; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-parent = <&vic0>; > + interrupts = <10>; > + gpio-line-names = "IOP_LED1", "IOP_LED2", > + "IOP_LED3", "IOP_LED4", Broken indentation. This is aligned with opening " in previous line. > + "IOP_LED5", "IOP_LED6", > + "IOP_LED7", "IOP_LED8", > + "FAN1_INST", "FAN2_INST", > + "FAN3_INST", "FAN4_INST", > + "FAN5_INST", "FAN6_INST", > + "FAN7_INST", "FAN8_INST", > + "FAN1_FAIL", "FAN2_FAIL", > + "FAN3_FAIL", "FAN4_FAIL", > + "FAN5_FAIL", "FAN6_FAIL", > + "FAN7_FAIL", "FAN8_FAIL", > + "FAN1_ID", "FAN2_ID", > + "FAN3_ID", "FAN4_ID", > + "FAN5_ID", "FAN6_ID", > + "FAN7_ID", "FAN8_ID", > + "IDENTIFY", "HEALTH_RED", > + "HEALTH_AMBER", "POWER_BUTTON", > + "UID_PRESS", "SLP", > + "NMI_BUTTON", "RESET_BUTTON", > + "SIO_S5", "SO_ON_CONTROL", > + "PSU1_INST", "PSU2_INST", > + "PSU3_INST", "PSU4_INST", > + "PSU5_INST", "PSU6_INST", > + "PSU7_INST", "PSU8_INST", > + "PSU1_AC", "PSU2_AC", > + "PSU3_AC", "PSU4_AC", > + "PSU5_AC", "PSU6_AC", > + "PSU7_AC", "PSU8_AC", > + "PSU1_DC", "PSU2_DC", > + "PSU3_DC", "PSU4_DC", > + "PSU5_DC", "PSU6_DC", > + "PSU7_DC", "PSU8_DC", > + "", "", > + "", "", > + "", "", > + "", "", > + "", "", > + "", "", > + "", ""; > + }; > + > + - | > + gpio@51000300 { > + compatible = "hpe,gxp-gpio-pl"; > + reg = <0x51000300 0x40>, <0x51000380 0x10>; > + reg-names = "base", "interrupt"; One example is enough, because this almost does not differ from previous. Best regards, Krzysztof
> If the host wants to own the fan status from gpio pins, it has to live up to > it and own it entirely. The kernel hwmon driver does not have access in that > case. > In a more "normal" world, the hwmon driver would "own" the gpio pin(s) > and user space would listen to associated hwmon attribute events (presumably > fan_enable and fan_fault), either by listening for sysfs attribute events > or via udev or both. Again, if you don't want to do that, and want user space > to have access to the raw gpio pins, you'll have to live with the consequences. > I don't see the need to bypass existing mechanisms just because user space > programmers want direct access to gpio pins. Greetings Guenter, Thank you for your valuable feedback with the solutions you have provided. Before I proceed though I have a quick query about the fan driver. If I were to let the user space "own" gpio pins, would it be permissible for the userspace to feed a kernel driver data via sysfs? Ex: GPIO Driver -> (OpenBMC) -> Fandriver (sysfs). Here the GPIO driver would provide fan presence information to OpenBMC and then OpenBMC would provide fan presence info to the fan driver. If it were permissible to provide data to the driver via this method I could apply it to the PSU driver as well. the PSU driver which requires presence info to verify a PSU is inserted / removed. Thanks, -Nick Hawkins
On Thu, Jun 1, 2023 at 5:48 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote: > Thank you for your valuable feedback with the solutions you have provided. > Before I proceed though I have a quick query about the fan driver. > If I were to let the user space "own" gpio pins, would it be permissible for > the userspace to feed a kernel driver data via sysfs? > > Ex: > GPIO Driver -> (OpenBMC) -> Fandriver (sysfs). > > Here the GPIO driver would provide fan presence information to OpenBMC > and then OpenBMC would provide fan presence info to the fan driver. But why? Don't be so obsessed about userspace doing stuff using sysfs, usually it is a better idea to let the kernel handle hardware. I think this is a simple thermal zone you can define in the device tree as indicated in my previous comment. > If it were permissible to provide data to the driver via this method I could > apply it to the PSU driver as well. the PSU driver which requires presence > info to verify a PSU is inserted / removed. It feels like you are looking for a way for two drivers to communicate with each other. This can be done several ways, the most straight-forward is notifiers. include/linux/notifier.h Yours, Linus Walleij
On 6/1/23 10:11, Linus Walleij wrote: > On Thu, Jun 1, 2023 at 5:48 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote: > >> Thank you for your valuable feedback with the solutions you have provided. >> Before I proceed though I have a quick query about the fan driver. >> If I were to let the user space "own" gpio pins, would it be permissible for >> the userspace to feed a kernel driver data via sysfs? >> >> Ex: >> GPIO Driver -> (OpenBMC) -> Fandriver (sysfs). >> >> Here the GPIO driver would provide fan presence information to OpenBMC >> and then OpenBMC would provide fan presence info to the fan driver. > > But why? Don't be so obsessed about userspace doing stuff using > sysfs, usually it is a better idea to let the kernel handle hardware. > > I think this is a simple thermal zone you can define in the device > tree as indicated in my previous comment. > >> If it were permissible to provide data to the driver via this method I could >> apply it to the PSU driver as well. the PSU driver which requires presence >> info to verify a PSU is inserted / removed. > > It feels like you are looking for a way for two drivers to communicate > with each other. > > This can be done several ways, the most straight-forward is notifiers. > include/linux/notifier.h > This is all unnecessary. The hwmon driver could register a gpio pin, including interrupt, and then report state changes to userspace with sysfs or udev events on the registered hwmon sysfs attributes. If they really want to use userspace for everything, they should just use userspace for everything and not bother with a kernel driver. Guenter
> > > > This can be done several ways, the most straight-forward is notifiers. > > include/linux/notifier.h > > > This is all unnecessary. The hwmon driver could register a gpio pin, > including interrupt, and then report state changes to userspace with > sysfs or udev events on the registered hwmon sysfs attributes. > If they really want to use userspace for everything, they should > just use userspace for everything and not bother with a kernel driver. Greetings Guenter and Linus, Thank you for your feedback and assistance. I discussed this with my team and the direction they are leaning is that they want to own the GPIOs in user space. The fan driver it would still need to be used to set and read PWMs as they are kernel protected registers. It will also need to be there to coordinate the proper offset in the GXP registers to control a particular fans PWM. For hot pluggable devices such as fans and psu they will need to bind/unbind the hwmon driver of the device as it is inserted/removed. Is this an acceptable path forward? If it is I will revise this patchset once more to make the fan independent of the GPIO driver. Thanks again for all the guidance, -Nick Hawkins
From: Nick Hawkins <nick.hawkins@hpe.com> Note: The previous version of this patchset was titled "Add GPIO and PSU support". Based on feedback and in an effort to reduce size PSU has been removed. Link: https://lore.kernel.org/all/20230418152824.110823-1-nick.hawkins@hpe.com/ The GXP SoC supports GPIO on multiple interfaces. The interfaces are CPLD and Host. The GPIOs is a combination of both physical and virtual I/O across the interfaces. The gpio-gxp driver specifically covers the CSM(physical), FN2(virtual), and VUHC(virtual) which are the host. The gpio-gxp-pl driver covers the CPLD which takes physical I/O from the board and shares it with GXP via a propriety interface that maps the I/O onto a specific register area of the GXP. The drivers both support interrupts but from different interrupt parents. There is a need for both the host OpenBMC and the gxp-fan-ctrl driver to access the same GPIO information from the CPLD. The OpenBMC stack is reacting to changes in GPIOs and taking action. This requires it to hold the GPIO which creates a problem where both the host and linux cannot have the same GPIO. Thus an attempt to remedy this was to add a shared variable between the GPIO driver and the fan control driver to provide fan presence and failure information. This is why hwmon has been included in this patchset. --- Changes since v1: *Removed ARM device tree changes and defconfig changes to reduce patchset size *Removed GXP PSU changes to reduce patchset size *Corrected hpe,gxp-gpio YAML file based on feedback *Created new gpio-gxp-pl file to reduce complexity *Separated code into two files to keep size down: gpio-gxp.c and gpio-gxp-pl.c *Fixed Kconfig indentation as well as add new entry for gpio-gxp-pl *Removed use of linux/of.h and linux/of_device.h *Added mod_devicetable.h and property.h *Fixed indentation of defines and uses consistent number of digits *Corrected defines with improper GPIO_ namespace. *For masks now use BIT() *Added comment for PLREG offsets *Move gpio_chip to be first in structure *Calculate offset for high and low byte GPIO reads instead of having H(High) and L(Low) letters added to the variables. *Removed repeditive use of "? 1 : 0" *Switched to handle_bad_irq() *Removed improper bailout on gpiochip_add_data *Used GENMASK to arm interrupts *Removed use of of_match_device *fixed sizeof in devm_kzalloc *Added COMPILE_TEST to Kconfig *Added dev_err_probe where applicable *Removed unecessary parent and compatible checks Nick Hawkins (5): dt-bindings: gpio: Add HPE GXP GPIO gpio: gxp: Add HPE GXP GPIO dt-bindings: hwmon: hpe,gxp-fanctrl: remove fn2 and pl regs hwmon: (gxp_fan_ctrl) Provide fan info via gpio MAINTAINERS: hpe: Add GPIO .../bindings/gpio/hpe,gxp-gpio.yaml | 190 ++++++ .../bindings/hwmon/hpe,gxp-fan-ctrl.yaml | 16 +- MAINTAINERS | 2 + drivers/gpio/Kconfig | 18 + drivers/gpio/Makefile | 2 + drivers/gpio/gpio-gxp-pl.c | 536 +++++++++++++++ drivers/gpio/gpio-gxp.c | 637 ++++++++++++++++++ drivers/hwmon/Kconfig | 2 +- drivers/hwmon/gxp-fan-ctrl.c | 61 +- drivers/hwmon/gxp-gpio.h | 13 + 10 files changed, 1409 insertions(+), 68 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml create mode 100644 drivers/gpio/gpio-gxp-pl.c create mode 100644 drivers/gpio/gpio-gxp.c create mode 100644 drivers/hwmon/gxp-gpio.h