Skip to contentSkip to conversation

i2c: npcm7xx: add NPCM i2c controller driver

PATCH Superseded Tali Perry 4 participants 9 messages May 10, 2020 – May 13, 2020

Subsystems (3)

v10 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Tali Perry

author maintainer
#0 lore raw

v10 cover i2c: npcm7xx: 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.

---
v10 -> v9:
- Fix according to maintainer comments.
- binding file changed to yaml format.
- Shorten recovery flow.
- Add support for health monitoring counters.

v9 -> v8:
- Fix according to maintainer comments.
- Split lines of iowrite..(ioread..) to separate lines.
- Use readx_poll_timeout_atomic
- resolve various style issues.

v8 -> v7:
- Split to two commits, one for master, one for slave.
- Rename smb to i2c.
- Remove global vars.

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 (3):
dt-bindings: i2c: npcm7xx: add NPCM I2C controller documentation
i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
i2c: npcm7xx: Add support for slave mode for Nuvoton NPCM BMC I2C
controller driver.

 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     |   62 +
 drivers/i2c/busses/Kconfig                    |    9 +
 drivers/i2c/busses/Makefile                   |    1 +
 drivers/i2c/busses/i2c-npcm7xx.c              | 2467 +++++++++++++++++
 4 files changed, 2539 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c

base-commit: 262f7a6b8317a06e7d51befb690f0bca06a473ea

Signature · 7 lines
2.22.0


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author maintainer
#1 lore raw

PATCH v10 1/3 dt-bindings: i2c: npcm7xx: add NPCM I2C controller

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

---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml new file +62 -0
new file mode 100644
index 000000000000..d6f553154388
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/nuvoton,npcm7xx-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: nuvoton NPCM7XX I2C Controller Device Tree Bindings
+
+description: |
+  The NPCM750x includes sixteen I2C bus controllers. All Controllers support
+  both master and slave mode. Each controller can switch between master and slave
+  at run time (i.e. IPMB mode). Each controller has two 16 byte HW FIFO for TX and
+  RX.
+
+maintainers:
+  - Tali Perry <[email protected]>
+
+properties:
+  compatible:
+      - const: nuvoton,npcm7xx-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    items:
+      - description: Reference clock for the I2C bus
+
+  clock-frequency:
+    maxItems: 1
+    default: 100000
+    enum: [ 100000, 400000, 1000000 ]
+    description:
+      SCL frequency to use (in Hz). If omitted, 100kHz is used.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    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
maintainer
#2 lore raw

Re: [PATCH v10 1/3] dt-bindings: i2c: npcm7xx: add NPCM I2C controller

Quoted 9 lines · Tali Perry

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

---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml

My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml: properties:compatible: [{'const': 'nuvoton,npcm7xx-i2c'}] is not of type 'object', 'boolean' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml: properties:clock-frequency:enum: False schema does not allow [100000, 400000, 1000000] /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml: properties:clocks: {'maxItems': 1, 'items': [{'description': 'Reference clock for the I2C bus'}]} is not valid under any of the given schemas (Possible causes of the failure):
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml: properties:clocks: 'maxItems' is not one of ['type', 'description', 'dependencies', 'properties', 'patternProperties', 'additionalProperties', 'unevaluatedProperties', 'deprecated', 'required', 'allOf', 'anyOf', 'oneOf', '$ref']

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml: properties:clock-frequency: 'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'default', '$ref'] Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml: ignoring, error in schema: properties: compatible
warning: no schema found in file: ./Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml: ignoring, error in schema: properties: compatible
warning: no schema found in file: ./Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1287083

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

#3 lore raw

Re: [PATCH v10 1/3] dt-bindings: i2c: npcm7xx: add NPCM I2C controller

In reply to Tali Perry

The example uses "bus-frequency" while description says "clock-frequency" for bus speed.

Quoted 87 lines · On Sun, May 10, 2020, 12:27 Tali Perry <[email protected]> wrote:

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

---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml new file mode 100644
index 000000000000..d6f553154388

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml new file +10 -0
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/nuvoton,npcm7xx-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: nuvoton NPCM7XX I2C Controller Device Tree Bindings
+
+description: |
+  The NPCM750x includes sixteen I2C bus controllers. All Controllers

support
+ both master and slave mode. Each controller can switch between master and slave
+ at run time (i.e. IPMB mode). Each controller has two 16 byte HW FIFO for TX and

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml new file +50 -1
+  RX.
+
+maintainers:
+  - Tali Perry <[email protected]>
+
+properties:
+  compatible:
+      - const: nuvoton,npcm7xx-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    items:
+      - description: Reference clock for the I2C bus
+
+  clock-frequency:
+    maxItems: 1
+    default: 100000
+    enum: [ 100000, 400000, 1000000 ]
+    description:
+      SCL frequency to use (in Hz). If omitted, 100kHz is used.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    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>;
+    };
+
+...
--

2.22.0

author maintainer
#4 lore raw

PATCH v10 2/3 i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

In reply to Tali Perry

Add Nuvoton NPCM BMC I2C controller driver.

---
 drivers/i2c/busses/Kconfig       |    9 +
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-npcm7xx.c | 1853 ++++++++++++++++++++++++++++++
 3 files changed, 1863 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c
drivers/i2c/busses/Kconfig +9 -0
index 2ddca08f8a76..a2cfc555c284 100644
@@ -791,6 +791,15 @@ 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 || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  Nuvoton I2C controller, which is available on the NPCM7xx BMC
+	  controller.
+	  Driver can also support slave mode (select I2C_SLAVE).
+
 config I2C_OCORES
 	tristate "OpenCores I2C Controller"
 	help
drivers/i2c/busses/Makefile +1 -0
index 25d60889713c..8f3dfd376bec 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 +1853 -0

1,853 lines not on this page · raw message

Signature · 7 lines
2.22.0


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#5 lore raw

