diff mbox series

[v2,07/15] peci: Add peci-aspeed controller driver

Message ID 20210803113134.2262882-8-iwona.winiarska@intel.com (mailing list archive)
State Not Applicable
Headers show
Series Introduce PECI subsystem | expand

Commit Message

Winiarska, Iwona Aug. 3, 2021, 11:31 a.m. UTC
From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>

ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical
interface (a.k.a PECI wire).

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 MAINTAINERS                           |   9 +
 drivers/peci/Kconfig                  |   6 +
 drivers/peci/Makefile                 |   3 +
 drivers/peci/controller/Kconfig       |  16 +
 drivers/peci/controller/Makefile      |   3 +
 drivers/peci/controller/peci-aspeed.c | 445 ++++++++++++++++++++++++++
 6 files changed, 482 insertions(+)
 create mode 100644 drivers/peci/controller/Kconfig
 create mode 100644 drivers/peci/controller/Makefile
 create mode 100644 drivers/peci/controller/peci-aspeed.c

Comments

Dan Williams Aug. 26, 2021, 1:35 a.m. UTC | #1
On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
<iwona.winiarska@intel.com> wrote:
>
> From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>
> ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical
> interface (a.k.a PECI wire).

Maybe a one sentence blurb here and in the Kconfig reminding people
why they should care if they have a PECI driver or not?

>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com>
> Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  MAINTAINERS                           |   9 +
>  drivers/peci/Kconfig                  |   6 +
>  drivers/peci/Makefile                 |   3 +
>  drivers/peci/controller/Kconfig       |  16 +
>  drivers/peci/controller/Makefile      |   3 +
>  drivers/peci/controller/peci-aspeed.c | 445 ++++++++++++++++++++++++++
>  6 files changed, 482 insertions(+)
>  create mode 100644 drivers/peci/controller/Kconfig
>  create mode 100644 drivers/peci/controller/Makefile
>  create mode 100644 drivers/peci/controller/peci-aspeed.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d411974aaa5e..6e9d53ff68ab 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2866,6 +2866,15 @@ S:       Maintained
>  F:     Documentation/hwmon/asc7621.rst
>  F:     drivers/hwmon/asc7621.c
>
> +ASPEED PECI CONTROLLER
> +M:     Iwona Winiarska <iwona.winiarska@intel.com>
> +M:     Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> +L:     linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
> +L:     openbmc@lists.ozlabs.org (moderated for non-subscribers)
> +S:     Supported
> +F:     Documentation/devicetree/bindings/peci/peci-aspeed.yaml
> +F:     drivers/peci/controller/peci-aspeed.c
> +
>  ASPEED PINCTRL DRIVERS
>  M:     Andrew Jeffery <andrew@aj.id.au>
>  L:     linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
> diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> index 71a4ad81225a..99279df97a78 100644
> --- a/drivers/peci/Kconfig
> +++ b/drivers/peci/Kconfig
> @@ -13,3 +13,9 @@ menuconfig PECI
>
>           This support is also available as a module. If so, the module
>           will be called peci.
> +
> +if PECI
> +
> +source "drivers/peci/controller/Kconfig"
> +
> +endif # PECI
> diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
> index e789a354e842..926d8df15cbd 100644
> --- a/drivers/peci/Makefile
> +++ b/drivers/peci/Makefile
> @@ -3,3 +3,6 @@
>  # Core functionality
>  peci-y := core.o
>  obj-$(CONFIG_PECI) += peci.o
> +
> +# Hardware specific bus drivers
> +obj-y += controller/
> diff --git a/drivers/peci/controller/Kconfig b/drivers/peci/controller/Kconfig
> new file mode 100644
> index 000000000000..6d48df08db1c
> --- /dev/null
> +++ b/drivers/peci/controller/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config PECI_ASPEED
> +       tristate "ASPEED PECI support"
> +       depends on ARCH_ASPEED || COMPILE_TEST
> +       depends on OF
> +       depends on HAS_IOMEM
> +       help
> +         This option enables PECI controller driver for ASPEED AST2400,
> +         AST2500 and AST2600 SoCs.
> +
> +         Say Y here if your system runs on ASPEED SoC and you are using it
> +         as BMC for Intel platform.
> +
> +         This driver can also be built as a module. If so, the module will
> +         be called peci-aspeed.
> diff --git a/drivers/peci/controller/Makefile b/drivers/peci/controller/Makefile
> new file mode 100644
> index 000000000000..022c28ef1bf0
> --- /dev/null
> +++ b/drivers/peci/controller/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_PECI_ASPEED)      += peci-aspeed.o
> diff --git a/drivers/peci/controller/peci-aspeed.c b/drivers/peci/controller/peci-aspeed.c
> new file mode 100644
> index 000000000000..1d708c983749
> --- /dev/null
> +++ b/drivers/peci/controller/peci-aspeed.c
> @@ -0,0 +1,445 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (C) 2012-2017 ASPEED Technology Inc.
> +// Copyright (c) 2018-2021 Intel Corporation

Why different copyright capitalization?

> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/peci.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +#include <asm/unaligned.h>

Why is this included?

> +
> +/* ASPEED PECI Registers */
> +/* Control Register */
> +#define ASPEED_PECI_CTRL                       0x00
> +#define   ASPEED_PECI_CTRL_SAMPLING_MASK       GENMASK(19, 16)
> +#define   ASPEED_PECI_CTRL_RD_MODE_MASK                GENMASK(13, 12)
> +#define     ASPEED_PECI_CTRL_RD_MODE_DBG       BIT(13)
> +#define     ASPEED_PECI_CTRL_RD_MODE_COUNT     BIT(12)
> +#define   ASPEED_PECI_CTRL_CLK_SOURCE          BIT(11)
> +#define   ASPEED_PECI_CTRL_CLK_DIV_MASK                GENMASK(10, 8)
> +#define   ASPEED_PECI_CTRL_INVERT_OUT          BIT(7)
> +#define   ASPEED_PECI_CTRL_INVERT_IN           BIT(6)
> +#define   ASPEED_PECI_CTRL_BUS_CONTENTION_EN   BIT(5)
> +#define   ASPEED_PECI_CTRL_PECI_EN             BIT(4)
> +#define   ASPEED_PECI_CTRL_PECI_CLK_EN         BIT(0)
> +
> +/* Timing Negotiation Register */
> +#define ASPEED_PECI_TIMING_NEGOTIATION         0x04
> +#define   ASPEED_PECI_T_NEGO_MSG_MASK          GENMASK(15, 8)
> +#define   ASPEED_PECI_T_NEGO_ADDR_MASK         GENMASK(7, 0)
> +
> +/* Command Register */
> +#define ASPEED_PECI_CMD                                0x08
> +#define   ASPEED_PECI_CMD_PIN_MONITORING       BIT(31)
> +#define   ASPEED_PECI_CMD_STS_MASK             GENMASK(27, 24)
> +#define     ASPEED_PECI_CMD_STS_ADDR_T_NEGO    0x3
> +#define   ASPEED_PECI_CMD_IDLE_MASK            \
> +         (ASPEED_PECI_CMD_STS_MASK | ASPEED_PECI_CMD_PIN_MONITORING)
> +#define   ASPEED_PECI_CMD_FIRE                 BIT(0)
> +
> +/* Read/Write Length Register */
> +#define ASPEED_PECI_RW_LENGTH                  0x0c
> +#define   ASPEED_PECI_AW_FCS_EN                        BIT(31)
> +#define   ASPEED_PECI_RD_LEN_MASK              GENMASK(23, 16)
> +#define   ASPEED_PECI_WR_LEN_MASK              GENMASK(15, 8)
> +#define   ASPEED_PECI_TARGET_ADDR_MASK         GENMASK(7, 0)
> +
> +/* Expected FCS Data Register */
> +#define ASPEED_PECI_EXPECTED_FCS               0x10
> +#define   ASPEED_PECI_EXPECTED_RD_FCS_MASK     GENMASK(23, 16)
> +#define   ASPEED_PECI_EXPECTED_AW_FCS_AUTO_MASK        GENMASK(15, 8)
> +#define   ASPEED_PECI_EXPECTED_WR_FCS_MASK     GENMASK(7, 0)
> +
> +/* Captured FCS Data Register */
> +#define ASPEED_PECI_CAPTURED_FCS               0x14
> +#define   ASPEED_PECI_CAPTURED_RD_FCS_MASK     GENMASK(23, 16)
> +#define   ASPEED_PECI_CAPTURED_WR_FCS_MASK     GENMASK(7, 0)
> +
> +/* Interrupt Register */
> +#define ASPEED_PECI_INT_CTRL                   0x18
> +#define   ASPEED_PECI_TIMING_NEGO_SEL_MASK     GENMASK(31, 30)
> +#define     ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO   0
> +#define     ASPEED_PECI_2ND_BIT_OF_ADDR_NEGO   1
> +#define     ASPEED_PECI_MESSAGE_NEGO           2
> +#define   ASPEED_PECI_INT_MASK                 GENMASK(4, 0)
> +#define     ASPEED_PECI_INT_BUS_TIMEOUT                BIT(4)
> +#define     ASPEED_PECI_INT_BUS_CONTENTION     BIT(3)
> +#define     ASPEED_PECI_INT_WR_FCS_BAD         BIT(2)
> +#define     ASPEED_PECI_INT_WR_FCS_ABORT       BIT(1)
> +#define     ASPEED_PECI_INT_CMD_DONE           BIT(0)
> +
> +/* Interrupt Status Register */
> +#define ASPEED_PECI_INT_STS                    0x1c
> +#define   ASPEED_PECI_INT_TIMING_RESULT_MASK   GENMASK(29, 16)
> +         /* bits[4..0]: Same bit fields in the 'Interrupt Register' */
> +
> +/* Rx/Tx Data Buffer Registers */
> +#define ASPEED_PECI_WR_DATA0                   0x20
> +#define ASPEED_PECI_WR_DATA1                   0x24
> +#define ASPEED_PECI_WR_DATA2                   0x28
> +#define ASPEED_PECI_WR_DATA3                   0x2c
> +#define ASPEED_PECI_RD_DATA0                   0x30
> +#define ASPEED_PECI_RD_DATA1                   0x34
> +#define ASPEED_PECI_RD_DATA2                   0x38
> +#define ASPEED_PECI_RD_DATA3                   0x3c
> +#define ASPEED_PECI_WR_DATA4                   0x40
> +#define ASPEED_PECI_WR_DATA5                   0x44
> +#define ASPEED_PECI_WR_DATA6                   0x48
> +#define ASPEED_PECI_WR_DATA7                   0x4c
> +#define ASPEED_PECI_RD_DATA4                   0x50
> +#define ASPEED_PECI_RD_DATA5                   0x54
> +#define ASPEED_PECI_RD_DATA6                   0x58
> +#define ASPEED_PECI_RD_DATA7                   0x5c
> +#define   ASPEED_PECI_DATA_BUF_SIZE_MAX                32
> +
> +/* Timing Negotiation */
> +#define ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT  8
> +#define ASPEED_PECI_RD_SAMPLING_POINT_MAX      (BIT(4) - 1)
> +#define ASPEED_PECI_CLK_DIV_DEFAULT            0
> +#define ASPEED_PECI_CLK_DIV_MAX                        (BIT(3) - 1)
> +#define ASPEED_PECI_MSG_TIMING_DEFAULT         1
> +#define ASPEED_PECI_MSG_TIMING_MAX             (BIT(8) - 1)
> +#define ASPEED_PECI_ADDR_TIMING_DEFAULT                1
> +#define ASPEED_PECI_ADDR_TIMING_MAX            (BIT(8) - 1)
> +
> +/* Timeout */
> +#define ASPEED_PECI_IDLE_CHECK_TIMEOUT_US      (50 * USEC_PER_MSEC)
> +#define ASPEED_PECI_IDLE_CHECK_INTERVAL_US     (10 * USEC_PER_MSEC)
> +#define ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT     (1000)
> +#define ASPEED_PECI_CMD_TIMEOUT_MS_MAX         (1000)
> +
> +struct aspeed_peci {
> +       struct peci_controller *controller;
> +       struct device *dev;
> +       void __iomem *base;
> +       struct clk *clk;
> +       struct reset_control *rst;
> +       int irq;
> +       spinlock_t lock; /* to sync completion status handling */
> +       struct completion xfer_complete;
> +       u32 status;
> +       u32 cmd_timeout_ms;
> +       u32 msg_timing;
> +       u32 addr_timing;
> +       u32 rd_sampling_point;
> +       u32 clk_div;
> +};
> +
> +static void aspeed_peci_init_regs(struct aspeed_peci *priv)
> +{
> +       u32 val;
> +
> +       val = FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, ASPEED_PECI_CLK_DIV_DEFAULT);
> +       val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
> +       writel(val, priv->base + ASPEED_PECI_CTRL);
> +       /*
> +        * Timing negotiation period setting.
> +        * The unit of the programmed value is 4 times of PECI clock period.
> +        */
> +       val = FIELD_PREP(ASPEED_PECI_T_NEGO_MSG_MASK, priv->msg_timing);
> +       val |= FIELD_PREP(ASPEED_PECI_T_NEGO_ADDR_MASK, priv->addr_timing);
> +       writel(val, priv->base + ASPEED_PECI_TIMING_NEGOTIATION);
> +
> +       /* Clear interrupts */
> +       val = readl(priv->base + ASPEED_PECI_INT_STS) | ASPEED_PECI_INT_MASK;
> +       writel(val, priv->base + ASPEED_PECI_INT_STS);
> +
> +       /* Set timing negotiation mode and enable interrupts */
> +       val = FIELD_PREP(ASPEED_PECI_TIMING_NEGO_SEL_MASK, ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO);
> +       val |= ASPEED_PECI_INT_MASK;
> +       writel(val, priv->base + ASPEED_PECI_INT_CTRL);
> +
> +       val = FIELD_PREP(ASPEED_PECI_CTRL_SAMPLING_MASK, priv->rd_sampling_point);
> +       val |= FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, priv->clk_div);
> +       val |= ASPEED_PECI_CTRL_PECI_EN;
> +       val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
> +       writel(val, priv->base + ASPEED_PECI_CTRL);
> +}
> +
> +static inline int aspeed_peci_check_idle(struct aspeed_peci *priv)
> +{
> +       u32 cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
> +
> +       if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts) == ASPEED_PECI_CMD_STS_ADDR_T_NEGO)
> +               aspeed_peci_init_regs(priv);
> +
> +       return readl_poll_timeout(priv->base + ASPEED_PECI_CMD,
> +                                 cmd_sts,
> +                                 !(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK),
> +                                 ASPEED_PECI_IDLE_CHECK_INTERVAL_US,
> +                                 ASPEED_PECI_IDLE_CHECK_TIMEOUT_US);
> +}
> +
> +static int aspeed_peci_xfer(struct peci_controller *controller,
> +                           u8 addr, struct peci_request *req)
> +{
> +       struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent);
> +       unsigned long flags, timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
> +       u32 peci_head;
> +       int ret;
> +
> +       if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
> +           req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
> +               return -EINVAL;
> +
> +       /* Check command sts and bus idle state */
> +       ret = aspeed_peci_check_idle(priv);
> +       if (ret)
> +               return ret; /* -ETIMEDOUT */
> +
> +       spin_lock_irqsave(&priv->lock, flags);
> +       reinit_completion(&priv->xfer_complete);
> +
> +       peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) |
> +                   FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) |
> +                   FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len);
> +
> +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
> +
> +       memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf, min_t(u8, req->tx.len, 16));
> +       if (req->tx.len > 16)
> +               memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf + 16,
> +                           req->tx.len - 16);
> +
> +       dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
> +       print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req->tx.len);

