| Message ID | 1339242351-8797-1-git-send-email-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
Dear Marek Vasut, > This patch configures the I2C bus timing registers according > to information passed via DT. Currently, 100kHz and 400kHz > modes are supported. [...] > +struct mxs_i2c_speed_config { > + uint32_t timing0; > + uint32_t timing1; > + uint32_t timing2; > +}; > + > +/* Timing values for the default 24MHz clock supplied into the i2c block. Thinking about these further -- does anyone have any idea how these numbers were derived? And possibly even formula for that? And maybe we should somehow make sure the source runs on 24MHz (how?). > */ +const struct mxs_i2c_speed_config mxs_i2c_95kHz_config = { > + .timing0 = 0x00780030, > + .timing1 = 0x00800030, > + .timing2 = 0x0015000d, > +}; > + > +const struct mxs_i2c_speed_config mxs_i2c_400kHz_config = { > + .timing0 = 0x000f0007, > + .timing1 = 0x001f000f, > + .timing2 = 0x0015000d, > +}; [...] Best regards, Marek Vasut
On Sat, Jun 09, 2012 at 01:45:50PM +0200, Marek Vasut wrote: > This patch configures the I2C bus timing registers according > to information passed via DT. Currently, 100kHz and 400kHz > modes are supported. > > Signed-off-by: Marek Vasut <marex@denx.de> I gave it a test on imx28-evk board with audio playback. It seems the patch makes the first time playback non-functional, but the later playback is still working. > +static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c) > +{ > + uint32_t speed; > + struct device *dev = i2c->dev; > + struct device_node *node = dev->of_node; > + > + if (!node) > + return -EINVAL; > + > + i2c->speed = &mxs_i2c_95kHz_config; > + ret = of_property_read_u32(node, "clock-frequency", &speed); "ret" is undeclared. > + if (ret) > + dev_warn(dev, "No I2C speed selected, using 100kHz\n"); > + else if (speed == 400000) > + i2c->speed = &mxs_i2c_400kHz_config; > + else if (speed != 100000) > + dev_warn(dev, "Invalid I2C speed selected, using 100kHz\n"); > + > + return 0; > +} > +
On Sun, Jun 10, 2012 at 01:53:05PM +0200, Marek Vasut wrote: > > +struct mxs_i2c_speed_config { > > + uint32_t timing0; > > + uint32_t timing1; > > + uint32_t timing2; > > +}; > > + > > +/* Timing values for the default 24MHz clock supplied into the i2c block. > > Thinking about these further -- does anyone have any idea how these numbers were > derived? And possibly even formula for that? > > And maybe we should somehow make sure the source runs on 24MHz (how?). > There is nothing about I2C clock mentioned in CLKCTRL chapter. I just contacted design team and was told that I2C clock sources from APBX (xbus) clock and always runs at the same frequency there.
Dear Shawn Guo, > On Sun, Jun 10, 2012 at 01:53:05PM +0200, Marek Vasut wrote: > > > +struct mxs_i2c_speed_config { > > > + uint32_t timing0; > > > + uint32_t timing1; > > > + uint32_t timing2; > > > +}; > > > + > > > +/* Timing values for the default 24MHz clock supplied into the i2c > > > block. > > > > Thinking about these further -- does anyone have any idea how these > > numbers were derived? And possibly even formula for that? > > > > And maybe we should somehow make sure the source runs on 24MHz (how?). > > There is nothing about I2C clock mentioned in CLKCTRL chapter. I just > contacted design team and was told that I2C clock sources from APBX > (xbus) clock and always runs at the same frequency there. Ok, then can you please try asking them how to exactly compute the values in timing0-timing2 registers? So we don't have to hardcode them like it's done now? Best regards, Marek Vasut
Dear Shawn Guo, > On Sat, Jun 09, 2012 at 01:45:50PM +0200, Marek Vasut wrote: > > This patch configures the I2C bus timing registers according > > to information passed via DT. Currently, 100kHz and 400kHz > > modes are supported. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > I gave it a test on imx28-evk board with audio playback. It seems > the patch makes the first time playback non-functional, but the later > playback is still working. Any hints what can be the source of this issue? I tested it with i2c eeprom, saw no issues in there. I'll poke into it later. > > +static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c) > > +{ > > + uint32_t speed; > > + struct device *dev = i2c->dev; > > + struct device_node *node = dev->of_node; > > + > > + if (!node) > > + return -EINVAL; > > + > > + i2c->speed = &mxs_i2c_95kHz_config; > > + ret = of_property_read_u32(node, "clock-frequency", &speed); > > "ret" is undeclared. > > > + if (ret) > > + dev_warn(dev, "No I2C speed selected, using 100kHz\n"); > > + else if (speed == 400000) > > + i2c->speed = &mxs_i2c_400kHz_config; > > + else if (speed != 100000) > > + dev_warn(dev, "Invalid I2C speed selected, using 100kHz\n"); > > + > > + return 0; > > +} > > + Best regards, Marek Vasut
On Mon, Jun 11, 2012 at 12:53:17PM +0200, Marek Vasut wrote: > Ok, then can you please try asking them how to exactly compute the values in > timing0-timing2 registers? So we don't have to hardcode them like it's done now? > It's determined I2C clock waveform you want to get. See i.MX28 RM "Figure 27-2. I2C Data and Clock Timing" and "Figure 27-3. I2C Data and Clock Timing Generation". For example, if you run 12MHz APBX clock, and set HIGH_COUNT to 60, the I2C clock will have 60 cycle x (1/12MHz) = 5us time for its high period.
Dear Shawn Guo, > On Mon, Jun 11, 2012 at 12:53:17PM +0200, Marek Vasut wrote: > > Ok, then can you please try asking them how to exactly compute the values > > in timing0-timing2 registers? So we don't have to hardcode them like > > it's done now? > > It's determined I2C clock waveform you want to get. See i.MX28 RM > "Figure 27-2. I2C Data and Clock Timing" and "Figure 27-3. I2C Data > and Clock Timing Generation". > > For example, if you run 12MHz APBX clock, and set HIGH_COUNT to 60, > the I2C clock will have 60 cycle x (1/12MHz) = 5us time for its high > period. Ok, I think I see the equation. But what still doesn't make sense is how you got to the value of 48 (RCV_COUNT at 95kHz). And how you got 120 for HIGH_COUNT and 128 for LOW_COUNT? Were these values based on some measurements, making them the best possible values? Won't computation of slightly different values affect reliability of this driver? Best regards, Marek Vasut
Dear Shawn Guo, > This patch configures the I2C bus timing registers according > to information passed via DT. Currently, 100kHz and 400kHz > modes are supported. [...] Is there any reason why this can not be merged other than the timing registers goo (which I believe shall stay as in the datasheet until we figure out if it's even reasonable to add some computation there). Best regards, Marek Vasut
On Sat, Jun 23, 2012 at 08:47:32PM +0200, Marek Vasut wrote: > Dear Shawn Guo, > > > This patch configures the I2C bus timing registers according > > to information passed via DT. Currently, 100kHz and 400kHz > > modes are supported. > [...] > > Is there any reason why this can not be merged other than the timing registers > goo (which I believe shall stay as in the datasheet until we figure out if it's > even reasonable to add some computation there). > I assume this is a question for Wolfram. But I guess part of the reason is there is still one comment from me staying unresolved.
Dear Shawn Guo, > On Sat, Jun 23, 2012 at 08:47:32PM +0200, Marek Vasut wrote: > > Dear Shawn Guo, > > > > > This patch configures the I2C bus timing registers according > > > to information passed via DT. Currently, 100kHz and 400kHz > > > modes are supported. > > > > [...] > > > > Is there any reason why this can not be merged other than the timing > > registers goo (which I believe shall stay as in the datasheet until we > > figure out if it's even reasonable to add some computation there). > > I assume this is a question for Wolfram. But I guess part of the reason > is there is still one comment from me staying unresolved. Ah, you mean about the audio? I'll poke into it tonight. Best regards, Marek Vasut
Dear Shawn Guo, > On Sat, Jun 23, 2012 at 08:47:32PM +0200, Marek Vasut wrote: > > Dear Shawn Guo, > > > > > This patch configures the I2C bus timing registers according > > > to information passed via DT. Currently, 100kHz and 400kHz > > > modes are supported. > > > > [...] > > > > Is there any reason why this can not be merged other than the timing > > registers goo (which I believe shall stay as in the datasheet until we > > figure out if it's even reasonable to add some computation there). > > I assume this is a question for Wolfram. But I guess part of the reason > is there is still one comment from me staying unresolved. Shawn, I just re-tested the i2c with mpg123 playing "MPEG 1.0 layer III, VBR, 44100 Hz joint-stereo" and it worked on a first try ... can you retest please? Best regards, Marek Vasut
Dear Shawn Guo, > > On Sat, Jun 09, 2012 at 01:45:50PM +0200, Marek Vasut wrote: > > > This patch configures the I2C bus timing registers according > > > to information passed via DT. Currently, 100kHz and 400kHz > > > modes are supported. > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > > I gave it a test on imx28-evk board with audio playback. It seems > > the patch makes the first time playback non-functional, but the later > > playback is still working. > > Any hints what can be the source of this issue? I tested it with i2c > eeprom, saw no issues in there. I'll poke into it later. Ok, I managed to replicate it just now. Scrap my previous email. Still, any idea what can cause this? Best regards, Marek Vasut
Dear Shawn Guo, > On Wed, Jun 27, 2012 at 03:30:31AM +0200, Marek Vasut wrote: > > Ok, I managed to replicate it just now. Scrap my previous email. > > > > Still, any idea what can cause this? > > I just spent some time on it. It looks like a document issue (dammit). > > The HW_I2C_TIMING2 EXAMPLE tells the value is 0x0015000d which is the > one you looked at and used. But the register field figure tells it's > 0x00300030, which seems the real case. > > The mxs_i2c_reset at probe changes the value to 0x0015000d which causes > the first time playback nonfunctional, but the mxs_i2c_reset call in > mxs_i2c_xfer_msg happens to reset the value back to 0x00300030, and > then second time playback starts working. > > The changes below get everything work fine, both 100k and 400k. So > with the changes in, you can add: Ok, can you push the FSL guys to roll out updated document? Do you consider it reasonable to add some calculations of these timing values or were they somehow fine-tuned with a scope/deep HW knowledge as the best working values and we should stick to those? > Tested-by: Shawn Guo <shawn.guo@linaro.org> > > Regards, > Shawn > > --8<--- > > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c > index e38be56..c2467e9 100644 > --- a/drivers/i2c/busses/i2c-mxs.c > +++ b/drivers/i2c/busses/i2c-mxs.c > @@ -112,13 +112,13 @@ struct mxs_i2c_speed_config { > const struct mxs_i2c_speed_config mxs_i2c_95kHz_config = { > .timing0 = 0x00780030, > .timing1 = 0x00800030, > - .timing2 = 0x0015000d, > + .timing2 = 0x00300030, > }; > > const struct mxs_i2c_speed_config mxs_i2c_400kHz_config = { > .timing0 = 0x000f0007, > .timing1 = 0x001f000f, > - .timing2 = 0x0015000d, > + .timing2 = 0x00300030, > }; Best regards, Marek Vasut
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt index 1bfc02d..d2bf750 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt @@ -4,6 +4,7 @@ Required properties: - compatible: Should be "fsl,<chip>-i2c" - reg: Should contain registers location and length - interrupts: Should contain ERROR and DMA interrupts +- clock-frequency: desired I2C bus clock frequency in Hz. Examples: diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi index a89da5a..714e63c 100644 --- a/arch/arm/boot/dts/imx28.dtsi +++ b/arch/arm/boot/dts/imx28.dtsi @@ -398,6 +398,7 @@ compatible = "fsl,imx28-i2c"; reg = <0x80058000 2000>; interrupts = <111 68>; + clock-frequency = <400000>; status = "disabled"; }; @@ -407,6 +408,7 @@ compatible = "fsl,imx28-i2c"; reg = <0x8005a000 2000>; interrupts = <110 69>; + clock-frequency = <400000>; status = "disabled"; }; diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c index 04eb441..b7d423e 100644 --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -46,6 +46,10 @@ #define MXS_I2C_CTRL0_DIRECTION 0x00010000 #define MXS_I2C_CTRL0_XFER_COUNT(v) ((v) & 0x0000FFFF) +#define MXS_I2C_TIMING0 (0x10) +#define MXS_I2C_TIMING1 (0x20) +#define MXS_I2C_TIMING2 (0x30) + #define MXS_I2C_CTRL1 (0x40) #define MXS_I2C_CTRL1_SET (0x44) #define MXS_I2C_CTRL1_CLR (0x48) @@ -97,6 +101,25 @@ #define MXS_CMD_I2C_READ (MXS_I2C_CTRL0_SEND_NAK_ON_LAST | \ MXS_I2C_CTRL0_MASTER_MODE) +struct mxs_i2c_speed_config { + uint32_t timing0; + uint32_t timing1; + uint32_t timing2; +}; + +/* Timing values for the default 24MHz clock supplied into the i2c block. */ +const struct mxs_i2c_speed_config mxs_i2c_95kHz_config = { + .timing0 = 0x00780030, + .timing1 = 0x00800030, + .timing2 = 0x0015000d, +}; + +const struct mxs_i2c_speed_config mxs_i2c_400kHz_config = { + .timing0 = 0x000f0007, + .timing1 = 0x001f000f, + .timing2 = 0x0015000d, +}; + /** * struct mxs_i2c_dev - per device, private MXS-I2C data * @@ -112,11 +135,17 @@ struct mxs_i2c_dev { struct completion cmd_complete; u32 cmd_err; struct i2c_adapter adapter; + const struct mxs_i2c_speed_config *speed; }; static void mxs_i2c_reset(struct mxs_i2c_dev *i2c) { stmp_reset_block(i2c->regs); + + writel(i2c->speed->timing0, i2c->regs + MXS_I2C_TIMING0); + writel(i2c->speed->timing1, i2c->regs + MXS_I2C_TIMING1); + writel(i2c->speed->timing2, i2c->regs + MXS_I2C_TIMING2); + writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET); writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE, i2c->regs + MXS_I2C_QUEUECTRL_SET); @@ -319,6 +348,27 @@ static const struct i2c_algorithm mxs_i2c_algo = { .functionality = mxs_i2c_func, }; +static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c) +{ + uint32_t speed; + struct device *dev = i2c->dev; + struct device_node *node = dev->of_node; + + if (!node) + return -EINVAL; + + i2c->speed = &mxs_i2c_95kHz_config; + ret = of_property_read_u32(node, "clock-frequency", &speed); + if (ret) + dev_warn(dev, "No I2C speed selected, using 100kHz\n"); + else if (speed == 400000) + i2c->speed = &mxs_i2c_400kHz_config; + else if (speed != 100000) + dev_warn(dev, "Invalid I2C speed selected, using 100kHz\n"); + + return 0; +} + static int __devinit mxs_i2c_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -358,6 +408,11 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev) return err; i2c->dev = dev; + + err = mxs_i2c_get_ofdata(i2c); + if (err) + return err; + platform_set_drvdata(pdev, i2c); /* Do reset to enforce correct startup after pinmuxing */
This patch configures the I2C bus timing registers according to information passed via DT. Currently, 100kHz and 400kHz modes are supported. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Detlev Zundel <dzu@denx.de> CC: Dong Aisheng <b29396@freescale.com> CC: Fabio Estevam <fabio.estevam@freescale.com> Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org> Cc: linux-i2c@vger.kernel.org CC: Sascha Hauer <s.hauer@pengutronix.de> CC: Shawn Guo <shawn.guo@linaro.org> Cc: Stefano Babic <sbabic@denx.de> CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Cc: Wolfgang Denk <wd@denx.de> Cc: Wolfram Sang <w.sang@pengutronix.de> --- Documentation/devicetree/bindings/i2c/i2c-mxs.txt | 1 + arch/arm/boot/dts/imx28.dtsi | 2 + drivers/i2c/busses/i2c-mxs.c | 55 +++++++++++++++++++++ 3 files changed, 58 insertions(+) V2: Use clock-frequency instead V3: Document that 24MHz clock are the base for i2c speed settings