[PATCH v2 0/2] phy: mediatek: Add PCIe PHY driver

[PATCH v2 0/2] phy: mediatek: Add PCIe PHY driver

[Jianjun Wang]

These series patches add support for PCIe PHY driver on MediaTek chipsets.

Changes in v2:
1. Add specific compatible name;
2. Read NVMEM data at probe time;
3. Fix typos.

Jianjun Wang (2):
  dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY
  phy: mediatek: Add PCIe PHY driver

 .../bindings/phy/mediatek,pcie-phy.yaml       |  75 ++++++
 drivers/phy/mediatek/Kconfig                  |  11 +
 drivers/phy/mediatek/Makefile                 |   1 +
 drivers/phy/mediatek/phy-mtk-pcie.c           | 246 ++++++++++++++++++
 4 files changed, 333 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
 create mode 100644 drivers/phy/mediatek/phy-mtk-pcie.c

-- 
2.18.0

[PATCH v2 1/2] dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY

[Jianjun Wang]

Add YAML schema documentation for PCIe PHY on MediaTek chipsets.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 .../bindings/phy/mediatek,pcie-phy.yaml       | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
new file mode 100644
index 000000000000..868bf976568b
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/mediatek,pcie-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek PCIe PHY
+
+maintainers:
+  - Jianjun Wang <jianjun.wang@mediatek.com>
+
+description: |
+  The PCIe PHY supports physical layer functionality for PCIe Gen3 port.
+
+properties:
+  compatible:
+    const: mediatek,mt8195-pcie-phy
+
+  reg:
+    maxItems: 1
+
+  reg-names:
+    items:
+      - const: sif
+
+  "#phy-cells":
+    const: 0
+
+  nvmem-cells:
+    description:
+      Phandles to nvmem cell that contains the efuse data, if unspecified,
+      default value is used.
+
+  nvmem-cell-names:
+    items:
+      - const: glb_intr
+      - const: tx_ln0_pmos
+      - const: tx_ln0_nmos
+      - const: rx_ln0
+      - const: tx_ln1_pmos
+      - const: tx_ln1_nmos
+      - const: rx_ln1
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - "#phy-cells"
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  - |
+    phy@11e80000 {
+        compatible = "mediatek,mt8195-pcie-phy";
+        #phy-cells = <0>;
+        reg = <0x11e80000 0x10000>;
+        reg-names = "sif";
+        nvmem-cells = <&pciephy_glb_intr>,
+                      <&pciephy_tx_ln0_pmos>,
+                      <&pciephy_tx_ln0_nmos>,
+                      <&pciephy_rx_ln0>,
+                      <&pciephy_tx_ln1_pmos>,
+                      <&pciephy_tx_ln1_nmos>,
+                      <&pciephy_rx_ln1>;
+        nvmem-cell-names = "glb_intr", "tx_ln0_pmos",
+                           "tx_ln0_nmos", "rx_ln0",
+                           "tx_ln1_pmos", "tx_ln1_nmos",
+                           "rx_ln1";
+        power-domains = <&spm 2>;
+    };
-- 
2.18.0

[PATCH v2 2/2] phy: mediatek: Add PCIe PHY driver

[Jianjun Wang]

Add PCIe GEN3 PHY driver support on MediaTek chipsets.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 drivers/phy/mediatek/Kconfig        |  11 ++
 drivers/phy/mediatek/Makefile       |   1 +
 drivers/phy/mediatek/phy-mtk-pcie.c | 246 ++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/phy/mediatek/phy-mtk-pcie.c

diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
index 55f8e6c048ab..387ed1b3f2cc 100644
--- a/drivers/phy/mediatek/Kconfig
+++ b/drivers/phy/mediatek/Kconfig
@@ -55,3 +55,14 @@ config PHY_MTK_MIPI_DSI
 	select GENERIC_PHY
 	help
 	  Support MIPI DSI for Mediatek SoCs.
+
+config PHY_MTK_PCIE
+	tristate "MediaTek PCIe-PHY Driver"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+	help
+	  Say 'Y' here to add support for MediaTek PCIe PHY driver.
+	  This driver create the basic PHY instance and provides initialize
+	  callback for PCIe GEN3 port, it supports software efuse
+	  initialization.
diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
index ace660fbed3a..788c13147f63 100644
--- a/drivers/phy/mediatek/Makefile
+++ b/drivers/phy/mediatek/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_PHY_MTK_TPHY)		+= phy-mtk-tphy.o
 obj-$(CONFIG_PHY_MTK_UFS)		+= phy-mtk-ufs.o
 obj-$(CONFIG_PHY_MTK_XSPHY)		+= phy-mtk-xsphy.o
+obj-$(CONFIG_PHY_MTK_PCIE)		+= phy-mtk-pcie.o
 
 phy-mtk-hdmi-drv-y			:= phy-mtk-hdmi.o
 phy-mtk-hdmi-drv-y			+= phy-mtk-hdmi-mt2701.o
