All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] add regmap-spi-avmm & Intel Max10 BMC chip support
@ 2020-07-16 10:42 Xu Yilun
  2020-07-16 10:42 ` [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support Xu Yilun
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Xu Yilun @ 2020-07-16 10:42 UTC (permalink / raw)
  To: broonie, lee.jones, linux-kernel
  Cc: trix, yilun.xu, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu

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 main part of regmap-spi-avmm.
Patch #2 is a fix of the HW issue in spi-avmm IP block.
Patch #3 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. 

Matthew Gerlach (1):
  regmap: spi-avmm: start with the last SOP on phy rx buffer parsing

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              | 932 +++++++++++++++++++++
 drivers/mfd/Kconfig                                |  13 +
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/intel-m10-bmc.c                        | 174 ++++
 include/linux/mfd/intel-m10-bmc.h                  |  57 ++
 include/linux/regmap.h                             |  36 +
 9 files changed, 1235 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support
  2020-07-16 10:42 [PATCH v2 0/3] add regmap-spi-avmm & Intel Max10 BMC chip support Xu Yilun
@ 2020-07-16 10:42 ` Xu Yilun
  2020-07-17 18:15   ` Mark Brown
  2020-07-16 10:42 ` [PATCH v2 2/3] regmap: spi-avmm: start with the last SOP on phy rx buffer parsing Xu Yilun
  2020-07-16 10:42 ` [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC Xu Yilun
  2 siblings, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2020-07-16 10:42 UTC (permalink / raw)
  To: broonie, lee.jones, linux-kernel
  Cc: trix, yilun.xu, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu

This patch add support for regmap API that is 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 mmio read/write on Avalon bus. The
driver implements the read/write protocol for a generic SPI master to
communicate with the spi-avmm IP.

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>
Signed-off-by: Tom Rix <trix@redhat.com>
---
v2: Split out the regmap-spi-avmm as an independent module.
---
 drivers/base/regmap/Kconfig           |   6 +-
 drivers/base/regmap/Makefile          |   1 +
 drivers/base/regmap/regmap-spi-avmm.c | 932 ++++++++++++++++++++++++++++++++++
 include/linux/regmap.h                |  36 ++
 4 files changed, 974 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..ab329d2
--- /dev/null
+++ b/drivers/base/regmap/regmap-spi-avmm.c
@@ -0,0 +1,932 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Intel SPI Slave to AVMM Bus Bridge
+ *
+ * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
+ *
+ */
+
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+
+/*
+ * This driver implements the read/write protocol for a generic SPI master to
+ * communicate with the "SPI slave to Avalon Master Bridge" (spi-avmm) IP.
+ *
+ * The spi-avmm IP act as a bridge to convert encoded streams of bytes from
+ * the host to the internal mmio 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.
+ *
+ * The main function of the physical layer is the use of PHY_IDLE (4a). Host
+ * issues SCLK to query data from slave, but if slave is not ready to submit
+ * data yet, it will repeat PHY_IDLE until data is prepared.
+ * Because of this special char, it also needs an ESCAPE char (4d), to help
+ * represent data "4a". The escape rule is "4d first, following 4a ^ 20".
+ * So "4d, 6a" for data "4a", and "4d, 6d" for data "4d".
+ *
+ * The packet layer defines the boundary of a whole packet. It defines the
+ * Start Of Packet (SOP, 7a) char and End Of Packet (EOP, 7b) char. Please
+ * note that the non-special byte after EOP is the last byte of the packet.
+ * The packet layer also defines a Channel char (7c) + Channel number for
+ * multiple channel transfer. This not currently supported by this driver. So
+ * host will always send "7c, 00" when needed, and will drop the packet if
+ * "7c, non-zero" is received.
+ * Finally, a packet layer ESCAPE char (7d) is also needed to represent a
+ * data value that is the same as the special chars. The escape rule is the
+ * same. The escape rule should be used if the last byte requires it. So if a
+ * packet ends up with data 7a, the last bytes should be "7b, 7d, 5a".
+ *
+ * The transaction layer defines several transaction formats, including host
+ * write/incrementing write request, slave write response, host
+ * read/incrementing read request.
+ *
+ * +------+------------------+------------------------------+
+ * | Byte |       Field      | Description                  |
+ * +------+------------------+------------------------------+
+ * |             Host transaction format                    |
+ * +------+------------------+------------------------------+
+ * |  0   | Transaction code | 0x0, Write non-incrementing  |
+ * |      |                  | 0x4, Write incrementing      |
+ * |      |                  | 0x10, Read non-incrementing  |
+ * |      |                  | 0x14, Read incrementing      |
+ * +------+------------------+------------------------------+
+ * |  1   | Reserved         |                              |
+ * +------+------------------+------------------------------+
+ * | 3:2  | Size             | Big endian                   |
+ * +------+------------------+------------------------------+
+ * | 7:4  | Address          | Big endian                   |
+ * +------+------------------+------------------------------+
+ * | n:8  | Data             | For Write only, Little endian|
+ * +------+------------------+------------------------------+
+ * |       Slave write complete response format             |
+ * +------+------------------+------------------------------+
+ * |  0   | Response code    | Transaction code ^ 0x80      |
+ * +------+------------------+------------------------------+
+ * |  1   | Reserved         |                              |
+ * +------+------------------+------------------------------+
+ * | 3:2  | Size             | Big endian                   |
+ * +------+------------------+------------------------------+
+ *
+ * For a slave read response, there is no transaction header, simply returns
+ * the read out data.
+ *
+ *
+ * Here is a simple case to illustrate the protocol. The host requests
+ * a write32 to addr 0x024b7a40. The following diagram shows how the slave
+ * parses the incoming byte streams from MOSI layer by layer.
+ *
+ *
+ * LSB                Physical layer                            MSB
+ *
+ * |4a|7a|7c|4a|00|00|00|4a|00|04|02|4b|7d|5a|40|4d|6a|ff|03|7b|5f|
+ *  |        |           |                       |  |
+ *  +--------+-----------+                   Escape |
+ *           |                                   +--+
+ *      IDLE, Dropped                               |
+ *                                           Escape dropped,
+ *                                        Next byte XORed with 0x20
+ *                                                  |
+ *                                                  |
+ *                    Packet layer                  |
+ *                                                  |
+ *             |7a|7c|00|00|00|00|04|02|4b|7d|5a|40|4a|ff|03|7b|5f|
+ *              |  |  |                    |  |              |  |
+ *            SOP  +--+                Escape |            EOP  |
+ *                  |                      +--+                 |
+ *                 Channel 0                  |            Last valid byte
+ *                                     Escape dropped,
+ *                              Next byte XORed with 0x20
+ *                                            |
+ *                                            |
+ *                    Transaction layer       |
+ *                                            |
+ *                         |00|00|00|04|02|4b|7a|40|4a|ff|03|5f|
+ *                          |     |  |  |  |  |  |  |  |  |  |
+ *                       Write    +--+  +--+--+--+  +--+--+--+
+ *                          |       |       |           |
+ *                          |   size=4Byte  |           |
+ *                          |       |       |           |
+ *                          +-------+       |           |
+ *                            |             |           |
+ *                         Command      Addr(BE):     Data(LE):
+ *                                     0x024b7a40    0x5f03ff4a
+ *
+ *
+ * This is how host and slave interact for the single write32, only transaction
+ * layer and PHY_IDLE chars are shown for simplicity:
+ *
+ * MOSI  |00|00|00|04|02|4b|3a|40|4a|ff|03|5f|XX|XX|...|XX|XX|XX|XX|XX|
+ * MISO  |4a|4a|4a|4a|4a|4a|4a|4a|4a|4a|4a|4a|4a|4a|...|4a|80|00|00|04|
+ *                                                         |        |
+ *                                      Write done, 0x00 ^ 0x80     |
+ *                                                               4bytes written
+ *
+ * This is another case, a single read32 of addr 0x024b3a40, slave returns
+ * 0x12345678.
+ *
+ * MOSI  |10|00|00|04|02|4b|3a|40|XX|XX|...............|XX|XX|XX|XX|XX|
+ * MISO  |4a|4a|4a|4a|4a|4a|4a|4a|4a|4a|...............|4a|78|56|34|12|
+ *                                                         |  |  |  |
+ *                                                         +--+--+--+
+ *                                                            |
+ *                                                       just return data
+ */
+
+#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 RX_NOT_READY_32		(PHY_IDLE << 24 | PHY_IDLE << 16 |	\
+				 PHY_IDLE << 8 | PHY_IDLE)
+#define RX_NOT_READY_8		PHY_IDLE
+
+#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 REG_SIZE		4UL
+/* slave's register value is 32 bits */
+#define 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_RX_CNT		256UL
+#define MAX_TX_CNT		1UL
+
+struct trans_header {
+	u8 trans_code;
+	u8 rsvd;
+	__be16 size;
+	__be32 addr;
+};
+
+struct trans_response {
+	u8 r_trans_code;
+	u8 rsvd;
+	__be16 size;
+};
+
+#define TRANS_HEAD_SIZE		(sizeof(struct trans_header))
+#define TRANS_RESP_SIZE		(sizeof(struct trans_response))
+
+#define WR_TRANS_TX_SIZE(n)	(TRANS_HEAD_SIZE + VAL_SIZE * (n))
+#define RD_TRANS_TX_SIZE	TRANS_HEAD_SIZE
+
+#define TRANS_TX_MAX		WR_TRANS_TX_SIZE(MAX_TX_CNT)
+/*
+ * In the worst case, all chars are escaped, plus 4 special chars (SOP,
+ * CHANNEL, CHANNEL_NUM, EOP). Finally make sure the length is aligned to SPI
+ * BPW.
+ */
+#define PHY_TX_MAX		ALIGN(2 * TRANS_TX_MAX + 4, 4)
+
+/* No additional chars are in transaction layer RX, just read out data */
+#define TRANS_RX_MAX		(VAL_SIZE * MAX_RX_CNT)
+/*
+ * Unlike tx, phy rx is affected by possible PHY_IDLE bytes from slave,
+ * the driver will read the words one by one and filter out pure IDLE words.
+ * The rest of words may still contain IDLE chars. A worse case could be
+ * receiving word 0x7a7a7a7a in 4 BPW transfer mode. The 4 bytes word may
+ * consume up to 12 bytes in rx buffer, like:
+ * |4a|4a|4a|7d| |5a|7d|5a|7d| |5a|7d|5a|4a|
+ * Besides, the packet layer header may consume up to 8 bytes, like:
+ * |4a|4a|4a|7a| |7c|00|4a|4a|
+ * So the PHY_RX_MAX is calculated as following.
+ */
+#define PHY_RX_MAX		(TRANS_RX_MAX * 3 + 8)
+
+#define TRANS_BUF_SIZE		((TRANS_TX_MAX > TRANS_RX_MAX) ?	\
+				 TRANS_TX_MAX : TRANS_RX_MAX)
+#define PHY_BUF_SIZE		((PHY_TX_MAX > PHY_RX_MAX) ?	\
+				 PHY_TX_MAX : PHY_RX_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.
+ * @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.
+ * @br_swap_words: the word swapping cb for phy data. NULL if not needed.
+ * @is_word_not_ready: the cb to test if there is valid data in a rx word.
+ *
+ * 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 phy_len;
+	/* bridge buffer used in translation between protocol layers */
+	char trans_buf[TRANS_BUF_SIZE];
+	char phy_buf[PHY_BUF_SIZE];
+	void (*br_swap_words)(char *buf, unsigned int len);
+	bool (*is_word_not_ready)(const char *buf);
+};
+
+/*
+ * Transaction Layer
+ *
+ * The transaction layer capsules transaction code (read/write),
+ * size (16 bits), addr (32 bits) and value (32 bits) into a transaction
+ * format. SEQ_READ/WRITE will read/write the number of bytes (specified by
+ * head->size) on incrementing addr start from header->addr field.
+ * The transaction header will be followed by register data for write
+ * operations.
+ *
+ * Please note that size and addr are sent in big endian but value is sent in
+ * little endian according to transaction layer protocol.
+ */
+
+/* Format a transaction layer byte stream for tx_buf */
+static void trans_tx_prepare(bool is_read, u32 reg, u16 count, u32 *wr_val,
+			     char *tx_buf, unsigned int *tx_len)
+{
+	struct trans_header *header;
+	u8 trans_code;
+	__le32 *data;
+	int i;
+
+	trans_code = is_read ?
+			(count == 1 ? TRANS_CODE_READ : TRANS_CODE_SEQ_READ) :
+			(count == 1 ? TRANS_CODE_WRITE : TRANS_CODE_SEQ_WRITE);
+
+	header = (struct trans_header *)tx_buf;
+	header->trans_code = trans_code;
+	header->rsvd = 0;
+	header->size = cpu_to_be16(count * VAL_SIZE);
+	header->addr = cpu_to_be32(reg);
+
+	if (is_read) {
+		*tx_len = RD_TRANS_TX_SIZE;
+	} else {
+		data = (__le32 *)(tx_buf + TRANS_HEAD_SIZE);
+
+		for (i = 0; i < count; i++)
+			*data++ = cpu_to_le32(*wr_val++);
+
+		*tx_len = WR_TRANS_TX_SIZE(count);
+	}
+}
+
+/*
+ * For read transactions, the avmm bus will directly return register values
+ * without transaction response header.
+ */
+static int rd_trans_rx_parse(char *rx_buf, unsigned int rx_len, u32 *val)
+{
+	unsigned int count, i;
+	__le32 *data;
+
+	if (!rx_len || !IS_ALIGNED(rx_len, VAL_SIZE))
+		return -EINVAL;
+
+	count = rx_len / VAL_SIZE;
+
+	data = (__le32 *)rx_buf;
+	for (i = 0; i < count; i++)
+		*val++ = le32_to_cpu(*data++);
+
+	return 0;
+}
+
+/*
+ * For write transactions, the slave will return a transaction response
+ * header.
+ */
+static int wr_trans_rx_parse(char *rx_buf, unsigned int rx_len,
+			     u16 expected_len)
+{
+	struct trans_response *resp;
+	u8 trans_code;
+	u16 val_len;
+
+	if (rx_len != TRANS_RESP_SIZE)
+		return -EINVAL;
+
+	resp = (struct trans_response *)rx_buf;
+
+	trans_code = resp->r_trans_code ^ 0x80;
+	val_len = be16_to_cpu(resp->size);
+	if (!val_len || !IS_ALIGNED(val_len, VAL_SIZE) ||
+	    val_len != expected_len)
+		return -EINVAL;
+
+	/* error out if the trans code doesn't align with what the host sent */
+	if ((val_len == VAL_SIZE && trans_code != TRANS_CODE_WRITE) ||
+	    (val_len > VAL_SIZE && trans_code != TRANS_CODE_SEQ_WRITE))
+		return -EFAULT;
+
+	return 0;
+}
+
+/*
+ * The input is a transaction layer byte stream in rx_buf, the output is read
+ * out data.
+ */
+static int trans_rx_parse(bool is_read, char *rx_buf, unsigned int rx_len,
+			  u16 expected_count, u32 *rd_val)
+{
+	unsigned int expected_len = expected_count * VAL_SIZE;
+
+	if (is_read) {
+		if (expected_len != rx_len)
+			return -EINVAL;
+
+		return rd_trans_rx_parse(rx_buf, rx_len, rd_val);
+	}
+
+	return wr_trans_rx_parse(rx_buf, rx_len, expected_len);
+}
+
+/* Packet Layer & Physical Layer */
+/* The input is a trans stream in trans_buf, format a phy stream in phy_buf. */
+static void pkt_phy_tx_prepare(char *trans_buf, unsigned int trans_len,
+			       char *phy_buf, unsigned int *phy_len)
+{
+	unsigned int i;
+	char *b, *p;
+
+	b = trans_buf;
+	p = phy_buf;
+
+	*p++ = PKT_SOP;
+
+	/*
+	 * The driver doesn't support multiple channels so the channel number
+	 * is always 0.
+	 */
+	*p++ = PKT_CHANNEL;
+	*p++ = 0x0;
+
+	for (i = 0; i < trans_len; i++) {
+		/* EOP should be inserted before the last valid char */
+		if (i == trans_len - 1)
+			*p++ = PKT_EOP;
+
+		/*
+		 * insert an ESCAPE char if the data value equals any special
+		 * char.
+		 */
+		switch (*b) {
+		case PKT_SOP:
+		case PKT_EOP:
+		case PKT_CHANNEL:
+		case PKT_ESC:
+			*p++ = PKT_ESC;
+			*p++ = *b++ ^ 0x20;
+			break;
+		case PHY_IDLE:
+		case PHY_ESC:
+			*p++ = PHY_ESC;
+			*p++ = *b++ ^ 0x20;
+			break;
+		default:
+			*p++ = *b++;
+			break;
+		}
+	}
+
+	*phy_len = p - phy_buf;
+}
+
+/*
+ * The input is a phy stream in pkt_buf, parse out a trans stream in
+ * trans_buf.
+ */
+static int pkt_phy_rx_parse(struct device *dev,
+			    char *phy_buf, unsigned int phy_len,
+			    char *trans_buf, unsigned int *trans_len)
+{
+	char *b, *p;
+
+	b = phy_buf;
+	p = trans_buf;
+
+	/* Find the SOP */
+	while (b < phy_buf + phy_len && *b != PKT_SOP)
+		b++;
+
+	if (b >= phy_buf + phy_len) {
+		dev_err(dev, "%s no SOP\n", __func__);
+		return -EINVAL;
+	}
+
+	b++;
+
+	while (b < phy_buf + phy_len) {
+		switch (*b) {
+		case PHY_IDLE:
+			b++;
+			break;
+		case PKT_CHANNEL:
+			/*
+			 * We don't support multiple channels, so error out if
+			 * a non-zero channel number is found.
+			 */
+			b++;
+			if (*b != 0x0)
+				return -EINVAL;
+			b++;
+			break;
+		case PHY_ESC:
+		case PKT_ESC:
+			b++;
+			*p++ = *b++ ^ 0x20;
+			break;
+		case PKT_SOP:
+			dev_err(dev, "%s 2nd SOP\n", __func__);
+			return -EINVAL;
+		case PKT_EOP:
+			/* the char after EOP is the last valid char */
+			b++;
+
+			switch (*b) {
+			case PHY_ESC:
+			case PKT_ESC:
+			/* the last char may also be escaped */
+				b++;
+				*p++ = *b++ ^ 0x20;
+				break;
+			case PHY_IDLE:
+			case PKT_SOP:
+			case PKT_CHANNEL:
+			case PKT_EOP:
+				dev_err(dev, "%s unexpected 0x%x\n",
+					__func__, *b);
+				return -EINVAL;
+			default:
+				*p++ = *b++;
+				break;
+			}
+
+			*trans_len = p - trans_buf;
+
+			return 0;
+		default:
+			*p++ = *b++;
+			break;
+		}
+	}
+
+	/* We parsed all the bytes but didn't find EOP */
+	dev_err(dev, "%s no EOP\n", __func__);
+	return -EINVAL;
+}
+
+/*
+ * tx_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 void phy_tx_pad(unsigned char word_len, char *phy_buf,
+		       unsigned int phy_len, unsigned int *aligned_len)
+{
+	char *p = &phy_buf[phy_len - 1], *dst_p;
+
+	*aligned_len = ALIGN(phy_len, word_len);
+
+	if (*aligned_len == phy_len)
+		return;
+
+	dst_p = &phy_buf[*aligned_len - 1];
+
+	/* move EOP and bytes after EOP to the end of aligned size */
+	while (p > phy_buf) {
+		*dst_p = *p;
+
+		if (*p == PKT_EOP)
+			break;
+
+		p--;
+		dst_p--;
+	}
+
+	/* fill the hole with PHY_IDLEs */
+	while (p < dst_p)
+		*p++ = PHY_IDLE;
+}
+
+static void br_swap_words_32(char *buf, unsigned int len)
+{
+	unsigned int count;
+	u32 *p = (u32 *)buf;
+
+	count = len / 4;
+	while (count--) {
+		*p = swab32p(p);
+		p++;
+	}
+}
+
+static bool is_word_not_ready_32(const char *rxbuf)
+{
+	return *(u32 *)rxbuf == RX_NOT_READY_32;
+}
+
+static bool is_word_not_ready_8(const char *rxbuf)
+{
+	return *rxbuf == RX_NOT_READY_8;
+}
+
+static int br_rx_word_timeout(struct spi_avmm_bridge *br, char *rxbuf)
+{
+	unsigned long poll_timeout;
+	bool last_try = false;
+	int ret;
+
+	poll_timeout = jiffies + SPI_AVMM_XFER_TIMEOUT;
+	for (;;) {
+		ret = spi_read(br->spi, rxbuf, br->word_len);
+		if (ret)
+			return ret;
+
+		/*
+		 * keep on reading if the rx word does not have a valid
+		 * byte.
+		 */
+		if (!br->is_word_not_ready(rxbuf))
+			break;
+
+		if (last_try)
+			return -ETIMEDOUT;
+
+		/*
+		 * We timeout when rx is 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 we return a valid rx word,
+		 * otherwise real a HW issue happens.
+		 */
+		if (time_after(jiffies, poll_timeout))
+			last_try = true;
+	}
+
+	return 0;
+}
+
+static void br_tx_prepare(struct spi_avmm_bridge *br, bool is_read, u32 reg,
+			  u16 count, u32 *wr_val)
+{
+	unsigned int tx_len;
+
+	trans_tx_prepare(is_read, reg, count, wr_val,
+			 br->trans_buf, &tx_len);
+	pkt_phy_tx_prepare(br->trans_buf, tx_len,
+			   br->phy_buf, &tx_len);
+	phy_tx_pad(br->word_len, br->phy_buf, tx_len, &tx_len);
+
+	br->phy_len = tx_len;
+}
+
+static int br_rx_parse(struct spi_avmm_bridge *br, bool is_read,
+		       u16 expected_count, u32 *rd_val)
+{
+	struct device *dev = &br->spi->dev;
+	unsigned int trans_len;
+	int ret;
+
+	ret = pkt_phy_rx_parse(dev, br->phy_buf, br->phy_len,
+			       br->trans_buf, &trans_len);
+	if (ret) {
+		dev_err(dev, "%s pkt_phy_rx_parse failed %d\n",
+			__func__, ret);
+		goto phy_pkt_rx_err;
+	}
+
+	ret = trans_rx_parse(is_read, br->trans_buf, trans_len,
+			     expected_count, rd_val);
+	if (!ret)
+		return 0;
+
+	dev_err(dev, "%s trans_rx_parse failed %d\n", __func__, ret);
+	print_hex_dump(KERN_DEBUG, "trans rx:", DUMP_PREFIX_OFFSET,
+		       16, 1, br->trans_buf, trans_len, true);
+
+phy_pkt_rx_err:
+	print_hex_dump(KERN_DEBUG, "phy rx:", DUMP_PREFIX_OFFSET,
+		       16, 1, br->phy_buf, br->phy_len, true);
+
+	return ret;
+}
+
+static void rx_max_adjust(const char *w, const char *eop, u8 word_len,
+			  u32 rx_len, u32 *rx_max)
+{
+	u32 remaining_bytes  = w + word_len - 1 - eop, additional_bytes = 0;
+	const char *ptr;
+
+	if (remaining_bytes == 0) {
+		/*
+		 * EOP is the last byte in the word, rx 2 more bytes and
+		 * finish.
+		 */
+		additional_bytes = 2;
+	} else if (remaining_bytes == 1) {
+		/* 1 byte left in the word after EOP. */
+		ptr = eop + 1;
+		/*
+		 * If the byte is an ESCAPE char, rx 1 more byte and
+		 * finish. Otherwise it is OK to finish rx immediately.
+		 */
+		if (*ptr == PHY_ESC || *ptr == PKT_ESC)
+			additional_bytes = 1;
+	}
+	/*
+	 * 2 or more bytes are left in the word after EOP, OK to finish rx
+	 * immediately.
+	 */
+
+	/* Adjust rx_max, make sure we don't exceed the original rx_max. */
+	*rx_max = min(*rx_max, ALIGN(rx_len + additional_bytes, word_len));
+}
+
+/*
+ * In tx phase, the slave only returns PHY_IDLE (0x4a). So the driver will
+ * ignore rx in tx phase.
+ *
+ * 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 filter out pure PHY_IDLE words. The rest of
+ * words have a predictable max length, the driver prepares a large enough
+ * buffer for them. See comments for definition of PHY_RX_MAX.
+ *
+ * When EOP is detected, 1 or 2 (if the last byte is escaped) more bytes should
+ * be received before the rx is finished.
+ */
+static int br_txrx(struct spi_avmm_bridge *br)
+{
+	u32 rx_max = PHY_RX_MAX, rx_len = 0;
+	const char *eop = NULL;
+	u8 wl = br->word_len;
+	char *rxbuf;
+	int ret;
+
+	/* reorder words for spi transfer */
+	if (br->br_swap_words)
+		br->br_swap_words(br->phy_buf, br->phy_len);
+
+	/* send all data in phy_buf  */
+	ret = spi_write(br->spi, br->phy_buf, br->phy_len);
+	if (ret)
+		goto out;
+
+	rxbuf = br->phy_buf;
+	while (rx_len <= rx_max - wl) {
+		/* read word by word */
+		ret = br_rx_word_timeout(br, rxbuf);
+		if (ret)
+			goto out;
+
+		rx_len += wl;
+
+		/* reorder word back now */
+		if (br->br_swap_words)
+			br->br_swap_words(rxbuf, wl);
+
+		if (!eop) {
+			/* find eop */
+			eop = memchr(rxbuf, PKT_EOP, wl);
+			if (eop) {
+				/*
+				 * When EOP is found in the word, then we are
+				 * about to finish rx, there is no need to fill
+				 * the whole phy buf. But if EOP is not the
+				 * last byte in rx stream, we may read 1 or 2
+				 * more bytes and early finish rx by adjusting
+				 * rx_max.
+				 */
+				rx_max_adjust(rxbuf, eop, wl, rx_len, &rx_max);
+			}
+		}
+
+		rxbuf += wl;
+	}
+
+out:
+	br->phy_len = rx_len;
+
+	ret = ret ? : (eop ? 0 : -EINVAL);
+	if (ret) {
+		dev_err(&br->spi->dev, "%s br txrx err %d\n", __func__, ret);
+		print_hex_dump(KERN_DEBUG, "phy rx:", DUMP_PREFIX_OFFSET,
+			       16, 1, br->phy_buf, rx_len, true);
+	}
+	return ret;
+}
+
+static int do_reg_access(void *context, bool is_read, unsigned int reg,
+			 unsigned int *value, u16 count)
+{
+	struct spi_avmm_bridge *br = context;
+	int ret;
+
+	br_tx_prepare(br, is_read, reg, count, value);
+
+	ret = br_txrx(br);
+	if (ret)
+		return ret;
+
+	return br_rx_parse(br, is_read, count, value);
+}
+
+#define do_reg_read(_ctx, _reg, _value, _count) \
+	do_reg_access(_ctx, true, _reg, _value, _count)
+#define do_reg_write(_ctx, _reg, _value, _count) \
+	do_reg_access(_ctx, false, _reg, _value, _count)
+
+static int regmap_spi_avmm_reg_read(void *context, unsigned int reg,
+				    unsigned int *val)
+{
+	return do_reg_read(context, reg, val, 1);
+}
+
+static int regmap_spi_avmm_reg_write(void *context, unsigned int reg,
+				     unsigned int val)
+{
+	return do_reg_write(context, reg, &val, 1);
+}
+
+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 != REG_SIZE)
+		return -EINVAL;
+
+	return do_reg_write(context, *(u32 *)reg_buf, (u32 *)val_buf,
+			    (u16)(val_len / VAL_SIZE));
+}
+
+static int regmap_spi_avmm_write(void *context, const void *data, size_t bytes)
+{
+	if (bytes < REG_SIZE + VAL_SIZE)
+		return -EINVAL;
+
+	return regmap_spi_avmm_gather_write(context, data, REG_SIZE,
+					    data + REG_SIZE, bytes - 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 != REG_SIZE)
+		return -EINVAL;
+
+	return do_reg_read(context, *(u32 *)reg_buf, val_buf,
+			   (u16)(val_len / 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 */
+	if (spi->bits_per_word != 8 && spi->bits_per_word != 32)
+		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) {
+		br->is_word_not_ready = is_word_not_ready_32;
+
+		/*
+		 * Phy layer data is filled byte by byte from low addr to high.
+		 * So the protocol requires LSB first. If the spi device cannot
+		 * do LSB_FIRST transfer, the driver needs to swap the byte
+		 * order word by word.
+		 */
+		if (!(br->spi->mode & SPI_LSB_FIRST))
+			br->br_swap_words = br_swap_words_32;
+	} else if (br->word_len == 1) {
+		br->is_word_not_ready = is_word_not_ready_8;
+	}
+
+	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 = VAL_SIZE * MAX_RX_CNT,
+	.max_raw_write = VAL_SIZE * MAX_TX_CNT,
+
+	.reg_write = regmap_spi_avmm_reg_write,
+	.reg_read = regmap_spi_avmm_reg_read,
+
+	.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 e3817c0..0820b9a 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/3] regmap: spi-avmm: start with the last SOP on phy rx buffer parsing
  2020-07-16 10:42 [PATCH v2 0/3] add regmap-spi-avmm & Intel Max10 BMC chip support Xu Yilun
  2020-07-16 10:42 ` [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support Xu Yilun
@ 2020-07-16 10:42 ` Xu Yilun
  2020-07-17 12:11   ` Mark Brown
  2020-07-16 10:42 ` [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC Xu Yilun
  2 siblings, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2020-07-16 10:42 UTC (permalink / raw)
  To: broonie, lee.jones, linux-kernel
  Cc: trix, yilun.xu, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

This patch works around a bug in the SPI Slave to Avalon Master bridge.
The SPI slave will send an unexpected extra SOP in the following case.

One in approximately one million read requests results in an apparant
stall on the avalon bus where the SPI slave inserts IDLE characters. When
the stall is over, the slave sends an extra SOP character instead of the
0x7c indicating channel. The other characters are correct.

To eliminate the impact of the bug, this patch changes to look for the
last SOP as the start point of the valid phy rx data.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>
Signed-off-by: Tom Rix <trix@redhat.com>
---
v2: no change.
---
 drivers/base/regmap/regmap-spi-avmm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regmap-spi-avmm.c b/drivers/base/regmap/regmap-spi-avmm.c
index ab329d2..632e018 100644
--- a/drivers/base/regmap/regmap-spi-avmm.c
+++ b/drivers/base/regmap/regmap-spi-avmm.c
@@ -433,14 +433,14 @@ static int pkt_phy_rx_parse(struct device *dev,
 {
 	char *b, *p;
 
-	b = phy_buf;
 	p = trans_buf;
 
-	/* Find the SOP */
-	while (b < phy_buf + phy_len && *b != PKT_SOP)
-		b++;
+	/* Find the last SOP */
+	b = (phy_buf + phy_len) - 1;
+	while (b >= phy_buf && *b != PKT_SOP)
+		b--;
 
-	if (b >= phy_buf + phy_len) {
+	if (b < phy_buf) {
 		dev_err(dev, "%s no SOP\n", __func__);
 		return -EINVAL;
 	}
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
  2020-07-16 10:42 [PATCH v2 0/3] add regmap-spi-avmm & Intel Max10 BMC chip support Xu Yilun
  2020-07-16 10:42 ` [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support Xu Yilun
  2020-07-16 10:42 ` [PATCH v2 2/3] regmap: spi-avmm: start with the last SOP on phy rx buffer parsing Xu Yilun
@ 2020-07-16 10:42 ` Xu Yilun
  2020-07-17  9:45   ` Lee Jones
  2020-07-17 18:16   ` Mark Brown
  2 siblings, 2 replies; 14+ messages in thread
From: Xu Yilun @ 2020-07-16 10:42 UTC (permalink / raw)
  To: broonie, lee.jones, linux-kernel
  Cc: trix, yilun.xu, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu

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 reliable
register access from FPGA, an Avalon Memory-Mapped (Avmm) transaction
protocol over the SPI bus is used between host and slave.

This driver implements the basic register access using the
regmap-spi-avmm. The mfd cells array is empty now as a placeholder.

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>
Signed-off-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.
---
 .../ABI/testing/sysfs-driver-intel-m10-bmc         |  15 ++
 drivers/mfd/Kconfig                                |  13 ++
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/intel-m10-bmc.c                        | 174 +++++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h                  |  57 +++++++
 5 files changed, 261 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..54cf5bc
--- /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.9
+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.9
+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 d13bb0a..42252c8 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2130,5 +2130,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 1c8d6be..149d7d1 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -265,3 +265,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..0d5e5d6
--- /dev/null
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Max10 Board Management Controller chip Driver
+ *
+ * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/module.h>
+#include <linux/mfd/core.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/mfd/intel-m10-bmc.h>
+
+enum m10bmc_type {
+	M10_N3000,
+};
+
+static struct mfd_cell m10bmc_pacn3000_subdevs[] = {};
+
+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 m10bmc_spi_setup(struct spi_device *spi)
+{
+	/* try 32 bits bpw first then fall back to 8 bits bpw */
+	spi->mode = SPI_MODE_1;
+	spi->bits_per_word = 32;
+	if (!spi_setup(spi))
+		return 0;
+
+	spi->bits_per_word = 8;
+	return spi_setup(spi);
+}
+
+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;
+
+	ret = m10bmc_spi_setup(spi);
+	if (ret)
+		return ret;
+
+	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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
  2020-07-16 10:42 ` [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC Xu Yilun
@ 2020-07-17  9:45   ` Lee Jones
  2020-07-21  3:26     ` Xu Yilun
  2020-07-17 18:16   ` Mark Brown
  1 sibling, 1 reply; 14+ messages in thread
From: Lee Jones @ 2020-07-17  9:45 UTC (permalink / raw)
  To: Xu Yilun
  Cc: broonie, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu

On Thu, 16 Jul 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.
> 
> This BMC chip is connected to FPGA by a SPI bus. To provide reliable
> register access from FPGA, an Avalon Memory-Mapped (Avmm) transaction
> protocol over the SPI bus is used between host and slave.
> 
> This driver implements the basic register access using the
> regmap-spi-avmm. The mfd cells array is empty now as a placeholder.
> 
> 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>
> Signed-off-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.
> ---
>  .../ABI/testing/sysfs-driver-intel-m10-bmc         |  15 ++
>  drivers/mfd/Kconfig                                |  13 ++
>  drivers/mfd/Makefile                               |   2 +
>  drivers/mfd/intel-m10-bmc.c                        | 174 +++++++++++++++++++++
>  include/linux/mfd/intel-m10-bmc.h                  |  57 +++++++
>  5 files changed, 261 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..54cf5bc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> @@ -0,0 +1,15 @@
> +What:		/sys/bus/spi/devices/.../bmc_version

Why is this in your MFD patch?

Again, this looks like it's documenting a SPI device?

> +Date:		June 2020
> +KernelVersion:	5.9
> +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.9
> +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 d13bb0a..42252c8 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2130,5 +2130,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"

"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 1c8d6be..149d7d1 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -265,3 +265,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..0d5e5d6
> --- /dev/null
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Max10 Board Management Controller chip Driver

Drop "Driver"

> + * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> + *
> + */
> +#include <linux/bitfield.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/init.h>
> +#include <linux/spi/spi.h>
> +#include <linux/mfd/intel-m10-bmc.h>

Alphabetical.

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

Will there be others?

> +static struct mfd_cell m10bmc_pacn3000_subdevs[] = {};
> +
> +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) {

So the only supported version number is all 1's?

> +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int m10bmc_spi_setup(struct spi_device *spi)
> +{
> +	/* try 32 bits bpw first then fall back to 8 bits bpw */
> +	spi->mode = SPI_MODE_1;
> +	spi->bits_per_word = 32;
> +	if (!spi_setup(spi))
> +		return 0;
> +
> +	spi->bits_per_word = 8;
> +	return spi_setup(spi);

Looks odd.  Comments needed.

> +}
> +
> +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;
> +
> +	ret = m10bmc_spi_setup(spi);
> +	if (ret)
> +		return ret;
> +
> +	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);

This is just an empty cell though, right?

I'm not even sure why this doesn't just crash.

> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> +				   NULL, 0, NULL);