On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of
reading through this buffer, but skip emitting it. Are you sure you
want to pay that overhead for every transaction?

> +
> +       priv->status = 0;
> +       writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
> +       spin_unlock_irqrestore(&priv->lock, flags);
> +
> +       ret = wait_for_completion_interruptible_timeout(&priv->xfer_complete, timeout);

spin_lock_irqsave() says "I don't know if interrupts are disabled
already, so I'll save the state, whatever it is, and restore later"

wait_for_completion_interruptible_timeout() says "I know I am in a
sleepable context where interrupts are enabled"

So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()?


> +       if (ret < 0)
> +               return ret;
> +
> +       if (ret == 0) {
> +               dev_dbg(priv->dev, "Timeout waiting for a response!\n");
> +               return -ETIMEDOUT;
> +       }
> +
> +       spin_lock_irqsave(&priv->lock, flags);
> +
> +       writel(0, priv->base + ASPEED_PECI_CMD);
> +
> +       if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
> +               spin_unlock_irqrestore(&priv->lock, flags);
> +               dev_dbg(priv->dev, "No valid response!\n");
> +               return -EIO;
> +       }
> +
> +       spin_unlock_irqrestore(&priv->lock, flags);
> +
> +       memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0, min_t(u8, req->rx.len, 16));
> +       if (req->rx.len > 16)
> +               memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_RD_DATA4,
> +                             req->rx.len - 16);
> +
> +       print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);
> +
> +       return 0;
> +}
> +
> +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
> +{
> +       struct aspeed_peci *priv = arg;
> +       u32 status;
> +
> +       spin_lock(&priv->lock);
> +       status = readl(priv->base + ASPEED_PECI_INT_STS);
> +       writel(status, priv->base + ASPEED_PECI_INT_STS);
> +       priv->status |= (status & ASPEED_PECI_INT_MASK);
> +
> +       /*
> +        * In most cases, interrupt bits will be set one by one but also note
> +        * that multiple interrupt bits could be set at the same time.
> +        */
> +       if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_TIMEOUT\n");
> +
> +       if (status & ASPEED_PECI_INT_BUS_CONTENTION)
> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONTENTION\n");
> +
> +       if (status & ASPEED_PECI_INT_WR_FCS_BAD)
> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_WR_FCS_BAD\n");
> +
> +       if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_WR_FCS_ABORT\n");

Are you sure these would not be better as tracepoints? If you're
debugging an interrupt related failure, the ratelimiting might get in
your way when you really need to know when one of these error
interrupts fire relative to another event.

> +
> +       /*
> +        * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE bit
> +        * set even in an error case.
> +        */
> +       if (status & ASPEED_PECI_INT_CMD_DONE)
> +               complete(&priv->xfer_complete);

Hmm, no need to check if there was a sequencing error, like a command
was never submitted?

> +
> +       spin_unlock(&priv->lock);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void aspeed_peci_property_sanitize(struct device *dev, const char *propname,
> +                                         u32 min, u32 max, u32 default_val, u32 *propval)
> +{
> +       u32 val;
> +       int ret;
> +
> +       ret = device_property_read_u32(dev, propname, &val);
> +       if (ret) {
> +               val = default_val;
> +       } else if (val > max || val < min) {
> +               dev_warn(dev, "Invalid %s: %u, falling back to: %u\n",
> +                        propname, val, default_val);
> +
> +               val = default_val;
> +       }
> +
> +       *propval = val;
> +}
> +
> +static void aspeed_peci_property_setup(struct aspeed_peci *priv)
> +{
> +       aspeed_peci_property_sanitize(priv->dev, "aspeed,clock-divider",
> +                                     0, ASPEED_PECI_CLK_DIV_MAX,
> +                                     ASPEED_PECI_CLK_DIV_DEFAULT, &priv->clk_div);
> +       aspeed_peci_property_sanitize(priv->dev, "aspeed,msg-timing",
> +                                     0, ASPEED_PECI_MSG_TIMING_MAX,
> +                                     ASPEED_PECI_MSG_TIMING_DEFAULT, &priv->msg_timing);
> +       aspeed_peci_property_sanitize(priv->dev, "aspeed,addr-timing",
> +                                     0, ASPEED_PECI_ADDR_TIMING_MAX,
> +                                     ASPEED_PECI_ADDR_TIMING_DEFAULT, &priv->addr_timing);
> +       aspeed_peci_property_sanitize(priv->dev, "aspeed,rd-sampling-point",
> +                                     0, ASPEED_PECI_RD_SAMPLING_POINT_MAX,
> +                                     ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT,
> +                                     &priv->rd_sampling_point);
> +       aspeed_peci_property_sanitize(priv->dev, "cmd-timeout-ms",
> +                                     1, ASPEED_PECI_CMD_TIMEOUT_MS_MAX,
> +                                     ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT, &priv->cmd_timeout_ms);
> +}
> +
> +static struct peci_controller_ops aspeed_ops = {
> +       .xfer = aspeed_peci_xfer,
> +};
> +
> +static void aspeed_peci_reset_control_release(void *data)
> +{
> +       reset_control_assert(data);
> +}
> +
> +int aspeed_peci_reset_control_deassert(struct device *dev, struct reset_control *rst)

I'd recommend naming this devm_aspeed_peci_reset_control_deassert(),
because I came looking here from reading probe for why there was no
reassertion of reset on driver ->remove().

> +{
> +       int ret;
> +
> +       ret = reset_control_deassert(rst);
> +       if (ret)
> +               return ret;
> +
> +       return devm_add_action_or_reset(dev, aspeed_peci_reset_control_release, rst);
> +}
> +
> +static void aspeed_peci_clk_release(void *data)
> +{
> +       clk_disable_unprepare(data);
> +}
> +
> +static int aspeed_peci_clk_enable(struct device *dev, struct clk *clk)

...ditto on the devm prefix, just to speed readability.

> +{
> +       int ret;
> +
> +       ret = clk_prepare_enable(clk);
> +       if (ret)
> +               return ret;
> +
> +       return devm_add_action_or_reset(dev, aspeed_peci_clk_release, clk);
> +}
> +
> +static int aspeed_peci_probe(struct platform_device *pdev)
> +{
> +       struct peci_controller *controller;
> +       struct aspeed_peci *priv;
> +       int ret;
> +
> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->dev = &pdev->dev;
> +       dev_set_drvdata(priv->dev, priv);
> +
> +       priv->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(priv->base))
> +               return PTR_ERR(priv->base);
> +
> +       priv->irq = platform_get_irq(pdev, 0);
> +       if (!priv->irq)
> +               return priv->irq;
> +
> +       ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler,
> +                              0, "peci-aspeed", priv);
> +       if (ret)
> +               return ret;
> +
> +       init_completion(&priv->xfer_complete);
> +       spin_lock_init(&priv->lock);
> +
> +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> +       if (IS_ERR(priv->rst))
> +               return dev_err_probe(priv->dev, PTR_ERR(priv->rst),
> +                                    "failed to get reset control\n");
> +
> +       ret = aspeed_peci_reset_control_deassert(priv->dev, priv->rst);
> +       if (ret)
> +               return dev_err_probe(priv->dev, ret, "cannot deassert reset control\n");
> +
> +       priv->clk = devm_clk_get(priv->dev, NULL);
> +       if (IS_ERR(priv->clk))
> +               return dev_err_probe(priv->dev, PTR_ERR(priv->clk), "failed to get clk\n");
> +
> +       ret = aspeed_peci_clk_enable(priv->dev, priv->clk);
> +       if (ret)
> +               return dev_err_probe(priv->dev, ret, "failed to enable clock\n");
> +
> +       aspeed_peci_property_setup(priv);
> +
> +       aspeed_peci_init_regs(priv);
> +
> +       controller = devm_peci_controller_add(priv->dev, &aspeed_ops);
> +       if (IS_ERR(controller))
> +               return dev_err_probe(priv->dev, PTR_ERR(controller),
> +                                    "failed to add aspeed peci controller\n");
> +
> +       priv->controller = controller;
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id aspeed_peci_of_table[] = {
> +       { .compatible = "aspeed,ast2400-peci", },
> +       { .compatible = "aspeed,ast2500-peci", },
> +       { .compatible = "aspeed,ast2600-peci", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
> +
> +static struct platform_driver aspeed_peci_driver = {
> +       .probe  = aspeed_peci_probe,
> +       .driver = {
> +               .name           = "peci-aspeed",
> +               .of_match_table = aspeed_peci_of_table,
> +       },
> +};
> +module_platform_driver(aspeed_peci_driver);
> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
> +MODULE_DESCRIPTION("ASPEED PECI driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PECI);
> --
> 2.31.1
>
Winiarska, Iwona Aug. 26, 2021, 11:54 p.m. UTC | #2
On Wed, 2021-08-25 at 18:35 -0700, Dan Williams wrote:
> On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
> <iwona.winiarska@intel.com> wrote:
> > 
> > From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > 
> > ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical
> > interface (a.k.a PECI wire).
> 
> Maybe a one sentence blurb here and in the Kconfig reminding people
> why they should care if they have a PECI driver or not?

Ok, I'll expand it a bit.

> 
> > 
> > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com>
> > Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > ---
> >  MAINTAINERS                           |   9 +
> >  drivers/peci/Kconfig                  |   6 +
> >  drivers/peci/Makefile                 |   3 +
> >  drivers/peci/controller/Kconfig       |  16 +
> >  drivers/peci/controller/Makefile      |   3 +
> >  drivers/peci/controller/peci-aspeed.c | 445 ++++++++++++++++++++++++++
> >  6 files changed, 482 insertions(+)
> >  create mode 100644 drivers/peci/controller/Kconfig
> >  create mode 100644 drivers/peci/controller/Makefile
> >  create mode 100644 drivers/peci/controller/peci-aspeed.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d411974aaa5e..6e9d53ff68ab 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2866,6 +2866,15 @@ S:       Maintained
> >  F:     Documentation/hwmon/asc7621.rst
> >  F:     drivers/hwmon/asc7621.c
> > 
> > +ASPEED PECI CONTROLLER
> > +M:     Iwona Winiarska <iwona.winiarska@intel.com>
> > +M:     Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > +L:     linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
> > +L:     openbmc@lists.ozlabs.org (moderated for non-subscribers)
> > +S:     Supported
> > +F:     Documentation/devicetree/bindings/peci/peci-aspeed.yaml
> > +F:     drivers/peci/controller/peci-aspeed.c
> > +
> >  ASPEED PINCTRL DRIVERS
> >  M:     Andrew Jeffery <andrew@aj.id.au>
> >  L:     linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
> > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> > index 71a4ad81225a..99279df97a78 100644
> > --- a/drivers/peci/Kconfig
> > +++ b/drivers/peci/Kconfig
> > @@ -13,3 +13,9 @@ menuconfig PECI
> > 
> >           This support is also available as a module. If so, the module
> >           will be called peci.
> > +
> > +if PECI
> > +
> > +source "drivers/peci/controller/Kconfig"
> > +
> > +endif # PECI
> > diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
> > index e789a354e842..926d8df15cbd 100644
> > --- a/drivers/peci/Makefile
> > +++ b/drivers/peci/Makefile
> > @@ -3,3 +3,6 @@
> >  # Core functionality
> >  peci-y := core.o
> >  obj-$(CONFIG_PECI) += peci.o
> > +
> > +# Hardware specific bus drivers
> > +obj-y += controller/
> > diff --git a/drivers/peci/controller/Kconfig
> > b/drivers/peci/controller/Kconfig
> > new file mode 100644
> > index 000000000000..6d48df08db1c
> > --- /dev/null
> > +++ b/drivers/peci/controller/Kconfig
> > @@ -0,0 +1,16 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config PECI_ASPEED
> > +       tristate "ASPEED PECI support"
> > +       depends on ARCH_ASPEED || COMPILE_TEST
> > +       depends on OF
> > +       depends on HAS_IOMEM
> > +       help
> > +         This option enables PECI controller driver for ASPEED AST2400,
> > +         AST2500 and AST2600 SoCs.
> > +
> > +         Say Y here if your system runs on ASPEED SoC and you are using it
> > +         as BMC for Intel platform.
> > +
> > +         This driver can also be built as a module. If so, the module will
> > +         be called peci-aspeed.
> > diff --git a/drivers/peci/controller/Makefile
> > b/drivers/peci/controller/Makefile
> > new file mode 100644
> > index 000000000000..022c28ef1bf0
> > --- /dev/null
> > +++ b/drivers/peci/controller/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +obj-$(CONFIG_PECI_ASPEED)      += peci-aspeed.o
> > diff --git a/drivers/peci/controller/peci-aspeed.c
> > b/drivers/peci/controller/peci-aspeed.c
> > new file mode 100644
> > index 000000000000..1d708c983749
> > --- /dev/null
> > +++ b/drivers/peci/controller/peci-aspeed.c
> > @@ -0,0 +1,445 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// Copyright (C) 2012-2017 ASPEED Technology Inc.
> > +// Copyright (c) 2018-2021 Intel Corporation
> 
> Why different copyright capitalization?

I'll make them consistent.

> 
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/peci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +
> > +#include <asm/unaligned.h>
> 
> Why is this included?

Leftover - I'll remove it.

> 
> > +
> > +/* ASPEED PECI Registers */
> > +/* Control Register */
> > +#define ASPEED_PECI_CTRL                       0x00
> > +#define   ASPEED_PECI_CTRL_SAMPLING_MASK       GENMASK(19, 16)
> > +#define   ASPEED_PECI_CTRL_RD_MODE_MASK                GENMASK(13, 12)
> > +#define     ASPEED_PECI_CTRL_RD_MODE_DBG       BIT(13)
> > +#define     ASPEED_PECI_CTRL_RD_MODE_COUNT     BIT(12)
> > +#define   ASPEED_PECI_CTRL_CLK_SOURCE          BIT(11)
> > +#define   ASPEED_PECI_CTRL_CLK_DIV_MASK                GENMASK(10, 8)
> > +#define   ASPEED_PECI_CTRL_INVERT_OUT          BIT(7)
> > +#define   ASPEED_PECI_CTRL_INVERT_IN           BIT(6)
> > +#define   ASPEED_PECI_CTRL_BUS_CONTENTION_EN   BIT(5)
> > +#define   ASPEED_PECI_CTRL_PECI_EN             BIT(4)
> > +#define   ASPEED_PECI_CTRL_PECI_CLK_EN         BIT(0)
> > +
> > +/* Timing Negotiation Register */
> > +#define ASPEED_PECI_TIMING_NEGOTIATION         0x04
> > +#define   ASPEED_PECI_T_NEGO_MSG_MASK          GENMASK(15, 8)
> > +#define   ASPEED_PECI_T_NEGO_ADDR_MASK         GENMASK(7, 0)
> > +
> > +/* Command Register */
> > +#define ASPEED_PECI_CMD                                0x08
> > +#define   ASPEED_PECI_CMD_PIN_MONITORING       BIT(31)
> > +#define   ASPEED_PECI_CMD_STS_MASK             GENMASK(27, 24)
> > +#define     ASPEED_PECI_CMD_STS_ADDR_T_NEGO    0x3
> > +#define   ASPEED_PECI_CMD_IDLE_MASK            \
> > +         (ASPEED_PECI_CMD_STS_MASK | ASPEED_PECI_CMD_PIN_MONITORING)
> > +#define   ASPEED_PECI_CMD_FIRE                 BIT(0)
> > +
> > +/* Read/Write Length Register */
> > +#define ASPEED_PECI_RW_LENGTH                  0x0c
> > +#define   ASPEED_PECI_AW_FCS_EN                        BIT(31)
> > +#define   ASPEED_PECI_RD_LEN_MASK              GENMASK(23, 16)
> > +#define   ASPEED_PECI_WR_LEN_MASK              GENMASK(15, 8)
> > +#define   ASPEED_PECI_TARGET_ADDR_MASK         GENMASK(7, 0)
> > +
> > +/* Expected FCS Data Register */
> > +#define ASPEED_PECI_EXPECTED_FCS               0x10
> > +#define   ASPEED_PECI_EXPECTED_RD_FCS_MASK     GENMASK(23, 16)
> > +#define   ASPEED_PECI_EXPECTED_AW_FCS_AUTO_MASK        GENMASK(15, 8)
> > +#define   ASPEED_PECI_EXPECTED_WR_FCS_MASK     GENMASK(7, 0)
> > +
> > +/* Captured FCS Data Register */
> > +#define ASPEED_PECI_CAPTURED_FCS               0x14
> > +#define   ASPEED_PECI_CAPTURED_RD_FCS_MASK     GENMASK(23, 16)
> > +#define   ASPEED_PECI_CAPTURED_WR_FCS_MASK     GENMASK(7, 0)
> > +
> > +/* Interrupt Register */
> > +#define ASPEED_PECI_INT_CTRL                   0x18
> > +#define   ASPEED_PECI_TIMING_NEGO_SEL_MASK     GENMASK(31, 30)
> > +#define     ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO   0
> > +#define     ASPEED_PECI_2ND_BIT_OF_ADDR_NEGO   1
> > +#define     ASPEED_PECI_MESSAGE_NEGO           2
> > +#define   ASPEED_PECI_INT_MASK                 GENMASK(4, 0)
> > +#define     ASPEED_PECI_INT_BUS_TIMEOUT                BIT(4)
> > +#define     ASPEED_PECI_INT_BUS_CONTENTION     BIT(3)
> > +#define     ASPEED_PECI_INT_WR_FCS_BAD         BIT(2)
> > +#define     ASPEED_PECI_INT_WR_FCS_ABORT       BIT(1)
> > +#define     ASPEED_PECI_INT_CMD_DONE           BIT(0)
> > +
> > +/* Interrupt Status Register */
> > +#define ASPEED_PECI_INT_STS                    0x1c
> > +#define   ASPEED_PECI_INT_TIMING_RESULT_MASK   GENMASK(29, 16)
> > +         /* bits[4..0]: Same bit fields in the 'Interrupt Register' */
> > +
> > +/* Rx/Tx Data Buffer Registers */
> > +#define ASPEED_PECI_WR_DATA0                   0x20
> > +#define ASPEED_PECI_WR_DATA1                   0x24
> > +#define ASPEED_PECI_WR_DATA2                   0x28
> > +#define ASPEED_PECI_WR_DATA3                   0x2c
> > +#define ASPEED_PECI_RD_DATA0                   0x30
> > +#define ASPEED_PECI_RD_DATA1                   0x34
> > +#define ASPEED_PECI_RD_DATA2                   0x38
> > +#define ASPEED_PECI_RD_DATA3                   0x3c
> > +#define ASPEED_PECI_WR_DATA4                   0x40
> > +#define ASPEED_PECI_WR_DATA5                   0x44
> > +#define ASPEED_PECI_WR_DATA6                   0x48
> > +#define ASPEED_PECI_WR_DATA7                   0x4c
> > +#define ASPEED_PECI_RD_DATA4                   0x50
> > +#define ASPEED_PECI_RD_DATA5                   0x54
> > +#define ASPEED_PECI_RD_DATA6                   0x58
> > +#define ASPEED_PECI_RD_DATA7                   0x5c
> > +#define   ASPEED_PECI_DATA_BUF_SIZE_MAX                32
> > +
> > +/* Timing Negotiation */
> > +#define ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT  8
> > +#define ASPEED_PECI_RD_SAMPLING_POINT_MAX      (BIT(4) - 1)
> > +#define ASPEED_PECI_CLK_DIV_DEFAULT            0
> > +#define ASPEED_PECI_CLK_DIV_MAX                        (BIT(3) - 1)
> > +#define ASPEED_PECI_MSG_TIMING_DEFAULT         1
> > +#define ASPEED_PECI_MSG_TIMING_MAX             (BIT(8) - 1)
> > +#define ASPEED_PECI_ADDR_TIMING_DEFAULT                1
> > +#define ASPEED_PECI_ADDR_TIMING_MAX            (BIT(8) - 1)
> > +
> > +/* Timeout */
> > +#define ASPEED_PECI_IDLE_CHECK_TIMEOUT_US      (50 * USEC_PER_MSEC)
> > +#define ASPEED_PECI_IDLE_CHECK_INTERVAL_US     (10 * USEC_PER_MSEC)
> > +#define ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT     (1000)
> > +#define ASPEED_PECI_CMD_TIMEOUT_MS_MAX         (1000)
> > +
> > +struct aspeed_peci {
> > +       struct peci_controller *controller;
> > +       struct device *dev;
> > +       void __iomem *base;
> > +       struct clk *clk;
> > +       struct reset_control *rst;
> > +       int irq;
> > +       spinlock_t lock; /* to sync completion status handling */
> > +       struct completion xfer_complete;
> > +       u32 status;
> > +       u32 cmd_timeout_ms;
> > +       u32 msg_timing;
> > +       u32 addr_timing;
> > +       u32 rd_sampling_point;
> > +       u32 clk_div;
> > +};
> > +
> > +static void aspeed_peci_init_regs(struct aspeed_peci *priv)
> > +{
> > +       u32 val;
> > +
> > +       val = FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK,
> > ASPEED_PECI_CLK_DIV_DEFAULT);
> > +       val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
> > +       writel(val, priv->base + ASPEED_PECI_CTRL);
> > +       /*
> > +        * Timing negotiation period setting.
> > +        * The unit of the programmed value is 4 times of PECI clock period.
> > +        */
> > +       val = FIELD_PREP(ASPEED_PECI_T_NEGO_MSG_MASK, priv->msg_timing);
> > +       val |= FIELD_PREP(ASPEED_PECI_T_NEGO_ADDR_MASK, priv->addr_timing);
> > +       writel(val, priv->base + ASPEED_PECI_TIMING_NEGOTIATION);
> > +
> > +       /* Clear interrupts */
> > +       val = readl(priv->base + ASPEED_PECI_INT_STS) |
> > ASPEED_PECI_INT_MASK;
> > +       writel(val, priv->base + ASPEED_PECI_INT_STS);
> > +
> > +       /* Set timing negotiation mode and enable interrupts */
> > +       val = FIELD_PREP(ASPEED_PECI_TIMING_NEGO_SEL_MASK,
> > ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO);
> > +       val |= ASPEED_PECI_INT_MASK;
> > +       writel(val, priv->base + ASPEED_PECI_INT_CTRL);
> > +
> > +       val = FIELD_PREP(ASPEED_PECI_CTRL_SAMPLING_MASK, priv-
> > >rd_sampling_point);
> > +       val |= FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, priv->clk_div);
> > +       val |= ASPEED_PECI_CTRL_PECI_EN;
> > +       val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
> > +       writel(val, priv->base + ASPEED_PECI_CTRL);
> > +}
> > +
> > +static inline int aspeed_peci_check_idle(struct aspeed_peci *priv)
> > +{
> > +       u32 cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
> > +
> > +       if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts) ==
> > ASPEED_PECI_CMD_STS_ADDR_T_NEGO)
> > +               aspeed_peci_init_regs(priv);
> > +
> > +       return readl_poll_timeout(priv->base + ASPEED_PECI_CMD,
> > +                                 cmd_sts,
> > +                                 !(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK),
> > +                                 ASPEED_PECI_IDLE_CHECK_INTERVAL_US,
> > +                                 ASPEED_PECI_IDLE_CHECK_TIMEOUT_US);
> > +}
> > +
> > +static int aspeed_peci_xfer(struct peci_controller *controller,
> > +                           u8 addr, struct peci_request *req)
> > +{
> > +       struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent);
> > +       unsigned long flags, timeout = msecs_to_jiffies(priv-
> > >cmd_timeout_ms);
> > +       u32 peci_head;
> > +       int ret;
> > +
> > +       if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
> > +           req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
> > +               return -EINVAL;
> > +
> > +       /* Check command sts and bus idle state */
> > +       ret = aspeed_peci_check_idle(priv);
> > +       if (ret)
> > +               return ret; /* -ETIMEDOUT */
> > +
> > +       spin_lock_irqsave(&priv->lock, flags);
> > +       reinit_completion(&priv->xfer_complete);
> > +
> > +       peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) |
> > +                   FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) |
> > +                   FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len);
> > +
> > +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
> > +
> > +       memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf,
> > min_t(u8, req->tx.len, 16));
> > +       if (req->tx.len > 16)
> > +               memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf +
> > 16,
> > +                           req->tx.len - 16);
> > +
> > +       dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
> > +       print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req-
> > >tx.len);
> 
> On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of
> reading through this buffer, but skip emitting it. Are you sure you
> want to pay that overhead for every transaction?

I can remove it or I can add something like:

#if IS_ENABLED(CONFIG_PECI_DEBUG)
#define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__)
#else
#define peci_debug(...) do { } while (0)
#endif

(and similar peci_trace with trace_printk for usage in IRQ handlers and such).

What do you think?

> 
> > +
> > +       priv->status = 0;
> > +       writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
> > +       spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +       ret = wait_for_completion_interruptible_timeout(&priv-
> > >xfer_complete, timeout);
> 
> spin_lock_irqsave() says "I don't know if interrupts are disabled
> already, so I'll save the state, whatever it is, and restore later"
> 
> wait_for_completion_interruptible_timeout() says "I know I am in a
> sleepable context where interrupts are enabled"
> 
> So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()?

You're right - I'll fix it.

> 
> 
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (ret == 0) {
> > +               dev_dbg(priv->dev, "Timeout waiting for a response!\n");
> > +               return -ETIMEDOUT;
> > +       }
> > +
> > +       spin_lock_irqsave(&priv->lock, flags);
> > +
> > +       writel(0, priv->base + ASPEED_PECI_CMD);
> > +
> > +       if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
> > +               spin_unlock_irqrestore(&priv->lock, flags);
> > +               dev_dbg(priv->dev, "No valid response!\n");
> > +               return -EIO;
> > +       }
> > +
> > +       spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +       memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0,
> > min_t(u8, req->rx.len, 16));
> > +       if (req->rx.len > 16)
> > +               memcpy_fromio(req->rx.buf + 16, priv->base +
> > ASPEED_PECI_RD_DATA4,
> > +                             req->rx.len - 16);
> > +
> > +       print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req-
> > >rx.len);
> > +
> > +       return 0;
> > +}
> > +
> > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
> > +{
> > +       struct aspeed_peci *priv = arg;
> > +       u32 status;
> > +
> > +       spin_lock(&priv->lock);
> > +       status = readl(priv->base + ASPEED_PECI_INT_STS);
> > +       writel(status, priv->base + ASPEED_PECI_INT_STS);
> > +       priv->status |= (status & ASPEED_PECI_INT_MASK);
> > +
> > +       /*
> > +        * In most cases, interrupt bits will be set one by one but also
> > note
> > +        * that multiple interrupt bits could be set at the same time.
> > +        */
> > +       if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
> > +               dev_dbg_ratelimited(priv->dev,
> > "ASPEED_PECI_INT_BUS_TIMEOUT\n");
> > +
> > +       if (status & ASPEED_PECI_INT_BUS_CONTENTION)
> > +               dev_dbg_ratelimited(priv->dev,
> > "ASPEED_PECI_INT_BUS_CONTENTION\n");
> > +
> > +       if (status & ASPEED_PECI_INT_WR_FCS_BAD)
> > +               dev_dbg_ratelimited(priv->dev,
> > "ASPEED_PECI_INT_WR_FCS_BAD\n");
> > +
> > +       if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
> > +               dev_dbg_ratelimited(priv->dev,
> > "ASPEED_PECI_INT_WR_FCS_ABORT\n");
> 
> Are you sure these would not be better as tracepoints? If you're
> debugging an interrupt related failure, the ratelimiting might get in
> your way when you really need to know when one of these error
> interrupts fire relative to another event.

Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ status
would probably be too much.
I was thinking about something like trace_printk hidden under a
"CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future
improvement?

> 
> > +
> > +       /*
> > +        * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE
> > bit
> > +        * set even in an error case.
> > +        */
> > +       if (status & ASPEED_PECI_INT_CMD_DONE)
> > +               complete(&priv->xfer_complete);
> 
> Hmm, no need to check if there was a sequencing error, like a command
> was never submitted?

It's handled by checking if HW is idle in xfer before a command is sent, where
we just expect a single interrupt per command.

> 
> > +
> > +       spin_unlock(&priv->lock);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static void aspeed_peci_property_sanitize(struct device *dev, const char
> > *propname,
> > +                                         u32 min, u32 max, u32 default_val,
> > u32 *propval)
> > +{
> > +       u32 val;
> > +       int ret;
> > +
> > +       ret = device_property_read_u32(dev, propname, &val);
> > +       if (ret) {
> > +               val = default_val;
> > +       } else if (val > max || val < min) {
> > +               dev_warn(dev, "Invalid %s: %u, falling back to: %u\n",
> > +                        propname, val, default_val);
> > +
> > +               val = default_val;
> > +       }
> > +
> > +       *propval = val;
> > +}
> > +
> > +static void aspeed_peci_property_setup(struct aspeed_peci *priv)
> > +{
> > +       aspeed_peci_property_sanitize(priv->dev, "aspeed,clock-divider",
> > +                                     0, ASPEED_PECI_CLK_DIV_MAX,
> > +                                     ASPEED_PECI_CLK_DIV_DEFAULT, &priv-
> > >clk_div);
> > +       aspeed_peci_property_sanitize(priv->dev, "aspeed,msg-timing",
> > +                                     0, ASPEED_PECI_MSG_TIMING_MAX,
> > +                                     ASPEED_PECI_MSG_TIMING_DEFAULT, &priv-
> > >msg_timing);
> > +       aspeed_peci_property_sanitize(priv->dev, "aspeed,addr-timing",
> > +                                     0, ASPEED_PECI_ADDR_TIMING_MAX,
> > +                                     ASPEED_PECI_ADDR_TIMING_DEFAULT,
> > &priv->addr_timing);
> > +       aspeed_peci_property_sanitize(priv->dev, "aspeed,rd-sampling-point",
> > +                                     0, ASPEED_PECI_RD_SAMPLING_POINT_MAX,
> > +                                     ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT,
> > +                                     &priv->rd_sampling_point);
> > +       aspeed_peci_property_sanitize(priv->dev, "cmd-timeout-ms",
> > +                                     1, ASPEED_PECI_CMD_TIMEOUT_MS_MAX,
> > +                                     ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT,
> > &priv->cmd_timeout_ms);
> > +}
> > +
> > +static struct peci_controller_ops aspeed_ops = {
> > +       .xfer = aspeed_peci_xfer,
> > +};
> > +
> > +static void aspeed_peci_reset_control_release(void *data)
> > +{
> > +       reset_control_assert(data);
> > +}
> > +
> > +int aspeed_peci_reset_control_deassert(struct device *dev, struct
> > reset_control *rst)
> 
> I'd recommend naming this devm_aspeed_peci_reset_control_deassert(),
> because I came looking here from reading probe for why there was no
> reassertion of reset on driver ->remove().

Ok.

> 
> > +{
> > +       int ret;
> > +
> > +       ret = reset_control_deassert(rst);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return devm_add_action_or_reset(dev,
> > aspeed_peci_reset_control_release, rst);
> > +}
> > +
> > +static void aspeed_peci_clk_release(void *data)
> > +{
> > +       clk_disable_unprepare(data);
> > +}
> > +
> > +static int aspeed_peci_clk_enable(struct device *dev, struct clk *clk)
> 
> ...ditto on the devm prefix, just to speed readability.

Ok.

Thanks
-Iwona

> 
> > +{
> > +       int ret;
> > +
> > +       ret = clk_prepare_enable(clk);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return devm_add_action_or_reset(dev, aspeed_peci_clk_release, clk);
> > +}
> > +
> > +static int aspeed_peci_probe(struct platform_device *pdev)
> > +{
> > +       struct peci_controller *controller;
> > +       struct aspeed_peci *priv;
> > +       int ret;
> > +
> > +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       priv->dev = &pdev->dev;
> > +       dev_set_drvdata(priv->dev, priv);
> > +
> > +       priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(priv->base))
> > +               return PTR_ERR(priv->base);
> > +
> > +       priv->irq = platform_get_irq(pdev, 0);
> > +       if (!priv->irq)
> > +               return priv->irq;
> > +
> > +       ret = devm_request_irq(&pdev->dev, priv->irq,
> > aspeed_peci_irq_handler,
> > +                              0, "peci-aspeed", priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       init_completion(&priv->xfer_complete);
> > +       spin_lock_init(&priv->lock);
> > +
> > +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> > +       if (IS_ERR(priv->rst))
> > +               return dev_err_probe(priv->dev, PTR_ERR(priv->rst),
> > +                                    "failed to get reset control\n");
> > +
> > +       ret = aspeed_peci_reset_control_deassert(priv->dev, priv->rst);
> > +       if (ret)
> > +               return dev_err_probe(priv->dev, ret, "cannot deassert reset
> > control\n");
> > +
> > +       priv->clk = devm_clk_get(priv->dev, NULL);
> > +       if (IS_ERR(priv->clk))
> > +               return dev_err_probe(priv->dev, PTR_ERR(priv->clk), "failed
> > to get clk\n");
> > +
> > +       ret = aspeed_peci_clk_enable(priv->dev, priv->clk);
> > +       if (ret)
> > +               return dev_err_probe(priv->dev, ret, "failed to enable
> > clock\n");
> > +
> > +       aspeed_peci_property_setup(priv);
> > +
> > +       aspeed_peci_init_regs(priv);
> > +
> > +       controller = devm_peci_controller_add(priv->dev, &aspeed_ops);
> > +       if (IS_ERR(controller))
> > +               return dev_err_probe(priv->dev, PTR_ERR(controller),
> > +                                    "failed to add aspeed peci
> > controller\n");
> > +
> > +       priv->controller = controller;
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id aspeed_peci_of_table[] = {
> > +       { .compatible = "aspeed,ast2400-peci", },
> > +       { .compatible = "aspeed,ast2500-peci", },
> > +       { .compatible = "aspeed,ast2600-peci", },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
> > +
> > +static struct platform_driver aspeed_peci_driver = {
> > +       .probe  = aspeed_peci_probe,
> > +       .driver = {
> > +               .name           = "peci-aspeed",
> > +               .of_match_table = aspeed_peci_of_table,
> > +       },
> > +};
> > +module_platform_driver(aspeed_peci_driver);
> > +
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> > +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
> > +MODULE_DESCRIPTION("ASPEED PECI driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS(PECI);
> > --
> > 2.31.1
> >
Dan Williams Aug. 27, 2021, 4:24 p.m. UTC | #3
On Thu, Aug 26, 2021 at 4:55 PM Winiarska, Iwona
<iwona.winiarska@intel.com> wrote:
>
> On Wed, 2021-08-25 at 18:35 -0700, Dan Williams wrote:
> > On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
> > <iwona.winiarska@intel.com> wrote:
> > >
> > > From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > >
> > > ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical
> > > interface (a.k.a PECI wire).
> >
> > Maybe a one sentence blurb here and in the Kconfig reminding people
> > why they should care if they have a PECI driver or not?
>
> Ok, I'll expand it a bit.
[..]
> > > +static int aspeed_peci_xfer(struct peci_controller *controller,
> > > +                           u8 addr, struct peci_request *req)
> > > +{
> > > +       struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent);
> > > +       unsigned long flags, timeout = msecs_to_jiffies(priv-
> > > >cmd_timeout_ms);
> > > +       u32 peci_head;
> > > +       int ret;
> > > +
> > > +       if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
> > > +           req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
> > > +               return -EINVAL;
> > > +
> > > +       /* Check command sts and bus idle state */
> > > +       ret = aspeed_peci_check_idle(priv);
> > > +       if (ret)
> > > +               return ret; /* -ETIMEDOUT */
> > > +
> > > +       spin_lock_irqsave(&priv->lock, flags);
> > > +       reinit_completion(&priv->xfer_complete);
> > > +
> > > +       peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) |
> > > +                   FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) |
> > > +                   FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len);
> > > +
> > > +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
> > > +
> > > +       memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf,
> > > min_t(u8, req->tx.len, 16));
> > > +       if (req->tx.len > 16)
> > > +               memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf +
> > > 16,
> > > +                           req->tx.len - 16);
> > > +
> > > +       dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
> > > +       print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req-
> > > >tx.len);
> >
> > On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of
> > reading through this buffer, but skip emitting it. Are you sure you
> > want to pay that overhead for every transaction?
>
> I can remove it or I can add something like:
>
> #if IS_ENABLED(CONFIG_PECI_DEBUG)
> #define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__)
> #else
> #define peci_debug(...) do { } while (0)
> #endif

It's the hex dump I'm worried about, not the debug statements as much.

I think the choices are remove the print_hex_dump_bytes(), put it
behind an IS_ENABLED(CONFIG_DYNAMIC_DEBUG) to ensure the overhead is
skipped in the CONFIG_DYNAMIC_DEBUG=n case, or live with the overhead
if this is not a fast path / infrequently used.

>
> (and similar peci_trace with trace_printk for usage in IRQ handlers and such).
>
> What do you think?

In general, no, don't wrap the base level print routines with
driver-specific ones. Also, trace_printk() is only for debug builds.
Note that trace points are built to be even less overhead than
dev_dbg(), so there's no overhead concern with disabled tracepoints,
they literally translate to nops when disabled.

>
> >
> > > +
> > > +       priv->status = 0;
> > > +       writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
> > > +       spin_unlock_irqrestore(&priv->lock, flags);
> > > +
> > > +       ret = wait_for_completion_interruptible_timeout(&priv-
> > > >xfer_complete, timeout);
> >
> > spin_lock_irqsave() says "I don't know if interrupts are disabled
> > already, so I'll save the state, whatever it is, and restore later"
> >
> > wait_for_completion_interruptible_timeout() says "I know I am in a
> > sleepable context where interrupts are enabled"
> >
> > So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()?
>
> You're right - I'll fix it.
>
> >
> >
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       if (ret == 0) {
> > > +               dev_dbg(priv->dev, "Timeout waiting for a response!\n");
> > > +               return -ETIMEDOUT;
> > > +       }
> > > +
> > > +       spin_lock_irqsave(&priv->lock, flags);
> > > +
> > > +       writel(0, priv->base + ASPEED_PECI_CMD);
> > > +
> > > +       if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
> > > +               spin_unlock_irqrestore(&priv->lock, flags);
> > > +               dev_dbg(priv->dev, "No valid response!\n");
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       spin_unlock_irqrestore(&priv->lock, flags);
> > > +
> > > +       memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0,
> > > min_t(u8, req->rx.len, 16));
> > > +       if (req->rx.len > 16)
> > > +               memcpy_fromio(req->rx.buf + 16, priv->base +
> > > ASPEED_PECI_RD_DATA4,
> > > +                             req->rx.len - 16);
> > > +
> > > +       print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req-
> > > >rx.len);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
> > > +{
> > > +       struct aspeed_peci *priv = arg;
> > > +       u32 status;
> > > +
> > > +       spin_lock(&priv->lock);
> > > +       status = readl(priv->base + ASPEED_PECI_INT_STS);
> > > +       writel(status, priv->base + ASPEED_PECI_INT_STS);
> > > +       priv->status |= (status & ASPEED_PECI_INT_MASK);
> > > +
> > > +       /*
> > > +        * In most cases, interrupt bits will be set one by one but also
> > > note
> > > +        * that multiple interrupt bits could be set at the same time.
> > > +        */
> > > +       if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
> > > +               dev_dbg_ratelimited(priv->dev,
> > > "ASPEED_PECI_INT_BUS_TIMEOUT\n");
> > > +
> > > +       if (status & ASPEED_PECI_INT_BUS_CONTENTION)
> > > +               dev_dbg_ratelimited(priv->dev,
> > > "ASPEED_PECI_INT_BUS_CONTENTION\n");
> > > +
> > > +       if (status & ASPEED_PECI_INT_WR_FCS_BAD)
> > > +               dev_dbg_ratelimited(priv->dev,
> > > "ASPEED_PECI_INT_WR_FCS_BAD\n");
> > > +
> > > +       if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
> > > +               dev_dbg_ratelimited(priv->dev,
> > > "ASPEED_PECI_INT_WR_FCS_ABORT\n");
> >
> > Are you sure these would not be better as tracepoints? If you're
> > debugging an interrupt related failure, the ratelimiting might get in
> > your way when you really need to know when one of these error
> > interrupts fire relative to another event.
>
> Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ status
> would probably be too much.

Tracepoints become ABI once someone ships tooling that depends on them
being there. These don't look  attractive for a tool, and they don't
look difficult to maintain if the interrupt handler needs to be
reworked. I.e. it would be trivial to keep a dead tracepoint around if
worse came to worse to keep a tool from failing to load.

> I was thinking about something like trace_printk hidden under a
> "CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future
> improvement?

Again trace_printk() is only for private builds.

>
> >
> > > +
> > > +       /*
> > > +        * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE
> > > bit
> > > +        * set even in an error case.
> > > +        */
> > > +       if (status & ASPEED_PECI_INT_CMD_DONE)
> > > +               complete(&priv->xfer_complete);
> >
> > Hmm, no need to check if there was a sequencing error, like a command
> > was never submitted?
>
> It's handled by checking if HW is idle in xfer before a command is sent, where
> we just expect a single interrupt per command.

I'm asking how do you determine if this status was spurious, or there
was a sequencing error in the driver?
Winiarska, Iwona Aug. 29, 2021, 7:42 p.m. UTC | #4
On Fri, 2021-08-27 at 09:24 -0700, Dan Williams wrote:
> On Thu, Aug 26, 2021 at 4:55 PM Winiarska, Iwona
> <iwona.winiarska@intel.com> wrote:
> > 
> > On Wed, 2021-08-25 at 18:35 -0700, Dan Williams wrote:
> > > On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
> > > <iwona.winiarska@intel.com> wrote:
> > > > 
> > > > From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > > > 
> > > > ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical
> > > > interface (a.k.a PECI wire).
> > > 
> > > Maybe a one sentence blurb here and in the Kconfig reminding people
> > > why they should care if they have a PECI driver or not?
> > 
> > Ok, I'll expand it a bit.
> [..]
> > > > +static int aspeed_peci_xfer(struct peci_controller *controller,
> > > > +                           u8 addr, struct peci_request *req)
> > > > +{
> > > > +       struct aspeed_peci *priv = dev_get_drvdata(controller-
> > > > >dev.parent);
> > > > +       unsigned long flags, timeout = msecs_to_jiffies(priv-
> > > > > cmd_timeout_ms);
> > > > +       u32 peci_head;
> > > > +       int ret;
> > > > +
> > > > +       if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
> > > > +           req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
> > > > +               return -EINVAL;
> > > > +
> > > > +       /* Check command sts and bus idle state */
> > > > +       ret = aspeed_peci_check_idle(priv);
> > > > +       if (ret)
> > > > +               return ret; /* -ETIMEDOUT */
> > > > +
> > > > +       spin_lock_irqsave(&priv->lock, flags);
> > > > +       reinit_completion(&priv->xfer_complete);
> > > > +
> > > > +       peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) |
> > > > +                   FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) |
> > > > +                   FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len);
> > > > +
> > > > +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
> > > > +
> > > > +       memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf,
> > > > min_t(u8, req->tx.len, 16));
> > > > +       if (req->tx.len > 16)
> > > > +               memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req-
> > > > >tx.buf +
> > > > 16,
> > > > +                           req->tx.len - 16);
> > > > +
> > > > +       dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
> > > > +       print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf,
> > > > req-
> > > > > tx.len);
> > > 
> > > On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of
> > > reading through this buffer, but skip emitting it. Are you sure you
> > > want to pay that overhead for every transaction?
> > 
> > I can remove it or I can add something like:
> > 
> > #if IS_ENABLED(CONFIG_PECI_DEBUG)
> > #define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__)
> > #else
> > #define peci_debug(...) do { } while (0)
> > #endif
> 
> It's the hex dump I'm worried about, not the debug statements as much.
> 
> I think the choices are remove the print_hex_dump_bytes(), put it
> behind an IS_ENABLED(CONFIG_DYNAMIC_DEBUG) to ensure the overhead is
> skipped in the CONFIG_DYNAMIC_DEBUG=n case, or live with the overhead
> if this is not a fast path / infrequently used.

