From: "Winiarska, Iwona" <iwona.winiarska@intel.com>
To: "zweiss@equinix.com" <zweiss@equinix.com>
Cc: "corbet@lwn.net" <corbet@lwn.net>,
"jae.hyun.yoo@linux.intel.com" <jae.hyun.yoo@linux.intel.com>,
"Lutomirski, Andy" <luto@kernel.org>,
"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"Luck, Tony" <tony.luck@intel.com>,
"andrew@aj.id.au" <andrew@aj.id.au>,
"mchehab@kernel.org" <mchehab@kernel.org>,
"jdelvare@suse.com" <jdelvare@suse.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"mingo@redhat.com" <mingo@redhat.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"linux@roeck-us.net" <linux@roeck-us.net>,
"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"yazen.ghannam@amd.com" <yazen.ghannam@amd.com>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"bp@alien8.de" <bp@alien8.de>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"pierre-louis.bossart@linux.intel.com"
<pierre-louis.bossart@linux.intel.com>,
"andriy.shevchenko@linux.intel.com"
<andriy.shevchenko@linux.intel.com>,
"x86@kernel.org" <x86@kernel.org>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 07/14] peci: Add peci-aspeed controller driver
Date: Thu, 29 Jul 2021 14:03:28 +0000 [thread overview]
Message-ID: <e6b7588abe48b00b2822ab4614ec0600f9e044f0.camel@intel.com> (raw)
In-Reply-To: <20210727084901.GQ8018@packtop>
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
WARNING: multiple messages have this Message-ID (diff)
From: "Winiarska, Iwona" <iwona.winiarska@intel.com>
To: "zweiss@equinix.com" <zweiss@equinix.com>
Cc: "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"jae.hyun.yoo@linux.intel.com" <jae.hyun.yoo@linux.intel.com>,
"mchehab@kernel.org" <mchehab@kernel.org>,
"corbet@lwn.net" <corbet@lwn.net>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"x86@kernel.org" <x86@kernel.org>,
"pierre-louis.bossart@linux.intel.com"
<pierre-louis.bossart@linux.intel.com>,
"mingo@redhat.com" <mingo@redhat.com>,
"linux@roeck-us.net" <linux@roeck-us.net>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"jdelvare@suse.com" <jdelvare@suse.com>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"bp@alien8.de" <bp@alien8.de>,
"Lutomirski, Andy" <luto@kernel.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"andriy.shevchenko@linux.intel.com"
<andriy.shevchenko@linux.intel.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"Luck, Tony" <tony.luck@intel.com>,
"andrew@aj.id.au" <andrew@aj.id.au>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"yazen.ghannam@amd.com" <yazen.ghannam@amd.com>
Subject: Re: [PATCH 07/14] peci: Add peci-aspeed controller driver
Date: Thu, 29 Jul 2021 14:03:28 +0000 [thread overview]
Message-ID: <e6b7588abe48b00b2822ab4614ec0600f9e044f0.camel@intel.com> (raw)
In-Reply-To: <20210727084901.GQ8018@packtop>
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
WARNING: multiple messages have this Message-ID (diff)
From: "Winiarska, Iwona" <iwona.winiarska@intel.com>
To: "zweiss@equinix.com" <zweiss@equinix.com>
Cc: "corbet@lwn.net" <corbet@lwn.net>,
"jae.hyun.yoo@linux.intel.com" <jae.hyun.yoo@linux.intel.com>,
"Lutomirski, Andy" <luto@kernel.org>,
"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"Luck, Tony" <tony.luck@intel.com>,
"andrew@aj.id.au" <andrew@aj.id.au>,
"mchehab@kernel.org" <mchehab@kernel.org>,
"jdelvare@suse.com" <jdelvare@suse.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"mingo@redhat.com" <mingo@redhat.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"linux@roeck-us.net" <linux@roeck-us.net>,
"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"yazen.ghannam@amd.com" <yazen.ghannam@amd.com>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"bp@alien8.de" <bp@alien8.de>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"pierre-louis.bossart@linux.intel.com"
<pierre-louis.bossart@linux.intel.com>,
"andriy.shevchenko@linux.intel.com"
<andriy.shevchenko@linux.intel.com>,
"x86@kernel.org" <x86@kernel.org>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 07/14] peci: Add peci-aspeed controller driver
Date: Thu, 29 Jul 2021 14:03:28 +0000 [thread overview]
Message-ID: <e6b7588abe48b00b2822ab4614ec0600f9e044f0.camel@intel.com> (raw)
In-Reply-To: <20210727084901.GQ8018@packtop>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-07-29 14:09 UTC|newest]
Thread overview: 215+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-12 22:04 [PATCH 00/14] Introduce PECI subsystem Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` [PATCH 01/14] x86/cpu: Move intel-family to arch-independent headers Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-14 16:54 ` Williams, Dan J
2021-07-14 16:54 ` Williams, Dan J
2021-07-14 16:54 ` Williams, Dan J
2021-07-15 16:47 ` Winiarska, Iwona
2021-07-15 16:47 ` Winiarska, Iwona
2021-07-15 16:47 ` Winiarska, Iwona
2021-07-15 18:13 ` Dan Williams
2021-07-15 18:13 ` Dan Williams
2021-07-15 18:13 ` Dan Williams
2021-07-15 18:29 ` Luck, Tony
2021-07-15 18:29 ` Luck, Tony
2021-07-15 18:29 ` Luck, Tony
2021-07-12 22:04 ` [PATCH 02/14] x86/cpu: Extract cpuid helpers to arch-independent Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-14 16:58 ` Williams, Dan J
2021-07-14 16:58 ` Williams, Dan J
2021-07-14 16:58 ` Williams, Dan J
2021-07-15 16:51 ` Winiarska, Iwona
2021-07-15 16:51 ` Winiarska, Iwona
2021-07-15 16:51 ` Winiarska, Iwona
2021-07-15 16:58 ` Winiarska, Iwona
2021-07-15 16:58 ` Winiarska, Iwona
2021-07-15 16:58 ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 03/14] dt-bindings: Add generic bindings for PECI Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` [PATCH 04/14] dt-bindings: Add bindings for peci-aspeed Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-15 16:28 ` Rob Herring
2021-07-15 16:28 ` Rob Herring
2021-07-15 16:28 ` Rob Herring
2021-07-16 21:22 ` Winiarska, Iwona
2021-07-16 21:22 ` Winiarska, Iwona
2021-07-16 21:22 ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 05/14] ARM: dts: aspeed: Add PECI controller nodes Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` [PATCH 06/14] peci: Add core infrastructure Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-14 17:19 ` Williams, Dan J
2021-07-14 17:19 ` Williams, Dan J
2021-07-14 17:19 ` Williams, Dan J
2021-07-16 21:08 ` Winiarska, Iwona
2021-07-16 21:08 ` Winiarska, Iwona
2021-07-16 21:08 ` Winiarska, Iwona
2021-07-16 21:50 ` Dan Williams
2021-07-16 21:50 ` Dan Williams
2021-07-16 21:50 ` Dan Williams
2021-07-17 6:12 ` gregkh
2021-07-17 6:12 ` gregkh
2021-07-17 6:12 ` gregkh
2021-07-17 20:54 ` Dan Williams
2021-07-17 20:54 ` Dan Williams
2021-07-17 20:54 ` Dan Williams
2021-07-12 22:04 ` [PATCH 07/14] peci: Add peci-aspeed controller driver Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-13 5:02 ` Randy Dunlap
2021-07-13 5:02 ` Randy Dunlap
2021-07-13 5:02 ` Randy Dunlap
2021-07-15 16:42 ` Winiarska, Iwona
2021-07-15 16:42 ` Winiarska, Iwona
2021-07-15 16:42 ` Winiarska, Iwona
2021-07-14 17:39 ` Williams, Dan J
2021-07-14 17:39 ` Williams, Dan J
2021-07-14 17:39 ` Williams, Dan J
2021-07-16 21:17 ` Winiarska, Iwona
2021-07-16 21:17 ` Winiarska, Iwona
2021-07-16 21:17 ` Winiarska, Iwona
2021-07-27 8:49 ` Zev Weiss
2021-07-27 8:49 ` Zev Weiss
2021-07-27 8:49 ` Zev Weiss
2021-07-29 14:03 ` Winiarska, Iwona [this message]
2021-07-29 14:03 ` Winiarska, Iwona
2021-07-29 14:03 ` Winiarska, Iwona
2021-07-29 18:15 ` Zev Weiss
2021-07-29 18:15 ` Zev Weiss
2021-07-29 18:15 ` Zev Weiss
2021-07-12 22:04 ` [PATCH 08/14] peci: Add device detection Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-14 21:05 ` Williams, Dan J
2021-07-14 21:05 ` Williams, Dan J
2021-07-14 21:05 ` Williams, Dan J
2021-07-16 21:20 ` Winiarska, Iwona
2021-07-16 21:20 ` Winiarska, Iwona
2021-07-16 21:20 ` Winiarska, Iwona
2021-07-27 17:49 ` Zev Weiss
2021-07-27 17:49 ` Zev Weiss
2021-07-27 17:49 ` Zev Weiss
2021-07-29 18:55 ` Winiarska, Iwona
2021-07-29 18:55 ` Winiarska, Iwona
2021-07-29 18:55 ` Winiarska, Iwona
2021-07-29 20:50 ` Zev Weiss
2021-07-29 20:50 ` Zev Weiss
2021-07-29 20:50 ` Zev Weiss
2021-07-30 20:10 ` Winiarska, Iwona
2021-07-30 20:10 ` Winiarska, Iwona
2021-07-30 20:10 ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 09/14] peci: Add support for PECI device drivers Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-27 20:10 ` Zev Weiss
2021-07-27 20:10 ` Zev Weiss
2021-07-27 20:10 ` Zev Weiss
2021-07-27 21:23 ` Guenter Roeck
2021-07-27 21:23 ` Guenter Roeck
2021-07-27 21:23 ` Guenter Roeck
2021-07-29 21:17 ` Winiarska, Iwona
2021-07-29 21:17 ` Winiarska, Iwona
2021-07-29 21:17 ` Winiarska, Iwona
2021-07-29 23:22 ` Zev Weiss
2021-07-29 23:22 ` Zev Weiss
2021-07-29 23:22 ` Zev Weiss
2021-07-30 20:13 ` Winiarska, Iwona
2021-07-30 20:13 ` Winiarska, Iwona
2021-07-30 20:13 ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 10/14] peci: Add peci-cpu driver Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-27 11:16 ` David Müller (ELSOFT AG)
2021-07-27 11:16 ` David Müller (ELSOFT AG)
2021-07-30 20:14 ` Winiarska, Iwona
2021-07-30 20:14 ` Winiarska, Iwona
2021-07-30 20:14 ` Winiarska, Iwona
2021-07-27 21:33 ` Zev Weiss
2021-07-27 21:33 ` Zev Weiss
2021-07-27 21:33 ` Zev Weiss
2021-07-30 21:21 ` Winiarska, Iwona
2021-07-30 21:21 ` Winiarska, Iwona
2021-07-30 21:21 ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 11/14] hwmon: peci: Add cputemp driver Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-15 17:45 ` Guenter Roeck
2021-07-15 17:45 ` Guenter Roeck
2021-07-15 17:45 ` Guenter Roeck
2021-07-19 20:12 ` Winiarska, Iwona
2021-07-19 20:12 ` Winiarska, Iwona
2021-07-19 20:12 ` Winiarska, Iwona
2021-07-19 20:35 ` Guenter Roeck
2021-07-19 20:35 ` Guenter Roeck
2021-07-19 20:35 ` Guenter Roeck
2021-07-27 7:06 ` Zev Weiss
2021-07-27 7:06 ` Zev Weiss
2021-07-27 7:06 ` Zev Weiss
2021-07-30 21:51 ` Winiarska, Iwona
2021-07-30 21:51 ` Winiarska, Iwona
2021-07-30 21:51 ` Winiarska, Iwona
2021-07-30 22:04 ` Guenter Roeck
2021-07-30 22:04 ` Guenter Roeck
2021-07-30 22:04 ` Guenter Roeck
2021-07-12 22:04 ` [PATCH 12/14] hwmon: peci: Add dimmtemp driver Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-15 17:56 ` Guenter Roeck
2021-07-15 17:56 ` Guenter Roeck
2021-07-15 17:56 ` Guenter Roeck
2021-07-19 20:31 ` Winiarska, Iwona
2021-07-19 20:31 ` Winiarska, Iwona
2021-07-19 20:31 ` Winiarska, Iwona
2021-07-19 20:36 ` Guenter Roeck
2021-07-19 20:36 ` Guenter Roeck
2021-07-19 20:36 ` Guenter Roeck
2021-07-26 22:08 ` Zev Weiss
2021-07-26 22:08 ` Zev Weiss
2021-07-26 22:08 ` Zev Weiss
2021-07-30 22:48 ` Winiarska, Iwona
2021-07-30 22:48 ` Winiarska, Iwona
2021-07-30 22:48 ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 13/14] docs: hwmon: Document PECI drivers Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-27 22:58 ` Zev Weiss
2021-07-27 22:58 ` Zev Weiss
2021-07-27 22:58 ` Zev Weiss
2021-07-28 0:49 ` Guenter Roeck
2021-07-28 0:49 ` Guenter Roeck
2021-07-28 0:49 ` Guenter Roeck
2021-08-02 11:39 ` Winiarska, Iwona
2021-08-02 11:39 ` Winiarska, Iwona
2021-08-02 11:39 ` Winiarska, Iwona
2021-08-02 11:37 ` Winiarska, Iwona
2021-08-02 11:37 ` Winiarska, Iwona
2021-08-02 11:37 ` Winiarska, Iwona
2021-08-04 17:52 ` Zev Weiss
2021-08-04 17:52 ` Zev Weiss
2021-08-04 17:52 ` Zev Weiss
2021-08-04 18:05 ` Guenter Roeck
2021-08-04 18:05 ` Guenter Roeck
2021-08-04 18:05 ` Guenter Roeck
2021-08-05 21:42 ` Winiarska, Iwona
2021-08-05 21:42 ` Winiarska, Iwona
2021-08-05 21:42 ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 14/14] docs: Add PECI documentation Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-12 22:04 ` Iwona Winiarska
2021-07-14 16:51 ` [PATCH 00/14] Introduce PECI subsystem Williams, Dan J
2021-07-14 16:51 ` Williams, Dan J
2021-07-14 16:51 ` Williams, Dan J
2021-07-15 17:33 ` Winiarska, Iwona
2021-07-15 17:33 ` Winiarska, Iwona
2021-07-15 17:33 ` Winiarska, Iwona
2021-07-15 19:34 ` Dan Williams
2021-07-15 19:34 ` Dan Williams
2021-07-15 19:34 ` Dan Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e6b7588abe48b00b2822ab4614ec0600f9e044f0.camel@intel.com \
--to=iwona.winiarska@intel.com \
--cc=andrew@aj.id.au \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bp@alien8.de \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jae.hyun.yoo@linux.intel.com \
--cc=jdelvare@suse.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=luto@kernel.org \
--cc=mchehab@kernel.org \
--cc=mingo@redhat.com \
--cc=openbmc@lists.ozlabs.org \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=robh+dt@kernel.org \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=x86@kernel.org \
--cc=yazen.ghannam@amd.com \
--cc=zweiss@equinix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.