[PATCH v3 0/4] riscv: rtc: sophgo: add mfd and rtc support for CV1800

[PATCH v3 0/4] riscv: rtc: sophgo: add mfd and rtc support for CV1800

[Jingbao Qiu]

This patch serises are to add rtc driver and providers by writing
and reading syscon registers for the CV1800 RISC-V SoC. And add
documentation and nodes to describe System Controller(syscon).

This patch series depends on the clk patch support for Sophgo CV1800 SoC
patch series which can be found at below link:
https://lore.kernel.org/all/IA1PR20MB495354167CE560FC18E28DC5BB90A@IA1PR20MB4953.namprd20.prod.outlook.com/

Changes since v2:
- add mfd support for CV1800
- add rtc to mfd
- using regmap replace iomap
- merge register address in dts

v2: https://lore.kernel.org/lkml/20231217110952.78784-1-qiujingbao.dlmu@gmail.com/

Changes since v1
- fix duplicate names in subject
- using RTC replace RTC controller
- improve the properties of dt-bindings
- using `unevaluatedProperties` replace `additionalProperties`
- dt-bindings passed the test
- using `devm_platform_ioremap_resource()` replace
  `platform_get_resource()` and `devm_ioremap_resource()`
- fix random order of the code
- fix wrong wrapping of the `devm_request_irq()` and map the flag with dts
- using devm_clk_get_enabled replace `devm_clk_get()` and
  `clk_prepare_enable()`
- fix return style
- add rtc clock calibration function
- use spinlock when write register on read/set time

v1: https://lore.kernel.org/lkml/20231121094642.2973795-1-qiujingbao.dlmu@gmail.com/

Jingbao Qiu (4):
  dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800
    series SoC
  dt-bindings: rtc: sophgo: add RTC support for Sophgo CV1800 series SoC
  rtc: sophgo: add rtc support for Sophgo CV1800 SoC
  riscv: dts: sophgo: add rtc dt node for CV1800

 .../bindings/mfd/sophgo,cv1800-subsys.yaml    |  51 +++
 .../bindings/rtc/sophgo,cv1800-rtc.yaml       |  45 ++
 arch/riscv/boot/dts/sophgo/cv1800b.dtsi       |  11 +
 drivers/rtc/Kconfig                           |   6 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-cv1800.c                      | 417 ++++++++++++++++++
 6 files changed, 531 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
 create mode 100644 Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.yaml
 create mode 100644 drivers/rtc/rtc-cv1800.c


base-commit: dc0684adf3b6be6b20fec9295027980021d30353
-- 
2.25.1

[PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Jingbao Qiu]

Add devicetree binding for Sophgo CV1800 SoC MFD subsys.

Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
---
 .../bindings/mfd/sophgo,cv1800-subsys.yaml    | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml

diff --git a/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml b/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
new file mode 100644
index 000000000000..c2a071c8a2de
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/sophgo,cv1800-subsys.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo CV1800 SoC subsys controller
+
+maintainers:
+  - Jingbao Qiu <qiujingbao.dlmu@gmail.com>
+
+description:
+  The Sophgo CV1800 SoC subsys controller contains many functions
+  for example, RTC and restart. In addition, CV1800 has an 8051
+  subsystem, which is configured through registers at this controller.
+
+properties:
+  compatible:
+    items:
+      - const: sophgo,cv1800b-subsys
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  rtc:
+    $ref: /schemas/rtc/sophgo,cv1800-rtc.yaml#
+    type: object
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/sophgo,cv1800.h>
+
+    syscon@5025000 {
+      compatible = "sophgo,cv1800b-subsys", "syscon", "simple-mfd";
+      reg = <0x05025000 0x2000>;
+
+      rtc {
+        compatible = "sophgo,cv1800b-rtc";
+        interrupts = <17 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clk CLK_RTC_25M>;
+      };
+    };
-- 
2.25.1

Re: [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Rob Herring]

On Tue, 26 Dec 2023 18:04:28 +0800, Jingbao Qiu wrote:
> Add devicetree binding for Sophgo CV1800 SoC MFD subsys.
> 
> Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> ---
>  .../bindings/mfd/sophgo,cv1800-subsys.yaml    | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml:
Error in referenced schema matching $id: http://devicetree.org/schemas/rtc/sophgo,cv1800-rtc.yaml
Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.example.dts:25:18: fatal error: dt-bindings/clock/sophgo,cv1800.h: No such file or directory
   25 |         #include <dt-bindings/clock/sophgo,cv1800.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231226100431.331616-2-qiujingbao.dlmu@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Jingbao Qiu]

On Tue, Dec 26, 2023 at 7:39 PM Rob Herring <robh@kernel.org> wrote:
>
>
> On Tue, 26 Dec 2023 18:04:28 +0800, Jingbao Qiu wrote:
> > Add devicetree binding for Sophgo CV1800 SoC MFD subsys.
> >
> > Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> > ---
> >  .../bindings/mfd/sophgo,cv1800-subsys.yaml    | 51 +++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>

I'm sorry for that. My current patch depends on a CLK patch that has
not been merged yet.
So should I wait for the clk patch to merge before submitting?
the depends patch link:
https://lore.kernel.org/all/IA1PR20MB49539CDAD9A268CBF6CA184BBB9FA@IA1PR20MB4953.namprd20.prod.outlook.com/

> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml:
> Error in referenced schema matching $id: http://devicetree.org/schemas/rtc/sophgo,cv1800-rtc.yaml
> Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.example.dts:25:18: fatal error: dt-bindings/clock/sophgo,cv1800.h: No such file or directory
>    25 |         #include <dt-bindings/clock/sophgo,cv1800.h>
>       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.
> make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.example.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231226100431.331616-2-qiujingbao.dlmu@gmail.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>

Best regards,
Jingbao Qiu

Re: [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Krzysztof Kozlowski]

On 26/12/2023 11:04, Jingbao Qiu wrote:
> Add devicetree binding for Sophgo CV1800 SoC MFD subsys.

Subject and commit msg: there is no such hardware as "MFD subsys". Is
this a PMIC? Does not look like. You must describe here hardware, not
Linux subsystem.

> 
> Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> ---

Please mention the dependency here.

>  .../bindings/mfd/sophgo,cv1800-subsys.yaml    | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml b/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
> new file mode 100644
> index 000000000000..c2a071c8a2de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/sophgo,cv1800-subsys.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo CV1800 SoC subsys controller
> +
> +maintainers:
> +  - Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> +
> +description:
> +  The Sophgo CV1800 SoC subsys controller contains many functions

What is "subsys"? Why is it in MFD directory? SoC components like
system-controllers do not go to MFD.

> +  for example, RTC and restart. In addition, CV1800 has an 8051
> +  subsystem, which is configured through registers at this controller.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: sophgo,cv1800b-subsys
> +      - const: syscon
> +      - const: simple-mfd
> +
> +  reg:
> +    maxItems: 1
> +
> +  rtc:
> +    $ref: /schemas/rtc/sophgo,cv1800-rtc.yaml#

Your patchset is not bisectable. What's more, you have dependency
between patches, so bindings cannot go via separate trees: mfd and rtc.
You need to make this *EXPLICIT* in the cover letter or patch changelog.

I do not see any resources in MFD block, so why having it as separate
node? What other devices you did not describe here? You mentioned
restart and 8051, so where are they? Which driver implements them?


Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Jingbao Qiu]

On Tue, Dec 26, 2023 at 8:18 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 26/12/2023 11:04, Jingbao Qiu wrote:
> > Add devicetree binding for Sophgo CV1800 SoC MFD subsys.
>
> Subject and commit msg: there is no such hardware as "MFD subsys". Is
> this a PMIC? Does not look like. You must describe here hardware, not
> Linux subsystem.
>

I don't think this is a PMIC device. the RTC restart and 8051
configure register share one
common range address, and the address have other function that power management.
the datasheet link:
Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf
chapter: 3.9 RTC 3.12 8051 subsystem

> >
> > Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> > ---
>
> Please mention the dependency here.

Thanks,I will fix it.

>
> >  .../bindings/mfd/sophgo,cv1800-subsys.yaml    | 51 +++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml b/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
> > new file mode 100644
> > index 000000000000..c2a071c8a2de
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml
> > @@ -0,0 +1,51 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/sophgo,cv1800-subsys.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sophgo CV1800 SoC subsys controller
> > +
> > +maintainers:
> > +  - Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> > +
> > +description:
> > +  The Sophgo CV1800 SoC subsys controller contains many functions
>
> What is "subsys"? Why is it in MFD directory? SoC components like
> system-controllers do not go to MFD.