What sub-devices does this actually register?

> +	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;
> +}

Why are you abstracting Regmap?

> +#define m10bmc_sys_read(m10bmc, offset, val) \
> +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> +
> +#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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/3] regmap: spi-avmm: start with the last SOP on phy rx buffer parsing
  2020-07-16 10:42 ` [PATCH v2 2/3] regmap: spi-avmm: start with the last SOP on phy rx buffer parsing Xu Yilun
@ 2020-07-17 12:11   ` Mark Brown
  0 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2020-07-17 12:11 UTC (permalink / raw)
  To: Xu Yilun
  Cc: lee.jones, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu

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

On Thu, Jul 16, 2020 at 06:42:53PM +0800, Xu Yilun wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> This patch works around a bug in the SPI Slave to Avalon Master bridge.
> The SPI slave will send an unexpected extra SOP in the following case.

This is fixing a bug introduced in patch 1 - just send a version of
patch 1 that works, don't incrementally fix it.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support
  2020-07-16 10:42 ` [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support Xu Yilun
@ 2020-07-17 18:15   ` Mark Brown
  2020-07-20 17:11     ` Xu Yilun
  0 siblings, 1 reply; 14+ messages in thread
From: Mark Brown @ 2020-07-17 18:15 UTC (permalink / raw)
  To: Xu Yilun
  Cc: lee.jones, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu

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

On Thu, Jul 16, 2020 at 06:42:52PM +0800, Xu Yilun wrote:

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

At a very high level this looks far too complicated.  Some of that is
due to the protocol it's implementing which looks awfully like you've
got a microcontroller in there trying to proxy a bus of some kind but
there looks to be a lot of abstraction layers simply within the software
here which make things hard to follow.  At the very least there doesn't
seem to be any interaction with this other than proxying stuff.  As
things stand this would be the largest regmap bus implementation by an
order of magnitude.

It is not clear to me that this is really a generic thing that will have
multiple users and that it shouldn't be implemented as reg_read() and
reg_write() operations in the driver that uses it.

> +config REGMAP_SPI_AVMM
> +	tristate
> +	depends on SPI

Note that for selected symbols dependencies don't take effect.

> @@ -0,0 +1,932 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Intel SPI Slave to AVMM Bus Bridge

Please make the entire comment a C++ one so things look omre
intentional.

> + * Reference Documents could be found at:
> + * https://www.intel.com/content/www/us/en/programmable/documentation/
> + * sfo1400787952932.html

Please don't corrupt links like this, that makes them harder to use.

> + * The main function of the physical layer is the use of PHY_IDLE (4a). Host
> + * issues SCLK to query data from slave, but if slave is not ready to submit
> + * data yet, it will repeat PHY_IDLE until data is prepared.
> + * Because of this special char, it also needs an ESCAPE char (4d), to help
> + * represent data "4a". The escape rule is "4d first, following 4a ^ 20".
> + * So "4d, 6a" for data "4a", and "4d, 6d" for data "4d".

> + * The packet layer defines the boundary of a whole packet. It defines the
> + * Start Of Packet (SOP, 7a) char and End Of Packet (EOP, 7b) char. Please
> + * note that the non-special byte after EOP is the last byte of the packet.
> + * The packet layer also defines a Channel char (7c) + Channel number for
> + * multiple channel transfer. This not currently supported by this driver. So
> + * host will always send "7c, 00" when needed, and will drop the packet if
> + * "7c, non-zero" is received.
> + * Finally, a packet layer ESCAPE char (7d) is also needed to represent a
> + * data value that is the same as the special chars. The escape rule is the
> + * same. The escape rule should be used if the last byte requires it. So if a
> + * packet ends up with data 7a, the last bytes should be "7b, 7d, 5a".

This comment is not particularly intelligable.  I assume all these
number/letter combinations are references to something - it'd be good to
say what.

> +/* slave's register addr is 32 bits */
> +#define REG_SIZE		4UL
> +/* slave's register value is 32 bits */
> +#define VAL_SIZE		4UL

These and many of the other constants are liable to conflict with
something, namespacing would be good.

> +struct trans_header {
> +	u8 trans_code;
> +	u8 rsvd;
> +	__be16 size;
> +	__be32 addr;
> +};
> +
> +struct trans_response {
> +	u8 r_trans_code;
> +	u8 rsvd;
> +	__be16 size;
> +};

Do these need to be packed?

> +/* No additional chars are in transaction layer RX, just read out data */
> +#define TRANS_RX_MAX		(VAL_SIZE * MAX_RX_CNT)
> +/*

Blank line before the comment.

> +#define TRANS_BUF_SIZE		((TRANS_TX_MAX > TRANS_RX_MAX) ?	\
> +				 TRANS_TX_MAX : TRANS_RX_MAX)

This could be more simply written using max() couldn't it (which IIRC
can also be evaluated at compile time)?

> +/* Format a transaction layer byte stream for tx_buf */
> +static void trans_tx_prepare(bool is_read, u32 reg, u16 count, u32 *wr_val,
> +			     char *tx_buf, unsigned int *tx_len)
> +{
> +	struct trans_header *header;
> +	u8 trans_code;
> +	__le32 *data;
> +	int i;
> +
> +	trans_code = is_read ?
> +			(count == 1 ? TRANS_CODE_READ : TRANS_CODE_SEQ_READ) :
> +			(count == 1 ? TRANS_CODE_WRITE : TRANS_CODE_SEQ_WRITE);

Please write this using normal conditional statements for legibility.

> +	header = (struct trans_header *)tx_buf;

No need to cast away from char *.

> +/*
> + * The input is a transaction layer byte stream in rx_buf, the output is read
> + * out data.
> + */
> +static int trans_rx_parse(bool is_read, char *rx_buf, unsigned int rx_len,
> +			  u16 expected_count, u32 *rd_val)
> +{
> +	unsigned int expected_len = expected_count * VAL_SIZE;
> +
> +	if (is_read) {
> +		if (expected_len != rx_len)
> +			return -EINVAL;
> +
> +		return rd_trans_rx_parse(rx_buf, rx_len, rd_val);
> +	}
> +
> +	return wr_trans_rx_parse(rx_buf, rx_len, expected_len);
> +}

Please write the is_read check as an if/else, it'd be clearer.

> +/* Packet Layer & Physical Layer */
> +/* The input is a trans stream in trans_buf, format a phy stream in phy_buf. */
> +static void pkt_phy_tx_prepare(char *trans_buf, unsigned int trans_len,
> +			       char *phy_buf, unsigned int *phy_len)
> +{
> +	unsigned int i;
> +	char *b, *p;
> +
> +	b = trans_buf;
> +	p = phy_buf;

I'm not seeing any bounds checking on the size of an operation in this
function and I think I'd expect some, similarly for a lot of the other
I/O operations.

> +	poll_timeout = jiffies + SPI_AVMM_XFER_TIMEOUT;
> +	for (;;) {

Please rewrite this loop so it's clear that it terminates, usually
that'd mean a do { } while (!timeout) type construction.  The extra
effort involved in doing one more check after timeout could just as well
be a slightly longer timeout and it'd be clearer.

> +		/*
> +		 * We timeout when rx is 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 we return a valid rx word,
> +		 * otherwise real a HW issue happens.
> +		 */
> +		if (time_after(jiffies, poll_timeout))
> +			last_try = true;

If the system is that heavily loaded I fear there's bigger problems, you
could also express the timeout in terms of number of times you poll with
a delay (at the minute this will busy wait which doesn't seem ideal
either).

> +static void br_tx_prepare(struct spi_avmm_bridge *br, bool is_read, u32 reg,
> +			  u16 count, u32 *wr_val)
> +{
> +	unsigned int tx_len;
> +
> +	trans_tx_prepare(is_read, reg, count, wr_val,
> +			 br->trans_buf, &tx_len);
> +	pkt_phy_tx_prepare(br->trans_buf, tx_len,
> +			   br->phy_buf, &tx_len);
> +	phy_tx_pad(br->word_len, br->phy_buf, tx_len, &tx_len);
> +
> +	br->phy_len = tx_len;
> +}

This is the sort of thing I'm looking at when I say this code has too
many abstraction layers, this function and all of the functions it calls
have exactly one caller spread out all over the code and I can't help
but think that some of them could be usefully inlined.  As things stand
it's really hard to tell what any individual bit of code is supposed to
be doing or if it's doing it correctly.

> +	ret = ret ? : (eop ? 0 : -EINVAL);
> +	if (ret) {
> +		dev_err(&br->spi->dev, "%s br txrx err %d\n", __func__, ret);
> +		print_hex_dump(KERN_DEBUG, "phy rx:", DUMP_PREFIX_OFFSET,
> +			       16, 1, br->phy_buf, rx_len, true);
> +	}

Please write normal conditional statements for legibility.

> +
> +#define do_reg_read(_ctx, _reg, _value, _count) \
> +	do_reg_access(_ctx, true, _reg, _value, _count)
> +#define do_reg_write(_ctx, _reg, _value, _count) \
> +	do_reg_access(_ctx, false, _reg, _value, _count)

Please write these as proper functions so that the compiler can check
things more thoroughly.

> +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 != REG_SIZE)
> +		return -EINVAL;
> +
> +	return do_reg_write(context, *(u32 *)reg_buf, (u32 *)val_buf,
> +			    (u16)(val_len / VAL_SIZE));
> +}

The cast on the length looks worrying...  

> +	/* Only support BPW == 8 or 32 now */
> +	if (spi->bits_per_word != 8 && spi->bits_per_word != 32)
> +		return ERR_PTR(-EINVAL);

Most controllers support configurable bits per word (and modes) - you
shouldn't be rejecting things based on whatever the default happens to
be.

> +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 = VAL_SIZE * MAX_RX_CNT,
> +	.max_raw_write = VAL_SIZE * MAX_TX_CNT,
> +
> +	.reg_write = regmap_spi_avmm_reg_write,
> +	.reg_read = regmap_spi_avmm_reg_read,
> +
> +	.free_context = spi_avmm_bridge_ctx_free,
> +};

This seems confused, you shouldn't have both unformatted I/O operations
and register I/O operations.  Either the hardware implements a byte
stream and the core will handle formatting register operations into that
byte stream or the hardware understands registers and values in which
case there shouldn't be unformatted operations.  A combination of the
two is not going to work well.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
  2020-07-16 10:42 ` [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC Xu Yilun
  2020-07-17  9:45   ` Lee Jones
@ 2020-07-17 18:16   ` Mark Brown
  2020-07-21  3:47     ` Xu Yilun
  1 sibling, 1 reply; 14+ messages in thread
From: Mark Brown @ 2020-07-17 18:16 UTC (permalink / raw)
  To: Xu Yilun
  Cc: lee.jones, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu

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

On Thu, Jul 16, 2020 at 06:42:54PM +0800, Xu Yilun wrote:

> +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);

This device has no ACPI information - how will it be instantiated?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support
  2020-07-17 18:15   ` Mark Brown
@ 2020-07-20 17:11     ` Xu Yilun
  2020-07-22 11:32       ` Mark Brown
  0 siblings, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2020-07-20 17:11 UTC (permalink / raw)
  To: Mark Brown
  Cc: lee.jones, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu, yilun.xu

On Fri, Jul 17, 2020 at 07:15:08PM +0100, Mark Brown wrote:
> On Thu, Jul 16, 2020 at 06:42:52PM +0800, Xu Yilun wrote:
> 
> > This patch add support for regmap API that is intended to be used by
> > the drivers of some SPI slave chips which integrate the "SPI slave to
> > Avalon Master Bridge" (spi-avmm) IP.
> 
> At a very high level this looks far too complicated.  Some of that is
> due to the protocol it's implementing which looks awfully like you've
> got a microcontroller in there trying to proxy a bus of some kind but

Maybe I should introduce about the role of the spi-avmm in the system.
Usually it is used in a SPI slave chip which integrates an Avalon Memory
Mapped bus (Avmm bus) and several sub devices. Like the following
diagram:

 +-------------------------------------+
 |              Max10 BMC              |
 |                                     |
 | +---------+    |    |    +--------+ |
 | |subdev A |    |    |    |subdev B| |
 | |register |----|Avmm|----|register| |
 | |space    |    |bus |    |space   | |
 | |(Avmm    |    |    |    |(Avmm   | |    +---------+
 | | Slave)  |    |    |    | Slave) | |    |         |
 | +---------+    |    |    +--------+ |    | Host    |
 |                |    |               |    | system  |
 | +---------+    |    |      +--------|    |         |
 | |Embedded |----|    |------|spi-avmm|    |         |
 | |processor|    |    |      |bridge  |    |------+  |
 | | (Avmm   |    |    |      |(Avmm   |----|spi   |  |
 | |  Master)|                | Master)|    |master|  |
 | +---------+                +--------|    |------+  |
 |                                     |    |         |
 +-------------------------------------+    +---------+

In order for the host to access the sub device's register space, the
spi-avmm IP is needed to receive the formatted byte stream from host,
convert them to internal Avmm RW, and send back results to host. 

> there looks to be a lot of abstraction layers simply within the software
> here which make things hard to follow.  At the very least there doesn't

We need to follow the 3 layers protocol for register accessing. On SPI
slave side, spi-avmm HW finishes the whole protocol encoding/decoding
work. But on host side, no dedicated IP block is designed. So host need
to handle all the protocol work by SW. This introduces the complexity of
the driver.
We don't introduce any extra software layers, just follows the
definition of 3 layers in HW spec. But I know reviewing the detail of the
protocol is tedious. Maybe I should put more comments about what part of
SPEC should be referenced for some piece of code. Hope this makes the
review work easier.

> seem to be any interaction with this other than proxying stuff.  As
> things stand this would be the largest regmap bus implementation by an
> order of magnitude.
> 
> It is not clear to me that this is really a generic thing that will have
> multiple users and that it shouldn't be implemented as reg_read() and
> reg_write() operations in the driver that uses it.

This spi-avmm IP is not dedicated to the intel Max10 bmc. It is a soft IP
provided in Altera (now Intel) IP library. It could be implemented on
FPGA/CPLDs according to designers need. Actually Max10 is the CPLD chip which
is programed to have this IP on it.
I think it may be good to implement the regmap to support any SPI chips with
this IP.

Another thing is, the Max10 could integrate some sub devices (soft IPs) which
are already supported by platform drivers. Using the regmap could make
the sub devices supported by these drivers in a generic way, just like the way
we do in recent spi-altera patchset.

> 
> > +config REGMAP_SPI_AVMM
> > +	tristate
> > +	depends on SPI
> 
> Note that for selected symbols dependencies don't take effect.

I see. So should I change something here? I see the same case for other
regmap options.

> 
> > @@ -0,0 +1,932 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Intel SPI Slave to AVMM Bus Bridge
> 
> Please make the entire comment a C++ one so things look omre
> intentional.

Yes I'll change it.

> 
> > + * Reference Documents could be found at:
> > + * https://www.intel.com/content/www/us/en/programmable/documentation/
> > + * sfo1400787952932.html
> 
> Please don't corrupt links like this, that makes them harder to use.

I see. No wrapper for the URL.

> 
> > + * The main function of the physical layer is the use of PHY_IDLE (4a). Host
> > + * issues SCLK to query data from slave, but if slave is not ready to submit
> > + * data yet, it will repeat PHY_IDLE until data is prepared.
> > + * Because of this special char, it also needs an ESCAPE char (4d), to help
> > + * represent data "4a". The escape rule is "4d first, following 4a ^ 20".
> > + * So "4d, 6a" for data "4a", and "4d, 6d" for data "4d".
> 
> > + * The packet layer defines the boundary of a whole packet. It defines the
> > + * Start Of Packet (SOP, 7a) char and End Of Packet (EOP, 7b) char. Please
> > + * note that the non-special byte after EOP is the last byte of the packet.
> > + * The packet layer also defines a Channel char (7c) + Channel number for
> > + * multiple channel transfer. This not currently supported by this driver. So
> > + * host will always send "7c, 00" when needed, and will drop the packet if
> > + * "7c, non-zero" is received.
> > + * Finally, a packet layer ESCAPE char (7d) is also needed to represent a
> > + * data value that is the same as the special chars. The escape rule is the
> > + * same. The escape rule should be used if the last byte requires it. So if a
> > + * packet ends up with data 7a, the last bytes should be "7b, 7d, 5a".
> 
> This comment is not particularly intelligable.  I assume all these
> number/letter combinations are references to something - it'd be good to
> say what.

Yes I'll add the table of definitions of these chars referenced in HW
spec.

> 
> > +/* slave's register addr is 32 bits */
> > +#define REG_SIZE		4UL
> > +/* slave's register value is 32 bits */
> > +#define VAL_SIZE		4UL
> 
> These and many of the other constants are liable to conflict with
> something, namespacing would be good.

Yes I'll add the prefix for naming.

> 
> > +struct trans_header {
> > +	u8 trans_code;
> > +	u8 rsvd;
> > +	__be16 size;
> > +	__be32 addr;
> > +};
> > +
> > +struct trans_response {
> > +	u8 r_trans_code;
> > +	u8 rsvd;
> > +	__be16 size;
> > +};
> 
> Do these need to be packed?

Yes.

> 
> > +/* No additional chars are in transaction layer RX, just read out data */
> > +#define TRANS_RX_MAX		(VAL_SIZE * MAX_RX_CNT)
> > +/*
> 
> Blank line before the comment.

Yes.

> 
> > +#define TRANS_BUF_SIZE		((TRANS_TX_MAX > TRANS_RX_MAX) ?	\
> > +				 TRANS_TX_MAX : TRANS_RX_MAX)
> 
> This could be more simply written using max() couldn't it (which IIRC
> can also be evaluated at compile time)?

I tried to use max(), but seems it can only be used within a function.
We intend to use the macro in struct definition:

  struct spi_avmm_bridge {
          ...
          char trans_buf[TRANS_BUF_SIZE];
          char phy_buf[PHY_BUF_SIZE];
  };

It will cause the compile error with max().

> 
> > +/* Format a transaction layer byte stream for tx_buf */
> > +static void trans_tx_prepare(bool is_read, u32 reg, u16 count, u32 *wr_val,
> > +			     char *tx_buf, unsigned int *tx_len)
> > +{
> > +	struct trans_header *header;
> > +	u8 trans_code;
> > +	__le32 *data;
> > +	int i;
> > +
> > +	trans_code = is_read ?
> > +			(count == 1 ? TRANS_CODE_READ : TRANS_CODE_SEQ_READ) :
> > +			(count == 1 ? TRANS_CODE_WRITE : TRANS_CODE_SEQ_WRITE);
> 
> Please write this using normal conditional statements for legibility.

Yes.

> 
> > +	header = (struct trans_header *)tx_buf;
> 
> No need to cast away from char *.

Yes.

> 
> > +/*
> > + * The input is a transaction layer byte stream in rx_buf, the output is read
> > + * out data.
> > + */
> > +static int trans_rx_parse(bool is_read, char *rx_buf, unsigned int rx_len,
> > +			  u16 expected_count, u32 *rd_val)
> > +{
> > +	unsigned int expected_len = expected_count * VAL_SIZE;
> > +
> > +	if (is_read) {
> > +		if (expected_len != rx_len)
> > +			return -EINVAL;
> > +
> > +		return rd_trans_rx_parse(rx_buf, rx_len, rd_val);
> > +	}
> > +
> > +	return wr_trans_rx_parse(rx_buf, rx_len, expected_len);
> > +}
> 
> Please write the is_read check as an if/else, it'd be clearer.

Yes.

> 
> > +/* Packet Layer & Physical Layer */
> > +/* The input is a trans stream in trans_buf, format a phy stream in phy_buf. */
> > +static void pkt_phy_tx_prepare(char *trans_buf, unsigned int trans_len,
> > +			       char *phy_buf, unsigned int *phy_len)
> > +{
> > +	unsigned int i;
> > +	char *b, *p;
> > +
> > +	b = trans_buf;
> > +	p = phy_buf;
> 
> I'm not seeing any bounds checking on the size of an operation in this
> function and I think I'd expect some, similarly for a lot of the other
> I/O operations.

I have caculated and allocated 2 buffers that are large enough to
contain any possible data stream for regmap_bus.max_raw_read/write. See
the definition of TRANS_BUF_SIZE & PHY_BUF_SIZE.

So maybe we don't have to check the length?

> 
> > +	poll_timeout = jiffies + SPI_AVMM_XFER_TIMEOUT;
> > +	for (;;) {
> 
> Please rewrite this loop so it's clear that it terminates, usually
> that'd mean a do { } while (!timeout) type construction.  The extra
> effort involved in doing one more check after timeout could just as well
> be a slightly longer timeout and it'd be clearer.
> 
> > +		/*
> > +		 * We timeout when rx is 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 we return a valid rx word,
> > +		 * otherwise real a HW issue happens.
> > +		 */
> > +		if (time_after(jiffies, poll_timeout))
> > +			last_try = true;
> 
> If the system is that heavily loaded I fear there's bigger problems, you
> could also express the timeout in terms of number of times you poll with
> a delay (at the minute this will busy wait which doesn't seem ideal
> either).

Actually this is to fix issue found on systems with Realtime kernel
patchset. In the system you may be scheduled out for enough long time as
long as the higher priority tasks keep running. In that case the slightly
longer timeout didn't work. The number of times delay is not a good
solution either. So I still want to keep the last_try machanism. But
I'll try my best to rewrite this to make it clear when it terminates.

> 
> > +static void br_tx_prepare(struct spi_avmm_bridge *br, bool is_read, u32 reg,
> > +			  u16 count, u32 *wr_val)
> > +{
> > +	unsigned int tx_len;
> > +
> > +	trans_tx_prepare(is_read, reg, count, wr_val,
> > +			 br->trans_buf, &tx_len);
> > +	pkt_phy_tx_prepare(br->trans_buf, tx_len,
> > +			   br->phy_buf, &tx_len);
> > +	phy_tx_pad(br->word_len, br->phy_buf, tx_len, &tx_len);
> > +
> > +	br->phy_len = tx_len;
> > +}
> 
> This is the sort of thing I'm looking at when I say this code has too
> many abstraction layers, this function and all of the functions it calls
> have exactly one caller spread out all over the code and I can't help
> but think that some of them could be usefully inlined.  As things stand
> it's really hard to tell what any individual bit of code is supposed to
> be doing or if it's doing it correctly.

OK. I'll try to refactor the code and remove unnecessary function calls,
only keep the functions that represents the steps clearly defined in HW
spec, and add some comments about what part of spec they are referenced.

> 
> > +	ret = ret ? : (eop ? 0 : -EINVAL);
> > +	if (ret) {
> > +		dev_err(&br->spi->dev, "%s br txrx err %d\n", __func__, ret);
> > +		print_hex_dump(KERN_DEBUG, "phy rx:", DUMP_PREFIX_OFFSET,
> > +			       16, 1, br->phy_buf, rx_len, true);
> > +	}
> 
> Please write normal conditional statements for legibility.

Yes.

> 
> > +
> > +#define do_reg_read(_ctx, _reg, _value, _count) \
> > +	do_reg_access(_ctx, true, _reg, _value, _count)
> > +#define do_reg_write(_ctx, _reg, _value, _count) \
> > +	do_reg_access(_ctx, false, _reg, _value, _count)
> 
> Please write these as proper functions so that the compiler can check
> things more thoroughly.

Yes

> 
> > +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 != REG_SIZE)
> > +		return -EINVAL;
> > +
> > +	return do_reg_write(context, *(u32 *)reg_buf, (u32 *)val_buf,
> > +			    (u16)(val_len / VAL_SIZE));
> > +}
> 
> The cast on the length looks worrying...  

OK, I'll reconsider the do_reg_access definition. Cast to u16 here seems
a little confusing.

> 
> > +	/* Only support BPW == 8 or 32 now */
> > +	if (spi->bits_per_word != 8 && spi->bits_per_word != 32)
> > +		return ERR_PTR(-EINVAL);
> 
> Most controllers support configurable bits per word (and modes) - you
> shouldn't be rejecting things based on whatever the default happens to
> be.

I'm not sure if it is good that the spi_avmm_regmap_init changes the
configuration of spi devices silently.
In my current implementation, the spi device driver should be aware and
choose the right spi setup before trying to init the regmap. Would it be
too hard for other drivers to use it?

> 
> > +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 = VAL_SIZE * MAX_RX_CNT,
> > +	.max_raw_write = VAL_SIZE * MAX_TX_CNT,
> > +
> > +	.reg_write = regmap_spi_avmm_reg_write,
> > +	.reg_read = regmap_spi_avmm_reg_read,
> > +
> > +	.free_context = spi_avmm_bridge_ctx_free,
> > +};
> 
> This seems confused, you shouldn't have both unformatted I/O operations
> and register I/O operations.  Either the hardware implements a byte
> stream and the core will handle formatting register operations into that
> byte stream or the hardware understands registers and values in which
> case there shouldn't be unformatted operations.  A combination of the
> two is not going to work well.

I see. We implemented the byte stream callbacks because we need to
support incremental register read/write mode. I'll remove the
reg_write/read callbacks.

Thanks,
Yilun

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
  2020-07-17  9:45   ` Lee Jones
@ 2020-07-21  3:26     ` Xu Yilun
  0 siblings, 0 replies; 14+ messages in thread
From: Xu Yilun @ 2020-07-21  3:26 UTC (permalink / raw)
  To: Lee Jones
  Cc: broonie, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu, yilun.xu

On Fri, Jul 17, 2020 at 10:45:42AM +0100, Lee Jones wrote:
> On Thu, 16 Jul 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.
> > 
> > This BMC chip is connected to FPGA by a SPI bus. To provide reliable
> > register access from FPGA, an Avalon Memory-Mapped (Avmm) transaction
> > protocol over the SPI bus is used between host and slave.
> > 
> > This driver implements the basic register access using the
> > regmap-spi-avmm. The mfd cells array is empty now as a placeholder.
> > 
> > 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>
> > Signed-off-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.
> > ---
> >  .../ABI/testing/sysfs-driver-intel-m10-bmc         |  15 ++
> >  drivers/mfd/Kconfig                                |  13 ++
> >  drivers/mfd/Makefile                               |   2 +
> >  drivers/mfd/intel-m10-bmc.c                        | 174 +++++++++++++++++++++
> >  include/linux/mfd/intel-m10-bmc.h                  |  57 +++++++
> >  5 files changed, 261 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..54cf5bc
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> > @@ -0,0 +1,15 @@
> > +What:		/sys/bus/spi/devices/.../bmc_version
> 
> Why is this in your MFD patch?
> 
> Again, this looks like it's documenting a SPI device?

The intel-m10-bmc device is a spi device, which contains several sub
devices in it, and I use the mfd APIs for subdev creating.
I see some example drivers in mfd folder which also match a spi/i2c parent
device and creates the sub devices. So I think I may follow the examples
and put the intel-m10-bmc driver in mfd folder.

A little difference is that the driver added 2 sysfs nodes for the
parent device itself. I'm not sure how I should document the sysfs path
correctly. The device is a mfd parent, but I didn't find a sysfs path that
could explicitly indicate its role of mfd parent.

Do you have any suggestion on that?

> 
> > +Date:		June 2020
> > +KernelVersion:	5.9
> > +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.9
> > +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 d13bb0a..42252c8 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2130,5 +2130,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"
> 
> "Board Management Controller"

Yes, I'll change it.

> 
> > +	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 1c8d6be..149d7d1 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -265,3 +265,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..0d5e5d6
> > --- /dev/null
> > +++ b/drivers/mfd/intel-m10-bmc.c
> > @@ -0,0 +1,174 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel Max10 Board Management Controller chip Driver
> 
> Drop "Driver"

Yes

> 
> > + * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> > + *
> > + */
> > +#include <linux/bitfield.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/module.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/init.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/mfd/intel-m10-bmc.h>
> 
> Alphabetical.

Yes

> 
> > +enum m10bmc_type {
> > +	M10_N3000,
> > +};
> 
> Will there be others?

Yes. 
The M10_N3000 is for "intel pac n3000" FPGA card. There is another M10_D5005
for "intel pac d5005" FPGA card.
We may add d5005 support later on.

> 
> > +static struct mfd_cell m10bmc_pacn3000_subdevs[] = {};
> > +
> > +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) {
> 
> So the only supported version number is all 1's?

No, this is to exclude the legacy version of m10bmc.
The legacy version of m10bmc has different system register base. So if
there is valid version value in the legacy version register, it is the
legacy m10bmc and we filter it out.

> 
> > +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int m10bmc_spi_setup(struct spi_device *spi)
> > +{
> > +	/* try 32 bits bpw first then fall back to 8 bits bpw */
> > +	spi->mode = SPI_MODE_1;
> > +	spi->bits_per_word = 32;
> > +	if (!spi_setup(spi))
> > +		return 0;
> > +
> > +	spi->bits_per_word = 8;
> > +	return spi_setup(spi);
> 
> Looks odd.  Comments needed.

OK, I'll add more comments here.

> 
> > +}
> > +
> > +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;
> > +
> > +	ret = m10bmc_spi_setup(spi);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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);
> 
> This is just an empty cell though, right?
> 
> I'm not even sure why this doesn't just crash.

