| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM5PR0201MB35254435007BB1A4272160A6B8A90@DM5PR0201MB3525.namprd02.prod.outlook.com>
Date: Thu, 6 Dec 2018 23:08:33 +0000
From: Jolly Shah <JOLLYS@...inx.com>
To: Rob Herring <robh@...nel.org>
CC: "mark.rutland@....com" <mark.rutland@....com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Nava kishore Manne <navam@...inx.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Rajan Vaja <RAJANV@...inx.com>,
Michal Simek <michals@...inx.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH 0/9] dt-bindings: Firmware node binding for ZynqMP core
Hi Rob,
> -----Original Message-----
> From: Rob Herring [mailto:robh@...nel.org]
> Sent: Wednesday, December 05, 2018 2:21 PM
> To: Jolly Shah <JOLLYS@...inx.com>
> Cc: mark.rutland@....com; devicetree@...r.kernel.org; Nava kishore Manne
> <navam@...inx.com>; linux-kernel@...r.kernel.org; Rajan Vaja
> <RAJANV@...inx.com>; Michal Simek <michals@...inx.com>; linux-arm-
> kernel@...ts.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@...nel.org]
> > > Sent: Tuesday, December 04, 2018 2:06 PM
> > > To: Jolly Shah <JOLLYS@...inx.com>
> > > Cc: mark.rutland@....com; Michal Simek <michals@...inx.com>; Rajan Vaja
> > > <RAJANV@...inx.com>; Nava kishore Manne <navam@...inx.com>; linux-
> arm-
> > > kernel@...ts.infradead.org; linux-kernel@...r.kernel.org;
> > > devicetree@...r.kernel.org; Jolly Shah <JOLLYS@...inx.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>, <>_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@...f0000 {
> > >
> > > 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@...00000 {
> > >
> > > 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
Powered by blists - more mailing lists