[07/14] peci: Add peci-aspeed controller driver

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       |  12 +
 drivers/peci/controller/Makefile      |   3 +
 drivers/peci/controller/peci-aspeed.c | 501 ++++++++++++++++++++++++++
 6 files changed, 534 insertions(+)
 create mode 100644 drivers/peci/controller/Kconfig
 create mode 100644 drivers/peci/controller/Makefile
 create mode 100644 drivers/peci/controller/peci-aspeed.c

On 7/12/21 3:04 PM, Iwona Winiarska wrote:
> diff --git a/drivers/peci/controller/Kconfig b/drivers/peci/controller/Kconfig

> new file mode 100644

> index 000000000000..8ddbe494677f

> --- /dev/null

> +++ b/drivers/peci/controller/Kconfig

> @@ -0,0 +1,12 @@

> +# 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

> +	  Enable this driver if you want to support ASPEED PECI controller.

> +

> +	  This driver can be also build as a module. If so, the module


	              can also be built as a module.

> +	  will be called peci-aspeed.



-- 
~Randy

On Tue, 2021-07-13 at 00:04 +0200, Iwona Winiarska 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).

> 

> 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       |  12 +

>  drivers/peci/controller/Makefile      |   3 +

>  drivers/peci/controller/peci-aspeed.c | 501 ++++++++++++++++++++++++++

>  6 files changed, 534 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 47411e2b6336..4ba874afa2fa 100644

> --- a/MAINTAINERS

> +++ b/MAINTAINERS

> @@ -2865,6 +2865,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 601cc3c3c852..0d0ee8009713 100644

> --- a/drivers/peci/Kconfig

> +++ b/drivers/peci/Kconfig

> @@ -12,3 +12,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 2bb2f51bcda7..621a993e306a 100644

> --- a/drivers/peci/Makefile

> +++ b/drivers/peci/Makefile

> @@ -3,3 +3,6 @@

>  # Core functionality

>  peci-y := core.o sysfs.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..8ddbe494677f

> --- /dev/null

> +++ b/drivers/peci/controller/Kconfig

> @@ -0,0 +1,12 @@

> +# 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

> +         Enable this driver if you want to support ASPEED PECI controller.


Perhaps a note about how one might make this determination, or maybe a
general recommendation that if they are building for deployment on an
OpenBMC system say Y else say N?

> +

> +         This driver can be also build 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..888b46383ea4

> --- /dev/null

> +++ b/drivers/peci/controller/peci-aspeed.c

> @@ -0,0 +1,501 @@

> +// 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_READ_MODE_MASK      GENMASK(13, 12)

> +#define   ASPEED_PECI_CTRL_READ_MODE_COUNT     BIT(12)

> +#define   ASPEED_PECI_CTRL_READ_MODE_DBG       BIT(13)

> +#define   ASPEED_PECI_CTRL_CLK_SOURCE_MASK     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_CONTENT_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_TIMING_MESSAGE_MASK      GENMASK(15, 8)

> +#define   ASPEED_PECI_TIMING_ADDRESS_MASK      GENMASK(7, 0)

> +

> +/* Command Register */

> +#define ASPEED_PECI_CMD                                0x08

> +#define   ASPEED_PECI_CMD_PIN_MON              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_MON)

> +#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_READ_LEN_MASK            GENMASK(23, 16)

> +#define   ASPEED_PECI_WRITE_LEN_MASK           GENMASK(15, 8)

> +#define   ASPEED_PECI_TAGET_ADDR_MASK          GENMASK(7, 0)

> +

> +/* Expected FCS Data Register */

> +#define ASPEED_PECI_EXP_FCS                    0x10

> +#define   ASPEED_PECI_EXP_READ_FCS_MASK                GENMASK(23, 16)

> +#define   ASPEED_PECI_EXP_AW_FCS_AUTO_MASK     GENMASK(15, 8)

> +#define   ASPEED_PECI_EXP_WRITE_FCS_MASK       GENMASK(7, 0)

> +

> +/* Captured FCS Data Register */

> +#define ASPEED_PECI_CAP_FCS                    0x14

> +#define   ASPEED_PECI_CAP_READ_FCS_MASK                GENMASK(23, 16)

> +#define   ASPEED_PECI_CAP_WRITE_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_CONNECT          BIT(3)

> +#define   ASPEED_PECI_INT_W_FCS_BAD            BIT(2)

> +#define   ASPEED_PECI_INT_W_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_W_DATA0                    0x20

> +#define ASPEED_PECI_W_DATA1                    0x24

> +#define ASPEED_PECI_W_DATA2                    0x28

> +#define ASPEED_PECI_W_DATA3                    0x2c

> +#define ASPEED_PECI_R_DATA0                    0x30

> +#define ASPEED_PECI_R_DATA1                    0x34

> +#define ASPEED_PECI_R_DATA2                    0x38

> +#define ASPEED_PECI_R_DATA3                    0x3c

> +#define ASPEED_PECI_W_DATA4                    0x40

> +#define ASPEED_PECI_W_DATA5                    0x44

> +#define ASPEED_PECI_W_DATA6                    0x48

> +#define ASPEED_PECI_W_DATA7                    0x4c

> +#define ASPEED_PECI_R_DATA4                    0x50

> +#define ASPEED_PECI_R_DATA5                    0x54

> +#define ASPEED_PECI_R_DATA6                    0x58

> +#define ASPEED_PECI_R_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;


Uh oh... this looks like a driver private data structure, and I know
there's a 'struct device' allocated in @controller. /me goes to check
->probe()...

> +       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 inline struct aspeed_peci *to_aspeed_peci(struct peci_controller *a)

> +{

> +       return container_of(a, struct aspeed_peci, controller);

> +}

> +

> +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_TIMING_MESSAGE_MASK, priv->msg_timing);

> +       val |= FIELD_PREP(ASPEED_PECI_TIMING_ADDRESS_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);


Do these MMIO access follow a standard? I.e. is there any possibility
to have a common / generic MMIO xfer function, but just pass in a
different base address discovered by the PECI device rather than a
fully custom xfer function per controller?

> +}

> +

> +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 = to_aspeed_peci(controller);

> +       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_TAGET_ADDR_MASK, addr) |

> +                   FIELD_PREP(ASPEED_PECI_WRITE_LEN_MASK, req->tx.len) |

> +                   FIELD_PREP(ASPEED_PECI_READ_LEN_MASK, req->rx.len);

> +

> +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);

> +

> +       memcpy_toio(priv->base + ASPEED_PECI_W_DATA0, req->tx.buf,

> +                   req->tx.len > 16 ? 16 : req->tx.len);

> +       if (req->tx.len > 16)

> +               memcpy_toio(priv->base + ASPEED_PECI_W_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_R_DATA0,

> +                     req->rx.len > 16 ? 16 : req->rx.len);

> +       if (req->rx.len > 16)

> +               memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_R_DATA4,

> +                             req->rx.len - 16);

> +

> +       print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);


If dynamic debug is not enabled this will be an unconditional
printk(KERN_DEBUG.

I'm ok with dev_dbg() in slow paths, but in fast paths you should look
to tracing, or putting potentially heavyweight debug behind a
CONFIG_X_DEBUG option. I have seen Greg is even less of a fan of
dev_dbg().

> +

> +       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_CONNECT)

> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONNECT\n");

> +

> +       if (status & ASPEED_PECI_INT_W_FCS_BAD)

> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_BAD\n");

> +

> +       if (status & ASPEED_PECI_INT_W_FCS_ABORT)

> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_ABORT\n");


What's the utility of these debug statements? If they are for
development than maybe they are ok, if they are for debug in the field
I would make them counters and export them via debugfs, or sysfs if
you expect to always be able to debug these events in case a kernel in
the field has dev_dbg and debugfs disabled.

> +