This device has multiple different functions, because they have 8051 subsystem,
so I named him "subsys". I will carefully consider and rename it.

>
> > +  for example, RTC and restart. In addition, CV1800 has an 8051
> > +  subsystem, which is configured through registers at this controller.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: sophgo,cv1800b-subsys
> > +      - const: syscon
> > +      - const: simple-mfd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  rtc:
> > +    $ref: /schemas/rtc/sophgo,cv1800-rtc.yaml#
>
> Your patchset is not bisectable. What's more, you have dependency
> between patches, so bindings cannot go via separate trees: mfd and rtc.
> You need to make this *EXPLICIT* in the cover letter or patch changelog.

ok,I will fix it.

>
> I do not see any resources in MFD block, so why having it as separate
> node? What other devices you did not describe here? You mentioned
> restart and 8051, so where are they? Which driver implements them?
>

I'am sorry for that other drivers have not been implemented yet. I
will implement it
after rtc. They have the same address range, so I use mfd to describe them.

>
> Best regards,
> Krzysztof
>

Best regards,
Jingbao Qiu

Re: [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Krzysztof Kozlowski]

On 27/12/2023 08:35, Jingbao Qiu wrote:
>>
>> I do not see any resources in MFD block, so why having it as separate
>> node? What other devices you did not describe here? You mentioned
>> restart and 8051, so where are they? Which driver implements them?
>>
> 
> I'am sorry for that other drivers have not been implemented yet. I
> will implement it
> after rtc. They have the same address range, so I use mfd to describe them.

Bindings should be complete even if your driver is not ready. After
looking at such device node, I say you do not need that rtc child. If
you sent complete bindings, then of course discussion would be
different, but...

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Jingbao Qiu]

On Wed, Dec 27, 2023 at 7:37 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 27/12/2023 08:35, Jingbao Qiu wrote:
> >>
> >> I do not see any resources in MFD block, so why having it as separate
> >> node? What other devices you did not describe here? You mentioned
> >> restart and 8051, so where are they? Which driver implements them?
> >>
> >
> > I'am sorry for that other drivers have not been implemented yet. I
> > will implement it
> > after rtc. They have the same address range, so I use mfd to describe them.
>
> Bindings should be complete even if your driver is not ready. After
> looking at such device node, I say you do not need that rtc child. If
> you sent complete bindings, then of course discussion would be
> different, but...

Thank you for your patient explanation. I will supplement it completely
in the next version

Best regards,
Jingbao Qiu

>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Krzysztof Kozlowski]

On 26/12/2023 11:04, Jingbao Qiu wrote:
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/clock/sophgo,cv1800.h>
> +
> +    syscon@5025000 {

This example and DTS suggest this is system-controller, so use that
name. Assuming this is system-controller, because I am still not sure.

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC

[Jingbao Qiu]

On Tue, Dec 26, 2023 at 8:21 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 26/12/2023 11:04, Jingbao Qiu wrote:
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/clock/sophgo,cv1800.h>
> > +
> > +    syscon@5025000 {
>
> This example and DTS suggest this is system-controller, so use that
> name. Assuming this is system-controller, because I am still not sure.
>

Thanks,I will carefully consider how to name him.

> Best regards,
> Krzysztof
>

[PATCH v3 2/4] dt-bindings: rtc: sophgo: add RTC support for Sophgo CV1800 series SoC

[Jingbao Qiu]

Add devicetree binding for Sophgo CV1800 SoC.

Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
---
 .../bindings/rtc/sophgo,cv1800-rtc.yaml       | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.yaml

diff --git a/Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.yaml b/Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.yaml
new file mode 100644
index 000000000000..eb86fd432053
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/sophgo,cv1800-rtc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Real Time Clock of the Sophgo CV1800 SoC
+
+maintainers:
+  - Jingbao Qiu <qiujingbao.dlmu@gmail.com>
+
+description:
+  This is the RTC device for the CV1800 SoC. This device
+  is placed under MFD and shares an address with other devices.
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    const: sophgo,cv1800b-rtc
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - interrupts
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/sophgo,cv1800.h>
+
+    rtc {
+      compatible = "sophgo,cv1800b-rtc";
+      interrupts = <17 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&clk CLK_RTC_25M>;
+    };
-- 
2.25.1

Re: [PATCH v3 2/4] dt-bindings: rtc: sophgo: add RTC support for Sophgo CV1800 series SoC

[Rob Herring]

On Tue, 26 Dec 2023 18:04:29 +0800, Jingbao Qiu wrote:
> Add devicetree binding for Sophgo CV1800 SoC.
> 
> Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> ---
>  .../bindings/rtc/sophgo,cv1800-rtc.yaml       | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.example.dts:25:18: fatal error: dt-bindings/clock/sophgo,cv1800.h: No such file or directory
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.example.dtb] Error 1

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231226100431.331616-3-qiujingbao.dlmu@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC

[Jingbao Qiu]

Implement the RTC driver for CV1800, which able to provide time alarm
and calibrate functionality.

Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
---
 drivers/rtc/Kconfig      |   6 +
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-cv1800.c | 417 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 424 insertions(+)
 create mode 100644 drivers/rtc/rtc-cv1800.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 4e3c2fde2d6a..d0b35ad92d52 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1103,6 +1103,12 @@ config RTC_DRV_DS2404
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-ds2404.
 
+config RTC_DRV_CV1800
+	tristate "Sophgo CV1800 RTC"
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  Say y here to support the RTC driver for Sophgo CV1800.
+
 config RTC_DRV_DA9052
 	tristate "Dialog DA9052/DA9053 RTC"
 	depends on PMIC_DA9052
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 6817ea4e4468..c08433c2126c 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_RTC_DRV_CADENCE)	+= rtc-cadence.o
 obj-$(CONFIG_RTC_DRV_CMOS)	+= rtc-cmos.o
 obj-$(CONFIG_RTC_DRV_CPCAP)	+= rtc-cpcap.o
 obj-$(CONFIG_RTC_DRV_CROS_EC)	+= rtc-cros-ec.o
+obj-$(CONFIG_RTC_DRV_CV1800)	+= rtc-cv1800.o
 obj-$(CONFIG_RTC_DRV_DA9052)	+= rtc-da9052.o
 obj-$(CONFIG_RTC_DRV_DA9055)	+= rtc-da9055.o
 obj-$(CONFIG_RTC_DRV_DA9063)	+= rtc-da9063.o
