[PATCH v4 0/2] add regmap-spi-avmm & Intel Max10 BMC chip support

[PATCH v4 0/2] add regmap-spi-avmm & Intel Max10 BMC chip support

[Xu Yilun]

This patchset adds the regmap-spi-avmm to support the Intel SPI Slave to
AVMM Bus Bridge (spi-avmm) IP block. It also implements the usercase - the
driver of Intel Max10 BMC chip which integrates this IP block.

Patch #1 implements the regmap-spi-avmm.
Patch #2 implements the mfd driver of Intel Max10 BMC chip.

Main changes from v1:
- Split out the regmap-spi-avmm module out of intel-m10-bmc module.

Main changes from v2:
- Refactor the code of regmap-spi-avmm.
   - Rewrites the rx flow and simplifies the implementation, collapse some
     function calls.
   - Add bounds checking every time we fill trans_buf & phy_buf.
   - Try to configure spi mode on regmap init
   - delete regmap_bus.reg_write/reg_read callbacks.
   - Squash the bug fixing patch.
- Add the sub devices in mfd_cell for Max10 bmc driver.
- Improve comments and some minor fixes.

Main changes from v3:
- Rebased to 5.9-rc1.
- Collapse the phy buf padding code in br_pkt_phy_tx_prepare().

Xu Yilun (2):
  regmap: add Intel SPI Slave to AVMM Bus Bridge support
  mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

 .../ABI/testing/sysfs-driver-intel-m10-bmc         |  15 +
 drivers/base/regmap/Kconfig                        |   6 +-
 drivers/base/regmap/Makefile                       |   1 +
 drivers/base/regmap/regmap-spi-avmm.c              | 719 +++++++++++++++++++++
 drivers/mfd/Kconfig                                |  13 +
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/intel-m10-bmc.c                        | 169 +++++
 include/linux/mfd/intel-m10-bmc.h                  |  57 ++
 include/linux/regmap.h                             |  36 ++
 9 files changed, 1017 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
 create mode 100644 drivers/base/regmap/regmap-spi-avmm.c
 create mode 100644 drivers/mfd/intel-m10-bmc.c
 create mode 100644 include/linux/mfd/intel-m10-bmc.h

-- 
2.7.4

[PATCH v4 1/2] regmap: add Intel SPI Slave to AVMM Bus Bridge support

[Xu Yilun]

This patch add support for regmap APIs that are intended to be used by
the drivers of some SPI slave chips which integrate the "SPI slave to
Avalon Master Bridge" (spi-avmm) IP.

The spi-avmm IP acts as a bridge to convert encoded streams of bytes
from the host to the chip's internal register read/write on Avalon bus.
The driver implements the register read/write operations for a generic
SPI master to access the sub devices behind spi-avmm bridge.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Reviewed-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>
---
v2: Split out the regmap-spi-avmm as an independent module.
v3: rewrite the rx flow, now the driver reads one word and parse it
     immediately into trans_buf. This simplifies the rx handling.
    collapse some function calls.
    Add bounds checking every time we fill trans_buf & phy_buf.
    Try to configure spi mode on regmap init.
    delete regmap_bus.reg_write/reg_read callbacks.
    squash the bug fixing patch.
    improve comments and minor fixes.
v4: collapse the phy buf padding code in br_pkt_phy_tx_prepare().
---
 drivers/base/regmap/Kconfig           |   6 +-
 drivers/base/regmap/Makefile          |   1 +
 drivers/base/regmap/regmap-spi-avmm.c | 719 ++++++++++++++++++++++++++++++++++
 include/linux/regmap.h                |  36 ++
 4 files changed, 761 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-spi-avmm.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 1d1d26b..bcb90d8 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -4,7 +4,7 @@
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SCCB || REGMAP_I3C)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SCCB || REGMAP_I3C || REGMAP_SPI_AVMM)
 	select IRQ_DOMAIN if REGMAP_IRQ
 	bool
 
@@ -53,3 +53,7 @@ config REGMAP_SCCB
 config REGMAP_I3C
 	tristate
 	depends on I3C
+
+config REGMAP_SPI_AVMM
+	tristate
+	depends on SPI
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index ff6c7d8..ac1b69e 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_REGMAP_W1) += regmap-w1.o
 obj-$(CONFIG_REGMAP_SOUNDWIRE) += regmap-sdw.o
 obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
 obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o
