Message ID | cover.1684983279.git.zhoubinbin@loongson.cn |
---|---|
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.
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. > >