Re: [PATCH 1/2] Documentation: add binding description of Rockchip PCIe controller

[Shawn Lin]

On 2016/5/20 19:20, Heiko Stuebner wrote:
> Hi Shawn,
>
> Am Freitag, 20. Mai 2016, 18:29:06 schrieb Shawn Lin:
> > This patch add some required and optional properties for Rockchip
> > PCIe controller. Also we add a example for how to use it.
> >
> > Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> >
> > ---
> >
> >  .../devicetree/bindings/pci/rockchip-pcie.txt      | 93
> > ++++++++++++++++++++++ 1 file changed, 93 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt new file mode
> > 100644
> > index 0000000..69a0804
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > @@ -0,0 +1,93 @@
> > +* Rockchip AXI PCIe Root Port Bridge DT description
> > +
> > +Required properties:
> > +- #address-cells: Address representation for root ports, set to <3>
> > +- #size-cells: Size representation for root ports, set to <2>
> > +- #interrupt-cells: specifies the number of cells needed to encode an
> > +		interrupt source. The value must be 1.
> > +- compatible: Should contain "rockchip,rk3399-pcie"
> > +- reg: Two register ranges as listed in the reg-names property
> > +- reg-names: The first entry must be "axi-base" for the core registers
> > +	The second entry must be "apb-base" for the client pcie registers
> > +- clocks: Must contain an entry for each entry in clock-names.
> > +		See ../clocks/clock-bindings.txt for details.
> > +- clock-names: Must include the following entries:
> > +	- "aclk_pcie"
> > +	- "aclk_perf_pcie"
> > +	- "hclk_pcie"
> > +	- "clk_pciephy_ref"
>
> clock names are always in the scope of the node/driver, so the names could
> easily be "aclk", "aclk-perf", "hclk", "phy".
>
> Although from my cursory-glance at the phy-related code, it looks more like
> it shouldn't be in the pcie-driver itself, but in a an actual phy-driver
> (together with the phy reset and clock)?
>
> > +- interrupts: Three interrupt entries must be specified.
> > +- interrupt-names: Must include the following names
> > +	- "pcie-sys"
> > +	- "pcie-legacy"
> > +	- "pcie-client"
>
> Same as above, names could simply be "sys", "legacy", "client"
>
>
> > +- resets: Must contain five entries for each entry in reset-names.
> > +	   See ../reset/reset.txt for details.
> > +- reset-names: Must include the following names
> > +	- "phy-rst"
> > +	- "core-rst"
> > +	- "mgmt-rst"
> > +	- "mgmt-sticky-rst"
> > +	- "pipe-rst"
>
> and again (= without the "-rst")

okay, will simplify clk, int and rst mentioned above.

> > +- rockchip,grf: phandle to the syscon managing the "general register
> > files"
>
> > +- pcie-conf: offset of pcie client block for  configuration
> > +- pcie-status: offset of pcie client block for status
> > +- pcie-laneoff: offset of pcie client block for lane
>
> These are GRF offsets (GRF_SOC_CON8, GRF_SOC_CON5_PCIE, GRF_SOC_STATUS1)
> Those registers are generally prone to change (even their layout) for future
> socs and I'd suggest instead of declaring them in the devicetree, move them
> to the per-soc data in the rockchip_pcie_of_match struct.

sure.

> But it looks as if they're pretty phy-specific, so see comment about possible
> separate phy driver above.
>
>
> > +- msi-parent: Link to the hardware entity that serves as the Message
> > +- pinctrl-names : The pin control state names
> > +- pinctrl-0: The "default" pinctrl state
>
> I'm not sure if pinctrl-properties need to be described when you don't need
> special handling in the form of additional pin states. The pcie part does
> not do any pin-handling of its own.

We need it in prevention of any firmwares change the default state
of #CLKREQ which is useful for ASPM. Also we have a backup pin for
clkreqn called clkreqnb, which should be taken more consideration since
when refering to any one of these two, pinctrl should configure the
bit[14] of GRF_SOC_CON7 automatically. But it is unfortunately beyound
the implementation of pinctrl-rk3399.

BTW, I don't know if we wanna support this action inside the pinctrl
code?

> > +- interrupt-map-mask and interrupt-map: standard PCI properties
> > +- interrupt-controller: identifies the node as an interrupt controller
> > +
> > +Optional Property:
> > +- ep-gpios: contain the entry for pre-reset gpio
> > +- num-lanes: number of lanes to use
> > +- assigned-clocks, assigned-clock-parents and assigned-clock-rates:
> > standard +		   clock bindings. See ../clock/clock-bindings.txt
>
> Again that (assigned-clocks handling) is not actual part of the pci-
> controllers actions, but other parts and also described already elsewhere.

Basically it does. But this is an alternative choice for pcie-phy to
generate the ref_clk. When we want 100MHz src clk for PLL inside the
pcie-phyïwe should add them, otherwise it's taken from xin 24MHz.

This is useful for SI testing or some others special cases. So should we
add it as an option and leave a sample here?

> Heiko


--
Best Regards
Shawn Lin