[PATCH v4 0/2] PCI: rcar: Fix missing MACCTLR register setting (take2)

[PATCH v4 0/2] PCI: rcar: Fix missing MACCTLR register setting (take2)

[Yoshihiro Shimoda]

This patch series is based on the latest pci/rcar branch of Lorenzo's pci.git.
The commit 175cc093888e ("PCI: rcar: Fix missing MACCTLR register setting
in rcar_pcie_hw_init()") description/code don't follow the manual
accurately, so that it's difficult to understand. So, this patch
series reverts the commit at first, and then applies a new fixed patch.

Reference:
https://marc.info/?l=linux-renesas-soc&m=157242422327368&w=2

Changes from v3:
 - Add Geet-san's Acked-by into the patch 1/2.
 - Fix a #define name in the patch 2/2.
 - Remove reduntancy comment from the patch 2/2.
 - Add Geet-san's Reviewed-by into the patch 2/2.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=197851

Changes from v2:
 - Rebase on the latest pcir/rcar branch.
 - Add Euguniu-san's Reported-by in the patch 2/2.
 - Add the bits definitions instead of magic number to make the initial value
   so that I don't add Euguniu-san's Reviewed-by tag in the patch 2/2.
 https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=196557

Changes from v1:
 - Follow -stable rule in patch 1/2.
 - Add some comments about SPCHG bit of MACCTLR register.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=195717

Yoshihiro Shimoda (2):
  Revert "PCI: rcar: Fix missing MACCTLR register setting in
    rcar_pcie_hw_init()"
  PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

 drivers/pci/controller/pcie-rcar.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH v4 1/2] Revert "PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()"

[Yoshihiro Shimoda]

This reverts commit 175cc093888ee74a17c4dd5f99ba9a6bc86de5be.

The commit description/code don't follow the manual accurately,
it's difficult to understand. So, this patch reverts the commit.

Fixes: 175cc093888e ("PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()"
Cc: <stable@vger.kernel.org>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pci/controller/pcie-rcar.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 0dadccb..40d8c54 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -91,7 +91,6 @@
 #define  LINK_SPEED_2_5GTS	(1 << 16)
 #define  LINK_SPEED_5_0GTS	(2 << 16)
 #define MACCTLR			0x011058
-#define  MACCTLR_RESERVED	BIT(0)
 #define  SPEED_CHANGE		BIT(24)
 #define  SCRAMBLE_DISABLE	BIT(27)
 #define PMSR			0x01105c
@@ -614,8 +613,6 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		rcar_pci_write_reg(pcie, 0x801f0000, PCIEMSITXR);
 
-	rcar_rmw32(pcie, MACCTLR, MACCTLR_RESERVED, 0);
-
 	/* Finish initialization - establish a PCI Express link */
 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
 
@@ -1238,7 +1235,6 @@ static int rcar_pcie_resume_noirq(struct device *dev)
 		return 0;
 
 	/* Re-establish the PCIe link */
-	rcar_rmw32(pcie, MACCTLR, MACCTLR_RESERVED, 0);
 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
 	return rcar_pcie_wait_for_dl(pcie);
 }
-- 
2.7.4

[PATCH v4 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Yoshihiro Shimoda]

According to the R-Car Gen2/3 manual, "Be sure to write the initial
value (= H'80FF 0000) to MACCTLR before enabling PCIETCTLR.CFINIT".
To avoid unexpected behaviors, this patch fixes it. Note that
the SPCHG bit of MACCTLR register description said "Only writing 1
is valid and writing 0 is invalid" but this "invalid" means
"ignored", not "prohibited". So, any documentation conflict doesn't
exist about writing the MACCTLR register.

Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver")
Fixes: be20bbcb0a8c ("PCI: rcar: Add the initialization of PCIe link in resume_noirq()")
Cc: <stable@vger.kernel.org> # v5.2+
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pci/controller/pcie-rcar.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 40d8c54..94ba4fe 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -91,8 +91,11 @@
 #define  LINK_SPEED_2_5GTS	(1 << 16)
 #define  LINK_SPEED_5_0GTS	(2 << 16)
 #define MACCTLR			0x011058
+#define  MACCTLR_NFTS_MASK	GENMASK(23, 16)	/* The name is from SH7786 */
 #define  SPEED_CHANGE		BIT(24)
 #define  SCRAMBLE_DISABLE	BIT(27)
+#define  LTSMDIS		BIT(31)
+#define  MACCTLR_INIT_VAL	(LTSMDIS | MACCTLR_NFTS_MASK)
 #define PMSR			0x01105c
 #define MACS2R			0x011078
 #define MACCGSPSETR		0x011084
@@ -613,6 +616,8 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		rcar_pci_write_reg(pcie, 0x801f0000, PCIEMSITXR);
 
+	rcar_pci_write_reg(pcie, MACCTLR_INIT_VAL, MACCTLR);
+
 	/* Finish initialization - establish a PCI Express link */
 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
 
@@ -1235,6 +1240,7 @@ static int rcar_pcie_resume_noirq(struct device *dev)
 		return 0;
 
 	/* Re-establish the PCIe link */
+	rcar_pci_write_reg(pcie, MACCTLR_INIT_VAL, MACCTLR);
 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
 	return rcar_pcie_wait_for_dl(pcie);
 }
