[PATCH 0/8] soc: samsung: Add USIv2 driver

[PATCH 0/8] soc: samsung: Add USIv2 driver

[Sam Protsenko]

USIv2 IP-core provides selectable serial protocol (UART, SPI or
High-Speed I2C); only one can be chosen at a time. This series
implements USIv2 driver, which allows one to select particular USI
function in device tree, and also performs USI block initialization.

With that driver implemented, it's not needed to do USI initialization
in protocol drivers anymore, so that code is removed from the serial
driver.

Because USI driver is tristate (can be built as a module), serial driver
was reworked so it's possible to use its console part as a module too.
This way we can load serial driver module from user space and still have
serial console functional.

Make it impossible to build UART/SPI/I2C driver as a built-in when USIv2
driver built as a module: USIv2 configuration must be always done before
tinkering with particular protocol it implements.

Design features:
  - "reg" property contains USI registers start address (0xc0 offset);
    it's used in the driver to access USI_CON and USI_OPTION registers.
    This way all USI initialization (reset, HWACG, etc) can be done in
    USIv2 driver separately, rather than duplicating that code over
    UART/SPI/I2C drivers
  - System Register (system controller node) and its SW_CONF register
    offset are provided in "samsung,sysreg" property; it's used to
    select USI function (protocol to be used)
  - USI function is specified in "samsung,mode" property; integer value
    is used to simplify parsing
  - there is "samsung,clkreq-on" bool property, which makes driver
    disable HWACG control (needed for UART to work properly)
  - PCLK and IPCLK clocks are both provided to USI node; apparently both
    need to be enabled to access USI registers
  - protocol nodes are embedded (as a child nodes) in USI node; it
    allows correct init order, and reflects HW properly
  - USIv2 driver is a tristate: can be also useful from Android GKI
    requirements point of view
  - driver functions are implemented with further development in mind:
    we might want to add some SysFS interface later for example, or
    provide some functions to serial drivers with EXPORT_SYMBOL(), etc

Sam Protsenko (8):
  dt-bindings: soc: samsung: Add Exynos USIv2 bindings
  dt-bindings: soc: samsung: Add Exynos USIv2 bindings doc
  soc: samsung: Add USIv2 driver
  tty: serial: samsung: Remove USI initialization
  tty: serial: samsung: Enable console as module
  tty: serial: Make SERIAL_SAMSUNG=y impossible when EXYNOS_USI_V2=m
  i2c: Make I2C_EXYNOS5=y impossible when EXYNOS_USI_V2=m
  spi: Make SPI_S3C64XX=y impossible when EXYNOS_USI_V2=m

 .../bindings/soc/samsung/exynos-usi-v2.yaml   | 124 +++++++++
 drivers/i2c/busses/Kconfig                    |   1 +
 drivers/soc/samsung/Kconfig                   |  14 +
 drivers/soc/samsung/Makefile                  |   2 +
 drivers/soc/samsung/exynos-usi-v2.c           | 242 ++++++++++++++++++
 drivers/spi/Kconfig                           |   1 +
 drivers/tty/serial/Kconfig                    |   3 +-
 drivers/tty/serial/samsung_tty.c              |  57 ++---
 .../dt-bindings/soc/samsung,exynos-usi-v2.h   |  16 ++
 include/linux/serial_s3c.h                    |   9 -
 10 files changed, 425 insertions(+), 44 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/samsung/exynos-usi-v2.yaml
 create mode 100644 drivers/soc/samsung/exynos-usi-v2.c
 create mode 100644 include/dt-bindings/soc/samsung,exynos-usi-v2.h

-- 
2.30.2

[PATCH 1/8] dt-bindings: soc: samsung: Add Exynos USIv2 bindings

[Sam Protsenko]

Add constants for choosing USIv2 configuration mode in device tree.
Those are further used in USIv2 driver to figure out which value to
write into SW_CONF register.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 include/dt-bindings/soc/samsung,exynos-usi-v2.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 include/dt-bindings/soc/samsung,exynos-usi-v2.h

diff --git a/include/dt-bindings/soc/samsung,exynos-usi-v2.h b/include/dt-bindings/soc/samsung,exynos-usi-v2.h
new file mode 100644
index 000000000000..b406c6f6f89e
--- /dev/null
+++ b/include/dt-bindings/soc/samsung,exynos-usi-v2.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (C) 2021 Linaro Ltd.
+ * Author: Sam Protsenko <semen.protsenko@linaro.org>
+ *
+ * Device Tree bindings for Samsung Exynos USI v2 (Universal Serial Interface).
+ */
+
+#ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_V2_H
+#define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_V2_H
+
+#define USI_V2_UART		0
+#define USI_V2_SPI		1
+#define USI_V2_I2C		2
+
+#endif /* __DT_BINDINGS_SAMSUNG_EXYNOS_USI_V2_H */
-- 
2.30.2

[PATCH 2/8] dt-bindings: soc: samsung: Add Exynos USIv2 bindings doc

[Sam Protsenko]

Document USIv2 IP-core bindings.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 .../bindings/soc/samsung/exynos-usi-v2.yaml   | 124 ++++++++++++++++++
 1 file changed, 124 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/samsung/exynos-usi-v2.yaml

diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi-v2.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi-v2.yaml
new file mode 100644
index 000000000000..d7466aa463dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/samsung/exynos-usi-v2.yaml
@@ -0,0 +1,124 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/samsung/exynos-usi-v2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung's Exynos USIv2 (Universal Serial Interface) binding
+
+maintainers:
+  - Sam Protsenko <semen.protsenko@linaro.org>
+  - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
+
+description: |
+  USIv2 IP-core provides selectable serial protocol (UART, SPI or High-Speed
+  I2C); only one can be chosen at a time. It is modeled as a node with zero or
+  more child nodes, each representing a serial sub-node device. The mode setting
+  selects which particular function will be used.
+
+  Refer to next bindings documentation for information on protocol subnodes that
+  can exist under USI node:
+
+  [1] Documentation/devicetree/bindings/serial/samsung_uart.yaml
+  [2] Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
+  [3] Documentation/devicetree/bindings/spi/spi-samsung.txt
+
+properties:
+  $nodename:
+    pattern: "^usi@[0-9a-f]+$"
+
+  compatible:
+    const: samsung,exynos-usi-v2
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Bus (APB) clock
+      - description: Operating clock for UART/SPI/I2C protocol
+
+  clock-names:
+    items:
+      - const: pclk
+      - const: ipclk
+
+  ranges: true
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+  samsung,sysreg:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Should be phandle/offset pair. The phandle to System Register syscon node
+      (for the same domain where this USIv2 controller resides) and the offset
+      of SW_CONF register for this USIv2 controller.
+
+  samsung,mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Selects USIv2 function (which serial protocol to use). Refer to
+      <include/dt-bindings/soc/samsung,exynos-usi-v2.h> for valid USI mode
+      values.
+
+  samsung,clkreq-on:
+    type: boolean
+    description:
+      Enable this property if underlying protocol requires the clock to be
+      continuously provided without automatic gating. As suggested by SoC
+      manual, it should be set in case of SPI/I2C slave, UART Rx and I2C
+      multi-master mode. Usually this property is needed if USI mode is set
+      to "UART".
+
+      This property is optional.
+
+patternProperties:
+  # All other properties should be child nodes
+  "^.*@[0-9a-f]+$":
+    type: object
+    description: Child node describing underlying USIv2 serial protocol
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - ranges
+  - "#address-cells"
+  - "#size-cells"
+  - samsung,sysreg
+  - samsung,mode
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/soc/samsung,exynos-usi-v2.h>
+
+    usi_uart: usi@138200c0 {
+        compatible = "samsung,exynos-usi-v2";
+        reg = <0x138200c0 0x20>;
+        samsung,sysreg = <&sysreg_peri 0x1010>;
+        samsung,mode = <USI_V2_UART>;
+        samsung,clkreq-on; /* needed for UART mode */
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+        clocks = <&cmu_peri 32>, <&cmu_peri 31>;
+        clock-names = "pclk", "ipclk";
+        status = "disabled";
+
+        serial_0: serial@13820000 {
+            compatible = "samsung,exynos850-uart";
+            reg = <0x13820000 0xc0>;
+            interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&cmu_peri 32>, <&cmu_peri 31>;
+            clock-names = "uart", "clk_uart_baud0";
+            status = "disabled";
+        };
+    };
-- 
2.30.2

Re: [PATCH 2/8] dt-bindings: soc: samsung: Add Exynos USIv2 bindings doc

[Krzysztof Kozlowski]

On 27/11/2021 23:32, Sam Protsenko wrote:
> Document USIv2 IP-core bindings.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  .../bindings/soc/samsung/exynos-usi-v2.yaml   | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/samsung/exynos-usi-v2.yaml
> 

I propose to squash it with patch #1.

Rest looks good to me.


Best regards,
Krzysztof

[PATCH 3/8] soc: samsung: Add USIv2 driver

[Sam Protsenko]

USIv2 IP-core is found on modern ARM64 Exynos SoCs (like Exynos850) and
provides selectable serial protocol (one of: UART, SPI, I2C). USIv2
registers usually reside in the same register map as a particular
underlying protocol it implements, but have some particular offset. E.g.
on Exynos850 the USI_UART has 0x13820000 base address, where UART
registers have 0x00..0x40 offsets, and USI registers have 0xc0..0xdc
offsets. Desired protocol can be chosen via SW_CONF register from System
Register block of the same domain as USI.

Before starting to use a particular protocol, USIv2 must be configured
properly:
  1. Select protocol to be used via System Register
  2. Clear "reset" flag in USI_CON
  3. Configure HWACG behavior (e.g. for UART Rx the HWACG must be
     disabled, so that the IP clock is not gated automatically); this is
     done using USI_OPTION register
  4. Keep both USI clocks (PCLK and IPCLK) running during USI registers
     modification

This driver implements above behavior. Of course, USIv2 driver should be
probed before UART/I2C/SPI drivers. It can be achived by embedding
UART/I2C/SPI nodes inside of USI node (in Device Tree); driver then
walks underlying nodes and instantiates those. Driver also handles USI
configuration on PM resume, as register contents can be lost during CPU
suspend.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/soc/samsung/Kconfig         |  14 ++
 drivers/soc/samsung/Makefile        |   2 +
 drivers/soc/samsung/exynos-usi-v2.c | 242 ++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/soc/samsung/exynos-usi-v2.c

diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
index e2cedef1e8d1..b168973c887f 100644
--- a/drivers/soc/samsung/Kconfig
+++ b/drivers/soc/samsung/Kconfig
@@ -23,6 +23,20 @@ config EXYNOS_CHIPID
 	  Support for Samsung Exynos SoC ChipID and Adaptive Supply Voltage.
 	  This driver can also be built as module (exynos_chipid).
 
+config EXYNOS_USI_V2
+	tristate "Exynos USIv2 (Universal Serial Interface) driver"
+	default ARCH_EXYNOS && ARM64
+	depends on ARCH_EXYNOS || COMPILE_TEST
+	select MFD_SYSCON
+	help
+	  Enable support for USIv2 block. USI (Universal Serial Interface) is an
+	  IP-core found in modern Samsung Exynos SoCs, like Exynos850 and
+	  ExynosAutoV0. USI block can be configured to provide one of the
+	  following serial protocols: UART, SPI or High Speed I2C.
+
+	  This driver allows one to configure USI for desired protocol, which
+	  is usually done in USI node in Device Tree.
+
 config EXYNOS_PMU
 	bool "Exynos PMU controller driver" if COMPILE_TEST
 	depends on ARCH_EXYNOS || ((ARM || ARM64) && COMPILE_TEST)
diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
index 2ae4bea804cf..0b746b2fd78f 100644
--- a/drivers/soc/samsung/Makefile
+++ b/drivers/soc/samsung/Makefile
@@ -4,6 +4,8 @@ obj-$(CONFIG_EXYNOS_ASV_ARM)	+= exynos5422-asv.o
 obj-$(CONFIG_EXYNOS_CHIPID)	+= exynos_chipid.o
 exynos_chipid-y			+= exynos-chipid.o exynos-asv.o
 
+obj-$(CONFIG_EXYNOS_USI_V2)	+= exynos-usi-v2.o
+
 obj-$(CONFIG_EXYNOS_PMU)	+= exynos-pmu.o
 
 obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS)	+= exynos3250-pmu.o exynos4-pmu.o \
