Skip to contentSkip to conversation

i2c: npcm: add NPCM i2c controller driver

PATCH Superseded Tali Perry 3 participants 8 messages Nov 21, 2019 – Feb 25, 2020

Subsystems (3)

v7 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Tali Perry

author maintainer
#0 lore raw

v7 cover i2c: npcm: add NPCM i2c controller driver

This patch set adds i2c controller support
for the Nuvoton NPCM Baseboard Management Controller (BMC).

NPCM7xx includes 16 I2C controllers. This driver operates the controller. This module also includes a slave mode.

---

v7 -> v6:
- Rebased on Linux 5.4-rc8 (was Linux 5.4-rc7).
- Fix issue found by kbuild test robot (redundant include).
- Note: left a warning related to fall through. This fall through is intentional.

v6 -> v5:
- Update documentation

v5 -> v4:
- support recovery
- master-slave switch support needed for IPMB

v4 -> v3:
- typo on cover letter.

v3 -> v2:
- fix dt binding: compatible name: omit "bus"

v2 -> v1:
- run check patch in strict mode.
- use linux crc.
- define regs in constant offset without base.
- remove debug prints.
- no declarations for local functions.

v1: initial version

---
Tali Perry (2):
Added device tree binding documentation for Nuvoton BMC NPCM I2C
controller.
Add Nuvoton NPCM BMC I2C controller driver.

 .../devicetree/bindings/i2c/i2c-npcm7xx.txt   |   29 +
 drivers/i2c/busses/Kconfig                    |   11 +
 drivers/i2c/busses/Makefile                   |    1 +
 drivers/i2c/busses/i2c-npcm7xx.c              | 2485 +++++++++++++++++
 4 files changed, 2526 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt
 create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c

base-commit: af42d3466bdc8f39806b26f593604fdc54140bcb

Signature · 1 line
2.22.0
author maintainer
#1 lore raw

PATCH v7 1/2 dt-bindings: i2c: npcm7xx: add NPCM I2C controller documentation

Added device tree binding documentation for Nuvoton BMC NPCM I2C controller.

---

 .../devicetree/bindings/i2c/i2c-npcm7xx.txt   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt
Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt new file +29 -0
new file mode 100644
index 000000000000..d36cc20742fd
@@ -0,0 +1,29 @@
+Nuvoton NPCM7XX I2C bus
+
+The NPCM750x includes sixteen I2C bus controllers. All Controllers support
+both master and slave mode. Each controller has two 16 byte HW FIFO for TX and
+RX.
+
+Required properties:
+- compatible      : must be "nuvoton,npcm750-i2c"
+- reg             : Offset and length of the register set for the device.
+- interrupts      : Contain the I2C interrupt with flags for falling edge.
+- clocks          : phandle of I2C reference clock.
+
+Optional:
+- bus-frequency   : Contain the I2C bus frequency,
+			the default I2C bus frequency is 100000.
+- pinctrl-0       : for example <&smbX_pins>, X is module number
+			(on NPCM7XX it's 0 to 15)
+- pinctrl-names   : should be set to "default"
+Example:
+
+	i2c0: i2c@80000 {
+		compatible = "nuvoton,npcm750-i2c";
+		reg = <0x80000 0x1000>;
+		clocks = <&clk NPCM7XX_CLK_APB2>;
+		bus-frequency = <100000>;
+		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&smb0_pins>;
+	};
Signature · 1 line
2.22.0
author maintainer
#2 lore raw

PATCH v7 2/2 i2c: npcm: Add Nuvoton NPCM I2C controller driver

In reply to Tali Perry

Add Nuvoton NPCM BMC i2c controller driver.

---
 drivers/i2c/busses/Kconfig       |   11 +
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-npcm7xx.c | 2485 ++++++++++++++++++++++++++++++
 3 files changed, 2497 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c
drivers/i2c/busses/Kconfig +11 -0
index 146ce40d8e0a..9091b93aaf90 100644
@@ -786,6 +786,17 @@ config I2C_NOMADIK
 	  I2C interface from ST-Ericsson's Nomadik and Ux500 architectures,
 	  as well as the STA2X11 PCIe I/O HUB.
 
+config I2C_NPCM7XX
+	tristate "Nuvoton I2C Controller"
+	depends on ARCH_NPCM7XX
+	select I2C_SLAVE
+	help
+	  If you say yes to this option, support will be included for the
+	  Nuvoton I2C controller.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-npcm7xx.
+
 config I2C_OCORES
 	tristate "OpenCores I2C Controller"
 	help
drivers/i2c/busses/Makefile +1 -0
index 3ab8aebc39c9..4af59a806f3c 100644
@@ -81,6 +81,7 @@ obj-$(CONFIG_I2C_MT7621)	+= i2c-mt7621.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
+obj-$(CONFIG_I2C_NPCM7XX)	+= i2c-npcm7xx.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
drivers/i2c/busses/i2c-npcm7xx.c new file +2485 -0

2,485 lines not on this page · raw message

Signature · 1 line
2.22.0
#3 lore raw

Re: [PATCH v7 2/2] i2c: npcm: Add Nuvoton NPCM I2C controller driver

Quoted 3 lines · Tali Perry

Add Nuvoton NPCM BMC i2c controller driver.

Looking at all this SMB_* naming of the registers and also the quirks, this looks more like an SMBUS controller to me?

drivers/i2c/busses/i2c-npcm7xx.c line 346

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+	// currently I2C slave IF only supports single byte operations.
+	// in order to utilyze the npcm HW FIFO, the driver will ask for 16bytes
+	// at a time, pack them in buffer, and then transmit them all together
+	// to the FIFO and onward to the bus .
+	// NACK on read will be once reached to bus->adap->quirks->max_read_len
+	// sending a NACK whever the backend requests for it is not supported.

This for example...

drivers/i2c/busses/i2c-npcm7xx.c line 2331

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+static const struct i2c_adapter_quirks npcm_i2c_quirks = {
+	.max_read_len = 32768,
+	.max_write_len = 32768,
+	.max_num_msgs = 2,
+	.flags = I2C_AQ_COMB_WRITE_THEN_READ
+};

... and this. Like SMBus with the only exception of being able to send 32K in a row. Or?

author maintainer
#4 lore raw

Re: [PATCH v7 2/2] i2c: npcm: Add Nuvoton NPCM I2C controller driver

Hi Wolfram,

Thanks for your comments.

The NPCM7XX BMC I2C\SMB controller HW module supports both SMB and I2C. It's main features are:
1. Supports Fast-Mode (400 KHz clock) I2C and Fast-Mode-plus (1 MHz clock) I 2C
2. Supports the ‘fairness’ arbitration protocol defined by the MCTP SMBus/I2C Transport Binding Specification v1.0.0
3. 32KB packets : this is an I2C spec limitation. The HW has no limit on packets size. It has a 16 bytes FIFO which can be reloaded over and over.
4. w\o size byte (for SMB block protocol).
5. Both master and slave. It can also replace modes in run time (requirement for IPMB and MCTP).
6. Bus timing is selected to support both specs.

Originally the HW spec stated SMB everywhere .

Should I rename the SMB to I2C all over the driver?

Thanks,
Tali Perry

Quoted 32 lines · Wolfram Sang
Quoted 3 lines · Tali Perry

Add Nuvoton NPCM BMC i2c controller driver.

Looking at all this SMB_* naming of the registers and also the quirks, this looks more like an SMBUS controller to me?

Review

Quoted 2 lines
(quoted diff) +2 -0
+     // currently I2C slave IF only supports single byte operations.
+     // in order to utilyze the npcm HW FIFO, the driver will ask for

16bytes

Quoted 1 line

+ // at a time, pack them in buffer, and then transmit them all

together

Review

Quoted 2 lines
(quoted diff) +2 -0
+     // to the FIFO and onward to the bus .
+     // NACK on read will be once reached to

bus->adap->quirks->max_read_len

Quoted 1 line

+ // sending a NACK whever the backend requests for it is not

supported.

This for example...

drivers/i2c/busses/i2c-npcm7xx.c line 2331

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+static const struct i2c_adapter_quirks npcm_i2c_quirks = {
+     .max_read_len = 32768,
+     .max_write_len = 32768,
+     .max_num_msgs = 2,
+     .flags = I2C_AQ_COMB_WRITE_THEN_READ
+};

... and this. Like SMBus with the only exception of being able to send 32K in a row. Or?

author maintainer
#5 lore raw

Re: [PATCH v7 2/2] i2c: npcm: Add Nuvoton NPCM I2C controller driver

Hi Wolfram,

Thanks for your comments.

The NPCM7XX BMC I2C\SMB controller HW module supports both SMB and I2C. It's main features are:
1. Supports Fast-Mode (400 KHz clock) I2C and Fast-Mode-plus (1 MHz clock) I2C
2. Supports the ‘fairness’ arbitration protocol defined by the MCTP SMBus/I2C Transport Binding Specification v1.0.0
3. 32KB packets : this is an I2C spec limitation. The HW has no limit on packets size. It has a 16 bytes FIFO which can be reloaded over and over.
4. w\o size byte (for SMB block protocol).
5. Both master and slave. It can also replace modes in run time (requirement for IPMB and MCTP).
6. Bus timing is selected to support both specs.

Originally the HW spec stated SMB everywhere .

Should I rename the SMB to I2C all over the driver?

Thanks,
Tali Perry

Quoted 51 lines · Tali Perry

Hi Wolfram,

Thanks for your comments.

The NPCM7XX BMC I2C\SMB controller HW module supports both SMB and I2C.
It's main features are:
1. Supports Fast-Mode (400 KHz clock) I2C and Fast-Mode-plus (1 MHz clock) I2C
2. Supports the ‘fairness’ arbitration protocol defined by the MCTP SMBus/I2C Transport Binding Specification v1.0.0
3. 32KB packets : this is an I2C spec limitation. The HW has no limit on packets size. It has a 16 bytes FIFO which can be reloaded over and over.
4. w\o size byte (for SMB block protocol).
5. Both master and slave. It can also replace modes in run time (requirement for IPMB and MCTP).
6. Bus timing is selected to support both specs.

Originally the HW spec stated SMB everywhere .

Should I rename the SMB to I2C all over the driver?

Thanks,
Tali Perry

Quoted 28 lines · Wolfram Sang
Quoted 3 lines · Tali Perry

Add Nuvoton NPCM BMC i2c controller driver.

Looking at all this SMB_* naming of the registers and also the quirks, this looks more like an SMBUS controller to me?

drivers/i2c/busses/i2c-npcm7xx.c line 346

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+     // currently I2C slave IF only supports single byte operations.
+     // in order to utilyze the npcm HW FIFO, the driver will ask for 16bytes
+     // at a time, pack them in buffer, and then transmit them all together
+     // to the FIFO and onward to the bus .
+     // NACK on read will be once reached to bus->adap->quirks->max_read_len
+     // sending a NACK whever the backend requests for it is not supported.

This for example...

drivers/i2c/busses/i2c-npcm7xx.c line 2331

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+static const struct i2c_adapter_quirks npcm_i2c_quirks = {
+     .max_read_len = 32768,
+     .max_write_len = 32768,
+     .max_num_msgs = 2,
+     .flags = I2C_AQ_COMB_WRITE_THEN_READ
+};

... and this. Like SMBus with the only exception of being able to send 32K in a row. Or?

#6 lore raw

Re: [PATCH v7 2/2] i2c: npcm: Add Nuvoton NPCM I2C controller driver

Quoted 18 lines · Tali Perry

Hi Wolfram,

Thanks for your comments.

The NPCM7XX BMC I2C\SMB controller HW module supports both SMB and I2C. It's main features are:
1. Supports Fast-Mode (400 KHz clock) I2C and Fast-Mode-plus (1 MHz clock) I2C
2. Supports the ‘fairness’ arbitration protocol defined by the MCTP SMBus/I2C Transport Binding Specification v1.0.0
3. 32KB packets : this is an I2C spec limitation. The HW has no limit on packets size. It has a 16 bytes FIFO which can be reloaded over and over.
4. w\o size byte (for SMB block protocol).
5. Both master and slave. It can also replace modes in run time (requirement for IPMB and MCTP).
6. Bus timing is selected to support both specs.

Quoted 1 line

Originally the HW spec stated SMB everywhere .

Alright, so it sounds like the HW supports I2C and also has SMBus specific support?

Quoted 1 line

Should I rename the SMB to I2C all over the driver?

If the HW supports general I2C; then yes, you should use the I2C naming scheme. If you have SMBus specific support for some things, it might be helpful to tag on smb in the name for only those functions/data structures.

Also, please don't top-post.

Quoted 15 lines · Tali Perry
Quoted 14 lines

Hi Wolfram,

Thanks for your comments.

The NPCM7XX BMC I2C\SMB controller HW module supports both SMB and I2C.
It's main features are:
1. Supports Fast-Mode (400 KHz clock) I2C and Fast-Mode-plus (1 MHz clock) I2C
2. Supports the ‘fairness’ arbitration protocol defined by the MCTP SMBus/I2C Transport Binding Specification v1.0.0
3. 32KB packets : this is an I2C spec limitation. The HW has no limit on packets size. It has a 16 bytes FIFO which can be reloaded over and over.
4. w\o size byte (for SMB block protocol).
5. Both master and slave. It can also replace modes in run time (requirement for IPMB and MCTP).
6. Bus timing is selected to support both specs.

Quoted 3 lines
Quoted 3 lines

Originally the HW spec stated SMB everywhere .

Should I rename the SMB to I2C all over the driver?

And please don't reiterate yourself like this; it can confuse the conversation.

Quoted 29 lines
Quoted 29 lines · Wolfram Sang
Quoted 28 lines · Tali Perry
Quoted 3 lines

Add Nuvoton NPCM BMC i2c controller driver.

Looking at all this SMB_* naming of the registers and also the quirks, this looks more like an SMBUS controller to me?

drivers/i2c/busses/i2c-npcm7xx.c line 346

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+     // currently I2C slave IF only supports single byte operations.
+     // in order to utilyze the npcm HW FIFO, the driver will ask for 16bytes
+     // at a time, pack them in buffer, and then transmit them all together
+     // to the FIFO and onward to the bus .
+     // NACK on read will be once reached to bus->adap->quirks->max_read_len
+     // sending a NACK whever the backend requests for it is not supported.

This for example...

drivers/i2c/busses/i2c-npcm7xx.c line 2331

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+static const struct i2c_adapter_quirks npcm_i2c_quirks = {
+     .max_read_len = 32768,
+     .max_write_len = 32768,
+     .max_num_msgs = 2,
+     .flags = I2C_AQ_COMB_WRITE_THEN_READ
+};

... and this. Like SMBus with the only exception of being able to send 32K in a row. Or?

#7 lore raw

Re: [PATCH v7 2/2] i2c: npcm: Add Nuvoton NPCM I2C controller driver

In reply to Tali Perry
Quoted 15 lines · Tali Perry

Add Nuvoton NPCM BMC i2c controller driver.

---
 drivers/i2c/busses/Kconfig       |   11 +
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-npcm7xx.c | 2485 ++++++++++++++++++++++++++++++
 3 files changed, 2497 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c
drivers/i2c/busses/Kconfig +0 -0
index 146ce40d8e0a..9091b93aaf90 100644

drivers/i2c/busses/Kconfig config I2C_NOMADIK · line 791

Quoted 6 lines
drivers/i2c/busses/Kconfig +3 -0
@@ -786,6 +786,17 @@ config I2C_NOMADIK
 	  I2C interface from ST-Ericsson's Nomadik and Ux500 architectures,
 	  as well as the STA2X11 PCIe I/O HUB.
 
+config I2C_NPCM7XX
+	tristate "Nuvoton I2C Controller"
+	depends on ARCH_NPCM7XX

This should also depend on "|| COMPILE_TEST"

drivers/i2c/busses/Kconfig config I2C_NOMADIK · line 792

Quoted 1 line
drivers/i2c/busses/Kconfig +1 -0
+	select I2C_SLAVE

Personally, I would prefer if this was optional, but it's up to you.

drivers/i2c/busses/Kconfig config I2C_NOMADIK · line 795

Quoted 3 lines
drivers/i2c/busses/Kconfig +3 -0
+	help
+	  If you say yes to this option, support will be included for the
+	  Nuvoton I2C controller.

Might want to elaborate about the capabilities of the driver/hardware.

drivers/i2c/busses/Kconfig config I2C_NOMADIK · line 798

Quoted 2 lines
drivers/i2c/busses/Kconfig +2 -0
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-npcm7xx.

Probably unnecessary; this follows the pattern that most other I2C drivers do.

Quoted 21 lines
drivers/i2c/busses/Kconfig +0 -0
 config I2C_OCORES
 	tristate "OpenCores I2C Controller"
 	help
drivers/i2c/busses/Makefile +1 -0
index 3ab8aebc39c9..4af59a806f3c 100644
@@ -81,6 +81,7 @@ obj-$(CONFIG_I2C_MT7621)	+= i2c-mt7621.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
+obj-$(CONFIG_I2C_NPCM7XX)	+= i2c-npcm7xx.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
drivers/i2c/busses/i2c-npcm7xx.c new file +0 -0
new file mode 100644
index 000000000000..ce9699d56835

drivers/i2c/busses/i2c-npcm7xx.c line 5

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5 -0
@@ -0,0 +1,2485 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton NPCM7xx SMB Controller driver
+ *
+ * Copyright (C) 2018 Nuvoton Technologies [email protected]

You should update the date.

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+ */
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>

drivers/i2c/busses/i2c-npcm7xx.c line 21

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/jiffies.h>
+
+#define I2C_VERSION "0.1.0"

nit: You only use this macro in one place. I think it would be clearer to just use the raw string there.

drivers/i2c/busses/i2c-npcm7xx.c line 26

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5 -0
+
+enum smb_mode {
+	SMB_SLAVE = 1,
+	SMB_MASTER
+};

Please use proper namespacing.

Quoted 51 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +51 -0
+/*
+ * External SMB Interface driver xfer indication values, which indicate status
+ * of the bus.
+ */
+enum smb_state_ind {
+	SMB_NO_STATUS_IND = 0,
+	SMB_SLAVE_RCV_IND = 1,
+	SMB_SLAVE_XMIT_IND = 2,
+	SMB_SLAVE_XMIT_MISSING_DATA_IND = 3,
+	SMB_SLAVE_RESTART_IND = 4,
+	SMB_SLAVE_DONE_IND = 5,
+	SMB_MASTER_DONE_IND = 6,
+	SMB_NACK_IND = 8,
+	SMB_BUS_ERR_IND = 9,
+	SMB_WAKE_UP_IND = 10,
+	SMB_BLOCK_BYTES_ERR_IND = 12,
+	SMB_SLAVE_RCV_MISSING_DATA_IND = 14,
+};
+
+// SMBus Operation type values
+enum smb_oper {
+	SMB_NO_OPER = 0,
+	SMB_WRITE_OPER = 1,
+	SMB_READ_OPER = 2
+};
+
+// SMBus Bank (FIFO mode)
+enum smb_bank {
+	SMB_BANK_0 = 0,
+	SMB_BANK_1 = 1
+};
+
+// Internal SMB states values (for the SMB module state machine).
+enum smb_state {
+	SMB_DISABLE = 0,
+	SMB_IDLE,
+	SMB_MASTER_START,
+	SMB_SLAVE_MATCH,
+	SMB_OPER_STARTED,
+	SMB_STOP_PENDING
+};
+
+// Module supports setting multiple own slave addresses
+enum smb_addr {
+	SMB_SLAVE_ADDR1 = 0,
+	SMB_SLAVE_ADDR2,
+	SMB_SLAVE_ADDR3,
+	SMB_SLAVE_ADDR4,
+	SMB_SLAVE_ADDR5,
+	SMB_SLAVE_ADDR6,
+	SMB_SLAVE_ADDR7,

drivers/i2c/busses/i2c-npcm7xx.c line 88

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+	SMB_SLAVE_ADDR8,
+	SMB_SLAVE_ADDR9,
+	SMB_SLAVE_ADDR10,
+	SMB_GC_ADDR,
+	SMB_ARP_ADDR
+};
+
+// global regs
+static struct regmap *gcr_regmap;
+static struct regmap *clk_regmap;

It's generally best to avoid global variables.

drivers/i2c/busses/i2c-npcm7xx.c line 91

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+#define NPCM_I2CSEGCTL  0xE4
+#define I2CSEGCTL_VAL	0x0333F000

What is this value? It doesn't look like a register offset.

drivers/i2c/busses/i2c-npcm7xx.c line 104

Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12 -0
+// Common regs
+#define NPCM_SMBSDA			0x000
+#define NPCM_SMBST			0x002
+#define NPCM_SMBCST			0x004
+#define NPCM_SMBCTL1			0x006
+#define NPCM_SMBADDR1			0x008
+#define NPCM_SMBCTL2			0x00A
+#define NPCM_SMBADDR2			0x00C
+#define NPCM_SMBCTL3			0x00E
+#define NPCM_SMBCST2			0x018
+#define NPCM_SMBCST3			0x019
+#define SMB_VER				0x01F

nit: I am guessing these are not 12 bit values. Can you use just two digits?

drivers/i2c/busses/i2c-npcm7xx.c line 117

Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12 -0
+// BANK 0 regs
+#define NPCM_SMBADDR3			0x010
+#define NPCM_SMBADDR7			0x011
+#define NPCM_SMBADDR4			0x012
+#define NPCM_SMBADDR8			0x013
+#define NPCM_SMBADDR5			0x014
+#define NPCM_SMBADDR9			0x015
+#define NPCM_SMBADDR6			0x016
+#define NPCM_SMBADDR10			0x017
+
+// SMBADDR array: because the addr regs are sprinkled all over the address space
+const int  NPCM_SMBADDR[10] = {NPCM_SMBADDR1, NPCM_SMBADDR2, NPCM_SMBADDR3,

Please only use all caps for macros.

Quoted 131 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +131 -0
+			       NPCM_SMBADDR4, NPCM_SMBADDR5, NPCM_SMBADDR6,
+			       NPCM_SMBADDR7, NPCM_SMBADDR8, NPCM_SMBADDR9,
+			       NPCM_SMBADDR10};
+
+#define NPCM_SMBCTL4			0x01A
+#define NPCM_SMBCTL5			0x01B
+#define NPCM_SMBSCLLT			0x01C // SCL Low Time
+#define NPCM_SMBFIF_CTL			0x01D // FIFO Control
+#define NPCM_SMBSCLHT			0x01E // SCL High Time
+
+// BANK 1 regs
+#define NPCM_SMBFIF_CTS			0x010 // Both FIFOs Control and status
+#define NPCM_SMBTXF_CTL			0x012 // Tx-FIFO Control
+#define NPCM_SMBT_OUT			0x014 // Bus T.O.
+#define NPCM_SMBPEC			0x016 // PEC Data
+#define NPCM_SMBTXF_STS			0x01A // Tx-FIFO Status
+#define NPCM_SMBRXF_STS			0x01C // Rx-FIFO Status
+#define NPCM_SMBRXF_CTL			0x01E // Rx-FIFO Control
+
+// NPCM_SMBST reg fields
+#define NPCM_SMBST_XMIT			BIT(0)
+#define NPCM_SMBST_MASTER		BIT(1)
+#define NPCM_SMBST_NMATCH		BIT(2)
+#define NPCM_SMBST_STASTR		BIT(3)
+#define NPCM_SMBST_NEGACK		BIT(4)
+#define NPCM_SMBST_BER			BIT(5)
+#define NPCM_SMBST_SDAST		BIT(6)
+#define NPCM_SMBST_SLVSTP		BIT(7)
+
+// NPCM_SMBCST reg fields
+#define NPCM_SMBCST_BUSY		BIT(0)
+#define NPCM_SMBCST_BB			BIT(1)
+#define NPCM_SMBCST_MATCH		BIT(2)
+#define NPCM_SMBCST_GCMATCH		BIT(3)
+#define NPCM_SMBCST_TSDA		BIT(4)
+#define NPCM_SMBCST_TGSCL		BIT(5)
+#define NPCM_SMBCST_MATCHAF		BIT(6)
+#define NPCM_SMBCST_ARPMATCH		BIT(7)
+
+// NPCM_SMBCTL1 reg fields
+#define NPCM_SMBCTL1_START		BIT(0)
+#define NPCM_SMBCTL1_STOP		BIT(1)
+#define NPCM_SMBCTL1_INTEN		BIT(2)
+#define NPCM_SMBCTL1_EOBINTE		BIT(3)
+#define NPCM_SMBCTL1_ACK		BIT(4)
+#define NPCM_SMBCTL1_GCMEN		BIT(5)
+#define NPCM_SMBCTL1_NMINTE		BIT(6)
+#define NPCM_SMBCTL1_STASTRE		BIT(7)
+
+// RW1S fields (inside a RW reg):
+#define NPCM_SMBCTL1_RWS_FIELDS	  (NPCM_SMBCTL1_START | NPCM_SMBCTL1_STOP | \
+				   NPCM_SMBCTL1_ACK)
+// NPCM_SMBADDR reg fields
+#define NPCM_SMBADDR_A			GENMASK(6, 0)
+#define NPCM_SMBADDR_SAEN		BIT(7)
+
+// NPCM_SMBCTL2 reg fields
+#define SMBCTL2_ENABLE			BIT(0)
+#define SMBCTL2_SCLFRQ6_0		GENMASK(7, 1)
+
+// NPCM_SMBCTL3 reg fields
+#define SMBCTL3_SCLFRQ8_7		GENMASK(1, 0)
+#define SMBCTL3_ARPMEN			BIT(2)
+#define SMBCTL3_IDL_START		BIT(3)
+#define SMBCTL3_400K_MODE		BIT(4)
+#define SMBCTL3_BNK_SEL			BIT(5)
+#define SMBCTL3_SDA_LVL			BIT(6)
+#define SMBCTL3_SCL_LVL			BIT(7)
+
+// NPCM_SMBCST2 reg fields
+#define NPCM_SMBCST2_MATCHA1F		BIT(0)
+#define NPCM_SMBCST2_MATCHA2F		BIT(1)
+#define NPCM_SMBCST2_MATCHA3F		BIT(2)
+#define NPCM_SMBCST2_MATCHA4F		BIT(3)
+#define NPCM_SMBCST2_MATCHA5F		BIT(4)
+#define NPCM_SMBCST2_MATCHA6F		BIT(5)
+#define NPCM_SMBCST2_MATCHA7F		BIT(5)
+#define NPCM_SMBCST2_INTSTS		BIT(7)
+
+// NPCM_SMBCST3 reg fields
+#define NPCM_SMBCST3_MATCHA8F		BIT(0)
+#define NPCM_SMBCST3_MATCHA9F		BIT(1)
+#define NPCM_SMBCST3_MATCHA10F		BIT(2)
+#define NPCM_SMBCST3_EO_BUSY		BIT(7)
+
+// NPCM_SMBCTL4 reg fields
+#define SMBCTL4_HLDT			GENMASK(5, 0)
+#define SMBCTL4_LVL_WE			BIT(7)
+
+// NPCM_SMBCTL5 reg fields
+#define SMBCTL5_DBNCT			GENMASK(3, 0)
+
+// NPCM_SMBFIF_CTS reg fields
+#define NPCM_SMBFIF_CTS_RXF_TXE		BIT(1)
+#define NPCM_SMBFIF_CTS_RFTE_IE		BIT(3)
+#define NPCM_SMBFIF_CTS_CLR_FIFO	BIT(6)
+#define NPCM_SMBFIF_CTS_SLVRSTR		BIT(7)
+
+// NPCM_SMBTXF_CTL reg fields
+#define NPCM_SMBTXF_CTL_TX_THR		GENMASK(4, 0)
+#define NPCM_SMBTXF_CTL_THR_TXIE	BIT(6)
+
+// NPCM_SMBT_OUT reg fields
+#define NPCM_SMBT_OUT_TO_CKDIV		GENMASK(5, 0)
+#define NPCM_SMBT_OUT_T_OUTIE		BIT(6)
+#define NPCM_SMBT_OUT_T_OUTST		BIT(7)
+
+// NPCM_SMBTXF_STS reg fields
+#define NPCM_SMBTXF_STS_TX_BYTES	GENMASK(4, 0)
+#define NPCM_SMBTXF_STS_TX_THST		BIT(6)
+
+// NPCM_SMBRXF_STS reg fields
+#define NPCM_SMBRXF_STS_RX_BYTES	GENMASK(4, 0)
+#define NPCM_SMBRXF_STS_RX_THST		BIT(6)
+
+// NPCM_SMBFIF_CTL reg fields
+#define NPCM_SMBFIF_CTL_FIFO_EN		BIT(4)
+
+// NPCM_SMBRXF_CTL reg fields
+#define NPCM_SMBRXF_CTL_RX_THR		GENMASK(4, 0)
+#define NPCM_SMBRXF_CTL_LAST_PEC	BIT(5)
+#define NPCM_SMBRXF_CTL_THR_RXIE	BIT(6)
+
+#define SMBUS_FIFO_SIZE			16
+
+// SMB_VER reg fields
+#define SMB_VER_VERSION			GENMASK(6, 0)
+#define SMB_VER_FIFO_EN			BIT(7)
+
+// stall/stuck timeout
+const unsigned int DEFAULT_STALL_COUNT =	25;