-- 
2.7.4

Re: [PATCH v4 1/2] Revert "PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()"

[Lorenzo Pieralisi]

On Tue, Nov 05, 2019 at 07:51:28PM +0900, Yoshihiro Shimoda wrote:
> This reverts commit 175cc093888ee74a17c4dd5f99ba9a6bc86de5be.
> 
> The commit description/code don't follow the manual accurately,
> it's difficult to understand. So, this patch reverts the commit.
> 
> Fixes: 175cc093888e ("PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()"

This patch is not in the mainline, I will just drop it.

> Cc: <stable@vger.kernel.org>

This is valid for any fix: there is no reason to send to stable a fix
for a patch that is not in the mainline yet.

Lorenzo

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/pci/controller/pcie-rcar.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 0dadccb..40d8c54 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -91,7 +91,6 @@
>  #define  LINK_SPEED_2_5GTS	(1 << 16)
>  #define  LINK_SPEED_5_0GTS	(2 << 16)
>  #define MACCTLR			0x011058
> -#define  MACCTLR_RESERVED	BIT(0)
>  #define  SPEED_CHANGE		BIT(24)
>  #define  SCRAMBLE_DISABLE	BIT(27)
>  #define PMSR			0x01105c
> @@ -614,8 +613,6 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		rcar_pci_write_reg(pcie, 0x801f0000, PCIEMSITXR);
>  
> -	rcar_rmw32(pcie, MACCTLR, MACCTLR_RESERVED, 0);
> -
>  	/* Finish initialization - establish a PCI Express link */
>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>  
> @@ -1238,7 +1235,6 @@ static int rcar_pcie_resume_noirq(struct device *dev)
>  		return 0;
>  
>  	/* Re-establish the PCIe link */
> -	rcar_rmw32(pcie, MACCTLR, MACCTLR_RESERVED, 0);
>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>  	return rcar_pcie_wait_for_dl(pcie);
>  }
> -- 
> 2.7.4
> 

Re: [PATCH v4 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Lorenzo Pieralisi]

Never ever add stable@vger.kernel.org in the CC list of the email
header. You should add the tag in the commit log as you did but
never CC stable when sending the patch email.

On Tue, Nov 05, 2019 at 07:51:29PM +0900, Yoshihiro Shimoda wrote:
> According to the R-Car Gen2/3 manual, "Be sure to write the initial
> value (= H'80FF 0000) to MACCTLR before enabling PCIETCTLR.CFINIT".
> To avoid unexpected behaviors, this patch fixes it. Note that
> the SPCHG bit of MACCTLR register description said "Only writing 1
> is valid and writing 0 is invalid" but this "invalid" means
> "ignored", not "prohibited". So, any documentation conflict doesn't
> exist about writing the MACCTLR register.

I am sorry but I don't understand what you mean, if either you or
any rcar maintainer can help me rewrite this log I will merge this
patch then, appreciated.

Thanks,
Lorenzo

> Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver")
> Fixes: be20bbcb0a8c ("PCI: rcar: Add the initialization of PCIe link in resume_noirq()")
> Cc: <stable@vger.kernel.org> # v5.2+
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/pci/controller/pcie-rcar.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 40d8c54..94ba4fe 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -91,8 +91,11 @@
>  #define  LINK_SPEED_2_5GTS	(1 << 16)
>  #define  LINK_SPEED_5_0GTS	(2 << 16)
>  #define MACCTLR			0x011058
> +#define  MACCTLR_NFTS_MASK	GENMASK(23, 16)	/* The name is from SH7786 */
>  #define  SPEED_CHANGE		BIT(24)
>  #define  SCRAMBLE_DISABLE	BIT(27)
> +#define  LTSMDIS		BIT(31)
> +#define  MACCTLR_INIT_VAL	(LTSMDIS | MACCTLR_NFTS_MASK)
>  #define PMSR			0x01105c
>  #define MACS2R			0x011078
>  #define MACCGSPSETR		0x011084
> @@ -613,6 +616,8 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		rcar_pci_write_reg(pcie, 0x801f0000, PCIEMSITXR);
>  
> +	rcar_pci_write_reg(pcie, MACCTLR_INIT_VAL, MACCTLR);
> +
>  	/* Finish initialization - establish a PCI Express link */
>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>  
> @@ -1235,6 +1240,7 @@ static int rcar_pcie_resume_noirq(struct device *dev)
>  		return 0;
>  
>  	/* Re-establish the PCIe link */
> +	rcar_pci_write_reg(pcie, MACCTLR_INIT_VAL, MACCTLR);
>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>  	return rcar_pcie_wait_for_dl(pcie);
>  }
> -- 
> 2.7.4
> 

