[PATCH 0/3] Add Samsung SPEEDY serial bus host controller driver

[PATCH 0/3] Add Samsung SPEEDY serial bus host controller driver

[Markuss Broks]

Hey,

This series adds support for the Samsung SPEEDY serial bus host
controller. Samsung SPEEDY (actually an acronym) is a proprietary
Samsung 1 wire serial bus, which is used on various Samsung devices.

This driver adds support for the version of controller without the
IP_BATCHER block. It appears that block is a small MCU attached to
the SPEEDY controller to offload the SPEEDY I/O tasks from the AP.
IP_BATCHER is found on Exynos7885, but not found on Exynos9810 and
Exynos8895. This version of driver should still work on Exynos7885
though, but the IP_BATCHER is not supported at the moment.

On Exynos9810, SPEEDY controllers are also mapped into MMIO space
of other processors on the CPU. For example, APM also has a window
to the SPEEDY IP, and it uses it for power-management related things.
During testing however, it seems that if APM is not active the AP can
access the SPEEDY controller freely and without interference from APM
firmware.

Things to improve:

- SPEEDY host controller has an interrupt line to the AP, but current
implementation uses polling instead,
- add support for handling IP_BATCHER block,
- add support for bulk transfers,
- test on other SoCs (Exynos9820, 9830, 9840, ...).
- runtime PM

- Markuss

To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Alim Akhtar <alim.akhtar@samsung.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-samsung-soc@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Cc: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
Markuss Broks (3):
      dt-bindings: soc: samsung: exynos-speedy: Document SPEEDY host controller bindings
      soc: samsung: Add a driver for Samsung SPEEDY host controller
      MAINTAINERS: Add entry for the Samsung Exynos SPEEDY host controller

 .../bindings/soc/samsung/exynos-speedy.yaml        |  78 ++++
 MAINTAINERS                                        |   7 +
 drivers/soc/samsung/Kconfig                        |  13 +
 drivers/soc/samsung/Makefile                       |   2 +
 drivers/soc/samsung/exynos-speedy.c                | 457 +++++++++++++++++++++
 include/linux/soc/samsung/exynos-speedy.h          |  56 +++
 6 files changed, 613 insertions(+)
---
base-commit: 1b2ab8149928c1cea2d7eca30cd35bb7fe014053
change-id: 20241210-speedy-e43f5df2b1d6

Best regards,
-- 
Markuss Broks <markuss.broks@gmail.com>

[PATCH 1/3] dt-bindings: soc: samsung: exynos-speedy: Document SPEEDY host controller bindings

[Markuss Broks]

Add the schema for the Samsung SPEEDY serial bus host controller.
The bus has 4 bit wide addresses for addressing devices
and 8 bit wide register addressing. Each register is also 8
bit long, so the address can be 0-f (hexadecimal), node name
for child device follows the format: node_name@[0-f].

Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
 .../bindings/soc/samsung/exynos-speedy.yaml        | 78 ++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..304b322a74ea70f23d8c072b44b6ca86b7cc807f
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/samsung/exynos-speedy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung Exynos SPEEDY serial bus host controller
+
+maintainers:
+  - Markuss Broks <markuss.broks@gmail.com>
+
+description:
+  Samsung SPEEDY is a proprietary Samsung serial 1-wire bus.
+  It is used on various Samsung Exynos chips. The bus can
+  address at most 4 bit (16) devices. The devices on the bus
+  have 8 bit long register line, and the registers are also
+  8 bit long each. It is typically used for communicating with
+  Samsung PMICs (s2mps17, s2mps18, ...) and other Samsung chips,
+  such as RF parts.
+
+properties:
+  compatible:
+    - items:
+        - enum:
+            - samsung,exynos9810-speedy
+        - const: samsung,exynos-speedy
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    - const: pclk
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+patternProperties:
+  "^[a-z][a-z0-9]*@[0-9a-f]$":
+    type: object
+    additionalProperties: true
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    speedy0: speedy@141c0000 {
+      compatible = "samsung,exynos9810-speedy",
+                   "samsung-exynos-speedy";
+      reg = <0x141c0000 0x2000>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      pmic@0 {
+        compatible = "samsung,s2mps18-pmic";
+        reg = <0x0>;
+      };
+
+      regulator@1 {
+        compatible = "samsung,s2mps18-regulator";
+        reg = <0x1>;
+      };
+    };

-- 
2.47.0

[PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Markuss Broks]

Add a driver for Samsung SPEEDY serial bus host controller.
SPEEDY is a proprietary 1 wire serial bus used by Samsung
in various devices (usually mobile), like Samsung Galaxy
phones. It is usually used for connecting PMIC or various
other peripherals, like audio codecs or RF components.

This bus can address at most 1MiB (4 bit device address,
8 bit registers per device, 8 bit wide registers:
256*256*16 = 1MiB of address space.

Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
 drivers/soc/samsung/Kconfig               |  13 +
 drivers/soc/samsung/Makefile              |   2 +
 drivers/soc/samsung/exynos-speedy.c       | 457 ++++++++++++++++++++++++++++++
 include/linux/soc/samsung/exynos-speedy.h |  56 ++++
 4 files changed, 528 insertions(+)

diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
index 1a5dfdc978dc4069eb71c4e8eada7ff1913b86b3..a38150fc9999ded1e1e93e2a9ef43b88175d34bd 100644
--- a/drivers/soc/samsung/Kconfig
+++ b/drivers/soc/samsung/Kconfig
@@ -49,6 +49,19 @@ config EXYNOS_PMU_ARM_DRIVERS
 	bool "Exynos PMU ARMv7-specific driver extensions" if COMPILE_TEST
 	depends on EXYNOS_PMU
 
+config EXYNOS_SPEEDY
+	tristate "Exynos SPEEDY host controller driver"
+	depends on ARCH_EXYNOS || COMPILE_TEST
+	depends on OF
+	depends on REGMAP_MMIO
+	help
+	  Enable support for Exynos SPEEDY host controller block.
+	  SPEEDY is a 1 wire proprietary Samsung serial bus, found in
+	  modern Samsung Exynos SoCs, like Exynos8895 and newer.
+
+	  Select this if you have a Samsung Exynos device which uses
+	  SPEEDY bus.
+
 config SAMSUNG_PM_CHECK
 	bool "S3C2410 PM Suspend Memory CRC"
 	depends on PM && (ARCH_S3C64XX || ARCH_S5PV210)
diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
index 248a33d7754af1a1e5fbbbb79413eb300bbbc8e5..9fe824075be77dbcd22c5f1477c4b34029016ed2 100644
--- a/drivers/soc/samsung/Makefile
+++ b/drivers/soc/samsung/Makefile
@@ -12,4 +12,6 @@ obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS)	+= exynos3250-pmu.o exynos4-pmu.o \
 					exynos5250-pmu.o exynos5420-pmu.o
 obj-$(CONFIG_EXYNOS_REGULATOR_COUPLER) += exynos-regulator-coupler.o
 
+obj-$(CONFIG_EXYNOS_SPEEDY)	+= exynos-speedy.o
+
 obj-$(CONFIG_SAMSUNG_PM_CHECK)	+= s3c-pm-check.o
diff --git a/drivers/soc/samsung/exynos-speedy.c b/drivers/soc/samsung/exynos-speedy.c
new file mode 100644
index 0000000000000000000000000000000000000000..e897b67c80edacee485169ea6dc1ffe0cefdd21f
--- /dev/null
+++ b/drivers/soc/samsung/exynos-speedy.c
@@ -0,0 +1,457 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * exynos-speedy.c - Samsung Exynos SPEEDY Host Controller Driver
+ *
+ * Copyright 2024, Markuss Broks <markuss.broks@gmail.com>
+ * Copyright 2024, Maksym Holovach <nergzd@nergzd723.xyz>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#include <linux/soc/samsung/exynos-speedy.h>
+
+/* Speedy MMIO register map */
+#define SPEEDY_CTRL					0x000
+#define SPEEDY_FIFO_CTRL				0x004
+#define SPEEDY_CMD					0x008
+#define SPEEDY_INT_ENABLE				0x00C
+#define SPEEDY_INT_STATUS				0x010
+#define SPEEDY_FIFO_STATUS				0x030
+#define SPEEDY_TX_DATA					0x034
+#define SPEEDY_RX_DATA					0x038
+#define SPEEDY_PACKET_GAP_TIME				0x044
+#define SPEEDY_TIMEOUT_COUNT				0x048
+#define SPEEDY_FIFO_DEBUG				0x100
+#define SPEEDY_CTRL_STATUS				0x104
+
+/* SPEEDY_CTRL register bits */
+#define SPEEDY_ENABLE					(1 << 0)
+#define SPEEDY_TIMEOUT_CMD_DISABLE			(1 << 1)
+#define SPEEDY_TIMEOUT_STANDBY_DISABLE			(1 << 2)
+#define SPEEDY_TIMEOUT_DATA_DISABLE			(1 << 3)
+#define SPEEDY_ALWAYS_PULLUP_EN				(1 << 7)
+#define SPEEDY_DATA_WIDTH_8BIT				(0 << 8)
+#define SPEEDY_REMOTE_RESET_REQ				(1 << 30)
+#define SPEEDY_SW_RST					(1 << 31)
+
+/* SPEEDY_FIFO_CTRL register bits */
+#define SPEEDY_RX_TRIGGER_LEVEL(x)			((x) << 0)
+#define SPEEDY_TX_TRIGGER_LEVEL(x)			((x) << 8)
+#define SPEEDY_FIFO_RESET				(1 << 31)
+
+/* SPEEDY_CMD register bits */
+#define SPEEDY_BURST_LENGTH(x)				((x) << 0)
+#define SPEEDY_BURST_FIXED				(0 << 5)
+#define SPEEDY_BURST_INCR				(1 << 5)
+#define SPEEDY_BURST_EXTENSION				(2 << 5)
+#define SPEEDY_ACCESS_BURST				(0 << 19)
+#define SPEEDY_ACCESS_RANDOM				(1 << 19)
+#define SPEEDY_DIRECTION_READ				(0 << 20)
+#define SPEEDY_DIRECTION_WRITE				(1 << 20)
+
+/* SPEEDY_INT_ENABLE register bits */
+#define SPEEDY_TRANSFER_DONE_EN				(1 << 0)
+#define SPEEDY_TIMEOUT_CMD_EN				(1 << 1)
+#define SPEEDY_TIMEOUT_STANDBY_EN			(1 << 2)
+#define SPEEDY_TIMEOUT_DATA_EN				(1 << 3)
+#define SPEEDY_FIFO_TX_ALMOST_EMPTY_EN			(1 << 4)
+#define SPEEDY_FIFO_RX_ALMOST_FULL_EN			(1 << 8)
+#define SPEEDY_RX_FIFO_INT_TRAILER_EN			(1 << 9)
+#define SPEEDY_RX_MODEBIT_ERR_EN			(1 << 16)
+#define SPEEDY_RX_GLITCH_ERR_EN				(1 << 17)
+#define SPEEDY_RX_ENDBIT_ERR_EN				(1 << 18)
+#define SPEEDY_TX_LINE_BUSY_ERR_EN			(1 << 20)
+#define SPEEDY_TX_STOPBIT_ERR_EN			(1 << 21)
+#define SPEEDY_REMOTE_RESET_REQ_EN			(1 << 31)
+
+/* SPEEDY_INT_STATUS register bits */
+#define SPEEDY_TRANSFER_DONE				(1 << 0)
+#define SPEEDY_TIMEOUT_CMD				(1 << 1)
+#define SPEEDY_TIMEOUT_STANDBY				(1 << 2)
+#define SPEEDY_TIMEOUT_DATA				(1 << 3)
+#define SPEEDY_FIFO_TX_ALMOST_EMPTY			(1 << 4)
+#define SPEEDY_FIFO_RX_ALMOST_FULL			(1 << 8)
+#define SPEEDY_RX_FIFO_INT_TRAILER			(1 << 9)
+#define SPEEDY_RX_MODEBIT_ERR				(1 << 16)
+#define SPEEDY_RX_GLITCH_ERR				(1 << 17)
+#define SPEEDY_RX_ENDBIT_ERR				(1 << 18)
+#define SPEEDY_TX_LINE_BUSY_ERR				(1 << 20)
+#define SPEEDY_TX_STOPBIT_ERR				(1 << 21)
+#define SPEEDY_REMOTE_RESET_REQ_STAT			(1 << 31)
+
+/* SPEEDY_FIFO_STATUS register bits */
+#define SPEEDY_VALID_DATA_CNT				(0 << 0)
+#define SPEEDY_FIFO_FULL				(1 << 5)
+#define SPEEDY_FIFO_EMPTY				(1 << 6)
+
+/* SPEEDY_PACKET_GAP_TIME register bits */
+#define SPEEDY_FIFO_TX_ALMOST_EMPTY			(1 << 4)
+#define SPEEDY_FIFO_RX_ALMOST_FULL			(1 << 8)
+#define SPEEDY_FSM_INIT					(1 << 1)
+#define SPEEDY_FSM_TX_CMD				(1 << 2)
+#define SPEEDY_FSM_STANDBY				(1 << 3)
+#define SPEEDY_FSM_DATA					(1 << 4)
+#define SPEEDY_FSM_TIMEOUT				(1 << 5)
+#define SPEEDY_FSM_TRANS_DONE				(1 << 6)
+#define SPEEDY_FSM_IO_RX_STAT_MASK			(3 << 7)
+#define SPEEDY_FSM_IO_TX_IDLE				(1 << 9)
+#define SPEEDY_FSM_IO_TX_GET_PACKET			(1 << 10)
+#define SPEEDY_FSM_IO_TX_PACKET				(1 << 11)
+#define SPEEDY_FSM_IO_TX_DONE				(1 << 12)
+
+#define SPEEDY_RX_LENGTH(n)				((n) << 0)
+#define SPEEDY_TX_LENGTH(n)				((n) << 8)
+
+#define SPEEDY_DEVICE(x)				((x & 0xf) << 15)
+#define SPEEDY_ADDRESS(x)				((x & 0xff) << 7)
+
+static const struct of_device_id speedy_match[] = {
+	{ .compatible = "samsung,exynos9810-speedy" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, speedy_match);
+
+static const struct regmap_config speedy_map_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+};
+
+/**
+ * speedy_int_clear() - clear interrupt status
+ * @speedy:	pointer to speedy controller struct
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static int speedy_int_clear(struct speedy_controller *speedy)
+{
+	int ret;
+
+	ret = regmap_set_bits(speedy->map, SPEEDY_INT_STATUS, 0xffffffff);
+	if (ret)
+		return ret;
+
+	udelay(10);
+
+	return 0;
+}
+
+/**
+ * speedy_fifo_reset() - reset FIFO of the controller
+ * @speedy:	pointer to speedy controller struct
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static int speedy_fifo_reset(struct speedy_controller *speedy)
+{
+	int ret;
+
+	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL, SPEEDY_FIFO_RESET);
+	if (ret)
+		return ret;
+
+	udelay(10);
+
+	return 0;
+}
+
+/**
+ * _speedy_read() - internal speedy read operation
+ * @speedy:	pointer to speedy controller struct
+ * @reg:	address of device on the bus
+ * @addr:       address to read
+ * @val:        pointer to store result
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static int _speedy_read(struct speedy_controller *speedy, u32 reg, u32 addr, u32 *val)
+{
+	int ret;
+	u32 cmd, int_ctl, int_status;
+
+	mutex_lock(&speedy->io_lock);
+
+	ret = speedy_fifo_reset(speedy);
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
+			      SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
+	if (ret)
+		return ret;
+
+	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
+	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
+
+	int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
+		  SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
+		  SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
+		  SPEEDY_REMOTE_RESET_REQ_EN;
+
+	ret = speedy_int_clear(speedy);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
+	if (ret)
+		return ret;
+
+	/* Wait for xfer done */
+	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
+				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
+	if (ret)
+		return ret;
+
+	ret = speedy_int_clear(speedy);
+
+	mutex_unlock(&speedy->io_lock);
+
+	return ret;
+}
+
+int exynos_speedy_read(const struct speedy_device *device, u32 addr, u32 *val)
+{
+	return _speedy_read(device->speedy, device->reg, addr, val);
+}
+EXPORT_SYMBOL_GPL(exynos_speedy_read);
+
+/**
+ * _speedy_write() - internal speedy write operation
+ * @speedy:	pointer to speedy controller struct
+ * @reg:	address of device on the bus
+ * @addr:       address to write
+ * @val:        value to write
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static int _speedy_write(struct speedy_controller *speedy, u32 reg, u32 addr, u32 val)
+{
+	int ret;
+	u32 cmd, int_ctl, int_status;
+
+	mutex_lock(&speedy->io_lock);
+
+	ret = speedy_fifo_reset(speedy);
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
+			      SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
+	if (ret)
+		return ret;
+
+	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_WRITE |
+	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
+
+	int_ctl = (SPEEDY_TRANSFER_DONE_EN |
+		   SPEEDY_FIFO_TX_ALMOST_EMPTY_EN |
+		   SPEEDY_TX_LINE_BUSY_ERR_EN |
+		   SPEEDY_TX_STOPBIT_ERR_EN |
+		   SPEEDY_REMOTE_RESET_REQ_EN);
+
+	ret = speedy_int_clear(speedy);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(speedy->map, SPEEDY_TX_DATA, val);
+	if (ret)
+		return ret;
+
+	/* Wait for xfer done */
+	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
+				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
+	if (ret)
+		return ret;
+
+	speedy_int_clear(speedy);
+
+	mutex_unlock(&speedy->io_lock);
+
+	return 0;
+}
+
+int exynos_speedy_write(const struct speedy_device *device, u32 addr, u32 val)
+{
+	return _speedy_write(device->speedy, device->reg, addr, val);
+}
+EXPORT_SYMBOL_GPL(exynos_speedy_write);
+
+static void devm_speedy_release(struct device *dev, void *res)
+{
+	const struct speedy_device **ptr = res;
+	const struct speedy_device *handle = *ptr;
+
+	kfree(handle);
+}
+
+/**
+ * speedy_get_by_phandle() - internal get speedy device handle
+ * @np:	pointer to OF device node of device
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static const struct speedy_device *speedy_get_device(struct device_node *np)
+{
+	const struct of_device_id *speedy_id;
+	struct device_node *speedy_np;
+	struct platform_device *speedy_pdev;
+	struct speedy_controller *speedy = NULL;
+	struct speedy_device *handle;
+	int ret;
+
+	if (!np) {
+		pr_err("I need a device pointer\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	speedy_np = of_get_parent(np);
+	if (!speedy_np)
+		return ERR_PTR(-ENODEV);
+
+	/* Verify if parent node is a speedy controller */
+	speedy_id = of_match_node(speedy_match, speedy_np);
+	if (!speedy_id) {
+		handle = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	/* Get platform device of the speedy controller */
+	speedy_pdev = of_find_device_by_node(speedy_np);
+	if (!speedy_pdev) {
+		handle = ERR_PTR(-EPROBE_DEFER);
+		goto out;
+	}
+
+	/* Get drvdata of speedy controller */
+	speedy = platform_get_drvdata(speedy_pdev);
+	if (!speedy) {
+		handle = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	handle = kzalloc(sizeof(struct speedy_device), GFP_KERNEL);
+	if (!handle) {
+		handle = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+	handle->speedy = speedy;
+	ret = of_property_read_u32(np, "reg", &handle->reg);
+	if (ret) {
+		kfree(handle);
+		handle = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+out:
+	of_node_put(speedy_np);
+	return handle;
+}
+
+const struct speedy_device *devm_speedy_get_device(struct device *dev)
+{
+	const struct speedy_device *handle;
+	const struct speedy_device **ptr;
+
+	ptr = devres_alloc(devm_speedy_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	handle = speedy_get_device(dev_of_node(dev));
+	if (!IS_ERR(handle)) {
+		*ptr = handle;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return handle;
+}
+EXPORT_SYMBOL_GPL(devm_speedy_get_device);
+
+static int speedy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct speedy_controller *speedy;
+	void __iomem *mem;
+	int ret;
+
+	speedy = devm_kzalloc(dev, sizeof(struct speedy_controller), GFP_KERNEL);
+	if (!speedy)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, speedy);
+	speedy->pdev = pdev;
+
+	mutex_init(&speedy->io_lock);
+
+	mem = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mem))
+		return dev_err_probe(dev, PTR_ERR(mem), "Failed to ioremap memory\n");
+
+	speedy->map = devm_regmap_init_mmio(dev, mem, &speedy_map_cfg);
+	if (IS_ERR(speedy->map))
+		return dev_err_probe(dev, PTR_ERR(speedy->map), "Failed to init the regmap\n");
+
+	/* Clear any interrupt status remaining */
+	ret = speedy_int_clear(speedy);
+	if (ret)
+		return ret;
+
+	/* Reset the controller */
+	ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_SW_RST);
+	if (ret)
+		return ret;
+
+	msleep(20);
+
+	/* Enable the hw */
+	ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_ENABLE);
+	if (ret)
+		return ret;
+
+	msleep(20);
+
+	/* Probe child devices */
+	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
+	if (ret)
+		dev_err(dev, "Failed to populate child devices: %d\n", ret);
+
+	return ret;
+}
+
+static struct platform_driver speedy_driver = {
+	.probe = speedy_probe,
+	.driver = {
+		.name = "exynos-speedy",
+		.of_match_table = speedy_match,
+	},
+};
+
+module_platform_driver(speedy_driver);
+
+MODULE_DESCRIPTION("Samsung Exynos SPEEDY host controller driver");
+MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/soc/samsung/exynos-speedy.h b/include/linux/soc/samsung/exynos-speedy.h
new file mode 100644
index 0000000000000000000000000000000000000000..b2857d65d3b50927373866dd8ae1c47e98af6d7b
--- /dev/null
+++ b/include/linux/soc/samsung/exynos-speedy.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2024, Markuss Broks <markuss.broks@gmail.com>
+ * Copyright 2024, Maksym Holovach <nergzd@nergzd723.xyz>
+ */
+
+#ifndef __EXYNOS_SPEEDY_H
+#define __EXYNOS_SPEEDY_H
+
+#include <linux/types.h>
+
+struct device;
+struct mutex;
+struct platform_device;
+struct regmap;
+
+struct speedy_controller {
+	struct mutex io_lock;
+	struct platform_device *pdev;
+	struct regmap *map;
+};
+
+struct speedy_device {
+	u32 reg;
+	struct speedy_controller *speedy;
+};
+
+/**
+ * exynos_speedy_read() - exynos speedy read operation
+ * @device:	pointer to speedy device struct
+ * @addr:       address to read
+ * @val:        pointer to store result
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+int exynos_speedy_read(const struct speedy_device *device, u32 addr, u32 *val);
+
+/**
+ * exynos_speedy_write() - exynos speedy write operation
+ * @device:	pointer to speedy device struct
+ * @addr:       address to write
+ * @val:        value to write
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+int exynos_speedy_write(const struct speedy_device *device, u32 addr, u32 val);
+
+/**
+ * devm_speedy_get_device() - managed get speedy device.
+ * @dev:	device pointer requesting speedy device handle.
+ *
+ * Return: pointer to handle on success, ERR_PTR(-errno) otherwise.
+ */
+const struct speedy_device *devm_speedy_get_device(struct device *dev);
+
+#endif /* __EXYNOS_SPEEDY_H */

