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

[Henrik Grimler <henrik@grimler.se>]

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