[PATCH v2 00/11] Add RTC support for the Renesas RZ/G3S SoC

[PATCH v2 00/11] Add RTC support for the Renesas RZ/G3S SoC

[Claudiu]

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

Hi,

On the Renesas RZ/G3S SoC the RTC clock is provided by the VBATTB
IP. A 32 KHz crystall oscillator could be connected to the VBATTB
input pins. The logic to control this clock (and pass it to RTC)
is inside the VBATTB IP. For this, the clk-vbattb driver was added
(patches 01-03/11).

Patches 04-05/11 add the RTC driver.

Patches 06-09/11 update the device trees with proper nodes to enable RTC.

Patches 10-11/11 enable proper config flags for RTC to work on RZ/G3S SoC.

Thank you,
Claudiu Beznea

Changes in v2:
- dropped patch "clk: renesas: r9a08g045: Add clock, reset and power domain
  support for the VBATTB IP" as it was already integrated
- kept only a documentation file for both VBATT MFD and clock drivers as
  suggested
- addressed review comments
- used cleanup.h lock helpers
- update startup sequence for the RTC driver
- switch to 24 hours mode on the RTC driver
- fixed range for the RTC driver
- added a generic compatible for the RTC driver as this will also be
  used by RZ/V2H
- used clkin/xin clock names for the VBATTB clock driver to determine
  if bypass should be configured on registers instead of having
  dedicated DT property
- added mfd driver for VBATTB
- updated Kconfig flag names to include vendor name
- removed DT node labels from Documentation files
- used items to describe the interrupts and clocks

Claudiu Beznea (11):
  dt-bindings: mfd: renesas,r9a08g045-vbattb: Document VBATTB
  mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
  clk: renesas: clk-vbattb: Add VBATTB clock driver
  dt-bindings: rtc: renesas,rzg3s-rtc: Document the Renesas RTCA-3 IP
  rtc: renesas-rtca3: Add driver for RTCA-3 available on Renesas RZ/G3S
    SoC
  arm64: dts: renesas: r9a08g045: Add VBATTB node
  arm64: dts: renesas: r9a08g045: Add RTC node
  arm64: dts: renesas: rzg3s-smarc-som: Enable VBATTB clock
  arm64: dts: renesas: rzg3s-smarc-som: Enable RTC
  arm64: defconfig: Enable VBATTB
  arm64: defconfig: Enable Renesas RTCA-3 flag

 .../mfd/renesas,r9a08g045-vbattb.yaml         | 136 +++
 .../bindings/rtc/renesas,rz-rtca3.yaml        |  69 ++
 MAINTAINERS                                   |   8 +
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi    |  43 +
 .../boot/dts/renesas/rzg3s-smarc-som.dtsi     |  17 +
 arch/arm64/configs/defconfig                  |   3 +
 drivers/clk/renesas/Kconfig                   |   5 +
 drivers/clk/renesas/Makefile                  |   1 +
 drivers/clk/renesas/clk-vbattb.c              | 212 +++++
 drivers/mfd/Kconfig                           |   8 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/renesas-vbattb.c                  |  78 ++
 drivers/rtc/Kconfig                           |  10 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-renesas-rtca3.c               | 853 ++++++++++++++++++
 15 files changed, 1445 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
 create mode 100644 Documentation/devicetree/bindings/rtc/renesas,rz-rtca3.yaml
 create mode 100644 drivers/clk/renesas/clk-vbattb.c
 create mode 100644 drivers/mfd/renesas-vbattb.c
 create mode 100644 drivers/rtc/rtc-renesas-rtca3.c

-- 
2.39.2

[PATCH v2 01/11] dt-bindings: mfd: renesas,r9a08g045-vbattb: Document VBATTB

[Claudiu]

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

The VBATTB IP of the Renesas RZ/G3S SoC controls the clock for RTC,
the tamper detector and a small general usage memory of 128B. Add
documentation for it.

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

Changes in v2:
- changed file name and compatible
- updated title, description sections
- added clock controller part documentation and drop dedicated file
  for it included in v1
- used items to describe interrupts, interrupt-names, clocks, clock-names,
  resets
- dropped node labels and status
- updated clock-names for clock controller to cope with the new
  logic on detecting the necessity to setup bypass

 .../mfd/renesas,r9a08g045-vbattb.yaml         | 136 ++++++++++++++++++
 1 file changed, 136 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml

diff --git a/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml b/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
new file mode 100644
index 000000000000..30e4da65e2f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
@@ -0,0 +1,136 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/renesas,r9a08g045-vbattb.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas Battery Backup Function (VBATTB)
+
+description:
+  Renesas VBATTB is an always on powered module (backed by battery) which
+  controls the RTC clock (VBATTCLK), tamper detection logic and a small
+  general usage memory (128B).
+
+maintainers:
+  - Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
+
+properties:
+  compatible:
+    const: renesas,r9a08g045-vbattb
+
+  reg:
+    maxItems: 1
+
+  ranges: true
+
+  interrupts:
+    items:
+      - description: tamper detector interrupt
+
+  interrupt-names:
+    items:
+      - const: tampdi
+
+  clocks:
+    items:
+      - description: VBATTB module clock
+
+  clock-names:
+    items:
+      - const: bclk
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    items:
+      - description: VBATTB module reset
+
+  '#address-cells':
+    const: 2
+
+  '#size-cells':
+    const: 2
+
+patternProperties:
+  "^clock-controller@1c+$":
+    type: object
+    description: VBATTCLK clock
+
+    properties:
+      compatible:
+        const: renesas,r9a08g045-vbattb-clk
+
+      reg:
+        maxItems: 1
+
+      clocks:
+        items:
+          - description: input clock for VBATTCLK
+
+      clock-names:
+        description: |
+          Use xin if connected to an external crystal oscillator.
+          Use clkin if connected to an external hardware device generating the
+          clock.
+        enum:
+          - xin
+          - clkin
+
+      '#clock-cells':
+        const: 0
+
+      renesas,vbattb-load-nanofarads:
+        description: load capacitance of the on board xtal
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [ 4000, 7000, 9000, 12500 ]
+
+    required:
+      - compatible
+      - reg
+      - clocks
+      - clock-names
+      - '#clock-cells'
+      - renesas,vbattb-load-nanofarads
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+  - power-domains
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r9a08g045-cpg.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    vbattb@1005c000 {
+        compatible = "renesas,r9a08g045-vbattb";
+        reg = <0x1005c000 0x1000>;
+        ranges = <0 0 0x1005c000 0 0x1000>;
+        interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "tampdi";
+        clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>;
+        clock-names = "bclk";
+        power-domains = <&cpg>;
+        resets = <&cpg R9A08G045_VBAT_BRESETN>;
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@1c {
+            compatible = "renesas,r9a08g045-vbattb-clk";
+            reg = <0 0x1c 0 0x10>;
+            clocks = <&vbattb_xtal>;
+            clock-names = "xin";
+            #clock-cells = <0>;
+            renesas,vbattb-load-nanofarads = <12500>;
+        };
+    };
-- 
2.39.2

Re: [PATCH v2 01/11] dt-bindings: mfd: renesas,r9a08g045-vbattb: Document VBATTB

[Rob Herring]

On Tue, Jul 16, 2024 at 01:30:15PM +0300, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The VBATTB IP of the Renesas RZ/G3S SoC controls the clock for RTC,
> the tamper detector and a small general usage memory of 128B. Add
> documentation for it.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - changed file name and compatible
> - updated title, description sections
> - added clock controller part documentation and drop dedicated file
>   for it included in v1
> - used items to describe interrupts, interrupt-names, clocks, clock-names,
>   resets
> - dropped node labels and status
> - updated clock-names for clock controller to cope with the new
>   logic on detecting the necessity to setup bypass
> 
>  .../mfd/renesas,r9a08g045-vbattb.yaml         | 136 ++++++++++++++++++
>  1 file changed, 136 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml b/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
> new file mode 100644
> index 000000000000..30e4da65e2f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
> @@ -0,0 +1,136 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/renesas,r9a08g045-vbattb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas Battery Backup Function (VBATTB)
> +
> +description:
> +  Renesas VBATTB is an always on powered module (backed by battery) which
> +  controls the RTC clock (VBATTCLK), tamper detection logic and a small
> +  general usage memory (128B).
> +
> +maintainers:
> +  - Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> +
> +properties:
> +  compatible:
> +    const: renesas,r9a08g045-vbattb
> +
> +  reg:
> +    maxItems: 1
> +
> +  ranges: true
> +
> +  interrupts:
> +    items:
> +      - description: tamper detector interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: tampdi

Don't really need -names with only 1 entry.

> +
> +  clocks:
> +    items:
> +      - description: VBATTB module clock
> +
> +  clock-names:
> +    items:
> +      - const: bclk
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    items:
> +      - description: VBATTB module reset
> +
> +  '#address-cells':
> +    const: 2
> +
> +  '#size-cells':
> +    const: 2
> +
> +patternProperties:
> +  "^clock-controller@1c+$":
> +    type: object
> +    description: VBATTCLK clock
> +
> +    properties:
> +      compatible:
> +        const: renesas,r9a08g045-vbattb-clk
> +
> +      reg:
> +        maxItems: 1
> +
> +      clocks:
> +        items:
> +          - description: input clock for VBATTCLK
> +
> +      clock-names:
> +        description: |
> +          Use xin if connected to an external crystal oscillator.
> +          Use clkin if connected to an external hardware device generating the
> +          clock.
> +        enum:
> +          - xin
> +          - clkin
> +
> +      '#clock-cells':
> +        const: 0
> +
> +      renesas,vbattb-load-nanofarads:
> +        description: load capacitance of the on board xtal
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [ 4000, 7000, 9000, 12500 ]
> +
> +    required:
> +      - compatible
> +      - reg
> +      - clocks
> +      - clock-names
> +      - '#clock-cells'
> +      - renesas,vbattb-load-nanofarads
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - resets
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r9a08g045-cpg.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    vbattb@1005c000 {
> +        compatible = "renesas,r9a08g045-vbattb";
> +        reg = <0x1005c000 0x1000>;
> +        ranges = <0 0 0x1005c000 0 0x1000>;
> +        interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-names = "tampdi";
> +        clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>;
> +        clock-names = "bclk";
> +        power-domains = <&cpg>;
> +        resets = <&cpg R9A08G045_VBAT_BRESETN>;
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        clock-controller@1c {
> +            compatible = "renesas,r9a08g045-vbattb-clk";
> +            reg = <0 0x1c 0 0x10>;
> +            clocks = <&vbattb_xtal>;
> +            clock-names = "xin";
> +            #clock-cells = <0>;
> +            renesas,vbattb-load-nanofarads = <12500>;
> +        };

Is this really a separate device? Doesn't really look like it. This can 
all be moved to the parent node.

Rob

Re: [PATCH v2 01/11] dt-bindings: mfd: renesas,r9a08g045-vbattb: Document VBATTB

[claudiu beznea]

Hi, Rob,

On 23.07.2024 05:17, Rob Herring wrote:
> On Tue, Jul 16, 2024 at 01:30:15PM +0300, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The VBATTB IP of the Renesas RZ/G3S SoC controls the clock for RTC,
>> the tamper detector and a small general usage memory of 128B. Add
>> documentation for it.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - changed file name and compatible
>> - updated title, description sections
>> - added clock controller part documentation and drop dedicated file
>>   for it included in v1
>> - used items to describe interrupts, interrupt-names, clocks, clock-names,
>>   resets
>> - dropped node labels and status
>> - updated clock-names for clock controller to cope with the new
>>   logic on detecting the necessity to setup bypass
>>
>>  .../mfd/renesas,r9a08g045-vbattb.yaml         | 136 ++++++++++++++++++
>>  1 file changed, 136 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml b/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
>> new file mode 100644
>> index 000000000000..30e4da65e2f6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml
>> @@ -0,0 +1,136 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/renesas,r9a08g045-vbattb.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Renesas Battery Backup Function (VBATTB)
>> +
>> +description:
>> +  Renesas VBATTB is an always on powered module (backed by battery) which
>> +  controls the RTC clock (VBATTCLK), tamper detection logic and a small
>> +  general usage memory (128B).
>> +
>> +maintainers:
>> +  - Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: renesas,r9a08g045-vbattb
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  ranges: true
>> +
>> +  interrupts:
>> +    items:
>> +      - description: tamper detector interrupt
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: tampdi
> 
> Don't really need -names with only 1 entry.
> 
>> +
>> +  clocks:
>> +    items:
>> +      - description: VBATTB module clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: bclk
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    items:
>> +      - description: VBATTB module reset
>> +
>> +  '#address-cells':
>> +    const: 2
>> +
>> +  '#size-cells':
>> +    const: 2
>> +
>> +patternProperties:
>> +  "^clock-controller@1c+$":
>> +    type: object
>> +    description: VBATTCLK clock
>> +
>> +    properties:
>> +      compatible:
>> +        const: renesas,r9a08g045-vbattb-clk
>> +
>> +      reg:
>> +        maxItems: 1
>> +
>> +      clocks:
>> +        items:
>> +          - description: input clock for VBATTCLK
>> +
>> +      clock-names:
>> +        description: |
>> +          Use xin if connected to an external crystal oscillator.
>> +          Use clkin if connected to an external hardware device generating the
>> +          clock.
>> +        enum:
>> +          - xin
>> +          - clkin
>> +
>> +      '#clock-cells':
>> +        const: 0
>> +
>> +      renesas,vbattb-load-nanofarads:
>> +        description: load capacitance of the on board xtal
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [ 4000, 7000, 9000, 12500 ]
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +      - clocks
>> +      - clock-names
>> +      - '#clock-cells'
>> +      - renesas,vbattb-load-nanofarads
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - interrupt-names
>> +  - clocks
>> +  - clock-names
>> +  - power-domains
>> +  - resets
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/r9a08g045-cpg.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    vbattb@1005c000 {
>> +        compatible = "renesas,r9a08g045-vbattb";
>> +        reg = <0x1005c000 0x1000>;
>> +        ranges = <0 0 0x1005c000 0 0x1000>;
>> +        interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
>> +        interrupt-names = "tampdi";
>> +        clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>;
>> +        clock-names = "bclk";
>> +        power-domains = <&cpg>;
>> +        resets = <&cpg R9A08G045_VBAT_BRESETN>;
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        clock-controller@1c {
>> +            compatible = "renesas,r9a08g045-vbattb-clk";
>> +            reg = <0 0x1c 0 0x10>;
>> +            clocks = <&vbattb_xtal>;
>> +            clock-names = "xin";
>> +            #clock-cells = <0>;
>> +            renesas,vbattb-load-nanofarads = <12500>;
>> +        };
> 
> Is this really a separate device?

It's not.

> Doesn't really look like it. This can 
> all be moved to the parent node.

I'll move it to the parent node.

Thank you for your review,
Claudiu Beznea

> 
> Rob

[PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[Claudiu]

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

Renesas VBATTB IP has logic to control the RTC clock, tamper detection
and a small 128B memory. Add a MFD driver to do the basic initialization
of the VBATTB IP for the inner components to work.

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

Changes in v2:
- none; this driver is new

 drivers/mfd/Kconfig          |  8 ++++
 drivers/mfd/Makefile         |  1 +
 drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/mfd/renesas-vbattb.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bc8be2e593b6..df93e8b05065 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
 	  This driver provides common support for accessing the SC27xx PMICs,
 	  and it also adds the irq_chip parts for handling the PMIC chip events.
 
+config MFD_RENESAS_VBATTB
+	tristate "Renesas VBATTB driver"
+	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
+	select MFD_CORE
+	help
+	  Select this option to enable Renesas RZ/G3S VBATTB driver which
+	  provides support for the RTC clock, tamper detector and 128B SRAM.
+
 config RZ_MTU3
 	tristate "Renesas RZ/G2L MTU3a core driver"
 	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 02b651cd7535..cd2f27492df2 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
 obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
 obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
 obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
+obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
 obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
 obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
 obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c
new file mode 100644
index 000000000000..5d71565b8cbf
--- /dev/null
+++ b/drivers/mfd/renesas-vbattb.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * VBATTB driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#include <linux/mod_devicetable.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+static int vbattb_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct reset_control *rstc;
+	int ret;
+
+	rstc = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR(rstc))
+		return PTR_ERR(rstc);
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(rstc);
+	if (ret)
+		goto rpm_put;
+
+	platform_set_drvdata(pdev, rstc);
+
+	ret = devm_of_platform_populate(dev);
+	if (ret)
+		goto reset_assert;
+
+	return 0;
+
+reset_assert:
+	reset_control_assert(rstc);
+rpm_put:
+	pm_runtime_put(dev);
+	return ret;
+}
+
+static void vbattb_remove(struct platform_device *pdev)
+{
+	struct reset_control *rstc = platform_get_drvdata(pdev);
+
+	reset_control_assert(rstc);
+	pm_runtime_put(&pdev->dev);
+}
+
+static const struct of_device_id vbattb_match[] = {
+	{ .compatible = "renesas,r9a08g045-vbattb" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, vbattb_match);
+
+static struct platform_driver vbattb_driver = {
+	.probe = vbattb_probe,
+	.remove_new = vbattb_remove,
+	.driver = {
+		.name = "renesas-vbattb",
+		.of_match_table = vbattb_match,
+	},
+};
+module_platform_driver(vbattb_driver);
+
+MODULE_ALIAS("platform:renesas-vbattb");
+MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas VBATTB driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

RE: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[Biju Das]

Hi Claudiu,

Thanks for the patch.


> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Tuesday, July 16, 2024 11:30 AM
> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Renesas VBATTB IP has logic to control the RTC clock, tamper detection and a small 128B memory. Add a
> MFD driver to do the basic initialization of the VBATTB IP for the inner components to work.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this driver is new
> 
>  drivers/mfd/Kconfig          |  8 ++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/mfd/renesas-vbattb.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index bc8be2e593b6..df93e8b05065 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>  	  This driver provides common support for accessing the SC27xx PMICs,
>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
> 
> +config MFD_RENESAS_VBATTB
> +	tristate "Renesas VBATTB driver"
> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> +	select MFD_CORE

There is no MFD calls??  What is the purpose of selecting MFD_CORE??

> +	help
> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> +
>  config RZ_MTU3
>  	tristate "Renesas RZ/G2L MTU3a core driver"
>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile
> b/drivers/mfd/Makefile index 02b651cd7535..cd2f27492df2 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
> 000000000000..5d71565b8cbf
> --- /dev/null
> +++ b/drivers/mfd/renesas-vbattb.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VBATTB driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +static int vbattb_probe(struct platform_device *pdev) {
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int ret;
> +
> +	rstc = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(rstc))
> +		return PTR_ERR(rstc);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(rstc);
> +	if (ret)
> +		goto rpm_put;
> +
> +	platform_set_drvdata(pdev, rstc);
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret)
> +		goto reset_assert;
> +
> +	return 0;
> +
> +reset_assert:
> +	reset_control_assert(rstc);
> +rpm_put:
> +	pm_runtime_put(dev);
> +	return ret;
> +}
> +
> +static void vbattb_remove(struct platform_device *pdev) {
> +	struct reset_control *rstc = platform_get_drvdata(pdev);
> +
> +	reset_control_assert(rstc);
> +	pm_runtime_put(&pdev->dev);
> +}
> +
> +static const struct of_device_id vbattb_match[] = {
> +	{ .compatible = "renesas,r9a08g045-vbattb" },
> +	{ /* sentinel */ },

Drop comma.

> +};
> +MODULE_DEVICE_TABLE(of, vbattb_match);
> +
> +static struct platform_driver vbattb_driver = {
> +	.probe = vbattb_probe,
> +	.remove_new = vbattb_remove,

Maybe remove canbe replaced with devm_add_action_or_reset()
That simplifies probe() aswell??

> +	.driver = {
> +		.name = "renesas-vbattb",
> +		.of_match_table = vbattb_match,
> +	},
> +};
> +module_platform_driver(vbattb_driver);
> +
> +MODULE_ALIAS("platform:renesas-vbattb");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas VBATTB driver"); MODULE_LICENSE("GPL");
> --
> 2.39.2
> 

Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[claudiu beznea]