diff --git a/drivers/phy/mediatek/phy-mtk-pcie.c b/drivers/phy/mediatek/phy-mtk-pcie.c
new file mode 100644
index 000000000000..0f5d7c7e2b7e
--- /dev/null
+++ b/drivers/phy/mediatek/phy-mtk-pcie.c
@@ -0,0 +1,246 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 MediaTek Inc.
+ * Author: Jianjun Wang <jianjun.wang@mediatek.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/compiler_types.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "phy-mtk-io.h"
+
+#define PEXTP_ANA_GLB_00_REG		0x9000
+#define PEXTP_ANA_LN0_TRX_REG		0xa000
+#define PEXTP_ANA_TX_OFFSET		0x04
+#define PEXTP_ANA_RX_OFFSET		0x3c
+#define PEXTP_ANA_LANE_OFFSET		0x100
+
+/* PEXTP_GLB_00_REG[28:24] Internal Resistor Selection of TX Bias Current */
+#define EFUSE_GLB_INTR_SEL		GENMASK(28, 24)
+#define EFUSE_GLB_INTR_VAL(x)		((0x1f & (x)) << 24)
+
+/* PEXTP_ANA_LN_RX_REG[3:0] RX impedance selection */
+#define EFUSE_LN_RX_SEL			GENMASK(3, 0)
+#define EFUSE_LN_RX_VAL(x)		(0xf & (x))
+
+/* PEXTP_ANA_LN_TX_REG[5:2] TX PMOS impedance selection */
+#define EFUSE_LN_TX_PMOS_SEL		GENMASK(5, 2)
+#define EFUSE_LN_TX_PMOS_VAL(x)		((0xf & (x)) << 2)
+
+/* PEXTP_ANA_LN_TX_REG[11:8] TX NMOS impedance selection */
+#define EFUSE_LN_TX_NMOS_SEL		GENMASK(11, 8)
+#define EFUSE_LN_TX_NMOS_VAL(x)		((0xf & (x)) << 8)
+
+/* Efuse data for each lane */
+struct mtk_pcie_lane_efuse {
+	u32 tx_pmos;
+	u32 tx_nmos;
+	u32 rx_data;
+	bool lane_efuse_supported;
+};
+
+struct mtk_pcie_phy {
+	struct device *dev;
+	struct phy *phy;
+	void __iomem *sif_base;
+
+	/*
+	 * Support software efuse initialization,
+	 * currently we only support 2 lane in maximum.
+	 */
+	bool sw_efuse_supported;
+	u32 efuse_glb_intr;
+	struct mtk_pcie_lane_efuse efuse[2];
+};
+
+static void mtk_pcie_efuse_set_lane(struct mtk_pcie_phy *pcie_phy,
+				    unsigned int lane)
+{
+	struct mtk_pcie_lane_efuse *data;
+	void __iomem *addr;
+
+	if (lane >= ARRAY_SIZE(pcie_phy->efuse)) {
+		dev_err(pcie_phy->dev,
+			"Requested lane number %d exceeds maximum %ld\n",
+			lane, ARRAY_SIZE(pcie_phy->efuse) - 1);
+		return;
+	}
+
+	data = &pcie_phy->efuse[lane];
+	if (!data->lane_efuse_supported)
+		return;
+
+	addr = pcie_phy->sif_base + PEXTP_ANA_LN0_TRX_REG +
+	       lane * PEXTP_ANA_LANE_OFFSET;
+
+	mtk_phy_update_bits(addr + PEXTP_ANA_TX_OFFSET, EFUSE_LN_TX_PMOS_SEL,
+			    EFUSE_LN_TX_PMOS_VAL(data->tx_pmos));
+
+	mtk_phy_update_bits(addr + PEXTP_ANA_TX_OFFSET, EFUSE_LN_TX_NMOS_SEL,
+			    EFUSE_LN_TX_NMOS_VAL(data->tx_nmos));
+
+	mtk_phy_update_bits(addr + PEXTP_ANA_RX_OFFSET, EFUSE_LN_RX_SEL,
+			    EFUSE_LN_RX_VAL(data->rx_data));
+}
+
+/**
+ * mtk_pcie_phy_init() - Initialize the phy
+ * @phy: the phy to be initialized
+ *
+ * Initialize the phy by setting the efuse data.
+ * The hardware settings will be reset during suspend, it should be
+ * reinitialized when the consumer calls phy_init() again on resume.
+ */
+static int mtk_pcie_phy_init(struct phy *phy)
+{
+	struct mtk_pcie_phy *pcie_phy = phy_get_drvdata(phy);
+
+	if (!pcie_phy->sw_efuse_supported)
+		return 0;
+
+	/* Set global data */
+	mtk_phy_update_bits(pcie_phy->sif_base + PEXTP_ANA_GLB_00_REG,
+			    EFUSE_GLB_INTR_SEL,
+			    EFUSE_GLB_INTR_VAL(pcie_phy->efuse_glb_intr));
+
+	mtk_pcie_efuse_set_lane(pcie_phy, 0);
+
+	mtk_pcie_efuse_set_lane(pcie_phy, 1);
+
+	return 0;
+}
+
+static const struct phy_ops mtk_pcie_phy_ops = {
+	.init	= mtk_pcie_phy_init,
+	.owner	= THIS_MODULE,
+};
+
+static int mtk_pcie_efuse_read_for_lane(struct mtk_pcie_phy *pcie_phy,
+					unsigned int lane)
+{
+	struct device *dev = pcie_phy->dev;
+	struct mtk_pcie_lane_efuse *data;
+	char efuse_id[15];
+	int ret;
+
+	if (lane >= ARRAY_SIZE(pcie_phy->efuse))
+		return dev_err_probe(pcie_phy->dev, -EINVAL,
+				     "Requested lane number %d exceeds maximum %ld\n",
+				     lane, ARRAY_SIZE(pcie_phy->efuse) - 1);
+
+	data = &pcie_phy->efuse[lane];
+
+	snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_pmos", lane);
+	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->tx_pmos);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
+
+	snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_nmos", lane);
+	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->tx_nmos);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
+
+	snprintf(efuse_id, sizeof(efuse_id), "rx_ln%d", lane);
+	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->rx_data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
+
+	if (!(data->tx_pmos || data->tx_nmos || data->rx_data))
+		return dev_err_probe(dev, -EINVAL,
+				     "No efuse data found for lane%d, but dts enable it\n",
+				     lane);
+
+	data->lane_efuse_supported = true;
+
+	return 0;
+}
+
+static int mtk_pcie_read_efuse(struct mtk_pcie_phy *pcie_phy)
+{
+	struct device *dev = pcie_phy->dev;
+	bool nvmem_enabled;
+	int ret;
+
+	nvmem_enabled = device_property_read_bool(dev, "nvmem-cells");
+	if (!nvmem_enabled)
+		return -ENODEV;
+
+	ret = nvmem_cell_read_variable_le_u32(dev, "glb_intr",
+					      &pcie_phy->efuse_glb_intr);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read glb_intr\n");
+
+	pcie_phy->sw_efuse_supported = true;
+
+	ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 0);
+	if (ret)
+		return ret;
+
+	ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 1);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mtk_pcie_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy_provider *provider;
+	struct mtk_pcie_phy *pcie_phy;
+
+	pcie_phy = devm_kzalloc(dev, sizeof(*pcie_phy), GFP_KERNEL);
+	if (!pcie_phy)
+		return -ENOMEM;
+
+	pcie_phy->dev = dev;
+
+	pcie_phy->sif_base = devm_platform_ioremap_resource_byname(pdev, "sif");
+	if (IS_ERR(pcie_phy->sif_base))
+		return dev_err_probe(dev, PTR_ERR(pcie_phy->sif_base),
+				     "Failed to map phy-sif base\n");
+
+	pcie_phy->phy = devm_phy_create(dev, dev->of_node, &mtk_pcie_phy_ops);
+	if (IS_ERR(pcie_phy->phy))
+		return dev_err_probe(dev, PTR_ERR(pcie_phy->phy),
+				     "Failed to create PCIe phy\n");
+
+	/*
+	 * Failed to read the efuse data is not a fatal problem,
+	 * ignore the failure and keep going.
+	 */
+	mtk_pcie_read_efuse(pcie_phy);
+
+	phy_set_drvdata(pcie_phy->phy, pcie_phy);
+
+	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(provider))
+		return dev_err_probe(dev, PTR_ERR(provider),
+				     "PCIe phy probe failed\n");
+
+	return 0;
+}
+
+static const struct of_device_id mtk_pcie_phy_of_match[] = {
+	{ .compatible = "mediatek,mt8195-pcie-phy" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mtk_pcie_phy_of_match);
+
+static struct platform_driver mtk_pcie_phy_driver = {
+	.probe	= mtk_pcie_phy_probe,
+	.driver	= {
+		.name = "mtk-pcie-phy",
+		.of_match_table = mtk_pcie_phy_of_match,
+	},
+};
+module_platform_driver(mtk_pcie_phy_driver);
+
+MODULE_DESCRIPTION("MediaTek PCIe PHY driver");
+MODULE_AUTHOR("Jianjun Wang <jianjun.wang@mediatek.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0

Re: [PATCH v2 2/2] phy: mediatek: Add PCIe PHY driver

[Chen-Yu Tsai]

On Fri, Mar 18, 2022 at 5:54 PM Jianjun Wang <jianjun.wang@mediatek.com> wrote:
>
> Add PCIe GEN3 PHY driver support on MediaTek chipsets.
>
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  drivers/phy/mediatek/Kconfig        |  11 ++
>  drivers/phy/mediatek/Makefile       |   1 +
>  drivers/phy/mediatek/phy-mtk-pcie.c | 246 ++++++++++++++++++++++++++++
>  3 files changed, 258 insertions(+)
>  create mode 100644 drivers/phy/mediatek/phy-mtk-pcie.c
>
> diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
> index 55f8e6c048ab..387ed1b3f2cc 100644
> --- a/drivers/phy/mediatek/Kconfig
> +++ b/drivers/phy/mediatek/Kconfig
> @@ -55,3 +55,14 @@ config PHY_MTK_MIPI_DSI
>         select GENERIC_PHY
>         help
>           Support MIPI DSI for Mediatek SoCs.
> +
> +config PHY_MTK_PCIE
> +       tristate "MediaTek PCIe-PHY Driver"
> +       depends on ARCH_MEDIATEK || COMPILE_TEST
> +       depends on OF
> +       select GENERIC_PHY
> +       help
> +         Say 'Y' here to add support for MediaTek PCIe PHY driver.
> +         This driver create the basic PHY instance and provides initialize
> +         callback for PCIe GEN3 port, it supports software efuse
> +         initialization.
> diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
> index ace660fbed3a..788c13147f63 100644
> --- a/drivers/phy/mediatek/Makefile
> +++ b/drivers/phy/mediatek/Makefile
> @@ -6,6 +6,7 @@
>  obj-$(CONFIG_PHY_MTK_TPHY)             += phy-mtk-tphy.o
>  obj-$(CONFIG_PHY_MTK_UFS)              += phy-mtk-ufs.o
>  obj-$(CONFIG_PHY_MTK_XSPHY)            += phy-mtk-xsphy.o
> +obj-$(CONFIG_PHY_MTK_PCIE)             += phy-mtk-pcie.o
>
>  phy-mtk-hdmi-drv-y                     := phy-mtk-hdmi.o
>  phy-mtk-hdmi-drv-y                     += phy-mtk-hdmi-mt2701.o
> diff --git a/drivers/phy/mediatek/phy-mtk-pcie.c b/drivers/phy/mediatek/phy-mtk-pcie.c
> new file mode 100644
> index 000000000000..0f5d7c7e2b7e
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-pcie.c
> @@ -0,0 +1,246 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Jianjun Wang <jianjun.wang@mediatek.com>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/compiler_types.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "phy-mtk-io.h"
> +
> +#define PEXTP_ANA_GLB_00_REG           0x9000
> +#define PEXTP_ANA_LN0_TRX_REG          0xa000
> +#define PEXTP_ANA_TX_OFFSET            0x04
> +#define PEXTP_ANA_RX_OFFSET            0x3c
> +#define PEXTP_ANA_LANE_OFFSET          0x100
> +
> +/* PEXTP_GLB_00_REG[28:24] Internal Resistor Selection of TX Bias Current */
> +#define EFUSE_GLB_INTR_SEL             GENMASK(28, 24)
> +#define EFUSE_GLB_INTR_VAL(x)          ((0x1f & (x)) << 24)
> +
> +/* PEXTP_ANA_LN_RX_REG[3:0] RX impedance selection */
> +#define EFUSE_LN_RX_SEL                        GENMASK(3, 0)
> +#define EFUSE_LN_RX_VAL(x)             (0xf & (x))
> +
> +/* PEXTP_ANA_LN_TX_REG[5:2] TX PMOS impedance selection */
> +#define EFUSE_LN_TX_PMOS_SEL           GENMASK(5, 2)
> +#define EFUSE_LN_TX_PMOS_VAL(x)                ((0xf & (x)) << 2)
> +
> +/* PEXTP_ANA_LN_TX_REG[11:8] TX NMOS impedance selection */
> +#define EFUSE_LN_TX_NMOS_SEL           GENMASK(11, 8)
> +#define EFUSE_LN_TX_NMOS_VAL(x)                ((0xf & (x)) << 8)
> +
> +/* Efuse data for each lane */
> +struct mtk_pcie_lane_efuse {
> +       u32 tx_pmos;
> +       u32 tx_nmos;
> +       u32 rx_data;
> +       bool lane_efuse_supported;
> +};
> +
> +struct mtk_pcie_phy {
> +       struct device *dev;
> +       struct phy *phy;
> +       void __iomem *sif_base;
> +
> +       /*
> +        * Support software efuse initialization,
> +        * currently we only support 2 lane in maximum.
> +        */
> +       bool sw_efuse_supported;
> +       u32 efuse_glb_intr;
> +       struct mtk_pcie_lane_efuse efuse[2];
> +};
> +
> +static void mtk_pcie_efuse_set_lane(struct mtk_pcie_phy *pcie_phy,
> +                                   unsigned int lane)
> +{
> +       struct mtk_pcie_lane_efuse *data;
> +       void __iomem *addr;
> +
> +       if (lane >= ARRAY_SIZE(pcie_phy->efuse)) {
> +               dev_err(pcie_phy->dev,
> +                       "Requested lane number %d exceeds maximum %ld\n",
> +                       lane, ARRAY_SIZE(pcie_phy->efuse) - 1);
> +               return;
> +       }
> +
> +       data = &pcie_phy->efuse[lane];
> +       if (!data->lane_efuse_supported)
> +               return;
> +
> +       addr = pcie_phy->sif_base + PEXTP_ANA_LN0_TRX_REG +
> +              lane * PEXTP_ANA_LANE_OFFSET;
> +
> +       mtk_phy_update_bits(addr + PEXTP_ANA_TX_OFFSET, EFUSE_LN_TX_PMOS_SEL,
> +                           EFUSE_LN_TX_PMOS_VAL(data->tx_pmos));
> +
> +       mtk_phy_update_bits(addr + PEXTP_ANA_TX_OFFSET, EFUSE_LN_TX_NMOS_SEL,
> +                           EFUSE_LN_TX_NMOS_VAL(data->tx_nmos));
> +
> +       mtk_phy_update_bits(addr + PEXTP_ANA_RX_OFFSET, EFUSE_LN_RX_SEL,
> +                           EFUSE_LN_RX_VAL(data->rx_data));
> +}
> +
> +/**
> + * mtk_pcie_phy_init() - Initialize the phy
> + * @phy: the phy to be initialized
> + *
> + * Initialize the phy by setting the efuse data.
> + * The hardware settings will be reset during suspend, it should be
> + * reinitialized when the consumer calls phy_init() again on resume.
> + */
> +static int mtk_pcie_phy_init(struct phy *phy)
> +{
> +       struct mtk_pcie_phy *pcie_phy = phy_get_drvdata(phy);
> +
> +       if (!pcie_phy->sw_efuse_supported)
> +               return 0;
> +
> +       /* Set global data */
> +       mtk_phy_update_bits(pcie_phy->sif_base + PEXTP_ANA_GLB_00_REG,
> +                           EFUSE_GLB_INTR_SEL,
> +                           EFUSE_GLB_INTR_VAL(pcie_phy->efuse_glb_intr));
> +
> +       mtk_pcie_efuse_set_lane(pcie_phy, 0);
> +
> +       mtk_pcie_efuse_set_lane(pcie_phy, 1);
> +
> +       return 0;
> +}
> +
> +static const struct phy_ops mtk_pcie_phy_ops = {
> +       .init   = mtk_pcie_phy_init,
> +       .owner  = THIS_MODULE,
> +};
> +
> +static int mtk_pcie_efuse_read_for_lane(struct mtk_pcie_phy *pcie_phy,
> +                                       unsigned int lane)
> +{
> +       struct device *dev = pcie_phy->dev;
> +       struct mtk_pcie_lane_efuse *data;
> +       char efuse_id[15];
> +       int ret;
> +
> +       if (lane >= ARRAY_SIZE(pcie_phy->efuse))
> +               return dev_err_probe(pcie_phy->dev, -EINVAL,
> +                                    "Requested lane number %d exceeds maximum %ld\n",
> +                                    lane, ARRAY_SIZE(pcie_phy->efuse) - 1);
> +
> +       data = &pcie_phy->efuse[lane];
> +
> +       snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_pmos", lane);
> +       ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->tx_pmos);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
> +
> +       snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_nmos", lane);
> +       ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->tx_nmos);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
> +
> +       snprintf(efuse_id, sizeof(efuse_id), "rx_ln%d", lane);
> +       ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->rx_data);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
> +
> +       if (!(data->tx_pmos || data->tx_nmos || data->rx_data))
> +               return dev_err_probe(dev, -EINVAL,
> +                                    "No efuse data found for lane%d, but dts enable it\n",
> +                                    lane);
> +
> +       data->lane_efuse_supported = true;
> +
> +       return 0;
> +}
> +
> +static int mtk_pcie_read_efuse(struct mtk_pcie_phy *pcie_phy)
> +{
> +       struct device *dev = pcie_phy->dev;
> +       bool nvmem_enabled;
> +       int ret;
> +
> +       nvmem_enabled = device_property_read_bool(dev, "nvmem-cells");
> +       if (!nvmem_enabled)
> +               return -ENODEV;
> +
> +       ret = nvmem_cell_read_variable_le_u32(dev, "glb_intr",
> +                                             &pcie_phy->efuse_glb_intr);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to read glb_intr\n");
> +
> +       pcie_phy->sw_efuse_supported = true;
> +
> +       ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 0);
> +       if (ret)
> +               return ret;
> +
> +       ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 1);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int mtk_pcie_phy_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct phy_provider *provider;
> +       struct mtk_pcie_phy *pcie_phy;
> +
> +       pcie_phy = devm_kzalloc(dev, sizeof(*pcie_phy), GFP_KERNEL);
> +       if (!pcie_phy)
> +               return -ENOMEM;
> +
> +       pcie_phy->dev = dev;
> +
> +       pcie_phy->sif_base = devm_platform_ioremap_resource_byname(pdev, "sif");
> +       if (IS_ERR(pcie_phy->sif_base))
> +               return dev_err_probe(dev, PTR_ERR(pcie_phy->sif_base),
> +                                    "Failed to map phy-sif base\n");
> +
> +       pcie_phy->phy = devm_phy_create(dev, dev->of_node, &mtk_pcie_phy_ops);
> +       if (IS_ERR(pcie_phy->phy))
> +               return dev_err_probe(dev, PTR_ERR(pcie_phy->phy),
> +                                    "Failed to create PCIe phy\n");
> +
> +       /*
> +        * Failed to read the efuse data is not a fatal problem,
> +        * ignore the failure and keep going.
> +        */
> +       mtk_pcie_read_efuse(pcie_phy);

