mbox series

[v13,00/15] Support AMD Pensando Elba SoC

Message ID 20230410184526.15990-1-blarson@amd.com
Headers show
Series Support AMD Pensando Elba SoC | expand

Message

Brad Larson April 10, 2023, 6:45 p.m. UTC
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

== V13 changes ==
v13-0013-mmc-sdhci-cadence-Add-AMD-Pensando-Elba-SoC-supp
- Use GENMASK(7, 3) in elba_priv_writel() to set all byte enables 
- Add a variable 'shift' with GENMASK(1, 0) in elba_write_w() and
  elba_write_b() to set the byte enable variable.

v13-0015-soc-amd-Add-support-for-AMD-Pensando-SoC-Control
- 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 ==
v12-0004-dt-bindings-spi-dw-Add-AMD-Pensando-Elba-SoC-SPI
- Correct property amd,pensando-elba-syscon description

v12-0010-spi-dw-Add-support-for-AMD-Pensando-Elba-SoC
- Add a newline in function dw_spi_elba_init()

v12-0015-soc-amd-Add-support-for-AMD-Pensando-SoC-Control
- Fix gcc-12.1.0 warning:

== V11 changes ==
v11-0002-dt-bindings-mmc-cdns-Add-AMD-Pensando-Elba-SoC
- Remove resets description and reset-names
- Add descriptions for amd,pensando-elba-sd4hc reg items

v11-0003-dt-bindings-spi-cdns-Add-compatible-for-AMD-Pens
- Removed redundant if/then for amd,pensando-elba-qspi

v11-0005-dt-bindings-soc-amd-amd-pensando-elba-ctrl-Add-P
- Fixed the compatible which should have stayed as
  'amd,pensando-elba-ctrl', the commit message, and the filename.
- Reference spi-peripheral-props
- Delete spi-max-frequency
- Remove num-cs from example

v11-0008-arm64-dts-Add-AMD-Pensando-Elba-SoC-support
- Delete reset-names
- Fix spi0 compatible to be specific 'amd,pensando-elba-ctrl'

v11-0010-spi-dw-Add-support-for-AMD-Pensando-Elba-SoC
- Simplify dw_spi_elb_init by using syscon_regmap_lookup_by_phandle()

v11-0013-mmc-sdhci-cadence-Add-AMD-Pensando-Elba-SoC-supp
- Remove elba-drv_init() call to platform_get_resource() since that
  check is done inside devm_platform_ioremap_resource()
- Move spin_lock_init() before error check
- Remove extra parentheses

v11-0015-soc-amd-Add-support-for-AMD-Pensando-SoC-Control
- Fix the compatible to be specific 'amd,pensando-elba-ctrl'
- Cast arguments flagged with a gcc-12.1.0 warning:
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/oe-kbuild-all/202303061526.I8VPcR1M-lkp@intel.com/

== V10 changes ==
Binding property amd,pensando-elba-syscon was merged in 6.2

v10-0002-dt-bindings-mmc-cdns-Add-AMD-Pensando-Elba-SoC
- Move reset-names property definition next to existing resets prop
- Move allOf to the bottom and set resets/reset-names required only for pensando
- Fix reg maxItems for existing, must be 1

v10-0003-dt-bindings-spi-cdns-Add-compatible-for-AMD-Pens
- Fix cdns,fifo-depth, only amd,pensando-elba-qspi is 1024 bytes

v10-0004-dt-bindings-spi-dw-Add-AMD-Pensando-Elba-SoC-SPI
- Move definition of amd,pensando-elba-syscon into properties with a better description
- Add amd,pensando-elba-syscon: false for non elba designs

v10-0005-dt-bindings-soc-amd-amd-pensando-elbasr-Add-AMD-
- Property renamed to amd,pensando-ctrl
- Driver is renamed and moved to soc/drivers/amd affecting binding
- Delete cs property, driver handles device node creation from parent num-cs
  fixing schema reg error in a different way

v10-0010-spi-dw-Add-support-for-AMD-Pensando-Elba-SoC
- Delete struct dw_spi_elba, use regmap directly in priv

v10-0011-mmc-sdhci-cadence-Enable-device-specific-overrid
- The 1st patch adding private writel() is unchanged.  The 2nd patch is split
  into two patches to provide for device specific init in one patch with no
  effect on existing designs.  Then add the pensando support into the next patch.
  Then the 4th patch is mmc hardware reset support which is unchanged.

v10-0012-mmc-sdhci-cadence-Support-device-specific-init-i
- New patch to provide for platform specific init() with no change
  to existing designs.

v10-0013-mmc-sdhci-cadence-Add-AMD-Pensando-Elba-SoC-supp
- Add Elba specific support into this 3rd patch.  This builds on the private
  writel() enabled in patch 1 followed by platform specific init() in patch 2.
- Specify when first used the reason for the spinlock use to order byte-enable
  prior to write data.

v10-0015-soc-amd-Add-support-for-AMD-Pensando-SoC-Control
- 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 ==
v9-0002-dt-bindings-mmc-cdns-Add-AMD-Pensando-Elba-SoC
- Add reset-names and resets properties
- Add if/then on property amd,pensando-elba-sd4hc to set reg property
  values for minItems and maxItems

v9-0003-dt-bindings-spi-cdns-Add-compatible-for-AMD-Pensa
- Add 1024 to cdns,fifo-depth property to resolve dtbs_check error

v9-0004-dt-bindings-spi-dw-Add-AMD-Pensando-Elba-SoC-SPI-
- Define property amd,pensando-elba-syscon
- Move compatible amd,pensando-elba-spi ahead of baikal,bt1-ssi

v9-0006-dt-bindings-mfd-amd-pensando-elbasr-Add-AMD-Pensa
- Instead of four nodes, one per chip-select, a single
  node is used with reset-cells in the parent.
- No MFD API is used anymore in the driver so it made
  sense to move this to drivers/spi.
- This driver is common for all Pensando SoC based designs
  so changed the name to pensando-sr.c to not make it Elba
  SoC specific.
- Added property cs for the chip-select number which is used
  by the driver to create /dev/pensr0.<cs>

