Message ID | 20230515181606.65953-1-blarson@amd.com |
---|---|
Headers | show |
Series | Support AMD Pensando Elba SoC | expand |
On Mon, May 15, 2023 at 9:18 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> Unneeded inclusion. > +#include <linux/reset-controller.h> > +#include <linux/spi/spi.h> ... > + u8 tx_buf[PENCTRL_MAX_MSG_LEN]; > + u8 rx_buf[PENCTRL_MAX_MSG_LEN]; Does it need to be DMA-capable? ... > + spi->chip_select = current_cs; > + spi->cs_gpiod = spi->controller->cs_gpiods[current_cs]; Nowadays these require API calls instead of direct assignments. ... > +static int penctrl_release(struct inode *inode, struct file *filp) > +{ > + filp->private_data = NULL; > + return 0; > +} Is it possible to unload the module without releasing the device node? ... > + u8 txbuf[3]; > + u8 rxbuf[1]; Same question about DMA. ... > + ret = spi_sync(spi, &m); > + if (ret == 0) > + *val = rxbuf[0]; > + > + return ret; Can also be written in more usual way: if (ret) return ret; ... return 0; ... > + u8 txbuf[4]; DMA? ... > + spi->chip_select = 0; > + spi->cs_gpiod = spi->controller->cs_gpiods[0]; Setter APIs. ... > + spi->chip_select = 0; > + spi->cs_gpiod = spi->controller->cs_gpiods[0]; Ditto. > + 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"); Hmm... Shouldn't SPI core take care of this in a generic way? Yes, I understand that you need the number for the allocation, but I would expect something like spi_fw_get_num_cs() to exist (seems not?). ... > + penctrl->rcdev.of_node = spi->dev.of_node; Use device_set_node(). It helps to modify the data types beneath. -- With Best Regards, Andy Shevchenko
> -----Original Message----- > From: Brad Larson <blarson@amd.com> > Sent: Monday, May 15, 2023 11:46 PM > To: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org; linux-mmc@vger.kernel.org; linux- > spi@vger.kernel.org; adrian.hunter@intel.com; alcooperx@gmail.com; > andy.shevchenko@gmail.com; arnd@arndb.de; Larson, Bradley > <Bradley.Larson@amd.com>; brendan.higgins@linux.dev; > briannorris@chromium.org; catalin.marinas@arm.com; > conor+dt@kernel.org; davidgow@google.com; gsomlo@gmail.com; > gerg@linux-m68k.org; hal.feng@starfivetech.com; hasegawa- > hitomi@fujitsu.com; j.neuschaefer@gmx.net; joel@jms.id.au; > kernel@esmil.dk; krzk@kernel.org; krzysztof.kozlowski+dt@linaro.org; > lee@kernel.org; lee.jones@linaro.org; broonie@kernel.org; > p.zabel@pengutronix.de; rdunlap@infradead.org; robh+dt@kernel.org; > samuel@sholland.org; fancer.lancer@gmail.com; > skhan@linuxfoundation.org; Suthikulpanit, Suravee > <Suravee.Suthikulpanit@amd.com>; Lendacky, Thomas > <Thomas.Lendacky@amd.com>; tonyhuang.sunplus@gmail.com; > ulf.hansson@linaro.org; vaishnav.a@ti.com; walker.chen@starfivetech.com; > will@kernel.org; zhuyinbo@loongson.cn; devicetree@vger.kernel.org > Subject: [PATCH v14 8/8] soc: amd: Add support for AMD Pensando SoC > Controller > > 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> > --- > > 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: > 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 | 368 +++++++++++++++++++++++++ > include/uapi/linux/amd-pensando-ctrl.h | 29 ++ > 6 files changed, 417 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 > > 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..a7ddd181dfe8 > --- /dev/null > +++ b/drivers/soc/amd/pensando-ctrl.c > @@ -0,0 +1,368 @@ > +// 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 reset_controller_dev rcdev; > + struct spi_device *spi; > +}; > + > +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)) && !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 (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; New set/get APIs for accessing spi->chip_select were introduced by https://github.com/torvalds/linux/commit/303feb3cc06ac0665d0ee9c1414941200e60e8a3 please use these APIs instead of accessing spi->chip_select dricetly. > + 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 = 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 == 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 = 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; Same here. > + 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 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; Same here. Regards, Amit > + 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 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 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("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 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; > + > + 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-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..e5f9f0dfe146 > --- /dev/null > +++ b/include/uapi/linux/amd-pensando-ctrl.h > @@ -0,0 +1,29 @@ > +/* 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_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 */ > -- > 2.17.1
On 15/05/2023 20:15, Brad Larson wrote: > This series enables support for AMD Pensando Elba SoC based platforms. > > The Elba SoC has the following features: > - Sixteen ARM64 A72 cores > - Dual DDR 4/5 memory controllers > - 32 lanes of PCIe Gen3/4 to the Host > - Network interfaces: Dual 200GE, Quad 100GE, 50GE, 25GE, 10GE and > also a single 1GE management port. > - Storage/crypto offloads and 144 programmable P4 cores. > - QSPI and EMMC for SoC storage > - Two SPI interfaces for peripheral management > - I2C bus for platform management > > == V14 changes == > Updated email list using get_maintainer.pl If you split the patches per subsystem (e.g. SPI), you would get more of them merged already. Best regards, Krzysztof
On 15/05/2023 20:16, Brad Larson wrote: > Add AMD Pensando common and Elba SoC specific device nodes > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 5/16/23 07:19, Mahapatra, Amit Kumar wrote: > > >> -----Original Message----- >> From: Brad Larson <blarson@amd.com> >> Sent: Monday, May 15, 2023 11:46 PM >> To: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org; linux-mmc@vger.kernel.org; linux- >> spi@vger.kernel.org; adrian.hunter@intel.com; alcooperx@gmail.com; >> andy.shevchenko@gmail.com; arnd@arndb.de; Larson, Bradley >> <Bradley.Larson@amd.com>; brendan.higgins@linux.dev; >> briannorris@chromium.org; catalin.marinas@arm.com; >> conor+dt@kernel.org; davidgow@google.com; gsomlo@gmail.com; >> gerg@linux-m68k.org; hal.feng@starfivetech.com; hasegawa- >> hitomi@fujitsu.com; j.neuschaefer@gmx.net; joel@jms.id.au; >> kernel@esmil.dk; krzk@kernel.org; krzysztof.kozlowski+dt@linaro.org; >> lee@kernel.org; lee.jones@linaro.org; broonie@kernel.org; >> p.zabel@pengutronix.de; rdunlap@infradead.org; robh+dt@kernel.org; >> samuel@sholland.org; fancer.lancer@gmail.com; >> skhan@linuxfoundation.org; Suthikulpanit, Suravee >> <Suravee.Suthikulpanit@amd.com>; Lendacky, Thomas >> <Thomas.Lendacky@amd.com>; tonyhuang.sunplus@gmail.com; >> ulf.hansson@linaro.org; vaishnav.a@ti.com; walker.chen@starfivetech.com; >> will@kernel.org; zhuyinbo@loongson.cn; devicetree@vger.kernel.org >> Subject: [PATCH v14 8/8] soc: amd: Add support for AMD Pensando SoC >> Controller >> >> 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> >> --- >> >> 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: >> 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 | 368 +++++++++++++++++++++++++ >> include/uapi/linux/amd-pensando-ctrl.h | 29 ++ >> 6 files changed, 417 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 >> >> 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..a7ddd181dfe8 >> --- /dev/null >> +++ b/drivers/soc/amd/pensando-ctrl.c >> @@ -0,0 +1,368 @@ >> +// 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 reset_controller_dev rcdev; >> + struct spi_device *spi; >> +}; >> + >> +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)) && !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 (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; > > New set/get APIs for accessing spi->chip_select were introduced by > https://github.com/torvalds/linux/commit/303feb3cc06ac0665d0ee9c1414941200e60e8a3 > please use these APIs instead of accessing spi->chip_select dricetly. > >> + 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 = 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 == 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 = 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; > > Same here. > >> + 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 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; > > Same here. > > Regards, > Amit > >> + 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 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 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("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 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; >> + >> + 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-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..e5f9f0dfe146 >> --- /dev/null >> +++ b/include/uapi/linux/amd-pensando-ctrl.h >> @@ -0,0 +1,29 @@ >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >> +/* >> + * Userspace interface for /dev/penctrl You are missing AMD copyright here. Also in DT patches I have seen that you didn't switch to 2023 year yet. Thanks, Michal
On 5/15/23 20:16, Brad Larson wrote: > Add AMD Pensando common and Elba SoC specific device nodes > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > > v14 changes: > - Fix dtbs_check l2-cache* property issue by adding required > cache-level and cache-unified properties > - Observed the issue after updating dtschema from 2023.1 to 2023.4 > and yamllint from 1.26.3 to 1.30.0 > > v11 changes: > - Delete reset-names > - Fix spi0 compatible to be specific 'amd,pensando-elba-ctrl' > > v9 changes: > - Single node for spi0 system-controller and squash > the reset-controller child into parent > > --- > arch/arm64/boot/dts/amd/Makefile | 1 + > arch/arm64/boot/dts/amd/elba-16core.dtsi | 197 ++++++++++++++++++ > arch/arm64/boot/dts/amd/elba-asic-common.dtsi | 80 +++++++ > arch/arm64/boot/dts/amd/elba-asic.dts | 28 +++ > arch/arm64/boot/dts/amd/elba-flash-parts.dtsi | 106 ++++++++++ > arch/arm64/boot/dts/amd/elba.dtsi | 191 +++++++++++++++++ > 6 files changed, 603 insertions(+) > create mode 100644 arch/arm64/boot/dts/amd/elba-16core.dtsi > create mode 100644 arch/arm64/boot/dts/amd/elba-asic-common.dtsi > create mode 100644 arch/arm64/boot/dts/amd/elba-asic.dts > create mode 100644 arch/arm64/boot/dts/amd/elba-flash-parts.dtsi > create mode 100644 arch/arm64/boot/dts/amd/elba.dtsi > > diff --git a/arch/arm64/boot/dts/amd/Makefile b/arch/arm64/boot/dts/amd/Makefile > index 68103a8b0ef5..8502cc2afbc5 100644 > --- a/arch/arm64/boot/dts/amd/Makefile > +++ b/arch/arm64/boot/dts/amd/Makefile > @@ -1,2 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0 > +dtb-$(CONFIG_ARCH_PENSANDO) += elba-asic.dtb > dtb-$(CONFIG_ARCH_SEATTLE) += amd-overdrive-rev-b0.dtb amd-overdrive-rev-b1.dtb > diff --git a/arch/arm64/boot/dts/amd/elba-16core.dtsi b/arch/arm64/boot/dts/amd/elba-16core.dtsi > new file mode 100644 > index 000000000000..f9f9f5fd5f69 > --- /dev/null > +++ b/arch/arm64/boot/dts/amd/elba-16core.dtsi > @@ -0,0 +1,197 @@ > +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) > +/* > + * Copyright 2020-2022 Advanced Micro Devices, Inc. 2023 and the same below. > + */ > + > +/ { > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + cpu-map { > + cluster0 { > + core0 { cpu = <&cpu0>; }; > + core1 { cpu = <&cpu1>; }; > + core2 { cpu = <&cpu2>; }; > + core3 { cpu = <&cpu3>; }; > + }; > + > + cluster1 { > + core0 { cpu = <&cpu4>; }; > + core1 { cpu = <&cpu5>; }; > + core2 { cpu = <&cpu6>; }; > + core3 { cpu = <&cpu7>; }; > + }; > + > + cluster2 { > + core0 { cpu = <&cpu8>; }; > + core1 { cpu = <&cpu9>; }; > + core2 { cpu = <&cpu10>; }; > + core3 { cpu = <&cpu11>; }; > + }; > + > + cluster3 { > + core0 { cpu = <&cpu12>; }; > + core1 { cpu = <&cpu13>; }; > + core2 { cpu = <&cpu14>; }; > + core3 { cpu = <&cpu15>; }; > + }; > + }; > + > + /* CLUSTER 0 */ > + cpu0: cpu@0 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x0>; Do you really need 2/0 split here. The first cell is 0 anyway. > + next-level-cache = <&l2_0>; > + enable-method = "psci"; > + }; > + > + cpu1: cpu@1 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x1>; > + next-level-cache = <&l2_0>; > + enable-method = "psci"; > + }; > + > + cpu2: cpu@2 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x2>; > + next-level-cache = <&l2_0>; > + enable-method = "psci"; > + }; > + > + cpu3: cpu@3 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x3>; > + next-level-cache = <&l2_0>; > + enable-method = "psci"; > + }; > + > + l2_0: l2-cache0 { > + compatible = "cache"; > + cache-unified; > + cache-level = <2>; > + }; > + > + /* CLUSTER 1 */ > + cpu4: cpu@100 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x100>; > + next-level-cache = <&l2_1>; > + enable-method = "psci"; > + }; > + > + cpu5: cpu@101 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x101>; > + next-level-cache = <&l2_1>; > + enable-method = "psci"; > + }; > + > + cpu6: cpu@102 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x102>; > + next-level-cache = <&l2_1>; > + enable-method = "psci"; > + }; > + > + cpu7: cpu@103 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x103>; > + next-level-cache = <&l2_1>; > + enable-method = "psci"; > + }; > + > + l2_1: l2-cache1 { > + compatible = "cache"; > + cache-unified; > + cache-level = <2>; > + }; > + > + /* CLUSTER 2 */ > + cpu8: cpu@200 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x200>; > + next-level-cache = <&l2_2>; > + enable-method = "psci"; > + }; > + > + cpu9: cpu@201 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x201>; > + next-level-cache = <&l2_2>; > + enable-method = "psci"; > + }; > + > + cpu10: cpu@202 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x202>; > + next-level-cache = <&l2_2>; > + enable-method = "psci"; > + }; > + > + cpu11: cpu@203 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x203>; > + next-level-cache = <&l2_2>; > + enable-method = "psci"; > + }; > + > + l2_2: l2-cache2 { > + compatible = "cache"; > + cache-unified; > + cache-level = <2>; > + }; > + > + /* CLUSTER 3 */ > + cpu12: cpu@300 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x300>; > + next-level-cache = <&l2_3>; > + enable-method = "psci"; > + }; > + > + cpu13: cpu@301 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x301>; > + next-level-cache = <&l2_3>; > + enable-method = "psci"; > + }; > + > + cpu14: cpu@302 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x302>; > + next-level-cache = <&l2_3>; > + enable-method = "psci"; > + }; > + > + cpu15: cpu@303 { > + device_type = "cpu"; > + compatible = "arm,cortex-a72"; > + reg = <0 0x303>; > + next-level-cache = <&l2_3>; > + enable-method = "psci"; > + }; > + > + l2_3: l2-cache3 { > + compatible = "cache"; > + cache-unified; > + cache-level = <2>; > + }; > + }; > +}; > diff --git a/arch/arm64/boot/dts/amd/elba-asic-common.dtsi b/arch/arm64/boot/dts/amd/elba-asic-common.dtsi > new file mode 100644 > index 000000000000..1a615788f54e > --- /dev/null > +++ b/arch/arm64/boot/dts/amd/elba-asic-common.dtsi > @@ -0,0 +1,80 @@ > +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) > +/* > + * Copyright 2020-2022 Advanced Micro Devices, Inc. > + */ > + > +&ahb_clk { > + clock-frequency = <400000000>; > +}; > + > +&emmc_clk { > + clock-frequency = <200000000>; > +}; > + > +&flash_clk { > + clock-frequency = <400000000>; > +}; > + > +&ref_clk { > + clock-frequency = <156250000>; > +}; > + > +&qspi { > + status = "okay"; > + > + flash0: flash@0 { > + compatible = "jedec,spi-nor"; > + reg = <0>; > + spi-max-frequency = <40000000>; > + spi-rx-bus-width = <2>; > + m25p,fast-read; > + cdns,read-delay = <0>; > + cdns,tshsl-ns = <0>; > + cdns,tsd2d-ns = <0>; > + cdns,tchsh-ns = <0>; > + cdns,tslch-ns = <0>; > + }; > +}; > + > +&gpio0 { > + status = "okay"; > +}; > + > +&emmc { > + bus-width = <8>; > + cap-mmc-hw-reset; > + resets = <&rstc 0>; > + status = "okay"; > +}; > + > +&wdt0 { > + status = "okay"; > +}; > + > +&i2c0 { > + clock-frequency = <100000>; > + status = "okay"; > + > + rtc@51 { > + compatible = "nxp,pcf85263"; > + reg = <0x51>; > + }; > +}; > + > +&spi0 { > + #address-cells = <1>; > + #size-cells = <0>; > + num-cs = <4>; > + cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, > + <&porta 7 GPIO_ACTIVE_LOW>; > + status = "okay"; > + > + rstc: system-controller@0 { > + compatible = "amd,pensando-elba-ctrl"; > + reg = <0>; > + spi-max-frequency = <12000000>; > + interrupt-parent = <&porta>; > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > + #reset-cells = <1>; > + }; > +}; > diff --git a/arch/arm64/boot/dts/amd/elba-asic.dts b/arch/arm64/boot/dts/amd/elba-asic.dts > new file mode 100644 > index 000000000000..c3f4da2f7449 > --- /dev/null > +++ b/arch/arm64/boot/dts/amd/elba-asic.dts > @@ -0,0 +1,28 @@ > +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) > +/* > + * Device Tree file for AMD Pensando Elba Board. > + * > + * Copyright 2020-2022 Advanced Micro Devices, Inc. > + */ > + > +/dts-v1/; > + > +#include "elba.dtsi" > +#include "elba-16core.dtsi" > +#include "elba-asic-common.dtsi" > +#include "elba-flash-parts.dtsi" > + > +/ { > + model = "AMD Pensando Elba Board"; > + compatible = "amd,pensando-elba-ortano", "amd,pensando-elba"; > + > + aliases { > + serial0 = &uart0; > + spi0 = &spi0; > + spi1 = &qspi; > + }; > + > + chosen { > + stdout-path = "serial0:115200n8"; > + }; > +}; > diff --git a/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi b/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi > new file mode 100644 > index 000000000000..734893fef2c3 > --- /dev/null > +++ b/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi > @@ -0,0 +1,106 @@ > +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) > +/* > + * Copyright 2020-2022 Advanced Micro Devices, Inc. > + */ > + > +&flash0 { > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + partition@0 { > + label = "flash"; > + reg = <0x10000 0xfff0000>; This doesn't fit with partition@0 above. Also size is weird. > + }; > + > + partition@f0000 { > + label = "golduenv"; > + reg = <0xf0000 0x10000>; > + }; > + > + partition@100000 { > + label = "boot0"; > + reg = <0x100000 0x80000>; > + }; > + > + partition@180000 { > + label = "golduboot"; > + reg = <0x180000 0x200000>; > + }; > + > + partition@380000 { > + label = "brdcfg0"; > + reg = <0x380000 0x10000>; > + }; > + > + partition@390000 { > + label = "brdcfg1"; > + reg = <0x390000 0x10000>; > + }; > + > + partition@400000 { > + label = "goldfw"; > + reg = <0x400000 0x3c00000>; This size looks weird. > + }; > + > + partition@4010000 { > + label = "fwmap"; > + reg = <0x4010000 0x20000>; > + }; > + > + partition@4030000 { > + label = "fwsel"; > + reg = <0x4030000 0x20000>; > + }; > + > + partition@4090000 { > + label = "bootlog"; > + reg = <0x4090000 0x20000>; > + }; > + > + partition@40b0000 { > + label = "panicbuf"; > + reg = <0x40b0000 0x20000>; > + }; > + > + partition@40d0000 { > + label = "uservars"; > + reg = <0x40d0000 0x20000>; > + }; > + > + partition@4200000 { > + label = "uboota"; > + reg = <0x4200000 0x400000>; > + }; > + > + partition@4600000 { > + label = "ubootb"; > + reg = <0x4600000 0x400000>; > + }; > + > + partition@4a00000 { > + label = "mainfwa"; > + reg = <0x4a00000 0x1000000>; > + }; > + > + partition@5a00000 { > + label = "mainfwb"; > + reg = <0x5a00000 0x1000000>; > + }; > + > + partition@6a00000 { > + label = "diaguboot"; > + reg = <0x6a00000 0x400000>; > + }; > + here is gap > + partition@8000000 { > + label = "diagfw"; > + reg = <0x8000000 0x7fe0000>; > + }; > + > + partition@ffe0000 { > + label = "ubootenv"; > + reg = <0xffe0000 0x10000>; > + }; And this is missing space description. Thanks, Michal
On Mon, May 15, 2023, at 20:16, 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> Hi Brad, I'm sorry I wasn't paying enough attention to this driver as the past 13 revisions went by. > 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. it looks like this was a fundamental change that I failed to see. > +# 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. So generally speaking, I don't want custom user interfaces in drivers/soc. It looks like this one has internal interfaces for a reset controller and the regmap, so those parts are fine, but I'm confused about the purpose of the ioctl interface: > +static long > +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ > + 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); This seems to be just a passthrough of user space messages, which is what the spidev driver already provides, but using a different ioctl interface. I don't really want any user-level interfaces in drivers/soc as a rule, but having one that duplicates existing functionality seems particularly wrong. Can you explain what the purpose is? Is this about serializing access between the in-kernel reset control and the user-side access? Also, can you explain why this needs a low-lever user interface in the first place, rather than something that can be expressed using high-level abstractions as you already do with the reset control? All of the above should be part of the changelog text to get a driver like this merged. I don't think we can get a quick solution here though, so maybe you can start by removing the ioctl side and having the rest of the driver in drivers/reset? Arnd
Hi Michal,
On Tue, May 16, 2023 at 9:42 AM Michal Simek <michal.simek@amd.com> wrote:
> Also in DT patches I have seen that you didn't switch to 2023 year yet.
If no substantial changes were made in 2023, there is no need nor
desire to update the copyright year.
Gr{oetje,eeting}s,
Geert
On Mon, 15 May 2023 11:15:58 -0700, Brad Larson wrote: > This series enables support for AMD Pensando Elba SoC based platforms. > > The Elba SoC has the following features: > - Sixteen ARM64 A72 cores > - Dual DDR 4/5 memory controllers > - 32 lanes of PCIe Gen3/4 to the Host > - Network interfaces: Dual 200GE, Quad 100GE, 50GE, 25GE, 10GE and > also a single 1GE management port. > - Storage/crypto offloads and 144 programmable P4 cores. > - QSPI and EMMC for SoC storage > - Two SPI interfaces for peripheral management > - I2C bus for platform management > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [2/8] dt-bindings: spi: cdns: Add compatible for AMD Pensando Elba SoC commit: f2156989bf3014c67707d73ccd202b2ada09080b [7/8] spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC commit: f5c2f9f9584353bc816d76a65c97dd03dc61678c All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Andy, On Tue, May 16, 2023 at 00:05:32 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, May 15, 2023 at 9:18 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> > > Unneeded inclusion. Removed >> +#include <linux/reset-controller.h> >> +#include <linux/spi/spi.h> > > ... > > >> + u8 tx_buf[PENCTRL_MAX_MSG_LEN]; >> + u8 rx_buf[PENCTRL_MAX_MSG_LEN]; > > Does it need to be DMA-capable? Doesn't need to be DMA-capable > ... > >> + spi->chip_select = current_cs; >> + spi->cs_gpiod = spi->controller->cs_gpiods[current_cs]; > > Nowadays these require API calls instead of direct assignments. Changed to: spi_set_csgpiod(spi, 0, spi->controller->cs_gpiods[current_cs]); > ... > >> +static int penctrl_release(struct inode *inode, struct file *filp) >> +{ >> + filp->private_data = NULL; >> + return 0; >> +} > > Is it possible to unload the module without releasing the device node? If the refcount is not zero the kernel prevents the module from being unloaded. > ... > >> + u8 txbuf[3]; >> + u8 rxbuf[1]; > > Same question about DMA. Not DMA-capable > ... > >> + ret = spi_sync(spi, &m); > >> + if (ret == 0) >> + *val = rxbuf[0]; >> + >> + return ret; > > Can also be written in more usual way: > > if (ret) > return ret; > ... > return 0; Yes, changed to: ret = spi_sync(spi, &m); if (ret) return ret; *val = rxbuf[0]; return 0; > ... > >> + u8 txbuf[4]; > > DMA? Not DMA-capable > ... > >> + spi->chip_select = 0; >> + spi->cs_gpiod = spi->controller->cs_gpiods[0]; > > Setter APIs. Changed to: spi_set_csgpiod(spi, 0, spi->controller->cs_gpiods[0]); > > ... > >> + spi->chip_select = 0; >> + spi->cs_gpiod = spi->controller->cs_gpiods[0]; > > Ditto. Changed to: spi_set_csgpiod(spi, 0, spi->controller->cs_gpiods[0]); >> + 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"); > > Hmm... Shouldn't SPI core take care of this in a generic way? Yes, I > understand that you need the number for the allocation, but I would > expect something like spi_fw_get_num_cs() to exist (seems not?). > No need to look into the parent node, changed to this: num_cs = spi->controller->num_chipselect; > ... > >> + penctrl->rcdev.of_node = spi->dev.of_node; > > Use device_set_node(). It helps to modify the data types beneath. Added: device_set_node(penctrl->rcdev.dev, dev_fwnode(&spi->dev)); Regards, Brad
Hi Michal, Thanks for reviewing the patch. On 5/16/23 09:54, Michal Simek wrote: > On 5/15/23 20:16, Brad Larson wrote: >> Add AMD Pensando common and Elba SoC specific device nodes >> >> Signed-off-by: Brad Larson <blarson@amd.com> >> --- >> >> v14 changes: >> - Fix dtbs_check l2-cache* property issue by adding required >> cache-level and cache-unified properties >> - Observed the issue after updating dtschema from 2023.1 to 2023.4 >> and yamllint from 1.26.3 to 1.30.0 >> >> v11 changes: >> - Delete reset-names >> - Fix spi0 compatible to be specific 'amd,pensando-elba-ctrl' >> >> v9 changes: >> - Single node for spi0 system-controller and squash >> the reset-controller child into parent >> >> --- >> arch/arm64/boot/dts/amd/Makefile | 1 + >> arch/arm64/boot/dts/amd/elba-16core.dtsi | 197 ++++++++++++++++++ >> arch/arm64/boot/dts/amd/elba-asic-common.dtsi | 80 +++++++ >> arch/arm64/boot/dts/amd/elba-asic.dts | 28 +++ >> arch/arm64/boot/dts/amd/elba-flash-parts.dtsi | 106 ++++++++++ >> arch/arm64/boot/dts/amd/elba.dtsi | 191 +++++++++++++++++ >> 6 files changed, 603 insertions(+) >> create mode 100644 arch/arm64/boot/dts/amd/elba-16core.dtsi >> create mode 100644 arch/arm64/boot/dts/amd/elba-asic-common.dtsi >> create mode 100644 arch/arm64/boot/dts/amd/elba-asic.dts >> create mode 100644 arch/arm64/boot/dts/amd/elba-flash-parts.dtsi >> create mode 100644 arch/arm64/boot/dts/amd/elba.dtsi >> >> diff --git a/arch/arm64/boot/dts/amd/Makefile b/arch/arm64/boot/dts/amd/Makefile >> index 68103a8b0ef5..8502cc2afbc5 100644 >> --- a/arch/arm64/boot/dts/amd/Makefile >> +++ b/arch/arm64/boot/dts/amd/Makefile >> @@ -1,2 +1,3 @@ >> # SPDX-License-Identifier: GPL-2.0 >> +dtb-$(CONFIG_ARCH_PENSANDO) += elba-asic.dtb >> dtb-$(CONFIG_ARCH_SEATTLE) += amd-overdrive-rev-b0.dtb amd-overdrive-rev-b1.dtb >> diff --git a/arch/arm64/boot/dts/amd/elba-16core.dtsi b/arch/arm64/boot/dts/amd/elba-16core.dtsi >> new file mode 100644 >> index 000000000000..f9f9f5fd5f69 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/amd/elba-16core.dtsi >> @@ -0,0 +1,197 @@ >> +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) >> +/* >> + * Copyright 2020-2022 Advanced Micro Devices, Inc. > > 2023 and the same below. I'll update the copyright in the next submit >> + */ >> + >> +/ { >> + cpus { >> + #address-cells = <2>; >> + #size-cells = <0>; >> + >> + cpu-map { >> + cluster0 { >> + core0 { cpu = <&cpu0>; }; >> + core1 { cpu = <&cpu1>; }; >> + core2 { cpu = <&cpu2>; }; >> + core3 { cpu = <&cpu3>; }; >> + }; >> + >> + cluster1 { >> + core0 { cpu = <&cpu4>; }; >> + core1 { cpu = <&cpu5>; }; >> + core2 { cpu = <&cpu6>; }; >> + core3 { cpu = <&cpu7>; }; >> + }; >> + >> + cluster2 { >> + core0 { cpu = <&cpu8>; }; >> + core1 { cpu = <&cpu9>; }; >> + core2 { cpu = <&cpu10>; }; >> + core3 { cpu = <&cpu11>; }; >> + }; >> + >> + cluster3 { >> + core0 { cpu = <&cpu12>; }; >> + core1 { cpu = <&cpu13>; }; >> + core2 { cpu = <&cpu14>; }; >> + core3 { cpu = <&cpu15>; }; >> + }; >> + }; >> + >> + /* CLUSTER 0 */ >> + cpu0: cpu@0 { >> + device_type = "cpu"; >> + compatible = "arm,cortex-a72"; >> + reg = <0 0x0>; > > Do you really need 2/0 split here. The first cell is 0 anyway. Yes following 64-bit system definition ... >> diff --git a/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi b/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi >> new file mode 100644 >> index 000000000000..734893fef2c3 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi >> @@ -0,0 +1,106 @@ >> +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) >> +/* >> + * Copyright 2020-2022 Advanced Micro Devices, Inc. >> + */ >> + >> +&flash0 { 0xf0000>> + partitions { >> + compatible = "fixed-partitions"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + partition@0 { >> + label = "flash"; >> + reg = <0x10000 0xfff0000>; > > This doesn't fit with partition@0 above. > Also size is weird. This is intended to not expose sector 0. >> + }; >> + >> + partition@f0000 { >> + label = "golduenv"; >> + reg = <0xf0000 0x10000>; >> + }; >> + >> + partition@100000 { >> + label = "boot0"; >> + reg = <0x100000 0x80000>; >> + }; >> + >> + partition@180000 { >> + label = "golduboot"; >> + reg = <0x180000 0x200000>; >> + }; >> + >> + partition@380000 { >> + label = "brdcfg0"; >> + reg = <0x380000 0x10000>; >> + }; >> + >> + partition@390000 { >> + label = "brdcfg1"; >> + reg = <0x390000 0x10000>; >> + }; >> + >> + partition@400000 { >> + label = "goldfw"; >> + reg = <0x400000 0x3c00000>; > > This size looks weird. It's the allocated size for this firmware component. >> + }; >> + >> + partition@4010000 { >> + label = "fwmap"; >> + reg = <0x4010000 0x20000>; >> + }; >> + >> + partition@4030000 { >> + label = "fwsel"; >> + reg = <0x4030000 0x20000>; >> + }; >> + >> + partition@4090000 { >> + label = "bootlog"; >> + reg = <0x4090000 0x20000>; >> + }; >> + >> + partition@40b0000 { >> + label = "panicbuf"; >> + reg = <0x40b0000 0x20000>; >> + }; >> + >> + partition@40d0000 { >> + label = "uservars"; >> + reg = <0x40d0000 0x20000>; >> + }; >> + >> + partition@4200000 { >> + label = "uboota"; >> + reg = <0x4200000 0x400000>; >> + }; >> + >> + partition@4600000 { >> + label = "ubootb"; >> + reg = <0x4600000 0x400000>; >> + }; >> + >> + partition@4a00000 { >> + label = "mainfwa"; >> + reg = <0x4a00000 0x1000000>; >> + }; >> + >> + partition@5a00000 { >> + label = "mainfwb"; >> + reg = <0x5a00000 0x1000000>; >> + }; >> + >> + partition@6a00000 { >> + label = "diaguboot"; >> + reg = <0x6a00000 0x400000>; >> + }; >> + > > here is gap This is intentional for unallocated space. I'll put in a 'spare' partition. >> + partition@8000000 { >> + label = "diagfw"; >> + reg = <0x8000000 0x7fe0000>; >> + }; >> + >> + partition@ffe0000 { >> + label = "ubootenv"; >> + reg = <0xffe0000 0x10000>; >> + }; > > And this is missing space description. space description? Regards, Brad
Hi Arnd, > On Mon, May 15, 2023, at 20:16, 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> > > Hi Brad, > > I'm sorry I wasn't paying enough attention to this driver as the > past 13 revisions went by. > No worries, bit of a saga. See explanation below. >> 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. > > it looks like this was a fundamental change that I failed to see. See explanation below. >> +# 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. > > So generally speaking, I don't want custom user interfaces in > drivers/soc. It looks like this one has internal interfaces for > a reset controller and the regmap, so those parts are fine, but > I'm confused about the purpose of the ioctl interface: > >> +static long >> +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) >> +{ > >> + 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); > > This seems to be just a passthrough of user space messages, which > is what the spidev driver already provides, but using a different > ioctl interface. I don't really want any user-level interfaces > in drivers/soc as a rule, but having one that duplicates existing > functionality seems particularly wrong. > > Can you explain what the purpose is? Is this about serializing > access between the in-kernel reset control and the user-side > access? > > Also, can you explain why this needs a low-lever user interface > in the first place, rather than something that can be expressed > using high-level abstractions as you already do with the reset > control? > > All of the above should be part of the changelog text to get a > driver like this merged. I don't think we can get a quick > solution here though, so maybe you can start by removing the > ioctl side and having the rest of the driver in drivers/reset? In the original patchset I added a pensando compatible to spidev and that was nacked in review and reusing some random compatible that made it into spidev was just wrong. Further it was recommended this should be a system specific driver and don't rely on a debug driver like spidev. I changed over to a platform specific driver and at that time I also needed to include a reset controller (emmc reset only). I put these in drivers/mfd and drivers/reset. Review of the device tree for this approach went back and forth to _not_ have four child nodes on the spi device each with the same compatible. Decision was to squash the child nodes into the parent and put the reset-controller there also. One driver and since its pensando specific its currently in drivers/soc/amd. There are five different user processes and some utilities that access the functionality in the cpld/fpga. You're correct, its passing messages that are specific to the IP accessed via chip-select. No Elba system will boot without this driver providing ioctl access. Regards, Brad
Hi Brad, On Tue, May 23, 2023 at 9:30 PM Brad Larson <blarson@amd.com> wrote: > On 5/16/23 09:54, Michal Simek wrote: > > On 5/15/23 20:16, Brad Larson wrote: > >> --- /dev/null > >> +++ b/arch/arm64/boot/dts/amd/elba-16core.dtsi > >> @@ -0,0 +1,197 @@ > >> +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) > >> +/* > >> + * Copyright 2020-2022 Advanced Micro Devices, Inc. > > > > 2023 and the same below. > > I'll update the copyright in the next submit Did you make any substantial changes in 2023? > >> + */ > >> + > >> +/ { > >> + cpus { > >> + #address-cells = <2>; > >> + #size-cells = <0>; > >> + > >> + cpu-map { > >> + cluster0 { > >> + core0 { cpu = <&cpu0>; }; > >> + core1 { cpu = <&cpu1>; }; > >> + core2 { cpu = <&cpu2>; }; > >> + core3 { cpu = <&cpu3>; }; > >> + }; > >> + > >> + cluster1 { > >> + core0 { cpu = <&cpu4>; }; > >> + core1 { cpu = <&cpu5>; }; > >> + core2 { cpu = <&cpu6>; }; > >> + core3 { cpu = <&cpu7>; }; > >> + }; > >> + > >> + cluster2 { > >> + core0 { cpu = <&cpu8>; }; > >> + core1 { cpu = <&cpu9>; }; > >> + core2 { cpu = <&cpu10>; }; > >> + core3 { cpu = <&cpu11>; }; > >> + }; > >> + > >> + cluster3 { > >> + core0 { cpu = <&cpu12>; }; > >> + core1 { cpu = <&cpu13>; }; > >> + core2 { cpu = <&cpu14>; }; > >> + core3 { cpu = <&cpu15>; }; > >> + }; > >> + }; > >> + > >> + /* CLUSTER 0 */ > >> + cpu0: cpu@0 { > >> + device_type = "cpu"; > >> + compatible = "arm,cortex-a72"; > >> + reg = <0 0x0>; > > > > Do you really need 2/0 split here. The first cell is 0 anyway. > > Yes following 64-bit system definition You mean for the 64-bit main address space? The CPU address space under /cpus is unrelated. > >> +++ b/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi > >> @@ -0,0 +1,106 @@ > >> +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) > >> +/* > >> + * Copyright 2020-2022 Advanced Micro Devices, Inc. > >> + */ > >> + > >> +&flash0 { > 0xf0000>> + partitions { > >> + compatible = "fixed-partitions"; > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + partition@0 { > >> + label = "flash"; > >> + reg = <0x10000 0xfff0000>; > > > > This doesn't fit with partition@0 above. > > Also size is weird. > > This is intended to not expose sector 0. The unit address should still match the first reg entry => partition@10000. Gr{oetje,eeting}s, Geert
On Wed, May 24, 2023, at 00:11, Brad Larson wrote: >> On Mon, May 15, 2023, at 20:16, Brad Larson wrote: > >> Also, can you explain why this needs a low-lever user interface >> in the first place, rather than something that can be expressed >> using high-level abstractions as you already do with the reset >> control? >> >> All of the above should be part of the changelog text to get a >> driver like this merged. I don't think we can get a quick >> solution here though, so maybe you can start by removing the >> ioctl side and having the rest of the driver in drivers/reset? > > In the original patchset I added a pensando compatible to spidev and that > was nacked in review and reusing some random compatible that made it into > spidev was just wrong. Further it was recommended this should be a system > specific driver and don't rely on a debug driver like spidev. I changed > over to a platform specific driver and at that time I also needed to include > a reset controller (emmc reset only). I put these in drivers/mfd and > drivers/reset. Review of the device tree for this approach went back and > forth to _not_ have four child nodes on the spi device each with the same > compatible. Decision was to squash the child nodes into the parent and put > the reset-controller there also. One driver and since its pensando > specific its currently in drivers/soc/amd. > > There are five different user processes and some utilities that access the > functionality in the cpld/fpga. You're correct, its passing messages that > are specific to the IP accessed via chip-select. No Elba system will boot > without this driver providing ioctl access. Thank you for the detailed summary. Moving away from spidev and from mfd seems all reasonable here. I'm still a bit confused by why you have multiple chipselects here that are for different subdevices but ended with a single user interface for all of them, but that's not a big deal. The main bit that sticks out about this high-level design is how it relies on user space utilities at all to understand the message format. From what I understand about the actual functionality of this device, it most closely resembles an embedded controller that you might find in a laptop or server machine, and those usually have kernel drivers in drivers/platform/ to interact with the device. Has anyone tried to do it like that? Maybe it would help to see what the full protocol and the user space side looks like, in order to move some or all of it into the kernel. Arnd
Hi Geert, On Wed, May 24, 2023 at 13:52 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, May 23, 2023 at 9:30â¯PM Brad Larson <blarson@amd.com> wrote: >> On 5/16/23 09:54, Michal Simek wrote: >> > On 5/15/23 20:16, Brad Larson wrote: >> >> --- /dev/null >> >> +++ b/arch/arm64/boot/dts/amd/elba-16core.dtsi >> >> @@ -0,0 +1,197 @@ >> >> +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) >> >> +/* >> >> + * Copyright 2020-2022 Advanced Micro Devices, Inc. >> > >> > 2023 and the same below. >> >> I'll update the copyright in the next submit > > Did you make any substantial changes in 2023? Yes, additional properties were added to l2-cache* >> >> + */ >> >> + >> >> +/ { >> >> + cpus { >> >> + #address-cells = <2>; >> >> + #size-cells = <0>; >> >> + >> >> + cpu-map { >> >> + cluster0 { >> >> + core0 { cpu = <&cpu0>; }; >> >> + core1 { cpu = <&cpu1>; }; >> >> + core2 { cpu = <&cpu2>; }; >> >> + core3 { cpu = <&cpu3>; }; >> >> + }; >> >> + >> >> + cluster1 { >> >> + core0 { cpu = <&cpu4>; }; >> >> + core1 { cpu = <&cpu5>; }; >> >> + core2 { cpu = <&cpu6>; }; >> >> + core3 { cpu = <&cpu7>; }; >> >> + }; >> >> + >> >> + cluster2 { >> >> + core0 { cpu = <&cpu8>; }; >> >> + core1 { cpu = <&cpu9>; }; >> >> + core2 { cpu = <&cpu10>; }; >> >> + core3 { cpu = <&cpu11>; }; >> >> + }; >> >> + >> >> + cluster3 { >> >> + core0 { cpu = <&cpu12>; }; >> >> + core1 { cpu = <&cpu13>; }; >> >> + core2 { cpu = <&cpu14>; }; >> >> + core3 { cpu = <&cpu15>; }; >> >> + }; >> >> + }; >> >> + >> >> + /* CLUSTER 0 */ >> >> + cpu0: cpu@0 { >> >> + device_type = "cpu"; >> >> + compatible = "arm,cortex-a72"; >> >> + reg = <0 0x0>; >> > >> > Do you really need 2/0 split here. The first cell is 0 anyway. >> >> Yes following 64-bit system definition > > You mean for the 64-bit main address space? > The CPU address space under /cpus is unrelated. Yes, the reg prop for this node is CPU/threads per dt spec. Checked the history and the Elba dt was derived from socionext for these nodes and this is how those device trees are configured along with over a dozen other devices. I changed to address-cells = <1> and dropped the leading zero from all cpu* reg<> and booting the system I'm observing no change. Looking in drivers/of I'm not seeing where cpu*/reg is read and used, any recommendation? >> >> +++ b/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi >> >> @@ -0,0 +1,106 @@ >> >> +// SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) >> >> +/* >> >> + * Copyright 2020-2022 Advanced Micro Devices, Inc. >> >> + */ >> >> + >> >> +&flash0 { >> 0xf0000>> + partitions { >> >> + compatible = "fixed-partitions"; >> >> + #address-cells = <1>; >> >> + #size-cells = <1>; >> >> + partition@0 { >> >> + label = "flash"; >> >> + reg = <0x10000 0xfff0000>; >> > >> > This doesn't fit with partition@0 above. >> > Also size is weird. >> >> This is intended to not expose sector 0. > > The unit address should still match the first reg entry > => partition@10000. Changed to this: partition@0 { label = "rsvd"; reg = <0x0 0x10000>; read-only; }; partition@10000 { label = "flash"; reg = <0x10000 0xfff0000>; }; Regards, Brad
Hi Brad, On Wed, May 31, 2023 at 12:04 AM Brad Larson <blarson@amd.com> wrote: > On Wed, May 24, 2023 at 13:52 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, May 23, 2023 at 9:30 PM Brad Larson <blarson@amd.com> wrote: > >> On 5/16/23 09:54, Michal Simek wrote: > >> > On 5/15/23 20:16, Brad Larson wrote: > >> >> + */ > >> >> + > >> >> +/ { > >> >> + cpus { > >> >> + #address-cells = <2>; > >> >> + #size-cells = <0>; > >> >> + > >> >> + cpu-map { > >> >> + cluster0 { > >> >> + core0 { cpu = <&cpu0>; }; > >> >> + core1 { cpu = <&cpu1>; }; > >> >> + core2 { cpu = <&cpu2>; }; > >> >> + core3 { cpu = <&cpu3>; }; > >> >> + }; > >> >> + > >> >> + cluster1 { > >> >> + core0 { cpu = <&cpu4>; }; > >> >> + core1 { cpu = <&cpu5>; }; > >> >> + core2 { cpu = <&cpu6>; }; > >> >> + core3 { cpu = <&cpu7>; }; > >> >> + }; > >> >> + > >> >> + cluster2 { > >> >> + core0 { cpu = <&cpu8>; }; > >> >> + core1 { cpu = <&cpu9>; }; > >> >> + core2 { cpu = <&cpu10>; }; > >> >> + core3 { cpu = <&cpu11>; }; > >> >> + }; > >> >> + > >> >> + cluster3 { > >> >> + core0 { cpu = <&cpu12>; }; > >> >> + core1 { cpu = <&cpu13>; }; > >> >> + core2 { cpu = <&cpu14>; }; > >> >> + core3 { cpu = <&cpu15>; }; > >> >> + }; > >> >> + }; > >> >> + > >> >> + /* CLUSTER 0 */ > >> >> + cpu0: cpu@0 { > >> >> + device_type = "cpu"; > >> >> + compatible = "arm,cortex-a72"; > >> >> + reg = <0 0x0>; > >> > > >> > Do you really need 2/0 split here. The first cell is 0 anyway. > >> > >> Yes following 64-bit system definition > > > > You mean for the 64-bit main address space? > > The CPU address space under /cpus is unrelated. > > Yes, the reg prop for this node is CPU/threads per dt spec. Checked the history and > the Elba dt was derived from socionext for these nodes and this is how those device > trees are configured along with over a dozen other devices. I changed to > address-cells = <1> and dropped the leading zero from all cpu* reg<> and booting > the system I'm observing no change. Looking in drivers/of I'm not seeing where > cpu*/reg is read and used, any recommendation? drivers/of/cpu.c Looks like there are lots of DTS files that use #address-cells = <2> for CPU nodes :-( git grep -w -A1 cpus -- "*dts*" | grep address-cells | grep "<2>" I would use <1> is the first cell is always zero... Gr{oetje,eeting}s, Geert
Hi Geert, On Wed, May 31, 2023 at 15:09 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, May 31, 2023 at 12:04 AM Brad Larson <blarson@amd.com> wrote: >> On Wed, May 24, 2023 at 13:52 Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> > On Tue, May 23, 2023 at 9:30 PM Brad Larson <blarson@amd.com> wrote: >> >> On 5/16/23 09:54, Michal Simek wrote: >> >> > On 5/15/23 20:16, Brad Larson wrote: ... >> >> >> + /* CLUSTER 0 */ >> >> >> + cpu0: cpu@0 { >> >> >> + device_type = "cpu"; >> >> >> + compatible = "arm,cortex-a72"; >> >> >> + reg = <0 0x0>; >> >> > >> >> > Do you really need 2/0 split here. The first cell is 0 anyway. >> >> >> >> Yes following 64-bit system definition >> > >> > You mean for the 64-bit main address space? >> > The CPU address space under /cpus is unrelated. >> >> Yes, the reg prop for this node is CPU/threads per dt spec. Checked the history and >> the Elba dt was derived from socionext for these nodes and this is how those device >> trees are configured along with over a dozen other devices. I changed to >> address-cells = <1> and dropped the leading zero from all cpu* reg<> and booting >> the system I'm observing no change. Looking in drivers/of I'm not seeing where >> cpu*/reg is read and used, any recommendation? > > drivers/of/cpu.c > > Looks like there are lots of DTS files that use #address-cells = <2> for > CPU nodes :-( > > git grep -w -A1 cpus -- "*dts*" | grep address-cells | grep "<2>" > > I would use <1> is the first cell is always zero... I'll do that. Tha variation across DTS is likely coming from ~5.10 devicetree/bindings/arm/cpus.txt - #address-cells ... # On ARM v8 64-bit systems value should be set to 2, that corresponds to the MPIDR_EL1 register size. If MPIDR_EL1[63:32] value is equal to 0 on all CPUs in the system, #address-cells can be set to 1, since MPIDR_EL1[63:32] bits are not used for CPUs identification. where the size of MPIDR_EL1 register is 2 for Elba cores. However the shorthand is allowed if MPIDR_EL1[63:32] bita are not used. Latest version: On ARM v8 64-bit systems this property is required and matches the MPIDR_EL1 register affinity bits. * If cpus node's #address-cells property is set to 2 The first reg cell bits [7:0] must be set to bits [39:32] of MPIDR_EL1. The second reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1. * If cpus node's #address-cells property is set to 1 The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1. All other bits in the reg cells must be set to 0. Regards, Brad
Hi Arnd, > On Wed, May 24, 2023, at 00:11, Brad Larson wrote: >>> On Mon, May 15, 2023, at 20:16, Brad Larson wrote: ... >>> Also, can you explain why this needs a low-lever user interface >>> in the first place, rather than something that can be expressed >>> using high-level abstractions as you already do with the reset >>> control? >>> >>> All of the above should be part of the changelog text to get a >>> driver like this merged. I don't think we can get a quick >>> solution here though, so maybe you can start by removing the >>> ioctl side and having the rest of the driver in drivers/reset? Might be best to pull the whole thing for now until an acceptable solution is reached. The reset function is a recovery mechanisim rarely used where the byte access to the different IP at the 4 chip-selects is needed for a system to boot. >> In the original patchset I added a pensando compatible to spidev and that >> was nacked in review and reusing some random compatible that made it into >> spidev was just wrong. Further it was recommended this should be a system >> specific driver and don't rely on a debug driver like spidev. I changed >> over to a platform specific driver and at that time I also needed to include >> a reset controller (emmc reset only). I put these in drivers/mfd and >> drivers/reset. Review of the device tree for this approach went back and >> forth to _not_ have four child nodes on the spi device each with the same >> compatible. Decision was to squash the child nodes into the parent and put >> the reset-controller there also. One driver and since its pensando >> specific its currently in drivers/soc/amd. >> >> There are five different user processes and some utilities that access the >> functionality in the cpld/fpga. You're correct, its passing messages that >> are specific to the IP accessed via chip-select. No Elba system will boot >> without this driver providing ioctl access. > Thank you for the detailed summary. Moving away from spidev and > from mfd seems all reasonable here. I'm still a bit confused by > why you have multiple chipselects here that are for different > subdevices but ended with a single user interface for all of them, > but that's not a big deal. The goal is to isolate the the kernel from device and platform specific changes. All the IO to the spi connected CPLD/FPGA (design/cost dependent) is a byte at a time or up to 16 bytes for internal flash mgmt. Performance is not an issue and spidev was sufficient. Maybe this paints the right picture to zero in on a correct approach. Internal and external IP can be present at CS1/CS2 depending on the design where the CS0 board controller registers get additions over time in a backward compatible manner. Design 1: FPGA CS0: Board controller registers CS1: Designware SPI to I2C to board peripherals CS2: Lattice dual I2C master CS3: Internal storage Design 2: CPLD CS0: Board controller registers CS1: Not used or some other board specific registers CS2: Lattice dual I2C master CS3: Internal storage > The main bit that sticks out about this high-level design is how > it relies on user space utilities at all to understand the message > format. From what I understand about the actual functionality of > this device, it most closely resembles an embedded controller that > you might find in a laptop or server machine, and those usually > have kernel drivers in drivers/platform/ to interact with the > device. The dozens of registers at CS0 for board management are defined in userspace programs or script. Only the regsiter offset/bit for emmc reset is needed for the reset function in the patches. > Has anyone tried to do it like that? Maybe it would help > to see what the full protocol and the user space side looks > like, in order to move some or all of it into the kernel. Looking at drivers/platform its pretty sparse. What do you recommend based on the design 1/2 variations? Regards, Brad