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

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

[Yoshihiro Shimoda]

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

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

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

Changes from v8:
https://lore.kernel.org/linux-pci/20240520074300.125969-1-yoshihiro.shimoda.uh@renesas.com/
- Add Reviewed-by in the patch 3/5.
- Revise commit description in the patch [2345]/5.
- Some minor updates of the code in the patch 4/5.

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

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

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

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

-- 
2.25.1

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

[Yoshihiro Shimoda]

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

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

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index f8e5431a207b..0476f9b6186b 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 v9 2/5] PCI: rcar-gen4: Add rcar_gen4_pcie_drvdata

[Yoshihiro Shimoda]

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

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

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index cfeccc2f9ee1..eb3ea4a5fd0a 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;
@@ -451,9 +455,11 @@ static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
 /* Common */
 static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar)
 {
-	rcar->mode = (uintptr_t)of_device_get_match_data(&rcar->pdev->dev);
+	rcar->drvdata = of_device_get_match_data(&rcar->pdev->dev);
+	if (!rcar->drvdata)
+		return -EINVAL;
 
-	switch (rcar->mode) {
+	switch (rcar->drvdata->mode) {
 	case DW_PCIE_RC_TYPE:
 		return rcar_gen4_add_dw_pcie_rp(rcar);
 	case DW_PCIE_EP_TYPE:
@@ -494,7 +500,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;
@@ -514,14 +520,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 v9 3/5] PCI: rcar-gen4: Add .ltssm_control() for other SoC support

[Yoshihiro Shimoda]

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

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

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

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

[Yoshihiro Shimoda]

Add support for r8a779g0 (R-Car V4H).

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

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

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

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

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index dac78388975d..c67097e718d3 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,48 @@
 #define PCIEDMAINTSTSEN		0x0314
 #define PCIEDMAINTSTSEN_INIT	GENMASK(15, 0)
 
+/* Port Logic Registers 89 */
+#define PRTLGC89		0x0b70
+
+/* Port Logic Registers 90 */
+#define PRTLGC90		0x0b74
+
 /* PCIe Reset Control Register 1 */
 #define PCIERSTCTRL1		0x0014
 #define APP_HOLD_PHY_RST	BIT(16)
 #define APP_LTSSM_ENABLE	BIT(0)
 
+/* PCIe Power Management Control */
+#define PCIEPWRMNGCTRL		0x0070
+#define APP_CLK_REQ_N		BIT(11)
+#define APP_CLK_PM_EN		BIT(10)
+
 #define RCAR_NUM_SPEED_CHANGE_RETRIES	10
 #define RCAR_MAX_LINK_SPEED		4
 
 #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
 #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
 
+/*
+ * The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
+ * the datasheet is a text file. But, Renesas is not able to distribute
+ * the firmware freely. So, we require converting the text file
+ * to a binary before the driver runs by using the following script:
+ *
+ * $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
+ *      { print substr($2,5,2) substr($2,3,2) }' \
+ *      104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
+ *      rcar_gen4_pcie.bin
+ *    $ sha1sum rcar_gen4_pcie.bin
+ *      1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
+ */
+#define RCAR_GEN4_PCIE_FIRMWARE_NAME		"rcar_gen4_pcie.bin"
+#define RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR	0xc000
+MODULE_FIRMWARE(RCAR_GEN4_PCIE_FIRMWARE_NAME);
+
 struct rcar_gen4_pcie;
 struct rcar_gen4_pcie_drvdata {
+	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
 	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
 	enum dw_pcie_device_mode mode;
 };
@@ -57,6 +89,7 @@ struct rcar_gen4_pcie_drvdata {
 struct rcar_gen4_pcie {
 	struct dw_pcie dw;
 	void __iomem *base;
+	void __iomem *phy_base;
 	struct platform_device *pdev;
 	const struct rcar_gen4_pcie_drvdata *drvdata;
 };
@@ -180,6 +213,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
 	if (ret)
 		goto err_unprepare;
 
+	if (rcar->drvdata->additional_common_init)
+		rcar->drvdata->additional_common_init(rcar);
+
 	return 0;
 
 err_unprepare:
@@ -221,6 +257,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");
 
@@ -528,6 +568,167 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
 	return 0;
 }
 
+static void rcar_gen4_pcie_additional_common_init(struct rcar_gen4_pcie *rcar)
+{
+	struct dw_pcie *dw = &rcar->dw;
+	u32 val;
+
+	val = dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW);
+	val &= ~PORT_LANE_SKEW_INSERT_MASK;
+	if (dw->num_lanes < 4)
+		val |= BIT(6);
+	dw_pcie_writel_dbi(dw, PCIE_PORT_LANE_SKEW, val);
+
+	val = readl(rcar->base + PCIEPWRMNGCTRL);
+	val |= APP_CLK_REQ_N | APP_CLK_PM_EN;
+	writel(val, rcar->base + PCIEPWRMNGCTRL);
+}
+
+static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
+					       u32 offset, u32 mask, u32 val)
+{
+	u32 tmp;
+
+	tmp = readl(rcar->phy_base + offset);
+	tmp &= ~mask;
+	tmp |= val;
+	writel(tmp, rcar->phy_base + offset);
+}
+
+/*
+ * SoC datasheet suggests checking port logic register bits during firmware
+ * write. If read returns non-zero value, then this function returns -EAGAIN
+ * indicating that the write needs to be done again. If read returns zero,
+ * then return 0 to indicate success.
+ */
+static int rcar_gen4_pcie_reg_test_bit(struct rcar_gen4_pcie *rcar,
+				       u32 offset, u32 mask)
+{
+	struct dw_pcie *dw = &rcar->dw;
+
+	if (dw_pcie_readl_dbi(dw, offset) & mask)
+		return -EAGAIN;
+
+	return 0;
+}
+
+static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
+{
+	/* The check_addr values are magical numbers in the datasheet */
+	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
+	struct dw_pcie *dw = &rcar->dw;
+	const struct firmware *fw;
+	unsigned int i, timeout;
+	u32 data;
+	int ret;
+
+	ret = request_firmware(&fw, RCAR_GEN4_PCIE_FIRMWARE_NAME, dw->dev);
+	if (ret) {
+		dev_err(dw->dev, "Failed to load firmware (%s): %d\n",
+			RCAR_GEN4_PCIE_FIRMWARE_NAME, ret);
+		return ret;
+	}
+
+	for (i = 0; i < (fw->size / 2); i++) {
+		data = fw->data[(i * 2) + 1] << 8 | fw->data[i * 2];
+		timeout = 100;
+		do {
+			dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR + i);
+			dw_pcie_writel_dbi(dw, PRTLGC90, data);
+			if (!rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30)))
+				break;
+			if (!(--timeout)) {
+				ret = -ETIMEDOUT;
+				goto exit;
+			}
+			usleep_range(100, 200);
+		} while (1);
+	}
+
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
+
+	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
+		timeout = 100;
+		do {
+			dw_pcie_writel_dbi(dw, PRTLGC89, check_addr[i]);
+			ret = rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30));
+			ret |= rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC90, BIT(0));
+			if (!ret)
+				break;
+			if (!(--timeout)) {
+				ret = -ETIMEDOUT;
+				goto exit;
+			}
+			usleep_range(100, 200);
+		} while (1);
+	}
+
+exit:
+	release_firmware(fw);
+
+	return ret;
+}
+
+static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
+{
+	struct dw_pcie *dw = &rcar->dw;
+	u32 val;
+	int ret;
+
+	if (!enable) {
+		val = readl(rcar->base + PCIERSTCTRL1);
+		val &= ~APP_LTSSM_ENABLE;
+		writel(val, rcar->base + PCIERSTCTRL1);
+
+		return 0;
+	}
+
+	val = dw_pcie_readl_dbi(dw, PCIE_PORT_FORCE);
+	val |= PORT_FORCE_DO_DESKEW_FOR_SRIS;
+	dw_pcie_writel_dbi(dw, PCIE_PORT_FORCE, val);
+
+	val = readl(rcar->base + PCIEMSR0);
+	val |= APP_SRIS_MODE;
+	writel(val, rcar->base + PCIEMSR0);
+
+	/*
+	 * The R-Car Gen4 datasheet doesn't describe the PHY registers' name.
+	 * But, the initialization procedure describes these offsets. So,
+	 * this driver has magical offset numbers.
+	 */
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(28), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(20), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(12), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(4), 0);
+
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(23, 22), BIT(22));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(18, 16), GENMASK(17, 16));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(7, 6), BIT(6));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(2, 0), GENMASK(11, 0));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x1d4, GENMASK(16, 15), GENMASK(16, 15));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x514, BIT(26), BIT(26));
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(16), 0);
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(19), BIT(19));
+
+	val = readl(rcar->base + PCIERSTCTRL1);
+	val &= ~APP_HOLD_PHY_RST;
+	writel(val, rcar->base + PCIERSTCTRL1);
+
+	ret = readl_poll_timeout(rcar->phy_base + 0x0f8, val, !(val & BIT(18)), 100, 10000);
+	if (ret < 0)
+		return ret;
+
+	ret = rcar_gen4_pcie_download_phy_firmware(rcar);
+	if (ret)
+		return ret;
+
+	val = readl(rcar->base + PCIERSTCTRL1);
+	val |= APP_LTSSM_ENABLE;
+	writel(val, rcar->base + PCIERSTCTRL1);
+
+	return 0;
+}
+
 static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
 	.ltssm_control = r8a779f0_pcie_ltssm_control,
 	.mode = DW_PCIE_RC_TYPE,