+obj-$(CONFIG_REGMAP_SPI_AVMM) += regmap-spi-avmm.o
diff --git a/drivers/base/regmap/regmap-spi-avmm.c b/drivers/base/regmap/regmap-spi-avmm.c
new file mode 100644
index 0000000..ad1da83
--- /dev/null
+++ b/drivers/base/regmap/regmap-spi-avmm.c
@@ -0,0 +1,719 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Register map access API - SPI AVMM support
+//
+// Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
+
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+/*
+ * This driver implements the regmap operations for a generic SPI
+ * master to access the registers of the spi slave chip which has an
+ * Avalone bus in it.
+ *
+ * The "SPI slave to Avalon Master Bridge" (spi-avmm) IP should be integrated
+ * in the spi slave chip. The IP acts as a bridge to convert encoded streams of
+ * bytes from the host to the internal register read/write on Avalon bus. In
+ * order to issue register access requests to the slave chip, the host should
+ * send formatted bytes that conform to the transfer protocol.
+ * The transfer protocol contains 3 layers: transaction layer, packet layer
+ * and physical layer.
+ *
+ * Reference Documents could be found at:
+ * https://www.intel.com/content/www/us/en/programmable/documentation/sfo1400787952932.html
+ *
+ * Chapter "SPI Slave/JTAG to Avalon Master Bridge Cores" is a general
+ * introduction to the protocol.
+ *
+ * Chapter "Avalon Packets to Transactions Converter Core" describes
+ * the transaction layer.
+ *
+ * Chapter "Avalon-ST Bytes to Packets and Packets to Bytes Converter Cores"
+ * describes the packet layer.
+ *
+ * Chapter "Avalon-ST Serial Peripheral Interface Core" describes the
+ * physical layer.
+ *
+ *
+ * When host issues a regmap read/write, the driver will transform the request
+ * to byte stream layer by layer. It formats the register addr, value and
+ * length to the transaction layer request, then converts the request to packet
+ * layer bytes stream and then to physical layer bytes stream. Finally the
+ * driver sends the formatted byte stream over SPI bus to the slave chip.
+ *
+ * The spi-avmm IP on the slave chip decodes the byte stream and initiates
+ * register read/write on its internal Avalon bus, and then encodes the
+ * response to byte stream and sends back to host.
+ *
+ * The driver receives the byte stream, reverses the 3 layers transformation,
+ * and finally gets the response value (read out data for register read,
+ * successful written size for register write).
+ */
+
+#define PKT_SOP			0x7a
+#define PKT_EOP			0x7b
+#define PKT_CHANNEL		0x7c
+#define PKT_ESC			0x7d
+
+#define PHY_IDLE		0x4a
+#define PHY_ESC			0x4d
+
+#define TRANS_CODE_WRITE	0x0
+#define TRANS_CODE_SEQ_WRITE	0x4
+#define TRANS_CODE_READ		0x10
+#define TRANS_CODE_SEQ_READ	0x14
+#define TRANS_CODE_NO_TRANS	0x7f
+
+#define SPI_AVMM_XFER_TIMEOUT	(msecs_to_jiffies(200))
+
+/* slave's register addr is 32 bits */
+#define SPI_AVMM_REG_SIZE		4UL
+/* slave's register value is 32 bits */
+#define SPI_AVMM_VAL_SIZE		4UL
+
+/*
+ * max rx size could be larger. But considering the buffer consuming,
+ * it is proper that we limit 1KB xfer at max.
+ */
+#define MAX_READ_CNT		256UL
+#define MAX_WRITE_CNT		1UL
+
+struct trans_req_header {
+	u8 code;
+	u8 rsvd;
+	__be16 size;
+	__be32 addr;
+} __packed;
+
+struct trans_resp_header {
+	u8 r_code;
+	u8 rsvd;
+	__be16 size;
+} __packed;
+
+#define TRANS_REQ_HD_SIZE	(sizeof(struct trans_req_header))
+#define TRANS_RESP_HD_SIZE	(sizeof(struct trans_resp_header))
+
+/*
+ * In transaction layer,
+ * the write request format is: Transaction request header + data
+ * the read request format is: Transaction request header
+ * the write response format is: Transaction response header
+ * the read response format is: pure data, no Transaction response header
+ */
+#define TRANS_WR_TX_SIZE(n)	(TRANS_REQ_HD_SIZE + SPI_AVMM_VAL_SIZE * (n))
+#define TRANS_RD_TX_SIZE	TRANS_REQ_HD_SIZE
+#define TRANS_TX_MAX		TRANS_WR_TX_SIZE(MAX_WRITE_CNT)
+
+#define TRANS_RD_RX_SIZE(n)	(SPI_AVMM_VAL_SIZE * (n))
+#define TRANS_WR_RX_SIZE	TRANS_RESP_HD_SIZE
+#define TRANS_RX_MAX		TRANS_RD_RX_SIZE(MAX_READ_CNT)
+
+/* tx & rx share one transaction layer buffer */
+#define TRANS_BUF_SIZE		((TRANS_TX_MAX > TRANS_RX_MAX) ?	\
+				 TRANS_TX_MAX : TRANS_RX_MAX)
+
+/*
+ * In tx phase, the host prepares all the phy layer bytes of a request in the
+ * phy buffer and sends them in a batch.
+ *
+ * The packet layer and physical layer defines several special chars for
+ * various purpose, when a transaction layer byte hits one of these special
+ * chars, it should be escaped. The escape rule is, "Escape char first,
+ * following the byte XOR'ed with 0x20".
+ *
+ * This macro defines the max possible length of the phy data. In the worst
+ * case, all transaction layer bytes need to be escaped (so the data length
+ * doubles), plus 4 special chars (SOP, CHANNEL, CHANNEL_NUM, EOP). Finally
+ * we should make sure the length is aligned to SPI BPW.
+ */
+#define PHY_TX_MAX		ALIGN(2 * TRANS_TX_MAX + 4, 4)
+
+/*
+ * Unlike tx, phy rx is affected by possible PHY_IDLE bytes from slave, the max
+ * length of the rx bit stream is unpredictable. So the driver reads the words
+ * one by one, and parses each word immediately into transaction layer buffer.
+ * Only one word length of phy buffer is used for rx.
+ */
+#define PHY_BUF_SIZE		PHY_TX_MAX
+
+/**
+ * struct spi_avmm_bridge - SPI slave to AVMM bus master bridge
+ *
+ * @spi: spi slave associated with this bridge.
+ * @word_len: bytes of word for spi transfer.
+ * @trans_len: length of valid data in trans_buf.
+ * @phy_len: length of valid data in phy_buf.
+ * @trans_buf: the bridge buffer for transaction layer data.
+ * @phy_buf: the bridge buffer for physical layer data.
+ * @swap_words: the word swapping cb for phy data. NULL if not needed.
+ *
+ * As a device's registers are implemented on the AVMM bus address space, it
+ * requires the driver to issue formatted requests to spi slave to AVMM bus
+ * master bridge to perform register access.
+ */
+struct spi_avmm_bridge {
+	struct spi_device *spi;
+	unsigned char word_len;
+	unsigned int trans_len;
+	unsigned int phy_len;
+	/* bridge buffer used in translation between protocol layers */
+	char trans_buf[TRANS_BUF_SIZE];
+	char phy_buf[PHY_BUF_SIZE];
+	void (*swap_words)(char *buf, unsigned int len);
+};
+
+static void br_swap_words_32(char *buf, unsigned int len)
+{
+	u32 *p = (u32 *)buf;
+	unsigned int count;
+
+	count = len / 4;
+	while (count--) {
+		*p = swab32p(p);
+		p++;
+	}
+}
+
+/*
+ * Format transaction layer data in br->trans_buf according to the register
+ * access request, Store valid transaction layer data length in br->trans_len.
+ */
+static int br_trans_tx_prepare(struct spi_avmm_bridge *br, bool is_read, u32 reg,
+			       u32 *wr_val, u32 count)
+{
+	struct trans_req_header *header;
+	unsigned int trans_len;
+	u8 code;
+	__le32 *data;
+	int i;
+
+	if (is_read) {
+		if (count == 1)
+			code = TRANS_CODE_READ;
+		else
+			code = TRANS_CODE_SEQ_READ;
+	} else {
+		if (count == 1)
+			code = TRANS_CODE_WRITE;
+		else
+			code = TRANS_CODE_SEQ_WRITE;
+	}
+
+	header = (struct trans_req_header *)br->trans_buf;
+	header->code = code;
+	header->rsvd = 0;
+	header->size = cpu_to_be16((u16)count * SPI_AVMM_VAL_SIZE);
+	header->addr = cpu_to_be32(reg);
+
+	trans_len = TRANS_REQ_HD_SIZE;
+
+	if (!is_read) {
+		trans_len += SPI_AVMM_VAL_SIZE * count;
+		if (trans_len > sizeof(br->trans_buf))
+			return -ENOMEM;
+
+		data = (__le32 *)(br->trans_buf + TRANS_REQ_HD_SIZE);
+
+		for (i = 0; i < count; i++)
+			*data++ = cpu_to_le32(*wr_val++);
+	}
+
+	/* Store valid trans data length for next layer */
+	br->trans_len = trans_len;
+
+	return 0;
+}
+
+/*
+ * Convert transaction layer data (in br->trans_buf) to phy layer data, store
+ * them in br->phy_buf. Pad the phy_buf aligned with SPI's BPW. Store valid phy
+ * layer data length in br->phy_len.
+ *
+ * phy_buf len should be aligned with SPI's BPW. Spare bytes should be padded
+ * with PHY_IDLE, then the slave will just drop them.
+ *
+ * The driver will not simply pad 4a at the tail. The concern is that driver
+ * will not store MISO data during tx phase, if the driver pads 4a at the tail,
+ * it is possible that if the slave is fast enough to response at the padding
+ * time. As a result these rx bytes are lost. In the following case, 7a,7c,00
+ * will lost.
+ * MOSI ...|7a|7c|00|10| |00|00|04|02| |4b|7d|5a|7b| |40|4a|4a|4a| |XX|XX|...
+ * MISO ...|4a|4a|4a|4a| |4a|4a|4a|4a| |4a|4a|4a|4a| |4a|7a|7c|00| |78|56|...
+ *
+ * So the driver moves EOP and bytes after EOP to the end of the aligned size,
+ * then fill the hole with PHY_IDLE. As following:
+ * before pad ...|7a|7c|00|10| |00|00|04|02| |4b|7d|5a|7b| |40|
+ * after pad  ...|7a|7c|00|10| |00|00|04|02| |4b|7d|5a|4a| |4a|4a|7b|40|
+ * Then if the slave will not get the entire packet before the tx phase is
+ * over, it can't responsed to anything either.
+ */
+static int br_pkt_phy_tx_prepare(struct spi_avmm_bridge *br)
+{
+	char *tb, *tb_end, *pb, *pb_limit, *pb_eop = NULL;
+	unsigned int aligned_phy_len, move_size;
+	bool need_esc = false;
+
+	tb = br->trans_buf;
+	tb_end = tb + br->trans_len;
+	pb = br->phy_buf;
+	pb_limit = pb + ARRAY_SIZE(br->phy_buf);
+
+	*pb++ = PKT_SOP;
+
+	/*
+	 * The driver doesn't support multiple channels so the channel number
+	 * is always 0.
+	 */
+	*pb++ = PKT_CHANNEL;
+	*pb++ = 0x0;
+
+	for (; pb < pb_limit && tb < tb_end; pb++) {
+		if (need_esc) {
+			*pb = *tb++ ^ 0x20;
+			need_esc = false;
+			continue;
+		}
+
+		/* EOP should be inserted before the last valid char */
+		if (tb == tb_end - 1 && !pb_eop) {
+			*pb = PKT_EOP;
+			pb_eop = pb;
+			continue;
+		}
+
+		/*
+		 * insert an ESCAPE char if the data value equals any special
+		 * char.
+		 */
+		switch (*tb) {
+		case PKT_SOP:
+		case PKT_EOP:
+		case PKT_CHANNEL:
+		case PKT_ESC:
+			*pb = PKT_ESC;
+			need_esc = true;
+			break;
+		case PHY_IDLE:
+		case PHY_ESC:
+			*pb = PHY_ESC;
+			need_esc = true;
+			break;
+		default:
+			*pb = *tb++;
+			break;
+		}
+	}
+
+	/* The phy buffer is used out but transaction layer data remains */
+	if (tb < tb_end)
+		return -ENOMEM;
+
+	/* Store valid phy data length for spi transfer */
+	br->phy_len = pb - br->phy_buf;
+
+	if (br->word_len == 1)
+		return 0;
+
+	/* Do phy buf padding if word_len > 1 byte. */
+	aligned_phy_len = ALIGN(br->phy_len, br->word_len);
+	if (aligned_phy_len > sizeof(br->phy_buf))
+		return -ENOMEM;
+
+	if (aligned_phy_len == br->phy_len)
+		return 0;
+
+	/* move EOP and bytes after EOP to the end of aligned size */
+	move_size = pb - pb_eop;
+	memmove(&br->phy_buf[aligned_phy_len - move_size], pb_eop, move_size);
+
+	/* fill the hole with PHY_IDLEs */
+	memset(pb_eop, PHY_IDLE, aligned_phy_len - br->phy_len);
+
+	/* update the phy data length */
+	br->phy_len = aligned_phy_len;
+
+	return 0;
+}
+
+/*
+ * In tx phase, the slave only returns PHY_IDLE (0x4a). So the driver will
+ * ignore rx in tx phase.
+ */
+static int br_do_tx(struct spi_avmm_bridge *br)
+{
+	/* reorder words for spi transfer */
+	if (br->swap_words)
+		br->swap_words(br->phy_buf, br->phy_len);
+
+	/* send all data in phy_buf  */
+	return spi_write(br->spi, br->phy_buf, br->phy_len);
+}
+
+/*
+ * This function read the rx byte stream from SPI word by word and convert
+ * them to transaction layer data in br->trans_buf. It also stores the length
+ * of rx transaction layer data in br->trans_len
+ *
+ * The slave may send an unknown number of PHY_IDLEs in rx phase, so we cannot
+ * prepare a fixed length buffer to receive all of the rx data in a batch. We
+ * have to read word by word and convert them to transaction layer data at
+ * once.
+ */
+static int br_do_rx_and_pkt_phy_parse(struct spi_avmm_bridge *br)
+{
+	bool eop_found = false, channel_found = false, esc_found = false;
+	bool valid_word = false, last_try = false;
+	struct device *dev = &br->spi->dev;
+	char *pb, *tb_limit, *tb = NULL;
+	unsigned long poll_timeout;
+	int ret, i;
+
+	tb_limit = br->trans_buf + ARRAY_SIZE(br->trans_buf);
+	pb = br->phy_buf;
+	poll_timeout = jiffies + SPI_AVMM_XFER_TIMEOUT;
+	while (tb < tb_limit) {
+		ret = spi_read(br->spi, pb, br->word_len);
+		if (ret)
+			return ret;
+
+		/* reorder the word back */
+		if (br->swap_words)
+			br->swap_words(pb, br->word_len);
+
+		valid_word = false;
+		for (i = 0; i < br->word_len; i++) {
+			/* drop everything before first SOP */
+			if (!tb && pb[i] != PKT_SOP)
+				continue;
+
+			/* drop PHY_IDLE */
+			if (pb[i] == PHY_IDLE)
+				continue;
+
+			valid_word = true;
+
+			/*
+			 * We don't support multiple channels, so error out if
+			 * a non-zero channel number is found.
+			 */
+			if (channel_found) {
+				if (pb[i] != 0) {
+					dev_err(dev, "%s channel num != 0\n",
+						__func__);
+					return -EFAULT;
+				}
+
+				channel_found = false;
+				continue;
+			}
+
+			switch (pb[i]) {
+			case PKT_SOP:
+				/*
+				 * reset the parsing if a second SOP appears.
+				 */
+				tb = br->trans_buf;
+				eop_found = false;
+				channel_found = false;
+				esc_found = false;
+				break;
+			case PKT_EOP:
+				/*
+				 * No special char is expected after ESC char.
+				 * No special char (except ESC & PHY_IDLE) is
+				 * expected after EOP char.
+				 *
+				 * The special chars are all dropped.
+				 */
+				if (esc_found || eop_found)
+					return -EFAULT;
+
+				eop_found = true;
+				break;
+			case PKT_CHANNEL:
+				if (esc_found || eop_found)
+					return -EFAULT;
+
+				channel_found = true;
+				break;
+			case PKT_ESC:
+			case PHY_ESC:
+				if (esc_found)
+					return -EFAULT;
+
+				esc_found = true;
+				break;
+			default:
+				/* Record the normal byte in trans_buf. */
+				if (esc_found) {
+					*tb++ = pb[i] ^ 0x20;
+					esc_found = false;
+				} else {
+					*tb++ = pb[i];
+				}
+
+				/*
+				 * We get the last normal byte after EOP, it is
+				 * time we finish. Normally the function should
+				 * return here.
+				 */
+				if (eop_found) {
+					br->trans_len = tb - br->trans_buf;
+					return 0;
+				}
+			}
+		}
+
+		if (valid_word) {
+			/* update poll timeout when we get valid word */
+			poll_timeout = jiffies + SPI_AVMM_XFER_TIMEOUT;
+			last_try = false;
+		} else {
+			/*
+			 * We timeout when rx keeps invalid for some time. But
+			 * it is possible we are scheduled out for long time
+			 * after a spi_read. So when we are scheduled in, a SW
+			 * timeout happens. But actually HW may have worked fine and
+			 * has been ready long time ago. So we need to do an extra
+			 * read, if we get a valid word then we could continue rx,
+			 * otherwise real a HW issue happens.
+			 */
+			if (last_try)
+				return -ETIMEDOUT;
+
+			if (time_after(jiffies, poll_timeout))
+				last_try = true;
+		}
+	}
+
+	/*
+	 * We have used out all transfer layer buffer but cannot find the end
+	 * of the byte stream.
+	 */
+	dev_err(dev, "%s transfer buffer is full but rx doesn't end\n",
+		__func__);
+
+	return -EFAULT;
+}
+
+/*
+ * For read transactions, the avmm bus will directly return register values
+ * without transaction response header.
+ */
+static int br_rd_trans_rx_parse(struct spi_avmm_bridge *br,
+				u32 *val, unsigned int expected_count)
+{
+	unsigned int i, trans_len = br->trans_len;
+	__le32 *data;
+
+	if (expected_count * SPI_AVMM_VAL_SIZE != trans_len)
+		return -EFAULT;
+
+	data = (__le32 *)br->trans_buf;
+	for (i = 0; i < expected_count; i++)
+		*val++ = le32_to_cpu(*data++);
+
+	return 0;
+}
+
+/*
+ * For write transactions, the slave will return a transaction response
+ * header.
+ */
+static int br_wr_trans_rx_parse(struct spi_avmm_bridge *br,
+				unsigned int expected_count)
+{
+	unsigned int trans_len = br->trans_len;
+	struct trans_resp_header *resp;
+	u8 code;
+	u16 val_len;
+
+	if (trans_len != TRANS_RESP_HD_SIZE)
+		return -EFAULT;
+
+	resp = (struct trans_resp_header *)br->trans_buf;
+
+	code = resp->r_code ^ 0x80;
+	val_len = be16_to_cpu(resp->size);
+	if (!val_len || val_len != expected_count * SPI_AVMM_VAL_SIZE)
+		return -EFAULT;
+
+	/* error out if the trans code doesn't align with the val size */
+	if ((val_len == SPI_AVMM_VAL_SIZE && code != TRANS_CODE_WRITE) ||
+	    (val_len > SPI_AVMM_VAL_SIZE && code != TRANS_CODE_SEQ_WRITE))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int do_reg_access(void *context, bool is_read, unsigned int reg,
+			 unsigned int *value, unsigned int count)
+{
+	struct spi_avmm_bridge *br = context;
+	int ret;
+
+	/* invalidate bridge buffers first */
+	br->trans_len = 0;
+	br->phy_len = 0;
+
+	ret = br_trans_tx_prepare(br, is_read, reg, value, count);
+	if (ret)
+		return ret;
+
+	ret = br_pkt_phy_tx_prepare(br);
+	if (ret)
+		return ret;
+
+	ret = br_do_tx(br);
+	if (ret)
+		return ret;
+
+	ret = br_do_rx_and_pkt_phy_parse(br);
+	if (ret)
+		return ret;
+
+	if (is_read)
+		return br_rd_trans_rx_parse(br, value, count);
+	else
+		return br_wr_trans_rx_parse(br, count);
+}
+
+static int regmap_spi_avmm_gather_write(void *context,
+					const void *reg_buf, size_t reg_len,
+					const void *val_buf, size_t val_len)
+{
+	if (reg_len != SPI_AVMM_REG_SIZE)
+		return -EINVAL;
+
+	if (!IS_ALIGNED(val_len, SPI_AVMM_VAL_SIZE))
+		return -EINVAL;
+
+	return do_reg_access(context, false, *(u32 *)reg_buf, (u32 *)val_buf,
+			     val_len / SPI_AVMM_VAL_SIZE);
+}
+
+static int regmap_spi_avmm_write(void *context, const void *data, size_t bytes)
+{
+	if (bytes < SPI_AVMM_REG_SIZE + SPI_AVMM_VAL_SIZE)
+		return -EINVAL;
+
+	return regmap_spi_avmm_gather_write(context, data, SPI_AVMM_REG_SIZE,
+					    data + SPI_AVMM_REG_SIZE,
+					    bytes - SPI_AVMM_REG_SIZE);
+}
+
+static int regmap_spi_avmm_read(void *context,
+				const void *reg_buf, size_t reg_len,
+				void *val_buf, size_t val_len)
+{
+	if (reg_len != SPI_AVMM_REG_SIZE)
+		return -EINVAL;
+
+	if (!IS_ALIGNED(val_len, SPI_AVMM_VAL_SIZE))
+		return -EINVAL;
+
+	return do_reg_access(context, true, *(u32 *)reg_buf, val_buf,
+			     (val_len / SPI_AVMM_VAL_SIZE));
+}
+
+static struct spi_avmm_bridge *
+spi_avmm_bridge_ctx_gen(struct spi_device *spi)
+{
+	struct spi_avmm_bridge *br;
+
+	if (!spi)
+		return ERR_PTR(-ENODEV);
+
+	/* Only support BPW == 8 or 32 now. Try 32 BPW first. */
+	spi->mode = SPI_MODE_1;
+	spi->bits_per_word = 32;
+	if (spi_setup(spi)) {
+		spi->bits_per_word = 8;
+		if (spi_setup(spi))
+			return ERR_PTR(-EINVAL);
+	}
+
+	br = kzalloc(sizeof(*br), GFP_KERNEL);
+	if (!br)
+		return ERR_PTR(-ENOMEM);
+
+	br->spi = spi;
+	br->word_len = spi->bits_per_word / 8;
+	if (br->word_len == 4) {
+		/*
+		 * The protocol requires little endian byte order but MSB
+		 * first. So driver needs to swap the byte order word by word
+		 * if word length > 1.
+		 */
+		br->swap_words = br_swap_words_32;
+	}
+
+	return br;
+}
+
+static void spi_avmm_bridge_ctx_free(void *context)
+{
+	kfree(context);
+}
+
+static const struct regmap_bus regmap_spi_avmm_bus = {
+	.write = regmap_spi_avmm_write,
+	.gather_write = regmap_spi_avmm_gather_write,
+	.read = regmap_spi_avmm_read,
+	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+	.max_raw_read = SPI_AVMM_VAL_SIZE * MAX_READ_CNT,
+	.max_raw_write = SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT,
+	.free_context = spi_avmm_bridge_ctx_free,
+};
+
+struct regmap *__regmap_init_spi_avmm(struct spi_device *spi,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name)
+{
+	struct spi_avmm_bridge *bridge;
+	struct regmap *map;
+
+	bridge = spi_avmm_bridge_ctx_gen(spi);
+	if (IS_ERR(bridge))
+		return ERR_CAST(bridge);
+
+	map = __regmap_init(&spi->dev, &regmap_spi_avmm_bus,
+			    bridge, config, lock_key, lock_name);
+	if (IS_ERR(map)) {
+		spi_avmm_bridge_ctx_free(bridge);
+		return ERR_CAST(map);
+	}
+
+	return map;
+}
+EXPORT_SYMBOL_GPL(__regmap_init_spi_avmm);
+
+struct regmap *__devm_regmap_init_spi_avmm(struct spi_device *spi,
+					   const struct regmap_config *config,
+					   struct lock_class_key *lock_key,
+					   const char *lock_name)
+{
+	struct spi_avmm_bridge *bridge;
+	struct regmap *map;
+
+	bridge = spi_avmm_bridge_ctx_gen(spi);
+	if (IS_ERR(bridge))
+		return ERR_CAST(bridge);
+
+	map = __devm_regmap_init(&spi->dev, &regmap_spi_avmm_bus,
+				 bridge, config, lock_key, lock_name);
+	if (IS_ERR(map)) {
+		spi_avmm_bridge_ctx_free(bridge);
+		return ERR_CAST(map);
+	}
+
+	return map;
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_spi_avmm);
+
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1970ed5..d865d8f 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -567,6 +567,10 @@ struct regmap *__regmap_init_sdw(struct sdw_slave *sdw,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__regmap_init_spi_avmm(struct spi_device *spi,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name);
 
 struct regmap *__devm_regmap_init(struct device *dev,
 				  const struct regmap_bus *bus,
@@ -620,6 +624,10 @@ struct regmap *__devm_regmap_init_i3c(struct i3c_device *i3c,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__devm_regmap_init_spi_avmm(struct spi_device *spi,
+					   const struct regmap_config *config,
+					   struct lock_class_key *lock_key,
+					   const char *lock_name);
 /*
  * Wrapper for regmap_init macros to include a unique lockdep key and name
  * for each call. No-op if CONFIG_LOCKDEP is not set.
@@ -806,6 +814,19 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__regmap_init_sdw, #config,		\
 				sdw, config)
 
+/**
+ * regmap_init_spi_avmm() - Initialize register map for Intel SPI Slave
+ * to AVMM Bus Bridge
+ *
+ * @spi: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.
+ */
+#define regmap_init_spi_avmm(spi, config)					\
+	__regmap_lockdep_wrapper(__regmap_init_spi_avmm, #config,		\
+				 spi, config)
 
 /**
  * devm_regmap_init() - Initialise managed register map
@@ -993,6 +1014,21 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__devm_regmap_init_i3c, #config,	\
 				i3c, config)
 
+/**
+ * devm_regmap_init_spi_avmm() - Initialize register map for Intel SPI Slave
+ * to AVMM Bus Bridge
+ *
+ * @spi: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The map will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_spi_avmm(spi, config)				\
+	__regmap_lockdep_wrapper(__devm_regmap_init_spi_avmm, #config,	\
+				 spi, config)
+
 int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk);
 void regmap_mmio_detach_clk(struct regmap *map);
 void regmap_exit(struct regmap *map);
-- 
2.7.4

[PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Xu Yilun]

This patch implements the basic functions of the BMC chip for some Intel
FPGA PCIe Acceleration Cards (PAC). The BMC is implemented using the
intel max10 CPLD.

This BMC chip is connected to FPGA by a SPI bus. To provide direct
register access from FPGA, the "SPI slave to Avalon Master Bridge"
(spi-avmm) IP is integrated in the chip. It converts encoded streams of
bytes from the host to the internal register read/write on Avalon bus.
So This driver uses the regmap-spi-avmm for register accessing.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v2: Split out the regmap-spi-avmm part.
    Rename the file intel-m10-bmc-main.c to intel-m10-bmc.c, cause
     there is only one source file left for this module now.
v3: add the sub devices in mfd_cell.
    some minor fixes.
v4: no change.
---
 .../ABI/testing/sysfs-driver-intel-m10-bmc         |  15 ++
 drivers/mfd/Kconfig                                |  13 ++
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/intel-m10-bmc.c                        | 169 +++++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h                  |  57 +++++++
 5 files changed, 256 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
 create mode 100644 drivers/mfd/intel-m10-bmc.c
 create mode 100644 include/linux/mfd/intel-m10-bmc.h

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
new file mode 100644
index 0000000..979a2d6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
@@ -0,0 +1,15 @@
+What:		/sys/bus/spi/devices/.../bmc_version
+Date:		June 2020
+KernelVersion:	5.10
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the hardware build version of Intel
+		MAX10 BMC chip.
+		Format: "0x%x".
+
+What:		/sys/bus/spi/devices/.../bmcfw_version
+Date:		June 2020
+KernelVersion:	5.10
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the firmware version of Intel MAX10
+		BMC chip.
+		Format: "0x%x".
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 33df083..57745f5 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2118,5 +2118,18 @@ config SGI_MFD_IOC3
 	  If you have an SGI Origin, Octane, or a PCI IOC3 card,
 	  then say Y. Otherwise say N.
 
+config MFD_INTEL_M10_BMC
+	tristate "Intel MAX10 Board Management Controller"
+	depends on SPI_MASTER
+	select REGMAP_SPI_AVMM
+	select MFD_CORE
+	help
+	  Support for the Intel MAX10 board management controller using the
+	  SPI interface.
+
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the functionality
+	  of the device.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a60e5f8..dd2cc7b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -264,3 +264,5 @@ obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
 obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
+
+obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
new file mode 100644
index 0000000..0dfd73a
--- /dev/null
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Max10 Board Management Controller chip
+ *
+ * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+enum m10bmc_type {
+	M10_N3000,
+};
+
+static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
+	{
+		.name = "n3000bmc-hwmon",
+	},
+	{
+		.name = "n3000bmc-pkvl",
+	},
+	{
+		.name = "m10bmc-secure",
+	},
+
+};
+
+static struct regmap_config intel_m10bmc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = M10BMC_MEM_END,
+};
+
+static ssize_t bmc_version_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct intel_m10bmc *m10bmc = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(m10bmc, M10BMC_BUILD_VER, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", val);
+}
+static DEVICE_ATTR_RO(bmc_version);
+
+static ssize_t bmcfw_version_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct intel_m10bmc *max10 = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(max10, NIOS2_FW_VERSION, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", val);
+}
+static DEVICE_ATTR_RO(bmcfw_version);
+
+static struct attribute *m10bmc_attrs[] = {
+	&dev_attr_bmc_version.attr,
+	&dev_attr_bmcfw_version.attr,
+	NULL,
+};
+
+static struct attribute_group m10bmc_attr_group = {
+	.attrs = m10bmc_attrs,
+};
+
+static const struct attribute_group *m10bmc_dev_groups[] = {
+	&m10bmc_attr_group,
+	NULL
+};
+
+static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
+{
+	unsigned int v;
+
+	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
+			    &v))
+		return -ENODEV;
+
+	if (v != 0xffffffff) {
+		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int intel_m10_bmc_spi_probe(struct spi_device *spi)
+{
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	struct device *dev = &spi->dev;
+	struct mfd_cell *cells;
+	struct intel_m10bmc *m10bmc;
+	int ret, n_cell;
+
+	m10bmc = devm_kzalloc(dev, sizeof(*m10bmc), GFP_KERNEL);
+	if (!m10bmc)
+		return -ENOMEM;
+
+	m10bmc->dev = dev;
+
+	m10bmc->regmap =
+		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
+	if (IS_ERR(m10bmc->regmap)) {
+		ret = PTR_ERR(m10bmc->regmap);
+		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
+		return ret;
+	}
+
+	spi_set_drvdata(spi, m10bmc);
+
+	ret = check_m10bmc_version(m10bmc);
+	if (ret) {
+		dev_err(dev, "Failed to identify m10bmc hardware\n");
+		return ret;
+	}
+
+	switch (id->driver_data) {
+	case M10_N3000:
+		cells = m10bmc_pacn3000_subdevs;
+		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
+				   NULL, 0, NULL);
+	if (ret)
+		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
+
+	return ret;
+}
+
+static const struct spi_device_id m10bmc_spi_id[] = {
+	{ "m10-n3000", M10_N3000 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
+
+static struct spi_driver intel_m10bmc_spi_driver = {
+	.driver = {
+		.name = "intel-m10-bmc",
+		.dev_groups = m10bmc_dev_groups,
+	},
+	.probe = intel_m10_bmc_spi_probe,
+	.id_table = m10bmc_spi_id,
+};
+
+module_spi_driver(intel_m10bmc_spi_driver);
+
+MODULE_DESCRIPTION("Intel Max10 BMC Device Driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:intel-m10-bmc");
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
new file mode 100644
index 0000000..4979756
--- /dev/null
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Driver Header File for Intel Max10 Board Management Controller chip.
+ *
+ * Copyright (C) 2018-2020 Intel Corporation, Inc.
+ *
+ */
+#ifndef __INTEL_M10_BMC_H
+#define __INTEL_M10_BMC_H
+
+#include <linux/regmap.h>
+
+#define M10BMC_LEGACY_SYS_BASE	0x300400
+#define M10BMC_SYS_BASE		0x300800
+#define M10BMC_MEM_END		0x200000fc
+
+/* Register offset of system registers */
+#define NIOS2_FW_VERSION	0x0
+#define M10BMC_TEST_REG		0x3c
+#define M10BMC_BUILD_VER	0x68
+#define M10BMC_VER_MAJOR_MSK	GENMASK(23, 16)
+#define M10BMC_VER_PCB_INFO_MSK	GENMASK(31, 24)
+
+/**
+ * struct intel_m10bmc - Intel Max10 BMC MFD device private data structure
+ * @dev: this device
+ * @regmap: the regmap used to access registers by m10bmc itself
+ */
+struct intel_m10bmc {
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+/*
+ * register access helper functions.
+ *
+ * m10bmc_raw_read - read m10bmc register per addr
+ * m10bmc_sys_read - read m10bmc system register per offset
+ */
+static inline int
+m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
+		unsigned int *val)
+{
+	int ret;
+
+	ret = regmap_read(m10bmc->regmap, addr, val);
+	if (ret)
+		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
+			addr, ret);
+
+	return ret;
+}
+
+#define m10bmc_sys_read(m10bmc, offset, val) \
+	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
+
+#endif /* __INTEL_M10_BMC_H */
-- 
2.7.4

Re: [PATCH v4 0/2] add regmap-spi-avmm & Intel Max10 BMC chip support

[Mark Brown]

On Wed, 19 Aug 2020 15:34:55 +0800, Xu Yilun wrote:
> This patchset adds the regmap-spi-avmm to support the Intel SPI Slave to
> AVMM Bus Bridge (spi-avmm) IP block. It also implements the usercase - the
> driver of Intel Max10 BMC chip which integrates this IP block.
> 
> Patch #1 implements the regmap-spi-avmm.
> Patch #2 implements the mfd driver of Intel Max10 BMC chip.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/2] regmap: add Intel SPI Slave to AVMM Bus Bridge support
      commit: 7f9fb67358a2bcaacbdfeee12e0f19e98c8bdf55
[2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
      commit: 53be8bbc2f4058d4a6bfff3dadf34164bcaafa87

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v4 0/2] add regmap-spi-avmm & Intel Max10 BMC chip support

[Lee Jones]

On Wed, 26 Aug 2020, Mark Brown wrote:

> On Wed, 19 Aug 2020 15:34:55 +0800, Xu Yilun wrote:
> > This patchset adds the regmap-spi-avmm to support the Intel SPI Slave to
> > AVMM Bus Bridge (spi-avmm) IP block. It also implements the usercase - the
> > driver of Intel Max10 BMC chip which integrates this IP block.
> > 
> > Patch #1 implements the regmap-spi-avmm.
> > Patch #2 implements the mfd driver of Intel Max10 BMC chip.
> > 
> > [...]
> 
> Applied to
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
> 
> Thanks!
> 
> [1/2] regmap: add Intel SPI Slave to AVMM Bus Bridge support
>       commit: 7f9fb67358a2bcaacbdfeee12e0f19e98c8bdf55

> [2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
>       commit: 53be8bbc2f4058d4a6bfff3dadf34164bcaafa87

Que?  This is yet to be reviewed.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 0/2] add regmap-spi-avmm & Intel Max10 BMC chip support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

On Thu, Aug 27, 2020 at 07:56:47AM +0100, Lee Jones wrote:
> On Wed, 26 Aug 2020, Mark Brown wrote:

> > [2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
> >       commit: 53be8bbc2f4058d4a6bfff3dadf34164bcaafa87

> Que?  This is yet to be reviewed.

Sorry, meant to only fetch the first patch.  Dropped now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 0/2] add regmap-spi-avmm & Intel Max10 BMC chip support

[Lee Jones]

On Thu, 27 Aug 2020, Mark Brown wrote:

> On Thu, Aug 27, 2020 at 07:56:47AM +0100, Lee Jones wrote:
> > On Wed, 26 Aug 2020, Mark Brown wrote:
> 
> > > [2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
> > >       commit: 53be8bbc2f4058d4a6bfff3dadf34164bcaafa87
> 
> > Que?  This is yet to be reviewed.
> 
> Sorry, meant to only fetch the first patch.  Dropped now.

Thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Lee Jones]

On Wed, 19 Aug 2020, Xu Yilun wrote:

> This patch implements the basic functions of the BMC chip for some Intel
> FPGA PCIe Acceleration Cards (PAC). The BMC is implemented using the
> intel max10 CPLD.

Nit: "Intel MAX 10"

> This BMC chip is connected to FPGA by a SPI bus. To provide direct

Nit: "to *the* FPGA"

> register access from FPGA, the "SPI slave to Avalon Master Bridge"

Nit: "from *the* FPGA"

> (spi-avmm) IP is integrated in the chip. It converts encoded streams of
> bytes from the host to the internal register read/write on Avalon bus.

Nit: "on *the* Avalon bus"

> So This driver uses the regmap-spi-avmm for register accessing.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v2: Split out the regmap-spi-avmm part.
>     Rename the file intel-m10-bmc-main.c to intel-m10-bmc.c, cause
>      there is only one source file left for this module now.
> v3: add the sub devices in mfd_cell.
>     some minor fixes.
> v4: no change.
> ---
>  .../ABI/testing/sysfs-driver-intel-m10-bmc         |  15 ++
>  drivers/mfd/Kconfig                                |  13 ++
>  drivers/mfd/Makefile                               |   2 +
>  drivers/mfd/intel-m10-bmc.c                        | 169 +++++++++++++++++++++
>  include/linux/mfd/intel-m10-bmc.h                  |  57 +++++++
>  5 files changed, 256 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
>  create mode 100644 drivers/mfd/intel-m10-bmc.c
>  create mode 100644 include/linux/mfd/intel-m10-bmc.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> new file mode 100644
> index 0000000..979a2d6
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> @@ -0,0 +1,15 @@
> +What:		/sys/bus/spi/devices/.../bmc_version
> +Date:		June 2020
> +KernelVersion:	5.10
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the hardware build version of Intel
> +		MAX10 BMC chip.
> +		Format: "0x%x".
> +
> +What:		/sys/bus/spi/devices/.../bmcfw_version
> +Date:		June 2020
> +KernelVersion:	5.10
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the firmware version of Intel MAX10
> +		BMC chip.
> +		Format: "0x%x".
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df083..57745f5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2118,5 +2118,18 @@ config SGI_MFD_IOC3
>  	  If you have an SGI Origin, Octane, or a PCI IOC3 card,
>  	  then say Y. Otherwise say N.
>  
> +config MFD_INTEL_M10_BMC
> +	tristate "Intel MAX10 Board Management Controller"
> +	depends on SPI_MASTER
> +	select REGMAP_SPI_AVMM
> +	select MFD_CORE
> +	help
> +	  Support for the Intel MAX10 board management controller using the
> +	  SPI interface.
> +
> +	  This driver provides common support for accessing the device,
> +	  additional drivers must be enabled in order to use the functionality
> +	  of the device.
> +
>  endmenu
>  endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a60e5f8..dd2cc7b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -264,3 +264,5 @@ obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
>  obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  
>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
> +
> +obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> new file mode 100644
> index 0000000..0dfd73a
> --- /dev/null
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Max10 Board Management Controller chip

"Intel MAX 10"

> + * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> + *

Remove this line.

> + */
> +#include <linux/bitfield.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>

Alphabetical.

> +enum m10bmc_type {
> +	M10_N3000,
> +};

Seems over-kill.  Will there be others?

> +static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
> +	{
> +		.name = "n3000bmc-hwmon",
> +	},
> +	{
> +		.name = "n3000bmc-pkvl",
> +	},
> +	{
> +		.name = "m10bmc-secure",
> +	},

Each entry on one line please.

> +

Remove this line.

> +};
> +
> +static struct regmap_config intel_m10bmc_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = M10BMC_MEM_END,
> +};
> +
> +static ssize_t bmc_version_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{

Does this line up to the '(' in code?

> +	struct intel_m10bmc *m10bmc = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = m10bmc_sys_read(m10bmc, M10BMC_BUILD_VER, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "0x%x\n", val);

Is this safe?  Have you considered snprintf()?

> +}
> +static DEVICE_ATTR_RO(bmc_version);
> +
> +static ssize_t bmcfw_version_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct intel_m10bmc *max10 = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = m10bmc_sys_read(max10, NIOS2_FW_VERSION, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "0x%x\n", val);

As above.

> +}
> +static DEVICE_ATTR_RO(bmcfw_version);
> +
> +static struct attribute *m10bmc_attrs[] = {
> +	&dev_attr_bmc_version.attr,
> +	&dev_attr_bmcfw_version.attr,
> +	NULL,
> +};

