[5/6] i2c: Xilinx IIC: make reset after TX error configurable
diff mbox

Message ID 1438344034-20211-7-git-send-email-rabel@cit-ec.uni-bielefeld.de
State New
Headers show

Commit Message

Robert ABEL July 31, 2015, noon UTC
CONFIG_I2C_XILINX_ERRATA makes resetting XIIC after every
Master Transmit error configurable.
Also mention proper module name for XIIC kernel module.

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 drivers/i2c/busses/Kconfig    | 9 ++++++++-
 drivers/i2c/busses/i2c-xiic.c | 9 +++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Michal Simek Aug. 3, 2015, 5:34 a.m. UTC | #1
On 07/31/2015 02:00 PM, Robert ABEL wrote:
> CONFIG_I2C_XILINX_ERRATA makes resetting XIIC after every
> Master Transmit error configurable.
> Also mention proper module name for XIIC kernel module.

Datasheet? version.

> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/i2c/busses/Kconfig    | 9 ++++++++-
>  drivers/i2c/busses/i2c-xiic.c | 9 +++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 46d5488..3255e89 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -886,7 +886,14 @@ config I2C_XILINX
>  	  Xilinx I2C controller.
>  
>  	  This driver can also be built as a module.  If so, the module
> -	  will be called xilinx_i2c.
> +	  will be called i2c-xiic.
> +
> +config I2C_XILINX_ERRATA
> +	bool "Reset on "
> +	depends on I2C_XILINX
> +	help
> +	  By enabling this option, the Xilinx I2C Controller will be reset
> +	  after Master Transmit Errors.
>  
>  config I2C_XLR
>  	tristate "XLR I2C support"
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 5c9897e..6a834bc 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -509,6 +509,15 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  			break;
>  		}
>  
> +#if defined(CONFIG_I2C_XILINX_ERRATA)
> +		if (!(msg->flags & I2C_M_RD)) {
> +			/* dynamic mode seem to suffer from problems if we just flush
> +			 * fifos and the next message is a TX with len 0 (only addr)
> +			 * reset the IP instead of just flushing fifos
> +			 */
> +			xiic_reinit(i2c);
> +		}
> +#endif
>  	}
>  	
>  	/* Receive FIFO is full */
> 

Make it DT configurable.

Thanks,
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert ABEL Aug. 3, 2015, 7:50 a.m. UTC | #2
Hi Michal,

On Mon, Aug 3, 2015 at 7:34 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> Datasheet? version.
>
> Make it DT configurable.

I'm not sure what you're asking here. This errata isn't covered by any
datasheets and I don't have the time to confirm whether it actually
exists.
I'm thinking it might have been an issue with the old ISR and not the
IP itself (it works fine in simulation when I cause a TX error, then
transmit only SLA afterwards).
I cannot test exhaustively on hardware right now, as I'm not able to
monitor the bus directly on my development platform.

Because of the above 'hunch', I didn't make this DT configurable,
because I feel either the bug is present and thus the code should
always be run, or the bug doesn't exist.
I don't think it depends on the hardware device, so hence no DT binding.

Regards

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Simek Aug. 6, 2015, 9:18 a.m. UTC | #3
On 08/03/2015 09:50 AM, Robert Abel wrote:
> Hi Michal,
> 
> On Mon, Aug 3, 2015 at 7:34 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>> Datasheet? version.
>>
>> Make it DT configurable.
> 
> I'm not sure what you're asking here. This errata isn't covered by any
> datasheets and I don't have the time to confirm whether it actually
> exists.
> I'm thinking it might have been an issue with the old ISR and not the
> IP itself (it works fine in simulation when I cause a TX error, then
> transmit only SLA afterwards).
> I cannot test exhaustively on hardware right now, as I'm not able to
> monitor the bus directly on my development platform.

Every patch like this has to fix something. If some certain IP versions
are affected then it should be listed at least one version.


> Because of the above 'hunch', I didn't make this DT configurable,
> because I feel either the bug is present and thus the code should
> always be run, or the bug doesn't exist.
> I don't think it depends on the hardware device, so hence no DT binding.

Huh. Don't get what you are saying here. Based on your description it is
HW bug and it is completely fine to have dt property to avoid this bug.
Having another config property is just not acceptable solution now.

Thanks,
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert ABEL Aug. 6, 2015, 9:40 a.m. UTC | #4
Hi Michal,

On Thu, Aug 6, 2015 at 11:18 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> Every patch like this has to fix something. If some certain IP versions
> are affected then it should be listed at least one version.

Please re-read my email. I said:
- I do NOT know of any bug.
- all information about a possible bug come from that three lines of
commentary which are in the _original_ mainline driver.
- there is no errata mentioned in any of the datasheets I read.

> Huh. Don't get what you are saying here. Based on your description it is
> HW bug and it is completely fine to have dt property to avoid this bug.
> Having another config property is just not acceptable solution now.

You misunderstood. To recapitulate:

- I have not seen this bug in action.-
- I have not observed this bug in simulation.
- I don't think it exists in current IP.
- I personally think any odd behavior the original committers might
have observed came down to bugs in their ISR.

If any errata had existed in previous IP, I would expect the
datasheets to mention this-- they don't. I have looked at datasheets
axi_iic_ds756 v1.01a, axi_iic_ds756 v1.01b, axi_iic_ds756 v1.02a and
xps_iic_ds606 v2.03a.
I have simulated axi_iic v1.02a and successfully use axi_iic v1.02a in
hardware with my patched driver without the CONFIG_I2C_XILINX_ERRATA
configuration option configured, i.e. # CONFIG_I2C_XILINX_ERRATA is
not set.

On the off-chance this bug _does_ exist in previous or current IP, I
provided this compile-time configurable option to enable the _old_
mainline behavior of resetting the IP after every TX error so as to
not break somebody's platform.
And that's why it isn't a DT property. It can always be promoted to a
DT property when the existence of any errata were actually verified,
but I have neither time nor inclination to do so.

Therefore, I _don't know_ which version of the IP _if any_ is
affected. If you require more information, I suggest you inquire with
Richard Röjfors or possibly Ben Dooks about it. Richard Röjfors is to
git blame for the reset-after-Tx-error functionality and the comment
mentioning a possible errata.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 46d5488..3255e89 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -886,7 +886,14 @@  config I2C_XILINX
 	  Xilinx I2C controller.
 
 	  This driver can also be built as a module.  If so, the module
-	  will be called xilinx_i2c.
+	  will be called i2c-xiic.
+
+config I2C_XILINX_ERRATA
+	bool "Reset on "
+	depends on I2C_XILINX
+	help
+	  By enabling this option, the Xilinx I2C Controller will be reset
+	  after Master Transmit Errors.
 
 config I2C_XLR
 	tristate "XLR I2C support"
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 5c9897e..6a834bc 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -509,6 +509,15 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 			break;
 		}
 
+#if defined(CONFIG_I2C_XILINX_ERRATA)
+		if (!(msg->flags & I2C_M_RD)) {
+			/* dynamic mode seem to suffer from problems if we just flush
+			 * fifos and the next message is a TX with len 0 (only addr)
+			 * reset the IP instead of just flushing fifos
+			 */
+			xiic_reinit(i2c);
+		}
+#endif
 	}
 	
 	/* Receive FIFO is full */