diff --git a/drivers/rtc/rtc-cv1800.c b/drivers/rtc/rtc-cv1800.c
new file mode 100644
index 000000000000..865577bc3246
--- /dev/null
+++ b/drivers/rtc/rtc-cv1800.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * rtc-cv1800.c: RTC driver for Sophgo cv1800 RTC
+ *
+ * Author: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
+ */
+#include <linux/kernel.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/delay.h>
+#include <linux/rtc.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#define ANA_CALIB                   0x1000
+#define SEC_PULSE_GEN               0x1004
+#define ALARM_TIME                  0x1008
+#define ALARM_ENABLE                0x100C
+#define SET_SEC_CNTR_VAL            0x1010
+#define SET_SEC_CNTR_TRIG           0x1014
+#define SEC_CNTR_VAL                0x1018
+#define APB_RDATA_SEL               0x103C
+#define POR_DB_MAGIC_KEY            0x1068
+#define EN_PWR_WAKEUP               0x10BC
+#define MACRO_DA_CLEAR_ALL          0x1480
+#define MACRO_DA_SOC_READY          0x148C
+#define MACRO_RO_T                  0x14A8
+#define MACRO_RG_SET_T              0x1498
+
+#define CTRL                        0x08
+#define FC_COARSE_EN                0x40
+#define FC_COARSE_CAL               0x44
+#define FC_FINE_EN                  0x48
+#define FC_FINE_CAL                 0x50
+#define CTRL_MODE_MASK              BIT(10)
+#define CTRL_MODE_OSC32K            0x00UL
+#define CTRL_MODE_XTAL32K           BIT(0)
+
+#define FC_COARSE_CAL_VAL_SHIFT     0
+#define FC_COARSE_CAL_VAL_MASK      GENMASK(15, 0)
+#define FC_COARSE_CAL_TIME_SHIFT    16
+#define FC_COARSE_CAL_TIME_MASK     GENMASK(31, 16)
+#define FC_FINE_CAL_VAL_SHIFT       0
+#define FC_FINE_CAL_VAL_MASK        GENMASK(23, 0)
+#define FC_FINE_CAL_TIME_SHIFT      24
+#define FC_FINE_CAL_TIME_MASK       GENMASK(31, 24)
+
+#define SEC_PULSE_GEN_INT_SHIFT     0
+#define SEC_PULSE_GEN_INT_MASK      GENMASK(7, 0)
+#define SEC_PULSE_GEN_FRAC_SHIFT    8
+#define SEC_PULSE_GEN_FRAC_MASK     GENMASK(24, 8)
+#define SEC_PULSE_GEN_SEL_SHIFT     31
+#define SEC_PULSE_GEN_SEL_MASK      GENMASK(30, 0)
+#define SEC_PULSE_SEL_INNER         BIT(31)
+
+#define CALIB_INIT_VAL              (BIT(8) || BIT(16))
+#define CALIB_SEL_FTUNE_MASK        GENMASK(30, 0)
+#define CALIB_SEL_FTUNE_INNER       0x00UL
+#define CALIB_OFFSET_INIT           128
+#define CALIB_OFFSET_SHIFT          BIT(0)
+#define CALIB_FREQ                  256000000000
+#define CALIB_FRAC_EXT              10000
+#define CALIB_FREQ_NS               40
+#define CALIB_FREQ_MULT             256
+#define CALIB_FC_COARSE_PLUS_OFFSET 770
+#define CALIB_FC_COARSE_SUB_OFFSET  755
+
+#define REG_ENABLE_FUN              BIT(0)
+#define REG_DISABLE_FUN             0x00UL
+#define REG_INIT_TIMEOUT            100
+#define SEC_MAX_VAL                 0xFFFFFFFF
+#define ALARM_ENABLE_MASK           BIT(0)
+#define SET_SEC_CNTR_VAL_UPDATE     (BIT(28) || BIT(29))
+#define DEALY_TIME_PREPARE          400
+#define DEALY_TIME_LOOP             100
+
+struct cv1800_rtc_priv {
+	struct rtc_device *rtc_dev;
+	struct device *dev;
+	struct regmap *rtc_map;
+	struct clk *clk;
+	spinlock_t rtc_lock;
+	int irq;
+};
+
+static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
+
+	if (enabled)
+		regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
+	else
+		regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
+
+	return 0;
+}
+
+static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
+	unsigned long alarm_time;
+
+	alarm_time = rtc_tm_to_time64(&alrm->time);
+
+	if (alarm_time > SEC_MAX_VAL)
+		return -EINVAL;
+
+	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
+
+	udelay(DEALY_TIME_PREPARE);
+
+	regmap_write(info->rtc_map, ALARM_TIME, alarm_time);
+	regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
+
+	return 0;
+}
+
+static int cv1800_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
+	uint32_t enabled;
+	uint32_t time;
+
+	regmap_read(info->rtc_map, ALARM_ENABLE, &enabled);
+
+	alarm->enabled = enabled & ALARM_ENABLE_MASK;
+
+	regmap_read(info->rtc_map, ALARM_TIME, &time);
+
+	rtc_time64_to_tm(time, &alarm->time);
+
+	return 0;
+}
+
+static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info)
+{
+	uint32_t calib_val = 0;
+	uint32_t coarse_val = 0;
+	uint32_t time_now = 0;
+	uint32_t time_next = 0;
+	uint32_t offset = CALIB_OFFSET_INIT;
+	uint32_t coarse_timeout = REG_INIT_TIMEOUT;
+	uint32_t get_val_timeout = 0;
+
+	regmap_write(info->rtc_map, ANA_CALIB, CALIB_INIT_VAL);
+
+	udelay(DEALY_TIME_PREPARE);
+
+	/* Select 32K OSC tuning val source from rtc_sys */
+	regmap_update_bits(info->rtc_map, SEC_PULSE_GEN,
+			   (unsigned int)(~SEC_PULSE_GEN_SEL_MASK),
+			   (unsigned int)(~SEC_PULSE_SEL_INNER));
+
+	regmap_read(info->rtc_map, ANA_CALIB, &calib_val);
+
+	regmap_write(info->rtc_map, FC_COARSE_EN, REG_ENABLE_FUN);
+
+	while (--coarse_timeout) {
+		regmap_read(info->rtc_map, FC_COARSE_CAL, &time_now);
+		time_now >>= FC_COARSE_CAL_TIME_SHIFT;
+
+		get_val_timeout = REG_INIT_TIMEOUT;
+
+		while (time_next <= time_now &&
+		       --get_val_timeout) {
+			regmap_read(info->rtc_map, FC_COARSE_CAL,
+				    &time_next);
+			time_next >>= FC_COARSE_CAL_TIME_SHIFT;
+			udelay(DEALY_TIME_LOOP);
+		}
+
+		if (!get_val_timeout)
+			return -1;
+
+		udelay(DEALY_TIME_PREPARE);
+
+		regmap_read(info->rtc_map, FC_COARSE_CAL, &coarse_val);
+		coarse_val &= FC_COARSE_CAL_VAL_MASK;
+
+		if (coarse_val > CALIB_FC_COARSE_PLUS_OFFSET) {
+			calib_val += offset;
+			offset >>= CALIB_OFFSET_SHIFT;
+			regmap_write(info->rtc_map, ANA_CALIB,
+				     calib_val);
+		} else if (coarse_val < CALIB_FC_COARSE_SUB_OFFSET) {
+			calib_val -= offset;
+			offset >>= CALIB_OFFSET_SHIFT;
+			regmap_write(info->rtc_map, ANA_CALIB,
+				     calib_val);
+		} else {
+			regmap_write(info->rtc_map, FC_COARSE_EN,
+				     REG_DISABLE_FUN);
+			break;
+		}
+
+		if (offset == 0)
+			return -1;
+	}
+
+	return 0;
+}
+
+static int cv1800_rtc_32k_fine_val_calib(struct cv1800_rtc_priv *info)
+{
+	uint32_t val;
+	uint64_t freq = CALIB_FREQ;
+	uint32_t sec_cnt;
+	uint32_t timeout = REG_INIT_TIMEOUT;
+	uint32_t time_now = 0;
+	uint32_t time_next = 0;
+
+	regmap_write(info->rtc_map, FC_FINE_EN, REG_ENABLE_FUN);
+
+	regmap_read(info->rtc_map, FC_FINE_CAL, &time_now);
+	time_now >>= FC_FINE_CAL_TIME_SHIFT;
+
+	while (time_next <= time_now && --timeout) {
+		regmap_read(info->rtc_map, FC_FINE_CAL, &time_next);
+		time_next >>= FC_FINE_CAL_TIME_SHIFT;
+		udelay(DEALY_TIME_LOOP);
+	}
+
+	if (!timeout)
+		return -1;
+
+	regmap_read(info->rtc_map, FC_FINE_CAL, &val);
+	val &= FC_FINE_CAL_VAL_MASK;
+
+	do_div(freq, CALIB_FREQ_NS);
+	freq = freq * CALIB_FRAC_EXT;
+	do_div(freq, val);
+
+	sec_cnt = ((do_div(freq, CALIB_FRAC_EXT) * CALIB_FREQ_MULT) /
+			   CALIB_FRAC_EXT &
+		   SEC_PULSE_GEN_INT_MASK) +
+		  (freq << SEC_PULSE_GEN_FRAC_SHIFT);
+
+	regmap_write(info->rtc_map, SEC_PULSE_GEN, sec_cnt);
+	regmap_write(info->rtc_map, FC_FINE_EN, REG_DISABLE_FUN);
+
+	return 0;
+}
+
+static void rtc_enable_sec_counter(struct cv1800_rtc_priv *info)
+{
+	/* select inner sec pulse and select reg set calibration val */
+	regmap_update_bits(info->rtc_map, SEC_PULSE_GEN,
+			   (unsigned int)(~SEC_PULSE_GEN_SEL_MASK),
+			   (unsigned int)(~SEC_PULSE_SEL_INNER));
+
+	regmap_update_bits(info->rtc_map, ANA_CALIB,
+			   (unsigned int)(~CALIB_SEL_FTUNE_MASK),
+			   CALIB_SEL_FTUNE_INNER);
+
+	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
+}
+
+static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
+	unsigned int sec;
+	unsigned int sec_ro_t;
+	unsigned long flag;
+
+	spin_lock_irqsave(&info->rtc_lock, flag);
+
+	regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec);
+	regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t);
+
+	if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) {
+		sec = sec_ro_t;
+		regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
+		regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);
+	}
+
+	spin_unlock_irqrestore(&info->rtc_lock, flag);
+
+	rtc_time64_to_tm(sec, tm);
+
+	return 0;
+}
+
+static int cv1800_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
+	unsigned long sec;
+	int ret;
+	unsigned long flag;
+
+	ret = rtc_valid_tm(tm);
+	if (ret)
+		return ret;
+
+	sec = rtc_tm_to_time64(tm);
+
+	spin_lock_irqsave(&info->rtc_lock, flag);
+
+	regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
+	regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);
+
+	regmap_write(info->rtc_map, MACRO_RG_SET_T, sec);
+
+	spin_unlock_irqrestore(&info->rtc_lock, flag);
+
+	return 0;
+}
+
+static irqreturn_t cv1800_rtc_irq_handler(int irq, void *dev_id)
+{
+	struct device *dev = dev_id;
+	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
+	struct rtc_wkalrm alrm;
+
+	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
+
+	rtc_read_alarm(info->rtc_dev, &alrm);
+	alrm.enabled = 0;
+	rtc_set_alarm(info->rtc_dev, &alrm);
+
+	return IRQ_HANDLED;
+}
+
+static const struct rtc_class_ops cv800b_rtc_ops = {
+	.read_time = cv1800_rtc_read_time,
+	.set_time = cv1800_rtc_set_time,
+	.read_alarm = cv1800_rtc_read_alarm,
+	.set_alarm = cv1800_rtc_set_alarm,
+	.alarm_irq_enable = cv1800_rtc_alarm_irq_enable,
+};
+
+static int cv1800_rtc_probe(struct platform_device *pdev)
+{
+	struct cv1800_rtc_priv *rtc;
+	uint32_t ctrl_val;
+	int ret;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv),
+			   GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	rtc->dev = &pdev->dev;
+
+	rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent);
+	if (IS_ERR(rtc->rtc_map))
+		return PTR_ERR(rtc->rtc_map);
+
+	rtc->irq = platform_get_irq(pdev, 0);
+	if (rtc->irq < 0)
+		return rtc->irq;
+
+	ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler,
+			       IRQF_TRIGGER_HIGH, "alarm", &pdev->dev);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "cannot register interrupt handler\n");
+
+	rtc->clk = devm_clk_get(rtc->dev, NULL);
+	if (IS_ERR(rtc->clk))
+		return PTR_ERR(rtc->clk);
+
+	rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(rtc->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk),
+				     "clk not found\n");
+
+	platform_set_drvdata(pdev, rtc);
+
+	spin_lock_init(&rtc->rtc_lock);
+
+	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev,
+								dev_name(&pdev->dev),
+								&cv800b_rtc_ops,
+								THIS_MODULE);
+	if (IS_ERR(rtc->rtc_dev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev),
+				     "can't register rtc device\n");
+
+	/* if use internal clk,so coarse calibrate rtc */
+	regmap_read(rtc->rtc_map, CTRL, &ctrl_val);
+	ctrl_val &= CTRL_MODE_MASK;
+
+	if (ctrl_val == CTRL_MODE_OSC32K) {
+		ret = cv1800_rtc_32k_coarse_val_calib(rtc);
+		if (ret)
+			dev_err(&pdev->dev, "failed to coarse RTC val !\n");
+
+		ret = cv1800_rtc_32k_fine_val_calib(rtc);
+		if (ret)
+			dev_err(&pdev->dev, "failed to fine RTC val !\n");
+	}
+
+	rtc_enable_sec_counter(rtc);
+
+	return 0;
+}
+
+static const struct of_device_id cv1800_dt_ids[] = {
+	{ .compatible = "sophgo,cv1800b-rtc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, cv1800_dt_ids);
+
+static struct platform_driver cv1800_rtc_driver = {
+	.driver = {
+			.name = "sophgo-cv800b-rtc",
+			.of_match_table = cv1800_dt_ids,
+		},
+	.probe = cv1800_rtc_probe,
+};
+
+module_platform_driver(cv1800_rtc_driver);
+MODULE_AUTHOR("Jingbao Qiu");
+MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC

[Alexandre Belloni]

Hello,

please run checkpatch.pl --strict, there are a few issues.

On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote:
> +struct cv1800_rtc_priv {
> +	struct rtc_device *rtc_dev;
> +	struct device *dev;
> +	struct regmap *rtc_map;
> +	struct clk *clk;
> +	spinlock_t rtc_lock;

This lock seems unnecessary, please check

> +	int irq;
> +};
> +
> +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +
> +	if (enabled)
> +		regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
> +	else
> +		regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> +

This could be:
	regmap_write(info->rtc_map, ALARM_ENABLE, enabled);

> +	return 0;
> +}
> +
> +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +	unsigned long alarm_time;
> +
> +	alarm_time = rtc_tm_to_time64(&alrm->time);
> +
> +	if (alarm_time > SEC_MAX_VAL)
> +		return -EINVAL;

The core is already checking fr this.

> +
> +	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> +
> +	udelay(DEALY_TIME_PREPARE);

Why is this needed?

> +
> +	regmap_write(info->rtc_map, ALARM_TIME, alarm_time);
> +	regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);