Yes, I didn't add the sub device info now. I tested it will not crash,
but I think I don't have to the subdev info one by one along with the
sub device drivers, right? An empty cell is confusing.

> 
> > +		break;
> > +	default:
> > +		return -ENODEV;
> > +	}
> > +
> > +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> > +				   NULL, 0, NULL);
> 
> What sub-devices does this actually register?

We have 3 sub devices now.
"n3000bmc-hwmon" is a set of sensors
"n3000bmc-pkvl" is a set of ethernet phys
"m10bmc-secure" is the secure reprograming engine for FPGA

I'll add the sub devices info in my next version.

> 
> > +	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;
> > +}
> 
> Why are you abstracting Regmap?

I may paste the comments from previous patch:

  This spi-avmm IP is not dedicated to the intel Max10 bmc. It is a soft
  IP provided in Altera (now Intel) IP library. It could be implemented on
  FPGA/CPLDs according to designers need. Actually Max10 is the CPLD chip
  which is programed to have this IP on it.
  I think it may be good to implement the regmap to support any SPI chips
  with this IP.

  Another thing is, the Max10 could integrate some sub devices (soft IPs)
  which are already supported by platform drivers. Using the regmap could
  make the sub devices supported by these drivers in a generic way.

We wapper the regmap operations in an function just to help print some
error info if regmap r/w fails.