You need to check the return value for -EPROBE_DEFER to defer probing.

> +
> +       phy_set_drvdata(pcie_phy->phy, pcie_phy);
> +
> +       provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +       if (IS_ERR(provider))
> +               return dev_err_probe(dev, PTR_ERR(provider),
> +                                    "PCIe phy probe failed\n");
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id mtk_pcie_phy_of_match[] = {
> +       { .compatible = "mediatek,mt8195-pcie-phy" },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_pcie_phy_of_match);
> +
> +static struct platform_driver mtk_pcie_phy_driver = {
> +       .probe  = mtk_pcie_phy_probe,
> +       .driver = {
> +               .name = "mtk-pcie-phy",
> +               .of_match_table = mtk_pcie_phy_of_match,
> +       },
> +};
> +module_platform_driver(mtk_pcie_phy_driver);
> +
> +MODULE_DESCRIPTION("MediaTek PCIe PHY driver");
> +MODULE_AUTHOR("Jianjun Wang <jianjun.wang@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.18.0
>

Re: [PATCH v2 2/2] phy: mediatek: Add PCIe PHY driver

[AngeloGioacchino Del Regno]

Il 18/03/22 10:54, Jianjun Wang ha scritto:
> Add PCIe GEN3 PHY driver support on MediaTek chipsets.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>   drivers/phy/mediatek/Kconfig        |  11 ++
>   drivers/phy/mediatek/Makefile       |   1 +
>   drivers/phy/mediatek/phy-mtk-pcie.c | 246 ++++++++++++++++++++++++++++
>   3 files changed, 258 insertions(+)
>   create mode 100644 drivers/phy/mediatek/phy-mtk-pcie.c
> 
> diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
> index 55f8e6c048ab..387ed1b3f2cc 100644
> --- a/drivers/phy/mediatek/Kconfig
> +++ b/drivers/phy/mediatek/Kconfig
> @@ -55,3 +55,14 @@ config PHY_MTK_MIPI_DSI
>   	select GENERIC_PHY
>   	help
>   	  Support MIPI DSI for Mediatek SoCs.
> +
> +config PHY_MTK_PCIE
> +	tristate "MediaTek PCIe-PHY Driver"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on OF
> +	select GENERIC_PHY
> +	help
> +	  Say 'Y' here to add support for MediaTek PCIe PHY driver.
> +	  This driver create the basic PHY instance and provides initialize
> +	  callback for PCIe GEN3 port, it supports software efuse
> +	  initialization.
> diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
> index ace660fbed3a..788c13147f63 100644
> --- a/drivers/phy/mediatek/Makefile
> +++ b/drivers/phy/mediatek/Makefile
> @@ -6,6 +6,7 @@
>   obj-$(CONFIG_PHY_MTK_TPHY)		+= phy-mtk-tphy.o
>   obj-$(CONFIG_PHY_MTK_UFS)		+= phy-mtk-ufs.o
>   obj-$(CONFIG_PHY_MTK_XSPHY)		+= phy-mtk-xsphy.o
> +obj-$(CONFIG_PHY_MTK_PCIE)		+= phy-mtk-pcie.o
>   
>   phy-mtk-hdmi-drv-y			:= phy-mtk-hdmi.o
>   phy-mtk-hdmi-drv-y			+= phy-mtk-hdmi-mt2701.o
> diff --git a/drivers/phy/mediatek/phy-mtk-pcie.c b/drivers/phy/mediatek/phy-mtk-pcie.c
> new file mode 100644
> index 000000000000..0f5d7c7e2b7e
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-pcie.c
> @@ -0,0 +1,246 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Jianjun Wang <jianjun.wang@mediatek.com>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/compiler_types.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "phy-mtk-io.h"
> +
> +#define PEXTP_ANA_GLB_00_REG		0x9000
> +#define PEXTP_ANA_LN0_TRX_REG		0xa000
> +#define PEXTP_ANA_TX_OFFSET		0x04
> +#define PEXTP_ANA_RX_OFFSET		0x3c
> +#define PEXTP_ANA_LANE_OFFSET		0x100
> +
> +/* PEXTP_GLB_00_REG[28:24] Internal Resistor Selection of TX Bias Current */
> +#define EFUSE_GLB_INTR_SEL		GENMASK(28, 24)
> +#define EFUSE_GLB_INTR_VAL(x)		((0x1f & (x)) << 24)
> +
> +/* PEXTP_ANA_LN_RX_REG[3:0] RX impedance selection */
> +#define EFUSE_LN_RX_SEL			GENMASK(3, 0)
> +#define EFUSE_LN_RX_VAL(x)		(0xf & (x))
> +
> +/* PEXTP_ANA_LN_TX_REG[5:2] TX PMOS impedance selection */
> +#define EFUSE_LN_TX_PMOS_SEL		GENMASK(5, 2)
> +#define EFUSE_LN_TX_PMOS_VAL(x)		((0xf & (x)) << 2)
> +
> +/* PEXTP_ANA_LN_TX_REG[11:8] TX NMOS impedance selection */
> +#define EFUSE_LN_TX_NMOS_SEL		GENMASK(11, 8)
> +#define EFUSE_LN_TX_NMOS_VAL(x)		((0xf & (x)) << 8)
> +
> +/* Efuse data for each lane */

What about some kerneldoc?

/**
  * struct mtk_pcie_lane_efuse - eFuse data for each lane
  * @tx_pmos:
  ......etc :))

> +struct mtk_pcie_lane_efuse {
> +	u32 tx_pmos;
> +	u32 tx_nmos;
> +	u32 rx_data;
> +	bool lane_efuse_supported;
> +};
> +

Same here

/**
  * struct mtk_pcie_phy - PCIe phy driver main structure
  * @dev: ......

> +struct mtk_pcie_phy {
> +	struct device *dev;
> +	struct phy *phy;
> +	void __iomem *sif_base;
> +
> +	/*
> +	 * Support software efuse initialization,
> +	 * currently we only support 2 lane in maximum.
> +	 */

Obviously, if you add kerneldoc, this comment would get moved to that kerneldoc.

> +	bool sw_efuse_supported;
> +	u32 efuse_glb_intr;


> +	struct mtk_pcie_lane_efuse efuse[2];

If you dynamically allocate this one, you will be able to support any number
of lanes, futureproofing this driver and giving it more flexibility.

> +};
> +

