[PATCH v2 0/2] Enable Samsung I2C drivers on newer platforms

[PATCH v2 0/2] Enable Samsung I2C drivers on newer platforms

[David Virag]

This set of patches should enable the Samsung I2C drivers to work on
platforms other than EXYNOS4/EXYNOS5.

This has been tested on Exynos7885 with the S3C I2C driver.
With the clocks for it implemented in it's driver, this should also
enable S3C I2C to work on Exynos850.

While at it, clean up some dead code as well.

Changes in v2:
- Fix compile on Exynos4/Exynos5 platform

David Virag (2):
  i2c: samsung: Drop s3c24x0 specific code.
  i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5

 drivers/i2c/Kconfig         |  2 +-
 drivers/i2c/exynos_hs_i2c.c | 25 +++++++++++++++++++++----
 drivers/i2c/s3c24x0_i2c.c   | 32 ++++++++++++++++++++++++--------
 drivers/i2c/s3c24x0_i2c.h   |  2 ++
 4 files changed, 48 insertions(+), 13 deletions(-)

-- 
2.46.0

[PATCH v2 1/2] i2c: samsung: Drop s3c24x0 specific code.

[David Virag]

This has been dead code for many years now. Remove it.

Signed-off-by: David Virag <virag.david003@gmail.com>
---
 drivers/i2c/exynos_hs_i2c.c | 4 ----
 drivers/i2c/s3c24x0_i2c.c   | 8 --------
 2 files changed, 12 deletions(-)

diff --git a/drivers/i2c/exynos_hs_i2c.c b/drivers/i2c/exynos_hs_i2c.c
index 2ab0bae449..189ce6d509 100644
--- a/drivers/i2c/exynos_hs_i2c.c
+++ b/drivers/i2c/exynos_hs_i2c.c
@@ -145,11 +145,7 @@ static int hsi2c_get_clk_details(struct s3c24x0_i2c_bus *i2c_bus)
 	unsigned int i = 0, utemp0 = 0, utemp1 = 0;
 	unsigned int t_ftl_cycle;
 
-#if defined(CONFIG_ARCH_EXYNOS4) || defined(CONFIG_ARCH_EXYNOS5)
 	clkin = get_i2c_clk();
-#else
-	clkin = get_PCLK();
-#endif
 	/* FPCLK / FI2C =
 	 * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 * FLT_CYCLE
 	 * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
index 72d2ab0f73..ae3a801cad 100644
--- a/drivers/i2c/s3c24x0_i2c.c
+++ b/drivers/i2c/s3c24x0_i2c.c
@@ -8,14 +8,10 @@
 #include <dm.h>
 #include <fdtdec.h>
 #include <time.h>
-#if defined(CONFIG_ARCH_EXYNOS4) || defined(CONFIG_ARCH_EXYNOS5)
 #include <log.h>
 #include <asm/arch/clk.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/pinmux.h>
-#else
-#include <asm/arch/s3c24x0_cpu.h>
-#endif
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <i2c.h>
@@ -53,11 +49,7 @@ static void read_write_byte(struct s3c24x0_i2c *i2c)
 static void i2c_ch_init(struct s3c24x0_i2c *i2c, int speed, int slaveadd)
 {
 	ulong freq, pres = 16, div;
-#if defined(CONFIG_ARCH_EXYNOS4) || defined(CONFIG_ARCH_EXYNOS5)
 	freq = get_i2c_clk();
-#else
-	freq = get_PCLK();
-#endif
 	/* calculate prescaler and divisor values */
 	if ((freq / pres / (16 + 1)) > speed)
 		/* set prescaler to 512 */
-- 
2.46.0

[PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5

[David Virag]

Newer Samsung SoCs (including newer Exynos, ExynosAuto, Google Tensor)
still use these IPs, or slightly newer versions of it.

Make these drivers available on these platforms by guarding
EXYNOS4/EXYNOS5 specific code behind their configs, and using CCF for
clocks on other platforms.

Tested S3C I2C driver on Exynos7885.
This along with extended clock driver should enable S3C I2C on
Exynos850.

Signed-off-by: David Virag <virag.david003@gmail.com>
---
 drivers/i2c/Kconfig         |  2 +-
 drivers/i2c/exynos_hs_i2c.c | 25 +++++++++++++++++++++++--
 drivers/i2c/s3c24x0_i2c.c   | 30 +++++++++++++++++++++++++++---
 drivers/i2c/s3c24x0_i2c.h   |  2 ++
 4 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index cba7f84894..52067fa7c1 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -650,7 +650,7 @@ config SYS_I2C_GENI
 
 config SYS_I2C_S3C24X0
 	bool "Samsung I2C driver"
-	depends on (ARCH_EXYNOS4 || ARCH_EXYNOS5) && DM_I2C
+	depends on DM_I2C
 	help
 	  Support for Samsung I2C controller as Samsung SoCs.
 
diff --git a/drivers/i2c/exynos_hs_i2c.c b/drivers/i2c/exynos_hs_i2c.c
index 189ce6d509..fa0d1c8f64 100644
--- a/drivers/i2c/exynos_hs_i2c.c
+++ b/drivers/i2c/exynos_hs_i2c.c
@@ -9,11 +9,15 @@
 #include <dm.h>
 #include <i2c.h>
 #include <log.h>
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 #include <asm/arch/clk.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/pinmux.h>
+#endif
 #include <asm/global_data.h>
+#include <asm/io.h>
 #include <linux/delay.h>
+#include <clk.h>
 #include "s3c24x0_i2c.h"
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -137,15 +141,26 @@ static int hsi2c_wait_for_trx(struct exynos5_hsi2c *i2c)
 	return I2C_NOK_TOUT;
 }
 
