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