Hi, Biju,
On 29.06.2024 08:38, Biju Das wrote:
> Hi Claudiu,
>
> Thanks for the patch.
>
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@xxxxxxxxx>
>> 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@xxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxx>
>> ---
>>
>> 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
>