Thanks,
Yilun 

> 
> > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > +
> > +#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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
  2020-07-17 18:16   ` Mark Brown
@ 2020-07-21  3:47     ` Xu Yilun
  2020-07-21  3:50       ` Xu Yilun
  0 siblings, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2020-07-21  3:47 UTC (permalink / raw)
  To: Mark Brown
  Cc: lee.jones, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu, yilun.xu

On Fri, Jul 17, 2020 at 07:16:09PM +0100, Mark Brown wrote:
> On Thu, Jul 16, 2020 at 06:42:54PM +0800, Xu Yilun wrote:
> 
> > +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);
> 
> This device has no ACPI information - how will it be instantiated?

In our case, The m10-bmc is connected to the intel FPGA (PAC N3000),
which uses the Device Feature List (DFL) mechanism to enumerate features
(devices) on FPGA. Each feature in DFL has a feature_id. And for this
m10-n3000 feature (feature_id = 0xd), it contains a spi-altera & a
m10-n3000 chip. So the DFL subsystem would help enumerate the info.

Recently I added the platform data for slave information in spi-altera,
to support this use case.

Thanks,
Yilun

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
  2020-07-21  3:47     ` Xu Yilun
@ 2020-07-21  3:50       ` Xu Yilun
  0 siblings, 0 replies; 14+ messages in thread
From: Xu Yilun @ 2020-07-21  3:50 UTC (permalink / raw)
  To: Mark Brown
  Cc: lee.jones, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu, yilun.xu

On Tue, Jul 21, 2020 at 11:47:13AM +0800, Xu Yilun wrote:
> On Fri, Jul 17, 2020 at 07:16:09PM +0100, Mark Brown wrote:
> > On Thu, Jul 16, 2020 at 06:42:54PM +0800, Xu Yilun wrote:
> > 
> > > +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);
> > 
> > This device has no ACPI information - how will it be instantiated?
> 
> In our case, The m10-bmc is connected to the intel FPGA (PAC N3000),
> which uses the Device Feature List (DFL) mechanism to enumerate features
> (devices) on FPGA. Each feature in DFL has a feature_id. And for this
> m10-n3000 feature (feature_id = 0xd), it contains a spi-altera & a
> m10-n3000 chip. So the DFL subsystem would help enumerate the info.

And some DFL related infomation:
  Documentation/fpga/dfl.rst
  drivers/fpga/dfl*

Thanks,
Yilun

> 
> Recently I added the platform data for slave information in spi-altera,
> to support this use case.
> 
> Thanks,
> Yilun

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support
  2020-07-20 17:11     ` Xu Yilun
@ 2020-07-22 11:32       ` Mark Brown
  2020-07-23  1:30         ` Xu Yilun
  0 siblings, 1 reply; 14+ messages in thread
From: Mark Brown @ 2020-07-22 11:32 UTC (permalink / raw)
  To: Xu Yilun
  Cc: lee.jones, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu

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

On Tue, Jul 21, 2020 at 01:11:31AM +0800, Xu Yilun wrote:
> On Fri, Jul 17, 2020 at 07:15:08PM +0100, Mark Brown wrote:

> > there looks to be a lot of abstraction layers simply within the software
> > here which make things hard to follow.  At the very least there doesn't

> We need to follow the 3 layers protocol for register accessing. On SPI
> slave side, spi-avmm HW finishes the whole protocol encoding/decoding
> work. But on host side, no dedicated IP block is designed. So host need
> to handle all the protocol work by SW. This introduces the complexity of
> the driver.

> We don't introduce any extra software layers, just follows the
> definition of 3 layers in HW spec. But I know reviewing the detail of the
> protocol is tedious. Maybe I should put more comments about what part of
> SPEC should be referenced for some piece of code. Hope this makes the
> review work easier.

You don't need to reflect all the layers that the system has in the
software, if some of the layers are always used together with no
possibility of replacing them then it can make sense to collapse them in
software.  That did seem to be the case here.

