diff mbox

[v4,3/4] dt-bindings: phy: Add support for QMP phy

Message ID 1484045519-19030-4-git-send-email-vivek.gautam@codeaurora.org
State Not Applicable, archived
Headers show

Commit Message

Vivek Gautam Jan. 10, 2017, 10:51 a.m. UTC
Qualcomm chipsets have QMP phy controller that provides
support to a number of controller, viz. PCIe, UFS, and USB.
Adding dt binding information for the same.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Acked-by: Rob Herring <robh@kernel.org>
---

Changes since v3:
 - Added #clock-cells = <1>, indicating that phy is a clock provider.

Changes since v2:
 - Removed binding for "ref_clk_src" since we don't request this
   clock in the driver.
 - Addressed s/ref_clk/ref. Don't need to add '_clk' suffix to clock names.
 - Using 'phy' for the node name.

Changes since v1:
 - New patch, forked out of the original driver patch:
   "phy: qcom-qmp: new qmp phy driver for qcom-chipsets"
 - Added 'Acked-by' from Rob.
 - Updated bindings to include mem resource as a list of
   offset - length pair for serdes block and for each lane.
 - Added a new binding for 'lane-offsets' that contains offsets
   to tx, rx and pcs blocks from each lane base address.

 .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 76 ++++++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt

Comments

Kishon Vijay Abraham I Jan. 16, 2017, 8:49 a.m. UTC | #1
Hi,

