[PATCH 0/3] PHY: Add support for SERDES in TI's AM654 platform

[PATCH 0/3] PHY: Add support for SERDES in TI's AM654 platform

[Kishon Vijay Abraham I]

Patch series adds support for SERDES module in am654. This also
modifies phy_reset API to invoke pm_runtime_get/pm_runtime_put since
the reset callback can access registers.

Kishon Vijay Abraham I (3):
  phy: core: Invoke pm_runtime_get_*/pm_runtime_put_* before invoking
    reset callback
  dt-bindings: phy: ti: Add dt binding documentation for SERDES in
    AM654x SoC
  phy: ti: Add a new SERDES driver for TI's AM654x SoC

 .../devicetree/bindings/phy/ti-phy.txt        |  77 +++
 drivers/phy/phy-core.c                        |   6 +
 drivers/phy/ti/Kconfig                        |  11 +
 drivers/phy/ti/Makefile                       |   1 +
 drivers/phy/ti/phy-am654-serdes.c             | 528 ++++++++++++++++++
 include/dt-bindings/phy/phy-am654-serdes.h    |  13 +
 6 files changed, 636 insertions(+)
 create mode 100644 drivers/phy/ti/phy-am654-serdes.c
 create mode 100644 include/dt-bindings/phy/phy-am654-serdes.h

-- 
2.17.1

[PATCH 1/3] phy: core: Invoke pm_runtime_get_*/pm_runtime_put_* before invoking reset callback

[Kishon Vijay Abraham I]

PHY drivers may try to access PHY registers in the ->reset() callback.
Invoke phy_pm_runtime_get_sync() before invoking the ->reset() callback
so that the PHY drivers don't have to enable clocks by themselves before
accessing PHY registers.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/phy-core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 19b05e824ee4..bf3a1fc18117 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -384,10 +384,16 @@ int phy_reset(struct phy *phy)
 	if (!phy || !phy->ops->reset)
 		return 0;
 
+	ret = phy_pm_runtime_get_sync(phy);
+	if (ret < 0 && ret != -ENOTSUPP)
+		return ret;
+
 	mutex_lock(&phy->mutex);
 	ret = phy->ops->reset(phy);
 	mutex_unlock(&phy->mutex);
 
+	phy_pm_runtime_put(phy);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(phy_reset);
-- 
2.17.1

[PATCH 2/3] dt-bindings: phy: ti: Add dt binding documentation for SERDES in AM654x SoC

[Kishon Vijay Abraham I]

AM654x has two SERDES instances. Each instance has three input clocks
(left input, externel reference clock and right input) and two output
clocks (left output and right output) in addition to a PLL mux clock
which the SERDES uses for Clock Multiplier Unit (CMU refclock).
The PLL mux clock can select from one of the three input clocks.
The right output can select between left input and external reference
clock while the left output can select between the right input and
external reference clock.

The left and right input reference clock of SERDES0 and SERDES1
respectively are connected to the SoC clock. In the case of two lane
SERDES personality card, the left input of SERDES1 is connected to
the right output of SERDES0 in a chained fashion.

See section "Reference Clock Distribution" of AM65x Sitara Processors
TRM (SPRUID7 – April 2018) for more details.

Add dt-binding documentation in order to represent all these different
configurations in device tree.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../devicetree/bindings/phy/ti-phy.txt        | 77 +++++++++++++++++++
 include/dt-bindings/phy/phy-am654-serdes.h    | 13 ++++
 2 files changed, 90 insertions(+)
 create mode 100644 include/dt-bindings/phy/phy-am654-serdes.h

diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
index 57dfda8a7a1d..fc2fff6b2c37 100644
--- a/Documentation/devicetree/bindings/phy/ti-phy.txt
+++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
@@ -132,3 +132,80 @@ sata_phy: phy@4a096000 {
 	syscon-pllreset = <&scm_conf 0x3fc>;
 	#phy-cells = <0>;
 };
+
+
+TI AM654 SERDES
+
+Required properties:
+ - compatible: Should be "ti,phy-am654-serdes"
+ - reg : Address and length of the register set for the device.
+ - reg-names: Should be "serdes" which corresponds to the register space
+	populated in "reg".
+ - #phy-cells: determine the number of cells that should be given in the
+	phandle while referencing this phy. Should be "2". The 1st cell
+	corresponds to the phy type (should be one of the types specified in
+	include/dt-bindings/phy/phy.h) and the 2nd cell should be the serdes
+	lane function.
+	If SERDES0 is referenced 2nd cell should be:
+		0 - USB3
+		1 - PCIe0 Lane0
+		2 - ICSS2 SGMII Lane0
+	If SERDES1 is referenced 2nd cell should be:
+		0 - PCIe1 Lane0
+		1 - PCIe0 Lane1
+		2 - ICSS2 SGMII Lane1
+ - clocks: List of clock-specifiers representing the input to the SERDES.
+	Should have 3 items representing the left input clock, external
+	reference clock and right input clock in that order.
+ - clock-output-names: List of clock names for each of the clock outputs of
+	SERDES. Should have 3 items for CMU reference clock,
+	left output clock and right output clock in that order.
+ - assigned-clocks: As defined in
+	Documentation/devicetree/bindings/clock/clock-bindings.txt
+ - assigned-clock-parents: As defined in
+	Documentation/devicetree/bindings/clock/clock-bindings.txt
+ - #clock-cells: Should be <1> to choose between the 3 output clocks.
+	Defined in Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+   The following macros are defined in dt-bindings/phy/phy-am654-serdes.h
+   for selecting the correct reference clock. This can be used while
+   specifying the clocks created by SERDES.
+	=> AM654_SERDES_CMU_REFCLK
+	=> AM654_SERDES_LO_REFCLK
+	=> AM654_SERDES_RO_REFCLK
+
+ - mux-controls: phandle to the multiplexer
+
+Example:
+
+Example for SERDES0 is given below. It has 3 clock inputs;
+left input reference clock as indicated by <&k3_clks 153 4>, external
+reference clock as indicated by <&k3_clks 153 1> and right input
+reference clock as indicated by <&serdes1 AM654_SERDES_LO_REFCLK>. (The
+right input of SERDES0 is connected to the left output of SERDES1).
+
+SERDES0 registers 3 clock outputs as indicated in clock-output-names. The
+first refers to the CMU reference clock, second refers to the left output
+reference clock and the third refers to the right output reference clock.
+
+The assigned-clocks and assigned-clock-parents is used here to set the
+parent of left input reference clock to MAINHSDIV_CLKOUT4 and parent of
+CMU reference clock to left input reference clock.
+
+serdes0: serdes@900000 {
+	compatible = "ti,phy-am654-serdes";
+	reg = <0x0 0x900000 0x0 0x2000>;
+	reg-names = "serdes";
+	#phy-cells = <2>;
+	power-domains = <&k3_pds 153>;
+	clocks = <&k3_clks 153 4>, <&k3_clks 153 1>,
+			<&serdes1 AM654_SERDES_LO_REFCLK>;
+	clock-output-names = "serdes0_cmu_refclk", "serdes0_lo_refclk",
+				"serdes0_ro_refclk";
+	assigned-clocks = <&k3_clks 153 4>, <&serdes0 AM654_SERDES_CMU_REFCLK>;
+	assigned-clock-parents = <&k3_clks 153 8>, <&k3_clks 153 4>;
+	ti,serdes-clk = <&serdes0_clk>;
+	mux-controls = <&serdes_mux 0>;
+	#clock-cells = <1>;
+	status = "disabled";
+};
diff --git a/include/dt-bindings/phy/phy-am654-serdes.h b/include/dt-bindings/phy/phy-am654-serdes.h
new file mode 100644
index 000000000000..e8d901729ed9
--- /dev/null
+++ b/include/dt-bindings/phy/phy-am654-serdes.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides constants for AM654 SERDES.
+ */
+
+#ifndef _DT_BINDINGS_AM654_SERDES
+#define _DT_BINDINGS_AM654_SERDES
+
+#define AM654_SERDES_CMU_REFCLK	0
+#define AM654_SERDES_LO_REFCLK	1
+#define AM654_SERDES_RO_REFCLK	2
+
+#endif /* _DT_BINDINGS_AM654_SERDES */
-- 
2.17.1

Re: [PATCH 2/3] dt-bindings: phy: ti: Add dt binding documentation for SERDES in AM654x SoC

[Roger Quadros]

Kishon,

On 21/01/19 08:48, Kishon Vijay Abraham I wrote:
> AM654x has two SERDES instances. Each instance has three input clocks
> (left input, externel reference clock and right input) and two output
> clocks (left output and right output) in addition to a PLL mux clock
> which the SERDES uses for Clock Multiplier Unit (CMU refclock).
> The PLL mux clock can select from one of the three input clocks.
> The right output can select between left input and external reference
> clock while the left output can select between the right input and
> external reference clock.
> 
> The left and right input reference clock of SERDES0 and SERDES1
> respectively are connected to the SoC clock. In the case of two lane
> SERDES personality card, the left input of SERDES1 is connected to
> the right output of SERDES0 in a chained fashion.
> 
> See section "Reference Clock Distribution" of AM65x Sitara Processors
> TRM (SPRUID7 – April 2018) for more details.
> 
> Add dt-binding documentation in order to represent all these different
> configurations in device tree.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../devicetree/bindings/phy/ti-phy.txt        | 77 +++++++++++++++++++
>  include/dt-bindings/phy/phy-am654-serdes.h    | 13 ++++
>  2 files changed, 90 insertions(+)
>  create mode 100644 include/dt-bindings/phy/phy-am654-serdes.h
> 
> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
> index 57dfda8a7a1d..fc2fff6b2c37 100644
> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> @@ -132,3 +132,80 @@ sata_phy: phy@4a096000 {
>  	syscon-pllreset = <&scm_conf 0x3fc>;
>  	#phy-cells = <0>;
>  };
> +
> +
> +TI AM654 SERDES
> +
> +Required properties:
> + - compatible: Should be "ti,phy-am654-serdes"
> + - reg : Address and length of the register set for the device.
> + - reg-names: Should be "serdes" which corresponds to the register space
> +	populated in "reg".
> + - #phy-cells: determine the number of cells that should be given in the
> +	phandle while referencing this phy. Should be "2". The 1st cell
> +	corresponds to the phy type (should be one of the types specified in
> +	include/dt-bindings/phy/phy.h) and the 2nd cell should be the serdes
> +	lane function.
> +	If SERDES0 is referenced 2nd cell should be:
> +		0 - USB3
> +		1 - PCIe0 Lane0
> +		2 - ICSS2 SGMII Lane0
> +	If SERDES1 is referenced 2nd cell should be:
> +		0 - PCIe1 Lane0
> +		1 - PCIe0 Lane1
> +		2 - ICSS2 SGMII Lane1

Can we have a way to change default lane at probe time without having any user dependencies.

e.g. To work in USB2.0 mode I don't want SERDES0 to be in lane 0 (which is SoC default).
But at the same time the application might not be using PCIe or SGMII, so there is no
PHY user to change the lane to 1 or 2.

A DT property to allow selection of a default lane at probe time would help.


> + - clocks: List of clock-specifiers representing the input to the SERDES.
> +	Should have 3 items representing the left input clock, external
> +	reference clock and right input clock in that order.
> + - clock-output-names: List of clock names for each of the clock outputs of
> +	SERDES. Should have 3 items for CMU reference clock,
> +	left output clock and right output clock in that order.
> + - assigned-clocks: As defined in
> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
> + - assigned-clock-parents: As defined in
> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
> + - #clock-cells: Should be <1> to choose between the 3 output clocks.
> +	Defined in Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +   The following macros are defined in dt-bindings/phy/phy-am654-serdes.h
> +   for selecting the correct reference clock. This can be used while
> +   specifying the clocks created by SERDES.
> +	=> AM654_SERDES_CMU_REFCLK
> +	=> AM654_SERDES_LO_REFCLK
> +	=> AM654_SERDES_RO_REFCLK
> +
> + - mux-controls: phandle to the multiplexer
> +
> +Example:
> +
> +Example for SERDES0 is given below. It has 3 clock inputs;
> +left input reference clock as indicated by <&k3_clks 153 4>, external
> +reference clock as indicated by <&k3_clks 153 1> and right input
> +reference clock as indicated by <&serdes1 AM654_SERDES_LO_REFCLK>. (The
> +right input of SERDES0 is connected to the left output of SERDES1).
> +
> +SERDES0 registers 3 clock outputs as indicated in clock-output-names. The
> +first refers to the CMU reference clock, second refers to the left output
> +reference clock and the third refers to the right output reference clock.
> +
> +The assigned-clocks and assigned-clock-parents is used here to set the
> +parent of left input reference clock to MAINHSDIV_CLKOUT4 and parent of
> +CMU reference clock to left input reference clock.
> +
> +serdes0: serdes@900000 {
> +	compatible = "ti,phy-am654-serdes";
> +	reg = <0x0 0x900000 0x0 0x2000>;
> +	reg-names = "serdes";
> +	#phy-cells = <2>;
> +	power-domains = <&k3_pds 153>;
> +	clocks = <&k3_clks 153 4>, <&k3_clks 153 1>,
> +			<&serdes1 AM654_SERDES_LO_REFCLK>;
> +	clock-output-names = "serdes0_cmu_refclk", "serdes0_lo_refclk",
> +				"serdes0_ro_refclk";
> +	assigned-clocks = <&k3_clks 153 4>, <&serdes0 AM654_SERDES_CMU_REFCLK>;
> +	assigned-clock-parents = <&k3_clks 153 8>, <&k3_clks 153 4>;
> +	ti,serdes-clk = <&serdes0_clk>;
> +	mux-controls = <&serdes_mux 0>;
> +	#clock-cells = <1>;
> +	status = "disabled";

We don't keep status "disabled" for AM6. All nodes are enabled by default.

> +};
> diff --git a/include/dt-bindings/phy/phy-am654-serdes.h b/include/dt-bindings/phy/phy-am654-serdes.h
> new file mode 100644
> index 000000000000..e8d901729ed9
> --- /dev/null
> +++ b/include/dt-bindings/phy/phy-am654-serdes.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides constants for AM654 SERDES.
> + */
> +
> +#ifndef _DT_BINDINGS_AM654_SERDES
> +#define _DT_BINDINGS_AM654_SERDES
> +
> +#define AM654_SERDES_CMU_REFCLK	0
> +#define AM654_SERDES_LO_REFCLK	1
> +#define AM654_SERDES_RO_REFCLK	2
> +
> +#endif /* _DT_BINDINGS_AM654_SERDES */
> 

cheers,
-roger

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] dt-bindings: phy: ti: Add dt binding documentation for SERDES in AM654x SoC

[Kishon Vijay Abraham I]

Hi Roger,

On 21/01/19 3:25 PM, Roger Quadros wrote:
> Kishon,
> 
> On 21/01/19 08:48, Kishon Vijay Abraham I wrote:
>> AM654x has two SERDES instances. Each instance has three input clocks
>> (left input, externel reference clock and right input) and two output
>> clocks (left output and right output) in addition to a PLL mux clock
>> which the SERDES uses for Clock Multiplier Unit (CMU refclock).
>> The PLL mux clock can select from one of the three input clocks.
>> The right output can select between left input and external reference
>> clock while the left output can select between the right input and
>> external reference clock.
>>
>> The left and right input reference clock of SERDES0 and SERDES1
>> respectively are connected to the SoC clock. In the case of two lane
>> SERDES personality card, the left input of SERDES1 is connected to
>> the right output of SERDES0 in a chained fashion.
>>
>> See section "Reference Clock Distribution" of AM65x Sitara Processors
>> TRM (SPRUID7 – April 2018) for more details.
>>
>> Add dt-binding documentation in order to represent all these different
>> configurations in device tree.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../devicetree/bindings/phy/ti-phy.txt        | 77 +++++++++++++++++++
>>  include/dt-bindings/phy/phy-am654-serdes.h    | 13 ++++
>>  2 files changed, 90 insertions(+)
>>  create mode 100644 include/dt-bindings/phy/phy-am654-serdes.h
>>
>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>> index 57dfda8a7a1d..fc2fff6b2c37 100644
>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>> @@ -132,3 +132,80 @@ sata_phy: phy@4a096000 {
>>  	syscon-pllreset = <&scm_conf 0x3fc>;
>>  	#phy-cells = <0>;
>>  };
>> +
>> +
>> +TI AM654 SERDES
>> +
>> +Required properties:
>> + - compatible: Should be "ti,phy-am654-serdes"
>> + - reg : Address and length of the register set for the device.
>> + - reg-names: Should be "serdes" which corresponds to the register space
>> +	populated in "reg".
>> + - #phy-cells: determine the number of cells that should be given in the
>> +	phandle while referencing this phy. Should be "2". The 1st cell
>> +	corresponds to the phy type (should be one of the types specified in
>> +	include/dt-bindings/phy/phy.h) and the 2nd cell should be the serdes
>> +	lane function.
>> +	If SERDES0 is referenced 2nd cell should be:
>> +		0 - USB3
>> +		1 - PCIe0 Lane0
>> +		2 - ICSS2 SGMII Lane0
>> +	If SERDES1 is referenced 2nd cell should be:
>> +		0 - PCIe1 Lane0
>> +		1 - PCIe0 Lane1
>> +		2 - ICSS2 SGMII Lane1
> 
> Can we have a way to change default lane at probe time without having any user dependencies.
> 
> e.g. To work in USB2.0 mode I don't want SERDES0 to be in lane 0 (which is SoC default).
> But at the same time the application might not be using PCIe or SGMII, so there is no
> PHY user to change the lane to 1 or 2.
> 
> A DT property to allow selection of a default lane at probe time would help
Ideally we should be disabling the module that is not used ("status" property
of SERDES0 dt would be "disabled"). So there is no guarantee SERDES will be probed.


> 
> 
>> + - clocks: List of clock-specifiers representing the input to the SERDES.
>> +	Should have 3 items representing the left input clock, external
>> +	reference clock and right input clock in that order.
>> + - clock-output-names: List of clock names for each of the clock outputs of
>> +	SERDES. Should have 3 items for CMU reference clock,
>> +	left output clock and right output clock in that order.
>> + - assigned-clocks: As defined in
>> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
>> + - assigned-clock-parents: As defined in
>> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
>> + - #clock-cells: Should be <1> to choose between the 3 output clocks.
>> +	Defined in Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +   The following macros are defined in dt-bindings/phy/phy-am654-serdes.h
>> +   for selecting the correct reference clock. This can be used while
>> +   specifying the clocks created by SERDES.
>> +	=> AM654_SERDES_CMU_REFCLK
>> +	=> AM654_SERDES_LO_REFCLK
>> +	=> AM654_SERDES_RO_REFCLK
>> +
>> + - mux-controls: phandle to the multiplexer
>> +
>> +Example:
>> +
>> +Example for SERDES0 is given below. It has 3 clock inputs;
>> +left input reference clock as indicated by <&k3_clks 153 4>, external
>> +reference clock as indicated by <&k3_clks 153 1> and right input
>> +reference clock as indicated by <&serdes1 AM654_SERDES_LO_REFCLK>. (The
>> +right input of SERDES0 is connected to the left output of SERDES1).
>> +
>> +SERDES0 registers 3 clock outputs as indicated in clock-output-names. The
>> +first refers to the CMU reference clock, second refers to the left output
>> +reference clock and the third refers to the right output reference clock.
>> +
>> +The assigned-clocks and assigned-clock-parents is used here to set the
>> +parent of left input reference clock to MAINHSDIV_CLKOUT4 and parent of
>> +CMU reference clock to left input reference clock.
>> +
>> +serdes0: serdes@900000 {
>> +	compatible = "ti,phy-am654-serdes";
>> +	reg = <0x0 0x900000 0x0 0x2000>;
>> +	reg-names = "serdes";
>> +	#phy-cells = <2>;
>> +	power-domains = <&k3_pds 153>;
>> +	clocks = <&k3_clks 153 4>, <&k3_clks 153 1>,
>> +			<&serdes1 AM654_SERDES_LO_REFCLK>;
>> +	clock-output-names = "serdes0_cmu_refclk", "serdes0_lo_refclk",
>> +				"serdes0_ro_refclk";
>> +	assigned-clocks = <&k3_clks 153 4>, <&serdes0 AM654_SERDES_CMU_REFCLK>;
>> +	assigned-clock-parents = <&k3_clks 153 8>, <&k3_clks 153 4>;
>> +	ti,serdes-clk = <&serdes0_clk>;
>> +	mux-controls = <&serdes_mux 0>;
>> +	#clock-cells = <1>;
>> +	status = "disabled";
> 
> We don't keep status "disabled" for AM6. All nodes are enabled by default.

Should all nodes be enabled by default? Is it not based on the features
supported by the EVM?

Thanks
Kishon

Re: [PATCH 2/3] dt-bindings: phy: ti: Add dt binding documentation for SERDES in AM654x SoC

[Roger Quadros]

On 21/01/19 12:47, Kishon Vijay Abraham I wrote:
> Hi Roger,
> 
> On 21/01/19 3:25 PM, Roger Quadros wrote:
>> Kishon,
>>
>> On 21/01/19 08:48, Kishon Vijay Abraham I wrote:
>>> AM654x has two SERDES instances. Each instance has three input clocks
>>> (left input, externel reference clock and right input) and two output
>>> clocks (left output and right output) in addition to a PLL mux clock
>>> which the SERDES uses for Clock Multiplier Unit (CMU refclock).
>>> The PLL mux clock can select from one of the three input clocks.
>>> The right output can select between left input and external reference
>>> clock while the left output can select between the right input and
>>> external reference clock.
>>>
>>> The left and right input reference clock of SERDES0 and SERDES1
>>> respectively are connected to the SoC clock. In the case of two lane
>>> SERDES personality card, the left input of SERDES1 is connected to
>>> the right output of SERDES0 in a chained fashion.
>>>
>>> See section "Reference Clock Distribution" of AM65x Sitara Processors
>>> TRM (SPRUID7 – April 2018) for more details.
>>>
>>> Add dt-binding documentation in order to represent all these different
>>> configurations in device tree.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  .../devicetree/bindings/phy/ti-phy.txt        | 77 +++++++++++++++++++
>>>  include/dt-bindings/phy/phy-am654-serdes.h    | 13 ++++
>>>  2 files changed, 90 insertions(+)
>>>  create mode 100644 include/dt-bindings/phy/phy-am654-serdes.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>> index 57dfda8a7a1d..fc2fff6b2c37 100644
>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>> @@ -132,3 +132,80 @@ sata_phy: phy@4a096000 {
>>>  	syscon-pllreset = <&scm_conf 0x3fc>;
>>>  	#phy-cells = <0>;
>>>  };
>>> +
>>> +
>>> +TI AM654 SERDES
>>> +
>>> +Required properties:
>>> + - compatible: Should be "ti,phy-am654-serdes"
>>> + - reg : Address and length of the register set for the device.
>>> + - reg-names: Should be "serdes" which corresponds to the register space
>>> +	populated in "reg".
>>> + - #phy-cells: determine the number of cells that should be given in the
>>> +	phandle while referencing this phy. Should be "2". The 1st cell
>>> +	corresponds to the phy type (should be one of the types specified in
>>> +	include/dt-bindings/phy/phy.h) and the 2nd cell should be the serdes
>>> +	lane function.
>>> +	If SERDES0 is referenced 2nd cell should be:
>>> +		0 - USB3
>>> +		1 - PCIe0 Lane0
>>> +		2 - ICSS2 SGMII Lane0
>>> +	If SERDES1 is referenced 2nd cell should be:
>>> +		0 - PCIe1 Lane0
>>> +		1 - PCIe0 Lane1
>>> +		2 - ICSS2 SGMII Lane1
>>
>> Can we have a way to change default lane at probe time without having any user dependencies.
>>
>> e.g. To work in USB2.0 mode I don't want SERDES0 to be in lane 0 (which is SoC default).
>> But at the same time the application might not be using PCIe or SGMII, so there is no
>> PHY user to change the lane to 1 or 2.
>>
>> A DT property to allow selection of a default lane at probe time would help
> Ideally we should be disabling the module that is not used ("status" property
> of SERDES0 dt would be "disabled"). So there is no guarantee SERDES will be probed.
> 

OK. Is there something that can be done at the serdes_mux?
As per bindings/mux/mmio-mux.txt

Optional properties:
- idle-states : if present, the state the muxes will have when idle. The
                special state MUX_IDLE_AS_IS is the default.

Could that be used?

> 
>>
>>
>>> + - clocks: List of clock-specifiers representing the input to the SERDES.
>>> +	Should have 3 items representing the left input clock, external
>>> +	reference clock and right input clock in that order.
>>> + - clock-output-names: List of clock names for each of the clock outputs of
>>> +	SERDES. Should have 3 items for CMU reference clock,
>>> +	left output clock and right output clock in that order.
>>> + - assigned-clocks: As defined in
>>> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> + - assigned-clock-parents: As defined in
>>> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> + - #clock-cells: Should be <1> to choose between the 3 output clocks.
>>> +	Defined in Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> +
>>> +   The following macros are defined in dt-bindings/phy/phy-am654-serdes.h
>>> +   for selecting the correct reference clock. This can be used while
>>> +   specifying the clocks created by SERDES.
>>> +	=> AM654_SERDES_CMU_REFCLK
>>> +	=> AM654_SERDES_LO_REFCLK
>>> +	=> AM654_SERDES_RO_REFCLK
>>> +
>>> + - mux-controls: phandle to the multiplexer
>>> +
>>> +Example:
>>> +
>>> +Example for SERDES0 is given below. It has 3 clock inputs;
>>> +left input reference clock as indicated by <&k3_clks 153 4>, external
>>> +reference clock as indicated by <&k3_clks 153 1> and right input
>>> +reference clock as indicated by <&serdes1 AM654_SERDES_LO_REFCLK>. (The
>>> +right input of SERDES0 is connected to the left output of SERDES1).
>>> +
>>> +SERDES0 registers 3 clock outputs as indicated in clock-output-names. The
>>> +first refers to the CMU reference clock, second refers to the left output
>>> +reference clock and the third refers to the right output reference clock.
>>> +
>>> +The assigned-clocks and assigned-clock-parents is used here to set the
>>> +parent of left input reference clock to MAINHSDIV_CLKOUT4 and parent of
>>> +CMU reference clock to left input reference clock.
>>> +
>>> +serdes0: serdes@900000 {
>>> +	compatible = "ti,phy-am654-serdes";
>>> +	reg = <0x0 0x900000 0x0 0x2000>;
>>> +	reg-names = "serdes";
>>> +	#phy-cells = <2>;
>>> +	power-domains = <&k3_pds 153>;
>>> +	clocks = <&k3_clks 153 4>, <&k3_clks 153 1>,
>>> +			<&serdes1 AM654_SERDES_LO_REFCLK>;
>>> +	clock-output-names = "serdes0_cmu_refclk", "serdes0_lo_refclk",
>>> +				"serdes0_ro_refclk";
>>> +	assigned-clocks = <&k3_clks 153 4>, <&serdes0 AM654_SERDES_CMU_REFCLK>;
>>> +	assigned-clock-parents = <&k3_clks 153 8>, <&k3_clks 153 4>;
>>> +	ti,serdes-clk = <&serdes0_clk>;
>>> +	mux-controls = <&serdes_mux 0>;
>>> +	#clock-cells = <1>;
>>> +	status = "disabled";
>>
>> We don't keep status "disabled" for AM6. All nodes are enabled by default.
> 
> Should all nodes be enabled by default? Is it not based on the features
> supported by the EVM?

Yes, enabled in the SoC. EVM is supposed to disable nodes that are not required.
See https://www.spinics.net/lists/arm-kernel/msg700105.html

cheers,
-roger
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] dt-bindings: phy: ti: Add dt binding documentation for SERDES in AM654x SoC

[Kishon Vijay Abraham I]

Hi,

On 21/01/19 4:52 PM, Roger Quadros wrote:
> 
> 
> On 21/01/19 12:47, Kishon Vijay Abraham I wrote:
>> Hi Roger,
>>
>> On 21/01/19 3:25 PM, Roger Quadros wrote:
>>> Kishon,
>>>
>>> On 21/01/19 08:48, Kishon Vijay Abraham I wrote:
>>>> AM654x has two SERDES instances. Each instance has three input clocks
>>>> (left input, externel reference clock and right input) and two output
>>>> clocks (left output and right output) in addition to a PLL mux clock
>>>> which the SERDES uses for Clock Multiplier Unit (CMU refclock).
>>>> The PLL mux clock can select from one of the three input clocks.
>>>> The right output can select between left input and external reference
>>>> clock while the left output can select between the right input and
>>>> external reference clock.
>>>>
>>>> The left and right input reference clock of SERDES0 and SERDES1
>>>> respectively are connected to the SoC clock. In the case of two lane
>>>> SERDES personality card, the left input of SERDES1 is connected to
>>>> the right output of SERDES0 in a chained fashion.
>>>>
>>>> See section "Reference Clock Distribution" of AM65x Sitara Processors
>>>> TRM (SPRUID7 – April 2018) for more details.
>>>>
>>>> Add dt-binding documentation in order to represent all these different
>>>> configurations in device tree.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  .../devicetree/bindings/phy/ti-phy.txt        | 77 +++++++++++++++++++
>>>>  include/dt-bindings/phy/phy-am654-serdes.h    | 13 ++++
>>>>  2 files changed, 90 insertions(+)
>>>>  create mode 100644 include/dt-bindings/phy/phy-am654-serdes.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>> index 57dfda8a7a1d..fc2fff6b2c37 100644
>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>> @@ -132,3 +132,80 @@ sata_phy: phy@4a096000 {
>>>>  	syscon-pllreset = <&scm_conf 0x3fc>;
>>>>  	#phy-cells = <0>;
>>>>  };
>>>> +
>>>> +
>>>> +TI AM654 SERDES
>>>> +
>>>> +Required properties:
>>>> + - compatible: Should be "ti,phy-am654-serdes"
>>>> + - reg : Address and length of the register set for the device.
>>>> + - reg-names: Should be "serdes" which corresponds to the register space
>>>> +	populated in "reg".
>>>> + - #phy-cells: determine the number of cells that should be given in the
>>>> +	phandle while referencing this phy. Should be "2". The 1st cell
>>>> +	corresponds to the phy type (should be one of the types specified in
>>>> +	include/dt-bindings/phy/phy.h) and the 2nd cell should be the serdes
>>>> +	lane function.
>>>> +	If SERDES0 is referenced 2nd cell should be:
>>>> +		0 - USB3
>>>> +		1 - PCIe0 Lane0
>>>> +		2 - ICSS2 SGMII Lane0
>>>> +	If SERDES1 is referenced 2nd cell should be:
>>>> +		0 - PCIe1 Lane0
>>>> +		1 - PCIe0 Lane1
>>>> +		2 - ICSS2 SGMII Lane1
>>>
>>> Can we have a way to change default lane at probe time without having any user dependencies.
>>>
>>> e.g. To work in USB2.0 mode I don't want SERDES0 to be in lane 0 (which is SoC default).
>>> But at the same time the application might not be using PCIe or SGMII, so there is no
>>> PHY user to change the lane to 1 or 2.
>>>
>>> A DT property to allow selection of a default lane at probe time would help
>> Ideally we should be disabling the module that is not used ("status" property
>> of SERDES0 dt would be "disabled"). So there is no guarantee SERDES will be probed.
>>
> 
> OK. Is there something that can be done at the serdes_mux?
> As per bindings/mux/mmio-mux.txt
> 
> Optional properties:
> - idle-states : if present, the state the muxes will have when idle. The
>                 special state MUX_IDLE_AS_IS is the default.
> 
> Could that be used?

Looks like that's something we could use.

Thanks
Kishon

Re: [PATCH 2/3] dt-bindings: phy: ti: Add dt binding documentation for SERDES in AM654x SoC

[Kishon Vijay Abraham I]

Rob,

On 21/01/19 12:18 PM, Kishon Vijay Abraham I wrote:
> AM654x has two SERDES instances. Each instance has three input clocks
> (left input, externel reference clock and right input) and two output
> clocks (left output and right output) in addition to a PLL mux clock
> which the SERDES uses for Clock Multiplier Unit (CMU refclock).
> The PLL mux clock can select from one of the three input clocks.
> The right output can select between left input and external reference
> clock while the left output can select between the right input and
> external reference clock.
> 
> The left and right input reference clock of SERDES0 and SERDES1
> respectively are connected to the SoC clock. In the case of two lane
> SERDES personality card, the left input of SERDES1 is connected to
> the right output of SERDES0 in a chained fashion.
> 
> See section "Reference Clock Distribution" of AM65x Sitara Processors
> TRM (SPRUID7 – April 2018) for more details.
> 
> Add dt-binding documentation in order to represent all these different
> configurations in device tree.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

Can you review this patch?

Thanks
Kishon

> ---
>  .../devicetree/bindings/phy/ti-phy.txt        | 77 +++++++++++++++++++
>  include/dt-bindings/phy/phy-am654-serdes.h    | 13 ++++
>  2 files changed, 90 insertions(+)
>  create mode 100644 include/dt-bindings/phy/phy-am654-serdes.h
> 
> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
> index 57dfda8a7a1d..fc2fff6b2c37 100644
> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> @@ -132,3 +132,80 @@ sata_phy: phy@4a096000 {
>  	syscon-pllreset = <&scm_conf 0x3fc>;
>  	#phy-cells = <0>;
>  };
> +
> +
> +TI AM654 SERDES
> +
> +Required properties:
> + - compatible: Should be "ti,phy-am654-serdes"
> + - reg : Address and length of the register set for the device.
> + - reg-names: Should be "serdes" which corresponds to the register space
> +	populated in "reg".
> + - #phy-cells: determine the number of cells that should be given in the
> +	phandle while referencing this phy. Should be "2". The 1st cell
> +	corresponds to the phy type (should be one of the types specified in
> +	include/dt-bindings/phy/phy.h) and the 2nd cell should be the serdes
> +	lane function.
> +	If SERDES0 is referenced 2nd cell should be:
> +		0 - USB3
> +		1 - PCIe0 Lane0
> +		2 - ICSS2 SGMII Lane0
> +	If SERDES1 is referenced 2nd cell should be:
> +		0 - PCIe1 Lane0
> +		1 - PCIe0 Lane1
> +		2 - ICSS2 SGMII Lane1
> + - clocks: List of clock-specifiers representing the input to the SERDES.
> +	Should have 3 items representing the left input clock, external
> +	reference clock and right input clock in that order.
> + - clock-output-names: List of clock names for each of the clock outputs of
> +	SERDES. Should have 3 items for CMU reference clock,
> +	left output clock and right output clock in that order.
> + - assigned-clocks: As defined in
> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
> + - assigned-clock-parents: As defined in
> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
> + - #clock-cells: Should be <1> to choose between the 3 output clocks.
> +	Defined in Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +   The following macros are defined in dt-bindings/phy/phy-am654-serdes.h
> +   for selecting the correct reference clock. This can be used while
> +   specifying the clocks created by SERDES.
> +	=> AM654_SERDES_CMU_REFCLK
> +	=> AM654_SERDES_LO_REFCLK
> +	=> AM654_SERDES_RO_REFCLK
> +
> + - mux-controls: phandle to the multiplexer
> +
> +Example:
> +
> +Example for SERDES0 is given below. It has 3 clock inputs;
> +left input reference clock as indicated by <&k3_clks 153 4>, external
> +reference clock as indicated by <&k3_clks 153 1> and right input
> +reference clock as indicated by <&serdes1 AM654_SERDES_LO_REFCLK>. (The
> +right input of SERDES0 is connected to the left output of SERDES1).
> +
> +SERDES0 registers 3 clock outputs as indicated in clock-output-names. The
> +first refers to the CMU reference clock, second refers to the left output
> +reference clock and the third refers to the right output reference clock.
> +
> +The assigned-clocks and assigned-clock-parents is used here to set the
> +parent of left input reference clock to MAINHSDIV_CLKOUT4 and parent of
> +CMU reference clock to left input reference clock.
> +
> +serdes0: serdes@900000 {
> +	compatible = "ti,phy-am654-serdes";
> +	reg = <0x0 0x900000 0x0 0x2000>;
> +	reg-names = "serdes";
> +	#phy-cells = <2>;
> +	power-domains = <&k3_pds 153>;
> +	clocks = <&k3_clks 153 4>, <&k3_clks 153 1>,
> +			<&serdes1 AM654_SERDES_LO_REFCLK>;
> +	clock-output-names = "serdes0_cmu_refclk", "serdes0_lo_refclk",
> +				"serdes0_ro_refclk";
> +	assigned-clocks = <&k3_clks 153 4>, <&serdes0 AM654_SERDES_CMU_REFCLK>;
> +	assigned-clock-parents = <&k3_clks 153 8>, <&k3_clks 153 4>;
> +	ti,serdes-clk = <&serdes0_clk>;
> +	mux-controls = <&serdes_mux 0>;
> +	#clock-cells = <1>;
> +	status = "disabled";
> +};
> diff --git a/include/dt-bindings/phy/phy-am654-serdes.h b/include/dt-bindings/phy/phy-am654-serdes.h
> new file mode 100644
> index 000000000000..e8d901729ed9
> --- /dev/null
> +++ b/include/dt-bindings/phy/phy-am654-serdes.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides constants for AM654 SERDES.
> + */
> +
> +#ifndef _DT_BINDINGS_AM654_SERDES
> +#define _DT_BINDINGS_AM654_SERDES
> +
> +#define AM654_SERDES_CMU_REFCLK	0
> +#define AM654_SERDES_LO_REFCLK	1
> +#define AM654_SERDES_RO_REFCLK	2
> +
> +#endif /* _DT_BINDINGS_AM654_SERDES */
> 

[PATCH 3/3] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Kishon Vijay Abraham I]

Add a new SERDES driver for TI's AM654x SoC which configures
the SERDES only for PCIe. Support fo USB3 will be added later.

SERDES in am654x has three input clocks (left input, externel reference
clock and right input) and two output clocks (left output and right
output) in addition to a PLL mux clock which the SERDES uses for Clock
Multiplier Unit (CMU refclock).

The PLL mux clock can select from one of the three input clocks.
The right output can select between left input and external reference
clock while the left output can select between the right input and
external reference clock.

The driver has support to select PLL mux and left/right output mux as
specified in device tree.

[rogerq@ti.com: Fix boot lockup caused by accessing a structure member
(hw->init) allocated in stack of probe() and accessed in get_parent]
[rogerq@ti.com: Fix "Failed to find the parent" warnings]
Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/ti/Kconfig            |  11 +
 drivers/phy/ti/Makefile           |   1 +
 drivers/phy/ti/phy-am654-serdes.c | 528 ++++++++++++++++++++++++++++++
 3 files changed, 540 insertions(+)
 create mode 100644 drivers/phy/ti/phy-am654-serdes.c

diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
index f137e0107764..6357c32de115 100644
--- a/drivers/phy/ti/Kconfig
+++ b/drivers/phy/ti/Kconfig
@@ -20,6 +20,17 @@ config PHY_DM816X_USB
 	help
 	  Enable this for dm816x USB to work.
 
+config PHY_AM654_SERDES
+	tristate "TI AM654 SERDES support"
+	depends on OF && ARCH_K3 || COMPILE_TEST
+	select GENERIC_PHY
+	select MULTIPLEXER
+	select REGMAP_MMIO
+	select MUX_MMIO
+	help
+	  This option enables support for TI AM654 SerDes PHY used for
+	  PCIe.
+
 config OMAP_CONTROL_PHY
 	tristate "OMAP CONTROL PHY Driver"
 	depends on ARCH_OMAP2PLUS || COMPILE_TEST
diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
index bea8f25a137a..bff901eb0ecc 100644
--- a/drivers/phy/ti/Makefile
+++ b/drivers/phy/ti/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
 obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
 obj-$(CONFIG_PHY_TUSB1210)		+= phy-tusb1210.o
 obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
+obj-$(CONFIG_PHY_AM654_SERDES)		+= phy-am654-serdes.o
 obj-$(CONFIG_PHY_TI_GMII_SEL)		+= phy-gmii-sel.o
