[PATCH v2 0/6] PCI: dwc: rcar-gen4: Add R-Car V4H support

[PATCH v2 0/6] PCI: dwc: rcar-gen4: Add R-Car V4H support

[Yoshihiro Shimoda]

The pcie-rcar-gen4 driver can reuse other R-Car Gen4 support. However,
some initializing settings differs 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 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 (6):
  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: dwc: rcar-gen4: Add a new function pointer for other SoC support
  PCI: dwc: rcar-gen4: Add support for other R-Car Gen4 PCIe controller
  misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

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

-- 
2.25.1

[PATCH v2 1/6] 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 v2 2/6] 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 v2 3/6] 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

[PATCH v2 4/6] PCI: dwc: rcar-gen4: Add a new function pointer for other SoC support

[Yoshihiro Shimoda]

This driver can reuse other R-Car Gen4 SoC support. However, some
initializing settings differs between r8a779f0 and others. So, add
a new function pointer start_link_enable() to support other R-Car
Gen4 SoC in the future. No behavior changes.

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

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index 0be760ed420b..a37613dd9ff4 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -53,9 +53,16 @@ struct rcar_gen4_pcie {
 	void __iomem *base;
 	struct platform_device *pdev;
 	enum dw_pcie_device_mode mode;
+
+	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
 };
 #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
 
+struct rcar_gen4_pcie_platdata {
+	enum dw_pcie_device_mode mode;
+	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
+};
+
 /* Common */
 static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar,
 					bool enable)
@@ -123,9 +130,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->start_link_enable) {
+		ret = rcar->start_link_enable(rcar);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * Require direct speed change with retrying here if the link_gen is
@@ -437,7 +448,10 @@ 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);
+	const struct rcar_gen4_pcie_platdata *pd = of_device_get_match_data(&rcar->pdev->dev);
+
+	rcar->mode = pd->mode;
+	rcar->start_link_enable = pd->start_link_enable;
 
 	switch (rcar->mode) {
 	case DW_PCIE_RC_TYPE:
@@ -500,14 +514,47 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
 	rcar_gen4_pcie_unprepare(rcar);
 }
 
+static int r8a779f0_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
+{
+	rcar_gen4_pcie_ltssm_enable(rcar, true);
+
+	return 0;
+}
+
+static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie = {
+	.mode = DW_PCIE_RC_TYPE,
+	.start_link_enable = r8a779f0_pcie_start_link_enable,
+};
+
+static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
+	.mode = DW_PCIE_EP_TYPE,
+	.start_link_enable = r8a779f0_pcie_start_link_enable,
+};
+
+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,r8a779f0-pcie",
+		.data = &platdata_r8a779f0_pcie,
+	},
+	{
+		.compatible = "renesas,r8a779f0-pcie-ep",
+		.data = &platdata_r8a779f0_pcie_ep,
+	},
 	{
 		.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 v2 4/6] PCI: dwc: rcar-gen4: Add a new function pointer for other SoC support

[Bjorn Helgaas]

Include the function pointer name in the subject so it's a little more
specific.

On Tue, Mar 26, 2024 at 11:45:38AM +0900, Yoshihiro Shimoda wrote:
> This driver can reuse other R-Car Gen4 SoC support. However, some
> initializing settings differs between r8a779f0 and others. So, add
> a new function pointer start_link_enable() to support other R-Car
> Gen4 SoC in the future. No behavior changes.

Make it clear here what the new SoC is.  I think it's r8a779f0, but
you have to read the patch and look for the new .compatible string to
figure that out.

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 57 +++++++++++++++++++--
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index 0be760ed420b..a37613dd9ff4 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -53,9 +53,16 @@ struct rcar_gen4_pcie {
>  	void __iomem *base;
>  	struct platform_device *pdev;
>  	enum dw_pcie_device_mode mode;
> +
> +	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
>  };
>  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
>  
> +struct rcar_gen4_pcie_platdata {
> +	enum dw_pcie_device_mode mode;
> +	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);

I think it's confusing to repeat "mode" and "start_link_enable" in
both rcar_gen4_pcie and rcar_gen4_pcie_platdata.  I know several other
drivers use this pattern, but I think it is simpler overall to just
save the pointer directly, e.g.,

  imx6_pcie_probe
    imx6_pcie->drvdata = of_device_get_match_data(dev);

  ls_pcie_probe
    pcie->drvdata = of_device_get_match_data(dev);

  tegra_pcie_dw_probe
    data = of_device_get_match_data(dev);
    pcie->of_data = (struct tegra_pcie_dw_of_data *)data;

So I think the best thing would be to add struct
rcar_gen4_pcie_platdata, *move* rcar_gen4_pcie.mode there, and save a
pointer to the rcar_gen4_pcie_platdata in struct rcar_gen4_pcie.

That could be its own separate patch, which is nice on its own because
it gets rid of the (void *) casts in rcar_gen4_pcie_of_match[].

Then add .start_link_enable() (or .ltssm_enable(), see below) and the
r8a779f0 bits in another patch.

