| Message ID | 20230531151918.105223-5-nick.hawkins@hpe.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: Add GPIO support | expand |
On 5/31/23 08:19, nick.hawkins@hpe.com wrote: > From: Nick Hawkins <nick.hawkins@hpe.com> > > The fan driver now receives fan data from GPIO via a shared variable. No, it is not necessary. The driver can and should get the GPIO data using the gpio API. > This is a necessity as the Host and the driver need access to the same > information. Part of the changes includes removing a system power check > as the GPIO driver needs it to report power state to host. > > Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com> > > --- > > v2: > *Removed use of shared functions to GPIO in favor of a shared variable > *Added build dependency on GXP GPIO driver. > --- > drivers/hwmon/Kconfig | 2 +- > drivers/hwmon/gxp-fan-ctrl.c | 61 +++++------------------------------- > drivers/hwmon/gxp-gpio.h | 13 ++++++++ > 3 files changed, 21 insertions(+), 55 deletions(-) > create mode 100644 drivers/hwmon/gxp-gpio.h > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 5b3b76477b0e..5c606bea31f9 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -716,7 +716,7 @@ config SENSORS_GPIO_FAN > > config SENSORS_GXP_FAN_CTRL > tristate "HPE GXP fan controller" > - depends on ARCH_HPE_GXP || COMPILE_TEST > + depends on (ARCH_HPE_GXP && GPIO_GXP_PL) || COMPILE_TEST Compile test will fail badly unless those external variables are declared. > help > If you say yes here you get support for GXP fan control functionality. > > diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c > index 0014b8b0fd41..8555231328d7 100644 > --- a/drivers/hwmon/gxp-fan-ctrl.c > +++ b/drivers/hwmon/gxp-fan-ctrl.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0-only > -/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */ > +/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */ > > #include <linux/bits.h> > #include <linux/err.h> > @@ -8,51 +8,28 @@ > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > +#include "gxp-gpio.h" > > #define OFS_FAN_INST 0 /* Is 0 because plreg base will be set at INST */ > #define OFS_FAN_FAIL 2 /* Is 2 bytes after base */ > -#define OFS_SEVSTAT 0 /* Is 0 because fn2 base will be set at SEVSTAT */ > -#define POWER_BIT 24 > > struct gxp_fan_ctrl_drvdata { > - void __iomem *base; > - void __iomem *plreg; > - void __iomem *fn2; > + void __iomem *base; > }; > > static bool fan_installed(struct device *dev, int fan) > { > - struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev); > - u8 val; > - > - val = readb(drvdata->plreg + OFS_FAN_INST); > - > - return !!(val & BIT(fan)); > + return !!(fan_presence & BIT(fan)); > } > > static long fan_failed(struct device *dev, int fan) > { > - struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev); > - u8 val; > - > - val = readb(drvdata->plreg + OFS_FAN_FAIL); > - > - return !!(val & BIT(fan)); > + return !!(fan_fail & BIT(fan)); > } > > static long fan_enabled(struct device *dev, int fan) > { > - struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev); > - u32 val; > - > - /* > - * Check the power status as if the platform is off the value > - * reported for the PWM will be incorrect. Report fan as > - * disabled. > - */ > - val = readl(drvdata->fn2 + OFS_SEVSTAT); > - > - return !!((val & BIT(POWER_BIT)) && fan_installed(dev, fan)); > + return !!(fan_installed(dev, fan)); Unnecessary () around function call. > } > > static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val) > @@ -98,20 +75,8 @@ static int gxp_fan_read(struct device *dev, u32 attr, int channel, long *val) > static int gxp_pwm_read(struct device *dev, u32 attr, int channel, long *val) > { > struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev); > - u32 reg; > > - /* > - * Check the power status of the platform. If the platform is off > - * the value reported for the PWM will be incorrect. In this case > - * report a PWM of zero. > - */ > - > - reg = readl(drvdata->fn2 + OFS_SEVSTAT); > - > - if (reg & BIT(POWER_BIT)) > - *val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0; > - else > - *val = 0; > + *val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0; > > return 0; > } > @@ -212,18 +177,6 @@ static int gxp_fan_ctrl_probe(struct platform_device *pdev) > return dev_err_probe(dev, PTR_ERR(drvdata->base), > "failed to map base\n"); > > - drvdata->plreg = devm_platform_ioremap_resource_byname(pdev, > - "pl"); > - if (IS_ERR(drvdata->plreg)) > - return dev_err_probe(dev, PTR_ERR(drvdata->plreg), > - "failed to map plreg\n"); > - > - drvdata->fn2 = devm_platform_ioremap_resource_byname(pdev, > - "fn2"); > - if (IS_ERR(drvdata->fn2)) > - return dev_err_probe(dev, PTR_ERR(drvdata->fn2), > - "failed to map fn2\n"); > - > hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > "hpe_gxp_fan_ctrl", > drvdata, > diff --git a/drivers/hwmon/gxp-gpio.h b/drivers/hwmon/gxp-gpio.h > new file mode 100644 > index 000000000000..88abe60bbe83 > --- /dev/null > +++ b/drivers/hwmon/gxp-gpio.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */ > + > +#ifndef __GPIO_GXP_H__ > +#define __GPIO_GXP_H__ > + > +#include <linux/err.h> > + > +/* Data provided by GPIO */ > +extern u8 fan_presence; > +extern u8 fan_fail; > + This is not acceptable. It is way too generic for a global variable, and it does not use the gpio API. Besides, the variables would have to be declared in an include file associated with the code introducing them. If you want to use gpio pins in the hwmon driver, use the gpio API ([devm_]gpiod_get() and associated functions). There are lots of examples in the kernel showing how to do that. Guenter > +#endif /* __GPIO_GXP_H__ */
> 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
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 5b3b76477b0e..5c606bea31f9 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -716,7 +716,7 @@ config SENSORS_GPIO_FAN config SENSORS_GXP_FAN_CTRL tristate "HPE GXP fan controller" - depends on ARCH_HPE_GXP || COMPILE_TEST + depends on (ARCH_HPE_GXP && GPIO_GXP_PL) || COMPILE_TEST help If you say yes here you get support for GXP fan control functionality. diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c index 0014b8b0fd41..8555231328d7 100644 --- a/drivers/hwmon/gxp-fan-ctrl.c +++ b/drivers/hwmon/gxp-fan-ctrl.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-only -/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */ +/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */ #include <linux/bits.h> #include <linux/err.h> @@ -8,51 +8,28 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/platform_device.h> +#include "gxp-gpio.h" #define OFS_FAN_INST 0 /* Is 0 because plreg base will be set at INST */ #define OFS_FAN_FAIL 2 /* Is 2 bytes after base */ -#define OFS_SEVSTAT 0 /* Is 0 because fn2 base will be set at SEVSTAT */ -#define POWER_BIT 24 struct gxp_fan_ctrl_drvdata { - void __iomem *base; - void __iomem *plreg; - void __iomem *fn2; + void __iomem *base; }; static bool fan_installed(struct device *dev, int fan) { - struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev); - u8 val; - - val = readb(drvdata->plreg + OFS_FAN_INST); - - return !!(val & BIT(fan)); + return !!(fan_presence & BIT(fan)); } static long fan_failed(struct device *dev, int fan) { - struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev); - u8 val; - - val = readb(drvdata->plreg + OFS_FAN_FAIL); - - return !!(val & BIT(fan)); + return !!(fan_fail & BIT(fan)); } static long fan_enabled(struct device *dev, int fan) { - struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev); - u32 val; - - /* - * Check the power status as if the platform is off the value - * reported for the PWM will be incorrect. Report fan as - * disabled. - */ - val = readl(drvdata->fn2 + OFS_SEVSTAT); - - return !!((val & BIT(POWER_BIT)) && fan_installed(dev, fan)); + return !!(fan_installed(dev, fan)); } static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val) @@ -98,20 +75,8 @@ static int gxp_fan_read(struct device *dev, u32 attr, int channel, long *val) static int gxp_pwm_read(struct device *dev, u32 attr, int channel, long *val) { struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev); - u32 reg; - /* - * Check the power status of the platform. If the platform is off - * the value reported for the PWM will be incorrect. In this case - * report a PWM of zero. - */ - - reg = readl(drvdata->fn2 + OFS_SEVSTAT); - - if (reg & BIT(POWER_BIT)) - *val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0; - else - *val = 0; + *val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0; return 0; } @@ -212,18 +177,6 @@ static int gxp_fan_ctrl_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(drvdata->base), "failed to map base\n"); - drvdata->plreg = devm_platform_ioremap_resource_byname(pdev, - "pl"); - if (IS_ERR(drvdata->plreg)) - return dev_err_probe(dev, PTR_ERR(drvdata->plreg), - "failed to map plreg\n"); - - drvdata->fn2 = devm_platform_ioremap_resource_byname(pdev, - "fn2"); - if (IS_ERR(drvdata->fn2)) - return dev_err_probe(dev, PTR_ERR(drvdata->fn2), - "failed to map fn2\n"); - hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "hpe_gxp_fan_ctrl", drvdata, diff --git a/drivers/hwmon/gxp-gpio.h b/drivers/hwmon/gxp-gpio.h new file mode 100644 index 000000000000..88abe60bbe83 --- /dev/null +++ b/drivers/hwmon/gxp-gpio.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */ + +#ifndef __GPIO_GXP_H__ +#define __GPIO_GXP_H__ + +#include <linux/err.h> + +/* Data provided by GPIO */ +extern u8 fan_presence; +extern u8 fan_fail; + +#endif /* __GPIO_GXP_H__ */