diff --git a/drivers/phy/ti/phy-am654-serdes.c b/drivers/phy/ti/phy-am654-serdes.c
new file mode 100644
index 000000000000..fcc0a98fbbd3
--- /dev/null
+++ b/drivers/phy/ti/phy-am654-serdes.c
@@ -0,0 +1,528 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * PCIe SERDES driver for AM654x SoC
+ *
+ * Copyright (C) 2018 Texas Instruments
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ */
+
+#include <dt-bindings/phy/phy.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mux/consumer.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/mfd/syscon.h>
+
+#define CMU_R07C		0x7c
+#define CMU_MASTER_CDN_O	BIT(24)
+
+#define COMLANE_R138		0xb38
+#define CONFIG_VERSION_REG_MASK	GENMASK(23, 16)
+#define CONFIG_VERSION_REG_SHIFT 16
+#define VERSION			0x70
+
+#define COMLANE_R190		0xb90
+#define L1_MASTER_CDN_O		BIT(9)
+
+#define COMLANE_R194		0xb94
+#define CMU_OK_I_0		BIT(19)
+
+#define SERDES_CTRL		0x1fd0
+#define POR_EN			BIT(29)
+
+#define WIZ_LANEXCTL_STS	0x1fe0
+#define TX0_ENABLE_OVL		BIT(31)
+#define TX0_ENABLE_MASK		GENMASK(30, 29)
+#define TX0_ENABLE_SHIFT	29
+#define TX0_DISABLE_STATE	0x0
+#define TX0_SLEEP_STATE		0x1
+#define TX0_SNOOZE_STATE	0x2
+#define TX0_ENABLE_STATE	0x3
+#define RX0_ENABLE_OVL		BIT(15)
+#define RX0_ENABLE_MASK		GENMASK(14, 13)
+#define RX0_ENABLE_SHIFT	13
+#define RX0_DISABLE_STATE	0x0
+#define RX0_SLEEP_STATE		0x1
+#define RX0_SNOOZE_STATE	0x2
+#define RX0_ENABLE_STATE	0x3
+
+#define WIZ_PLL_CTRL		0x1ff4
+#define PLL_ENABLE_OVL		BIT(31)
+#define PLL_ENABLE_MASK		GENMASK(30, 29)
+#define PLL_ENABLE_SHIFT	29
+#define PLL_DISABLE_STATE	0x0
+#define PLL_SLEEP_STATE		0x1
+#define PLL_SNOOZE_STATE	0x2
+#define PLL_ENABLE_STATE	0x3
+#define PLL_OK			BIT(28)
+
+#define PLL_LOCK_TIME		100000	/* in microseconds */
+#define SLEEP_TIME		100	/* in microseconds */
+
+#define LANE_USB3		0x0
+#define LANE_PCIE0_LANE0	0x1
+
+#define LANE_PCIE1_LANE0	0x0
+#define LANE_PCIE0_LANE1	0x1
+
+#define SERDES_NUM_CLOCKS	3
+
+struct serdes_am654_clk_mux {
+	struct clk_hw	hw;
+	struct regmap	*regmap;
+	unsigned int	reg;
+	int		*table;
+	u32		mask;
+	u8		shift;
+	struct clk_init_data clk_data;
+};
+
+#define to_serdes_am654_clk_mux(_hw)	\
+		container_of(_hw, struct serdes_am654_clk_mux, hw)
+
+static struct regmap_config serdes_am654_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+struct serdes_am654 {
+	struct regmap		*regmap;
+	struct device		*dev;
+	struct mux_control	*control;
+	bool			busy;
+	u32			type;
+	struct device_node	*of_node;
+	struct clk_onecell_data	clk_data;
+	struct clk		*clks[SERDES_NUM_CLOCKS];
+};
+
+static int serdes_am654_enable_pll(struct serdes_am654 *phy)
+{
+	u32 mask = PLL_ENABLE_OVL | PLL_ENABLE_MASK;
+	u32 val = PLL_ENABLE_OVL | (PLL_ENABLE_STATE << PLL_ENABLE_SHIFT);
+
+	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, val);
+
+	return regmap_read_poll_timeout(phy->regmap, WIZ_PLL_CTRL, val,
+					val & PLL_OK, 1000, PLL_LOCK_TIME);
+}
+
+static void serdes_am654_disable_pll(struct serdes_am654 *phy)
+{
+	u32 mask = PLL_ENABLE_OVL | PLL_ENABLE_MASK;
+
+	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, 0);
+}
+
+static int serdes_am654_enable_txrx(struct serdes_am654 *phy)
+{
+	u32 mask;
+	u32 val;
+
+	/* Enable TX */
+	mask = TX0_ENABLE_OVL | TX0_ENABLE_MASK;
+	val = TX0_ENABLE_OVL | (TX0_ENABLE_STATE << TX0_ENABLE_SHIFT);
+	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, val);
+
+	/* Enable RX */
+	mask = RX0_ENABLE_OVL | RX0_ENABLE_MASK;
+	val = RX0_ENABLE_OVL | (RX0_ENABLE_STATE << RX0_ENABLE_SHIFT);
+	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, val);
+
+	return 0;
+}
+
+static int serdes_am654_disable_txrx(struct serdes_am654 *phy)
+{
+	u32 mask;
+
+	/* Disable TX */
+	mask = TX0_ENABLE_OVL | TX0_ENABLE_MASK;
+	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, 0);
+
+	/* Disable RX */
+	mask = RX0_ENABLE_OVL | RX0_ENABLE_MASK;
+	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, 0);
+
+	return 0;
+}
+
+static int serdes_am654_power_on(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+	struct device *dev = phy->dev;
+	int ret;
+	u32 val;
+
+	ret = serdes_am654_enable_pll(phy);
+	if (ret) {
+		dev_err(dev, "Failed to enable PLL\n");
+		return ret;
+	}
+
+	ret = serdes_am654_enable_txrx(phy);
+	if (ret) {
+		dev_err(dev, "Failed to enable TX RX\n");
+		return ret;
+	}
+
+	return regmap_read_poll_timeout(phy->regmap, COMLANE_R194, val,
+					val & CMU_OK_I_0, SLEEP_TIME,
+					PLL_LOCK_TIME);
+}
+
+static int serdes_am654_power_off(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+
+	serdes_am654_disable_txrx(phy);
+	serdes_am654_disable_pll(phy);
+
+	return 0;
+}
+
+static int serdes_am654_init(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+	u32 mask;
+	u32 val;
+
+	mask = CONFIG_VERSION_REG_MASK;
+	val = VERSION << CONFIG_VERSION_REG_SHIFT;
+	regmap_update_bits(phy->regmap, COMLANE_R138, mask, val);
+
+	val = CMU_MASTER_CDN_O;
+	regmap_update_bits(phy->regmap, CMU_R07C, val, val);
+
+	val = L1_MASTER_CDN_O;
+	regmap_update_bits(phy->regmap, COMLANE_R190, val, val);
+
+	return 0;
+}
+
+static int serdes_am654_reset(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+	u32 val;
+
+	val = POR_EN;
+	regmap_update_bits(phy->regmap, SERDES_CTRL, val, val);
+	mdelay(1);
+	regmap_update_bits(phy->regmap, SERDES_CTRL, val, 0);
+
+	return 0;
+}
+
+struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
+				 *args)
+{
+	struct serdes_am654 *am654_phy;
+	struct phy *phy;
+	int ret;
+
+	phy = of_phy_simple_xlate(dev, args);
+	if (IS_ERR(phy))
+		return phy;
+
+	am654_phy = phy_get_drvdata(phy);
+	if (am654_phy->type != args->args[0] && am654_phy->busy)
+		return ERR_PTR(-EBUSY);
+
+	ret = mux_control_select(am654_phy->control, args->args[1]);
+	if (ret) {
+		dev_err(dev, "Failed to select SERDES Lane Function\n");
+		return ERR_PTR(ret);
+	}
+
+	am654_phy->busy = true;
+	am654_phy->type = args->args[0];
+
+	return phy;
+}
+
+static const struct phy_ops ops = {
+	.reset		= serdes_am654_reset,
+	.init		= serdes_am654_init,
+	.power_on	= serdes_am654_power_on,
+	.power_off	= serdes_am654_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static u8 serdes_am654_clk_mux_get_parent(struct clk_hw *hw)
+{
+	struct serdes_am654_clk_mux *mux = to_serdes_am654_clk_mux(hw);
+	unsigned int num_parents = clk_hw_get_num_parents(hw);
+	struct regmap *regmap = mux->regmap;
+	unsigned int reg = mux->reg;
+	unsigned int val;
+	int i;
+
+	regmap_read(regmap, reg, &val);
+	val >>= mux->shift;
+	val &= mux->mask;
+
+	for (i = 0; i < num_parents; i++)
+		if (mux->table[i] == val)
+			return i;
+
+	/*
+	 * No parent? This should never happen!
+	 * Verify if we set a valid parent in serdes_am654_clk_register()
+	 */
+	WARN(1, "Failed to find the parent of %s clock\n", hw->init->name);
+
+	/* Make the parent lookup to fail */
+	return num_parents;
+}
+
+static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct serdes_am654_clk_mux *mux = to_serdes_am654_clk_mux(hw);
+	struct regmap *regmap = mux->regmap;
+	unsigned int reg = mux->reg;
+	int val;
+	int ret;
+
+	val = mux->table[index];
+
+	if (val == -1)
+		return -EINVAL;
+
+	val <<= mux->shift;
+	ret = regmap_update_bits(regmap, reg, mux->mask << mux->shift, val);
+
+	return ret;
+}
+
+static const struct clk_ops serdes_am654_clk_mux_ops = {
+	.set_parent = serdes_am654_clk_mux_set_parent,
+	.get_parent = serdes_am654_clk_mux_get_parent,
+};
+
+static int mux_table[SERDES_NUM_CLOCKS][3] = {
+	/*
+	 * The entries represent values for selecting between
+	 * {left input, external reference clock, right input}
+	 * Only one of Left Output or Right Output should be used since
+	 * both left and right output clock uses the same bits and modifying
+	 * one clock will impact the other.
+	 */
+	{ BIT(2),               0, BIT(0) }, /* Mux of CMU refclk */
+	{     -1,          BIT(3), BIT(1) }, /* Mux of Left Output */
+	{ BIT(1), BIT(3) | BIT(1),     -1 }, /* Mux of Right Output */
+};
+
+static int mux_mask[SERDES_NUM_CLOCKS] = { 0x5, 0xa, 0xa };
+
+static int serdes_am654_clk_register(struct serdes_am654 *am654_phy,
+				     const char *clock_name, int clock_num)
+{
+	struct device_node *node = am654_phy->of_node;
+	struct device *dev = am654_phy->dev;
+	struct serdes_am654_clk_mux *mux;
+	struct device_node *regmap_node;
+	const char **parent_names;
+	struct clk_init_data *init;
+	unsigned int num_parents;
+	struct regmap *regmap;
+	const __be32 *addr;
+	unsigned int reg;
+	struct clk *clk;
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return -ENOMEM;
+
+	init = &mux->clk_data;
+
+	regmap_node = of_parse_phandle(node, "ti,serdes-clk", 0);
+	of_node_put(regmap_node);
+	if (!regmap_node) {
+		dev_err(dev, "Fail to get serdes-clk node\n");
+		return -ENODEV;
+	}
+
+	regmap = syscon_node_to_regmap(regmap_node->parent);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Fail to get Syscon regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	num_parents = of_clk_get_parent_count(node);
+	if (num_parents < 3) {
+		dev_err(dev, "SERDES clock must have parents\n");
+		return -EINVAL;
+	}
+
+	parent_names = devm_kzalloc(dev, (sizeof(char *) * num_parents),
+				    GFP_KERNEL);
+	if (!parent_names)
+		return -ENOMEM;
+
+	of_clk_parent_fill(node, parent_names, num_parents);
+
+	addr = of_get_address(regmap_node, 0, NULL, NULL);
+	if (!addr)
+		return -EINVAL;
+
+	reg = be32_to_cpu(*addr);
+
+	init->ops = &serdes_am654_clk_mux_ops;
+	init->flags = CLK_SET_RATE_NO_REPARENT;
+	init->parent_names = parent_names;
+	init->num_parents = num_parents;
+	init->name = clock_name;
+
+	mux->table = mux_table[clock_num];
+	mux->regmap = regmap;
+	mux->reg = reg;
+	mux->shift = 4;
+	mux->mask = mux_mask[clock_num];
+	mux->hw.init = init;
+
+	/*
+	 * setup a sane default so get_parent() call evaluates
+	 * to a valid parent. Index 1 is the safest choice as
+	 * the default as it is valid value for all of serdes's
+	 * output clocks.
+	 */
+	serdes_am654_clk_mux_set_parent(&mux->hw, 1);
+	clk = devm_clk_register(dev, &mux->hw);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	am654_phy->clks[clock_num] = clk;
+
+	return 0;
+}
+
+static const struct of_device_id serdes_am654_id_table[] = {
+	{
+		.compatible = "ti,phy-am654-serdes",
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, serdes_am654_id_table);
+
+static int serdes_am654_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct clk_onecell_data *clk_data;
+	struct serdes_am654 *am654_phy;
+	struct mux_control *control;
+	const char *clock_name;
+	struct regmap *regmap;
+	struct resource *res;
+	void __iomem *base;
+	struct phy *phy;
+	int ret;
+	int i;
+
+	am654_phy = devm_kzalloc(dev, sizeof(*am654_phy), GFP_KERNEL);
+	if (!am654_phy)
+		return -ENOMEM;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "serdes");
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &serdes_am654_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	control = devm_mux_control_get(dev, NULL);
+	if (IS_ERR(control))
+		return PTR_ERR(control);
+
+	am654_phy->dev = dev;
+	am654_phy->of_node = node;
+	am654_phy->regmap = regmap;
+	am654_phy->control = control;
+
+	platform_set_drvdata(pdev, am654_phy);
+
+	for (i = 0; i < SERDES_NUM_CLOCKS; i++) {
+		ret = of_property_read_string_index(node, "clock-output-names",
+						    i, &clock_name);
+		if (ret) {
+			dev_err(dev, "Failed to get clock name\n");
+			return ret;
+		}
+
+		ret = serdes_am654_clk_register(am654_phy, clock_name, i);
+		if (ret) {
+			dev_err(dev, "Failed to initialize clock %s\n",
+				clock_name);
+			return ret;
+		}
+	}
+
+	clk_data = &am654_phy->clk_data;
+	clk_data->clks = am654_phy->clks;
+	clk_data->clk_num = SERDES_NUM_CLOCKS;
+	ret = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+	if (ret)
+		return ret;
+
+	pm_runtime_enable(dev);
+
+	phy = devm_phy_create(dev, NULL, &ops);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy_set_drvdata(phy, am654_phy);
+	phy_provider = devm_of_phy_provider_register(dev, serdes_am654_xlate);
+	if (IS_ERR(phy_provider)) {
+		ret = PTR_ERR(phy_provider);
+		goto clk_err;
+	}
+
+	return 0;
+
+clk_err:
+	of_clk_del_provider(node);
+
+	return ret;
+}
+
+static int serdes_am654_remove(struct platform_device *pdev)
+{
+	struct serdes_am654 *am654_phy = platform_get_drvdata(pdev);
+	struct device_node *node = am654_phy->of_node;
+
+	pm_runtime_disable(&pdev->dev);
+	of_clk_del_provider(node);
+
+	return 0;
+}
+
+static struct platform_driver serdes_am654_driver = {
+	.probe		= serdes_am654_probe,
+	.remove		= serdes_am654_remove,
+	.driver		= {
+		.name	= "phy-am654",
+		.of_match_table = serdes_am654_id_table,
+	},
+};
+module_platform_driver(serdes_am654_driver);
+
+MODULE_ALIAS("platform:phy-am654");
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_DESCRIPTION("TI AM654x SERDES driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH 3/3] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Roger Quadros]

Hi Kishon,

On 21/01/19 08:48, Kishon Vijay Abraham I wrote:
> Add a new SERDES driver for TI's AM654x SoC which configures
> the SERDES only for PCIe. Support fo USB3 will be added later.
> 
> SERDES in am654x has three input clocks (left input, externel reference
> clock and right input) and two output clocks (left output and right
> output) in addition to a PLL mux clock which the SERDES uses for Clock
> Multiplier Unit (CMU refclock).
> 
> The PLL mux clock can select from one of the three input clocks.
> The right output can select between left input and external reference
> clock while the left output can select between the right input and
> external reference clock.
> 
> The driver has support to select PLL mux and left/right output mux as
> specified in device tree.
> 
> [rogerq@ti.com: Fix boot lockup caused by accessing a structure member
> (hw->init) allocated in stack of probe() and accessed in get_parent]
> [rogerq@ti.com: Fix "Failed to find the parent" warnings]
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/phy/ti/Kconfig            |  11 +
>  drivers/phy/ti/Makefile           |   1 +
>  drivers/phy/ti/phy-am654-serdes.c | 528 ++++++++++++++++++++++++++++++
>  3 files changed, 540 insertions(+)
>  create mode 100644 drivers/phy/ti/phy-am654-serdes.c
> 
> diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
> index f137e0107764..6357c32de115 100644
> --- a/drivers/phy/ti/Kconfig
> +++ b/drivers/phy/ti/Kconfig
> @@ -20,6 +20,17 @@ config PHY_DM816X_USB
>  	help
>  	  Enable this for dm816x USB to work.
>  
> +config PHY_AM654_SERDES
> +	tristate "TI AM654 SERDES support"
> +	depends on OF && ARCH_K3 || COMPILE_TEST
> +	select GENERIC_PHY
> +	select MULTIPLEXER
> +	select REGMAP_MMIO
> +	select MUX_MMIO
> +	help
> +	  This option enables support for TI AM654 SerDes PHY used for
> +	  PCIe.
> +
>  config OMAP_CONTROL_PHY
>  	tristate "OMAP CONTROL PHY Driver"
>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
> index bea8f25a137a..bff901eb0ecc 100644
> --- a/drivers/phy/ti/Makefile
> +++ b/drivers/phy/ti/Makefile
> @@ -6,4 +6,5 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>  obj-$(CONFIG_PHY_TUSB1210)		+= phy-tusb1210.o
>  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
> +obj-$(CONFIG_PHY_AM654_SERDES)		+= phy-am654-serdes.o
>  obj-$(CONFIG_PHY_TI_GMII_SEL)		+= phy-gmii-sel.o
> diff --git a/drivers/phy/ti/phy-am654-serdes.c b/drivers/phy/ti/phy-am654-serdes.c
> new file mode 100644
> index 000000000000..fcc0a98fbbd3
> --- /dev/null
> +++ b/drivers/phy/ti/phy-am654-serdes.c
> @@ -0,0 +1,528 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * PCIe SERDES driver for AM654x SoC
> + *
> + * Copyright (C) 2018 Texas Instruments
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + */
> +
> +#include <dt-bindings/phy/phy.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define CMU_R07C		0x7c
> +#define CMU_MASTER_CDN_O	BIT(24)
> +
> +#define COMLANE_R138		0xb38
> +#define CONFIG_VERSION_REG_MASK	GENMASK(23, 16)
> +#define CONFIG_VERSION_REG_SHIFT 16
> +#define VERSION			0x70
> +
> +#define COMLANE_R190		0xb90
> +#define L1_MASTER_CDN_O		BIT(9)
> +
> +#define COMLANE_R194		0xb94
> +#define CMU_OK_I_0		BIT(19)
> +
> +#define SERDES_CTRL		0x1fd0
> +#define POR_EN			BIT(29)
> +
> +#define WIZ_LANEXCTL_STS	0x1fe0
> +#define TX0_ENABLE_OVL		BIT(31)
> +#define TX0_ENABLE_MASK		GENMASK(30, 29)
> +#define TX0_ENABLE_SHIFT	29
> +#define TX0_DISABLE_STATE	0x0
> +#define TX0_SLEEP_STATE		0x1
> +#define TX0_SNOOZE_STATE	0x2
> +#define TX0_ENABLE_STATE	0x3
> +#define RX0_ENABLE_OVL		BIT(15)
> +#define RX0_ENABLE_MASK		GENMASK(14, 13)
> +#define RX0_ENABLE_SHIFT	13
> +#define RX0_DISABLE_STATE	0x0
> +#define RX0_SLEEP_STATE		0x1
> +#define RX0_SNOOZE_STATE	0x2
> +#define RX0_ENABLE_STATE	0x3
> +
> +#define WIZ_PLL_CTRL		0x1ff4
> +#define PLL_ENABLE_OVL		BIT(31)
> +#define PLL_ENABLE_MASK		GENMASK(30, 29)
> +#define PLL_ENABLE_SHIFT	29
> +#define PLL_DISABLE_STATE	0x0
> +#define PLL_SLEEP_STATE		0x1
> +#define PLL_SNOOZE_STATE	0x2
> +#define PLL_ENABLE_STATE	0x3
> +#define PLL_OK			BIT(28)
> +
> +#define PLL_LOCK_TIME		100000	/* in microseconds */
> +#define SLEEP_TIME		100	/* in microseconds */
> +
> +#define LANE_USB3		0x0
> +#define LANE_PCIE0_LANE0	0x1
> +
> +#define LANE_PCIE1_LANE0	0x0
> +#define LANE_PCIE0_LANE1	0x1
> +
> +#define SERDES_NUM_CLOCKS	3
> +
> +struct serdes_am654_clk_mux {
> +	struct clk_hw	hw;
> +	struct regmap	*regmap;
> +	unsigned int	reg;
> +	int		*table;
> +	u32		mask;
> +	u8		shift;
> +	struct clk_init_data clk_data;
> +};
> +
> +#define to_serdes_am654_clk_mux(_hw)	\
> +		container_of(_hw, struct serdes_am654_clk_mux, hw)
> +
> +static struct regmap_config serdes_am654_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +struct serdes_am654 {
> +	struct regmap		*regmap;
> +	struct device		*dev;
> +	struct mux_control	*control;
> +	bool			busy;
> +	u32			type;
> +	struct device_node	*of_node;
> +	struct clk_onecell_data	clk_data;
> +	struct clk		*clks[SERDES_NUM_CLOCKS];
> +};
> +
> +static int serdes_am654_enable_pll(struct serdes_am654 *phy)
> +{
> +	u32 mask = PLL_ENABLE_OVL | PLL_ENABLE_MASK;
> +	u32 val = PLL_ENABLE_OVL | (PLL_ENABLE_STATE << PLL_ENABLE_SHIFT);
> +
> +	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, val);
> +
> +	return regmap_read_poll_timeout(phy->regmap, WIZ_PLL_CTRL, val,
> +					val & PLL_OK, 1000, PLL_LOCK_TIME);
> +}
> +
> +static void serdes_am654_disable_pll(struct serdes_am654 *phy)
> +{
> +	u32 mask = PLL_ENABLE_OVL | PLL_ENABLE_MASK;
> +
> +	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, 0);

Shouldn't PLL_ENABLE_OVL be left set otherwise override mechanism won't work?

So,

	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, PLL_ENABLE_OVL);

> +}
> +
> +static int serdes_am654_enable_txrx(struct serdes_am654 *phy)
> +{
> +	u32 mask;
> +	u32 val;
> +
> +	/* Enable TX */
> +	mask = TX0_ENABLE_OVL | TX0_ENABLE_MASK;
> +	val = TX0_ENABLE_OVL | (TX0_ENABLE_STATE << TX0_ENABLE_SHIFT);
> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, val);
> +
> +	/* Enable RX */
> +	mask = RX0_ENABLE_OVL | RX0_ENABLE_MASK;
> +	val = RX0_ENABLE_OVL | (RX0_ENABLE_STATE << RX0_ENABLE_SHIFT);
> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, val);
> +
> +	return 0;
> +}
> +
> +static int serdes_am654_disable_txrx(struct serdes_am654 *phy)
> +{
> +	u32 mask;
> +
> +	/* Disable TX */
> +	mask = TX0_ENABLE_OVL | TX0_ENABLE_MASK;
> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, 0);

Here too.
> +
> +	/* Disable RX */
> +	mask = RX0_ENABLE_OVL | RX0_ENABLE_MASK;
> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, 0);

Here as well.