Can this be const?

> +static struct attribute_group m10bmc_attr_group = {
> +	.attrs = m10bmc_attrs,
> +};

Can this be const?

> +static const struct attribute_group *m10bmc_dev_groups[] = {
> +	&m10bmc_attr_group,
> +	NULL

Comma (like above).

> +};
> +
> +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> +{
> +	unsigned int v;
> +
> +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> +			    &v))
> +		return -ENODEV;

Please break functions out of if statements.

Does m10bmc_raw_read() return 0 on success?

Seems odd for a read function.

> +	if (v != 0xffffffff) {
> +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> +		return -ENODEV;
> +	}

The only acceptable version is -1?

> +	return 0;
> +}
> +
> +static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	struct device *dev = &spi->dev;
> +	struct mfd_cell *cells;
> +	struct intel_m10bmc *m10bmc;

Prefer the generic 'ddata'.

> +	int ret, n_cell;
> +
> +	m10bmc = devm_kzalloc(dev, sizeof(*m10bmc), GFP_KERNEL);
> +	if (!m10bmc)
> +		return -ENOMEM;
> +
> +	m10bmc->dev = dev;
> +
> +	m10bmc->regmap =
> +		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
> +	if (IS_ERR(m10bmc->regmap)) {
> +		ret = PTR_ERR(m10bmc->regmap);
> +		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	spi_set_drvdata(spi, m10bmc);
> +
> +	ret = check_m10bmc_version(m10bmc);
> +	if (ret) {
> +		dev_err(dev, "Failed to identify m10bmc hardware\n");
> +		return ret;
> +	}
> +
> +	switch (id->driver_data) {
> +	case M10_N3000:
> +		cells = m10bmc_pacn3000_subdevs;
> +		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> +		break;
> +	default:
> +		return -ENODEV;
> +	}

Will there be other versions?

> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> +				   NULL, 0, NULL);
> +	if (ret)
> +		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static const struct spi_device_id m10bmc_spi_id[] = {
> +	{ "m10-n3000", M10_N3000 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> +
> +static struct spi_driver intel_m10bmc_spi_driver = {
> +	.driver = {
> +		.name = "intel-m10-bmc",
> +		.dev_groups = m10bmc_dev_groups,
> +	},
> +	.probe = intel_m10_bmc_spi_probe,
> +	.id_table = m10bmc_spi_id,
> +};
> +

Remove this line.

> +module_spi_driver(intel_m10bmc_spi_driver);
> +
> +MODULE_DESCRIPTION("Intel Max10 BMC Device Driver");

"Intel MAX 10"

> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:intel-m10-bmc");
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> new file mode 100644
> index 0000000..4979756
> --- /dev/null
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Driver Header File for Intel Max10 Board Management Controller chip.

Please drop the "Driver Header File for" part.

> + * Copyright (C) 2018-2020 Intel Corporation, Inc.
> + *

Remove this line.

> + */
> +#ifndef __INTEL_M10_BMC_H
> +#define __INTEL_M10_BMC_H

"__MFD_INTEL..."

> +#include <linux/regmap.h>
> +
> +#define M10BMC_LEGACY_SYS_BASE	0x300400
> +#define M10BMC_SYS_BASE		0x300800
> +#define M10BMC_MEM_END		0x200000fc
> +
> +/* Register offset of system registers */
> +#define NIOS2_FW_VERSION	0x0
> +#define M10BMC_TEST_REG		0x3c
> +#define M10BMC_BUILD_VER	0x68
> +#define M10BMC_VER_MAJOR_MSK	GENMASK(23, 16)
> +#define M10BMC_VER_PCB_INFO_MSK	GENMASK(31, 24)
> +
> +/**
> + * struct intel_m10bmc - Intel Max10 BMC MFD device private data structure

"Intel MAX 10 BMC parent driver data structure"

> + * @dev: this device
> + * @regmap: the regmap used to access registers by m10bmc itself
> + */
> +struct intel_m10bmc {
> +	struct device *dev;
> +	struct regmap *regmap;
> +};
> +
> +/*
> + * register access helper functions.
> + *
> + * m10bmc_raw_read - read m10bmc register per addr
> + * m10bmc_sys_read - read m10bmc system register per offset
> + */
> +static inline int
> +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> +		unsigned int *val)
> +{
> +	int ret;
> +
> +	ret = regmap_read(m10bmc->regmap, addr, val);
> +	if (ret)
> +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> +			addr, ret);
> +
> +	return ret;
> +}
> +
> +#define m10bmc_sys_read(m10bmc, offset, val) \
> +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)