> +};
> +
>  /* Common */
>  static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar,
>  					bool enable)
> @@ -123,9 +130,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->start_link_enable) {
> +		ret = rcar->start_link_enable(rcar);

This looks basically like what qcom does:

  qcom_pcie_start_link
    if (pcie->cfg->ops->ltssm_enable)
      pcie->cfg->ops->ltssm_enable(pcie)

Can you copy that and use the same name for the pointer and function
name (.ltssm_enable, .*_ltssm_enable())?

> +		if (ret)
> +			return ret;
> +	}
>  
>  	/*
>  	 * Require direct speed change with retrying here if the link_gen is
> @@ -437,7 +448,10 @@ 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);
> +	const struct rcar_gen4_pcie_platdata *pd = of_device_get_match_data(&rcar->pdev->dev);
> +
> +	rcar->mode = pd->mode;
> +	rcar->start_link_enable = pd->start_link_enable;
>  
>  	switch (rcar->mode) {
>  	case DW_PCIE_RC_TYPE:
> @@ -500,14 +514,47 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
>  	rcar_gen4_pcie_unprepare(rcar);
>  }
>  
> +static int r8a779f0_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
> +{
> +	rcar_gen4_pcie_ltssm_enable(rcar, true);

Previously we called rcar_gen4_pcie_ltssm_enable() for
"renesas,rcar-gen4-pcie" and "renesas,rcar-gen4-pcie-ep".  But after
this patch, it looks like we only call it for "renesas,r8a779f0-pcie"
and "renesas,r8a779f0-pcie-ep"?

> +
> +	return 0;
> +}
> +
> +static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie = {
> +	.mode = DW_PCIE_RC_TYPE,
> +	.start_link_enable = r8a779f0_pcie_start_link_enable,
> +};
> +
> +static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
> +	.mode = DW_PCIE_EP_TYPE,
> +	.start_link_enable = r8a779f0_pcie_start_link_enable,
> +};
> +
> +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,r8a779f0-pcie",
> +		.data = &platdata_r8a779f0_pcie,
> +	},
> +	{
> +		.compatible = "renesas,r8a779f0-pcie-ep",
> +		.data = &platdata_r8a779f0_pcie_ep,
> +	},
>  	{
>  		.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 v2 4/6] PCI: dwc: rcar-gen4: Add a new function pointer for other SoC support

[Yoshihiro Shimoda]

Hi Bjorn,

> From: Bjorn Helgaas, Sent: Wednesday, March 27, 2024 5:21 AM
> 
> Include the function pointer name in the subject so it's a little more
> specific.

I got it. I'll change the subject.

> On Tue, Mar 26, 2024 at 11:45:38AM +0900, Yoshihiro Shimoda wrote:
> > This driver can reuse other R-Car Gen4 SoC support. However, some
> > initializing settings differs between r8a779f0 and others. So, add
> > a new function pointer start_link_enable() to support other R-Car
> > Gen4 SoC in the future. No behavior changes.
> 
> Make it clear here what the new SoC is.  I think it's r8a779f0, but
> you have to read the patch and look for the new .compatible string to
> figure that out.

I got it. The new SoC is r8a779g0. So, I should add such description here.

> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 57 +++++++++++++++++++--
> >  1 file changed, 52 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index 0be760ed420b..a37613dd9ff4 100644
> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > @@ -53,9 +53,16 @@ struct rcar_gen4_pcie {
> >  	void __iomem *base;
> >  	struct platform_device *pdev;
> >  	enum dw_pcie_device_mode mode;
> > +
> > +	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
> >  };
> >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> >
> > +struct rcar_gen4_pcie_platdata {
> > +	enum dw_pcie_device_mode mode;
> > +	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
> 
> I think it's confusing to repeat "mode" and "start_link_enable" in
> both rcar_gen4_pcie and rcar_gen4_pcie_platdata.  I know several other
> drivers use this pattern, but I think it is simpler overall to just
> save the pointer directly, e.g.,
> 
>   imx6_pcie_probe
>     imx6_pcie->drvdata = of_device_get_match_data(dev);
> 
>   ls_pcie_probe
>     pcie->drvdata = of_device_get_match_data(dev);
> 
>   tegra_pcie_dw_probe
>     data = of_device_get_match_data(dev);
>     pcie->of_data = (struct tegra_pcie_dw_of_data *)data;
> 
> So I think the best thing would be to add struct
> rcar_gen4_pcie_platdata, *move* rcar_gen4_pcie.mode there, and save a
> pointer to the rcar_gen4_pcie_platdata in struct rcar_gen4_pcie.

I got it. I'll modify the patch.

> That could be its own separate patch, which is nice on its own because
> it gets rid of the (void *) casts in rcar_gen4_pcie_of_match[].
> 
> Then add .start_link_enable() (or .ltssm_enable(), see below) and the
> r8a779f0 bits in another patch.

I got it. I'll make such a patch at first.

> > +};
> > +
> >  /* Common */
> >  static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar,
> >  					bool enable)
> > @@ -123,9 +130,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->start_link_enable) {
> > +		ret = rcar->start_link_enable(rcar);
> 
> This looks basically like what qcom does:
> 
>   qcom_pcie_start_link
>     if (pcie->cfg->ops->ltssm_enable)
>       pcie->cfg->ops->ltssm_enable(pcie)
> 
> Can you copy that and use the same name for the pointer and function
> name (.ltssm_enable, .*_ltssm_enable())?

Yes, I can. I'll rename the pointer and function.

> > +		if (ret)
> > +			return ret;
> > +	}
> >
> >  	/*
> >  	 * Require direct speed change with retrying here if the link_gen is
> > @@ -437,7 +448,10 @@ 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);
> > +	const struct rcar_gen4_pcie_platdata *pd = of_device_get_match_data(&rcar->pdev->dev);
> > +
> > +	rcar->mode = pd->mode;
> > +	rcar->start_link_enable = pd->start_link_enable;
> >
> >  	switch (rcar->mode) {
> >  	case DW_PCIE_RC_TYPE:
> > @@ -500,14 +514,47 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
> >  	rcar_gen4_pcie_unprepare(rcar);
> >  }
> >
> > +static int r8a779f0_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
> > +{
> > +	rcar_gen4_pcie_ltssm_enable(rcar, true);
> 
> Previously we called rcar_gen4_pcie_ltssm_enable() for
> "renesas,rcar-gen4-pcie" and "renesas,rcar-gen4-pcie-ep".  But after
> this patch, it looks like we only call it for "renesas,r8a779f0-pcie"
> and "renesas,r8a779f0-pcie-ep"?

Yes.

Best regards,
Yoshihiro Shimoda

> > +
> > +	return 0;
> > +}
> > +
> > +static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie = {
> > +	.mode = DW_PCIE_RC_TYPE,
> > +	.start_link_enable = r8a779f0_pcie_start_link_enable,
> > +};
> > +
> > +static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
> > +	.mode = DW_PCIE_EP_TYPE,
> > +	.start_link_enable = r8a779f0_pcie_start_link_enable,
> > +};
> > +
> > +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,r8a779f0-pcie",
> > +		.data = &platdata_r8a779f0_pcie,
> > +	},
> > +	{
> > +		.compatible = "renesas,r8a779f0-pcie-ep",
> > +		.data = &platdata_r8a779f0_pcie_ep,
> > +	},
> >  	{
> >  		.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 v2 5/6] PCI: dwc: rcar-gen4: Add support for other R-Car Gen4 PCIe controller

[Yoshihiro Shimoda]

The PCIe controllers of R-Car V4H (r8a779g0) and one more SoC require
different initializing settings than R-Car S4-8 (r8a779f0). So, add
specific functions for them as "renesas,rcar-gen4-pcie{-ep}" compatible.

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

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index a37613dd9ff4..7f3b5e9ca405 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,33 +40,179 @@
 #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
 
+#define RCAR_GEN4_PCIE_FIRMEARE_NAME		"rcar_gen4_pcie.bin"
+#define RCAR_GEN4_PCIE_FIRMEARE_BASE_ADDR	0xc000
+
 struct rcar_gen4_pcie {
 	struct dw_pcie dw;
 	void __iomem *base;
+	/*
+	 * The R-Car Gen4 documents don't describe the PHY registers' name.
+	 * But, the initialization procedure describes these offsets. So,
+	 * this driver has "phy_base + magical offset number" for it.
+	 */
+	void __iomem *phy_base;
 	struct platform_device *pdev;
 	enum dw_pcie_device_mode mode;
 
 	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
