| Message ID | 20230621213115.113266-1-nick.hawkins@hpe.com |
|---|---|
| Headers | show |
| Series | ARM: Add GPIO support | expand |
Hi Nick, thanks for your patch! This is looking pretty good, I have some minor questions. On Wed, Jun 21, 2023 at 11:35 PM <nick.hawkins@hpe.com> wrote: > From: Nick Hawkins <nick.hawkins@hpe.com> > > The GXP SoC supports GPIO on multiple interfaces. The interfaces are > CPLD and Host. The gpio-gxp-pl driver covers the CPLD which takes > physical I/O from the board and shares it with GXP via a proprietary > interface that maps the I/O onto a specific register area of the GXP. > This driver supports interrupts from the CPLD. > > Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com> (...) > +enum pl_gpio_pn { > + IOP_LED1 = 0, > + IOP_LED2 = 1, > + IOP_LED3 = 2, > + IOP_LED4 = 3, (...) The confusing bit here is that GPIO means *generic purpose input/output* and these use cases are hardcoded into the driver and do not look very generic purpose at all. But I understand that it is convenient. I would add some comment saying that if there is a new version with a different layout of the pins, we need to make this kind of stuff go away and just use the numbers. > +static const struct gpio_chip template_chip = { > + .label = "gxp_gpio_plreg", > + .owner = THIS_MODULE, > + .get = gxp_gpio_pl_get, > + .set = gxp_gpio_pl_set, > + .get_direction = gxp_gpio_pl_get_direction, > + .direction_input = gxp_gpio_pl_direction_input, > + .direction_output = gxp_gpio_pl_direction_output, > + .base = -1, > +}; Neat! Since you so explicitly have assigned a meaning to each GPIO line, you can go ahead and assign the .names property as well. Check in the kernel tree for other drivers doing this. > + drvdata->chip = template_chip; > + drvdata->chip.ngpio = 80; If you're always assigning 80 to this you can just put that in the template as well. Other than that I think it looks good! Yours, Linus Walleij
On Wed, Jun 21, 2023 at 11:35 PM <nick.hawkins@hpe.com> wrote: > The gxp-fan-ctrl driver in HWMON no longer will report fan presence > or fan failure states as these GPIOs providing this information will be > consumed by the host. It will be the hosts function to keep track of > fan presence and status. I understand the approach such that you have also constructed a userspace cooling daemon that will consume the fan and GPIO information to drive the hardware monitoring and that is what you mean when you say "the host" will do it. This is a *bad idea*. While I can't stop you since these are indeed userspace interfaces we provide, I urge you to look into my earlier proposal to use a thermal zone to manage the cooling inside the kernel and get rid of all that custom userspace. The kernel has all that is needed to regulate the thermal zone with PID and on/off regulation. It will work even if the userspace crashes completely, which is what you want. The code is reviewed by a large community and very well tested. I think I showed this example before from arch/arm/boot/dts/gemini-dlink-dns-313.dts: thermal-zones { chassis-thermal { /* Poll every 20 seconds */ polling-delay = <20000>; /* Poll every 2nd second when cooling */ polling-delay-passive = <2000>; thermal-sensors = <&g751>; /* Tripping points from the fan.script in the rootfs */ trips { chassis_alert0: chassis-alert0 { /* At 43 degrees turn on low speed */ temperature = <43000>; hysteresis = <3000>; type = "active"; }; chassis_alert1: chassis-alert1 { /* At 47 degrees turn on high speed */ temperature = <47000>; hysteresis = <3000>; type = "active"; }; chassis_crit: chassis-crit { /* Just shut down at 60 degrees */ temperature = <60000>; hysteresis = <2000>; type = "critical"; }; }; cooling-maps { map0 { trip = <&chassis_alert0>; cooling-device = <&fan0 1 1>; }; map1 { trip = <&chassis_alert1>; cooling-device = <&fan0 2 2>; }; }; }; }; This uses a thermal sensor and a fan with two speeds. Adding a "presence" GPIO to the thermal zone core to enable and disable it which is what your use case needs should be pretty trivial. Yours, Linus Walleij
> I understand the approach such that you have also constructed a > userspace cooling daemon that will consume the fan and GPIO > information to drive the hardware monitoring and that is what you > mean when you say "the host" will do it. > This is a *bad idea*. > While I can't stop you since these are indeed userspace interfaces we > provide, I urge you to look into my earlier proposal to use a thermal > zone to manage the cooling inside the kernel and get rid of all that > custom userspace. > The kernel has all that is needed to regulate the thermal zone with > PID and on/off regulation. It will work even if the userspace crashes > completely, which is what you want. The code is reviewed by a large > community and very well tested. > I think I showed this example before from > arch/arm/boot/dts/gemini-dlink-dns-313.dts: > thermal-zones { > chassis-thermal { > /* Poll every 20 seconds */ > polling-delay = <20000>; > /* Poll every 2nd second when cooling */ > polling-delay-passive = <2000>; > thermal-sensors = <&g751>; > /* Tripping points from the fan.script in the rootfs */ > trips { > chassis_alert0: chassis-alert0 { > /* At 43 degrees turn on low speed */ > temperature = <43000>; > hysteresis = <3000>; > type = "active"; > }; > chassis_alert1: chassis-alert1 { > /* At 47 degrees turn on high speed */ > temperature = <47000>; > hysteresis = <3000>; > type = "active"; > }; > chassis_crit: chassis-crit { > /* Just shut down at 60 degrees */ > temperature = <60000>; > hysteresis = <2000>; > type = "critical"; > }; > }; > cooling-maps { > map0 { > trip = <&chassis_alert0>; > cooling-device = <&fan0 1 1>; > }; > map1 { > trip = <&chassis_alert1>; > cooling-device = <&fan0 2 2>; > }; > }; > }; > }; > This uses a thermal sensor and a fan with two speeds. > Adding a "presence" GPIO to the thermal zone core to enable and > disable it which is what your use case needs should be pretty trivial. Greetings Linus, As always thank you for your feedback and suggestions. Sorry for the delayed response. I will bring this concept to my team to discuss. A possible issue with this could be how our cooling profile varies based on options present such as extra DIMMS, CPU, storage, network ... etc. Thanks, -Nick Hawkins
On Mon, Jul 3, 2023 at 5:31 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote: > As always thank you for your feedback and suggestions. Sorry for the > delayed response. I will bring this concept to my team to discuss. Thanks! > A possible issue with this could be how our cooling profile varies > based on options present such as extra DIMMS, CPU, storage, > network ... etc. The thermal subsystem has plenty of tunables in sysfs, see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-thermal I don't think it's a problem to add more, if there are technical requirements for it. Yours, Linus Walleij
From: Nick Hawkins <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. After exploring the recommendation of using regmap_gpio it does not seem like a good fit. Some of the GPIOs are a combination of several bits in a byte where others are not contiguous blocks of GPIOs. The gxp-fan-ctrl driver in HWMON no longer will report fan presence or fan failure states as these GPIOs providing this information will be consumed by the host. It will be the hosts function to keep track of fan presence and status. --- Changes since v3: *Added called with debugfs to read server id *Added reviewed-by: tags to hwmon fan driver and fan yaml *Changed maxItems to be 4 instead of 6 on reg and reg-names in gpio yaml *Moved gpio-gxp-pl.c to be in a separate patch/commit. *Moved regmap_config out of function in both gpio drivers to turn into static const *Removed unnecesary variables and redundant conditionals *Modified regmap_read switch statements to calculate offset and mask then read at end *Removed use of -EOPNOTSUPP in favor of -ENOTSUPP *Removed redundant casting *Switched generic_handle_irq for generic_handle_domain_irq *Used GENMASK where applicable *Used bitmap_xor and for_each_bit_set in PL PSU interrupt *Made GPIO chip const and marked as a template (in the name) *Made irq_chip const and immutable *Corrected check on devm_gpiochip_add_data *Removed dev_err_probe on platform_get_irq *Changed return 0 to devm_request_irq Changes since v2: *Removed shared fan variables between HWMON and GPIO based on feedback *Removed reporting fan presence and failure from hwmon gxp-fan-ctrl driver *Removed GPIO dependency from gxp-fan-ctrl driver *Changed description and title for hpe,gxp-gpio binding *Corrected indention on example for hpe,gxp-gpio binding *Removed additional example from hpe,gxp-gpio binding 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): gpio: gxp: Add HPE GXP GPIO gpio: gxp: Add HPE GXP GPIO PL dt-bindings: hwmon: hpe,gxp-fan-ctrl: remove fn2 and pl registers hwmon: (gxp_fan_ctrl) Provide fan info via gpio MAINTAINERS: hpe: Add GPIO .../bindings/hwmon/hpe,gxp-fan-ctrl.yaml | 16 +- MAINTAINERS | 2 + drivers/gpio/Kconfig | 18 + drivers/gpio/Makefile | 2 + drivers/gpio/gpio-gxp-pl.c | 582 ++++++++++++++++++ drivers/gpio/gpio-gxp.c | 573 +++++++++++++++++ drivers/hwmon/Kconfig | 2 +- drivers/hwmon/gxp-fan-ctrl.c | 108 +--- 8 files changed, 1184 insertions(+), 119 deletions(-) create mode 100644 drivers/gpio/gpio-gxp-pl.c create mode 100644 drivers/gpio/gpio-gxp.c