[PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Roland Stigge]

The I2C Control Register bits RFDAIE and RFFIE were mixed up. In addition to
this fix, this patch adds the missing bit DRSIE for completeness.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
Applies to v3.6-rc1

This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx, not yet in
mainline). Can you please test and double-check the manuals of PNX4008 and
LPC31xx? I only found this via the manual of LPC32xx but assume it's the same
for the others, also.

Thanks in advance!

 drivers/i2c/busses/i2c-pnx.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- linux-2.6.orig/drivers/i2c/busses/i2c-pnx.c
+++ linux-2.6/drivers/i2c/busses/i2c-pnx.c
@@ -48,8 +48,9 @@ enum {
 	mcntrl_afie = 0x00000002,
 	mcntrl_naie = 0x00000004,
 	mcntrl_drmie = 0x00000008,
-	mcntrl_daie = 0x00000020,
-	mcntrl_rffie = 0x00000040,
+	mcntrl_drsie = 0x00000010,
+	mcntrl_rffie = 0x00000020,
+	mcntrl_daie = 0x00000040,
 	mcntrl_tffie = 0x00000080,
 	mcntrl_reset = 0x00000100,
 	mcntrl_cdbmode = 0x00000400,

[PATCH RESEND 2/2] i2c: pnx: Fix read transactions of >= 2 bytes

[Roland Stigge]

On transactions with n>=2 bytes, the controller actually wrongly clocks in n+1
bytes. This is caused by the (wrong) assumption that RFE in the Status Register
is 1 iff there is no byte already ordered (via a dummy TX byte). This lead to
the implementation of synchronized byte ordering, e.g.:

Dummy-TX - RX - Dummy-TX - RX - ...

But since RFE actually stays high after some Dummy-TX, it rather looks like:

Dummy-TX - Dummy-TX - RX - Dummy-TX - RX - (RX)

The last RX byte is clocked in by the bus controller, but ignored by the kernel
when filling the userspace buffer.

This patch fixes the issue by asking for RX via Dummy-TX asynchronously.
Introducing a separate counter for TX bytes.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
Applies to v3.6-rc1

This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx, not yet in
mainline). Can you please test?

Thanks in advance!

 drivers/i2c/busses/i2c-pnx.c |   48 +++++++++++++++++++++++++------------------
 include/linux/i2c-pnx.h      |    1 
 2 files changed, 29 insertions(+), 20 deletions(-)