Re: [PATCH v10 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

Quoted 1 line · Tali Perry

Add Nuvoton NPCM BMC I2C controller driver.

Some cosmetic changes needs to be done.

...

Quoted 5 lines

+/*
+ * Nuvoton NPCM7xx I2C Controller driver
+ *
+ * Copyright (C) 2020 Nuvoton Technologies [email protected]
+ */

So, entire file has C99 comment style, but this and few other places. Any reason of inconsistency?

...

Quoted 1 line

+#if IS_ENABLED(CONFIG_DEBUG_FS)

Why?

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+#include <linux/debugfs.h>
+#endif

...

Quoted 1 line

+//#define _I2C_DEBUG_

Leftover, remove.

...

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

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5 -0
+// Common regs
+#define NPCM_I2CSDA                       0x00
+#define NPCM_I2CST                        0x02
+#define NPCM_I2CCST                       0x04
 
+#define NPCM_I2CCTL1	                  0x06

Indentation issue. And it's better to use TABs over spaces here and in the similar places above and below.

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+#define NPCM_I2CADDR1                     0x08
+#define NPCM_I2CCTL2                      0x0A
+#define NPCM_I2CADDR2                     0x0C
+#define NPCM_I2CCTL3                      0x0E
+#define NPCM_I2CCST2                      0x18
+#define NPCM_I2CCST3                      0x19

...

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

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+// supported clk settings. values in KHz.
+#define I2C_FREQ_MIN                      10
+#define I2C_FREQ_MAX                      1000

_KHZ to both.

...

Quoted 1 line

+#define I2C_NUM_OF_ADDR 10

Is it 10-bit address support or what?

...

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

Quoted 8 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8 -0
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static struct dentry *npcm_i2c_debugfs_dir;   /* i2c debugfs directory */
+static const char *ber_cnt_name      = "ber_count";
+static const char *rec_succ_cnt_name = "rec_succ_count";
+static const char *rec_fail_cnt_name = "rec_fail_count";
+static const char *nack_cnt_name     = "nack_count";
+static const char *timeout_cnt_name  = "timeout_count";
+#endif

Why are these global?

...

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

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus,
+					  u16 max_bytes_to_send)
+{
+	// Fill the FIFO, while the FIFO is not full and there are more bytes to
+	// write
 
+	while ((max_bytes_to_send--) &&

Inner parentheses are redundant.

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7 -0
+	       (I2C_HW_FIFO_SIZE - npcm_i2c_fifo_usage(bus))) {
+		if (bus->wr_ind < bus->wr_size)
+			npcm_i2c_wr_byte(bus, bus->wr_buf[bus->wr_ind++]);
+		else
+			npcm_i2c_wr_byte(bus, 0xFF);
+	}
+}

...

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

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+		// Clear stall only after setting STOP
+		iowrite8(NPCM_I2CST_STASTR, bus->reg + NPCM_I2CST);
+
+		ret =  IRQ_HANDLED;

Indentation issue.

...

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

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+				if (bus->wr_size)
+					npcm_i2c_set_fifo(bus, -1,
+							  bus->wr_size);
+				else
+					npcm_i2c_set_fifo(bus, bus->rd_size,
+							  -1);

These two looks much better on one line.

...

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

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5 -0
+				if (npcm_i2c_is_quick(bus) || bus->wr_size)
+					npcm_i2c_wr_byte(bus, bus->dest_addr);
+				else
+					npcm_i2c_wr_byte(bus, bus->dest_addr |
+							      0x01);

0x01 has its definition, hasn't it?

...

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

Quoted 11 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11 -0
+	// Repeat the following sequence until SDA is released
+	do {
+		// Issue a single SCL toggle
+		iowrite8(NPCM_I2CCST_TGSCL, bus->reg + NPCM_I2CCST);
+		udelay(20);
+		// If SDA line is inactive (high), stop
+		if (npcm_i2c_get_SDA(_adap)) {
+			done = true;
+			status = 0;
+		}
+	} while (!done && iter--);

readx_poll_timeout() ?

...

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

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	if (!status) {

Why not positive condition?

	if (status) {
		...
	} else {
		...
	}
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+	} else {
 
+	}
+#endif

...

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

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+static int npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq)
+{
+	u32  k1 = 0;
+	u32  k2 = 0;
+	u8   dbnct = 0;
+	u32  sclfrq = 0;
+	u8   hldt = 7;
+	bool fast_mode = false;
 
+	u32  src_clk_freq; // in KHz

src_clk_khz ?

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

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+
+	src_clk_freq = bus->apb_clk / 1000;
+	bus->bus_freq = bus_freq;
+
+	// 100KHz and below:
 
+	if (bus_freq <= (I2C_MAX_STANDARD_MODE_FREQ / 1000)) {

Instead of all these / 1000 can't you use bus frequency in Hz and do division when it's really needed?

Quoted 24 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +24 -0
+		sclfrq = src_clk_freq / (bus_freq * 4);
+
+		if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
+			return -EDOM;
+
+		if (src_clk_freq >= 40000)
+			hldt = 17;
+		else if (src_clk_freq >= 12500)
+			hldt = 15;
+		else
+			hldt = 7;
+	}
+
+	// 400KHz:
+	else if (bus_freq <= (I2C_MAX_FAST_MODE_FREQ / 1000)) {
+		sclfrq = 0;
+		fast_mode = true;
+
+		if (src_clk_freq < 7500)
+			// 400KHZ cannot be supported for core clock < 7.5 MHZ
+			return -EDOM;
+
+		else if (src_clk_freq >= 50000) {
+			k1 = 80;

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

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+			k2 = 48;
+			hldt = 12;
+			dbnct = 7;
+		}
+
+		// Master or Slave with frequency > 25 MHZ
+		else if (src_clk_freq > 25000) {
 
+			hldt = (DIV_ROUND_UP(src_clk_freq * 300,
+							 1000000) + 7) & 0xFF;

How ' & 0xFF' is not no-op here and in the similar cases?

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

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5 -0
+
+			k1 = DIV_ROUND_UP(src_clk_freq * 1600,
+						   1000000);
+			k2 = DIV_ROUND_UP(src_clk_freq * 900,
+						   1000000);

Perhaps,

#define clk_coef(freq, mul) DIV_ROUND_UP((freq) * (mul), 1000000)

?

Quoted 44 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +43 -0
+			k1 = round_up(k1, 2);
+			k2 = round_up(k2 + 1, 2);
+			if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
+			    k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
+				return -EDOM;
+		}
+	}
+
+	else if (bus_freq <= (I2C_MAX_FAST_MODE_PLUS_FREQ / 1000)) {
+		sclfrq = 0;
+		fast_mode = true;
+
+		if (src_clk_freq < 24000)
+		// 1MHZ cannot be supported for master core clock < 15 MHZ
+		// or slave core clock < 24 MHZ
+			return -EDOM;
+
+		k1 = round_up((DIV_ROUND_UP(src_clk_freq * 620,
+						     1000000)), 2);
+		k2 = round_up((DIV_ROUND_UP(src_clk_freq * 380,
+						     1000000) + 1), 2);
+		if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
+		    k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
+			return -EDOM;
+
+		// Core clk > 40 MHZ
+		if (src_clk_freq > 40000) {
+			// Set HLDT:
+			// SDA hold time:  (HLDT-7) * T(CLK) >= 120
+			// HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
+			hldt = (DIV_ROUND_UP(src_clk_freq * 120,
+							 1000000) + 7) & 0xFF;
+		} else {
+			hldt = 7;
+			dbnct = 2;
+		}
+	}
+
+	// Frequency larger than 1 MHZ is not supported
+	else
+		return false;
 
+	return true;
+}

...

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

Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+	ret = device_property_read_u32(&pdev->dev, "bus-frequency", &clk_freq);
+	if (ret < 0) {
+		dev_info(&pdev->dev, "Could not read bus-frequency property\n");
 
+		clk_freq = 100000;

We have define for this, don't we?

Review

Quoted 1 line
(quoted diff) +1 -0
+	}

Wolfram, we discussed this simplified timings property parser, any news about it?

...

Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+static irqreturn_t npcm_i2c_bus_irq(int irq, void *dev_id)
+{
+	irqreturn_t ret;
+	struct npcm_i2c *bus = dev_id;
+
+	bus->int_cnt++;

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

Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9 -0
+
+	if (npcm_i2c_is_master(bus))
+		bus->master_or_slave = I2C_MASTER;
+
+	if (bus->master_or_slave == I2C_MASTER)	{
+		bus->int_time_stamp = jiffies;
+		ret = npcm_i2c_int_master_handler(bus);
 
+		if (ret == IRQ_HANDLED)
+			return ret;

What's the point?

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+	}
+	return IRQ_HANDLED;
+}

...

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

Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1 -0
+	bus->dest_addr = (slave_addr << 1) & 0xFE;

How ' & 0xFE' part is not a no-op?

...

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

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+	time_left = jiffies +
+		    msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;

It's perfectly one line. Fix here and in any other place with similar issue.

...

Quoted 1 line

+static int i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus)

Should be void.

...

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

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+	bus->irq = platform_get_irq(pdev, 0);
+	if (bus->irq < 0) {
 
+		dev_err(&pdev->dev, "failed to get IRQ number\n");

Duplicate message.

Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+		return bus->irq;
+	}

...

Quoted 1 line

+#if IS_ENABLED(CONFIG_DEBUG_FS)

Why? Okay, why here instead of making a stub?

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

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3 -0
+	ret = i2c_init_debugfs(pdev, bus);
 
+	if (ret < 0)
+		return ret;

Wrong. Should not fail the probe.

Quoted 1 line

+#endif

...

Quoted 1 line

+#if IS_ENABLED(CONFIG_DEBUG_FS)

Why? Just make it always present in the structure.

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

Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1 -0
+	if (!!bus->debugfs) {

!! ???

Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4 -0
+		debugfs_remove_recursive(bus->debugfs);
+		bus->debugfs = NULL;
+	}
+#endif

...

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

Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6 -0
+	npcm_i2c_debugfs_dir = debugfs_create_dir("i2c", NULL);
 
+	if (IS_ERR_OR_NULL(npcm_i2c_debugfs_dir)) {
+		pr_warn("i2c init of debugfs failed\n");
+		npcm_i2c_debugfs_dir = NULL;
+		return -ENOMEM;
+	}

Shouldn't prevent driver to work.

...

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

Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1 -0
+	if (!!npcm_i2c_debugfs_dir) {

!! ???

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

Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2 -0
+		debugfs_remove_recursive(npcm_i2c_debugfs_dir);
 
+		npcm_i2c_debugfs_dir = NULL;

What's the point?

Quoted 1 line
(quoted diff) +1 -0
+	}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#6 lore raw

Re: [PATCH v10 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

Quoted 6 lines · Andy Shevchenko
Quoted 1 line · Tali Perry

Add Nuvoton NPCM BMC I2C controller driver.

Some cosmetic changes needs to be done.

Thanks for the review and the comments.
Will fix all, have a few questions (below)

Quoted 8 lines

...

Quoted 5 lines

+/*
+ * Nuvoton NPCM7xx I2C Controller driver
+ *
+ * Copyright (C) 2020 Nuvoton Technologies [email protected]
+ */

Quoted 8 lines

So, entire file has C99 comment style, but this and few other places. Any reason of inconsistency?

...

Quoted 1 line

+#if IS_ENABLED(CONFIG_DEBUG_FS)

Why?

We wanted to add an optional feature to track i2c slave status.
the NPCM has 16 channels handling multiple devices each. Some of the devices are polled periodically, and might power down.
The user wanted to implement a health monitoring option
to occasionally check the status of the buses (how many timeouts, recovery etc.) This feature is optional and depends on CONFIG_DEBUG_FS The counters are exposed to user through the file system.

....

Quoted 6 lines

...

Quoted 1 line

+#define I2C_NUM_OF_ADDR 10

Is it 10-bit address support or what?

No, the NPCM has an option to respond to multiple slave addresses (10 own slave addresses)

...

Quoted 12 lines

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

Quoted 11 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11 -0
+     // Repeat the following sequence until SDA is released
+     do {
+             // Issue a single SCL toggle
+             iowrite8(NPCM_I2CCST_TGSCL, bus->reg + NPCM_I2CCST);
+             udelay(20);
+             // If SDA line is inactive (high), stop
+             if (npcm_i2c_get_SDA(_adap)) {
+                     done = true;
+                     status = 0;
+             }
+     } while (!done && iter--);
Quoted 1 line

readx_poll_timeout() ?

Not exactly, readx_poll_timeout includes only a read operation, here there is a write in the middle. (iowrite8)

Quoted 3 lines

...

...

Thanks!
Tali Perry
Nuvoton Technologies

#7 lore raw

Re: [PATCH v10 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

Quoted 3 lines · Tali Perry
Quoted 1 line · Andy Shevchenko

On Sun, May 10, 2020 at 01:23:29PM +0300, Tali Perry wrote:

...

Quoted 11 lines
Quoted 3 lines
Quoted 1 line

+#if IS_ENABLED(CONFIG_DEBUG_FS)

Why?

We wanted to add an optional feature to track i2c slave status.
the NPCM has 16 channels handling multiple devices each. Some of the devices are polled periodically, and might power down.
The user wanted to implement a health monitoring option
to occasionally check the status of the buses (how many timeouts, recovery etc.) This feature is optional and depends on CONFIG_DEBUG_FS The counters are exposed to user through the file system.

What I meant is why do you need an #ifdef?

...

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

+#define I2C_NUM_OF_ADDR 10

Is it 10-bit address support or what?

No, the NPCM has an option to respond to multiple slave addresses (10 own slave addresses)

Perhaps more descriptive name then?

...

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11 -0
+     // Repeat the following sequence until SDA is released
+     do {
+             // Issue a single SCL toggle
+             iowrite8(NPCM_I2CCST_TGSCL, bus->reg + NPCM_I2CCST);
+             udelay(20);
+             // If SDA line is inactive (high), stop
+             if (npcm_i2c_get_SDA(_adap)) {
+                     done = true;
+                     status = 0;
+             }
+     } while (!done && iter--);
Quoted 4 lines
Quoted 1 line

readx_poll_timeout() ?

Not exactly, readx_poll_timeout includes only a read operation, here there is a write in the middle. (iowrite8)

Ah, indeed. Perhaps time to add writex_poll_timeout() ?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#8 lore raw

PATCH v10 3/3 i2c: npcm7xx: Add support for slave mode for Nuvoton

In reply to Tali Perry

Add support for slave mode for Nuvoton
NPCM BMC I2C controller driver.

---
 drivers/i2c/busses/i2c-npcm7xx.c | 614 +++++++++++++++++++++++++++++++
 1 file changed, 614 insertions(+)
drivers/i2c/busses/i2c-npcm7xx.c +614 -0
index e732c40f0cf3..54af253249d5 100644
@@ -73,6 +73,24 @@ enum i2c_state {
 	I2C_STOP_PENDING,
 };
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+// Module supports setting multiple own slave addresses
+enum i2c_addr {
+	I2C_SLAVE_ADDR1 = 0,
+	I2C_SLAVE_ADDR2,
+	I2C_SLAVE_ADDR3,
+	I2C_SLAVE_ADDR4,
+	I2C_SLAVE_ADDR5,
+	I2C_SLAVE_ADDR6,
+	I2C_SLAVE_ADDR7,
+	I2C_SLAVE_ADDR8,
+	I2C_SLAVE_ADDR9,
+	I2C_SLAVE_ADDR10,
+	I2C_GC_ADDR,
+	I2C_ARP_ADDR,
+};
+#endif
+
 // init register and default value required to enable module
 #define NPCM_I2CSEGCTL                    0xE4
 #define NPCM_I2CSEGCTL_INIT_VAL           0x0333F000
@@ -100,6 +118,14 @@ enum i2c_state {
 #define NPCM_I2CADDR6                     0x16
 #define NPCM_I2CADDR10                    0x17
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+// I2CADDR array: because the addr regs are sprinkled all over the address space
+const int  npcm_i2caddr[10] = {NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3,
+			       NPCM_I2CADDR4, NPCM_I2CADDR5, NPCM_I2CADDR6,
+			       NPCM_I2CADDR7, NPCM_I2CADDR8, NPCM_I2CADDR9,
+			       NPCM_I2CADDR10};
+#endif
+
 #define NPCM_I2CCTL4                      0x1A
 #define NPCM_I2CCTL5                      0x1B
 #define NPCM_I2CSCLLT                     0x1C // SCL Low Time
@@ -273,6 +299,16 @@ struct npcm_i2c {
 	unsigned long int_time_stamp;
 	unsigned long bus_freq; // in kHz
 	u32 xmits;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	u8 own_slave_addr;
+	struct i2c_client *slave;
+	int slv_rd_size;
+	int slv_rd_ind;
+	int slv_wr_size;
+	int slv_wr_ind;
+	u8 slv_rd_buf[I2C_HW_FIFO_SIZE];
+	u8 slv_wr_buf[I2C_HW_FIFO_SIZE];
+#endif
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 	struct dentry *debugfs;/* debugfs device directory */
 	uint64_t ber_count;
@@ -316,6 +352,10 @@ static void npcm_i2c_init_params(struct npcm_i2c *bus)
 	bus->int_time_stamp = 0;
 	bus->PEC_use = false;
 	bus->PEC_mask = 0;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave)
+		bus->master_or_slave = I2C_SLAVE;
+#endif
 }
 
 static inline void npcm_i2c_wr_byte(struct npcm_i2c *bus, u8 data)
@@ -363,6 +403,18 @@ static void npcm_i2c_disable(struct npcm_i2c *bus)
 {
 	u8 i2cctl2;
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	int i;
+
+	// select bank 0 for I2C addresses
+	npcm_i2c_select_bank(bus, I2C_BANK_0);
+
+	// Slave addresses removal
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OF_ADDR; i++)
+		iowrite8(0, bus->reg + npcm_i2caddr[i]);
+
+	npcm_i2c_select_bank(bus, I2C_BANK_1);
+#endif
 	// Disable module.
 	i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);
 	i2cctl2 = i2cctl2 & ~I2CCTL2_ENABLE;
@@ -529,6 +581,63 @@ static inline void npcm_i2c_nack(struct npcm_i2c *bus)
 	iowrite8(val, bus->reg + NPCM_I2CCTL1);
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static void npcm_i2c_slave_int_enable(struct npcm_i2c *bus, bool enable)
+{
+	u8 i2cctl1;
+
+	// enable interrupt on slave match:
+	i2cctl1 = ioread8(bus->reg + NPCM_I2CCTL1);
+	i2cctl1 &= ~NPCM_I2CCTL1_RWS;
+	if (enable)
+		i2cctl1 |= NPCM_I2CCTL1_NMINTE;
+	else
+		i2cctl1 &= ~NPCM_I2CCTL1_NMINTE;
+	iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
+}
+
+static int  npcm_i2c_slave_enable_l(struct npcm_i2c *bus,
+				    enum i2c_addr addr_type, u8 addr,
+				    bool enable)
+{
+	u8 i2cctl1;
+	u8 i2cctl3;
+	u8 sa_reg;
+
+	// npcm_i2c_slave_int_enable(bus, false);
+	sa_reg = (addr & 0x7F) | FIELD_PREP(NPCM_I2CADDR_SAEN, enable);
+	if (addr_type == I2C_GC_ADDR) {
+		i2cctl1 = ioread8(bus->reg + NPCM_I2CCTL1);
+		if (enable)
+			i2cctl1 |= NPCM_I2CCTL1_GCMEN;
+		else
+			i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
+		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
+		return 0;
+	}
+	if (addr_type == I2C_ARP_ADDR) {
+		i2cctl3 = ioread8(bus->reg + NPCM_I2CCTL3);
+		if (enable)
+			i2cctl3 |= I2CCTL3_ARPMEN;
+		else
+			i2cctl3 &= ~I2CCTL3_ARPMEN;
+		iowrite8(i2cctl3, bus->reg + NPCM_I2CCTL3);
+		return 0;
+	}
+	if (addr_type >= I2C_ARP_ADDR)
+		return -EFAULT;
+	// select bank 0 for address 3 to 10
+	if (addr_type > I2C_SLAVE_ADDR2)
+		npcm_i2c_select_bank(bus, I2C_BANK_0);
+	// Set and enable the address
+	iowrite8(sa_reg, bus->reg + npcm_i2caddr[(int)addr_type]);
+	npcm_i2c_slave_int_enable(bus, enable);
+	if (addr_type > I2C_SLAVE_ADDR2)
+		npcm_i2c_select_bank(bus, I2C_BANK_1);
+	return 0;
+}
+#endif
+
 static void npcm_i2c_reset(struct npcm_i2c *bus)
 {
 	// Save I2CCTL1 relevant bits. It is being cleared when the module
@@ -556,6 +665,12 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)
 	// Clear all fifo bits:
 	iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO, bus->reg + NPCM_I2CFIF_CTS);
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave)
+		npcm_i2c_slave_enable_l(bus, I2C_SLAVE_ADDR1, bus->slave->addr,
+					true);
+#endif
+
 	bus->state = I2C_IDLE;
 }
 
@@ -620,6 +735,10 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
 	}
 
 	bus->operation = I2C_NO_OPER;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave)
+		bus->master_or_slave = I2C_SLAVE;
+#endif
 }
 
 static u32 npcm_i2c_fifo_usage(struct npcm_i2c *bus)
@@ -647,6 +766,33 @@ static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus,
 	}
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static void npcm_i2c_write_to_fifo_slave(struct npcm_i2c *bus,
+					 u16 max_bytes_to_send)
+{
+	// Fill the FIFO, while the FIFO is not full and there are more bytes to
+	// write
+	npcm_i2c_clear_fifo_int(bus);
+	npcm_i2c_clear_tx_fifo(bus);
+	iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
+
+	while ((max_bytes_to_send--) &&
+		(I2C_HW_FIFO_SIZE - npcm_i2c_fifo_usage(bus))) {
+
+		if (bus->slv_wr_size > 0) {
+			bus->slv_wr_ind = bus->slv_wr_ind % I2C_HW_FIFO_SIZE;
+			npcm_i2c_wr_byte(bus,
+					 bus->slv_wr_buf[bus->slv_wr_ind]);
+			bus->slv_wr_ind++;
+			bus->slv_wr_ind = bus->slv_wr_ind % I2C_HW_FIFO_SIZE;
+			bus->slv_wr_size--; // size indicates the # of bytes in
+					    // the SW buffer, not HW fifo.
+		} else {
+			break;
+		}
+	}
+}
+#endif
 /*
  * configure the FIFO before using it. If nread is -1 RX FIFO will not be
  * configured. same for nwrite
@@ -705,6 +851,21 @@ static void npcm_i2c_read_from_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo)
 			if (bus->rd_ind < bus->rd_size)
 				bus->rd_buf[bus->rd_ind++] = data;
 		}
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		else {
+			if (bus->slave) {
+				bus->slv_rd_ind = bus->slv_rd_ind %
+						  I2C_HW_FIFO_SIZE;
+				bus->slv_rd_buf[bus->slv_rd_ind] = data;
+				bus->slv_rd_ind++;
+				if (bus->slv_rd_ind == 1 && bus->read_block_use)
+					// 1st byte is length in block protocol
+					bus->slv_rd_size = data +
+							   (u8)bus->PEC_use +
+							(u8)bus->read_block_use;
+			}
+		}
+#endif
 	}
 }
 
@@ -727,6 +888,433 @@ static void npcm_i2c_master_abort(struct npcm_i2c *bus)
 	}
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static u8 npcm_i2c_get_slave_addr(struct npcm_i2c *bus,
+				  enum i2c_addr addr_type)
+{
+	u8 slave_add;
+
+	// select bank 0 for address 3 to 10
+	if (addr_type > I2C_SLAVE_ADDR2)
+		npcm_i2c_select_bank(bus, I2C_BANK_0);
+
+	slave_add = ioread8(bus->reg + npcm_i2caddr[(int)addr_type]);
+
+	if (addr_type > I2C_SLAVE_ADDR2)
+		npcm_i2c_select_bank(bus, I2C_BANK_1);
+
+	return  slave_add;
+}
+
+static int  npcm_i2c_remove_slave_addr(struct npcm_i2c *bus, u8 slave_add)
+{
+	int i;
+
+	// Set the enable bit
+	slave_add |= 0x80;
+	npcm_i2c_select_bank(bus, I2C_BANK_0);
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OF_ADDR; i++) {
+		if (ioread8(bus->reg + npcm_i2caddr[i]) == slave_add)
+			iowrite8(0, bus->reg + npcm_i2caddr[i]);
+	}
+	npcm_i2c_select_bank(bus, I2C_BANK_1);
+	return 0;
+}
+
+static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
+{
+	int i;
+	u8 value = 0;
+	int ret = bus->slv_wr_ind;
+
+	// fill a cyclic buffer
+	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
+		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
+			break;
+		i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+		bus->slv_wr_buf[(bus->slv_wr_ind + bus->slv_wr_size) %
+				 I2C_HW_FIFO_SIZE] = value;
+		bus->slv_wr_size++;
+		i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
+	}
+	return I2C_HW_FIFO_SIZE - ret;
+}
+
+static void npcm_i2c_slave_send_rd_buf(struct npcm_i2c *bus)
+{
+	int i;
+
+	for (i = 0; i < bus->slv_rd_ind; i++)
+		i2c_slave_event(bus->slave, I2C_SLAVE_WRITE_RECEIVED,
+				&bus->slv_rd_buf[i]);
+
+	// once we send bytes up, need to reset the counter of the wr buf
+	// got data from master (new offset in device), ignore wr fifo:
+	if (bus->slv_rd_ind) {
+		bus->slv_wr_size = 0;
+		bus->slv_wr_ind = 0;
+	}
+
+	bus->slv_rd_ind = 0;
+	bus->slv_rd_size = bus->adap.quirks->max_read_len;
+
+	npcm_i2c_clear_fifo_int(bus);
+	npcm_i2c_clear_rx_fifo(bus);
+}
+
+static bool npcm_i2c_slave_receive(struct npcm_i2c *bus, u16 nread,
+				   u8 *read_data)
+{
+	bus->state = I2C_OPER_STARTED;
+	bus->operation	 = I2C_READ_OPER;
+	bus->slv_rd_size = nread;
+	bus->slv_rd_ind	= 0;
+
+	iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
+	iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CRXF_CTL);
+
+	npcm_i2c_clear_tx_fifo(bus);
+	npcm_i2c_clear_rx_fifo(bus);
+
+	return true;
+}
+
+static bool npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
+				u8 *write_data)
+{
+	if (nwrite == 0)
+		return false;
+
+	bus->state = I2C_OPER_STARTED;
+	bus->operation = I2C_WRITE_OPER;
+
+	// get the next buffer
+	npcm_i2c_slave_get_wr_buf(bus);
+
+	if (nwrite > 0)
+		npcm_i2c_write_to_fifo_slave(bus, nwrite);
+
+	return true;
+}
+
+/*
+ * currently slave IF only supports single byte operations.
+ * in order to utilyze the npcm HW FIFO, the driver will ask for 16 bytes
+ * 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 wherever the backend requests for it is not supported.
+ * the next two functions allow reading to local buffer before writing it all
+ * to the HW FIFO.
+ * ret val: number of bytes read from the IF:
+ */
+static int npcm_i2c_slave_wr_buf_sync(struct npcm_i2c *bus)
+{
+	int left_in_fifo;
+
+	left_in_fifo = FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
+			ioread8(bus->reg + NPCM_I2CTXF_STS));
+	if (left_in_fifo >= I2C_HW_FIFO_SIZE)
+		return left_in_fifo;
+
+	// fifo already full:
+	if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
+		return left_in_fifo;
+
+	// update the wr fifo ind, back to the untransmitted bytes:
+	bus->slv_wr_ind = bus->slv_wr_ind - left_in_fifo;
+	bus->slv_wr_size = bus->slv_wr_size + left_in_fifo;
+
+	if (bus->slv_wr_ind < 0)
+		bus->slv_wr_ind += I2C_HW_FIFO_SIZE;
+
+	return left_in_fifo;
+}
+
+static void npcm_i2c_slave_rd_wr(struct npcm_i2c *bus)
+{
+	if (FIELD_GET(NPCM_I2CST_XMIT, ioread8(bus->reg + NPCM_I2CST))) {
+		/*
+		 * Slave got an address match with direction bit 1 so
+		 * it should transmit data
+		 * Write till the master will NACK
+		 */
+		bus->operation = I2C_WRITE_OPER;
+		npcm_i2c_slave_xmit(bus,
+				    bus->adap.quirks->max_write_len,
+				    bus->slv_wr_buf);
+	} else {
+		/*
+		 * Slave got an address match with direction bit 0
+		 * so it should receive data.
+		 * this module does not support saying no to bytes.
+		 * it will always ACK.
+		 */
+		bus->operation = I2C_READ_OPER;
+		npcm_i2c_read_from_fifo(bus, npcm_i2c_fifo_usage(bus));
+		bus->stop_ind = I2C_SLAVE_RCV_IND;
+		npcm_i2c_slave_send_rd_buf(bus);
+		npcm_i2c_slave_receive(bus,
+				       bus->adap.quirks->max_read_len,
+				       bus->slv_rd_buf);
+	}
+}
+
+static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus)
+{
+	u8 val;
+	irqreturn_t ret = IRQ_NONE;
+	u8 i2cst = ioread8(bus->reg + NPCM_I2CST);
+
+	// Slave: A NACK has occurred
+	if (FIELD_GET(NPCM_I2CST_NEGACK, i2cst)) {
+		bus->stop_ind = I2C_NACK_IND;
+		npcm_i2c_slave_wr_buf_sync(bus);
+		if (bus->fifo_use)
+			// clear the FIFO
+			iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO,
+				 bus->reg + NPCM_I2CFIF_CTS);
+
+		// In slave write, NACK is OK, otherwise it is a problem
+		bus->stop_ind = I2C_NO_STATUS_IND;
+		bus->operation = I2C_NO_OPER;
+		bus->own_slave_addr = 0xFF;
+
+		/*
+		 * Slave has to wait for I2C_STOP to decide this is the end
+		 * of the transaction.
+		 * Therefore transaction is not yet considered as done
+		 */
+		iowrite8(NPCM_I2CST_NEGACK, bus->reg + NPCM_I2CST);
+
+		ret = IRQ_HANDLED;
+	}
+
+	// Slave mode: a Bus Error (BER) has been identified
+	if (FIELD_GET(NPCM_I2CST_BER, i2cst)) {
+		// Check whether bus arbitration or Start or Stop during data
+		// xfer bus arbitration problem should not result in recovery
+		bus->stop_ind = I2C_BUS_ERR_IND;
+
+		// wait for bus busy before clear fifo
+		iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO, bus->reg + NPCM_I2CFIF_CTS);
+
+		bus->state = I2C_IDLE;
+
+		// in BER case we might get 2 interrupts: one for slave one for
+		// master ( for a channel which is master\slave switching)
+		if (completion_done(&bus->cmd_complete) == false) {
+			bus->cmd_err = -EIO;
+			complete(&bus->cmd_complete);
+		}
+		bus->own_slave_addr = 0xFF;
+		iowrite8(NPCM_I2CST_BER, bus->reg + NPCM_I2CST);
+		ret =  IRQ_HANDLED;
+	}
+
+	// A Slave Stop Condition has been identified
+	if (FIELD_GET(NPCM_I2CST_SLVSTP, i2cst)) {
+		int bytes_in_fifo = npcm_i2c_fifo_usage(bus);
+
+		bus->stop_ind = I2C_SLAVE_DONE_IND;
+
+		if (bus->operation == I2C_READ_OPER) {
+			npcm_i2c_read_from_fifo(bus, bytes_in_fifo);
+
+			// Slave done transmitting or receiving
+			// if the buffer is empty nothing will be sent
+		}
+
+		// Slave done transmitting or receiving
+		// if the buffer is empty nothing will be sent
+		npcm_i2c_slave_send_rd_buf(bus);
+
+		bus->stop_ind = I2C_NO_STATUS_IND;
+
+		/*
+		 * Note, just because we got here, it doesn't mean we through
+		 * away the wr buffer.
+		 * we keep it until the next received offset.
+		 */
+		bus->operation = I2C_NO_OPER;
+		bus->int_cnt = 0;
+		bus->own_slave_addr = 0xFF;
+		i2c_slave_event(bus->slave, I2C_SLAVE_STOP, 0);
+		iowrite8(NPCM_I2CST_SLVSTP, bus->reg + NPCM_I2CST);
+		if (bus->fifo_use) {
+			npcm_i2c_clear_fifo_int(bus);
+			npcm_i2c_clear_rx_fifo(bus);
+			npcm_i2c_clear_tx_fifo(bus);
+
+			iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO,
+				 bus->reg + NPCM_I2CFIF_CTS);
+		}
+		bus->state = I2C_IDLE;
+		ret =  IRQ_HANDLED;
+	}
+
+	// restart condition occurred and Rx-FIFO was not empty
+	if (bus->fifo_use && FIELD_GET(NPCM_I2CFIF_CTS_SLVRSTR,
+				       ioread8(bus->reg + NPCM_I2CFIF_CTS))) {
+		bus->stop_ind = I2C_SLAVE_RESTART_IND;
+		bus->master_or_slave = I2C_SLAVE;
+		if (bus->operation == I2C_READ_OPER)
+			npcm_i2c_read_from_fifo(bus, npcm_i2c_fifo_usage(bus));
+		bus->operation = I2C_WRITE_OPER;
+		iowrite8(0, bus->reg + NPCM_I2CRXF_CTL);
+		val = NPCM_I2CFIF_CTS_CLR_FIFO | NPCM_I2CFIF_CTS_SLVRSTR |
+			 NPCM_I2CFIF_CTS_RXF_TXE;
+		iowrite8(val, bus->reg + NPCM_I2CFIF_CTS);
+		npcm_i2c_slave_rd_wr(bus);
+		ret =  IRQ_HANDLED;
+	}
+
+	// A Slave Address Match has been identified
+	if (FIELD_GET(NPCM_I2CST_NMATCH, i2cst)) {
+		u8 info = 0;
+
+		// Address match automatically implies slave mode
+		bus->master_or_slave = I2C_SLAVE;
+		npcm_i2c_clear_fifo_int(bus);
+		npcm_i2c_clear_rx_fifo(bus);
+		npcm_i2c_clear_tx_fifo(bus);
+		iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
+		iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CRXF_CTL);
+		if (FIELD_GET(NPCM_I2CST_XMIT, i2cst)) {
+			bus->operation = I2C_WRITE_OPER;
+		} else {
+			i2c_slave_event(bus->slave, I2C_SLAVE_WRITE_REQUESTED,
+					&info);
+			bus->operation = I2C_READ_OPER;
+		}
+		if (bus->own_slave_addr == 0xFF) { // unknown address
+			// Check which type of address match
+			val = ioread8(bus->reg + NPCM_I2CCST);
+			if (!!(NPCM_I2CCST_MATCH & val)) {
+				u16 addr;
+				enum i2c_addr eaddr;
+				u8 i2ccst2;
+				u8 i2ccst3;
+
+				i2ccst3 = ioread8(bus->reg + NPCM_I2CCST3);
+				i2ccst2 = ioread8(bus->reg + NPCM_I2CCST2);
+
+				// the i2c module can response to 10 own SA.
+				// check which one was addressed by the master.
+				// repond to the first one.
+				addr = ((i2ccst3 & 0x07) << 7) |
+					(i2ccst2 & 0x7F);
+				info = ffs(addr);
+				eaddr = (enum i2c_addr)info;
+				addr = npcm_i2c_get_slave_addr(bus, eaddr);
+				addr &= 0x7F;
+				bus->own_slave_addr = addr;
+				if (bus->PEC_mask & BIT(info))
+					bus->PEC_use = true;
+				else
+					bus->PEC_use = false;
+			} else {
+				val = ioread8(bus->reg + NPCM_I2CCST);
+				if (!!(NPCM_I2CCST_GCMATCH & val))
+					bus->own_slave_addr = 0;
+				if (!!(NPCM_I2CCST_ARPMATCH & val))
+					bus->own_slave_addr = 0x61;
+			}
+		} else {
+			/*
+			 *  Slave match can happen in two options:
+			 *  1. Start, SA, read	(slave read without further ado)
+			 *  2. Start, SA, read, data, restart, SA, read,  ...
+			 *     (slave read in fragmented mode)
+			 *  3. Start, SA, write, data, restart, SA, read, ..
+			 *     (regular write-read mode)
+			 */
+			if ((bus->state == I2C_OPER_STARTED &&
+			     bus->operation == I2C_READ_OPER &&
+			     bus->stop_ind == I2C_SLAVE_XMIT_IND) ||
+			     bus->stop_ind == I2C_SLAVE_RCV_IND) {
+				// slave transmit after slave receive w/o Slave
+				// Stop implies repeated start
+				bus->stop_ind = I2C_SLAVE_RESTART_IND;
+			}
+		}
+
+		if (FIELD_GET(NPCM_I2CST_XMIT, i2cst))
+			bus->stop_ind = I2C_SLAVE_XMIT_IND;
+		else
+			bus->stop_ind = I2C_SLAVE_RCV_IND;
+		bus->state = I2C_SLAVE_MATCH;
+		npcm_i2c_slave_rd_wr(bus);
+		iowrite8(NPCM_I2CST_NMATCH, bus->reg + NPCM_I2CST);
+		ret = IRQ_HANDLED;
+	}
+
+	// Slave SDA status is set - transmit or receive, slave
+	if (FIELD_GET(NPCM_I2CST_SDAST, i2cst) ||
+	    (bus->fifo_use   &&
+	    (npcm_i2c_tx_fifo_empty(bus) || npcm_i2c_rx_fifo_full(bus)))) {
+		npcm_i2c_slave_rd_wr(bus);
+		iowrite8(NPCM_I2CST_SDAST, bus->reg + NPCM_I2CST);
+		ret = IRQ_HANDLED;
+	} //SDAST
+
+	return ret;
+}
+
+static int  npcm_i2c_reg_slave(struct i2c_client *client)
+{
+	unsigned long lock_flags;
+	struct npcm_i2c *bus = i2c_get_adapdata(client->adapter);
+
+	bus->slave = client;
+
+	if (!bus->slave)
+		return -EINVAL;
+
+	if (client->flags & I2C_CLIENT_TEN)
+		return -EAFNOSUPPORT;
+
+	spin_lock_irqsave(&bus->lock, lock_flags);
+
+	npcm_i2c_init_params(bus);
+	bus->slv_rd_size = 0;
+	bus->slv_wr_size = 0;
+	bus->slv_rd_ind = 0;
+	bus->slv_wr_ind = 0;
+	if (client->flags & I2C_CLIENT_PEC)
+		bus->PEC_use = true;
+
+	dev_info(bus->dev, "i2c%d register slave SA=0x%x, PEC=%d\n", bus->num,
+		 client->addr, bus->PEC_use);
+
+	npcm_i2c_slave_enable_l(bus, I2C_SLAVE_ADDR1, client->addr, true);
+	npcm_i2c_clear_fifo_int(bus);
+	npcm_i2c_clear_rx_fifo(bus);
+	npcm_i2c_clear_tx_fifo(bus);
+	npcm_i2c_slave_int_enable(bus, true);
+
+	spin_unlock_irqrestore(&bus->lock, lock_flags);
+	return 0;
+}
+
+static int  npcm_i2c_unreg_slave(struct i2c_client *client)
+{
+	struct npcm_i2c *bus = client->adapter->algo_data;
+	unsigned long lock_flags;
+
+	spin_lock_irqsave(&bus->lock, lock_flags);
+	if (!bus->slave) {
+		spin_unlock_irqrestore(&bus->lock, lock_flags);
+		return -EINVAL;
+	}
+	npcm_i2c_slave_int_enable(bus, false);
+	npcm_i2c_remove_slave_addr(bus, client->addr);
+	bus->slave = NULL;
+	spin_unlock_irqrestore(&bus->lock, lock_flags);
+	return 0;
+}
+#endif // CONFIG_I2C_SLAVE
+
 static void npcm_i2c_master_fifo_read(struct npcm_i2c *bus)
 {
 	int rcount;
@@ -1393,6 +1981,9 @@ static int __npcm_i2c_init(struct npcm_i2c *bus, struct platform_device *pdev)
 	bus->state = I2C_DISABLE;
 	bus->master_or_slave = I2C_SLAVE;
 	bus->int_time_stamp = 0;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	bus->slave = NULL;
+#endif
 	bus->xmits = 0;
 
 	ret = device_property_read_u32(&pdev->dev, "bus-frequency", &clk_freq);
@@ -1426,6 +2017,14 @@ static irqreturn_t npcm_i2c_bus_irq(int irq, void *dev_id)
 		if (ret == IRQ_HANDLED)
 			return ret;
 	}
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (bus->slave) {
+		bus->master_or_slave = I2C_SLAVE;
+		ret = npcm_i2c_int_slave_handler(bus);
+		if (ret == IRQ_HANDLED)
+			return ret;
+	}
+#endif
 	return IRQ_HANDLED;
 }
 