> > > +config REGMAP_SPI_AVMM
> > > +	tristate
> > > +	depends on SPI

> > Note that for selected symbols dependencies don't take effect.

> I see. So should I change something here? I see the same case for other
> regmap options.

It's fine, just be aware that you still need to have appropriate
dependencies and selects in the user.

> > > +/* The input is a trans stream in trans_buf, format a phy stream in phy_buf. */
> > > +static void pkt_phy_tx_prepare(char *trans_buf, unsigned int trans_len,
> > > +			       char *phy_buf, unsigned int *phy_len)
> > > +{
> > > +	unsigned int i;
> > > +	char *b, *p;
> > > +
> > > +	b = trans_buf;
> > > +	p = phy_buf;

> > I'm not seeing any bounds checking on the size of an operation in this
> > function and I think I'd expect some, similarly for a lot of the other
> > I/O operations.

> I have caculated and allocated 2 buffers that are large enough to
> contain any possible data stream for regmap_bus.max_raw_read/write. See
> the definition of TRANS_BUF_SIZE & PHY_BUF_SIZE.

> So maybe we don't have to check the length?

This isn't at all clear looking at the code, perhaps it would be clearer
with fewer layers of abstraction but at the minute it's a lot of effort
to confirm if it's safe.

> > > +	if (spi->bits_per_word != 8 && spi->bits_per_word != 32)
> > > +		return ERR_PTR(-EINVAL);

> > Most controllers support configurable bits per word (and modes) - you
> > shouldn't be rejecting things based on whatever the default happens to
> > be.

> I'm not sure if it is good that the spi_avmm_regmap_init changes the
> configuration of spi devices silently.

That's the expected usage.

> In my current implementation, the spi device driver should be aware and
> choose the right spi setup before trying to init the regmap. Would it be
> too hard for other drivers to use it?

It'd be duplicated effort in all the users, if the only way to use the
bus is with this configuration then simply saying that they're using the
bus should be enough.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support
  2020-07-22 11:32       ` Mark Brown
@ 2020-07-23  1:30         ` Xu Yilun
  0 siblings, 0 replies; 14+ messages in thread
From: Xu Yilun @ 2020-07-23  1:30 UTC (permalink / raw)
  To: Mark Brown
  Cc: lee.jones, linux-kernel, trix, matthew.gerlach, russell.h.weight,
	lgoncalv, hao.wu

On Wed, Jul 22, 2020 at 12:32:02PM +0100, Mark Brown wrote:
> On Tue, Jul 21, 2020 at 01:11:31AM +0800, Xu Yilun wrote:
> > On Fri, Jul 17, 2020 at 07:15:08PM +0100, Mark Brown wrote:
> 
> > > there looks to be a lot of abstraction layers simply within the software
> > > here which make things hard to follow.  At the very least there doesn't
> 
> > We need to follow the 3 layers protocol for register accessing. On SPI
> > slave side, spi-avmm HW finishes the whole protocol encoding/decoding
> > work. But on host side, no dedicated IP block is designed. So host need
> > to handle all the protocol work by SW. This introduces the complexity of
> > the driver.
> 
> > We don't introduce any extra software layers, just follows the
> > definition of 3 layers in HW spec. But I know reviewing the detail of the
> > protocol is tedious. Maybe I should put more comments about what part of
> > SPEC should be referenced for some piece of code. Hope this makes the
> > review work easier.
> 
> You don't need to reflect all the layers that the system has in the
> software, if some of the layers are always used together with no
> possibility of replacing them then it can make sense to collapse them in
> software.  That did seem to be the case here.

OK. I understand your point.

> 
> > > > +config REGMAP_SPI_AVMM
> > > > +	tristate
> > > > +	depends on SPI
> 
> > > Note that for selected symbols dependencies don't take effect.
> 
> > I see. So should I change something here? I see the same case for other
> > regmap options.
> 
> It's fine, just be aware that you still need to have appropriate
> dependencies and selects in the user.

OK. Thanks for the reminding.

> 
> > > > +/* The input is a trans stream in trans_buf, format a phy stream in phy_buf. */
> > > > +static void pkt_phy_tx_prepare(char *trans_buf, unsigned int trans_len,
> > > > +			       char *phy_buf, unsigned int *phy_len)
> > > > +{
> > > > +	unsigned int i;
> > > > +	char *b, *p;
> > > > +
> > > > +	b = trans_buf;
> > > > +	p = phy_buf;
> 
> > > I'm not seeing any bounds checking on the size of an operation in this
> > > function and I think I'd expect some, similarly for a lot of the other
> > > I/O operations.
> 
> > I have caculated and allocated 2 buffers that are large enough to
> > contain any possible data stream for regmap_bus.max_raw_read/write. See
> > the definition of TRANS_BUF_SIZE & PHY_BUF_SIZE.
> 
> > So maybe we don't have to check the length?
> 
> This isn't at all clear looking at the code, perhaps it would be clearer
> with fewer layers of abstraction but at the minute it's a lot of effort
> to confirm if it's safe.

OK. I'll add the check.

> 
> > > > +	if (spi->bits_per_word != 8 && spi->bits_per_word != 32)
> > > > +		return ERR_PTR(-EINVAL);
> 
> > > Most controllers support configurable bits per word (and modes) - you
> > > shouldn't be rejecting things based on whatever the default happens to
> > > be.
> 
> > I'm not sure if it is good that the spi_avmm_regmap_init changes the
> > configuration of spi devices silently.
> 
> That's the expected usage.
> 
> > In my current implementation, the spi device driver should be aware and
> > choose the right spi setup before trying to init the regmap. Would it be
> > too hard for other drivers to use it?
> 
> It'd be duplicated effort in all the users, if the only way to use the
> bus is with this configuration then simply saying that they're using the
> bus should be enough.

OK. I could add the spi_setup on init.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-07-23  1:33 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 10:42 [PATCH v2 0/3] add regmap-spi-avmm & Intel Max10 BMC chip support Xu Yilun
2020-07-16 10:42 ` [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support Xu Yilun
2020-07-17 18:15   ` Mark Brown
2020-07-20 17:11     ` Xu Yilun
2020-07-22 11:32       ` Mark Brown
2020-07-23  1:30         ` Xu Yilun
2020-07-16 10:42 ` [PATCH v2 2/3] regmap: spi-avmm: start with the last SOP on phy rx buffer parsing Xu Yilun
2020-07-17 12:11   ` Mark Brown
2020-07-16 10:42 ` [PATCH v2 3/3] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC Xu Yilun
2020-07-17  9:45   ` Lee Jones
2020-07-21  3:26     ` Xu Yilun
2020-07-17 18:16   ` Mark Brown
2020-07-21  3:47     ` Xu Yilun
2020-07-21  3:50       ` Xu Yilun

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.