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

Henrik Grimler henrik at grimler.se
Fri Aug 9 23:08:18 CEST 2024


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 at 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
> 


More information about the U-Boot mailing list