[PATCH v3 0/2] i2c: exynos5: add support for ExynosAutov9 SoC

[PATCH v3 0/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Jaewon Kim]

Serial IPs(UART, I2C, SPI) are integrated into New IP-Core
called USI(Universal Serial Interface).

As it is integrated into USI, there are additinal HW changes.
Registers to control USI and sysreg to set serial IPs have been added.
Also, some timing registres have been changed.

Changes in v3:
 - Changes has_usi variable type
 - Changes comment style

Changes in v2:
 - Changes compatible name to "samsung,exynosautov9-hsi2c"
 - Changes I2C type name to "I2C_TYPE_EXYNOSAUTOV9" from "I2C_TYPE_USI"
 - Changes to clear after reset instread of clearing reset
 - Add description about system register for USI
 - fix typo in description

Jaewon Kim (2):
  dt-bindings: i2c: exynos5: add exynosautov9-hsi2c compatible
  i2c: exynos5: add support for ExynosAutov9 SoC

 .../devicetree/bindings/i2c/i2c-exynos5.txt   |   7 +
 drivers/i2c/busses/i2c-exynos5.c              | 135 ++++++++++++++++--
 2 files changed, 132 insertions(+), 10 deletions(-)

-- 
2.33.1

[PATCH v3 1/2] dt-bindings: i2c: exynos5: add exynosautov9-hsi2c compatible

[Jaewon Kim]

This patch adds new "samsung,exynosautov9-hsi2c" compatible.
It is for i2c compatible with HSI2C available on Exynos SoC with USI.

Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 Documentation/devicetree/bindings/i2c/i2c-exynos5.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
index 2dbc0b62daa6..39f4067d9d1f 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
@@ -14,6 +14,8 @@ Required properties:
 				on Exynos5260 SoCs.
 	-> "samsung,exynos7-hsi2c", for i2c compatible with HSI2C available
 				on Exynos7 SoCs.
+	-> "samsung,exynosautov9-hsi2c", for i2c compatible with HSI2C available
+				on ExynosAutov9 SoCs.
 
   - reg: physical base address of the controller and length of memory mapped
     region.
@@ -31,6 +33,11 @@ Optional properties:
        at 100khz.
     -> If specified, the bus operates in high-speed mode only if the
        clock-frequency is >= 1Mhz.
+  - samsung,sysreg : system registers controller phandle to control USI.
+    -> If I2C integrated to USI(Universal Serial Interface), this property
+       is required. When using Exynos USI block, it needs to select which type
+       of Serial IPs(UART, SPI, I2C) to use with system register. So, it
+       requires samsung,sysreg phandle and offset value of system register.
 
 Example:
 
-- 
2.33.1

Re: [PATCH v3 1/2] dt-bindings: i2c: exynos5: add exynosautov9-hsi2c compatible

[Sam Protsenko]

On Fri, 12 Nov 2021 at 03:06, Jaewon Kim <jaewon02.kim@samsung.com> wrote:
>
> This patch adds new "samsung,exynosautov9-hsi2c" compatible.
> It is for i2c compatible with HSI2C available on Exynos SoC with USI.
>
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  Documentation/devicetree/bindings/i2c/i2c-exynos5.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> index 2dbc0b62daa6..39f4067d9d1f 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> @@ -14,6 +14,8 @@ Required properties:
>                                 on Exynos5260 SoCs.
>         -> "samsung,exynos7-hsi2c", for i2c compatible with HSI2C available
>                                 on Exynos7 SoCs.
> +       -> "samsung,exynosautov9-hsi2c", for i2c compatible with HSI2C available
> +                               on ExynosAutov9 SoCs.
>
>    - reg: physical base address of the controller and length of memory mapped
>      region.
> @@ -31,6 +33,11 @@ Optional properties:
>         at 100khz.
>      -> If specified, the bus operates in high-speed mode only if the
>         clock-frequency is >= 1Mhz.
> +  - samsung,sysreg : system registers controller phandle to control USI.
> +    -> If I2C integrated to USI(Universal Serial Interface), this property
> +       is required. When using Exynos USI block, it needs to select which type
> +       of Serial IPs(UART, SPI, I2C) to use with system register. So, it
> +       requires samsung,sysreg phandle and offset value of system register.
>
>  Example:
>
> --
> 2.33.1
>

Re: [PATCH v3 1/2] dt-bindings: i2c: exynos5: add exynosautov9-hsi2c compatible

[Krzysztof Kozlowski]

On 12/11/2021 02:01, Jaewon Kim wrote:
> This patch adds new "samsung,exynosautov9-hsi2c" compatible.
> It is for i2c compatible with HSI2C available on Exynos SoC with USI.
> 
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Thanks Jaewon for your patch. After more discussions and remarks from
David, I think we should go with dedicated USI driver instead of using
sysreg/syscon in every I2C/UART/SPI.