No unnecessary abstractions.

Just use the Regmap API directly please.

> +#endif /* __INTEL_M10_BMC_H */

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Tom Rix]

>> +
>> +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
>> +{
>> +	unsigned int v;
>> +
>> +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
>> +			    &v))
>> +		return -ENODEV;
> Please break functions out of if statements.
>
> Does m10bmc_raw_read() return 0 on success?
>
> Seems odd for a read function.
>
>> +	if (v != 0xffffffff) {
>> +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
>> +		return -ENODEV;
>> +	}
> The only acceptable version is -1?

I ran into this in testing.  This is a check if the board is using a very old legacy bmc version. The M10BMC_LEGACY_SYS_BASE is the offset to this old block of mmio regs.  On the old boards, v would have not been f's, on the current boards it is f's. The check is necessary because future calls use the M10BMC_SYS_BASE offset which was not valid on the old boards.

Tom

Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Xu Yilun]

Thanks for your comments, I'll fix them. Some comments inline.

On Fri, Aug 28, 2020 at 11:02:36AM +0100, Lee Jones wrote:
> On Wed, 19 Aug 2020, Xu Yilun wrote:
> 
> 
> > +enum m10bmc_type {
> > +	M10_N3000,
> > +};
> 
> Seems over-kill.  Will there be others?