..snip..

> +
> +static int mtk_pcie_efuse_read_for_lane(struct mtk_pcie_phy *pcie_phy,
> +					unsigned int lane)
> +{
> +	struct device *dev = pcie_phy->dev;
> +	struct mtk_pcie_lane_efuse *data;
> +	char efuse_id[15];
> +	int ret;
> +
> +	if (lane >= ARRAY_SIZE(pcie_phy->efuse))
> +		return dev_err_probe(pcie_phy->dev, -EINVAL,
> +				     "Requested lane number %d exceeds maximum %ld\n",
> +				     lane, ARRAY_SIZE(pcie_phy->efuse) - 1);

I don't like seeing dev_err_probe() outside of a probe function, but I acknowledge
that the Linux documentation doesn't seem to give any direction about that, so
this is a personal preference, at this point.

> +
> +	data = &pcie_phy->efuse[lane];
> +
> +	snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_pmos", lane);
> +	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->tx_pmos);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
> +
> +	snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_nmos", lane);
> +	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->tx_nmos);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
> +
> +	snprintf(efuse_id, sizeof(efuse_id), "rx_ln%d", lane);
> +	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data->rx_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read %s\n", efuse_id);
> +
> +	if (!(data->tx_pmos || data->tx_nmos || data->rx_data))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "No efuse data found for lane%d, but dts enable it\n",
> +				     lane);
> +
> +	data->lane_efuse_supported = true;
> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_read_efuse(struct mtk_pcie_phy *pcie_phy)
> +{
> +	struct device *dev = pcie_phy->dev;
> +	bool nvmem_enabled;
> +	int ret;
> +
> +	nvmem_enabled = device_property_read_bool(dev, "nvmem-cells");
> +	if (!nvmem_enabled)
> +		return -ENODEV;
> +
> +	ret = nvmem_cell_read_variable_le_u32(dev, "glb_intr",
> +					      &pcie_phy->efuse_glb_intr);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read glb_intr\n");
> +
> +	pcie_phy->sw_efuse_supported = true;
> +
> +	ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 1);
> +	if (ret)
> +		return ret;

