| Message ID | 20220508142624.491045-2-matthew.gerlach@linux.intel.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add device tree for Intel n6000 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 08/05/2022 16:26, matthew.gerlach@linux.intel.com wrote: > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > Add device tree bindings documentation for the Intel Hard > Processor System (HPS) Copy Engine. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Mon, 9 May 2022, Krzysztof Kozlowski wrote: > On 08/05/2022 16:26, matthew.gerlach@linux.intel.com wrote: >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> >> Add device tree bindings documentation for the Intel Hard >> Processor System (HPS) Copy Engine. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Hi Krzysztof, Thank you for the review. I will add your tag and submit a v5 patchset. Matthew > > > Best regards, > Krzysztof >
On Sun, May 08, 2022 at 07:26:22AM -0700, matthew.gerlach@linux.intel.com wrote: > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > Add device tree bindings documentation for the Intel Hard > Processor System (HPS) Copy Engine. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > v4: > - move from soc to soc/intel/ > > v3: > - remove unused label > - move from misc to soc > - remove 0x from #address-cells/#size-cells values > - change hps_cp_eng@0 to dma-controller@0 > - remote inaccurate 'items:' tag > --- > .../soc/intel/intel,hps-copy-engine.yaml | 51 +++++++++++++++++++ > 1 file changed, 51 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml > > diff --git a/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml b/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml > new file mode 100644 > index 000000000000..8634865015cd > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml > @@ -0,0 +1,51 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (C) 2022, Intel Corporation > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/soc/intel/intel,hps-copy-engine.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: Intel HPS Copy Engine > + > +maintainers: > + - Matthew Gerlach <matthew.gerlach@linux.intel.com> > + > +description: | > + The Intel Hard Processor System (HPS) Copy Engine is an IP block used to copy > + a bootable image from host memory to HPS DDR. Additionally, there is a > + register the HPS can use to indicate the state of booting the copied image as > + well as a keep-a-live indication to the host. > + > +properties: > + compatible: > + const: intel,hps-copy-engine > + > + '#dma-cells': > + const: 1 > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + bus@80000000 { > + compatible = "simple-bus"; > + reg = <0x80000000 0x60000000>, > + <0xf9000000 0x00100000>; > + reg-names = "axi_h2f", "axi_h2f_lw"; A simple-bus doesn't have regs because it is simple. If you have registers, then you need a specific compatible. You can have 'simple-bus' as a fallback if the bus is completely setup by firmware and the OS never needs to configure/manage it. It also looks odd that ranges only has 4K of bus space and the bus registers are 1.5GB of space. That's all kind of outside of the scope of this binding and you should just drop that part. > + #address-cells = <2>; > + #size-cells = <1>; > + ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>; > + > + dma-controller@0 { > + compatible = "intel,hps-copy-engine"; > + reg = <0x00000000 0x00000000 0x00001000>; > + #dma-cells = <1>; > + }; > + }; > -- > 2.25.1 > >
On Sun, May 08, 2022 at 07:26:22AM -0700, matthew.gerlach@linux.intel.com wrote: > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > Add device tree bindings documentation for the Intel Hard > Processor System (HPS) Copy Engine. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > v4: > - move from soc to soc/intel/ > > v3: > - remove unused label > - move from misc to soc > - remove 0x from #address-cells/#size-cells values > - change hps_cp_eng@0 to dma-controller@0 > - remote inaccurate 'items:' tag > --- > .../soc/intel/intel,hps-copy-engine.yaml | 51 +++++++++++++++++++ Also, this should go under bindings/dma/ > 1 file changed, 51 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml
On Tue, 10 May 2022, Rob Herring wrote: > On Sun, May 08, 2022 at 07:26:22AM -0700, matthew.gerlach@linux.intel.com wrote: >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> >> Add device tree bindings documentation for the Intel Hard >> Processor System (HPS) Copy Engine. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> v4: >> - move from soc to soc/intel/ >> >> v3: >> - remove unused label >> - move from misc to soc >> - remove 0x from #address-cells/#size-cells values >> - change hps_cp_eng@0 to dma-controller@0 >> - remote inaccurate 'items:' tag >> --- >> .../soc/intel/intel,hps-copy-engine.yaml | 51 +++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >> >> diff --git a/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml b/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >> new file mode 100644 >> index 000000000000..8634865015cd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >> @@ -0,0 +1,51 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +# Copyright (C) 2022, Intel Corporation >> +%YAML 1.2 >> +--- >> +$id: "http://devicetree.org/schemas/soc/intel/intel,hps-copy-engine.yaml#" >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#" >> + >> +title: Intel HPS Copy Engine >> + >> +maintainers: >> + - Matthew Gerlach <matthew.gerlach@linux.intel.com> >> + >> +description: | >> + The Intel Hard Processor System (HPS) Copy Engine is an IP block used to copy >> + a bootable image from host memory to HPS DDR. Additionally, there is a >> + register the HPS can use to indicate the state of booting the copied image as >> + well as a keep-a-live indication to the host. >> + >> +properties: >> + compatible: >> + const: intel,hps-copy-engine >> + >> + '#dma-cells': >> + const: 1 >> + >> + reg: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + bus@80000000 { >> + compatible = "simple-bus"; >> + reg = <0x80000000 0x60000000>, >> + <0xf9000000 0x00100000>; >> + reg-names = "axi_h2f", "axi_h2f_lw"; > > A simple-bus doesn't have regs because it is simple. If you have > registers, then you need a specific compatible. You can have > 'simple-bus' as a fallback if the bus is completely setup by firmware > and the OS never needs to configure/manage it. The hardware I'm trying to describe above is the connection from the HPS/SOC to the FPGA. There are two ranges of physical addresses with this connection referred to as the "HPS to FPGA bridge" and the "Lightweight HPS to FPGA bridge". Device tree subnodes of bus@80000000 are IP blocks in the FPGA. The IP blocks may be connected to one or both of the physical address ranges. Since these physical address ranges are not registers of the bus@80000000, the field names, reg and reg-names, are probably wrong. Should the reg field above really be ranges? > > It also looks odd that ranges only has 4K of bus space and the bus > registers are 1.5GB of space. The intent of the ranges field below is to show the ranges of actual registers associated subnodes of bus@80000000. This is probably incorrect as well. > > That's all kind of outside of the scope of this binding and you should > just drop that part. I agree that discussion of bus@80000000 is outside the scope of this binding. I will resubmit this binding with only the dma-controller@0 node as the example. > >> + #address-cells = <2>; >> + #size-cells = <1>; >> + ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>; >> + >> + dma-controller@0 { >> + compatible = "intel,hps-copy-engine"; >> + reg = <0x00000000 0x00000000 0x00001000>; >> + #dma-cells = <1>; >> + }; >> + }; >> -- >> 2.25.1 >> >> >
On Tue, 10 May 2022, Rob Herring wrote: > On Sun, May 08, 2022 at 07:26:22AM -0700, matthew.gerlach@linux.intel.com wrote: >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> >> Add device tree bindings documentation for the Intel Hard >> Processor System (HPS) Copy Engine. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> v4: >> - move from soc to soc/intel/ >> >> v3: >> - remove unused label >> - move from misc to soc >> - remove 0x from #address-cells/#size-cells values >> - change hps_cp_eng@0 to dma-controller@0 >> - remote inaccurate 'items:' tag >> --- >> .../soc/intel/intel,hps-copy-engine.yaml | 51 +++++++++++++++++++ > > Also, this should go under bindings/dma/ I will move .../soc/intel/intel,hps-copy-engine.yaml to .../dma/intel,hps-copy-engine.yaml unless you think I should start a intel subdirectory of bindings/dma. > >> 1 file changed, 51 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml >
On Tue, May 10, 2022 at 03:23:23PM -0700, matthew.gerlach@linux.intel.com wrote: > > > On Tue, 10 May 2022, Rob Herring wrote: > > > On Sun, May 08, 2022 at 07:26:22AM -0700, matthew.gerlach@linux.intel.com wrote: > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > > > > > Add device tree bindings documentation for the Intel Hard > > > Processor System (HPS) Copy Engine. > > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > > --- > > > v4: > > > - move from soc to soc/intel/ > > > > > > v3: > > > - remove unused label > > > - move from misc to soc > > > - remove 0x from #address-cells/#size-cells values > > > - change hps_cp_eng@0 to dma-controller@0 > > > - remote inaccurate 'items:' tag > > > --- > > > .../soc/intel/intel,hps-copy-engine.yaml | 51 +++++++++++++++++++ > > > 1 file changed, 51 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml b/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml > > > new file mode 100644 > > > index 000000000000..8634865015cd > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml > > > @@ -0,0 +1,51 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +# Copyright (C) 2022, Intel Corporation > > > +%YAML 1.2 > > > +--- > > > +$id: "http://devicetree.org/schemas/soc/intel/intel,hps-copy-engine.yaml#" > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > > + > > > +title: Intel HPS Copy Engine > > > + > > > +maintainers: > > > + - Matthew Gerlach <matthew.gerlach@linux.intel.com> > > > + > > > +description: | > > > + The Intel Hard Processor System (HPS) Copy Engine is an IP block used to copy > > > + a bootable image from host memory to HPS DDR. Additionally, there is a > > > + register the HPS can use to indicate the state of booting the copied image as > > > + well as a keep-a-live indication to the host. > > > + > > > +properties: > > > + compatible: > > > + const: intel,hps-copy-engine > > > + > > > + '#dma-cells': > > > + const: 1 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > +required: > > > + - compatible > > > + - reg > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + bus@80000000 { > > > + compatible = "simple-bus"; > > > + reg = <0x80000000 0x60000000>, > > > + <0xf9000000 0x00100000>; > > > + reg-names = "axi_h2f", "axi_h2f_lw"; > > > > A simple-bus doesn't have regs because it is simple. If you have > > registers, then you need a specific compatible. You can have > > 'simple-bus' as a fallback if the bus is completely setup by firmware > > and the OS never needs to configure/manage it. > > The hardware I'm trying to describe above is the connection from the HPS/SOC > to the FPGA. There are two ranges of physical addresses with this > connection referred to as the "HPS to FPGA bridge" and the "Lightweight HPS > to FPGA bridge". Device tree subnodes of bus@80000000 are IP blocks in the > FPGA. The IP blocks may be connected to one or both of the > physical address ranges. Since these physical address ranges are not registers > of the bus@80000000, the field names, reg and reg-names, are probably wrong. > Should the reg field above really be ranges? Yes, sound likes it should be. Rob
diff --git a/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml b/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml new file mode 100644 index 000000000000..8634865015cd --- /dev/null +++ b/Documentation/devicetree/bindings/soc/intel/intel,hps-copy-engine.yaml @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (C) 2022, Intel Corporation +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/soc/intel/intel,hps-copy-engine.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Intel HPS Copy Engine + +maintainers: + - Matthew Gerlach <matthew.gerlach@linux.intel.com> + +description: | + The Intel Hard Processor System (HPS) Copy Engine is an IP block used to copy + a bootable image from host memory to HPS DDR. Additionally, there is a + register the HPS can use to indicate the state of booting the copied image as + well as a keep-a-live indication to the host. + +properties: + compatible: + const: intel,hps-copy-engine + + '#dma-cells': + const: 1 + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + bus@80000000 { + compatible = "simple-bus"; + reg = <0x80000000 0x60000000>, + <0xf9000000 0x00100000>; + reg-names = "axi_h2f", "axi_h2f_lw"; + #address-cells = <2>; + #size-cells = <1>; + ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>; + + dma-controller@0 { + compatible = "intel,hps-copy-engine"; + reg = <0x00000000 0x00000000 0x00001000>; + #dma-cells = <1>; + }; + };