drivers/i2c/busses/i2c-npcm7xx.c line 258

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+
+// retries in a loop for master abort
+const unsigned int RETRIES_NUM =	10000;
+
+// SMBus spec. values in KHZ
+const unsigned int SMBUS_FREQ_MIN = 10;
+const unsigned int SMBUS_FREQ_MAX = 1000;
+const unsigned int SMBUS_FREQ_100KHZ = 100;
+const unsigned int SMBUS_FREQ_400KHZ = 400;
+const unsigned int SMBUS_FREQ_1MHZ = 1000;

Does the hardware only support these clock speeds?

Quoted 68 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +68 -0
+// SCLFRQ min/max field values
+const unsigned int SCLFRQ_MIN = 10;
+const unsigned int SCLFRQ_MAX = 511;
+
+// SCLFRQ field position
+#define SCLFRQ_0_TO_6		GENMASK(6, 0)
+#define SCLFRQ_7_TO_8		GENMASK(8, 7)
+
+const unsigned int SMB_NUM_OF_ADDR = 10;
+
+#define NPCM_I2C_EVENT_START	BIT(0)
+#define NPCM_I2C_EVENT_STOP	BIT(1)
+#define NPCM_I2C_EVENT_ABORT	BIT(2)
+#define NPCM_I2C_EVENT_WRITE	BIT(3)
+
+#define NPCM_I2C_EVENT_READ	BIT(4)
+#define NPCM_I2C_EVENT_BER	BIT(5)
+#define NPCM_I2C_EVENT_NACK	BIT(6)
+#define NPCM_I2C_EVENT_TO	BIT(7)
+
+#define NPCM_I2C_EVENT_EOB	BIT(8)
+#define NPCM_I2C_EVENT_STALL	BIT(9)
+#define NPCM_I2C_EVENT_CB	BIT(10)
+#define NPCM_I2C_EVENT_DONE	BIT(11)
+
+#define NPCM_I2C_EVENT_READ1	BIT(12)
+#define NPCM_I2C_EVENT_READ2	BIT(13)
+#define NPCM_I2C_EVENT_READ3	BIT(14)
+#define NPCM_I2C_EVENT_READ4	BIT(15)
+
+#define NPCM_I2C_EVENT_NMATCH_SLV	BIT(16)
+#define NPCM_I2C_EVENT_NMATCH_MSTR	BIT(17)
+#define NPCM_I2C_EVENT_BER_SLV		BIT(18)
+
+#define NPCM_I2C_EVENT_LOG(event)	(bus->event_log |= event)
+
+// Status of one SMBus module
+struct npcm_i2c {
+	struct i2c_adapter	adap;
+	struct device		*dev;
+	unsigned char __iomem	*reg;
+	spinlock_t		lock;   /* IRQ synchronization */
+	struct completion	cmd_complete;
+	int			irq;
+	int			cmd_err;
+	struct i2c_msg		*msgs;
+	int			msgs_num;
+	int			num;
+	u32			apb_clk;
+	struct i2c_bus_recovery_info rinfo;
+	enum smb_state		state;
+	enum smb_oper		operation;
+	enum smb_mode		master_or_slave;
+	enum smb_state_ind	stop_ind;
+	u8			dest_addr;
+	u8			*rd_buf;
+	u16			rd_size;
+	u16			rd_ind;
+	u8			*wr_buf;
+	u16			wr_size;
+	u16			wr_ind;
+	bool			fifo_use;
+
+	// PEC bit mask per slave address.
+	//		1: use PEC for this address,
+	//		0: do not use PEC for this address
+	u16			PEC_mask;
+	bool			PEC_use;

drivers/i2c/busses/i2c-npcm7xx.c line 337

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+	bool			read_block_use;
+	u8			int_cnt;
+	u32			event_log;
+	u32			event_log_prev;
+	u32			clk_period_us;
+	unsigned long		int_time_stamp;
+	unsigned long		bus_freq; // in kHz
+	u32			xmits;
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)

Aha! It appears that selecting I2C_SLAVE is unnecessary.

Since you already ifdeffed out all the I2C slave code, can you put it in a separate patch to make this easier to review?

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+	u8			own_slave_addr;
+	struct i2c_client	*slave;
+
+	// currently I2C slave IF only supports single byte operations.
+	// in order to utilyze the npcm HW FIFO, the driver will ask for 16bytes
+	// at a time, pack them in buffer, and then transmit them all together
+	// to the FIFO and onward to the bus .
+	// NACK on read will be once reached to bus->adap->quirks->max_read_len
+	// sending a NACK whever the backend requests for it is not supported.
+

drivers/i2c/busses/i2c-npcm7xx.c line 357

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10 -0
+	// This module can be master and slave at the same time. separate ptrs
+	// and counters:
+	int			slv_rd_size;
+	int			slv_rd_ind;
+	int			slv_wr_size;
+	int			slv_wr_ind;
+	u8			slv_rd_buf[SMBUS_FIFO_SIZE];
+	u8			slv_wr_buf[SMBUS_FIFO_SIZE];
+#endif
+};

Cheers!