sfc: Use kernel I2C system and i2c-algo-bit driver

PATCH In review Ben Hutchings 3 participants 7 messages May 30, 2008 – May 31, 2008 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ben Hutchings

author

2008-05-30 21:27 UTC #1 lore raw

PATCH 1/2 sfc: Use kernel I2C system and i2c-algo-bit driver

Remove our own implementation of I2C bit-banging.

Signed-off-by Ben Hutchings <[email protected]>

---
 drivers/net/sfc/Kconfig      |    2 +
 drivers/net/sfc/Makefile     |    2 +-
 drivers/net/sfc/boards.c     |    2 +-
 drivers/net/sfc/boards.h     |    3 +-
 drivers/net/sfc/efx.c        |    2 +
 drivers/net/sfc/falcon.c     |   72 ++++++---
 drivers/net/sfc/i2c-direct.c |  381 ------------------------------------------
 drivers/net/sfc/i2c-direct.h |   91 ----------
 drivers/net/sfc/net_driver.h |   11 +-
 drivers/net/sfc/sfe4001.c    |  126 ++++++++-------
 10 files changed, 133 insertions(+), 559 deletions(-)
 delete mode 100644 drivers/net/sfc/i2c-direct.c
 delete mode 100644 drivers/net/sfc/i2c-direct.h

drivers/net/sfc/Kconfig +2 -0

index dbad95c..3be13b5 100644
@@ -4,6 +4,8 @@ config SFC
 	select MII
 	select INET_LRO
 	select CRC32
+	select I2C
+	select I2C_ALGOBIT
 	help
 	  This driver supports 10-gigabit Ethernet cards based on
 	  the Solarflare Communications Solarstorm SFC4000 controller.

drivers/net/sfc/Makefile +1 -1

index 1d2daee..c8f5704 100644
@@ -1,5 +1,5 @@
 sfc-y			+= efx.o falcon.o tx.o rx.o falcon_xmac.o \
-			   i2c-direct.o selftest.o ethtool.o xfp_phy.o \
+			   selftest.o ethtool.o xfp_phy.o \
 			   mdio_10g.o tenxpress.o boards.o sfe4001.o
 
 obj-$(CONFIG_SFC)	+= sfc.o

drivers/net/sfc/boards.c +1 -1

index 7fc0328..d3d3dd0 100644
@@ -109,7 +109,7 @@ static struct efx_board_data board_data[] = {
 	[EFX_BOARD_INVALID] =
 	{NULL,	    NULL,                  dummy_init},
 	[EFX_BOARD_SFE4001] =
-	{"SFE4001", "10GBASE-T adapter",   sfe4001_poweron},
+	{"SFE4001", "10GBASE-T adapter",   sfe4001_init},
 	[EFX_BOARD_SFE4002] =
 	{"SFE4002", "XFP adapter",         sfe4002_init},
 };

drivers/net/sfc/boards.h +1 -2

index 695764d..e5e8443 100644
@@ -20,8 +20,7 @@ enum efx_board_type {
 };
 
 extern int efx_set_board_info(struct efx_nic *efx, u16 revision_info);
-extern int sfe4001_poweron(struct efx_nic *efx);
-extern void sfe4001_poweroff(struct efx_nic *efx);
+extern int sfe4001_init(struct efx_nic *efx);
 /* Are we putting the PHY into flash config mode */
 extern unsigned int sfe4001_phy_flash_cfg;

drivers/net/sfc/efx.c +2 -0

index 4497606..74265d8 100644
@@ -1815,6 +1815,7 @@ static struct efx_board efx_dummy_board_info = {
 	.init    = efx_nic_dummy_op_int,
 	.init_leds = efx_port_dummy_op_int,
 	.set_fault_led = efx_port_dummy_op_blink,
+	.fini	= efx_port_dummy_op_void,
 };
 
 /**************************************************************************

@@ -1941,6 +1942,7 @@ static void efx_pci_remove_main(struct efx_nic *efx)
 	efx_fini_port(efx);
 
 	/* Shutdown the board, then the NIC and board state */