Therefore I want to remove my reviewed-by and instead ask to work on
dedicated USI driver (option 1 from Chanho's email).

It's not that this solution is anything bad, just it won't be flexible
to support USIv1. It will also lead to duplicated - and possibly
conflicting - USI configuration in each of drivers (I2C/UART/SPI).

> ---
>  Documentation/devicetree/bindings/i2c/i2c-exynos5.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> index 2dbc0b62daa6..39f4067d9d1f 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> @@ -14,6 +14,8 @@ Required properties:
>  				on Exynos5260 SoCs.
>  	-> "samsung,exynos7-hsi2c", for i2c compatible with HSI2C available
>  				on Exynos7 SoCs.
> +	-> "samsung,exynosautov9-hsi2c", for i2c compatible with HSI2C available
> +				on ExynosAutov9 SoCs.
>  
>    - reg: physical base address of the controller and length of memory mapped
>      region.
> @@ -31,6 +33,11 @@ Optional properties:
>         at 100khz.
>      -> If specified, the bus operates in high-speed mode only if the
>         clock-frequency is >= 1Mhz.
> +  - samsung,sysreg : system registers controller phandle to control USI.
> +    -> If I2C integrated to USI(Universal Serial Interface), this property
> +       is required. When using Exynos USI block, it needs to select which type
> +       of Serial IPs(UART, SPI, I2C) to use with system register. So, it
> +       requires samsung,sysreg phandle and offset value of system register.
>  
>  Example:
>  
> 


Best regards,
Krzysztof

[PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Jaewon Kim]

Serial IPs(UART, I2C, SPI) are integrated into New IP-Core
called USI(Universal Serial Interface).

As it is integrated into USI, there are additinal HW changes.
Registers to control USI and sysreg to set serial IPs have been added.
Also, some timing registres have been changed.

Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
---
 drivers/i2c/busses/i2c-exynos5.c | 135 ++++++++++++++++++++++++++++---
 1 file changed, 125 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 97d4f3ac0abd..6ce94795a618 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -22,6 +22,8 @@
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/spinlock.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 /*
  * HSI2C controller from Samsung supports 2 modes of operation
@@ -166,9 +168,21 @@
 
 #define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(100))
 
+/* USI(Universal Serial Interface) Register map */
+#define USI_CON					0xc4
+#define USI_OPTION				0xc8
+
+/* USI(Universal Serial Interface) Register bits */
+#define USI_CON_RESET				BIT(0)
+
+/* SYSREG Register bit */
+#define SYSREG_USI_SW_CONF_MASK			(0x7 << 0)
+#define SYSREG_I2C_SW_CONF			BIT(2)
+
 enum i2c_type_exynos {
 	I2C_TYPE_EXYNOS5,
 	I2C_TYPE_EXYNOS7,
+	I2C_TYPE_EXYNOSAUTOV9,
 };
 
 struct exynos5_i2c {
@@ -199,6 +213,10 @@ struct exynos5_i2c {
 
 	/* Version of HS-I2C Hardware */
 	const struct exynos_hsi2c_variant *variant;
+
+	/* USI sysreg info */
+	struct regmap		*usi_sysreg;
+	unsigned int		usi_offset;
 };
 
 /**
@@ -213,21 +231,31 @@ struct exynos5_i2c {
 struct exynos_hsi2c_variant {
 	unsigned int		fifo_depth;
 	enum i2c_type_exynos	hw;
+	bool			has_usi;
 };
 
 static const struct exynos_hsi2c_variant exynos5250_hsi2c_data = {
 	.fifo_depth	= 64,
 	.hw		= I2C_TYPE_EXYNOS5,
+	.has_usi	= false,
 };
 
 static const struct exynos_hsi2c_variant exynos5260_hsi2c_data = {
 	.fifo_depth	= 16,
 	.hw		= I2C_TYPE_EXYNOS5,
+	.has_usi	= false,
 };
 
 static const struct exynos_hsi2c_variant exynos7_hsi2c_data = {
 	.fifo_depth	= 16,
 	.hw		= I2C_TYPE_EXYNOS7,
+	.has_usi	= false,
+};
+
+static const struct exynos_hsi2c_variant exynosautov9_hsi2c_data = {
+	.fifo_depth	= 64,
+	.hw		= I2C_TYPE_EXYNOSAUTOV9,
+	.has_usi	= true,
 };
 
 static const struct of_device_id exynos5_i2c_match[] = {
@@ -243,6 +271,9 @@ static const struct of_device_id exynos5_i2c_match[] = {
 	}, {
 		.compatible = "samsung,exynos7-hsi2c",
 		.data = &exynos7_hsi2c_data
+	}, {
+		.compatible = "samsung,exynosautov9-hsi2c",
+		.data = &exynosautov9_hsi2c_data
 	}, {},
 };
 MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
@@ -281,6 +312,32 @@ static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c, bool hs_timings)
 		i2c->op_clock;
 	int div, clk_cycle, temp;
 
+	/*
+	 * In case of HSI2C controllers in EXYNOSAUTOV9
+	 * timing control formula changed.
+	 *
+	 * FSCL = IPCLK / ((CLK_DIV + 1) * 16)
+	 * T_SCL_LOW = IPCLK * (CLK_DIV + 1) * (N + M)
+	 *  [N : number of 0's in the TSCL_H_HS]
+	 *  [M : number of 0's in the TSCL_L_HS]
+	 * T_SCL_HIGH = IPCLK * (CLK_DIV + 1) * (N + M)
+	 *  [N : number of 1's in the TSCL_H_HS]
+	 *  [M : number of 1's in the TSCL_L_HS]
+	 *
+	 *  result of (N + M) is always 8.
+	 *  In general use case, we don't need to control timing_s1, timing_s2.
+	 */
+	if (i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9) {
+		div = ((clkin / (16 * i2c->op_clock)) - 1);
+		i2c_timing_s3 = div << 16;
+		if (hs_timings)
+			writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_HS3);
+		else
+			writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_FS3);
+
+		return 0;
+	}
+
 	/*
 	 * In case of HSI2C controller in Exynos5 series
 	 * FPCLK / FI2C =
@@ -355,6 +412,20 @@ static int exynos5_hsi2c_clock_setup(struct exynos5_i2c *i2c)
 	return exynos5_i2c_set_timing(i2c, true);
 }
 
+static void exynos_usi_reset(struct exynos5_i2c *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->regs + USI_CON);
+	val |= USI_CON_RESET;
+	writel(val, i2c->regs + USI_CON);
+	udelay(1);
+
+	val = readl(i2c->regs + USI_CON);
+	val &= ~USI_CON_RESET;
+	writel(val, i2c->regs + USI_CON);
+}
+
 /*
  * exynos5_i2c_init: configures the controller for I2C functionality
  * Programs I2C controller for Master mode operation
@@ -385,6 +456,9 @@ static void exynos5_i2c_reset(struct exynos5_i2c *i2c)
 {
 	u32 i2c_ctl;
 
+	if (i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9)
+		exynos_usi_reset(i2c);
+
 	/* Set and clear the bit for reset */
 	i2c_ctl = readl(i2c->regs + HSI2C_CTL);
 	i2c_ctl |= HSI2C_SW_RST;
@@ -422,7 +496,8 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 	writel(int_status, i2c->regs + HSI2C_INT_STATUS);
 
 	/* handle interrupt related to the transfer status */