You must follow alrm->enabled instead of unconditionally enabling the
alarm.

> +
> +	return 0;
> +}
> +


> +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info)

Please explain those two calibration functions. I don't think you can
achieve what you want to do.

> +{
> +	uint32_t calib_val = 0;
> +	uint32_t coarse_val = 0;
> +	uint32_t time_now = 0;
> +	uint32_t time_next = 0;
> +	uint32_t offset = CALIB_OFFSET_INIT;
> +	uint32_t coarse_timeout = REG_INIT_TIMEOUT;
> +	uint32_t get_val_timeout = 0;
> +
> +	regmap_write(info->rtc_map, ANA_CALIB, CALIB_INIT_VAL);
> +
> +	udelay(DEALY_TIME_PREPARE);
> +
> +	/* Select 32K OSC tuning val source from rtc_sys */
> +	regmap_update_bits(info->rtc_map, SEC_PULSE_GEN,
> +			   (unsigned int)(~SEC_PULSE_GEN_SEL_MASK),
> +			   (unsigned int)(~SEC_PULSE_SEL_INNER));
> +
> +	regmap_read(info->rtc_map, ANA_CALIB, &calib_val);
> +
> +	regmap_write(info->rtc_map, FC_COARSE_EN, REG_ENABLE_FUN);
> +
> +	while (--coarse_timeout) {
> +		regmap_read(info->rtc_map, FC_COARSE_CAL, &time_now);
> +		time_now >>= FC_COARSE_CAL_TIME_SHIFT;
> +
> +		get_val_timeout = REG_INIT_TIMEOUT;
> +
> +		while (time_next <= time_now &&
> +		       --get_val_timeout) {
> +			regmap_read(info->rtc_map, FC_COARSE_CAL,
> +				    &time_next);
> +			time_next >>= FC_COARSE_CAL_TIME_SHIFT;
> +			udelay(DEALY_TIME_LOOP);
> +		}
> +
> +		if (!get_val_timeout)
> +			return -1;
> +
> +		udelay(DEALY_TIME_PREPARE);
> +
> +		regmap_read(info->rtc_map, FC_COARSE_CAL, &coarse_val);
> +		coarse_val &= FC_COARSE_CAL_VAL_MASK;
> +
> +		if (coarse_val > CALIB_FC_COARSE_PLUS_OFFSET) {
> +			calib_val += offset;
> +			offset >>= CALIB_OFFSET_SHIFT;
> +			regmap_write(info->rtc_map, ANA_CALIB,
> +				     calib_val);
> +		} else if (coarse_val < CALIB_FC_COARSE_SUB_OFFSET) {
> +			calib_val -= offset;
> +			offset >>= CALIB_OFFSET_SHIFT;
> +			regmap_write(info->rtc_map, ANA_CALIB,
> +				     calib_val);
> +		} else {
> +			regmap_write(info->rtc_map, FC_COARSE_EN,
> +				     REG_DISABLE_FUN);
> +			break;
> +		}
> +
> +		if (offset == 0)
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cv1800_rtc_32k_fine_val_calib(struct cv1800_rtc_priv *info)
> +{
> +	uint32_t val;
> +	uint64_t freq = CALIB_FREQ;
> +	uint32_t sec_cnt;
> +	uint32_t timeout = REG_INIT_TIMEOUT;
> +	uint32_t time_now = 0;
> +	uint32_t time_next = 0;
> +
> +	regmap_write(info->rtc_map, FC_FINE_EN, REG_ENABLE_FUN);
> +
> +	regmap_read(info->rtc_map, FC_FINE_CAL, &time_now);
> +	time_now >>= FC_FINE_CAL_TIME_SHIFT;
> +
> +	while (time_next <= time_now && --timeout) {
> +		regmap_read(info->rtc_map, FC_FINE_CAL, &time_next);
> +		time_next >>= FC_FINE_CAL_TIME_SHIFT;
> +		udelay(DEALY_TIME_LOOP);
> +	}
> +
> +	if (!timeout)
> +		return -1;
> +
> +	regmap_read(info->rtc_map, FC_FINE_CAL, &val);
> +	val &= FC_FINE_CAL_VAL_MASK;
> +
> +	do_div(freq, CALIB_FREQ_NS);
> +	freq = freq * CALIB_FRAC_EXT;
> +	do_div(freq, val);
> +
> +	sec_cnt = ((do_div(freq, CALIB_FRAC_EXT) * CALIB_FREQ_MULT) /
> +			   CALIB_FRAC_EXT &
> +		   SEC_PULSE_GEN_INT_MASK) +
> +		  (freq << SEC_PULSE_GEN_FRAC_SHIFT);
> +
> +	regmap_write(info->rtc_map, SEC_PULSE_GEN, sec_cnt);
> +	regmap_write(info->rtc_map, FC_FINE_EN, REG_DISABLE_FUN);
> +
> +	return 0;
> +}
> +
> +static void rtc_enable_sec_counter(struct cv1800_rtc_priv *info)
> +{
> +	/* select inner sec pulse and select reg set calibration val */
> +	regmap_update_bits(info->rtc_map, SEC_PULSE_GEN,
> +			   (unsigned int)(~SEC_PULSE_GEN_SEL_MASK),
> +			   (unsigned int)(~SEC_PULSE_SEL_INNER));
> +
> +	regmap_update_bits(info->rtc_map, ANA_CALIB,
> +			   (unsigned int)(~CALIB_SEL_FTUNE_MASK),
> +			   CALIB_SEL_FTUNE_INNER);
> +
> +	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);

Don't disable alarms on probe.

> +}
> +
> +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +	unsigned int sec;
> +	unsigned int sec_ro_t;
> +	unsigned long flag;
> +
> +	spin_lock_irqsave(&info->rtc_lock, flag);
> +
> +	regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec);
> +	regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t);
> +
> +	if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) {
> +		sec = sec_ro_t;
> +		regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
> +		regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);