+	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
 };
 #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
 
 struct rcar_gen4_pcie_platdata {
 	enum dw_pcie_device_mode mode;
 	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
+	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
 };
 
 /* 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_FIRMEARE_NAME, dw->dev);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < (fw->size / 2); i++) {
+		data = fw->data[i * 2] | fw->data[(i * 2) + 1] << 8;
+		timeout = 100;
+retry_data:
+		dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMEARE_BASE_ADDR + i);
+		dw_pcie_writel_dbi(dw, PRTLGC90, data);
+		if (rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC89, BIT(30)) < 0) {
+			if (!(--timeout)) {
+				ret = -ETIMEDOUT;
+				goto exit;
+			}
+			usleep_range(100, 200);
+			goto retry_data;
+		}
+	}
+
+	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
+
+	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
+		timeout = 100;
+retry_check:
+		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) {
+			if (!(--timeout)) {
+				ret = -ETIMEDOUT;
+				goto exit;
+			}
+			usleep_range(100, 200);
+			goto retry_check;
+		}
+	}
+
+	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, 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_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_enable(struct rcar_gen4_pcie *rcar,
 					bool enable)
 {
@@ -201,6 +350,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
 	if (ret)
 		goto err_unprepare;
 
+	if (rcar->additional_common_init)
+		rcar->additional_common_init(rcar);
+
 	return 0;
 
 err_unprepare:
@@ -242,6 +394,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");
 
@@ -452,6 +608,7 @@ static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar)
 
 	rcar->mode = pd->mode;
 	rcar->start_link_enable = pd->start_link_enable;
+	rcar->additional_common_init = pd->additional_common_init;
 
 	switch (rcar->mode) {
 	case DW_PCIE_RC_TYPE:
@@ -521,6 +678,31 @@ static int r8a779f0_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
 	return 0;
 }
 
+static int rcar_gen4_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
+{
+	return rcar_gen4_pcie_enable_phy(rcar);
+}
+
+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 struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie = {
 	.mode = DW_PCIE_RC_TYPE,
 	.start_link_enable = r8a779f0_pcie_start_link_enable,
@@ -533,10 +715,14 @@ static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
 
 static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie = {
 	.mode = DW_PCIE_RC_TYPE,
+	.start_link_enable = rcar_gen4_pcie_start_link_enable,
+	.additional_common_init = rcar_gen4_pcie_additional_common_init,
 };
 
 static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie_ep = {
 	.mode = DW_PCIE_EP_TYPE,
+	.start_link_enable = rcar_gen4_pcie_start_link_enable,
+	.additional_common_init = rcar_gen4_pcie_additional_common_init,
 };
 
 static const struct of_device_id rcar_gen4_pcie_of_match[] = {
-- 
2.25.1

Re: [PATCH v2 5/6] PCI: dwc: rcar-gen4: Add support for other R-Car Gen4 PCIe controller

[Bjorn Helgaas]

The subject line should specify which controller(s) this adds support
for.

On Tue, Mar 26, 2024 at 11:45:39AM +0900, Yoshihiro Shimoda wrote:
> The PCIe controllers of R-Car V4H (r8a779g0) and one more SoC require
> different initializing settings than R-Car S4-8 (r8a779f0). So, add
> specific functions for them as "renesas,rcar-gen4-pcie{-ep}" compatible.

I can't tell from this what's being added.  This should say something
like "this driver previously supported r8....  Add support for r8...."
so it's clear what was existing and what is new.

Hmm... the first use of request_firmware() in drivers/pci/.  That
warrants a mention here as it's a pretty significant change.

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 188 +++++++++++++++++++-
>  1 file changed, 187 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index a37613dd9ff4..7f3b5e9ca405 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,33 +40,179 @@
>  #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
>  
> +#define RCAR_GEN4_PCIE_FIRMEARE_NAME		"rcar_gen4_pcie.bin"
> +#define RCAR_GEN4_PCIE_FIRMEARE_BASE_ADDR	0xc000

s/FIRMEARE/FIRMWARE/

>  struct rcar_gen4_pcie {
>  	struct dw_pcie dw;
>  	void __iomem *base;
> +	/*
> +	 * The R-Car Gen4 documents don't describe the PHY registers' name.
> +	 * But, the initialization procedure describes these offsets. So,
> +	 * this driver has "phy_base + magical offset number" for it.

Make up your own #defines for the offsets.  That would be better than
magic hex offsets below.

> +	void __iomem *phy_base;
>  	struct platform_device *pdev;
>  	enum dw_pcie_device_mode mode;
>  
>  	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
> +	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
>  };
>  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
>  
>  struct rcar_gen4_pcie_platdata {
>  	enum dw_pcie_device_mode mode;
>  	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
> +	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
>  };
>  
>  /* 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_FIRMEARE_NAME, dw->dev);
> +	if (ret)
> +		return ret;

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.

> +	for (i = 0; i < (fw->size / 2); i++) {
> +		data = fw->data[i * 2] | fw->data[(i * 2) + 1] << 8;
> +		timeout = 100;
> +retry_data:
> +		dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMEARE_BASE_ADDR + i);
> +		dw_pcie_writel_dbi(dw, PRTLGC90, data);
> +		if (rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC89, BIT(30)) < 0) {
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +			goto retry_data;
> +		}
> +	}
> +
> +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
> +
> +	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
> +		timeout = 100;
> +retry_check:
> +		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) {
> +			if (!(--timeout)) {
> +				ret = -ETIMEDOUT;
> +				goto exit;
> +			}
> +			usleep_range(100, 200);
> +			goto retry_check;
> +		}
> +	}
> +
> +	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, 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_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_enable(struct rcar_gen4_pcie *rcar,
>  					bool enable)
>  {
> @@ -201,6 +350,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
>  	if (ret)
>  		goto err_unprepare;
>  
> +	if (rcar->additional_common_init)
> +		rcar->additional_common_init(rcar);
>
>  	return 0;
>  
>  err_unprepare:
> @@ -242,6 +394,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);

I don't get it.  This imposes a new requirement (presence of "phy"
resource) on the existing SoCs.  That doesn't sound right.

>  	/* Renesas-specific registers */
>  	rcar->base = devm_platform_ioremap_resource_byname(rcar->pdev, "app");
>  
> @@ -452,6 +608,7 @@ static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar)
>  
>  	rcar->mode = pd->mode;
>  	rcar->start_link_enable = pd->start_link_enable;
> +	rcar->additional_common_init = pd->additional_common_init;
>  
>  	switch (rcar->mode) {
>  	case DW_PCIE_RC_TYPE:
> @@ -521,6 +678,31 @@ static int r8a779f0_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
>  	return 0;
>  }
>  
> +static int rcar_gen4_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
> +{
> +	return rcar_gen4_pcie_enable_phy(rcar);
> +}
> +
> +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);