-	if (i2c->variant->hw == I2C_TYPE_EXYNOS7) {
+	if (i2c->variant->hw == I2C_TYPE_EXYNOS7 ||
+			i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9) {
 		if (int_status & HSI2C_INT_TRANS_DONE) {
 			i2c->trans_done = 1;
 			i2c->state = 0;
@@ -569,13 +644,13 @@ static void exynos5_i2c_bus_check(struct exynos5_i2c *i2c)
 {
 	unsigned long timeout;
 
-	if (i2c->variant->hw != I2C_TYPE_EXYNOS7)
+	if (i2c->variant->hw == I2C_TYPE_EXYNOS5)
 		return;
 
 	/*
-	 * HSI2C_MASTER_ST_LOSE state in EXYNOS7 variant before transaction
-	 * indicates that bus is stuck (SDA is low). In such case bus recovery
-	 * can be performed.
+	 * HSI2C_MASTER_ST_LOSE state in EXYNOS7 or EXYNOSAUTOV9 variant before
+	 * transaction indicates that bus is stuck (SDA is low).
+	 * In such case bus recovery can be performed.
 	 */
 	timeout = jiffies + msecs_to_jiffies(100);
 	for (;;) {
@@ -611,10 +686,10 @@ static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop)
 	unsigned long flags;
 	unsigned short trig_lvl;
 
-	if (i2c->variant->hw == I2C_TYPE_EXYNOS7)
-		int_en |= HSI2C_INT_I2C_TRANS;
-	else
+	if (i2c->variant->hw == I2C_TYPE_EXYNOS5)
 		int_en |= HSI2C_INT_I2C;
+	else
+		int_en |= HSI2C_INT_I2C_TRANS;
 
 	i2c_ctl = readl(i2c->regs + HSI2C_CTL);
 	i2c_ctl &= ~(HSI2C_TXCHON | HSI2C_RXCHON);
@@ -738,6 +813,42 @@ static const struct i2c_algorithm exynos5_i2c_algorithm = {
 	.functionality		= exynos5_i2c_func,
 };
 
+static int exynos_usi_init(struct exynos5_i2c *i2c)
+{
+	struct device *dev = i2c->dev;
+	int ret;
+
+	if (!i2c->variant->has_usi)
+		return 0;
+
+	/*
+	 * System Register has a field that can select the serial IP
+	 * provided by USI. We need to set it to I2C to use I2C.
+	 */
+	i2c->usi_sysreg = syscon_regmap_lookup_by_phandle(dev->of_node,
+							  "samsung,sysreg");
+	if (IS_ERR(i2c->usi_sysreg)) {
+		dev_err(dev, "Cannot find sysreg\n");
+		return PTR_ERR(i2c->usi_sysreg);
+	}
+
+	ret = of_property_read_u32_index(dev->of_node, "samsung,sysreg",
+					 1, &i2c->usi_offset);
+	if (ret) {
+		dev_err(dev, "sysreg offset is not specified\n");
+		return ret;
+	}
+
+	ret = regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset,
+			SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF);
+	if (ret < 0)
+		return ret;
+
+	exynos_usi_reset(i2c);
+
+	return 0;
+}
+
 static int exynos5_i2c_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -777,6 +888,12 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
 	i2c->adap.algo_data = i2c;
 	i2c->adap.dev.parent = &pdev->dev;
 
+	i2c->variant = of_device_get_match_data(&pdev->dev);
+
+	ret = exynos_usi_init(i2c);
+	if (ret)
+		return ret;
+
 	/* Clear pending interrupts from u-boot or misc causes */
 	exynos5_i2c_clr_pend_irq(i2c);
 
@@ -794,8 +911,6 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 
-	i2c->variant = of_device_get_match_data(&pdev->dev);
-
 	ret = exynos5_hsi2c_clock_setup(i2c);
 	if (ret)
 		goto err_clk;
-- 
2.33.1

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Krzysztof Kozlowski]

On 12/11/2021 02:01, Jaewon Kim wrote:
> Serial IPs(UART, I2C, SPI) are integrated into New IP-Core
> called USI(Universal Serial Interface).
> 
> As it is integrated into USI, there are additinal HW changes.
> Registers to control USI and sysreg to set serial IPs have been added.
> Also, some timing registres have been changed.
> 
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
>  drivers/i2c/busses/i2c-exynos5.c | 135 ++++++++++++++++++++++++++++---
>  1 file changed, 125 insertions(+), 10 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Sam Protsenko]

On Fri, 12 Nov 2021 at 03:06, Jaewon Kim <jaewon02.kim@samsung.com> wrote:
>
> Serial IPs(UART, I2C, SPI) are integrated into New IP-Core
> called USI(Universal Serial Interface).
>
> As it is integrated into USI, there are additinal HW changes.
> Registers to control USI and sysreg to set serial IPs have been added.
> Also, some timing registres have been changed.
>
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Tested-by: Sam Protsenko <semen.protsenko@linaro.org>

With this patch the Exynos850 HSI2C becomes functional. The only
nit-pick from my side (just a food for thought): do we want to
configure USI related config inside of particular drivers (SPI, I2C,
UART)? Or it would be better design to implement some platform driver
for that, so we can choose USI configuration (SPI/I2C/UART) in device
tree? I think this series is good to be merged as is, but we should
probably consider all upsides and downsides of each option, for the
future work.

Thanks!

>  drivers/i2c/busses/i2c-exynos5.c | 135 ++++++++++++++++++++++++++++---
>  1 file changed, 125 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index 97d4f3ac0abd..6ce94795a618 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -22,6 +22,8 @@
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/spinlock.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
>
>  /*
>   * HSI2C controller from Samsung supports 2 modes of operation
> @@ -166,9 +168,21 @@
>
>  #define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(100))
>
> +/* USI(Universal Serial Interface) Register map */
> +#define USI_CON                                        0xc4
> +#define USI_OPTION                             0xc8
> +
> +/* USI(Universal Serial Interface) Register bits */
> +#define USI_CON_RESET                          BIT(0)
> +
> +/* SYSREG Register bit */
> +#define SYSREG_USI_SW_CONF_MASK                        (0x7 << 0)
> +#define SYSREG_I2C_SW_CONF                     BIT(2)
> +
>  enum i2c_type_exynos {
>         I2C_TYPE_EXYNOS5,
>         I2C_TYPE_EXYNOS7,
> +       I2C_TYPE_EXYNOSAUTOV9,
>  };
>
>  struct exynos5_i2c {
> @@ -199,6 +213,10 @@ struct exynos5_i2c {
>
>         /* Version of HS-I2C Hardware */
>         const struct exynos_hsi2c_variant *variant;
> +
> +       /* USI sysreg info */
> +       struct regmap           *usi_sysreg;
> +       unsigned int            usi_offset;
>  };
>
>  /**
> @@ -213,21 +231,31 @@ struct exynos5_i2c {
>  struct exynos_hsi2c_variant {
>         unsigned int            fifo_depth;
>         enum i2c_type_exynos    hw;
> +       bool                    has_usi;
>  };
>
>  static const struct exynos_hsi2c_variant exynos5250_hsi2c_data = {
>         .fifo_depth     = 64,
>         .hw             = I2C_TYPE_EXYNOS5,
> +       .has_usi        = false,
>  };
>
>  static const struct exynos_hsi2c_variant exynos5260_hsi2c_data = {
>         .fifo_depth     = 16,
>         .hw             = I2C_TYPE_EXYNOS5,
> +       .has_usi        = false,
>  };
>
>  static const struct exynos_hsi2c_variant exynos7_hsi2c_data = {
>         .fifo_depth     = 16,
>         .hw             = I2C_TYPE_EXYNOS7,
> +       .has_usi        = false,
> +};
> +
> +static const struct exynos_hsi2c_variant exynosautov9_hsi2c_data = {
> +       .fifo_depth     = 64,
> +       .hw             = I2C_TYPE_EXYNOSAUTOV9,
> +       .has_usi        = true,
>  };
>
>  static const struct of_device_id exynos5_i2c_match[] = {
> @@ -243,6 +271,9 @@ static const struct of_device_id exynos5_i2c_match[] = {
>         }, {
>                 .compatible = "samsung,exynos7-hsi2c",
>                 .data = &exynos7_hsi2c_data
> +       }, {
> +               .compatible = "samsung,exynosautov9-hsi2c",
> +               .data = &exynosautov9_hsi2c_data
>         }, {},
>  };
>  MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
> @@ -281,6 +312,32 @@ static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c, bool hs_timings)
>                 i2c->op_clock;
>         int div, clk_cycle, temp;
>
> +       /*
> +        * In case of HSI2C controllers in EXYNOSAUTOV9
> +        * timing control formula changed.
> +        *
> +        * FSCL = IPCLK / ((CLK_DIV + 1) * 16)
> +        * T_SCL_LOW = IPCLK * (CLK_DIV + 1) * (N + M)
> +        *  [N : number of 0's in the TSCL_H_HS]
> +        *  [M : number of 0's in the TSCL_L_HS]
> +        * T_SCL_HIGH = IPCLK * (CLK_DIV + 1) * (N + M)
> +        *  [N : number of 1's in the TSCL_H_HS]
> +        *  [M : number of 1's in the TSCL_L_HS]
> +        *
> +        *  result of (N + M) is always 8.
> +        *  In general use case, we don't need to control timing_s1, timing_s2.
> +        */
> +       if (i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9) {
> +               div = ((clkin / (16 * i2c->op_clock)) - 1);
> +               i2c_timing_s3 = div << 16;
> +               if (hs_timings)
> +                       writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_HS3);
> +               else
> +                       writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_FS3);
> +
> +               return 0;
> +       }
> +
>         /*
>          * In case of HSI2C controller in Exynos5 series
>          * FPCLK / FI2C =
> @@ -355,6 +412,20 @@ static int exynos5_hsi2c_clock_setup(struct exynos5_i2c *i2c)
>         return exynos5_i2c_set_timing(i2c, true);
>  }
>
> +static void exynos_usi_reset(struct exynos5_i2c *i2c)
> +{
> +       u32 val;
> +
> +       val = readl(i2c->regs + USI_CON);
> +       val |= USI_CON_RESET;
> +       writel(val, i2c->regs + USI_CON);
> +       udelay(1);
> +
> +       val = readl(i2c->regs + USI_CON);
> +       val &= ~USI_CON_RESET;
> +       writel(val, i2c->regs + USI_CON);
> +}
> +
>  /*
>   * exynos5_i2c_init: configures the controller for I2C functionality
>   * Programs I2C controller for Master mode operation
> @@ -385,6 +456,9 @@ static void exynos5_i2c_reset(struct exynos5_i2c *i2c)
>  {
>         u32 i2c_ctl;
>
> +       if (i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9)
> +               exynos_usi_reset(i2c);
> +
>         /* Set and clear the bit for reset */
>         i2c_ctl = readl(i2c->regs + HSI2C_CTL);
>         i2c_ctl |= HSI2C_SW_RST;
> @@ -422,7 +496,8 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>         writel(int_status, i2c->regs + HSI2C_INT_STATUS);
>
>         /* handle interrupt related to the transfer status */
> -       if (i2c->variant->hw == I2C_TYPE_EXYNOS7) {
> +       if (i2c->variant->hw == I2C_TYPE_EXYNOS7 ||
> +                       i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9) {
>                 if (int_status & HSI2C_INT_TRANS_DONE) {
>                         i2c->trans_done = 1;
>                         i2c->state = 0;
> @@ -569,13 +644,13 @@ static void exynos5_i2c_bus_check(struct exynos5_i2c *i2c)
>  {
>         unsigned long timeout;
>
> -       if (i2c->variant->hw != I2C_TYPE_EXYNOS7)
> +       if (i2c->variant->hw == I2C_TYPE_EXYNOS5)
>                 return;
>
>         /*
> -        * HSI2C_MASTER_ST_LOSE state in EXYNOS7 variant before transaction
> -        * indicates that bus is stuck (SDA is low). In such case bus recovery
> -        * can be performed.
> +        * HSI2C_MASTER_ST_LOSE state in EXYNOS7 or EXYNOSAUTOV9 variant before
> +        * transaction indicates that bus is stuck (SDA is low).
> +        * In such case bus recovery can be performed.
>          */
>         timeout = jiffies + msecs_to_jiffies(100);
>         for (;;) {
> @@ -611,10 +686,10 @@ static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop)
>         unsigned long flags;
>         unsigned short trig_lvl;
>
> -       if (i2c->variant->hw == I2C_TYPE_EXYNOS7)
> -               int_en |= HSI2C_INT_I2C_TRANS;
> -       else
> +       if (i2c->variant->hw == I2C_TYPE_EXYNOS5)
>                 int_en |= HSI2C_INT_I2C;
> +       else
> +               int_en |= HSI2C_INT_I2C_TRANS;
>
>         i2c_ctl = readl(i2c->regs + HSI2C_CTL);
>         i2c_ctl &= ~(HSI2C_TXCHON | HSI2C_RXCHON);
> @@ -738,6 +813,42 @@ static const struct i2c_algorithm exynos5_i2c_algorithm = {
>         .functionality          = exynos5_i2c_func,
>  };
>
> +static int exynos_usi_init(struct exynos5_i2c *i2c)
> +{
> +       struct device *dev = i2c->dev;
> +       int ret;
> +
> +       if (!i2c->variant->has_usi)
> +               return 0;
> +
> +       /*
> +        * System Register has a field that can select the serial IP
> +        * provided by USI. We need to set it to I2C to use I2C.
> +        */
> +       i2c->usi_sysreg = syscon_regmap_lookup_by_phandle(dev->of_node,
> +                                                         "samsung,sysreg");
> +       if (IS_ERR(i2c->usi_sysreg)) {
> +               dev_err(dev, "Cannot find sysreg\n");
> +               return PTR_ERR(i2c->usi_sysreg);
> +       }
> +
> +       ret = of_property_read_u32_index(dev->of_node, "samsung,sysreg",
> +                                        1, &i2c->usi_offset);
> +       if (ret) {
> +               dev_err(dev, "sysreg offset is not specified\n");
> +               return ret;
> +       }
> +
> +       ret = regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset,
> +                       SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF);
> +       if (ret < 0)
> +               return ret;
> +
> +       exynos_usi_reset(i2c);
> +
> +       return 0;
> +}
> +
>  static int exynos5_i2c_probe(struct platform_device *pdev)
>  {
>         struct device_node *np = pdev->dev.of_node;
> @@ -777,6 +888,12 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
>         i2c->adap.algo_data = i2c;
>         i2c->adap.dev.parent = &pdev->dev;
>
> +       i2c->variant = of_device_get_match_data(&pdev->dev);
> +
> +       ret = exynos_usi_init(i2c);
> +       if (ret)
> +               return ret;
> +
>         /* Clear pending interrupts from u-boot or misc causes */
>         exynos5_i2c_clr_pend_irq(i2c);
>
> @@ -794,8 +911,6 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
>                 goto err_clk;
>         }
>
> -       i2c->variant = of_device_get_match_data(&pdev->dev);
> -
>         ret = exynos5_hsi2c_clock_setup(i2c);
>         if (ret)
>                 goto err_clk;
> --
> 2.33.1
>

RE: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Chanho Park]

> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick
> from my side (just a food for thought): do we want to configure USI
> related config inside of particular drivers (SPI, I2C, UART)? Or it would
> be better design to implement some platform driver for that, so we can
> choose USI configuration (SPI/I2C/UART) in device tree? I think this
> series is good to be merged as is, but we should probably consider all
> upsides and downsides of each option, for the future work.

I'm also considering how to support this USI configuration gracefully.
Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far.
But, there is a possibility that the USI hw version can be bumped for future SoCs.

As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree.

Option1) Make a USI driver under soc/samsung/ like [1].
Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver.
Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree.

[1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c

Best Regards,
Chanho Park

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Krzysztof Kozlowski]

On 16/11/2021 02:12, Chanho Park wrote:
>> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick
>> from my side (just a food for thought): do we want to configure USI
>> related config inside of particular drivers (SPI, I2C, UART)? Or it would
>> be better design to implement some platform driver for that, so we can
>> choose USI configuration (SPI/I2C/UART) in device tree? I think this
>> series is good to be merged as is, but we should probably consider all
>> upsides and downsides of each option, for the future work.
> 
> I'm also considering how to support this USI configuration gracefully.
> Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far.
> But, there is a possibility that the USI hw version can be bumped for future SoCs.
> 
> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree.
> 
> Option1) Make a USI driver under soc/samsung/ like [1].
> Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver.
> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree.
> 
> [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c

I don't have user manuals, so all my knowledge here is based on
Exynos9825 vendor source code, therefore it is quite limited. In
devicetree the USI devices have their own nodes - but does it mean it's
separate SFR range dedicated to USI? Looks like that, especially that
address space is just for one register (4 bytes).

In such case having separate dedicated driver makes sense and you would
only have to care about driver ordering (e.g. via device links or phandles).

Option 2 looks interesting - reusing reset framework to set proper USI
mode, however this looks more like a hack. As you said Chanho, if there
is a USI version 3, this reset framework might not be sufficient.

In option 3 each driver (UART/I2C/SPI) would need to receive second IO
range and toggle some registers, which could be done via shared
function. If USI v3 is coming, all such drivers could get more complicated.

I think option 1 is the cleanest and extendable in future. It's easy to
add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It
also nicely encapsulates USI-related stuff in separate driver. Probe
ordering should not be a problem now.

But as I said, I don't have even the big picture here, so I rely on your
opinions more.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Sam Protsenko]

