openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: "Winiarska, Iwona" <iwona.winiarska@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>
Cc: "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"zweiss@equinix.com" <zweiss@equinix.com>,
	"jae.hyun.yoo@linux.intel.com" <jae.hyun.yoo@linux.intel.com>,
	"andriy.shevchenko@linux.intel.com"
	<andriy.shevchenko@linux.intel.com>,
	"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>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"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>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"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>,
	"rdunlap@infradead.org" <rdunlap@infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"yazen.ghannam@amd.com" <yazen.ghannam@amd.com>,
	"olof@lixom.net" <olof@lixom.net>
Subject: Re: [PATCH v2 07/15] peci: Add peci-aspeed controller driver
Date: Thu, 26 Aug 2021 23:54:56 +0000	[thread overview]
Message-ID: <b26ee278838698289869964fe59578f0d5f7b19c.camel@intel.com> (raw)
In-Reply-To: <CAPcyv4jPVSt9Wr2TkDActFVLP+ygaDwBnsKG410Nf1qfP_MB9A@mail.gmail.com>

On Wed, 2021-08-25 at 18:35 -0700, Dan Williams wrote:
> On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
> <iwona.winiarska@intel.com> wrote:
> > 
> > From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > 
> > ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical
> > interface (a.k.a PECI wire).
> 
> Maybe a one sentence blurb here and in the Kconfig reminding people
> why they should care if they have a PECI driver or not?

Ok, I'll expand it a bit.

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

I'll make them consistent.

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

Leftover - I'll remove it.

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

I can remove it or I can add something like:

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

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

What do you think?

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

You're right - I'll fix it.

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

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

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

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

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

Ok.

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

Ok.

Thanks
-Iwona

