| Message ID | 20241119211215.352797-1-cbabroski@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c-mlxbf: Add repeated start condition support | expand |
Hi Chris, Can I have an ack from the other nvidia guys, please? Khalil and Asmaa? ... > @@ -694,16 +695,19 @@ static void mlxbf_i2c_smbus_read_data(struct mlxbf_i2c_priv *priv, > } > > static int mlxbf_i2c_smbus_enable(struct mlxbf_i2c_priv *priv, u8 slave, > - u8 len, u8 block_en, u8 pec_en, bool read) > + u8 len, u8 block_en, u8 pec_en, bool read, bool no_stop) I don't really like how the alignment turned out here. This file rarely exceeds 80 characters, so can we keep it that way? > { > - u32 command; > + u32 command = 0; > > /* Set Master GW control word. */ > + if (!no_stop) > + command |= MLXBF_I2C_MASTER_STOP_BIT; If "no stop" we enable the stop bit? Can we make it a bit more straight? > + ... > -/* > - * Note that the mlxbf_i2c_timings->timeout value is not related to the > - * bus frequency, it is impacted by the time it takes the driver to > - * complete data transmission before transaction abort. > - */ > +/* Timing values are in nanoseconds */ > static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = { > [MLXBF_I2C_TIMING_CONFIG_100KHZ] = { > .scl_high = 4810, > @@ -1203,8 +1218,8 @@ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = { > .scl_fall = 50, > .hold_data = 300, > .buf = 20000, > - .thigh_max = 5000, > - .timeout = 106500 > + .thigh_max = 50000, > + .timeout = 35000000 > }, > [MLXBF_I2C_TIMING_CONFIG_400KHZ] = { > .scl_high = 1011, > @@ -1219,24 +1234,24 @@ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = { > .scl_fall = 50, > .hold_data = 300, > .buf = 20000, > - .thigh_max = 5000, > - .timeout = 106500 > + .thigh_max = 50000, > + .timeout = 35000000 > }, > [MLXBF_I2C_TIMING_CONFIG_1000KHZ] = { > - .scl_high = 600, > - .scl_low = 1300, > + .scl_high = 383, > + .scl_low = 460, > .hold_start = 600, > - .setup_start = 600, > - .setup_stop = 600, > - .setup_data = 100, > + .setup_start = 260, > + .setup_stop = 260, > + .setup_data = 50, > .sda_rise = 50, > .sda_fall = 50, > .scl_rise = 50, > .scl_fall = 50, > .hold_data = 300, > - .buf = 20000, > - .thigh_max = 5000, > - .timeout = 106500 > + .buf = 500, > + .thigh_max = 50000, > + .timeout = 35000000 Can you please explain better how the stop bit affects the timing values? Thanks, Andi
> Can I have an ack from the other nvidia guys, please? Khalil and > Asmaa? I know that they have been reviewing this offline, but I need this to happen in the list. Thanks, Andi
Acked-by: Khalil Blaiech <kblaiech@nvidia.com> -----Original Message----- From: Andi Shyti <andi.shyti@kernel.org> Sent: Thursday, January 9, 2025 10:56 AM To: Chris Babroski <cbabroski@nvidia.com> Cc: Khalil Blaiech <kblaiech@nvidia.com>; Asmaa Mnebhi <asmaa@nvidia.com>; David Thompson <davthompson@nvidia.com>; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] i2c-mlxbf: Add repeated start condition support > Can I have an ack from the other nvidia guys, please? Khalil and > Asmaa? I know that they have been reviewing this offline, but I need this to happen in the list. Thanks, Andi
Acked-by: Asmaa Mnebhi <asmaa@nvidia.com> > -----Original Message----- > From: Khalil Blaiech <kblaiech@nvidia.com> > Sent: Thursday, January 9, 2025 11:00 AM > To: Andi Shyti <andi.shyti@kernel.org>; Chris Babroski > <cbabroski@nvidia.com> > Cc: Asmaa Mnebhi <asmaa@nvidia.com>; David Thompson > <davthompson@nvidia.com>; linux-i2c@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: RE: [PATCH v2] i2c-mlxbf: Add repeated start condition support > > Acked-by: Khalil Blaiech <kblaiech@nvidia.com> > > -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: Thursday, January 9, 2025 10:56 AM > To: Chris Babroski <cbabroski@nvidia.com> > Cc: Khalil Blaiech <kblaiech@nvidia.com>; Asmaa Mnebhi > <asmaa@nvidia.com>; David Thompson <davthompson@nvidia.com>; linux- > i2c@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2] i2c-mlxbf: Add repeated start condition support > > > Can I have an ack from the other nvidia guys, please? Khalil and > > Asmaa? > > I know that they have been reviewing this offline, but I need this to happen in > the list. > > Thanks, > Andi
Hi Andi, Thank you for reviewing. I will send an updated V3 patch with the changes you have requested soon. > Can you please explain better how the stop bit affects the timing > values? There were two changes needed to get I2C communication working with a specific I2C device on BlueField 3 and are contained within this patch: 1. Support repeated start conditions for SMBus I2C block read. This is implemented by removing the stop bit for the first write of a block read transaction. 2. Increase the time that the clock can be held low before an I2C transaction is aborted (the "timeout" value in the bus timing configuration). Without this change the bus will time out attempting a block read with this device. While debugging to find #2 above we noticed that the "timeout" value, along with several other timing values, did not match our ATF I2C driver which contains the latest bus timing configuration and has been fully verified by SW and HW teams. When I updated the "timeout" value I also updated any other timing values that were incorrect to ensure our Linux kernel driver uses the complete tested bus timing configuration. Testing this bug fix requires both the repeated start condition and I2C bus timing changes which is why I have included them in a single patch. If you would prefer for the timing changes to be separated into a different patch then I can do that instead. Thanks, Chris
diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c index b3a73921ab69..8926dafa0270 100644 --- a/drivers/i2c/busses/i2c-mlxbf.c +++ b/drivers/i2c/busses/i2c-mlxbf.c @@ -196,6 +196,7 @@ #define MLXBF_I2C_MASK_8 GENMASK(7, 0) #define MLXBF_I2C_MASK_16 GENMASK(15, 0) +#define MLXBF_I2C_MASK_32 GENMASK(31, 0) #define MLXBF_I2C_MST_ADDR_OFFSET 0x200 @@ -221,8 +222,7 @@ #define MLXBF_I2C_MASTER_STOP_BIT BIT(3) /* Control stop. */ #define MLXBF_I2C_MASTER_ENABLE \ - (MLXBF_I2C_MASTER_LOCK_BIT | MLXBF_I2C_MASTER_BUSY_BIT | \ - MLXBF_I2C_MASTER_START_BIT | MLXBF_I2C_MASTER_STOP_BIT) + (MLXBF_I2C_MASTER_LOCK_BIT | MLXBF_I2C_MASTER_BUSY_BIT | MLXBF_I2C_MASTER_START_BIT) #define MLXBF_I2C_MASTER_ENABLE_WRITE \ (MLXBF_I2C_MASTER_ENABLE | MLXBF_I2C_MASTER_CTL_WRITE_BIT) @@ -336,6 +336,7 @@ enum { MLXBF_I2C_F_SMBUS_BLOCK = BIT(5), MLXBF_I2C_F_SMBUS_PEC = BIT(6), MLXBF_I2C_F_SMBUS_PROCESS_CALL = BIT(7), + MLXBF_I2C_F_WRITE_WITHOUT_STOP = BIT(8), }; /* Mellanox BlueField chip type. */ @@ -694,16 +695,19 @@ static void mlxbf_i2c_smbus_read_data(struct mlxbf_i2c_priv *priv, } static int mlxbf_i2c_smbus_enable(struct mlxbf_i2c_priv *priv, u8 slave, - u8 len, u8 block_en, u8 pec_en, bool read) + u8 len, u8 block_en, u8 pec_en, bool read, bool no_stop) { - u32 command; + u32 command = 0; /* Set Master GW control word. */ + if (!no_stop) + command |= MLXBF_I2C_MASTER_STOP_BIT; + if (read) { - command = MLXBF_I2C_MASTER_ENABLE_READ; + command |= MLXBF_I2C_MASTER_ENABLE_READ; command |= rol32(len, MLXBF_I2C_MASTER_READ_SHIFT); } else { - command = MLXBF_I2C_MASTER_ENABLE_WRITE; + command |= MLXBF_I2C_MASTER_ENABLE_WRITE; command |= rol32(len, MLXBF_I2C_MASTER_WRITE_SHIFT); } command |= rol32(slave, MLXBF_I2C_MASTER_SLV_ADDR_SHIFT); @@ -738,9 +742,11 @@ mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv, u8 op_idx, data_idx, data_len, write_len, read_len; struct mlxbf_i2c_smbus_operation *operation; u8 read_en, write_en, block_en, pec_en; - u8 slave, flags, addr; + bool write_wo_stop = false; + u8 slave, addr; u8 *read_buf; int ret = 0; + u32 flags; if (request->operation_cnt > MLXBF_I2C_SMBUS_MAX_OP_CNT) return -EINVAL; @@ -799,7 +805,16 @@ mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv, memcpy(data_desc + data_idx, operation->buffer, operation->length); data_idx += operation->length; + + /* + * The stop condition can be skipped when writing on the bus + * to implement a repeated start condition on the next read + * as required for several SMBus and I2C operations. + */ + if (flags & MLXBF_I2C_F_WRITE_WITHOUT_STOP) + write_wo_stop = true; } + /* * We assume that read operations are performed only once per * SMBus transaction. *TBD* protect this statement so it won't @@ -825,7 +840,7 @@ mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv, if (write_en) { ret = mlxbf_i2c_smbus_enable(priv, slave, write_len, block_en, - pec_en, 0); + pec_en, 0, write_wo_stop); if (ret) goto out_unlock; } @@ -835,7 +850,7 @@ mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv, mlxbf_i2c_smbus_write_data(priv, (const u8 *)&addr, 1, MLXBF_I2C_MASTER_DATA_DESC_ADDR, true); ret = mlxbf_i2c_smbus_enable(priv, slave, read_len, block_en, - pec_en, 1); + pec_en, 1, false); if (!ret) { /* Get Master GW data descriptor. */ mlxbf_i2c_smbus_read_data(priv, data_desc, read_len + 1, @@ -940,6 +955,9 @@ mlxbf_i2c_smbus_i2c_block_func(struct mlxbf_i2c_smbus_request *request, request->operation[0].flags |= pec_check ? MLXBF_I2C_F_SMBUS_PEC : 0; request->operation[0].buffer = command; + if (read) + request->operation[0].flags |= MLXBF_I2C_F_WRITE_WITHOUT_STOP; + /* * As specified in the standard, the max number of bytes to read/write * per block operation is 32 bytes. In Golan code, the controller can @@ -1174,7 +1192,8 @@ static void mlxbf_i2c_set_timings(struct mlxbf_i2c_priv *priv, MLXBF_I2C_MASK_16, MLXBF_I2C_SHIFT_16); writel(timer, priv->timer->io + MLXBF_I2C_SMBUS_THIGH_MAX_TBUF); - timer = timings->timeout; + timer = mlxbf_i2c_set_timer(priv, timings->timeout, false, + MLXBF_I2C_MASK_32, MLXBF_I2C_SHIFT_0); writel(timer, priv->timer->io + MLXBF_I2C_SMBUS_SCL_LOW_TIMEOUT); } @@ -1184,11 +1203,7 @@ enum mlxbf_i2c_timings_config { MLXBF_I2C_TIMING_CONFIG_1000KHZ, }; -/* - * Note that the mlxbf_i2c_timings->timeout value is not related to the - * bus frequency, it is impacted by the time it takes the driver to - * complete data transmission before transaction abort. - */ +/* Timing values are in nanoseconds */ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = { [MLXBF_I2C_TIMING_CONFIG_100KHZ] = { .scl_high = 4810, @@ -1203,8 +1218,8 @@ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = { .scl_fall = 50, .hold_data = 300, .buf = 20000, - .thigh_max = 5000, - .timeout = 106500 + .thigh_max = 50000, + .timeout = 35000000 }, [MLXBF_I2C_TIMING_CONFIG_400KHZ] = { .scl_high = 1011, @@ -1219,24 +1234,24 @@ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = { .scl_fall = 50, .hold_data = 300, .buf = 20000, - .thigh_max = 5000, - .timeout = 106500 + .thigh_max = 50000, + .timeout = 35000000 }, [MLXBF_I2C_TIMING_CONFIG_1000KHZ] = { - .scl_high = 600, - .scl_low = 1300, + .scl_high = 383, + .scl_low = 460, .hold_start = 600, - .setup_start = 600, - .setup_stop = 600, - .setup_data = 100, + .setup_start = 260, + .setup_stop = 260, + .setup_data = 50, .sda_rise = 50, .sda_fall = 50, .scl_rise = 50, .scl_fall = 50, .hold_data = 300, - .buf = 20000, - .thigh_max = 5000, - .timeout = 106500 + .buf = 500, + .thigh_max = 50000, + .timeout = 35000000 } };
Add support for SMBus repeated start conditions to the Mellanox I2C driver. This support is specifically enabled for the I2C_FUNC_SMBUS_WRITE_I2C_BLOCK implementation which is required for communication with various I2C devices on Bluefield 3. The I2C bus timing configuration values have also been updated based on latest HW testing results and found to be necessary to support repeated start transactions. Signed-off-by: Chris Babroski <cbabroski@nvidia.com> --- V1 -> V2: Removed default "Reviewed-by:" tags drivers/i2c/busses/i2c-mlxbf.c | 69 +++++++++++++++++++++------------- 1 file changed, 42 insertions(+), 27 deletions(-) base-commit: 0a9b9d17f3a781dea03baca01c835deaa07f7cc3