[PATCH v8 0/5] PCI: rcar-gen4: Add R-Car V4H support

[PATCH v8 0/5] PCI: rcar-gen4: Add R-Car V4H support

[Yoshihiro Shimoda]

The pcie-rcar-gen4 driver can reuse other R-Car Gen4 support like
r8a779g0 (R-Car V4H) and r8a779h0 (R-Car V4M). However, some
initializing settings differ between R-Car S4-8 (r8a779f0) and
others. The R-Car S4-8 will be minority about the setting way. So,
R-Car V4H will be majority and this is generic initialization way
as "renesas,rcar-gen4-pcie{-ep}" compatible.

About the firmware binary, please refer to the following patch
descirption:
  PCI: rcar-gen4: Add support for r8a779g0

For now, I tested both R-Car S4-8 and R-Car V4H on this driver.
I'll support one more other SoC (R-Car V4M) in the future.

Changes from v7:
https://lore.kernel.org/linux-pci/20240415081135.3814373-1-yoshihiro.shimoda.uh@renesas.com/
- Since the following patches are merged into pci.git / dt-bindings branch,
  drop them from this patch series:
   dt-bindings: PCI: rcar-gen4-pci-host: Add R-Car V4H compatible
   dt-bindings: PCI: rcar-gen4-pci-ep: Add R-Car V4H compatible
- Add Reviewed-by in the patch [25]/5.
- Add a condition to avoid automated tools report in the patch 2/5.
- Change the new function which adds an "enable" flag in the patch 3/5.
  So, change the commit description and move some functions' places.
- Revise the commit description and add firmware information in detail in
  the patch 4/5.
- Use the offset directly instead of definitions in the patch 4/5.
- Add comments for some magical offsets/values in the patch 4/5.
- Change error message when request_firmware() failed in the patch 4/5.

Changes from v6:
https://lore.kernel.org/linux-pci/20240410004832.3726922-1-yoshihiro.shimoda.uh@renesas.com/
- Add Manivannan's Reviewed-by in patch [37]/7.
- Rename a struct from "platdata" to "drvdata" in patch [4/7].
- Revise the commit descriptions in patch [456]/7.
- Rename some functions in patch 6/7.
- Fix the return value of an error path in patch 6/7.

Changes from v5:
https://lore.kernel.org/linux-pci/20240408012458.3717977-1-yoshihiro.shimoda.uh@renesas.com/
- Drop "dwc: " prefixes from the subjects in patch [0456]/7.

Changes from v4:
https://lore.kernel.org/linux-pci/20240403053304.3695096-1-yoshihiro.shimoda.uh@renesas.com/
- Fix compatible string for renesas,r8a779f0-pcie-ep which was described
  accidentally from v3...

Yoshihiro Shimoda (5):
  PCI: dwc: Add PCIE_PORT_{FORCE,LANE_SKEW} macros
  PCI: rcar-gen4: Add rcar_gen4_pcie_drvdata
  PCI: rcar-gen4: Add .ltssm_control() for other SoC support
  PCI: rcar-gen4: Add support for r8a779g0
  misc: pci_endpoint_test: Document a policy about adding pci_device_id

 drivers/misc/pci_endpoint_test.c             |   1 +
 drivers/pci/controller/dwc/pcie-designware.h |   6 +
 drivers/pci/controller/dwc/pcie-rcar-gen4.c  | 300 ++++++++++++++++---
 3 files changed, 273 insertions(+), 34 deletions(-)

-- 
2.25.1

[PATCH v8 1/5] PCI: dwc: Add PCIE_PORT_{FORCE,LANE_SKEW} macros

[Yoshihiro Shimoda]

R-Car Gen4 PCIe controller needs to use the Synopsys-specific PCIe
configuration registers. So, add the macros.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-designware.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 26dae4837462..aa4db6eaf02a 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -71,6 +71,9 @@
 #define LINK_WAIT_IATU			9
 
 /* Synopsys-specific PCIe configuration registers */
+#define PCIE_PORT_FORCE			0x708
+#define PORT_FORCE_DO_DESKEW_FOR_SRIS	BIT(23)
+
 #define PCIE_PORT_AFR			0x70C
 #define PORT_AFR_N_FTS_MASK		GENMASK(15, 8)
 #define PORT_AFR_N_FTS(n)		FIELD_PREP(PORT_AFR_N_FTS_MASK, n)
@@ -92,6 +95,9 @@
 #define PORT_LINK_MODE_4_LANES		PORT_LINK_MODE(0x7)
 #define PORT_LINK_MODE_8_LANES		PORT_LINK_MODE(0xf)
 
+#define PCIE_PORT_LANE_SKEW		0x714
+#define PORT_LANE_SKEW_INSERT_MASK	GENMASK(23, 0)
+
 #define PCIE_PORT_DEBUG0		0x728
 #define PORT_LOGIC_LTSSM_STATE_MASK	0x1f
 #define PORT_LOGIC_LTSSM_STATE_L0	0x11
-- 
2.25.1

[PATCH v8 2/5] PCI: rcar-gen4: Add rcar_gen4_pcie_drvdata

[Yoshihiro Shimoda]

In other to support future SoCs such as r8a779g0 and r8a779h0 that
require different initialization settings, let's introduce SoC
specific driver data with the initial member being the device mode.
No functional change.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-rcar-gen4.c | 32 +++++++++++++++------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index 0be760ed420b..b11e09505b0b 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -48,11 +48,15 @@
 #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
 #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
 
+struct rcar_gen4_pcie_drvdata {
+	enum dw_pcie_device_mode mode;
+};
+
 struct rcar_gen4_pcie {
 	struct dw_pcie dw;
 	void __iomem *base;
 	struct platform_device *pdev;
-	enum dw_pcie_device_mode mode;
+	const struct rcar_gen4_pcie_drvdata *drvdata;
 };
 #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
 
@@ -137,7 +141,7 @@ static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
 	 * Since dw_pcie_setup_rc() sets it once, PCIe Gen2 will be trained.
 	 * So, this needs remaining times for up to PCIe Gen4 if RC mode.
 	 */
-	if (changes && rcar->mode == DW_PCIE_RC_TYPE)
+	if (changes && rcar->drvdata->mode == DW_PCIE_RC_TYPE)
 		changes--;
 
 	for (i = 0; i < changes; i++) {
@@ -172,9 +176,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
 		reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc);
 
 	val = readl(rcar->base + PCIEMSR0);