+	efx->board_info.fini(efx);
 	falcon_fini_interrupt(efx);
 
 	efx_fini_napi(efx);

drivers/net/sfc/falcon.c +52 ~~-20~~

131 lines not on this page · raw message

drivers/net/sfc/i2c-direct.c deleted +0 ~~-381~~

381 lines not on this page · raw message

drivers/net/sfc/i2c-direct.h deleted +0 ~~-91~~

91 lines not on this page · raw message

drivers/net/sfc/net_driver.h +8 -3

index 5e20e75..d803b86 100644
@@ -26,10 +26,10 @@
 #include <linux/highmem.h>
 #include <linux/workqueue.h>
 #include <linux/inet_lro.h>
+#include <linux/i2c.h>
 
 #include "enum.h"
 #include "bitfield.h"
-#include "i2c-direct.h"
 
 #define EFX_MAX_LRO_DESCRIPTORS 8
 #define EFX_MAX_LRO_AGGR MAX_SKB_FRAGS

@@ -418,7 +418,10 @@ struct efx_blinker {
  * @init_leds: Sets up board LEDs
  * @set_fault_led: Turns the fault LED on or off
  * @blink: Starts/stops blinking
+ * @fini: Cleanup function
  * @blinker: used to blink LEDs in software
+ * @hwmon_client: I2C client for hardware monitor
+ * @ioexp_client: I2C client for power/port control
  */
 struct efx_board {
 	int type;

@@ -431,7 +434,9 @@ struct efx_board {
 	int (*init_leds)(struct efx_nic *efx);
 	void (*set_fault_led) (struct efx_nic *efx, int state);
 	void (*blink) (struct efx_nic *efx, int start);
+	void (*fini) (struct efx_nic *nic);
 	struct efx_blinker blinker;
+	struct i2c_client *hwmon_client, *ioexp_client;
 };
 
 #define STRING_TABLE_LOOKUP(val, member)	\

@@ -618,7 +623,7 @@ union efx_multicast_hash {
  * @membase: Memory BAR value
  * @biu_lock: BIU (bus interface unit) lock
  * @interrupt_mode: Interrupt mode
- * @i2c: I2C interface
+ * @i2c_adap: I2C adapter
  * @board_info: Board-level information
  * @state: Device state flag. Serialised by the rtnl_lock.
  * @reset_pending: Pending reset method (normally RESET_TYPE_NONE)

@@ -686,7 +691,7 @@ struct efx_nic {
 	spinlock_t biu_lock;
 	enum efx_int_mode interrupt_mode;
 
-	struct efx_i2c_interface i2c;
+	struct i2c_adapter i2c_adap;
 	struct efx_board board_info;
 
 	enum nic_state state;

drivers/net/sfc/sfe4001.c +66 ~~-60~~

201 lines not on this page · raw message

Signature · 2 lines

Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Jean Delvare

2008-05-31 11:19 UTC #3 lore raw

In reply to Ben Hutchings

Hi Ben,

Quoted 1 line · Ben Hutchings

Remove our own implementation of I2C bit-banging.

Thanks a lot for doing this. It's always nice to see hundreds of lines of code being removed from the kernel :) Some comments:

Quoted 29 lines

Signed-off-by Ben Hutchings <[email protected]>

---
 drivers/net/sfc/Kconfig      |    2 +
 drivers/net/sfc/Makefile     |    2 +-
 drivers/net/sfc/boards.c     |    2 +-
 drivers/net/sfc/boards.h     |    3 +-
 drivers/net/sfc/efx.c        |    2 +
 drivers/net/sfc/falcon.c     |   72 ++++++---
 drivers/net/sfc/i2c-direct.c |  381 ------------------------------------------
 drivers/net/sfc/i2c-direct.h |   91 ----------
 drivers/net/sfc/net_driver.h |   11 +-
 drivers/net/sfc/sfe4001.c    |  126 ++++++++-------
 10 files changed, 133 insertions(+), 559 deletions(-)
 delete mode 100644 drivers/net/sfc/i2c-direct.c
 delete mode 100644 drivers/net/sfc/i2c-direct.h

(...)

drivers/net/sfc/falcon.c +3 -3

index d3f749c..8acd53d 100644
(...)
-static struct efx_i2c_bit_operations falcon_i2c_bit_operations = {
-	.setsda		= falcon_setsdascl,
-	.setscl		= falcon_setsdascl,
+static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
+	.setsda		= falcon_setsda,
+	.setscl		= falcon_setscl,
 	.getsda		= falcon_getsda,

drivers/net/sfc/falcon.c static inline int falcon_event_present(efx_qword_t *event) · line 232

Quoted 10 lines

drivers/net/sfc/falcon.c +7 -1

 	.getscl		= falcon_getscl,
 	.udelay		= 100,
-	.mdelay		= 10,
+	/*
+	 * This is the number of system clock ticks after which
+	 * i2c-algo-bit gives up waiting for SCL to become high.
+	 * It must be at least 2 since the first tick can happen
+	 * immediately after it starts waiting.
+	 */
+	.timeout	= 2,

This is unreasonably low at HZ=1000 (2 ms). The SMBus specification states that clients can hold the clock line low for up to 50 ms if they need additional time to process the previous commands. So I would use msecs_to_jiffies(50) (don't forget to include <linux/jiffies.h>.)

Quoted 17 lines

};

/**************************************************************************

drivers/net/sfc/falcon.c +0 -6

@@ -2403,12 +2425,6 @@ int falcon_probe_nic(struct efx_nic *efx)
 	struct falcon_nic_data *nic_data;
 	int rc;
 
-	/* Initialise I2C interface state */
-	efx->i2c.efx = efx;
-	efx->i2c.op = &falcon_i2c_bit_operations;
-	efx->i2c.sda = 1;
-	efx->i2c.scl = 1;
-
 	/* Allocate storage for hardware specific data */
 	nic_data = kzalloc(sizeof(*nic_data), GFP_KERNEL);
 	efx->nic_data = nic_data;

drivers/net/sfc/falcon.c int falcon_probe_nic(struct efx_nic *efx) · line 2480

Quoted 6 lines

drivers/net/sfc/falcon.c +3 -0

@@ -2459,6 +2475,18 @@ int falcon_probe_nic(struct efx_nic *efx)
 	if (rc)
 		goto fail5;
 
+	/* Initialise I2C adapter */
+ 	efx->i2c_adap.owner = THIS_MODULE;
+ 	efx->i2c_adap.class = I2C_CLASS_HWMON;

I doubt you want to do this. This would let any hardware monitoring driver probe your bus for a device, while presumably you already know which hardware monitoring device is present and you want to instantiate the i2c client yourself. This will probably become clearer when you start using the lm87 driver and modify it to support new-style i2c binding.

drivers/net/sfc/falcon.c int falcon_probe_nic(struct efx_nic *efx) · line 2485

Quoted 5 lines

drivers/net/sfc/falcon.c +5 -0

+	nic_data->i2c_data = falcon_i2c_bit_operations;
+	nic_data->i2c_data.data = efx;
+ 	efx->i2c_adap.algo_data = &nic_data->i2c_data;
+	efx->i2c_adap.dev.parent = &efx->pci_dev->dev;
+	strcpy(efx->i2c_adap.name, "SFC4000 GPIO");

Please always use strlcpy.

Quoted 8 lines

drivers/net/sfc/falcon.c +4 -0

+	rc = i2c_bit_add_bus(&efx->i2c_adap);
+	if (rc)
+		goto fail5;
+
 	return 0;
 
  fail5:

drivers/net/sfc/falcon.c int falcon_init_nic(struct efx_nic *efx) · line 2667

Quoted 7 lines

drivers/net/sfc/falcon.c +4 -0

@@ -2633,6 +2661,10 @@ int falcon_init_nic(struct efx_nic *efx)
 void falcon_remove_nic(struct efx_nic *efx)
 {
 	struct falcon_nic_data *nic_data = efx->nic_data;
+	int rc;
+
+	rc = i2c_del_adapter(&efx->i2c_adap);
+	BUG_ON(rc);

That's pretty aggressive and probably not needed.

Quoted 3 lines

drivers/net/sfc/falcon.c +0 -0
 	falcon_free_buffer(efx, &efx->irq_status);
 

Signature · 1 line

Jean Delvare

Ben Hutchings

author

2008-05-31 11:27 UTC #4 lore raw

Jean Delvare wrote:
[...]

Quoted 19 lines

drivers/net/sfc/falcon.c +7 -4

index d3f749c..8acd53d 100644
(...)
-static struct efx_i2c_bit_operations falcon_i2c_bit_operations = {
-	.setsda		= falcon_setsdascl,
-	.setscl		= falcon_setsdascl,
+static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
+	.setsda		= falcon_setsda,
+	.setscl		= falcon_setscl,
 	.getsda		= falcon_getsda,
 	.getscl		= falcon_getscl,
 	.udelay		= 100,
-	.mdelay		= 10,
+	/*
+	 * This is the number of system clock ticks after which
+	 * i2c-algo-bit gives up waiting for SCL to become high.
+	 * It must be at least 2 since the first tick can happen

Quoted 8 lines

drivers/net/sfc/falcon.c static inline int falcon_event_present(efx_qword_t *event) · line 232
Quoted 3 lines
drivers/net/sfc/falcon.c +3 -0
+	 * immediately after it starts waiting.
+	 */
+	.timeout	= 2,
This is unreasonably low at HZ=1000 (2 ms). The SMBus specification states that clients can hold the clock line low for up to 50 ms if they need additional time to process the previous commands. So I would use msecs_to_jiffies(50) (don't forget to include <linux/jiffies.h>.)

OK. We can't use msecs_to_jiffies() in a static initialiser, though.

[...]

Quoted 4 lines

drivers/net/sfc/falcon.c +0 -0

@@ -2459,6 +2475,18 @@ int falcon_probe_nic(struct efx_nic *efx)
 	if (rc)
 		goto fail5;

Quoted 10 lines

drivers/net/sfc/falcon.c int falcon_probe_nic(struct efx_nic *efx) · line 2480
Quoted 3 lines
drivers/net/sfc/falcon.c +3 -0
+	/* Initialise I2C adapter */
+ 	efx->i2c_adap.owner = THIS_MODULE;
+ 	efx->i2c_adap.class = I2C_CLASS_HWMON;
I doubt you want to do this. This would let any hardware monitoring driver probe your bus for a device, while presumably you already know which hardware monitoring device is present and you want to instantiate the i2c client yourself. This will probably become clearer when you start using the lm87 driver and modify it to support new-style i2c binding.

So the class should be, what, 0?

Quoted 7 lines

drivers/net/sfc/falcon.c int falcon_probe_nic(struct efx_nic *efx) · line 2485
Quoted 5 lines
drivers/net/sfc/falcon.c +5 -0
+	nic_data->i2c_data = falcon_i2c_bit_operations;
+	nic_data->i2c_data.data = efx;
+ 	efx->i2c_adap.algo_data = &nic_data->i2c_data;
+	efx->i2c_adap.dev.parent = &efx->pci_dev->dev;
+	strcpy(efx->i2c_adap.name, "SFC4000 GPIO");
Please always use strlcpy.

OK. I thought about it but it didn't seem worthwhile for a short constant string.

Quoted 6 lines

drivers/net/sfc/falcon.c +4 -0

+	rc = i2c_bit_add_bus(&efx->i2c_adap);
+	if (rc)
+		goto fail5;
+
 	return 0;

Quoted 11 lines

drivers/net/sfc/falcon.c int falcon_init_nic(struct efx_nic *efx) · line 2667
Quoted 9 lines
drivers/net/sfc/falcon.c +4 -0
  fail5:
@@ -2633,6 +2661,10 @@ int falcon_init_nic(struct efx_nic *efx)
 void falcon_remove_nic(struct efx_nic *efx)
 {
 	struct falcon_nic_data *nic_data = efx->nic_data;
+	int rc;
+
+	rc = i2c_del_adapter(&efx->i2c_adap);
+	BUG_ON(rc);
That's pretty aggressive and probably not needed.

We have no way of cancelling the removal at this point, so if the adapter is not properly deleted then it will hang around referring to a NIC structure that has gone away.

Ben.

Signature · 2 lines

Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Jean Delvare

2008-05-31 15:07 UTC #5 lore raw

Quoted 16 lines · Ben Hutchings

Quoted 4 lines · Jean Delvare
Quoted 4 lines
drivers/net/sfc/falcon.c +0 -0
@@ -2459,6 +2475,18 @@ int falcon_probe_nic(struct efx_nic *efx)
 	if (rc)
 		goto fail5;
 
Quoted 10 lines
drivers/net/sfc/falcon.c int falcon_probe_nic(struct efx_nic *efx) · line 2480
Quoted 3 lines
drivers/net/sfc/falcon.c +3 -0
+	/* Initialise I2C adapter */
+ 	efx->i2c_adap.owner = THIS_MODULE;
+ 	efx->i2c_adap.class = I2C_CLASS_HWMON;
I doubt you want to do this. This would let any hardware monitoring driver probe your bus for a device, while presumably you already know which hardware monitoring device is present and you want to instantiate the i2c client yourself. This will probably become clearer when you start using the lm87 driver and modify it to support new-style i2c binding.

Quoted 1 line

So the class should be, what, 0?

Yes. Assuming a kzalloc'd structure, you can simply omit it.

Quoted 10 lines

Quoted 7 lines
drivers/net/sfc/falcon.c int falcon_probe_nic(struct efx_nic *efx) · line 2485
Quoted 5 lines
drivers/net/sfc/falcon.c +5 -0
+	nic_data->i2c_data = falcon_i2c_bit_operations;
+	nic_data->i2c_data.data = efx;
+ 	efx->i2c_adap.algo_data = &nic_data->i2c_data;
+	efx->i2c_adap.dev.parent = &efx->pci_dev->dev;
+	strcpy(efx->i2c_adap.name, "SFC4000 GPIO");
Please always use strlcpy.
OK. I thought about it but it didn't seem worthwhile for a short constant string.

Until someone changes the string for a longer one (it's common to include the I/O base) or the structure field is shortened to save some memory... Always use strlcpy. In fact I'd like to see strcpy removed from the kernel, it's almost always the wrong function to use.

Signature · 1 line

Jean Delvare

Ben Hutchings

author

2008-05-30 21:28 UTC #6 lore raw

PATCH 2/2 sfc: Reduce I2C udelay to 5 resulting in a clock frequency of 100 kHz

In reply to a message not in the archive

Signed-off-by Ben Hutchings <[email protected]>

---
 drivers/net/sfc/falcon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/sfc/falcon.c +1 -1

index 8acd53d..8cb5798 100644
@@ -222,7 +222,7 @@ static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
 	.setscl		= falcon_setscl,
 	.getsda		= falcon_getsda,
 	.getscl		= falcon_getscl,
-	.udelay		= 100,
+	.udelay		= 5,
 	/*
 	 * This is the number of system clock ticks after which
 	 * i2c-algo-bit gives up waiting for SCL to become high.

Signature · 2 lines

Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Jean Delvare

Acked-by on v1 2/2

2008-05-31 11:19 UTC #7 lore raw

Re: [PATCH 2/2] sfc: Reduce I2C udelay to 5 resulting in a clock frequency of 100 kHz

Quoted 11 lines · Ben Hutchings

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sfc/falcon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/sfc/falcon.c +0 -0
index 8acd53d..8cb5798 100644

drivers/net/sfc/falcon.c static struct i2c_algo_bit_data falcon_i2c_bit_operations = { · line 228

Quoted 8 lines

drivers/net/sfc/falcon.c +1 -1

@@ -222,7 +222,7 @@ static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
 	.setscl		= falcon_setscl,
 	.getsda		= falcon_getsda,
 	.getscl		= falcon_getscl,
-	.udelay		= 100,
+	.udelay		= 5,
 	/*
 	 * This is the number of system clock ticks after which
 	 * i2c-algo-bit gives up waiting for SCL to become high.

Acked-by Jean Delvare <[email protected]>

Signature · 1 line

Jean Delvare