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

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


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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.7.4

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

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 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 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.

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>
---
 drivers/pci/controller/pcie-rcar.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 40d8c54..d470ab8 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -91,6 +91,7 @@
 #define  LINK_SPEED_2_5GTS	(1 << 16)
 #define  LINK_SPEED_5_0GTS	(2 << 16)
 #define MACCTLR			0x011058
+#define  MACCTLR_INIT_VAL	0x80ff0000
 #define  SPEED_CHANGE		BIT(24)
 #define  SCRAMBLE_DISABLE	BIT(27)
 #define PMSR			0x01105c
@@ -613,6 +614,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 +1238,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 1/2] Revert "PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()"

[Greg KH]

On Wed, Oct 30, 2019 at 08:27:03PM +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.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/pcie-rcar.c | 4 ----
>  1 file changed, 4 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

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

[Eugeniu Rosca]

Hello Shimoda-san, hi Geert, hi Sergei,

On Wed, Oct 30, 2019 at 08:27:04PM +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.
> 
> 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>
> ---
>  drivers/pci/controller/pcie-rcar.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 40d8c54..d470ab8 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -91,6 +91,7 @@
>  #define  LINK_SPEED_2_5GTS	(1 << 16)
>  #define  LINK_SPEED_5_0GTS	(2 << 16)
>  #define MACCTLR			0x011058
> +#define  MACCTLR_INIT_VAL	0x80ff0000

Actually, I do believe there is an inconsistency in the manual,
since the following statements pretty much contradict one another:

1. (as quoted by Shimoda-san from "Initial Setting of PCI Express")
   > Be sure to write the initial value (= H'80FF 0000) to MACCTLR
   > before enabling PCIETCTLR.CFINIT.
2. Description of SPCHG bit in "54.2.98 MAC Control Register (MACCTLR)"
   > Only writing 1 is valid and writing 0 is invalid.

The last "invalid" sounds like "bad things can happen" aka "expect
undefined behaviors" when SPCHG is written "0".

While leaving the decision on the patch inclusion to the maintainers, I
hope, in the long run, Renesas can resolve the documentation conflict
with the HW team and the tech writers.

>  #define  SPEED_CHANGE		BIT(24)
>  #define  SCRAMBLE_DISABLE	BIT(27)
>  #define PMSR			0x01105c
> @@ -613,6 +614,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 +1238,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);
>  }

-- 
Best Regards,
Eugeniu

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

[Yoshihiro Shimoda]

Hello Eugeniu-san,

> From: Eugeniu Rosca, Sent: Thursday, October 31, 2019 1:35 AM
<snip>
> > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> > index 40d8c54..d470ab8 100644
> > --- a/drivers/pci/controller/pcie-rcar.c
> > +++ b/drivers/pci/controller/pcie-rcar.c
> > @@ -91,6 +91,7 @@
> >  #define  LINK_SPEED_2_5GTS	(1 << 16)
> >  #define  LINK_SPEED_5_0GTS	(2 << 16)
> >  #define MACCTLR			0x011058
> > +#define  MACCTLR_INIT_VAL	0x80ff0000
> 
> Actually, I do believe there is an inconsistency in the manual,
> since the following statements pretty much contradict one another:
> 
> 1. (as quoted by Shimoda-san from "Initial Setting of PCI Express")
>    > Be sure to write the initial value (= H'80FF 0000) to MACCTLR
>    > before enabling PCIETCTLR.CFINIT.
> 2. Description of SPCHG bit in "54.2.98 MAC Control Register (MACCTLR)"
>    > Only writing 1 is valid and writing 0 is invalid.
> 
> The last "invalid" sounds like "bad things can happen" aka "expect
> undefined behaviors" when SPCHG is written "0".

I asked the hardware team about the "invalid" in the SPCHG bit and then
this "invalid" means "ignored", not "prohibited". So, even if we write
the SPCHG to 0, no bad things/undefined behaviors happen.

> While leaving the decision on the patch inclusion to the maintainers, I
> hope, in the long run, Renesas can resolve the documentation conflict
> with the HW team and the tech writers.

So, I don't think the documentation conflict exists about the MACCTLR register.
But, what do you think?

Best regards,
Yoshihiro Shimoda

> >  #define  SPEED_CHANGE		BIT(24)
> >  #define  SCRAMBLE_DISABLE	BIT(27)
> >  #define PMSR			0x01105c
> > @@ -613,6 +614,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 +1238,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);
> >  }
> 
> --
> Best Regards,
> Eugeniu

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

