[v3,06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx

Message ID 20180410183212.16787-7-jae.hyun.yoo@linux.intel.com
State New
Headers show
Series
  • PECI device driver introduction
Related show

Commit Message

Jae Hyun Yoo April 10, 2018, 6:32 p.m.
This commit adds PECI adapter driver implementation for Aspeed
AST24xx/AST25xx.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Haiyue Wang <haiyue.wang@linux.intel.com>
Reviewed-by: James Feist <james.feist@linux.intel.com>
Reviewed-by: Vernon Mauery <vernon.mauery@linux.intel.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Fengguang Wu <fengguang.wu@intel.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jason M Biils <jason.m.bills@linux.intel.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Julia Cartwright <juliac@eso.teric.us>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc: Milton Miller II <miltonm@us.ibm.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stef van Os <stef.van.os@prodrive-technologies.com>
Cc: Sumeet R Pawnikar <sumeet.r.pawnikar@intel.com>
---
 drivers/peci/Kconfig       |  28 +++
 drivers/peci/Makefile      |   3 +
 drivers/peci/peci-aspeed.c | 504 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 535 insertions(+)
 create mode 100644 drivers/peci/peci-aspeed.c

Comments

Joel Stanley April 11, 2018, 11:51 a.m. | #1
Hello Jae,

On 11 April 2018 at 04:02, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
> This commit adds PECI adapter driver implementation for Aspeed
> AST24xx/AST25xx.

The driver is looking good!

It looks like you've done some kind of review that we weren't allowed
to see, which is a double edged sword - I might be asking about things
that you've already spoken about with someone else.

I'm only just learning about PECI, but I do have some general comments below.

> ---
>  drivers/peci/Kconfig       |  28 +++
>  drivers/peci/Makefile      |   3 +
>  drivers/peci/peci-aspeed.c | 504 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 535 insertions(+)
>  create mode 100644 drivers/peci/peci-aspeed.c
>
> diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> index 1fbc13f9e6c2..0e33420365de 100644
> --- a/drivers/peci/Kconfig
> +++ b/drivers/peci/Kconfig
> @@ -14,4 +14,32 @@ config PECI
>           processors and chipset components to external monitoring or control
>           devices.
>
> +         If you want PECI support, you should say Y here and also to the
> +         specific driver for your bus adapter(s) below.
> +
> +if PECI
> +
> +#
> +# PECI hardware bus configuration
> +#
> +
> +menu "PECI Hardware Bus support"
> +
> +config PECI_ASPEED
> +       tristate "Aspeed AST24xx/AST25xx PECI support"

I think just saying ASPEED PECI support is enough. That way if the
next ASPEED SoC happens to have PECI we don't need to update all of
the help text :)

> +       select REGMAP_MMIO
> +       depends on OF
> +       depends on ARCH_ASPEED || COMPILE_TEST
> +       help
> +         Say Y here if you want support for the Platform Environment Control
> +         Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX
> +         SoCs.
> +
> +         This support is also available as a module.  If so, the module
> +         will be called peci-aspeed.
> +
> +endmenu
> +
> +endif # PECI
> +
>  endmenu
> diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
> index 9e8615e0d3ff..886285e69765 100644
> --- a/drivers/peci/Makefile
> +++ b/drivers/peci/Makefile
> @@ -4,3 +4,6 @@
>
>  # Core functionality
>  obj-$(CONFIG_PECI)             += peci-core.o
> +
> +# Hardware specific bus drivers
> +obj-$(CONFIG_PECI_ASPEED)      += peci-aspeed.o
> diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c
> new file mode 100644
> index 000000000000..be2a1f327eb1
> --- /dev/null
> +++ b/drivers/peci/peci-aspeed.c
> @@ -0,0 +1,504 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2012-2017 ASPEED Technology Inc.
> +// Copyright (c) 2018 Intel Corporation
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/peci.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define DUMP_DEBUG 0
> +
> +/* Aspeed PECI Registers */
> +#define AST_PECI_CTRL     0x00

Nit: we use ASPEED instead of AST in the upstream kernel to distingush
from the aspeed sdk drivers. If you feel strongly about this then I
won't insist you change.

> +#define AST_PECI_TIMING   0x04
> +#define AST_PECI_CMD      0x08
> +#define AST_PECI_CMD_CTRL 0x0c
> +#define AST_PECI_EXP_FCS  0x10
> +#define AST_PECI_CAP_FCS  0x14
> +#define AST_PECI_INT_CTRL 0x18
> +#define AST_PECI_INT_STS  0x1c
> +#define AST_PECI_W_DATA0  0x20
> +#define AST_PECI_W_DATA1  0x24
> +#define AST_PECI_W_DATA2  0x28
> +#define AST_PECI_W_DATA3  0x2c
> +#define AST_PECI_R_DATA0  0x30
> +#define AST_PECI_R_DATA1  0x34
> +#define AST_PECI_R_DATA2  0x38
> +#define AST_PECI_R_DATA3  0x3c
> +#define AST_PECI_W_DATA4  0x40
> +#define AST_PECI_W_DATA5  0x44
> +#define AST_PECI_W_DATA6  0x48
> +#define AST_PECI_W_DATA7  0x4c
> +#define AST_PECI_R_DATA4  0x50
> +#define AST_PECI_R_DATA5  0x54
> +#define AST_PECI_R_DATA6  0x58
> +#define AST_PECI_R_DATA7  0x5c
> +
> +/* AST_PECI_CTRL - 0x00 : Control Register */
> +#define PECI_CTRL_SAMPLING_MASK     GENMASK(19, 16)
> +#define PECI_CTRL_SAMPLING(x)       (((x) << 16) & PECI_CTRL_SAMPLING_MASK)
> +#define PECI_CTRL_SAMPLING_GET(x)   (((x) & PECI_CTRL_SAMPLING_MASK) >> 16)
> +#define PECI_CTRL_READ_MODE_MASK    GENMASK(13, 12)
> +#define PECI_CTRL_READ_MODE(x)      (((x) << 12) & PECI_CTRL_READ_MODE_MASK)
> +#define PECI_CTRL_READ_MODE_GET(x)  (((x) & PECI_CTRL_READ_MODE_MASK) >> 12)
> +#define PECI_CTRL_READ_MODE_COUNT   BIT(12)
> +#define PECI_CTRL_READ_MODE_DBG     BIT(13)
> +#define PECI_CTRL_CLK_SOURCE_MASK   BIT(11)
> +#define PECI_CTRL_CLK_SOURCE(x)     (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK)
> +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11)
> +#define PECI_CTRL_CLK_DIV_MASK      GENMASK(10, 8)
> +#define PECI_CTRL_CLK_DIV(x)        (((x) << 8) & PECI_CTRL_CLK_DIV_MASK)
> +#define PECI_CTRL_CLK_DIV_GET(x)    (((x) & PECI_CTRL_CLK_DIV_MASK) >> 8)
> +#define PECI_CTRL_INVERT_OUT        BIT(7)
> +#define PECI_CTRL_INVERT_IN         BIT(6)
> +#define PECI_CTRL_BUS_CONTENT_EN    BIT(5)
> +#define PECI_CTRL_PECI_EN           BIT(4)
> +#define PECI_CTRL_PECI_CLK_EN       BIT(0)

I know these come from the ASPEED sdk driver. Do we need them all?