Hi, Biju,

On 16.07.2024 14:00, Biju Das wrote:
> Hi Claudiu,
> 
> Thanks for the patch.
> 
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Tuesday, July 16, 2024 11:30 AM
>> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Renesas VBATTB IP has logic to control the RTC clock, tamper detection and a small 128B memory. Add a
>> MFD driver to do the basic initialization of the VBATTB IP for the inner components to work.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this driver is new
>>
>>  drivers/mfd/Kconfig          |  8 ++++
>>  drivers/mfd/Makefile         |  1 +
>>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 87 insertions(+)
>>  create mode 100644 drivers/mfd/renesas-vbattb.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index bc8be2e593b6..df93e8b05065 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>>  	  This driver provides common support for accessing the SC27xx PMICs,
>>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>>
>> +config MFD_RENESAS_VBATTB
>> +	tristate "Renesas VBATTB driver"
>> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
>> +	select MFD_CORE
> 
> There is no MFD calls??  What is the purpose of selecting MFD_CORE??

I missed to remove it from here.

> 
>> +	help
>> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
>> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
>> +
>>  config RZ_MTU3
>>  	tristate "Renesas RZ/G2L MTU3a core driver"
>>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile
>> b/drivers/mfd/Makefile index 02b651cd7535..cd2f27492df2 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
>> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
>> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
>> 000000000000..5d71565b8cbf
>> --- /dev/null
>> +++ b/drivers/mfd/renesas-vbattb.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VBATTB driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +
>> +static int vbattb_probe(struct platform_device *pdev) {
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *rstc;
>> +	int ret;
>> +
>> +	rstc = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(rstc))
>> +		return PTR_ERR(rstc);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = reset_control_deassert(rstc);
>> +	if (ret)
>> +		goto rpm_put;
>> +
>> +	platform_set_drvdata(pdev, rstc);
>> +
>> +	ret = devm_of_platform_populate(dev);
>> +	if (ret)
>> +		goto reset_assert;
>> +
>> +	return 0;
>> +
>> +reset_assert:
>> +	reset_control_assert(rstc);
>> +rpm_put:
>> +	pm_runtime_put(dev);
>> +	return ret;
>> +}
>> +
>> +static void vbattb_remove(struct platform_device *pdev) {
>> +	struct reset_control *rstc = platform_get_drvdata(pdev);
>> +
>> +	reset_control_assert(rstc);
>> +	pm_runtime_put(&pdev->dev);
>> +}
>> +
>> +static const struct of_device_id vbattb_match[] = {
>> +	{ .compatible = "renesas,r9a08g045-vbattb" },
>> +	{ /* sentinel */ },
> 
> Drop comma.
> 
>> +};
>> +MODULE_DEVICE_TABLE(of, vbattb_match);
>> +
>> +static struct platform_driver vbattb_driver = {
>> +	.probe = vbattb_probe,
>> +	.remove_new = vbattb_remove,
> 
> Maybe remove canbe replaced with devm_add_action_or_reset()
> That simplifies probe() aswell??

This approach needs a new structure to keep references to the rstc and dev,
to be able to handle reset and runtime PM in action function. I wanted to
avoid adding a new structure.

Thank you for your review,
Claudiu Beznea

> 
>> +	.driver = {
>> +		.name = "renesas-vbattb",
>> +		.of_match_table = vbattb_match,
>> +	},
>> +};
>> +module_platform_driver(vbattb_driver);
>> +
>> +MODULE_ALIAS("platform:renesas-vbattb");
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
>> +MODULE_DESCRIPTION("Renesas VBATTB driver"); MODULE_LICENSE("GPL");
>> --
>> 2.39.2
>>
> 

RE: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[Biju Das]