-static int hsi2c_get_clk_details(struct s3c24x0_i2c_bus *i2c_bus)
+static int hsi2c_get_clk_details(struct udevice *dev)
 {
+	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
 	struct exynos5_hsi2c *hsregs = i2c_bus->hsregs;
 	ulong clkin;
 	unsigned int op_clk = i2c_bus->clock_frequency;
 	unsigned int i = 0, utemp0 = 0, utemp1 = 0;
 	unsigned int t_ftl_cycle;
 
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 	clkin = get_i2c_clk();
+#else
+	struct clk clk;
+	int ret;
+
+	ret = clk_get_by_name(dev, "hsi2c", &clk);
+	if (ret < 0)
+		return ret;
+	clkin = clk_get_rate(&clk);
+#endif
 	/* FPCLK / FI2C =
 	 * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 * FLT_CYCLE
 	 * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
@@ -487,7 +502,7 @@ static int s3c24x0_i2c_set_bus_speed(struct udevice *dev, unsigned int speed)
 
 	i2c_bus->clock_frequency = speed;
 
-	if (hsi2c_get_clk_details(i2c_bus))
+	if (hsi2c_get_clk_details(dev))
 		return -EFAULT;
 	hsi2c_ch_init(i2c_bus);
 
@@ -514,7 +529,9 @@ static int s3c24x0_i2c_probe(struct udevice *dev, uint chip, uint chip_flags)
 
 static int s3c_i2c_of_to_plat(struct udevice *dev)
 {
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 	const void *blob = gd->fdt_blob;
+#endif
 	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
 	int node;
 
@@ -522,7 +539,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
 
 	i2c_bus->hsregs = dev_read_addr_ptr(dev);
 
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 	i2c_bus->id = pinmux_decode_periph_id(blob, node);
+#endif
 
 	i2c_bus->clock_frequency =
 		dev_read_u32_default(dev, "clock-frequency",
@@ -530,7 +549,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
 	i2c_bus->node = node;
 	i2c_bus->bus_num = dev_seq(dev);
 
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 	exynos_pinmux_config(i2c_bus->id, PINMUX_FLAG_HS_MODE);
+#endif
 
 	i2c_bus->active = true;
 
diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
index ae3a801cad..ade1ad6cef 100644
--- a/drivers/i2c/s3c24x0_i2c.c
+++ b/drivers/i2c/s3c24x0_i2c.c
@@ -9,12 +9,15 @@
 #include <fdtdec.h>
 #include <time.h>
 #include <log.h>
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 #include <asm/arch/clk.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/pinmux.h>
+#endif
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <i2c.h>
+#include <clk.h>
 #include "s3c24x0_i2c.h"
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -46,10 +49,23 @@ static void read_write_byte(struct s3c24x0_i2c *i2c)
 	clrbits_le32(&i2c->iiccon, I2CCON_IRPND);
 }
 
-static void i2c_ch_init(struct s3c24x0_i2c *i2c, int speed, int slaveadd)
+static int i2c_ch_init(struct udevice *dev, int speed, int slaveadd)
 {
+	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
+	struct s3c24x0_i2c *i2c = i2c_bus->regs;
 	ulong freq, pres = 16, div;
+
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || defined(CONFIG_ARCH_EXYNOS5)
 	freq = get_i2c_clk();
+#else
+	struct clk clk;
+	int ret;
+
+	ret = clk_get_by_name(dev, "i2c", &clk);
+	if (ret < 0)
+		return ret;
+	freq = clk_get_rate(&clk);
+#endif
 	/* calculate prescaler and divisor values */
 	if ((freq / pres / (16 + 1)) > speed)
 		/* set prescaler to 512 */
@@ -67,6 +83,7 @@ static void i2c_ch_init(struct s3c24x0_i2c *i2c, int speed, int slaveadd)
 	writel(slaveadd, &i2c->iicadd);
 	/* program Master Transmit (and implicit STOP) */
 	writel(I2C_MODE_MT | I2C_TXRX_ENA, &i2c->iicstat);
+	return 0;
 }
 
 #define SYS_I2C_S3C24X0_SLAVE_ADDR	0
@@ -77,8 +94,9 @@ static int s3c24x0_i2c_set_bus_speed(struct udevice *dev, unsigned int speed)
 
 	i2c_bus->clock_frequency = speed;
 
-	i2c_ch_init(i2c_bus->regs, i2c_bus->clock_frequency,
-		    SYS_I2C_S3C24X0_SLAVE_ADDR);
+	if (i2c_ch_init(dev, i2c_bus->clock_frequency,
+			SYS_I2C_S3C24X0_SLAVE_ADDR))
+		return -EFAULT;
 
 	return 0;
 }
