| Message ID | 20230410184526.15990-16-blarson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
On Mon, Apr 10, 2023 at 9:48 PM Brad Larson <blarson@amd.com> 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. ... > +#include <linux/cdev.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/reset-controller.h> > +#include <linux/spi/spi.h> + Blank line? > +#include <linux/amd-pensando-ctrl.h> ... > +struct penctrl_device { > + struct spi_device *spi; > + struct reset_controller_dev rcdev; Try to swap them and check if the code will be smaller (it depends on how often one or another member is being used), > +}; ... > +static long > +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ > + void __user *in_arg = (void __user *)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; > + > + /* Check for a valid command */ > + if (_IOC_TYPE(cmd) != PENCTRL_IOC_MAGIC) > + return -ENOTTY; > + > + if (_IOC_NR(cmd) > PENCTRL_IOC_MAXNR) > + return -ENOTTY; > + > + if (_IOC_DIR(cmd) & _IOC_READ) > + ret = !access_ok(in_arg, _IOC_SIZE(cmd)); > + else if (_IOC_DIR(cmd) & _IOC_WRITE) > + ret = !access_ok(in_arg, _IOC_SIZE(cmd)); > + Unneeded blank line. > + if (ret) > + return -EFAULT; But it seems you can actually rewrite above in less lines: if ((_IOC_DIR(cmd) & _IOC_READ) && !access_ok(in_arg, _IOC_SIZE(cmd))) return -EFAULT; if ((_IOC_DIR(cmd) & _IOC_WRITE) && !access_ok(in_arg, _IOC_SIZE(cmd))) return -EFAULT; > + /* 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 (size == 0 || size % sizeof(struct penctrl_spi_xfer)) { > + ret = -EINVAL; > + goto done; > + } > + msg = memdup_user((struct penctrl_spi_xfer *)arg, size); > + if (!msg) { > + ret = PTR_ERR(msg); This is strange. > + goto done; > + } > + if (msg->len > PENCTRL_MAX_MSG_LEN) { > + ret = -EINVAL; > + goto done; > + } > + > + 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 done; > + } > + if (num_msgs > 1) { > + msg++; > + if (msg->len > PENCTRL_MAX_MSG_LEN) { > + ret = -EINVAL; > + goto done; > + } > + t[1].rx_buf = rx_buf; > + t[1].len = msg->len; > + } > + spi_message_init_with_transfers(&m, t, num_msgs); It seems there is no validation for the messages 3+. > + /* Perform the transfer */ > + mutex_lock(&spi_lock); > + ret = spi_sync(spi, &m); > + mutex_unlock(&spi_lock); > + > + if (ret || (num_msgs == 1)) > + goto done; > + > + if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len)) > + ret = -EFAULT; > +done: 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; > + if (!penctrl) > + return -ENODEV; Is it possible? > + filp->private_data = penctrl; > + current_cs = iminor(inode); > + spi = penctrl->spi; > + spi->chip_select = current_cs; > + spi->cs_gpiod = spi->controller->cs_gpiods[current_cs]; Hmm... Why do you need this one? Isn't it a job of SPI core? > + spi_setup(spi); > + return stream_open(inode, filp); > +} > +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 = 3; sizeof(txbuf) ? > + rxbuf[0] = 0; > + t[1].rx_buf = rxbuf; > + t[1].len = 1; sizeof(rxbuf) ? > + spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t)); > + ret = spi_sync(spi, &m); > + if (ret == 0) > + *val = rxbuf[0]; > + > + return ret; > +} > + > +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 = 4; 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->cs_gpiod = 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 done; > + } > + > + val |= BIT(6); > + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val); > + if (ret) > + dev_err(&spi->dev, "error writing ctrl0 reg\n"); > +done: 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->cs_gpiod = 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 done; > + } > + > + val &= ~BIT(6); > + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val); > + if (ret) > + dev_err(&spi->dev, "error writing ctrl0 reg\n"); > +done: out_unlock: ? > + mutex_unlock(&spi_lock); > + return ret; > +} > +static int penctrl_spi_probe(struct spi_device *spi) > +{ > + struct device *dev; > + struct cdev *cdev; > + u32 num_cs; > + int ret; > + u32 cs; > + > + ret = device_property_read_u32(spi->dev.parent, "num-cs", &num_cs); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "number of chip-selects not defined\n"); > + > + ret = alloc_chrdev_region(&penctrl_devt, 0, num_cs, "penctrl"); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "failed to alloc chrdev region\n"); > + > + penctrl_class = class_create(THIS_MODULE, "penctrl"); > + if (IS_ERR(penctrl_class)) { > + ret = dev_err_probe(&spi->dev, PTR_ERR(penctrl_class), > + "failed to create class\n"); > + goto unregister_chrdev; > + } > + > + cdev = cdev_alloc(); > + if (!cdev) { > + ret = dev_err_probe(&spi->dev, -ENOMEM, > + "allocation of cdev failed\n"); > + goto destroy_class; > + } > + cdev->owner = THIS_MODULE; > + cdev_init(cdev, &penctrl_fops); > + > + ret = cdev_add(cdev, penctrl_devt, num_cs); > + if (ret) { > + ret = dev_err_probe(&spi->dev, ret, > + "register of cdev failed\n"); > + goto free_cdev; > + } > + > + /* Allocate driver data */ > + penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL); > + if (!penctrl) { > + ret = -ENOMEM; > + goto free_cdev; > + } > + penctrl->spi = spi; > + mutex_init(&spi_lock); > + > + /* Create a device for each chip select */ > + for (cs = 0; cs < num_cs; cs++) { > + dev = device_create(penctrl_class, > + &spi->dev, > + MKDEV(MAJOR(penctrl_devt), cs), > + penctrl, > + "penctrl0.%d", > + cs); > + if (IS_ERR(dev)) { > + ret = dev_err_probe(&spi->dev, PTR_ERR(dev), > + "error creating device\n"); > + goto destroy_device; > + } > + dev_dbg(&spi->dev, "created device major %u, minor %d\n", > + MAJOR(penctrl_devt), cs); > + } > + > + /* Register emmc hardware reset */ > + penctrl->rcdev.nr_resets = 1; > + penctrl->rcdev.owner = THIS_MODULE; > + penctrl->rcdev.dev = &spi->dev; > + penctrl->rcdev.ops = &penctrl_reset_ops; > + penctrl->rcdev.of_node = spi->dev.of_node; Either redundant or wrong. Shouldn't you first have the firmware node to be set for spi->dev? > + device_set_node(&spi->dev, dev_fwnode(dev)); > + > + ret = reset_controller_register(&penctrl->rcdev); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "failed to register reset controller\n"); > + return 0; > + > +destroy_device: > + for (cs = 0; cs < num_cs; cs++) > + device_destroy(penctrl_class, MKDEV(MAJOR(penctrl_devt), cs)); > + kfree(penctrl); > +free_cdev: > + cdev_del(cdev); > +destroy_class: > + class_destroy(penctrl_class); > +unregister_chrdev: > + unregister_chrdev(MAJOR(penctrl_devt), "penctrl"); > + > + return ret; > +} ... > +++ b/include/uapi/linux/amd-pensando-ctrl.h > @@ -0,0 +1,30 @@ > +/* 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/ioctl.h> Not used header. > +#include <linux/types.h> > + > +#define PENCTRL_SPI_CMD_REGRD 0x0b > +#define PENCTRL_SPI_CMD_REGWR 0x02 > +#define PENCTRL_IOC_MAGIC 'k' > +#define PENCTRL_IOC_MAXNR 0 > +#define PENCTRL_MAX_MSG_LEN 16 > +#define PENCTRL_MAX_REG 0xff > +#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 */
Hi Andy, Thanks for the additional review. On Tue, Apr 11, 2023 at 12:20:43 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 10, 2023 at 9:48 PM Brad Larson <blarson@amd.com> 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. > > ... > >> +#include <linux/cdev.h> >> +#include <linux/device.h> >> +#include <linux/err.h> >> +#include <linux/init.h> >> +#include <linux/mod_devicetable.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/of.h> >> +#include <linux/reset-controller.h> >> +#include <linux/spi/spi.h> > > + Blank line? Added blank line >> +#include <linux/amd-pensando-ctrl.h> > > ... > >> +struct penctrl_device { >> + struct spi_device *spi; >> + struct reset_controller_dev rcdev; > > Try to swap them and check if the code will be smaller (it depends on > how often one or another member is being used), Reversed the order to reduced code size by 8 bytes. >> +}; > > ... > >> +static long >> +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) >> +{ >> + void __user *in_arg = (void __user *)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; >> + >> + /* Check for a valid command */ >> + if (_IOC_TYPE(cmd) != PENCTRL_IOC_MAGIC) >> + return -ENOTTY; >> + >> + if (_IOC_NR(cmd) > PENCTRL_IOC_MAXNR) >> + return -ENOTTY; >> + >> + if (_IOC_DIR(cmd) & _IOC_READ) >> + ret = !access_ok(in_arg, _IOC_SIZE(cmd)); >> + else if (_IOC_DIR(cmd) & _IOC_WRITE) >> + ret = !access_ok(in_arg, _IOC_SIZE(cmd)); > >> + > > Unneeded blank line. > >> + if (ret) >> + return -EFAULT; > > But it seems you can actually rewrite above in less lines: > > if ((_IOC_DIR(cmd) & _IOC_READ) && !access_ok(in_arg, _IOC_SIZE(cmd))) > return -EFAULT; > > if ((_IOC_DIR(cmd) & _IOC_WRITE) && !access_ok(in_arg, _IOC_SIZE(cmd))) > return -EFAULT; Yes, changed to save a line. >> + /* 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 (size == 0 || size % sizeof(struct penctrl_spi_xfer)) { >> + ret = -EINVAL; >> + goto done; >> + } >> + msg = memdup_user((struct penctrl_spi_xfer *)arg, size); > >> + if (!msg) { >> + ret = PTR_ERR(msg); > > This is strange. Yes, changed to msg = memdup_user((struct penctrl_spi_xfer *)arg, size); if (IS_ERR(msg)) { ret = PTR_ERR(msg); goto out_unlock; } >> + goto done; >> + } >> + if (msg->len > PENCTRL_MAX_MSG_LEN) { >> + ret = -EINVAL; >> + goto done; >> + } >> + >> + 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 done; >> + } >> + if (num_msgs > 1) { >> + msg++; >> + if (msg->len > PENCTRL_MAX_MSG_LEN) { >> + ret = -EINVAL; >> + goto done; >> + } >> + t[1].rx_buf = rx_buf; >> + t[1].len = msg->len; >> + } >> + spi_message_init_with_transfers(&m, t, num_msgs); > > It seems there is no validation for the messages 3+. The device doesn't support and applications don't use num_msgs > 2, added this check here /* 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; } >> + /* Perform the transfer */ >> + mutex_lock(&spi_lock); >> + ret = spi_sync(spi, &m); >> + mutex_unlock(&spi_lock); >> + >> + if (ret || (num_msgs == 1)) >> + goto done; >> + >> + if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len)) >> + ret = -EFAULT; > >> +done: > > out_unlock: ? Changed to 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; > >> + if (!penctrl) >> + return -ENODEV; > > Is it possible? No, removed as a non-existent device can't be opened. >> + filp->private_data = penctrl; >> + current_cs = iminor(inode); >> + spi = penctrl->spi; >> + spi->chip_select = current_cs; > >> + spi->cs_gpiod = spi->controller->cs_gpiods[current_cs]; > > Hmm... Why do you need this one? Isn't it a job of SPI core? When the four device tree nodes, one per cs, was squashed into the parent the SPI core no longer handles this and the driver needs to do it. >> + spi_setup(spi); >> + return stream_open(inode, filp); >> +} > >> +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 = 3; > > sizeof(txbuf) ? Changed to sizeof() >> + rxbuf[0] = 0; >> + t[1].rx_buf = rxbuf; >> + t[1].len = 1; > > sizeof(rxbuf) ? Changed to sizeof() >> + spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t)); >> + ret = spi_sync(spi, &m); >> + if (ret == 0) >> + *val = rxbuf[0]; >> + >> + return ret; >> +} >> + >> +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 = 4; > > sizeof(txbuf) ? Changed to sizeof() >> + 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->cs_gpiod = 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 done; >> + } >> + >> + val |= BIT(6); >> + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val); >> + if (ret) >> + dev_err(&spi->dev, "error writing ctrl0 reg\n"); > >> +done: > > out_unlock: ? Changed to 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->cs_gpiod = 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 done; >> + } >> + >> + val &= ~BIT(6); >> + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val); >> + if (ret) >> + dev_err(&spi->dev, "error writing ctrl0 reg\n"); > >> +done: > > out_unlock: ? Changed to out_unlock >> + mutex_unlock(&spi_lock); >> + return ret; >> +} > >> +static int penctrl_spi_probe(struct spi_device *spi) >> +{ >> + struct device *dev; >> + struct cdev *cdev; >> + u32 num_cs; >> + int ret; >> + u32 cs; >> + >> + ret = device_property_read_u32(spi->dev.parent, "num-cs", &num_cs); >> + if (ret) >> + return dev_err_probe(&spi->dev, ret, >> + "number of chip-selects not defined\n"); >> + >> + ret = alloc_chrdev_region(&penctrl_devt, 0, num_cs, "penctrl"); >> + if (ret) >> + return dev_err_probe(&spi->dev, ret, >> + "failed to alloc chrdev region\n"); >> + >> + penctrl_class = class_create(THIS_MODULE, "penctrl"); >> + if (IS_ERR(penctrl_class)) { >> + ret = dev_err_probe(&spi->dev, PTR_ERR(penctrl_class), >> + "failed to create class\n"); >> + goto unregister_chrdev; >> + } >> + >> + cdev = cdev_alloc(); >> + if (!cdev) { >> + ret = dev_err_probe(&spi->dev, -ENOMEM, >> + "allocation of cdev failed\n"); >> + goto destroy_class; >> + } >> + cdev->owner = THIS_MODULE; >> + cdev_init(cdev, &penctrl_fops); >> + >> + ret = cdev_add(cdev, penctrl_devt, num_cs); >> + if (ret) { >> + ret = dev_err_probe(&spi->dev, ret, >> + "register of cdev failed\n"); >> + goto free_cdev; >> + } >> + >> + /* Allocate driver data */ >> + penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL); >> + if (!penctrl) { >> + ret = -ENOMEM; >> + goto free_cdev; >> + } >> + penctrl->spi = spi; >> + mutex_init(&spi_lock); >> + >> + /* Create a device for each chip select */ >> + for (cs = 0; cs < num_cs; cs++) { >> + dev = device_create(penctrl_class, >> + &spi->dev, >> + MKDEV(MAJOR(penctrl_devt), cs), >> + penctrl, >> + "penctrl0.%d", >> + cs); >> + if (IS_ERR(dev)) { >> + ret = dev_err_probe(&spi->dev, PTR_ERR(dev), >> + "error creating device\n"); >> + goto destroy_device; >> + } >> + dev_dbg(&spi->dev, "created device major %u, minor %d\n", >> + MAJOR(penctrl_devt), cs); >> + } >> + >> + /* Register emmc hardware reset */ >> + penctrl->rcdev.nr_resets = 1; >> + penctrl->rcdev.owner = THIS_MODULE; >> + penctrl->rcdev.dev = &spi->dev; >> + penctrl->rcdev.ops = &penctrl_reset_ops; > >> + penctrl->rcdev.of_node = spi->dev.of_node; > > Either redundant or wrong. Shouldn't you first have the firmware node > to be set for spi->dev? The spi device firmware node is set on entry to penctrl_spi_probe(). Just the reset controller of_node needs to be set like this 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; ret = reset_controller_register(&penctrl->rcdev); which is similar to other reset controllers for example reset-sunplus.c: static int sp_reset_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; ... reset->rcdev.ops = &sp_reset_ops; reset->rcdev.owner = THIS_MODULE; reset->rcdev.of_node = dev->of_node; reset->rcdev.nr_resets = resource_size(res) / 4 * BITS_PER_HWM_REG; ret = devm_reset_controller_register(dev, &reset->rcdev); } for of_node at the same level as dev in reset_controller_dev struct reset_controller_dev { const struct reset_control_ops *ops; ... struct device *dev; struct device_node *of_node; ... }; >> + device_set_node(&spi->dev, dev_fwnode(dev)); >> + >> + ret = reset_controller_register(&penctrl->rcdev); >> + if (ret) >> + return dev_err_probe(&spi->dev, ret, >> + "failed to register reset controller\n"); >> + return 0; >> + >> +destroy_device: >> + for (cs = 0; cs < num_cs; cs++) >> + device_destroy(penctrl_class, MKDEV(MAJOR(penctrl_devt), cs)); >> + kfree(penctrl); >> +free_cdev: >> + cdev_del(cdev); >> +destroy_class: >> + class_destroy(penctrl_class); >> +unregister_chrdev: >> + unregister_chrdev(MAJOR(penctrl_devt), "penctrl"); >> + >> + return ret; >> +} > > ... > >> +++ b/include/uapi/linux/amd-pensando-ctrl.h >> @@ -0,0 +1,30 @@ >> +/* 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/ioctl.h> > > Not used header. Removed >> +#include <linux/types.h> >> + >> +#define PENCTRL_SPI_CMD_REGRD 0x0b >> +#define PENCTRL_SPI_CMD_REGWR 0x02 >> +#define PENCTRL_IOC_MAGIC 'k' >> +#define PENCTRL_IOC_MAXNR 0 >> +#define PENCTRL_MAX_MSG_LEN 16 >> +#define PENCTRL_MAX_REG 0xff >> +#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 */
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig index 4e176280113a..9e023f74e47c 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 3b0f9fb3b5c8..8914530f2721 100644 --- a/drivers/soc/Makefile +++ b/drivers/soc/Makefile @@ -4,6 +4,7 @@ # obj-$(CONFIG_ARCH_ACTIONS) += actions/ +obj-y += 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..6e4066564684 --- /dev/null +++ b/drivers/soc/amd/pensando-ctrl.c @@ -0,0 +1,373 @@ +// 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/init.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/reset-controller.h> +#include <linux/spi/spi.h> +#include <linux/amd-pensando-ctrl.h> + +struct penctrl_device { + struct spi_device *spi; + struct reset_controller_dev rcdev; +}; + +static DEFINE_MUTEX(spi_lock); +static dev_t penctrl_devt; +static struct penctrl_device *penctrl; +static struct class *penctrl_class; + +static long +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +{ + void __user *in_arg = (void __user *)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; + + /* Check for a valid command */ + if (_IOC_TYPE(cmd) != PENCTRL_IOC_MAGIC) + return -ENOTTY; + + if (_IOC_NR(cmd) > PENCTRL_IOC_MAXNR) + return -ENOTTY; + + if (_IOC_DIR(cmd) & _IOC_READ) + ret = !access_ok(in_arg, _IOC_SIZE(cmd)); + else if (_IOC_DIR(cmd) & _IOC_WRITE) + ret = !access_ok(in_arg, _IOC_SIZE(cmd)); + + if (ret) + return -EFAULT; + + /* 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 (size == 0 || size % sizeof(struct penctrl_spi_xfer)) { + ret = -EINVAL; + goto done; + } + msg = memdup_user((struct penctrl_spi_xfer *)arg, size); + if (!msg) { + ret = PTR_ERR(msg); + goto done; + } + if (msg->len > PENCTRL_MAX_MSG_LEN) { + ret = -EINVAL; + goto done; + } + + 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 done; + } + if (num_msgs > 1) { + msg++; + if (msg->len > PENCTRL_MAX_MSG_LEN) { + ret = -EINVAL; + goto done; + } + 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 done; + + if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len)) + ret = -EFAULT; + +done: + spi_dev_put(spi); + return ret; +} + +static int penctrl_open(struct inode *inode, struct file *filp) +{ + struct spi_device *spi; + u8 current_cs; + + if (!penctrl) + return -ENODEV; + + filp->private_data = penctrl; + current_cs = iminor(inode); + spi = penctrl->spi; + spi->chip_select = current_cs; + spi->cs_gpiod = 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 = 3; + + rxbuf[0] = 0; + t[1].rx_buf = rxbuf; + t[1].len = 1; + + spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t)); + ret = spi_sync(spi, &m); + if (ret == 0) + *val = rxbuf[0]; + + return ret; +} + +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 = 4; + 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->cs_gpiod = 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 done; + } + + val |= BIT(6); + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val); + if (ret) + dev_err(&spi->dev, "error writing ctrl0 reg\n"); + +done: + 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->cs_gpiod = 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 done; + } + + val &= ~BIT(6); + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val); + if (ret) + dev_err(&spi->dev, "error writing ctrl0 reg\n"); + +done: + mutex_unlock(&spi_lock); + return ret; +} + +static const struct reset_control_ops penctrl_reset_ops = { + .assert = penctrl_reset_assert, + .deassert = penctrl_reset_deassert, +}; + +static int penctrl_spi_probe(struct spi_device *spi) +{ + struct device *dev; + struct cdev *cdev; + u32 num_cs; + int ret; + u32 cs; + + ret = device_property_read_u32(spi->dev.parent, "num-cs", &num_cs); + if (ret) + return dev_err_probe(&spi->dev, ret, + "number of chip-selects not defined\n"); + + ret = alloc_chrdev_region(&penctrl_devt, 0, num_cs, "penctrl"); + if (ret) + return dev_err_probe(&spi->dev, ret, + "failed to alloc chrdev region\n"); + + penctrl_class = class_create(THIS_MODULE, "penctrl"); + if (IS_ERR(penctrl_class)) { + ret = dev_err_probe(&spi->dev, PTR_ERR(penctrl_class), + "failed to create class\n"); + goto unregister_chrdev; + } + + cdev = cdev_alloc(); + if (!cdev) { + ret = dev_err_probe(&spi->dev, -ENOMEM, + "allocation of cdev failed\n"); + goto destroy_class; + } + cdev->owner = THIS_MODULE; + cdev_init(cdev, &penctrl_fops); + + ret = cdev_add(cdev, penctrl_devt, num_cs); + if (ret) { + ret = dev_err_probe(&spi->dev, ret, + "register of cdev failed\n"); + goto free_cdev; + } + + /* Allocate driver data */ + penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL); + if (!penctrl) { + ret = -ENOMEM; + goto free_cdev; + } + penctrl->spi = spi; + mutex_init(&spi_lock); + + /* Create a device for each chip select */ + for (cs = 0; cs < num_cs; cs++) { + dev = device_create(penctrl_class, + &spi->dev, + MKDEV(MAJOR(penctrl_devt), cs), + penctrl, + "penctrl0.%d", + cs); + if (IS_ERR(dev)) { + ret = dev_err_probe(&spi->dev, PTR_ERR(dev), + "error creating device\n"); + goto destroy_device; + } + dev_dbg(&spi->dev, "created device major %u, minor %d\n", + MAJOR(penctrl_devt), cs); + } + + /* Register emmc hardware reset */ + penctrl->rcdev.nr_resets = 1; + penctrl->rcdev.owner = THIS_MODULE; + penctrl->rcdev.dev = &spi->dev; + penctrl->rcdev.ops = &penctrl_reset_ops; + penctrl->rcdev.of_node = spi->dev.of_node; + device_set_node(&spi->dev, dev_fwnode(dev)); + + ret = reset_controller_register(&penctrl->rcdev); + if (ret) + return dev_err_probe(&spi->dev, ret, + "failed to register reset controller\n"); + return 0; + +destroy_device: + for (cs = 0; cs < num_cs; cs++) + device_destroy(penctrl_class, MKDEV(MAJOR(penctrl_devt), cs)); + kfree(penctrl); +free_cdev: + cdev_del(cdev); +destroy_class: + class_destroy(penctrl_class); +unregister_chrdev: + unregister_chrdev(MAJOR(penctrl_devt), "penctrl"); + + 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-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..2508a1aef22c --- /dev/null +++ b/include/uapi/linux/amd-pensando-ctrl.h @@ -0,0 +1,30 @@ +/* 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/ioctl.h> +#include <linux/types.h> + +#define PENCTRL_SPI_CMD_REGRD 0x0b +#define PENCTRL_SPI_CMD_REGWR 0x02 +#define PENCTRL_IOC_MAGIC 'k' +#define PENCTRL_IOC_MAXNR 0 +#define PENCTRL_MAX_MSG_LEN 16 +#define PENCTRL_MAX_REG 0xff +#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> --- 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: Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/oe-kbuild-all/202303120925.SxLjwOd2-lkp@intel.com/ 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 | 373 +++++++++++++++++++++++++ include/uapi/linux/amd-pensando-ctrl.h | 30 ++ 6 files changed, 423 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