diff mbox series

[v12,15/15] soc: amd: Add support for AMD Pensando SoC Controller

Message ID 20230323000657.28664-16-blarson@amd.com (mailing list archive)
State Superseded
Headers show
Series Support AMD Pensando Elba SoC | expand

Commit Message

Brad Larson March 23, 2023, 12:06 a.m. UTC
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>
---

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        | 380 +++++++++++++++++++++++++
 include/uapi/linux/amd-pensando-ctrl.h |  30 ++
 6 files changed, 430 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

Comments

Andy Shevchenko March 23, 2023, 11:06 a.m. UTC | #1
On Thu, Mar 23, 2023 at 2:11 AM 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.

...

> +config AMD_PENSANDO_CTRL
> +       tristate "AMD Pensando SoC Controller"
> +       depends on SPI_MASTER=y
> +       depends on (ARCH_PENSANDO && OF) || COMPILE_TEST
> +       default y if ARCH_PENSANDO

       default ARCH_PENSANDO

?

> +       select REGMAP_SPI
> +       select MFD_SYSCON

...

> +/*
> + * 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 regsiters and management IO support.

registers ?

> + *
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + */

...

> +#include <linux/cdev.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>

Seems semi-random. Are you sure you use this and not missing mod_devicetable.h?

> +#include <linux/reset-controller.h>
> +#include <linux/spi/spi.h>

...

> +struct penctrl_device {
> +       struct spi_device *spi_dev;
> +       struct reset_controller_dev rcdev;

Perhaps swapping these two might provide a better code generation.

> +};

...

> +       struct spi_transfer t[2] = { 0 };

0 is not needed.

...

> +       if (_IOC_DIR(cmd) & _IOC_READ)
> +               ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));
> +       else if (_IOC_DIR(cmd) & _IOC_WRITE)
> +               ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));


Maybe you should create a temporary variable as

    void __user *in = ... arg;

?

> +       if (ret)
> +               return -EFAULT;

...

> +       /* Verify and prepare spi message */

SPI

> +       size = _IOC_SIZE(cmd);
> +       if ((size % sizeof(struct penctrl_spi_xfer)) != 0) {

' != 0' is redundant.

> +               ret = -EINVAL;
> +               goto done;
> +       }
> +       num_msgs = size / sizeof(struct penctrl_spi_xfer);

> +       if (num_msgs == 0) {
> +               ret = -EINVAL;
> +               goto done;
> +       }

Can be unified with a previous check as

if (size == 0 || size % ...)

> +       msg = memdup_user((struct penctrl_spi_xfer __user *)arg, size);
> +       if (!msg) {
> +               ret = PTR_ERR(msg);
> +               goto done;
> +       }

...

> +       if (copy_from_user((void *)(uintptr_t)tx_buf,
> +                          (void __user *)msg->tx_buf, msg->len)) {

Why are all these castings here?

> +               ret = -EFAULT;
> +               goto done;
> +       }

...

> +       if (copy_to_user((void __user *)msg->rx_buf,
> +                        (void *)(uintptr_t)rx_buf, msg->len))
> +               ret = -EFAULT;

Ditto.

...

> +       struct spi_transfer t[2] = { 0 };

0 is redundant.

...

> +       struct spi_transfer t[1] = { 0 };

Ditto.

Why is this an array?

...

> +       ret = spi_sync(spi_dev, &m);
> +       return ret;

return spi_sync(...);

...

> +       np = spi_dev->dev.parent->of_node;
> +       ret = of_property_read_u32(np, "num-cs", &num_cs);

Why not simply device_property_read_u32()?

> +       if (ret)
> +               return dev_err_probe(&spi_dev->dev, ret,
> +                                    "number of chip-selects not defined");

...

> +       cdev = cdev_alloc();
> +       if (!cdev) {
> +               dev_err(&spi_dev->dev, "allocation of cdev failed");
> +               ret = -ENOMEM;

ret = dev_err_probe(...);

> +               goto cdev_failed;
> +       }

...

