[PATCH v2 00/12] i2c: riic: Add support for Renesas RZ/G3S

[PATCH v2 00/12] i2c: riic: Add support for Renesas RZ/G3S

[Claudiu]

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(-)

-- 
2.39.2

[PATCH v2 01/12] clk: renesas: r9a08g045: Add clock, reset and power domain support for I2C

[Claudiu]

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

 drivers/clk/renesas/r9a08g045-cpg.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index b068733b145f..5f372d73c3ac 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -213,6 +213,10 @@ static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
 	DEF_COUPLED("eth1_axi",		R9A08G045_ETH1_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 1),
 	DEF_COUPLED("eth1_chi",		R9A08G045_ETH1_CLK_CHI, R9A08G045_CLK_ZT, 0x57c, 1),
 	DEF_MOD("eth1_refclk",		R9A08G045_ETH1_REFCLK, R9A08G045_CLK_HP, 0x57c, 9),
+	DEF_MOD("i2c0_pclk",		R9A08G045_I2C0_PCLK, R9A08G045_CLK_P0, 0x580, 0),
+	DEF_MOD("i2c1_pclk",		R9A08G045_I2C1_PCLK, R9A08G045_CLK_P0, 0x580, 1),
+	DEF_MOD("i2c2_pclk",		R9A08G045_I2C2_PCLK, R9A08G045_CLK_P0, 0x580, 2),
+	DEF_MOD("i2c3_pclk",		R9A08G045_I2C3_PCLK, R9A08G045_CLK_P0, 0x580, 3),
 	DEF_MOD("scif0_clk_pck",	R9A08G045_SCIF0_CLK_PCK, R9A08G045_CLK_P0, 0x584, 0),
 	DEF_MOD("gpio_hclk",		R9A08G045_GPIO_HCLK, R9A08G045_OSCCLK, 0x598, 0),
 };
@@ -227,6 +231,10 @@ static const struct rzg2l_reset r9a08g045_resets[] = {
 	DEF_RST(R9A08G045_SDHI2_IXRST, 0x854, 2),
 	DEF_RST(R9A08G045_ETH0_RST_HW_N, 0x87c, 0),
 	DEF_RST(R9A08G045_ETH1_RST_HW_N, 0x87c, 1),
+	DEF_RST(R9A08G045_I2C0_MRST, 0x880, 0),
+	DEF_RST(R9A08G045_I2C1_MRST, 0x880, 1),
+	DEF_RST(R9A08G045_I2C2_MRST, 0x880, 2),
+	DEF_RST(R9A08G045_I2C3_MRST, 0x880, 3),
 	DEF_RST(R9A08G045_SCIF0_RST_SYSTEM_N, 0x884, 0),
 	DEF_RST(R9A08G045_GPIO_RSTN, 0x898, 0),
 	DEF_RST(R9A08G045_GPIO_PORT_RESETN, 0x898, 1),
@@ -272,6 +280,18 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
 	DEF_PD("eth1",		R9A08G045_PD_ETHER1,
 				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(3)),
 				RZG2L_PD_F_NONE),
+	DEF_PD("i2c0",		R9A08G045_PD_I2C0,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(10)),
+				RZG2L_PD_F_NONE),
+	DEF_PD("i2c1",		R9A08G045_PD_I2C1,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(11)),
+				RZG2L_PD_F_NONE),
+	DEF_PD("i2c2",		R9A08G045_PD_I2C2,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(12)),
+				RZG2L_PD_F_NONE),
+	DEF_PD("i2c3",		R9A08G045_PD_I2C3,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(13)),
+				RZG2L_PD_F_NONE),
 	DEF_PD("scif0",		R9A08G045_PD_SCIF0,
 				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(1)),
 				RZG2L_PD_F_NONE),
-- 
2.39.2

Re: [PATCH v2 01/12] clk: renesas: r9a08g045: Add clock, reset and power domain support for I2C

[Geert Uytterhoeven]

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

-- 
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

[PATCH v2 02/12] i2c: riic: Use temporary variable for struct device

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use a temporary variable for the struct device pointers to avoid
dereferencing. 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).

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- updated commit description to reflect all the changes done

 drivers/i2c/busses/i2c-riic.c | 53 ++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index f608b1838cad..c08c988f50c7 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -131,11 +131,12 @@ static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u
 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;
 	u8 start_bit;
 
-	pm_runtime_get_sync(adap->dev.parent);
+	pm_runtime_get_sync(dev);
 
 	if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
 		riic->err = -EBUSY;
@@ -168,7 +169,7 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	}
 
  out:
-	pm_runtime_put(adap->dev.parent);
+	pm_runtime_put(dev);
 
 	return riic->err ?: num;
 }
@@ -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);
 		ret = -EINVAL;
@@ -347,7 +349,7 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 	}
 
 	if (brl > (0x1F + 3)) {
-		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
+		dev_err(dev, "invalid speed (%lu). Too slow.\n",
 			(unsigned long)t->bus_freq_hz);
 		ret = -EINVAL;
 		goto out;
@@ -396,7 +398,7 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
 
 out:
-	pm_runtime_put(riic->adapter.dev.parent);
+	pm_runtime_put(dev);
 	return ret;
 }
 
@@ -415,13 +417,14 @@ static void riic_reset_control_assert(void *data)
 
 static int riic_i2c_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct riic_dev *riic;
 	struct i2c_adapter *adap;
 	struct i2c_timings i2c_t;
 	struct reset_control *rstc;
 	int i, ret;
 
-	riic = devm_kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL);
+	riic = devm_kzalloc(dev, sizeof(*riic), GFP_KERNEL);
 	if (!riic)
 		return -ENOMEM;
 
@@ -429,22 +432,22 @@ static int riic_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(riic->base))
 		return PTR_ERR(riic->base);
 
-	riic->clk = devm_clk_get(&pdev->dev, NULL);
+	riic->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(riic->clk)) {
-		dev_err(&pdev->dev, "missing controller clock");
+		dev_err(dev, "missing controller clock");
 		return PTR_ERR(riic->clk);
 	}
 
-	rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
 	if (IS_ERR(rstc))
-		return dev_err_probe(&pdev->dev, PTR_ERR(rstc),
+		return dev_err_probe(dev, PTR_ERR(rstc),
 				     "Error: missing reset ctrl\n");
 
 	ret = reset_control_deassert(rstc);
 	if (ret)
 		return ret;
 
-	ret = devm_add_action_or_reset(&pdev->dev, riic_reset_control_assert, rstc);
+	ret = devm_add_action_or_reset(dev, riic_reset_control_assert, rstc);
 	if (ret)
 		return ret;
 
@@ -453,29 +456,29 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		if (ret < 0)
 			return ret;
 
-		ret = devm_request_irq(&pdev->dev, ret, riic_irqs[i].isr,
+		ret = devm_request_irq(dev, ret, riic_irqs[i].isr,
 				       0, riic_irqs[i].name, riic);
 		if (ret) {
-			dev_err(&pdev->dev, "failed to request irq %s\n", riic_irqs[i].name);
+			dev_err(dev, "failed to request irq %s\n", riic_irqs[i].name);
 			return ret;
 		}
 	}
 
-	riic->info = of_device_get_match_data(&pdev->dev);
+	riic->info = of_device_get_match_data(dev);
 
 	adap = &riic->adapter;
 	i2c_set_adapdata(adap, riic);
 	strscpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name));
 	adap->owner = THIS_MODULE;
 	adap->algo = &riic_algo;
-	adap->dev.parent = &pdev->dev;
-	adap->dev.of_node = pdev->dev.of_node;
+	adap->dev.parent = dev;
+	adap->dev.of_node = dev->of_node;
 
 	init_completion(&riic->msg_done);
 
-	i2c_parse_fw_timings(&pdev->dev, &i2c_t, true);
+	i2c_parse_fw_timings(dev, &i2c_t, true);
 
-	pm_runtime_enable(&pdev->dev);
+	pm_runtime_enable(dev);
 
 	ret = riic_init_hw(riic, &i2c_t);
 	if (ret)
@@ -487,24 +490,24 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, riic);
 
-	dev_info(&pdev->dev, "registered with %dHz bus speed\n",
-		 i2c_t.bus_freq_hz);
+	dev_info(dev, "registered with %dHz bus speed\n", i2c_t.bus_freq_hz);
 	return 0;
 
 out:
-	pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(dev);
 	return ret;
 }
 
 static void riic_i2c_remove(struct platform_device *pdev)
 {
 	struct riic_dev *riic = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
 
-	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_get_sync(dev);
 	riic_writeb(riic, 0, RIIC_ICIER);
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 	i2c_del_adapter(&riic->adapter);
-	pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(dev);
 }
 
 static const struct riic_of_data riic_rz_a_info = {
-- 
2.39.2

Re: [PATCH v2 02/12] i2c: riic: Use temporary variable for struct device

[Andi Shyti]

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

Re: [PATCH v2 02/12] i2c: riic: Use temporary variable for struct device

[claudiu beznea]

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

[PATCH v2 03/12] i2c: riic: Call pm_runtime_get_sync() when need to access registers

[Claudiu]

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>
---

Changes in v2:
- none

 drivers/i2c/busses/i2c-riic.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index c08c988f50c7..83e4d5e14ab6 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -301,19 +301,15 @@ static const struct i2c_algorithm riic_algo = {
 
 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(dev);
-
 	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);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	rate = clk_get_rate(riic->clk);
@@ -351,8 +347,7 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 	if (brl > (0x1F + 3)) {
 		dev_err(dev, "invalid speed (%lu). Too slow.\n",
 			(unsigned long)t->bus_freq_hz);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	brh = total_ticks - brl;
@@ -384,6 +379,8 @@ 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);
+
 	/* Changing the order of accessing IICRST and ICE may break things! */
 	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
 	riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
@@ -397,9 +394,8 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 
 	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
 
-out:
 	pm_runtime_put(dev);
-	return ret;
+	return 0;
 }
 
 static struct riic_irq_desc riic_irqs[] = {
-- 
2.39.2

Re: [PATCH v2 03/12] i2c: riic: Call pm_runtime_get_sync() when need to access registers

[Andi Shyti]

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

[PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Claudiu]

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>
---

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);
+		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

RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Biju Das]

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
> 

Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[claudiu beznea]

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
>>
> 

RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Biju Das]

> -----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
> >>
> >

Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[claudiu beznea]

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
>>>>
>>>

Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Andi Shyti]

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

Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Geert Uytterhoeven]

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

-- 
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

RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Biju Das]

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

Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Andi Shyti]

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
> 

Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Andi Shyti]

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

Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[claudiu beznea]

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
> 

RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Biju Das]

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

RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get()

[Biju Das]

> -----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

[PATCH v2 05/12] i2c: riic: Enable runtime PM autosuspend support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Enable runtime PM autosuspend support for the RIIC driver. With this, in
case there are consecutive xfer requests the device wouldn't be runtime
enabled/disabled after each consecutive xfer but after the
the delay configured by user. With this, we can avoid touching hardware
registers involved in runtime PM suspend/resume saving in this way some
cycles. The default chosen autosuspend delay is zero to keep the
previous driver behavior.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none

 drivers/i2c/busses/i2c-riic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 002b11b020fa..24c0d62544fb 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -175,7 +175,8 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	}
 
  out:
-	pm_runtime_put(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 
 	return riic->err ?: num;
 }
@@ -405,7 +406,8 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 
 	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
 
-	pm_runtime_put(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 	return 0;
 }
 
@@ -485,6 +487,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 	i2c_parse_fw_timings(dev, &i2c_t, true);
 
+	pm_runtime_set_autosuspend_delay(dev, 0);
+	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
 	ret = riic_init_hw(riic, &i2c_t);
@@ -502,6 +506,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 out:
 	pm_runtime_disable(dev);
+	pm_runtime_dont_use_autosuspend(dev);
 	return ret;
 }
 
@@ -522,6 +527,7 @@ static void riic_i2c_remove(struct platform_device *pdev)
 	}
 
 	pm_runtime_disable(dev);
+	pm_runtime_dont_use_autosuspend(dev);
 }
 
 static const struct riic_of_data riic_rz_a_info = {
-- 
2.39.2

[PATCH v2 06/12] i2c: riic: Add suspend/resume support

[Claudiu]

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>
---

Changes in v2:
- deassert the reset line in resume if riic_init_hw() fails as
  if that happens there is no way to recover the controller

 drivers/i2c/busses/i2c-riic.c | 68 +++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 24c0d62544fb..9fe007609076 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -105,6 +105,8 @@ struct riic_dev {
 	struct completion msg_done;
 	struct i2c_adapter adapter;
 	struct clk *clk;
+	struct reset_control *rstc;
+	struct i2c_timings i2c_t;
 };
 
 struct riic_irq_desc {
@@ -306,11 +308,12 @@ static const struct i2c_algorithm riic_algo = {
 	.functionality	= riic_func,
 };
 
-static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
+static int riic_init_hw(struct riic_dev *riic)
 {
 	int ret;
 	unsigned long rate;
 	int total_ticks, cks, brl, brh;
+	struct i2c_timings *t = &riic->i2c_t;
 	struct device *dev = riic->adapter.dev.parent;
 
 	if (t->bus_freq_hz > I2C_MAX_FAST_MODE_FREQ) {
@@ -429,8 +432,6 @@ static int riic_i2c_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct riic_dev *riic;
 	struct i2c_adapter *adap;
-	struct i2c_timings i2c_t;
-	struct reset_control *rstc;
 	int i, ret;
 
 	riic = devm_kzalloc(dev, sizeof(*riic), GFP_KERNEL);
@@ -447,16 +448,16 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(riic->clk);
 	}
 
-	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
-	if (IS_ERR(rstc))
-		return dev_err_probe(dev, PTR_ERR(rstc),
+	riic->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(riic->rstc))
+		return dev_err_probe(dev, PTR_ERR(riic->rstc),
 				     "Error: missing reset ctrl\n");
 
-	ret = reset_control_deassert(rstc);
+	ret = reset_control_deassert(riic->rstc);
 	if (ret)
 		return ret;
 
-	ret = devm_add_action_or_reset(dev, riic_reset_control_assert, rstc);
+	ret = devm_add_action_or_reset(dev, riic_reset_control_assert, riic->rstc);
 	if (ret)
 		return ret;
 
@@ -485,13 +486,13 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 	init_completion(&riic->msg_done);
 
-	i2c_parse_fw_timings(dev, &i2c_t, true);
+	i2c_parse_fw_timings(dev, &riic->i2c_t, true);
 
 	pm_runtime_set_autosuspend_delay(dev, 0);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
-	ret = riic_init_hw(riic, &i2c_t);
+	ret = riic_init_hw(riic);
 	if (ret)
 		goto out;
 
@@ -501,7 +502,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, riic);
 
-	dev_info(dev, "registered with %dHz bus speed\n", i2c_t.bus_freq_hz);
+	dev_info(dev, "registered with %dHz bus speed\n", riic->i2c_t.bus_freq_hz);
 	return 0;
 
 out:
@@ -562,6 +563,50 @@ static const struct riic_of_data riic_rz_v2h_info = {
 	},
 };
 
+static int riic_i2c_suspend(struct device *dev)
+{
+	struct riic_dev *riic = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	i2c_mark_adapter_suspended(&riic->adapter);
+
+	/* Disable output on SDA, SCL pins. */
+	riic_clear_set_bit(riic, ICCR1_ICE, 0, RIIC_ICCR1);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_sync(dev);
+
+	return reset_control_assert(riic->rstc);
+}
+
+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);
+		return ret;
+	}
+
+	i2c_mark_adapter_resumed(&riic->adapter);
+
+	return 0;
+}
+
+static const struct dev_pm_ops riic_i2c_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(riic_i2c_suspend, riic_i2c_resume)
+};
+
 static const struct of_device_id riic_i2c_dt_ids[] = {
 	{ .compatible = "renesas,riic-rz", .data = &riic_rz_a_info },
 	{ .compatible = "renesas,riic-r9a09g057", .data = &riic_rz_v2h_info },
@@ -574,6 +619,7 @@ static struct platform_driver riic_i2c_driver = {
 	.driver		= {
 		.name	= "i2c-riic",
 		.of_match_table = riic_i2c_dt_ids,
+		.pm	= pm_ptr(&riic_i2c_pm_ops),
 	},
 };
 
-- 
2.39.2

[PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Claudiu]

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;
 };
 
 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

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

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
> 

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea]

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
>>
> 

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

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
> >>
> >

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea]

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
>>>>
>>>

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea]

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
>>>>>
>>>>

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

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

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea]

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
> 

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

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

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

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

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea]

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

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

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

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea]

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

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

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

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

> 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

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea]

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
> 

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea]

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
> 

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Andi Shyti]

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

Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Geert Uytterhoeven]

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

-- 
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

RE: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[Biju Das]

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

[PATCH v2 08/12] dt-bindings: i2c: renesas,riic: Document the R9A08G045 support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Document the Renesas RZ/G3S (R9A08G045) RIIC IP. This is compatible with
the version available on Renesas RZ/V2H (R9A09G075).

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- dropped the renesas,riic-no-fast-mode-plus
- updated commit description

 Documentation/devicetree/bindings/i2c/renesas,riic.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
index 91ecf17b7a81..dde37a209b6e 100644
--- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
+++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
@@ -25,6 +25,10 @@ properties:
               - renesas,riic-r9a07g054  # RZ/V2L
           - const: renesas,riic-rz      # RZ/A or RZ/G2L
 
+      - items:
+          - const: renesas,riic-r9a08g045   # RZ/G3S
+          - const: renesas,riic-r9a09g057
+
       - const: renesas,riic-r9a09g057   # RZ/V2H(P)
 
   reg:
-- 
2.39.2

Re: [PATCH v2 08/12] dt-bindings: i2c: renesas,riic: Document the R9A08G045 support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On Tue, Jun 25, 2024 at 03:13:54PM +0300, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Document the Renesas RZ/G3S (R9A08G045) RIIC IP. This is compatible with
> the version available on Renesas RZ/V2H (R9A09G075).
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[Claudiu]

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, },
 	{ .compatible = "renesas,riic-r9a09g057", .data = &riic_rz_v2h_info },
 	{ /* Sentinel */ },
 };
-- 
2.39.2

Re: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[claudiu beznea]

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
> 

Re: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[claudiu beznea]

Hi, Geert, all,

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.

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.

With this:

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index ba969ad5f015..150e7841f178 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -457,7 +457,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
                return dev_err_probe(dev, PTR_ERR(riic->rstc),
                                     "Error: missing reset ctrl\n");

-       ret = reset_control_deassert(riic->rstc);
+       ret = reset_control_reset(riic->rstc);
        if (ret)
                return ret;

I've did basic tests (i2cdetect + i2cget with FM+ frequency) on RZ/G2{L,
LC, UL}, RZ/V2L and all was good.

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
> 

Re: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[claudiu beznea]

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
> 

RE: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[Biju Das]

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

Re: [PATCH v2 09/12] i2c: riic: Add support for fast mode plus

[claudiu beznea]

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
> 

[PATCH v2 10/12] arm64: dts: renesas: r9a08g045: Add I2C nodes

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S has 4 I2C channels. Add DT nodes for it.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- dropped renesas,riic-no-fast-mode-plus

 arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 88 ++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index 2162c247d6de..387d74a163ee 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -72,6 +72,94 @@ scif0: serial@1004b800 {
 			status = "disabled";
 		};
 
+		i2c0: i2c@10090000 {
+			compatible = "renesas,riic-r9a08g045", "renesas,riic-r9a09g057";
+			reg = <0 0x10090000 0 0x400>;
+			interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 263 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 260 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tei", "ri", "ti", "spi", "sti",
+					  "naki", "ali", "tmoi";
+			clocks = <&cpg CPG_MOD R9A08G045_I2C0_PCLK>;
+			clock-frequency = <100000>;
+			resets = <&cpg R9A08G045_I2C0_MRST>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
+		i2c1: i2c@10090400 {
+			compatible = "renesas,riic-r9a08g045", "renesas,riic-r9a09g057";
+			reg = <0 0x10090400 0 0x400>;
+			interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 271 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 272 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tei", "ri", "ti", "spi", "sti",
+					  "naki", "ali", "tmoi";
+			clocks = <&cpg CPG_MOD R9A08G045_I2C1_PCLK>;
+			clock-frequency = <100000>;
+			resets = <&cpg R9A08G045_I2C1_MRST>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
+		i2c2: i2c@10090800 {
+			compatible = "renesas,riic-r9a08g045", "renesas,riic-r9a09g057";
+			reg = <0 0x10090800 0 0x400>;
+			interrupts = <GIC_SPI 273 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 279 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 280 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 275 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 276 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 274 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 277 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tei", "ri", "ti", "spi", "sti",
+					  "naki", "ali", "tmoi";
+			clocks = <&cpg CPG_MOD R9A08G045_I2C2_PCLK>;
+			clock-frequency = <100000>;
+			resets = <&cpg R9A08G045_I2C2_MRST>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
+		i2c3: i2c@10090c00 {
+			compatible = "renesas,riic-r9a08g045", "renesas,riic-r9a09g057";
+			reg = <0 0x10090c00 0 0x400>;
+			interrupts = <GIC_SPI 281 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 287 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 288 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 286 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tei", "ri", "ti", "spi", "sti",
+					  "naki", "ali", "tmoi";
+			clocks = <&cpg CPG_MOD R9A08G045_I2C3_PCLK>;
+			clock-frequency = <100000>;
+			resets = <&cpg R9A08G045_I2C3_MRST>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		cpg: clock-controller@11010000 {
 			compatible = "renesas,r9a08g045-cpg";
 			reg = <0 0x11010000 0 0x10000>;
-- 
2.39.2

[PATCH v2 11/12] arm64: dts: renesas: rzg3s-smarc: Enable i2c0 node

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Enable i2c0 node.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
index deb2ad37bb2e..7945d44e6ee1 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
@@ -11,6 +11,7 @@
 
 / {
 	aliases {
+		i2c0 = &i2c0;
 		serial0 = &scif0;
 		mmc1 = &sdhi1;
 	};
@@ -66,6 +67,12 @@ vccq_sdhi1: regulator-vccq-sdhi1 {
 	};
 };
 
+&i2c0 {
+	status = "okay";
+
+	clock-frequency = <1000000>;
+};
+
 &pinctrl {
 	key-1-gpio-hog {
 		gpio-hog;
-- 
2.39.2

[PATCH v2 12/12] arm64: dts: renesas: rzg3s-smarc-som: Enable i2c1 node

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Enable i2c1 node.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index 8a3d302f1535..21bfa4e03972 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -32,6 +32,7 @@ / {
 	compatible = "renesas,rzg3s-smarcm", "renesas,r9a08g045s33", "renesas,r9a08g045";
 
 	aliases {
+		i2c1 = &i2c1;
 		mmc0 = &sdhi0;
 #if SW_CONFIG3 == SW_OFF
 		mmc2 = &sdhi2;
@@ -150,6 +151,10 @@ &extal_clk {
 	clock-frequency = <24000000>;
 };
 
+&i2c1 {
+	status = "okay";
+};
+
 #if SW_CONFIG2 == SW_ON
 /* SD0 slot */
 &sdhi0 {
-- 
2.39.2