[PATCH V4 0/5] rtc: Add rtc driver for the Loongson family chips

[PATCH V4 0/5] rtc: Add rtc driver for the Loongson family chips

[Binbin Zhou]

Hi all:

The initial DT-base ls2x rtc driver was written by Wang Xuerui, He has
released five versions of patchset before, and all related mail records
are shown below if you are interested:

https://lore.kernel.org/all/?q=ls2x-rtc

In this series of patches, based on the code above, I have added the
following support:

1. Add ACPI-related support, as Loongson-3A5000 + LS7A is now ACPI-base
   by default under LoongArch architecture;
2. Add rtc alarm/walarm related functions;
3. Merge LS1X rtc and LS2X rtc into a unified rtc-loongson driver.

I have tested on Loongson-3A5000LA+LS7A1000/LS7A2000, Loongson-2K1000LA,
Loongson-2K0500 and Loongson-2K2000(ACPI/DT).

Thanks to everyone for reviewing and testing the code.

-------
v4:
- Rebase on the top of rtc-next;
- Drop defconfig-related patches;
patch(1/5)
  - New patch, Loongson RTC bindings have been rewritten and we have
    dropped the wildcard (ls2x) in compatible.
    Thanks to Krzysztof for your comments;
patch(2/5)
  - New patch, drop the ls1x rtc driver;
patch(3/5)
  - Clear RTC_FEATURE_UPDATE_INTERRUPT bit, for the rtc does not support
    UIE;
  - Add LS2K2000 support(DT/ACPI);
  - Merge ls1x support and name the driver rtc-loongson;
    Thanks to Jiaxun for his comments and Keguang for his assistance in
testing.

v3:
https://lore.kernel.org/linux-rtc/cover.1681370153.git.zhoubinbin@loongson.cn/
patch(2/7):
 - Check patchset with "checkpatch.pl --strict";
 - Drop static inline functions which are used only once, such as
   ls2x_rtc_regs_to_time;
 - Remove the suruct ls2x_rtc_regs and regmap_bulk_xxx() is used to read
   and write rtc registers;
 - Clear the RTC wakeup interrupt manually;
 - Enable the RTC in set_time() and check in read_time();
 - device_get_match_data() is used to get ls2x_pm_offset, for LS2k1000
   has the different value;
 - Remove some inexact CONFIG_ACPI.

v2:
1. Rebased on top of latest loongarch-next;
2. Add interrupt descriptions to the ls2k and ls7a DTS files to avoid
errors when the driver gets the IRQ number, Thanks to Qing Zhang for
testing;
3. Remove some inexact CONFIG_ACPI.

Thanks.

Binbin Zhou (5):
  dt-bindings: rtc: Remove the LS2X from the trivial RTCs
  rtc: Remove the Loongson-1 RTC driver
  rtc: Add rtc driver for the Loongson family chips
  MIPS: Loongson64: DTS: Add RTC support to LS7A PCH
  MIPS: Loongson64: DTS: Add RTC support to Loongson-2K1000

 .../devicetree/bindings/rtc/loongson,rtc.yaml |  47 +++
 .../devicetree/bindings/rtc/trivial-rtc.yaml  |   2 -
 .../boot/dts/loongson/loongson64-2k1000.dtsi  |   7 +
 arch/mips/boot/dts/loongson/ls7a-pch.dtsi     |   7 +
 drivers/rtc/Kconfig                           |  23 +-
 drivers/rtc/Makefile                          |   2 +-
 drivers/rtc/rtc-loongson.c                    | 390 ++++++++++++++++++
 drivers/rtc/rtc-ls1x.c                        | 192 ---------
 8 files changed, 465 insertions(+), 205 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
 create mode 100644 drivers/rtc/rtc-loongson.c
 delete mode 100644 drivers/rtc/rtc-ls1x.c

-- 
2.39.1

[PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Binbin Zhou]

Move Loongson RTC bindings from trivial-rtc.yaml into loongson,rtc.yaml.

Also, we will discard the use of wildcards in compatible (ls2x-rtc),
soc-based compatible is more accurate for hardware differences between
chips.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../devicetree/bindings/rtc/loongson,rtc.yaml | 47 +++++++++++++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
 2 files changed, 47 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/loongson,rtc.yaml

diff --git a/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml b/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
new file mode 100644
index 000000000000..68e56829e390
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/loongson,rtc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson Real-Time Clock
+
+maintainers:
+  - Binbin Zhou <zhoubinbin@loongson.cn>
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    enum:
+      - loongson,ls1b-rtc
+      - loongson,ls1c-rtc
+      - loongson,ls7a-rtc
+      - loongson,ls2k0500-rtc
+      - loongson,ls2k1000-rtc
+      - loongson,ls2k2000-rtc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    rtc_dev: rtc@1fe27800 {
+      compatible = "loongson,ls2k0500-rtc";
+      reg = <0x1fe27800 0x100>;
+
+      interrupt-parent = <&liointc1>;
+      interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
index a3603e638c37..9af77f21bb7f 100644
--- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
@@ -47,8 +47,6 @@ properties:
       - isil,isl1218
       # Intersil ISL12022 Real-time Clock
       - isil,isl12022
-      # Loongson-2K Socs/LS7A bridge Real-time Clock
-      - loongson,ls2x-rtc
       # Real Time Clock Module with I2C-Bus
       - microcrystal,rv3029
       # Real Time Clock
-- 
2.39.1

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Conor Dooley]

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

Hey Binbin,

On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:
> Move Loongson RTC bindings from trivial-rtc.yaml into loongson,rtc.yaml.
> 
> Also, we will discard the use of wildcards in compatible (ls2x-rtc),
> soc-based compatible is more accurate for hardware differences between
> chips.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  .../devicetree/bindings/rtc/loongson,rtc.yaml | 47 +++++++++++++++++++
>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>  2 files changed, 47 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml b/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> new file mode 100644
> index 000000000000..68e56829e390
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/loongson,rtc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson Real-Time Clock
> +
> +maintainers:
> +  - Binbin Zhou <zhoubinbin@loongson.cn>
> +
> +allOf:
> +  - $ref: rtc.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - loongson,ls1b-rtc
> +      - loongson,ls1c-rtc
> +      - loongson,ls7a-rtc
> +      - loongson,ls2k0500-rtc
> +      - loongson,ls2k1000-rtc
> +      - loongson,ls2k2000-rtc