-	if (rcar->mode == DW_PCIE_RC_TYPE) {
+	if (rcar->drvdata->mode == DW_PCIE_RC_TYPE) {
 		val |= DEVICE_TYPE_RC;
-	} else if (rcar->mode == DW_PCIE_EP_TYPE) {
+	} else if (rcar->drvdata->mode == DW_PCIE_EP_TYPE) {
 		val |= DEVICE_TYPE_EP;
 	} else {
 		ret = -EINVAL;
@@ -437,9 +441,11 @@ static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
 /* Common */
 static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar)
 {
-	rcar->mode = (uintptr_t)of_device_get_match_data(&rcar->pdev->dev);
+	rcar->drvdata = of_device_get_match_data(&rcar->pdev->dev);
+	if (!rcar->drvdata)
+		return -EINVAL;
 
-	switch (rcar->mode) {
+	switch (rcar->drvdata->mode) {
 	case DW_PCIE_RC_TYPE:
 		return rcar_gen4_add_dw_pcie_rp(rcar);
 	case DW_PCIE_EP_TYPE:
@@ -480,7 +486,7 @@ static int rcar_gen4_pcie_probe(struct platform_device *pdev)
 
 static void rcar_gen4_remove_dw_pcie(struct rcar_gen4_pcie *rcar)
 {
-	switch (rcar->mode) {
+	switch (rcar->drvdata->mode) {
 	case DW_PCIE_RC_TYPE:
 		rcar_gen4_remove_dw_pcie_rp(rcar);
 		break;
@@ -500,14 +506,22 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
 	rcar_gen4_pcie_unprepare(rcar);
 }
 
+static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static const struct of_device_id rcar_gen4_pcie_of_match[] = {
 	{
 		.compatible = "renesas,rcar-gen4-pcie",
-		.data = (void *)DW_PCIE_RC_TYPE,
+		.data = &drvdata_rcar_gen4_pcie,
 	},
 	{
 		.compatible = "renesas,rcar-gen4-pcie-ep",
-		.data = (void *)DW_PCIE_EP_TYPE,
+		.data = &drvdata_rcar_gen4_pcie_ep,
 	},
 	{},
 };
-- 
2.25.1

[PATCH v8 3/5] PCI: rcar-gen4: Add .ltssm_control() for other SoC support

[Yoshihiro Shimoda]

Sequence for controlling the LTSSM state machine is going to change
for SoCs like r8a779f0. So let's move the LTSSM code to a new callback
ltssm_control() and populate it for each SoCs.

This also warrants the addition of new compatibles for r8a779g0 and
r8a779h0. But since they are already part of the DT binding, it won't
make any difference.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pci/controller/dwc/pcie-rcar-gen4.c | 74 ++++++++++++++-------
 1 file changed, 50 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index b11e09505b0b..bcbf0a52890d 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -48,7 +48,9 @@
 #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
 #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
 
+struct rcar_gen4_pcie;
 struct rcar_gen4_pcie_drvdata {
+	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
 	enum dw_pcie_device_mode mode;
 };
 
@@ -61,27 +63,6 @@ struct rcar_gen4_pcie {
 #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
 
 /* Common */
-static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar,
-					bool enable)
-{
-	u32 val;
-
-	val = readl(rcar->base + PCIERSTCTRL1);
-	if (enable) {
-		val |= APP_LTSSM_ENABLE;
-		val &= ~APP_HOLD_PHY_RST;
-	} else {
-		/*
-		 * Since the datasheet of R-Car doesn't mention how to assert
-		 * the APP_HOLD_PHY_RST, don't assert it again. Otherwise,
-		 * hang-up issue happened in the dw_edma_core_off() when
-		 * the controller didn't detect a PCI device.
-		 */
-		val &= ~APP_LTSSM_ENABLE;
-	}
-	writel(val, rcar->base + PCIERSTCTRL1);
-}
-
 static int rcar_gen4_pcie_link_up(struct dw_pcie *dw)
 {
 	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
@@ -127,9 +108,13 @@ static int rcar_gen4_pcie_speed_change(struct dw_pcie *dw)
 static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
 {
 	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
-	int i, changes;
+	int i, changes, ret;
 
-	rcar_gen4_pcie_ltssm_enable(rcar, true);
+	if (rcar->drvdata->ltssm_control) {
+		ret = rcar->drvdata->ltssm_control(rcar, true);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * Require direct speed change with retrying here if the link_gen is
@@ -157,7 +142,8 @@ static void rcar_gen4_pcie_stop_link(struct dw_pcie *dw)
 {
 	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
 
-	rcar_gen4_pcie_ltssm_enable(rcar, false);
+	if (rcar->drvdata->ltssm_control)
+		rcar->drvdata->ltssm_control(rcar, false);
 }
 
 static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
@@ -506,6 +492,38 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
 	rcar_gen4_pcie_unprepare(rcar);
 }
 
+static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
+{
+	u32 val;
+
+	val = readl(rcar->base + PCIERSTCTRL1);
+	if (enable) {
+		val |= APP_LTSSM_ENABLE;
+		val &= ~APP_HOLD_PHY_RST;
+	} else {
+		/*
+		 * Since the datasheet of R-Car doesn't mention how to assert
+		 * the APP_HOLD_PHY_RST, don't assert it again. Otherwise,
+		 * hang-up issue happened in the dw_edma_core_off() when
+		 * the controller didn't detect a PCI device.
+		 */
+		val &= ~APP_LTSSM_ENABLE;
+	}
+	writel(val, rcar->base + PCIERSTCTRL1);
+
+	return 0;
+}
+
+static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
+	.ltssm_control = r8a779f0_pcie_ltssm_control,
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
+	.ltssm_control = r8a779f0_pcie_ltssm_control,
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
 	.mode = DW_PCIE_RC_TYPE,
 };
@@ -515,6 +533,14 @@ static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
 };
 
 static const struct of_device_id rcar_gen4_pcie_of_match[] = {
+	{
+		.compatible = "renesas,r8a779f0-pcie",
+		.data = &drvdata_r8a779f0_pcie,
+	},
+	{
+		.compatible = "renesas,r8a779f0-pcie-ep",
+		.data = &drvdata_r8a779f0_pcie_ep,
+	},
 	{
 		.compatible = "renesas,rcar-gen4-pcie",
 		.data = &drvdata_rcar_gen4_pcie,
-- 
2.25.1

[PATCH v8 4/5] PCI: rcar-gen4: Add support for r8a779g0

[Yoshihiro Shimoda]

Add support for r8a779g0 (R-Car V4H).

This driver previously supported r8a779f0 (R-Car S4-8). PCIe features
of both r8a779f0 and r8a779g0 are almost all the same. For example:
 - PCI Express Base Specification Revision 4.0
 - Root complex mode and endpoint mode are supported
However, r8a779g0 requires specific firmware downloading, to
initialize the PHY. Otherwise, the PCIe controller cannot work.

The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
the manual is a text file. But, Renesas is not able to distribute
the firmware freely. So, we require converting the text file
to a binary before the driver runs by using the following script:

 $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
   { print substr($2,5,2) substr($2,3,2) }' \
   104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
   rcar_gen4_pcie.bin
 $ sha1sum rcar_gen4_pcie.bin
   1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pci/controller/dwc/pcie-rcar-gen4.c | 194 +++++++++++++++++++-
 1 file changed, 193 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index bcbf0a52890d..f766a9739e15 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -5,8 +5,10 @@
  */
 
 #include <linux/delay.h>
+#include <linux/firmware.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pci.h>
@@ -20,9 +22,10 @@
 /* Renesas-specific */
 /* PCIe Mode Setting Register 0 */
 #define PCIEMSR0		0x0000
-#define BIFUR_MOD_SET_ON	BIT(0)
+#define APP_SRIS_MODE		BIT(6)
 #define DEVICE_TYPE_EP		0
 #define DEVICE_TYPE_RC		BIT(4)
+#define BIFUR_MOD_SET_ON	BIT(0)
 
 /* PCIe Interrupt Status 0 */
 #define PCIEINTSTS0		0x0084
@@ -37,19 +40,36 @@
 #define PCIEDMAINTSTSEN		0x0314
 #define PCIEDMAINTSTSEN_INIT	GENMASK(15, 0)
 
+/* Port Logic Registers 89 */
+#define PRTLGC89		0x0b70
+
+/* Port Logic Registers 90 */
+#define PRTLGC90		0x0b74
+
 /* PCIe Reset Control Register 1 */
 #define PCIERSTCTRL1		0x0014
 #define APP_HOLD_PHY_RST	BIT(16)
 #define APP_LTSSM_ENABLE	BIT(0)
 
+/* PCIe Power Management Control */
+#define PCIEPWRMNGCTRL		0x0070
+#define APP_CLK_REQ_N		BIT(11)
+#define APP_CLK_PM_EN		BIT(10)
+
 #define RCAR_NUM_SPEED_CHANGE_RETRIES	10
 #define RCAR_MAX_LINK_SPEED		4
 
 #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
 #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
 
+/* About the firmware, please refer to the commit log */
+#define RCAR_GEN4_PCIE_FIRMWARE_NAME		"rcar_gen4_pcie.bin"
+#define RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR	0xc000
+MODULE_FIRMWARE(RCAR_GEN4_PCIE_FIRMWARE_NAME);
+
 struct rcar_gen4_pcie;
 struct rcar_gen4_pcie_drvdata {
+	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
 	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
 	enum dw_pcie_device_mode mode;
 };
@@ -57,6 +77,7 @@ struct rcar_gen4_pcie_drvdata {
 struct rcar_gen4_pcie {
 	struct dw_pcie dw;
 	void __iomem *base;
+	void __iomem *phy_base;
 	struct platform_device *pdev;
 	const struct rcar_gen4_pcie_drvdata *drvdata;
 };
@@ -180,6 +201,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
 	if (ret)
 		goto err_unprepare;
 
+	if (rcar->drvdata->additional_common_init)
+		rcar->drvdata->additional_common_init(rcar);
+
 	return 0;
 
 err_unprepare:
@@ -221,6 +245,10 @@ static void rcar_gen4_pcie_unprepare(struct rcar_gen4_pcie *rcar)
 
 static int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar)
 {
+	rcar->phy_base = devm_platform_ioremap_resource_byname(rcar->pdev, "phy");
+	if (IS_ERR(rcar->phy_base))
+		return PTR_ERR(rcar->phy_base);
+
 	/* Renesas-specific registers */
 	rcar->base = devm_platform_ioremap_resource_byname(rcar->pdev, "app");
 
@@ -514,6 +542,166 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
 	return 0;
 }
 
+static void rcar_gen4_pcie_additional_common_init(struct rcar_gen4_pcie *rcar)
+{
+	struct dw_pcie *dw = &rcar->dw;
+	u32 val;
+
+	/*
+	 * The SoC manual said the register setting is required. Otherwise,
+	 * linkup failed.
+	 */
+	val = dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW);
+	val &= ~PORT_LANE_SKEW_INSERT_MASK;
+	if (dw->num_lanes < 4)
+		val |= BIT(6);
+	dw_pcie_writel_dbi(dw, PCIE_PORT_LANE_SKEW, val);
+
+	val = readl(rcar->base + PCIEPWRMNGCTRL);
+	val |= APP_CLK_REQ_N | APP_CLK_PM_EN;
+	writel(val, rcar->base + PCIEPWRMNGCTRL);
+}
+
+static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
+					       u32 offset, u32 mask, u32 val)
+{
+	u32 tmp;
+
+	tmp = readl(rcar->phy_base + offset);
+	tmp &= ~mask;
+	tmp |= val;
+	writel(tmp, rcar->phy_base + offset);
+}
+
+static int rcar_gen4_pcie_reg_test_bit(struct rcar_gen4_pcie *rcar,
+				       u32 offset, u32 mask)
+{
+	struct dw_pcie *dw = &rcar->dw;
+
+	if (dw_pcie_readl_dbi(dw, offset) & mask)
+		return -EAGAIN;
+
+	return 0;
+}
+
+static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
+{
+	/* The check_addr values are magical numbers in the datasheet */
+	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
+	struct dw_pcie *dw = &rcar->dw;
+	const struct firmware *fw;
+	unsigned int i, timeout;
+	u32 data;
+	int ret;
+
+	ret = request_firmware(&fw, RCAR_GEN4_PCIE_FIRMWARE_NAME, dw->dev);
+	if (ret) {
+		dev_err(dw->dev, "%s: Requesting firmware failed (%s)\n",
+			__func__, RCAR_GEN4_PCIE_FIRMWARE_NAME);
+		return ret;
+	}
+
+	for (i = 0; i < (fw->size / 2); i++) {
+		data = fw->data[(i * 2) + 1] << 8 | fw->data[i * 2];
+		timeout = 100;
+		do {
+			dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR + i);
+			dw_pcie_writel_dbi(dw, PRTLGC90, data);
+			if (rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30)) >= 0)
+				break;
+			if (!(--timeout)) {
+				ret = -ETIMEDOUT;
+				goto exit;
+			}
+			usleep_range(100, 200);
+		} while (1);
+	}
+
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
+
+	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
+		timeout = 100;
+		do {
+			dw_pcie_writel_dbi(dw, PRTLGC89, check_addr[i]);
+			ret = rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30));
+			ret |= rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC90, BIT(0));
+			if (ret >= 0)
+				break;
+			if (!(--timeout)) {
+				ret = -ETIMEDOUT;
+				goto exit;
+			}
+			usleep_range(100, 200);
+		} while (1);
+	}
+
+	ret = 0;
+exit:
+	release_firmware(fw);
+
+	return ret;
+}
+
+static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
+{
+	struct dw_pcie *dw = &rcar->dw;
+	u32 val;
+	int ret;
+
+	if (!enable) {
+		val = readl(rcar->base + PCIERSTCTRL1);
+		val &= ~APP_LTSSM_ENABLE;
+		writel(val, rcar->base + PCIERSTCTRL1);
+
+		return 0;
+	}
+
+	val = dw_pcie_readl_dbi(dw, PCIE_PORT_FORCE);
+	val |= PORT_FORCE_DO_DESKEW_FOR_SRIS;
+	dw_pcie_writel_dbi(dw, PCIE_PORT_FORCE, val);
+
+	val = readl(rcar->base + PCIEMSR0);
+	val |= APP_SRIS_MODE;
+	writel(val, rcar->base + PCIEMSR0);
+
+	/*
+	 * The R-Car Gen4 documents don't describe the PHY registers' name.
+	 * But, the initialization procedure describes these offsets. So,
+	 * this driver has magical offset numbers.
+	 */
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(28), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(20), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(12), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(4), 0);
+
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(23, 22), BIT(22));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(18, 16), GENMASK(17, 16));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(7, 6), BIT(6));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(2, 0), GENMASK(11, 0));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x1d4, GENMASK(16, 15), GENMASK(16, 15));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x514, BIT(26), BIT(26));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(16), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(19), BIT(19));
+
+	val = readl(rcar->base + PCIERSTCTRL1);
+	val &= ~APP_HOLD_PHY_RST;
+	writel(val, rcar->base + PCIERSTCTRL1);
+
+	ret = readl_poll_timeout(rcar->phy_base + 0x0f8, val, !(val & BIT(18)), 100, 10000);
+	if (ret < 0)
+		return ret;
+
+	ret = rcar_gen4_pcie_download_phy_firmware(rcar);
+	if (ret)
+		return ret;
+
+	val = readl(rcar->base + PCIERSTCTRL1);
+	val |= APP_LTSSM_ENABLE;
+	writel(val, rcar->base + PCIERSTCTRL1);
+
+	return 0;
+}
+
 static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
 	.ltssm_control = r8a779f0_pcie_ltssm_control,
 	.mode = DW_PCIE_RC_TYPE,