Hi Claudium

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, July 17, 2024 8:38 AM
@vger.kernel.org; Claudiu Beznea
> <claudiu.beznea.uj@bp.renesas.com>
> Subject: Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for
> the Renesas VBATTB IP
> 
> Hi, Biju,
> 
> On 16.07.2024 14:00, Biju Das wrote:
> > Hi Claudiu,
> >
> > Thanks for the patch.
> >
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Tuesday, July 16, 2024 11:30 AM
> >> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for
> >> the Renesas VBATTB IP
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Renesas VBATTB IP has logic to control the RTC clock, tamper
> >> detection and a small 128B memory. Add a MFD driver to do the basic
> initialization of the VBATTB IP for the inner components to work.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v2:
> >> - none; this driver is new
> >>
> >>  drivers/mfd/Kconfig          |  8 ++++
> >>  drivers/mfd/Makefile         |  1 +
> >>  drivers/mfd/renesas-vbattb.c | 78
> >> ++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 87 insertions(+)
> >>  create mode 100644 drivers/mfd/renesas-vbattb.c
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> >> bc8be2e593b6..df93e8b05065 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
> >>  	  This driver provides common support for accessing the SC27xx PMICs,
> >>  	  and it also adds the irq_chip parts for handling the PMIC chip
> events.
> >>
> >> +config MFD_RENESAS_VBATTB
> >> +	tristate "Renesas VBATTB driver"
> >> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> >> +	select MFD_CORE
> >
> > There is no MFD calls??  What is the purpose of selecting MFD_CORE??
> 
> I missed to remove it from here.
> 
> >
> >> +	help
> >> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> >> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> >> +
> >>  config RZ_MTU3
> >>  	tristate "Renesas RZ/G2L MTU3a core driver"
> >>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git
> >> a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> >> 02b651cd7535..cd2f27492df2 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o
> pcf50633-irq.o
> >>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
> >>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
> >>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> >> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
> >>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
> >>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
> >>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> >> diff --git a/drivers/mfd/renesas-vbattb.c
> >> b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
> >> 000000000000..5d71565b8cbf
> >> --- /dev/null
> >> +++ b/drivers/mfd/renesas-vbattb.c
> >> @@ -0,0 +1,78 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * VBATTB driver
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#include <linux/mod_devicetable.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/reset.h>
> >> +
> >> +static int vbattb_probe(struct platform_device *pdev) {
> >> +	struct device *dev = &pdev->dev;
> >> +	struct reset_control *rstc;
> >> +	int ret;
> >> +
> >> +	rstc = devm_reset_control_array_get_exclusive(dev);
> >> +	if (IS_ERR(rstc))
> >> +		return PTR_ERR(rstc);
> >> +
> >> +	ret = devm_pm_runtime_enable(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = pm_runtime_resume_and_get(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = reset_control_deassert(rstc);
> >> +	if (ret)
> >> +		goto rpm_put;
> >> +
> >> +	platform_set_drvdata(pdev, rstc);
> >> +
> >> +	ret = devm_of_platform_populate(dev);
> >> +	if (ret)
> >> +		goto reset_assert;
> >> +
> >> +	return 0;
> >> +
> >> +reset_assert:
> >> +	reset_control_assert(rstc);
> >> +rpm_put:
> >> +	pm_runtime_put(dev);
> >> +	return ret;
> >> +}
> >> +
> >> +static void vbattb_remove(struct platform_device *pdev) {
> >> +	struct reset_control *rstc = platform_get_drvdata(pdev);
> >> +
> >> +	reset_control_assert(rstc);
> >> +	pm_runtime_put(&pdev->dev);
> >> +}
> >> +
> >> +static const struct of_device_id vbattb_match[] = {
> >> +	{ .compatible = "renesas,r9a08g045-vbattb" },
> >> +	{ /* sentinel */ },
> >
> > Drop comma.
> >
> >> +};
> >> +MODULE_DEVICE_TABLE(of, vbattb_match);
> >> +
> >> +static struct platform_driver vbattb_driver = {
> >> +	.probe = vbattb_probe,
> >> +	.remove_new = vbattb_remove,
> >
> > Maybe remove canbe replaced with devm_add_action_or_reset() That
> > simplifies probe() aswell??
> 
> This approach needs a new structure to keep references to the rstc and dev,
> to be able to handle reset and runtime PM in action function. I wanted to
> avoid adding a new structure.


You can pass dev as context parameter in devm_add_action_or_reset(), from that you get rstc=dev_get_drvdata(dev); 
So you don't need new structure.

Cheers,
Biju

RE: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Tuesday, July 16, 2024 11:30 AM
> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Renesas VBATTB IP has logic to control the RTC clock, tamper detection and a small 128B memory. Add a
> MFD driver to do the basic initialization of the VBATTB IP for the inner components to work.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this driver is new
> 
>  drivers/mfd/Kconfig          |  8 ++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/mfd/renesas-vbattb.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index bc8be2e593b6..df93e8b05065 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>  	  This driver provides common support for accessing the SC27xx PMICs,
>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
> 
> +config MFD_RENESAS_VBATTB
> +	tristate "Renesas VBATTB driver"
> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> +	select MFD_CORE
> +	help
> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> +
>  config RZ_MTU3
>  	tristate "Renesas RZ/G2L MTU3a core driver"
>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile
> b/drivers/mfd/Makefile index 02b651cd7535..cd2f27492df2 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
> 000000000000..5d71565b8cbf
> --- /dev/null
> +++ b/drivers/mfd/renesas-vbattb.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VBATTB driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +static int vbattb_probe(struct platform_device *pdev) {
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int ret;
> +
> +	rstc = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(rstc))
> +		return PTR_ERR(rstc);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;

Maybe as an optimization drop pm calls and use "devm_clk_get_enabled"
instead as it perfectly fits in this scenario??

> +
> +	ret = reset_control_deassert(rstc);
> +	if (ret)
> +		goto rpm_put;
> +
> +	platform_set_drvdata(pdev, rstc);
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret)
> +		goto reset_assert;
> +
> +	return 0;
> +
> +reset_assert:
> +	reset_control_assert(rstc);
> +rpm_put:
> +	pm_runtime_put(dev);
> +	return ret;
> +}
> +
> +static void vbattb_remove(struct platform_device *pdev) {
> +	struct reset_control *rstc = platform_get_drvdata(pdev);
> +
> +	reset_control_assert(rstc);
> +	pm_runtime_put(&pdev->dev);
> +}
> +
> +static const struct of_device_id vbattb_match[] = {
> +	{ .compatible = "renesas,r9a08g045-vbattb" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, vbattb_match);
> +
> +static struct platform_driver vbattb_driver = {
> +	.probe = vbattb_probe,
> +	.remove_new = vbattb_remove,
> +	.driver = {
> +		.name = "renesas-vbattb",
> +		.of_match_table = vbattb_match,
> +	},
> +};
> +module_platform_driver(vbattb_driver);
> +
> +MODULE_ALIAS("platform:renesas-vbattb");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas VBATTB driver"); MODULE_LICENSE("GPL");
> --
> 2.39.2
> 

Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[claudiu beznea]

Hi, Biju,

On 16.07.2024 14:07, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Tuesday, July 16, 2024 11:30 AM
>> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Renesas VBATTB IP has logic to control the RTC clock, tamper detection and a small 128B memory. Add a
>> MFD driver to do the basic initialization of the VBATTB IP for the inner components to work.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this driver is new
>>
>>  drivers/mfd/Kconfig          |  8 ++++
>>  drivers/mfd/Makefile         |  1 +
>>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 87 insertions(+)
>>  create mode 100644 drivers/mfd/renesas-vbattb.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index bc8be2e593b6..df93e8b05065 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>>  	  This driver provides common support for accessing the SC27xx PMICs,
>>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>>
>> +config MFD_RENESAS_VBATTB
>> +	tristate "Renesas VBATTB driver"
>> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
>> +	select MFD_CORE
>> +	help
>> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
>> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
>> +
>>  config RZ_MTU3
>>  	tristate "Renesas RZ/G2L MTU3a core driver"
>>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile
>> b/drivers/mfd/Makefile index 02b651cd7535..cd2f27492df2 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
>> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
>> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
>> 000000000000..5d71565b8cbf
>> --- /dev/null
>> +++ b/drivers/mfd/renesas-vbattb.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VBATTB driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +
>> +static int vbattb_probe(struct platform_device *pdev) {
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *rstc;
>> +	int ret;
>> +
>> +	rstc = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(rstc))
>> +		return PTR_ERR(rstc);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		return ret;
> 
> Maybe as an optimization drop pm calls and use "devm_clk_get_enabled"
> instead as it perfectly fits in this scenario??

VBATTB is still part of a PM domain. That needs to be enabled as well.
pm_runtime_* APIs takes care of both clock enable and power domain on
operations.

One thing to notice is that on RZ/G3S the VBATTB clock is CRITICAL (thus
enabled in the probe of the clock driver), the PM domain is always ON (and
also enabled by clock driver). We can get rid of pm_runtime_* at all for
RZ/G3S but I think it would be better to have it here as well for future
platforms and to emphasize from driver that these resources are needed by
the VBATTB. The same approach is used for  other RZ/G2 drivers that
declares critical clocks/always-on domains (e.g. dma driver, IA55 driver).

Thank you,
Claudiu Beznea

> 
>> +
>> +	ret = reset_control_deassert(rstc);
>> +	if (ret)
>> +		goto rpm_put;
>> +
>> +	platform_set_drvdata(pdev, rstc);
>> +
>> +	ret = devm_of_platform_populate(dev);
>> +	if (ret)
>> +		goto reset_assert;
>> +
>> +	return 0;
>> +
>> +reset_assert:
>> +	reset_control_assert(rstc);
>> +rpm_put:
>> +	pm_runtime_put(dev);
>> +	return ret;
>> +}
>> +
>> +static void vbattb_remove(struct platform_device *pdev) {
>> +	struct reset_control *rstc = platform_get_drvdata(pdev);
>> +
>> +	reset_control_assert(rstc);
>> +	pm_runtime_put(&pdev->dev);
>> +}
>> +
>> +static const struct of_device_id vbattb_match[] = {
>> +	{ .compatible = "renesas,r9a08g045-vbattb" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, vbattb_match);
>> +
>> +static struct platform_driver vbattb_driver = {
>> +	.probe = vbattb_probe,
>> +	.remove_new = vbattb_remove,
>> +	.driver = {
>> +		.name = "renesas-vbattb",
>> +		.of_match_table = vbattb_match,
>> +	},
>> +};
>> +module_platform_driver(vbattb_driver);
>> +
>> +MODULE_ALIAS("platform:renesas-vbattb");
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
>> +MODULE_DESCRIPTION("Renesas VBATTB driver"); MODULE_LICENSE("GPL");
>> --
>> 2.39.2
>>
> 

RE: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, July 17, 2024 8:46 AM
> Subject: Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for
> the Renesas VBATTB IP
> 
> Hi, Biju,
> 
> On 16.07.2024 14:07, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Tuesday, July 16, 2024 11:30 AM
> >> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for
> >> the Renesas VBATTB IP
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Renesas VBATTB IP has logic to control the RTC clock, tamper
> >> detection and a small 128B memory. Add a MFD driver to do the basic
> initialization of the VBATTB IP for the inner components to work.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v2:
> >> - none; this driver is new
> >>
> >>  drivers/mfd/Kconfig          |  8 ++++
> >>  drivers/mfd/Makefile         |  1 +
> >>  drivers/mfd/renesas-vbattb.c | 78
> >> ++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 87 insertions(+)
> >>  create mode 100644 drivers/mfd/renesas-vbattb.c
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> >> bc8be2e593b6..df93e8b05065 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
> >>  	  This driver provides common support for accessing the SC27xx PMICs,
> >>  	  and it also adds the irq_chip parts for handling the PMIC chip
> events.
> >>
> >> +config MFD_RENESAS_VBATTB
> >> +	tristate "Renesas VBATTB driver"
> >> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> >> +	select MFD_CORE
> >> +	help
> >> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> >> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> >> +
> >>  config RZ_MTU3
> >>  	tristate "Renesas RZ/G2L MTU3a core driver"
> >>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git
> >> a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> >> 02b651cd7535..cd2f27492df2 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o
> pcf50633-irq.o
> >>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
> >>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
> >>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> >> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
> >>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
> >>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
> >>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> >> diff --git a/drivers/mfd/renesas-vbattb.c
> >> b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
> >> 000000000000..5d71565b8cbf
> >> --- /dev/null
> >> +++ b/drivers/mfd/renesas-vbattb.c
> >> @@ -0,0 +1,78 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * VBATTB driver
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#include <linux/mod_devicetable.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/reset.h>
> >> +
> >> +static int vbattb_probe(struct platform_device *pdev) {
> >> +	struct device *dev = &pdev->dev;
> >> +	struct reset_control *rstc;
> >> +	int ret;
> >> +
> >> +	rstc = devm_reset_control_array_get_exclusive(dev);
> >> +	if (IS_ERR(rstc))
> >> +		return PTR_ERR(rstc);
> >> +
> >> +	ret = devm_pm_runtime_enable(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = pm_runtime_resume_and_get(dev);
> >> +	if (ret)
> >> +		return ret;
> >
> > Maybe as an optimization drop pm calls and use "devm_clk_get_enabled"
> > instead as it perfectly fits in this scenario??
> 
> VBATTB is still part of a PM domain. That needs to be enabled as well.
> pm_runtime_* APIs takes care of both clock enable and power domain on
> operations.
> 
> One thing to notice is that on RZ/G3S the VBATTB clock is CRITICAL (thus
> enabled in the probe of the clock driver), the PM domain is always ON (and
> also enabled by clock driver). We can get rid of pm_runtime_* at all for
> RZ/G3S but I think it would be better to have it here as well for future
> platforms and to emphasize from driver that these resources are needed by
> the VBATTB. The same approach is used for  other RZ/G2 drivers that
> declares critical clocks/always-on domains (e.g. dma driver, IA55 driver).


If it is critical clock, the parent has to be critical clock. So you can blindly
use devm_clk_get_enabled() that simplifies probe(), if the PM domain controls only clock.


If the PM domain controls voltage means you should use PM domain APIs

Cheers,
Biju

Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[Krzysztof Kozlowski]

On 16/07/2024 12:30, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Renesas VBATTB IP has logic to control the RTC clock, tamper detection
> and a small 128B memory. Add a MFD driver to do the basic initialization
> of the VBATTB IP for the inner components to work.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---


> +
> +static struct platform_driver vbattb_driver = {
> +	.probe = vbattb_probe,
> +	.remove_new = vbattb_remove,
> +	.driver = {
> +		.name = "renesas-vbattb",
> +		.of_match_table = vbattb_match,
> +	},
> +};
> +module_platform_driver(vbattb_driver);
> +
> +MODULE_ALIAS("platform:renesas-vbattb");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas VBATTB driver");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof

Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[claudiu beznea]

Hi, Krzysztof,

On 16.07.2024 22:29, Krzysztof Kozlowski wrote:
> On 16/07/2024 12:30, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Renesas VBATTB IP has logic to control the RTC clock, tamper detection
>> and a small 128B memory. Add a MFD driver to do the basic initialization
>> of the VBATTB IP for the inner components to work.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
> 
> 
>> +
>> +static struct platform_driver vbattb_driver = {
>> +	.probe = vbattb_probe,
>> +	.remove_new = vbattb_remove,
>> +	.driver = {
>> +		.name = "renesas-vbattb",
>> +		.of_match_table = vbattb_match,
>> +	},
>> +};
>> +module_platform_driver(vbattb_driver);
>> +
>> +MODULE_ALIAS("platform:renesas-vbattb");
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.

It's not needed. I took another driver as reference and missed to removed
it from here in the end.

Thank  you for your review,
Claudiu Beznea


> 
> 
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
>> +MODULE_DESCRIPTION("Renesas VBATTB driver");
>> +MODULE_LICENSE("GPL");
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[Lee Jones]

On Tue, 16 Jul 2024, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Renesas VBATTB IP has logic to control the RTC clock, tamper detection
> and a small 128B memory. Add a MFD driver to do the basic initialization
> of the VBATTB IP for the inner components to work.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this driver is new
> 
>  drivers/mfd/Kconfig          |  8 ++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/mfd/renesas-vbattb.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bc8be2e593b6..df93e8b05065 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>  	  This driver provides common support for accessing the SC27xx PMICs,
>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>  
> +config MFD_RENESAS_VBATTB
> +	tristate "Renesas VBATTB driver"
> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> +	select MFD_CORE
> +	help
> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> +
>  config RZ_MTU3
>  	tristate "Renesas RZ/G2L MTU3a core driver"
>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 02b651cd7535..cd2f27492df2 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c
> new file mode 100644
> index 000000000000..5d71565b8cbf
> --- /dev/null
> +++ b/drivers/mfd/renesas-vbattb.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VBATTB driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +static int vbattb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int ret;
> +
> +	rstc = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(rstc))
> +		return PTR_ERR(rstc);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(rstc);
> +	if (ret)
> +		goto rpm_put;
> +
> +	platform_set_drvdata(pdev, rstc);

Where is this consumed?

> +	ret = devm_of_platform_populate(dev);


Which devices will this probe?

> +	if (ret)
> +		goto reset_assert;
> +
> +	return 0;
> +
> +reset_assert:
> +	reset_control_assert(rstc);
> +rpm_put:
> +	pm_runtime_put(dev);
> +	return ret;
> +}
> +
> +static void vbattb_remove(struct platform_device *pdev)
> +{
> +	struct reset_control *rstc = platform_get_drvdata(pdev);
> +
> +	reset_control_assert(rstc);
> +	pm_runtime_put(&pdev->dev);
> +}
> +
> +static const struct of_device_id vbattb_match[] = {
> +	{ .compatible = "renesas,r9a08g045-vbattb" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, vbattb_match);
> +
> +static struct platform_driver vbattb_driver = {
> +	.probe = vbattb_probe,
> +	.remove_new = vbattb_remove,
> +	.driver = {
> +		.name = "renesas-vbattb",
> +		.of_match_table = vbattb_match,
> +	},
> +};
> +module_platform_driver(vbattb_driver);
> +
> +MODULE_ALIAS("platform:renesas-vbattb");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas VBATTB driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.39.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

[claudiu beznea]

Hi, Lee,

On 24.07.2024 17:53, Lee Jones wrote:
> On Tue, 16 Jul 2024, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Renesas VBATTB IP has logic to control the RTC clock, tamper detection
>> and a small 128B memory. Add a MFD driver to do the basic initialization
>> of the VBATTB IP for the inner components to work.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this driver is new
>>
>>  drivers/mfd/Kconfig          |  8 ++++
>>  drivers/mfd/Makefile         |  1 +
>>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 87 insertions(+)
>>  create mode 100644 drivers/mfd/renesas-vbattb.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index bc8be2e593b6..df93e8b05065 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>>  	  This driver provides common support for accessing the SC27xx PMICs,
>>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>>  
>> +config MFD_RENESAS_VBATTB
>> +	tristate "Renesas VBATTB driver"
>> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
>> +	select MFD_CORE
>> +	help
>> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
>> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
>> +
>>  config RZ_MTU3
>>  	tristate "Renesas RZ/G2L MTU3a core driver"
>>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 02b651cd7535..cd2f27492df2 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
>> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
>> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c
>> new file mode 100644
>> index 000000000000..5d71565b8cbf
>> --- /dev/null
>> +++ b/drivers/mfd/renesas-vbattb.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VBATTB driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +
>> +static int vbattb_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *rstc;
>> +	int ret;
>> +
>> +	rstc = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(rstc))
>> +		return PTR_ERR(rstc);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = reset_control_deassert(rstc);
>> +	if (ret)
>> +		goto rpm_put;
>> +
>> +	platform_set_drvdata(pdev, rstc);
> 
> Where is this consumed?

In vbattb_remove().

> 
>> +	ret = devm_of_platform_populate(dev);
> 
> 
> Which devices will this probe?

In this version it is used by clock logic from VBATTB IP, modeled as
individual device. A schema of the blocks controlled in the VBATTB IP can
be found at [1] (please note that there is also the RTC mentioned there but
because it is on the PD_VBATT always-on power domain (backed by battery);
in fact, it is an individual device mapped at it's own address). Here:
- the 32KHz-clock oscillator, mux (with XC, XBYP inputs), CGC are used in
  the cock driver (introduced in this series)
- tamper detector module with gate controlled by TAMPICR1 being part of
  tamper detection logic (no driver for this ATM)
- backup registers being the SRAM (tried locally with mmio-sram driver
  (drives/misc/sram.c)), subnode described with (for internal development):

vbattb: vbattb@1005c000 {

	// ...

	backup_sram: sram@80 {
		compatible = "mmio-sram";
		reg = <0 0x80 0 0x80>;
		clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>;
		clock-names = "bclk";
		power-domains = <&cpg>;
		resets = <&cpg R9A08G045_VBAT_BRESETN>;
		no-memory-wc;

		#address-cells = <2>;
		#size-cells = <2>;
		ranges = <0 0 0 0x80 0 0x80>;

		pool@0 {
			reg = <0 0 0 0x80>;
			label = "sram-test";
			export;
		};
	};
};

My initial idea was to have logic blocks of the VBATTB IP grouped as
devices (clock for the moment and for future, at least the small SRAM, if
needed at some point). After the discussion with Stephen on clock driver I
tend to give up this model.

Please let me know if you have any hints on how to go forward.

Thank you for  your review,
Claudiu Beznea

[1] https://i2.paste.pics/RFKJ0.png?rand=Xq8w1RLDvZ

> 
>> +	if (ret)
>> +		goto reset_assert;
>> +
>> +	return 0;
>> +
>> +reset_assert:
>> +	reset_control_assert(rstc);
>> +rpm_put:
>> +	pm_runtime_put(dev);
>> +	return ret;
>> +}
>> +
>> +static void vbattb_remove(struct platform_device *pdev)
>> +{
>> +	struct reset_control *rstc = platform_get_drvdata(pdev);
>> +
>> +	reset_control_assert(rstc);
>> +	pm_runtime_put(&pdev->dev);
>> +}
>> +
>> +static const struct of_device_id vbattb_match[] = {
>> +	{ .compatible = "renesas,r9a08g045-vbattb" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, vbattb_match);
>> +
>> +static struct platform_driver vbattb_driver = {
>> +	.probe = vbattb_probe,
>> +	.remove_new = vbattb_remove,
>> +	.driver = {
>> +		.name = "renesas-vbattb",
>> +		.of_match_table = vbattb_match,
>> +	},
>> +};
>> +module_platform_driver(vbattb_driver);
>> +
>> +MODULE_ALIAS("platform:renesas-vbattb");
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
>> +MODULE_DESCRIPTION("Renesas VBATTB driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.39.2
>>
> 

[PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[Claudiu]

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

The VBATTB IP of the Renesas RZ/G3S SoC controls the clock that is used
by the RTC. The input to the VBATTB could be a 32KHz crystal oscillator
or an external clock device. The driver detects the type of the input clock
based on the device tree clock name (xin for crystal, clkin for external
clock device).

The load capacitance of the on-board oscillator need to be configured with
renesas,vbattb-load-nanofarads DT property.

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

Changes in v2:
- updated patch description
- added vendor name in Kconfig flag
- used cleanup.h lock helpers
- dropped the MFD code
- updated registers offsets
- added vbattb_clk_update_bits() and used it where possible
- added vbattb_clk_need_bypass() to detect the bypass setup necessity
- changed the compatible and driver names

 drivers/clk/renesas/Kconfig      |   5 +
 drivers/clk/renesas/Makefile     |   1 +
 drivers/clk/renesas/clk-vbattb.c | 212 +++++++++++++++++++++++++++++++
 3 files changed, 218 insertions(+)
 create mode 100644 drivers/clk/renesas/clk-vbattb.c

diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
index 4410d16de4e2..1f5f38136eb2 100644
--- a/drivers/clk/renesas/Kconfig
+++ b/drivers/clk/renesas/Kconfig
@@ -228,6 +228,11 @@ config CLK_RZG2L
 	bool "RZ/{G2L,G2UL,G3S,V2L} family clock support" if COMPILE_TEST
 	select RESET_CONTROLLER
 
+config CLK_RENESAS_VBATTB
+	bool "Renesas VBATTB clock controller"
+	depends on MFD_RENESAS_VBATTB
+	select RESET_CONTROLLER
+
 # Generic
 config CLK_RENESAS_CPG_MSSR
 	bool "CPG/MSSR clock support" if COMPILE_TEST
diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefile
index f7e18679c3b8..84a2783a7b46 100644
--- a/drivers/clk/renesas/Makefile
+++ b/drivers/clk/renesas/Makefile
@@ -51,3 +51,4 @@ obj-$(CONFIG_CLK_RZG2L)			+= rzg2l-cpg.o
 obj-$(CONFIG_CLK_RENESAS_CPG_MSSR)	+= renesas-cpg-mssr.o
 obj-$(CONFIG_CLK_RENESAS_CPG_MSTP)	+= clk-mstp.o
 obj-$(CONFIG_CLK_RENESAS_DIV6)		+= clk-div6.o
+obj-$(CONFIG_CLK_RENESAS_VBATTB)	+= clk-vbattb.o
diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c
new file mode 100644
index 000000000000..8effe141fc0b
--- /dev/null
+++ b/drivers/clk/renesas/clk-vbattb.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * VBATTB clock driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#define VBATTB_BKSCCR			0x0
+#define VBATTB_BKSCCR_SOSEL		BIT(6)
+#define VBATTB_SOSCCR2			0x8
+#define VBATTB_SOSCCR2_SOSTP2		BIT(0)
+#define VBATTB_XOSCCR			0x14
+#define VBATTB_XOSCCR_OUTEN		BIT(16)
+#define VBATTB_XOSCCR_XSEL		GENMASK(1, 0)
+#define VBATTB_XOSCCR_XSEL_4_PF		0x0
+#define VBATTB_XOSCCR_XSEL_7_PF		0x1
+#define VBATTB_XOSCCR_XSEL_9_PF		0x2
+#define VBATTB_XOSCCR_XSEL_12_5_PF	0x3
+
+/**
+ * struct vbattb_clk - VBATTB clock data structure
+ * @base: base address
+ * @hw: clk hw
+ * @lock: lock
+ * @load_capacitance: load capacitance
+ */
+struct vbattb_clk {
+	void __iomem *base;
+	struct clk_hw hw;
+	spinlock_t lock;
+	u8 load_capacitance;
+};
+
+#define to_vbattb_clk(_hw) container_of(_hw, struct vbattb_clk, hw)
+
+static void vbattb_clk_update_bits(void __iomem *base, u32 offset, u32 mask, u32 val)
+{
+	u32 tmp;
+
+	tmp = readl_relaxed(base + offset);
+	tmp &= ~mask;
+	tmp |= (val & mask);
+	writel_relaxed(tmp, base + offset);
+}
+
+static int vbattb_clk_enable(struct clk_hw *hw)
+{
+	struct vbattb_clk *vbclk = to_vbattb_clk(hw);
+	void __iomem *base = vbclk->base;
+
+	guard(spinlock)(&vbclk->lock);
+
+	vbattb_clk_update_bits(base, VBATTB_SOSCCR2, VBATTB_SOSCCR2_SOSTP2, 0);
+	vbattb_clk_update_bits(base, VBATTB_XOSCCR, VBATTB_XOSCCR_OUTEN | VBATTB_XOSCCR_XSEL,
+			       VBATTB_XOSCCR_OUTEN | vbclk->load_capacitance);
+
+	return 0;
+}
+
+static void vbattb_clk_disable(struct clk_hw *hw)
+{
+	struct vbattb_clk *vbclk = to_vbattb_clk(hw);
+	void __iomem *base = vbclk->base;
+
+	guard(spinlock)(&vbclk->lock);
+
+	vbattb_clk_update_bits(base, VBATTB_XOSCCR, VBATTB_XOSCCR_OUTEN, 0);
+	vbattb_clk_update_bits(base, VBATTB_SOSCCR2, VBATTB_SOSCCR2_SOSTP2, VBATTB_SOSCCR2_SOSTP2);
+}
+
+static int vbattb_clk_is_enabled(struct clk_hw *hw)
+{
+	struct vbattb_clk *vbclk = to_vbattb_clk(hw);
+	void __iomem *base = vbclk->base;
+	unsigned int xosccr, sosccr2;
+
+	guard(spinlock)(&vbclk->lock);
+
+	xosccr = readl_relaxed(base + VBATTB_XOSCCR);
+	sosccr2 = readl_relaxed(base + VBATTB_SOSCCR2);
+
+	return ((xosccr & VBATTB_XOSCCR_OUTEN) && !(sosccr2 & VBATTB_SOSCCR2_SOSTP2));
+}
+
+static const struct clk_ops vbattb_clk_ops = {
+	.enable = vbattb_clk_enable,
+	.disable = vbattb_clk_disable,
+	.is_enabled = vbattb_clk_is_enabled,
+};
+
+static int vbattb_clk_validate_load_capacitance(struct vbattb_clk *vbclk, u32 load_capacitance)
+{
+	switch (load_capacitance) {
+	case 4000:
+		vbclk->load_capacitance = VBATTB_XOSCCR_XSEL_4_PF;
+		break;
+	case 7000:
+		vbclk->load_capacitance = VBATTB_XOSCCR_XSEL_7_PF;
+		break;
+	case 9000:
+		vbclk->load_capacitance = VBATTB_XOSCCR_XSEL_9_PF;
+		break;
+	case 12500:
+		vbclk->load_capacitance = VBATTB_XOSCCR_XSEL_12_5_PF;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int vbattb_clk_need_bypass(struct device *dev)
+{
+	struct clk *clkin, *xin;
+
+	clkin = devm_clk_get_optional(dev, "clkin");
+	xin = devm_clk_get_optional(dev, "xin");
+
+	if (!IS_ERR_OR_NULL(clkin) && !IS_ERR_OR_NULL(xin))
+		return -EINVAL;
+	else if (!clkin && !IS_ERR_OR_NULL(xin))
+		return 0;
+	else if (!IS_ERR_OR_NULL(clkin) && !xin)
+		return 1;
+
+	return -EINVAL;
+}
+
+static int vbattb_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct clk_parent_data parent_data = {};
+	struct device *dev = &pdev->dev;
+	struct clk_init_data init = {};
+	struct vbattb_clk *vbclk;
+	u32 load_capacitance;
+	struct clk_hw *hw;
+	int ret, bypass;
+
+	vbclk = devm_kzalloc(dev, sizeof(*vbclk), GFP_KERNEL);
+	if (!vbclk)
+		return -ENOMEM;
+
+	vbclk->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(vbclk->base))
+		return PTR_ERR(vbclk->base);
+
+	bypass = vbattb_clk_need_bypass(dev);
+	if (bypass < 0) {
+		return bypass;
+	} else if (bypass) {
+		parent_data.fw_name = "clkin";
+		bypass = VBATTB_BKSCCR_SOSEL;
+	} else {
+		parent_data.fw_name = "xin";
+	}
+
+	ret = of_property_read_u32(np, "renesas,vbattb-load-nanofarads", &load_capacitance);
+	if (ret)
+		return ret;
+
+	ret = vbattb_clk_validate_load_capacitance(vbclk, load_capacitance);
+	if (ret)
+		return ret;
+
+	vbattb_clk_update_bits(vbclk->base, VBATTB_BKSCCR, VBATTB_BKSCCR_SOSEL, bypass);
+
+	spin_lock_init(&vbclk->lock);
+
+	init.name = "vbattclk";
+	init.ops = &vbattb_clk_ops;
+	init.parent_data = &parent_data;
+	init.num_parents = 1;
+	init.flags = 0;
+
+	vbclk->hw.init = &init;
+	hw = &vbclk->hw;
+
+	ret = devm_clk_hw_register(dev, hw);
+	if (ret)
+		return ret;
+
+	return of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw);
+}
+
+static const struct of_device_id vbattb_clk_match[] = {
+	{ .compatible = "renesas,r9a08g045-vbattb-clk" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver vbattb_clk_driver = {
+	.driver		= {
+		.name	= "renesas-vbattb-clk",
+		.of_match_table = vbattb_clk_match,
+	},
+	.probe = vbattb_clk_probe,
+};
+module_platform_driver(vbattb_clk_driver);
+
+MODULE_DESCRIPTION("Renesas VBATTB Clock Driver");
+MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[Stephen Boyd]

Quoting Claudiu (2024-07-16 03:30:17)
> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c
> new file mode 100644
> index 000000000000..8effe141fc0b
> --- /dev/null
> +++ b/drivers/clk/renesas/clk-vbattb.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VBATTB clock driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>

Prefer clk providers to not be clk consumers.

> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>

Is of_platform.h used?

Include mod_devicetable.h for of_device_id.

> +#include <linux/platform_device.h>
> +
> +#define VBATTB_BKSCCR                  0x0
> +#define VBATTB_BKSCCR_SOSEL            BIT(6)
> +#define VBATTB_SOSCCR2                 0x8
> +#define VBATTB_SOSCCR2_SOSTP2          BIT(0)
[..]
> +
> +static int vbattb_clk_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct clk_parent_data parent_data = {};
> +       struct device *dev = &pdev->dev;
> +       struct clk_init_data init = {};
> +       struct vbattb_clk *vbclk;
> +       u32 load_capacitance;
> +       struct clk_hw *hw;
> +       int ret, bypass;
> +
> +       vbclk = devm_kzalloc(dev, sizeof(*vbclk), GFP_KERNEL);
> +       if (!vbclk)
> +               return -ENOMEM;
> +
> +       vbclk->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(vbclk->base))
> +               return PTR_ERR(vbclk->base);
> +
> +       bypass = vbattb_clk_need_bypass(dev);

This is a tri-state bool :(

> +       if (bypass < 0) {
> +               return bypass;
> +       } else if (bypass) {
> +               parent_data.fw_name = "clkin";
> +               bypass = VBATTB_BKSCCR_SOSEL;

And now it is a mask value.

> +       } else {
> +               parent_data.fw_name = "xin";
> +       }
> +
> +       ret = of_property_read_u32(np, "renesas,vbattb-load-nanofarads", &load_capacitance);
> +       if (ret)
> +               return ret;
> +
> +       ret = vbattb_clk_validate_load_capacitance(vbclk, load_capacitance);
> +       if (ret)
> +               return ret;
> +
> +       vbattb_clk_update_bits(vbclk->base, VBATTB_BKSCCR, VBATTB_BKSCCR_SOSEL, bypass);

Please don't overload 'bypass'. Use two variables or a conditional.

I also wonder if this is really a mux, and either assigned-clock-parents
should be used, or the clk_ops should have an init routine that looks at
which parent is present by determining the index and then use that to
set the mux. The framework can take care of failing to set the other
parent when it isn't present.

> +
> +       spin_lock_init(&vbclk->lock);
> +
> +       init.name = "vbattclk";
> +       init.ops = &vbattb_clk_ops;
> +       init.parent_data = &parent_data;
> +       init.num_parents = 1;
> +       init.flags = 0;
> +
> +       vbclk->hw.init = &init;
> +       hw = &vbclk->hw;
> +
> +       ret = devm_clk_hw_register(dev, hw);
> +       if (ret)
> +               return ret;
> +
> +       return of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw);
> +}
> +
> +static const struct of_device_id vbattb_clk_match[] = {
> +       { .compatible = "renesas,r9a08g045-vbattb-clk" },
> +       { /* sentinel */ }
> +};

Any MODULE_DEVICE_TABLE?

Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[claudiu beznea]

Hi, Stephen,

On 17.07.2024 01:28, Stephen Boyd wrote:
> Quoting Claudiu (2024-07-16 03:30:17)
>> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c
>> new file mode 100644
>> index 000000000000..8effe141fc0b
>> --- /dev/null
>> +++ b/drivers/clk/renesas/clk-vbattb.c
>> @@ -0,0 +1,212 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VBATTB clock driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/cleanup.h>
>> +#include <linux/clk.h>
> 
> Prefer clk providers to not be clk consumers.

I added it here to be able to use devm_clk_get_optional() as it was
proposed to me in v1 to avoid adding a new binding for bypass and detect if
it's needed by checking the input clock name.


> 
>> +#include <linux/clk-provider.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
> 
> Is of_platform.h used?
> 
> Include mod_devicetable.h for of_device_id.

Ok.

> 
>> +#include <linux/platform_device.h>
>> +
>> +#define VBATTB_BKSCCR                  0x0
>> +#define VBATTB_BKSCCR_SOSEL            BIT(6)
>> +#define VBATTB_SOSCCR2                 0x8
>> +#define VBATTB_SOSCCR2_SOSTP2          BIT(0)
> [..]
>> +
>> +static int vbattb_clk_probe(struct platform_device *pdev)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       struct clk_parent_data parent_data = {};
>> +       struct device *dev = &pdev->dev;
>> +       struct clk_init_data init = {};
>> +       struct vbattb_clk *vbclk;
>> +       u32 load_capacitance;
>> +       struct clk_hw *hw;
>> +       int ret, bypass;
>> +
>> +       vbclk = devm_kzalloc(dev, sizeof(*vbclk), GFP_KERNEL);
>> +       if (!vbclk)
>> +               return -ENOMEM;
>> +
>> +       vbclk->base = devm_platform_ioremap_resource(pdev, 0);
>> +       if (IS_ERR(vbclk->base))
>> +               return PTR_ERR(vbclk->base);
>> +
>> +       bypass = vbattb_clk_need_bypass(dev);
> 
> This is a tri-state bool :(
> 
>> +       if (bypass < 0) {
>> +               return bypass;
>> +       } else if (bypass) {
>> +               parent_data.fw_name = "clkin";
>> +               bypass = VBATTB_BKSCCR_SOSEL;
> 
> And now it is a mask value.
> 
>> +       } else {
>> +               parent_data.fw_name = "xin";
>> +       }
>> +
>> +       ret = of_property_read_u32(np, "renesas,vbattb-load-nanofarads", &load_capacitance);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = vbattb_clk_validate_load_capacitance(vbclk, load_capacitance);
>> +       if (ret)
>> +               return ret;
>> +
>> +       vbattb_clk_update_bits(vbclk->base, VBATTB_BKSCCR, VBATTB_BKSCCR_SOSEL, bypass);
> 
> Please don't overload 'bypass'. Use two variables or a conditional.

OK.

> 
> I also wonder if this is really a mux, 

It's a way to determine what type of clock (crystal oscillator or device
clock) is connected to RTXIN/RTXOUT pins of the module
(the module block diagram at [1]) based on the clock name. Depending on the
type of the clock connected to RTXIN/RTXOUT we need to select the XC or
XBYP as input for the mux at [1].

[1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png


> and either assigned-clock-parents should be used, 
> or the clk_ops should have an init routine that looks at
> which parent is present by determining the index and then use that to
> set the mux. The framework can take care of failing to set the other
> parent when it isn't present.


On the board, at any moment, it will be only one clock as input to the
VBATTB clock (either crystal oscillator or a clock device). If I'm getting
you correctly, this will involve describing both clocks in some scenarios.

E.g. if want to use crystal osc, I can use this DT description:

vbattclk: clock-controller@1c {
	compatible = "renesas,r9a08g045-vbattb-clk";
	reg = <0 0x1c 0 0x10>;
	clocks = <&vbattb_xtal>;
	clock-names = "xin";
	#clock-cells = <0>;
	status = "disabled";
};

vbattb_xtal: vbattb-xtal {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <32768>;
};

If external clock device is to be used, I should describe a fake clock too:

vbattclk: clock-controller@1c {
	compatible = "renesas,r9a08g045-vbattb-clk";
	reg = <0 0x1c 0 0x10>;
	clocks = <&vbattb_xtal>, <&ext_clk>;
	clock-names = "xin", "clkin";
	#clock-cells = <0>;
	status = "disabled";
};

vbattb_xtal: vbattb-xtal {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <0>;
};

ext_clk: ext-clk {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <32768>;
};

Is this what you are suggesting?


> 
>> +
>> +       spin_lock_init(&vbclk->lock);
>> +
>> +       init.name = "vbattclk";
>> +       init.ops = &vbattb_clk_ops;
>> +       init.parent_data = &parent_data;
>> +       init.num_parents = 1;
>> +       init.flags = 0;
>> +
>> +       vbclk->hw.init = &init;
>> +       hw = &vbclk->hw;
>> +
>> +       ret = devm_clk_hw_register(dev, hw);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw);
>> +}
>> +
>> +static const struct of_device_id vbattb_clk_match[] = {
>> +       { .compatible = "renesas,r9a08g045-vbattb-clk" },
>> +       { /* sentinel */ }
>> +};
> 
> Any MODULE_DEVICE_TABLE?

I failed to added it.

Thank you for your review,
Claudiu Beznea

Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[Stephen Boyd]

Quoting claudiu beznea (2024-07-17 01:31:20)
> Hi, Stephen,
> 
> On 17.07.2024 01:28, Stephen Boyd wrote:
> > Quoting Claudiu (2024-07-16 03:30:17)
> >> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c
> >> new file mode 100644
> >> index 000000000000..8effe141fc0b
> >> --- /dev/null
> >> +++ b/drivers/clk/renesas/clk-vbattb.c
> >> @@ -0,0 +1,212 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * VBATTB clock driver
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#include <linux/cleanup.h>
> >> +#include <linux/clk.h>
> > 
> > Prefer clk providers to not be clk consumers.
> 
> I added it here to be able to use devm_clk_get_optional() as it was
> proposed to me in v1 to avoid adding a new binding for bypass and detect if
> it's needed by checking the input clock name.
> 

Understood.

> 
> > 
> > I also wonder if this is really a mux, 
> 
> It's a way to determine what type of clock (crystal oscillator or device
> clock) is connected to RTXIN/RTXOUT pins of the module
> (the module block diagram at [1]) based on the clock name. Depending on the
> type of the clock connected to RTXIN/RTXOUT we need to select the XC or
> XBYP as input for the mux at [1].
> 
> [1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png

That diagram shows a mux block, so at least something in there is a mux.
From what I can tell the binding uses clock-names to describe the mux.
What I'd like to avoid is using clk_get() to determine how to configure
the mux. That's because clk_get() is a clk consumer API, and because we
want clk providers to be able to register clks without making sure that
the entire parent chain has been registered first. Eventually, we'd like
clk_get() to probe defer if the clk is an orphan. Having clk providers
use clk_get() breaks that pretty quickly.

> 
> 
> > and either assigned-clock-parents should be used, 
> > or the clk_ops should have an init routine that looks at
> > which parent is present by determining the index and then use that to
> > set the mux. The framework can take care of failing to set the other
> > parent when it isn't present.
> 
> 
> On the board, at any moment, it will be only one clock as input to the
> VBATTB clock (either crystal oscillator or a clock device). If I'm getting
> you correctly, this will involve describing both clocks in some scenarios.
> 
> E.g. if want to use crystal osc, I can use this DT description:
> 
> vbattclk: clock-controller@1c {
>         compatible = "renesas,r9a08g045-vbattb-clk";
>         reg = <0 0x1c 0 0x10>;
>         clocks = <&vbattb_xtal>;
>         clock-names = "xin";
>         #clock-cells = <0>;
>         status = "disabled";
> };
> 
> vbattb_xtal: vbattb-xtal {
>         compatible = "fixed-clock";
>         #clock-cells = <0>;
>         clock-frequency = <32768>;
> };
> 
> If external clock device is to be used, I should describe a fake clock too:
> 
> vbattclk: clock-controller@1c {
>         compatible = "renesas,r9a08g045-vbattb-clk";
>         reg = <0 0x1c 0 0x10>;
>         clocks = <&vbattb_xtal>, <&ext_clk>;

Is vbattb_xtal the fake clk? If so, I'd expect this to be

	clocks = <0>, <&ext_clk>;

so that we don't have a useless clk node.

>         clock-names = "xin", "clkin";
>         #clock-cells = <0>;
>         status = "disabled";
> };
> 
> vbattb_xtal: vbattb-xtal {
>         compatible = "fixed-clock";
>         #clock-cells = <0>;
>         clock-frequency = <0>;
> };
> 
> ext_clk: ext-clk {
>         compatible = "fixed-clock";
>         #clock-cells = <0>;
>         clock-frequency = <32768>;
> };
> 
> Is this what you are suggesting?
> 

Sort of. Ignoring the problem with the subnode for the clk driver, I
don't really like having clock-names that don't match the hardware pin
names. From the diagram you provided, it looks like clock-names should
be "bclk" and "rtxin" for the bus clock and the rtxin signal. Then the
clock-cells should be "1" instead of "0", and the mux should be one of
those provided clks and "xc" and "xbyp" should be the other two. If that
was done, then assigned-clocks could be used to assign the parent of the
mux.

#define VBATTBCLK          0
#define VBATTB_XBYP        1
#define VBATTB_XC          2

    vbattb: vbattb@1005c000 {
        compatible = "renesas,r9a08g045-vbattb";
        reg = <0x1005c000 0x1000>;
        ranges = <0 0 0x1005c000 0 0x1000>;
        interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
        interrupt-names = "tampdi";
        clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>;
        clock-names = "bclk", "rtxin";
        power-domains = <&cpg>;
        resets = <&cpg R9A08G045_VBAT_BRESETN>;
        #clock-cells = <1>;
        assigned-clocks = <&vbattb VBATTBCLK>;
	assigned-clock-parents = <&vbattb VBATTB_XBYP>;
        renesas,vbattb-load-nanofarads = <12500>;
    };

One last thing that I don't really understand is why this needs to be a
clk provider. In the diagram, the RTC is also part of vbattb, so it
looks odd to have this node be a clk provider with #clock-cells at all.
Is it the case that if the rtxin pin is connected, you mux that over,
and if the pin is disconnected you mux over the internal oscillator? I'm
really wondering why a clk provider is implemented at all. Why not just
hit the registers directly from the RTC driver depending on a
devm_clk_get_optional() call?

Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[claudiu beznea]

On 18.07.2024 03:39, Stephen Boyd wrote:
> Quoting claudiu beznea (2024-07-17 01:31:20)
>> Hi, Stephen,
>>
>> On 17.07.2024 01:28, Stephen Boyd wrote:
>>> Quoting Claudiu (2024-07-16 03:30:17)
>>>> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c
>>>> new file mode 100644
>>>> index 000000000000..8effe141fc0b
>>>> --- /dev/null
>>>> +++ b/drivers/clk/renesas/clk-vbattb.c
>>>> @@ -0,0 +1,212 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * VBATTB clock driver
>>>> + *
>>>> + * Copyright (C) 2024 Renesas Electronics Corp.
>>>> + */
>>>> +
>>>> +#include <linux/cleanup.h>
>>>> +#include <linux/clk.h>
>>>
>>> Prefer clk providers to not be clk consumers.
>>
>> I added it here to be able to use devm_clk_get_optional() as it was
>> proposed to me in v1 to avoid adding a new binding for bypass and detect if
>> it's needed by checking the input clock name.
>>
> 
> Understood.
> 
>>
>>>
>>> I also wonder if this is really a mux, 
>>
>> It's a way to determine what type of clock (crystal oscillator or device
>> clock) is connected to RTXIN/RTXOUT pins of the module
>> (the module block diagram at [1]) based on the clock name. Depending on the
>> type of the clock connected to RTXIN/RTXOUT we need to select the XC or
>> XBYP as input for the mux at [1].
>>
>> [1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png
> 
> That diagram shows a mux block, so at least something in there is a mux.
> From what I can tell the binding uses clock-names to describe the mux.
> What I'd like to avoid is using clk_get() to determine how to configure
> the mux. That's because clk_get() is a clk consumer API, and because we
> want clk providers to be able to register clks without making sure that
> the entire parent chain has been registered first. Eventually, we'd like
> clk_get() to probe defer if the clk is an orphan. Having clk providers
> use clk_get() breaks that pretty quickly.
> 
>>
>>
>>> and either assigned-clock-parents should be used, 
>>> or the clk_ops should have an init routine that looks at
>>> which parent is present by determining the index and then use that to
>>> set the mux. The framework can take care of failing to set the other
>>> parent when it isn't present.
>>
>>
>> On the board, at any moment, it will be only one clock as input to the
>> VBATTB clock (either crystal oscillator or a clock device). If I'm getting
>> you correctly, this will involve describing both clocks in some scenarios.
>>
>> E.g. if want to use crystal osc, I can use this DT description:
>>
>> vbattclk: clock-controller@1c {
>>         compatible = "renesas,r9a08g045-vbattb-clk";
>>         reg = <0 0x1c 0 0x10>;
>>         clocks = <&vbattb_xtal>;
>>         clock-names = "xin";
>>         #clock-cells = <0>;
>>         status = "disabled";
>> };
>>
>> vbattb_xtal: vbattb-xtal {
>>         compatible = "fixed-clock";
>>         #clock-cells = <0>;
>>         clock-frequency = <32768>;
>> };
>>
>> If external clock device is to be used, I should describe a fake clock too:
>>
>> vbattclk: clock-controller@1c {
>>         compatible = "renesas,r9a08g045-vbattb-clk";
>>         reg = <0 0x1c 0 0x10>;
>>         clocks = <&vbattb_xtal>, <&ext_clk>;
> 
> Is vbattb_xtal the fake clk? If so, I'd expect this to be
> 
> 	clocks = <0>, <&ext_clk>;
> 
> so that we don't have a useless clk node.
> 
>>         clock-names = "xin", "clkin";
>>         #clock-cells = <0>;
>>         status = "disabled";
>> };
>>
>> vbattb_xtal: vbattb-xtal {
>>         compatible = "fixed-clock";
>>         #clock-cells = <0>;
>>         clock-frequency = <0>;
>> };
>>
>> ext_clk: ext-clk {
>>         compatible = "fixed-clock";
>>         #clock-cells = <0>;
>>         clock-frequency = <32768>;
>> };
>>
>> Is this what you are suggesting?
>>
> 
> Sort of. Ignoring the problem with the subnode for the clk driver, I
> don't really like having clock-names that don't match the hardware pin
> names. From the diagram you provided, it looks like clock-names should
> be "bclk" and "rtxin" for the bus clock and the rtxin signal. Then the
> clock-cells should be "1" instead of "0", and the mux should be one of
> those provided clks and "xc" and "xbyp" should be the other two. If that
> was done, then assigned-clocks could be used to assign the parent of the
> mux.
> 
> #define VBATTBCLK          0
> #define VBATTB_XBYP        1
> #define VBATTB_XC          2
> 
>     vbattb: vbattb@1005c000 {
>         compatible = "renesas,r9a08g045-vbattb";
>         reg = <0x1005c000 0x1000>;
>         ranges = <0 0 0x1005c000 0 0x1000>;
>         interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
>         interrupt-names = "tampdi";
>         clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>;
>         clock-names = "bclk", "rtxin";
>         power-domains = <&cpg>;
>         resets = <&cpg R9A08G045_VBAT_BRESETN>;
>         #clock-cells = <1>;
>         assigned-clocks = <&vbattb VBATTBCLK>;
> 	assigned-clock-parents = <&vbattb VBATTB_XBYP>;
>         renesas,vbattb-load-nanofarads = <12500>;
>     };

I think I got it now. Thank you for the detailed explanation.

> 
> One last thing that I don't really understand is why this needs to be a
> clk provider. In the diagram, the RTC is also part of vbattb, so it
> looks odd to have this node be a clk provider with #clock-cells at all.

I did it like this because the RTC is a different IP mapped at it's own
address and considering the other VBATTB functionalities (tamper, SRAM)
might be implemented at some point.

I also failed to notice that RTC might not work w/o bclk being enabled
(thanks for pointing it).

I saw that diagram more like describing the always-on power domain
(PD_VBATTB) where the VBATTB logic and RTC resides. That power domain is
backed by battery. From HW manual [1]: "PD_VBATT domain is the area where
the RTC/backup register is located, works on battery power when the power
of PD_VCC and PD_ISOVCC domain are turned off."

[1]
https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz-mpus/rzg3s-general-purpose-microprocessors-single-core-arm-cortex-a55-11-ghz-cpu-and-dual-core-cortex-m33-250

> Is it the case that if the rtxin pin is connected, you mux that over,
> and if the pin is disconnected you mux over the internal oscillator? 

From the description here at [2] I'm getting that the "32-KHz clock
oscillator" block is used when crystal oscillator is connected to RTXIN,
RTXOUT pins and it is skipped if external clock device is connected.

[2] https://i2.paste.pics/RFKJ0.png?rand=Xq8w1RLDvZ

> I'm
> really wondering why a clk provider is implemented at all. Why not just
> hit the registers directly from the RTC driver depending on a
> devm_clk_get_optional() call?

I did it like this because the RTC is a different IP mapped at it's own
address with it's own interrupts, clock, power domain and considering that
the other VBATTB functionalities (tamper, SRAM) might be used at some point
in future. At the same time I failed to noticed the VBATTB clock might be
needed for RTC.

Do you consider better to just take a regmap to VBATTB from RTC driver and
set the VBATTB from RTC driver itself?

Thank you,
Claudiu Beznea

Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[Stephen Boyd]

Quoting claudiu beznea (2024-07-18 07:41:03)
> 
> 
> On 18.07.2024 03:39, Stephen Boyd wrote:
> > 
> > Sort of. Ignoring the problem with the subnode for the clk driver, I
> > don't really like having clock-names that don't match the hardware pin
> > names. From the diagram you provided, it looks like clock-names should
> > be "bclk" and "rtxin" for the bus clock and the rtxin signal. Then the
> > clock-cells should be "1" instead of "0", and the mux should be one of
> > those provided clks and "xc" and "xbyp" should be the other two. If that
> > was done, then assigned-clocks could be used to assign the parent of the
> > mux.
> > 
> > #define VBATTBCLK          0
> > #define VBATTB_XBYP        1
> > #define VBATTB_XC          2
> > 
> >     vbattb: vbattb@1005c000 {
> >         compatible = "renesas,r9a08g045-vbattb";
> >         reg = <0x1005c000 0x1000>;
> >         ranges = <0 0 0x1005c000 0 0x1000>;
> >         interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> >         interrupt-names = "tampdi";
> >         clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>;
> >         clock-names = "bclk", "rtxin";
> >         power-domains = <&cpg>;
> >         resets = <&cpg R9A08G045_VBAT_BRESETN>;
> >         #clock-cells = <1>;
> >         assigned-clocks = <&vbattb VBATTBCLK>;
> >       assigned-clock-parents = <&vbattb VBATTB_XBYP>;
> >         renesas,vbattb-load-nanofarads = <12500>;
> >     };
> 
> I think I got it now. Thank you for the detailed explanation.
> 
> > 
> > One last thing that I don't really understand is why this needs to be a
> > clk provider. In the diagram, the RTC is also part of vbattb, so it
> > looks odd to have this node be a clk provider with #clock-cells at all.
> 
> I did it like this because the RTC is a different IP mapped at it's own
> address and considering the other VBATTB functionalities (tamper, SRAM)
> might be implemented at some point.
> 
> I also failed to notice that RTC might not work w/o bclk being enabled
> (thanks for pointing it).

> 
> I saw that diagram more like describing the always-on power domain
> (PD_VBATTB) where the VBATTB logic and RTC resides. That power domain is
> backed by battery. From HW manual [1]: "PD_VBATT domain is the area where
> the RTC/backup register is located, works on battery power when the power
> of PD_VCC and PD_ISOVCC domain are turned off."

Ah ok, so PD_VBATTB is like a power domain/wrapper and not an IP block
mapped on the bus at the same address as the RTC that it wraps. That
changes things a bit.

> 
> [1]
> https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz-mpus/rzg3s-general-purpose-microprocessors-single-core-arm-cortex-a55-11-ghz-cpu-and-dual-core-cortex-m33-250
> 
> > Is it the case that if the rtxin pin is connected, you mux that over,
> > and if the pin is disconnected you mux over the internal oscillator? 
> 
> From the description here at [2] I'm getting that the "32-KHz clock
> oscillator" block is used when crystal oscillator is connected to RTXIN,
> RTXOUT pins and it is skipped if external clock device is connected.
> 
> [2] https://i2.paste.pics/RFKJ0.png?rand=Xq8w1RLDvZ

It looks like in both cases something is connected to the pins. XC is to
use an external crystal connected to both pins and XBYP is to use a
single clk. Given that, perhaps naming the clk "rtx" is simplest and
then using assigned-clock-parents is most direct because there's only
really one "logical" clk input for this device. That means the XC and
XBYP clks have the same parent, "rtx", and the XC clk is a gateable
fixed rate clk while the XBYP clk is a fixed factor clk that does
nothing besides pass on the rate of the "rtx" clk.

> 
> > I'm
> > really wondering why a clk provider is implemented at all. Why not just
> > hit the registers directly from the RTC driver depending on a
> > devm_clk_get_optional() call?
> 
> I did it like this because the RTC is a different IP mapped at it's own
> address with it's own interrupts, clock, power domain and considering that
> the other VBATTB functionalities (tamper, SRAM) might be used at some point
> in future. At the same time I failed to noticed the VBATTB clock might be
> needed for RTC.

The docs say VBATT in some places. Not sure if you want to rename it to
vbatt and drop the extra b which probably stands for "backup"?

> 
> Do you consider better to just take a regmap to VBATTB from RTC driver and
> set the VBATTB from RTC driver itself?

No, don't do that. The only change from the above DT node is that the
assigned-clocks and assigned-clock-parents property should be moved to
the RTC node.


     vbattb: vbattb@1005c000 {
         compatible = "renesas,r9a08g045-vbattb";
         reg = <0x1005c000 0x1000>;
         ranges = <0 0 0x1005c000 0 0x1000>;
         interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
         interrupt-names = "tampdi";
         clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>;
         clock-names = "bclk", "rtx";
         power-domains = <&cpg>;
         resets = <&cpg R9A08G045_VBAT_BRESETN>;
         #clock-cells = <1>;
         renesas,vbattb-load-nanofarads = <12500>;
     };

     rtc@1004ec00 {
         compatible = "renesas,r9a08g045-rtc";
         reg = <0x1004ec00 0x400>;
         clocks = <&vbattb VBATTBCLK>;
         assigned-clocks = <&vbattb VBATTBCLK>;
         assigned-clock-parents = <&vbattb VBATTB_XBYP>; // Or VBATTB_XC if external crystal connected
     };

[PATCH v2 04/11] dt-bindings: rtc: renesas,rzg3s-rtc: Document the Renesas RTCA-3 IP

[Claudiu]

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

Document the RTC IP (RTCA-3) available on the Renesas RZ/G3S SoC.
The RTC IP available on Renesas RZ/V2H is almost identical with the
one found on Renesas RZ/G3S (it misses the time capture functionality
which is not yet implemented on proposed driver). For this, added also a
generic compatible that will be used at the moment as fallback for both
RZ/G3S and RZ/V2H.

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

Changes in v2:
- updated patch description and title
- included reference to rtc.yaml
- updated compatible list with a generic compatible as explained in
  patch description; with this the node in examples section has also been
  updated
- used items to describe interrupts, interrupt-names, clock, clock-names
- updated title section

 .../bindings/rtc/renesas,rz-rtca3.yaml        | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/renesas,rz-rtca3.yaml

diff --git a/Documentation/devicetree/bindings/rtc/renesas,rz-rtca3.yaml b/Documentation/devicetree/bindings/rtc/renesas,rz-rtca3.yaml
new file mode 100644
index 000000000000..21f104b1e86b
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/renesas,rz-rtca3.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/renesas,rz-rtca3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RTCA-3 Real Time Clock
+
+maintainers:
+  - Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a08g045-rtca3 # RZ/G3S
+      - const: renesas,rz-rtca3
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: Alarm interrupt
+      - description: Periodic interrupt
+      - description: Carry interrupt
+
+  interrupt-names:
+    items:
+      - const: alarm
+      - const: period
+      - const: carry
+
+  clocks:
+    items:
+      - description: RTC counter clock
+
+  clock-names:
+    items:
+      - const: counter
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    rtc@1004ec00 {
+        compatible = "renesas,r9a08g045-rtca3", "renesas,rz-rtca3";
+        reg = <0x1004ec00 0x400>;
+        interrupts = <GIC_SPI 315 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "alarm", "period", "carry";
+        clocks = <&vbattclk>;
+        clock-names = "counter";
+    };
-- 
2.39.2

Re: [PATCH v2 04/11] dt-bindings: rtc: renesas,rzg3s-rtc: Document the Renesas RTCA-3 IP

[Conor Dooley]

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

On Tue, Jul 16, 2024 at 01:30:18PM +0300, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Document the RTC IP (RTCA-3) available on the Renesas RZ/G3S SoC.
> The RTC IP available on Renesas RZ/V2H is almost identical with the
> one found on Renesas RZ/G3S (it misses the time capture functionality
> which is not yet implemented on proposed driver). For this, added also a
> generic compatible that will be used at the moment as fallback for both
> RZ/G3S and RZ/V2H.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

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

[PATCH v2 05/11] rtc: renesas-rtca3: Add driver for RTCA-3 available on Renesas RZ/G3S SoC

[Claudiu]

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

The RTC IP (RTCA-3) available on the Renesas RZ/G3S SoC has calendar count
mode and binary count mode (selectable though RCR2.CNTMD) capabilities,
alarm capabilities, clock error correction capabilities. It can generate
alarm, period, carry interrupts.

Add a driver for RTCA-3 IP. The driver implements calendar count mode (as
the conversion b/w RTC and system time is simpler, done with bcd2bin(),
bin2bcd()), read and set time, read and set alarm, read and set
an offset.

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

Changes in v2:
- used cleanup.h helpers for locking
- updated the MAINTAINERS entry with the new name for RTCA-3 documentation
  file and a new title (from "RENESAS RZ/G3S RTC DRIVER" to
  "RENESAS RTCA-3 RTC DRIVER")
- used 24 hours mode
- changed startup sequence (rtca3_initial_setup()) to avoid stopping
  the RTC if it's already configured
- updated the RTC range to 2000-2099
- updated the compatible with the generic one (renesas,rz-rtca3) in the
  idea the driver will be also used by the RZ/V2H w/o the necessity to
  add a new compatible

 MAINTAINERS                     |   8 +
 drivers/rtc/Kconfig             |  10 +
 drivers/rtc/Makefile            |   1 +
 drivers/rtc/rtc-renesas-rtca3.c | 853 ++++++++++++++++++++++++++++++++
 4 files changed, 872 insertions(+)
 create mode 100644 drivers/rtc/rtc-renesas-rtca3.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3c9cc609cdfd..067f080a4d30 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19419,6 +19419,14 @@ S:	Supported
 F:	Documentation/devicetree/bindings/timer/renesas,rz-mtu3.yaml
 F:	drivers/counter/rz-mtu3-cnt.c
 
+RENESAS RTCA-3 RTC DRIVER
+M:	Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
+L:	linux-rtc@vger.kernel.org
+L:	linux-renesas-soc@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/rtc/renesas,rz-rtca3.yaml
+F:	drivers/rtc/rtc-renesas-rtca3.c
+
 RENESAS RZ/N1 A5PSW SWITCH DRIVER
 M:	Clément Léger <clement.leger@bootlin.com>
 L:	linux-renesas-soc@vger.kernel.org
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 2a95b05982ad..3b29b35e48e0 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1978,6 +1978,16 @@ config RTC_DRV_MA35D1
 	   This driver can also be built as a module, if so, the module
 	   will be called "rtc-ma35d1".
 
+config RTC_DRV_RENESAS_RTCA3
+	tristate "Renesas RTCA-3 RTC"
+	depends on ARCH_RENESAS
+	help
+	  If you say yes here you get support for the Renesas RTCA-3 RTC
+	  available on the Renesas RZ/G3S SoC.
+
+	  This driver can also be built as a module, if so, the module
+	  will be called "rtc-rtca3".
+
 comment "HID Sensor RTC drivers"
 
 config RTC_DRV_HID_SENSOR_TIME
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 3004e372f25f..52844f13b247 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -157,6 +157,7 @@ obj-$(CONFIG_RTC_DRV_RX8025)	+= rtc-rx8025.o
 obj-$(CONFIG_RTC_DRV_RX8111)	+= rtc-rx8111.o
 obj-$(CONFIG_RTC_DRV_RX8581)	+= rtc-rx8581.o
 obj-$(CONFIG_RTC_DRV_RZN1)	+= rtc-rzn1.o
+obj-$(CONFIG_RTC_DRV_RENESAS_RTCA3)	+= rtc-renesas-rtca3.o
 obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
 obj-$(CONFIG_RTC_DRV_S3C)	+= rtc-s3c.o
 obj-$(CONFIG_RTC_DRV_S5M)	+= rtc-s5m.o
diff --git a/drivers/rtc/rtc-renesas-rtca3.c b/drivers/rtc/rtc-renesas-rtca3.c
new file mode 100644
index 000000000000..c25971ff847e
--- /dev/null
+++ b/drivers/rtc/rtc-renesas-rtca3.c
@@ -0,0 +1,853 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * On-Chip RTC Support available on RZ/G3S SoC
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+#include <linux/bcd.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/iopoll.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+/* Counter registers. */
+#define RTCA3_RSECCNT			0x2
+#define RTCA3_RSECCNT_SEC		GENMASK(6, 0)
+#define RTCA3_RMINCNT			0x4
+#define RTCA3_RMINCNT_MIN		GENMASK(6, 0)
+#define RTCA3_RHRCNT			0x6
+#define RTCA3_RHRCNT_HR			GENMASK(5, 0)
+#define RTCA3_RHRCNT_PM			BIT(6)
+#define RTCA3_RWKCNT			0x8
+#define RTCA3_RWKCNT_WK			GENMASK(2, 0)
+#define RTCA3_RDAYCNT			0xa
+#define RTCA3_RDAYCNT_DAY		GENMASK(5, 0)
+#define RTCA3_RMONCNT			0xc
+#define RTCA3_RMONCNT_MONTH		GENMASK(4, 0)
+#define RTCA3_RYRCNT			0xe
+#define RTCA3_RYRCNT_YEAR		GENMASK(7, 0)
+
+/* Alarm registers. */
+#define RTCA3_RSECAR			0x10
+#define RTCA3_RSECAR_SEC		GENMASK(6, 0)
+#define RTCA3_RMINAR			0x12
+#define RTCA3_RMINAR_MIN		GENMASK(6, 0)
+#define RTCA3_RHRAR			0x14
+#define RTCA3_RHRAR_HR			GENMASK(5, 0)
+#define RTCA3_RHRAR_PM			BIT(6)
+#define RTCA3_RWKAR			0x16
+#define RTCA3_RWKAR_DAYW		GENMASK(2, 0)
+#define RTCA3_RDAYAR			0x18
+#define RTCA3_RDAYAR_DATE		GENMASK(5, 0)
+#define RTCA3_RMONAR			0x1a
+#define RTCA3_RMONAR_MON		GENMASK(4, 0)
+#define RTCA3_RYRAR			0x1c
+#define RTCA3_RYRAR_YR			GENMASK(7, 0)
+#define RTCA3_RYRAREN			0x1e
+
+/* Alarm enable bit (for all alarm registers). */
+#define RTCA3_AR_ENB			BIT(7)
+
+/* Control registers. */
+#define RTCA3_RCR1			0x22
+#define RTCA3_RCR1_AIE			BIT(0)
+#define RTCA3_RCR1_CIE			BIT(1)
+#define RTCA3_RCR1_PIE			BIT(2)
+#define RTCA3_RCR1_PES			GENMASK(7, 4)
+#define RTCA3_RCR1_PES_1_64_SEC		0x8
+#define RTCA3_RCR2			0x24
+#define RTCA3_RCR2_START		BIT(0)
+#define RTCA3_RCR2_RESET		BIT(1)
+#define RTCA3_RCR2_AADJE		BIT(4)
+#define RTCA3_RCR2_ADJP			BIT(5)
+#define RTCA3_RCR2_HR24			BIT(6)
+#define RTCA3_RCR2_CNTMD		BIT(7)
+#define RTCA3_RSR			0x20
+#define RTCA3_RSR_AF			BIT(0)
+#define RTCA3_RSR_CF			BIT(1)
+#define RTCA3_RSR_PF			BIT(2)
+#define RTCA3_RADJ			0x2e
+#define RTCA3_RADJ_ADJ			GENMASK(5, 0)
+#define RTCA3_RADJ_ADJ_MAX		0x3f
+#define RTCA3_RADJ_PMADJ		GENMASK(7, 6)
+#define RTCA3_RADJ_PMADJ_NONE		0
+#define RTCA3_RADJ_PMADJ_ADD		1
+#define RTCA3_RADJ_PMADJ_SUB		2
+
+/* Polling operation timeouts. */
+#define RTCA3_DEFAULT_TIMEOUT_US	150
+#define RTCA3_IRQSET_TIMEOUT_US		5000
+#define RTCA3_START_TIMEOUT_US		150000
+#define RTCA3_RESET_TIMEOUT_US		200000
+
+/**
+ * enum rtca3_alrm_set_step - RTCA3 alarm set steps
+ * @RTCA3_ALRM_SSTEP_DONE: alarm setup done step
+ * @RTCA3_ALRM_SSTEP_IRQ: two 1/64 periodic IRQs were generated step
+ * @RTCA3_ALRM_SSTEP_INIT: alarm setup initialization step
+ */
+enum rtca3_alrm_set_step {
+	RTCA3_ALRM_SSTEP_DONE = 0,
+	RTCA3_ALRM_SSTEP_IRQ = 1,
+	RTCA3_ALRM_SSTEP_INIT = 3,
+};
+
+/**
+ * struct rtca3_ppb_per_cycle - PPB per cycle
+ * @ten_sec: PPB per cycle in 10 seconds adjutment mode
+ * @sixty_sec: PPB per cycle in 60 seconds adjustment mode
+ */
+struct rtca3_ppb_per_cycle {
+	int ten_sec;
+	int sixty_sec;
+};
+
+/**
+ * struct rtca3_priv - RTCA3 private data structure
+ * @base: base address
+ * @clk: RTC clock
+ * @rtc_dev: RTC device
+ * @set_alarm_completion: alarm setup completion
+ * @alrm_sstep: alarm setup step (see enum rtca3_alrm_set_step)
+ * @lock: device lock
+ * @ppb: ppb per cycle for each the available adjustment modes
+ * @wakeup_irq: wakeup IRQ
+ */
+struct rtca3_priv {
+	void __iomem *base;
+	struct clk *clk;
+	struct rtc_device *rtc_dev;
+	struct completion set_alarm_completion;
+	atomic_t alrm_sstep;
+	spinlock_t lock;
+	struct rtca3_ppb_per_cycle ppb;
+	int wakeup_irq;
+};
+
+static void rtca3_byte_update_bits(struct rtca3_priv *priv, u8 off, u8 mask, u8 val)
+{
+	u8 tmp;
+
+	tmp = readb(priv->base + off);
+	tmp &= ~mask;
+	tmp |= (val & mask);
+	writeb(tmp, priv->base + off);
+}
+
+static u8 rtca3_alarm_handler_helper(struct rtca3_priv *priv)
+{
+	u8 val, pending;
+
+	val = readb(priv->base + RTCA3_RSR);
+	pending = val & RTCA3_RSR_AF;
+	writeb(val & ~pending, priv->base + RTCA3_RSR);
+
+	if (pending)
+		rtc_update_irq(priv->rtc_dev, 1, RTC_AF | RTC_IRQF);
+
+	return pending;
+}
+
+static irqreturn_t rtca3_alarm_handler(int irq, void *dev_id)
+{
+	struct rtca3_priv *priv = dev_id;
+	u8 pending;
+
+	guard(spinlock)(&priv->lock);
+
+	pending = rtca3_alarm_handler_helper(priv);
+
+	return IRQ_RETVAL(pending);
+}
+
+static irqreturn_t rtca3_periodic_handler(int irq, void *dev_id)
+{
+	struct rtca3_priv *priv = dev_id;
+	u8 val, pending;
+
+	guard(spinlock)(&priv->lock);
+
+	val = readb(priv->base + RTCA3_RSR);
+	pending = val & RTCA3_RSR_PF;
+
+	if (pending) {
+		writeb(val & ~pending, priv->base + RTCA3_RSR);
+
+		if (atomic_read(&priv->alrm_sstep) > RTCA3_ALRM_SSTEP_IRQ) {
+			/* Alarm setup in progress. */
+			atomic_dec(&priv->alrm_sstep);
+
+			if (atomic_read(&priv->alrm_sstep) == RTCA3_ALRM_SSTEP_IRQ) {
+				/*
+				 * We got 2 * 1/64 periodic interrupts. Disable
+				 * interrupt and let alarm setup continue.
+				 */
+				rtca3_byte_update_bits(priv, RTCA3_RCR1,
+						       RTCA3_RCR1_PIE, 0);
+				readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, val,
+							  !(val & RTCA3_RCR1_PIE),
+							  10, RTCA3_DEFAULT_TIMEOUT_US);
+				complete(&priv->set_alarm_completion);
+			}
+		}
+	}
+
+	return IRQ_RETVAL(pending);
+}
+
+static void rtca3_prepare_cntalrm_regs_for_read(struct rtca3_priv *priv, bool cnt)
+{
+	/* Offset b/w time and alarm registers. */
+	u8 offset = cnt ? 0 : 0xe;
+
+	/*
+	 * According to HW manual (section 22.6.4. Notes on writing to and
+	 * reading from registers) after writing to count registers, alarm
+	 * registers, year alarm enable register, bits RCR2.AADJE, AADJP,
+	 * and HR24 register, we need to do 3 empty reads before being
+	 * able to fetch the registers content.
+	 */
+	for (u8 i = 0; i < 3; i++) {
+		readb(priv->base + RTCA3_RSECCNT + offset);
+		readb(priv->base + RTCA3_RMINCNT + offset);
+		readb(priv->base + RTCA3_RHRCNT  + offset);
+		readb(priv->base + RTCA3_RWKCNT  + offset);
+		readb(priv->base + RTCA3_RDAYCNT + offset);
+		readw(priv->base + RTCA3_RYRCNT  + offset);
+		if (!cnt)
+			readb(priv->base + RTCA3_RYRAREN);
+	}
+}
+
+static int rtca3_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+	u8 sec, min, hour, wday, mday, month, tmp;
+	u8 trials = 0;
+	u32 year100;
+	u16 year;
+
+	guard(spinlock_irqsave)(&priv->lock);
+
+	tmp = readb(priv->base + RTCA3_RCR2);
+	if (!(tmp & RTCA3_RCR2_START))
+		return -EINVAL;
+
+	do {
+		/* Clear carry interrupt. */
+		rtca3_byte_update_bits(priv, RTCA3_RSR, RTCA3_RSR_CF, 0);
+
+		/* Read counters. */
+		sec = readb(priv->base + RTCA3_RSECCNT);
+		min = readb(priv->base + RTCA3_RMINCNT);
+		hour = readb(priv->base + RTCA3_RHRCNT);
+		wday = readb(priv->base + RTCA3_RWKCNT);
+		mday = readb(priv->base + RTCA3_RDAYCNT);
+		month = readb(priv->base + RTCA3_RMONCNT);
+		year = readw(priv->base + RTCA3_RYRCNT);
+
+		tmp = readb(priv->base + RTCA3_RSR);
+
+		/*
+		 * We cannot generate carries due to reading 64Hz counter as
+		 * the driver doesn't implement carry, thus, carries will be
+		 * generated once per seconds. Add a timeout of 5 trials here
+		 * to avoid infinite loop, if any.
+		 */
+	} while ((tmp & RTCA3_RSR_CF) && ++trials < 5);
+
+	if (trials >= 5)
+		return -ETIMEDOUT;
+
+	tm->tm_sec = bcd2bin(FIELD_GET(RTCA3_RSECCNT_SEC, sec));
+	tm->tm_min = bcd2bin(FIELD_GET(RTCA3_RMINCNT_MIN, min));
+	tm->tm_hour = bcd2bin(FIELD_GET(RTCA3_RHRCNT_HR, hour));
+	tm->tm_wday = bcd2bin(FIELD_GET(RTCA3_RWKCNT_WK, wday));
+	tm->tm_mday = bcd2bin(FIELD_GET(RTCA3_RDAYCNT_DAY, mday));
+	tm->tm_mon = bcd2bin(FIELD_GET(RTCA3_RMONCNT_MONTH, month)) - 1;
+	year = FIELD_GET(RTCA3_RYRCNT_YEAR, year);
+	year100 = bcd2bin((year == 0x99) ? 0x19 : 0x20);
+	tm->tm_year = (year100 * 100 + bcd2bin(year)) - 1900;
+
+	return 0;
+}
+
+static int rtca3_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+	u8 rcr2, tmp;
+	int ret;
+
+	guard(spinlock_irqsave)(&priv->lock);
+
+	/* Stop the RTC. */
+	rcr2 = readb(priv->base + RTCA3_RCR2);
+	writeb(rcr2 & ~RTCA3_RCR2_START, priv->base + RTCA3_RCR2);
+	ret = readb_poll_timeout_atomic(priv->base + RTCA3_RCR2, tmp,
+					!(tmp & RTCA3_RCR2_START),
+					10, RTCA3_DEFAULT_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	/* Update time. */
+	writeb(bin2bcd(tm->tm_sec), priv->base + RTCA3_RSECCNT);
+	writeb(bin2bcd(tm->tm_min), priv->base + RTCA3_RMINCNT);
+	writeb(bin2bcd(tm->tm_hour), priv->base + RTCA3_RHRCNT);
+	writeb(bin2bcd(tm->tm_wday), priv->base + RTCA3_RWKCNT);
+	writeb(bin2bcd(tm->tm_mday), priv->base + RTCA3_RDAYCNT);
+	writeb(bin2bcd(tm->tm_mon + 1), priv->base + RTCA3_RMONCNT);
+	writew(bin2bcd(tm->tm_year % 100), priv->base + RTCA3_RYRCNT);
+
+	/* Make sure we can read back the counters. */
+	rtca3_prepare_cntalrm_regs_for_read(priv, true);
+
+	/* Start RTC. */
+	writeb(rcr2 | RTCA3_RCR2_START, priv->base + RTCA3_RCR2);
+	return readb_poll_timeout_atomic(priv->base + RTCA3_RCR2, tmp,
+					 (tmp & RTCA3_RCR2_START),
+					 10, RTCA3_DEFAULT_TIMEOUT_US);
+}
+
+static int rtca3_alarm_irq_enable_helper(struct rtca3_priv *priv,
+					 unsigned int enabled)
+{
+	u8 tmp, mask;
+
+	if (enabled) {
+		rtca3_byte_update_bits(priv, RTCA3_RSR, RTCA3_RSR_AF, 0);
+		mask = RTCA3_RCR1_AIE;
+	} else {
+		mask = 0;
+	}
+
+	rtca3_byte_update_bits(priv, RTCA3_RCR1, RTCA3_RCR1_AIE, mask);
+	return readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp,
+					 ((tmp & RTCA3_RCR1_AIE) == mask),
+					 10, RTCA3_IRQSET_TIMEOUT_US);
+}
+
+static int rtca3_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+
+	guard(spinlock_irqsave)(&priv->lock);
+
+	return rtca3_alarm_irq_enable_helper(priv, enabled);
+}
+
+static int rtca3_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+	u8 sec, min, hour, wday, mday, month;
+	struct rtc_time *tm = &wkalrm->time;
+	u32 year100;
+	u16 year;
+
+	guard(spinlock_irqsave)(&priv->lock);
+
+	sec = readb(priv->base + RTCA3_RSECAR);
+	min = readb(priv->base + RTCA3_RMINAR);
+	hour = readb(priv->base + RTCA3_RHRAR);
+	wday = readb(priv->base + RTCA3_RWKAR);
+	mday = readb(priv->base + RTCA3_RDAYAR);
+	month = readb(priv->base + RTCA3_RMONAR);
+	year = readw(priv->base + RTCA3_RYRAR);
+
+	tm->tm_sec = bcd2bin(FIELD_GET(RTCA3_RSECAR_SEC, sec));
+	tm->tm_min = bcd2bin(FIELD_GET(RTCA3_RMINAR_MIN, min));
+	tm->tm_hour = bcd2bin(FIELD_GET(RTCA3_RHRAR_HR, hour));
+	tm->tm_wday = bcd2bin(FIELD_GET(RTCA3_RWKAR_DAYW, wday));
+	tm->tm_mday = bcd2bin(FIELD_GET(RTCA3_RDAYAR_DATE, mday));
+	tm->tm_mon = bcd2bin(FIELD_GET(RTCA3_RMONAR_MON, month)) - 1;
+	year = FIELD_GET(RTCA3_RYRAR_YR, year);
+	year100 = bcd2bin((year == 0x99) ? 0x19 : 0x20);
+	tm->tm_year = (year100 * 100 + bcd2bin(year)) - 1900;
+
+	wkalrm->enabled = !!(readb(priv->base + RTCA3_RCR1) & RTCA3_RCR1_AIE);
+
+	return 0;
+}
+
+static int rtca3_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+	struct rtc_time *tm = &wkalrm->time;
+	u8 rcr1, tmp;
+	int ret;
+
+	scoped_guard(spinlock_irqsave, &priv->lock) {
+		tmp = readb(priv->base + RTCA3_RCR2);
+		if (!(tmp & RTCA3_RCR2_START))
+			return -EPERM;
+
+		/* Disable AIE to prevent false interrupts. */
+		rcr1 = readb(priv->base + RTCA3_RCR1);
+		rcr1 &= ~RTCA3_RCR1_AIE;
+		writeb(rcr1, priv->base + RTCA3_RCR1);
+		ret = readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp,
+						!(tmp & RTCA3_RCR1_AIE),
+						10, RTCA3_DEFAULT_TIMEOUT_US);
+		if (ret)
+			return ret;
+
+		/* Set the time and enable the alarm. */
+		writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_sec), priv->base + RTCA3_RSECAR);
+		writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_min), priv->base + RTCA3_RMINAR);
+		writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_hour), priv->base + RTCA3_RHRAR);
+		writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_wday), priv->base + RTCA3_RWKAR);
+		writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_mday), priv->base + RTCA3_RDAYAR);
+		writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_mon + 1), priv->base + RTCA3_RMONAR);
+
+		writew(bin2bcd(tm->tm_year % 100), priv->base + RTCA3_RYRAR);
+		writeb(RTCA3_AR_ENB, priv->base + RTCA3_RYRAREN);
+
+		/* Make sure we can read back the counters. */
+		rtca3_prepare_cntalrm_regs_for_read(priv, false);
+
+		/* Need to wait for 2 * 1/64 periodic interrupts to be generated. */
+		atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_INIT);
+		reinit_completion(&priv->set_alarm_completion);
+
+		/* Enable periodic interrupt. */
+		rcr1 |= RTCA3_RCR1_PIE;
+		writeb(rcr1, priv->base + RTCA3_RCR1);
+		ret = readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp,
+						(tmp & RTCA3_RCR1_PIE),
+						10, RTCA3_IRQSET_TIMEOUT_US);
+	}
+
+	if (ret)
+		goto setup_failed;
+
+	/* Wait for the 2 * 1/64 periodic interrupts. */
+	ret = wait_for_completion_interruptible_timeout(&priv->set_alarm_completion,
+							msecs_to_jiffies(500));
+	if (ret <= 0) {
+		ret = -ETIMEDOUT;
+		goto setup_failed;
+	}
+
+	guard(spinlock_irqsave)(&priv->lock);
+
+	ret = rtca3_alarm_irq_enable_helper(priv, wkalrm->enabled);
+	atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_DONE);
+
+	return ret;
+
+setup_failed:
+	scoped_guard(spinlock_irqsave, &priv->lock) {
+		/*
+		 * Disable PIE to avoid interrupt storm in case HW needed more than
+		 * specified timeout for setup.
+		 */
+		writeb(rcr1 & ~RTCA3_RCR1_PIE, priv->base + RTCA3_RCR1);
+		readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp, !(tmp & ~RTCA3_RCR1_PIE),
+					  10, RTCA3_DEFAULT_TIMEOUT_US);
+		atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_DONE);
+	}
+
+	return ret;
+}
+
+static int rtca3_read_offset(struct device *dev, long *offset)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+	u8 val, radj, cycles;
+	u32 ppb_per_cycle;
+
+	scoped_guard(spinlock_irqsave, &priv->lock) {
+		radj = readb(priv->base + RTCA3_RADJ);
+		val = readb(priv->base + RTCA3_RCR2);
+	}
+
+	cycles = FIELD_GET(RTCA3_RADJ_ADJ, radj);
+
+	if (!cycles) {
+		*offset = 0;
+		return 0;
+	}
+
+	if (val & RTCA3_RCR2_ADJP)
+		ppb_per_cycle = priv->ppb.ten_sec;
+	else
+		ppb_per_cycle = priv->ppb.sixty_sec;
+
+	*offset = cycles * ppb_per_cycle;
+	val = FIELD_GET(RTCA3_RADJ_PMADJ, radj);
+	if (val == RTCA3_RADJ_PMADJ_SUB)
+		*offset = -(*offset);
+
+	return 0;
+}
+
+static int rtca3_set_offset(struct device *dev, long offset)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+	int cycles, cycles10, cycles60;
+	u8 radj, adjp, tmp;
+	int ret;
+
+	/*
+	 * Automatic time error adjustment could be set at intervals of 10
+	 * or 60 seconds.
+	 */
+	cycles10 = DIV_ROUND_CLOSEST(offset, priv->ppb.ten_sec);
+	cycles60 = DIV_ROUND_CLOSEST(offset, priv->ppb.sixty_sec);
+
+	/* We can set b/w 1 and 63 clock cycles. */
+	if (cycles60 >= -RTCA3_RADJ_ADJ_MAX &&
+	    cycles60 <= RTCA3_RADJ_ADJ_MAX) {
+		cycles = cycles60;
+		adjp = 0;
+	} else if (cycles10 >= -RTCA3_RADJ_ADJ_MAX &&
+		   cycles10 <= RTCA3_RADJ_ADJ_MAX) {
+		cycles = cycles10;
+		adjp = RTCA3_RCR2_ADJP;
+	} else {
+		return -ERANGE;
+	}
+
+	radj = FIELD_PREP(RTCA3_RADJ_ADJ, abs(cycles));
+	if (!cycles)
+		radj |= FIELD_PREP(RTCA3_RADJ_PMADJ, RTCA3_RADJ_PMADJ_NONE);
+	else if (cycles > 0)
+		radj |= FIELD_PREP(RTCA3_RADJ_PMADJ, RTCA3_RADJ_PMADJ_ADD);
+	else
+		radj |= FIELD_PREP(RTCA3_RADJ_PMADJ, RTCA3_RADJ_PMADJ_SUB);
+
+	guard(spinlock_irqsave)(&priv->lock);
+
+	tmp = readb(priv->base + RTCA3_RCR2);
+
+	if ((tmp & RTCA3_RCR2_ADJP) != adjp) {
+		/* RADJ.PMADJ need to be set to zero before setting RCR2.ADJP. */
+		writeb(0, priv->base + RTCA3_RADJ);
+		ret = readb_poll_timeout_atomic(priv->base + RTCA3_RADJ, tmp, !tmp,
+						10, RTCA3_DEFAULT_TIMEOUT_US);
+		if (ret)
+			return ret;
+
+		rtca3_byte_update_bits(priv, RTCA3_RCR2, RTCA3_RCR2_ADJP, adjp);
+		ret = readb_poll_timeout_atomic(priv->base + RTCA3_RCR2, tmp,
+						((tmp & RTCA3_RCR2_ADJP) == adjp),
+						10, RTCA3_DEFAULT_TIMEOUT_US);
+		if (ret)
+			return ret;
+	}
+
+	writeb(radj, priv->base + RTCA3_RADJ);
+	return readb_poll_timeout_atomic(priv->base + RTCA3_RADJ, tmp, (tmp == radj),
+					 10, RTCA3_DEFAULT_TIMEOUT_US);
+}
+
+static const struct rtc_class_ops rtca3_ops = {
+	.read_time = rtca3_read_time,
+	.set_time = rtca3_set_time,
+	.read_alarm = rtca3_read_alarm,
+	.set_alarm = rtca3_set_alarm,
+	.alarm_irq_enable = rtca3_alarm_irq_enable,
+	.set_offset = rtca3_set_offset,
+	.read_offset = rtca3_read_offset,
+};
+
+static int rtca3_initial_setup(struct rtca3_priv *priv)
+{
+	unsigned long osc32k_rate;
+	u8 val, tmp, mask;
+	u32 sleep_us;
+	int ret;
+
+	osc32k_rate = clk_get_rate(priv->clk);
+	if (!osc32k_rate)
+		return -EINVAL;
+
+	sleep_us = DIV_ROUND_UP_ULL(1000000ULL, osc32k_rate) * 6;
+
+	priv->ppb.ten_sec = DIV_ROUND_CLOSEST_ULL(1000000000ULL, (osc32k_rate * 10));
+	priv->ppb.sixty_sec = DIV_ROUND_CLOSEST_ULL(1000000000ULL, (osc32k_rate * 60));
+
+	/*
+	 * According to HW manual (section 22.4.2. Clock and count mode setting procedure)
+	 * we need to wait at least 6 cycles of the 32KHz clock after clock was enabled.
+	 */
+	usleep_range(sleep_us, sleep_us + 10);
+
+	/* Disable alarm and carry interrupts. */
+	mask = RTCA3_RCR1_AIE | RTCA3_RCR1_CIE;
+	rtca3_byte_update_bits(priv, RTCA3_RCR1, mask, 0);
+	ret = readb_poll_timeout(priv->base + RTCA3_RCR1, tmp, !(tmp & mask),
+				 10, RTCA3_DEFAULT_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	mask = RTCA3_RCR2_START | RTCA3_RCR2_HR24;
+	val = readb(priv->base + RTCA3_RCR2);
+	/* Nothing to do if already started in 24 hours and calendar count mode. */
+	if ((val & mask) == mask)
+		return 0;
+
+	/* Reconfigure the RTC in 24 hours and calendar count mode. */
+	mask = RTCA3_RCR2_START | RTCA3_RCR2_CNTMD;
+	writeb(0, priv->base + RTCA3_RCR2);
+	ret = readb_poll_timeout(priv->base + RTCA3_RCR2, tmp, !(tmp & mask),
+				 10, RTCA3_DEFAULT_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	/*
+	 * Set 24 hours mode. According to HW manual (section 22.3.19. RTC Control
+	 * Register 2) this needs to be done separate from stop operation.
+	 */
+	mask = RTCA3_RCR2_HR24;
+	val = RTCA3_RCR2_HR24;
+	writeb(val, priv->base + RTCA3_RCR2);
+	ret = readb_poll_timeout(priv->base + RTCA3_RCR2, tmp, (tmp & mask),
+				 10, RTCA3_DEFAULT_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	/* Execute reset. */
+	mask = RTCA3_RCR2_RESET;
+	writeb(val | RTCA3_RCR2_RESET, priv->base + RTCA3_RCR2);
+	ret = readb_poll_timeout(priv->base + RTCA3_RCR2, tmp, !(tmp & mask),
+				 10, RTCA3_RESET_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	/*
+	 * According to HW manual (section 22.6.3. Notes on writing to and reading
+	 * from registers) after reset we need to wait 6 clock cycles before
+	 * writing to RTC registers.
+	 */
+	usleep_range(sleep_us, sleep_us + 10);
+
+	/* Set no adjustment. */
+	writeb(0, priv->base + RTCA3_RADJ);
+	ret = readb_poll_timeout(priv->base + RTCA3_RADJ, tmp, !tmp, 10,
+				 RTCA3_DEFAULT_TIMEOUT_US);
+
+	/* Start the RTC and enable automatic time error adjustment. */
+	mask = RTCA3_RCR2_START | RTCA3_RCR2_AADJE;
+	val |= RTCA3_RCR2_START | RTCA3_RCR2_AADJE;
+	writeb(val, priv->base + RTCA3_RCR2);
+	ret = readb_poll_timeout(priv->base + RTCA3_RCR2, tmp, ((tmp & mask) == mask),
+				 10, RTCA3_START_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	/*
+	 * According to HW manual (section 22.6.4. Notes on writing to and reading
+	 * from registers) we need to wait 1/128 seconds while the clock is operating
+	 * (RCR2.START bit = 1) to be able to read the counters after a return from
+	 * reset.
+	 */
+	usleep_range(8000, 9000);
+
+	/* Set period interrupt to 1/64 seconds. It is necessary for alarm setup. */
+	val = FIELD_PREP(RTCA3_RCR1_PES, RTCA3_RCR1_PES_1_64_SEC);
+	rtca3_byte_update_bits(priv, RTCA3_RCR1, RTCA3_RCR1_PES, val);
+	return readb_poll_timeout(priv->base + RTCA3_RCR1, tmp, ((tmp & RTCA3_RCR1_PES) == val),
+				  10, RTCA3_DEFAULT_TIMEOUT_US);
+}
+
+static int rtca3_request_irqs(struct platform_device *pdev, struct rtca3_priv *priv)
+{
+	struct device *dev = &pdev->dev;
+	int ret, irq;
+
+	irq = platform_get_irq_byname(pdev, "alarm");
+	if (irq < 0)
+		return dev_err_probe(dev, irq, "Failed to get alarm IRQ!\n");
+
+	ret = devm_request_irq(dev, irq, rtca3_alarm_handler, 0, "rtca3-alarm", priv);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request alarm IRQ!\n");
+	priv->wakeup_irq = irq;
+
+	irq = platform_get_irq_byname(pdev, "period");
+	if (irq < 0)
+		return dev_err_probe(dev, irq, "Failed to get period IRQ!\n");
+
+	ret = devm_request_irq(dev, irq, rtca3_periodic_handler, 0, "rtca3-period", priv);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request period IRQ!\n");
+
+	/*
+	 * Driver doesn't implement carry handler. Just get the IRQ here
+	 * for backward compatibility, in case carry support will be added later.
+	 */
+	irq = platform_get_irq_byname(pdev, "carry");
+	if (irq < 0)
+		return dev_err_probe(dev, irq, "Failed to get carry IRQ!\n");
+
+	return 0;
+}
+
+static int rtca3_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rtca3_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get_enabled(dev, "counter");
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	platform_set_drvdata(pdev, priv);
+
+	spin_lock_init(&priv->lock);
+	atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_DONE);
+	init_completion(&priv->set_alarm_completion);
+
+	ret = rtca3_initial_setup(priv);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to setup the RTC!\n");
+
+	ret = rtca3_request_irqs(pdev, priv);
+	if (ret)
+		return ret;
+
+	device_init_wakeup(&pdev->dev, 1);
+
+	priv->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(priv->rtc_dev))
+		return PTR_ERR(priv->rtc_dev);
+
+	priv->rtc_dev->ops = &rtca3_ops;
+	priv->rtc_dev->max_user_freq = 256;
+	priv->rtc_dev->range_min = mktime64(2000, 1, 1, 0, 0, 0);
+	priv->rtc_dev->range_max = mktime64(2099, 12, 31, 23, 59, 59);
+
+	return devm_rtc_register_device(priv->rtc_dev);
+}
+
+static void rtca3_remove(struct platform_device *pdev)
+{
+	struct rtca3_priv *priv = platform_get_drvdata(pdev);
+	u8 tmp, mask = RTCA3_RCR1_AIE | RTCA3_RCR1_PIE;
+
+	guard(spinlock_irqsave)(&priv->lock);
+
+	/* Disable alarm, periodic interrupt. */
+	rtca3_byte_update_bits(priv, RTCA3_RCR1, mask, 0);
+	readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp, !(tmp & mask),
+				  10, RTCA3_IRQSET_TIMEOUT_US);
+}
+
+static int __maybe_unused rtca3_suspend(struct device *dev)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	/* Alarm setup in progress. */
+	if (atomic_read(&priv->alrm_sstep) != RTCA3_ALRM_SSTEP_DONE)
+		return -EBUSY;
+
+	enable_irq_wake(priv->wakeup_irq);
+
+	return 0;
+}
+
+static int rtca3_clean_alarm(struct rtca3_priv *priv)
+{
+	struct rtc_device *rtc_dev = priv->rtc_dev;
+	time64_t alarm_time, now;
+	struct rtc_wkalrm alarm;
+	struct rtc_time tm;
+	u8 pending;
+	int ret;
+
+	ret = rtc_read_alarm(rtc_dev, &alarm);
+	if (ret)
+		return ret;
+
+	if (!alarm.enabled)
+		return 0;
+
+	ret = rtc_read_time(rtc_dev, &tm);
+	if (ret)
+		return ret;
+
+	alarm_time = rtc_tm_to_time64(&alarm.time);
+	now = rtc_tm_to_time64(&tm);
+	if (alarm_time >= now)
+		return 0;
+
+	/*
+	 * Heuristically, it has been determined that when returning from deep
+	 * sleep state the RTCA3_RSR.AF is zero even though the alarm expired.
+	 * Call again the rtc_update_irq() if alarm helper detects this.
+	 */
+
+	guard(spinlock_irqsave)(&priv->lock);
+
+	pending = rtca3_alarm_handler_helper(priv);
+	if (!pending)
+		rtc_update_irq(priv->rtc_dev, 1, RTC_AF | RTC_IRQF);
+
+	return 0;
+}
+
+static int __maybe_unused rtca3_resume(struct device *dev)
+{
+	struct rtca3_priv *priv = dev_get_drvdata(dev);
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	disable_irq_wake(priv->wakeup_irq);
+
+	/*
+	 * According to the HW manual (section 22.6.4 Notes on writing to
+	 * and reading from registers) we need to wait 1/128 seconds while
+	 * RCR2.START = 1 to be able to read the counters after a return from low
+	 * power consumption state.
+	 */
+	mdelay(8);
+
+	/*
+	 * The alarm cannot wake the system from deep sleep states. In case
+	 * we return from deep sleep states and the alarm expired we need
+	 * to disable it to avoid failures when setting another alarm.
+	 */
+	return rtca3_clean_alarm(priv);
+}
+
+static SIMPLE_DEV_PM_OPS(rtca3_pm_ops, rtca3_suspend, rtca3_resume);
+
+static const struct of_device_id rtca3_of_match[] = {
+	{ .compatible = "renesas,rz-rtca3", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rtca3_of_match);
+
+static struct platform_driver rtca3_platform_driver = {
+	.driver = {
+		.name = "rtc-rtca3",
+		.pm = &rtca3_pm_ops,
+		.of_match_table = rtca3_of_match,
+	},
+	.probe = rtca3_probe,
+	.remove_new = rtca3_remove,
+};
+module_platform_driver(rtca3_platform_driver);
+
+MODULE_DESCRIPTION("Renesas RTCA-3 RTC driver");
+MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v2 05/11] rtc: renesas-rtca3: Add driver for RTCA-3 available on Renesas RZ/G3S SoC