@@ -539,10 +740,14 @@ static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
 };
 
 static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
+	.additional_common_init = rcar_gen4_pcie_additional_common_init,
+	.ltssm_control = rcar_gen4_pcie_ltssm_control,
 	.mode = DW_PCIE_RC_TYPE,
 };
 
 static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
+	.additional_common_init = rcar_gen4_pcie_additional_common_init,
+	.ltssm_control = rcar_gen4_pcie_ltssm_control,
 	.mode = DW_PCIE_EP_TYPE,
 };
 
-- 
2.25.1

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

[Yoshihiro Shimoda]

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

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

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index c38a6083f0a7..75ac447ffd34 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -980,6 +980,10 @@ static const struct pci_endpoint_test_data j721e_data = {
 	.irq_type = IRQ_TYPE_MSI,
 };
 
+/*
+ * If the controller's Vendor/Device ID are programmable, you may be able to
+ * use one of the existing entries for testing instead of adding a new one.
+ */
 static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x),
 	  .driver_data = (kernel_ulong_t)&default_data,
-- 
2.25.1

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

[Manivannan Sadhasivam]

On Tue, Jun 11, 2024 at 09:50:56PM +0900, Yoshihiro Shimoda wrote:
> Add support for r8a779g0 (R-Car V4H).
> 
> This driver previously supported r8a779f0 (R-Car S4-8). PCIe features
> of both r8a779f0 and r8a779g0 are almost all the same. For example:
>  - PCI Express Base Specification Revision 4.0
>  - Root complex mode and endpoint mode are supported
> However, r8a779g0 requires specific firmware downloading, to
> initialize the PHY. Otherwise, the PCIe controller cannot work.
> 
> The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> the datasheet is a text file. But, Renesas is not able to distribute
> the firmware freely. So, we require converting the text file
> to a binary before the driver runs by using the following script:
> 
>  $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
>    { print substr($2,5,2) substr($2,3,2) }' \
>    104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
>    rcar_gen4_pcie.bin
>  $ sha1sum rcar_gen4_pcie.bin
>    1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 207 +++++++++++++++++++-
>  1 file changed, 206 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index dac78388975d..c67097e718d3 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,48 @@
>  #define PCIEDMAINTSTSEN		0x0314
>  #define PCIEDMAINTSTSEN_INIT	GENMASK(15, 0)
>  
> +/* Port Logic Registers 89 */
> +#define PRTLGC89		0x0b70
> +
> +/* Port Logic Registers 90 */
> +#define PRTLGC90		0x0b74
> +
>  /* PCIe Reset Control Register 1 */
>  #define PCIERSTCTRL1		0x0014
>  #define APP_HOLD_PHY_RST	BIT(16)
>  #define APP_LTSSM_ENABLE	BIT(0)
>  
> +/* PCIe Power Management Control */
> +#define PCIEPWRMNGCTRL		0x0070
> +#define APP_CLK_REQ_N		BIT(11)
> +#define APP_CLK_PM_EN		BIT(10)
> +
>  #define RCAR_NUM_SPEED_CHANGE_RETRIES	10
>  #define RCAR_MAX_LINK_SPEED		4
>  
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
>  
> +/*
> + * The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> + * the datasheet is a text file. But, Renesas is not able to distribute
> + * the firmware freely. So, we require converting the text file
> + * to a binary before the driver runs by using the following script:
> + *
> + * $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
> + *      { print substr($2,5,2) substr($2,3,2) }' \
> + *      104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
> + *      rcar_gen4_pcie.bin
> + *    $ sha1sum rcar_gen4_pcie.bin
> + *      1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> + */
> +#define RCAR_GEN4_PCIE_FIRMWARE_NAME		"rcar_gen4_pcie.bin"
> +#define RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR	0xc000
> +MODULE_FIRMWARE(RCAR_GEN4_PCIE_FIRMWARE_NAME);
> +
>  struct rcar_gen4_pcie;
>  struct rcar_gen4_pcie_drvdata {
> +	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
>  	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
>  	enum dw_pcie_device_mode mode;
>  };
> @@ -57,6 +89,7 @@ struct rcar_gen4_pcie_drvdata {
>  struct rcar_gen4_pcie {
>  	struct dw_pcie dw;
>  	void __iomem *base;
> +	void __iomem *phy_base;
>  	struct platform_device *pdev;
>  	const struct rcar_gen4_pcie_drvdata *drvdata;
>  };
> @@ -180,6 +213,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
>  	if (ret)
>  		goto err_unprepare;
>  
> +	if (rcar->drvdata->additional_common_init)
> +		rcar->drvdata->additional_common_init(rcar);
> +
>  	return 0;
>  
>  err_unprepare:
> @@ -221,6 +257,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);
> +