> 
> > +{
> > +       int ret;
> > +
> > +       ret = clk_prepare_enable(clk);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return devm_add_action_or_reset(dev, aspeed_peci_clk_release, clk);
> > +}
> > +
> > +static int aspeed_peci_probe(struct platform_device *pdev)
> > +{
> > +       struct peci_controller *controller;
> > +       struct aspeed_peci *priv;
> > +       int ret;
> > +
> > +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       priv->dev = &pdev->dev;
> > +       dev_set_drvdata(priv->dev, priv);
> > +
> > +       priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(priv->base))
> > +               return PTR_ERR(priv->base);
> > +
> > +       priv->irq = platform_get_irq(pdev, 0);
> > +       if (!priv->irq)
> > +               return priv->irq;
> > +
> > +       ret = devm_request_irq(&pdev->dev, priv->irq,
> > aspeed_peci_irq_handler,
> > +                              0, "peci-aspeed", priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       init_completion(&priv->xfer_complete);
> > +       spin_lock_init(&priv->lock);
> > +
> > +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> > +       if (IS_ERR(priv->rst))
> > +               return dev_err_probe(priv->dev, PTR_ERR(priv->rst),
> > +                                    "failed to get reset control\n");
> > +
> > +       ret = aspeed_peci_reset_control_deassert(priv->dev, priv->rst);
> > +       if (ret)
> > +               return dev_err_probe(priv->dev, ret, "cannot deassert reset
> > control\n");
> > +
> > +       priv->clk = devm_clk_get(priv->dev, NULL);
> > +       if (IS_ERR(priv->clk))
> > +               return dev_err_probe(priv->dev, PTR_ERR(priv->clk), "failed
> > to get clk\n");
> > +
> > +       ret = aspeed_peci_clk_enable(priv->dev, priv->clk);
> > +       if (ret)
> > +               return dev_err_probe(priv->dev, ret, "failed to enable
> > clock\n");
> > +
> > +       aspeed_peci_property_setup(priv);
> > +
> > +       aspeed_peci_init_regs(priv);
> > +
> > +       controller = devm_peci_controller_add(priv->dev, &aspeed_ops);
> > +       if (IS_ERR(controller))
> > +               return dev_err_probe(priv->dev, PTR_ERR(controller),
> > +                                    "failed to add aspeed peci
> > controller\n");
> > +
> > +       priv->controller = controller;
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id aspeed_peci_of_table[] = {
> > +       { .compatible = "aspeed,ast2400-peci", },
> > +       { .compatible = "aspeed,ast2500-peci", },
> > +       { .compatible = "aspeed,ast2600-peci", },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
> > +
> > +static struct platform_driver aspeed_peci_driver = {
> > +       .probe  = aspeed_peci_probe,
> > +       .driver = {
> > +               .name           = "peci-aspeed",
> > +               .of_match_table = aspeed_peci_of_table,
> > +       },
> > +};
> > +module_platform_driver(aspeed_peci_driver);
> > +
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> > +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
> > +MODULE_DESCRIPTION("ASPEED PECI driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS(PECI);
> > --
> > 2.31.1
> > 


  reply	other threads:[~2021-08-27  5:45 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-03 11:31 [PATCH v2 00/15] Introduce PECI subsystem Iwona Winiarska
2021-08-03 11:31 ` [PATCH v2 01/15] x86/cpu: Move intel-family to arch-independent headers Iwona Winiarska
2021-10-04 19:03   ` Borislav Petkov
2021-10-11 19:21     ` Winiarska, Iwona
2021-10-11 19:40       ` Dave Hansen
2021-10-11 20:53         ` Winiarska, Iwona
2021-10-11 23:12           ` Dave Hansen
2021-10-11 20:06       ` Borislav Petkov
2021-10-11 20:38         ` Winiarska, Iwona
2021-10-11 21:31           ` Borislav Petkov
2021-10-12 23:15             ` Winiarska, Iwona
2021-10-13  6:42               ` Borislav Petkov
2021-08-03 11:31 ` [PATCH v2 02/15] x86/cpu: Extract cpuid helpers to arch-independent Iwona Winiarska
2021-10-04 19:08   ` Borislav Petkov
2021-10-11 19:32     ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 03/15] dt-bindings: Add generic bindings for PECI Iwona Winiarska
2021-08-11 18:11   ` Rob Herring
2021-08-03 11:31 ` [PATCH v2 04/15] dt-bindings: Add bindings for peci-aspeed Iwona Winiarska
2021-08-11 18:11   ` Rob Herring
2021-08-03 11:31 ` [PATCH v2 05/15] ARM: dts: aspeed: Add PECI controller nodes Iwona Winiarska
2021-08-03 11:31 ` [PATCH v2 06/15] peci: Add core infrastructure Iwona Winiarska
2021-08-25 22:58   ` Dan Williams
2021-08-26 22:40     ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 07/15] peci: Add peci-aspeed controller driver Iwona Winiarska
2021-08-26  1:35   ` Dan Williams
2021-08-26 23:54     ` Winiarska, Iwona [this message]
2021-08-27 16:24       ` Dan Williams
2021-08-29 19:42         ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 08/15] peci: Add device detection Iwona Winiarska
2021-08-27 19:01   ` Dan Williams
2021-11-15 22:18     ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 09/15] peci: Add sysfs interface for PECI bus Iwona Winiarska
2021-08-27 19:11   ` Dan Williams
2021-11-15 22:19     ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 10/15] peci: Add support for PECI device drivers Iwona Winiarska
2021-08-27 21:19   ` Dan Williams
2021-11-15 22:20     ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 11/15] peci: Add peci-cpu driver Iwona Winiarska
2021-08-03 11:31 ` [PATCH v2 12/15] hwmon: peci: Add cputemp driver Iwona Winiarska
2021-08-03 15:24   ` Guenter Roeck
2021-08-04 10:43     ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 13/15] hwmon: peci: Add dimmtemp driver Iwona Winiarska
2021-08-03 15:39   ` Guenter Roeck
2021-08-04 10:46     ` Winiarska, Iwona
2021-08-04 17:33       ` Guenter Roeck
2021-08-05 21:48         ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 14/15] docs: hwmon: Document PECI drivers Iwona Winiarska
2021-08-03 11:31 ` [PATCH v2 15/15] docs: Add PECI documentation Iwona Winiarska
2021-08-05 12:17 ` [PATCH v2 00/15] Introduce PECI subsystem Greg Kroah-Hartman

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=b26ee278838698289869964fe59578f0d5f7b19c.camel@intel.com \
    --to=iwona.winiarska@intel.com \
    --cc=andrew@aj.id.au \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=arnd@arndb.de \
    --cc=bp@alien8.de \
    --cc=corbet@lwn.net \
    --cc=dan.j.williams@intel.com \
    --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=olof@lixom.net \
    --cc=openbmc@lists.ozlabs.org \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=rdunlap@infradead.org \
    --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 \
    --subject='Re: [PATCH v2 07/15] peci: Add peci-aspeed controller driver' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).