From: Tyrone Ting <[email protected]>
This patchset includes the following fixes:
- Add dt-bindings description for NPCM845.
- Bug fix for timeout calculation.
- Better handling of spurious interrupts.
- Fix for event type in slave mode.
- Removal of own slave addresses [2:10].
- Support for next gen BMC (NPCM845).
The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.
Addressed comments from:
- Krzysztof Kozlowski : https://lkml.org/lkml/2022/3/3/289
- Andy Shevchenko : https://lkml.org/lkml/2022/3/3/286
- Andy Shevchenko : https://lkml.org/lkml/2022/4/5/140
- Andy Shevchenko : https://lkml.org/lkml/2022/3/3/295
Changes since version 3:
- Correct the const format in if condition in i2c binding document.
- Add the oops message statement and register information in register
access width patch.
- Add the occurring rate of the i2c spurious interrupt issue and more
details in driver's behavior to overcome this issue.
- Address Andy's comments in the patch to support NPCM845.
Changes since version 2:
- Keep old code as fallback, if getting nuvoton,sys-mgr property fails.
- Fix the error reported by running 'make DT_CHECKER_FLAGS=-m
dt_binding_check'.
- Make nuvoton,sys-mgr required for nuvoton,npcm845-i2c.
- Correct the patch's subject about changing the way of getting GCR
regmap and add the description about keeping old code as fallback
if getting nuvoton,sys-mgr property fails.
- Correct the patch title and description about removing the unused
variable clk_regmap.
- Use the data field directly instead of the macros since macros are
not constants anymore in this patch.
Changes since version 1:
- Add nuvoton,sys-mgr property in NPCM devicetree.
- Describe the commit message in imperative mood.
- Modify the description in i2c binding document to cover NPCM series.
- Add new property in i2c binding document.
- Create a new patch for client address calculation.
- Create a new patch for updating gcr property name.
- Create a new patch for removing unused clock node.
- Explain EOB in the commit description.
- Create a new patch for correcting NPCM register access width.
- Remove some comment since the corresponding logic no longer exists.
- Remove fixes tag while the patch adds an additional feature.
- Use devicetree data field to support NPCM845.
Tali Perry (6):
i2c: npcm: Change the way of getting GCR regmap
i2c: npcm: Remove unused variable clk_regmap
i2c: npcm: Fix timeout calculation
i2c: npcm: Add tx complete counter
i2c: npcm: Handle spurious interrupts
i2c: npcm: Remove own slave addresses 2:10
Tyrone Ting (3):
dt-bindings: i2c: npcm: support NPCM845
i2c: npcm: Correct register access width
i2c: npcm: Support NPCM845
.../bindings/i2c/nuvoton,npcm7xx-i2c.yaml | 25 +-
drivers/i2c/busses/Kconfig | 8 +-
drivers/i2c/busses/Makefile | 2 +-
drivers/i2c/busses/i2c-npcm7xx.c | 277 +++++++++++-------
4 files changed, 196 insertions(+), 116 deletions(-)
--
2.17.1
From: Tali Perry <[email protected]>
Remove unused variable clk_regmap.
Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <[email protected]>
Reviewed-by: Jonathan Neuschäfer <[email protected]>
---
drivers/i2c/busses/i2c-npcm7xx.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index e6ae11ed2d82..1f92c84317e4 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2233,7 +2233,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
struct i2c_adapter *adap;
struct clk *i2c_clk;
static struct regmap *gcr_regmap;
- static struct regmap *clk_regmap;
int irq;
int ret;
struct device_node *np = pdev->dev.of_node;
@@ -2259,10 +2258,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
return PTR_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 PTR_ERR(clk_regmap);
-
bus->reg = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(bus->reg))
return PTR_ERR(bus->reg);
--
2.17.1
From: Tyrone Ting <[email protected]>
The SMBnCTL3 register is 8-bit wide and the 32-bit access was always
incorrect, but simply didn't cause a visible error on the 32-bit machine.
On the 64-bit machine, the kernel message reports that ESR value is
0x96000021. Checking Arm Architecture Reference Manual Armv8 suggests that
it's the alignment fault.
SMBnCTL3's address is 0xE.
Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tyrone Ting <[email protected]>
Reviewed-by: Jonathan Neuschäfer <[email protected]>
---
drivers/i2c/busses/i2c-npcm7xx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 56af571f65c1..e3c13f801f1a 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -360,14 +360,14 @@ static int npcm_i2c_get_SCL(struct i2c_adapter *_adap)
{
struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
- return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+ return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
}
static int npcm_i2c_get_SDA(struct i2c_adapter *_adap)
{
struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
- return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+ return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
}
static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
--
2.17.1
From: Tyrone Ting <[email protected]>
Add NPCM8XX I2C support.
The NPCM8XX uses a similar i2c module as NPCM7XX.
The internal HW FIFO is larger in NPCM8XX.
Signed-off-by: Tyrone Ting <[email protected]>
---
drivers/i2c/busses/Kconfig | 8 +--
drivers/i2c/busses/Makefile | 2 +-
drivers/i2c/busses/i2c-npcm7xx.c | 114 +++++++++++++++++++------------
3 files changed, 76 insertions(+), 48 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a1bae59208e3..b1d7069dd377 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -838,13 +838,13 @@ 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
+config I2C_NPCM
tristate "Nuvoton I2C Controller"
- depends on ARCH_NPCM7XX || COMPILE_TEST
+ depends on ARCH_NPCM || 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.
+ Nuvoton I2C controller, which is available on the NPCM BMC
+ controllers.
Driver can also support slave mode (select I2C_SLAVE).
config I2C_OCORES
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 479f60e4ee3d..b0a10e5d9ee9 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -84,7 +84,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_NPCM) += i2c-npcm7xx.o
obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o
obj-$(CONFIG_I2C_OMAP) += i2c-omap.o
obj-$(CONFIG_I2C_OWL) += i2c-owl.o
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 29d8a44b23c4..415a24df78e9 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -17,6 +17,7 @@
#include <linux/mfd/syscon.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
@@ -91,7 +92,6 @@ enum i2c_addr {
/* init register and default value required to enable module */
#define NPCM_I2CSEGCTL 0xE4
-#define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
/* Common regs */
#define NPCM_I2CSDA 0x00
@@ -228,8 +228,7 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
#define NPCM_I2CFIF_CTS_CLR_FIFO BIT(6)
#define NPCM_I2CFIF_CTS_SLVRSTR BIT(7)
-/* NPCM_I2CTXF_CTL reg fields */
-#define NPCM_I2CTXF_CTL_TX_THR GENMASK(4, 0)
+/* NPCM_I2CTXF_CTL reg field */
#define NPCM_I2CTXF_CTL_THR_TXIE BIT(6)
/* NPCM_I2CT_OUT reg fields */
@@ -238,22 +237,18 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
#define NPCM_I2CT_OUT_T_OUTST BIT(7)
/* NPCM_I2CTXF_STS reg fields */
-#define NPCM_I2CTXF_STS_TX_BYTES GENMASK(4, 0)
#define NPCM_I2CTXF_STS_TX_THST BIT(6)
/* NPCM_I2CRXF_STS reg fields */
-#define NPCM_I2CRXF_STS_RX_BYTES GENMASK(4, 0)
#define NPCM_I2CRXF_STS_RX_THST BIT(6)
/* NPCM_I2CFIF_CTL reg fields */
#define NPCM_I2CFIF_CTL_FIFO_EN BIT(4)
/* NPCM_I2CRXF_CTL reg fields */
-#define NPCM_I2CRXF_CTL_RX_THR GENMASK(4, 0)
-#define NPCM_I2CRXF_CTL_LAST_PEC BIT(5)
#define NPCM_I2CRXF_CTL_THR_RXIE BIT(6)
-#define I2C_HW_FIFO_SIZE 16
+#define MAX_I2C_HW_FIFO_SIZE 32
/* I2C_VER reg fields */
#define I2C_VER_VERSION GENMASK(6, 0)
@@ -270,11 +265,36 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
#define I2C_FREQ_MIN_HZ 10000
#define I2C_FREQ_MAX_HZ I2C_MAX_FAST_MODE_PLUS_FREQ
+struct npcm_i2c_data {
+ u8 fifo_size;
+ u32 segctl_init_val;
+ u8 txf_sts_tx_bytes;
+ u8 rxf_sts_rx_bytes;
+ u8 rxf_ctl_last_pec;
+};
+
+static const struct npcm_i2c_data npxm7xx_i2c_data = {
+ .fifo_size = 16,
+ .segctl_init_val = 0x0333F000,
+ .txf_sts_tx_bytes = GENMASK(4, 0),
+ .rxf_sts_rx_bytes = GENMASK(4, 0),
+ .rxf_ctl_last_pec = BIT(5),
+};
+
+static const struct npcm_i2c_data npxm8xx_i2c_data = {
+ .fifo_size = 32,
+ .segctl_init_val = 0x9333F000,
+ .txf_sts_tx_bytes = GENMASK(5, 0),
+ .rxf_sts_rx_bytes = GENMASK(5, 0),
+ .rxf_ctl_last_pec = BIT(7),
+};
+
/* Status of one I2C module */
struct npcm_i2c {
struct i2c_adapter adap;
struct device *dev;
unsigned char __iomem *reg;
+ const struct npcm_i2c_data *data;
spinlock_t lock; /* IRQ synchronization */
struct completion cmd_complete;
int cmd_err;
@@ -307,8 +327,8 @@ struct npcm_i2c {
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];
+ u8 slv_rd_buf[MAX_I2C_HW_FIFO_SIZE];
+ u8 slv_wr_buf[MAX_I2C_HW_FIFO_SIZE];
#endif
struct dentry *debugfs; /* debugfs device directory */
u64 ber_cnt;
@@ -441,7 +461,7 @@ static inline bool npcm_i2c_tx_fifo_empty(struct npcm_i2c *bus)
tx_fifo_sts = ioread8(bus->reg + NPCM_I2CTXF_STS);
/* check if TX FIFO is not empty */
- if ((tx_fifo_sts & NPCM_I2CTXF_STS_TX_BYTES) == 0)
+ if ((tx_fifo_sts & bus->data->txf_sts_tx_bytes) == 0)
return false;
/* check if TX FIFO status bit is set: */
@@ -454,7 +474,7 @@ static inline bool npcm_i2c_rx_fifo_full(struct npcm_i2c *bus)
rx_fifo_sts = ioread8(bus->reg + NPCM_I2CRXF_STS);
/* check if RX FIFO is not empty: */
- if ((rx_fifo_sts & NPCM_I2CRXF_STS_RX_BYTES) == 0)
+ if ((rx_fifo_sts & bus->data->rxf_sts_rx_bytes) == 0)
return false;
/* check if rx fifo full status is set: */
@@ -743,11 +763,11 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
static u8 npcm_i2c_fifo_usage(struct npcm_i2c *bus)
{
if (bus->operation == I2C_WRITE_OPER)
- return FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
- ioread8(bus->reg + NPCM_I2CTXF_STS));
+ return (bus->data->txf_sts_tx_bytes &
+ ioread8(bus->reg + NPCM_I2CTXF_STS));
if (bus->operation == I2C_READ_OPER)
- return FIELD_GET(NPCM_I2CRXF_STS_RX_BYTES,
- ioread8(bus->reg + NPCM_I2CRXF_STS));
+ return (bus->data->rxf_sts_rx_bytes &
+ ioread8(bus->reg + NPCM_I2CRXF_STS));
return 0;
}
@@ -759,13 +779,13 @@ static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus, u16 max_bytes)
* Fill the FIFO, while the FIFO is not full and there are more bytes
* to write
*/
- size_free_fifo = I2C_HW_FIFO_SIZE - npcm_i2c_fifo_usage(bus);
+ size_free_fifo = bus->data->fifo_size - npcm_i2c_fifo_usage(bus);
while (max_bytes-- && size_free_fifo) {
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);
- size_free_fifo = I2C_HW_FIFO_SIZE - npcm_i2c_fifo_usage(bus);
+ size_free_fifo = bus->data->fifo_size - npcm_i2c_fifo_usage(bus);
}
}
@@ -786,11 +806,11 @@ static void npcm_i2c_set_fifo(struct npcm_i2c *bus, int nread, int nwrite)
/* configure RX FIFO */
if (nread > 0) {
- rxf_ctl = min_t(int, nread, I2C_HW_FIFO_SIZE);
+ rxf_ctl = min_t(int, nread, bus->data->fifo_size);
/* set LAST bit. if LAST is set next FIFO packet is nacked */
- if (nread <= I2C_HW_FIFO_SIZE)
- rxf_ctl |= NPCM_I2CRXF_CTL_LAST_PEC;
+ if (nread <= bus->data->fifo_size)
+ rxf_ctl |= bus->data->rxf_ctl_last_pec;
/*
* if we are about to read the first byte in blk rd mode,
@@ -808,9 +828,9 @@ static void npcm_i2c_set_fifo(struct npcm_i2c *bus, int nread, int nwrite)
/* configure TX FIFO */
if (nwrite > 0) {
- if (nwrite > I2C_HW_FIFO_SIZE)
+ if (nwrite > bus->data->fifo_size)
/* data to send is more then FIFO size. */
- iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CTXF_CTL);
+ iowrite8(bus->data->fifo_size, bus->reg + NPCM_I2CTXF_CTL);
else
iowrite8(nwrite, bus->reg + NPCM_I2CTXF_CTL);
@@ -879,13 +899,13 @@ static void npcm_i2c_write_fifo_slave(struct npcm_i2c *bus, u16 max_bytes)
npcm_i2c_clear_fifo_int(bus);
npcm_i2c_clear_tx_fifo(bus);
iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
- while (max_bytes-- && I2C_HW_FIFO_SIZE != npcm_i2c_fifo_usage(bus)) {
+ while (max_bytes-- && bus->data->fifo_size != npcm_i2c_fifo_usage(bus)) {
if (bus->slv_wr_size <= 0)
break;
- bus->slv_wr_ind = bus->slv_wr_ind % I2C_HW_FIFO_SIZE;
+ bus->slv_wr_ind = bus->slv_wr_ind & (bus->data->fifo_size - 1);
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_ind = bus->slv_wr_ind & (bus->data->fifo_size - 1);
bus->slv_wr_size--;
}
}
@@ -900,7 +920,7 @@ static void npcm_i2c_read_fifo_slave(struct npcm_i2c *bus, u8 bytes_in_fifo)
while (bytes_in_fifo--) {
data = npcm_i2c_rd_byte(bus);
- bus->slv_rd_ind = bus->slv_rd_ind % I2C_HW_FIFO_SIZE;
+ bus->slv_rd_ind = bus->slv_rd_ind & (bus->data->fifo_size - 1);
bus->slv_rd_buf[bus->slv_rd_ind] = data;
bus->slv_rd_ind++;
@@ -918,8 +938,8 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
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)
+ for (i = 0; i < bus->data->fifo_size; i++) {
+ if (bus->slv_wr_size >= bus->data->fifo_size)
break;
if (bus->state == I2C_SLAVE_MATCH) {
i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
@@ -927,11 +947,11 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
} else {
i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
}
- ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
+ ind = (bus->slv_wr_ind + bus->slv_wr_size) & (bus->data->fifo_size - 1);
bus->slv_wr_buf[ind] = value;
bus->slv_wr_size++;
}
- return I2C_HW_FIFO_SIZE - ret;
+ return bus->data->fifo_size - ret;
}
static void npcm_i2c_slave_send_rd_buf(struct npcm_i2c *bus)
@@ -966,7 +986,7 @@ static void npcm_i2c_slave_receive(struct npcm_i2c *bus, u16 nread,
bus->slv_rd_ind = 0;
iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
- iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CRXF_CTL);
+ iowrite8(bus->data->fifo_size, bus->reg + NPCM_I2CRXF_CTL);
npcm_i2c_clear_tx_fifo(bus);
npcm_i2c_clear_rx_fifo(bus);
}
@@ -999,12 +1019,12 @@ static void 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));
+ left_in_fifo = bus->data->txf_sts_tx_bytes &
+ ioread8(bus->reg + NPCM_I2CTXF_STS);
/* fifo already full: */
- if (left_in_fifo >= I2C_HW_FIFO_SIZE ||
- bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
+ if (left_in_fifo >= bus->data->fifo_size ||
+ bus->slv_wr_size >= bus->data->fifo_size)
return;
/* update the wr fifo index back to the untransmitted bytes: */
@@ -1012,7 +1032,7 @@ static void npcm_i2c_slave_wr_buf_sync(struct npcm_i2c *bus)
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;
+ bus->slv_wr_ind += bus->data->fifo_size;
}
static void npcm_i2c_slave_rd_wr(struct npcm_i2c *bus)
@@ -1158,7 +1178,7 @@ static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *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);
+ iowrite8(bus->data->fifo_size, bus->reg + NPCM_I2CRXF_CTL);
if (NPCM_I2CST_XMIT & i2cst) {
bus->operation = I2C_WRITE_OPER;
} else {
@@ -1319,8 +1339,8 @@ static void npcm_i2c_master_fifo_read(struct npcm_i2c *bus)
* read == FIFO Size + C (where C < FIFO Size)then first read C bytes
* and in the next int we read rest of the data.
*/
- if (rcount < (2 * I2C_HW_FIFO_SIZE) && rcount > I2C_HW_FIFO_SIZE)
- fifo_bytes = rcount - I2C_HW_FIFO_SIZE;
+ if (rcount < (2 * bus->data->fifo_size) && rcount > bus->data->fifo_size)
+ fifo_bytes = rcount - bus->data->fifo_size;
if (rcount <= fifo_bytes) {
/* last bytes are about to be read - end of tx */
@@ -2200,7 +2220,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
* It cannot be cleared without resetting the module.
*/
else if (bus->cmd_err &&
- (NPCM_I2CRXF_CTL_LAST_PEC & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
+ (bus->data->rxf_ctl_last_pec & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
npcm_i2c_reset(bus);
/* after any xfer, successful or not, stall and EOB must be disabled */
@@ -2271,6 +2291,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
struct i2c_adapter *adap;
struct clk *i2c_clk;
static struct regmap *gcr_regmap;
+ struct device *dev = &pdev->dev;
int irq;
int ret;
struct device_node *np = pdev->dev.of_node;
@@ -2281,6 +2302,12 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
bus->dev = &pdev->dev;
+ bus->data = of_device_get_match_data(dev);
+ if (!bus->data) {
+ dev_err(dev, "OF data missing\n");
+ return -EINVAL;
+ }
+
bus->num = of_alias_get_id(pdev->dev.of_node, "i2c");
/* core clk must be acquired to calculate module timing settings */
i2c_clk = devm_clk_get(&pdev->dev, NULL);
@@ -2294,7 +2321,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
if (IS_ERR(gcr_regmap))
return PTR_ERR(gcr_regmap);
- regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
+ regmap_write(gcr_regmap, NPCM_I2CSEGCTL, bus->data->segctl_init_val);
bus->reg = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(bus->reg))
@@ -2356,7 +2383,8 @@ static int npcm_i2c_remove_bus(struct platform_device *pdev)
}
static const struct of_device_id npcm_i2c_bus_of_table[] = {
- { .compatible = "nuvoton,npcm750-i2c", },
+ { .compatible = "nuvoton,npcm750-i2c", .data = &npxm7xx_i2c_data },
+ { .compatible = "nuvoton,npcm845-i2c", .data = &npxm8xx_i2c_data },
{}
};
MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);
--
2.17.1
From: Tali Perry <[email protected]>
Change the way of getting NPCM system manager reigster (GCR)
and still maintain the old mechanism as a fallback if getting
nuvoton,sys-mgr fails while working with the legacy devicetree
file.
Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
---
drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 71aad029425d..e6ae11ed2d82 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2236,6 +2236,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
static struct regmap *clk_regmap;
int irq;
int ret;
+ struct device_node *np = pdev->dev.of_node;
bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
if (!bus)
@@ -2250,7 +2251,10 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
return PTR_ERR(i2c_clk);
bus->apb_clk = clk_get_rate(i2c_clk);
- gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
+ gcr_regmap = syscon_regmap_lookup_by_phandle(np, "nuvoton,sys-mgr");
+ if (IS_ERR(gcr_regmap))
+ gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
+
if (IS_ERR(gcr_regmap))
return PTR_ERR(gcr_regmap);
regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
--
2.17.1
From: Tali Perry <[email protected]>
On some platforms in rare cases (1 to 100,000 transactions),
the i2c gets a spurious interrupt which means that we enter an interrupt
but in the interrupt handler we don't find any status bit that points to
the reason we got this interrupt.
This may be a case of a rare HW issue or signal integrity issue that is
still under investigation.
In order to overcome this we are doing the following:
1. Disable incoming interrupts in master mode only when slave mode is not
enabled.
2. Clear end of busy (EOB) after every interrupt.
3. Clear other status bits (just in case since we found them cleared)
4. Return correct status during the interrupt that will finish the
transaction.
On next xmit transaction if the bus is still busy the master will issue a
recovery process before issuing the new transaction.
Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <[email protected]>
---
drivers/i2c/busses/i2c-npcm7xx.c | 92 ++++++++++++++++++++++----------
1 file changed, 63 insertions(+), 29 deletions(-)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index e3c13f801f1a..82e600c24831 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -564,6 +564,15 @@ static inline void npcm_i2c_nack(struct npcm_i2c *bus)
iowrite8(val, bus->reg + NPCM_I2CCTL1);
}
+static inline void npcm_i2c_clear_master_status(struct npcm_i2c *bus)
+{
+ u8 val;
+
+ /* Clear NEGACK, STASTR and BER bits */
+ val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | NPCM_I2CST_STASTR;
+ iowrite8(val, bus->reg + NPCM_I2CST);
+}
+
#if IS_ENABLED(CONFIG_I2C_SLAVE)
static void npcm_i2c_slave_int_enable(struct npcm_i2c *bus, bool enable)
{
@@ -643,8 +652,8 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)
iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
iowrite8(0xFF, bus->reg + NPCM_I2CST);
- /* Clear EOB bit */
- iowrite8(NPCM_I2CCST3_EO_BUSY, bus->reg + NPCM_I2CCST3);
+ /* Clear and disable EOB */
+ npcm_i2c_eob_int(bus, false);
/* Clear all fifo bits: */
iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO, bus->reg + NPCM_I2CFIF_CTS);
@@ -656,6 +665,9 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)
}
#endif
+ /* clear status bits for spurious interrupts */
+ npcm_i2c_clear_master_status(bus);
+
bus->state = I2C_IDLE;
}
@@ -818,15 +830,6 @@ static void npcm_i2c_read_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo)
}
}
-static inline void npcm_i2c_clear_master_status(struct npcm_i2c *bus)
-{
- u8 val;
-
- /* Clear NEGACK, STASTR and BER bits */
- val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | NPCM_I2CST_STASTR;
- iowrite8(val, bus->reg + NPCM_I2CST);
-}
-
static void npcm_i2c_master_abort(struct npcm_i2c *bus)
{
/* Only current master is allowed to issue a stop condition */
@@ -1234,7 +1237,16 @@ static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus)
ret = IRQ_HANDLED;
} /* SDAST */
- return ret;
+ /*
+ * if irq is not one of the above, make sure EOB is disabled and all
+ * status bits are cleared.
+ */
+ if (ret == IRQ_NONE) {
+ npcm_i2c_eob_int(bus, false);
+ npcm_i2c_clear_master_status(bus);
+ }
+
+ return IRQ_HANDLED;
}
static int npcm_i2c_reg_slave(struct i2c_client *client)
@@ -1470,6 +1482,9 @@ static void npcm_i2c_irq_handle_nack(struct npcm_i2c *bus)
npcm_i2c_eob_int(bus, false);
npcm_i2c_master_stop(bus);
+ /* Clear SDA Status bit (by reading dummy byte) */
+ npcm_i2c_rd_byte(bus);
+
/*
* The bus is released from stall only after the SW clears
* NEGACK bit. Then a Stop condition is sent.
@@ -1477,6 +1492,8 @@ static void npcm_i2c_irq_handle_nack(struct npcm_i2c *bus)
npcm_i2c_clear_master_status(bus);
readx_poll_timeout_atomic(ioread8, bus->reg + NPCM_I2CCST, val,
!(val & NPCM_I2CCST_BUSY), 10, 200);
+ /* verify no status bits are still set after bus is released */
+ npcm_i2c_clear_master_status(bus);
}
bus->state = I2C_IDLE;
@@ -1675,10 +1692,10 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
int iter = 27;
if ((npcm_i2c_get_SDA(_adap) == 1) && (npcm_i2c_get_SCL(_adap) == 1)) {
- dev_dbg(bus->dev, "bus%d recovery skipped, bus not stuck",
- bus->num);
+ dev_dbg(bus->dev, "bus%d-0x%x recovery skipped, bus not stuck",
+ bus->num, bus->dest_addr);
npcm_i2c_reset(bus);
- return status;
+ return 0;
}
npcm_i2c_int_enable(bus, false);
@@ -1912,6 +1929,7 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode,
bus_freq_hz < I2C_FREQ_MIN_HZ || bus_freq_hz > I2C_FREQ_MAX_HZ)
return -EINVAL;
+ npcm_i2c_int_enable(bus, false);
npcm_i2c_disable(bus);
/* Configure FIFO mode : */
@@ -1940,10 +1958,18 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode,
val = (val | NPCM_I2CCTL1_NMINTE) & ~NPCM_I2CCTL1_RWS;
iowrite8(val, bus->reg + NPCM_I2CCTL1);
- npcm_i2c_int_enable(bus, true);
-
npcm_i2c_reset(bus);
+ /* check HW is OK: SDA and SCL should be high at this point. */
+ if ((npcm_i2c_get_SDA(&bus->adap) == 0) ||
+ (npcm_i2c_get_SCL(&bus->adap) == 0)) {
+ dev_err(bus->dev, "I2C%d init fail: lines are low", bus->num);
+ dev_err(bus->dev, "SDA=%d SCL=%d", npcm_i2c_get_SDA(&bus->adap),
+ npcm_i2c_get_SCL(&bus->adap));
+ return -ENXIO;
+ }
+
+ npcm_i2c_int_enable(bus, true);
return 0;
}
@@ -1991,10 +2017,14 @@ static irqreturn_t npcm_i2c_bus_irq(int irq, void *dev_id)
#if IS_ENABLED(CONFIG_I2C_SLAVE)
if (bus->slave) {
bus->master_or_slave = I2C_SLAVE;
- return npcm_i2c_int_slave_handler(bus);
+ if (npcm_i2c_int_slave_handler(bus))
+ return IRQ_HANDLED;
}
#endif
- return IRQ_NONE;
+ /* clear status bits for spurious interrupts */
+ npcm_i2c_clear_master_status(bus);
+
+ return IRQ_HANDLED;
}
static bool npcm_i2c_master_start_xmit(struct npcm_i2c *bus,
@@ -2051,7 +2081,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
u8 *write_data, *read_data;
u8 slave_addr;
unsigned long timeout;
- int ret = 0;
bool read_block = false;
bool read_PEC = false;
u8 bus_busy;
@@ -2141,12 +2170,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
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) {
+ npcm_i2c_int_enable(bus, true);
+
+ if (npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
+ write_data, read_data, read_PEC,
+ read_block)) {
time_left = wait_for_completion_timeout(&bus->cmd_complete,
timeout);
@@ -2160,26 +2189,31 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
}
}
}
- 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);
+ bus->cmd_err = i2c_recover_bus(adap);
/*
* After any type of error, check if LAST bit is still set,
* due to a HW issue.
* It cannot be cleared without resetting the module.
*/
- if (bus->cmd_err &&
- (NPCM_I2CRXF_CTL_LAST_PEC & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
+ else if (bus->cmd_err &&
+ (NPCM_I2CRXF_CTL_LAST_PEC & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
npcm_i2c_reset(bus);
+ /* after any xfer, successful or not, stall and EOB must be disabled */
+ npcm_i2c_stall_after_start(bus, false);
+ npcm_i2c_eob_int(bus, false);
+
#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);
+#else
+ npcm_i2c_int_enable(bus, false);
#endif
return bus->cmd_err;
}
--
2.17.1
On Tue, May 10, 2022 at 05:16:47PM +0800, Tyrone Ting wrote:
> From: Tali Perry <[email protected]>
>
> Change the way of getting NPCM system manager reigster (GCR)
> and still maintain the old mechanism as a fallback if getting
> nuvoton,sys-mgr fails while working with the legacy devicetree
> file.
...
> @@ -2236,6 +2236,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
> static struct regmap *clk_regmap;
> int irq;
> int ret;
> + struct device_node *np = pdev->dev.of_node;
Can we keep "longer line first" order?
--
With Best Regards,
Andy Shevchenko
From: Tali Perry <[email protected]>
tx_complete counter is used to indicate successful transaction
count.
Similar counters for failed tx were previously added.
Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <[email protected]>
---
drivers/i2c/busses/i2c-npcm7xx.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index eaec051e080d..56af571f65c1 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -314,6 +314,7 @@ struct npcm_i2c {
u64 rec_fail_cnt;
u64 nack_cnt;
u64 timeout_cnt;
+ u64 tx_complete_cnt;
};
static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
@@ -684,6 +685,8 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
switch (op_status) {
case I2C_MASTER_DONE_IND:
bus->cmd_err = bus->msgs_num;
+ if (bus->tx_complete_cnt < ULLONG_MAX)
+ bus->tx_complete_cnt++;
fallthrough;
case I2C_BLOCK_BYTES_ERR_IND:
/* Master tx finished and all transmit bytes were sent */
@@ -2223,6 +2226,7 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
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);
+ debugfs_create_u64("tx_complete_cnt", 0444, d, &bus->tx_complete_cnt);
bus->debugfs = d;
}
--
2.17.1
From: Tali Perry <[email protected]>
NPCM can support up to 10 own slave addresses.
In practice, only one address is actually being used.
In order to access addresses 2 and above, need to switch
register banks. The switch needs spinlock.
To avoid using spinlock for this useless feature
removed support of SA >= 2.
Also fix returned slave event enum.
Remove some comment since the bank selection is not
required. The bank selection is not required since
the supported slave addresses are reduced.
Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <[email protected]>
---
drivers/i2c/busses/i2c-npcm7xx.c | 46 ++++++++++++++++----------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 82e600c24831..29d8a44b23c4 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -124,6 +124,8 @@ enum i2c_addr {
* use this array to get the address or each register.
*/
#define I2C_NUM_OWN_ADDR 10
+#define I2C_NUM_OWN_ADDR_SUPPORTED 2
+
static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,
@@ -392,14 +394,10 @@ static void npcm_i2c_disable(struct npcm_i2c *bus)
#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_OWN_ADDR; i++)
+ for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR_SUPPORTED; 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);
@@ -604,8 +602,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
return 0;
- }
- if (addr_type == I2C_ARP_ADDR) {
+ } else if (addr_type == I2C_ARP_ADDR) {
i2cctl3 = ioread8(bus->reg + NPCM_I2CCTL3);
if (enable)
i2cctl3 |= I2CCTL3_ARPMEN;
@@ -614,16 +611,18 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
iowrite8(i2cctl3, bus->reg + NPCM_I2CCTL3);
return 0;
}
+ if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
+ dev_err(bus->dev,
+ "try to enable more then 2 SA not supported\n");
+ }
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[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
@@ -846,15 +845,13 @@ 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);
+ if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
+ dev_err(bus->dev,
+ "get slave: try to use more then 2 slave addresses not supported\n");
+ }
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;
}
@@ -864,12 +861,12 @@ static int npcm_i2c_remove_slave_addr(struct npcm_i2c *bus, u8 slave_add)
/* Set the enable bit */
slave_add |= 0x80;
- npcm_i2c_select_bank(bus, I2C_BANK_0);
- for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR; i++) {
+
+ for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR_SUPPORTED; 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;
}
@@ -924,11 +921,15 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
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);
+ if (bus->state == I2C_SLAVE_MATCH) {
+ i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+ bus->state = I2C_OPER_STARTED;
+ } else {
+ i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
+ }
ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
bus->slv_wr_buf[ind] = value;
bus->slv_wr_size++;
- i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
}
return I2C_HW_FIFO_SIZE - ret;
}
@@ -976,7 +977,6 @@ static void npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
if (nwrite == 0)
return;
- bus->state = I2C_OPER_STARTED;
bus->operation = I2C_WRITE_OPER;
/* get the next buffer */
--
2.17.1
From: Tyrone Ting <[email protected]>
Add compatible and nuvoton,sys-mgr description for NPCM i2c module.
Signed-off-by: Tyrone Ting <[email protected]>
---
.../bindings/i2c/nuvoton,npcm7xx-i2c.yaml | 25 +++++++++++++++----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
index 128444942aec..99fae8489771 100644
--- a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
@@ -7,17 +7,18 @@ $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.
+ I2C bus controllers of the NPCM series support both master and
+ slave mode. Each controller can switch between master and slave at run time
+ (i.e. IPMB mode). HW FIFO for TX and RX are supported.
maintainers:
- Tali Perry <[email protected]>
properties:
compatible:
- const: nuvoton,npcm750-i2c
+ enum:
+ - nuvoton,npcm750-i2c
+ - nuvoton,npcm845-i2c
reg:
maxItems: 1
@@ -36,6 +37,10 @@ properties:
default: 100000
enum: [100000, 400000, 1000000]
+ nuvoton,sys-mgr:
+ $ref: "/schemas/types.yaml#/definitions/phandle"
+ description: The phandle of system manager register node.
+
required:
- compatible
- reg
@@ -44,6 +49,15 @@ required:
allOf:
- $ref: /schemas/i2c/i2c-controller.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: nuvoton,npcm845-i2c
+
+ then:
+ required:
+ - nuvoton,sys-mgr
unevaluatedProperties: false
@@ -57,6 +71,7 @@ examples:
clock-frequency = <100000>;
interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
compatible = "nuvoton,npcm750-i2c";
+ nuvoton,sys-mgr = <&gcr>;
};
...
--
2.17.1
On Tue, May 10, 2022 at 05:16:52PM +0800, Tyrone Ting wrote:
> From: Tali Perry <[email protected]>
>
> On some platforms in rare cases (1 to 100,000 transactions),
> the i2c gets a spurious interrupt which means that we enter an interrupt
> but in the interrupt handler we don't find any status bit that points to
> the reason we got this interrupt.
>
> This may be a case of a rare HW issue or signal integrity issue that is
> still under investigation.
>
> In order to overcome this we are doing the following:
> 1. Disable incoming interrupts in master mode only when slave mode is not
> enabled.
> 2. Clear end of busy (EOB) after every interrupt.
> 3. Clear other status bits (just in case since we found them cleared)
> 4. Return correct status during the interrupt that will finish the
> transaction.
>
> On next xmit transaction if the bus is still busy the master will issue a
> recovery process before issuing the new transaction.
...
> + /* clear status bits for spurious interrupts */
Clear
...
> + /*
> + * if irq is not one of the above, make sure EOB is disabled and all
If
> + * status bits are cleared.
> + */
...
> + /* verify no status bits are still set after bus is released */
Verify
...
> + /* check HW is OK: SDA and SCL should be high at this point. */
Check
...
> + if ((npcm_i2c_get_SDA(&bus->adap) == 0) ||
> + (npcm_i2c_get_SCL(&bus->adap) == 0)) {
This fits one line
> + dev_err(bus->dev, "I2C%d init fail: lines are low", bus->num);
> + dev_err(bus->dev, "SDA=%d SCL=%d", npcm_i2c_get_SDA(&bus->adap),
> + npcm_i2c_get_SCL(&bus->adap));
No '\n' at the end of each?!
> + return -ENXIO;
> + }
...
> + /* clear status bits for spurious interrupts */
Clear
...
> + /* after any xfer, successful or not, stall and EOB must be disabled */
After
...
Maybe you chose the small letter for one-liner comments, but I see even in the
original code the inconsistent style. Please, add an explanation to the cover
letter and follow it, assuming you add the patch at the end of the series that
makes comment style consistent (for the one-liners, for the multi-line comments
we have a clear understanding about the style).
--
With Best Regards,
Andy Shevchenko
On Tue, May 10, 2022 at 05:16:53PM +0800, Tyrone Ting wrote:
> From: Tali Perry <[email protected]>
>
> NPCM can support up to 10 own slave addresses.
> In practice, only one address is actually being used.
> In order to access addresses 2 and above, need to switch
> register banks. The switch needs spinlock.
> To avoid using spinlock for this useless feature
> removed support of SA >= 2.
> Also fix returned slave event enum.
>
> Remove some comment since the bank selection is not
> required. The bank selection is not required since
> the supported slave addresses are reduced.
Fancy indentation. Please fix it in all your commit messages where it applies.
...
> + if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
> + dev_err(bus->dev,
> + "try to enable more then 2 SA not supported\n");
Make it one line and drop {}.
> + }
...
> + if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
> + dev_err(bus->dev,
> + "get slave: try to use more then 2 slave addresses not supported\n");
As per above be consistent with abbreviations ("SA" here, which makes line
shorter) and follow the above recommendation.
> + }
--
With Best Regards,
Andy Shevchenko
From: Tali Perry <[email protected]>
Use adap.timeout for timeout calculation instead of hard-coded
value of 35ms.
Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
drivers/i2c/busses/i2c-npcm7xx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 1f92c84317e4..eaec051e080d 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2047,7 +2047,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
u16 nwrite, nread;
u8 *write_data, *read_data;
u8 slave_addr;
- int timeout;
+ unsigned long timeout;
int ret = 0;
bool read_block = false;
bool read_PEC = false;
@@ -2099,13 +2099,13 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
* 9: bits per transaction (including the ack/nack)
*/
timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
- timeout = max(msecs_to_jiffies(35), usecs_to_jiffies(timeout_usec));
+ timeout = max_t(unsigned long, bus->adap.timeout, 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;
}
- time_left = jiffies + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
+ time_left = jiffies + timeout + 1;
do {
/*
* we must clear slave address immediately when the bus is not
@@ -2268,7 +2268,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
adap = &bus->adap;
adap->owner = THIS_MODULE;
adap->retries = 3;
- adap->timeout = HZ;
+ adap->timeout = msecs_to_jiffies(35);
adap->algo = &npcm_i2c_algo;
adap->quirks = &npcm_i2c_quirks;
adap->algo_data = bus;
--
2.17.1
Hi Andy:
Thank you for your comment and it will be addressed.
Andy Shevchenko <[email protected]> 於 2022年5月10日 週二 下午6:08寫道：
>
> On Tue, May 10, 2022 at 05:16:47PM +0800, Tyrone Ting wrote:
> > From: Tali Perry <[email protected]>
> >
> > Change the way of getting NPCM system manager reigster (GCR)
> > and still maintain the old mechanism as a fallback if getting
> > nuvoton,sys-mgr fails while working with the legacy devicetree
> > file.
>
> ...
>
> > @@ -2236,6 +2236,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
> > static struct regmap *clk_regmap;
> > int irq;
> > int ret;
> > + struct device_node *np = pdev->dev.of_node;
>
> Can we keep "longer line first" order?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best Regards,
Tyrone
Hi Andy:
Thank you for your comments and they will be addressed.
Andy Shevchenko <[email protected]> 於 2022年5月10日 週二 下午6:15寫道：
>
> On Tue, May 10, 2022 at 05:16:52PM +0800, Tyrone Ting wrote:
> > From: Tali Perry <[email protected]>
> >
> > On some platforms in rare cases (1 to 100,000 transactions),
> > the i2c gets a spurious interrupt which means that we enter an interrupt
> > but in the interrupt handler we don't find any status bit that points to
> > the reason we got this interrupt.
> >
> > This may be a case of a rare HW issue or signal integrity issue that is
> > still under investigation.
> >
> > In order to overcome this we are doing the following:
> > 1. Disable incoming interrupts in master mode only when slave mode is not
> > enabled.
> > 2. Clear end of busy (EOB) after every interrupt.
> > 3. Clear other status bits (just in case since we found them cleared)
> > 4. Return correct status during the interrupt that will finish the
> > transaction.
> >
> > On next xmit transaction if the bus is still busy the master will issue a
> > recovery process before issuing the new transaction.
>
> ...
>
> > + /* clear status bits for spurious interrupts */
>
> Clear
>
> ...
>
> > + /*
> > + * if irq is not one of the above, make sure EOB is disabled and all
>
> If
>
> > + * status bits are cleared.
> > + */
>
> ...
>
> > + /* verify no status bits are still set after bus is released */
>
> Verify
>
> ...
>
> > + /* check HW is OK: SDA and SCL should be high at this point. */
>
> Check
>
> ...
>
> > + if ((npcm_i2c_get_SDA(&bus->adap) == 0) ||
> > + (npcm_i2c_get_SCL(&bus->adap) == 0)) {
>
> This fits one line
>
> > + dev_err(bus->dev, "I2C%d init fail: lines are low", bus->num);
> > + dev_err(bus->dev, "SDA=%d SCL=%d", npcm_i2c_get_SDA(&bus->adap),
> > + npcm_i2c_get_SCL(&bus->adap));
>
> No '\n' at the end of each?!
>
> > + return -ENXIO;
> > + }
>
> ...
>
> > + /* clear status bits for spurious interrupts */
>
> Clear
>
> ...
>
> > + /* after any xfer, successful or not, stall and EOB must be disabled */
>
> After
>
> ...
>
> Maybe you chose the small letter for one-liner comments, but I see even in the
> original code the inconsistent style. Please, add an explanation to the cover
> letter and follow it, assuming you add the patch at the end of the series that
> makes comment style consistent (for the one-liners, for the multi-line comments
> we have a clear understanding about the style).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best Regards,
Tyrone
Hi Andy:
Thank you for your comments and they will be addressed.
Andy Shevchenko <[email protected]> 於 2022年5月10日 週二 下午6:19寫道：
>
> On Tue, May 10, 2022 at 05:16:53PM +0800, Tyrone Ting wrote:
> > From: Tali Perry <[email protected]>
> >
> > NPCM can support up to 10 own slave addresses.
> > In practice, only one address is actually being used.
> > In order to access addresses 2 and above, need to switch
> > register banks. The switch needs spinlock.
> > To avoid using spinlock for this useless feature
> > removed support of SA >= 2.
>
> > Also fix returned slave event enum.
> >
> > Remove some comment since the bank selection is not
> > required. The bank selection is not required since
> > the supported slave addresses are reduced.
>
> Fancy indentation. Please fix it in all your commit messages where it applies.
>
> ...
>
> > + if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
> > + dev_err(bus->dev,
> > + "try to enable more then 2 SA not supported\n");
>
> Make it one line and drop {}.
>
> > + }
>
> ...
>
> > + if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
> > + dev_err(bus->dev,
> > + "get slave: try to use more then 2 slave addresses not supported\n");
>
> As per above be consistent with abbreviations ("SA" here, which makes line
> shorter) and follow the above recommendation.
>
> > + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best Regards,
Tyrone
Tyrone Ting <[email protected]> 於 2022年5月11日 週三 上午8:25寫道：
>
> Hi Krzysztof:
>
> Thank you for your review and it will be addressed.
>
> Krzysztof Kozlowski <[email protected]> 於 2022年5月10日 週二 下午6:59寫道：
> >
> > On 10/05/2022 12:03, Tyrone Ting wrote:
> > >
> > > maintainers:
> > > - Tali Perry <[email protected]>
> > >
> > > properties:
> > > compatible:
> > > - const: nuvoton,npcm750-i2c
> > > + enum:
> > > + - nuvoton,npcm750-i2c
> > > + - nuvoton,npcm845-i2c
> > >
> > > reg:
> > > maxItems: 1
> > > @@ -36,6 +37,10 @@ properties:
> > > default: 100000
> > > enum: [100000, 400000, 1000000]
> > >
> > > + nuvoton,sys-mgr:
> > > + $ref: "/schemas/types.yaml#/definitions/phandle"
> >
> > Minor nit: No quotes. The other places don't have it.
> >
> >
> > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> >
> >
> > Best regards,
> > Krzysztof
>
> Best regards,
> Tyrone
On Tue, 10 May 2022 17:16:46 +0800, Tyrone Ting wrote:
> From: Tyrone Ting <[email protected]>
>
> Add compatible and nuvoton,sys-mgr description for NPCM i2c module.
>
> Signed-off-by: Tyrone Ting <[email protected]>
> ---
> .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml | 25 +++++++++++++++----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>
Hi Rob:
Thank you for your review. I would like to apply Krzysztof's comment
from the link https://www.spinics.net/lists/linux-i2c/msg56795.html
and remove the
quotes of the statement "$ref: "/schemas/types.yaml#/definitions/phandle"".
If it's okay with you, I'll come up with a new patch with
"Reviewed-by" from you and Krzysztof.
Thank you.
Rob Herring <[email protected]> 於 2022年5月11日 週三 下午11:24寫道：
>
> On Tue, 10 May 2022 17:16:46 +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <[email protected]>
> >
> > Add compatible and nuvoton,sys-mgr description for NPCM i2c module.
> >
> > Signed-off-by: Tyrone Ting <[email protected]>
> > ---
> > .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml | 25 +++++++++++++++----
> > 1 file changed, 20 insertions(+), 5 deletions(-)
> >
>
> Reviewed-by: Rob Herring <[email protected]>
Best Regards,
Tyrone
On 12/05/2022 03:31, Tyrone Ting wrote:
> Hi Rob:
>
> Thank you for your review. I would like to apply Krzysztof's comment
> from the link https://www.spinics.net/lists/linux-i2c/msg56795.html
> and remove the
> quotes of the statement "$ref: "/schemas/types.yaml#/definitions/phandle"".
>
> If it's okay with you, I'll come up with a new patch with
> "Reviewed-by" from you and Krzysztof.
Go ahead and keep the review-tags in new patch.
Best regards,
Krzysztof
Hi Andy:
I would like to have your comments about the "Fancy indentation"
because I'm not sure if I get the point.
I remove extra empty lines and reformat the commit message.
Please see the details below.
Thank you.
Tyrone Ting <[email protected]> 於 2022年5月11日 週三 上午9:39寫道：
>
> Hi Andy:
>
> Thank you for your comments and they will be addressed.
>
> Andy Shevchenko <[email protected]> 於 2022年5月10日 週二 下午6:19寫道：
> >
> > On Tue, May 10, 2022 at 05:16:53PM +0800, Tyrone Ting wrote:
> > > From: Tali Perry <[email protected]>
> > >
> > > NPCM can support up to 10 own slave addresses.
> > > In practice, only one address is actually being used.
> > > In order to access addresses 2 and above, need to switch
> > > register banks. The switch needs spinlock.
> > > To avoid using spinlock for this useless feature
> > > removed support of SA >= 2.
> >
> > > Also fix returned slave event enum.
> > >
> > > Remove some comment since the bank selection is not
> > > required. The bank selection is not required since
> > > the supported slave addresses are reduced.
> >
> > Fancy indentation. Please fix it in all your commit messages where it applies.
> >
I modify the commit message as following:
NPCM can support up to 10 own slave addresses. In practice, only one
address is actually being used. In order to access addresses 2 and above,
need to switch register banks. The switch needs spinlock.
To avoid using spinlock for this useless feature removed support of SA >=
2. Also fix returned slave event enum.
Remove some comment since the bank selection is not required. The bank
selection is not required since the supported slave addresses are reduced.
> > ...
> >
> > > + if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
> > > + dev_err(bus->dev,
> > > + "try to enable more then 2 SA not supported\n");
> >
> > Make it one line and drop {}.
> >
> > > + }
> >
> > ...
> >
> > > + if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
> > > + dev_err(bus->dev,
> > > + "get slave: try to use more then 2 slave addresses not supported\n");
> >
> > As per above be consistent with abbreviations ("SA" here, which makes line
> > shorter) and follow the above recommendation.
> >
> > > + }
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
>
> Best Regards,
> Tyrone
Best Regards,
Tyrone
On Tue, May 10, 2022 at 05:16:47PM +0800, Tyrone Ting wrote:
> From: Tali Perry <[email protected]>
>
> Change the way of getting NPCM system manager reigster (GCR)
> and still maintain the old mechanism as a fallback if getting
> nuvoton,sys-mgr fails while working with the legacy devicetree
> file.
>
> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
> Signed-off-by: Tali Perry <[email protected]>
> Acked-by: Krzysztof Kozlowski <[email protected]>
Tyrone, your SoB is missing for all patches from Tali.
Hi Wolfram:
Got it and thank you for your help.
I'll come up with the new patch set with the corrected SoBs.
Wolfram Sang <[email protected]> 於 2022年5月17日 週二 下午2:04寫道：
>
>
> > After reviewing these two links, we decided to keep only the author's
> > SoB for each commit.
> >
> > https://lore.kernel.org/lkml/YiCZlhJoXPLpQ6%[email protected]/
> > https://lore.kernel.org/lkml/[email protected]/
>
> When Andy said "chain is wrong", he meant the order. You must add your
> SoB, otherwise I can't take the patches. But yours should be last. The
> SoBs need to be in the order people worked on it.
>
> Does that make sense to you?
>
Best Regards,
Tyrone
Hi Wolfram:
Thank you for your feedback and here is my reply below.
Wolfram Sang <[email protected]> 於 2022年5月17日 週二 上午3:48寫道：
>
> On Tue, May 10, 2022 at 05:16:47PM +0800, Tyrone Ting wrote:
> > From: Tali Perry <[email protected]>
> >
> > Change the way of getting NPCM system manager reigster (GCR)
> > and still maintain the old mechanism as a fallback if getting
> > nuvoton,sys-mgr fails while working with the legacy devicetree
> > file.
> >
> > Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
> > Signed-off-by: Tali Perry <[email protected]>
> > Acked-by: Krzysztof Kozlowski <[email protected]>
>
> Tyrone, your SoB is missing for all patches from Tali.
>
After reviewing these two links, we decided to keep only the author's
SoB for each commit.
https://lore.kernel.org/lkml/YiCZlhJoXPLpQ6%[email protected]/
https://lore.kernel.org/lkml/[email protected]/
Best Regards,
Tyrone
> After reviewing these two links, we decided to keep only the author's
> SoB for each commit.
>
> https://lore.kernel.org/lkml/YiCZlhJoXPLpQ6%[email protected]/
> https://lore.kernel.org/lkml/[email protected]/
When Andy said "chain is wrong", he meant the order. You must add your
SoB, otherwise I can't take the patches. But yours should be last. The
SoBs need to be in the order people worked on it.
Does that make sense to you?