To give some more future-proofing to this driver, I would instead either add a
u32 devicetree property "num-lanes" or, if the same SoC may not have a different
number of lanes across controller instances, I would add a number of lanes
parameter as data for each of_match.

You'd be at that point using a for loop here like:

for (i = 0; i < pcie_phy->num_lanes, i++) {
	ret = mtk_pcie_efuse_read_for_lane(pcie_phy, i);
	if (ret)
		return ret;
}

Of course, the same logic would apply to mtk_pcie_phy_init(), where you are
instead calling mtk_pcie_efuse_set_lane().

> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *provider;
> +	struct mtk_pcie_phy *pcie_phy;
> +
> +	pcie_phy = devm_kzalloc(dev, sizeof(*pcie_phy), GFP_KERNEL);
> +	if (!pcie_phy)
> +		return -ENOMEM;
> +
> +	pcie_phy->dev = dev;
> +
> +	pcie_phy->sif_base = devm_platform_ioremap_resource_byname(pdev, "sif");
> +	if (IS_ERR(pcie_phy->sif_base))
> +		return dev_err_probe(dev, PTR_ERR(pcie_phy->sif_base),
> +				     "Failed to map phy-sif base\n");
> +
> +	pcie_phy->phy = devm_phy_create(dev, dev->of_node, &mtk_pcie_phy_ops);
> +	if (IS_ERR(pcie_phy->phy))
> +		return dev_err_probe(dev, PTR_ERR(pcie_phy->phy),
> +				     "Failed to create PCIe phy\n");
> +
> +	/*
> +	 * Failed to read the efuse data is not a fatal problem,
> +	 * ignore the failure and keep going.
> +	 */
> +	mtk_pcie_read_efuse(pcie_phy);

If you get an -EPROBE_DEFER here, you surely want to defer probing this driver,
so, yes you're free to ignore the other failures, but you should fix that corner
case.

Everything else looks good, so, please make sure to add me to the Cc's for the
next version of this series for me to give you a faster review.

Regards,
Angelo

Re: [PATCH v2 1/2] dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY

[AngeloGioacchino Del Regno]

Il 18/03/22 10:54, Jianjun Wang ha scritto:
> Add YAML schema documentation for PCIe PHY on MediaTek chipsets.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>   .../bindings/phy/mediatek,pcie-phy.yaml       | 75 +++++++++++++++++++
>   1 file changed, 75 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> new file mode 100644
> index 000000000000..868bf976568b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/mediatek,pcie-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek PCIe PHY
> +
> +maintainers:
> +  - Jianjun Wang <jianjun.wang@mediatek.com>
> +
> +description: |
> +  The PCIe PHY supports physical layer functionality for PCIe Gen3 port.
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt8195-pcie-phy

Since I don't expect this driver to be only for MT8195, but to be extended to
support some more future MediaTek SoCs and, depending on the number of differences
in the possible future Gen4 PHYs, even different gen's, I propose to add a generic
compatible as const.

So you'll have something like:

- enum:
     - mediatek,mt8195-pcie-phy
- const: mediatek,pcie-gen3-phy

> +
> +  reg:
> +    maxItems: 1
> +

