[PATCH v5 0/6] i2c: npcm: read/write operation, checkpatch

[PATCH v5 0/6] i2c: npcm: read/write operation, checkpatch

[Tyrone Ting]

This patchset includes the following fixes:

- Enable the target functionality in the interrupt handling routine 
  when the i2c transfer is about to finish.
- Correct the read/write operation procedure.
- Introduce a software flag to handle the bus error (BER) condition
  which is not caused by the i2c transfer.
- Modify timeout calculation.
- Assign the client address earlier logically.
- Use an i2c frequency table for the frequency parameters assignment.
- Coding style fix.

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Addressed comments from:
- Andi Shyti : https://lore.kernel.org/lkml/5mxsciw6443k5rlpymrs7addvme
  53f5v3zuj5u7tvlggfeirik@dy2bvyz2lyue/
- Andi Shyti : https://lore.kernel.org/lkml/z4g5alkk3cug7v5bsmrmzspgvo4
  hhu2ebtykht72ewwhsqxqgq@kg2tlpvz3ctp/
- Andy Shevchenko : https://lore.kernel.org/lkml/Zu2HmkagbpMf_CNE@smile
  .fi.intel.com/

Changes since version 4:
- Add more description for the codes.
- Modify the term "function" to "function()" in the commit message
and code comments.

Changes since version 3:
- Remove "Bug fixes" in the cover letter title.
- Modify the term "function" to "func()" in the commit message and
  code comments.
- Correct the coding style.

Changes since version 2:
- Add more explanations in the commit message and code modification.
- Use lower character names for declarations.
- Remove Fixes tags in commits which are not to fix bugs.

Changes since version 1:
- Restore the npcm_i2caddr array length to fix the smatch warning.
- Remove unused variables.
- Handle the condition where scl_table_cnt reaches to the maximum value.
- Fix the checkpatch warning.

Charles Boyer (1):
  i2c: npcm: Enable slave in eob interrupt

Tyrone Ting (5):
  i2c: npcm: correct the read/write operation procedure
  i2c: npcm: use a software flag to indicate a BER condition
  i2c: npcm: Modify timeout evaluation mechanism
  i2c: npcm: Modify the client address assignment
  i2c: npcm: use i2c frequency table

 drivers/i2c/busses/i2c-npcm7xx.c | 440 ++++++++++++++++++++++++-------
 1 file changed, 338 insertions(+), 102 deletions(-)


base-commit: f56f4ba2fc1dbefd3242946f2fad35338a60e3bc
-- 
2.34.1

[PATCH v5 1/6] i2c: npcm: correct the read/write operation procedure

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

Originally the driver uses the XMIT bit in SMBnST register to decide
the upcoming i2c transaction. If XMIT bit is 1, then it will be an i2c
write operation. If it's 0, then a read operation will be executed.

In slave mode the XMIT bit can simply be used directly to set the state.
XMIT bit can be used as an indication to the current state of the state
machine during slave operation. (meaning XMIT = 1 during writing and
XMIT = 0 during reading).

In master operation XMIT is valid only if there are no bus errors.
For example: in a multi master where the same module is switching from
master to slave at runtime, and there are collisions, the XMIT bit
cannot be trusted.

However the maser already "knows" what the bus state is, so this bit
is not needed and the driver can just track what it is currently doing.

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index bbcb4d6668ce..2b76dbfba438 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -1628,13 +1628,10 @@ static void npcm_i2c_irq_handle_sda(struct npcm_i2c *bus, u8 i2cst)
 			npcm_i2c_wr_byte(bus, bus->dest_addr | BIT(0));
 	/* SDA interrupt, after start\restart */
 	} else {
-		if (NPCM_I2CST_XMIT & i2cst) {
-			bus->operation = I2C_WRITE_OPER;
+		if (bus->operation == I2C_WRITE_OPER)
 			npcm_i2c_irq_master_handler_write(bus);
-		} else {
-			bus->operation = I2C_READ_OPER;
+		else if (bus->operation == I2C_READ_OPER)
 			npcm_i2c_irq_master_handler_read(bus);
-		}
 	}
 }
 
-- 
2.34.1

[PATCH v5 2/6] i2c: npcm: use a software flag to indicate a BER condition

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

If not clearing the BB (bus busy) condition in the BER (bus error)
interrupt, the driver causes a timeout and hence the i2c core
doesn't do the i2c transfer retry but returns the driver's return
value to the upper layer instead.

Clear the BB condition in the BER interrupt and a software flag is
used. The driver does an i2c recovery without causing the timeout
if the flag is set.

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2b76dbfba438..7620bdcdc235 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -334,6 +334,7 @@ struct npcm_i2c {
 	u64 nack_cnt;
 	u64 timeout_cnt;
 	u64 tx_complete_cnt;
+	bool ber_state; /* Indicate the bus error state */
 };
 
 static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
@@ -1521,6 +1522,7 @@ static void npcm_i2c_irq_handle_ber(struct npcm_i2c *bus)
 	if (npcm_i2c_is_master(bus)) {
 		npcm_i2c_master_abort(bus);
 	} else {
+		bus->ber_state = true;
 		npcm_i2c_clear_master_status(bus);
 
 		/* Clear BB (BUS BUSY) bit */
@@ -1699,6 +1701,7 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
 		dev_dbg(bus->dev, "bus%d-0x%x recovery skipped, bus not stuck",
 			bus->num, bus->dest_addr);
 		npcm_i2c_reset(bus);
+		bus->ber_state = false;
 		return 0;
 	}
 