> +       ret = cdev_add(cdev, penctrl_devt, num_cs);
> +       if (ret) {

> +               dev_err(&spi_dev->dev, "register of cdev failed");

dev_err_probe() ?

> +               goto cdev_delete;
> +       }

...

> +       penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
> +       if (!penctrl) {

> +               ret = -ENOMEM;
> +               dev_err(&spi_dev->dev, "allocate driver data failed");

ret = dev_err_probe();
But we do not print memory allocation failure messages.

> +               goto cdev_delete;
> +       }

...

> +               if (IS_ERR(dev)) {
> +                       ret = IS_ERR(dev);
> +                       dev_err(&spi_dev->dev, "error creating device\n");

ret = dev_err_probe();

> +                       goto cdev_delete;
> +               }
> +               dev_dbg(&spi_dev->dev, "created device major %u, minor %d\n",
> +                       MAJOR(penctrl_devt), cs);
> +       }

...

> +       spi_set_drvdata(spi_dev, penctrl);

Is it in use?

...

> +       penctrl->rcdev.of_node = spi_dev->dev.of_node;

device_set_node();

...

> +       ret = reset_controller_register(&penctrl->rcdev);
> +       if (ret)
> +               return dev_err_probe(&spi_dev->dev, ret,
> +                                    "failed to register reset controller\n");
> +       return ret;

return 0;

...

> +       device_destroy(penctrl_class, penctrl_devt);

Are you sure this is the correct API?

> +       return ret;

...

> +#include <linux/types.h>
> +#include <linux/ioctl.h>

Sorted?
Brad Larson March 31, 2023, 10:26 p.m. UTC | #2
Hi Andy,

Thanks for the review.

On Thu, Mar 23, 2023 at 13:06 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 23, 2023 at 2:11 AM 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.
>
> ...
>
>> +config AMD_PENSANDO_CTRL
>> +       tristate "AMD Pensando SoC Controller"
>> +       depends on SPI_MASTER=y
>> +       depends on (ARCH_PENSANDO && OF) || COMPILE_TEST
>> +       default y if ARCH_PENSANDO
>
>       default ARCH_PENSANDO

Changed to default ARCH_PENSANDO

>> +       select REGMAP_SPI
>> +       select MFD_SYSCON
>
> ...
>
>> +/*
>> + * 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 regsiters and management IO support.
>
> registers ?

Fixed the typo

> ...
>
>> +#include <linux/cdev.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/fs.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>
> Seems semi-random. Are you sure you use this and not missing mod_devicetable.h?

Added mod_devicetable.h.
Removed delay.h, fs.h and of_device.h

>> +#include <linux/reset-controller.h>
>> +#include <linux/spi/spi.h>
>
>...
>
>> +struct penctrl_device {
>> +       struct spi_device *spi_dev;
>> +       struct reset_controller_dev rcdev;
>
> Perhaps swapping these two might provide a better code generation.

Its a 96 byte struct with pointer followed by the reset controller.
The spi_device variable is accessed frequently and rcdev during
boot and ideally never again so if rcdev is mostly missing from
cache that is fine.  Likely the address of spi_dev is also in 
cache given it is periodically accessed.

> ...
>
>> +       struct spi_transfer t[2] = { 0 };
>
> 0 is not needed.

Dropped the 0.

> ...
>
>> +       if (_IOC_DIR(cmd) & _IOC_READ)
>> +               ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));
>> +       else if (_IOC_DIR(cmd) & _IOC_WRITE)
>> +               ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));
>
>
> Maybe you should create a temporary variable as
>
>    void __user *in = ... arg;

Yes, created temp variable.

>
>> +       if (ret)
>> +               return -EFAULT;
>
> ...
>
>> +       /* Verify and prepare spi message */
>
> SPI

Changed to SPI

>> +       size = _IOC_SIZE(cmd);
>> +       if ((size % sizeof(struct penctrl_spi_xfer)) != 0) {
>
> ' != 0' is redundant.

Fixed

>
>> +               ret = -EINVAL;
>> +               goto done;
>> +       }
>> +       num_msgs = size / sizeof(struct penctrl_spi_xfer);
>
>> +       if (num_msgs == 0) {
>> +               ret = -EINVAL;
>> +               goto done;
>> +       }
>
> Can be unified with a previous check as
>
> if (size == 0 || size % ...)

