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

[David Virag]

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