[PATCH v7 0/7] PCI: rcar-gen4: Add R-Car V4H support

[PATCH v7 0/7] 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 downloading, we can get information from
the following wiki page:
https://elinux.org/index.php?title=R-Car/Boards/WhiteHawk&oldid=581944#PCIe_firmware

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 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 (7):
  dt-bindings: PCI: rcar-gen4-pci-host: Add R-Car V4H compatible
  dt-bindings: PCI: rcar-gen4-pci-ep: Add R-Car V4H compatible
  PCI: dwc: Add PCIE_PORT_{FORCE,LANE_SKEW} macros
  PCI: rcar-gen4: Add rcar_gen4_pcie_drvdata
  PCI: rcar-gen4: Add .ltssm_enable() for other SoC support
  PCI: rcar-gen4: Add support for r8a779g0
  misc: pci_endpoint_test: Document a policy about adding pci_device_id

 .../bindings/pci/rcar-gen4-pci-ep.yaml        |   4 +-
 .../bindings/pci/rcar-gen4-pci-host.yaml      |   4 +-
 drivers/misc/pci_endpoint_test.c              |   1 +
 drivers/pci/controller/dwc/pcie-designware.h  |   6 +
 drivers/pci/controller/dwc/pcie-rcar-gen4.c   | 272 +++++++++++++++++-
 5 files changed, 270 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH v7 1/7] dt-bindings: PCI: rcar-gen4-pci-host: Add R-Car V4H compatible

[Yoshihiro Shimoda]

Document bindings for R-Car V4H (R8A779G0) PCIe host module.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/pci/rcar-gen4-pci-host.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/rcar-gen4-pci-host.yaml b/Documentation/devicetree/bindings/pci/rcar-gen4-pci-host.yaml
index ffb34339b637..955c664f1fbb 100644
--- a/Documentation/devicetree/bindings/pci/rcar-gen4-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/rcar-gen4-pci-host.yaml
@@ -16,7 +16,9 @@ allOf:
 properties:
   compatible:
     items:
-      - const: renesas,r8a779f0-pcie   # R-Car S4-8
+      - enum:
+          - renesas,r8a779f0-pcie      # R-Car S4-8
+          - renesas,r8a779g0-pcie      # R-Car V4H
       - const: renesas,rcar-gen4-pcie  # R-Car Gen4
 
   reg:
-- 
2.25.1

[PATCH v7 2/7] dt-bindings: PCI: rcar-gen4-pci-ep: Add R-Car V4H compatible

[Yoshihiro Shimoda]

Document bindings for R-Car V4H (R8A779G0) PCIe endpoint module.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/pci/rcar-gen4-pci-ep.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/rcar-gen4-pci-ep.yaml b/Documentation/devicetree/bindings/pci/rcar-gen4-pci-ep.yaml
index fe38f62da066..91b81ac75592 100644
--- a/Documentation/devicetree/bindings/pci/rcar-gen4-pci-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/rcar-gen4-pci-ep.yaml
@@ -16,7 +16,9 @@ allOf:
 properties:
   compatible:
     items:
-      - const: renesas,r8a779f0-pcie-ep   # R-Car S4-8
+      - enum:
+          - renesas,r8a779f0-pcie-ep      # R-Car S4-8
+          - renesas,r8a779g0-pcie-ep      # R-Car V4H
       - const: renesas,rcar-gen4-pcie-ep  # R-Car Gen4
 
   reg:
-- 
2.25.1

[PATCH v7 3/7] 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 v7 4/7] 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>
---
 drivers/pci/controller/dwc/pcie-rcar-gen4.c | 30 ++++++++++++++-------
 1 file changed, 21 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..3da0a844e1b6 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,9 @@ 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);
 
-	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 +484,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 +504,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 v7 5/7] PCI: rcar-gen4: Add .ltssm_enable() for other SoC support

[Yoshihiro Shimoda]

This driver can reuse other R-Car Gen4 SoCs support like r8a779g0 and
r8a779h0. However, r8a779g0 and r8a779h0 require other initializing
settings that differ than r8a779f0. So, add a new function pointer
.ltssm_enable() for it.

After applied this patch, probing SoCs by rcar_gen4_pcie_of_match[]
will be changed like below:

- r8a779f0 as "renesas,r8a779f0-pcie" and "renesas,r8a779f0-pcie-ep"

Existing dts files have the compatible above. So, no behavior changes.

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

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index 3da0a844e1b6..980a916933d6 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_enable)(struct rcar_gen4_pcie *rcar);
 	enum dw_pcie_device_mode mode;
 };
 
@@ -61,8 +63,8 @@ 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)
+static void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
+					 bool enable)
 {
 	u32 val;
 
@@ -127,9 +129,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_enable) {
+		ret = rcar->drvdata->ltssm_enable(rcar);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * Require direct speed change with retrying here if the link_gen is
@@ -157,7 +163,7 @@ 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);
+	rcar_gen4_pcie_ltssm_control(rcar, false);
 }
 
 static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
@@ -504,6 +510,23 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
 	rcar_gen4_pcie_unprepare(rcar);
 }
 
+static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
+{
+	rcar_gen4_pcie_ltssm_control(rcar, true);
+
+	return 0;
+}
+
+static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
+	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
+	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
 	.mode = DW_PCIE_RC_TYPE,
 };
@@ -513,6 +536,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 v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Yoshihiro Shimoda]

This driver previously supported r8a779f0 (R-Car S4-8). Add support
for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
So, convert it to a binary file by using a script.

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

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index 980a916933d6..4e934e9156f2 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,47 @@
 #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)
+
+/*
+ * The R-Car Gen4 documents don't describe the PHY registers' name.
+ * But, the initialization procedure describes these offsets. So,
+ * this driver makes up own #defines for the offsets.
+ */
+#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
+#define RCAR_GEN4_PCIE_PHY_148	0x148
+#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
+#define RCAR_GEN4_PCIE_PHY_514	0x514
+#define RCAR_GEN4_PCIE_PHY_700	0x700
+
 #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
 
+#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_enable)(struct rcar_gen4_pcie *rcar);
 	enum dw_pcie_device_mode mode;
 };
@@ -57,12 +88,144 @@ 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;
 };
 #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
 
 /* Common */
