Re: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Biju,

On 29.06.2024 08:38, Biju Das wrote:
> Hi Claudiu,
> 
> Thanks for the patch.
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Tuesday, June 25, 2024 1:14 PM
>> Subject: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Fast mode plus is available on most of the IP variants that RIIC driver is working with. The
>> exception is (according to HW manuals of the SoCs where this IP is available) the Renesas RZ/A1H.
>> For this, patch introduces the struct riic_of_data::fast_mode_plus.
>>
>> Fast mode plus was tested on RZ/G3S, RZ/G2{L,UL,LC}, RZ/Five by instantiating the RIIC frequency to
>> 1MHz and issuing i2c reads on the fast mode plus capable devices (and the i2c clock frequency was
>> checked on RZ/G3S).
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - dropped code that handles the renesas,riic-no-fast-mode-plus
>> - updated commit description
>>
>>  drivers/i2c/busses/i2c-riic.c | 28 ++++++++++++++++++++++++----
>>  1 file changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index
>> 8ffbead95492..c07317f95e82 100644
>> --- a/drivers/i2c/busses/i2c-riic.c
>> +++ b/drivers/i2c/busses/i2c-riic.c
>> @@ -63,6 +63,8 @@
>>  #define ICMR3_ACKWP	0x10
>>  #define ICMR3_ACKBT	0x08
>>
>> +#define ICFER_FMPE	0x80
>> +
>>  #define ICIER_TIE	0x80
>>  #define ICIER_TEIE	0x40
>>  #define ICIER_RIE	0x20
>> @@ -80,6 +82,7 @@ enum riic_reg_list {
>>  	RIIC_ICCR2,
>>  	RIIC_ICMR1,
>>  	RIIC_ICMR3,
>> +	RIIC_ICFER,
>>  	RIIC_ICSER,
>>  	RIIC_ICIER,
>>  	RIIC_ICSR2,
>> @@ -92,6 +95,7 @@ enum riic_reg_list {
>>
>>  struct riic_of_data {
>>  	const u8 *regs;
>> +	bool fast_mode_plus;
>>  };
>>
>>  struct riic_dev {
>> @@ -315,11 +319,13 @@ static int riic_init_hw(struct riic_dev *riic)
>>  	int total_ticks, cks, brl, brh;
>>  	struct i2c_timings *t = &riic->i2c_t;
>>  	struct device *dev = riic->adapter.dev.parent;
>> +	const struct riic_of_data *info = riic->info;
>>
>> -	if (t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) {
>> -		dev_err(dev,
>> -			"unsupported bus speed (%dHz). %d max\n",
>> -			t->bus_freq_hz, I2C_MAX_FAST_MODE_FREQ);
>> +	if ((!info->fast_mode_plus && t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) ||
>> +	    (info->fast_mode_plus && t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ)) {
>> +		dev_err(dev, "unsupported bus speed (%dHz). %d max\n", t->bus_freq_hz,
>> +			info->fast_mode_plus ? I2C_MAX_FAST_MODE_PLUS_FREQ :
>> +			I2C_MAX_FAST_MODE_FREQ);
>>  		return -EINVAL;
>>  	}
>>
>> @@ -407,6 +413,9 @@ static int riic_init_hw(struct riic_dev *riic)
>>  	riic_writeb(riic, 0, RIIC_ICSER);
>>  	riic_writeb(riic, ICMR3_ACKWP | ICMR3_RDRFS, RIIC_ICMR3);
>>
>> +	if (info->fast_mode_plus && t->bus_freq_hz == I2C_MAX_FAST_MODE_PLUS_FREQ)
>> +		riic_clear_set_bit(riic, 0, ICFER_FMPE, RIIC_ICFER);
>> +
>>  	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
>>
>>  	pm_runtime_mark_last_busy(dev);
>> @@ -536,6 +545,7 @@ static const u8 riic_rz_a_regs[RIIC_REG_END] = {
>>  	[RIIC_ICCR2] = 0x04,
>>  	[RIIC_ICMR1] = 0x08,
>>  	[RIIC_ICMR3] = 0x10,
>> +	[RIIC_ICFER] = 0x14,
>>  	[RIIC_ICSER] = 0x18,
>>  	[RIIC_ICIER] = 0x1c,
>>  	[RIIC_ICSR2] = 0x24,
>> @@ -549,11 +559,17 @@ static const struct riic_of_data riic_rz_a_info = {
>>  	.regs = riic_rz_a_regs,
>>  };
>>
>> +static const struct riic_of_data riic_rz_g2_info = {
>> +	.regs = riic_rz_a_regs,
>> +	.fast_mode_plus = true,
>> +};
>> +
>>  static const u8 riic_rz_v2h_regs[RIIC_REG_END] = {
>>  	[RIIC_ICCR1] = 0x00,
>>  	[RIIC_ICCR2] = 0x01,
>>  	[RIIC_ICMR1] = 0x02,
>>  	[RIIC_ICMR3] = 0x04,
>> +	[RIIC_ICFER] = 0x05,
>>  	[RIIC_ICSER] = 0x06,
>>  	[RIIC_ICIER] = 0x07,
>>  	[RIIC_ICSR2] = 0x09,
>> @@ -565,6 +581,7 @@ static const u8 riic_rz_v2h_regs[RIIC_REG_END] = {
>>
>>  static const struct riic_of_data riic_rz_v2h_info = {
>>  	.regs = riic_rz_v2h_regs,
>> +	.fast_mode_plus = true,
>>  };
>>
>>  static int riic_i2c_suspend(struct device *dev) @@ -613,6 +630,9 @@ static const struct dev_pm_ops
>> riic_i2c_pm_ops = {
>>
>>  static const struct of_device_id riic_i2c_dt_ids[] = {
>>  	{ .compatible = "renesas,riic-rz", .data = &riic_rz_a_info },
>> +	{ .compatible = "renesas,riic-r9a07g043", .data =  &riic_rz_g2_info, },
>> +	{ .compatible = "renesas,riic-r9a07g044", .data =  &riic_rz_g2_info, },
>> +	{ .compatible = "renesas,riic-r9a07g054", .data =  &riic_rz_g2_info,
>> +},
> 
> I feel, the better way is 
> 
> { .compatible = "renesas, renesas,r7s72100", .data = &riic_rz_a_info },--> As this SoC does not support FMP
> { .compatible = "renesas,riic-rz", .data =  &riic_rz_g2_info, },--> As this SoCs has FMP+ support
> { .compatible = "renesas,riic-r9a09g057", .data = &riic_rz_v2h_info },--> As this SoCs has different register layout and FMP+

This is the natural way going forward if we enable it for all platforms
supporting FM+ (not only for those that could be currently tested).

If there are no comments against it I'll go with this compatible list.

Thank you,
Claudiu Beznea

> 
> With this the number of compatible entries in the device tables reduced from 5 to 3.
> 
> Cheers,
> Biju
>