mbox series

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

Message ID 20220503194546.1287679-1-matthew.gerlach@linux.intel.com
Headers show
Series Add device tree for Intel n6000 | expand

Message

matthew.gerlach@linux.intel.com May 3, 2022, 7:45 p.m. UTC
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

Comments

Krzysztof Kozlowski May 4, 2022, 2:56 p.m. UTC | #1
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
Krzysztof Kozlowski May 4, 2022, 3:02 p.m. UTC | #2
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
matthew.gerlach@linux.intel.com May 4, 2022, 9:14 p.m. UTC | #3
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
>
matthew.gerlach@linux.intel.com May 4, 2022, 9:22 p.m. UTC | #4
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
>
Krzysztof Kozlowski May 5, 2022, 6:42 a.m. UTC | #5
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
matthew.gerlach@linux.intel.com May 5, 2022, 5:16 p.m. UTC | #6
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
>