[PATCH v4 0/4] i2c: thunderx: Marvell thunderx i2c changes

[PATCH v4 0/4] i2c: thunderx: Marvell thunderx i2c changes

[Piyush Malgujar]

The changes are for Marvell OcteonTX2 SOC family:

- Handling clock divisor logic using subsytem ID
- Support for high speed mode
- Handle watchdog timeout
- Added ioclk support

Changes since V3:
- Removed the MAINTAINER file change from this series
- Modified the commit message to include more details
- Minor changes such as adding macros, comments modified
  to have more detail as required

Changes since V2:
- Respinning the series, no functional change
- Added Marvell member in MAINTAINERS file
- Added macro OTX2_REF_FREQ_DEFAULT for 100 MHz

Changes since V1:
- Addressed comments, added defines as required
- Removed unnecessary code
- Added a patch to support ioclk if sclk not present in ACPI table

Piyush Malgujar (1):
  i2c: thunderx: Adding ioclk support

Suneel Garapati (3):
  i2c: thunderx: Clock divisor logic changes
  i2c: thunderx: Add support for High speed mode
  i2c: octeon: Handle watchdog timeout

 drivers/i2c/busses/i2c-octeon-core.c     | 105 +++++++++++++++++------
 drivers/i2c/busses/i2c-octeon-core.h     |  25 ++++++
 drivers/i2c/busses/i2c-thunderx-pcidrv.c |  23 +++--
 3 files changed, 123 insertions(+), 30 deletions(-)

-- 
2.43.0

[PATCH v4 1/4] i2c: thunderx: Clock divisor logic changes

[Piyush Malgujar]

From: Suneel Garapati <sgarapati@marvell.com>

Handle changes to clock divisor logic for OcteonTX2 SoC family using
subsystem ID and using default reference clock source as 100MHz.

Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
Acked-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-octeon-core.c     | 38 +++++++++++++++++++++---
 drivers/i2c/busses/i2c-octeon-core.h     | 15 ++++++++++
 drivers/i2c/busses/i2c-thunderx-pcidrv.c |  7 +++++
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 845eda70b8cab52a0453c9f4cb545010fba4305d..10330ed3203f9fd99d5c04dbaf29a9bd49ad0f4a 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -17,9 +17,14 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 
 #include "i2c-octeon-core.h"
 