> +       /*

> +        * 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 __sanitize_clock_divider(struct aspeed_peci *priv)

> +{

> +       u32 clk_div;

> +       int ret;

> +

> +       ret = device_property_read_u32(priv->dev, "clock-divider", &clk_div);

> +       if (ret) {

> +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;

> +       } else if (clk_div > ASPEED_PECI_CLK_DIV_MAX) {

> +               dev_warn(priv->dev, "Invalid clock-divider: %u, Using default: %u\n",

> +                        clk_div, ASPEED_PECI_CLK_DIV_DEFAULT);

> +

> +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;

> +       }

> +

> +       priv->clk_div = clk_div;

> +}

> +

> +static void __sanitize_msg_timing(struct aspeed_peci *priv)

> +{

> +       u32 msg_timing;

> +       int ret;

> +

> +       ret = device_property_read_u32(priv->dev, "msg-timing", &msg_timing);

> +       if (ret) {

> +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;

> +       } else if (msg_timing > ASPEED_PECI_MSG_TIMING_MAX) {

> +               dev_warn(priv->dev, "Invalid msg-timing : %u, Use default : %u\n",

> +                        msg_timing, ASPEED_PECI_MSG_TIMING_DEFAULT);

> +

> +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;

> +       }

> +

> +       priv->msg_timing = msg_timing;

> +}

> +

> +static void __sanitize_addr_timing(struct aspeed_peci *priv)

> +{

> +       u32 addr_timing;

> +       int ret;

> +

> +       ret = device_property_read_u32(priv->dev, "addr-timing", &addr_timing);

> +       if (ret) {

> +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;

> +       } else if (addr_timing > ASPEED_PECI_ADDR_TIMING_MAX) {

> +               dev_warn(priv->dev, "Invalid addr-timing : %u, Use default : %u\n",

> +                        addr_timing, ASPEED_PECI_ADDR_TIMING_DEFAULT);

> +

> +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;

> +       }

> +

> +       priv->addr_timing = addr_timing;

> +}

> +

> +static void __sanitize_rd_sampling_point(struct aspeed_peci *priv)

> +{

> +       u32 rd_sampling_point;

> +       int ret;

> +

> +       ret = device_property_read_u32(priv->dev, "rd-sampling-point", &rd_sampling_point);

> +       if (ret) {

> +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;

> +       } else if (rd_sampling_point > ASPEED_PECI_RD_SAMPLING_POINT_MAX) {

> +               dev_warn(priv->dev, "Invalid rd-sampling-point: %u, Use default : %u\n",

> +                        rd_sampling_point, ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);

> +

> +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;

> +       }

> +

> +       priv->rd_sampling_point = rd_sampling_point;

> +}

> +

> +static void __sanitize_cmd_timeout(struct aspeed_peci *priv)

> +{

> +       u32 timeout;

> +       int ret;

> +

> +       ret = device_property_read_u32(priv->dev, "cmd-timeout-ms", &timeout);

> +       if (ret) {

> +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;

> +       } else if (timeout > ASPEED_PECI_CMD_TIMEOUT_MS_MAX || timeout == 0) {

> +               dev_warn(priv->dev, "Invalid cmd-timeout-ms: %u, Use default: %u\n",

> +                        timeout, ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT);

> +


For all of the same pattern like this above I would say "falling back
to: %u" otherwise "Use default" sounds like an action the platform
owner is expected to take.

Also, if the driver is correcting the issue does the log need to be
spammed with a warning? Is this 'info' or 'debug'?

> +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;

> +       }

> +

> +       priv->cmd_timeout_ms = timeout;

> +}

> +

> +static void aspeed_peci_device_property_sanitize(struct aspeed_peci *priv)

> +{

> +       __sanitize_clock_divider(priv);

> +       __sanitize_msg_timing(priv);

> +       __sanitize_addr_timing(priv);

> +       __sanitize_rd_sampling_point(priv);

> +       __sanitize_cmd_timeout(priv);

> +}

> +

> +static void aspeed_peci_disable_clk(void *data)

> +{

> +       clk_disable_unprepare(data);

> +}

> +

> +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)

> +{

> +       int ret;

> +

> +       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 source\n");

> +

> +       ret = clk_prepare_enable(priv->clk);

> +       if (ret) {

> +               dev_err(priv->dev, "Failed to enable clock\n");

> +               return ret;

> +       }

> +

> +       ret = devm_add_action_or_reset(priv->dev, aspeed_peci_disable_clk, priv->clk);

> +       if (ret)

> +               return ret;

> +

> +       aspeed_peci_device_property_sanitize(priv);

> +

> +       aspeed_peci_init_regs(priv);

> +

> +       return 0;

> +}

> +

> +static int aspeed_peci_probe(struct platform_device *pdev)

> +{

> +       struct aspeed_peci *priv;

> +       int ret;

> +

> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);


..."uh oh" from above confirmed. devm allocation lifetime and 'struct
device' lifetime are not compatible.

You can trigger use after free bugs by turning on
CONFIG_DEBUG_KOBJECT_RELEASE.

devm can be used to automatically unregister the peci_controller
device. The flow would be something like:

priv = devm_kzalloc(..., sizeof(*priv), ...);
controller = peci_controller_alloc(...);
if (IS_ERR(controller))
    return PTR_ERR(controller);
rc = devm_peci_controller_add(...)
if (rc)
    return rc;

This arranges for the peci_controller_alloc() to be undone by
put_device() in all cases. Internal to peci_controller_alloc() is
typical goto unwind allocation error handling.

> +       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-irq", priv);

> +       if (ret)

> +               return ret;

> +

> +       init_completion(&priv->xfer_complete);

> +       spin_lock_init(&priv->lock);

> +

> +       priv->controller.xfer = aspeed_peci_xfer;

> +

> +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);

> +       if (IS_ERR(priv->rst)) {

> +               dev_err(&pdev->dev, "Missing or invalid reset controller entry\n");

> +               return PTR_ERR(priv->rst);

> +       }

> +       reset_control_deassert(priv->rst);

> +

> +       ret = aspeed_peci_init_ctrl(priv);

> +       if (ret)

> +               return ret;

> +

> +       return peci_controller_add(&priv->controller, priv->dev);

> +}

> +

> +static int aspeed_peci_remove(struct platform_device *pdev)

> +{

> +       struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);

> +

> +       peci_controller_remove(&priv->controller);

> +       reset_control_assert(priv->rst);

> +


It's odd to have devm in the probe path and still publish a remove
handler, i.e. why not handle controller removal and reset via devm?
The example above with devm_peci_controller_add() already assumes
peci_controller_remove is triggered by devm, reset assert can be
managed the same way.

> +       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,

> +       .remove = aspeed_peci_remove,

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


Same comments about MODULE_AUTHOR from patch 6, i.e. make sure this is
not duplicating what MAINTAINERS and git log handle.

I'll pause here until you've had a chance to consider fixes to the
devm vs 'struct device' lifetime issue.

> +MODULE_DESCRIPTION("ASPEED PECI driver");

> +MODULE_LICENSE("GPL");

> +MODULE_IMPORT_NS(PECI);

On Mon, 2021-07-12 at 22:02 -0700, Randy Dunlap wrote:
> On 7/12/21 3:04 PM, Iwona Winiarska wrote:

> > diff --git a/drivers/peci/controller/Kconfig

> > b/drivers/peci/controller/Kconfig

> > new file mode 100644

> > index 000000000000..8ddbe494677f

> > --- /dev/null

> > +++ b/drivers/peci/controller/Kconfig

> > @@ -0,0 +1,12 @@

> > +# 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

> > +         Enable this driver if you want to support ASPEED PECI

> > controller.

> > +

> > +         This driver can be also build as a module. If so, the

> > module

> 

>                       can also be built as a module.


Thank you Randy - I'll fix this in v2.

-Iwona

> 

> > +         will be called peci-aspeed.

> 

> 

> -- 

> ~Randy

>

On Wed, 2021-07-14 at 17:39 +0000, Williams, Dan J wrote:
> On Tue, 2021-07-13 at 00:04 +0200, Iwona Winiarska 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).

> > 

> > 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       |  12 +

> >  drivers/peci/controller/Makefile      |   3 +

> >  drivers/peci/controller/peci-aspeed.c | 501 ++++++++++++++++++++++++++

> >  6 files changed, 534 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 47411e2b6336..4ba874afa2fa 100644

> > --- a/MAINTAINERS

> > +++ b/MAINTAINERS

> > @@ -2865,6 +2865,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 601cc3c3c852..0d0ee8009713 100644

> > --- a/drivers/peci/Kconfig

> > +++ b/drivers/peci/Kconfig

> > @@ -12,3 +12,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 2bb2f51bcda7..621a993e306a 100644

> > --- a/drivers/peci/Makefile

> > +++ b/drivers/peci/Makefile

> > @@ -3,3 +3,6 @@

> >  # Core functionality

> >  peci-y := core.o sysfs.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..8ddbe494677f

> > --- /dev/null

> > +++ b/drivers/peci/controller/Kconfig

> > @@ -0,0 +1,12 @@

> > +# 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

> > +         Enable this driver if you want to support ASPEED PECI controller.

> 

> Perhaps a note about how one might make this determination, or maybe a

> general recommendation that if they are building for deployment on an

> OpenBMC system say Y else say N?


Ack.

> 

> > +

> > +         This driver can be also build 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..888b46383ea4

> > --- /dev/null

> > +++ b/drivers/peci/controller/peci-aspeed.c

> > @@ -0,0 +1,501 @@

> > +// 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_READ_MODE_MASK      GENMASK(13, 12)

> > +#define   ASPEED_PECI_CTRL_READ_MODE_COUNT     BIT(12)

> > +#define   ASPEED_PECI_CTRL_READ_MODE_DBG       BIT(13)

> > +#define   ASPEED_PECI_CTRL_CLK_SOURCE_MASK     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_CONTENT_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_TIMING_MESSAGE_MASK      GENMASK(15, 8)

> > +#define   ASPEED_PECI_TIMING_ADDRESS_MASK      GENMASK(7, 0)

> > +

> > +/* Command Register */

> > +#define ASPEED_PECI_CMD                                0x08

> > +#define   ASPEED_PECI_CMD_PIN_MON              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_MON)

> > +#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_READ_LEN_MASK            GENMASK(23, 16)

> > +#define   ASPEED_PECI_WRITE_LEN_MASK           GENMASK(15, 8)

> > +#define   ASPEED_PECI_TAGET_ADDR_MASK          GENMASK(7, 0)

> > +

> > +/* Expected FCS Data Register */

> > +#define ASPEED_PECI_EXP_FCS                    0x10

> > +#define   ASPEED_PECI_EXP_READ_FCS_MASK                GENMASK(23, 16)

> > +#define   ASPEED_PECI_EXP_AW_FCS_AUTO_MASK     GENMASK(15, 8)

> > +#define   ASPEED_PECI_EXP_WRITE_FCS_MASK       GENMASK(7, 0)

> > +

> > +/* Captured FCS Data Register */

> > +#define ASPEED_PECI_CAP_FCS                    0x14

> > +#define   ASPEED_PECI_CAP_READ_FCS_MASK                GENMASK(23, 16)

