lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241015212717.GA1983714-robh@kernel.org>
Date: Tue, 15 Oct 2024 16:27:17 -0500
From: Rob Herring <robh@...nel.org>
To: Ciprian Costea <ciprianmarian.costea@....nxp.com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-rtc@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	NXP S32 Linux Team <s32@....com>,
	Christophe Lizzi <clizzi@...hat.com>,
	Alberto Ruiz <aruizrui@...hat.com>,
	Enric Balletbo <eballetb@...hat.com>,
	Bogdan-Gabriel Roman <bogdan-gabriel.roman@....com>,
	Ghennadi Procopciuc <ghennadi.procopciuc@....com>
Subject: Re: [PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3
 SoCs

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@....nxp.com>
> > 
> > This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
> > 
> > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@....com>
> > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@....com>
> > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
> > Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
> > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@....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@....com>
> > +  - Ciprian Marian Costea <ciprianmarian.costea@....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@...60000 {
> > +        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.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