--- linux-2.6.orig/drivers/i2c/busses/i2c-pnx.c
+++ linux-2.6/drivers/i2c/busses/i2c-pnx.c
@@ -291,31 +291,37 @@ static int i2c_pnx_master_rcv(struct i2c
 	 * or we didn't 'ask' for it yet.
 	 */
 	if (ioread32(I2C_REG_STS(alg_data)) & mstatus_rfe) {
-		dev_dbg(&alg_data->adapter.dev,
-			"%s(): Write dummy data to fill Rx-fifo...\n",
-			__func__);
+		/* 'Asking' is done asynchronously, e.g. dummy TX of several
+		 * bytes is done before the first actual RX arrives in FIFO.
+		 * Therefore, ordered bytes (via TX) are counted separately.
+		 */
+		if (alg_data->mif.order) {
+			dev_dbg(&alg_data->adapter.dev,
+				"%s(): Write dummy data to fill Rx-fifo...\n",
+				__func__);
 
-		if (alg_data->mif.len == 1) {
-			/* Last byte, do not acknowledge next rcv. */
-			val |= stop_bit;
+			if (alg_data->mif.order == 1) {
+				/* Last byte, do not acknowledge next rcv. */
+				val |= stop_bit;
+
+				/*
+				 * Enable interrupt RFDAIE (data in Rx fifo),
+				 * and disable DRMIE (need data for Tx)
+				 */
+				ctl = ioread32(I2C_REG_CTL(alg_data));
+				ctl |= mcntrl_rffie | mcntrl_daie;
+				ctl &= ~mcntrl_drmie;
+				iowrite32(ctl, I2C_REG_CTL(alg_data));
+			}
 
 			/*
-			 * Enable interrupt RFDAIE (data in Rx fifo),
-			 * and disable DRMIE (need data for Tx)
+			 * Now we'll 'ask' for data:
+			 * For each byte we want to receive, we must
+			 * write a (dummy) byte to the Tx-FIFO.
 			 */
-			ctl = ioread32(I2C_REG_CTL(alg_data));
-			ctl |= mcntrl_rffie | mcntrl_daie;
-			ctl &= ~mcntrl_drmie;
-			iowrite32(ctl, I2C_REG_CTL(alg_data));
+			iowrite32(val, I2C_REG_TX(alg_data));
+			alg_data->mif.order--;
 		}
-
-		/*
-		 * Now we'll 'ask' for data:
-		 * For each byte we want to receive, we must
-		 * write a (dummy) byte to the Tx-FIFO.
-		 */
-		iowrite32(val, I2C_REG_TX(alg_data));
-
 		return 0;
 	}
 
@@ -515,6 +521,7 @@ i2c_pnx_xfer(struct i2c_adapter *adap, s
 
 		alg_data->mif.buf = pmsg->buf;
 		alg_data->mif.len = pmsg->len;
+		alg_data->mif.order = pmsg->len;
 		alg_data->mif.mode = (pmsg->flags & I2C_M_RD) ?
 			I2C_SMBUS_READ : I2C_SMBUS_WRITE;
 		alg_data->mif.ret = 0;
@@ -567,6 +574,7 @@ i2c_pnx_xfer(struct i2c_adapter *adap, s
 	/* Cleanup to be sure... */
 	alg_data->mif.buf = NULL;
 	alg_data->mif.len = 0;
+	alg_data->mif.order = 0;
 
 	dev_dbg(&alg_data->adapter.dev, "%s(): exiting, stat = %x\n",
 		__func__, ioread32(I2C_REG_STS(alg_data)));
--- linux-2.6.orig/include/linux/i2c-pnx.h
+++ linux-2.6/include/linux/i2c-pnx.h
@@ -22,6 +22,7 @@ struct i2c_pnx_mif {
 	struct timer_list	timer;		/* Timeout */
 	u8 *			buf;		/* Data buffer */
 	int			len;		/* Length of data buffer */
+	int			order;		/* RX Bytes to order via TX */
 };
 
 struct i2c_pnx_algo_data {

Re: [PATCH RESEND 2/2] i2c: pnx: Fix read transactions of >= 2 bytes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

On Wed, Aug 08, 2012 at 09:42:32AM +0200, Roland Stigge wrote:
> On transactions with n>=2 bytes, the controller actually wrongly clocks in n+1
> bytes. This is caused by the (wrong) assumption that RFE in the Status Register
> is 1 iff there is no byte already ordered (via a dummy TX byte). This lead to
> the implementation of synchronized byte ordering, e.g.:
> 
> Dummy-TX - RX - Dummy-TX - RX - ...
> 
> But since RFE actually stays high after some Dummy-TX, it rather looks like:
> 
> Dummy-TX - Dummy-TX - RX - Dummy-TX - RX - (RX)
> 
> The last RX byte is clocked in by the bus controller, but ignored by the kernel
> when filling the userspace buffer.
> 
> This patch fixes the issue by asking for RX via Dummy-TX asynchronously.
> Introducing a separate counter for TX bytes.
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> 
> ---
> Applies to v3.6-rc1
> 
> This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx, not yet in
> mainline). Can you please test?
> 
> Thanks in advance!

I assume you checked this on LPC32xx? I'll wait a bit more but tend to
take the patch anyhow, even if there is no explicit ACK from PNX.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RESEND 2/2] i2c: pnx: Fix read transactions of >= 2 bytes

[Roland Stigge]

Hi,

On 18/08/12 11:52, Wolfram Sang wrote:
> On Wed, Aug 08, 2012 at 09:42:32AM +0200, Roland Stigge wrote:
>> On transactions with n>=2 bytes, the controller actually wrongly
>> clocks in n+1 bytes. This is caused by the (wrong) assumption
>> that RFE in the Status Register is 1 iff there is no byte already
>> ordered (via a dummy TX byte). This lead to the implementation of
>> synchronized byte ordering, e.g.:
>> 
>> Dummy-TX - RX - Dummy-TX - RX - ...
>> 
>> But since RFE actually stays high after some Dummy-TX, it rather
>> looks like:
>> 
>> Dummy-TX - Dummy-TX - RX - Dummy-TX - RX - (RX)
>> 
>> The last RX byte is clocked in by the bus controller, but ignored
>> by the kernel when filling the userspace buffer.
>> 
>> This patch fixes the issue by asking for RX via Dummy-TX
>> asynchronously. Introducing a separate counter for TX bytes.
>> 
>> Signed-off-by: Roland Stigge <stigge@antcom.de>
>> 
>> --- Applies to v3.6-rc1
>> 
>> This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx,
>> not yet in mainline). Can you please test?
>> 
>> Thanks in advance!
> 
> I assume you checked this on LPC32xx?

Yes.

The bug surfaced when we debugged unexpected behaviour with I2C
clients. A colleague noticed that sometimes, there were more bytes
transferred than expected, confusing some I2C clients.

The patch fixes exactly that.

Thanks,

Roland

Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On Wed, Aug 08, 2012 at 09:42:31AM +0200, Roland Stigge wrote:
> The I2C Control Register bits RFDAIE and RFFIE were mixed up. In addition to
> this fix, this patch adds the missing bit DRSIE for completeness.
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> 
> ---
> Applies to v3.6-rc1
> 
> This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx, not yet in
> mainline). Can you please test and double-check the manuals of PNX4008 and
> LPC31xx? I only found this via the manual of LPC32xx but assume it's the same
> for the others, also.
> 
> Thanks in advance!

Kevin, since the other manuals seem to be not easily available, can you
please check?

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Roland Stigge]