> > +#define   ASPEED_PECI_CAP_WRITE_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_CONNECT          BIT(3)

> > +#define   ASPEED_PECI_INT_W_FCS_BAD            BIT(2)

> > +#define   ASPEED_PECI_INT_W_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_W_DATA0                    0x20

> > +#define ASPEED_PECI_W_DATA1                    0x24

> > +#define ASPEED_PECI_W_DATA2                    0x28

> > +#define ASPEED_PECI_W_DATA3                    0x2c

> > +#define ASPEED_PECI_R_DATA0                    0x30

> > +#define ASPEED_PECI_R_DATA1                    0x34

> > +#define ASPEED_PECI_R_DATA2                    0x38

> > +#define ASPEED_PECI_R_DATA3                    0x3c

> > +#define ASPEED_PECI_W_DATA4                    0x40

> > +#define ASPEED_PECI_W_DATA5                    0x44

> > +#define ASPEED_PECI_W_DATA6                    0x48

> > +#define ASPEED_PECI_W_DATA7                    0x4c

> > +#define ASPEED_PECI_R_DATA4                    0x50

> > +#define ASPEED_PECI_R_DATA5                    0x54

> > +#define ASPEED_PECI_R_DATA6                    0x58

> > +#define ASPEED_PECI_R_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;

> 

> Uh oh... this looks like a driver private data structure, and I know

> there's a 'struct device' allocated in @controller. /me goes to check

> ->probe()...

> 

> > +       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 inline struct aspeed_peci *to_aspeed_peci(struct peci_controller *a)

> > +{

> > +       return container_of(a, struct aspeed_peci, controller);

> > +}

> > +

> > +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_TIMING_MESSAGE_MASK, priv->msg_timing);

> > +       val |= FIELD_PREP(ASPEED_PECI_TIMING_ADDRESS_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);

> 

> Do these MMIO access follow a standard? I.e. is there any possibility

> to have a common / generic MMIO xfer function, but just pass in a

> different base address discovered by the PECI device rather than a

> fully custom xfer function per controller?

> 


No, it's vendor specific.

> > +}

> > +

> > +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 = to_aspeed_peci(controller);

> > +       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_TAGET_ADDR_MASK, addr) |

> > +                   FIELD_PREP(ASPEED_PECI_WRITE_LEN_MASK, req->tx.len) |

> > +                   FIELD_PREP(ASPEED_PECI_READ_LEN_MASK, req->rx.len);

> > +

> > +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);

> > +

> > +       memcpy_toio(priv->base + ASPEED_PECI_W_DATA0, req->tx.buf,

> > +                   req->tx.len > 16 ? 16 : req->tx.len);

> > +       if (req->tx.len > 16)

> > +               memcpy_toio(priv->base + ASPEED_PECI_W_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_R_DATA0,

> > +                     req->rx.len > 16 ? 16 : req->rx.len);

> > +       if (req->rx.len > 16)

> > +               memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_R_DATA4,

> > +                             req->rx.len - 16);

> > +

> > +       print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);

> 

> If dynamic debug is not enabled this will be an unconditional