+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_check(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)
+{
+	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\n", __func__);
+		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_check(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, RCAR_GEN4_PCIE_PHY_0f8, 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_check(rcar, PRTLGC89, BIT(30));
+			ret |= rcar_gen4_pcie_reg_check(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_enable_phy(struct rcar_gen4_pcie *rcar)
+{
+	struct dw_pcie *dw = &rcar->dw;
+	u32 val;
+	int ret;
+
+	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);
+
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(28), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(20), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(12), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(4), 0);
+
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
+					   GENMASK(23, 22), BIT(22));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
+					   GENMASK(18, 16), GENMASK(17, 16));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
+					   GENMASK(7, 6), BIT(6));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
+					   GENMASK(2, 0), GENMASK(11, 0));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_1d4,
+					   GENMASK(16, 15), GENMASK(16, 15));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_514, BIT(26), BIT(26));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, BIT(16), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, 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 + RCAR_GEN4_PCIE_PHY_0f8, 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 void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
 					 bool enable)
 {
@@ -200,6 +363,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:
@@ -241,6 +407,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");
 
@@ -517,6 +687,31 @@ static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
 	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 int rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
+{
+	return rcar_gen4_pcie_enable_phy(rcar);
+}
+
 static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
 	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
 	.mode = DW_PCIE_RC_TYPE,
@@ -528,10 +723,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_enable = rcar_gen4_pcie_ltssm_enable,
 	.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_enable = rcar_gen4_pcie_ltssm_enable,
 	.mode = DW_PCIE_EP_TYPE,
 };
 
-- 
2.25.1

[PATCH v7 7/7] 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>
---
Cc: 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 v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Niklas Cassel]

On Mon, Apr 15, 2024 at 05:11:34PM +0900, Yoshihiro Shimoda wrote:
> This driver previously supported r8a779f0 (R-Car S4-8). Add support
> for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
> So, convert it to a binary file by using a script.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 201 +++++++++++++++++++-
>  1 file changed, 200 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index 980a916933d6..4e934e9156f2 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,47 @@
>  #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)
> +
> +/*
> + * The R-Car Gen4 documents don't describe the PHY registers' name.
> + * But, the initialization procedure describes these offsets. So,
> + * this driver makes up own #defines for the offsets.
> + */
> +#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
> +#define RCAR_GEN4_PCIE_PHY_148	0x148
> +#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
> +#define RCAR_GEN4_PCIE_PHY_514	0x514
> +#define RCAR_GEN4_PCIE_PHY_700	0x700
> +
>  #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
>  
> +#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_enable)(struct rcar_gen4_pcie *rcar);
>  	enum dw_pcie_device_mode mode;
>  };
> @@ -57,12 +88,144 @@ 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;
>  };
>  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
>  
>  /* Common */
> +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_check(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)
> +{
> +	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\n", __func__);
> +		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_check(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, RCAR_GEN4_PCIE_PHY_0f8, 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_check(rcar, PRTLGC89, BIT(30));
> +			ret |= rcar_gen4_pcie_reg_check(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_enable_phy(struct rcar_gen4_pcie *rcar)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 val;
> +	int ret;
> +
> +	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);
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(28), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(20), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(12), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(4), 0);
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> +					   GENMASK(23, 22), BIT(22));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> +					   GENMASK(18, 16), GENMASK(17, 16));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> +					   GENMASK(7, 6), BIT(6));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> +					   GENMASK(2, 0), GENMASK(11, 0));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_1d4,
> +					   GENMASK(16, 15), GENMASK(16, 15));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_514, BIT(26), BIT(26));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, BIT(16), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, 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 + RCAR_GEN4_PCIE_PHY_0f8, 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;
> +}
> +

Is there a reason why you didn't chose to implement this as a PHY driver
in drivers/phy ?


Kind regards,
Niklas


>  static void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
>  					 bool enable)
>  {
> @@ -200,6 +363,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:
> @@ -241,6 +407,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");
>  
> @@ -517,6 +687,31 @@ static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
>  	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 int rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> +{
> +	return rcar_gen4_pcie_enable_phy(rcar);
> +}
> +
>  static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
>  	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
>  	.mode = DW_PCIE_RC_TYPE,
> @@ -528,10 +723,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_enable = rcar_gen4_pcie_ltssm_enable,
>  	.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_enable = rcar_gen4_pcie_ltssm_enable,
>  	.mode = DW_PCIE_EP_TYPE,
>  };
>  
> -- 
> 2.25.1
> 

RE: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Yoshihiro Shimoda]

Hi Niklas,

> From: Niklas Cassel, Sent: Monday, April 15, 2024 5:27 PM
> 
> On Mon, Apr 15, 2024 at 05:11:34PM +0900, Yoshihiro Shimoda wrote:
> > This driver previously supported r8a779f0 (R-Car S4-8). Add support
> > for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
> > So, convert it to a binary file by using a script.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 201 +++++++++++++++++++-
> >  1 file changed, 200 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index 980a916933d6..4e934e9156f2 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,47 @@
> >  #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)
> > +
> > +/*
> > + * The R-Car Gen4 documents don't describe the PHY registers' name.
> > + * But, the initialization procedure describes these offsets. So,
> > + * this driver makes up own #defines for the offsets.
> > + */
> > +#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
> > +#define RCAR_GEN4_PCIE_PHY_148	0x148
> > +#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
> > +#define RCAR_GEN4_PCIE_PHY_514	0x514
> > +#define RCAR_GEN4_PCIE_PHY_700	0x700
> > +
> >  #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
> >
> > +#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_enable)(struct rcar_gen4_pcie *rcar);
> >  	enum dw_pcie_device_mode mode;
> >  };
> > @@ -57,12 +88,144 @@ 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;
> >  };
> >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> >
> >  /* Common */
> > +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_check(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)
> > +{
> > +	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\n", __func__);
> > +		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_check(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, RCAR_GEN4_PCIE_PHY_0f8, 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_check(rcar, PRTLGC89, BIT(30));
> > +			ret |= rcar_gen4_pcie_reg_check(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_enable_phy(struct rcar_gen4_pcie *rcar)
> > +{
> > +	struct dw_pcie *dw = &rcar->dw;
> > +	u32 val;
> > +	int ret;
> > +
> > +	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);
> > +
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(28), 0);
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(20), 0);
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(12), 0);
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(4), 0);
> > +
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > +					   GENMASK(23, 22), BIT(22));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > +					   GENMASK(18, 16), GENMASK(17, 16));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > +					   GENMASK(7, 6), BIT(6));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > +					   GENMASK(2, 0), GENMASK(11, 0));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_1d4,
> > +					   GENMASK(16, 15), GENMASK(16, 15));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_514, BIT(26), BIT(26));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, BIT(16), 0);
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, 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 + RCAR_GEN4_PCIE_PHY_0f8, 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;
> > +}
> > +
> 
> Is there a reason why you didn't chose to implement this as a PHY driver
> in drivers/phy ?