What does this do?

> +	}
> +
> +	spin_unlock_irqrestore(&info->rtc_lock, flag);
> +
> +	rtc_time64_to_tm(sec, tm);
> +
> +	return 0;
> +}
> +
> +static int cv1800_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +	unsigned long sec;
> +	int ret;
> +	unsigned long flag;
> +
> +	ret = rtc_valid_tm(tm);

This is useless, tm will always be valid

> +	if (ret)
> +		return ret;
> +
> +	sec = rtc_tm_to_time64(tm);
> +
> +	spin_lock_irqsave(&info->rtc_lock, flag);
> +
> +	regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
> +	regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);
> +
> +	regmap_write(info->rtc_map, MACRO_RG_SET_T, sec);
> +
> +	spin_unlock_irqrestore(&info->rtc_lock, flag);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t cv1800_rtc_irq_handler(int irq, void *dev_id)
> +{
> +	struct device *dev = dev_id;
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +	struct rtc_wkalrm alrm;
> +
> +	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> +
> +	rtc_read_alarm(info->rtc_dev, &alrm);
> +	alrm.enabled = 0;
> +	rtc_set_alarm(info->rtc_dev, &alrm);

I don't get what you are doing here, you should call rtc_update_irq, not
mess with alarms that have been set.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct rtc_class_ops cv800b_rtc_ops = {
> +	.read_time = cv1800_rtc_read_time,
> +	.set_time = cv1800_rtc_set_time,
> +	.read_alarm = cv1800_rtc_read_alarm,
> +	.set_alarm = cv1800_rtc_set_alarm,
> +	.alarm_irq_enable = cv1800_rtc_alarm_irq_enable,
> +};
> +
> +static int cv1800_rtc_probe(struct platform_device *pdev)
> +{
> +	struct cv1800_rtc_priv *rtc;
> +	uint32_t ctrl_val;
> +	int ret;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv),
> +			   GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	rtc->dev = &pdev->dev;
> +
> +	rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent);
> +	if (IS_ERR(rtc->rtc_map))
> +		return PTR_ERR(rtc->rtc_map);
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0)
> +		return rtc->irq;
> +
> +	ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler,
> +			       IRQF_TRIGGER_HIGH, "alarm", &pdev->dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "cannot register interrupt handler\n");
> +
> +	rtc->clk = devm_clk_get(rtc->dev, NULL);
> +	if (IS_ERR(rtc->clk))
> +		return PTR_ERR(rtc->clk);
> +

You are going to leak rtc->clk after the next call.

> +	rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(rtc->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk),
> +				     "clk not found\n");
> +
> +	platform_set_drvdata(pdev, rtc);
> +
> +	spin_lock_init(&rtc->rtc_lock);
> +
> +	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev,
> +								dev_name(&pdev->dev),
> +								&cv800b_rtc_ops,
> +								THIS_MODULE);
> +	if (IS_ERR(rtc->rtc_dev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev),
> +				     "can't register rtc device\n");

Please use devm_rtc_allocate_device and devm_rtc_register_device

> +
> +	/* if use internal clk,so coarse calibrate rtc */
> +	regmap_read(rtc->rtc_map, CTRL, &ctrl_val);
> +	ctrl_val &= CTRL_MODE_MASK;
> +
> +	if (ctrl_val == CTRL_MODE_OSC32K) {
> +		ret = cv1800_rtc_32k_coarse_val_calib(rtc);
> +		if (ret)
> +			dev_err(&pdev->dev, "failed to coarse RTC val !\n");
> +
> +		ret = cv1800_rtc_32k_fine_val_calib(rtc);
> +		if (ret)
> +			dev_err(&pdev->dev, "failed to fine RTC val !\n");
> +	}
> +
> +	rtc_enable_sec_counter(rtc);

I'm pretty sure you don't want to do that on every probe.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id cv1800_dt_ids[] = {
> +	{ .compatible = "sophgo,cv1800b-rtc" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, cv1800_dt_ids);
> +
> +static struct platform_driver cv1800_rtc_driver = {
> +	.driver = {
> +			.name = "sophgo-cv800b-rtc",
> +			.of_match_table = cv1800_dt_ids,
> +		},
> +	.probe = cv1800_rtc_probe,
> +};
> +
> +module_platform_driver(cv1800_rtc_driver);
> +MODULE_AUTHOR("Jingbao Qiu");
> +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC

[Jingbao Qiu]

On Tue, Dec 26, 2023 at 8:37 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> Hello,
>
> please run checkpatch.pl --strict, there are a few issues.
>
> On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote:
> > +struct cv1800_rtc_priv {
> > +     struct rtc_device *rtc_dev;
> > +     struct device *dev;
> > +     struct regmap *rtc_map;
> > +     struct clk *clk;
> > +     spinlock_t rtc_lock;
>
> This lock seems unnecessary, please check
>

you are right. I will fix it.

> > +     int irq;
> > +};
> > +
> > +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> > +{
> > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > +
> > +     if (enabled)
> > +             regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
> > +     else
> > +             regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> > +
>
> This could be:
>         regmap_write(info->rtc_map, ALARM_ENABLE, enabled);

you are right, i will fix it.

>
> > +     return 0;
> > +}
> > +
> > +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > +     unsigned long alarm_time;
> > +
> > +     alarm_time = rtc_tm_to_time64(&alrm->time);
> > +
> > +     if (alarm_time > SEC_MAX_VAL)
> > +             return -EINVAL;
>
> The core is already checking fr this.

Thanks, I will remove it.

>
> > +
> > +     regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> > +
> > +     udelay(DEALY_TIME_PREPARE);
>
> Why is this needed?

This doesn't seem to require waiting, I will check it.

>
> > +
> > +     regmap_write(info->rtc_map, ALARM_TIME, alarm_time);
> > +     regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
>
> You must follow alrm->enabled instead of unconditionally enabling the
> alarm.

ok,i will fix it.

>
> > +
> > +     return 0;
> > +}
> > +
>
>
> > +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info)
>
> Please explain those two calibration functions. I don't think you can
> achieve what you want to do.