@@ -525,10 +713,14 @@ static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
 };
 
 static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
+	.additional_common_init = rcar_gen4_pcie_additional_common_init,
+	.ltssm_control = rcar_gen4_pcie_ltssm_control,
 	.mode = DW_PCIE_RC_TYPE,
 };
 
 static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
+	.additional_common_init = rcar_gen4_pcie_additional_common_init,
+	.ltssm_control = rcar_gen4_pcie_ltssm_control,
 	.mode = DW_PCIE_EP_TYPE,
 };
 
-- 
2.25.1

[PATCH v8 5/5] misc: pci_endpoint_test: Document a policy about adding pci_device_id

[Yoshihiro Shimoda]

To avoid becoming struct pci_device_id pci_endpoint_test_tbl longer
and longer, document a policy. For example, if PCIe endpoint controller
can configure vendor id and/or product id, you can reuse one of
existing entries to test.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/misc/pci_endpoint_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index c38a6083f0a7..727db13b6450 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -980,6 +980,7 @@ static const struct pci_endpoint_test_data j721e_data = {
 	.irq_type = IRQ_TYPE_MSI,
 };
 
+/* Do not add a new entry if the controller can use existing VID:PID combinations */
 static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x),
 	  .driver_data = (kernel_ulong_t)&default_data,
-- 
2.25.1

Re: [PATCH v8 3/5] PCI: rcar-gen4: Add .ltssm_control() for other SoC support

[Manivannan Sadhasivam]

