| Message ID | 20240625121358.590547-8-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/12] clk: renesas: r9a08g045: Add clock, reset and power domain support for I2C | expand |
Hi Claudiu, > -----Original Message----- > From: Claudiu <claudiu.beznea@tuxon.dev> > Sent: Tuesday, June 25, 2024 1:14 PM > Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Define individual arrays to describe the register offsets. In this way we can describe different IP > variants that share the same register offsets but have differences in other characteristics. Commit > prepares for the addition of fast mode plus. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - none > > drivers/i2c/busses/i2c-riic.c | 58 +++++++++++++++++++---------------- > 1 file changed, 31 insertions(+), 27 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index > 9fe007609076..8ffbead95492 100644 > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -91,7 +91,7 @@ enum riic_reg_list { > }; > > struct riic_of_data { > - u8 regs[RIIC_REG_END]; > + const u8 *regs; Since you are touching this part, can we drop struct and Use u8* as device_data instead? ie, replace const struct riic_of_data *info->const u8 *regs in struct riic_dev and use .data = riic_rz_xx_regs in of_match_table? Cheers, Biju > }; > > struct riic_dev { > @@ -531,36 +531,40 @@ static void riic_i2c_remove(struct platform_device *pdev) > pm_runtime_dont_use_autosuspend(dev); > } > > +static const u8 riic_rz_a_regs[RIIC_REG_END] = { > + [RIIC_ICCR1] = 0x00, > + [RIIC_ICCR2] = 0x04, > + [RIIC_ICMR1] = 0x08, > + [RIIC_ICMR3] = 0x10, > + [RIIC_ICSER] = 0x18, > + [RIIC_ICIER] = 0x1c, > + [RIIC_ICSR2] = 0x24, > + [RIIC_ICBRL] = 0x34, > + [RIIC_ICBRH] = 0x38, > + [RIIC_ICDRT] = 0x3c, > + [RIIC_ICDRR] = 0x40, > +}; > + > static const struct riic_of_data riic_rz_a_info = { > - .regs = { > - [RIIC_ICCR1] = 0x00, > - [RIIC_ICCR2] = 0x04, > - [RIIC_ICMR1] = 0x08, > - [RIIC_ICMR3] = 0x10, > - [RIIC_ICSER] = 0x18, > - [RIIC_ICIER] = 0x1c, > - [RIIC_ICSR2] = 0x24, > - [RIIC_ICBRL] = 0x34, > - [RIIC_ICBRH] = 0x38, > - [RIIC_ICDRT] = 0x3c, > - [RIIC_ICDRR] = 0x40, > - }, > + .regs = riic_rz_a_regs, > +}; > + > +static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { > + [RIIC_ICCR1] = 0x00, > + [RIIC_ICCR2] = 0x01, > + [RIIC_ICMR1] = 0x02, > + [RIIC_ICMR3] = 0x04, > + [RIIC_ICSER] = 0x06, > + [RIIC_ICIER] = 0x07, > + [RIIC_ICSR2] = 0x09, > + [RIIC_ICBRL] = 0x10, > + [RIIC_ICBRH] = 0x11, > + [RIIC_ICDRT] = 0x12, > + [RIIC_ICDRR] = 0x13, > }; > > static const struct riic_of_data riic_rz_v2h_info = { > - .regs = { > - [RIIC_ICCR1] = 0x00, > - [RIIC_ICCR2] = 0x01, > - [RIIC_ICMR1] = 0x02, > - [RIIC_ICMR3] = 0x04, > - [RIIC_ICSER] = 0x06, > - [RIIC_ICIER] = 0x07, > - [RIIC_ICSR2] = 0x09, > - [RIIC_ICBRL] = 0x10, > - [RIIC_ICBRH] = 0x11, > - [RIIC_ICDRT] = 0x12, > - [RIIC_ICDRR] = 0x13, > - }, > + .regs = riic_rz_v2h_regs, > }; > > static int riic_i2c_suspend(struct device *dev) > -- > 2.39.2 >
Hi, Biju, On 28.06.2024 08:59, Biju Das wrote: > Hi Claudiu, > >> -----Original Message----- >> From: Claudiu <claudiu.beznea@tuxon.dev> >> Sent: Tuesday, June 25, 2024 1:14 PM >> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets >> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Define individual arrays to describe the register offsets. In this way we can describe different IP >> variants that share the same register offsets but have differences in other characteristics. Commit >> prepares for the addition of fast mode plus. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v2: >> - none >> >> drivers/i2c/busses/i2c-riic.c | 58 +++++++++++++++++++---------------- >> 1 file changed, 31 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index >> 9fe007609076..8ffbead95492 100644 >> --- a/drivers/i2c/busses/i2c-riic.c >> +++ b/drivers/i2c/busses/i2c-riic.c >> @@ -91,7 +91,7 @@ enum riic_reg_list { >> }; >> >> struct riic_of_data { >> - u8 regs[RIIC_REG_END]; >> + const u8 *regs; > > > Since you are touching this part, can we drop struct and > Use u8* as device_data instead? Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct riic_of_data. That new member is needed to differentiate b/w hardware versions supporting fast mode plus based on compatible. Keeping struct riic_of_data is necessary (unless I misunderstood your proposal). Thank you, Claudiu Beznea > > ie, replace const struct riic_of_data *info->const u8 *regs in struct riic_dev > and use .data = riic_rz_xx_regs in of_match_table? > > Cheers, > Biju >> }; >> >> struct riic_dev { >> @@ -531,36 +531,40 @@ static void riic_i2c_remove(struct platform_device *pdev) >> pm_runtime_dont_use_autosuspend(dev); >> } >> >> +static const u8 riic_rz_a_regs[RIIC_REG_END] = { >> + [RIIC_ICCR1] = 0x00, >> + [RIIC_ICCR2] = 0x04, >> + [RIIC_ICMR1] = 0x08, >> + [RIIC_ICMR3] = 0x10, >> + [RIIC_ICSER] = 0x18, >> + [RIIC_ICIER] = 0x1c, >> + [RIIC_ICSR2] = 0x24, >> + [RIIC_ICBRL] = 0x34, >> + [RIIC_ICBRH] = 0x38, >> + [RIIC_ICDRT] = 0x3c, >> + [RIIC_ICDRR] = 0x40, >> +}; >> + >> static const struct riic_of_data riic_rz_a_info = { >> - .regs = { >> - [RIIC_ICCR1] = 0x00, >> - [RIIC_ICCR2] = 0x04, >> - [RIIC_ICMR1] = 0x08, >> - [RIIC_ICMR3] = 0x10, >> - [RIIC_ICSER] = 0x18, >> - [RIIC_ICIER] = 0x1c, >> - [RIIC_ICSR2] = 0x24, >> - [RIIC_ICBRL] = 0x34, >> - [RIIC_ICBRH] = 0x38, >> - [RIIC_ICDRT] = 0x3c, >> - [RIIC_ICDRR] = 0x40, >> - }, >> + .regs = riic_rz_a_regs, >> +}; >> + >> +static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { >> + [RIIC_ICCR1] = 0x00, >> + [RIIC_ICCR2] = 0x01, >> + [RIIC_ICMR1] = 0x02, >> + [RIIC_ICMR3] = 0x04, >> + [RIIC_ICSER] = 0x06, >> + [RIIC_ICIER] = 0x07, >> + [RIIC_ICSR2] = 0x09, >> + [RIIC_ICBRL] = 0x10, >> + [RIIC_ICBRH] = 0x11, >> + [RIIC_ICDRT] = 0x12, >> + [RIIC_ICDRR] = 0x13, >> }; >> >> static const struct riic_of_data riic_rz_v2h_info = { >> - .regs = { >> - [RIIC_ICCR1] = 0x00, >> - [RIIC_ICCR2] = 0x01, >> - [RIIC_ICMR1] = 0x02, >> - [RIIC_ICMR3] = 0x04, >> - [RIIC_ICSER] = 0x06, >> - [RIIC_ICIER] = 0x07, >> - [RIIC_ICSR2] = 0x09, >> - [RIIC_ICBRL] = 0x10, >> - [RIIC_ICBRH] = 0x11, >> - [RIIC_ICDRT] = 0x12, >> - [RIIC_ICDRR] = 0x13, >> - }, >> + .regs = riic_rz_v2h_regs, >> }; >> >> static int riic_i2c_suspend(struct device *dev) >> -- >> 2.39.2 >> >
Hi Claudiu, > -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > Sent: Friday, June 28, 2024 8:32 AM > Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > Hi, Biju, > > On 28.06.2024 08:59, Biju Das wrote: > > Hi Claudiu, > > > >> -----Original Message----- > >> From: Claudiu <claudiu.beznea@tuxon.dev> > >> Sent: Tuesday, June 25, 2024 1:14 PM > >> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >> describe the register offsets > >> > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> Define individual arrays to describe the register offsets. In this > >> way we can describe different IP variants that share the same > >> register offsets but have differences in other characteristics. Commit prepares for the addition > of fast mode plus. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> --- > >> > >> Changes in v2: > >> - none > >> > >> drivers/i2c/busses/i2c-riic.c | 58 > >> +++++++++++++++++++---------------- > >> 1 file changed, 31 insertions(+), 27 deletions(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-riic.c > >> b/drivers/i2c/busses/i2c-riic.c index > >> 9fe007609076..8ffbead95492 100644 > >> --- a/drivers/i2c/busses/i2c-riic.c > >> +++ b/drivers/i2c/busses/i2c-riic.c > >> @@ -91,7 +91,7 @@ enum riic_reg_list { }; > >> > >> struct riic_of_data { > >> - u8 regs[RIIC_REG_END]; > >> + const u8 *regs; > > > > > > Since you are touching this part, can we drop struct and Use u8* as > > device_data instead? > > Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct riic_of_data. > That new member is needed to differentiate b/w hardware versions supporting fast mode plus based on > compatible. Are we sure RZ/A does not support fast mode plus? I haven't checked the H/W manual? If it does not, then it make sense to keep the patch as it is. Cheers, Biju > > Keeping struct riic_of_data is necessary (unless I misunderstood your proposal). > > Thank you, > Claudiu Beznea > > > > > ie, replace const struct riic_of_data *info->const u8 *regs in struct > > riic_dev and use .data = riic_rz_xx_regs in of_match_table? > > > > Cheers, > > Biju > >> }; > >> > >> struct riic_dev { > >> @@ -531,36 +531,40 @@ static void riic_i2c_remove(struct platform_device *pdev) > >> pm_runtime_dont_use_autosuspend(dev); > >> } > >> > >> +static const u8 riic_rz_a_regs[RIIC_REG_END] = { > >> + [RIIC_ICCR1] = 0x00, > >> + [RIIC_ICCR2] = 0x04, > >> + [RIIC_ICMR1] = 0x08, > >> + [RIIC_ICMR3] = 0x10, > >> + [RIIC_ICSER] = 0x18, > >> + [RIIC_ICIER] = 0x1c, > >> + [RIIC_ICSR2] = 0x24, > >> + [RIIC_ICBRL] = 0x34, > >> + [RIIC_ICBRH] = 0x38, > >> + [RIIC_ICDRT] = 0x3c, > >> + [RIIC_ICDRR] = 0x40, > >> +}; > >> + > >> static const struct riic_of_data riic_rz_a_info = { > >> - .regs = { > >> - [RIIC_ICCR1] = 0x00, > >> - [RIIC_ICCR2] = 0x04, > >> - [RIIC_ICMR1] = 0x08, > >> - [RIIC_ICMR3] = 0x10, > >> - [RIIC_ICSER] = 0x18, > >> - [RIIC_ICIER] = 0x1c, > >> - [RIIC_ICSR2] = 0x24, > >> - [RIIC_ICBRL] = 0x34, > >> - [RIIC_ICBRH] = 0x38, > >> - [RIIC_ICDRT] = 0x3c, > >> - [RIIC_ICDRR] = 0x40, > >> - }, > >> + .regs = riic_rz_a_regs, > >> +}; > >> + > >> +static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { > >> + [RIIC_ICCR1] = 0x00, > >> + [RIIC_ICCR2] = 0x01, > >> + [RIIC_ICMR1] = 0x02, > >> + [RIIC_ICMR3] = 0x04, > >> + [RIIC_ICSER] = 0x06, > >> + [RIIC_ICIER] = 0x07, > >> + [RIIC_ICSR2] = 0x09, > >> + [RIIC_ICBRL] = 0x10, > >> + [RIIC_ICBRH] = 0x11, > >> + [RIIC_ICDRT] = 0x12, > >> + [RIIC_ICDRR] = 0x13, > >> }; > >> > >> static const struct riic_of_data riic_rz_v2h_info = { > >> - .regs = { > >> - [RIIC_ICCR1] = 0x00, > >> - [RIIC_ICCR2] = 0x01, > >> - [RIIC_ICMR1] = 0x02, > >> - [RIIC_ICMR3] = 0x04, > >> - [RIIC_ICSER] = 0x06, > >> - [RIIC_ICIER] = 0x07, > >> - [RIIC_ICSR2] = 0x09, > >> - [RIIC_ICBRL] = 0x10, > >> - [RIIC_ICBRH] = 0x11, > >> - [RIIC_ICDRT] = 0x12, > >> - [RIIC_ICDRR] = 0x13, > >> - }, > >> + .regs = riic_rz_v2h_regs, > >> }; > >> > >> static int riic_i2c_suspend(struct device *dev) > >> -- > >> 2.39.2 > >> > >
On 28.06.2024 10:55, Biju Das wrote: > Hi Claudiu, > >> -----Original Message----- >> From: claudiu beznea <claudiu.beznea@tuxon.dev> >> Sent: Friday, June 28, 2024 8:32 AM >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets >> >> Hi, Biju, >> >> On 28.06.2024 08:59, Biju Das wrote: >>> Hi Claudiu, >>> >>>> -----Original Message----- >>>> From: Claudiu <claudiu.beznea@tuxon.dev> >>>> Sent: Tuesday, June 25, 2024 1:14 PM >>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>> describe the register offsets >>>> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> Define individual arrays to describe the register offsets. In this >>>> way we can describe different IP variants that share the same >>>> register offsets but have differences in other characteristics. Commit prepares for the addition >> of fast mode plus. >>>> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> --- >>>> >>>> Changes in v2: >>>> - none >>>> >>>> drivers/i2c/busses/i2c-riic.c | 58 >>>> +++++++++++++++++++---------------- >>>> 1 file changed, 31 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-riic.c >>>> b/drivers/i2c/busses/i2c-riic.c index >>>> 9fe007609076..8ffbead95492 100644 >>>> --- a/drivers/i2c/busses/i2c-riic.c >>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; >>>> >>>> struct riic_of_data { >>>> - u8 regs[RIIC_REG_END]; >>>> + const u8 *regs; >>> >>> >>> Since you are touching this part, can we drop struct and Use u8* as >>> device_data instead? >> >> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct riic_of_data. >> That new member is needed to differentiate b/w hardware versions supporting fast mode plus based on >> compatible. > > Are we sure RZ/A does not support fast mode plus?
On 28.06.2024 11:02, claudiu beznea wrote: > > > On 28.06.2024 10:55, Biju Das wrote: >> Hi Claudiu, >> >>> -----Original Message----- >>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>> Sent: Friday, June 28, 2024 8:32 AM >>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets >>> >>> Hi, Biju, >>> >>> On 28.06.2024 08:59, Biju Das wrote: >>>> Hi Claudiu, >>>> >>>>> -----Original Message----- >>>>> From: Claudiu <claudiu.beznea@tuxon.dev> >>>>> Sent: Tuesday, June 25, 2024 1:14 PM >>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>>> describe the register offsets >>>>> >>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>> >>>>> Define individual arrays to describe the register offsets. In this >>>>> way we can describe different IP variants that share the same >>>>> register offsets but have differences in other characteristics. Commit prepares for the addition >>> of fast mode plus. >>>>> >>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>> --- >>>>> >>>>> Changes in v2: >>>>> - none >>>>> >>>>> drivers/i2c/busses/i2c-riic.c | 58 >>>>> +++++++++++++++++++---------------- >>>>> 1 file changed, 31 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/drivers/i2c/busses/i2c-riic.c >>>>> b/drivers/i2c/busses/i2c-riic.c index >>>>> 9fe007609076..8ffbead95492 100644 >>>>> --- a/drivers/i2c/busses/i2c-riic.c >>>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; >>>>> >>>>> struct riic_of_data { >>>>> - u8 regs[RIIC_REG_END]; >>>>> + const u8 *regs; >>>> >>>> >>>> Since you are touching this part, can we drop struct and Use u8* as >>>> device_data instead? >>> >>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct riic_of_data. >>> That new member is needed to differentiate b/w hardware versions supporting fast mode plus based on >>> compatible. >> >> Are we sure RZ/A does not support fast mode plus? > > From commit description of patch 09/12: > > 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. > > I checked the manuals of all the SoCs where this driver is used. > > I haven't checked the H/W manual? That's Biju's previous statement. Sorry for not formatting it properly. > > On the manual I've downloaded from Renesas web site the FMPE bit of > RIICnFER is not available on RZ/A1H. > > Thank you, > Claudiu Beznea > >> If it does not, then it make sense to keep the patch as it is. >> >> Cheers, >> Biju >> >>> >>> Keeping struct riic_of_data is necessary (unless I misunderstood your proposal). >>> >>> Thank you, >>> Claudiu Beznea >>> >>>> >>>> ie, replace const struct riic_of_data *info->const u8 *regs in struct >>>> riic_dev and use .data = riic_rz_xx_regs in of_match_table? >>>> >>>> Cheers, >>>> Biju >>>>> }; >>>>> >>>>> struct riic_dev { >>>>> @@ -531,36 +531,40 @@ static void riic_i2c_remove(struct platform_device *pdev) >>>>> pm_runtime_dont_use_autosuspend(dev); >>>>> } >>>>> >>>>> +static const u8 riic_rz_a_regs[RIIC_REG_END] = { >>>>> + [RIIC_ICCR1] = 0x00, >>>>> + [RIIC_ICCR2] = 0x04, >>>>> + [RIIC_ICMR1] = 0x08, >>>>> + [RIIC_ICMR3] = 0x10, >>>>> + [RIIC_ICSER] = 0x18, >>>>> + [RIIC_ICIER] = 0x1c, >>>>> + [RIIC_ICSR2] = 0x24, >>>>> + [RIIC_ICBRL] = 0x34, >>>>> + [RIIC_ICBRH] = 0x38, >>>>> + [RIIC_ICDRT] = 0x3c, >>>>> + [RIIC_ICDRR] = 0x40, >>>>> +}; >>>>> + >>>>> static const struct riic_of_data riic_rz_a_info = { >>>>> - .regs = { >>>>> - [RIIC_ICCR1] = 0x00, >>>>> - [RIIC_ICCR2] = 0x04, >>>>> - [RIIC_ICMR1] = 0x08, >>>>> - [RIIC_ICMR3] = 0x10, >>>>> - [RIIC_ICSER] = 0x18, >>>>> - [RIIC_ICIER] = 0x1c, >>>>> - [RIIC_ICSR2] = 0x24, >>>>> - [RIIC_ICBRL] = 0x34, >>>>> - [RIIC_ICBRH] = 0x38, >>>>> - [RIIC_ICDRT] = 0x3c, >>>>> - [RIIC_ICDRR] = 0x40, >>>>> - }, >>>>> + .regs = riic_rz_a_regs, >>>>> +}; >>>>> + >>>>> +static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { >>>>> + [RIIC_ICCR1] = 0x00, >>>>> + [RIIC_ICCR2] = 0x01, >>>>> + [RIIC_ICMR1] = 0x02, >>>>> + [RIIC_ICMR3] = 0x04, >>>>> + [RIIC_ICSER] = 0x06, >>>>> + [RIIC_ICIER] = 0x07, >>>>> + [RIIC_ICSR2] = 0x09, >>>>> + [RIIC_ICBRL] = 0x10, >>>>> + [RIIC_ICBRH] = 0x11, >>>>> + [RIIC_ICDRT] = 0x12, >>>>> + [RIIC_ICDRR] = 0x13, >>>>> }; >>>>> >>>>> static const struct riic_of_data riic_rz_v2h_info = { >>>>> - .regs = { >>>>> - [RIIC_ICCR1] = 0x00, >>>>> - [RIIC_ICCR2] = 0x01, >>>>> - [RIIC_ICMR1] = 0x02, >>>>> - [RIIC_ICMR3] = 0x04, >>>>> - [RIIC_ICSER] = 0x06, >>>>> - [RIIC_ICIER] = 0x07, >>>>> - [RIIC_ICSR2] = 0x09, >>>>> - [RIIC_ICBRL] = 0x10, >>>>> - [RIIC_ICBRH] = 0x11, >>>>> - [RIIC_ICDRT] = 0x12, >>>>> - [RIIC_ICDRR] = 0x13, >>>>> - }, >>>>> + .regs = riic_rz_v2h_regs, >>>>> }; >>>>> >>>>> static int riic_i2c_suspend(struct device *dev) >>>>> -- >>>>> 2.39.2 >>>>> >>>>
Hi Claudiu, > -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > Sent: Friday, June 28, 2024 9:03 AM > Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > > > On 28.06.2024 10:55, Biju Das wrote: > > Hi Claudiu, > > > >> -----Original Message----- > >> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >> Sent: Friday, June 28, 2024 8:32 AM > >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >> describe the register offsets > >> > >> Hi, Biju, > >> > >> On 28.06.2024 08:59, Biju Das wrote: > >>> Hi Claudiu, > >>> > >>>> -----Original Message----- > >>>> From: Claudiu <claudiu.beznea@tuxon.dev> > >>>> Sent: Tuesday, June 25, 2024 1:14 PM > >>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >>>> describe the register offsets > >>>> > >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>> > >>>> Define individual arrays to describe the register offsets. In this > >>>> way we can describe different IP variants that share the same > >>>> register offsets but have differences in other characteristics. > >>>> Commit prepares for the addition > >> of fast mode plus. > >>>> > >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>> --- > >>>> > >>>> Changes in v2: > >>>> - none > >>>> > >>>> drivers/i2c/busses/i2c-riic.c | 58 > >>>> +++++++++++++++++++---------------- > >>>> 1 file changed, 31 insertions(+), 27 deletions(-) > >>>> > >>>> diff --git a/drivers/i2c/busses/i2c-riic.c > >>>> b/drivers/i2c/busses/i2c-riic.c index > >>>> 9fe007609076..8ffbead95492 100644 > >>>> --- a/drivers/i2c/busses/i2c-riic.c > >>>> +++ b/drivers/i2c/busses/i2c-riic.c > >>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; > >>>> > >>>> struct riic_of_data { > >>>> - u8 regs[RIIC_REG_END]; > >>>> + const u8 *regs; > >>> > >>> > >>> Since you are touching this part, can we drop struct and Use u8* as > >>> device_data instead? > >> > >> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct > riic_of_data. > >> That new member is needed to differentiate b/w hardware versions > >> supporting fast mode plus based on compatible. > > > > Are we sure RZ/A does not support fast mode plus? > > From commit description of patch 09/12: > > 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. > > I checked the manuals of all the SoCs where this driver is used. > > I haven't checked the H/W manual? > > On the manual I've downloaded from Renesas web site the FMPE bit of RIICnFER is not available on > RZ/A1H. I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. Wolfram tested it with r7s72100 genmai board acessing an eeprom. Not sure is it RZ/A1 or RZ/A2? Cheers, Biju
On 28.06.2024 11:09, Biju Das wrote: > > Hi Claudiu, > >> -----Original Message----- >> From: claudiu beznea <claudiu.beznea@tuxon.dev> >> Sent: Friday, June 28, 2024 9:03 AM >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets >> >> >> >> On 28.06.2024 10:55, Biju Das wrote: >>> Hi Claudiu, >>> >>>> -----Original Message----- >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>> Sent: Friday, June 28, 2024 8:32 AM >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>> describe the register offsets >>>> >>>> Hi, Biju, >>>> >>>> On 28.06.2024 08:59, Biju Das wrote: >>>>> Hi Claudiu, >>>>> >>>>>> -----Original Message----- >>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> >>>>>> Sent: Tuesday, June 25, 2024 1:14 PM >>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>>>> describe the register offsets >>>>>> >>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>>> >>>>>> Define individual arrays to describe the register offsets. In this >>>>>> way we can describe different IP variants that share the same >>>>>> register offsets but have differences in other characteristics. >>>>>> Commit prepares for the addition >>>> of fast mode plus. >>>>>> >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>>> --- >>>>>> >>>>>> Changes in v2: >>>>>> - none >>>>>> >>>>>> drivers/i2c/busses/i2c-riic.c | 58 >>>>>> +++++++++++++++++++---------------- >>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) >>>>>> >>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c >>>>>> b/drivers/i2c/busses/i2c-riic.c index >>>>>> 9fe007609076..8ffbead95492 100644 >>>>>> --- a/drivers/i2c/busses/i2c-riic.c >>>>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; >>>>>> >>>>>> struct riic_of_data { >>>>>> - u8 regs[RIIC_REG_END]; >>>>>> + const u8 *regs; >>>>> >>>>> >>>>> Since you are touching this part, can we drop struct and Use u8* as >>>>> device_data instead? >>>> >>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct >> riic_of_data. >>>> That new member is needed to differentiate b/w hardware versions >>>> supporting fast mode plus based on compatible. >>> >>> Are we sure RZ/A does not support fast mode plus? >> >> From commit description of patch 09/12: >> >> 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. >> >> I checked the manuals of all the SoCs where this driver is used. >> >> I haven't checked the H/W manual? >> >> On the manual I've downloaded from Renesas web site the FMPE bit of RIICnFER is not available on >> RZ/A1H. > > I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > Wolfram tested it with r7s72100 genmai board acessing an eeprom. Not sure is it RZ/A1 or RZ/A2? > > Cheers, > Biju >
Hi Claudiu, > -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > Sent: Friday, June 28, 2024 9:13 AM > Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > > > On 28.06.2024 11:09, Biju Das wrote: > > > > Hi Claudiu, > > > >> -----Original Message----- > >> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >> Sent: Friday, June 28, 2024 9:03 AM > >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >> describe the register offsets > >> > >> > >> > >> On 28.06.2024 10:55, Biju Das wrote: > >>> Hi Claudiu, > >>> > >>>> -----Original Message----- > >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>> Sent: Friday, June 28, 2024 8:32 AM > >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > >>>> to describe the register offsets > >>>> > >>>> Hi, Biju, > >>>> > >>>> On 28.06.2024 08:59, Biju Das wrote: > >>>>> Hi Claudiu, > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> > >>>>>> Sent: Tuesday, June 25, 2024 1:14 PM > >>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >>>>>> describe the register offsets > >>>>>> > >>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>>>> > >>>>>> Define individual arrays to describe the register offsets. In > >>>>>> this way we can describe different IP variants that share the > >>>>>> same register offsets but have differences in other characteristics. > >>>>>> Commit prepares for the addition > >>>> of fast mode plus. > >>>>>> > >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>>>> --- > >>>>>> > >>>>>> Changes in v2: > >>>>>> - none > >>>>>> > >>>>>> drivers/i2c/busses/i2c-riic.c | 58 > >>>>>> +++++++++++++++++++---------------- > >>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c > >>>>>> b/drivers/i2c/busses/i2c-riic.c index > >>>>>> 9fe007609076..8ffbead95492 100644 > >>>>>> --- a/drivers/i2c/busses/i2c-riic.c > >>>>>> +++ b/drivers/i2c/busses/i2c-riic.c > >>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; > >>>>>> > >>>>>> struct riic_of_data { > >>>>>> - u8 regs[RIIC_REG_END]; > >>>>>> + const u8 *regs; > >>>>> > >>>>> > >>>>> Since you are touching this part, can we drop struct and Use u8* > >>>>> as device_data instead? > >>>> > >>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new > >>>> member to struct > >> riic_of_data. > >>>> That new member is needed to differentiate b/w hardware versions > >>>> supporting fast mode plus based on compatible. > >>> > >>> Are we sure RZ/A does not support fast mode plus? > >> > >> From commit description of patch 09/12: > >> > >> 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. > >> > >> I checked the manuals of all the SoCs where this driver is used. > >> > >> I haven't checked the H/W manual? > >> > >> On the manual I've downloaded from Renesas web site the FMPE bit of > >> RIICnFER is not available on RZ/A1H. > > > > I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > > I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. Maybe make the register layout as per SoC RZ/A1 --> &riic_rz_a_info RZ/A2 and RZ/{G2L,G2LC,V2L,G2UL,FIVE} --> &riic_rz_g2_info RZ/G3S and RZ/V2H --> &riic_rz_v2h_info Then except RZ/A1, set FMP. Currently register layout of RZ/A2 is not matching with Hardware manual. Cheers, Biju
Hi Caludiu, > -----Original Message----- > From: Biju Das <biju.das.jz@bp.renesas.com> > Sent: Friday, June 28, 2024 9:24 AM > Subject: RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > Hi Claudiu, > > > -----Original Message----- > > From: claudiu beznea <claudiu.beznea@tuxon.dev> > > Sent: Friday, June 28, 2024 9:13 AM > > Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to > > describe the register offsets > > > > > > > > On 28.06.2024 11:09, Biju Das wrote: > > > > > > Hi Claudiu, > > > > > >> -----Original Message----- > > >> From: claudiu beznea <claudiu.beznea@tuxon.dev> > > >> Sent: Friday, June 28, 2024 9:03 AM > > >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > > >> to describe the register offsets > > >> > > >> > > >> > > >> On 28.06.2024 10:55, Biju Das wrote: > > >>> Hi Claudiu, > > >>> > > >>>> -----Original Message----- > > >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > > >>>> Sent: Friday, June 28, 2024 8:32 AM > > >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > > >>>> to describe the register offsets > > >>>> > > >>>> Hi, Biju, > > >>>> > > >>>> On 28.06.2024 08:59, Biju Das wrote: > > >>>>> Hi Claudiu, > > >>>>> > > >>>>>> -----Original Message----- > > >>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> > > >>>>>> Sent: Tuesday, June 25, 2024 1:14 PM > > >>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays > > >>>>>> to describe the register offsets > > >>>>>> > > >>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > >>>>>> > > >>>>>> Define individual arrays to describe the register offsets. In > > >>>>>> this way we can describe different IP variants that share the > > >>>>>> same register offsets but have differences in other characteristics. > > >>>>>> Commit prepares for the addition > > >>>> of fast mode plus. > > >>>>>> > > >>>>>> Signed-off-by: Claudiu Beznea > > >>>>>> <claudiu.beznea.uj@bp.renesas.com> > > >>>>>> --- > > >>>>>> > > >>>>>> Changes in v2: > > >>>>>> - none > > >>>>>> > > >>>>>> drivers/i2c/busses/i2c-riic.c | 58 > > >>>>>> +++++++++++++++++++---------------- > > >>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) > > >>>>>> > > >>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c > > >>>>>> b/drivers/i2c/busses/i2c-riic.c index > > >>>>>> 9fe007609076..8ffbead95492 100644 > > >>>>>> --- a/drivers/i2c/busses/i2c-riic.c > > >>>>>> +++ b/drivers/i2c/busses/i2c-riic.c > > >>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; > > >>>>>> > > >>>>>> struct riic_of_data { > > >>>>>> - u8 regs[RIIC_REG_END]; > > >>>>>> + const u8 *regs; > > >>>>> > > >>>>> > > >>>>> Since you are touching this part, can we drop struct and Use u8* > > >>>>> as device_data instead? > > >>>> > > >>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a > > >>>> new member to struct > > >> riic_of_data. > > >>>> That new member is needed to differentiate b/w hardware versions > > >>>> supporting fast mode plus based on compatible. > > >>> > > >>> Are we sure RZ/A does not support fast mode plus? > > >> > > >> From commit description of patch 09/12: > > >> > > >> 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. > > >> > > >> I checked the manuals of all the SoCs where this driver is used. > > >> > > >> I haven't checked the H/W manual? > > >> > > >> On the manual I've downloaded from Renesas web site the FMPE bit of > > >> RIICnFER is not available on RZ/A1H. > > > > > > I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > > > > I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > > Maybe make the register layout as per SoC > > RZ/A1 --> &riic_rz_a_info > RZ/A2 and RZ/{G2L,G2LC,V2L,G2UL,FIVE} --> &riic_rz_g2_info RZ/G3S and RZ/V2H --> &riic_rz_v2h_info > > Then except RZ/A1, set FMP. > > Currently register layout of RZ/A2 is not matching with Hardware manual. One more thing, From register layout, you can detect SOC has FMP capability or not So you don’t need riic_of_data::fast_mode_plus. Cheers, Biju
Hi Biju, On Fri, Jun 28, 2024 at 10:09 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > -----Original Message----- > > From: claudiu beznea <claudiu.beznea@tuxon.dev> > > On 28.06.2024 10:55, Biju Das wrote: > > > Are we sure RZ/A does not support fast mode plus? > > > > From commit description of patch 09/12: > > > > 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. > > > > I checked the manuals of all the SoCs where this driver is used. > > > > I haven't checked the H/W manual? > > > > On the manual I've downloaded from Renesas web site the FMPE bit of RIICnFER is not available on > > RZ/A1H. > > I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > Wolfram tested it with r7s72100 genmai board acessing an eeprom. Not sure is it RZ/A1 or RZ/A2? Genmai is RZ/A1H (r7s72100). Gr{oetje,eeting}s, Geert
Hi Claudiu, On Fri, Jun 28, 2024 at 10:12 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 28.06.2024 11:09, Biju Das wrote: > >> -----Original Message----- > >> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >> Sent: Friday, June 28, 2024 9:03 AM > >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > >> > >> > >> > >> On 28.06.2024 10:55, Biju Das wrote: > >>> Hi Claudiu, > >>> > >>>> -----Original Message----- > >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>> Sent: Friday, June 28, 2024 8:32 AM > >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >>>> describe the register offsets > >>>> > >>>> Hi, Biju, > >>>> > >>>> On 28.06.2024 08:59, Biju Das wrote: > >>>>> Hi Claudiu, > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> > >>>>>> Sent: Tuesday, June 25, 2024 1:14 PM > >>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >>>>>> describe the register offsets > >>>>>> > >>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>>>> > >>>>>> Define individual arrays to describe the register offsets. In this > >>>>>> way we can describe different IP variants that share the same > >>>>>> register offsets but have differences in other characteristics. > >>>>>> Commit prepares for the addition > >>>> of fast mode plus. > >>>>>> > >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>>>> --- > >>>>>> > >>>>>> Changes in v2: > >>>>>> - none > >>>>>> > >>>>>> drivers/i2c/busses/i2c-riic.c | 58 > >>>>>> +++++++++++++++++++---------------- > >>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c > >>>>>> b/drivers/i2c/busses/i2c-riic.c index > >>>>>> 9fe007609076..8ffbead95492 100644 > >>>>>> --- a/drivers/i2c/busses/i2c-riic.c > >>>>>> +++ b/drivers/i2c/busses/i2c-riic.c > >>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; > >>>>>> > >>>>>> struct riic_of_data { > >>>>>> - u8 regs[RIIC_REG_END]; > >>>>>> + const u8 *regs; > >>>>> > >>>>> > >>>>> Since you are touching this part, can we drop struct and Use u8* as > >>>>> device_data instead? > >>>> > >>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct > >> riic_of_data. > >>>> That new member is needed to differentiate b/w hardware versions > >>>> supporting fast mode plus based on compatible. > >>> > >>> Are we sure RZ/A does not support fast mode plus? > >> > >> From commit description of patch 09/12: > >> > >> 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. > >> > >> I checked the manuals of all the SoCs where this driver is used. > >> > >> I haven't checked the H/W manual? > >> > >> On the manual I've downloaded from Renesas web site the FMPE bit of RIICnFER is not available on > >> RZ/A1H. > > > > I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > > I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. Do you need to check for that? The ICFER_FMPE bit won't be set unless the user specifies the FM+ clock-frequency. Setting clock-frequency beyond Fast Mode on RZ/A1H would be very wrong. Gr{oetje,eeting}s, Geert
Hi Geert, > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Friday, June 28, 2024 10:09 AM > Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > Hi Biju, > > On Fri, Jun 28, 2024 at 10:09 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > -----Original Message----- > > > From: claudiu beznea <claudiu.beznea@tuxon.dev> On 28.06.2024 10:55, > > > Biju Das wrote: > > > > Are we sure RZ/A does not support fast mode plus? > > > > > > From commit description of patch 09/12: > > > > > > 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. > > > > > > I checked the manuals of all the SoCs where this driver is used. > > > > > > I haven't checked the H/W manual? > > > > > > On the manual I've downloaded from Renesas web site the FMPE bit of > > > RIICnFER is not available on RZ/A1H. > > > > I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > > Wolfram tested it with r7s72100 genmai board acessing an eeprom. Not sure is it RZ/A1 or RZ/A2? > > Genmai is RZ/A1H (r7s72100). Thanks for the information. So RZ/A1 is the odd one, which does not have FMP capability, while others have. Cheers, Biju
On 28.06.2024 11:24, Biju Das wrote: > Hi Claudiu, > >> -----Original Message----- >> From: claudiu beznea <claudiu.beznea@tuxon.dev> >> Sent: Friday, June 28, 2024 9:13 AM >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets >> >> >> >> On 28.06.2024 11:09, Biju Das wrote: >>> >>> Hi Claudiu, >>> >>>> -----Original Message----- >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>> Sent: Friday, June 28, 2024 9:03 AM >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>> describe the register offsets >>>> >>>> >>>> >>>> On 28.06.2024 10:55, Biju Das wrote: >>>>> Hi Claudiu, >>>>> >>>>>> -----Original Message----- >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>>>> Sent: Friday, June 28, 2024 8:32 AM >>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays >>>>>> to describe the register offsets >>>>>> >>>>>> Hi, Biju, >>>>>> >>>>>> On 28.06.2024 08:59, Biju Das wrote: >>>>>>> Hi Claudiu, >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> >>>>>>>> Sent: Tuesday, June 25, 2024 1:14 PM >>>>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>>>>>> describe the register offsets >>>>>>>> >>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>>>>> >>>>>>>> Define individual arrays to describe the register offsets. In >>>>>>>> this way we can describe different IP variants that share the >>>>>>>> same register offsets but have differences in other characteristics. >>>>>>>> Commit prepares for the addition >>>>>> of fast mode plus. >>>>>>>> >>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>>>>> --- >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - none >>>>>>>> >>>>>>>> drivers/i2c/busses/i2c-riic.c | 58 >>>>>>>> +++++++++++++++++++---------------- >>>>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c >>>>>>>> b/drivers/i2c/busses/i2c-riic.c index >>>>>>>> 9fe007609076..8ffbead95492 100644 >>>>>>>> --- a/drivers/i2c/busses/i2c-riic.c >>>>>>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; >>>>>>>> >>>>>>>> struct riic_of_data { >>>>>>>> - u8 regs[RIIC_REG_END]; >>>>>>>> + const u8 *regs; >>>>>>> >>>>>>> >>>>>>> Since you are touching this part, can we drop struct and Use u8* >>>>>>> as device_data instead? >>>>>> >>>>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new >>>>>> member to struct >>>> riic_of_data. >>>>>> That new member is needed to differentiate b/w hardware versions >>>>>> supporting fast mode plus based on compatible. >>>>> >>>>> Are we sure RZ/A does not support fast mode plus? >>>> >>>> From commit description of patch 09/12: >>>> >>>> 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. >>>> >>>> I checked the manuals of all the SoCs where this driver is used. >>>> >>>> I haven't checked the H/W manual? >>>> >>>> On the manual I've downloaded from Renesas web site the FMPE bit of >>>> RIICnFER is not available on RZ/A1H. >>> >>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. >> >> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > > Maybe make the register layout as per SoC > > RZ/A1 --> &riic_rz_a_info > RZ/A2 and RZ/{G2L,G2LC,V2L,G2UL,FIVE} --> &riic_rz_g2_info > RZ/G3S and RZ/V2H --> &riic_rz_v2h_info Sorry, but I don't understand. Patch 09/12 already does that but a bit differently: RZ/{G2L, G2LC, G2UL, V2L, FIVE} -> riic_rz_g2_info RZ/G3S and RZ/V2H -> riic_rz_v2h_info Everything else: riic_rz_a_info I don't have anything at hand to test the "everything else" thus I enabled it for RZ/{G2L, G2LC, G2UL, V2L, FIVE}, RZ/G3S and RZ/V2H. > > Then except RZ/A1, set FMP. I cannot test all these. > > Currently register layout of RZ/A2 is not matching with > Hardware manual. I cannot test this either. Also, I think this is not the purpose of this series. Thank you, Claudiu Beznea > > Cheers, > Biju
Hi, Geert, On 28.06.2024 12:13, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Fri, Jun 28, 2024 at 10:12 AM claudiu beznea > <claudiu.beznea@tuxon.dev> wrote: >> On 28.06.2024 11:09, Biju Das wrote: >>>> -----Original Message----- >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>> Sent: Friday, June 28, 2024 9:03 AM >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets >>>> >>>> >>>> >>>> On 28.06.2024 10:55, Biju Das wrote: >>>>> Hi Claudiu, >>>>> >>>>>> -----Original Message----- >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>>>> Sent: Friday, June 28, 2024 8:32 AM >>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>>>> describe the register offsets >>>>>> >>>>>> Hi, Biju, >>>>>> >>>>>> On 28.06.2024 08:59, Biju Das wrote: >>>>>>> Hi Claudiu, >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> >>>>>>>> Sent: Tuesday, June 25, 2024 1:14 PM >>>>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>>>>>> describe the register offsets >>>>>>>> >>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>>>>> >>>>>>>> Define individual arrays to describe the register offsets. In this >>>>>>>> way we can describe different IP variants that share the same >>>>>>>> register offsets but have differences in other characteristics. >>>>>>>> Commit prepares for the addition >>>>>> of fast mode plus. >>>>>>>> >>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>>>>> --- >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - none >>>>>>>> >>>>>>>> drivers/i2c/busses/i2c-riic.c | 58 >>>>>>>> +++++++++++++++++++---------------- >>>>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c >>>>>>>> b/drivers/i2c/busses/i2c-riic.c index >>>>>>>> 9fe007609076..8ffbead95492 100644 >>>>>>>> --- a/drivers/i2c/busses/i2c-riic.c >>>>>>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; >>>>>>>> >>>>>>>> struct riic_of_data { >>>>>>>> - u8 regs[RIIC_REG_END]; >>>>>>>> + const u8 *regs; >>>>>>> >>>>>>> >>>>>>> Since you are touching this part, can we drop struct and Use u8* as >>>>>>> device_data instead? >>>>>> >>>>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct >>>> riic_of_data. >>>>>> That new member is needed to differentiate b/w hardware versions >>>>>> supporting fast mode plus based on compatible. >>>>> >>>>> Are we sure RZ/A does not support fast mode plus? >>>> >>>> From commit description of patch 09/12: >>>> >>>> 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. >>>> >>>> I checked the manuals of all the SoCs where this driver is used. >>>> >>>> I haven't checked the H/W manual? >>>> >>>> On the manual I've downloaded from Renesas web site the FMPE bit of RIICnFER is not available on >>>> RZ/A1H. >>> >>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. >> >> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > > Do you need to check for that? > > The ICFER_FMPE bit won't be set unless the user specifies the FM+ > clock-frequency. Setting clock-frequency beyond Fast Mode on RZ/A1H > would be very wrong. I need it to avoid this scenario ^. In patch 09/12 there is this code: + 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; to avoid giving the user the possibility to set FM+ freq on platforms not supporting it. Please let me know if I'm missing something (or wrongly understood your statement). Thank you, Claudiu Beznea > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, > -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > Sent: Friday, June 28, 2024 11:25 AM > Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > > > On 28.06.2024 11:24, Biju Das wrote: > > Hi Claudiu, > > > >> -----Original Message----- > >> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >> Sent: Friday, June 28, 2024 9:13 AM > >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >> describe the register offsets > >> > >> > >> > >> On 28.06.2024 11:09, Biju Das wrote: > >>> > >>> Hi Claudiu, > >>> > >>>> -----Original Message----- > >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>> Sent: Friday, June 28, 2024 9:03 AM > >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > >>>> to describe the register offsets > >>>> > >>>> > >>>> > >>>> On 28.06.2024 10:55, Biju Das wrote: > >>>>> Hi Claudiu, > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>>>> Sent: Friday, June 28, 2024 8:32 AM > >>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > >>>>>> to describe the register offsets > >>>>>> > >>>>>> Hi, Biju, > >>>>>> > >>>>>> On 28.06.2024 08:59, Biju Das wrote: > >>>>>>> Hi Claudiu, > >>>>>>> > >>>>>>>> -----Original Message----- > >>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> > >>>>>>>> Sent: Tuesday, June 25, 2024 1:14 PM > >>>>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays > >>>>>>>> to describe the register offsets > >>>>>>>> > >>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>>>>>> > >>>>>>>> Define individual arrays to describe the register offsets. In > >>>>>>>> this way we can describe different IP variants that share the > >>>>>>>> same register offsets but have differences in other characteristics. > >>>>>>>> Commit prepares for the addition > >>>>>> of fast mode plus. > >>>>>>>> > >>>>>>>> Signed-off-by: Claudiu Beznea > >>>>>>>> <claudiu.beznea.uj@bp.renesas.com> > >>>>>>>> --- > >>>>>>>> > >>>>>>>> Changes in v2: > >>>>>>>> - none > >>>>>>>> > >>>>>>>> drivers/i2c/busses/i2c-riic.c | 58 > >>>>>>>> +++++++++++++++++++---------------- > >>>>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c > >>>>>>>> b/drivers/i2c/busses/i2c-riic.c index > >>>>>>>> 9fe007609076..8ffbead95492 100644 > >>>>>>>> --- a/drivers/i2c/busses/i2c-riic.c > >>>>>>>> +++ b/drivers/i2c/busses/i2c-riic.c > >>>>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; > >>>>>>>> > >>>>>>>> struct riic_of_data { > >>>>>>>> - u8 regs[RIIC_REG_END]; > >>>>>>>> + const u8 *regs; > >>>>>>> > >>>>>>> > >>>>>>> Since you are touching this part, can we drop struct and Use u8* > >>>>>>> as device_data instead? > >>>>>> > >>>>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a > >>>>>> new member to struct > >>>> riic_of_data. > >>>>>> That new member is needed to differentiate b/w hardware versions > >>>>>> supporting fast mode plus based on compatible. > >>>>> > >>>>> Are we sure RZ/A does not support fast mode plus? > >>>> > >>>> From commit description of patch 09/12: > >>>> > >>>> 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. > >>>> > >>>> I checked the manuals of all the SoCs where this driver is used. > >>>> > >>>> I haven't checked the H/W manual? > >>>> > >>>> On the manual I've downloaded from Renesas web site the FMPE bit of > >>>> RIICnFER is not available on RZ/A1H. > >>> > >>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > >> > >> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > > > > Maybe make the register layout as per SoC > > > > RZ/A1 --> &riic_rz_a_info > > RZ/A2 and RZ/{G2L,G2LC,V2L,G2UL,FIVE} --> &riic_rz_g2_info RZ/G3S and > > RZ/V2H --> &riic_rz_v2h_info > > Sorry, but I don't understand. Patch 09/12 already does that but a bit > differently: Now register layout is added to differentiate the SoCs for adding support to RZ/G3S and this layout should match with the hardware manual for all supported SoCs. Currently it is wrong for RZ/A2 SoC, while you fixed it for all other SoCs. > > RZ/{G2L, G2LC, G2UL, V2L, FIVE} -> riic_rz_g2_info RZ/G3S and RZ/V2H -> riic_rz_v2h_info Everything > else: riic_rz_a_info > > I don't have anything at hand to test the "everything else" thus I enabled it for RZ/{G2L, G2LC, > G2UL, V2L, FIVE}, RZ/G3S and RZ/V2H. You don't need to test, as the existing other users don't have FMP+ enabled in device tree. Cheers, Biju
On 28.06.2024 13:49, Biju Das wrote: > Hi Claudiu, > >> -----Original Message----- >> From: claudiu beznea <claudiu.beznea@tuxon.dev> >> Sent: Friday, June 28, 2024 11:25 AM >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets >> >> >> >> On 28.06.2024 11:24, Biju Das wrote: >>> Hi Claudiu, >>> >>>> -----Original Message----- >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>> Sent: Friday, June 28, 2024 9:13 AM >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to >>>> describe the register offsets >>>> >>>> >>>> >>>> On 28.06.2024 11:09, Biju Das wrote: >>>>> >>>>> Hi Claudiu, >>>>> >>>>>> -----Original Message----- >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>>>> Sent: Friday, June 28, 2024 9:03 AM >>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays >>>>>> to describe the register offsets >>>>>> >>>>>> >>>>>> >>>>>> On 28.06.2024 10:55, Biju Das wrote: >>>>>>> Hi Claudiu, >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>>>>>> Sent: Friday, June 28, 2024 8:32 AM >>>>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays >>>>>>>> to describe the register offsets >>>>>>>> >>>>>>>> Hi, Biju, >>>>>>>> >>>>>>>> On 28.06.2024 08:59, Biju Das wrote: >>>>>>>>> Hi Claudiu, >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> >>>>>>>>>> Sent: Tuesday, June 25, 2024 1:14 PM >>>>>>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays >>>>>>>>>> to describe the register offsets >>>>>>>>>> >>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>>>>>>> >>>>>>>>>> Define individual arrays to describe the register offsets. In >>>>>>>>>> this way we can describe different IP variants that share the >>>>>>>>>> same register offsets but have differences in other characteristics. >>>>>>>>>> Commit prepares for the addition >>>>>>>> of fast mode plus. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Claudiu Beznea >>>>>>>>>> <claudiu.beznea.uj@bp.renesas.com> >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> Changes in v2: >>>>>>>>>> - none >>>>>>>>>> >>>>>>>>>> drivers/i2c/busses/i2c-riic.c | 58 >>>>>>>>>> +++++++++++++++++++---------------- >>>>>>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c >>>>>>>>>> b/drivers/i2c/busses/i2c-riic.c index >>>>>>>>>> 9fe007609076..8ffbead95492 100644 >>>>>>>>>> --- a/drivers/i2c/busses/i2c-riic.c >>>>>>>>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>>>>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; >>>>>>>>>> >>>>>>>>>> struct riic_of_data { >>>>>>>>>> - u8 regs[RIIC_REG_END]; >>>>>>>>>> + const u8 *regs; >>>>>>>>> >>>>>>>>> >>>>>>>>> Since you are touching this part, can we drop struct and Use u8* >>>>>>>>> as device_data instead? >>>>>>>> >>>>>>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a >>>>>>>> new member to struct >>>>>> riic_of_data. >>>>>>>> That new member is needed to differentiate b/w hardware versions >>>>>>>> supporting fast mode plus based on compatible. >>>>>>> >>>>>>> Are we sure RZ/A does not support fast mode plus? >>>>>> >>>>>> From commit description of patch 09/12: >>>>>> >>>>>> 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. >>>>>> >>>>>> I checked the manuals of all the SoCs where this driver is used. >>>>>> >>>>>> I haven't checked the H/W manual? >>>>>> >>>>>> On the manual I've downloaded from Renesas web site the FMPE bit of >>>>>> RIICnFER is not available on RZ/A1H. >>>>> >>>>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. >>>> >>>> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. >>> >>> Maybe make the register layout as per SoC >>> >>> RZ/A1 --> &riic_rz_a_info >>> RZ/A2 and RZ/{G2L,G2LC,V2L,G2UL,FIVE} --> &riic_rz_g2_info RZ/G3S and >>> RZ/V2H --> &riic_rz_v2h_info >> >> Sorry, but I don't understand. Patch 09/12 already does that but a bit >> differently: > > Now register layout is added to differentiate the SoCs for adding support > to RZ/G3S and this layout should match with the hardware manual for all supported SoCs. > Currently it is wrong for RZ/A2 SoC, while you fixed it for all other SoCs. I checked RZ/A2M. There is nothing broken. The only thing that I see is that the FP+ is not enabled on RZ/A2M (please let me know if there is anything else I missed). I don't see this broken. It is the same behavior that was before this patch. Anyway, I'll update it for that too, if nobody has something against, but I cannot test it. If any hardware bug for it, I cannot say. > >> >> RZ/{G2L, G2LC, G2UL, V2L, FIVE} -> riic_rz_g2_info RZ/G3S and RZ/V2H -> riic_rz_v2h_info Everything >> else: riic_rz_a_info >> >> I don't have anything at hand to test the "everything else" thus I enabled it for RZ/{G2L, G2LC, >> G2UL, V2L, FIVE}, RZ/G3S and RZ/V2H. > > You don't need to test, as > the existing other users don't have FMP+ enabled in device tree. It's the same as today (w/o adding specific entry for it). > > Cheers, > Biju
Hi Claudiu, > -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > Sent: Friday, June 28, 2024 12:25 PM > Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > > > On 28.06.2024 13:49, Biju Das wrote: > > Hi Claudiu, > > > >> -----Original Message----- > >> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >> Sent: Friday, June 28, 2024 11:25 AM > >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to > >> describe the register offsets > >> > >> > >> > >> On 28.06.2024 11:24, Biju Das wrote: > >>> Hi Claudiu, > >>> > >>>> -----Original Message----- > >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>> Sent: Friday, June 28, 2024 9:13 AM > >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > >>>> to describe the register offsets > >>>> > >>>> > >>>> > >>>> On 28.06.2024 11:09, Biju Das wrote: > >>>>> > >>>>> Hi Claudiu, > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>>>> Sent: Friday, June 28, 2024 9:03 AM > >>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > >>>>>> to describe the register offsets > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 28.06.2024 10:55, Biju Das wrote: > >>>>>>> Hi Claudiu, > >>>>>>> > >>>>>>>> -----Original Message----- > >>>>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>>>>>> Sent: Friday, June 28, 2024 8:32 AM > >>>>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual > >>>>>>>> arrays to describe the register offsets > >>>>>>>> > >>>>>>>> Hi, Biju, > >>>>>>>> > >>>>>>>> On 28.06.2024 08:59, Biju Das wrote: > >>>>>>>>> Hi Claudiu, > >>>>>>>>> > >>>>>>>>>> -----Original Message----- > >>>>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> > >>>>>>>>>> Sent: Tuesday, June 25, 2024 1:14 PM > >>>>>>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays > >>>>>>>>>> to describe the register offsets > >>>>>>>>>> > >>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>>>>>>>> > >>>>>>>>>> Define individual arrays to describe the register offsets. In > >>>>>>>>>> this way we can describe different IP variants that share the > >>>>>>>>>> same register offsets but have differences in other characteristics. > >>>>>>>>>> Commit prepares for the addition > >>>>>>>> of fast mode plus. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Claudiu Beznea > >>>>>>>>>> <claudiu.beznea.uj@bp.renesas.com> > >>>>>>>>>> --- > >>>>>>>>>> > >>>>>>>>>> Changes in v2: > >>>>>>>>>> - none > >>>>>>>>>> > >>>>>>>>>> drivers/i2c/busses/i2c-riic.c | 58 > >>>>>>>>>> +++++++++++++++++++---------------- > >>>>>>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c > >>>>>>>>>> b/drivers/i2c/busses/i2c-riic.c index > >>>>>>>>>> 9fe007609076..8ffbead95492 100644 > >>>>>>>>>> --- a/drivers/i2c/busses/i2c-riic.c > >>>>>>>>>> +++ b/drivers/i2c/busses/i2c-riic.c > >>>>>>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; > >>>>>>>>>> > >>>>>>>>>> struct riic_of_data { > >>>>>>>>>> - u8 regs[RIIC_REG_END]; > >>>>>>>>>> + const u8 *regs; > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> Since you are touching this part, can we drop struct and Use > >>>>>>>>> u8* as device_data instead? > >>>>>>>> > >>>>>>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a > >>>>>>>> new member to struct > >>>>>> riic_of_data. > >>>>>>>> That new member is needed to differentiate b/w hardware > >>>>>>>> versions supporting fast mode plus based on compatible. > >>>>>>> > >>>>>>> Are we sure RZ/A does not support fast mode plus? > >>>>>> > >>>>>> From commit description of patch 09/12: > >>>>>> > >>>>>> 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. > >>>>>> > >>>>>> I checked the manuals of all the SoCs where this driver is used. > >>>>>> > >>>>>> I haven't checked the H/W manual? > >>>>>> > >>>>>> On the manual I've downloaded from Renesas web site the FMPE bit > >>>>>> of RIICnFER is not available on RZ/A1H. > >>>>> > >>>>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > >>>> > >>>> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > >>> > >>> Maybe make the register layout as per SoC > >>> > >>> RZ/A1 --> &riic_rz_a_info > >>> RZ/A2 and RZ/{G2L,G2LC,V2L,G2UL,FIVE} --> &riic_rz_g2_info RZ/G3S > >>> and RZ/V2H --> &riic_rz_v2h_info > >> > >> Sorry, but I don't understand. Patch 09/12 already does that but a > >> bit > >> differently: > > > > Now register layout is added to differentiate the SoCs for adding > > support to RZ/G3S and this layout should match with the hardware manual for all supported SoCs. > > Currently it is wrong for RZ/A2 SoC, while you fixed it for all other SoCs. > > I checked RZ/A2M. There is nothing broken. The only thing that I see is that the FP+ is not enabled > on RZ/A2M (please let me know if there is anything else I missed). I don't see this broken. It is > the same behavior that was before this patch. As per RZ/A2M hardware manual, ICFER register is present While as per [1], you don't have this register. So according to me RZ/A2 SoC register layout is broken and it is same as RZ/A1. [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240625121358.590547-10-claudiu.beznea.uj@bp.renesas.com/ Cheers, Biju
On Fri, Jun 28, 2024 at 12:29 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 28.06.2024 12:13, Geert Uytterhoeven wrote: > > On Fri, Jun 28, 2024 at 10:12 AM claudiu beznea > > <claudiu.beznea@tuxon.dev> wrote: > >> On 28.06.2024 11:09, Biju Das wrote: > >>>> -----Original Message----- > >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>> On 28.06.2024 10:55, Biju Das wrote: > >>>>>> -----Original Message----- > >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > >>>>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct > >>>> riic_of_data. > >>>>>> That new member is needed to differentiate b/w hardware versions > >>>>>> supporting fast mode plus based on compatible. > >>>>> > >>>>> Are we sure RZ/A does not support fast mode plus? > >>>> > >>>> From commit description of patch 09/12: > >>>> > >>>> 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. > >>>> > >>>> I checked the manuals of all the SoCs where this driver is used. > >>>> > >>>> I haven't checked the H/W manual? > >>>> > >>>> On the manual I've downloaded from Renesas web site the FMPE bit of RIICnFER is not available on > >>>> RZ/A1H. > >>> > >>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > >> > >> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > > > > Do you need to check for that? > > > > The ICFER_FMPE bit won't be set unless the user specifies the FM+ > > clock-frequency. Setting clock-frequency beyond Fast Mode on RZ/A1H > > would be very wrong. > > I need it to avoid this scenario ^. In patch 09/12 there is this code: > > + 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; > > to avoid giving the user the possibility to set FM+ freq on platforms not > supporting it. > > Please let me know if I'm missing something (or wrongly understood your > statement). Wolfram/Andi: what is your view on this? Gr{oetje,eeting}s, Geert
> Subject: RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets > > Hi Claudiu, > > > -----Original Message----- > > From: claudiu beznea <claudiu.beznea@tuxon.dev> > > Sent: Friday, June 28, 2024 12:25 PM > > Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to > > describe the register offsets > > > > > > > > On 28.06.2024 13:49, Biju Das wrote: > > > Hi Claudiu, > > > > > >> -----Original Message----- > > >> From: claudiu beznea <claudiu.beznea@tuxon.dev> > > >> Sent: Friday, June 28, 2024 11:25 AM > > >> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > > >> to describe the register offsets > > >> > > >> > > >> > > >> On 28.06.2024 11:24, Biju Das wrote: > > >>> Hi Claudiu, > > >>> > > >>>> -----Original Message----- > > >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > > >>>> Sent: Friday, June 28, 2024 9:13 AM > > >>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays > > >>>> to describe the register offsets > > >>>> > > >>>> > > >>>> > > >>>> On 28.06.2024 11:09, Biju Das wrote: > > >>>>> > > >>>>> Hi Claudiu, > > >>>>> > > >>>>>> -----Original Message----- > > >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > > >>>>>> Sent: Friday, June 28, 2024 9:03 AM > > >>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual > > >>>>>> arrays to describe the register offsets > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> On 28.06.2024 10:55, Biju Das wrote: > > >>>>>>> Hi Claudiu, > > >>>>>>> > > >>>>>>>> -----Original Message----- > > >>>>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> > > >>>>>>>> Sent: Friday, June 28, 2024 8:32 AM > > >>>>>>>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual > > >>>>>>>> arrays to describe the register offsets > > >>>>>>>> > > >>>>>>>> Hi, Biju, > > >>>>>>>> > > >>>>>>>> On 28.06.2024 08:59, Biju Das wrote: > > >>>>>>>>> Hi Claudiu, > > >>>>>>>>> > > >>>>>>>>>> -----Original Message----- > > >>>>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev> > > >>>>>>>>>> Sent: Tuesday, June 25, 2024 1:14 PM > > >>>>>>>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual > > >>>>>>>>>> arrays to describe the register offsets > > >>>>>>>>>> > > >>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > >>>>>>>>>> > > >>>>>>>>>> Define individual arrays to describe the register offsets. > > >>>>>>>>>> In this way we can describe different IP variants that > > >>>>>>>>>> share the same register offsets but have differences in other characteristics. > > >>>>>>>>>> Commit prepares for the addition > > >>>>>>>> of fast mode plus. > > >>>>>>>>>> > > >>>>>>>>>> Signed-off-by: Claudiu Beznea > > >>>>>>>>>> <claudiu.beznea.uj@bp.renesas.com> > > >>>>>>>>>> --- > > >>>>>>>>>> > > >>>>>>>>>> Changes in v2: > > >>>>>>>>>> - none > > >>>>>>>>>> > > >>>>>>>>>> drivers/i2c/busses/i2c-riic.c | 58 > > >>>>>>>>>> +++++++++++++++++++---------------- > > >>>>>>>>>> 1 file changed, 31 insertions(+), 27 deletions(-) > > >>>>>>>>>> > > >>>>>>>>>> diff --git a/drivers/i2c/busses/i2c-riic.c > > >>>>>>>>>> b/drivers/i2c/busses/i2c-riic.c index > > >>>>>>>>>> 9fe007609076..8ffbead95492 100644 > > >>>>>>>>>> --- a/drivers/i2c/busses/i2c-riic.c > > >>>>>>>>>> +++ b/drivers/i2c/busses/i2c-riic.c > > >>>>>>>>>> @@ -91,7 +91,7 @@ enum riic_reg_list { }; > > >>>>>>>>>> > > >>>>>>>>>> struct riic_of_data { > > >>>>>>>>>> - u8 regs[RIIC_REG_END]; > > >>>>>>>>>> + const u8 *regs; > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> Since you are touching this part, can we drop struct and Use > > >>>>>>>>> u8* as device_data instead? > > >>>>>>>> > > >>>>>>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds > > >>>>>>>> a new member to struct > > >>>>>> riic_of_data. > > >>>>>>>> That new member is needed to differentiate b/w hardware > > >>>>>>>> versions supporting fast mode plus based on compatible. > > >>>>>>> > > >>>>>>> Are we sure RZ/A does not support fast mode plus? > > >>>>>> > > >>>>>> From commit description of patch 09/12: > > >>>>>> > > >>>>>> 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. > > >>>>>> > > >>>>>> I checked the manuals of all the SoCs where this driver is used. > > >>>>>> > > >>>>>> I haven't checked the H/W manual? > > >>>>>> > > >>>>>> On the manual I've downloaded from Renesas web site the FMPE > > >>>>>> bit of RIICnFER is not available on RZ/A1H. > > >>>>> > > >>>>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > > >>>> > > >>>> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > > >>> > > >>> Maybe make the register layout as per SoC > > >>> > > >>> RZ/A1 --> &riic_rz_a_info > > >>> RZ/A2 and RZ/{G2L,G2LC,V2L,G2UL,FIVE} --> &riic_rz_g2_info RZ/G3S > > >>> and RZ/V2H --> &riic_rz_v2h_info > > >> > > >> Sorry, but I don't understand. Patch 09/12 already does that but a > > >> bit > > >> differently: > > > > > > Now register layout is added to differentiate the SoCs for adding > > > support to RZ/G3S and this layout should match with the hardware manual for all supported SoCs. > > > Currently it is wrong for RZ/A2 SoC, while you fixed it for all other SoCs. > > > > I checked RZ/A2M. There is nothing broken. The only thing that I see > > is that the FP+ is not enabled on RZ/A2M (please let me know if there > > is anything else I missed). I don't see this broken. It is the same behavior that was before this > patch. > > As per RZ/A2M hardware manual, ICFER register is present > > While as per [1], you don't have this register. So according to me RZ/A2 SoC register layout is > broken and it is same as RZ/A1. > Oops, ICFER register is present in RZ/A1 as well. Currently same compatible used for RZ/A1 and RZ/A2 that needs to be updated in patch#9 as the later has FMP capability like RZ/G2L. Cheers, Biju
On 28.06.2024 14:39, Geert Uytterhoeven wrote: > On Fri, Jun 28, 2024 at 12:29 PM claudiu beznea > <claudiu.beznea@tuxon.dev> wrote: >> On 28.06.2024 12:13, Geert Uytterhoeven wrote: >>> On Fri, Jun 28, 2024 at 10:12 AM claudiu beznea >>> <claudiu.beznea@tuxon.dev> wrote: >>>> On 28.06.2024 11:09, Biju Das wrote: >>>>>> -----Original Message----- >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>>>> On 28.06.2024 10:55, Biju Das wrote: >>>>>>>> -----Original Message----- >>>>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev> >>>>>>>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct >>>>>> riic_of_data. >>>>>>>> That new member is needed to differentiate b/w hardware versions >>>>>>>> supporting fast mode plus based on compatible. >>>>>>> >>>>>>> Are we sure RZ/A does not support fast mode plus? >>>>>> >>>>>> From commit description of patch 09/12: >>>>>> >>>>>> 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. >>>>>> >>>>>> I checked the manuals of all the SoCs where this driver is used. >>>>>> >>>>>> I haven't checked the H/W manual? >>>>>> >>>>>> On the manual I've downloaded from Renesas web site the FMPE bit of RIICnFER is not available on >>>>>> RZ/A1H. >>>>> >>>>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. >>>> >>>> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. >>> >>> Do you need to check for that? >>> >>> The ICFER_FMPE bit won't be set unless the user specifies the FM+ >>> clock-frequency. Setting clock-frequency beyond Fast Mode on RZ/A1H >>> would be very wrong. >> >> I need it to avoid this scenario ^. In patch 09/12 there is this code: >> >> + 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; >> FTR, the full context of this change is (from patch 09/12): @@ -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; } Thank you, Claudiu Beznea >> to avoid giving the user the possibility to set FM+ freq on platforms not >> supporting it. >> >> Please let me know if I'm missing something (or wrongly understood your >> statement). > > Wolfram/Andi: what is your view on this? > > Gr{oetje,eeting}s, > > Geert >
Hi Geert, I've been reading all the review history of this series and now I'm walking through all the review history again do see if everything has been > > >>>> On the manual I've downloaded from Renesas web site the FMPE bit of RIICnFER is not available on > > >>>> RZ/A1H. > > >>> > > >>> I just found RZ/A2M manual, it supports FMP and register layout looks similar to RZ/G2L. > > >> > > >> I introduced struct riic_of_data::fast_mode_plus because of RZ/A1H. > > > > > > Do you need to check for that? > > > > > > The ICFER_FMPE bit won't be set unless the user specifies the FM+ > > > clock-frequency. Setting clock-frequency beyond Fast Mode on RZ/A1H > > > would be very wrong. > > > > I need it to avoid this scenario ^. In patch 09/12 there is this code: > > > > + 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; > > > > to avoid giving the user the possibility to set FM+ freq on platforms not > > supporting it. > > > > Please let me know if I'm missing something (or wrongly understood your > > statement). > > Wolfram/Andi: what is your view on this? I don't have anything against it... what exactly are you proposing here? If you want you can directly reply to the v4 6/11 patch. Thanks Geert for checking on this series. Andi
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index 9fe007609076..8ffbead95492 100644 --- a/drivers/i2c/busses/i2c-riic.c +++ b/drivers/i2c/busses/i2c-riic.c @@ -91,7 +91,7 @@ enum riic_reg_list { }; struct riic_of_data { - u8 regs[RIIC_REG_END]; + const u8 *regs; }; struct riic_dev { @@ -531,36 +531,40 @@ static void riic_i2c_remove(struct platform_device *pdev) pm_runtime_dont_use_autosuspend(dev); } +static const u8 riic_rz_a_regs[RIIC_REG_END] = { + [RIIC_ICCR1] = 0x00, + [RIIC_ICCR2] = 0x04, + [RIIC_ICMR1] = 0x08, + [RIIC_ICMR3] = 0x10, + [RIIC_ICSER] = 0x18, + [RIIC_ICIER] = 0x1c, + [RIIC_ICSR2] = 0x24, + [RIIC_ICBRL] = 0x34, + [RIIC_ICBRH] = 0x38, + [RIIC_ICDRT] = 0x3c, + [RIIC_ICDRR] = 0x40, +}; + static const struct riic_of_data riic_rz_a_info = { - .regs = { - [RIIC_ICCR1] = 0x00, - [RIIC_ICCR2] = 0x04, - [RIIC_ICMR1] = 0x08, - [RIIC_ICMR3] = 0x10, - [RIIC_ICSER] = 0x18, - [RIIC_ICIER] = 0x1c, - [RIIC_ICSR2] = 0x24, - [RIIC_ICBRL] = 0x34, - [RIIC_ICBRH] = 0x38, - [RIIC_ICDRT] = 0x3c, - [RIIC_ICDRR] = 0x40, - }, + .regs = riic_rz_a_regs, +}; + +static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { + [RIIC_ICCR1] = 0x00, + [RIIC_ICCR2] = 0x01, + [RIIC_ICMR1] = 0x02, + [RIIC_ICMR3] = 0x04, + [RIIC_ICSER] = 0x06, + [RIIC_ICIER] = 0x07, + [RIIC_ICSR2] = 0x09, + [RIIC_ICBRL] = 0x10, + [RIIC_ICBRH] = 0x11, + [RIIC_ICDRT] = 0x12, + [RIIC_ICDRR] = 0x13, }; static const struct riic_of_data riic_rz_v2h_info = { - .regs = { - [RIIC_ICCR1] = 0x00, - [RIIC_ICCR2] = 0x01, - [RIIC_ICMR1] = 0x02, - [RIIC_ICMR3] = 0x04, - [RIIC_ICSER] = 0x06, - [RIIC_ICIER] = 0x07, - [RIIC_ICSR2] = 0x09, - [RIIC_ICBRL] = 0x10, - [RIIC_ICBRH] = 0x11, - [RIIC_ICDRT] = 0x12, - [RIIC_ICDRR] = 0x13, - }, + .regs = riic_rz_v2h_regs, }; static int riic_i2c_suspend(struct device *dev)