There will be another version of the BMC which support the Intel PAC
D5005. The functionalities are similar with N3000. So I defined the
device type enum here.

> > +static ssize_t bmc_version_show(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> 
> Does this line up to the '(' in code?

Yes, It does.

> 
> > +	struct intel_m10bmc *m10bmc = dev_get_drvdata(dev);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = m10bmc_sys_read(m10bmc, M10BMC_BUILD_VER, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sprintf(buf, "0x%x\n", val);
> 
> Is this safe?  Have you considered snprintf()?

It formats a 32 bits value to string, so the strlen should be 8 chars at
most. So I think it should be safe here.

I see in Documentation/filesystems/sysfs.rst:
- show() must not use snprintf() when formatting the value to be
  returned to user space. If you can guarantee that an overflow
  will never happen you can use sprintf() otherwise you must use
  scnprintf().

And seeing from this mail https://lkml.org/lkml/2019/4/25/1050
Greg is discouraging use of scnprintf for sysfs attributes where it's not
needed.

> > +}
> > +static DEVICE_ATTR_RO(bmcfw_version);
> > +
> > +static struct attribute *m10bmc_attrs[] = {
> > +	&dev_attr_bmc_version.attr,
> > +	&dev_attr_bmcfw_version.attr,
> > +	NULL,
> > +};
> 
> Can this be const?

Seems we can not const this pointer or this array.

If we const the array, static const struct attribute *m10bmc_attrs[],
then:
	error: initialization from incompatible pointer type

If we const the pointer, static struct attribute * const m10bmc_attrs[],
then:
	warning: initialization discards ‘const’ qualifier from pointer
		 target type

> 
> > +static struct attribute_group m10bmc_attr_group = {
> > +	.attrs = m10bmc_attrs,
> > +};
> 
> Can this be const?

Yes, we can const it.

> 
> > +static const struct attribute_group *m10bmc_dev_groups[] = {
> > +	&m10bmc_attr_group,
> > +	NULL
> 
> > +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> > +{
> > +	unsigned int v;
> > +
> > +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> > +			    &v))
> > +		return -ENODEV;
> 
> Please break functions out of if statements.
> 
> Does m10bmc_raw_read() return 0 on success?

Yes, this function just wrappered the regmap_read()

> 
> Seems odd for a read function.
> 
> > +	if (v != 0xffffffff) {
> > +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> > +		return -ENODEV;
> > +	}
> 
> The only acceptable version is -1?

As mentioned by Tom, this is a check if the board is using a very old legacy
bmc version, the driver doesn't mean to support this old legacy bmc
version.


> > +	case M10_N3000:
> > +		cells = m10bmc_pacn3000_subdevs;
> > +		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> > +		break;
> > +	default:
> > +		return -ENODEV;
> > +	}
> 
> Will there be other versions?