@@ -293,7 +311,9 @@ static int s3c24x0_i2c_xfer(struct udevice *dev, struct i2c_msg *msg,
 
 static int s3c_i2c_of_to_plat(struct udevice *dev)
 {
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 	const void *blob = gd->fdt_blob;
+#endif
 	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
 	int node;
 
@@ -301,7 +321,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
 
 	i2c_bus->regs = dev_read_addr_ptr(dev);
 
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 	i2c_bus->id = pinmux_decode_periph_id(blob, node);
+#endif
 
 	i2c_bus->clock_frequency =
 		dev_read_u32_default(dev, "clock-frequency",
@@ -309,7 +331,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
 	i2c_bus->node = node;
 	i2c_bus->bus_num = dev_seq(dev);
 
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 	exynos_pinmux_config(i2c_bus->id, 0);
+#endif
 
 	i2c_bus->active = true;
 
diff --git a/drivers/i2c/s3c24x0_i2c.h b/drivers/i2c/s3c24x0_i2c.h
index ec8f1acaef..12249d5c14 100644
--- a/drivers/i2c/s3c24x0_i2c.h
+++ b/drivers/i2c/s3c24x0_i2c.h
@@ -54,7 +54,9 @@ struct s3c24x0_i2c_bus {
 	struct exynos5_hsi2c *hsregs;
 	int is_highspeed;	/* High speed type, rather than I2C */
 	unsigned clock_frequency;
+#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
 	int id;
+#endif
 	unsigned clk_cycle;
 	unsigned clk_div;
 };
-- 
2.46.0

Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5

[Henrik Grimler]

Hi David,

On Fri, Aug 02, 2024 at 09:19:16PM +0200, David Virag wrote:
> Newer Samsung SoCs (including newer Exynos, ExynosAuto, Google Tensor)
> still use these IPs, or slightly newer versions of it.
> 
> Make these drivers available on these platforms by guarding
> EXYNOS4/EXYNOS5 specific code behind their configs, and using CCF for
> clocks on other platforms.
> 
> Tested S3C I2C driver on Exynos7885.
> This along with extended clock driver should enable S3C I2C on
> Exynos850.
> 
> Signed-off-by: David Virag <virag.david003@gmail.com>

Tested-by: Henrik Grimler <henrik@grimler.se>

Tested on exynos4412-odroid-u2 and exynos5422-odroid-xu4.  On both
devices i2c still works fine, thanks!

Best regards,
Henrik Grimler

> ---
>  drivers/i2c/Kconfig         |  2 +-
>  drivers/i2c/exynos_hs_i2c.c | 25 +++++++++++++++++++++++--
>  drivers/i2c/s3c24x0_i2c.c   | 30 +++++++++++++++++++++++++++---
>  drivers/i2c/s3c24x0_i2c.h   |  2 ++
>  4 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index cba7f84894..52067fa7c1 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -650,7 +650,7 @@ config SYS_I2C_GENI
>  
>  config SYS_I2C_S3C24X0
>  	bool "Samsung I2C driver"
> -	depends on (ARCH_EXYNOS4 || ARCH_EXYNOS5) && DM_I2C
> +	depends on DM_I2C
>  	help
>  	  Support for Samsung I2C controller as Samsung SoCs.
>  
> diff --git a/drivers/i2c/exynos_hs_i2c.c b/drivers/i2c/exynos_hs_i2c.c
> index 189ce6d509..fa0d1c8f64 100644
> --- a/drivers/i2c/exynos_hs_i2c.c
> +++ b/drivers/i2c/exynos_hs_i2c.c
> @@ -9,11 +9,15 @@
>  #include <dm.h>
>  #include <i2c.h>
>  #include <log.h>
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  #include <asm/arch/clk.h>
>  #include <asm/arch/cpu.h>
>  #include <asm/arch/pinmux.h>
> +#endif
>  #include <asm/global_data.h>
> +#include <asm/io.h>
>  #include <linux/delay.h>
> +#include <clk.h>
>  #include "s3c24x0_i2c.h"
>  
>  DECLARE_GLOBAL_DATA_PTR;
> @@ -137,15 +141,26 @@ static int hsi2c_wait_for_trx(struct exynos5_hsi2c *i2c)
>  	return I2C_NOK_TOUT;
>  }
>  
> -static int hsi2c_get_clk_details(struct s3c24x0_i2c_bus *i2c_bus)
> +static int hsi2c_get_clk_details(struct udevice *dev)
>  {
> +	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
>  	struct exynos5_hsi2c *hsregs = i2c_bus->hsregs;
>  	ulong clkin;
>  	unsigned int op_clk = i2c_bus->clock_frequency;
>  	unsigned int i = 0, utemp0 = 0, utemp1 = 0;
>  	unsigned int t_ftl_cycle;
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	clkin = get_i2c_clk();
> +#else
> +	struct clk clk;
> +	int ret;
> +
> +	ret = clk_get_by_name(dev, "hsi2c", &clk);
> +	if (ret < 0)
> +		return ret;
> +	clkin = clk_get_rate(&clk);
> +#endif
>  	/* FPCLK / FI2C =
>  	 * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 * FLT_CYCLE
>  	 * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
> @@ -487,7 +502,7 @@ static int s3c24x0_i2c_set_bus_speed(struct udevice *dev, unsigned int speed)
>  
>  	i2c_bus->clock_frequency = speed;
>  
> -	if (hsi2c_get_clk_details(i2c_bus))
> +	if (hsi2c_get_clk_details(dev))
>  		return -EFAULT;
>  	hsi2c_ch_init(i2c_bus);
>  
> @@ -514,7 +529,9 @@ static int s3c24x0_i2c_probe(struct udevice *dev, uint chip, uint chip_flags)
>  
>  static int s3c_i2c_of_to_plat(struct udevice *dev)
>  {
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	const void *blob = gd->fdt_blob;
> +#endif
>  	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
>  	int node;
>  
> @@ -522,7 +539,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
>  
>  	i2c_bus->hsregs = dev_read_addr_ptr(dev);
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	i2c_bus->id = pinmux_decode_periph_id(blob, node);
> +#endif
>  
>  	i2c_bus->clock_frequency =
>  		dev_read_u32_default(dev, "clock-frequency",
> @@ -530,7 +549,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
>  	i2c_bus->node = node;
>  	i2c_bus->bus_num = dev_seq(dev);
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	exynos_pinmux_config(i2c_bus->id, PINMUX_FLAG_HS_MODE);
> +#endif
>  
>  	i2c_bus->active = true;
>  
> diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
> index ae3a801cad..ade1ad6cef 100644
> --- a/drivers/i2c/s3c24x0_i2c.c
> +++ b/drivers/i2c/s3c24x0_i2c.c
> @@ -9,12 +9,15 @@
>  #include <fdtdec.h>
>  #include <time.h>
>  #include <log.h>
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  #include <asm/arch/clk.h>
>  #include <asm/arch/cpu.h>
>  #include <asm/arch/pinmux.h>
> +#endif
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <i2c.h>
> +#include <clk.h>
>  #include "s3c24x0_i2c.h"
>  
>  DECLARE_GLOBAL_DATA_PTR;
> @@ -46,10 +49,23 @@ static void read_write_byte(struct s3c24x0_i2c *i2c)
>  	clrbits_le32(&i2c->iiccon, I2CCON_IRPND);
>  }
>  
> -static void i2c_ch_init(struct s3c24x0_i2c *i2c, int speed, int slaveadd)
> +static int i2c_ch_init(struct udevice *dev, int speed, int slaveadd)
>  {
> +	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
> +	struct s3c24x0_i2c *i2c = i2c_bus->regs;
>  	ulong freq, pres = 16, div;
> +
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || defined(CONFIG_ARCH_EXYNOS5)
>  	freq = get_i2c_clk();
> +#else
> +	struct clk clk;
> +	int ret;
> +
> +	ret = clk_get_by_name(dev, "i2c", &clk);
> +	if (ret < 0)
> +		return ret;
> +	freq = clk_get_rate(&clk);
> +#endif
>  	/* calculate prescaler and divisor values */
>  	if ((freq / pres / (16 + 1)) > speed)
>  		/* set prescaler to 512 */
> @@ -67,6 +83,7 @@ static void i2c_ch_init(struct s3c24x0_i2c *i2c, int speed, int slaveadd)
>  	writel(slaveadd, &i2c->iicadd);
>  	/* program Master Transmit (and implicit STOP) */
>  	writel(I2C_MODE_MT | I2C_TXRX_ENA, &i2c->iicstat);
> +	return 0;
>  }
>  
>  #define SYS_I2C_S3C24X0_SLAVE_ADDR	0
> @@ -77,8 +94,9 @@ static int s3c24x0_i2c_set_bus_speed(struct udevice *dev, unsigned int speed)
>  
>  	i2c_bus->clock_frequency = speed;
>  
> -	i2c_ch_init(i2c_bus->regs, i2c_bus->clock_frequency,
> -		    SYS_I2C_S3C24X0_SLAVE_ADDR);
> +	if (i2c_ch_init(dev, i2c_bus->clock_frequency,
> +			SYS_I2C_S3C24X0_SLAVE_ADDR))
> +		return -EFAULT;
>  
>  	return 0;
>  }
> @@ -293,7 +311,9 @@ static int s3c24x0_i2c_xfer(struct udevice *dev, struct i2c_msg *msg,
>  
>  static int s3c_i2c_of_to_plat(struct udevice *dev)
>  {
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	const void *blob = gd->fdt_blob;
> +#endif
>  	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
>  	int node;
>  
> @@ -301,7 +321,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
>  
>  	i2c_bus->regs = dev_read_addr_ptr(dev);
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	i2c_bus->id = pinmux_decode_periph_id(blob, node);
> +#endif
>  
>  	i2c_bus->clock_frequency =
>  		dev_read_u32_default(dev, "clock-frequency",
> @@ -309,7 +331,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
>  	i2c_bus->node = node;
>  	i2c_bus->bus_num = dev_seq(dev);
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	exynos_pinmux_config(i2c_bus->id, 0);
> +#endif
>  
>  	i2c_bus->active = true;
>  
> diff --git a/drivers/i2c/s3c24x0_i2c.h b/drivers/i2c/s3c24x0_i2c.h
> index ec8f1acaef..12249d5c14 100644
> --- a/drivers/i2c/s3c24x0_i2c.h
> +++ b/drivers/i2c/s3c24x0_i2c.h
> @@ -54,7 +54,9 @@ struct s3c24x0_i2c_bus {
>  	struct exynos5_hsi2c *hsregs;
>  	int is_highspeed;	/* High speed type, rather than I2C */
>  	unsigned clock_frequency;
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	int id;
> +#endif
>  	unsigned clk_cycle;
>  	unsigned clk_div;
>  };
> -- 
> 2.46.0
> 

Re: [PATCH v2 1/2] i2c: samsung: Drop s3c24x0 specific code.

[Heiko Schocher]

Hello David,

On 02.08.24 21:19, David Virag wrote:
> This has been dead code for many years now. Remove it.
> 
> Signed-off-by: David Virag <virag.david003@gmail.com>
> ---
>   drivers/i2c/exynos_hs_i2c.c | 4 ----
>   drivers/i2c/s3c24x0_i2c.c   | 8 --------
>   2 files changed, 12 deletions(-)

Reviewed-by: Heiko Schocher <hs@denx.de>

Thanks!

bye,
Heiko

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5

[Heiko Schocher]

Hello David,

On 02.08.24 21:19, David Virag wrote:
> Newer Samsung SoCs (including newer Exynos, ExynosAuto, Google Tensor)
> still use these IPs, or slightly newer versions of it.
> 
> Make these drivers available on these platforms by guarding
> EXYNOS4/EXYNOS5 specific code behind their configs, and using CCF for
> clocks on other platforms.
> 
> Tested S3C I2C driver on Exynos7885.
> This along with extended clock driver should enable S3C I2C on
> Exynos850.
> 
> Signed-off-by: David Virag <virag.david003@gmail.com>
> ---
>   drivers/i2c/Kconfig         |  2 +-
>   drivers/i2c/exynos_hs_i2c.c | 25 +++++++++++++++++++++++--
>   drivers/i2c/s3c24x0_i2c.c   | 30 +++++++++++++++++++++++++++---
>   drivers/i2c/s3c24x0_i2c.h   |  2 ++
>   4 files changed, 53 insertions(+), 6 deletions(-)

Reviewed-by: Heiko Schocher <hs@denx.de>

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5

[Henrik Grimler]

Hi David,

Thinking about this a bit more...

On Fri, Aug 02, 2024 at 09:19:16PM +0200, David Virag wrote:
> Newer Samsung SoCs (including newer Exynos, ExynosAuto, Google Tensor)
> still use these IPs, or slightly newer versions of it.
> 
> Make these drivers available on these platforms by guarding
> EXYNOS4/EXYNOS5 specific code behind their configs, and using CCF for
> clocks on other platforms.
> 
> Tested S3C I2C driver on Exynos7885.
> This along with extended clock driver should enable S3C I2C on
> Exynos850.
> 
> Signed-off-by: David Virag <virag.david003@gmail.com>
> ---
>  drivers/i2c/Kconfig         |  2 +-
>  drivers/i2c/exynos_hs_i2c.c | 25 +++++++++++++++++++++++--
>  drivers/i2c/s3c24x0_i2c.c   | 30 +++++++++++++++++++++++++++---
>  drivers/i2c/s3c24x0_i2c.h   |  2 ++
>  4 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index cba7f84894..52067fa7c1 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -650,7 +650,7 @@ config SYS_I2C_GENI
>  
>  config SYS_I2C_S3C24X0
>  	bool "Samsung I2C driver"
> -	depends on (ARCH_EXYNOS4 || ARCH_EXYNOS5) && DM_I2C
> +	depends on DM_I2C
>  	help
>  	  Support for Samsung I2C controller as Samsung SoCs.
>  
> diff --git a/drivers/i2c/exynos_hs_i2c.c b/drivers/i2c/exynos_hs_i2c.c
> index 189ce6d509..fa0d1c8f64 100644
> --- a/drivers/i2c/exynos_hs_i2c.c
> +++ b/drivers/i2c/exynos_hs_i2c.c
> @@ -9,11 +9,15 @@
>  #include <dm.h>
>  #include <i2c.h>
>  #include <log.h>
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  #include <asm/arch/clk.h>
>  #include <asm/arch/cpu.h>
>  #include <asm/arch/pinmux.h>
> +#endif

We want to try to move the Exynos 4 and 5 devices to CCF and
OF_UPSTREAM as well, which will make this messy.  Can we check
CONFIG_CLK_EXYNOS and CONFIG_PINCTRL_EXYNOS instead?

>  #include <asm/global_data.h>
> +#include <asm/io.h>
>  #include <linux/delay.h>
> +#include <clk.h>
>  #include "s3c24x0_i2c.h"
>  
>  DECLARE_GLOBAL_DATA_PTR;
> @@ -137,15 +141,26 @@ static int hsi2c_wait_for_trx(struct exynos5_hsi2c *i2c)
>  	return I2C_NOK_TOUT;
>  }
>  
> -static int hsi2c_get_clk_details(struct s3c24x0_i2c_bus *i2c_bus)
> +static int hsi2c_get_clk_details(struct udevice *dev)
>  {
> +	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
>  	struct exynos5_hsi2c *hsregs = i2c_bus->hsregs;
>  	ulong clkin;
>  	unsigned int op_clk = i2c_bus->clock_frequency;
>  	unsigned int i = 0, utemp0 = 0, utemp1 = 0;
>  	unsigned int t_ftl_cycle;
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)

As above, can we check CONFIG_CLK_EXYNOS instead?

>  	clkin = get_i2c_clk();
> +#else
> +	struct clk clk;
> +	int ret;
> +
> +	ret = clk_get_by_name(dev, "hsi2c", &clk);
> +	if (ret < 0)
> +		return ret;
> +	clkin = clk_get_rate(&clk);
> +#endif
>  	/* FPCLK / FI2C =
>  	 * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 * FLT_CYCLE
>  	 * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
> @@ -487,7 +502,7 @@ static int s3c24x0_i2c_set_bus_speed(struct udevice *dev, unsigned int speed)
>  
>  	i2c_bus->clock_frequency = speed;
>  
> -	if (hsi2c_get_clk_details(i2c_bus))
> +	if (hsi2c_get_clk_details(dev))
>  		return -EFAULT;
>  	hsi2c_ch_init(i2c_bus);
>  
> @@ -514,7 +529,9 @@ static int s3c24x0_i2c_probe(struct udevice *dev, uint chip, uint chip_flags)
>  
>  static int s3c_i2c_of_to_plat(struct udevice *dev)
>  {
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	const void *blob = gd->fdt_blob;
> +#endif
>  	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
>  	int node;
>  
> @@ -522,7 +539,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
>  
>  	i2c_bus->hsregs = dev_read_addr_ptr(dev);
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	i2c_bus->id = pinmux_decode_periph_id(blob, node);
> +#endif
>  
>  	i2c_bus->clock_frequency =
>  		dev_read_u32_default(dev, "clock-frequency",
> @@ -530,7 +549,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
>  	i2c_bus->node = node;
>  	i2c_bus->bus_num = dev_seq(dev);
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	exynos_pinmux_config(i2c_bus->id, PINMUX_FLAG_HS_MODE);
> +#endif

I think these three #if def's can be consolidated in one #if def group
towards the end of s3c_i2c_of_to_plat to make it easier to follow.

Also, can we check CONFIG_PINCTRL_EXYNOS instead?  That would make it
easier when migrating Exynos 4 and 5 devices to pinctrl driver and
OF_UPSTREAM.

Same comments apply to s3c24x0_i2c.c below.

Best regards,
Henrik Grimler

>  	i2c_bus->active = true;
>  
> diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
> index ae3a801cad..ade1ad6cef 100644
> --- a/drivers/i2c/s3c24x0_i2c.c
> +++ b/drivers/i2c/s3c24x0_i2c.c
> @@ -9,12 +9,15 @@
>  #include <fdtdec.h>
>  #include <time.h>
>  #include <log.h>
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  #include <asm/arch/clk.h>
>  #include <asm/arch/cpu.h>
>  #include <asm/arch/pinmux.h>
> +#endif
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <i2c.h>
> +#include <clk.h>
>  #include "s3c24x0_i2c.h"
>  
>  DECLARE_GLOBAL_DATA_PTR;
> @@ -46,10 +49,23 @@ static void read_write_byte(struct s3c24x0_i2c *i2c)
>  	clrbits_le32(&i2c->iiccon, I2CCON_IRPND);
>  }
>  
> -static void i2c_ch_init(struct s3c24x0_i2c *i2c, int speed, int slaveadd)
> +static int i2c_ch_init(struct udevice *dev, int speed, int slaveadd)
>  {
> +	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
> +	struct s3c24x0_i2c *i2c = i2c_bus->regs;
>  	ulong freq, pres = 16, div;
> +
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || defined(CONFIG_ARCH_EXYNOS5)
>  	freq = get_i2c_clk();
> +#else
> +	struct clk clk;
> +	int ret;
> +
> +	ret = clk_get_by_name(dev, "i2c", &clk);
> +	if (ret < 0)
> +		return ret;
> +	freq = clk_get_rate(&clk);
> +#endif
>  	/* calculate prescaler and divisor values */
>  	if ((freq / pres / (16 + 1)) > speed)
>  		/* set prescaler to 512 */
> @@ -67,6 +83,7 @@ static void i2c_ch_init(struct s3c24x0_i2c *i2c, int speed, int slaveadd)
>  	writel(slaveadd, &i2c->iicadd);
>  	/* program Master Transmit (and implicit STOP) */
>  	writel(I2C_MODE_MT | I2C_TXRX_ENA, &i2c->iicstat);
> +	return 0;
>  }
>  
>  #define SYS_I2C_S3C24X0_SLAVE_ADDR	0
> @@ -77,8 +94,9 @@ static int s3c24x0_i2c_set_bus_speed(struct udevice *dev, unsigned int speed)
>  
>  	i2c_bus->clock_frequency = speed;
>  
> -	i2c_ch_init(i2c_bus->regs, i2c_bus->clock_frequency,
> -		    SYS_I2C_S3C24X0_SLAVE_ADDR);
> +	if (i2c_ch_init(dev, i2c_bus->clock_frequency,
> +			SYS_I2C_S3C24X0_SLAVE_ADDR))
> +		return -EFAULT;
>  
>  	return 0;
>  }
> @@ -293,7 +311,9 @@ static int s3c24x0_i2c_xfer(struct udevice *dev, struct i2c_msg *msg,
>  
>  static int s3c_i2c_of_to_plat(struct udevice *dev)
>  {
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	const void *blob = gd->fdt_blob;
> +#endif
>  	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
>  	int node;
>  
> @@ -301,7 +321,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
>  
>  	i2c_bus->regs = dev_read_addr_ptr(dev);
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	i2c_bus->id = pinmux_decode_periph_id(blob, node);
> +#endif
>  
>  	i2c_bus->clock_frequency =
>  		dev_read_u32_default(dev, "clock-frequency",
> @@ -309,7 +331,9 @@ static int s3c_i2c_of_to_plat(struct udevice *dev)
>  	i2c_bus->node = node;
>  	i2c_bus->bus_num = dev_seq(dev);
>  
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	exynos_pinmux_config(i2c_bus->id, 0);
> +#endif
>  
>  	i2c_bus->active = true;
>  
> diff --git a/drivers/i2c/s3c24x0_i2c.h b/drivers/i2c/s3c24x0_i2c.h
> index ec8f1acaef..12249d5c14 100644
> --- a/drivers/i2c/s3c24x0_i2c.h
> +++ b/drivers/i2c/s3c24x0_i2c.h
> @@ -54,7 +54,9 @@ struct s3c24x0_i2c_bus {
>  	struct exynos5_hsi2c *hsregs;
>  	int is_highspeed;	/* High speed type, rather than I2C */
>  	unsigned clock_frequency;
> +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) || IS_ENABLED(CONFIG_ARCH_EXYNOS5)
>  	int id;
> +#endif
>  	unsigned clk_cycle;
>  	unsigned clk_div;
>  };
> -- 
> 2.46.0
> 

Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5

[David Virag]

Hi Henrik,

On Fri, 2024-08-09 at 23:08 +0200, Henrik Grimler wrote:
> Hi David,
> 
> Thinking about this a bit more...
> 
> On Fri, Aug 02, 2024 at 09:19:16PM +0200, David Virag wrote:
[snip]
> > @@ -9,11 +9,15 @@
> >  #include <dm.h>
> >  #include <i2c.h>
> >  #include <log.h>
> > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> >  #include <asm/arch/clk.h>
> >  #include <asm/arch/cpu.h>
> >  #include <asm/arch/pinmux.h>
> > +#endif
> 
> We want to try to move the Exynos 4 and 5 devices to CCF and
> OF_UPSTREAM as well, which will make this messy.  Can we check
> CONFIG_CLK_EXYNOS and CONFIG_PINCTRL_EXYNOS instead?
> 

Happy to hear that move, but you are a bit late, this got merged into
master not long ago. Sending a patch for changing it should be fine,
the 7885 port I'm working on uses mostly the same things as the E850-
96board (just has some support for 2 boards right now, though I may
submit jackpotlte only first, will see)

> >  #include <asm/global_data.h>
> > +#include <asm/io.h>
> >  #include <linux/delay.h>
> > +#include <clk.h>
> >  #include "s3c24x0_i2c.h"
> >  
> >  DECLARE_GLOBAL_DATA_PTR;
> > @@ -137,15 +141,26 @@ static int hsi2c_wait_for_trx(struct
> > exynos5_hsi2c *i2c)
> >  	return I2C_NOK_TOUT;
> >  }
> >  
> > -static int hsi2c_get_clk_details(struct s3c24x0_i2c_bus *i2c_bus)
> > +static int hsi2c_get_clk_details(struct udevice *dev)
> >  {
> > +	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
> >  	struct exynos5_hsi2c *hsregs = i2c_bus->hsregs;
> >  	ulong clkin;
> >  	unsigned int op_clk = i2c_bus->clock_frequency;
> >  	unsigned int i = 0, utemp0 = 0, utemp1 = 0;
> >  	unsigned int t_ftl_cycle;
> >  
> > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> 
> As above, can we check CONFIG_CLK_EXYNOS instead?
> 

Should be fine

> >  	clkin = get_i2c_clk();
> > +#else
> > +	struct clk clk;
> > +	int ret;
> > +
> > +	ret = clk_get_by_name(dev, "hsi2c", &clk);
> > +	if (ret < 0)
> > +		return ret;
> > +	clkin = clk_get_rate(&clk);
> > +#endif
> >  	/* FPCLK / FI2C =
> >  	 * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 *
> > FLT_CYCLE
> >  	 * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
> > @@ -487,7 +502,7 @@ static int s3c24x0_i2c_set_bus_speed(struct
> > udevice *dev, unsigned int speed)
> >  
> >  	i2c_bus->clock_frequency = speed;
> >  
> > -	if (hsi2c_get_clk_details(i2c_bus))
> > +	if (hsi2c_get_clk_details(dev))
> >  		return -EFAULT;
> >  	hsi2c_ch_init(i2c_bus);
> >  
> > @@ -514,7 +529,9 @@ static int s3c24x0_i2c_probe(struct udevice
> > *dev, uint chip, uint chip_flags)
> >  
> >  static int s3c_i2c_of_to_plat(struct udevice *dev)
> >  {
> > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> >  	const void *blob = gd->fdt_blob;
> > +#endif
> >  	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
> >  	int node;
> >  
> > @@ -522,7 +539,9 @@ static int s3c_i2c_of_to_plat(struct udevice
> > *dev)
> >  
> >  	i2c_bus->hsregs = dev_read_addr_ptr(dev);
> >  
> > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> >  	i2c_bus->id = pinmux_decode_periph_id(blob, node);
> > +#endif
> >  
> >  	i2c_bus->clock_frequency =
> >  		dev_read_u32_default(dev, "clock-frequency",
> > @@ -530,7 +549,9 @@ static int s3c_i2c_of_to_plat(struct udevice
> > *dev)
> >  	i2c_bus->node = node;
> >  	i2c_bus->bus_num = dev_seq(dev);
> >  
> > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> >  	exynos_pinmux_config(i2c_bus->id, PINMUX_FLAG_HS_MODE);
> > +#endif
> 
> I think these three #if def's can be consolidated in one #if def
> group
> towards the end of s3c_i2c_of_to_plat to make it easier to follow.

Hm, sounds good, though I don't know if moving the variable declaration
from the top would work. I don't know which C standard uboot is
compiled with, but I do know that some don't like that. If it works, it
works.

> 
> Also, can we check CONFIG_PINCTRL_EXYNOS instead?  That would make it
> easier when migrating Exynos 4 and 5 devices to pinctrl driver and
> OF_UPSTREAM.

Yeah, that works too.
Only reason I used EXYNOS4/5 is that that's really what it directly
depends on, and I didn't know it was planned to move Exynos4/5 specific
stuff to CCF/pinctrl.

My only real nitpick is that these changes would mean we'd have to
reintroduce the dependency on exynos platforms, since if we say "if we
don't have exynos ccf driver, use exynos specific stuff", then if we
just add the driver to some other non-exynos platform without exynos
ccf driver enabled for some reason, it would not compile, since it'd be
trying to use the old exynos4/5 stuff.
Though for now it should be fine if we limit it to Exynos only.

> 
> Same comments apply to s3c24x0_i2c.c below.
> 
> Best regards,
> Henrik Grimler
> 
> 

Best regards,
David

Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5

[Henrik Grimler]

Hi David,

On Sat, Aug 10, 2024 at 01:05:06AM +0200, David Virag wrote:
> Hi Henrik,
> 
> On Fri, 2024-08-09 at 23:08 +0200, Henrik Grimler wrote:
> > Hi David,
> > 
> > Thinking about this a bit more...
> > 
> > On Fri, Aug 02, 2024 at 09:19:16PM +0200, David Virag wrote:
> [snip]
> > > @@ -9,11 +9,15 @@
> > >  #include <dm.h>
> > >  #include <i2c.h>
> > >  #include <log.h>
> > > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> > >  #include <asm/arch/clk.h>
> > >  #include <asm/arch/cpu.h>
> > >  #include <asm/arch/pinmux.h>
> > > +#endif
> > 
> > We want to try to move the Exynos 4 and 5 devices to CCF and
> > OF_UPSTREAM as well, which will make this messy.  Can we check
> > CONFIG_CLK_EXYNOS and CONFIG_PINCTRL_EXYNOS instead?
> > 
> 
> Happy to hear that move, but you are a bit late, this got merged into
> master not long ago. Sending a patch for changing it should be fine,
> the 7885 port I'm working on uses mostly the same things as the E850-
> 96board (just has some support for 2 boards right now, though I may
> submit jackpotlte only first, will see)

Ah, oops, missed that!  Yeah, no problem, I will touch it in an
upcoming patchset.

> > >  #include <asm/global_data.h>
> > > +#include <asm/io.h>
> > >  #include <linux/delay.h>
> > > +#include <clk.h>
> > >  #include "s3c24x0_i2c.h"
> > >  
> > >  DECLARE_GLOBAL_DATA_PTR;
> > > @@ -137,15 +141,26 @@ static int hsi2c_wait_for_trx(struct
> > > exynos5_hsi2c *i2c)
> > >  	return I2C_NOK_TOUT;
> > >  }
> > >  
> > > -static int hsi2c_get_clk_details(struct s3c24x0_i2c_bus *i2c_bus)
> > > +static int hsi2c_get_clk_details(struct udevice *dev)
> > >  {
> > > +	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
> > >  	struct exynos5_hsi2c *hsregs = i2c_bus->hsregs;
> > >  	ulong clkin;
> > >  	unsigned int op_clk = i2c_bus->clock_frequency;
> > >  	unsigned int i = 0, utemp0 = 0, utemp1 = 0;
> > >  	unsigned int t_ftl_cycle;
> > >  
> > > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> > 
> > As above, can we check CONFIG_CLK_EXYNOS instead?
> > 
> 
> Should be fine
> 
> > >  	clkin = get_i2c_clk();
> > > +#else
> > > +	struct clk clk;
> > > +	int ret;
> > > +
> > > +	ret = clk_get_by_name(dev, "hsi2c", &clk);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	clkin = clk_get_rate(&clk);
> > > +#endif
> > >  	/* FPCLK / FI2C =
> > >  	 * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 *
> > > FLT_CYCLE
> > >  	 * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
> > > @@ -487,7 +502,7 @@ static int s3c24x0_i2c_set_bus_speed(struct
> > > udevice *dev, unsigned int speed)
> > >  
> > >  	i2c_bus->clock_frequency = speed;
> > >  
> > > -	if (hsi2c_get_clk_details(i2c_bus))
> > > +	if (hsi2c_get_clk_details(dev))
> > >  		return -EFAULT;
> > >  	hsi2c_ch_init(i2c_bus);
> > >  
> > > @@ -514,7 +529,9 @@ static int s3c24x0_i2c_probe(struct udevice
> > > *dev, uint chip, uint chip_flags)
> > >  
> > >  static int s3c_i2c_of_to_plat(struct udevice *dev)
> > >  {
> > > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> > >  	const void *blob = gd->fdt_blob;
> > > +#endif
> > >  	struct s3c24x0_i2c_bus *i2c_bus = dev_get_priv(dev);
> > >  	int node;
> > >  
> > > @@ -522,7 +539,9 @@ static int s3c_i2c_of_to_plat(struct udevice
> > > *dev)
> > >  
> > >  	i2c_bus->hsregs = dev_read_addr_ptr(dev);
> > >  
> > > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> > >  	i2c_bus->id = pinmux_decode_periph_id(blob, node);
> > > +#endif
> > >  
> > >  	i2c_bus->clock_frequency =
> > >  		dev_read_u32_default(dev, "clock-frequency",
> > > @@ -530,7 +549,9 @@ static int s3c_i2c_of_to_plat(struct udevice
> > > *dev)
> > >  	i2c_bus->node = node;
> > >  	i2c_bus->bus_num = dev_seq(dev);
> > >  
> > > +#if IS_ENABLED(CONFIG_ARCH_EXYNOS4) ||
> > > IS_ENABLED(CONFIG_ARCH_EXYNOS5)
> > >  	exynos_pinmux_config(i2c_bus->id, PINMUX_FLAG_HS_MODE);
> > > +#endif
> > 
> > I think these three #if def's can be consolidated in one #if def
> > group
> > towards the end of s3c_i2c_of_to_plat to make it easier to follow.
> 
> Hm, sounds good, though I don't know if moving the variable declaration
> from the top would work. I don't know which C standard uboot is
> compiled with, but I do know that some don't like that. If it works, it
> works.

We can probably drop "const void *blob" altogether and use
gd->fdt_blob straight away.

> > 
> > Also, can we check CONFIG_PINCTRL_EXYNOS instead?  That would make it
> > easier when migrating Exynos 4 and 5 devices to pinctrl driver and
> > OF_UPSTREAM.
> 
> Yeah, that works too.
> Only reason I used EXYNOS4/5 is that that's really what it directly
> depends on, and I didn't know it was planned to move Exynos4/5 specific
> stuff to CCF/pinctrl.

I am working on migrating exynos4412-odroid-u2 and
exyno5422-odroid-xu4, as well as adding support for exynos4210-i9100.

> My only real nitpick is that these changes would mean we'd have to
> reintroduce the dependency on exynos platforms, since if we say "if we
> don't have exynos ccf driver, use exynos specific stuff", then if we
> just add the driver to some other non-exynos platform without exynos
> ccf driver enabled for some reason, it would not compile, since it'd be
> trying to use the old exynos4/5 stuff.
> Though for now it should be fine if we limit it to Exynos only.

I guess only exynos and older s3c/s5p devices will ever use the
driver, based on current situation in Linux at least.  Even if we fix
compilation for a none-exynos4/5 device without CCF I suppose driver
would not work without modifications to deal with clocks.

Anyways, thanks!  Will CC you on future changes!

Best regards,
Henrik Grimler