Hi Rob, As I understand, clock, pinctrl, reset, clock, power domain nodes should be child nodes of firmware node. Others (Qspi, afi, nvmem-fw, fpga, remoteproc etc) who are just using fw interface for specific privileged access/function, should be separate DT nodes. Please confirm and correct me if wrong. I will submit v2 soon with mentioned changes. Thanks, Jolly Shah > -----Original Message----- > From: Rob Herring [mailto:robh@xxxxxxxxxx] > Sent: Wednesday, December 05, 2018 2:21 PM > To: Jolly Shah <JOLLYS@xxxxxxxxxx> > Cc: mark.rutland@xxxxxxx; devicetree@xxxxxxxxxxxxxxx; Nava kishore Manne > <navam@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; Rajan Vaja > <RAJANV@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx > 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, > > Jolly Shah > > > > > -----Original Message----- > > > From: Rob Herring [mailto:robh@xxxxxxxxxx] > > > Sent: Tuesday, December 04, 2018 2:06 PM > > > To: Jolly Shah <JOLLYS@xxxxxxxxxx> > > > Cc: mark.rutland@xxxxxxx; Michal Simek <michals@xxxxxxxxxx>; Rajan Vaja > > > <RAJANV@xxxxxxxxxx>; Nava kishore Manne <navam@xxxxxxxxxx>; linux- > arm- > > > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > > devicetree@xxxxxxxxxxxxxxx; Jolly Shah <JOLLYS@xxxxxxxxxx> > > > 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>, <>_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