v9-0009-arm64-dts-Add-AMD-Pensando-Elba-SoC-support
- Single node for spi0 system-controller and squash
  the reset-controller child into parent

v9-0010-spi-cadence-quadspi-Add-compatible-for-AMD-Pensan
- Rebase to linux-next 6.2.0-rc1

v9-0011-spi-dw-Add-support-for-AMD-Pensando-Elba-SoC
- Add use of macros GENMASK() and BIT()
- Change ELBA_SPICS_SHIFT() to ELBA_SPICS_OFFSET()

v9-0012-mmc-sdhci-cadence-Enable-device-specific-override
- No change to this patch but as some patches are deleted and this is
  a respin the three successive patches to sdhci-cadence.c are
  patches 12, 13, and 14 which do the following:
  1. Add ability for Cadence specific design to have priv writel().
  2. Add Elba SoC support that requires its own priv writel() for
     byte-lane control .
  3. Add support for mmc hardware reset.

v9-0014-mmc-sdhci-cadence-Support-mmc-hardware-reset
- Previously patch 17/17
- Changed delay after reset_control_assert() from 9 to 3 usec
- Renamed sdhci_mmc_hw_reset() to sdhci_cdns_mmc_hw_reset()

v9-0015-spi-pensando-sr-Add-AMD-Pensando-SoC-System-Resou
- 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


== V6 changes ==
- Updated copyright and SPDX

v6-0001-dt-bindings-arm-add-AMD-Pensando-boards
- Delete 'Device Tree Bindings' in title

v6-0002-dt-bindings-mmc-cdns-Add-AMD-Pensando-Elba-SoC
- Change if/then for Elba which has a second reg for byte-lane control

v6-0003-dt-bindings-spi-cdns-Add-compatible-for-AMD-Pensa
- no change

v6-0004-dt-bindings-spi-dw-Add-AMD-Pensando-Elba-SoC-SPI-
- Add amd,pensando-elba-syscon

v6-0005-dt-bindings-mfd-syscon-Add-amd-pensando-elba-sysc
- no change

v6-0006-dt-bindings-mfd-amd-pensando-elbasr-Add-AMD-Pensa
- Expand description, rename nodes and change compatible usage

v6-0007-dt-bindings-reset-amd-pensando-elbasr-reset-Add-A
- Delete nodename pattern and changed spi0 to spi
- File amd,pensando-elba-reset.h is deleted as there is only
  one reset used.
- Update example

v6-0008-MAINTAINERS-Add-entry-for-AMD-PENSANDO
- no change

v6-0009-arm64-Add-config-for-AMD-Pensando-SoC-platforms
- no change

v6-0010-arm64-dts-Add-AMD-Pensando-Elba-SoC-support
- Update node names and add amd,pensando-elba-syscon
- Delete use of amd,pensando-elba-reset.h which had a single definition

v6-0011-spi-cadence-quadspi-Add-compatible-for-AMD-Pensan
- Remove (void) cast

v6-0012-spi-dw-Add-support-for-AMD-Pensando-Elba-SoC
- Update use of amd,pensando-elba-syscon

v6-0013-mmc-sdhci-cadence-Enable-device-specific-override
- Change this patch to add a priv_writel() callback where all
  existing designs use writel().  This separates the Elba
  support into three patches.  The second patch is added
  to the end of the sequence for Elba support.  The third
  patch enables mmc hardware reset.

v6-0014-mfd-pensando-elbasr-Add-AMD-Pensando-Elba-System-
- Updates from review comments
- Use spi_message_init_with_transfers instead of init/add_tail API

v6-0015-reset-elbasr-Add-AMD-Pensando-Elba-SR-Reset-Contr
- Remove use of amd,pensando-elba-reset.h and use BIT()

v6-0016-mmc-sdhci-cadence-Add-AMD-Pensando-Elba-SoC-suppo
- Elba sdhci-cadence.c support added in this patch to build on
  0013 which just adds a callback to override priv_writel()

v6-0017-mmc-sdhci-cadence-Support-mmc-hardware-reset
- New patch where Elba has a reset-controller for mmc hardware
  reset.  The reset is implemented by a register in the cpld.

== V5 changes ==
- Change to AMD Pensando instead of Pensando.
- No reference to spidev in the device tree.  Add multi-function driver
  pensando-elbasr and sub-device reset-elbasr which provides mfd and
  /dev interface to the cpld.
- Rebase to linux-next tag next-20220609 5.19.0-rc1
- Redo the email list after rebase and using scripts/get_maintainer.pl

== V4 changes ==
The version of dtschema used is 2022.3.2.

v4-0001-dt-bindings-arm-add-Pensando-boards.patch
- Add description and board compatible

v4-0003-dt-bindings-mmc-Add-Pensando-Elba-SoC-binding.patch
- Change from elba-emmc to elba-sd4hc to match file convention
- Use minItems: 1 and maxItems: 2 to pass schema check

v4-0005-dt-bindings-spi-dw-Add-Pensando-Elba-SoC-SPI-Control.patch
- Add required property pensando,syscon-spics to go with
  pensando,elba-spi

v4-0006-MAINTAINERS-Add-entry-for-PENSANDO.patch
- Change Maintained to Supported

v4-0007-arm64-Add-config-for-Pensando-SoC-platforms.patch
- Fix a typo on interface max speed

v4-0008-spi-cadence-quadspi-Add-compatible-for-Pensando-Elba.patch
- Update due to spi-cadence-quadspi.c changes

v4-0009-mmc-sdhci-cadence-Add-Pensando-Elba-SoC-support.patch
- Change from elba-emmc to elba-sd4hc to match file convention

v4-0010-spi-dw-Add-support-for-Pensando-Elba-SoC.patch
- Use more descriptive dt property pensando,syscon-spics
- Minor changes from review input

v4-0011-arm64-dts-Add-Pensando-Elba-SoC-support.patch
- Changed to dual copyright (GPL-2.0+ OR MIT)
- Minor changes from review input

== V3 changes ==
v3-0001-gpio-Add-Elba-SoC-gpio-driver-for-spi-cs-control.patch
- This patch is deleted.  Elba SOC specific gpio spics control is
  integrated into spi-dw-mmio.c.

