On Mon, Jul 12, 2021 at 05:04:40PM CDT, Iwona Winiarska wrote:

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

+#define   ASPEED_PECI_CTRL_CLK_SOURCE_MASK	BIT(11)

_MASK suffix seems out of place on this one.

+#define   ASPEED_PECI_CTRL_CLK_DIV_MASK		GENMASK(10, 8)
+#define   ASPEED_PECI_CTRL_INVERT_OUT		BIT(7)
+#define   ASPEED_PECI_CTRL_INVERT_IN		BIT(6)
+#define   ASPEED_PECI_CTRL_BUS_CONTENT_EN	BIT(5)

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

+#define   ASPEED_PECI_CTRL_PECI_EN		BIT(4)
+#define   ASPEED_PECI_CTRL_PECI_CLK_EN		BIT(0)
+
+/* Timing Negotiation Register */
+#define ASPEED_PECI_TIMING_NEGOTIATION		0x04
+#define   ASPEED_PECI_TIMING_MESSAGE_MASK	GENMASK(15, 8)
+#define   ASPEED_PECI_TIMING_ADDRESS_MASK	GENMASK(7, 0)
+
+/* Command Register */
+#define ASPEED_PECI_CMD				0x08
+#define   ASPEED_PECI_CMD_PIN_MON		BIT(31)
+#define   ASPEED_PECI_CMD_STS_MASK		GENMASK(27, 24)
+#define     ASPEED_PECI_CMD_STS_ADDR_T_NEGO	0x3
+#define   ASPEED_PECI_CMD_IDLE_MASK		\
+	  (ASPEED_PECI_CMD_STS_MASK | ASPEED_PECI_CMD_PIN_MON)
+#define   ASPEED_PECI_CMD_FIRE			BIT(0)
+
+/* Read/Write Length Register */
+#define ASPEED_PECI_RW_LENGTH			0x0c
+#define   ASPEED_PECI_AW_FCS_EN			BIT(31)
+#define   ASPEED_PECI_READ_LEN_MASK		GENMASK(23, 16)
+#define   ASPEED_PECI_WRITE_LEN_MASK		GENMASK(15, 8)
+#define   ASPEED_PECI_TAGET_ADDR_MASK		GENMASK(7, 0)

s/TAGET/TARGET/

+
+/* Expected FCS Data Register */
+#define ASPEED_PECI_EXP_FCS			0x10
+#define   ASPEED_PECI_EXP_READ_FCS_MASK		GENMASK(23, 16)
+#define   ASPEED_PECI_EXP_AW_FCS_AUTO_MASK	GENMASK(15, 8)
+#define   ASPEED_PECI_EXP_WRITE_FCS_MASK	GENMASK(7, 0)
+
+/* Captured FCS Data Register */
+#define ASPEED_PECI_CAP_FCS			0x14
+#define   ASPEED_PECI_CAP_READ_FCS_MASK		GENMASK(23, 16)
+#define   ASPEED_PECI_CAP_WRITE_FCS_MASK	GENMASK(7, 0)
+
+/* Interrupt Register */
+#define ASPEED_PECI_INT_CTRL			0x18
+#define   ASPEED_PECI_TIMING_NEGO_SEL_MASK	GENMASK(31, 30)
+#define     ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO	0
+#define     ASPEED_PECI_2ND_BIT_OF_ADDR_NEGO	1
+#define     ASPEED_PECI_MESSAGE_NEGO		2
+#define   ASPEED_PECI_INT_MASK			GENMASK(4, 0)
+#define   ASPEED_PECI_INT_BUS_TIMEOUT		BIT(4)
+#define   ASPEED_PECI_INT_BUS_CONNECT		BIT(3)

s/CONNECT/CONTENTION/