..snip..

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    phy@11e80000 {
> +        compatible = "mediatek,mt8195-pcie-phy";

... which would reflect here as

compatible = "mediatek,mt8195-pcie-phy", "mediatek,pcie-gen3-phy"

> +        #phy-cells = <0>;
> +        reg = <0x11e80000 0x10000>;
> +        reg-names = "sif";
> +        nvmem-cells = <&pciephy_glb_intr>,
> +                      <&pciephy_tx_ln0_pmos>,
> +                      <&pciephy_tx_ln0_nmos>,
> +                      <&pciephy_rx_ln0>,
> +                      <&pciephy_tx_ln1_pmos>,
> +                      <&pciephy_tx_ln1_nmos>,
> +                      <&pciephy_rx_ln1>;
> +        nvmem-cell-names = "glb_intr", "tx_ln0_pmos",
> +                           "tx_ln0_nmos", "rx_ln0",
> +                           "tx_ln1_pmos", "tx_ln1_nmos",
> +                           "rx_ln1";
> +        power-domains = <&spm 2>;
> +    };


Regards,
Angelo

Re: [PATCH v2 2/2] phy: mediatek: Add PCIe PHY driver

[Jianjun Wang]

Hi Angelo,

Thanks for all these suggestions, I'll fix them in the next version.

Thanks.

On Fri, 2022-03-18 at 11:57 +0100, AngeloGioacchino Del Regno wrote:
> Il 18/03/22 10:54, Jianjun Wang ha scritto:
> > Add PCIe GEN3 PHY driver support on MediaTek chipsets.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > ---
> >   drivers/phy/mediatek/Kconfig        |  11 ++
> >   drivers/phy/mediatek/Makefile       |   1 +
> >   drivers/phy/mediatek/phy-mtk-pcie.c | 246
> > ++++++++++++++++++++++++++++
> >   3 files changed, 258 insertions(+)
> >   create mode 100644 drivers/phy/mediatek/phy-mtk-pcie.c
> > 
> > diff --git a/drivers/phy/mediatek/Kconfig
> > b/drivers/phy/mediatek/Kconfig
> > index 55f8e6c048ab..387ed1b3f2cc 100644
> > --- a/drivers/phy/mediatek/Kconfig
> > +++ b/drivers/phy/mediatek/Kconfig
> > @@ -55,3 +55,14 @@ config PHY_MTK_MIPI_DSI
> >   	select GENERIC_PHY
> >   	help
> >   	  Support MIPI DSI for Mediatek SoCs.
> > +
> > +config PHY_MTK_PCIE
> > +	tristate "MediaTek PCIe-PHY Driver"
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	depends on OF
> > +	select GENERIC_PHY
> > +	help
> > +	  Say 'Y' here to add support for MediaTek PCIe PHY driver.
> > +	  This driver create the basic PHY instance and provides
> > initialize
> > +	  callback for PCIe GEN3 port, it supports software efuse
> > +	  initialization.
> > diff --git a/drivers/phy/mediatek/Makefile
> > b/drivers/phy/mediatek/Makefile
> > index ace660fbed3a..788c13147f63 100644
> > --- a/drivers/phy/mediatek/Makefile
> > +++ b/drivers/phy/mediatek/Makefile
> > @@ -6,6 +6,7 @@
> >   obj-$(CONFIG_PHY_MTK_TPHY)		+= phy-mtk-tphy.o
> >   obj-$(CONFIG_PHY_MTK_UFS)		+= phy-mtk-ufs.o
> >   obj-$(CONFIG_PHY_MTK_XSPHY)		+= phy-mtk-xsphy.o
> > +obj-$(CONFIG_PHY_MTK_PCIE)		+= phy-mtk-pcie.o
> >   
> >   phy-mtk-hdmi-drv-y			:= phy-mtk-hdmi.o
> >   phy-mtk-hdmi-drv-y			+= phy-mtk-hdmi-
> > mt2701.o
> > diff --git a/drivers/phy/mediatek/phy-mtk-pcie.c
> > b/drivers/phy/mediatek/phy-mtk-pcie.c
> > new file mode 100644
> > index 000000000000..0f5d7c7e2b7e
> > --- /dev/null
> > +++ b/drivers/phy/mediatek/phy-mtk-pcie.c
> > @@ -0,0 +1,246 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Jianjun Wang <jianjun.wang@mediatek.com>
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#include "phy-mtk-io.h"
> > +
> > +#define PEXTP_ANA_GLB_00_REG		0x9000
> > +#define PEXTP_ANA_LN0_TRX_REG		0xa000
> > +#define PEXTP_ANA_TX_OFFSET		0x04
> > +#define PEXTP_ANA_RX_OFFSET		0x3c
> > +#define PEXTP_ANA_LANE_OFFSET		0x100
> > +
> > +/* PEXTP_GLB_00_REG[28:24] Internal Resistor Selection of TX Bias
> > Current */
> > +#define EFUSE_GLB_INTR_SEL		GENMASK(28, 24)
> > +#define EFUSE_GLB_INTR_VAL(x)		((0x1f & (x)) << 24)
> > +
> > +/* PEXTP_ANA_LN_RX_REG[3:0] RX impedance selection */
> > +#define EFUSE_LN_RX_SEL			GENMASK(3, 0)
> > +#define EFUSE_LN_RX_VAL(x)		(0xf & (x))
> > +
> > +/* PEXTP_ANA_LN_TX_REG[5:2] TX PMOS impedance selection */
> > +#define EFUSE_LN_TX_PMOS_SEL		GENMASK(5, 2)
> > +#define EFUSE_LN_TX_PMOS_VAL(x)		((0xf & (x)) << 2)
> > +
> > +/* PEXTP_ANA_LN_TX_REG[11:8] TX NMOS impedance selection */
> > +#define EFUSE_LN_TX_NMOS_SEL		GENMASK(11, 8)
> > +#define EFUSE_LN_TX_NMOS_VAL(x)		((0xf & (x)) << 8)
> > +
> > +/* Efuse data for each lane */
> 
> What about some kerneldoc?
> 
> /**
>   * struct mtk_pcie_lane_efuse - eFuse data for each lane
>   * @tx_pmos:
>   ......etc :))
> 
> > +struct mtk_pcie_lane_efuse {
> > +	u32 tx_pmos;
> > +	u32 tx_nmos;
> > +	u32 rx_data;
> > +	bool lane_efuse_supported;
> > +};
> > +
> 
> Same here
> 
> /**
>   * struct mtk_pcie_phy - PCIe phy driver main structure
>   * @dev: ......
> 
> > +struct mtk_pcie_phy {
> > +	struct device *dev;
> > +	struct phy *phy;
> > +	void __iomem *sif_base;
> > +
> > +	/*
> > +	 * Support software efuse initialization,
> > +	 * currently we only support 2 lane in maximum.
> > +	 */
> 
> Obviously, if you add kerneldoc, this comment would get moved to that
> kerneldoc.
> 
> > +	bool sw_efuse_supported;
> > +	u32 efuse_glb_intr;
> 
> 
> > +	struct mtk_pcie_lane_efuse efuse[2];
> 
> If you dynamically allocate this one, you will be able to support any
> number
> of lanes, futureproofing this driver and giving it more flexibility.
> 
> > +};
> > +
> 
> ..snip..
> 
> > +
> > +static int mtk_pcie_efuse_read_for_lane(struct mtk_pcie_phy
> > *pcie_phy,
> > +					unsigned int lane)
> > +{
> > +	struct device *dev = pcie_phy->dev;
> > +	struct mtk_pcie_lane_efuse *data;
> > +	char efuse_id[15];
> > +	int ret;
> > +
> > +	if (lane >= ARRAY_SIZE(pcie_phy->efuse))
> > +		return dev_err_probe(pcie_phy->dev, -EINVAL,
> > +				     "Requested lane number %d exceeds
> > maximum %ld\n",
> > +				     lane, ARRAY_SIZE(pcie_phy->efuse)
> > - 1);
> 
> I don't like seeing dev_err_probe() outside of a probe function, but
> I acknowledge
> that the Linux documentation doesn't seem to give any direction about
> that, so
> this is a personal preference, at this point.
> 
> > +
> > +	data = &pcie_phy->efuse[lane];
> > +
> > +	snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_pmos", lane);
> > +	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data-
> > >tx_pmos);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to read %s\n",
> > efuse_id);
> > +
> > +	snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_nmos", lane);
> > +	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data-
> > >tx_nmos);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to read %s\n",
> > efuse_id);
> > +
> > +	snprintf(efuse_id, sizeof(efuse_id), "rx_ln%d", lane);
> > +	ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data-
> > >rx_data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to read %s\n",
> > efuse_id);
> > +
> > +	if (!(data->tx_pmos || data->tx_nmos || data->rx_data))
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "No efuse data found for lane%d,
> > but dts enable it\n",
> > +				     lane);
> > +
> > +	data->lane_efuse_supported = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_read_efuse(struct mtk_pcie_phy *pcie_phy)
> > +{
> > +	struct device *dev = pcie_phy->dev;
> > +	bool nvmem_enabled;
> > +	int ret;
> > +
> > +	nvmem_enabled = device_property_read_bool(dev, "nvmem-cells");
> > +	if (!nvmem_enabled)
> > +		return -ENODEV;
> > +
> > +	ret = nvmem_cell_read_variable_le_u32(dev, "glb_intr",
> > +					      &pcie_phy-
> > >efuse_glb_intr);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to read
> > glb_intr\n");
> > +
> > +	pcie_phy->sw_efuse_supported = true;
> > +
> > +	ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 1);
> > +	if (ret)
> > +		return ret;
> 
> To give some more future-proofing to this driver, I would instead
> either add a
> u32 devicetree property "num-lanes" or, if the same SoC may not have
> a different
> number of lanes across controller instances, I would add a number of
> lanes
> parameter as data for each of_match.
> 
> You'd be at that point using a for loop here like:
> 
> for (i = 0; i < pcie_phy->num_lanes, i++) {
> 	ret = mtk_pcie_efuse_read_for_lane(pcie_phy, i);
> 	if (ret)
> 		return ret;
> }
> 
> Of course, the same logic would apply to mtk_pcie_phy_init(), where
> you are
> instead calling mtk_pcie_efuse_set_lane().
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_phy_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct phy_provider *provider;
> > +	struct mtk_pcie_phy *pcie_phy;
> > +
> > +	pcie_phy = devm_kzalloc(dev, sizeof(*pcie_phy), GFP_KERNEL);
> > +	if (!pcie_phy)
> > +		return -ENOMEM;
> > +
> > +	pcie_phy->dev = dev;
> > +
> > +	pcie_phy->sif_base =
> > devm_platform_ioremap_resource_byname(pdev, "sif");
> > +	if (IS_ERR(pcie_phy->sif_base))
> > +		return dev_err_probe(dev, PTR_ERR(pcie_phy->sif_base),
> > +				     "Failed to map phy-sif base\n");
> > +
> > +	pcie_phy->phy = devm_phy_create(dev, dev->of_node,
> > &mtk_pcie_phy_ops);
> > +	if (IS_ERR(pcie_phy->phy))
> > +		return dev_err_probe(dev, PTR_ERR(pcie_phy->phy),
> > +				     "Failed to create PCIe phy\n");
> > +
> > +	/*
> > +	 * Failed to read the efuse data is not a fatal problem,
> > +	 * ignore the failure and keep going.
> > +	 */
> > +	mtk_pcie_read_efuse(pcie_phy);
> 
> If you get an -EPROBE_DEFER here, you surely want to defer probing
> this driver,
> so, yes you're free to ignore the other failures, but you should fix
> that corner
> case.
> 
> Everything else looks good, so, please make sure to add me to the
> Cc's for the
> next version of this series for me to give you a faster review.
> 
> Regards,
> Angelo
> 