> +
> +	return 0;
> +}
> +
> +static int serdes_am654_power_on(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +	struct device *dev = phy->dev;
> +	int ret;
> +	u32 val;
> +
> +	ret = serdes_am654_enable_pll(phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable PLL\n");
> +		return ret;
> +	}
> +
> +	ret = serdes_am654_enable_txrx(phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable TX RX\n");
> +		return ret;
> +	}
> +
> +	return regmap_read_poll_timeout(phy->regmap, COMLANE_R194, val,
> +					val & CMU_OK_I_0, SLEEP_TIME,
> +					PLL_LOCK_TIME);
> +}
> +
> +static int serdes_am654_power_off(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +
> +	serdes_am654_disable_txrx(phy);
> +	serdes_am654_disable_pll(phy);
> +
> +	return 0;
> +}
> +
> +static int serdes_am654_init(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +	u32 mask;
> +	u32 val;
> +
> +	mask = CONFIG_VERSION_REG_MASK;
> +	val = VERSION << CONFIG_VERSION_REG_SHIFT;
> +	regmap_update_bits(phy->regmap, COMLANE_R138, mask, val);
> +
> +	val = CMU_MASTER_CDN_O;
> +	regmap_update_bits(phy->regmap, CMU_R07C, val, val);
> +
> +	val = L1_MASTER_CDN_O;
> +	regmap_update_bits(phy->regmap, COMLANE_R190, val, val);
> +
> +	return 0;
> +}
> +
> +static int serdes_am654_reset(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +	u32 val;
> +
> +	val = POR_EN;
> +	regmap_update_bits(phy->regmap, SERDES_CTRL, val, val);
> +	mdelay(1);
> +	regmap_update_bits(phy->regmap, SERDES_CTRL, val, 0);
> +
> +	return 0;
> +}
> +
> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
> +				 *args)
> +{
> +	struct serdes_am654 *am654_phy;
> +	struct phy *phy;
> +	int ret;
> +
> +	phy = of_phy_simple_xlate(dev, args);
> +	if (IS_ERR(phy))
> +		return phy;
> +
> +	am654_phy = phy_get_drvdata(phy);
> +	if (am654_phy->type != args->args[0] && am654_phy->busy)
> +		return ERR_PTR(-EBUSY);

You set the busy flag in this function but it is never cleared.
This means the second phy_get() will always fail. We might get into this
situation for example if the driver doing the phy_get() had to bail out
due to some reason (e.g. -EPROBE_DEFER), and gets probed again and does
a phy_get() a second time for the same PHY and lane.

Can we clear the busy flag and call mux_control_deselect() on phy_put()?

cheers,
-roger

> +
> +	ret = mux_control_select(am654_phy->control, args->args[1]);
> +	if (ret) {
> +		dev_err(dev, "Failed to select SERDES Lane Function\n");
> +		return ERR_PTR(ret);
> +	}
> +
> +	am654_phy->busy = true;
> +	am654_phy->type = args->args[0];
> +
> +	return phy;
> +}
> +
> +static const struct phy_ops ops = {
> +	.reset		= serdes_am654_reset,
> +	.init		= serdes_am654_init,
> +	.power_on	= serdes_am654_power_on,
> +	.power_off	= serdes_am654_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static u8 serdes_am654_clk_mux_get_parent(struct clk_hw *hw)
> +{
> +	struct serdes_am654_clk_mux *mux = to_serdes_am654_clk_mux(hw);
> +	unsigned int num_parents = clk_hw_get_num_parents(hw);
> +	struct regmap *regmap = mux->regmap;
> +	unsigned int reg = mux->reg;
> +	unsigned int val;
> +	int i;
> +
> +	regmap_read(regmap, reg, &val);
> +	val >>= mux->shift;
> +	val &= mux->mask;
> +
> +	for (i = 0; i < num_parents; i++)
> +		if (mux->table[i] == val)
> +			return i;
> +
> +	/*
> +	 * No parent? This should never happen!
> +	 * Verify if we set a valid parent in serdes_am654_clk_register()
> +	 */
> +	WARN(1, "Failed to find the parent of %s clock\n", hw->init->name);
> +
> +	/* Make the parent lookup to fail */
> +	return num_parents;
> +}
> +
> +static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct serdes_am654_clk_mux *mux = to_serdes_am654_clk_mux(hw);
> +	struct regmap *regmap = mux->regmap;
> +	unsigned int reg = mux->reg;
> +	int val;
> +	int ret;
> +
> +	val = mux->table[index];
> +
> +	if (val == -1)
> +		return -EINVAL;
> +
> +	val <<= mux->shift;
> +	ret = regmap_update_bits(regmap, reg, mux->mask << mux->shift, val);
> +
> +	return ret;
> +}
> +
> +static const struct clk_ops serdes_am654_clk_mux_ops = {
> +	.set_parent = serdes_am654_clk_mux_set_parent,
> +	.get_parent = serdes_am654_clk_mux_get_parent,
> +};
> +
> +static int mux_table[SERDES_NUM_CLOCKS][3] = {
> +	/*
> +	 * The entries represent values for selecting between
> +	 * {left input, external reference clock, right input}
> +	 * Only one of Left Output or Right Output should be used since
> +	 * both left and right output clock uses the same bits and modifying
> +	 * one clock will impact the other.
> +	 */
> +	{ BIT(2),               0, BIT(0) }, /* Mux of CMU refclk */
> +	{     -1,          BIT(3), BIT(1) }, /* Mux of Left Output */
> +	{ BIT(1), BIT(3) | BIT(1),     -1 }, /* Mux of Right Output */
> +};
> +
> +static int mux_mask[SERDES_NUM_CLOCKS] = { 0x5, 0xa, 0xa };
> +
> +static int serdes_am654_clk_register(struct serdes_am654 *am654_phy,
> +				     const char *clock_name, int clock_num)
> +{
> +	struct device_node *node = am654_phy->of_node;
> +	struct device *dev = am654_phy->dev;
> +	struct serdes_am654_clk_mux *mux;
> +	struct device_node *regmap_node;
> +	const char **parent_names;
> +	struct clk_init_data *init;
> +	unsigned int num_parents;
> +	struct regmap *regmap;
> +	const __be32 *addr;
> +	unsigned int reg;
> +	struct clk *clk;
> +
> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return -ENOMEM;
> +
> +	init = &mux->clk_data;
> +
> +	regmap_node = of_parse_phandle(node, "ti,serdes-clk", 0);
> +	of_node_put(regmap_node);
> +	if (!regmap_node) {
> +		dev_err(dev, "Fail to get serdes-clk node\n");
> +		return -ENODEV;
> +	}
> +
> +	regmap = syscon_node_to_regmap(regmap_node->parent);
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "Fail to get Syscon regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	num_parents = of_clk_get_parent_count(node);
> +	if (num_parents < 3) {
> +		dev_err(dev, "SERDES clock must have parents\n");
> +		return -EINVAL;
> +	}
> +
> +	parent_names = devm_kzalloc(dev, (sizeof(char *) * num_parents),
> +				    GFP_KERNEL);
> +	if (!parent_names)
> +		return -ENOMEM;
> +
> +	of_clk_parent_fill(node, parent_names, num_parents);
> +
> +	addr = of_get_address(regmap_node, 0, NULL, NULL);
> +	if (!addr)
> +		return -EINVAL;
> +
> +	reg = be32_to_cpu(*addr);
> +
> +	init->ops = &serdes_am654_clk_mux_ops;
> +	init->flags = CLK_SET_RATE_NO_REPARENT;
> +	init->parent_names = parent_names;
> +	init->num_parents = num_parents;
> +	init->name = clock_name;
> +
> +	mux->table = mux_table[clock_num];
> +	mux->regmap = regmap;
> +	mux->reg = reg;
> +	mux->shift = 4;
> +	mux->mask = mux_mask[clock_num];
> +	mux->hw.init = init;
> +
> +	/*
> +	 * setup a sane default so get_parent() call evaluates
> +	 * to a valid parent. Index 1 is the safest choice as
> +	 * the default as it is valid value for all of serdes's
> +	 * output clocks.
> +	 */
> +	serdes_am654_clk_mux_set_parent(&mux->hw, 1);
> +	clk = devm_clk_register(dev, &mux->hw);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	am654_phy->clks[clock_num] = clk;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id serdes_am654_id_table[] = {
> +	{
> +		.compatible = "ti,phy-am654-serdes",
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, serdes_am654_id_table);
> +
> +static int serdes_am654_probe(struct platform_device *pdev)
> +{
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct clk_onecell_data *clk_data;
> +	struct serdes_am654 *am654_phy;
> +	struct mux_control *control;
> +	const char *clock_name;
> +	struct regmap *regmap;
> +	struct resource *res;
> +	void __iomem *base;
> +	struct phy *phy;
> +	int ret;
> +	int i;
> +
> +	am654_phy = devm_kzalloc(dev, sizeof(*am654_phy), GFP_KERNEL);
> +	if (!am654_phy)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "serdes");
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	regmap = devm_regmap_init_mmio(dev, base, &serdes_am654_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "Failed to initialize regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	control = devm_mux_control_get(dev, NULL);
> +	if (IS_ERR(control))
> +		return PTR_ERR(control);
> +
> +	am654_phy->dev = dev;
> +	am654_phy->of_node = node;
> +	am654_phy->regmap = regmap;
> +	am654_phy->control = control;
> +
> +	platform_set_drvdata(pdev, am654_phy);
> +
> +	for (i = 0; i < SERDES_NUM_CLOCKS; i++) {
> +		ret = of_property_read_string_index(node, "clock-output-names",
> +						    i, &clock_name);
> +		if (ret) {
> +			dev_err(dev, "Failed to get clock name\n");
> +			return ret;
> +		}
> +
> +		ret = serdes_am654_clk_register(am654_phy, clock_name, i);
> +		if (ret) {
> +			dev_err(dev, "Failed to initialize clock %s\n",
> +				clock_name);
> +			return ret;
> +		}
> +	}
> +
> +	clk_data = &am654_phy->clk_data;
> +	clk_data->clks = am654_phy->clks;
> +	clk_data->clk_num = SERDES_NUM_CLOCKS;
> +	ret = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_enable(dev);
> +
> +	phy = devm_phy_create(dev, NULL, &ops);
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
> +
> +	phy_set_drvdata(phy, am654_phy);
> +	phy_provider = devm_of_phy_provider_register(dev, serdes_am654_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		ret = PTR_ERR(phy_provider);
> +		goto clk_err;
> +	}
> +
> +	return 0;
> +
> +clk_err:
> +	of_clk_del_provider(node);
> +
> +	return ret;
> +}
> +
> +static int serdes_am654_remove(struct platform_device *pdev)
> +{
> +	struct serdes_am654 *am654_phy = platform_get_drvdata(pdev);
> +	struct device_node *node = am654_phy->of_node;
> +
> +	pm_runtime_disable(&pdev->dev);
> +	of_clk_del_provider(node);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver serdes_am654_driver = {
> +	.probe		= serdes_am654_probe,
> +	.remove		= serdes_am654_remove,
> +	.driver		= {
> +		.name	= "phy-am654",
> +		.of_match_table = serdes_am654_id_table,
> +	},
> +};
> +module_platform_driver(serdes_am654_driver);
> +
> +MODULE_ALIAS("platform:phy-am654");
> +MODULE_AUTHOR("Texas Instruments Inc.");
> +MODULE_DESCRIPTION("TI AM654x SERDES driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 3/3] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Kishon Vijay Abraham I]

Hi Roger,

On 21/01/19 3:17 PM, Roger Quadros wrote:
> Hi Kishon,
> 
> On 21/01/19 08:48, Kishon Vijay Abraham I wrote:
>> Add a new SERDES driver for TI's AM654x SoC which configures
>> the SERDES only for PCIe. Support fo USB3 will be added later.
>>
>> SERDES in am654x has three input clocks (left input, externel reference
>> clock and right input) and two output clocks (left output and right
>> output) in addition to a PLL mux clock which the SERDES uses for Clock
>> Multiplier Unit (CMU refclock).
>>
>> The PLL mux clock can select from one of the three input clocks.
>> The right output can select between left input and external reference
>> clock while the left output can select between the right input and
>> external reference clock.
>>
>> The driver has support to select PLL mux and left/right output mux as
>> specified in device tree.
>>
>> [rogerq@ti.com: Fix boot lockup caused by accessing a structure member
>> (hw->init) allocated in stack of probe() and accessed in get_parent]
>> [rogerq@ti.com: Fix "Failed to find the parent" warnings]
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/phy/ti/Kconfig            |  11 +
>>  drivers/phy/ti/Makefile           |   1 +
>>  drivers/phy/ti/phy-am654-serdes.c | 528 ++++++++++++++++++++++++++++++
>>  3 files changed, 540 insertions(+)
>>  create mode 100644 drivers/phy/ti/phy-am654-serdes.c
>>
>> diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
>> index f137e0107764..6357c32de115 100644
>> --- a/drivers/phy/ti/Kconfig
>> +++ b/drivers/phy/ti/Kconfig
>> @@ -20,6 +20,17 @@ config PHY_DM816X_USB
>>  	help
>>  	  Enable this for dm816x USB to work.
>>  
>> +config PHY_AM654_SERDES
>> +	tristate "TI AM654 SERDES support"
>> +	depends on OF && ARCH_K3 || COMPILE_TEST
>> +	select GENERIC_PHY
>> +	select MULTIPLEXER
>> +	select REGMAP_MMIO
>> +	select MUX_MMIO
>> +	help
>> +	  This option enables support for TI AM654 SerDes PHY used for
>> +	  PCIe.
>> +
>>  config OMAP_CONTROL_PHY
>>  	tristate "OMAP CONTROL PHY Driver"
>>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
>> diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
>> index bea8f25a137a..bff901eb0ecc 100644
>> --- a/drivers/phy/ti/Makefile
>> +++ b/drivers/phy/ti/Makefile
>> @@ -6,4 +6,5 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>>  obj-$(CONFIG_PHY_TUSB1210)		+= phy-tusb1210.o
>>  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>> +obj-$(CONFIG_PHY_AM654_SERDES)		+= phy-am654-serdes.o
>>  obj-$(CONFIG_PHY_TI_GMII_SEL)		+= phy-gmii-sel.o
>> diff --git a/drivers/phy/ti/phy-am654-serdes.c b/drivers/phy/ti/phy-am654-serdes.c
>> new file mode 100644
>> index 000000000000..fcc0a98fbbd3
>> --- /dev/null
>> +++ b/drivers/phy/ti/phy-am654-serdes.c
>> @@ -0,0 +1,528 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
>> + * PCIe SERDES driver for AM654x SoC
>> + *
>> + * Copyright (C) 2018 Texas Instruments
>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>> + */
>> +
>> +#include <dt-bindings/phy/phy.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/mux/consumer.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define CMU_R07C		0x7c
>> +#define CMU_MASTER_CDN_O	BIT(24)
>> +
>> +#define COMLANE_R138		0xb38
>> +#define CONFIG_VERSION_REG_MASK	GENMASK(23, 16)
>> +#define CONFIG_VERSION_REG_SHIFT 16
>> +#define VERSION			0x70
>> +
>> +#define COMLANE_R190		0xb90
>> +#define L1_MASTER_CDN_O		BIT(9)
>> +
>> +#define COMLANE_R194		0xb94
>> +#define CMU_OK_I_0		BIT(19)
>> +
>> +#define SERDES_CTRL		0x1fd0
>> +#define POR_EN			BIT(29)
>> +
>> +#define WIZ_LANEXCTL_STS	0x1fe0
>> +#define TX0_ENABLE_OVL		BIT(31)
>> +#define TX0_ENABLE_MASK		GENMASK(30, 29)
>> +#define TX0_ENABLE_SHIFT	29
>> +#define TX0_DISABLE_STATE	0x0
>> +#define TX0_SLEEP_STATE		0x1
>> +#define TX0_SNOOZE_STATE	0x2
>> +#define TX0_ENABLE_STATE	0x3
>> +#define RX0_ENABLE_OVL		BIT(15)
>> +#define RX0_ENABLE_MASK		GENMASK(14, 13)
>> +#define RX0_ENABLE_SHIFT	13
>> +#define RX0_DISABLE_STATE	0x0
>> +#define RX0_SLEEP_STATE		0x1
>> +#define RX0_SNOOZE_STATE	0x2
>> +#define RX0_ENABLE_STATE	0x3
>> +
>> +#define WIZ_PLL_CTRL		0x1ff4
>> +#define PLL_ENABLE_OVL		BIT(31)
>> +#define PLL_ENABLE_MASK		GENMASK(30, 29)
>> +#define PLL_ENABLE_SHIFT	29
>> +#define PLL_DISABLE_STATE	0x0
>> +#define PLL_SLEEP_STATE		0x1
>> +#define PLL_SNOOZE_STATE	0x2
>> +#define PLL_ENABLE_STATE	0x3
>> +#define PLL_OK			BIT(28)
>> +
>> +#define PLL_LOCK_TIME		100000	/* in microseconds */
>> +#define SLEEP_TIME		100	/* in microseconds */
>> +
>> +#define LANE_USB3		0x0
>> +#define LANE_PCIE0_LANE0	0x1
>> +
>> +#define LANE_PCIE1_LANE0	0x0
>> +#define LANE_PCIE0_LANE1	0x1
>> +
>> +#define SERDES_NUM_CLOCKS	3
>> +
>> +struct serdes_am654_clk_mux {
>> +	struct clk_hw	hw;
>> +	struct regmap	*regmap;
>> +	unsigned int	reg;
>> +	int		*table;
>> +	u32		mask;
>> +	u8		shift;
>> +	struct clk_init_data clk_data;
>> +};
>> +
>> +#define to_serdes_am654_clk_mux(_hw)	\
>> +		container_of(_hw, struct serdes_am654_clk_mux, hw)
>> +
>> +static struct regmap_config serdes_am654_regmap_config = {
>> +	.reg_bits = 32,
>> +	.val_bits = 32,
>> +	.reg_stride = 4,
>> +	.fast_io = true,
>> +};
>> +
>> +struct serdes_am654 {
>> +	struct regmap		*regmap;
>> +	struct device		*dev;
>> +	struct mux_control	*control;
>> +	bool			busy;
>> +	u32			type;
>> +	struct device_node	*of_node;
>> +	struct clk_onecell_data	clk_data;
>> +	struct clk		*clks[SERDES_NUM_CLOCKS];
>> +};
>> +
>> +static int serdes_am654_enable_pll(struct serdes_am654 *phy)
>> +{
>> +	u32 mask = PLL_ENABLE_OVL | PLL_ENABLE_MASK;
>> +	u32 val = PLL_ENABLE_OVL | (PLL_ENABLE_STATE << PLL_ENABLE_SHIFT);
>> +
>> +	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, val);
>> +
>> +	return regmap_read_poll_timeout(phy->regmap, WIZ_PLL_CTRL, val,
>> +					val & PLL_OK, 1000, PLL_LOCK_TIME);
>> +}
>> +
>> +static void serdes_am654_disable_pll(struct serdes_am654 *phy)
>> +{
>> +	u32 mask = PLL_ENABLE_OVL | PLL_ENABLE_MASK;
>> +
>> +	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, 0);
> 
> Shouldn't PLL_ENABLE_OVL be left set otherwise override mechanism won't work?

I have to check this.
> 
> So,
> 
> 	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, PLL_ENABLE_OVL);
> 
>> +}
>> +
>> +static int serdes_am654_enable_txrx(struct serdes_am654 *phy)
>> +{
>> +	u32 mask;
>> +	u32 val;
>> +
>> +	/* Enable TX */
>> +	mask = TX0_ENABLE_OVL | TX0_ENABLE_MASK;
>> +	val = TX0_ENABLE_OVL | (TX0_ENABLE_STATE << TX0_ENABLE_SHIFT);
>> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, val);
>> +
>> +	/* Enable RX */
>> +	mask = RX0_ENABLE_OVL | RX0_ENABLE_MASK;
>> +	val = RX0_ENABLE_OVL | (RX0_ENABLE_STATE << RX0_ENABLE_SHIFT);
>> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static int serdes_am654_disable_txrx(struct serdes_am654 *phy)
>> +{
>> +	u32 mask;
>> +
>> +	/* Disable TX */
>> +	mask = TX0_ENABLE_OVL | TX0_ENABLE_MASK;
>> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, 0);
> 
> Here too.
>> +
>> +	/* Disable RX */
>> +	mask = RX0_ENABLE_OVL | RX0_ENABLE_MASK;
>> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, 0);
> 
> Here as well.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int serdes_am654_power_on(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +	struct device *dev = phy->dev;
>> +	int ret;
>> +	u32 val;
>> +
>> +	ret = serdes_am654_enable_pll(phy);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable PLL\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = serdes_am654_enable_txrx(phy);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable TX RX\n");
>> +		return ret;
>> +	}
>> +
>> +	return regmap_read_poll_timeout(phy->regmap, COMLANE_R194, val,
>> +					val & CMU_OK_I_0, SLEEP_TIME,
>> +					PLL_LOCK_TIME);
>> +}
>> +
>> +static int serdes_am654_power_off(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +
>> +	serdes_am654_disable_txrx(phy);
>> +	serdes_am654_disable_pll(phy);
>> +
>> +	return 0;
>> +}
>> +
>> +static int serdes_am654_init(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +	u32 mask;
>> +	u32 val;
>> +
>> +	mask = CONFIG_VERSION_REG_MASK;
>> +	val = VERSION << CONFIG_VERSION_REG_SHIFT;
>> +	regmap_update_bits(phy->regmap, COMLANE_R138, mask, val);
>> +
>> +	val = CMU_MASTER_CDN_O;
>> +	regmap_update_bits(phy->regmap, CMU_R07C, val, val);
>> +
>> +	val = L1_MASTER_CDN_O;
>> +	regmap_update_bits(phy->regmap, COMLANE_R190, val, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static int serdes_am654_reset(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +	u32 val;
>> +
>> +	val = POR_EN;
>> +	regmap_update_bits(phy->regmap, SERDES_CTRL, val, val);
>> +	mdelay(1);
>> +	regmap_update_bits(phy->regmap, SERDES_CTRL, val, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
>> +				 *args)
>> +{
>> +	struct serdes_am654 *am654_phy;
>> +	struct phy *phy;
>> +	int ret;
>> +
>> +	phy = of_phy_simple_xlate(dev, args);
>> +	if (IS_ERR(phy))
>> +		return phy;
>> +
>> +	am654_phy = phy_get_drvdata(phy);
>> +	if (am654_phy->type != args->args[0] && am654_phy->busy)
>> +		return ERR_PTR(-EBUSY);
> 
> You set the busy flag in this function but it is never cleared.
> This means the second phy_get() will always fail. We might get into this
> situation for example if the driver doing the phy_get() had to bail out
> due to some reason (e.g. -EPROBE_DEFER), and gets probed again and does
> a phy_get() a second time for the same PHY and lane.
> 
> Can we clear the busy flag and call mux_control_deselect() on phy_put()?

Good point. Right now, the PHY framework doesn't have a callback to the PHY
drivers on phy_put. Looks like we might have to add it for this scenario.

Thanks
Kishon

Re: [PATCH 3/3] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Kishon Vijay Abraham I]

Hi Roger,

On 21/01/19 4:41 PM, Kishon Vijay Abraham I wrote:
> Hi Roger,
> 
> On 21/01/19 3:17 PM, Roger Quadros wrote:
>> Hi Kishon,
>>
>> On 21/01/19 08:48, Kishon Vijay Abraham I wrote:
>>> Add a new SERDES driver for TI's AM654x SoC which configures
>>> the SERDES only for PCIe. Support fo USB3 will be added later.
>>>
>>> SERDES in am654x has three input clocks (left input, externel reference
>>> clock and right input) and two output clocks (left output and right
>>> output) in addition to a PLL mux clock which the SERDES uses for Clock
>>> Multiplier Unit (CMU refclock).
>>>
>>> The PLL mux clock can select from one of the three input clocks.
>>> The right output can select between left input and external reference
>>> clock while the left output can select between the right input and
>>> external reference clock.
>>>
>>> The driver has support to select PLL mux and left/right output mux as
>>> specified in device tree.
>>>
>>> [rogerq@ti.com: Fix boot lockup caused by accessing a structure member
>>> (hw->init) allocated in stack of probe() and accessed in get_parent]
>>> [rogerq@ti.com: Fix "Failed to find the parent" warnings]
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  drivers/phy/ti/Kconfig            |  11 +
>>>  drivers/phy/ti/Makefile           |   1 +
>>>  drivers/phy/ti/phy-am654-serdes.c | 528 ++++++++++++++++++++++++++++++
>>>  3 files changed, 540 insertions(+)
>>>  create mode 100644 drivers/phy/ti/phy-am654-serdes.c
>>>
>>> diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
>>> index f137e0107764..6357c32de115 100644
>>> --- a/drivers/phy/ti/Kconfig
>>> +++ b/drivers/phy/ti/Kconfig
>>> @@ -20,6 +20,17 @@ config PHY_DM816X_USB
>>>  	help
>>>  	  Enable this for dm816x USB to work.
>>>  
>>> +config PHY_AM654_SERDES
>>> +	tristate "TI AM654 SERDES support"
>>> +	depends on OF && ARCH_K3 || COMPILE_TEST
>>> +	select GENERIC_PHY
>>> +	select MULTIPLEXER
>>> +	select REGMAP_MMIO
>>> +	select MUX_MMIO
>>> +	help
>>> +	  This option enables support for TI AM654 SerDes PHY used for
>>> +	  PCIe.
>>> +
>>>  config OMAP_CONTROL_PHY
>>>  	tristate "OMAP CONTROL PHY Driver"
>>>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
>>> diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
>>> index bea8f25a137a..bff901eb0ecc 100644
>>> --- a/drivers/phy/ti/Makefile
>>> +++ b/drivers/phy/ti/Makefile
>>> @@ -6,4 +6,5 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>>>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>>>  obj-$(CONFIG_PHY_TUSB1210)		+= phy-tusb1210.o
>>>  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>>> +obj-$(CONFIG_PHY_AM654_SERDES)		+= phy-am654-serdes.o
>>>  obj-$(CONFIG_PHY_TI_GMII_SEL)		+= phy-gmii-sel.o
>>> diff --git a/drivers/phy/ti/phy-am654-serdes.c b/drivers/phy/ti/phy-am654-serdes.c
>>> new file mode 100644
>>> index 000000000000..fcc0a98fbbd3
>>> --- /dev/null
>>> +++ b/drivers/phy/ti/phy-am654-serdes.c
>>> @@ -0,0 +1,528 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/**
>>> + * PCIe SERDES driver for AM654x SoC
>>> + *
>>> + * Copyright (C) 2018 Texas Instruments
>>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>>> + */
>>> +
>>> +#include <dt-bindings/phy/phy.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mux/consumer.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/phy/phy.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/mfd/syscon.h>
>>> +
>>> +#define CMU_R07C		0x7c
>>> +#define CMU_MASTER_CDN_O	BIT(24)
>>> +
>>> +#define COMLANE_R138		0xb38
>>> +#define CONFIG_VERSION_REG_MASK	GENMASK(23, 16)
>>> +#define CONFIG_VERSION_REG_SHIFT 16
>>> +#define VERSION			0x70
>>> +
>>> +#define COMLANE_R190		0xb90
>>> +#define L1_MASTER_CDN_O		BIT(9)
>>> +
>>> +#define COMLANE_R194		0xb94
>>> +#define CMU_OK_I_0		BIT(19)
>>> +
>>> +#define SERDES_CTRL		0x1fd0
>>> +#define POR_EN			BIT(29)
>>> +
>>> +#define WIZ_LANEXCTL_STS	0x1fe0
>>> +#define TX0_ENABLE_OVL		BIT(31)
>>> +#define TX0_ENABLE_MASK		GENMASK(30, 29)
>>> +#define TX0_ENABLE_SHIFT	29
>>> +#define TX0_DISABLE_STATE	0x0
>>> +#define TX0_SLEEP_STATE		0x1
>>> +#define TX0_SNOOZE_STATE	0x2
>>> +#define TX0_ENABLE_STATE	0x3
>>> +#define RX0_ENABLE_OVL		BIT(15)
>>> +#define RX0_ENABLE_MASK		GENMASK(14, 13)
>>> +#define RX0_ENABLE_SHIFT	13
>>> +#define RX0_DISABLE_STATE	0x0
>>> +#define RX0_SLEEP_STATE		0x1
>>> +#define RX0_SNOOZE_STATE	0x2
>>> +#define RX0_ENABLE_STATE	0x3
>>> +
>>> +#define WIZ_PLL_CTRL		0x1ff4
>>> +#define PLL_ENABLE_OVL		BIT(31)
>>> +#define PLL_ENABLE_MASK		GENMASK(30, 29)
>>> +#define PLL_ENABLE_SHIFT	29
>>> +#define PLL_DISABLE_STATE	0x0
>>> +#define PLL_SLEEP_STATE		0x1
>>> +#define PLL_SNOOZE_STATE	0x2
>>> +#define PLL_ENABLE_STATE	0x3
>>> +#define PLL_OK			BIT(28)
>>> +
>>> +#define PLL_LOCK_TIME		100000	/* in microseconds */
>>> +#define SLEEP_TIME		100	/* in microseconds */
>>> +
>>> +#define LANE_USB3		0x0
>>> +#define LANE_PCIE0_LANE0	0x1
>>> +
>>> +#define LANE_PCIE1_LANE0	0x0
>>> +#define LANE_PCIE0_LANE1	0x1
>>> +
>>> +#define SERDES_NUM_CLOCKS	3
>>> +
>>> +struct serdes_am654_clk_mux {
>>> +	struct clk_hw	hw;
>>> +	struct regmap	*regmap;
>>> +	unsigned int	reg;
>>> +	int		*table;
>>> +	u32		mask;
>>> +	u8		shift;
>>> +	struct clk_init_data clk_data;
>>> +};
>>> +
>>> +#define to_serdes_am654_clk_mux(_hw)	\
>>> +		container_of(_hw, struct serdes_am654_clk_mux, hw)
>>> +
>>> +static struct regmap_config serdes_am654_regmap_config = {
>>> +	.reg_bits = 32,
>>> +	.val_bits = 32,
>>> +	.reg_stride = 4,
>>> +	.fast_io = true,
>>> +};
>>> +
>>> +struct serdes_am654 {
>>> +	struct regmap		*regmap;
>>> +	struct device		*dev;
>>> +	struct mux_control	*control;
>>> +	bool			busy;
>>> +	u32			type;
>>> +	struct device_node	*of_node;
>>> +	struct clk_onecell_data	clk_data;
>>> +	struct clk		*clks[SERDES_NUM_CLOCKS];
>>> +};
>>> +
>>> +static int serdes_am654_enable_pll(struct serdes_am654 *phy)
>>> +{
>>> +	u32 mask = PLL_ENABLE_OVL | PLL_ENABLE_MASK;
>>> +	u32 val = PLL_ENABLE_OVL | (PLL_ENABLE_STATE << PLL_ENABLE_SHIFT);
>>> +
>>> +	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, val);
>>> +
>>> +	return regmap_read_poll_timeout(phy->regmap, WIZ_PLL_CTRL, val,
>>> +					val & PLL_OK, 1000, PLL_LOCK_TIME);
>>> +}
>>> +
>>> +static void serdes_am654_disable_pll(struct serdes_am654 *phy)
>>> +{
>>> +	u32 mask = PLL_ENABLE_OVL | PLL_ENABLE_MASK;
>>> +
>>> +	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, 0);
>>
>> Shouldn't PLL_ENABLE_OVL be left set otherwise override mechanism won't work?
> 
> I have to check this.

You are right, PLL_ENABLE_OVL should be set.

Thanks
Kishon

>>
>> So,
>>
>> 	regmap_update_bits(phy->regmap, WIZ_PLL_CTRL, mask, PLL_ENABLE_OVL);
>>
>>> +}
>>> +
>>> +static int serdes_am654_enable_txrx(struct serdes_am654 *phy)
>>> +{
>>> +	u32 mask;
>>> +	u32 val;
>>> +
>>> +	/* Enable TX */
>>> +	mask = TX0_ENABLE_OVL | TX0_ENABLE_MASK;
>>> +	val = TX0_ENABLE_OVL | (TX0_ENABLE_STATE << TX0_ENABLE_SHIFT);
>>> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, val);
>>> +
>>> +	/* Enable RX */
>>> +	mask = RX0_ENABLE_OVL | RX0_ENABLE_MASK;
>>> +	val = RX0_ENABLE_OVL | (RX0_ENABLE_STATE << RX0_ENABLE_SHIFT);
>>> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, val);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int serdes_am654_disable_txrx(struct serdes_am654 *phy)
>>> +{
>>> +	u32 mask;
>>> +
>>> +	/* Disable TX */
>>> +	mask = TX0_ENABLE_OVL | TX0_ENABLE_MASK;
>>> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, 0);
>>
>> Here too.
>>> +
>>> +	/* Disable RX */
>>> +	mask = RX0_ENABLE_OVL | RX0_ENABLE_MASK;
>>> +	regmap_update_bits(phy->regmap, WIZ_LANEXCTL_STS, mask, 0);
>>
>> Here as well.
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int serdes_am654_power_on(struct phy *x)
>>> +{
>>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>>> +	struct device *dev = phy->dev;
>>> +	int ret;
>>> +	u32 val;
>>> +
>>> +	ret = serdes_am654_enable_pll(phy);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to enable PLL\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = serdes_am654_enable_txrx(phy);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to enable TX RX\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	return regmap_read_poll_timeout(phy->regmap, COMLANE_R194, val,
>>> +					val & CMU_OK_I_0, SLEEP_TIME,
>>> +					PLL_LOCK_TIME);
>>> +}
>>> +
>>> +static int serdes_am654_power_off(struct phy *x)
>>> +{
>>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>>> +
>>> +	serdes_am654_disable_txrx(phy);
>>> +	serdes_am654_disable_pll(phy);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int serdes_am654_init(struct phy *x)
>>> +{
>>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>>> +	u32 mask;
>>> +	u32 val;
>>> +
>>> +	mask = CONFIG_VERSION_REG_MASK;
>>> +	val = VERSION << CONFIG_VERSION_REG_SHIFT;
>>> +	regmap_update_bits(phy->regmap, COMLANE_R138, mask, val);
>>> +
>>> +	val = CMU_MASTER_CDN_O;
>>> +	regmap_update_bits(phy->regmap, CMU_R07C, val, val);
>>> +
>>> +	val = L1_MASTER_CDN_O;
>>> +	regmap_update_bits(phy->regmap, COMLANE_R190, val, val);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int serdes_am654_reset(struct phy *x)
>>> +{
>>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>>> +	u32 val;
>>> +
>>> +	val = POR_EN;
>>> +	regmap_update_bits(phy->regmap, SERDES_CTRL, val, val);
>>> +	mdelay(1);
>>> +	regmap_update_bits(phy->regmap, SERDES_CTRL, val, 0);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
>>> +				 *args)
>>> +{
>>> +	struct serdes_am654 *am654_phy;
>>> +	struct phy *phy;
>>> +	int ret;
>>> +
>>> +	phy = of_phy_simple_xlate(dev, args);
>>> +	if (IS_ERR(phy))
>>> +		return phy;
>>> +
>>> +	am654_phy = phy_get_drvdata(phy);
>>> +	if (am654_phy->type != args->args[0] && am654_phy->busy)
>>> +		return ERR_PTR(-EBUSY);
>>
>> You set the busy flag in this function but it is never cleared.
>> This means the second phy_get() will always fail. We might get into this
>> situation for example if the driver doing the phy_get() had to bail out
>> due to some reason (e.g. -EPROBE_DEFER), and gets probed again and does
>> a phy_get() a second time for the same PHY and lane.
>>
>> Can we clear the busy flag and call mux_control_deselect() on phy_put()?
> 
> Good point. Right now, the PHY framework doesn't have a callback to the PHY
> drivers on phy_put. Looks like we might have to add it for this scenario.
> 
> Thanks
> Kishon
>