There will be a M10_D5005, we haven't fully prepared the code yet, but I
mean to reserve the place for it.

> > + * m10bmc_raw_read - read m10bmc register per addr
> > + * m10bmc_sys_read - read m10bmc system register per offset
> > + */
> > +static inline int
> > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > +		unsigned int *val)
> > +{
> > +	int ret;
> > +
> > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > +	if (ret)
> > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > +			addr, ret);
> > +
> > +	return ret;
> > +}
> > +
> > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> 
> No unnecessary abstractions.
> 
> Just use the Regmap API directly please.

Could we keep the 2 definition?

For m10bmc_raw_read(), we make it to help print some error info if
regmap RW fail. So we don't have to write "if (ret) dev_err" every time
we use regmap.

For m10bmc_sys_read(), the offset of BMC system registers could be
configured by HW developers (The MAX 10 is an CPLD, it could be easily
reprogrammed). And the HW SPEC will not add the offset when describing
the addresses of system registers. So:
1. It makes the definition of system registers in code align with HW SPEC.
2. It makes developers easier to make changes when the offset is adjusted
   in HW (I've been told by HW guys, it is sometimes necessary to adjust
   the offset when changing RTL, required by Altera EDA tool - Quartus).

Thanks,
Yilun

> 
> -- 
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Lee Jones]

