diff mbox

[v6,10/19] i2c: octeon: Add support for cn78xx chips

Message ID 878b7035b447b6700abc7ec79c375df090067ebc.1460387640.git.jglauber@cavium.com
State Superseded
Headers show

Commit Message

Jan Glauber April 11, 2016, 3:28 p.m. UTC
From: David Daney <david.daney@cavium.com>

cn78xx has a different interrupt architecture, so we have to manage
the interrupts differently.

Signed-off-by: David Daney <ddaney@caviumnetworks.com>
Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon.c | 131 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 120 insertions(+), 11 deletions(-)

Comments

Wolfram Sang April 20, 2016, 9:52 p.m. UTC | #1
On Mon, Apr 11, 2016 at 05:28:41PM +0200, Jan Glauber wrote:
> From: David Daney <david.daney@cavium.com>
> 
> cn78xx has a different interrupt architecture, so we have to manage
> the interrupts differently.

I'd appreciate if you could explain here why interrupts use NOAUTOEN and
have to be manually en-/disabled? This is surely unusual.

> 
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> ---
>  drivers/i2c/busses/i2c-octeon.c | 131 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 120 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
> index 0bb9f0b..fff1ac0 100644
> --- a/drivers/i2c/busses/i2c-octeon.c
> +++ b/drivers/i2c/busses/i2c-octeon.c
> @@ -11,6 +11,7 @@
>   * warranty of any kind, whether express or implied.
>   */
>  
> +#include <linux/atomic.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -112,11 +113,18 @@ struct octeon_i2c {
>  	wait_queue_head_t queue;
>  	struct i2c_adapter adap;
>  	int irq;
> +	int hlc_irq;		/* For cn7890 only */
>  	u32 twsi_freq;
>  	int sys_freq;
>  	void __iomem *twsi_base;
>  	struct device *dev;
>  	bool hlc_enabled;
> +	void (*int_en)(struct octeon_i2c *);
> +	void (*int_dis)(struct octeon_i2c *);
> +	void (*hlc_int_en)(struct octeon_i2c *);
> +	void (*hlc_int_dis)(struct octeon_i2c *);

I'd prefer having 'enable' and 'disable' written out, but I leave that
to you.

> +	atomic_t int_en_cnt;
> +	atomic_t hlc_int_en_cnt;
>  };
>
David Daney April 20, 2016, 10:28 p.m. UTC | #2
On 04/20/2016 02:52 PM, Wolfram Sang wrote:
> On Mon, Apr 11, 2016 at 05:28:41PM +0200, Jan Glauber wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> cn78xx has a different interrupt architecture, so we have to manage
>> the interrupts differently.
>
> I'd appreciate if you could explain here why interrupts use NOAUTOEN and
> have to be manually en-/disabled? This is surely unusual.

It is quite nice, isn't it?

Since there were enable bits in both the interrupt controller *and* the 
I2C bus hardware, we had redundancy that could profitably be eliminated.

Now with the advent of the cn78xx, the system is much better.  This 
redundancy has been eliminated and interrupts are enabled only in the 
interrupt controller.

Fortunately, the kernel supplies us with convenience functions for 
manipulating these interrupt enable bits (enable_irq() and 
disable_irq()).  Since we don't want to irqs enabled when the driver is 
probed we have to set NOAUTOEN before calling request_irq().

Personally, I preferred the old way where each device had interrupt 
enable bits that could be controlled independently of the interrupt 
controller.  I have been told that I should abandon my preference for 
that type of antiquated architecture and welcome the brave new world of 
the unified interrupt enable bit.  Never the less, I still feel some 
nostalgia for the old way.

The good news is that it is not very much work to add software support 
for the cn78xx style interrupt enabling/masking.  The hooks we add can 
also be used to support the MSI-X interrupts in the Thunder support 
patch that follows.

David.


