| Message ID | 9a2fbd6860f37760ca6089c150fd6f67628405f6.1684983279.git.zhoubinbin@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: Add rtc driver for the Loongson family chips | expand |
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 >
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 > >
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.
> 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.
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.
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.
> 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
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.
> 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.
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.
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.
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.
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.
>
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. > >
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
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
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.
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
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)
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
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 >
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 > >
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
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