| Message ID | 20240711115207.2843133-1-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | i2c: riic: Add support for Renesas RZ/G3S | expand |
Hi all,
> Series adds I2C support for the Renesas RZ/G3S SoC.
So, I will review this series today. I was hoping to also test it on a
Genmai board (R7S72100) but I have issues with it. If I cannot boot from
it until tomorrow, it will be review only from my side.
Happy hacking,
Wolfram
On Thu, Jul 11, 2024 at 02:51:57PM +0300, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Use a temporary variable for the struct device pointers to avoid > dereferencing. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Looks good, builds fine: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On Thu, Jul 11, 2024 at 02:51:59PM +0300, Claudiu wrote: > 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. pm_runtime_resume_and_get() > takes care of this. Thus use it. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Looks valid to me: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On Thu, Jul 11, 2024 at 02:52:01PM +0300, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Add suspend/resume support for the RIIC driver. This is necessary for the > Renesas RZ/G3S SoC which support suspend to deep sleep state where power > to most of the SoC components is turned off. As a result the I2C controller > needs to be reconfigured after suspend/resume. For this, the reset line > was stored in the driver private data structure as well as i2c timings. > The reset line and I2C timings are necessary to re-initialize the > controller after resume. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> ? Doesn't apply on top of the previous patches for me? > +static int riic_i2c_resume(struct device *dev) > +{ > + struct riic_dev *riic = dev_get_drvdata(dev); > + int ret; > + > + ret = reset_control_deassert(riic->rstc); > + if (ret) > + return ret; > + > + ret = riic_init_hw(riic); > + if (ret) { > + reset_control_assert(riic->rstc); Is this assertion really needed? It is not done when init_hw fails in probe().
> + - const: renesas,riic-r9a08g045 # RZ/G3S > + - const: renesas,riic-r9a09g057 Why no comment after the latter one?
On 08.08.2024 18:15, Wolfram Sang wrote: > On Thu, Jul 11, 2024 at 02:52:01PM +0300, Claudiu wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Add suspend/resume support for the RIIC driver. This is necessary for the >> Renesas RZ/G3S SoC which support suspend to deep sleep state where power >> to most of the SoC components is turned off. As a result the I2C controller >> needs to be reconfigured after suspend/resume. For this, the reset line >> was stored in the driver private data structure as well as i2c timings. >> The reset line and I2C timings are necessary to re-initialize the >> controller after resume. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > ? Doesn't apply on top of the previous patches for me? I just checked it on next-20240809. It should be due to commit e1571b1fb4ff ("i2c: riic: reword according to newest specification") which introduced changes around riic_algo object, present also in the diff of this patch. > >> +static int riic_i2c_resume(struct device *dev) >> +{ >> + struct riic_dev *riic = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = reset_control_deassert(riic->rstc); >> + if (ret) >> + return ret; >> + >> + ret = riic_init_hw(riic); >> + if (ret) { >> + reset_control_assert(riic->rstc); > > Is this assertion really needed? It is not done when init_hw fails in > probe(). In case riic_init_hw() fails there is no recovering way for this driver, AFAICT, and thus there is no point in keeping the reset signal de-asserted. In probe this is handled by the devres through action or reset function (riic_reset_control_assert) registered by: ret = devm_add_action_or_reset(dev, riic_reset_control_assert, rstc); if (ret) return ret; Thank you, Claudiu Beznea
On 08.08.2024 18:21, Wolfram Sang wrote: > >> + - const: renesas,riic-r9a08g045 # RZ/G3S >> + - const: renesas,riic-r9a09g057 > > Why no comment after the latter one? > I kept it like this to avoid confusion b/w RZ/G3S and RZ/V2H(P) documented below, as the RZ/G3S falls back to renesas,riic-r9a09g057 (RZ/V2H(P)). I can add a comment here, too, if you still consider necessary. Please let me know. Thank you, Claudiu Beznea
> I just checked it on next-20240809. It should be due to commit > e1571b1fb4ff ("i2c: riic: reword according to newest specification") > which introduced changes around riic_algo object, present also in the diff > of this patch. Ah, okay, this patch is the culprit. I wonder, though, because it is already in 6.11-rc1 which was the base for my test. But you need to resend anyhow... > In case riic_init_hw() fails there is no recovering way for this driver, > AFAICT, and thus there is no point in keeping the reset signal de-asserted. Right. If it fails in resume(), then the driver will still not be removed.
> I kept it like this to avoid confusion b/w RZ/G3S and RZ/V2H(P) documented > below, as the RZ/G3S falls back to renesas,riic-r9a09g057 (RZ/V2H(P)). > > I can add a comment here, too, if you still consider necessary. Please let > me know. I see. I don't know how such fallbacks are documented usually, so I won't consider it necessary. I was just wondering about it. Let's just be consistent with previous fallbacks, however they were handled.
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-03/12 - add some cleanups on RIIC driver - patch 04/12 - enable runtime autosuspend support on the RIIC driver - patch 05/12 - add suspend to RAM support on the RIIC driver - patch 06/12 - prepares for the addition of fast mode plus - patch 07/12 - updates the I2C documentation for the RZ/G3S SoC - patch 08/12 - add fast mode plus support on the RIIC driver - patches 09-11/11 - device tree support Thank you, Claudiu Beznea Changes in v3: - dropped patch "clk: renesas: r9a08g045: Add clock, reset and power domain support for I2C" as it was already integrated - addressed review comments 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 (11): 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/i2c/busses/i2c-riic.c | 220 ++++++++++++------ 5 files changed, 255 insertions(+), 69 deletions(-)