Message-ID: <20220525100146.3xg3vjj4edizl6yb@mobilestation>
Date:   Wed, 25 May 2022 13:01:46 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Jens Axboe <axboe@...nel.dk>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v3 01/23] dt-bindings: ata: ahci-platform: Drop
 dma-coherent property declaration

On Tue, May 24, 2022 at 09:57:58AM -0500, Rob Herring wrote:
> 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.

It makes sense now. Thanks for clarification.

> 
> > 
> > > [fancer@...ilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml | grep dma-coherent
> > >        dma-coherent;
> > > [fancer@...ilestation] 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@...ilestation] kernel $ cat Documentation/devicetree/bindings/ata/snps,dwc-ahci.example.dts | grep dma-coherent
> > >           dma-coherent;
> > > [fancer@...ilestation] 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).

Ok. I see. Just to note in case if a non-related schema error is
found the unknown property error is printed too. Like this:

/.../ata/snps,dwc-ahci.example.dtb: sata@...f0000: interrupts: [[0, 115, 4], [0, 116, 4]] is too long
        From schema: /.../ata/snps,dwc-ahci.yaml
/../ata/snps,dwc-ahci.example.dtb: sata@...f0000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'phys', 'phy-names', 'ports-implemented', 'bla-bla-bla' were unexpected)

If I fix the interrupts-property error, the dt-schema check procedure
will work just fine.

> 
> > 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.

Right. This is what I was going to add to the patch log.

> 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.

Right. It is mainly the platform property. In particular the DMA
coherency is determined by the system interconnect design. In our case
the l1 and l2 caches are embedded into the CPU cores block while the
DDR and other SoC peripheral devices/controllers are attached to the
cores via a dedicated AXI3 interconnect bus, which has nothing to do
with the caches. That's why none of the system devices are
cache-coherent.

> If we could generically identify DMA capable 
> devices, then dma-coherent would be allowed on them automatically.

Got it. I'll drop this patch then.

-Sergey

> 
> Rob

Powered by blists - more mailing lists