Yes, made this change.

>> +       msg = memdup_user((struct penctrl_spi_xfer __user *)arg, size);
>> +       if (!msg) {
>> +               ret = PTR_ERR(msg);
>> +               goto done;
>> +       }
>
>...
>
>> +       if (copy_from_user((void *)(uintptr_t)tx_buf,
>> +                          (void __user *)msg->tx_buf, msg->len)) {
>
> Why are all these castings here?

Yes, overkill, changed to:

        if (copy_from_user(tx_buf, (void __user *)msg->tx_buf, msg->len)) {

> ...
>
>> +       if (copy_to_user((void __user *)msg->rx_buf,
>> +                        (void *)(uintptr_t)rx_buf, msg->len))
>> +               ret = -EFAULT;
>
> Ditto.

Changed to:

        if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len))

> ...
>
>> +       struct spi_transfer t[2] = { 0 };
>
> 0 is redundant.

Dropped the 0.

> ...
>
>> +       struct spi_transfer t[1] = { 0 };
>
> Ditto.

Dropped the 0.

> Why is this an array?

Fixed, it's a single message and shouldn't be an array.

> ...
>
>> +       ret = spi_sync(spi_dev, &m);
>> +       return ret;
>
> return spi_sync(...);

Fixed.

> ...
>
>> +       np = spi_dev->dev.parent->of_node;
>> +       ret = of_property_read_u32(np, "num-cs", &num_cs);
>
> Why not simply device_property_read_u32()?

It can be and removed two lines with below result. Also changed the
variable spi_dev to spi which is the more common usage.

        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");