Re: [PATCH v2 1/2] dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY

[Krzysztof Kozlowski]

On 18/03/2022 12:12, AngeloGioacchino Del Regno wrote:
> Il 18/03/22 10:54, Jianjun Wang ha scritto:
>> Add YAML schema documentation for PCIe PHY on MediaTek chipsets.
>>
>> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
>> ---
>>   .../bindings/phy/mediatek,pcie-phy.yaml       | 75 +++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
>> new file mode 100644
>> index 000000000000..868bf976568b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
>> @@ -0,0 +1,75 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/mediatek,pcie-phy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek PCIe PHY
>> +
>> +maintainers:
>> +  - Jianjun Wang <jianjun.wang@mediatek.com>
>> +
>> +description: |
>> +  The PCIe PHY supports physical layer functionality for PCIe Gen3 port.
>> +
>> +properties:
>> +  compatible:
>> +    const: mediatek,mt8195-pcie-phy
> 
> Since I don't expect this driver to be only for MT8195, but to be extended to
> support some more future MediaTek SoCs and, depending on the number of differences
> in the possible future Gen4 PHYs, even different gen's, I propose to add a generic
> compatible as const.
> 
> So you'll have something like:
> 
> - enum:
>      - mediatek,mt8195-pcie-phy
> - const: mediatek,pcie-gen3-phy

I am not sure if this is a good idea. How sure are you that there will
be no different PCIe Gen3 PHY not compatible with this one?


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY

[Krzysztof Kozlowski]

On 18/03/2022 10:54, Jianjun Wang wrote:
> Add YAML schema documentation for PCIe PHY on MediaTek chipsets.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  .../bindings/phy/mediatek,pcie-phy.yaml       | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> new file mode 100644
> index 000000000000..868bf976568b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/mediatek,pcie-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek PCIe PHY
> +
> +maintainers:
> +  - Jianjun Wang <jianjun.wang@mediatek.com>
> +
> +description: |
> +  The PCIe PHY supports physical layer functionality for PCIe Gen3 port.
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt8195-pcie-phy
> +
> +  reg:
> +    maxItems: 1
> +
> +  reg-names:
> +    items:
> +      - const: sif
> +
> +  "#phy-cells":
> +    const: 0
> +
> +  nvmem-cells:
> +    description:
> +      Phandles to nvmem cell that contains the efuse data, if unspecified,
> +      default value is used.

maxItems: 7

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY

[AngeloGioacchino Del Regno]

