From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-path: MIME-Version: 1.0 Sender: joel.stan@gmail.com In-Reply-To: <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> References: <20180410183212.16787-1-jae.hyun.yoo@linux.intel.com> <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> From: Joel Stanley Date: Wed, 11 Apr 2018 21:21:43 +0930 Message-ID: Subject: Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx To: Jae Hyun Yoo , Ryan Chen Cc: Alan Cox , Andrew Jeffery , Andrew Lunn , Andy Shevchenko , Arnd Bergmann , Benjamin Herrenschmidt , Fengguang Wu , Greg KH , Guenter Roeck , Haiyue Wang , James Feist , Jason M Biils , Jean Delvare , Julia Cartwright , Miguel Ojeda , Milton Miller II , Pavel Machek , Randy Dunlap , Stef van Os , Sumeet R Pawnikar , Vernon Mauery , Linux Kernel Mailing List , linux-doc@vger.kernel.org, devicetree , linux-hwmon@vger.kernel.org, Linux ARM , OpenBMC Maillist Content-Type: text/plain; charset="UTF-8" List-ID: Hello Jae, On 11 April 2018 at 04:02, Jae Hyun Yoo wrote: > This commit adds PECI adapter driver implementation for Aspeed > AST24xx/AST25xx. The driver is looking good! It looks like you've done some kind of review that we weren't allowed to see, which is a double edged sword - I might be asking about things that you've already spoken about with someone else. I'm only just learning about PECI, but I do have some general comments below. > --- > drivers/peci/Kconfig | 28 +++ > drivers/peci/Makefile | 3 + > drivers/peci/peci-aspeed.c | 504 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 535 insertions(+) > create mode 100644 drivers/peci/peci-aspeed.c > > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig > index 1fbc13f9e6c2..0e33420365de 100644 > --- a/drivers/peci/Kconfig > +++ b/drivers/peci/Kconfig > @@ -14,4 +14,32 @@ config PECI > processors and chipset components to external monitoring or control > devices. > > + If you want PECI support, you should say Y here and also to the > + specific driver for your bus adapter(s) below. > + > +if PECI > + > +# > +# PECI hardware bus configuration > +# > + > +menu "PECI Hardware Bus support" > + > +config PECI_ASPEED > + tristate "Aspeed AST24xx/AST25xx PECI support" I think just saying ASPEED PECI support is enough. That way if the next ASPEED SoC happens to have PECI we don't need to update all of the help text :) > + select REGMAP_MMIO > + depends on OF > + depends on ARCH_ASPEED || COMPILE_TEST > + help > + Say Y here if you want support for the Platform Environment Control > + Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX > + SoCs. > + > + This support is also available as a module. If so, the module > + will be called peci-aspeed. > + > +endmenu > + > +endif # PECI > + > endmenu > diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile > index 9e8615e0d3ff..886285e69765 100644 > --- a/drivers/peci/Makefile > +++ b/drivers/peci/Makefile > @@ -4,3 +4,6 @@ > > # Core functionality > obj-$(CONFIG_PECI) += peci-core.o > + > +# Hardware specific bus drivers > +obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o > diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c > new file mode 100644 > index 000000000000..be2a1f327eb1 > --- /dev/null > +++ b/drivers/peci/peci-aspeed.c > @@ -0,0 +1,504 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2012-2017 ASPEED Technology Inc. > +// Copyright (c) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DUMP_DEBUG 0 > + > +/* Aspeed PECI Registers */ > +#define AST_PECI_CTRL 0x00 Nit: we use ASPEED instead of AST in the upstream kernel to distingush from the aspeed sdk drivers. If you feel strongly about this then I won't insist you change. > +#define AST_PECI_TIMING 0x04 > +#define AST_PECI_CMD 0x08 > +#define AST_PECI_CMD_CTRL 0x0c > +#define AST_PECI_EXP_FCS 0x10 > +#define AST_PECI_CAP_FCS 0x14 > +#define AST_PECI_INT_CTRL 0x18 > +#define AST_PECI_INT_STS 0x1c > +#define AST_PECI_W_DATA0 0x20 > +#define AST_PECI_W_DATA1 0x24 > +#define AST_PECI_W_DATA2 0x28 > +#define AST_PECI_W_DATA3 0x2c > +#define AST_PECI_R_DATA0 0x30 > +#define AST_PECI_R_DATA1 0x34 > +#define AST_PECI_R_DATA2 0x38 > +#define AST_PECI_R_DATA3 0x3c > +#define AST_PECI_W_DATA4 0x40 > +#define AST_PECI_W_DATA5 0x44 > +#define AST_PECI_W_DATA6 0x48 > +#define AST_PECI_W_DATA7 0x4c > +#define AST_PECI_R_DATA4 0x50 > +#define AST_PECI_R_DATA5 0x54 > +#define AST_PECI_R_DATA6 0x58 > +#define AST_PECI_R_DATA7 0x5c > + > +/* AST_PECI_CTRL - 0x00 : Control Register */ > +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) > +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) > +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) > +#define PECI_CTRL_READ_MODE_MASK GENMASK(13, 12) > +#define PECI_CTRL_READ_MODE(x) (((x) << 12) & PECI_CTRL_READ_MODE_MASK) > +#define PECI_CTRL_READ_MODE_GET(x) (((x) & PECI_CTRL_READ_MODE_MASK) >> 12) > +#define PECI_CTRL_READ_MODE_COUNT BIT(12) > +#define PECI_CTRL_READ_MODE_DBG BIT(13) > +#define PECI_CTRL_CLK_SOURCE_MASK BIT(11) > +#define PECI_CTRL_CLK_SOURCE(x) (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK) > +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11) > +#define PECI_CTRL_CLK_DIV_MASK GENMASK(10, 8) > +#define PECI_CTRL_CLK_DIV(x) (((x) << 8) & PECI_CTRL_CLK_DIV_MASK) > +#define PECI_CTRL_CLK_DIV_GET(x) (((x) & PECI_CTRL_CLK_DIV_MASK) >> 8) > +#define PECI_CTRL_INVERT_OUT BIT(7) > +#define PECI_CTRL_INVERT_IN BIT(6) > +#define PECI_CTRL_BUS_CONTENT_EN BIT(5) > +#define PECI_CTRL_PECI_EN BIT(4) > +#define PECI_CTRL_PECI_CLK_EN BIT(0) I know these come from the ASPEED sdk driver. Do we need them all? > + > +/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */ > +#define PECI_TIMING_MESSAGE_MASK GENMASK(15, 8) > +#define PECI_TIMING_MESSAGE(x) (((x) << 8) & PECI_TIMING_MESSAGE_MASK) > +#define PECI_TIMING_MESSAGE_GET(x) (((x) & PECI_TIMING_MESSAGE_MASK) >> 8) > +#define PECI_TIMING_ADDRESS_MASK GENMASK(7, 0) > +#define PECI_TIMING_ADDRESS(x) ((x) & PECI_TIMING_ADDRESS_MASK) > +#define PECI_TIMING_ADDRESS_GET(x) ((x) & PECI_TIMING_ADDRESS_MASK) > + > +/* AST_PECI_CMD - 0x08 : Command Register */ > +#define PECI_CMD_PIN_MON BIT(31) > +#define PECI_CMD_STS_MASK GENMASK(27, 24) > +#define PECI_CMD_STS_GET(x) (((x) & PECI_CMD_STS_MASK) >> 24) > +#define PECI_CMD_FIRE BIT(0) > + > +/* AST_PECI_LEN - 0x0C : Read/Write Length Register */ > +#define PECI_AW_FCS_EN BIT(31) > +#define PECI_READ_LEN_MASK GENMASK(23, 16) > +#define PECI_READ_LEN(x) (((x) << 16) & PECI_READ_LEN_MASK) > +#define PECI_WRITE_LEN_MASK GENMASK(15, 8) > +#define PECI_WRITE_LEN(x) (((x) << 8) & PECI_WRITE_LEN_MASK) > +#define PECI_TAGET_ADDR_MASK GENMASK(7, 0) > +#define PECI_TAGET_ADDR(x) ((x) & PECI_TAGET_ADDR_MASK) > + > +/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register */ > +#define PECI_EXPECT_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_EXPECT_READ_FCS_GET(x) (((x) & PECI_EXPECT_READ_FCS_MASK) >> 16) > +#define PECI_EXPECT_AW_FCS_AUTO_MASK GENMASK(15, 8) > +#define PECI_EXPECT_AW_FCS_AUTO_GET(x) (((x) & PECI_EXPECT_AW_FCS_AUTO_MASK) \ > + >> 8) > +#define PECI_EXPECT_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_EXPECT_WRITE_FCS_GET(x) ((x) & PECI_EXPECT_WRITE_FCS_MASK) > + > +/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */ > +#define PECI_CAPTURE_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_CAPTURE_READ_FCS_GET(x) (((x) & PECI_CAPTURE_READ_FCS_MASK) >> 16) > +#define PECI_CAPTURE_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_CAPTURE_WRITE_FCS_GET(x) ((x) & PECI_CAPTURE_WRITE_FCS_MASK) > + > +/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */ > +#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30) > +#define PECI_INT_TIMEOUT BIT(4) > +#define PECI_INT_CONNECT BIT(3) > +#define PECI_INT_W_FCS_BAD BIT(2) > +#define PECI_INT_W_FCS_ABORT BIT(1) > +#define PECI_INT_CMD_DONE BIT(0) > + > +struct aspeed_peci { > + struct peci_adapter adaper; > + struct device *dev; > + struct regmap *regmap; > + int irq; > + struct completion xfer_complete; > + u32 status; > + u32 cmd_timeout_ms; > +}; > + > +#define PECI_INT_MASK (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \ > + PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \ > + PECI_INT_CMD_DONE) > + > +#define PECI_IDLE_CHECK_TIMEOUT_MS 50 > +#define PECI_IDLE_CHECK_INTERVAL_MS 10 > + > +#define PECI_RD_SAMPLING_POINT_DEFAULT 8 > +#define PECI_RD_SAMPLING_POINT_MAX 15 > +#define PECI_CLK_DIV_DEFAULT 0 > +#define PECI_CLK_DIV_MAX 7 > +#define PECI_MSG_TIMING_NEGO_DEFAULT 1 > +#define PECI_MSG_TIMING_NEGO_MAX 255 > +#define PECI_ADDR_TIMING_NEGO_DEFAULT 1 > +#define PECI_ADDR_TIMING_NEGO_MAX 255 > +#define PECI_CMD_TIMEOUT_MS_DEFAULT 1000 > +#define PECI_CMD_TIMEOUT_MS_MAX 60000 > + > +static int aspeed_peci_xfer_native(struct aspeed_peci *priv, > + struct peci_xfer_msg *msg) > +{ > + long err, timeout = msecs_to_jiffies(priv->cmd_timeout_ms); > + u32 peci_head, peci_state, rx_data, cmd_sts; > + ktime_t start, end; > + s64 elapsed_ms; > + int i, rc = 0; > + uint reg; > + > + start = ktime_get(); > + > + /* Check command sts and bus idle state */ > + while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts) && > + (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) { > + end = ktime_get(); > + elapsed_ms = ktime_to_ms(ktime_sub(end, start)); > + if (elapsed_ms >= PECI_IDLE_CHECK_TIMEOUT_MS) { > + dev_dbg(priv->dev, "Timeout waiting for idle state!\n"); > + return -ETIMEDOUT; > + } > + > + usleep_range(PECI_IDLE_CHECK_INTERVAL_MS * 1000, > + (PECI_IDLE_CHECK_INTERVAL_MS * 1000) + 1000); > + }; Could the above use regmap_read_poll_timeout instead? > + > + reinit_completion(&priv->xfer_complete); > + > + peci_head = PECI_TAGET_ADDR(msg->addr) | > + PECI_WRITE_LEN(msg->tx_len) | > + PECI_READ_LEN(msg->rx_len); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head); > + if (rc) > + return rc; > + > + for (i = 0; i < msg->tx_len; i += 4) { > + reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 : > + AST_PECI_W_DATA4 + i % 16; > + rc = regmap_write(priv->regmap, reg, > + (msg->tx_buf[i + 3] << 24) | > + (msg->tx_buf[i + 2] << 16) | > + (msg->tx_buf[i + 1] << 8) | > + msg->tx_buf[i + 0]); That looks like an endian swap. Can we do something like this? regmap_write(map, reg, cpu_to_be32p((void *)msg->tx_buff)) > + if (rc) > + return rc; > + } > + > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > +#if DUMP_DEBUG Having #defines is frowned upon. I think print_hex_dump_debug will do what you want here. > + print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->tx_buf, msg->tx_len, true); > +#endif > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE); > + if (rc) > + return rc; > + > + err = wait_for_completion_interruptible_timeout(&priv->xfer_complete, > + timeout); > + > + dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->status); > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, 0); > + if (rc) > + return rc; > + > + if (err <= 0 || !(priv->status & PECI_INT_CMD_DONE)) { > + if (err < 0) { /* -ERESTARTSYS */ > + return (int)err; > + } else if (err == 0) { > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > + return -ETIMEDOUT; > + } > + > + dev_dbg(priv->dev, "No valid response!\n"); > + return -EIO; > + } > + > + for (i = 0; i < msg->rx_len; i++) { > + u8 byte_offset = i % 4; > + > + if (byte_offset == 0) { > + reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 : > + AST_PECI_R_DATA4 + i % 16; I find this hard to read. Use a few more lines to make it clear what your code is doing. Actually, the entire for loop is cryptic. I understand what it's doing now. Can you rework it to make it more readable? You follow a similar pattern above in the write case. > + rc = regmap_read(priv->regmap, reg, &rx_data); > + if (rc) > + return rc; > + } > + > + msg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3)) > + } > + > +#if DUMP_DEBUG > + print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->rx_buf, msg->rx_len, true); > +#endif > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); Given the regmap_read is always going to be a memory read on the aspeed, I can't think of a situation where the read will fail. On that note, is there a reason you are using regmap and not just accessing the hardware directly? regmap imposes a number of pointer lookups and tests each time you do a read or write. > + dev_dbg(priv->dev, "------------------------\n"); > + > + return rc; > +} > + > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > +{ > + struct aspeed_peci *priv = arg; > + u32 status_ack = 0; > + > + if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->status)) > + return IRQ_NONE; Again, a memory mapped read won't fail. How about we check that the regmap is working once in your _probe() function, and assume it will continue working from there (or remove the regmap abstraction all together). > + > + /* Be noted that multiple interrupt bits can be set at the same time */ > + if (priv->status & PECI_INT_TIMEOUT) { > + dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n"); > + status_ack |= PECI_INT_TIMEOUT; > + } > + > + if (priv->status & PECI_INT_CONNECT) { > + dev_dbg(priv->dev, "PECI_INT_CONNECT\n"); > + status_ack |= PECI_INT_CONNECT; > + } > + > + if (priv->status & PECI_INT_W_FCS_BAD) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n"); > + status_ack |= PECI_INT_W_FCS_BAD; > + } > + > + if (priv->status & PECI_INT_W_FCS_ABORT) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n"); > + status_ack |= PECI_INT_W_FCS_ABORT; > + } All of this code is for debugging only. Do you want to put it behind some kind of conditional? > + > + /** > + * All commands should be ended up with a PECI_INT_CMD_DONE bit set > + * even in an error case. > + */ > + if (priv->status & PECI_INT_CMD_DONE) { > + dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n"); > + status_ack |= PECI_INT_CMD_DONE; > + complete(&priv->xfer_complete); > + } > + > + if (regmap_write(priv->regmap, AST_PECI_INT_STS, status_ack)) > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} > + > +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv) > +{ > + u32 msg_timing_nego, addr_timing_nego, rd_sampling_point; > + u32 clk_freq, clk_divisor, clk_div_val = 0; > + struct clk *clkin; > + int ret; > + > + clkin = devm_clk_get(priv->dev, NULL); > + if (IS_ERR(clkin)) { > + dev_err(priv->dev, "Failed to get clk source.\n"); > + return PTR_ERR(clkin); > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "clock-frequency", > + &clk_freq); > + if (ret < 0) { > + dev_err(priv->dev, > + "Could not read clock-frequency property.\n"); > + return ret; > + } > + > + clk_divisor = clk_get_rate(clkin) / clk_freq; > + devm_clk_put(priv->dev, clkin); > + > + while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX)) > + clk_div_val++; We have a framework for doing clocks in the kernel. Would it make sense to write a driver for this clock and add it to drivers/clk/clk-aspeed.c? > + > + ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego", > + &msg_timing_nego); > + if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid msg-timing-nego : %u, Use default : %u\n", > + msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT); The property is optional so I suggest we don't print a message if it's not present. We certainly don't want to print a message saying "invalid". The same comment applies to the other optional properties below. > + msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego", > + &addr_timing_nego); > + if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid addr-timing-nego : %u, Use default : %u\n", > + addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT); > + addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point", > + &rd_sampling_point); > + if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) { > + dev_warn(priv->dev, > + "Invalid rd-sampling-point : %u. Use default : %u\n", > + rd_sampling_point, > + PECI_RD_SAMPLING_POINT_DEFAULT); > + rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms", > + &priv->cmd_timeout_ms); > + if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX || > + priv->cmd_timeout_ms == 0) { > + dev_warn(priv->dev, > + "Invalid cmd-timeout-ms : %u. Use default : %u\n", > + priv->cmd_timeout_ms, > + PECI_CMD_TIMEOUT_MS_DEFAULT); > + priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT; > + } > + > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) | > + PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + usleep_range(1000, 5000); Can we probe in parallel? If not, putting a sleep in the _probe will hold up the rest of drivers from being able to do anything, and hold up boot. If you decide that you do need to probe here, please add a comment. (This is the wait for the clock to be stable?) > + > + /** > + * Timing negotiation period setting. > + * The unit of the programmed value is 4 times of PECI clock period. > + */ > + ret = regmap_write(priv->regmap, AST_PECI_TIMING, > + PECI_TIMING_MESSAGE(msg_timing_nego) | > + PECI_TIMING_ADDRESS(addr_timing_nego)); > + if (ret) > + return ret; > + > + /* Clear interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Enable interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Read sampling point and clock speed setting */ > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_SAMPLING(rd_sampling_point) | > + PECI_CTRL_CLK_DIV(clk_div_val) | > + PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static const struct regmap_config aspeed_peci_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = AST_PECI_R_DATA7, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > + .fast_io = true, > +}; > + > +static int aspeed_peci_xfer(struct peci_adapter *adaper, > + struct peci_xfer_msg *msg) > +{ > + struct aspeed_peci *priv = peci_get_adapdata(adaper); > + > + return aspeed_peci_xfer_native(priv, msg); > +} > + > +static int aspeed_peci_probe(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv; > + struct resource *res; > + void __iomem *base; > + int ret = 0; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + dev_set_drvdata(&pdev->dev, priv); > + priv->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &aspeed_peci_regmap_config); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->irq = platform_get_irq(pdev, 0); > + if (!priv->irq) > + return -ENODEV; > + > + ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler, > + IRQF_SHARED, This interrupt is only for the peci device. Why is it marked as shared? > + "peci-aspeed-irq", > + priv); > + if (ret < 0) > + return ret; > + > + init_completion(&priv->xfer_complete); > + > + priv->adaper.dev.parent = priv->dev; > + priv->adaper.dev.of_node = of_node_get(dev_of_node(priv->dev)); > + strlcpy(priv->adaper.name, pdev->name, sizeof(priv->adaper.name)); > + priv->adaper.xfer = aspeed_peci_xfer; > + peci_set_adapdata(&priv->adaper, priv); > + > + ret = aspeed_peci_init_ctrl(priv); > + if (ret < 0) > + return ret; > + > + ret = peci_add_adapter(&priv->adaper); > + if (ret < 0) > + return ret; > + > + dev_info(&pdev->dev, "peci bus %d registered, irq %d\n", > + priv->adaper.nr, priv->irq); > + > + return 0; > +} > + > +static int aspeed_peci_remove(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev); > + > + peci_del_adapter(&priv->adaper); > + of_node_put(priv->adaper.dev.of_node); > + > + return 0; > +} > + > +static const struct of_device_id aspeed_peci_of_table[] = { > + { .compatible = "aspeed,ast2400-peci", }, > + { .compatible = "aspeed,ast2500-peci", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table); > + > +static struct platform_driver aspeed_peci_driver = { > + .probe = aspeed_peci_probe, > + .remove = aspeed_peci_remove, > + .driver = { > + .name = "peci-aspeed", > + .of_match_table = of_match_ptr(aspeed_peci_of_table), > + }, > +}; > +module_platform_driver(aspeed_peci_driver); > + > +MODULE_AUTHOR("Ryan Chen "); > +MODULE_AUTHOR("Jae Hyun Yoo "); > +MODULE_DESCRIPTION("Aspeed PECI driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.16.2 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joel Stanley Subject: Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx Date: Wed, 11 Apr 2018 21:21:43 +0930 Message-ID: References: <20180410183212.16787-1-jae.hyun.yoo@linux.intel.com> <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> Sender: linux-kernel-owner@vger.kernel.org To: Jae Hyun Yoo , Ryan Chen Cc: Alan Cox , Andrew Jeffery , Andrew Lunn , Andy Shevchenko , Arnd Bergmann , Benjamin Herrenschmidt , Fengguang Wu , Greg KH , Guenter Roeck , Haiyue Wang , James Feist , Jason M Biils , Jean Delvare , Julia Cartwright , Miguel Ojeda , Milton Miller II , Pavel Machek , Randy Dunlap , Stef van Os List-Id: devicetree@vger.kernel.org Hello Jae, On 11 April 2018 at 04:02, Jae Hyun Yoo wrote: > This commit adds PECI adapter driver implementation for Aspeed > AST24xx/AST25xx. The driver is looking good! It looks like you've done some kind of review that we weren't allowed to see, which is a double edged sword - I might be asking about things that you've already spoken about with someone else. I'm only just learning about PECI, but I do have some general comments below. > --- > drivers/peci/Kconfig | 28 +++ > drivers/peci/Makefile | 3 + > drivers/peci/peci-aspeed.c | 504 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 535 insertions(+) > create mode 100644 drivers/peci/peci-aspeed.c > > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig > index 1fbc13f9e6c2..0e33420365de 100644 > --- a/drivers/peci/Kconfig > +++ b/drivers/peci/Kconfig > @@ -14,4 +14,32 @@ config PECI > processors and chipset components to external monitoring or control > devices. > > + If you want PECI support, you should say Y here and also to the > + specific driver for your bus adapter(s) below. > + > +if PECI > + > +# > +# PECI hardware bus configuration > +# > + > +menu "PECI Hardware Bus support" > + > +config PECI_ASPEED > + tristate "Aspeed AST24xx/AST25xx PECI support" I think just saying ASPEED PECI support is enough. That way if the next ASPEED SoC happens to have PECI we don't need to update all of the help text :) > + select REGMAP_MMIO > + depends on OF > + depends on ARCH_ASPEED || COMPILE_TEST > + help > + Say Y here if you want support for the Platform Environment Control > + Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX > + SoCs. > + > + This support is also available as a module. If so, the module > + will be called peci-aspeed. > + > +endmenu > + > +endif # PECI > + > endmenu > diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile > index 9e8615e0d3ff..886285e69765 100644 > --- a/drivers/peci/Makefile > +++ b/drivers/peci/Makefile > @@ -4,3 +4,6 @@ > > # Core functionality > obj-$(CONFIG_PECI) += peci-core.o > + > +# Hardware specific bus drivers > +obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o > diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c > new file mode 100644 > index 000000000000..be2a1f327eb1 > --- /dev/null > +++ b/drivers/peci/peci-aspeed.c > @@ -0,0 +1,504 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2012-2017 ASPEED Technology Inc. > +// Copyright (c) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DUMP_DEBUG 0 > + > +/* Aspeed PECI Registers */ > +#define AST_PECI_CTRL 0x00 Nit: we use ASPEED instead of AST in the upstream kernel to distingush from the aspeed sdk drivers. If you feel strongly about this then I won't insist you change. > +#define AST_PECI_TIMING 0x04 > +#define AST_PECI_CMD 0x08 > +#define AST_PECI_CMD_CTRL 0x0c > +#define AST_PECI_EXP_FCS 0x10 > +#define AST_PECI_CAP_FCS 0x14 > +#define AST_PECI_INT_CTRL 0x18 > +#define AST_PECI_INT_STS 0x1c > +#define AST_PECI_W_DATA0 0x20 > +#define AST_PECI_W_DATA1 0x24 > +#define AST_PECI_W_DATA2 0x28 > +#define AST_PECI_W_DATA3 0x2c > +#define AST_PECI_R_DATA0 0x30 > +#define AST_PECI_R_DATA1 0x34 > +#define AST_PECI_R_DATA2 0x38 > +#define AST_PECI_R_DATA3 0x3c > +#define AST_PECI_W_DATA4 0x40 > +#define AST_PECI_W_DATA5 0x44 > +#define AST_PECI_W_DATA6 0x48 > +#define AST_PECI_W_DATA7 0x4c > +#define AST_PECI_R_DATA4 0x50 > +#define AST_PECI_R_DATA5 0x54 > +#define AST_PECI_R_DATA6 0x58 > +#define AST_PECI_R_DATA7 0x5c > + > +/* AST_PECI_CTRL - 0x00 : Control Register */ > +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) > +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) > +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) > +#define PECI_CTRL_READ_MODE_MASK GENMASK(13, 12) > +#define PECI_CTRL_READ_MODE(x) (((x) << 12) & PECI_CTRL_READ_MODE_MASK) > +#define PECI_CTRL_READ_MODE_GET(x) (((x) & PECI_CTRL_READ_MODE_MASK) >> 12) > +#define PECI_CTRL_READ_MODE_COUNT BIT(12) > +#define PECI_CTRL_READ_MODE_DBG BIT(13) > +#define PECI_CTRL_CLK_SOURCE_MASK BIT(11) > +#define PECI_CTRL_CLK_SOURCE(x) (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK) > +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11) > +#define PECI_CTRL_CLK_DIV_MASK GENMASK(10, 8) > +#define PECI_CTRL_CLK_DIV(x) (((x) << 8) & PECI_CTRL_CLK_DIV_MASK) > +#define PECI_CTRL_CLK_DIV_GET(x) (((x) & PECI_CTRL_CLK_DIV_MASK) >> 8) > +#define PECI_CTRL_INVERT_OUT BIT(7) > +#define PECI_CTRL_INVERT_IN BIT(6) > +#define PECI_CTRL_BUS_CONTENT_EN BIT(5) > +#define PECI_CTRL_PECI_EN BIT(4) > +#define PECI_CTRL_PECI_CLK_EN BIT(0) I know these come from the ASPEED sdk driver. Do we need them all? > + > +/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */ > +#define PECI_TIMING_MESSAGE_MASK GENMASK(15, 8) > +#define PECI_TIMING_MESSAGE(x) (((x) << 8) & PECI_TIMING_MESSAGE_MASK) > +#define PECI_TIMING_MESSAGE_GET(x) (((x) & PECI_TIMING_MESSAGE_MASK) >> 8) > +#define PECI_TIMING_ADDRESS_MASK GENMASK(7, 0) > +#define PECI_TIMING_ADDRESS(x) ((x) & PECI_TIMING_ADDRESS_MASK) > +#define PECI_TIMING_ADDRESS_GET(x) ((x) & PECI_TIMING_ADDRESS_MASK) > + > +/* AST_PECI_CMD - 0x08 : Command Register */ > +#define PECI_CMD_PIN_MON BIT(31) > +#define PECI_CMD_STS_MASK GENMASK(27, 24) > +#define PECI_CMD_STS_GET(x) (((x) & PECI_CMD_STS_MASK) >> 24) > +#define PECI_CMD_FIRE BIT(0) > + > +/* AST_PECI_LEN - 0x0C : Read/Write Length Register */ > +#define PECI_AW_FCS_EN BIT(31) > +#define PECI_READ_LEN_MASK GENMASK(23, 16) > +#define PECI_READ_LEN(x) (((x) << 16) & PECI_READ_LEN_MASK) > +#define PECI_WRITE_LEN_MASK GENMASK(15, 8) > +#define PECI_WRITE_LEN(x) (((x) << 8) & PECI_WRITE_LEN_MASK) > +#define PECI_TAGET_ADDR_MASK GENMASK(7, 0) > +#define PECI_TAGET_ADDR(x) ((x) & PECI_TAGET_ADDR_MASK) > + > +/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register */ > +#define PECI_EXPECT_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_EXPECT_READ_FCS_GET(x) (((x) & PECI_EXPECT_READ_FCS_MASK) >> 16) > +#define PECI_EXPECT_AW_FCS_AUTO_MASK GENMASK(15, 8) > +#define PECI_EXPECT_AW_FCS_AUTO_GET(x) (((x) & PECI_EXPECT_AW_FCS_AUTO_MASK) \ > + >> 8) > +#define PECI_EXPECT_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_EXPECT_WRITE_FCS_GET(x) ((x) & PECI_EXPECT_WRITE_FCS_MASK) > + > +/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */ > +#define PECI_CAPTURE_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_CAPTURE_READ_FCS_GET(x) (((x) & PECI_CAPTURE_READ_FCS_MASK) >> 16) > +#define PECI_CAPTURE_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_CAPTURE_WRITE_FCS_GET(x) ((x) & PECI_CAPTURE_WRITE_FCS_MASK) > + > +/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */ > +#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30) > +#define PECI_INT_TIMEOUT BIT(4) > +#define PECI_INT_CONNECT BIT(3) > +#define PECI_INT_W_FCS_BAD BIT(2) > +#define PECI_INT_W_FCS_ABORT BIT(1) > +#define PECI_INT_CMD_DONE BIT(0) > + > +struct aspeed_peci { > + struct peci_adapter adaper; > + struct device *dev; > + struct regmap *regmap; > + int irq; > + struct completion xfer_complete; > + u32 status; > + u32 cmd_timeout_ms; > +}; > + > +#define PECI_INT_MASK (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \ > + PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \ > + PECI_INT_CMD_DONE) > + > +#define PECI_IDLE_CHECK_TIMEOUT_MS 50 > +#define PECI_IDLE_CHECK_INTERVAL_MS 10 > + > +#define PECI_RD_SAMPLING_POINT_DEFAULT 8 > +#define PECI_RD_SAMPLING_POINT_MAX 15 > +#define PECI_CLK_DIV_DEFAULT 0 > +#define PECI_CLK_DIV_MAX 7 > +#define PECI_MSG_TIMING_NEGO_DEFAULT 1 > +#define PECI_MSG_TIMING_NEGO_MAX 255 > +#define PECI_ADDR_TIMING_NEGO_DEFAULT 1 > +#define PECI_ADDR_TIMING_NEGO_MAX 255 > +#define PECI_CMD_TIMEOUT_MS_DEFAULT 1000 > +#define PECI_CMD_TIMEOUT_MS_MAX 60000 > + > +static int aspeed_peci_xfer_native(struct aspeed_peci *priv, > + struct peci_xfer_msg *msg) > +{ > + long err, timeout = msecs_to_jiffies(priv->cmd_timeout_ms); > + u32 peci_head, peci_state, rx_data, cmd_sts; > + ktime_t start, end; > + s64 elapsed_ms; > + int i, rc = 0; > + uint reg; > + > + start = ktime_get(); > + > + /* Check command sts and bus idle state */ > + while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts) && > + (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) { > + end = ktime_get(); > + elapsed_ms = ktime_to_ms(ktime_sub(end, start)); > + if (elapsed_ms >= PECI_IDLE_CHECK_TIMEOUT_MS) { > + dev_dbg(priv->dev, "Timeout waiting for idle state!\n"); > + return -ETIMEDOUT; > + } > + > + usleep_range(PECI_IDLE_CHECK_INTERVAL_MS * 1000, > + (PECI_IDLE_CHECK_INTERVAL_MS * 1000) + 1000); > + }; Could the above use regmap_read_poll_timeout instead? > + > + reinit_completion(&priv->xfer_complete); > + > + peci_head = PECI_TAGET_ADDR(msg->addr) | > + PECI_WRITE_LEN(msg->tx_len) | > + PECI_READ_LEN(msg->rx_len); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head); > + if (rc) > + return rc; > + > + for (i = 0; i < msg->tx_len; i += 4) { > + reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 : > + AST_PECI_W_DATA4 + i % 16; > + rc = regmap_write(priv->regmap, reg, > + (msg->tx_buf[i + 3] << 24) | > + (msg->tx_buf[i + 2] << 16) | > + (msg->tx_buf[i + 1] << 8) | > + msg->tx_buf[i + 0]); That looks like an endian swap. Can we do something like this? regmap_write(map, reg, cpu_to_be32p((void *)msg->tx_buff)) > + if (rc) > + return rc; > + } > + > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > +#if DUMP_DEBUG Having #defines is frowned upon. I think print_hex_dump_debug will do what you want here. > + print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->tx_buf, msg->tx_len, true); > +#endif > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE); > + if (rc) > + return rc; > + > + err = wait_for_completion_interruptible_timeout(&priv->xfer_complete, > + timeout); > + > + dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->status); > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, 0); > + if (rc) > + return rc; > + > + if (err <= 0 || !(priv->status & PECI_INT_CMD_DONE)) { > + if (err < 0) { /* -ERESTARTSYS */ > + return (int)err; > + } else if (err == 0) { > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > + return -ETIMEDOUT; > + } > + > + dev_dbg(priv->dev, "No valid response!\n"); > + return -EIO; > + } > + > + for (i = 0; i < msg->rx_len; i++) { > + u8 byte_offset = i % 4; > + > + if (byte_offset == 0) { > + reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 : > + AST_PECI_R_DATA4 + i % 16; I find this hard to read. Use a few more lines to make it clear what your code is doing. Actually, the entire for loop is cryptic. I understand what it's doing now. Can you rework it to make it more readable? You follow a similar pattern above in the write case. > + rc = regmap_read(priv->regmap, reg, &rx_data); > + if (rc) > + return rc; > + } > + > + msg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3)) > + } > + > +#if DUMP_DEBUG > + print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->rx_buf, msg->rx_len, true); > +#endif > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); Given the regmap_read is always going to be a memory read on the aspeed, I can't think of a situation where the read will fail. On that note, is there a reason you are using regmap and not just accessing the hardware directly? regmap imposes a number of pointer lookups and tests each time you do a read or write. > + dev_dbg(priv->dev, "------------------------\n"); > + > + return rc; > +} > + > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > +{ > + struct aspeed_peci *priv = arg; > + u32 status_ack = 0; > + > + if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->status)) > + return IRQ_NONE; Again, a memory mapped read won't fail. How about we check that the regmap is working once in your _probe() function, and assume it will continue working from there (or remove the regmap abstraction all together). > + > + /* Be noted that multiple interrupt bits can be set at the same time */ > + if (priv->status & PECI_INT_TIMEOUT) { > + dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n"); > + status_ack |= PECI_INT_TIMEOUT; > + } > + > + if (priv->status & PECI_INT_CONNECT) { > + dev_dbg(priv->dev, "PECI_INT_CONNECT\n"); > + status_ack |= PECI_INT_CONNECT; > + } > + > + if (priv->status & PECI_INT_W_FCS_BAD) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n"); > + status_ack |= PECI_INT_W_FCS_BAD; > + } > + > + if (priv->status & PECI_INT_W_FCS_ABORT) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n"); > + status_ack |= PECI_INT_W_FCS_ABORT; > + } All of this code is for debugging only. Do you want to put it behind some kind of conditional? > + > + /** > + * All commands should be ended up with a PECI_INT_CMD_DONE bit set > + * even in an error case. > + */ > + if (priv->status & PECI_INT_CMD_DONE) { > + dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n"); > + status_ack |= PECI_INT_CMD_DONE; > + complete(&priv->xfer_complete); > + } > + > + if (regmap_write(priv->regmap, AST_PECI_INT_STS, status_ack)) > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} > + > +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv) > +{ > + u32 msg_timing_nego, addr_timing_nego, rd_sampling_point; > + u32 clk_freq, clk_divisor, clk_div_val = 0; > + struct clk *clkin; > + int ret; > + > + clkin = devm_clk_get(priv->dev, NULL); > + if (IS_ERR(clkin)) { > + dev_err(priv->dev, "Failed to get clk source.\n"); > + return PTR_ERR(clkin); > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "clock-frequency", > + &clk_freq); > + if (ret < 0) { > + dev_err(priv->dev, > + "Could not read clock-frequency property.\n"); > + return ret; > + } > + > + clk_divisor = clk_get_rate(clkin) / clk_freq; > + devm_clk_put(priv->dev, clkin); > + > + while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX)) > + clk_div_val++; We have a framework for doing clocks in the kernel. Would it make sense to write a driver for this clock and add it to drivers/clk/clk-aspeed.c? > + > + ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego", > + &msg_timing_nego); > + if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid msg-timing-nego : %u, Use default : %u\n", > + msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT); The property is optional so I suggest we don't print a message if it's not present. We certainly don't want to print a message saying "invalid". The same comment applies to the other optional properties below. > + msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego", > + &addr_timing_nego); > + if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid addr-timing-nego : %u, Use default : %u\n", > + addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT); > + addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point", > + &rd_sampling_point); > + if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) { > + dev_warn(priv->dev, > + "Invalid rd-sampling-point : %u. Use default : %u\n", > + rd_sampling_point, > + PECI_RD_SAMPLING_POINT_DEFAULT); > + rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms", > + &priv->cmd_timeout_ms); > + if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX || > + priv->cmd_timeout_ms == 0) { > + dev_warn(priv->dev, > + "Invalid cmd-timeout-ms : %u. Use default : %u\n", > + priv->cmd_timeout_ms, > + PECI_CMD_TIMEOUT_MS_DEFAULT); > + priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT; > + } > + > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) | > + PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + usleep_range(1000, 5000); Can we probe in parallel? If not, putting a sleep in the _probe will hold up the rest of drivers from being able to do anything, and hold up boot. If you decide that you do need to probe here, please add a comment. (This is the wait for the clock to be stable?) > + > + /** > + * Timing negotiation period setting. > + * The unit of the programmed value is 4 times of PECI clock period. > + */ > + ret = regmap_write(priv->regmap, AST_PECI_TIMING, > + PECI_TIMING_MESSAGE(msg_timing_nego) | > + PECI_TIMING_ADDRESS(addr_timing_nego)); > + if (ret) > + return ret; > + > + /* Clear interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Enable interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Read sampling point and clock speed setting */ > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_SAMPLING(rd_sampling_point) | > + PECI_CTRL_CLK_DIV(clk_div_val) | > + PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static const struct regmap_config aspeed_peci_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = AST_PECI_R_DATA7, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > + .fast_io = true, > +}; > + > +static int aspeed_peci_xfer(struct peci_adapter *adaper, > + struct peci_xfer_msg *msg) > +{ > + struct aspeed_peci *priv = peci_get_adapdata(adaper); > + > + return aspeed_peci_xfer_native(priv, msg); > +} > + > +static int aspeed_peci_probe(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv; > + struct resource *res; > + void __iomem *base; > + int ret = 0; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + dev_set_drvdata(&pdev->dev, priv); > + priv->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &aspeed_peci_regmap_config); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->irq = platform_get_irq(pdev, 0); > + if (!priv->irq) > + return -ENODEV; > + > + ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler, > + IRQF_SHARED, This interrupt is only for the peci device. Why is it marked as shared? > + "peci-aspeed-irq", > + priv); > + if (ret < 0) > + return ret; > + > + init_completion(&priv->xfer_complete); > + > + priv->adaper.dev.parent = priv->dev; > + priv->adaper.dev.of_node = of_node_get(dev_of_node(priv->dev)); > + strlcpy(priv->adaper.name, pdev->name, sizeof(priv->adaper.name)); > + priv->adaper.xfer = aspeed_peci_xfer; > + peci_set_adapdata(&priv->adaper, priv); > + > + ret = aspeed_peci_init_ctrl(priv); > + if (ret < 0) > + return ret; > + > + ret = peci_add_adapter(&priv->adaper); > + if (ret < 0) > + return ret; > + > + dev_info(&pdev->dev, "peci bus %d registered, irq %d\n", > + priv->adaper.nr, priv->irq); > + > + return 0; > +} > + > +static int aspeed_peci_remove(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev); > + > + peci_del_adapter(&priv->adaper); > + of_node_put(priv->adaper.dev.of_node); > + > + return 0; > +} > + > +static const struct of_device_id aspeed_peci_of_table[] = { > + { .compatible = "aspeed,ast2400-peci", }, > + { .compatible = "aspeed,ast2500-peci", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table); > + > +static struct platform_driver aspeed_peci_driver = { > + .probe = aspeed_peci_probe, > + .remove = aspeed_peci_remove, > + .driver = { > + .name = "peci-aspeed", > + .of_match_table = of_match_ptr(aspeed_peci_of_table), > + }, > +}; > +module_platform_driver(aspeed_peci_driver); > + > +MODULE_AUTHOR("Ryan Chen "); > +MODULE_AUTHOR("Jae Hyun Yoo "); > +MODULE_DESCRIPTION("Aspeed PECI driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.16.2 > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on archive.lwn.net X-Spam-Level: X-Spam-Status: No, score=-5.6 required=5.0 tests=DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=unavailable autolearn_force=no version=3.4.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by archive.lwn.net (Postfix) with ESMTP id 8C22A7DE78 for ; Wed, 11 Apr 2018 11:54:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751898AbeDKLwH (ORCPT ); Wed, 11 Apr 2018 07:52:07 -0400 Received: from mail-qk0-f196.google.com ([209.85.220.196]:45555 "EHLO mail-qk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751819AbeDKLwF (ORCPT ); Wed, 11 Apr 2018 07:52:05 -0400 Received: by mail-qk0-f196.google.com with SMTP id s9so1446567qke.12; Wed, 11 Apr 2018 04:52:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=kZWqitmvM9JWSCtKTw2cMLrBV0xAcx8l9wae3/4Rs5s=; b=r3wSV/btVk8DZBNa3qYLcNgwUZby6d9Id4Mo6NNkUdrknOdVKRfx2K72xkNgW3ba3l QWT1Ox5bZrjDvw7G1AN7Q9drqjEAIQfFZVEDRRInBztaU8XUTMZrQFALAlSykV+82tsv xRVJIrFmzqx2Q+VA2iwzvA806JdtK7IqkifqIPbZ0nJw8ey/rlG4CT9HjVlHqkuMEDL1 KItBevC26bYdeSLXwOGT+W5HSW3X76Md/btzleQVLu6cYV2U62tSwXI7rSlktOJtQtmP IRhgogHM8NLopn2BPyLbNLfTSJ2H2RDMj3qpRjig0R3y1BakFEj5VuUoDEr/+SyfYUCP wI3A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jms.id.au; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=kZWqitmvM9JWSCtKTw2cMLrBV0xAcx8l9wae3/4Rs5s=; b=VB6RNkzq2oO7cJVJV60ZiPId7SLRWj0LH0Zvx/FxHRoJ1zBIzOycJ5yV+mjJnwH+CB cxBhvC3rdLKlvRjVVjOMrzyiwZYQce7bztGss4Tl8J/93KURid6WCrzpeYsToGsKhLAK gF3hnU1U8oyyeFtxiN6MT8s1PPx8T+gjiXYkI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=kZWqitmvM9JWSCtKTw2cMLrBV0xAcx8l9wae3/4Rs5s=; b=i7ut3/D15HyKbvHJR9AK/qQov2K9HD6Rn3ib7sq5OCh5vkq1VyFKM7uqpiL7MjUqTo XB4t01vkqp3xMk3XIdqPf2cE/MyGzC8vYnh9GWKlRO7Y4fUUGXZkDSoCfszDU4p2ESp8 zX8e9WIFnMbCBx7WX12pASrj9PQN96g+yd8KdWV3ZCkPBycGvvebt2daFveULrqB7cbH tN5sD6AXqXgkBYGtkcoY+Fbz9iWWP3/KHuxWZKVNV8fdd0fD209X04BS7Zm4ZcrE/0kg oWr4WlWeYiWQIUZZUic4Nnv9w6llXcbWUSomkOTpr5zG7uYTbXNYRG6GadWl+O090wX3 PRyA== X-Gm-Message-State: ALQs6tBtR1+ZOcvcd5uJYFdmV3qMK2NlzfynAAfoA4w69d1kOYBfkkRQ YA0F4qPkKLK+ohaI3kxk/Q8QIRyiZuBQxzIi7bY= X-Google-Smtp-Source: AIpwx4/0A3vCWCULvCHNwepIwih56YgyGt6yey85+S8YFYptMGg5sYI2Bbrw10l9080H/ifXYNnTMqSTUC3KOGcIYxY= X-Received: by 10.55.44.3 with SMTP id s3mr6220350qkh.231.1523447523686; Wed, 11 Apr 2018 04:52:03 -0700 (PDT) MIME-Version: 1.0 Received: by 10.200.63.197 with HTTP; Wed, 11 Apr 2018 04:51:43 -0700 (PDT) In-Reply-To: <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> References: <20180410183212.16787-1-jae.hyun.yoo@linux.intel.com> <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> From: Joel Stanley Date: Wed, 11 Apr 2018 21:21:43 +0930 X-Google-Sender-Auth: KFsfZFM82sqWoHa4miFWFLuxNJk Message-ID: Subject: Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx To: Jae Hyun Yoo , Ryan Chen Cc: Alan Cox , Andrew Jeffery , Andrew Lunn , Andy Shevchenko , Arnd Bergmann , Benjamin Herrenschmidt , Fengguang Wu , Greg KH , Guenter Roeck , Haiyue Wang , James Feist , Jason M Biils , Jean Delvare , Julia Cartwright , Miguel Ojeda , Milton Miller II , Pavel Machek , Randy Dunlap , Stef van Os , Sumeet R Pawnikar , Vernon Mauery , Linux Kernel Mailing List , linux-doc@vger.kernel.org, devicetree , linux-hwmon@vger.kernel.org, Linux ARM , OpenBMC Maillist Content-Type: text/plain; charset="UTF-8" Sender: linux-doc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org Hello Jae, On 11 April 2018 at 04:02, Jae Hyun Yoo wrote: > This commit adds PECI adapter driver implementation for Aspeed > AST24xx/AST25xx. The driver is looking good! It looks like you've done some kind of review that we weren't allowed to see, which is a double edged sword - I might be asking about things that you've already spoken about with someone else. I'm only just learning about PECI, but I do have some general comments below. > --- > drivers/peci/Kconfig | 28 +++ > drivers/peci/Makefile | 3 + > drivers/peci/peci-aspeed.c | 504 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 535 insertions(+) > create mode 100644 drivers/peci/peci-aspeed.c > > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig > index 1fbc13f9e6c2..0e33420365de 100644 > --- a/drivers/peci/Kconfig > +++ b/drivers/peci/Kconfig > @@ -14,4 +14,32 @@ config PECI > processors and chipset components to external monitoring or control > devices. > > + If you want PECI support, you should say Y here and also to the > + specific driver for your bus adapter(s) below. > + > +if PECI > + > +# > +# PECI hardware bus configuration > +# > + > +menu "PECI Hardware Bus support" > + > +config PECI_ASPEED > + tristate "Aspeed AST24xx/AST25xx PECI support" I think just saying ASPEED PECI support is enough. That way if the next ASPEED SoC happens to have PECI we don't need to update all of the help text :) > + select REGMAP_MMIO > + depends on OF > + depends on ARCH_ASPEED || COMPILE_TEST > + help > + Say Y here if you want support for the Platform Environment Control > + Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX > + SoCs. > + > + This support is also available as a module. If so, the module > + will be called peci-aspeed. > + > +endmenu > + > +endif # PECI > + > endmenu > diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile > index 9e8615e0d3ff..886285e69765 100644 > --- a/drivers/peci/Makefile > +++ b/drivers/peci/Makefile > @@ -4,3 +4,6 @@ > > # Core functionality > obj-$(CONFIG_PECI) += peci-core.o > + > +# Hardware specific bus drivers > +obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o > diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c > new file mode 100644 > index 000000000000..be2a1f327eb1 > --- /dev/null > +++ b/drivers/peci/peci-aspeed.c > @@ -0,0 +1,504 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2012-2017 ASPEED Technology Inc. > +// Copyright (c) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DUMP_DEBUG 0 > + > +/* Aspeed PECI Registers */ > +#define AST_PECI_CTRL 0x00 Nit: we use ASPEED instead of AST in the upstream kernel to distingush from the aspeed sdk drivers. If you feel strongly about this then I won't insist you change. > +#define AST_PECI_TIMING 0x04 > +#define AST_PECI_CMD 0x08 > +#define AST_PECI_CMD_CTRL 0x0c > +#define AST_PECI_EXP_FCS 0x10 > +#define AST_PECI_CAP_FCS 0x14 > +#define AST_PECI_INT_CTRL 0x18 > +#define AST_PECI_INT_STS 0x1c > +#define AST_PECI_W_DATA0 0x20 > +#define AST_PECI_W_DATA1 0x24 > +#define AST_PECI_W_DATA2 0x28 > +#define AST_PECI_W_DATA3 0x2c > +#define AST_PECI_R_DATA0 0x30 > +#define AST_PECI_R_DATA1 0x34 > +#define AST_PECI_R_DATA2 0x38 > +#define AST_PECI_R_DATA3 0x3c > +#define AST_PECI_W_DATA4 0x40 > +#define AST_PECI_W_DATA5 0x44 > +#define AST_PECI_W_DATA6 0x48 > +#define AST_PECI_W_DATA7 0x4c > +#define AST_PECI_R_DATA4 0x50 > +#define AST_PECI_R_DATA5 0x54 > +#define AST_PECI_R_DATA6 0x58 > +#define AST_PECI_R_DATA7 0x5c > + > +/* AST_PECI_CTRL - 0x00 : Control Register */ > +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) > +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) > +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) > +#define PECI_CTRL_READ_MODE_MASK GENMASK(13, 12) > +#define PECI_CTRL_READ_MODE(x) (((x) << 12) & PECI_CTRL_READ_MODE_MASK) > +#define PECI_CTRL_READ_MODE_GET(x) (((x) & PECI_CTRL_READ_MODE_MASK) >> 12) > +#define PECI_CTRL_READ_MODE_COUNT BIT(12) > +#define PECI_CTRL_READ_MODE_DBG BIT(13) > +#define PECI_CTRL_CLK_SOURCE_MASK BIT(11) > +#define PECI_CTRL_CLK_SOURCE(x) (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK) > +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11) > +#define PECI_CTRL_CLK_DIV_MASK GENMASK(10, 8) > +#define PECI_CTRL_CLK_DIV(x) (((x) << 8) & PECI_CTRL_CLK_DIV_MASK) > +#define PECI_CTRL_CLK_DIV_GET(x) (((x) & PECI_CTRL_CLK_DIV_MASK) >> 8) > +#define PECI_CTRL_INVERT_OUT BIT(7) > +#define PECI_CTRL_INVERT_IN BIT(6) > +#define PECI_CTRL_BUS_CONTENT_EN BIT(5) > +#define PECI_CTRL_PECI_EN BIT(4) > +#define PECI_CTRL_PECI_CLK_EN BIT(0) I know these come from the ASPEED sdk driver. Do we need them all? > + > +/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */ > +#define PECI_TIMING_MESSAGE_MASK GENMASK(15, 8) > +#define PECI_TIMING_MESSAGE(x) (((x) << 8) & PECI_TIMING_MESSAGE_MASK) > +#define PECI_TIMING_MESSAGE_GET(x) (((x) & PECI_TIMING_MESSAGE_MASK) >> 8) > +#define PECI_TIMING_ADDRESS_MASK GENMASK(7, 0) > +#define PECI_TIMING_ADDRESS(x) ((x) & PECI_TIMING_ADDRESS_MASK) > +#define PECI_TIMING_ADDRESS_GET(x) ((x) & PECI_TIMING_ADDRESS_MASK) > + > +/* AST_PECI_CMD - 0x08 : Command Register */ > +#define PECI_CMD_PIN_MON BIT(31) > +#define PECI_CMD_STS_MASK GENMASK(27, 24) > +#define PECI_CMD_STS_GET(x) (((x) & PECI_CMD_STS_MASK) >> 24) > +#define PECI_CMD_FIRE BIT(0) > + > +/* AST_PECI_LEN - 0x0C : Read/Write Length Register */ > +#define PECI_AW_FCS_EN BIT(31) > +#define PECI_READ_LEN_MASK GENMASK(23, 16) > +#define PECI_READ_LEN(x) (((x) << 16) & PECI_READ_LEN_MASK) > +#define PECI_WRITE_LEN_MASK GENMASK(15, 8) > +#define PECI_WRITE_LEN(x) (((x) << 8) & PECI_WRITE_LEN_MASK) > +#define PECI_TAGET_ADDR_MASK GENMASK(7, 0) > +#define PECI_TAGET_ADDR(x) ((x) & PECI_TAGET_ADDR_MASK) > + > +/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register */ > +#define PECI_EXPECT_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_EXPECT_READ_FCS_GET(x) (((x) & PECI_EXPECT_READ_FCS_MASK) >> 16) > +#define PECI_EXPECT_AW_FCS_AUTO_MASK GENMASK(15, 8) > +#define PECI_EXPECT_AW_FCS_AUTO_GET(x) (((x) & PECI_EXPECT_AW_FCS_AUTO_MASK) \ > + >> 8) > +#define PECI_EXPECT_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_EXPECT_WRITE_FCS_GET(x) ((x) & PECI_EXPECT_WRITE_FCS_MASK) > + > +/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */ > +#define PECI_CAPTURE_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_CAPTURE_READ_FCS_GET(x) (((x) & PECI_CAPTURE_READ_FCS_MASK) >> 16) > +#define PECI_CAPTURE_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_CAPTURE_WRITE_FCS_GET(x) ((x) & PECI_CAPTURE_WRITE_FCS_MASK) > + > +/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */ > +#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30) > +#define PECI_INT_TIMEOUT BIT(4) > +#define PECI_INT_CONNECT BIT(3) > +#define PECI_INT_W_FCS_BAD BIT(2) > +#define PECI_INT_W_FCS_ABORT BIT(1) > +#define PECI_INT_CMD_DONE BIT(0) > + > +struct aspeed_peci { > + struct peci_adapter adaper; > + struct device *dev; > + struct regmap *regmap; > + int irq; > + struct completion xfer_complete; > + u32 status; > + u32 cmd_timeout_ms; > +}; > + > +#define PECI_INT_MASK (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \ > + PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \ > + PECI_INT_CMD_DONE) > + > +#define PECI_IDLE_CHECK_TIMEOUT_MS 50 > +#define PECI_IDLE_CHECK_INTERVAL_MS 10 > + > +#define PECI_RD_SAMPLING_POINT_DEFAULT 8 > +#define PECI_RD_SAMPLING_POINT_MAX 15 > +#define PECI_CLK_DIV_DEFAULT 0 > +#define PECI_CLK_DIV_MAX 7 > +#define PECI_MSG_TIMING_NEGO_DEFAULT 1 > +#define PECI_MSG_TIMING_NEGO_MAX 255 > +#define PECI_ADDR_TIMING_NEGO_DEFAULT 1 > +#define PECI_ADDR_TIMING_NEGO_MAX 255 > +#define PECI_CMD_TIMEOUT_MS_DEFAULT 1000 > +#define PECI_CMD_TIMEOUT_MS_MAX 60000 > + > +static int aspeed_peci_xfer_native(struct aspeed_peci *priv, > + struct peci_xfer_msg *msg) > +{ > + long err, timeout = msecs_to_jiffies(priv->cmd_timeout_ms); > + u32 peci_head, peci_state, rx_data, cmd_sts; > + ktime_t start, end; > + s64 elapsed_ms; > + int i, rc = 0; > + uint reg; > + > + start = ktime_get(); > + > + /* Check command sts and bus idle state */ > + while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts) && > + (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) { > + end = ktime_get(); > + elapsed_ms = ktime_to_ms(ktime_sub(end, start)); > + if (elapsed_ms >= PECI_IDLE_CHECK_TIMEOUT_MS) { > + dev_dbg(priv->dev, "Timeout waiting for idle state!\n"); > + return -ETIMEDOUT; > + } > + > + usleep_range(PECI_IDLE_CHECK_INTERVAL_MS * 1000, > + (PECI_IDLE_CHECK_INTERVAL_MS * 1000) + 1000); > + }; Could the above use regmap_read_poll_timeout instead? > + > + reinit_completion(&priv->xfer_complete); > + > + peci_head = PECI_TAGET_ADDR(msg->addr) | > + PECI_WRITE_LEN(msg->tx_len) | > + PECI_READ_LEN(msg->rx_len); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head); > + if (rc) > + return rc; > + > + for (i = 0; i < msg->tx_len; i += 4) { > + reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 : > + AST_PECI_W_DATA4 + i % 16; > + rc = regmap_write(priv->regmap, reg, > + (msg->tx_buf[i + 3] << 24) | > + (msg->tx_buf[i + 2] << 16) | > + (msg->tx_buf[i + 1] << 8) | > + msg->tx_buf[i + 0]); That looks like an endian swap. Can we do something like this? regmap_write(map, reg, cpu_to_be32p((void *)msg->tx_buff)) > + if (rc) > + return rc; > + } > + > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > +#if DUMP_DEBUG Having #defines is frowned upon. I think print_hex_dump_debug will do what you want here. > + print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->tx_buf, msg->tx_len, true); > +#endif > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE); > + if (rc) > + return rc; > + > + err = wait_for_completion_interruptible_timeout(&priv->xfer_complete, > + timeout); > + > + dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->status); > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, 0); > + if (rc) > + return rc; > + > + if (err <= 0 || !(priv->status & PECI_INT_CMD_DONE)) { > + if (err < 0) { /* -ERESTARTSYS */ > + return (int)err; > + } else if (err == 0) { > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > + return -ETIMEDOUT; > + } > + > + dev_dbg(priv->dev, "No valid response!\n"); > + return -EIO; > + } > + > + for (i = 0; i < msg->rx_len; i++) { > + u8 byte_offset = i % 4; > + > + if (byte_offset == 0) { > + reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 : > + AST_PECI_R_DATA4 + i % 16; I find this hard to read. Use a few more lines to make it clear what your code is doing. Actually, the entire for loop is cryptic. I understand what it's doing now. Can you rework it to make it more readable? You follow a similar pattern above in the write case. > + rc = regmap_read(priv->regmap, reg, &rx_data); > + if (rc) > + return rc; > + } > + > + msg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3)) > + } > + > +#if DUMP_DEBUG > + print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->rx_buf, msg->rx_len, true); > +#endif > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); Given the regmap_read is always going to be a memory read on the aspeed, I can't think of a situation where the read will fail. On that note, is there a reason you are using regmap and not just accessing the hardware directly? regmap imposes a number of pointer lookups and tests each time you do a read or write. > + dev_dbg(priv->dev, "------------------------\n"); > + > + return rc; > +} > + > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > +{ > + struct aspeed_peci *priv = arg; > + u32 status_ack = 0; > + > + if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->status)) > + return IRQ_NONE; Again, a memory mapped read won't fail. How about we check that the regmap is working once in your _probe() function, and assume it will continue working from there (or remove the regmap abstraction all together). > + > + /* Be noted that multiple interrupt bits can be set at the same time */ > + if (priv->status & PECI_INT_TIMEOUT) { > + dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n"); > + status_ack |= PECI_INT_TIMEOUT; > + } > + > + if (priv->status & PECI_INT_CONNECT) { > + dev_dbg(priv->dev, "PECI_INT_CONNECT\n"); > + status_ack |= PECI_INT_CONNECT; > + } > + > + if (priv->status & PECI_INT_W_FCS_BAD) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n"); > + status_ack |= PECI_INT_W_FCS_BAD; > + } > + > + if (priv->status & PECI_INT_W_FCS_ABORT) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n"); > + status_ack |= PECI_INT_W_FCS_ABORT; > + } All of this code is for debugging only. Do you want to put it behind some kind of conditional? > + > + /** > + * All commands should be ended up with a PECI_INT_CMD_DONE bit set > + * even in an error case. > + */ > + if (priv->status & PECI_INT_CMD_DONE) { > + dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n"); > + status_ack |= PECI_INT_CMD_DONE; > + complete(&priv->xfer_complete); > + } > + > + if (regmap_write(priv->regmap, AST_PECI_INT_STS, status_ack)) > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} > + > +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv) > +{ > + u32 msg_timing_nego, addr_timing_nego, rd_sampling_point; > + u32 clk_freq, clk_divisor, clk_div_val = 0; > + struct clk *clkin; > + int ret; > + > + clkin = devm_clk_get(priv->dev, NULL); > + if (IS_ERR(clkin)) { > + dev_err(priv->dev, "Failed to get clk source.\n"); > + return PTR_ERR(clkin); > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "clock-frequency", > + &clk_freq); > + if (ret < 0) { > + dev_err(priv->dev, > + "Could not read clock-frequency property.\n"); > + return ret; > + } > + > + clk_divisor = clk_get_rate(clkin) / clk_freq; > + devm_clk_put(priv->dev, clkin); > + > + while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX)) > + clk_div_val++; We have a framework for doing clocks in the kernel. Would it make sense to write a driver for this clock and add it to drivers/clk/clk-aspeed.c? > + > + ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego", > + &msg_timing_nego); > + if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid msg-timing-nego : %u, Use default : %u\n", > + msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT); The property is optional so I suggest we don't print a message if it's not present. We certainly don't want to print a message saying "invalid". The same comment applies to the other optional properties below. > + msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego", > + &addr_timing_nego); > + if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid addr-timing-nego : %u, Use default : %u\n", > + addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT); > + addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point", > + &rd_sampling_point); > + if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) { > + dev_warn(priv->dev, > + "Invalid rd-sampling-point : %u. Use default : %u\n", > + rd_sampling_point, > + PECI_RD_SAMPLING_POINT_DEFAULT); > + rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms", > + &priv->cmd_timeout_ms); > + if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX || > + priv->cmd_timeout_ms == 0) { > + dev_warn(priv->dev, > + "Invalid cmd-timeout-ms : %u. Use default : %u\n", > + priv->cmd_timeout_ms, > + PECI_CMD_TIMEOUT_MS_DEFAULT); > + priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT; > + } > + > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) | > + PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + usleep_range(1000, 5000); Can we probe in parallel? If not, putting a sleep in the _probe will hold up the rest of drivers from being able to do anything, and hold up boot. If you decide that you do need to probe here, please add a comment. (This is the wait for the clock to be stable?) > + > + /** > + * Timing negotiation period setting. > + * The unit of the programmed value is 4 times of PECI clock period. > + */ > + ret = regmap_write(priv->regmap, AST_PECI_TIMING, > + PECI_TIMING_MESSAGE(msg_timing_nego) | > + PECI_TIMING_ADDRESS(addr_timing_nego)); > + if (ret) > + return ret; > + > + /* Clear interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Enable interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Read sampling point and clock speed setting */ > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_SAMPLING(rd_sampling_point) | > + PECI_CTRL_CLK_DIV(clk_div_val) | > + PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static const struct regmap_config aspeed_peci_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = AST_PECI_R_DATA7, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > + .fast_io = true, > +}; > + > +static int aspeed_peci_xfer(struct peci_adapter *adaper, > + struct peci_xfer_msg *msg) > +{ > + struct aspeed_peci *priv = peci_get_adapdata(adaper); > + > + return aspeed_peci_xfer_native(priv, msg); > +} > + > +static int aspeed_peci_probe(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv; > + struct resource *res; > + void __iomem *base; > + int ret = 0; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + dev_set_drvdata(&pdev->dev, priv); > + priv->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &aspeed_peci_regmap_config); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->irq = platform_get_irq(pdev, 0); > + if (!priv->irq) > + return -ENODEV; > + > + ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler, > + IRQF_SHARED, This interrupt is only for the peci device. Why is it marked as shared? > + "peci-aspeed-irq", > + priv); > + if (ret < 0) > + return ret; > + > + init_completion(&priv->xfer_complete); > + > + priv->adaper.dev.parent = priv->dev; > + priv->adaper.dev.of_node = of_node_get(dev_of_node(priv->dev)); > + strlcpy(priv->adaper.name, pdev->name, sizeof(priv->adaper.name)); > + priv->adaper.xfer = aspeed_peci_xfer; > + peci_set_adapdata(&priv->adaper, priv); > + > + ret = aspeed_peci_init_ctrl(priv); > + if (ret < 0) > + return ret; > + > + ret = peci_add_adapter(&priv->adaper); > + if (ret < 0) > + return ret; > + > + dev_info(&pdev->dev, "peci bus %d registered, irq %d\n", > + priv->adaper.nr, priv->irq); > + > + return 0; > +} > + > +static int aspeed_peci_remove(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev); > + > + peci_del_adapter(&priv->adaper); > + of_node_put(priv->adaper.dev.of_node); > + > + return 0; > +} > + > +static const struct of_device_id aspeed_peci_of_table[] = { > + { .compatible = "aspeed,ast2400-peci", }, > + { .compatible = "aspeed,ast2500-peci", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table); > + > +static struct platform_driver aspeed_peci_driver = { > + .probe = aspeed_peci_probe, > + .remove = aspeed_peci_remove, > + .driver = { > + .name = "peci-aspeed", > + .of_match_table = of_match_ptr(aspeed_peci_of_table), > + }, > +}; > +module_platform_driver(aspeed_peci_driver); > + > +MODULE_AUTHOR("Ryan Chen "); > +MODULE_AUTHOR("Jae Hyun Yoo "); > +MODULE_DESCRIPTION("Aspeed PECI driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.16.2 > -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gmail.com (client-ip=2607:f8b0:400d:c09::242; helo=mail-qk0-x242.google.com; envelope-from=joel.stan@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=jms.id.au Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="r3wSV/bt"; dkim=pass (1024-bit key; secure) header.d=jms.id.au header.i=@jms.id.au header.b="VB6RNkzq"; dkim-atps=neutral Received: from mail-qk0-x242.google.com (mail-qk0-x242.google.com [IPv6:2607:f8b0:400d:c09::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 40Lj7p7575zF0QP for ; Wed, 11 Apr 2018 21:52:06 +1000 (AEST) Received: by mail-qk0-x242.google.com with SMTP id c188so1468299qkg.2 for ; Wed, 11 Apr 2018 04:52:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=kZWqitmvM9JWSCtKTw2cMLrBV0xAcx8l9wae3/4Rs5s=; b=r3wSV/btVk8DZBNa3qYLcNgwUZby6d9Id4Mo6NNkUdrknOdVKRfx2K72xkNgW3ba3l QWT1Ox5bZrjDvw7G1AN7Q9drqjEAIQfFZVEDRRInBztaU8XUTMZrQFALAlSykV+82tsv xRVJIrFmzqx2Q+VA2iwzvA806JdtK7IqkifqIPbZ0nJw8ey/rlG4CT9HjVlHqkuMEDL1 KItBevC26bYdeSLXwOGT+W5HSW3X76Md/btzleQVLu6cYV2U62tSwXI7rSlktOJtQtmP IRhgogHM8NLopn2BPyLbNLfTSJ2H2RDMj3qpRjig0R3y1BakFEj5VuUoDEr/+SyfYUCP wI3A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jms.id.au; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=kZWqitmvM9JWSCtKTw2cMLrBV0xAcx8l9wae3/4Rs5s=; b=VB6RNkzq2oO7cJVJV60ZiPId7SLRWj0LH0Zvx/FxHRoJ1zBIzOycJ5yV+mjJnwH+CB cxBhvC3rdLKlvRjVVjOMrzyiwZYQce7bztGss4Tl8J/93KURid6WCrzpeYsToGsKhLAK gF3hnU1U8oyyeFtxiN6MT8s1PPx8T+gjiXYkI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=kZWqitmvM9JWSCtKTw2cMLrBV0xAcx8l9wae3/4Rs5s=; b=sb9I2BeYB2AHWPXB7UNVLR3YmFdAh626VtVPHTwxvvRF4TVM+w0jJzUPx5uSH9sh22 +p2nkiYb8UL1L+9Ow+PFZffSoMJkJgLQ1DyHrTnwSYdj8fYChqznsdnjdPP9E7rdnKiB vzQaBkWLopbof2VVgyB4JrzquZME1Z17q2V7tvdO910HYOKaQka/Rb2ZTqMKSV55b3Pf XgZxJKzvwpt++DUc/9YVfyJ6hCf0SQZoAn+5ZL9wcRhfSQeCyPf5KPhTdsadKIMqbyIn aFxF3zeAcOBg294aLaW6IgjA+menze+PRf8FGpLuIfEehTuhBmwDbKH6JVucF0816TV0 kLqA== X-Gm-Message-State: ALQs6tDy1kskRdJrPEt+gXltStC9ymCkdiV50sETakjCQjS0jZaeIQfp UV7HZqMZ59kVMT4qJ0Da36ZB5V+W1t0MmvJLcf0= X-Google-Smtp-Source: AIpwx4/0A3vCWCULvCHNwepIwih56YgyGt6yey85+S8YFYptMGg5sYI2Bbrw10l9080H/ifXYNnTMqSTUC3KOGcIYxY= X-Received: by 10.55.44.3 with SMTP id s3mr6220350qkh.231.1523447523686; Wed, 11 Apr 2018 04:52:03 -0700 (PDT) MIME-Version: 1.0 Sender: joel.stan@gmail.com Received: by 10.200.63.197 with HTTP; Wed, 11 Apr 2018 04:51:43 -0700 (PDT) In-Reply-To: <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> References: <20180410183212.16787-1-jae.hyun.yoo@linux.intel.com> <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> From: Joel Stanley Date: Wed, 11 Apr 2018 21:21:43 +0930 X-Google-Sender-Auth: KFsfZFM82sqWoHa4miFWFLuxNJk Message-ID: Subject: Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx To: Jae Hyun Yoo , Ryan Chen Cc: Alan Cox , Andrew Jeffery , Andrew Lunn , Andy Shevchenko , Arnd Bergmann , Benjamin Herrenschmidt , Fengguang Wu , Greg KH , Guenter Roeck , Haiyue Wang , James Feist , Jason M Biils , Jean Delvare , Julia Cartwright , Miguel Ojeda , Milton Miller II , Pavel Machek , Randy Dunlap , Stef van Os , Sumeet R Pawnikar , Vernon Mauery , Linux Kernel Mailing List , linux-doc@vger.kernel.org, devicetree , linux-hwmon@vger.kernel.org, Linux ARM , OpenBMC Maillist Content-Type: text/plain; charset="UTF-8" X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Apr 2018 11:52:10 -0000 Hello Jae, On 11 April 2018 at 04:02, Jae Hyun Yoo wrote: > This commit adds PECI adapter driver implementation for Aspeed > AST24xx/AST25xx. The driver is looking good! It looks like you've done some kind of review that we weren't allowed to see, which is a double edged sword - I might be asking about things that you've already spoken about with someone else. I'm only just learning about PECI, but I do have some general comments below. > --- > drivers/peci/Kconfig | 28 +++ > drivers/peci/Makefile | 3 + > drivers/peci/peci-aspeed.c | 504 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 535 insertions(+) > create mode 100644 drivers/peci/peci-aspeed.c > > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig > index 1fbc13f9e6c2..0e33420365de 100644 > --- a/drivers/peci/Kconfig > +++ b/drivers/peci/Kconfig > @@ -14,4 +14,32 @@ config PECI > processors and chipset components to external monitoring or control > devices. > > + If you want PECI support, you should say Y here and also to the > + specific driver for your bus adapter(s) below. > + > +if PECI > + > +# > +# PECI hardware bus configuration > +# > + > +menu "PECI Hardware Bus support" > + > +config PECI_ASPEED > + tristate "Aspeed AST24xx/AST25xx PECI support" I think just saying ASPEED PECI support is enough. That way if the next ASPEED SoC happens to have PECI we don't need to update all of the help text :) > + select REGMAP_MMIO > + depends on OF > + depends on ARCH_ASPEED || COMPILE_TEST > + help > + Say Y here if you want support for the Platform Environment Control > + Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX > + SoCs. > + > + This support is also available as a module. If so, the module > + will be called peci-aspeed. > + > +endmenu > + > +endif # PECI > + > endmenu > diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile > index 9e8615e0d3ff..886285e69765 100644 > --- a/drivers/peci/Makefile > +++ b/drivers/peci/Makefile > @@ -4,3 +4,6 @@ > > # Core functionality > obj-$(CONFIG_PECI) += peci-core.o > + > +# Hardware specific bus drivers > +obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o > diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c > new file mode 100644 > index 000000000000..be2a1f327eb1 > --- /dev/null > +++ b/drivers/peci/peci-aspeed.c > @@ -0,0 +1,504 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2012-2017 ASPEED Technology Inc. > +// Copyright (c) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DUMP_DEBUG 0 > + > +/* Aspeed PECI Registers */ > +#define AST_PECI_CTRL 0x00 Nit: we use ASPEED instead of AST in the upstream kernel to distingush from the aspeed sdk drivers. If you feel strongly about this then I won't insist you change. > +#define AST_PECI_TIMING 0x04 > +#define AST_PECI_CMD 0x08 > +#define AST_PECI_CMD_CTRL 0x0c > +#define AST_PECI_EXP_FCS 0x10 > +#define AST_PECI_CAP_FCS 0x14 > +#define AST_PECI_INT_CTRL 0x18 > +#define AST_PECI_INT_STS 0x1c > +#define AST_PECI_W_DATA0 0x20 > +#define AST_PECI_W_DATA1 0x24 > +#define AST_PECI_W_DATA2 0x28 > +#define AST_PECI_W_DATA3 0x2c > +#define AST_PECI_R_DATA0 0x30 > +#define AST_PECI_R_DATA1 0x34 > +#define AST_PECI_R_DATA2 0x38 > +#define AST_PECI_R_DATA3 0x3c > +#define AST_PECI_W_DATA4 0x40 > +#define AST_PECI_W_DATA5 0x44 > +#define AST_PECI_W_DATA6 0x48 > +#define AST_PECI_W_DATA7 0x4c > +#define AST_PECI_R_DATA4 0x50 > +#define AST_PECI_R_DATA5 0x54 > +#define AST_PECI_R_DATA6 0x58 > +#define AST_PECI_R_DATA7 0x5c > + > +/* AST_PECI_CTRL - 0x00 : Control Register */ > +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) > +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) > +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) > +#define PECI_CTRL_READ_MODE_MASK GENMASK(13, 12) > +#define PECI_CTRL_READ_MODE(x) (((x) << 12) & PECI_CTRL_READ_MODE_MASK) > +#define PECI_CTRL_READ_MODE_GET(x) (((x) & PECI_CTRL_READ_MODE_MASK) >> 12) > +#define PECI_CTRL_READ_MODE_COUNT BIT(12) > +#define PECI_CTRL_READ_MODE_DBG BIT(13) > +#define PECI_CTRL_CLK_SOURCE_MASK BIT(11) > +#define PECI_CTRL_CLK_SOURCE(x) (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK) > +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11) > +#define PECI_CTRL_CLK_DIV_MASK GENMASK(10, 8) > +#define PECI_CTRL_CLK_DIV(x) (((x) << 8) & PECI_CTRL_CLK_DIV_MASK) > +#define PECI_CTRL_CLK_DIV_GET(x) (((x) & PECI_CTRL_CLK_DIV_MASK) >> 8) > +#define PECI_CTRL_INVERT_OUT BIT(7) > +#define PECI_CTRL_INVERT_IN BIT(6) > +#define PECI_CTRL_BUS_CONTENT_EN BIT(5) > +#define PECI_CTRL_PECI_EN BIT(4) > +#define PECI_CTRL_PECI_CLK_EN BIT(0) I know these come from the ASPEED sdk driver. Do we need them all? > + > +/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */ > +#define PECI_TIMING_MESSAGE_MASK GENMASK(15, 8) > +#define PECI_TIMING_MESSAGE(x) (((x) << 8) & PECI_TIMING_MESSAGE_MASK) > +#define PECI_TIMING_MESSAGE_GET(x) (((x) & PECI_TIMING_MESSAGE_MASK) >> 8) > +#define PECI_TIMING_ADDRESS_MASK GENMASK(7, 0) > +#define PECI_TIMING_ADDRESS(x) ((x) & PECI_TIMING_ADDRESS_MASK) > +#define PECI_TIMING_ADDRESS_GET(x) ((x) & PECI_TIMING_ADDRESS_MASK) > + > +/* AST_PECI_CMD - 0x08 : Command Register */ > +#define PECI_CMD_PIN_MON BIT(31) > +#define PECI_CMD_STS_MASK GENMASK(27, 24) > +#define PECI_CMD_STS_GET(x) (((x) & PECI_CMD_STS_MASK) >> 24) > +#define PECI_CMD_FIRE BIT(0) > + > +/* AST_PECI_LEN - 0x0C : Read/Write Length Register */ > +#define PECI_AW_FCS_EN BIT(31) > +#define PECI_READ_LEN_MASK GENMASK(23, 16) > +#define PECI_READ_LEN(x) (((x) << 16) & PECI_READ_LEN_MASK) > +#define PECI_WRITE_LEN_MASK GENMASK(15, 8) > +#define PECI_WRITE_LEN(x) (((x) << 8) & PECI_WRITE_LEN_MASK) > +#define PECI_TAGET_ADDR_MASK GENMASK(7, 0) > +#define PECI_TAGET_ADDR(x) ((x) & PECI_TAGET_ADDR_MASK) > + > +/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register */ > +#define PECI_EXPECT_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_EXPECT_READ_FCS_GET(x) (((x) & PECI_EXPECT_READ_FCS_MASK) >> 16) > +#define PECI_EXPECT_AW_FCS_AUTO_MASK GENMASK(15, 8) > +#define PECI_EXPECT_AW_FCS_AUTO_GET(x) (((x) & PECI_EXPECT_AW_FCS_AUTO_MASK) \ > + >> 8) > +#define PECI_EXPECT_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_EXPECT_WRITE_FCS_GET(x) ((x) & PECI_EXPECT_WRITE_FCS_MASK) > + > +/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */ > +#define PECI_CAPTURE_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_CAPTURE_READ_FCS_GET(x) (((x) & PECI_CAPTURE_READ_FCS_MASK) >> 16) > +#define PECI_CAPTURE_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_CAPTURE_WRITE_FCS_GET(x) ((x) & PECI_CAPTURE_WRITE_FCS_MASK) > + > +/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */ > +#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30) > +#define PECI_INT_TIMEOUT BIT(4) > +#define PECI_INT_CONNECT BIT(3) > +#define PECI_INT_W_FCS_BAD BIT(2) > +#define PECI_INT_W_FCS_ABORT BIT(1) > +#define PECI_INT_CMD_DONE BIT(0) > + > +struct aspeed_peci { > + struct peci_adapter adaper; > + struct device *dev; > + struct regmap *regmap; > + int irq; > + struct completion xfer_complete; > + u32 status; > + u32 cmd_timeout_ms; > +}; > + > +#define PECI_INT_MASK (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \ > + PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \ > + PECI_INT_CMD_DONE) > + > +#define PECI_IDLE_CHECK_TIMEOUT_MS 50 > +#define PECI_IDLE_CHECK_INTERVAL_MS 10 > + > +#define PECI_RD_SAMPLING_POINT_DEFAULT 8 > +#define PECI_RD_SAMPLING_POINT_MAX 15 > +#define PECI_CLK_DIV_DEFAULT 0 > +#define PECI_CLK_DIV_MAX 7 > +#define PECI_MSG_TIMING_NEGO_DEFAULT 1 > +#define PECI_MSG_TIMING_NEGO_MAX 255 > +#define PECI_ADDR_TIMING_NEGO_DEFAULT 1 > +#define PECI_ADDR_TIMING_NEGO_MAX 255 > +#define PECI_CMD_TIMEOUT_MS_DEFAULT 1000 > +#define PECI_CMD_TIMEOUT_MS_MAX 60000 > + > +static int aspeed_peci_xfer_native(struct aspeed_peci *priv, > + struct peci_xfer_msg *msg) > +{ > + long err, timeout = msecs_to_jiffies(priv->cmd_timeout_ms); > + u32 peci_head, peci_state, rx_data, cmd_sts; > + ktime_t start, end; > + s64 elapsed_ms; > + int i, rc = 0; > + uint reg; > + > + start = ktime_get(); > + > + /* Check command sts and bus idle state */ > + while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts) && > + (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) { > + end = ktime_get(); > + elapsed_ms = ktime_to_ms(ktime_sub(end, start)); > + if (elapsed_ms >= PECI_IDLE_CHECK_TIMEOUT_MS) { > + dev_dbg(priv->dev, "Timeout waiting for idle state!\n"); > + return -ETIMEDOUT; > + } > + > + usleep_range(PECI_IDLE_CHECK_INTERVAL_MS * 1000, > + (PECI_IDLE_CHECK_INTERVAL_MS * 1000) + 1000); > + }; Could the above use regmap_read_poll_timeout instead? > + > + reinit_completion(&priv->xfer_complete); > + > + peci_head = PECI_TAGET_ADDR(msg->addr) | > + PECI_WRITE_LEN(msg->tx_len) | > + PECI_READ_LEN(msg->rx_len); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head); > + if (rc) > + return rc; > + > + for (i = 0; i < msg->tx_len; i += 4) { > + reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 : > + AST_PECI_W_DATA4 + i % 16; > + rc = regmap_write(priv->regmap, reg, > + (msg->tx_buf[i + 3] << 24) | > + (msg->tx_buf[i + 2] << 16) | > + (msg->tx_buf[i + 1] << 8) | > + msg->tx_buf[i + 0]); That looks like an endian swap. Can we do something like this? regmap_write(map, reg, cpu_to_be32p((void *)msg->tx_buff)) > + if (rc) > + return rc; > + } > + > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > +#if DUMP_DEBUG Having #defines is frowned upon. I think print_hex_dump_debug will do what you want here. > + print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->tx_buf, msg->tx_len, true); > +#endif > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE); > + if (rc) > + return rc; > + > + err = wait_for_completion_interruptible_timeout(&priv->xfer_complete, > + timeout); > + > + dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->status); > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, 0); > + if (rc) > + return rc; > + > + if (err <= 0 || !(priv->status & PECI_INT_CMD_DONE)) { > + if (err < 0) { /* -ERESTARTSYS */ > + return (int)err; > + } else if (err == 0) { > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > + return -ETIMEDOUT; > + } > + > + dev_dbg(priv->dev, "No valid response!\n"); > + return -EIO; > + } > + > + for (i = 0; i < msg->rx_len; i++) { > + u8 byte_offset = i % 4; > + > + if (byte_offset == 0) { > + reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 : > + AST_PECI_R_DATA4 + i % 16; I find this hard to read. Use a few more lines to make it clear what your code is doing. Actually, the entire for loop is cryptic. I understand what it's doing now. Can you rework it to make it more readable? You follow a similar pattern above in the write case. > + rc = regmap_read(priv->regmap, reg, &rx_data); > + if (rc) > + return rc; > + } > + > + msg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3)) > + } > + > +#if DUMP_DEBUG > + print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->rx_buf, msg->rx_len, true); > +#endif > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); Given the regmap_read is always going to be a memory read on the aspeed, I can't think of a situation where the read will fail. On that note, is there a reason you are using regmap and not just accessing the hardware directly? regmap imposes a number of pointer lookups and tests each time you do a read or write. > + dev_dbg(priv->dev, "------------------------\n"); > + > + return rc; > +} > + > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > +{ > + struct aspeed_peci *priv = arg; > + u32 status_ack = 0; > + > + if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->status)) > + return IRQ_NONE; Again, a memory mapped read won't fail. How about we check that the regmap is working once in your _probe() function, and assume it will continue working from there (or remove the regmap abstraction all together). > + > + /* Be noted that multiple interrupt bits can be set at the same time */ > + if (priv->status & PECI_INT_TIMEOUT) { > + dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n"); > + status_ack |= PECI_INT_TIMEOUT; > + } > + > + if (priv->status & PECI_INT_CONNECT) { > + dev_dbg(priv->dev, "PECI_INT_CONNECT\n"); > + status_ack |= PECI_INT_CONNECT; > + } > + > + if (priv->status & PECI_INT_W_FCS_BAD) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n"); > + status_ack |= PECI_INT_W_FCS_BAD; > + } > + > + if (priv->status & PECI_INT_W_FCS_ABORT) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n"); > + status_ack |= PECI_INT_W_FCS_ABORT; > + } All of this code is for debugging only. Do you want to put it behind some kind of conditional? > + > + /** > + * All commands should be ended up with a PECI_INT_CMD_DONE bit set > + * even in an error case. > + */ > + if (priv->status & PECI_INT_CMD_DONE) { > + dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n"); > + status_ack |= PECI_INT_CMD_DONE; > + complete(&priv->xfer_complete); > + } > + > + if (regmap_write(priv->regmap, AST_PECI_INT_STS, status_ack)) > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} > + > +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv) > +{ > + u32 msg_timing_nego, addr_timing_nego, rd_sampling_point; > + u32 clk_freq, clk_divisor, clk_div_val = 0; > + struct clk *clkin; > + int ret; > + > + clkin = devm_clk_get(priv->dev, NULL); > + if (IS_ERR(clkin)) { > + dev_err(priv->dev, "Failed to get clk source.\n"); > + return PTR_ERR(clkin); > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "clock-frequency", > + &clk_freq); > + if (ret < 0) { > + dev_err(priv->dev, > + "Could not read clock-frequency property.\n"); > + return ret; > + } > + > + clk_divisor = clk_get_rate(clkin) / clk_freq; > + devm_clk_put(priv->dev, clkin); > + > + while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX)) > + clk_div_val++; We have a framework for doing clocks in the kernel. Would it make sense to write a driver for this clock and add it to drivers/clk/clk-aspeed.c? > + > + ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego", > + &msg_timing_nego); > + if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid msg-timing-nego : %u, Use default : %u\n", > + msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT); The property is optional so I suggest we don't print a message if it's not present. We certainly don't want to print a message saying "invalid". The same comment applies to the other optional properties below. > + msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego", > + &addr_timing_nego); > + if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid addr-timing-nego : %u, Use default : %u\n", > + addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT); > + addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point", > + &rd_sampling_point); > + if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) { > + dev_warn(priv->dev, > + "Invalid rd-sampling-point : %u. Use default : %u\n", > + rd_sampling_point, > + PECI_RD_SAMPLING_POINT_DEFAULT); > + rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms", > + &priv->cmd_timeout_ms); > + if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX || > + priv->cmd_timeout_ms == 0) { > + dev_warn(priv->dev, > + "Invalid cmd-timeout-ms : %u. Use default : %u\n", > + priv->cmd_timeout_ms, > + PECI_CMD_TIMEOUT_MS_DEFAULT); > + priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT; > + } > + > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) | > + PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + usleep_range(1000, 5000); Can we probe in parallel? If not, putting a sleep in the _probe will hold up the rest of drivers from being able to do anything, and hold up boot. If you decide that you do need to probe here, please add a comment. (This is the wait for the clock to be stable?) > + > + /** > + * Timing negotiation period setting. > + * The unit of the programmed value is 4 times of PECI clock period. > + */ > + ret = regmap_write(priv->regmap, AST_PECI_TIMING, > + PECI_TIMING_MESSAGE(msg_timing_nego) | > + PECI_TIMING_ADDRESS(addr_timing_nego)); > + if (ret) > + return ret; > + > + /* Clear interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Enable interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Read sampling point and clock speed setting */ > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_SAMPLING(rd_sampling_point) | > + PECI_CTRL_CLK_DIV(clk_div_val) | > + PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static const struct regmap_config aspeed_peci_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = AST_PECI_R_DATA7, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > + .fast_io = true, > +}; > + > +static int aspeed_peci_xfer(struct peci_adapter *adaper, > + struct peci_xfer_msg *msg) > +{ > + struct aspeed_peci *priv = peci_get_adapdata(adaper); > + > + return aspeed_peci_xfer_native(priv, msg); > +} > + > +static int aspeed_peci_probe(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv; > + struct resource *res; > + void __iomem *base; > + int ret = 0; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + dev_set_drvdata(&pdev->dev, priv); > + priv->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &aspeed_peci_regmap_config); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->irq = platform_get_irq(pdev, 0); > + if (!priv->irq) > + return -ENODEV; > + > + ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler, > + IRQF_SHARED, This interrupt is only for the peci device. Why is it marked as shared? > + "peci-aspeed-irq", > + priv); > + if (ret < 0) > + return ret; > + > + init_completion(&priv->xfer_complete); > + > + priv->adaper.dev.parent = priv->dev; > + priv->adaper.dev.of_node = of_node_get(dev_of_node(priv->dev)); > + strlcpy(priv->adaper.name, pdev->name, sizeof(priv->adaper.name)); > + priv->adaper.xfer = aspeed_peci_xfer; > + peci_set_adapdata(&priv->adaper, priv); > + > + ret = aspeed_peci_init_ctrl(priv); > + if (ret < 0) > + return ret; > + > + ret = peci_add_adapter(&priv->adaper); > + if (ret < 0) > + return ret; > + > + dev_info(&pdev->dev, "peci bus %d registered, irq %d\n", > + priv->adaper.nr, priv->irq); > + > + return 0; > +} > + > +static int aspeed_peci_remove(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev); > + > + peci_del_adapter(&priv->adaper); > + of_node_put(priv->adaper.dev.of_node); > + > + return 0; > +} > + > +static const struct of_device_id aspeed_peci_of_table[] = { > + { .compatible = "aspeed,ast2400-peci", }, > + { .compatible = "aspeed,ast2500-peci", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table); > + > +static struct platform_driver aspeed_peci_driver = { > + .probe = aspeed_peci_probe, > + .remove = aspeed_peci_remove, > + .driver = { > + .name = "peci-aspeed", > + .of_match_table = of_match_ptr(aspeed_peci_of_table), > + }, > +}; > +module_platform_driver(aspeed_peci_driver); > + > +MODULE_AUTHOR("Ryan Chen "); > +MODULE_AUTHOR("Jae Hyun Yoo "); > +MODULE_DESCRIPTION("Aspeed PECI driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.16.2 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: joel@jms.id.au (Joel Stanley) Date: Wed, 11 Apr 2018 21:21:43 +0930 Subject: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx In-Reply-To: <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> References: <20180410183212.16787-1-jae.hyun.yoo@linux.intel.com> <20180410183212.16787-7-jae.hyun.yoo@linux.intel.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello Jae, On 11 April 2018 at 04:02, Jae Hyun Yoo wrote: > This commit adds PECI adapter driver implementation for Aspeed > AST24xx/AST25xx. The driver is looking good! It looks like you've done some kind of review that we weren't allowed to see, which is a double edged sword - I might be asking about things that you've already spoken about with someone else. I'm only just learning about PECI, but I do have some general comments below. > --- > drivers/peci/Kconfig | 28 +++ > drivers/peci/Makefile | 3 + > drivers/peci/peci-aspeed.c | 504 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 535 insertions(+) > create mode 100644 drivers/peci/peci-aspeed.c > > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig > index 1fbc13f9e6c2..0e33420365de 100644 > --- a/drivers/peci/Kconfig > +++ b/drivers/peci/Kconfig > @@ -14,4 +14,32 @@ config PECI > processors and chipset components to external monitoring or control > devices. > > + If you want PECI support, you should say Y here and also to the > + specific driver for your bus adapter(s) below. > + > +if PECI > + > +# > +# PECI hardware bus configuration > +# > + > +menu "PECI Hardware Bus support" > + > +config PECI_ASPEED > + tristate "Aspeed AST24xx/AST25xx PECI support" I think just saying ASPEED PECI support is enough. That way if the next ASPEED SoC happens to have PECI we don't need to update all of the help text :) > + select REGMAP_MMIO > + depends on OF > + depends on ARCH_ASPEED || COMPILE_TEST > + help > + Say Y here if you want support for the Platform Environment Control > + Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX > + SoCs. > + > + This support is also available as a module. If so, the module > + will be called peci-aspeed. > + > +endmenu > + > +endif # PECI > + > endmenu > diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile > index 9e8615e0d3ff..886285e69765 100644 > --- a/drivers/peci/Makefile > +++ b/drivers/peci/Makefile > @@ -4,3 +4,6 @@ > > # Core functionality > obj-$(CONFIG_PECI) += peci-core.o > + > +# Hardware specific bus drivers > +obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o > diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c > new file mode 100644 > index 000000000000..be2a1f327eb1 > --- /dev/null > +++ b/drivers/peci/peci-aspeed.c > @@ -0,0 +1,504 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2012-2017 ASPEED Technology Inc. > +// Copyright (c) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DUMP_DEBUG 0 > + > +/* Aspeed PECI Registers */ > +#define AST_PECI_CTRL 0x00 Nit: we use ASPEED instead of AST in the upstream kernel to distingush from the aspeed sdk drivers. If you feel strongly about this then I won't insist you change. > +#define AST_PECI_TIMING 0x04 > +#define AST_PECI_CMD 0x08 > +#define AST_PECI_CMD_CTRL 0x0c > +#define AST_PECI_EXP_FCS 0x10 > +#define AST_PECI_CAP_FCS 0x14 > +#define AST_PECI_INT_CTRL 0x18 > +#define AST_PECI_INT_STS 0x1c > +#define AST_PECI_W_DATA0 0x20 > +#define AST_PECI_W_DATA1 0x24 > +#define AST_PECI_W_DATA2 0x28 > +#define AST_PECI_W_DATA3 0x2c > +#define AST_PECI_R_DATA0 0x30 > +#define AST_PECI_R_DATA1 0x34 > +#define AST_PECI_R_DATA2 0x38 > +#define AST_PECI_R_DATA3 0x3c > +#define AST_PECI_W_DATA4 0x40 > +#define AST_PECI_W_DATA5 0x44 > +#define AST_PECI_W_DATA6 0x48 > +#define AST_PECI_W_DATA7 0x4c > +#define AST_PECI_R_DATA4 0x50 > +#define AST_PECI_R_DATA5 0x54 > +#define AST_PECI_R_DATA6 0x58 > +#define AST_PECI_R_DATA7 0x5c > + > +/* AST_PECI_CTRL - 0x00 : Control Register */ > +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) > +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) > +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) > +#define PECI_CTRL_READ_MODE_MASK GENMASK(13, 12) > +#define PECI_CTRL_READ_MODE(x) (((x) << 12) & PECI_CTRL_READ_MODE_MASK) > +#define PECI_CTRL_READ_MODE_GET(x) (((x) & PECI_CTRL_READ_MODE_MASK) >> 12) > +#define PECI_CTRL_READ_MODE_COUNT BIT(12) > +#define PECI_CTRL_READ_MODE_DBG BIT(13) > +#define PECI_CTRL_CLK_SOURCE_MASK BIT(11) > +#define PECI_CTRL_CLK_SOURCE(x) (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK) > +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11) > +#define PECI_CTRL_CLK_DIV_MASK GENMASK(10, 8) > +#define PECI_CTRL_CLK_DIV(x) (((x) << 8) & PECI_CTRL_CLK_DIV_MASK) > +#define PECI_CTRL_CLK_DIV_GET(x) (((x) & PECI_CTRL_CLK_DIV_MASK) >> 8) > +#define PECI_CTRL_INVERT_OUT BIT(7) > +#define PECI_CTRL_INVERT_IN BIT(6) > +#define PECI_CTRL_BUS_CONTENT_EN BIT(5) > +#define PECI_CTRL_PECI_EN BIT(4) > +#define PECI_CTRL_PECI_CLK_EN BIT(0) I know these come from the ASPEED sdk driver. Do we need them all? > + > +/* AST_PECI_TIMING - 0x04 : Timing Negotiation Register */ > +#define PECI_TIMING_MESSAGE_MASK GENMASK(15, 8) > +#define PECI_TIMING_MESSAGE(x) (((x) << 8) & PECI_TIMING_MESSAGE_MASK) > +#define PECI_TIMING_MESSAGE_GET(x) (((x) & PECI_TIMING_MESSAGE_MASK) >> 8) > +#define PECI_TIMING_ADDRESS_MASK GENMASK(7, 0) > +#define PECI_TIMING_ADDRESS(x) ((x) & PECI_TIMING_ADDRESS_MASK) > +#define PECI_TIMING_ADDRESS_GET(x) ((x) & PECI_TIMING_ADDRESS_MASK) > + > +/* AST_PECI_CMD - 0x08 : Command Register */ > +#define PECI_CMD_PIN_MON BIT(31) > +#define PECI_CMD_STS_MASK GENMASK(27, 24) > +#define PECI_CMD_STS_GET(x) (((x) & PECI_CMD_STS_MASK) >> 24) > +#define PECI_CMD_FIRE BIT(0) > + > +/* AST_PECI_LEN - 0x0C : Read/Write Length Register */ > +#define PECI_AW_FCS_EN BIT(31) > +#define PECI_READ_LEN_MASK GENMASK(23, 16) > +#define PECI_READ_LEN(x) (((x) << 16) & PECI_READ_LEN_MASK) > +#define PECI_WRITE_LEN_MASK GENMASK(15, 8) > +#define PECI_WRITE_LEN(x) (((x) << 8) & PECI_WRITE_LEN_MASK) > +#define PECI_TAGET_ADDR_MASK GENMASK(7, 0) > +#define PECI_TAGET_ADDR(x) ((x) & PECI_TAGET_ADDR_MASK) > + > +/* AST_PECI_EXP_FCS - 0x10 : Expected FCS Data Register */ > +#define PECI_EXPECT_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_EXPECT_READ_FCS_GET(x) (((x) & PECI_EXPECT_READ_FCS_MASK) >> 16) > +#define PECI_EXPECT_AW_FCS_AUTO_MASK GENMASK(15, 8) > +#define PECI_EXPECT_AW_FCS_AUTO_GET(x) (((x) & PECI_EXPECT_AW_FCS_AUTO_MASK) \ > + >> 8) > +#define PECI_EXPECT_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_EXPECT_WRITE_FCS_GET(x) ((x) & PECI_EXPECT_WRITE_FCS_MASK) > + > +/* AST_PECI_CAP_FCS - 0x14 : Captured FCS Data Register */ > +#define PECI_CAPTURE_READ_FCS_MASK GENMASK(23, 16) > +#define PECI_CAPTURE_READ_FCS_GET(x) (((x) & PECI_CAPTURE_READ_FCS_MASK) >> 16) > +#define PECI_CAPTURE_WRITE_FCS_MASK GENMASK(7, 0) > +#define PECI_CAPTURE_WRITE_FCS_GET(x) ((x) & PECI_CAPTURE_WRITE_FCS_MASK) > + > +/* AST_PECI_INT_CTRL/STS - 0x18/0x1c : Interrupt Register */ > +#define PECI_INT_TIMING_RESULT_MASK GENMASK(31, 30) > +#define PECI_INT_TIMEOUT BIT(4) > +#define PECI_INT_CONNECT BIT(3) > +#define PECI_INT_W_FCS_BAD BIT(2) > +#define PECI_INT_W_FCS_ABORT BIT(1) > +#define PECI_INT_CMD_DONE BIT(0) > + > +struct aspeed_peci { > + struct peci_adapter adaper; > + struct device *dev; > + struct regmap *regmap; > + int irq; > + struct completion xfer_complete; > + u32 status; > + u32 cmd_timeout_ms; > +}; > + > +#define PECI_INT_MASK (PECI_INT_TIMEOUT | PECI_INT_CONNECT | \ > + PECI_INT_W_FCS_BAD | PECI_INT_W_FCS_ABORT | \ > + PECI_INT_CMD_DONE) > + > +#define PECI_IDLE_CHECK_TIMEOUT_MS 50 > +#define PECI_IDLE_CHECK_INTERVAL_MS 10 > + > +#define PECI_RD_SAMPLING_POINT_DEFAULT 8 > +#define PECI_RD_SAMPLING_POINT_MAX 15 > +#define PECI_CLK_DIV_DEFAULT 0 > +#define PECI_CLK_DIV_MAX 7 > +#define PECI_MSG_TIMING_NEGO_DEFAULT 1 > +#define PECI_MSG_TIMING_NEGO_MAX 255 > +#define PECI_ADDR_TIMING_NEGO_DEFAULT 1 > +#define PECI_ADDR_TIMING_NEGO_MAX 255 > +#define PECI_CMD_TIMEOUT_MS_DEFAULT 1000 > +#define PECI_CMD_TIMEOUT_MS_MAX 60000 > + > +static int aspeed_peci_xfer_native(struct aspeed_peci *priv, > + struct peci_xfer_msg *msg) > +{ > + long err, timeout = msecs_to_jiffies(priv->cmd_timeout_ms); > + u32 peci_head, peci_state, rx_data, cmd_sts; > + ktime_t start, end; > + s64 elapsed_ms; > + int i, rc = 0; > + uint reg; > + > + start = ktime_get(); > + > + /* Check command sts and bus idle state */ > + while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts) && > + (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) { > + end = ktime_get(); > + elapsed_ms = ktime_to_ms(ktime_sub(end, start)); > + if (elapsed_ms >= PECI_IDLE_CHECK_TIMEOUT_MS) { > + dev_dbg(priv->dev, "Timeout waiting for idle state!\n"); > + return -ETIMEDOUT; > + } > + > + usleep_range(PECI_IDLE_CHECK_INTERVAL_MS * 1000, > + (PECI_IDLE_CHECK_INTERVAL_MS * 1000) + 1000); > + }; Could the above use regmap_read_poll_timeout instead? > + > + reinit_completion(&priv->xfer_complete); > + > + peci_head = PECI_TAGET_ADDR(msg->addr) | > + PECI_WRITE_LEN(msg->tx_len) | > + PECI_READ_LEN(msg->rx_len); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD_CTRL, peci_head); > + if (rc) > + return rc; > + > + for (i = 0; i < msg->tx_len; i += 4) { > + reg = i < 16 ? AST_PECI_W_DATA0 + i % 16 : > + AST_PECI_W_DATA4 + i % 16; > + rc = regmap_write(priv->regmap, reg, > + (msg->tx_buf[i + 3] << 24) | > + (msg->tx_buf[i + 2] << 16) | > + (msg->tx_buf[i + 1] << 8) | > + msg->tx_buf[i + 0]); That looks like an endian swap. Can we do something like this? regmap_write(map, reg, cpu_to_be32p((void *)msg->tx_buff)) > + if (rc) > + return rc; > + } > + > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > +#if DUMP_DEBUG Having #defines is frowned upon. I think print_hex_dump_debug will do what you want here. > + print_hex_dump(KERN_DEBUG, "TX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->tx_buf, msg->tx_len, true); > +#endif > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, PECI_CMD_FIRE); > + if (rc) > + return rc; > + > + err = wait_for_completion_interruptible_timeout(&priv->xfer_complete, > + timeout); > + > + dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->status); > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); > + > + rc = regmap_write(priv->regmap, AST_PECI_CMD, 0); > + if (rc) > + return rc; > + > + if (err <= 0 || !(priv->status & PECI_INT_CMD_DONE)) { > + if (err < 0) { /* -ERESTARTSYS */ > + return (int)err; > + } else if (err == 0) { > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > + return -ETIMEDOUT; > + } > + > + dev_dbg(priv->dev, "No valid response!\n"); > + return -EIO; > + } > + > + for (i = 0; i < msg->rx_len; i++) { > + u8 byte_offset = i % 4; > + > + if (byte_offset == 0) { > + reg = i < 16 ? AST_PECI_R_DATA0 + i % 16 : > + AST_PECI_R_DATA4 + i % 16; I find this hard to read. Use a few more lines to make it clear what your code is doing. Actually, the entire for loop is cryptic. I understand what it's doing now. Can you rework it to make it more readable? You follow a similar pattern above in the write case. > + rc = regmap_read(priv->regmap, reg, &rx_data); > + if (rc) > + return rc; > + } > + > + msg->rx_buf[i] = (u8)(rx_data >> (byte_offset << 3)) > + } > + > +#if DUMP_DEBUG > + print_hex_dump(KERN_DEBUG, "RX : ", DUMP_PREFIX_NONE, 16, 1, > + msg->rx_buf, msg->rx_len, true); > +#endif > + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state)) > + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n", > + PECI_CMD_STS_GET(peci_state)); > + else > + dev_dbg(priv->dev, "PECI_STATE : read error\n"); Given the regmap_read is always going to be a memory read on the aspeed, I can't think of a situation where the read will fail. On that note, is there a reason you are using regmap and not just accessing the hardware directly? regmap imposes a number of pointer lookups and tests each time you do a read or write. > + dev_dbg(priv->dev, "------------------------\n"); > + > + return rc; > +} > + > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > +{ > + struct aspeed_peci *priv = arg; > + u32 status_ack = 0; > + > + if (regmap_read(priv->regmap, AST_PECI_INT_STS, &priv->status)) > + return IRQ_NONE; Again, a memory mapped read won't fail. How about we check that the regmap is working once in your _probe() function, and assume it will continue working from there (or remove the regmap abstraction all together). > + > + /* Be noted that multiple interrupt bits can be set at the same time */ > + if (priv->status & PECI_INT_TIMEOUT) { > + dev_dbg(priv->dev, "PECI_INT_TIMEOUT\n"); > + status_ack |= PECI_INT_TIMEOUT; > + } > + > + if (priv->status & PECI_INT_CONNECT) { > + dev_dbg(priv->dev, "PECI_INT_CONNECT\n"); > + status_ack |= PECI_INT_CONNECT; > + } > + > + if (priv->status & PECI_INT_W_FCS_BAD) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_BAD\n"); > + status_ack |= PECI_INT_W_FCS_BAD; > + } > + > + if (priv->status & PECI_INT_W_FCS_ABORT) { > + dev_dbg(priv->dev, "PECI_INT_W_FCS_ABORT\n"); > + status_ack |= PECI_INT_W_FCS_ABORT; > + } All of this code is for debugging only. Do you want to put it behind some kind of conditional? > + > + /** > + * All commands should be ended up with a PECI_INT_CMD_DONE bit set > + * even in an error case. > + */ > + if (priv->status & PECI_INT_CMD_DONE) { > + dev_dbg(priv->dev, "PECI_INT_CMD_DONE\n"); > + status_ack |= PECI_INT_CMD_DONE; > + complete(&priv->xfer_complete); > + } > + > + if (regmap_write(priv->regmap, AST_PECI_INT_STS, status_ack)) > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} > + > +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv) > +{ > + u32 msg_timing_nego, addr_timing_nego, rd_sampling_point; > + u32 clk_freq, clk_divisor, clk_div_val = 0; > + struct clk *clkin; > + int ret; > + > + clkin = devm_clk_get(priv->dev, NULL); > + if (IS_ERR(clkin)) { > + dev_err(priv->dev, "Failed to get clk source.\n"); > + return PTR_ERR(clkin); > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "clock-frequency", > + &clk_freq); > + if (ret < 0) { > + dev_err(priv->dev, > + "Could not read clock-frequency property.\n"); > + return ret; > + } > + > + clk_divisor = clk_get_rate(clkin) / clk_freq; > + devm_clk_put(priv->dev, clkin); > + > + while ((clk_divisor >> 1) && (clk_div_val < PECI_CLK_DIV_MAX)) > + clk_div_val++; We have a framework for doing clocks in the kernel. Would it make sense to write a driver for this clock and add it to drivers/clk/clk-aspeed.c? > + > + ret = of_property_read_u32(priv->dev->of_node, "msg-timing-nego", > + &msg_timing_nego); > + if (ret || msg_timing_nego > PECI_MSG_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid msg-timing-nego : %u, Use default : %u\n", > + msg_timing_nego, PECI_MSG_TIMING_NEGO_DEFAULT); The property is optional so I suggest we don't print a message if it's not present. We certainly don't want to print a message saying "invalid". The same comment applies to the other optional properties below. > + msg_timing_nego = PECI_MSG_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "addr-timing-nego", > + &addr_timing_nego); > + if (ret || addr_timing_nego > PECI_ADDR_TIMING_NEGO_MAX) { > + dev_warn(priv->dev, > + "Invalid addr-timing-nego : %u, Use default : %u\n", > + addr_timing_nego, PECI_ADDR_TIMING_NEGO_DEFAULT); > + addr_timing_nego = PECI_ADDR_TIMING_NEGO_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "rd-sampling-point", > + &rd_sampling_point); > + if (ret || rd_sampling_point > PECI_RD_SAMPLING_POINT_MAX) { > + dev_warn(priv->dev, > + "Invalid rd-sampling-point : %u. Use default : %u\n", > + rd_sampling_point, > + PECI_RD_SAMPLING_POINT_DEFAULT); > + rd_sampling_point = PECI_RD_SAMPLING_POINT_DEFAULT; > + } > + > + ret = of_property_read_u32(priv->dev->of_node, "cmd-timeout-ms", > + &priv->cmd_timeout_ms); > + if (ret || priv->cmd_timeout_ms > PECI_CMD_TIMEOUT_MS_MAX || > + priv->cmd_timeout_ms == 0) { > + dev_warn(priv->dev, > + "Invalid cmd-timeout-ms : %u. Use default : %u\n", > + priv->cmd_timeout_ms, > + PECI_CMD_TIMEOUT_MS_DEFAULT); > + priv->cmd_timeout_ms = PECI_CMD_TIMEOUT_MS_DEFAULT; > + } > + > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_CLK_DIV(PECI_CLK_DIV_DEFAULT) | > + PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + usleep_range(1000, 5000); Can we probe in parallel? If not, putting a sleep in the _probe will hold up the rest of drivers from being able to do anything, and hold up boot. If you decide that you do need to probe here, please add a comment. (This is the wait for the clock to be stable?) > + > + /** > + * Timing negotiation period setting. > + * The unit of the programmed value is 4 times of PECI clock period. > + */ > + ret = regmap_write(priv->regmap, AST_PECI_TIMING, > + PECI_TIMING_MESSAGE(msg_timing_nego) | > + PECI_TIMING_ADDRESS(addr_timing_nego)); > + if (ret) > + return ret; > + > + /* Clear interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_STS, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Enable interrupts */ > + ret = regmap_write(priv->regmap, AST_PECI_INT_CTRL, PECI_INT_MASK); > + if (ret) > + return ret; > + > + /* Read sampling point and clock speed setting */ > + ret = regmap_write(priv->regmap, AST_PECI_CTRL, > + PECI_CTRL_SAMPLING(rd_sampling_point) | > + PECI_CTRL_CLK_DIV(clk_div_val) | > + PECI_CTRL_PECI_EN | PECI_CTRL_PECI_CLK_EN); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static const struct regmap_config aspeed_peci_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = AST_PECI_R_DATA7, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > + .fast_io = true, > +}; > + > +static int aspeed_peci_xfer(struct peci_adapter *adaper, > + struct peci_xfer_msg *msg) > +{ > + struct aspeed_peci *priv = peci_get_adapdata(adaper); > + > + return aspeed_peci_xfer_native(priv, msg); > +} > + > +static int aspeed_peci_probe(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv; > + struct resource *res; > + void __iomem *base; > + int ret = 0; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + dev_set_drvdata(&pdev->dev, priv); > + priv->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &aspeed_peci_regmap_config); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->irq = platform_get_irq(pdev, 0); > + if (!priv->irq) > + return -ENODEV; > + > + ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler, > + IRQF_SHARED, This interrupt is only for the peci device. Why is it marked as shared? > + "peci-aspeed-irq", > + priv); > + if (ret < 0) > + return ret; > + > + init_completion(&priv->xfer_complete); > + > + priv->adaper.dev.parent = priv->dev; > + priv->adaper.dev.of_node = of_node_get(dev_of_node(priv->dev)); > + strlcpy(priv->adaper.name, pdev->name, sizeof(priv->adaper.name)); > + priv->adaper.xfer = aspeed_peci_xfer; > + peci_set_adapdata(&priv->adaper, priv); > + > + ret = aspeed_peci_init_ctrl(priv); > + if (ret < 0) > + return ret; > + > + ret = peci_add_adapter(&priv->adaper); > + if (ret < 0) > + return ret; > + > + dev_info(&pdev->dev, "peci bus %d registered, irq %d\n", > + priv->adaper.nr, priv->irq); > + > + return 0; > +} > + > +static int aspeed_peci_remove(struct platform_device *pdev) > +{ > + struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev); > + > + peci_del_adapter(&priv->adaper); > + of_node_put(priv->adaper.dev.of_node); > + > + return 0; > +} > + > +static const struct of_device_id aspeed_peci_of_table[] = { > + { .compatible = "aspeed,ast2400-peci", }, > + { .compatible = "aspeed,ast2500-peci", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table); > + > +static struct platform_driver aspeed_peci_driver = { > + .probe = aspeed_peci_probe, > + .remove = aspeed_peci_remove, > + .driver = { > + .name = "peci-aspeed", > + .of_match_table = of_match_ptr(aspeed_peci_of_table), > + }, > +}; > +module_platform_driver(aspeed_peci_driver); > + > +MODULE_AUTHOR("Ryan Chen "); > +MODULE_AUTHOR("Jae Hyun Yoo "); > +MODULE_DESCRIPTION("Aspeed PECI driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.16.2 >