| Message ID | 20220511231810.4928-2-Sergey.Semin@baikalelectronics.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci: Add DWC/Baikal-T1 AHCI SATA support | expand |
On 5/12/22 01:17, 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. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v2: > - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel. > --- > Documentation/devicetree/bindings/ata/ahci-platform.yaml | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml > index c146ab8e14e5..9304e4731965 100644 > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml > @@ -87,8 +87,6 @@ properties: > description: > regulator for AHCI controller > > - dma-coherent: true > - > phy-supply: > description: > regulator for PHY power Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
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. Rob
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: > [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. 2) Am I wrong in thinking that the unevaluatedProperties setting concerns the generic properties defined in the DT-core schema? If it doesn't concern the generic properties then does it work for the $ref'ed schemas only? 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? -Sergey > > Rob
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
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@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). 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@122f0000: interrupts: [[0, 115, 4], [0, 116, 4]] is too long From schema: /.../ata/snps,dwc-ahci.yaml /../ata/snps,dwc-ahci.example.dtb: sata@122f0000: 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
diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml index c146ab8e14e5..9304e4731965 100644 --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml @@ -87,8 +87,6 @@ properties: description: regulator for AHCI controller - dma-coherent: true - phy-supply: description: regulator for PHY power
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. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- Changelog v2: - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel. --- Documentation/devicetree/bindings/ata/ahci-platform.yaml | 2 -- 1 file changed, 2 deletions(-)