+#define   ASPEED_PECI_INT_W_FCS_BAD		BIT(2)
+#define   ASPEED_PECI_INT_W_FCS_ABORT		BIT(1)
+#define   ASPEED_PECI_INT_CMD_DONE		BIT(0)
+
+/* Interrupt Status Register */
+#define ASPEED_PECI_INT_STS			0x1c
+#define   ASPEED_PECI_INT_TIMING_RESULT_MASK	GENMASK(29, 16)
+	  /* bits[4..0]: Same bit fields in the 'Interrupt Register' */
+
+/* Rx/Tx Data Buffer Registers */
+#define ASPEED_PECI_W_DATA0			0x20
+#define ASPEED_PECI_W_DATA1			0x24
+#define ASPEED_PECI_W_DATA2			0x28
+#define ASPEED_PECI_W_DATA3			0x2c
+#define ASPEED_PECI_R_DATA0			0x30
+#define ASPEED_PECI_R_DATA1			0x34
+#define ASPEED_PECI_R_DATA2			0x38
+#define ASPEED_PECI_R_DATA3			0x3c
+#define ASPEED_PECI_W_DATA4			0x40
+#define ASPEED_PECI_W_DATA5			0x44
+#define ASPEED_PECI_W_DATA6			0x48
+#define ASPEED_PECI_W_DATA7			0x4c
+#define ASPEED_PECI_R_DATA4			0x50
+#define ASPEED_PECI_R_DATA5			0x54
+#define ASPEED_PECI_R_DATA6			0x58
+#define ASPEED_PECI_R_DATA7			0x5c
+#define   ASPEED_PECI_DATA_BUF_SIZE_MAX		32
+
+/* Timing Negotiation */
+#define ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT	8
+#define ASPEED_PECI_RD_SAMPLING_POINT_MAX	(BIT(4) - 1)
+#define ASPEED_PECI_CLK_DIV_DEFAULT		0
+#define ASPEED_PECI_CLK_DIV_MAX			(BIT(3) - 1)
+#define ASPEED_PECI_MSG_TIMING_DEFAULT		1
+#define ASPEED_PECI_MSG_TIMING_MAX		(BIT(8) - 1)
+#define ASPEED_PECI_ADDR_TIMING_DEFAULT		1
+#define ASPEED_PECI_ADDR_TIMING_MAX		(BIT(8) - 1)
+
+/* Timeout */
+#define ASPEED_PECI_IDLE_CHECK_TIMEOUT_US	(50 * USEC_PER_MSEC)
+#define ASPEED_PECI_IDLE_CHECK_INTERVAL_US	(10 * USEC_PER_MSEC)
+#define ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT	(1000)
+#define ASPEED_PECI_CMD_TIMEOUT_MS_MAX		(1000)
+
+struct aspeed_peci {
+	struct peci_controller controller;
+	struct device *dev;
+	void __iomem *base;
+	struct clk *clk;
+	struct reset_control *rst;
+	int irq;
+	spinlock_t lock; /* to sync completion status handling */
+	struct completion xfer_complete;
+	u32 status;
+	u32 cmd_timeout_ms;
+	u32 msg_timing;
+	u32 addr_timing;
+	u32 rd_sampling_point;
+	u32 clk_div;
+};
+
+static inline struct aspeed_peci *to_aspeed_peci(struct peci_controller *a)
+{
+	return container_of(a, struct aspeed_peci, controller);
+}
+
+static void aspeed_peci_init_regs(struct aspeed_peci *priv)
+{
+	u32 val;
+
+	val = FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, ASPEED_PECI_CLK_DIV_DEFAULT);
+	val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
+	writel(val, priv->base + ASPEED_PECI_CTRL);
+	/*
+	 * Timing negotiation period setting.
+	 * The unit of the programmed value is 4 times of PECI clock period.
+	 */
+	val = FIELD_PREP(ASPEED_PECI_TIMING_MESSAGE_MASK, priv->msg_timing);
+	val |= FIELD_PREP(ASPEED_PECI_TIMING_ADDRESS_MASK, priv->addr_timing);
+	writel(val, priv->base + ASPEED_PECI_TIMING_NEGOTIATION);
+
+	/* Clear interrupts */
+	val = readl(priv->base + ASPEED_PECI_INT_STS) | ASPEED_PECI_INT_MASK;

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

