| Message ID | 20240625121358.590547-1-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| 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 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > pm_runtime_get_sync() may return with error. In case it returns with error > dev->power.usage_count needs to be decremented. > dev->pm_runtime_resume_and_get() > takes care of this. Thus use it. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - delete i2c adapter all the time in remove > > drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index > 83e4d5e14ab6..002b11b020fa 100644 > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -113,6 +113,8 @@ struct riic_irq_desc { > char *name; > }; > > +static const char * const riic_rpm_err_msg = "Failed to runtime > +resume"; > + > static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) { > writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 +135,14 @@ static int > riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > struct riic_dev *riic = i2c_get_adapdata(adap); > struct device *dev = adap->dev.parent; > unsigned long time_left; > - int i; > + int i, ret; > u8 start_bit; > > - pm_runtime_get_sync(dev); > + ret = pm_runtime_resume_and_get(dev); > + if (ret) { > + dev_err(dev, riic_rpm_err_msg); As at the moment we don't know how to reproduce this error condition Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? Cheers, Biju > + return ret; > + } > > if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) { > riic->err = -EBUSY; > @@ -301,6 +307,7 @@ static const struct i2c_algorithm riic_algo = { > > static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t) { > + int ret; > unsigned long rate; > int total_ticks, cks, brl, brh; > struct device *dev = riic->adapter.dev.parent; @@ -379,7 +386,11 @@ static int > riic_init_hw(struct riic_dev *riic, struct i2c_timings *t) > t->scl_fall_ns / (1000000000 / rate), > t->scl_rise_ns / (1000000000 / rate), cks, brl, brh); > > - pm_runtime_get_sync(dev); > + ret = pm_runtime_resume_and_get(dev); > + if (ret) { > + dev_err(dev, riic_rpm_err_msg); > + return ret; > + } > > /* Changing the order of accessing IICRST and ICE may break things! */ > riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1); @@ -498,11 +509,18 @@ static void > riic_i2c_remove(struct platform_device *pdev) { > struct riic_dev *riic = platform_get_drvdata(pdev); > struct device *dev = &pdev->dev; > + int ret; > > - pm_runtime_get_sync(dev); > - riic_writeb(riic, 0, RIIC_ICIER); > - pm_runtime_put(dev); > i2c_del_adapter(&riic->adapter); > + > + ret = pm_runtime_resume_and_get(dev); > + if (ret) { > + dev_err(dev, riic_rpm_err_msg); > + } else { > + riic_writeb(riic, 0, RIIC_ICIER); > + pm_runtime_put(dev); > + } > + > pm_runtime_disable(dev); > } > > -- > 2.39.2 >
Hi, Biju, On 25.06.2024 18:53, Biju Das wrote: > Hi Claudiu, > >> -----Original Message----- >> From: Claudiu <claudiu.beznea@tuxon.dev> >> Sent: Tuesday, June 25, 2024 1:14 PM >> Subject: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() >> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> pm_runtime_get_sync() may return with error. In case it returns with error >> dev->power.usage_count needs to be decremented. >> dev->pm_runtime_resume_and_get() >> takes care of this. Thus use it. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v2: >> - delete i2c adapter all the time in remove >> >> drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------ >> 1 file changed, 24 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index >> 83e4d5e14ab6..002b11b020fa 100644 >> --- a/drivers/i2c/busses/i2c-riic.c >> +++ b/drivers/i2c/busses/i2c-riic.c >> @@ -113,6 +113,8 @@ struct riic_irq_desc { >> char *name; >> }; >> >> +static const char * const riic_rpm_err_msg = "Failed to runtime >> +resume"; >> + >> static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) { >> writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 +135,14 @@ static int >> riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >> struct riic_dev *riic = i2c_get_adapdata(adap); >> struct device *dev = adap->dev.parent; >> unsigned long time_left; >> - int i; >> + int i, ret; >> u8 start_bit; >> >> - pm_runtime_get_sync(dev); >> + ret = pm_runtime_resume_and_get(dev); >> + if (ret) { >> + dev_err(dev, riic_rpm_err_msg); > > As at the moment we don't know how to reproduce this error condition > Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with dev_err() or something similar. Thank you, Claudiu Beznea [1] https://lwn.net/Articles/969923/ > > Cheers, > Biju > >> + return ret; >> + } >> >> if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) { >> riic->err = -EBUSY; >> @@ -301,6 +307,7 @@ static const struct i2c_algorithm riic_algo = { >> >> static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t) { >> + int ret; >> unsigned long rate; >> int total_ticks, cks, brl, brh; >> struct device *dev = riic->adapter.dev.parent; @@ -379,7 +386,11 @@ static int >> riic_init_hw(struct riic_dev *riic, struct i2c_timings *t) >> t->scl_fall_ns / (1000000000 / rate), >> t->scl_rise_ns / (1000000000 / rate), cks, brl, brh); >> >> - pm_runtime_get_sync(dev); >> + ret = pm_runtime_resume_and_get(dev); >> + if (ret) { >> + dev_err(dev, riic_rpm_err_msg); >> + return ret; >> + } >> >> /* Changing the order of accessing IICRST and ICE may break things! */ >> riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1); @@ -498,11 +509,18 @@ static void >> riic_i2c_remove(struct platform_device *pdev) { >> struct riic_dev *riic = platform_get_drvdata(pdev); >> struct device *dev = &pdev->dev; >> + int ret; >> >> - pm_runtime_get_sync(dev); >> - riic_writeb(riic, 0, RIIC_ICIER); >> - pm_runtime_put(dev); >> i2c_del_adapter(&riic->adapter); >> + >> + ret = pm_runtime_resume_and_get(dev); >> + if (ret) { >> + dev_err(dev, riic_rpm_err_msg); >> + } else { >> + riic_writeb(riic, 0, RIIC_ICIER); >> + pm_runtime_put(dev); >> + } >> + >> pm_runtime_disable(dev); >> } >> >> -- >> 2.39.2 >> >
> -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > Sent: Wednesday, June 26, 2024 7:14 AM > To: Biju Das <biju.das.jz@bp.renesas.com>; Chris Brandt <Chris.Brandt@renesas.com>; > andi.shyti@kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > geert+renesas@glider.be; magnus.damm@gmail.com; mturquette@baylibre.com; sboyd@kernel.org; > p.zabel@pengutronix.de; wsa+renesas@sang-engineering.com > Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-clk@vger.kernel.org; Claudiu Beznea > <claudiu.beznea.uj@bp.renesas.com> > Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi, Biju, > > On 25.06.2024 18:53, Biju Das wrote: > > Hi Claudiu, > > > >> -----Original Message----- > >> From: Claudiu <claudiu.beznea@tuxon.dev> > >> Sent: Tuesday, June 25, 2024 1:14 PM > >> Subject: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > >> > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> pm_runtime_get_sync() may return with error. In case it returns with > >> error > >> dev->power.usage_count needs to be decremented. > >> dev->pm_runtime_resume_and_get() > >> takes care of this. Thus use it. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> --- > >> > >> Changes in v2: > >> - delete i2c adapter all the time in remove > >> > >> drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------ > >> 1 file changed, 24 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-riic.c > >> b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa > >> 100644 > >> --- a/drivers/i2c/busses/i2c-riic.c > >> +++ b/drivers/i2c/busses/i2c-riic.c > >> @@ -113,6 +113,8 @@ struct riic_irq_desc { > >> char *name; > >> }; > >> > >> +static const char * const riic_rpm_err_msg = "Failed to runtime > >> +resume"; > >> + > >> static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) { > >> writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 > >> +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > >> struct riic_dev *riic = i2c_get_adapdata(adap); > >> struct device *dev = adap->dev.parent; > >> unsigned long time_left; > >> - int i; > >> + int i, ret; > >> u8 start_bit; > >> > >> - pm_runtime_get_sync(dev); > >> + ret = pm_runtime_resume_and_get(dev); > >> + if (ret) { > >> + dev_err(dev, riic_rpm_err_msg); > > > > As at the moment we don't know how to reproduce this error condition > > Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? > > [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with > dev_err() or something similar. WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? Currently we don't know how to trigger pm_runtime_resume_and_get() error condition in our setup using a testapp and we are expecting an error may happen in future. If at all there is an error in future, we need detailed error info so that we can handle it and fix the bug. Cheers, Biju > > Thank you, > Claudiu Beznea > > [1] https://lwn.net/Articles/969923/ > > > > > Cheers, > > Biju > > > >> + return ret; > >> + } > >> > >> if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) { > >> riic->err = -EBUSY; > >> @@ -301,6 +307,7 @@ static const struct i2c_algorithm riic_algo = { > >> > >> static int riic_init_hw(struct riic_dev *riic, struct i2c_timings > >> *t) { > >> + int ret; > >> unsigned long rate; > >> int total_ticks, cks, brl, brh; > >> struct device *dev = riic->adapter.dev.parent; @@ -379,7 +386,11 @@ > >> static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t) > >> t->scl_fall_ns / (1000000000 / rate), > >> t->scl_rise_ns / (1000000000 / rate), cks, brl, brh); > >> > >> - pm_runtime_get_sync(dev); > >> + ret = pm_runtime_resume_and_get(dev); > >> + if (ret) { > >> + dev_err(dev, riic_rpm_err_msg); > >> + return ret; > >> + } > >> > >> /* Changing the order of accessing IICRST and ICE may break things! */ > >> riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1); @@ > >> -498,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev) { > >> struct riic_dev *riic = platform_get_drvdata(pdev); > >> struct device *dev = &pdev->dev; > >> + int ret; > >> > >> - pm_runtime_get_sync(dev); > >> - riic_writeb(riic, 0, RIIC_ICIER); > >> - pm_runtime_put(dev); > >> i2c_del_adapter(&riic->adapter); > >> + > >> + ret = pm_runtime_resume_and_get(dev); > >> + if (ret) { > >> + dev_err(dev, riic_rpm_err_msg); > >> + } else { > >> + riic_writeb(riic, 0, RIIC_ICIER); > >> + pm_runtime_put(dev); > >> + } > >> + > >> pm_runtime_disable(dev); > >> } > >> > >> -- > >> 2.39.2 > >> > >
On 26.06.2024 09:23, Biju Das wrote: > > >> -----Original Message----- >> From: claudiu beznea <claudiu.beznea@tuxon.dev> >> Sent: Wednesday, June 26, 2024 7:14 AM >> To: Biju Das <biju.das.jz@bp.renesas.com>; Chris Brandt <Chris.Brandt@renesas.com>; >> andi.shyti@kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; >> geert+renesas@glider.be; magnus.damm@gmail.com; mturquette@baylibre.com; sboyd@kernel.org; >> p.zabel@pengutronix.de; wsa+renesas@sang-engineering.com >> Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; >> linux-kernel@vger.kernel.org; linux-clk@vger.kernel.org; Claudiu Beznea >> <claudiu.beznea.uj@bp.renesas.com> >> Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() >> >> Hi, Biju, >> >> On 25.06.2024 18:53, Biju Das wrote: >>> Hi Claudiu, >>> >>>> -----Original Message----- >>>> From: Claudiu <claudiu.beznea@tuxon.dev> >>>> Sent: Tuesday, June 25, 2024 1:14 PM >>>> Subject: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() >>>> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> pm_runtime_get_sync() may return with error. In case it returns with >>>> error >>>> dev->power.usage_count needs to be decremented. >>>> dev->pm_runtime_resume_and_get() >>>> takes care of this. Thus use it. >>>> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> --- >>>> >>>> Changes in v2: >>>> - delete i2c adapter all the time in remove >>>> >>>> drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------ >>>> 1 file changed, 24 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-riic.c >>>> b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa >>>> 100644 >>>> --- a/drivers/i2c/busses/i2c-riic.c >>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>> @@ -113,6 +113,8 @@ struct riic_irq_desc { >>>> char *name; >>>> }; >>>> >>>> +static const char * const riic_rpm_err_msg = "Failed to runtime >>>> +resume"; >>>> + >>>> static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) { >>>> writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 >>>> +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >>>> struct riic_dev *riic = i2c_get_adapdata(adap); >>>> struct device *dev = adap->dev.parent; >>>> unsigned long time_left; >>>> - int i; >>>> + int i, ret; >>>> u8 start_bit; >>>> >>>> - pm_runtime_get_sync(dev); >>>> + ret = pm_runtime_resume_and_get(dev); >>>> + if (ret) { >>>> + dev_err(dev, riic_rpm_err_msg); >>> >>> As at the moment we don't know how to reproduce this error condition >>> Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? >> >> [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with >> dev_err() or something similar. > > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? Ok, I'm leaving this to I2C maintainers. Andi, Wolfram, Would you prefer having WARN_ON_ONCE() instead of dev_err() for potential failures of pm_runtime_resume_and_get()? Thank you, Claudiu Beznea > > Currently we don't know how to trigger pm_runtime_resume_and_get() error > condition in our setup using a testapp and we are expecting an error may > happen in future. If at all there is an error in future, we need detailed > error info so that we can handle it and fix the bug. > > Cheers, > Biju > >> >> Thank you, >> Claudiu Beznea >> >> [1] https://lwn.net/Articles/969923/ >> >>> >>> Cheers, >>> Biju >>> >>>> + return ret; >>>> + } >>>> >>>> if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) { >>>> riic->err = -EBUSY; >>>> @@ -301,6 +307,7 @@ static const struct i2c_algorithm riic_algo = { >>>> >>>> static int riic_init_hw(struct riic_dev *riic, struct i2c_timings >>>> *t) { >>>> + int ret; >>>> unsigned long rate; >>>> int total_ticks, cks, brl, brh; >>>> struct device *dev = riic->adapter.dev.parent; @@ -379,7 +386,11 @@ >>>> static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t) >>>> t->scl_fall_ns / (1000000000 / rate), >>>> t->scl_rise_ns / (1000000000 / rate), cks, brl, brh); >>>> >>>> - pm_runtime_get_sync(dev); >>>> + ret = pm_runtime_resume_and_get(dev); >>>> + if (ret) { >>>> + dev_err(dev, riic_rpm_err_msg); >>>> + return ret; >>>> + } >>>> >>>> /* Changing the order of accessing IICRST and ICE may break things! */ >>>> riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1); @@ >>>> -498,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev) { >>>> struct riic_dev *riic = platform_get_drvdata(pdev); >>>> struct device *dev = &pdev->dev; >>>> + int ret; >>>> >>>> - pm_runtime_get_sync(dev); >>>> - riic_writeb(riic, 0, RIIC_ICIER); >>>> - pm_runtime_put(dev); >>>> i2c_del_adapter(&riic->adapter); >>>> + >>>> + ret = pm_runtime_resume_and_get(dev); >>>> + if (ret) { >>>> + dev_err(dev, riic_rpm_err_msg); >>>> + } else { >>>> + riic_writeb(riic, 0, RIIC_ICIER); >>>> + pm_runtime_put(dev); >>>> + } >>>> + >>>> pm_runtime_disable(dev); >>>> } >>>> >>>> -- >>>> 2.39.2 >>>> >>>
Hi Biju, On Wed, Jun 26, 2024 at 8:23 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: claudiu beznea <claudiu.beznea@tuxon.dev> > > On 25.06.2024 18:53, Biju Das wrote: > > >> From: Claudiu <claudiu.beznea@tuxon.dev> > > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > >> > > >> pm_runtime_get_sync() may return with error. In case it returns with > > >> error > > >> dev->power.usage_count needs to be decremented. > > >> dev->pm_runtime_resume_and_get() > > >> takes care of this. Thus use it. > > >> > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > >> - pm_runtime_get_sync(dev); > > >> + ret = pm_runtime_resume_and_get(dev); > > >> + if (ret) { > > >> + dev_err(dev, riic_rpm_err_msg); > > > > > > As at the moment we don't know how to reproduce this error condition > > > Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? > > > > [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with > > dev_err() or something similar. > > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? > > Currently we don't know how to trigger pm_runtime_resume_and_get() error > condition in our setup using a testapp and we are expecting an error may > happen in future. If at all there is an error in future, we need detailed > error info so that we can handle it and fix the bug. On Renesas systems, pm_runtime_resume_and_get() never fails. That's the reason why originally we didn't care to check the return value of pm_runtime_get_sync(). The various janitors disagreed, causing cascaded changes all over the place... IMHO, WARN_ON_ONCE() is definitely overkill, only bloating the code. Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Wednesday, June 26, 2024 8:11 AM > Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi Biju, > > On Wed, Jun 26, 2024 at 8:23 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > From: claudiu beznea <claudiu.beznea@tuxon.dev> On 25.06.2024 18:53, > > > Biju Das wrote: > > > >> From: Claudiu <claudiu.beznea@tuxon.dev> > > > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > >> > > > >> pm_runtime_get_sync() may return with error. In case it returns > > > >> with error > > > >> dev->power.usage_count needs to be decremented. > > > >> dev->pm_runtime_resume_and_get() > > > >> takes care of this. Thus use it. > > > >> > > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > >> - pm_runtime_get_sync(dev); > > > >> + ret = pm_runtime_resume_and_get(dev); if (ret) { > > > >> + dev_err(dev, riic_rpm_err_msg); > > > > > > > > As at the moment we don't know how to reproduce this error > > > > condition Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? > > > > > > [1] states "So, naturally, use of WARN_ON() is also now discouraged > > > much of the time". I've go with > > > dev_err() or something similar. > > > > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? > > > > Currently we don't know how to trigger pm_runtime_resume_and_get() > > error condition in our setup using a testapp and we are expecting an > > error may happen in future. If at all there is an error in future, we > > need detailed error info so that we can handle it and fix the bug. > > On Renesas systems, pm_runtime_resume_and_get() never fails. > That's the reason why originally we didn't care to check the return value of pm_runtime_get_sync(). I agree, I was under the impression, if the code guarantees balanced usage, then pm_runtime_get_sync()/put() it will never fails. But here we are adding checks in frequent calls like xfer on the assumption it may fail in future due to PM changes. Xfer, we are incrementing pm usage count with pm_runtime_get_sync() And then decrementing it with pm_runtime_put() once transfer completed So, there is no imbalance here. > > The various janitors disagreed, causing cascaded changes all over the place... Even the core code does not have check for this. https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L792 > > IMHO, WARN_ON_ONCE() is definitely overkill, only bloating the code. I suggested because we are adding this check because something wrong will happen in future due to PM subsystem changes and the check will capture the issue and will give detailed warning info in kernel log. Cheers, Biju
On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add clock, reset and power domain support for the I2C channels available > on the Renesas RZ/G3S SoC. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - updated clock names to match the documentation Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> i.e. will queue in renesas-clk for v6.11. Gr{oetje,eeting}s, Geert
Hi Claudiu, First of all, thanks Biju for checking the code and bringing up this topic. On Wed, Jun 26, 2024 at 09:30:52AM GMT, claudiu beznea wrote: > >> On 25.06.2024 18:53, Biju Das wrote: ... > >>>> static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) { > >>>> writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 > >>>> +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > >>>> struct riic_dev *riic = i2c_get_adapdata(adap); > >>>> struct device *dev = adap->dev.parent; > >>>> unsigned long time_left; > >>>> - int i; > >>>> + int i, ret; > >>>> u8 start_bit; > >>>> > >>>> - pm_runtime_get_sync(dev); > >>>> + ret = pm_runtime_resume_and_get(dev); > >>>> + if (ret) { > >>>> + dev_err(dev, riic_rpm_err_msg); > >>> > >>> As at the moment we don't know how to reproduce this error condition > >>> Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? > >> > >> [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with > >> dev_err() or something similar. > > > > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? > > Ok, I'm leaving this to I2C maintainers. > > Andi, Wolfram, > > Would you prefer having WARN_ON_ONCE() instead of dev_err() for potential > failures of pm_runtime_resume_and_get()? I prefer dev_err. WARN_ON should be used for some serious failures in the code. E.g. memory corruption, like: a = 5; WARN_ON(a != 5); but the system might still work even with such errors (otherwise there is BUG_ON()). Besides, WARN_ON() prints some information that are not really useful to understand why the system didn't resume. For example you don't need the stack trace for power management failures, but you need it for code tracing code bugs. Andi
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
Hi Claudiu, On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Fast mode plus is available on most of the IP variants that RIIC driver > is working with. The exception is (according to HW manuals of the SoCs > where this IP is available) the Renesas RZ/A1H. For this, patch > introduces the struct riic_of_data::fast_mode_plus. > > Fast mode plus was tested on RZ/G3S, RZ/G2{L,UL,LC}, RZ/Five by > instantiating the RIIC frequency to 1MHz and issuing i2c reads on the > fast mode plus capable devices (and the i2c clock frequency was checked on > RZ/G3S). > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -407,6 +413,9 @@ static int riic_init_hw(struct riic_dev *riic) > riic_writeb(riic, 0, RIIC_ICSER); > riic_writeb(riic, ICMR3_ACKWP | ICMR3_RDRFS, RIIC_ICMR3); > > + if (info->fast_mode_plus && t->bus_freq_hz == I2C_MAX_FAST_MODE_PLUS_FREQ) > + riic_clear_set_bit(riic, 0, ICFER_FMPE, RIIC_ICFER); Unless FM+ is specified, RIIC_ICFER is never written to. Probably the register should always be initialized, also to make sure the FMPE bit is cleared when it was set by the boot loader, but FM+ is not to be used. > + > riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1); > > pm_runtime_mark_last_busy(dev); Gr{oetje,eeting}s, Geert
On 28.06.2024 12:22, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Fast mode plus is available on most of the IP variants that RIIC driver >> is working with. The exception is (according to HW manuals of the SoCs >> where this IP is available) the Renesas RZ/A1H. For this, patch >> introduces the struct riic_of_data::fast_mode_plus. >> >> Fast mode plus was tested on RZ/G3S, RZ/G2{L,UL,LC}, RZ/Five by >> instantiating the RIIC frequency to 1MHz and issuing i2c reads on the >> fast mode plus capable devices (and the i2c clock frequency was checked on >> RZ/G3S). >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/i2c/busses/i2c-riic.c >> +++ b/drivers/i2c/busses/i2c-riic.c >> @@ -407,6 +413,9 @@ static int riic_init_hw(struct riic_dev *riic) >> riic_writeb(riic, 0, RIIC_ICSER); >> riic_writeb(riic, ICMR3_ACKWP | ICMR3_RDRFS, RIIC_ICMR3); >> >> + if (info->fast_mode_plus && t->bus_freq_hz == I2C_MAX_FAST_MODE_PLUS_FREQ) >> + riic_clear_set_bit(riic, 0, ICFER_FMPE, RIIC_ICFER); > > Unless FM+ is specified, RIIC_ICFER is never written to. > Probably the register should always be initialized, also to make sure > the FMPE bit is cleared when it was set by the boot loader, but FM+ > is not to be used. OK, that's a good point. Thank you, Claudiu Beznea > > >> + >> riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1); >> >> pm_runtime_mark_last_busy(dev); > > Gr{oetje,eeting}s, > > Geert >
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
Hi Claudiu, Thanks for the patch. > -----Original Message----- > From: Claudiu <claudiu.beznea@tuxon.dev> > Sent: Tuesday, June 25, 2024 1:14 PM > Subject: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Fast mode plus is available on most of the IP variants that RIIC driver is working with. The > exception is (according to HW manuals of the SoCs where this IP is available) the Renesas RZ/A1H. > For this, patch introduces the struct riic_of_data::fast_mode_plus. > > Fast mode plus was tested on RZ/G3S, RZ/G2{L,UL,LC}, RZ/Five by instantiating the RIIC frequency to > 1MHz and issuing i2c reads on the fast mode plus capable devices (and the i2c clock frequency was > checked on RZ/G3S). > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - dropped code that handles the renesas,riic-no-fast-mode-plus > - updated commit description > > drivers/i2c/busses/i2c-riic.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index > 8ffbead95492..c07317f95e82 100644 > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -63,6 +63,8 @@ > #define ICMR3_ACKWP 0x10 > #define ICMR3_ACKBT 0x08 > > +#define ICFER_FMPE 0x80 > + > #define ICIER_TIE 0x80 > #define ICIER_TEIE 0x40 > #define ICIER_RIE 0x20 > @@ -80,6 +82,7 @@ enum riic_reg_list { > RIIC_ICCR2, > RIIC_ICMR1, > RIIC_ICMR3, > + RIIC_ICFER, > RIIC_ICSER, > RIIC_ICIER, > RIIC_ICSR2, > @@ -92,6 +95,7 @@ enum riic_reg_list { > > struct riic_of_data { > const u8 *regs; > + bool fast_mode_plus; > }; > > struct riic_dev { > @@ -315,11 +319,13 @@ static int riic_init_hw(struct riic_dev *riic) > int total_ticks, cks, brl, brh; > struct i2c_timings *t = &riic->i2c_t; > struct device *dev = riic->adapter.dev.parent; > + const struct riic_of_data *info = riic->info; > > - if (t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) { > - dev_err(dev, > - "unsupported bus speed (%dHz). %d max\n", > - t->bus_freq_hz, I2C_MAX_FAST_MODE_FREQ); > + if ((!info->fast_mode_plus && t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) || > + (info->fast_mode_plus && t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ)) { > + dev_err(dev, "unsupported bus speed (%dHz). %d max\n", t->bus_freq_hz, > + info->fast_mode_plus ? I2C_MAX_FAST_MODE_PLUS_FREQ : > + I2C_MAX_FAST_MODE_FREQ); > return -EINVAL; > } > > @@ -407,6 +413,9 @@ static int riic_init_hw(struct riic_dev *riic) > riic_writeb(riic, 0, RIIC_ICSER); > riic_writeb(riic, ICMR3_ACKWP | ICMR3_RDRFS, RIIC_ICMR3); > > + if (info->fast_mode_plus && t->bus_freq_hz == I2C_MAX_FAST_MODE_PLUS_FREQ) > + riic_clear_set_bit(riic, 0, ICFER_FMPE, RIIC_ICFER); > + > riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1); > > pm_runtime_mark_last_busy(dev); > @@ -536,6 +545,7 @@ static const u8 riic_rz_a_regs[RIIC_REG_END] = { > [RIIC_ICCR2] = 0x04, > [RIIC_ICMR1] = 0x08, > [RIIC_ICMR3] = 0x10, > + [RIIC_ICFER] = 0x14, > [RIIC_ICSER] = 0x18, > [RIIC_ICIER] = 0x1c, > [RIIC_ICSR2] = 0x24, > @@ -549,11 +559,17 @@ static const struct riic_of_data riic_rz_a_info = { > .regs = riic_rz_a_regs, > }; > > +static const struct riic_of_data riic_rz_g2_info = { > + .regs = riic_rz_a_regs, > + .fast_mode_plus = true, > +}; > + > static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { > [RIIC_ICCR1] = 0x00, > [RIIC_ICCR2] = 0x01, > [RIIC_ICMR1] = 0x02, > [RIIC_ICMR3] = 0x04, > + [RIIC_ICFER] = 0x05, > [RIIC_ICSER] = 0x06, > [RIIC_ICIER] = 0x07, > [RIIC_ICSR2] = 0x09, > @@ -565,6 +581,7 @@ static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { > > static const struct riic_of_data riic_rz_v2h_info = { > .regs = riic_rz_v2h_regs, > + .fast_mode_plus = true, > }; > > static int riic_i2c_suspend(struct device *dev) @@ -613,6 +630,9 @@ static const struct dev_pm_ops > riic_i2c_pm_ops = { > > static const struct of_device_id riic_i2c_dt_ids[] = { > { .compatible = "renesas,riic-rz", .data = &riic_rz_a_info }, > + { .compatible = "renesas,riic-r9a07g043", .data = &riic_rz_g2_info, }, > + { .compatible = "renesas,riic-r9a07g044", .data = &riic_rz_g2_info, }, > + { .compatible = "renesas,riic-r9a07g054", .data = &riic_rz_g2_info, > +}, I feel, the better way is { .compatible = "renesas, renesas,r7s72100", .data = &riic_rz_a_info },--> As this SoC does not support FMP { .compatible = "renesas,riic-rz", .data = &riic_rz_g2_info, },--> As this SoCs has FMP+ support { .compatible = "renesas,riic-r9a09g057", .data = &riic_rz_v2h_info },--> As this SoCs has different register layout and FMP+ With this the number of compatible entries in the device tables reduced from 5 to 3. Cheers, Biju
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, Biju, On 29.06.2024 08:38, Biju Das wrote: > Hi Claudiu, > > Thanks for the patch. > >> -----Original Message----- >> From: Claudiu <claudiu.beznea@tuxon.dev> >> Sent: Tuesday, June 25, 2024 1:14 PM >> Subject: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus >> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Fast mode plus is available on most of the IP variants that RIIC driver is working with. The >> exception is (according to HW manuals of the SoCs where this IP is available) the Renesas RZ/A1H. >> For this, patch introduces the struct riic_of_data::fast_mode_plus. >> >> Fast mode plus was tested on RZ/G3S, RZ/G2{L,UL,LC}, RZ/Five by instantiating the RIIC frequency to >> 1MHz and issuing i2c reads on the fast mode plus capable devices (and the i2c clock frequency was >> checked on RZ/G3S). >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v2: >> - dropped code that handles the renesas,riic-no-fast-mode-plus >> - updated commit description >> >> drivers/i2c/busses/i2c-riic.c | 28 ++++++++++++++++++++++++---- >> 1 file changed, 24 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index >> 8ffbead95492..c07317f95e82 100644 >> --- a/drivers/i2c/busses/i2c-riic.c >> +++ b/drivers/i2c/busses/i2c-riic.c >> @@ -63,6 +63,8 @@ >> #define ICMR3_ACKWP 0x10 >> #define ICMR3_ACKBT 0x08 >> >> +#define ICFER_FMPE 0x80 >> + >> #define ICIER_TIE 0x80 >> #define ICIER_TEIE 0x40 >> #define ICIER_RIE 0x20 >> @@ -80,6 +82,7 @@ enum riic_reg_list { >> RIIC_ICCR2, >> RIIC_ICMR1, >> RIIC_ICMR3, >> + RIIC_ICFER, >> RIIC_ICSER, >> RIIC_ICIER, >> RIIC_ICSR2, >> @@ -92,6 +95,7 @@ enum riic_reg_list { >> >> struct riic_of_data { >> const u8 *regs; >> + bool fast_mode_plus; >> }; >> >> struct riic_dev { >> @@ -315,11 +319,13 @@ static int riic_init_hw(struct riic_dev *riic) >> int total_ticks, cks, brl, brh; >> struct i2c_timings *t = &riic->i2c_t; >> struct device *dev = riic->adapter.dev.parent; >> + const struct riic_of_data *info = riic->info; >> >> - if (t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) { >> - dev_err(dev, >> - "unsupported bus speed (%dHz). %d max\n", >> - t->bus_freq_hz, I2C_MAX_FAST_MODE_FREQ); >> + if ((!info->fast_mode_plus && t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) || >> + (info->fast_mode_plus && t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ)) { >> + dev_err(dev, "unsupported bus speed (%dHz). %d max\n", t->bus_freq_hz, >> + info->fast_mode_plus ? I2C_MAX_FAST_MODE_PLUS_FREQ : >> + I2C_MAX_FAST_MODE_FREQ); >> return -EINVAL; >> } >> >> @@ -407,6 +413,9 @@ static int riic_init_hw(struct riic_dev *riic) >> riic_writeb(riic, 0, RIIC_ICSER); >> riic_writeb(riic, ICMR3_ACKWP | ICMR3_RDRFS, RIIC_ICMR3); >> >> + if (info->fast_mode_plus && t->bus_freq_hz == I2C_MAX_FAST_MODE_PLUS_FREQ) >> + riic_clear_set_bit(riic, 0, ICFER_FMPE, RIIC_ICFER); >> + >> riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1); >> >> pm_runtime_mark_last_busy(dev); >> @@ -536,6 +545,7 @@ static const u8 riic_rz_a_regs[RIIC_REG_END] = { >> [RIIC_ICCR2] = 0x04, >> [RIIC_ICMR1] = 0x08, >> [RIIC_ICMR3] = 0x10, >> + [RIIC_ICFER] = 0x14, >> [RIIC_ICSER] = 0x18, >> [RIIC_ICIER] = 0x1c, >> [RIIC_ICSR2] = 0x24, >> @@ -549,11 +559,17 @@ static const struct riic_of_data riic_rz_a_info = { >> .regs = riic_rz_a_regs, >> }; >> >> +static const struct riic_of_data riic_rz_g2_info = { >> + .regs = riic_rz_a_regs, >> + .fast_mode_plus = true, >> +}; >> + >> static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { >> [RIIC_ICCR1] = 0x00, >> [RIIC_ICCR2] = 0x01, >> [RIIC_ICMR1] = 0x02, >> [RIIC_ICMR3] = 0x04, >> + [RIIC_ICFER] = 0x05, >> [RIIC_ICSER] = 0x06, >> [RIIC_ICIER] = 0x07, >> [RIIC_ICSR2] = 0x09, >> @@ -565,6 +581,7 @@ static const u8 riic_rz_v2h_regs[RIIC_REG_END] = { >> >> static const struct riic_of_data riic_rz_v2h_info = { >> .regs = riic_rz_v2h_regs, >> + .fast_mode_plus = true, >> }; >> >> static int riic_i2c_suspend(struct device *dev) @@ -613,6 +630,9 @@ static const struct dev_pm_ops >> riic_i2c_pm_ops = { >> >> static const struct of_device_id riic_i2c_dt_ids[] = { >> { .compatible = "renesas,riic-rz", .data = &riic_rz_a_info }, >> + { .compatible = "renesas,riic-r9a07g043", .data = &riic_rz_g2_info, }, >> + { .compatible = "renesas,riic-r9a07g044", .data = &riic_rz_g2_info, }, >> + { .compatible = "renesas,riic-r9a07g054", .data = &riic_rz_g2_info, >> +}, > > I feel, the better way is > > { .compatible = "renesas, renesas,r7s72100", .data = &riic_rz_a_info },--> As this SoC does not support FMP > { .compatible = "renesas,riic-rz", .data = &riic_rz_g2_info, },--> As this SoCs has FMP+ support > { .compatible = "renesas,riic-r9a09g057", .data = &riic_rz_v2h_info },--> As this SoCs has different register layout and FMP+ This is the natural way going forward if we enable it for all platforms supporting FM+ (not only for those that could be currently tested). If there are no comments against it I'll go with this compatible list. Thank you, Claudiu Beznea > > With this the number of compatible entries in the device tables reduced from 5 to 3. > > Cheers, > Biju >
Hi Claudiu, ... > Use a temporary variable for the struct device pointers to avoid > dereferencing. So far just refactoring... > While at it, replace riic->adapter.dev argument of > dev_err() from riic_init_hw() with the temporary variable (pointing to > riic->adapter.dev.parent). This is the real change in this patch and you are not explaining why you did it. ... > @@ -303,11 +304,12 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t) > int ret = 0; > unsigned long rate; > int total_ticks, cks, brl, brh; > + struct device *dev = riic->adapter.dev.parent; > > - pm_runtime_get_sync(riic->adapter.dev.parent); > + pm_runtime_get_sync(dev); > > if (t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) { > - dev_err(&riic->adapter.dev, > + dev_err(dev, > "unsupported bus speed (%dHz). %d max\n", > t->bus_freq_hz, I2C_MAX_FAST_MODE_FREQ); I personally prefer the reference to the current device, it's more traceable. If you think it's not providing enough information, then you can improve it, but I wouldn't like to lose reference to this driver in the log. Andi
Hi Claudiu, On Tue, Jun 25, 2024 at 03:13:49PM GMT, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > There is no need to runtime resume the device as long as the IP registers > are not accessed. Calling pm_runtime_get_sync() at the register access > time leads to a simpler error path. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Claudiu, > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c > index 83e4d5e14ab6..002b11b020fa 100644 > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -113,6 +113,8 @@ struct riic_irq_desc { > char *name; > }; > > +static const char * const riic_rpm_err_msg = "Failed to runtime resume"; Please, don't do this. Much clearer to write the message explicitly. > + > static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) > { > writeb(val, riic->base + riic->info->regs[offset]); > @@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > struct riic_dev *riic = i2c_get_adapdata(adap); > struct device *dev = adap->dev.parent; > unsigned long time_left; > - int i; > + int i, ret; > u8 start_bit; > > - pm_runtime_get_sync(dev); > + ret = pm_runtime_resume_and_get(dev); In principle I like the error message to be always checked and I will always approve it. Whenever there is a return value, even when we are sure it's always '0', it needs to be checked. I had lots of discussions in the past about this topic but I haven't always found support. I'd love to have the ack from a renesas maintainer here. > + if (ret) { > + dev_err(dev, riic_rpm_err_msg); > + return ret; > + } > > if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) { > riic->err = -EBUSY; ... > @@ -498,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev) > { > struct riic_dev *riic = platform_get_drvdata(pdev); > struct device *dev = &pdev->dev; > + int ret; > > - pm_runtime_get_sync(dev); > - riic_writeb(riic, 0, RIIC_ICIER); > - pm_runtime_put(dev); > i2c_del_adapter(&riic->adapter); You swapped the position of this. Please put the i2c_del_adapter() at the end. Thanks, Andi > + > + ret = pm_runtime_resume_and_get(dev); > + if (ret) { > + dev_err(dev, riic_rpm_err_msg); > + } else { > + riic_writeb(riic, 0, RIIC_ICIER); > + pm_runtime_put(dev); > + } > + > pm_runtime_disable(dev); > } > > -- > 2.39.2 >
Hi Andi, On Fri, Jul 5, 2024 at 12:42 AM Andi Shyti <andi.shyti@kernel.org> wrote: > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c > > index 83e4d5e14ab6..002b11b020fa 100644 > > --- a/drivers/i2c/busses/i2c-riic.c > > +++ b/drivers/i2c/busses/i2c-riic.c > > @@ -113,6 +113,8 @@ struct riic_irq_desc { > > char *name; > > }; > > > > +static const char * const riic_rpm_err_msg = "Failed to runtime resume"; > > Please, don't do this. Much clearer to write the message > explicitly. And the compiler will merge all identical strings, emitting just a single string. > > > + > > static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) > > { > > writeb(val, riic->base + riic->info->regs[offset]); > > @@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > struct riic_dev *riic = i2c_get_adapdata(adap); > > struct device *dev = adap->dev.parent; > > unsigned long time_left; > > - int i; > > + int i, ret; > > u8 start_bit; > > > > - pm_runtime_get_sync(dev); > > + ret = pm_runtime_resume_and_get(dev); > > In principle I like the error message to be always checked and I s/message/condition/? > will always approve it. Whenever there is a return value, even > when we are sure it's always '0', it needs to be checked. > > I had lots of discussions in the past about this topic but I > haven't always found support. I'd love to have the ack from a > renesas maintainer here. I don't mind checking for the error here. > > > + if (ret) { > > + dev_err(dev, riic_rpm_err_msg); Do you need to print these error messages? AFAIU, this cannot happen anyway. Ultimately, I expect the device driver that requested the transfer to handle failures, and print a message when needed. Gr{oetje,eeting}s, Geert
Hi Geert, > > > static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) > > > { > > > writeb(val, riic->base + riic->info->regs[offset]); > > > @@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > > struct riic_dev *riic = i2c_get_adapdata(adap); > > > struct device *dev = adap->dev.parent; > > > unsigned long time_left; > > > - int i; > > > + int i, ret; > > > u8 start_bit; > > > > > > - pm_runtime_get_sync(dev); > > > + ret = pm_runtime_resume_and_get(dev); > > > > In principle I like the error message to be always checked and I > > s/message/condition/? yes :-) > > will always approve it. Whenever there is a return value, even > > when we are sure it's always '0', it needs to be checked. > > > > I had lots of discussions in the past about this topic but I > > haven't always found support. I'd love to have the ack from a > > renesas maintainer here. > > I don't mind checking for the error here. > > > > > > + if (ret) { > > > + dev_err(dev, riic_rpm_err_msg); > > Do you need to print these error messages? I don't think it's needed, indeed. > AFAIU, this cannot happen anyway. That's what I was saying earlier. It's just a different point of view. To be honest, I don't really mind. Thanks, Andi > Ultimately, I expect the device driver that requested the transfer to > handle failures, and print a message when needed. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi, Andi, On 05.07.2024 01:30, Andi Shyti wrote: > Hi Claudiu, > > ... > >> Use a temporary variable for the struct device pointers to avoid >> dereferencing. > > So far just refactoring... > >> While at it, replace riic->adapter.dev argument of >> dev_err() from riic_init_hw() with the temporary variable (pointing to >> riic->adapter.dev.parent). > > This is the real change in this patch and you are not explaining > why you did it. > > ... > >> @@ -303,11 +304,12 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t) >> int ret = 0; >> unsigned long rate; >> int total_ticks, cks, brl, brh; >> + struct device *dev = riic->adapter.dev.parent; >> >> - pm_runtime_get_sync(riic->adapter.dev.parent); >> + pm_runtime_get_sync(dev); >> >> if (t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) { >> - dev_err(&riic->adapter.dev, >> + dev_err(dev, >> "unsupported bus speed (%dHz). %d max\n", >> t->bus_freq_hz, I2C_MAX_FAST_MODE_FREQ); > > I personally prefer the reference to the current device, it's > more traceable. If you think it's not providing enough OK, I'll keep it as it was initially. Thank you, Claudiu Beznea > information, then you can improve it, but I wouldn't like to lose > reference to this driver in the log. > > Andi
On 05.07.2024 10:19, Geert Uytterhoeven wrote: > Hi Andi, > > On Fri, Jul 5, 2024 at 12:42 AM Andi Shyti <andi.shyti@kernel.org> wrote: >>> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c >>> index 83e4d5e14ab6..002b11b020fa 100644 >>> --- a/drivers/i2c/busses/i2c-riic.c >>> +++ b/drivers/i2c/busses/i2c-riic.c >>> @@ -113,6 +113,8 @@ struct riic_irq_desc { >>> char *name; >>> }; >>> >>> +static const char * const riic_rpm_err_msg = "Failed to runtime resume"; >> >> Please, don't do this. Much clearer to write the message >> explicitly. > > And the compiler will merge all identical strings, emitting > just a single string. > >> >>> + >>> static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset) >>> { >>> writeb(val, riic->base + riic->info->regs[offset]); >>> @@ -133,10 +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >>> struct riic_dev *riic = i2c_get_adapdata(adap); >>> struct device *dev = adap->dev.parent; >>> unsigned long time_left; >>> - int i; >>> + int i, ret; >>> u8 start_bit; >>> >>> - pm_runtime_get_sync(dev); >>> + ret = pm_runtime_resume_and_get(dev); >> >> In principle I like the error message to be always checked and I > > s/message/condition/? > >> will always approve it. Whenever there is a return value, even >> when we are sure it's always '0', it needs to be checked. >> >> I had lots of discussions in the past about this topic but I >> haven't always found support. I'd love to have the ack from a >> renesas maintainer here. > > I don't mind checking for the error here. > >> >>> + if (ret) { >>> + dev_err(dev, riic_rpm_err_msg); > > Do you need to print these error messages? No. I have it here as a result of some internal reviews. Thank you, Claudiu Beznea > AFAIU, this cannot happen anyway. > Ultimately, I expect the device driver that requested the transfer to > handle failures, and print a message when needed. > > Gr{oetje,eeting}s, > > Geert >
Hi Andi and Claudiu, > -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: Thursday, July 4, 2024 11:43 PM > Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi Claudiu, > > > diff --git a/drivers/i2c/busses/i2c-riic.c > > b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa > > 100644 > > --- a/drivers/i2c/busses/i2c-riic.c > > +++ b/drivers/i2c/busses/i2c-riic.c > > @@ -113,6 +113,8 @@ struct riic_irq_desc { > > char *name; > > }; > > > > +static const char * const riic_rpm_err_msg = "Failed to runtime > > +resume"; > > Please, don't do this. Much clearer to write the message explicitly. > > > + > > static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 > > offset) { > > writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 > > +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > struct riic_dev *riic = i2c_get_adapdata(adap); > > struct device *dev = adap->dev.parent; > > unsigned long time_left; > > - int i; > > + int i, ret; > > u8 start_bit; > > > > - pm_runtime_get_sync(dev); > > + ret = pm_runtime_resume_and_get(dev); > > In principle I like the error message to be always checked and I will always approve it. Whenever > there is a return value, even when we are sure it's always '0', it needs to be checked. > > I had lots of discussions in the past about this topic but I haven't always found support. I'd love > to have the ack from a renesas maintainer here. > > > + if (ret) { Checking ret will lead to imbalance. It should be ret < 0 as ret = 1 corresponds to RPM_ACTIVE and the API does not call put() when ret = 1; see [1] and [2] [1] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L778 [2] https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/pm_runtime.h#L431 Another question, pm_runtime_put [3] can also return error, don't we need to propagate that as well to caller?? [3] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L1086 Cheers, Biju
> -----Original Message----- > From: Biju Das > Sent: Monday, July 8, 2024 6:37 AM > Subject: RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi Andi and Claudiu, > > > -----Original Message----- > > From: Andi Shyti <andi.shyti@kernel.org> > > Sent: Thursday, July 4, 2024 11:43 PM > > Subject: Re: [PATCH v2 04/12] i2c: riic: Use > > pm_runtime_resume_and_get() > > > > Hi Claudiu, > > > > > diff --git a/drivers/i2c/busses/i2c-riic.c > > > b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa > > > 100644 > > > --- a/drivers/i2c/busses/i2c-riic.c > > > +++ b/drivers/i2c/busses/i2c-riic.c > > > @@ -113,6 +113,8 @@ struct riic_irq_desc { > > > char *name; > > > }; > > > > > > +static const char * const riic_rpm_err_msg = "Failed to runtime > > > +resume"; > > > > Please, don't do this. Much clearer to write the message explicitly. > > > > > + > > > static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 > > > offset) { > > > writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 > > > +135,14 @@ static int riic_xfer(struct i2c_adapter *adap, struct > > > +i2c_msg msgs[], int num) > > > struct riic_dev *riic = i2c_get_adapdata(adap); > > > struct device *dev = adap->dev.parent; > > > unsigned long time_left; > > > - int i; > > > + int i, ret; > > > u8 start_bit; > > > > > > - pm_runtime_get_sync(dev); > > > + ret = pm_runtime_resume_and_get(dev); > > > > In principle I like the error message to be always checked and I will > > always approve it. Whenever there is a return value, even when we are sure it's always '0', it > needs to be checked. > > > > I had lots of discussions in the past about this topic but I haven't > > always found support. I'd love to have the ack from a renesas maintainer here. > > > > > + if (ret) { > > Checking ret will lead to imbalance. It should be ret < 0 as ret = 1 corresponds to RPM_ACTIVE and > the API does not call put() when ret = 1; see [1] and [2] > > [1] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L778 > > [2] https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/pm_runtime.h#L431 I got confused. the code for pm_runtime_resume_and_get() seems correct. https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L436 > > > Another question, pm_runtime_put [3] can also return error, don't we need to propagate that as well > to caller?? > > [3] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L1086 > > Cheers, > Biju
Hi Claudiu, On Wed, Jul 10, 2024 at 4:20 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > On 28.06.2024 12:22, Geert Uytterhoeven wrote: > > On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> Fast mode plus is available on most of the IP variants that RIIC driver > >> is working with. The exception is (according to HW manuals of the SoCs > >> where this IP is available) the Renesas RZ/A1H. For this, patch > >> introduces the struct riic_of_data::fast_mode_plus. > >> > >> Fast mode plus was tested on RZ/G3S, RZ/G2{L,UL,LC}, RZ/Five by > >> instantiating the RIIC frequency to 1MHz and issuing i2c reads on the > >> fast mode plus capable devices (and the i2c clock frequency was checked on > >> RZ/G3S). > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Thanks for your patch! > > > >> --- a/drivers/i2c/busses/i2c-riic.c > >> +++ b/drivers/i2c/busses/i2c-riic.c > >> @@ -407,6 +413,9 @@ static int riic_init_hw(struct riic_dev *riic) > >> riic_writeb(riic, 0, RIIC_ICSER); > >> riic_writeb(riic, ICMR3_ACKWP | ICMR3_RDRFS, RIIC_ICMR3); > >> > >> + if (info->fast_mode_plus && t->bus_freq_hz == I2C_MAX_FAST_MODE_PLUS_FREQ) > >> + riic_clear_set_bit(riic, 0, ICFER_FMPE, RIIC_ICFER); > > > > Unless FM+ is specified, RIIC_ICFER is never written to. > > Probably the register should always be initialized, also to make sure > > the FMPE bit is cleared when it was set by the boot loader, but FM+ > > is not to be used. > > Instead of clearing only this bit, what do you think about using > reset_control_reset() instead of reset_control_deassert() in riic_i2c_probe()? > > HW manuals for all the devices listed in > Documentation/devicetree/bindings/i2c/renesas,riic.yaml specifies that > ICFER_FMPE register is initialized with a default value by reset. All the > other registers are initialized with default values at reset (according to > HW manuals). I've checked it on RZ/G3S and it behaves like this. RZ/A1 and RZ/A2M do not have reset controller support yet, so calling reset_control_reset() is a no-op on these SoCs. However, I overlooked that riic_init_hw() does an internal reset first by setting the ICCR1_IICRST bit in RIIC_ICCR1. Is that sufficient to reset the FMPE bit? Gr{oetje,eeting}s, Geert
Hi, Geert, On 11.07.2024 10:53, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Wed, Jul 10, 2024 at 4:20 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: >> On 28.06.2024 12:22, Geert Uytterhoeven wrote: >>> On Tue, Jun 25, 2024 at 2:14 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> Fast mode plus is available on most of the IP variants that RIIC driver >>>> is working with. The exception is (according to HW manuals of the SoCs >>>> where this IP is available) the Renesas RZ/A1H. For this, patch >>>> introduces the struct riic_of_data::fast_mode_plus. >>>> >>>> Fast mode plus was tested on RZ/G3S, RZ/G2{L,UL,LC}, RZ/Five by >>>> instantiating the RIIC frequency to 1MHz and issuing i2c reads on the >>>> fast mode plus capable devices (and the i2c clock frequency was checked on >>>> RZ/G3S). >>>> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> Thanks for your patch! >>> >>>> --- a/drivers/i2c/busses/i2c-riic.c >>>> +++ b/drivers/i2c/busses/i2c-riic.c >>>> @@ -407,6 +413,9 @@ static int riic_init_hw(struct riic_dev *riic) >>>> riic_writeb(riic, 0, RIIC_ICSER); >>>> riic_writeb(riic, ICMR3_ACKWP | ICMR3_RDRFS, RIIC_ICMR3); >>>> >>>> + if (info->fast_mode_plus && t->bus_freq_hz == I2C_MAX_FAST_MODE_PLUS_FREQ) >>>> + riic_clear_set_bit(riic, 0, ICFER_FMPE, RIIC_ICFER); >>> >>> Unless FM+ is specified, RIIC_ICFER is never written to. >>> Probably the register should always be initialized, also to make sure >>> the FMPE bit is cleared when it was set by the boot loader, but FM+ >>> is not to be used. >> >> Instead of clearing only this bit, what do you think about using >> reset_control_reset() instead of reset_control_deassert() in riic_i2c_probe()? >> >> HW manuals for all the devices listed in >> Documentation/devicetree/bindings/i2c/renesas,riic.yaml specifies that >> ICFER_FMPE register is initialized with a default value by reset. All the >> other registers are initialized with default values at reset (according to >> HW manuals). I've checked it on RZ/G3S and it behaves like this. > > RZ/A1 and RZ/A2M do not have reset controller support yet, so calling > reset_control_reset() is a no-op on these SoCs. > > However, I overlooked that riic_init_hw() does an internal reset first > by setting the ICCR1_IICRST bit in RIIC_ICCR1. > Is that sufficient to reset the FMPE bit? Yes, that also restores the content of RIIC_ICFER register to the default value (including clearing the FMPE bit). Thanks for pointing it, I overlooked it, too. Thank you, Claudiu Beznea > > Gr{oetje,eeting}s, > > Geert >
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
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Hi, Series adds I2C support for the Renesas RZ/G3S SoC. Series is split as follows: - patch 01/12 - add clock, reset and PM domain support - patch 02-03/12 - add some cleanups on RIIC driver - patch 05/12 - enable runtime autosuspend support on the RIIC driver - patch 06/12 - add suspend to RAM support on the RIIC driver - patch 07/12 - prepares for the addition of fast mode plus - patch 08/12 - updates the I2C documentation for the RZ/G3S SoC - patch 09/12 - add fast mode plus support on the RIIC driver - patches 10-12/12 - device tree support Thank you, Claudiu Beznea Changes in v2: - change the i2c clock names to match the documentation - update commit description for patch "i2c: riic: Use temporary variable for struct device" - addressed review comments - dropped renesas,riic-no-fast-mode-plus DT property and associated code Claudiu Beznea (12): clk: renesas: r9a08g045: Add clock, reset and power domain support for I2C i2c: riic: Use temporary variable for struct device i2c: riic: Call pm_runtime_get_sync() when need to access registers i2c: riic: Use pm_runtime_resume_and_get() i2c: riic: Enable runtime PM autosuspend support i2c: riic: Add suspend/resume support i2c: riic: Define individual arrays to describe the register offsets dt-bindings: i2c: renesas,riic: Document the R9A08G045 support i2c: riic: Add support for fast mode plus arm64: dts: renesas: r9a08g045: Add I2C nodes arm64: dts: renesas: rzg3s-smarc: Enable i2c0 node arm64: dts: renesas: rzg3s-smarc-som: Enable i2c1 node .../devicetree/bindings/i2c/renesas,riic.yaml | 4 + arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 88 +++++++ .../boot/dts/renesas/rzg3s-smarc-som.dtsi | 5 + arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi | 7 + drivers/clk/renesas/r9a08g045-cpg.c | 20 ++ drivers/i2c/busses/i2c-riic.c | 237 ++++++++++++------ 6 files changed, 289 insertions(+), 72 deletions(-)