I will place it behind IS_ENABLED(CONFIG_DYNAMIC_DEBUG).

> 
> > 
> > (and similar peci_trace with trace_printk for usage in IRQ handlers and
> > such).
> > 
> > What do you think?
> 
> In general, no, don't wrap the base level print routines with
> driver-specific ones. Also, trace_printk() is only for debug builds.
> Note that trace points are built to be even less overhead than
> dev_dbg(), so there's no overhead concern with disabled tracepoints,
> they literally translate to nops when disabled.

Ack.

> 
> > 
> > > 
> > > > +
> > > > +       priv->status = 0;
> > > > +       writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
> > > > +       spin_unlock_irqrestore(&priv->lock, flags);
> > > > +
> > > > +       ret = wait_for_completion_interruptible_timeout(&priv-
> > > > > xfer_complete, timeout);
> > > 
> > > spin_lock_irqsave() says "I don't know if interrupts are disabled
> > > already, so I'll save the state, whatever it is, and restore later"
> > > 
> > > wait_for_completion_interruptible_timeout() says "I know I am in a
> > > sleepable context where interrupts are enabled"
> > > 
> > > So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()?
> > 
> > You're right - I'll fix it.
> > 
> > > 
> > > 
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +
> > > > +       if (ret == 0) {
> > > > +               dev_dbg(priv->dev, "Timeout waiting for a response!\n");
> > > > +               return -ETIMEDOUT;
> > > > +       }
> > > > +
> > > > +       spin_lock_irqsave(&priv->lock, flags);
> > > > +
> > > > +       writel(0, priv->base + ASPEED_PECI_CMD);
> > > > +
> > > > +       if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
> > > > +               spin_unlock_irqrestore(&priv->lock, flags);
> > > > +               dev_dbg(priv->dev, "No valid response!\n");
> > > > +               return -EIO;
> > > > +       }
> > > > +
> > > > +       spin_unlock_irqrestore(&priv->lock, flags);
> > > > +
> > > > +       memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0,
> > > > min_t(u8, req->rx.len, 16));
> > > > +       if (req->rx.len > 16)
> > > > +               memcpy_fromio(req->rx.buf + 16, priv->base +
> > > > ASPEED_PECI_RD_DATA4,
> > > > +                             req->rx.len - 16);
> > > > +
> > > > +       print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf,
> > > > req-
> > > > > rx.len);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
> > > > +{
> > > > +       struct aspeed_peci *priv = arg;
> > > > +       u32 status;
> > > > +
> > > > +       spin_lock(&priv->lock);
> > > > +       status = readl(priv->base + ASPEED_PECI_INT_STS);
> > > > +       writel(status, priv->base + ASPEED_PECI_INT_STS);
> > > > +       priv->status |= (status & ASPEED_PECI_INT_MASK);
> > > > +
> > > > +       /*
> > > > +        * In most cases, interrupt bits will be set one by one but also
> > > > note
> > > > +        * that multiple interrupt bits could be set at the same time.
> > > > +        */
> > > > +       if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
> > > > +               dev_dbg_ratelimited(priv->dev,
> > > > "ASPEED_PECI_INT_BUS_TIMEOUT\n");
> > > > +
> > > > +       if (status & ASPEED_PECI_INT_BUS_CONTENTION)
> > > > +               dev_dbg_ratelimited(priv->dev,
> > > > "ASPEED_PECI_INT_BUS_CONTENTION\n");
> > > > +
> > > > +       if (status & ASPEED_PECI_INT_WR_FCS_BAD)
> > > > +               dev_dbg_ratelimited(priv->dev,
> > > > "ASPEED_PECI_INT_WR_FCS_BAD\n");
> > > > +
> > > > +       if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
> > > > +               dev_dbg_ratelimited(priv->dev,
> > > > "ASPEED_PECI_INT_WR_FCS_ABORT\n");
> > > 
> > > Are you sure these would not be better as tracepoints? If you're
> > > debugging an interrupt related failure, the ratelimiting might get in
> > > your way when you really need to know when one of these error
> > > interrupts fire relative to another event.
> > 
> > Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ
> > status
> > would probably be too much.
> 
> Tracepoints become ABI once someone ships tooling that depends on them
> being there. These don't look  attractive for a tool, and they don't
> look difficult to maintain if the interrupt handler needs to be
> reworked. I.e. it would be trivial to keep a dead tracepoint around if
> worse came to worse to keep a tool from failing to load.

After more consideration, I would prefer to remove these logs for now - in case
of error I'll log full status in xfer().

> 
> > I was thinking about something like trace_printk hidden under a
> > "CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future
> > improvement?
> 
> Again trace_printk() is only for private builds.
> 
> > 
> > > 
> > > > +
> > > > +       /*
> > > > +        * All commands should be ended up with a
> > > > ASPEED_PECI_INT_CMD_DONE
> > > > bit
> > > > +        * set even in an error case.
> > > > +        */
> > > > +       if (status & ASPEED_PECI_INT_CMD_DONE)
> > > > +               complete(&priv->xfer_complete);
> > > 
> > > Hmm, no need to check if there was a sequencing error, like a command
> > > was never submitted?
> > 
> > It's handled by checking if HW is idle in xfer before a command is sent,
> > where
> > we just expect a single interrupt per command.
> 
> I'm asking how do you determine if this status was spurious, or there
> was a sequencing error in the driver?

I don't think we have any means to determine it.
PECI itself doesn't provide any mechanism to verify it (there is no sequence
number or tag to match request/response).
We're relying on the fact that BMC is a requester and initiates communication
with CPU - the interrupt won't be generated if BMC doesn't send any request.