diff --git a/drivers/soc/samsung/exynos-usi-v2.c b/drivers/soc/samsung/exynos-usi-v2.c
new file mode 100644
index 000000000000..5a315890e4ec
--- /dev/null
+++ b/drivers/soc/samsung/exynos-usi-v2.c
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 Linaro Ltd.
+ * Author: Sam Protsenko <semen.protsenko@linaro.org>
+ *
+ * Samsung Exynos USI v2 driver (Universal Serial Interface).
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#include <dt-bindings/soc/samsung,exynos-usi-v2.h>
+
+/* System Register: SW_CONF register bits */
+#define SW_CONF_UART		BIT(0)
+#define SW_CONF_SPI		BIT(1)
+#define SW_CONF_I2C		BIT(2)
+#define SW_CONF_MASK		(SW_CONF_UART | SW_CONF_SPI | SW_CONF_I2C)
+
+/* USI register offsets */
+#define USI_CON			0x04
+#define USI_OPTION		0x08
+
+/* USI register bits */
+#define USI_CON_RESET		BIT(0)
+#define USI_OPTION_CLKREQ_ON	BIT(1)
+#define USI_OPTION_CLKSTOP_ON	BIT(2)
+
+struct usi_v2_mode {
+	const char *name;		/* mode name */
+	unsigned int val;		/* mode register value */
+};
+
+struct usi_v2 {
+	struct device *dev;
+	void __iomem *regs;		/* USI register map */
+	struct clk *pclk;		/* USI bus clock */
+	struct clk *ipclk;		/* USI operating clock */
+
+	size_t mode;			/* current USI SW_CONF mode index */
+	bool clkreq_on;			/* always provide clock to IP */
+
+	/* System Register */
+	struct regmap *sysreg;		/* System Register map */
+	unsigned int sw_conf;		/* SW_CONF register offset in sysreg */
+};
+
+static const struct usi_v2_mode usi_v2_modes[] = {
+	[USI_V2_UART] =	{ .name = "uart", .val = SW_CONF_UART },
+	[USI_V2_SPI] =	{ .name = "spi",  .val = SW_CONF_SPI },
+	[USI_V2_I2C] =	{ .name = "i2c",  .val = SW_CONF_I2C },
+};
+
+/**
+ * usi_v2_set_sw_conf - Set USI block configuration mode
+ * @usi: USI driver object
+ * @mode: Mode index
+ *
+ * Select underlying serial protocol (UART/SPI/I2C) in USI IP-core.
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
+static int usi_v2_set_sw_conf(struct usi_v2 *usi, size_t mode)
+{
+	unsigned int val;
+	int ret;
+
+	if (mode >= ARRAY_SIZE(usi_v2_modes))
+		return -EINVAL;
+
+	val = usi_v2_modes[mode].val;
+	ret = regmap_update_bits(usi->sysreg, usi->sw_conf, SW_CONF_MASK, val);
+	if (ret)
+		return ret;
+
+	usi->mode = mode;
+	dev_dbg(usi->dev, "USIv2 protocol: %s\n", usi_v2_modes[usi->mode].name);
+
+	return 0;
+}
+
+/**
+ * usi_v2_enable - Initialize USI block
+ * @usi: USI driver object
+ *
+ * USI IP-core start state is "reset" (on startup and after CPU resume). This
+ * routine enables USI block by clearing the reset flag. It also configures
+ * HWACG behavior (needed e.g. for UART Rx). It should be performed before
+ * underlying protocol becomes functional.
+ *
+ * Both 'pclk' and 'ipclk' clocks should be enabled when running this function.
+ */
+static void usi_v2_enable(const struct usi_v2 *usi)
+{
+	u32 val;
+
+	/* Enable USI block */
+	val = readl(usi->regs + USI_CON);
+	val &= ~USI_CON_RESET;
+	writel(val, usi->regs + USI_CON);
+	udelay(1);
+
+	/* Continuously provide the clock to USI IP w/o gating */
+	if (usi->clkreq_on) {
+		val = readl(usi->regs + USI_OPTION);
+		val &= ~USI_OPTION_CLKSTOP_ON;
+		val |= USI_OPTION_CLKREQ_ON;
+		writel(val, usi->regs + USI_OPTION);
+	}
+}
+
+static int usi_v2_configure(struct usi_v2 *usi)
+{
+	int ret;
+
+	ret = clk_prepare_enable(usi->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(usi->ipclk);
+	if (ret)
+		goto err_pclk;
+
+	ret = usi_v2_set_sw_conf(usi, usi->mode);
+	if (ret)
+		goto err_ipclk;
+
+	usi_v2_enable(usi);
+
+err_ipclk:
+	clk_disable_unprepare(usi->ipclk);
+err_pclk:
+	clk_disable_unprepare(usi->pclk);
+	return ret;
+}
+
+static int usi_v2_parse_dt(struct device_node *np, struct usi_v2 *usi)
+{
+	int ret;
+	u32 mode;
+
+	ret = of_property_read_u32(np, "samsung,mode", &mode);
+	if (ret)
+		return ret;
+	usi->mode = mode;
+
+	usi->clkreq_on = of_property_read_bool(np, "samsung,clkreq-on");
+
+	usi->sysreg = syscon_regmap_lookup_by_phandle(np, "samsung,sysreg");
+	if (IS_ERR(usi->sysreg))
+		return PTR_ERR(usi->sysreg);
+
+	return of_property_read_u32_index(np, "samsung,sysreg", 1,
+					  &usi->sw_conf);
+}
+
+static int usi_v2_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct usi_v2 *usi;
+	int ret;
+
+	usi = devm_kzalloc(dev, sizeof(*usi), GFP_KERNEL);
+	if (!usi)
+		return -ENOMEM;
+
+	usi->dev = dev;
+	platform_set_drvdata(pdev, usi);
+
+	usi->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(usi->regs))
+		return PTR_ERR(usi->regs);
+
+	ret = usi_v2_parse_dt(np, usi);
+	if (ret)
+		return ret;
+
+	usi->pclk = devm_clk_get(dev, "pclk");
+	if (IS_ERR(usi->pclk))
+		return PTR_ERR(usi->pclk);
+
+	usi->ipclk = devm_clk_get(dev, "ipclk");
+	if (IS_ERR(usi->ipclk))
+		return PTR_ERR(usi->ipclk);
+
+	ret = usi_v2_configure(usi);
+	if (ret)
+		return ret;
+
+	/* Make it possible to embed protocol nodes into USI np */
+	return of_platform_populate(np, NULL, NULL, dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int usi_v2_resume_noirq(struct device *dev)
+{
+	struct usi_v2 *usi = dev_get_drvdata(dev);
+
+	return usi_v2_configure(usi);
+}
+#endif
+
+static const struct dev_pm_ops usi_v2_pm = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, usi_v2_resume_noirq)
+};
+
+static const struct of_device_id usi_v2_dt_match[] = {
+	{ .compatible = "samsung,exynos-usi-v2", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, usi_v2_dt_match);
+
+static struct platform_driver usi_v2_driver = {
+	.driver = {
+		.name		= "exynos-usi-v2",
+		.pm		= &usi_v2_pm,
+		.of_match_table	= usi_v2_dt_match,
+	},
+	.probe = usi_v2_probe,
+};
+
+static int __init usi_v2_init(void)
+{
+	return platform_driver_register(&usi_v2_driver);
+}
+arch_initcall(usi_v2_init);
+
+static void __exit usi_v2_exit(void)
+{
+	platform_driver_unregister(&usi_v2_driver);
+}
+module_exit(usi_v2_exit);
+
+MODULE_DESCRIPTION("Samsung USI v2 driver");
+MODULE_AUTHOR("Sam Protsenko <semen.protsenko@linaro.org>");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH 3/8] soc: samsung: Add USIv2 driver

[Krzysztof Kozlowski]

On 27/11/2021 23:32, Sam Protsenko wrote:
> USIv2 IP-core is found on modern ARM64 Exynos SoCs (like Exynos850) and
> provides selectable serial protocol (one of: UART, SPI, I2C). USIv2
> registers usually reside in the same register map as a particular
> underlying protocol it implements, but have some particular offset. E.g.
> on Exynos850 the USI_UART has 0x13820000 base address, where UART
> registers have 0x00..0x40 offsets, and USI registers have 0xc0..0xdc
> offsets. Desired protocol can be chosen via SW_CONF register from System
> Register block of the same domain as USI.
> 
> Before starting to use a particular protocol, USIv2 must be configured
> properly:
>   1. Select protocol to be used via System Register
>   2. Clear "reset" flag in USI_CON
>   3. Configure HWACG behavior (e.g. for UART Rx the HWACG must be
>      disabled, so that the IP clock is not gated automatically); this is
>      done using USI_OPTION register
>   4. Keep both USI clocks (PCLK and IPCLK) running during USI registers
>      modification
> 
> This driver implements above behavior. Of course, USIv2 driver should be
> probed before UART/I2C/SPI drivers. It can be achived by embedding
> UART/I2C/SPI nodes inside of USI node (in Device Tree); driver then
> walks underlying nodes and instantiates those. Driver also handles USI
> configuration on PM resume, as register contents can be lost during CPU
> suspend.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/soc/samsung/Kconfig         |  14 ++
>  drivers/soc/samsung/Makefile        |   2 +
>  drivers/soc/samsung/exynos-usi-v2.c | 242 ++++++++++++++++++++++++++++

You used everywhere v2 naming, but I actually hope this driver will be
able to support also v1 and vx of USI. IOW, I expect to have only one
USI driver, so please drop everywhere v2 (bindings, symbols, Kconfig,
functions) except the compatible.

