[PATCH 1/2] i2c: exynos5: support USI(Universal Serial Interface)

[PATCH 1/2] i2c: exynos5: support USI(Universal Serial Interface)

[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 | 120 ++++++++++++++++++++++++++++---
 1 file changed, 110 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 97d4f3ac0abd..f2dc7848f840 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				(1 << 0)
+
+/* SYSREG Register bit */
+#define SYSREG_USI_SW_CONF_MASK			(0x7 << 0)
+#define SYSREG_I2C_SW_CONF			(1u << 2)
+
 enum i2c_type_exynos {
 	I2C_TYPE_EXYNOS5,
 	I2C_TYPE_EXYNOS7,
+	I2C_TYPE_USI,
 };
 
 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;
 };
 
 /**
@@ -230,6 +248,11 @@ static const struct exynos_hsi2c_variant exynos7_hsi2c_data = {
 	.hw		= I2C_TYPE_EXYNOS7,
 };
 
+static const struct exynos_hsi2c_variant exynos_usi_hsi2c_data = {
+	.fifo_depth	= 64,
+	.hw		= I2C_TYPE_USI,
+};
+
 static const struct of_device_id exynos5_i2c_match[] = {
 	{
 		.compatible = "samsung,exynos5-hsi2c",
@@ -243,6 +266,9 @@ static const struct of_device_id exynos5_i2c_match[] = {
 	}, {
 		.compatible = "samsung,exynos7-hsi2c",
 		.data = &exynos7_hsi2c_data
+	}, {
+		.compatible = "samsung,exynos-usi-hsi2c",
+		.data = &exynos_usi_hsi2c_data
 	}, {},
 };
 MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
@@ -281,6 +307,31 @@ 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 USI
+	 * 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 genaral case, we don`t need to control timing_s1, timing_s2.
+	 */
+	if (i2c->variant->hw == I2C_TYPE_USI) {
+		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 +406,16 @@ 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;
+
+	/* Clear the software reset of USI block (it's set at startup) */
+	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 +446,9 @@ static void exynos5_i2c_reset(struct exynos5_i2c *i2c)
 {
 	u32 i2c_ctl;
 
+	if (i2c->variant->hw == I2C_TYPE_USI)
+		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 +486,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_USI) {
 		if (int_status & HSI2C_INT_TRANS_DONE) {
 			i2c->trans_done = 1;
 			i2c->state = 0;
@@ -569,13 +634,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 EXYNOS_USI 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 +676,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 +803,37 @@ 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->hw != I2C_TYPE_USI)
+		return 0;
+
+	/* USI regmap control */
+	i2c->usi_sysreg = syscon_regmap_lookup_by_phandle(
+			dev->of_node, "samsung,usi-sysreg");
+	if (IS_ERR(i2c->usi_sysreg)) {
+		dev_err(dev, "Cannot find usi-sysreg\n");
+		return PTR_ERR(i2c->usi_sysreg);
+	}
+
+	ret = of_property_read_u32_index(dev->of_node,
+				"samsung,usi-sysreg", 1, &i2c->usi_offset);
+	if (ret) {
+		dev_err(dev, "usi-sysreg offset is not specified\n");
+		return ret;
+	}
+
+	regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset,
+			SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF);
+
+	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 +873,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 +896,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

[PATCH 2/2] dt-bindings: i2c: exynos5: add exynos-usi-hsi2c compatible

[Jaewon Kim]

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

Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
---
 Documentation/devicetree/bindings/i2c/i2c-exynos5.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
index 2dbc0b62daa6..ce2373c7a357 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,exynos-usi-hsi2c", for i2c compatible with HSI2C available
+				on Exynos SoCs with USI.
 
   - reg: physical base address of the controller and length of memory mapped
     region.
@@ -31,6 +33,8 @@ Optional properties:
        at 100khz.
     -> If specified, the bus operates in high-speed mode only if the
        clock-frequency is >= 1Mhz.
+  - samsung,usi-sysreg : sysreg handle to control USI type.
+    -> sysreg phandle for "samsung,exynos-usi-hsic" compatible.
 
 Example:
 
@@ -46,6 +50,8 @@ hsi2c@12ca0000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 
+	samsung,usi-sysreg = <&usi_sysreg 0x28>;
+
 	s2mps11_pmic@66 {
 		compatible = "samsung,s2mps11-pmic";
 		reg = <0x66>;
-- 
2.33.1

Re: [PATCH 2/2] dt-bindings: i2c: exynos5: add exynos-usi-hsi2c compatible

[Krzysztof Kozlowski]

On 01/11/2021 12:38, Jaewon Kim wrote:
> This patch adds new "samsung,exynos-usi-hsi2c" compatible.
> It is for i2c compatible with HSI2C available on Exynos SoC with USI.
> 
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-exynos5.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

The bindings go as first patch, please.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> index 2dbc0b62daa6..ce2373c7a357 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,exynos-usi-hsi2c", for i2c compatible with HSI2C available
> +				on Exynos SoCs with USI.

I would prefer to describe the Exynos model, not the feature. USI might
change between different SoCs, so then it will be "usiv2"?

>  
>    - reg: physical base address of the controller and length of memory mapped
>      region.
> @@ -31,6 +33,8 @@ Optional properties:
>         at 100khz.
>      -> If specified, the bus operates in high-speed mode only if the
>         clock-frequency is >= 1Mhz.
> +  - samsung,usi-sysreg : sysreg handle to control USI type.
> +    -> sysreg phandle for "samsung,exynos-usi-hsic" compatible.

s/sysreg/system registers controller/
s/handle/phandle/

Please document to what is this phandle. To which block.

Why it cannot be the existing generic samsung,sysreg?

>  
>  Example:
>  
> @@ -46,6 +50,8 @@ hsi2c@12ca0000 {
>  	#address-cells = <1>;
>  	#size-cells = <0>;
>  
> +	samsung,usi-sysreg = <&usi_sysreg 0x28>;

This does not look correct for this compatible. We should really convert
the bindings to YAML...

> +
>  	s2mps11_pmic@66 {
>  		compatible = "samsung,s2mps11-pmic";
>  		reg = <0x66>;
> 


Best regards,
Krzysztof

Re: [PATCH 1/2] i2c: exynos5: support USI(Universal Serial Interface)

[Krzysztof Kozlowski]

On 01/11/2021 12:38, 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 | 120 ++++++++++++++++++++++++++++---
>  1 file changed, 110 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index 97d4f3ac0abd..f2dc7848f840 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				(1 << 0)

BIT()

> +
> +/* SYSREG Register bit */
> +#define SYSREG_USI_SW_CONF_MASK			(0x7 << 0)
> +#define SYSREG_I2C_SW_CONF			(1u << 2)

BIT()

> +
>  enum i2c_type_exynos {
>  	I2C_TYPE_EXYNOS5,
>  	I2C_TYPE_EXYNOS7,
> +	I2C_TYPE_USI,
>  };
>  
>  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;
>  };
>  
>  /**
> @@ -230,6 +248,11 @@ static const struct exynos_hsi2c_variant exynos7_hsi2c_data = {
>  	.hw		= I2C_TYPE_EXYNOS7,
>  };
>  
> +static const struct exynos_hsi2c_variant exynos_usi_hsi2c_data = {
> +	.fifo_depth	= 64,
> +	.hw		= I2C_TYPE_USI,
> +};
> +
>  static const struct of_device_id exynos5_i2c_match[] = {
>  	{
>  		.compatible = "samsung,exynos5-hsi2c",
> @@ -243,6 +266,9 @@ static const struct of_device_id exynos5_i2c_match[] = {
>  	}, {
>  		.compatible = "samsung,exynos7-hsi2c",
>  		.data = &exynos7_hsi2c_data
> +	}, {
> +		.compatible = "samsung,exynos-usi-hsi2c",
> +		.data = &exynos_usi_hsi2c_data
>  	}, {},
>  };
>  MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
> @@ -281,6 +307,31 @@ 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 USI
> +	 * 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 genaral case, we don`t need to control timing_s1, timing_s2.

s/genaral/general/
(please run spellcheck)
s/don`t/don't/

> +	 */
> +	if (i2c->variant->hw == I2C_TYPE_USI) {
> +		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 +406,16 @@ 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)

The name of function suggests you are performing a reset but the code
looks like it is only clearing the reset flag. How about calling the
function exynos_usi_clear_reset()?

> +{
> +	u32 val;
> +
> +	/* Clear the software reset of USI block (it's set at startup) */
> +	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 +446,9 @@ static void exynos5_i2c_reset(struct exynos5_i2c *i2c)
>  {
>  	u32 i2c_ctl;
>  
> +	if (i2c->variant->hw == I2C_TYPE_USI)
> +		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 +486,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_USI) {
>  		if (int_status & HSI2C_INT_TRANS_DONE) {
>  			i2c->trans_done = 1;
>  			i2c->state = 0;
> @@ -569,13 +634,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 EXYNOS_USI 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 +676,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 +803,37 @@ 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->hw != I2C_TYPE_USI)
> +		return 0;
> +
> +	/* USI regmap control */

Drop the comment, it's obvious. What is missing here, is a comment
explaining what are you initializing exactly in the USI. Please add it.

> +	i2c->usi_sysreg = syscon_regmap_lookup_by_phandle(
> +			dev->of_node, "samsung,usi-sysreg");

Align the lines to opening parenthesis.

> +	if (IS_ERR(i2c->usi_sysreg)) {
> +		dev_err(dev, "Cannot find usi-sysreg\n");
> +		return PTR_ERR(i2c->usi_sysreg);
> +	}
> +
> +	ret = of_property_read_u32_index(dev->of_node,
> +				"samsung,usi-sysreg", 1, &i2c->usi_offset);

Align the lines to opening parenthesis.

Offset is not described in the bindings.

> +	if (ret) {
> +		dev_err(dev, "usi-sysreg offset is not specified\n");
> +		return ret;
> +	}
> +
> +	regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset,
> +			SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF);
> +
> +	exynos_usi_reset(i2c);

You are clearing the reset flag, but not setting it back on probe
failure. What happens if the probe fails after this clear()? E.g.
because of deferred probe? The next probe try will start on a not-reset
controller. Will it work?

> +
> +	return 0;
> +}
> +
>  static int exynos5_i2c_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -777,6 +873,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 +896,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;
> 


Best regards,
Krzysztof

RE: [PATCH 2/2] dt-bindings: i2c: exynos5: add exynos-usi-hsi2c compatible

[Jaewon Kim]

Hello Krzysztof

> On 01/11/2021 12:38, Jaewon Kim wrote:
> > This patch adds new "samsung,exynos-usi-hsi2c" compatible.
> > It is for i2c compatible with HSI2C available on Exynos SoC with USI.
> >
> > Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> > ---
> >  Documentation/devicetree/bindings/i2c/i2c-exynos5.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> 
> The bindings go as first patch, please.

Okay, I will change patch order in next.

> 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> > b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> > index 2dbc0b62daa6..ce2373c7a357 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,exynos-usi-hsi2c", for i2c compatible with HSI2C available
> > +				on Exynos SoCs with USI.
> 
> I would prefer to describe the Exynos model, not the feature. USI might change between different SoCs,
> so then it will be "usiv2"?
> 

Okay, I will use Exynos model instaed of feature name.
"samsung,exynosautov9-hsi2c"

> >
> >    - reg: physical base address of the controller and length of memory mapped
> >      region.
> > @@ -31,6 +33,8 @@ Optional properties:
> >         at 100khz.
> >      -> If specified, the bus operates in high-speed mode only if the
> >         clock-frequency is >= 1Mhz.
> > +  - samsung,usi-sysreg : sysreg handle to control USI type.
> > +    -> sysreg phandle for "samsung,exynos-usi-hsic" compatible.
> 
> s/sysreg/system registers controller/
> s/handle/phandle/
> 

Okay, I will change it.

> Please document to what is this phandle. To which block.
> 

Okay, I will add below description in next patch.

When using Exynos USI Block, it needs to select which type of Serial IPs(UART, SPI, I2C) to use with
system control register. So, it requires system register control and needs offset value of system register.

> Why it cannot be the existing generic samsung,sysreg?
> 

It is generic samsung,sysreg not for speical system register for USI.
I will change property name to "samsung,sysreg".

> >
> >  Example:
> >
> > @@ -46,6 +50,8 @@ hsi2c@12ca0000 {
> >  	#address-cells = <1>;
> >  	#size-cells = <0>;
> >
> > +	samsung,usi-sysreg = <&usi_sysreg 0x28>;
> 
> This does not look correct for this compatible. We should really convert the bindings to YAML...
> 

I will remove this property example.

> > +
> >  	s2mps11_pmic@66 {
> >  		compatible = "samsung,s2mps11-pmic";
> >  		reg = <0x66>;
> >
> 
> 
> Best regards,
> Krzysztof

Thanks
Jaewon Kim

RE: [PATCH 1/2] i2c: exynos5: support USI(Universal Serial Interface)

[Jaewon Kim]

Hello Krzysztof

> On 01/11/2021 12:38, 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 | 120
> > ++++++++++++++++++++++++++++---
> >  1 file changed, 110 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-exynos5.c
> > b/drivers/i2c/busses/i2c-exynos5.c
> > index 97d4f3ac0abd..f2dc7848f840 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				(1 << 0)
> 
> BIT()
> 

Okay, I will change it to BIT()

> > +
> > +/* SYSREG Register bit */
> > +#define SYSREG_USI_SW_CONF_MASK			(0x7 << 0)
> > +#define SYSREG_I2C_SW_CONF			(1u << 2)
> 
> BIT()
> 

Okay, I will change it to BIT()

> > +
> >  enum i2c_type_exynos {
> >  	I2C_TYPE_EXYNOS5,
> >  	I2C_TYPE_EXYNOS7,
> > +	I2C_TYPE_USI,
> >  };
> >
> >  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;
> >  };
> >
> >  /**
> > @@ -230,6 +248,11 @@ static const struct exynos_hsi2c_variant exynos7_hsi2c_data = {
> >  	.hw		= I2C_TYPE_EXYNOS7,
> >  };
> >
> > +static const struct exynos_hsi2c_variant exynos_usi_hsi2c_data = {
> > +	.fifo_depth	= 64,
> > +	.hw		= I2C_TYPE_USI,
> > +};
> > +
> >  static const struct of_device_id exynos5_i2c_match[] = {
> >  	{
> >  		.compatible = "samsung,exynos5-hsi2c", @@ -243,6 +266,9 @@ static
> > const struct of_device_id exynos5_i2c_match[] = {
> >  	}, {
> >  		.compatible = "samsung,exynos7-hsi2c",
> >  		.data = &exynos7_hsi2c_data
> > +	}, {
> > +		.compatible = "samsung,exynos-usi-hsi2c",
> > +		.data = &exynos_usi_hsi2c_data
> >  	}, {},
> >  };
> >  MODULE_DEVICE_TABLE(of, exynos5_i2c_match); @@ -281,6 +307,31 @@
> > 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 USI
> > +	 * 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 genaral case, we don`t need to control timing_s1, timing_s2.
> 
> s/genaral/general/
> (please run spellcheck)
> s/don`t/don't/
> 

Sorry, I will run spellcheck in next.

> > +	 */
> > +	if (i2c->variant->hw == I2C_TYPE_USI) {
> > +		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 +406,16 @@ 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)
> 
> The name of function suggests you are performing a reset but the code looks like it is only clearing
> the reset flag. How about calling the function exynos_usi_clear_reset()?
> 

Accroding to below review, I will add reset and clear code.

> > +{
> > +	u32 val;
> > +
> > +	/* Clear the software reset of USI block (it's set at startup) */
> > +	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 +446,9
> > @@ static void exynos5_i2c_reset(struct exynos5_i2c *i2c)  {
> >  	u32 i2c_ctl;
> >
> > +	if (i2c->variant->hw == I2C_TYPE_USI)
> > +		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 +486,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_USI) {
> >  		if (int_status & HSI2C_INT_TRANS_DONE) {
> >  			i2c->trans_done = 1;
> >  			i2c->state = 0;
> > @@ -569,13 +634,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 EXYNOS_USI 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 +676,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 +803,37 @@
> > 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->hw != I2C_TYPE_USI)
> > +		return 0;
> > +
> > +	/* USI regmap control */
> 
> Drop the comment, it's obvious. What is missing here, is a comment explaining what are you
> initializing exactly in the USI. Please add it.
> 

Okay, I will add detailed information.

> > +	i2c->usi_sysreg = syscon_regmap_lookup_by_phandle(
> > +			dev->of_node, "samsung,usi-sysreg");
> 
> Align the lines to opening parenthesis.
> 
> > +	if (IS_ERR(i2c->usi_sysreg)) {
> > +		dev_err(dev, "Cannot find usi-sysreg\n");
> > +		return PTR_ERR(i2c->usi_sysreg);
> > +	}
> > +
> > +	ret = of_property_read_u32_index(dev->of_node,
> > +				"samsung,usi-sysreg", 1, &i2c->usi_offset);
> 
> Align the lines to opening parenthesis.
> 

Okay.

> Offset is not described in the bindings.
> 

Okay, I will add offset description in bindings docs.

> > +	if (ret) {
> > +		dev_err(dev, "usi-sysreg offset is not specified\n");
> > +		return ret;
> > +	}
> > +
> > +	regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset,
> > +			SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF);
> > +
> > +	exynos_usi_reset(i2c);
> 
> You are clearing the reset flag, but not setting it back on probe failure. What happens if the probe
> fails after this clear()? E.g.
> because of deferred probe? The next probe try will start on a not-reset controller. Will it work?
> 

The user manual guides USI_RESET to be done after changing the system register.
For clarity, we will change not only to clear reset, but to clear after reset.

> > +
> > +	return 0;
> > +}
> > +
> >  static int exynos5_i2c_probe(struct platform_device *pdev)  {
> >  	struct device_node *np = pdev->dev.of_node; @@ -777,6 +873,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 +896,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;
> >
> 
> 
> Best regards,
> Krzysztof


Thanks
Jaewon Kim

Re: [PATCH 1/2] i2c: exynos5: support USI(Universal Serial Interface)

[Krzysztof Kozlowski]

On 04/11/2021 09:10, Jaewon Kim wrote:
> 
>>> +	if (ret) {
>>> +		dev_err(dev, "usi-sysreg offset is not specified\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset,
>>> +			SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF);
>>> +
>>> +	exynos_usi_reset(i2c);
>>
>> You are clearing the reset flag, but not setting it back on probe failure. What happens if the probe
>> fails after this clear()? E.g.
>> because of deferred probe? The next probe try will start on a not-reset controller. Will it work?
>>
> 
> The user manual guides USI_RESET to be done after changing the system register.
> For clarity, we will change not only to clear reset, but to clear after reset.
> 

What I meant, is do you handle probe failure correctly (e.g. probe
deferral)? It's fine to leave the reset cleared after deferred probe?

Best regards,
Krzysztof

RE: [PATCH 1/2] i2c: exynos5: support USI(Universal Serial Interface)

[Jaewon Kim]

Hello Krzysztof

> On 04/11/2021 09:10, Jaewon Kim wrote:
> >
> >>> +	if (ret) {
> >>> +		dev_err(dev, "usi-sysreg offset is not specified\n");
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset,
> >>> +			SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF);
> >>> +
> >>> +	exynos_usi_reset(i2c);
> >>
> >> You are clearing the reset flag, but not setting it back on probe
> >> failure. What happens if the probe fails after this clear()? E.g.
> >> because of deferred probe? The next probe try will start on a not-reset controller. Will it work?
> >>
> >
> > The user manual guides USI_RESET to be done after changing the system register.
> > For clarity, we will change not only to clear reset, but to clear after reset.
> >
> 
> What I meant, is do you handle probe failure correctly (e.g. probe deferral)? It's fine to leave the
> reset cleared after deferred probe?
> 

I understood.
In my opinion, rather than resetting USI_CON_RESET in the probe fail case.
It seems it is more clear to set the reset as shown below.

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


> Best regards,
> Krzysztof

Thanks
Jaewon Kim