[kernel test robot]

Hi Claudiu,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20240716]
[also build test ERROR on v6.10]
[cannot apply to geert-renesas-devel/next lee-mfd/for-mfd-next lee-mfd/for-mfd-fixes abelloni/rtc-next linus/master v6.10 v6.10-rc7 v6.10-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Claudiu/dt-bindings-mfd-renesas-r9a08g045-vbattb-Document-VBATTB/20240716-190833
base:   next-20240716
patch link:    https://lore.kernel.org/r/20240716103025.1198495-6-claudiu.beznea.uj%40bp.renesas.com
patch subject: [PATCH v2 05/11] rtc: renesas-rtca3: Add driver for RTCA-3 available on Renesas RZ/G3S SoC
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20240719/202407191156.wJPjHtKG-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project ad154281230d83ee551e12d5be48bb956ef47ed3)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240719/202407191156.wJPjHtKG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407191156.wJPjHtKG-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/rtc/rtc-renesas-rtca3.c:433:3: error: cannot jump from this goto statement to its label
     433 |                 goto setup_failed;
         |                 ^
   drivers/rtc/rtc-renesas-rtca3.c:436:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
     436 |         guard(spinlock_irqsave)(&priv->lock);
         |         ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:67:1: note: expanded from here
      67 | __UNIQUE_ID_guard738
         | ^
   drivers/rtc/rtc-renesas-rtca3.c:426:3: error: cannot jump from this goto statement to its label
     426 |                 goto setup_failed;
         |                 ^
   drivers/rtc/rtc-renesas-rtca3.c:436:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
     436 |         guard(spinlock_irqsave)(&priv->lock);
         |         ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:67:1: note: expanded from here
      67 | __UNIQUE_ID_guard738
         | ^
   2 errors generated.