v3-0002-spi-cadence-quadspi-Add-QSPI-support-for-Pensando-El.patch
- Changed compatible to "pensando,elba-qspi" to be more descriptive
  in spi-cadence-quadspi.c.

- Arnd wondered if moving to DT properties for quirks may be the
  way to go.  Feedback I've received on other patches was don't
  mix two efforts in one patch so I'm currently just adding the
  Elba support to the current design.

v3-0003-spi-dw-Add-support-for-Pensando-Elba-SoC-SPI.patch
- Changed the implementation to use existing dw_spi_set_cs() and
  integrated Elba specific CS control into spi-dw-mmio.c.  The
  native designware support is for two chip-selects while Elba
  provides 4 chip-selects.  Instead of adding a new file for
  this support in gpio-elba-spics.c the support is in one
  file (spi-dw-mmio.c).

v3-0004-spidev-Add-Pensando-CPLD-compatible.patch
- This patch is deleted.  The addition of compatible "pensando,cpld"
  to spidev.c is not added and an existing compatible is used
  in the device tree to enable.

v3-0005-mmc-sdhci-cadence-Add-Pensando-Elba-SoC-support.patch
- Ulf and Yamada-san agreed the amount of code for this support
  is not enough to need a new file.  The support is added into
  sdhci-cadence.c and new files sdhci-cadence-elba.c and
  sdhci-cadence.h are deleted.
- Redundant defines are removed (e.g. use SDHCI_CDNS_HRS04 and
  remove SDIO_REG_HRS4).
- Removed phy init function sd4_set_dlyvr() and used existing
  sdhci_cdns_phy_init(). Init values are from DT properties.
- Replace  devm_ioremap_resource(&pdev->dev, iomem)
     with  devm_platform_ioremap_resource(pdev, 1)
- Refactored the elba priv_writ_l() and elba_write_l() to
  remove a little redundant code.
- The config option CONFIG_MMC_SDHCI_CADENCE_ELBA goes away.
- Only C syntax and Elba functions are prefixed with elba_

v3-0006-arm64-Add-config-for-Pensando-SoC-platforms.patch
- Added a little more info to the platform help text to assist
  users to decide on including platform support or not.

v3-0007-arm64-dts-Add-Pensando-Elba-SoC-support.patch
- Node names changed to DT generic names
- Changed from using 'spi@' which is reserved
- The elba-flash-parts.dtsi is kept separate as
  it is included in multiple dts files.
- SPDX license tags at the top of each file
- The compatible = "pensando,elba" and 'model' are
  now together in the board file.
- UIO nodes removed
- Ordered nodes by increasing unit address
- Removed an unreferenced container node.
- Dropped deprecated 'device_type' for uart0 node.

v3-0010-dt-bindings-spi-cadence-qspi-Add-support-for-Pensand.patch
- Updated since the latest documentation has been converted to yaml

v3-0011-dt-bindings-gpio-Add-Pensando-Elba-SoC-support.patch
- This patch is deleted since the Elba gpio spics is added to
  the spi dw driver and documented there.

Because of the deletion of patches and merging of code
the new patchset is not similar.  A changelog is added into
the patches for merged code to be helpful on the history.

== V2 changes ==
- 01    Fix typo, return code value and log message.
- 03    Remove else clause, intrinsic DW chip-select is never used.
- 08-11 Split out dts and bindings to sub-patches
- 10    Converted existing cadence-quadspi.txt to YAML schema
- 13    New driver should use <linux/gpio/driver.h>

Brad Larson (15):
  dt-bindings: arm: add AMD Pensando boards
  dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC
  dt-bindings: spi: cdns: Add compatible for AMD Pensando Elba SoC
  dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller
  dt-bindings: soc: amd: amd,pensando-elba-ctrl: Add Pensando SoC
    Controller
  MAINTAINERS: Add entry for AMD PENSANDO
  arm64: Add config for AMD Pensando SoC platforms
  arm64: dts: Add AMD Pensando Elba SoC support
  spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC
  spi: dw: Add support for AMD Pensando Elba SoC
  mmc: sdhci-cadence: Enable device specific override of writel()
  mmc: sdhci-cadence: Support device specific init during probe
  mmc: sdhci-cadence: Add AMD Pensando Elba SoC support
  mmc: sdhci-cadence: Support mmc hardware reset
  soc: amd: Add support for AMD Pensando SoC Controller

 .../devicetree/bindings/arm/amd,pensando.yaml |  26 ++
 .../devicetree/bindings/mmc/cdns,sdhci.yaml   |  27 +-
 .../soc/amd/amd,pensando-elba-ctrl.yaml       |  58 +++
 .../bindings/spi/cdns,qspi-nor.yaml           |  19 +-
 .../bindings/spi/snps,dw-apb-ssi.yaml         |  19 +
 MAINTAINERS                                   |   9 +
 arch/arm64/Kconfig.platforms                  |  12 +
 arch/arm64/boot/dts/amd/Makefile              |   1 +
 arch/arm64/boot/dts/amd/elba-16core.dtsi      | 189 +++++++++
 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 +++++++++
 drivers/mmc/host/Kconfig                      |   1 +
 drivers/mmc/host/sdhci-cadence.c              | 175 +++++++-
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/amd/Kconfig                       |  16 +
 drivers/soc/amd/Makefile                      |   2 +
 drivers/soc/amd/pensando-ctrl.c               | 373 ++++++++++++++++++
 drivers/spi/spi-cadence-quadspi.c             |  19 +
 drivers/spi/spi-dw-mmio.c                     |  58 +++
 include/uapi/linux/amd-pensando-ctrl.h        |  30 ++
 23 files changed, 1421 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/amd,pensando.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-elba-ctrl.yaml
 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
 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


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Comments

Andy Shevchenko April 11, 2023, 9:20 a.m. UTC | #1
On Mon, Apr 10, 2023 at 9:48 PM Brad Larson <blarson@amd.com> wrote:
>
> The Pensando SoC controller is a SPI connected companion device
> that is present in all Pensando SoC board designs.  The essential
> board management registers are accessed on chip select 0 with
> board mgmt IO support accessed using additional chip selects.