On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 16/11/2021 02:12, Chanho Park wrote:
> >> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick
> >> from my side (just a food for thought): do we want to configure USI
> >> related config inside of particular drivers (SPI, I2C, UART)? Or it would
> >> be better design to implement some platform driver for that, so we can
> >> choose USI configuration (SPI/I2C/UART) in device tree? I think this
> >> series is good to be merged as is, but we should probably consider all
> >> upsides and downsides of each option, for the future work.
> >
> > I'm also considering how to support this USI configuration gracefully.
> > Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far.
> > But, there is a possibility that the USI hw version can be bumped for future SoCs.
> >
> > As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree.
> >
> > Option1) Make a USI driver under soc/samsung/ like [1].
> > Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver.
> > Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree.
> >
> > [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c
>
> I don't have user manuals, so all my knowledge here is based on
> Exynos9825 vendor source code, therefore it is quite limited. In
> devicetree the USI devices have their own nodes - but does it mean it's
> separate SFR range dedicated to USI? Looks like that, especially that
> address space is just for one register (4 bytes).
>
> In such case having separate dedicated driver makes sense and you would
> only have to care about driver ordering (e.g. via device links or phandles).
>
> Option 2 looks interesting - reusing reset framework to set proper USI
> mode, however this looks more like a hack. As you said Chanho, if there
> is a USI version 3, this reset framework might not be sufficient.
>
> In option 3 each driver (UART/I2C/SPI) would need to receive second IO
> range and toggle some registers, which could be done via shared
> function. If USI v3 is coming, all such drivers could get more complicated.
>
> I think option 1 is the cleanest and extendable in future. It's easy to
> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It
> also nicely encapsulates USI-related stuff in separate driver. Probe
> ordering should not be a problem now.
>
> But as I said, I don't have even the big picture here, so I rely on your
> opinions more.
>

To provide more context, USI registers are split across two different
register maps:

    - USI protocol configuration (where we choose which protocol to
use: HSI2C, SPI or UART) is done via *_SW_CONF registers, from System
Register SFR range. To access those SW_CONF registers we need to
either:
        (Option 3) pass System Register registers to corresponding
driver (HSI2C/SPI/UART) via syscon
        (Option 1) or implement separate USI driver, so we can choose
desired protocol in device tree; in that case I guess System Register
registers should be passed via syscon to USI driver
    - USI registers (like USI_CON register, which contains USI_RESET
bit) are contained in the same SFR range as corresponding
HSI2C/SPI/UART IP-core. Or rather HSI2C/SPI/UART IP-cores are
encapsulated within USI block(s). So to access registers like USI_CON
we only need to use memory already passed to corresponding
HSI2C/SPI/UART driver via "reg" property.

So I'd say ideally we should do next:
  - modify USI registers (like USI_CON) in corresponding
HSI2C/SPI/UART drivers; but because all HSI2C/SPI/UART drivers share
the same USI registers, we might want to pull USI register offsets and
bits to some common header file, for example (to not duplicate that
code in drivers)
  - implement separate USI driver to control SW_CONF registers from
System Register module (Option 1), so we can choose desired protocol
in device tree (in some USI node, not in HSI2C node)
  - also, it probably makes sense to group all USI related nodes in
some separate *-usi.dtsi file; that would reduce confusion, given that
we have even more encapsulation in Exynos850 thanks to CMGP (Common
GPIO) block

My suggestion is to take this patch as is, and then we can work on USI
driver implementation/upstreaming. Right now we don't have much of
device tree files that use USI HSI2C driver, so it'll be fairly easy
to fix those dts's once we implemented USI driver. That will also
unblock me with submitting dev board support (dts files) I'm working
on right now. Krzysztof, please let me know if I'm wrong and if we
shouldn't change dts API too much, so it's mandatory to implement USI
driver before accepting this patch.

Thanks!

> Best regards,
> Krzysztof

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Krzysztof Kozlowski]