[Yoshihiro Shimoda]

Hello again,

> From: Yoshihiro Shimoda, Sent: Friday, November 1, 2019 2:08 PM
<snip>
> Hello Eugeniu-san,
> 
> > From: Eugeniu Rosca, Sent: Thursday, October 31, 2019 1:35 AM
> <snip>
> > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> > > index 40d8c54..d470ab8 100644
> > > --- a/drivers/pci/controller/pcie-rcar.c
> > > +++ b/drivers/pci/controller/pcie-rcar.c
> > > @@ -91,6 +91,7 @@
> > >  #define  LINK_SPEED_2_5GTS	(1 << 16)
> > >  #define  LINK_SPEED_5_0GTS	(2 << 16)
> > >  #define MACCTLR			0x011058
> > > +#define  MACCTLR_INIT_VAL	0x80ff0000
> >
> > Actually, I do believe there is an inconsistency in the manual,
> > since the following statements pretty much contradict one another:
> >
> > 1. (as quoted by Shimoda-san from "Initial Setting of PCI Express")
> >    > Be sure to write the initial value (= H'80FF 0000) to MACCTLR
> >    > before enabling PCIETCTLR.CFINIT.
> > 2. Description of SPCHG bit in "54.2.98 MAC Control Register (MACCTLR)"
> >    > Only writing 1 is valid and writing 0 is invalid.
> >
> > The last "invalid" sounds like "bad things can happen" aka "expect
> > undefined behaviors" when SPCHG is written "0".
> 
> I asked the hardware team about the "invalid" in the SPCHG bit and then
> this "invalid" means "ignored", not "prohibited". So, even if we write
> the SPCHG to 0, no bad things/undefined behaviors happen.
> 
> > While leaving the decision on the patch inclusion to the maintainers, I
> > hope, in the long run, Renesas can resolve the documentation conflict
> > with the HW team and the tech writers.
> 
> So, I don't think the documentation conflict exists about the MACCTLR register.
> But, what do you think?

JFYI, I have submitted v2 patch series which was described about the SPCHG:
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=196557

Best regards,
Yoshihiro Shimoda

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

[Eugeniu Rosca]

Hello Shimoda-san,

On Fri, Nov 01, 2019 at 06:39:03AM +0000, Yoshihiro Shimoda wrote:
> Hello again,
> 
> > From: Yoshihiro Shimoda, Sent: Friday, November 1, 2019 2:08 PM
> <snip>
> > Hello Eugeniu-san,
> > 
> > > From: Eugeniu Rosca, Sent: Thursday, October 31, 2019 1:35 AM
> > <snip>
> > > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> > > > index 40d8c54..d470ab8 100644
> > > > --- a/drivers/pci/controller/pcie-rcar.c
> > > > +++ b/drivers/pci/controller/pcie-rcar.c
> > > > @@ -91,6 +91,7 @@
> > > >  #define  LINK_SPEED_2_5GTS	(1 << 16)
> > > >  #define  LINK_SPEED_5_0GTS	(2 << 16)
> > > >  #define MACCTLR			0x011058
> > > > +#define  MACCTLR_INIT_VAL	0x80ff0000
> > >
> > > Actually, I do believe there is an inconsistency in the manual,
> > > since the following statements pretty much contradict one another:
> > >
> > > 1. (as quoted by Shimoda-san from "Initial Setting of PCI Express")
> > >    > Be sure to write the initial value (= H'80FF 0000) to MACCTLR
> > >    > before enabling PCIETCTLR.CFINIT.
> > > 2. Description of SPCHG bit in "54.2.98 MAC Control Register (MACCTLR)"
> > >    > Only writing 1 is valid and writing 0 is invalid.
> > >
> > > The last "invalid" sounds like "bad things can happen" aka "expect
> > > undefined behaviors" when SPCHG is written "0".
> > 
> > I asked the hardware team about the "invalid" in the SPCHG bit and then
> > this "invalid" means "ignored", not "prohibited". So, even if we write
> > the SPCHG to 0, no bad things/undefined behaviors happen.
> > 
> > > While leaving the decision on the patch inclusion to the maintainers, I
> > > hope, in the long run, Renesas can resolve the documentation conflict
> > > with the HW team and the tech writers.
> > 
> > So, I don't think the documentation conflict exists about the MACCTLR register.
> > But, what do you think?
> 
> JFYI, I have submitted v2 patch series which was described about the SPCHG:
> https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=196557

Thanks for clarifying the exact meaning of the "invalid" wording in the
description of the SPCHG bit so promptly and for stressing that in v2.
Greatly appreciated from our side.

-- 
Best Regards,
Eugeniu

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

[Andrew Murray]

On Wed, Oct 30, 2019 at 08:27:04PM +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.
> 
> 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>
> ---
>  drivers/pci/controller/pcie-rcar.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 40d8c54..d470ab8 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -91,6 +91,7 @@
>  #define  LINK_SPEED_2_5GTS	(1 << 16)
>  #define  LINK_SPEED_5_0GTS	(2 << 16)
>  #define MACCTLR			0x011058
> +#define  MACCTLR_INIT_VAL	0x80ff0000

Geert's previous feedback was to avoid using magic numbers such as this. Is it
possible to define the bits you set instead?

Also perhaps Lorenzo has some feedback as if he prefers these two patches to
be squashed together or not, rather than a revert commit.

Thanks,

Andrew Murray

>  #define  SPEED_CHANGE		BIT(24)
>  #define  SCRAMBLE_DISABLE	BIT(27)
>  #define PMSR			0x01105c
> @@ -613,6 +614,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 +1238,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 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Yoshihiro Shimoda]

Hello Andrew-san,

> From: Andrew Murray, Sent: Friday, November 1, 2019 7:46 PM
> 
> On Wed, Oct 30, 2019 at 08:27:04PM +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.
> >
> > 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>
> > ---
> >  drivers/pci/controller/pcie-rcar.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> > index 40d8c54..d470ab8 100644
> > --- a/drivers/pci/controller/pcie-rcar.c
> > +++ b/drivers/pci/controller/pcie-rcar.c
> > @@ -91,6 +91,7 @@
> >  #define  LINK_SPEED_2_5GTS	(1 << 16)
> >  #define  LINK_SPEED_5_0GTS	(2 << 16)
> >  #define MACCTLR			0x011058
> > +#define  MACCTLR_INIT_VAL	0x80ff0000
> 
> Geert's previous feedback was to avoid using magic numbers such as this. Is it
> possible to define the bits you set instead?

Oops, you're correct. I'll fix it.

> Also perhaps Lorenzo has some feedback as if he prefers these two patches to
> be squashed together or not, rather than a revert commit.

I got it. At the moment, I'll update this series as v3.

Best regards,
Yoshihiro Shimoda

> Thanks,
> 
> Andrew Murray
> 
> >  #define  SPEED_CHANGE		BIT(24)
> >  #define  SCRAMBLE_DISABLE	BIT(27)
> >  #define PMSR			0x01105c
> > @@ -613,6 +614,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 +1238,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
> >