[v2,1/1] i2c: designware: Add SMBus Quick Command support

[v2,1/1] i2c: designware: Add SMBus Quick Command support

[ende.tan]

From: Tan En De <ende.tan@starfivetech.com>

Add support for SMBus Quick Read and Quick Write commands.

Signed-off-by: Tan En De <ende.tan@starfivetech.com>
---
v1 -> v2: Removed redundant check of tx_limit and rx_limit
---
 drivers/i2c/busses/i2c-designware-core.h   |  4 ++++
 drivers/i2c/busses/i2c-designware-master.c | 18 +++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 347843b4f5dd..91f17181ece1 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -40,6 +40,8 @@
 
 #define DW_IC_DATA_CMD_DAT			GENMASK(7, 0)
 #define DW_IC_DATA_CMD_FIRST_DATA_BYTE		BIT(11)
+#define DW_IC_DATA_CMD_STOP			BIT(9)
+#define DW_IC_DATA_CMD_CMD			BIT(8)
 
 /*
  * Registers offset
@@ -123,7 +125,9 @@
 
 #define DW_IC_ERR_TX_ABRT			0x1
 
+#define DW_IC_TAR_SMBUS_QUICK_CMD		BIT(16)
 #define DW_IC_TAR_10BITADDR_MASTER		BIT(12)
+#define DW_IC_TAR_SPECIAL			BIT(11)
 
 #define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH	(BIT(2) | BIT(3))
 #define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK	GENMASK(3, 2)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index c5394229b77f..a67add117e44 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -268,6 +268,10 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	regmap_update_bits(dev->map, DW_IC_CON, DW_IC_CON_10BITADDR_MASTER,
 			   ic_con);
 
+	/* i2c-core-smbus.c: Only I2C_SMBUS_QUICK has msg[0].len = 0 */
+	if (dev->msgs[0].len == 0)
+		ic_tar |= DW_IC_TAR_SMBUS_QUICK_CMD | DW_IC_TAR_SPECIAL;
+
 	/*
 	 * Set the slave (target) address and enable 10-bit addressing mode
 	 * if applicable.
@@ -474,6 +478,16 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 		regmap_read(dev->map, DW_IC_RXFLR, &flr);
 		rx_limit = dev->rx_fifo_depth - flr;
 
+		/* i2c-core-smbus.c: Only I2C_SMBUS_QUICK has msg[0].len = 0 */
+		if (buf_len == 0) {
+			regmap_write(
+				dev->map, DW_IC_DATA_CMD,
+				*buf | DW_IC_DATA_CMD_STOP |
+				((msgs[dev->msg_write_idx].flags & I2C_M_RD) ?
+				DW_IC_DATA_CMD_CMD : 0)
+			);
+		}
+
 		while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
 			u32 cmd = 0;
 
@@ -919,7 +933,9 @@ void i2c_dw_configure_master(struct dw_i2c_dev *dev)
 {
 	struct i2c_timings *t = &dev->timings;
 
-	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
+	dev->functionality = I2C_FUNC_10BIT_ADDR |
+			     I2C_FUNC_SMBUS_QUICK |
+			     DW_IC_DEFAULT_FUNCTIONALITY;
 
 	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
 			  DW_IC_CON_RESTART_EN;
-- 
2.34.1

Re: [v2,1/1] i2c: designware: Add SMBus Quick Command support

[Andy Shevchenko]

On Sat, Apr 12, 2025 at 05:34:14PM +0800, ende.tan@starfivetech.com wrote:
> From: Tan En De <ende.tan@starfivetech.com>
> 
> Add support for SMBus Quick Read and Quick Write commands.

it's interesting how you made a versioning. Just run
`git format-patch -v<X>...` where <X> is the number of version you want,
it will make it properly in the Subject.

...

> +	/* i2c-core-smbus.c: Only I2C_SMBUS_QUICK has msg[0].len = 0 */

Please, remove filenames from the code, better to refer to the actual functions
as func(). This helps also to grep for all usages in case of renaming.

...

> +		/* i2c-core-smbus.c: Only I2C_SMBUS_QUICK has msg[0].len = 0 */

Ditto.

...

> +			regmap_write(
> +				dev->map, DW_IC_DATA_CMD,

Something wrong with the indentation. And you have plenty of room on the
previous line.

> +				*buf | DW_IC_DATA_CMD_STOP |
> +				((msgs[dev->msg_write_idx].flags & I2C_M_RD) ?
> +				DW_IC_DATA_CMD_CMD : 0)
> +			);

...

> -	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
> +	dev->functionality = I2C_FUNC_10BIT_ADDR |
> +			     I2C_FUNC_SMBUS_QUICK |
> +			     DW_IC_DEFAULT_FUNCTIONALITY;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [v2,1/1] i2c: designware: Add SMBus Quick Command support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]


