Patchwork [1/2,V3] MXS: Set I2C timing registers for mxs-i2c

login
register
mail settings
Submitter Marek Vasut
Date June 9, 2012, 11:45 a.m.
Message ID <1339242351-8797-1-git-send-email-marex@denx.de>
Download mbox | patch
Permalink /patch/163910/
State New
Headers show

Comments

Marek Vasut - June 9, 2012, 11:45 a.m.
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
Marek Vasut - June 10, 2012, 11:53 a.m.
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
Shawn Guo - June 11, 2012, 6:39 a.m.
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;
> +}
> +
Shawn Guo - June 11, 2012, 7:30 a.m.
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.
Marek Vasut - June 11, 2012, 10:53 a.m.
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
Marek Vasut - June 11, 2012, 10:54 a.m.
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
Shawn Guo - June 11, 2012, 2:42 p.m.
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.
Marek Vasut - June 23, 2012, 6:19 p.m.
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
Marek Vasut - June 23, 2012, 6:47 p.m.
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
Shawn Guo - June 25, 2012, 3:43 p.m.
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.
Marek Vasut - June 25, 2012, 4:02 p.m.
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
Marek Vasut - June 27, 2012, 1:15 a.m.
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
Marek Vasut - June 27, 2012, 1:30 a.m.
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
Marek Vasut - June 27, 2012, 2:34 a.m.
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

Patch

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 */