The goal of these two calibration functions is to achieve calibration
of RTC time.
The code is written according to the data manual.

The calibration circuit uses 25MHz crystal clock to sample 32KHz
clock. In coarse
tune mode, the 25MHz crystal clock samples one 32KHz clock cycle period and
report the counting results.

the datasheet link:
Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf
page:195

>
> > +{
> > +     uint32_t calib_val = 0;
> > +     uint32_t coarse_val = 0;
> > +     uint32_t time_now = 0;
> > +     uint32_t time_next = 0;
> > +     uint32_t offset = CALIB_OFFSET_INIT;
> > +     uint32_t coarse_timeout = REG_INIT_TIMEOUT;
> > +     uint32_t get_val_timeout = 0;
> > +
> > +     regmap_write(info->rtc_map, ANA_CALIB, CALIB_INIT_VAL);
> > +
> > +     udelay(DEALY_TIME_PREPARE);
> > +
> > +     /* Select 32K OSC tuning val source from rtc_sys */
> > +     regmap_update_bits(info->rtc_map, SEC_PULSE_GEN,
> > +                        (unsigned int)(~SEC_PULSE_GEN_SEL_MASK),
> > +                        (unsigned int)(~SEC_PULSE_SEL_INNER));
> > +
> > +     regmap_read(info->rtc_map, ANA_CALIB, &calib_val);
> > +
> > +     regmap_write(info->rtc_map, FC_COARSE_EN, REG_ENABLE_FUN);
> > +
> > +     while (--coarse_timeout) {
> > +             regmap_read(info->rtc_map, FC_COARSE_CAL, &time_now);
> > +             time_now >>= FC_COARSE_CAL_TIME_SHIFT;
> > +
> > +             get_val_timeout = REG_INIT_TIMEOUT;
> > +
> > +             while (time_next <= time_now &&
> > +                    --get_val_timeout) {
> > +                     regmap_read(info->rtc_map, FC_COARSE_CAL,
> > +                                 &time_next);
> > +                     time_next >>= FC_COARSE_CAL_TIME_SHIFT;
> > +                     udelay(DEALY_TIME_LOOP);
> > +             }
> > +
> > +             if (!get_val_timeout)
> > +                     return -1;
> > +
> > +             udelay(DEALY_TIME_PREPARE);
> > +
> > +             regmap_read(info->rtc_map, FC_COARSE_CAL, &coarse_val);
> > +             coarse_val &= FC_COARSE_CAL_VAL_MASK;
> > +
> > +             if (coarse_val > CALIB_FC_COARSE_PLUS_OFFSET) {
> > +                     calib_val += offset;
> > +                     offset >>= CALIB_OFFSET_SHIFT;
> > +                     regmap_write(info->rtc_map, ANA_CALIB,
> > +                                  calib_val);
> > +             } else if (coarse_val < CALIB_FC_COARSE_SUB_OFFSET) {
> > +                     calib_val -= offset;
> > +                     offset >>= CALIB_OFFSET_SHIFT;
> > +                     regmap_write(info->rtc_map, ANA_CALIB,
> > +                                  calib_val);
> > +             } else {
> > +                     regmap_write(info->rtc_map, FC_COARSE_EN,
> > +                                  REG_DISABLE_FUN);
> > +                     break;
> > +             }
> > +
> > +             if (offset == 0)
> > +                     return -1;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int cv1800_rtc_32k_fine_val_calib(struct cv1800_rtc_priv *info)
> > +{
> > +     uint32_t val;
> > +     uint64_t freq = CALIB_FREQ;
> > +     uint32_t sec_cnt;
> > +     uint32_t timeout = REG_INIT_TIMEOUT;
> > +     uint32_t time_now = 0;
> > +     uint32_t time_next = 0;
> > +
> > +     regmap_write(info->rtc_map, FC_FINE_EN, REG_ENABLE_FUN);
> > +
> > +     regmap_read(info->rtc_map, FC_FINE_CAL, &time_now);
> > +     time_now >>= FC_FINE_CAL_TIME_SHIFT;
> > +
> > +     while (time_next <= time_now && --timeout) {
> > +             regmap_read(info->rtc_map, FC_FINE_CAL, &time_next);
> > +             time_next >>= FC_FINE_CAL_TIME_SHIFT;
> > +             udelay(DEALY_TIME_LOOP);
> > +     }
> > +
> > +     if (!timeout)
> > +             return -1;
> > +
> > +     regmap_read(info->rtc_map, FC_FINE_CAL, &val);
> > +     val &= FC_FINE_CAL_VAL_MASK;
> > +
> > +     do_div(freq, CALIB_FREQ_NS);
> > +     freq = freq * CALIB_FRAC_EXT;
> > +     do_div(freq, val);
> > +
> > +     sec_cnt = ((do_div(freq, CALIB_FRAC_EXT) * CALIB_FREQ_MULT) /
> > +                        CALIB_FRAC_EXT &
> > +                SEC_PULSE_GEN_INT_MASK) +
> > +               (freq << SEC_PULSE_GEN_FRAC_SHIFT);
> > +
> > +     regmap_write(info->rtc_map, SEC_PULSE_GEN, sec_cnt);
> > +     regmap_write(info->rtc_map, FC_FINE_EN, REG_DISABLE_FUN);
> > +
> > +     return 0;
> > +}
> > +
> > +static void rtc_enable_sec_counter(struct cv1800_rtc_priv *info)
> > +{
> > +     /* select inner sec pulse and select reg set calibration val */
> > +     regmap_update_bits(info->rtc_map, SEC_PULSE_GEN,
> > +                        (unsigned int)(~SEC_PULSE_GEN_SEL_MASK),
> > +                        (unsigned int)(~SEC_PULSE_SEL_INNER));
> > +
> > +     regmap_update_bits(info->rtc_map, ANA_CALIB,
> > +                        (unsigned int)(~CALIB_SEL_FTUNE_MASK),
> > +                        CALIB_SEL_FTUNE_INNER);
> > +
> > +     regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
>
> Don't disable alarms on probe.

thanks,I will fix it.

>
> > +}
> > +
> > +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > +     unsigned int sec;
> > +     unsigned int sec_ro_t;
> > +     unsigned long flag;
> > +
> > +     spin_lock_irqsave(&info->rtc_lock, flag);
> > +
> > +     regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec);
> > +     regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t);
> > +
> > +     if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) {
> > +             sec = sec_ro_t;
> > +             regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
> > +             regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);
>
> What does this do?

the sec_ro_t be considered to have high accuracy after calibration.
So every time read the time, update the RTC time.

>
> > +     }
> > +
> > +     spin_unlock_irqrestore(&info->rtc_lock, flag);
> > +
> > +     rtc_time64_to_tm(sec, tm);
> > +
> > +     return 0;
> > +}
> > +
> > +static int cv1800_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > +     unsigned long sec;
> > +     int ret;
> > +     unsigned long flag;
> > +
> > +     ret = rtc_valid_tm(tm);
>
> This is useless, tm will always be valid

ok,i will fix it.