I don't get this either.  You do this "additional_common_init" part
only for the existing "renesas,rcar-gen4-pcie" and
"renesas,rcar-gen4-pcie-ep", but PCIE_PORT_LANE_SKEW and
PCIEPWRMNGCTRL do not appear in the driver prior to these patches.  I
must be missing something.  Or this is backwards and you meant to do
this for the *new* SoC?

If you need to limit some functionality to existing SoCs and add new
functionality for new SoCs, do those in separate patches if you can.

> +}
> +
>  static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie = {
>  	.mode = DW_PCIE_RC_TYPE,
>  	.start_link_enable = r8a779f0_pcie_start_link_enable,
> @@ -533,10 +715,14 @@ static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
>  
>  static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie = {
>  	.mode = DW_PCIE_RC_TYPE,
> +	.start_link_enable = rcar_gen4_pcie_start_link_enable,
> +	.additional_common_init = rcar_gen4_pcie_additional_common_init,
>  };
>  
>  static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie_ep = {
>  	.mode = DW_PCIE_EP_TYPE,
> +	.start_link_enable = rcar_gen4_pcie_start_link_enable,
> +	.additional_common_init = rcar_gen4_pcie_additional_common_init,
>  };
>  
>  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
> -- 
> 2.25.1
> 

RE: [PATCH v2 5/6] PCI: dwc: rcar-gen4: Add support for other R-Car Gen4 PCIe controller

[Yoshihiro Shimoda]

Hi Bjorn,

> From: Bjorn Helgaas, Sent: Wednesday, March 27, 2024 5:49 AM
> 
> The subject line should specify which controller(s) this adds support
> for.

I got it. I'll change the subject.

> On Tue, Mar 26, 2024 at 11:45:39AM +0900, Yoshihiro Shimoda wrote:
> > The PCIe controllers of R-Car V4H (r8a779g0) and one more SoC require
> > different initializing settings than R-Car S4-8 (r8a779f0). So, add
> > specific functions for them as "renesas,rcar-gen4-pcie{-ep}" compatible.
> 
> I can't tell from this what's being added.  This should say something
> like "this driver previously supported r8....  Add support for r8...."
> so it's clear what was existing and what is new.

I got it. I'll modify the description.

> Hmm... the first use of request_firmware() in drivers/pci/.  That
> warrants a mention here as it's a pretty significant change.

I got it. I'll add such a description.

> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 188 +++++++++++++++++++-
> >  1 file changed, 187 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index a37613dd9ff4..7f3b5e9ca405 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,33 +40,179 @@
> >  #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
> >
> > +#define RCAR_GEN4_PCIE_FIRMEARE_NAME		"rcar_gen4_pcie.bin"
> > +#define RCAR_GEN4_PCIE_FIRMEARE_BASE_ADDR	0xc000
> 
> s/FIRMEARE/FIRMWARE/

Oops. I'll fix it.

> >  struct rcar_gen4_pcie {
> >  	struct dw_pcie dw;
> >  	void __iomem *base;
> > +	/*
> > +	 * The R-Car Gen4 documents don't describe the PHY registers' name.
> > +	 * But, the initialization procedure describes these offsets. So,
> > +	 * this driver has "phy_base + magical offset number" for it.
> 
> Make up your own #defines for the offsets.  That would be better than
> magic hex offsets below.

I got it. I'll add #defines for it.

> > +	void __iomem *phy_base;
> >  	struct platform_device *pdev;
> >  	enum dw_pcie_device_mode mode;
> >
> >  	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
> > +	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
> >  };
> >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> >
> >  struct rcar_gen4_pcie_platdata {
> >  	enum dw_pcie_device_mode mode;
> >  	int (*start_link_enable)(struct rcar_gen4_pcie *rcar);
> > +	void (*additional_common_init)(struct rcar_gen4_pcie *rcar);
> >  };
> >
> >  /* 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_FIRMEARE_NAME, dw->dev);
> > +	if (ret)
> > +		return ret;
> 
> 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.

I got it. I'll add such a code here.

> > +	for (i = 0; i < (fw->size / 2); i++) {
> > +		data = fw->data[i * 2] | fw->data[(i * 2) + 1] << 8;
> > +		timeout = 100;
> > +retry_data:
> > +		dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMEARE_BASE_ADDR + i);
> > +		dw_pcie_writel_dbi(dw, PRTLGC90, data);
> > +		if (rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC89, BIT(30)) < 0) {
> > +			if (!(--timeout)) {
> > +				ret = -ETIMEDOUT;
> > +				goto exit;
> > +			}
> > +			usleep_range(100, 200);
> > +			goto retry_data;
> > +		}
> > +	}
> > +
> > +	rcar_gen4_pcie_phy_reg_update_bits(rcar, 0x0f8, BIT(17), BIT(17));
> > +
> > +	for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
> > +		timeout = 100;
> > +retry_check:
> > +		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) {
> > +			if (!(--timeout)) {
> > +				ret = -ETIMEDOUT;
> > +				goto exit;
> > +			}
> > +			usleep_range(100, 200);
> > +			goto retry_check;
> > +		}
> > +	}
> > +
> > +	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, 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_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_enable(struct rcar_gen4_pcie *rcar,
> >  					bool enable)
> >  {
> > @@ -201,6 +350,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
> >  	if (ret)
> >  		goto err_unprepare;
> >
> > +	if (rcar->additional_common_init)
> > +		rcar->additional_common_init(rcar);
> >
> >  	return 0;
> >
> >  err_unprepare:
> > @@ -242,6 +394,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);
> 
> I don't get it.  This imposes a new requirement (presence of "phy"
> resource) on the existing SoCs.  That doesn't sound right.

According to the dt-binding doc, the existing SoCs are also required for the "phy".
That's why I didn't add any condition to simplify the code.

> >  	/* Renesas-specific registers */
> >  	rcar->base = devm_platform_ioremap_resource_byname(rcar->pdev, "app");
> >
> > @@ -452,6 +608,7 @@ static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar)
> >
> >  	rcar->mode = pd->mode;
> >  	rcar->start_link_enable = pd->start_link_enable;
> > +	rcar->additional_common_init = pd->additional_common_init;
> >
> >  	switch (rcar->mode) {
> >  	case DW_PCIE_RC_TYPE:
> > @@ -521,6 +678,31 @@ static int r8a779f0_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
> >  	return 0;
> >  }
> >
> > +static int rcar_gen4_pcie_start_link_enable(struct rcar_gen4_pcie *rcar)
> > +{
> > +	return rcar_gen4_pcie_enable_phy(rcar);
> > +}
> > +
> > +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);
> 
> I don't get this either.  You do this "additional_common_init" part
> only for the existing "renesas,rcar-gen4-pcie" and
> "renesas,rcar-gen4-pcie-ep", but PCIE_PORT_LANE_SKEW and
> PCIEPWRMNGCTRL do not appear in the driver prior to these patches.  I
> must be missing something.  Or this is backwards and you meant to do
> this for the *new* SoC?