On Mon, May 20, 2024 at 04:42:58PM +0900, Yoshihiro Shimoda wrote:
> Sequence for controlling the LTSSM state machine is going to change
> for SoCs like r8a779f0. So let's move the LTSSM code to a new callback
> ltssm_control() and populate it for each SoCs.
> 
> This also warrants the addition of new compatibles for r8a779g0 and
> r8a779h0. But since they are already part of the DT binding, it won't
> make any difference.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 74 ++++++++++++++-------
>  1 file changed, 50 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index b11e09505b0b..bcbf0a52890d 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -48,7 +48,9 @@
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
>  
> +struct rcar_gen4_pcie;
>  struct rcar_gen4_pcie_drvdata {
> +	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
>  	enum dw_pcie_device_mode mode;
>  };
>  
> @@ -61,27 +63,6 @@ struct rcar_gen4_pcie {
>  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
>  
>  /* Common */
> -static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar,
> -					bool enable)
> -{
> -	u32 val;
> -
> -	val = readl(rcar->base + PCIERSTCTRL1);
> -	if (enable) {
> -		val |= APP_LTSSM_ENABLE;
> -		val &= ~APP_HOLD_PHY_RST;
> -	} else {
> -		/*
> -		 * Since the datasheet of R-Car doesn't mention how to assert
> -		 * the APP_HOLD_PHY_RST, don't assert it again. Otherwise,
> -		 * hang-up issue happened in the dw_edma_core_off() when
> -		 * the controller didn't detect a PCI device.
> -		 */
> -		val &= ~APP_LTSSM_ENABLE;
> -	}
> -	writel(val, rcar->base + PCIERSTCTRL1);
> -}
> -
>  static int rcar_gen4_pcie_link_up(struct dw_pcie *dw)
>  {
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> @@ -127,9 +108,13 @@ static int rcar_gen4_pcie_speed_change(struct dw_pcie *dw)
>  static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
>  {
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> -	int i, changes;
> +	int i, changes, ret;
>  
> -	rcar_gen4_pcie_ltssm_enable(rcar, true);
> +	if (rcar->drvdata->ltssm_control) {
> +		ret = rcar->drvdata->ltssm_control(rcar, true);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/*
>  	 * Require direct speed change with retrying here if the link_gen is
> @@ -157,7 +142,8 @@ static void rcar_gen4_pcie_stop_link(struct dw_pcie *dw)
>  {
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
>  
> -	rcar_gen4_pcie_ltssm_enable(rcar, false);
> +	if (rcar->drvdata->ltssm_control)
> +		rcar->drvdata->ltssm_control(rcar, false);
>  }
>  
>  static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
> @@ -506,6 +492,38 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
>  	rcar_gen4_pcie_unprepare(rcar);
>  }
>  
> +static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +{
> +	u32 val;
> +
> +	val = readl(rcar->base + PCIERSTCTRL1);
> +	if (enable) {
> +		val |= APP_LTSSM_ENABLE;
> +		val &= ~APP_HOLD_PHY_RST;
> +	} else {
> +		/*
> +		 * Since the datasheet of R-Car doesn't mention how to assert
> +		 * the APP_HOLD_PHY_RST, don't assert it again. Otherwise,
> +		 * hang-up issue happened in the dw_edma_core_off() when
> +		 * the controller didn't detect a PCI device.
> +		 */
> +		val &= ~APP_LTSSM_ENABLE;
> +	}
> +	writel(val, rcar->base + PCIERSTCTRL1);
> +
> +	return 0;
> +}
> +
> +static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
> +	.ltssm_control = r8a779f0_pcie_ltssm_control,
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
> +	.ltssm_control = r8a779f0_pcie_ltssm_control,
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
>  static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
>  	.mode = DW_PCIE_RC_TYPE,
>  };
> @@ -515,6 +533,14 @@ static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
>  };
>  
>  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
> +	{
> +		.compatible = "renesas,r8a779f0-pcie",
> +		.data = &drvdata_r8a779f0_pcie,
> +	},
> +	{
> +		.compatible = "renesas,r8a779f0-pcie-ep",
> +		.data = &drvdata_r8a779f0_pcie_ep,
> +	},
>  	{
>  		.compatible = "renesas,rcar-gen4-pcie",
>  		.data = &drvdata_rcar_gen4_pcie,
> -- 
> 2.25.1
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 5/5] misc: pci_endpoint_test: Document a policy about adding pci_device_id

[Bjorn Helgaas]

On Mon, May 20, 2024 at 04:43:00PM +0900, Yoshihiro Shimoda wrote:
> To avoid becoming struct pci_device_id pci_endpoint_test_tbl longer
> and longer, document a policy. For example, if PCIe endpoint controller
> can configure vendor id and/or product id, you can reuse one of
> existing entries to test.

Possible text:

  Add a comment suggesting that if the endpoint controller Vendor and
  Device ID are programmable, an existing entry might be usable for
  testing without having to add an entry to pci_endpoint_test_tbl[].

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index c38a6083f0a7..727db13b6450 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -980,6 +980,7 @@ static const struct pci_endpoint_test_data j721e_data = {
>  	.irq_type = IRQ_TYPE_MSI,
>  };
>  
> +/* Do not add a new entry if the controller can use existing VID:PID combinations */

  /*
   * If the controller's Vendor/Device ID are programmable, you may be
   * able to use one of the existing entries for testing instead of
   * adding a new one.
   */

>  static const struct pci_device_id pci_endpoint_test_tbl[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x),
>  	  .driver_data = (kernel_ulong_t)&default_data,
> -- 
> 2.25.1
> 

Re: [PATCH v8 3/5] PCI: rcar-gen4: Add .ltssm_control() for other SoC support

[Bjorn Helgaas]

On Mon, May 20, 2024 at 04:42:58PM +0900, Yoshihiro Shimoda wrote:
> Sequence for controlling the LTSSM state machine is going to change
> for SoCs like r8a779f0. So let's move the LTSSM code to a new callback
> ltssm_control() and populate it for each SoCs.

s/So let's move/Move/

No need to repost for this, whoever applies it can tweak it.