-- 
2.47.0

[PATCH 3/3] MAINTAINERS: Add entry for the Samsung Exynos SPEEDY host controller

[Markuss Broks]

Add an entry adding myself as a maintainer of the Exynos SPEEDY
host controller driver.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bf2dcd4e0261785add520b5eac747ceac523e112..14041c5373df76c7a7093f106dded1080b5d8664 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20754,6 +20754,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/rng/samsung,exynos4-rng.yaml
 F:	drivers/crypto/exynos-rng.c
 
+SAMSUNG EXYNOS SPEEDY SERIAL HOST CONTROLLER DRIVER
+M:	Markuss Broks <markuss.broks@gmail.com>
+L:	linux-samsung-soc@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml
+F:	drivers/soc/samsung/exynos-speedy.c
+
 SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
 M:	Łukasz Stelmach <l.stelmach@samsung.com>
 L:	linux-samsung-soc@vger.kernel.org

-- 
2.47.0

Re: [PATCH 1/3] dt-bindings: soc: samsung: exynos-speedy: Document SPEEDY host controller bindings

[Rob Herring (Arm)]

On Thu, 12 Dec 2024 23:09:01 +0200, Markuss Broks wrote:
> Add the schema for the Samsung SPEEDY serial bus host controller.
> The bus has 4 bit wide addresses for addressing devices
> and 8 bit wide register addressing. Each register is also 8
> bit long, so the address can be 0-f (hexadecimal), node name
> for child device follows the format: node_name@[0-f].
> 
> Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
> Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>  .../bindings/soc/samsung/exynos-speedy.yaml        | 78 ++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml: ignoring, error in schema: properties: clock-names
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml: properties:compatible: [{'items': [{'enum': ['samsung,exynos9810-speedy']}, {'const': 'samsung,exynos-speedy'}]}] is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml: properties:clock-names: [{'const': 'pclk'}] is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml: properties:compatible: [{'items': [{'enum': ['samsung,exynos9810-speedy']}, {'const': 'samsung,exynos-speedy'}]}] is not of type 'object', 'boolean'
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml: properties:clock-names: [{'const': 'pclk'}] is not of type 'object', 'boolean'
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
Documentation/devicetree/bindings/soc/samsung/exynos-speedy.example.dtb: /example-0/speedy@141c0000: failed to match any schema with compatible: ['samsung,exynos9810-speedy', 'samsung-exynos-speedy']
Documentation/devicetree/bindings/soc/samsung/exynos-speedy.example.dtb: /example-0/speedy@141c0000: failed to match any schema with compatible: ['samsung,exynos9810-speedy', 'samsung-exynos-speedy']
Documentation/devicetree/bindings/soc/samsung/exynos-speedy.example.dtb: /example-0/speedy@141c0000/pmic@0: failed to match any schema with compatible: ['samsung,s2mps18-pmic']
Documentation/devicetree/bindings/soc/samsung/exynos-speedy.example.dtb: /example-0/speedy@141c0000/regulator@1: failed to match any schema with compatible: ['samsung,s2mps18-regulator']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241212-speedy-v1-1-544ad7bcfb6a@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/3] dt-bindings: soc: samsung: exynos-speedy: Document SPEEDY host controller bindings

[Krzysztof Kozlowski]

On 12/12/2024 22:09, Markuss Broks wrote:
> Add the schema for the Samsung SPEEDY serial bus host controller.
> The bus has 4 bit wide addresses for addressing devices
> and 8 bit wide register addressing. Each register is also 8
> bit long, so the address can be 0-f (hexadecimal), node name
> for child device follows the format: node_name@[0-f].


This wasn't tested so limited review.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
> Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>  .../bindings/soc/samsung/exynos-speedy.yaml        | 78 ++++++++++++++++++++++

Filename must match compatible.

>  1 file changed, 78 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..304b322a74ea70f23d8c072b44b6ca86b7cc807f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/samsung/exynos-speedy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung Exynos SPEEDY serial bus host controller

Speedy or SPEEDY?

> +
> +maintainers:
> +  - Markuss Broks <markuss.broks@gmail.com>
> +
> +description:
> +  Samsung SPEEDY is a proprietary Samsung serial 1-wire bus.

1-wire? But not compatible with w1 (onwire)?

> +  It is used on various Samsung Exynos chips. The bus can
> +  address at most 4 bit (16) devices. The devices on the bus
> +  have 8 bit long register line, and the registers are also
> +  8 bit long each. It is typically used for communicating with
> +  Samsung PMICs (s2mps17, s2mps18, ...) and other Samsung chips,
> +  such as RF parts.
> +
> +properties:
> +  compatible:
> +    - items:
> +        - enum:
> +            - samsung,exynos9810-speedy
> +        - const: samsung,exynos-speedy

Drop last compatible and use only SoC specific.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    - const: pclk

Drop clock-names, not needed for one entry.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"

You do not have them in the properties, anyway required goes before
additionalProperties

> +
> +patternProperties:
> +  "^[a-z][a-z0-9]*@[0-9a-f]$":

That's odd regex. Look at other bus bindings.

> +    type: object
> +    additionalProperties: true
> +
> +    properties:
> +      reg:
> +        maxItems: 1

maximum: 15

> +
> +    required:
> +      - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    speedy0: speedy@141c0000 {

Drop unused label.

> +      compatible = "samsung,exynos9810-speedy",
> +                   "samsung-exynos-speedy";
> +      reg = <0x141c0000 0x2000>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +

No resources? No clocks? No interrupts?



Best regards,
Krzysztof

Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Krzysztof Kozlowski]

On 12/12/2024 22:09, Markuss Broks wrote:
> Add a driver for Samsung SPEEDY serial bus host controller.
> SPEEDY is a proprietary 1 wire serial bus used by Samsung
> in various devices (usually mobile), like Samsung Galaxy
> phones. It is usually used for connecting PMIC or various
> other peripherals, like audio codecs or RF components.
> 
> This bus can address at most 1MiB (4 bit device address,
> 8 bit registers per device, 8 bit wide registers:
> 256*256*16 = 1MiB of address space.
> 
> Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
> Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>  drivers/soc/samsung/Kconfig               |  13 +
>  drivers/soc/samsung/Makefile              |   2 +
>  drivers/soc/samsung/exynos-speedy.c       | 457 ++++++++++++++++++++++++++++++
>  include/linux/soc/samsung/exynos-speedy.h |  56 ++++
>  4 files changed, 528 insertions(+)
> 
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index 1a5dfdc978dc4069eb71c4e8eada7ff1913b86b3..a38150fc9999ded1e1e93e2a9ef43b88175d34bd 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -49,6 +49,19 @@ config EXYNOS_PMU_ARM_DRIVERS
>  	bool "Exynos PMU ARMv7-specific driver extensions" if COMPILE_TEST
>  	depends on EXYNOS_PMU
>  
> +config EXYNOS_SPEEDY
> +	tristate "Exynos SPEEDY host controller driver"
> +	depends on ARCH_EXYNOS || COMPILE_TEST
> +	depends on OF
> +	depends on REGMAP_MMIO
> +	help
> +	  Enable support for Exynos SPEEDY host controller block.
> +	  SPEEDY is a 1 wire proprietary Samsung serial bus, found in
> +	  modern Samsung Exynos SoCs, like Exynos8895 and newer.


I did not check that much but this looks like 1wire for which we have
subsystem. Please investigate more and figure out the differences.

> +
> +	  Select this if you have a Samsung Exynos device which uses
> +	  SPEEDY bus.
> +


> +
> +/* SPEEDY_PACKET_GAP_TIME register bits */
> +#define SPEEDY_FIFO_TX_ALMOST_EMPTY			(1 << 4)
> +#define SPEEDY_FIFO_RX_ALMOST_FULL			(1 << 8)
> +#define SPEEDY_FSM_INIT					(1 << 1)
> +#define SPEEDY_FSM_TX_CMD				(1 << 2)
> +#define SPEEDY_FSM_STANDBY				(1 << 3)
> +#define SPEEDY_FSM_DATA					(1 << 4)
> +#define SPEEDY_FSM_TIMEOUT				(1 << 5)
> +#define SPEEDY_FSM_TRANS_DONE				(1 << 6)
> +#define SPEEDY_FSM_IO_RX_STAT_MASK			(3 << 7)
> +#define SPEEDY_FSM_IO_TX_IDLE				(1 << 9)
> +#define SPEEDY_FSM_IO_TX_GET_PACKET			(1 << 10)
> +#define SPEEDY_FSM_IO_TX_PACKET				(1 << 11)
> +#define SPEEDY_FSM_IO_TX_DONE				(1 << 12)
> +
> +#define SPEEDY_RX_LENGTH(n)				((n) << 0)
> +#define SPEEDY_TX_LENGTH(n)				((n) << 8)
> +
> +#define SPEEDY_DEVICE(x)				((x & 0xf) << 15)
> +#define SPEEDY_ADDRESS(x)				((x & 0xff) << 7)
> +
> +static const struct of_device_id speedy_match[] = {
> +	{ .compatible = "samsung,exynos9810-speedy" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, speedy_match);

This is never at top of the file, but immediately before driver
structure. Look at other drivers.

> +
> +static const struct regmap_config speedy_map_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +};
> +
> +/**
> + * speedy_int_clear() - clear interrupt status
> + * @speedy:	pointer to speedy controller struct
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static int speedy_int_clear(struct speedy_controller *speedy)
> +{
> +	int ret;
> +
> +	ret = regmap_set_bits(speedy->map, SPEEDY_INT_STATUS, 0xffffffff);
> +	if (ret)
> +		return ret;
> +
> +	udelay(10);
> +
> +	return 0;
> +}
> +
> +/**
> + * speedy_fifo_reset() - reset FIFO of the controller
> + * @speedy:	pointer to speedy controller struct
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static int speedy_fifo_reset(struct speedy_controller *speedy)
> +{
> +	int ret;
> +
> +	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL, SPEEDY_FIFO_RESET);
> +	if (ret)
> +		return ret;
> +
> +	udelay(10);
> +
> +	return 0;
> +}
> +
> +/**
> + * _speedy_read() - internal speedy read operation
> + * @speedy:	pointer to speedy controller struct
> + * @reg:	address of device on the bus
> + * @addr:       address to read
> + * @val:        pointer to store result
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static int _speedy_read(struct speedy_controller *speedy, u32 reg, u32 addr, u32 *val)
> +{
> +	int ret;
> +	u32 cmd, int_ctl, int_status;
> +
> +	mutex_lock(&speedy->io_lock);
> +
> +	ret = speedy_fifo_reset(speedy);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
> +			      SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
> +	if (ret)
> +		return ret;
> +
> +	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
> +	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
> +
> +	int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
> +		  SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
> +		  SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
> +		  SPEEDY_REMOTE_RESET_REQ_EN;
> +
> +	ret = speedy_int_clear(speedy);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for xfer done */
> +	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
> +				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = speedy_int_clear(speedy);
> +
> +	mutex_unlock(&speedy->io_lock);
> +
> +	return ret;
> +}
> +
> +int exynos_speedy_read(const struct speedy_device *device, u32 addr, u32 *val)
> +{
> +	return _speedy_read(device->speedy, device->reg, addr, val);
> +}
> +EXPORT_SYMBOL_GPL(exynos_speedy_read);

Nope, drop, unused.

> +
> +/**
> + * _speedy_write() - internal speedy write operation
> + * @speedy:	pointer to speedy controller struct
> + * @reg:	address of device on the bus
> + * @addr:       address to write
> + * @val:        value to write
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static int _speedy_write(struct speedy_controller *speedy, u32 reg, u32 addr, u32 val)
> +{
> +	int ret;
> +	u32 cmd, int_ctl, int_status;
> +
> +	mutex_lock(&speedy->io_lock);
> +
> +	ret = speedy_fifo_reset(speedy);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
> +			      SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
> +	if (ret)
> +		return ret;
> +
> +	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_WRITE |
> +	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
> +
> +	int_ctl = (SPEEDY_TRANSFER_DONE_EN |
> +		   SPEEDY_FIFO_TX_ALMOST_EMPTY_EN |
> +		   SPEEDY_TX_LINE_BUSY_ERR_EN |
> +		   SPEEDY_TX_STOPBIT_ERR_EN |
> +		   SPEEDY_REMOTE_RESET_REQ_EN);
> +
> +	ret = speedy_int_clear(speedy);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_TX_DATA, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for xfer done */
> +	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
> +				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
> +	if (ret)
> +		return ret;
> +
> +	speedy_int_clear(speedy);
> +
> +	mutex_unlock(&speedy->io_lock);
> +
> +	return 0;
> +}
> +
> +int exynos_speedy_write(const struct speedy_device *device, u32 addr, u32 val)
> +{
> +	return _speedy_write(device->speedy, device->reg, addr, val);

Just write the function here and drop _.

> +}
> +EXPORT_SYMBOL_GPL(exynos_speedy_write);
> +
> +static void devm_speedy_release(struct device *dev, void *res)
> +{
> +	const struct speedy_device **ptr = res;
> +	const struct speedy_device *handle = *ptr;
> +
> +	kfree(handle);
> +}
> +
> +/**
> + * speedy_get_by_phandle() - internal get speedy device handle
> + * @np:	pointer to OF device node of device
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static const struct speedy_device *speedy_get_device(struct device_node *np)
> +{
> +	const struct of_device_id *speedy_id;
> +	struct device_node *speedy_np;
> +	struct platform_device *speedy_pdev;
> +	struct speedy_controller *speedy = NULL;
> +	struct speedy_device *handle;
> +	int ret;
> +
> +	if (!np) {
> +		pr_err("I need a device pointer\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	speedy_np = of_get_parent(np);
> +	if (!speedy_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	/* Verify if parent node is a speedy controller */
> +	speedy_id = of_match_node(speedy_match, speedy_np);
> +	if (!speedy_id) {
> +		handle = ERR_PTR(-EINVAL);
> +		goto out;
> +	}
> +
> +	/* Get platform device of the speedy controller */
> +	speedy_pdev = of_find_device_by_node(speedy_np);
> +	if (!speedy_pdev) {
> +		handle = ERR_PTR(-EPROBE_DEFER);
> +		goto out;
> +	}
> +
> +	/* Get drvdata of speedy controller */
> +	speedy = platform_get_drvdata(speedy_pdev);
> +	if (!speedy) {
> +		handle = ERR_PTR(-EINVAL);
> +		goto out;
> +	}
> +
> +	handle = kzalloc(sizeof(struct speedy_device), GFP_KERNEL);
> +	if (!handle) {
> +		handle = ERR_PTR(-ENOMEM);
> +		goto out;
> +	}
> +	handle->speedy = speedy;
> +	ret = of_property_read_u32(np, "reg", &handle->reg);
> +	if (ret) {
> +		kfree(handle);
> +		handle = ERR_PTR(-EINVAL);
> +		goto out;
> +	}
> +
> +out:
> +	of_node_put(speedy_np);
> +	return handle;
> +}
> +
> +const struct speedy_device *devm_speedy_get_device(struct device *dev)

Not used, drop function.

> +{
> +	const struct speedy_device *handle;
> +	const struct speedy_device **ptr;
> +
> +	ptr = devres_alloc(devm_speedy_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	handle = speedy_get_device(dev_of_node(dev));
> +	if (!IS_ERR(handle)) {
> +		*ptr = handle;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return handle;
> +}
> +EXPORT_SYMBOL_GPL(devm_speedy_get_device);

Not used, drop.



> +
> +static int speedy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct speedy_controller *speedy;
> +	void __iomem *mem;
> +	int ret;
> +
> +	speedy = devm_kzalloc(dev, sizeof(struct speedy_controller), GFP_KERNEL);

sizeof(*) everywhere. You need to clean the downstream code when
upstreaming.

> +	if (!speedy)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, speedy);
> +	speedy->pdev = pdev;
> +
> +	mutex_init(&speedy->io_lock);
> +
> +	mem = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mem))
> +		return dev_err_probe(dev, PTR_ERR(mem), "Failed to ioremap memory\n");
> +
> +	speedy->map = devm_regmap_init_mmio(dev, mem, &speedy_map_cfg);
> +	if (IS_ERR(speedy->map))
> +		return dev_err_probe(dev, PTR_ERR(speedy->map), "Failed to init the regmap\n");

Wrap at 80.

> +
> +	/* Clear any interrupt status remaining */
> +	ret = speedy_int_clear(speedy);
> +	if (ret)
> +		return ret;
> +
> +	/* Reset the controller */
> +	ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_SW_RST);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	/* Enable the hw */
> +	ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	/* Probe child devices */
> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
> +	if (ret)
> +		dev_err(dev, "Failed to populate child devices: %d\n", ret);
> +
> +	return ret;

Missing cleanup in remove().

> +}
> +
> +static struct platform_driver speedy_driver = {
> +	.probe = speedy_probe,
> +	.driver = {
> +		.name = "exynos-speedy",
> +		.of_match_table = speedy_match,
> +	},
> +};
> +
> +module_platform_driver(speedy_driver);
> +
> +MODULE_DESCRIPTION("Samsung Exynos SPEEDY host controller driver");
> +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/soc/samsung/exynos-speedy.h b/include/linux/soc/samsung/exynos-speedy.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..b2857d65d3b50927373866dd8ae1c47e98af6d7b
> --- /dev/null
> +++ b/include/linux/soc/samsung/exynos-speedy.h

Drop the header, not used.



Best regards,
Krzysztof

Re: [PATCH 0/3] Add Samsung SPEEDY serial bus host controller driver

[Krzysztof Kozlowski]

On 12/12/2024 22:09, Markuss Broks wrote:
> Hey,
> 
> This series adds support for the Samsung SPEEDY serial bus host
> controller. Samsung SPEEDY (actually an acronym) is a proprietary
> Samsung 1 wire serial bus, which is used on various Samsung devices.
> 
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Please run standard kernel tools for static analysis, like coccinelle,
smatch and sparse, and fix reported warnings. Also please check for
warnings when building with W=1. Most of these commands (checks or W=1
build) can build specific targets, like some directory, to narrow the
scope to only your code. The code here looks like it needs a fix. Feel
free to get in touch if the warning is not clear.

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

Best regards,
Krzysztof

Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Markuss Broks]

Hi Krzysztof,