> printk(KERN_DEBUG.

> 

> I'm ok with dev_dbg() in slow paths, but in fast paths you should look

> to tracing, or putting potentially heavyweight debug behind a

> CONFIG_X_DEBUG option. I have seen Greg is even less of a fan of

> dev_dbg().

> 


Except hiding that behind CONFIG_PECI_DEBUG would require "custom"
printers (to avoid sprinkling ifdefs all over the code) and people seem
to dislike custom printers :(
If it was entirely up to me - I'd hide everything used for debug
("verbose" debug - hotpath or not) under peci_trace (that's noop if
!CONFIG_PECI_DEBUG) printer and use trace_printk under the hood.

> > +

> > +       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_CONNECT)

> > +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONNECT\n");

> > +

> > +       if (status & ASPEED_PECI_INT_W_FCS_BAD)

> > +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_BAD\n");

> > +

> > +       if (status & ASPEED_PECI_INT_W_FCS_ABORT)

> > +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_ABORT\n");

> 

> What's the utility of these debug statements? If they are for

> development than maybe they are ok, if they are for debug in the field

> I would make them counters and export them via debugfs, or sysfs if

> you expect to always be able to debug these events in case a kernel in

> the field has dev_dbg and debugfs disabled.

> 


Development for now. Once the subsystem develops a bit more (with
additional controllers), we can think about adding counters (or
tracepoints?) that can be useful in the field but are not controller
specific.

> > +

> > +       /*

> > +        * 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 __sanitize_clock_divider(struct aspeed_peci *priv)

> > +{

> > +       u32 clk_div;

> > +       int ret;

> > +

> > +       ret = device_property_read_u32(priv->dev, "clock-divider", &clk_div);

> > +       if (ret) {

> > +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;

> > +       } else if (clk_div > ASPEED_PECI_CLK_DIV_MAX) {

> > +               dev_warn(priv->dev, "Invalid clock-divider: %u, Using default: %u\n",

> > +                        clk_div, ASPEED_PECI_CLK_DIV_DEFAULT);

> > +

> > +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;

> > +       }

> > +

> > +       priv->clk_div = clk_div;

> > +}

> > +

> > +static void __sanitize_msg_timing(struct aspeed_peci *priv)

> > +{

> > +       u32 msg_timing;

> > +       int ret;

> > +

> > +       ret = device_property_read_u32(priv->dev, "msg-timing", &msg_timing);

> > +       if (ret) {

> > +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;

> > +       } else if (msg_timing > ASPEED_PECI_MSG_TIMING_MAX) {

> > +               dev_warn(priv->dev, "Invalid msg-timing : %u, Use default : %u\n",

> > +                        msg_timing, ASPEED_PECI_MSG_TIMING_DEFAULT);

> > +

> > +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;

> > +       }

> > +

> > +       priv->msg_timing = msg_timing;

> > +}

> > +

> > +static void __sanitize_addr_timing(struct aspeed_peci *priv)

> > +{

> > +       u32 addr_timing;

> > +       int ret;

> > +

> > +       ret = device_property_read_u32(priv->dev, "addr-timing", &addr_timing);

> > +       if (ret) {

> > +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;

> > +       } else if (addr_timing > ASPEED_PECI_ADDR_TIMING_MAX) {

> > +               dev_warn(priv->dev, "Invalid addr-timing : %u, Use default : %u\n",

> > +                        addr_timing, ASPEED_PECI_ADDR_TIMING_DEFAULT);

> > +

> > +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;

> > +       }

> > +

> > +       priv->addr_timing = addr_timing;

> > +}

> > +

> > +static void __sanitize_rd_sampling_point(struct aspeed_peci *priv)

> > +{

> > +       u32 rd_sampling_point;

> > +       int ret;

> > +

> > +       ret = device_property_read_u32(priv->dev, "rd-sampling-point", &rd_sampling_point);

> > +       if (ret) {

> > +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;

> > +       } else if (rd_sampling_point > ASPEED_PECI_RD_SAMPLING_POINT_MAX) {

> > +               dev_warn(priv->dev, "Invalid rd-sampling-point: %u, Use default : %u\n",

> > +                        rd_sampling_point, ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);

> > +

> > +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;

> > +       }

> > +

> > +       priv->rd_sampling_point = rd_sampling_point;

> > +}

> > +

> > +static void __sanitize_cmd_timeout(struct aspeed_peci *priv)

> > +{

> > +       u32 timeout;

> > +       int ret;

> > +

> > +       ret = device_property_read_u32(priv->dev, "cmd-timeout-ms", &timeout);

> > +       if (ret) {

> > +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;

> > +       } else if (timeout > ASPEED_PECI_CMD_TIMEOUT_MS_MAX || timeout == 0) {

> > +               dev_warn(priv->dev, "Invalid cmd-timeout-ms: %u, Use default: %u\n",

> > +                        timeout, ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT);

> > +

> 

> For all of the same pattern like this above I would say "falling back

> to: %u" otherwise "Use default" sounds like an action the platform

> owner is expected to take.

> 


Agree.

> Also, if the driver is correcting the issue does the log need to be

> spammed with a warning? Is this 'info' or 'debug'?


It suggests that device tree for that platform is incompatible with the
schema ("make dtbs_check" wasn't done).
For that reason I would prefer to keep it as a warning.

> 

> > +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;

> > +       }

> > +

> > +       priv->cmd_timeout_ms = timeout;

> > +}

> > +

> > +static void aspeed_peci_device_property_sanitize(struct aspeed_peci *priv)

> > +{

> > +       __sanitize_clock_divider(priv);

> > +       __sanitize_msg_timing(priv);

> > +       __sanitize_addr_timing(priv);

> > +       __sanitize_rd_sampling_point(priv);

> > +       __sanitize_cmd_timeout(priv);

> > +}

> > +

> > +static void aspeed_peci_disable_clk(void *data)

> > +{

> > +       clk_disable_unprepare(data);

> > +}

> > +

> > +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)

> > +{

> > +       int ret;

> > +

> > +       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 source\n");

> > +

> > +       ret = clk_prepare_enable(priv->clk);

> > +       if (ret) {

> > +               dev_err(priv->dev, "Failed to enable clock\n");

> > +               return ret;

> > +       }

> > +

> > +       ret = devm_add_action_or_reset(priv->dev, aspeed_peci_disable_clk, priv->clk);

> > +       if (ret)

> > +               return ret;

> > +

> > +       aspeed_peci_device_property_sanitize(priv);

> > +

> > +       aspeed_peci_init_regs(priv);

> > +

> > +       return 0;

> > +}

> > +

> > +static int aspeed_peci_probe(struct platform_device *pdev)

> > +{

> > +       struct aspeed_peci *priv;

> > +       int ret;

> > +

> > +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

> 

> ..."uh oh" from above confirmed. devm allocation lifetime and 'struct

> device' lifetime are not compatible.

> 

> You can trigger use after free bugs by turning on

> CONFIG_DEBUG_KOBJECT_RELEASE.

> 

> devm can be used to automatically unregister the peci_controller

> device. The flow would be something like:

> 

> priv = devm_kzalloc(..., sizeof(*priv), ...);

> controller = peci_controller_alloc(...);

> if (IS_ERR(controller))

>     return PTR_ERR(controller);

> rc = devm_peci_controller_add(...)

> if (rc)

>     return rc;

> 

> This arranges for the peci_controller_alloc() to be undone by

> put_device() in all cases. Internal to peci_controller_alloc() is

> typical goto unwind allocation error handling.


Yeah, it will be taken into account during refactoring that you
suggested in the previous patch review.

> 

> > +       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-irq", priv);

> > +       if (ret)

> > +               return ret;

> > +

> > +       init_completion(&priv->xfer_complete);

> > +       spin_lock_init(&priv->lock);

> > +

> > +       priv->controller.xfer = aspeed_peci_xfer;

> > +

> > +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);

> > +       if (IS_ERR(priv->rst)) {

> > +               dev_err(&pdev->dev, "Missing or invalid reset controller entry\n");

> > +               return PTR_ERR(priv->rst);

> > +       }

> > +       reset_control_deassert(priv->rst);

> > +

> > +       ret = aspeed_peci_init_ctrl(priv);

> > +       if (ret)

> > +               return ret;

> > +

> > +       return peci_controller_add(&priv->controller, priv->dev);

> > +}

> > +

> > +static int aspeed_peci_remove(struct platform_device *pdev)

> > +{

> > +       struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);

> > +

> > +       peci_controller_remove(&priv->controller);

> > +       reset_control_assert(priv->rst);

> > +

> 

> It's odd to have devm in the probe path and still publish a remove

> handler, i.e. why not handle controller removal and reset via devm?

> The example above with devm_peci_controller_add() already assumes

> peci_controller_remove is triggered by devm, reset assert can be

> managed the same way.


I think the pattern is pretty common for subsystems that don't have
devres support. Drivers use devres for resources that support it, and
go with regular "manual" remove for everything else.

But I'm fine with using devres in PECI.

> 

> > +       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,

> > +       .remove = aspeed_peci_remove,

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

> 

> Same comments about MODULE_AUTHOR from patch 6, i.e. make sure this is

> not duplicating what MAINTAINERS and git log handle.

> 

> I'll pause here until you've had a chance to consider fixes to the

> devm vs 'struct device' lifetime issue.


Sure, thank you
-Iwona

> 

> > +MODULE_DESCRIPTION("ASPEED PECI driver");

> > +MODULE_LICENSE("GPL");

> > +MODULE_IMPORT_NS(PECI);

>

On Mon, Jul 12, 2021 at 05:04:40PM CDT, Iwona Winiarska 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).

>

>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       |  12 +

> drivers/peci/controller/Makefile      |   3 +

> drivers/peci/controller/peci-aspeed.c | 501 ++++++++++++++++++++++++++

> 6 files changed, 534 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 47411e2b6336..4ba874afa2fa 100644

>--- a/MAINTAINERS

>+++ b/MAINTAINERS

>@@ -2865,6 +2865,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 601cc3c3c852..0d0ee8009713 100644

>--- a/drivers/peci/Kconfig

>+++ b/drivers/peci/Kconfig

>@@ -12,3 +12,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 2bb2f51bcda7..621a993e306a 100644

>--- a/drivers/peci/Makefile

>+++ b/drivers/peci/Makefile

>@@ -3,3 +3,6 @@

> # Core functionality

> peci-y := core.o sysfs.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..8ddbe494677f

>--- /dev/null

>+++ b/drivers/peci/controller/Kconfig

>@@ -0,0 +1,12 @@

>+# 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

>+	  Enable this driver if you want to support ASPEED PECI controller.

>+

>+	  This driver can be also build 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..888b46383ea4

>--- /dev/null

>+++ b/drivers/peci/controller/peci-aspeed.c

>@@ -0,0 +1,501 @@

>+// 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_READ_MODE_MASK	GENMASK(13, 12)

>+#define   ASPEED_PECI_CTRL_READ_MODE_COUNT	BIT(12)

>+#define   ASPEED_PECI_CTRL_READ_MODE_DBG	BIT(13)


Nitpick: might be nice to keep things in a consistent descending order
here (13 then 12).

>+#define   ASPEED_PECI_CTRL_CLK_SOURCE_MASK	BIT(11)


_MASK suffix seems out of place on this one.

>+#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_CONTENT_EN	BIT(5)


It *is* already kind of a long macro name, but abbreviating "contention"
to "content" seems a bit confusing; I'd suggest keeping the extra three
characters (or maybe drop the _EN suffix if you want to avoid making it
even longer).

>+#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_TIMING_MESSAGE_MASK	GENMASK(15, 8)

>+#define   ASPEED_PECI_TIMING_ADDRESS_MASK	GENMASK(7, 0)

>+

>+/* Command Register */

>+#define ASPEED_PECI_CMD				0x08

>+#define   ASPEED_PECI_CMD_PIN_MON		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_MON)

>+#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_READ_LEN_MASK		GENMASK(23, 16)

>+#define   ASPEED_PECI_WRITE_LEN_MASK		GENMASK(15, 8)

>+#define   ASPEED_PECI_TAGET_ADDR_MASK		GENMASK(7, 0)


s/TAGET/TARGET/

>+

>+/* Expected FCS Data Register */

>+#define ASPEED_PECI_EXP_FCS			0x10

>+#define   ASPEED_PECI_EXP_READ_FCS_MASK		GENMASK(23, 16)

>+#define   ASPEED_PECI_EXP_AW_FCS_AUTO_MASK	GENMASK(15, 8)

>+#define   ASPEED_PECI_EXP_WRITE_FCS_MASK	GENMASK(7, 0)

>+

>+/* Captured FCS Data Register */

>+#define ASPEED_PECI_CAP_FCS			0x14

>+#define   ASPEED_PECI_CAP_READ_FCS_MASK		GENMASK(23, 16)

>+#define   ASPEED_PECI_CAP_WRITE_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_CONNECT		BIT(3)


s/CONNECT/CONTENTION/

>+#define   ASPEED_PECI_INT_W_FCS_BAD		BIT(2)

>+#define   ASPEED_PECI_INT_W_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_W_DATA0			0x20

>+#define ASPEED_PECI_W_DATA1			0x24

>+#define ASPEED_PECI_W_DATA2			0x28

>+#define ASPEED_PECI_W_DATA3			0x2c

>+#define ASPEED_PECI_R_DATA0			0x30

>+#define ASPEED_PECI_R_DATA1			0x34

>+#define ASPEED_PECI_R_DATA2			0x38

>+#define ASPEED_PECI_R_DATA3			0x3c

>+#define ASPEED_PECI_W_DATA4			0x40

>+#define ASPEED_PECI_W_DATA5			0x44

>+#define ASPEED_PECI_W_DATA6			0x48

>+#define ASPEED_PECI_W_DATA7			0x4c

>+#define ASPEED_PECI_R_DATA4			0x50

>+#define ASPEED_PECI_R_DATA5			0x54

>+#define ASPEED_PECI_R_DATA6			0x58

>+#define ASPEED_PECI_R_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 inline struct aspeed_peci *to_aspeed_peci(struct peci_controller *a)

>+{

>+	return container_of(a, struct aspeed_peci, controller);

>+}

>+

>+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_TIMING_MESSAGE_MASK, priv->msg_timing);

>+	val |= FIELD_PREP(ASPEED_PECI_TIMING_ADDRESS_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;


This should be & instead of |, I'm guessing?

>+	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 = to_aspeed_peci(controller);

>+	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_TAGET_ADDR_MASK, addr) |

>+		    FIELD_PREP(ASPEED_PECI_WRITE_LEN_MASK, req->tx.len) |

>+		    FIELD_PREP(ASPEED_PECI_READ_LEN_MASK, req->rx.len);

>+

>+	writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);

>+

>+	memcpy_toio(priv->base + ASPEED_PECI_W_DATA0, req->tx.buf,

>+		    req->tx.len > 16 ? 16 : req->tx.len);


min(req->tx.len, 16) for the third argument there might be a bit
clearer.

>+	if (req->tx.len > 16)

>+		memcpy_toio(priv->base + ASPEED_PECI_W_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_R_DATA0,

>+		      req->rx.len > 16 ? 16 : req->rx.len);


Likewise, min(req->rx.len, 16) here.

>+	if (req->rx.len > 16)

>+		memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_R_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_CONNECT)

>+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONNECT\n");


s/CONNECT/CONTENTION/ here too (in the message string).

>+

>+	if (status & ASPEED_PECI_INT_W_FCS_BAD)

>+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_BAD\n");

>+

>+	if (status & ASPEED_PECI_INT_W_FCS_ABORT)

>+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_ABORT\n");