> +
> +/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */
> +#define PECI_TIMING_MESSAGE_MASK   GENMASK(15, 8)
> +#define PECI_TIMING_MESSAGE(x)     (((x) << 8) & PECI_TIMING_MESSAGE_MASK)
> +#define PECI_TIMING_MESSAGE_GET(x) (((x) & PECI_TIMING_MESSAGE_MASK) >> 8)
> +#define PECI_TIMING_ADDRESS_MASK   GENMASK(7, 0)
> +#define PECI_TIMING_ADDRESS(x)     ((x) & PECI_TIMING_ADDRESS_MASK)
> +#define PECI_TIMING_ADDRESS_GET(x) ((x) & PECI_TIMING_ADDRESS_MASK)
> +
> +/* AST_PECI_CMD - 0x08 : Command Register */
> +#define PECI_CMD_PIN_MON    BIT(31)
> +#define PECI_CMD_STS_MASK   GENMASK(27, 24)
> +#define PECI_CMD_STS_GET(x) (((x) & PECI_CMD_STS_MASK) >> 24)
> +#define PECI_CMD_FIRE       BIT(0)
> +
> +/* AST_PECI_LEN - 0x0C : Read/Write Length Register */
> +#define PECI_AW_FCS_EN       BIT(31)
> +#define PECI_READ_LEN_MASK   GENMASK(23, 16)
> +#define PECI_READ_LEN(x)     (((x) << 16) & PECI_READ_LEN_MASK)
> +#define PECI_WRITE_LEN_MASK  GENMASK(15, 8)
> +#define PECI_WRITE_LEN(x)    (((x) << 8) & PECI_WRITE_LEN_MASK)
> +#define PECI_TAGET_ADDR_MASK GENMASK(7, 0)
> +#define PECI_TAGET_ADDR(x)   ((x) & PECI_TAGET_ADDR_MASK)
> +
> +/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register */
> +#define PECI_EXPECT_READ_FCS_MASK      GENMASK(23, 16)
> +#define PECI_EXPECT_READ_FCS_GET(x)    (((x) & PECI_EXPECT_READ_FCS_MASK) >> 16)
> +#define PECI_EXPECT_AW_FCS_AUTO_MASK   GENMASK(15, 8)
> +#define PECI_EXPECT_AW_FCS_AUTO_GET(x) (((x) & PECI_EXPECT_AW_FCS_AUTO_MASK) \
> +                                       >> 8)
> +#define PECI_EXPECT_WRITE_FCS_MASK     GENMASK(7, 0)
> +#define PECI_EXPECT_WRITE_FCS_GET(x)   ((x) & PECI_EXPECT_WRITE_FCS_MASK)
> +
> +/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */
> +#define PECI_CAPTURE_READ_FCS_MASK    GENMASK(23, 16)
> +#define PECI_CAPTURE_READ_FCS_GET(x)  (((x) & PECI_CAPTURE_READ_FCS_MASK) >> 16)
> +#define PECI_CAPTURE_WRITE_FCS_MASK   GENMASK(7, 0)
> +#define PECI_CAPTURE_WRITE_FCS_GET(x) ((x) & PECI_CAPTURE_WRITE_FCS_MASK)
> +
> +/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */
> +#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30)
> +#define PECI_INT_TIMEOUT            BIT(4)
> +#define PECI_INT_CONNECT            BIT(3)
> +#define PECI_INT_W_FCS_BAD          BIT(2)
> +#define PECI_INT_W_FCS_ABORT        BIT(1)
> +#define PECI_INT_CMD_DONE           BIT(0)
> +
> +struct aspeed_peci {
> +       struct peci_adapter     adaper;
> +       struct device           *dev;
> +       struct regmap           *regmap;
> +       int                     irq;
> +       struct completion       xfer_complete;
> +       u32                     status;
> +       u32                     cmd_timeout_ms;
> +};
> +
> +#define PECI_INT_MASK  (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \
> +                       PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \
> +                       PECI_INT_CMD_DONE)
> +
> +#define PECI_IDLE_CHECK_TIMEOUT_MS      50
> +#define PECI_IDLE_CHECK_INTERVAL_MS     10
> +
> +#define PECI_RD_SAMPLING_POINT_DEFAULT  8
> +#define PECI_RD_SAMPLING_POINT_MAX      15
> +#define PECI_CLK_DIV_DEFAULT            0
> +#define PECI_CLK_DIV_MAX                7
> +#define PECI_MSG_TIMING_NEGO_DEFAULT    1
> +#define PECI_MSG_TIMING_NEGO_MAX        255
> +#define PECI_ADDR_TIMING_NEGO_DEFAULT   1
> +#define PECI_ADDR_TIMING_NEGO_MAX       255
> +#define PECI_CMD_TIMEOUT_MS_DEFAULT     1000
> +#define PECI_CMD_TIMEOUT_MS_MAX         60000
> +
> +static int aspeed_peci_xfer_native(struct aspeed_peci *priv,
> +                                  struct peci_xfer_msg *msg)
> +{
> +       long err, timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
> +       u32 peci_head, peci_state, rx_data, cmd_sts;
> +       ktime_t start, end;
> +       s64 elapsed_ms;
> +       int i, rc = 0;
> +       uint reg;
> +
> +       start = ktime_get();
> +
> +       /* Check command sts and bus idle state */
> +       while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts) &&
> +              (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
> +               end = ktime_get();
> +               elapsed_ms = ktime_to_ms(ktime_sub(end, start));
> +               if (elapsed_ms >= PECI_IDLE_CHECK_TIMEOUT_MS) {
> +                       dev_dbg(priv->dev, "Timeout waiting for idle state!\n");
> +                       return -ETIMEDOUT;
> +               }
> +
> +               usleep_range(PECI_IDLE_CHECK_INTERVAL_MS * 1000,
> +                            (PECI_IDLE_CHECK_INTERVAL_MS * 1000) + 1000);
> +       };

Could the above use regmap_read_poll_timeout instead?

> +
> +       reinit_completion(&priv->xfer_complete);
> +
> +       peci_head = PECI_TAGET_ADDR(msg->addr) |
> +                                   PECI_WRITE_LEN(msg->tx_len) |
> +                                   PECI_READ_LEN(msg->rx_len);
> +
> +       rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head);
> +       if (rc)
> +               return rc;
> +
> +       for (i = 0; i < msg->tx_len; i += 4) {
> +               reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 :
> +                              AST_PECI_W_DATA4 + i % 16;
> +               rc = regmap_write(priv->regmap, reg,
> +                                 (msg->tx_buf[i + 3] << 24) |
> +                                 (msg->tx_buf[i + 2] << 16) |
> +                                 (msg->tx_buf[i + 1] << 8) |
> +                                 msg->tx_buf[i + 0]);

That looks like an endian swap. Can we do something like this?

 regmap_write(map, reg, cpu_to_be32p((void *)msg->tx_buff))

> +               if (rc)
> +                       return rc;
> +       }
> +
> +       dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
> +#if DUMP_DEBUG

Having #defines is frowned upon. I think print_hex_dump_debug will do
what you want here.

> +       print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1,
> +                      msg->tx_buf, msg->tx_len, true);
> +#endif
> +
> +       rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE);
> +       if (rc)
> +               return rc;
> +
> +       err = wait_for_completion_interruptible_timeout(&priv->xfer_complete,
> +                                                       timeout);
> +
> +       dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->status);
> +       if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
> +               dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
> +                       PECI_CMD_STS_GET(peci_state));
> +       else
> +               dev_dbg(priv->dev, "PECI_STATE : read error\n");
> +
> +       rc = regmap_write(priv->regmap, AST_PECI_CMD, 0);
> +       if (rc)
> +               return rc;
> +
> +       if (err <= 0 || !(priv->status & PECI_INT_CMD_DONE)) {
> +               if (err < 0) { /* -ERESTARTSYS */
> +                       return (int)err;
> +               } else if (err == 0) {
> +                       dev_dbg(priv->dev, "Timeout waiting for a response!\n");
> +                       return -ETIMEDOUT;
> +               }
> +
> +               dev_dbg(priv->dev, "No valid response!\n");
> +               return -EIO;
> +       }
> +
> +       for (i = 0; i < msg->rx_len; i++) {
> +               u8 byte_offset = i % 4;
> +
> +               if (byte_offset == 0) {
> +                       reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 :
> +                                      AST_PECI_R_DATA4 + i % 16;

I find this hard to read. Use a few more lines to make it clear what
your code is doing.

Actually, the entire for loop is cryptic. I understand what it's doing
now. Can you rework it to make it more readable? You follow a similar
pattern above in the write case.

> +                       rc = regmap_read(priv->regmap, reg, &rx_data);
> +                       if (rc)
> +                               return rc;
> +               }
> +
> +               msg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3))
> +       }
> +
> +#if DUMP_DEBUG
> +       print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1,
> +                      msg->rx_buf, msg->rx_len, true);
> +#endif
> +       if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
> +               dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
> +                       PECI_CMD_STS_GET(peci_state));
> +       else
> +               dev_dbg(priv->dev, "PECI_STATE : read error\n");

Given the regmap_read is always going to be a memory read on the
aspeed, I can't think of a situation where the read will fail.

On that note, is there a reason you are using regmap and not just
accessing the hardware directly? regmap imposes a number of pointer
lookups and tests each time you do a read or write.

> +       dev_dbg(priv->dev, "------------------------\n");
> +
> +       return rc;
> +}
> +
> +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
> +{
> +       struct aspeed_peci *priv = arg;
> +       u32 status_ack = 0;
> +
> +       if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->status))
> +               return IRQ_NONE;

Again, a memory mapped read won't fail. How about we check that the
regmap is working once in your _probe() function, and assume it will
continue working from there (or remove the regmap abstraction all
together).

> +
> +       /* Be noted that multiple interrupt bits can be set at the same time */
> +       if (priv->status & PECI_INT_TIMEOUT) {
> +               dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n");
> +               status_ack |= PECI_INT_TIMEOUT;
> +       }
> +
> +       if (priv->status & PECI_INT_CONNECT) {
> +               dev_dbg(priv->dev, "PECI_INT_CONNECT\n");
> +               status_ack |= PECI_INT_CONNECT;
> +       }
> +
> +       if (priv->status & PECI_INT_W_FCS_BAD) {
> +               dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n");
> +               status_ack |= PECI_INT_W_FCS_BAD;
> +       }
> +
> +       if (priv->status & PECI_INT_W_FCS_ABORT) {
> +               dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n");
> +               status_ack |= PECI_INT_W_FCS_ABORT;
> +       }

All of this code is for debugging only. Do you want to put it behind
some kind of conditional?