On Fri, 28 Aug 2020, Tom Rix wrote:

> 
> >> +
> >> +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> >> +{
> >> +	unsigned int v;
> >> +
> >> +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> >> +			    &v))
> >> +		return -ENODEV;
> > Please break functions out of if statements.
> >
> > Does m10bmc_raw_read() return 0 on success?
> >
> > Seems odd for a read function.
> >
> >> +	if (v != 0xffffffff) {
> >> +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> >> +		return -ENODEV;
> >> +	}
> > The only acceptable version is -1?
> 
> I ran into this in testing.  This is a check if the board is using a
> very old legacy bmc version. The M10BMC_LEGACY_SYS_BASE is the
> offset to this old block of mmio regs.  On the old boards, v would
> have not been f's, on the current boards it is f's. The check is
> necessary because future calls use the M10BMC_SYS_BASE offset which
> was not valid on the old boards.

This should be made more clear.  Either as a comment or as a define.

Preferably both!

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Lee Jones]

> > > +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> > > +{
> > > +	unsigned int v;
> > > +
> > > +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> > > +			    &v))
> > > +		return -ENODEV;
> > 
> > Please break functions out of if statements.
> > 
> > Does m10bmc_raw_read() return 0 on success?
> 
> Yes, this function just wrappered the regmap_read()