> > +	/* i2c-core-smbus.c: Only I2C_SMBUS_QUICK has msg[0].len = 0 */
> 
> Please, remove filenames from the code, better to refer to the actual functions
> as func(). This helps also to grep for all usages in case of renaming.

The comment can be removed. Regular I2C messages can also have a length
of 0.

> > +			     DW_IC_DEFAULT_FUNCTIONALITY;

Sidenote: Why do we have this define? It is used only once and makes
reading the actual functionalities harder, I'd say.

Also, I wonder if this patch really works in practice. The
I2C_AQ_NO_ZERO_LEN flag is still set, so the core should prevent such
messages to be passed on to the driver?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v2,1/1] i2c: designware: Add SMBus Quick Command support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 191 bytes --]


> +#define DW_IC_TAR_SMBUS_QUICK_CMD		BIT(16)

This bit does not exist on the instance in the Renesas RZ/N1D SoC. So,
this patch needs some versioning which HW can do this and which can't.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v2,1/1] i2c: designware: Add SMBus Quick Command support

[Jarkko Nikula]

On 4/14/25 12:02 PM, Wolfram Sang wrote:
> 
>> +#define DW_IC_TAR_SMBUS_QUICK_CMD		BIT(16)
> 
> This bit does not exist on the instance in the Renesas RZ/N1D SoC. So,
> this patch needs some versioning which HW can do this and which can't.
> 
Yes, SMBUS features are only available starting from IP version 2.00a.

Re: [v2,1/1] i2c: designware: Add SMBus Quick Command support

[Jarkko Nikula]

Hi

On 4/12/25 12:34 PM, ende.tan@starfivetech.com wrote:
> From: Tan En De <ende.tan@starfivetech.com>
> 
> Add support for SMBus Quick Read and Quick Write commands.
> 
> Signed-off-by: Tan En De <ende.tan@starfivetech.com>
> ---
> v1 -> v2: Removed redundant check of tx_limit and rx_limit
> ---
>   drivers/i2c/busses/i2c-designware-core.h   |  4 ++++
>   drivers/i2c/busses/i2c-designware-master.c | 18 +++++++++++++++++-
>   2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 347843b4f5dd..91f17181ece1 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -40,6 +40,8 @@
>   
>   #define DW_IC_DATA_CMD_DAT			GENMASK(7, 0)
>   #define DW_IC_DATA_CMD_FIRST_DATA_BYTE		BIT(11)
> +#define DW_IC_DATA_CMD_STOP			BIT(9)
> +#define DW_IC_DATA_CMD_CMD			BIT(8)
>   
If you like these defines would be nice to have in another patch before 
this and convert i2c-designware-master.c to use them instead of 0x100, 
BIT(9) and BIT(10) in existing code. Makes code more uniform together 
with your change.

I'm not demanding it but wanted to give you idea if you like to do some 
additional cleanups to the code :-)

RE: [v2,1/1] i2c: designware: Add SMBus Quick Command support

[EnDe Tan]

As SMBus Quick Command support may depend on the IP version,
and given that it seems we are transitioning from `dw_apb_i2c` to `dwc_i2c` in the future,
I think I shall leave this patch here for reference in case the `dwc_i2c` driver is upstreamed later.