mbox series

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

Message ID cover.1684983279.git.zhoubinbin@loongson.cn
Headers show
Series rtc: Add rtc driver for the Loongson family chips | expand

Message

Binbin Zhou May 25, 2023, 12:55 p.m. UTC
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

Comments

Conor Dooley May 25, 2023, 5:05 p.m. UTC | #1
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
>
Binbin Zhou May 26, 2023, 1:37 a.m. UTC | #2
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
> >
Conor Dooley May 26, 2023, 12:06 p.m. UTC | #3
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.
Jiaxun Yang May 26, 2023, 12:22 p.m. UTC | #4
> 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.
Conor Dooley May 26, 2023, 12:38 p.m. UTC | #5
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.
Binbin Zhou May 27, 2023, 9:22 a.m. UTC | #6
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.
Jiaxun Yang May 27, 2023, 4:13 p.m. UTC | #7
> 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
Conor Dooley May 27, 2023, 4:23 p.m. UTC | #8
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.
Jiaxun Yang May 27, 2023, 9:59 p.m. UTC | #9
> 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.
Conor Dooley May 27, 2023, 10:22 p.m. UTC | #10
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.
Alexandre Belloni May 29, 2023, 10:20 p.m. UTC | #11
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.
> >