Avoid unnecessarily wrapping functions if possible.

> > Seems odd for a read function.
> > 
> > > +	if (v != 0xffffffff) {
> > > +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > The only acceptable version is -1?
> 
> As mentioned by Tom, this is a check if the board is using a very old legacy
> bmc version, the driver doesn't mean to support this old legacy bmc
> version.

Please add a descriptive comment and define the value.

> > > + * m10bmc_raw_read - read m10bmc register per addr
> > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > + */
> > > +static inline int
> > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > +		unsigned int *val)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > +	if (ret)
> > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > +			addr, ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > 
> > No unnecessary abstractions.
> > 
> > Just use the Regmap API directly please.
> 
> Could we keep the 2 definition?
> 
> For m10bmc_raw_read(), we make it to help print some error info if
> regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> we use regmap.

How many call sites are there?

> For m10bmc_sys_read(), the offset of BMC system registers could be
> configured by HW developers (The MAX 10 is an CPLD, it could be easily
> reprogrammed). And the HW SPEC will not add the offset when describing
> the addresses of system registers. So:
> 1. It makes the definition of system registers in code align with HW SPEC.
> 2. It makes developers easier to make changes when the offset is adjusted
>    in HW (I've been told by HW guys, it is sometimes necessary to adjust
>    the offset when changing RTL, required by Altera EDA tool - Quartus).

Make sure you justify this for the function(s) you keep.

I'll take a closer look on the next submission.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Xu Yilun]

> > > > + * m10bmc_raw_read - read m10bmc register per addr
> > > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > > + */
> > > > +static inline int
> > > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > > +		unsigned int *val)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > > +	if (ret)
> > > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > > +			addr, ret);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > > 
> > > No unnecessary abstractions.
> > > 
> > > Just use the Regmap API directly please.
> > 
> > Could we keep the 2 definition?
> > 
> > For m10bmc_raw_read(), we make it to help print some error info if
> > regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> > we use regmap.
> 
> How many call sites are there?

There are about 20 calls of the register read in m10bmc base driver and
sub device drivers. Most of them calls m10bmc_sys_read().
I prefer to keep the function for unified error log, but I'm also good
to follow your opinion. How do you think?

I also realize that it is not necessary that we define so many
m10bmc_raw_bulk_read/bulk_write/update_bits ... which are not frequently
used. We could change them.

> 
> > For m10bmc_sys_read(), the offset of BMC system registers could be
> > configured by HW developers (The MAX 10 is an CPLD, it could be easily
> > reprogrammed). And the HW SPEC will not add the offset when describing
> > the addresses of system registers. So:
> > 1. It makes the definition of system registers in code align with HW SPEC.
> > 2. It makes developers easier to make changes when the offset is adjusted
> >    in HW (I've been told by HW guys, it is sometimes necessary to adjust
> >    the offset when changing RTL, required by Altera EDA tool - Quartus).
> 
> Make sure you justify this for the function(s) you keep.

Yes, I could add some comments.

Thanks,
Yilun

Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Lee Jones]

On Wed, 09 Sep 2020, Xu Yilun wrote:

> > > > > + * m10bmc_raw_read - read m10bmc register per addr
> > > > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > > > + */
> > > > > +static inline int
> > > > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > > > +		unsigned int *val)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > > > +	if (ret)
> > > > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > > > +			addr, ret);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > > > 
> > > > No unnecessary abstractions.
> > > > 
> > > > Just use the Regmap API directly please.
> > > 
> > > Could we keep the 2 definition?
> > > 
> > > For m10bmc_raw_read(), we make it to help print some error info if
> > > regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> > > we use regmap.
> > 
> > How many call sites are there?
> 
> There are about 20 calls of the register read in m10bmc base driver and
> sub device drivers. Most of them calls m10bmc_sys_read().
> I prefer to keep the function for unified error log, but I'm also good
> to follow your opinion. How do you think?

Avoidable abstraction is one of my pet hates.  However,
unified/centralised error handling is a valid(ish) reason for
abstraction to exist.  Do you really need to know which read failed?
Is there a case where a read from only a particular register would
fail where others succeed?

> I also realize that it is not necessary that we define so many
> m10bmc_raw_bulk_read/bulk_write/update_bits ... which are not frequently
> used. We could change them.

Yes please.

> > > For m10bmc_sys_read(), the offset of BMC system registers could be
> > > configured by HW developers (The MAX 10 is an CPLD, it could be easily
> > > reprogrammed). And the HW SPEC will not add the offset when describing
> > > the addresses of system registers. So:
> > > 1. It makes the definition of system registers in code align with HW SPEC.
> > > 2. It makes developers easier to make changes when the offset is adjusted
> > >    in HW (I've been told by HW guys, it is sometimes necessary to adjust
> > >    the offset when changing RTL, required by Altera EDA tool - Quartus).
> > 
> > Make sure you justify this for the function(s) you keep.
> 
> Yes, I could add some comments.
> 
> Thanks,
> Yilun

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC

[Xu Yilun]

On Wed, Sep 09, 2020 at 08:31:40AM +0100, Lee Jones wrote:
> On Wed, 09 Sep 2020, Xu Yilun wrote:
> 
> > > > > > + * m10bmc_raw_read - read m10bmc register per addr
> > > > > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > > > > + */
> > > > > > +static inline int
> > > > > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > > > > +		unsigned int *val)
> > > > > > +{
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > > > > +	if (ret)
> > > > > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > > > > +			addr, ret);
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +
> > > > > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > > > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > > > > 
> > > > > No unnecessary abstractions.
> > > > > 
> > > > > Just use the Regmap API directly please.
> > > > 
> > > > Could we keep the 2 definition?
> > > > 
> > > > For m10bmc_raw_read(), we make it to help print some error info if
> > > > regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> > > > we use regmap.
> > > 
> > > How many call sites are there?
> > 
> > There are about 20 calls of the register read in m10bmc base driver and
> > sub device drivers. Most of them calls m10bmc_sys_read().
> > I prefer to keep the function for unified error log, but I'm also good
> > to follow your opinion. How do you think?
> 
> Avoidable abstraction is one of my pet hates.  However,
> unified/centralised error handling is a valid(ish) reason for
> abstraction to exist.  Do you really need to know which read failed?
> Is there a case where a read from only a particular register would
> fail where others succeed?

I think it do helps we know which read failed in the first place when
communication error happens between FPGA & BMC.

Generally, if the error happens randomly on all registers, it may be the
problem of SPI bus.

But it is possible in some case error happens on some registers while
others are fine. The BMC has a internal Avalon mmio bus inside, and sub devices
connect on the bus. But the sub devices may response to the bus read/write
request differently according to hardware design. Once I run into a case
that the sub device stucks on one particular register read for long time
cause it prepares data so slowly. And the driver always timeout on that
register.

Thanks,
Yilun

> 
> > I also realize that it is not necessary that we define so many
> > m10bmc_raw_bulk_read/bulk_write/update_bits ... which are not frequently
> > used. We could change them.
> 
> Yes please.
>