Bus contention can of course arise legitimately, and I suppose an
offline host CPU might result in a timeout, so dbg seems fine for those
(though as Dan suggests, making some counters available seems like a
good idea, especially for contention).  Are the FCS error cases
significant enough to warrant something less likely to go unnoticed
though?  (e.g. dev_warn_ratelimited() or something?)

>+

>+	/*

>+	 * 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 __sanitize_clock_divider(struct aspeed_peci *priv)

>+{

>+	u32 clk_div;

>+	int ret;

>+

>+	ret = device_property_read_u32(priv->dev, "clock-divider", &clk_div);

>+	if (ret) {

>+		clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;

>+	} else if (clk_div > ASPEED_PECI_CLK_DIV_MAX) {

>+		dev_warn(priv->dev, "Invalid clock-divider: %u, Using default: %u\n",

>+			 clk_div, ASPEED_PECI_CLK_DIV_DEFAULT);

>+

>+		clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;

>+	}

>+

>+	priv->clk_div = clk_div;

>+}

>+


The naming of these __sanitize_*() functions is a bit inconsistent with
the rest of the driver -- though given how similar they all look, could
they instead be refactored into a single helper function taking
property-name, default-value, and max-value parameters?

>+static void __sanitize_msg_timing(struct aspeed_peci *priv)

>+{

>+	u32 msg_timing;

>+	int ret;

>+

>+	ret = device_property_read_u32(priv->dev, "msg-timing", &msg_timing);

>+	if (ret) {

>+		msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;

>+	} else if (msg_timing > ASPEED_PECI_MSG_TIMING_MAX) {

>+		dev_warn(priv->dev, "Invalid msg-timing : %u, Use default : %u\n",

>+			 msg_timing, ASPEED_PECI_MSG_TIMING_DEFAULT);

>+

>+		msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;

>+	}

>+

>+	priv->msg_timing = msg_timing;

>+}

>+

>+static void __sanitize_addr_timing(struct aspeed_peci *priv)

>+{

>+	u32 addr_timing;

>+	int ret;

>+

>+	ret = device_property_read_u32(priv->dev, "addr-timing", &addr_timing);

>+	if (ret) {

>+		addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;

>+	} else if (addr_timing > ASPEED_PECI_ADDR_TIMING_MAX) {

>+		dev_warn(priv->dev, "Invalid addr-timing : %u, Use default : %u\n",

>+			 addr_timing, ASPEED_PECI_ADDR_TIMING_DEFAULT);

>+

>+		addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;

>+	}

>+

>+	priv->addr_timing = addr_timing;

>+}

>+

>+static void __sanitize_rd_sampling_point(struct aspeed_peci *priv)

>+{

>+	u32 rd_sampling_point;

>+	int ret;

>+

>+	ret = device_property_read_u32(priv->dev, "rd-sampling-point", &rd_sampling_point);

>+	if (ret) {

>+		rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;

>+	} else if (rd_sampling_point > ASPEED_PECI_RD_SAMPLING_POINT_MAX) {

>+		dev_warn(priv->dev, "Invalid rd-sampling-point: %u, Use default : %u\n",

>+			 rd_sampling_point, ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);

>+

>+		rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;

>+	}

>+

>+	priv->rd_sampling_point = rd_sampling_point;

>+}

>+

>+static void __sanitize_cmd_timeout(struct aspeed_peci *priv)

>+{

>+	u32 timeout;

>+	int ret;

>+

>+	ret = device_property_read_u32(priv->dev, "cmd-timeout-ms", &timeout);

>+	if (ret) {

>+		timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;

>+	} else if (timeout > ASPEED_PECI_CMD_TIMEOUT_MS_MAX || timeout == 0) {

>+		dev_warn(priv->dev, "Invalid cmd-timeout-ms: %u, Use default: %u\n",

>+			 timeout, ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT);

>+

>+		timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;

>+	}

>+

>+	priv->cmd_timeout_ms = timeout;

>+}

>+

>+static void aspeed_peci_device_property_sanitize(struct aspeed_peci *priv)

>+{

>+	__sanitize_clock_divider(priv);

>+	__sanitize_msg_timing(priv);

>+	__sanitize_addr_timing(priv);

>+	__sanitize_rd_sampling_point(priv);

>+	__sanitize_cmd_timeout(priv);

>+}

>+

>+static void aspeed_peci_disable_clk(void *data)

>+{

>+	clk_disable_unprepare(data);

>+}

>+

>+static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)

>+{

>+	int ret;

>+

>+	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 source\n");

>+

>+	ret = clk_prepare_enable(priv->clk);

>+	if (ret) {

>+		dev_err(priv->dev, "Failed to enable clock\n");

>+		return ret;

>+	}

>+

>+	ret = devm_add_action_or_reset(priv->dev, aspeed_peci_disable_clk, priv->clk);

>+	if (ret)

>+		return ret;

>+

>+	aspeed_peci_device_property_sanitize(priv);

>+

>+	aspeed_peci_init_regs(priv);

>+

>+	return 0;

>+}

>+

>+static int aspeed_peci_probe(struct platform_device *pdev)

>+{

>+	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-irq", priv);


Might as well drop the "-irq" suffix here?  (Seems a bit redundant, and
a quick glance through /proc/interrupts on the systems I have at hand
doesn't show anything else following that convention.)

>+	if (ret)

>+		return ret;

>+

>+	init_completion(&priv->xfer_complete);

>+	spin_lock_init(&priv->lock);

>+

>+	priv->controller.xfer = aspeed_peci_xfer;

>+

>+	priv->rst = devm_reset_control_get(&pdev->dev, NULL);

>+	if (IS_ERR(priv->rst)) {

>+		dev_err(&pdev->dev, "Missing or invalid reset controller entry\n");

>+		return PTR_ERR(priv->rst);

>+	}

>+	reset_control_deassert(priv->rst);

>+

>+	ret = aspeed_peci_init_ctrl(priv);

>+	if (ret)

>+		return ret;

>+

>+	return peci_controller_add(&priv->controller, priv->dev);

>+}

>+

>+static int aspeed_peci_remove(struct platform_device *pdev)

>+{

>+	struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);

>+

>+	peci_controller_remove(&priv->controller);

>+	reset_control_assert(priv->rst);

>+

>+	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,

>+	.remove = aspeed_peci_remove,

>+	.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

>

On Tue, 2021-07-27 at 08:49 +0000, Zev Weiss wrote:
> On Mon, Jul 12, 2021 at 05:04:40PM CDT, Iwona Winiarska 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).
> > 
> > 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       |  12 +
> > drivers/peci/controller/Makefile      |   3 +
> > drivers/peci/controller/peci-aspeed.c | 501 ++++++++++++++++++++++++++
> > 6 files changed, 534 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 47411e2b6336..4ba874afa2fa 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2865,6 +2865,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 601cc3c3c852..0d0ee8009713 100644
> > --- a/drivers/peci/Kconfig
> > +++ b/drivers/peci/Kconfig
> > @@ -12,3 +12,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 2bb2f51bcda7..621a993e306a 100644
> > --- a/drivers/peci/Makefile
> > +++ b/drivers/peci/Makefile
> > @@ -3,3 +3,6 @@
> > # Core functionality
> > peci-y := core.o sysfs.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..8ddbe494677f
> > --- /dev/null
> > +++ b/drivers/peci/controller/Kconfig
> > @@ -0,0 +1,12 @@
> > +# 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
> > +         Enable this driver if you want to support ASPEED PECI controller.
> > +
> > +         This driver can be also build 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..888b46383ea4
> > --- /dev/null
> > +++ b/drivers/peci/controller/peci-aspeed.c
> > @@ -0,0 +1,501 @@
> > +// 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_READ_MODE_MASK      GENMASK(13, 12)
> > +#define   ASPEED_PECI_CTRL_READ_MODE_COUNT     BIT(12)
> > +#define   ASPEED_PECI_CTRL_READ_MODE_DBG       BIT(13)
> 
> Nitpick: might be nice to keep things in a consistent descending order
> here (13 then 12).
> 

Sure, I'll change it in v2.

> > +#define   ASPEED_PECI_CTRL_CLK_SOURCE_MASK     BIT(11)
> 
> _MASK suffix seems out of place on this one.

Ack.

> 
> > +#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_CONTENT_EN      BIT(5)
> 
> It *is* already kind of a long macro name, but abbreviating "contention"
> to "content" seems a bit confusing; I'd suggest keeping the extra three
> characters (or maybe drop the _EN suffix if you want to avoid making it
> even longer).
> 

You're right - it'll be renamed properly in v2.

> > +#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_TIMING_MESSAGE_MASK      GENMASK(15, 8)
> > +#define   ASPEED_PECI_TIMING_ADDRESS_MASK      GENMASK(7, 0)
> > +
> > +/* Command Register */
> > +#define ASPEED_PECI_CMD                                0x08
> > +#define   ASPEED_PECI_CMD_PIN_MON              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_MON)
> > +#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_READ_LEN_MASK            GENMASK(23, 16)
> > +#define   ASPEED_PECI_WRITE_LEN_MASK           GENMASK(15, 8)
> > +#define   ASPEED_PECI_TAGET_ADDR_MASK          GENMASK(7, 0)
> 
> s/TAGET/TARGET/
> 

Ack.