I'm sorry for the confusion. This is for the new SoC.

I should have explained before though, existing support SoC is:
- r8a779f0 as "renesas,rcar-gen4-pcie" and "renesas,rcar-gen4-pcie-ep".

After we applied the patch series:

- r8a779f0 as "renesas,r8a779f0-pcie" and "renesas,r8a779f0-pcie-ep".
- r8a779g0 as "renesas,rcar-gen4-pcie" and "renesas,rcar-gen4--pcie-ep".

Also, I have a plan to add r8a779h0 support in the future:

- r8a779f0 as "renesas,r8a779f0-pcie" and "renesas,r8a779f0-pcie-ep".
- r8a779g0 as "renesas,rcar-gen4-pcie" and "renesas,rcar-gen4--pcie-ep".
- r8a779h0 as "renesas,rcar-gen4-pcie" and "renesas,rcar-gen4--pcie-ep".

And r8a779[gh]0 need this additional_common_init.

> If you need to limit some functionality to existing SoCs and add new
> functionality for new SoCs, do those in separate patches if you can.

I got it. I'll make such a patch if I can.

Best regards,
Yoshihiro Shimoda

> > +}
> > +
> >  static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie = {
> >  	.mode = DW_PCIE_RC_TYPE,
> >  	.start_link_enable = r8a779f0_pcie_start_link_enable,
> > @@ -533,10 +715,14 @@ static struct rcar_gen4_pcie_platdata platdata_r8a779f0_pcie_ep = {
> >
> >  static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie = {
> >  	.mode = DW_PCIE_RC_TYPE,
> > +	.start_link_enable = rcar_gen4_pcie_start_link_enable,
> > +	.additional_common_init = rcar_gen4_pcie_additional_common_init,
> >  };
> >
> >  static struct rcar_gen4_pcie_platdata platdata_rcar_gen4_pcie_ep = {
> >  	.mode = DW_PCIE_EP_TYPE,
> > +	.start_link_enable = rcar_gen4_pcie_start_link_enable,
> > +	.additional_common_init = rcar_gen4_pcie_additional_common_init,
> >  };
> >
> >  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
> > --
> > 2.25.1
> >

Re: [PATCH v2 5/6] PCI: dwc: rcar-gen4: Add support for other R-Car Gen4 PCIe controller

[Bjorn Helgaas]

On Wed, Mar 27, 2024 at 05:32:57AM +0000, Yoshihiro Shimoda wrote:
> > From: Bjorn Helgaas, Sent: Wednesday, March 27, 2024 5:49 AM

> > >  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);
> > 
> > I don't get it.  This imposes a new requirement (presence of "phy"
> > resource) on the existing SoCs.  That doesn't sound right.
> 
> According to the dt-binding doc, the existing SoCs are also required
> for the "phy".  That's why I didn't add any condition to simplify
> the code.