> This also warrants the addition of new compatibles for r8a779g0 and
> r8a779h0. But since they are already part of the DT binding, it won't
> make any difference.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 74 ++++++++++++++-------
>  1 file changed, 50 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index b11e09505b0b..bcbf0a52890d 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -48,7 +48,9 @@
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
>  
> +struct rcar_gen4_pcie;
>  struct rcar_gen4_pcie_drvdata {
> +	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
>  	enum dw_pcie_device_mode mode;
>  };
>  
> @@ -61,27 +63,6 @@ struct rcar_gen4_pcie {
>  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
>  
>  /* Common */
> -static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar,
> -					bool enable)
> -{
> -	u32 val;
> -
> -	val = readl(rcar->base + PCIERSTCTRL1);
> -	if (enable) {
> -		val |= APP_LTSSM_ENABLE;
> -		val &= ~APP_HOLD_PHY_RST;
> -	} else {
> -		/*
> -		 * Since the datasheet of R-Car doesn't mention how to assert
> -		 * the APP_HOLD_PHY_RST, don't assert it again. Otherwise,
> -		 * hang-up issue happened in the dw_edma_core_off() when
> -		 * the controller didn't detect a PCI device.
> -		 */
> -		val &= ~APP_LTSSM_ENABLE;
> -	}
> -	writel(val, rcar->base + PCIERSTCTRL1);
> -}
> -
>  static int rcar_gen4_pcie_link_up(struct dw_pcie *dw)
>  {
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> @@ -127,9 +108,13 @@ static int rcar_gen4_pcie_speed_change(struct dw_pcie *dw)
>  static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
>  {
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> -	int i, changes;
> +	int i, changes, ret;
>  
> -	rcar_gen4_pcie_ltssm_enable(rcar, true);
> +	if (rcar->drvdata->ltssm_control) {
> +		ret = rcar->drvdata->ltssm_control(rcar, true);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/*
>  	 * Require direct speed change with retrying here if the link_gen is
> @@ -157,7 +142,8 @@ static void rcar_gen4_pcie_stop_link(struct dw_pcie *dw)
>  {
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
>  
> -	rcar_gen4_pcie_ltssm_enable(rcar, false);
> +	if (rcar->drvdata->ltssm_control)
> +		rcar->drvdata->ltssm_control(rcar, false);
>  }
>  
>  static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
> @@ -506,6 +492,38 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
>  	rcar_gen4_pcie_unprepare(rcar);
>  }
>  
> +static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +{
> +	u32 val;
> +
> +	val = readl(rcar->base + PCIERSTCTRL1);
> +	if (enable) {
> +		val |= APP_LTSSM_ENABLE;
> +		val &= ~APP_HOLD_PHY_RST;
> +	} else {
> +		/*
> +		 * Since the datasheet of R-Car doesn't mention how to assert
> +		 * the APP_HOLD_PHY_RST, don't assert it again. Otherwise,
> +		 * hang-up issue happened in the dw_edma_core_off() when
> +		 * the controller didn't detect a PCI device.
> +		 */
> +		val &= ~APP_LTSSM_ENABLE;
> +	}
> +	writel(val, rcar->base + PCIERSTCTRL1);
> +
> +	return 0;
> +}
> +
> +static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
> +	.ltssm_control = r8a779f0_pcie_ltssm_control,
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
> +	.ltssm_control = r8a779f0_pcie_ltssm_control,
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
>  static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
>  	.mode = DW_PCIE_RC_TYPE,
>  };
> @@ -515,6 +533,14 @@ static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
>  };
>  
>  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
> +	{
> +		.compatible = "renesas,r8a779f0-pcie",
> +		.data = &drvdata_r8a779f0_pcie,
> +	},
> +	{
> +		.compatible = "renesas,r8a779f0-pcie-ep",
> +		.data = &drvdata_r8a779f0_pcie_ep,
> +	},
>  	{
>  		.compatible = "renesas,rcar-gen4-pcie",
>  		.data = &drvdata_rcar_gen4_pcie,
> -- 
> 2.25.1
> 

Re: [PATCH v8 2/5] PCI: rcar-gen4: Add rcar_gen4_pcie_drvdata

[Bjorn Helgaas]

On Mon, May 20, 2024 at 04:42:57PM +0900, Yoshihiro Shimoda wrote:
> In other to support future SoCs such as r8a779g0 and r8a779h0 that
> require different initialization settings, let's introduce SoC
> specific driver data with the initial member being the device mode.
> No functional change.

s/In other to/To/ or s/In other/In order/ if you prefer.
s/let's//

Whoever applies this can tweak it, no need to repost for this.

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 32 +++++++++++++++------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index 0be760ed420b..b11e09505b0b 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -48,11 +48,15 @@
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
>  
> +struct rcar_gen4_pcie_drvdata {
> +	enum dw_pcie_device_mode mode;
> +};
> +
>  struct rcar_gen4_pcie {
>  	struct dw_pcie dw;
>  	void __iomem *base;
>  	struct platform_device *pdev;
> -	enum dw_pcie_device_mode mode;
> +	const struct rcar_gen4_pcie_drvdata *drvdata;
>  };
>  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
>  
> @@ -137,7 +141,7 @@ static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
>  	 * Since dw_pcie_setup_rc() sets it once, PCIe Gen2 will be trained.
>  	 * So, this needs remaining times for up to PCIe Gen4 if RC mode.
>  	 */
> -	if (changes && rcar->mode == DW_PCIE_RC_TYPE)
> +	if (changes && rcar->drvdata->mode == DW_PCIE_RC_TYPE)
>  		changes--;
>  
>  	for (i = 0; i < changes; i++) {
> @@ -172,9 +176,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
>  		reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc);
>  
>  	val = readl(rcar->base + PCIEMSR0);
> -	if (rcar->mode == DW_PCIE_RC_TYPE) {
> +	if (rcar->drvdata->mode == DW_PCIE_RC_TYPE) {
>  		val |= DEVICE_TYPE_RC;
> -	} else if (rcar->mode == DW_PCIE_EP_TYPE) {
> +	} else if (rcar->drvdata->mode == DW_PCIE_EP_TYPE) {
>  		val |= DEVICE_TYPE_EP;
>  	} else {
>  		ret = -EINVAL;
> @@ -437,9 +441,11 @@ static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
>  /* Common */
>  static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar)
>  {
> -	rcar->mode = (uintptr_t)of_device_get_match_data(&rcar->pdev->dev);
> +	rcar->drvdata = of_device_get_match_data(&rcar->pdev->dev);
> +	if (!rcar->drvdata)
> +		return -EINVAL;
>  
> -	switch (rcar->mode) {
> +	switch (rcar->drvdata->mode) {
>  	case DW_PCIE_RC_TYPE:
>  		return rcar_gen4_add_dw_pcie_rp(rcar);
>  	case DW_PCIE_EP_TYPE:
> @@ -480,7 +486,7 @@ static int rcar_gen4_pcie_probe(struct platform_device *pdev)
>  
>  static void rcar_gen4_remove_dw_pcie(struct rcar_gen4_pcie *rcar)
>  {
> -	switch (rcar->mode) {
> +	switch (rcar->drvdata->mode) {
>  	case DW_PCIE_RC_TYPE:
>  		rcar_gen4_remove_dw_pcie_rp(rcar);
>  		break;
> @@ -500,14 +506,22 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
>  	rcar_gen4_pcie_unprepare(rcar);
>  }
>  
> +static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
>  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
>  	{
>  		.compatible = "renesas,rcar-gen4-pcie",
> -		.data = (void *)DW_PCIE_RC_TYPE,
> +		.data = &drvdata_rcar_gen4_pcie,
>  	},
>  	{
>  		.compatible = "renesas,rcar-gen4-pcie-ep",
> -		.data = (void *)DW_PCIE_EP_TYPE,
> +		.data = &drvdata_rcar_gen4_pcie_ep,
>  	},
>  	{},
>  };
> -- 
> 2.25.1
> 

Re: [PATCH v8 4/5] PCI: rcar-gen4: Add support for r8a779g0

[Manivannan Sadhasivam]

On Mon, May 20, 2024 at 04:42:59PM +0900, Yoshihiro Shimoda wrote:
> Add support for r8a779g0 (R-Car V4H).
> 
> This driver previously supported r8a779f0 (R-Car S4-8). PCIe features
> of both r8a779f0 and r8a779g0 are almost all the same. For example:
>  - PCI Express Base Specification Revision 4.0
>  - Root complex mode and endpoint mode are supported
> However, r8a779g0 requires specific firmware downloading, to
> initialize the PHY. Otherwise, the PCIe controller cannot work.
> 
> The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> the manual is a text file. But, Renesas is not able to distribute
> the firmware freely. So, we require converting the text file
> to a binary before the driver runs by using the following script:
> 
>  $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
>    { print substr($2,5,2) substr($2,3,2) }' \
>    104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
>    rcar_gen4_pcie.bin
>  $ sha1sum rcar_gen4_pcie.bin
>    1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 194 +++++++++++++++++++-
>  1 file changed, 193 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index bcbf0a52890d..f766a9739e15 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -5,8 +5,10 @@
>   */
>  
>  #include <linux/delay.h>
> +#include <linux/firmware.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pci.h>
> @@ -20,9 +22,10 @@
>  /* Renesas-specific */
>  /* PCIe Mode Setting Register 0 */
>  #define PCIEMSR0		0x0000
> -#define BIFUR_MOD_SET_ON	BIT(0)
> +#define APP_SRIS_MODE		BIT(6)
>  #define DEVICE_TYPE_EP		0
>  #define DEVICE_TYPE_RC		BIT(4)
> +#define BIFUR_MOD_SET_ON	BIT(0)
>  
>  /* PCIe Interrupt Status 0 */
>  #define PCIEINTSTS0		0x0084
> @@ -37,19 +40,36 @@
>  #define PCIEDMAINTSTSEN		0x0314
>  #define PCIEDMAINTSTSEN_INIT	GENMASK(15, 0)
>  
> +/* Port Logic Registers 89 */
> +#define PRTLGC89		0x0b70
> +
> +/* Port Logic Registers 90 */
> +#define PRTLGC90		0x0b74
> +
>  /* PCIe Reset Control Register 1 */
>  #define PCIERSTCTRL1		0x0014
>  #define APP_HOLD_PHY_RST	BIT(16)
>  #define APP_LTSSM_ENABLE	BIT(0)
>  
> +/* PCIe Power Management Control */
> +#define PCIEPWRMNGCTRL		0x0070
> +#define APP_CLK_REQ_N		BIT(11)
> +#define APP_CLK_PM_EN		BIT(10)
> +
>  #define RCAR_NUM_SPEED_CHANGE_RETRIES	10
>  #define RCAR_MAX_LINK_SPEED		4
>  
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
>  
> +/* About the firmware, please refer to the commit log */

Which commit log? This is no useful info to the developers. Please add the
actual information about the firmware here (whatever you mentioned in the commit
message).

> +#define RCAR_GEN4_PCIE_FIRMWARE_NAME		"rcar_gen4_pcie.bin"
> +#define RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR	0xc000
> +MODULE_FIRMWARE(RCAR_GEN4_PCIE_FIRMWARE_NAME);
> +
>  struct rcar_gen4_pcie;
>  struct rcar_gen4_pcie_drvdata {
> +	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
>  	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
>  	enum dw_pcie_device_mode mode;
>  };
> @@ -57,6 +77,7 @@ struct rcar_gen4_pcie_drvdata {
>  struct rcar_gen4_pcie {
>  	struct dw_pcie dw;
>  	void __iomem *base;
> +	void __iomem *phy_base;
>  	struct platform_device *pdev;
>  	const struct rcar_gen4_pcie_drvdata *drvdata;
>  };
> @@ -180,6 +201,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
>  	if (ret)
>  		goto err_unprepare;
>  
> +	if (rcar->drvdata->additional_common_init)
> +		rcar->drvdata->additional_common_init(rcar);
> +
>  	return 0;
>  
>  err_unprepare:
> @@ -221,6 +245,10 @@ static void rcar_gen4_pcie_unprepare(struct rcar_gen4_pcie *rcar)
>  
>  static int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar)
>  {
> +	rcar->phy_base = devm_platform_ioremap_resource_byname(rcar->pdev, "phy");
> +	if (IS_ERR(rcar->phy_base))
> +		return PTR_ERR(rcar->phy_base);
> +
>  	/* Renesas-specific registers */
>  	rcar->base = devm_platform_ioremap_resource_byname(rcar->pdev, "app");
>  
> @@ -514,6 +542,166 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
>  	return 0;
>  }
>  
> +static void rcar_gen4_pcie_additional_common_init(struct rcar_gen4_pcie *rcar)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 val;
> +
> +	/*
> +	 * The SoC manual said the register setting is required. Otherwise,
> +	 * linkup failed.

This also can be dropped.

> +	 */
> +	val = dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW);
> +	val &= ~PORT_LANE_SKEW_INSERT_MASK;
> +	if (dw->num_lanes < 4)
> +		val |= BIT(6);
> +	dw_pcie_writel_dbi(dw, PCIE_PORT_LANE_SKEW, val);
> +
> +	val = readl(rcar->base + PCIEPWRMNGCTRL);
> +	val |= APP_CLK_REQ_N | APP_CLK_PM_EN;
> +	writel(val, rcar->base + PCIEPWRMNGCTRL);
> +}
> +
> +static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
> +					       u32 offset, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = readl(rcar->phy_base + offset);
> +	tmp &= ~mask;
> +	tmp |= val;

Use FIELD_* macros to avoid using the shift value.

> +	writel(tmp, rcar->phy_base + offset);
> +}
> +
> +static int rcar_gen4_pcie_reg_test_bit(struct rcar_gen4_pcie *rcar,
> +				       u32 offset, u32 mask)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +
> +	if (dw_pcie_readl_dbi(dw, offset) & mask)
> +		return -EAGAIN;

Here a comment like below could be added at the top of the function
definition.

	/*
	 * SoC reference manual suggests checking port logic register bits
	 * during firmware write. If read returns non-zero value, then this
	 * function returns -EAGAIN indicating that the write needs to be done
	 * again. If read returns zero, then return 0 to indicate success.
	 */

> +
> +	return 0;
> +}
> +
> +static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
> +{
> +	/* The check_addr values are magical numbers in the datasheet */
> +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
> +	struct dw_pcie *dw = &rcar->dw;
> +	const struct firmware *fw;
> +	unsigned int i, timeout;
> +	u32 data;
> +	int ret;
> +
> +	ret = request_firmware(&fw, RCAR_GEN4_PCIE_FIRMWARE_NAME, dw->dev);
> +	if (ret) {
> +		dev_err(dw->dev, "%s: Requesting firmware failed (%s)\n",
> +			__func__, RCAR_GEN4_PCIE_FIRMWARE_NAME);

		dev_err(dw->dev, "Failed to load firmware (%s): %d",
			RCAR_GEN4_PCIE_FIRMWARE_NAME, ret);

> +		return ret;
> +	}
> +
> +	for (i = 0; i < (fw->size / 2); i++) {
> +		data = fw->data[(i * 2) + 1] << 8 | fw->data[i * 2];
> +		timeout = 100;
> +		do {
> +			dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR + i);
> +			dw_pcie_writel_dbi(dw, PRTLGC90, data);
> +			if (rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30)) >= 0)

Can the return value > 0?

> +				break;
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +		} while (1);
> +	}
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
> +
> +	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
> +		timeout = 100;
> +		do {
> +			dw_pcie_writel_dbi(dw, PRTLGC89, check_addr[i]);
> +			ret = rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30));
> +			ret |= rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC90, BIT(0));
> +			if (ret >= 0)

