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