Thanks
-Iwona
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index d411974aaa5e..6e9d53ff68ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2866,6 +2866,15 @@  S:	Maintained
 F:	Documentation/hwmon/asc7621.rst
 F:	drivers/hwmon/asc7621.c
 
+ASPEED PECI CONTROLLER
+M:	Iwona Winiarska <iwona.winiarska@intel.com>
+M:	Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
+L:	linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
+L:	openbmc@lists.ozlabs.org (moderated for non-subscribers)
+S:	Supported
+F:	Documentation/devicetree/bindings/peci/peci-aspeed.yaml
+F:	drivers/peci/controller/peci-aspeed.c
+
 ASPEED PINCTRL DRIVERS
 M:	Andrew Jeffery <andrew@aj.id.au>
 L:	linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
index 71a4ad81225a..99279df97a78 100644
--- a/drivers/peci/Kconfig
+++ b/drivers/peci/Kconfig
@@ -13,3 +13,9 @@  menuconfig PECI
 
 	  This support is also available as a module. If so, the module
 	  will be called peci.
+
+if PECI
+
+source "drivers/peci/controller/Kconfig"
+
+endif # PECI
diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
index e789a354e842..926d8df15cbd 100644
--- a/drivers/peci/Makefile
+++ b/drivers/peci/Makefile
@@ -3,3 +3,6 @@ 
 # Core functionality
 peci-y := core.o
 obj-$(CONFIG_PECI) += peci.o
+
+# Hardware specific bus drivers
+obj-y += controller/
diff --git a/drivers/peci/controller/Kconfig b/drivers/peci/controller/Kconfig
new file mode 100644
index 000000000000..6d48df08db1c
--- /dev/null
+++ b/drivers/peci/controller/Kconfig
@@ -0,0 +1,16 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+
+config PECI_ASPEED
+	tristate "ASPEED PECI support"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	depends on OF
+	depends on HAS_IOMEM
+	help
+	  This option enables PECI controller driver for ASPEED AST2400,
+	  AST2500 and AST2600 SoCs.
+
+	  Say Y here if your system runs on ASPEED SoC and you are using it
+	  as BMC for Intel platform.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called peci-aspeed.
diff --git a/drivers/peci/controller/Makefile b/drivers/peci/controller/Makefile
new file mode 100644
index 000000000000..022c28ef1bf0
--- /dev/null
+++ b/drivers/peci/controller/Makefile
@@ -0,0 +1,3 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_PECI_ASPEED)	+= peci-aspeed.o
diff --git a/drivers/peci/controller/peci-aspeed.c b/drivers/peci/controller/peci-aspeed.c
new file mode 100644
index 000000000000..1d708c983749
--- /dev/null
+++ b/drivers/peci/controller/peci-aspeed.c
@@ -0,0 +1,445 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2012-2017 ASPEED Technology Inc.
+// Copyright (c) 2018-2021 Intel Corporation
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/peci.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+#include <asm/unaligned.h>
+
+/* ASPEED PECI Registers */
+/* Control Register */
+#define ASPEED_PECI_CTRL			0x00
+#define   ASPEED_PECI_CTRL_SAMPLING_MASK	GENMASK(19, 16)
+#define   ASPEED_PECI_CTRL_RD_MODE_MASK		GENMASK(13, 12)
+#define     ASPEED_PECI_CTRL_RD_MODE_DBG	BIT(13)
+#define     ASPEED_PECI_CTRL_RD_MODE_COUNT	BIT(12)
+#define   ASPEED_PECI_CTRL_CLK_SOURCE		BIT(11)
+#define   ASPEED_PECI_CTRL_CLK_DIV_MASK		GENMASK(10, 8)
+#define   ASPEED_PECI_CTRL_INVERT_OUT		BIT(7)
+#define   ASPEED_PECI_CTRL_INVERT_IN		BIT(6)
+#define   ASPEED_PECI_CTRL_BUS_CONTENTION_EN	BIT(5)
+#define   ASPEED_PECI_CTRL_PECI_EN		BIT(4)
+#define   ASPEED_PECI_CTRL_PECI_CLK_EN		BIT(0)
+
+/* Timing Negotiation Register */
+#define ASPEED_PECI_TIMING_NEGOTIATION		0x04
+#define   ASPEED_PECI_T_NEGO_MSG_MASK		GENMASK(15, 8)
+#define   ASPEED_PECI_T_NEGO_ADDR_MASK		GENMASK(7, 0)
+
+/* Command Register */
+#define ASPEED_PECI_CMD				0x08
+#define   ASPEED_PECI_CMD_PIN_MONITORING	BIT(31)
+#define   ASPEED_PECI_CMD_STS_MASK		GENMASK(27, 24)
+#define     ASPEED_PECI_CMD_STS_ADDR_T_NEGO	0x3
+#define   ASPEED_PECI_CMD_IDLE_MASK		\
+	  (ASPEED_PECI_CMD_STS_MASK | ASPEED_PECI_CMD_PIN_MONITORING)
+#define   ASPEED_PECI_CMD_FIRE			BIT(0)
+
+/* Read/Write Length Register */
+#define ASPEED_PECI_RW_LENGTH			0x0c
+#define   ASPEED_PECI_AW_FCS_EN			BIT(31)
+#define   ASPEED_PECI_RD_LEN_MASK		GENMASK(23, 16)
+#define   ASPEED_PECI_WR_LEN_MASK		GENMASK(15, 8)
+#define   ASPEED_PECI_TARGET_ADDR_MASK		GENMASK(7, 0)
+
+/* Expected FCS Data Register */
+#define ASPEED_PECI_EXPECTED_FCS		0x10
+#define   ASPEED_PECI_EXPECTED_RD_FCS_MASK	GENMASK(23, 16)
+#define   ASPEED_PECI_EXPECTED_AW_FCS_AUTO_MASK	GENMASK(15, 8)
+#define   ASPEED_PECI_EXPECTED_WR_FCS_MASK	GENMASK(7, 0)
+
+/* Captured FCS Data Register */
+#define ASPEED_PECI_CAPTURED_FCS		0x14
+#define   ASPEED_PECI_CAPTURED_RD_FCS_MASK	GENMASK(23, 16)
+#define   ASPEED_PECI_CAPTURED_WR_FCS_MASK	GENMASK(7, 0)
+
+/* Interrupt Register */
+#define ASPEED_PECI_INT_CTRL			0x18
+#define   ASPEED_PECI_TIMING_NEGO_SEL_MASK	GENMASK(31, 30)
+#define     ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO	0
+#define     ASPEED_PECI_2ND_BIT_OF_ADDR_NEGO	1
+#define     ASPEED_PECI_MESSAGE_NEGO		2
+#define   ASPEED_PECI_INT_MASK			GENMASK(4, 0)
+#define     ASPEED_PECI_INT_BUS_TIMEOUT		BIT(4)
+#define     ASPEED_PECI_INT_BUS_CONTENTION	BIT(3)
+#define     ASPEED_PECI_INT_WR_FCS_BAD		BIT(2)
+#define     ASPEED_PECI_INT_WR_FCS_ABORT	BIT(1)
+#define     ASPEED_PECI_INT_CMD_DONE		BIT(0)
+
+/* Interrupt Status Register */
+#define ASPEED_PECI_INT_STS			0x1c
+#define   ASPEED_PECI_INT_TIMING_RESULT_MASK	GENMASK(29, 16)
+	  /* bits[4..0]: Same bit fields in the 'Interrupt Register' */
+
+/* Rx/Tx Data Buffer Registers */
+#define ASPEED_PECI_WR_DATA0			0x20
+#define ASPEED_PECI_WR_DATA1			0x24
+#define ASPEED_PECI_WR_DATA2			0x28
+#define ASPEED_PECI_WR_DATA3			0x2c
+#define ASPEED_PECI_RD_DATA0			0x30
+#define ASPEED_PECI_RD_DATA1			0x34
+#define ASPEED_PECI_RD_DATA2			0x38
+#define ASPEED_PECI_RD_DATA3			0x3c
+#define ASPEED_PECI_WR_DATA4			0x40
+#define ASPEED_PECI_WR_DATA5			0x44
+#define ASPEED_PECI_WR_DATA6			0x48
+#define ASPEED_PECI_WR_DATA7			0x4c
+#define ASPEED_PECI_RD_DATA4			0x50
+#define ASPEED_PECI_RD_DATA5			0x54
+#define ASPEED_PECI_RD_DATA6			0x58
+#define ASPEED_PECI_RD_DATA7			0x5c
+#define   ASPEED_PECI_DATA_BUF_SIZE_MAX		32
+
+/* Timing Negotiation */
+#define ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT	8
+#define ASPEED_PECI_RD_SAMPLING_POINT_MAX	(BIT(4) - 1)
+#define ASPEED_PECI_CLK_DIV_DEFAULT		0
+#define ASPEED_PECI_CLK_DIV_MAX			(BIT(3) - 1)
+#define ASPEED_PECI_MSG_TIMING_DEFAULT		1
+#define ASPEED_PECI_MSG_TIMING_MAX		(BIT(8) - 1)
+#define ASPEED_PECI_ADDR_TIMING_DEFAULT		1
+#define ASPEED_PECI_ADDR_TIMING_MAX		(BIT(8) - 1)
+
+/* Timeout */
+#define ASPEED_PECI_IDLE_CHECK_TIMEOUT_US	(50 * USEC_PER_MSEC)
+#define ASPEED_PECI_IDLE_CHECK_INTERVAL_US	(10 * USEC_PER_MSEC)
+#define ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT	(1000)
+#define ASPEED_PECI_CMD_TIMEOUT_MS_MAX		(1000)
+
+struct aspeed_peci {
+	struct peci_controller *controller;
+	struct device *dev;
+	void __iomem *base;
+	struct clk *clk;
+	struct reset_control *rst;
+	int irq;
+	spinlock_t lock; /* to sync completion status handling */
+	struct completion xfer_complete;
+	u32 status;
+	u32 cmd_timeout_ms;
+	u32 msg_timing;
+	u32 addr_timing;
+	u32 rd_sampling_point;
+	u32 clk_div;
+};
+
+static void aspeed_peci_init_regs(struct aspeed_peci *priv)
+{
+	u32 val;
+
+	val = FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, ASPEED_PECI_CLK_DIV_DEFAULT);
+	val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
+	writel(val, priv->base + ASPEED_PECI_CTRL);
+	/*
+	 * Timing negotiation period setting.
+	 * The unit of the programmed value is 4 times of PECI clock period.
+	 */
+	val = FIELD_PREP(ASPEED_PECI_T_NEGO_MSG_MASK, priv->msg_timing);
+	val |= FIELD_PREP(ASPEED_PECI_T_NEGO_ADDR_MASK, priv->addr_timing);
+	writel(val, priv->base + ASPEED_PECI_TIMING_NEGOTIATION);
+
+	/* Clear interrupts */
+	val = readl(priv->base + ASPEED_PECI_INT_STS) | ASPEED_PECI_INT_MASK;
+	writel(val, priv->base + ASPEED_PECI_INT_STS);
+
+	/* Set timing negotiation mode and enable interrupts */
+	val = FIELD_PREP(ASPEED_PECI_TIMING_NEGO_SEL_MASK, ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO);
+	val |= ASPEED_PECI_INT_MASK;
+	writel(val, priv->base + ASPEED_PECI_INT_CTRL);
+
+	val = FIELD_PREP(ASPEED_PECI_CTRL_SAMPLING_MASK, priv->rd_sampling_point);
+	val |= FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, priv->clk_div);
+	val |= ASPEED_PECI_CTRL_PECI_EN;
+	val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
+	writel(val, priv->base + ASPEED_PECI_CTRL);
+}
+
+static inline int aspeed_peci_check_idle(struct aspeed_peci *priv)
+{
+	u32 cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
+
+	if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts) == ASPEED_PECI_CMD_STS_ADDR_T_NEGO)
+		aspeed_peci_init_regs(priv);
+
+	return readl_poll_timeout(priv->base + ASPEED_PECI_CMD,
+				  cmd_sts,
+				  !(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK),
+				  ASPEED_PECI_IDLE_CHECK_INTERVAL_US,
+				  ASPEED_PECI_IDLE_CHECK_TIMEOUT_US);
+}
+
+static int aspeed_peci_xfer(struct peci_controller *controller,
+			    u8 addr, struct peci_request *req)
+{
+	struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent);
+	unsigned long flags, timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
+	u32 peci_head;
+	int ret;
+
+	if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
+	    req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
+		return -EINVAL;
+
+	/* Check command sts and bus idle state */
+	ret = aspeed_peci_check_idle(priv);
+	if (ret)
+		return ret; /* -ETIMEDOUT */
+
+	spin_lock_irqsave(&priv->lock, flags);
+	reinit_completion(&priv->xfer_complete);
+
+	peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) |
+		    FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) |
+		    FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len);
+
+	writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
+
+	memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf, min_t(u8, req->tx.len, 16));
+	if (req->tx.len > 16)
+		memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf + 16,
+			    req->tx.len - 16);
+
+	dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
+	print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req->tx.len);
+
+	priv->status = 0;
+	writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	ret = wait_for_completion_interruptible_timeout(&priv->xfer_complete, timeout);
+	if (ret < 0)
+		return ret;
+
+	if (ret == 0) {
+		dev_dbg(priv->dev, "Timeout waiting for a response!\n");
+		return -ETIMEDOUT;
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	writel(0, priv->base + ASPEED_PECI_CMD);
+
+	if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		dev_dbg(priv->dev, "No valid response!\n");
+		return -EIO;
+	}
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0, min_t(u8, req->rx.len, 16));
+	if (req->rx.len > 16)
+		memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_RD_DATA4,
+			      req->rx.len - 16);
+
+	print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);
+
+	return 0;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+	struct aspeed_peci *priv = arg;
+	u32 status;
+
+	spin_lock(&priv->lock);
+	status = readl(priv->base + ASPEED_PECI_INT_STS);
+	writel(status, priv->base + ASPEED_PECI_INT_STS);
+	priv->status |= (status & ASPEED_PECI_INT_MASK);
+
+	/*
+	 * In most cases, interrupt bits will be set one by one but also note
+	 * that multiple interrupt bits could be set at the same time.
+	 */
+	if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_TIMEOUT\n");
+
+	if (status & ASPEED_PECI_INT_BUS_CONTENTION)
+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONTENTION\n");
+
+	if (status & ASPEED_PECI_INT_WR_FCS_BAD)
+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_WR_FCS_BAD\n");
+
+	if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_WR_FCS_ABORT\n");
+
+	/*
+	 * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE bit
+	 * set even in an error case.
+	 */
+	if (status & ASPEED_PECI_INT_CMD_DONE)
+		complete(&priv->xfer_complete);
+
+	spin_unlock(&priv->lock);
+
+	return IRQ_HANDLED;
+}
+
+static void aspeed_peci_property_sanitize(struct device *dev, const char *propname,
+					  u32 min, u32 max, u32 default_val, u32 *propval)
+{
+	u32 val;
+	int ret;
+
+	ret = device_property_read_u32(dev, propname, &val);
+	if (ret) {
+		val = default_val;
+	} else if (val > max || val < min) {
+		dev_warn(dev, "Invalid %s: %u, falling back to: %u\n",
+			 propname, val, default_val);
+
+		val = default_val;
+	}
+
+	*propval = val;
+}
+
+static void aspeed_peci_property_setup(struct aspeed_peci *priv)
+{
+	aspeed_peci_property_sanitize(priv->dev, "aspeed,clock-divider",
+				      0, ASPEED_PECI_CLK_DIV_MAX,
+				      ASPEED_PECI_CLK_DIV_DEFAULT, &priv->clk_div);
+	aspeed_peci_property_sanitize(priv->dev, "aspeed,msg-timing",
+				      0, ASPEED_PECI_MSG_TIMING_MAX,
+				      ASPEED_PECI_MSG_TIMING_DEFAULT, &priv->msg_timing);
+	aspeed_peci_property_sanitize(priv->dev, "aspeed,addr-timing",
+				      0, ASPEED_PECI_ADDR_TIMING_MAX,
+				      ASPEED_PECI_ADDR_TIMING_DEFAULT, &priv->addr_timing);
+	aspeed_peci_property_sanitize(priv->dev, "aspeed,rd-sampling-point",
+				      0, ASPEED_PECI_RD_SAMPLING_POINT_MAX,
+				      ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT,
+				      &priv->rd_sampling_point);
+	aspeed_peci_property_sanitize(priv->dev, "cmd-timeout-ms",
+				      1, ASPEED_PECI_CMD_TIMEOUT_MS_MAX,
+				      ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT, &priv->cmd_timeout_ms);
+}
+
+static struct peci_controller_ops aspeed_ops = {
+	.xfer = aspeed_peci_xfer,
+};
+
+static void aspeed_peci_reset_control_release(void *data)
+{
+	reset_control_assert(data);
+}
+
+int aspeed_peci_reset_control_deassert(struct device *dev, struct reset_control *rst)
+{
+	int ret;
+
+	ret = reset_control_deassert(rst);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, aspeed_peci_reset_control_release, rst);
+}
+
+static void aspeed_peci_clk_release(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static int aspeed_peci_clk_enable(struct device *dev, struct clk *clk)
+{
+	int ret;
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, aspeed_peci_clk_release, clk);
+}
+
+static int aspeed_peci_probe(struct platform_device *pdev)
+{
+	struct peci_controller *controller;
+	struct aspeed_peci *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+	dev_set_drvdata(priv->dev, priv);
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (!priv->irq)
+		return priv->irq;
+
+	ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler,
+			       0, "peci-aspeed", priv);
+	if (ret)
+		return ret;
+
+	init_completion(&priv->xfer_complete);
+	spin_lock_init(&priv->lock);
+
+	priv->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->rst))
+		return dev_err_probe(priv->dev, PTR_ERR(priv->rst),
+				     "failed to get reset control\n");
+
+	ret = aspeed_peci_reset_control_deassert(priv->dev, priv->rst);
+	if (ret)
+		return dev_err_probe(priv->dev, ret, "cannot deassert reset control\n");
+
+	priv->clk = devm_clk_get(priv->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(priv->dev, PTR_ERR(priv->clk), "failed to get clk\n");
+
+	ret = aspeed_peci_clk_enable(priv->dev, priv->clk);
+	if (ret)
+		return dev_err_probe(priv->dev, ret, "failed to enable clock\n");
+
+	aspeed_peci_property_setup(priv);
+
+	aspeed_peci_init_regs(priv);
+
+	controller = devm_peci_controller_add(priv->dev, &aspeed_ops);
+	if (IS_ERR(controller))
+		return dev_err_probe(priv->dev, PTR_ERR(controller),
+				     "failed to add aspeed peci controller\n");
+
+	priv->controller = controller;
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_peci_of_table[] = {
+	{ .compatible = "aspeed,ast2400-peci", },
+	{ .compatible = "aspeed,ast2500-peci", },
+	{ .compatible = "aspeed,ast2600-peci", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
+
+static struct platform_driver aspeed_peci_driver = {
+	.probe  = aspeed_peci_probe,
+	.driver = {
+		.name           = "peci-aspeed",
+		.of_match_table = aspeed_peci_of_table,
+	},
+};
+module_platform_driver(aspeed_peci_driver);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
+MODULE_DESCRIPTION("ASPEED PECI driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PECI);