Is there anything that enforces that?  Is it possible that DTs exist
in the field without it?  We don't want to break any existing setup.

RE: [PATCH v2 5/6] PCI: dwc: rcar-gen4: Add support for other R-Car Gen4 PCIe controller

[Yoshihiro Shimoda]

Hi Bjorn,

> From: Bjorn Helgaas, Sent: Thursday, March 28, 2024 3:15 AM
> 
> On Wed, Mar 27, 2024 at 05:32:57AM +0000, Yoshihiro Shimoda wrote:
> > > From: Bjorn Helgaas, Sent: Wednesday, March 27, 2024 5:49 AM
> 
> > > >  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);
> > >
> > > I don't get it.  This imposes a new requirement (presence of "phy"
> > > resource) on the existing SoCs.  That doesn't sound right.
> >
> > According to the dt-binding doc, the existing SoCs are also required
> > for the "phy".  That's why I didn't add any condition to simplify
> > the code.
> 
> Is there anything that enforces that?  Is it possible that DTs exist
> in the field without it?  We don't want to break any existing setup.

Using make dtbs_check can detect an error if the "phy" doesn't exist like below:

/home/shimoda/development/linux/linux/arch/arm64/boot/dts/renesas/r8a779f0-spider.dtb: pcie@e65d0000: reg-names:5: 'phy' was expected
        from schema $id: http://devicetree.org/schemas/pci/rcar-gen4-pci-host.yaml#

