| Message ID | 20240625121358.590547-8-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: riic: Add support for Renesas RZ/G3S | 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? 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. 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 >>>> >>>
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
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)