linux-kernel - Re: [PATCH v1 09/30] reset: starfive: jh7100: Move necessary properties to device tree

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]

Date:   Wed, 5 Oct 2022 15:20:06 +0200
From:   Emil Renner Berthing <emil.renner.berthing@...onical.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Hal Feng <hal.feng@...ux.starfivetech.com>,
        linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-gpio@...r.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Emil Renner Berthing <kernel@...il.dk>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 09/30] reset: starfive: jh7100: Move necessary
 properties to device tree

On Fri, 30 Sept 2022 at 22:51, Rob Herring <robh@...nel.org> wrote:
>
> On Thu, Sep 29, 2022 at 10:32:04PM +0800, Hal Feng wrote:
> > Store the necessary properties in device tree instead of .c file,
> > in order to apply this reset driver to other StarFive SoCs.
> >
> > Signed-off-by: Hal Feng <hal.feng@...ux.starfivetech.com>
> > ---
> >  .../bindings/reset/starfive,jh7100-reset.yaml | 20 ++++++++
> >  arch/riscv/boot/dts/starfive/jh7100.dtsi      |  3 ++
> >  drivers/reset/reset-starfive-jh7100.c         | 50 +++++++++++++------
> >  3 files changed, 57 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
> > index 300359a5e14b..3eff3f72a1ed 100644
> > --- a/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
> > +++ b/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
> > @@ -20,19 +20,39 @@ properties:
> >    "#reset-cells":
> >      const: 1
> >
> > +  starfive,assert-offset:
> > +    description: Offset of the first ASSERT register
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  starfive,status-offset:
> > +    description: Offset of the first STATUS register
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  starfive,nr-resets:
> > +    description: Number of reset signals
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> >  required:
> >    - compatible
> >    - reg
> >    - "#reset-cells"
> > +  - starfive,assert-offset
> > +  - starfive,status-offset
> > +  - starfive,nr-resets
>
> Adding required properties is a red flag. You can't add required
> properties to an existing binding. That breaks the ABI unless the OS
> deals with the properties being absent. If the OS has to do that, then
> why add them in the first place? All this should be implied by the
> compatible string.

Indeed. I really don't understand why this is even necessary. As
mentioned in my reply to the clock driver my original code just had a
combined driver for the whole CRG (clock and reset generator I
presume), and then you just need a simple node like this:

syscrg: clock-controller@...20000 {
    compatible = "starfive,jh7110-syscrg";
    reg = <0x0 0x13020000 0x0 0x10000>;
    clocks = <&osc>, <&gmac1_rmii_refin>,
             <&gmac1_rgmii_rxin>,
             <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
             <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
             <&tdm_ext>, <&mclk_ext>;
    clock-names = "osc", "gmac1_rmii_refin",
                  "gmac1_rgmii_rxin",
                  "i2stx_bclk_ext", "i2stx_lrck_ext",
                  "i2srx_bclk_ext", "i2srx_lrck_ext",
                  "tdm_ext", "mclk_ext";
    #clock-cells = <1>;
    #reset-cells = <1>;
};

/Emil