Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC

[Sascha Hauer]

On Thu, May 09, 2019 at 04:35:33AM +0000, Sumit Batra wrote:
> > > > So the clock driver reports the wrong clock. Please fix the clock driver then.
> > > No, this is a problem with the i2c driver. It is not a problem with 
> > > the clock driver, so the i2c driver needs to be modified.
> > 
> > So how does this RCW bit get evaluated? 
> According to the reference manual
> > only one clock goes to the i2c module (described as 1/2 Platform
> > Clock) and the i2c module only takes one clock. So it seems there must 
> > be a /2 divider somewhere, either in each i2c module or somewhere 
> > outside. Can your IC guys tell you where it is?
> I need to confirm this with the IC team

[Reformated a bit to make it readable]:

> There are 2 places where clock division takes place -
> 
> 1) There is a clock divider outside of I2C block, which makes the clock reaching
>    I2C module as - Platform Clock/2
> 2) There is another clock divider which specifically divides the clock to the I2C block,
>    based on RCW bit 424 (if 424th bit is 0 then the baud clock source is Platform Clock/4,
>    if 424th bit is 1 then it remains Platform Clock/2)

So there is a clock divider which based on RCW bit 424 divides the clock
*to* the i2c module. This suggests the divider is outside of the i2c
module itself and thus part of the clock module.

We could argue that this divider sits between the clock module and the
i2c module, but for sure it's not in the i2c module. I really suggest to
put this SoC specific into the SoC specific clock driver rather than
littering the i2c driver with it.

Sascha

> 
> Now based on the what is the desired SCL value (100KHz etc) and the clock which is
> received by I2C block, there is a calculation that goes on inside the I2C driver
> module which is used to map a value in this imx_i2c_clk_div table. This value is used
> to program the IMX_I2C_IFDR register of the I2C block. Now if we don't consider the
> RCW bit 424 in our I2C driver calculation then the IMX_I2C_IFDR value that gets set
> makes SCL half of what is desired by the user. This is because if you make the RCW
> 424th bit as 0 then anyhow I2C block (hardware) will receive Platform Clock/4, but
> the driver (since it has not considered this bit) will consider it as Platform Clock/2
> so it'll program a bigger divider from the imx_i2c_clk_div table

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |