On Thu, May 21, 2020 at 02:09:09PM +0300, Tali Perry wrote:

Add Nuvoton NPCM BMC I2C controller driver.

Signed-off-by: Tali Perry <tali.perry1@gmail.com>

This is a very complex driver, so I can really comment only about high
level things. Thank you very much for keeping at it!

My code checkers say:

CHECKPATCH:
CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst
#1210: FILE: drivers/i2c/busses/i2c-npcm7xx.c:1161:
+			udelay(200);

(a few of them)

GCC:
  CC      drivers/i2c/busses/i2c-npcm7xx.o
drivers/i2c/busses/i2c-npcm7xx.c: In function ‘npcm_i2c_reset’:
drivers/i2c/busses/i2c-npcm7xx.c:521:5: warning: variable ‘i2cctl2’ set but not used [-Wunused-but-set-variable]

+/* Status of one I2C 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 i2c_state state;
+	enum i2c_oper operation;
+	enum i2c_mode master_or_slave;
+	enum i2c_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;
+	u16 PEC_mask; /* PEC bit mask per slave address */
+	bool PEC_use;
+	bool read_block_use;
+	u8 int_cnt;

What is this for? It is written to but never read.

+	u32 clk_period_us;

Not used? Seems this struct could need some cleaning up.

+	unsigned long int_time_stamp;
+	unsigned long bus_freq; /* in kHz */
+	u32 xmits;
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *debugfs; /* debugfs device directory */
+	u64 ber_cnt;
+	u64 rec_succ_cnt;
+	u64 rec_fail_cnt;
+	u64 nack_cnt;
+	u64 timeout_cnt;
+#endif
+};
+

...

+static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
+{
+	if (bus->operation == I2C_READ_OPER)
+		return bus->rd_ind;
+	if (bus->operation == I2C_WRITE_OPER)
+		return bus->wr_ind;
+	return 0;

I2C_NO_OPER?

...

+/* recovery using bit banging functionality of the module */
+static int npcm_i2c_recovery_init(struct i2c_adapter *_adap)
+{
+	struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
+	struct i2c_bus_recovery_info *rinfo = &bus->rinfo;
+
+	rinfo->recover_bus = npcm_i2c_recovery_tgclk;
+	rinfo->prepare_recovery = NULL;
+	rinfo->unprepare_recovery = NULL;
+	rinfo->set_scl = NULL;
+	rinfo->set_sda = NULL;

'bus' is kzalloced, so no need for these NULLs.

What I wonder more, though, is if you can't populate {set|get}_{scl|sda}
and use the internal i2c_generic_scl_recovery()? Are there any issues
with it?

+
+	dev_dbg(bus->dev, "init i2c recovery using TGCLK\n");

There is no error path here, so I think this message is not useful.
Means also this function could be 'void'.

+
+	rinfo->get_scl = npcm_i2c_get_SCL;
+	rinfo->get_sda = npcm_i2c_get_SDA;

Not needed when you have a custom function.

+
+	_adap->bus_recovery_info = rinfo;
+
+	return 0;
+}
+

...

+static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+				int num)
+{
+	struct npcm_i2c *bus = container_of(adap, struct npcm_i2c, adap);
+	struct i2c_msg *msg0, *msg1;
+	unsigned long time_left, flags;
+	u16 nwrite, nread;
+	u8 *write_data, *read_data;
+	u8 slave_addr;
+	int timeout;
+	int ret = 0;
+	bool read_block = false;
+	bool read_PEC = false;
+	u8 bus_busy;
+	unsigned long timeout_usec;
+
+	if (bus->state == I2C_DISABLE) {
+		dev_err(bus->dev, "I2C%d module is disabled", bus->num);
+		return -EINVAL;
+	}
+
+	if (num > 2 || num < 1) {
+		dev_err(bus->dev, "I2C cmd not supported num of msgs=%d", num);
+		return -EINVAL;
+	}

Since you have an 'i2c_adapter_quirks' struct filled, the core will
I2C check that for you.

+
+	msg0 = &msgs[0];
+	slave_addr = msg0->addr;
+	if (msg0->flags & I2C_M_RD) { /* read */
+		if (num == 2) {
+			dev_err(bus->dev, "num=2 but 1st msg rd instead of wr");
+			return -EINVAL;

Ditto.

+		}
+		nwrite = 0;
+		write_data = NULL;
+		read_data = msg0->buf;
+		if (msg0->flags & I2C_M_RECV_LEN) {
+			nread = 1;
+			read_block = true;
+			if (msg0->flags & I2C_CLIENT_PEC)
+				read_PEC = true;
+		} else {
+			nread = msg0->len;
+		}
+	} else { /* write */
+		nwrite = msg0->len;
+		write_data = msg0->buf;
+		nread = 0;
+		read_data = NULL;
+		if (num == 2) {
+			msg1 = &msgs[1];
+			read_data = msg1->buf;
+			if (slave_addr != msg1->addr) {
+				dev_err(bus->dev,
+					"SA==%02x but msg1->addr==%02x\n",
+				       slave_addr, msg1->addr);
+				return -EINVAL;

Ditto.

+			}
+			if ((msg1->flags & I2C_M_RD) == 0) {
+				dev_err(bus->dev,
+					"num = 2 but both msg are write.\n");
+				return -EINVAL;
+			}

Ditto.

+			if (msg1->flags & I2C_M_RECV_LEN) {
+				nread = 1;
+				read_block = true;
+				if (msg1->flags & I2C_CLIENT_PEC)
+					read_PEC = true;
+			} else {
+				nread = msg1->len;
+				read_block = false;
+			}
+		}
+	}
+
+	/* Adaptive TimeOut: astimated time in usec  + 100% margin */
+	timeout_usec = (2 * 10000 / bus->bus_freq) * (2 + nread + nwrite);
+	timeout = max(msecs_to_jiffies(35), usecs_to_jiffies(timeout_usec));
+	if (nwrite >= 32 * 1024 ||  nread >= 32 * 1024) {
+		dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
+		return -EINVAL;
+	}

Ditto.

+
+	time_left = jiffies + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
+	do {
+		/*
+		 * we must clear slave address immediately when the bus is not
+		 * busy, so we spinlock it, but we don't keep the lock for the
+		 * entire while since it is too long.
+		 */
+		spin_lock_irqsave(&bus->lock, flags);
+		bus_busy = ioread8(bus->reg + NPCM_I2CCST) & NPCM_I2CCST_BB;
+		spin_unlock_irqrestore(&bus->lock, flags);
+
+	} while (time_is_after_jiffies(time_left) && bus_busy);
+
+	if (bus_busy) {
+		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
+		npcm_i2c_reset(bus);
+		i2c_recover_bus(adap);
+		return -EAGAIN;
+	}
+
+	npcm_i2c_init_params(bus);
+	bus->dest_addr = slave_addr;
+	bus->msgs = msgs;
+	bus->msgs_num = num;
+	bus->cmd_err = 0;
+	bus->read_block_use = read_block;
+
+	reinit_completion(&bus->cmd_complete);
+	if (!npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
+					write_data, read_data, read_PEC,
+					read_block))
+		ret = -EBUSY;
+
+	if (ret != -EBUSY) {
+		time_left = wait_for_completion_timeout(&bus->cmd_complete,
+							timeout);
+
+		if (time_left == 0) {
+#ifdef CONFIG_DEBUG_FS
+			if (bus->timeout_cnt == ULLONG_MAX) {
+				dev_dbg(bus->dev,
+					"timeout_cnt reach max, reset to 0");
+				bus->timeout_cnt = 0;
+			}
+			bus->timeout_cnt++;
+#endif
+			if (bus->master_or_slave == I2C_MASTER) {
+				i2c_recover_bus(adap);
+				bus->cmd_err = -EIO;
+				bus->state = I2C_IDLE;
+			}
+		}
+	}
+	ret = bus->cmd_err;
+
+	/* if there was BER, check if need to recover the bus: */
+	if (bus->cmd_err == -EAGAIN)
+		ret = i2c_recover_bus(adap);
+
+	return bus->cmd_err;
+}
+
+static u32 npcm_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C |
+	       I2C_FUNC_SMBUS_EMUL |
+	       I2C_FUNC_SMBUS_BLOCK_DATA |
+	       I2C_FUNC_SMBUS_PEC |
+	       I2C_FUNC_SLAVE;
+}
+
+static const struct i2c_adapter_quirks npcm_i2c_quirks = {
+	.max_read_len = 32768,
+	.max_write_len = 32768,

These limits are for simple reads/writes with num_msgs == 1. If you have
limits also for num_msgs == 2, then you also need to fill
'max_comb_1st_msg_len' and 'max_comb_2nd_msg_len'. (Because for some HW
these are different values then)

+	.max_num_msgs = 2,

You can drop this because I2C_AQ_COMB_WRITE_THEN_READ implies it.

+	.flags = I2C_AQ_COMB_WRITE_THEN_READ,
+};
+
+static const struct i2c_algorithm npcm_i2c_algo = {
+	.master_xfer = npcm_i2c_master_xfer,
+	.functionality = npcm_i2c_functionality,
+};
+
+#ifdef CONFIG_DEBUG_FS
+/* i2c debugfs directory: used to keep health monitor of i2c devices */
+static struct dentry *npcm_i2c_debugfs_dir;
+
+static void i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus)
+{
+	struct dentry *d;
+
+	if (!npcm_i2c_debugfs_dir)
+		return;
+
+	d = debugfs_create_dir(dev_name(&pdev->dev), npcm_i2c_debugfs_dir);
+	if (IS_ERR_OR_NULL(d))
+		return;
+
+	debugfs_create_u64("ber_cnt", 0444, d, &bus->ber_cnt);
+	debugfs_create_u64("nack_cnt", 0444, d, &bus->nack_cnt);
+	debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
+	debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
+	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
+
+	bus->debugfs = d;
+}
+#else
+static void i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus)
+{
+}
+#endif
+
+static int  npcm_i2c_probe_bus(struct platform_device *pdev)
+{
+	struct npcm_i2c *bus;
+	struct i2c_adapter *adap;
+	struct clk *i2c_clk;
+	static struct regmap *gcr_regmap;
+	static struct regmap *clk_regmap;
+	int ret;
+	int num;
+
+	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+	if (!bus)
+		return -ENOMEM;
+
+	bus->dev = &pdev->dev;
+
+	num = of_alias_get_id(pdev->dev.of_node, "i2c");
+	bus->num = num;

Why not assigning it directly and save the 'num' variable?

+	/* core clk must be acquired to calculate module timing settings */
+	i2c_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(i2c_clk))
+		return PTR_ERR(i2c_clk);
+	bus->apb_clk = clk_get_rate(i2c_clk);
+
+	gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
+	if (IS_ERR(gcr_regmap))
+		return IS_ERR(gcr_regmap);
+	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
+
+	clk_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-clk");
+	if (IS_ERR(clk_regmap))
+		return IS_ERR(clk_regmap);
+
+	bus->reg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(bus->reg))
+		return PTR_ERR((bus)->reg);
+
+	spin_lock_init(&bus->lock);
+	init_completion(&bus->cmd_complete);
+
+	adap = &bus->adap;
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD | I2C_CLIENT_SLAVE;

Since you have a DT compatible, you won't need classes. Just drop them.

+	adap->retries = 3;
+	adap->timeout = HZ;
+	adap->algo = &npcm_i2c_algo;
+	adap->quirks = &npcm_i2c_quirks;
+	adap->algo_data = bus;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+	adap->nr = pdev->id;
+
+	bus->irq = platform_get_irq(pdev, 0);
+	if (bus->irq < 0)
+		return bus->irq;
+
+	ret = devm_request_irq(bus->dev, bus->irq, npcm_i2c_bus_irq, 0,
+			       dev_name(bus->dev), bus);
+	if (ret)
+		return ret;
+
+	ret = __npcm_i2c_init(bus, pdev);
+	if (ret)
+		return ret;
+
+	ret = npcm_i2c_recovery_init(adap);
+	if (ret)
+		return ret;
+
+	i2c_set_adapdata(adap, bus);
+
+	snprintf(bus->adap.name, sizeof(bus->adap.name), "Nuvoton i2c");

Maybe you want to add something more specific in case you have multiple
instances of this driver at runtime.

+	ret = i2c_add_numbered_adapter(&bus->adap);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add numbered adapter %d\n", ret);

The I2C core will print warnings for you.

+		return ret;
+	}
+	platform_set_drvdata(pdev, bus);
+
+	i2c_init_debugfs(pdev, bus);
+	return 0;
+}
+

...

+#ifdef CONFIG_DEBUG_FS
+static int __init npcm_i2c_init(void)
+{
+	struct dentry *dir;
+
+	dir = debugfs_create_dir("i2c", NULL);

Okay, the GPIO fault injector could also need such a directory. I will
add this to the core. And then send an incremental patch for your
driver.

+	if (IS_ERR_OR_NULL(dir))
+		return 0;
+
+	npcm_i2c_debugfs_dir = dir;
+	return 0;
+}
+
+static void __exit npcm_i2c_exit(void)
+{
+	debugfs_remove_recursive(npcm_i2c_debugfs_dir);
+}
+
+module_init(npcm_i2c_init);
+module_exit(npcm_i2c_exit);
+#endif
+
+MODULE_AUTHOR("Avi Fishman [off-list ref]");
+MODULE_AUTHOR("Tali Perry [off-list ref]");
+MODULE_AUTHOR("Tyrone Ting [off-list ref]");
+MODULE_DESCRIPTION("Nuvoton I2C Bus Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("0.1.3");
+
-- 
2.22.0