RE: [PATCH v4 1/2] Revert "PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()"

[Yoshihiro Shimoda]

Hi Lorenzo,

> From: Lorenzo Pieralisi, Sent: Monday, November 11, 2019 11:39 PM
> 
> On Tue, Nov 05, 2019 at 07:51:28PM +0900, Yoshihiro Shimoda wrote:
> > This reverts commit 175cc093888ee74a17c4dd5f99ba9a6bc86de5be.
> >
> > The commit description/code don't follow the manual accurately,
> > it's difficult to understand. So, this patch reverts the commit.
> >
> > Fixes: 175cc093888e ("PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()"
> 
> This patch is not in the mainline, I will just drop it.

I got it.

> > Cc: <stable@vger.kernel.org>
> 
> This is valid for any fix: there is no reason to send to stable a fix
> for a patch that is not in the mainline yet.

I understood it. So, I'll drop both tags on v5 patches.

Best regards,
Yoshihiro Shimoda

RE: [PATCH v4 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Yoshihiro Shimoda]

Hi Lorenzo,

> From: Lorenzo Pieralisi, Sent: Monday, November 11, 2019 11:43 PM
> 
> Never ever add stable@vger.kernel.org in the CC list of the email
> header. You should add the tag in the commit log as you did but
> never CC stable when sending the patch email.

Thank you for the pointed it out. I had added stable@vger.kernel.org
in such cases, but I understood it. I will drop stable@vger.kernel.org.

> On Tue, Nov 05, 2019 at 07:51:29PM +0900, Yoshihiro Shimoda wrote:
> > According to the R-Car Gen2/3 manual, "Be sure to write the initial
> > value (= H'80FF 0000) to MACCTLR before enabling PCIETCTLR.CFINIT".
> > To avoid unexpected behaviors, this patch fixes it. Note that
> > the SPCHG bit of MACCTLR register description said "Only writing 1
> > is valid and writing 0 is invalid" but this "invalid" means
> > "ignored", not "prohibited". So, any documentation conflict doesn't
> > exist about writing the MACCTLR register.
> 
> I am sorry but I don't understand what you mean, if either you or
> any rcar maintainer can help me rewrite this log I will merge this
> patch then, appreciated.

I'm sorry. I think I should not drop a sentence "because the bit 0 is
set to 1 on reset" which has the reverted patch from this version. Also,
the note seems to be difficult to understand. So, I'll rewrite this log
like below. Is it acceptable?

---
According to the R-Car Gen2/3 manual, "Be sure to write the initial
value (= H'80FF 0000) to MACCTLR before enabling PCIETCTLR.CFINIT"
because the bit 0 of MACCTLR is set to 1 on reset. To avoid unexpected
behaviors, this patch fixes it.

Note that the SPCHG bit (bit 24) of MACCTLR register description said
"Only writing 1 is valid and writing 0 is invalid", but this "invalid"
means "ignored", not "prohibited". So, even if the driver writes
the SPCHG to 0, there is no problem.
---

Best regards,
Yoshihiro Shimoda

Re: [PATCH v4 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Lorenzo Pieralisi]

[removed CC stable]

On Tue, Nov 12, 2019 at 01:48:03AM +0000, Yoshihiro Shimoda wrote:

[...]

> I'm sorry. I think I should not drop a sentence "because the bit 0 is
> set to 1 on reset" which has the reverted patch from this version. Also,
> the note seems to be difficult to understand. So, I'll rewrite this log
> like below. Is it acceptable?
> 
> ---
> According to the R-Car Gen2/3 manual,

Is this a publicly available manual ? If yes we provide a link, if
not I will reword the sentence below.

> "Be sure to write the initial value (= H'80FF 0000) to MACCTLR before
> enabling PCIETCTLR.CFINIT" because the bit 0 of MACCTLR is set to 1 on
> reset. To avoid unexpected behaviors, this patch fixes it.
> 
> Note that the SPCHG bit (bit 24) of MACCTLR register description said
> "Only writing 1 is valid and writing 0 is invalid", but this "invalid"
> means "ignored", not "prohibited". So, even if the driver writes
> the SPCHG to 0, there is no problem.

I know understand it, let me know if we can add a link to a manual
(plus section/paragraph, etc.), I will rewrite the commit log
accordingly.

Thanks,
Lorenzo

> ---
> 
> Best regards,
> Yoshihiro Shimoda
> 

Re: [PATCH v4 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Geert Uytterhoeven]

Hi Lorenzo,

On Tue, Nov 12, 2019 at 11:27 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, Nov 12, 2019 at 01:48:03AM +0000, Yoshihiro Shimoda wrote:
> > I'm sorry. I think I should not drop a sentence "because the bit 0 is
> > set to 1 on reset" which has the reverted patch from this version. Also,
> > the note seems to be difficult to understand. So, I'll rewrite this log
> > like below. Is it acceptable?
> >
> > ---
> > According to the R-Car Gen2/3 manual,
>
> Is this a publicly available manual ? If yes we provide a link, if
> not I will reword the sentence below.

The same hardware block is present in the RZ/G series, and documented
in RZ/G Series User's Manual: Hardware
https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html#documents
Chapter 39 ("PCIEC").
Section 39.28.98 ("MAC Control Register (MACCTLR)")
Section 39.3.1 ("Initialization"), Paragraph 3 ("Initial Setting of
PCI Express").

Thanks, and happy digesting ;-)

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 v4 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Lorenzo Pieralisi]

On Tue, Nov 12, 2019 at 11:46:42AM +0100, Geert Uytterhoeven wrote:
> Hi Lorenzo,
> 
> On Tue, Nov 12, 2019 at 11:27 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Tue, Nov 12, 2019 at 01:48:03AM +0000, Yoshihiro Shimoda wrote:
> > > I'm sorry. I think I should not drop a sentence "because the bit 0 is
> > > set to 1 on reset" which has the reverted patch from this version. Also,
> > > the note seems to be difficult to understand. So, I'll rewrite this log
> > > like below. Is it acceptable?
> > >
> > > ---
> > > According to the R-Car Gen2/3 manual,
> >
> > Is this a publicly available manual ? If yes we provide a link, if
> > not I will reword the sentence below.
> 
> The same hardware block is present in the RZ/G series, and documented
> in RZ/G Series User's Manual: Hardware
> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html#documents
> Chapter 39 ("PCIEC").
> Section 39.28.98 ("MAC Control Register (MACCTLR)")
> Section 39.3.1 ("Initialization"), Paragraph 3 ("Initial Setting of
> PCI Express").
> 
> Thanks, and happy digesting ;-)

Thank you, updated commit log and pushed out, pci/rcar branch.

Thanks,
Lorenzo

RE: [PATCH v4 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Yoshihiro Shimoda]

Hi Lorenzo,

> From: Lorenzo Pieralisi, Sent: Tuesday, November 12, 2019 8:25 PM
> 
> On Tue, Nov 12, 2019 at 11:46:42AM +0100, Geert Uytterhoeven wrote:
> > Hi Lorenzo,
> >
> > On Tue, Nov 12, 2019 at 11:27 AM Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > > On Tue, Nov 12, 2019 at 01:48:03AM +0000, Yoshihiro Shimoda wrote:
> > > > I'm sorry. I think I should not drop a sentence "because the bit 0 is
> > > > set to 1 on reset" which has the reverted patch from this version. Also,
> > > > the note seems to be difficult to understand. So, I'll rewrite this log
> > > > like below. Is it acceptable?
> > > >
> > > > ---
> > > > According to the R-Car Gen2/3 manual,
> > >
> > > Is this a publicly available manual ? If yes we provide a link, if
> > > not I will reword the sentence below.
> >
> > The same hardware block is present in the RZ/G series, and documented
> > in RZ/G Series User's Manual: Hardware
> > https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html#documents
> > Chapter 39 ("PCIEC").
> > Section 39.28.98 ("MAC Control Register (MACCTLR)")
> > Section 39.3.1 ("Initialization"), Paragraph 3 ("Initial Setting of
> > PCI Express").
> >
> > Thanks, and happy digesting ;-)
> 
> Thank you, updated commit log and pushed out, pci/rcar branch.

Thank you very much for updating the commit log. It's very clear than mine :)

Best regards,
Yoshihiro Shimoda

> Thanks,
> Lorenzo