>
>>
>> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
>> Signed-off-by: Jan Glauber <jglauber@cavium.com>
>> ---
>>   drivers/i2c/busses/i2c-octeon.c | 131 ++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 120 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
>> index 0bb9f0b..fff1ac0 100644
>> --- a/drivers/i2c/busses/i2c-octeon.c
>> +++ b/drivers/i2c/busses/i2c-octeon.c
>> @@ -11,6 +11,7 @@
>>    * warranty of any kind, whether express or implied.
>>    */
>>
>> +#include <linux/atomic.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/kernel.h>
>> @@ -112,11 +113,18 @@ struct octeon_i2c {
>>   	wait_queue_head_t queue;
>>   	struct i2c_adapter adap;
>>   	int irq;
>> +	int hlc_irq;		/* For cn7890 only */
>>   	u32 twsi_freq;
>>   	int sys_freq;
>>   	void __iomem *twsi_base;
>>   	struct device *dev;
>>   	bool hlc_enabled;
>> +	void (*int_en)(struct octeon_i2c *);
>> +	void (*int_dis)(struct octeon_i2c *);
>> +	void (*hlc_int_en)(struct octeon_i2c *);
>> +	void (*hlc_int_dis)(struct octeon_i2c *);
>
> I'd prefer having 'enable' and 'disable' written out, but I leave that
> to you.
>
>> +	atomic_t int_en_cnt;
>> +	atomic_t hlc_int_en_cnt;
>>   };
>>

--
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
Wolfram Sang April 25, 2016, 9:45 p.m. UTC | #3
> Personally, I preferred the old way where each device had interrupt enable
> bits that could be controlled independently of the interrupt controller.  I
> have been told that I should abandon my preference for that type of
> antiquated architecture and welcome the brave new world of the unified
> interrupt enable bit.  Never the less, I still feel some nostalgia for the
> old way.

:) I think I am with you on this one.
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
index 0bb9f0b..fff1ac0 100644
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -11,6 +11,7 @@ 
  * warranty of any kind, whether express or implied.
  */
 
+#include <linux/atomic.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -112,11 +113,18 @@  struct octeon_i2c {
 	wait_queue_head_t queue;
 	struct i2c_adapter adap;
 	int irq;
+	int hlc_irq;		/* For cn7890 only */
 	u32 twsi_freq;
 	int sys_freq;
 	void __iomem *twsi_base;
 	struct device *dev;
 	bool hlc_enabled;
+	void (*int_en)(struct octeon_i2c *);
+	void (*int_dis)(struct octeon_i2c *);
+	void (*hlc_int_en)(struct octeon_i2c *);
+	void (*hlc_int_dis)(struct octeon_i2c *);
+	atomic_t int_en_cnt;
+	atomic_t hlc_int_en_cnt;
 };
 
 /**
@@ -212,6 +220,58 @@  static void octeon_i2c_int_disable(struct octeon_i2c *i2c)
 	octeon_i2c_write_int(i2c, 0);
 }
 
+/**
+ * octeon_i2c_int_enable78 - enable the CORE interrupt
+ * @i2c: The struct octeon_i2c
+ *
+ * The interrupt will be asserted when there is non-STAT_IDLE state in the
+ * SW_TWSI_EOP_TWSI_STAT register.
+ */
+static void octeon_i2c_int_enable78(struct octeon_i2c *i2c)
+{
+	atomic_inc_return(&i2c->int_en_cnt);
+	enable_irq(i2c->irq);
+}
+
+static void __octeon_i2c_irq_disable(atomic_t *cnt, int irq)
+{
+	int count;
+
+	/*
+	 * The interrupt can be disabled in two places, but we only
+	 * want to make the disable_irq_nosync() call once, so keep
+	 * track with the atomic variable.
+	 */
+	count = atomic_dec_if_positive(cnt);
+	if (count >= 0)
+		disable_irq_nosync(irq);
+}
+
+/* disable the CORE interrupt */
+static void octeon_i2c_int_disable78(struct octeon_i2c *i2c)
+{
+	__octeon_i2c_irq_disable(&i2c->int_en_cnt, i2c->irq);
+}
+
+/**
+ * octeon_i2c_hlc_int_enable78 - enable the ST interrupt
+ * @i2c: The struct octeon_i2c
+ *
+ * The interrupt will be asserted when there is non-STAT_IDLE state in
+ * the SW_TWSI_EOP_TWSI_STAT register.
+ */
+static void octeon_i2c_hlc_int_enable78(struct octeon_i2c *i2c)
+{
+	atomic_inc_return(&i2c->hlc_int_en_cnt);
+	enable_irq(i2c->hlc_irq);
+}
+
+/* disable the ST interrupt */
+static void octeon_i2c_hlc_int_disable78(struct octeon_i2c *i2c)
+{
+	__octeon_i2c_irq_disable(&i2c->hlc_int_en_cnt, i2c->hlc_irq);
+}
+
 /*
  * Cleanup low-level state & enable high-level controller.
  */
@@ -258,7 +318,18 @@  static irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
 {
 	struct octeon_i2c *i2c = dev_id;
 
-	octeon_i2c_int_disable(i2c);
+	i2c->int_dis(i2c);
+	wake_up(&i2c->queue);
+
+	return IRQ_HANDLED;
+}
+
+/* HLC interrupt service routine */
+static irqreturn_t octeon_i2c_hlc_isr78(int irq, void *dev_id)
+{
+	struct octeon_i2c *i2c = dev_id;
+
+	i2c->hlc_int_dis(i2c);
 	wake_up(&i2c->queue);
 
 	return IRQ_HANDLED;
@@ -279,10 +350,10 @@  static int octeon_i2c_wait(struct octeon_i2c *i2c)
 {
 	long time_left;
 
-	octeon_i2c_int_enable(i2c);
+	i2c->int_en(i2c);
 	time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c),
 				       i2c->adap.timeout);
-	octeon_i2c_int_disable(i2c);
+	i2c->int_dis(i2c);
 	if (!time_left) {
 		dev_dbg(i2c->dev, "%s: timeout\n", __func__);
 		return -ETIMEDOUT;
@@ -378,11 +449,11 @@  static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
 {
 	int time_left;
 
-	octeon_i2c_hlc_int_enable(i2c);
+	i2c->hlc_int_en(i2c);
 	time_left = wait_event_interruptible_timeout(i2c->queue,
 					octeon_i2c_hlc_test_ready(i2c),
 					i2c->adap.timeout);
-	octeon_i2c_int_disable(i2c);
+	i2c->hlc_int_dis(i2c);
 	if (!time_left) {
 		octeon_i2c_hlc_int_clear(i2c);
 		dev_dbg(i2c->dev, "%s: timeout\n", __func__);
@@ -958,14 +1029,26 @@  static struct i2c_adapter octeon_i2c_ops = {
 static int octeon_i2c_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
+	int irq, result = 0, hlc_irq = 0;
 	struct resource *res_mem;
 	struct octeon_i2c *i2c;
-	int irq, result = 0;
-
-	/* All adaptors have an irq.  */
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	bool cn78xx_style;
+
+	cn78xx_style = of_device_is_compatible(node, "cavium,octeon-7890-twsi");
+	if (cn78xx_style) {
+		hlc_irq = platform_get_irq(pdev, 0);
+		if (hlc_irq < 0)
+			return hlc_irq;
+
+		irq = platform_get_irq(pdev, 2);
+		if (irq < 0)
+			return irq;
+	} else {
+		/* All adaptors have an irq.  */
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0)
+			return irq;
+	}
 
 	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c) {
@@ -1000,6 +1083,31 @@  static int octeon_i2c_probe(struct platform_device *pdev)
 
 	i2c->irq = irq;
 
+	if (cn78xx_style) {
+		i2c->hlc_irq = hlc_irq;
+
+		i2c->int_en = octeon_i2c_int_enable78;
+		i2c->int_dis = octeon_i2c_int_disable78;
+		i2c->hlc_int_en = octeon_i2c_hlc_int_enable78;
+		i2c->hlc_int_dis = octeon_i2c_hlc_int_disable78;
+
+		irq_set_status_flags(i2c->irq, IRQ_NOAUTOEN);
+		irq_set_status_flags(i2c->hlc_irq, IRQ_NOAUTOEN);
+
+		result = devm_request_irq(&pdev->dev, i2c->hlc_irq,
+					  octeon_i2c_hlc_isr78, 0,
+					  DRV_NAME, i2c);
+		if (result < 0) {
+			dev_err(i2c->dev, "failed to attach interrupt\n");
+			goto out;
+		}
+	} else {
+		i2c->int_en = octeon_i2c_int_enable;
+		i2c->int_dis = octeon_i2c_int_disable;
+		i2c->hlc_int_en = octeon_i2c_hlc_int_enable;
+		i2c->hlc_int_dis = octeon_i2c_int_disable;
+	}
+
 	result = devm_request_irq(&pdev->dev, i2c->irq,
 				  octeon_i2c_isr, 0, DRV_NAME, i2c);
 	if (result < 0) {
@@ -1046,6 +1154,7 @@  static int octeon_i2c_remove(struct platform_device *pdev)
 
 static const struct of_device_id octeon_i2c_match[] = {
 	{ .compatible = "cavium,octeon-3860-twsi", },
+	{ .compatible = "cavium,octeon-7890-twsi", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, octeon_i2c_match);