Re: [PATCH v3 01/23] dt-bindings: ata: ahci-platform: Drop dma-coherent property declaration

[Rob Herring <robh@kernel.org>]

On Sat, May 21, 2022 at 12:22:48PM +0300, Serge Semin wrote:
> On Tue, May 17, 2022 at 01:58:41PM -0500, Rob Herring wrote:
> > On Thu, May 12, 2022 at 02:17:48AM +0300, Serge Semin wrote:
> > > It's redundant to have the 'dma-coherent' property explicitly specified in
> > > the DT schema because it's a generic property described in the core
> > > DT-schema by which the property is always evaluated.
> > 
> 
> > It is not redundant.
> > 
> > The core schema defines the property (as a boolean), but this schema 
> > defines it being used in this binding. Otherwise, it won't be allowed.
> 
> I thought that the generic properties like ranges, dma-ranges, etc
> including the dma-coherent one due to being defined in the dt-core
> schema are always evaluated. As such seeing the unevaluatedProperties
> property is set to false here, they can be used in the DT-nodes with
> no need to be explicitly specified in the DT node bindings. In
> addition to that I tested this assumption by dropping the dma-coherent
> property definition from the AHCI-common schema and executed the
> DT-bindings check procedure. No error has been spotted:

Those common properties are always applied, but not at the same time as 
a device binding. IOW, it's 2 schemas that are applied to an instance 
(node) independently. For things like 'reg', the common schema does type 
checks and the device schema does size (number of entries) checks.

There a few things always allowed like 'status', and those are added to 
the device schema by the tools.

> 
> > [fancer@mobilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml | grep dma-coherent
> >        dma-coherent;
> > [fancer@mobilestation] kernel $ make -j8 DT_SCHEMA_FILES=Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml dt_binding_check
> >   LINT    Documentation/devicetree/bindings
> >   DTEX    Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts
> >   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> >   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> >   DTC     Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dtb
> >   CHECK   Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dtb
> > [fancer@mobilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts | grep dma-coherent
> >           dma-coherent;
> > [fancer@mobilestation] kernel $ echo $?
> > 0
> Due to that here are a few backward questions:
> 1) Am I doing something wrong in the framework of the DT-bindings
> evaluation? Really I even tried to specify unknown property in the
> DT-bindings example like "bla-bla-bla;" and no evaluation error was
> printed. Anyway If what you are saying was correct I would have got an
> error during the DT-bindings evaluation, but as you can see there was
> none.

I think this is a known issue which has a pending fix. If a referenced 
schema has 'additionalProperties: true' in it, then the referring schema 
never has any unevaluated properties. The fix is pending because all 
the schema examples that start failing have to be fixed and in a base 
that people work on (i.e. rc1).

> 2) Am I wrong in thinking that the unevaluatedProperties setting
> concerns the generic properties defined in the DT-core schema? 

You are wrong as explained above.

> If it
> doesn't concern the generic properties then does it work for the
> $ref'ed schemas only? 

Yes, except for the issue making it not work.

> Getting back to the patch topic. We need to drop the dma-coherent
> property from the schema anyway. AHCI-specification doesn't
> regulate the DMA operations coherency. The dma-coherent property is
> more specific to the particular controller implementation mainly
> dependent on the platform settings. So I'll change the patch log, but
> get to keep the patch in the series. What do you think?

Intel wrote the spec, so they probably assume coherent. In DT, PPC is 
default coherent and Arm is default non-coherent.

You'll need to add it to whatever specific device schemas need it if you 
remove it. Personally, I think it is fine where it is. dma-coherent is 
valid on any DMA capable device and it's not really a property of the 
device, but the system. If we could generically identify DMA capable 
devices, then dma-coherent would be allowed on them automatically.

Rob