Same as above.

> +				break;
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +		} while (1);
> +	}
> +
> +	ret = 0;

At this point, ret should be 0, right?

> +exit:
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 val;
> +	int ret;
> +
> +	if (!enable) {
> +		val = readl(rcar->base + PCIERSTCTRL1);

There is no need to use 'readl/writel' here (which has the barrier), but since
this driver is already using them, I'm not asking to change it now. But please
consider switching to _relaxed variants in a separate series.

> +		val &= ~APP_LTSSM_ENABLE;
> +		writel(val, rcar->base + PCIERSTCTRL1);
> +
> +		return 0;
> +	}
> +
> +	val = dw_pcie_readl_dbi(dw, PCIE_PORT_FORCE);
> +	val |= PORT_FORCE_DO_DESKEW_FOR_SRIS;
> +	dw_pcie_writel_dbi(dw, PCIE_PORT_FORCE, val);
> +
> +	val = readl(rcar->base + PCIEMSR0);
> +	val |= APP_SRIS_MODE;
> +	writel(val, rcar->base + PCIEMSR0);
> +
> +	/*
> +	 * The R-Car Gen4 documents don't describe the PHY registers' name.

Gen4 datasheet? or reference manual?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

RE: [PATCH v8 5/5] misc: pci_endpoint_test: Document a policy about adding pci_device_id

[Yoshihiro Shimoda]

Hi Bjorn,

> From: Bjorn Helgaas, Sent: Thursday, June 6, 2024 2:28 AM
> 
> On Mon, May 20, 2024 at 04:43:00PM +0900, Yoshihiro Shimoda wrote:
> > To avoid becoming struct pci_device_id pci_endpoint_test_tbl longer
> > and longer, document a policy. For example, if PCIe endpoint controller
> > can configure vendor id and/or product id, you can reuse one of
> > existing entries to test.
> 
> Possible text:
> 
>   Add a comment suggesting that if the endpoint controller Vendor and
>   Device ID are programmable, an existing entry might be usable for
>   testing without having to add an entry to pci_endpoint_test_tbl[].

Thank you for the suggestion. I'll fix the description.

> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/misc/pci_endpoint_test.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index c38a6083f0a7..727db13b6450 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -980,6 +980,7 @@ static const struct pci_endpoint_test_data j721e_data = {
> >  	.irq_type = IRQ_TYPE_MSI,
> >  };
> >
> > +/* Do not add a new entry if the controller can use existing VID:PID combinations */
> 
>   /*
>    * If the controller's Vendor/Device ID are programmable, you may be
>    * able to use one of the existing entries for testing instead of
>    * adding a new one.
>    */

I'll write this on v9.

Best regards,
Yoshihiro Shimoda

> >  static const struct pci_device_id pci_endpoint_test_tbl[] = {
> >  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x),
> >  	  .driver_data = (kernel_ulong_t)&default_data,
> > --
> > 2.25.1
> >

RE: [PATCH v8 3/5] PCI: rcar-gen4: Add .ltssm_control() for other SoC support

[Yoshihiro Shimoda]

Hi Bjorn,

> From: Bjorn Helgaas, Sent: Thursday, June 6, 2024 2:29 AM
> 
> On Mon, May 20, 2024 at 04:42:58PM +0900, Yoshihiro Shimoda wrote:
> > Sequence for controlling the LTSSM state machine is going to change
> > for SoCs like r8a779f0. So let's move the LTSSM code to a new callback
> > ltssm_control() and populate it for each SoCs.
> 
> s/So let's move/Move/
> 
> No need to repost for this, whoever applies it can tweak it.

I need to post v9 patches for Manivannan's feedback, so I'll fix the description on v9.

Best regards,
Yoshihiro Shimoda

> > This also warrants the addition of new compatibles for r8a779g0 and
> > r8a779h0. But since they are already part of the DT binding, it won't
> > make any difference.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 74 ++++++++++++++-------
> >  1 file changed, 50 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index b11e09505b0b..bcbf0a52890d 100644
> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > @@ -48,7 +48,9 @@
> >  #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
> >  #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
> >
> > +struct rcar_gen4_pcie;
> >  struct rcar_gen4_pcie_drvdata {
> > +	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
> >  	enum dw_pcie_device_mode mode;
> >  };
> >
> > @@ -61,27 +63,6 @@ struct rcar_gen4_pcie {
> >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> >
> >  /* Common */
> > -static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar,
> > -					bool enable)
> > -{
> > -	u32 val;
> > -
> > -	val = readl(rcar->base + PCIERSTCTRL1);
> > -	if (enable) {
> > -		val |= APP_LTSSM_ENABLE;
> > -		val &= ~APP_HOLD_PHY_RST;
> > -	} else {
> > -		/*
> > -		 * Since the datasheet of R-Car doesn't mention how to assert
> > -		 * the APP_HOLD_PHY_RST, don't assert it again. Otherwise,
> > -		 * hang-up issue happened in the dw_edma_core_off() when
> > -		 * the controller didn't detect a PCI device.
> > -		 */
> > -		val &= ~APP_LTSSM_ENABLE;
> > -	}
> > -	writel(val, rcar->base + PCIERSTCTRL1);
> > -}
> > -
> >  static int rcar_gen4_pcie_link_up(struct dw_pcie *dw)
> >  {
> >  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > @@ -127,9 +108,13 @@ static int rcar_gen4_pcie_speed_change(struct dw_pcie *dw)
> >  static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
> >  {
> >  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > -	int i, changes;
> > +	int i, changes, ret;
> >
> > -	rcar_gen4_pcie_ltssm_enable(rcar, true);
> > +	if (rcar->drvdata->ltssm_control) {
> > +		ret = rcar->drvdata->ltssm_control(rcar, true);
> > +		if (ret)
> > +			return ret;
> > +	}
> >
> >  	/*
> >  	 * Require direct speed change with retrying here if the link_gen is
> > @@ -157,7 +142,8 @@ static void rcar_gen4_pcie_stop_link(struct dw_pcie *dw)
> >  {
> >  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> >
> > -	rcar_gen4_pcie_ltssm_enable(rcar, false);
> > +	if (rcar->drvdata->ltssm_control)
> > +		rcar->drvdata->ltssm_control(rcar, false);
> >  }
> >
> >  static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
> > @@ -506,6 +492,38 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
> >  	rcar_gen4_pcie_unprepare(rcar);
> >  }
> >
> > +static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> > +{
> > +	u32 val;
> > +
> > +	val = readl(rcar->base + PCIERSTCTRL1);
> > +	if (enable) {
> > +		val |= APP_LTSSM_ENABLE;
> > +		val &= ~APP_HOLD_PHY_RST;
> > +	} else {
> > +		/*
> > +		 * Since the datasheet of R-Car doesn't mention how to assert
> > +		 * the APP_HOLD_PHY_RST, don't assert it again. Otherwise,
> > +		 * hang-up issue happened in the dw_edma_core_off() when
> > +		 * the controller didn't detect a PCI device.
> > +		 */
> > +		val &= ~APP_LTSSM_ENABLE;
> > +	}
> > +	writel(val, rcar->base + PCIERSTCTRL1);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
> > +	.ltssm_control = r8a779f0_pcie_ltssm_control,
> > +	.mode = DW_PCIE_RC_TYPE,
> > +};
> > +
> > +static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
> > +	.ltssm_control = r8a779f0_pcie_ltssm_control,
> > +	.mode = DW_PCIE_EP_TYPE,
> > +};
> > +
> >  static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
> >  	.mode = DW_PCIE_RC_TYPE,
> >  };
> > @@ -515,6 +533,14 @@ static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
> >  };
> >
> >  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
> > +	{
> > +		.compatible = "renesas,r8a779f0-pcie",
> > +		.data = &drvdata_r8a779f0_pcie,
> > +	},
> > +	{
> > +		.compatible = "renesas,r8a779f0-pcie-ep",
> > +		.data = &drvdata_r8a779f0_pcie_ep,
> > +	},
> >  	{
> >  		.compatible = "renesas,rcar-gen4-pcie",
> >  		.data = &drvdata_rcar_gen4_pcie,
> > --
> > 2.25.1
> >

RE: [PATCH v8 4/5] PCI: rcar-gen4: Add support for r8a779g0

[Yoshihiro Shimoda]

Hi Manivannan,

> From: Manivannan Sadhasivam, Sent: Saturday, June 8, 2024 5:25 PM
> 
> On Mon, May 20, 2024 at 04:42:59PM +0900, Yoshihiro Shimoda wrote:
> > Add support for r8a779g0 (R-Car V4H).
> >
> > This driver previously supported r8a779f0 (R-Car S4-8). PCIe features
> > of both r8a779f0 and r8a779g0 are almost all the same. For example:
> >  - PCI Express Base Specification Revision 4.0
> >  - Root complex mode and endpoint mode are supported
> > However, r8a779g0 requires specific firmware downloading, to
> > initialize the PHY. Otherwise, the PCIe controller cannot work.
> >
> > The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> > the manual is a text file. But, Renesas is not able to distribute
> > the firmware freely. So, we require converting the text file
> > to a binary before the driver runs by using the following script:
> >
> >  $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
> >    { print substr($2,5,2) substr($2,3,2) }' \
> >    104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
> >    rcar_gen4_pcie.bin
> >  $ sha1sum rcar_gen4_pcie.bin
> >    1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 194 +++++++++++++++++++-
> >  1 file changed, 193 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index bcbf0a52890d..f766a9739e15 100644
> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > @@ -5,8 +5,10 @@
> >   */
> >
> >  #include <linux/delay.h>
> > +#include <linux/firmware.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/pci.h>
> > @@ -20,9 +22,10 @@
> >  /* Renesas-specific */
> >  /* PCIe Mode Setting Register 0 */
> >  #define PCIEMSR0		0x0000
> > -#define BIFUR_MOD_SET_ON	BIT(0)
> > +#define APP_SRIS_MODE		BIT(6)
> >  #define DEVICE_TYPE_EP		0
> >  #define DEVICE_TYPE_RC		BIT(4)
> > +#define BIFUR_MOD_SET_ON	BIT(0)
> >
> >  /* PCIe Interrupt Status 0 */
> >  #define PCIEINTSTS0		0x0084
> > @@ -37,19 +40,36 @@
> >  #define PCIEDMAINTSTSEN		0x0314
> >  #define PCIEDMAINTSTSEN_INIT	GENMASK(15, 0)
> >
> > +/* Port Logic Registers 89 */
> > +#define PRTLGC89		0x0b70
> > +
> > +/* Port Logic Registers 90 */
> > +#define PRTLGC90		0x0b74
> > +
> >  /* PCIe Reset Control Register 1 */
> >  #define PCIERSTCTRL1		0x0014
> >  #define APP_HOLD_PHY_RST	BIT(16)
> >  #define APP_LTSSM_ENABLE	BIT(0)
> >
> > +/* PCIe Power Management Control */
> > +#define PCIEPWRMNGCTRL		0x0070
> > +#define APP_CLK_REQ_N		BIT(11)
> > +#define APP_CLK_PM_EN		BIT(10)
> > +
> >  #define RCAR_NUM_SPEED_CHANGE_RETRIES	10
> >  #define RCAR_MAX_LINK_SPEED		4
> >
> >  #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
> >  #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
> >
> > +/* About the firmware, please refer to the commit log */
> 
> Which commit log? This is no useful info to the developers. Please add the
> actual information about the firmware here (whatever you mentioned in the commit
> message).

I got it.

> > +#define RCAR_GEN4_PCIE_FIRMWARE_NAME		"rcar_gen4_pcie.bin"
> > +#define RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR	0xc000
> > +MODULE_FIRMWARE(RCAR_GEN4_PCIE_FIRMWARE_NAME);
> > +
> >  struct rcar_gen4_pcie;
> >  struct rcar_gen4_pcie_drvdata {
> > +	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
> >  	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
> >  	enum dw_pcie_device_mode mode;
> >  };
> > @@ -57,6 +77,7 @@ struct rcar_gen4_pcie_drvdata {
> >  struct rcar_gen4_pcie {
> >  	struct dw_pcie dw;
> >  	void __iomem *base;
> > +	void __iomem *phy_base;
> >  	struct platform_device *pdev;
> >  	const struct rcar_gen4_pcie_drvdata *drvdata;
> >  };
> > @@ -180,6 +201,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
> >  	if (ret)
> >  		goto err_unprepare;
> >
> > +	if (rcar->drvdata->additional_common_init)
> > +		rcar->drvdata->additional_common_init(rcar);
> > +
> >  	return 0;
> >
> >  err_unprepare:
> > @@ -221,6 +245,10 @@ static void rcar_gen4_pcie_unprepare(struct rcar_gen4_pcie *rcar)
> >
> >  static int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar)
> >  {
> > +	rcar->phy_base = devm_platform_ioremap_resource_byname(rcar->pdev, "phy");
> > +	if (IS_ERR(rcar->phy_base))
> > +		return PTR_ERR(rcar->phy_base);
> > +
> >  	/* Renesas-specific registers */
> >  	rcar->base = devm_platform_ioremap_resource_byname(rcar->pdev, "app");
> >
> > @@ -514,6 +542,166 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> >  	return 0;
> >  }
> >
> > +static void rcar_gen4_pcie_additional_common_init(struct rcar_gen4_pcie *rcar)
> > +{
> > +	struct dw_pcie *dw = &rcar->dw;
> > +	u32 val;
> > +
> > +	/*
> > +	 * The SoC manual said the register setting is required. Otherwise,
> > +	 * linkup failed.
> 
> This also can be dropped.

I got it.

> > +	 */
> > +	val = dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW);
> > +	val &= ~PORT_LANE_SKEW_INSERT_MASK;
> > +	if (dw->num_lanes < 4)
> > +		val |= BIT(6);
> > +	dw_pcie_writel_dbi(dw, PCIE_PORT_LANE_SKEW, val);
> > +
> > +	val = readl(rcar->base + PCIEPWRMNGCTRL);
> > +	val |= APP_CLK_REQ_N | APP_CLK_PM_EN;
> > +	writel(val, rcar->base + PCIEPWRMNGCTRL);
> > +}
> > +
> > +static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
> > +					       u32 offset, u32 mask, u32 val)
> > +{
> > +	u32 tmp;
> > +
> > +	tmp = readl(rcar->phy_base + offset);
> > +	tmp &= ~mask;
> > +	tmp |= val;
> 
> Use FIELD_* macros to avoid using the shift value.

According to the bitfield.h,
---
* FIELD_{GET,PREP} macros take as first parameter shifted mask
 * from which they extract the base mask and shift amount.
 * Mask must be a compilation time constant.
---
So, since the mask is a variable here, we cannot use FIELD_* macros for this function.

> > +	writel(tmp, rcar->phy_base + offset);
> > +}
> > +
> > +static int rcar_gen4_pcie_reg_test_bit(struct rcar_gen4_pcie *rcar,
> > +				       u32 offset, u32 mask)
> > +{
> > +	struct dw_pcie *dw = &rcar->dw;
> > +
> > +	if (dw_pcie_readl_dbi(dw, offset) & mask)
> > +		return -EAGAIN;
> 
> Here a comment like below could be added at the top of the function
> definition.
> 
> 	/*
> 	 * SoC reference manual suggests checking port logic register bits
> 	 * during firmware write. If read returns non-zero value, then this
> 	 * function returns -EAGAIN indicating that the write needs to be done
> 	 * again. If read returns zero, then return 0 to indicate success.
> 	 */

I got it.

> > +
> > +	return 0;
> > +}
> > +
> > +static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
> > +{
> > +	/* The check_addr values are magical numbers in the datasheet */
> > +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
> > +	struct dw_pcie *dw = &rcar->dw;
> > +	const struct firmware *fw;
> > +	unsigned int i, timeout;
> > +	u32 data;
> > +	int ret;
> > +
> > +	ret = request_firmware(&fw, RCAR_GEN4_PCIE_FIRMWARE_NAME, dw->dev);
> > +	if (ret) {
> > +		dev_err(dw->dev, "%s: Requesting firmware failed (%s)\n",
> > +			__func__, RCAR_GEN4_PCIE_FIRMWARE_NAME);
> 
> 		dev_err(dw->dev, "Failed to load firmware (%s): %d",
> 			RCAR_GEN4_PCIE_FIRMWARE_NAME, ret);
> 
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < (fw->size / 2); i++) {
> > +		data = fw->data[(i * 2) + 1] << 8 | fw->data[i * 2];
> > +		timeout = 100;
> > +		do {
> > +			dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR + i);
> > +			dw_pcie_writel_dbi(dw, PRTLGC90, data);
> > +			if (rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30)) >= 0)
> 
> Can the return value > 0?

