On Fri, 30 Sept 2022 at 22:51, Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > .../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@13020000 {
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
