Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT

[Timur Tabi <timur@freescale.com>]

Scott Wood wrote:
> Timur Tabi wrote:
>> Grant Likely wrote:
>>
>>> No it doesn't, it depends on the register interface to decide
>>> compatibility.  Clock interface is part of that. 
>> I don't think so.  The interface for programming the clock registers is
>> identical on all 8[356]xx parts.  The only thing that matters is what specific
>> values to put in the FDR and DFSR registers to get a desired I2C bus speed.
> 
> If it affects the values you need to write to the registers to achieve a 
> given result, how is it not a difference in the register interface?

I think we're splitting hairs, here.  The code for actually programming the
registers is the same, regardless of the input frequency.  It's just that the
input frequency is a value needed to calculate the right value.  But like I
said, I don't think this distinction is that relevant.

> A clock-frequency property is OK, and is in line with what we do in 
> other types of nodes.  However, in the long run it might be nice to 
> introduce some sort of clock binding where, for example, the i2c node 
> can point to a clock elsewhere in the device tree as an input clock.

The only problem with that is that the actual input clock to the I2C device is
not the same as any other device.  It's a unique clock.  Look at the code I had
to write to figure out this clock just on 85xx:

	/*
	 * The base clock for I2C depends on the actual SOC.  Unfortunately,
	 * there is no pattern that can be used to determine the frequency, so
	 * the only choice is to look up the actual SOC number and use the value
	 * for that SOC. This information is taken from application note
	 * AN2919.
	 */
#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
	defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555)
	gd->i2c1_clk = sys_info.freqSystemBus;
#elif defined(CONFIG_MPC8544)
	/*
	 * On the 8544, the I2C clock is the same as the SEC clock.  This can be
	 * either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See
	 * 4.4.3.3 of the 8544 RM.  Note that this might actually work for all
	 * 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the
	 * PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544.
	 */
	if (gur->pordevsr2 & MPC85xx_PORDEVSR2_SEC_CFG)
		gd->i2c1_clk = sys_info.freqSystemBus / 3;
	else
		gd->i2c1_clk = sys_info.freqSystemBus / 2;
#else
	/* Most 85xx SOCs use CCB/2, so this is the default behavior. */
	gd->i2c1_clk = sys_info.freqSystemBus / 2;
#endif

This is just for 85xx, and it only applies to the I2C devices.

> That way, less knowledge is required by the firmware to poke values all 
> over the place, and it also allows one to describe situations where the 
> frequency of the input clock can change (such as in low-power modes).

I don't think it's possible to do what you want to do.  I2C clocking is
completely screwed up, and that's just the way the hardware was designed.

-- 
Timur Tabi
Linux kernel developer at Freescale