@@ -1763,6 +1766,7 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
 		if (bus->rec_succ_cnt < ULLONG_MAX)
 			bus->rec_succ_cnt++;
 	}
+	bus->ber_state = false;
 	return status;
 }
 
@@ -2158,7 +2162,16 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 	} while (time_is_after_jiffies(time_left) && bus_busy);
 
-	if (bus_busy) {
+	/*
+	 * Check the BER (bus error) state, when ber_state is true, it means that the module
+	 * detects the bus error which is caused by some factor like that the electricity
+	 * noise occurs on the bus. Under this condition, the module is reset and the bus
+	 * gets recovered.
+	 *
+	 * While ber_state is false, the module reset and bus recovery also get done as the
+	 * bus is busy.
+	 */
+	if (bus_busy || bus->ber_state) {
 		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
 		npcm_i2c_reset(bus);
 		i2c_recover_bus(adap);
-- 
2.34.1

[PATCH v5 3/6] i2c: npcm: Modify timeout evaluation mechanism

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

The users want to connect a lot of masters on the same bus.
This timeout is used to determine the time it takes to take bus ownership.
The transactions are very long, so waiting 35ms is not enough.

Increase the timeout and treat it as the total timeout, including retries.
The total timeout is 2 seconds now.

The i2c core layer will have chances to retry to call the i2c driver
transfer function if the i2c driver reports that the bus is busy and
returns EAGAIN.

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 7620bdcdc235..03d6c8702ecf 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2132,19 +2132,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		}
 	}
 
-	/*
-	 * Adaptive TimeOut: estimated time in usec + 100% margin:
-	 * 2: double the timeout for clock stretching case
-	 * 9: bits per transaction (including the ack/nack)
-	 */
-	timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
-	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 + timeout + 1;
+	time_left = jiffies + bus->adap.timeout / bus->adap.retries + 1;
 	do {
 		/*
 		 * we must clear slave address immediately when the bus is not
@@ -2192,6 +2185,14 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	if (npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
 				       write_data, read_data, read_PEC,
 				       read_block)) {
+		/*
+		 * Adaptive TimeOut: estimated time in usec + 100% margin:
+		 * 2: double the timeout for clock stretching case
+		 * 9: bits per transaction (including the ack/nack)
+		 */
+		timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
+		timeout = max_t(unsigned long, bus->adap.timeout / bus->adap.retries,
+				usecs_to_jiffies(timeout_usec));
 		time_left = wait_for_completion_timeout(&bus->cmd_complete,
 							timeout);
 
@@ -2317,7 +2318,12 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	adap = &bus->adap;
 	adap->owner = THIS_MODULE;
 	adap->retries = 3;
-	adap->timeout = msecs_to_jiffies(35);
+	/*
+	 * The users want to connect a lot of masters on the same bus.
+	 * This timeout is used to determine the time it takes to take bus ownership.
+	 * The transactions are very long, so waiting 35ms is not enough.
+	 */
+	adap->timeout = 2 * HZ;
 	adap->algo = &npcm_i2c_algo;
 	adap->quirks = &npcm_i2c_quirks;
 	adap->algo_data = bus;
-- 
2.34.1

[PATCH v5 4/6] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

Store the client address earlier since it might get called in
the i2c_recover_bus() logic flow at the early stage of
npcm_i2c_master_xfer().

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 03d6c8702ecf..0ee77e1fbc08 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2155,6 +2155,19 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 	} while (time_is_after_jiffies(time_left) && bus_busy);
 