> > +
> > +/* Expected FCS Data Register */
> > +#define ASPEED_PECI_EXP_FCS                    0x10
> > +#define   ASPEED_PECI_EXP_READ_FCS_MASK                GENMASK(23, 16)
> > +#define   ASPEED_PECI_EXP_AW_FCS_AUTO_MASK     GENMASK(15, 8)
> > +#define   ASPEED_PECI_EXP_WRITE_FCS_MASK       GENMASK(7, 0)
> > +
> > +/* Captured FCS Data Register */
> > +#define ASPEED_PECI_CAP_FCS                    0x14
> > +#define   ASPEED_PECI_CAP_READ_FCS_MASK                GENMASK(23, 16)
> > +#define   ASPEED_PECI_CAP_WRITE_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_CONNECT          BIT(3)
> 
> s/CONNECT/CONTENTION/

Ack.

> 
> > +#define   ASPEED_PECI_INT_W_FCS_BAD            BIT(2)
> > +#define   ASPEED_PECI_INT_W_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_W_DATA0                    0x20
> > +#define ASPEED_PECI_W_DATA1                    0x24
> > +#define ASPEED_PECI_W_DATA2                    0x28
> > +#define ASPEED_PECI_W_DATA3                    0x2c
> > +#define ASPEED_PECI_R_DATA0                    0x30
> > +#define ASPEED_PECI_R_DATA1                    0x34
> > +#define ASPEED_PECI_R_DATA2                    0x38
> > +#define ASPEED_PECI_R_DATA3                    0x3c
> > +#define ASPEED_PECI_W_DATA4                    0x40
> > +#define ASPEED_PECI_W_DATA5                    0x44
> > +#define ASPEED_PECI_W_DATA6                    0x48
> > +#define ASPEED_PECI_W_DATA7                    0x4c
> > +#define ASPEED_PECI_R_DATA4                    0x50
> > +#define ASPEED_PECI_R_DATA5                    0x54
> > +#define ASPEED_PECI_R_DATA6                    0x58
> > +#define ASPEED_PECI_R_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 inline struct aspeed_peci *to_aspeed_peci(struct peci_controller *a)
> > +{
> > +       return container_of(a, struct aspeed_peci, controller);
> > +}
> > +
> > +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_TIMING_MESSAGE_MASK, priv->msg_timing);
> > +       val |= FIELD_PREP(ASPEED_PECI_TIMING_ADDRESS_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;
> 
> This should be & instead of |, I'm guessing?
> 

I believe the idea is to unconditionally clear all known interrupt status bits,
(irrelevant of what value is already set in regs), and the HW expects that this
is done by writing 1 to corresponding bits.

> > +       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 = to_aspeed_peci(controller);
> > +       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_TAGET_ADDR_MASK, addr) |
> > +                   FIELD_PREP(ASPEED_PECI_WRITE_LEN_MASK, req->tx.len) |
> > +                   FIELD_PREP(ASPEED_PECI_READ_LEN_MASK, req->rx.len);
> > +
> > +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
> > +
> > +       memcpy_toio(priv->base + ASPEED_PECI_W_DATA0, req->tx.buf,
> > +                   req->tx.len > 16 ? 16 : req->tx.len);
> 
> min(req->tx.len, 16) for the third argument there might be a bit
> clearer.

Ack.

> 
> > +       if (req->tx.len > 16)
> > +               memcpy_toio(priv->base + ASPEED_PECI_W_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_R_DATA0,
> > +                     req->rx.len > 16 ? 16 : req->rx.len);
> 
> Likewise, min(req->rx.len, 16) here.

Ack.

> 
> > +       if (req->rx.len > 16)
> > +               memcpy_fromio(req->rx.buf + 16, priv->base +
> > ASPEED_PECI_R_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_CONNECT)
> > +               dev_dbg_ratelimited(priv->dev,
> > "ASPEED_PECI_INT_BUS_CONNECT\n");
> 
> s/CONNECT/CONTENTION/ here too (in the message string).

Ack.

> 
> > +
> > +       if (status & ASPEED_PECI_INT_W_FCS_BAD)
> > +               dev_dbg_ratelimited(priv->dev,
> > "ASPEED_PECI_INT_W_FCS_BAD\n");
> > +
> > +       if (status & ASPEED_PECI_INT_W_FCS_ABORT)
> > +               dev_dbg_ratelimited(priv->dev,
> > "ASPEED_PECI_INT_W_FCS_ABORT\n");
> 
> Bus contention can of course arise legitimately, and I suppose an
> offline host CPU might result in a timeout, so dbg seems fine for those
> (though as Dan suggests, making some counters available seems like a
> good idea, especially for contention).  Are the FCS error cases
> significant enough to warrant something less likely to go unnoticed
> though?  (e.g. dev_warn_ratelimited() or something?)

It's similar story for FCS errors (can occur legitimately).
We can hit ASPEED_PECI_INT_W_FCS_BAD in completely valid scenarios, e.g.
unsuccessful detect during rescan.
In case of ASPEED_PECI_INT_W_FCS_ABORT - caller can hit this by providing e.g.
malformed command. Since we do return -EIO in this case, caller can print its
own log. In other words, it's not always an error condition in peci-aspeed (or
HW). Moreover, if we ever expose more direct PECI access to userspace (pecidev,
or something similar) this warn would be user triggerable.

I would prefer to keep this at debug level for now.

