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

[PATCH v6 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. 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 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...

Changes from v3:
https://lore.kernel.org/linux-pci/20240401023942.134704-1-yoshihiro.shimoda.uh@renesas.com/
- Modify the code to use "do .. while" instead of goto in patch 6/7.

Changes from v2:
https://lore.kernel.org/linux-pci/20240326024540.2336155-1-yoshihiro.shimoda.uh@renesas.com/
- Add a new patch which just add a platdata in patch 4/7.
- Modify the subjects in patch [56]/7.
- Modify the description and code about Bjorn's comment in patch [56]/7.
- Add missing MODULE_FIRMWARE(9 in patch 6/7.
- Document a policy aboud adding pci_device_id instead of adding r8a779g0's id
  in patch 7/7.

Changes from v1:
https://lore.kernel.org/linux-pci/20240229120719.2553638-1-yoshihiro.shimoda.uh@renesas.com/
- Based on v6.9-rc1.
- Add Acked-by and/or Reviewed-by in patch [126/6].

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_platdata
  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 v6 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 v6 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 v6 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>
---
 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

Re: [PATCH v6 3/7] PCI: dwc: Add PCIE_PORT_{FORCE,LANE_SKEW} macros

[Manivannan Sadhasivam]

On Wed, Apr 10, 2024 at 09:48:28AM +0900, Yoshihiro Shimoda wrote:
> 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>

- Mani

> ---
>  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 v6 4/7] PCI: rcar-gen4: Add rcar_gen4_pcie_platdata

[Yoshihiro Shimoda]

This driver supports r8a779f0 now. In the future, add support for
r8a779g0 and r8a779h0. To support these new SoCs, need other
initializing settings. So, at first, add rcar_gen4_pcie_platdata
and have a member with mode. No behavior changes.

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..da2821d6efce 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_platdata {
+	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_platdata *platdata;
 };
 #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->platdata->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->platdata->mode == DW_PCIE_RC_TYPE) {
 		val |= DEVICE_TYPE_RC;
-	} else if (rcar->mode == DW_PCIE_EP_TYPE) {
+	} else if (rcar->platdata->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->platdata = of_device_get_match_data(&rcar->pdev->dev);
 
-	switch (rcar->mode) {
+	switch (rcar->platdata->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->platdata->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_platdata platdata_rcar_gen4_pcie = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static struct rcar_gen4_pcie_platdata platdata_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 = &platdata_rcar_gen4_pcie,
 	},
 	{
 		.compatible = "renesas,rcar-gen4-pcie-ep",
-		.data = (void *)DW_PCIE_EP_TYPE,
+		.data = &platdata_rcar_gen4_pcie_ep,
 	},
 	{},
 };
-- 
2.25.1

Re: [PATCH v6 4/7] PCI: rcar-gen4: Add rcar_gen4_pcie_platdata

[Manivannan Sadhasivam]

On Wed, Apr 10, 2024 at 09:48:29AM +0900, Yoshihiro Shimoda wrote:
> This driver supports r8a779f0 now. In the future, add support for
> r8a779g0 and r8a779h0. To support these new SoCs, need other
> initializing settings. So, at first, add rcar_gen4_pcie_platdata
> and have a member with mode. No behavior changes.
> 

How about,

"In order 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..da2821d6efce 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_platdata {

Common naming convention is '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_platdata *platdata;
>  };
>  #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->platdata->mode == DW_PCIE_RC_TYPE)

I'd recommend checking for the existence of the drvdata first. But if you are
sure that it will be present for all SoCs, then it can be left.

- Mani