+	/*
+	 * Previously, the address was stored w/o left-shift by one bit and
+	 * with that shift in the following call to npcm_i2c_master_start_xmit().
+	 *
+	 * Since there are cases that the i2c_recover_bus() gets called at the
+	 * early stage of npcm_i2c_master_xfer(), the address is stored with
+	 * the shift and used in the i2c_recover_bus().
+	 *
+	 * The address is stored from bit 1 to bit 7 in the register for
+	 * sending the i2c address later so it's left-shifted by 1 bit.
+	 */
+	bus->dest_addr = slave_addr << 1;
+
 	/*
 	 * Check the BER (bus error) state, when ber_state is true, it means that the module
 	 * detects the bus error which is caused by some factor like that the electricity
@@ -2172,7 +2185,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	}
 
 	npcm_i2c_init_params(bus);
-	bus->dest_addr = slave_addr;
 	bus->msgs = msgs;
 	bus->msgs_num = num;
 	bus->cmd_err = 0;
-- 
2.34.1

[PATCH v5 5/6] i2c: npcm: use i2c frequency table

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

Modify i2c frequency from table parameters
for NPCM i2c modules.

Supported frequencies are:

1. 100KHz
2. 400KHz
3. 1MHz

The original equations were tested on a variety of chips and base clocks.
Since we added devices that use higher frequencies of the module we
saw that there is a mismatch between the equation and the actual
results on the bus itself, measured on scope.

Meanwhile, the equations were not accurate to begin with.
They are an approximation of the ideal value. The ideal value is
calculated per frequency of the core module.

So instead of using the equations we did an optimization per module
frequency, verified on a device.

Most of the work was focused on the rise time of the SCL and SDA,
which depends on external load of the bus and PU.

Different PCB designs, or specifically to this case: the number
and type of targets on the bus, impact the required values for
the timing registers.

Users can recalculate the numbers for each bus and get an even better
optimization, but our users chose not to.

We manually picked values per frequency that match the entire valid
range of targets (from 1 to max number). Then we check against the
AMR described in SMB spec and make sure that none of the values
is exceeding.

This process was led by the chip architect and included a lot of testing.

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 374 ++++++++++++++++++++++++-------
 1 file changed, 288 insertions(+), 86 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 0ee77e1fbc08..2ed69e92edf6 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -263,6 +263,265 @@ 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 smb_timing_t {
+	u32 core_clk;
+	u8 hldt;
+	u8 dbcnt;
+	u16 sclfrq;
+	u8 scllt;
+	u8 sclht;
+	bool fast_mode;
+};
+
+static struct smb_timing_t smb_timing_100khz[] = {
+	{
+		.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x4,
+		.sclfrq = 0xFB, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 62500000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x9D, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 50000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x7E, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 48000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x79, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 40000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x65, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 30000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x4C, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 29000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x49, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 26000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x42, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 25000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x3F, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 24000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x3D, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 20000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x33, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 16180000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x29, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 15000000, .hldt = 0x23, .dbcnt = 0x1,
+		.sclfrq = 0x26, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 13000000, .hldt = 0x1D, .dbcnt = 0x1,
+		.sclfrq = 0x21, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 12000000, .hldt = 0x1B, .dbcnt = 0x1,
+		.sclfrq = 0x1F, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 10000000, .hldt = 0x18, .dbcnt = 0x1,
+		.sclfrq = 0x1A, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 9000000, .hldt = 0x16, .dbcnt = 0x1,
+		.sclfrq = 0x17, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 8090000, .hldt = 0x14, .dbcnt = 0x1,
+		.sclfrq = 0x15, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 7500000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x13, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 6500000, .hldt = 0xE, .dbcnt = 0x1,
+		.sclfrq = 0x11, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 4000000, .hldt = 0x9, .dbcnt = 0x1,
+		.sclfrq = 0xB, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+};
+
+static struct smb_timing_t smb_timing_400khz[] = {
+	{
+		.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x3,
+		.sclfrq = 0x0, .scllt = 0x47, .sclht = 0x35,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 62500000, .hldt = 0x2A, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0x2C, .sclht = 0x22,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 50000000, .hldt = 0x21, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x24, .sclht = 0x1B,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 48000000, .hldt = 0x1E, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x24, .sclht = 0x19,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 40000000, .hldt = 0x1B, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x1E, .sclht = 0x14,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 33000000, .hldt = 0x15, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x19, .sclht = 0x11,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 30000000, .hldt = 0x15, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x19, .sclht = 0xD,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 29000000, .hldt = 0x11, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x15, .sclht = 0x10,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 26000000, .hldt = 0x10, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x13, .sclht = 0xE,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 25000000, .hldt = 0xF, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x13, .sclht = 0xD,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 24000000, .hldt = 0xD, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x12, .sclht = 0xD,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 20000000, .hldt = 0xB, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xF, .sclht = 0xA,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 16180000, .hldt = 0xA, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xC, .sclht = 0x9,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 15000000, .hldt = 0x9, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xB, .sclht = 0x8,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 13000000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xA, .sclht = 0x7,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 12000000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xA, .sclht = 0x6,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 10000000, .hldt = 0x6, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x8, .sclht = 0x5,
+		.fast_mode = true,
+	},
+};
+
+static struct smb_timing_t smb_timing_1000khz[] = {
+	{
+		.core_clk = 100000000, .hldt = 0x15, .dbcnt = 0x4,
+		.sclfrq = 0x0, .scllt = 0x1C, .sclht = 0x15,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 62500000, .hldt = 0xF, .dbcnt = 0x3,
+		.sclfrq = 0x0, .scllt = 0x11, .sclht = 0xE,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 50000000, .hldt = 0xA, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0xE, .sclht = 0xB,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 48000000, .hldt = 0x9, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0xD, .sclht = 0xB,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 41000000, .hldt = 0x9, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0xC, .sclht = 0x9,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 40000000, .hldt = 0x8, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0xB, .sclht = 0x9,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 33000000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xA, .sclht = 0x7,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 25000000, .hldt = 0x4, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x7, .sclht = 0x6,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 24000000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x8, .sclht = 0x5,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 20000000, .hldt = 0x4, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x6, .sclht = 0x4,
+		.fast_mode = true,
+	},
+};
+
 struct npcm_i2c_data {
 	u8 fifo_size;
 	u32 segctl_init_val;
@@ -1805,102 +2064,45 @@ static void npcm_i2c_recovery_init(struct i2c_adapter *_adap)
  */
 static int npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq_hz)
 {
-	u32  k1 = 0;
-	u32  k2 = 0;
-	u8   dbnct = 0;
-	u32  sclfrq = 0;
-	u8   hldt = 7;
+	struct  smb_timing_t *smb_timing;
+	u8   scl_table_cnt = 0, table_size = 0;
 	u8   fast_mode = 0;
-	u32  src_clk_khz;
-	u32  bus_freq_khz;
 
-	src_clk_khz = bus->apb_clk / 1000;
-	bus_freq_khz = bus_freq_hz / 1000;
 	bus->bus_freq = bus_freq_hz;
 
-	/* 100KHz and below: */
-	if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
-		sclfrq = src_clk_khz / (bus_freq_khz * 4);
-
-		if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
-			return -EDOM;
-
-		if (src_clk_khz >= 40000)
-			hldt = 17;
-		else if (src_clk_khz >= 12500)
-			hldt = 15;
-		else
-			hldt = 7;
-	}
-
-	/* 400KHz: */
-	else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
-		sclfrq = 0;
+	switch (bus_freq_hz) {
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		smb_timing = smb_timing_100khz;
+		table_size = ARRAY_SIZE(smb_timing_100khz);
+		break;
+	case I2C_MAX_FAST_MODE_FREQ:
+		smb_timing = smb_timing_400khz;
+		table_size = ARRAY_SIZE(smb_timing_400khz);
 		fast_mode = I2CCTL3_400K_MODE;
-
-		if (src_clk_khz < 7500)
-			/* 400KHZ cannot be supported for core clock < 7.5MHz */
-			return -EDOM;
-
-		else if (src_clk_khz >= 50000) {
-			k1 = 80;
-			k2 = 48;
-			hldt = 12;
-			dbnct = 7;
-		}
-
-		/* Master or Slave with frequency > 25MHz */
-		else if (src_clk_khz > 25000) {
-			hldt = clk_coef(src_clk_khz, 300) + 7;
-			k1 = clk_coef(src_clk_khz, 1600);
-			k2 = clk_coef(src_clk_khz, 900);
-		}
-	}
-
-	/* 1MHz: */
-	else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
-		sclfrq = 0;
+		break;
+	case I2C_MAX_FAST_MODE_PLUS_FREQ:
+		smb_timing = smb_timing_1000khz;
+		table_size = ARRAY_SIZE(smb_timing_1000khz);
 		fast_mode = I2CCTL3_400K_MODE;
-
-		/* 1MHZ cannot be supported for core clock < 24 MHz */
-		if (src_clk_khz < 24000)
-			return -EDOM;
-
-		k1 = clk_coef(src_clk_khz, 620);
-		k2 = clk_coef(src_clk_khz, 380);
-
-		/* Core clk > 40 MHz */
-		if (src_clk_khz > 40000) {
-			/*
-			 * Set HLDT:
-			 * SDA hold time:  (HLDT-7) * T(CLK) >= 120
-			 * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
-			 */
-			hldt = clk_coef(src_clk_khz, 120) + 7;
-		} else {
-			hldt = 7;
-			dbnct = 2;
-		}
+		break;
+	default:
+		return -EINVAL;
 	}
 
-	/* Frequency larger than 1 MHz is not supported */
-	else
-		return -EINVAL;
+	for (scl_table_cnt = 0; scl_table_cnt < table_size; scl_table_cnt++)
+		if (bus->apb_clk >= smb_timing[scl_table_cnt].core_clk)
+			break;
 
-	if (bus_freq_hz >= I2C_MAX_FAST_MODE_FREQ) {
-		k1 = round_up(k1, 2);
-		k2 = round_up(k2 + 1, 2);
-		if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
-		    k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
-			return -EDOM;
-	}
+	if (scl_table_cnt == table_size)
+		return -EINVAL;
 
 	/* write sclfrq value. bits [6:0] are in I2CCTL2 reg */
-	iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, sclfrq & 0x7F),
+	iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, smb_timing[scl_table_cnt].sclfrq & 0x7F),
 		 bus->reg + NPCM_I2CCTL2);
 
 	/* bits [8:7] are in I2CCTL3 reg */
-	iowrite8(fast_mode | FIELD_PREP(I2CCTL3_SCLFRQ8_7, (sclfrq >> 7) & 0x3),
+	iowrite8(FIELD_PREP(I2CCTL3_SCLFRQ8_7, (smb_timing[scl_table_cnt].sclfrq >> 7) & 0x3) |
+		 fast_mode,
 		 bus->reg + NPCM_I2CCTL3);
 
 	/* Select Bank 0 to access NPCM_I2CCTL4/NPCM_I2CCTL5 */
@@ -1912,13 +2114,13 @@ static int npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq_hz)
 		 * k1 = 2 * SCLLT7-0 -> Low Time  = k1 / 2
 		 * k2 = 2 * SCLLT7-0 -> High Time = k2 / 2
 		 */
-		iowrite8(k1 / 2, bus->reg + NPCM_I2CSCLLT);
-		iowrite8(k2 / 2, bus->reg + NPCM_I2CSCLHT);
+		iowrite8(smb_timing[scl_table_cnt].scllt, bus->reg + NPCM_I2CSCLLT);
+		iowrite8(smb_timing[scl_table_cnt].sclht, bus->reg + NPCM_I2CSCLHT);
 
-		iowrite8(dbnct, bus->reg + NPCM_I2CCTL5);
+		iowrite8(smb_timing[scl_table_cnt].dbcnt, bus->reg + NPCM_I2CCTL5);
 	}
 
-	iowrite8(hldt, bus->reg + NPCM_I2CCTL4);
+	iowrite8(smb_timing[scl_table_cnt].hldt, bus->reg + NPCM_I2CCTL4);
 
 	/* Return to Bank 1, and stay there by default: */
 	npcm_i2c_select_bank(bus, I2C_BANK_1);
-- 
2.34.1

[PATCH v5 6/6] i2c: npcm: Enable slave in eob interrupt

[Tyrone Ting]

From: Charles Boyer <Charles.Boyer@fii-usa.com>

Nuvoton slave enable was in user space API call master_xfer, so it is
subject to delays from the OS scheduler. If the BMC is not enabled for
slave mode in time for master to send response, then it will NAK the
address match. Then the PLDM request timeout occurs.

If the slave enable is moved to the EOB interrupt service routine, then
the BMC can be ready in slave mode by the time it needs to receive a
response.

Signed-off-by: Charles Boyer <Charles.Boyer@fii-usa.com>
Signed-off-by: Vivekanand Veeracholan <vveerach@google.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2ed69e92edf6..90a6e6842c6b 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -1925,6 +1925,12 @@ static int npcm_i2c_int_master_handler(struct npcm_i2c *bus)
 	    (FIELD_GET(NPCM_I2CCST3_EO_BUSY,
 		       ioread8(bus->reg + NPCM_I2CCST3)))) {
 		npcm_i2c_irq_handle_eob(bus);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		/* reenable slave if it was enabled */
+		if (bus->slave)
+			iowrite8((bus->slave->addr & 0x7F) | NPCM_I2CADDR_SAEN,
+				 bus->reg + NPCM_I2CADDR1);
+#endif
 		return 0;
 	}
 
