Re: [PATCH 1/4] dt-bindings: soc: actions: Add Actions Semi Owl socinfo binding

[Cristian Ciocaltea]

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@xxxxxxxxx>
> > ---
> >  .../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