>  		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->platdata->mode == DW_PCIE_RC_TYPE) {
>  		val |= DEVICE_TYPE_RC;
> -	} else if (rcar->mode == DW_PCIE_EP_TYPE) {
> +	} else if (rcar->platdata->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->platdata = of_device_get_match_data(&rcar->pdev->dev);
>  
> -	switch (rcar->mode) {
> +	switch (rcar->platdata->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->platdata->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_platdata platdata_rcar_gen4_pcie = {
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static struct rcar_gen4_pcie_platdata platdata_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 = &platdata_rcar_gen4_pcie,
>  	},
>  	{
>  		.compatible = "renesas,rcar-gen4-pcie-ep",
> -		.data = (void *)DW_PCIE_EP_TYPE,
> +		.data = &platdata_rcar_gen4_pcie_ep,
>  	},
>  	{},
>  };
> -- 
> 2.25.1
> 
> 

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

RE: [PATCH v6 4/7] PCI: rcar-gen4: Add rcar_gen4_pcie_platdata

[Yoshihiro Shimoda]

Hello Manivannan,

> From: Manivannan Sadhasivam, Sent: Thursday, April 11, 2024 2:17 AM
> 
> On Wed, Apr 10, 2024 at 09:48:29AM +0900, Yoshihiro Shimoda wrote:
> > This driver supports r8a779f0 now. In the future, add support for
> > r8a779g0 and r8a779h0. To support these new SoCs, need other
> > initializing settings. So, at first, add rcar_gen4_pcie_platdata
> > and have a member with mode. No behavior changes.
> >
> 
> How about,
> 
> "In order 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."

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

> > 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..da2821d6efce 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_platdata {
> 
> Common naming convention is 'drvdata'.

I got it. I'll rename it.

> > +	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_platdata *platdata;
> >  };
> >  #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->platdata->mode == DW_PCIE_RC_TYPE)
> 
> I'd recommend checking for the existence of the drvdata first. But if you are
> sure that it will be present for all SoCs, then it can be left.

Since it will be present for all SoC, I will not change the code.

Best regards,
Yoshihiro Shimoda

> - Mani
> 
> >  		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->platdata->mode == DW_PCIE_RC_TYPE) {
> >  		val |= DEVICE_TYPE_RC;
> > -	} else if (rcar->mode == DW_PCIE_EP_TYPE) {
> > +	} else if (rcar->platdata->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->platdata = of_device_get_match_data(&rcar->pdev->dev);
> >
> > -	switch (rcar->mode) {
> > +	switch (rcar->platdata->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->platdata->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_platdata platdata_rcar_gen4_pcie = {
> > +	.mode = DW_PCIE_RC_TYPE,
> > +};
> > +
> > +static struct rcar_gen4_pcie_platdata platdata_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 = &platdata_rcar_gen4_pcie,
> >  	},
> >  	{
> >  		.compatible = "renesas,rcar-gen4-pcie-ep",
> > -		.data = (void *)DW_PCIE_EP_TYPE,
> > +		.data = &platdata_rcar_gen4_pcie_ep,
> >  	},
> >  	{},
> >  };
> > --
> > 2.25.1
> >
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

[PATCH v6 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. No behavior changes.

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"

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 da2821d6efce..47ec394885f5 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_platdata {
+	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->platdata->ltssm_enable) {
+		ret = rcar->platdata->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_platdata platdata_r8a779f0_pcie = {
+	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
+	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie = {
 	.mode = DW_PCIE_RC_TYPE,
 };
@@ -513,6 +536,14 @@ static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie_ep = {
 };
 
 static const struct of_device_id rcar_gen4_pcie_of_match[] = {
+	{
+		.compatible = "renesas,r8a779f0-pcie",
+		.data = &platdata_r8a779f0_pcie,
+	},
+	{
+		.compatible = "renesas,r8a779f0-pcie-ep",
+		.data = &platdata_r8a779f0_pcie_ep,
+	},
 	{
 		.compatible = "renesas,rcar-gen4-pcie",
 		.data = &platdata_rcar_gen4_pcie,
-- 
2.25.1

Re: [PATCH v6 5/7] PCI: rcar-gen4: Add .ltssm_enable() for other SoC support

[Manivannan Sadhasivam]

On Wed, Apr 10, 2024 at 09:48:30AM +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. No behavior changes.
> 
> 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"
> 

If r8a779f0 SoC can work with the existing 'renesas,rcar-gen4-pcie' and
'renesas,rcar-gen4-pcie-ep' compatibles, then you should just leave it as it is
and add a new compatible with dedicated callbacks for only r8a779g0 and
r8a779h0.

- Mani

> 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 da2821d6efce..47ec394885f5 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_platdata {
> +	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->platdata->ltssm_enable) {
> +		ret = rcar->platdata->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_platdata platdata_r8a779f0_pcie = {
> +	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
> +	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
>  static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie = {
>  	.mode = DW_PCIE_RC_TYPE,
>  };
> @@ -513,6 +536,14 @@ static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie_ep = {
>  };
>  
>  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
> +	{
> +		.compatible = "renesas,r8a779f0-pcie",
> +		.data = &platdata_r8a779f0_pcie,
> +	},
> +	{
> +		.compatible = "renesas,r8a779f0-pcie-ep",
> +		.data = &platdata_r8a779f0_pcie_ep,
> +	},
>  	{
>  		.compatible = "renesas,rcar-gen4-pcie",
>  		.data = &platdata_rcar_gen4_pcie,
> -- 
> 2.25.1
> 
> 

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

RE: [PATCH v6 5/7] PCI: rcar-gen4: Add .ltssm_enable() for other SoC support

[Yoshihiro Shimoda]

Hello Manivannan,

> From: Manivannan Sadhasivam, Sent: Thursday, April 11, 2024 2:24 AM
> 
> On Wed, Apr 10, 2024 at 09:48:30AM +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. No behavior changes.
> >
> > 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"
> >
> 
> If r8a779f0 SoC can work with the existing 'renesas,rcar-gen4-pcie' and
> 'renesas,rcar-gen4-pcie-ep' compatibles, then you should just leave it as it is
> and add a new compatible with dedicated callbacks for only r8a779g0 and
> r8a779h0.

My implementation will have 4 entries. And, it can support r8a779[fgh]0:
---
static const struct of_device_id rcar_gen4_pcie_of_match[] = {
        {
                .compatible = "renesas,r8a779f0-pcie",
                .data = &platdata_r8a779f0_pcie,
        },
        {
                .compatible = "renesas,r8a779f0-pcie-ep",
                .data = &platdata_r8a779f0_pcie_ep,
        },
        {
                .compatible = "renesas,rcar-gen4-pcie",	/* for r8a779[gh]0 */
                .data = &platdata_rcar_gen4_pcie,
        },
        {
                .compatible = "renesas,rcar-gen4-pcie-ep",	/* for r8a779[gh]0 */
                .data = &platdata_rcar_gen4_pcie_ep,
        },
---
Also it will not break existing all r8a779f0 SoC environment because
the compatible is
                         compatible = "renesas,r8a779f0-pcie",
                                     "renesas,rcar-gen4-pcie";

However, if I changed the implementation like your suggestion,
it will be 6 entries like below:
---
static const struct of_device_id rcar_gen4_pcie_of_match[] = {
        {
                .compatible = "renesas,r8a779g0-pcie",
                .data = &platdata_r8a779g0_pcie,
        },
        {
                .compatible = "renesas,r8a779g0-pcie-ep",
                .data = &platdata_r8a779g0_pcie_ep,
        },
        {
                .compatible = "renesas,r8a779h0-pcie",
                .data = &platdata_r8a779g0_pcie,		/* We can reuse r8a779g0's one */
        },
        {
                .compatible = "renesas,r8a779h0-pcie-ep",
                .data = &platdata_r8a779g0_pcie_ep, 	/* We can reuse r8a779g0's one */
        },        {
                .compatible = "renesas,rcar-gen4-pcie",
                .data = &platdata_rcar_gen4_pcie,		/* for r8a779f0 */
        },
        {
                .compatible = "renesas,rcar-gen4-pcie-ep",
                .data = &platdata_rcar_gen4_pcie_ep, 	/* for r8a779f0 */
        },
-----

So, I prefer my implementation because code readability is good to me. But, what do you think?

Best regards,
Yoshihiro Shimoda

> - Mani
> 
> > 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 da2821d6efce..47ec394885f5 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_platdata {
> > +	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->platdata->ltssm_enable) {
> > +		ret = rcar->platdata->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_platdata platdata_r8a779f0_pcie = {
> > +	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
> > +	.mode = DW_PCIE_RC_TYPE,
> > +};
> > +
> > +static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
> > +	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
> > +	.mode = DW_PCIE_EP_TYPE,
> > +};
> > +
> >  static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie = {
> >  	.mode = DW_PCIE_RC_TYPE,
> >  };
> > @@ -513,6 +536,14 @@ static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie_ep = {
> >  };
> >
> >  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
> > +	{
> > +		.compatible = "renesas,r8a779f0-pcie",
> > +		.data = &platdata_r8a779f0_pcie,
> > +	},
> > +	{
> > +		.compatible = "renesas,r8a779f0-pcie-ep",
> > +		.data = &platdata_r8a779f0_pcie_ep,
> > +	},
> >  	{
> >  		.compatible = "renesas,rcar-gen4-pcie",
> >  		.data = &platdata_rcar_gen4_pcie,
> > --
> > 2.25.1
> >
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

[PATCH v6 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).

To support r8a779g0, it requires specific firmware.

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 47ec394885f5..a62804674f4e 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_platdata {
+	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_platdata {
 struct rcar_gen4_pcie {
 	struct dw_pcie dw;
 	void __iomem *base;
+	void __iomem *phy_base;
 	struct platform_device *pdev;
 	const struct rcar_gen4_pcie_platdata *platdata;
 };
 #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_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_update_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] | fw->data[(i * 2) + 1] << 8;
+		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_bit(rcar, PRTLGC89, BIT(30)) >= 0)
+				break;
+			if (!(--timeout)) {
+				ret = -ETIMEDOUT;
+				goto exit;
+			}
+			usleep_range(100, 200);
+		} while (1);
+	}
+
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 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_bit(rcar, PRTLGC89, BIT(30));
+			ret |= rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC90, BIT(0));
+			if (ret >= 0)
+				break;
+			if (!(--timeout)) {
+				ret = -ETIMEDOUT;
+				goto exit;
+			}
+			usleep_range(100, 200);
+		} while (1);
+	}
+
+	ret = 0;
+exit:
+	release_firmware(fw);
+
+	return ret;
+}
+
+static int rcar_gen4_pcie_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_update_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->platdata->additional_common_init)
+		rcar->platdata->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->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_platdata platdata_r8a779f0_pcie = {
 	.ltssm_enable = r8a779f0_pcie_ltssm_enable,
 	.mode = DW_PCIE_RC_TYPE,
@@ -528,10 +723,14 @@ static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
 };
 
 static struct rcar_gen4_pcie_platdata platdata_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_platdata platdata_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 v6 6/7] PCI: rcar-gen4: Add support for r8a779g0

[Manivannan Sadhasivam]

On Wed, Apr 10, 2024 at 09:48:31AM +0900, Yoshihiro Shimoda wrote:
> This driver previously supported r8a779f0 (R-Car S4-8). Add support
> for r8a779g0 (R-Car V4H).
> 
> To support r8a779g0, it requires specific firmware.
> 

Add more information about the new SoC. Like features, why firmware is needed,
how it is downloaded/verified etc...

> 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 47ec394885f5..a62804674f4e 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.
> + */

This provides no information at all. So better hardcode them.

> +#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_platdata {
> +	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_platdata {
>  struct rcar_gen4_pcie {
>  	struct dw_pcie dw;
>  	void __iomem *base;
> +	void __iomem *phy_base;
>  	struct platform_device *pdev;
>  	const struct rcar_gen4_pcie_platdata *platdata;
>  };
>  #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);

If you use FIELD_* macros, then the values can be passed sensibly ie., just 0
and 1.

> +}
> +
> +static int rcar_gen4_pcie_reg_check_bit(struct rcar_gen4_pcie *rcar,

rcar_gen4_pcie_reg_check()?

> +					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_update_phy_firmware(struct rcar_gen4_pcie *rcar)
> +{
> +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};

What are these addresses?

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

Is this the PHY firmware or PCIe?

> +	if (ret) {
> +		dev_err(dw->dev, "%s: Requesting firmware failed\n", __func__);

Please, do not print function names in error log.

> +		return ret;
> +	}
> +
> +	for (i = 0; i < (fw->size / 2); i++) {
> +		data = fw->data[i * 2] | fw->data[(i * 2) + 1] << 8;

Well, the usual concat order is:

		data << 8 | data

> +		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_bit(rcar, PRTLGC89, BIT(30)) >= 0)

What is going on here? Please add a comment to make it clear.

> +				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_bit(rcar, PRTLGC89, BIT(30));
> +			ret |= rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC90, BIT(0));
> +			if (ret >= 0)
> +				break;
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +		} while (1);
> +	}
> +
> +	ret = 0;

return 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_update_phy_firmware(rcar);

Updating or downloading the PHY firmware?

> +	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->platdata->additional_common_init)
> +		rcar->platdata->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->base);

rcar->phy_base?

- Mani

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

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

[Yoshihiro Shimoda]

Hello Manivannan,

> From: Manivannan Sadhasivam, Sent: Thursday, April 11, 2024 2:52 AM
> 
> On Wed, Apr 10, 2024 at 09:48:31AM +0900, Yoshihiro Shimoda wrote:
> > This driver previously supported r8a779f0 (R-Car S4-8). Add support
> > for r8a779g0 (R-Car V4H).
> >
> > To support r8a779g0, it requires specific firmware.
> >
> 
> Add more information about the new SoC. Like features, why firmware is needed,
> how it is downloaded/verified etc...

I got it. I'll add such descriptions.

> > 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 47ec394885f5..a62804674f4e 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.
> > + */
> 
> This provides no information at all. So better hardcode them.

Hmm, Bjorn suggested this instead of hardcode:
---
Make up your own #defines for the offsets.  That would be better than
magic hex offsets below.
https://lore.kernel.org/linux-pci/20240326204842.GA1493890@bhelgaas/
---

I don't have any preference about the defines and hardcoded.

> > +#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_platdata {
> > +	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_platdata {
> >  struct rcar_gen4_pcie {
> >  	struct dw_pcie dw;
> >  	void __iomem *base;
> > +	void __iomem *phy_base;
> >  	struct platform_device *pdev;
> >  	const struct rcar_gen4_pcie_platdata *platdata;
> >  };
> >  #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);
> 
> If you use FIELD_* macros, then the values can be passed sensibly ie., just 0
> and 1.

I got it. I'll use FIELD_* macros.

> > +}
> > +
> > +static int rcar_gen4_pcie_reg_check_bit(struct rcar_gen4_pcie *rcar,
> 
> rcar_gen4_pcie_reg_check()?

I'll rename the function.

> > +					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_update_phy_firmware(struct rcar_gen4_pcie *rcar)
> > +{
> > +	const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
> 
> What are these addresses?

These are also hardcoded addresses on the manual...

> > +	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);
> 
> Is this the PHY firmware or PCIe?

The PHY firmware.

> > +	if (ret) {
> > +		dev_err(dw->dev, "%s: Requesting firmware failed\n", __func__);
> 
> Please, do not print function names in error log.

Bjorn suggested this:
---
It looks like a failure here leads to a probe failure, so I think this
needs a diagnostic message so the user has a hint about what went
wrong.
https://lore.kernel.org/linux-pci/20240326204842.GA1493890@bhelgaas/
---

I don't have any preference about with or without error log here.

> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < (fw->size / 2); i++) {
> > +		data = fw->data[i * 2] | fw->data[(i * 2) + 1] << 8;
> 
> Well, the usual concat order is:
> 
> 		data << 8 | data

I got it. I'll fix it.

> > +		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_bit(rcar, PRTLGC89, BIT(30)) >= 0)
> 
> What is going on here? Please add a comment to make it clear.

Unfortunately, the manual describes a flowchart only.

> > +				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_bit(rcar, PRTLGC89, BIT(30));
> > +			ret |= rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC90, BIT(0));
> > +			if (ret >= 0)
> > +				break;
> > +			if (!(--timeout)) {
> > +				ret = -ETIMEDOUT;
> > +				goto exit;
> > +			}
> > +			usleep_range(100, 200);
> > +		} while (1);
> > +	}
> > +
> > +	ret = 0;
> 
> return 0