>
> > +     if (ret)
> > +             return ret;
> > +
> > +     sec = rtc_tm_to_time64(tm);
> > +
> > +     spin_lock_irqsave(&info->rtc_lock, flag);
> > +
> > +     regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
> > +     regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);
> > +
> > +     regmap_write(info->rtc_map, MACRO_RG_SET_T, sec);
> > +
> > +     spin_unlock_irqrestore(&info->rtc_lock, flag);
> > +
> > +     return 0;
> > +}
> > +
> > +static irqreturn_t cv1800_rtc_irq_handler(int irq, void *dev_id)
> > +{
> > +     struct device *dev = dev_id;
> > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > +     struct rtc_wkalrm alrm;
> > +
> > +     regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> > +
> > +     rtc_read_alarm(info->rtc_dev, &alrm);
> > +     alrm.enabled = 0;
> > +     rtc_set_alarm(info->rtc_dev, &alrm);
>
> I don't get what you are doing here, you should call rtc_update_irq, not
> mess with alarms that have been set.

Thanks,I will fix it.

>
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static const struct rtc_class_ops cv800b_rtc_ops = {
> > +     .read_time = cv1800_rtc_read_time,
> > +     .set_time = cv1800_rtc_set_time,
> > +     .read_alarm = cv1800_rtc_read_alarm,
> > +     .set_alarm = cv1800_rtc_set_alarm,
> > +     .alarm_irq_enable = cv1800_rtc_alarm_irq_enable,
> > +};
> > +
> > +static int cv1800_rtc_probe(struct platform_device *pdev)
> > +{
> > +     struct cv1800_rtc_priv *rtc;
> > +     uint32_t ctrl_val;
> > +     int ret;
> > +
> > +     rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv),
> > +                        GFP_KERNEL);
> > +     if (!rtc)
> > +             return -ENOMEM;
> > +
> > +     rtc->dev = &pdev->dev;
> > +
> > +     rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent);
> > +     if (IS_ERR(rtc->rtc_map))
> > +             return PTR_ERR(rtc->rtc_map);
> > +
> > +     rtc->irq = platform_get_irq(pdev, 0);
> > +     if (rtc->irq < 0)
> > +             return rtc->irq;
> > +
> > +     ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler,
> > +                            IRQF_TRIGGER_HIGH, "alarm", &pdev->dev);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret,
> > +                                  "cannot register interrupt handler\n");
> > +
> > +     rtc->clk = devm_clk_get(rtc->dev, NULL);
> > +     if (IS_ERR(rtc->clk))
> > +             return PTR_ERR(rtc->clk);
> > +
>
> You are going to leak rtc->clk after the next call.

I will release him at the appropriate time. And add the remove
function to release.

>
> > +     rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > +     if (IS_ERR(rtc->clk))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk),
> > +                                  "clk not found\n");
> > +
> > +     platform_set_drvdata(pdev, rtc);
> > +
> > +     spin_lock_init(&rtc->rtc_lock);
> > +
> > +     rtc->rtc_dev = devm_rtc_device_register(&pdev->dev,
> > +                                                             dev_name(&pdev->dev),
> > +                                                             &cv800b_rtc_ops,
> > +                                                             THIS_MODULE);
> > +     if (IS_ERR(rtc->rtc_dev))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev),
> > +                                  "can't register rtc device\n");
>
> Please use devm_rtc_allocate_device and devm_rtc_register_device

ok,I will use it.

>
> > +
> > +     /* if use internal clk,so coarse calibrate rtc */
> > +     regmap_read(rtc->rtc_map, CTRL, &ctrl_val);
> > +     ctrl_val &= CTRL_MODE_MASK;
> > +
> > +     if (ctrl_val == CTRL_MODE_OSC32K) {
> > +             ret = cv1800_rtc_32k_coarse_val_calib(rtc);
> > +             if (ret)
> > +                     dev_err(&pdev->dev, "failed to coarse RTC val !\n");
> > +
> > +             ret = cv1800_rtc_32k_fine_val_calib(rtc);
> > +             if (ret)
> > +                     dev_err(&pdev->dev, "failed to fine RTC val !\n");
> > +     }
> > +
> > +     rtc_enable_sec_counter(rtc);
>
> I'm pretty sure you don't want to do that on every probe.

you are right,i will fix it.

>
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id cv1800_dt_ids[] = {
> > +     { .compatible = "sophgo,cv1800b-rtc" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, cv1800_dt_ids);
> > +
> > +static struct platform_driver cv1800_rtc_driver = {
> > +     .driver = {
> > +                     .name = "sophgo-cv800b-rtc",
> > +                     .of_match_table = cv1800_dt_ids,
> > +             },
> > +     .probe = cv1800_rtc_probe,
> > +};
> > +
> > +module_platform_driver(cv1800_rtc_driver);
> > +MODULE_AUTHOR("Jingbao Qiu");
> > +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Best regards,
Jingbao Qiu

Re: [PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC

[Alexandre Belloni]

On 27/12/2023 16:03:56+0800, Jingbao Qiu wrote:
> On Tue, Dec 26, 2023 at 8:37 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > Hello,
> >
> > please run checkpatch.pl --strict, there are a few issues.
> >
> > On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote:
> > > +struct cv1800_rtc_priv {
> > > +     struct rtc_device *rtc_dev;
> > > +     struct device *dev;
> > > +     struct regmap *rtc_map;
> > > +     struct clk *clk;
> > > +     spinlock_t rtc_lock;
> >
> > This lock seems unnecessary, please check
> >
> 
> you are right. I will fix it.
> 
> > > +     int irq;
> > > +};
> > > +
> > > +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> > > +{
> > > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > > +
> > > +     if (enabled)
> > > +             regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
> > > +     else
> > > +             regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> > > +
> >
> > This could be:
> >         regmap_write(info->rtc_map, ALARM_ENABLE, enabled);
> 
> you are right, i will fix it.
> 
> >
> > > +     return 0;
> > > +}
> > > +
> > > +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > > +{
> > > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > > +     unsigned long alarm_time;
> > > +
> > > +     alarm_time = rtc_tm_to_time64(&alrm->time);
> > > +
> > > +     if (alarm_time > SEC_MAX_VAL)
> > > +             return -EINVAL;
> >
> > The core is already checking fr this.
> 
> Thanks, I will remove it.
> 
> >
> > > +
> > > +     regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> > > +
> > > +     udelay(DEALY_TIME_PREPARE);
> >
> > Why is this needed?
> 
> This doesn't seem to require waiting, I will check it.
> 
> >
> > > +
> > > +     regmap_write(info->rtc_map, ALARM_TIME, alarm_time);
> > > +     regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
> >
> > You must follow alrm->enabled instead of unconditionally enabling the
> > alarm.
> 
> ok,i will fix it.
> 
> >
> > > +
> > > +     return 0;
> > > +}
> > > +
> >
> >
> > > +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info)
> >
> > Please explain those two calibration functions. I don't think you can
> > achieve what you want to do.
> 
> The goal of these two calibration functions is to achieve calibration
> of RTC time.
> The code is written according to the data manual.
> 
> The calibration circuit uses 25MHz crystal clock to sample 32KHz
> clock. In coarse
> tune mode, the 25MHz crystal clock samples one 32KHz clock cycle period and
> report the counting results.
> 
> the datasheet link:
> Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf
> page:195


I'm really curious as to why this is calibrated using a 25MHz crystal as
it may be as imprecise as the 32kHz one. I'm asking because we have an
interface to get calibration done properly so you can use a precise clock
like GPS, NTP or PTP. This is what you should probably implement
instead or on top of it.

> >
> > > +}
> > > +
> > > +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > > +     unsigned int sec;
> > > +     unsigned int sec_ro_t;
> > > +     unsigned long flag;
> > > +
> > > +     spin_lock_irqsave(&info->rtc_lock, flag);
> > > +
> > > +     regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec);
> > > +     regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t);
> > > +
> > > +     if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) {
> > > +             sec = sec_ro_t;
> > > +             regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
> > > +             regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);
> >
> > What does this do?
> 
> the sec_ro_t be considered to have high accuracy after calibration.
> So every time read the time, update the RTC time.

So why don't you always use sec_ro_t instead of sec?
Also, why is this done conditionally on a arbitrary value? As it stands,
it will happen if the date is after 1995-07-09T16:12:48 for no good
reason.
This is awful because the alarm is matching SEC_CNTR_VAL with ALARM_TIME
so if this means the calibration doesn't affect SEC_CNTR_VAL (which I
seriously doubt), the alarm will end up being imprecise anyway

