[v2,4/5] hwmon: (gxp_fan_ctrl) Provide fan info via gpio
Commit Message
From: Nick Hawkins <nick.hawkins@hpe.com>
The fan driver now receives fan data from GPIO via a shared variable.
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
Comments
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__ */
> 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.
Hi Guenter,
Thank you for the feedback. I did try and create a driver for both the fan
and the psu but I had an issue where the host and linux driver both
need to monitor and access it.
I made a brief query about it here to the mailing list.
(Apologies if this is the incorrect way to share a link)
https://lore.kernel.org/all/DM4PR84MB19274817C2D8A751E3218F4888759@DM4PR84MB1927.NAMPRD84.PROD.OUTLOOK.COM/
I am open for trying a different approach, I am just not sure what is
the correct way to proceed.
Is there a way for the driver to temporarily take the GPIO away from the
Host and return it? The host is wanting to hold the GPIO all the time to
monitor for change.
Another thought I had was perhaps having some duplicate I/Os where
there is one for the host consumption and the other for linux kernel
driver consumption.
Thank you for the assistance,
-Nick Hawkins
On 5/31/23 11:17, Hawkins, Nick wrote:
>> 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.
>
> Hi Guenter,
>
> Thank you for the feedback. I did try and create a driver for both the fan
> and the psu but I had an issue where the host and linux driver both
> need to monitor and access it.
>
> I made a brief query about it here to the mailing list.
> (Apologies if this is the incorrect way to share a link)
> https://lore.kernel.org/all/DM4PR84MB19274817C2D8A751E3218F4888759@DM4PR84MB1927.NAMPRD84.PROD.OUTLOOK.COM/
>
> I am open for trying a different approach, I am just not sure what is
> the correct way to proceed.
>
> Is there a way for the driver to temporarily take the GPIO away from the
> Host and return it? The host is wanting to hold the GPIO all the time to
I don't think so.
> monitor for change.
>
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.
> Another thought I had was perhaps having some duplicate I/Os where
> there is one for the host consumption and the other for linux kernel
> driver consumption.
>
I neither think this is a good idea nor that it is really necessary.
Again, the desire to have direct user space access to gpio pins is
something you _want_, not something that is really needed.
Guenter
> 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
@@ -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.
@@ -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,
new file mode 100644
@@ -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__ */