On 18/08/12 11:51, Wolfram Sang wrote:
> On Wed, Aug 08, 2012 at 09:42:31AM +0200, Roland Stigge wrote:
>> The I2C Control Register bits RFDAIE and RFFIE were mixed up. In
>> addition to this fix, this patch adds the missing bit DRSIE for
>> completeness.
>> 
>> Signed-off-by: Roland Stigge <stigge@antcom.de>
>> 
>> --- Applies to v3.6-rc1
>> 
>> This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx,
>> not yet in mainline). Can you please test and double-check the
>> manuals of PNX4008 and LPC31xx? I only found this via the manual
>> of LPC32xx but assume it's the same for the others, also.
>> 
>> Thanks in advance!
> 
> Kevin, since the other manuals seem to be not easily available, can
> you please check?

Yes, would be great if someone at NXP could confirm that PNX actually
uses the same IP core for the I2C controller as LPCs do (which is
currently assumed by Linux anyway).

Thanks in advance,

Roland

RE: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Kevin Wells]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1352 bytes --]

>>> This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx,
>>> not yet in mainline). Can you please test and double-check the
>>> manuals of PNX4008 and LPC31xx? I only found this via the manual
>>> of LPC32xx but assume it's the same for the others, also.
>>> 
>>> Thanks in advance!
>> 
>> Kevin, since the other manuals seem to be not easily available, can
>> you please check?
>
>Yes, would be great if someone at NXP could confirm that PNX actually
>uses the same IP core for the I2C controller as LPCs do (which is
>currently assumed by Linux anyway).

I've never had my hands on a PNX4008 chip at NXP, but I do believe they
are the same IP. That specific I2C IP was used in a number of NXP/Phillips
chips besides the PNX4008/LPC32xx. I don't think there are any PNX4008's in
the wild, and even working in NXP, I can't find any non-marketing reference
material for that part (including the user manual).

The pnx I2C block is used in the LPC32xx (3 instances), the LPC313x,
LPC2000 (1 instance for USB OTG), and the LPC17xx/LPC40xx (1 instance for
USB OTG). The LPC2000, LPC17xx, and LPC40xx devices also use another
completely different (non-pnx) I2C block for non-OTG I2C.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Roland Stigge]

On 08/20/2012 06:26 PM, Kevin Wells wrote:
>>>> This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx,
>>>> not yet in mainline). Can you please test and double-check the
>>>> manuals of PNX4008 and LPC31xx? I only found this via the manual
>>>> of LPC32xx but assume it's the same for the others, also.
>>>>
>>>> Thanks in advance!
>>>
>>> Kevin, since the other manuals seem to be not easily available, can
>>> you please check?
>>
>> Yes, would be great if someone at NXP could confirm that PNX actually
>> uses the same IP core for the I2C controller as LPCs do (which is
>> currently assumed by Linux anyway).
> 
> I've never had my hands on a PNX4008 chip at NXP, but I do believe they
> are the same IP. That specific I2C IP was used in a number of NXP/Phillips
> chips besides the PNX4008/LPC32xx. I don't think there are any PNX4008's in
> the wild, and even working in NXP, I can't find any non-marketing reference
> material for that part (including the user manual).

Considering this, it might be a good idea to remove support for PNX4008
(arch/arm/mach-pnx4008/) altogether. It's hard to maintain support for
hardware which isn't available, even at NXP. It would also simplify
maintenance of mach-lpc32xx because the overlap currently makes me
always wonder if the respective changes still work with mach-pnx4008.

Any opposition?

Roland


PS: I just wonder how mach-pnx4008 came into the kernel at all...

Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Arnd Bergmann]

On Monday 20 August 2012, Roland Stigge wrote:
> On 08/20/2012 06:26 PM, Kevin Wells wrote:
> > I've never had my hands on a PNX4008 chip at NXP, but I do believe they
> > are the same IP. That specific I2C IP was used in a number of NXP/Phillips
> > chips besides the PNX4008/LPC32xx. I don't think there are any PNX4008's in
> > the wild, and even working in NXP, I can't find any non-marketing reference
> > material for that part (including the user manual).
> 
> Considering this, it might be a good idea to remove support for PNX4008
> (arch/arm/mach-pnx4008/) altogether. It's hard to maintain support for
> hardware which isn't available, even at NXP. It would also simplify
> maintenance of mach-lpc32xx because the overlap currently makes me
> always wonder if the respective changes still work with mach-pnx4008.
> 
> Any opposition?
> 
> 
> PS: I just wonder how mach-pnx4008 came into the kernel at all...

