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

[Rob Herring <robh@kernel.org>]

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