Re: Enabling pmbus power control
On Tue, Apr 20, 2021 at 12:15:40PM CDT, Mark Brown wrote: On Tue, Apr 20, 2021 at 11:40:24AM -0500, Zev Weiss wrote: On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote: > I already suggested writing a driver or drivers that represent the > hardware you have, that advice remains. It's hard to follow what you > were trying to say with your long mail earlier today but it seems like That email was an attempt to explain why writing a driver for the specific hardware devices we're powering seems like a poor fit to me. To summarize: - There's a wide variety of different devices potentially behind an LM25066. This is true for lots of hardware, we still integrate things into frameworks. - A hypothetical driver for any one of them would be completely non-specific to that device and functionally identical to a driverfor any other, because the only hardware it would actually betouching is the LM25066, and in ways that are again completelynon-specific to anything but the LM25066 itself. I don't see why that would be the case at all. Even within the indended application as a power controller for a hotpluggable bus there's plenty of potential for integration into a wider representation of the socket things get inserted into - for example I've worked with buses that had support for operator signalling of hotplug (buttons to press to initiate hot removal, with lights to signal when a clean shutdown of the card had been completed), you might also want to have additional environment monitoring and of course the labelling that I mentioned in an earlier post. I can imagine you probably have some other connection of some kind to the host too (eg, network ports) to join up and perhaps sync hotplug for. Consider the power shelf I mentioned earlier -- it's a rackmount power supply and that's about it. It provides DC power to arbitrary devices that it has no other connection to, just ground and +12V. Those devices might be servers, or cooling fans, or vacuum cleaners or floodlights -- the power shelf doesn't know, or care. It's a lot like a switchable network PDU in that it just provides a way for an operator to remotely cut power to a thing that's plugged into it. There's no other bus or anything in the picture. (And pragmatically, given that its most common usage is likely to be to provide a cold power cycle as a last-ditch recovery option when things are wedged in some unresponsive state, attempting any sort of coordination with the downstream device would probably be a dead end anyway.) Zev
Re: Enabling pmbus power control
On Tue, Apr 20, 2021 at 11:40:24AM -0500, Zev Weiss wrote: > On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote: > > I already suggested writing a driver or drivers that represent the > > hardware you have, that advice remains. It's hard to follow what you > > were trying to say with your long mail earlier today but it seems like > That email was an attempt to explain why writing a driver for the specific > hardware devices we're powering seems like a poor fit to me. To summarize: > - There's a wide variety of different devices potentially behind an > LM25066. This is true for lots of hardware, we still integrate things into frameworks. > - A hypothetical driver for any one of them would be completely > non-specific to that device and functionally identical to a driverfor > any other, because the only hardware it would actually betouching is the > LM25066, and in ways that are again completelynon-specific to anything > but the LM25066 itself. I don't see why that would be the case at all. Even within the indended application as a power controller for a hotpluggable bus there's plenty of potential for integration into a wider representation of the socket things get inserted into - for example I've worked with buses that had support for operator signalling of hotplug (buttons to press to initiate hot removal, with lights to signal when a clean shutdown of the card had been completed), you might also want to have additional environment monitoring and of course the labelling that I mentioned in an earlier post. I can imagine you probably have some other connection of some kind to the host too (eg, network ports) to join up and perhaps sync hotplug for. > I'm not at all opposed to using a abstractions or frameworks (I'd very much > like to do so in fact). The problem is that I've thus far been unable to > determine exactly what the appropriate one is. Perhaps you need to write some kind of generic system for hotplugging modules if you can't find one. signature.asc Description: PGP signature
Re: Enabling pmbus power control
On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote: On Tue, Apr 20, 2021 at 10:19:04AM -0500, Zev Weiss wrote: Mark, do you have any further input on what a viable approach might look like? I already suggested writing a driver or drivers that represent the hardware you have, that advice remains. It's hard to follow what you were trying to say with your long mail earlier today but it seems like That email was an attempt to explain why writing a driver for the specific hardware devices we're powering seems like a poor fit to me. To summarize: - There's a wide variety of different devices potentially behind an LM25066. - A hypothetical driver for any one of them would be completely non-specific to that device and functionally identical to a driver for any other, because the only hardware it would actually be touching is the LM25066, and in ways that are again completely non-specific to anything but the LM25066 itself. you basically don't want to use any abstraction or framework, but that's not really suitable for upstream integration I'm not at all opposed to using a abstractions or frameworks (I'd very much like to do so in fact). The problem is that I've thus far been unable to determine exactly what the appropriate one is. other hardware that looks similar to the end user should look similar in the kernel too. Agreed -- hence my disinclination to write drivers artificially specific to whatever is behind the LM25066. What it looks like to the end user, and I'd hope evetually the kernel as well, is a simple power switch and nothing more. Zev
Re: Enabling pmbus power control
On Tue, Apr 20, 2021 at 10:19:04AM -0500, Zev Weiss wrote: > Mark, do you have any further input on what a viable approach might look > like? I already suggested writing a driver or drivers that represent the hardware you have, that advice remains. It's hard to follow what you were trying to say with your long mail earlier today but it seems like you basically don't want to use any abstraction or framework, but that's not really suitable for upstream integration - other hardware that looks similar to the end user should look similar in the kernel too. signature.asc Description: PGP signature
Re: Enabling pmbus power control
On Tue, Apr 20, 2021 at 05:52:16AM CDT, Guenter Roeck wrote: On 4/20/21 12:06 AM, Zev Weiss wrote: On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote: On 4/19/21 10:50 PM, Zev Weiss wrote: [ ... ] I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch). As an alternative, would something like the patch below be more along the lines of what you're suggesting? And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support? No. Don't access pmbus functions from outside drivers/hwmon/pmbus. I used to be opposed to function export restrictions (aka export namespaces), but you are making a good case that we need to introduce them for pmbus functions. Guenter Okay -- I figured that was likely to be frowned upon, but the alternative seemed to be effectively duplicating non-trivial chunks of the pmbus code. However, upon realizing that the LM25066 doesn't actually require any of the paging work the generic pmbus code provides, I suppose it can actually be done with a simple smbus read & write. Does this version look better? It is just getting worse and worse. You are re-implementing regulator support for the lm25066. The correct solution would be to implement regulator support in the lm25066 and use it from the secondary driver (which should be chip independent). Guenter Implementing it by way of regulator support seems like it would make sense, but that in turn sounds like it would then end up just being a reimplementation of REGULATOR_USERSPACE_CONSUMER, which (unless there was some more subtle distinction that I missed) Mark already told me in no uncertain terms is not the way to go. So I hope I could be forgiven for feeling a bit stuck here. Mark, do you have any further input on what a viable approach might look like? Thanks, Zev
Re: Enabling pmbus power control
On 4/20/21 12:06 AM, Zev Weiss wrote: > On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote: >> On 4/19/21 10:50 PM, Zev Weiss wrote: >> [ ... ] >> >>> I had a glance at the enclosure driver; it looks pretty geared toward >>> SES-like things (drivers/scsi/ses.c being its only usage I can see in the >>> kernel at the moment) and while it could perhaps be pressed into working >>> for this it seems like it would probably drag in a fair amount of >>> boilerplate and result in a somewhat gratuitously confusing driver >>> arrangement (calling the things involved in the cases we're looking at >>> "enclosures" seems like a bit of a stretch). >>> >>> As an alternative, would something like the patch below be more along the >>> lines of what you're suggesting? And if so, would it make sense to >>> generalize it into something like 'pmbus-switch.c' and add a >>> PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code >>> instead of hardcoding it for only LM25066 support? >>> >>> >> >> No. Don't access pmbus functions from outside drivers/hwmon/pmbus. >> >> I used to be opposed to function export restrictions (aka export namespaces), >> but you are making a good case that we need to introduce them for pmbus >> functions. >> >> Guenter > > Okay -- I figured that was likely to be frowned upon, but the alternative > seemed to be effectively duplicating non-trivial chunks of the pmbus code. > However, upon realizing that the LM25066 doesn't actually require any of the > paging work the generic pmbus code provides, I suppose it can actually be > done with a simple smbus read & write. Does this version look better? > It is just getting worse and worse. You are re-implementing regulator support for the lm25066. The correct solution would be to implement regulator support in the lm25066 and use it from the secondary driver (which should be chip independent). Guenter > > Zev > > > From 1662e1c59c498ad6b208e6ab450bd467d71def34 Mon Sep 17 00:00:00 2001 > From: Zev Weiss > Date: Wed, 31 Mar 2021 01:58:35 -0500 > Subject: [PATCH] misc: add lm25066-switch driver > > This driver allows an lm25066 to be switched on and off from userspace > via sysfs. > > Signed-off-by: Zev Weiss > --- > drivers/misc/Kconfig | 7 ++ > drivers/misc/Makefile | 1 + > drivers/misc/lm25066-switch.c | 126 ++ > 3 files changed, 134 insertions(+) > create mode 100644 drivers/misc/lm25066-switch.c > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index f532c59bb59b..384b6022ec15 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -445,6 +445,13 @@ config HISI_HIKEY_USB > switching between the dual-role USB-C port and the USB-A host ports > using only one USB controller. > > +config LM25066_SWITCH > + tristate "LM25066 power switch support" > + depends on OF && SENSORS_LM25066 > + help > + This driver augments LM25066 hwmon support with power switch > + functionality controllable from userspace via sysfs. > + > source "drivers/misc/c2port/Kconfig" > source "drivers/misc/eeprom/Kconfig" > source "drivers/misc/cb710/Kconfig" > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 99b6f15a3c70..c948510d0cc9 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI) += habanalabs/ > obj-$(CONFIG_UACCE) += uacce/ > obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o > obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o > +obj-$(CONFIG_LM25066_SWITCH) += lm25066-switch.o > diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c > new file mode 100644 > index ..9adc67c320f9 > --- /dev/null > +++ b/drivers/misc/lm25066-switch.c > @@ -0,0 +1,126 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * This module provides a thin wrapper around the lm25066 hwmon driver that > + * additionally exposes a userspace-controllable on/off power switch via > + * sysfs. > + * > + * Author: Zev Weiss > + * > + * Copyright (C) 2021 Equinix Services, Inc. > + */ > +#include > +#include > +#include > +#include > +#include > + > +/* > + * The relevant PMBus command and data values for controlling the LM25066 > + * power state. Because it's not a paged device we skip the usual paging > + * business other PMBus devices might require. > + */ > +#define CMD_OPERATION 0x01 > +#define OPERATION_ON 0x80 > +#define OPERATION_OFF 0x00 > + > +static ssize_t switch_show_state(struct device *dev, struct device_attribute > *attr, > + char *buf) > +{ > + struct i2c_client *pmic = dev_get_drvdata(dev); > + ssize_t ret = i2c_smbus_read_byte_data(pmic, CMD_OPERATION); > + if (ret < 0) > + return ret; > + > + return sysfs_emit(buf, "%s\n", (ret & OPERATION_ON) ? "on" : "off"); > +} > + > +static ssize_t switch_set_state(struct device *dev, struct device_attribute
Re: Enabling pmbus power control
On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote: On 4/19/21 10:50 PM, Zev Weiss wrote: [ ... ] I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch). As an alternative, would something like the patch below be more along the lines of what you're suggesting? And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support? No. Don't access pmbus functions from outside drivers/hwmon/pmbus. I used to be opposed to function export restrictions (aka export namespaces), but you are making a good case that we need to introduce them for pmbus functions. Guenter Okay -- I figured that was likely to be frowned upon, but the alternative seemed to be effectively duplicating non-trivial chunks of the pmbus code. However, upon realizing that the LM25066 doesn't actually require any of the paging work the generic pmbus code provides, I suppose it can actually be done with a simple smbus read & write. Does this version look better? Zev From 1662e1c59c498ad6b208e6ab450bd467d71def34 Mon Sep 17 00:00:00 2001 From: Zev Weiss Date: Wed, 31 Mar 2021 01:58:35 -0500 Subject: [PATCH] misc: add lm25066-switch driver This driver allows an lm25066 to be switched on and off from userspace via sysfs. Signed-off-by: Zev Weiss --- drivers/misc/Kconfig | 7 ++ drivers/misc/Makefile | 1 + drivers/misc/lm25066-switch.c | 126 ++ 3 files changed, 134 insertions(+) create mode 100644 drivers/misc/lm25066-switch.c diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index f532c59bb59b..384b6022ec15 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -445,6 +445,13 @@ config HISI_HIKEY_USB switching between the dual-role USB-C port and the USB-A host ports using only one USB controller. +config LM25066_SWITCH + tristate "LM25066 power switch support" + depends on OF && SENSORS_LM25066 + help + This driver augments LM25066 hwmon support with power switch + functionality controllable from userspace via sysfs. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig" diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 99b6f15a3c70..c948510d0cc9 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI) += habanalabs/ obj-$(CONFIG_UACCE)+= uacce/ obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o +obj-$(CONFIG_LM25066_SWITCH) += lm25066-switch.o diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c new file mode 100644 index ..9adc67c320f9 --- /dev/null +++ b/drivers/misc/lm25066-switch.c @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * This module provides a thin wrapper around the lm25066 hwmon driver that + * additionally exposes a userspace-controllable on/off power switch via + * sysfs. + * + * Author: Zev Weiss + * + * Copyright (C) 2021 Equinix Services, Inc. + */ +#include +#include +#include +#include +#include + +/* + * The relevant PMBus command and data values for controlling the LM25066 + * power state. Because it's not a paged device we skip the usual paging + * business other PMBus devices might require. + */ +#define CMD_OPERATION 0x01 +#define OPERATION_ON 0x80 +#define OPERATION_OFF 0x00 + +static ssize_t switch_show_state(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct i2c_client *pmic = dev_get_drvdata(dev); + ssize_t ret = i2c_smbus_read_byte_data(pmic, CMD_OPERATION); + if (ret < 0) + return ret; + + return sysfs_emit(buf, "%s\n", (ret & OPERATION_ON) ? "on" : "off"); +} + +static ssize_t switch_set_state(struct device *dev, struct device_attribute *attr, +const char *buf, size_t count) +{ + int status; + u8 value; + struct i2c_client *pmic = dev_get_drvdata(dev); + if (sysfs_streq(buf, "on")) + value = OPERATION_ON; + else if (sysfs_streq(buf, "off")) + value = OPERATION_OFF; + else + return -EINVAL; + status = i2c_smbus_write_byte_data(pmic, CMD_OPERATION, value); + return status ? : count; +} + +static DEVICE_ATTR(state, 0644, switch_
Re: Enabling pmbus power control
On 4/19/21 10:50 PM, Zev Weiss wrote: [ ... ] > I had a glance at the enclosure driver; it looks pretty geared toward > SES-like things (drivers/scsi/ses.c being its only usage I can see in the > kernel at the moment) and while it could perhaps be pressed into working for > this it seems like it would probably drag in a fair amount of boilerplate and > result in a somewhat gratuitously confusing driver arrangement (calling the > things involved in the cases we're looking at "enclosures" seems like a bit > of a stretch). > > As an alternative, would something like the patch below be more along the > lines of what you're suggesting? And if so, would it make sense to > generalize it into something like 'pmbus-switch.c' and add a > PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead > of hardcoding it for only LM25066 support? > > No. Don't access pmbus functions from outside drivers/hwmon/pmbus. I used to be opposed to function export restrictions (aka export namespaces), but you are making a good case that we need to introduce them for pmbus functions. Guenter
Re: Enabling pmbus power control
On Mon, Apr 19, 2021 at 10:36:48PM CDT, Guenter Roeck wrote: On Mon, Apr 19, 2021 at 08:29:53PM -0500, Zev Weiss wrote: On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote: > On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote: > > On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote: > > > > > Okay, to expand a bit on the description in my initial message -- we've > > > got a single chassis with multiple server boards and a single manager board > > > that handles, among other things, power control for the servers. > > > The manager board has one LM25066 for each attached server, which acts as > > > the "power switch" for that server. There thus really isn't any driver to > > > speak of for the downstream device. > > > > This sounds like you need a driver representing those server boards (or > > the slots they plug into perhaps) that represents everything about those > > boards to userspace, including power switching. I don't see why you > > wouldn't have a driver for that - it's a thing that physically exists > > and clearly has some software control, and you'd presumably also expect > > to represent some labelling about the slot as well. > > Absolutely agree. > > Thanks, > Guenter Hi Guenter, Mark, I wanted to return to this to try to explain why structuring the kernel support for this in a way that's specific to the device behind the PMIC seems like an awkward fit to me, and ultimately kind of artificial. In the system I described, the manager board with the LM25066 devices is its own little self-contained BMC system running its own Linux kernel (though "BMC" is perhaps a slightly misleading term because there's no host system that it manages). The PMICs are really the only relevant connection it has to the servers it controls power for -- they have their own dedicated local BMCs on board as well doing all the usual BMC tasks. A hypothetical dedicated driver for this on the manager board wouldn't have any other hardware to touch aside from the pmbus interface of the LM25066 itself, so calling it a server board driver seems pretty arbitrary -- and in fact the same system also has another LM25066 that controls the power supply to the chassis cooling fans (a totally different downstream device, but one that would be equally well-served by the same software). More recently, another system has entered the picture for us that might illustrate it more starkly -- the Delta Open19 power shelf [0] supported by a recent code release from LinkedIn [1]. This is a rackmount power supply All I can see is that this code is a mess. with fifty outputs, each independently switchable via an LM25066 attached to an on-board BMC-style management controller (though again, no host system involved). We (Equinix Metal) are interested in porting a modern OpenBMC to it to replace the dated, crufty, pre-Linux-Foundation version of OpenBMC it currently runs (as found in the linked repo). The exact nature of the devices powered by its outputs is well outside the scope of the firmware running on that controller (it could be any arbitrary thing that runs on 12VDC), but we still want to be able to both (a) retrieve per-output voltage/current/power readings as provided by the existing LM25066 hwmon support, and (b) control the on/off state of those outputs from userspace. Given the array of possible use-cases, an approach of adding power-switch functionality to the existing LM25066 support seems like the most obvious way to support this, so I'm hoping to see if I can get some idea of what an acceptable implementation of that might look like. Sorry, that is simply a no-go for the LM25066 driver. I mentioned it before, and it is still true: The hwmon subsystem is not in the business of power control. Have you looked into enclosures (drivers/misc/enclosure.c) ? Maybe that or something similar could be used for your purposes. Thanks, Guenter I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch). As an alternative, would something like the patch below be more along the lines of what you're suggesting? And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support? Thanks, Zev From f4c0a3637371d69a414b6fb882497d22e5de3ba0 Mon Sep 17 00:00:00 2001 From: Zev Weiss Date: Wed, 31 Mar 2021 01:58:35 -0500 Subject: [PATCH] misc: add lm25066-switch driver This driver allows an lm25066 to be switched on and off from userspace via sysf
Re: Enabling pmbus power control
On Mon, Apr 19, 2021 at 08:29:53PM -0500, Zev Weiss wrote: > On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote: > > On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote: > > > On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote: > > > > > > > Okay, to expand a bit on the description in my initial message -- we've > > > > got a single chassis with multiple server boards and a single manager > > > > board > > > > that handles, among other things, power control for the servers. > > > > The manager board has one LM25066 for each attached server, which acts > > > > as > > > > the "power switch" for that server. There thus really isn't any driver > > > > to > > > > speak of for the downstream device. > > > > > > This sounds like you need a driver representing those server boards (or > > > the slots they plug into perhaps) that represents everything about those > > > boards to userspace, including power switching. I don't see why you > > > wouldn't have a driver for that - it's a thing that physically exists > > > and clearly has some software control, and you'd presumably also expect > > > to represent some labelling about the slot as well. > > > > Absolutely agree. > > > > Thanks, > > Guenter > > Hi Guenter, Mark, > > I wanted to return to this to try to explain why structuring the kernel > support for this in a way that's specific to the device behind the PMIC > seems like an awkward fit to me, and ultimately kind of artificial. > > In the system I described, the manager board with the LM25066 devices is its > own little self-contained BMC system running its own Linux kernel (though > "BMC" is perhaps a slightly misleading term because there's no host system > that it manages). The PMICs are really the only relevant connection it has > to the servers it controls power for -- they have their own dedicated local > BMCs on board as well doing all the usual BMC tasks. A hypothetical > dedicated driver for this on the manager board wouldn't have any other > hardware to touch aside from the pmbus interface of the LM25066 itself, so > calling it a server board driver seems pretty arbitrary -- and in fact the > same system also has another LM25066 that controls the power supply to the > chassis cooling fans (a totally different downstream device, but one that > would be equally well-served by the same software). > > More recently, another system has entered the picture for us that might > illustrate it more starkly -- the Delta Open19 power shelf [0] supported by > a recent code release from LinkedIn [1]. This is a rackmount power supply All I can see is that this code is a mess. > with fifty outputs, each independently switchable via an LM25066 attached to > an on-board BMC-style management controller (though again, no host system > involved). We (Equinix Metal) are interested in porting a modern OpenBMC to > it to replace the dated, crufty, pre-Linux-Foundation version of OpenBMC it > currently runs (as found in the linked repo). The exact nature of the > devices powered by its outputs is well outside the scope of the firmware > running on that controller (it could be any arbitrary thing that runs on > 12VDC), but we still want to be able to both (a) retrieve per-output > voltage/current/power readings as provided by the existing LM25066 hwmon > support, and (b) control the on/off state of those outputs from userspace. > > Given the array of possible use-cases, an approach of adding power-switch > functionality to the existing LM25066 support seems like the most obvious > way to support this, so I'm hoping to see if I can get some idea of what an > acceptable implementation of that might look like. > Sorry, that is simply a no-go for the LM25066 driver. I mentioned it before, and it is still true: The hwmon subsystem is not in the business of power control. Have you looked into enclosures (drivers/misc/enclosure.c) ? Maybe that or something similar could be used for your purposes. Thanks, Guenter
Re: Enabling pmbus power control
On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote: On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote: On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote: > Okay, to expand a bit on the description in my initial message -- we've > got a single chassis with multiple server boards and a single manager board > that handles, among other things, power control for the servers. > The manager board has one LM25066 for each attached server, which acts as > the "power switch" for that server. There thus really isn't any driver to > speak of for the downstream device. This sounds like you need a driver representing those server boards (or the slots they plug into perhaps) that represents everything about those boards to userspace, including power switching. I don't see why you wouldn't have a driver for that - it's a thing that physically exists and clearly has some software control, and you'd presumably also expect to represent some labelling about the slot as well. Absolutely agree. Thanks, Guenter Hi Guenter, Mark, I wanted to return to this to try to explain why structuring the kernel support for this in a way that's specific to the device behind the PMIC seems like an awkward fit to me, and ultimately kind of artificial. In the system I described, the manager board with the LM25066 devices is its own little self-contained BMC system running its own Linux kernel (though "BMC" is perhaps a slightly misleading term because there's no host system that it manages). The PMICs are really the only relevant connection it has to the servers it controls power for -- they have their own dedicated local BMCs on board as well doing all the usual BMC tasks. A hypothetical dedicated driver for this on the manager board wouldn't have any other hardware to touch aside from the pmbus interface of the LM25066 itself, so calling it a server board driver seems pretty arbitrary -- and in fact the same system also has another LM25066 that controls the power supply to the chassis cooling fans (a totally different downstream device, but one that would be equally well-served by the same software). More recently, another system has entered the picture for us that might illustrate it more starkly -- the Delta Open19 power shelf [0] supported by a recent code release from LinkedIn [1]. This is a rackmount power supply with fifty outputs, each independently switchable via an LM25066 attached to an on-board BMC-style management controller (though again, no host system involved). We (Equinix Metal) are interested in porting a modern OpenBMC to it to replace the dated, crufty, pre-Linux-Foundation version of OpenBMC it currently runs (as found in the linked repo). The exact nature of the devices powered by its outputs is well outside the scope of the firmware running on that controller (it could be any arbitrary thing that runs on 12VDC), but we still want to be able to both (a) retrieve per-output voltage/current/power readings as provided by the existing LM25066 hwmon support, and (b) control the on/off state of those outputs from userspace. Given the array of possible use-cases, an approach of adding power-switch functionality to the existing LM25066 support seems like the most obvious way to support this, so I'm hoping to see if I can get some idea of what an acceptable implementation of that might look like. Thanks, Zev [0] https://www.open19.org/marketplace/delta-16kw-power-shelf/ [1] https://github.com/linkedin/o19-bmc-firmware/tree/master/meta-openbmc/meta-linkedin/meta-deltapower
Re: Enabling pmbus power control
On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote: > On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote: > > > Okay, to expand a bit on the description in my initial message -- we've > > got a single chassis with multiple server boards and a single manager board > > that handles, among other things, power control for the servers. > > The manager board has one LM25066 for each attached server, which acts as > > the "power switch" for that server. There thus really isn't any driver to > > speak of for the downstream device. > > This sounds like you need a driver representing those server boards (or > the slots they plug into perhaps) that represents everything about those > boards to userspace, including power switching. I don't see why you > wouldn't have a driver for that - it's a thing that physically exists > and clearly has some software control, and you'd presumably also expect > to represent some labelling about the slot as well. Absolutely agree. Thanks, Guenter
Re: Enabling pmbus power control
On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote: > Okay, to expand a bit on the description in my initial message -- we've > got a single chassis with multiple server boards and a single manager board > that handles, among other things, power control for the servers. > The manager board has one LM25066 for each attached server, which acts as > the "power switch" for that server. There thus really isn't any driver to > speak of for the downstream device. This sounds like you need a driver representing those server boards (or the slots they plug into perhaps) that represents everything about those boards to userspace, including power switching. I don't see why you wouldn't have a driver for that - it's a thing that physically exists and clearly has some software control, and you'd presumably also expect to represent some labelling about the slot as well. signature.asc Description: PGP signature
Re: Enabling pmbus power control
On Tue, Mar 30, 2021 at 12:42:21PM CDT, Mark Brown wrote: On Tue, Mar 30, 2021 at 12:19:29PM -0500, Zev Weiss wrote: On Tue, Mar 30, 2021 at 06:22:54AM CDT, Mark Brown wrote: > On Tue, Mar 30, 2021 at 03:34:16AM -0700, Guenter Roeck wrote: > > (and I don't know if the userspace consumer code is appropriate - you > > might want to check with the regulator maintainer on that). > It's not, you should never see this in a production system. Sorry, can you clarify what exactly "this" refers to here? The userspace consumer. > I can't really tell what the issue is here without more context, the > global name list should not be relevant for much in a system that's well > configured so it sounds like it's user error. My initial attempt I guess followed the existing ltc2978 code a little too closely and I ended up with all my lm25066 regulators registered under the same (static) name, so when I went to attach the reg-userspace-consumer instances to them by way of that name I got this: I don't know what you're trying to do or why, nor how you're going about achieving it so I can't really comment. Like I say anything that's instantiating a userspace consumer in upstream code is broken, it's there for test during development of regulator drivers. Whatever device is supplied by the regulator should have a driver which should control the regulator at runtime if that is needed. Okay, to expand a bit on the description in my initial message -- we've got a single chassis with multiple server boards and a single manager board that handles, among other things, power control for the servers. The manager board has one LM25066 for each attached server, which acts as the "power switch" for that server. There thus really isn't any driver to speak of for the downstream device. We need to be able to toggle that power switch from userspace on the manager board, which we could achieve via i2cset, but we don't want to give up the sensors provided by the hwmon driver. The hardware seems perfectly capable of providing the functionality we need -- is there any way of implementing the requisite kernel support in a way that the relevant subsystem maintainers would find palatable, or is carrying an out-of-tree patch my only option for this? Thanks, Zev
Re: Enabling pmbus power control
On Tue, Mar 30, 2021 at 12:19:29PM -0500, Zev Weiss wrote: > On Tue, Mar 30, 2021 at 06:22:54AM CDT, Mark Brown wrote: > > On Tue, Mar 30, 2021 at 03:34:16AM -0700, Guenter Roeck wrote: > > > (and I don't know if the userspace consumer code is appropriate - you > > > might want to check with the regulator maintainer on that). > > It's not, you should never see this in a production system. > Sorry, can you clarify what exactly "this" refers to here? The userspace consumer. > > I can't really tell what the issue is here without more context, the > > global name list should not be relevant for much in a system that's well > > configured so it sounds like it's user error. > My initial attempt I guess followed the existing ltc2978 code a little too > closely and I ended up with all my lm25066 regulators registered under the > same (static) name, so when I went to attach the reg-userspace-consumer > instances to them by way of that name I got this: I don't know what you're trying to do or why, nor how you're going about achieving it so I can't really comment. Like I say anything that's instantiating a userspace consumer in upstream code is broken, it's there for test during development of regulator drivers. Whatever device is supplied by the regulator should have a driver which should control the regulator at runtime if that is needed. signature.asc Description: PGP signature
Re: Enabling pmbus power control
On Tue, Mar 30, 2021 at 06:22:54AM CDT, Mark Brown wrote: On Tue, Mar 30, 2021 at 03:34:16AM -0700, Guenter Roeck wrote: (and I don't know if the userspace consumer code is appropriate - you might want to check with the regulator maintainer on that). It's not, you should never see this in a production system. Sorry, can you clarify what exactly "this" refers to here? > first attempt at this ran into problems with all the > reg-userspace-consumer instances getting attached to the first > regulator device, I think due to all of the regulators ending up under > the same name in the global namespace of regulator_map_list. I worked > around that by adding an ID counter to produce a unique name for each, > though that changes device names in userspace-visible ways that I'm > not sure would be considered OK for backwards compatibility. (I'm not > familiar enough with the regulator code to know if there's a better > way of fixing that problem.) The #if-ing to keep it behind a Kconfig Maybe ask that question on the regulator mailing list. I can't really tell what the issue is here without more context, the global name list should not be relevant for much in a system that's well configured so it sounds like it's user error. My initial attempt I guess followed the existing ltc2978 code a little too closely and I ended up with all my lm25066 regulators registered under the same (static) name, so when I went to attach the reg-userspace-consumer instances to them by way of that name I got this: # ls -l /sys/kernel/debug/regulator/7-004?-vout0 /sys/kernel/debug/regulator/7-0040-vout0: -r--r--r--1 root root 0 Jan 1 1970 bypass_count -r--r--r--1 root root 0 Jan 1 1970 open_count drwxr-xr-x2 root root 0 Jan 1 1970 reg-userspace-consumer.0.auto-vout0 drwxr-xr-x2 root root 0 Jan 1 1970 reg-userspace-consumer.1.auto-vout0 drwxr-xr-x2 root root 0 Jan 1 1970 reg-userspace-consumer.2.auto-vout0 -r--r--r--1 root root 0 Jan 1 1970 use_count /sys/kernel/debug/regulator/7-0041-vout0: -r--r--r--1 root root 0 Jan 1 1970 bypass_count -r--r--r--1 root root 0 Jan 1 1970 open_count -r--r--r--1 root root 0 Jan 1 1970 use_count /sys/kernel/debug/regulator/7-0043-vout0: -r--r--r--1 root root 0 Jan 1 1970 bypass_count -r--r--r--1 root root 0 Jan 1 1970 open_count -r--r--r--1 root root 0 Jan 1 1970 use_count (When of course the intent is to have one reg-userspace-consumer for each regulator.) I now have a revised version that takes Guenter's comments into account and puts this logic in the lm25066 driver instead of the pmbus core though, and in the process arrived at what I expect is a better solution to the name-collision problem at least (below). Thanks, Zev diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index 32d2fc850621..d9905e95d510 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -120,6 +120,21 @@ config SENSORS_LM25066 This driver can also be built as a module. If so, the module will be called lm25066. +config SENSORS_LM25066_REGULATOR + bool "Regulator support for LM25066 and compatibles" + depends on SENSORS_LM25066 && REGULATOR + help + If you say yes here you get regulator support for National + Semiconductor LM25056, LM25066, LM5064, and LM5066. + +config SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER + bool "Userspace regulator consumer support for PMBus devices" + depends on SENSORS_LM25066_REGULATOR && REGULATOR_USERSPACE_CONSUMER + help + If you say yes here, regulator-enabled PMBus devices such as + the LM25066 and LTC2978 will have their on/off state + controllable from userspace via sysfs. + config SENSORS_LTC2978 tristate "Linear Technologies LTC2978 and compatibles" help diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c index e9a66fd9e144..4e7e66d1ca8c 100644 --- a/drivers/hwmon/pmbus/lm25066.c +++ b/drivers/hwmon/pmbus/lm25066.c @@ -14,6 +14,9 @@ #include #include #include +#include +#include +#include #include "pmbus.h" enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i }; @@ -207,6 +210,13 @@ struct lm25066_data { int id; u16 rlimit; /* Maximum register value */ struct pmbus_driver_info info; + +#if IS_ENABLED(CONFIG_SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER) + struct regulator_desc reg_desc; + struct regulator_bulk_data reg_supply; + struct regulator_userspace_consumer_data consumerdata; + struct platform_device platdev; +#endif }; #define to_lm25066_data(x) container_of(x, struct lm25066_data, info)
Re: Enabling pmbus power control
On Tue, Mar 30, 2021 at 03:34:16AM -0700, Guenter Roeck wrote: > (and I don't know if the userspace consumer code is appropriate - you > might want to check with the regulator maintainer on that). It's not, you should never see this in a production system. > > first attempt at this ran into problems with all the > > reg-userspace-consumer instances getting attached to the first > > regulator device, I think due to all of the regulators ending up under > > the same name in the global namespace of regulator_map_list. I worked > > around that by adding an ID counter to produce a unique name for each, > > though that changes device names in userspace-visible ways that I'm > > not sure would be considered OK for backwards compatibility. (I'm not > > familiar enough with the regulator code to know if there's a better > > way of fixing that problem.) The #if-ing to keep it behind a Kconfig > Maybe ask that question on the regulator mailing list. I can't really tell what the issue is here without more context, the global name list should not be relevant for much in a system that's well configured so it sounds like it's user error. signature.asc Description: PGP signature
Re: Enabling pmbus power control
On 3/30/21 1:17 AM, Zev Weiss wrote: > > Hello, > > I'm working on a board that has a handful of LM25066 PMICs controlling > the power supply to various devices, and I'd like to have both the > existing hwmon sensor functionality as well as userspace power on/off > control, which does not currently seem to be available (other than via > 'i2cset -f', which I'd of course prefer to avoid). I've drafted up a > couple possible versions of this, and was hoping to get some opinions > on the appropriate overall approach. > > One option is to add a read-write sysfs attribute to the existing > hwmon directory (current incarnation of the patch: > https://thorn.bewilderbeest.net/~zev/patches/pmbus-statectl.patch). > This bears a vague resemblance to a patch that was rejected a couple > years ago > (https://lore.kernel.org/linux-hwmon/20190417161817.ga13...@roeck-us.net/), > but is different enough that I wonder if it might potentially be > tolerable? (It exposes significantly less, for one thing.) > This is a no-go. We are not going to replicate regulator functionality in the hwmon subsystem, no matter by what means. > The other approach involves layering a regulator device over the pmbus > device as is done in the LTC2978 driver, and then putting a > reg-userspace-consumer on top of that (current patch: > https://thorn.bewilderbeest.net/~zev/patches/pmbus-ureg.patch). My This is the way to go, but the regulator descriptor (what is currently CONFIG_PMBUS_USERSPACE_REGULATOR_CONSUMER) should be in the lm25066 driver. I don't want to pollute the pmbus core with that at this point (and I don't know if the userspace consumer code is appropriate - you might want to check with the regulator maintainer on that). > first attempt at this ran into problems with all the > reg-userspace-consumer instances getting attached to the first > regulator device, I think due to all of the regulators ending up under > the same name in the global namespace of regulator_map_list. I worked > around that by adding an ID counter to produce a unique name for each, > though that changes device names in userspace-visible ways that I'm > not sure would be considered OK for backwards compatibility. (I'm not > familiar enough with the regulator code to know if there's a better > way of fixing that problem.) The #if-ing to keep it behind a Kconfig Maybe ask that question on the regulator mailing list. Guenter > option is also kind of ugly as it stands. > > The first version seems simpler to me (and avoids the rather more > cumbersome sysfs paths the second one produces, for what that's > worth). I think the second is (at least structurally) perhaps more > aligned with what Guenter was saying in the previous discussion linked > above, though. Does anyone have any advice on the best way to proceed > with this? If the reg-userspace-consumer approach is the preferred > route, suggestions on a better fix for the name collision problem > would be welcome. > > > Thanks, > Zev >
Enabling pmbus power control
Hello, I'm working on a board that has a handful of LM25066 PMICs controlling the power supply to various devices, and I'd like to have both the existing hwmon sensor functionality as well as userspace power on/off control, which does not currently seem to be available (other than via 'i2cset -f', which I'd of course prefer to avoid). I've drafted up a couple possible versions of this, and was hoping to get some opinions on the appropriate overall approach. One option is to add a read-write sysfs attribute to the existing hwmon directory (current incarnation of the patch: https://thorn.bewilderbeest.net/~zev/patches/pmbus-statectl.patch). This bears a vague resemblance to a patch that was rejected a couple years ago (https://lore.kernel.org/linux-hwmon/20190417161817.ga13...@roeck-us.net/), but is different enough that I wonder if it might potentially be tolerable? (It exposes significantly less, for one thing.) The other approach involves layering a regulator device over the pmbus device as is done in the LTC2978 driver, and then putting a reg-userspace-consumer on top of that (current patch: https://thorn.bewilderbeest.net/~zev/patches/pmbus-ureg.patch). My first attempt at this ran into problems with all the reg-userspace-consumer instances getting attached to the first regulator device, I think due to all of the regulators ending up under the same name in the global namespace of regulator_map_list. I worked around that by adding an ID counter to produce a unique name for each, though that changes device names in userspace-visible ways that I'm not sure would be considered OK for backwards compatibility. (I'm not familiar enough with the regulator code to know if there's a better way of fixing that problem.) The #if-ing to keep it behind a Kconfig option is also kind of ugly as it stands. The first version seems simpler to me (and avoids the rather more cumbersome sysfs paths the second one produces, for what that's worth). I think the second is (at least structurally) perhaps more aligned with what Guenter was saying in the previous discussion linked above, though. Does anyone have any advice on the best way to proceed with this? If the reg-userspace-consumer approach is the preferred route, suggestions on a better fix for the name collision problem would be welcome. Thanks, Zev