I failed to spot this in earlier reviews. Since this 'phy' region is only
applicable for r8a779g0, wouldn't this fail on other platforms?

- Mani

>  	/* Renesas-specific registers */
>  	rcar->base = devm_platform_ioremap_resource_byname(rcar->pdev, "app");
>  
> @@ -528,6 +568,167 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
>  	return 0;
>  }
>  
> +static void rcar_gen4_pcie_additional_common_init(struct rcar_gen4_pcie *rcar)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 val;
> +
> +	val = dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW);
> +	val &= ~PORT_LANE_SKEW_INSERT_MASK;
> +	if (dw->num_lanes < 4)
> +		val |= BIT(6);
> +	dw_pcie_writel_dbi(dw, PCIE_PORT_LANE_SKEW, val);
> +
> +	val = readl(rcar->base + PCIEPWRMNGCTRL);
> +	val |= APP_CLK_REQ_N | APP_CLK_PM_EN;
> +	writel(val, rcar->base + PCIEPWRMNGCTRL);
> +}
> +
> +static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
> +					       u32 offset, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = readl(rcar->phy_base + offset);
> +	tmp &= ~mask;
> +	tmp |= val;
> +	writel(tmp, rcar->phy_base + offset);
> +}
> +
> +/*
> + * SoC datasheet suggests checking port logic register bits during firmware
> + * write. If read returns non-zero value, then this function returns -EAGAIN
> + * indicating that the write needs to be done again. If read returns zero,
> + * then return 0 to indicate success.
> + */
> +static int rcar_gen4_pcie_reg_test_bit(struct rcar_gen4_pcie *rcar,
> +				       u32 offset, u32 mask)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +
> +	if (dw_pcie_readl_dbi(dw, offset) & mask)
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
> +{
> +	/* The check_addr values are magical numbers in the datasheet */
> +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
> +	struct dw_pcie *dw = &rcar->dw;
> +	const struct firmware *fw;
> +	unsigned int i, timeout;
> +	u32 data;
> +	int ret;
> +
> +	ret = request_firmware(&fw, RCAR_GEN4_PCIE_FIRMWARE_NAME, dw->dev);
> +	if (ret) {
> +		dev_err(dw->dev, "Failed to load firmware (%s): %d\n",
> +			RCAR_GEN4_PCIE_FIRMWARE_NAME, ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < (fw->size / 2); i++) {
> +		data = fw->data[(i * 2) + 1] << 8 | fw->data[i * 2];
> +		timeout = 100;
> +		do {
> +			dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR + i);
> +			dw_pcie_writel_dbi(dw, PRTLGC90, data);
> +			if (!rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30)))
> +				break;
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +		} while (1);
> +	}
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
> +
> +	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
> +		timeout = 100;
> +		do {
> +			dw_pcie_writel_dbi(dw, PRTLGC89, check_addr[i]);
> +			ret = rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30));
> +			ret |= rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC90, BIT(0));
> +			if (!ret)
> +				break;
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +		} while (1);
> +	}
> +
> +exit:
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 val;
> +	int ret;
> +
> +	if (!enable) {
> +		val = readl(rcar->base + PCIERSTCTRL1);
> +		val &= ~APP_LTSSM_ENABLE;
> +		writel(val, rcar->base + PCIERSTCTRL1);
> +
> +		return 0;
> +	}
> +
> +	val = dw_pcie_readl_dbi(dw, PCIE_PORT_FORCE);
> +	val |= PORT_FORCE_DO_DESKEW_FOR_SRIS;
> +	dw_pcie_writel_dbi(dw, PCIE_PORT_FORCE, val);
> +
> +	val = readl(rcar->base + PCIEMSR0);
> +	val |= APP_SRIS_MODE;
> +	writel(val, rcar->base + PCIEMSR0);
> +
> +	/*
> +	 * The R-Car Gen4 datasheet doesn't describe the PHY registers' name.
> +	 * But, the initialization procedure describes these offsets. So,
> +	 * this driver has magical offset numbers.
> +	 */
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(28), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(20), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(12), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(4), 0);
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(23, 22), BIT(22));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(18, 16), GENMASK(17, 16));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(7, 6), BIT(6));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(2, 0), GENMASK(11, 0));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x1d4, GENMASK(16, 15), GENMASK(16, 15));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x514, BIT(26), BIT(26));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(16), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(19), BIT(19));
> +
> +	val = readl(rcar->base + PCIERSTCTRL1);
> +	val &= ~APP_HOLD_PHY_RST;
> +	writel(val, rcar->base + PCIERSTCTRL1);
> +
> +	ret = readl_poll_timeout(rcar->phy_base + 0x0f8, val, !(val & BIT(18)), 100, 10000);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = rcar_gen4_pcie_download_phy_firmware(rcar);
> +	if (ret)
> +		return ret;
> +
> +	val = readl(rcar->base + PCIERSTCTRL1);
> +	val |= APP_LTSSM_ENABLE;
> +	writel(val, rcar->base + PCIERSTCTRL1);
> +
> +	return 0;
> +}
> +
>  static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
>  	.ltssm_control = r8a779f0_pcie_ltssm_control,
>  	.mode = DW_PCIE_RC_TYPE,
> @@ -539,10 +740,14 @@ static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
>  };
>  
>  static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
> +	.additional_common_init = rcar_gen4_pcie_additional_common_init,
> +	.ltssm_control = rcar_gen4_pcie_ltssm_control,
>  	.mode = DW_PCIE_RC_TYPE,
>  };
>  
>  static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
> +	.additional_common_init = rcar_gen4_pcie_additional_common_init,
> +	.ltssm_control = rcar_gen4_pcie_ltssm_control,
>  	.mode = DW_PCIE_EP_TYPE,
>  };
>  
> -- 
> 2.25.1
> 

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

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