IIUC, calling release_firmware(fw) is required here.

> > +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_update_phy_firmware(rcar);
> 
> Updating or downloading the PHY firmware?

I think downloading the PHY firmware is better. So, I'll rename the function.

> > +	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->platdata->additional_common_init)
> > +		rcar->platdata->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->base);
> 
> rcar->phy_base?

Oops. I'll fix it.

Best regards,
Yoshihiro Shimoda

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

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

[Yoshihiro Shimoda]

Hello Manivannan again,

> From: Yoshihiro Shimoda, Sent: Thursday, April 11, 2024 6:10 PM
> > From: Manivannan Sadhasivam, Sent: Thursday, April 11, 2024 2:52 AM
> > On Wed, Apr 10, 2024 at 09:48:31AM +0900, Yoshihiro Shimoda wrote:
...
> > > +static void rcar_gen4_pcie_phy_reg_update_bits(struct rcar_gen4_pcie *rcar,
> > > +					       u32 offset, u32 mask, u32 val)
> > > +{
> > > +	u32 tmp;
> > > +
> > > +	tmp = readl(rcar->phy_base + offset);
> > > +	tmp &= ~mask;
> > > +	tmp |= val;
> > > +	writel(tmp, rcar->phy_base + offset);
> >
> > If you use FIELD_* macros, then the values can be passed sensibly ie., just 0
> > and 1.
> 
> I got it. I'll use FIELD_* macros.

When I modified the code like below, build error happened.
Is the code below your expectation?
---
        tmp = readl(rcar->phy_base + offset);
        tmp &= ~mask;
        tmp |= FIELD_PREP(mask, val);
        writel(tmp, rcar->phy_base + offset);
---
drivers/pci/controller/dwc/pcie-rcar-gen4.c: In function 'rcar_gen4_pcie_phy_reg_update_bits':
././include/linux/compiler_types.h:449:45: error: call to '__compiletime_assert_408' declared with attribute error: FIELD_PREP: mask is not constant
  449 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
      |
---

It seemed that we cannot use a variable in the first argument of FIELD_PREP().

Best regards,
Yoshihiro Shimoda

[PATCH v6 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>
---
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..3c8a0afad91d 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,
 };
 
+/* Don't need to add a new entry if you can use existing entry to test */
 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 v6 7/7] misc: pci_endpoint_test: Document a policy about adding pci_device_id

[Manivannan Sadhasivam]

On Wed, Apr 10, 2024 at 09:48:32AM +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>

One comment below. With that addressed,

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..3c8a0afad91d 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,
>  };
>  
> +/* Don't need to add a new entry if you can use existing entry to test */

'Do not add a new entry if the controller can use existing VID:PID combinations'

- Mani

>  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
> 
> 

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