Il 18/03/22 14:51, Krzysztof Kozlowski ha scritto:
> On 18/03/2022 12:12, AngeloGioacchino Del Regno wrote:
>> Il 18/03/22 10:54, Jianjun Wang ha scritto:
>>> Add YAML schema documentation for PCIe PHY on MediaTek chipsets.
>>>
>>> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
>>> ---
>>>    .../bindings/phy/mediatek,pcie-phy.yaml       | 75 +++++++++++++++++++
>>>    1 file changed, 75 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
>>> new file mode 100644
>>> index 000000000000..868bf976568b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
>>> @@ -0,0 +1,75 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/phy/mediatek,pcie-phy.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MediaTek PCIe PHY
>>> +
>>> +maintainers:
>>> +  - Jianjun Wang <jianjun.wang@mediatek.com>
>>> +
>>> +description: |
>>> +  The PCIe PHY supports physical layer functionality for PCIe Gen3 port.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: mediatek,mt8195-pcie-phy
>>
>> Since I don't expect this driver to be only for MT8195, but to be extended to
>> support some more future MediaTek SoCs and, depending on the number of differences
>> in the possible future Gen4 PHYs, even different gen's, I propose to add a generic
>> compatible as const.
>>
>> So you'll have something like:
>>
>> - enum:
>>       - mediatek,mt8195-pcie-phy
>> - const: mediatek,pcie-gen3-phy
> 
> I am not sure if this is a good idea. How sure are you that there will
> be no different PCIe Gen3 PHY not compatible with this one?
> 
> 

Thanks for pointing that out, I have underestimated this option.

Perhaps Jianjun may be more informed about whether my proposal is valid or not.

Cheers,
Angelo

> Best regards,
> Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY

[Jianjun Wang]

On Fri, 2022-03-18 at 14:56 +0100, AngeloGioacchino Del Regno wrote:
> Il 18/03/22 14:51, Krzysztof Kozlowski ha scritto:
> > On 18/03/2022 12:12, AngeloGioacchino Del Regno wrote:
> > > Il 18/03/22 10:54, Jianjun Wang ha scritto:
> > > > Add YAML schema documentation for PCIe PHY on MediaTek
> > > > chipsets.
> > > > 
> > > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > > > ---
> > > >    .../bindings/phy/mediatek,pcie-phy.yaml       | 75
> > > > +++++++++++++++++++
> > > >    1 file changed, 75 insertions(+)
> > > >    create mode 100644
> > > > Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> > > > b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> > > > new file mode 100644
> > > > index 000000000000..868bf976568b
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/phy/mediatek,pcie-
> > > > phy.yaml
> > > > @@ -0,0 +1,75 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/phy/mediatek,pcie-phy.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: MediaTek PCIe PHY
> > > > +
> > > > +maintainers:
> > > > +  - Jianjun Wang <jianjun.wang@mediatek.com>
> > > > +
> > > > +description: |
> > > > +  The PCIe PHY supports physical layer functionality for PCIe
> > > > Gen3 port.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: mediatek,mt8195-pcie-phy
> > > 
> > > Since I don't expect this driver to be only for MT8195, but to be
> > > extended to
> > > support some more future MediaTek SoCs and, depending on the
> > > number of differences
> > > in the possible future Gen4 PHYs, even different gen's, I propose
> > > to add a generic
> > > compatible as const.
> > > 
> > > So you'll have something like:
> > > 
> > > - enum:
> > >       - mediatek,mt8195-pcie-phy
> > > - const: mediatek,pcie-gen3-phy
> > 
> > I am not sure if this is a good idea. How sure are you that there
> > will
> > be no different PCIe Gen3 PHY not compatible with this one?
> > 
> > 
> 
> Thanks for pointing that out, I have underestimated this option.
> 
> Perhaps Jianjun may be more informed about whether my proposal is
> valid or not.

Many thanks for the suggestions.

Currently, we only have this PCIe Gen3 PHY, and I don't think we are
planning other PCIe Gen3 PHYs with different software interfaces, even
in the next generation, we want to make sure it has a similar interface
to this generation, so I prefer to add a generic ones to support more
SoCs that need this driver.

Thanks.

> 
> Cheers,
> Angelo
> 
> > Best regards,
> > Krzysztof
> 
> 

Re: [PATCH v2 1/2] dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY

[Jianjun Wang]

Hi Krzysztof,

On Fri, 2022-03-18 at 14:55 +0100, Krzysztof Kozlowski wrote:
> On 18/03/2022 10:54, Jianjun Wang wrote:
> > Add YAML schema documentation for PCIe PHY on MediaTek chipsets.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > ---
> >  .../bindings/phy/mediatek,pcie-phy.yaml       | 75
> > +++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/mediatek,pcie-
> > phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,pcie-
> > phy.yaml
> > new file mode 100644
> > index 000000000000..868bf976568b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> > @@ -0,0 +1,75 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/mediatek,pcie-phy.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek PCIe PHY
> > +
> > +maintainers:
> > +  - Jianjun Wang <jianjun.wang@mediatek.com>
> > +
> > +description: |
> > +  The PCIe PHY supports physical layer functionality for PCIe Gen3
> > port.
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,mt8195-pcie-phy
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reg-names:
> > +    items:
> > +      - const: sif
> > +
> > +  "#phy-cells":
> > +    const: 0
> > +
> > +  nvmem-cells:
> > +    description:
> > +      Phandles to nvmem cell that contains the efuse data, if
> > unspecified,
> > +      default value is used.
> 
> maxItems: 7

Thanks for your review, I'll fix it in the next version.

> 
> Best regards,
> Krzysztof

Re: [PATCH v2 1/2] dt-bindings: phy: mediatek: Add YAML schema for PCIe PHY

[Chen-Yu Tsai]

On Fri, Mar 18, 2022 at 9:56 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 18/03/22 14:51, Krzysztof Kozlowski ha scritto:
> > On 18/03/2022 12:12, AngeloGioacchino Del Regno wrote:
> >> Il 18/03/22 10:54, Jianjun Wang ha scritto:
> >>> Add YAML schema documentation for PCIe PHY on MediaTek chipsets.
> >>>
> >>> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> >>> ---
> >>>    .../bindings/phy/mediatek,pcie-phy.yaml       | 75 +++++++++++++++++++
> >>>    1 file changed, 75 insertions(+)
> >>>    create mode 100644 Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> >>> new file mode 100644
> >>> index 000000000000..868bf976568b
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/phy/mediatek,pcie-phy.yaml
> >>> @@ -0,0 +1,75 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/phy/mediatek,pcie-phy.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: MediaTek PCIe PHY
> >>> +
> >>> +maintainers:
> >>> +  - Jianjun Wang <jianjun.wang@mediatek.com>
> >>> +
> >>> +description: |
> >>> +  The PCIe PHY supports physical layer functionality for PCIe Gen3 port.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: mediatek,mt8195-pcie-phy
> >>
> >> Since I don't expect this driver to be only for MT8195, but to be extended to
> >> support some more future MediaTek SoCs and, depending on the number of differences
> >> in the possible future Gen4 PHYs, even different gen's, I propose to add a generic
> >> compatible as const.
> >>
> >> So you'll have something like:
> >>
> >> - enum:
> >>       - mediatek,mt8195-pcie-phy
> >> - const: mediatek,pcie-gen3-phy
> >
> > I am not sure if this is a good idea. How sure are you that there will
> > be no different PCIe Gen3 PHY not compatible with this one?
> >
> >
>
> Thanks for pointing that out, I have underestimated this option.
>
> Perhaps Jianjun may be more informed about whether my proposal is valid or not.

Just FYI, for Allwinner and I believe Rockchip as well, the compatible strings
always list the first SoC the hardware block was seen on known at the time
of driver/binding submission. No generic compatible strings are ever used.

Not sure if that's the general rule or not.


ChenYu