vim +433 drivers/rtc/rtc-renesas-rtca3.c

   376	
   377	static int rtca3_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
   378	{
   379		struct rtca3_priv *priv = dev_get_drvdata(dev);
   380		struct rtc_time *tm = &wkalrm->time;
   381		u8 rcr1, tmp;
   382		int ret;
   383	
   384		scoped_guard(spinlock_irqsave, &priv->lock) {
   385			tmp = readb(priv->base + RTCA3_RCR2);
   386			if (!(tmp & RTCA3_RCR2_START))
   387				return -EPERM;
   388	
   389			/* Disable AIE to prevent false interrupts. */
   390			rcr1 = readb(priv->base + RTCA3_RCR1);
   391			rcr1 &= ~RTCA3_RCR1_AIE;
   392			writeb(rcr1, priv->base + RTCA3_RCR1);
   393			ret = readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp,
   394							!(tmp & RTCA3_RCR1_AIE),
   395							10, RTCA3_DEFAULT_TIMEOUT_US);
   396			if (ret)
   397				return ret;
   398	
   399			/* Set the time and enable the alarm. */
   400			writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_sec), priv->base + RTCA3_RSECAR);
   401			writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_min), priv->base + RTCA3_RMINAR);
   402			writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_hour), priv->base + RTCA3_RHRAR);
   403			writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_wday), priv->base + RTCA3_RWKAR);
   404			writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_mday), priv->base + RTCA3_RDAYAR);
   405			writeb(RTCA3_AR_ENB | bin2bcd(tm->tm_mon + 1), priv->base + RTCA3_RMONAR);
   406	
   407			writew(bin2bcd(tm->tm_year % 100), priv->base + RTCA3_RYRAR);
   408			writeb(RTCA3_AR_ENB, priv->base + RTCA3_RYRAREN);
   409	
   410			/* Make sure we can read back the counters. */
   411			rtca3_prepare_cntalrm_regs_for_read(priv, false);
   412	
   413			/* Need to wait for 2 * 1/64 periodic interrupts to be generated. */
   414			atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_INIT);
   415			reinit_completion(&priv->set_alarm_completion);
   416	
   417			/* Enable periodic interrupt. */
   418			rcr1 |= RTCA3_RCR1_PIE;
   419			writeb(rcr1, priv->base + RTCA3_RCR1);
   420			ret = readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp,
   421							(tmp & RTCA3_RCR1_PIE),
   422							10, RTCA3_IRQSET_TIMEOUT_US);
   423		}
   424	
   425		if (ret)
   426			goto setup_failed;
   427	
   428		/* Wait for the 2 * 1/64 periodic interrupts. */
   429		ret = wait_for_completion_interruptible_timeout(&priv->set_alarm_completion,
   430								msecs_to_jiffies(500));
   431		if (ret <= 0) {
   432			ret = -ETIMEDOUT;
 > 433			goto setup_failed;
   434		}
   435	
   436		guard(spinlock_irqsave)(&priv->lock);
   437	
   438		ret = rtca3_alarm_irq_enable_helper(priv, wkalrm->enabled);
   439		atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_DONE);
   440	
   441		return ret;
   442	
   443	setup_failed:
   444		scoped_guard(spinlock_irqsave, &priv->lock) {
   445			/*
   446			 * Disable PIE to avoid interrupt storm in case HW needed more than
   447			 * specified timeout for setup.
   448			 */
   449			writeb(rcr1 & ~RTCA3_RCR1_PIE, priv->base + RTCA3_RCR1);
   450			readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp, !(tmp & ~RTCA3_RCR1_PIE),
   451						  10, RTCA3_DEFAULT_TIMEOUT_US);
   452			atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_DONE);
   453		}
   454	
   455		return ret;
   456	}
   457	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 06/11] arm64: dts: renesas: r9a08g045: Add VBATTB node

