Re: [PATCH 1/5] Phy: DT binding documentation for Marvell MVEBU SATA phy.

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Dec 05, 2013 at 11:33:46AM +0530, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 04 December 2013 10:46 PM, Andrew Lunn wrote:
> > Describe the binding for the Marvell MVEBU SATA phy. This driver
> > can be used at least with Kirkwood, Dove and maybe others.
> > Additionally, update the SATA binding with the properties to link
> > to the phy nodes.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  Documentation/devicetree/bindings/ata/marvell.txt  |  6 ++++++
> >  .../devicetree/bindings/phy/phy-mvebu-sata.txt     | 22 ++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-sata.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
> > index b5cdd20cde9c..e072fa105b49 100644
> > --- a/Documentation/devicetree/bindings/ata/marvell.txt
> > +++ b/Documentation/devicetree/bindings/ata/marvell.txt
> > @@ -6,11 +6,17 @@ Required Properties:
> >  - interrupts    : Interrupt controller is using
> >  - nr-ports      : Number of SATA ports in use.
> >  
> > +Optional Properties:
> > +- phys		: List of phandles to sata phys
> > +- phy-names	: Should be "0", "1", etc, one number per phandle
> 
> over aligned..

Could you explain please? Here is what it looks like without the patch
formatting, which could be messing up the display of tabs, due to the
+ at the beginning:

Required Properties:
- compatibility : "marvell,orion-sata"
- reg		: Address range of controller
- interrupts	: Interrupt controller is using
- nr-ports	: Number of SATA ports in use.

Optional Properties:
- phys		: List of phandles to sata phys
- phy-names	: Should be "0", "1", etc, one number per phandle



> > +
> >  Example:
> >  
> >  	sata@80000 {
> >  		compatible = "marvell,orion-sata";
> >  		reg = <0x80000 0x5000>;
> >  		interrupts = <21>;
> > +		phys = <&sata_phy0>, <&sata_phy1>;
> > +		phy-names = "0", "1";
> 
> more descriptive phy-names? sata-phy0?
> >  		nr-ports = <2>;

I could do, but i was following how the clocks work. Unfortunately,
the binding documentation is out of date and does not contain
clocks. A real example is:

		sata@80000 {
			compatible = "marvell,orion-sata";
			reg = <0x80000 0x5000>;
			interrupts = <21>;
			clocks = <&gate_clk 14>, <&gate_clk 15>;
			clock-names = "0", "1";
			phys = <&sata_phy0>, <&sata_phy1>;
			phy-names = "0", "1";
			status = "disabled";
		};

So clocks and the phy are described nearly identically. I can however
handle phys differently if you wish.

I will also submit a separate patch to the binding documentation to
add the clocks.


> >  	}
> > diff --git a/Documentation/devicetree/bindings/phy/phy-mvebu-sata.txt b/Documentation/devicetree/bindings/phy/phy-mvebu-sata.txt
> > new file mode 100644
> > index 000000000000..1cf9cef50b4b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/phy-mvebu-sata.txt
> 
> Just name this mvebu-phy.txt so that we can add bindings of other mvebu PHYs
> here when it's added.

O.K. I also have a pcie phy driver, but it does not work
yet. devm_phy_get() is too restrictive for a complex device like a
PCIe controller.

> > +		sata-phy@1 {
> 
> The value after '@' must match the first address specified in the reg property
> of the node according to the ePAPR spec.

O.K, will fix.

> > +			compatible = "marvell,mvebu-sata-phy";
> > +			reg = <0x84000 0x0334>;
> > +			clocks = <&gate_clk 15>;
> > +			clock-names = "sata";
> > +			#phy-cells = <1>;
> 
> Is it on purpose that your are having phy-cells value to 1?

Yes. Each instance only controls one phy.

     Thanks
	Andrew