> 
> > +
> > +       /*
> > +        * 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 __sanitize_clock_divider(struct aspeed_peci *priv)
> > +{
> > +       u32 clk_div;
> > +       int ret;
> > +
> > +       ret = device_property_read_u32(priv->dev, "clock-divider",
> > &clk_div);
> > +       if (ret) {
> > +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;
> > +       } else if (clk_div > ASPEED_PECI_CLK_DIV_MAX) {
> > +               dev_warn(priv->dev, "Invalid clock-divider: %u, Using
> > default: %u\n",
> > +                        clk_div, ASPEED_PECI_CLK_DIV_DEFAULT);
> > +
> > +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;
> > +       }
> > +
> > +       priv->clk_div = clk_div;
> > +}
> > +
> 
> The naming of these __sanitize_*() functions is a bit inconsistent with
> the rest of the driver -- though given how similar they all look, could
> they instead be refactored into a single helper function taking
> property-name, default-value, and max-value parameters?

You're right - we can have a single helper.

Regarding naming, the idea was to have a simple "inner" helper function to be
called by the more appropriately named aspeed_peci_device_property_sanitize().

Do you think I should use "aspeed_peci_" prefix in this function name or just
remove "__" and name it "sanitize_property()"?

> 
> > +static void __sanitize_msg_timing(struct aspeed_peci *priv)
> > +{
> > +       u32 msg_timing;
> > +       int ret;
> > +
> > +       ret = device_property_read_u32(priv->dev, "msg-timing",
> > &msg_timing);
> > +       if (ret) {
> > +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;
> > +       } else if (msg_timing > ASPEED_PECI_MSG_TIMING_MAX) {
> > +               dev_warn(priv->dev, "Invalid msg-timing : %u, Use default :
> > %u\n",
> > +                        msg_timing, ASPEED_PECI_MSG_TIMING_DEFAULT);
> > +
> > +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;
> > +       }
> > +
> > +       priv->msg_timing = msg_timing;
> > +}
> > +
> > +static void __sanitize_addr_timing(struct aspeed_peci *priv)
> > +{
> > +       u32 addr_timing;
> > +       int ret;
> > +
> > +       ret = device_property_read_u32(priv->dev, "addr-timing",
> > &addr_timing);
> > +       if (ret) {
> > +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;
> > +       } else if (addr_timing > ASPEED_PECI_ADDR_TIMING_MAX) {
> > +               dev_warn(priv->dev, "Invalid addr-timing : %u, Use default :
> > %u\n",
> > +                        addr_timing, ASPEED_PECI_ADDR_TIMING_DEFAULT);
> > +
> > +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;
> > +       }
> > +
> > +       priv->addr_timing = addr_timing;
> > +}
> > +
> > +static void __sanitize_rd_sampling_point(struct aspeed_peci *priv)
> > +{
> > +       u32 rd_sampling_point;
> > +       int ret;
> > +
> > +       ret = device_property_read_u32(priv->dev, "rd-sampling-point",
> > &rd_sampling_point);
> > +       if (ret) {
> > +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;
> > +       } else if (rd_sampling_point > ASPEED_PECI_RD_SAMPLING_POINT_MAX) {
> > +               dev_warn(priv->dev, "Invalid rd-sampling-point: %u, Use
> > default : %u\n",
> > +                        rd_sampling_point,
> > ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);
> > +
> > +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;
> > +       }
> > +
> > +       priv->rd_sampling_point = rd_sampling_point;
> > +}
> > +
> > +static void __sanitize_cmd_timeout(struct aspeed_peci *priv)
> > +{
> > +       u32 timeout;
> > +       int ret;
> > +
> > +       ret = device_property_read_u32(priv->dev, "cmd-timeout-ms",
> > &timeout);
> > +       if (ret) {
> > +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;
> > +       } else if (timeout > ASPEED_PECI_CMD_TIMEOUT_MS_MAX || timeout == 0)
> > {
> > +               dev_warn(priv->dev, "Invalid cmd-timeout-ms: %u, Use
> > default: %u\n",
> > +                        timeout, ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT);
> > +
> > +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;
> > +       }
> > +
> > +       priv->cmd_timeout_ms = timeout;
> > +}
> > +
> > +static void aspeed_peci_device_property_sanitize(struct aspeed_peci *priv)
> > +{
> > +       __sanitize_clock_divider(priv);
> > +       __sanitize_msg_timing(priv);
> > +       __sanitize_addr_timing(priv);
> > +       __sanitize_rd_sampling_point(priv);
> > +       __sanitize_cmd_timeout(priv);
> > +}
> > +
> > +static void aspeed_peci_disable_clk(void *data)
> > +{
> > +       clk_disable_unprepare(data);
> > +}
> > +
> > +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)
> > +{
> > +       int ret;
> > +
> > +       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 source\n");
> > +
> > +       ret = clk_prepare_enable(priv->clk);
> > +       if (ret) {
> > +               dev_err(priv->dev, "Failed to enable clock\n");
> > +               return ret;
> > +       }
> > +
> > +       ret = devm_add_action_or_reset(priv->dev, aspeed_peci_disable_clk,
> > priv->clk);
> > +       if (ret)
> > +               return ret;
> > +
> > +       aspeed_peci_device_property_sanitize(priv);
> > +
> > +       aspeed_peci_init_regs(priv);
> > +
> > +       return 0;
> > +}
> > +
> > +static int aspeed_peci_probe(struct platform_device *pdev)
> > +{
> > +       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-irq", priv);
> 
> Might as well drop the "-irq" suffix here?  (Seems a bit redundant, and
> a quick glance through /proc/interrupts on the systems I have at hand
> doesn't show anything else following that convention.)

I'll remove it.

Thank you
-Iwona

> 
> > +       if (ret)
> > +               return ret;
> > +
> > +       init_completion(&priv->xfer_complete);
> > +       spin_lock_init(&priv->lock);
> > +
> > +       priv->controller.xfer = aspeed_peci_xfer;
> > +
> > +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> > +       if (IS_ERR(priv->rst)) {
> > +               dev_err(&pdev->dev, "Missing or invalid reset controller
> > entry\n");
> > +               return PTR_ERR(priv->rst);
> > +       }
> > +       reset_control_deassert(priv->rst);
> > +
> > +       ret = aspeed_peci_init_ctrl(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return peci_controller_add(&priv->controller, priv->dev);
> > +}
> > +
> > +static int aspeed_peci_remove(struct platform_device *pdev)
> > +{
> > +       struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);
> > +
> > +       peci_controller_remove(&priv->controller);
> > +       reset_control_assert(priv->rst);
> > +
> > +       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,
> > +       .remove = aspeed_peci_remove,
> > +       .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

On Thu, Jul 29, 2021 at 09:03:28AM CDT, Winiarska, Iwona wrote:
>On Tue, 2021-07-27 at 08:49 +0000, Zev Weiss wrote:

>> On Mon, Jul 12, 2021 at 05:04:40PM CDT, Iwona Winiarska 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).

>> >

>> > 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       |  12 +

>> > drivers/peci/controller/Makefile      |   3 +

>> > drivers/peci/controller/peci-aspeed.c | 501 ++++++++++++++++++++++++++

>> > 6 files changed, 534 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 47411e2b6336..4ba874afa2fa 100644

>> > --- a/MAINTAINERS

>> > +++ b/MAINTAINERS

>> > @@ -2865,6 +2865,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 601cc3c3c852..0d0ee8009713 100644

>> > --- a/drivers/peci/Kconfig

>> > +++ b/drivers/peci/Kconfig

>> > @@ -12,3 +12,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 2bb2f51bcda7..621a993e306a 100644

>> > --- a/drivers/peci/Makefile

>> > +++ b/drivers/peci/Makefile

>> > @@ -3,3 +3,6 @@

>> > # Core functionality

>> > peci-y := core.o sysfs.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..8ddbe494677f

>> > --- /dev/null

>> > +++ b/drivers/peci/controller/Kconfig

>> > @@ -0,0 +1,12 @@

>> > +# 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

>> > +         Enable this driver if you want to support ASPEED PECI controller.

>> > +

>> > +         This driver can be also build 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..888b46383ea4

>> > --- /dev/null

>> > +++ b/drivers/peci/controller/peci-aspeed.c

>> > @@ -0,0 +1,501 @@

>> > +// 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_READ_MODE_MASK      GENMASK(13, 12)

>> > +#define   ASPEED_PECI_CTRL_READ_MODE_COUNT     BIT(12)

>> > +#define   ASPEED_PECI_CTRL_READ_MODE_DBG       BIT(13)

>>

>> Nitpick: might be nice to keep things in a consistent descending order

>> here (13 then 12).

>>

>

>Sure, I'll change it in v2.

>

>> > +#define   ASPEED_PECI_CTRL_CLK_SOURCE_MASK     BIT(11)

>>

>> _MASK suffix seems out of place on this one.

>

>Ack.

>

>>

>> > +#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_CONTENT_EN      BIT(5)

>>

>> It *is* already kind of a long macro name, but abbreviating "contention"

>> to "content" seems a bit confusing; I'd suggest keeping the extra three

>> characters (or maybe drop the _EN suffix if you want to avoid making it

>> even longer).

>>

>

>You're right - it'll be renamed properly in v2.

>

>> > +#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_TIMING_MESSAGE_MASK      GENMASK(15, 8)

>> > +#define   ASPEED_PECI_TIMING_ADDRESS_MASK      GENMASK(7, 0)

>> > +

>> > +/* Command Register */

>> > +#define ASPEED_PECI_CMD                                0x08

>> > +#define   ASPEED_PECI_CMD_PIN_MON              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_MON)

>> > +#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_READ_LEN_MASK            GENMASK(23, 16)

>> > +#define   ASPEED_PECI_WRITE_LEN_MASK           GENMASK(15, 8)

>> > +#define   ASPEED_PECI_TAGET_ADDR_MASK          GENMASK(7, 0)

>>

>> s/TAGET/TARGET/

>>

>

>Ack.

>

>> > +

>> > +/* Expected FCS Data Register */

>> > +#define ASPEED_PECI_EXP_FCS                    0x10

>> > +#define   ASPEED_PECI_EXP_READ_FCS_MASK                GENMASK(23, 16)

>> > +#define   ASPEED_PECI_EXP_AW_FCS_AUTO_MASK     GENMASK(15, 8)

>> > +#define   ASPEED_PECI_EXP_WRITE_FCS_MASK       GENMASK(7, 0)

>> > +

>> > +/* Captured FCS Data Register */

>> > +#define ASPEED_PECI_CAP_FCS                    0x14

>> > +#define   ASPEED_PECI_CAP_READ_FCS_MASK                GENMASK(23, 16)

>> > +#define   ASPEED_PECI_CAP_WRITE_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_CONNECT          BIT(3)

>>

>> s/CONNECT/CONTENTION/

>

>Ack.

>

>>

>> > +#define   ASPEED_PECI_INT_W_FCS_BAD            BIT(2)

>> > +#define   ASPEED_PECI_INT_W_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_W_DATA0                    0x20

>> > +#define ASPEED_PECI_W_DATA1                    0x24

>> > +#define ASPEED_PECI_W_DATA2                    0x28

>> > +#define ASPEED_PECI_W_DATA3                    0x2c

>> > +#define ASPEED_PECI_R_DATA0                    0x30

>> > +#define ASPEED_PECI_R_DATA1                    0x34

>> > +#define ASPEED_PECI_R_DATA2                    0x38

>> > +#define ASPEED_PECI_R_DATA3                    0x3c

>> > +#define ASPEED_PECI_W_DATA4                    0x40

>> > +#define ASPEED_PECI_W_DATA5                    0x44

>> > +#define ASPEED_PECI_W_DATA6                    0x48

>> > +#define ASPEED_PECI_W_DATA7                    0x4c

>> > +#define ASPEED_PECI_R_DATA4                    0x50

>> > +#define ASPEED_PECI_R_DATA5                    0x54

>> > +#define ASPEED_PECI_R_DATA6                    0x58

>> > +#define ASPEED_PECI_R_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 inline struct aspeed_peci *to_aspeed_peci(struct peci_controller *a)

>> > +{

>> > +       return container_of(a, struct aspeed_peci, controller);

>> > +}

>> > +

>> > +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_TIMING_MESSAGE_MASK, priv->msg_timing);

>> > +       val |= FIELD_PREP(ASPEED_PECI_TIMING_ADDRESS_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;

>>

>> This should be & instead of |, I'm guessing?

>>

>

>I believe the idea is to unconditionally clear all known interrupt status bits,

>(irrelevant of what value is already set in regs), and the HW expects that this

>is done by writing 1 to corresponding bits.

>


Ah -- I had been thinking we needed to ensure that we were only writing
zeros to reserved or RO bits, but I suppose re-writing whatever bit
pattern they provide on a read is probably okay (I'm having trouble
finding any explicit statement either way in the datasheet I've got).

>> > +       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 = to_aspeed_peci(controller);

>> > +       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_TAGET_ADDR_MASK, addr) |

>> > +                   FIELD_PREP(ASPEED_PECI_WRITE_LEN_MASK, req->tx.len) |

>> > +                   FIELD_PREP(ASPEED_PECI_READ_LEN_MASK, req->rx.len);

>> > +

>> > +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);

>> > +

>> > +       memcpy_toio(priv->base + ASPEED_PECI_W_DATA0, req->tx.buf,

>> > +                   req->tx.len > 16 ? 16 : req->tx.len);

>>

>> min(req->tx.len, 16) for the third argument there might be a bit

>> clearer.

>

>Ack.

>

>>

>> > +       if (req->tx.len > 16)

>> > +               memcpy_toio(priv->base + ASPEED_PECI_W_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_R_DATA0,

>> > +                     req->rx.len > 16 ? 16 : req->rx.len);

>>

>> Likewise, min(req->rx.len, 16) here.

>

>Ack.

>

>>

>> > +       if (req->rx.len > 16)