[Yoshihiro Shimoda]

Hello Manivannan,

Thank you for your review!

> From: Manivannan Sadhasivam, Sent: Tuesday, June 18, 2024 4:09 PM
> 
> On Tue, Jun 11, 2024 at 09:50:56PM +0900, Yoshihiro Shimoda wrote:
<snip>
> > @@ -221,6 +257,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);
> > +
> 
> I failed to spot this in earlier reviews. Since this 'phy' region is only
> applicable for r8a779g0, wouldn't this fail on other platforms?

This will not fail on other platforms because the dtsi file of the other platform (r8a779f0.dtsi)
already has the 'phy' region in the first commit which I added PCIe nodes:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/boot/dts/renesas/r8a779f0.dtsi?h=v6.10-rc4&id=183a709d3719e5c9919a6f12c86c0a3e088b712d

Best regards,
Yoshihiro Shimoda

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

[Manivannan Sadhasivam]

On Tue, Jun 18, 2024 at 08:10:43AM +0000, Yoshihiro Shimoda wrote:
> Hello Manivannan,
> 
> Thank you for your review!
> 
> > From: Manivannan Sadhasivam, Sent: Tuesday, June 18, 2024 4:09 PM
> > 
> > On Tue, Jun 11, 2024 at 09:50:56PM +0900, Yoshihiro Shimoda wrote:
> <snip>
> > > @@ -221,6 +257,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);
> > > +
> > 
> > I failed to spot this in earlier reviews. Since this 'phy' region is only
> > applicable for r8a779g0, wouldn't this fail on other platforms?
> 
> This will not fail on other platforms because the dtsi file of the other platform (r8a779f0.dtsi)
> already has the 'phy' region in the first commit which I added PCIe nodes:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/boot/dts/renesas/r8a779f0.dtsi?h=v6.10-rc4&id=183a709d3719e5c9919a6f12c86c0a3e088b712d
> 

Ah okay. I didn't check the dts/binding. This is fine.

- Mani

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

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

[Manivannan Sadhasivam]

On Tue, Jun 11, 2024 at 09:50:56PM +0900, Yoshihiro Shimoda wrote:
> Add support for r8a779g0 (R-Car V4H).
> 
> This driver previously supported r8a779f0 (R-Car S4-8). PCIe features
> of both r8a779f0 and r8a779g0 are almost all the same. For example:
>  - PCI Express Base Specification Revision 4.0
>  - Root complex mode and endpoint mode are supported
> However, r8a779g0 requires specific firmware downloading, to
> initialize the PHY. Otherwise, the PCIe controller cannot work.
> 
> The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> the datasheet is a text file. But, Renesas is not able to distribute
> the firmware freely. So, we require converting the text file
> to a binary before the driver runs by using the following script:
> 
>  $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
>    { print substr($2,5,2) substr($2,3,2) }' \
>    104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
>    rcar_gen4_pcie.bin
>  $ sha1sum rcar_gen4_pcie.bin
>    1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

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

- Mani

> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 207 +++++++++++++++++++-
>  1 file changed, 206 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index dac78388975d..c67097e718d3 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,48 @@
>  #define PCIEDMAINTSTSEN		0x0314
>  #define PCIEDMAINTSTSEN_INIT	GENMASK(15, 0)
>  
> +/* Port Logic Registers 89 */
> +#define PRTLGC89		0x0b70
> +
> +/* Port Logic Registers 90 */
> +#define PRTLGC90		0x0b74
> +
>  /* PCIe Reset Control Register 1 */
>  #define PCIERSTCTRL1		0x0014
>  #define APP_HOLD_PHY_RST	BIT(16)
>  #define APP_LTSSM_ENABLE	BIT(0)
>  
> +/* PCIe Power Management Control */
> +#define PCIEPWRMNGCTRL		0x0070
> +#define APP_CLK_REQ_N		BIT(11)
> +#define APP_CLK_PM_EN		BIT(10)
> +
>  #define RCAR_NUM_SPEED_CHANGE_RETRIES	10
>  #define RCAR_MAX_LINK_SPEED		4
>  
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
>  #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
>  
> +/*
> + * The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> + * the datasheet is a text file. But, Renesas is not able to distribute
> + * the firmware freely. So, we require converting the text file
> + * to a binary before the driver runs by using the following script:
> + *
> + * $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
> + *      { print substr($2,5,2) substr($2,3,2) }' \
> + *      104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
> + *      rcar_gen4_pcie.bin
> + *    $ sha1sum rcar_gen4_pcie.bin
> + *      1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> + */
> +#define RCAR_GEN4_PCIE_FIRMWARE_NAME		"rcar_gen4_pcie.bin"
> +#define RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR	0xc000
> +MODULE_FIRMWARE(RCAR_GEN4_PCIE_FIRMWARE_NAME);
> +
>  struct rcar_gen4_pcie;
>  struct rcar_gen4_pcie_drvdata {
> +	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
>  	int (*ltssm_control)(struct rcar_gen4_pcie *rcar, bool enable);
>  	enum dw_pcie_device_mode mode;
>  };
> @@ -57,6 +89,7 @@ struct rcar_gen4_pcie_drvdata {
>  struct rcar_gen4_pcie {
>  	struct dw_pcie dw;
>  	void __iomem *base;
> +	void __iomem *phy_base;
>  	struct platform_device *pdev;
>  	const struct rcar_gen4_pcie_drvdata *drvdata;
>  };
> @@ -180,6 +213,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
>  	if (ret)
>  		goto err_unprepare;
>  
> +	if (rcar->drvdata->additional_common_init)
> +		rcar->drvdata->additional_common_init(rcar);
> +
>  	return 0;
>  
>  err_unprepare:
> @@ -221,6 +257,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");
>  
> @@ -528,6 +568,167 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
>  	return 0;
>  }
>  
> +static void rcar_gen4_pcie_additional_common_init(struct rcar_gen4_pcie *rcar)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 val;
> +
> +	val = dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW);
> +	val &= ~PORT_LANE_SKEW_INSERT_MASK;
> +	if (dw->num_lanes < 4)
> +		val |= BIT(6);
> +	dw_pcie_writel_dbi(dw, PCIE_PORT_LANE_SKEW, val);
> +
> +	val = readl(rcar->base + PCIEPWRMNGCTRL);
> +	val |= APP_CLK_REQ_N | APP_CLK_PM_EN;
> +	writel(val, rcar->base + PCIEPWRMNGCTRL);
> +}
> +
> +static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
> +					       u32 offset, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = readl(rcar->phy_base + offset);
> +	tmp &= ~mask;
> +	tmp |= val;
> +	writel(tmp, rcar->phy_base + offset);
> +}
> +
> +/*
> + * SoC datasheet suggests checking port logic register bits during firmware
> + * write. If read returns non-zero value, then this function returns -EAGAIN
> + * indicating that the write needs to be done again. If read returns zero,
> + * then return 0 to indicate success.
> + */
> +static int rcar_gen4_pcie_reg_test_bit(struct rcar_gen4_pcie *rcar,
> +				       u32 offset, u32 mask)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +
> +	if (dw_pcie_readl_dbi(dw, offset) & mask)
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +static int rcar_gen4_pcie_download_phy_firmware(struct rcar_gen4_pcie *rcar)
> +{
> +	/* The check_addr values are magical numbers in the datasheet */
> +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
> +	struct dw_pcie *dw = &rcar->dw;
> +	const struct firmware *fw;
> +	unsigned int i, timeout;
> +	u32 data;
> +	int ret;
> +
> +	ret = request_firmware(&fw, RCAR_GEN4_PCIE_FIRMWARE_NAME, dw->dev);
> +	if (ret) {
> +		dev_err(dw->dev, "Failed to load firmware (%s): %d\n",
> +			RCAR_GEN4_PCIE_FIRMWARE_NAME, ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < (fw->size / 2); i++) {
> +		data = fw->data[(i * 2) + 1] << 8 | fw->data[i * 2];
> +		timeout = 100;
> +		do {
> +			dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR + i);
> +			dw_pcie_writel_dbi(dw, PRTLGC90, data);
> +			if (!rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30)))
> +				break;
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +		} while (1);
> +	}
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
> +
> +	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
> +		timeout = 100;
> +		do {
> +			dw_pcie_writel_dbi(dw, PRTLGC89, check_addr[i]);
> +			ret = rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC89, BIT(30));
> +			ret |= rcar_gen4_pcie_reg_test_bit(rcar, PRTLGC90, BIT(0));
> +			if (!ret)
> +				break;
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +		} while (1);
> +	}
> +
> +exit:
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 val;
> +	int ret;
> +
> +	if (!enable) {
> +		val = readl(rcar->base + PCIERSTCTRL1);
> +		val &= ~APP_LTSSM_ENABLE;
> +		writel(val, rcar->base + PCIERSTCTRL1);
> +
> +		return 0;
> +	}
> +
> +	val = dw_pcie_readl_dbi(dw, PCIE_PORT_FORCE);
> +	val |= PORT_FORCE_DO_DESKEW_FOR_SRIS;
> +	dw_pcie_writel_dbi(dw, PCIE_PORT_FORCE, val);
> +
> +	val = readl(rcar->base + PCIEMSR0);
> +	val |= APP_SRIS_MODE;
> +	writel(val, rcar->base + PCIEMSR0);
> +
> +	/*
> +	 * The R-Car Gen4 datasheet doesn't describe the PHY registers' name.
> +	 * But, the initialization procedure describes these offsets. So,
> +	 * this driver has magical offset numbers.
> +	 */
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(28), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(20), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(12), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x700, BIT(4), 0);
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(23, 22), BIT(22));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(18, 16), GENMASK(17, 16));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(7, 6), BIT(6));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x148, GENMASK(2, 0), GENMASK(11, 0));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x1d4, GENMASK(16, 15), GENMASK(16, 15));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x514, BIT(26), BIT(26));
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(16), 0);
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(19), BIT(19));
> +
> +	val = readl(rcar->base + PCIERSTCTRL1);
> +	val &= ~APP_HOLD_PHY_RST;
> +	writel(val, rcar->base + PCIERSTCTRL1);
> +
> +	ret = readl_poll_timeout(rcar->phy_base + 0x0f8, val, !(val & BIT(18)), 100, 10000);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = rcar_gen4_pcie_download_phy_firmware(rcar);
> +	if (ret)
> +		return ret;
> +
> +	val = readl(rcar->base + PCIERSTCTRL1);
> +	val |= APP_LTSSM_ENABLE;
> +	writel(val, rcar->base + PCIERSTCTRL1);
> +
> +	return 0;
> +}
> +
>  static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie = {
>  	.ltssm_control = r8a779f0_pcie_ltssm_control,
>  	.mode = DW_PCIE_RC_TYPE,
> @@ -539,10 +740,14 @@ static struct rcar_gen4_pcie_drvdata drvdata_r8a779f0_pcie_ep = {
>  };
>  
>  static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = {
> +	.additional_common_init = rcar_gen4_pcie_additional_common_init,
> +	.ltssm_control = rcar_gen4_pcie_ltssm_control,
>  	.mode = DW_PCIE_RC_TYPE,
>  };
>  
>  static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = {
> +	.additional_common_init = rcar_gen4_pcie_additional_common_init,
> +	.ltssm_control = rcar_gen4_pcie_ltssm_control,
>  	.mode = DW_PCIE_EP_TYPE,
>  };
>  
> -- 
> 2.25.1
> 
> 

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

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

[Krzysztof Wilczyński]

Hello,

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

Applied to endpoint, thank you!

[1/1] misc: pci_endpoint_test: Document a policy about adding pci_device_id
      https://git.kernel.org/pci/pci/c/1947ff399bb7

	Krzysztof

Re: [PATCH v9 0/5] 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.

Applied to controller/rcar-gen4, thank you!

[01/04] PCI: dwc: Add PCIE_PORT_{FORCE,LANE_SKEW} macros
        https://git.kernel.org/pci/pci/c/544a18c936f9

[02/04] PCI: rcar-gen4: Add struct rcar_gen4_pcie_drvdata
        https://git.kernel.org/pci/pci/c/ac1d89f8dcc3

[03/04] PCI: rcar-gen4: Add .ltssm_control() for other SoC support
        https://git.kernel.org/pci/pci/c/2c49151b3fff

[04/04] PCI: rcar-gen4: Add support for R-Car V4H
        https://git.kernel.org/pci/pci/c/60ad25bcac1d

	Krzysztof

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

[Krzysztof Wilczyński]

Hello,

[...]
> +/*
> + * The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> + * the datasheet is a text file. But, Renesas is not able to distribute
> + * the firmware freely. So, we require converting the text file
> + * to a binary before the driver runs by using the following script:
> + *
> + * $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
> + *      { print substr($2,5,2) substr($2,3,2) }' \
> + *      104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
> + *      rcar_gen4_pcie.bin
> + *    $ sha1sum rcar_gen4_pcie.bin
> + *      1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> + */

I moved this comment to the top of the file, since it serves as more of
a documentation bit for the controller itself, and is not specific to the
function it was attached to, strictly speaking.

That said, I wonder if we should include this at all?  The file name might
and most likely will eventually change, as the datasheet gets updated to
include new revisions and erratas, etc.

So, I wonder if this is simply better to be included in the datasheet
itself, or some product guide and such, where Renesas can keep this up to
date and the users appraised of what they need to do to convert the
firmware correctly.

Users who do not have the datasheet and are not licensed wouldn't care.

	Krzysztof

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

[Krzysztof Wilczyński]

Hello,

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

This quite a sad state of affairs, and usually would I oppose including
drivers that rely on closed proprietary firmware blobs to operate.  That
said, Renesas is not really setting any precedent here, so we will live
with this.

Shimoda-san, if you can, please pass the feedback to your bosses that this
decision to keep the firmware closed is rather unfortunate.

	Krzysztof

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

[Bjorn Helgaas]

On Sun, Jun 30, 2024 at 05:06:50AM +0900, Krzysztof Wilczyński wrote:
> Hello,
> 
> [...]
> > About the firmware binary, please refer to the following patch
> > descirption:
> >   PCI: rcar-gen4: Add support for r8a779g0
> 
> This quite a sad state of affairs, and usually would I oppose including
> drivers that rely on closed proprietary firmware blobs to operate.  That
> said, Renesas is not really setting any precedent here, so we will live
> with this.

What are the existing similar situations?  Just for curiosity, I'd
like to know what precedent we are relying on here.

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

[Yoshihiro Shimoda]

Hello Krzysztof-san,

> From: Krzysztof Wilczyński, Sent: Sunday, June 30, 2024 4:56 AM
> 
> Hello,
> 
> [...]
> > +/*
> > + * The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> > + * the datasheet is a text file. But, Renesas is not able to distribute
> > + * the firmware freely. So, we require converting the text file
> > + * to a binary before the driver runs by using the following script:
> > + *
> > + * $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
> > + *      { print substr($2,5,2) substr($2,3,2) }' \
> > + *      104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
> > + *      rcar_gen4_pcie.bin
> > + *    $ sha1sum rcar_gen4_pcie.bin
> > + *      1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> > + */
> 
> I moved this comment to the top of the file, since it serves as more of
> a documentation bit for the controller itself, and is not specific to the
> function it was attached to, strictly speaking.

I got it. Thank you.

> That said, I wonder if we should include this at all?  The file name might
> and most likely will eventually change, as the datasheet gets updated to
> include new revisions and erratas, etc.
> 
> So, I wonder if this is simply better to be included in the datasheet
> itself, or some product guide and such, where Renesas can keep this up to
> date and the users appraised of what they need to do to convert the
> firmware correctly.

I understood it. I'll consider it somehow...

> Users who do not have the datasheet and are not licensed wouldn't care.

Yes. If so, it's convenient.

Best regards,
Yoshihiro Shimoda

> 	Krzysztof

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

[Yoshihiro Shimoda]

Hello Krzysztof-san,

> From: Krzysztof Wilczyński, Sent: Sunday, June 30, 2024 5:07 AM
> 
> Hello,
> 
> [...]
> > About the firmware binary, please refer to the following patch
> > descirption:
> >   PCI: rcar-gen4: Add support for r8a779g0
> 
> This quite a sad state of affairs, and usually would I oppose including
> drivers that rely on closed proprietary firmware blobs to operate.  That
> said, Renesas is not really setting any precedent here, so we will live
> with this.
> 
> Shimoda-san, if you can, please pass the feedback to your bosses that this
> decision to keep the firmware closed is rather unfortunate.

I got it. I'll try it.

Best regards,
Yoshihiro Shimoda


> 	Krzysztof

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

[Yoshihiro Shimoda]

Hello Bjorn,

> From: Bjorn Helgaas, Sent: Sunday, June 30, 2024 5:46 AM
> 
> On Sun, Jun 30, 2024 at 05:06:50AM +0900, Krzysztof Wilczyński wrote:
> > Hello,
> >
> > [...]
> > > About the firmware binary, please refer to the following patch
> > > descirption:
> > >   PCI: rcar-gen4: Add support for r8a779g0
> >
> > This quite a sad state of affairs, and usually would I oppose including
> > drivers that rely on closed proprietary firmware blobs to operate.  That
> > said, Renesas is not really setting any precedent here, so we will live
> > with this.
> 
> What are the existing similar situations?  Just for curiosity, I'd
> like to know what precedent we are relying on here.

Wolfram mentioned it on previous email thread [1].

[1]
https://lore.kernel.org/linux-pci/53sfkav45djcaapqkzsps6ofsinf5lnxbhrjvgsevt3w6qcms6@e2vptwrj645q/

Best regards,
Yoshihiro Shimoda

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

[Krzysztof Wilczyński]

> > > +/*
> > > + * The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> > > + * the datasheet is a text file. But, Renesas is not able to distribute
> > > + * the firmware freely. So, we require converting the text file
> > > + * to a binary before the driver runs by using the following script:
> > > + *
> > > + * $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
> > > + *      { print substr($2,5,2) substr($2,3,2) }' \
> > > + *      104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
> > > + *      rcar_gen4_pcie.bin
> > > + *    $ sha1sum rcar_gen4_pcie.bin
> > > + *      1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> > > + */
> > 
> > I moved this comment to the top of the file, since it serves as more of
> > a documentation bit for the controller itself, and is not specific to the
> > function it was attached to, strictly speaking.
> 
> I got it. Thank you.

I decided to drop this comment.  See below.

> > That said, I wonder if we should include this at all?  The file name might
> > and most likely will eventually change, as the datasheet gets updated to
> > include new revisions and erratas, etc.
> > 
> > So, I wonder if this is simply better to be included in the datasheet
> > itself, or some product guide and such, where Renesas can keep this up to
> > date and the users appraised of what they need to do to convert the
> > firmware correctly.
> 
> I understood it. I'll consider it somehow...

After thinking about this a little more, I believe that it might be better
to follow what some other drivers have done, and simply add this guide as
a text file under a dedicated ../Documentation/admin-guide directory.

We can then mention, or not, at the top of the file that there is a guide
available at this location for people to read on how to obtain and prepare
the firmware.

Thoughts?

	Krzysztof

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

[Krzysztof Wilczyński]

Hello,

[...]
> > > [...]
> > > > About the firmware binary, please refer to the following patch
> > > > descirption:
> > > >   PCI: rcar-gen4: Add support for r8a779g0
> > >
> > > This quite a sad state of affairs, and usually would I oppose including
> > > drivers that rely on closed proprietary firmware blobs to operate.  That
> > > said, Renesas is not really setting any precedent here, so we will live
> > > with this.
> > 
> > What are the existing similar situations?  Just for curiosity, I'd
> > like to know what precedent we are relying on here.
> 
> Wolfram mentioned it on previous email thread [1].
> 
> [1]
> https://lore.kernel.org/linux-pci/53sfkav45djcaapqkzsps6ofsinf5lnxbhrjvgsevt3w6qcms6@e2vptwrj645q/

Another example could be the Marvell's (formerly Aquantia) "Atlantic"
network cards family, which requires a custom firmware blob that wasn't
readily or freely distributed.  The firmware files were never added to
the linux-firmware repository.

... unless things have changes since I looked some time ago.

	Krzysztof

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

[Yoshihiro Shimoda]

Hello Krzysztof-san,

> From: Krzysztof Wilczyński, Sent: Tuesday, July 2, 2024 5:44 AM
> > > > +/*
> > > > + * The attached firmware file "104_PCIe_fw_addr_data_ver1.05.txt" in
> > > > + * the datasheet is a text file. But, Renesas is not able to distribute
> > > > + * the firmware freely. So, we require converting the text file
> > > > + * to a binary before the driver runs by using the following script:
> > > > + *
> > > > + * $ awk '/^\s*0x[0-9A-Fa-f]{4}\s+0x[0-9A-Fa-f]{4}/ \
> > > > + *      { print substr($2,5,2) substr($2,3,2) }' \
> > > > + *      104_PCIe_fw_addr_data_ver1.05.txt | xxd -p -r > \
> > > > + *      rcar_gen4_pcie.bin
> > > > + *    $ sha1sum rcar_gen4_pcie.bin
> > > > + *      1d0bd4b189b4eb009f5d564b1f93a79112994945  rcar_gen4_pcie.bin
> > > > + */
> > >
> > > I moved this comment to the top of the file, since it serves as more of
> > > a documentation bit for the controller itself, and is not specific to the
> > > function it was attached to, strictly speaking.
> >
> > I got it. Thank you.
> 
> I decided to drop this comment.  See below.

I got it.

> > > That said, I wonder if we should include this at all?  The file name might
> > > and most likely will eventually change, as the datasheet gets updated to
> > > include new revisions and erratas, etc.
> > >
> > > So, I wonder if this is simply better to be included in the datasheet
> > > itself, or some product guide and such, where Renesas can keep this up to
> > > date and the users appraised of what they need to do to convert the
> > > firmware correctly.
> >
> > I understood it. I'll consider it somehow...
> 
> After thinking about this a little more, I believe that it might be better
> to follow what some other drivers have done, and simply add this guide as
> a text file under a dedicated ../Documentation/admin-guide directory.
> 
> We can then mention, or not, at the top of the file that there is a guide
> available at this location for people to read on how to obtain and prepare
> the firmware.
> 
> Thoughts?

Thank you for your comments. I agreed. So, may I send such a patch? Or, should
I send whole patches as v10?

Best regards,
Yoshihiro Shimoda

> 	Krzysztof

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

[Bjorn Helgaas]

On Tue, Jun 11, 2024 at 09:50:56PM +0900, Yoshihiro Shimoda wrote:
> Add support for r8a779g0 (R-Car V4H).
> 
> This driver previously supported r8a779f0 (R-Car S4-8). PCIe features
> of both r8a779f0 and r8a779g0 are almost all the same. For example:
>  - PCI Express Base Specification Revision 4.0

This item is too general to be really useful.  I don't think it means
the device supports every feature described in the r4.0 spec.  I
suspect it means it supports the 16.0 GT/s transfer rate that was
added in r4.0?  *That* would be useful information here.  Perhaps also
the number of lanes it supports.

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

[Yoshihiro Shimoda]

Hello Bjorn,

> From: Bjorn Helgaas, Sent: Wednesday, July 3, 2024 2:14 AM
> 
> On Tue, Jun 11, 2024 at 09:50:56PM +0900, Yoshihiro Shimoda wrote:
> > Add support for r8a779g0 (R-Car V4H).
> >
> > This driver previously supported r8a779f0 (R-Car S4-8). PCIe features
> > of both r8a779f0 and r8a779g0 are almost all the same. For example:
> >  - PCI Express Base Specification Revision 4.0
> 
> This item is too general to be really useful.  I don't think it means
> the device supports every feature described in the r4.0 spec.  I
> suspect it means it supports the 16.0 GT/s transfer rate that was
> added in r4.0?  *That* would be useful information here.  Perhaps also
> the number of lanes it supports.

You're correct. The device doesn't support every feature described in the r4.0 spec.
The device supports the 16.0 GT/s transfer rate. Also, it supports up to 4 lanes.

Best regards,
Yoshihiro Shimoda

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

[Krzysztof Wilczyński]

[...]
> > After thinking about this a little more, I believe that it might be better
> > to follow what some other drivers have done, and simply add this guide as
> > a text file under a dedicated ../Documentation/admin-guide directory.
> > 
> > We can then mention, or not, at the top of the file that there is a guide
> > available at this location for people to read on how to obtain and prepare
> > the firmware.
> > 
> > Thoughts?
> 
> Thank you for your comments. I agreed. So, may I send such a patch? Or, should
> I send whole patches as v10?

No need to send a new series.  A small patch that I can pick would be fine.

Thank you!

	Krzysztof

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

[Yoshihiro Shimoda]

Hello Krzysztof-san,

> From: Krzysztof Wilczyński, Sent: Wednesday, July 3, 2024 5:32 PM
> [...]
> > > After thinking about this a little more, I believe that it might be better
> > > to follow what some other drivers have done, and simply add this guide as
> > > a text file under a dedicated ../Documentation/admin-guide directory.
> > >
> > > We can then mention, or not, at the top of the file that there is a guide
> > > available at this location for people to read on how to obtain and prepare
> > > the firmware.
> > >
> > > Thoughts?
> >
> > Thank you for your comments. I agreed. So, may I send such a patch? Or, should
> > I send whole patches as v10?
> 
> No need to send a new series.  A small patch that I can pick would be fine.

I got it. I'll send such a small patch soon.

Best regards,
Yoshihiro Shimoda

> Thank you!
> 
> 	Krzysztof