mbox series

[0/9] dt-bindings: Firmware node binding for ZynqMP core

Message ID 1542412619-387-1-git-send-email-jollys@xilinx.com (mailing list archive)
Headers show
Series dt-bindings: Firmware node binding for ZynqMP core | expand

Message

Jolly Shah Nov. 16, 2018, 11:56 p.m. UTC
Base firmware node and clock child node binding are part of mainline kernel. This patchset adds documentation to describe rest of the firmware child node bindings. 
Complete firmware DT node example is shown below for ease of understanding:

firmware {
	zynqmp_firmware: zynqmp-firmware {
		compatible = "xlnx,zynqmp-firmware";
		method = "smc";
		#power-domain-cells = <1>;
		#reset-cells = <1>;

		zynqmp_clk: clock-controller {
			#clock-cells = <1>;
			compatible = "xlnx,zynqmp-clk";
			clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, <&aux_ref_clk>, <&gt_crx_ref_clk>;
			clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk";
		};

		zynqmp_power: zynqmp-power {
			compatible = "xlnx,zynqmp-power";
			interrupts = <0 35 4>;
		};

		nvmem_firmware {
			compatible = "xlnx,zynqmp-nvmem-fw";
			#address-cells = <1>;
			#size-cells = <1>;

			/* Data cells */
			soc_revision: soc_revision {
				reg = <0x0 0x4>;
			};
		};

		afi0: afi0 {
			compatible = "xlnx,afi-fpga";
			config-afi = <0 2>, <1 1>, <2 1>;
		};

		qspi: spi@ff0f0000 {
			compatible = "xlnx,zynqmp-qspi-1.0";
			clock-names = "ref_clk", "pclk";
			clocks = <&misc_clk &misc_clk>;
			interrupts = <0 15 4>;
			interrupt-parent = <&gic>;
			num-cs = <1>;
			reg = <0x0 0xff0f0000 0x1000>,<0x0 0xc0000000 0x8000000>;
		};

		serdes: zynqmp_phy@fd400000 {
			compatible = "xlnx,zynqmp-psgtr";
			status = "okay";
			reg = <0x0 0xfd400000 0x0 0x40000>, <0x0 0xfd3d0000 0x0 0x1000>,
				<0x0 0xff5e0000 0x0 0x1000>;
			reg-names = "serdes", "siou", "lpd";

			lane0: lane@0 {
				#phy-cells = <4>;
			};
			lane1: lane@1 {
				#phy-cells = <4>;
			};
			lane2: lane@2 {
				#phy-cells = <4>;
			};
			lane3: lane@3 {
				#phy-cells = <4>;
			};
		};

		pinctrl_uart1_default: uart1-default {
			mux {
				groups = "uart0_4_grp";
				function = "uart0";
			};

			conf {
				groups = "uart0_4_grp";
				slew-rate = <SLEW_RATE_SLOW>;
				io-standard = <IO_STANDARD_LVCMOS18>;
			};

			conf-rx {
				pins = "MIO18";
				bias-high-impedance;
			};

			conf-tx {
				pins = "MIO19";
				bias-disable;
				schmitt-cmos = <PIN_INPUT_TYPE_CMOS>;
			};
		};
		zynqmp-r5-remoteproc@0 {
			compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
			reg = <0x0 0xFFE00000 0x0 0x10000>,
				<0x0 0xFFE20000 0x0 0x10000>,
				<0x0 0xff340000 0x0 0x100>;
			reg-names = "tcm_a", "tcm_b", "ipi";
			dma-ranges;
			core_conf = "split0";
			memory-region = <&rproc_0_fw_reserved>,
					<&rproc_0_dma_reserved>;
			tcm-pnode-id = <0xf>, <0x10>;
			rpu-pnode-id = <0x7>;
			interrupt-parent = <&gic>;
			interrupts = <0 29 4>;
		};
	};
};

Jolly Shah (2):
  dt-bindings: phy: Add dt bindings for ZynqMP PHY
  dt-bindings: fpga: Add binding doc for the afi config driver

Nava kishore Manne (2):
  dt-bindings: reset: Add bindings for ZynqMP reset driver
  dt-bindings: nvmem: Add bindings for ZynqMP nvmem driver

Rajan Vaja (4):
  dt-bindings: power: Add ZynqMP power domain bindings
  dt-bindings: soc: Add ZynqMP PM bindings
  dt-bindings: pinctrl: Add ZynqMP pin controller bindings
  dt-bindings: spi: zynqmp: Move SPI node under zynqmp firmware

Wendy Liang (1):
  dt-bindings: remoteproc: Add Xilinx R5 rproc binding

 .../devicetree/bindings/fpga/xlnx,afi-fpga.txt     |  67 +++++
 .../bindings/nvmem/xlnx,zynqmp-nvmem.txt           |  44 ++++
 .../devicetree/bindings/phy/phy-zynqmp.txt         | 126 ++++++++++
 .../bindings/pinctrl/xlnx,zynqmp-pinctrl.txt       | 272 +++++++++++++++++++++
 .../bindings/power/reset/xlnx,zynqmp-power.txt     |  25 ++
 .../bindings/power/xlnx,zynqmp-genpd.txt           |  34 +++
 .../remoteproc/xlnx,zynqmp-r5-remoteproc.txt       |  81 ++++++
 .../bindings/reset/xlnx,zynqmp-reset.txt           | 148 +++++++++++
 .../devicetree/bindings/spi/spi-zynqmp-qspi.txt    |  22 +-
 include/dt-bindings/power/xlnx-zynqmp-power.h      |  39 +++
 10 files changed, 850 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,afi-fpga.txt
 create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
 create mode 100644 Documentation/devicetree/bindings/phy/phy-zynqmp.txt
 create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynqmp-pinctrl.txt
 create mode 100644 Documentation/devicetree/bindings/power/reset/xlnx,zynqmp-power.txt
 create mode 100644 Documentation/devicetree/bindings/power/xlnx,zynqmp-genpd.txt
 create mode 100644 Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5-remoteproc.txt
 create mode 100644 Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.txt
 create mode 100644 include/dt-bindings/power/xlnx-zynqmp-power.h

Comments

Jolly Shah Nov. 26, 2018, 9:39 p.m. UTC | #1
Ping for comments

> -----Original Message-----
> From: Jolly Shah [mailto:jolly.shah@xilinx.com]
> Sent: Friday, November 16, 2018 3:57 PM
> To: robh+dt@kernel.org; mark.rutland@arm.com
> Cc: Michal Simek <michals@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>;
> Nava kishore Manne <navam@xilinx.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Jolly Shah <JOLLYS@xilinx.com>
> Subject: [PATCH 0/9] dt-bindings: Firmware node binding for ZynqMP core
> 
> Base firmware node and clock child node binding are part of mainline kernel.
> This patchset adds documentation to describe rest of the firmware child node
> bindings.
> Complete firmware DT node example is shown below for ease of understanding:
> 
> firmware {
> 	zynqmp_firmware: zynqmp-firmware {
> 		compatible = "xlnx,zynqmp-firmware";
> 		method = "smc";
> 		#power-domain-cells = <1>;
> 		#reset-cells = <1>;
> 
> 		zynqmp_clk: clock-controller {
> 			#clock-cells = <1>;
> 			compatible = "xlnx,zynqmp-clk";
> 			clocks = <&pss_ref_clk>, <&video_clk>,
> <&pss_alt_ref_clk>, <&aux_ref_clk>, <&gt_crx_ref_clk>;
> 			clock-names = "pss_ref_clk", "video_clk",
> "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk";
> 		};
> 
> 		zynqmp_power: zynqmp-power {
> 			compatible = "xlnx,zynqmp-power";
> 			interrupts = <0 35 4>;
> 		};
> 
> 		nvmem_firmware {
> 			compatible = "xlnx,zynqmp-nvmem-fw";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 
> 			/* Data cells */
> 			soc_revision: soc_revision {
> 				reg = <0x0 0x4>;
> 			};
> 		};
> 
> 		afi0: afi0 {
> 			compatible = "xlnx,afi-fpga";
> 			config-afi = <0 2>, <1 1>, <2 1>;
> 		};
> 
> 		qspi: spi@ff0f0000 {
> 			compatible = "xlnx,zynqmp-qspi-1.0";
> 			clock-names = "ref_clk", "pclk";
> 			clocks = <&misc_clk &misc_clk>;
> 			interrupts = <0 15 4>;
> 			interrupt-parent = <&gic>;
> 			num-cs = <1>;
> 			reg = <0x0 0xff0f0000 0x1000>,<0x0 0xc0000000
> 0x8000000>;
> 		};
> 
> 		serdes: zynqmp_phy@fd400000 {
> 			compatible = "xlnx,zynqmp-psgtr";
> 			status = "okay";
> 			reg = <0x0 0xfd400000 0x0 0x40000>, <0x0 0xfd3d0000
> 0x0 0x1000>,
> 				<0x0 0xff5e0000 0x0 0x1000>;
> 			reg-names = "serdes", "siou", "lpd";
> 
> 			lane0: lane@0 {
> 				#phy-cells = <4>;
> 			};
> 			lane1: lane@1 {
> 				#phy-cells = <4>;
> 			};
> 			lane2: lane@2 {
> 				#phy-cells = <4>;
> 			};
> 			lane3: lane@3 {
> 				#phy-cells = <4>;
> 			};
> 		};
> 
> 		pinctrl_uart1_default: uart1-default {
> 			mux {
> 				groups = "uart0_4_grp";
> 				function = "uart0";
> 			};
> 
> 			conf {
> 				groups = "uart0_4_grp";
> 				slew-rate = <SLEW_RATE_SLOW>;
> 				io-standard = <IO_STANDARD_LVCMOS18>;
> 			};
> 
> 			conf-rx {
> 				pins = "MIO18";
> 				bias-high-impedance;
> 			};
> 
> 			conf-tx {
> 				pins = "MIO19";
> 				bias-disable;
> 				schmitt-cmos = <PIN_INPUT_TYPE_CMOS>;
> 			};
> 		};
> 		zynqmp-r5-remoteproc@0 {
> 			compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> 			reg = <0x0 0xFFE00000 0x0 0x10000>,
> 				<0x0 0xFFE20000 0x0 0x10000>,
> 				<0x0 0xff340000 0x0 0x100>;
> 			reg-names = "tcm_a", "tcm_b", "ipi";
> 			dma-ranges;
> 			core_conf = "split0";
> 			memory-region = <&rproc_0_fw_reserved>,
> 					<&rproc_0_dma_reserved>;
> 			tcm-pnode-id = <0xf>, <0x10>;
> 			rpu-pnode-id = <0x7>;
> 			interrupt-parent = <&gic>;
> 			interrupts = <0 29 4>;
> 		};
> 	};
> };
> 
> Jolly Shah (2):
>   dt-bindings: phy: Add dt bindings for ZynqMP PHY
>   dt-bindings: fpga: Add binding doc for the afi config driver
> 
> Nava kishore Manne (2):
>   dt-bindings: reset: Add bindings for ZynqMP reset driver
>   dt-bindings: nvmem: Add bindings for ZynqMP nvmem driver
> 
> Rajan Vaja (4):
>   dt-bindings: power: Add ZynqMP power domain bindings
>   dt-bindings: soc: Add ZynqMP PM bindings
>   dt-bindings: pinctrl: Add ZynqMP pin controller bindings
>   dt-bindings: spi: zynqmp: Move SPI node under zynqmp firmware
> 
> Wendy Liang (1):
>   dt-bindings: remoteproc: Add Xilinx R5 rproc binding
> 
>  .../devicetree/bindings/fpga/xlnx,afi-fpga.txt     |  67 +++++
>  .../bindings/nvmem/xlnx,zynqmp-nvmem.txt           |  44 ++++
>  .../devicetree/bindings/phy/phy-zynqmp.txt         | 126 ++++++++++
>  .../bindings/pinctrl/xlnx,zynqmp-pinctrl.txt       | 272 +++++++++++++++++++++
>  .../bindings/power/reset/xlnx,zynqmp-power.txt     |  25 ++
>  .../bindings/power/xlnx,zynqmp-genpd.txt           |  34 +++
>  .../remoteproc/xlnx,zynqmp-r5-remoteproc.txt       |  81 ++++++
>  .../bindings/reset/xlnx,zynqmp-reset.txt           | 148 +++++++++++
>  .../devicetree/bindings/spi/spi-zynqmp-qspi.txt    |  22 +-
>  include/dt-bindings/power/xlnx-zynqmp-power.h      |  39 +++
>  10 files changed, 850 insertions(+), 8 deletions(-)  create mode 100644
> Documentation/devicetree/bindings/fpga/xlnx,afi-fpga.txt
>  create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-
> nvmem.txt
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-zynqmp.txt
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynqmp-
> pinctrl.txt
>  create mode 100644
> Documentation/devicetree/bindings/power/reset/xlnx,zynqmp-power.txt
>  create mode 100644 Documentation/devicetree/bindings/power/xlnx,zynqmp-
> genpd.txt
>  create mode 100644
> Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5-
> remoteproc.txt
>  create mode 100644 Documentation/devicetree/bindings/reset/xlnx,zynqmp-
> reset.txt
>  create mode 100644 include/dt-bindings/power/xlnx-zynqmp-power.h
> 
> --
> 2.7.4
Rob Herring Dec. 4, 2018, 10:06 p.m. UTC | #2
On Fri, Nov 16, 2018 at 03:56:50PM -0800, Jolly Shah wrote:
> Base firmware node and clock child node binding are part of mainline kernel. This patchset adds documentation to describe rest of the firmware child node bindings. 
> Complete firmware DT node example is shown below for ease of understanding:

Shouldn't there be a fpga mgr node too? Called pcap IIRC.

> 
> firmware {
> 	zynqmp_firmware: zynqmp-firmware {
> 		compatible = "xlnx,zynqmp-firmware";
> 		method = "smc";
> 		#power-domain-cells = <1>;
> 		#reset-cells = <1>;
> 
> 		zynqmp_clk: clock-controller {
> 			#clock-cells = <1>;
> 			compatible = "xlnx,zynqmp-clk";
> 			clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, <&aux_ref_clk>, <&gt_crx_ref_clk>;
> 			clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk";
> 		};
> 
> 		zynqmp_power: zynqmp-power {
> 			compatible = "xlnx,zynqmp-power";
> 			interrupts = <0 35 4>;
> 		};
> 
> 		nvmem_firmware {
> 			compatible = "xlnx,zynqmp-nvmem-fw";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 
> 			/* Data cells */
> 			soc_revision: soc_revision {
> 				reg = <0x0 0x4>;
> 			};
> 		};
> 
> 		afi0: afi0 {
> 			compatible = "xlnx,afi-fpga";
> 			config-afi = <0 2>, <1 1>, <2 1>;
> 		};
> 
> 		qspi: spi@ff0f0000 {

Why is this under firmware node?

> 			compatible = "xlnx,zynqmp-qspi-1.0";
> 			clock-names = "ref_clk", "pclk";
> 			clocks = <&misc_clk &misc_clk>;
> 			interrupts = <0 15 4>;
> 			interrupt-parent = <&gic>;
> 			num-cs = <1>;
> 			reg = <0x0 0xff0f0000 0x1000>,<0x0 0xc0000000 0x8000000>;
> 		};
> 
> 		serdes: zynqmp_phy@fd400000 {

And this?

> 			compatible = "xlnx,zynqmp-psgtr";
> 			status = "okay";
> 			reg = <0x0 0xfd400000 0x0 0x40000>, <0x0 0xfd3d0000 0x0 0x1000>,
> 				<0x0 0xff5e0000 0x0 0x1000>;
> 			reg-names = "serdes", "siou", "lpd";
> 
> 			lane0: lane@0 {
> 				#phy-cells = <4>;
> 			};
> 			lane1: lane@1 {
> 				#phy-cells = <4>;
> 			};
> 			lane2: lane@2 {
> 				#phy-cells = <4>;
> 			};
> 			lane3: lane@3 {
> 				#phy-cells = <4>;
> 			};
> 		};
> 
> 		pinctrl_uart1_default: uart1-default {

This goes under a pinctrl node.

> 			mux {
> 				groups = "uart0_4_grp";
> 				function = "uart0";
> 			};
> 
> 			conf {
> 				groups = "uart0_4_grp";
> 				slew-rate = <SLEW_RATE_SLOW>;
> 				io-standard = <IO_STANDARD_LVCMOS18>;
> 			};
> 
> 			conf-rx {
> 				pins = "MIO18";
> 				bias-high-impedance;
> 			};
> 
> 			conf-tx {
> 				pins = "MIO19";
> 				bias-disable;
> 				schmitt-cmos = <PIN_INPUT_TYPE_CMOS>;
> 			};
> 		};
> 		zynqmp-r5-remoteproc@0 {

Wrong unit-address and this doesn't belong here.

> 			compatible = "xlnx,zynqmp-r5-remoteproc-1.0";

'remoteproc' is what the h/w block is called?

> 			reg = <0x0 0xFFE00000 0x0 0x10000>,
> 				<0x0 0xFFE20000 0x0 0x10000>,
> 				<0x0 0xff340000 0x0 0x100>;
> 			reg-names = "tcm_a", "tcm_b", "ipi";
> 			dma-ranges;
> 			core_conf = "split0";
> 			memory-region = <&rproc_0_fw_reserved>,
> 					<&rproc_0_dma_reserved>;
> 			tcm-pnode-id = <0xf>, <0x10>;
> 			rpu-pnode-id = <0x7>;
> 			interrupt-parent = <&gic>;
> 			interrupts = <0 29 4>;
> 		};
> 	};
> };
Jolly Shah Dec. 5, 2018, 8:29 p.m. UTC | #3
Hi Rob,

Thanks for the review. Please find my responses inline.

Thanks,
Jolly Shah

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Tuesday, December 04, 2018 2:06 PM
> To: Jolly Shah <JOLLYS@xilinx.com>
> Cc: mark.rutland@arm.com; Michal Simek <michals@xilinx.com>; Rajan Vaja
> <RAJANV@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Jolly Shah <JOLLYS@xilinx.com>
> Subject: Re: [PATCH 0/9] dt-bindings: Firmware node binding for ZynqMP core
> 
> On Fri, Nov 16, 2018 at 03:56:50PM -0800, Jolly Shah wrote:
> > Base firmware node and clock child node binding are part of mainline kernel.
> This patchset adds documentation to describe rest of the firmware child node
> bindings.
> > Complete firmware DT node example is shown below for ease of
> understanding:
> 
> Shouldn't there be a fpga mgr node too? Called pcap IIRC.
> 
[Jolly] As you suggested, we only added child nodes if the sub-functions have their own resources (clks, irqs, etc.). FPGA doesn't have any resources so not added . Firmware driver would still register it as mfd device to instantiate the driver.

> >
> > firmware {
> > 	zynqmp_firmware: zynqmp-firmware {
> > 		compatible = "xlnx,zynqmp-firmware";
> > 		method = "smc";
> > 		#power-domain-cells = <1>;
> > 		#reset-cells = <1>;
> >
> > 		zynqmp_clk: clock-controller {
> > 			#clock-cells = <1>;
> > 			compatible = "xlnx,zynqmp-clk";
> > 			clocks = <&pss_ref_clk>, <&video_clk>,
> <&pss_alt_ref_clk>, <&aux_ref_clk>, <&gt_crx_ref_clk>;
> > 			clock-names = "pss_ref_clk", "video_clk",
> "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk";
> > 		};
> >
> > 		zynqmp_power: zynqmp-power {
> > 			compatible = "xlnx,zynqmp-power";
> > 			interrupts = <0 35 4>;
> > 		};
> >
> > 		nvmem_firmware {
> > 			compatible = "xlnx,zynqmp-nvmem-fw";
> > 			#address-cells = <1>;
> > 			#size-cells = <1>;
> >
> > 			/* Data cells */
> > 			soc_revision: soc_revision {
> > 				reg = <0x0 0x4>;
> > 			};
> > 		};
> >
> > 		afi0: afi0 {
> > 			compatible = "xlnx,afi-fpga";
> > 			config-afi = <0 2>, <1 1>, <2 1>;
> > 		};
> >
> > 		qspi: spi@ff0f0000 {
> 
> Why is this under firmware node?
[Jolly] Qspi is a user of eemi API provided by firmware node to perform privileged register writes. Alternatively, we can keep such user nodes outside of firmware node and keep nodes which firmware is provider for like clock, reset, pins and power.
Please suggest.

> 
> > 			compatible = "xlnx,zynqmp-qspi-1.0";
> > 			clock-names = "ref_clk", "pclk";
> > 			clocks = <&misc_clk &misc_clk>;
> > 			interrupts = <0 15 4>;
> > 			interrupt-parent = <&gic>;
> > 			num-cs = <1>;
> > 			reg = <0x0 0xff0f0000 0x1000>,<0x0 0xc0000000
> 0x8000000>;
> > 		};
> >
> > 		serdes: zynqmp_phy@fd400000 {
> 
> And this?

[Jolly] Same as above.

> 
> > 			compatible = "xlnx,zynqmp-psgtr";
> > 			status = "okay";
> > 			reg = <0x0 0xfd400000 0x0 0x40000>, <0x0 0xfd3d0000
> 0x0 0x1000>,
> > 				<0x0 0xff5e0000 0x0 0x1000>;
> > 			reg-names = "serdes", "siou", "lpd";
> >
> > 			lane0: lane@0 {
> > 				#phy-cells = <4>;
> > 			};
> > 			lane1: lane@1 {
> > 				#phy-cells = <4>;
> > 			};
> > 			lane2: lane@2 {
> > 				#phy-cells = <4>;
> > 			};
> > 			lane3: lane@3 {
> > 				#phy-cells = <4>;
> > 			};
> > 		};
> >
> > 		pinctrl_uart1_default: uart1-default {
> 
> This goes under a pinctrl node.
[Jolly] Pincontrol node is not added as it doesn't have any resources. As I understand, you suggest to still add pincontrol node and this under pincontrol node.

> 
> > 			mux {
> > 				groups = "uart0_4_grp";
> > 				function = "uart0";
> > 			};
> >
> > 			conf {
> > 				groups = "uart0_4_grp";
> > 				slew-rate = <SLEW_RATE_SLOW>;
> > 				io-standard = <IO_STANDARD_LVCMOS18>;
> > 			};
> >
> > 			conf-rx {
> > 				pins = "MIO18";
> > 				bias-high-impedance;
> > 			};
> >
> > 			conf-tx {
> > 				pins = "MIO19";
> > 				bias-disable;
> > 				schmitt-cmos = <PIN_INPUT_TYPE_CMOS>;
> > 			};
> > 		};
> > 		zynqmp-r5-remoteproc@0 {
> 
> Wrong unit-address and this doesn't belong here.
[Jolly]  Again as it is one of the user of firmware APIs, its kept here. Alternatively, we can keep such user nodes outside of firmware node and keep nodes which firmware is provider for like clock, reset, pins and power.
Please suggest.

> 
> > 			compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> 
> 'remoteproc' is what the h/w block is called?

[Jolly] The hw block is called rpu. 

Thanks,
Jolly Shah 

> 
> > 			reg = <0x0 0xFFE00000 0x0 0x10000>,
> > 				<0x0 0xFFE20000 0x0 0x10000>,
> > 				<0x0 0xff340000 0x0 0x100>;
> > 			reg-names = "tcm_a", "tcm_b", "ipi";
> > 			dma-ranges;
> > 			core_conf = "split0";
> > 			memory-region = <&rproc_0_fw_reserved>,
> > 					<&rproc_0_dma_reserved>;
> > 			tcm-pnode-id = <0xf>, <0x10>;
> > 			rpu-pnode-id = <0x7>;
> > 			interrupt-parent = <&gic>;
> > 			interrupts = <0 29 4>;
> > 		};
> > 	};
> > };
Rob Herring Dec. 5, 2018, 10:20 p.m. UTC | #4
On Wed, Dec 05, 2018 at 08:29:36PM +0000, Jolly Shah wrote:
> Hi Rob,
> 
> Thanks for the review. Please find my responses inline.

You need to fix your mail client to wrap lines.
 
> Thanks,
> Jolly Shah
> 
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Tuesday, December 04, 2018 2:06 PM
> > To: Jolly Shah <JOLLYS@xilinx.com>
> > Cc: mark.rutland@arm.com; Michal Simek <michals@xilinx.com>; Rajan Vaja
> > <RAJANV@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; Jolly Shah <JOLLYS@xilinx.com>
> > Subject: Re: [PATCH 0/9] dt-bindings: Firmware node binding for ZynqMP core
> > 
> > On Fri, Nov 16, 2018 at 03:56:50PM -0800, Jolly Shah wrote:
> > > Base firmware node and clock child node binding are part of mainline kernel.
> > This patchset adds documentation to describe rest of the firmware child node
> > bindings.
> > > Complete firmware DT node example is shown below for ease of
> > understanding:
> > 
> > Shouldn't there be a fpga mgr node too? Called pcap IIRC.
> > 
> [Jolly] As you suggested, we only added child nodes if the 
> sub-functions have their own resources (clks, irqs, etc.). FPGA doesn't 
> have any resources so not added . Firmware driver would still register 
> it as mfd device to instantiate the driver.

Okay, but won't their need to be child devices for 


> 
> > >
> > > firmware {
> > > 	zynqmp_firmware: zynqmp-firmware {
> > > 		compatible = "xlnx,zynqmp-firmware";
> > > 		method = "smc";
> > > 		#power-domain-cells = <1>;
> > > 		#reset-cells = <1>;
> > >
> > > 		zynqmp_clk: clock-controller {
> > > 			#clock-cells = <1>;
> > > 			compatible = "xlnx,zynqmp-clk";
> > > 			clocks = <&pss_ref_clk>, <&video_clk>,
> > <&pss_alt_ref_clk>, <&aux_ref_clk>, <&gt_crx_ref_clk>;
> > > 			clock-names = "pss_ref_clk", "video_clk",
> > "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk";
> > > 		};
> > >
> > > 		zynqmp_power: zynqmp-power {
> > > 			compatible = "xlnx,zynqmp-power";
> > > 			interrupts = <0 35 4>;
> > > 		};
> > >
> > > 		nvmem_firmware {
> > > 			compatible = "xlnx,zynqmp-nvmem-fw";
> > > 			#address-cells = <1>;
> > > 			#size-cells = <1>;
> > >
> > > 			/* Data cells */
> > > 			soc_revision: soc_revision {
> > > 				reg = <0x0 0x4>;
> > > 			};
> > > 		};
> > >
> > > 		afi0: afi0 {
> > > 			compatible = "xlnx,afi-fpga";
> > > 			config-afi = <0 2>, <1 1>, <2 1>;
> > > 		};
> > >
> > > 		qspi: spi@ff0f0000 {
> > 
> > Why is this under firmware node?
> [Jolly] Qspi is a user of eemi API provided by firmware node to 
> perform privileged register writes. Alternatively, we can keep such 
> user nodes outside of firmware node and keep nodes which firmware is 
> provider for like clock, reset, pins and power.
> Please suggest.

Child nodes of the firmware should be providers, not consumers (of the 
firmware). If you had a firmware interface to that provided a SPI 
interface, then it would be here. But just having a special mechanism to 
access the registers. 

> > 
> > > 			compatible = "xlnx,zynqmp-qspi-1.0";

If this same block works with unprivileged accesses, then you will need 
some way to distinguish that.

> > > 			clock-names = "ref_clk", "pclk";
> > > 			clocks = <&misc_clk &misc_clk>;
> > > 			interrupts = <0 15 4>;
> > > 			interrupt-parent = <&gic>;
> > > 			num-cs = <1>;
> > > 			reg = <0x0 0xff0f0000 0x1000>,<0x0 0xc0000000
> > 0x8000000>;
> > > 		};
> > >
> > > 		serdes: zynqmp_phy@fd400000 {
> > 
> > And this?
> 
> [Jolly] Same as above.
> 
> > 
> > > 			compatible = "xlnx,zynqmp-psgtr";
> > > 			status = "okay";
> > > 			reg = <0x0 0xfd400000 0x0 0x40000>, <0x0 0xfd3d0000
> > 0x0 0x1000>,
> > > 				<0x0 0xff5e0000 0x0 0x1000>;
> > > 			reg-names = "serdes", "siou", "lpd";
> > >
> > > 			lane0: lane@0 {
> > > 				#phy-cells = <4>;
> > > 			};
> > > 			lane1: lane@1 {
> > > 				#phy-cells = <4>;
> > > 			};
> > > 			lane2: lane@2 {
> > > 				#phy-cells = <4>;
> > > 			};
> > > 			lane3: lane@3 {
> > > 				#phy-cells = <4>;
> > > 			};
> > > 		};
> > >
> > > 		pinctrl_uart1_default: uart1-default {
> > 
> > This goes under a pinctrl node.
> [Jolly] Pincontrol node is not added as it doesn't have any 
> resources. As I understand, you suggest to still add pincontrol node 
> and this under pincontrol node.

These nodes are resources, so yes you should have a child here.
> 
> > 
> > > 			mux {
> > > 				groups = "uart0_4_grp";
> > > 				function = "uart0";
> > > 			};
> > >
> > > 			conf {
> > > 				groups = "uart0_4_grp";
> > > 				slew-rate = <SLEW_RATE_SLOW>;
> > > 				io-standard = <IO_STANDARD_LVCMOS18>;
> > > 			};
> > >
> > > 			conf-rx {
> > > 				pins = "MIO18";
> > > 				bias-high-impedance;
> > > 			};
> > >
> > > 			conf-tx {
> > > 				pins = "MIO19";
> > > 				bias-disable;
> > > 				schmitt-cmos = <PIN_INPUT_TYPE_CMOS>;
> > > 			};
> > > 		};
> > > 		zynqmp-r5-remoteproc@0 {
> > 
> > Wrong unit-address and this doesn't belong here.
> [Jolly]  Again as it is one of the user of firmware APIs, its kept 
> here. Alternatively, we can keep such user nodes outside of firmware 
> node and keep nodes which firmware is provider for like clock, reset, 
> pins and power.
> Please suggest.

Yes, it should be outside this.

> > 
> > > 			compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> > 
> > 'remoteproc' is what the h/w block is called?
> 
> [Jolly] The hw block is called rpu. 

Then call it that in the DT.

Rob
Jolly Shah Dec. 6, 2018, 11:08 p.m. UTC | #5
Hi Rob,

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Wednesday, December 05, 2018 2:21 PM
> To: Jolly Shah <JOLLYS@xilinx.com>
> Cc: mark.rutland@arm.com; devicetree@vger.kernel.org; Nava kishore Manne
> <navam@xilinx.com>; linux-kernel@vger.kernel.org; Rajan Vaja
> <RAJANV@xilinx.com>; Michal Simek <michals@xilinx.com>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 0/9] dt-bindings: Firmware node binding for ZynqMP core
> 
> On Wed, Dec 05, 2018 at 08:29:36PM +0000, Jolly Shah wrote:
> > Hi Rob,
> >
> > Thanks for the review. Please find my responses inline.
> 
> You need to fix your mail client to wrap lines.

Thanks I will.

> 
> > Thanks,
> > Jolly Shah
> >
> > > -----Original Message-----
> > > From: Rob Herring [mailto:robh@kernel.org]
> > > Sent: Tuesday, December 04, 2018 2:06 PM
> > > To: Jolly Shah <JOLLYS@xilinx.com>
> > > Cc: mark.rutland@arm.com; Michal Simek <michals@xilinx.com>; Rajan Vaja
> > > <RAJANV@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; linux-
> arm-
> > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; Jolly Shah <JOLLYS@xilinx.com>
> > > Subject: Re: [PATCH 0/9] dt-bindings: Firmware node binding for ZynqMP
> core
> > >
> > > On Fri, Nov 16, 2018 at 03:56:50PM -0800, Jolly Shah wrote:
> > > > Base firmware node and clock child node binding are part of mainline
> kernel.
> > > This patchset adds documentation to describe rest of the firmware child
> node
> > > bindings.
> > > > Complete firmware DT node example is shown below for ease of
> > > understanding:
> > >
> > > Shouldn't there be a fpga mgr node too? Called pcap IIRC.
> > >
> > [Jolly] As you suggested, we only added child nodes if the
> > sub-functions have their own resources (clks, irqs, etc.). FPGA doesn't
> > have any resources so not added . Firmware driver would still register
> > it as mfd device to instantiate the driver.
> 
> Okay, but won't their need to be child devices for

There are no fpga child devices. Should it be moved out?

> 
> 
> >
> > > >
> > > > firmware {
> > > > 	zynqmp_firmware: zynqmp-firmware {
> > > > 		compatible = "xlnx,zynqmp-firmware";
> > > > 		method = "smc";
> > > > 		#power-domain-cells = <1>;
> > > > 		#reset-cells = <1>;
> > > >
> > > > 		zynqmp_clk: clock-controller {
> > > > 			#clock-cells = <1>;
> > > > 			compatible = "xlnx,zynqmp-clk";
> > > > 			clocks = <&pss_ref_clk>, <&video_clk>,
> > > <&pss_alt_ref_clk>, <&aux_ref_clk>, <&gt_crx_ref_clk>;
> > > > 			clock-names = "pss_ref_clk", "video_clk",
> > > "pss_alt_ref_clk","aux_ref_clk", "gt_crx_ref_clk";
> > > > 		};
> > > >
> > > > 		zynqmp_power: zynqmp-power {
> > > > 			compatible = "xlnx,zynqmp-power";
> > > > 			interrupts = <0 35 4>;
> > > > 		};
> > > >
> > > > 		nvmem_firmware {
> > > > 			compatible = "xlnx,zynqmp-nvmem-fw";
> > > > 			#address-cells = <1>;
> > > > 			#size-cells = <1>;
> > > >
> > > > 			/* Data cells */
> > > > 			soc_revision: soc_revision {
> > > > 				reg = <0x0 0x4>;
> > > > 			};
> > > > 		};
> > > >
> > > > 		afi0: afi0 {
> > > > 			compatible = "xlnx,afi-fpga";
> > > > 			config-afi = <0 2>, <1 1>, <2 1>;
> > > > 		};
> > > >
> > > > 		qspi: spi@ff0f0000 {
> > >
> > > Why is this under firmware node?
> > [Jolly] Qspi is a user of eemi API provided by firmware node to
> > perform privileged register writes. Alternatively, we can keep such
> > user nodes outside of firmware node and keep nodes which firmware is
> > provider for like clock, reset, pins and power.
> > Please suggest.
> 
> Child nodes of the firmware should be providers, not consumers (of the
> firmware). If you had a firmware interface to that provided a SPI
> interface, then it would be here. But just having a special mechanism to
> access the registers.

Ok got it. So will move it out.

> 
> > >
> > > > 			compatible = "xlnx,zynqmp-qspi-1.0";
> 
> If this same block works with unprivileged accesses, then you will need
> some way to distinguish that.
> 
> > > > 			clock-names = "ref_clk", "pclk";
> > > > 			clocks = <&misc_clk &misc_clk>;
> > > > 			interrupts = <0 15 4>;
> > > > 			interrupt-parent = <&gic>;
> > > > 			num-cs = <1>;
> > > > 			reg = <0x0 0xff0f0000 0x1000>,<0x0 0xc0000000
> > > 0x8000000>;
> > > > 		};
> > > >
> > > > 		serdes: zynqmp_phy@fd400000 {
> > >
> > > And this?
> >
> > [Jolly] Same as above.
> >
> > >
> > > > 			compatible = "xlnx,zynqmp-psgtr";
> > > > 			status = "okay";
> > > > 			reg = <0x0 0xfd400000 0x0 0x40000>, <0x0 0xfd3d0000
> > > 0x0 0x1000>,
> > > > 				<0x0 0xff5e0000 0x0 0x1000>;
> > > > 			reg-names = "serdes", "siou", "lpd";
> > > >
> > > > 			lane0: lane@0 {
> > > > 				#phy-cells = <4>;
> > > > 			};
> > > > 			lane1: lane@1 {
> > > > 				#phy-cells = <4>;
> > > > 			};
> > > > 			lane2: lane@2 {
> > > > 				#phy-cells = <4>;
> > > > 			};
> > > > 			lane3: lane@3 {
> > > > 				#phy-cells = <4>;
> > > > 			};
> > > > 		};
> > > >
> > > > 		pinctrl_uart1_default: uart1-default {
> > >
> > > This goes under a pinctrl node.
> > [Jolly] Pincontrol node is not added as it doesn't have any
> > resources. As I understand, you suggest to still add pincontrol node
> > and this under pincontrol node.
> 
> These nodes are resources, so yes you should have a child here.

Got it. Will add pinctrl node along with below resources.

> >
> > >
> > > > 			mux {
> > > > 				groups = "uart0_4_grp";
> > > > 				function = "uart0";
> > > > 			};
> > > >
> > > > 			conf {
> > > > 				groups = "uart0_4_grp";
> > > > 				slew-rate = <SLEW_RATE_SLOW>;
> > > > 				io-standard = <IO_STANDARD_LVCMOS18>;
> > > > 			};
> > > >
> > > > 			conf-rx {
> > > > 				pins = "MIO18";
> > > > 				bias-high-impedance;
> > > > 			};
> > > >
> > > > 			conf-tx {
> > > > 				pins = "MIO19";
> > > > 				bias-disable;
> > > > 				schmitt-cmos = <PIN_INPUT_TYPE_CMOS>;
> > > > 			};
> > > > 		};
> > > > 		zynqmp-r5-remoteproc@0 {
> > >
> > > Wrong unit-address and this doesn't belong here.
> > [Jolly]  Again as it is one of the user of firmware APIs, its kept
> > here. Alternatively, we can keep such user nodes outside of firmware
> > node and keep nodes which firmware is provider for like clock, reset,
> > pins and power.
> > Please suggest.
> 
> Yes, it should be outside this.

Ok. 

> 
> > >
> > > > 			compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> > >
> > > 'remoteproc' is what the h/w block is called?
> >
> > [Jolly] The hw block is called rpu.
> 
> Then call it that in the DT.

Ok.

Thanks,
Jolly Shah

> 
> Rob