This is because the initialization needs both PCIe and PHY registers' settings
alternately like below:
 Write PHY regs
 Write PCI regs
 Read a PHY reg
 Write PCI regs
 Write a PHY reg
 ...

Best regards,
Yoshihiro Shimoda

> 
> Kind regards,
> Niklas
> 
> 
> >  static void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
> >  					 bool enable)
> >  {
> > @@ -200,6 +363,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:
> > @@ -241,6 +407,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");
> >
> > @@ -517,6 +687,31 @@ static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> >  	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 int rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> > +{
> > +	return rcar_gen4_pcie_enable_phy(rcar);
> > +}
> > +
> >  static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
> >  	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
> >  	.mode = DW_PCIE_RC_TYPE,
> > @@ -528,10 +723,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_enable = rcar_gen4_pcie_ltssm_enable,
> >  	.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_enable = rcar_gen4_pcie_ltssm_enable,
> >  	.mode = DW_PCIE_EP_TYPE,
> >  };
> >
> > --
> > 2.25.1
> >

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Niklas Cassel]

On Mon, Apr 15, 2024 at 09:19:43AM +0000, Yoshihiro Shimoda wrote:
> Hi Niklas,
> 
> > From: Niklas Cassel, Sent: Monday, April 15, 2024 5:27 PM
> > 
> > On Mon, Apr 15, 2024 at 05:11:34PM +0900, Yoshihiro Shimoda wrote:
> > > This driver previously supported r8a779f0 (R-Car S4-8). Add support
> > > for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
> > > So, convert it to a binary file by using a script.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 201 +++++++++++++++++++-
> > >  1 file changed, 200 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > index 980a916933d6..4e934e9156f2 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,47 @@
> > >  #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)
> > > +
> > > +/*
> > > + * The R-Car Gen4 documents don't describe the PHY registers' name.
> > > + * But, the initialization procedure describes these offsets. So,
> > > + * this driver makes up own #defines for the offsets.
> > > + */
> > > +#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
> > > +#define RCAR_GEN4_PCIE_PHY_148	0x148
> > > +#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
> > > +#define RCAR_GEN4_PCIE_PHY_514	0x514
> > > +#define RCAR_GEN4_PCIE_PHY_700	0x700
> > > +
> > >  #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
> > >
> > > +#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_enable)(struct rcar_gen4_pcie *rcar);
> > >  	enum dw_pcie_device_mode mode;
> > >  };
> > > @@ -57,12 +88,144 @@ 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;
> > >  };
> > >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> > >
> > >  /* Common */
> > > +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_check(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)
> > > +{
> > > +	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\n", __func__);
> > > +		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_check(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, RCAR_GEN4_PCIE_PHY_0f8, 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_check(rcar, PRTLGC89, BIT(30));
> > > +			ret |= rcar_gen4_pcie_reg_check(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_enable_phy(struct rcar_gen4_pcie *rcar)
> > > +{
> > > +	struct dw_pcie *dw = &rcar->dw;
> > > +	u32 val;
> > > +	int ret;
> > > +
> > > +	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);
> > > +
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(28), 0);
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(20), 0);
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(12), 0);
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(4), 0);
> > > +
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > > +					   GENMASK(23, 22), BIT(22));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > > +					   GENMASK(18, 16), GENMASK(17, 16));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > > +					   GENMASK(7, 6), BIT(6));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > > +					   GENMASK(2, 0), GENMASK(11, 0));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_1d4,
> > > +					   GENMASK(16, 15), GENMASK(16, 15));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_514, BIT(26), BIT(26));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, BIT(16), 0);
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, 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 + RCAR_GEN4_PCIE_PHY_0f8, 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;
> > > +}
> > > +
> > 
> > Is there a reason why you didn't chose to implement this as a PHY driver
> > in drivers/phy ?
> 
> This is because the initialization needs both PCIe and PHY registers' settings
> alternately like below:
>  Write PHY regs
>  Write PCI regs
>  Read a PHY reg
>  Write PCI regs
>  Write a PHY reg
>  ...
> 
> Best regards,
> Yoshihiro Shimoda

I see, that makes sense. Thanks.


Kind regards,
Niklas

Re: [PATCH v7 7/7] misc: pci_endpoint_test: Document a policy about adding pci_device_id

[Frank Li]

On Mon, Apr 15, 2024 at 05:11:35PM +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.
> 
> 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>

> ---
> Cc: 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 v7 4/7] PCI: rcar-gen4: Add rcar_gen4_pcie_drvdata

[Manivannan Sadhasivam]

On Mon, Apr 15, 2024 at 05:11:32PM +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.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

One nitpick below. With that addressed,

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

> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 30 ++++++++++++++-------
>  1 file changed, 21 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..3da0a844e1b6 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,9 @@ 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);

Even though rcar->drvdata won't be NULL, the lack of NULL check will cause
folks to send fixup patch later. So please add a NULL check here itself.

- Mani

>  
> -	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 +484,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 +504,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 v7 5/7] PCI: rcar-gen4: Add .ltssm_enable() for other SoC support

[Manivannan Sadhasivam]

PCI: rcar-gen4: Move LTSSM handling to ltssm_control() callback

On Mon, Apr 15, 2024 at 05:11:33PM +0900, Yoshihiro Shimoda wrote:
> This driver can reuse other R-Car Gen4 SoCs support like r8a779g0 and
> r8a779h0. However, r8a779g0 and r8a779h0 require other initializing
> settings that differ than r8a779f0. So, add a new function pointer
> .ltssm_enable() for it.
> 
> After applied this patch, probing SoCs by rcar_gen4_pcie_of_match[]
> will be changed like below:
> 
> - r8a779f0 as "renesas,r8a779f0-pcie" and "renesas,r8a779f0-pcie-ep"
> 
> Existing dts files have the compatible above. So, no behavior changes.
> 

How about:

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 | 41 ++++++++++++++++++---
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index 3da0a844e1b6..980a916933d6 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_enable)(struct rcar_gen4_pcie *rcar);

