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

[Shawn Lin]

On 2016/5/24 3:53, Heiko Stuebner wrote:
> Am Samstag, 21. Mai 2016, 11:55:35 schrieb 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>
> > > >
> > > > ---
>
> [...]
>
> > > > +- 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?
>
> The TRM says for me for that bit only "pcie_clkreq_sel port control" and
> that naming really suggests that it is a property of the pcie controller,
> not the generic pinctrl. So if this needs to be touched the pcie controller
> needs to do it.

I don't agree that pcie controller should do it. As a common driver, it
should not care two much setting related to io selection which is very
likely to be changed in the future Socs. Shuld it always keep a
reference to bit[ABC] of GRF_SOC_CONXYZ, and should it adds some code
to see which IO is selected for #CLKREQ?

Currently I do it in firmware, but it's worth to make some discussion
as there are also some IO backup slelections the GRF of RK3399. Anyway,
let's skip this topic from the $SUBJECT patch.

> > > > +- 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?
>
> What I meant was that while clock handling is important when looking at the
> whole system, the pcie controller itself does only care that it gets a
> clock, but not that much where you get it from.
>
> So while assigned-clocks has its place in the real devicetree, I don't think
> it is an element of the actual pcie-controller binding.

Oh, I see.. So it seems good to keep all the assigned-clocks in on
place. I will remove it from this patch.

> Heiko


--
Best Regards
Shawn Lin