2019-12-24 17:33:28

by Remi Pommarel

[permalink] [raw]
Subject: [PATCH v3 0/5] PCI: amlogic: Make PCIe working reliably on AXG platforms

PCIe device probing failures have been seen on AXG platforms and were due
to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit in
MIPI's PHY registers solved the problem. This bit appears to control band
gap reference.

As discussed here [1] one of these shared MIPI/PCIE PHY register bits was
mistakenly implemented in the clock driver as CLKID_MIPI_ENABLE. This adds
a PHY driver to control this bit through syscon subsystem instead, as well
as setting the band gap one in order to get reliable PCIE communication.

While at it adding this PHY make AXG code close to G12A one thus allowing
to remove some specific platform handling in pci-meson driver.

Please note that CLKID_MIPI_ENABLE removable will be done in a different
serie.

Changes since v2:
- Remove shared MIPI/PCIE device driver and use syscon to access register
in PCIE only driver instead
- Include devicetree documentation

Changes sinve v1:
- Move HHI_MIPI_CNTL0 bit control in its own PHY driver
- Add a PHY driver for PCIE_PHY registers
- Modify pci-meson.c to make use of both PHYs and remove specific
handling for AXG and G12A

[1] https://lkml.org/lkml/2019/12/16/119

Remi Pommarel (5):
phy: amlogic: Add Amlogic AXG PCIE PHY Driver
PCI: amlogic: Use AXG PCIE PHY
arm64: dts: meson-axg: Add PCIE PHY node
dt-bindings: PCI: meson: Update PCIE bindings documentation
dt-bindings: Add AXG PCIE PHY bindings

.../bindings/pci/amlogic,meson-pcie.txt | 22 +--
.../bindings/phy/amlogic,meson-axg-pcie.yaml | 51 +++++
arch/arm64/boot/dts/amlogic/meson-axg.dtsi | 9 +
drivers/pci/controller/dwc/pci-meson.c | 116 ++---------
drivers/phy/amlogic/Kconfig | 11 ++
drivers/phy/amlogic/Makefile | 1 +
drivers/phy/amlogic/phy-meson-axg-pcie.c | 185 ++++++++++++++++++
7 files changed, 287 insertions(+), 108 deletions(-)
create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
create mode 100644 drivers/phy/amlogic/phy-meson-axg-pcie.c

--
2.24.0


2019-12-24 17:33:28

by Remi Pommarel

[permalink] [raw]
Subject: [PATCH v3 1/5] phy: amlogic: Add Amlogic AXG PCIE PHY Driver

This adds support for the PCI PHY found in the Amlogic AXG SoC Family.

This PHY configures the PCIE_PHY registers as well as the HHI_MIPI_CNTL0
through syscon subsystem. This would allow to remove the mipi_enable
clock gating logic which was mistakenly added in the clock subsystem.

It also set a non documented band gap bit in those registers that makes
PCIE clock signal generation on AXG platforms reliable.