According to the git logs, Vitaly Wool originally added support for the
platform in 2006 when working at MontaVista, and that year was also the
last time he or anyone else from that company contributed anything in
that directory. Russell was the only other person to make substantial
contributions to it, but they all seem to be cross-platform changes.

In the platform code, there is only a single board number reserved,
with the name of the SoC: MACHINE_START(PNX4008, "Philips PNX4008").
This indicates that whoever was actually using the code did not have
their board code upstream and relied on out-of-tree patches for the
platform.

>From all I can tell, the PNX4008 family probably went to ST-Ericsson,
not to NXP in the various acquisitions and mergers that happened
around NXP. This explains why Kevin has no documentation or hardware
for it. On the ST-Ericsson web site, I could find some information
about the PNX4908, presumably a follow-on chip, but not about PNX4008,
so I guess the company also considers the product line dead.

Finally, the chips seems to be targetted at mobile phones and was
introduced seven years ago, which is multiple generations of
products in that market, so probably people stopped caring about
them long ago, unlike embedded chips from the same era for other
markets.

I'd say let's wait for Vitaly to reply on this matter, if he doesn't
care about the code, we can kill it off in 3.7 or 3.8.

	Arnd

Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

On Mon, Aug 20, 2012 at 06:26:54PM +0200, Kevin Wells wrote:
> >>> This patch for i2c-pnx affects PNX4008 and LPC32xx (and LPC31xx,
> >>> not yet in mainline). Can you please test and double-check the
> >>> manuals of PNX4008 and LPC31xx? I only found this via the manual
> >>> of LPC32xx but assume it's the same for the others, also.
> >>> 
> >>> Thanks in advance!
> >> 
> >> Kevin, since the other manuals seem to be not easily available, can
> >> you please check?
> >
> >Yes, would be great if someone at NXP could confirm that PNX actually
> >uses the same IP core for the I2C controller as LPCs do (which is
> >currently assumed by Linux anyway).
> 
> I've never had my hands on a PNX4008 chip at NXP, but I do believe they
> are the same IP. That specific I2C IP was used in a number of NXP/Phillips
> chips besides the PNX4008/LPC32xx. I don't think there are any PNX4008's in
> the wild, and even working in NXP, I can't find any non-marketing reference
> material for that part (including the user manual).

Thanks for the heads up. I'll apply both patches, then.

Roland, this looks like 3.6-material?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Roland Stigge]

On 20/08/12 19:55, Wolfram Sang wrote:
>> I've never had my hands on a PNX4008 chip at NXP, but I do
>> believe they are the same IP. That specific I2C IP was used in a
>> number of NXP/Phillips chips besides the PNX4008/LPC32xx. I don't
>> think there are any PNX4008's in the wild, and even working in
>> NXP, I can't find any non-marketing reference material for that
>> part (including the user manual).
> 
> Thanks for the heads up. I'll apply both patches, then.
> 
> Roland, this looks like 3.6-material?

Yes, all bugfixes.

Thanks,

Roland

Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Roland Stigge]

Hi Wolfram,

On 20/08/12 19:55, Wolfram Sang wrote:
>> I've never had my hands on a PNX4008 chip at NXP, but I do
>> believe they are the same IP. That specific I2C IP was used in a
>> number of NXP/Phillips chips besides the PNX4008/LPC32xx. I don't
>> think there are any PNX4008's in the wild, and even working in
>> NXP, I can't find any non-marketing reference material for that
>> part (including the user manual).
> 
> Thanks for the heads up. I'll apply both patches, then.

Should I consider the two patches as applied now (where?
i2c-embedded?), or should I resend? ;-)

Thanks in advance,

Roland

Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On Mon, Sep 03, 2012 at 10:16:22PM +0200, Roland Stigge wrote:
> Hi Wolfram,
> 
> On 20/08/12 19:55, Wolfram Sang wrote:
> >> I've never had my hands on a PNX4008 chip at NXP, but I do
> >> believe they are the same IP. That specific I2C IP was used in a
> >> number of NXP/Phillips chips besides the PNX4008/LPC32xx. I don't
> >> think there are any PNX4008's in the wild, and even working in
> >> NXP, I can't find any non-marketing reference material for that
> >> part (including the user manual).
> > 
> > Thanks for the heads up. I'll apply both patches, then.
> 
> Should I consider the two patches as applied now (where?
> i2c-embedded?), or should I resend? ;-)

I will apply them these days and then let them stay in linux-next for a
few days. So, I'd think they should go in for rc6.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]