So, I believe that this can enforce that in review process at least.
Now arch/arm64/boot/dts/renesas/r8a779f0.dtsi has the pcie compatible,
and all pcie nodes in the dtsi have "phy". So, this patch will not break any existing setup.

Best regards,
Yoshihiro Shimoda

[PATCH v2 6/6] misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

[Yoshihiro Shimoda]

Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
can be used for testing PCIe EP on R-Car V4H.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 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..2fa3c6473c7d 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -83,6 +83,7 @@
 #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
 #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
 #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
+#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
 
 static DEFINE_IDA(pci_endpoint_test_ida);
 
@@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
 	  .driver_data = (kernel_ulong_t)&default_data,
 	},
+	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
+	  .driver_data = (kernel_ulong_t)&default_data,
+	},
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
 	  .driver_data = (kernel_ulong_t)&j721e_data,
 	},
-- 
2.25.1

Re: [PATCH v2 6/6] misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

[Frank Li]

On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> can be used for testing PCIe EP on R-Car V4H.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  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..2fa3c6473c7d 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -83,6 +83,7 @@
>  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
>  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
>  #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
> +#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
>  
>  static DEFINE_IDA(pci_endpoint_test_ida);
>  
> @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
>  	  .driver_data = (kernel_ulong_t)&default_data,
>  	},
> +	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> +	  .driver_data = (kernel_ulong_t)&default_data,
> +	},

You use default_data, why need new device_id? I think you can use 0x0031
to do test.

Frank

>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
>  	  .driver_data = (kernel_ulong_t)&j721e_data,
>  	},
> -- 
> 2.25.1
> 

RE: [PATCH v2 6/6] misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

[Yoshihiro Shimoda]

Hi Frank,

> From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
 
> On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > can be used for testing PCIe EP on R-Car V4H.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  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..2fa3c6473c7d 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -83,6 +83,7 @@
> >  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
> >  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
> >  #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
> > +#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
> >
> >  static DEFINE_IDA(pci_endpoint_test_ida);
> >
> > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> >  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> >  	  .driver_data = (kernel_ulong_t)&default_data,
> >  	},
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > +	  .driver_data = (kernel_ulong_t)&default_data,
> > +	},
> 
> You use default_data, why need new device_id? I think you can use 0x0031
> to do test.

I thought we can add a new device_id freely like other devices.
Since the PCIe controller's endpoint mode can configure the device id,
I can use 0x0031 to do test though.

If such a reusable entry exists, is adding a new device id into the driver prohibited?

Best regards,
Yoshihiro Shimoda

> Frank
> 
> >  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
> >  	  .driver_data = (kernel_ulong_t)&j721e_data,
> >  	},
> > --
> > 2.25.1
> >

Re: [PATCH v2 6/6] misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

[Frank Li]

On Tue, Mar 26, 2024 at 05:47:23AM +0000, Yoshihiro Shimoda wrote:
> Hi Frank,
> 
> > From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
>  
> > On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > > can be used for testing PCIe EP on R-Car V4H.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > >  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..2fa3c6473c7d 100644
> > > --- a/drivers/misc/pci_endpoint_test.c
> > > +++ b/drivers/misc/pci_endpoint_test.c
> > > @@ -83,6 +83,7 @@
> > >  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
> > >  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
> > >  #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
> > > +#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
> > >
> > >  static DEFINE_IDA(pci_endpoint_test_ida);
> > >
> > > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> > >  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> > >  	  .driver_data = (kernel_ulong_t)&default_data,
> > >  	},
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > > +	  .driver_data = (kernel_ulong_t)&default_data,
> > > +	},
> > 
> > You use default_data, why need new device_id? I think you can use 0x0031
> > to do test.
> 
> I thought we can add a new device_id freely like other devices.
> Since the PCIe controller's endpoint mode can configure the device id,
> I can use 0x0031 to do test though.
> 
> If such a reusable entry exists, is adding a new device id into the driver prohibited?

I just think it is not necessary. This list will become longer and longer.
And difference device id can't help us at all. 

We should use difference production as difference functions, or difference
configuration.  Such as usb gadget product id, we use 0x4545 for all mass
storage. 

Using difference devices id for difference function, such as 0x31 for
ep_test 0x30 for virtual net, 0x29 for virtual console ...

Or using difference devices id indicate some features. For example, use
0x30 means support write to EP MSI ITS to trigger irq.

Donate a device_id to more valuable things.

Frank

> 
> Best regards,
> Yoshihiro Shimoda
> 
> > Frank
> > 
> > >  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
> > >  	  .driver_data = (kernel_ulong_t)&j721e_data,
> > >  	},
> > > --
> > > 2.25.1
> > >

RE: [PATCH v2 6/6] misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

[Yoshihiro Shimoda]

Hi Frank,