> > > +static int cv1800_rtc_probe(struct platform_device *pdev)
> > > +{
> > > +     struct cv1800_rtc_priv *rtc;
> > > +     uint32_t ctrl_val;
> > > +     int ret;
> > > +
> > > +     rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv),
> > > +                        GFP_KERNEL);
> > > +     if (!rtc)
> > > +             return -ENOMEM;
> > > +
> > > +     rtc->dev = &pdev->dev;
> > > +
> > > +     rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent);
> > > +     if (IS_ERR(rtc->rtc_map))
> > > +             return PTR_ERR(rtc->rtc_map);
> > > +
> > > +     rtc->irq = platform_get_irq(pdev, 0);
> > > +     if (rtc->irq < 0)
> > > +             return rtc->irq;
> > > +
> > > +     ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler,
> > > +                            IRQF_TRIGGER_HIGH, "alarm", &pdev->dev);
> > > +     if (ret)
> > > +             return dev_err_probe(&pdev->dev, ret,
> > > +                                  "cannot register interrupt handler\n");
> > > +
> > > +     rtc->clk = devm_clk_get(rtc->dev, NULL);
> > > +     if (IS_ERR(rtc->clk))
> > > +             return PTR_ERR(rtc->clk);
> > > +
> >
> > You are going to leak rtc->clk after the next call.
> 
> I will release him at the appropriate time. And add the remove
> function to release.
> 
> >
> > > +     rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > > +     if (IS_ERR(rtc->clk))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk),
> > > +                                  "clk not found\n");
> > > +
> > > +     platform_set_drvdata(pdev, rtc);
> > > +
> > > +     spin_lock_init(&rtc->rtc_lock);
> > > +
> > > +     rtc->rtc_dev = devm_rtc_device_register(&pdev->dev,
> > > +                                                             dev_name(&pdev->dev),
> > > +                                                             &cv800b_rtc_ops,
> > > +                                                             THIS_MODULE);
> > > +     if (IS_ERR(rtc->rtc_dev))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev),
> > > +                                  "can't register rtc device\n");
> >
> > Please use devm_rtc_allocate_device and devm_rtc_register_device
> 
> ok,I will use it.

Also you have to set the RTC range properly.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC

[Jingbao Qiu]

On Wed, Dec 27, 2023 at 9:50 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 27/12/2023 16:03:56+0800, Jingbao Qiu wrote:
> > On Tue, Dec 26, 2023 at 8:37 PM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > >
> > > Hello,
> > >
> > > please run checkpatch.pl --strict, there are a few issues.
> > >
> > > On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote:
> > > > +struct cv1800_rtc_priv {
> > > > +     struct rtc_device *rtc_dev;
> > > > +     struct device *dev;
> > > > +     struct regmap *rtc_map;
> > > > +     struct clk *clk;
> > > > +     spinlock_t rtc_lock;
> > >
> > > This lock seems unnecessary, please check
> > >
> >
> > you are right. I will fix it.
> >
> > > > +     int irq;
> > > > +};
> > > > +
> > > > +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> > > > +{
> > > > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > > > +
> > > > +     if (enabled)
> > > > +             regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
> > > > +     else
> > > > +             regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> > > > +
> > >
> > > This could be:
> > >         regmap_write(info->rtc_map, ALARM_ENABLE, enabled);
> >
> > you are right, i will fix it.
> >
> > >
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > > > +{
> > > > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > > > +     unsigned long alarm_time;
> > > > +
> > > > +     alarm_time = rtc_tm_to_time64(&alrm->time);
> > > > +
> > > > +     if (alarm_time > SEC_MAX_VAL)
> > > > +             return -EINVAL;
> > >
> > > The core is already checking fr this.
> >
> > Thanks, I will remove it.
> >
> > >
> > > > +
> > > > +     regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> > > > +
> > > > +     udelay(DEALY_TIME_PREPARE);
> > >
> > > Why is this needed?
> >
> > This doesn't seem to require waiting, I will check it.
> >
> > >
> > > > +
> > > > +     regmap_write(info->rtc_map, ALARM_TIME, alarm_time);
> > > > +     regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
> > >
> > > You must follow alrm->enabled instead of unconditionally enabling the
> > > alarm.
> >
> > ok,i will fix it.
> >
> > >
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > >
> > >
> > > > +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info)
> > >
> > > Please explain those two calibration functions. I don't think you can
> > > achieve what you want to do.
> >
> > The goal of these two calibration functions is to achieve calibration
> > of RTC time.
> > The code is written according to the data manual.
> >
> > The calibration circuit uses 25MHz crystal clock to sample 32KHz
> > clock. In coarse
> > tune mode, the 25MHz crystal clock samples one 32KHz clock cycle period and
> > report the counting results.
> >
> > the datasheet link:
> > Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf
> > page:195
>
>
> I'm really curious as to why this is calibrated using a 25MHz crystal as
> it may be as imprecise as the 32kHz one. I'm asking because we have an
> interface to get calibration done properly so you can use a precise clock
> like GPS, NTP or PTP. This is what you should probably implement
> instead or on top of it.
>

Thanks, I will communicate with the IC designers later.

> > >
> > > > +}
> > > > +
> > > > +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > > +{
> > > > +     struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> > > > +     unsigned int sec;
> > > > +     unsigned int sec_ro_t;
> > > > +     unsigned long flag;
> > > > +
> > > > +     spin_lock_irqsave(&info->rtc_lock, flag);
> > > > +
> > > > +     regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec);
> > > > +     regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t);
> > > > +
> > > > +     if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) {
> > > > +             sec = sec_ro_t;
> > > > +             regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
> > > > +             regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);
> > >
> > > What does this do?
> >
> > the sec_ro_t be considered to have high accuracy after calibration.
> > So every time read the time, update the RTC time.
>
> So why don't you always use sec_ro_t instead of sec?
> Also, why is this done conditionally on a arbitrary value? As it stands,
> it will happen if the date is after 1995-07-09T16:12:48 for no good
> reason.
> This is awful because the alarm is matching SEC_CNTR_VAL with ALARM_TIME
> so if this means the calibration doesn't affect SEC_CNTR_VAL (which I
> seriously doubt), the alarm will end up being imprecise anyway
>

Thanks, I will communicate with the IC designers later.

> > > > +static int cv1800_rtc_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct cv1800_rtc_priv *rtc;
> > > > +     uint32_t ctrl_val;
> > > > +     int ret;
> > > > +
> > > > +     rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv),
> > > > +                        GFP_KERNEL);
> > > > +     if (!rtc)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     rtc->dev = &pdev->dev;
> > > > +
> > > > +     rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent);
> > > > +     if (IS_ERR(rtc->rtc_map))
> > > > +             return PTR_ERR(rtc->rtc_map);
> > > > +
> > > > +     rtc->irq = platform_get_irq(pdev, 0);
> > > > +     if (rtc->irq < 0)
> > > > +             return rtc->irq;
> > > > +
> > > > +     ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler,
> > > > +                            IRQF_TRIGGER_HIGH, "alarm", &pdev->dev);
> > > > +     if (ret)
> > > > +             return dev_err_probe(&pdev->dev, ret,
> > > > +                                  "cannot register interrupt handler\n");
> > > > +
> > > > +     rtc->clk = devm_clk_get(rtc->dev, NULL);
> > > > +     if (IS_ERR(rtc->clk))
> > > > +             return PTR_ERR(rtc->clk);
> > > > +
> > >
> > > You are going to leak rtc->clk after the next call.
> >
> > I will release him at the appropriate time. And add the remove
> > function to release.
> >
> > >
> > > > +     rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > > > +     if (IS_ERR(rtc->clk))
> > > > +             return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk),
> > > > +                                  "clk not found\n");
> > > > +
> > > > +     platform_set_drvdata(pdev, rtc);
> > > > +
> > > > +     spin_lock_init(&rtc->rtc_lock);
> > > > +
> > > > +     rtc->rtc_dev = devm_rtc_device_register(&pdev->dev,
> > > > +                                                             dev_name(&pdev->dev),
> > > > +                                                             &cv800b_rtc_ops,
> > > > +                                                             THIS_MODULE);
> > > > +     if (IS_ERR(rtc->rtc_dev))
> > > > +             return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev),
> > > > +                                  "can't register rtc device\n");
> > >
> > > Please use devm_rtc_allocate_device and devm_rtc_register_device
> >
> > ok,I will use it.
>
> Also you have to set the RTC range properly.

Thanks,I will fix it.

Best regards,
Jingbao Qiu

>
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[PATCH v3 4/4] riscv: dts: sophgo: add rtc dt node for CV1800

[Jingbao Qiu]

Add the rtc device tree node to cv1800 SoC.

Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
---
 arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
index df40e87ee063..da9c42ef6fd4 100644
--- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
@@ -114,6 +114,17 @@ plic: interrupt-controller@70000000 {
 			riscv,ndev = <101>;
 		};
 
+		syscon@5025000 {
+			compatible = "sophgo,cv1800b-subsys", "syscon", "simple-mfd";
+			reg = <0x05025000 0x2000>;
+
+			rtc {
+				compatible = "sophgo,cv1800b-rtc";
+				interrupts = <17 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk CLK_RTC_25M>;
+			};
+		};
+
 		clint: timer@74000000 {
 			compatible = "sophgo,cv1800b-clint", "thead,c900-clint";
 			reg = <0x74000000 0x10000>;
-- 
2.25.1