+	writel(val, priv->base + ASPEED_PECI_INT_STS);
+
+	/* Set timing negotiation mode and enable interrupts */
+	val = FIELD_PREP(ASPEED_PECI_TIMING_NEGO_SEL_MASK, ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO);
+	val |= ASPEED_PECI_INT_MASK;
+	writel(val, priv->base + ASPEED_PECI_INT_CTRL);
+
+	val = FIELD_PREP(ASPEED_PECI_CTRL_SAMPLING_MASK, priv->rd_sampling_point);
+	val |= FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, priv->clk_div);
+	val |= ASPEED_PECI_CTRL_PECI_EN;
+	val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
+	writel(val, priv->base + ASPEED_PECI_CTRL);
+}
+
+static inline int aspeed_peci_check_idle(struct aspeed_peci *priv)
+{
+	u32 cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
+
+	if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts) == ASPEED_PECI_CMD_STS_ADDR_T_NEGO)
+		aspeed_peci_init_regs(priv);
+
+	return readl_poll_timeout(priv->base + ASPEED_PECI_CMD,
+				  cmd_sts,
+				  !(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK),
+				  ASPEED_PECI_IDLE_CHECK_INTERVAL_US,
+				  ASPEED_PECI_IDLE_CHECK_TIMEOUT_US);
+}
+
+static int aspeed_peci_xfer(struct peci_controller *controller,
+			    u8 addr, struct peci_request *req)
+{
+	struct aspeed_peci *priv = to_aspeed_peci(controller);
+	unsigned long flags, timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
+	u32 peci_head;
+	int ret;
+
+	if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
+	    req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
+		return -EINVAL;
+
+	/* Check command sts and bus idle state */
+	ret = aspeed_peci_check_idle(priv);
+	if (ret)
+		return ret; /* -ETIMEDOUT */
+
+	spin_lock_irqsave(&priv->lock, flags);
+	reinit_completion(&priv->xfer_complete);
+
+	peci_head = FIELD_PREP(ASPEED_PECI_TAGET_ADDR_MASK, addr) |
+		    FIELD_PREP(ASPEED_PECI_WRITE_LEN_MASK, req->tx.len) |
+		    FIELD_PREP(ASPEED_PECI_READ_LEN_MASK, req->rx.len);
+
+	writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
+
+	memcpy_toio(priv->base + ASPEED_PECI_W_DATA0, req->tx.buf,
+		    req->tx.len > 16 ? 16 : req->tx.len);

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

+	if (req->tx.len > 16)
+		memcpy_toio(priv->base + ASPEED_PECI_W_DATA4, req->tx.buf + 16,
+			    req->tx.len - 16);
+
+	dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
+	print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req->tx.len);
+
+	priv->status = 0;
+	writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	ret = wait_for_completion_interruptible_timeout(&priv->xfer_complete, timeout);
+	if (ret < 0)
+		return ret;
+
+	if (ret == 0) {
+		dev_dbg(priv->dev, "Timeout waiting for a response!\n");
+		return -ETIMEDOUT;
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	writel(0, priv->base + ASPEED_PECI_CMD);
+
+	if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		dev_dbg(priv->dev, "No valid response!\n");
+		return -EIO;
+	}
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_R_DATA0,
+		      req->rx.len > 16 ? 16 : req->rx.len);

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

+	if (req->rx.len > 16)
+		memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_R_DATA4,
+			      req->rx.len - 16);
+
+	print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);
+
+	return 0;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+	struct aspeed_peci *priv = arg;
+	u32 status;
+
+	spin_lock(&priv->lock);
+	status = readl(priv->base + ASPEED_PECI_INT_STS);
+	writel(status, priv->base + ASPEED_PECI_INT_STS);
+	priv->status |= (status & ASPEED_PECI_INT_MASK);
+
+	/*
+	 * In most cases, interrupt bits will be set one by one but also note
+	 * that multiple interrupt bits could be set at the same time.
+	 */
+	if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_TIMEOUT\n");
+
+	if (status & ASPEED_PECI_INT_BUS_CONNECT)
+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONNECT\n");

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

+
+	if (status & ASPEED_PECI_INT_W_FCS_BAD)
+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_BAD\n");
+
+	if (status & ASPEED_PECI_INT_W_FCS_ABORT)
+		dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_ABORT\n");

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

+
+	/*
+	 * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE bit
+	 * set even in an error case.
+	 */
+	if (status & ASPEED_PECI_INT_CMD_DONE)
+		complete(&priv->xfer_complete);
+
+	spin_unlock(&priv->lock);
+
+	return IRQ_HANDLED;
+}
+
+static void __sanitize_clock_divider(struct aspeed_peci *priv)
+{
+	u32 clk_div;
+	int ret;
+
+	ret = device_property_read_u32(priv->dev, "clock-divider", &clk_div);
+	if (ret) {
+		clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;
+	} else if (clk_div > ASPEED_PECI_CLK_DIV_MAX) {
+		dev_warn(priv->dev, "Invalid clock-divider: %u, Using default: %u\n",
+			 clk_div, ASPEED_PECI_CLK_DIV_DEFAULT);
+
+		clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;
+	}
+
+	priv->clk_div = clk_div;
+}
+

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

