Re: [PATCH v2 03/14] dt-bindings: PCI: Add bindings for more Brcmstb chips

[Jim Quinlan]

On Tue, Jun 2, 2020 at 5:41 PM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Tue, Jun 2, 2020 at 2:53 PM Jim Quinlan <james.quinlan@xxxxxxxxxxxx> wrote:
> >
> > On Fri, May 29, 2020 at 1:46 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > On Tue, May 26, 2020 at 03:12:42PM -0400, Jim Quinlan wrote:
> > > > From: Jim Quinlan <jquinlan@xxxxxxxxxxxx>
> > > >
> > > > - Add compatible strings for three more Broadcom STB chips: 7278, 7216,
> > > >   7211 (STB version of RPi4).
> > > > - add new property 'brcm,scb-sizes'
> > > > - add new property 'resets'
> > > > - add new property 'reset-names'
> > > > - allow 'ranges' and 'dma-ranges' to have more than one item and update
> > > >   the example to show this.
> > > >
> > > > Signed-off-by: Jim Quinlan <jquinlan@xxxxxxxxxxxx>
> > > > ---
> > > >  .../bindings/pci/brcm,stb-pcie.yaml           | 40 +++++++++++++++++--
> > > >  1 file changed, 36 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > index 8680a0f86c5a..66a7df45983d 100644
> > > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > @@ -14,7 +14,13 @@ allOf:
> > > >
> > > >  properties:
> > > >    compatible:
> > > > -    const: brcm,bcm2711-pcie # The Raspberry Pi 4
> > > > +    items:
> > > > +      - enum:
> > >
> > > Don't need items here. Just change the const to enum.
> > >
> > > > +          - brcm,bcm2711-pcie # The Raspberry Pi 4
> > > > +          - brcm,bcm7211-pcie # Broadcom STB version of RPi4
> > > > +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> > > > +          - brcm,bcm7216-pcie # Broadcom 7216 Arm
> > > > +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> > > >
> > > >    reg:
> > > >      maxItems: 1
> > > > @@ -34,10 +40,12 @@ properties:
> > > >        - const: msi
> > > >
> > > >    ranges:
> > > > -    maxItems: 1
> > > > +    minItems: 1
> > > > +    maxItems: 4
> > > >
> > > >    dma-ranges:
> > > > -    maxItems: 1
> > > > +    minItems: 1
> > > > +    maxItems: 6
> > > >
> > > >    clocks:
> > > >      maxItems: 1
> > > > @@ -58,8 +66,30 @@ properties:
> > > >
> > > >    aspm-no-l0s: true
> > > >
> > > > +  resets:
> > > > +    description: for "brcm,bcm7216-pcie", must be a valid reset
> > > > +      phandle pointing to the RESCAL reset controller provider node.
> > > > +    $ref: "/schemas/types.yaml#/definitions/phandle"
> > > > +
> > > > +  reset-names:
> > > > +    items:
> > > > +      - const: rescal
> > >
> > > These are going to need to be an if/then schema if they only apply to
> > > certain compatible(s).
> >
> > Why is that -- the code is general enough to handle its presence or
> > not (it is an optional compatibility)>
>
> Because an if/then schema expresses in a parse-able form what your
> 'description' does in free form text.
>
> Presumably a 'resets' property for !brcm,bcm7216-pcie is invalid, so
> we should check that. The schema shouldn't be just what some driver
> happens to currently allow. Also, it's not a driver's job to validate
> DT, so it shouldn't check any of this.
Got it, will fix.
>
> > > > +  brcm,scb-sizes:
> > > > +    description: (u32, u32) tuple giving the 64bit PCIe memory
> > > > +      viewport size of a memory controller.  There may be up to
> > > > +      three controllers, and each size must be a power of two
> > > > +      with a size greater or equal to the amount of memory the
> > > > +      controller supports.
> > >
> > > This sounds like what dma-ranges should express?
> >
> > There is some overlap but this contains information that is not in the
> > dma-ranges.  Believe me I tried.
>
> I don't understand. If you had 3 dma-ranges entries, you'd have 3
> sizes. Can you expand or show me what you tried?
Here is a simple example: suppose you have two dma-ranges.  This could
be from one of two cases:

- Both dma-ranges are from the same memory controller (the second
range is the "extended" region).
- One dma-range can be from memc0 and the other can be from memc1; the
extensions are not used.

The driver has to determine (a)  how many memory controllers there are
and (b) what the size should be for each of them.  It is impossible to
do this with the case above.

>
> > > If not, we do have 64-bit size if that what you need.
> >
> > IIRC I tried the 64-bit size but the YAML validator did not like it;
> > it wanted numbers like  <0x1122334455667788> while dtc wanted <
> > 0x11223344 0x55667788>.  I gave up trying and switched to u32.
>
> You used the /bits/ annotation for dtc?:
>
> /bits/ 64 <0x1122334455667788>
>
> I also made a recent fix to dt-schema around handling of 64-bit sizes,
> so update if you have problems still.
I didn't try the /bits/ so I'll pursue this.

Thanks,
Jim

>
> Rob