-- 
2.34.1

Re: [PATCH v5 3/6] i2c: npcm: Modify timeout evaluation mechanism

[Andy Shevchenko]

On Tue, Oct 01, 2024 at 02:28:52PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> The users want to connect a lot of masters on the same bus.
> This timeout is used to determine the time it takes to take bus ownership.
> The transactions are very long, so waiting 35ms is not enough.
> 
> Increase the timeout and treat it as the total timeout, including retries.
> The total timeout is 2 seconds now.
> 
> The i2c core layer will have chances to retry to call the i2c driver
> transfer function if the i2c driver reports that the bus is busy and
> returns EAGAIN.

-EAGAIN

...

> +		/*
> +		 * Adaptive TimeOut: estimated time in usec + 100% margin:
> +		 * 2: double the timeout for clock stretching case
> +		 * 9: bits per transaction (including the ack/nack)
> +		 */
> +		timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);

Side note (as I see it was in the original code), from physics
point of view the USEC_PER_SEC here should be simply MICRO
(as 1/Hz == s, and here it will be read as s^2 in the result),
but if one finds the current more understandable, okay then.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 4/6] i2c: npcm: Modify the client address assignment

[Andy Shevchenko]

On Tue, Oct 01, 2024 at 02:28:53PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> Store the client address earlier since it might get called in
> the i2c_recover_bus() logic flow at the early stage of
> npcm_i2c_master_xfer().

...

> +	/*
> +	 * Previously, the address was stored w/o left-shift by one bit and
> +	 * with that shift in the following call to npcm_i2c_master_start_xmit().
> +	 *
> +	 * Since there are cases that the i2c_recover_bus() gets called at the
> +	 * early stage of npcm_i2c_master_xfer(), the address is stored with
> +	 * the shift and used in the i2c_recover_bus().
> +	 *
> +	 * The address is stored from bit 1 to bit 7 in the register for
> +	 * sending the i2c address later so it's left-shifted by 1 bit.
> +	 */
> +	bus->dest_addr = slave_addr << 1;

I'm wondering if it's better to use i2c_8bit_addr_from_msg() here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 5/6] i2c: npcm: use i2c frequency table

[Andy Shevchenko]

On Tue, Oct 01, 2024 at 02:28:54PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> Modify i2c frequency from table parameters
> for NPCM i2c modules.

This two lines have a too small wrapping limit.

> Supported frequencies are:
> 
> 1. 100KHz
> 2. 400KHz
> 3. 1MHz
> 
> The original equations were tested on a variety of chips and base clocks.
> Since we added devices that use higher frequencies of the module we
> saw that there is a mismatch between the equation and the actual
> results on the bus itself, measured on scope.
> 
> Meanwhile, the equations were not accurate to begin with.
> They are an approximation of the ideal value. The ideal value is
> calculated per frequency of the core module.
> 
> So instead of using the equations we did an optimization per module
> frequency, verified on a device.
> 
> Most of the work was focused on the rise time of the SCL and SDA,
> which depends on external load of the bus and PU.
> 
> Different PCB designs, or specifically to this case: the number
> and type of targets on the bus, impact the required values for
> the timing registers.
> 
> Users can recalculate the numbers for each bus and get an even better
> optimization, but our users chose not to.
> 
> We manually picked values per frequency that match the entire valid
> range of targets (from 1 to max number). Then we check against the
> AMR described in SMB spec and make sure that none of the values
> is exceeding.
> 
> This process was led by the chip architect and included a lot of testing.

Personally I consider table approach is not so flexible and it is definitely
does not scale (in the result — hard to maintain for all customers), but if
it's hard to calculate all necessary data and there are other pros of it,
I'm fine.

TL;DR: I don't like this patch, but I don't want to stop you, hence no tags
from me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 0/6] i2c: npcm: read/write operation, checkpatch

[Andi Shyti]

Hi Tyrone,

> Tyrone Ting (5):
>   i2c: npcm: correct the read/write operation procedure
>   i2c: npcm: use a software flag to indicate a BER condition

I merged just these two patches to i2c/i2c-host.

Thanks,
Andi

>   i2c: npcm: Modify timeout evaluation mechanism
>   i2c: npcm: Modify the client address assignment
>   i2c: npcm: use i2c frequency table

Re: [PATCH v5 0/6] i2c: npcm: read/write operation, checkpatch

[Tyrone Ting]

Hi Andi:

Thank you for your support.

Andi Shyti <andi.shyti@kernel.org> 於 2024年10月2日 週三 下午4:34寫道：
>
> Hi Tyrone,
>
> > Tyrone Ting (5):
> >   i2c: npcm: correct the read/write operation procedure
> >   i2c: npcm: use a software flag to indicate a BER condition
>
> I merged just these two patches to i2c/i2c-host.
>
> Thanks,
> Andi
>
> >   i2c: npcm: Modify timeout evaluation mechanism
> >   i2c: npcm: Modify the client address assignment
> >   i2c: npcm: use i2c frequency table

Have a nice day.

Regards,
Tyrone

Re: [PATCH v5 4/6] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andy:

Thank you for your comments and they'll be addressed.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年10月1日 週二 下午9:17寫道：
>
> On Tue, Oct 01, 2024 at 02:28:53PM +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > Store the client address earlier since it might get called in
> > the i2c_recover_bus() logic flow at the early stage of
> > npcm_i2c_master_xfer().
>
> ...
>
> > +     /*
> > +      * Previously, the address was stored w/o left-shift by one bit and
> > +      * with that shift in the following call to npcm_i2c_master_start_xmit().
> > +      *
> > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > +      * early stage of npcm_i2c_master_xfer(), the address is stored with
> > +      * the shift and used in the i2c_recover_bus().
> > +      *
> > +      * The address is stored from bit 1 to bit 7 in the register for
> > +      * sending the i2c address later so it's left-shifted by 1 bit.
> > +      */
> > +     bus->dest_addr = slave_addr << 1;
>
> I'm wondering if it's better to use i2c_8bit_addr_from_msg() here?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Have a nice day.

Regards,
Tyrone

Re: [PATCH v5 5/6] i2c: npcm: use i2c frequency table

[Tyrone Ting]

Hi Andy:

Thank you for your comments.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年10月1日 週二 下午9:23寫道：
>
> On Tue, Oct 01, 2024 at 02:28:54PM +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > Modify i2c frequency from table parameters
> > for NPCM i2c modules.
>
> This two lines have a too small wrapping limit.
>

I'll make the statement in one line.

> > Supported frequencies are:
> >
> > 1. 100KHz
> > 2. 400KHz
> > 3. 1MHz
> >
> > The original equations were tested on a variety of chips and base clocks.
> > Since we added devices that use higher frequencies of the module we
> > saw that there is a mismatch between the equation and the actual
> > results on the bus itself, measured on scope.
> >
> > Meanwhile, the equations were not accurate to begin with.
> > They are an approximation of the ideal value. The ideal value is
> > calculated per frequency of the core module.
> >
> > So instead of using the equations we did an optimization per module
> > frequency, verified on a device.
> >
> > Most of the work was focused on the rise time of the SCL and SDA,
> > which depends on external load of the bus and PU.
> >
> > Different PCB designs, or specifically to this case: the number
> > and type of targets on the bus, impact the required values for
> > the timing registers.
> >
> > Users can recalculate the numbers for each bus and get an even better
> > optimization, but our users chose not to.
> >
> > We manually picked values per frequency that match the entire valid
> > range of targets (from 1 to max number). Then we check against the
> > AMR described in SMB spec and make sure that none of the values
> > is exceeding.
> >
> > This process was led by the chip architect and included a lot of testing.
>
> Personally I consider table approach is not so flexible and it is definitely
> does not scale (in the result — hard to maintain for all customers), but if
> it's hard to calculate all necessary data and there are other pros of it,
> I'm fine.
>
> TL;DR: I don't like this patch, but I don't want to stop you, hence no tags
> from me.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thank you again.

Regards,
Tyrone

Re: [PATCH v5 4/6] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andy:

Thank you for your comments.

After a second thought, I'll explain why slave_addr << 1 is given here.

Tyrone Ting <warp5tw@gmail.com> 於 2024年10月4日 週五 上午9:49寫道：
>
> Hi Andy:
>
> Thank you for your comments and they'll be addressed.
>
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年10月1日 週二 下午9:17寫道：
> >
> > On Tue, Oct 01, 2024 at 02:28:53PM +0800, Tyrone Ting wrote:
> > > From: Tyrone Ting <kfting@nuvoton.com>
> > >
> > > Store the client address earlier since it might get called in
> > > the i2c_recover_bus() logic flow at the early stage of
> > > npcm_i2c_master_xfer().
> >
> > ...
> >
> > > +     /*
> > > +      * Previously, the address was stored w/o left-shift by one bit and
> > > +      * with that shift in the following call to npcm_i2c_master_start_xmit().
> > > +      *
> > > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > > +      * early stage of npcm_i2c_master_xfer(), the address is stored with
> > > +      * the shift and used in the i2c_recover_bus().
> > > +      *
> > > +      * The address is stored from bit 1 to bit 7 in the register for
> > > +      * sending the i2c address later so it's left-shifted by 1 bit.
> > > +      */
> > > +     bus->dest_addr = slave_addr << 1;
> >
> > I'm wondering if it's better to use i2c_8bit_addr_from_msg() here?
> >