...

> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spi/spi.h>

+ Blank line?

> +#include <linux/amd-pensando-ctrl.h>

...

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

Try to swap them and check if the code will be smaller (it depends on
how often one or another member is being used),

> +};

...

> +static long
> +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +       void __user *in_arg = (void __user *)arg;
> +       struct penctrl_device *penctrl;
> +       u8 tx_buf[PENCTRL_MAX_MSG_LEN];
> +       u8 rx_buf[PENCTRL_MAX_MSG_LEN];
> +       struct spi_transfer t[2] = {};
> +       struct penctrl_spi_xfer *msg;
> +       struct spi_device *spi;
> +       unsigned int num_msgs;
> +       struct spi_message m;
> +       u32 size;
> +       int ret;
> +
> +       /* Check for a valid command */
> +       if (_IOC_TYPE(cmd) != PENCTRL_IOC_MAGIC)
> +               return -ENOTTY;
> +
> +       if (_IOC_NR(cmd) > PENCTRL_IOC_MAXNR)
> +               return -ENOTTY;
> +
> +       if (_IOC_DIR(cmd) & _IOC_READ)
> +               ret = !access_ok(in_arg, _IOC_SIZE(cmd));
> +       else if (_IOC_DIR(cmd) & _IOC_WRITE)
> +               ret = !access_ok(in_arg, _IOC_SIZE(cmd));

> +

Unneeded blank line.

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

But it seems you can actually rewrite above in less lines:

       if ((_IOC_DIR(cmd) & _IOC_READ) && !access_ok(in_arg, _IOC_SIZE(cmd)))
         return -EFAULT;

       if ((_IOC_DIR(cmd) & _IOC_WRITE) && !access_ok(in_arg, _IOC_SIZE(cmd)))
         return -EFAULT;

> +       /* Get a reference to the SPI device */
> +       penctrl = filp->private_data;
> +       if (!penctrl)
> +               return -ESHUTDOWN;
> +
> +       spi = spi_dev_get(penctrl->spi);
> +       if (!spi)
> +               return -ESHUTDOWN;
> +
> +       /* Verify and prepare SPI message */
> +       size = _IOC_SIZE(cmd);
> +       num_msgs = size / sizeof(struct penctrl_spi_xfer);
> +       if (size == 0 || size % sizeof(struct penctrl_spi_xfer)) {
> +               ret = -EINVAL;
> +               goto done;
> +       }
> +       msg = memdup_user((struct penctrl_spi_xfer *)arg, size);

> +       if (!msg) {
> +               ret = PTR_ERR(msg);

This is strange.

> +               goto done;
> +       }
> +       if (msg->len > PENCTRL_MAX_MSG_LEN) {
> +               ret = -EINVAL;
> +               goto done;
> +       }
> +
> +       t[0].tx_buf = tx_buf;
> +       t[0].len = msg->len;
> +       if (copy_from_user(tx_buf, (void __user *)msg->tx_buf, msg->len)) {
> +               ret = -EFAULT;
> +               goto done;
> +       }
> +       if (num_msgs > 1) {
> +               msg++;
> +               if (msg->len > PENCTRL_MAX_MSG_LEN) {
> +                       ret = -EINVAL;
> +                       goto done;
> +               }
> +               t[1].rx_buf = rx_buf;
> +               t[1].len = msg->len;
> +       }
> +       spi_message_init_with_transfers(&m, t, num_msgs);

It seems there is no validation for the messages 3+.

> +       /* Perform the transfer */
> +       mutex_lock(&spi_lock);
> +       ret = spi_sync(spi, &m);
> +       mutex_unlock(&spi_lock);
> +
> +       if (ret || (num_msgs == 1))
> +               goto done;
> +
> +       if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len))
> +               ret = -EFAULT;

> +done:

out_unlock: ?

> +       spi_dev_put(spi);
> +       return ret;
> +}
> +
> +static int penctrl_open(struct inode *inode, struct file *filp)
> +{
> +       struct spi_device *spi;
> +       u8 current_cs;

> +       if (!penctrl)
> +               return -ENODEV;

Is it possible?

> +       filp->private_data = penctrl;
> +       current_cs = iminor(inode);
> +       spi = penctrl->spi;
> +       spi->chip_select = current_cs;

> +       spi->cs_gpiod = spi->controller->cs_gpiods[current_cs];

Hmm... Why do you need this one? Isn't it a job of SPI core?

> +       spi_setup(spi);
> +       return stream_open(inode, filp);
> +}

> +static int penctrl_regs_read(struct penctrl_device *penctrl, u32 reg, u32 *val)
> +{
> +       struct spi_device *spi = penctrl->spi;
> +       struct spi_transfer t[2] = {};
> +       struct spi_message m;
> +       u8 txbuf[3];
> +       u8 rxbuf[1];
> +       int ret;
> +
> +       txbuf[0] = PENCTRL_SPI_CMD_REGRD;
> +       txbuf[1] = reg;
> +       txbuf[2] = 0;
> +       t[0].tx_buf = txbuf;
> +       t[0].len = 3;

sizeof(txbuf) ?

> +       rxbuf[0] = 0;
> +       t[1].rx_buf = rxbuf;
> +       t[1].len = 1;

sizeof(rxbuf) ?

> +       spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
> +       ret = spi_sync(spi, &m);
> +       if (ret == 0)
> +               *val = rxbuf[0];
> +
> +       return ret;
> +}
> +
> +static int penctrl_regs_write(struct penctrl_device *penctrl, u32 reg, u32 val)
> +{
> +       struct spi_device *spi = penctrl->spi;
> +       struct spi_transfer t;
> +       struct spi_message m;
> +       u8 txbuf[4];
> +
> +       txbuf[0] = PENCTRL_SPI_CMD_REGWR;
> +       txbuf[1] = reg;
> +       txbuf[2] = val;
> +       txbuf[3] = 0;
> +
> +       t.tx_buf = txbuf;
> +       t.len = 4;

sizeof(txbuf) ?

> +       spi_message_init_with_transfers(&m, &t, 1);
> +       return spi_sync(spi, &m);
> +}
> +
> +static int penctrl_reset_assert(struct reset_controller_dev *rcdev,
> +                               unsigned long id)
> +{
> +       struct penctrl_device *penctrl =
> +               container_of(rcdev, struct penctrl_device, rcdev);
> +       struct spi_device *spi = penctrl->spi;
> +       unsigned int val;
> +       int ret;
> +
> +       mutex_lock(&spi_lock);
> +       spi->chip_select = 0;
> +       spi->cs_gpiod = spi->controller->cs_gpiods[0];
> +       spi_setup(spi);
> +       ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val);
> +       if (ret) {
> +               dev_err(&spi->dev, "error reading ctrl0 reg\n");
> +               goto done;
> +       }
> +
> +       val |= BIT(6);
> +       ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val);
> +       if (ret)
> +               dev_err(&spi->dev, "error writing ctrl0 reg\n");

> +done:

out_unlock: ?

> +       mutex_unlock(&spi_lock);
> +       return ret;
> +}
> +
> +static int penctrl_reset_deassert(struct reset_controller_dev *rcdev,
> +                                 unsigned long id)
> +{
> +       struct penctrl_device *penctrl =
> +               container_of(rcdev, struct penctrl_device, rcdev);
> +       struct spi_device *spi = penctrl->spi;
> +       unsigned int val;
> +       int ret;
> +
> +       mutex_lock(&spi_lock);
> +       spi->chip_select = 0;
> +       spi->cs_gpiod = spi->controller->cs_gpiods[0];
> +       spi_setup(spi);
> +       ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val);
> +       if (ret) {
> +               dev_err(&spi->dev, "error reading ctrl0 reg\n");
> +               goto done;
> +       }
> +
> +       val &= ~BIT(6);
> +       ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val);
> +       if (ret)
> +               dev_err(&spi->dev, "error writing ctrl0 reg\n");

> +done:

out_unlock: ?

> +       mutex_unlock(&spi_lock);
> +       return ret;
> +}

> +static int penctrl_spi_probe(struct spi_device *spi)
> +{
> +       struct device *dev;
> +       struct cdev *cdev;
> +       u32 num_cs;
> +       int ret;
> +       u32 cs;
> +
> +       ret = device_property_read_u32(spi->dev.parent, "num-cs", &num_cs);
> +       if (ret)
> +               return dev_err_probe(&spi->dev, ret,
> +                                    "number of chip-selects not defined\n");
> +
> +       ret = alloc_chrdev_region(&penctrl_devt, 0, num_cs, "penctrl");
> +       if (ret)
> +               return dev_err_probe(&spi->dev, ret,
> +                                    "failed to alloc chrdev region\n");
> +
> +       penctrl_class = class_create(THIS_MODULE, "penctrl");
> +       if (IS_ERR(penctrl_class)) {
> +               ret = dev_err_probe(&spi->dev, PTR_ERR(penctrl_class),
> +                                   "failed to create class\n");
> +               goto unregister_chrdev;
> +       }
> +
> +       cdev = cdev_alloc();
> +       if (!cdev) {
> +               ret = dev_err_probe(&spi->dev, -ENOMEM,
> +                                   "allocation of cdev failed\n");
> +               goto destroy_class;
> +       }
> +       cdev->owner = THIS_MODULE;
> +       cdev_init(cdev, &penctrl_fops);
> +
> +       ret = cdev_add(cdev, penctrl_devt, num_cs);
> +       if (ret) {
> +               ret = dev_err_probe(&spi->dev, ret,
> +                                   "register of cdev failed\n");
> +               goto free_cdev;
> +       }
> +
> +       /* Allocate driver data */
> +       penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
> +       if (!penctrl) {
> +               ret = -ENOMEM;
> +               goto free_cdev;
> +       }
> +       penctrl->spi = spi;
> +       mutex_init(&spi_lock);
> +
> +       /* Create a device for each chip select */
> +       for (cs = 0; cs < num_cs; cs++) {
> +               dev = device_create(penctrl_class,
> +                                   &spi->dev,
> +                                   MKDEV(MAJOR(penctrl_devt), cs),
> +                                   penctrl,
> +                                   "penctrl0.%d",
> +                                   cs);
> +               if (IS_ERR(dev)) {
> +                       ret = dev_err_probe(&spi->dev, PTR_ERR(dev),
> +                                           "error creating device\n");
> +                       goto destroy_device;
> +               }
> +               dev_dbg(&spi->dev, "created device major %u, minor %d\n",
> +                       MAJOR(penctrl_devt), cs);
> +       }
> +
> +       /* Register emmc hardware reset */
> +       penctrl->rcdev.nr_resets = 1;
> +       penctrl->rcdev.owner = THIS_MODULE;
> +       penctrl->rcdev.dev = &spi->dev;
> +       penctrl->rcdev.ops = &penctrl_reset_ops;

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

Either redundant or wrong. Shouldn't you first have the firmware node
to be set for spi->dev?

> +       device_set_node(&spi->dev, dev_fwnode(dev));
> +
> +       ret = reset_controller_register(&penctrl->rcdev);
> +       if (ret)
> +               return dev_err_probe(&spi->dev, ret,
> +                                    "failed to register reset controller\n");
> +       return 0;
> +
> +destroy_device:
> +       for (cs = 0; cs < num_cs; cs++)
> +               device_destroy(penctrl_class, MKDEV(MAJOR(penctrl_devt), cs));
> +       kfree(penctrl);
> +free_cdev:
> +       cdev_del(cdev);
> +destroy_class:
> +       class_destroy(penctrl_class);
> +unregister_chrdev:
> +       unregister_chrdev(MAJOR(penctrl_devt), "penctrl");
> +
> +       return ret;
> +}

...

> +++ b/include/uapi/linux/amd-pensando-ctrl.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace interface for /dev/penctrl
> + *
> + * This file can be used by applications that need to communicate
> + * with the AMD Pensando SoC controller device via the ioctl interface.
> + */
> +#ifndef _UAPI_LINUX_AMD_PENSANDO_CTRL_H
> +#define _UAPI_LINUX_AMD_PENSANDO_CTRL_H

> +#include <linux/ioctl.h>

