Re: [PATCH v3 05/11] spi: dw: Add Pensando Elba SoC SPI Controller bindings

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Tue, Nov 16, 2021 at 03:11:53PM -0800, Brad Larson wrote:
> Hi Sergey,
> 
> On Tue, Nov 16, 2021 at 3:29 AM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> >
> > > > AFAICS from the driver-part of the patchset it's not enough. You've
> > > > also got the syscon phandle, which needs to be reflected in the
> > > > bindings. That also makes me thinking that you didn't perform the
> > > > "dtbs_check" on the dts-files you were going to submit, but for some
> > > > reason discarded from this series (btw why?). If you did you would
> > > > have got an error of an unevaluated property detection.
> > >
> > > I ran the checks below and didn't get errors.  Rob provided some info
> > > and I found the server did not have yamllint installed (not flagged by
> > > tool).  Also dt-schema was not the latest.  I'm re-doing this and
> > > including "DT_CHECKER_FLAGS=-m" as that is new with v5.13.
> > >
> >
> > > make ARCH=arm64 dtbs_check
> > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> > > make ARCH=arm64 dtbs_check
> > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > > make ARCH=arm64 dtbs_check
> > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > > make ARCH=arm64 dtbs_check
> > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/vendor-prefixes.yaml
> > > make ARCH=arm64 dtbs_check
> > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/pensando,elba.yaml
> >
> > Hmm, that's weird. Rob, does dtschema tool have the
> > "unevaluatedProperties" property support?
> >
> > Brad, anyway you still need to add the syscon-property (pensando,*spics)
> > requirement in the snps,dw-apb-ssi.yaml schema. See the way it's done there
> > for instance for "baikal,bt1-sys-ssi" when it comes to the
> > vendor-specific properties definition in the allOf composition block.
> > You'll need to define a custom phandle property there in case if a
> > DT-node is compatible with you SPI controller.
> 
> Updating and adding only this bindings update to file
> snps,dw-apb-ssi.yaml in 5.16.0-rc1 (next-20211116):
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index d7e08b03e204..99deb587a47b 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -37,6 +37,21 @@ allOf:
>      else:
>        required:
>          - interrupts
> +  - if:
> +      properties:

> +        compatible:
> +          contains:
> +            enum:
> +              - pensando,elba-spics

I was wrong using that construction here (fixup patch would be very
welcome) seeing the "snps,dw-apb-ssi" doesn't permit having a generic
"snps,dw*" compatible string. So just const-compatible property should
be enough:

+        compatible:
+          const: pensando,elba-spics


> +    then:
> +      properties:
> +        pensando,spics:
> +          $ref: /schemas/types.yaml#/definitions/phandle
> +          description:
> +            Phandle to the system control device node which provides access to
> +            the spics control register
> +      required:

> +        - pensando,spics

Please note, I've asked to be more specific in this property naming.
Something like this should be fine
"pensando,elba-syscon-spics"/"pensando,syscon-spics".

> 
>  properties:
>    compatible:
> @@ -73,6 +88,8 @@ properties:
>                - renesas,r9a06g032-spi # RZ/N1D
>                - renesas,r9a06g033-spi # RZ/N1S
>            - const: renesas,rzn1-spi   # RZ/N1
> +      - description: Pensando Elba SoC SPI Controller
> +        const: pensando,elba-spics
> 
>    reg:
>      minItems: 1
> 
> $ make ARCH=arm64 defconfig
> ...
> 

> $ make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml

I am kind of surprised that this command is still evaluating all
the schemas. Compiling only the depended DT-schemas would much
better... Anyway that's why you are getting unrelated to the
snps,dw-apb-ssi.yaml errors.

> ...
>   DTEX    Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.example.dts
>   DTC     Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.example.dt.yaml
>   CHECK   Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.example.dt.yaml
> Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.example.dt.yaml:0:0:
> /example-0/spi@fff00000/flash@1: failed to match any schema with
> compatible: ['spi-nand']

That means there is no bindings defined for the "spi-nand"-compatible
node. AFAICS it doesn't make the tool to fail though. Anyway fixing this
part would be a subject of additional patches (which would be very
appreciated). That would concern converting the
Documentation/devicetree/bindings/mtd/spi-nand.txt legacy bindings to
the DT-schema. It's not enough though. Seeing that file lacks of
the NAND Flash specific DT-property description, you'd need to detach
ones (described by the "^nand@[a-f0-9]$"-pattern property) from the
Documentation/devicetree/bindings/mtd/nand-controller.yaml schema and
place them into a separate DT-schema file for generic nand-flashes
Documentation/devicetree/bindings/mtd/nand-flash.yaml in a framework
of a pre-requisite patch. Than in the legacy bindings conversion patch
you'd need to use it to correctly evaluate a generic NAND flash node.
Adding some example properties to the DT-schema would be also
required.

-Sergey

> 
> The spi-nand schema match failure happens before I make any change.
> The tool also throws errors for these files which are unrelated
> 
> Documentation/devicetree/bindings/net/qcom,ipa.yaml: ignoring, error
> in schema: properties: qcom,smem-state-names
> Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml:
> ignoring, error in schema: patternProperties: ^filter@[0-9]+$:
> properties: st,adc-channel-names
> Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml:
> ignoring, error in schema: properties: qcom,bcm-voter-names
> 
> Thanks,
> Brad