> +
> +       /**
> +        * All commands should be ended up with a PECI_INT_CMD_DONE bit set
> +        * even in an error case.
> +        */
> +       if (priv->status & PECI_INT_CMD_DONE) {
> +               dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n");
> +               status_ack |= PECI_INT_CMD_DONE;
> +               complete(&priv->xfer_complete);
> +       }
> +
> +       if (regmap_write(priv->regmap, AST_PECI_INT_STS, status_ack))
> +               return IRQ_NONE;
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)
> +{
> +       u32 msg_timing_nego, addr_timing_nego, rd_sampling_point;
> +       u32 clk_freq, clk_divisor, clk_div_val = 0;
> +       struct clk *clkin;
> +       int ret;
> +
> +       clkin = devm_clk_get(priv->dev, NULL);
> +       if (IS_ERR(clkin)) {
> +               dev_err(priv->dev, "Failed to get clk source.\n");
> +               return PTR_ERR(clkin);
> +       }
> +
> +       ret = of_property_read_u32(priv->dev->of_node, "clock-frequency",
> +                                  &clk_freq);
> +       if (ret < 0) {
> +               dev_err(priv->dev,
> +                       "Could not read clock-frequency property.\n");
> +               return ret;
> +       }
> +
> +       clk_divisor = clk_get_rate(clkin) / clk_freq;
> +       devm_clk_put(priv->dev, clkin);
> +
> +       while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX))
> +               clk_div_val++;

We have a framework for doing clocks in the kernel. Would it make
sense to write a driver for this clock and add it to
drivers/clk/clk-aspeed.c?

> +
> +       ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego",
> +                                  &msg_timing_nego);
> +       if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) {
> +               dev_warn(priv->dev,
> +                        "Invalid msg-timing-nego : %u, Use default : %u\n",
> +                        msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT);

The property is optional so I suggest we don't print a message if it's
not present. We certainly don't want to print a message saying
"invalid".

The same comment applies to the other optional properties below.

> +               msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT;
> +       }
> +
> +       ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego",
> +                                  &addr_timing_nego);
> +       if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) {
> +               dev_warn(priv->dev,
> +                        "Invalid addr-timing-nego : %u, Use default : %u\n",
> +                        addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT);
> +               addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT;
> +       }
> +
> +       ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point",
> +                                  &rd_sampling_point);
> +       if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) {
> +               dev_warn(priv->dev,
> +                        "Invalid rd-sampling-point : %u. Use default : %u\n",
> +                        rd_sampling_point,
> +                        PECI_RD_SAMPLING_POINT_DEFAULT);
> +               rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT;
> +       }
> +
> +       ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms",
> +                                  &priv->cmd_timeout_ms);
> +       if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX ||
> +           priv->cmd_timeout_ms == 0) {
> +               dev_warn(priv->dev,
> +                        "Invalid cmd-timeout-ms : %u. Use default : %u\n",
> +                        priv->cmd_timeout_ms,
> +                        PECI_CMD_TIMEOUT_MS_DEFAULT);
> +               priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT;
> +       }
> +
> +       ret = regmap_write(priv->regmap, AST_PECI_CTRL,
> +                          PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) |
> +                          PECI_CTRL_PECI_CLK_EN);
> +       if (ret)
> +               return ret;
> +
> +       usleep_range(1000, 5000);

Can we probe in parallel? If not, putting a sleep in the _probe will
hold up the rest of drivers from being able to do anything, and hold
up boot.

If you decide that you do need to probe here, please add a comment.
(This is the wait for the clock to be stable?)