[Claudiu]

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

Add the DT node for the VBATTB IP along with DT bindings for the clock
it provides.

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

Changes in v2:
- update compatibles
- updated clocks and clock-names for clock-controller node
- removed the power domain from the clock-controller as this is
  controlled by parent node in v2

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

diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index 0d5c47a65e46..78b4e088a3a5 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -72,6 +72,30 @@ scif0: serial@1004b800 {
 			status = "disabled";
 		};
 
+		vbattb: vbattb@1005c000 {
+			compatible = "renesas,r9a08g045-vbattb";
+			reg = <0 0x1005c000 0 0x1000>;
+			ranges = <0 0 0 0x1005c000 0 0x1000>;
+			interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tampdi";
+			clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>;
+			clock-names = "bclk";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A08G045_VBAT_BRESETN>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			status = "disabled";
+
+			vbattclk: clock-controller@1c {
+				compatible = "renesas,r9a08g045-vbattb-clk";
+				reg = <0 0x1c 0 0x10>;
+				clocks = <&vbattb_xtal>;
+				clock-names = "xin";
+				#clock-cells = <0>;
+				status = "disabled";
+			};
+		};
+
 		cpg: clock-controller@11010000 {
 			compatible = "renesas,r9a08g045-cpg";
 			reg = <0 0x11010000 0 0x10000>;
@@ -299,4 +323,11 @@ timer {
 		interrupt-names = "sec-phys", "phys", "virt", "hyp-phys",
 				  "hyp-virt";
 	};
+
+	vbattb_xtal: vbattb-xtal {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		/* This value must be overridden by the board. */
+		clock-frequency = <0>;
+	};
 };