int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);

>  	enum dw_pcie_device_mode mode;
>  };
>  
> @@ -61,8 +63,8 @@ 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)
> +static void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
> +					 bool enable)
>  {
>  	u32 val;
>  
> @@ -127,9 +129,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_enable) {
> +		ret = rcar->drvdata->ltssm_enable(rcar);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/*
>  	 * Require direct speed change with retrying here if the link_gen is
> @@ -157,7 +163,7 @@ 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);
> +	rcar_gen4_pcie_ltssm_control(rcar, false);

You should use the callback here as like above.

>  }
>  
>  static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
> @@ -504,6 +510,23 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
>  	rcar_gen4_pcie_unprepare(rcar);
>  }
>  
> +static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> +{
> +	rcar_gen4_pcie_ltssm_control(rcar, true);
> +
> +	return 0;
> +}
> +
> +static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
> +	.ltssm_enable = r8a779f0_pcie_ltssm_enable,

Just pass rcar_gen4_pcie_ltssm_control() directly.

- Mani

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

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Manivannan Sadhasivam]

On Mon, Apr 15, 2024 at 05:11:34PM +0900, Yoshihiro Shimoda wrote:
> This driver previously supported r8a779f0 (R-Car S4-8). Add support
> for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
> So, convert it to a binary file by using a script.

The firmware is expected to be present in userspace. So where is it btw? Is it
upstreamed to linux-firmware?

You cannot ask users to manually copy the text and convert it to a binary file.
But if the firmware or sequence is not going to change, why can't you hardcode
it in the driver itself?

> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 201 +++++++++++++++++++-
>  1 file changed, 200 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index 980a916933d6..4e934e9156f2 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,47 @@
>  #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)
> +
> +/*
> + * The R-Car Gen4 documents don't describe the PHY registers' name.
> + * But, the initialization procedure describes these offsets. So,
> + * this driver makes up own #defines for the offsets.
> + */
> +#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
> +#define RCAR_GEN4_PCIE_PHY_148	0x148
> +#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
> +#define RCAR_GEN4_PCIE_PHY_514	0x514
> +#define RCAR_GEN4_PCIE_PHY_700	0x700
> +

As I said before, these defines provide no information about the registers at
all. So please use the offset directly and add a comment.

>  #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
>  
> +#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);

What is this init for? Controller? PHY?

>  	int (*ltssm_enable)(struct rcar_gen4_pcie *rcar);
>  	enum dw_pcie_device_mode mode;
>  };
> @@ -57,12 +88,144 @@ 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;
>  };
>  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
>  
>  /* Common */
> +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_check(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;

What is this function checking actually? It is just a DBI read. Do you expect
these register accesses to fail?

> +
> +	return 0;
> +}
> +
> +static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
> +{
> +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};

What does this check_addr corresponds to?

> +	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\n", __func__);

Print the firmware name in the case of error.

But as I said above, please try to hardcode the fw if it is not going to change.
We do this in other drivers as well.

- Mani

> +		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_check(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, RCAR_GEN4_PCIE_PHY_0f8, 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_check(rcar, PRTLGC89, BIT(30));
> +			ret |= rcar_gen4_pcie_reg_check(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_enable_phy(struct rcar_gen4_pcie *rcar)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 val;
> +	int ret;
> +
> +	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);
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(28), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(20), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(12), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(4), 0);
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> +					   GENMASK(23, 22), BIT(22));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> +					   GENMASK(18, 16), GENMASK(17, 16));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> +					   GENMASK(7, 6), BIT(6));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> +					   GENMASK(2, 0), GENMASK(11, 0));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_1d4,
> +					   GENMASK(16, 15), GENMASK(16, 15));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_514, BIT(26), BIT(26));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, BIT(16), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, 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 + RCAR_GEN4_PCIE_PHY_0f8, 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 void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
>  					 bool enable)
>  {
> @@ -200,6 +363,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:
> @@ -241,6 +407,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");
>  
> @@ -517,6 +687,31 @@ static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
>  	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 int rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> +{
> +	return rcar_gen4_pcie_enable_phy(rcar);
> +}
> +
>  static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
>  	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
>  	.mode = DW_PCIE_RC_TYPE,
> @@ -528,10 +723,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_enable = rcar_gen4_pcie_ltssm_enable,
>  	.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_enable = rcar_gen4_pcie_ltssm_enable,
>  	.mode = DW_PCIE_EP_TYPE,
>  };
>  
> -- 
> 2.25.1
> 
> 

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

Re: [PATCH v7 4/7] PCI: rcar-gen4: Add rcar_gen4_pcie_drvdata

[Geert Uytterhoeven]

Hi Mani,

On Sat, May 11, 2024 at 9:37 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> On Mon, Apr 15, 2024 at 05:11:32PM +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.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>
> One nitpick below. With that addressed,
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c

> > @@ -437,9 +441,9 @@ 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);
>
> Even though rcar->drvdata won't be NULL, the lack of NULL check will cause
> folks to send fixup patch later. So please add a NULL check here itself.

I tend to disagree: this can never return NULL.
Less than half of the callers of of_device_get_match_data() check for
a NULL pointer, and many of them do so because they are used both
with and without DT.

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 v7 5/7] PCI: rcar-gen4: Add .ltssm_enable() for other SoC support

[Yoshihiro Shimoda]

Hi Manivannan,

> From: Manivannan Sadhasivam, Sent: Saturday, May 11, 2024 4:38 PM
> 
> PCI: rcar-gen4: Move LTSSM handling to ltssm_control() callback
> 
> On Mon, Apr 15, 2024 at 05:11:33PM +0900, Yoshihiro Shimoda wrote:
> > This driver can reuse other R-Car Gen4 SoCs support like r8a779g0 and
> > r8a779h0. However, r8a779g0 and r8a779h0 require other initializing
> > settings that differ than r8a779f0. So, add a new function pointer
> > .ltssm_enable() for it.
> >
> > After applied this patch, probing SoCs by rcar_gen4_pcie_of_match[]
> > will be changed like below:
> >
> > - r8a779f0 as "renesas,r8a779f0-pcie" and "renesas,r8a779f0-pcie-ep"
> >
> > Existing dts files have the compatible above. So, no behavior changes.
> >
> 
> How about:
> 
> 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.

Thank you for suggestion. It good to me. I'll use this description.

> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 41 ++++++++++++++++++---
> >  1 file changed, 36 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index 3da0a844e1b6..980a916933d6 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_enable)(struct rcar_gen4_pcie *rcar);
> 
> int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);

I got it.

> >  	enum dw_pcie_device_mode mode;
> >  };
> >
> > @@ -61,8 +63,8 @@ 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)
> > +static void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
> > +					 bool enable)
> >  {
> >  	u32 val;
> >
> > @@ -127,9 +129,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_enable) {
> > +		ret = rcar->drvdata->ltssm_enable(rcar);
> > +		if (ret)
> > +			return ret;
> > +	}
> >
> >  	/*
> >  	 * Require direct speed change with retrying here if the link_gen is
> > @@ -157,7 +163,7 @@ 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);
> > +	rcar_gen4_pcie_ltssm_control(rcar, false);
> 
> You should use the callback here as like above.

I got it. JFYI, the current patch used rcar_gen4_pcie_ltssm_control(rcar, false) for
both r8a779f0 and r8a779g0. So, I'll modify the rcar_gen4_pcie_ltssm_enable() for case of false.

> >  }
> >
> >  static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
> > @@ -504,6 +510,23 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
> >  	rcar_gen4_pcie_unprepare(rcar);
> >  }
> >
> > +static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> > +{
> > +	rcar_gen4_pcie_ltssm_control(rcar, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
> > +	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
> 
> Just pass rcar_gen4_pcie_ltssm_control() directly.

I got it.

Best regards,
Yoshihiro Shimoda

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

RE: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Yoshihiro Shimoda]

Hi Manivannan,

> From: Manivannan Sadhasivam, Sent: Saturday, May 11, 2024 5:03 PM
> 
> On Mon, Apr 15, 2024 at 05:11:34PM +0900, Yoshihiro Shimoda wrote:
> > This driver previously supported r8a779f0 (R-Car S4-8). Add support
> > for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
> > So, convert it to a binary file by using a script.
> 
> The firmware is expected to be present in userspace. So where is it btw? Is it
> upstreamed to linux-firmware?

I may misunderstand your question, but the firmware is in the SoC's datasheet as
a text file. So, we need to convert it to a binary file and store it into the rootfs.
(Also, for debug purpose, we can use built-in firmware from "CONFIG_EXTRA_FIRMWARE".)

> You cannot ask users to manually copy the text and convert it to a binary file.
> But if the firmware or sequence is not going to change, why can't you hardcode
> it in the driver itself?

This is because that Renesas is not able to distribute the firmware freely.

> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 201 +++++++++++++++++++-
> >  1 file changed, 200 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index 980a916933d6..4e934e9156f2 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,47 @@
> >  #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)
> > +
> > +/*
> > + * The R-Car Gen4 documents don't describe the PHY registers' name.
> > + * But, the initialization procedure describes these offsets. So,
> > + * this driver makes up own #defines for the offsets.
> > + */
> > +#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
> > +#define RCAR_GEN4_PCIE_PHY_148	0x148
> > +#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
> > +#define RCAR_GEN4_PCIE_PHY_514	0x514
> > +#define RCAR_GEN4_PCIE_PHY_700	0x700
> > +
> 
> As I said before, these defines provide no information about the registers at
> all. So please use the offset directly and add a comment.

I got it.

> >  #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
> >
> > +#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);
> 
> What is this init for? Controller? PHY?

This init is for controller.

> >  	int (*ltssm_enable)(struct rcar_gen4_pcie *rcar);
> >  	enum dw_pcie_device_mode mode;
> >  };
> > @@ -57,12 +88,144 @@ 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;
> >  };
> >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> >
> >  /* Common */
> > +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_check(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;
> 
> What is this function checking actually? It is just a DBI read. Do you expect
> these register accesses to fail?

This function is checking whether the register's value with mask is zero or not.
- If non-zero, return -EAGAIN.
- If zero, return 0. (this is expected value.)

Perhaps, should I change the function name? For example, rcar_gen4_pcie_reg_test_bit()?
According to the datasheet, software needs to write registers again if the register
value(s) is(are) not expected value(s).

> > +
> > +	return 0;
> > +}
> > +
> > +static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
> > +{
> > +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
> 
> What does this check_addr corresponds to?

I don't know. The datasheet just said that like other magic numbers.
I'll add such a comment here.

> > +	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\n", __func__);
> 
> Print the firmware name in the case of error.

I got it.

> But as I said above, please try to hardcode the fw if it is not going to change.
> We do this in other drivers as well.

As I mentioned above, I would like to keep calling this function.

Best regards,
Yoshihiro Shimoda

> - Mani
> 
> > +		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_check(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, RCAR_GEN4_PCIE_PHY_0f8, 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_check(rcar, PRTLGC89, BIT(30));
> > +			ret |= rcar_gen4_pcie_reg_check(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_enable_phy(struct rcar_gen4_pcie *rcar)
> > +{
> > +	struct dw_pcie *dw = &rcar->dw;
> > +	u32 val;
> > +	int ret;
> > +
> > +	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);
> > +
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(28), 0);
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(20), 0);
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(12), 0);
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(4), 0);
> > +
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > +					   GENMASK(23, 22), BIT(22));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > +					   GENMASK(18, 16), GENMASK(17, 16));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > +					   GENMASK(7, 6), BIT(6));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > +					   GENMASK(2, 0), GENMASK(11, 0));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_1d4,
> > +					   GENMASK(16, 15), GENMASK(16, 15));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_514, BIT(26), BIT(26));
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, BIT(16), 0);
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, 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 + RCAR_GEN4_PCIE_PHY_0f8, 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 void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
> >  					 bool enable)
> >  {
> > @@ -200,6 +363,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:
> > @@ -241,6 +407,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");
> >
> > @@ -517,6 +687,31 @@ static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> >  	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 int rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> > +{
> > +	return rcar_gen4_pcie_enable_phy(rcar);
> > +}
> > +
> >  static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
> >  	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
> >  	.mode = DW_PCIE_RC_TYPE,
> > @@ -528,10 +723,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_enable = rcar_gen4_pcie_ltssm_enable,
> >  	.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_enable = rcar_gen4_pcie_ltssm_enable,
> >  	.mode = DW_PCIE_EP_TYPE,
> >  };
> >
> > --
> > 2.25.1
> >
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v7 4/7] PCI: rcar-gen4: Add rcar_gen4_pcie_drvdata

[Manivannan Sadhasivam]