> +
> +       /**
> +        * Timing negotiation period setting.
> +        * The unit of the programmed value is 4 times of PECI clock period.
> +        */
> +       ret = regmap_write(priv->regmap, AST_PECI_TIMING,
> +                          PECI_TIMING_MESSAGE(msg_timing_nego) |
> +                          PECI_TIMING_ADDRESS(addr_timing_nego));
> +       if (ret)
> +               return ret;
> +
> +       /* Clear interrupts */
> +       ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK);
> +       if (ret)
> +               return ret;
> +
> +       /* Enable interrupts */
> +       ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK);
> +       if (ret)
> +               return ret;
> +
> +       /* Read sampling point and clock speed setting */
> +       ret = regmap_write(priv->regmap, AST_PECI_CTRL,
> +                          PECI_CTRL_SAMPLING(rd_sampling_point) |
> +                          PECI_CTRL_CLK_DIV(clk_div_val) |
> +                          PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static const struct regmap_config aspeed_peci_regmap_config = {
> +       .reg_bits = 32,
> +       .val_bits = 32,
> +       .reg_stride = 4,
> +       .max_register = AST_PECI_R_DATA7,
> +       .val_format_endian = REGMAP_ENDIAN_LITTLE,
> +       .fast_io = true,
> +};
> +
> +static int aspeed_peci_xfer(struct peci_adapter *adaper,
> +                           struct peci_xfer_msg *msg)
> +{
> +       struct aspeed_peci *priv = peci_get_adapdata(adaper);
> +
> +       return aspeed_peci_xfer_native(priv, msg);
> +}
> +
> +static int aspeed_peci_probe(struct platform_device *pdev)
> +{
> +       struct aspeed_peci *priv;
> +       struct resource *res;
> +       void __iomem *base;
> +       int ret = 0;
> +
> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       dev_set_drvdata(&pdev->dev, priv);
> +       priv->dev = &pdev->dev;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +                                            &aspeed_peci_regmap_config);
> +       if (IS_ERR(priv->regmap))
> +               return PTR_ERR(priv->regmap);
> +
> +       priv->irq = platform_get_irq(pdev, 0);
> +       if (!priv->irq)
> +               return -ENODEV;
> +
> +       ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler,
> +                              IRQF_SHARED,

This interrupt is only for the peci device. Why is it marked as shared?

> +                              "peci-aspeed-irq",
> +                              priv);
> +       if (ret < 0)
> +               return ret;
> +
> +       init_completion(&priv->xfer_complete);
> +
> +       priv->adaper.dev.parent = priv->dev;
> +       priv->adaper.dev.of_node = of_node_get(dev_of_node(priv->dev));
> +       strlcpy(priv->adaper.name, pdev->name, sizeof(priv->adaper.name));
> +       priv->adaper.xfer = aspeed_peci_xfer;
> +       peci_set_adapdata(&priv->adaper, priv);
> +
> +       ret = aspeed_peci_init_ctrl(priv);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = peci_add_adapter(&priv->adaper);
> +       if (ret < 0)
> +               return ret;
> +
> +       dev_info(&pdev->dev, "peci bus %d registered, irq %d\n",
> +                priv->adaper.nr, priv->irq);
> +
> +       return 0;
> +}
> +
> +static int aspeed_peci_remove(struct platform_device *pdev)
> +{
> +       struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);
> +
> +       peci_del_adapter(&priv->adaper);
> +       of_node_put(priv->adaper.dev.of_node);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id aspeed_peci_of_table[] = {
> +       { .compatible = "aspeed,ast2400-peci", },
> +       { .compatible = "aspeed,ast2500-peci", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
> +
> +static struct platform_driver aspeed_peci_driver = {
> +       .probe  = aspeed_peci_probe,
> +       .remove = aspeed_peci_remove,
> +       .driver = {
> +               .name           = "peci-aspeed",
> +               .of_match_table = of_match_ptr(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 v2");
> --
> 2.16.2
>
Jae Hyun Yoo April 12, 2018, 2:03 a.m. | #2
Hello Joel,

Thanks for sharing your time. Please see my answers inline.

On 4/11/2018 4:51 AM, Joel Stanley wrote:
> Hello Jae,
> 
> On 11 April 2018 at 04:02, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>> This commit adds PECI adapter driver implementation for Aspeed
>> AST24xx/AST25xx.
> 
> The driver is looking good!
> 
> It looks like you've done some kind of review that we weren't allowed
> to see, which is a double edged sword - I might be asking about things
> that you've already spoken about with someone else.
> 
> I'm only just learning about PECI, but I do have some general comments below.
> 

Yes, it took a hidden review process between v2 and v3. I know it's an 
unusual process but it was requested. Hopefully, change logs in cover 
letter could roughly provide the details. Thanks for your comments.

>> ---
>>   drivers/peci/Kconfig       |  28 +++
>>   drivers/peci/Makefile      |   3 +
>>   drivers/peci/peci-aspeed.c | 504 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 535 insertions(+)
>>   create mode 100644 drivers/peci/peci-aspeed.c
>>
>> diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
>> index 1fbc13f9e6c2..0e33420365de 100644
>> --- a/drivers/peci/Kconfig
>> +++ b/drivers/peci/Kconfig
>> @@ -14,4 +14,32 @@ config PECI
>>            processors and chipset components to external monitoring or control
>>            devices.
>>
>> +         If you want PECI support, you should say Y here and also to the
>> +         specific driver for your bus adapter(s) below.
>> +
>> +if PECI
>> +
>> +#
>> +# PECI hardware bus configuration
>> +#
>> +
>> +menu "PECI Hardware Bus support"
>> +
>> +config PECI_ASPEED
>> +       tristate "Aspeed AST24xx/AST25xx PECI support"
> 
> I think just saying ASPEED PECI support is enough. That way if the
> next ASPEED SoC happens to have PECI we don't need to update all of
> the help text :)
> 

Agreed. I'll change the description.

>> +       select REGMAP_MMIO
>> +       depends on OF
>> +       depends on ARCH_ASPEED || COMPILE_TEST
>> +       help
>> +         Say Y here if you want support for the Platform Environment Control
>> +         Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX
>> +         SoCs.
>> +
>> +         This support is also available as a module.  If so, the module
>> +         will be called peci-aspeed.
>> +
>> +endmenu
>> +
>> +endif # PECI
>> +
>>   endmenu
>> diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
>> index 9e8615e0d3ff..886285e69765 100644
>> --- a/drivers/peci/Makefile
>> +++ b/drivers/peci/Makefile
>> @@ -4,3 +4,6 @@
>>
>>   # Core functionality
>>   obj-$(CONFIG_PECI)             += peci-core.o
>> +
>> +# Hardware specific bus drivers
>> +obj-$(CONFIG_PECI_ASPEED)      += peci-aspeed.o
>> diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c
>> new file mode 100644
>> index 000000000000..be2a1f327eb1
>> --- /dev/null
>> +++ b/drivers/peci/peci-aspeed.c
>> @@ -0,0 +1,504 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2012-2017 ASPEED Technology Inc.
>> +// Copyright (c) 2018 Intel Corporation
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/peci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#define DUMP_DEBUG 0
>> +
>> +/* Aspeed PECI Registers */
>> +#define AST_PECI_CTRL     0x00
> 
> Nit: we use ASPEED instead of AST in the upstream kernel to distingush
> from the aspeed sdk drivers. If you feel strongly about this then I
> won't insist you change.
> 

Okay then, better change it now than later. Will change all defines.

>> +#define AST_PECI_TIMING   0x04
>> +#define AST_PECI_CMD      0x08
>> +#define AST_PECI_CMD_CTRL 0x0c
>> +#define AST_PECI_EXP_FCS  0x10
>> +#define AST_PECI_CAP_FCS  0x14
>> +#define AST_PECI_INT_CTRL 0x18
>> +#define AST_PECI_INT_STS  0x1c
>> +#define AST_PECI_W_DATA0  0x20
>> +#define AST_PECI_W_DATA1  0x24
>> +#define AST_PECI_W_DATA2  0x28
>> +#define AST_PECI_W_DATA3  0x2c
>> +#define AST_PECI_R_DATA0  0x30
>> +#define AST_PECI_R_DATA1  0x34
>> +#define AST_PECI_R_DATA2  0x38
>> +#define AST_PECI_R_DATA3  0x3c
>> +#define AST_PECI_W_DATA4  0x40
>> +#define AST_PECI_W_DATA5  0x44
>> +#define AST_PECI_W_DATA6  0x48
>> +#define AST_PECI_W_DATA7  0x4c
>> +#define AST_PECI_R_DATA4  0x50
>> +#define AST_PECI_R_DATA5  0x54
>> +#define AST_PECI_R_DATA6  0x58
>> +#define AST_PECI_R_DATA7  0x5c
>> +
>> +/* AST_PECI_CTRL - 0x00 : Control Register */
>> +#define PECI_CTRL_SAMPLING_MASK     GENMASK(19, 16)
>> +#define PECI_CTRL_SAMPLING(x)       (((x) << 16) & PECI_CTRL_SAMPLING_MASK)
>> +#define PECI_CTRL_SAMPLING_GET(x)   (((x) & PECI_CTRL_SAMPLING_MASK) >> 16)
>> +#define PECI_CTRL_READ_MODE_MASK    GENMASK(13, 12)
>> +#define PECI_CTRL_READ_MODE(x)      (((x) << 12) & PECI_CTRL_READ_MODE_MASK)
>> +#define PECI_CTRL_READ_MODE_GET(x)  (((x) & PECI_CTRL_READ_MODE_MASK) >> 12)
>> +#define PECI_CTRL_READ_MODE_COUNT   BIT(12)
>> +#define PECI_CTRL_READ_MODE_DBG     BIT(13)
>> +#define PECI_CTRL_CLK_SOURCE_MASK   BIT(11)
>> +#define PECI_CTRL_CLK_SOURCE(x)     (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK)
>> +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11)
>> +#define PECI_CTRL_CLK_DIV_MASK      GENMASK(10, 8)
>> +#define PECI_CTRL_CLK_DIV(x)        (((x) << 8) & PECI_CTRL_CLK_DIV_MASK)
>> +#define PECI_CTRL_CLK_DIV_GET(x)    (((x) & PECI_CTRL_CLK_DIV_MASK) >> 8)
>> +#define PECI_CTRL_INVERT_OUT        BIT(7)
>> +#define PECI_CTRL_INVERT_IN         BIT(6)
>> +#define PECI_CTRL_BUS_CONTENT_EN    BIT(5)
>> +#define PECI_CTRL_PECI_EN           BIT(4)
>> +#define PECI_CTRL_PECI_CLK_EN       BIT(0)
> 
> I know these come from the ASPEED sdk driver. Do we need them all?
> 

It doesn't use all but better keep for bug fix or improvement use, I think.

>> +
>> +/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */
>> +#define PECI_TIMING_MESSAGE_MASK   GENMASK(15, 8)
>> +#define PECI_TIMING_MESSAGE(x)     (((x) << 8) & PECI_TIMING_MESSAGE_MASK)
>> +#define PECI_TIMING_MESSAGE_GET(x) (((x) & PECI_TIMING_MESSAGE_MASK) >> 8)
>> +#define PECI_TIMING_ADDRESS_MASK   GENMASK(7, 0)
>> +#define PECI_TIMING_ADDRESS(x)     ((x) & PECI_TIMING_ADDRESS_MASK)
>> +#define PECI_TIMING_ADDRESS_GET(x) ((x) & PECI_TIMING_ADDRESS_MASK)
>> +
>> +/* AST_PECI_CMD - 0x08 : Command Register */
>> +#define PECI_CMD_PIN_MON    BIT(31)
>> +#define PECI_CMD_STS_MASK   GENMASK(27, 24)
>> +#define PECI_CMD_STS_GET(x) (((x) & PECI_CMD_STS_MASK) >> 24)
>> +#define PECI_CMD_FIRE       BIT(0)
>> +
>> +/* AST_PECI_LEN - 0x0C : Read/Write Length Register */
>> +#define PECI_AW_FCS_EN       BIT(31)
>> +#define PECI_READ_LEN_MASK   GENMASK(23, 16)
>> +#define PECI_READ_LEN(x)     (((x) << 16) & PECI_READ_LEN_MASK)
>> +#define PECI_WRITE_LEN_MASK  GENMASK(15, 8)
>> +#define PECI_WRITE_LEN(x)    (((x) << 8) & PECI_WRITE_LEN_MASK)
>> +#define PECI_TAGET_ADDR_MASK GENMASK(7, 0)
>> +#define PECI_TAGET_ADDR(x)   ((x) & PECI_TAGET_ADDR_MASK)
>> +
>> +/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register */
>> +#define PECI_EXPECT_READ_FCS_MASK      GENMASK(23, 16)
>> +#define PECI_EXPECT_READ_FCS_GET(x)    (((x) & PECI_EXPECT_READ_FCS_MASK) >> 16)
>> +#define PECI_EXPECT_AW_FCS_AUTO_MASK   GENMASK(15, 8)
>> +#define PECI_EXPECT_AW_FCS_AUTO_GET(x) (((x) & PECI_EXPECT_AW_FCS_AUTO_MASK) \
>> +                                       >> 8)
>> +#define PECI_EXPECT_WRITE_FCS_MASK     GENMASK(7, 0)
>> +#define PECI_EXPECT_WRITE_FCS_GET(x)   ((x) & PECI_EXPECT_WRITE_FCS_MASK)
>> +
>> +/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */
>> +#define PECI_CAPTURE_READ_FCS_MASK    GENMASK(23, 16)
>> +#define PECI_CAPTURE_READ_FCS_GET(x)  (((x) & PECI_CAPTURE_READ_FCS_MASK) >> 16)
>> +#define PECI_CAPTURE_WRITE_FCS_MASK   GENMASK(7, 0)
>> +#define PECI_CAPTURE_WRITE_FCS_GET(x) ((x) & PECI_CAPTURE_WRITE_FCS_MASK)
>> +
>> +/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */
>> +#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30)
>> +#define PECI_INT_TIMEOUT            BIT(4)
>> +#define PECI_INT_CONNECT            BIT(3)
>> +#define PECI_INT_W_FCS_BAD          BIT(2)
>> +#define PECI_INT_W_FCS_ABORT        BIT(1)
>> +#define PECI_INT_CMD_DONE           BIT(0)
>> +
>> +struct aspeed_peci {
>> +       struct peci_adapter     adaper;
>> +       struct device           *dev;
>> +       struct regmap           *regmap;
>> +       int                     irq;
>> +       struct completion       xfer_complete;
>> +       u32                     status;
>> +       u32                     cmd_timeout_ms;
>> +};
>> +
>> +#define PECI_INT_MASK  (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \
>> +                       PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \
>> +                       PECI_INT_CMD_DONE)
>> +
>> +#define PECI_IDLE_CHECK_TIMEOUT_MS      50
>> +#define PECI_IDLE_CHECK_INTERVAL_MS     10
>> +
>> +#define PECI_RD_SAMPLING_POINT_DEFAULT  8
>> +#define PECI_RD_SAMPLING_POINT_MAX      15
>> +#define PECI_CLK_DIV_DEFAULT            0
>> +#define PECI_CLK_DIV_MAX                7
>> +#define PECI_MSG_TIMING_NEGO_DEFAULT    1
>> +#define PECI_MSG_TIMING_NEGO_MAX        255
>> +#define PECI_ADDR_TIMING_NEGO_DEFAULT   1
>> +#define PECI_ADDR_TIMING_NEGO_MAX       255
>> +#define PECI_CMD_TIMEOUT_MS_DEFAULT     1000
>> +#define PECI_CMD_TIMEOUT_MS_MAX         60000
>> +
>> +static int aspeed_peci_xfer_native(struct aspeed_peci *priv,
>> +                                  struct peci_xfer_msg *msg)
>> +{
>> +       long err, timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
>> +       u32 peci_head, peci_state, rx_data, cmd_sts;
>> +       ktime_t start, end;
>> +       s64 elapsed_ms;
>> +       int i, rc = 0;
>> +       uint reg;
>> +
>> +       start = ktime_get();
>> +
>> +       /* Check command sts and bus idle state */
>> +       while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts) &&
>> +              (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
>> +               end = ktime_get();
>> +               elapsed_ms = ktime_to_ms(ktime_sub(end, start));
>> +               if (elapsed_ms >= PECI_IDLE_CHECK_TIMEOUT_MS) {
>> +                       dev_dbg(priv->dev, "Timeout waiting for idle state!\n");
>> +                       return -ETIMEDOUT;
>> +               }
>> +
>> +               usleep_range(PECI_IDLE_CHECK_INTERVAL_MS * 1000,
>> +                            (PECI_IDLE_CHECK_INTERVAL_MS * 1000) + 1000);
>> +       };
> 
> Could the above use regmap_read_poll_timeout instead?
> 

Yes, that would be better. I'll rewrite it.

>> +
>> +       reinit_completion(&priv->xfer_complete);
>> +
>> +       peci_head = PECI_TAGET_ADDR(msg->addr) |
>> +                                   PECI_WRITE_LEN(msg->tx_len) |
>> +                                   PECI_READ_LEN(msg->rx_len);
>> +
>> +       rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head);
>> +       if (rc)
>> +               return rc;
>> +
>> +       for (i = 0; i < msg->tx_len; i += 4) {
>> +               reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 :
>> +                              AST_PECI_W_DATA4 + i % 16;
>> +               rc = regmap_write(priv->regmap, reg,
>> +                                 (msg->tx_buf[i + 3] << 24) |
>> +                                 (msg->tx_buf[i + 2] << 16) |
>> +                                 (msg->tx_buf[i + 1] << 8) |
>> +                                 msg->tx_buf[i + 0]);
> 
> That looks like an endian swap. Can we do something like this?
> 
>   regmap_write(map, reg, cpu_to_be32p((void *)msg->tx_buff))
> 

Yes, it could be simplified like you pointed out. Will change it.

>> +               if (rc)
>> +                       return rc;
>> +       }
>> +
>> +       dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
>> +#if DUMP_DEBUG
> 
> Having #defines is frowned upon. I think print_hex_dump_debug will do
> what you want here.
> 