Not used header.

> +#include <linux/types.h>
> +
> +#define PENCTRL_SPI_CMD_REGRD  0x0b
> +#define PENCTRL_SPI_CMD_REGWR  0x02
> +#define PENCTRL_IOC_MAGIC      'k'
> +#define PENCTRL_IOC_MAXNR      0
> +#define PENCTRL_MAX_MSG_LEN    16
> +#define PENCTRL_MAX_REG                0xff
> +#define PENCTRL_REG_CTRL0      0x10
> +
> +struct penctrl_spi_xfer {
> +       __u64 tx_buf;
> +       __u64 rx_buf;
> +       __u32 len;
> +       __u32 speed_hz;
> +       __u64 compat;
> +};
> +
> +#endif /* _UAPI_LINUX_AMD_PENSANDO_CTRL_H */
Mark Brown April 17, 2023, 7:28 p.m. UTC | #2
On Mon, 10 Apr 2023 11:45:11 -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!

[03/15] dt-bindings: spi: cdns: Add compatible for AMD Pensando Elba SoC
        (no commit info)
[04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller
        commit: 6282a6ceef62f5732082f691de8f82fcd49d4fb4
[09/15] spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC
        (no commit info)
[10/15] spi: dw: Add support for AMD Pensando Elba SoC
        commit: 2c8606040a808aa01d2d9e4f5b9332e87bb66377

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
Brad Larson April 20, 2023, 10:52 p.m. UTC | #3
Hi Andy,

Thanks for the additional review.

On Tue, Apr 11, 2023 at 12:20:43 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Apr 10, 2023 at 9:48 PM Brad Larson <blarson@amd.com> wrote:
>>
>> The Pensando SoC controller is a SPI connected companion device
>> that is present in all Pensando SoC board designs.  The essential
>> board management registers are accessed on chip select 0 with
>> board mgmt IO support accessed using additional chip selects.
>
> ...
>
>> +#include <linux/cdev.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/spi/spi.h>
>
> + Blank line?

Added blank line

>> +#include <linux/amd-pensando-ctrl.h>
>
> ...
>
>> +struct penctrl_device {
>> +       struct spi_device *spi;
>> +       struct reset_controller_dev rcdev;
>
> Try to swap them and check if the code will be smaller (it depends on
> how often one or another member is being used),

Reversed the order to reduced code size by 8 bytes.

>> +};
>
> ...
>
>> +static long
>> +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> +{
>> +       void __user *in_arg = (void __user *)arg;
>> +       struct penctrl_device *penctrl;
>> +       u8 tx_buf[PENCTRL_MAX_MSG_LEN];
>> +       u8 rx_buf[PENCTRL_MAX_MSG_LEN];
>> +       struct spi_transfer t[2] = {};
>> +       struct penctrl_spi_xfer *msg;
>> +       struct spi_device *spi;
>> +       unsigned int num_msgs;
>> +       struct spi_message m;
>> +       u32 size;
>> +       int ret;
>> +
>> +       /* Check for a valid command */
>> +       if (_IOC_TYPE(cmd) != PENCTRL_IOC_MAGIC)
>> +               return -ENOTTY;
>> +
>> +       if (_IOC_NR(cmd) > PENCTRL_IOC_MAXNR)
>> +               return -ENOTTY;
>> +
>> +       if (_IOC_DIR(cmd) & _IOC_READ)
>> +               ret = !access_ok(in_arg, _IOC_SIZE(cmd));
>> +       else if (_IOC_DIR(cmd) & _IOC_WRITE)
>> +               ret = !access_ok(in_arg, _IOC_SIZE(cmd));
>
>> +
>
> Unneeded blank line.
>
>> +       if (ret)
>> +               return -EFAULT;
>
> But it seems you can actually rewrite above in less lines:
>
>       if ((_IOC_DIR(cmd) & _IOC_READ) && !access_ok(in_arg, _IOC_SIZE(cmd)))
>         return -EFAULT;
>
>       if ((_IOC_DIR(cmd) & _IOC_WRITE) && !access_ok(in_arg, _IOC_SIZE(cmd)))
>         return -EFAULT;

Yes, changed to save a line.

>> +       /* Get a reference to the SPI device */
>> +       penctrl = filp->private_data;
>> +       if (!penctrl)
>> +               return -ESHUTDOWN;
>> +
>> +       spi = spi_dev_get(penctrl->spi);
>> +       if (!spi)
>> +               return -ESHUTDOWN;
>> +
>> +       /* Verify and prepare SPI message */
>> +       size = _IOC_SIZE(cmd);
>> +       num_msgs = size / sizeof(struct penctrl_spi_xfer);
>> +       if (size == 0 || size % sizeof(struct penctrl_spi_xfer)) {
>> +               ret = -EINVAL;
>> +               goto done;
>> +       }
>> +       msg = memdup_user((struct penctrl_spi_xfer *)arg, size);
>
>> +       if (!msg) {
>> +               ret = PTR_ERR(msg);
>
> This is strange.

Yes, changed to

        msg = memdup_user((struct penctrl_spi_xfer *)arg, size);
        if (IS_ERR(msg)) {
                ret = PTR_ERR(msg);
                goto out_unlock;
        }

>> +               goto done;
>> +       }
>> +       if (msg->len > PENCTRL_MAX_MSG_LEN) {
>> +               ret = -EINVAL;
>> +               goto done;
>> +       }
>> +
>> +       t[0].tx_buf = tx_buf;
>> +       t[0].len = msg->len;
>> +       if (copy_from_user(tx_buf, (void __user *)msg->tx_buf, msg->len)) {
>> +               ret = -EFAULT;
>> +               goto done;
>> +       }
>> +       if (num_msgs > 1) {
>> +               msg++;
>> +               if (msg->len > PENCTRL_MAX_MSG_LEN) {
>> +                       ret = -EINVAL;
>> +                       goto done;
>> +               }
>> +               t[1].rx_buf = rx_buf;
>> +               t[1].len = msg->len;
>> +       }
>> +       spi_message_init_with_transfers(&m, t, num_msgs);
>
> It seems there is no validation for the messages 3+.

The device doesn't support and applications don't use num_msgs > 2, added this check here

        /* Verify and prepare SPI message */
        size = _IOC_SIZE(cmd);
        num_msgs = size / sizeof(struct penctrl_spi_xfer);
        if (num_msgs > 2 || size == 0 || size % sizeof(struct penctrl_spi_xfer)) {
                ret = -EINVAL;
                goto out_unlock;
        }

>> +       /* Perform the transfer */
>> +       mutex_lock(&spi_lock);
>> +       ret = spi_sync(spi, &m);
>> +       mutex_unlock(&spi_lock);
>> +
>> +       if (ret || (num_msgs == 1))
>> +               goto done;
>> +
>> +       if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len))
>> +               ret = -EFAULT;
>
>> +done:
>
> out_unlock: ?