On 16/11/2021 16:31, Sam Protsenko wrote:
> On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 16/11/2021 02:12, Chanho Park wrote:
>>>> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick
>>>> from my side (just a food for thought): do we want to configure USI
>>>> related config inside of particular drivers (SPI, I2C, UART)? Or it would
>>>> be better design to implement some platform driver for that, so we can
>>>> choose USI configuration (SPI/I2C/UART) in device tree? I think this
>>>> series is good to be merged as is, but we should probably consider all
>>>> upsides and downsides of each option, for the future work.
>>>
>>> I'm also considering how to support this USI configuration gracefully.
>>> Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far.
>>> But, there is a possibility that the USI hw version can be bumped for future SoCs.
>>>
>>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree.
>>>
>>> Option1) Make a USI driver under soc/samsung/ like [1].
>>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver.
>>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree.
>>>
>>> [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c
>>
>> I don't have user manuals, so all my knowledge here is based on
>> Exynos9825 vendor source code, therefore it is quite limited. In
>> devicetree the USI devices have their own nodes - but does it mean it's
>> separate SFR range dedicated to USI? Looks like that, especially that
>> address space is just for one register (4 bytes).
>>
>> In such case having separate dedicated driver makes sense and you would
>> only have to care about driver ordering (e.g. via device links or phandles).
>>
>> Option 2 looks interesting - reusing reset framework to set proper USI
>> mode, however this looks more like a hack. As you said Chanho, if there
>> is a USI version 3, this reset framework might not be sufficient.
>>
>> In option 3 each driver (UART/I2C/SPI) would need to receive second IO
>> range and toggle some registers, which could be done via shared
>> function. If USI v3 is coming, all such drivers could get more complicated.
>>
>> I think option 1 is the cleanest and extendable in future. It's easy to
>> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It
>> also nicely encapsulates USI-related stuff in separate driver. Probe
>> ordering should not be a problem now.
>>
>> But as I said, I don't have even the big picture here, so I rely on your
>> opinions more.
>>
> 
> To provide more context, USI registers are split across two different
> register maps:
> 
>     - USI protocol configuration (where we choose which protocol to
> use: HSI2C, SPI or UART) is done via *_SW_CONF registers, from System
> Register SFR range. To access those SW_CONF registers we need to
> either:
>         (Option 3) pass System Register registers to corresponding
> driver (HSI2C/SPI/UART) via syscon
>         (Option 1) or implement separate USI driver, so we can choose
> desired protocol in device tree; in that case I guess System Register
> registers should be passed via syscon to USI driver
>     - USI registers (like USI_CON register, which contains USI_RESET
> bit) are contained in the same SFR range as corresponding
> HSI2C/SPI/UART IP-core. Or rather HSI2C/SPI/UART IP-cores are
> encapsulated within USI block(s). So to access registers like USI_CON
> we only need to use memory already passed to corresponding
> HSI2C/SPI/UART driver via "reg" property.
> 
> So I'd say ideally we should do next:
>   - modify USI registers (like USI_CON) in corresponding
> HSI2C/SPI/UART drivers; but because all HSI2C/SPI/UART drivers share
> the same USI registers, we might want to pull USI register offsets and
> bits to some common header file, for example (to not duplicate that
> code in drivers)
>   - implement separate USI driver to control SW_CONF registers from
> System Register module (Option 1), so we can choose desired protocol
> in device tree (in some USI node, not in HSI2C node)
>   - also, it probably makes sense to group all USI related nodes in
> some separate *-usi.dtsi file; that would reduce confusion, given that
> we have even more encapsulation in Exynos850 thanks to CMGP (Common
> GPIO) block
> 
> My suggestion is to take this patch as is, and then we can work on USI
> driver implementation/upstreaming. 

No, you cannot later rework it. It becomes a binding which you need to
support.

> Right now we don't have much of
> device tree files that use USI HSI2C driver, so it'll be fairly easy
> to fix those dts's once we implemented USI driver. 

Once sysreg solution is accepted, it's removal would be ABI break.
Please do not rush with incomplete solutions.

> That will also
> unblock me with submitting dev board support (dts files) I'm working
> on right now. Krzysztof, please let me know if I'm wrong and if we
> shouldn't change dts API too much, so it's mandatory to implement USI
> driver before accepting this patch.

David's point of USIv1 points me to the separate driver solution.


Best regards,
Krzysztof

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Sam Protsenko]

