| 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: <20181205222032.GA810@bogus>
Date: Wed, 5 Dec 2018 16:20:32 -0600
From: Rob Herring <robh@...nel.org>
To: Jolly Shah <JOLLYS@...inx.com>
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
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@...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
>
> > >
> > > 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.
> >
> > > 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.
>
> >
> > > 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
Powered by blists - more mailing lists