On Mon, May 13, 2024 at 11:20:56AM +0200, Geert Uytterhoeven wrote:
> Hi Mani,
> 
> On Sat, May 11, 2024 at 9:37 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
> > On Mon, Apr 15, 2024 at 05:11:32PM +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.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > One nitpick below. With that addressed,
> >
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> > > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> 
> > > @@ -437,9 +441,9 @@ 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);
> >
> > Even though rcar->drvdata won't be NULL, the lack of NULL check will cause
> > folks to send fixup patch later. So please add a NULL check here itself.
> 
> I tend to disagree: this can never return NULL.
> Less than half of the callers of of_device_get_match_data() check for
> a NULL pointer, and many of them do so because they are used both
> with and without DT.
> 

As I said, there is no way it can be NULL. But folks tend to send the 'fix'
patches as the automated tools report these kind of non-existent issues. And I
agree that we can decide not to accept those patches, but I just wanted to avoid
that noise. As a maintainer, I try to avoid seeing those kind of patches if
there is a way we can avoid it.

But no strong preference here.

- Mani

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

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Manivannan Sadhasivam]

On Tue, May 14, 2024 at 11:27:30AM +0000, Yoshihiro Shimoda wrote:
> Hi Manivannan,
> 
> > From: Manivannan Sadhasivam, Sent: Saturday, May 11, 2024 5:03 PM
> > 
> > On Mon, Apr 15, 2024 at 05:11:34PM +0900, Yoshihiro Shimoda wrote:
> > > This driver previously supported r8a779f0 (R-Car S4-8). Add support
> > > for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
> > > So, convert it to a binary file by using a script.
> > 
> > The firmware is expected to be present in userspace. So where is it btw? Is it
> > upstreamed to linux-firmware?
> 
> I may misunderstand your question, but the firmware is in the SoC's datasheet as
> a text file. So, we need to convert it to a binary file and store it into the rootfs.
> (Also, for debug purpose, we can use built-in firmware from "CONFIG_EXTRA_FIRMWARE".)
> 
> > You cannot ask users to manually copy the text and convert it to a binary file.
> > But if the firmware or sequence is not going to change, why can't you hardcode
> > it in the driver itself?
> 
> This is because that Renesas is not able to distribute the firmware freely.
> 

Seriously? Are you saying that the user has to sign NDA to get this firmware?

- Mani

> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 201 +++++++++++++++++++-
> > >  1 file changed, 200 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > index 980a916933d6..4e934e9156f2 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,47 @@
> > >  #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)
> > > +
> > > +/*
> > > + * The R-Car Gen4 documents don't describe the PHY registers' name.
> > > + * But, the initialization procedure describes these offsets. So,
> > > + * this driver makes up own #defines for the offsets.
> > > + */
> > > +#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
> > > +#define RCAR_GEN4_PCIE_PHY_148	0x148
> > > +#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
> > > +#define RCAR_GEN4_PCIE_PHY_514	0x514
> > > +#define RCAR_GEN4_PCIE_PHY_700	0x700
> > > +
> > 
> > As I said before, these defines provide no information about the registers at
> > all. So please use the offset directly and add a comment.
> 
> I got it.
> 
> > >  #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
> > >
> > > +#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);
> > 
> > What is this init for? Controller? PHY?
> 
> This init is for controller.
> 
> > >  	int (*ltssm_enable)(struct rcar_gen4_pcie *rcar);
> > >  	enum dw_pcie_device_mode mode;
> > >  };
> > > @@ -57,12 +88,144 @@ 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;
> > >  };
> > >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> > >
> > >  /* Common */
> > > +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_check(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;
> > 
> > What is this function checking actually? It is just a DBI read. Do you expect
> > these register accesses to fail?
> 
> This function is checking whether the register's value with mask is zero or not.
> - If non-zero, return -EAGAIN.
> - If zero, return 0. (this is expected value.)
> 
> Perhaps, should I change the function name? For example, rcar_gen4_pcie_reg_test_bit()?
> According to the datasheet, software needs to write registers again if the register
> value(s) is(are) not expected value(s).
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
> > > +{
> > > +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
> > 
> > What does this check_addr corresponds to?
> 
> I don't know. The datasheet just said that like other magic numbers.
> I'll add such a comment here.
> 
> > > +	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\n", __func__);
> > 
> > Print the firmware name in the case of error.
> 
> I got it.
> 
> > But as I said above, please try to hardcode the fw if it is not going to change.
> > We do this in other drivers as well.
> 
> As I mentioned above, I would like to keep calling this function.
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > - Mani
> > 
> > > +		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_check(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, RCAR_GEN4_PCIE_PHY_0f8, 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_check(rcar, PRTLGC89, BIT(30));
> > > +			ret |= rcar_gen4_pcie_reg_check(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_enable_phy(struct rcar_gen4_pcie *rcar)
> > > +{
> > > +	struct dw_pcie *dw = &rcar->dw;
> > > +	u32 val;
> > > +	int ret;
> > > +
> > > +	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);
> > > +
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(28), 0);
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(20), 0);
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(12), 0);
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_700, BIT(4), 0);
> > > +
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > > +					   GENMASK(23, 22), BIT(22));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > > +					   GENMASK(18, 16), GENMASK(17, 16));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > > +					   GENMASK(7, 6), BIT(6));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_148,
> > > +					   GENMASK(2, 0), GENMASK(11, 0));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_1d4,
> > > +					   GENMASK(16, 15), GENMASK(16, 15));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_514, BIT(26), BIT(26));
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, BIT(16), 0);
> > > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, 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 + RCAR_GEN4_PCIE_PHY_0f8, 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 void rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar,
> > >  					 bool enable)
> > >  {
> > > @@ -200,6 +363,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:
> > > @@ -241,6 +407,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");
> > >
> > > @@ -517,6 +687,31 @@ static int r8a779f0_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> > >  	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 int rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar)
> > > +{
> > > +	return rcar_gen4_pcie_enable_phy(rcar);
> > > +}
> > > +
> > >  static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
> > >  	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
> > >  	.mode = DW_PCIE_RC_TYPE,
> > > @@ -528,10 +723,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_enable = rcar_gen4_pcie_ltssm_enable,
> > >  	.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_enable = rcar_gen4_pcie_ltssm_enable,
> > >  	.mode = DW_PCIE_EP_TYPE,
> > >  };
> > >
> > > --
> > > 2.25.1
> > >
> > >
> > 
> > --
> > மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]


