Date:	Sat, 21 May 2016 11:55:35 +0800
From:	Shawn Lin <shawn.lin@...k-chips.com>
To:	Heiko Stuebner <heiko@...ech.de>
Cc:	shawn.lin@...k-chips.com, Bjorn Helgaas <bhelgaas@...gle.com>,
	Wenrui Li <wenrui.li@...k-chips.com>,
	Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
	Doug Anderson <dianders@...omium.org>,
	linux-pci@...r.kernel.org, linux-rockchip@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] Documentation: add binding description of Rockchip
 PCIe controller

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@...k-chips.com>
>>
>> ---
>>
>>  .../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

Powered by blists - more mailing lists