On 12/13/24 9:49 AM, Krzysztof Kozlowski wrote:
> On 12/12/2024 22:09, Markuss Broks wrote:
>> Add a driver for Samsung SPEEDY serial bus host controller.
>> SPEEDY is a proprietary 1 wire serial bus used by Samsung
>> in various devices (usually mobile), like Samsung Galaxy
>> phones. It is usually used for connecting PMIC or various
>> other peripherals, like audio codecs or RF components.
>>
>> This bus can address at most 1MiB (4 bit device address,
>> 8 bit registers per device, 8 bit wide registers:
>> 256*256*16 = 1MiB of address space.
>>
>> Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
>> Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>> ---
>>   drivers/soc/samsung/Kconfig               |  13 +
>>   drivers/soc/samsung/Makefile              |   2 +
>>   drivers/soc/samsung/exynos-speedy.c       | 457 ++++++++++++++++++++++++++++++
>>   include/linux/soc/samsung/exynos-speedy.h |  56 ++++
>>   4 files changed, 528 insertions(+)
>>
>> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
>> index 1a5dfdc978dc4069eb71c4e8eada7ff1913b86b3..a38150fc9999ded1e1e93e2a9ef43b88175d34bd 100644
>> --- a/drivers/soc/samsung/Kconfig
>> +++ b/drivers/soc/samsung/Kconfig
>> @@ -49,6 +49,19 @@ config EXYNOS_PMU_ARM_DRIVERS
>>   	bool "Exynos PMU ARMv7-specific driver extensions" if COMPILE_TEST
>>   	depends on EXYNOS_PMU
>>   
>> +config EXYNOS_SPEEDY
>> +	tristate "Exynos SPEEDY host controller driver"
>> +	depends on ARCH_EXYNOS || COMPILE_TEST
>> +	depends on OF
>> +	depends on REGMAP_MMIO
>> +	help
>> +	  Enable support for Exynos SPEEDY host controller block.
>> +	  SPEEDY is a 1 wire proprietary Samsung serial bus, found in
>> +	  modern Samsung Exynos SoCs, like Exynos8895 and newer.
>
> I did not check that much but this looks like 1wire for which we have
> subsystem. Please investigate more and figure out the differences.

This is not compatible with Dallas Semi 1-Wire bus. There are several 
differences but the phy level is not compatible, looking at the Samsung 
patent. [1] The most obvious difference is that 1-Wire is discoverable, 
and this bus isn't. I'm pretty sure this is Samsung's own solution to a 
serial interface through only one wire.

>
>> +
>> +	  Select this if you have a Samsung Exynos device which uses
>> +	  SPEEDY bus.
>> +
>
>> +
>> +/* SPEEDY_PACKET_GAP_TIME register bits */
>> +#define SPEEDY_FIFO_TX_ALMOST_EMPTY			(1 << 4)
>> +#define SPEEDY_FIFO_RX_ALMOST_FULL			(1 << 8)
>> +#define SPEEDY_FSM_INIT					(1 << 1)
>> +#define SPEEDY_FSM_TX_CMD				(1 << 2)
>> +#define SPEEDY_FSM_STANDBY				(1 << 3)
>> +#define SPEEDY_FSM_DATA					(1 << 4)
>> +#define SPEEDY_FSM_TIMEOUT				(1 << 5)
>> +#define SPEEDY_FSM_TRANS_DONE				(1 << 6)
>> +#define SPEEDY_FSM_IO_RX_STAT_MASK			(3 << 7)
>> +#define SPEEDY_FSM_IO_TX_IDLE				(1 << 9)
>> +#define SPEEDY_FSM_IO_TX_GET_PACKET			(1 << 10)
>> +#define SPEEDY_FSM_IO_TX_PACKET				(1 << 11)
>> +#define SPEEDY_FSM_IO_TX_DONE				(1 << 12)
>> +
>> +#define SPEEDY_RX_LENGTH(n)				((n) << 0)
>> +#define SPEEDY_TX_LENGTH(n)				((n) << 8)
>> +
>> +#define SPEEDY_DEVICE(x)				((x & 0xf) << 15)
>> +#define SPEEDY_ADDRESS(x)				((x & 0xff) << 7)
>> +
>> +static const struct of_device_id speedy_match[] = {
>> +	{ .compatible = "samsung,exynos9810-speedy" },
>> +	{ /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, speedy_match);
> This is never at top of the file, but immediately before driver
> structure. Look at other drivers.
The function speedy_get_device uses this to match the compatible, do I 
just leave the prototype here?
>
>> +
>> +static const struct regmap_config speedy_map_cfg = {
>> +	.reg_bits = 32,
>> +	.val_bits = 32,
>> +};
>> +
>> +/**
>> + * speedy_int_clear() - clear interrupt status
>> + * @speedy:	pointer to speedy controller struct
>> + *
>> + * Return: 0 on success, -errno otherwise
>> + */
>> +static int speedy_int_clear(struct speedy_controller *speedy)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_set_bits(speedy->map, SPEEDY_INT_STATUS, 0xffffffff);
>> +	if (ret)
>> +		return ret;
>> +
>> +	udelay(10);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * speedy_fifo_reset() - reset FIFO of the controller
>> + * @speedy:	pointer to speedy controller struct
>> + *
>> + * Return: 0 on success, -errno otherwise
>> + */
>> +static int speedy_fifo_reset(struct speedy_controller *speedy)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL, SPEEDY_FIFO_RESET);
>> +	if (ret)
>> +		return ret;
>> +
>> +	udelay(10);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * _speedy_read() - internal speedy read operation
>> + * @speedy:	pointer to speedy controller struct
>> + * @reg:	address of device on the bus
>> + * @addr:       address to read
>> + * @val:        pointer to store result
>> + *
>> + * Return: 0 on success, -errno otherwise
>> + */
>> +static int _speedy_read(struct speedy_controller *speedy, u32 reg, u32 addr, u32 *val)
>> +{
>> +	int ret;
>> +	u32 cmd, int_ctl, int_status;
>> +
>> +	mutex_lock(&speedy->io_lock);
>> +
>> +	ret = speedy_fifo_reset(speedy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
>> +			      SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
>> +	if (ret)
>> +		return ret;
>> +
>> +	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
>> +	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
>> +
>> +	int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
>> +		  SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
>> +		  SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
>> +		  SPEEDY_REMOTE_RESET_REQ_EN;
>> +
>> +	ret = speedy_int_clear(speedy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for xfer done */
>> +	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
>> +				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = speedy_int_clear(speedy);
>> +
>> +	mutex_unlock(&speedy->io_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +int exynos_speedy_read(const struct speedy_device *device, u32 addr, u32 *val)
>> +{
>> +	return _speedy_read(device->speedy, device->reg, addr, val);
>> +}
>> +EXPORT_SYMBOL_GPL(exynos_speedy_read);
> Nope, drop, unused.
This is intended to be used with other device drivers, this driver in 
itself doesn't do anything, it only configures the controller and makes 
it ready for transmitting data, it's other drivers that will come (e.g. 
S2MPS18 PMIC, which uses SPEEDY for communication with the SoC) that 
will utilize those functions.
>
>> +
>> +/**
>> + * _speedy_write() - internal speedy write operation
>> + * @speedy:	pointer to speedy controller struct
>> + * @reg:	address of device on the bus
>> + * @addr:       address to write
>> + * @val:        value to write
>> + *
>> + * Return: 0 on success, -errno otherwise
>> + */
>> +static int _speedy_write(struct speedy_controller *speedy, u32 reg, u32 addr, u32 val)
>> +{
>> +	int ret;
>> +	u32 cmd, int_ctl, int_status;
>> +
>> +	mutex_lock(&speedy->io_lock);
>> +
>> +	ret = speedy_fifo_reset(speedy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
>> +			      SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
>> +	if (ret)
>> +		return ret;
>> +
>> +	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_WRITE |
>> +	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
>> +
>> +	int_ctl = (SPEEDY_TRANSFER_DONE_EN |
>> +		   SPEEDY_FIFO_TX_ALMOST_EMPTY_EN |
>> +		   SPEEDY_TX_LINE_BUSY_ERR_EN |
>> +		   SPEEDY_TX_STOPBIT_ERR_EN |
>> +		   SPEEDY_REMOTE_RESET_REQ_EN);
>> +
>> +	ret = speedy_int_clear(speedy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(speedy->map, SPEEDY_TX_DATA, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for xfer done */
>> +	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
>> +				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	speedy_int_clear(speedy);
>> +
>> +	mutex_unlock(&speedy->io_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +int exynos_speedy_write(const struct speedy_device *device, u32 addr, u32 val)
>> +{
>> +	return _speedy_write(device->speedy, device->reg, addr, val);
> Just write the function here and drop _.
>
>> +}
>> +EXPORT_SYMBOL_GPL(exynos_speedy_write);
>> +
>> +static void devm_speedy_release(struct device *dev, void *res)
>> +{
>> +	const struct speedy_device **ptr = res;
>> +	const struct speedy_device *handle = *ptr;
>> +
>> +	kfree(handle);
>> +}
>> +
>> +/**
>> + * speedy_get_by_phandle() - internal get speedy device handle
>> + * @np:	pointer to OF device node of device
>> + *
>> + * Return: 0 on success, -errno otherwise
>> + */
>> +static const struct speedy_device *speedy_get_device(struct device_node *np)
>> +{
>> +	const struct of_device_id *speedy_id;
>> +	struct device_node *speedy_np;
>> +	struct platform_device *speedy_pdev;
>> +	struct speedy_controller *speedy = NULL;
>> +	struct speedy_device *handle;
>> +	int ret;
>> +
>> +	if (!np) {
>> +		pr_err("I need a device pointer\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	speedy_np = of_get_parent(np);
>> +	if (!speedy_np)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	/* Verify if parent node is a speedy controller */
>> +	speedy_id = of_match_node(speedy_match, speedy_np);
>> +	if (!speedy_id) {
>> +		handle = ERR_PTR(-EINVAL);
>> +		goto out;
>> +	}
>> +
>> +	/* Get platform device of the speedy controller */
>> +	speedy_pdev = of_find_device_by_node(speedy_np);
>> +	if (!speedy_pdev) {
>> +		handle = ERR_PTR(-EPROBE_DEFER);
>> +		goto out;
>> +	}
>> +
>> +	/* Get drvdata of speedy controller */
>> +	speedy = platform_get_drvdata(speedy_pdev);
>> +	if (!speedy) {
>> +		handle = ERR_PTR(-EINVAL);
>> +		goto out;
>> +	}
>> +
>> +	handle = kzalloc(sizeof(struct speedy_device), GFP_KERNEL);
>> +	if (!handle) {
>> +		handle = ERR_PTR(-ENOMEM);
>> +		goto out;
>> +	}
>> +	handle->speedy = speedy;
>> +	ret = of_property_read_u32(np, "reg", &handle->reg);
>> +	if (ret) {
>> +		kfree(handle);
>> +		handle = ERR_PTR(-EINVAL);
>> +		goto out;
>> +	}
>> +
>> +out:
>> +	of_node_put(speedy_np);
>> +	return handle;
>> +}
>> +
>> +const struct speedy_device *devm_speedy_get_device(struct device *dev)
> Not used, drop function.
>
>> +{
>> +	const struct speedy_device *handle;
>> +	const struct speedy_device **ptr;
>> +
>> +	ptr = devres_alloc(devm_speedy_release, sizeof(*ptr), GFP_KERNEL);
>> +	if (!ptr)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	handle = speedy_get_device(dev_of_node(dev));
>> +	if (!IS_ERR(handle)) {
>> +		*ptr = handle;
>> +		devres_add(dev, ptr);
>> +	} else {
>> +		devres_free(ptr);
>> +	}
>> +
>> +	return handle;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_speedy_get_device);
> Not used, drop.
>
>
>
>> +
>> +static int speedy_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct speedy_controller *speedy;
>> +	void __iomem *mem;
>> +	int ret;
>> +
>> +	speedy = devm_kzalloc(dev, sizeof(struct speedy_controller), GFP_KERNEL);
> sizeof(*) everywhere. You need to clean the downstream code when
> upstreaming.
Oh, okay, sorry, missed it.
>
>> +	if (!speedy)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, speedy);
>> +	speedy->pdev = pdev;
>> +
>> +	mutex_init(&speedy->io_lock);
>> +
>> +	mem = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(mem))
>> +		return dev_err_probe(dev, PTR_ERR(mem), "Failed to ioremap memory\n");
>> +
>> +	speedy->map = devm_regmap_init_mmio(dev, mem, &speedy_map_cfg);
>> +	if (IS_ERR(speedy->map))
>> +		return dev_err_probe(dev, PTR_ERR(speedy->map), "Failed to init the regmap\n");
> Wrap at 80.
>
>> +
>> +	/* Clear any interrupt status remaining */
>> +	ret = speedy_int_clear(speedy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Reset the controller */
>> +	ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_SW_RST);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msleep(20);
>> +
>> +	/* Enable the hw */
>> +	ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_ENABLE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msleep(20);
>> +
>> +	/* Probe child devices */
>> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
>> +	if (ret)
>> +		dev_err(dev, "Failed to populate child devices: %d\n", ret);
>> +
>> +	return ret;
> Missing cleanup in remove().
Will be done.
>
>> +}
>> +
>> +static struct platform_driver speedy_driver = {
>> +	.probe = speedy_probe,
>> +	.driver = {
>> +		.name = "exynos-speedy",
>> +		.of_match_table = speedy_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(speedy_driver);
>> +
>> +MODULE_DESCRIPTION("Samsung Exynos SPEEDY host controller driver");
>> +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/soc/samsung/exynos-speedy.h b/include/linux/soc/samsung/exynos-speedy.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..b2857d65d3b50927373866dd8ae1c47e98af6d7b
>> --- /dev/null
>> +++ b/include/linux/soc/samsung/exynos-speedy.h
> Drop the header, not used.
Same here, please clarify how this should be handled. This driver 
implements the devm_speedy_get_device and read/write functions for its 
child devices in that header, future drivers for e.g. PMIC would use 
this header and call devm_speedy_get_device to get a speedy_device 
pointer and then use read/write functions to read/write from the bus.
>
>
>
> Best regards,
> Krzysztof

- Markuss


[1] https://patents.google.com/patent/US9882711B1/en

Re: [PATCH 1/3] dt-bindings: soc: samsung: exynos-speedy: Document SPEEDY host controller bindings

[Markuss Broks]

Hi Krzysztof,

On 12/13/24 9:40 AM, Krzysztof Kozlowski wrote:
> On 12/12/2024 22:09, Markuss Broks wrote:
>> Add the schema for the Samsung SPEEDY serial bus host controller.
>> The bus has 4 bit wide addresses for addressing devices
>> and 8 bit wide register addressing. Each register is also 8
>> bit long, so the address can be 0-f (hexadecimal), node name
>> for child device follows the format: node_name@[0-f].
>
> This wasn't tested so limited review.
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
>> Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
>> Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>> ---
>>   .../bindings/soc/samsung/exynos-speedy.yaml        | 78 ++++++++++++++++++++++
> Filename must match compatible.
>
>>   1 file changed, 78 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..304b322a74ea70f23d8c072b44b6ca86b7cc807f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/samsung/exynos-speedy.yaml
>> @@ -0,0 +1,78 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/samsung/exynos-speedy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Samsung Exynos SPEEDY serial bus host controller
> Speedy or SPEEDY?
Technically it's an acronym (Serial Protocol in an EffEctive Digital 
waY), but we could agree on if we use the capitalized or uncapitalised 
version and use it consistently throughout.
>
>> +
>> +maintainers:
>> +  - Markuss Broks <markuss.broks@gmail.com>
>> +
>> +description:
>> +  Samsung SPEEDY is a proprietary Samsung serial 1-wire bus.
> 1-wire? But not compatible with w1 (onwire)?
Nope, I suppose this requires more clarification, as explained in the 
previous letter, there are several differences between the protocols, 
looking at the Samsung patent. [1]
>
>> +  It is used on various Samsung Exynos chips. The bus can
>> +  address at most 4 bit (16) devices. The devices on the bus
>> +  have 8 bit long register line, and the registers are also
>> +  8 bit long each. It is typically used for communicating with
>> +  Samsung PMICs (s2mps17, s2mps18, ...) and other Samsung chips,
>> +  such as RF parts.
>> +
>> +properties:
>> +  compatible:
>> +    - items:
>> +        - enum:
>> +            - samsung,exynos9810-speedy
>> +        - const: samsung,exynos-speedy
> Drop last compatible and use only SoC specific.
Makes sense, for some reason I didn't realise it doesn't make much sense.
>
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    - const: pclk
> Drop clock-names, not needed for one entry.
>
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#address-cells"
>> +  - "#size-cells"
> You do not have them in the properties, anyway required goes before
> additionalProperties
>
>> +
>> +patternProperties:
>> +  "^[a-z][a-z0-9]*@[0-9a-f]$":
> That's odd regex. Look at other bus bindings.
Okay, I'll look into it.
>
>> +    type: object
>> +    additionalProperties: true
>> +
>> +    properties:
>> +      reg:
>> +        maxItems: 1
> maximum: 15
>
>> +
>> +    required:
>> +      - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    speedy0: speedy@141c0000 {
> Drop unused label.
>
>> +      compatible = "samsung,exynos9810-speedy",
>> +                   "samsung-exynos-speedy";
>> +      reg = <0x141c0000 0x2000>;
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
> No resources? No clocks? No interrupts?
Will extend the example.
>
>
>
> Best regards,
> Krzysztof

- Markuss


[1] https://patents.google.com/patent/US9882711B1/en

Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Krzysztof Kozlowski]

On 13/12/2024 10:42, Markuss Broks wrote:
> Hi Krzysztof,
> 
> On 12/13/24 9:49 AM, Krzysztof Kozlowski wrote:
>> On 12/12/2024 22:09, Markuss Broks wrote:
>>> Add a driver for Samsung SPEEDY serial bus host controller.
>>> SPEEDY is a proprietary 1 wire serial bus used by Samsung
>>> in various devices (usually mobile), like Samsung Galaxy
>>> phones. It is usually used for connecting PMIC or various
>>> other peripherals, like audio codecs or RF components.
>>>
>>> This bus can address at most 1MiB (4 bit device address,
>>> 8 bit registers per device, 8 bit wide registers:
>>> 256*256*16 = 1MiB of address space.
>>>
>>> Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
>>> Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
>>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>>> ---
>>>   drivers/soc/samsung/Kconfig               |  13 +
>>>   drivers/soc/samsung/Makefile              |   2 +
>>>   drivers/soc/samsung/exynos-speedy.c       | 457 ++++++++++++++++++++++++++++++
>>>   include/linux/soc/samsung/exynos-speedy.h |  56 ++++
>>>   4 files changed, 528 insertions(+)
>>>
>>> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
>>> index 1a5dfdc978dc4069eb71c4e8eada7ff1913b86b3..a38150fc9999ded1e1e93e2a9ef43b88175d34bd 100644
>>> --- a/drivers/soc/samsung/Kconfig
>>> +++ b/drivers/soc/samsung/Kconfig
>>> @@ -49,6 +49,19 @@ config EXYNOS_PMU_ARM_DRIVERS
>>>   	bool "Exynos PMU ARMv7-specific driver extensions" if COMPILE_TEST
>>>   	depends on EXYNOS_PMU
>>>   
>>> +config EXYNOS_SPEEDY
>>> +	tristate "Exynos SPEEDY host controller driver"
>>> +	depends on ARCH_EXYNOS || COMPILE_TEST
>>> +	depends on OF
>>> +	depends on REGMAP_MMIO
>>> +	help
>>> +	  Enable support for Exynos SPEEDY host controller block.
>>> +	  SPEEDY is a 1 wire proprietary Samsung serial bus, found in
>>> +	  modern Samsung Exynos SoCs, like Exynos8895 and newer.
>>
>> I did not check that much but this looks like 1wire for which we have
>> subsystem. Please investigate more and figure out the differences.
> 
> This is not compatible with Dallas Semi 1-Wire bus. There are several 
> differences but the phy level is not compatible, looking at the Samsung 
> patent. [1] The most obvious difference is that 1-Wire is discoverable, 
> and this bus isn't. I'm pretty sure this is Samsung's own solution to a 
> serial interface through only one wire.
> 

It's fine then.

>>
>>> +
>>> +	  Select this if you have a Samsung Exynos device which uses
>>> +	  SPEEDY bus.
>>> +
>>
>>> +
>>> +/* SPEEDY_PACKET_GAP_TIME register bits */
>>> +#define SPEEDY_FIFO_TX_ALMOST_EMPTY			(1 << 4)
>>> +#define SPEEDY_FIFO_RX_ALMOST_FULL			(1 << 8)
>>> +#define SPEEDY_FSM_INIT					(1 << 1)
>>> +#define SPEEDY_FSM_TX_CMD				(1 << 2)
>>> +#define SPEEDY_FSM_STANDBY				(1 << 3)
>>> +#define SPEEDY_FSM_DATA					(1 << 4)
>>> +#define SPEEDY_FSM_TIMEOUT				(1 << 5)
>>> +#define SPEEDY_FSM_TRANS_DONE				(1 << 6)
>>> +#define SPEEDY_FSM_IO_RX_STAT_MASK			(3 << 7)
>>> +#define SPEEDY_FSM_IO_TX_IDLE				(1 << 9)
>>> +#define SPEEDY_FSM_IO_TX_GET_PACKET			(1 << 10)
>>> +#define SPEEDY_FSM_IO_TX_PACKET				(1 << 11)
>>> +#define SPEEDY_FSM_IO_TX_DONE				(1 << 12)
>>> +
>>> +#define SPEEDY_RX_LENGTH(n)				((n) << 0)
>>> +#define SPEEDY_TX_LENGTH(n)				((n) << 8)
>>> +
>>> +#define SPEEDY_DEVICE(x)				((x & 0xf) << 15)
>>> +#define SPEEDY_ADDRESS(x)				((x & 0xff) << 7)
>>> +
>>> +static const struct of_device_id speedy_match[] = {
>>> +	{ .compatible = "samsung,exynos9810-speedy" },
>>> +	{ /* Sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, speedy_match);
>> This is never at top of the file, but immediately before driver
>> structure. Look at other drivers.
> The function speedy_get_device uses this to match the compatible, do I 
> just leave the prototype here?


1. Entire speedy_get_device() is unused so it will be removed.
2. Even if it stays, speedy_get_device() is not supposed to match
anything. How are you supposed to use Samsung PMIC on different
controller? These things should not be tied.


>>
>>> +
>>> +static const struct regmap_config speedy_map_cfg = {
>>> +	.reg_bits = 32,
>>> +	.val_bits = 32,
>>> +};


...

>>> +	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
>>> +	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
>>> +
>>> +	int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
>>> +		  SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
>>> +		  SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
>>> +		  SPEEDY_REMOTE_RESET_REQ_EN;
>>> +
>>> +	ret = speedy_int_clear(speedy);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Wait for xfer done */
>>> +	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
>>> +				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = speedy_int_clear(speedy);
>>> +
>>> +	mutex_unlock(&speedy->io_lock);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +int exynos_speedy_read(const struct speedy_device *device, u32 addr, u32 *val)
>>> +{
>>> +	return _speedy_read(device->speedy, device->reg, addr, val);
>>> +}
>>> +EXPORT_SYMBOL_GPL(exynos_speedy_read);
>> Nope, drop, unused.
> This is intended to be used with other device drivers, this driver in 
> itself doesn't do anything, it only configures the controller and makes 
> it ready for transmitting data, it's other drivers that will come (e.g. 
> S2MPS18 PMIC, which uses SPEEDY for communication with the SoC) that 
> will utilize those functions.

Post entire series, so we see users of this API. If you post API without
users, it won't be accepted simply because there are no users and we do
not want dead, unused code.

Anyway we must see how you intend to use that interface to properly
review it.

>>
>>> +
>>> +/**
>>> + * _speedy_write() - internal speedy write operation
>>> + * @speedy:	pointer to speedy controller struct
>>> + * @reg:	address of device on the bus
>>> + * @addr:       address to write
>>> + * @val:        value to write
>>> + *


...

>>> +}
>>> +
>>> +static struct platform_driver speedy_driver = {
>>> +	.probe = speedy_probe,
>>> +	.driver = {
>>> +		.name = "exynos-speedy",
>>> +		.of_match_table = speedy_match,
>>> +	},
>>> +};
>>> +
>>> +module_platform_driver(speedy_driver);
>>> +
>>> +MODULE_DESCRIPTION("Samsung Exynos SPEEDY host controller driver");
>>> +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/soc/samsung/exynos-speedy.h b/include/linux/soc/samsung/exynos-speedy.h
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..b2857d65d3b50927373866dd8ae1c47e98af6d7b
>>> --- /dev/null
>>> +++ b/include/linux/soc/samsung/exynos-speedy.h
>> Drop the header, not used.
> Same here, please clarify how this should be handled. This driver 
> implements the devm_speedy_get_device and read/write functions for its 
> child devices in that header, future drivers for e.g. PMIC would use 
> this header and call devm_speedy_get_device to get a speedy_device 
> pointer and then use read/write functions to read/write from the bus.

This needs to be proper bus with proper speedy_driver clients. See how
other buses - struct bus_type. Recent example of bus using platform
drivers for devices would be pwrseq (power/sequence). Not so old other
bus using its own xxx_driver for devices could be cxl or ffa (arm_ffa).
This current non-bus approach, could also work if this is really Samsung
specific. See memory/ for examples of MMIO buses and MMIO clients.

I don't know good examples of non-MMIO buses not using bus_type and I am
not sure whether this is accepted in general. I'll ask on IRC, maybe
someone will give some hints.


Best regards,
Krzysztof

Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Markuss Broks]

Hi Krzysztof,

On 12/13/24 3:55 PM, Krzysztof Kozlowski wrote:
> On 13/12/2024 10:42, Markuss Broks wrote:
>> Hi Krzysztof,
>>
>> On 12/13/24 9:49 AM, Krzysztof Kozlowski wrote:
>>> On 12/12/2024 22:09, Markuss Broks wrote:
>>>> Add a driver for Samsung SPEEDY serial bus host controller.
>>>> SPEEDY is a proprietary 1 wire serial bus used by Samsung
>>>> in various devices (usually mobile), like Samsung Galaxy
>>>> phones. It is usually used for connecting PMIC or various
>>>> other peripherals, like audio codecs or RF components.
>>>>
>>>> This bus can address at most 1MiB (4 bit device address,
>>>> 8 bit registers per device, 8 bit wide registers:
>>>> 256*256*16 = 1MiB of address space.
>>>>
>>>> Co-developed-by: Maksym Holovach <nergzd@nergzd723.xyz>
>>>> Signed-off-by: Maksym Holovach <nergzd@nergzd723.xyz>
>>>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>>>> ---
>>>>    drivers/soc/samsung/Kconfig               |  13 +
>>>>    drivers/soc/samsung/Makefile              |   2 +
>>>>    drivers/soc/samsung/exynos-speedy.c       | 457 ++++++++++++++++++++++++++++++
>>>>    include/linux/soc/samsung/exynos-speedy.h |  56 ++++
>>>>    4 files changed, 528 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
>>>> index 1a5dfdc978dc4069eb71c4e8eada7ff1913b86b3..a38150fc9999ded1e1e93e2a9ef43b88175d34bd 100644
>>>> --- a/drivers/soc/samsung/Kconfig
>>>> +++ b/drivers/soc/samsung/Kconfig
>>>> @@ -49,6 +49,19 @@ config EXYNOS_PMU_ARM_DRIVERS
>>>>    	bool "Exynos PMU ARMv7-specific driver extensions" if COMPILE_TEST
>>>>    	depends on EXYNOS_PMU
>>>>    
>>>> +config EXYNOS_SPEEDY
>>>> +	tristate "Exynos SPEEDY host controller driver"
>>>> +	depends on ARCH_EXYNOS || COMPILE_TEST
>>>> +	depends on OF
>>>> +	depends on REGMAP_MMIO
>>>> +	help
>>>> +	  Enable support for Exynos SPEEDY host controller block.
>>>> +	  SPEEDY is a 1 wire proprietary Samsung serial bus, found in
>>>> +	  modern Samsung Exynos SoCs, like Exynos8895 and newer.
>>> I did not check that much but this looks like 1wire for which we have
>>> subsystem. Please investigate more and figure out the differences.
>> This is not compatible with Dallas Semi 1-Wire bus. There are several
>> differences but the phy level is not compatible, looking at the Samsung
>> patent. [1] The most obvious difference is that 1-Wire is discoverable,
>> and this bus isn't. I'm pretty sure this is Samsung's own solution to a
>> serial interface through only one wire.
>>
> It's fine then.
>
>>>> +
>>>> +	  Select this if you have a Samsung Exynos device which uses
>>>> +	  SPEEDY bus.
>>>> +
>>>> +
>>>> +/* SPEEDY_PACKET_GAP_TIME register bits */
>>>> +#define SPEEDY_FIFO_TX_ALMOST_EMPTY			(1 << 4)
>>>> +#define SPEEDY_FIFO_RX_ALMOST_FULL			(1 << 8)
>>>> +#define SPEEDY_FSM_INIT					(1 << 1)
>>>> +#define SPEEDY_FSM_TX_CMD				(1 << 2)
>>>> +#define SPEEDY_FSM_STANDBY				(1 << 3)
>>>> +#define SPEEDY_FSM_DATA					(1 << 4)
>>>> +#define SPEEDY_FSM_TIMEOUT				(1 << 5)
>>>> +#define SPEEDY_FSM_TRANS_DONE				(1 << 6)
>>>> +#define SPEEDY_FSM_IO_RX_STAT_MASK			(3 << 7)
>>>> +#define SPEEDY_FSM_IO_TX_IDLE				(1 << 9)
>>>> +#define SPEEDY_FSM_IO_TX_GET_PACKET			(1 << 10)
>>>> +#define SPEEDY_FSM_IO_TX_PACKET				(1 << 11)
>>>> +#define SPEEDY_FSM_IO_TX_DONE				(1 << 12)
>>>> +
>>>> +#define SPEEDY_RX_LENGTH(n)				((n) << 0)
>>>> +#define SPEEDY_TX_LENGTH(n)				((n) << 8)
>>>> +
>>>> +#define SPEEDY_DEVICE(x)				((x & 0xf) << 15)
>>>> +#define SPEEDY_ADDRESS(x)				((x & 0xff) << 7)
>>>> +
>>>> +static const struct of_device_id speedy_match[] = {
>>>> +	{ .compatible = "samsung,exynos9810-speedy" },
>>>> +	{ /* Sentinel */ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, speedy_match);
>>> This is never at top of the file, but immediately before driver
>>> structure. Look at other drivers.
>> The function speedy_get_device uses this to match the compatible, do I
>> just leave the prototype here?
>
> 1. Entire speedy_get_device() is unused so it will be removed.
> 2. Even if it stays, speedy_get_device() is not supposed to match
> anything. How are you supposed to use Samsung PMIC on different
> controller? These things should not be tied.


speedy_get_device was my approach to not make it a proper bus driver, 
since I've thought it would be overkill for the purposes. 
speedy_get_device() is supposed to be called by a child device through 
the devm_ helper, and what it does is: it gets the parent node, verifies 
if that node matches the speedy compatible, then if it does it gets the 
platform_device and crafts a new struct speedy_device for the child 
device to use. This approach is perhaps hacky, but it helps simplify 
things a bit by not implementing the whole bus logic properly, let me 
know if it's too hacky...

Actually, one of my other concerns is that S2MPS18 has both SPEEDY and 
I2C interface for communicating with host, and with current approach 
it's going to be hacky if we want to support both. I've not seen S2MPS18 
used with I2C on any real boards, but it should really be trivial to 
support both, and with current hacky approach it might be not trivial at 
all...

>
>
>>>> +
>>>> +static const struct regmap_config speedy_map_cfg = {
>>>> +	.reg_bits = 32,
>>>> +	.val_bits = 32,
>>>> +};
>
> ...
>
>>>> +	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
>>>> +	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
>>>> +
>>>> +	int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
>>>> +		  SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
>>>> +		  SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
>>>> +		  SPEEDY_REMOTE_RESET_REQ_EN;
>>>> +
>>>> +	ret = speedy_int_clear(speedy);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/* Wait for xfer done */
>>>> +	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
>>>> +				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = speedy_int_clear(speedy);
>>>> +
>>>> +	mutex_unlock(&speedy->io_lock);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +int exynos_speedy_read(const struct speedy_device *device, u32 addr, u32 *val)
>>>> +{
>>>> +	return _speedy_read(device->speedy, device->reg, addr, val);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(exynos_speedy_read);
>>> Nope, drop, unused.
>> This is intended to be used with other device drivers, this driver in
>> itself doesn't do anything, it only configures the controller and makes
>> it ready for transmitting data, it's other drivers that will come (e.g.
>> S2MPS18 PMIC, which uses SPEEDY for communication with the SoC) that
>> will utilize those functions.
> Post entire series, so we see users of this API. If you post API without
> users, it won't be accepted simply because there are no users and we do
> not want dead, unused code.
>
> Anyway we must see how you intend to use that interface to properly
> review it.


Sure!

>
>>>> +
>>>> +/**
>>>> + * _speedy_write() - internal speedy write operation
>>>> + * @speedy:	pointer to speedy controller struct
>>>> + * @reg:	address of device on the bus
>>>> + * @addr:       address to write
>>>> + * @val:        value to write
>>>> + *
>
> ...
>
>>>> +}
>>>> +
>>>> +static struct platform_driver speedy_driver = {
>>>> +	.probe = speedy_probe,
>>>> +	.driver = {
>>>> +		.name = "exynos-speedy",
>>>> +		.of_match_table = speedy_match,
>>>> +	},
>>>> +};
>>>> +
>>>> +module_platform_driver(speedy_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("Samsung Exynos SPEEDY host controller driver");
>>>> +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
>>>> +MODULE_LICENSE("GPL");
>>>> diff --git a/include/linux/soc/samsung/exynos-speedy.h b/include/linux/soc/samsung/exynos-speedy.h
>>>> new file mode 100644
>>>> index 0000000000000000000000000000000000000000..b2857d65d3b50927373866dd8ae1c47e98af6d7b
>>>> --- /dev/null
>>>> +++ b/include/linux/soc/samsung/exynos-speedy.h
>>> Drop the header, not used.
>> Same here, please clarify how this should be handled. This driver
>> implements the devm_speedy_get_device and read/write functions for its
>> child devices in that header, future drivers for e.g. PMIC would use
>> this header and call devm_speedy_get_device to get a speedy_device
>> pointer and then use read/write functions to read/write from the bus.
> This needs to be proper bus with proper speedy_driver clients. See how
> other buses - struct bus_type. Recent example of bus using platform
> drivers for devices would be pwrseq (power/sequence). Not so old other
> bus using its own xxx_driver for devices could be cxl or ffa (arm_ffa).
> This current non-bus approach, could also work if this is really Samsung
> specific. See memory/ for examples of MMIO buses and MMIO clients.


Let me know if the current approach is acceptable or not, it is fine in 
my eyes if it needs to be a full-fledged bus driver, but in my opinion 
it's a lot of effort and additional logic for a niche samsung-specific 
usecase. Although I've heard that they're still using SPEEDY in newer 
devices as well, perhaps for more advanced purposes than just an 
interface for PMIC, which would mean additional things to consider...

I'll be glad to hear how you think it should be implemented, because I 
myself do not know a lot about kernel drivers like this...

>
> I don't know good examples of non-MMIO buses not using bus_type and I am
> not sure whether this is accepted in general. I'll ask on IRC, maybe
> someone will give some hints.
>
>
> Best regards,
> Krzysztof

Thanks,

- Markuss

Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Markus Elfring]

…
> SPEEDY is a proprietary 1 wire serial bus used by Samsung
> in various devices …

You may occasionally put more than 57 characters into text lines
of such a change description.


…
> +++ b/drivers/soc/samsung/exynos-speedy.c
> @@ -0,0 +1,457 @@
…
> +static int _speedy_read(struct speedy_controller *speedy, u32 reg, u32 addr, u32 *val)
> +{
> +	int ret;
> +	u32 cmd, int_ctl, int_status;
> +
> +	mutex_lock(&speedy->io_lock);
…
> +	ret = speedy_int_clear(speedy);
> +
> +	mutex_unlock(&speedy->io_lock);
> +
> +	return ret;
> +}
…

Under which circumstances would you become interested to apply a statement
like “guard(mutex)(&speedy->io_lock);”?
https://elixir.bootlin.com/linux/v6.13-rc2/source/include/linux/mutex.h#L201

Regards,
Markus

Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Christophe JAILLET]

Le 12/12/2024 à 22:09, Markuss Broks a écrit :
> Add a driver for Samsung SPEEDY serial bus host controller.
> SPEEDY is a proprietary 1 wire serial bus used by Samsung
> in various devices (usually mobile), like Samsung Galaxy
> phones. It is usually used for connecting PMIC or various
> other peripherals, like audio codecs or RF components.
> 
> This bus can address at most 1MiB (4 bit device address,
> 8 bit registers per device, 8 bit wide registers:
> 256*256*16 = 1MiB of address space.

...

> +static int _speedy_read(struct speedy_controller *speedy, u32 reg, u32 addr, u32 *val)
> +{
> +	int ret;
> +	u32 cmd, int_ctl, int_status;
> +
> +	mutex_lock(&speedy->io_lock);

All error handling paths fail to release the mutex.
guard(mutex) would help here.

> +
> +	ret = speedy_fifo_reset(speedy);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
> +			      SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
> +	if (ret)
> +		return ret;
> +
> +	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
> +	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
> +
> +	int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
> +		  SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
> +		  SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
> +		  SPEEDY_REMOTE_RESET_REQ_EN;
> +
> +	ret = speedy_int_clear(speedy);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for xfer done */
> +	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
> +				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = speedy_int_clear(speedy);
> +
> +	mutex_unlock(&speedy->io_lock);
> +
> +	return ret;
> +}

...

> +static int _speedy_write(struct speedy_controller *speedy, u32 reg, u32 addr, u32 val)
> +{
> +	int ret;
> +	u32 cmd, int_ctl, int_status;
> +
> +	mutex_lock(&speedy->io_lock);
> +
> +	ret = speedy_fifo_reset(speedy);
> +	if (ret)
> +		return ret;

All error handling paths fail to release the mutex.
guard(mutex) would help here.

> +
> +	ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
> +			      SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
> +	if (ret)
> +		return ret;
> +
> +	cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_WRITE |
> +	      SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
> +
> +	int_ctl = (SPEEDY_TRANSFER_DONE_EN |
> +		   SPEEDY_FIFO_TX_ALMOST_EMPTY_EN |
> +		   SPEEDY_TX_LINE_BUSY_ERR_EN |
> +		   SPEEDY_TX_STOPBIT_ERR_EN |
> +		   SPEEDY_REMOTE_RESET_REQ_EN);
> +
> +	ret = speedy_int_clear(speedy);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(speedy->map, SPEEDY_TX_DATA, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for xfer done */
> +	ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
> +				       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
> +	if (ret)
> +		return ret;
> +
> +	speedy_int_clear(speedy);
> +
> +	mutex_unlock(&speedy->io_lock);
> +
> +	return 0;
> +}

...

> +/**
> + * speedy_get_by_phandle() - internal get speedy device handle
> + * @np:	pointer to OF device node of device
> + *
> + * Return: 0 on success, -errno otherwise

On success, a handle is returned, not 0.

> + */
> +static const struct speedy_device *speedy_get_device(struct device_node *np)
> +{
...

> +out:
> +	of_node_put(speedy_np);
> +	return handle;
> +}

...

> +static int speedy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct speedy_controller *speedy;
> +	void __iomem *mem;
> +	int ret;
> +
> +	speedy = devm_kzalloc(dev, sizeof(struct speedy_controller), GFP_KERNEL);
> +	if (!speedy)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, speedy);
> +	speedy->pdev = pdev;
> +
> +	mutex_init(&speedy->io_lock);
> +
> +	mem = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mem))
> +		return dev_err_probe(dev, PTR_ERR(mem), "Failed to ioremap memory\n");
> +
> +	speedy->map = devm_regmap_init_mmio(dev, mem, &speedy_map_cfg);
> +	if (IS_ERR(speedy->map))
> +		return dev_err_probe(dev, PTR_ERR(speedy->map), "Failed to init the regmap\n");
> +
> +	/* Clear any interrupt status remaining */
> +	ret = speedy_int_clear(speedy);
> +	if (ret)
> +		return ret;
> +
> +	/* Reset the controller */
> +	ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_SW_RST);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	/* Enable the hw */
> +	ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	/* Probe child devices */
> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
> +	if (ret)
> +		dev_err(dev, "Failed to populate child devices: %d\n", ret);

Could be dev_err_probe() as well, at least for consistency.

> +
> +	return ret;
> +}

...

CJ

Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Markuss Broks]

Hi Christophe,

On 12/14/24 5:52 PM, Christophe JAILLET wrote:
> Le 12/12/2024 à 22:09, Markuss Broks a écrit :
>> Add a driver for Samsung SPEEDY serial bus host controller.
>> SPEEDY is a proprietary 1 wire serial bus used by Samsung
>> in various devices (usually mobile), like Samsung Galaxy
>> phones. It is usually used for connecting PMIC or various
>> other peripherals, like audio codecs or RF components.
>>
>> This bus can address at most 1MiB (4 bit device address,
>> 8 bit registers per device, 8 bit wide registers:
>> 256*256*16 = 1MiB of address space.
>
> ...
>
>> +static int _speedy_read(struct speedy_controller *speedy, u32 reg, 
>> u32 addr, u32 *val)
>> +{
>> +    int ret;
>> +    u32 cmd, int_ctl, int_status;
>> +
>> +    mutex_lock(&speedy->io_lock);
>
> All error handling paths fail to release the mutex.
> guard(mutex) would help here.

True, I didn't know that such a thing existed, thanks for the tip! :)


>
>> +
>> +    ret = speedy_fifo_reset(speedy);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
>> +                  SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
>> +    if (ret)
>> +        return ret;
>> +
>> +    cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
>> +          SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
>> +
>> +    int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
>> +          SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
>> +          SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
>> +          SPEEDY_REMOTE_RESET_REQ_EN;
>> +
>> +    ret = speedy_int_clear(speedy);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Wait for xfer done */
>> +    ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, 
>> int_status,
>> +                       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = speedy_int_clear(speedy);
>> +
>> +    mutex_unlock(&speedy->io_lock);
>> +
>> +    return ret;
>> +}
>
> ...
>
>> +static int _speedy_write(struct speedy_controller *speedy, u32 reg, 
>> u32 addr, u32 val)
>> +{
>> +    int ret;
>> +    u32 cmd, int_ctl, int_status;
>> +
>> +    mutex_lock(&speedy->io_lock);
>> +
>> +    ret = speedy_fifo_reset(speedy);
>> +    if (ret)
>> +        return ret;
>
> All error handling paths fail to release the mutex.
> guard(mutex) would help here.
>
>> +
>> +    ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
>> +                  SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
>> +    if (ret)
>> +        return ret;
>> +
>> +    cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_WRITE |
>> +          SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
>> +
>> +    int_ctl = (SPEEDY_TRANSFER_DONE_EN |
>> +           SPEEDY_FIFO_TX_ALMOST_EMPTY_EN |
>> +           SPEEDY_TX_LINE_BUSY_ERR_EN |
>> +           SPEEDY_TX_STOPBIT_ERR_EN |
>> +           SPEEDY_REMOTE_RESET_REQ_EN);
>> +
>> +    ret = speedy_int_clear(speedy);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_write(speedy->map, SPEEDY_TX_DATA, val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Wait for xfer done */
>> +    ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, 
>> int_status,
>> +                       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
>> +    if (ret)
>> +        return ret;
>> +
>> +    speedy_int_clear(speedy);
>> +
>> +    mutex_unlock(&speedy->io_lock);
>> +
>> +    return 0;
>> +}
>
> ...
>
>> +/**
>> + * speedy_get_by_phandle() - internal get speedy device handle
>> + * @np:    pointer to OF device node of device
>> + *
>> + * Return: 0 on success, -errno otherwise
>
> On success, a handle is returned, not 0.
>
>> + */
>> +static const struct speedy_device *speedy_get_device(struct 
>> device_node *np)
>> +{
> ...
>
>> +out:
>> +    of_node_put(speedy_np);
>> +    return handle;
>> +}
>
> ...
>
>> +static int speedy_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct speedy_controller *speedy;
>> +    void __iomem *mem;
>> +    int ret;
>> +
>> +    speedy = devm_kzalloc(dev, sizeof(struct speedy_controller), 
>> GFP_KERNEL);
>> +    if (!speedy)
>> +        return -ENOMEM;
>> +
>> +    platform_set_drvdata(pdev, speedy);
>> +    speedy->pdev = pdev;
>> +
>> +    mutex_init(&speedy->io_lock);
>> +
>> +    mem = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(mem))
>> +        return dev_err_probe(dev, PTR_ERR(mem), "Failed to ioremap 
>> memory\n");
>> +
>> +    speedy->map = devm_regmap_init_mmio(dev, mem, &speedy_map_cfg);
>> +    if (IS_ERR(speedy->map))
>> +        return dev_err_probe(dev, PTR_ERR(speedy->map), "Failed to 
>> init the regmap\n");
>> +
>> +    /* Clear any interrupt status remaining */
>> +    ret = speedy_int_clear(speedy);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Reset the controller */
>> +    ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_SW_RST);
>> +    if (ret)
>> +        return ret;
>> +
>> +    msleep(20);
>> +
>> +    /* Enable the hw */
>> +    ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_ENABLE);
>> +    if (ret)
>> +        return ret;
>> +
>> +    msleep(20);
>> +
>> +    /* Probe child devices */
>> +    ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
>> +    if (ret)
>> +        dev_err(dev, "Failed to populate child devices: %d\n", ret);
>
> Could be dev_err_probe() as well, at least for consistency.


I agree, will fix in the next revision.


>
>> +
>> +    return ret;
>> +}
>
> ...
>
> CJ


- Markuss

Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Markuss Broks]

Hi Markus,

On 12/14/24 4:43 PM, Markus Elfring wrote:
> …
>> SPEEDY is a proprietary 1 wire serial bus used by Samsung
>> in various devices …
> You may occasionally put more than 57 characters into text lines
> of such a change description.

But does it really matter where I break the line? For me, it just seems 
ugly no matter where I do it...

>
>
> …
>> +++ b/drivers/soc/samsung/exynos-speedy.c
>> @@ -0,0 +1,457 @@
> …
>> +static int _speedy_read(struct speedy_controller *speedy, u32 reg, u32 addr, u32 *val)
>> +{
>> +	int ret;
>> +	u32 cmd, int_ctl, int_status;
>> +
>> +	mutex_lock(&speedy->io_lock);
> …
>> +	ret = speedy_int_clear(speedy);
>> +
>> +	mutex_unlock(&speedy->io_lock);
>> +
>> +	return ret;
>> +}
> …
>
> Under which circumstances would you become interested to apply a statement
> like “guard(mutex)(&speedy->io_lock);”?
> https://elixir.bootlin.com/linux/v6.13-rc2/source/include/linux/mutex.h#L201


I did not know such statement existed, thanks for the tip, it definitely 
helps and makes it simpler!


>
> Regards,
> Markus


- Markuss