> > This is because that Renesas is not able to distribute the firmware freely.
> > 
> 
> Seriously? Are you saying that the user has to sign NDA to get this firmware?

No, the user has to buy the SoC and will get the firmware with the
documentation. Renesas is not allowed to distribute the firmware to
non-users of the SoC. So, linux-firmware cannot be used, sadly. We all
wish it would be different.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]


> No, the user has to buy the SoC and will get the firmware with the
> documentation. Renesas is not allowed to distribute the firmware to
> non-users of the SoC. So, linux-firmware cannot be used, sadly. We all
> wish it would be different.

I am not a lawyer and all this. But this is how I, as a contractor
working for Renesas, understand the situation.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Niklas Cassel]

On Wed, May 15, 2024 at 10:59:39AM +0200, Wolfram Sang wrote:
> 
> > > This is because that Renesas is not able to distribute the firmware freely.
> > > 
> > 
> > Seriously? Are you saying that the user has to sign NDA to get this firmware?
> 
> No, the user has to buy the SoC and will get the firmware with the
> documentation. Renesas is not allowed to distribute the firmware to
> non-users of the SoC. So, linux-firmware cannot be used, sadly. We all
> wish it would be different.

If Renesas could bother to spend the effort to be legally allowed to
include the firmware in linux-firmware, do we want to spend the effort
to maintain the support for this PCIe controller in mainline?

Is there even an example of a device driver that *requires* firmware
that is not in linux-firmware?

(I know of some device drivers that does not have firmware in
linux-firmware, but in that case the firmware is *optional*,
so the device driver still works even without loading an updated
firmware. For this patch series, the driver seems to error out if
request_firmware fails.)


Kind regards,
Niklas

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

Hi,

> If Renesas could bother to spend the effort to be legally allowed to
> include the firmware in linux-firmware, do we want to spend the effort
> to maintain the support for this PCIe controller in mainline?

We (including me) have no indication if Renesas bothered or not. Maybe
they tried but could not convince a third party. I don't know.

> Is there even an example of a device driver that *requires* firmware
> that is not in linux-firmware?

Sure thing!

=== snip here

$ cat Documentation/admin-guide/media/opera-firmware.rst
.. SPDX-License-Identifier: GPL-2.0

Opera firmware
==============

Author: Marco Gittler <g.marco@freenet.de>

To extract the firmware for the Opera DVB-S1 USB-Box
you need to copy the files:

2830SCap2.sys
2830SLoad2.sys

from the windriver disk into this directory.

Then run:

.. code-block:: none

	scripts/get_dvb_firmware opera1

and after that you have 2 files:

dvb-usb-opera-01.fw
dvb-usb-opera1-fpga-01.fw

in here.

Copy them into /lib/firmware/ .

After that the driver can load the firmware
(if you have enabled firmware loading
in kernel config and have hotplug running).

=== snap

And this is probably not the only driver where you need to get firmware
out of a Windows driver (media drivers and webcam drivers are good
candidates). And the redistribution of such drivers is likely to be
limited as well.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Manivannan Sadhasivam]

On Wed, May 15, 2024 at 10:59:39AM +0200, Wolfram Sang wrote:
> 
> > > This is because that Renesas is not able to distribute the firmware freely.
> > > 
> > 
> > Seriously? Are you saying that the user has to sign NDA to get this firmware?
> 
> No, the user has to buy the SoC and will get the firmware with the
> documentation. Renesas is not allowed to distribute the firmware to
> non-users of the SoC. So, linux-firmware cannot be used, sadly. We all
> wish it would be different.
> 

Okay. I think this could be _one_ of the exceptions we have in the kernel, but
I'll leave it up to Lorenzo/Krzysztof (PCIe controller maintainers) to make a
call.

- Mani

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

Re: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Manivannan Sadhasivam]

On Tue, May 14, 2024 at 11:27:30AM +0000, Yoshihiro Shimoda wrote:
> Hi Manivannan,
> 
> > From: Manivannan Sadhasivam, Sent: Saturday, May 11, 2024 5:03 PM
> > 
> > On Mon, Apr 15, 2024 at 05:11:34PM +0900, Yoshihiro Shimoda wrote:
> > > This driver previously supported r8a779f0 (R-Car S4-8). Add support
> > > for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
> > > So, convert it to a binary file by using a script.
> > 
> > The firmware is expected to be present in userspace. So where is it btw? Is it
> > upstreamed to linux-firmware?
> 
> I may misunderstand your question, but the firmware is in the SoC's datasheet as
> a text file. So, we need to convert it to a binary file and store it into the rootfs.
> (Also, for debug purpose, we can use built-in firmware from "CONFIG_EXTRA_FIRMWARE".)
> 

So how does the conversion need to happen? Does it require any tool or just copy
the content to a file and pass it as a firmware?

Whatever the method is, it should be documented in the commit message (also in
the driver).

> > You cannot ask users to manually copy the text and convert it to a binary file.
> > But if the firmware or sequence is not going to change, why can't you hardcode
> > it in the driver itself?
> 
> This is because that Renesas is not able to distribute the firmware freely.
> 
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 201 +++++++++++++++++++-
> > >  1 file changed, 200 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > index 980a916933d6..4e934e9156f2 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,47 @@
> > >  #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)
> > > +
> > > +/*
> > > + * The R-Car Gen4 documents don't describe the PHY registers' name.
> > > + * But, the initialization procedure describes these offsets. So,
> > > + * this driver makes up own #defines for the offsets.
> > > + */
> > > +#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
> > > +#define RCAR_GEN4_PCIE_PHY_148	0x148
> > > +#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
> > > +#define RCAR_GEN4_PCIE_PHY_514	0x514
> > > +#define RCAR_GEN4_PCIE_PHY_700	0x700
> > > +
> > 
> > As I said before, these defines provide no information about the registers at
> > all. So please use the offset directly and add a comment.
> 
> I got it.
> 
> > >  #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
> > >
> > > +#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);
> > 
> > What is this init for? Controller? PHY?
> 
> This init is for controller.
> 

Why do you need a callback for this? There is just a single function that is
used as of now, so please move the contents to rcar_gen4_pcie_common_init().

