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

[Sumit Batra]

Hi Sascha,
Please check my comment

-----Original Message-----
From: Chuanhua Han 
Sent: Wednesday, May 8, 2019 5:05 PM
To: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
Cc: shawnguo@xxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; wsa+renesas@xxxxxxxxxxxxxxxxxxxx; u.kleine-koenig@xxxxxxxxxxxxxx; eha@xxxxxxxx; linux@xxxxxxxxxxxxxxxx; l.stach@xxxxxxxxxxxxxx; peda@xxxxxxxxxx; Sumit Batra <sumit.batra@xxxxxxx>
Subject: RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC



> -----Original Message-----
> From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Sent: 2019å5æ6æ 15:38
> To: Chuanhua Han <chuanhua.han@xxxxxxx>
> Cc: shawnguo@xxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; 
> robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; 
> linux-kernel@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; 
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx 
> <linux-imx@xxxxxxx>; wsa+renesas@xxxxxxxxxxxxxxxxxxxx; 
> u.kleine-koenig@xxxxxxxxxxxxxx; eha@xxxxxxxx; linux@xxxxxxxxxxxxxxxx; 
> l.stach@xxxxxxxxxxxxxx; peda@xxxxxxxxxx; Sumit Batra 
> <sumit.batra@xxxxxxx>
> Subject: Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't 
> consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
> 
> Caution: EXT Email
> 
> On Sat, May 04, 2019 at 09:28:48AM +0000, Chuanhua Han wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > > Sent: 2019å4æ30æ 20:51
> > > To: Chuanhua Han <chuanhua.han@xxxxxxx>
> > > Cc: shawnguo@xxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>;
> > > robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; 
> > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > > devicetree@xxxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx 
> > > <linux-imx@xxxxxxx>; wsa+renesas@xxxxxxxxxxxxxxxxxxxx; 
> > > u.kleine-koenig@xxxxxxxxxxxxxx; eha@xxxxxxxx; 
> > > linux@xxxxxxxxxxxxxxxx; l.stach@xxxxxxxxxxxxxx; peda@xxxxxxxxxx; 
> > > Sumit Batra <sumit.batra@xxxxxxx>
> > > Subject: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't 
> > > consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
> > >
> > > Caution: EXT Email
> > >
> > > On Tue, Apr 30, 2019 at 12:47:18PM +0800, Chuanhua Han wrote:
> > > > The current kernel driver does not consider I2C_IPGCLK_SEL (424 
> > > > bit of
> > > > RCW) in deciding  i2c_clk_rate in function i2c_imx_set_clk() { 0 
> > > > Platform clock/4, 1 Platform clock/2}.
> > > >
> > > > When using ls1046a SoC, this populates incorrect value in IBFD 
> > > > register if I2C_IPGCLK_SEL = 0, which generates half of the desired Clock.
> > > >
> > > > Therefore, if ls1046a SoC is used, we need to set the i2c clock 
> > > > according to the corresponding RCW.
> > >
> > > 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
[Sumit Batra] 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)
              3) 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		
> 
> One reason I suggested the clock driver is that the clock driver 
> contains SoC specific code already, so it should be easier to integrate there.
It seems inappropriate to put the clock frequency division modification of i2c in the clock driver, because the clock driver is for all IP and is a universal code, so I think it is better to modify the clock in the IP driver.
> 
> Sascha
> 
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> e ngutronix.de%2F&amp;data=02%7C01%7Cchuanhua.han%40nxp.com%7Cb2d
> 4680699c448e8514308d6d1f5bf82%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636927250743516563&amp;sdata=pFdCbiDXE%2FDll01X9Nj
> Hg3SCDpECzgrr8MLtYBdKH5c%3D&amp;reserved=0  | Peiner Str. 6-8, 31137 
> Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |