| Message ID | 20230927202347.2795170-1-scott@os.amperecomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: hotplug: Add extension driver for Ampere Altra hotplug LED control | expand |
On Thu, Sep 28, 2023 at 5:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Sep 27, 2023 at 01:23:47PM -0700, D Scott Phillips wrote: > > On Ampere Altra, PCIe hotplug is handled through ACPI. A side interface is > > also present to request system firmware control of attention LEDs. Add an > > ACPI PCI Hotplug companion driver to support attention LED control. > > > > Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com> > > --- > > drivers/pci/hotplug/Kconfig | 13 ++ > > drivers/pci/hotplug/Makefile | 3 +- > > drivers/pci/hotplug/acpiphp_ampere_altra.c | 141 +++++++++++++++++++++ > > 3 files changed, 156 insertions(+), 1 deletion(-) > > create mode 100644 drivers/pci/hotplug/acpiphp_ampere_altra.c > > > > diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig > > index 48113b210cf93..9fde600a9ad3e 100644 > > --- a/drivers/pci/hotplug/Kconfig > > +++ b/drivers/pci/hotplug/Kconfig > > @@ -61,6 +61,19 @@ config HOTPLUG_PCI_ACPI > > > > When in doubt, say N. > > > > +config HOTPLUG_PCI_ACPI_AMPERE_ALTRA > > + tristate "ACPI PCI Hotplug driver Ampere Altra extensions" > > + depends on HOTPLUG_PCI_ACPI > > + depends on HAVE_ARM_SMCCC_DISCOVERY > > + depends on m > > Why is this restricted to being a module? It's not unprecedented, but > unless this only works as a module for some reason, I would leave that > choice up to the user. > > > + help > > + Say Y here if you have an Ampere Altra system. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called acpiphp_ampere_altra. > > + > > + When in doubt, say N. > > + > > config HOTPLUG_PCI_ACPI_IBM > > tristate "ACPI PCI Hotplug driver IBM extensions" > > depends on HOTPLUG_PCI_ACPI > > diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile > > index 5196983220df6..29d7f6171b305 100644 > > --- a/drivers/pci/hotplug/Makefile > > +++ b/drivers/pci/hotplug/Makefile > > @@ -21,8 +21,9 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR) += rpadlpar_io.o > > obj-$(CONFIG_HOTPLUG_PCI_ACPI) += acpiphp.o > > obj-$(CONFIG_HOTPLUG_PCI_S390) += s390_pci_hpc.o > > > > -# acpiphp_ibm extends acpiphp, so should be linked afterwards. > > +# acpiphp_ibm extend acpiphp, so should be linked afterwards. > > > > +obj-$(CONFIG_HOTPLUG_PCI_ACPI_AMPERE_ALTRA) += acpiphp_ampere_altra.o > > obj-$(CONFIG_HOTPLUG_PCI_ACPI_IBM) += acpiphp_ibm.o > > > > pci_hotplug-objs := pci_hotplug_core.o > > diff --git a/drivers/pci/hotplug/acpiphp_ampere_altra.c b/drivers/pci/hotplug/acpiphp_ampere_altra.c > > new file mode 100644 > > index 0000000000000..8692b939dea78 > > --- /dev/null > > +++ b/drivers/pci/hotplug/acpiphp_ampere_altra.c > > @@ -0,0 +1,141 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * ACPI PCI Hot Plug Ampere Altra Extension > > Would be helpful to include a hint about what this module *does*. > IIUC, it controls the attention indicator. > > > + * > > + * Copyright (C) 2023 Ampere Computing LLC > > + * > > Spurious blank line. > > > + */ > > + > > +#define pr_fmt(fmt) "acpiphp_ampere_altra: " fmt > > + > > +#include <linux/init.h> > > +#include <linux/module.h> > > +#include <linux/pci.h> > > +#include <linux/pci_hotplug.h> > > + > > +#include "acpiphp.h" > > + > > +#define HANDLE_OPEN 0xb0200000 > > +#define HANDLE_CLOSE 0xb0300000 > > +#define REQUEST 0xf0700000 > > +#define LED_CMD 0x00000004 > > +#define LED_ATTENTION 0x00000002 > > +#define LED_SET_ON 0x00000001 > > +#define LED_SET_OFF 0x00000002 > > +#define LED_SET_BLINK 0x00000003 > > + > > +static const struct acpi_device_id acpi_ids[] = { > > + {"AMPC0008", 0}, {} > > +}; > > +MODULE_DEVICE_TABLE(acpi, acpi_ids); > > + > > +static u32 led_service_id[4]; > > + > > +static int led_status(u8 status) > > +{ > > + switch (status) { > > + case 1: return LED_SET_ON; > > + case 2: return LED_SET_BLINK; > > + default: return LED_SET_OFF; > > + } > > +} > > + > > +static int set_attention_status(struct hotplug_slot *slot, u8 status) > > +{ > > + struct arm_smccc_res res; > > + struct pci_bus *bus; > > + struct pci_dev *root_port; > > + unsigned long flags; > > + u32 handle; > > + int ret = 0; > > + > > + bus = slot->pci_slot->bus; > > + root_port = pcie_find_root_port(bus->self); > > + if (!root_port) > > + return -ENODEV; > > + > > + local_irq_save(flags); > > + arm_smccc_smc(HANDLE_OPEN, led_service_id[0], led_service_id[1], > > + led_service_id[2], led_service_id[3], 0, 0, 0, &res); > > + if (res.a0) { > > + ret = -ENODEV; > > + goto out; > > + } > > + handle = res.a1 & 0xffff0000; > > + > > + arm_smccc_smc(REQUEST, LED_CMD, led_status(status), LED_ATTENTION, > > + pci_domain_nr(bus) | ((root_port->devfn >> 3) << 4), 0, 0, > > PCI_SLOT(root_port->devfn) > > > + handle, &res); > > + if (res.a0) > > + ret = -ENODEV; > > + > > + arm_smccc_smc(HANDLE_CLOSE, handle, 0, 0, 0, 0, 0, 0, &res); > > + > > + out: > > + local_irq_restore(flags); > > + return ret; > > +} > > + > > +static int get_attention_status(struct hotplug_slot *slot, u8 *status) > > +{ > > + return -EINVAL; > > +} > > + > > +static struct acpiphp_attention_info ampere_altra_attn = { > > + .set_attn = set_attention_status, > > + .get_attn = get_attention_status, > > + .owner = THIS_MODULE, > > +}; > > + > > +static acpi_status __init get_acpi_handle(acpi_handle handle, u32 level, > > + void *context, void **return_value) > > +{ > > + *(acpi_handle *)return_value = handle; > > + return AE_CTRL_TERMINATE; > > +} > > + > > +static int __init acpiphp_ampere_altra_init(void) > > +{ > > + struct fwnode_handle *fwnode; > > + acpi_handle leds_handle = NULL; > > + struct acpi_device *leds; > > + acpi_status status; > > + int ret; > > + > > + status = acpi_get_devices("AMPC0008", get_acpi_handle, NULL, > > + &leds_handle); > > Rafael, can you comment on whether we should use acpi_get_devices(), > acpi_bus_register_driver(), acpi_acpi_scan_add_handler(), or something > else here? Personally, I would go for a platform driver, because the ACPI core should create a platform device for this object. acpi_get_devices() carries out a namespace walk that is costly and entirely avoidable. > I try to avoid pci_get_device() because it subverts the > driver model (no hotplug support, no driver/device binding). > > I see Documentation/driver-api/acpi/scan_handlers.rst, but I'm not > clear on when to use acpi_bus_register_driver() vs Never. > acpi_acpi_scan_add_handler(). When you don't want the ACPI core to create a platform device for your ACPI device object. There are cases like that, but they are rare. > I guess the only ACPI connection here is to retrieve the "uuid" > property once, and there's no need for any ACPI services after that. That's in agreement with my understanding.
On Fri, Sep 29, 2023 at 09:06:02PM +0200, Rafael J. Wysocki wrote: > On Thu, Sep 28, 2023 at 5:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Sep 27, 2023 at 01:23:47PM -0700, D Scott Phillips wrote: > > > On Ampere Altra, PCIe hotplug is handled through ACPI. A side interface is > > > also present to request system firmware control of attention LEDs. Add an > > > ACPI PCI Hotplug companion driver to support attention LED control. > > > +static int __init acpiphp_ampere_altra_init(void) > > > +{ > > > + struct fwnode_handle *fwnode; > > > + acpi_handle leds_handle = NULL; > > > + struct acpi_device *leds; > > > + acpi_status status; > > > + int ret; > > > + > > > + status = acpi_get_devices("AMPC0008", get_acpi_handle, NULL, > > > + &leds_handle); > > > > Rafael, can you comment on whether we should use acpi_get_devices(), > > acpi_bus_register_driver(), acpi_acpi_scan_add_handler(), or something > > else here? > > Personally, I would go for a platform driver, because the ACPI core > should create a platform device for this object. > > acpi_get_devices() carries out a namespace walk that is costly and > entirely avoidable. > > > I try to avoid pci_get_device() because it subverts the > > driver model (no hotplug support, no driver/device binding). > > > > I see Documentation/driver-api/acpi/scan_handlers.rst, but I'm not > > clear on when to use acpi_bus_register_driver() vs > > Never. > > > acpi_acpi_scan_add_handler(). > > When you don't want the ACPI core to create a platform device for your > ACPI device object. There are cases like that, but they are rare. Ah, so none of the above (not acpi_get_devices(), acpi_bus_register_driver(), OR acpi_acpi_scan_add_handler()). IIUC, what you propose would look something like this: static u32 led_service_id[4]; static int altra_led_probe(struct platform_device *pdev) { struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev); fwnode_property_read_u32_array(fwnode, "uuid", led_service_id, 4); } static const struct acpi_device_id altra_led_ids[] = { {"AMPC0008", 0}, {} }; MODULE_DEVICE_TABLE(acpi, altra_led_ids); static struct platform_driver altra_led_driver = { .driver = { .acpi_match_table = altra_led_ids, }, .probe = altra_led_probe, }; module_platform_driver(altra_led_driver); Bjorn
diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig index 48113b210cf93..9fde600a9ad3e 100644 --- a/drivers/pci/hotplug/Kconfig +++ b/drivers/pci/hotplug/Kconfig @@ -61,6 +61,19 @@ config HOTPLUG_PCI_ACPI When in doubt, say N. +config HOTPLUG_PCI_ACPI_AMPERE_ALTRA + tristate "ACPI PCI Hotplug driver Ampere Altra extensions" + depends on HOTPLUG_PCI_ACPI + depends on HAVE_ARM_SMCCC_DISCOVERY + depends on m + help + Say Y here if you have an Ampere Altra system. + + To compile this driver as a module, choose M here: the + module will be called acpiphp_ampere_altra. + + When in doubt, say N. + config HOTPLUG_PCI_ACPI_IBM tristate "ACPI PCI Hotplug driver IBM extensions" depends on HOTPLUG_PCI_ACPI diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile index 5196983220df6..29d7f6171b305 100644 --- a/drivers/pci/hotplug/Makefile +++ b/drivers/pci/hotplug/Makefile @@ -21,8 +21,9 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR) += rpadlpar_io.o obj-$(CONFIG_HOTPLUG_PCI_ACPI) += acpiphp.o obj-$(CONFIG_HOTPLUG_PCI_S390) += s390_pci_hpc.o -# acpiphp_ibm extends acpiphp, so should be linked afterwards. +# acpiphp_ibm extend acpiphp, so should be linked afterwards. +obj-$(CONFIG_HOTPLUG_PCI_ACPI_AMPERE_ALTRA) += acpiphp_ampere_altra.o obj-$(CONFIG_HOTPLUG_PCI_ACPI_IBM) += acpiphp_ibm.o pci_hotplug-objs := pci_hotplug_core.o diff --git a/drivers/pci/hotplug/acpiphp_ampere_altra.c b/drivers/pci/hotplug/acpiphp_ampere_altra.c new file mode 100644 index 0000000000000..8692b939dea78 --- /dev/null +++ b/drivers/pci/hotplug/acpiphp_ampere_altra.c @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ACPI PCI Hot Plug Ampere Altra Extension + * + * Copyright (C) 2023 Ampere Computing LLC + * + */ + +#define pr_fmt(fmt) "acpiphp_ampere_altra: " fmt + +#include <linux/init.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/pci_hotplug.h> + +#include "acpiphp.h" + +#define HANDLE_OPEN 0xb0200000 +#define HANDLE_CLOSE 0xb0300000 +#define REQUEST 0xf0700000 +#define LED_CMD 0x00000004 +#define LED_ATTENTION 0x00000002 +#define LED_SET_ON 0x00000001 +#define LED_SET_OFF 0x00000002 +#define LED_SET_BLINK 0x00000003 + +static const struct acpi_device_id acpi_ids[] = { + {"AMPC0008", 0}, {} +}; +MODULE_DEVICE_TABLE(acpi, acpi_ids); + +static u32 led_service_id[4]; + +static int led_status(u8 status) +{ + switch (status) { + case 1: return LED_SET_ON; + case 2: return LED_SET_BLINK; + default: return LED_SET_OFF; + } +} + +static int set_attention_status(struct hotplug_slot *slot, u8 status) +{ + struct arm_smccc_res res; + struct pci_bus *bus; + struct pci_dev *root_port; + unsigned long flags; + u32 handle; + int ret = 0; + + bus = slot->pci_slot->bus; + root_port = pcie_find_root_port(bus->self); + if (!root_port) + return -ENODEV; + + local_irq_save(flags); + arm_smccc_smc(HANDLE_OPEN, led_service_id[0], led_service_id[1], + led_service_id[2], led_service_id[3], 0, 0, 0, &res); + if (res.a0) { + ret = -ENODEV; + goto out; + } + handle = res.a1 & 0xffff0000; + + arm_smccc_smc(REQUEST, LED_CMD, led_status(status), LED_ATTENTION, + pci_domain_nr(bus) | ((root_port->devfn >> 3) << 4), 0, 0, + handle, &res); + if (res.a0) + ret = -ENODEV; + + arm_smccc_smc(HANDLE_CLOSE, handle, 0, 0, 0, 0, 0, 0, &res); + + out: + local_irq_restore(flags); + return ret; +} + +static int get_attention_status(struct hotplug_slot *slot, u8 *status) +{ + return -EINVAL; +} + +static struct acpiphp_attention_info ampere_altra_attn = { + .set_attn = set_attention_status, + .get_attn = get_attention_status, + .owner = THIS_MODULE, +}; + +static acpi_status __init get_acpi_handle(acpi_handle handle, u32 level, + void *context, void **return_value) +{ + *(acpi_handle *)return_value = handle; + return AE_CTRL_TERMINATE; +} + +static int __init acpiphp_ampere_altra_init(void) +{ + struct fwnode_handle *fwnode; + acpi_handle leds_handle = NULL; + struct acpi_device *leds; + acpi_status status; + int ret; + + status = acpi_get_devices("AMPC0008", get_acpi_handle, NULL, + &leds_handle); + if (ACPI_FAILURE(status) || !leds_handle) + return -ENODEV; + leds = acpi_get_acpi_dev(leds_handle); + if (!leds) { + pr_err("can't find device\n"); + return -ENODEV; + } + + fwnode = acpi_fwnode_handle(leds); + ret = fwnode_property_read_u32_array(fwnode, "uuid", led_service_id, 4); + acpi_put_acpi_dev(leds); + if (ret) { + pr_err("can't find uuid\n"); + return -ENODEV; + } + + if (acpiphp_register_attention(&ere_altra_attn)) { + pr_err("can't register driver\n"); + return -ENODEV; + } + + return 0; +} + +module_init(acpiphp_ampere_altra_init); + +static void __exit acpiphp_ampere_altra_exit(void) +{ + acpiphp_unregister_attention(&ere_altra_attn); +} + +module_exit(acpiphp_ampere_altra_exit); + +MODULE_AUTHOR("D Scott Phillips <scott@os.amperecomputing.com>"); +MODULE_LICENSE("GPL");
On Ampere Altra, PCIe hotplug is handled through ACPI. A side interface is also present to request system firmware control of attention LEDs. Add an ACPI PCI Hotplug companion driver to support attention LED control. Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com> --- drivers/pci/hotplug/Kconfig | 13 ++ drivers/pci/hotplug/Makefile | 3 +- drivers/pci/hotplug/acpiphp_ampere_altra.c | 141 +++++++++++++++++++++ 3 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 drivers/pci/hotplug/acpiphp_ampere_altra.c