+static void __sanitize_msg_timing(struct aspeed_peci *priv)
+{
+	u32 msg_timing;
+	int ret;
+
+	ret = device_property_read_u32(priv->dev, "msg-timing", &msg_timing);
+	if (ret) {
+		msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;
+	} else if (msg_timing > ASPEED_PECI_MSG_TIMING_MAX) {
+		dev_warn(priv->dev, "Invalid msg-timing : %u, Use default : %u\n",
+			 msg_timing, ASPEED_PECI_MSG_TIMING_DEFAULT);
+
+		msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;
+	}
+
+	priv->msg_timing = msg_timing;
+}
+
+static void __sanitize_addr_timing(struct aspeed_peci *priv)
+{
+	u32 addr_timing;
+	int ret;
+
+	ret = device_property_read_u32(priv->dev, "addr-timing", &addr_timing);
+	if (ret) {
+		addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;
+	} else if (addr_timing > ASPEED_PECI_ADDR_TIMING_MAX) {
+		dev_warn(priv->dev, "Invalid addr-timing : %u, Use default : %u\n",
+			 addr_timing, ASPEED_PECI_ADDR_TIMING_DEFAULT);
+
+		addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;
+	}
+
+	priv->addr_timing = addr_timing;
+}
+
+static void __sanitize_rd_sampling_point(struct aspeed_peci *priv)
+{
+	u32 rd_sampling_point;
+	int ret;
+
+	ret = device_property_read_u32(priv->dev, "rd-sampling-point", &rd_sampling_point);
+	if (ret) {
+		rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;
+	} else if (rd_sampling_point > ASPEED_PECI_RD_SAMPLING_POINT_MAX) {
+		dev_warn(priv->dev, "Invalid rd-sampling-point: %u, Use default : %u\n",
+			 rd_sampling_point, ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);
+
+		rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;
+	}
+
+	priv->rd_sampling_point = rd_sampling_point;
+}
+
+static void __sanitize_cmd_timeout(struct aspeed_peci *priv)
+{
+	u32 timeout;
+	int ret;
+
+	ret = device_property_read_u32(priv->dev, "cmd-timeout-ms", &timeout);
+	if (ret) {
+		timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;
+	} else if (timeout > ASPEED_PECI_CMD_TIMEOUT_MS_MAX || timeout == 0) {
+		dev_warn(priv->dev, "Invalid cmd-timeout-ms: %u, Use default: %u\n",
+			 timeout, ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT);
+
+		timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;
+	}
+
+	priv->cmd_timeout_ms = timeout;
+}
+
+static void aspeed_peci_device_property_sanitize(struct aspeed_peci *priv)
+{
+	__sanitize_clock_divider(priv);
+	__sanitize_msg_timing(priv);
+	__sanitize_addr_timing(priv);
+	__sanitize_rd_sampling_point(priv);
+	__sanitize_cmd_timeout(priv);
+}
+
+static void aspeed_peci_disable_clk(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)
+{
+	int ret;
+
+	priv->clk = devm_clk_get(priv->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(priv->dev, PTR_ERR(priv->clk), "Failed to get clk source\n");
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(priv->dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(priv->dev, aspeed_peci_disable_clk, priv->clk);
+	if (ret)
+		return ret;
+
+	aspeed_peci_device_property_sanitize(priv);
+
+	aspeed_peci_init_regs(priv);
+
+	return 0;
+}
+
+static int aspeed_peci_probe(struct platform_device *pdev)
+{
+	struct aspeed_peci *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+	dev_set_drvdata(priv->dev, priv);
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (!priv->irq)
+		return priv->irq;
+
+	ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler,
+			       0, "peci-aspeed-irq", priv);

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

+	if (ret)
+		return ret;
+
+	init_completion(&priv->xfer_complete);
+	spin_lock_init(&priv->lock);
+
+	priv->controller.xfer = aspeed_peci_xfer;
+
+	priv->rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->rst)) {
+		dev_err(&pdev->dev, "Missing or invalid reset controller entry\n");
+		return PTR_ERR(priv->rst);
+	}
+	reset_control_deassert(priv->rst);
+
+	ret = aspeed_peci_init_ctrl(priv);
+	if (ret)
+		return ret;
+
+	return peci_controller_add(&priv->controller, priv->dev);
+}
+
+static int aspeed_peci_remove(struct platform_device *pdev)
+{
+	struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);
+
+	peci_controller_remove(&priv->controller);
+	reset_control_assert(priv->rst);
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_peci_of_table[] = {
+	{ .compatible = "aspeed,ast2400-peci", },
+	{ .compatible = "aspeed,ast2500-peci", },
+	{ .compatible = "aspeed,ast2600-peci", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
+
+static struct platform_driver aspeed_peci_driver = {
+	.probe  = aspeed_peci_probe,
+	.remove = aspeed_peci_remove,
+	.driver = {
+		.name           = "peci-aspeed",
+		.of_match_table = aspeed_peci_of_table,
+	},
+};
+module_platform_driver(aspeed_peci_driver);
+
+MODULE_AUTHOR("Ryan Chen [off-list ref]");
+MODULE_AUTHOR("Jae Hyun Yoo [off-list ref]");
+MODULE_DESCRIPTION("ASPEED PECI driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PECI);
-- 
2.31.1