> ...
>
>> +       cdev = cdev_alloc();
>> +       if (!cdev) {
>> +               dev_err(&spi_dev->dev, "allocation of cdev failed");
>> +               ret = -ENOMEM;
>
> ret = dev_err_probe(...);

Fixed.

>> +               goto cdev_failed;
>> +       }
>
> ...
>
>> +       ret = cdev_add(cdev, penctrl_devt, num_cs);
>> +       if (ret) {
>
>> +               dev_err(&spi_dev->dev, "register of cdev failed");
>
> dev_err_probe() ?

Fixed.

>> +               goto cdev_delete;
>> +       }
>
> ...
>
>> +       penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
>> +       if (!penctrl) {
>
>> +               ret = -ENOMEM;
>> +               dev_err(&spi_dev->dev, "allocate driver data failed");
>
> ret = dev_err_probe();
> But we do not print memory allocation failure messages.

Fixed this way

        penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
        if (!penctrl) {
                ret = -ENOMEM;
                goto free_cdev;
        }

> ...
>
>> +               if (IS_ERR(dev)) {
>> +                       ret = IS_ERR(dev);
>> +                       dev_err(&spi_dev->dev, "error creating device\n");
>
> ret = dev_err_probe();

Fixed.

> ...
>
> +       spi_set_drvdata(spi_dev, penctrl);
>
> Is it in use?

Not used and now dropped.

> ...
>
>> +       penctrl->rcdev.of_node = spi_dev->dev.of_node;
>
> device_set_node();

Added: device_set_node(&spi->dev, dev_fwnode(dev));

> ...
>
>> +       ret = reset_controller_register(&penctrl->rcdev);
>> +       if (ret)
>> +               return dev_err_probe(&spi_dev->dev, ret,
>> +                                    "failed to register reset controller\n");
>> +       return ret;
>
> return 0;

Yes, changed.

> ...
>
>> +       device_destroy(penctrl_class, penctrl_devt);
>
> Are you sure this is the correct API?

Yes, however a probe error could call up to 5 APIs to clean up which resulted
in this update:

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;

> ...
>
>> +#include <linux/types.h>
>> +#include <linux/ioctl.h>
>
> Sorted?

Swapped these

Regards,
Brad
diff mbox series

Patch

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..9fc03de49f11
--- /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 y if 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..91ed0ee424c9
--- /dev/null
+++ b/drivers/soc/amd/pensando-ctrl.c
@@ -0,0 +1,380 @@ 
+// 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 regsiters and management IO support.
+ *
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/cdev.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/reset-controller.h>
+#include <linux/spi/spi.h>
+#include <linux/amd-pensando-ctrl.h>
+
+struct penctrl_device {
+	struct spi_device *spi_dev;
+	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)
+{
+	struct spi_transfer t[2] = { 0 };
+	struct penctrl_device *penctrl;
+	struct penctrl_spi_xfer *msg;
+	struct spi_device *spi_dev;
+	unsigned int num_msgs;
+	struct spi_message m;
+	u8 tx_buf[PENCTRL_MAX_MSG_LEN];
+	u8 rx_buf[PENCTRL_MAX_MSG_LEN];
+	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((void __user *)arg, _IOC_SIZE(cmd));
+	else if (_IOC_DIR(cmd) & _IOC_WRITE)
+		ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));
+
+	if (ret)
+		return -EFAULT;
+
+	/* Get a reference to the SPI device */
+	penctrl = filp->private_data;
+	if (!penctrl)
+		return -ESHUTDOWN;
+
+	spi_dev = spi_dev_get(penctrl->spi_dev);
+	if (!spi_dev)
+		return -ESHUTDOWN;
+
+	/* Verify and prepare spi message */
+	size = _IOC_SIZE(cmd);
+	if ((size % sizeof(struct penctrl_spi_xfer)) != 0) {
+		ret = -EINVAL;
+		goto done;
+	}
+	num_msgs = size / sizeof(struct penctrl_spi_xfer);
+	if (num_msgs == 0) {
+		ret = -EINVAL;
+		goto done;
+	}
+	msg = memdup_user((struct penctrl_spi_xfer __user *)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((void *)(uintptr_t)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_dev, &m);
+	mutex_unlock(&spi_lock);
+
+	if (ret || (num_msgs == 1))
+		goto done;
+
+	if (copy_to_user((void __user *)msg->rx_buf,
+			 (void *)(uintptr_t)rx_buf, msg->len))
+		ret = -EFAULT;
+
+done:
+	spi_dev_put(spi_dev);
+	return ret;
+}
+
+static int penctrl_open(struct inode *inode, struct file *filp)
+{
+	struct spi_device *spi_dev;
+	u8 current_cs;
+
+	if (!penctrl)
+		return -ENODEV;
+
+	filp->private_data = penctrl;
+	current_cs = iminor(inode);
+	spi_dev = penctrl->spi_dev;
+	spi_dev->chip_select = current_cs;
+	spi_dev->cs_gpiod = spi_dev->controller->cs_gpiods[current_cs];
+	spi_setup(spi_dev);
+	stream_open(inode, filp);
+
+	return 0;
+}
+
+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_dev = penctrl->spi_dev;
+	struct spi_transfer t[2] = { 0 };
+	struct spi_message m;
+	u8 txbuf[3];
+	u8 rxbuf[1];
+	int ret;
+
+	txbuf[0] = PENCTRL_SPI_CMD_REGRD;
+	txbuf[1] = reg;
+	txbuf[2] = 0x0;
+	t[0].tx_buf = txbuf;
+	t[0].len = 3;
+
+	rxbuf[0] = 0x0;
+	t[1].rx_buf = rxbuf;
+	t[1].len = 1;
+
+	spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
+	ret = spi_sync(spi_dev, &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_dev = penctrl->spi_dev;
+	struct spi_transfer t[1] = { 0 };
+	struct spi_message m;
+	u8 txbuf[4];
+	int ret;
+
+	txbuf[0] = PENCTRL_SPI_CMD_REGWR;
+	txbuf[1] = reg;
+	txbuf[2] = val;
+	txbuf[3] = 0;
+
+	t[0].tx_buf = txbuf;
+	t[0].len = 4;
+	spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
+	ret = spi_sync(spi_dev, &m);
+	return ret;
+}
+
+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_dev = penctrl->spi_dev;
+	unsigned int val;
+	int ret;
+
+	mutex_lock(&spi_lock);
+	spi_dev->chip_select = 0;
+	spi_dev->cs_gpiod = spi_dev->controller->cs_gpiods[0];
+	spi_setup(spi_dev);
+	ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val);
+	if (ret) {
+		dev_err(&spi_dev->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->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_dev = penctrl->spi_dev;
+	unsigned int val;
+	int ret;
+
+	mutex_lock(&spi_lock);
+	spi_dev->chip_select = 0;
+	spi_dev->cs_gpiod = spi_dev->controller->cs_gpiods[0];
+	spi_setup(spi_dev);
+	ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val);
+	if (ret) {
+		dev_err(&spi_dev->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->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_dev)
+{
+	struct device_node *np;
+	struct device *dev;
+	struct cdev *cdev;
+	u32 num_cs;
+	int ret;
+	u32 cs;
+
+	np = spi_dev->dev.parent->of_node;
+	ret = of_property_read_u32(np, "num-cs", &num_cs);
+	if (ret)
+		return dev_err_probe(&spi_dev->dev, ret,
+				     "number of chip-selects not defined");
+
+	ret = alloc_chrdev_region(&penctrl_devt, 0, num_cs, "penctrl");
+	if (ret)
+		return dev_err_probe(&spi_dev->dev, ret,
+				     "failed to alloc chrdev region\n");
+
+	penctrl_class = class_create(THIS_MODULE, "penctrl");
+	if (IS_ERR(penctrl_class)) {
+		unregister_chrdev(MAJOR(penctrl_devt), "penctrl");
+		return dev_err_probe(&spi_dev->dev, PTR_ERR(penctrl_class),
+				     "failed to create class\n");
+	}
+
+	cdev = cdev_alloc();
+	if (!cdev) {
+		dev_err(&spi_dev->dev, "allocation of cdev failed");
+		ret = -ENOMEM;
+		goto cdev_failed;
+	}
+	cdev->owner = THIS_MODULE;
+	cdev_init(cdev, &penctrl_fops);
+
+	ret = cdev_add(cdev, penctrl_devt, num_cs);
+	if (ret) {
+		dev_err(&spi_dev->dev, "register of cdev failed");
+		goto cdev_delete;
+	}
+
+	/* Allocate driver data */
+	penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
+	if (!penctrl) {
+		ret = -ENOMEM;
+		dev_err(&spi_dev->dev, "allocate driver data failed");
+		goto cdev_delete;
+	}
+
+	penctrl->spi_dev = spi_dev;
+	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->dev,
+				    MKDEV(MAJOR(penctrl_devt), cs),
+				    penctrl,
+				    "penctrl0.%d",
+				    cs);
+		if (IS_ERR(dev)) {
+			ret = IS_ERR(dev);
+			dev_err(&spi_dev->dev, "error creating device\n");
+			goto cdev_delete;
+		}
+		dev_dbg(&spi_dev->dev, "created device major %u, minor %d\n",
+			MAJOR(penctrl_devt), cs);
+	}
+
+	spi_set_drvdata(spi_dev, penctrl);
+
+	/* Register emmc hardware reset */
+	penctrl->rcdev.nr_resets = 1;
+	penctrl->rcdev.owner = THIS_MODULE;
+	penctrl->rcdev.dev = &spi_dev->dev;
+	penctrl->rcdev.ops = &penctrl_reset_ops;
+	penctrl->rcdev.of_node = spi_dev->dev.of_node;
+	ret = reset_controller_register(&penctrl->rcdev);
+	if (ret)
+		return dev_err_probe(&spi_dev->dev, ret,
+				     "failed to register reset controller\n");
+	return ret;
+
+cdev_delete:
+	cdev_del(cdev);
+cdev_failed:
+	device_destroy(penctrl_class, penctrl_devt);
+	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..0a6caf3b764c
--- /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/types.h>
+#include <linux/ioctl.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 */