The current implementation of i2c_8bit_addr_from_msg() (ref link:
https://github.com/torvalds/linux/blob/master/include/linux/i2c.h#L947)
is
"return (msg->addr << 1) | (msg->flags & I2C_M_RD);" and it takes
extra consideration about the read flag when retrieving the i2c
address.
IOW, if there is a read event, the i2c address contains a read
indication (bit 0 of the i2c address is 1).

The patch code "bus->dest_addr = slave_addr << 1;" might get used in
i2c_recover_bus() later. (ref link:
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1691)

Suppose there is a read event and the i2c address is 0x60.

With i2c_8bit_addr_from_msg(), bus->dest_addr will be 0xc1.
With the original patch, bus->dest_addr will be 0xc0.

If some error condition occurs and it requires i2c_recover_bus() to
recover the bus, according to the description at
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1742,
the address "0xc1" is used
as a parameter to npcm_i2c_wr_byte() which is used to send the address
in the write direction.

If i2c_8bit_addr_from_msg() is applied, it might not fit the scenario
described at
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1742,
which is about to send
an address in a write direction since the address from
i2c_8bit_addr_from_msg() contains a read indication.

> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
>
> Have a nice day.
>
> Regards,
> Tyrone

Thank you.

Regards,
Tyrone

Re: [PATCH v5 4/6] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andy:

May I have your feedback on my reply about keeping the original patch
"bus->dest_addr = slave_addr << 1;"?

Thank you for your time.

Tyrone Ting <warp5tw@gmail.com> 於 2024年10月4日 週五 上午10:29寫道：
>
> Hi Andy:
>
> Thank you for your comments.
>
> After a second thought, I'll explain why slave_addr << 1 is given here.
>
> Tyrone Ting <warp5tw@gmail.com> 於 2024年10月4日 週五 上午9:49寫道：
> >
> > Hi Andy:
> >
> > Thank you for your comments and they'll be addressed.
> >
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年10月1日 週二 下午9:17寫道：
> > >
> > > On Tue, Oct 01, 2024 at 02:28:53PM +0800, Tyrone Ting wrote:
> > > > From: Tyrone Ting <kfting@nuvoton.com>
> > > >
> > > > Store the client address earlier since it might get called in
> > > > the i2c_recover_bus() logic flow at the early stage of
> > > > npcm_i2c_master_xfer().
> > >
> > > ...
> > >
> > > > +     /*
> > > > +      * Previously, the address was stored w/o left-shift by one bit and
> > > > +      * with that shift in the following call to npcm_i2c_master_start_xmit().
> > > > +      *
> > > > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > > > +      * early stage of npcm_i2c_master_xfer(), the address is stored with
> > > > +      * the shift and used in the i2c_recover_bus().
> > > > +      *
> > > > +      * The address is stored from bit 1 to bit 7 in the register for
> > > > +      * sending the i2c address later so it's left-shifted by 1 bit.
> > > > +      */
> > > > +     bus->dest_addr = slave_addr << 1;
> > >
> > > I'm wondering if it's better to use i2c_8bit_addr_from_msg() here?
> > >
>
> The current implementation of i2c_8bit_addr_from_msg() (ref link:
> https://github.com/torvalds/linux/blob/master/include/linux/i2c.h#L947)
> is
> "return (msg->addr << 1) | (msg->flags & I2C_M_RD);" and it takes
> extra consideration about the read flag when retrieving the i2c
> address.
> IOW, if there is a read event, the i2c address contains a read
> indication (bit 0 of the i2c address is 1).
>
> The patch code "bus->dest_addr = slave_addr << 1;" might get used in
> i2c_recover_bus() later. (ref link:
> https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1691)
>
> Suppose there is a read event and the i2c address is 0x60.
>
> With i2c_8bit_addr_from_msg(), bus->dest_addr will be 0xc1.
> With the original patch, bus->dest_addr will be 0xc0.
>
> If some error condition occurs and it requires i2c_recover_bus() to
> recover the bus, according to the description at
> https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1742,
> the address "0xc1" is used
> as a parameter to npcm_i2c_wr_byte() which is used to send the address
> in the write direction.
>
> If i2c_8bit_addr_from_msg() is applied, it might not fit the scenario
> described at
> https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1742,
> which is about to send
> an address in a write direction since the address from
> i2c_8bit_addr_from_msg() contains a read indication.
>
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> >
> > Have a nice day.
> >
> > Regards,
> > Tyrone
>
> Thank you.
>
> Regards,
> Tyrone

Thank you.

Regards,
Tyrone

Re: [PATCH v5 4/6] i2c: npcm: Modify the client address assignment

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 10:29:10AM +0800, Tyrone Ting wrote:
> Hi Andy:
> 
> Thank you for your comments.
> 
> After a second thought, I'll explain why slave_addr << 1 is given here.
> 
> Tyrone Ting <warp5tw@gmail.com> 於 2024年10月4日 週五 上午9:49寫道：
> >
> > Hi Andy:
> >
> > Thank you for your comments and they'll be addressed.
> >
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年10月1日 週二 下午9:17寫道：
> > >
> > > On Tue, Oct 01, 2024 at 02:28:53PM +0800, Tyrone Ting wrote:
> > > > From: Tyrone Ting <kfting@nuvoton.com>
> > > >
> > > > Store the client address earlier since it might get called in
> > > > the i2c_recover_bus() logic flow at the early stage of
> > > > npcm_i2c_master_xfer().
> > >
> > > ...
> > >
> > > > +     /*
> > > > +      * Previously, the address was stored w/o left-shift by one bit and
> > > > +      * with that shift in the following call to npcm_i2c_master_start_xmit().
> > > > +      *
> > > > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > > > +      * early stage of npcm_i2c_master_xfer(), the address is stored with
> > > > +      * the shift and used in the i2c_recover_bus().
> > > > +      *
> > > > +      * The address is stored from bit 1 to bit 7 in the register for
> > > > +      * sending the i2c address later so it's left-shifted by 1 bit.
> > > > +      */
> > > > +     bus->dest_addr = slave_addr << 1;
> > >
> > > I'm wondering if it's better to use i2c_8bit_addr_from_msg() here?
> > >
> 
> The current implementation of i2c_8bit_addr_from_msg() (ref link:
> https://github.com/torvalds/linux/blob/master/include/linux/i2c.h#L947)
> is
> "return (msg->addr << 1) | (msg->flags & I2C_M_RD);" and it takes
> extra consideration about the read flag when retrieving the i2c
> address.
> IOW, if there is a read event, the i2c address contains a read
> indication (bit 0 of the i2c address is 1).
> 
> The patch code "bus->dest_addr = slave_addr << 1;" might get used in
> i2c_recover_bus() later. (ref link:
> https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1691)
> 
> Suppose there is a read event and the i2c address is 0x60.
> 
> With i2c_8bit_addr_from_msg(), bus->dest_addr will be 0xc1.
> With the original patch, bus->dest_addr will be 0xc0.
> 
> If some error condition occurs and it requires i2c_recover_bus() to
> recover the bus, according to the description at
> https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1742,
> the address "0xc1" is used
> as a parameter to npcm_i2c_wr_byte() which is used to send the address
> in the write direction.
> 
> If i2c_8bit_addr_from_msg() is applied, it might not fit the scenario
> described at
> https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1742,
> which is about to send
> an address in a write direction since the address from
> i2c_8bit_addr_from_msg() contains a read indication.

Okay, then I would do the i2c_8bit_addr_from_msg() call here as AFAICS
this is the real event where you save the address *of the event*.

And in the respective user update the comment to summarize above and do
rather ->dest_addr & ~I2C_M_RD there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 4/6] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andy:

Thank you for your comments and they'll be addressed.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年10月9日 週三 上午12:21寫道：
>
> On Fri, Oct 04, 2024 at 10:29:10AM +0800, Tyrone Ting wrote:
> > Hi Andy:
> >
> > Thank you for your comments.
> >
> > After a second thought, I'll explain why slave_addr << 1 is given here.
> >
> > Tyrone Ting <warp5tw@gmail.com> 於 2024年10月4日 週五 上午9:49寫道：
> > >
> > > Hi Andy:
> > >
> > > Thank you for your comments and they'll be addressed.
> > >
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年10月1日 週二 下午9:17寫道：
> > > >
> > > > On Tue, Oct 01, 2024 at 02:28:53PM +0800, Tyrone Ting wrote:
> > > > > From: Tyrone Ting <kfting@nuvoton.com>
> > > > >
> > > > > Store the client address earlier since it might get called in
> > > > > the i2c_recover_bus() logic flow at the early stage of
> > > > > npcm_i2c_master_xfer().
> > > >
> > > > ...
> > > >
> > > > > +     /*
> > > > > +      * Previously, the address was stored w/o left-shift by one bit and
> > > > > +      * with that shift in the following call to npcm_i2c_master_start_xmit().
> > > > > +      *
> > > > > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > > > > +      * early stage of npcm_i2c_master_xfer(), the address is stored with
> > > > > +      * the shift and used in the i2c_recover_bus().
> > > > > +      *
> > > > > +      * The address is stored from bit 1 to bit 7 in the register for
> > > > > +      * sending the i2c address later so it's left-shifted by 1 bit.
> > > > > +      */
> > > > > +     bus->dest_addr = slave_addr << 1;
> > > >
> > > > I'm wondering if it's better to use i2c_8bit_addr_from_msg() here?
> > > >
> >
> > The current implementation of i2c_8bit_addr_from_msg() (ref link:
> > https://github.com/torvalds/linux/blob/master/include/linux/i2c.h#L947)
> > is
> > "return (msg->addr << 1) | (msg->flags & I2C_M_RD);" and it takes
> > extra consideration about the read flag when retrieving the i2c
> > address.
> > IOW, if there is a read event, the i2c address contains a read
> > indication (bit 0 of the i2c address is 1).
> >
> > The patch code "bus->dest_addr = slave_addr << 1;" might get used in
> > i2c_recover_bus() later. (ref link:
> > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1691)
> >
> > Suppose there is a read event and the i2c address is 0x60.
> >
> > With i2c_8bit_addr_from_msg(), bus->dest_addr will be 0xc1.
> > With the original patch, bus->dest_addr will be 0xc0.
> >
> > If some error condition occurs and it requires i2c_recover_bus() to
> > recover the bus, according to the description at
> > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1742,
> > the address "0xc1" is used
> > as a parameter to npcm_i2c_wr_byte() which is used to send the address
> > in the write direction.
> >
> > If i2c_8bit_addr_from_msg() is applied, it might not fit the scenario
> > described at
> > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1742,
> > which is about to send
> > an address in a write direction since the address from
> > i2c_8bit_addr_from_msg() contains a read indication.
>
> Okay, then I would do the i2c_8bit_addr_from_msg() call here as AFAICS
> this is the real event where you save the address *of the event*.
>
> And in the respective user update the comment to summarize above and do
> rather ->dest_addr & ~I2C_M_RD there.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thank you.

Regards,
Tyrone

Re: [PATCH v5 3/6] i2c: npcm: Modify timeout evaluation mechanism

[Tyrone Ting]

Hi Andy:

Thank you for your comments.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年10月1日 週二 下午9:14寫道：
>
> On Tue, Oct 01, 2024 at 02:28:52PM +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > The users want to connect a lot of masters on the same bus.
> > This timeout is used to determine the time it takes to take bus ownership.
> > The transactions are very long, so waiting 35ms is not enough.
> >
> > Increase the timeout and treat it as the total timeout, including retries.
> > The total timeout is 2 seconds now.
> >
> > The i2c core layer will have chances to retry to call the i2c driver
> > transfer function if the i2c driver reports that the bus is busy and
> > returns EAGAIN.
>
> -EAGAIN
>
> ...
>
> > +             /*
> > +              * Adaptive TimeOut: estimated time in usec + 100% margin:
> > +              * 2: double the timeout for clock stretching case
> > +              * 9: bits per transaction (including the ack/nack)
> > +              */
> > +             timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
>
> Side note (as I see it was in the original code), from physics
> point of view the USEC_PER_SEC here should be simply MICRO
> (as 1/Hz == s, and here it will be read as s^2 in the result),
> but if one finds the current more understandable, okay then.
>

I just check with Nuvoton members and they prefer the USEC_PER_SEC way.

> --
> With Best Regards,
> Andy Shevchenko
>
>

Thank you.

Regards,
Tyrone