>  3 files changed, 258 insertions(+)
>  create mode 100644 drivers/soc/samsung/exynos-usi-v2.c
> 
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index e2cedef1e8d1..b168973c887f 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -23,6 +23,20 @@ config EXYNOS_CHIPID
>  	  Support for Samsung Exynos SoC ChipID and Adaptive Supply Voltage.
>  	  This driver can also be built as module (exynos_chipid).
>  
> +config EXYNOS_USI_V2
> +	tristate "Exynos USIv2 (Universal Serial Interface) driver"
> +	default ARCH_EXYNOS && ARM64
> +	depends on ARCH_EXYNOS || COMPILE_TEST
> +	select MFD_SYSCON
> +	help
> +	  Enable support for USIv2 block. USI (Universal Serial Interface) is an
> +	  IP-core found in modern Samsung Exynos SoCs, like Exynos850 and
> +	  ExynosAutoV0. USI block can be configured to provide one of the
> +	  following serial protocols: UART, SPI or High Speed I2C.
> +
> +	  This driver allows one to configure USI for desired protocol, which
> +	  is usually done in USI node in Device Tree.
> +
>  config EXYNOS_PMU
>  	bool "Exynos PMU controller driver" if COMPILE_TEST
>  	depends on ARCH_EXYNOS || ((ARM || ARM64) && COMPILE_TEST)
> diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
> index 2ae4bea804cf..0b746b2fd78f 100644
> --- a/drivers/soc/samsung/Makefile
> +++ b/drivers/soc/samsung/Makefile
> @@ -4,6 +4,8 @@ obj-$(CONFIG_EXYNOS_ASV_ARM)	+= exynos5422-asv.o
>  obj-$(CONFIG_EXYNOS_CHIPID)	+= exynos_chipid.o
>  exynos_chipid-y			+= exynos-chipid.o exynos-asv.o
>  
> +obj-$(CONFIG_EXYNOS_USI_V2)	+= exynos-usi-v2.o
> +
>  obj-$(CONFIG_EXYNOS_PMU)	+= exynos-pmu.o
>  
>  obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS)	+= exynos3250-pmu.o exynos4-pmu.o \
> diff --git a/drivers/soc/samsung/exynos-usi-v2.c b/drivers/soc/samsung/exynos-usi-v2.c
> new file mode 100644
> index 000000000000..5a315890e4ec
> --- /dev/null
> +++ b/drivers/soc/samsung/exynos-usi-v2.c
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 Linaro Ltd.
> + * Author: Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * Samsung Exynos USI v2 driver (Universal Serial Interface).
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#include <dt-bindings/soc/samsung,exynos-usi-v2.h>
> +
> +/* System Register: SW_CONF register bits */
> +#define SW_CONF_UART		BIT(0)
> +#define SW_CONF_SPI		BIT(1)
> +#define SW_CONF_I2C		BIT(2)
> +#define SW_CONF_MASK		(SW_CONF_UART | SW_CONF_SPI | SW_CONF_I2C)
> +
> +/* USI register offsets */
> +#define USI_CON			0x04
> +#define USI_OPTION		0x08
> +
> +/* USI register bits */
> +#define USI_CON_RESET		BIT(0)
> +#define USI_OPTION_CLKREQ_ON	BIT(1)
> +#define USI_OPTION_CLKSTOP_ON	BIT(2)
> +
> +struct usi_v2_mode {

Everywhere here:
s/usi_v2/exynos_usi/

> +	const char *name;		/* mode name */
> +	unsigned int val;		/* mode register value */
> +};
> +
> +struct usi_v2 {
> +	struct device *dev;> +	void __iomem *regs;		/* USI register map */
> +	struct clk *pclk;		/* USI bus clock */
> +	struct clk *ipclk;		/* USI operating clock */
> +
> +	size_t mode;			/* current USI SW_CONF mode index */
> +	bool clkreq_on;			/* always provide clock to IP */
> +
> +	/* System Register */
> +	struct regmap *sysreg;		/* System Register map */
> +	unsigned int sw_conf;		/* SW_CONF register offset in sysreg */
> +};
> +
> +static const struct usi_v2_mode usi_v2_modes[] = {
> +	[USI_V2_UART] =	{ .name = "uart", .val = SW_CONF_UART },
> +	[USI_V2_SPI] =	{ .name = "spi",  .val = SW_CONF_SPI },
> +	[USI_V2_I2C] =	{ .name = "i2c",  .val = SW_CONF_I2C },
> +};
> +
> +/**
> + * usi_v2_set_sw_conf - Set USI block configuration mode
> + * @usi: USI driver object
> + * @mode: Mode index
> + *
> + * Select underlying serial protocol (UART/SPI/I2C) in USI IP-core.
> + *
> + * Return: 0 on success, or negative error code on failure.
> + */
> +static int usi_v2_set_sw_conf(struct usi_v2 *usi, size_t mode)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (mode >= ARRAY_SIZE(usi_v2_modes))
> +		return -EINVAL;
> +
> +	val = usi_v2_modes[mode].val;
> +	ret = regmap_update_bits(usi->sysreg, usi->sw_conf, SW_CONF_MASK, val);
> +	if (ret)
> +		return ret;
> +
> +	usi->mode = mode;
> +	dev_dbg(usi->dev, "USIv2 protocol: %s\n", usi_v2_modes[usi->mode].name);
> +
> +	return 0;
> +}
> +
> +/**
> + * usi_v2_enable - Initialize USI block
> + * @usi: USI driver object
> + *
> + * USI IP-core start state is "reset" (on startup and after CPU resume). This
> + * routine enables USI block by clearing the reset flag. It also configures
> + * HWACG behavior (needed e.g. for UART Rx). It should be performed before
> + * underlying protocol becomes functional.
> + *
> + * Both 'pclk' and 'ipclk' clocks should be enabled when running this function.
> + */
> +static void usi_v2_enable(const struct usi_v2 *usi)
> +{
> +	u32 val;
> +
> +	/* Enable USI block */
> +	val = readl(usi->regs + USI_CON);
> +	val &= ~USI_CON_RESET;
> +	writel(val, usi->regs + USI_CON);
> +	udelay(1);
> +
> +	/* Continuously provide the clock to USI IP w/o gating */
> +	if (usi->clkreq_on) {
> +		val = readl(usi->regs + USI_OPTION);
> +		val &= ~USI_OPTION_CLKSTOP_ON;
> +		val |= USI_OPTION_CLKREQ_ON;
> +		writel(val, usi->regs + USI_OPTION);
> +	}
> +}
> +
> +static int usi_v2_configure(struct usi_v2 *usi)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(usi->pclk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(usi->ipclk);
> +	if (ret)
> +		goto err_pclk;
> +
> +	ret = usi_v2_set_sw_conf(usi, usi->mode);
> +	if (ret)
> +		goto err_ipclk;
> +
> +	usi_v2_enable(usi);
> +
> +err_ipclk:
> +	clk_disable_unprepare(usi->ipclk);
> +err_pclk:
> +	clk_disable_unprepare(usi->pclk);
> +	return ret;
> +}
> +
> +static int usi_v2_parse_dt(struct device_node *np, struct usi_v2 *usi)
> +{
> +	int ret;
> +	u32 mode;
> +
> +	ret = of_property_read_u32(np, "samsung,mode", &mode);
> +	if (ret)
> +		return ret;
> +	usi->mode = mode;

Parse and validate mode here, instead of usi_v2_set_sw_conf(). We expect
DT to be correct, so if it is not, then there is no point to probe the
device.

Best regards,
Krzysztof

[PATCH 4/8] tty: serial: samsung: Remove USI initialization

[Sam Protsenko]

USI control is now extracted to dedicated USIv2 driver. Remove USI
related code from serial driver to avoid conflicts and code duplication.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 36 ++++----------------------------
 include/linux/serial_s3c.h       |  9 --------
 2 files changed, 4 insertions(+), 41 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index ca084c10d0bb..f986a9253dc8 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -65,7 +65,6 @@ enum s3c24xx_port_type {
 struct s3c24xx_uart_info {
 	char			*name;
 	enum s3c24xx_port_type	type;
-	bool			has_usi;
 	unsigned int		port_type;
 	unsigned int		fifosize;
 	unsigned long		rx_fifomask;
@@ -1357,28 +1356,6 @@ static int apple_s5l_serial_startup(struct uart_port *port)
 	return ret;
 }
 
-static void exynos_usi_init(struct uart_port *port)
-{
-	struct s3c24xx_uart_port *ourport = to_ourport(port);
-	struct s3c24xx_uart_info *info = ourport->info;
-	unsigned int val;
-
-	if (!info->has_usi)
-		return;
-
-	/* Clear the software reset of USI block (it's set at startup) */
-	val = rd_regl(port, USI_CON);
-	val &= ~USI_CON_RESET_MASK;
-	wr_regl(port, USI_CON, val);
-	udelay(1);
-
-	/* Continuously provide the clock to USI IP w/o gating (for Rx mode) */
-	val = rd_regl(port, USI_OPTION);
-	val &= ~USI_OPTION_HWACG_MASK;
-	val |= USI_OPTION_HWACG_CLKREQ_ON;
-	wr_regl(port, USI_OPTION, val);
-}
-
 /* power power management control */
 
 static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
@@ -1405,8 +1382,6 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
 
 		if (!IS_ERR(ourport->baudclk))
 			clk_prepare_enable(ourport->baudclk);
-
-		exynos_usi_init(port);
 		break;
 	default:
 		dev_err(port->dev, "s3c24xx_serial: unknown pm %d\n", level);
@@ -2130,8 +2105,6 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
 	if (ret)
 		pr_warn("uart: failed to enable baudclk\n");
 
-	exynos_usi_init(port);
-
 	/* Keep all interrupts masked and cleared */
 	switch (ourport->info->type) {
 	case TYPE_S3C6400:
@@ -2780,11 +2753,10 @@ static struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
 #endif
 
 #if defined(CONFIG_ARCH_EXYNOS)
-#define EXYNOS_COMMON_SERIAL_DRV_DATA(_has_usi)			\
+#define EXYNOS_COMMON_SERIAL_DRV_DATA()				\
 	.info = &(struct s3c24xx_uart_info) {			\
 		.name		= "Samsung Exynos UART",	\
 		.type		= TYPE_S3C6400,			\
-		.has_usi	= _has_usi,			\
 		.port_type	= PORT_S3C6400,			\
 		.has_divslot	= 1,				\
 		.rx_fifomask	= S5PV210_UFSTAT_RXMASK,	\
@@ -2805,17 +2777,17 @@ static struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
 	}							\
 
 static struct s3c24xx_serial_drv_data exynos4210_serial_drv_data = {
-	EXYNOS_COMMON_SERIAL_DRV_DATA(false),
+	EXYNOS_COMMON_SERIAL_DRV_DATA(),
 	.fifosize = { 256, 64, 16, 16 },
 };
 
 static struct s3c24xx_serial_drv_data exynos5433_serial_drv_data = {
-	EXYNOS_COMMON_SERIAL_DRV_DATA(false),
+	EXYNOS_COMMON_SERIAL_DRV_DATA(),
 	.fifosize = { 64, 256, 16, 256 },
 };
 
 static struct s3c24xx_serial_drv_data exynos850_serial_drv_data = {
-	EXYNOS_COMMON_SERIAL_DRV_DATA(true),
+	EXYNOS_COMMON_SERIAL_DRV_DATA(),
 	.fifosize = { 256, 64, 64, 64 },
 };
 
diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
index cf0de4a86640..f6c3323fc4c5 100644
--- a/include/linux/serial_s3c.h
+++ b/include/linux/serial_s3c.h
@@ -27,15 +27,6 @@
 #define S3C2410_UERSTAT	  (0x14)
 #define S3C2410_UFSTAT	  (0x18)
 #define S3C2410_UMSTAT	  (0x1C)
-#define USI_CON		  (0xC4)
-#define USI_OPTION	  (0xC8)
-
-#define USI_CON_RESET			(1<<0)
-#define USI_CON_RESET_MASK		(1<<0)
-
-#define USI_OPTION_HWACG_CLKREQ_ON	(1<<1)
-#define USI_OPTION_HWACG_CLKSTOP_ON	(1<<2)
-#define USI_OPTION_HWACG_MASK		(3<<1)
 
 #define S3C2410_LCON_CFGMASK	  ((0xF<<3)|(0x3))
 
-- 
2.30.2

Re: [PATCH 4/8] tty: serial: samsung: Remove USI initialization

[Greg Kroah-Hartman]

On Sun, Nov 28, 2021 at 12:32:49AM +0200, Sam Protsenko wrote:
> USI control is now extracted to dedicated USIv2 driver. Remove USI
> related code from serial driver to avoid conflicts and code duplication.

What conflicts?

What duplication?  All you did here was delete code.

confused,

greg k-h

Re: [PATCH 4/8] tty: serial: samsung: Remove USI initialization

[Sam Protsenko]

On Sun, 28 Nov 2021 at 16:28, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Nov 28, 2021 at 12:32:49AM +0200, Sam Protsenko wrote:
> > USI control is now extracted to dedicated USIv2 driver. Remove USI
> > related code from serial driver to avoid conflicts and code duplication.
>
> What conflicts?
>

There might be possible conflicts when accessing the same USI register
from both serial driver and USIv2 driver. Also there will be conflicts
when trying to access the same I/O address space in those both
drivers.

> What duplication?  All you did here was delete code.
>

It's all explained in [PATCH 0/8], but long story short, I've added
USIv2 driver (in this series) which handles the code that's removed
from serial driver in this patch.

> confused,
>
> greg k-h

Re: [PATCH 4/8] tty: serial: samsung: Remove USI initialization

[Sam Protsenko]

On Sun, 28 Nov 2021 at 18:26, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> On Sun, 28 Nov 2021 at 16:28, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Nov 28, 2021 at 12:32:49AM +0200, Sam Protsenko wrote:
> > > USI control is now extracted to dedicated USIv2 driver. Remove USI
> > > related code from serial driver to avoid conflicts and code duplication.
> >
> > What conflicts?
> >
>
> There might be possible conflicts when accessing the same USI register
> from both serial driver and USIv2 driver. Also there will be conflicts
> when trying to access the same I/O address space in those both
> drivers.
>
> > What duplication?  All you did here was delete code.
> >
>
> It's all explained in [PATCH 0/8], but long story short, I've added
> USIv2 driver (in this series) which handles the code that's removed
> from serial driver in this patch.
>

In other words, this code is now present here: [1]. But of course
USIv2 driver must be applied first, and then this patch (removing the
same code from serial driver). That's why it's in the same series and
it's placed after USIv2 driver ([PATCH 3/8]).

[1] https://patchwork.kernel.org/project/linux-samsung-soc/patch/20211127223253.19098-4-semen.protsenko@linaro.org/

> > confused,
> >
> > greg k-h

[PATCH 5/8] tty: serial: samsung: Enable console as module

[Sam Protsenko]

Enable serial driver to be built as a module. To do so, init the console
support on driver/module load instead of using console_initcall().

This is needed for proper support of USIv2 driver (which can be built as
a module, which in turn makes SERIAL_SAMSUNG be a module too). It also
might be useful for Android GKI modularization efforts.

Inspired by commit 87a0b9f98ac5 ("tty: serial: meson: enable console as
module").

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/tty/serial/Kconfig       |  2 +-
 drivers/tty/serial/samsung_tty.c | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index fc543ac97c13..0e5ccb25bdb1 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -263,7 +263,7 @@ config SERIAL_SAMSUNG_UARTS
 
 config SERIAL_SAMSUNG_CONSOLE
 	bool "Support for console on Samsung SoC serial port"
-	depends on SERIAL_SAMSUNG=y
+	depends on SERIAL_SAMSUNG
 	select SERIAL_CORE_CONSOLE
 	select SERIAL_EARLYCON
 	help
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index f986a9253dc8..92a63e9392ed 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1720,10 +1720,10 @@ static int __init s3c24xx_serial_console_init(void)
 	register_console(&s3c24xx_serial_console);
 	return 0;
 }
-console_initcall(s3c24xx_serial_console_init);
 
 #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
 #else
+static inline int s3c24xx_serial_console_init(void) { return 0; }
 #define S3C24XX_SERIAL_CONSOLE NULL
 #endif
 
@@ -2898,7 +2898,24 @@ static struct platform_driver samsung_serial_driver = {
 	},
 };
 
-module_platform_driver(samsung_serial_driver);
+static int __init samsung_serial_init(void)
+{
+	int ret;
+
+	ret = s3c24xx_serial_console_init();
+	if (ret)
+		return ret;
+
+	return platform_driver_register(&samsung_serial_driver);
+}
+
+static void __exit samsung_serial_exit(void)
+{
+	platform_driver_unregister(&samsung_serial_driver);
+}
+
+module_init(samsung_serial_init);
+module_exit(samsung_serial_exit);
 
 #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
 /*
-- 
2.30.2

Re: [PATCH 5/8] tty: serial: samsung: Enable console as module

[Krzysztof Kozlowski]

On 27/11/2021 23:32, Sam Protsenko wrote:
> Enable serial driver to be built as a module. To do so, init the console
> support on driver/module load instead of using console_initcall().
> 
> This is needed for proper support of USIv2 driver (which can be built as
> a module, which in turn makes SERIAL_SAMSUNG be a module too). It also
> might be useful for Android GKI modularization efforts.
> 
> Inspired by commit 87a0b9f98ac5 ("tty: serial: meson: enable console as
> module").
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/tty/serial/Kconfig       |  2 +-
>  drivers/tty/serial/samsung_tty.c | 21 +++++++++++++++++++--
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index fc543ac97c13..0e5ccb25bdb1 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -263,7 +263,7 @@ config SERIAL_SAMSUNG_UARTS
>  
>  config SERIAL_SAMSUNG_CONSOLE
>  	bool "Support for console on Samsung SoC serial port"
> -	depends on SERIAL_SAMSUNG=y
> +	depends on SERIAL_SAMSUNG
>  	select SERIAL_CORE_CONSOLE
>  	select SERIAL_EARLYCON
>  	help
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index f986a9253dc8..92a63e9392ed 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1720,10 +1720,10 @@ static int __init s3c24xx_serial_console_init(void)
>  	register_console(&s3c24xx_serial_console);
>  	return 0;
>  }
> -console_initcall(s3c24xx_serial_console_init);
>  
>  #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
>  #else
> +static inline int s3c24xx_serial_console_init(void) { return 0; }
>  #define S3C24XX_SERIAL_CONSOLE NULL
>  #endif
>  
> @@ -2898,7 +2898,24 @@ static struct platform_driver samsung_serial_driver = {
>  	},
>  };
>  
> -module_platform_driver(samsung_serial_driver);
> +static int __init samsung_serial_init(void)
> +{
> +	int ret;
> +
> +	ret = s3c24xx_serial_console_init();
> +	if (ret)
> +		return ret;

This will trigger warns on module re-loading, won't it? Either suppress
unbind or cleanup in module exit.

> +
> +	return platform_driver_register(&samsung_serial_driver);
> +}
> +
> +static void __exit samsung_serial_exit(void)
> +{
> +	platform_driver_unregister(&samsung_serial_driver);
> +}
> +
> +module_init(samsung_serial_init);
> +module_exit(samsung_serial_exit);
>  
>  #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
>  /*
> 


Best regards,
Krzysztof

Re: [PATCH 5/8] tty: serial: samsung: Enable console as module

[Sam Protsenko]

On Mon, 29 Nov 2021 at 10:52, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 27/11/2021 23:32, Sam Protsenko wrote:
> > Enable serial driver to be built as a module. To do so, init the console
> > support on driver/module load instead of using console_initcall().
> >
> > This is needed for proper support of USIv2 driver (which can be built as
> > a module, which in turn makes SERIAL_SAMSUNG be a module too). It also
> > might be useful for Android GKI modularization efforts.
> >
> > Inspired by commit 87a0b9f98ac5 ("tty: serial: meson: enable console as
> > module").
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/tty/serial/Kconfig       |  2 +-
> >  drivers/tty/serial/samsung_tty.c | 21 +++++++++++++++++++--
> >  2 files changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index fc543ac97c13..0e5ccb25bdb1 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -263,7 +263,7 @@ config SERIAL_SAMSUNG_UARTS
> >
> >  config SERIAL_SAMSUNG_CONSOLE
> >       bool "Support for console on Samsung SoC serial port"
> > -     depends on SERIAL_SAMSUNG=y
> > +     depends on SERIAL_SAMSUNG
> >       select SERIAL_CORE_CONSOLE
> >       select SERIAL_EARLYCON
> >       help
> > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > index f986a9253dc8..92a63e9392ed 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -1720,10 +1720,10 @@ static int __init s3c24xx_serial_console_init(void)
> >       register_console(&s3c24xx_serial_console);
> >       return 0;
> >  }
> > -console_initcall(s3c24xx_serial_console_init);
> >
> >  #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
> >  #else
> > +static inline int s3c24xx_serial_console_init(void) { return 0; }
> >  #define S3C24XX_SERIAL_CONSOLE NULL
> >  #endif
> >
> > @@ -2898,7 +2898,24 @@ static struct platform_driver samsung_serial_driver = {
> >       },
> >  };
> >
> > -module_platform_driver(samsung_serial_driver);
> > +static int __init samsung_serial_init(void)
> > +{
> > +     int ret;
> > +
> > +     ret = s3c24xx_serial_console_init();
> > +     if (ret)
> > +             return ret;
>
> This will trigger warns on module re-loading, won't it? Either suppress
> unbind or cleanup in module exit.
>

I guess that's already taken care of in  samsung_serial_remove(): it's
doing uart_remove_one_port(), which in turn does unregister_console().
So I don't think anything extra should be done on module exit. Or I'm
missing something?

That case (unload/load) actually doesn't work well in my case: serial
console doesn't work after doing "modprobe -r samsung_tty; modprobe
samsung_tty" (but it works fine e.g. in case of i2c_exynos5 driver).
Not sure what is wrong, but I can see that my board keeps running
(heartbeat LED is still blinking). Not even sure if that use case
(unload/load) was ever functional before.

Anyway, please let me know if you think something should be done about
this particular patch. Right now I don't see anything missing.

> > +
> > +     return platform_driver_register(&samsung_serial_driver);
> > +}
> > +
> > +static void __exit samsung_serial_exit(void)
> > +{
> > +     platform_driver_unregister(&samsung_serial_driver);
> > +}
> > +
> > +module_init(samsung_serial_init);
> > +module_exit(samsung_serial_exit);
> >
> >  #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
> >  /*
> >
>
>
> Best regards,
> Krzysztof

Re: [PATCH 5/8] tty: serial: samsung: Enable console as module

[Sam Protsenko]

On Mon, 29 Nov 2021 at 22:18, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> On Mon, 29 Nov 2021 at 10:52, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
> >
> > On 27/11/2021 23:32, Sam Protsenko wrote:
> > > Enable serial driver to be built as a module. To do so, init the console
> > > support on driver/module load instead of using console_initcall().
> > >
> > > This is needed for proper support of USIv2 driver (which can be built as
> > > a module, which in turn makes SERIAL_SAMSUNG be a module too). It also
> > > might be useful for Android GKI modularization efforts.
> > >
> > > Inspired by commit 87a0b9f98ac5 ("tty: serial: meson: enable console as
> > > module").
> > >
> > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > > ---
> > >  drivers/tty/serial/Kconfig       |  2 +-
> > >  drivers/tty/serial/samsung_tty.c | 21 +++++++++++++++++++--
> > >  2 files changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > > index fc543ac97c13..0e5ccb25bdb1 100644
> > > --- a/drivers/tty/serial/Kconfig
> > > +++ b/drivers/tty/serial/Kconfig
> > > @@ -263,7 +263,7 @@ config SERIAL_SAMSUNG_UARTS
> > >
> > >  config SERIAL_SAMSUNG_CONSOLE
> > >       bool "Support for console on Samsung SoC serial port"
> > > -     depends on SERIAL_SAMSUNG=y
> > > +     depends on SERIAL_SAMSUNG
> > >       select SERIAL_CORE_CONSOLE
> > >       select SERIAL_EARLYCON
> > >       help
> > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > > index f986a9253dc8..92a63e9392ed 100644
> > > --- a/drivers/tty/serial/samsung_tty.c
> > > +++ b/drivers/tty/serial/samsung_tty.c
> > > @@ -1720,10 +1720,10 @@ static int __init s3c24xx_serial_console_init(void)
> > >       register_console(&s3c24xx_serial_console);
> > >       return 0;
> > >  }
> > > -console_initcall(s3c24xx_serial_console_init);
> > >
> > >  #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
> > >  #else
> > > +static inline int s3c24xx_serial_console_init(void) { return 0; }
> > >  #define S3C24XX_SERIAL_CONSOLE NULL
> > >  #endif
> > >
> > > @@ -2898,7 +2898,24 @@ static struct platform_driver samsung_serial_driver = {
> > >       },
> > >  };
> > >
> > > -module_platform_driver(samsung_serial_driver);
> > > +static int __init samsung_serial_init(void)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = s3c24xx_serial_console_init();
> > > +     if (ret)
> > > +             return ret;
> >
> > This will trigger warns on module re-loading, won't it? Either suppress
> > unbind or cleanup in module exit.
> >
>
> I guess that's already taken care of in  samsung_serial_remove(): it's
> doing uart_remove_one_port(), which in turn does unregister_console().
> So I don't think anything extra should be done on module exit. Or I'm
> missing something?
>
> That case (unload/load) actually doesn't work well in my case: serial
> console doesn't work after doing "modprobe -r samsung_tty; modprobe
> samsung_tty" (but it works fine e.g. in case of i2c_exynos5 driver).
> Not sure what is wrong, but I can see that my board keeps running
> (heartbeat LED is still blinking). Not even sure if that use case
> (unload/load) was ever functional before.
>
> Anyway, please let me know if you think something should be done about
> this particular patch. Right now I don't see anything missing.
>

...But I'll actually add proper error path handling in
samsung_serial_init(), i.e. unregister console if
platform_driver_register() fails. And I'll add the same console
unregister in samsung_serial_exit(), just in case.

> > > +
> > > +     return platform_driver_register(&samsung_serial_driver);
> > > +}
> > > +
> > > +static void __exit samsung_serial_exit(void)
> > > +{
> > > +     platform_driver_unregister(&samsung_serial_driver);
> > > +}
> > > +
> > > +module_init(samsung_serial_init);
> > > +module_exit(samsung_serial_exit);
> > >
> > >  #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
> > >  /*
> > >
> >
> >
> > Best regards,
> > Krzysztof

[PATCH 6/8] tty: serial: Make SERIAL_SAMSUNG=y impossible when EXYNOS_USI_V2=m

[Sam Protsenko]

When UART is encapsulated in USIv2 block (e.g. in Exynos850), USIv2
driver must be loaded first, as it's preparing USI hardware for
particular protocol use. Make it impossible for Samsung serial driver to
be built-in when USIv2 driver is built as a module, to prevent incorrect
booting order for those drivers.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/tty/serial/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 0e5ccb25bdb1..47bc24e74041 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -237,6 +237,7 @@ config SERIAL_CLPS711X_CONSOLE
 config SERIAL_SAMSUNG
 	tristate "Samsung SoC serial support"
 	depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || ARCH_APPLE || COMPILE_TEST
+	depends on EXYNOS_USI_V2 || !EXYNOS_USI_V2
 	select SERIAL_CORE
 	help
 	  Support for the on-chip UARTs on the Samsung
-- 
2.30.2

Re: [PATCH 6/8] tty: serial: Make SERIAL_SAMSUNG=y impossible when EXYNOS_USI_V2=m

[Greg Kroah-Hartman]

On Sun, Nov 28, 2021 at 12:32:51AM +0200, Sam Protsenko wrote:
> When UART is encapsulated in USIv2 block (e.g. in Exynos850), USIv2
> driver must be loaded first, as it's preparing USI hardware for
> particular protocol use. Make it impossible for Samsung serial driver to
> be built-in when USIv2 driver is built as a module, to prevent incorrect
> booting order for those drivers.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/tty/serial/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 0e5ccb25bdb1..47bc24e74041 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -237,6 +237,7 @@ config SERIAL_CLPS711X_CONSOLE
>  config SERIAL_SAMSUNG
>  	tristate "Samsung SoC serial support"
>  	depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || ARCH_APPLE || COMPILE_TEST
> +	depends on EXYNOS_USI_V2 || !EXYNOS_USI_V2

That's odd, and is not going to help if everything is built as a module
and loaded that way.

This needs to be done properly in code to handle the issues if the
"wrong" code is loaded first.  Please trigger off of the hardware type
correctly so you don't have to worry about this at all.

thanks,

greg k-h

Re: [PATCH 6/8] tty: serial: Make SERIAL_SAMSUNG=y impossible when EXYNOS_USI_V2=m

[Sam Protsenko]

On Sun, 28 Nov 2021 at 16:27, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Nov 28, 2021 at 12:32:51AM +0200, Sam Protsenko wrote:
> > When UART is encapsulated in USIv2 block (e.g. in Exynos850), USIv2
> > driver must be loaded first, as it's preparing USI hardware for
> > particular protocol use. Make it impossible for Samsung serial driver to
> > be built-in when USIv2 driver is built as a module, to prevent incorrect
> > booting order for those drivers.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/tty/serial/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 0e5ccb25bdb1..47bc24e74041 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -237,6 +237,7 @@ config SERIAL_CLPS711X_CONSOLE
> >  config SERIAL_SAMSUNG
> >       tristate "Samsung SoC serial support"
> >       depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || ARCH_APPLE || COMPILE_TEST
> > +     depends on EXYNOS_USI_V2 || !EXYNOS_USI_V2
>
> That's odd, and is not going to help if everything is built as a module
> and loaded that way.
>
> This needs to be done properly in code to handle the issues if the
> "wrong" code is loaded first.  Please trigger off of the hardware type
> correctly so you don't have to worry about this at all.
>

You are right. The only thing that should be done is "__init" should
be removed from s3c24xx_serial_console_setup() and
s3c24xx_serial_get_options() functions. Because in case when USIv2
driver instantiates the serial driver via of_platform_populate(), when
USI_V2=m and SERIAL_SAMSUNG=y, those symbols will be thrown away
already. And of course "[PATCH 5/8] tty: serial: samsung: Enable
console as module" is needed as well. Correct init order (USI vs
serial) is already ensured by embedding serial node in USI node (as a
child node).

We'll still have some weird init order in that case (USI_V2=m and
SERIAL_SAMSUNG=y), like doing serial console init first (and
earlycon), then registering USI driver as a module (reconfiguring USI
IP-core), and then doing serial probe. But at least that doesn't crash
and works fine (only causing some delay once, in the middle of dmesg
output). But I guess that would be a problem of people who decided to
go with such weird config.

Bottom line is, this patch is not needed. I'll re-send v2 soon,
excluding it from there, and will also add that mentioned "__init"
removal.

Thanks for review!

> thanks,
>
> greg k-h

[PATCH 7/8] i2c: Make I2C_EXYNOS5=y impossible when EXYNOS_USI_V2=m

[Sam Protsenko]

When HSI2C is encapsulated in USIv2 block (e.g. in Exynos850), USIv2
driver must be loaded first, as it's preparing USI hardware for
particular protocol use. Make it impossible for i2c-exynos5 driver to be
built-in when USIv2 driver is built as a module, to prevent incorrect
booting order for those drivers.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/i2c/busses/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index df89cb809330..e815a9dffb2c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -613,6 +613,7 @@ config I2C_EXYNOS5
 	tristate "Exynos high-speed I2C driver"
 	depends on OF
 	depends on ARCH_EXYNOS || COMPILE_TEST
+	depends on EXYNOS_USI_V2 || !EXYNOS_USI_V2
 	default y if ARCH_EXYNOS
 	help
 	  High-speed I2C controller on Samsung Exynos5 and newer Samsung SoCs:
-- 
2.30.2

Re: [PATCH 7/8] i2c: Make I2C_EXYNOS5=y impossible when EXYNOS_USI_V2=m

[Sam Protsenko]

On Sun, 28 Nov 2021 at 00:33, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> When HSI2C is encapsulated in USIv2 block (e.g. in Exynos850), USIv2
> driver must be loaded first, as it's preparing USI hardware for
> particular protocol use. Make it impossible for i2c-exynos5 driver to be
> built-in when USIv2 driver is built as a module, to prevent incorrect
> booting order for those drivers.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---

This patch is not needed, please ignore it.

>  drivers/i2c/busses/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index df89cb809330..e815a9dffb2c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -613,6 +613,7 @@ config I2C_EXYNOS5
>         tristate "Exynos high-speed I2C driver"
>         depends on OF
>         depends on ARCH_EXYNOS || COMPILE_TEST
> +       depends on EXYNOS_USI_V2 || !EXYNOS_USI_V2
>         default y if ARCH_EXYNOS
>         help
>           High-speed I2C controller on Samsung Exynos5 and newer Samsung SoCs:
> --
> 2.30.2
>

[PATCH 8/8] spi: Make SPI_S3C64XX=y impossible when EXYNOS_USI_V2=m

[Sam Protsenko]

When S3C64XX SPI is encapsulated in USIv2 block (e.g. in Exynos850),
USIv2 driver must be loaded first, as it's preparing USI hardware for
particular protocol use. Make it impossible for spi-s3c64xx driver to be
built-in when USIv2 driver is built as a module, to prevent incorrect
booting order for those drivers.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/spi/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index b2a8821971e1..fbdf901248be 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -761,6 +761,7 @@ config SPI_S3C24XX_FIQ
 config SPI_S3C64XX
 	tristate "Samsung S3C64XX/Exynos SoC series type SPI"
 	depends on (PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST)
+	depends on EXYNOS_USI_V2 || !EXYNOS_USI_V2
 	help
 	  SPI driver for Samsung S3C64XX, S5Pv210 and Exynos SoCs.
 	  Choose Y/M here only if you build for such Samsung SoC.
-- 
2.30.2

Re: [PATCH 8/8] spi: Make SPI_S3C64XX=y impossible when EXYNOS_USI_V2=m

[Sam Protsenko]

On Sun, 28 Nov 2021 at 00:33, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> When S3C64XX SPI is encapsulated in USIv2 block (e.g. in Exynos850),
> USIv2 driver must be loaded first, as it's preparing USI hardware for
> particular protocol use. Make it impossible for spi-s3c64xx driver to be
> built-in when USIv2 driver is built as a module, to prevent incorrect
> booting order for those drivers.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---

This patch is not needed, please ignore it.

>  drivers/spi/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index b2a8821971e1..fbdf901248be 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -761,6 +761,7 @@ config SPI_S3C24XX_FIQ
>  config SPI_S3C64XX
>         tristate "Samsung S3C64XX/Exynos SoC series type SPI"
>         depends on (PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST)
> +       depends on EXYNOS_USI_V2 || !EXYNOS_USI_V2
>         help
>           SPI driver for Samsung S3C64XX, S5Pv210 and Exynos SoCs.
>           Choose Y/M here only if you build for such Samsung SoC.
> --
> 2.30.2
>

Re: [PATCH 0/8] soc: samsung: Add USIv2 driver

[David Virag]

On Sun, 2021-11-28 at 00:32 +0200, Sam Protsenko wrote:
> USIv2 IP-core provides selectable serial protocol (UART, SPI or
> High-Speed I2C); only one can be chosen at a time. This series
> implements USIv2 driver, which allows one to select particular USI
> function in device tree, and also performs USI block initialization.
> 
> With that driver implemented, it's not needed to do USI
> initialization
> in protocol drivers anymore, so that code is removed from the serial
> driver.
> 

I think the downstream way of doing this (USI node reg being on the
SW_CONF register itself rather than an offset from uart/i2c/spi, the
USI driver only controlling the SW_CONF, and the uart/i2c/spi drivers
controlling their USI_CON and USI_OPTION regs) is cleaner, better, and
easier to adapt to USIv1 too.

For example: I'm sure this is the case on USIv2 devices too, but on
Exynos7885, different devices have USI modes configured differently.
For example a Samsung Galaxy A8 (2018) has all the USI blocks
configured as SPI while a Samsung Galaxy M20 has the first USI
configured as dual HSI2C, the second as HSI2C on the first 2 pins and
the third as HSI2C on the last 2 pins. With this way of doing
everything on USIv2 we'd need 3 disabled USIv2 nodes in the SoC DTSI
for one USI block, each for every protocol the USI block can do, all
having a single child for their protocol and each referencing the same
sysreg (not even sure if that's even supported). Then the board DTS
could enable the USI node it needs.

With the downstream way we could have just one USI node and we could
add the 3 protocols it can do disabled as seperate or child nodes. This
way the board DTS only needs to set the appropriate mode setting and
enable the protocol it needs. I'd say much better than having 3 USI
nodes for the same USI block.

Also this way is pretty USIv2 centric. Adding USIv1 support to this
driver is difficult this way because of the the lack of USI_CON and
USI_OPTION registers as a whole (so having nowhere to actually set the
reg of the USI node to, as the only thing USIv1 has is the SW_CONF
register). In my opinion being able to use the same driver and same
device tree layout for USIv1 and USIv2 is a definite plus

The only real drawback of that way is having to add code for USIv2
inside the UART, HSI2C, and SPI drivers but in my opinion the benefits
overweigh the drawbacks greatly. We could even make the uart/spi/hsi2c
drivers call a helper function in the USI driver to set their USI_CON
and USI_OPTION registers up so that code would be shared and not
duplicated. Wether this patch gets applied like this is not my choice
though, I'll let the people responsible decide
:-)

Anyways, soon enough I can write an USIv1 driver after I submit all the
7885 stuff I'm working on currently. If you want to, you can add USIv2
support to that driver, or if an USIv2 driver is already in upstream at
that point, if it is written in the downstream way I can add v1 support
to that, or if it's like this I'll have to make a whole seperate driver
with a whole seperate DT structure.

Best regards,
David

Re: [PATCH 0/8] soc: samsung: Add USIv2 driver

[Krzysztof Kozlowski]

On 28/11/2021 04:15, David Virag wrote:
> On Sun, 2021-11-28 at 00:32 +0200, Sam Protsenko wrote:
>> USIv2 IP-core provides selectable serial protocol (UART, SPI or
>> High-Speed I2C); only one can be chosen at a time. This series
>> implements USIv2 driver, which allows one to select particular USI
>> function in device tree, and also performs USI block initialization.
>>
>> With that driver implemented, it's not needed to do USI
>> initialization
>> in protocol drivers anymore, so that code is removed from the serial
>> driver.
>>
> 
> I think the downstream way of doing this (USI node reg being on the
> SW_CONF register itself rather than an offset from uart/i2c/spi, the
> USI driver only controlling the SW_CONF, and the uart/i2c/spi drivers
> controlling their USI_CON and USI_OPTION regs) is cleaner, better, and
> easier to adapt to USIv1 too.
> 
> For example: I'm sure this is the case on USIv2 devices too, but on
> Exynos7885, different devices have USI modes configured differently.
> For example a Samsung Galaxy A8 (2018) has all the USI blocks
> configured as SPI while a Samsung Galaxy M20 has the first USI
> configured as dual HSI2C, the second as HSI2C on the first 2 pins and
> the third as HSI2C on the last 2 pins. With this way of doing
> everything on USIv2 we'd need 3 disabled USIv2 nodes in the SoC DTSI
> for one USI block, each for every protocol the USI block can do, all
> having a single child for their protocol and each referencing the same
> sysreg (not even sure if that's even supported). Then the board DTS
> could enable the USI node it needs.

It's not supported (one cannot have three same nodes with same unit
addresses), so this would be solved by dropping out unused interfaces,
commenting them out or storing everything under one USI:

usi@0x1abcdef0 {
  serial@.... {
    status = "okay";
  }

  i2c@.... {
    status = "disabled";
  }

  spi@.... {
    status = "disabled";
  }
}

> 
> With the downstream way we could have just one USI node and we could
> add the 3 protocols it can do disabled as seperate or child nodes. This
> way the board DTS only needs to set the appropriate mode setting and
> enable the protocol it needs. I'd say much better than having 3 USI
> nodes for the same USI block.

Then however you need to handle probe ordering and possible probe deferrals.

> 
> Also this way is pretty USIv2 centric. Adding USIv1 support to this
> driver is difficult this way because of the the lack of USI_CON and
> USI_OPTION registers as a whole (so having nowhere to actually set the
> reg of the USI node to, as the only thing USIv1 has is the SW_CONF
> register). 

How is it difficult? Not having a register is easy - noop on given platform.

> In my opinion being able to use the same driver and same
> device tree layout for USIv1 and USIv2 is a definite plus
> 
> The only real drawback of that way is having to add code for USIv2
> inside the UART, HSI2C, and SPI drivers but in my opinion the benefits
> overweigh the drawbacks greatly. We could even make the uart/spi/hsi2c
> drivers call a helper function in the USI driver to set their USI_CON
> and USI_OPTION registers up so that code would be shared and not
> duplicated. Wether this patch gets applied like this is not my choice
> though, I'll let the people responsible decide
> :-)
> 
> Anyways, soon enough I can write an USIv1 driver after I submit all the
> 7885 stuff I'm working on currently. If you want to, you can add USIv2
> support to that driver, or if an USIv2 driver is already in upstream at
> that point, if it is written in the downstream way I can add v1 support
> to that, or if it's like this I'll have to make a whole seperate driver
> with a whole seperate DT structure.
> 
> Best regards,
> David
> 


Best regards,
Krzysztof

Re: [PATCH 0/8] soc: samsung: Add USIv2 driver

[Sam Protsenko]

On Sun, 28 Nov 2021 at 05:15, David Virag <virag.david003@gmail.com> wrote:
>
> On Sun, 2021-11-28 at 00:32 +0200, Sam Protsenko wrote:
> > USIv2 IP-core provides selectable serial protocol (UART, SPI or
> > High-Speed I2C); only one can be chosen at a time. This series
> > implements USIv2 driver, which allows one to select particular USI
> > function in device tree, and also performs USI block initialization.
> >
> > With that driver implemented, it's not needed to do USI
> > initialization
> > in protocol drivers anymore, so that code is removed from the serial
> > driver.
> >
>
> I think the downstream way of doing this (USI node reg being on the
> SW_CONF register itself rather than an offset from uart/i2c/spi, the
> USI driver only controlling the SW_CONF, and the uart/i2c/spi drivers
> controlling their USI_CON and USI_OPTION regs) is cleaner, better, and
> easier to adapt to USIv1 too.
>

One reason why I think it's better to provide SW_CONF register via
syscon node, is that it helps us to avoid possible register access
conflicts in future, and also conflicts when requesting corresponding
resources. In other words, the System Register block can be used by
many consumers (drivers) in future; those consumers might try to
modify the same registers simultaneously, which might lead to race
conditions (as RMW operation is not atomic), so some kind of
serialization should be done (like locking in regmap), which is
provided by syscon. Also, that wouldn't even come to that: you just
can't request the same I/O area twice in Linux. So if SW_CONF is
passed via "reg" property to USI driver, and then we try to map the
whole System Register (or its portion that includes SW_CONF), that
request would fail.

Although passing one SW_CONF register via "reg" might look easier to
implement, it might also bring us all sort of problems later on. And I
think a good design should account for such pitfalls.

As for the USI registers: I really don't think that duplicating the
code for USI block reset across uart/i2c/spi drivers would help us to
accomplish anything. Why those drivers should be even aware of USI
reset? At least in USIv2 block, the USI registers and uart/i2c/spi
registers are not mixed: they are located at different and always
fixed addresses. We can benefit from that fact, and provide Device
Tree structure which reflects the hardware one, separating USI control
from actual protocol nodes.

> For example: I'm sure this is the case on USIv2 devices too, but on
> Exynos7885, different devices have USI modes configured differently.
> For example a Samsung Galaxy A8 (2018) has all the USI blocks
> configured as SPI while a Samsung Galaxy M20 has the first USI
> configured as dual HSI2C, the second as HSI2C on the first 2 pins and
> the third as HSI2C on the last 2 pins. With this way of doing
> everything on USIv2 we'd need 3 disabled USIv2 nodes in the SoC DTSI
> for one USI block, each for every protocol the USI block can do, all
> having a single child for their protocol and each referencing the same
> sysreg (not even sure if that's even supported). Then the board DTS
> could enable the USI node it needs.
>

If I'm following you correctly, then it's not like that. I guess
Krzysztof already replied to that, so I'll probably just repeat his
words. In that case you'll have something like this in your SoC dtsi
(for your USIv1 case of course, because dual HSI2C is not present in
USIv2):

<<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
usi1 {
    spi1 {
    };

    hsi2c1_1 {
    };

    hsi2c1_2 {
    };
};

usi2 {
    spi2 {
    };

    hsi2c2_1 {
    };
};


usi3 {
    spi3 {
    };

    hsi2c2_2 {
    };
};
<<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>

and then in your board dts you just have to enable corresponding usi's
with proper modes, and enable chosen protocol nodes, like this:

<<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
&usi1 {
    status = "okay"
    samsung,mode = <USI_V1_DUAL_I2C>;
};

&hsi2c1_1 {
    status = "okay"
};

&hsi2c1_2 {
    status = "okay"
};
<<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>

> With the downstream way we could have just one USI node and we could
> add the 3 protocols it can do disabled as seperate or child nodes. This
> way the board DTS only needs to set the appropriate mode setting and
> enable the protocol it needs. I'd say much better than having 3 USI
> nodes for the same USI block.
>

Not sure if with downstream USI driver you can actually have protocols
as sub-nodes in USI node though. It doesn't do anything like
of_platform_populate().

Also, with this USIv2 driver you can do the same thing you described:
you can have just one USI node with 3 protocols as sub-nodes (or you
can even have protocol nodes outside of USI node, but I'd not
recommend that).

Actually I can see that it's my fault for not describing that case in
bindings example. I'll make sure to do that in v2. You also got me
thinking about default mode: sometimes SW_CONF reset value chooses
some protocol. In that case maybe it'd useful to have something like
USI_V2_DEFAULT, to tell driver to not touch SW_CONF at all. And also I
can add USI_V2_NONE while at it, so that driver can write 0x0 to
SW_CONF: that way no protocol will be selected. Maybe that can be
beneficial for PM reasons, if some board doesn't use some USI blocks
at all. Do you think it's feasible to add those two values to
dt-bindings header? And is it possible to do so in USIv1?

> Also this way is pretty USIv2 centric. Adding USIv1 support to this
> driver is difficult this way because of the the lack of USI_CON and
> USI_OPTION registers as a whole (so having nowhere to actually set the
> reg of the USI node to, as the only thing USIv1 has is the SW_CONF
> register). In my opinion being able to use the same driver and same
> device tree layout for USIv1 and USIv2 is a definite plus
>

Well, it's USIv2 driver after all. I never expected it can be extended
for USIv1 support. If you think it can be reused for USIv1, it's fine
by me. But we need to consider next things:
  - rename the driver to just "usi.c" (and also its configuration symbol)
  - provide different compatible for USIv1 (and maybe corresponding driver data)
  - rework bindings (header and doc); make sure existing bindings are
intact (we shouldn't change already introduced interfaces)
  - in case of USIv1 compatible; don't try to tinker with USIv2 registers
  - samsung,clkreq-on won't be available in case of USIv1 compatible

Because I don't have USIv1 SoC TRM (and neither do I possess some
USIv1 board which I can use for test), I don't think it's my place to
add USIv1 support. But I think it's possible to do so, using my input
above.

I can see how it might be frustrating having to do some extra work
(comparing to just using the code existing in downstream). But I guess
that's the difference: vendor is mostly concerned about competitive
advantage and getting to market fast, while upstream is more concerned
about quality, considering all use cases, and having proper design.
Anyway, we can work together to make it right, and to have both
IP-cores support. In the worst case, if those are too different, we
can have two separate drivers for those.

> The only real drawback of that way is having to add code for USIv2
> inside the UART, HSI2C, and SPI drivers but in my opinion the benefits
> overweigh the drawbacks greatly. We could even make the uart/spi/hsi2c
> drivers call a helper function in the USI driver to set their USI_CON
> and USI_OPTION registers up so that code would be shared and not
> duplicated. Wether this patch gets applied like this is not my choice
> though, I'll let the people responsible decide
> :-)
>

I'd argue that there are a lot of real drawbacks of using downstream
driver as is. That's why I completely re-designed and re-implemented
it. Downstream driver can't be built and function as a module, it
doesn't respect System Register sharing between consumers, it leads to
USI reset code duplication scattered across protocol drivers (that
arguably shouldn't even be aware of that), it doesn't reflect HW
structure clearly, it's not holding clocks needed for registers access
(btw, sysreg clock can be provided in syscon node, exactly for that
reason). As Krzysztof said, it also can't handle correct probe order
and deferred probes. Downstream driver might work fine for some
particular use-cases the vendor has, but in upstream it's better to
cover more cases we can expect, as upstream kernel is used on more
platforms, with more user space variants, etc.

I don't really think protocol drivers should be aware of USI registers
at all, but if we they do -- we can provide some API from USIv2 driver
later, with EXPORT_SYMBOL(), referencing corresponding USI instance by
phandle or using some other mechanism for inter-driver communication.

Of course, it's not my place to decide on patch acceptance too. But I
was under the impression that maintainers would be ok with this course
of actions. Also, upstream kernel seems to already follow the same
design for some similar drivers. See for example
drivers/soc/qcom/qcom_gsbi.c.

> Anyways, soon enough I can write an USIv1 driver after I submit all the
> 7885 stuff I'm working on currently. If you want to, you can add USIv2
> support to that driver, or if an USIv2 driver is already in upstream at
> that point, if it is written in the downstream way I can add v1 support
> to that, or if it's like this I'll have to make a whole seperate driver
> with a whole seperate DT structure.
>

If it's like you said (USIv1 only touches the SW_CONF register), I
guess USIv2 driver can be extended for USIv1 case. I already provided
my thoughts on such rework above. It's probably better to consult with
Krzysztof first. I guess the only way to figure out if it's feasible
or it's better to have separate exynos-usi-v1.c for USIv1, is to try
and add USIv1 support into USIv2 driver and see how pretty or ugly it
is :) Whatever the way you decide to go with, please add me to Cc list
when sending USIv1 patches.

> Best regards,
> David

Re: [PATCH 0/8] soc: samsung: Add USIv2 driver

[Krzysztof Kozlowski]

On 29/11/2021 14:56, Sam Protsenko wrote:
> On Sun, 28 Nov 2021 at 05:15, David Virag <virag.david003@gmail.com> wrote:
>>
>> Also this way is pretty USIv2 centric. Adding USIv1 support to this
>> driver is difficult this way because of the the lack of USI_CON and
>> USI_OPTION registers as a whole (so having nowhere to actually set the
>> reg of the USI node to, as the only thing USIv1 has is the SW_CONF
>> register). In my opinion being able to use the same driver and same
>> device tree layout for USIv1 and USIv2 is a definite plus
>>
> 
> Well, it's USIv2 driver after all. I never expected it can be extended
> for USIv1 support. If you think it can be reused for USIv1, it's fine
> by me. But we need to consider next things:
>   - rename the driver to just "usi.c" (and also its configuration symbol)
>   - provide different compatible for USIv1 (and maybe corresponding driver data)
>   - rework bindings (header and doc); make sure existing bindings are
> intact (we shouldn't change already introduced interfaces)
>   - in case of USIv1 compatible; don't try to tinker with USIv2 registers
>   - samsung,clkreq-on won't be available in case of USIv1 compatible

I expect this driver to be in future extended for USIv1 and I do not see
any problems in doing that for current Sam's approach. Most of our
drivers support several devices, sometimes with differences, and we
already have patterns solving it, e.g. ops structure or quirks bitmap.
Driver for new USIv1 compatible would skip setting USI_CON (or any other
unrelated register). Modification of SW_CONF could be shared or could be
also split, depending on complexity.

> 
> Because I don't have USIv1 SoC TRM (and neither do I possess some
> USIv1 board which I can use for test), I don't think it's my place to
> add USIv1 support. But I think it's possible to do so, using my input
> above.
> 
> I can see how it might be frustrating having to do some extra work
> (comparing to just using the code existing in downstream). But I guess
> that's the difference: vendor is mostly concerned about competitive
> advantage and getting to market fast, while upstream is more concerned
> about quality, considering all use cases, and having proper design.
> Anyway, we can work together to make it right, and to have both
> IP-cores support. In the worst case, if those are too different, we
> can have two separate drivers for those.
> 
>> The only real drawback of that way is having to add code for USIv2
>> inside the UART, HSI2C, and SPI drivers but in my opinion the benefits
>> overweigh the drawbacks greatly. We could even make the uart/spi/hsi2c
>> drivers call a helper function in the USI driver to set their USI_CON
>> and USI_OPTION registers up so that code would be shared and not
>> duplicated. Wether this patch gets applied like this is not my choice
>> though, I'll let the people responsible decide
>> :-)
>>
> 
> I'd argue that there are a lot of real drawbacks of using downstream
> driver as is. That's why I completely re-designed and re-implemented
> it. Downstream driver can't be built and function as a module, it
> doesn't respect System Register sharing between consumers, it leads to
> USI reset code duplication scattered across protocol drivers (that
> arguably shouldn't even be aware of that), it doesn't reflect HW
> structure clearly, it's not holding clocks needed for registers access
> (btw, sysreg clock can be provided in syscon node, exactly for that
> reason). As Krzysztof said, it also can't handle correct probe order
> and deferred probes. Downstream driver might work fine for some
> particular use-cases the vendor has, but in upstream it's better to
> cover more cases we can expect, as upstream kernel is used on more
> platforms, with more user space variants, etc.

Implementing USI in each of I2C/SPI/UART drivers is a big minus. Current
approach nicely encapsulates USI in dedicated driver without polluting
the other drivers with unrelated bus/protocol stuff.

Best regards,
Krzysztof

Re: [PATCH 0/8] soc: samsung: Add USIv2 driver

[David Virag]

On Mon, 2021-11-29 at 15:56 +0200, Sam Protsenko wrote:
> On Sun, 28 Nov 2021 at 05:15, David Virag <virag.david003@gmail.com>
> wrote:
> > 
> > On Sun, 2021-11-28 at 00:32 +0200, Sam Protsenko wrote:
> > > USIv2 IP-core provides selectable serial protocol (UART, SPI or
> > > High-Speed I2C); only one can be chosen at a time. This series
> > > implements USIv2 driver, which allows one to select particular USI
> > > function in device tree, and also performs USI block
> > > initialization.
> > > 
> > > With that driver implemented, it's not needed to do USI
> > > initialization
> > > in protocol drivers anymore, so that code is removed from the
> > > serial
> > > driver.
> > > 
> > 
> > I think the downstream way of doing this (USI node reg being on the
> > SW_CONF register itself rather than an offset from uart/i2c/spi, the
> > USI driver only controlling the SW_CONF, and the uart/i2c/spi drivers
> > controlling their USI_CON and USI_OPTION regs) is cleaner, better,
> > and
> > easier to adapt to USIv1 too.
> > 
> 
> One reason why I think it's better to provide SW_CONF register via
> syscon node, is that it helps us to avoid possible register access
> conflicts in future, and also conflicts when requesting corresponding
> resources. In other words, the System Register block can be used by
> many consumers (drivers) in future; those consumers might try to
> modify the same registers simultaneously, which might lead to race
> conditions (as RMW operation is not atomic), so some kind of
> serialization should be done (like locking in regmap), which is
> provided by syscon. Also, that wouldn't even come to that: you just
> can't request the same I/O area twice in Linux. So if SW_CONF is
> passed via "reg" property to USI driver, and then we try to map the
> whole System Register (or its portion that includes SW_CONF), that
> request would fail.

I've got to admit, that's something I didn't think about much, partly
because the lack of TRM on my hand, as I'm working with just vendor
kernel sources and consumer phones. What other things are in the sysreg
in your case? Looking at my vendor device tree, the USI SW_CONF
registers are at 0x10032000-0x10032008 in my case and the DT lacks
anything else close by (in the 0x1003xxxx region).

> 
> Although passing one SW_CONF register via "reg" might look easier to
> implement, it might also bring us all sort of problems later on. And I
> think a good design should account for such pitfalls.
> 
> As for the USI registers: I really don't think that duplicating the
> code for USI block reset across uart/i2c/spi drivers would help us to
> accomplish anything. Why those drivers should be even aware of USI
> reset? At least in USIv2 block, the USI registers and uart/i2c/spi
> registers are not mixed: they are located at different and always
> fixed addresses. We can benefit from that fact, and provide Device
> Tree structure which reflects the hardware one, separating USI control
> from actual protocol nodes.
> 
> > For example: I'm sure this is the case on USIv2 devices too, but on
> > Exynos7885, different devices have USI modes configured differently.
> > For example a Samsung Galaxy A8 (2018) has all the USI blocks
> > configured as SPI while a Samsung Galaxy M20 has the first USI
> > configured as dual HSI2C, the second as HSI2C on the first 2 pins and
> > the third as HSI2C on the last 2 pins. With this way of doing
> > everything on USIv2 we'd need 3 disabled USIv2 nodes in the SoC DTSI
> > for one USI block, each for every protocol the USI block can do, all
> > having a single child for their protocol and each referencing the
> > same
> > sysreg (not even sure if that's even supported). Then the board DTS
> > could enable the USI node it needs.
> > 
> 
> If I'm following you correctly, then it's not like that. I guess
> Krzysztof already replied to that, so I'll probably just repeat his
> words. In that case you'll have something like this in your SoC dtsi
> (for your USIv1 case of course, because dual HSI2C is not present in
> USIv2):
> 
> <<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
> usi1 {
>     spi1 {
>     };
> 
>     hsi2c1_1 {
>     };
> 
>     hsi2c1_2 {
>     };
> };
> 
> usi2 {
>     spi2 {
>     };
> 
>     hsi2c2_1 {
>     };
> };
> 
> 
> usi3 {
>     spi3 {
>     };
> 
>     hsi2c2_2 {
>     };
> };
> <<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
> 
> and then in your board dts you just have to enable corresponding usi's
> with proper modes, and enable chosen protocol nodes, like this:
> 
> <<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
> &usi1 {
>     status = "okay"
>     samsung,mode = <USI_V1_DUAL_I2C>;
> };
> 
> &hsi2c1_1 {
>     status = "okay"
> };
> 
> &hsi2c1_2 {
>     status = "okay"
> };
> <<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>

What got me confused is the following: Upon checking vendor drivers I
was under the impression that we have all 3 protocols at seperate
addresses, and the USI SW_CONF register kind of works like a
multiplexer for the USI pins to switch between protocols. Now I see
that I was wrong, and the addresses are in fact the same. Now on a
hardware level it might still work just as a multiplexer but it
swithches the entire address space for a whole different protocol
block. Dumb little misunderstanding on my part, never mind! They are on
the same address even on USIv1. Not sure how I haven't noticed that
before, I guess since I never started experimenting with USI before,
just looked at the code as a reference I assumed a lot of things.

> 
> > With the downstream way we could have just one USI node and we could
> > add the 3 protocols it can do disabled as seperate or child nodes.
> > This
> > way the board DTS only needs to set the appropriate mode setting and
> > enable the protocol it needs. I'd say much better than having 3 USI
> > nodes for the same USI block.
> > 
> 
> Not sure if with downstream USI driver you can actually have protocols
> as sub-nodes in USI node though. It doesn't do anything like
> of_platform_populate().

It can't as far as I'm aware, I was just thinking that did seem like a
good idea to keep.

> 
> Also, with this USIv2 driver you can do the same thing you described:
> you can have just one USI node with 3 protocols as sub-nodes (or you
> can even have protocol nodes outside of USI node, but I'd not
> recommend that).
> 
> Actually I can see that it's my fault for not describing that case in
> bindings example. I'll make sure to do that in v2. You also got me
> thinking about default mode: sometimes SW_CONF reset value chooses
> some protocol. In that case maybe it'd useful to have something like
> USI_V2_DEFAULT, to tell driver to not touch SW_CONF at all.

Not sure if that's useful, I'm thinking we specify some protocol for
the USIs in board dts anyways, and if we don't, then we probably don't
use that USI block anyways, so at a minimum all protocols should be
probably disabled in that case, and probably the USI block as a whole
too. (SoC dtsi has them disabled, board dts doesn't touch them, so they
remain disabled). May I know how do you think a defult mode would be
useful?

> And also I
> can add USI_V2_NONE while at it, so that driver can write 0x0 to
> SW_CONF: that way no protocol will be selected. Maybe that can be
> beneficial for PM reasons, if some board doesn't use some USI blocks
> at all. Do you think it's feasible to add those two values to
> dt-bindings header? And is it possible to do so in USIv1?

I think I saw some downstream driver do something similiar, that sounds
like a good idea. In USIv1 I can see the HSI2C driver writing 0 to the
SW_CONF register at pm suspend. Not sure why that's in the HSI2C driver
rather than the USI but I'm guessing it should do the same thing as for
you. I have no TRM though, so not sure. We'll probably just have to
assume that's how it works here, maybe someone that has access to an
USIv1 SoC TRM could confirm? Probably won't get any response from
anyone who has it though.
 
> 
> > Also this way is pretty USIv2 centric. Adding USIv1 support to this
> > driver is difficult this way because of the the lack of USI_CON and
> > USI_OPTION registers as a whole (so having nowhere to actually set
> > the
> > reg of the USI node to, as the only thing USIv1 has is the SW_CONF
> > register). In my opinion being able to use the same driver and same
> > device tree layout for USIv1 and USIv2 is a definite plus
> > 
> 
> Well, it's USIv2 driver after all. I never expected it can be extended
> for USIv1 support. If you think it can be reused for USIv1, it's fine
> by me. But we need to consider next things:
>   - rename the driver to just "usi.c" (and also its configuration
> symbol)
>   - provide different compatible for USIv1 (and maybe corresponding
> driver data)
>   - rework bindings (header and doc); make sure existing bindings are
> intact (we shouldn't change already introduced interfaces)
>   - in case of USIv1 compatible; don't try to tinker with USIv2
> registers
>   - samsung,clkreq-on won't be available in case of USIv1 compatible
> 
> Because I don't have USIv1 SoC TRM (and neither do I possess some
> USIv1 board which I can use for test), I don't think it's my place to
> add USIv1 support. But I think it's possible to do so, using my input
> above.
> 
> I can see how it might be frustrating having to do some extra work
> (comparing to just using the code existing in downstream). But I guess
> that's the difference: vendor is mostly concerned about competitive
> advantage and getting to market fast, while upstream is more concerned
> about quality, considering all use cases, and having proper design.

It's not really the extra work, I just didn't see the benefits of this
way, and my misunderstanding caused me to not see how this would work.
I never really wanted to use the downstream driver as is, but in my
head I was thinking that "layout" should work.

> Anyway, we can work together to make it right, and to have both
> IP-cores support. In the worst case, if those are too different, we
> can have two separate drivers for those.
> 
> > The only real drawback of that way is having to add code for USIv2
> > inside the UART, HSI2C, and SPI drivers but in my opinion the
> > benefits
> > overweigh the drawbacks greatly. We could even make the
> > uart/spi/hsi2c
> > drivers call a helper function in the USI driver to set their
> > USI_CON
> > and USI_OPTION registers up so that code would be shared and not
> > duplicated. Wether this patch gets applied like this is not my
> > choice
> > though, I'll let the people responsible decide
> > :-)
> > 
> 
> I'd argue that there are a lot of real drawbacks of using downstream
> driver as is. That's why I completely re-designed and re-implemented
> it. Downstream driver can't be built and function as a module, it
> doesn't respect System Register sharing between consumers, it leads
> to
> USI reset code duplication scattered across protocol drivers (that
> arguably shouldn't even be aware of that), it doesn't reflect HW
> structure clearly, it's not holding clocks needed for registers
> access
> (btw, sysreg clock can be provided in syscon node, exactly for that
> reason). As Krzysztof said, it also can't handle correct probe order
> and deferred probes. Downstream driver might work fine for some
> particular use-cases the vendor has, but in upstream it's better to
> cover more cases we can expect, as upstream kernel is used on more
> platforms, with more user space variants, etc.

I do agree now, as I said a bit of a misunderstanding made me believe
this was wrong. (as if the addresses were different and the downstream
drivers worked the same way that would mean each USIv2 would have 3
sets of USI_CON and USI_OPTION registers for each protocol which would
definitely have to be handled somewhat differently.

> 
> I don't really think protocol drivers should be aware of USI
> registers
> at all, but if we they do -- we can provide some API from USIv2
> driver
> later, with EXPORT_SYMBOL(), referencing corresponding USI instance
> by
> phandle or using some other mechanism for inter-driver communication.
> 
> Of course, it's not my place to decide on patch acceptance too. But I
> was under the impression that maintainers would be ok with this
> course
> of actions. Also, upstream kernel seems to already follow the same
> design for some similar drivers. See for example
> drivers/soc/qcom/qcom_gsbi.c.
> 
> > Anyways, soon enough I can write an USIv1 driver after I submit all
> > the
> > 7885 stuff I'm working on currently. If you want to, you can add
> > USIv2
> > support to that driver, or if an USIv2 driver is already in
> > upstream at
> > that point, if it is written in the downstream way I can add v1
> > support
> > to that, or if it's like this I'll have to make a whole seperate
> > driver
> > with a whole seperate DT structure.
> > 
> 
> If it's like you said (USIv1 only touches the SW_CONF register), I
> guess USIv2 driver can be extended for USIv1 case. I already provided
> my thoughts on such rework above. It's probably better to consult
> with
> Krzysztof first. I guess the only way to figure out if it's feasible
> or it's better to have separate exynos-usi-v1.c for USIv1, is to try
> and add USIv1 support into USIv2 driver and see how pretty or ugly it
> is :) Whatever the way you decide to go with, please add me to Cc
> list
> when sending USIv1 patches.

Sure, I'll try doing it on top of the final version of your driver
then! Sorry for the misunderstanding there!

> 
> > Best regards,
> > David

Re: [PATCH 0/8] soc: samsung: Add USIv2 driver

[Sam Protsenko]

On Mon, 29 Nov 2021 at 21:19, David Virag <virag.david003@gmail.com> wrote:
>
> On Mon, 2021-11-29 at 15:56 +0200, Sam Protsenko wrote:
> > On Sun, 28 Nov 2021 at 05:15, David Virag <virag.david003@gmail.com>
> > wrote:
> > >
> > > On Sun, 2021-11-28 at 00:32 +0200, Sam Protsenko wrote:
> > > > USIv2 IP-core provides selectable serial protocol (UART, SPI or
> > > > High-Speed I2C); only one can be chosen at a time. This series
> > > > implements USIv2 driver, which allows one to select particular USI
> > > > function in device tree, and also performs USI block
> > > > initialization.
> > > >
> > > > With that driver implemented, it's not needed to do USI
> > > > initialization
> > > > in protocol drivers anymore, so that code is removed from the
> > > > serial
> > > > driver.
> > > >
> > >
> > > I think the downstream way of doing this (USI node reg being on the
> > > SW_CONF register itself rather than an offset from uart/i2c/spi, the
> > > USI driver only controlling the SW_CONF, and the uart/i2c/spi drivers
> > > controlling their USI_CON and USI_OPTION regs) is cleaner, better,
> > > and
> > > easier to adapt to USIv1 too.
> > >
> >
> > One reason why I think it's better to provide SW_CONF register via
> > syscon node, is that it helps us to avoid possible register access
> > conflicts in future, and also conflicts when requesting corresponding
> > resources. In other words, the System Register block can be used by
> > many consumers (drivers) in future; those consumers might try to
> > modify the same registers simultaneously, which might lead to race
> > conditions (as RMW operation is not atomic), so some kind of
> > serialization should be done (like locking in regmap), which is
> > provided by syscon. Also, that wouldn't even come to that: you just
> > can't request the same I/O area twice in Linux. So if SW_CONF is
> > passed via "reg" property to USI driver, and then we try to map the
> > whole System Register (or its portion that includes SW_CONF), that
> > request would fail.
>
> I've got to admit, that's something I didn't think about much, partly
> because the lack of TRM on my hand, as I'm working with just vendor
> kernel sources and consumer phones. What other things are in the sysreg
> in your case? Looking at my vendor device tree, the USI SW_CONF
> registers are at 0x10032000-0x10032008 in my case and the DT lacks
> anything else close by (in the 0x1003xxxx region).
>

Just in case, System Register is not a single register, but a register
block. In case of Exynos850 I have all sorts of registers in SYSREG.
Basically I have one SYSREG per domain, e.g. for PERI domain I have
SYSREG_PERI. Registers inside of each SYSREG may vary. SYREG_PERI has
IPCLK control register, SW_CONF registers, APB register, some USER
registers, etc, etc...

You can use something like this to find SYSREG info in your kernel:

    $ find drivers/ -type f -name '*7885*' -exec grep -Hni 'SYSREG' {} \;
    $ git grep -n --all-match -e SYSREG -e 1003 -- drivers/*7885*

Looking at Exynos7885 downstream kernel, you have next SYSREGs:

    SYSREG_PERI    0x10030000
    SYSREG_MIF0    0x10470000
    SYSREG_MIF1    0x10570000
    SYSREG_CPUCL0    0x10910000
    SYSREG_CPUCL1    0x10810000
    SYSREG_CPUCL2    0x10A10000
    SYSREG_APM    0x11C20000
    SYSREG_CORE    0x12010000
    SYSREG_FSYS    0x13420000

Those are base addresses for each sysreg. My wild guess, each SYSREG
size would be at least 0x10000.

SYSREG which contains SW_CONF registers for USI blocks is apparently
SYSREG_PERI. And SW_CONF offsets for each USI (inside of SYSREG_PERIO)
are:

    USI0: 0x2000
    USI1: 0x2004
    USI2: 0x2008

> >
> > Although passing one SW_CONF register via "reg" might look easier to
> > implement, it might also bring us all sort of problems later on. And I
> > think a good design should account for such pitfalls.
> >
> > As for the USI registers: I really don't think that duplicating the
> > code for USI block reset across uart/i2c/spi drivers would help us to
> > accomplish anything. Why those drivers should be even aware of USI
> > reset? At least in USIv2 block, the USI registers and uart/i2c/spi
> > registers are not mixed: they are located at different and always
> > fixed addresses. We can benefit from that fact, and provide Device
> > Tree structure which reflects the hardware one, separating USI control
> > from actual protocol nodes.
> >
> > > For example: I'm sure this is the case on USIv2 devices too, but on
> > > Exynos7885, different devices have USI modes configured differently.
> > > For example a Samsung Galaxy A8 (2018) has all the USI blocks
> > > configured as SPI while a Samsung Galaxy M20 has the first USI
> > > configured as dual HSI2C, the second as HSI2C on the first 2 pins and
> > > the third as HSI2C on the last 2 pins. With this way of doing
> > > everything on USIv2 we'd need 3 disabled USIv2 nodes in the SoC DTSI
> > > for one USI block, each for every protocol the USI block can do, all
> > > having a single child for their protocol and each referencing the
> > > same
> > > sysreg (not even sure if that's even supported). Then the board DTS
> > > could enable the USI node it needs.
> > >
> >
> > If I'm following you correctly, then it's not like that. I guess
> > Krzysztof already replied to that, so I'll probably just repeat his
> > words. In that case you'll have something like this in your SoC dtsi
> > (for your USIv1 case of course, because dual HSI2C is not present in
> > USIv2):
> >
> > <<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
> > usi1 {
> >     spi1 {
> >     };
> >
> >     hsi2c1_1 {
> >     };
> >
> >     hsi2c1_2 {
> >     };
> > };
> >
> > usi2 {
> >     spi2 {
> >     };
> >
> >     hsi2c2_1 {
> >     };
> > };
> >
> >
> > usi3 {
> >     spi3 {
> >     };
> >
> >     hsi2c2_2 {
> >     };
> > };
> > <<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
> >
> > and then in your board dts you just have to enable corresponding usi's
> > with proper modes, and enable chosen protocol nodes, like this:
> >
> > <<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
> > &usi1 {
> >     status = "okay"
> >     samsung,mode = <USI_V1_DUAL_I2C>;
> > };
> >
> > &hsi2c1_1 {
> >     status = "okay"
> > };
> >
> > &hsi2c1_2 {
> >     status = "okay"
> > };
> > <<<<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>>>>>>
>
> What got me confused is the following: Upon checking vendor drivers I
> was under the impression that we have all 3 protocols at seperate
> addresses, and the USI SW_CONF register kind of works like a
> multiplexer for the USI pins to switch between protocols. Now I see
> that I was wrong, and the addresses are in fact the same. Now on a
> hardware level it might still work just as a multiplexer but it
> swithches the entire address space for a whole different protocol
> block. Dumb little misunderstanding on my part, never mind! They are on
> the same address even on USIv1. Not sure how I haven't noticed that
> before, I guess since I never started experimenting with USI before,
> just looked at the code as a reference I assumed a lot of things.
>

Ah, yeah, USI block actually shares most of its internal circuits
within each protocol. So you can only choose one protocol per USI. I
should probably add that info to the bindings doc.

> >
> > > With the downstream way we could have just one USI node and we could
> > > add the 3 protocols it can do disabled as seperate or child nodes.
> > > This
> > > way the board DTS only needs to set the appropriate mode setting and
> > > enable the protocol it needs. I'd say much better than having 3 USI
> > > nodes for the same USI block.
> > >
> >
> > Not sure if with downstream USI driver you can actually have protocols
> > as sub-nodes in USI node though. It doesn't do anything like
> > of_platform_populate().
>
> It can't as far as I'm aware, I was just thinking that did seem like a
> good idea to keep.
>
> >
> > Also, with this USIv2 driver you can do the same thing you described:
> > you can have just one USI node with 3 protocols as sub-nodes (or you
> > can even have protocol nodes outside of USI node, but I'd not
> > recommend that).
> >
> > Actually I can see that it's my fault for not describing that case in
> > bindings example. I'll make sure to do that in v2. You also got me
> > thinking about default mode: sometimes SW_CONF reset value chooses
> > some protocol. In that case maybe it'd useful to have something like
> > USI_V2_DEFAULT, to tell driver to not touch SW_CONF at all.
>
> Not sure if that's useful, I'm thinking we specify some protocol for
> the USIs in board dts anyways, and if we don't, then we probably don't
> use that USI block anyways, so at a minimum all protocols should be
> probably disabled in that case, and probably the USI block as a whole
> too. (SoC dtsi has them disabled, board dts doesn't touch them, so they
> remain disabled). May I know how do you think a defult mode would be
> useful?
>

Yeah, you are right. I'll probably add USI_NONE configuration for 0x0.
Default one is really of no use.

> > And also I
> > can add USI_V2_NONE while at it, so that driver can write 0x0 to
> > SW_CONF: that way no protocol will be selected. Maybe that can be
> > beneficial for PM reasons, if some board doesn't use some USI blocks
> > at all. Do you think it's feasible to add those two values to
> > dt-bindings header? And is it possible to do so in USIv1?
>
> I think I saw some downstream driver do something similiar, that sounds
> like a good idea. In USIv1 I can see the HSI2C driver writing 0 to the
> SW_CONF register at pm suspend. Not sure why that's in the HSI2C driver
> rather than the USI but I'm guessing it should do the same thing as for
> you. I have no TRM though, so not sure. We'll probably just have to
> assume that's how it works here, maybe someone that has access to an
> USIv1 SoC TRM could confirm? Probably won't get any response from
> anyone who has it though.
>

I guess it's enough to have that kernel source code to figure out
essentials. When you set 0x0, no protocol is chosen, so we can imagine
roughly what happens inside of USI IP-core (internal circuits are not
connected, muxes are opened, etc). As I understand, 0x0 might be the
reset value for some SW_CONF registers, so it'll appear on PM resume,
so one should set SW_CONF on resume again (which is done in my driver
already).

> >
> > > Also this way is pretty USIv2 centric. Adding USIv1 support to this
> > > driver is difficult this way because of the the lack of USI_CON and
> > > USI_OPTION registers as a whole (so having nowhere to actually set
> > > the
> > > reg of the USI node to, as the only thing USIv1 has is the SW_CONF
> > > register). In my opinion being able to use the same driver and same
> > > device tree layout for USIv1 and USIv2 is a definite plus
> > >
> >
> > Well, it's USIv2 driver after all. I never expected it can be extended
> > for USIv1 support. If you think it can be reused for USIv1, it's fine
> > by me. But we need to consider next things:
> >   - rename the driver to just "usi.c" (and also its configuration
> > symbol)
> >   - provide different compatible for USIv1 (and maybe corresponding
> > driver data)
> >   - rework bindings (header and doc); make sure existing bindings are
> > intact (we shouldn't change already introduced interfaces)
> >   - in case of USIv1 compatible; don't try to tinker with USIv2
> > registers
> >   - samsung,clkreq-on won't be available in case of USIv1 compatible
> >
> > Because I don't have USIv1 SoC TRM (and neither do I possess some
> > USIv1 board which I can use for test), I don't think it's my place to
> > add USIv1 support. But I think it's possible to do so, using my input
> > above.
> >
> > I can see how it might be frustrating having to do some extra work
> > (comparing to just using the code existing in downstream). But I guess
> > that's the difference: vendor is mostly concerned about competitive
> > advantage and getting to market fast, while upstream is more concerned
> > about quality, considering all use cases, and having proper design.
>
> It's not really the extra work, I just didn't see the benefits of this
> way, and my misunderstanding caused me to not see how this would work.
> I never really wanted to use the downstream driver as is, but in my
> head I was thinking that "layout" should work.
>
> > Anyway, we can work together to make it right, and to have both
> > IP-cores support. In the worst case, if those are too different, we
> > can have two separate drivers for those.
> >
> > > The only real drawback of that way is having to add code for USIv2
> > > inside the UART, HSI2C, and SPI drivers but in my opinion the
> > > benefits
> > > overweigh the drawbacks greatly. We could even make the
> > > uart/spi/hsi2c
> > > drivers call a helper function in the USI driver to set their
> > > USI_CON
> > > and USI_OPTION registers up so that code would be shared and not
> > > duplicated. Wether this patch gets applied like this is not my
> > > choice
> > > though, I'll let the people responsible decide
> > > :-)
> > >
> >
> > I'd argue that there are a lot of real drawbacks of using downstream
> > driver as is. That's why I completely re-designed and re-implemented
> > it. Downstream driver can't be built and function as a module, it
> > doesn't respect System Register sharing between consumers, it leads
> > to
> > USI reset code duplication scattered across protocol drivers (that
> > arguably shouldn't even be aware of that), it doesn't reflect HW
> > structure clearly, it's not holding clocks needed for registers
> > access
> > (btw, sysreg clock can be provided in syscon node, exactly for that
> > reason). As Krzysztof said, it also can't handle correct probe order
> > and deferred probes. Downstream driver might work fine for some
> > particular use-cases the vendor has, but in upstream it's better to
> > cover more cases we can expect, as upstream kernel is used on more
> > platforms, with more user space variants, etc.
>
> I do agree now, as I said a bit of a misunderstanding made me believe
> this was wrong. (as if the addresses were different and the downstream
> drivers worked the same way that would mean each USIv2 would have 3
> sets of USI_CON and USI_OPTION registers for each protocol which would
> definitely have to be handled somewhat differently.
>

I've checked USIv2 driver code in Exynos7885 kernel (publicly
available), and it looks like it would be relatively easy to add that
to the driver I submitted. Please wait for my series to be Acked or
applied, then you can go ahead and send your additions on top of that.
I don't want to do that, as I don't have any HW I can validate that,
so it doesn't make much sense.

> >
> > I don't really think protocol drivers should be aware of USI
> > registers
> > at all, but if we they do -- we can provide some API from USIv2
> > driver
> > later, with EXPORT_SYMBOL(), referencing corresponding USI instance
> > by
> > phandle or using some other mechanism for inter-driver communication.
> >
> > Of course, it's not my place to decide on patch acceptance too. But I
> > was under the impression that maintainers would be ok with this
> > course
> > of actions. Also, upstream kernel seems to already follow the same
> > design for some similar drivers. See for example
> > drivers/soc/qcom/qcom_gsbi.c.
> >
> > > Anyways, soon enough I can write an USIv1 driver after I submit all
> > > the
> > > 7885 stuff I'm working on currently. If you want to, you can add
> > > USIv2
> > > support to that driver, or if an USIv2 driver is already in
> > > upstream at
> > > that point, if it is written in the downstream way I can add v1
> > > support
> > > to that, or if it's like this I'll have to make a whole seperate
> > > driver
> > > with a whole seperate DT structure.
> > >
> >
> > If it's like you said (USIv1 only touches the SW_CONF register), I
> > guess USIv2 driver can be extended for USIv1 case. I already provided
> > my thoughts on such rework above. It's probably better to consult
> > with
> > Krzysztof first. I guess the only way to figure out if it's feasible
> > or it's better to have separate exynos-usi-v1.c for USIv1, is to try
> > and add USIv1 support into USIv2 driver and see how pretty or ugly it
> > is :) Whatever the way you decide to go with, please add me to Cc
> > list
> > when sending USIv1 patches.
>
> Sure, I'll try doing it on top of the final version of your driver
> then! Sorry for the misunderstanding there!
>
> >
> > > Best regards,
> > > David
>