Changed to out_unlock

>> +       spi_dev_put(spi);
>> +       return ret;
>> +}
>> +
>> +static int penctrl_open(struct inode *inode, struct file *filp)
>> +{
>> +       struct spi_device *spi;
>> +       u8 current_cs;
>
>> +       if (!penctrl)
>> +               return -ENODEV;
>
> Is it possible?

No, removed as a non-existent device can't be opened.

>> +       filp->private_data = penctrl;
>> +       current_cs = iminor(inode);
>> +       spi = penctrl->spi;
>> +       spi->chip_select = current_cs;
>
>> +       spi->cs_gpiod = spi->controller->cs_gpiods[current_cs];
>
> Hmm... Why do you need this one? Isn't it a job of SPI core?

When the four device tree nodes, one per cs, was squashed into the parent the
SPI core no longer handles this and the driver needs to do it. 

>> +       spi_setup(spi);
>> +       return stream_open(inode, filp);
>> +}
>
>> +static int penctrl_regs_read(struct penctrl_device *penctrl, u32 reg, u32 *val)
>> +{
>> +       struct spi_device *spi = penctrl->spi;
>> +       struct spi_transfer t[2] = {};
>> +       struct spi_message m;
>> +       u8 txbuf[3];
>> +       u8 rxbuf[1];
>> +       int ret;
>> +
>> +       txbuf[0] = PENCTRL_SPI_CMD_REGRD;
>> +       txbuf[1] = reg;
>> +       txbuf[2] = 0;
>> +       t[0].tx_buf = txbuf;
>> +       t[0].len = 3;
>
> sizeof(txbuf) ?

Changed to sizeof()

>> +       rxbuf[0] = 0;
>> +       t[1].rx_buf = rxbuf;
>> +       t[1].len = 1;
>
> sizeof(rxbuf) ?

Changed to sizeof()

>> +       spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
>> +       ret = spi_sync(spi, &m);
>> +       if (ret == 0)
>> +               *val = rxbuf[0];
>> +
>> +       return ret;
>> +}
>> +
>> +static int penctrl_regs_write(struct penctrl_device *penctrl, u32 reg, u32 val)
>> +{
>> +       struct spi_device *spi = penctrl->spi;
>> +       struct spi_transfer t;
>> +       struct spi_message m;
>> +       u8 txbuf[4];
>> +
>> +       txbuf[0] = PENCTRL_SPI_CMD_REGWR;
>> +       txbuf[1] = reg;
>> +       txbuf[2] = val;
>> +       txbuf[3] = 0;
>> +
>> +       t.tx_buf = txbuf;
>> +       t.len = 4;
>
> sizeof(txbuf) ?

Changed to sizeof()

>> +       spi_message_init_with_transfers(&m, &t, 1);
>> +       return spi_sync(spi, &m);
>> +}
>> +
>> +static int penctrl_reset_assert(struct reset_controller_dev *rcdev,
>> +                               unsigned long id)
>> +{
>> +       struct penctrl_device *penctrl =
>> +               container_of(rcdev, struct penctrl_device, rcdev);
>> +       struct spi_device *spi = penctrl->spi;
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       mutex_lock(&spi_lock);
>> +       spi->chip_select = 0;
>> +       spi->cs_gpiod = spi->controller->cs_gpiods[0];
>> +       spi_setup(spi);
>> +       ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val);
>> +       if (ret) {
>> +               dev_err(&spi->dev, "error reading ctrl0 reg\n");
>> +               goto done;
>> +       }
>> +
>> +       val |= BIT(6);
>> +       ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val);
>> +       if (ret)
>> +               dev_err(&spi->dev, "error writing ctrl0 reg\n");
>
>> +done:
>
> out_unlock: ?

Changed to out_unlock

>> +       mutex_unlock(&spi_lock);
>> +       return ret;
>> +}
>> +
>> +static int penctrl_reset_deassert(struct reset_controller_dev *rcdev,
>> +                                 unsigned long id)
>> +{
>> +       struct penctrl_device *penctrl =
>> +               container_of(rcdev, struct penctrl_device, rcdev);
>> +       struct spi_device *spi = penctrl->spi;
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       mutex_lock(&spi_lock);
>> +       spi->chip_select = 0;
>> +       spi->cs_gpiod = spi->controller->cs_gpiods[0];
>> +       spi_setup(spi);
>> +       ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val);
>> +       if (ret) {
>> +               dev_err(&spi->dev, "error reading ctrl0 reg\n");
>> +               goto done;
>> +       }
>> +
>> +       val &= ~BIT(6);
>> +       ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val);
>> +       if (ret)
>> +               dev_err(&spi->dev, "error writing ctrl0 reg\n");
>
>> +done:
>
> out_unlock: ?

Changed to out_unlock