> > >  	int (*ltssm_enable)(struct rcar_gen4_pcie *rcar);
> > >  	enum dw_pcie_device_mode mode;
> > >  };
> > > @@ -57,12 +88,144 @@ 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;
> > >  };
> > >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> > >
> > >  /* Common */
> > > +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_check(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;
> > 
> > What is this function checking actually? It is just a DBI read. Do you expect
> > these register accesses to fail?
> 
> This function is checking whether the register's value with mask is zero or not.
> - If non-zero, return -EAGAIN.
> - If zero, return 0. (this is expected value.)
> 
> Perhaps, should I change the function name? For example, rcar_gen4_pcie_reg_test_bit()?
> According to the datasheet, software needs to write registers again if the register
> value(s) is(are) not expected value(s).
> 

Well, I was asking under what circumstances the write may result in non-zero
value?

- Mani

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

Re: [PATCH v7 0/7] PCI: rcar-gen4: Add R-Car V4H support

[Krzysztof Wilczyński]

Hello,

> 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 downloading, we can get information from
> the following wiki page:
> https://elinux.org/index.php?title=R-Car/Boards/WhiteHawk&oldid=581944#PCIe_firmware
> 
> 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.

Applied to dt-bindings, thank you!

[01/02] dt-bindings: PCI: rcar-gen4-pci-host: Add R-Car V4H compatible
        https://git.kernel.org/pci/pci/c/5220b11a5beb
[02/02] dt-bindings: PCI: rcar-gen4-pci-ep: Add R-Car V4H compatible
        https://git.kernel.org/pci/pci/c/c037263db4ce

	Krzysztof

RE: [PATCH v7 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Yoshihiro Shimoda]

Hi Manivannan,

> From: Manivannan Sadhasivam, Sent: Thursday, May 16, 2024 9:52 PM
> 
> On Tue, May 14, 2024 at 11:27:30AM +0000, Yoshihiro Shimoda wrote:
> > Hi Manivannan,
> >
> > > From: Manivannan Sadhasivam, Sent: Saturday, May 11, 2024 5:03 PM
> > >
> > > On Mon, Apr 15, 2024 at 05:11:34PM +0900, Yoshihiro Shimoda wrote:
> > > > This driver previously supported r8a779f0 (R-Car S4-8). Add support
> > > > for r8a779g0 (R-Car V4H). 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 firmware is attached in the manual of the r8a779g0 as text.
> > > > So, convert it to a binary file by using a script.
> > >
> > > The firmware is expected to be present in userspace. So where is it btw? Is it
> > > upstreamed to linux-firmware?
> >
> > I may misunderstand your question, but the firmware is in the SoC's datasheet as
> > a text file. So, we need to convert it to a binary file and store it into the rootfs.
> > (Also, for debug purpose, we can use built-in firmware from "CONFIG_EXTRA_FIRMWARE".)
> >
> 
> So how does the conversion need to happen? Does it require any tool or just copy
> the content to a file and pass it as a firmware?

I'm sorry for lack explanation. The way is:
1) copy a text file to a PC.
2) do a script with the text file, and then generate a binary file.
3) copy the binary file to the rootfs of the R-Car.

> Whatever the method is, it should be documented in the commit message (also in
> the driver).

I got it. I'll add such description in the commit and the driver.

> > > You cannot ask users to manually copy the text and convert it to a binary file.
> > > But if the firmware or sequence is not going to change, why can't you hardcode
> > > it in the driver itself?
> >
> > This is because that Renesas is not able to distribute the firmware freely.
> >
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 201 +++++++++++++++++++-
> > > >  1 file changed, 200 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > > index 980a916933d6..4e934e9156f2 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,47 @@
> > > >  #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)
> > > > +
> > > > +/*
> > > > + * The R-Car Gen4 documents don't describe the PHY registers' name.
> > > > + * But, the initialization procedure describes these offsets. So,
> > > > + * this driver makes up own #defines for the offsets.
> > > > + */
> > > > +#define RCAR_GEN4_PCIE_PHY_0f8	0x0f8
> > > > +#define RCAR_GEN4_PCIE_PHY_148	0x148
> > > > +#define RCAR_GEN4_PCIE_PHY_1d4	0x1d4
> > > > +#define RCAR_GEN4_PCIE_PHY_514	0x514
> > > > +#define RCAR_GEN4_PCIE_PHY_700	0x700
> > > > +
> > >
> > > As I said before, these defines provide no information about the registers at
> > > all. So please use the offset directly and add a comment.
> >
> > I got it.
> >
> > > >  #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
> > > >
> > > > +#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);
> > >
> > > What is this init for? Controller? PHY?
> >
> > This init is for controller.
> >
> 
> Why do you need a callback for this? There is just a single function that is
> used as of now, so please move the contents to rcar_gen4_pcie_common_init().

r8a779g0 requires the settings. But, r8a779f0 doesn't requires the setting.
So, we can not move the contents to rcar_gen4_pcie_common_init().

> > > >  	int (*ltssm_enable)(struct rcar_gen4_pcie *rcar);
> > > >  	enum dw_pcie_device_mode mode;
> > > >  };
> > > > @@ -57,12 +88,144 @@ 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;
> > > >  };
> > > >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> > > >
> > > >  /* Common */
> > > > +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_check(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;
> > >
> > > What is this function checking actually? It is just a DBI read. Do you expect
> > > these register accesses to fail?
> >
> > This function is checking whether the register's value with mask is zero or not.
> > - If non-zero, return -EAGAIN.
> > - If zero, return 0. (this is expected value.)
> >
> > Perhaps, should I change the function name? For example, rcar_gen4_pcie_reg_test_bit()?
> > According to the datasheet, software needs to write registers again if the register
> > value(s) is(are) not expected value(s).
> >
> 
> Well, I was asking under what circumstances the write may result in non-zero
> value?

The current implementation is:
-----
                        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_check(rcar, PRTLGC89, BIT(30)) >= 0)
                                break;
...
                        dw_pcie_writel_dbi(dw, PRTLGC89, check_addr[i]);
                        ret = rcar_gen4_pcie_reg_check(rcar, PRTLGC89, BIT(30));
                        ret |= rcar_gen4_pcie_reg_check(rcar, PRTLGC90, BIT(0));
                        if (ret >= 0)
                                break;
-----

This is from flowchart of the datasheet. Unfortunately, the datasheet doesn't describe detail.
But, I think that I should implement such a code. And, I assume that these registers have a flag
in the bits, so the write may result in non-zero value.

Best regards,
Yoshihiro Shimoda

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