Re: [PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 15/10/2024 16:27:17-0500, Rob Herring wrote:
> On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote:
> > On Tue, Oct 15, 2024 at 01:51:30PM +0300, Ciprian Costea wrote:
> > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > 
> > > This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
> > > 
> > > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> > > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> > > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > > Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > ---
> > >  .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 ++++++++++++++++++
> > >  1 file changed, 102 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > new file mode 100644
> > > index 000000000000..3a77d4dd8f3d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > @@ -0,0 +1,102 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: NXP S32G2/S32G3 Real Time Clock (RTC)
> > > +
> > > +maintainers:
> > > +  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
> > > +  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - enum:
> > > +          - nxp,s32g2-rtc
> > > +      - items:
> > > +          - const: nxp,s32g3-rtc
> > > +          - const: nxp,s32g2-rtc
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  "#clock-cells":
> > > +    const: 1
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: ipg clock drives the access to the
> > > +          RTC iomapped registers
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: ipg
> > > +
> > > +  assigned-clocks:
> > > +    minItems: 1
> > > +    items:
> > > +      - description: Runtime clock source. It must be a clock
> > > +            source for the RTC module. It will be disabled by hardware
> > > +            during Standby/Suspend.
> > > +      - description: Standby/Suspend clock source. It is optional
> > > +            and can be used in case the RTC will continue ticking during
> > > +            platform/system suspend. RTC hardware module contains a
> > > +            hardware mux for clock source selection.
> > 
> > If the RTC h/w contains a mux, then your mux inputs should be listed in 
> > 'clocks', not here.
> > 
> > > +
> > > +  assigned-clock-parents:
> > > +    description: List of phandles to each parent clock.
> > > +
> > > +  assigned-clock-rates:
> > > +    description: List of frequencies for RTC clock sources.
> > > +            RTC module contains 2 hardware divisors which can be
> > > +            enabled or not. Hence, available frequencies are the following
> > > +            parent_freq, parent_freq / 512, parent_freq / 32 or
> > > +            parent_freq / (512 * 32)
> > 
> > In general, assigned-clocks* do not need to be documented and should 
> > never be required.
> > 
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - "#clock-cells"
> > > +  - clocks
> > > +  - clock-names
> > > +  - assigned-clocks
> > > +  - assigned-clock-parents
> > > +  - assigned-clock-rates
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    rtc0: rtc@40060000 {
> > > +        compatible = "nxp,s32g3-rtc",
> > > +                   "nxp,s32g2-rtc";
> > > +        reg = <0x40060000 0x1000>;
> > > +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
> > > +        #clock-cells = <1>;
> > > +        clocks = <&clks 54>;
> > > +        clock-names = "ipg";
> > > +        /*
> > > +         * Configuration of default parent clocks.
> > > +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
> > > +         * 4-7 IDs are Suspend/Standby clock sources.
> > > +         */
> > > +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
> > 
> > That's weird...
> > 
> > > +        assigned-clock-parents = <&clks 56>, <&clks 55>;
> > 
> > I'd expect these should be in 'clocks'. I don't think this node should 
> > be a clock provider unless it provides a clock to something outside the 
> > RTC.
> > 
> > Looks like you are just using assigned-clocks to configure the clock mux 
> > in the RTC. That's way over complicated. Just define a vendor specific 
> > property with the mux settings. 
> 
> I just read v1 and got told use the clock framework...
> 
> I disagree completely. Tons of h/w blocks have the ability to select 
> (internal to the block) from multiple clock sources. Making the block a 
> clock provider to itself is completely pointless and an overkill, and 
> we *never* do that. Any display controller or audio interface has 
> mutiple clock sources as just 2 examples.

And in 6 months, we are going to learn that the rtc is used to clock the
wifi chip or whatever and we are going to need to add everything in the
CCF and we will have an unused property that we are going to have to
support forever to avoid breaking the ABI. This already happened...

> 
> However, I don't see why you need the divider config in DT. Can't you 
> figure out what divider you need based on input frequency? The output 
> frequency should be fixed, right?
> 
> Rob

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