On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 16/11/2021 02:12, Chanho Park wrote:
> >> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick
> >> from my side (just a food for thought): do we want to configure USI
> >> related config inside of particular drivers (SPI, I2C, UART)? Or it would
> >> be better design to implement some platform driver for that, so we can
> >> choose USI configuration (SPI/I2C/UART) in device tree? I think this
> >> series is good to be merged as is, but we should probably consider all
> >> upsides and downsides of each option, for the future work.
> >
> > I'm also considering how to support this USI configuration gracefully.
> > Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far.
> > But, there is a possibility that the USI hw version can be bumped for future SoCs.
> >
> > As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree.
> >
> > Option1) Make a USI driver under soc/samsung/ like [1].
> > Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver.
> > Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree.
> >
> > [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c
>
> I don't have user manuals, so all my knowledge here is based on
> Exynos9825 vendor source code, therefore it is quite limited. In
> devicetree the USI devices have their own nodes - but does it mean it's
> separate SFR range dedicated to USI? Looks like that, especially that
> address space is just for one register (4 bytes).
>
> In such case having separate dedicated driver makes sense and you would
> only have to care about driver ordering (e.g. via device links or phandles).
>
> Option 2 looks interesting - reusing reset framework to set proper USI
> mode, however this looks more like a hack. As you said Chanho, if there
> is a USI version 3, this reset framework might not be sufficient.
>
> In option 3 each driver (UART/I2C/SPI) would need to receive second IO
> range and toggle some registers, which could be done via shared
> function. If USI v3 is coming, all such drivers could get more complicated.
>
> I think option 1 is the cleanest and extendable in future. It's easy to
> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It
> also nicely encapsulates USI-related stuff in separate driver. Probe
> ordering should not be a problem now.
>
> But as I said, I don't have even the big picture here, so I rely on your
> opinions more.
>

Hi Krzysztof,

Can you please let me know if you're going to apply this series as is,
or if you want me to submit USIv2 driver first, and then rework this
patch on top of it? I'm working on some HSI2C related patches right
now, and thus it'd nice to know about your decision on this series
beforehand, as some of my patches (like bindings doc patches) might
depend on it. Basically I'd like to base my patches on the proper
baseline, so we don't have to rebase those later.

Thanks!

> Best regards,
> Krzysztof

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Krzysztof Kozlowski]