> From: Frank Li, Sent: Tuesday, March 26, 2024 11:09 PM
> 
> On Tue, Mar 26, 2024 at 05:47:23AM +0000, Yoshihiro Shimoda wrote:
> > Hi Frank,
> >
> > > From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
> >
> > > On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > > > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > > > can be used for testing PCIe EP on R-Car V4H.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > > >  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..2fa3c6473c7d 100644
> > > > --- a/drivers/misc/pci_endpoint_test.c
> > > > +++ b/drivers/misc/pci_endpoint_test.c
> > > > @@ -83,6 +83,7 @@
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
> > > > +#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
> > > >
> > > >  static DEFINE_IDA(pci_endpoint_test_ida);
> > > >
> > > > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> > > >  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> > > >  	  .driver_data = (kernel_ulong_t)&default_data,
> > > >  	},
> > > > +	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > > > +	  .driver_data = (kernel_ulong_t)&default_data,
> > > > +	},
> > >
> > > You use default_data, why need new device_id? I think you can use 0x0031
> > > to do test.
> >
> > I thought we can add a new device_id freely like other devices.
> > Since the PCIe controller's endpoint mode can configure the device id,
> > I can use 0x0031 to do test though.
> >
> > If such a reusable entry exists, is adding a new device id into the driver prohibited?
> 
> I just think it is not necessary. This list will become longer and longer.
> And difference device id can't help us at all.

I agreed. To record it, I'll make a patch to add such description in the pci_endpoint_test.c.

> We should use difference production as difference functions, or difference
> configuration.  Such as usb gadget product id, we use 0x4545 for all mass
> storage.

I see.

> Using difference devices id for difference function, such as 0x31 for
> ep_test 0x30 for virtual net, 0x29 for virtual console ...
> 
> Or using difference devices id indicate some features. For example, use
> 0x30 means support write to EP MSI ITS to trigger irq.
> 
> Donate a device_id to more valuable things.

I think so.

Best regards,
Yoshihiro Shimoda

> Frank
> 
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> > > Frank
> > >
> > > >  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
> > > >  	  .driver_data = (kernel_ulong_t)&j721e_data,
> > > >  	},
> > > > --
> > > > 2.25.1
> > > >

Re: [PATCH v2 6/6] misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

[Geert Uytterhoeven]

Hi Shimoda-san,

On Tue, Mar 26, 2024 at 6:47 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
> > On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > > can be used for testing PCIe EP on R-Car V4H.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > >  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..2fa3c6473c7d 100644
> > > --- a/drivers/misc/pci_endpoint_test.c
> > > +++ b/drivers/misc/pci_endpoint_test.c
> > > @@ -83,6 +83,7 @@
> > >  #define PCI_DEVICE_ID_RENESAS_R8A774C0             0x002d
> > >  #define PCI_DEVICE_ID_RENESAS_R8A774E1             0x0025
> > >  #define PCI_DEVICE_ID_RENESAS_R8A779F0             0x0031
> > > +#define PCI_DEVICE_ID_RENESAS_R8A779G0             0x0030
> > >
> > >  static DEFINE_IDA(pci_endpoint_test_ida);
> > >
> > > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> > >     { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> > >       .driver_data = (kernel_ulong_t)&default_data,
> > >     },
> > > +   { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > > +     .driver_data = (kernel_ulong_t)&default_data,
> > > +   },
> >
> > You use default_data, why need new device_id? I think you can use 0x0031
> > to do test.
>
> I thought we can add a new device_id freely like other devices.
> Since the PCIe controller's endpoint mode can configure the device id,
> I can use 0x0031 to do test though.

Can it? The documentation for the PCICONF0Fi register states both the
DEVICE_ID and VENDOR_ID bits are read-only.

Gr{oetje,eeting}s,

                        Geert

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

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

RE: [PATCH v2 6/6] misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Wednesday, March 27, 2024 5:12 PM
> 
> Hi Shimoda-san,
> 
> On Tue, Mar 26, 2024 at 6:47 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
> > > On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > > > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > > > can be used for testing PCIe EP on R-Car V4H.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > > >  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..2fa3c6473c7d 100644
> > > > --- a/drivers/misc/pci_endpoint_test.c
> > > > +++ b/drivers/misc/pci_endpoint_test.c
> > > > @@ -83,6 +83,7 @@
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A774C0             0x002d
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A774E1             0x0025
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A779F0             0x0031
> > > > +#define PCI_DEVICE_ID_RENESAS_R8A779G0             0x0030
> > > >
> > > >  static DEFINE_IDA(pci_endpoint_test_ida);
> > > >
> > > > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> > > >     { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> > > >       .driver_data = (kernel_ulong_t)&default_data,
> > > >     },
> > > > +   { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > > > +     .driver_data = (kernel_ulong_t)&default_data,
> > > > +   },
> > >
> > > You use default_data, why need new device_id? I think you can use 0x0031
> > > to do test.
> >
> > I thought we can add a new device_id freely like other devices.
> > Since the PCIe controller's endpoint mode can configure the device id,
> > I can use 0x0031 to do test though.
> 
> Can it?

Yes, the following function can write the device id in endpoint mode.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-ep.c?h=v6.9-rc1#n108

> The documentation for the PCICONF0Fi register states both the
> DEVICE_ID and VENDOR_ID bits are read-only.

You're correct. The documentation (R-Car V4H hardware manual) said these bits are read-only.
However, actual hardware IP seems to allow writing this register if the write protect is disabled in endpoint mode.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds