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

[PATCH v2 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.

Changes since v1: [0]
 - move 'apb' and 'port' reset to ctrl driver
 - format correcting

[0] : https://lkml.org/lkml/2018/8/14/66 

Yue Wang (2):
  dt-bindings: phy: 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        |  21 ++++
 drivers/phy/amlogic/Kconfig                        |   8 ++
 drivers/phy/amlogic/Makefile                       |   1 +
 drivers/phy/amlogic/phy-meson-axg-pcie.c           | 124 +++++++++++++++++++++
 4 files changed, 154 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 v2 1/2] dt-bindings: phy: 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 backward compatible
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         | 21 +++++++++++++++++++++
 1 file changed, 21 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..e2f0a27
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
@@ -0,0 +1,21 @@
+* 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"
+	- "phy"	PHY 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_PHY>;
+		reset-names = "phy";
+	};
-- 
2.7.4

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

[Rob Herring]

On Fri, 24 Aug 2018 15:33:25 +0800, 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 backward compatible
> 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         | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Hanjie Lin]

On 2018/8/29 8:37, Rob Herring wrote:
> On Fri, 24 Aug 2018 15:33:25 +0800, 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 backward compatible
>> 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         | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-pcie-phy.txt
>>
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> .
> 

Thanks for the review.

As described during the discussion [0], we consider it's too overkill to
have a dedicated phy driver which only process reset line. So we will abandon phy driver 
and integrate phy reset into the controller driver int the next version. 

[0] https://lkml.kernel.org/r/1535096165-45827-1-git-send-email-hanjie.lin@amlogic.

[PATCH v2 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 | 124 +++++++++++++++++++++++++++++++
 3 files changed, 133 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..b517f9e
--- /dev/null
+++ b/drivers/phy/amlogic/phy-meson-axg-pcie.c
@@ -0,0 +1,124 @@
+// 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	*phy;
+};
+
+struct meson_pcie_phy {
+	const struct meson_pcie_phy_data	*data;
+	struct meson_pcie_reset	reset;
+	void __iomem	*phy_base;
+};
+
+#define MESON_PCIE_PHY_POWERUP 0x1c
+
+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(MESON_PCIE_PHY_POWERUP, mphy->phy_base);
+	reset_control_assert(mrst->phy);
+	udelay(400);
+	reset_control_deassert(mrst->phy);
+	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->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);
+	}
+
+	reset_control_deassert(mrst->phy);
+
+	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 v2 2/2] PCI: meson: add the Amlogic Meson PCIe phy driver

[Jerome Brunet]

On Fri, 2018-08-24 at 15:33 +0800, Hanjie Lin wrote:
> +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->phy = devm_reset_control_get_shared(dev, "phy");

I thought you said there was only phy on this platform.
If that's the case, what is this reset shared with ?

> +       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);
> +       }
> +
> +       reset_control_deassert(mrst->phy);

Is it really necessary before init() is called by the consumer ?

> +
> +       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",
> +       }
> +};



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Hanjie Lin]

On 2018/8/29 23:57, Jerome Brunet wrote:
> On Fri, 2018-08-24 at 15:33 +0800, Hanjie Lin wrote:
>> +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->phy = devm_reset_control_get_shared(dev, "phy");
> 
> I thought you said there was only phy on this platform.
> If that's the case, what is this reset shared with ?

Amlogic axg soc includes two pcie controllers and they share the same pcie phy.
Because of two pcie controllers, meson_pcie_phy_probe() will be called two times.
So, the phy reset must be shared.

> 
>> +       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);
>> +       }
>> +
>> +       reset_control_deassert(mrst->phy);
> 
> Is it really necessary before init() is called by the consumer ?
> 

For shared reset we must deassert first before do assert,
currently if we don't do this deassert, we will get the below warning when
controller driver do the share reset.

int reset_control_assert(struct reset_control *rstc)
{
...
	if (rstc->shared) {
		if (WARN_ON(atomic_read(&rstc->deassert_count) == 0))




>> +
>> +       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",
>> +       }
>> +};
> 
> 
> .
> 

Thanks for the review.

As described during the discussion [0], we consider it's too overkill to
have a dedicated phy driver which only process reset line. So we will abandon phy driver 
and integrate phy reset into the controller driver int the next version. 

[0] https://lkml.kernel.org/r/1535096165-45827-1-git-send-email-hanjie.lin@amlogic.

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

[Jerome Brunet]

On Thu, 2018-08-30 at 16:02 +0800, Hanjie Lin wrote:
> > I thought you said there was only phy on this platform.
> > If that's the case, what is this reset shared with ?
> 
> Amlogic axg soc includes two pcie controllers and they share the same pcie phy.
> Because of two pcie controllers, meson_pcie_phy_probe() will be called two times.
> So, the phy reset must be shared.

You are abusing the API then.
The phy should have exclusive control of its *own* reset line, not other device
should claim this reset. 
I should then manage the fact that it may have more than one consumer.