-- 
2.39.2

[PATCH v2 07/11] arm64: dts: renesas: r9a08g045: Add RTC node

[Claudiu]

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

Add the DT node for the RTC IP available on the Renesas RZ/G3S SoC.

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

Changes in v2:
- updated compatibles

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

diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index 78b4e088a3a5..22008407848c 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -72,6 +72,18 @@ scif0: serial@1004b800 {
 			status = "disabled";
 		};
 
+		rtc: rtc@1004ec00 {
+			compatible = "renesas,r9a08g045-rtca3", "renesas,rz-rtca3";
+			reg = <0 0x1004ec00 0 0x400>;
+			interrupts = <GIC_SPI 315 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 316 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "alarm", "period", "carry";
+			clocks = <&vbattclk>;
+			clock-names = "counter";
+			status = "disabled";
+		};
+
 		vbattb: vbattb@1005c000 {
 			compatible = "renesas,r9a08g045-vbattb";
 			reg = <0 0x1005c000 0 0x1000>;
-- 
2.39.2

[PATCH v2 08/11] arm64: dts: renesas: rzg3s-smarc-som: Enable VBATTB clock

[Claudiu]

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

Enable the VBATTB clock controller.

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

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi | 13 +++++++++++++
 1 file changed, 13 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..517ce275916a 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -341,6 +341,19 @@ mux {
 	};
 };
 