On 18/11/2021 20:59, Sam Protsenko wrote:
> On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 16/11/2021 02:12, Chanho Park wrote:
>>>> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick
>>>> from my side (just a food for thought): do we want to configure USI
>>>> related config inside of particular drivers (SPI, I2C, UART)? Or it would
>>>> be better design to implement some platform driver for that, so we can
>>>> choose USI configuration (SPI/I2C/UART) in device tree? I think this
>>>> series is good to be merged as is, but we should probably consider all
>>>> upsides and downsides of each option, for the future work.
>>>
>>> I'm also considering how to support this USI configuration gracefully.
>>> Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far.
>>> But, there is a possibility that the USI hw version can be bumped for future SoCs.
>>>
>>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree.
>>>
>>> Option1) Make a USI driver under soc/samsung/ like [1].
>>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver.
>>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree.
>>>
>>> [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c
>>
>> I don't have user manuals, so all my knowledge here is based on
>> Exynos9825 vendor source code, therefore it is quite limited. In
>> devicetree the USI devices have their own nodes - but does it mean it's
>> separate SFR range dedicated to USI? Looks like that, especially that
>> address space is just for one register (4 bytes).
>>
>> In such case having separate dedicated driver makes sense and you would
>> only have to care about driver ordering (e.g. via device links or phandles).
>>
>> Option 2 looks interesting - reusing reset framework to set proper USI
>> mode, however this looks more like a hack. As you said Chanho, if there
>> is a USI version 3, this reset framework might not be sufficient.
>>
>> In option 3 each driver (UART/I2C/SPI) would need to receive second IO
>> range and toggle some registers, which could be done via shared
>> function. If USI v3 is coming, all such drivers could get more complicated.
>>
>> I think option 1 is the cleanest and extendable in future. It's easy to
>> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It
>> also nicely encapsulates USI-related stuff in separate driver. Probe
>> ordering should not be a problem now.
>>
>> But as I said, I don't have even the big picture here, so I rely on your
>> opinions more.
>>
> 
> Hi Krzysztof,
> 
> Can you please let me know if you're going to apply this series as is,
> or if you want me to submit USIv2 driver first, and then rework this
> patch on top of it? I'm working on some HSI2C related patches right
> now, and thus it'd nice to know about your decision on this series
> beforehand, as some of my patches (like bindings doc patches) might
> depend on it. Basically I'd like to base my patches on the proper
> baseline, so we don't have to rebase those later.

This set won't go via my tree anyway, but I am against it. David pointed
out that his USIv1 is a little bit different and embedding in each of
I2C/UART/SPI drivers the logic of controlling USIv1 and USIv2 looks too
big. The solution with a dedicated driver looks to me more flexible and
encapsulated/cleaner.

Therefore after the discussions I am against this solution, so a
soft-NAK from my side.


Best regards,
Krzysztof

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Sam Protsenko]