Signed-off-by: Remi Pommarel <[email protected]>
---
drivers/phy/amlogic/Kconfig | 11 ++
drivers/phy/amlogic/Makefile | 1 +
drivers/phy/amlogic/phy-meson-axg-pcie.c | 185 +++++++++++++++++++++++
3 files changed, 197 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 af774ac2b934..a77848501d43 100644
--- a/drivers/phy/amlogic/Kconfig
+++ b/drivers/phy/amlogic/Kconfig
@@ -59,3 +59,14 @@ config PHY_MESON_G12A_USB3_PCIE
Enable this to support the Meson USB3 + PCIE Combo PHY found
in Meson G12A SoCs.
If unsure, say N.
+
+config PHY_MESON_AXG_PCIE
+ tristate "Meson AXG PCIE PHY driver"
+ default ARCH_MESON
+ depends on OF && (ARCH_MESON || COMPILE_TEST)
+ select GENERIC_PHY
+ select MFD_SYSCON
+ select REGMAP_MMIO
+ help
+ Enable this to support the Meson PCIE PHY found in AXG SoCs.
+ If unsure, say N.
diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
index 11d1c42ac2be..0772e82fa1f8 100644
--- a/drivers/phy/amlogic/Makefile
+++ b/drivers/phy/amlogic/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_PHY_MESON_GXL_USB2) += phy-meson-gxl-usb2.o
obj-$(CONFIG_PHY_MESON_G12A_USB2) += phy-meson-g12a-usb2.o
obj-$(CONFIG_PHY_MESON_GXL_USB3) += phy-meson-gxl-usb3.o
obj-$(CONFIG_PHY_MESON_G12A_USB3_PCIE) += phy-meson-g12a-usb3-pcie.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 000000000000..06b5443fe3b3
--- /dev/null
+++ b/drivers/phy/amlogic/phy-meson-axg-pcie.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Amlogic AXG PCIE PHY driver
+ *
+ * Copyright (C) 2019 Remi Pommarel <[email protected]>
+ */
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <linux/bitfield.h>
+#include <dt-bindings/phy/phy.h>
+
+#define HHI_MIPI_CNTL0 0x00
+#define HHI_MIPI_CNTL0_ENABLE BIT(29)
+#define HHI_MIPI_CNTL0_BANDGAP BIT(26)
+
+#define MESON_PCIE_REG0 0x00
+#define MESON_PCIE_COMMON_CLK BIT(4)
+#define MESON_PCIE_PORT_SEL GENMASK(3, 2)
+#define MESON_PCIE_CLK BIT(1)
+#define MESON_PCIE_POWERDOWN BIT(0)
+
+#define MESON_PCIE_TWO_X1 FIELD_PREP(MESON_PCIE_PORT_SEL, 0x3)
+#define MESON_PCIE_COMMON_REF_CLK FIELD_PREP(MESON_PCIE_COMMON_CLK, 0x1)
+#define MESON_PCIE_PHY_INIT (MESON_PCIE_TWO_X1 | \
+ MESON_PCIE_COMMON_REF_CLK)
+#define MESON_PCIE_RESET_DELAY 500
+
+struct phy_axg_pcie_priv {
+ struct phy *phy;
+ struct regmap *regmap;
+ struct regmap *regmap_hhi;
+ struct reset_control *reset;
+};
+
+static const struct regmap_config phy_axg_pcie_regmap_conf = {
+ .reg_bits = 8,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register = MESON_PCIE_REG0,
+};
+
+static int phy_axg_pcie_power_on(struct phy *phy)
+{
+ struct phy_axg_pcie_priv *priv = phy_get_drvdata(phy);
+
+ regmap_update_bits(priv->regmap, MESON_PCIE_REG0,
+ MESON_PCIE_POWERDOWN, 0);
+
+ regmap_update_bits(priv->regmap_hhi, HHI_MIPI_CNTL0,
+ HHI_MIPI_CNTL0_BANDGAP, HHI_MIPI_CNTL0_BANDGAP);
+
+ regmap_update_bits(priv->regmap_hhi, HHI_MIPI_CNTL0,
+ HHI_MIPI_CNTL0_ENABLE, HHI_MIPI_CNTL0_ENABLE);
+ return 0;
+}
+
+static int phy_axg_pcie_power_off(struct phy *phy)
+{
+ struct phy_axg_pcie_priv *priv = phy_get_drvdata(phy);
+
+ regmap_update_bits(priv->regmap_hhi, HHI_MIPI_CNTL0,
+ HHI_MIPI_CNTL0_ENABLE, 0);
+
+ regmap_update_bits(priv->regmap_hhi, HHI_MIPI_CNTL0,
+ HHI_MIPI_CNTL0_BANDGAP, 0);
+
+ regmap_update_bits(priv->regmap, MESON_PCIE_REG0,
+ MESON_PCIE_POWERDOWN, 1);
+ return 0;
+}
+
+static int phy_axg_pcie_init(struct phy *phy)
+{
+ struct phy_axg_pcie_priv *priv = phy_get_drvdata(phy);
+
+ regmap_write(priv->regmap, MESON_PCIE_REG0, MESON_PCIE_PHY_INIT);
+ return reset_control_reset(priv->reset);
+}
+
+static int phy_axg_pcie_exit(struct phy *phy)
+{
+ struct phy_axg_pcie_priv *priv = phy_get_drvdata(phy);
+
+ return reset_control_reset(priv->reset);
+}
+
+static int phy_axg_pcie_reset(struct phy *phy)
+{
+ struct phy_axg_pcie_priv *priv = phy_get_drvdata(phy);
+ int ret = 0;
+
+ ret = reset_control_assert(priv->reset);
+ if (ret != 0)
+ goto out;
+ udelay(MESON_PCIE_RESET_DELAY);
+
+ ret = reset_control_deassert(priv->reset);
+ if (ret != 0)
+ goto out;
+ udelay(MESON_PCIE_RESET_DELAY);
+
+out:
+ return ret;
+}
+
+static const struct phy_ops phy_axg_pcie_ops = {
+ .init = phy_axg_pcie_init,
+ .exit = phy_axg_pcie_exit,
+ .power_on = phy_axg_pcie_power_on,
+ .power_off = phy_axg_pcie_power_off,
+ .reset = phy_axg_pcie_reset,
+ .owner = THIS_MODULE,
+};
+
+static int phy_axg_pcie_probe(struct platform_device *pdev)
+{
+ struct phy_provider *pphy;
+ struct device *dev = &pdev->dev;
+ struct phy_axg_pcie_priv *priv;
+ struct device_node *np = dev->of_node;
+ struct resource *res;
+ void __iomem *base;
+ int ret;
+
+ priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->phy = devm_phy_create(dev, np, &phy_axg_pcie_ops);
+ if (IS_ERR(priv->phy)) {
+ ret = PTR_ERR(priv->phy);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to create PHY\n");
+ return ret;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ priv->regmap = devm_regmap_init_mmio(dev, base,
+ &phy_axg_pcie_regmap_conf);
+ if (IS_ERR(priv->regmap))
+ return PTR_ERR(priv->regmap);
+
+ priv->regmap_hhi = syscon_regmap_lookup_by_phandle(np, "aml,hhi-gpr");
+ if (IS_ERR(priv->regmap_hhi))
+ return PTR_ERR(priv->regmap_hhi);
+
+ priv->reset = devm_reset_control_array_get(dev, false, false);
+ if (IS_ERR(priv->reset))
+ return PTR_ERR(priv->reset);
+
+ phy_set_drvdata(priv->phy, priv);
+ dev_set_drvdata(dev, priv);
+ pphy = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+ return PTR_ERR_OR_ZERO(pphy);
+}
+
+static const struct of_device_id phy_axg_pcie_of_match[] = {
+ {
+ .compatible = "amlogic,axg-pcie-phy",
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(of, phy_axg_pcie_of_match);
+
+static struct platform_driver phy_axg_pcie_driver = {
+ .probe = phy_axg_pcie_probe,
+ .driver = {
+ .name = "phy-axg-pcie",
+ .of_match_table = phy_axg_pcie_of_match,
+ },
+};
+module_platform_driver(phy_axg_pcie_driver);
+
+MODULE_AUTHOR("Remi Pommarel <[email protected]>");
+MODULE_DESCRIPTION("Amlogic AXG PCIE PHY driver");
+MODULE_LICENSE("GPL v2");
--
2.24.0

2019-12-24 17:33:40

by Remi Pommarel

[permalink] [raw]
Subject: [PATCH v3 3/5] arm64: dts: meson-axg: Add PCIE PHY node

Enable PCIE PHY node to make PCIE reliable on AXG SoC.

Signed-off-by: Remi Pommarel <[email protected]>
---
arch/arm64/boot/dts/amlogic/meson-axg.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
index 04803c3bccfa..e679ef26ab79 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -1356,6 +1356,15 @@ tdmout_c: audio-controller@580 {
};
};

+ pcie_phy: pcie-phy@ff644000 {
+ compatible = "amlogic,axg-pcie-phy";
+ reg = <0x0 0xff644000 0x0 0x2000>;
+ aml,hhi-gpr = <&sysctrl>;
+ resets = <&reset RESET_PCIE_PHY>;
+ reset-names = "phy";
+ #phy-cells = <0>;
+ };
+
aobus: bus@ff800000 {
compatible = "simple-bus";
reg = <0x0 0xff800000 0x0 0x100000>;
--
2.24.0

2019-12-24 17:33:49

by Remi Pommarel

[permalink] [raw]
Subject: [PATCH v3 4/5] dt-bindings: PCI: meson: Update PCIE bindings documentation

Now that a new PHYs has been introduced for AXG SoC family, update
dt bindings documentation.

Signed-off-by: Remi Pommarel <[email protected]>
---
.../bindings/pci/amlogic,meson-pcie.txt | 22 ++++++++-----------
1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
index 84fdc422792e..b6acbe694ffb 100644
--- a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
@@ -18,7 +18,6 @@ Required properties:
- reg-names: Must be
- "elbi" External local bus interface registers
- "cfg" Meson specific registers
- - "phy" Meson PCIE PHY registers for AXG SoC Family
- "config" PCIe configuration space
- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
- clocks: Must contain an entry for each entry in clock-names.
@@ -26,13 +25,13 @@ Required properties:
- "pclk" PCIe GEN 100M PLL clock
- "port" PCIe_x(A or B) RC clock gate
- "general" PCIe Phy clock
- - "mipi" PCIe_x(A or B) 100M ref clock gate for AXG SoC Family
- resets: phandle to the reset lines.
-- reset-names: must contain "phy" "port" and "apb"
- - "phy" Share PHY reset for AXG SoC Family
+- reset-names: must contain "port" and "apb"
- "port" Port A or B reset
- "apb" Share APB reset
-- phys: should contain a phandle to the shared phy for G12A SoC Family
+- phys: should contain a phandle to the PCIE phy
+- phy-names: must contain "pcie"
+
- device_type:
should be "pci". As specified in designware-pcie.txt

@@ -43,9 +42,8 @@ Example configuration:
compatible = "amlogic,axg-pcie", "snps,dw-pcie";
reg = <0x0 0xf9800000 0x0 0x400000
0x0 0xff646000 0x0 0x2000
- 0x0 0xff644000 0x0 0x2000
0x0 0xf9f00000 0x0 0x100000>;
- reg-names = "elbi", "cfg", "phy", "config";
+ reg-names = "elbi", "cfg", "config";
reset-gpios = <&gpio GPIOX_19 GPIO_ACTIVE_HIGH>;
interrupts = <GIC_SPI 177 IRQ_TYPE_EDGE_RISING>;
#interrupt-cells = <1>;
@@ -58,17 +56,15 @@ Example configuration:
ranges = <0x82000000 0 0 0x0 0xf9c00000 0 0x00300000>;

clocks = <&clkc CLKID_USB
- &clkc CLKID_MIPI_ENABLE
&clkc CLKID_PCIE_A
&clkc CLKID_PCIE_CML_EN0>;
clock-names = "general",
- "mipi",
"pclk",
"port";
- resets = <&reset RESET_PCIE_PHY>,
- <&reset RESET_PCIE_A>,
+ resets = <&reset RESET_PCIE_A>,
<&reset RESET_PCIE_APB>;
- reset-names = "phy",
- "port",
+ reset-names = "port",
"apb";
+ phys = <&pcie_phy>;
+ phy-names = "pcie";
};
--
2.24.0

2019-12-24 17:35:01

by Remi Pommarel

[permalink] [raw]
Subject: [PATCH v3 5/5] dt-bindings: Add AXG PCIE PHY bindings

Add documentation for PCIE PHYs found in AXG SoCs.

Signed-off-by: Remi Pommarel <[email protected]>
---
.../bindings/phy/amlogic,meson-axg-pcie.yaml | 51 +++++++++++++++++++
1 file changed, 51 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml

diff --git a/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
new file mode 100644
index 000000000000..c622a1b38ffc
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019 BayLibre, SAS
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/phy/amlogic,meson-axg-pcie.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic AXG PCIE PHY
+
+maintainers:
+ - Remi Pommarel <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - amlogic,axg-pcie-phy
+
+ reg:
+ maxItems: 1
+
+ aml,hhi-gpr:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+ reset-names:
+ items:
+ - const: phy
+
+ "#phy-cells":
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - aml,hhi-gpr
+ - resets
+ - reset-names
+ - "#phy-cells"
+
+examples:
+ - |
+ pcie_phy: pcie-phy@ff644000 {
+ compatible = "amlogic,axg-pcie-phy";
+ reg = <0x0 0xff644000 0x0 0x2000>;
+ aml,hhi-gpr = <&sysctrl>;
+ resets = <&reset RESET_PCIE_PHY>;
+ reset-names = "phy";
+ #phy-cells = <0>;
+ };
--
2.24.0

2019-12-25 18:55:22

by Remi Pommarel

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] dt-bindings: Add AXG PCIE PHY bindings

On Tue, Dec 24, 2019 at 06:39:42PM +0100, Remi Pommarel wrote:
> Add documentation for PCIE PHYs found in AXG SoCs.
>
> Signed-off-by: Remi Pommarel <[email protected]>
> ---
> .../bindings/phy/amlogic,meson-axg-pcie.yaml | 51 +++++++++++++++++++
> 1 file changed, 51 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
> new file mode 100644
> index 000000000000..c622a1b38ffc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 BayLibre, SAS
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/phy/amlogic,meson-axg-pcie.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Amlogic AXG PCIE PHY
> +
> +maintainers:
> + - Remi Pommarel <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - amlogic,axg-pcie-phy
> +
> + reg:
> + maxItems: 1
> +
> + aml,hhi-gpr:
> + maxItems: 1

My bad, I didn't know about devicetree schemas and their verification,
I will fix the missing $ref: '/schemas/types.yaml#/definitions/phandle'
here, add a description and add the missing include for reset in the
example below in v4 along with upcoming comments on the other patches.

Sorry about that.

> +
> + resets:
> + maxItems: 1
> +
> + reset-names:
> + items:
> + - const: phy
> +
> + "#phy-cells":
> + const: 0
> +
> +required:
> + - compatible
> + - reg
> + - aml,hhi-gpr
> + - resets
> + - reset-names
> + - "#phy-cells"
> +
> +examples:
> + - |
> + pcie_phy: pcie-phy@ff644000 {
> + compatible = "amlogic,axg-pcie-phy";
> + reg = <0x0 0xff644000 0x0 0x2000>;
> + aml,hhi-gpr = <&sysctrl>;
> + resets = <&reset RESET_PCIE_PHY>;
> + reset-names = "phy";
> + #phy-cells = <0>;
> + };
> --
> 2.24.0
>

2019-12-26 09:58:35

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH v3 0/5] PCI: amlogic: Make PCIe working reliably on AXG platforms