+&vbattb_xtal {
+	clock-frequency = <32768>;
+};
+
+&vbattb {
+	status = "okay";
+};
+
+&vbattclk {
+	renesas,vbattb-load-nanofarads = <12500>;
+	status = "okay";
+};
+
 &wdt0 {
 	timeout-sec = <60>;
 	status = "okay";
-- 
2.39.2

[PATCH v2 09/11] arm64: dts: renesas: rzg3s-smarc-som: Enable RTC

[Claudiu]

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

Enable RTC.

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

Changes in v2:
- none

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

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index 517ce275916a..82a80fd8e7ec 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -341,6 +341,10 @@ mux {
 	};
 };
 
+&rtc {
+	status = "okay";
+};
+
 &vbattb_xtal {
 	clock-frequency = <32768>;
 };
-- 
2.39.2

[PATCH v2 10/11] arm64: defconfig: Enable VBATTB

[Claudiu]

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

Enable VBATTB MFD and clock driver flags.

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

Changes in v2:
- added CONFIG_MFD_RENESAS_VBATTB
- added vendor name in the VBATTB clock flag

 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 5c9fcf9ad395..f35fd006bbbc 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -750,6 +750,7 @@ CONFIG_MFD_MAX77620=y
 CONFIG_MFD_MT6360=y
 CONFIG_MFD_MT6397=y
 CONFIG_MFD_SPMI_PMIC=y
+CONFIG_MFD_RENESAS_VBATTB=y
 CONFIG_MFD_RK8XX_I2C=y
 CONFIG_MFD_RK8XX_SPI=y
 CONFIG_MFD_SEC_CORE=y
@@ -1358,6 +1359,7 @@ CONFIG_SM_VIDEOCC_8250=y
 CONFIG_QCOM_HFPLL=y
 CONFIG_CLK_GFM_LPASS_SM8250=m
 CONFIG_CLK_RCAR_USB2_CLOCK_SEL=y
+CONFIG_CLK_RENESAS_VBATTB=y
 CONFIG_HWSPINLOCK=y
 CONFIG_HWSPINLOCK_QCOM=y
 CONFIG_TEGRA186_TIMER=y
-- 
2.39.2

[PATCH v2 11/11] arm64: defconfig: Enable Renesas RTCA-3 flag

[Claudiu]

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

Enable Renesas RTCA-3 flag for the Renesas RZ/G3S SoC.

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

Changes in v2:
- none

 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index f35fd006bbbc..e90578659447 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1213,6 +1213,7 @@ CONFIG_RTC_DRV_IMX_SC=m
 CONFIG_RTC_DRV_MT6397=m
 CONFIG_RTC_DRV_XGENE=y
 CONFIG_RTC_DRV_TI_K3=m
+CONFIG_RTC_DRV_RENESAS_RTCA3=y
 CONFIG_DMADEVICES=y
 CONFIG_DMA_BCM2835=y
 CONFIG_DMA_SUN6I=m
-- 
2.39.2