[PATCH v2 0/3] Add device tree for Intel n6000

[PATCH v2 0/3] Add device tree for Intel n6000

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

This patch set adds a device tree for the Hard Processor System (HPS)
on an Agilex based Intel n6000 board.

Patch 1 defines the device tree binding for the HPS Copy Engine IP
used to copy a bootable image from host memory to HPS DDR.

Patch 2 defines the binding for the Intel n6000 board itself.

Patch 3 adds the device tree for the n6000 board.

Changelog v1 -> v2:
  - add dt binding for copy enging
  - add dt binding for n6000 board
  - fix copy engine node name
  - fix compatible field for copy engine
  - remove redundant status field
  - add compatibility field for the board
  - fix SPDX
  - fix how osc1 clock frequency is set


Matthew Gerlach (3):
  dt-bindings: misc: add bindings for Intel HPS Copy Engine
  dt-bindings: intel: add binding for Intel n6000
  arm64: dts: intel: add device tree for n6000

 .../bindings/arm/intel,socfpga.yaml           |  1 +
 .../bindings/misc/intel,hps-copy-engine.yaml  | 48 ++++++++++++
 arch/arm64/boot/dts/intel/Makefile            |  3 +-
 .../boot/dts/intel/socfpga_agilex_n6000.dts   | 76 +++++++++++++++++++
 4 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/misc/intel,hps-copy-engine.yaml
 create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts

-- 
2.25.1

[PATCH v2 1/3] dt-bindings: misc: add bindings for Intel HPS Copy Engine

[matthew.gerlach]

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>
---
 .../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 <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:
+    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

[PATCH v2 2/3] dt-bindings: intel: add binding for Intel n6000

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Add the binding string for the Agilex based Intel n6000 board.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 Documentation/devicetree/bindings/arm/intel,socfpga.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/intel,socfpga.yaml b/Documentation/devicetree/bindings/arm/intel,socfpga.yaml
index 6e043459fcd5..61a454a40e87 100644
--- a/Documentation/devicetree/bindings/arm/intel,socfpga.yaml
+++ b/Documentation/devicetree/bindings/arm/intel,socfpga.yaml
@@ -18,6 +18,7 @@ properties:
         items:
           - enum:
               - intel,n5x-socdk
+              - intel,socfpga-agilex-n6000
               - intel,socfpga-agilex-socdk
           - const: intel,socfpga-agilex
 
-- 
2.25.1

[PATCH v2 3/3] arm64: dts: intel: add device tree for n6000

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Add a device tree for the n6000 instantiation of Agilex
Hard Processor System (HPS).

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v2:
  - fix copy engine node name
  - fix compatible field for copy engine
  - remove redundant status field
  - add compatibility field for the board
  - fix SPDX
  - fix how osc1 clock frequency is set
---
 arch/arm64/boot/dts/intel/Makefile            |  3 +-
 .../boot/dts/intel/socfpga_agilex_n6000.dts   | 76 +++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts

diff --git a/arch/arm64/boot/dts/intel/Makefile b/arch/arm64/boot/dts/intel/Makefile
index 0b5477442263..c2a723838344 100644
--- a/arch/arm64/boot/dts/intel/Makefile
+++ b/arch/arm64/boot/dts/intel/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
-dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += socfpga_agilex_socdk.dtb \
+dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += socfpga_agilex_n6000.dtb \
+				socfpga_agilex_socdk.dtb \
 				socfpga_agilex_socdk_nand.dtb \
 				socfpga_n5x_socdk.dtb
 dtb-$(CONFIG_ARCH_KEEMBAY) += keembay-evm.dtb
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts b/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts
new file mode 100644
index 000000000000..6f8b7bf7a53f
--- /dev/null
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021-2022, Intel Corporation
+ */
+#include "socfpga_agilex.dtsi"
+
+/ {
+	model = "SoCFPGA Agilex n6000";
+	compatible = "intel,socfpga-agilex-n6000", "intel,socfpga-agilex";
+
+	aliases {
+		serial0 = &uart1;
+		serial1 = &uart0;
+		ethernet0 = &gmac0;
+		ethernet1 = &gmac1;
+		ethernet2 = &gmac2;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	memory {
+		device_type = "memory";
+		/* We expect the bootloader to fill in the reg */
+		reg = <0 0 0 0>;
+	};
+
+	soc {
+		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>;
+			};
+		};
+	};
+};
+
+&osc1 {
+	clock-frequency = <25000000>;
+};
+
+&uart0 {
+	status = "okay";
+};
+
+&uart1 {
+	status = "okay";
+};
+
+&spi0 {
+	status = "okay";
+
+	spidev: spidev@0 {
+		status = "okay";
+		compatible = "linux,spidev";
+		spi-max-frequency = <25000000>;
+		reg = <0>;
+	};
+};
+
+&watchdog0 {
+	status = "okay";
+};
+
+&fpga_mgr {
+	status = "disabled";
+};
-- 
2.25.1

Re: [PATCH v2 2/3] dt-bindings: intel: add binding for Intel n6000

[Krzysztof Kozlowski]

On 03/05/2022 21:45, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add the binding string for the Agilex based Intel n6000 board.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: intel: add device tree for n6000

[Krzysztof Kozlowski]

On 03/05/2022 21:45, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a device tree for the n6000 instantiation of Agilex
> Hard Processor System (HPS).
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> +
> +&spi0 {
> +	status = "okay";
> +
> +	spidev: spidev@0 {
> +		status = "okay";
> +		compatible = "linux,spidev";
> +		spi-max-frequency = <25000000>;
> +		reg = <0>;

You should see big fat warnings - from checkpatch and when you boot your
device. This compatible is not accepted.

Please be sure you run checkpatch on your patches. Using reviewers time
instead of automated tool for the same job is discouraged...


Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: intel: add device tree for n6000

[Krzysztof Kozlowski]

On 03/05/2022 21:45, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a device tree for the n6000 instantiation of Agilex
> Hard Processor System (HPS).
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>

> +
> +	soc {
> +		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 {

No underscores in node names.  dtc W=1 should complain about it.
The node name should be generic, matching class of a device. What is
this exactly?

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: misc: add bindings for Intel HPS Copy Engine

[Krzysztof Kozlowski]

On 03/05/2022 21:45, 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>
> ---
>  .../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 <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:
> +    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

Re: [PATCH v2 2/3] dt-bindings: intel: add binding for Intel n6000

[matthew.gerlach]

On Wed, 4 May 2022, Krzysztof Kozlowski wrote:

> On 03/05/2022 21:45, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add the binding string for the Agilex based Intel n6000 board.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thank you for your time and the Acked-by. I will add it to my v3 patch 
set.

Matthew
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 3/3] arm64: dts: intel: add device tree for n6000

[matthew.gerlach]

On Wed, 4 May 2022, Krzysztof Kozlowski wrote:

> On 03/05/2022 21:45, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a device tree for the n6000 instantiation of Agilex
>> Hard Processor System (HPS).
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> +
>> +&spi0 {
>> +	status = "okay";
>> +
>> +	spidev: spidev@0 {
>> +		status = "okay";
>> +		compatible = "linux,spidev";
>> +		spi-max-frequency = <25000000>;
>> +		reg = <0>;
>
> You should see big fat warnings - from checkpatch and when you boot your
> device. This compatible is not accepted.

I must have missed the warning for the compatible string.  I see it now, 
and I remove the node in the v3 patch set.

Thanks for the feedback.
>
> Please be sure you run checkpatch on your patches. Using reviewers time
> instead of automated tool for the same job is discouraged...
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 3/3] arm64: dts: intel: add device tree for n6000

[matthew.gerlach]

On Wed, 4 May 2022, Krzysztof Kozlowski wrote:

> On 03/05/2022 21:45, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a device tree for the n6000 instantiation of Agilex
>> Hard Processor System (HPS).
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
>> +
>> +	soc {
>> +		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 {
>
> No underscores in node names.  dtc W=1 should complain about it.

I will remove the underscores in the name.  I didn't see a complaint when 
I compiled it with "make W=1" in the kernel tree.

> The node name should be generic, matching class of a device. What is
> this exactly?

The component is a specialized IP block instantiated in the FPGA directly 
connected to the HPS.  In one sense the IP block is a simple DMA 
controller, but it also has some registers for hand shaking between the 
HPS and a host processor connected to the FPGA via PCIe.  Should I call 
the node, dma@0?

Thanks for your review,
Matthew

> > Best regards,
> Krzysztof
>

Re: [PATCH v2 1/3] dt-bindings: misc: add bindings for Intel HPS Copy Engine

[matthew.gerlach]

On Wed, 4 May 2022, Krzysztof Kozlowski wrote:

> On 03/05/2022 21:45, 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>
>> ---
>>  .../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 <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:
>> +    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

Re: [PATCH v2 3/3] arm64: dts: intel: add device tree for n6000

[Krzysztof Kozlowski]

On 04/05/2022 23:22, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Wed, 4 May 2022, Krzysztof Kozlowski wrote:
> 
>> On 03/05/2022 21:45, matthew.gerlach@linux.intel.com wrote:
>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>
>>> Add a device tree for the n6000 instantiation of Agilex
>>> Hard Processor System (HPS).
>>>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>>> +
>>> +	soc {
>>> +		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 {
>>
>> No underscores in node names.  dtc W=1 should complain about it.
> 
> I will remove the underscores in the name.  I didn't see a complaint when 
> I compiled it with "make W=1" in the kernel tree.
> 
>> The node name should be generic, matching class of a device. What is
>> this exactly?
> 
> The component is a specialized IP block instantiated in the FPGA directly 
> connected to the HPS.  In one sense the IP block is a simple DMA 
> controller, but it also has some registers for hand shaking between the 
> HPS and a host processor connected to the FPGA via PCIe.  Should I call 
> the node, dma@0?

Then maybe the closest is dma-controller.


Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: misc: add bindings for Intel HPS Copy Engine

[Krzysztof Kozlowski]

On 05/05/2022 00:41, matthew.gerlach@linux.intel.com 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

Re: [PATCH v2 3/3] arm64: dts: intel: add device tree for n6000

[matthew.gerlach]

On Thu, 5 May 2022, Krzysztof Kozlowski wrote:

> On 04/05/2022 23:22, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Wed, 4 May 2022, Krzysztof Kozlowski wrote:
>>
>>> On 03/05/2022 21:45, matthew.gerlach@linux.intel.com wrote:
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Add a device tree for the n6000 instantiation of Agilex
>>>> Hard Processor System (HPS).
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>
>>>> +
>>>> +	soc {
>>>> +		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 {
>>>
>>> No underscores in node names.  dtc W=1 should complain about it.
>>
>> I will remove the underscores in the name.  I didn't see a complaint when
>> I compiled it with "make W=1" in the kernel tree.
>>
>>> The node name should be generic, matching class of a device. What is
>>> this exactly?
>>
>> The component is a specialized IP block instantiated in the FPGA directly
>> connected to the HPS.  In one sense the IP block is a simple DMA
>> controller, but it also has some registers for hand shaking between the
>> HPS and a host processor connected to the FPGA via PCIe.  Should I call
>> the node, dma@0?
>
> Then maybe the closest is dma-controller.

OK, I will call it dma-controller@0.


Thanks,
Matthew
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/3] dt-bindings: misc: add bindings for Intel HPS Copy Engine

[matthew.gerlach]

On Thu, 5 May 2022, Krzysztof Kozlowski wrote:

> On 05/05/2022 00:41, matthew.gerlach@linux.intel.com 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
>