|+static const struct of_device_id loongson_rtc_of_match[] = {
|+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
|+       { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
|+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
|+       { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
|+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
|+       { .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
|+       { /* sentinel */ }
|+};

This is a sign to me that your compatibles here are could do with some
fallbacks. Both of the ls1 ones are compatible with each other & there
are three that are generic.

I would allow the following:
"loongson,ls1b-rtc"
"loongson,ls1c-rtc", "loongson,ls1b-rtc"
"loongson,ls7a-rtc"
"loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
"loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
"loongson,ls2k1000-rtc"

And then the driver only needs:
|+static const struct of_device_id loongson_rtc_of_match[] = {
|+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
|+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
|+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
|+       { /* sentinel */ }
|+};

And ~if~when you add support for more devices in the future that are
compatible with the existing ones no code changes are required.

To maintain compatibility with the existing devicetrees, should the old
"loongson,ls2x-rtc" be kept in the driver?

Thanks,
Conor.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    rtc_dev: rtc@1fe27800 {
> +      compatible = "loongson,ls2k0500-rtc";
> +      reg = <0x1fe27800 0x100>;
> +
> +      interrupt-parent = <&liointc1>;
> +      interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> index a3603e638c37..9af77f21bb7f 100644
> --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> @@ -47,8 +47,6 @@ properties:
>        - isil,isl1218
>        # Intersil ISL12022 Real-time Clock
>        - isil,isl12022
> -      # Loongson-2K Socs/LS7A bridge Real-time Clock
> -      - loongson,ls2x-rtc
>        # Real Time Clock Module with I2C-Bus
>        - microcrystal,rv3029
>        # Real Time Clock
> -- 
> 2.39.1
> 

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

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Binbin Zhou]

On Fri, May 26, 2023 at 1:05 AM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Binbin,
>
> On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:
> > Move Loongson RTC bindings from trivial-rtc.yaml into loongson,rtc.yaml.
> >
> > Also, we will discard the use of wildcards in compatible (ls2x-rtc),
> > soc-based compatible is more accurate for hardware differences between
> > chips.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  .../devicetree/bindings/rtc/loongson,rtc.yaml | 47 +++++++++++++++++++
> >  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
> >  2 files changed, 47 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml b/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> > new file mode 100644
> > index 000000000000..68e56829e390
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/rtc/loongson,rtc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Loongson Real-Time Clock
> > +
> > +maintainers:
> > +  - Binbin Zhou <zhoubinbin@loongson.cn>
> > +
> > +allOf:
> > +  - $ref: rtc.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - loongson,ls1b-rtc
> > +      - loongson,ls1c-rtc
> > +      - loongson,ls7a-rtc
> > +      - loongson,ls2k0500-rtc
> > +      - loongson,ls2k1000-rtc
> > +      - loongson,ls2k2000-rtc
>
> |+static const struct of_device_id loongson_rtc_of_match[] = {
> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> |+       { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> |+       { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> |+       { .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
> |+       { /* sentinel */ }
> |+};
>
> This is a sign to me that your compatibles here are could do with some
> fallbacks. Both of the ls1 ones are compatible with each other & there
> are three that are generic.
>
> I would allow the following:
> "loongson,ls1b-rtc"
> "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> "loongson,ls7a-rtc"
> "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
> "loongson,ls2k1000-rtc"
>
> And then the driver only needs:
> |+static const struct of_device_id loongson_rtc_of_match[] = {
> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> |+       { /* sentinel */ }
> |+};
>
> And ~if~when you add support for more devices in the future that are
> compatible with the existing ones no code changes are required.

Hi Conor:

Thanks for your reply.

Yes, this is looking much cleaner. But it can't show every chip that
supports that driver.

As we know, Loongson is a family of chips:
ls1b/ls1c represent the Loongson-1 family of CPU chips;
ls7a represents the Loongson LS7A bridge chip;
ls2k0500/ls2k1000/ls2k2000 represent the Loongson-2 family of CPU chips.

Based on my previous conversations with Krzysztof, it seems that
soc-based to order compatible is more popular, so I have listed all
the chips that support that RTC driver.

>
> To maintain compatibility with the existing devicetrees, should the old
> "loongson,ls2x-rtc" be kept in the driver?

No, It seems that wildcards in compatible are not allowed."
loongson,ls2x-rtc" itself was part of this patch series at one time,
but apparently it is not the right way to describe these chips.

Here is Krzysztof's previous reply:
https://lore.kernel.org/linux-rtc/05ebf834-2220-d1e6-e07a-529b8f9cb100@linaro.org/

Thanks.
Binbin

>
> Thanks,
> Conor.
>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    rtc_dev: rtc@1fe27800 {
> > +      compatible = "loongson,ls2k0500-rtc";
> > +      reg = <0x1fe27800 0x100>;
> > +
> > +      interrupt-parent = <&liointc1>;
> > +      interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
> > +    };
> > +
> > +...
> > diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > index a3603e638c37..9af77f21bb7f 100644
> > --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > @@ -47,8 +47,6 @@ properties:
> >        - isil,isl1218
> >        # Intersil ISL12022 Real-time Clock
> >        - isil,isl12022
> > -      # Loongson-2K Socs/LS7A bridge Real-time Clock
> > -      - loongson,ls2x-rtc
> >        # Real Time Clock Module with I2C-Bus
> >        - microcrystal,rv3029
> >        # Real Time Clock
> > --
> > 2.39.1
> >

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Conor Dooley]

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

On Fri, May 26, 2023 at 09:37:02AM +0800, Binbin Zhou wrote:
> On Fri, May 26, 2023 at 1:05 AM Conor Dooley <conor@kernel.org> wrote:
> > On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:

>> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - loongson,ls1b-rtc
> > > +      - loongson,ls1c-rtc
> > > +      - loongson,ls7a-rtc
> > > +      - loongson,ls2k0500-rtc
> > > +      - loongson,ls2k1000-rtc
> > > +      - loongson,ls2k2000-rtc
> >
> > |+static const struct of_device_id loongson_rtc_of_match[] = {
> > |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> > |+       { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
> > |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> > |+       { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
> > |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> > |+       { .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
> > |+       { /* sentinel */ }
> > |+};
> >
> > This is a sign to me that your compatibles here are could do with some
> > fallbacks. Both of the ls1 ones are compatible with each other & there
> > are three that are generic.
> >
> > I would allow the following:
> > "loongson,ls1b-rtc"
> > "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> > "loongson,ls7a-rtc"
> > "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> > "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
> > "loongson,ls2k1000-rtc"
> >
> > And then the driver only needs:
> > |+static const struct of_device_id loongson_rtc_of_match[] = {
> > |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> > |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> > |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> > |+       { /* sentinel */ }
> > |+};
> >
> > And ~if~when you add support for more devices in the future that are
> > compatible with the existing ones no code changes are required.
> 
> Hi Conor:
> 
> Thanks for your reply.
> 
> Yes, this is looking much cleaner. But it can't show every chip that
> supports that driver.
> 
> As we know, Loongson is a family of chips:
> ls1b/ls1c represent the Loongson-1 family of CPU chips;
> ls7a represents the Loongson LS7A bridge chip;
> ls2k0500/ls2k1000/ls2k2000 represent the Loongson-2 family of CPU chips.
> 
> Based on my previous conversations with Krzysztof, it seems that
> soc-based to order compatible is more popular, so I have listed all
> the chips that support that RTC driver.

Right. You don't actually have to list them all *in the driver* though,
just in the binding and in the devicetree. I think what you have missed
is:
> > I would allow the following:
> > "loongson,ls1b-rtc"
> > "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> > "loongson,ls7a-rtc"
> > "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> > "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
> > "loongson,ls2k1000-rtc"

This is what you would put in the compatible section of a devicetree
node, using "fallback compatibles". So for a ls1c you put in
compatible = "loongson,ls1c-rtc", "loongson,ls1b-rtc";
and the kernel first tries to find a driver that supports
"loongson,ls1c-rtc" but if that fails it tries to find one that supports
"loongson,ls1b-rtc". This gives you the best of both worlds - you can
add support easily for new systems (when an ls1d comes out, you don't
even need to change the driver for it to just work!) and you have a
soc-specific compatible in case you need to add some workaround for
hardware errata etc in the future.

> > To maintain compatibility with the existing devicetrees, should the old
> > "loongson,ls2x-rtc" be kept in the driver?
> 
> No, It seems that wildcards in compatible are not allowed."
> loongson,ls2x-rtc" itself was part of this patch series at one time,
> but apparently it is not the right way to describe these chips.

Right, but it has been merged - you are deleting the driver that supports
it after all - which means that any dtb with the old compatible will
stop working.
I don't disagree with Krzysztof that having wildcard based compatibles
is bad, but I do not think that regressing rtc support for systems with
these old devicetrees is the right way to go either.

Thanks,
Conor.

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

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Jiaxun Yang]

> 2023年5月26日 13:06，Conor Dooley <conor.dooley@microchip.com> 写道：
Hi all,

[...]
My two cents here as Loongson64 maintainer.

> 
>>> To maintain compatibility with the existing devicetrees, should the old
>>> "loongson,ls2x-rtc" be kept in the driver?
>> 
>> No, It seems that wildcards in compatible are not allowed."
>> loongson,ls2x-rtc" itself was part of this patch series at one time,
>> but apparently it is not the right way to describe these chips.
> 
> Right, but it has been merged - you are deleting the driver that supports
> it after all - which means that any dtb with the old compatible will
> stop working.
It is perfectly fine to break DTB compatibility for Loongson64 systems
As they *only* use builtin dtb. Bootloader will only pass machine type information
and kernel will choose one dtb from it’s dtbs pool.

Thanks
- Jiaxun

> I don't disagree with Krzysztof that having wildcard based compatibles
> is bad, but I do not think that regressing rtc support for systems with
> these old devicetrees is the right way to go either.
> 
> Thanks,
> Conor.

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Conor Dooley]

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

On Fri, May 26, 2023 at 01:22:15PM +0100, Jiaxun Yang wrote:
> 
> 
> > 2023年5月26日 13:06，Conor Dooley <conor.dooley@microchip.com> 写道：
> Hi all,
> 
> [...]
> My two cents here as Loongson64 maintainer.
> 
> > 
> >>> To maintain compatibility with the existing devicetrees, should the old
> >>> "loongson,ls2x-rtc" be kept in the driver?
> >> 
> >> No, It seems that wildcards in compatible are not allowed."
> >> loongson,ls2x-rtc" itself was part of this patch series at one time,
> >> but apparently it is not the right way to describe these chips.
> > 
> > Right, but it has been merged - you are deleting the driver that supports
> > it after all - which means that any dtb with the old compatible will
> > stop working.
> It is perfectly fine to break DTB compatibility for Loongson64 systems
> As they *only* use builtin dtb. Bootloader will only pass machine type information
> and kernel will choose one dtb from it’s dtbs pool.

Ah, that is good to know thanks! I think that should be mentioned in the
commit messages for the next revision.

Cheers,
Conor.

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

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Binbin Zhou]

On Fri, May 26, 2023 at 8:07 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Fri, May 26, 2023 at 09:37:02AM +0800, Binbin Zhou wrote:
> > On Fri, May 26, 2023 at 1:05 AM Conor Dooley <conor@kernel.org> wrote:
> > > On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:
>
> >> > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - loongson,ls1b-rtc
> > > > +      - loongson,ls1c-rtc
> > > > +      - loongson,ls7a-rtc
> > > > +      - loongson,ls2k0500-rtc
> > > > +      - loongson,ls2k1000-rtc
> > > > +      - loongson,ls2k2000-rtc
> > >
> > > |+static const struct of_device_id loongson_rtc_of_match[] = {
> > > |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> > > |+       { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
> > > |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> > > |+       { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
> > > |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> > > |+       { .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
> > > |+       { /* sentinel */ }
> > > |+};
> > >
> > > This is a sign to me that your compatibles here are could do with some
> > > fallbacks. Both of the ls1 ones are compatible with each other & there
> > > are three that are generic.
> > >
> > > I would allow the following:
> > > "loongson,ls1b-rtc"
> > > "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> > > "loongson,ls7a-rtc"
> > > "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> > > "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
> > > "loongson,ls2k1000-rtc"
> > >
> > > And then the driver only needs:
> > > |+static const struct of_device_id loongson_rtc_of_match[] = {
> > > |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> > > |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> > > |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> > > |+       { /* sentinel */ }
> > > |+};
> > >
> > > And ~if~when you add support for more devices in the future that are
> > > compatible with the existing ones no code changes are required.
> >
> > Hi Conor:
> >
> > Thanks for your reply.
> >
> > Yes, this is looking much cleaner. But it can't show every chip that
> > supports that driver.
> >
> > As we know, Loongson is a family of chips:
> > ls1b/ls1c represent the Loongson-1 family of CPU chips;
> > ls7a represents the Loongson LS7A bridge chip;
> > ls2k0500/ls2k1000/ls2k2000 represent the Loongson-2 family of CPU chips.
> >
> > Based on my previous conversations with Krzysztof, it seems that
> > soc-based to order compatible is more popular, so I have listed all
> > the chips that support that RTC driver.
>
> Right. You don't actually have to list them all *in the driver* though,
> just in the binding and in the devicetree. I think what you have missed
> is:
> > > I would allow the following:
> > > "loongson,ls1b-rtc"
> > > "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> > > "loongson,ls7a-rtc"
> > > "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> > > "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
> > > "loongson,ls2k1000-rtc"
>
> This is what you would put in the compatible section of a devicetree
> node, using "fallback compatibles". So for a ls1c you put in
> compatible = "loongson,ls1c-rtc", "loongson,ls1b-rtc";
> and the kernel first tries to find a driver that supports
> "loongson,ls1c-rtc" but if that fails it tries to find one that supports
> "loongson,ls1b-rtc". This gives you the best of both worlds - you can
> add support easily for new systems (when an ls1d comes out, you don't
> even need to change the driver for it to just work!) and you have a
> soc-specific compatible in case you need to add some workaround for
> hardware errata etc in the future.

Hi Conor:

I seem to understand what you are talking about.
I hadn't delved into "fallback compatibles" before, so thanks for the
detailed explanation.

In fact, I have thought before if there is a good way to do it other
than adding comptable to the driver frequently, and "fallback
compatibles" should be the most suitable.

So in the dt-bindings file, should we just write this:

  compatible:
    oneOf:
      - items:
          - enum:
              - loongson,ls1c-rtc
          - const: loongson,ls1b-rtc
      - items:
          - enum:
              - loongson,ls2k0500-rtc
              - loongson,ls2k2000-rtc
          - const: loongson,ls7a-rtc
      - items:
          - const: loongson,ls2k1000-rtc

Thanks.
Binbin

>
> > > To maintain compatibility with the existing devicetrees, should the old
> > > "loongson,ls2x-rtc" be kept in the driver?
> >
> > No, It seems that wildcards in compatible are not allowed."
> > loongson,ls2x-rtc" itself was part of this patch series at one time,
> > but apparently it is not the right way to describe these chips.
>
> Right, but it has been merged - you are deleting the driver that supports
> it after all - which means that any dtb with the old compatible will
> stop working.
> I don't disagree with Krzysztof that having wildcard based compatibles
> is bad, but I do not think that regressing rtc support for systems with
> these old devicetrees is the right way to go either.
>
> Thanks,
> Conor.

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Jiaxun Yang]

> 2023年5月27日 10:22，Binbin Zhou <zhoubb.aaron@gmail.com> 写道：
> 
> On Fri, May 26, 2023 at 8:07 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>> 
>> On Fri, May 26, 2023 at 09:37:02AM +0800, Binbin Zhou wrote:
>>> On Fri, May 26, 2023 at 1:05 AM Conor Dooley <conor@kernel.org> wrote:
>>>> On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:
>> 
>>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - loongson,ls1b-rtc
>>>>> +      - loongson,ls1c-rtc
>>>>> +      - loongson,ls7a-rtc
>>>>> +      - loongson,ls2k0500-rtc
>>>>> +      - loongson,ls2k1000-rtc
>>>>> +      - loongson,ls2k2000-rtc
>>>> 
>>>> |+static const struct of_device_id loongson_rtc_of_match[] = {
>>>> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
>>>> |+       { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
>>>> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
>>>> |+       { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
>>>> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
>>>> |+       { .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
>>>> |+       { /* sentinel */ }
>>>> |+};
>>>> 
>>>> This is a sign to me that your compatibles here are could do with some
>>>> fallbacks. Both of the ls1 ones are compatible with each other & there
>>>> are three that are generic.
>>>> 
>>>> I would allow the following:
>>>> "loongson,ls1b-rtc"
>>>> "loongson,ls1c-rtc", "loongson,ls1b-rtc"
>>>> "loongson,ls7a-rtc"
>>>> "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
>>>> "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
>>>> "loongson,ls2k1000-rtc"
>>>> 
>>>> And then the driver only needs:
>>>> |+static const struct of_device_id loongson_rtc_of_match[] = {
>>>> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
>>>> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
>>>> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
>>>> |+       { /* sentinel */ }
>>>> |+};
>>>> 
>>>> And ~if~when you add support for more devices in the future that are
>>>> compatible with the existing ones no code changes are required.
>>> 
>>> Hi Conor:
>>> 
>>> Thanks for your reply.
>>> 
>>> Yes, this is looking much cleaner. But it can't show every chip that
>>> supports that driver.
>>> 
>>> As we know, Loongson is a family of chips:
>>> ls1b/ls1c represent the Loongson-1 family of CPU chips;
>>> ls7a represents the Loongson LS7A bridge chip;
>>> ls2k0500/ls2k1000/ls2k2000 represent the Loongson-2 family of CPU chips.
>>> 
>>> Based on my previous conversations with Krzysztof, it seems that
>>> soc-based to order compatible is more popular, so I have listed all
>>> the chips that support that RTC driver.
>> 
>> Right. You don't actually have to list them all *in the driver* though,
>> just in the binding and in the devicetree. I think what you have missed
>> is:
>>>> I would allow the following:
>>>> "loongson,ls1b-rtc"
>>>> "loongson,ls1c-rtc", "loongson,ls1b-rtc"
>>>> "loongson,ls7a-rtc"
>>>> "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
>>>> "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
>>>> "loongson,ls2k1000-rtc"
>> 
>> This is what you would put in the compatible section of a devicetree
>> node, using "fallback compatibles". So for a ls1c you put in
>> compatible = "loongson,ls1c-rtc", "loongson,ls1b-rtc";
>> and the kernel first tries to find a driver that supports
>> "loongson,ls1c-rtc" but if that fails it tries to find one that supports
>> "loongson,ls1b-rtc". This gives you the best of both worlds - you can
>> add support easily for new systems (when an ls1d comes out, you don't
>> even need to change the driver for it to just work!) and you have a
>> soc-specific compatible in case you need to add some workaround for
>> hardware errata etc in the future.
> 
> Hi Conor:
> 
> I seem to understand what you are talking about.
> I hadn't delved into "fallback compatibles" before, so thanks for the
> detailed explanation.
> 
> In fact, I have thought before if there is a good way to do it other
> than adding comptable to the driver frequently, and "fallback
> compatibles" should be the most suitable.
> 
> So in the dt-bindings file, should we just write this:
> 
>  compatible:
>    oneOf:
>      - items:
>          - enum:
>              - loongson,ls1c-rtc
>          - const: loongson,ls1b-rtc
>      - items:
>          - enum:
>              - loongson,ls2k0500-rtc
>              - loongson,ls2k2000-rtc
>          - const: loongson,ls7a-rtc
>      - items:
>          - const: loongson,ls2k1000-rtc

My recommendation is leaving compatible string as is.

Thanks
- Jiaxun

> 
> Thanks.
> Binbin

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Conor Dooley]

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

On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
> > 2023年5月27日 10:22，Binbin Zhou <zhoubb.aaron@gmail.com> 写道：
> > On Fri, May 26, 2023 at 8:07 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> >> On Fri, May 26, 2023 at 09:37:02AM +0800, Binbin Zhou wrote:
> >>> On Fri, May 26, 2023 at 1:05 AM Conor Dooley <conor@kernel.org> wrote:
> >>>> On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:
> >> 
> >>>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    enum:
> >>>>> +      - loongson,ls1b-rtc
> >>>>> +      - loongson,ls1c-rtc
> >>>>> +      - loongson,ls7a-rtc
> >>>>> +      - loongson,ls2k0500-rtc
> >>>>> +      - loongson,ls2k1000-rtc
> >>>>> +      - loongson,ls2k2000-rtc
> >>>> 
> >>>> |+static const struct of_device_id loongson_rtc_of_match[] = {
> >>>> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> >>>> |+       { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
> >>>> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> >>>> |+       { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
> >>>> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> >>>> |+       { .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
> >>>> |+       { /* sentinel */ }
> >>>> |+};
> >>>> 
> >>>> This is a sign to me that your compatibles here are could do with some
> >>>> fallbacks. Both of the ls1 ones are compatible with each other & there
> >>>> are three that are generic.
> >>>> 
> >>>> I would allow the following:
> >>>> "loongson,ls1b-rtc"
> >>>> "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> >>>> "loongson,ls7a-rtc"
> >>>> "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> >>>> "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
> >>>> "loongson,ls2k1000-rtc"
> >>>> 
> >>>> And then the driver only needs:
> >>>> |+static const struct of_device_id loongson_rtc_of_match[] = {
> >>>> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> >>>> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> >>>> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> >>>> |+       { /* sentinel */ }
> >>>> |+};
> >>>> 
> >>>> And ~if~when you add support for more devices in the future that are
> >>>> compatible with the existing ones no code changes are required.
> >>> 
> >>> Hi Conor:
> >>> 
> >>> Thanks for your reply.
> >>> 
> >>> Yes, this is looking much cleaner. But it can't show every chip that
> >>> supports that driver.
> >>> 
> >>> As we know, Loongson is a family of chips:
> >>> ls1b/ls1c represent the Loongson-1 family of CPU chips;
> >>> ls7a represents the Loongson LS7A bridge chip;
> >>> ls2k0500/ls2k1000/ls2k2000 represent the Loongson-2 family of CPU chips.
> >>> 
> >>> Based on my previous conversations with Krzysztof, it seems that
> >>> soc-based to order compatible is more popular, so I have listed all
> >>> the chips that support that RTC driver.
> >> 
> >> Right. You don't actually have to list them all *in the driver* though,
> >> just in the binding and in the devicetree. I think what you have missed
> >> is:
> >>>> I would allow the following:
> >>>> "loongson,ls1b-rtc"
> >>>> "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> >>>> "loongson,ls7a-rtc"
> >>>> "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> >>>> "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
> >>>> "loongson,ls2k1000-rtc"
> >> 
> >> This is what you would put in the compatible section of a devicetree
> >> node, using "fallback compatibles". So for a ls1c you put in
> >> compatible = "loongson,ls1c-rtc", "loongson,ls1b-rtc";
> >> and the kernel first tries to find a driver that supports
> >> "loongson,ls1c-rtc" but if that fails it tries to find one that supports
> >> "loongson,ls1b-rtc". This gives you the best of both worlds - you can
> >> add support easily for new systems (when an ls1d comes out, you don't
> >> even need to change the driver for it to just work!) and you have a
> >> soc-specific compatible in case you need to add some workaround for
> >> hardware errata etc in the future.
> > 
> > I seem to understand what you are talking about.
> > I hadn't delved into "fallback compatibles" before, so thanks for the
> > detailed explanation.
> > 
> > In fact, I have thought before if there is a good way to do it other
> > than adding comptable to the driver frequently, and "fallback
> > compatibles" should be the most suitable.
> > 
> > So in the dt-bindings file, should we just write this:

Not quite, because you still need to allow for ls1b-rtc and ls7a-rtc
appearing on their own. That's just two more entries like the
ls2k1000-rtc one.

> > 
> >  compatible:
> >    oneOf:
> >      - items:
> >          - enum:
> >              - loongson,ls1c-rtc
> >          - const: loongson,ls1b-rtc
> >      - items:
> >          - enum:
> >              - loongson,ls2k0500-rtc
> >              - loongson,ls2k2000-rtc
> >          - const: loongson,ls7a-rtc

> >      - items:
> >          - const: loongson,ls2k1000-rtc

This one is just "const:", you don't need "items: const:".
I didn't test this, but I figure it would be:
	compatible:
	  oneOf:
	    - items:
	        - enum:
	            - loongson,ls1c-rtc
	        - const: loongson,ls1b-rtc
	    - items:
	        - enum:
	            - loongson,ls2k0500-rtc
	            - loongson,ls2k2000-rtc
	        - const: loongson,ls7a-rtc
	    - const: loongson,ls1b-rtc
	    - const: loongson,ls2k1000-rtc
	    - const: loongson,ls7a-rtc

> My recommendation is leaving compatible string as is.

"as is" meaning "as it is right now in Linus' tree", or "as it is in
this patch"?

Cheers,
Conor.

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

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Jiaxun Yang]

> 2023年5月27日 17:23，Conor Dooley <conor@kernel.org> 写道：
> 
> On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
>>> 2023年5月27日 10:22，Binbin Zhou <zhoubb.aaron@gmail.com> 写道：
>>> On Fri, May 26, 2023 at 8:07 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>>>> On Fri, May 26, 2023 at 09:37:02AM +0800, Binbin Zhou wrote:
>>>>> On Fri, May 26, 2023 at 1:05 AM Conor Dooley <conor@kernel.org> wrote:
>>>>>> On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:
>>>> 
>>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    enum:
>>>>>>> +      - loongson,ls1b-rtc
>>>>>>> +      - loongson,ls1c-rtc
>>>>>>> +      - loongson,ls7a-rtc
>>>>>>> +      - loongson,ls2k0500-rtc
>>>>>>> +      - loongson,ls2k1000-rtc
>>>>>>> +      - loongson,ls2k2000-rtc
>>>>>> 
>>>>>> |+static const struct of_device_id loongson_rtc_of_match[] = {
>>>>>> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
>>>>>> |+       { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
>>>>>> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
>>>>>> |+       { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
>>>>>> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
>>>>>> |+       { .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
>>>>>> |+       { /* sentinel */ }
>>>>>> |+};
>>>>>> 
>>>>>> This is a sign to me that your compatibles here are could do with some
>>>>>> fallbacks. Both of the ls1 ones are compatible with each other & there
>>>>>> are three that are generic.
>>>>>> 
>>>>>> I would allow the following:
>>>>>> "loongson,ls1b-rtc"
>>>>>> "loongson,ls1c-rtc", "loongson,ls1b-rtc"
>>>>>> "loongson,ls7a-rtc"
>>>>>> "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
>>>>>> "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
>>>>>> "loongson,ls2k1000-rtc"
>>>>>> 
>>>>>> And then the driver only needs:
>>>>>> |+static const struct of_device_id loongson_rtc_of_match[] = {
>>>>>> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
>>>>>> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
>>>>>> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
>>>>>> |+       { /* sentinel */ }
>>>>>> |+};
>>>>>> 
>>>>>> And ~if~when you add support for more devices in the future that are
>>>>>> compatible with the existing ones no code changes are required.
>>>>> 
>>>>> Hi Conor:
>>>>> 
>>>>> Thanks for your reply.
>>>>> 
>>>>> Yes, this is looking much cleaner. But it can't show every chip that
>>>>> supports that driver.
>>>>> 
>>>>> As we know, Loongson is a family of chips:
>>>>> ls1b/ls1c represent the Loongson-1 family of CPU chips;
>>>>> ls7a represents the Loongson LS7A bridge chip;
>>>>> ls2k0500/ls2k1000/ls2k2000 represent the Loongson-2 family of CPU chips.
>>>>> 
>>>>> Based on my previous conversations with Krzysztof, it seems that
>>>>> soc-based to order compatible is more popular, so I have listed all
>>>>> the chips that support that RTC driver.
>>>> 
>>>> Right. You don't actually have to list them all *in the driver* though,
>>>> just in the binding and in the devicetree. I think what you have missed
>>>> is:
>>>>>> I would allow the following:
>>>>>> "loongson,ls1b-rtc"
>>>>>> "loongson,ls1c-rtc", "loongson,ls1b-rtc"
>>>>>> "loongson,ls7a-rtc"
>>>>>> "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
>>>>>> "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
>>>>>> "loongson,ls2k1000-rtc"
>>>> 
>>>> This is what you would put in the compatible section of a devicetree
>>>> node, using "fallback compatibles". So for a ls1c you put in
>>>> compatible = "loongson,ls1c-rtc", "loongson,ls1b-rtc";
>>>> and the kernel first tries to find a driver that supports
>>>> "loongson,ls1c-rtc" but if that fails it tries to find one that supports
>>>> "loongson,ls1b-rtc". This gives you the best of both worlds - you can
>>>> add support easily for new systems (when an ls1d comes out, you don't
>>>> even need to change the driver for it to just work!) and you have a
>>>> soc-specific compatible in case you need to add some workaround for
>>>> hardware errata etc in the future.
>>> 
>>> I seem to understand what you are talking about.
>>> I hadn't delved into "fallback compatibles" before, so thanks for the
>>> detailed explanation.
>>> 
>>> In fact, I have thought before if there is a good way to do it other
>>> than adding comptable to the driver frequently, and "fallback
>>> compatibles" should be the most suitable.
>>> 
>>> So in the dt-bindings file, should we just write this:
> 
> Not quite, because you still need to allow for ls1b-rtc and ls7a-rtc
> appearing on their own. That's just two more entries like the
> ls2k1000-rtc one.
> 
>>> 
>>> compatible:
>>>   oneOf:
>>>     - items:
>>>         - enum:
>>>             - loongson,ls1c-rtc
>>>         - const: loongson,ls1b-rtc
>>>     - items:
>>>         - enum:
>>>             - loongson,ls2k0500-rtc
>>>             - loongson,ls2k2000-rtc
>>>         - const: loongson,ls7a-rtc
> 
>>>     - items:
>>>         - const: loongson,ls2k1000-rtc
> 
> This one is just "const:", you don't need "items: const:".
> I didn't test this, but I figure it would be:
> compatible:
>   oneOf:
>     - items:
>         - enum:
>             - loongson,ls1c-rtc
>         - const: loongson,ls1b-rtc
>     - items:
>         - enum:
>             - loongson,ls2k0500-rtc
>             - loongson,ls2k2000-rtc
>         - const: loongson,ls7a-rtc
>     - const: loongson,ls1b-rtc
>     - const: loongson,ls2k1000-rtc
>     - const: loongson,ls7a-rtc
> 
>> My recommendation is leaving compatible string as is.
> 
> "as is" meaning "as it is right now in Linus' tree", or "as it is in
> this patch"?

Ah sorry I meant in this patch.

Since there won’t be any new ls1x chip that will boot Linux any time soon (due to
Loongson move away from MIPS but LoongArch32 is undefined for now), and
rest compatible strings are wide enough to cover their family, I think the present
compatible strings in this patch describes hardware best.

Thanks
- Jiaxun

> 
> Cheers,
> Conor.

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Conor Dooley]

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

On Sat, May 27, 2023 at 10:59:48PM +0100, Jiaxun Yang wrote:
> > 2023年5月27日 17:23，Conor Dooley <conor@kernel.org> 写道：
> > On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:

> >> My recommendation is leaving compatible string as is.
> > 
> > "as is" meaning "as it is right now in Linus' tree", or "as it is in
> > this patch"?
> 
> Ah sorry I meant in this patch.
> 
> Since there won’t be any new ls1x chip that will boot Linux any time soon (due to
> Loongson move away from MIPS but LoongArch32 is undefined for now), and
> rest compatible strings are wide enough to cover their family, I think the present
> compatible strings in this patch describes hardware best.

I don't see why new bindings being written for old hardware should somehow
be treated differently than new bindings for new hardware.

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

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Keguang Zhang]

On Sun, May 28, 2023 at 6:22 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Sat, May 27, 2023 at 10:59:48PM +0100, Jiaxun Yang wrote:
> > > 2023年5月27日 17:23，Conor Dooley <conor@kernel.org> 写道：
> > > On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
>
> > >> My recommendation is leaving compatible string as is.
> > >
> > > "as is" meaning "as it is right now in Linus' tree", or "as it is in
> > > this patch"?
> >
> > Ah sorry I meant in this patch.
> >
> > Since there won’t be any new ls1x chip that will boot Linux any time soon (due to
> > Loongson move away from MIPS but LoongArch32 is undefined for now), and
> > rest compatible strings are wide enough to cover their family, I think the present
> > compatible strings in this patch describes hardware best.
>
> I don't see why new bindings being written for old hardware should somehow
> be treated differently than new bindings for new hardware.

Let me add that ls1b RTC and ls1c RTC are not exactly the same.
The former supports RTC interrupt, while the latter does not.
So my suggestion is to leave the compatible string as it is in this patch.

-- 
Best regards,

Keguang Zhang

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Conor Dooley]

On 29 May 2023 03:59:57 IST, Keguang Zhang <keguang.zhang@gmail.com> wrote:
>On Sun, May 28, 2023 at 6:22 AM Conor Dooley <conor@kernel.org> wrote:
>>
>> On Sat, May 27, 2023 at 10:59:48PM +0100, Jiaxun Yang wrote:
>> > > 2023年5月27日 17:23，Conor Dooley <conor@kernel.org> 写道：
>> > > On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
>>
>> > >> My recommendation is leaving compatible string as is.
>> > >
>> > > "as is" meaning "as it is right now in Linus' tree", or "as it is in
>> > > this patch"?
>> >
>> > Ah sorry I meant in this patch.
>> >
>> > Since there won’t be any new ls1x chip that will boot Linux any time soon (due to
>> > Loongson move away from MIPS but LoongArch32 is undefined for now), and
>> > rest compatible strings are wide enough to cover their family, I think the present
>> > compatible strings in this patch describes hardware best.
>>
>> I don't see why new bindings being written for old hardware should somehow
>> be treated differently than new bindings for new hardware.
>
>Let me add that ls1b RTC and ls1c RTC are not exactly the same.
>The former supports RTC interrupt, while the latter does not.
>So my suggestion is to leave the compatible string as it is in this patch.

Just as a reminder, there are more than ls1b & c in the patch, lest we forget.
Also, fallback compatibles mean a compatible subset, not only that two devices are identical.
The interrupt is passed by the interrupts property.

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Binbin Zhou]

Hi Krzysztof:

Excuse me.
We have different opinions on how to better describe rtc-loongson compatible.

Based on my previous communication with you, I think we should list
all the Socs in the driver and drop the wildcards.
This should be clearer and more straightforward:

        { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
}, //ls1b soc
        { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
}, //ls1c soc
        { .compatible = "loongson,ls7a-rtc", .data =
&generic_rtc_config }, //ls7a bridge chip
        { .compatible = "loongson,ls2k0500-rtc", .data =
&generic_rtc_config }, // ls2k0500 soc
        { .compatible = "loongson,ls2k2000-rtc", .data =
&generic_rtc_config }, // ls2k2000 soc
        { .compatible = "loongson,ls2k1000-rtc", .data =
&ls2k1000_rtc_config }, // ls2k1000 soc

And Conor thought it should be rendered using a fallback compatible
form based on ".data".

        "loongson,ls1b-rtc"
        "loongson,ls1c-rtc", "loongson,ls1b-rtc"
        "loongson,ls7a-rtc"
        "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
        "longson,ls2k2000-rtc", "longson,ls7a-rtc"
        "loonson,ls2k1000-rtc"

        { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
        { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
        { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }

In this form,  I think it might not be possible to show very
graphically which chips are using the driver.
Also, for example, "ls7a" is a bridge chip, while
"ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
integrate them into one item.

Which one do you think is more suitable for us?

Here is the link to our discussion:

https://lore.kernel.org/linux-rtc/E229B204-1B00-4B24-B4BF-15277682FB4B@kernel.org/T/#m6c1ae9b74fceafc4042f7598b1bc594e68e5ec76

Thanks.
Binbin


On Mon, May 29, 2023 at 2:24 PM Conor Dooley <conor@kernel.org> wrote:
>
>
>
> On 29 May 2023 03:59:57 IST, Keguang Zhang <keguang.zhang@gmail.com> wrote:
> >On Sun, May 28, 2023 at 6:22 AM Conor Dooley <conor@kernel.org> wrote:
> >>
> >> On Sat, May 27, 2023 at 10:59:48PM +0100, Jiaxun Yang wrote:
> >> > > 2023年5月27日 17:23，Conor Dooley <conor@kernel.org> 写道：
> >> > > On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
> >>
> >> > >> My recommendation is leaving compatible string as is.
> >> > >
> >> > > "as is" meaning "as it is right now in Linus' tree", or "as it is in
> >> > > this patch"?
> >> >
> >> > Ah sorry I meant in this patch.
> >> >
> >> > Since there won’t be any new ls1x chip that will boot Linux any time soon (due to
> >> > Loongson move away from MIPS but LoongArch32 is undefined for now), and
> >> > rest compatible strings are wide enough to cover their family, I think the present
> >> > compatible strings in this patch describes hardware best.
> >>
> >> I don't see why new bindings being written for old hardware should somehow
> >> be treated differently than new bindings for new hardware.
> >
> >Let me add that ls1b RTC and ls1c RTC are not exactly the same.
> >The former supports RTC interrupt, while the latter does not.
> >So my suggestion is to leave the compatible string as it is in this patch.
>
> Just as a reminder, there are more than ls1b & c in the patch, lest we forget.
> Also, fallback compatibles mean a compatible subset, not only that two devices are identical.
> The interrupt is passed by the interrupts property.
>

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Alexandre Belloni]

Hello,

Honestly, the list of compatibles is fine for me. I wouldn't go for
fallback. The improvement would be to drop "loongson,ls1c-rtc",
and probably "loongson,ls2k0500-rtc" and "loongson,ls2k2000-rtc".

loongson,ls1c-rtc is definitively not needed, the alarm may not be wired
but the registers are there.

For 2k0500 and 2k2000, I don't mind either way.

On 29/05/2023 16:31:42+0800, Binbin Zhou wrote:
> Hi Krzysztof:
> 
> Excuse me.
> We have different opinions on how to better describe rtc-loongson compatible.
> 
> Based on my previous communication with you, I think we should list
> all the Socs in the driver and drop the wildcards.
> This should be clearer and more straightforward:
> 
>         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
> }, //ls1b soc
>         { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
> }, //ls1c soc
>         { .compatible = "loongson,ls7a-rtc", .data =
> &generic_rtc_config }, //ls7a bridge chip
>         { .compatible = "loongson,ls2k0500-rtc", .data =
> &generic_rtc_config }, // ls2k0500 soc
>         { .compatible = "loongson,ls2k2000-rtc", .data =
> &generic_rtc_config }, // ls2k2000 soc
>         { .compatible = "loongson,ls2k1000-rtc", .data =
> &ls2k1000_rtc_config }, // ls2k1000 soc
> 
> And Conor thought it should be rendered using a fallback compatible
> form based on ".data".
> 
>         "loongson,ls1b-rtc"
>         "loongson,ls1c-rtc", "loongson,ls1b-rtc"
>         "loongson,ls7a-rtc"
>         "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
>         "longson,ls2k2000-rtc", "longson,ls7a-rtc"
>         "loonson,ls2k1000-rtc"
> 
>         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
>         { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
>         { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> 
> In this form,  I think it might not be possible to show very
> graphically which chips are using the driver.
> Also, for example, "ls7a" is a bridge chip, while
> "ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
> integrate them into one item.
> 
> Which one do you think is more suitable for us?
> 
> Here is the link to our discussion:
> 
> https://lore.kernel.org/linux-rtc/E229B204-1B00-4B24-B4BF-15277682FB4B@kernel.org/T/#m6c1ae9b74fceafc4042f7598b1bc594e68e5ec76
> 
> Thanks.
> Binbin
> 
> 
> On Mon, May 29, 2023 at 2:24 PM Conor Dooley <conor@kernel.org> wrote:
> >
> >
> >
> > On 29 May 2023 03:59:57 IST, Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > >On Sun, May 28, 2023 at 6:22 AM Conor Dooley <conor@kernel.org> wrote:
> > >>
> > >> On Sat, May 27, 2023 at 10:59:48PM +0100, Jiaxun Yang wrote:
> > >> > > 2023年5月27日 17:23，Conor Dooley <conor@kernel.org> 写道：
> > >> > > On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
> > >>
> > >> > >> My recommendation is leaving compatible string as is.
> > >> > >
> > >> > > "as is" meaning "as it is right now in Linus' tree", or "as it is in
> > >> > > this patch"?
> > >> >
> > >> > Ah sorry I meant in this patch.
> > >> >
> > >> > Since there won’t be any new ls1x chip that will boot Linux any time soon (due to
> > >> > Loongson move away from MIPS but LoongArch32 is undefined for now), and
> > >> > rest compatible strings are wide enough to cover their family, I think the present
> > >> > compatible strings in this patch describes hardware best.
> > >>
> > >> I don't see why new bindings being written for old hardware should somehow
> > >> be treated differently than new bindings for new hardware.
> > >
> > >Let me add that ls1b RTC and ls1c RTC are not exactly the same.
> > >The former supports RTC interrupt, while the latter does not.
> > >So my suggestion is to leave the compatible string as it is in this patch.
> >
> > Just as a reminder, there are more than ls1b & c in the patch, lest we forget.
> > Also, fallback compatibles mean a compatible subset, not only that two devices are identical.
> > The interrupt is passed by the interrupts property.
> >

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

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Binbin Zhou]

On Tue, May 30, 2023 at 6:20 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> Hello,
>
> Honestly, the list of compatibles is fine for me. I wouldn't go for
> fallback. The improvement would be to drop "loongson,ls1c-rtc",
> and probably "loongson,ls2k0500-rtc" and "loongson,ls2k2000-rtc".
>
> loongson,ls1c-rtc is definitively not needed, the alarm may not be wired
> but the registers are there.

Hi Alexandre:

I am glad to receive your reply.

Yes, we tested and found that ls1c indeed can't trigger alarm
interrupts, but can read and write RTC time normally.
Also, in the latest rtc driver (V4), we measure the alarm function by
the interrupts property.
Therefore, whether the ls1c compatible can be retained?

Thanks.
Binbin

>
> For 2k0500 and 2k2000, I don't mind either way.
>
> On 29/05/2023 16:31:42+0800, Binbin Zhou wrote:
> > Hi Krzysztof:
> >
> > Excuse me.
> > We have different opinions on how to better describe rtc-loongson compatible.
> >
> > Based on my previous communication with you, I think we should list
> > all the Socs in the driver and drop the wildcards.
> > This should be clearer and more straightforward:
> >
> >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
> > }, //ls1b soc
> >         { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
> > }, //ls1c soc
> >         { .compatible = "loongson,ls7a-rtc", .data =
> > &generic_rtc_config }, //ls7a bridge chip
> >         { .compatible = "loongson,ls2k0500-rtc", .data =
> > &generic_rtc_config }, // ls2k0500 soc
> >         { .compatible = "loongson,ls2k2000-rtc", .data =
> > &generic_rtc_config }, // ls2k2000 soc
> >         { .compatible = "loongson,ls2k1000-rtc", .data =
> > &ls2k1000_rtc_config }, // ls2k1000 soc
> >
> > And Conor thought it should be rendered using a fallback compatible
> > form based on ".data".
> >
> >         "loongson,ls1b-rtc"
> >         "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> >         "loongson,ls7a-rtc"
> >         "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> >         "longson,ls2k2000-rtc", "longson,ls7a-rtc"
> >         "loonson,ls2k1000-rtc"
> >
> >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
> >         { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
> >         { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> >
> > In this form,  I think it might not be possible to show very
> > graphically which chips are using the driver.
> > Also, for example, "ls7a" is a bridge chip, while
> > "ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
> > integrate them into one item.
> >
> > Which one do you think is more suitable for us?
> >
> > Here is the link to our discussion:
> >
> > https://lore.kernel.org/linux-rtc/E229B204-1B00-4B24-B4BF-15277682FB4B@kernel.org/T/#m6c1ae9b74fceafc4042f7598b1bc594e68e5ec76
> >
> > Thanks.
> > Binbin
> >
> >
> > On Mon, May 29, 2023 at 2:24 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > >
> > >
> > > On 29 May 2023 03:59:57 IST, Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > > >On Sun, May 28, 2023 at 6:22 AM Conor Dooley <conor@kernel.org> wrote:
> > > >>
> > > >> On Sat, May 27, 2023 at 10:59:48PM +0100, Jiaxun Yang wrote:
> > > >> > > 2023年5月27日 17:23，Conor Dooley <conor@kernel.org> 写道：
> > > >> > > On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
> > > >>
> > > >> > >> My recommendation is leaving compatible string as is.
> > > >> > >
> > > >> > > "as is" meaning "as it is right now in Linus' tree", or "as it is in
> > > >> > > this patch"?
> > > >> >
> > > >> > Ah sorry I meant in this patch.
> > > >> >
> > > >> > Since there won’t be any new ls1x chip that will boot Linux any time soon (due to
> > > >> > Loongson move away from MIPS but LoongArch32 is undefined for now), and
> > > >> > rest compatible strings are wide enough to cover their family, I think the present
> > > >> > compatible strings in this patch describes hardware best.
> > > >>
> > > >> I don't see why new bindings being written for old hardware should somehow
> > > >> be treated differently than new bindings for new hardware.
> > > >
> > > >Let me add that ls1b RTC and ls1c RTC are not exactly the same.
> > > >The former supports RTC interrupt, while the latter does not.
> > > >So my suggestion is to leave the compatible string as it is in this patch.
> > >
> > > Just as a reminder, there are more than ls1b & c in the patch, lest we forget.
> > > Also, fallback compatibles mean a compatible subset, not only that two devices are identical.
> > > The interrupt is passed by the interrupts property.
> > >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Krzysztof Kozlowski]

On 29/05/2023 10:31, Binbin Zhou wrote:
> Hi Krzysztof:
> 
> Excuse me.
> We have different opinions on how to better describe rtc-loongson compatible.
> 
> Based on my previous communication with you, I think we should list
> all the Socs in the driver and drop the wildcards.

Suggestion was about the bindings. Not in the driver. I never said to
list all compatibles in the driver...

> This should be clearer and more straightforward:
> 
>         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
> }, //ls1b soc
>         { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
> }, //ls1c soc
>         { .compatible = "loongson,ls7a-rtc", .data =
> &generic_rtc_config }, //ls7a bridge chip
>         { .compatible = "loongson,ls2k0500-rtc", .data =
> &generic_rtc_config }, // ls2k0500 soc
>         { .compatible = "loongson,ls2k2000-rtc", .data =
> &generic_rtc_config }, // ls2k2000 soc
>         { .compatible = "loongson,ls2k1000-rtc", .data =
> &ls2k1000_rtc_config }, // ls2k1000 soc

I would suggest to use fallbacks as suggested by Conor at least for some
of them. You referred to my previous comments about wildcards.
Wildcard != fallback.

> 
> And Conor thought it should be rendered using a fallback compatible
> form based on ".data".

Based on common (compatible) programming model unless you already have
clear hardware differences making them incompatible.

> 
>         "loongson,ls1b-rtc"
>         "loongson,ls1c-rtc", "loongson,ls1b-rtc"
>         "loongson,ls7a-rtc"
>         "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
>         "longson,ls2k2000-rtc", "longson,ls7a-rtc"
>         "loonson,ls2k1000-rtc"
> 
>         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
>         { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
>         { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> 
> In this form,  I think it might not be possible to show very
> graphically which chips are using the driver.

??? How is it impossible? For all other SoCs and architectures it is
possible, so what is special for Loongson?

> Also, for example, "ls7a" is a bridge chip, while
> "ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
> integrate them into one item.

Why it is inappropriate? I don't see the issue here... what is a
"bridge" chip? Isn't this also an SoC?


> 
> Which one do you think is more suitable for us?

Use fallbacks for some. You pointed difference in alarm for ls1x, right?
If so, then they can stay separate.

ls2k500 and ls2k2000 seem compatible with each other so should use fallback.

Best regards,
Krzysztof

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Alexandre Belloni]

On 30/05/2023 10:17:43+0200, Krzysztof Kozlowski wrote:
> On 29/05/2023 10:31, Binbin Zhou wrote:
> > Hi Krzysztof:
> > 
> > Excuse me.
> > We have different opinions on how to better describe rtc-loongson compatible.
> > 
> > Based on my previous communication with you, I think we should list
> > all the Socs in the driver and drop the wildcards.
> 
> Suggestion was about the bindings. Not in the driver. I never said to
> list all compatibles in the driver...
> 
> > This should be clearer and more straightforward:
> > 
> >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
> > }, //ls1b soc
> >         { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
> > }, //ls1c soc
> >         { .compatible = "loongson,ls7a-rtc", .data =
> > &generic_rtc_config }, //ls7a bridge chip
> >         { .compatible = "loongson,ls2k0500-rtc", .data =
> > &generic_rtc_config }, // ls2k0500 soc
> >         { .compatible = "loongson,ls2k2000-rtc", .data =
> > &generic_rtc_config }, // ls2k2000 soc
> >         { .compatible = "loongson,ls2k1000-rtc", .data =
> > &ls2k1000_rtc_config }, // ls2k1000 soc
> 
> I would suggest to use fallbacks as suggested by Conor at least for some
> of them. You referred to my previous comments about wildcards.
> Wildcard != fallback.
> 
> > 
> > And Conor thought it should be rendered using a fallback compatible
> > form based on ".data".
> 
> Based on common (compatible) programming model unless you already have
> clear hardware differences making them incompatible.
> 
> > 
> >         "loongson,ls1b-rtc"
> >         "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> >         "loongson,ls7a-rtc"
> >         "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> >         "longson,ls2k2000-rtc", "longson,ls7a-rtc"
> >         "loonson,ls2k1000-rtc"
> > 
> >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
> >         { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
> >         { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> > 
> > In this form,  I think it might not be possible to show very
> > graphically which chips are using the driver.
> 
> ??? How is it impossible? For all other SoCs and architectures it is
> possible, so what is special for Loongson?
> 
> > Also, for example, "ls7a" is a bridge chip, while
> > "ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
> > integrate them into one item.
> 
> Why it is inappropriate? I don't see the issue here... what is a
> "bridge" chip? Isn't this also an SoC?
> 
> 
> > 
> > Which one do you think is more suitable for us?
> 
> Use fallbacks for some. You pointed difference in alarm for ls1x, right?
> If so, then they can stay separate.

From what I seen the IP and register set is the same, it is just the
integration on the SoC that differs.


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

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Keguang Zhang]

On Tue, May 30, 2023 at 4:40 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 30/05/2023 10:17:43+0200, Krzysztof Kozlowski wrote:
> > On 29/05/2023 10:31, Binbin Zhou wrote:
> > > Hi Krzysztof:
> > >
> > > Excuse me.
> > > We have different opinions on how to better describe rtc-loongson compatible.
> > >
> > > Based on my previous communication with you, I think we should list
> > > all the Socs in the driver and drop the wildcards.
> >
> > Suggestion was about the bindings. Not in the driver. I never said to
> > list all compatibles in the driver...
> >
> > > This should be clearer and more straightforward:
> > >
> > >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
> > > }, //ls1b soc
> > >         { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
> > > }, //ls1c soc
> > >         { .compatible = "loongson,ls7a-rtc", .data =
> > > &generic_rtc_config }, //ls7a bridge chip
> > >         { .compatible = "loongson,ls2k0500-rtc", .data =
> > > &generic_rtc_config }, // ls2k0500 soc
> > >         { .compatible = "loongson,ls2k2000-rtc", .data =
> > > &generic_rtc_config }, // ls2k2000 soc
> > >         { .compatible = "loongson,ls2k1000-rtc", .data =
> > > &ls2k1000_rtc_config }, // ls2k1000 soc
> >
> > I would suggest to use fallbacks as suggested by Conor at least for some
> > of them. You referred to my previous comments about wildcards.
> > Wildcard != fallback.
> >
> > >
> > > And Conor thought it should be rendered using a fallback compatible
> > > form based on ".data".
> >
> > Based on common (compatible) programming model unless you already have
> > clear hardware differences making them incompatible.
> >
> > >
> > >         "loongson,ls1b-rtc"
> > >         "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> > >         "loongson,ls7a-rtc"
> > >         "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> > >         "longson,ls2k2000-rtc", "longson,ls7a-rtc"
> > >         "loonson,ls2k1000-rtc"
> > >
> > >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
> > >         { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
> > >         { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> > >
> > > In this form,  I think it might not be possible to show very
> > > graphically which chips are using the driver.
> >
> > ??? How is it impossible? For all other SoCs and architectures it is
> > possible, so what is special for Loongson?
> >
> > > Also, for example, "ls7a" is a bridge chip, while
> > > "ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
> > > integrate them into one item.
> >
> > Why it is inappropriate? I don't see the issue here... what is a
> > "bridge" chip? Isn't this also an SoC?
> >
> >
> > >
> > > Which one do you think is more suitable for us?
> >
> > Use fallbacks for some. You pointed difference in alarm for ls1x, right?
> > If so, then they can stay separate.
>
> From what I seen the IP and register set is the same, it is just the
> integration on the SoC that differs.
>
Actually, ls1c RTC registers are not the same as ls1b.
ls1c doesn't have the following resgisters.
+#define TOY_MATCH0_REG         0x34 /* TOY timing interrupt 0 */
+#define TOY_MATCH1_REG         0x38 /* TOY timing interrupt 1 */
+#define TOY_MATCH2_REG         0x3c /* TOY timing interrupt 2 */

+#define RTC_CTRL_REG           0x40 /* TOY and RTC control register */
+#define RTC_TRIM_REG           0x60 /* Must be initialized to 0 */
+#define RTC_WRITE0_REG         0x64 /* RTC counters value (write-only) */
+#define RTC_READ0_REG          0x68 /* RTC counters value (read-only) */
+#define RTC_MATCH0_REG         0x6c /* RTC timing interrupt 0 */
+#define RTC_MATCH1_REG         0x70 /* RTC timing interrupt 1 */
+#define RTC_MATCH2_REG         0x74 /* RTC timing interrupt 2 */

As you can see, it doesn't support match function, which is why ls1c
doesn't support RTC interrupt.
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
Best regards,

Keguang Zhang

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Alexandre Belloni]

On 30/05/2023 17:13:12+0800, Keguang Zhang wrote:
> On Tue, May 30, 2023 at 4:40 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > On 30/05/2023 10:17:43+0200, Krzysztof Kozlowski wrote:
> > > On 29/05/2023 10:31, Binbin Zhou wrote:
> > > > Hi Krzysztof:
> > > >
> > > > Excuse me.
> > > > We have different opinions on how to better describe rtc-loongson compatible.
> > > >
> > > > Based on my previous communication with you, I think we should list
> > > > all the Socs in the driver and drop the wildcards.
> > >
> > > Suggestion was about the bindings. Not in the driver. I never said to
> > > list all compatibles in the driver...
> > >
> > > > This should be clearer and more straightforward:
> > > >
> > > >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
> > > > }, //ls1b soc
> > > >         { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
> > > > }, //ls1c soc
> > > >         { .compatible = "loongson,ls7a-rtc", .data =
> > > > &generic_rtc_config }, //ls7a bridge chip
> > > >         { .compatible = "loongson,ls2k0500-rtc", .data =
> > > > &generic_rtc_config }, // ls2k0500 soc
> > > >         { .compatible = "loongson,ls2k2000-rtc", .data =
> > > > &generic_rtc_config }, // ls2k2000 soc
> > > >         { .compatible = "loongson,ls2k1000-rtc", .data =
> > > > &ls2k1000_rtc_config }, // ls2k1000 soc
> > >
> > > I would suggest to use fallbacks as suggested by Conor at least for some
> > > of them. You referred to my previous comments about wildcards.
> > > Wildcard != fallback.
> > >
> > > >
> > > > And Conor thought it should be rendered using a fallback compatible
> > > > form based on ".data".
> > >
> > > Based on common (compatible) programming model unless you already have
> > > clear hardware differences making them incompatible.
> > >
> > > >
> > > >         "loongson,ls1b-rtc"
> > > >         "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> > > >         "loongson,ls7a-rtc"
> > > >         "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> > > >         "longson,ls2k2000-rtc", "longson,ls7a-rtc"
> > > >         "loonson,ls2k1000-rtc"
> > > >
> > > >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
> > > >         { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
> > > >         { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> > > >
> > > > In this form,  I think it might not be possible to show very
> > > > graphically which chips are using the driver.
> > >
> > > ??? How is it impossible? For all other SoCs and architectures it is
> > > possible, so what is special for Loongson?
> > >
> > > > Also, for example, "ls7a" is a bridge chip, while
> > > > "ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
> > > > integrate them into one item.
> > >
> > > Why it is inappropriate? I don't see the issue here... what is a
> > > "bridge" chip? Isn't this also an SoC?
> > >
> > >
> > > >
> > > > Which one do you think is more suitable for us?
> > >
> > > Use fallbacks for some. You pointed difference in alarm for ls1x, right?
> > > If so, then they can stay separate.
> >
> > From what I seen the IP and register set is the same, it is just the
> > integration on the SoC that differs.
> >
> Actually, ls1c RTC registers are not the same as ls1b.
> ls1c doesn't have the following resgisters.
> +#define TOY_MATCH0_REG         0x34 /* TOY timing interrupt 0 */
> +#define TOY_MATCH1_REG         0x38 /* TOY timing interrupt 1 */
> +#define TOY_MATCH2_REG         0x3c /* TOY timing interrupt 2 */
> 
> +#define RTC_CTRL_REG           0x40 /* TOY and RTC control register */
> +#define RTC_TRIM_REG           0x60 /* Must be initialized to 0 */
> +#define RTC_WRITE0_REG         0x64 /* RTC counters value (write-only) */
> +#define RTC_READ0_REG          0x68 /* RTC counters value (read-only) */
> +#define RTC_MATCH0_REG         0x6c /* RTC timing interrupt 0 */
> +#define RTC_MATCH1_REG         0x70 /* RTC timing interrupt 1 */
> +#define RTC_MATCH2_REG         0x74 /* RTC timing interrupt 2 */
> 
> As you can see, it doesn't support match function, which is why ls1c
> doesn't support RTC interrupt.

They are in the Loongson1C Processor User Manual I have which states:

21.2.6 SYS_TOYMATCH0/1/2 (no register in 1C2)

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

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Keguang Zhang]

On Tue, May 30, 2023 at 5:22 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 30/05/2023 17:13:12+0800, Keguang Zhang wrote:
> > On Tue, May 30, 2023 at 4:40 PM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > >
> > > On 30/05/2023 10:17:43+0200, Krzysztof Kozlowski wrote:
> > > > On 29/05/2023 10:31, Binbin Zhou wrote:
> > > > > Hi Krzysztof:
> > > > >
> > > > > Excuse me.
> > > > > We have different opinions on how to better describe rtc-loongson compatible.
> > > > >
> > > > > Based on my previous communication with you, I think we should list
> > > > > all the Socs in the driver and drop the wildcards.
> > > >
> > > > Suggestion was about the bindings. Not in the driver. I never said to
> > > > list all compatibles in the driver...
> > > >
> > > > > This should be clearer and more straightforward:
> > > > >
> > > > >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
> > > > > }, //ls1b soc
> > > > >         { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
> > > > > }, //ls1c soc
> > > > >         { .compatible = "loongson,ls7a-rtc", .data =
> > > > > &generic_rtc_config }, //ls7a bridge chip
> > > > >         { .compatible = "loongson,ls2k0500-rtc", .data =
> > > > > &generic_rtc_config }, // ls2k0500 soc
> > > > >         { .compatible = "loongson,ls2k2000-rtc", .data =
> > > > > &generic_rtc_config }, // ls2k2000 soc
> > > > >         { .compatible = "loongson,ls2k1000-rtc", .data =
> > > > > &ls2k1000_rtc_config }, // ls2k1000 soc
> > > >
> > > > I would suggest to use fallbacks as suggested by Conor at least for some
> > > > of them. You referred to my previous comments about wildcards.
> > > > Wildcard != fallback.
> > > >
> > > > >
> > > > > And Conor thought it should be rendered using a fallback compatible
> > > > > form based on ".data".
> > > >
> > > > Based on common (compatible) programming model unless you already have
> > > > clear hardware differences making them incompatible.
> > > >
> > > > >
> > > > >         "loongson,ls1b-rtc"
> > > > >         "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> > > > >         "loongson,ls7a-rtc"
> > > > >         "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> > > > >         "longson,ls2k2000-rtc", "longson,ls7a-rtc"
> > > > >         "loonson,ls2k1000-rtc"
> > > > >
> > > > >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
> > > > >         { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
> > > > >         { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> > > > >
> > > > > In this form,  I think it might not be possible to show very
> > > > > graphically which chips are using the driver.
> > > >
> > > > ??? How is it impossible? For all other SoCs and architectures it is
> > > > possible, so what is special for Loongson?
> > > >
> > > > > Also, for example, "ls7a" is a bridge chip, while
> > > > > "ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
> > > > > integrate them into one item.
> > > >
> > > > Why it is inappropriate? I don't see the issue here... what is a
> > > > "bridge" chip? Isn't this also an SoC?
> > > >
> > > >
> > > > >
> > > > > Which one do you think is more suitable for us?
> > > >
> > > > Use fallbacks for some. You pointed difference in alarm for ls1x, right?
> > > > If so, then they can stay separate.
> > >
> > > From what I seen the IP and register set is the same, it is just the
> > > integration on the SoC that differs.
> > >
> > Actually, ls1c RTC registers are not the same as ls1b.
> > ls1c doesn't have the following resgisters.
> > +#define TOY_MATCH0_REG         0x34 /* TOY timing interrupt 0 */
> > +#define TOY_MATCH1_REG         0x38 /* TOY timing interrupt 1 */
> > +#define TOY_MATCH2_REG         0x3c /* TOY timing interrupt 2 */
> >
> > +#define RTC_CTRL_REG           0x40 /* TOY and RTC control register */
> > +#define RTC_TRIM_REG           0x60 /* Must be initialized to 0 */
> > +#define RTC_WRITE0_REG         0x64 /* RTC counters value (write-only) */
> > +#define RTC_READ0_REG          0x68 /* RTC counters value (read-only) */
> > +#define RTC_MATCH0_REG         0x6c /* RTC timing interrupt 0 */
> > +#define RTC_MATCH1_REG         0x70 /* RTC timing interrupt 1 */
> > +#define RTC_MATCH2_REG         0x74 /* RTC timing interrupt 2 */
> >
> > As you can see, it doesn't support match function, which is why ls1c
> > doesn't support RTC interrupt.
>
> They are in the Loongson1C Processor User Manual I have which states:
>
> 21.2.6 SYS_TOYMATCH0/1/2 (no register in 1C2)
>
I'm afraid that your user manual is outdated.
The latest 1C300 user manual (v1.5) doesn't have section 21.2.6 at all.
Sorry, I can't find English version.
Here is the Chinese version:
https://www.loongson.cn/uploads/images/2022051616223977135.%E9%BE%99%E8%8A%AF1C300%E5%A4%84%E7%90%86%E5%99%A8%E7%94%A8%E6%88%B7%E6%89%8B%E5%86%8C.pdf
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
Best regards,

Keguang Zhang

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Binbin Zhou]

On Tue, May 30, 2023 at 4:17 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/05/2023 10:31, Binbin Zhou wrote:
> > Hi Krzysztof:
> >
> > Excuse me.
> > We have different opinions on how to better describe rtc-loongson compatible.
> >
> > Based on my previous communication with you, I think we should list
> > all the Socs in the driver and drop the wildcards.
>
> Suggestion was about the bindings. Not in the driver. I never said to
> list all compatibles in the driver...
>
> > This should be clearer and more straightforward:
> >
> >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config
> > }, //ls1b soc
> >         { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config
> > }, //ls1c soc
> >         { .compatible = "loongson,ls7a-rtc", .data =
> > &generic_rtc_config }, //ls7a bridge chip
> >         { .compatible = "loongson,ls2k0500-rtc", .data =
> > &generic_rtc_config }, // ls2k0500 soc
> >         { .compatible = "loongson,ls2k2000-rtc", .data =
> > &generic_rtc_config }, // ls2k2000 soc
> >         { .compatible = "loongson,ls2k1000-rtc", .data =
> > &ls2k1000_rtc_config }, // ls2k1000 soc
>
> I would suggest to use fallbacks as suggested by Conor at least for some
> of them. You referred to my previous comments about wildcards.
> Wildcard != fallback.
>
> >
> > And Conor thought it should be rendered using a fallback compatible
> > form based on ".data".
>
> Based on common (compatible) programming model unless you already have
> clear hardware differences making them incompatible.
>
> >
> >         "loongson,ls1b-rtc"
> >         "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> >         "loongson,ls7a-rtc"
> >         "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> >         "longson,ls2k2000-rtc", "longson,ls7a-rtc"
> >         "loonson,ls2k1000-rtc"
> >
> >         { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
> >         { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
> >         { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> >
> > In this form,  I think it might not be possible to show very
> > graphically which chips are using the driver.
>
> ??? How is it impossible? For all other SoCs and architectures it is
> possible, so what is special for Loongson?
>
> > Also, for example, "ls7a" is a bridge chip, while
> > "ls2k2000"/"ls2k0500" are soc chips, and it seems inappropriate to
> > integrate them into one item.
>
> Why it is inappropriate? I don't see the issue here... what is a
> "bridge" chip? Isn't this also an SoC?
>
Hi Krzysztof:

LS7A bridge chip can be considered as a combination of South and North
bridge. Generally, it will be connected to the Loongson-3 series CPUs.
LS2K500/LS2K1000/LS2K2000 refers to the LS2K series embedded CPU chip.

Therefore, from the understanding of the driver code, I don't think it
is appropriate to fallback them together. Please pardon me if this
view does not apply to dt-binding.

If fallback is necessary, can we have this:

Let ls7a remain a separate item.

"loongson,ls1b-rtc"
"loongson,ls1c-rtc", "loongson,ls1b-rtc"
"loongson,ls7a-rtc"
"loongson,ls2k0500-rtc"
"loongson,ls2k2000-rtc", "loongson,ls2k0500-rtc"
"loongson,ls2k1000-rtc"

{ .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
{ .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
{ .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config }
{ .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }

Thanks.
Binbin

>
> >
> > Which one do you think is more suitable for us?
>
> Use fallbacks for some. You pointed difference in alarm for ls1x, right?
> If so, then they can stay separate.
>
> ls2k500 and ls2k2000 seem compatible with each other so should use fallback.
>
> Best regards,
> Krzysztof
>

Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Alexandre Belloni]

On 30/05/2023 19:39:50+0800, Binbin Zhou wrote:
> Hi Krzysztof:
> 
> LS7A bridge chip can be considered as a combination of South and North
> bridge. Generally, it will be connected to the Loongson-3 series CPUs.
> LS2K500/LS2K1000/LS2K2000 refers to the LS2K series embedded CPU chip.
> 
> Therefore, from the understanding of the driver code, I don't think it
> is appropriate to fallback them together. Please pardon me if this
> view does not apply to dt-binding.
> 
> If fallback is necessary, can we have this:
> 
> Let ls7a remain a separate item.
> 
> "loongson,ls1b-rtc"
> "loongson,ls1c-rtc", "loongson,ls1b-rtc"

Based on Keguang's feedback, "loongson,ls1b-rtc" is not a fallback for
"loongson,ls1c-rtc" as it is missing registers, keep it standalone.

> "loongson,ls7a-rtc"
> "loongson,ls2k0500-rtc"
> "loongson,ls2k2000-rtc", "loongson,ls2k0500-rtc"
> "loongson,ls2k1000-rtc"
> 
> { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config }
> { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config }
> { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config }
> { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config }
> 
> Thanks.
> Binbin
> 
> >
> > >
> > > Which one do you think is more suitable for us?
> >
> > Use fallbacks for some. You pointed difference in alarm for ls1x, right?
> > If so, then they can stay separate.
> >
> > ls2k500 and ls2k2000 seem compatible with each other so should use fallback.
> >
> > Best regards,
> > Krzysztof
> >

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

[PATCH V4 2/5] rtc: Remove the Loongson-1 RTC driver

[Binbin Zhou]

Remove the ls1x-rtc driver as it is obsolete. We will continue to
support the ls1x RTC in the upcoming Loongson unified RTC driver
rtc-loongson.

Cc: Keguang Zhang <keguang.zhang@gmail.com>
Cc: zhao zhang <zhzhl555@gmail.com>
Cc: Yang Ling <gnaygnil@gmail.com>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/rtc/Kconfig    |  10 ---
 drivers/rtc/Makefile   |   1 -
 drivers/rtc/rtc-ls1x.c | 192 -----------------------------------------
 3 files changed, 203 deletions(-)
 delete mode 100644 drivers/rtc/rtc-ls1x.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 753872408615..599f5110a251 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1726,16 +1726,6 @@ config RTC_DRV_TEGRA
 	  This drive can also be built as a module. If so, the module
 	  will be called rtc-tegra.
 
-config RTC_DRV_LOONGSON1
-	tristate "loongson1 RTC support"
-	depends on MACH_LOONGSON32
-	help
-	  This is a driver for the loongson1 on-chip Counter0 (Time-Of-Year
-	  counter) to be used as a RTC.
-
-	  This driver can also be built as a module. If so, the module
-	  will be called rtc-ls1x.
-
 config RTC_DRV_MXC
 	tristate "Freescale MXC Real Time Clock"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index ea445d1ebb17..c50afd8fb9f4 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -78,7 +78,6 @@ obj-$(CONFIG_RTC_DRV_ISL12022)	+= rtc-isl12022.o
 obj-$(CONFIG_RTC_DRV_ISL12026)	+= rtc-isl12026.o
 obj-$(CONFIG_RTC_DRV_ISL1208)	+= rtc-isl1208.o
 obj-$(CONFIG_RTC_DRV_JZ4740)	+= rtc-jz4740.o
-obj-$(CONFIG_RTC_DRV_LOONGSON1)	+= rtc-ls1x.o
 obj-$(CONFIG_RTC_DRV_LP8788)	+= rtc-lp8788.o
 obj-$(CONFIG_RTC_DRV_LPC24XX)	+= rtc-lpc24xx.o
 obj-$(CONFIG_RTC_DRV_LPC32XX)	+= rtc-lpc32xx.o
diff --git a/drivers/rtc/rtc-ls1x.c b/drivers/rtc/rtc-ls1x.c
deleted file mode 100644
index 5af26dc5c2a3..000000000000
--- a/drivers/rtc/rtc-ls1x.c
+++ /dev/null
@@ -1,192 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright (c) 2011 Zhao Zhang <zhzhl555@gmail.com>
- *
- * Derived from driver/rtc/rtc-au1xxx.c
- */
-
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/rtc.h>
-#include <linux/init.h>
-#include <linux/platform_device.h>
-#include <linux/delay.h>
-#include <linux/types.h>
-#include <linux/io.h>
-#include <loongson1.h>
-
-#define LS1X_RTC_REG_OFFSET	(LS1X_RTC_BASE + 0x20)
-#define LS1X_RTC_REGS(x) \
-		((void __iomem *)KSEG1ADDR(LS1X_RTC_REG_OFFSET + (x)))
-
-/*RTC programmable counters 0 and 1*/
-#define SYS_COUNTER_CNTRL		(LS1X_RTC_REGS(0x20))
-#define SYS_CNTRL_ERS			(1 << 23)
-#define SYS_CNTRL_RTS			(1 << 20)
-#define SYS_CNTRL_RM2			(1 << 19)
-#define SYS_CNTRL_RM1			(1 << 18)
-#define SYS_CNTRL_RM0			(1 << 17)
-#define SYS_CNTRL_RS			(1 << 16)
-#define SYS_CNTRL_BP			(1 << 14)
-#define SYS_CNTRL_REN			(1 << 13)
-#define SYS_CNTRL_BRT			(1 << 12)
-#define SYS_CNTRL_TEN			(1 << 11)
-#define SYS_CNTRL_BTT			(1 << 10)
-#define SYS_CNTRL_E0			(1 << 8)
-#define SYS_CNTRL_ETS			(1 << 7)
-#define SYS_CNTRL_32S			(1 << 5)
-#define SYS_CNTRL_TTS			(1 << 4)
-#define SYS_CNTRL_TM2			(1 << 3)
-#define SYS_CNTRL_TM1			(1 << 2)
-#define SYS_CNTRL_TM0			(1 << 1)
-#define SYS_CNTRL_TS			(1 << 0)
-
-/* Programmable Counter 0 Registers */
-#define SYS_TOYTRIM		(LS1X_RTC_REGS(0))
-#define SYS_TOYWRITE0		(LS1X_RTC_REGS(4))
-#define SYS_TOYWRITE1		(LS1X_RTC_REGS(8))
-#define SYS_TOYREAD0		(LS1X_RTC_REGS(0xC))
-#define SYS_TOYREAD1		(LS1X_RTC_REGS(0x10))
-#define SYS_TOYMATCH0		(LS1X_RTC_REGS(0x14))
-#define SYS_TOYMATCH1		(LS1X_RTC_REGS(0x18))
-#define SYS_TOYMATCH2		(LS1X_RTC_REGS(0x1C))
-
-/* Programmable Counter 1 Registers */
-#define SYS_RTCTRIM		(LS1X_RTC_REGS(0x40))
-#define SYS_RTCWRITE0		(LS1X_RTC_REGS(0x44))
-#define SYS_RTCREAD0		(LS1X_RTC_REGS(0x48))
-#define SYS_RTCMATCH0		(LS1X_RTC_REGS(0x4C))
-#define SYS_RTCMATCH1		(LS1X_RTC_REGS(0x50))
-#define SYS_RTCMATCH2		(LS1X_RTC_REGS(0x54))
-
-#define LS1X_SEC_OFFSET		(4)
-#define LS1X_MIN_OFFSET		(10)
-#define LS1X_HOUR_OFFSET	(16)
-#define LS1X_DAY_OFFSET		(21)
-#define LS1X_MONTH_OFFSET	(26)
-
-
-#define LS1X_SEC_MASK		(0x3f)
-#define LS1X_MIN_MASK		(0x3f)
-#define LS1X_HOUR_MASK		(0x1f)
-#define LS1X_DAY_MASK		(0x1f)
-#define LS1X_MONTH_MASK		(0x3f)
-#define LS1X_YEAR_MASK		(0xffffffff)
-
-#define ls1x_get_sec(t)		(((t) >> LS1X_SEC_OFFSET) & LS1X_SEC_MASK)
-#define ls1x_get_min(t)		(((t) >> LS1X_MIN_OFFSET) & LS1X_MIN_MASK)
-#define ls1x_get_hour(t)	(((t) >> LS1X_HOUR_OFFSET) & LS1X_HOUR_MASK)
-#define ls1x_get_day(t)		(((t) >> LS1X_DAY_OFFSET) & LS1X_DAY_MASK)
-#define ls1x_get_month(t)	(((t) >> LS1X_MONTH_OFFSET) & LS1X_MONTH_MASK)
-
-#define RTC_CNTR_OK (SYS_CNTRL_E0 | SYS_CNTRL_32S)
-
-static int ls1x_rtc_read_time(struct device *dev, struct rtc_time *rtm)
-{
-	unsigned long v;
-	time64_t t;
-
-	v = readl(SYS_TOYREAD0);
-	t = readl(SYS_TOYREAD1);
-
-	memset(rtm, 0, sizeof(struct rtc_time));
-	t  = mktime64((t & LS1X_YEAR_MASK), ls1x_get_month(v),
-			ls1x_get_day(v), ls1x_get_hour(v),
-			ls1x_get_min(v), ls1x_get_sec(v));
-	rtc_time64_to_tm(t, rtm);
-
-	return 0;
-}
-
-static int ls1x_rtc_set_time(struct device *dev, struct  rtc_time *rtm)
-{
-	unsigned long v, t, c;
-	int ret = -ETIMEDOUT;
-
-	v = ((rtm->tm_mon + 1)  << LS1X_MONTH_OFFSET)
-		| (rtm->tm_mday << LS1X_DAY_OFFSET)
-		| (rtm->tm_hour << LS1X_HOUR_OFFSET)
-		| (rtm->tm_min  << LS1X_MIN_OFFSET)
-		| (rtm->tm_sec  << LS1X_SEC_OFFSET);
-
-	writel(v, SYS_TOYWRITE0);
-	c = 0x10000;
-	/* add timeout check counter, for more safe */
-	while ((readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TS) && --c)
-		usleep_range(1000, 3000);
-
-	if (!c) {
-		dev_err(dev, "set time timeout!\n");
-		goto err;
-	}
-
-	t = rtm->tm_year + 1900;
-	writel(t, SYS_TOYWRITE1);
-	c = 0x10000;
-	while ((readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TS) && --c)
-		usleep_range(1000, 3000);
-
-	if (!c) {
-		dev_err(dev, "set time timeout!\n");
-		goto err;
-	}
-	return 0;
-err:
-	return ret;
-}
-
-static const struct rtc_class_ops  ls1x_rtc_ops = {
-	.read_time	= ls1x_rtc_read_time,
-	.set_time	= ls1x_rtc_set_time,
-};
-
-static int ls1x_rtc_probe(struct platform_device *pdev)
-{
-	struct rtc_device *rtcdev;
-	unsigned long v;
-
-	v = readl(SYS_COUNTER_CNTRL);
-	if (!(v & RTC_CNTR_OK)) {
-		dev_err(&pdev->dev, "rtc counters not working\n");
-		return -ENODEV;
-	}
-
-	/* set to 1 HZ if needed */
-	if (readl(SYS_TOYTRIM) != 32767) {
-		v = 0x100000;
-		while ((readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TTS) && --v)
-			usleep_range(1000, 3000);
-
-		if (!v) {
-			dev_err(&pdev->dev, "time out\n");
-			return -ETIMEDOUT;
-		}
-		writel(32767, SYS_TOYTRIM);
-	}
-	/* this loop coundn't be endless */
-	while (readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TTS)
-		usleep_range(1000, 3000);
-
-	rtcdev = devm_rtc_allocate_device(&pdev->dev);
-	if (IS_ERR(rtcdev))
-		return PTR_ERR(rtcdev);
-
-	platform_set_drvdata(pdev, rtcdev);
-	rtcdev->ops = &ls1x_rtc_ops;
-	rtcdev->range_min = RTC_TIMESTAMP_BEGIN_1900;
-	rtcdev->range_max = RTC_TIMESTAMP_END_2099;
-
-	return devm_rtc_register_device(rtcdev);
-}
-
-static struct platform_driver  ls1x_rtc_driver = {
-	.driver		= {
-		.name	= "ls1x-rtc",
-	},
-	.probe		= ls1x_rtc_probe,
-};
-
-module_platform_driver(ls1x_rtc_driver);
-
-MODULE_AUTHOR("zhao zhang <zhzhl555@gmail.com>");
-MODULE_LICENSE("GPL");
-- 
2.39.1

Re: [PATCH V4 2/5] rtc: Remove the Loongson-1 RTC driver

[Krzysztof Kozlowski]

On 25/05/2023 14:55, Binbin Zhou wrote:
> Remove the ls1x-rtc driver as it is obsolete. We will continue to
> support the ls1x RTC in the upcoming Loongson unified RTC driver
> rtc-loongson.
> 
> Cc: Keguang Zhang <keguang.zhang@gmail.com>
> Cc: zhao zhang <zhzhl555@gmail.com>
> Cc: Yang Ling <gnaygnil@gmail.com>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  drivers/rtc/Kconfig    |  10 ---
>  drivers/rtc/Makefile   |   1 -
>  drivers/rtc/rtc-ls1x.c | 192 -----------------------------------------
>  3 files changed, 203 deletions(-)

Removal in one commit and adding a new driver in second is not what we
usually do. We expect code to be developed and to evolve.

Best regards,
Krzysztof

Re: [PATCH V4 2/5] rtc: Remove the Loongson-1 RTC driver

[Alexandre Belloni]

On 30/05/2023 10:08:22+0200, Krzysztof Kozlowski wrote:
> On 25/05/2023 14:55, Binbin Zhou wrote:
> > Remove the ls1x-rtc driver as it is obsolete. We will continue to
> > support the ls1x RTC in the upcoming Loongson unified RTC driver
> > rtc-loongson.
> > 
> > Cc: Keguang Zhang <keguang.zhang@gmail.com>
> > Cc: zhao zhang <zhzhl555@gmail.com>
> > Cc: Yang Ling <gnaygnil@gmail.com>
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  drivers/rtc/Kconfig    |  10 ---
> >  drivers/rtc/Makefile   |   1 -
> >  drivers/rtc/rtc-ls1x.c | 192 -----------------------------------------
> >  3 files changed, 203 deletions(-)
> 
> Removal in one commit and adding a new driver in second is not what we
> usually do. We expect code to be developed and to evolve.
> 

I'm fine with that, even if unusual.


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

[PATCH V4 3/5] rtc: Add rtc driver for the Loongson family chips

[Binbin Zhou]

The Loongson family chips use an on-chip counter 0 (Time Of Year
counter) as the RTC. We will refer to them as rtc-loongson.

Cc: Keguang Zhang <keguang.zhang@gmail.com>
Cc: Yang Ling <gnaygnil@gmail.com>
Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
Signed-off-by: Huacai Chen <chenhuacai@kernel.org>
Signed-off-by: WANG Xuerui <git@xen0n.name>
---
 drivers/rtc/Kconfig        |  13 ++
 drivers/rtc/Makefile       |   1 +
 drivers/rtc/rtc-loongson.c | 390 +++++++++++++++++++++++++++++++++++++
 3 files changed, 404 insertions(+)
 create mode 100644 drivers/rtc/rtc-loongson.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 599f5110a251..9f5b0afdbad0 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1685,6 +1685,19 @@ config RTC_DRV_JZ4740
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-jz4740.
 
+config RTC_DRV_LOONGSON
+	tristate "Loongson On-chip RTC"
+	depends on MACH_LOONGSON32 || MACH_LOONGSON64 || COMPILE_TEST
+	select REGMAP_MMIO
+	help
+	  This is a driver for the Loongson on-chip Counter0 (Time-Of-Year
+	  counter) to be used as a RTC.
+	  It can be found on Loongson-1 series cpu, Loongson-2K series cpu
+	  and Loongson LS7A bridge chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-loongson.
+
 config RTC_DRV_LPC24XX
 	tristate "NXP RTC for LPC178x/18xx/408x/43xx"
 	depends on ARCH_LPC18XX || COMPILE_TEST
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index c50afd8fb9f4..fd209883ee2e 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_RTC_DRV_ISL12022)	+= rtc-isl12022.o
 obj-$(CONFIG_RTC_DRV_ISL12026)	+= rtc-isl12026.o
 obj-$(CONFIG_RTC_DRV_ISL1208)	+= rtc-isl1208.o
 obj-$(CONFIG_RTC_DRV_JZ4740)	+= rtc-jz4740.o
+obj-$(CONFIG_RTC_DRV_LOONGSON)	+= rtc-loongson.o
 obj-$(CONFIG_RTC_DRV_LP8788)	+= rtc-lp8788.o
 obj-$(CONFIG_RTC_DRV_LPC24XX)	+= rtc-lpc24xx.o
 obj-$(CONFIG_RTC_DRV_LPC32XX)	+= rtc-lpc32xx.o
diff --git a/drivers/rtc/rtc-loongson.c b/drivers/rtc/rtc-loongson.c
new file mode 100644
index 000000000000..6f5d2dfa1f3b
--- /dev/null
+++ b/drivers/rtc/rtc-loongson.c
@@ -0,0 +1,390 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Loongson RTC driver
+ *
+ * Maintained out-of-tree by Huacai Chen <chenhuacai@kernel.org>.
+ * Rewritten for mainline by WANG Xuerui <git@xen0n.name>.
+ *                           Binbin Zhou <zhoubinbin@loongson.cn>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+#include <linux/acpi.h>
+
+/* Time Of Year(TOY) counters registers */
+#define TOY_TRIM_REG		0x20 /* Must be initialized to 0 */
+#define TOY_WRITE0_REG		0x24 /* TOY low 32-bits value (write-only) */
+#define TOY_WRITE1_REG		0x28 /* TOY high 32-bits value (write-only) */
+#define TOY_READ0_REG		0x2c /* TOY low 32-bits value (read-only) */
+#define TOY_READ1_REG		0x30 /* TOY high 32-bits value (read-only) */
+#define TOY_MATCH0_REG		0x34 /* TOY timing interrupt 0 */
+#define TOY_MATCH1_REG		0x38 /* TOY timing interrupt 1 */
+#define TOY_MATCH2_REG		0x3c /* TOY timing interrupt 2 */
+
+/* RTC counters registers */
+#define RTC_CTRL_REG		0x40 /* TOY and RTC control register */
+#define RTC_TRIM_REG		0x60 /* Must be initialized to 0 */
+#define RTC_WRITE0_REG		0x64 /* RTC counters value (write-only) */
+#define RTC_READ0_REG		0x68 /* RTC counters value (read-only) */
+#define RTC_MATCH0_REG		0x6c /* RTC timing interrupt 0 */
+#define RTC_MATCH1_REG		0x70 /* RTC timing interrupt 1 */
+#define RTC_MATCH2_REG		0x74 /* RTC timing interrupt 2 */
+
+/* bitmask of TOY_WRITE0_REG */
+#define TOY_MON			GENMASK(31, 26)
+#define TOY_DAY			GENMASK(25, 21)
+#define TOY_HOUR		GENMASK(20, 16)
+#define TOY_MIN			GENMASK(15, 10)
+#define TOY_SEC			GENMASK(9, 4)
+#define TOY_MSEC		GENMASK(3, 0)
+
+/* bitmask of TOY_MATCH0/1/2_REG */
+#define TOY_MATCH_YEAR		GENMASK(31, 26)
+#define TOY_MATCH_MON		GENMASK(25, 22)
+#define TOY_MATCH_DAY		GENMASK(21, 17)
+#define TOY_MATCH_HOUR		GENMASK(16, 12)
+#define TOY_MATCH_MIN		GENMASK(11, 6)
+#define TOY_MATCH_SEC		GENMASK(5, 0)
+
+/* bitmask of RTC_CTRL_REG */
+#define RTC_ENABLE		BIT(13) /* 1: RTC counters enable */
+#define TOY_ENABLE		BIT(11) /* 1: TOY counters enable */
+#define OSC_ENABLE		BIT(8) /* 1: 32.768k crystal enable */
+#define TOY_ENABLE_MASK		(TOY_ENABLE | OSC_ENABLE)
+
+/* PM domain registers */
+#define PM1_STS_REG		0x0c	/* Power management 1 status register */
+#define RTC_STS			BIT(10)	/* RTC status */
+#define PM1_EN_REG		0x10	/* Power management 1 enable register */
+#define RTC_EN			BIT(10)	/* RTC event enable */
+
+/* Workaround for LS1X systems hanging when accessing RTC_CTRL_REG. */
+#define LS1X_RTC_CTRL_WORKAROUND	BIT(0)
+
+struct loongson_rtc_config {
+	u32 pm_offset;	/* Offset of PM domain, for RTC alarm */
+	u32 flags;	/* Workaround bits */
+};
+
+struct loongson_rtc_priv {
+	spinlock_t lock;	/* protects PM registers access */
+	u32 fix_year;		/* RTC alarm year compensation value */
+	struct rtc_device *rtcdev;
+	struct regmap *regmap;
+	void __iomem *pm_base;	/* PM domain base, for RTC alarm */
+	const struct loongson_rtc_config *config;
+};
+
+static const struct loongson_rtc_config generic_rtc_config = {
+	.pm_offset = 0x100,
+	.flags = 0,
+};
+
+static const struct loongson_rtc_config ls2k1000_rtc_config = {
+	.pm_offset = 0x800,
+	.flags = 0,
+};
+
+static const struct loongson_rtc_config ls1x_rtc_config = {
+	.pm_offset = 0,
+	.flags = LS1X_RTC_CTRL_WORKAROUND,
+};
+
+static const struct regmap_config loongson_rtc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+/* IRQ Handlers */
+static irqreturn_t loongson_rtc_isr(int irq, void *id)
+{
+	struct loongson_rtc_priv *priv = (struct loongson_rtc_priv *)id;
+
+	rtc_update_irq(priv->rtcdev, 1, RTC_AF | RTC_IRQF);
+	return IRQ_HANDLED;
+}
+
+static u32 loongson_rtc_handler(void *id)
+{
+	struct loongson_rtc_priv *priv = (struct loongson_rtc_priv *)id;
+
+	spin_lock(&priv->lock);
+	/* Disable RTC event */
+	writel(readl(priv->pm_base + PM1_EN_REG) & ~RTC_EN,
+	       priv->pm_base + PM1_EN_REG);
+
+	/* Clear RTC interrupt status */
+	writel(RTC_STS, priv->pm_base + PM1_STS_REG);
+	spin_unlock(&priv->lock);
+
+	/*
+	 * The TOY_MATCH0_REG should be cleared 0 here,
+	 * otherwise the interrupt cannot be cleared.
+	 */
+	return regmap_write(priv->regmap, TOY_MATCH0_REG, 0);
+}
+
+static int loongson_rtc_set_enabled(struct device *dev)
+{
+	struct loongson_rtc_priv *priv = dev_get_drvdata(dev);
+
+	if (priv->config->flags & LS1X_RTC_CTRL_WORKAROUND)
+		return 0;
+
+	/* Enable RTC TOY counters and crystal */
+	return regmap_update_bits(priv->regmap, RTC_CTRL_REG, TOY_ENABLE_MASK,
+				  TOY_ENABLE_MASK);
+}
+
+static bool loongson_rtc_get_enabled(struct device *dev)
+{
+	int ret;
+	u32 ctrl_data;
+	struct loongson_rtc_priv *priv = dev_get_drvdata(dev);
+
+	if (priv->config->flags & LS1X_RTC_CTRL_WORKAROUND)
+		return true;
+
+	ret = regmap_read(priv->regmap, RTC_CTRL_REG, &ctrl_data);
+	if (ret < 0)
+		return false;
+
+	return ctrl_data & TOY_ENABLE_MASK;
+}
+
+static int loongson_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	int ret;
+	u32 rtc_data[2];
+	struct loongson_rtc_priv *priv = dev_get_drvdata(dev);
+
+	if (!loongson_rtc_get_enabled(dev))
+		return -EINVAL;
+
+	ret = regmap_bulk_read(priv->regmap, TOY_READ0_REG, rtc_data,
+			       ARRAY_SIZE(rtc_data));
+	if (ret < 0)
+		return ret;
+
+	tm->tm_sec = FIELD_GET(TOY_SEC, rtc_data[0]);
+	tm->tm_min = FIELD_GET(TOY_MIN, rtc_data[0]);
+	tm->tm_hour = FIELD_GET(TOY_HOUR, rtc_data[0]);
+	tm->tm_mday = FIELD_GET(TOY_DAY, rtc_data[0]);
+	tm->tm_mon = FIELD_GET(TOY_MON, rtc_data[0]) - 1;
+	tm->tm_year = rtc_data[1];
+
+	/* Prepare for RTC alarm year compensation value. */
+	priv->fix_year = tm->tm_year / 64 * 64;
+	return 0;
+}
+
+static int loongson_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	int ret;
+	u32 rtc_data[2];
+	struct loongson_rtc_priv *priv = dev_get_drvdata(dev);
+
+	rtc_data[0] = FIELD_PREP(TOY_SEC, tm->tm_sec)
+		    | FIELD_PREP(TOY_MIN, tm->tm_min)
+		    | FIELD_PREP(TOY_HOUR, tm->tm_hour)
+		    | FIELD_PREP(TOY_DAY, tm->tm_mday)
+		    | FIELD_PREP(TOY_MON, tm->tm_mon + 1);
+	rtc_data[1] = tm->tm_year;
+
+	ret = regmap_bulk_write(priv->regmap, TOY_WRITE0_REG, rtc_data,
+				ARRAY_SIZE(rtc_data));
+	if (ret < 0)
+		return ret;
+
+	return loongson_rtc_set_enabled(dev);
+}
+
+static int loongson_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	int ret;
+	u32 alarm_data;
+	struct loongson_rtc_priv *priv = dev_get_drvdata(dev);
+
+	ret = regmap_read(priv->regmap, TOY_MATCH0_REG, &alarm_data);
+	if (ret < 0)
+		return ret;
+
+	alrm->time.tm_sec = FIELD_GET(TOY_MATCH_SEC, alarm_data);
+	alrm->time.tm_min = FIELD_GET(TOY_MATCH_MIN, alarm_data);
+	alrm->time.tm_hour = FIELD_GET(TOY_MATCH_HOUR, alarm_data);
+	alrm->time.tm_mday = FIELD_GET(TOY_MATCH_DAY, alarm_data);
+	alrm->time.tm_mon = FIELD_GET(TOY_MATCH_MON, alarm_data) - 1;
+	/*
+	 * This is a hardware bug: the year field of SYS_TOYMATCH is only 6 bits,
+	 * making it impossible to save year values larger than 64.
+	 *
+	 * SYS_TOYMATCH is used to match the alarm time value and determine if
+	 * an alarm is triggered, so we must keep the lower 6 bits of the year
+	 * value constant during the value conversion.
+	 *
+	 * In summary, we need to manually add 64(or a multiple of 64) to the
+	 * year value to avoid the invalid alarm prompt at startup.
+	 */
+	alrm->time.tm_year = FIELD_GET(TOY_MATCH_YEAR, alarm_data) + priv->fix_year;
+
+	alrm->enabled = !!(readl(priv->pm_base + PM1_EN_REG) & RTC_EN);
+	return 0;
+}
+
+static int loongson_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	u32 val;
+	struct loongson_rtc_priv *priv = dev_get_drvdata(dev);
+
+	spin_lock(&priv->lock);
+	val = readl(priv->pm_base + PM1_EN_REG);
+	/* Enable RTC alarm */
+	writel(enabled ? val | RTC_EN : val & ~RTC_EN,
+	       priv->pm_base + PM1_EN_REG);
+	spin_unlock(&priv->lock);
+
+	return 0;
+}
+
+static int loongson_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	int ret;
+	u32 alarm_data;
+	struct loongson_rtc_priv *priv = dev_get_drvdata(dev);
+
+	alarm_data = FIELD_PREP(TOY_MATCH_SEC, alrm->time.tm_sec)
+		   | FIELD_PREP(TOY_MATCH_MIN, alrm->time.tm_min)
+		   | FIELD_PREP(TOY_MATCH_HOUR, alrm->time.tm_hour)
+		   | FIELD_PREP(TOY_MATCH_DAY, alrm->time.tm_mday)
+		   | FIELD_PREP(TOY_MATCH_MON, alrm->time.tm_mon + 1)
+		   | FIELD_PREP(TOY_MATCH_YEAR, alrm->time.tm_year - priv->fix_year);
+
+	ret = regmap_write(priv->regmap, TOY_MATCH0_REG, alarm_data);
+	if (ret < 0)
+		return ret;
+
+	return loongson_rtc_alarm_irq_enable(dev, alrm->enabled);
+}
+
+static const struct rtc_class_ops loongson_rtc_ops = {
+	.read_time = loongson_rtc_read_time,
+	.set_time = loongson_rtc_set_time,
+	.read_alarm = loongson_rtc_read_alarm,
+	.set_alarm = loongson_rtc_set_alarm,
+	.alarm_irq_enable = loongson_rtc_alarm_irq_enable,
+};
+
+static int loongson_rtc_probe(struct platform_device *pdev)
+{
+	int ret, alarm_irq;
+	void __iomem *regs;
+	struct loongson_rtc_priv *priv;
+	struct device *dev = &pdev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs))
+		return dev_err_probe(dev, PTR_ERR(regs),
+				     "devm_platform_ioremap_resource failed\n");
+
+	priv->regmap = devm_regmap_init_mmio(dev, regs,
+					     &loongson_rtc_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(dev, PTR_ERR(priv->regmap),
+				     "devm_regmap_init_mmio failed\n");
+
+	priv->config = device_get_match_data(dev);
+	spin_lock_init(&priv->lock);
+	platform_set_drvdata(pdev, priv);
+
+	priv->rtcdev = devm_rtc_allocate_device(dev);
+	if (IS_ERR(priv->rtcdev))
+		return dev_err_probe(dev, PTR_ERR(priv->rtcdev),
+				     "devm_rtc_allocate_device failed\n");
+
+	/* Get RTC alarm irq */
+	alarm_irq = platform_get_irq(pdev, 0);
+	if (alarm_irq > 0) {
+		ret = devm_request_irq(dev, alarm_irq, loongson_rtc_isr,
+				       0, "loongson-alarm", priv);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Unable to request irq %d\n",
+					     alarm_irq);
+
+		priv->pm_base = regs - priv->config->pm_offset;
+		device_init_wakeup(dev, 1);
+
+		if (has_acpi_companion(dev))
+			acpi_install_fixed_event_handler(ACPI_EVENT_RTC,
+							 loongson_rtc_handler, priv);
+	} else {
+		/* Loongson-1 RTC does not support alarm */
+		clear_bit(RTC_FEATURE_ALARM, priv->rtcdev->features);
+	}
+
+	/* Loongson RTC does not support UIE */
+	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, priv->rtcdev->features);
+	priv->rtcdev->ops = &loongson_rtc_ops;
+	priv->rtcdev->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	priv->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
+
+	return devm_rtc_register_device(priv->rtcdev);
+}
+
+static void loongson_rtc_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct loongson_rtc_priv *priv = dev_get_drvdata(dev);
+
+	if (!test_bit(RTC_FEATURE_ALARM, priv->rtcdev->features))
+		return;
+
+	if (has_acpi_companion(dev))
+		acpi_remove_fixed_event_handler(ACPI_EVENT_RTC,
+						loongson_rtc_handler);
+
+	device_init_wakeup(dev, 0);
+	loongson_rtc_alarm_irq_enable(dev, 0);
+}
+
+static const struct of_device_id loongson_rtc_of_match[] = {
+	{ .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
+	{ .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
+	{ .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
+	{ .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
+	{ .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
+	{ .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, loongson_rtc_of_match);
+
+static const struct acpi_device_id loongson_rtc_acpi_match[] = {
+	{ "LOON0001", .driver_data = (kernel_ulong_t)&generic_rtc_config },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, loongson_rtc_acpi_match);
+
+static struct platform_driver loongson_rtc_driver = {
+	.probe		= loongson_rtc_probe,
+	.remove_new	= loongson_rtc_remove,
+	.driver		= {
+		.name	= "loongson-rtc",
+		.of_match_table = loongson_rtc_of_match,
+		.acpi_match_table = loongson_rtc_acpi_match,
+	},
+};
+module_platform_driver(loongson_rtc_driver);
+
+MODULE_DESCRIPTION("Loongson RTC driver");
+MODULE_AUTHOR("Binbin Zhou <zhoubinbin@loongson.cn>");
+MODULE_AUTHOR("WANG Xuerui <git@xen0n.name>");
+MODULE_AUTHOR("Huacai Chen <chenhuacai@kernel.org>");
+MODULE_LICENSE("GPL");
-- 
2.39.1

[PATCH V4 4/5] MIPS: Loongson64: DTS: Add RTC support to LS7A PCH

[Binbin Zhou]

The RTC module is now supported, enable it.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
Signed-off-by: WANG Xuerui <git@xen0n.name>
Acked-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/boot/dts/loongson/ls7a-pch.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/mips/boot/dts/loongson/ls7a-pch.dtsi b/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
index 2f45fce2cdc4..7c69e8245c2f 100644
--- a/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
+++ b/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
@@ -19,6 +19,13 @@ pic: interrupt-controller@10000000 {
 			#interrupt-cells = <2>;
 		};
 
+		rtc0: rtc@100d0100 {
+			compatible = "loongson,ls7a-rtc";
+			reg = <0 0x100d0100 0 0x78>;
+			interrupt-parent = <&pic>;
+			interrupts = <52 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
 		ls7a_uart0: serial@10080000 {
 			compatible = "ns16550a";
 			reg = <0 0x10080000 0 0x100>;
-- 
2.39.1

[PATCH V4 5/5] MIPS: Loongson64: DTS: Add RTC support to Loongson-2K1000

[Binbin Zhou]

The module is now supported, enable it.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
Signed-off-by: WANG Xuerui <git@xen0n.name>
Acked-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi b/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
index 8143a61111e3..f878f47e4501 100644
--- a/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
+++ b/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
@@ -97,6 +97,13 @@ liointc1: interrupt-controller@1fe11440 {
 						<0x00000000>; /* int3 */
 		};
 
+		rtc0: rtc@1fe07800 {
+			compatible = "loongson,ls2k1000-rtc";
+			reg = <0 0x1fe07800 0 0x78>;
+			interrupt-parent = <&liointc0>;
+			interrupts = <60 IRQ_TYPE_LEVEL_LOW>;
+		};
+
 		uart0: serial@1fe00000 {
 			compatible = "ns16550a";
 			reg = <0 0x1fe00000 0 0x8>;
-- 
2.39.1