Got it. I'll replace it with print_hex_dump_debug() after removing the 
define.

>> +       print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1,
>> +                      msg->tx_buf, msg->tx_len, true);
>> +#endif
>> +
>> +       rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE);
>> +       if (rc)
>> +               return rc;
>> +
>> +       err = wait_for_completion_interruptible_timeout(&priv->xfer_complete,
>> +                                                       timeout);
>> +
>> +       dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->status);
>> +       if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
>> +               dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
>> +                       PECI_CMD_STS_GET(peci_state));
>> +       else
>> +               dev_dbg(priv->dev, "PECI_STATE : read error\n");
>> +
>> +       rc = regmap_write(priv->regmap, AST_PECI_CMD, 0);
>> +       if (rc)
>> +               return rc;
>> +
>> +       if (err <= 0 || !(priv->status & PECI_INT_CMD_DONE)) {
>> +               if (err < 0) { /* -ERESTARTSYS */
>> +                       return (int)err;
>> +               } else if (err == 0) {
>> +                       dev_dbg(priv->dev, "Timeout waiting for a response!\n");
>> +                       return -ETIMEDOUT;
>> +               }
>> +
>> +               dev_dbg(priv->dev, "No valid response!\n");
>> +               return -EIO;
>> +       }
>> +
>> +       for (i = 0; i < msg->rx_len; i++) {
>> +               u8 byte_offset = i % 4;
>> +
>> +               if (byte_offset == 0) {
>> +                       reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 :
>> +                                      AST_PECI_R_DATA4 + i % 16;
> 
> I find this hard to read. Use a few more lines to make it clear what
> your code is doing.
> 
> Actually, the entire for loop is cryptic. I understand what it's doing
> now. Can you rework it to make it more readable? You follow a similar
> pattern above in the write case.
> 

Intention was that make it run just amount up to the rx_len but it's not 
efficient. I'll rewrite it like you suggested.

>> +                       rc = regmap_read(priv->regmap, reg, &rx_data);
>> +                       if (rc)
>> +                               return rc;
>> +               }
>> +
>> +               msg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3))
>> +       }
>> +
>> +#if DUMP_DEBUG
>> +       print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1,
>> +                      msg->rx_buf, msg->rx_len, true);
>> +#endif
>> +       if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
>> +               dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
>> +                       PECI_CMD_STS_GET(peci_state));
>> +       else
>> +               dev_dbg(priv->dev, "PECI_STATE : read error\n");
> 
> Given the regmap_read is always going to be a memory read on the
> aspeed, I can't think of a situation where the read will fail.
> 
> On that note, is there a reason you are using regmap and not just
> accessing the hardware directly? regmap imposes a number of pointer
> lookups and tests each time you do a read or write.
> 

No specific reason. regmap makes some overhead as you mentioned but it 
also provides some advantages on access simplification, endianness 
handling and register dump at run time. I'd not insist using of regmap 
if you prefer using of raw readl and writel. Do you?

>> +       dev_dbg(priv->dev, "------------------------\n");
>> +
>> +       return rc;
>> +}
>> +
>> +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
>> +{
>> +       struct aspeed_peci *priv = arg;
>> +       u32 status_ack = 0;
>> +
>> +       if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->status))
>> +               return IRQ_NONE;
> 
> Again, a memory mapped read won't fail. How about we check that the
> regmap is working once in your _probe() function, and assume it will
> continue working from there (or remove the regmap abstraction all
> together).
> 

You are right. I'll keep this checking only in _probe() function and 
remove all redundant error checking codes on memory mapped IO.

>> +
>> +       /* Be noted that multiple interrupt bits can be set at the same time */
>> +       if (priv->status & PECI_INT_TIMEOUT) {
>> +               dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n");
>> +               status_ack |= PECI_INT_TIMEOUT;
>> +       }
>> +
>> +       if (priv->status & PECI_INT_CONNECT) {
>> +               dev_dbg(priv->dev, "PECI_INT_CONNECT\n");
>> +               status_ack |= PECI_INT_CONNECT;
>> +       }
>> +
>> +       if (priv->status & PECI_INT_W_FCS_BAD) {
>> +               dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n");
>> +               status_ack |= PECI_INT_W_FCS_BAD;
>> +       }
>> +
>> +       if (priv->status & PECI_INT_W_FCS_ABORT) {
>> +               dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n");
>> +               status_ack |= PECI_INT_W_FCS_ABORT;
>> +       }
> 
> All of this code is for debugging only. Do you want to put it behind
> some kind of conditional?
> 

This code makes changes on the status_ack variable to write back ack bit 
on each interrupt.

>> +
>> +       /**
>> +        * All commands should be ended up with a PECI_INT_CMD_DONE bit set
>> +        * even in an error case.
>> +        */
>> +       if (priv->status & PECI_INT_CMD_DONE) {
>> +               dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n");
>> +               status_ack |= PECI_INT_CMD_DONE;
>> +               complete(&priv->xfer_complete);
>> +       }
>> +
>> +       if (regmap_write(priv->regmap, AST_PECI_INT_STS, status_ack))
>> +               return IRQ_NONE;
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)
>> +{
>> +       u32 msg_timing_nego, addr_timing_nego, rd_sampling_point;
>> +       u32 clk_freq, clk_divisor, clk_div_val = 0;
>> +       struct clk *clkin;
>> +       int ret;
>> +
>> +       clkin = devm_clk_get(priv->dev, NULL);
>> +       if (IS_ERR(clkin)) {
>> +               dev_err(priv->dev, "Failed to get clk source.\n");
>> +               return PTR_ERR(clkin);
>> +       }
>> +
>> +       ret = of_property_read_u32(priv->dev->of_node, "clock-frequency",
>> +                                  &clk_freq);
>> +       if (ret < 0) {
>> +               dev_err(priv->dev,
>> +                       "Could not read clock-frequency property.\n");
>> +               return ret;
>> +       }
>> +
>> +       clk_divisor = clk_get_rate(clkin) / clk_freq;
>> +       devm_clk_put(priv->dev, clkin);
>> +
>> +       while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX))
>> +               clk_div_val++;
> 
> We have a framework for doing clocks in the kernel. Would it make
> sense to write a driver for this clock and add it to
> drivers/clk/clk-aspeed.c?
> 

