mbox series

[v3,00/11] i2c: riic: Add support for Renesas RZ/G3S

Message ID 20240711115207.2843133-1-claudiu.beznea.uj@bp.renesas.com
Headers show
Series i2c: riic: Add support for Renesas RZ/G3S | expand

Message

Claudiu Beznea July 11, 2024, 11:51 a.m. UTC
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(-)

Comments

Wolfram Sang Aug. 8, 2024, 8:38 a.m. UTC | #1
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
Wolfram Sang Aug. 8, 2024, 2:59 p.m. UTC | #2
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>
Wolfram Sang Aug. 8, 2024, 3:03 p.m. UTC | #3
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>
Wolfram Sang Aug. 8, 2024, 3:15 p.m. UTC | #4
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().
Wolfram Sang Aug. 8, 2024, 3:21 p.m. UTC | #5
> +          - const: renesas,riic-r9a08g045   # RZ/G3S
> +          - const: renesas,riic-r9a09g057

Why no comment after the latter one?
Claudiu Beznea Aug. 9, 2024, 7:08 a.m. UTC | #6
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
Claudiu Beznea Aug. 9, 2024, 7:20 a.m. UTC | #7
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
Wolfram Sang Aug. 9, 2024, 1:35 p.m. UTC | #8
> 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.
Wolfram Sang Aug. 9, 2024, 1:39 p.m. UTC | #9
> 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.