>> > +               memcpy_fromio(req->rx.buf + 16, priv->base +

>> > ASPEED_PECI_R_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_CONNECT)

>> > +               dev_dbg_ratelimited(priv->dev,

>> > "ASPEED_PECI_INT_BUS_CONNECT\n");

>>

>> s/CONNECT/CONTENTION/ here too (in the message string).

>

>Ack.

>

>>

>> > +

>> > +       if (status & ASPEED_PECI_INT_W_FCS_BAD)

>> > +               dev_dbg_ratelimited(priv->dev,

>> > "ASPEED_PECI_INT_W_FCS_BAD\n");

>> > +

>> > +       if (status & ASPEED_PECI_INT_W_FCS_ABORT)

>> > +               dev_dbg_ratelimited(priv->dev,

>> > "ASPEED_PECI_INT_W_FCS_ABORT\n");

>>

>> Bus contention can of course arise legitimately, and I suppose an

>> offline host CPU might result in a timeout, so dbg seems fine for those

>> (though as Dan suggests, making some counters available seems like a

>> good idea, especially for contention).  Are the FCS error cases

>> significant enough to warrant something less likely to go unnoticed

>> though?  (e.g. dev_warn_ratelimited() or something?)

>

>It's similar story for FCS errors (can occur legitimately).

>We can hit ASPEED_PECI_INT_W_FCS_BAD in completely valid scenarios, e.g.

>unsuccessful detect during rescan.

>In case of ASPEED_PECI_INT_W_FCS_ABORT - caller can hit this by providing e.g.

>malformed command. Since we do return -EIO in this case, caller can print its

>own log. In other words, it's not always an error condition in peci-aspeed (or

>HW). Moreover, if we ever expose more direct PECI access to userspace (pecidev,

>or something similar) this warn would be user triggerable.

>

>I would prefer to keep this at debug level for now.

>


Okay, I guess that's alright -- counters of some sort (e.g. in sysfs)
would be a nice thing to supplement that with for diagnosing problems
though, I think.

>>

>> > +

>> > +       /*

>> > +        * 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 __sanitize_clock_divider(struct aspeed_peci *priv)

>> > +{

>> > +       u32 clk_div;

>> > +       int ret;

>> > +

>> > +       ret = device_property_read_u32(priv->dev, "clock-divider",

>> > &clk_div);

>> > +       if (ret) {

>> > +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;

>> > +       } else if (clk_div > ASPEED_PECI_CLK_DIV_MAX) {

>> > +               dev_warn(priv->dev, "Invalid clock-divider: %u, Using

>> > default: %u\n",

>> > +                        clk_div, ASPEED_PECI_CLK_DIV_DEFAULT);

>> > +

>> > +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;

>> > +       }

>> > +

>> > +       priv->clk_div = clk_div;

>> > +}

>> > +

>>

>> The naming of these __sanitize_*() functions is a bit inconsistent with

>> the rest of the driver -- though given how similar they all look, could

>> they instead be refactored into a single helper function taking

>> property-name, default-value, and max-value parameters?

>

>You're right - we can have a single helper.

>

>Regarding naming, the idea was to have a simple "inner" helper function to be

>called by the more appropriately named aspeed_peci_device_property_sanitize().

>

>Do you think I should use "aspeed_peci_" prefix in this function name or just

>remove "__" and name it "sanitize_property()"?

>


I just think it's generally nice to have function names give some
indication as to what part of the kernel they belong to -- that way when
they show up in a stack bracktrace or a symbol list (e.g. for ftrace
usage and such) it's clearer what they are (and reduces the likelihood
of name collisions and ensuing confusion).

>>

>> > +static void __sanitize_msg_timing(struct aspeed_peci *priv)

>> > +{

>> > +       u32 msg_timing;

>> > +       int ret;

>> > +

>> > +       ret = device_property_read_u32(priv->dev, "msg-timing",

>> > &msg_timing);

>> > +       if (ret) {

>> > +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;

>> > +       } else if (msg_timing > ASPEED_PECI_MSG_TIMING_MAX) {

>> > +               dev_warn(priv->dev, "Invalid msg-timing : %u, Use default :

>> > %u\n",

>> > +                        msg_timing, ASPEED_PECI_MSG_TIMING_DEFAULT);

>> > +

>> > +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;

>> > +       }

>> > +

>> > +       priv->msg_timing = msg_timing;

>> > +}

>> > +

>> > +static void __sanitize_addr_timing(struct aspeed_peci *priv)

>> > +{

>> > +       u32 addr_timing;

>> > +       int ret;

>> > +

>> > +       ret = device_property_read_u32(priv->dev, "addr-timing",

>> > &addr_timing);

>> > +       if (ret) {

>> > +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;

>> > +       } else if (addr_timing > ASPEED_PECI_ADDR_TIMING_MAX) {

>> > +               dev_warn(priv->dev, "Invalid addr-timing : %u, Use default :

>> > %u\n",

>> > +                        addr_timing, ASPEED_PECI_ADDR_TIMING_DEFAULT);

>> > +

>> > +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;

>> > +       }

>> > +

>> > +       priv->addr_timing = addr_timing;

>> > +}

>> > +

>> > +static void __sanitize_rd_sampling_point(struct aspeed_peci *priv)

>> > +{

>> > +       u32 rd_sampling_point;

>> > +       int ret;

>> > +

>> > +       ret = device_property_read_u32(priv->dev, "rd-sampling-point",

>> > &rd_sampling_point);

>> > +       if (ret) {

>> > +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;

>> > +       } else if (rd_sampling_point > ASPEED_PECI_RD_SAMPLING_POINT_MAX) {

>> > +               dev_warn(priv->dev, "Invalid rd-sampling-point: %u, Use

>> > default : %u\n",

>> > +                        rd_sampling_point,

>> > ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);

>> > +

>> > +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;

>> > +       }

>> > +

>> > +       priv->rd_sampling_point = rd_sampling_point;

>> > +}

>> > +

>> > +static void __sanitize_cmd_timeout(struct aspeed_peci *priv)

>> > +{

>> > +       u32 timeout;

>> > +       int ret;

>> > +

>> > +       ret = device_property_read_u32(priv->dev, "cmd-timeout-ms",

>> > &timeout);

>> > +       if (ret) {

>> > +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;

>> > +       } else if (timeout > ASPEED_PECI_CMD_TIMEOUT_MS_MAX || timeout == 0)

>> > {

>> > +               dev_warn(priv->dev, "Invalid cmd-timeout-ms: %u, Use

>> > default: %u\n",

>> > +                        timeout, ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT);

>> > +

>> > +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;

>> > +       }

>> > +

>> > +       priv->cmd_timeout_ms = timeout;

>> > +}

>> > +

>> > +static void aspeed_peci_device_property_sanitize(struct aspeed_peci *priv)

>> > +{

>> > +       __sanitize_clock_divider(priv);

>> > +       __sanitize_msg_timing(priv);

>> > +       __sanitize_addr_timing(priv);

>> > +       __sanitize_rd_sampling_point(priv);

>> > +       __sanitize_cmd_timeout(priv);

>> > +}

>> > +

>> > +static void aspeed_peci_disable_clk(void *data)

>> > +{

>> > +       clk_disable_unprepare(data);

>> > +}

>> > +

>> > +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)

>> > +{

>> > +       int ret;

>> > +

>> > +       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 source\n");

>> > +

>> > +       ret = clk_prepare_enable(priv->clk);

>> > +       if (ret) {

>> > +               dev_err(priv->dev, "Failed to enable clock\n");

>> > +               return ret;

>> > +       }

>> > +

>> > +       ret = devm_add_action_or_reset(priv->dev, aspeed_peci_disable_clk,

>> > priv->clk);

>> > +       if (ret)

>> > +               return ret;

>> > +

>> > +       aspeed_peci_device_property_sanitize(priv);

>> > +

>> > +       aspeed_peci_init_regs(priv);

>> > +

>> > +       return 0;

>> > +}

>> > +

>> > +static int aspeed_peci_probe(struct platform_device *pdev)

>> > +{

>> > +       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-irq", priv);

>>

>> Might as well drop the "-irq" suffix here?  (Seems a bit redundant, and

>> a quick glance through /proc/interrupts on the systems I have at hand

>> doesn't show anything else following that convention.)

>

>I'll remove it.

>

>Thank you

>-Iwona

>

>>

>> > +       if (ret)

>> > +               return ret;

>> > +

>> > +       init_completion(&priv->xfer_complete);

>> > +       spin_lock_init(&priv->lock);

>> > +

>> > +       priv->controller.xfer = aspeed_peci_xfer;

>> > +

>> > +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);

>> > +       if (IS_ERR(priv->rst)) {

>> > +               dev_err(&pdev->dev, "Missing or invalid reset controller

>> > entry\n");

>> > +               return PTR_ERR(priv->rst);

>> > +       }

>> > +       reset_control_deassert(priv->rst);

>> > +

>> > +       ret = aspeed_peci_init_ctrl(priv);

>> > +       if (ret)

>> > +               return ret;

>> > +

>> > +       return peci_controller_add(&priv->controller, priv->dev);

>> > +}

>> > +

>> > +static int aspeed_peci_remove(struct platform_device *pdev)

>> > +{

>> > +       struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);

>> > +

>> > +       peci_controller_remove(&priv->controller);

>> > +       reset_control_assert(priv->rst);

>> > +

>> > +       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,

>> > +       .remove = aspeed_peci_remove,

>> > +       .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

>