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