| Message ID | 20240625121358.590547-10-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: riic: Add support for Renesas RZ/G3S | expand |
Hi Claudiu, On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > 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> Thanks for your patch! > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -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); Unless FM+ is specified, RIIC_ICFER is never written to. Probably the register should always be initialized, also to make sure the FMPE bit is cleared when it was set by the boot loader, but FM+ is not to be used. > + > riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1); > > pm_runtime_mark_last_busy(dev); Gr{oetje,eeting}s, Geert
On 28.06.2024 12:22, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> 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> > > Thanks for your patch! > >> --- a/drivers/i2c/busses/i2c-riic.c >> +++ b/drivers/i2c/busses/i2c-riic.c >> @@ -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); > > Unless FM+ is specified, RIIC_ICFER is never written to. > Probably the register should always be initialized, also to make sure > the FMPE bit is cleared when it was set by the boot loader, but FM+ > is not to be used. OK, that's a good point. Thank you, Claudiu Beznea > > >> + >> riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1); >> >> pm_runtime_mark_last_busy(dev); > > Gr{oetje,eeting}s, > > Geert >
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+ With this the number of compatible entries in the device tables reduced from 5 to 3. Cheers, Biju
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 >
Hi, Geert, all, On 28.06.2024 12:22, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> 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> > > Thanks for your patch! > >> --- a/drivers/i2c/busses/i2c-riic.c >> +++ b/drivers/i2c/busses/i2c-riic.c >> @@ -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); > > Unless FM+ is specified, RIIC_ICFER is never written to. > Probably the register should always be initialized, also to make sure > the FMPE bit is cleared when it was set by the boot loader, but FM+ > is not to be used. Instead of clearing only this bit, what do you think about using reset_control_reset() instead of reset_control_deassert() in riic_i2c_probe()? HW manuals for all the devices listed in Documentation/devicetree/bindings/i2c/renesas,riic.yaml specifies that ICFER_FMPE register is initialized with a default value by reset. All the other registers are initialized with default values at reset (according to HW manuals). I've checked it on RZ/G3S and it behaves like this. With this: diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index ba969ad5f015..150e7841f178 100644 --- a/drivers/i2c/busses/i2c-riic.c +++ b/drivers/i2c/busses/i2c-riic.c @@ -457,7 +457,7 @@ static int riic_i2c_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(riic->rstc), "Error: missing reset ctrl\n"); - ret = reset_control_deassert(riic->rstc); + ret = reset_control_reset(riic->rstc); if (ret) return ret; I've did basic tests (i2cdetect + i2cget with FM+ frequency) on RZ/G2{L, LC, UL}, RZ/V2L and all was good. Thank you, Claudiu Beznea > > >> + >> riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1); >> >> pm_runtime_mark_last_busy(dev); > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, On Wed, Jul 10, 2024 at 4:20 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 28.06.2024 12:22, Geert Uytterhoeven wrote: > > On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> 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> > > > > Thanks for your patch! > > > >> --- a/drivers/i2c/busses/i2c-riic.c > >> +++ b/drivers/i2c/busses/i2c-riic.c > >> @@ -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); > > > > Unless FM+ is specified, RIIC_ICFER is never written to. > > Probably the register should always be initialized, also to make sure > > the FMPE bit is cleared when it was set by the boot loader, but FM+ > > is not to be used. > > Instead of clearing only this bit, what do you think about using > reset_control_reset() instead of reset_control_deassert() in riic_i2c_probe()? > > HW manuals for all the devices listed in > Documentation/devicetree/bindings/i2c/renesas,riic.yaml specifies that > ICFER_FMPE register is initialized with a default value by reset. All the > other registers are initialized with default values at reset (according to > HW manuals). I've checked it on RZ/G3S and it behaves like this. RZ/A1 and RZ/A2M do not have reset controller support yet, so calling reset_control_reset() is a no-op on these SoCs. However, I overlooked that riic_init_hw() does an internal reset first by setting the ICCR1_IICRST bit in RIIC_ICCR1. Is that sufficient to reset the FMPE bit? Gr{oetje,eeting}s, Geert
Hi, Geert, On 11.07.2024 10:53, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Wed, Jul 10, 2024 at 4:20 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: >> On 28.06.2024 12:22, Geert Uytterhoeven wrote: >>> On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >>>> 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> >>> >>> Thanks for your patch! >>> >>>> --- a/drivers/i2c/busses/i2c-riic.c >>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>> @@ -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); >>> >>> Unless FM+ is specified, RIIC_ICFER is never written to. >>> Probably the register should always be initialized, also to make sure >>> the FMPE bit is cleared when it was set by the boot loader, but FM+ >>> is not to be used. >> >> Instead of clearing only this bit, what do you think about using >> reset_control_reset() instead of reset_control_deassert() in riic_i2c_probe()? >> >> HW manuals for all the devices listed in >> Documentation/devicetree/bindings/i2c/renesas,riic.yaml specifies that >> ICFER_FMPE register is initialized with a default value by reset. All the >> other registers are initialized with default values at reset (according to >> HW manuals). I've checked it on RZ/G3S and it behaves like this. > > RZ/A1 and RZ/A2M do not have reset controller support yet, so calling > reset_control_reset() is a no-op on these SoCs. > > However, I overlooked that riic_init_hw() does an internal reset first > by setting the ICCR1_IICRST bit in RIIC_ICCR1. > Is that sufficient to reset the FMPE bit? Yes, that also restores the content of RIIC_ICFER register to the default value (including clearing the FMPE bit). Thanks for pointing it, I overlooked it, too. Thank you, Claudiu Beznea > > Gr{oetje,eeting}s, > > Geert >
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, }, { .compatible = "renesas,riic-r9a09g057", .data = &riic_rz_v2h_info }, { /* Sentinel */ }, };