* [PATCH v7 01/10] PCI: kirin: Reorganize the PHY logic inside the driver
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 02/10] PCI: kirin: Add support for a PHY layer Mauro Carvalho Chehab
` (9 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Xiaowei Song, linux-kernel, linux-pci
The pcie-kirin PCIe driver contains internally a PHY interface for
Kirin 960.
As the next patches will add support for using an external PHY
driver, reorganize the driver in a way that the PHY part
will be self-contained.
This could be moved to a separate PHY driver, but a change
like that would mean a non-backward-compatible DT schema
change.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/pci/controller/dwc/pcie-kirin.c | 298 ++++++++++++++----------
1 file changed, 173 insertions(+), 125 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 026fd1e42a55..b4063a3434df 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -28,26 +28,16 @@
#define to_kirin_pcie(x) dev_get_drvdata((x)->dev)
-#define REF_CLK_FREQ 100000000
-
/* PCIe ELBI registers */
#define SOC_PCIECTRL_CTRL0_ADDR 0x000
#define SOC_PCIECTRL_CTRL1_ADDR 0x004
-#define SOC_PCIEPHY_CTRL2_ADDR 0x008
-#define SOC_PCIEPHY_CTRL3_ADDR 0x00c
#define PCIE_ELBI_SLV_DBI_ENABLE (0x1 << 21)
/* info located in APB */
#define PCIE_APP_LTSSM_ENABLE 0x01c
-#define PCIE_APB_PHY_CTRL0 0x0
-#define PCIE_APB_PHY_CTRL1 0x4
#define PCIE_APB_PHY_STATUS0 0x400
#define PCIE_LINKUP_ENABLE (0x8020)
#define PCIE_LTSSM_ENABLE_BIT (0x1 << 11)
-#define PIPE_CLK_STABLE (0x1 << 19)
-#define PHY_REF_PAD_BIT (0x1 << 8)
-#define PHY_PWR_DOWN_BIT (0x1 << 22)
-#define PHY_RST_ACK_BIT (0x1 << 16)
/* info located in sysctrl */
#define SCTRL_PCIE_CMOS_OFFSET 0x60
@@ -60,6 +50,29 @@
#define PCIE_DEBOUNCE_PARAM 0xF0F400
#define PCIE_OE_BYPASS (0x3 << 28)
+struct kirin_pcie {
+ struct dw_pcie *pci;
+ struct phy *phy;
+ void __iomem *apb_base;
+ void *phy_priv; /* Needed for Kirin 960 PHY */
+};
+
+/*
+ * Kirin 960 PHY. Can't be split into a PHY driver without changing the
+ * DT schema.
+ */
+
+#define REF_CLK_FREQ 100000000
+
+/* PHY info located in APB */
+#define PCIE_APB_PHY_CTRL0 0x0
+#define PCIE_APB_PHY_CTRL1 0x4
+#define PCIE_APB_PHY_STATUS0 0x400
+#define PIPE_CLK_STABLE BIT(19)
+#define PHY_REF_PAD_BIT BIT(8)
+#define PHY_PWR_DOWN_BIT BIT(22)
+#define PHY_RST_ACK_BIT BIT(16)
+
/* peri_crg ctrl */
#define CRGCTRL_PCIE_ASSERT_OFFSET 0x88
#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000
@@ -69,8 +82,6 @@
#define REF_2_PERST_MAX 25000
#define PERST_2_ACCESS_MIN 10000
#define PERST_2_ACCESS_MAX 12000
-#define LINK_WAIT_MIN 900
-#define LINK_WAIT_MAX 1000
#define PIPE_CLK_WAIT_MIN 550
#define PIPE_CLK_WAIT_MAX 600
#define TIME_CMOS_MIN 100
@@ -78,118 +89,112 @@
#define TIME_PHY_PD_MIN 10
#define TIME_PHY_PD_MAX 11
-struct kirin_pcie {
- struct dw_pcie *pci;
- void __iomem *apb_base;
- void __iomem *phy_base;
+struct hi3660_pcie_phy {
+ struct device *dev;
+ void __iomem *base;
struct regmap *crgctrl;
struct regmap *sysctrl;
struct clk *apb_sys_clk;
struct clk *apb_phy_clk;
struct clk *phy_ref_clk;
- struct clk *pcie_aclk;
- struct clk *pcie_aux_clk;
+ struct clk *aclk;
+ struct clk *aux_clk;
int gpio_id_reset;
};
-/* Registers in PCIeCTRL */
-static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
- u32 val, u32 reg)
-{
- writel(val, kirin_pcie->apb_base + reg);
-}
-
-static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
-{
- return readl(kirin_pcie->apb_base + reg);
-}
-
/* Registers in PCIePHY */
-static inline void kirin_apb_phy_writel(struct kirin_pcie *kirin_pcie,
+static inline void kirin_apb_phy_writel(struct hi3660_pcie_phy *hi3660_pcie_phy,
u32 val, u32 reg)
{
- writel(val, kirin_pcie->phy_base + reg);
+ writel(val, hi3660_pcie_phy->base + reg);
}
-static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg)
+static inline u32 kirin_apb_phy_readl(struct hi3660_pcie_phy *hi3660_pcie_phy,
+ u32 reg)
{
- return readl(kirin_pcie->phy_base + reg);
+ return readl(hi3660_pcie_phy->base + reg);
}
-static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie,
- struct platform_device *pdev)
+static int hi3660_pcie_phy_get_clk(struct hi3660_pcie_phy *phy)
{
- struct device *dev = &pdev->dev;
+ struct device *dev = phy->dev;
- kirin_pcie->phy_ref_clk = devm_clk_get(dev, "pcie_phy_ref");
- if (IS_ERR(kirin_pcie->phy_ref_clk))
- return PTR_ERR(kirin_pcie->phy_ref_clk);
+ phy->phy_ref_clk = devm_clk_get(dev, "pcie_phy_ref");
+ if (IS_ERR(phy->phy_ref_clk))
+ return PTR_ERR(phy->phy_ref_clk);
- kirin_pcie->pcie_aux_clk = devm_clk_get(dev, "pcie_aux");
- if (IS_ERR(kirin_pcie->pcie_aux_clk))
- return PTR_ERR(kirin_pcie->pcie_aux_clk);
+ phy->aux_clk = devm_clk_get(dev, "pcie_aux");
+ if (IS_ERR(phy->aux_clk))
+ return PTR_ERR(phy->aux_clk);
- kirin_pcie->apb_phy_clk = devm_clk_get(dev, "pcie_apb_phy");
- if (IS_ERR(kirin_pcie->apb_phy_clk))
- return PTR_ERR(kirin_pcie->apb_phy_clk);
+ phy->apb_phy_clk = devm_clk_get(dev, "pcie_apb_phy");
+ if (IS_ERR(phy->apb_phy_clk))
+ return PTR_ERR(phy->apb_phy_clk);
- kirin_pcie->apb_sys_clk = devm_clk_get(dev, "pcie_apb_sys");
- if (IS_ERR(kirin_pcie->apb_sys_clk))
- return PTR_ERR(kirin_pcie->apb_sys_clk);
+ phy->apb_sys_clk = devm_clk_get(dev, "pcie_apb_sys");
+ if (IS_ERR(phy->apb_sys_clk))
+ return PTR_ERR(phy->apb_sys_clk);
- kirin_pcie->pcie_aclk = devm_clk_get(dev, "pcie_aclk");
- if (IS_ERR(kirin_pcie->pcie_aclk))
- return PTR_ERR(kirin_pcie->pcie_aclk);
+ phy->aclk = devm_clk_get(dev, "pcie_aclk");
+ if (IS_ERR(phy->aclk))
+ return PTR_ERR(phy->aclk);
return 0;
}
-static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
- struct platform_device *pdev)
+static int hi3660_pcie_phy_get_resource(struct hi3660_pcie_phy *phy)
{
- kirin_pcie->apb_base =
- devm_platform_ioremap_resource_byname(pdev, "apb");
- if (IS_ERR(kirin_pcie->apb_base))
- return PTR_ERR(kirin_pcie->apb_base);
-
- kirin_pcie->phy_base =
- devm_platform_ioremap_resource_byname(pdev, "phy");
- if (IS_ERR(kirin_pcie->phy_base))
- return PTR_ERR(kirin_pcie->phy_base);
-
- kirin_pcie->crgctrl =
- syscon_regmap_lookup_by_compatible("hisilicon,hi3660-crgctrl");
- if (IS_ERR(kirin_pcie->crgctrl))
- return PTR_ERR(kirin_pcie->crgctrl);
-
- kirin_pcie->sysctrl =
- syscon_regmap_lookup_by_compatible("hisilicon,hi3660-sctrl");
- if (IS_ERR(kirin_pcie->sysctrl))
- return PTR_ERR(kirin_pcie->sysctrl);
+ struct device *dev = phy->dev;
+ struct platform_device *pdev;
+
+ /* registers */
+ pdev = container_of(dev, struct platform_device, dev);
+
+ phy->base = devm_platform_ioremap_resource_byname(pdev, "phy");
+ if (IS_ERR(phy->base))
+ return PTR_ERR(phy->base);
+
+ phy->crgctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3660-crgctrl");
+ if (IS_ERR(phy->crgctrl))
+ return PTR_ERR(phy->crgctrl);
+
+ phy->sysctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3660-sctrl");
+ if (IS_ERR(phy->sysctrl))
+ return PTR_ERR(phy->sysctrl);
+
+ /* gpios */
+ phy->gpio_id_reset = of_get_named_gpio(dev->of_node,
+ "reset-gpios", 0);
+ if (phy->gpio_id_reset == -EPROBE_DEFER) {
+ return -EPROBE_DEFER;
+ } else if (!gpio_is_valid(phy->gpio_id_reset)) {
+ dev_err(phy->dev, "unable to get a valid gpio pin\n");
+ return -ENODEV;
+ }
return 0;
}
-static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie)
+static int hi3660_pcie_phy_start(struct hi3660_pcie_phy *phy)
{
- struct device *dev = kirin_pcie->pci->dev;
+ struct device *dev = phy->dev;
u32 reg_val;
- reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1);
+ reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_CTRL1);
reg_val &= ~PHY_REF_PAD_BIT;
- kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL1);
+ kirin_apb_phy_writel(phy, reg_val, PCIE_APB_PHY_CTRL1);
- reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL0);
+ reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_CTRL0);
reg_val &= ~PHY_PWR_DOWN_BIT;
- kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL0);
+ kirin_apb_phy_writel(phy, reg_val, PCIE_APB_PHY_CTRL0);
usleep_range(TIME_PHY_PD_MIN, TIME_PHY_PD_MAX);
- reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1);
+ reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_CTRL1);
reg_val &= ~PHY_RST_ACK_BIT;
- kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL1);
+ kirin_apb_phy_writel(phy, reg_val, PCIE_APB_PHY_CTRL1);
usleep_range(PIPE_CLK_WAIT_MIN, PIPE_CLK_WAIT_MAX);
- reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
+ reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_STATUS0);
if (reg_val & PIPE_CLK_STABLE) {
dev_err(dev, "PIPE clk is not stable\n");
return -EINVAL;
@@ -198,105 +203,157 @@ static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie)
return 0;
}
-static void kirin_pcie_oe_enable(struct kirin_pcie *kirin_pcie)
+static void hi3660_pcie_phy_oe_enable(struct hi3660_pcie_phy *phy)
{
u32 val;
- regmap_read(kirin_pcie->sysctrl, SCTRL_PCIE_OE_OFFSET, &val);
+ regmap_read(phy->sysctrl, SCTRL_PCIE_OE_OFFSET, &val);
val |= PCIE_DEBOUNCE_PARAM;
val &= ~PCIE_OE_BYPASS;
- regmap_write(kirin_pcie->sysctrl, SCTRL_PCIE_OE_OFFSET, val);
+ regmap_write(phy->sysctrl, SCTRL_PCIE_OE_OFFSET, val);
}
-static int kirin_pcie_clk_ctrl(struct kirin_pcie *kirin_pcie, bool enable)
+static int hi3660_pcie_phy_clk_ctrl(struct hi3660_pcie_phy *phy, bool enable)
{
int ret = 0;
if (!enable)
goto close_clk;
- ret = clk_set_rate(kirin_pcie->phy_ref_clk, REF_CLK_FREQ);
+ ret = clk_set_rate(phy->phy_ref_clk, REF_CLK_FREQ);
if (ret)
return ret;
- ret = clk_prepare_enable(kirin_pcie->phy_ref_clk);
+ ret = clk_prepare_enable(phy->phy_ref_clk);
if (ret)
return ret;
- ret = clk_prepare_enable(kirin_pcie->apb_sys_clk);
+ ret = clk_prepare_enable(phy->apb_sys_clk);
if (ret)
goto apb_sys_fail;
- ret = clk_prepare_enable(kirin_pcie->apb_phy_clk);
+ ret = clk_prepare_enable(phy->apb_phy_clk);
if (ret)
goto apb_phy_fail;
- ret = clk_prepare_enable(kirin_pcie->pcie_aclk);
+ ret = clk_prepare_enable(phy->aclk);
if (ret)
goto aclk_fail;
- ret = clk_prepare_enable(kirin_pcie->pcie_aux_clk);
+ ret = clk_prepare_enable(phy->aux_clk);
if (ret)
goto aux_clk_fail;
return 0;
close_clk:
- clk_disable_unprepare(kirin_pcie->pcie_aux_clk);
+ clk_disable_unprepare(phy->aux_clk);
aux_clk_fail:
- clk_disable_unprepare(kirin_pcie->pcie_aclk);
+ clk_disable_unprepare(phy->aclk);
aclk_fail:
- clk_disable_unprepare(kirin_pcie->apb_phy_clk);
+ clk_disable_unprepare(phy->apb_phy_clk);
apb_phy_fail:
- clk_disable_unprepare(kirin_pcie->apb_sys_clk);
+ clk_disable_unprepare(phy->apb_sys_clk);
apb_sys_fail:
- clk_disable_unprepare(kirin_pcie->phy_ref_clk);
+ clk_disable_unprepare(phy->phy_ref_clk);
return ret;
}
-static int kirin_pcie_power_on(struct kirin_pcie *kirin_pcie)
+static int hi3660_pcie_phy_power_on(struct kirin_pcie *pcie)
{
+ struct hi3660_pcie_phy *phy = pcie->phy_priv;
int ret;
/* Power supply for Host */
- regmap_write(kirin_pcie->sysctrl,
+ regmap_write(phy->sysctrl,
SCTRL_PCIE_CMOS_OFFSET, SCTRL_PCIE_CMOS_BIT);
usleep_range(TIME_CMOS_MIN, TIME_CMOS_MAX);
- kirin_pcie_oe_enable(kirin_pcie);
- ret = kirin_pcie_clk_ctrl(kirin_pcie, true);
+ hi3660_pcie_phy_oe_enable(phy);
+
+ ret = hi3660_pcie_phy_clk_ctrl(phy, true);
if (ret)
return ret;
/* ISO disable, PCIeCtrl, PHY assert and clk gate clear */
- regmap_write(kirin_pcie->sysctrl,
+ regmap_write(phy->sysctrl,
SCTRL_PCIE_ISO_OFFSET, SCTRL_PCIE_ISO_BIT);
- regmap_write(kirin_pcie->crgctrl,
+ regmap_write(phy->crgctrl,
CRGCTRL_PCIE_ASSERT_OFFSET, CRGCTRL_PCIE_ASSERT_BIT);
- regmap_write(kirin_pcie->sysctrl,
+ regmap_write(phy->sysctrl,
SCTRL_PCIE_HPCLK_OFFSET, SCTRL_PCIE_HPCLK_BIT);
- ret = kirin_pcie_phy_init(kirin_pcie);
+ ret = hi3660_pcie_phy_start(phy);
if (ret)
- goto close_clk;
+ goto disable_clks;
/* perst assert Endpoint */
- if (!gpio_request(kirin_pcie->gpio_id_reset, "pcie_perst")) {
+ if (!gpio_request(phy->gpio_id_reset, "pcie_perst")) {
usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
- ret = gpio_direction_output(kirin_pcie->gpio_id_reset, 1);
+ ret = gpio_direction_output(phy->gpio_id_reset, 1);
if (ret)
- goto close_clk;
+ goto disable_clks;
usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
-
return 0;
}
-close_clk:
- kirin_pcie_clk_ctrl(kirin_pcie, false);
+disable_clks:
+ hi3660_pcie_phy_clk_ctrl(phy, false);
return ret;
}
+static int hi3660_pcie_phy_init(struct platform_device *pdev,
+ struct kirin_pcie *pcie)
+{
+ struct device *dev = &pdev->dev;
+ struct hi3660_pcie_phy *phy;
+ int ret;
+
+ phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ pcie->phy_priv = phy;
+ phy->dev = dev;
+
+ /* registers */
+ pdev = container_of(dev, struct platform_device, dev);
+
+ ret = hi3660_pcie_phy_get_clk(phy);
+ if (ret)
+ return ret;
+
+ return hi3660_pcie_phy_get_resource(phy);
+}
+
+/*
+ * The non-PHY part starts here
+ */
+
+/* Registers in PCIeCTRL */
+static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
+ u32 val, u32 reg)
+{
+ writel(val, kirin_pcie->apb_base + reg);
+}
+
+static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
+{
+ return readl(kirin_pcie->apb_base + reg);
+}
+
+static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
+ struct platform_device *pdev)
+{
+ kirin_pcie->apb_base =
+ devm_platform_ioremap_resource_byname(pdev, "apb");
+ if (IS_ERR(kirin_pcie->apb_base))
+ return PTR_ERR(kirin_pcie->apb_base);
+
+ return 0;
+}
+
static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
bool on)
{
@@ -444,7 +501,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
pci->pp.ops = &kirin_pcie_host_ops;
kirin_pcie->pci = pci;
- ret = kirin_pcie_get_clk(kirin_pcie, pdev);
+ ret = hi3660_pcie_phy_init(pdev, kirin_pcie);
if (ret)
return ret;
@@ -452,16 +509,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
if (ret)
return ret;
- kirin_pcie->gpio_id_reset = of_get_named_gpio(dev->of_node,
- "reset-gpios", 0);
- if (kirin_pcie->gpio_id_reset == -EPROBE_DEFER) {
- return -EPROBE_DEFER;
- } else if (!gpio_is_valid(kirin_pcie->gpio_id_reset)) {
- dev_err(dev, "unable to get a valid gpio pin\n");
- return -ENODEV;
- }
-
- ret = kirin_pcie_power_on(kirin_pcie);
+ ret = hi3660_pcie_phy_power_on(kirin_pcie);
if (ret)
return ret;
@@ -479,8 +527,8 @@ static struct platform_driver kirin_pcie_driver = {
.probe = kirin_pcie_probe,
.driver = {
.name = "kirin-pcie",
- .of_match_table = kirin_pcie_match,
- .suppress_bind_attrs = true,
+ .of_match_table = kirin_pcie_match,
+ .suppress_bind_attrs = true,
},
};
builtin_platform_driver(kirin_pcie_driver);
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v7 02/10] PCI: kirin: Add support for a PHY layer
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 01/10] PCI: kirin: Reorganize the PHY logic inside the driver Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 03/10] PCI: kirin: Use regmap for APB registers Mauro Carvalho Chehab
` (8 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Xiaowei Song, linux-kernel, linux-pci
The pcie-kirin driver contains both PHY and generic PCI driver
on it.
The best would be, instead, to support a PCI PHY driver, making
the driver more generic.
However, it is too late to remove the Kirin 960 PHY, as a change
like that would make the DT schema incompatible with past versions.
So, add support for an external PHY driver without removing the
existing Kirin 960 PHY from it.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/pci/controller/dwc/pcie-kirin.c | 95 +++++++++++++++++++++----
1 file changed, 80 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index b4063a3434df..31514a5d4bb4 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -8,16 +8,18 @@
* Author: Xiaowei Song <songxiaowei@huawei.com>
*/
-#include <linux/compiler.h>
#include <linux/clk.h>
+#include <linux/compiler.h>
#include <linux/delay.h>
#include <linux/err.h>
#include <linux/gpio.h>
#include <linux/interrupt.h>
#include <linux/mfd/syscon.h>
#include <linux/of_address.h>
+#include <linux/of_device.h>
#include <linux/of_gpio.h>
#include <linux/of_pci.h>
+#include <linux/phy/phy.h>
#include <linux/pci.h>
#include <linux/pci_regs.h>
#include <linux/platform_device.h>
@@ -50,11 +52,18 @@
#define PCIE_DEBOUNCE_PARAM 0xF0F400
#define PCIE_OE_BYPASS (0x3 << 28)
+enum pcie_kirin_phy_type {
+ PCIE_KIRIN_INTERNAL_PHY,
+ PCIE_KIRIN_EXTERNAL_PHY
+};
+
struct kirin_pcie {
+ enum pcie_kirin_phy_type type;
+
struct dw_pcie *pci;
struct phy *phy;
void __iomem *apb_base;
- void *phy_priv; /* Needed for Kirin 960 PHY */
+ void *phy_priv; /* only for PCIE_KIRIN_INTERNAL_PHY */
};
/*
@@ -476,8 +485,63 @@ static const struct dw_pcie_host_ops kirin_pcie_host_ops = {
.host_init = kirin_pcie_host_init,
};
+static int kirin_pcie_power_on(struct platform_device *pdev,
+ struct kirin_pcie *kirin_pcie)
+{
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY) {
+ ret = hi3660_pcie_phy_init(pdev, kirin_pcie);
+ if (ret)
+ return ret;
+
+ return hi3660_pcie_phy_power_on(kirin_pcie);
+ }
+
+ kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL);
+ if (IS_ERR(kirin_pcie->phy))
+ return PTR_ERR(kirin_pcie->phy);
+
+ ret = phy_init(kirin_pcie->phy);
+ if (ret)
+ goto err;
+
+ ret = phy_power_on(kirin_pcie->phy);
+ if (ret)
+ goto err;
+
+ return 0;
+err:
+ phy_exit(kirin_pcie->phy);
+ return ret;
+}
+
+static int __exit kirin_pcie_remove(struct platform_device *pdev)
+{
+ struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
+
+ if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
+ return 0;
+
+ phy_power_off(kirin_pcie->phy);
+ phy_exit(kirin_pcie->phy);
+
+ return 0;
+}
+
+static const struct of_device_id kirin_pcie_match[] = {
+ {
+ .compatible = "hisilicon,kirin960-pcie",
+ .data = (void *)PCIE_KIRIN_INTERNAL_PHY
+ },
+ {},
+};
+
static int kirin_pcie_probe(struct platform_device *pdev)
{
+ enum pcie_kirin_phy_type phy_type;
+ const struct of_device_id *of_id;
struct device *dev = &pdev->dev;
struct kirin_pcie *kirin_pcie;
struct dw_pcie *pci;
@@ -488,6 +552,14 @@ static int kirin_pcie_probe(struct platform_device *pdev)
return -EINVAL;
}
+ of_id = of_match_device(kirin_pcie_match, dev);
+ if (!of_id) {
+ dev_err(dev, "OF data missing\n");
+ return -EINVAL;
+ }
+
+ phy_type = (enum pcie_kirin_phy_type)of_id->data;
+
kirin_pcie = devm_kzalloc(dev, sizeof(struct kirin_pcie), GFP_KERNEL);
if (!kirin_pcie)
return -ENOMEM;
@@ -500,31 +572,24 @@ static int kirin_pcie_probe(struct platform_device *pdev)
pci->ops = &kirin_dw_pcie_ops;
pci->pp.ops = &kirin_pcie_host_ops;
kirin_pcie->pci = pci;
-
- ret = hi3660_pcie_phy_init(pdev, kirin_pcie);
- if (ret)
- return ret;
+ kirin_pcie->type = phy_type;
ret = kirin_pcie_get_resource(kirin_pcie, pdev);
if (ret)
return ret;
- ret = hi3660_pcie_phy_power_on(kirin_pcie);
- if (ret)
- return ret;
-
platform_set_drvdata(pdev, kirin_pcie);
+ ret = kirin_pcie_power_on(pdev, kirin_pcie);
+ if (ret)
+ return ret;
+
return dw_pcie_host_init(&pci->pp);
}
-static const struct of_device_id kirin_pcie_match[] = {
- { .compatible = "hisilicon,kirin960-pcie" },
- {},
-};
-
static struct platform_driver kirin_pcie_driver = {
.probe = kirin_pcie_probe,
+ .remove = __exit_p(kirin_pcie_remove),
.driver = {
.name = "kirin-pcie",
.of_match_table = kirin_pcie_match,
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v7 03/10] PCI: kirin: Use regmap for APB registers
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 01/10] PCI: kirin: Reorganize the PHY logic inside the driver Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 02/10] PCI: kirin: Add support for a PHY layer Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 04/10] PCI: kirin: Add MODULE_* macros Mauro Carvalho Chehab
` (7 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Xiaowei Song, linux-kernel, linux-pci
The PHY layer need to access APB registers too, for Kirin 970.
So, place them into a named regmap.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/pci/controller/dwc/pcie-kirin.c | 39 +++++++++++++++++--------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 31514a5d4bb4..5fe0cd0af572 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -61,8 +61,8 @@ struct kirin_pcie {
enum pcie_kirin_phy_type type;
struct dw_pcie *pci;
+ struct regmap *apb;
struct phy *phy;
- void __iomem *apb_base;
void *phy_priv; /* only for PCIE_KIRIN_INTERNAL_PHY */
};
@@ -340,6 +340,13 @@ static int hi3660_pcie_phy_init(struct platform_device *pdev,
* The non-PHY part starts here
*/
+static const struct regmap_config pcie_kirin_regmap_conf = {
+ .name = "kirin_pcie_apb",
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+};
+
/* Registers in PCIeCTRL */
static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
u32 val, u32 reg)
@@ -355,10 +362,17 @@ static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
struct platform_device *pdev)
{
- kirin_pcie->apb_base =
- devm_platform_ioremap_resource_byname(pdev, "apb");
- if (IS_ERR(kirin_pcie->apb_base))
- return PTR_ERR(kirin_pcie->apb_base);
+ struct device *dev = &pdev->dev;
+ void __iomem *apb_base;
+
+ apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
+ if (IS_ERR(apb_base))
+ return PTR_ERR(apb_base);
+
+ kirin_pcie->apb = devm_regmap_init_mmio(dev, apb_base,
+ &pcie_kirin_regmap_conf);
+ if (IS_ERR(kirin_pcie->apb))
+ return PTR_ERR(kirin_pcie->apb);
return 0;
}
@@ -368,13 +382,13 @@ static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
{
u32 val;
- val = kirin_apb_ctrl_readl(kirin_pcie, SOC_PCIECTRL_CTRL0_ADDR);
+ regmap_read(kirin_pcie->apb, SOC_PCIECTRL_CTRL0_ADDR, &val);
if (on)
val = val | PCIE_ELBI_SLV_DBI_ENABLE;
else
val = val & ~PCIE_ELBI_SLV_DBI_ENABLE;
- kirin_apb_ctrl_writel(kirin_pcie, val, SOC_PCIECTRL_CTRL0_ADDR);
+ regmap_write(kirin_pcie->apb, SOC_PCIECTRL_CTRL0_ADDR, val);
}
static void kirin_pcie_sideband_dbi_r_mode(struct kirin_pcie *kirin_pcie,
@@ -382,13 +396,13 @@ static void kirin_pcie_sideband_dbi_r_mode(struct kirin_pcie *kirin_pcie,
{
u32 val;
- val = kirin_apb_ctrl_readl(kirin_pcie, SOC_PCIECTRL_CTRL1_ADDR);
+ regmap_read(kirin_pcie->apb, SOC_PCIECTRL_CTRL1_ADDR, &val);
if (on)
val = val | PCIE_ELBI_SLV_DBI_ENABLE;
else
val = val & ~PCIE_ELBI_SLV_DBI_ENABLE;
- kirin_apb_ctrl_writel(kirin_pcie, val, SOC_PCIECTRL_CTRL1_ADDR);
+ regmap_write(kirin_pcie->apb, SOC_PCIECTRL_CTRL1_ADDR, val);
}
static int kirin_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
@@ -448,8 +462,9 @@ static void kirin_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
static int kirin_pcie_link_up(struct dw_pcie *pci)
{
struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
- u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
+ u32 val;
+ regmap_read(kirin_pcie->apb, PCIE_APB_PHY_STATUS0, &val);
if ((val & PCIE_LINKUP_ENABLE) == PCIE_LINKUP_ENABLE)
return 1;
@@ -461,8 +476,8 @@ static int kirin_pcie_start_link(struct dw_pcie *pci)
struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
/* assert LTSSM enable */
- kirin_apb_ctrl_writel(kirin_pcie, PCIE_LTSSM_ENABLE_BIT,
- PCIE_APP_LTSSM_ENABLE);
+ regmap_write(kirin_pcie->apb, PCIE_APP_LTSSM_ENABLE,
+ PCIE_LTSSM_ENABLE_BIT);
return 0;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v7 04/10] PCI: kirin: Add MODULE_* macros
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
` (2 preceding siblings ...)
2021-07-21 8:39 ` [PATCH v7 03/10] PCI: kirin: Use regmap for APB registers Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 05/10] dt-bindings: PCI: kirin: Fix compatible string Mauro Carvalho Chehab
` (6 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Xiaowei Song, linux-kernel, linux-pci
This driver misses the MODULE_* macros. Add them.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/pci/controller/dwc/pcie-kirin.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 5fe0cd0af572..a32f7f36461c 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -612,3 +612,8 @@ static struct platform_driver kirin_pcie_driver = {
},
};
builtin_platform_driver(kirin_pcie_driver);
+
+MODULE_DEVICE_TABLE(of, kirin_pcie_match);
+MODULE_DESCRIPTION("PCIe host controller driver for Kirin Phone SoCs");
+MODULE_AUTHOR("Xiaowei Song <songxiaowei@huawei.com>");
+MODULE_LICENSE("GPL v2");
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v7 05/10] dt-bindings: PCI: kirin: Fix compatible string
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
` (3 preceding siblings ...)
2021-07-21 8:39 ` [PATCH v7 04/10] PCI: kirin: Add MODULE_* macros Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY Mauro Carvalho Chehab
` (5 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Rob Herring,
devicetree, linux-kernel, linux-pci
The pcie-kirin driver doesn't declare a hisilicon,kirin-pcie.
Also, remove the useless comment after the description, as other
compat will be supported by the same driver in the future.
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
Documentation/devicetree/bindings/pci/kirin-pcie.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
index 7db30534498f..7adab8999a6a 100644
--- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
@@ -9,7 +9,7 @@ Additional properties are described here:
Required properties
- compatible:
- "hisilicon,kirin960-pcie" for PCIe of Kirin960 SoC
+ "hisilicon,kirin960-pcie"
- reg: Should contain rc_dbi, apb, phy, config registers location and length.
- reg-names: Must include the following entries:
"dbi": controller configuration registers;
@@ -23,7 +23,7 @@ Optional properties:
Example based on kirin960:
pcie@f4000000 {
- compatible = "hisilicon,kirin-pcie";
+ compatible = "hisilicon,kirin960-pcie";
reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
<0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
reg-names = "dbi","apb","phy", "config";
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
` (4 preceding siblings ...)
2021-07-21 8:39 ` [PATCH v7 05/10] dt-bindings: PCI: kirin: Fix compatible string Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-23 22:50 ` Rob Herring
2021-07-21 8:39 ` [PATCH v7 07/10] phy: HiSilicon: Add driver for Kirin " Mauro Carvalho Chehab
` (4 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
Kishon Vijay Abraham I, Rob Herring, devicetree, linux-kernel,
linux-phy
Document the bindings for HiKey 970 (hi3670) PCIe PHY
interface, supported via the pcie-kirin driver.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
.../phy/hisilicon,phy-hi3670-pcie.yaml | 95 +++++++++++++++++++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
new file mode 100644
index 000000000000..a5ea13332cac
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin970 PCIe PHY
+
+maintainers:
+ - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
+
+description: |+
+ Bindings for PCIe PHY on HiSilicon Kirin 970.
+
+properties:
+ compatible:
+ const: hisilicon,hi970-pcie-phy
+
+ "#phy-cells":
+ const: 0
+
+ reg:
+ maxItems: 1
+ description: PHY Control registers
+
+ phy-supply:
+ description: The PCIe PHY power supply
+
+ clocks:
+ items:
+ - description: PCIe PHY clock
+ - description: PCIe AUX clock
+ - description: PCIe APB PHY clock
+ - description: PCIe APB SYS clock
+ - description: PCIe ACLK clock
+
+ clock-names:
+ items:
+ - const: phy_ref
+ - const: aux
+ - const: apb_phy
+ - const: apb_sys
+ - const: aclk
+
+ reset-gpios:
+ description: PCI PERST reset GPIOs
+ maxItems: 4
+
+ clkreq-gpios:
+ description: Clock request GPIOs
+ maxItems: 3
+
+ hisilicon,eye-diagram-param:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: Eye diagram for phy.
+
+required:
+ - "#phy-cells"
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - reset-gpios
+ - clkreq-gpios
+ - hisilicon,eye-diagram-param
+ - phy-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/hi3670-clock.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ pcie_phy: pcie-phy@fc000000 {
+ compatible = "hisilicon,hi970-pcie-phy";
+ reg = <0x0 0xfc000000 0x0 0x80000>;
+ #phy-cells = <0>;
+ phy-supply = <&ldo33>;
+ clocks = <&crg_ctrl HI3670_CLK_GATE_PCIEPHY_REF>,
+ <&crg_ctrl HI3670_CLK_GATE_PCIEAUX>,
+ <&crg_ctrl HI3670_PCLK_GATE_PCIE_PHY>,
+ <&crg_ctrl HI3670_PCLK_GATE_PCIE_SYS>,
+ <&crg_ctrl HI3670_ACLK_GATE_PCIE>;
+ clock-names = "phy_ref", "aux",
+ "apb_phy", "apb_sys", "aclk";
+ reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+ <&gpio3 1 0 >, <&gpio27 4 0 >;
+ clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >, <&gpio17 0 0 >;
+ hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
+ 0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF>;
+ };
+ };
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-21 8:39 ` [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY Mauro Carvalho Chehab
@ 2021-07-23 22:50 ` Rob Herring
2021-07-24 0:12 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2021-07-23 22:50 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Vinod Koul, Bjorn Helgaas, linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:
> Document the bindings for HiKey 970 (hi3670) PCIe PHY
> interface, supported via the pcie-kirin driver.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> .../phy/hisilicon,phy-hi3670-pcie.yaml | 95 +++++++++++++++++++
> 1 file changed, 95 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> new file mode 100644
> index 000000000000..a5ea13332cac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon Kirin970 PCIe PHY
> +
> +maintainers:
> + - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> +
> +description: |+
> + Bindings for PCIe PHY on HiSilicon Kirin 970.
> +
> +properties:
> + compatible:
> + const: hisilicon,hi970-pcie-phy
> +
> + "#phy-cells":
> + const: 0
> +
> + reg:
> + maxItems: 1
> + description: PHY Control registers
> +
> + phy-supply:
> + description: The PCIe PHY power supply
> +
> + clocks:
> + items:
> + - description: PCIe PHY clock
> + - description: PCIe AUX clock
> + - description: PCIe APB PHY clock
> + - description: PCIe APB SYS clock
> + - description: PCIe ACLK clock
> +
> + clock-names:
> + items:
> + - const: phy_ref
> + - const: aux
> + - const: apb_phy
> + - const: apb_sys
> + - const: aclk
> +
> + reset-gpios:
> + description: PCI PERST reset GPIOs
> + maxItems: 4
> +
> + clkreq-gpios:
> + description: Clock request GPIOs
> + maxItems: 3
Again, this will not work.
It boils down to this fails to describe how the GPIOs are connected
which is the point of GPIOs in DT. This in no way captures the hierarchy
of devices. While you may be lucky that you can just assert or
deassert all the lines at one time, that's not likely to work in a
more complicated case (such as having to power up/down each device).
I realize the right solution is more complex, but that's the only way to
handle this in a host bridge and board independent way.
If you want the simple solution, just configure all these GPIOs in
firmware before Linux boots.
Rob
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-23 22:50 ` Rob Herring
@ 2021-07-24 0:12 ` Mauro Carvalho Chehab
2021-07-26 21:37 ` Rob Herring
0 siblings, 1 reply; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-24 0:12 UTC (permalink / raw)
To: Rob Herring
Cc: Vinod Koul, Bjorn Helgaas, linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
Em Fri, 23 Jul 2021 16:50:59 -0600
Rob Herring <robh@kernel.org> escreveu:
> On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:
> > Document the bindings for HiKey 970 (hi3670) PCIe PHY
> > interface, supported via the pcie-kirin driver.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > .../phy/hisilicon,phy-hi3670-pcie.yaml | 95 +++++++++++++++++++
> > 1 file changed, 95 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > new file mode 100644
> > index 000000000000..a5ea13332cac
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: HiSilicon Kirin970 PCIe PHY
> > +
> > +maintainers:
> > + - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > +
> > +description: |+
> > + Bindings for PCIe PHY on HiSilicon Kirin 970.
> > +
> > +properties:
> > + compatible:
> > + const: hisilicon,hi970-pcie-phy
> > +
> > + "#phy-cells":
> > + const: 0
> > +
> > + reg:
> > + maxItems: 1
> > + description: PHY Control registers
> > +
> > + phy-supply:
> > + description: The PCIe PHY power supply
> > +
> > + clocks:
> > + items:
> > + - description: PCIe PHY clock
> > + - description: PCIe AUX clock
> > + - description: PCIe APB PHY clock
> > + - description: PCIe APB SYS clock
> > + - description: PCIe ACLK clock
> > +
> > + clock-names:
> > + items:
> > + - const: phy_ref
> > + - const: aux
> > + - const: apb_phy
> > + - const: apb_sys
> > + - const: aclk
> > +
> > + reset-gpios:
> > + description: PCI PERST reset GPIOs
> > + maxItems: 4
> > +
> > + clkreq-gpios:
> > + description: Clock request GPIOs
> > + maxItems: 3
>
> Again, this will not work.
Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
here, right?
> It boils down to this fails to describe how the GPIOs are connected
> which is the point of GPIOs in DT. This in no way captures the hierarchy
> of devices. While you may be lucky that you can just assert or
> deassert all the lines at one time, that's not likely to work in a
> more complicated case (such as having to power up/down each device).
There's no way to power up/down each device, as they all share the
same regulator line (LDO33). So, when this is powered on, all PCI
devices are powered at the same time.
The original DT had names for each reset-gpio, but this was just
informative, as the only possible way for this hardware to work is
to send the PERST# signal via all GPIOs at the same time.
Ok, we might overdesign the DT, in order to consider a non-existent
scenario where it would be possible to power on and reset the devices
in separate, but I can't think on a way to do that, except by maybe
creating virtual phy (or pcie) DT nodes, one for each combination of
regulator + PERST#, and have separate drivers for each one. Such kind
of scenario only makes sense when each PCIe device can be powered on
independently (which is not the case here).
If you have a better idea, I'm all ears.
>
> I realize the right solution is more complex, but that's the only way to
> handle this in a host bridge and board independent way.
>
> If you want the simple solution, just configure all these GPIOs in
> firmware before Linux boots.
This won't work. The PERST# signal should be sent after initializing
the PCIe + PHY and powering up the PEX8606 PCIe bridge chipset
(via LDO33). That happens when the PCIe driver is loaded.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-24 0:12 ` Mauro Carvalho Chehab
@ 2021-07-26 21:37 ` Rob Herring
2021-07-26 23:50 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2021-07-26 21:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
On Fri, Jul 23, 2021 at 6:12 PM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Fri, 23 Jul 2021 16:50:59 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:
> > > Document the bindings for HiKey 970 (hi3670) PCIe PHY
> > > interface, supported via the pcie-kirin driver.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > > .../phy/hisilicon,phy-hi3670-pcie.yaml | 95 +++++++++++++++++++
> > > 1 file changed, 95 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > > new file mode 100644
> > > index 000000000000..a5ea13332cac
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > > @@ -0,0 +1,95 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: HiSilicon Kirin970 PCIe PHY
> > > +
> > > +maintainers:
> > > + - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > +
> > > +description: |+
> > > + Bindings for PCIe PHY on HiSilicon Kirin 970.
> > > +
> > > +properties:
> > > + compatible:
> > > + const: hisilicon,hi970-pcie-phy
> > > +
> > > + "#phy-cells":
> > > + const: 0
> > > +
> > > + reg:
> > > + maxItems: 1
> > > + description: PHY Control registers
> > > +
> > > + phy-supply:
> > > + description: The PCIe PHY power supply
> > > +
> > > + clocks:
> > > + items:
> > > + - description: PCIe PHY clock
> > > + - description: PCIe AUX clock
> > > + - description: PCIe APB PHY clock
> > > + - description: PCIe APB SYS clock
> > > + - description: PCIe ACLK clock
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: phy_ref
> > > + - const: aux
> > > + - const: apb_phy
> > > + - const: apb_sys
> > > + - const: aclk
> > > +
> > > + reset-gpios:
> > > + description: PCI PERST reset GPIOs
> > > + maxItems: 4
> > > +
> > > + clkreq-gpios:
> > > + description: Clock request GPIOs
> > > + maxItems: 3
> >
> > Again, this will not work.
>
> Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> here, right?
Both that and CLKREQ.
> > It boils down to this fails to describe how the GPIOs are connected
> > which is the point of GPIOs in DT. This in no way captures the hierarchy
> > of devices. While you may be lucky that you can just assert or
> > deassert all the lines at one time, that's not likely to work in a
> > more complicated case (such as having to power up/down each device).
>
> There's no way to power up/down each device, as they all share the
> same regulator line (LDO33). So, when this is powered on, all PCI
> devices are powered at the same time.
I understand that for your board, but you could easily have a power
supply per device (or multiple supplies per device).
> The original DT had names for each reset-gpio, but this was just
> informative, as the only possible way for this hardware to work is
> to send the PERST# signal via all GPIOs at the same time.
What's the timing requirement here? I doubt 'at the same time' is the
actual h/w requirement. My guess is it is before the PCI bus scan if
you don't have any hook before each child bus is scanned.
> Ok, we might overdesign the DT, in order to consider a non-existent
> scenario where it would be possible to power on and reset the devices
> in separate, but I can't think on a way to do that, except by maybe
> creating virtual phy (or pcie) DT nodes, one for each combination of
> regulator + PERST#, and have separate drivers for each one. Such kind
> of scenario only makes sense when each PCIe device can be powered on
> independently (which is not the case here).
If someone made hikey970 with the topology you have, then someone can
just as easily make a different topology and one that doesn't work
with the assumptions you've made. We're only going to see more and
more embedded boards with multiple PCI devices.
> If you have a better idea, I'm all ears.
There's already a spec for populating PCI devices in DT. It's existed
for over 20 years with OpenFirmware[1]. It's not widely used on FDT
systems because most cases to date are just a single device attached
and they don't have extra things needing to be described in DT. There
are a few, but not many examples in the tree of PCI devices with DT
nodes. That's the only way to generically describe the topology you
have.
While I haven't seen another case exactly like yours yet, there are
frequent cases of PCI devices (and other discoverable buses) that have
extra resources that are not discoverable. And some of those need
control before the device can be discovered. I see various
work-arounds to the problem, but describing the devices in DT is the
right way. It's only going to get solved if the work-arounds are
rejected. I care more that the DT binding is correct and less if the
kernel side is clean. The kernel implementation can evolve, the DT
cannot.
> > I realize the right solution is more complex, but that's the only way to
> > handle this in a host bridge and board independent way.
> >
> > If you want the simple solution, just configure all these GPIOs in
> > firmware before Linux boots.
>
> This won't work. The PERST# signal should be sent after initializing
> the PCIe + PHY and powering up the PEX8606 PCIe bridge chipset
> (via LDO33). That happens when the PCIe driver is loaded.
Only because you have no hooks for handling PERST# on devices
downstream of the PEX8606. Surely a sequence like this would work:
deassert root PERST# (to PEX8606), scan root bus, find and init PCIe
bridge, deassert PEX8606 child bus(es) PERST#, scan child bus(es),
find and init child devices. I think the .add_bus() hook could work
for you. IIRC, that's called before a child bus is scanned.
Rob
[1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-26 21:37 ` Rob Herring
@ 2021-07-26 23:50 ` Mauro Carvalho Chehab
2021-07-27 6:52 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-26 23:50 UTC (permalink / raw)
To: Rob Herring
Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
Em Mon, 26 Jul 2021 15:37:28 -0600
Rob Herring <robh@kernel.org> escreveu:
> On Fri, Jul 23, 2021 at 6:12 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Fri, 23 Jul 2021 16:50:59 -0600
> > Rob Herring <robh@kernel.org> escreveu:
> >
> > > On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:
> > > > Document the bindings for HiKey 970 (hi3670) PCIe PHY
> > > > interface, supported via the pcie-kirin driver.
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > > .../phy/hisilicon,phy-hi3670-pcie.yaml | 95 +++++++++++++++++++
> > > > 1 file changed, 95 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > > > new file mode 100644
> > > > index 000000000000..a5ea13332cac
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > > > @@ -0,0 +1,95 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: HiSilicon Kirin970 PCIe PHY
> > > > +
> > > > +maintainers:
> > > > + - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > +
> > > > +description: |+
> > > > + Bindings for PCIe PHY on HiSilicon Kirin 970.
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + const: hisilicon,hi970-pcie-phy
> > > > +
> > > > + "#phy-cells":
> > > > + const: 0
> > > > +
> > > > + reg:
> > > > + maxItems: 1
> > > > + description: PHY Control registers
> > > > +
> > > > + phy-supply:
> > > > + description: The PCIe PHY power supply
> > > > +
> > > > + clocks:
> > > > + items:
> > > > + - description: PCIe PHY clock
> > > > + - description: PCIe AUX clock
> > > > + - description: PCIe APB PHY clock
> > > > + - description: PCIe APB SYS clock
> > > > + - description: PCIe ACLK clock
> > > > +
> > > > + clock-names:
> > > > + items:
> > > > + - const: phy_ref
> > > > + - const: aux
> > > > + - const: apb_phy
> > > > + - const: apb_sys
> > > > + - const: aclk
> > > > +
> > > > + reset-gpios:
> > > > + description: PCI PERST reset GPIOs
> > > > + maxItems: 4
> > > > +
> > > > + clkreq-gpios:
> > > > + description: Clock request GPIOs
> > > > + maxItems: 3
> > >
> > > Again, this will not work.
> >
> > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > here, right?
>
> Both that and CLKREQ.
>
> > > It boils down to this fails to describe how the GPIOs are connected
> > > which is the point of GPIOs in DT. This in no way captures the hierarchy
> > > of devices. While you may be lucky that you can just assert or
> > > deassert all the lines at one time, that's not likely to work in a
> > > more complicated case (such as having to power up/down each device).
> >
> > There's no way to power up/down each device, as they all share the
> > same regulator line (LDO33). So, when this is powered on, all PCI
> > devices are powered at the same time.
>
> I understand that for your board, but you could easily have a power
> supply per device (or multiple supplies per device).
There are probably thousands of alternatives, but I don't see any
benefit on trying to do write a very complex abstraction here.
>
> > The original DT had names for each reset-gpio, but this was just
> > informative, as the only possible way for this hardware to work is
> > to send the PERST# signal via all GPIOs at the same time.
>
> What's the timing requirement here? I doubt 'at the same time' is the
> actual h/w requirement. My guess is it is before the PCI bus scan if
> you don't have any hook before each child bus is scanned.
No idea, as the available documentation doesn't mention. As this is an
old hardware, finding any extra documentation about it is not easy,
and, afaikt, the developers who originally worked on it (back in 2017)
were already moved to work with least two or three generations that came
after this SoC. So, we need to check what the code does.
Looking at the code, both the PERST# signals and the CLKREQ happen
during the PHY power on sequence, and don't require any special
order. They just need to be initialized altogether at the same
power on step. The power on steps are:
1. turn on the power supply from the regulator to feed the bridge
and other PCI devices;
2. turn on the PHY;
3. request clocks;
4. set PHY clock and enable the other clocks;
5. configure some parameters at the PHY layer;
6. send the PERST# signals. This part has actually a 21 ms delay before
sending the signal and will wait for 10ms after sending PERST# to
all PCI devices. The actual PERST# code is:
usleep_range(21000, 23000);
for (i = 0; i < phy->n_gpio_resets; i++) {
ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
if (ret)
return ret;
}
usleep_range(10000, 11000);
In summary, all PERST# signals are sent (about) the same time,
and the driver logic will wait for 10 ms.
7. Wait for the PCIe to be stable;
8. Adjust the eye parameter;
9. power off NOC.
All the above happens before the PCI bus scan.
> > Ok, we might overdesign the DT, in order to consider a non-existent
> > scenario where it would be possible to power on and reset the devices
> > in separate, but I can't think on a way to do that, except by maybe
> > creating virtual phy (or pcie) DT nodes, one for each combination of
> > regulator + PERST#, and have separate drivers for each one. Such kind
> > of scenario only makes sense when each PCIe device can be powered on
> > independently (which is not the case here).
>
> If someone made hikey970 with the topology you have, then someone can
> just as easily make a different topology and one that doesn't work
> with the assumptions you've made. We're only going to see more and
> more embedded boards with multiple PCI devices.
I wouldn't expect a new device using this chipset being upstreamed.
As I said before, there are 3 generations after Kirin 970.
>
> > If you have a better idea, I'm all ears.
>
> There's already a spec for populating PCI devices in DT. It's existed
> for over 20 years with OpenFirmware[1]. It's not widely used on FDT
> systems because most cases to date are just a single device attached
> and they don't have extra things needing to be described in DT. There
> are a few, but not many examples in the tree of PCI devices with DT
> nodes. That's the only way to generically describe the topology you
> have.
I'll try to find something, but still I think that this is overdesign,
as this is really a single event that was split on multiple GPIOs just
due to some voltage/current/temperature constraints.
> While I haven't seen another case exactly like yours yet, there are
> frequent cases of PCI devices (and other discoverable buses) that have
> extra resources that are not discoverable. And some of those need
> control before the device can be discovered. I see various
> work-arounds to the problem, but describing the devices in DT is the
> right way. It's only going to get solved if the work-arounds are
> rejected. I care more that the DT binding is correct and less if the
> kernel side is clean. The kernel implementation can evolve, the DT
> cannot.
Yeah, I understand that DT changes would be painful, but, IMHO,
writing something very complex here just because the hardware design
opted to use multiple GPIOs instead of a single one is overkill.
> > > I realize the right solution is more complex, but that's the only way to
> > > handle this in a host bridge and board independent way.
> > >
> > > If you want the simple solution, just configure all these GPIOs in
> > > firmware before Linux boots.
> >
> > This won't work. The PERST# signal should be sent after initializing
> > the PCIe + PHY and powering up the PEX8606 PCIe bridge chipset
> > (via LDO33). That happens when the PCIe driver is loaded.
>
> Only because you have no hooks for handling PERST# on devices
> downstream of the PEX8606. Surely a sequence like this would work:
> deassert root PERST# (to PEX8606), scan root bus, find and init PCIe
> bridge, deassert PEX8606 child bus(es) PERST#, scan child bus(es),
> find and init child devices. I think the .add_bus() hook could work
> for you. IIRC, that's called before a child bus is scanned.
As explained above, after sending the PERST# signal and wait for
10 ms, it tunes the PHY eye diagram and powers off NOC.
> [1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps
Thanks,
Mauro
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-26 23:50 ` Mauro Carvalho Chehab
@ 2021-07-27 6:52 ` Mauro Carvalho Chehab
2021-07-27 22:17 ` Rob Herring
0 siblings, 1 reply; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-27 6:52 UTC (permalink / raw)
To: Rob Herring
Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
Em Tue, 27 Jul 2021 01:50:20 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> Em Mon, 26 Jul 2021 15:37:28 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > > > > + reset-gpios:
> > > > > + description: PCI PERST reset GPIOs
> > > > > + maxItems: 4
> > > > > +
> > > > > + clkreq-gpios:
> > > > > + description: Clock request GPIOs
> > > > > + maxItems: 3
> > > >
> > > > Again, this will not work.
> > >
> > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > here, right?
> >
> > Both that and CLKREQ.
The original DT from the downstream version (found at Linaro's tree)
has:
pcie@f4000000 {
compatible = "hisilicon,hikey970";
...
switch,reset-gpios = <&gpio7 0 0 >;
eth,reset-gpios = <&gpio25 2 0 >;
m_2,reset-gpios = <&gpio3 1 0 >;
mini1,reset-gpios = <&gpio27 4 0 >;
eth,clkreq-gpios = <&gpio20 6 0 >;
m_2,clkreq-gpios = <&gpio27 3 0 >;
mini1,clkreq-gpios = <&gpio17 0 0 >;
};
So, if we're willing to have a single reset-gpios for the PCIe
interface, in order to follow the current pci-bus.yaml schema,
this would probably be:
reset-gpios = <&gpio7 0 0 >;
which maps to the PEX8606 PCIe bridge chip.
With that, DT still need to point a per-slot clkreq and
reset-gpio.
One alternative would be to map it as either 3 PCI or PHY
child nodes. E. g. something like this:
pcie@f4000000 {
compatible = "hisilicon,kirin970-pcie";
...
reset-gpios = <&gpio7 0 0 >;
slot {
eth {
reset-gpios = <&gpio25 2 0>;
clkreq-gpios = <&gpio20 6 0>;
};
m2 {
reset-gpios = <&gpio3 1 0>;
clkreq-gpios = <&gpio27 3 0>;
};
mini1 {
reset-gpios = <&gpio27 4 0>;
clkreq-gpios = <&gpio17 0 0>;
};
};
};
Placing the child nodes ("slot"?) at the pci bus properties makes more
sense to me, but placing them at the PHY node has the advantage of
only affecting Kirin 970.
In either case, if each child would need a different power supply,
it won't be hard to add a "slot-supply" property later on.
Would something like that be acceptable for you?
> > > If you have a better idea, I'm all ears.
> >
> > There's already a spec for populating PCI devices in DT. It's existed
> > for over 20 years with OpenFirmware[1]. It's not widely used on FDT
> > systems because most cases to date are just a single device attached
> > and they don't have extra things needing to be described in DT. There
> > are a few, but not many examples in the tree of PCI devices with DT
> > nodes. That's the only way to generically describe the topology you
> > have.
> >
> > [1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps
I was unable to find anything useful there at the two PCI documents.
This one:
https://www.devicetree.org/open-firmware/bindings/pci/pci-express.txt
has just one property that might be useful:
physical-slot#
The main one:
https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
mentions a few child properties, but it doesn't show how those were
supposed to be mapped, and none of the properties mentioned there
specify clocks, gpios, or reset pins.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-27 6:52 ` Mauro Carvalho Chehab
@ 2021-07-27 22:17 ` Rob Herring
2021-07-28 7:38 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2021-07-27 22:17 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 27 Jul 2021 01:50:20 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
>
> > Em Mon, 26 Jul 2021 15:37:28 -0600
> > Rob Herring <robh@kernel.org> escreveu:
> >
>
> > > > > > + reset-gpios:
> > > > > > + description: PCI PERST reset GPIOs
> > > > > > + maxItems: 4
> > > > > > +
> > > > > > + clkreq-gpios:
> > > > > > + description: Clock request GPIOs
> > > > > > + maxItems: 3
> > > > >
> > > > > Again, this will not work.
> > > >
> > > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > > here, right?
> > >
> > > Both that and CLKREQ.
>
> The original DT from the downstream version (found at Linaro's tree)
> has:
>
> pcie@f4000000 {
> compatible = "hisilicon,hikey970";
> ...
> switch,reset-gpios = <&gpio7 0 0 >;
> eth,reset-gpios = <&gpio25 2 0 >;
> m_2,reset-gpios = <&gpio3 1 0 >;
> mini1,reset-gpios = <&gpio27 4 0 >;
>
> eth,clkreq-gpios = <&gpio20 6 0 >;
> m_2,clkreq-gpios = <&gpio27 3 0 >;
> mini1,clkreq-gpios = <&gpio17 0 0 >;
> };
>
> So, if we're willing to have a single reset-gpios for the PCIe
> interface, in order to follow the current pci-bus.yaml schema,
> this would probably be:
>
> reset-gpios = <&gpio7 0 0 >;
>
> which maps to the PEX8606 PCIe bridge chip.
>
> With that, DT still need to point a per-slot clkreq and
> reset-gpio.
>
> One alternative would be to map it as either 3 PCI or PHY
> child nodes. E. g. something like this:
>
> pcie@f4000000 {
> compatible = "hisilicon,kirin970-pcie";
> ...
> reset-gpios = <&gpio7 0 0 >;
>
> slot {
> eth {
> reset-gpios = <&gpio25 2 0>;
> clkreq-gpios = <&gpio20 6 0>;
> };
> m2 {
> reset-gpios = <&gpio3 1 0>;
> clkreq-gpios = <&gpio27 3 0>;
> };
> mini1 {
> reset-gpios = <&gpio27 4 0>;
> clkreq-gpios = <&gpio17 0 0>;
> };
> };
> };
>
>
> Placing the child nodes ("slot"?) at the pci bus properties makes more
> sense to me, but placing them at the PHY node has the advantage of
> only affecting Kirin 970.
>
> In either case, if each child would need a different power supply,
> it won't be hard to add a "slot-supply" property later on.
>
> Would something like that be acceptable for you?
On the right track, but there's already a definition for what child
devices look like in pci2_1.pdf. I think you want something like this:
pcie@f4000000 { // RP: Bus 0, Device 0
compatible = "hisilicon,kirin970-pcie";
...
reset-gpios = <&gpio7 0 0>; // PERST to switch
pcie@0 { // PCIe switch: Bus 1, Device 0
reg = <0 0 0 0 0>;
compatible = "pciclass,0604";
device_type = "pci";
pcie@1 { // NC (Can omit this node)
reg = <0x80 0 0 0 0>;
compatible = "pciclass,0604";
device_type = "pci";
};
pcie@4 { // M.2
reg = <0x200 0 0 0 0>;
compatible = "pciclass,0604";
device_type = "pci";
reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot
};
pcie@5 { // Mini
reg = <0x280 0 0 0 0>;
compatible = "pciclass,0604";
device_type = "pci";
reset-gpios = <&gpio7 2 0>; // PERST to Mini slot
};
pcie@7 { // Ethernet
reg = <0x380 0 0 0 0>;
compatible = "pciclass,0604";
device_type = "pci";
reset-gpios = <&gpio7 3 0>; // PERST to Ethernet
ethernet@0 {
reg = <0 0 0 0 0>;
local-mac-address = [ 00 01 02 03 04 05 06 ];
};
};
pcie@9 { // NC
reg = <0x480 0 0 0 0>;
compatible = "pciclass,0604";
device_type = "pci";
};
};
This is based on what you previously sent:
00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)
A few notes:
I left out #size-cells, #address-cells, and ranges for brevity.
I'm not completely sure I've got the bridges mapped to the right
functions on Hikey970. That's my best guess looking at the schematics.
You should be able to confirm which bridge is the parent bridge for
ethernet at least.
The compatible strings aren't strictly needed. Linux doesn't look at them.
There's a pretty complete example in:
arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
The simplest Linux implementation to handle the above is just walk the
child nodes and get all the 'reset-gpios' properties. That's not the
implementation I think we should have though. We should handle the
GPIO as each bridge is probed and children scanned.
Rob
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-27 22:17 ` Rob Herring
@ 2021-07-28 7:38 ` Mauro Carvalho Chehab
2021-07-28 14:28 ` Rob Herring
0 siblings, 1 reply; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-28 7:38 UTC (permalink / raw)
To: Rob Herring
Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
Em Tue, 27 Jul 2021 16:17:43 -0600
Rob Herring <robh@kernel.org> escreveu:
> On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Tue, 27 Jul 2021 01:50:20 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> >
> > > Em Mon, 26 Jul 2021 15:37:28 -0600
> > > Rob Herring <robh@kernel.org> escreveu:
> > >
> >
> > > > > > > + reset-gpios:
> > > > > > > + description: PCI PERST reset GPIOs
> > > > > > > + maxItems: 4
> > > > > > > +
> > > > > > > + clkreq-gpios:
> > > > > > > + description: Clock request GPIOs
> > > > > > > + maxItems: 3
> > > > > >
> > > > > > Again, this will not work.
> > > > >
> > > > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > > > here, right?
> > > >
> > > > Both that and CLKREQ.
> >
> > The original DT from the downstream version (found at Linaro's tree)
> > has:
> >
> > pcie@f4000000 {
> > compatible = "hisilicon,hikey970";
> > ...
> > switch,reset-gpios = <&gpio7 0 0 >;
> > eth,reset-gpios = <&gpio25 2 0 >;
> > m_2,reset-gpios = <&gpio3 1 0 >;
> > mini1,reset-gpios = <&gpio27 4 0 >;
> >
> > eth,clkreq-gpios = <&gpio20 6 0 >;
> > m_2,clkreq-gpios = <&gpio27 3 0 >;
> > mini1,clkreq-gpios = <&gpio17 0 0 >;
> > };
> >
> > So, if we're willing to have a single reset-gpios for the PCIe
> > interface, in order to follow the current pci-bus.yaml schema,
> > this would probably be:
> >
> > reset-gpios = <&gpio7 0 0 >;
> >
> > which maps to the PEX8606 PCIe bridge chip.
> >
> > With that, DT still need to point a per-slot clkreq and
> > reset-gpio.
> >
> > One alternative would be to map it as either 3 PCI or PHY
> > child nodes. E. g. something like this:
> >
> > pcie@f4000000 {
> > compatible = "hisilicon,kirin970-pcie";
> > ...
> > reset-gpios = <&gpio7 0 0 >;
> >
> > slot {
> > eth {
> > reset-gpios = <&gpio25 2 0>;
> > clkreq-gpios = <&gpio20 6 0>;
> > };
> > m2 {
> > reset-gpios = <&gpio3 1 0>;
> > clkreq-gpios = <&gpio27 3 0>;
> > };
> > mini1 {
> > reset-gpios = <&gpio27 4 0>;
> > clkreq-gpios = <&gpio17 0 0>;
> > };
> > };
> > };
> >
> >
> > Placing the child nodes ("slot"?) at the pci bus properties makes more
> > sense to me, but placing them at the PHY node has the advantage of
> > only affecting Kirin 970.
> >
> > In either case, if each child would need a different power supply,
> > it won't be hard to add a "slot-supply" property later on.
> >
> > Would something like that be acceptable for you?
>
> On the right track, but there's already a definition for what child
> devices look like in pci2_1.pdf. I think you want something like this:
>
> pcie@f4000000 { // RP: Bus 0, Device 0
> compatible = "hisilicon,kirin970-pcie";
> ...
> reset-gpios = <&gpio7 0 0>; // PERST to switch
>
> pcie@0 { // PCIe switch: Bus 1, Device 0
> reg = <0 0 0 0 0>;
> compatible = "pciclass,0604";
> device_type = "pci";
>
> pcie@1 { // NC (Can omit this node)
> reg = <0x80 0 0 0 0>;
> compatible = "pciclass,0604";
> device_type = "pci";
> };
>
> pcie@4 { // M.2
> reg = <0x200 0 0 0 0>;
Not sure what to put at reg. I suspect that the best would be to follow
the PEX port number, as, if one day someone decides to implement an
I2C driver, this might be useful.
> compatible = "pciclass,0604";
> device_type = "pci";
> reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot
We also need the clock-req phandle for the three devices.
> };
>
> pcie@5 { // Mini
> reg = <0x280 0 0 0 0>;
> compatible = "pciclass,0604";
> device_type = "pci";
> reset-gpios = <&gpio7 2 0>; // PERST to Mini slot
> };
>
> pcie@7 { // Ethernet
> reg = <0x380 0 0 0 0>;
> compatible = "pciclass,0604";
> device_type = "pci";
> reset-gpios = <&gpio7 3 0>; // PERST to Ethernet
>
> ethernet@0 {
> reg = <0 0 0 0 0>;
> local-mac-address = [ 00 01 02 03 04 05 06 ];
> };
No need to add a mac address here. The Ethernet card has a mac
already:
60:fa:9d:xx:xx:xx
Which seems to be a valid one:
https://hwaddress.com/?q=60%3Afa%3A9d
> };
>
> pcie@9 { // NC
> reg = <0x480 0 0 0 0>;
> compatible = "pciclass,0604";
> device_type = "pci";
> };
> };
>
> This is based on what you previously sent:
> 00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
> 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)
>
> A few notes:
> I left out #size-cells, #address-cells, and ranges for brevity.
>
> I'm not completely sure I've got the bridges mapped to the right
> functions on Hikey970. That's my best guess looking at the schematics.
> You should be able to confirm which bridge is the parent bridge for
> ethernet at least.
I added a NVMe to M.2 slot:
$ lspci -D -P -PP
0000:00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
0000:00:00.0/01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:01.0/03:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd Device a809
0000:00:00.0/01:00.0/02:07.0/06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)
it sounds that all devices are behind the PEX 8606 bridge:
- port 1 seems to be the M.2 slot
- port 7 seems to be the Ethernet adapter
I don't have any mini PCIe devices here. I'll try to get one in order to be
sure about the topology.
> The compatible strings aren't strictly needed. Linux doesn't look at them.
If not needed, IMO the best would be to not add it, keeping it as
simple as possible.
>
> There's a pretty complete example in:
> arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
Thanks!
> The simplest Linux implementation to handle the above is just walk the
> child nodes and get all the 'reset-gpios' properties. That's not the
> implementation I think we should have though. We should handle the
> GPIO as each bridge is probed and children scanned.
The power-on sequence is:
1. CLK, PHY and DWC initialization;
2. 21ms delay;
3. PERST# sent to each device;
4. 10ms delay;
5. adjust the eye diagram;
6. power off NOC.
Most of the above are at the PHY driver. Now, it would be possible to
map those as:
phy->init() - steps 1 and 2;
phy->power_on() - steps 4, 5 and 6.
And change somehow the pcie-kirin driver to only call phy->power_on()
after doing the bus probing sequence, but a change like that would mean
that the eye diagram will only be adjusted at the end. That doesn't
sound a good idea to me, as probing devices with a wrong eye diagram
could cause bit errors when talking to the devices inside the PCI bus.
This is likely not a problem if all devices are directly connected to
the hardware, but it could be an issue if someone uses either a M.2 or
a mini-PCI extensor.
So, IMO, the best would be for the PHY driver to walk the child nodes
and get all the 'reset-gpios' properties.
With regards to the clock-req phandles, those should be enabled before
the PHY clocks, in order to avoid the SError issue.
It should be easy to implement this at the the PCIe driver, but, this
should happen in early stages at the power-on sequence (before enabling
the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to
walk the child nodes and get all the 'reset-gpios' properties.
For the sake of avoiding to duplicate the walk-though and parsing
logic, I would do it only at the PHY driver.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-28 7:38 ` Mauro Carvalho Chehab
@ 2021-07-28 14:28 ` Rob Herring
2021-07-29 10:12 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2021-07-28 14:28 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
On Wed, Jul 28, 2021 at 1:38 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 27 Jul 2021 16:17:43 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab
> > <mchehab+huawei@kernel.org> wrote:
> > >
> > > Em Tue, 27 Jul 2021 01:50:20 +0200
> > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> > >
> > > > Em Mon, 26 Jul 2021 15:37:28 -0600
> > > > Rob Herring <robh@kernel.org> escreveu:
> > > >
> > >
> > > > > > > > + reset-gpios:
> > > > > > > > + description: PCI PERST reset GPIOs
> > > > > > > > + maxItems: 4
> > > > > > > > +
> > > > > > > > + clkreq-gpios:
> > > > > > > > + description: Clock request GPIOs
> > > > > > > > + maxItems: 3
> > > > > > >
> > > > > > > Again, this will not work.
> > > > > >
> > > > > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > > > > here, right?
> > > > >
> > > > > Both that and CLKREQ.
> > >
> > > The original DT from the downstream version (found at Linaro's tree)
> > > has:
> > >
> > > pcie@f4000000 {
> > > compatible = "hisilicon,hikey970";
> > > ...
> > > switch,reset-gpios = <&gpio7 0 0 >;
> > > eth,reset-gpios = <&gpio25 2 0 >;
> > > m_2,reset-gpios = <&gpio3 1 0 >;
> > > mini1,reset-gpios = <&gpio27 4 0 >;
> > >
> > > eth,clkreq-gpios = <&gpio20 6 0 >;
> > > m_2,clkreq-gpios = <&gpio27 3 0 >;
> > > mini1,clkreq-gpios = <&gpio17 0 0 >;
> > > };
> > >
> > > So, if we're willing to have a single reset-gpios for the PCIe
> > > interface, in order to follow the current pci-bus.yaml schema,
> > > this would probably be:
> > >
> > > reset-gpios = <&gpio7 0 0 >;
> > >
> > > which maps to the PEX8606 PCIe bridge chip.
> > >
> > > With that, DT still need to point a per-slot clkreq and
> > > reset-gpio.
> > >
> > > One alternative would be to map it as either 3 PCI or PHY
> > > child nodes. E. g. something like this:
> > >
> > > pcie@f4000000 {
> > > compatible = "hisilicon,kirin970-pcie";
> > > ...
> > > reset-gpios = <&gpio7 0 0 >;
> > >
> > > slot {
> > > eth {
> > > reset-gpios = <&gpio25 2 0>;
> > > clkreq-gpios = <&gpio20 6 0>;
> > > };
> > > m2 {
> > > reset-gpios = <&gpio3 1 0>;
> > > clkreq-gpios = <&gpio27 3 0>;
> > > };
> > > mini1 {
> > > reset-gpios = <&gpio27 4 0>;
> > > clkreq-gpios = <&gpio17 0 0>;
> > > };
> > > };
> > > };
> > >
> > >
> > > Placing the child nodes ("slot"?) at the pci bus properties makes more
> > > sense to me, but placing them at the PHY node has the advantage of
> > > only affecting Kirin 970.
> > >
> > > In either case, if each child would need a different power supply,
> > > it won't be hard to add a "slot-supply" property later on.
> > >
> > > Would something like that be acceptable for you?
> >
> > On the right track, but there's already a definition for what child
> > devices look like in pci2_1.pdf. I think you want something like this:
> >
> > pcie@f4000000 { // RP: Bus 0, Device 0
> > compatible = "hisilicon,kirin970-pcie";
> > ...
> > reset-gpios = <&gpio7 0 0>; // PERST to switch
> >
> > pcie@0 { // PCIe switch: Bus 1, Device 0
> > reg = <0 0 0 0 0>;
> > compatible = "pciclass,0604";
> > device_type = "pci";
> >
> > pcie@1 { // NC (Can omit this node)
> > reg = <0x80 0 0 0 0>;
> > compatible = "pciclass,0604";
> > device_type = "pci";
> > };
> >
> > pcie@4 { // M.2
> > reg = <0x200 0 0 0 0>;
>
> Not sure what to put at reg. I suspect that the best would be to follow
> the PEX port number, as, if one day someone decides to implement an
> I2C driver, this might be useful.
It's defined in the PCI bus binding. Basically, it's the BDF of the
device. However, as the bus number is dynamic, I think we want to
leave that as 0 for FDT. The function is optional and always 0 in this
case.
> > compatible = "pciclass,0604";
> > device_type = "pci";
> > reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot
>
> We also need the clock-req phandle for the three devices.
Hopefully, you can figure out where those belong now...
>
> > };
> >
> > pcie@5 { // Mini
> > reg = <0x280 0 0 0 0>;
> > compatible = "pciclass,0604";
> > device_type = "pci";
> > reset-gpios = <&gpio7 2 0>; // PERST to Mini slot
> > };
> >
> > pcie@7 { // Ethernet
> > reg = <0x380 0 0 0 0>;
> > compatible = "pciclass,0604";
> > device_type = "pci";
> > reset-gpios = <&gpio7 3 0>; // PERST to Ethernet
> >
> > ethernet@0 {
> > reg = <0 0 0 0 0>;
> > local-mac-address = [ 00 01 02 03 04 05 06 ];
> > };
>
> No need to add a mac address here. The Ethernet card has a mac
> already:
That was just for illustration of what a device node would look like
should you need extra properties for a device. If you only needed to
set the MAC address, guess what, you need to create the hierarchy
above.
>
> 60:fa:9d:xx:xx:xx
>
> Which seems to be a valid one:
>
> https://hwaddress.com/?q=60%3Afa%3A9d
>
> > };
> >
> > pcie@9 { // NC
> > reg = <0x480 0 0 0 0>;
> > compatible = "pciclass,0604";
> > device_type = "pci";
> > };
> > };
> >
> > This is based on what you previously sent:
> > 00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
> > 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)
> >
> > A few notes:
> > I left out #size-cells, #address-cells, and ranges for brevity.
> >
> > I'm not completely sure I've got the bridges mapped to the right
> > functions on Hikey970. That's my best guess looking at the schematics.
> > You should be able to confirm which bridge is the parent bridge for
> > ethernet at least.
>
> I added a NVMe to M.2 slot:
>
> $ lspci -D -P -PP
> 0000:00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
> 0000:00:00.0/01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:01.0/03:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd Device a809
> 0000:00:00.0/01:00.0/02:07.0/06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)
>
> it sounds that all devices are behind the PEX 8606 bridge:
> - port 1 seems to be the M.2 slot
> - port 7 seems to be the Ethernet adapter
>
> I don't have any mini PCIe devices here. I'll try to get one in order to be
> sure about the topology.
I found the mapping in table 4-1 from
https://docs.broadcom.com/doc/PEX_8606-BA_Data_Book_v1.3_31Mar11.pdf
So it is like this:
Device 0 - lane 0 - upstream
Device 1 - lane 4 - m.2
Device 5 - lane 5 - mini PCIe
Device 7 - lane 6 - ethernet
'lane' is the signal numbering in the schematics.
>
> > The compatible strings aren't strictly needed. Linux doesn't look at them.
>
> If not needed, IMO the best would be to not add it, keeping it as
> simple as possible.
I would say that anything with extra properties should have a compatible.
> >
> > There's a pretty complete example in:
> > arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
>
> Thanks!
>
> > The simplest Linux implementation to handle the above is just walk the
> > child nodes and get all the 'reset-gpios' properties. That's not the
> > implementation I think we should have though. We should handle the
> > GPIO as each bridge is probed and children scanned.
>
> The power-on sequence is:
>
> 1. CLK, PHY and DWC initialization;
> 2. 21ms delay;
> 3. PERST# sent to each device;
> 4. 10ms delay;
> 5. adjust the eye diagram;
> 6. power off NOC.
>
> Most of the above are at the PHY driver. Now, it would be possible to
> map those as:
>
> phy->init() - steps 1 and 2;
> phy->power_on() - steps 4, 5 and 6.
>
> And change somehow the pcie-kirin driver to only call phy->power_on()
> after doing the bus probing sequence, but a change like that would mean
> that the eye diagram will only be adjusted at the end. That doesn't
> sound a good idea to me, as probing devices with a wrong eye diagram
> could cause bit errors when talking to the devices inside the PCI bus.
> This is likely not a problem if all devices are directly connected to
> the hardware, but it could be an issue if someone uses either a M.2 or
> a mini-PCI extensor.
The eye diagram only applies to the link between the RP and switch.
There's no way that any of the downstream PERSTs matter, that's just
not logical. So you only need to deassert PERST on that link.
What happens if you only handle the switch PERST and CLKREQ? You
should simply only discover the switch and no downstream devices.
> So, IMO, the best would be for the PHY driver to walk the child nodes
> and get all the 'reset-gpios' properties.
>
> With regards to the clock-req phandles, those should be enabled before
> the PHY clocks, in order to avoid the SError issue.
Huh? What exactly causes an SError. Has to be some bus access. But
again, it's only going to be the one for the RP link that matters
here.
> It should be easy to implement this at the the PCIe driver, but, this
> should happen in early stages at the power-on sequence (before enabling
> the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to
> walk the child nodes and get all the 'reset-gpios' properties.
>
> For the sake of avoiding to duplicate the walk-though and parsing
> logic, I would do it only at the PHY driver.
Everyone else handles this stuff in their PCIe driver. You are not special...
I have a plan to make the PERST handling common across all PCIe host
drivers and also make the PHY handling common across DWC drivers.
Don't make that harder.
Rob
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
2021-07-28 14:28 ` Rob Herring
@ 2021-07-29 10:12 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-29 10:12 UTC (permalink / raw)
To: Rob Herring
Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy
Hi Rob,
Em Wed, 28 Jul 2021 08:28:26 -0600
Rob Herring <robh@kernel.org> escreveu:
> > > pcie@f4000000 { // RP: Bus 0, Device 0
> > > compatible = "hisilicon,kirin970-pcie";
> > > ...
> > > reset-gpios = <&gpio7 0 0>; // PERST to switch
> > >
> > > pcie@0 { // PCIe switch: Bus 1, Device 0
> > > reg = <0 0 0 0 0>;
> > > compatible = "pciclass,0604";
> > > device_type = "pci";
> > >
> > > pcie@1 { // NC (Can omit this node)
> > > reg = <0x80 0 0 0 0>;
> > > compatible = "pciclass,0604";
> > > device_type = "pci";
> > > };
> > >
> > > pcie@4 { // M.2
> > > reg = <0x200 0 0 0 0>;
> >
> > Not sure what to put at reg. I suspect that the best would be to follow
> > the PEX port number, as, if one day someone decides to implement an
> > I2C driver, this might be useful.
>
> It's defined in the PCI bus binding. Basically, it's the BDF of the
> device. However, as the bus number is dynamic, I think we want to
> leave that as 0 for FDT. The function is optional and always 0 in this
> case.
Ok. I'll use 0 there.
> > > compatible = "pciclass,0604";
> > > device_type = "pci";
> > > reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot
> >
> > We also need the clock-req phandle for the three devices.
>
> Hopefully, you can figure out where those belong now...
I added it here too.
> > With regards to the clock-req phandles, those should be enabled before
> > the PHY clocks, in order to avoid the SError issue.
>
> Huh? What exactly causes an SError. Has to be some bus access.
I'm not sure, but I got lots of those when playing with drivers
for this SoC. It is not easy to check the root case when such
errors happen, as the backtrace is useless.
>
> > It should be easy to implement this at the the PCIe driver, but, this
> > should happen in early stages at the power-on sequence (before enabling
> > the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to
> > walk the child nodes and get all the 'reset-gpios' properties.
> >
> > For the sake of avoiding to duplicate the walk-though and parsing
> > logic, I would do it only at the PHY driver.
>
> Everyone else handles this stuff in their PCIe driver. You are not special...
>
> I have a plan to make the PERST handling common across all PCIe host
> drivers and also make the PHY handling common across DWC drivers.
> Don't make that harder.
It turns that changing this to work the way you're expecting was
a lot simpler than I was expecting ;-)
The enclosed patch, applied on the top of my past series, does that.
I'll rebase my patch series to take this change into account.
Could you please check if the DTS there is OK from your side?
I'll then change the dt-schema to match such change.
Thanks,
Mauro
[PATCH] PCI: kirin: change DT schema to use child nodes
Instead of having multiple PERST# pins as part of pcie,
add PCIe child slots, and place there the information related
to clock request and PERST# signals.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
index cae90cd0b06a..a17562bb4dff 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
@@ -687,9 +687,6 @@ pcie_phy: pcie-phy@fc000000 {
"apb_phy", "apb_sys",
"aclk";
- clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >,
- <&gpio17 0 0 >;
-
/* vboost iboost pre post main */
hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
0xFFFFFFFF 0xFFFFFFFF
@@ -726,8 +723,40 @@ &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
&gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
<0x0 0 0 4
&gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
- reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
- <&gpio3 1 0 >, <&gpio27 4 0 >;
+ reset-gpios = <&gpio7 0 0 >;
+
+ pcie@4 { // Lane 4: M.2
+ reg = <0 0 0 0 0>;
+ compatible = "pciclass,0604";
+ device_type = "pci";
+ reset-gpios = <&gpio7 1 0>;
+ clkreq-gpios = <&gpio27 3 0 >;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges;
+ };
+
+ pcie@5 { // Lane 5: Mini PCIe
+ reg = <0 0 0 0 0>;
+ compatible = "pciclass,0604";
+ device_type = "pci";
+ reset-gpios = <&gpio7 2 0>;
+ clkreq-gpios = <&gpio17 0 0 >;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges;
+ };
+
+ pcie@7 { // Lane 7: Ethernet
+ reg = <0 0 0 0 0>;
+ compatible = "pciclass,0604";
+ device_type = "pci";
+ reset-gpios = <&gpio7 3 0>;
+ clkreq-gpios = <&gpio20 0 0 >;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges;
+ };
};
/* UFS */
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 9dad14929538..5f515c4c6076 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -52,6 +52,19 @@
#define PCIE_DEBOUNCE_PARAM 0xF0F400
#define PCIE_OE_BYPASS (0x3 << 28)
+/*
+ * Max number of connected PCI slots at an external PCI bridge
+ *
+ * This is used on HiKey 970, which has a PEX 8606 bridge with has
+ * 4 connected lanes (lane 0 upstream, and the other tree lanes,
+ * one connected to an in-board Ethernet adapter and the other two
+ * connected to M.2 and mini PCI slots.
+ *
+ * Each slot has a different clock source and uses a separate PERST#
+ * pin.
+ */
+#define MAX_PCI_SLOTS 3
+
enum pcie_kirin_phy_type {
PCIE_KIRIN_INTERNAL_PHY,
PCIE_KIRIN_EXTERNAL_PHY
@@ -64,6 +77,18 @@ struct kirin_pcie {
struct regmap *apb;
struct phy *phy;
void *phy_priv; /* only for PCIE_KIRIN_INTERNAL_PHY */
+
+ /* DWC PERST# */
+ int gpio_id_dwc_perst;
+
+ int num_slots;
+ /* Per-slot PERST# */
+ int gpio_id_reset[MAX_PCI_SLOTS];
+ const char *reset_names[MAX_PCI_SLOTS];
+
+ /* Per-slot clkreq */
+ int gpio_id_clkreq[MAX_PCI_SLOTS];
+ const char *clkreq_names[MAX_PCI_SLOTS];
};
/*
@@ -108,7 +133,6 @@ struct hi3660_pcie_phy {
struct clk *phy_ref_clk;
struct clk *aclk;
struct clk *aux_clk;
- int gpio_id_reset;
};
/* Registers in PCIePHY */
@@ -171,16 +195,6 @@ static int hi3660_pcie_phy_get_resource(struct hi3660_pcie_phy *phy)
if (IS_ERR(phy->sysctrl))
return PTR_ERR(phy->sysctrl);
- /* gpios */
- phy->gpio_id_reset = of_get_named_gpio(dev->of_node,
- "reset-gpios", 0);
- if (phy->gpio_id_reset == -EPROBE_DEFER) {
- return -EPROBE_DEFER;
- } else if (!gpio_is_valid(phy->gpio_id_reset)) {
- dev_err(phy->dev, "unable to get a valid gpio pin\n");
- return -ENODEV;
- }
-
return 0;
}
@@ -297,16 +311,6 @@ static int hi3660_pcie_phy_power_on(struct kirin_pcie *pcie)
if (ret)
goto disable_clks;
- /* perst assert Endpoint */
- if (!gpio_request(phy->gpio_id_reset, "pcie_perst")) {
- usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
- ret = gpio_direction_output(phy->gpio_id_reset, 1);
- if (ret)
- goto disable_clks;
- usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
- return 0;
- }
-
disable_clks:
hi3660_pcie_phy_clk_ctrl(phy, false);
return ret;
@@ -347,11 +351,54 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
.reg_stride = 4,
};
+static int kirin_pcie_parse_port(struct kirin_pcie *kirin_pcie,
+ struct platform_device *pdev,
+ struct device_node *node)
+{
+ struct device *dev = &pdev->dev;
+ int i = kirin_pcie->num_slots;
+ char name[32];
+
+ if (i + 1 > MAX_PCI_SLOTS) {
+ dev_err(dev, "Too many PCI slots!\n");
+ return -EINVAL;
+ }
+
+ kirin_pcie->num_slots++;
+
+ /* perst reset gpio */
+ kirin_pcie->gpio_id_reset[i] = of_get_named_gpio(node,
+ "reset-gpios", 0);
+
+ if (kirin_pcie->gpio_id_reset[i] < 0) {
+ dev_err(dev, "%d: Missing PERST# for %pOF\n", i, node);
+ return kirin_pcie->gpio_id_reset[i];
+ }
+
+ /* clkreq gpio */
+ kirin_pcie->gpio_id_clkreq[i] = of_get_named_gpio(node,
+ "clkreq-gpios", 0);
+ if (kirin_pcie->gpio_id_clkreq[i] < 0) {
+ dev_err(dev, "%d: Missing clqreq for %pOF\n", i, node);
+ return kirin_pcie->gpio_id_clkreq[i];
+ }
+
+ sprintf(name, "pcie_clkreq_%d", i);
+ kirin_pcie->clkreq_names[i] = devm_kstrdup_const(dev, name,
+ GFP_KERNEL);
+ if (!kirin_pcie->clkreq_names[i])
+ return -ENOMEM;
+
+ return 0;
+}
+
static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct device_node *node = dev->of_node, *child;
void __iomem *apb_base;
+ int ret;
apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
if (IS_ERR(apb_base))
@@ -362,6 +409,29 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
if (IS_ERR(kirin_pcie->apb))
return PTR_ERR(kirin_pcie->apb);
+ /* pcie internal PERST# gpio */
+ kirin_pcie->gpio_id_dwc_perst = of_get_named_gpio(dev->of_node,
+ "reset-gpios", 0);
+ if (kirin_pcie->gpio_id_dwc_perst == -EPROBE_DEFER) {
+ return -EPROBE_DEFER;
+ } else if (!gpio_is_valid(kirin_pcie->gpio_id_dwc_perst)) {
+ dev_err(dev, "unable to get a valid gpio pin\n");
+ return -ENODEV;
+ }
+
+ /* Parse OF children */
+ for_each_available_child_of_node(node, child) {
+ ret = of_pci_get_devfn(child);
+ if (ret < 0) {
+ dev_info(dev, "failed to parse devfn: %d\n", ret);
+ return ret;
+ }
+
+ ret = kirin_pcie_parse_port(kirin_pcie, pdev, child);
+ if (ret)
+ return ret;
+ }
+
return 0;
}
@@ -419,9 +489,31 @@ static int kirin_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn,
return PCIBIOS_SUCCESSFUL;
}
+static int kirin_pcie_add_bus(struct pci_bus *bus)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+ struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
+ int i, ret;
+
+ /* perst assert Endpoint */
+ usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
+
+ /* Send PERST# to each slot */
+ for (i = 0; i < kirin_pcie->num_slots; i++) {
+ ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
+ if (ret)
+ return ret;
+ }
+ usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
+
+ return 0;
+}
+
+
static struct pci_ops kirin_pci_ops = {
.read = kirin_pcie_rd_own_conf,
.write = kirin_pcie_wr_own_conf,
+ .add_bus = kirin_pcie_add_bus,
};
static u32 kirin_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
@@ -477,6 +569,46 @@ static int kirin_pcie_host_init(struct pcie_port *pp)
return 0;
}
+static int kirin_pcie_gpio_request(struct kirin_pcie *kirin_pcie,
+ struct device *dev)
+{
+ int ret, i;
+
+ for (i = 0; i < kirin_pcie->num_slots; i++) {
+ if (!gpio_is_valid(kirin_pcie->gpio_id_reset[i])) {
+ dev_err(dev, "unable to get a valid %s gpio\n",
+ kirin_pcie->reset_names[i]);
+ return -ENODEV;
+ }
+
+ ret = devm_gpio_request(dev, kirin_pcie->gpio_id_reset[i],
+ kirin_pcie->reset_names[i]);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < kirin_pcie->num_slots; i++) {
+ if (!gpio_is_valid(kirin_pcie->gpio_id_clkreq[i])) {
+ dev_err(dev, "unable to get a valid %s gpio\n",
+ kirin_pcie->clkreq_names[i]);
+ return -ENODEV;
+ }
+
+ ret = devm_gpio_request(dev, kirin_pcie->gpio_id_clkreq[i],
+ kirin_pcie->clkreq_names[i]);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < kirin_pcie->num_slots; i++) {
+ ret = gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 0);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
static const struct dw_pcie_ops kirin_dw_pcie_ops = {
.read_dbi = kirin_pcie_read_dbi,
.write_dbi = kirin_pcie_write_dbi,
@@ -499,24 +631,43 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
if (ret)
return ret;
- return hi3660_pcie_phy_power_on(kirin_pcie);
+ ret = kirin_pcie_gpio_request(kirin_pcie, dev);
+ if (ret)
+ return ret;
+
+ ret = hi3660_pcie_phy_power_on(kirin_pcie);
+ if (ret)
+ return ret;
+ } else {
+ kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL);
+ if (IS_ERR(kirin_pcie->phy))
+ return PTR_ERR(kirin_pcie->phy);
+
+ ret = phy_init(kirin_pcie->phy);
+ if (ret)
+ goto err;
+
+ ret = phy_power_on(kirin_pcie->phy);
+ if (ret)
+ goto err;
}
- kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL);
- if (IS_ERR(kirin_pcie->phy))
- return PTR_ERR(kirin_pcie->phy);
+ /* perst assert Endpoint */
+ usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
- ret = phy_init(kirin_pcie->phy);
- if (ret)
- goto err;
+ if (!gpio_request(kirin_pcie->gpio_id_dwc_perst, "pcie_perst")) {
+ ret = gpio_direction_output(kirin_pcie->gpio_id_dwc_perst, 1);
+ if (ret)
+ goto err;
+ }
- ret = phy_power_on(kirin_pcie->phy);
- if (ret)
- goto err;
+ usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
return 0;
err:
- phy_exit(kirin_pcie->phy);
+ if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
+ phy_exit(kirin_pcie->phy);
+
return ret;
}
diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c
index 82cc5fc4eac2..ac6052a05788 100644
--- a/drivers/phy/hisilicon/phy-hi3670-pcie.c
+++ b/drivers/phy/hisilicon/phy-hi3670-pcie.c
@@ -87,9 +87,6 @@
#define NOC_PW_MASK 0x10000
#define NOC_PW_SET_BIT 0x1
-/* Number of GPIOs required by PHY */
-#define MAX_GPIO_RESETS 4
-#define MAX_GPIO_CLKREQ 3
#define NUM_EYEPARAM 5
/* info located in sysctrl */
@@ -108,10 +105,6 @@
#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000
/* Time for delay */
-#define REF_2_PERST_MIN 20000
-#define REF_2_PERST_MAX 25000
-#define PERST_2_ACCESS_MIN 10000
-#define PERST_2_ACCESS_MAX 12000
#define PIPE_CLK_WAIT_MIN 550
#define PIPE_CLK_WAIT_MAX 600
#define TIME_CMOS_MIN 100
@@ -131,12 +124,6 @@ struct hi3670_pcie_phy {
struct clk *phy_ref_clk;
struct clk *aclk;
struct clk *aux_clk;
- int n_gpio_resets;
- int n_gpio_clkreq;
- int gpio_id_reset[MAX_GPIO_RESETS];
- const char *reset_names[MAX_GPIO_RESETS];
- int gpio_id_clkreq[MAX_GPIO_CLKREQ];
- const char *clkreq_names[MAX_GPIO_CLKREQ];
u32 eye_param[NUM_EYEPARAM];
};
@@ -230,40 +217,6 @@ static void hi3670_pcie_set_eyeparam(struct hi3670_pcie_phy *phy)
kirin_apb_natural_phy_writel(phy, val, LANEN_DIG_ASIC_TX_OVRD_IN_1);
}
-static int hi3670_pcie_gpio_request(struct hi3670_pcie_phy *phy,
- struct device *dev)
-{
- int ret, i;
-
- for (i = 0; i < phy->n_gpio_resets; i++) {
- if (!gpio_is_valid(phy->gpio_id_reset[i])) {
- dev_err(dev, "unable to get a valid %s gpio\n",
- phy->reset_names[i]);
- return -ENODEV;
- }
-
- ret = devm_gpio_request(dev, phy->gpio_id_reset[i],
- phy->reset_names[i]);
- if (ret)
- return ret;
- }
-
- for (i = 0; i < phy->n_gpio_clkreq; i++) {
- if (!gpio_is_valid(phy->gpio_id_clkreq[i])) {
- dev_err(dev, "unable to get a valid %s gpio\n",
- phy->clkreq_names[i]);
- return -ENODEV;
- }
-
- ret = devm_gpio_request(dev, phy->gpio_id_clkreq[i],
- phy->clkreq_names[i]);
- if (ret)
- return ret;
- }
-
- return ret;
-}
-
static void hi3670_pcie_natural_cfg(struct hi3670_pcie_phy *phy)
{
u32 val;
@@ -558,8 +511,6 @@ static int hi3670_pcie_get_resources_from_pcie(struct hi3670_pcie_phy *phy)
struct device_node *pcie_port;
struct device *dev = phy->dev;
struct device *pcie_dev;
- char name[32];
- int i;
pcie_port = of_get_child_by_name(dev->parent->of_node, "pcie");
if (!pcie_port) {
@@ -588,27 +539,6 @@ static int hi3670_pcie_get_resources_from_pcie(struct hi3670_pcie_phy *phy)
return -ENODEV;
}
- /* perst reset gpios */
- phy->n_gpio_resets = of_gpio_named_count(pcie_dev->of_node,
- "reset-gpios");
- if (phy->n_gpio_resets > MAX_GPIO_RESETS) {
- dev_err(dev, "Too many GPIO resets!\n");
- return -EINVAL;
- }
- for (i = 0; i < phy->n_gpio_resets; i++) {
- phy->gpio_id_reset[i] = of_get_named_gpio(pcie_dev->of_node,
- "reset-gpios", i);
- if (phy->gpio_id_reset[i] < 0)
- return phy->gpio_id_reset[i];
-
- sprintf(name, "pcie_perst_%d", i);
-
- phy->reset_names[i] = devm_kstrdup_const(dev, name,
- GFP_KERNEL);
- if (!phy->reset_names[i])
- return -ENOMEM;
- }
-
return 0;
}
@@ -663,7 +593,6 @@ static int kirin_pcie_clk_ctrl(struct hi3670_pcie_phy *phy, bool enable)
static int hi3670_pcie_phy_init(struct phy *generic_phy)
{
struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
- struct device *dev = phy->dev;
int ret;
/*
@@ -681,15 +610,6 @@ static int hi3670_pcie_phy_init(struct phy *generic_phy)
if (ret)
return ret;
- /* phy regulator needs to be powered on before calling it */
- return hi3670_pcie_gpio_request(phy, dev);
-}
-
-static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
-{
- struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
- int val, ret, i;
-
/* Power supply for Host */
regmap_write(phy->sysctrl,
SCTRL_PCIE_CMOS_OFFSET, SCTRL_PCIE_CMOS_BIT);
@@ -697,11 +617,13 @@ static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
hi3670_pcie_phy_oe_enable(phy);
- for (i = 0; i < phy->n_gpio_clkreq; i++) {
- ret = gpio_direction_output(phy->gpio_id_clkreq[i], 0);
- if (ret)
- return ret;
- }
+ return 0;
+}
+
+static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
+{
+ struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+ int val, ret;
ret = kirin_pcie_clk_ctrl(phy, true);
if (ret)
@@ -732,15 +654,6 @@ static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
regmap_write(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, val);
udelay(10);
- /* perst assert Endpoints */
- usleep_range(21000, 23000);
- for (i = 0; i < phy->n_gpio_resets; i++) {
- ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
- if (ret)
- return ret;
- }
- usleep_range(10000, 11000);
-
ret = is_pipe_clk_stable(phy);
if (!ret)
goto disable_clks;
@@ -781,9 +694,6 @@ static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *phy,
struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct device_node *np = dev->of_node;
- char name[32];
- int i;
/* syscon */
phy->crgctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3670-crgctrl");
@@ -824,25 +734,6 @@ static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *phy,
if (IS_ERR(phy->base))
return PTR_ERR(phy->base);
- /* clock request gpios */
- phy->n_gpio_clkreq = of_gpio_named_count(np, "clkreq-gpios");
- if (phy->n_gpio_clkreq > MAX_GPIO_CLKREQ) {
- dev_err(dev, "Too many GPIO clock requests!\n");
- return -EINVAL;
- }
- for (i = 0; i < phy->n_gpio_clkreq; i++) {
- phy->gpio_id_clkreq[i] = of_get_named_gpio(dev->of_node,
- "clkreq-gpios", i);
- if (phy->gpio_id_clkreq[i] < 0)
- return phy->gpio_id_clkreq[i];
-
- sprintf(name, "pcie_clkreq_%d", i);
- phy->clkreq_names[i] = devm_kstrdup_const(dev, name,
- GFP_KERNEL);
- if (!phy->clkreq_names[i])
- return -ENOMEM;
- }
-
hi3670_pcie_get_eyeparam(phy);
return 0;
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v7 07/10] phy: HiSilicon: Add driver for Kirin 970 PCIe PHY
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
` (5 preceding siblings ...)
2021-07-21 8:39 ` [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-21 8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
` (3 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
Krzysztof Wilczyński, Binghui Wang, Greg Kroah-Hartman,
Kishon Vijay Abraham I, Lorenzo Pieralisi, Manivannan Sadhasivam,
Xiaowei Song, linux-kernel, linux-pci, linux-phy
The Kirin 970 PHY is somewhat similar to the Kirin 960, but it
does a lot more. Add the needed bits for PCIe to start working on
HiKey 970 boards.
Co-developed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/pci/controller/dwc/pcie-kirin.c | 4 +
drivers/phy/hisilicon/Kconfig | 10 +
drivers/phy/hisilicon/Makefile | 1 +
drivers/phy/hisilicon/phy-hi3670-pcie.c | 898 ++++++++++++++++++++++++
4 files changed, 913 insertions(+)
create mode 100644 drivers/phy/hisilicon/phy-hi3670-pcie.c
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index a32f7f36461c..bfc0513f7b15 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -550,6 +550,10 @@ static const struct of_device_id kirin_pcie_match[] = {
.compatible = "hisilicon,kirin960-pcie",
.data = (void *)PCIE_KIRIN_INTERNAL_PHY
},
+ {
+ .compatible = "hisilicon,kirin970-pcie",
+ .data = (void *)PCIE_KIRIN_EXTERNAL_PHY
+ },
{},
};
diff --git a/drivers/phy/hisilicon/Kconfig b/drivers/phy/hisilicon/Kconfig
index 4d008cfc279c..d3b92c288554 100644
--- a/drivers/phy/hisilicon/Kconfig
+++ b/drivers/phy/hisilicon/Kconfig
@@ -33,6 +33,16 @@ config PHY_HI3670_USB
To compile this driver as a module, choose M here.
+config PHY_HI3670_PCIE
+ tristate "hi3670 PCIe PHY support"
+ depends on (ARCH_HISI && ARM64) || COMPILE_TEST
+ select GENERIC_PHY
+ select MFD_SYSCON
+ help
+ Enable this to support the HiSilicon hi3670 PCIe PHY.
+
+ To compile this driver as a module, choose M here.
+
config PHY_HISTB_COMBPHY
tristate "HiSilicon STB SoCs COMBPHY support"
depends on (ARCH_HISI && ARM64) || COMPILE_TEST
diff --git a/drivers/phy/hisilicon/Makefile b/drivers/phy/hisilicon/Makefile
index 51729868145b..4029d3813b1e 100644
--- a/drivers/phy/hisilicon/Makefile
+++ b/drivers/phy/hisilicon/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_PHY_HI6220_USB) += phy-hi6220-usb.o
obj-$(CONFIG_PHY_HI3660_USB) += phy-hi3660-usb3.o
obj-$(CONFIG_PHY_HI3670_USB) += phy-hi3670-usb3.o
+obj-$(CONFIG_PHY_HI3670_PCIE) += phy-hi3670-pcie.o
obj-$(CONFIG_PHY_HISTB_COMBPHY) += phy-histb-combphy.o
obj-$(CONFIG_PHY_HISI_INNO_USB2) += phy-hisi-inno-usb2.o
obj-$(CONFIG_PHY_HIX5HD2_SATA) += phy-hix5hd2-sata.o
diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c
new file mode 100644
index 000000000000..0bc0203e9140
--- /dev/null
+++ b/drivers/phy/hisilicon/phy-hi3670-pcie.c
@@ -0,0 +1,898 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe phy driver for Kirin 970
+ *
+ * Copyright (C) 2017 HiSilicon Electronics Co., Ltd.
+ * https://www.huawei.com
+ * Copyright (C) 2021 Huawei Technologies Co., Ltd.
+ * https://www.huawei.com
+ *
+ * Authors:
+ * Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
+ * Manivannan Sadhasivam <mani@kernel.org>
+ *
+ * Based on:
+ * https://lore.kernel.org/lkml/4c9d6581478aa966698758c0420933f5defab4dd.1612335031.git.mchehab+huawei@kernel.org/
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define AXI_CLK_FREQ 207500000
+#define REF_CLK_FREQ 100000000
+
+/* PCIe CTRL registers */
+#define SOC_PCIECTRL_CTRL0_ADDR 0x000
+#define SOC_PCIECTRL_CTRL1_ADDR 0x004
+#define SOC_PCIECTRL_CTRL7_ADDR 0x01c
+#define SOC_PCIECTRL_CTRL12_ADDR 0x030
+#define SOC_PCIECTRL_CTRL20_ADDR 0x050
+#define SOC_PCIECTRL_CTRL21_ADDR 0x054
+#define SOC_PCIECTRL_STATE0_ADDR 0x400
+
+/* PCIe PHY registers */
+#define SOC_PCIEPHY_CTRL0_ADDR 0x000
+#define SOC_PCIEPHY_CTRL1_ADDR 0x004
+#define SOC_PCIEPHY_CTRL2_ADDR 0x008
+#define SOC_PCIEPHY_CTRL3_ADDR 0x00c
+#define SOC_PCIEPHY_CTRL38_ADDR 0x0098
+#define SOC_PCIEPHY_STATE0_ADDR 0x400
+
+#define PCIE_LINKUP_ENABLE (0x8020)
+#define PCIE_ELBI_SLV_DBI_ENABLE (0x1 << 21)
+#define PCIE_LTSSM_ENABLE_BIT (0x1 << 11)
+#define PCIEPHY_RESET_BIT (0x1 << 17)
+#define PCIEPHY_PIPE_LINE0_RESET_BIT (0x1 << 19)
+
+#define PORT_MSI_CTRL_ADDR 0x820
+#define PORT_MSI_CTRL_UPPER_ADDR 0x824
+#define PORT_MSI_CTRL_INT0_ENABLE 0x828
+
+#define EYEPARAM_NOCFG 0xFFFFFFFF
+#define RAWLANEN_DIG_PCS_XF_TX_OVRD_IN_1 0x3001
+#define SUP_DIG_LVL_OVRD_IN 0xf
+#define LANEN_DIG_ASIC_TX_OVRD_IN_1 0x1002
+#define LANEN_DIG_ASIC_TX_OVRD_IN_2 0x1003
+
+/* hi3670 pciephy register */
+#define SOC_PCIEPHY_MMC1PLL_CTRL1 0xc04
+#define SOC_PCIEPHY_MMC1PLL_CTRL16 0xC40
+#define SOC_PCIEPHY_MMC1PLL_CTRL17 0xC44
+#define SOC_PCIEPHY_MMC1PLL_CTRL20 0xC50
+#define SOC_PCIEPHY_MMC1PLL_CTRL21 0xC54
+#define SOC_PCIEPHY_MMC1PLL_STAT0 0xE00
+
+#define CRGPERIPH_PEREN12 0x470
+#define CRGPERIPH_PERDIS12 0x474
+#define CRGPERIPH_PCIECTRL0 0x800
+
+/* define ie,oe cfg */
+#define IO_IE_EN_HARD_BYPASS (0x1 << 27)
+#define IO_OE_EN_HARD_BYPASS (0x1 << 11)
+#define IO_HARD_CTRL_DEBOUNCE_BYPASS (0x1 << 10)
+#define IO_OE_GT_MODE (0x2 << 7)
+#define DEBOUNCE_WAITCFG_IN (0xf << 20)
+#define DEBOUNCE_WAITCFG_OUT (0xf << 13)
+
+/* noc power domain */
+#define NOC_POWER_IDLEREQ_1 0x38c
+#define NOC_POWER_IDLE_1 0x394
+#define NOC_PW_MASK 0x10000
+#define NOC_PW_SET_BIT 0x1
+
+/* Number of GPIOs required by PHY */
+#define MAX_GPIO_RESETS 4
+#define MAX_GPIO_CLKREQ 3
+#define NUM_EYEPARAM 5
+
+/* info located in sysctrl */
+#define SCTRL_PCIE_CMOS_OFFSET 0x60
+#define SCTRL_PCIE_CMOS_BIT 0x10
+#define SCTRL_PCIE_ISO_OFFSET 0x44
+#define SCTRL_PCIE_ISO_BIT 0x30
+#define SCTRL_PCIE_HPCLK_OFFSET 0x190
+#define SCTRL_PCIE_HPCLK_BIT 0x184000
+#define SCTRL_PCIE_OE_OFFSET 0x14a
+#define PCIE_DEBOUNCE_PARAM 0xF0F400
+#define PCIE_OE_BYPASS (0x3 << 28)
+
+/* peri_crg ctrl */
+#define CRGCTRL_PCIE_ASSERT_OFFSET 0x88
+#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000
+
+/* Time for delay */
+#define REF_2_PERST_MIN 20000
+#define REF_2_PERST_MAX 25000
+#define PERST_2_ACCESS_MIN 10000
+#define PERST_2_ACCESS_MAX 12000
+#define PIPE_CLK_WAIT_MIN 550
+#define PIPE_CLK_WAIT_MAX 600
+#define TIME_CMOS_MIN 100
+#define TIME_CMOS_MAX 105
+#define TIME_PHY_PD_MIN 10
+#define TIME_PHY_PD_MAX 11
+
+struct hi3670_pcie_phy {
+ struct device *dev;
+ void __iomem *base;
+ struct regmap *apb;
+ struct regmap *crgctrl;
+ struct regmap *sysctrl;
+ struct regmap *pmctrl;
+ struct clk *apb_sys_clk;
+ struct clk *apb_phy_clk;
+ struct clk *phy_ref_clk;
+ struct clk *aclk;
+ struct clk *aux_clk;
+ int n_gpio_resets;
+ int n_gpio_clkreq;
+ int gpio_id_reset[MAX_GPIO_RESETS];
+ const char *reset_names[MAX_GPIO_RESETS];
+ int gpio_id_clkreq[MAX_GPIO_CLKREQ];
+ const char *clkreq_names[MAX_GPIO_CLKREQ];
+ u32 eye_param[NUM_EYEPARAM];
+};
+
+
+/* Registers in PCIePHY */
+static inline void hi3670_apb_phy_writel(struct hi3670_pcie_phy *phy,
+ u32 val, u32 reg)
+{
+ writel(val, phy->base + 0x40000 + reg);
+}
+
+static inline u32 hi3670_apb_phy_readl(struct hi3670_pcie_phy *phy, u32 reg)
+{
+ return readl(phy->base + 0x40000 + reg);
+}
+
+static inline void kirin_apb_natural_phy_writel(struct hi3670_pcie_phy *phy,
+ u32 val, u32 reg)
+{
+ writel(val, phy->base + reg * 4);
+}
+
+static inline u32 kirin_apb_natural_phy_readl(struct hi3670_pcie_phy *phy,
+ u32 reg)
+{
+ return readl(phy->base + reg * 4);
+}
+
+static void hi3670_pcie_phy_oe_enable(struct hi3670_pcie_phy *phy)
+{
+ u32 val;
+
+ regmap_read(phy->sysctrl, SCTRL_PCIE_OE_OFFSET, &val);
+ val |= PCIE_DEBOUNCE_PARAM;
+ val &= ~PCIE_OE_BYPASS;
+ regmap_write(phy->sysctrl, SCTRL_PCIE_OE_OFFSET, val);
+}
+
+static void hi3670_pcie_get_eyeparam(struct hi3670_pcie_phy *phy)
+{
+ struct device *dev = phy->dev;
+ struct device_node *np;
+ int ret, i;
+
+ np = dev->of_node;
+
+ ret = of_property_read_u32_array(np, "hisilicon,eye-diagram-param",
+ phy->eye_param, NUM_EYEPARAM);
+ if (!ret)
+ return;
+
+ /* There's no optional eye_param property. Set array to default */
+ for (i = 0; i < NUM_EYEPARAM; i++)
+ phy->eye_param[i] = EYEPARAM_NOCFG;
+}
+
+static void hi3670_pcie_set_eyeparam(struct hi3670_pcie_phy *phy)
+{
+ u32 val;
+
+ val = kirin_apb_natural_phy_readl(phy, RAWLANEN_DIG_PCS_XF_TX_OVRD_IN_1);
+
+ if (phy->eye_param[1] != EYEPARAM_NOCFG) {
+ val &= (~0xf00);
+ val |= (phy->eye_param[1] << 8) | (0x1 << 12);
+ }
+ kirin_apb_natural_phy_writel(phy, val, RAWLANEN_DIG_PCS_XF_TX_OVRD_IN_1);
+
+ val = kirin_apb_natural_phy_readl(phy, LANEN_DIG_ASIC_TX_OVRD_IN_2);
+ val &= (~0x1FBF);
+ if (phy->eye_param[2] != EYEPARAM_NOCFG)
+ val |= (phy->eye_param[2]<< 0) | (0x1 << 6);
+
+ if (phy->eye_param[3] != EYEPARAM_NOCFG)
+ val |= (phy->eye_param[3] << 7) | (0x1 << 13);
+
+ kirin_apb_natural_phy_writel(phy, val, LANEN_DIG_ASIC_TX_OVRD_IN_2);
+
+ val = kirin_apb_natural_phy_readl(phy, SUP_DIG_LVL_OVRD_IN);
+ if (phy->eye_param[0] != EYEPARAM_NOCFG) {
+ val &= (~0x1C0);
+ val |= (phy->eye_param[0] << 6) | (0x1 << 9);
+ }
+ kirin_apb_natural_phy_writel(phy, val, SUP_DIG_LVL_OVRD_IN);
+
+ val = kirin_apb_natural_phy_readl(phy, LANEN_DIG_ASIC_TX_OVRD_IN_1);
+ if (phy->eye_param[4] != EYEPARAM_NOCFG) {
+ val &= (~0x7E00);
+ val |= (phy->eye_param[4] << 9) | (0x1 << 15);
+ }
+ kirin_apb_natural_phy_writel(phy, val, LANEN_DIG_ASIC_TX_OVRD_IN_1);
+}
+
+static int hi3670_pcie_gpio_request(struct hi3670_pcie_phy *phy,
+ struct device *dev)
+{
+ int ret, i;
+
+ for (i = 0; i < phy->n_gpio_resets; i++) {
+ if (!gpio_is_valid(phy->gpio_id_reset[i])) {
+ dev_err(dev, "unable to get a valid %s gpio\n",
+ phy->reset_names[i]);
+ return -ENODEV;
+ }
+
+ ret = devm_gpio_request(dev, phy->gpio_id_reset[i],
+ phy->reset_names[i]);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < phy->n_gpio_clkreq; i++) {
+ if (!gpio_is_valid(phy->gpio_id_clkreq[i])) {
+ dev_err(dev, "unable to get a valid %s gpio\n",
+ phy->clkreq_names[i]);
+ return -ENODEV;
+ }
+
+ ret = devm_gpio_request(dev, phy->gpio_id_clkreq[i],
+ phy->clkreq_names[i]);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
+static void hi3670_pcie_natural_cfg(struct hi3670_pcie_phy *phy)
+{
+ u32 val;
+
+ /* change 2p mem_ctrl */
+ regmap_write(phy->apb, SOC_PCIECTRL_CTRL20_ADDR, 0x02605550);
+
+ /* pull up sys_aux_pwr_det */
+ regmap_read(phy->apb, SOC_PCIECTRL_CTRL7_ADDR, &val);
+ val |= (0x1 << 10);
+ regmap_write(phy->apb, SOC_PCIECTRL_CTRL7_ADDR, val);
+
+ /* output, pull down */
+ regmap_read(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, &val);
+ val &= ~(0x3 << 2);
+ val |= (0x1 << 1);
+ val &= ~(0x1 << 0);
+ regmap_write(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, val);
+
+ /* Handle phy_reset and lane0_reset to HW */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL1_ADDR);
+ val |= PCIEPHY_RESET_BIT;
+ val &= ~PCIEPHY_PIPE_LINE0_RESET_BIT;
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL1_ADDR);
+
+ /* fix chip bug: TxDetectRx fail */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL38_ADDR);
+ val |= (0x1 << 2);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL38_ADDR);
+}
+
+static void hi3670_pcie_pll_init(struct hi3670_pcie_phy *phy)
+{
+ u32 val;
+
+ /* choose FNPLL */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL1);
+ val |= (0x1 << 27);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL1);
+
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL16);
+ val &= 0xF000FFFF;
+ /* fnpll fbdiv = 0xD0 */
+ val |= (0xd0 << 16);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL16);
+
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL17);
+ val &= 0xFF000000;
+ /* fnpll fracdiv = 0x555555 */
+ val |= (0x555555 << 0);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL17);
+
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL20);
+ val &= 0xF5FF88FF;
+ /* fnpll dll_en = 0x1 */
+ val |= (0x1 << 27);
+ /* fnpll postdiv1 = 0x5 */
+ val |= (0x5 << 8);
+ /* fnpll postdiv2 = 0x4 */
+ val |= (0x4 << 12);
+ /* fnpll pll_mode = 0x0 */
+ val &= ~(0x1 << 25);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL20);
+
+ hi3670_apb_phy_writel(phy, 0x20, SOC_PCIEPHY_MMC1PLL_CTRL21);
+}
+
+static int hi3670_pcie_pll_ctrl(struct hi3670_pcie_phy *phy, bool enable)
+{
+ struct device *dev = phy->dev;
+ u32 val;
+ int time = 200;
+
+ if (enable) {
+ /* pd = 0 */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL16);
+ val &= ~(0x1 << 0);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL16);
+
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_STAT0);
+
+ /* choose FNPLL */
+ while (!(val & 0x10)) {
+ if (!time) {
+ dev_err(dev, "wait for pll_lock timeout\n");
+ return -1;
+ }
+ time --;
+ udelay(1);
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_STAT0);
+ }
+
+ /* pciepll_bp = 0 */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL20);
+ val &= ~(0x1 << 16);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL20);
+
+ } else {
+ /* pd = 1 */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL16);
+ val |= (0x1 << 0);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL16);
+
+ /* pciepll_bp = 1 */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL20);
+ val |= (0x1 << 16);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL20);
+ }
+
+ return 0;
+}
+
+static void hi3670_pcie_hp_debounce_gt(struct hi3670_pcie_phy *phy, bool open)
+{
+ if (open)
+ /* gt_clk_pcie_hp/gt_clk_pcie_debounce open */
+ regmap_write(phy->crgctrl, CRGPERIPH_PEREN12, 0x9000);
+ else
+ /* gt_clk_pcie_hp/gt_clk_pcie_debounce close */
+ regmap_write(phy->crgctrl, CRGPERIPH_PERDIS12, 0x9000);
+}
+
+static void hi3670_pcie_phyref_gt(struct hi3670_pcie_phy *phy, bool open)
+{
+ unsigned int val;
+
+ regmap_read(phy->crgctrl, CRGPERIPH_PCIECTRL0, &val);
+
+ if (open)
+ val &= ~(0x1 << 1); //enable hard gt mode
+ else
+ val |= (0x1 << 1); //disable hard gt mode
+
+ regmap_write(phy->crgctrl, CRGPERIPH_PCIECTRL0, val);
+
+ /* disable soft gt mode */
+ regmap_write(phy->crgctrl, CRGPERIPH_PERDIS12, 0x4000);
+}
+
+static void hi3670_pcie_oe_ctrl(struct hi3670_pcie_phy *phy, bool en_flag)
+{
+ unsigned int val;
+
+ regmap_read(phy->crgctrl , CRGPERIPH_PCIECTRL0, &val);
+
+ /* set ie cfg */
+ val |= IO_IE_EN_HARD_BYPASS;
+
+ /* set oe cfg */
+ val &= ~IO_HARD_CTRL_DEBOUNCE_BYPASS;
+
+ /* set phy_debounce in&out time */
+ val |= (DEBOUNCE_WAITCFG_IN | DEBOUNCE_WAITCFG_OUT);
+
+ /* select oe_gt_mode */
+ val |= IO_OE_GT_MODE;
+
+ if (en_flag)
+ val &= ~IO_OE_EN_HARD_BYPASS;
+ else
+ val |= IO_OE_EN_HARD_BYPASS;
+
+ regmap_write(phy->crgctrl, CRGPERIPH_PCIECTRL0, val);
+}
+
+static void hi3670_pcie_ioref_gt(struct hi3670_pcie_phy *phy, bool open)
+{
+ unsigned int val;
+
+ if (open) {
+ regmap_write(phy->apb, SOC_PCIECTRL_CTRL21_ADDR, 0x20000070);
+
+ hi3670_pcie_oe_ctrl(phy, true);
+
+ /* en hard gt mode */
+ regmap_read(phy->crgctrl, CRGPERIPH_PCIECTRL0, &val);
+ val &= ~(0x1 << 0);
+ regmap_write(phy->crgctrl, CRGPERIPH_PCIECTRL0, val);
+
+ /* disable soft gt mode */
+ regmap_write(phy->crgctrl, CRGPERIPH_PERDIS12, 0x2000);
+
+ } else {
+ /* disable hard gt mode */
+ regmap_read(phy->crgctrl, CRGPERIPH_PCIECTRL0, &val);
+ val |= (0x1 << 0);
+ regmap_write(phy->crgctrl, CRGPERIPH_PCIECTRL0, val);
+
+ /* disable soft gt mode */
+ regmap_write(phy->crgctrl, CRGPERIPH_PERDIS12, 0x2000);
+
+ hi3670_pcie_oe_ctrl(phy, false);
+ }
+}
+
+static int hi3670_pcie_allclk_ctrl(struct hi3670_pcie_phy *phy, bool clk_on)
+{
+ struct device *dev = phy->dev;
+ u32 val;
+ int ret = 0;
+
+ if (!clk_on)
+ goto close_clocks;
+
+ /* choose 100MHz clk src: Bit[8]==1 pad, Bit[8]==0 pll */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL1_ADDR);
+ val &= ~(0x1 << 8);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL1_ADDR);
+
+ hi3670_pcie_pll_init(phy);
+
+ ret = hi3670_pcie_pll_ctrl(phy, true);
+ if (ret) {
+ dev_err(dev, "Failed to enable pll\n");
+ return -1;
+ }
+ hi3670_pcie_hp_debounce_gt(phy, true);
+ hi3670_pcie_phyref_gt(phy, true);
+ hi3670_pcie_ioref_gt(phy, true);
+
+ ret = clk_set_rate(phy->aclk, AXI_CLK_FREQ);
+ if (ret) {
+ dev_err(dev, "Failed to set rate\n");
+ goto close_clocks;
+ }
+
+ return 0;
+
+close_clocks:
+ hi3670_pcie_ioref_gt(phy, false);
+ hi3670_pcie_phyref_gt(phy, false);
+ hi3670_pcie_hp_debounce_gt(phy, false);
+
+ hi3670_pcie_pll_ctrl(phy, false);
+
+ return ret;
+}
+
+static bool is_pipe_clk_stable(struct hi3670_pcie_phy *phy)
+{
+ struct device *dev = phy->dev;
+ u32 val;
+ u32 time = 100;
+ u32 pipe_clk_stable = 0x1 << 19;
+
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_STATE0_ADDR);
+ while (val & pipe_clk_stable) {
+ mdelay(1);
+ if (time == 0) {
+ dev_err(dev, "PIPE clk is not stable\n");
+ return false;
+ }
+ time--;
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_STATE0_ADDR);
+ }
+
+ return true;
+}
+
+static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable)
+{
+ struct device *dev = phy->dev;
+ u32 time = 100;
+ unsigned int val = NOC_PW_MASK;
+ int rst;
+
+ if (enable)
+ val = NOC_PW_MASK | NOC_PW_SET_BIT;
+ else
+ val = NOC_PW_MASK;
+ rst = enable ? 1 : 0;
+
+ regmap_write(phy->pmctrl, NOC_POWER_IDLEREQ_1, val);
+
+ time = 100;
+ regmap_read(phy->pmctrl, NOC_POWER_IDLE_1, &val);
+ while((val & NOC_PW_SET_BIT) != rst) {
+ udelay(10);
+ if (!time) {
+ dev_err(dev, "Failed to reverse noc power-status\n");
+ return -1;
+ }
+ time--;
+ regmap_read(phy->pmctrl, NOC_POWER_IDLE_1, &val);
+ }
+
+ return 0;
+}
+
+static int hi3670_pcie_get_apb(struct hi3670_pcie_phy *phy)
+{
+ struct device_node *pcie_port;
+ struct device *dev = phy->dev;
+ struct device *pcie_dev;
+
+ pcie_port = of_get_child_by_name(dev->parent->of_node, "pcie");
+ if (!pcie_port) {
+ dev_err(dev, "no pcie node found in %s\n",
+ dev->parent->of_node->full_name);
+ return -ENODEV;
+ }
+
+ pcie_dev = bus_find_device_by_of_node(&platform_bus_type, pcie_port);
+ if (!pcie_dev) {
+ dev_err(dev, "Didn't find pcie device\n");
+ return -ENODEV;
+ }
+
+ /*
+ * We might just use NULL instead of the APB name, as the
+ * pcie-kirin currently registers directly just one regmap (although
+ * the DWC driver register other regmaps).
+ *
+ * Yet, it sounds safer to warrant that it will be accessing the
+ * right regmap. So, let's use the named version.
+ */
+ phy->apb = dev_get_regmap(pcie_dev, "kirin_pcie_apb");
+ if (!phy->apb) {
+ dev_err(dev, "Failed to get APB regmap\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+
+static int kirin_pcie_clk_ctrl(struct hi3670_pcie_phy *phy, bool enable)
+{
+ int ret = 0;
+
+ if (!enable)
+ goto close_clk;
+
+ ret = clk_set_rate(phy->phy_ref_clk, REF_CLK_FREQ);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(phy->phy_ref_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(phy->apb_sys_clk);
+ if (ret)
+ goto apb_sys_fail;
+
+ ret = clk_prepare_enable(phy->apb_phy_clk);
+ if (ret)
+ goto apb_phy_fail;
+
+ ret = clk_prepare_enable(phy->aclk);
+ if (ret)
+ goto aclk_fail;
+
+ ret = clk_prepare_enable(phy->aux_clk);
+ if (ret)
+ goto aux_clk_fail;
+
+ return 0;
+
+close_clk:
+ clk_disable_unprepare(phy->aux_clk);
+aux_clk_fail:
+ clk_disable_unprepare(phy->aclk);
+aclk_fail:
+ clk_disable_unprepare(phy->apb_phy_clk);
+apb_phy_fail:
+ clk_disable_unprepare(phy->apb_sys_clk);
+apb_sys_fail:
+ clk_disable_unprepare(phy->phy_ref_clk);
+
+ return ret;
+}
+
+static int hi3670_pcie_phy_init(struct phy *generic_phy)
+{
+ struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+ struct device *dev = phy->dev;
+ int ret;
+
+ /*
+ * The code under hi3670_pcie_get_apb() need to access the
+ * DWC APB registers. So, get them from
+ * the pcie driver's regmap (see pcie-kirin regmap).
+ *
+ * Such kind of resource can only be obtained during the PCIe
+ * power_on sequence, as the code inside pcie-kirin needs to
+ * be already probed, as it needs to register the APB regmap.
+ */
+
+ ret = hi3670_pcie_get_apb(phy);
+ if (ret)
+ return ret;
+
+ /* phy regulator needs to be powered on before calling it */
+ return hi3670_pcie_gpio_request(phy, dev);
+}
+
+static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
+{
+ struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+ int val, ret, i;
+
+ /* Power supply for Host */
+ regmap_write(phy->sysctrl,
+ SCTRL_PCIE_CMOS_OFFSET, SCTRL_PCIE_CMOS_BIT);
+ usleep_range(TIME_CMOS_MIN, TIME_CMOS_MAX);
+
+ hi3670_pcie_phy_oe_enable(phy);
+
+ for (i = 0; i < phy->n_gpio_clkreq; i++) {
+ ret = gpio_direction_output(phy->gpio_id_clkreq[i], 0);
+ if (ret)
+ return ret;
+ }
+
+ ret = kirin_pcie_clk_ctrl(phy, true);
+ if (ret)
+ return ret;
+
+ /* ISO disable, PCIeCtrl, PHY assert and clk gate clear */
+ regmap_write(phy->sysctrl,
+ SCTRL_PCIE_ISO_OFFSET, SCTRL_PCIE_ISO_BIT);
+ regmap_write(phy->crgctrl,
+ CRGCTRL_PCIE_ASSERT_OFFSET, CRGCTRL_PCIE_ASSERT_BIT);
+ regmap_write(phy->sysctrl,
+ SCTRL_PCIE_HPCLK_OFFSET, SCTRL_PCIE_HPCLK_BIT);
+
+ hi3670_pcie_natural_cfg(phy);
+
+ ret = hi3670_pcie_allclk_ctrl(phy, true);
+ if (ret)
+ goto disable_clks;
+
+ /* pull down phy_test_powerdown signal */
+ val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL0_ADDR);
+ val &= ~(0x1 << 22);
+ hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL0_ADDR);
+
+ /* deassert controller perst_n */
+ regmap_read(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, &val);
+ val |= (0x1 << 2);
+ regmap_write(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, val);
+ udelay(10);
+
+ /* perst assert Endpoints */
+ usleep_range(21000, 23000);
+ for (i = 0; i < phy->n_gpio_resets; i++) {
+ ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
+ if (ret)
+ return ret;
+ }
+ usleep_range(10000, 11000);
+
+ ret = is_pipe_clk_stable(phy);
+ if (!ret)
+ goto disable_clks;
+
+ hi3670_pcie_set_eyeparam(phy);
+
+ ret = hi3670_pcie_noc_power(phy, false);
+ if (ret)
+ goto disable_clks;
+
+ return 0;
+
+disable_clks:
+ kirin_pcie_clk_ctrl(phy, false);
+ return ret;
+}
+
+static int hi3670_pcie_phy_power_off(struct phy *generic_phy)
+{
+ struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+
+ /* Drop power supply for Host */
+ regmap_write(phy->sysctrl, SCTRL_PCIE_CMOS_OFFSET, 0x00);
+
+ kirin_pcie_clk_ctrl(phy, false);
+
+ return 0;
+}
+
+static const struct phy_ops hi3670_phy_ops = {
+ .init = hi3670_pcie_phy_init,
+ .power_on = hi3670_pcie_phy_power_on,
+ .power_off = hi3670_pcie_phy_power_off,
+ .owner = THIS_MODULE,
+};
+
+static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *phy,
+ struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ char name[32];
+ int i;
+
+ /* syscon */
+ phy->crgctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3670-crgctrl");
+ if (IS_ERR(phy->crgctrl))
+ return PTR_ERR(phy->crgctrl);
+
+ phy->sysctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3670-sctrl");
+ if (IS_ERR(phy->sysctrl))
+ return PTR_ERR(phy->sysctrl);
+
+ phy->pmctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3670-pmctrl");
+ if (IS_ERR(phy->sysctrl))
+ return PTR_ERR(phy->sysctrl);
+
+ /* clocks */
+ phy->phy_ref_clk = devm_clk_get(dev, "phy_ref");
+ if (IS_ERR(phy->phy_ref_clk))
+ return PTR_ERR(phy->phy_ref_clk);
+
+ phy->aux_clk = devm_clk_get(dev, "aux");
+ if (IS_ERR(phy->aux_clk))
+ return PTR_ERR(phy->aux_clk);
+
+ phy->apb_phy_clk = devm_clk_get(dev, "apb_phy");
+ if (IS_ERR(phy->apb_phy_clk))
+ return PTR_ERR(phy->apb_phy_clk);
+
+ phy->apb_sys_clk = devm_clk_get(dev, "apb_sys");
+ if (IS_ERR(phy->apb_sys_clk))
+ return PTR_ERR(phy->apb_sys_clk);
+
+ phy->aclk = devm_clk_get(dev, "aclk");
+ if (IS_ERR(phy->aclk))
+ return PTR_ERR(phy->aclk);
+
+ /* registers */
+ phy->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(phy->base))
+ return PTR_ERR(phy->base);
+
+ /* perst reset gpios */
+ phy->n_gpio_resets = of_gpio_named_count(np, "reset-gpios");
+ if (phy->n_gpio_resets > MAX_GPIO_RESETS) {
+ dev_err(dev, "Too many GPIO resets!\n");
+ return -EINVAL;
+ }
+ for (i = 0; i < phy->n_gpio_resets; i++) {
+ phy->gpio_id_reset[i] = of_get_named_gpio(dev->of_node,
+ "reset-gpios", i);
+ if (phy->gpio_id_reset[i] < 0)
+ return phy->gpio_id_reset[i];
+
+ sprintf(name, "pcie_perst_%d", i);
+
+ phy->reset_names[i] = devm_kstrdup_const(dev, name,
+ GFP_KERNEL);
+ if (!phy->reset_names[i])
+ return -ENOMEM;
+ }
+
+ /* clock request gpios */
+ phy->n_gpio_clkreq = of_gpio_named_count(np, "clkreq-gpios");
+ if (phy->n_gpio_clkreq > MAX_GPIO_CLKREQ) {
+ dev_err(dev, "Too many GPIO clock requests!\n");
+ return -EINVAL;
+ }
+ for (i = 0; i < phy->n_gpio_clkreq; i++) {
+ phy->gpio_id_clkreq[i] = of_get_named_gpio(dev->of_node,
+ "clkreq-gpios", i);
+ if (phy->gpio_id_clkreq[i] < 0)
+ return phy->gpio_id_clkreq[i];
+
+ sprintf(name, "pcie_clkreq_%d", i);
+ phy->clkreq_names[i] = devm_kstrdup_const(dev, name,
+ GFP_KERNEL);
+ if (!phy->clkreq_names[i])
+ return -ENOMEM;
+ }
+
+ hi3670_pcie_get_eyeparam(phy);
+
+ return 0;
+}
+
+static int hi3670_pcie_phy_probe(struct platform_device *pdev)
+{
+ struct phy_provider *phy_provider;
+ struct device *dev = &pdev->dev;
+ struct hi3670_pcie_phy *phy;
+ struct phy *generic_phy;
+ int ret;
+
+ phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->dev = dev;
+
+ ret = hi3670_pcie_phy_get_resources(phy, pdev);
+ if (ret)
+ return ret;
+
+ generic_phy = devm_phy_create(dev, dev->of_node, &hi3670_phy_ops);
+ if (IS_ERR(generic_phy)) {
+ dev_err(dev, "failed to create PHY\n");
+ return PTR_ERR(generic_phy);
+ }
+
+ phy_set_drvdata(generic_phy, phy);
+ phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+ return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id hi3670_pcie_phy_match[] = {
+ {
+ .compatible = "hisilicon,hi970-pcie-phy",
+ },
+ {},
+};
+
+static struct platform_driver hi3670_pcie_phy_driver = {
+ .probe = hi3670_pcie_phy_probe,
+ .driver = {
+ .of_match_table = hi3670_pcie_phy_match,
+ .name = "hi3670_pcie_phy",
+ .suppress_bind_attrs = true,
+ }
+};
+builtin_platform_driver(hi3670_pcie_phy_driver);
+
+MODULE_DEVICE_TABLE(of, hi3670_pcie_phy_match);
+MODULE_DESCRIPTION("PCIe phy driver for Kirin 970");
+MODULE_AUTHOR("Mauro Carvalho Chehab <mchehab@kernel.org>");
+MODULE_AUTHOR("Manivannan Sadhasivam <mani@kernel.org>");
+MODULE_LICENSE("GPL v2");
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
` (6 preceding siblings ...)
2021-07-21 8:39 ` [PATCH v7 07/10] phy: HiSilicon: Add driver for Kirin " Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-22 13:36 ` Manivannan Sadhasivam
2021-08-16 18:26 ` Rob Herring
2021-07-21 8:39 ` [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml Mauro Carvalho Chehab
` (2 subsequent siblings)
10 siblings, 2 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Manivannan Sadhasivam,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
linux-kernel, linux-pci, Mauro Carvalho Chehab
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Add DTS bindings for the HiKey 970 board's PCIe hardware.
Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 71 +++++++++++++++++++
.../boot/dts/hisilicon/hikey970-pmic.dtsi | 1 -
drivers/pci/controller/dwc/pcie-kirin.c | 12 ----
3 files changed, 71 insertions(+), 13 deletions(-)
diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
index 1f228612192c..6dfcfcfeedae 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
@@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
#clock-cells = <1>;
};
+ pmctrl: pmctrl@fff31000 {
+ compatible = "hisilicon,hi3670-pmctrl", "syscon";
+ reg = <0x0 0xfff31000 0x0 0x1000>;
+ #clock-cells = <1>;
+ };
+
iomcu: iomcu@ffd7e000 {
compatible = "hisilicon,hi3670-iomcu", "syscon";
reg = <0x0 0xffd7e000 0x0 0x1000>;
@@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
clock-names = "apb_pclk";
};
+ its_pcie: interrupt-controller@f4000000 {
+ compatible = "arm,gic-v3-its";
+ msi-controller;
+ reg = <0x0 0xf5100000 0x0 0x100000>;
+ };
+
+ pcie_phy: pcie-phy@fc000000 {
+ compatible = "hisilicon,hi970-pcie-phy";
+ reg = <0x0 0xfc000000 0x0 0x80000>;
+
+ phy-supply = <&ldo33>;
+
+ clocks = <&crg_ctrl HI3670_CLK_GATE_PCIEPHY_REF>,
+ <&crg_ctrl HI3670_CLK_GATE_PCIEAUX>,
+ <&crg_ctrl HI3670_PCLK_GATE_PCIE_PHY>,
+ <&crg_ctrl HI3670_PCLK_GATE_PCIE_SYS>,
+ <&crg_ctrl HI3670_ACLK_GATE_PCIE>;
+ clock-names = "phy_ref", "aux",
+ "apb_phy", "apb_sys",
+ "aclk";
+
+ reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+ <&gpio3 1 0 >, <&gpio27 4 0 >;
+
+ clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >,
+ <&gpio17 0 0 >;
+
+ /* vboost iboost pre post main */
+ hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
+ 0xFFFFFFFF 0xFFFFFFFF
+ 0xFFFFFFFF>;
+
+ #phy-cells = <0>;
+ };
+
+ pcie@f4000000 {
+ compatible = "hisilicon,kirin970-pcie";
+ reg = <0x0 0xf4000000 0x0 0x1000000>,
+ <0x0 0xfc180000 0x0 0x1000>,
+ <0x0 0xf5000000 0x0 0x2000>;
+ reg-names = "dbi", "apb", "config";
+ bus-range = <0x0 0x1>;
+ msi-parent = <&its_pcie>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ device_type = "pci";
+ phys = <&pcie_phy>;
+ ranges = <0x02000000 0x0 0x00000000
+ 0x0 0xf6000000
+ 0x0 0x02000000>;
+ num-lanes = <1>;
+ #interrupt-cells = <1>;
+ interrupts = <0 283 4>;
+ interrupt-names = "msi";
+ interrupt-map-mask = <0 0 0 7>;
+ interrupt-map = <0x0 0 0 1
+ &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+ <0x0 0 0 2
+ &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+ <0x0 0 0 3
+ &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+ <0x0 0 0 4
+ &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+ };
+
/* UFS */
ufs: ufs@ff3c0000 {
compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";
diff --git a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
index 48c739eacba0..03452e627641 100644
--- a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
@@ -73,7 +73,6 @@ ldo33: LDO33 { /* PEX8606 */
regulator-name = "ldo33";
regulator-min-microvolt = <2500000>;
regulator-max-microvolt = <3300000>;
- regulator-boot-on;
};
ldo34: LDO34 { /* GPS AUX IN VDD */
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index bfc0513f7b15..9dad14929538 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -347,18 +347,6 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
.reg_stride = 4,
};
-/* Registers in PCIeCTRL */
-static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
- u32 val, u32 reg)
-{
- writel(val, kirin_pcie->apb_base + reg);
-}
-
-static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
-{
- return readl(kirin_pcie->apb_base + reg);
-}
-
static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
struct platform_device *pdev)
{
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
2021-07-21 8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
@ 2021-07-22 13:36 ` Manivannan Sadhasivam
2021-07-23 6:53 ` Mauro Carvalho Chehab
2021-08-16 18:26 ` Rob Herring
1 sibling, 1 reply; 29+ messages in thread
From: Manivannan Sadhasivam @ 2021-07-22 13:36 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Vinod Koul, Bjorn Helgaas, Rob Herring, linuxarm, mauro.chehab,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
linux-kernel, linux-pci
On Wed, Jul 21, 2021 at 10:39:10AM +0200, Mauro Carvalho Chehab wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> Add DTS bindings for the HiKey 970 board's PCIe hardware.
>
> Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 71 +++++++++++++++++++
> .../boot/dts/hisilicon/hikey970-pmic.dtsi | 1 -
> drivers/pci/controller/dwc/pcie-kirin.c | 12 ----
> 3 files changed, 71 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> index 1f228612192c..6dfcfcfeedae 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
> #clock-cells = <1>;
> };
>
> + pmctrl: pmctrl@fff31000 {
> + compatible = "hisilicon,hi3670-pmctrl", "syscon";
> + reg = <0x0 0xfff31000 0x0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
Irrelevant change to this patch.
> iomcu: iomcu@ffd7e000 {
> compatible = "hisilicon,hi3670-iomcu", "syscon";
> reg = <0x0 0xffd7e000 0x0 0x1000>;
> @@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
> clock-names = "apb_pclk";
> };
>
[...]
> + #interrupt-cells = <1>;
> + interrupts = <0 283 4>;
Use the DT flag for interrupts instead of hardcoded value
> + interrupt-names = "msi";
> + interrupt-map-mask = <0 0 0 7>;
> + interrupt-map = <0x0 0 0 1
> + &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
> + <0x0 0 0 2
> + &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
> + <0x0 0 0 3
> + &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
> + <0x0 0 0 4
> + &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> /* UFS */
> ufs: ufs@ff3c0000 {
> compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";
> diff --git a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> index 48c739eacba0..03452e627641 100644
> --- a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> @@ -73,7 +73,6 @@ ldo33: LDO33 { /* PEX8606 */
> regulator-name = "ldo33";
> regulator-min-microvolt = <2500000>;
> regulator-max-microvolt = <3300000>;
> - regulator-boot-on;
Again, irrelevant.
> };
>
> ldo34: LDO34 { /* GPS AUX IN VDD */
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index bfc0513f7b15..9dad14929538 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -347,18 +347,6 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
> .reg_stride = 4,
> };
>
> -/* Registers in PCIeCTRL */
> -static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
> - u32 val, u32 reg)
> -{
> - writel(val, kirin_pcie->apb_base + reg);
> -}
> -
> -static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
> -{
> - return readl(kirin_pcie->apb_base + reg);
> -}
> -
Same here...
Thanks,
Mani
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
2021-07-22 13:36 ` Manivannan Sadhasivam
@ 2021-07-23 6:53 ` Mauro Carvalho Chehab
2021-07-24 4:11 ` Manivannan Sadhasivam
0 siblings, 1 reply; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-23 6:53 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Vinod Koul, Bjorn Helgaas, Rob Herring, linuxarm, mauro.chehab,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
linux-kernel, linux-pci
Em Thu, 22 Jul 2021 19:06:28 +0530
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> escreveu:
> On Wed, Jul 21, 2021 at 10:39:10AM +0200, Mauro Carvalho Chehab wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >
> > Add DTS bindings for the HiKey 970 board's PCIe hardware.
> >
> > Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 71 +++++++++++++++++++
> > .../boot/dts/hisilicon/hikey970-pmic.dtsi | 1 -
> > drivers/pci/controller/dwc/pcie-kirin.c | 12 ----
> > 3 files changed, 71 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > index 1f228612192c..6dfcfcfeedae 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
> > #clock-cells = <1>;
> > };
> >
> > + pmctrl: pmctrl@fff31000 {
> > + compatible = "hisilicon,hi3670-pmctrl", "syscon";
> > + reg = <0x0 0xfff31000 0x0 0x1000>;
> > + #clock-cells = <1>;
> > + };
> > +
>
> Irrelevant change to this patch.
Huh?
This is used by PCIe PHY, as part of the power on procedures:
+static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable)
+{
+ struct device *dev = phy->dev;
+ u32 time = 100;
+ unsigned int val = NOC_PW_MASK;
+ int rst;
+
+ if (enable)
+ val = NOC_PW_MASK | NOC_PW_SET_BIT;
+ else
+ val = NOC_PW_MASK;
+ rst = enable ? 1 : 0;
+
+ regmap_write(phy->pmctrl, NOC_POWER_IDLEREQ_1, val);
>
> > iomcu: iomcu@ffd7e000 {
> > compatible = "hisilicon,hi3670-iomcu", "syscon";
> > reg = <0x0 0xffd7e000 0x0 0x1000>;
> > @@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
> > clock-names = "apb_pclk";
> > };
> >
>
> [...]
>
> > + #interrupt-cells = <1>;
> > + interrupts = <0 283 4>;
>
> Use the DT flag for interrupts instead of hardcoded value
Do you mean like this?
interrupts = <0 283 IRQ_TYPE_LEVEL_HIGH>;
>
> > + interrupt-names = "msi";
> > + interrupt-map-mask = <0 0 0 7>;
> > + interrupt-map = <0x0 0 0 1
> > + &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
> > + <0x0 0 0 2
> > + &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
> > + <0x0 0 0 3
> > + &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
> > + <0x0 0 0 4
> > + &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
> > + };
> > +
> > /* UFS */
> > ufs: ufs@ff3c0000 {
> > compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";
> > diff --git a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> > index 48c739eacba0..03452e627641 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> > @@ -73,7 +73,6 @@ ldo33: LDO33 { /* PEX8606 */
> > regulator-name = "ldo33";
> > regulator-min-microvolt = <2500000>;
> > regulator-max-microvolt = <3300000>;
> > - regulator-boot-on;
>
> Again, irrelevant.
I'll move it to the USB patch series, where the PMIC is added.
>
> > };
> >
> > ldo34: LDO34 { /* GPS AUX IN VDD */
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index bfc0513f7b15..9dad14929538 100644
> > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > @@ -347,18 +347,6 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
> > .reg_stride = 4,
> > };
> >
> > -/* Registers in PCIeCTRL */
> > -static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
> > - u32 val, u32 reg)
> > -{
> > - writel(val, kirin_pcie->apb_base + reg);
> > -}
> > -
> > -static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
> > -{
> > - return readl(kirin_pcie->apb_base + reg);
> > -}
> > -
>
> Same here...
This hunk should be on patch 03/10. Probably some rebase added it here by
mistake. I'll fix it on v8.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
2021-07-23 6:53 ` Mauro Carvalho Chehab
@ 2021-07-24 4:11 ` Manivannan Sadhasivam
2021-08-03 4:25 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 29+ messages in thread
From: Manivannan Sadhasivam @ 2021-07-24 4:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Vinod Koul, Bjorn Helgaas, Rob Herring, linuxarm, mauro.chehab,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
linux-kernel, linux-pci
On Fri, Jul 23, 2021 at 08:53:18AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 22 Jul 2021 19:06:28 +0530
> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> escreveu:
>
> > On Wed, Jul 21, 2021 at 10:39:10AM +0200, Mauro Carvalho Chehab wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > >
> > > Add DTS bindings for the HiKey 970 board's PCIe hardware.
> > >
> > > Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > > arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 71 +++++++++++++++++++
> > > .../boot/dts/hisilicon/hikey970-pmic.dtsi | 1 -
> > > drivers/pci/controller/dwc/pcie-kirin.c | 12 ----
> > > 3 files changed, 71 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > index 1f228612192c..6dfcfcfeedae 100644
> > > --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
> > > #clock-cells = <1>;
> > > };
> > >
> > > + pmctrl: pmctrl@fff31000 {
> > > + compatible = "hisilicon,hi3670-pmctrl", "syscon";
> > > + reg = <0x0 0xfff31000 0x0 0x1000>;
> > > + #clock-cells = <1>;
> > > + };
> > > +
> >
> > Irrelevant change to this patch.
>
> Huh?
>
> This is used by PCIe PHY, as part of the power on procedures:
>
> +static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable)
> +{
> + struct device *dev = phy->dev;
> + u32 time = 100;
> + unsigned int val = NOC_PW_MASK;
> + int rst;
> +
> + if (enable)
> + val = NOC_PW_MASK | NOC_PW_SET_BIT;
> + else
> + val = NOC_PW_MASK;
> + rst = enable ? 1 : 0;
> +
> + regmap_write(phy->pmctrl, NOC_POWER_IDLEREQ_1, val);
>
>
Ah... you're hardcoding the syscon compatible in driver. Sorry missed that.
But if these syscon nodes are independent memory regions or belong to non
PCI/PHY memory map, you could've fetched the reference through a DT property
along with the offset then used it in driver.
Like,
pcie_phy: pcie-phy@fc000000 {
...
hisilicon,noc-power-regs = <&pmctrl 0x38c>;
hisilicon,sctrl-cmos-regs = <&sctrl 0x60>;
...
};
The benefit of doing this way is, if the pmctrl, sctrl register layout changes
in future, you can handle it without any issues.
>
> >
> > > iomcu: iomcu@ffd7e000 {
> > > compatible = "hisilicon,hi3670-iomcu", "syscon";
> > > reg = <0x0 0xffd7e000 0x0 0x1000>;
> > > @@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
> > > clock-names = "apb_pclk";
> > > };
> > >
> >
> > [...]
> >
> > > + #interrupt-cells = <1>;
> > > + interrupts = <0 283 4>;
> >
> > Use the DT flag for interrupts instead of hardcoded value
>
> Do you mean like this?
>
> interrupts = <0 283 IRQ_TYPE_LEVEL_HIGH>;
>
yes but you could also use,
interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>;
Thanks,
Mani
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
2021-07-24 4:11 ` Manivannan Sadhasivam
@ 2021-08-03 4:25 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-08-03 4:25 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Vinod Koul, Bjorn Helgaas, Rob Herring, linuxarm, mauro.chehab,
Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
linux-kernel, linux-pci
Em Sat, 24 Jul 2021 09:41:50 +0530
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> escreveu:
> On Fri, Jul 23, 2021 at 08:53:18AM +0200, Mauro Carvalho Chehab wrote:
> > Em Thu, 22 Jul 2021 19:06:28 +0530
> > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> escreveu:
> >
> > > On Wed, Jul 21, 2021 at 10:39:10AM +0200, Mauro Carvalho Chehab wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > >
> > > > Add DTS bindings for the HiKey 970 board's PCIe hardware.
> > > >
> > > > Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > > arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 71 +++++++++++++++++++
> > > > .../boot/dts/hisilicon/hikey970-pmic.dtsi | 1 -
> > > > drivers/pci/controller/dwc/pcie-kirin.c | 12 ----
> > > > 3 files changed, 71 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > > index 1f228612192c..6dfcfcfeedae 100644
> > > > --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > > +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > > @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
> > > > #clock-cells = <1>;
> > > > };
> > > >
> > > > + pmctrl: pmctrl@fff31000 {
> > > > + compatible = "hisilicon,hi3670-pmctrl", "syscon";
> > > > + reg = <0x0 0xfff31000 0x0 0x1000>;
> > > > + #clock-cells = <1>;
> > > > + };
> > > > +
> > >
> > > Irrelevant change to this patch.
> >
> > Huh?
> >
> > This is used by PCIe PHY, as part of the power on procedures:
> >
> > +static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable)
> > +{
> > + struct device *dev = phy->dev;
> > + u32 time = 100;
> > + unsigned int val = NOC_PW_MASK;
> > + int rst;
> > +
> > + if (enable)
> > + val = NOC_PW_MASK | NOC_PW_SET_BIT;
> > + else
> > + val = NOC_PW_MASK;
> > + rst = enable ? 1 : 0;
> > +
> > + regmap_write(phy->pmctrl, NOC_POWER_IDLEREQ_1, val);
> >
> >
>
> Ah... you're hardcoding the syscon compatible in driver. Sorry missed that.
>
> But if these syscon nodes are independent memory regions or belong to non
> PCI/PHY memory map, you could've fetched the reference through a DT property
> along with the offset then used it in driver.
>
> Like,
>
> pcie_phy: pcie-phy@fc000000 {
> ...
> hisilicon,noc-power-regs = <&pmctrl 0x38c>;
> hisilicon,sctrl-cmos-regs = <&sctrl 0x60>;
> ...
> };
>
> The benefit of doing this way is, if the pmctrl, sctrl register layout changes
> in future, you can handle it without any issues.
Interesting approach, but probably overkill. I mean, the register mapping
here should be the same for all Kirin 970 PHY based devices. A PHY for a
different SoC will likely have other differences than just those two regs.
Regards,
Mauro
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
2021-07-21 8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
2021-07-22 13:36 ` Manivannan Sadhasivam
@ 2021-08-16 18:26 ` Rob Herring
1 sibling, 0 replies; 29+ messages in thread
From: Rob Herring @ 2021-08-16 18:26 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
Manivannan Sadhasivam, Krzysztof Wilczyński, Binghui Wang,
Lorenzo Pieralisi, Wei Xu, Xiaowei Song, devicetree,
linux-arm-kernel, linux-kernel, PCI
On Wed, Jul 21, 2021 at 3:39 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> Add DTS bindings for the HiKey 970 board's PCIe hardware.
>
> Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 71 +++++++++++++++++++
> .../boot/dts/hisilicon/hikey970-pmic.dtsi | 1 -
> drivers/pci/controller/dwc/pcie-kirin.c | 12 ----
> 3 files changed, 71 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> index 1f228612192c..6dfcfcfeedae 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
> #clock-cells = <1>;
> };
>
> + pmctrl: pmctrl@fff31000 {
> + compatible = "hisilicon,hi3670-pmctrl", "syscon";
> + reg = <0x0 0xfff31000 0x0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> iomcu: iomcu@ffd7e000 {
> compatible = "hisilicon,hi3670-iomcu", "syscon";
> reg = <0x0 0xffd7e000 0x0 0x1000>;
> @@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
> clock-names = "apb_pclk";
> };
>
> + its_pcie: interrupt-controller@f4000000 {
> + compatible = "arm,gic-v3-its";
> + msi-controller;
> + reg = <0x0 0xf5100000 0x0 0x100000>;
How does this h/w have a GIC-400 (which is GICv2) and then a GIC v3 ITS?
> + };
> +
> + pcie_phy: pcie-phy@fc000000 {
> + compatible = "hisilicon,hi970-pcie-phy";
> + reg = <0x0 0xfc000000 0x0 0x80000>;
> +
> + phy-supply = <&ldo33>;
> +
> + clocks = <&crg_ctrl HI3670_CLK_GATE_PCIEPHY_REF>,
> + <&crg_ctrl HI3670_CLK_GATE_PCIEAUX>,
> + <&crg_ctrl HI3670_PCLK_GATE_PCIE_PHY>,
> + <&crg_ctrl HI3670_PCLK_GATE_PCIE_SYS>,
> + <&crg_ctrl HI3670_ACLK_GATE_PCIE>;
> + clock-names = "phy_ref", "aux",
> + "apb_phy", "apb_sys",
> + "aclk";
> +
> + reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
> + <&gpio3 1 0 >, <&gpio27 4 0 >;
> +
> + clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >,
> + <&gpio17 0 0 >;
> +
> + /* vboost iboost pre post main */
> + hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
> + 0xFFFFFFFF 0xFFFFFFFF
> + 0xFFFFFFFF>;
> +
> + #phy-cells = <0>;
> + };
> +
> + pcie@f4000000 {
> + compatible = "hisilicon,kirin970-pcie";
> + reg = <0x0 0xf4000000 0x0 0x1000000>,
> + <0x0 0xfc180000 0x0 0x1000>,
> + <0x0 0xf5000000 0x0 0x2000>;
> + reg-names = "dbi", "apb", "config";
> + bus-range = <0x0 0x1>;
> + msi-parent = <&its_pcie>;
This means the PCI host doesn't have a MSI controller...
> + #address-cells = <3>;
> + #size-cells = <2>;
> + device_type = "pci";
> + phys = <&pcie_phy>;
> + ranges = <0x02000000 0x0 0x00000000
> + 0x0 0xf6000000
> + 0x0 0x02000000>;
> + num-lanes = <1>;
> + #interrupt-cells = <1>;
> + interrupts = <0 283 4>;
> + interrupt-names = "msi";
But then this says it does...
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
` (7 preceding siblings ...)
2021-07-21 8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-23 22:56 ` Rob Herring
2021-07-21 8:39 ` [PATCH v7 10/10] phy-hi3670-pcie: Move reset-gpios to the PCIe DT schema Mauro Carvalho Chehab
2021-07-21 10:15 ` [PATCH v7 11/10] PCI: kirin: Allow building it as a module Mauro Carvalho Chehab
10 siblings, 1 reply; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Binghui Wang,
Gustavo Pimentel, Jingoo Han, Rob Herring, Xiaowei Song,
devicetree, linux-kernel, linux-pci
Convert the file into a JSON description at the yaml format.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
.../bindings/pci/hisilicon,kirin-pcie.yaml | 87 +++++++++++++++++++
.../devicetree/bindings/pci/kirin-pcie.txt | 50 -----------
.../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +-
MAINTAINERS | 2 +-
4 files changed, 89 insertions(+), 52 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
delete mode 100644 Documentation/devicetree/bindings/pci/kirin-pcie.txt
diff --git a/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml b/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
new file mode 100644
index 000000000000..eabc651c9766
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/hisilicon,kirin-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin SoCs PCIe host DT description
+
+maintainers:
+ - Xiaowei Song <songxiaowei@hisilicon.com>
+ - Binghui Wang <wangbinghui@hisilicon.com>
+
+description: |
+ Kirin PCIe host controller is based on the Synopsys DesignWare PCI core.
+ It shares common functions with the PCIe DesignWare core driver and
+ inherits common properties defined in
+ Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml.
+
+allOf:
+ - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
+properties:
+ compatible:
+ contains:
+ enum:
+ - hisilicon,kirin960-pcie
+ - hisilicon,kirin970-pcie
+
+ reg:
+ description: |
+ Should contain rc_dbi, apb, config registers location and length.
+ minItems: 3
+ maxItems: 4
+
+ reg-names:
+ items:
+ - const: dbi # controller configuration registers
+ - const: apb # apb Ctrl register defined by Kirin
+ - const: config # PCIe configuration space registers
+ - const: phy # apb PHY register used on Kirin 960 PHY
+ minItems: 3
+ maxItems: 4
+
+ reset-gpios:
+ description: The GPIO(s) to generate PCIe PERST# assert and deassert signal.
+ minItems: 1
+ maxItems: 4
+
+required:
+ - compatible
+ - reg
+ - reg-names
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ pcie: pcie@f4000000 {
+ compatible = "hisilicon,kirin970-pcie";
+ reg = <0x0 0xf4000000 0x0 0x1000>,
+ <0x0 0xff3fe000 0x0 0x1000>,
+ <0x0 0xf4000000 0 0x2000>;
+ reg-names = "dbi", "apb", "config";
+ bus-range = <0x0 0x1>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ device_type = "pci";
+ ranges = <0x02000000 0x0 0x00000000 0x0 0xf5000000 0x0 0x2000000>;
+ num-lanes = <1>;
+ #interrupt-cells = <1>;
+ interrupts = <0 283 4>;
+ interrupt-names = "msi";
+ interrupt-map-mask = <0xf800 0 0 7>;
+ interrupt-map = <0x0 0 0 1 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+ <0x0 0 0 2 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+ <0x0 0 0 3 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+ <0x0 0 0 4 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+ reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+ <&gpio3 1 0 >, <&gpio27 4 0 >;
+ };
+ };
diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
deleted file mode 100644
index 7adab8999a6a..000000000000
--- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
+++ /dev/null
@@ -1,50 +0,0 @@
-HiSilicon Kirin SoCs PCIe host DT description
-
-Kirin PCIe host controller is based on the Synopsys DesignWare PCI core.
-It shares common functions with the PCIe DesignWare core driver and
-inherits common properties defined in
-Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml.
-
-Additional properties are described here:
-
-Required properties
-- compatible:
- "hisilicon,kirin960-pcie"
-- reg: Should contain rc_dbi, apb, phy, config registers location and length.
-- reg-names: Must include the following entries:
- "dbi": controller configuration registers;
- "apb": apb Ctrl register defined by Kirin;
- "phy": apb PHY register defined by Kirin;
- "config": PCIe configuration space registers.
-- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
-
-Optional properties:
-
-Example based on kirin960:
-
- pcie@f4000000 {
- compatible = "hisilicon,kirin960-pcie";
- reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
- <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
- reg-names = "dbi","apb","phy", "config";
- bus-range = <0x0 0x1>;
- #address-cells = <3>;
- #size-cells = <2>;
- device_type = "pci";
- ranges = <0x02000000 0x0 0x00000000 0x0 0xf5000000 0x0 0x2000000>;
- num-lanes = <1>;
- #interrupt-cells = <1>;
- interrupt-map-mask = <0xf800 0 0 7>;
- interrupt-map = <0x0 0 0 1 &gic 0 0 0 282 4>,
- <0x0 0 0 2 &gic 0 0 0 283 4>,
- <0x0 0 0 3 &gic 0 0 0 284 4>,
- <0x0 0 0 4 &gic 0 0 0 285 4>;
- clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
- <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
- <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
- <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
- <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
- clock-names = "pcie_phy_ref", "pcie_aux",
- "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk";
- reset-gpios = <&gpio11 1 0 >;
- };
diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
index a8c1db879fb9..d80894a5abf5 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
@@ -34,7 +34,7 @@ properties:
minItems: 2
maxItems: 5
items:
- enum: [dbi, dbi2, config, atu, app, elbi, mgmt, ctrl, parf, cfg, link]
+ enum: [dbi, dbi2, config, atu, apb, app, elbi, mgmt, ctrl, parf, cfg, link]
num-lanes:
description: |
diff --git a/MAINTAINERS b/MAINTAINERS
index b54bd9dd07ec..d5f53b2d3f9c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14420,7 +14420,7 @@ M: Xiaowei Song <songxiaowei@hisilicon.com>
M: Binghui Wang <wangbinghui@hisilicon.com>
L: linux-pci@vger.kernel.org
S: Maintained
-F: Documentation/devicetree/bindings/pci/kirin-pcie.txt
+F: Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
F: drivers/pci/controller/dwc/pcie-kirin.c
PCIE DRIVER FOR HISILICON STB
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml
2021-07-21 8:39 ` [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml Mauro Carvalho Chehab
@ 2021-07-23 22:56 ` Rob Herring
0 siblings, 0 replies; 29+ messages in thread
From: Rob Herring @ 2021-07-23 22:56 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Vinod Koul, Bjorn Helgaas, linuxarm, mauro.chehab, Binghui Wang,
Gustavo Pimentel, Jingoo Han, Xiaowei Song, devicetree,
linux-kernel, linux-pci
On Wed, Jul 21, 2021 at 10:39:11AM +0200, Mauro Carvalho Chehab wrote:
> Convert the file into a JSON description at the yaml format.
And add 970...
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> .../bindings/pci/hisilicon,kirin-pcie.yaml | 87 +++++++++++++++++++
> .../devicetree/bindings/pci/kirin-pcie.txt | 50 -----------
> .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +-
> MAINTAINERS | 2 +-
> 4 files changed, 89 insertions(+), 52 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
> delete mode 100644 Documentation/devicetree/bindings/pci/kirin-pcie.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml b/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
> new file mode 100644
> index 000000000000..eabc651c9766
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/hisilicon,kirin-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon Kirin SoCs PCIe host DT description
> +
> +maintainers:
> + - Xiaowei Song <songxiaowei@hisilicon.com>
> + - Binghui Wang <wangbinghui@hisilicon.com>
> +
> +description: |
> + Kirin PCIe host controller is based on the Synopsys DesignWare PCI core.
> + It shares common functions with the PCIe DesignWare core driver and
> + inherits common properties defined in
> + Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml.
> +
> +allOf:
> + - $ref: /schemas/pci/snps,dw-pcie.yaml#
> +
> +properties:
> + compatible:
> + contains:
> + enum:
> + - hisilicon,kirin960-pcie
> + - hisilicon,kirin970-pcie
> +
> + reg:
> + description: |
> + Should contain rc_dbi, apb, config registers location and length.
> + minItems: 3
> + maxItems: 4
> +
> + reg-names:
> + items:
> + - const: dbi # controller configuration registers
> + - const: apb # apb Ctrl register defined by Kirin
> + - const: config # PCIe configuration space registers
> + - const: phy # apb PHY register used on Kirin 960 PHY
> + minItems: 3
> + maxItems: 4
> +
> + reset-gpios:
> + description: The GPIO(s) to generate PCIe PERST# assert and deassert signal.
> + minItems: 1
> + maxItems: 4
I'll apply this, but only with 'maxItems: 1' if you want to separate the
discussion on that part.
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + pcie: pcie@f4000000 {
> + compatible = "hisilicon,kirin970-pcie";
> + reg = <0x0 0xf4000000 0x0 0x1000>,
> + <0x0 0xff3fe000 0x0 0x1000>,
> + <0x0 0xf4000000 0 0x2000>;
> + reg-names = "dbi", "apb", "config";
> + bus-range = <0x0 0x1>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + device_type = "pci";
> + ranges = <0x02000000 0x0 0x00000000 0x0 0xf5000000 0x0 0x2000000>;
> + num-lanes = <1>;
> + #interrupt-cells = <1>;
> + interrupts = <0 283 4>;
> + interrupt-names = "msi";
> + interrupt-map-mask = <0xf800 0 0 7>;
> + interrupt-map = <0x0 0 0 1 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
> + <0x0 0 0 2 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
> + <0x0 0 0 3 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
> + <0x0 0 0 4 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
> + reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
> + <&gpio3 1 0 >, <&gpio27 4 0 >;
> + };
> + };
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v7 10/10] phy-hi3670-pcie: Move reset-gpios to the PCIe DT schema
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
` (8 preceding siblings ...)
2021-07-21 8:39 ` [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml Mauro Carvalho Chehab
@ 2021-07-21 8:39 ` Mauro Carvalho Chehab
2021-07-21 10:15 ` [PATCH v7 11/10] PCI: kirin: Allow building it as a module Mauro Carvalho Chehab
10 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 8:39 UTC (permalink / raw)
To: Vinod Koul, Bjorn Helgaas, Rob Herring
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
Kishon Vijay Abraham I, Manivannan Sadhasivam, Rob Herring,
Wei Xu, devicetree, linux-arm-kernel, linux-kernel, linux-phy
The PHY interface as found on HiKey 970 uses 4 reset-gpios
instead of just one. That seems to be due to electrical
requirements, as, on HiKey 970, the PERST# signal is
provided via one GPIO per connected/available PCIe device:
- GPIO 56 has a pullup logic from 1V8 to 2V5
connected to a PCIe bridge chip (PEX 8606);
- GPIO 25 has a pullup logic from 1V8 to 3V3
connected to the PERST# pin at the M.2 slot;
- GPIO 220 has a pullup logic from 1V8 to 3V3
connected to the PERST# pin at the PCIe mini slot;
- GPIO 203 has a pullup logic from 1V8 to 3V3
connected to the PERST# pin at the Ethernet chipset.
Originally, this was mapped via the PHY interface, but, as such
design may also be used with different hardware, remap this
to use the pcie-bus DT schema.
This patch depends on a DT schema patch submitted at:
https://github.com/devicetree-org/dt-schema/pull/56
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
.../phy/hisilicon,phy-hi3670-pcie.yaml | 7 ---
arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 5 +-
drivers/phy/hisilicon/phy-hi3670-pcie.c | 54 ++++++++++---------
3 files changed, 31 insertions(+), 35 deletions(-)
diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
index a5ea13332cac..86767c53bc91 100644
--- a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
+++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
@@ -42,10 +42,6 @@ properties:
- const: apb_sys
- const: aclk
- reset-gpios:
- description: PCI PERST reset GPIOs
- maxItems: 4
-
clkreq-gpios:
description: Clock request GPIOs
maxItems: 3
@@ -60,7 +56,6 @@ required:
- reg
- clocks
- clock-names
- - reset-gpios
- clkreq-gpios
- hisilicon,eye-diagram-param
- phy-supply
@@ -86,8 +81,6 @@ examples:
<&crg_ctrl HI3670_ACLK_GATE_PCIE>;
clock-names = "phy_ref", "aux",
"apb_phy", "apb_sys", "aclk";
- reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
- <&gpio3 1 0 >, <&gpio27 4 0 >;
clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >, <&gpio17 0 0 >;
hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF>;
diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
index 6dfcfcfeedae..a07790c76b72 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
@@ -687,9 +687,6 @@ pcie_phy: pcie-phy@fc000000 {
"apb_phy", "apb_sys",
"aclk";
- reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
- <&gpio3 1 0 >, <&gpio27 4 0 >;
-
clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >,
<&gpio17 0 0 >;
@@ -729,6 +726,8 @@ &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
&gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
<0x0 0 0 4
&gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+ reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+ <&gpio3 1 0 >, <&gpio27 4 0 >;
};
/* UFS */
diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c
index 0bc0203e9140..82cc5fc4eac2 100644
--- a/drivers/phy/hisilicon/phy-hi3670-pcie.c
+++ b/drivers/phy/hisilicon/phy-hi3670-pcie.c
@@ -553,11 +553,13 @@ static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable)
return 0;
}
-static int hi3670_pcie_get_apb(struct hi3670_pcie_phy *phy)
+static int hi3670_pcie_get_resources_from_pcie(struct hi3670_pcie_phy *phy)
{
struct device_node *pcie_port;
struct device *dev = phy->dev;
struct device *pcie_dev;
+ char name[32];
+ int i;
pcie_port = of_get_child_by_name(dev->parent->of_node, "pcie");
if (!pcie_port) {
@@ -586,6 +588,27 @@ static int hi3670_pcie_get_apb(struct hi3670_pcie_phy *phy)
return -ENODEV;
}
+ /* perst reset gpios */
+ phy->n_gpio_resets = of_gpio_named_count(pcie_dev->of_node,
+ "reset-gpios");
+ if (phy->n_gpio_resets > MAX_GPIO_RESETS) {
+ dev_err(dev, "Too many GPIO resets!\n");
+ return -EINVAL;
+ }
+ for (i = 0; i < phy->n_gpio_resets; i++) {
+ phy->gpio_id_reset[i] = of_get_named_gpio(pcie_dev->of_node,
+ "reset-gpios", i);
+ if (phy->gpio_id_reset[i] < 0)
+ return phy->gpio_id_reset[i];
+
+ sprintf(name, "pcie_perst_%d", i);
+
+ phy->reset_names[i] = devm_kstrdup_const(dev, name,
+ GFP_KERNEL);
+ if (!phy->reset_names[i])
+ return -ENOMEM;
+ }
+
return 0;
}
@@ -644,16 +667,17 @@ static int hi3670_pcie_phy_init(struct phy *generic_phy)
int ret;
/*
- * The code under hi3670_pcie_get_apb() need to access the
- * DWC APB registers. So, get them from
- * the pcie driver's regmap (see pcie-kirin regmap).
+ * The code under hi3670_pcie_get_resources_from_pcie() need to
+ * access the reset-gpios and the APB registers, both from the
+ * pcie-kirin driver.
*
+ * The APB is obtained via the pcie driver's regmap
* Such kind of resource can only be obtained during the PCIe
* power_on sequence, as the code inside pcie-kirin needs to
* be already probed, as it needs to register the APB regmap.
*/
- ret = hi3670_pcie_get_apb(phy);
+ ret = hi3670_pcie_get_resources_from_pcie(phy);
if (ret)
return ret;
@@ -800,26 +824,6 @@ static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *phy,
if (IS_ERR(phy->base))
return PTR_ERR(phy->base);
- /* perst reset gpios */
- phy->n_gpio_resets = of_gpio_named_count(np, "reset-gpios");
- if (phy->n_gpio_resets > MAX_GPIO_RESETS) {
- dev_err(dev, "Too many GPIO resets!\n");
- return -EINVAL;
- }
- for (i = 0; i < phy->n_gpio_resets; i++) {
- phy->gpio_id_reset[i] = of_get_named_gpio(dev->of_node,
- "reset-gpios", i);
- if (phy->gpio_id_reset[i] < 0)
- return phy->gpio_id_reset[i];
-
- sprintf(name, "pcie_perst_%d", i);
-
- phy->reset_names[i] = devm_kstrdup_const(dev, name,
- GFP_KERNEL);
- if (!phy->reset_names[i])
- return -ENOMEM;
- }
-
/* clock request gpios */
phy->n_gpio_clkreq = of_gpio_named_count(np, "clkreq-gpios");
if (phy->n_gpio_clkreq > MAX_GPIO_CLKREQ) {
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v7 11/10] PCI: kirin: Allow building it as a module
2021-07-21 8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
` (9 preceding siblings ...)
2021-07-21 8:39 ` [PATCH v7 10/10] phy-hi3670-pcie: Move reset-gpios to the PCIe DT schema Mauro Carvalho Chehab
@ 2021-07-21 10:15 ` Mauro Carvalho Chehab
2021-07-21 11:55 ` Arnd Bergmann
10 siblings, 1 reply; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 10:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
Krzysztof Wilczyński, Alex Dewar, Arnd Bergmann,
Henry Styles, Jaehoon Chung, Kevin Hilman, Lorenzo Pieralisi,
Manivannan Sadhasivam, Paul Walmsley, Rob Herring, Wesley Sheng,
linux-kernel, linux-pci
There's nothing preventing this driver to be loaded as a
module. So, change its config from bool to tristate.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/pci/controller/dwc/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 423d35872ce4..e0091bfae5b5 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -227,7 +227,7 @@ config PCIE_INTEL_GW
config PCIE_KIRIN
depends on OF && (ARM64 || COMPILE_TEST)
- bool "HiSilicon Kirin series SoCs PCIe controllers"
+ tristate "HiSilicon Kirin series SoCs PCIe controllers"
depends on PCI_MSI_IRQ_DOMAIN
select PCIE_DW_HOST
help
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v7 11/10] PCI: kirin: Allow building it as a module
2021-07-21 10:15 ` [PATCH v7 11/10] PCI: kirin: Allow building it as a module Mauro Carvalho Chehab
@ 2021-07-21 11:55 ` Arnd Bergmann
2021-07-21 13:10 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 29+ messages in thread
From: Arnd Bergmann @ 2021-07-21 11:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Bjorn Helgaas, Linuxarm, Mauro Carvalho Chehab,
Krzysztof Wilczyński, Alex Dewar, Arnd Bergmann,
Henry Styles, Jaehoon Chung, Kevin Hilman, Lorenzo Pieralisi,
Manivannan Sadhasivam, Paul Walmsley, Rob Herring, Wesley Sheng,
Linux Kernel Mailing List, linux-pci
On Wed, Jul 21, 2021 at 12:15 PM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> There's nothing preventing this driver to be loaded as a
> module. So, change its config from bool to tristate.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
No objections from me, but I wonder if you would also consider making the
module removable. It's currently marked as 'builtin_platform_driver',
so you can load but not remove it. Rob has done some bug fixes that make
it possible to remove similar drivers, so it's probably not much work
here either.
Arnd
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v7 11/10] PCI: kirin: Allow building it as a module
2021-07-21 11:55 ` Arnd Bergmann
@ 2021-07-21 13:10 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21 13:10 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Bjorn Helgaas, Linuxarm, Mauro Carvalho Chehab,
Krzysztof Wilczyński, Alex Dewar, Henry Styles,
Jaehoon Chung, Kevin Hilman, Lorenzo Pieralisi,
Manivannan Sadhasivam, Paul Walmsley, Rob Herring, Wesley Sheng,
Linux Kernel Mailing List, linux-pci
Em Wed, 21 Jul 2021 13:55:07 +0200
Arnd Bergmann <arnd@arndb.de> escreveu:
> On Wed, Jul 21, 2021 at 12:15 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > There's nothing preventing this driver to be loaded as a
> > module. So, change its config from bool to tristate.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>
> No objections from me, but I wonder if you would also consider making the
> module removable. It's currently marked as 'builtin_platform_driver',
> so you can load but not remove it. Rob has done some bug fixes that make
> it possible to remove similar drivers, so it's probably not much work
> here either.
Yeah, I can probably work on a patch to unbind/remove this driver.
Never actually tried to write a patch removing the PCIe BUS, so no
idea if the refcounts for the in-board Ethernet NIC, M.2 and mini-PCIe
slots will be properly handled. If refcount is handled properly, I
guess a patch like that won't be hard, at least for Kirin 970 PHY.
The Kirin 960 PHY will require a small change at the current version,
as it currently misses the power_off logic.
I also need to double-check if devm resources will be freed at the
driver removal time, as, with some past tests with media devices,
we had some issues with that.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 29+ messages in thread