[PATCH 0/2] add the Amlogic Meson PCIe phy driver.

[PATCH 0/2] add the Amlogic Meson PCIe phy driver.

[Hanjie Lin]

This patcheset add the driver and dt-bindings for
the Meson-PCIE-PHY controller.

Yue Wang (2):
  dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe Phy
    controller
  PCI: meson: add the Amlogic Meson PCIe phy driver

 .../bindings/phy/amlogic,meson-pcie-phy.txt        |  31 ++++
 drivers/phy/amlogic/Kconfig                        |   8 ++
 drivers/phy/amlogic/Makefile                       |   1 +
 drivers/phy/amlogic/phy-meson-axg-pcie.c           | 160 +++++++++++++++++++++
 4 files changed, 200 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
 create mode 100644 drivers/phy/amlogic/phy-meson-axg-pcie.c

-- 
2.7.4

[PATCH 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe Phy controller

[Hanjie Lin]

From: Yue Wang <yue.wang@amlogic.com>

The Meson-PCIE-PHY controller supports the 5-Gbps data rate
of the PCI Express Gen 2 specification and is backwardcompatible
with the 2.5-Gbps Gen 1.1 specification with only
inferred idle detection supported on AMLOGIC SoCs.

Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
Signed-off-by: Yue Wang <yue.wang@amlogic.com>
---
 .../bindings/phy/amlogic,meson-pcie-phy.txt        | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt b/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
new file mode 100644
index 0000000..db99085
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
@@ -0,0 +1,31 @@
+* Amlogic Meson AXG PCIE PHY binding
+
+Required properties:
+- compatible:	Should be
+	- "amlogic,axg-pcie-phy"
+- #phys-cells:	must be 0 (see phy-bindings.txt in this directory)
+- reg:		The base address and length of the registers
+- resets:	phandle to the reset lines
+- reset-names:	must contain "phy" and "peripheral"
+	- "port_a" Port A reset
+	- "port_b" Port B reset
+	- "phy"	PHY reset
+	- "apb"	APB reset
+Optional properties:
+- phy-supply:	see phy-bindings.txt in this directory
+
+Example:
+	pcie_phy: pcie-phy@ff644000 {
+		#phy-cells = <0>;
+		compatible = "amlogic,axg-pcie-phy";
+		reg = <0x0 0xff644000 0x0 0x2000>;
+		resets = <&reset RESET_PCIE_A>,
+				<&reset RESET_PCIE_B>,
+				<&reset RESET_PCIE_PHY>,
+				<&reset RESET_PCIE_APB>;
+		reset-names =
+				"port_a",
+				"port_b",
+				"phy",
+				"apb";
+	};
-- 
2.7.4

Re: [PATCH 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe Phy controller

[Rob Herring]

On Tue, Aug 14, 2018 at 02:12:13AM -0400, Hanjie Lin wrote:
> From: Yue Wang <yue.wang@amlogic.com>

Subject should be "dt-bindings: phy: ..."

> The Meson-PCIE-PHY controller supports the 5-Gbps data rate
> of the PCI Express Gen 2 specification and is backwardcompatible

space                                                   ^

> with the 2.5-Gbps Gen 1.1 specification with only
> inferred idle detection supported on AMLOGIC SoCs.

AMLOGIC or Amlogic? 

> 
> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
> ---
>  .../bindings/phy/amlogic,meson-pcie-phy.txt        | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt b/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
> new file mode 100644
> index 0000000..db99085
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
> @@ -0,0 +1,31 @@
> +* Amlogic Meson AXG PCIE PHY binding
> +
> +Required properties:
> +- compatible:	Should be
> +	- "amlogic,axg-pcie-phy"
> +- #phys-cells:	must be 0 (see phy-bindings.txt in this directory)

You don't need to distinguish port A and B?

> +- reg:		The base address and length of the registers
> +- resets:	phandle to the reset lines
> +- reset-names:	must contain "phy" and "peripheral"
> +	- "port_a" Port A reset
> +	- "port_b" Port B reset
> +	- "phy"	PHY reset
> +	- "apb"	APB reset
> +Optional properties:
> +- phy-supply:	see phy-bindings.txt in this directory
> +
> +Example:
> +	pcie_phy: pcie-phy@ff644000 {
> +		#phy-cells = <0>;
> +		compatible = "amlogic,axg-pcie-phy";
> +		reg = <0x0 0xff644000 0x0 0x2000>;
> +		resets = <&reset RESET_PCIE_A>,
> +				<&reset RESET_PCIE_B>,
> +				<&reset RESET_PCIE_PHY>,
> +				<&reset RESET_PCIE_APB>;
> +		reset-names =
> +				"port_a",
> +				"port_b",
> +				"phy",
> +				"apb";
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe Phy controller

[Hanjie Lin]

On 2018/8/15 6:50, Rob Herring wrote:
> On Tue, Aug 14, 2018 at 02:12:13AM -0400, Hanjie Lin wrote:
>> From: Yue Wang <yue.wang@amlogic.com>
> 
> Subject should be "dt-bindings: phy: ..."
> 
>> The Meson-PCIE-PHY controller supports the 5-Gbps data rate
>> of the PCI Express Gen 2 specification and is backwardcompatible
> 
> space    

yes, I will fix
                                               ^
> 
>> with the 2.5-Gbps Gen 1.1 specification with only
>> inferred idle detection supported on AMLOGIC SoCs.
> 
> AMLOGIC or Amlogic? 
> 

yes, we will stick to 'Amlogic'

>>
>> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
>> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
>> ---
>>  .../bindings/phy/amlogic,meson-pcie-phy.txt        | 31 ++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt b/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
>> new file mode 100644
>> index 0000000..db99085
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
>> @@ -0,0 +1,31 @@
>> +* Amlogic Meson AXG PCIE PHY binding
>> +
>> +Required properties:
>> +- compatible:	Should be
>> +	- "amlogic,axg-pcie-phy"
>> +- #phys-cells:	must be 0 (see phy-bindings.txt in this directory)
> 
> You don't need to distinguish port A and B?

No, we don't. Theoretically there is only one phy in the chip,
and we have distinguished ports by reset lines.

Thanks for all corrections and suggestions.

> 
>> +- reg:		The base address and length of the registers
>> +- resets:	phandle to the reset lines
>> +- reset-names:	must contain "phy" and "peripheral"
>> +	- "port_a" Port A reset
>> +	- "port_b" Port B reset
>> +	- "phy"	PHY reset
>> +	- "apb"	APB reset
>> +Optional properties:
>> +- phy-supply:	see phy-bindings.txt in this directory
>> +
>> +Example:
>> +	pcie_phy: pcie-phy@ff644000 {
>> +		#phy-cells = <0>;
>> +		compatible = "amlogic,axg-pcie-phy";
>> +		reg = <0x0 0xff644000 0x0 0x2000>;
>> +		resets = <&reset RESET_PCIE_A>,
>> +				<&reset RESET_PCIE_B>,
>> +				<&reset RESET_PCIE_PHY>,
>> +				<&reset RESET_PCIE_APB>;
>> +		reset-names =
>> +				"port_a",
>> +				"port_b",
>> +				"phy",
>> +				"apb";
>> +	};
>> -- 
>> 2.7.4
>>
> 
> .
> 

[PATCH 2/2] PCI: meson: add the Amlogic Meson PCIe phy driver

[Hanjie Lin]

From: Yue Wang <yue.wang@amlogic.com>

The Meson-PCIE-PHY controller supports the 5-Gbps data rate
of the PCI Express Gen 2 specification and is backwardcompatible
with the 2.5-Gbps Gen 1.1 specification with only
inferred idle detection supported on AMLOGIC SoCs.

Signed-off-by: Yue Wang <yue.wang@amlogic.com>
Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
---
 drivers/phy/amlogic/Kconfig              |   8 ++
 drivers/phy/amlogic/Makefile             |   1 +
 drivers/phy/amlogic/phy-meson-axg-pcie.c | 160 +++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+)
 create mode 100644 drivers/phy/amlogic/phy-meson-axg-pcie.c

diff --git a/drivers/phy/amlogic/Kconfig b/drivers/phy/amlogic/Kconfig
index 23fe1cd..3ab07f9 100644
--- a/drivers/phy/amlogic/Kconfig
+++ b/drivers/phy/amlogic/Kconfig
@@ -36,3 +36,11 @@ config PHY_MESON_GXL_USB3
 	  Enable this to support the Meson USB3 PHY and OTG detection
 	  IP block found in Meson GXL and GXM SoCs.
 	  If unsure, say N.
+
+config PHY_MESON_AXG_PCIE
+	bool "Meson AXG PCIe PHY driver"
+	depends on OF && (ARCH_MESON || COMPILE_TEST)
+	select GENERIC_PHY
+	help
+	  Enable PCIe PHY support for Meson AXG SoC series.
+	  This driver provides PHY interface for Meson PCIe controller.
\ No newline at end of file
diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
index 4fd8848..5ab8578 100644
--- a/drivers/phy/amlogic/Makefile
+++ b/drivers/phy/amlogic/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_PHY_MESON8B_USB2)		+= phy-meson8b-usb2.o
 obj-$(CONFIG_PHY_MESON_GXL_USB2)	+= phy-meson-gxl-usb2.o
 obj-$(CONFIG_PHY_MESON_GXL_USB3)	+= phy-meson-gxl-usb3.o
+obj-$(CONFIG_PHY_MESON_AXG_PCIE)	+= phy-meson-axg-pcie.o
diff --git a/drivers/phy/amlogic/phy-meson-axg-pcie.c b/drivers/phy/amlogic/phy-meson-axg-pcie.c
new file mode 100644
index 0000000..8bc5c49
--- /dev/null
+++ b/drivers/phy/amlogic/phy-meson-axg-pcie.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: (GPL-2.0+ or MIT)
+/*
+ * Amlogic MESON SoC series PCIe PHY driver
+ *
+ * Phy provider for PCIe controller on MESON SoC series
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Yue Wang <yue.wang@amlogic.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/reset.h>
+
+struct meson_pcie_phy_data {
+	const struct phy_ops	*ops;
+};
+
+struct meson_pcie_reset {
+	struct reset_control	*port_a;
+	struct reset_control	*port_b;
+	struct reset_control	*phy;
+	struct reset_control	*apb;
+};
+
+struct meson_pcie_phy {
+	const struct meson_pcie_phy_data	*data;
+	struct meson_pcie_reset	reset;
+	void __iomem	*phy_base;
+};
+
+static int meson_pcie_phy_init(struct phy *phy)
+{
+	struct meson_pcie_phy *mphy = phy_get_drvdata(phy);
+	struct meson_pcie_reset *mrst = &mphy->reset;
+
+	writel(0x1c, mphy->phy_base);
+	reset_control_assert(mrst->port_a);
+	reset_control_assert(mrst->port_b);
+	reset_control_assert(mrst->phy);
+	reset_control_assert(mrst->apb);
+	udelay(400);
+	reset_control_deassert(mrst->port_a);
+	reset_control_deassert(mrst->port_b);
+	reset_control_deassert(mrst->phy);
+	reset_control_deassert(mrst->apb);
+	udelay(500);
+
+	return 0;
+}
+
+static const struct phy_ops meson_phy_ops = {
+	.init		= meson_pcie_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static const struct meson_pcie_phy_data meson_pcie_phy_data = {
+	.ops		= &meson_phy_ops,
+};
+
+static const struct of_device_id meson_pcie_phy_match[] = {
+	{
+		.compatible = "amlogic,axg-pcie-phy",
+		.data = &meson_pcie_phy_data,
+	},
+	{},
+};
+
+static int meson_pcie_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct meson_pcie_phy *mphy;
+	struct meson_pcie_reset *mrst;
+	struct phy *generic_phy;
+	struct phy_provider *phy_provider;
+	struct resource *res;
+	const struct meson_pcie_phy_data *data;
+
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
+	mphy = devm_kzalloc(dev, sizeof(*mphy), GFP_KERNEL);
+	if (!mphy)
+		return -ENOMEM;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mphy->phy_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mphy->phy_base))
+		return PTR_ERR(mphy->phy_base);
+
+	mrst = &mphy->reset;
+
+	mrst->port_a = devm_reset_control_get_shared(dev, "port_a");
+	if (IS_ERR(mrst->port_a)) {
+		if (PTR_ERR(mrst->port_a) != -EPROBE_DEFER)
+			dev_err(dev, "couldn't get port a reset %ld\n",
+				PTR_ERR(mrst->port_a));
+
+		return PTR_ERR(mrst->port_a);
+	}
+
+	mrst->port_b = devm_reset_control_get_shared(dev, "port_b");
+	if (IS_ERR(mrst->port_b)) {
+		if (PTR_ERR(mrst->port_b) != -EPROBE_DEFER)
+			dev_err(dev, "couldn't get port b reset %ld\n",
+				PTR_ERR(mrst->port_b));
+
+		return PTR_ERR(mrst->port_b);
+	}
+
+	mrst->phy = devm_reset_control_get_shared(dev, "phy");
+	if (IS_ERR(mrst->phy)) {
+		if (PTR_ERR(mrst->phy) != -EPROBE_DEFER)
+			dev_err(dev, "couldn't get phy reset\n");
+
+		return PTR_ERR(mrst->phy);
+	}
+
+	mrst->apb = devm_reset_control_get_shared(dev, "apb");
+	if (IS_ERR(mrst->apb)) {
+		if (PTR_ERR(mrst->apb) != -EPROBE_DEFER)
+			dev_err(dev, "couldn't get apb reset\n");
+
+		return PTR_ERR(mrst->apb);
+	}
+
+	reset_control_deassert(mrst->port_a);
+	reset_control_deassert(mrst->port_b);
+	reset_control_deassert(mrst->phy);
+	reset_control_deassert(mrst->apb);
+
+	mphy->data = data;
+
+	generic_phy = devm_phy_create(dev, dev->of_node, mphy->data->ops);
+	if (IS_ERR(generic_phy)) {
+		if (PTR_ERR(generic_phy) != -EPROBE_DEFER)
+			dev_err(dev, "failed to create PHY\n");
+
+		return PTR_ERR(generic_phy);
+	}
+
+	phy_set_drvdata(generic_phy, mphy);
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static struct platform_driver meson_pcie_phy_driver = {
+	.probe	= meson_pcie_phy_probe,
+	.driver = {
+		.of_match_table	= meson_pcie_phy_match,
+		.name		= "meson-pcie-phy",
+	}
+};
+
+builtin_platform_driver(meson_pcie_phy_driver);
-- 
2.7.4

Re: [PATCH 2/2] PCI: meson: add the Amlogic Meson PCIe phy driver

[Jerome Brunet]

On Tue, 2018-08-14 at 02:12 -0400, Hanjie Lin wrote:
> From: Yue Wang <yue.wang@amlogic.com>
> 
> The Meson-PCIE-PHY controller supports the 5-Gbps data rate
> of the PCI Express Gen 2 specification and is backwardcompatible
> with the 2.5-Gbps Gen 1.1 specification with only
> inferred idle detection supported on AMLOGIC SoCs.

It looks like the sole purpose of this driver is to provide the reset lines to
pcie driver.

I wonder why we need this ? Can't the pcie driver claim the reset lines itself.

Also, an init of this phy will always reset both port. What will happen if the
first port is in use and the 2nd port comes up ?? 

Looks the the pcie driver should claim 'apb' and 'phy' reset lines as "shared"
reset and the required 'port' as 'exclusive'

> 
> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
> ---
>  drivers/phy/amlogic/Kconfig              |   8 ++
>  drivers/phy/amlogic/Makefile             |   1 +
>  drivers/phy/amlogic/phy-meson-axg-pcie.c | 160 +++++++++++++++++++++++++++++++
>  3 files changed, 169 insertions(+)
>  create mode 100644 drivers/phy/amlogic/phy-meson-axg-pcie.c
> 
> diff --git a/drivers/phy/amlogic/Kconfig b/drivers/phy/amlogic/Kconfig
> index 23fe1cd..3ab07f9 100644
> --- a/drivers/phy/amlogic/Kconfig
> +++ b/drivers/phy/amlogic/Kconfig
> @@ -36,3 +36,11 @@ config PHY_MESON_GXL_USB3
>  	  Enable this to support the Meson USB3 PHY and OTG detection
>  	  IP block found in Meson GXL and GXM SoCs.
>  	  If unsure, say N.
> +
> +config PHY_MESON_AXG_PCIE
> +	bool "Meson AXG PCIe PHY driver"
> +	depends on OF && (ARCH_MESON || COMPILE_TEST)
> +	select GENERIC_PHY
> +	help
> +	  Enable PCIe PHY support for Meson AXG SoC series.
> +	  This driver provides PHY interface for Meson PCIe controller.
> \ No newline at end of file
> diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
> index 4fd8848..5ab8578 100644
> --- a/drivers/phy/amlogic/Makefile
> +++ b/drivers/phy/amlogic/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_PHY_MESON8B_USB2)		+= phy-meson8b-usb2.o
>  obj-$(CONFIG_PHY_MESON_GXL_USB2)	+= phy-meson-gxl-usb2.o
>  obj-$(CONFIG_PHY_MESON_GXL_USB3)	+= phy-meson-gxl-usb3.o
> +obj-$(CONFIG_PHY_MESON_AXG_PCIE)	+= phy-meson-axg-pcie.o
> diff --git a/drivers/phy/amlogic/phy-meson-axg-pcie.c b/drivers/phy/amlogic/phy-meson-axg-pcie.c
> new file mode 100644
> index 0000000..8bc5c49
> --- /dev/null
> +++ b/drivers/phy/amlogic/phy-meson-axg-pcie.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
> +/*
> + * Amlogic MESON SoC series PCIe PHY driver
> + *
> + * Phy provider for PCIe controller on MESON SoC series
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Yue Wang <yue.wang@amlogic.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/reset.h>
> +
> +struct meson_pcie_phy_data {
> +	const struct phy_ops	*ops;
> +};
> +
> +struct meson_pcie_reset {
> +	struct reset_control	*port_a;
> +	struct reset_control	*port_b;
> +	struct reset_control	*phy;
> +	struct reset_control	*apb;
> +};
> +
> +struct meson_pcie_phy {
> +	const struct meson_pcie_phy_data	*data;
> +	struct meson_pcie_reset	reset;
> +	void __iomem	*phy_base;
> +};
> +
> +static int meson_pcie_phy_init(struct phy *phy)
> +{
> +	struct meson_pcie_phy *mphy = phy_get_drvdata(phy);
> +	struct meson_pcie_reset *mrst = &mphy->reset;
> +
> +	writel(0x1c, mphy->phy_base);
> +	reset_control_assert(mrst->port_a);
> +	reset_control_assert(mrst->port_b);
> +	reset_control_assert(mrst->phy);
> +	reset_control_assert(mrst->apb);
> +	udelay(400);
> +	reset_control_deassert(mrst->port_a);
> +	reset_control_deassert(mrst->port_b);
> +	reset_control_deassert(mrst->phy);
> +	reset_control_deassert(mrst->apb);
> +	udelay(500);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops meson_phy_ops = {
> +	.init		= meson_pcie_phy_init,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct meson_pcie_phy_data meson_pcie_phy_data = {
> +	.ops		= &meson_phy_ops,
> +};
> +
> +static const struct of_device_id meson_pcie_phy_match[] = {
> +	{
> +		.compatible = "amlogic,axg-pcie-phy",
> +		.data = &meson_pcie_phy_data,
> +	},
> +	{},
> +};
> +
> +static int meson_pcie_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct meson_pcie_phy *mphy;
> +	struct meson_pcie_reset *mrst;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
> +	struct resource *res;
> +	const struct meson_pcie_phy_data *data;
> +
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	mphy = devm_kzalloc(dev, sizeof(*mphy), GFP_KERNEL);
> +	if (!mphy)
> +		return -ENOMEM;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mphy->phy_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(mphy->phy_base))
> +		return PTR_ERR(mphy->phy_base);
> +
> +	mrst = &mphy->reset;
> +
> +	mrst->port_a = devm_reset_control_get_shared(dev, "port_a");
> +	if (IS_ERR(mrst->port_a)) {
> +		if (PTR_ERR(mrst->port_a) != -EPROBE_DEFER)
> +			dev_err(dev, "couldn't get port a reset %ld\n",
> +				PTR_ERR(mrst->port_a));
> +
> +		return PTR_ERR(mrst->port_a);
> +	}
> +
> +	mrst->port_b = devm_reset_control_get_shared(dev, "port_b");
> +	if (IS_ERR(mrst->port_b)) {
> +		if (PTR_ERR(mrst->port_b) != -EPROBE_DEFER)
> +			dev_err(dev, "couldn't get port b reset %ld\n",
> +				PTR_ERR(mrst->port_b));
> +
> +		return PTR_ERR(mrst->port_b);
> +	}
> +
> +	mrst->phy = devm_reset_control_get_shared(dev, "phy");
> +	if (IS_ERR(mrst->phy)) {
> +		if (PTR_ERR(mrst->phy) != -EPROBE_DEFER)
> +			dev_err(dev, "couldn't get phy reset\n");
> +
> +		return PTR_ERR(mrst->phy);
> +	}
> +
> +	mrst->apb = devm_reset_control_get_shared(dev, "apb");
> +	if (IS_ERR(mrst->apb)) {
> +		if (PTR_ERR(mrst->apb) != -EPROBE_DEFER)
> +			dev_err(dev, "couldn't get apb reset\n");
> +
> +		return PTR_ERR(mrst->apb);
> +	}
> +
> +	reset_control_deassert(mrst->port_a);
> +	reset_control_deassert(mrst->port_b);
> +	reset_control_deassert(mrst->phy);
> +	reset_control_deassert(mrst->apb);
> +
> +	mphy->data = data;
> +
> +	generic_phy = devm_phy_create(dev, dev->of_node, mphy->data->ops);
> +	if (IS_ERR(generic_phy)) {
> +		if (PTR_ERR(generic_phy) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to create PHY\n");
> +
> +		return PTR_ERR(generic_phy);
> +	}
> +
> +	phy_set_drvdata(generic_phy, mphy);
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static struct platform_driver meson_pcie_phy_driver = {
> +	.probe	= meson_pcie_phy_probe,
> +	.driver = {
> +		.of_match_table	= meson_pcie_phy_match,
> +		.name		= "meson-pcie-phy",
> +	}
> +};
> +
> +builtin_platform_driver(meson_pcie_phy_driver);

Re: [PATCH 2/2] PCI: meson: add the Amlogic Meson PCIe phy driver

[Hanjie Lin]

On 2018/8/14 18:41, Jerome Brunet wrote:
> On Tue, 2018-08-14 at 02:12 -0400, Hanjie Lin wrote:
>> From: Yue Wang <yue.wang@amlogic.com>
>>
>> The Meson-PCIE-PHY controller supports the 5-Gbps data rate
>> of the PCI Express Gen 2 specification and is backwardcompatible
>> with the 2.5-Gbps Gen 1.1 specification with only
>> inferred idle detection supported on AMLOGIC SoCs.
> 
> It looks like the sole purpose of this driver is to provide the reset lines to
> pcie driver.
> 
> I wonder why we need this ? Can't the pcie driver claim the reset lines itself.
> 
> Also, an init of this phy will always reset both port. What will happen if the
> first port is in use and the 2nd port comes up ?? 
> 
> Looks the the pcie driver should claim 'apb' and 'phy' reset lines as "shared"
> reset and the required 'port' as 'exclusive'
> 

Thank you for your response.

Yes, 'apb' and 'phy' reset lines are shared, and ‘port' reset line is exclusive.
If we handle all reset lines during the first port initial sequence, 
and when the second port comes up, we will do nothing about the rest lines, 
and don't need a extra API to do ‘port' reset;

>>
>> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
>> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
>> ---
>>  drivers/phy/amlogic/Kconfig              |   8 ++
>>  drivers/phy/amlogic/Makefile             |   1 +
>>  drivers/phy/amlogic/phy-meson-axg-pcie.c | 160 +++++++++++++++++++++++++++++++
>>  3 files changed, 169 insertions(+)
>>  create mode 100644 drivers/phy/amlogic/phy-meson-axg-pcie.c
>>
>> diff --git a/drivers/phy/amlogic/Kconfig b/drivers/phy/amlogic/Kconfig
>> index 23fe1cd..3ab07f9 100644
>> --- a/drivers/phy/amlogic/Kconfig
>> +++ b/drivers/phy/amlogic/Kconfig
>> @@ -36,3 +36,11 @@ config PHY_MESON_GXL_USB3
>>  	  Enable this to support the Meson USB3 PHY and OTG detection
>>  	  IP block found in Meson GXL and GXM SoCs.
>>  	  If unsure, say N.
>> +
>> +config PHY_MESON_AXG_PCIE
>> +	bool "Meson AXG PCIe PHY driver"
>> +	depends on OF && (ARCH_MESON || COMPILE_TEST)
>> +	select GENERIC_PHY
>> +	help
>> +	  Enable PCIe PHY support for Meson AXG SoC series.
>> +	  This driver provides PHY interface for Meson PCIe controller.
>> \ No newline at end of file
>> diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
>> index 4fd8848..5ab8578 100644
>> --- a/drivers/phy/amlogic/Makefile
>> +++ b/drivers/phy/amlogic/Makefile
>> @@ -1,3 +1,4 @@
>>  obj-$(CONFIG_PHY_MESON8B_USB2)		+= phy-meson8b-usb2.o
>>  obj-$(CONFIG_PHY_MESON_GXL_USB2)	+= phy-meson-gxl-usb2.o
>>  obj-$(CONFIG_PHY_MESON_GXL_USB3)	+= phy-meson-gxl-usb3.o
>> +obj-$(CONFIG_PHY_MESON_AXG_PCIE)	+= phy-meson-axg-pcie.o
>> diff --git a/drivers/phy/amlogic/phy-meson-axg-pcie.c b/drivers/phy/amlogic/phy-meson-axg-pcie.c
>> new file mode 100644
>> index 0000000..8bc5c49
>> --- /dev/null
>> +++ b/drivers/phy/amlogic/phy-meson-axg-pcie.c
>> @@ -0,0 +1,160 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
>> +/*
>> + * Amlogic MESON SoC series PCIe PHY driver
>> + *
>> + * Phy provider for PCIe controller on MESON SoC series
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Yue Wang <yue.wang@amlogic.com>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/reset.h>
>> +
>> +struct meson_pcie_phy_data {
>> +	const struct phy_ops	*ops;
>> +};
>> +
>> +struct meson_pcie_reset {
>> +	struct reset_control	*port_a;
>> +	struct reset_control	*port_b;
>> +	struct reset_control	*phy;
>> +	struct reset_control	*apb;
>> +};
>> +
>> +struct meson_pcie_phy {
>> +	const struct meson_pcie_phy_data	*data;
>> +	struct meson_pcie_reset	reset;
>> +	void __iomem	*phy_base;
>> +};
>> +
>> +static int meson_pcie_phy_init(struct phy *phy)
>> +{
>> +	struct meson_pcie_phy *mphy = phy_get_drvdata(phy);
>> +	struct meson_pcie_reset *mrst = &mphy->reset;
>> +
>> +	writel(0x1c, mphy->phy_base);
>> +	reset_control_assert(mrst->port_a);
>> +	reset_control_assert(mrst->port_b);
>> +	reset_control_assert(mrst->phy);
>> +	reset_control_assert(mrst->apb);
>> +	udelay(400);
>> +	reset_control_deassert(mrst->port_a);
>> +	reset_control_deassert(mrst->port_b);
>> +	reset_control_deassert(mrst->phy);
>> +	reset_control_deassert(mrst->apb);
>> +	udelay(500);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct phy_ops meson_phy_ops = {
>> +	.init		= meson_pcie_phy_init,
>> +	.owner		= THIS_MODULE,
>> +};
>> +
>> +static const struct meson_pcie_phy_data meson_pcie_phy_data = {
>> +	.ops		= &meson_phy_ops,
>> +};
>> +
>> +static const struct of_device_id meson_pcie_phy_match[] = {
>> +	{
>> +		.compatible = "amlogic,axg-pcie-phy",
>> +		.data = &meson_pcie_phy_data,
>> +	},
>> +	{},
>> +};
>> +
>> +static int meson_pcie_phy_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct meson_pcie_phy *mphy;
>> +	struct meson_pcie_reset *mrst;
>> +	struct phy *generic_phy;
>> +	struct phy_provider *phy_provider;
>> +	struct resource *res;
>> +	const struct meson_pcie_phy_data *data;
>> +
>> +	data = of_device_get_match_data(dev);
>> +	if (!data)
>> +		return -ENODEV;
>> +
>> +	mphy = devm_kzalloc(dev, sizeof(*mphy), GFP_KERNEL);
>> +	if (!mphy)
>> +		return -ENOMEM;
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	mphy->phy_base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(mphy->phy_base))
>> +		return PTR_ERR(mphy->phy_base);
>> +
>> +	mrst = &mphy->reset;
>> +
>> +	mrst->port_a = devm_reset_control_get_shared(dev, "port_a");
>> +	if (IS_ERR(mrst->port_a)) {
>> +		if (PTR_ERR(mrst->port_a) != -EPROBE_DEFER)
>> +			dev_err(dev, "couldn't get port a reset %ld\n",
>> +				PTR_ERR(mrst->port_a));
>> +
>> +		return PTR_ERR(mrst->port_a);
>> +	}
>> +
>> +	mrst->port_b = devm_reset_control_get_shared(dev, "port_b");
>> +	if (IS_ERR(mrst->port_b)) {
>> +		if (PTR_ERR(mrst->port_b) != -EPROBE_DEFER)
>> +			dev_err(dev, "couldn't get port b reset %ld\n",
>> +				PTR_ERR(mrst->port_b));
>> +
>> +		return PTR_ERR(mrst->port_b);
>> +	}
>> +
>> +	mrst->phy = devm_reset_control_get_shared(dev, "phy");
>> +	if (IS_ERR(mrst->phy)) {
>> +		if (PTR_ERR(mrst->phy) != -EPROBE_DEFER)
>> +			dev_err(dev, "couldn't get phy reset\n");
>> +
>> +		return PTR_ERR(mrst->phy);
>> +	}
>> +
>> +	mrst->apb = devm_reset_control_get_shared(dev, "apb");
>> +	if (IS_ERR(mrst->apb)) {
>> +		if (PTR_ERR(mrst->apb) != -EPROBE_DEFER)
>> +			dev_err(dev, "couldn't get apb reset\n");
>> +
>> +		return PTR_ERR(mrst->apb);
>> +	}
>> +
>> +	reset_control_deassert(mrst->port_a);
>> +	reset_control_deassert(mrst->port_b);
>> +	reset_control_deassert(mrst->phy);
>> +	reset_control_deassert(mrst->apb);
>> +
>> +	mphy->data = data;
>> +
>> +	generic_phy = devm_phy_create(dev, dev->of_node, mphy->data->ops);
>> +	if (IS_ERR(generic_phy)) {
>> +		if (PTR_ERR(generic_phy) != -EPROBE_DEFER)
>> +			dev_err(dev, "failed to create PHY\n");
>> +
>> +		return PTR_ERR(generic_phy);
>> +	}
>> +
>> +	phy_set_drvdata(generic_phy, mphy);
>> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>> +
>> +	return PTR_ERR_OR_ZERO(phy_provider);
>> +}
>> +
>> +static struct platform_driver meson_pcie_phy_driver = {
>> +	.probe	= meson_pcie_phy_probe,
>> +	.driver = {
>> +		.of_match_table	= meson_pcie_phy_match,
>> +		.name		= "meson-pcie-phy",
>> +	}
>> +};
>> +
>> +builtin_platform_driver(meson_pcie_phy_driver);
> 
> 
> .
> 

Re: [PATCH 2/2] PCI: meson: add the Amlogic Meson PCIe phy driver

[Jerome Brunet]

On Thu, 2018-08-16 at 11:05 +0800, Hanjie Lin wrote:
> 
> On 2018/8/14 18:41, Jerome Brunet wrote:
> > On Tue, 2018-08-14 at 02:12 -0400, Hanjie Lin wrote:
> > > From: Yue Wang <yue.wang@amlogic.com>
> > > 
> > > The Meson-PCIE-PHY controller supports the 5-Gbps data rate
> > > of the PCI Express Gen 2 specification and is backwardcompatible
> > > with the 2.5-Gbps Gen 1.1 specification with only
> > > inferred idle detection supported on AMLOGIC SoCs.
> > 
> > It looks like the sole purpose of this driver is to provide the reset lines to
> > pcie driver.
> > 
> > I wonder why we need this ? Can't the pcie driver claim the reset lines itself.
> > 
> > Also, an init of this phy will always reset both port. What will happen if the
> > first port is in use and the 2nd port comes up ?? 
> > 
> > Looks the the pcie driver should claim 'apb' and 'phy' reset lines as "shared"
> > reset and the required 'port' as 'exclusive'
> > 
> 
> Thank you for your response.
> 
> Yes, 'apb' and 'phy' reset lines are shared, and ‘port' reset line is exclusive.
> If we handle all reset lines during the first port initial sequence, 
> and when the second port comes up, we will do nothing about the rest lines, 
> and don't need a extra API to do ‘port' reset;

With which other driver are your control shared ?

Looks it is the answer is none since this phy driver will reset both port
already, even if one is used.

In this case the fact that you are using shared control is just abusing the
framework to reset once.

As far as I can tell, this driver makes no sense. The appropriate reset lines
should be given directly to your pcie driver. 

Re: [PATCH 2/2] PCI: meson: add the Amlogic Meson PCIe phy driver

[Hanjie Lin]

On 2018/8/16 16:33, Jerome Brunet wrote:
> On Thu, 2018-08-16 at 11:05 +0800, Hanjie Lin wrote:
>>
>> On 2018/8/14 18:41, Jerome Brunet wrote:
>>> On Tue, 2018-08-14 at 02:12 -0400, Hanjie Lin wrote:
>>>> From: Yue Wang <yue.wang@amlogic.com>
>>>>
>>>> The Meson-PCIE-PHY controller supports the 5-Gbps data rate
>>>> of the PCI Express Gen 2 specification and is backwardcompatible
>>>> with the 2.5-Gbps Gen 1.1 specification with only
>>>> inferred idle detection supported on AMLOGIC SoCs.
>>>
>>> It looks like the sole purpose of this driver is to provide the reset lines to
>>> pcie driver.
>>>
>>> I wonder why we need this ? Can't the pcie driver claim the reset lines itself.
>>>
>>> Also, an init of this phy will always reset both port. What will happen if the
>>> first port is in use and the 2nd port comes up ?? 
>>>
>>> Looks the the pcie driver should claim 'apb' and 'phy' reset lines as "shared"
>>> reset and the required 'port' as 'exclusive'
>>>
>>
>> Thank you for your response.
>>
>> Yes, 'apb' and 'phy' reset lines are shared, and ‘port' reset line is exclusive.
>> If we handle all reset lines during the first port initial sequence, 
>> and when the second port comes up, we will do nothing about the rest lines, 
>> and don't need a extra API to do ‘port' reset;
> 
> With which other driver are your control shared ?
> 
> Looks it is the answer is none since this phy driver will reset both port
> already, even if one is used.
> 
> In this case the fact that you are using shared control is just abusing the
> framework to reset once.
> 
> As far as I can tell, this driver makes no sense. The appropriate reset lines
> should be given directly to your pcie driver. 
> 
> 
> 
> .
> 

Amlogic AXG SOC includes two pcie controllers and pipes when only one pcie phy: 

                                    (port_a reset)
                      |PCIE_RC_A---->PCIE_PIPE_A------| 
    (apb_reset)       |                               |  (phy reset)
    APB BUS--->       |                               |   PCIE_PHY
                      |                               |
                      |             (port_b_reset)    |
                      |PCIE_RC_B---->PCIE_PIPE_B------|

The phy_reset affect the PCIE_PHY.
The port_a_reset affect the PCIE_PIPE_A, port_b_reset affect the PCIE_PIPE_B. 

As your suggestion we will move the 'port' reset to controller driver,
and keeping the phy driver to process the 'apb' and 'phy' reset or any
more changes of the phy in future.

Re: [PATCH 2/2] PCI: meson: add the Amlogic Meson PCIe phy driver

[Jerome Brunet]

On Fri, 2018-08-17 at 14:12 +0800, Hanjie Lin wrote:
> 
> On 2018/8/16 16:33, Jerome Brunet wrote:
> > On Thu, 2018-08-16 at 11:05 +0800, Hanjie Lin wrote:
> > > 
> > > On 2018/8/14 18:41, Jerome Brunet wrote:
> > > > On Tue, 2018-08-14 at 02:12 -0400, Hanjie Lin wrote:
> > > > > From: Yue Wang <yue.wang@amlogic.com>
> > > > > 
> > > > > The Meson-PCIE-PHY controller supports the 5-Gbps data rate
> > > > > of the PCI Express Gen 2 specification and is backwardcompatible
> > > > > with the 2.5-Gbps Gen 1.1 specification with only
> > > > > inferred idle detection supported on AMLOGIC SoCs.
> > > > 
> > > > It looks like the sole purpose of this driver is to provide the reset lines to
> > > > pcie driver.
> > > > 
> > > > I wonder why we need this ? Can't the pcie driver claim the reset lines itself.
> > > > 
> > > > Also, an init of this phy will always reset both port. What will happen if the
> > > > first port is in use and the 2nd port comes up ?? 
> > > > 
> > > > Looks the the pcie driver should claim 'apb' and 'phy' reset lines as "shared"
> > > > reset and the required 'port' as 'exclusive'
> > > > 
> > > 
> > > Thank you for your response.
> > > 
> > > Yes, 'apb' and 'phy' reset lines are shared, and ‘port' reset line is exclusive.
> > > If we handle all reset lines during the first port initial sequence, 
> > > and when the second port comes up, we will do nothing about the rest lines, 
> > > and don't need a extra API to do ‘port' reset;
> > 
> > With which other driver are your control shared ?
> > 
> > Looks it is the answer is none since this phy driver will reset both port
> > already, even if one is used.
> > 
> > In this case the fact that you are using shared control is just abusing the
> > framework to reset once.
> > 
> > As far as I can tell, this driver makes no sense. The appropriate reset lines
> > should be given directly to your pcie driver. 
> > 
> > 
> > 
> > .
> > 
> 
> Amlogic AXG SOC includes two pcie controllers and pipes when only one pcie phy: 
> 
>                                     (port_a reset)
>                       |PCIE_RC_A---->PCIE_PIPE_A------| 
>     (apb_reset)       |                               |  (phy reset)
>     APB BUS--->       |                               |   PCIE_PHY
>                       |                               |
>                       |             (port_b_reset)    |
>                       |PCIE_RC_B---->PCIE_PIPE_B------|
> 
> The phy_reset affect the PCIE_PHY.
> The port_a_reset affect the PCIE_PIPE_A, port_b_reset affect the PCIE_PIPE_B. 
> 
> As your suggestion we will move the 'port' reset to controller driver,
> and keeping the phy driver to process the 'apb' and 'phy' reset or any
> more changes of the phy in future.

As far as I can tell from this diagram, It would only make sense for the "phy"
reset line to be controlled by your phy driver.

The apb and port is obviously related to the pcie device/driver itself, not the
PHY. And whether you 1 or 2 reset lines in it, IMO it is overkill and
unnecessary to make a phy driver for this. 

> 

Re: [PATCH 2/2] PCI: meson: add the Amlogic Meson PCIe phy driver

[Hanjie Lin]

On 2018/8/17 16:09, Jerome Brunet wrote:
> On Fri, 2018-08-17 at 14:12 +0800, Hanjie Lin wrote:
>>
>> On 2018/8/16 16:33, Jerome Brunet wrote:
>>> On Thu, 2018-08-16 at 11:05 +0800, Hanjie Lin wrote:
>>>>
>>>> On 2018/8/14 18:41, Jerome Brunet wrote:
>>>>> On Tue, 2018-08-14 at 02:12 -0400, Hanjie Lin wrote:
>>>>>> From: Yue Wang <yue.wang@amlogic.com>
>>>>>>
>>>>>> The Meson-PCIE-PHY controller supports the 5-Gbps data rate
>>>>>> of the PCI Express Gen 2 specification and is backwardcompatible
>>>>>> with the 2.5-Gbps Gen 1.1 specification with only
>>>>>> inferred idle detection supported on AMLOGIC SoCs.
>>>>>
>>>>> It looks like the sole purpose of this driver is to provide the reset lines to
>>>>> pcie driver.
>>>>>
>>>>> I wonder why we need this ? Can't the pcie driver claim the reset lines itself.
>>>>>
>>>>> Also, an init of this phy will always reset both port. What will happen if the
>>>>> first port is in use and the 2nd port comes up ?? 
>>>>>
>>>>> Looks the the pcie driver should claim 'apb' and 'phy' reset lines as "shared"
>>>>> reset and the required 'port' as 'exclusive'
>>>>>
>>>>
>>>> Thank you for your response.
>>>>
>>>> Yes, 'apb' and 'phy' reset lines are shared, and ‘port' reset line is exclusive.
>>>> If we handle all reset lines during the first port initial sequence, 
>>>> and when the second port comes up, we will do nothing about the rest lines, 
>>>> and don't need a extra API to do ‘port' reset;
>>>
>>> With which other driver are your control shared ?
>>>
>>> Looks it is the answer is none since this phy driver will reset both port
>>> already, even if one is used.
>>>
>>> In this case the fact that you are using shared control is just abusing the
>>> framework to reset once.
>>>
>>> As far as I can tell, this driver makes no sense. The appropriate reset lines
>>> should be given directly to your pcie driver. 
>>>
>>>
>>>
>>> .
>>>
>>
>> Amlogic AXG SOC includes two pcie controllers and pipes when only one pcie phy: 
>>
>>                                     (port_a reset)
>>                       |PCIE_RC_A---->PCIE_PIPE_A------| 
>>     (apb_reset)       |                               |  (phy reset)
>>     APB BUS--->       |                               |   PCIE_PHY
>>                       |                               |
>>                       |             (port_b_reset)    |
>>                       |PCIE_RC_B---->PCIE_PIPE_B------|
>>
>> The phy_reset affect the PCIE_PHY.
>> The port_a_reset affect the PCIE_PIPE_A, port_b_reset affect the PCIE_PIPE_B. 
>>
>> As your suggestion we will move the 'port' reset to controller driver,
>> and keeping the phy driver to process the 'apb' and 'phy' reset or any
>> more changes of the phy in future.
> 
> As far as I can tell from this diagram, It would only make sense for the "phy"
> reset line to be controlled by your phy driver.
> 
> The apb and port is obviously related to the pcie device/driver itself, not the
> PHY. And whether you 1 or 2 reset lines in it, IMO it is overkill and
> unnecessary to make a phy driver for this. 
> 

Yeah, that makes sense.
We will move 'apb' reset to controller driver in next version too.

Thanks.

>>
> 
> 
> .
>