by Jolly Shah

[permalink] [raw]

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

Hi Rob,

> -----Original Message-----
> From: Rob Herring [mailto:[email protected]]
> Sent: Wednesday, December 05, 2018 2:21 PM
> To: Jolly Shah <[email protected]>
> Cc: [email protected]; [email protected]; Nava kishore Manne
> <[email protected]>; [email protected]; Rajan Vaja
> <[email protected]>; Michal Simek <[email protected]>; linux-arm-
> [email protected]
> 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:[email protected]]
> > > Sent: Tuesday, December 04, 2018 2:06 PM
> > > To: Jolly Shah <[email protected]>
> > > Cc: [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> > > <[email protected]>; Nava kishore Manne <[email protected]>; linux-
> arm-
> > > [email protected]; [email protected];
> > > [email protected]; Jolly Shah <[email protected]>
> > > 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