* Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5
2024-08-02 19:19 ` [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5 David Virag
@ 2024-08-03 22:38 ` Henrik Grimler
2024-08-05 4:53 ` Heiko Schocher
2024-08-09 21:08 ` Henrik Grimler
2 siblings, 0 replies; 9+ messages in thread
From: Henrik Grimler @ 2024-08-03 22:38 UTC (permalink / raw)
To: David Virag
Cc: Heiko Schocher, Tom Rini, Paul Barker, Marek Vasut,
Caleb Connolly, Simon Glass, Neil Armstrong, Sam Protsenko,
u-boot
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5
2024-08-02 19:19 ` [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5 David Virag
2024-08-03 22:38 ` Henrik Grimler
@ 2024-08-05 4:53 ` Heiko Schocher
2024-08-09 21:08 ` Henrik Grimler
2 siblings, 0 replies; 9+ messages in thread
From: Heiko Schocher @ 2024-08-05 4:53 UTC (permalink / raw)
To: David Virag, Tom Rini, Paul Barker, Marek Vasut, Caleb Connolly,
Simon Glass, Neil Armstrong
Cc: Sam Protsenko, Henrik Grimler, u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5
2024-08-02 19:19 ` [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5 David Virag
2024-08-03 22:38 ` Henrik Grimler
2024-08-05 4:53 ` Heiko Schocher
@ 2024-08-09 21:08 ` Henrik Grimler
2024-08-09 23:05 ` David Virag
2 siblings, 1 reply; 9+ messages in thread
From: Henrik Grimler @ 2024-08-09 21:08 UTC (permalink / raw)
To: David Virag
Cc: Heiko Schocher, Tom Rini, Paul Barker, Marek Vasut,
Caleb Connolly, Simon Glass, Neil Armstrong, Sam Protsenko,
u-boot
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5
2024-08-09 21:08 ` Henrik Grimler
@ 2024-08-09 23:05 ` David Virag
2024-08-10 21:14 ` Henrik Grimler
0 siblings, 1 reply; 9+ messages in thread
From: David Virag @ 2024-08-09 23:05 UTC (permalink / raw)
To: Henrik Grimler
Cc: Heiko Schocher, Tom Rini, Paul Barker, Marek Vasut,
Caleb Connolly, Simon Glass, Neil Armstrong, Sam Protsenko,
u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/2] i2c: samsung: Support platforms other than EXYNOS4 and EXYNOS5
2024-08-09 23:05 ` David Virag
@ 2024-08-10 21:14 ` Henrik Grimler
0 siblings, 0 replies; 9+ messages in thread
From: Henrik Grimler @ 2024-08-10 21:14 UTC (permalink / raw)
To: David Virag
Cc: Heiko Schocher, Tom Rini, Paul Barker, Marek Vasut,
Caleb Connolly, Simon Glass, Neil Armstrong, Sam Protsenko,
u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread