Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote:
> On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote:
>> On 05/14/2014 08:12 PM, Arnd Bergmann wrote:
>>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote:
>>>> On 05/14/2014 06:57 PM, Antoine Ténart wrote:
>>>>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote:
>>>>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
>>>>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
[...]
>>>> Now, thinking about the PHY binding and the (possible) multi-protocol
>>>> support, it can be possible that on BG2Q there is a generic 2-lane
>>>> LVDS PHY that can be configured to support SATA or PCIe. Both are
>>>> electrically and bit-level compatible, so they could be internally
>>>> wired-up with AHCI and PCIe controller.
>>>
>>> Sounds like a reasonable guess. We have other PHY drivers doing the
>>> same thing already.
[...]
>>>> From a DT point-of-view, we need a way to (a) link each SATA or PCIe
>>>> port to the PHY, (b) specify the PHY lane to be used, and (c) specify
>>>> the protocol to be used on that lane. If I got it right, Arnd already
>>>> mentioned to use the phy-specifier to deal with it:
>>>>
>>>> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE>
>>>
>>> Right.
>>>
>>>> Let's assume we have one dual-port SATA controller and one PCIe
>>>> controller with either x1 or x2 support. The only sane DT binding,
>>>> I can think of then would be:
>>>>
>>>> berlin2q.dtsi:
>>>>
>>>> genphy: lvds@ea00ff {
>>>> 	compatible = "marvell,berlin-lvds-phy";
>>>> 	reg = <0xea00ff 0x100>;
>>>> 	#phy-cells = <2>;
>>>> };
>>>>
>>>> sata: sata@ab00ff {
>>>> 	compatible = "ahci-platform";
>>>> 	reg = <0xab00ff 0x100>;
>>>> 	
>>>> 	sata0: sata-port@0 {
>>>> 		reg = <0>;
>>>> 		phy = <&genphy 0 MODE_SATA>;
>>>> 		status = "disabled";
>>>> 	};
>>>>
>>>> 	sata1: sata-port@1 {
>>>> 		reg = <1>;
>>>> 		phy = <&genphy 1 MODE_SATA>;
>>>> 		status = "disabled";
>>>> 	};
>>>> };
>>>>
>>>> pcie: pcie@ab01ff {
>>>> 	compatible = "marvell,berlin-pcie";
>>>> 	reg = <0xab01ff 0x100>;
>>>>
>>>> 	pcie0: pcie-port@0 {
>>>> 		reg = <0>;
>>>> 		/* set phy on a per-board basis */
>>>> 		/* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */
>>>> 		/* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1
>>>> MODE_PCIE>; */
>>>> 		status = "disabled";
>>>> 	};
>>>> };
>>>>
>>>> berlin2q-dmp.dts:
>>>>
>>>> &sata1 {
>>>> 	status = "okay";
>>>> };
>>>>
>>>> &pcie0 {
>>>> 	phy = <&genphy 1 MODE_PCIE>;
>>>> };
>>>>
>>>> berlin2q-foo.dts:
>>>>
>>>> &pcie0 {
>>>> 	phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>;
>>>> };
>>>
>>> Exactly. I would also be fine with keeping the sub-nodes of the
>>> phy device as in v3 and using #phy-cells=<1> instead of #phy-cells.
>>> The result would be pretty much the same, it just depends on how
>>> closely connected the two logical phys are.
> 
> huh.. even with sub-nodes you'll need #phy-cells=<2> if we use a single *PHY
> PROVIDER*. Because with just PHYs node pointer we won't be able to get the PHY.
> We'll need PHY providers node pointer.
> 
> However I'd prefer to have sub-nodes for each individual PHYs and register a
> single PHY PROVIDER.

Depends on what you call PHY. In the example above the PHY is what
allows you to control both lanes.

So you want sub-nodes for each individual lane given the nomenclature
of the example?

Or like it is used in the example above, a single PHY node with an index
in the phy-specifier to pick an individual lane.

IMHO, having both phy-specifier index _and_ PHY sub-node per lane
has no benefit at all. You cannot even use the PHY sub-nodes for any
setup properties, as they depend on the consumer claiming the lane.

Sebastian