| Message ID | 20230913214944.59804-7-blarson@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
On Wed, Sep 13, 2023, at 17:49, Brad Larson wrote: > The Pensando SoC controller is a SPI connected companion device > that is present in all Pensando SoC board designs. The essential > board management registers are accessed on chip select 0 with > board mgmt IO support accessed using additional chip selects. > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > > v15 changes: > - Drop custom ioctl and use existing miscdevice interface. > - Delete unused definitions in amd-pensando-ctrl.h > - Makefile change to compile for ARCH_PENSANDO Hi Brad, I'm sorry I've been out of the loop for so long, and I hope we can find a way to manage your SoC support soon. My impression is that the normal support patches (1, 3, 4, and 5) are largely uncontroversial, while the SoC controller support seems like we are still not converging onto something that is ready to merge, so I would suggest you split the two parts and send the basic support for inclusion in linux-6.7 while we continue to discuss the soc controller driver. Please remove any references to the soc controller from the dts files and send that first series to:soc@kernel.org cc:linux-arm-kernel (and the other interested parties) so I can pick those up. Regarding the soc controller driver, let me try to give you my impression of where we are: - you have gone through 16 revisions already, which is way too much for a public review, we should have been able to find a solution earlier than that, and this is partly our fault on the reviewer side, and I'm sorry about that. - Andy's latest comments and a lot of the earlier reviews were focused on implementation details. While those comments are helpful suggestions for improving the code, they miss the larger point about the system design that I'm worried about and probably don't help you actually get it merged. - The main problem I still see is that this driver completely bypasses our normal kernel abstractions and instead creates a low-level passthrough interface for handling kernel functionality in userspace. This creates a liability both for the user ABI and the kernel implementation and prevents any - There is a chance that your design is in fact the best way to handle this particular hardware, but it is your job to write a convincing explanation of why this platform is different from all the others in the patch description. Your current one-paragraph text does not explain this at all. I would suggest you prioritize getting the other patches included for the moment, but we can keep discussion the API design for this driver either in this thread or on the #armlinux IRC channel (irc.libera.chat) in parallel if you like. In order to help you here, I would need either the documentation of the SPI software interface, or the source code for the userspace tool. Arnd
On Fri, Sep 22, 2023 at 1:24 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Sep 13, 2023, at 17:49, Brad Larson wrote: ... > > v15 changes: > > - Drop custom ioctl and use existing miscdevice interface. > > - Delete unused definitions in amd-pensando-ctrl.h > > - Makefile change to compile for ARCH_PENSANDO > > Hi Brad, > > I'm sorry I've been out of the loop for so long, and I hope > we can find a way to manage your SoC support soon. My impression > is that the normal support patches (1, 3, 4, and 5) are largely > uncontroversial, while the SoC controller support seems like > we are still not converging onto something that is ready to > merge, so I would suggest you split the two parts and send > the basic support for inclusion in linux-6.7 while we continue > to discuss the soc controller driver. > > Please remove any references to the soc controller from the > dts files and send that first series to:soc@kernel.org > cc:linux-arm-kernel (and the other interested parties) so > I can pick those up. > > Regarding the soc controller driver, let me try to give > you my impression of where we are: > > - you have gone through 16 revisions already, which is way > too much for a public review, we should have been able > to find a solution earlier than that, and this is partly > our fault on the reviewer side, and I'm sorry about that. > > - Andy's latest comments and a lot of the earlier reviews > were focused on implementation details. While those comments > are helpful suggestions for improving the code, they miss > the larger point about the system design that I'm worried > about and probably don't help you actually get it merged. True. The fact that the new versions left the design remaining make me think that the ABI was settled down. > - The main problem I still see is that this driver completely > bypasses our normal kernel abstractions and instead creates > a low-level passthrough interface for handling kernel > functionality in userspace. This creates a liability both > for the user ABI and the kernel implementation and prevents > any > > - There is a chance that your design is in fact the > best way to handle this particular hardware, but it is > your job to write a convincing explanation of why this > platform is different from all the others in the patch > description. Your current one-paragraph text does not > explain this at all. > > I would suggest you prioritize getting the other patches > included for the moment, but we can keep discussion the > API design for this driver either in this thread or on the > #armlinux IRC channel (irc.libera.chat) in parallel if you > like. In order to help you here, I would need either > the documentation of the SPI software interface, or the > source code for the userspace tool.
Hi Arnd, Thanks for the reply and guidance! On Fri, Sep 22, 2023, at 06:24:00 -0400, Arnd Bergmann wrote: > On Wed, Sep 13, 2023, at 17:49, Brad Larson wrote: >> The Pensando SoC controller is a SPI connected companion device >> that is present in all Pensando SoC board designs. The essential >> board management registers are accessed on chip select 0 with >> board mgmt IO support accessed using additional chip selects. >> >> Signed-off-by: Brad Larson <blarson@amd.com> >> --- >> >> v15 changes: >> - Drop custom ioctl and use existing miscdevice interface. >> - Delete unused definitions in amd-pensando-ctrl.h >> - Makefile change to compile for ARCH_PENSANDO > > Hi Brad, > > I'm sorry I've been out of the loop for so long, and I hope > we can find a way to manage your SoC support soon. My impression > is that the normal support patches (1, 3, 4, and 5) are largely > uncontroversial, while the SoC controller support seems like > we are still not converging onto something that is ready to > merge, so I would suggest you split the two parts and send > the basic support for inclusion in linux-6.7 while we continue > to discuss the soc controller driver. I've sent a patchset with only patches 1, 3, 4, and 5. Correctness is the priority for upstream inclusion. > Please remove any references to the soc controller from the > dts files and send that first series to:soc@kernel.org > cc:linux-arm-kernel (and the other interested parties) so > I can pick those up. Yes, I've removed the node for which the compatible driver is the problem. > > Regarding the soc controller driver, let me try to give > you my impression of where we are: > > - you have gone through 16 revisions already, which is way > too much for a public review, we should have been able > to find a solution earlier than that, and this is partly > our fault on the reviewer side, and I'm sorry about that. > > - Andy's latest comments and a lot of the earlier reviews > were focused on implementation details. While those comments > are helpful suggestions for improving the code, they miss > the larger point about the system design that I'm worried > about and probably don't help you actually get it merged. > > - The main problem I still see is that this driver completely > bypasses our normal kernel abstractions and instead creates > a low-level passthrough interface for handling kernel > functionality in userspace. This creates a liability both > for the user ABI and the kernel implementation and prevents > any > > - There is a chance that your design is in fact the > best way to handle this particular hardware, but it is > your job to write a convincing explanation of why this > platform is different from all the others in the patch > description. Your current one-paragraph text does not > explain this at all. > > I would suggest you prioritize getting the other patches > included for the moment, but we can keep discussion the > API design for this driver either in this thread or on the > #armlinux IRC channel (irc.libera.chat) in parallel if you > like. In order to help you here, I would need either > the documentation of the SPI software interface, or the > source code for the userspace tool. > > Arnd I'll redirect and reframe what the SoC driver is doing to #armlinux IRC to find an appropriate solution. Regards, Brad
On Tue, Sep 26, 2023, at 19:47, Brad Larson wrote: > On Fri, Sep 22, 2023, at 06:24:00 -0400, Arnd Bergmann wrote: >> On Wed, Sep 13, 2023, at 17:49, Brad Larson wrote: >> I'm sorry I've been out of the loop for so long, and I hope >> we can find a way to manage your SoC support soon. My impression >> is that the normal support patches (1, 3, 4, and 5) are largely >> uncontroversial, while the SoC controller support seems like >> we are still not converging onto something that is ready to >> merge, so I would suggest you split the two parts and send >> the basic support for inclusion in linux-6.7 while we continue >> to discuss the soc controller driver. > > I've sent a patchset with only patches 1, 3, 4, and 5. Correctness > is the priority for upstream inclusion. Ok, I took a look already and it all looks good. I should be able to pick it up into linux-next in the next few days when I open my branches for 6.7. > I'll redirect and reframe what the SoC driver is doing to > #armlinux IRC to find an appropriate solution. Ok. Arnd
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig index d21e75d69294..1a8a42141e78 100644 --- a/drivers/soc/Kconfig +++ b/drivers/soc/Kconfig @@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers" source "drivers/soc/actions/Kconfig" +source "drivers/soc/amd/Kconfig" source "drivers/soc/amlogic/Kconfig" source "drivers/soc/apple/Kconfig" source "drivers/soc/aspeed/Kconfig" diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile index 0706a27d13be..dbd651fcbecc 100644 --- a/drivers/soc/Makefile +++ b/drivers/soc/Makefile @@ -3,6 +3,7 @@ # Makefile for the Linux Kernel SOC specific device drivers. # +obj-$(CONFIG_ARCH_PENSANDO) += amd/ obj-y += apple/ obj-y += aspeed/ obj-$(CONFIG_ARCH_AT91) += atmel/ diff --git a/drivers/soc/amd/Kconfig b/drivers/soc/amd/Kconfig new file mode 100644 index 000000000000..011d5339d14e --- /dev/null +++ b/drivers/soc/amd/Kconfig @@ -0,0 +1,16 @@ +# SPDX-License-Identifier: GPL-2.0-only +menu "AMD Pensando SoC drivers" + +config AMD_PENSANDO_CTRL + tristate "AMD Pensando SoC Controller" + depends on SPI_MASTER=y + depends on (ARCH_PENSANDO && OF) || COMPILE_TEST + default ARCH_PENSANDO + select REGMAP_SPI + select MFD_SYSCON + help + Enables AMD Pensando SoC controller device support. This is a SPI + attached companion device in all Pensando SoC board designs which + provides essential board control/status registers and management IO + support. +endmenu diff --git a/drivers/soc/amd/Makefile b/drivers/soc/amd/Makefile new file mode 100644 index 000000000000..a2de0424f68d --- /dev/null +++ b/drivers/soc/amd/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0-only +obj-$(CONFIG_AMD_PENSANDO_CTRL) += pensando-ctrl.o diff --git a/drivers/soc/amd/pensando-ctrl.c b/drivers/soc/amd/pensando-ctrl.c new file mode 100644 index 000000000000..0b5a3a54d624 --- /dev/null +++ b/drivers/soc/amd/pensando-ctrl.c @@ -0,0 +1,311 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * AMD Pensando SoC Controller + * + * Userspace interface and reset driver support for SPI connected Pensando SoC + * controller device. This device is present in all Pensando SoC designs and + * contains board control/status registers and management IO support. + * + * Copyright 2023 Advanced Micro Devices, Inc. + */ + +#include <linux/cdev.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/fs.h> +#include <linux/init.h> +#include <linux/miscdevice.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/reset-controller.h> +#include <linux/spi/spi.h> +#include <linux/uaccess.h> + +#include <linux/amd-pensando-ctrl.h> + +struct penctrl_device { + struct reset_controller_dev rcdev; + struct spi_device *spi; +}; + +static struct penctrl_device *penctrl; +static DEFINE_MUTEX(spi_lock); + +static long +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +{ + struct penctrl_device *penctrl; + u8 tx_buf[PENCTRL_MAX_MSG_LEN]; + u8 rx_buf[PENCTRL_MAX_MSG_LEN]; + struct spi_transfer t[2] = {}; + struct penctrl_spi_xfer *msg; + struct spi_device *spi; + unsigned int num_msgs; + struct spi_message m; + u32 size; + int ret; + + /* Get a reference to the SPI device */ + penctrl = filp->private_data; + if (!penctrl) + return -ESHUTDOWN; + + spi = spi_dev_get(penctrl->spi); + if (!spi) + return -ESHUTDOWN; + + /* Verify and prepare SPI message */ + size = _IOC_SIZE(cmd); + num_msgs = size / sizeof(struct penctrl_spi_xfer); + if (num_msgs > 2 || size == 0 || size % sizeof(struct penctrl_spi_xfer)) { + ret = -EINVAL; + goto out_unlock; + } + msg = memdup_user((struct penctrl_spi_xfer *)arg, size); + if (IS_ERR(msg)) { + ret = PTR_ERR(msg); + goto out_unlock; + } + if (msg->len > PENCTRL_MAX_MSG_LEN) { + ret = -EINVAL; + goto out_unlock; + } + + t[0].tx_buf = tx_buf; + t[0].len = msg->len; + if (copy_from_user(tx_buf, (void __user *)msg->tx_buf, msg->len)) { + ret = -EFAULT; + goto out_unlock; + } + if (num_msgs > 1) { + msg++; + if (msg->len > PENCTRL_MAX_MSG_LEN) { + ret = -EINVAL; + goto out_unlock; + } + t[1].rx_buf = rx_buf; + t[1].len = msg->len; + } + spi_message_init_with_transfers(&m, t, num_msgs); + + /* Perform the transfer */ + mutex_lock(&spi_lock); + ret = spi_sync(spi, &m); + mutex_unlock(&spi_lock); + + if (ret || (num_msgs == 1)) + goto out_unlock; + + if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len)) + ret = -EFAULT; + +out_unlock: + spi_dev_put(spi); + return ret; +} + +static int penctrl_open(struct inode *inode, struct file *filp) +{ + struct spi_device *spi; + u8 current_cs; + + filp->private_data = penctrl; + current_cs = iminor(inode); + spi = penctrl->spi; + spi->chip_select = current_cs; + spi_set_csgpiod(spi, 0, spi->controller->cs_gpiods[current_cs]); + spi_setup(spi); + return stream_open(inode, filp); +} + +static int penctrl_release(struct inode *inode, struct file *filp) +{ + filp->private_data = NULL; + return 0; +} + +static const struct file_operations penctrl_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = penctrl_ioctl, + .open = penctrl_open, + .release = penctrl_release, + .llseek = no_llseek, +}; + +static int penctrl_regs_read(struct penctrl_device *penctrl, u32 reg, u32 *val) +{ + struct spi_device *spi = penctrl->spi; + struct spi_transfer t[2] = {}; + struct spi_message m; + u8 txbuf[3]; + u8 rxbuf[1]; + int ret; + + txbuf[0] = PENCTRL_SPI_CMD_REGRD; + txbuf[1] = reg; + txbuf[2] = 0; + t[0].tx_buf = txbuf; + t[0].len = sizeof(txbuf); + + rxbuf[0] = 0; + t[1].rx_buf = rxbuf; + t[1].len = sizeof(rxbuf); + + spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t)); + ret = spi_sync(spi, &m); + if (ret) + return ret; + + *val = rxbuf[0]; + return 0; +} + +static int penctrl_regs_write(struct penctrl_device *penctrl, u32 reg, u32 val) +{ + struct spi_device *spi = penctrl->spi; + struct spi_transfer t = {}; + struct spi_message m; + u8 txbuf[4]; + + txbuf[0] = PENCTRL_SPI_CMD_REGWR; + txbuf[1] = reg; + txbuf[2] = val; + txbuf[3] = 0; + + t.tx_buf = txbuf; + t.len = sizeof(txbuf); + spi_message_init_with_transfers(&m, &t, 1); + return spi_sync(spi, &m); +} + +static int penctrl_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct penctrl_device *penctrl = + container_of(rcdev, struct penctrl_device, rcdev); + struct spi_device *spi = penctrl->spi; + unsigned int val; + int ret; + + mutex_lock(&spi_lock); + spi->chip_select = 0; + spi_set_csgpiod(spi, 0, spi->controller->cs_gpiods[0]); + spi_setup(spi); + ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val); + if (ret) { + dev_err(&spi->dev, "error reading ctrl0 reg\n"); + goto out_unlock; + } + + val |= BIT(6); + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val); + if (ret) + dev_err(&spi->dev, "error writing ctrl0 reg\n"); + +out_unlock: + mutex_unlock(&spi_lock); + return ret; +} + +static int penctrl_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct penctrl_device *penctrl = + container_of(rcdev, struct penctrl_device, rcdev); + struct spi_device *spi = penctrl->spi; + unsigned int val; + int ret; + + mutex_lock(&spi_lock); + spi->chip_select = 0; + spi_set_csgpiod(spi, 0, spi->controller->cs_gpiods[0]); + spi_setup(spi); + ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val); + if (ret) { + dev_err(&spi->dev, "error reading ctrl0 reg\n"); + goto out_unlock; + } + + val &= ~BIT(6); + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val); + if (ret) + dev_err(&spi->dev, "error writing ctrl0 reg\n"); + +out_unlock: + mutex_unlock(&spi_lock); + return ret; +} + +static const struct reset_control_ops penctrl_reset_ops = { + .assert = penctrl_reset_assert, + .deassert = penctrl_reset_deassert, +}; + +static struct miscdevice penctrl_devices[] = { + { .minor = 0, .name = "penctrl.0", .fops = &penctrl_fops }, + { .minor = 1, .name = "penctrl.1", .fops = &penctrl_fops }, + { .minor = 2, .name = "penctrl.2", .fops = &penctrl_fops }, + { .minor = 3, .name = "penctrl.3", .fops = &penctrl_fops }, +}; + +static int penctrl_spi_probe(struct spi_device *spi) +{ + int i, ret; + + /* Allocate driver data */ + penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL); + if (!penctrl) + return -ENOMEM; + + penctrl->spi = spi; + mutex_init(&spi_lock); + + for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) { + ret = misc_register(&penctrl_devices[i]); + if (ret) { + dev_err(&spi->dev, "Failed to register device %s\n", + penctrl_devices[i].name); + goto cleanup; + } + } + + /* Register reset controller */ + penctrl->rcdev.dev = &spi->dev; + penctrl->rcdev.ops = &penctrl_reset_ops; + penctrl->rcdev.owner = THIS_MODULE; + penctrl->rcdev.of_node = spi->dev.of_node; + penctrl->rcdev.nr_resets = 1; + device_set_node(penctrl->rcdev.dev, dev_fwnode(&spi->dev)); + + ret = reset_controller_register(&penctrl->rcdev); + if (ret) + return dev_err_probe(&spi->dev, ret, + "failed to register reset controller\n"); + return 0; + +cleanup: + for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) { + if (penctrl_devices[i].this_device) + misc_deregister(&penctrl_devices[i]); + } + return ret; +} + +static const struct of_device_id penctrl_dt_match[] = { + { .compatible = "amd,pensando-elba-ctrl" }, + { /* sentinel */ } +}; + +static struct spi_driver penctrl_spi_driver = { + .probe = penctrl_spi_probe, + .driver = { + .name = "pensando-elba-ctrl", + .of_match_table = penctrl_dt_match, + }, +}; +module_spi_driver(penctrl_spi_driver); + +MODULE_AUTHOR("Brad Larson <blarson@amd.com>"); +MODULE_DESCRIPTION("AMD Pensando SoC Controller via SPI"); +MODULE_LICENSE("GPL"); diff --git a/include/uapi/linux/amd-pensando-ctrl.h b/include/uapi/linux/amd-pensando-ctrl.h new file mode 100644 index 000000000000..626b85e3fdb5 --- /dev/null +++ b/include/uapi/linux/amd-pensando-ctrl.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* + * Userspace interface for /dev/penctrl + * + * This file can be used by applications that need to communicate + * with the AMD Pensando SoC controller device via the ioctl interface. + */ +#ifndef _UAPI_LINUX_AMD_PENSANDO_CTRL_H +#define _UAPI_LINUX_AMD_PENSANDO_CTRL_H + +#include <linux/types.h> + +#define PENCTRL_SPI_CMD_REGRD 0x0b +#define PENCTRL_SPI_CMD_REGWR 0x02 +#define PENCTRL_MAX_MSG_LEN 16 +#define PENCTRL_REG_CTRL0 0x10 + +struct penctrl_spi_xfer { + __u64 tx_buf; + __u64 rx_buf; + __u32 len; + __u32 speed_hz; + __u64 compat; +}; + +#endif /* _UAPI_LINUX_AMD_PENSANDO_CTRL_H */
The Pensando SoC controller is a SPI connected companion device that is present in all Pensando SoC board designs. The essential board management registers are accessed on chip select 0 with board mgmt IO support accessed using additional chip selects. Signed-off-by: Brad Larson <blarson@amd.com> --- v15 changes: - Drop custom ioctl and use existing miscdevice interface. - Delete unused definitions in amd-pensando-ctrl.h - Makefile change to compile for ARCH_PENSANDO v14 changes: - Save 8 bytes of code size by swapping spi_device and reset_controller_dev in penctrl_device - Code simplification and clarity from review inputs - Set penctrl_spi_driver.driver.name to match compatible pensando-elba-ctrl - Remove unused include in amd-pensando-ctrl.h - Rebase to linux-next 6.4.0-rc1 class_create() API change v13 changes: - Update include list in pensando-ctrl.c - Change variable spi_dev to spi throughout - Removed unneeded variable initialization, simplification of error checks, remove extra castings, and use dev_err_probe() - Sort the includes in amd-pensando-ctrl.h - Updates to cleanup if there is an error in penctrl_spi_probe() v12 changes: - Fix gcc-12.1.0 warning v11 changes: - Fix the compatible to be specific 'amd,pensando-elba-ctrl' v10 changes: - Different driver implementation specific to this Pensando controller device. - Moved to soc/amd directory under new name based on guidance. This driver is of no use to any design other than all Pensando SoC based cards. - Removed use of builtin_driver, can be built as a module. v9 changes: - Previously patch 14/17 - After the change to the device tree node and squashing reset-cells into the parent simplified this to not use any MFD API and move it to drivers/spi/pensando-sr.c. - Change the naming to remove elba since this driver is common for all Pensando SoC designs . - Default yes SPI_PENSANDO_SR for ARCH_PENSANDO --- drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/amd/Kconfig | 16 ++ drivers/soc/amd/Makefile | 2 + drivers/soc/amd/pensando-ctrl.c | 311 +++++++++++++++++++++++++ include/uapi/linux/amd-pensando-ctrl.h | 26 +++ 6 files changed, 357 insertions(+) create mode 100644 drivers/soc/amd/Kconfig create mode 100644 drivers/soc/amd/Makefile create mode 100644 drivers/soc/amd/pensando-ctrl.c create mode 100644 include/uapi/linux/amd-pensando-ctrl.h