+#define INITIAL_DELTA_HZ	1000000
+#define TWSI_MASTER_CLK_REG_DEF_VAL	0x18
+#define TWSI_MASTER_CLK_REG_OTX2_VAL	0x3
+
 /* interrupt service routine */
 irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
 {
@@ -658,31 +663,56 @@ int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 void octeon_i2c_set_clock(struct octeon_i2c *i2c)
 {
 	int tclk, thp_base, inc, thp_idx, mdiv_idx, ndiv_idx, foscl, diff;
-	int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = 1000000;
+	unsigned int mdiv_min = 2;
+	/*
+	 * Find divisors to produce target frequency, start with large delta
+	 * to cover wider range of divisors, note thp = TCLK half period.
+	 */
+	unsigned int thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;
+	unsigned int delta_hz = INITIAL_DELTA_HZ;
+
+	bool is_plat_otx2 = octeon_i2c_is_otx2(to_pci_dev(i2c->dev));
+
+	if (is_plat_otx2) {
+		thp = TWSI_MASTER_CLK_REG_OTX2_VAL;
+		mdiv_min = 0;
+	}
 
 	for (ndiv_idx = 0; ndiv_idx < 8 && delta_hz != 0; ndiv_idx++) {
 		/*
 		 * An mdiv value of less than 2 seems to not work well
 		 * with ds1337 RTCs, so we constrain it to larger values.
 		 */
-		for (mdiv_idx = 15; mdiv_idx >= 2 && delta_hz != 0; mdiv_idx--) {
+		for (mdiv_idx = 15; mdiv_idx >= mdiv_min && delta_hz != 0; mdiv_idx--) {
 			/*
 			 * For given ndiv and mdiv values check the
 			 * two closest thp values.
 			 */
 			tclk = i2c->twsi_freq * (mdiv_idx + 1) * 10;
 			tclk *= (1 << ndiv_idx);
-			thp_base = (i2c->sys_freq / (tclk * 2)) - 1;
+			if (is_plat_otx2)
+				thp_base = (i2c->sys_freq / tclk) - 2;
+			else
+				thp_base = (i2c->sys_freq / (tclk * 2)) - 1;
 
 			for (inc = 0; inc <= 1; inc++) {
 				thp_idx = thp_base + inc;
 				if (thp_idx < 5 || thp_idx > 0xff)
 					continue;
 
-				foscl = i2c->sys_freq / (2 * (thp_idx + 1));
+				if (is_plat_otx2)
+					foscl = i2c->sys_freq / (thp_idx + 2);
+				else
+					foscl = i2c->sys_freq /
+						(2 * (thp_idx + 1));
 				foscl = foscl / (1 << ndiv_idx);
 				foscl = foscl / (mdiv_idx + 1) / 10;
 				diff = abs(foscl - i2c->twsi_freq);
+				/*
+				 * Diff holds difference between calculated frequency
+				 * value vs desired frequency.
+				 * Delta_hz is updated with last minimum diff.
+				 */
 				if (diff < delta_hz) {
 					delta_hz = diff;
 					thp = thp_idx;
diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index 9bb9f64fdda0392364638ecbaafe3fab5612baf6..8a0033c94a8a291fb255b0da03858274035c46f4 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -7,6 +7,7 @@
 #include <linux/i2c-smbus.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/pci.h>
 
 /* Controller command patterns */
 #define SW_TWSI_V		BIT_ULL(63)	/* Valid bit */
@@ -211,6 +212,20 @@ static inline void octeon_i2c_write_int(struct octeon_i2c *i2c, u64 data)
 	octeon_i2c_writeq_flush(data, i2c->twsi_base + TWSI_INT(i2c));
 }
 
+#define PCI_SUBSYS_DEVID_9XXX 0xB
+/**
+ * octeon_i2c_is_otx2 - check for chip ID
+ * @pdev: PCI dev structure
+ *
+ * Returns TRUE if OcteonTX2, FALSE otherwise.
+ */
+static inline bool octeon_i2c_is_otx2(struct pci_dev *pdev)
+{
+	u32 chip_id = (pdev->subsystem_device >> 12) & 0xF;
+
+	return (chip_id == PCI_SUBSYS_DEVID_9XXX);
+}
+
 /* Prototypes */
 irqreturn_t octeon_i2c_isr(int irq, void *dev_id);
 int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
index a77cd86fe75ed7401bc041b27c651b9fedf67285..75569774003857dc984e8540ef8f4d1bb084cfb0 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -28,6 +28,7 @@
 #define PCI_DEVICE_ID_THUNDER_TWSI	0xa012
 
 #define SYS_FREQ_DEFAULT		700000000
+#define OTX2_REF_FREQ_DEFAULT		100000000
 
 #define TWSI_INT_ENA_W1C		0x1028
 #define TWSI_INT_ENA_W1S		0x1030
@@ -205,6 +206,12 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 	if (ret)
 		goto error;
 
+	/*
+	 * For OcteonTX2 chips, set reference frequency to 100MHz
+	 * as refclk_src in TWSI_MODE register defaults to 100MHz.
+	 */
+	if (octeon_i2c_is_otx2(pdev))
+		i2c->sys_freq = OTX2_REF_FREQ_DEFAULT;
 	octeon_i2c_set_clock(i2c);
 
 	i2c->adap = thunderx_i2c_ops;
-- 
2.43.0

[PATCH v4 2/4] i2c: thunderx: Add support for High speed mode

[Piyush Malgujar]

From: Suneel Garapati <sgarapati@marvell.com>

Support High speed mode clock setup for OcteonTX2 platforms.
hs_mode bit in MODE register controls speed mode setup in controller
and frequency is setup using set_clock function which sets up dividers
for clock control register.

Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 drivers/i2c/busses/i2c-octeon-core.c     | 61 +++++++++++++++---------
 drivers/i2c/busses/i2c-octeon-core.h     |  6 +++
 drivers/i2c/busses/i2c-thunderx-pcidrv.c |  3 +-
 3 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 10330ed3203f9fd99d5c04dbaf29a9bd49ad0f4a..36e7beeab25c63c2341313d6aa3421049ad92990 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -612,25 +612,27 @@ int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
 	int i, ret = 0;
 
-	if (num == 1) {
-		if (msgs[0].len > 0 && msgs[0].len <= 8) {
-			if (msgs[0].flags & I2C_M_RD)
-				ret = octeon_i2c_hlc_read(i2c, msgs);
-			else
-				ret = octeon_i2c_hlc_write(i2c, msgs);
-			goto out;
-		}
-	} else if (num == 2) {
-		if ((msgs[0].flags & I2C_M_RD) == 0 &&
-		    (msgs[1].flags & I2C_M_RECV_LEN) == 0 &&
-		    msgs[0].len > 0 && msgs[0].len <= 2 &&
-		    msgs[1].len > 0 && msgs[1].len <= 8 &&
-		    msgs[0].addr == msgs[1].addr) {
-			if (msgs[1].flags & I2C_M_RD)
-				ret = octeon_i2c_hlc_comp_read(i2c, msgs);
-			else
-				ret = octeon_i2c_hlc_comp_write(i2c, msgs);
-			goto out;
+	if (IS_LS_FREQ(i2c->twsi_freq)) {
+		if (num == 1) {
+			if (msgs[0].len > 0 && msgs[0].len <= 8) {
+				if (msgs[0].flags & I2C_M_RD)
+					ret = octeon_i2c_hlc_read(i2c, msgs);
+				else
+					ret = octeon_i2c_hlc_write(i2c, msgs);
+				goto out;
+			}
+		} else if (num == 2) {
+			if ((msgs[0].flags & I2C_M_RD) == 0 &&
+			    (msgs[1].flags & I2C_M_RECV_LEN) == 0 &&
+			    msgs[0].len > 0 && msgs[0].len <= 2 &&
+			    msgs[1].len > 0 && msgs[1].len <= 8 &&
+			    msgs[0].addr == msgs[1].addr) {
+				if (msgs[1].flags & I2C_M_RD)
+					ret = octeon_i2c_hlc_comp_read(i2c, msgs);
+				else
+					ret = octeon_i2c_hlc_comp_write(i2c, msgs);
+				goto out;
+			}
 		}
 	}
 
@@ -668,7 +670,7 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
 	 * Find divisors to produce target frequency, start with large delta
 	 * to cover wider range of divisors, note thp = TCLK half period.
 	 */
-	unsigned int thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;
+	unsigned int ds = 10, thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;
 	unsigned int delta_hz = INITIAL_DELTA_HZ;
 
 	bool is_plat_otx2 = octeon_i2c_is_otx2(to_pci_dev(i2c->dev));
@@ -676,6 +678,8 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
 	if (is_plat_otx2) {
 		thp = TWSI_MASTER_CLK_REG_OTX2_VAL;
 		mdiv_min = 0;
+		if (!IS_LS_FREQ(i2c->twsi_freq))
+			ds = 15;
 	}
 
 	for (ndiv_idx = 0; ndiv_idx < 8 && delta_hz != 0; ndiv_idx++) {
@@ -688,7 +692,7 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
 			 * For given ndiv and mdiv values check the
 			 * two closest thp values.
 			 */
-			tclk = i2c->twsi_freq * (mdiv_idx + 1) * 10;
+			tclk = i2c->twsi_freq * (mdiv_idx + 1) * ds;
 			tclk *= (1 << ndiv_idx);
 			if (is_plat_otx2)
 				thp_base = (i2c->sys_freq / tclk) - 2;
@@ -706,7 +710,9 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
 					foscl = i2c->sys_freq /
 						(2 * (thp_idx + 1));
 				foscl = foscl / (1 << ndiv_idx);
-				foscl = foscl / (mdiv_idx + 1) / 10;
+				foscl = foscl / (mdiv_idx + 1) / ds;
+				if (foscl > i2c->twsi_freq)
+					continue;
 				diff = abs(foscl - i2c->twsi_freq);
 				/*
 				 * Diff holds difference between calculated frequency
@@ -724,6 +730,17 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
 	}
 	octeon_i2c_reg_write(i2c, SW_TWSI_OP_TWSI_CLK, thp);
 	octeon_i2c_reg_write(i2c, SW_TWSI_EOP_TWSI_CLKCTL, (mdiv << 3) | ndiv);
+	if (is_plat_otx2) {
+		u64 mode;
+
+		mode = __raw_readq(i2c->twsi_base + MODE(i2c));
+		/* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
+		if (!IS_LS_FREQ(i2c->twsi_freq))
+			mode |= TWSX_MODE_HS_MASK;
+		else
+			mode &= ~TWSX_MODE_HS_MASK;
+		octeon_i2c_writeq_flush(mode, i2c->twsi_base + MODE(i2c));
+	}
 }
 
 int octeon_i2c_init_lowlevel(struct octeon_i2c *i2c)
diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index 8a0033c94a8a291fb255b0da03858274035c46f4..5927d24926ce898efbee111046092960e48ce3ff 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -93,11 +93,16 @@ struct octeon_i2c_reg_offset {
 	unsigned int sw_twsi;
 	unsigned int twsi_int;
 	unsigned int sw_twsi_ext;
+	unsigned int mode;
 };
 
 #define SW_TWSI(x)	(x->roff.sw_twsi)
 #define TWSI_INT(x)	(x->roff.twsi_int)
 #define SW_TWSI_EXT(x)	(x->roff.sw_twsi_ext)
+#define MODE(x)		(x->roff.mode)
+
+/* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
+#define TWSX_MODE_HS_MASK	(BIT(4) | BIT(0))
 
 struct octeon_i2c {
 	wait_queue_head_t queue;
@@ -212,6 +217,7 @@ static inline void octeon_i2c_write_int(struct octeon_i2c *i2c, u64 data)
 	octeon_i2c_writeq_flush(data, i2c->twsi_base + TWSI_INT(i2c));
 }
 
+#define IS_LS_FREQ(twsi_freq) ((twsi_freq) <= 400000)
 #define PCI_SUBSYS_DEVID_9XXX 0xB
 /**
  * octeon_i2c_is_otx2 - check for chip ID
diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
index 75569774003857dc984e8540ef8f4d1bb084cfb0..31f11b77ab663626967c86086a03213876bf4a07 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -166,6 +166,7 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 	i2c->roff.sw_twsi = 0x1000;
 	i2c->roff.twsi_int = 0x1010;
 	i2c->roff.sw_twsi_ext = 0x1018;
+	i2c->roff.mode = 0x1038;
 
 	i2c->dev = dev;
 	pci_set_drvdata(pdev, i2c);
@@ -210,7 +211,7 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 	 * For OcteonTX2 chips, set reference frequency to 100MHz
 	 * as refclk_src in TWSI_MODE register defaults to 100MHz.
 	 */
-	if (octeon_i2c_is_otx2(pdev))
+	if (octeon_i2c_is_otx2(pdev) && IS_LS_FREQ(i2c->twsi_freq))
 		i2c->sys_freq = OTX2_REF_FREQ_DEFAULT;
 	octeon_i2c_set_clock(i2c);
 
-- 
2.43.0

[PATCH v4 3/4] i2c: octeon: Handle watchdog timeout

[Piyush Malgujar]

From: Suneel Garapati <sgarapati@marvell.com>

Add watchdog timeout handling to cater to the unhandled warnings
seen during validation on boards with different I2C slaves.
This status code reflects the state that controller couldn't
receive any response from slave while being in non-idle state
and HW recommends to reset before any further bus access.

Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
Acked-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-octeon-core.c | 8 ++++++++
 drivers/i2c/busses/i2c-octeon-core.h | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 36e7beeab25c63c2341313d6aa3421049ad92990..7cc50fe6ad3d032241bf2cc2e49c0343b72a6c03 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -182,6 +182,7 @@ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
 static int octeon_i2c_check_status(struct octeon_i2c *i2c, int final_read)
 {
 	u8 stat;
+	u64 mode;
 
 	/*
 	 * This is ugly... in HLC mode the status is not in the status register
@@ -244,6 +245,13 @@ static int octeon_i2c_check_status(struct octeon_i2c *i2c, int final_read)
 	case STAT_RXADDR_NAK:
 	case STAT_AD2W_NAK:
 		return -ENXIO;
+
+	case STAT_WDOG_TOUT:
+		mode = __raw_readq(i2c->twsi_base + MODE(i2c));
+		/* Set BUS_MON_RST to reset bus monitor */
+		mode |= BUS_MON_RST_MASK;
+		octeon_i2c_writeq_flush(mode, i2c->twsi_base + MODE(i2c));
+		return -EIO;
 	default:
 		dev_err(i2c->dev, "unhandled state: %d\n", stat);
 		return -EIO;
diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index 5927d24926ce898efbee111046092960e48ce3ff..aa0a6cf6ad67185cd116a2d674dccf97c57600c1 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -72,6 +72,7 @@
 #define STAT_SLAVE_ACK		0xC8
 #define STAT_AD2W_ACK		0xD0
 #define STAT_AD2W_NAK		0xD8
+#define STAT_WDOG_TOUT		0xF0
 #define STAT_IDLE		0xF8
 
 /* TWSI_INT values */
@@ -104,6 +105,9 @@ struct octeon_i2c_reg_offset {
 /* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
 #define TWSX_MODE_HS_MASK	(BIT(4) | BIT(0))
 
+/* Set BUS_MON_RST to reset bus monitor */
+#define BUS_MON_RST_MASK	BIT(3)
+
 struct octeon_i2c {
 	wait_queue_head_t queue;
 	struct i2c_adapter adap;
-- 
2.43.0

[PATCH v4 4/4] i2c: thunderx: Adding ioclk support

[Piyush Malgujar]

Read the ioclk property as reference clock if sclk not
present in acpi table to make it SOC agnostic.
In case, it's not populated from dts/acpi table, use 800MHz
as default clock.

Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 drivers/i2c/busses/i2c-thunderx-pcidrv.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
index 31f11b77ab663626967c86086a03213876bf4a07..15cf794a776533d1b0dbb08597fc0d9acf791b44 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -27,7 +27,7 @@
 
 #define PCI_DEVICE_ID_THUNDER_TWSI	0xa012
 
-#define SYS_FREQ_DEFAULT		700000000
+#define SYS_FREQ_DEFAULT		800000000
 #define OTX2_REF_FREQ_DEFAULT		100000000
 
 #define TWSI_INT_ENA_W1C		0x1028
@@ -100,7 +100,8 @@ static void thunder_i2c_clock_enable(struct device *dev, struct octeon_i2c *i2c)
 		i2c->sys_freq = clk_get_rate(i2c->clk);
 	} else {
 		/* ACPI */
-		device_property_read_u32(dev, "sclk", &i2c->sys_freq);
+		if (device_property_read_u32(dev, "sclk", &i2c->sys_freq))
+			device_property_read_u32(dev, "ioclk", &i2c->sys_freq);
 	}
 
 skip:
@@ -182,7 +183,6 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 	if (!i2c->twsi_base)
 		return -EINVAL;
 
-	thunder_i2c_clock_enable(dev, i2c);
 	ret = device_property_read_u32(dev, "clock-frequency", &i2c->twsi_freq);
 	if (ret)
 		i2c->twsi_freq = I2C_MAX_STANDARD_MODE_FREQ;
@@ -196,12 +196,12 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 
 	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
 	if (ret < 0)
-		goto error;
+		return ret;
 
 	ret = devm_request_irq(dev, pci_irq_vector(pdev, 0), octeon_i2c_isr, 0,
 			       DRV_NAME, i2c);
 	if (ret)
-		goto error;
+		return ret;
 
 	ret = octeon_i2c_init_lowlevel(i2c);
 	if (ret)
@@ -213,6 +213,9 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 	 */
 	if (octeon_i2c_is_otx2(pdev) && IS_LS_FREQ(i2c->twsi_freq))
 		i2c->sys_freq = OTX2_REF_FREQ_DEFAULT;
+	else
+		thunder_i2c_clock_enable(dev, i2c);
+
 	octeon_i2c_set_clock(i2c);
 
 	i2c->adap = thunderx_i2c_ops;
@@ -240,6 +243,8 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 
 error:
 	thunder_i2c_clock_disable(dev, i2c->clk);
+	if (!IS_LS_FREQ(i2c->twsi_freq))
+		thunder_i2c_clock_disable(dev, i2c->clk);
 	return ret;
 }
 
-- 
2.43.0

RE: [PATCH v4 1/4] i2c: thunderx: Clock divisor logic changes

[Piyush Malgujar]

Hi Andi,

We have updated these patches as per the comments. Can you
please review them.

Thanks,
Piyush

> -----Original Message-----
> From: Piyush Malgujar <pmalgujar@marvell.com>
> Sent: Friday, February 23, 2024 6:27 PM
> To: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org;
> andi.shyti@kernel.org
> Cc: Suneel Garapati <sgarapati@marvell.com>; Chandrakala Chavva
> <cchavva@marvell.com>; Jayanthi Annadurai <jannadurai@marvell.com>;
> Piyush Malgujar <pmalgujar@marvell.com>
> Subject: [PATCH v4 1/4] i2c: thunderx: Clock divisor logic changes
> 
> From: Suneel Garapati <sgarapati@marvell.com>
> 
> Handle changes to clock divisor logic for OcteonTX2 SoC family using
> subsystem ID and using default reference clock source as 100MHz.
> 
> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> Acked-by: Andi Shyti <andi.shyti@kernel.org>
> ---
>  drivers/i2c/busses/i2c-octeon-core.c     | 38 +++++++++++++++++++++---
>  drivers/i2c/busses/i2c-octeon-core.h     | 15 ++++++++++
>  drivers/i2c/busses/i2c-thunderx-pcidrv.c |  7 +++++
>  3 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-
> octeon-core.c
> index
> 845eda70b8cab52a0453c9f4cb545010fba4305d..10330ed3203f9fd99d5c04db
> af29a9bd49ad0f4a 100644
> --- a/drivers/i2c/busses/i2c-octeon-core.c
> +++ b/drivers/i2c/busses/i2c-octeon-core.c
> @@ -17,9 +17,14 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/pci.h>
> 
>  #include "i2c-octeon-core.h"
> 
> +#define INITIAL_DELTA_HZ	1000000
> +#define TWSI_MASTER_CLK_REG_DEF_VAL	0x18
> +#define TWSI_MASTER_CLK_REG_OTX2_VAL	0x3
> +
>  /* interrupt service routine */
>  irqreturn_t octeon_i2c_isr(int irq, void *dev_id)  { @@ -658,31 +663,56 @@
> int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> void octeon_i2c_set_clock(struct octeon_i2c *i2c)  {
>  	int tclk, thp_base, inc, thp_idx, mdiv_idx, ndiv_idx, foscl, diff;
> -	int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = 1000000;
> +	unsigned int mdiv_min = 2;
> +	/*
> +	 * Find divisors to produce target frequency, start with large delta
> +	 * to cover wider range of divisors, note thp = TCLK half period.
> +	 */
> +	unsigned int thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv =
> 0;
> +	unsigned int delta_hz = INITIAL_DELTA_HZ;
> +
> +	bool is_plat_otx2 = octeon_i2c_is_otx2(to_pci_dev(i2c->dev));
> +
> +	if (is_plat_otx2) {
> +		thp = TWSI_MASTER_CLK_REG_OTX2_VAL;
> +		mdiv_min = 0;
> +	}
> 
>  	for (ndiv_idx = 0; ndiv_idx < 8 && delta_hz != 0; ndiv_idx++) {
>  		/*
>  		 * An mdiv value of less than 2 seems to not work well
>  		 * with ds1337 RTCs, so we constrain it to larger values.
>  		 */
> -		for (mdiv_idx = 15; mdiv_idx >= 2 && delta_hz != 0; mdiv_idx-
> -) {
> +		for (mdiv_idx = 15; mdiv_idx >= mdiv_min && delta_hz != 0;
> +mdiv_idx--) {
>  			/*
>  			 * For given ndiv and mdiv values check the
>  			 * two closest thp values.
>  			 */
>  			tclk = i2c->twsi_freq * (mdiv_idx + 1) * 10;
>  			tclk *= (1 << ndiv_idx);
> -			thp_base = (i2c->sys_freq / (tclk * 2)) - 1;
> +			if (is_plat_otx2)
> +				thp_base = (i2c->sys_freq / tclk) - 2;
> +			else
> +				thp_base = (i2c->sys_freq / (tclk * 2)) - 1;
> 
>  			for (inc = 0; inc <= 1; inc++) {
>  				thp_idx = thp_base + inc;
>  				if (thp_idx < 5 || thp_idx > 0xff)
>  					continue;
> 
> -				foscl = i2c->sys_freq / (2 * (thp_idx + 1));
> +				if (is_plat_otx2)
> +					foscl = i2c->sys_freq / (thp_idx + 2);
> +				else
> +					foscl = i2c->sys_freq /
> +						(2 * (thp_idx + 1));
>  				foscl = foscl / (1 << ndiv_idx);
>  				foscl = foscl / (mdiv_idx + 1) / 10;
>  				diff = abs(foscl - i2c->twsi_freq);
> +				/*
> +				 * Diff holds difference between calculated
> frequency
> +				 * value vs desired frequency.
> +				 * Delta_hz is updated with last minimum diff.
> +				 */
>  				if (diff < delta_hz) {
>  					delta_hz = diff;
>  					thp = thp_idx;
> diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-
> octeon-core.h
> index
> 9bb9f64fdda0392364638ecbaafe3fab5612baf6..8a0033c94a8a291fb255b0da0
> 3858274035c46f4 100644
> --- a/drivers/i2c/busses/i2c-octeon-core.h
> +++ b/drivers/i2c/busses/i2c-octeon-core.h
> @@ -7,6 +7,7 @@
>  #include <linux/i2c-smbus.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/pci.h>
> 
>  /* Controller command patterns */
>  #define SW_TWSI_V		BIT_ULL(63)	/* Valid bit */
> @@ -211,6 +212,20 @@ static inline void octeon_i2c_write_int(struct
> octeon_i2c *i2c, u64 data)
>  	octeon_i2c_writeq_flush(data, i2c->twsi_base + TWSI_INT(i2c));  }
> 
> +#define PCI_SUBSYS_DEVID_9XXX 0xB
> +/**
> + * octeon_i2c_is_otx2 - check for chip ID
> + * @pdev: PCI dev structure
> + *
> + * Returns TRUE if OcteonTX2, FALSE otherwise.
> + */
> +static inline bool octeon_i2c_is_otx2(struct pci_dev *pdev) {
> +	u32 chip_id = (pdev->subsystem_device >> 12) & 0xF;
> +
> +	return (chip_id == PCI_SUBSYS_DEVID_9XXX); }
> +
>  /* Prototypes */
>  irqreturn_t octeon_i2c_isr(int irq, void *dev_id);  int octeon_i2c_xfer(struct
> i2c_adapter *adap, struct i2c_msg *msgs, int num); diff --git
> a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-
> pcidrv.c
> index
> a77cd86fe75ed7401bc041b27c651b9fedf67285..75569774003857dc984e8540
> ef8f4d1bb084cfb0 100644
> --- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> @@ -28,6 +28,7 @@
>  #define PCI_DEVICE_ID_THUNDER_TWSI	0xa012
> 
>  #define SYS_FREQ_DEFAULT		700000000
> +#define OTX2_REF_FREQ_DEFAULT		100000000
> 
>  #define TWSI_INT_ENA_W1C		0x1028
>  #define TWSI_INT_ENA_W1S		0x1030
> @@ -205,6 +206,12 @@ static int thunder_i2c_probe_pci(struct pci_dev
> *pdev,
>  	if (ret)
>  		goto error;
> 
> +	/*
> +	 * For OcteonTX2 chips, set reference frequency to 100MHz
> +	 * as refclk_src in TWSI_MODE register defaults to 100MHz.
> +	 */
> +	if (octeon_i2c_is_otx2(pdev))
> +		i2c->sys_freq = OTX2_REF_FREQ_DEFAULT;
>  	octeon_i2c_set_clock(i2c);
> 
>  	i2c->adap = thunderx_i2c_ops;
> --
> 2.43.0

Re: [PATCH v4 0/4] i2c: thunderx: Marvell thunderx i2c changes

[Andi Shyti]

Hi Piyush,

...

> - Removed the MAINTAINER file change from this series

I was not against adding Suneel here; on the contrary, I would
appreciate it if someone could help with reviews, maintenance,
and testing on this driver. However, I need to see a bit more
activity, especially during code reviews.

Thanks,
Andi

Re: [PATCH v4 1/4] i2c: thunderx: Clock divisor logic changes

[Andi Shyti]

Hi Piyush,

looks good, just a few little things.

...

> +#define INITIAL_DELTA_HZ	1000000
> +#define TWSI_MASTER_CLK_REG_DEF_VAL	0x18
> +#define TWSI_MASTER_CLK_REG_OTX2_VAL	0x3

nit: we can have a better alignment here

#define INITIAL_DELTA_HZ		1000000
#define TWSI_MASTER_CLK_REG_DEF_VAL	0x18
#define TWSI_MASTER_CLK_REG_OTX2_VAL	0x03

...

>  void octeon_i2c_set_clock(struct octeon_i2c *i2c)
>  {
>  	int tclk, thp_base, inc, thp_idx, mdiv_idx, ndiv_idx, foscl, diff;
> -	int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = 1000000;
> +	unsigned int mdiv_min = 2;
> +	/*
> +	 * Find divisors to produce target frequency, start with large delta
> +	 * to cover wider range of divisors, note thp = TCLK half period.
> +	 */
> +	unsigned int thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;
> +	unsigned int delta_hz = INITIAL_DELTA_HZ;
> +
> +	bool is_plat_otx2 = octeon_i2c_is_otx2(to_pci_dev(i2c->dev));

nit: please, don't leave space between variable declaration.
nit: please make important assignments not during the
     declaration, but on a different line.

...

> +/**
> + * octeon_i2c_is_otx2 - check for chip ID
> + * @pdev: PCI dev structure
> + *
> + * Returns TRUE if OcteonTX2, FALSE otherwise.

/TRUE/true/, /FALSE/false/

Can you please write it a bit better? At the end this becomes
documentation. Something like:

"Returns true if the device is an OcteonTX2, false otherwise."

> + */
> +static inline bool octeon_i2c_is_otx2(struct pci_dev *pdev)
> +{
> +	u32 chip_id = (pdev->subsystem_device >> 12) & 0xF;

You could use the bitops helpers here. I never remember which one
is the right one, if I remember correctly FIELD_GET() should be
the right one.

Andi

Re: [PATCH v4 2/4] i2c: thunderx: Add support for High speed mode

[Andi Shyti]

Hi Piyush,

On Fri, Feb 23, 2024 at 04:57:23AM -0800, Piyush Malgujar wrote:
> From: Suneel Garapati <sgarapati@marvell.com>
> 
> Support High speed mode clock setup for OcteonTX2 platforms.
> hs_mode bit in MODE register controls speed mode setup in controller

you could have called it Carl, Jim or John, but you decided to
call it hs_mode, why? Which bit? Bit 4? How setting it and
unsetting it affects the controller?

> and frequency is setup using set_clock function which sets up dividers

You mean octeon_set_clock()?

> for clock control register.

I asked you to explain better, but you just added a few words
here and there.

Please, explain what this patch really does and how does it. I do
not understand anocryms.

...

> @@ -668,7 +670,7 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
>  	 * Find divisors to produce target frequency, start with large delta
>  	 * to cover wider range of divisors, note thp = TCLK half period.
>  	 */
> -	unsigned int thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;
> +	unsigned int ds = 10, thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;

either you add a comment here or you give it a more meaningful
name than "ds".

>  	unsigned int delta_hz = INITIAL_DELTA_HZ;
>  
>  	bool is_plat_otx2 = octeon_i2c_is_otx2(to_pci_dev(i2c->dev));
> @@ -676,6 +678,8 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c)
>  	if (is_plat_otx2) {
>  		thp = TWSI_MASTER_CLK_REG_OTX2_VAL;
>  		mdiv_min = 0;
> +		if (!IS_LS_FREQ(i2c->twsi_freq))
> +			ds = 15;
>  	}

We could keep the assignments all in one place:

	if (is_plat_otx2)
		thp = ...
		mdiv_min = ...
		ds = ...
	else
		thp = ...
		mdiv_min = ...
		ds = ...

>  
>  	for (ndiv_idx = 0; ndiv_idx < 8 && delta_hz != 0; ndiv_idx++) {

...

>  #define SW_TWSI(x)	(x->roff.sw_twsi)
>  #define TWSI_INT(x)	(x->roff.twsi_int)
>  #define SW_TWSI_EXT(x)	(x->roff.sw_twsi_ext)
> +#define MODE(x)		(x->roff.mode)

A nice cleanup here is to add prefixes:

OCTEON_REG_SW_TWSI
OCTEON_REG_TWSI_INT
OCTEON_REG_SW_TWST_EXT
OCTEON_REG_MODE

MODE() is so generic. But this would be out of the scope of this
patch.

> +/* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
> +#define TWSX_MODE_HS_MASK	(BIT(4) | BIT(0))

I think it's cleaner to have different defines for bit 4 and bit
0.

Thanks,
Andi

Re: [PATCH v4 4/4] i2c: thunderx: Adding ioclk support

[Andi Shyti]

Hi Piyush,

On Fri, Feb 23, 2024 at 04:57:25AM -0800, Piyush Malgujar wrote:
> Read the ioclk property as reference clock if sclk not
> present in acpi table to make it SOC agnostic.
> In case, it's not populated from dts/acpi table, use 800MHz
> as default clock.

Why change from 700MHz to 800MHz?

> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> ---
>  drivers/i2c/busses/i2c-thunderx-pcidrv.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> index 31f11b77ab663626967c86086a03213876bf4a07..15cf794a776533d1b0dbb08597fc0d9acf791b44 100644
> --- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> @@ -27,7 +27,7 @@
>  
>  #define PCI_DEVICE_ID_THUNDER_TWSI	0xa012
>  
> -#define SYS_FREQ_DEFAULT		700000000
> +#define SYS_FREQ_DEFAULT		800000000
>  #define OTX2_REF_FREQ_DEFAULT		100000000
>  
>  #define TWSI_INT_ENA_W1C		0x1028
> @@ -100,7 +100,8 @@ static void thunder_i2c_clock_enable(struct device *dev, struct octeon_i2c *i2c)
>  		i2c->sys_freq = clk_get_rate(i2c->clk);
>  	} else {
>  		/* ACPI */
> -		device_property_read_u32(dev, "sclk", &i2c->sys_freq);
> +		if (device_property_read_u32(dev, "sclk", &i2c->sys_freq))
> +			device_property_read_u32(dev, "ioclk", &i2c->sys_freq);
>  	}
>  
>  skip:
> @@ -182,7 +183,6 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
>  	if (!i2c->twsi_base)
>  		return -EINVAL;
>  
> -	thunder_i2c_clock_enable(dev, i2c);

This change and the related goto's are not described in the
commit message. How are they related to this patch?

Andi

>  	ret = device_property_read_u32(dev, "clock-frequency", &i2c->twsi_freq);
>  	if (ret)
>  		i2c->twsi_freq = I2C_MAX_STANDARD_MODE_FREQ;
> @@ -196,12 +196,12 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
>  
>  	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
>  	if (ret < 0)
> -		goto error;
> +		return ret;
>  
>  	ret = devm_request_irq(dev, pci_irq_vector(pdev, 0), octeon_i2c_isr, 0,
>  			       DRV_NAME, i2c);
>  	if (ret)
> -		goto error;
> +		return ret;
>  
>  	ret = octeon_i2c_init_lowlevel(i2c);
>  	if (ret)
> @@ -213,6 +213,9 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
>  	 */
>  	if (octeon_i2c_is_otx2(pdev) && IS_LS_FREQ(i2c->twsi_freq))
>  		i2c->sys_freq = OTX2_REF_FREQ_DEFAULT;
> +	else
> +		thunder_i2c_clock_enable(dev, i2c);
> +
>  	octeon_i2c_set_clock(i2c);
>  
>  	i2c->adap = thunderx_i2c_ops;
> @@ -240,6 +243,8 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
>  
>  error:
>  	thunder_i2c_clock_disable(dev, i2c->clk);
> +	if (!IS_LS_FREQ(i2c->twsi_freq))
> +		thunder_i2c_clock_disable(dev, i2c->clk);
>  	return ret;
>  }
>  
> -- 
> 2.43.0
>