Unlike other HW module, PECI uses the 24MHz external clock as its clock 
source. Should it use clk-aspeed.c in this case?

>> +
>> +       ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego",
>> +                                  &msg_timing_nego);
>> +       if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) {
>> +               dev_warn(priv->dev,
>> +                        "Invalid msg-timing-nego : %u, Use default : %u\n",
>> +                        msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT);
> 
> The property is optional so I suggest we don't print a message if it's
> not present. We certainly don't want to print a message saying
> "invalid".
> 
> The same comment applies to the other optional properties below.
> 

Agreed. I'll make it print out the message only when ret == 0 and 
msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX.

>> +               msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT;
>> +       }
>> +
>> +       ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego",
>> +                                  &addr_timing_nego);
>> +       if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) {
>> +               dev_warn(priv->dev,
>> +                        "Invalid addr-timing-nego : %u, Use default : %u\n",
>> +                        addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT);
>> +               addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT;
>> +       }
>> +
>> +       ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point",
>> +                                  &rd_sampling_point);
>> +       if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) {
>> +               dev_warn(priv->dev,
>> +                        "Invalid rd-sampling-point : %u. Use default : %u\n",
>> +                        rd_sampling_point,
>> +                        PECI_RD_SAMPLING_POINT_DEFAULT);
>> +               rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT;
>> +       }
>> +
>> +       ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms",
>> +                                  &priv->cmd_timeout_ms);
>> +       if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX ||
>> +           priv->cmd_timeout_ms == 0) {
>> +               dev_warn(priv->dev,
>> +                        "Invalid cmd-timeout-ms : %u. Use default : %u\n",
>> +                        priv->cmd_timeout_ms,
>> +                        PECI_CMD_TIMEOUT_MS_DEFAULT);
>> +               priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT;
>> +       }
>> +
>> +       ret = regmap_write(priv->regmap, AST_PECI_CTRL,
>> +                          PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) |
>> +                          PECI_CTRL_PECI_CLK_EN);
>> +       if (ret)
>> +               return ret;
>> +
>> +       usleep_range(1000, 5000);
> 
> Can we probe in parallel? If not, putting a sleep in the _probe will
> hold up the rest of drivers from being able to do anything, and hold
> up boot.
> 
> If you decide that you do need to probe here, please add a comment.
> (This is the wait for the clock to be stable?)
> 

I'll test it again and will remove it if it is not necessary.

>> +
>> +       /**
>> +        * Timing negotiation period setting.
>> +        * The unit of the programmed value is 4 times of PECI clock period.
>> +        */
>> +       ret = regmap_write(priv->regmap, AST_PECI_TIMING,
>> +                          PECI_TIMING_MESSAGE(msg_timing_nego) |
>> +                          PECI_TIMING_ADDRESS(addr_timing_nego));
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Clear interrupts */
>> +       ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Enable interrupts */
>> +       ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Read sampling point and clock speed setting */
>> +       ret = regmap_write(priv->regmap, AST_PECI_CTRL,
>> +                          PECI_CTRL_SAMPLING(rd_sampling_point) |
>> +                          PECI_CTRL_CLK_DIV(clk_div_val) |
>> +                          PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct regmap_config aspeed_peci_regmap_config = {
>> +       .reg_bits = 32,
>> +       .val_bits = 32,
>> +       .reg_stride = 4,
>> +       .max_register = AST_PECI_R_DATA7,
>> +       .val_format_endian = REGMAP_ENDIAN_LITTLE,
>> +       .fast_io = true,
>> +};
>> +
>> +static int aspeed_peci_xfer(struct peci_adapter *adaper,
>> +                           struct peci_xfer_msg *msg)
>> +{
>> +       struct aspeed_peci *priv = peci_get_adapdata(adaper);
>> +
>> +       return aspeed_peci_xfer_native(priv, msg);
>> +}
>> +
>> +static int aspeed_peci_probe(struct platform_device *pdev)
>> +{
>> +       struct aspeed_peci *priv;
>> +       struct resource *res;
>> +       void __iomem *base;
>> +       int ret = 0;
>> +
>> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +       if (!priv)
>> +               return -ENOMEM;
>> +
>> +       dev_set_drvdata(&pdev->dev, priv);
>> +       priv->dev = &pdev->dev;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(base))
>> +               return PTR_ERR(base);
>> +
>> +       priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>> +                                            &aspeed_peci_regmap_config);
>> +       if (IS_ERR(priv->regmap))
>> +               return PTR_ERR(priv->regmap);
>> +
>> +       priv->irq = platform_get_irq(pdev, 0);
>> +       if (!priv->irq)
>> +               return -ENODEV;
>> +
>> +       ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler,
>> +                              IRQF_SHARED,
> 
> This interrupt is only for the peci device. Why is it marked as shared?
> 

You are right. I'll remove the flag.

>> +                              "peci-aspeed-irq",
>> +                              priv);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       init_completion(&priv->xfer_complete);
>> +
>> +       priv->adaper.dev.parent = priv->dev;
>> +       priv->adaper.dev.of_node = of_node_get(dev_of_node(priv->dev));
>> +       strlcpy(priv->adaper.name, pdev->name, sizeof(priv->adaper.name));
>> +       priv->adaper.xfer = aspeed_peci_xfer;
>> +       peci_set_adapdata(&priv->adaper, priv);
>> +
>> +       ret = aspeed_peci_init_ctrl(priv);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = peci_add_adapter(&priv->adaper);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       dev_info(&pdev->dev, "peci bus %d registered, irq %d\n",
>> +                priv->adaper.nr, priv->irq);
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_peci_remove(struct platform_device *pdev)
>> +{
>> +       struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);
>> +
>> +       peci_del_adapter(&priv->adaper);
>> +       of_node_put(priv->adaper.dev.of_node);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id aspeed_peci_of_table[] = {
>> +       { .compatible = "aspeed,ast2400-peci", },
>> +       { .compatible = "aspeed,ast2500-peci", },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
>> +
>> +static struct platform_driver aspeed_peci_driver = {
>> +       .probe  = aspeed_peci_probe,
>> +       .remove = aspeed_peci_remove,
>> +       .driver = {
>> +               .name           = "peci-aspeed",
>> +               .of_match_table = of_match_ptr(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 v2");
>> --
>> 2.16.2
>>
Robin Murphy April 17, 2018, 1:37 p.m. | #3
Just a drive-by nit:

On 10/04/18 19:32, Jae Hyun Yoo wrote:
[...]
> +#define PECI_CTRL_SAMPLING_MASK     GENMASK(19, 16)
> +#define PECI_CTRL_SAMPLING(x)       (((x) << 16) & PECI_CTRL_SAMPLING_MASK)
> +#define PECI_CTRL_SAMPLING_GET(x)   (((x) & PECI_CTRL_SAMPLING_MASK) >> 16)

FWIW, <linux/bitfield.h> already provides functionality like this, so it 
might be worth taking a look at FIELD_{GET,PREP}() to save all these 
local definitions.

Robin.
Jae Hyun Yoo April 17, 2018, 6:21 p.m. | #4
Hi Robin,

On 4/17/2018 6:37 AM, Robin Murphy wrote:
> Just a drive-by nit:
> 
> On 10/04/18 19:32, Jae Hyun Yoo wrote:
> [...]
>> +#define PECI_CTRL_SAMPLING_MASK     GENMASK(19, 16)
>> +#define PECI_CTRL_SAMPLING(x)       (((x) << 16) & 
>> PECI_CTRL_SAMPLING_MASK)
>> +#define PECI_CTRL_SAMPLING_GET(x)   (((x) & PECI_CTRL_SAMPLING_MASK) 
>> >> 16)
> 
> FWIW, <linux/bitfield.h> already provides functionality like this, so it 
> might be worth taking a look at FIELD_{GET,PREP}() to save all these 
> local definitions.
> 
> Robin.

Yes, that looks better. Thanks a lot for your pointing it out.

Jae

Patch

diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
index 1fbc13f9e6c2..0e33420365de 100644
--- a/drivers/peci/Kconfig
+++ b/drivers/peci/Kconfig
@@ -14,4 +14,32 @@  config PECI
 	  processors and chipset components to external monitoring or control
 	  devices.
 
+	  If you want PECI support, you should say Y here and also to the
+	  specific driver for your bus adapter(s) below.
+
+if PECI
+
+#
+# PECI hardware bus configuration
+#
+
+menu "PECI Hardware Bus support"
+
+config PECI_ASPEED
+	tristate "Aspeed AST24xx/AST25xx PECI support"
+	select REGMAP_MMIO
+	depends on OF
+	depends on ARCH_ASPEED || COMPILE_TEST
+	help
+	  Say Y here if you want support for the Platform Environment Control
+	  Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX
+	  SoCs.
+
+	  This support is also available as a module.  If so, the module
+	  will be called peci-aspeed.
+
+endmenu
+
+endif # PECI
+
 endmenu
diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
index 9e8615e0d3ff..886285e69765 100644
--- a/drivers/peci/Makefile
+++ b/drivers/peci/Makefile
@@ -4,3 +4,6 @@ 
 
 # Core functionality
 obj-$(CONFIG_PECI)		+= peci-core.o
+
+# Hardware specific bus drivers
+obj-$(CONFIG_PECI_ASPEED)	+= peci-aspeed.o
diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c
new file mode 100644
index 000000000000..be2a1f327eb1
--- /dev/null
+++ b/drivers/peci/peci-aspeed.c
@@ -0,0 +1,504 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2012-2017 ASPEED Technology Inc.
+// Copyright (c) 2018 Intel Corporation
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/peci.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define DUMP_DEBUG 0
+
+/* Aspeed PECI Registers */
+#define AST_PECI_CTRL     0x00
+#define AST_PECI_TIMING   0x04
+#define AST_PECI_CMD      0x08
+#define AST_PECI_CMD_CTRL 0x0c
+#define AST_PECI_EXP_FCS  0x10
+#define AST_PECI_CAP_FCS  0x14
+#define AST_PECI_INT_CTRL 0x18
+#define AST_PECI_INT_STS  0x1c
+#define AST_PECI_W_DATA0  0x20
+#define AST_PECI_W_DATA1  0x24
+#define AST_PECI_W_DATA2  0x28
+#define AST_PECI_W_DATA3  0x2c
+#define AST_PECI_R_DATA0  0x30
+#define AST_PECI_R_DATA1  0x34
+#define AST_PECI_R_DATA2  0x38
+#define AST_PECI_R_DATA3  0x3c
+#define AST_PECI_W_DATA4  0x40
+#define AST_PECI_W_DATA5  0x44
+#define AST_PECI_W_DATA6  0x48
+#define AST_PECI_W_DATA7  0x4c
+#define AST_PECI_R_DATA4  0x50
+#define AST_PECI_R_DATA5  0x54
+#define AST_PECI_R_DATA6  0x58
+#define AST_PECI_R_DATA7  0x5c
+
+/* AST_PECI_CTRL - 0x00 : Control Register */
+#define PECI_CTRL_SAMPLING_MASK     GENMASK(19, 16)
+#define PECI_CTRL_SAMPLING(x)       (((x) << 16) & PECI_CTRL_SAMPLING_MASK)
+#define PECI_CTRL_SAMPLING_GET(x)   (((x) & PECI_CTRL_SAMPLING_MASK) >> 16)
+#define PECI_CTRL_READ_MODE_MASK    GENMASK(13, 12)
+#define PECI_CTRL_READ_MODE(x)      (((x) << 12) & PECI_CTRL_READ_MODE_MASK)
+#define PECI_CTRL_READ_MODE_GET(x)  (((x) & PECI_CTRL_READ_MODE_MASK) >> 12)
+#define PECI_CTRL_READ_MODE_COUNT   BIT(12)
+#define PECI_CTRL_READ_MODE_DBG     BIT(13)
+#define PECI_CTRL_CLK_SOURCE_MASK   BIT(11)
+#define PECI_CTRL_CLK_SOURCE(x)     (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK)
+#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11)
+#define PECI_CTRL_CLK_DIV_MASK      GENMASK(10, 8)
+#define PECI_CTRL_CLK_DIV(x)        (((x) << 8) & PECI_CTRL_CLK_DIV_MASK)
+#define PECI_CTRL_CLK_DIV_GET(x)    (((x) & PECI_CTRL_CLK_DIV_MASK) >> 8)
+#define PECI_CTRL_INVERT_OUT        BIT(7)
+#define PECI_CTRL_INVERT_IN         BIT(6)
+#define PECI_CTRL_BUS_CONTENT_EN    BIT(5)
+#define PECI_CTRL_PECI_EN           BIT(4)
+#define PECI_CTRL_PECI_CLK_EN       BIT(0)
+
+/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */
+#define PECI_TIMING_MESSAGE_MASK   GENMASK(15, 8)
+#define PECI_TIMING_MESSAGE(x)     (((x) << 8) & PECI_TIMING_MESSAGE_MASK)
+#define PECI_TIMING_MESSAGE_GET(x) (((x) & PECI_TIMING_MESSAGE_MASK) >> 8)
+#define PECI_TIMING_ADDRESS_MASK   GENMASK(7, 0)
+#define PECI_TIMING_ADDRESS(x)     ((x) & PECI_TIMING_ADDRESS_MASK)
+#define PECI_TIMING_ADDRESS_GET(x) ((x) & PECI_TIMING_ADDRESS_MASK)
+
+/* AST_PECI_CMD - 0x08 : Command Register */
+#define PECI_CMD_PIN_MON    BIT(31)
+#define PECI_CMD_STS_MASK   GENMASK(27, 24)
+#define PECI_CMD_STS_GET(x) (((x) & PECI_CMD_STS_MASK) >> 24)
+#define PECI_CMD_FIRE       BIT(0)
+
+/* AST_PECI_LEN - 0x0C : Read/Write Length Register */
+#define PECI_AW_FCS_EN       BIT(31)
+#define PECI_READ_LEN_MASK   GENMASK(23, 16)
+#define PECI_READ_LEN(x)     (((x) << 16) & PECI_READ_LEN_MASK)
+#define PECI_WRITE_LEN_MASK  GENMASK(15, 8)
+#define PECI_WRITE_LEN(x)    (((x) << 8) & PECI_WRITE_LEN_MASK)
+#define PECI_TAGET_ADDR_MASK GENMASK(7, 0)
+#define PECI_TAGET_ADDR(x)   ((x) & PECI_TAGET_ADDR_MASK)
+
+/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register */
+#define PECI_EXPECT_READ_FCS_MASK      GENMASK(23, 16)
+#define PECI_EXPECT_READ_FCS_GET(x)    (((x) & PECI_EXPECT_READ_FCS_MASK) >> 16)
+#define PECI_EXPECT_AW_FCS_AUTO_MASK   GENMASK(15, 8)
+#define PECI_EXPECT_AW_FCS_AUTO_GET(x) (((x) & PECI_EXPECT_AW_FCS_AUTO_MASK) \
+					>> 8)
+#define PECI_EXPECT_WRITE_FCS_MASK     GENMASK(7, 0)
+#define PECI_EXPECT_WRITE_FCS_GET(x)   ((x) & PECI_EXPECT_WRITE_FCS_MASK)
+
+/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */
+#define PECI_CAPTURE_READ_FCS_MASK    GENMASK(23, 16)
+#define PECI_CAPTURE_READ_FCS_GET(x)  (((x) & PECI_CAPTURE_READ_FCS_MASK) >> 16)
+#define PECI_CAPTURE_WRITE_FCS_MASK   GENMASK(7, 0)
+#define PECI_CAPTURE_WRITE_FCS_GET(x) ((x) & PECI_CAPTURE_WRITE_FCS_MASK)
+
+/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */
+#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30)
+#define PECI_INT_TIMEOUT            BIT(4)
+#define PECI_INT_CONNECT            BIT(3)
+#define PECI_INT_W_FCS_BAD          BIT(2)
+#define PECI_INT_W_FCS_ABORT        BIT(1)
+#define PECI_INT_CMD_DONE           BIT(0)
+
+struct aspeed_peci {
+	struct peci_adapter	adaper;
+	struct device		*dev;
+	struct regmap		*regmap;
+	int			irq;
+	struct completion	xfer_complete;
+	u32			status;
+	u32			cmd_timeout_ms;
+};
+
+#define PECI_INT_MASK  (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \
+			PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \
+			PECI_INT_CMD_DONE)
+
+#define PECI_IDLE_CHECK_TIMEOUT_MS      50
+#define PECI_IDLE_CHECK_INTERVAL_MS     10
+
+#define PECI_RD_SAMPLING_POINT_DEFAULT  8
+#define PECI_RD_SAMPLING_POINT_MAX      15
+#define PECI_CLK_DIV_DEFAULT            0
+#define PECI_CLK_DIV_MAX                7
+#define PECI_MSG_TIMING_NEGO_DEFAULT    1
+#define PECI_MSG_TIMING_NEGO_MAX        255
+#define PECI_ADDR_TIMING_NEGO_DEFAULT   1
+#define PECI_ADDR_TIMING_NEGO_MAX       255
+#define PECI_CMD_TIMEOUT_MS_DEFAULT     1000
+#define PECI_CMD_TIMEOUT_MS_MAX         60000
+
+static int aspeed_peci_xfer_native(struct aspeed_peci *priv,
+				   struct peci_xfer_msg *msg)
+{
+	long err, timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
+	u32 peci_head, peci_state, rx_data, cmd_sts;
+	ktime_t start, end;
+	s64 elapsed_ms;
+	int i, rc = 0;
+	uint reg;
+
+	start = ktime_get();
+
+	/* Check command sts and bus idle state */
+	while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts) &&
+	       (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
+		end = ktime_get();
+		elapsed_ms = ktime_to_ms(ktime_sub(end, start));
+		if (elapsed_ms >= PECI_IDLE_CHECK_TIMEOUT_MS) {
+			dev_dbg(priv->dev, "Timeout waiting for idle state!\n");
+			return -ETIMEDOUT;
+		}
+
+		usleep_range(PECI_IDLE_CHECK_INTERVAL_MS * 1000,
+			     (PECI_IDLE_CHECK_INTERVAL_MS * 1000) + 1000);
+	};
+
+	reinit_completion(&priv->xfer_complete);
+
+	peci_head = PECI_TAGET_ADDR(msg->addr) |
+				    PECI_WRITE_LEN(msg->tx_len) |
+				    PECI_READ_LEN(msg->rx_len);
+
+	rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < msg->tx_len; i += 4) {
+		reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 :
+			       AST_PECI_W_DATA4 + i % 16;
+		rc = regmap_write(priv->regmap, reg,
+				  (msg->tx_buf[i + 3] << 24) |
+				  (msg->tx_buf[i + 2] << 16) |
+				  (msg->tx_buf[i + 1] << 8) |
+				  msg->tx_buf[i + 0]);
+		if (rc)
+			return rc;
+	}
+
+	dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
+#if DUMP_DEBUG
+	print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1,
+		       msg->tx_buf, msg->tx_len, true);
+#endif
+
+	rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE);
+	if (rc)
+		return rc;
+
+	err = wait_for_completion_interruptible_timeout(&priv->xfer_complete,
+							timeout);
+
+	dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->status);
+	if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
+		dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
+			PECI_CMD_STS_GET(peci_state));
+	else
+		dev_dbg(priv->dev, "PECI_STATE : read error\n");
+
+	rc = regmap_write(priv->regmap, AST_PECI_CMD, 0);
+	if (rc)
+		return rc;
+
+	if (err <= 0 || !(priv->status & PECI_INT_CMD_DONE)) {
+		if (err < 0) { /* -ERESTARTSYS */
+			return (int)err;
+		} else if (err == 0) {
+			dev_dbg(priv->dev, "Timeout waiting for a response!\n");
+			return -ETIMEDOUT;
+		}
+
+		dev_dbg(priv->dev, "No valid response!\n");
+		return -EIO;
+	}
+
+	for (i = 0; i < msg->rx_len; i++) {
+		u8 byte_offset = i % 4;
+
+		if (byte_offset == 0) {
+			reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 :
+				       AST_PECI_R_DATA4 + i % 16;
+			rc = regmap_read(priv->regmap, reg, &rx_data);
+			if (rc)
+				return rc;
+		}
+
+		msg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3));
+	}
+
+#if DUMP_DEBUG
+	print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1,
+		       msg->rx_buf, msg->rx_len, true);
+#endif
+	if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
+		dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
+			PECI_CMD_STS_GET(peci_state));
+	else
+		dev_dbg(priv->dev, "PECI_STATE : read error\n");
+	dev_dbg(priv->dev, "------------------------\n");
+
+	return rc;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+	struct aspeed_peci *priv = arg;
+	u32 status_ack = 0;
+
+	if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->status))
+		return IRQ_NONE;
+
+	/* Be noted that multiple interrupt bits can be set at the same time */
+	if (priv->status & PECI_INT_TIMEOUT) {
+		dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n");
+		status_ack |= PECI_INT_TIMEOUT;
+	}
+
+	if (priv->status & PECI_INT_CONNECT) {
+		dev_dbg(priv->dev, "PECI_INT_CONNECT\n");
+		status_ack |= PECI_INT_CONNECT;
+	}
+
+	if (priv->status & PECI_INT_W_FCS_BAD) {
+		dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n");
+		status_ack |= PECI_INT_W_FCS_BAD;
+	}
+
+	if (priv->status & PECI_INT_W_FCS_ABORT) {
+		dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n");
+		status_ack |= PECI_INT_W_FCS_ABORT;
+	}
+
+	/**
+	 * All commands should be ended up with a PECI_INT_CMD_DONE bit set
+	 * even in an error case.
+	 */
+	if (priv->status & PECI_INT_CMD_DONE) {
+		dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n");
+		status_ack |= PECI_INT_CMD_DONE;
+		complete(&priv->xfer_complete);
+	}
+
+	if (regmap_write(priv->regmap, AST_PECI_INT_STS, status_ack))
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)
+{
+	u32 msg_timing_nego, addr_timing_nego, rd_sampling_point;
+	u32 clk_freq, clk_divisor, clk_div_val = 0;
+	struct clk *clkin;
+	int ret;
+
+	clkin = devm_clk_get(priv->dev, NULL);
+	if (IS_ERR(clkin)) {
+		dev_err(priv->dev, "Failed to get clk source.\n");
+		return PTR_ERR(clkin);
+	}
+
+	ret = of_property_read_u32(priv->dev->of_node, "clock-frequency",
+				   &clk_freq);
+	if (ret < 0) {
+		dev_err(priv->dev,
+			"Could not read clock-frequency property.\n");
+		return ret;
+	}
+
+	clk_divisor = clk_get_rate(clkin) / clk_freq;
+	devm_clk_put(priv->dev, clkin);
+
+	while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX))
+		clk_div_val++;
+
+	ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego",
+				   &msg_timing_nego);
+	if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) {
+		dev_warn(priv->dev,
+			 "Invalid msg-timing-nego : %u, Use default : %u\n",
+			 msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT);
+		msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT;
+	}
+
+	ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego",
+				   &addr_timing_nego);
+	if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) {
+		dev_warn(priv->dev,
+			 "Invalid addr-timing-nego : %u, Use default : %u\n",
+			 addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT);
+		addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT;
+	}
+
+	ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point",
+				   &rd_sampling_point);
+	if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) {
+		dev_warn(priv->dev,
+			 "Invalid rd-sampling-point : %u. Use default : %u\n",
+			 rd_sampling_point,
+			 PECI_RD_SAMPLING_POINT_DEFAULT);
+		rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT;
+	}
+
+	ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms",
+				   &priv->cmd_timeout_ms);
+	if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX ||
+	    priv->cmd_timeout_ms == 0) {
+		dev_warn(priv->dev,
+			 "Invalid cmd-timeout-ms : %u. Use default : %u\n",
+			 priv->cmd_timeout_ms,
+			 PECI_CMD_TIMEOUT_MS_DEFAULT);
+		priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT;
+	}
+
+	ret = regmap_write(priv->regmap, AST_PECI_CTRL,
+			   PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) |
+			   PECI_CTRL_PECI_CLK_EN);
+	if (ret)
+		return ret;
+
+	usleep_range(1000, 5000);
+
+	/**
+	 * Timing negotiation period setting.
+	 * The unit of the programmed value is 4 times of PECI clock period.
+	 */
+	ret = regmap_write(priv->regmap, AST_PECI_TIMING,
+			   PECI_TIMING_MESSAGE(msg_timing_nego) |
+			   PECI_TIMING_ADDRESS(addr_timing_nego));
+	if (ret)
+		return ret;
+
+	/* Clear interrupts */
+	ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK);
+	if (ret)
+		return ret;
+
+	/* Enable interrupts */
+	ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK);
+	if (ret)
+		return ret;
+
+	/* Read sampling point and clock speed setting */
+	ret = regmap_write(priv->regmap, AST_PECI_CTRL,
+			   PECI_CTRL_SAMPLING(rd_sampling_point) |
+			   PECI_CTRL_CLK_DIV(clk_div_val) |
+			   PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct regmap_config aspeed_peci_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = AST_PECI_R_DATA7,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.fast_io = true,
+};
+
+static int aspeed_peci_xfer(struct peci_adapter *adaper,
+			    struct peci_xfer_msg *msg)
+{
+	struct aspeed_peci *priv = peci_get_adapdata(adaper);
+
+	return aspeed_peci_xfer_native(priv, msg);
+}
+
+static int aspeed_peci_probe(struct platform_device *pdev)
+{
+	struct aspeed_peci *priv;
+	struct resource *res;
+	void __iomem *base;
+	int ret = 0;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, priv);
+	priv->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					     &aspeed_peci_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (!priv->irq)
+		return -ENODEV;
+
+	ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler,
+			       IRQF_SHARED,
+			       "peci-aspeed-irq",
+			       priv);
+	if (ret < 0)
+		return ret;
+
+	init_completion(&priv->xfer_complete);
+
+	priv->adaper.dev.parent = priv->dev;
+	priv->adaper.dev.of_node = of_node_get(dev_of_node(priv->dev));
+	strlcpy(priv->adaper.name, pdev->name, sizeof(priv->adaper.name));
+	priv->adaper.xfer = aspeed_peci_xfer;
+	peci_set_adapdata(&priv->adaper, priv);
+
+	ret = aspeed_peci_init_ctrl(priv);
+	if (ret < 0)
+		return ret;
+
+	ret = peci_add_adapter(&priv->adaper);
+	if (ret < 0)
+		return ret;
+
+	dev_info(&pdev->dev, "peci bus %d registered, irq %d\n",
+		 priv->adaper.nr, priv->irq);
+
+	return 0;
+}
+
+static int aspeed_peci_remove(struct platform_device *pdev)
+{
+	struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);
+
+	peci_del_adapter(&priv->adaper);
+	of_node_put(priv->adaper.dev.of_node);
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_peci_of_table[] = {
+	{ .compatible = "aspeed,ast2400-peci", },
+	{ .compatible = "aspeed,ast2500-peci", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
+
+static struct platform_driver aspeed_peci_driver = {
+	.probe  = aspeed_peci_probe,
+	.remove = aspeed_peci_remove,
+	.driver = {
+		.name           = "peci-aspeed",
+		.of_match_table = of_match_ptr(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 v2");