On Fri, 19 Nov 2021 at 10:54, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 18/11/2021 20:59, Sam Protsenko wrote:
> > On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> On 16/11/2021 02:12, Chanho Park wrote:
> >>>> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick
> >>>> from my side (just a food for thought): do we want to configure USI
> >>>> related config inside of particular drivers (SPI, I2C, UART)? Or it would
> >>>> be better design to implement some platform driver for that, so we can
> >>>> choose USI configuration (SPI/I2C/UART) in device tree? I think this
> >>>> series is good to be merged as is, but we should probably consider all
> >>>> upsides and downsides of each option, for the future work.
> >>>
> >>> I'm also considering how to support this USI configuration gracefully.
> >>> Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far.
> >>> But, there is a possibility that the USI hw version can be bumped for future SoCs.
> >>>
> >>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree.
> >>>
> >>> Option1) Make a USI driver under soc/samsung/ like [1].
> >>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver.
> >>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree.
> >>>
> >>> [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c
> >>
> >> I don't have user manuals, so all my knowledge here is based on
> >> Exynos9825 vendor source code, therefore it is quite limited. In
> >> devicetree the USI devices have their own nodes - but does it mean it's
> >> separate SFR range dedicated to USI? Looks like that, especially that
> >> address space is just for one register (4 bytes).
> >>
> >> In such case having separate dedicated driver makes sense and you would
> >> only have to care about driver ordering (e.g. via device links or phandles).
> >>
> >> Option 2 looks interesting - reusing reset framework to set proper USI
> >> mode, however this looks more like a hack. As you said Chanho, if there
> >> is a USI version 3, this reset framework might not be sufficient.
> >>
> >> In option 3 each driver (UART/I2C/SPI) would need to receive second IO
> >> range and toggle some registers, which could be done via shared
> >> function. If USI v3 is coming, all such drivers could get more complicated.
> >>
> >> I think option 1 is the cleanest and extendable in future. It's easy to
> >> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It
> >> also nicely encapsulates USI-related stuff in separate driver. Probe
> >> ordering should not be a problem now.
> >>
> >> But as I said, I don't have even the big picture here, so I rely on your
> >> opinions more.
> >>
> >
> > Hi Krzysztof,
> >
> > Can you please let me know if you're going to apply this series as is,
> > or if you want me to submit USIv2 driver first, and then rework this
> > patch on top of it? I'm working on some HSI2C related patches right
> > now, and thus it'd nice to know about your decision on this series
> > beforehand, as some of my patches (like bindings doc patches) might
> > depend on it. Basically I'd like to base my patches on the proper
> > baseline, so we don't have to rebase those later.
>
> This set won't go via my tree anyway, but I am against it. David pointed
> out that his USIv1 is a little bit different and embedding in each of
> I2C/UART/SPI drivers the logic of controlling USIv1 and USIv2 looks too
> big. The solution with a dedicated driver looks to me more flexible and
> encapsulated/cleaner.
>
> Therefore after the discussions I am against this solution, so a
> soft-NAK from my side.
>

Hi Jaewon,

I'm going to submit USI driver soon, and also some more HSI2C patches.
Do you mind if I rework your patches to rely on USI drver (instead of
modifying System Register in HSI2C driver), and include those in my
patch series? Of course, I'll preserve your authorship. Just think
that would be easier and faster this way.

Thanks!

>
> Best regards,
> Krzysztof

RE: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[Jaewon Kim]

Hi Protsenko,

> On Fri, 19 Nov 2021 at 10:54, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote:
> >
> > On 18/11/2021 20:59, Sam Protsenko wrote:
> > > On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski
> > > <krzysztof.kozlowski@canonical.com> wrote:
> > >>
> > >> On 16/11/2021 02:12, Chanho Park wrote:
> > >>>> With this patch the Exynos850 HSI2C becomes functional. The only
> > >>>> nit-pick from my side (just a food for thought): do we want to
> > >>>> configure USI related config inside of particular drivers (SPI,
> > >>>> I2C, UART)? Or it would be better design to implement some
> > >>>> platform driver for that, so we can choose USI configuration
> > >>>> (SPI/I2C/UART) in device tree? I think this series is good to be
> > >>>> merged as is, but we should probably consider all upsides and downsides of each option, for the
> future work.
> > >>>
> > >>> I'm also considering how to support this USI configuration gracefully.
> > >>> Current version of USI is v2 which means there is a v1 version as well. It might be a non-
> upstream SoC so we don't need to consider it so far.
> > >>> But, there is a possibility that the USI hw version can be bumped for future SoCs.
> > >>>
> > >>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was
> designed to be configured the USI settings by device tree.
> > >>>
> > >>> Option1) Make a USI driver under soc/samsung/ like [1].
> > >>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the
> reset core driver.
> > >>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose
> some configurations which can be variable as device tree.
> > >>>
> > >>> [1]:
> > >>> https://protect2.fireeye.com/v1/url?k=b290a67b-ed0b9f6a-b2912d34-0
> > >>> cc47a31cdbc-ceadd8e62313162a&q=1&e=317825c0-3fac-46ad-9b4e-f93de42
> > >>> ad5ba&u=https%3A%2F%2Fgithub.com%2Fianmacd%2Fd2s%2Fblob%2Fmaster%2
> > >>> Fdrivers%2Fsoc%2Fsamsung%2Fusi_v2.c
> > >>
> > >> I don't have user manuals, so all my knowledge here is based on
> > >> Exynos9825 vendor source code, therefore it is quite limited. In
> > >> devicetree the USI devices have their own nodes - but does it mean
> > >> it's separate SFR range dedicated to USI? Looks like that,
> > >> especially that address space is just for one register (4 bytes).
> > >>
> > >> In such case having separate dedicated driver makes sense and you
> > >> would only have to care about driver ordering (e.g. via device links or phandles).
> > >>
> > >> Option 2 looks interesting - reusing reset framework to set proper
> > >> USI mode, however this looks more like a hack. As you said Chanho,
> > >> if there is a USI version 3, this reset framework might not be sufficient.
> > >>
> > >> In option 3 each driver (UART/I2C/SPI) would need to receive second
> > >> IO range and toggle some registers, which could be done via shared
> > >> function. If USI v3 is coming, all such drivers could get more complicated.
> > >>
> > >> I think option 1 is the cleanest and extendable in future. It's
> > >> easy to add usi-v3 or whatever without modifying the UART/I2C/SPI
> > >> drivers. It also nicely encapsulates USI-related stuff in separate
> > >> driver. Probe ordering should not be a problem now.
> > >>
> > >> But as I said, I don't have even the big picture here, so I rely on
> > >> your opinions more.
> > >>
> > >
> > > Hi Krzysztof,
> > >
> > > Can you please let me know if you're going to apply this series as
> > > is, or if you want me to submit USIv2 driver first, and then rework
> > > this patch on top of it? I'm working on some HSI2C related patches
> > > right now, and thus it'd nice to know about your decision on this
> > > series beforehand, as some of my patches (like bindings doc patches)
> > > might depend on it. Basically I'd like to base my patches on the
> > > proper baseline, so we don't have to rebase those later.
> >
> > This set won't go via my tree anyway, but I am against it. David
> > pointed out that his USIv1 is a little bit different and embedding in
> > each of I2C/UART/SPI drivers the logic of controlling USIv1 and USIv2
> > looks too big. The solution with a dedicated driver looks to me more
> > flexible and encapsulated/cleaner.
> >
> > Therefore after the discussions I am against this solution, so a
> > soft-NAK from my side.
> >
> 
> Hi Jaewon,
> 
> I'm going to submit USI driver soon, and also some more HSI2C patches.
> Do you mind if I rework your patches to rely on USI drver (instead of modifying System Register in
> HSI2C driver), and include those in my patch series? Of course, I'll preserve your authorship. Just
> think that would be easier and faster this way.
> 
> Thanks!
> 

I'm glad you're working on a USI driver.
You can use my patchset.

> >
> > Best regards,
> > Krzysztof

Thanks
Jaewon Kim

Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC

[David Virag]

> Current version of USI is v2 which means there is a v1 version as well.
> It might be a non-upstream SoC so we don't need to consider it so far.

The Exynos7885 I'm working on has USI v1. It doesn't seem to be heavily
used as the SoC has just 3 USI blocks if I didn't miss anything.

The most obvious difference I saw was instead of having 3 modes (SPI,
UART, and HSI2C) It has:
 - SPI
 - HSI2C0 (meaning I2C pins are connected to the first 2 pins out of
the 4 if I understand it correctly)
 - HSI2C1 (connected to last 2 pins)
 - HSI2C0_HSI2C1 (2 I2C devices connected to first 2 and last 2 pins)
 - UART
 - UART_HSI2C1 (first 2 pins are UART, rest is I2C)

Also there doesn't seem to be any USI_CON or USI_OPTION registers in
SPI, UART, or I2C. It seems like it's just the USI driver doing all the
work (just setting up the SYSREG) and the I2C driver writing values to
the SYSREG at suspend/resume for some reason. 

From the looks of it, it doesn't look like it'd be hard to add this to
USI v2 drivers when needed. (USI driver (if that's the way it will go)
would just need minor modifications to add v1 modes and UART/SPI/I2C
drivers may just work with non-USI compatibles/would only need SoC
specific data added).

Best Regards,
David