On Tue 24 Dec 2019 at 18:39, Remi Pommarel <[email protected]> wrote:

> PCIe device probing failures have been seen on AXG platforms and were due
> to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit in
> MIPI's PHY registers solved the problem. This bit appears to control band
> gap reference.
>
> As discussed here [1] one of these shared MIPI/PCIE PHY register bits was
> mistakenly implemented in the clock driver as CLKID_MIPI_ENABLE. This adds
> a PHY driver to control this bit through syscon subsystem instead, as well
> as setting the band gap one in order to get reliable PCIE communication.
>
> While at it adding this PHY make AXG code close to G12A one thus allowing
> to remove some specific platform handling in pci-meson driver.
>
> Please note that CLKID_MIPI_ENABLE removable will be done in a different
> serie.
>
> Changes since v2:
> - Remove shared MIPI/PCIE device driver and use syscon to access register
> in PCIE only driver instead
> - Include devicetree documentation
>
> Changes sinve v1:
> - Move HHI_MIPI_CNTL0 bit control in its own PHY driver
> - Add a PHY driver for PCIE_PHY registers
> - Modify pci-meson.c to make use of both PHYs and remove specific
> handling for AXG and G12A
>
> [1] https://lkml.org/lkml/2019/12/16/119
>
> Remi Pommarel (5):
> phy: amlogic: Add Amlogic AXG PCIE PHY Driver
> PCI: amlogic: Use AXG PCIE PHY
> arm64: dts: meson-axg: Add PCIE PHY node
> dt-bindings: PCI: meson: Update PCIE bindings documentation
> dt-bindings: Add AXG PCIE PHY bindings