On Tuesday 10 January 2017 04:21 PM, Vivek Gautam wrote:
> Qualcomm chipsets have QMP phy controller that provides
> support to a number of controller, viz. PCIe, UFS, and USB.
> Adding dt binding information for the same.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> 
> Changes since v3:
>  - Added #clock-cells = <1>, indicating that phy is a clock provider.
> 
> Changes since v2:
>  - Removed binding for "ref_clk_src" since we don't request this
>    clock in the driver.
>  - Addressed s/ref_clk/ref. Don't need to add '_clk' suffix to clock names.
>  - Using 'phy' for the node name.
> 
> Changes since v1:
>  - New patch, forked out of the original driver patch:
>    "phy: qcom-qmp: new qmp phy driver for qcom-chipsets"
>  - Added 'Acked-by' from Rob.
>  - Updated bindings to include mem resource as a list of
>    offset - length pair for serdes block and for each lane.
>  - Added a new binding for 'lane-offsets' that contains offsets
>    to tx, rx and pcs blocks from each lane base address.
> 
>  .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 76 ++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> new file mode 100644
> index 000000000000..6f510fe48f46
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> @@ -0,0 +1,76 @@
> +Qualcomm QMP PHY controller
> +===========================
> +
> +QMP phy controller supports physical layer functionality for a number of
> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
> +
> +Required properties:
> + - compatible: compatible list, contains:
> +	       "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
> +	       "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
> + - reg: list of offset and length pair of the PHY register sets.
> +	at index 0: offset and length of register set for PHY common
> +		    serdes block.
> +	from index 1 - N: offset and length of register set for each lane,
> +			  for N number of phy lanes (ports).
> + - lane-offsets: array of offsets to tx, rx and pcs blocks for phy lanes.
> + - #phy-cells: must be 1
> +    - Cell after phy phandle should be the port (lane) number.
> + - #clock-cells: must be 1
> +    - Phy pll outputs a bunch of clocks for Tx, Rx and Pipe
> +      interface (for pipe based PHYs). These clock are then gate-controlled
> +      by gcc.
> + - clocks: a list of phandles and clock-specifier pairs,
> +	   one for each entry in clock-names.
> + - clock-names: must be "cfg_ahb" for phy config clock,
> +			"aux" for phy aux clock,
> +			"ref" for 19.2 MHz ref clk,
> +			"pipe<port-number>" for pipe clock specific to
> +			each port/lane (Optional).
> + - resets: a list of phandles and reset controller specifier pairs,
> +	   one for each entry in reset-names.
> + - reset-names: must be "phy" for reset of phy block,
> +			"common" for phy common block reset,
> +			"cfg" for phy's ahb cfg block reset (Optional).
> +			"port<port-number>" for reset specific to
> +			each port/lane (Optional).
> + - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
> +
> +Optional properties:
> + - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
> +			pll block.
> +
> +Example:
> +	pcie_phy: phy@34000 {
> +		compatible = "qcom,msm8996-qmp-pcie-phy";
> +		reg = <0x034000 0x48f>,
> +			<0x035000 0x5bf>,
> +			<0x036000 0x5bf>,
> +			<0x037000 0x5bf>;
> +				/* tx, rx, pcs */
> +		lane-offsets = <0x0 0x200 0x400>;
> +		#phy-cells = <1>;
> +		#clock-cells = <1>;
> +
> +		clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
> +			<&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
> +			<&gcc GCC_PCIE_CLKREF_CLK>,
> +			<&gcc GCC_PCIE_0_PIPE_CLK>,
> +			<&gcc GCC_PCIE_1_PIPE_CLK>,
> +			<&gcc GCC_PCIE_2_PIPE_CLK>;
> +		clock-names = "aux", "cfg_ahb", "ref",
> +				"pipe0", "pipe1", "pipe2";
> +
> +		vdda-phy-supply = <&pm8994_l28>;
> +		vdda-pll-supply = <&pm8994_l12>;
> +
> +		resets = <&gcc GCC_PCIE_PHY_BCR>,
> +			<&gcc GCC_PCIE_PHY_COM_BCR>,
> +			<&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>,
> +			<&gcc GCC_PCIE_0_PHY_BCR>,
> +			<&gcc GCC_PCIE_1_PHY_BCR>,
> +			<&gcc GCC_PCIE_2_PHY_BCR>;
> +		reset-names = "phy", "common", "cfg",
> +				"lane0", "lane1", "lane2";

Each lane has a separate clock, separate reset.. why not create sub-nodes for
each lane?

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivek Gautam Jan. 18, 2017, 6:54 a.m. UTC | #2
Hi Kishon,


On 01/16/2017 02:19 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 10 January 2017 04:21 PM, Vivek Gautam wrote:
>> Qualcomm chipsets have QMP phy controller that provides
>> support to a number of controller, viz. PCIe, UFS, and USB.
>> Adding dt binding information for the same.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>
>> Changes since v3:
>>   - Added #clock-cells = <1>, indicating that phy is a clock provider.
>>
>> Changes since v2:
>>   - Removed binding for "ref_clk_src" since we don't request this
>>     clock in the driver.
>>   - Addressed s/ref_clk/ref. Don't need to add '_clk' suffix to clock names.
>>   - Using 'phy' for the node name.
>>
>> Changes since v1:
>>   - New patch, forked out of the original driver patch:
>>     "phy: qcom-qmp: new qmp phy driver for qcom-chipsets"
>>   - Added 'Acked-by' from Rob.
>>   - Updated bindings to include mem resource as a list of
>>     offset - length pair for serdes block and for each lane.
>>   - Added a new binding for 'lane-offsets' that contains offsets
>>     to tx, rx and pcs blocks from each lane base address.
>>
>>   .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 76 ++++++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> new file mode 100644
>> index 000000000000..6f510fe48f46
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> @@ -0,0 +1,76 @@
>> +Qualcomm QMP PHY controller
>> +===========================
>> +
>> +QMP phy controller supports physical layer functionality for a number of
>> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
>> +
>> +Required properties:
>> + - compatible: compatible list, contains:
>> +	       "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
>> +	       "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
>> + - reg: list of offset and length pair of the PHY register sets.
>> +	at index 0: offset and length of register set for PHY common
>> +		    serdes block.
>> +	from index 1 - N: offset and length of register set for each lane,
>> +			  for N number of phy lanes (ports).
>> + - lane-offsets: array of offsets to tx, rx and pcs blocks for phy lanes.
>> + - #phy-cells: must be 1
>> +    - Cell after phy phandle should be the port (lane) number.
>> + - #clock-cells: must be 1
>> +    - Phy pll outputs a bunch of clocks for Tx, Rx and Pipe
>> +      interface (for pipe based PHYs). These clock are then gate-controlled
>> +      by gcc.
>> + - clocks: a list of phandles and clock-specifier pairs,
>> +	   one for each entry in clock-names.
>> + - clock-names: must be "cfg_ahb" for phy config clock,
>> +			"aux" for phy aux clock,
>> +			"ref" for 19.2 MHz ref clk,
>> +			"pipe<port-number>" for pipe clock specific to
>> +			each port/lane (Optional).
>> + - resets: a list of phandles and reset controller specifier pairs,
>> +	   one for each entry in reset-names.
>> + - reset-names: must be "phy" for reset of phy block,
>> +			"common" for phy common block reset,
>> +			"cfg" for phy's ahb cfg block reset (Optional).
>> +			"port<port-number>" for reset specific to
>> +			each port/lane (Optional).
>> + - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
>> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
>> +
>> +Optional properties:
>> + - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
>> +			pll block.
>> +
>> +Example:
>> +	pcie_phy: phy@34000 {
>> +		compatible = "qcom,msm8996-qmp-pcie-phy";
>> +		reg = <0x034000 0x48f>,
>> +			<0x035000 0x5bf>,
>> +			<0x036000 0x5bf>,
>> +			<0x037000 0x5bf>;
>> +				/* tx, rx, pcs */
>> +		lane-offsets = <0x0 0x200 0x400>;
>> +		#phy-cells = <1>;
>> +		#clock-cells = <1>;
>> +
>> +		clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
>> +			<&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
>> +			<&gcc GCC_PCIE_CLKREF_CLK>,
>> +			<&gcc GCC_PCIE_0_PIPE_CLK>,
>> +			<&gcc GCC_PCIE_1_PIPE_CLK>,
>> +			<&gcc GCC_PCIE_2_PIPE_CLK>;
>> +		clock-names = "aux", "cfg_ahb", "ref",
>> +				"pipe0", "pipe1", "pipe2";
>> +
>> +		vdda-phy-supply = <&pm8994_l28>;
>> +		vdda-pll-supply = <&pm8994_l12>;
>> +
>> +		resets = <&gcc GCC_PCIE_PHY_BCR>,
>> +			<&gcc GCC_PCIE_PHY_COM_BCR>,
>> +			<&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>,
>> +			<&gcc GCC_PCIE_0_PHY_BCR>,
>> +			<&gcc GCC_PCIE_1_PHY_BCR>,
>> +			<&gcc GCC_PCIE_2_PHY_BCR>;
>> +		reset-names = "phy", "common", "cfg",
>> +				"lane0", "lane1", "lane2";
> Each lane has a separate clock, separate reset.. why not create sub-nodes for
> each lane?

Yes, each lane has separate pipe clock and resets.
I can have a binding such as written below.
Does it makes sense to pull in the tx, rx and pcs offsets as well
to the child node, and iomap the entire address space of the phy ?

+ pcie_phy: pciephy@34000 {
+               compatible = "qcom,qmp-14nm-phy";
+               reg = <0x034000 0x3fff>;
+               #address-cells = <1>;
+               #size-cells = <0>;

                   ...

+               port@0 {
+                       reg = <0>;
+                       tx-offset = <0x1000>;
+                       rx-offset = <0x1200>;
+                       pcs-offset = <0x1400>;
+                       clocks = <&gcc GCC_USB3_PHY_PIPE_CLK>;
+                       clock-names = "pipe0";
+                       resets = <&gcc GCC_PCIE_0_PHY_BCR>;
+                       reset-names = "lane0";
+               };
+ };


Regards
Vivek
>
> Thanks
> Kishon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Jan. 18, 2017, 6:22 p.m. UTC | #3
On Tue 17 Jan 22:54 PST 2017, Vivek Gautam wrote:
> On 01/16/2017 02:19 PM, Kishon Vijay Abraham I wrote:
> > On Tuesday 10 January 2017 04:21 PM, Vivek Gautam wrote:
[..]
> > > +		reset-names = "phy", "common", "cfg",
> > > +				"lane0", "lane1", "lane2";
> > Each lane has a separate clock, separate reset.. why not create sub-nodes for
> > each lane?
> 
> Yes, each lane has separate pipe clock and resets.
> I can have a binding such as written below.

+1

> Does it makes sense to pull in the tx, rx and pcs offsets as well
> to the child node, and iomap the entire address space of the phy ?
> 

Note that you don't have to follow the same structure in your device
driver as you describe your hardware in devicetree.

I would suggest that you replace the lane-offset and various lane
specific resources with subnodes, but keep the driver "as is".

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Jan. 19, 2017, 12:40 a.m. UTC | #4
On 01/18, Bjorn Andersson wrote:
> On Tue 17 Jan 22:54 PST 2017, Vivek Gautam wrote:
> > On 01/16/2017 02:19 PM, Kishon Vijay Abraham I wrote:
> > > On Tuesday 10 January 2017 04:21 PM, Vivek Gautam wrote:
> [..]
> > > > +		reset-names = "phy", "common", "cfg",
> > > > +				"lane0", "lane1", "lane2";
> > > Each lane has a separate clock, separate reset.. why not create sub-nodes for
> > > each lane?
> > 
> > Yes, each lane has separate pipe clock and resets.
> > I can have a binding such as written below.
> 
> +1
> 
> > Does it makes sense to pull in the tx, rx and pcs offsets as well
> > to the child node, and iomap the entire address space of the phy ?
> > 
> 
> Note that you don't have to follow the same structure in your device
> driver as you describe your hardware in devicetree.
> 
> I would suggest that you replace the lane-offset and various lane
> specific resources with subnodes, but keep the driver "as is".
> 

Didn't we already move away from subnodes for lanes in an earlier
revision of these patches? I seem to recall we did that because
lanes are not devices and the whole "phy as a bus" concept not
making sense.
Vivek Gautam Jan. 19, 2017, 5:12 a.m. UTC | #5
On 01/19/2017 06:10 AM, Stephen Boyd wrote:
> On 01/18, Bjorn Andersson wrote:
>> On Tue 17 Jan 22:54 PST 2017, Vivek Gautam wrote:
>>> On 01/16/2017 02:19 PM, Kishon Vijay Abraham I wrote:
>>>> On Tuesday 10 January 2017 04:21 PM, Vivek Gautam wrote:
>> [..]
>>>>> +		reset-names = "phy", "common", "cfg",
>>>>> +				"lane0", "lane1", "lane2";
>>>> Each lane has a separate clock, separate reset.. why not create sub-nodes for
>>>> each lane?
>>> Yes, each lane has separate pipe clock and resets.
>>> I can have a binding such as written below.
>> +1
>>
>>> Does it makes sense to pull in the tx, rx and pcs offsets as well
>>> to the child node, and iomap the entire address space of the phy ?
>>>
>> Note that you don't have to follow the same structure in your device
>> driver as you describe your hardware in devicetree.
>>
>> I would suggest that you replace the lane-offset and various lane
>> specific resources with subnodes, but keep the driver "as is".
>>
> Didn't we already move away from subnodes for lanes in an earlier
> revision of these patches? I seem to recall we did that because
> lanes are not devices and the whole "phy as a bus" concept not
> making sense.

Yea, we started out without having any sub-nodes and we
argued that we don't require them since the qmp device is
represented by the qmp node itself.
The lanes otoh are representative of gen_phys and related properties.

In the driver -
"struct qmp_phy " represents the lanes and holds "struct phy",
"struct qcom_qmp" represents the qmp block as a whole and holds "struct 
device"
Does this make lanes qualify to be childs of qmp ?

"phy as a bus" (just trying to understand here) -
let's say a usb phy controller has one HSIC phy port and one USB2 phy port.
So, should this phy controller be a bus providing two ports (and so we 
will have
couple of child nodes to the phy controller) ?


Regards
Vivek
Stephen Boyd Jan. 19, 2017, 9:42 p.m. UTC | #6
On 01/19, Vivek Gautam wrote:
> 
> On 01/19/2017 06:10 AM, Stephen Boyd wrote:
> >>
> >Didn't we already move away from subnodes for lanes in an earlier
> >revision of these patches? I seem to recall we did that because
> >lanes are not devices and the whole "phy as a bus" concept not
> >making sense.
> 
> Yea, we started out without having any sub-nodes and we
> argued that we don't require them since the qmp device is
> represented by the qmp node itself.
> The lanes otoh are representative of gen_phys and related properties.
> 
> In the driver -
> "struct qmp_phy " represents the lanes and holds "struct phy",
> "struct qcom_qmp" represents the qmp block as a whole and holds
> "struct device"
> Does this make lanes qualify to be childs of qmp ?

Hmm... maybe I was recalling the DSI phy binding. I think there
are lanes there too but we decided to just have one node.

> 
> "phy as a bus" (just trying to understand here) -
> let's say a usb phy controller has one HSIC phy port and one USB2 phy port.
> So, should this phy controller be a bus providing two ports (and so
> we will have
> couple of child nodes to the phy controller) ?
> 

Typically in DT a subnode or collection of subnodes means there's
some sort of bus involved. Usually each node corresponds to a
struct device, and the parent node corresponds to the bus or
controller for the logical bus.

In this case (only PCIe though? not UFS or USB?) it seems like we
have multiple phys that share a common register space, but
otherwise they have their own register space and power
management. Would you have each PCIe controller point to a
different subnode for their associated phy? I'm trying to
understand the benefit of the subnodes if they aren't treated as
struct devices.

At the least, please get DT reviewers to ack the new binding
before rewriting the code.
Vivek Gautam Jan. 23, 2017, 12:22 p.m. UTC | #7
On 01/20/2017 03:12 AM, Stephen Boyd wrote:
> On 01/19, Vivek Gautam wrote:
>> On 01/19/2017 06:10 AM, Stephen Boyd wrote:
>>> Didn't we already move away from subnodes for lanes in an earlier
>>> revision of these patches? I seem to recall we did that because
>>> lanes are not devices and the whole "phy as a bus" concept not
>>> making sense.
>> Yea, we started out without having any sub-nodes and we
>> argued that we don't require them since the qmp device is
>> represented by the qmp node itself.
>> The lanes otoh are representative of gen_phys and related properties.
>>
>> In the driver -
>> "struct qmp_phy " represents the lanes and holds "struct phy",
>> "struct qcom_qmp" represents the qmp block as a whole and holds
>> "struct device"
>> Does this make lanes qualify to be childs of qmp ?
> Hmm... maybe I was recalling the DSI phy binding. I think there
> are lanes there too but we decided to just have one node.
>
>> "phy as a bus" (just trying to understand here) -
>> let's say a usb phy controller has one HSIC phy port and one USB2 phy port.
>> So, should this phy controller be a bus providing two ports (and so
>> we will have
>> couple of child nodes to the phy controller) ?
>>
> Typically in DT a subnode or collection of subnodes means there's
> some sort of bus involved. Usually each node corresponds to a
> struct device, and the parent node corresponds to the bus or
> controller for the logical bus.
>
> In this case (only PCIe though? not UFS or USB?) it seems like we
> have multiple phys that share a common register space, but
> otherwise they have their own register space and power
> management. Would you have each PCIe controller point to a
> different subnode for their associated phy? I'm trying to
> understand the benefit of the subnodes if they aren't treated as
> struct devices.

AFAIU, It's not straight that forward to point each controller to
different subnodes and not associate a 'struct device' with subnodes.

The phy translation (of_phy_simple_xlate, in case we point each controller
to subnodes) will not be able to associate a correct phy_provider device
to the phy consumer.

Kishon,
Is my understanding correct here? Please correct me if i am missing things.


Regards
Vivek
>
> At the least, please get DT reviewers to ack the new binding
> before rewriting the code.
>
Kishon Vijay Abraham I Jan. 24, 2017, 9:33 a.m. UTC | #8
Hi,

On Friday 20 January 2017 03:12 AM, Stephen Boyd wrote:
> On 01/19, Vivek Gautam wrote:
>>
>> On 01/19/2017 06:10 AM, Stephen Boyd wrote:
>>>>
>>> Didn't we already move away from subnodes for lanes in an earlier
>>> revision of these patches? I seem to recall we did that because
>>> lanes are not devices and the whole "phy as a bus" concept not
>>> making sense.
>>
>> Yea, we started out without having any sub-nodes and we
>> argued that we don't require them since the qmp device is
>> represented by the qmp node itself.
>> The lanes otoh are representative of gen_phys and related properties.
>>
>> In the driver -
>> "struct qmp_phy " represents the lanes and holds "struct phy",
>> "struct qcom_qmp" represents the qmp block as a whole and holds
>> "struct device"
>> Does this make lanes qualify to be childs of qmp ?
> 
> Hmm... maybe I was recalling the DSI phy binding. I think there
> are lanes there too but we decided to just have one node.
> 
>>
>> "phy as a bus" (just trying to understand here) -
>> let's say a usb phy controller has one HSIC phy port and one USB2 phy port.
>> So, should this phy controller be a bus providing two ports (and so
>> we will have
>> couple of child nodes to the phy controller) ?
>>
> 
> Typically in DT a subnode or collection of subnodes means there's
> some sort of bus involved. Usually each node corresponds to a
> struct device, and the parent node corresponds to the bus or
> controller for the logical bus.
> 
> In this case (only PCIe though? not UFS or USB?) it seems like we
> have multiple phys that share a common register space, but
> otherwise they have their own register space and power
> management. Would you have each PCIe controller point to a
> different subnode for their associated phy? I'm trying to
> understand the benefit of the subnodes if they aren't treated as
> struct devices.

Yes, instead of having all the controller having a phandle to the same PHY and
then using other mechanisms to differentiate between the PHYs, each controller
can have a phandle to the exact port that it is connected to.

This also gives a better representation of the hardware and can avoid lot of
boilerplate code in the driver.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivek Gautam Jan. 24, 2017, 2:05 p.m. UTC | #9
On Tue, Jan 24, 2017 at 3:03 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi,
>
> On Friday 20 January 2017 03:12 AM, Stephen Boyd wrote:
>> On 01/19, Vivek Gautam wrote:
>>>
>>> On 01/19/2017 06:10 AM, Stephen Boyd wrote:
>>>>>
>>>> Didn't we already move away from subnodes for lanes in an earlier
>>>> revision of these patches? I seem to recall we did that because
>>>> lanes are not devices and the whole "phy as a bus" concept not
>>>> making sense.
>>>
>>> Yea, we started out without having any sub-nodes and we
>>> argued that we don't require them since the qmp device is
>>> represented by the qmp node itself.
>>> The lanes otoh are representative of gen_phys and related properties.
>>>
>>> In the driver -
>>> "struct qmp_phy " represents the lanes and holds "struct phy",
>>> "struct qcom_qmp" represents the qmp block as a whole and holds
>>> "struct device"
>>> Does this make lanes qualify to be childs of qmp ?
>>
>> Hmm... maybe I was recalling the DSI phy binding. I think there
>> are lanes there too but we decided to just have one node.
>>
>>>
>>> "phy as a bus" (just trying to understand here) -
>>> let's say a usb phy controller has one HSIC phy port and one USB2 phy port.
>>> So, should this phy controller be a bus providing two ports (and so
>>> we will have
>>> couple of child nodes to the phy controller) ?
>>>
>>
>> Typically in DT a subnode or collection of subnodes means there's
>> some sort of bus involved. Usually each node corresponds to a
>> struct device, and the parent node corresponds to the bus or
>> controller for the logical bus.
>>
>> In this case (only PCIe though? not UFS or USB?) it seems like we
>> have multiple phys that share a common register space, but
>> otherwise they have their own register space and power
>> management. Would you have each PCIe controller point to a
>> different subnode for their associated phy? I'm trying to
>> understand the benefit of the subnodes if they aren't treated as
>> struct devices.
>
> Yes, instead of having all the controller having a phandle to the same PHY and
> then using other mechanisms to differentiate between the PHYs, each controller
> can have a phandle to the exact port that it is connected to.
>
> This also gives a better representation of the hardware and can avoid lot of
> boilerplate code in the driver.

Below is one binding that works for me.
--------------------
               phy@34000 {
                        compatible = "qcom,msm8996-qmp-pcie-phy";
                        reg = <0x034000 0x488>;
                        #clock-cells = <1>;
                        #address-cells = <1>;
                        #size-cells = <1>;
                        ranges;

                        clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
                                <&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
                                <&gcc GCC_PCIE_CLKREF_CLK>;
                        clock-names = "aux", "cfg_ahb", "ref";

                        vdda-phy-supply = <&pm8994_l28>;
                        vdda-pll-supply = <&pm8994_l12>;

                        resets = <&gcc GCC_PCIE_PHY_BCR>,
                                <&gcc GCC_PCIE_PHY_COM_BCR>,
                                <&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>;
                        reset-names = "phy", "common", "cfg";

                        pciephy_p0: port@0 {
                                reg = <0x035000 0x130>,
                                        <0x035200 0x200>,
                                        <0x035400 0x1dc>;
                                #phy-cells = <0>;

                                clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
                                clock-names = "pipe0";
                                resets = <&gcc GCC_PCIE_0_PHY_BCR>;
                                reset-names = "lane0";
                        };

                      pciephy_p1: port@1 {
                                reg = <0x036000 0x130>,
                                        <0x036200 0x200>,
                                        <0x036400 0x1dc>;
                                #phy-cells = <0>;

                                clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
                                clock-names = "pipe1";
                                resets = <&gcc GCC_PCIE_1_PHY_BCR>;
                                reset-names = "lane1";
                        };

                        pciephy_p2: port@2 {
                                reg = <0x037000 0x130>,
                                        <0x037200 0x200>,
                                        <0x037400 0x1dc>;
                                #phy-cells = <0>;

                                clocks = <&gcc GCC_PCIE_2_PIPE_CLK>;
                                clock-names = "pipe2";
                                resets = <&gcc GCC_PCIE_2_PHY_BCR>;
                                reset-names = "lane2";
                        };
                };
--------------------

let me know if this looks okay.


Regards
Vivek

>
> Thanks
> Kishon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I Jan. 24, 2017, 2:15 p.m. UTC | #10
Hi,

On Tuesday 24 January 2017 07:35 PM, Vivek Gautam wrote:
> On Tue, Jan 24, 2017 at 3:03 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> Hi,
>>
>> On Friday 20 January 2017 03:12 AM, Stephen Boyd wrote:
>>> On 01/19, Vivek Gautam wrote:
>>>>
>>>> On 01/19/2017 06:10 AM, Stephen Boyd wrote:
>>>>>>
>>>>> Didn't we already move away from subnodes for lanes in an earlier
>>>>> revision of these patches? I seem to recall we did that because
>>>>> lanes are not devices and the whole "phy as a bus" concept not
>>>>> making sense.
>>>>
>>>> Yea, we started out without having any sub-nodes and we
>>>> argued that we don't require them since the qmp device is
>>>> represented by the qmp node itself.
>>>> The lanes otoh are representative of gen_phys and related properties.
>>>>
>>>> In the driver -
>>>> "struct qmp_phy " represents the lanes and holds "struct phy",
>>>> "struct qcom_qmp" represents the qmp block as a whole and holds
>>>> "struct device"
>>>> Does this make lanes qualify to be childs of qmp ?
>>>
>>> Hmm... maybe I was recalling the DSI phy binding. I think there
>>> are lanes there too but we decided to just have one node.
>>>
>>>>
>>>> "phy as a bus" (just trying to understand here) -
>>>> let's say a usb phy controller has one HSIC phy port and one USB2 phy port.
>>>> So, should this phy controller be a bus providing two ports (and so
>>>> we will have
>>>> couple of child nodes to the phy controller) ?
>>>>
>>>
>>> Typically in DT a subnode or collection of subnodes means there's
>>> some sort of bus involved. Usually each node corresponds to a
>>> struct device, and the parent node corresponds to the bus or
>>> controller for the logical bus.
>>>
>>> In this case (only PCIe though? not UFS or USB?) it seems like we
>>> have multiple phys that share a common register space, but
>>> otherwise they have their own register space and power
>>> management. Would you have each PCIe controller point to a
>>> different subnode for their associated phy? I'm trying to
>>> understand the benefit of the subnodes if they aren't treated as
>>> struct devices.
>>
>> Yes, instead of having all the controller having a phandle to the same PHY and
>> then using other mechanisms to differentiate between the PHYs, each controller
>> can have a phandle to the exact port that it is connected to.
>>
>> This also gives a better representation of the hardware and can avoid lot of
>> boilerplate code in the driver.
> 
> Below is one binding that works for me.
> --------------------
>                phy@34000 {
>                         compatible = "qcom,msm8996-qmp-pcie-phy";
>                         reg = <0x034000 0x488>;
>                         #clock-cells = <1>;
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>                         ranges;
> 
>                         clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
>                                 <&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
>                                 <&gcc GCC_PCIE_CLKREF_CLK>;
>                         clock-names = "aux", "cfg_ahb", "ref";
> 
>                         vdda-phy-supply = <&pm8994_l28>;
>                         vdda-pll-supply = <&pm8994_l12>;
> 
>                         resets = <&gcc GCC_PCIE_PHY_BCR>,
>                                 <&gcc GCC_PCIE_PHY_COM_BCR>,
>                                 <&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>;
>                         reset-names = "phy", "common", "cfg";
> 
>                         pciephy_p0: port@0 {
>                                 reg = <0x035000 0x130>,
>                                         <0x035200 0x200>,
>                                         <0x035400 0x1dc>;
>                                 #phy-cells = <0>;
> 
>                                 clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
>                                 clock-names = "pipe0";
>                                 resets = <&gcc GCC_PCIE_0_PHY_BCR>;
>                                 reset-names = "lane0";
>                         };
> 
>                       pciephy_p1: port@1 {
>                                 reg = <0x036000 0x130>,
>                                         <0x036200 0x200>,
>                                         <0x036400 0x1dc>;
>                                 #phy-cells = <0>;
> 
>                                 clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
>                                 clock-names = "pipe1";
>                                 resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>                                 reset-names = "lane1";
>                         };
> 
>                         pciephy_p2: port@2 {
>                                 reg = <0x037000 0x130>,
>                                         <0x037200 0x200>,
>                                         <0x037400 0x1dc>;
>                                 #phy-cells = <0>;
> 
>                                 clocks = <&gcc GCC_PCIE_2_PIPE_CLK>;
>                                 clock-names = "pipe2";
>                                 resets = <&gcc GCC_PCIE_2_PHY_BCR>;
>                                 reset-names = "lane2";
>                         };
>                 };
> --------------------
> 
> let me know if this looks okay.

looks good to me.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivek Gautam Jan. 24, 2017, 4:40 p.m. UTC | #11
Hi,


On Tue, Jan 24, 2017 at 7:45 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi,
>
> On Tuesday 24 January 2017 07:35 PM, Vivek Gautam wrote:
>> On Tue, Jan 24, 2017 at 3:03 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>> Hi,
>>>
>>> On Friday 20 January 2017 03:12 AM, Stephen Boyd wrote:
>>>> On 01/19, Vivek Gautam wrote:
>>>>>
>>>>> On 01/19/2017 06:10 AM, Stephen Boyd wrote:
>>>>>>>
>>>>>> Didn't we already move away from subnodes for lanes in an earlier
>>>>>> revision of these patches? I seem to recall we did that because
>>>>>> lanes are not devices and the whole "phy as a bus" concept not
>>>>>> making sense.
>>>>>
>>>>> Yea, we started out without having any sub-nodes and we
>>>>> argued that we don't require them since the qmp device is
>>>>> represented by the qmp node itself.
>>>>> The lanes otoh are representative of gen_phys and related properties.
>>>>>
>>>>> In the driver -
>>>>> "struct qmp_phy " represents the lanes and holds "struct phy",
>>>>> "struct qcom_qmp" represents the qmp block as a whole and holds
>>>>> "struct device"
>>>>> Does this make lanes qualify to be childs of qmp ?
>>>>
>>>> Hmm... maybe I was recalling the DSI phy binding. I think there
>>>> are lanes there too but we decided to just have one node.
>>>>
>>>>>
>>>>> "phy as a bus" (just trying to understand here) -
>>>>> let's say a usb phy controller has one HSIC phy port and one USB2 phy port.
>>>>> So, should this phy controller be a bus providing two ports (and so
>>>>> we will have
>>>>> couple of child nodes to the phy controller) ?
>>>>>
>>>>
>>>> Typically in DT a subnode or collection of subnodes means there's
>>>> some sort of bus involved. Usually each node corresponds to a
>>>> struct device, and the parent node corresponds to the bus or
>>>> controller for the logical bus.
>>>>
>>>> In this case (only PCIe though? not UFS or USB?) it seems like we
>>>> have multiple phys that share a common register space, but
>>>> otherwise they have their own register space and power
>>>> management. Would you have each PCIe controller point to a
>>>> different subnode for their associated phy? I'm trying to
>>>> understand the benefit of the subnodes if they aren't treated as
>>>> struct devices.
>>>
>>> Yes, instead of having all the controller having a phandle to the same PHY and
>>> then using other mechanisms to differentiate between the PHYs, each controller
>>> can have a phandle to the exact port that it is connected to.
>>>
>>> This also gives a better representation of the hardware and can avoid lot of
>>> boilerplate code in the driver.
>>
>> Below is one binding that works for me.
>> --------------------
>>                phy@34000 {
>>                         compatible = "qcom,msm8996-qmp-pcie-phy";
>>                         reg = <0x034000 0x488>;
>>                         #clock-cells = <1>;
>>                         #address-cells = <1>;
>>                         #size-cells = <1>;
>>                         ranges;
>>
>>                         clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
>>                                 <&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
>>                                 <&gcc GCC_PCIE_CLKREF_CLK>;
>>                         clock-names = "aux", "cfg_ahb", "ref";
>>
>>                         vdda-phy-supply = <&pm8994_l28>;
>>                         vdda-pll-supply = <&pm8994_l12>;
>>
>>                         resets = <&gcc GCC_PCIE_PHY_BCR>,
>>                                 <&gcc GCC_PCIE_PHY_COM_BCR>,
>>                                 <&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>;
>>                         reset-names = "phy", "common", "cfg";
>>
>>                         pciephy_p0: port@0 {
>>                                 reg = <0x035000 0x130>,
>>                                         <0x035200 0x200>,
>>                                         <0x035400 0x1dc>;
>>                                 #phy-cells = <0>;
>>
>>                                 clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
>>                                 clock-names = "pipe0";
>>                                 resets = <&gcc GCC_PCIE_0_PHY_BCR>;
>>                                 reset-names = "lane0";
>>                         };
>>
>>                       pciephy_p1: port@1 {
>>                                 reg = <0x036000 0x130>,
>>                                         <0x036200 0x200>,
>>                                         <0x036400 0x1dc>;
>>                                 #phy-cells = <0>;
>>
>>                                 clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
>>                                 clock-names = "pipe1";
>>                                 resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>>                                 reset-names = "lane1";
>>                         };
>>
>>                         pciephy_p2: port@2 {
>>                                 reg = <0x037000 0x130>,
>>                                         <0x037200 0x200>,
>>                                         <0x037400 0x1dc>;
>>                                 #phy-cells = <0>;
>>
>>                                 clocks = <&gcc GCC_PCIE_2_PIPE_CLK>;
>>                                 clock-names = "pipe2";
>>                                 resets = <&gcc GCC_PCIE_2_PHY_BCR>;
>>                                 reset-names = "lane2";
>>                         };
>>                 };
>> --------------------
>>
>> let me know if this looks okay.
>
> looks good to me.

Great.

Rob, please let me know if above bindings look okay to you.
I can re-spin the bindings patch then.

Thanks

Regards
Vivek

>
> Thanks
> Kishon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Jan. 26, 2017, 11:43 p.m. UTC | #12
On 01/24, Vivek Gautam wrote:
> 
> Below is one binding that works for me.
> --------------------
>                phy@34000 {
>                         compatible = "qcom,msm8996-qmp-pcie-phy";
>                         reg = <0x034000 0x488>;
>                         #clock-cells = <1>;
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>                         ranges;
> 
>                         clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
>                                 <&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
>                                 <&gcc GCC_PCIE_CLKREF_CLK>;
>                         clock-names = "aux", "cfg_ahb", "ref";
> 
>                         vdda-phy-supply = <&pm8994_l28>;
>                         vdda-pll-supply = <&pm8994_l12>;
> 
>                         resets = <&gcc GCC_PCIE_PHY_BCR>,
>                                 <&gcc GCC_PCIE_PHY_COM_BCR>,
>                                 <&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>;
>                         reset-names = "phy", "common", "cfg";
> 
>                         pciephy_p0: port@0 {

The unit address '@0' should be replaced with something from the
reg properties. 

Also 'port' and 'ports' are almost keywords in DT now with the
graph binding so we need to be careful when using them.

>                                 reg = <0x035000 0x130>,
>                                         <0x035200 0x200>,
>                                         <0x035400 0x1dc>;
>                                 #phy-cells = <0>;
> 
>                                 clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
>                                 clock-names = "pipe0";
>                                 resets = <&gcc GCC_PCIE_0_PHY_BCR>;
>                                 reset-names = "lane0";
>                         };
> 
>                       pciephy_p1: port@1 {
>                                 reg = <0x036000 0x130>,
>                                         <0x036200 0x200>,
>                                         <0x036400 0x1dc>;
>                                 #phy-cells = <0>;
> 
>                                 clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
>                                 clock-names = "pipe1";
>                                 resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>                                 reset-names = "lane1";
>                         };
> 
>                         pciephy_p2: port@2 {
>                                 reg = <0x037000 0x130>,
>                                         <0x037200 0x200>,
>                                         <0x037400 0x1dc>;
>                                 #phy-cells = <0>;
> 
>                                 clocks = <&gcc GCC_PCIE_2_PIPE_CLK>;
>                                 clock-names = "pipe2";
>                                 resets = <&gcc GCC_PCIE_2_PHY_BCR>;
>                                 reset-names = "lane2";
>                         };
>                 };
> --------------------
> 
> let me know if this looks okay.
> 
> 

What's the plan for non-pcie qmp phy binding? In that case we
don't have ports, so it gets folded into one node?
Vivek Gautam Jan. 27, 2017, 5:16 a.m. UTC | #13
On 01/27/2017 05:13 AM, Stephen Boyd wrote:
> On 01/24, Vivek Gautam wrote:
>> Below is one binding that works for me.
>> --------------------
>>                 phy@34000 {
>>                          compatible = "qcom,msm8996-qmp-pcie-phy";
>>                          reg = <0x034000 0x488>;
>>                          #clock-cells = <1>;
>>                          #address-cells = <1>;
>>                          #size-cells = <1>;
>>                          ranges;
>>
>>                          clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
>>                                  <&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
>>                                  <&gcc GCC_PCIE_CLKREF_CLK>;
>>                          clock-names = "aux", "cfg_ahb", "ref";
>>
>>                          vdda-phy-supply = <&pm8994_l28>;
>>                          vdda-pll-supply = <&pm8994_l12>;
>>
>>                          resets = <&gcc GCC_PCIE_PHY_BCR>,
>>                                  <&gcc GCC_PCIE_PHY_COM_BCR>,
>>                                  <&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>;
>>                          reset-names = "phy", "common", "cfg";
>>
>>                          pciephy_p0: port@0 {
> The unit address '@0' should be replaced with something from the
> reg properties.

Sure, will take care of this.

>   
>
> Also 'port' and 'ports' are almost keywords in DT now with the
> graph binding so we need to be careful when using them.

 From "./Documentation/devicetree/bindings/graph.txt" -
"The device tree graph bindings described herein abstract more complex
devices that can have multiple specifiable ports, each of which can be
linked to one or more ports of other devices."

So, this means we use 'port', 'ports' and 'endpoint' for devices whose one
or more ports is connected to other device's one or more ports.

I can use 'lane' for the node name here.

>
>>                                  reg = <0x035000 0x130>,
>>                                          <0x035200 0x200>,
>>                                          <0x035400 0x1dc>;
>>                                  #phy-cells = <0>;
>>
>>                                  clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
>>                                  clock-names = "pipe0";
>>                                  resets = <&gcc GCC_PCIE_0_PHY_BCR>;
>>                                  reset-names = "lane0";
>>                          };
>>
>>                        pciephy_p1: port@1 {
>>                                  reg = <0x036000 0x130>,
>>                                          <0x036200 0x200>,
>>                                          <0x036400 0x1dc>;
>>                                  #phy-cells = <0>;
>>
>>                                  clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
>>                                  clock-names = "pipe1";
>>                                  resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>>                                  reset-names = "lane1";
>>                          };
>>
>>                          pciephy_p2: port@2 {
>>                                  reg = <0x037000 0x130>,
>>                                          <0x037200 0x200>,
>>                                          <0x037400 0x1dc>;
>>                                  #phy-cells = <0>;
>>
>>                                  clocks = <&gcc GCC_PCIE_2_PIPE_CLK>;
>>                                  clock-names = "pipe2";
>>                                  resets = <&gcc GCC_PCIE_2_PHY_BCR>;
>>                                  reset-names = "lane2";
>>                          };
>>                  };
>> --------------------
>>
>> let me know if this looks okay.
>>
>>
> What's the plan for non-pcie qmp phy binding? In that case we
> don't have ports, so it gets folded into one node?
>
The non-pcie qmp phys still have one lane, that provides tx/rx.

I am of the opinion that we don't have two different ways to create
phys in the driver, and keep one port/lane for such phys in dt.


Regards
Vivek
Stephen Boyd March 7, 2017, 2 p.m. UTC | #14
(Not sure I replied so here it is)

On 01/27, Vivek Gautam wrote:
> 
> 
> On 01/27/2017 05:13 AM, Stephen Boyd wrote:
> >On 01/24, Vivek Gautam wrote:
> 
> From "./Documentation/devicetree/bindings/graph.txt" -
> "The device tree graph bindings described herein abstract more complex
> devices that can have multiple specifiable ports, each of which can be
> linked to one or more ports of other devices."
> 
> So, this means we use 'port', 'ports' and 'endpoint' for devices whose one
> or more ports is connected to other device's one or more ports.
> 
> I can use 'lane' for the node name here.

Ok.

> 
> >
> >>                                 reg = <0x035000 0x130>,
> >>                                         <0x035200 0x200>,
> >>                                         <0x035400 0x1dc>;
> >>                                 #phy-cells = <0>;
> >>
> >>                                 clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
> >>                                 clock-names = "pipe0";
> >>                                 resets = <&gcc GCC_PCIE_0_PHY_BCR>;
> >>                                 reset-names = "lane0";
> >>                         };
> >>
> >>                       pciephy_p1: port@1 {
> >>                                 reg = <0x036000 0x130>,
> >>                                         <0x036200 0x200>,
> >>                                         <0x036400 0x1dc>;
> >>                                 #phy-cells = <0>;
> >>
> >>                                 clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
> >>                                 clock-names = "pipe1";
> >>                                 resets = <&gcc GCC_PCIE_1_PHY_BCR>;
> >>                                 reset-names = "lane1";
> >>                         };
> >>
> >>                         pciephy_p2: port@2 {
> >>                                 reg = <0x037000 0x130>,
> >>                                         <0x037200 0x200>,
> >>                                         <0x037400 0x1dc>;
> >>                                 #phy-cells = <0>;
> >>
> >>                                 clocks = <&gcc GCC_PCIE_2_PIPE_CLK>;
> >>                                 clock-names = "pipe2";
> >>                                 resets = <&gcc GCC_PCIE_2_PHY_BCR>;
> >>                                 reset-names = "lane2";
> >>                         };
> >>                 };
> >>--------------------
> >>
> >>let me know if this looks okay.
> >>
> >>
> >What's the plan for non-pcie qmp phy binding? In that case we
> >don't have ports, so it gets folded into one node?
> >
> The non-pcie qmp phys still have one lane, that provides tx/rx.
> 
> I am of the opinion that we don't have two different ways to create
> phys in the driver, and keep one port/lane for such phys in dt.
> 

Ok so we would still have a subnode in that case. Sounds ok.
Vivek Gautam March 8, 2017, 6:45 a.m. UTC | #15
On 03/07/2017 07:30 PM, Stephen Boyd wrote:
> (Not sure I replied so here it is)
>
> On 01/27, Vivek Gautam wrote:
>>
>> On 01/27/2017 05:13 AM, Stephen Boyd wrote:
>>> On 01/24, Vivek Gautam wrote:
>>  From "./Documentation/devicetree/bindings/graph.txt" -
>> "The device tree graph bindings described herein abstract more complex
>> devices that can have multiple specifiable ports, each of which can be
>> linked to one or more ports of other devices."
>>
>> So, this means we use 'port', 'ports' and 'endpoint' for devices whose one
>> or more ports is connected to other device's one or more ports.
>>
>> I can use 'lane' for the node name here.
> Ok.
>
>>>>                                  reg = <0x035000 0x130>,
>>>>                                          <0x035200 0x200>,
>>>>                                          <0x035400 0x1dc>;
>>>>                                  #phy-cells = <0>;
>>>>
>>>>                                  clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
>>>>                                  clock-names = "pipe0";
>>>>                                  resets = <&gcc GCC_PCIE_0_PHY_BCR>;
>>>>                                  reset-names = "lane0";
>>>>                          };
>>>>
>>>>                        pciephy_p1: port@1 {
>>>>                                  reg = <0x036000 0x130>,
>>>>                                          <0x036200 0x200>,
>>>>                                          <0x036400 0x1dc>;
>>>>                                  #phy-cells = <0>;
>>>>
>>>>                                  clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
>>>>                                  clock-names = "pipe1";
>>>>                                  resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>>>>                                  reset-names = "lane1";
>>>>                          };
>>>>
>>>>                          pciephy_p2: port@2 {
>>>>                                  reg = <0x037000 0x130>,
>>>>                                          <0x037200 0x200>,
>>>>                                          <0x037400 0x1dc>;
>>>>                                  #phy-cells = <0>;
>>>>
>>>>                                  clocks = <&gcc GCC_PCIE_2_PIPE_CLK>;
>>>>                                  clock-names = "pipe2";
>>>>                                  resets = <&gcc GCC_PCIE_2_PHY_BCR>;
>>>>                                  reset-names = "lane2";
>>>>                          };
>>>>                  };
>>>> --------------------
>>>>
>>>> let me know if this looks okay.
>>>>
>>>>
>>> What's the plan for non-pcie qmp phy binding? In that case we
>>> don't have ports, so it gets folded into one node?
>>>
>> The non-pcie qmp phys still have one lane, that provides tx/rx.
>>
>> I am of the opinion that we don't have two different ways to create
>> phys in the driver, and keep one port/lane for such phys in dt.
>>
> Ok so we would still have a subnode in that case. Sounds ok.

Cool.

Thanks
Vivek
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
new file mode 100644
index 000000000000..6f510fe48f46
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -0,0 +1,76 @@ 
+Qualcomm QMP PHY controller
+===========================
+
+QMP phy controller supports physical layer functionality for a number of
+controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
+
+Required properties:
+ - compatible: compatible list, contains:
+	       "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
+	       "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
+ - reg: list of offset and length pair of the PHY register sets.
+	at index 0: offset and length of register set for PHY common
+		    serdes block.
+	from index 1 - N: offset and length of register set for each lane,
+			  for N number of phy lanes (ports).
+ - lane-offsets: array of offsets to tx, rx and pcs blocks for phy lanes.
+ - #phy-cells: must be 1
+    - Cell after phy phandle should be the port (lane) number.
+ - #clock-cells: must be 1
+    - Phy pll outputs a bunch of clocks for Tx, Rx and Pipe
+      interface (for pipe based PHYs). These clock are then gate-controlled
+      by gcc.
+ - clocks: a list of phandles and clock-specifier pairs,
+	   one for each entry in clock-names.
+ - clock-names: must be "cfg_ahb" for phy config clock,
+			"aux" for phy aux clock,
+			"ref" for 19.2 MHz ref clk,
+			"pipe<port-number>" for pipe clock specific to
+			each port/lane (Optional).
+ - resets: a list of phandles and reset controller specifier pairs,
+	   one for each entry in reset-names.
+ - reset-names: must be "phy" for reset of phy block,
+			"common" for phy common block reset,
+			"cfg" for phy's ahb cfg block reset (Optional).
+			"port<port-number>" for reset specific to
+			each port/lane (Optional).
+ - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
+ - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
+
+Optional properties:
+ - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
+			pll block.
+
+Example:
+	pcie_phy: phy@34000 {
+		compatible = "qcom,msm8996-qmp-pcie-phy";
+		reg = <0x034000 0x48f>,
+			<0x035000 0x5bf>,
+			<0x036000 0x5bf>,
+			<0x037000 0x5bf>;
+				/* tx, rx, pcs */
+		lane-offsets = <0x0 0x200 0x400>;
+		#phy-cells = <1>;
+		#clock-cells = <1>;
+
+		clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
+			<&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
+			<&gcc GCC_PCIE_CLKREF_CLK>,
+			<&gcc GCC_PCIE_0_PIPE_CLK>,
+			<&gcc GCC_PCIE_1_PIPE_CLK>,
+			<&gcc GCC_PCIE_2_PIPE_CLK>;
+		clock-names = "aux", "cfg_ahb", "ref",
+				"pipe0", "pipe1", "pipe2";
+
+		vdda-phy-supply = <&pm8994_l28>;
+		vdda-pll-supply = <&pm8994_l12>;
+
+		resets = <&gcc GCC_PCIE_PHY_BCR>,
+			<&gcc GCC_PCIE_PHY_COM_BCR>,
+			<&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>,
+			<&gcc GCC_PCIE_0_PHY_BCR>,
+			<&gcc GCC_PCIE_1_PHY_BCR>,
+			<&gcc GCC_PCIE_2_PHY_BCR>;
+		reset-names = "phy", "common", "cfg",
+				"lane0", "lane1", "lane2";
+	};