From: Matthew Gerlach <[email protected]>
Add device tree bindings documentation for the Intel Hard
Processor System (HPS) Copy Engine.
Signed-off-by: Matthew Gerlach <[email protected]>
---
.../bindings/misc/intel,hps-copy-engine.yaml | 48 +++++++++++++++++++
1 file changed, 48 insertions(+)
create mode 100644 Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
diff --git a/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml b/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
new file mode 100644
index 000000000000..74e7da9002f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (C) 2022, Intel Corporation
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/misc/intel,hps-copy-engine.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Intel HPS Copy Engine
+
+maintainers:
+ - Matthew Gerlach <[email protected]>
+
+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:
+ items:
+ - const: intel,hps-copy-engine
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ agilex_hps_bridges: bus@80000000 {
+ compatible = "simple-bus";
+ reg = <0x80000000 0x60000000>,
+ <0xf9000000 0x00100000>;
+ reg-names = "axi_h2f", "axi_h2f_lw";
+ #address-cells = <0x2>;
+ #size-cells = <0x1>;
+ ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>;
+
+ hps_cp_eng@0 {
+ compatible = "intel,hps-copy-engine";
+ reg = <0x00000000 0x00000000 0x00001000>;
+ };
+ };
--
2.25.1
On 03/05/2022 21:45, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add device tree bindings documentation for the Intel Hard
> Processor System (HPS) Copy Engine.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> ---
> .../bindings/misc/intel,hps-copy-engine.yaml | 48 +++++++++++++++++++
> 1 file changed, 48 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
>
> diff --git a/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml b/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
> new file mode 100644
> index 000000000000..74e7da9002f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
Please find appropriate directory matching this hardware, not "misc". As
a fallback SoC related bindings end up in "soc".
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (C) 2022, Intel Corporation
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/misc/intel,hps-copy-engine.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Intel HPS Copy Engine
> +
> +maintainers:
> + - Matthew Gerlach <[email protected]>
> +
> +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:
> + items:
No "items", you have just one item.
> + - const: intel,hps-copy-engine
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + agilex_hps_bridges: bus@80000000 {
Unused label...
> + compatible = "simple-bus";
> + reg = <0x80000000 0x60000000>,
> + <0xf9000000 0x00100000>;
> + reg-names = "axi_h2f", "axi_h2f_lw";
> + #address-cells = <0x2>;
$ git grep address-cell
Do not use inconsistent coding. The same applies to your DTS.
> + #size-cells = <0x1>;
> + ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>;
Why do you even need the simple-bus above and cannot put the device
directly on the bus?
> +
> + hps_cp_eng@0 {
No underscores in node names. Generic node name.
> + compatible = "intel,hps-copy-engine";
> + reg = <0x00000000 0x00000000 0x00001000>;
> + };
> + };
Best regards,
Krzysztof
On 05/05/2022 00:41, [email protected] wrote:
>> Please find appropriate directory matching this hardware, not "misc". As
>> a fallback SoC related bindings end up in "soc".
>
> I thought misc seemed appropriate because it is a very specific IP block
> in the FPGA connected to the HPS. It does perform a simple DMA function;
> so I considered putting it in the dma directory, but it also has some
> hand-shaking registers between the HPS and a host processor connected to the
> FPGA via PCIe; so I thought misc. Since the HPS "soc" accesses the
> component, I can put it in the "soc" directory, unless there is a better
> suggestion.
So let it be "soc".
>>
>> $ git grep address-cell
>> Do not use inconsistent coding. The same applies to your DTS.
>
> Is the inconsistency the use of '0x' in the values of #address-cells and
> #size-cells, or is the consistency having different values for
> #address-cells and #size-cells or both?
It's about "0x".
>
>>
>>> + #size-cells = <0x1>;
>>> + ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>;
>>
>> Why do you even need the simple-bus above and cannot put the device
>> directly on the bus?
>
> On an Agilex chip, the HPS is connected to the FPGA via two bridges,
> referred as the "HPS to FPGA bridge" and the "Lightweight HPS to FPGA
> bridge". An IP block in the FPGA could be connected to one or both of
> these bridges. I am anticipating device tree overlays being applied for
> other IP blocks instantiated in the FPGA.
OK
>
>>
>>> +
>>> + hps_cp_eng@0 {
>>
>> No underscores in node names. Generic node name.
>
> I understand. I am considering dma@0 for the generic node name.
Let's keep the same as in DTS.
Best regards,
Krzysztof
On Thu, 5 May 2022, Krzysztof Kozlowski wrote:
> On 05/05/2022 00:41, [email protected] wrote:
>>> Please find appropriate directory matching this hardware, not "misc". As
>>> a fallback SoC related bindings end up in "soc".
>>
>> I thought misc seemed appropriate because it is a very specific IP block
>> in the FPGA connected to the HPS. It does perform a simple DMA function;
>> so I considered putting it in the dma directory, but it also has some
>> hand-shaking registers between the HPS and a host processor connected to the
>> FPGA via PCIe; so I thought misc. Since the HPS "soc" accesses the
>> component, I can put it in the "soc" directory, unless there is a better
>> suggestion.
>
> So let it be "soc".
I will move it to the soc directory.
>
>>>
>>> $ git grep address-cell
>>> Do not use inconsistent coding. The same applies to your DTS.
>>
>> Is the inconsistency the use of '0x' in the values of #address-cells and
>> #size-cells, or is the consistency having different values for
>> #address-cells and #size-cells or both?
>
> It's about "0x".
Got it. I will remove the 0x.
>
>>
>>>
>>>> + #size-cells = <0x1>;
>>>> + ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>;
>>>
>>> Why do you even need the simple-bus above and cannot put the device
>>> directly on the bus?
>>
>> On an Agilex chip, the HPS is connected to the FPGA via two bridges,
>> referred as the "HPS to FPGA bridge" and the "Lightweight HPS to FPGA
>> bridge". An IP block in the FPGA could be connected to one or both of
>> these bridges. I am anticipating device tree overlays being applied for
>> other IP blocks instantiated in the FPGA.
>
> OK
>
>>
>>>
>>>> +
>>>> + hps_cp_eng@0 {
>>>
>>> No underscores in node names. Generic node name.
>>
>> I understand. I am considering dma@0 for the generic node name.
>
> Let's keep the same as in DTS.
Yes, we want the example and the DTS to be the same. It will be called
dma-controller@0.
Thank you for the feedback,
Matthew
>
>
> Best regards,
> Krzysztof
>
On Wed, 4 May 2022, Krzysztof Kozlowski wrote:
> On 03/05/2022 21:45, [email protected] wrote:
>> From: Matthew Gerlach <[email protected]>
>>
>> Add device tree bindings documentation for the Intel Hard
>> Processor System (HPS) Copy Engine.
>>
>> Signed-off-by: Matthew Gerlach <[email protected]>
>> ---
>> .../bindings/misc/intel,hps-copy-engine.yaml | 48 +++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml b/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
>> new file mode 100644
>> index 000000000000..74e7da9002f4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
>
> Please find appropriate directory matching this hardware, not "misc". As
> a fallback SoC related bindings end up in "soc".
I thought misc seemed appropriate because it is a very specific IP block
in the FPGA connected to the HPS. It does perform a simple DMA function;
so I considered putting it in the dma directory, but it also has some
hand-shaking registers between the HPS and a host processor connected to the
FPGA via PCIe; so I thought misc. Since the HPS "soc" accesses the
component, I can put it in the "soc" directory, unless there is a better
suggestion.
>
>> @@ -0,0 +1,48 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright (C) 2022, Intel Corporation
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/misc/intel,hps-copy-engine.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: Intel HPS Copy Engine
>> +
>> +maintainers:
>> + - Matthew Gerlach <[email protected]>
>> +
>> +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:
>> + items:
>
> No "items", you have just one item.
Got it. I will change it in v3.
>
>> + - const: intel,hps-copy-engine
>> +
>> + reg:
>> + maxItems: 1
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + agilex_hps_bridges: bus@80000000 {
>
> Unused label...
I will remove unused label in v3.
>
>> + compatible = "simple-bus";
>> + reg = <0x80000000 0x60000000>,
>> + <0xf9000000 0x00100000>;
>> + reg-names = "axi_h2f", "axi_h2f_lw";
>> + #address-cells = <0x2>;
>
> $ git grep address-cell
> Do not use inconsistent coding. The same applies to your DTS.
Is the inconsistency the use of '0x' in the values of #address-cells and
#size-cells, or is the consistency having different values for
#address-cells and #size-cells or both?
>
>> + #size-cells = <0x1>;
>> + ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>;
>
> Why do you even need the simple-bus above and cannot put the device
> directly on the bus?
On an Agilex chip, the HPS is connected to the FPGA via two bridges,
referred as the "HPS to FPGA bridge" and the "Lightweight HPS to FPGA
bridge". An IP block in the FPGA could be connected to one or both of
these bridges. I am anticipating device tree overlays being applied for
other IP blocks instantiated in the FPGA.
>
>> +
>> + hps_cp_eng@0 {
>
> No underscores in node names. Generic node name.
I understand. I am considering dma@0 for the generic node name.
>
>> + compatible = "intel,hps-copy-engine";
>> + reg = <0x00000000 0x00000000 0x00001000>;
>> + };
>> + };
>
>
> Best regards,
> Krzysztof
>
Thank you for the review,
Matthew