Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 30 Mar 2021 01:21:33 +0300
From:   Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
To:     Rob Herring <robh@kernel.org>
Cc:     Andreas =?iso-8859-1?Q?F=E4rber?= <afaerber@suse.de>,
        Manivannan Sadhasivam <mani@kernel.org>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-actions@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] dt-bindings: soc: actions: Add Actions Semi Owl
 socinfo binding
Message-ID: <20210329222133.GA1792209@BV030612LT>
References: <cover.1616178258.git.cristian.ciocaltea@gmail.com>
 <95105518f61408743d17783099ed9c373a3dfe18.1616178258.git.cristian.ciocaltea@gmail.com>
 <20210327163006.GA185779@robh.at.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210327163006.GA185779@robh.at.kernel.org>
Precedence: bulk

On Sat, Mar 27, 2021 at 10:30:06AM -0600, Rob Herring wrote:
> On Fri, Mar 19, 2021 at 08:27:59PM +0200, Cristian Ciocaltea wrote:
> > Add devicetree binding for the Actions Semi Owl SoCs info module.
> > 
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > ---
> >  .../bindings/soc/actions/owl-socinfo.yaml     | 71 +++++++++++++++++++

[...]

> > +
> > +patternProperties:
> > +  "^soc(@[0-9a-f]+)?$":
> 
> Make this a $nodename property.
> 
> > +    type: object
> > +    properties:
> 
> And move this up to top-level.
> 
> You need a custom 'select' entry to exclude 'simple-bus'.

Indeed, I missed it..

> > +      compatible:
> > +        items:
> > +          - enum:
> > +              - actions,s500-soc
> > +              - actions,s700-soc
> > +              - actions,s900-soc
> > +          - const: simple-bus
> > +
> > +      "#address-cells":
> > +        enum: [1, 2]
> > +
> > +      "#size-cells":
> > +        enum: [1, 2]
> > +
> > +      ranges: true
> > +
> > +      actions,serial-number-addrs:
> > +        description: |
> > +          Contains the physical addresses in DDR memory where the two parts
> > +          of the serial number (low & high) can be read from.
> > +          This is currently supported only on the S500 SoC variant.
> > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > +        minItems: 2
> > +        maxItems: 2
> 
> Humm, it doesn't really seem you have an actual device or bus here, but 
> are abusing DT to create your socinfo device.
> 
> As the only property is data in main memory, you should do a compatible 
> for that memory region and put it under reserved-memory. You need that 
> anyway to prevent the kernel from using the memory, right?

Right, this region should be exposed as reserved-memory. Will handle
it in the next revision.

> Rob

Thanks for the review,
Cristi