@@ -1568,6 +2167,11 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		 */
 		spin_lock_irqsave(&bus->lock, flags);
 		bus_busy = ioread8(bus->reg + NPCM_I2CCST) & NPCM_I2CCST_BB;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		if (!bus_busy && bus->slave)
+			iowrite8((bus->slave->addr & 0x7F),
+				 bus->reg + NPCM_I2CADDR1);
+#endif
 		spin_unlock_irqrestore(&bus->lock, flags);
 
 	} while (time_is_after_jiffies(time_left) && bus_busy);
@@ -1620,6 +2224,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	if (bus->cmd_err == -EAGAIN)
 		ret = i2c_recover_bus(adap);
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	// reenable slave if it was enabled
+	if (bus->slave)
+		iowrite8((bus->slave->addr & 0x7F) | NPCM_I2CADDR_SAEN,
+			 bus->reg + NPCM_I2CADDR1);
+#endif
 	return bus->cmd_err;
 }
 
@@ -1642,6 +2252,10 @@ static const struct i2c_adapter_quirks npcm_i2c_quirks = {
 static const struct i2c_algorithm npcm_i2c_algo = {
 	.master_xfer = npcm_i2c_master_xfer,
 	.functionality = npcm_i2c_functionality,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	.reg_slave	= npcm_i2c_reg_slave,
+	.unreg_slave	= npcm_i2c_unreg_slave,
+#endif
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
Signature · 7 lines
2.22.0


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel