[PATCH 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[PATCH 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Phil Edworthy]

Hi,

The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series
add the driver. One annoying problem is that the SoC uses a single reset
line for two i2c controllers, and unfortunately one of the controllers
is managed by some firmware, not by Linux. Therefore, the driver just
deasserts the reset.

Phil Edworthy (2):
  dt-bindings: i2c: Document RZ/V2M I2C controller
  i2c: Add Renesas RZ/V2M controller

 .../bindings/i2c/renesas,rzv2m.yaml           |  76 +++
 drivers/i2c/busses/Kconfig                    |  10 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-rzv2m.c                | 530 ++++++++++++++++++
 4 files changed, 617 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
 create mode 100644 drivers/i2c/busses/i2c-rzv2m.c

-- 
2.34.1

[PATCH 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Phil Edworthy]

Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../bindings/i2c/renesas,rzv2m.yaml           | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml

diff --git a/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
new file mode 100644
index 000000000000..9049461ad2f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/renesas,rzv2m.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2M I2C Bus Interface
+
+maintainers:
+  - Phil Edworthy <phil.edworthy@renesas.com>
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,i2c-r9a09g011  # RZ/V2M
+      - const: renesas,rzv2m-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: Data transmission/reception interrupt
+      - description: Status interrupt
+
+  interrupt-names:
+    items:
+      - const: tia
+      - const: tis
+
+  clock-frequency:
+    description:
+      Desired I2C bus clock frequency in Hz. The absence of this property
+      indicates the default frequency 100 kHz.
+
+  clocks:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - power-domains
+  - resets
+  - '#address-cells'
+  - '#size-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r9a09g011-cpg.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    i2c0: i2c@a4030000 {
+            compatible = "renesas,i2c-r9a09g011", "renesas,rzv2m-i2c";
+            reg = <0xa4030000 0x80>;
+            interrupts = <GIC_SPI 232 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 236 IRQ_TYPE_EDGE_RISING>;
+            interrupt-names = "tia", "tis";
+            clocks = <&cpg CPG_MOD R9A09G011_IIC_PCLK0>;
+            resets = <&cpg R9A09G011_IIC_GPA_PRESETN>;
+            power-domains = <&cpg>;
+            clock-frequency = <100000>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+    };
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Krzysztof Kozlowski]

On 24/06/2022 12:17, Phil Edworthy wrote:
> Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../bindings/i2c/renesas,rzv2m.yaml           | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> new file mode 100644
> index 000000000000..9049461ad2f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/renesas,rzv2m.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/V2M I2C Bus Interface
> +
> +maintainers:
> +  - Phil Edworthy <phil.edworthy@renesas.com>
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - renesas,i2c-r9a09g011  # RZ/V2M
> +      - const: renesas,rzv2m-i2c
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: Data transmission/reception interrupt
> +      - description: Status interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: tia
> +      - const: tis
> +
> +  clock-frequency:
> +    description:
> +      Desired I2C bus clock frequency in Hz. The absence of this property
> +      indicates the default frequency 100 kHz.

Instead of last sentence, just add "default: 100000".

> +
> +  clocks:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - power-domains
> +  - resets

This was not mentioned in properties. Why?

> +  - '#address-cells'
> +  - '#size-cells'
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r9a09g011-cpg.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    i2c0: i2c@a4030000 {
> +            compatible = "renesas,i2c-r9a09g011", "renesas,rzv2m-i2c";
> +            reg = <0xa4030000 0x80>;
> +            interrupts = <GIC_SPI 232 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 236 IRQ_TYPE_EDGE_RISING>;
> +            interrupt-names = "tia", "tis";
> +            clocks = <&cpg CPG_MOD R9A09G011_IIC_PCLK0>;
> +            resets = <&cpg R9A09G011_IIC_GPA_PRESETN>;
> +            power-domains = <&cpg>;
> +            clock-frequency = <100000>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +    };


Best regards,
Krzysztof

RE: [PATCH 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Phil Edworthy]

Hi Krzysztof,

Thanks for you review.

On 25 June 2022 21:43 Krzysztof Kozlowski wrote:
> On 24/06/2022 12:17, Phil Edworthy wrote:
> > Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  .../bindings/i2c/renesas,rzv2m.yaml           | 76 +++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> > new file mode 100644
> > index 000000000000..9049461ad2f4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/i2c/renesas,rzv2m.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas RZ/V2M I2C Bus Interface
> > +
> > +maintainers:
> > +  - Phil Edworthy <phil.edworthy@renesas.com>
> > +
> > +allOf:
> > +  - $ref: /schemas/i2c/i2c-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - renesas,i2c-r9a09g011  # RZ/V2M
> > +      - const: renesas,rzv2m-i2c
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Data transmission/reception interrupt
> > +      - description: Status interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: tia
> > +      - const: tis
> > +
> > +  clock-frequency:
> > +    description:
> > +      Desired I2C bus clock frequency in Hz. The absence of this
> property
> > +      indicates the default frequency 100 kHz.
> 
> Instead of last sentence, just add "default: 100000".
Right, I'll also and an enum for this as the HW can only support 100
or 400kHz.


> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-names
> > +  - clocks
> > +  - power-domains
> > +  - resets
> 
> This was not mentioned in properties. Why?
Oops, I'll add it.


> > +  - '#address-cells'
> > +  - '#size-cells'
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/r9a09g011-cpg.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    i2c0: i2c@a4030000 {
> > +            compatible = "renesas,i2c-r9a09g011", "renesas,rzv2m-i2c";
> > +            reg = <0xa4030000 0x80>;
> > +            interrupts = <GIC_SPI 232 IRQ_TYPE_EDGE_RISING>,
> > +                         <GIC_SPI 236 IRQ_TYPE_EDGE_RISING>;
> > +            interrupt-names = "tia", "tis";
> > +            clocks = <&cpg CPG_MOD R9A09G011_IIC_PCLK0>;
> > +            resets = <&cpg R9A09G011_IIC_GPA_PRESETN>;
> > +            power-domains = <&cpg>;
> > +            clock-frequency = <100000>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +    };

Thanks
Phil

Re: [PATCH 1/2] dt-bindings: i2c: Document RZ/V2M I2C controller

[Krzysztof Kozlowski]

On 27/06/2022 09:17, Phil Edworthy wrote:
> Hi Krzysztof,
> 
> Thanks for you review.
> 
> On 25 June 2022 21:43 Krzysztof Kozlowski wrote:
>> On 24/06/2022 12:17, Phil Edworthy wrote:
>>> Document Renesas RZ/V2M (r9a09g011) I2C controller bindings.
>>>
>>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> ---
>>>  .../bindings/i2c/renesas,rzv2m.yaml           | 76 +++++++++++++++++++
>>>  1 file changed, 76 insertions(+)
>>>  create mode 100644
>> Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
>> b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
>>> new file mode 100644
>>> index 000000000000..9049461ad2f4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/renesas,rzv2m.yaml
>>> @@ -0,0 +1,76 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/i2c/renesas,rzv2m.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Renesas RZ/V2M I2C Bus Interface
>>> +
>>> +maintainers:
>>> +  - Phil Edworthy <phil.edworthy@renesas.com>
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - renesas,i2c-r9a09g011  # RZ/V2M
>>> +      - const: renesas,rzv2m-i2c
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    items:
>>> +      - description: Data transmission/reception interrupt
>>> +      - description: Status interrupt
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: tia
>>> +      - const: tis
>>> +
>>> +  clock-frequency:
>>> +    description:
>>> +      Desired I2C bus clock frequency in Hz. The absence of this
>> property
>>> +      indicates the default frequency 100 kHz.
>>
>> Instead of last sentence, just add "default: 100000".
> Right, I'll also and an enum for this as the HW can only support 100
> or 400kHz.

Sure, sounds good. Thank you.

Best regards,
Krzysztof

[PATCH 2/2] i2c: Add Renesas RZ/V2M controller

[Phil Edworthy]

Yet another i2c controller from Renesas that is found on the RZ/V2M
(r9a09g011) SoC. It can support only 100kHz and 400KHz operation.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/i2c/busses/Kconfig     |  10 +
 drivers/i2c/busses/Makefile    |   1 +
 drivers/i2c/busses/i2c-rzv2m.c | 530 +++++++++++++++++++++++++++++++++
 3 files changed, 541 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-rzv2m.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b1d7069dd377..9e3f9eb1ea3c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -984,6 +984,16 @@ config I2C_RK3X
 	  This driver can also be built as a module. If so, the module will
 	  be called i2c-rk3x.
 
+config I2C_RZV2M
+	tristate "Renesas RZ/V2M adapter"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  Renesas RZ/V2M  I2C interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-rzv2m.
+
 config I2C_S3C2410
 	tristate "S3C/Exynos I2C Driver"
 	depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || \
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index b0a10e5d9ee9..7792ffc591f0 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_I2C_QCOM_GENI)	+= i2c-qcom-geni.o
 obj-$(CONFIG_I2C_QUP)		+= i2c-qup.o
 obj-$(CONFIG_I2C_RIIC)		+= i2c-riic.o
 obj-$(CONFIG_I2C_RK3X)		+= i2c-rk3x.o
+obj-$(CONFIG_I2C_RZV2M)		+= i2c-rzv2m.o
 obj-$(CONFIG_I2C_S3C2410)	+= i2c-s3c2410.o
 obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
 obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
diff --git a/drivers/i2c/busses/i2c-rzv2m.c b/drivers/i2c/busses/i2c-rzv2m.c
new file mode 100644
index 000000000000..209171f4ff43
--- /dev/null
+++ b/drivers/i2c/busses/i2c-rzv2m.c
@@ -0,0 +1,530 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the Renesas RZ/V2M I2C unit
+ *
+ * Copyright (C) 2016-2022 Renesas Electronics Corporation
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/i2c.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+/* Register offsets */
+#define IICB0DAT	0x00		/* Data Register */
+#define IICB0CTL0	0x08		/* Control Register 0 */
+#define IICB0TRG	0x0C		/* Trigger Register */
+#define IICB0STR0	0x10		/* Status Register 0 */
+#define IICB0CTL1	0x20		/* Control Register 1 */
+#define IICB0WL		0x24		/* Low Level Width Setting Reg */
+#define IICB0WH		0x28		/* How Level Width Setting Reg */
+
+/* IICB0CTL0 */
+#define IICB0IICE	BIT(7)		/* I2C Enable */
+#define IICB0SLWT	BIT(1)		/* Interrupt Request Timing */
+#define IICB0SLAC	BIT(0)		/* Acknowledge */
+
+/* IICB0TRG */
+#define IICB0WRET	BIT(2)		/* Quit Wait Trigger */
+#define IICB0STT	BIT(1)		/* Create Start Condition Trigger */
+#define IICB0SPT	BIT(0)		/* Create Stop Condition Trigger */
+
+/* IICB0STR0 */
+#define IICB0SSAC	BIT(8)		/* Ack Flag */
+#define IICB0SSBS	BIT(6)		/* Bus Flag */
+#define IICB0SSSP	BIT(4)		/* Stop Condition Flag */
+
+/* IICB0CTL1 */
+#define IICB0MDSC	BIT(7)		/* Bus Mode */
+#define IICB0SLSE	BIT(1)		/* Start condition output */
+
+#define rzv2m_i2c_priv_to_dev(p)	((p)->adap.dev.parent)
+
+#define bit_setl(addr, val)		writel(readl(addr) | (val), (addr))
+#define bit_clrl(addr, val)		writel(readl(addr) & ~(val), (addr))
+
+struct rzv2m_i2c_priv {
+	void __iomem *base;
+	struct i2c_adapter adap;
+	struct clk *clk;
+	int bus_mode;
+	struct completion msg_tia_done;
+	u32 iicb0wl;
+	u32 iicb0wh;
+};
+
+enum bcr_index {
+	RZV2M_I2C_100K = 0,
+	RZV2M_I2C_400K,
+};
+
+struct bitrate_config {
+	unsigned int percent_low;
+	unsigned int min_hold_time_ns;
+};
+
+static const struct bitrate_config bitrate_configs[] = {
+	{47, 3450},
+	{52, 900},
+};
+
+static irqreturn_t rzv2m_i2c_tia_irq_handler(int this_irq, void *dev_id)
+{
+	struct rzv2m_i2c_priv *priv = dev_id;
+
+	complete(&priv->msg_tia_done);
+
+	return IRQ_HANDLED;
+}
+
+/* Calculate IICB0WL and IICB0WH */
+static int rzv2m_i2c_clock_calculate(struct device *dev,
+				     struct rzv2m_i2c_priv *priv)
+{
+	const struct bitrate_config *config;
+	unsigned int hold_time_ns;
+	unsigned int total_pclks;
+	unsigned int trf_pclks;
+	unsigned long pclk_hz;
+	struct i2c_timings t;
+	u32 trf_ns;
+
+	i2c_parse_fw_timings(dev, &t, true);
+
+	pclk_hz = clk_get_rate(priv->clk);
+	total_pclks = pclk_hz / t.bus_freq_hz;
+
+	trf_ns = t.scl_rise_ns + t.scl_fall_ns;
+	trf_pclks = (u64)pclk_hz * trf_ns / NSEC_PER_SEC;
+
+	/* Config setting */
+	switch (t.bus_freq_hz) {
+	case I2C_MAX_FAST_MODE_FREQ:
+		priv->bus_mode = RZV2M_I2C_400K;
+		break;
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		priv->bus_mode = RZV2M_I2C_100K;
+		break;
+	default:
+		dev_err(dev, "transfer speed is invalid\n");
+		return -EINVAL;
+	}
+	config = &bitrate_configs[priv->bus_mode];
+
+	/* IICB0WL = (percent_low / Transfer clock) x PCLK */
+	priv->iicb0wl = total_pclks * config->percent_low / 100;
+	if (priv->iicb0wl > 0x3ff)
+		return -EINVAL;
+
+	/* IICB0WH = ((percent_high / Transfer clock) x PCLK) - (tR + tF) */
+	priv->iicb0wh = total_pclks - priv->iicb0wl - trf_pclks;
+	if (priv->iicb0wh > 0x3ff)
+		return -EINVAL;
+
+	/*
+	 * Data hold time must be less than 0.9us in fast mode and
+	 * 3.45us in standard mode.
+	 * Data hold time = IICB0WL[9:2] / PCLK
+	 */
+	hold_time_ns = (u64)(priv->iicb0wl >> 2) * NSEC_PER_SEC / pclk_hz;
+	if (hold_time_ns > config->min_hold_time_ns) {
+		dev_err(dev, "data hold time %dns is over %dns\n",
+			hold_time_ns, config->min_hold_time_ns);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void rzv2m_i2c_init(struct rzv2m_i2c_priv *priv)
+{
+	u32 i2c_ctl0;
+	u32 i2c_ctl1;
+
+	/* i2c disable */
+	writel(0, priv->base + IICB0CTL0);
+
+	/* IICB0CTL1 setting */
+	i2c_ctl1 = IICB0SLSE;
+	if (priv->bus_mode == RZV2M_I2C_400K)
+		i2c_ctl1 |= IICB0MDSC;
+	writel(i2c_ctl1, priv->base + IICB0CTL1);
+
+	/* IICB0WL IICB0WH setting */
+	writel(priv->iicb0wl, priv->base + IICB0WL);
+	writel(priv->iicb0wh, priv->base + IICB0WH);
+
+	/* i2c enable after setting */
+	i2c_ctl0 = IICB0SLWT | IICB0SLAC | IICB0IICE;
+	writel(i2c_ctl0, priv->base + IICB0CTL0);
+}
+
+static int rzv2m_i2c_write_with_ACK(struct rzv2m_i2c_priv *priv, u32 data)
+{
+	unsigned long time_left;
+
+	reinit_completion(&priv->msg_tia_done);
+
+	writel(data, priv->base + IICB0DAT);
+
+	time_left = wait_for_completion_timeout(&priv->msg_tia_done,
+						priv->adap.timeout);
+	if (!time_left)
+		return -ETIMEDOUT;
+
+	/* Confirm ACK */
+	if ((readl(priv->base + IICB0STR0) & IICB0SSAC) != IICB0SSAC)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int rzv2m_i2c_read_with_ACK(struct rzv2m_i2c_priv *priv, u8 *data,
+				   bool last)
+{
+	unsigned long time_left;
+	u32 data_tmp;
+
+	reinit_completion(&priv->msg_tia_done);
+
+	/* Interrupt request timing : 8th clock */
+	bit_clrl(priv->base + IICB0CTL0, IICB0SLWT);
+
+	/* Exit the wait state */
+	writel(IICB0WRET, priv->base + IICB0TRG);
+
+	/* Wait for transaction */
+	time_left = wait_for_completion_timeout(&priv->msg_tia_done,
+						priv->adap.timeout);
+	if (!time_left)
+		return -ETIMEDOUT;
+
+	if (!last) {
+		/* Read data */
+		data_tmp = readl(priv->base + IICB0DAT);
+	} else {
+		/* Disable ACK */
+		bit_clrl(priv->base + IICB0CTL0, IICB0SLAC);
+
+		/* Read data*/
+		data_tmp = readl(priv->base + IICB0DAT);
+
+		/* Interrupt request timing : 9th clock */
+		bit_setl(priv->base + IICB0CTL0, IICB0SLWT);
+
+		/* Exit the wait state */
+		writel(IICB0WRET, priv->base + IICB0TRG);
+
+		/* Wait for transaction */
+		time_left = wait_for_completion_timeout(&priv->msg_tia_done,
+							priv->adap.timeout);
+		if (!time_left)
+			return -ETIMEDOUT;
+
+		/* Enable ACK */
+		bit_setl(priv->base + IICB0CTL0, IICB0SLAC);
+	}
+
+	*data = (u8)(data_tmp & 0xff);
+
+	return 0;
+}
+
+static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
+			  int *count)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < msg->len; i++) {
+		ret = rzv2m_i2c_write_with_ACK(priv, msg->buf[i]);
+		if (ret < 0)
+			break;
+	}
+	*count = i;
+
+	return ret;
+}
+
+static int rzv2m_i2c_receive(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
+			     int *count)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < msg->len; i++) {
+		ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
+					      ((msg->len - 1) == i));
+		if (ret < 0)
+			break;
+	}
+	*count = i;
+
+	return ret;
+}
+
+static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
+				  struct i2c_msg *msg, int read)
+{
+	u32 addr;
+	int ret;
+
+	if (msg->flags & I2C_M_TEN) {
+		/* 10-bit address
+		 *   addr_1: 5'b11110 | addr[9:8] | (R/nW)
+		 *   addr_2: addr[7:0]
+		 */
+		addr = 0xf0 | ((msg->addr >> 7) & 0x06);
+		addr |= read;
+		/* Send 1st address(extend code) */
+		ret = rzv2m_i2c_write_with_ACK(priv, addr);
+		if (ret == 0) {
+			/* Send 2nd address */
+			ret = rzv2m_i2c_write_with_ACK(priv, msg->addr & 0xff);
+		}
+	} else {
+		/* 7-bit address */
+		addr = i2c_8bit_addr_from_msg(msg);
+		ret = rzv2m_i2c_write_with_ACK(priv, addr);
+	}
+
+	return ret;
+}
+
+static int rzv2m_i2c_stop_condition(struct rzv2m_i2c_priv *priv)
+{
+	u32 value;
+
+	/* Send stop condition */
+	writel(IICB0SPT, priv->base + IICB0TRG);
+	return readl_poll_timeout(priv->base + IICB0STR0,
+				  value, value & IICB0SSSP,
+				  100, jiffies_to_usecs(priv->adap.timeout));
+}
+
+static int rzv2m_i2c_master_xfer1(struct rzv2m_i2c_priv *priv,
+				  struct i2c_msg *msg, int stop)
+{
+	int count = 0;
+	int ret, read = !!(msg->flags & I2C_M_RD);
+
+	/* Send start condition */
+	writel(IICB0STT, priv->base + IICB0TRG);
+
+	ret = rzv2m_i2c_send_address(priv, msg, read);
+
+	if (ret == 0) {
+		if (read)
+			ret = rzv2m_i2c_receive(priv, msg, &count);
+		else
+			ret = rzv2m_i2c_send(priv, msg, &count);
+
+		if ((ret == 0) && stop)
+			ret = rzv2m_i2c_stop_condition(priv);
+	}
+
+	if (ret == -ENXIO)
+		rzv2m_i2c_stop_condition(priv);
+	else if (ret < 0)
+		rzv2m_i2c_init(priv);
+	else
+		ret = count;
+
+	return ret;
+}
+
+static int rzv2m_i2c_master_xfer(struct i2c_adapter *adap,
+				 struct i2c_msg *msgs, int num)
+{
+	struct rzv2m_i2c_priv *priv = i2c_get_adapdata(adap);
+	struct device *dev = rzv2m_i2c_priv_to_dev(priv);
+	int ret, i;
+
+	pm_runtime_get_sync(dev);
+
+	if (readl(priv->base + IICB0STR0) & IICB0SSBS) {
+		ret = -EAGAIN;
+		goto out;
+	}
+
+	/* I2C main transfer */
+	for (i = 0; i < num; i++) {
+		ret = rzv2m_i2c_master_xfer1(priv, &msgs[i], (i == (num - 1)));
+		if (ret < 0)
+			goto out;
+	}
+	ret = num;
+
+out:
+	pm_runtime_put(dev);
+
+	return ret;
+}
+
+static u32 rzv2m_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
+	       I2C_FUNC_10BIT_ADDR;
+}
+
+static const struct i2c_adapter_quirks rzv2m_i2c_quirks = {
+	.flags = I2C_AQ_NO_ZERO_LEN,
+};
+
+static struct i2c_algorithm rzv2m_i2c_algo = {
+	.master_xfer = rzv2m_i2c_master_xfer,
+	.functionality = rzv2m_i2c_func,
+};
+
+static const struct of_device_id rzv2m_i2c_ids[] = {
+	{ .compatible = "renesas,rzv2m-i2c", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rzv2m_i2c_ids);
+
+static int rzv2m_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rzv2m_i2c_priv *priv;
+	struct reset_control *rstc;
+	struct i2c_adapter *adap;
+	struct resource *res;
+	int irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(struct rzv2m_i2c_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err_probe(dev, PTR_ERR(priv->clk), "Can't get clock\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	rstc = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(rstc)) {
+		dev_err_probe(dev, PTR_ERR(rstc), "Missing reset ctrl\n");
+		return PTR_ERR(rstc);
+	}
+	/*
+	 * The reset also affects other HW that is not under the control
+	 * of Linux. Therefore, all we can do is deassert the reset.
+	 */
+	reset_control_deassert(rstc);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, rzv2m_i2c_tia_irq_handler, 0,
+			       dev_name(dev), priv);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);
+		return ret;
+	}
+
+	adap = &priv->adap;
+	adap->nr = pdev->id;
+	adap->algo = &rzv2m_i2c_algo;
+	adap->quirks = &rzv2m_i2c_quirks;
+	adap->class = I2C_CLASS_DEPRECATED;
+	adap->dev.parent = dev;
+	adap->dev.of_node = dev->of_node;
+	adap->owner = THIS_MODULE;
+	i2c_set_adapdata(adap, priv);
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	init_completion(&priv->msg_tia_done);
+
+	ret = rzv2m_i2c_clock_calculate(dev, priv);
+	if (ret < 0)
+		return ret;
+
+	pm_runtime_enable(dev);
+
+	pm_runtime_get_sync(dev);
+	rzv2m_i2c_init(priv);
+	pm_runtime_put(dev);
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = i2c_add_numbered_adapter(adap);
+	if (ret < 0)
+		pm_runtime_disable(dev);
+
+	return ret;
+}
+
+static int rzv2m_i2c_remove(struct platform_device *pdev)
+{
+	struct rzv2m_i2c_priv *priv = platform_get_drvdata(pdev);
+	struct device *dev = rzv2m_i2c_priv_to_dev(priv);
+
+	i2c_del_adapter(&priv->adap);
+	bit_clrl(priv->base + IICB0CTL0, IICB0IICE);
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int rzv2m_i2c_suspend(struct device *dev)
+{
+	struct rzv2m_i2c_priv *priv = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(dev);
+	bit_clrl(priv->base + IICB0CTL0, IICB0IICE);
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
+static int rzv2m_i2c_resume(struct device *dev)
+{
+	struct rzv2m_i2c_priv *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = rzv2m_i2c_clock_calculate(dev, priv);
+	if (ret < 0)
+		return ret;
+
+	pm_runtime_get_sync(dev);
+	rzv2m_i2c_init(priv);
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops rzv2m_i2c_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rzv2m_i2c_suspend, rzv2m_i2c_resume)
+};
+
+#define DEV_PM_OPS (&rzv2m_i2c_pm_ops)
+#else
+#define DEV_PM_OPS NULL
+#endif /* CONFIG_PM_SLEEP */
+
+static struct platform_driver rzv2m_i2c_driver = {
+	.driver = {
+		.name = "rzv2m-i2c",
+		.pm = DEV_PM_OPS,
+		.of_match_table = rzv2m_i2c_ids,
+	},
+	.probe	= rzv2m_i2c_probe,
+	.remove	= rzv2m_i2c_remove,
+};
+module_platform_driver(rzv2m_i2c_driver);
+
+MODULE_DESCRIPTION("RZ/V2M I2C bus driver");
+MODULE_AUTHOR("Renesas Electronics Corporation");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 2/2] i2c: Add Renesas RZ/V2M controller

[Arnd Bergmann]

On Fri, Jun 24, 2022 at 12:17 PM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
>
> Yet another i2c controller from Renesas that is found on the RZ/V2M
> (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.

I see nothing wrong with this, just one suggestion for a cleanup:

> +#ifdef CONFIG_PM_SLEEP
> +static int rzv2m_i2c_suspend(struct device *dev)
...> +static const struct dev_pm_ops rzv2m_i2c_pm_ops = {
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rzv2m_i2c_suspend, rzv2m_i2c_resume)
> +};
> +
> +#define DEV_PM_OPS (&rzv2m_i2c_pm_ops)
> +#else
> +#define DEV_PM_OPS NULL
> +#endif /* CONFIG_PM_SLEEP */

Remove the #ifdef here, and use the new NOIRQ_SYSTEM_SLEEP_PM_OPS()
in place of SET_NOIRQ_SYSTEM_SLEEP_PM_OPS().

         Arnd

Re: [PATCH 2/2] i2c: Add Renesas RZ/V2M controller

[Geert Uytterhoeven]

Hi Arnd,

On Fri, Jun 24, 2022 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 24, 2022 at 12:17 PM Phil Edworthy
> <phil.edworthy@renesas.com> wrote:
> >
> > Yet another i2c controller from Renesas that is found on the RZ/V2M
> > (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
>
> I see nothing wrong with this, just one suggestion for a cleanup:
>
> > +#ifdef CONFIG_PM_SLEEP
> > +static int rzv2m_i2c_suspend(struct device *dev)
> ...> +static const struct dev_pm_ops rzv2m_i2c_pm_ops = {
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rzv2m_i2c_suspend, rzv2m_i2c_resume)
> > +};
> > +
> > +#define DEV_PM_OPS (&rzv2m_i2c_pm_ops)
> > +#else
> > +#define DEV_PM_OPS NULL
> > +#endif /* CONFIG_PM_SLEEP */
>
> Remove the #ifdef here, and use the new NOIRQ_SYSTEM_SLEEP_PM_OPS()
> in place of SET_NOIRQ_SYSTEM_SLEEP_PM_OPS().

Cool, TIL!

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 2/2] i2c: Add Renesas RZ/V2M controller

[Andy Shevchenko]

On Fri, Jun 24, 2022 at 01:48:58PM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 24, 2022 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Fri, Jun 24, 2022 at 12:17 PM Phil Edworthy
> > <phil.edworthy@renesas.com> wrote:

...

> > Remove the #ifdef here, and use the new NOIRQ_SYSTEM_SLEEP_PM_OPS()
> > in place of SET_NOIRQ_SYSTEM_SLEEP_PM_OPS().
> 
> Cool, TIL!

There are also pm_ptr() and pm_sleep_ptr() macros (need to be used when assign
dev_pm_ops).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] i2c: Add Renesas RZ/V2M controller

[Geert Uytterhoeven]

Hi Andy,

On Tue, Jun 28, 2022 at 12:17 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Jun 24, 2022 at 01:48:58PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Jun 24, 2022 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Fri, Jun 24, 2022 at 12:17 PM Phil Edworthy
> > > <phil.edworthy@renesas.com> wrote:
>
> ...
>
> > > Remove the #ifdef here, and use the new NOIRQ_SYSTEM_SLEEP_PM_OPS()
> > > in place of SET_NOIRQ_SYSTEM_SLEEP_PM_OPS().
> >
> > Cool, TIL!
>
> There are also pm_ptr() and pm_sleep_ptr() macros (need to be used when assign
> dev_pm_ops).

Indeed, I have used the latter in a patch I posted yesterday ;-)

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 2/2] i2c: Add Renesas RZ/V2M controller

[Phil Edworthy]

Hi Andy,

On 28 June 2022 11:17 Andy Shevchenko wrote:
> On Fri, Jun 24, 2022 at 01:48:58PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Jun 24, 2022 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Fri, Jun 24, 2022 at 12:17 PM Phil Edworthy
> > > <phil.edworthy@renesas.com> wrote:
> 
> ...
> 
> > > Remove the #ifdef here, and use the new NOIRQ_SYSTEM_SLEEP_PM_OPS()
> > > in place of SET_NOIRQ_SYSTEM_SLEEP_PM_OPS().
> >
> > Cool, TIL!
> 
> There are also pm_ptr() and pm_sleep_ptr() macros (need to be used when
> assign
> dev_pm_ops).
I noticed these when looking at NOIRQ_SYSTEM_SLEEP_PM_OPS,
but thanks for pointing them out anyway.

Phil

RE: [PATCH 2/2] i2c: Add Renesas RZ/V2M controller

[Phil Edworthy]

Hi Arnd,

On 24 June 2022 12:27 Arnd Bergmann wrote:
> On Fri, Jun 24, 2022 at 12:17 PM Phil Edworthy wrote:
> >
> > Yet another i2c controller from Renesas that is found on the RZ/V2M
> > (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
> 
> I see nothing wrong with this, just one suggestion for a cleanup:
> 
> > +#ifdef CONFIG_PM_SLEEP
> > +static int rzv2m_i2c_suspend(struct device *dev)
> ...> +static const struct dev_pm_ops rzv2m_i2c_pm_ops = {
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rzv2m_i2c_suspend,
> rzv2m_i2c_resume)
> > +};
> > +
> > +#define DEV_PM_OPS (&rzv2m_i2c_pm_ops)
> > +#else
> > +#define DEV_PM_OPS NULL
> > +#endif /* CONFIG_PM_SLEEP */
> 
> Remove the #ifdef here, and use the new NOIRQ_SYSTEM_SLEEP_PM_OPS()
> in place of SET_NOIRQ_SYSTEM_SLEEP_PM_OPS().
Will do!

Thanks
Phil

Re: [PATCH 2/2] i2c: Add Renesas RZ/V2M controller

[Geert Uytterhoeven]

Hi Phil,

On Fri, Jun 24, 2022 at 12:18 PM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> Yet another i2c controller from Renesas that is found on the RZ/V2M
> (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-rzv2m.c

> +enum bcr_index {
> +       RZV2M_I2C_100K = 0,
> +       RZV2M_I2C_400K,
> +};
> +
> +struct bitrate_config {
> +       unsigned int percent_low;
> +       unsigned int min_hold_time_ns;
> +};
> +
> +static const struct bitrate_config bitrate_configs[] = {
> +       {47, 3450},
> +       {52, 900},

These are indexed by bcr_index, so perhaps

    .[RZV2M_I2C_100K] = { 47, 3450 },
    ...

?

> +};

> +/* Calculate IICB0WL and IICB0WH */
> +static int rzv2m_i2c_clock_calculate(struct device *dev,
> +                                    struct rzv2m_i2c_priv *priv)
> +{
> +       const struct bitrate_config *config;
> +       unsigned int hold_time_ns;
> +       unsigned int total_pclks;
> +       unsigned int trf_pclks;
> +       unsigned long pclk_hz;
> +       struct i2c_timings t;
> +       u32 trf_ns;
> +
> +       i2c_parse_fw_timings(dev, &t, true);
> +
> +       pclk_hz = clk_get_rate(priv->clk);
> +       total_pclks = pclk_hz / t.bus_freq_hz;
> +
> +       trf_ns = t.scl_rise_ns + t.scl_fall_ns;
> +       trf_pclks = (u64)pclk_hz * trf_ns / NSEC_PER_SEC;

This is an open-coded 64-by-ul division, which may cause link failures
when compile-tested on 32-bit. Please use mul_u64_u32_div() instead.

> +
> +       /* Config setting */
> +       switch (t.bus_freq_hz) {
> +       case I2C_MAX_FAST_MODE_FREQ:
> +               priv->bus_mode = RZV2M_I2C_400K;
> +               break;
> +       case I2C_MAX_STANDARD_MODE_FREQ:
> +               priv->bus_mode = RZV2M_I2C_100K;
> +               break;
> +       default:
> +               dev_err(dev, "transfer speed is invalid\n");
> +               return -EINVAL;
> +       }
> +       config = &bitrate_configs[priv->bus_mode];
> +
> +       /* IICB0WL = (percent_low / Transfer clock) x PCLK */
> +       priv->iicb0wl = total_pclks * config->percent_low / 100;
> +       if (priv->iicb0wl > 0x3ff)
> +               return -EINVAL;
> +
> +       /* IICB0WH = ((percent_high / Transfer clock) x PCLK) - (tR + tF) */
> +       priv->iicb0wh = total_pclks - priv->iicb0wl - trf_pclks;
> +       if (priv->iicb0wh > 0x3ff)
> +               return -EINVAL;
> +
> +       /*
> +        * Data hold time must be less than 0.9us in fast mode and
> +        * 3.45us in standard mode.
> +        * Data hold time = IICB0WL[9:2] / PCLK
> +        */
> +       hold_time_ns = (u64)(priv->iicb0wl >> 2) * NSEC_PER_SEC / pclk_hz;

div64_ul()

> +       if (hold_time_ns > config->min_hold_time_ns) {
> +               dev_err(dev, "data hold time %dns is over %dns\n",
> +                       hold_time_ns, config->min_hold_time_ns);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}

> +static int rzv2m_i2c_write_with_ACK(struct rzv2m_i2c_priv *priv, u32 data)

rzv2m_i2c_write_with_ack

> +{
> +       unsigned long time_left;
> +
> +       reinit_completion(&priv->msg_tia_done);
> +
> +       writel(data, priv->base + IICB0DAT);
> +
> +       time_left = wait_for_completion_timeout(&priv->msg_tia_done,
> +                                               priv->adap.timeout);
> +       if (!time_left)
> +               return -ETIMEDOUT;
> +
> +       /* Confirm ACK */
> +       if ((readl(priv->base + IICB0STR0) & IICB0SSAC) != IICB0SSAC)
> +               return -ENXIO;
> +
> +       return 0;
> +}
> +
> +static int rzv2m_i2c_read_with_ACK(struct rzv2m_i2c_priv *priv, u8 *data,

rzv2m_i2c_read_with_ack

> +                                  bool last)
> +{
> +       unsigned long time_left;
> +       u32 data_tmp;
> +
> +       reinit_completion(&priv->msg_tia_done);
> +
> +       /* Interrupt request timing : 8th clock */
> +       bit_clrl(priv->base + IICB0CTL0, IICB0SLWT);
> +
> +       /* Exit the wait state */
> +       writel(IICB0WRET, priv->base + IICB0TRG);
> +
> +       /* Wait for transaction */
> +       time_left = wait_for_completion_timeout(&priv->msg_tia_done,
> +                                               priv->adap.timeout);
> +       if (!time_left)
> +               return -ETIMEDOUT;
> +
> +       if (!last) {
> +               /* Read data */
> +               data_tmp = readl(priv->base + IICB0DAT);
> +       } else {
> +               /* Disable ACK */
> +               bit_clrl(priv->base + IICB0CTL0, IICB0SLAC);
> +
> +               /* Read data*/
> +               data_tmp = readl(priv->base + IICB0DAT);
> +
> +               /* Interrupt request timing : 9th clock */
> +               bit_setl(priv->base + IICB0CTL0, IICB0SLWT);
> +
> +               /* Exit the wait state */
> +               writel(IICB0WRET, priv->base + IICB0TRG);
> +
> +               /* Wait for transaction */
> +               time_left = wait_for_completion_timeout(&priv->msg_tia_done,
> +                                                       priv->adap.timeout);
> +               if (!time_left)
> +                       return -ETIMEDOUT;
> +
> +               /* Enable ACK */
> +               bit_setl(priv->base + IICB0CTL0, IICB0SLAC);
> +       }
> +
> +       *data = (u8)(data_tmp & 0xff);

No need to cast or mask.

> +
> +       return 0;
> +}
> +
> +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
> +                         int *count)

unsigned int *count

> +{
> +       int i, ret = 0;

unsigned int i

> +
> +       for (i = 0; i < msg->len; i++) {
> +               ret = rzv2m_i2c_write_with_ACK(priv, msg->buf[i]);
> +               if (ret < 0)
> +                       break;

return ret?

> +       }
> +       *count = i;
> +
> +       return ret;
> +}
> +
> +static int rzv2m_i2c_receive(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
> +                            int *count)

unsigned int *count

> +{
> +       int i, ret = 0;

unsigned int i

> +
> +       for (i = 0; i < msg->len; i++) {
> +               ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
> +                                             ((msg->len - 1) == i));
> +               if (ret < 0)
> +                       break;

return ret?

> +       }
> +       *count = i;
> +
> +       return ret;
> +}
> +
> +static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
> +                                 struct i2c_msg *msg, int read)

No need to pass read, as you have access to the full msg, and 10-bit
addressing is rare?

> +{
> +       u32 addr;
> +       int ret;
> +
> +       if (msg->flags & I2C_M_TEN) {
> +               /* 10-bit address
> +                *   addr_1: 5'b11110 | addr[9:8] | (R/nW)
> +                *   addr_2: addr[7:0]
> +                */
> +               addr = 0xf0 | ((msg->addr >> 7) & 0x06);
> +               addr |= read;
> +               /* Send 1st address(extend code) */
> +               ret = rzv2m_i2c_write_with_ACK(priv, addr);
> +               if (ret == 0) {
> +                       /* Send 2nd address */
> +                       ret = rzv2m_i2c_write_with_ACK(priv, msg->addr & 0xff);
> +               }
> +       } else {
> +               /* 7-bit address */
> +               addr = i2c_8bit_addr_from_msg(msg);
> +               ret = rzv2m_i2c_write_with_ACK(priv, addr);
> +       }
> +
> +       return ret;
> +}

> +static int rzv2m_i2c_master_xfer1(struct rzv2m_i2c_priv *priv,
> +                                 struct i2c_msg *msg, int stop)
> +{
> +       int count = 0;

unsigned int

> +       int ret, read = !!(msg->flags & I2C_M_RD);
> +
> +       /* Send start condition */
> +       writel(IICB0STT, priv->base + IICB0TRG);
> +
> +       ret = rzv2m_i2c_send_address(priv, msg, read);
> +

Please drop this blank line.

> +       if (ret == 0) {
> +               if (read)
> +                       ret = rzv2m_i2c_receive(priv, msg, &count);
> +               else
> +                       ret = rzv2m_i2c_send(priv, msg, &count);
> +
> +               if ((ret == 0) && stop)
> +                       ret = rzv2m_i2c_stop_condition(priv);
> +       }
> +
> +       if (ret == -ENXIO)
> +               rzv2m_i2c_stop_condition(priv);
> +       else if (ret < 0)
> +               rzv2m_i2c_init(priv);
> +       else
> +               ret = count;
> +
> +       return ret;
> +}
> +
> +static int rzv2m_i2c_master_xfer(struct i2c_adapter *adap,
> +                                struct i2c_msg *msgs, int num)
> +{
> +       struct rzv2m_i2c_priv *priv = i2c_get_adapdata(adap);
> +       struct device *dev = rzv2m_i2c_priv_to_dev(priv);
> +       int ret, i;

unsigned int i

> +
> +       pm_runtime_get_sync(dev);

Please use pm_runtime_resume_and_get() in new code
(and check its return code?).

> +
> +       if (readl(priv->base + IICB0STR0) & IICB0SSBS) {
> +               ret = -EAGAIN;
> +               goto out;
> +       }
> +
> +       /* I2C main transfer */
> +       for (i = 0; i < num; i++) {
> +               ret = rzv2m_i2c_master_xfer1(priv, &msgs[i], (i == (num - 1)));
> +               if (ret < 0)
> +                       goto out;
> +       }
> +       ret = num;
> +
> +out:
> +       pm_runtime_put(dev);
> +
> +       return ret;
> +}

> +static struct i2c_algorithm rzv2m_i2c_algo = {
> +       .master_xfer = rzv2m_i2c_master_xfer,
> +       .functionality = rzv2m_i2c_func,

No .(un)reg_slave()? ;-)
The hardware seems to support it.

> +};


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 2/2] i2c: Add Renesas RZ/V2M controller

[Phil Edworthy]

Hi Geert,

On 28 June 2022 14:42 Geert Uytterhoeven wrote:
> On Fri, Jun 24, 2022 at 12:18 PM Phil Edworthy wrote:
> > Yet another i2c controller from Renesas that is found on the RZ/V2M
> > (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
Thanks for your review!

> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-rzv2m.c
> 
> > +enum bcr_index {
> > +       RZV2M_I2C_100K = 0,
> > +       RZV2M_I2C_400K,
> > +};
> > +
> > +struct bitrate_config {
> > +       unsigned int percent_low;
> > +       unsigned int min_hold_time_ns;
> > +};
> > +
> > +static const struct bitrate_config bitrate_configs[] = {
> > +       {47, 3450},
> > +       {52, 900},
> 
> These are indexed by bcr_index, so perhaps
> 
>     .[RZV2M_I2C_100K] = { 47, 3450 },
>     ...
> 
> ?
Ok


> > +};
> 
> > +/* Calculate IICB0WL and IICB0WH */
> > +static int rzv2m_i2c_clock_calculate(struct device *dev,
> > +                                    struct rzv2m_i2c_priv *priv)
> > +{
> > +       const struct bitrate_config *config;
> > +       unsigned int hold_time_ns;
> > +       unsigned int total_pclks;
> > +       unsigned int trf_pclks;
> > +       unsigned long pclk_hz;
> > +       struct i2c_timings t;
> > +       u32 trf_ns;
> > +
> > +       i2c_parse_fw_timings(dev, &t, true);
> > +
> > +       pclk_hz = clk_get_rate(priv->clk);
> > +       total_pclks = pclk_hz / t.bus_freq_hz;
> > +
> > +       trf_ns = t.scl_rise_ns + t.scl_fall_ns;
> > +       trf_pclks = (u64)pclk_hz * trf_ns / NSEC_PER_SEC;
> 
> This is an open-coded 64-by-ul division, which may cause link failures
> when compile-tested on 32-bit. Please use mul_u64_u32_div() instead.
Sure


> > +
> > +       /* Config setting */
> > +       switch (t.bus_freq_hz) {
> > +       case I2C_MAX_FAST_MODE_FREQ:
> > +               priv->bus_mode = RZV2M_I2C_400K;
> > +               break;
> > +       case I2C_MAX_STANDARD_MODE_FREQ:
> > +               priv->bus_mode = RZV2M_I2C_100K;
> > +               break;
> > +       default:
> > +               dev_err(dev, "transfer speed is invalid\n");
> > +               return -EINVAL;
> > +       }
> > +       config = &bitrate_configs[priv->bus_mode];
> > +
> > +       /* IICB0WL = (percent_low / Transfer clock) x PCLK */
> > +       priv->iicb0wl = total_pclks * config->percent_low / 100;
> > +       if (priv->iicb0wl > 0x3ff)
> > +               return -EINVAL;
> > +
> > +       /* IICB0WH = ((percent_high / Transfer clock) x PCLK) - (tR +
> tF) */
> > +       priv->iicb0wh = total_pclks - priv->iicb0wl - trf_pclks;
> > +       if (priv->iicb0wh > 0x3ff)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * Data hold time must be less than 0.9us in fast mode and
> > +        * 3.45us in standard mode.
> > +        * Data hold time = IICB0WL[9:2] / PCLK
> > +        */
> > +       hold_time_ns = (u64)(priv->iicb0wl >> 2) * NSEC_PER_SEC /
> pclk_hz;
> 
> div64_ul()
Ok


> > +       if (hold_time_ns > config->min_hold_time_ns) {
> > +               dev_err(dev, "data hold time %dns is over %dns\n",
> > +                       hold_time_ns, config->min_hold_time_ns);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> > +static int rzv2m_i2c_write_with_ACK(struct rzv2m_i2c_priv *priv, u32
> data)
> 
> rzv2m_i2c_write_with_ack
Ok

<snip>
> > +       *data = (u8)(data_tmp & 0xff);
> 
> No need to cast or mask.
Ok


> > +
> > +       return 0;
> > +}
> > +
> > +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg
> *msg,
> > +                         int *count)
> 
> unsigned int *count
Ok


> > +{
> > +       int i, ret = 0;
> 
> unsigned int i
Ok


> > +
> > +       for (i = 0; i < msg->len; i++) {
> > +               ret = rzv2m_i2c_write_with_ACK(priv, msg->buf[i]);
> > +               if (ret < 0)
> > +                       break;
> 
> return ret?
Yes, makes sense as *count is only used if ret is >= 0


> > +       }
> > +       *count = i;
> > +
> > +       return ret;
> > +}
> > +
> > +static int rzv2m_i2c_receive(struct rzv2m_i2c_priv *priv, struct
> i2c_msg *msg,
> > +                            int *count)
> 
> unsigned int *count
Ok


> > +{
> > +       int i, ret = 0;
> 
> unsigned int i
Ok


> > +
> > +       for (i = 0; i < msg->len; i++) {
> > +               ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
> > +                                             ((msg->len - 1) == i));
> > +               if (ret < 0)
> > +                       break;
> 
> return ret?
Ok


> > +       }
> > +       *count = i;
> > +
> > +       return ret;
> > +}
> > +
> > +static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
> > +                                 struct i2c_msg *msg, int read)
> 
> No need to pass read, as you have access to the full msg, and 10-bit
> addressing is rare?
Ok, makes sense


> > +{
> > +       u32 addr;
> > +       int ret;
> > +
> > +       if (msg->flags & I2C_M_TEN) {
> > +               /* 10-bit address
> > +                *   addr_1: 5'b11110 | addr[9:8] | (R/nW)
> > +                *   addr_2: addr[7:0]
> > +                */
> > +               addr = 0xf0 | ((msg->addr >> 7) & 0x06);
> > +               addr |= read;
> > +               /* Send 1st address(extend code) */
> > +               ret = rzv2m_i2c_write_with_ACK(priv, addr);
> > +               if (ret == 0) {
> > +                       /* Send 2nd address */
> > +                       ret = rzv2m_i2c_write_with_ACK(priv, msg->addr &
> 0xff);
> > +               }
> > +       } else {
> > +               /* 7-bit address */
> > +               addr = i2c_8bit_addr_from_msg(msg);
> > +               ret = rzv2m_i2c_write_with_ACK(priv, addr);
> > +       }
> > +
> > +       return ret;
> > +}
> 
> > +static int rzv2m_i2c_master_xfer1(struct rzv2m_i2c_priv *priv,
> > +                                 struct i2c_msg *msg, int stop)
> > +{
> > +       int count = 0;
> 
> unsigned int
Ok


> > +       int ret, read = !!(msg->flags & I2C_M_RD);
> > +
> > +       /* Send start condition */
> > +       writel(IICB0STT, priv->base + IICB0TRG);
> > +
> > +       ret = rzv2m_i2c_send_address(priv, msg, read);
> > +
> 
> Please drop this blank line.
Ok


> > +       if (ret == 0) {
> > +               if (read)
> > +                       ret = rzv2m_i2c_receive(priv, msg, &count);
> > +               else
> > +                       ret = rzv2m_i2c_send(priv, msg, &count);
> > +
> > +               if ((ret == 0) && stop)
> > +                       ret = rzv2m_i2c_stop_condition(priv);
> > +       }
> > +
> > +       if (ret == -ENXIO)
> > +               rzv2m_i2c_stop_condition(priv);
> > +       else if (ret < 0)
> > +               rzv2m_i2c_init(priv);
> > +       else
> > +               ret = count;
> > +
> > +       return ret;
> > +}
> > +
> > +static int rzv2m_i2c_master_xfer(struct i2c_adapter *adap,
> > +                                struct i2c_msg *msgs, int num)
> > +{
> > +       struct rzv2m_i2c_priv *priv = i2c_get_adapdata(adap);
> > +       struct device *dev = rzv2m_i2c_priv_to_dev(priv);
> > +       int ret, i;
> 
> unsigned int i
Ok


> > +
> > +       pm_runtime_get_sync(dev);
> 
> Please use pm_runtime_resume_and_get() in new code
> (and check its return code?).
Ok


> > +
> > +       if (readl(priv->base + IICB0STR0) & IICB0SSBS) {
> > +               ret = -EAGAIN;
> > +               goto out;
> > +       }
> > +
> > +       /* I2C main transfer */
> > +       for (i = 0; i < num; i++) {
> > +               ret = rzv2m_i2c_master_xfer1(priv, &msgs[i], (i == (num
> - 1)));
> > +               if (ret < 0)
> > +                       goto out;
> > +       }
> > +       ret = num;
> > +
> > +out:
> > +       pm_runtime_put(dev);
I notice that a lot of i2c drivers use pm_runtime_mark_last_busy() and 
pm_runtime_put_autosuspend() here. I think they make sense here as well.


> > +
> > +       return ret;
> > +}
> 
> > +static struct i2c_algorithm rzv2m_i2c_algo = {
> > +       .master_xfer = rzv2m_i2c_master_xfer,
> > +       .functionality = rzv2m_i2c_func,
> 
> No .(un)reg_slave()? ;-)
> The hardware seems to support it.
You are right that the HW supports it, though I haven’t tested it!
I currently don't have something I can test it with either.

Thanks!
Phil

Re: [PATCH 0/2] i2c: Add new driver for Renesas RZ/V2M controller

[Geert Uytterhoeven]

Hi Phil,

Thanks for your series!

On Fri, Jun 24, 2022 at 12:17 PM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series
> add the driver. One annoying problem is that the SoC uses a single reset
> line for two i2c controllers, and unfortunately one of the controllers
> is managed by some firmware, not by Linux. Therefore, the driver just
> deasserts the reset.

That is actually an integration issue, not an i2c controller issue.

Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL,
to be set by the reset provider?

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