No. So, it seems using "!" like below is better, I think.

+			if (!rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30)))

> > +				break;
> > +			if (!(--timeout)) {
> > +				ret = -ETIMEDOUT;
> > +				goto exit;
> > +			}
> > +			usleep_range(100, 200);
> > +		} while (1);
> > +	}
> > +
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
> > +
> > +	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
> > +		timeout = 100;
> > +		do {
> > +			dw_pcie_writel_dbi(dw, PRTLGC89, check_addr[i]);
> > +			ret = rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30));
> > +			ret |= rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC90, BIT(0));
> > +			if (ret >= 0)
> 
> Same as above.

I got it.

> > +				break;
> > +			if (!(--timeout)) {
> > +				ret = -ETIMEDOUT;
> > +				goto exit;
> > +			}
> > +			usleep_range(100, 200);
> > +		} while (1);
> > +	}
> > +
> > +	ret = 0;
> 
> At this point, ret should be 0, right?

It's right. So, I'll drop this.

> > +exit:
> > +	release_firmware(fw);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> > +{
> > +	struct dw_pcie *dw = &rcar->dw;
> > +	u32 val;
> > +	int ret;
> > +
> > +	if (!enable) {
> > +		val = readl(rcar->base + PCIERSTCTRL1);
> 
> There is no need to use 'readl/writel' here (which has the barrier), but since
> this driver is already using them, I'm not asking to change it now. But please
> consider switching to _relaxed variants in a separate series.

I got it.

> > +		val &= ~APP_LTSSM_ENABLE;
> > +		writel(val, rcar->base + PCIERSTCTRL1);
> > +
> > +		return 0;
> > +	}
> > +
> > +	val = dw_pcie_readl_dbi(dw, PCIE_PORT_FORCE);
> > +	val |= PORT_FORCE_DO_DESKEW_FOR_SRIS;
> > +	dw_pcie_writel_dbi(dw, PCIE_PORT_FORCE, val);
> > +
> > +	val = readl(rcar->base + PCIEMSR0);
> > +	val |= APP_SRIS_MODE;
> > +	writel(val, rcar->base + PCIEMSR0);
> > +
> > +	/*
> > +	 * The R-Car Gen4 documents don't describe the PHY registers' name.
> 
> Gen4 datasheet? or reference manual?

I'll write "Gen4 datasheet" instead.

Best regards,
Yoshihiro Shimoda

> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 4/5] PCI: rcar-gen4: Add support for r8a779g0

[Geert Uytterhoeven]

On Tue, Jun 11, 2024 at 11:21 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Manivannan Sadhasivam, Sent: Saturday, June 8, 2024 5:25 PM
> > On Mon, May 20, 2024 at 04:42:59PM +0900, Yoshihiro Shimoda wrote:
> > > +static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
> > > +                                          u32 offset, u32 mask, u32 val)
> > > +{
> > > +   u32 tmp;
> > > +
> > > +   tmp = readl(rcar->phy_base + offset);
> > > +   tmp &= ~mask;
> > > +   tmp |= val;
> >
> > Use FIELD_* macros to avoid using the shift value.
>
> According to the bitfield.h,
> ---
> * FIELD_{GET,PREP} macros take as first parameter shifted mask
>  * from which they extract the base mask and shift amount.
>  * Mask must be a compilation time constant.
> ---
> So, since the mask is a variable here, we cannot use FIELD_* macros for this function.

Indeed.

I tried introducing non-constant field_{prep,get}() helpers[1] in series
[2], but there were some pushbacks.

Feel free to up-vote ;-)

[1] "[PATCH 01/17] bitfield: Add non-constant field_{prep,get}() helpers"
https://lore.kernel.org/all/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@glider.be/

[2] "[PATCH 00/17] Non-const bitfield helper conversions"
https://lore.kernel.org/all/cover.1637592133.git.geert+renesas@glider.be/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v8 4/5] PCI: rcar-gen4: Add support for r8a779g0

[Manivannan Sadhasivam]

On Tue, Jun 11, 2024 at 12:10:22PM +0200, Geert Uytterhoeven wrote:
> On Tue, Jun 11, 2024 at 11:21 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Manivannan Sadhasivam, Sent: Saturday, June 8, 2024 5:25 PM
> > > On Mon, May 20, 2024 at 04:42:59PM +0900, Yoshihiro Shimoda wrote:
> > > > +static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
> > > > +                                          u32 offset, u32 mask, u32 val)
> > > > +{
> > > > +   u32 tmp;
> > > > +
> > > > +   tmp = readl(rcar->phy_base + offset);
> > > > +   tmp &= ~mask;
> > > > +   tmp |= val;
> > >
> > > Use FIELD_* macros to avoid using the shift value.
> >
> > According to the bitfield.h,
> > ---
> > * FIELD_{GET,PREP} macros take as first parameter shifted mask
> >  * from which they extract the base mask and shift amount.
> >  * Mask must be a compilation time constant.
> > ---
> > So, since the mask is a variable here, we cannot use FIELD_* macros for this function.
> 
> Indeed.
> 

I just can't keep the constant factor in mind for some reason.

> I tried introducing non-constant field_{prep,get}() helpers[1] in series
> [2], but there were some pushbacks.
> 
> Feel free to up-vote ;-)
> 

For sure! This will be very useful, thanks.

- Mani

> [1] "[PATCH 01/17] bitfield: Add non-constant field_{prep,get}() helpers"
> https://lore.kernel.org/all/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@glider.be/
> 
> [2] "[PATCH 00/17] Non-const bitfield helper conversions"
> https://lore.kernel.org/all/cover.1637592133.git.geert+renesas@glider.be/
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

-- 
மணிவண்ணன் சதாசிவம்