>> +       mutex_unlock(&spi_lock);
>> +       return ret;
>> +}
>
>> +static int penctrl_spi_probe(struct spi_device *spi)
>> +{
>> +       struct device *dev;
>> +       struct cdev *cdev;
>> +       u32 num_cs;
>> +       int ret;
>> +       u32 cs;
>> +
>> +       ret = device_property_read_u32(spi->dev.parent, "num-cs", &num_cs);
>> +       if (ret)
>> +               return dev_err_probe(&spi->dev, ret,
>> +                                    "number of chip-selects not defined\n");
>> +
>> +       ret = alloc_chrdev_region(&penctrl_devt, 0, num_cs, "penctrl");
>> +       if (ret)
>> +               return dev_err_probe(&spi->dev, ret,
>> +                                    "failed to alloc chrdev region\n");
>> +
>> +       penctrl_class = class_create(THIS_MODULE, "penctrl");
>> +       if (IS_ERR(penctrl_class)) {
>> +               ret = dev_err_probe(&spi->dev, PTR_ERR(penctrl_class),
>> +                                   "failed to create class\n");
>> +               goto unregister_chrdev;
>> +       }
>> +
>> +       cdev = cdev_alloc();
>> +       if (!cdev) {
>> +               ret = dev_err_probe(&spi->dev, -ENOMEM,
>> +                                   "allocation of cdev failed\n");
>> +               goto destroy_class;
>> +       }
>> +       cdev->owner = THIS_MODULE;
>> +       cdev_init(cdev, &penctrl_fops);
>> +
>> +       ret = cdev_add(cdev, penctrl_devt, num_cs);
>> +       if (ret) {
>> +               ret = dev_err_probe(&spi->dev, ret,
>> +                                   "register of cdev failed\n");
>> +               goto free_cdev;
>> +       }
>> +
>> +       /* Allocate driver data */
>> +       penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
>> +       if (!penctrl) {
>> +               ret = -ENOMEM;
>> +               goto free_cdev;
>> +       }
>> +       penctrl->spi = spi;
>> +       mutex_init(&spi_lock);
>> +
>> +       /* Create a device for each chip select */
>> +       for (cs = 0; cs < num_cs; cs++) {
>> +               dev = device_create(penctrl_class,
>> +                                   &spi->dev,
>> +                                   MKDEV(MAJOR(penctrl_devt), cs),
>> +                                   penctrl,
>> +                                   "penctrl0.%d",
>> +                                   cs);
>> +               if (IS_ERR(dev)) {
>> +                       ret = dev_err_probe(&spi->dev, PTR_ERR(dev),
>> +                                           "error creating device\n");
>> +                       goto destroy_device;
>> +               }
>> +               dev_dbg(&spi->dev, "created device major %u, minor %d\n",
>> +                       MAJOR(penctrl_devt), cs);
>> +       }
>> +
>> +       /* Register emmc hardware reset */
>> +       penctrl->rcdev.nr_resets = 1;
>> +       penctrl->rcdev.owner = THIS_MODULE;
>> +       penctrl->rcdev.dev = &spi->dev;
>> +       penctrl->rcdev.ops = &penctrl_reset_ops;
>
>> +       penctrl->rcdev.of_node = spi->dev.of_node;
>
> Either redundant or wrong. Shouldn't you first have the firmware node
> to be set for spi->dev?

The spi device firmware node is set on entry to penctrl_spi_probe().  Just the
reset controller of_node needs to be set like this

        penctrl->rcdev.dev = &spi->dev;
        penctrl->rcdev.ops = &penctrl_reset_ops;
        penctrl->rcdev.owner = THIS_MODULE;
        penctrl->rcdev.of_node = spi->dev.of_node;
        penctrl->rcdev.nr_resets = 1;

        ret = reset_controller_register(&penctrl->rcdev);

which is similar to other reset controllers for example reset-sunplus.c:

static int sp_reset_probe(struct platform_device *pdev)
{
        struct device *dev = &pdev->dev;
...
        reset->rcdev.ops = &sp_reset_ops;
        reset->rcdev.owner = THIS_MODULE;
        reset->rcdev.of_node = dev->of_node;
        reset->rcdev.nr_resets = resource_size(res) / 4 * BITS_PER_HWM_REG;

        ret = devm_reset_controller_register(dev, &reset->rcdev);
}

for of_node at the same level as dev in reset_controller_dev 

struct reset_controller_dev {
        const struct reset_control_ops *ops;
	...
        struct device *dev;
        struct device_node *of_node;
	...
};

>> +       device_set_node(&spi->dev, dev_fwnode(dev));
>> +
>> +       ret = reset_controller_register(&penctrl->rcdev);
>> +       if (ret)
>> +               return dev_err_probe(&spi->dev, ret,
>> +                                    "failed to register reset controller\n");
>> +       return 0;
>> +
>> +destroy_device:
>> +       for (cs = 0; cs < num_cs; cs++)
>> +               device_destroy(penctrl_class, MKDEV(MAJOR(penctrl_devt), cs));
>> +       kfree(penctrl);
>> +free_cdev:
>> +       cdev_del(cdev);
>> +destroy_class:
>> +       class_destroy(penctrl_class);
>> +unregister_chrdev:
>> +       unregister_chrdev(MAJOR(penctrl_devt), "penctrl");
>> +
>> +       return ret;
>> +}
>
> ...
>
>> +++ b/include/uapi/linux/amd-pensando-ctrl.h
>> @@ -0,0 +1,30 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Userspace interface for /dev/penctrl
>> + *
>> + * This file can be used by applications that need to communicate
>> + * with the AMD Pensando SoC controller device via the ioctl interface.
>> + */
>> +#ifndef _UAPI_LINUX_AMD_PENSANDO_CTRL_H
>> +#define _UAPI_LINUX_AMD_PENSANDO_CTRL_H
>
>> +#include <linux/ioctl.h>
>
> Not used header.

Removed

>> +#include <linux/types.h>
>> +
>> +#define PENCTRL_SPI_CMD_REGRD  0x0b
>> +#define PENCTRL_SPI_CMD_REGWR  0x02
>> +#define PENCTRL_IOC_MAGIC      'k'
>> +#define PENCTRL_IOC_MAXNR      0
>> +#define PENCTRL_MAX_MSG_LEN    16
>> +#define PENCTRL_MAX_REG                0xff
>> +#define PENCTRL_REG_CTRL0      0x10
>> +
>> +struct penctrl_spi_xfer {
>> +       __u64 tx_buf;
>> +       __u64 rx_buf;
>> +       __u32 len;
>> +       __u32 speed_hz;
>> +       __u64 compat;
>> +};
>> +
>> +#endif /* _UAPI_LINUX_AMD_PENSANDO_CTRL_H */