Hi Remi,

Usually, you should put the dt documentation first in the series.
This way the properties are documented before being used

>
> .../bindings/pci/amlogic,meson-pcie.txt | 22 +--
> .../bindings/phy/amlogic,meson-axg-pcie.yaml | 51 +++++
> arch/arm64/boot/dts/amlogic/meson-axg.dtsi | 9 +
> drivers/pci/controller/dwc/pci-meson.c | 116 ++---------
> drivers/phy/amlogic/Kconfig | 11 ++
> drivers/phy/amlogic/Makefile | 1 +
> drivers/phy/amlogic/phy-meson-axg-pcie.c | 185 ++++++++++++++++++
> 7 files changed, 287 insertions(+), 108 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
> create mode 100644 drivers/phy/amlogic/phy-meson-axg-pcie.c

2019-12-26 20:12:10

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] dt-bindings: Add AXG PCIE PHY bindings

On Tue, Dec 24, 2019 at 6:32 PM Remi Pommarel <[email protected]> wrote:
>
> Add documentation for PCIE PHYs found in AXG SoCs.
>
> Signed-off-by: Remi Pommarel <[email protected]>
> ---
> .../bindings/phy/amlogic,meson-axg-pcie.yaml | 51 +++++++++++++++++++
> 1 file changed, 51 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
> new file mode 100644
> index 000000000000..c622a1b38ffc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 BayLibre, SAS
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/phy/amlogic,meson-axg-pcie.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Amlogic AXG PCIE PHY
> +
> +maintainers:
> + - Remi Pommarel <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - amlogic,axg-pcie-phy
> +
> + reg:
> + maxItems: 1
> +
> + aml,hhi-gpr:
> + maxItems: 1
nit-pick (as I didn't have time to review the whole series yet):
we have at least two other IP blocks that need this. they use
"amlogic,hhi-sysctrl" for the property name


Martin

2020-01-04 00:23:13

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v3 4/5] dt-bindings: PCI: meson: Update PCIE bindings documentation

On Tue, Dec 24, 2019 at 06:39:41PM +0100, Remi Pommarel wrote:
> Now that a new PHYs has been introduced for AXG SoC family, update
> dt bindings documentation.

This breaks compatibility. If that's okay, say so and why it is.

If only someone had said putting the phy here in the first place was
wrong:

https://lore.kernel.org/linux-amlogic/20180829004122.GA25928@bogus/

>
> Signed-off-by: Remi Pommarel <[email protected]>
> ---
> .../bindings/pci/amlogic,meson-pcie.txt | 22 ++++++++-----------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> index 84fdc422792e..b6acbe694ffb 100644
> --- a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> @@ -18,7 +18,6 @@ Required properties:
> - reg-names: Must be
> - "elbi" External local bus interface registers
> - "cfg" Meson specific registers
> - - "phy" Meson PCIE PHY registers for AXG SoC Family
> - "config" PCIe configuration space
> - reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
> - clocks: Must contain an entry for each entry in clock-names.
> @@ -26,13 +25,13 @@ Required properties:
> - "pclk" PCIe GEN 100M PLL clock
> - "port" PCIe_x(A or B) RC clock gate
> - "general" PCIe Phy clock
> - - "mipi" PCIe_x(A or B) 100M ref clock gate for AXG SoC Family
> - resets: phandle to the reset lines.
> -- reset-names: must contain "phy" "port" and "apb"
> - - "phy" Share PHY reset for AXG SoC Family
> +- reset-names: must contain "port" and "apb"
> - "port" Port A or B reset
> - "apb" Share APB reset
> -- phys: should contain a phandle to the shared phy for G12A SoC Family
> +- phys: should contain a phandle to the PCIE phy
> +- phy-names: must contain "pcie"
> +
> - device_type:
> should be "pci". As specified in designware-pcie.txt
>
> @@ -43,9 +42,8 @@ Example configuration:
> compatible = "amlogic,axg-pcie", "snps,dw-pcie";
> reg = <0x0 0xf9800000 0x0 0x400000
> 0x0 0xff646000 0x0 0x2000
> - 0x0 0xff644000 0x0 0x2000
> 0x0 0xf9f00000 0x0 0x100000>;
> - reg-names = "elbi", "cfg", "phy", "config";
> + reg-names = "elbi", "cfg", "config";
> reset-gpios = <&gpio GPIOX_19 GPIO_ACTIVE_HIGH>;
> interrupts = <GIC_SPI 177 IRQ_TYPE_EDGE_RISING>;
> #interrupt-cells = <1>;
> @@ -58,17 +56,15 @@ Example configuration:
> ranges = <0x82000000 0 0 0x0 0xf9c00000 0 0x00300000>;
>
> clocks = <&clkc CLKID_USB
> - &clkc CLKID_MIPI_ENABLE
> &clkc CLKID_PCIE_A
> &clkc CLKID_PCIE_CML_EN0>;
> clock-names = "general",
> - "mipi",
> "pclk",
> "port";
> - resets = <&reset RESET_PCIE_PHY>,
> - <&reset RESET_PCIE_A>,
> + resets = <&reset RESET_PCIE_A>,
> <&reset RESET_PCIE_APB>;
> - reset-names = "phy",
> - "port",
> + reset-names = "port",
> "apb";
> + phys = <&pcie_phy>;
> + phy-names = "pcie";
> };
> --
> 2.24.0
>

2020-01-04 00:25:25

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] dt-bindings: Add AXG PCIE PHY bindings

On Tue, Dec 24, 2019 at 06:39:42PM +0100, Remi Pommarel wrote:
> Add documentation for PCIE PHYs found in AXG SoCs.
>
> Signed-off-by: Remi Pommarel <[email protected]>
> ---
> .../bindings/phy/amlogic,meson-axg-pcie.yaml | 51 +++++++++++++++++++
> 1 file changed, 51 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
> new file mode 100644
> index 000000000000..c622a1b38ffc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-pcie.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 BayLibre, SAS
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/phy/amlogic,meson-axg-pcie.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Amlogic AXG PCIE PHY
> +
> +maintainers:
> + - Remi Pommarel <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - amlogic,axg-pcie-phy

Do you expect another compatible? If not, use 'const' instead.

> +
> + reg:
> + maxItems: 1
> +
> + aml,hhi-gpr:
> + maxItems: 1
> +
> + resets:
> + maxItems: 1
> +
> + reset-names:
> + items:
> + - const: phy

You don't need *-names when there's only one entry.

> +
> + "#phy-cells":
> + const: 0
> +
> +required:
> + - compatible
> + - reg
> + - aml,hhi-gpr
> + - resets
> + - reset-names
> + - "#phy-cells"
> +
> +examples:
> + - |
> + pcie_phy: pcie-phy@ff644000 {
> + compatible = "amlogic,axg-pcie-phy";
> + reg = <0x0 0xff644000 0x0 0x2000>;
> + aml,hhi-gpr = <&sysctrl>;
> + resets = <&reset RESET_PCIE_PHY>;
> + reset-names = "phy";
> + #phy-cells = <0>;
> + };
> --
> 2.24.0
>