| 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
On Thu, Jun 22, 2023 at 12:35 AM <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 GPIOs is a combination of both physical and virtual GPIOs are > I/O across the interfaces. The gpio-gxp driver specifically covers the > CSM(physical), FN2(virtual), and VUHC(virtual) which are the host. The "...are the host"?! hosts? > driver supports interrupts from the host. I will have some comments against the code, but I will try to review it probably later on (next week or so). Taking into account the approaching release date, I think we have a couple more months for polishing this series. -- With Best Regards, Andy Shevchenko
On Thu, Jun 22, 2023 at 12:35 AM <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 GPIOs is a combination of both physical and virtual are a > I/O across the interfaces. The gpio-gxp driver specifically covers the > CSM(physical), FN2(virtual), and VUHC(virtual) which are the host. The A bit of elaboration what the Host interface means and what is the difference to the CPLD. Perhaps spell it here as "Host interface", so it will be clear that above you mentioned it already. > driver supports interrupts from the host. ... > +#define GPIDAT 0x040 > +#define GPODAT 0x0b0 > +#define GPODAT2 0x0f8 > +#define GPOOWN 0x110 > +#define GPOOWN2 0x118 > +#define ASR_OFS 0x05c > +#define VUHC_OFS 0x064 Hmm... No GPIDAT2? I'm wondering if you can drop all these *2 definitions. Let see below... ... > +struct gxp_gpio_drvdata { > + struct gpio_chip chip; > + struct regmap *csm_map; > + void __iomem *fn2_vbtn; Looking into the code I have no clue why this is in this driver. You have regmaps and a separate resource for this. Why?! Is it in the window of GPIO MMIO? > + struct regmap *fn2_stat; > + struct regmap *vuhc0_map; > + int irq; > +}; ... > +/* > + * Note: Instead of definining all PINs here are the select few that defining (I have a déjà vu of already showing you typos in the commit message and comments and it looks like you ignored all of that. If so, then why?) > + * are specifically defined in DTS and offsets are used here. > + */ > +enum gxp_gpio_pn { > + RESET = 192, > + VPBTN = 210, /* aka POWER_OK */ > + PGOOD = 211, /* aka PS_PWROK */ > + PERST = 212, /* aka PCIERST */ > + POST_COMPLETE = 213, So, vbtn is a GPIO? Why does it need a special treatment? > +}; ... > +static int gxp_gpio_csm_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent); > + int ret = 0; Seems like a weird assignment. > + unsigned int reg_offset; > + unsigned int reg_mask; > + > + switch (offset) { > + case 0 ... 31: > + reg_offset = GPIDAT; > + reg_mask = BIT(offset); > + break; > + case 32 ... 63: > + reg_offset = GPIDAT + 0x20; > + reg_mask = BIT(offset - 32); > + break; So, here is the 0x20 offset shift... > + case 64 ... 95: > + reg_offset = GPODAT; > + reg_mask = BIT(offset - 64); > + break; > + case 96 ... 127: > + reg_offset = GPODAT + 0x04; > + reg_mask = BIT(offset - 96); > + break; ...and here (between two groups of GPO) is 0x48. Looks a bit weird. Does this GPIO have more functions than simply being a GPIO? To me looks like a PMIC-ish one. Is there any datasheet available? > + case 128 ... 159: > + reg_offset = GPODAT2; > + reg_mask = BIT(offset - 128); > + break; > + case 160 ... 191: > + reg_offset = GPODAT2 + 0x04; > + reg_mask = BIT(offset - 160); > + break; These (64-192) are two groups of the sequential bits in the address space. Why do you do this instead of the simplest calculus with bit and offset? > + case RESET: > + /* SW_RESET */ > + reg_offset = ASR_OFS; > + reg_mask = BIT(15); > + break; Does it really belong to this driver? Maybe it should be an MFD with GPIO and special functions with valid_mask properly assigned? > + default: > + break; > + } > + regmap_read(drvdata->csm_map, reg_offset, &ret); > + ret = (ret & reg_mask) ? 1 : 0; > + > + return ret; ret = regmap_read(, &value); if (ret) return ret; return !!(value & mask); > +} > + > +static void gxp_gpio_csm_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent); > + u32 tmp; > + switch (offset) { You definitely don't need this. All bits are in the sequential addresses. > + case 64 ... 95: > + /* Keep ownership setting */ > + regmap_read(drvdata->csm_map, GPOOWN, &tmp); > + tmp = (tmp & BIT(offset - 64)) ? 1 : 0; > + > + regmap_update_bits(drvdata->csm_map, GPOOWN, > + BIT(offset - 64), BIT(offset - 64)); > + regmap_update_bits(drvdata->csm_map, GPODAT, > + BIT(offset - 64), value ? BIT(offset - 64) : 0); > + > + /* Restore ownership setting */ > + regmap_update_bits(drvdata->csm_map, GPOOWN, > + BIT(offset - 64), tmp ? BIT(offset - 64) : 0); > + break; > + case 96 ... 127: > + /* Keep ownership setting */ > + regmap_read(drvdata->csm_map, GPOOWN + 0x04, &tmp); > + tmp = (tmp & BIT(offset - 96)) ? 1 : 0; > + > + regmap_update_bits(drvdata->csm_map, GPOOWN + 0x04, > + BIT(offset - 96), BIT(offset - 96)); > + regmap_update_bits(drvdata->csm_map, GPODAT + 0x04, > + BIT(offset - 96), value ? BIT(offset - 96) : 0); > + > + /* Restore ownership setting */ > + regmap_update_bits(drvdata->csm_map, GPOOWN + 0x04, > + BIT(offset - 96), tmp ? BIT(offset - 96) : 0); > + break; > + case 128 ... 159: > + /* Keep ownership setting */ > + regmap_read(drvdata->csm_map, GPOOWN2, &tmp); > + tmp = (tmp & BIT(offset - 128)) ? 1 : 0; > + > + regmap_update_bits(drvdata->csm_map, GPOOWN2, > + BIT(offset - 128), BIT(offset - 128)); > + regmap_update_bits(drvdata->csm_map, GPODAT2, > + BIT(offset - 128), value ? BIT(offset - 128) : 0); > + > + /* Restore ownership setting */ > + regmap_update_bits(drvdata->csm_map, GPOOWN2, > + BIT(offset - 128), tmp ? BIT(offset - 128) : 0); > + break; > + case 160 ... 191: > + /* Keep ownership setting */ > + regmap_read(drvdata->csm_map, GPOOWN2 + 0x04, &tmp); > + tmp = (tmp & BIT(offset - 160)) ? 1 : 0; > + > + regmap_update_bits(drvdata->csm_map, GPOOWN2 + 0x04, > + BIT(offset - 160), BIT(offset - 160)); > + regmap_update_bits(drvdata->csm_map, GPODAT2 + 0x04, > + BIT(offset - 160), value ? BIT(offset - 160) : 0); > + > + /* Restore ownership setting */ > + regmap_update_bits(drvdata->csm_map, GPOOWN2 + 0x04, > + BIT(offset - 160), tmp ? BIT(offset - 160) : 0); > + break; > + case 192: > + if (value) { > + regmap_update_bits(drvdata->csm_map, ASR_OFS, > + BIT(0), BIT(0)); > + regmap_update_bits(drvdata->csm_map, ASR_OFS, > + BIT(15), BIT(15)); > + } else { > + regmap_update_bits(drvdata->csm_map, ASR_OFS, > + BIT(15), 0); > + } > + break; Again, seems like a special function of GPIO that should probably have another driver that shares regmap with GPIO and GPIO marks this one is not valid for the GPIO operations. > + default: > + break; > + } > +} > + > +static int gxp_gpio_csm_get_direction(struct gpio_chip *chip, > + unsigned int offset) > +{ > + switch (offset) { Why do you use your custom definitions for the direction? We already have the generic ones for this. Please use them. > + case 0 ... 63: > + return GXP_GPIO_DIR_IN; > + case 64 ... 191: > + return GXP_GPIO_DIR_OUT; > + case 192 ... 193: > + return GXP_GPIO_DIR_OUT; > + case 194: > + return GXP_GPIO_DIR_IN; These are special cases. Not sure if it's for the GPIO, but basically you can check them separately and reduce switch-case to simple type = offset / 64; if (type) return OUT; return IN; Something similar to the rest of the functions. Note, that range operator in switch-case is non-standard. > + default: > + return -ENOTSUPP; > + } > +} ... > +static int gxp_gpio_vuhc_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent); > + unsigned int val; > + int ret = 0; > + > + if (offset < 8) { > + regmap_read(drvdata->vuhc0_map, VUHC_OFS + 4 * offset, &val); > + ret = (val & BIT(13)) ? 1 : 0; > + } > + > + return ret; > +} > + > +static void gxp_gpio_vuhc_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + /* Currently we are not supporting setting of these values yet */ > + switch (offset) { > + default: > + break; > + } > +} > + > +static int gxp_gpio_vuhc_get_direction(struct gpio_chip *chip, > + unsigned int offset) > +{ > + switch (offset) { > + case 0: > + case 1: > + case 2: > + return GXP_GPIO_DIR_IN; > + default: > + return -ENOTSUPP; > + } > +} > + > +static int gxp_gpio_vuhc_direction_input(struct gpio_chip *chip, > + unsigned int offset) > +{ > + switch (offset) { > + case 0: > + case 1: > + case 2: > + return 0; > + default: > + return -ENOTSUPP; > + } > +} > + > +static int gxp_gpio_vuhc_direction_output(struct gpio_chip *chip, > + unsigned int offset, int value) > +{ > + switch (offset) { > + default: > + return -ENOTSUPP; > + } > +} I'm not sure this belongs to the GPIO driver. > +static int gxp_gpio_fn2_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent); > + unsigned int val; > + int ret = 0; > + unsigned int reg_mask; > + > + switch (offset) { > + case PGOOD: > + regmap_read(drvdata->fn2_stat, 0, &val); > + reg_mask = BIT(24); > + > + break; > + case PERST: > + regmap_read(drvdata->fn2_stat, 0, &val); > + reg_mask = BIT(25); > + > + break; > + default: > + return -ENOTSUPP; > + } > + > + regmap_read(drvdata->fn2_stat, 0, &val); > + ret = (val & reg_mask); > + /* Return either 1 or 0 */ > + return ret ? 1 : 0; > +} > + > +static void gxp_gpio_fn2_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent); > + > + switch (offset) { > + case VPBTN: > + writeb(1, drvdata->fn2_vbtn); > + break; > + default: > + break; > + } > +} > + > +static int gxp_gpio_fn2_get_direction(struct gpio_chip *chip, > + unsigned int offset) > +{ > + switch (offset) { > + case VPBTN: > + return GXP_GPIO_DIR_OUT; > + default: > + return GXP_GPIO_DIR_IN; > + } > +} > + > +static int gxp_gpio_fn2_direction_input(struct gpio_chip *chip, > + unsigned int offset) > +{ > + switch (offset) { > + case PGOOD: > + case PERST: > + case POST_COMPLETE: > + return 0; > + default: > + return -ENOTSUPP; > + } > +} > + > +static int gxp_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + if (offset < 200) > + return gxp_gpio_csm_get(chip, offset); > + else if (offset >= 200 && offset < 210) > + return gxp_gpio_vuhc_get(chip, offset - 200); > + else if (offset >= 210) > + return gxp_gpio_fn2_get(chip, offset); > + > + return 0; > +} > + > +static void gxp_gpio_set(struct gpio_chip *chip, > + unsigned int offset, int value) > +{ > + if (offset < 200) > + gxp_gpio_csm_set(chip, offset, value); > + else if (offset >= 200 && offset < 210) > + gxp_gpio_vuhc_set(chip, offset - 200, value); > + else if (offset >= 210) > + gxp_gpio_fn2_set(chip, offset, value); > +} > + > +static int gxp_gpio_get_direction(struct gpio_chip *chip, > + unsigned int offset) > +{ > + if (offset < 200) > + return gxp_gpio_csm_get_direction(chip, offset); > + else if (offset >= 200 && offset < 210) > + return gxp_gpio_vuhc_get_direction(chip, offset - 200); > + else if (offset >= 210) > + return gxp_gpio_fn2_get_direction(chip, offset); > + > + return 0; > +} > + > +static int gxp_gpio_direction_input(struct gpio_chip *chip, > + unsigned int offset) > +{ > + if (offset < 200) > + return gxp_gpio_csm_direction_input(chip, offset); > + else if (offset >= 200 && offset < 210) > + return gxp_gpio_vuhc_direction_input(chip, offset - 200); > + else if (offset >= 210) > + return gxp_gpio_fn2_direction_input(chip, offset); > + > + return 0; > +} > + > +static int gxp_gpio_direction_output(struct gpio_chip *chip, > + unsigned int offset, int value) > +{ > + if (offset < 200) > + return gxp_gpio_csm_direction_output(chip, offset, value); > + else if (offset >= 200 && offset < 210) > + return gxp_gpio_vuhc_direction_output(chip, offset - 200, value); > + > + return 0; > +} ... > + /* Clear latched interrupt */ > + regmap_update_bits(drvdata->fn2_stat, 0, > + GENMASK(15, 0), GENMASK(15, 0)); unsigned int mask = GENMASK(...); unsigned int value = mask; regmap_update_bits(..., mask, value); ... > + regmap_update_bits(drvdata->fn2_stat, FN2_SEVMASK, > + BIT(0), set ? BIT(0) : 0); Ditto. unsigned int mask = BIT(0); unsigned int value = set ? mask : 0; ... So, overall it looks to me like an MFD device which should be split to GPIO, GPIO with IRQ (fh2), special cases and designated functionalities (somelike ~5 drivers all together). Without having a datasheet it's hard to say. -- With Best Regards, Andy Shevchenko
> defining (I have a déjà vu of already showing you typos in the commit > message and comments and it looks like you ignored all of that. If so, > then why?) Apologies Andy, I somehow completely missed / lost your mention of a typo in both the commit and comments. I have no intention of ignoring it. I will correct this. Thank you for the input you have provided. I have several questions and comments below. > > + * are specifically defined in DTS and offsets are used here. > > + */ > > +enum gxp_gpio_pn { > > + RESET = 192, > > + VPBTN = 210, /* aka POWER_OK */ > > + PGOOD = 211, /* aka PS_PWROK */ > > + PERST = 212, /* aka PCIERST */ > > + POST_COMPLETE = 213, > So, vbtn is a GPIO? Why does it need a special treatment? I was specifically grabbing the areas of memory that I needed instead of mapping the entire fn2 area of memory. I believe I can map the entire area instead. ... > > + case 64 ... 95: > > + reg_offset = GPODAT; > > + reg_mask = BIT(offset - 64); > > + break; > > + case 96 ... 127: > > + reg_offset = GPODAT + 0x04; > > + reg_mask = BIT(offset - 96); > > + break; > ...and here (between two groups of GPO) is 0x48. Looks a bit weird. > Does this GPIO have more functions than simply being a GPIO? To me > looks like a PMIC-ish one. Is there any datasheet available? Unfortunately, there is no public datasheet available currently. There are however some special functions others than being a simple GPIO. There are ownership bits as the same area is accessible VIA PCI. > > + case RESET: > > + /* SW_RESET */ > > + reg_offset = ASR_OFS; > > + reg_mask = BIT(15); > > + break; > Does it really belong to this driver? Maybe it should be an MFD with > GPIO and special functions with valid_mask properly assigned? Unlike your suggestion I quote directly below are you implying that My accesses to the CSM area of memory can be its own separate driver that is MFD and provides GPIO lines to read? > ... > > + case 192: > > + if (value) { > > + regmap_update_bits(drvdata->csm_map, ASR_OFS, > > + BIT(0), BIT(0)); > > + regmap_update_bits(drvdata->csm_map, ASR_OFS, > > + BIT(15), BIT(15)); > > + } else { > > + regmap_update_bits(drvdata->csm_map, ASR_OFS, > > + BIT(15), 0); > > + } > > + break; > Again, seems like a special function of GPIO that should probably have > another driver that shares regmap with GPIO and GPIO marks this one is > not valid for the GPIO operations. What do you mean by GPIO marking this one as not valid for GPIO operations? ... > > +static int gxp_gpio_vuhc_get(struct gpio_chip *chip, unsigned int offset) > > +{ > > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent); > > + unsigned int val; > > + int ret = 0; > > + > > + if (offset < 8) { > > + regmap_read(drvdata->vuhc0_map, VUHC_OFS + 4 * offset, &val); > > + ret = (val & BIT(13)) ? 1 : 0; > > + } > > + > > + return ret; > > +} > > + > > +static void gxp_gpio_vuhc_set(struct gpio_chip *chip, unsigned int offset, > > + int value) > > +{ > > + /* Currently we are not supporting setting of these values yet */ > > + switch (offset) { > > + default: > > + break; > > + } > > +} > > + > > +static int gxp_gpio_vuhc_get_direction(struct gpio_chip *chip, > > + unsigned int offset) > > +{ > > + switch (offset) { > > + case 0: > > + case 1: > > + case 2: > > + return GXP_GPIO_DIR_IN; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int gxp_gpio_vuhc_direction_input(struct gpio_chip *chip, > > + unsigned int offset) > > +{ > > + switch (offset) { > > + case 0: > > + case 1: > > + case 2: > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int gxp_gpio_vuhc_direction_output(struct gpio_chip *chip, > > + unsigned int offset, int value) > > +{ > > + switch (offset) { > > + default: > > + return -ENOTSUPP; > > + } > > +} > I'm not sure this belongs to the GPIO driver. By this do you mean that it needs to be in a separate non GPIO driver that shares a regmap as suggested above? ... > So, overall it looks to me like an MFD device which should be split to > GPIO, GPIO with IRQ (fh2), special cases and designated > functionalities (somelike ~5 drivers all together). Without having a > datasheet it's hard to say. Yes that sounds like a good plan to me I will see what I can work up. I might end up removing thils file entirely and just sticking with gpio-gxp-pl.c As for the gpio-gxp-pl.c are you okay with it? Thank you for the assistance and review, -Nick Hawkins
> 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